middleware icon indicating copy to clipboard operation
middleware copied to clipboard

Add Functionality for PRs merged without review

Open samad-yar-khan opened this issue 1 year ago • 22 comments

Why do we need this ?

  • A team may be interested in seeing how many PRs got merged without review in a time duration.
  • This can help in setting processes within the team

Acceptance Criteria

  • [ ] Add Python Backend API to fetch Teams PRs merged without review for a team.
  • [ ] Update Next JS BFF slices and state to fetch and persist a team PRs merged without review.
  • [ ] Add UI component to see PRs merged without review for a team.

Task 1, 2, 3 can be done in seperate PRs.

Further Comments / References

samad-yar-khan avatar Apr 29 '24 10:04 samad-yar-khan

If anyone else is interested in this being a part of the app, please comment or react to the thread. :)

jayantbh avatar Jul 02 '24 12:07 jayantbh

Sir , is this open for work ?

RISHIKESHk07 avatar Jul 18 '24 14:07 RISHIKESHk07

Sure! Do remember that because this will be a visual change, you'll need to clear up the visual changes before implementing them.

We already have an idea of how we want to build this, but you're welcome to pitch your idea of how you see this being built. In case you'd prefer to go with our idea itself, that's totally fine.

Let us know how you want to proceed.

jayantbh avatar Jul 18 '24 14:07 jayantbh

@jayantbh will proceed with your idea and here visual change meant ? , i have not much exp with flask , will work on that and i will have to create a route in pull_requests.py file for fetching the Prs which are not reviewed based on a team_id .

RISHIKESHk07 avatar Jul 18 '24 16:07 RISHIKESHk07

By visual change, I mean that if you see the original issue description, you'll see it talks about a UI change as well. That. 🙂

jayantbh avatar Jul 18 '24 16:07 jayantbh

@jayantbh thanks got it , thought it meant something different

RISHIKESHk07 avatar Jul 18 '24 17:07 RISHIKESHk07

image @jayantbh the settings here is a form of type validation ? , a bit confused with this part any resource i could use to understand it (get_settings service part and type validation process)

RISHIKESHk07 avatar Jul 19 '24 13:07 RISHIKESHk07

Sure, I believe @samad-yar-khan might be able to help here.

jayantbh avatar Jul 19 '24 16:07 jayantbh

image @jayantbh the settings here is a form of type validation ? , a bit confused with this part any resource i could use to understand it (get_settings service part and type validation process)

@RISHIKESHk07 get_settings_service() is a getter that fetches the SettingsService after injecting all the dependencies. Here we are first fetching the SettingsService and then fetching a specific setting using get_setting method. The get_setting method fetches a setting for a type and entity. This entity can be a team, user or org as a whole.

samad-yar-khan avatar Jul 20 '24 08:07 samad-yar-khan

@jayantbh @samad-yar-khan , this is my solution for making a backend route , please correct if i it is flawed (i) create a function merged_not_reviwed in PullRequestAnalyticsService (ii) create function for filtering data , i would find all ids with type "Review " from PullRequestEvent class then use this list to filter out data from the PullRequest class , return the result (iii) create a route ('/teams//prs/not_reviwed_merge') and put the above logic over here .

RISHIKESHk07 avatar Jul 20 '24 14:07 RISHIKESHk07

@jayantbh does the above look fine ?? \

RISHIKESHk07 avatar Jul 22 '24 16:07 RISHIKESHk07

I think it doesn't need to be a separate screen. This could be a small widget thing on one of the existing UIs.

If every metric is on its own page it'll be difficult to get the whole picture from any single point.

jayantbh avatar Jul 22 '24 17:07 jayantbh

Alright that does sound better than a whole page , do we use a component lib for styling ? , does the backend route creation look fine ?

RISHIKESHk07 avatar Jul 22 '24 17:07 RISHIKESHk07

I'll let @samad-yar-khan chime in on the API path.

Visually, it can be shown in the lead time/cycle time sidebar beside the CT/LT breakdown, like this:

image

jayantbh avatar Jul 23 '24 09:07 jayantbh

@samad-yar-khan Made a Pr

RISHIKESHk07 avatar Aug 01 '24 10:08 RISHIKESHk07

@jayantbh @samad-yar-khan any suggestions

RISHIKESHk07 avatar Aug 03 '24 14:08 RISHIKESHk07

Hey @samad-yar-khan , @jayantbh This issue sounds interesting to me, Can I work on the UI part of it ?

iam-vipin avatar Aug 10 '24 12:08 iam-vipin

Hey @VipinDevelops, sure!

jayantbh avatar Aug 15 '24 11:08 jayantbh