Added methods for feature Prs_not_reviwed_Merged in PulllrequestAnalysis Service #495

RISHIKESHk07 · 2024-08-01T10:57:47Z

Linked Issue(s)

#207

Acceptance Criteria fulfillment

Add Python Backend API to fetch Teams PRs merged without review for a team.

Proposed changes (including videos or screenshots)

Added required method in PullrequestAnalysisService & route in the pull_request.py

Added method which takes a team_id as the input , finds the list of org_repos from which we use to query the PullRequest realted tables
Finds the Prs (ids of pull_requests ) which are reviewed ( from the PullRequestEvent ) and then filters the Prs which are not reviewed , and finally filters for the ones which are Merged and returns the count of Prs which are not Reviwed But Merged

CLAassistant · 2024-08-01T10:57:53Z

All committers have signed the CLA.

RISHIKESHk07 · 2024-08-01T11:00:23Z

will clean up the the print statements and add types as well on the next commit

backend/analytics_server/mhq/api/pull_requests.py

backend/analytics_server/mhq/store/repos/code.py

backend/analytics_server/mhq/api/pull_requests.py

backend/analytics_server/mhq/service/code/pr_analytics.py

backend/analytics_server/mhq/store/repos/code.py

jayantbh · 2024-08-07T07:20:01Z

Hey @RISHIKESHk07, have you had a chance to look at this?

RISHIKESHk07 · 2024-08-07T07:46:46Z

Evening @jayantbh , i did look at it , have done the changes except for one doubt , did ping samad about it , the change involved the need of adding interval as argument in the function , any where I could refer similar implementation of using interval , it was used inorder to not call all the prs from the db . It should be in the changes requested above , Thanks for notifying

jayantbh · 2024-08-07T08:44:49Z

Sounds good, @RISHIKESHk07
You can also join our Slack (if you haven't) and ping in the #open-source channel for help.

jayantbh · 2024-08-08T09:05:13Z

@RISHIKESHk07 there's a failing linter check.

RISHIKESHk07 · 2024-08-09T13:35:42Z

@RISHIKESHk07 there's a failing linter check.

i did not run the linting command mostly , where could i find it ?

samad-yar-khan · 2024-08-13T11:06:43Z

@RISHIKESHk07 Try using black version 24.4.2

RISHIKESHk07 · 2024-08-15T05:30:09Z

@RISHIKESHk07 Try using black version 24.4.2

done

jayantbh

Lgtm. But I'll let @samad-yar-khan or @adnanhashmi09 take a final look.

samad-yar-khan · 2024-08-17T09:19:01Z

backend/analytics_server/mhq/api/pull_requests.py

+    Schema(
+        {
+            Required("from_time"): All(str, Coerce(datetime.fromisoformat)),
+            Required("to_time"): All(str, Coerce(datetime.fromisoformat)),
+        }
+    ),


@RISHIKESHk07 we additionally have a PR filter that we use in query params which is missing here. PR filter helps us add more querying from branch, author etc at a api level by just adding handler for the query.

@samad-yar-khan do i need to include pr_filter in the pr_analytics_service ( for prs_merged_without_review ) as well ? , any more info i need to know about this

yes, PR filter is applied at repo, so you will have to pass it down funcs as argument

samad-yar-khan · 2024-08-17T09:32:36Z

backend/analytics_server/mhq/store/repos/code.py

+                PullRequest.created_in_db_at.between(
+                    interval.from_time, interval.to_time
+                )
+            )


@Kamlesh72 created_at_in_db is the timestamp at which the data was stored in our db, which is irrelevant for a user. If they wish to query data in a date range, they are likely searching PRs that were either created or updated in that interval.

@samad-yar-khan created_at & updated_at should fix this

@RISHIKESHk07 there is one more logical bug in this query. Its fetching open PRs as well as merged PRs.

@samad-yar-khan I believe this is for @RISHIKESHk07

@samad-yar-khan querying state for merged should correct that , i did try testing on a team repo but i see only closed & merged state Prs ,but we do have a few open Prs , (selcted pr merge while creating team did i miss anything ??)

that should work but I dont see that in your PR.

samad-yar-khan · 2024-08-17T09:35:26Z

backend/analytics_server/mhq/api/pull_requests.py

+    pr_analytics = get_pr_analytics_service()
+    result = pr_analytics.get_prs_merged_without_review(team.id, interval)
+    prs_map = [pr.id for pr in result]
+    return {"PrsWithoutReviewMerged": prs_map}


I tested out this API. It just returns a list of ids. This is not useful for the front end. We need to send a list of PRs as data to the backend. We are already using a func in the same file in /lead_time/prs API. Please refer the implementation for that API. Should help.

We don't name our property keys that go in API responses in that case either. @RISHIKESHk07

I missed some things.

RISHIKESHk07 · 2024-10-04T14:16:03Z

@samad-yar-khan @jayantbh please take a look at the changes , apologies for the stale period of the PR was occupied with a university matter

samad-yar-khan · 2024-08-21T12:17:26Z

backend/analytics_server/mhq/store/repos/code.py

+                PullRequest.created_in_db_at.between(
+                    interval.from_time, interval.to_time
+                )
+            )


that should work but I dont see that in your PR.

samad-yar-khan · 2024-10-10T10:14:39Z

backend/analytics_server/mhq/api/resources/code_resouces.py

+
+def get_non_paginated_merged_without_review(prs: List[PullRequest]) -> List[Dict[str,any]]:
+    return {
+        "data": []


I think this func has to be deleted ? since you are now using get_non_paginated_pr_response

@samad-yar-khan Was going to implement my own before finding the get_non_paginated_pr_response , and if i use the _filter_prs_merged_in_interval, i would not need to query for merge state explicitly again as it will be handled by this method

samad-yar-khan · 2024-10-10T10:18:57Z

backend/analytics_server/mhq/store/repos/code.py

+            self._db.session.query(PullRequest)
+            .filter(PullRequest.repo_id.in_(AllOrg_ids))
+            .filter(PullRequest.state == PullRequestState.MERGED)
+            .filter(PullRequest.merge_time == None)


Suggested change

.filter(PullRequest.merge_time == None)

.filter(PullRequest.merge_time.is_(None))

samad-yar-khan · 2024-10-10T10:21:56Z

backend/analytics_server/mhq/store/repos/code.py

+                or_(
+                    PullRequest.created_at.between(
+                        interval.from_time, interval.to_time
+                    ),
+                    PullRequest.updated_at.between(
+                        interval.from_time, interval.to_time
+                    ),
+                )


since we are fetching PRs merged without review in interval. I think it would be valuable to look into _filter_prs_merged_in_interval method

@samad-yar-khan will replace it with already implemented method , did see this while scrolling the file

samad-yar-khan · 2024-10-10T10:22:40Z

backend/analytics_server/mhq/store/repos/code.py

+        AllOrg = self.get_team_repos(team_id)
+        AllOrg_ids = [tr.id for tr in AllOrg]


Suggested change

AllOrg = self.get_team_repos(team_id)

AllOrg_ids = [tr.id for tr in AllOrg]

team_repos = self.get_team_repos(team_id)

repo_ids = [team_repo.id for team_repo in team_repos]

variable names should be better and follow similar patterns as other part of the codebase. Having var names like AllOrg and AllOrg_ids makes the code harder to read and do not follow either snake case or camel case. We generally follow snake case in the python backend.

I think we can move the repo fetching part to PullRequestAnalyticsService and the just pass on repo_ids to the Repo. Separates the basic query from business layer and we can also follow the same pattern as get_prs_merged_in_interval

samad-yar-khan · 2024-10-10T10:34:35Z

@samad-yar-khan @jayantbh please take a look at the changes , apologies for the stale period of the PR was occupied with a university matter

@RISHIKESHk07 no worries, can totally understand. There has been significant improvement in the code quality since the previous review. Its 90% there and almost usable 🚀 Keep pushing 💯

…isService Signed-off-by: RISHIKESHk07 <[email protected]>

…d_pr_response

…estAnalyticsService

RISHIKESHk07 · 2024-10-30T16:50:05Z

@samad-yar-khan please take a look if possible

samad-yar-khan requested changes Aug 5, 2024

View reviewed changes

RISHIKESHk07 force-pushed the Feature-Prs_Not_Reviwed_Merged branch from d6bbf30 to 60165f9 Compare August 11, 2024 04:52

RISHIKESHk07 requested a review from samad-yar-khan August 11, 2024 04:55

RISHIKESHk07 force-pushed the Feature-Prs_Not_Reviwed_Merged branch from 60165f9 to 9ba14e2 Compare August 15, 2024 05:26

jayantbh previously approved these changes Aug 15, 2024

View reviewed changes

samad-yar-khan requested changes Aug 17, 2024

View reviewed changes

RISHIKESHk07 force-pushed the Feature-Prs_Not_Reviwed_Merged branch 2 times, most recently from 61ddad8 to 9422021 Compare October 4, 2024 14:13

samad-yar-khan requested changes Oct 10, 2024

View reviewed changes

RISHIKESHk07 added 5 commits October 30, 2024 22:13

Added methods for feature Prs_not_reviwed_Merged in PUllrequestAnalys…

65d36d7

…isService Signed-off-by: RISHIKESHk07 <[email protected]>

Cleaned the function at service layer

3915e71

Formatted with black

9a5cfcd

Added the pr_filter to query params and made response to non_paginate…

2f1637a

…d_pr_response

Optimised the Bussiness Logic function and moved repo_ids to PullRequ…

ac14715

…estAnalyticsService

RISHIKESHk07 force-pushed the Feature-Prs_Not_Reviwed_Merged branch from 9422021 to ac14715 Compare October 30, 2024 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added methods for feature Prs_not_reviwed_Merged in PulllrequestAnalysis Service #495

Added methods for feature Prs_not_reviwed_Merged in PulllrequestAnalysis Service #495

RISHIKESHk07 commented Aug 1, 2024

CLAassistant commented Aug 1, 2024 •

edited

Loading

RISHIKESHk07 commented Aug 1, 2024

jayantbh commented Aug 7, 2024

RISHIKESHk07 commented Aug 7, 2024

jayantbh commented Aug 7, 2024

jayantbh commented Aug 8, 2024

RISHIKESHk07 commented Aug 9, 2024

samad-yar-khan commented Aug 13, 2024

RISHIKESHk07 commented Aug 15, 2024

jayantbh left a comment

samad-yar-khan Aug 17, 2024

RISHIKESHk07 Aug 17, 2024 •

edited

Loading

samad-yar-khan Aug 17, 2024

samad-yar-khan Aug 17, 2024

RISHIKESHk07 Aug 17, 2024

samad-yar-khan Aug 17, 2024 •

edited

Loading

Kamlesh72 Aug 17, 2024

RISHIKESHk07 Aug 17, 2024

samad-yar-khan Aug 21, 2024

samad-yar-khan Aug 17, 2024

jayantbh Aug 17, 2024

RISHIKESHk07 commented Oct 4, 2024

samad-yar-khan Aug 21, 2024

samad-yar-khan Oct 10, 2024

RISHIKESHk07 Oct 13, 2024

samad-yar-khan Oct 10, 2024

samad-yar-khan Oct 10, 2024

RISHIKESHk07 Oct 13, 2024

samad-yar-khan Oct 10, 2024

samad-yar-khan Oct 10, 2024

samad-yar-khan Oct 10, 2024

samad-yar-khan commented Oct 10, 2024

RISHIKESHk07 commented Oct 30, 2024

	.filter(PullRequest.merge_time == None)
	.filter(PullRequest.merge_time.is_(None))

		AllOrg = self.get_team_repos(team_id)
		AllOrg_ids = [tr.id for tr in AllOrg]

Added methods for feature Prs_not_reviwed_Merged in PulllrequestAnalysis Service #495

Are you sure you want to change the base?

Added methods for feature Prs_not_reviwed_Merged in PulllrequestAnalysis Service #495

Conversation

RISHIKESHk07 commented Aug 1, 2024

Linked Issue(s)

Acceptance Criteria fulfillment

Proposed changes (including videos or screenshots)

Added required method in PullrequestAnalysisService & route in the pull_request.py

CLAassistant commented Aug 1, 2024 • edited Loading

RISHIKESHk07 commented Aug 1, 2024

jayantbh commented Aug 7, 2024

RISHIKESHk07 commented Aug 7, 2024

jayantbh commented Aug 7, 2024

jayantbh commented Aug 8, 2024

RISHIKESHk07 commented Aug 9, 2024

samad-yar-khan commented Aug 13, 2024

RISHIKESHk07 commented Aug 15, 2024

jayantbh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RISHIKESHk07 Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samad-yar-khan Aug 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RISHIKESHk07 commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samad-yar-khan commented Oct 10, 2024

RISHIKESHk07 commented Oct 30, 2024

CLAassistant commented Aug 1, 2024 •

edited

Loading

RISHIKESHk07 Aug 17, 2024 •

edited

Loading

samad-yar-khan Aug 17, 2024 •

edited

Loading