-
Notifications
You must be signed in to change notification settings - Fork 147
feat: Backend API for PRs Merged Without Review #691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -167,3 +167,38 @@ def get_team_lead_time_trends( | |||||||||||||
| week.isoformat(): adapt_lead_time_metrics(average_lead_time_metrics) | ||||||||||||||
| for week, average_lead_time_metrics in weekly_lead_time_metrics_avg_map.items() | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| @app.route("/teams/<team_id>/prs/merged_without_review", methods={"GET"}) | ||||||||||||||
| @queryschema( | ||||||||||||||
| Schema({ | ||||||||||||||
| Required("from_time"): All(str, Coerce(datetime.fromisoformat)), | ||||||||||||||
| Required("to_time"): All(str, Coerce(datetime.fromisoformat)), | ||||||||||||||
| Optional("pr_filter"): All(str, Coerce(json.loads)), | ||||||||||||||
| }) | ||||||||||||||
| ) | ||||||||||||||
| def get_prs_merged_without_review( | ||||||||||||||
| team_id: str, | ||||||||||||||
| from_time: datetime, | ||||||||||||||
| to_time: datetime, | ||||||||||||||
| pr_filter: Dict = None, | ||||||||||||||
| ) -> List[PullRequest]: | ||||||||||||||
| query_validator = get_query_validator() | ||||||||||||||
|
|
||||||||||||||
| pr_analytics = get_pr_analytics_service() | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove trailing whitespace. Line 187 has trailing whitespace after the function call. Apply this diff: - pr_analytics = get_pr_analytics_service()
+ pr_analytics = get_pr_analytics_service()📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
|
|
||||||||||||||
| interval: Interval = query_validator.interval_validator(from_time, to_time) | ||||||||||||||
|
|
||||||||||||||
| pr_filter: PRFilter = apply_pr_filter( | ||||||||||||||
| pr_filter, EntityType.TEAM, team_id, [SettingType.EXCLUDED_PRS_SETTING] | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| team_repos = pr_analytics.get_team_repos(team_id) | ||||||||||||||
| if not team_repos: | ||||||||||||||
| return [] | ||||||||||||||
|
Comment on lines
+195
to
+197
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good defensive programming, but clean up trailing whitespace. The early return when no team repos exist is good practice. However, line 197 has trailing whitespace. Apply this diff to remove the trailing whitespace: - return []
+ return []📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
|
|
||||||||||||||
| repo_ids = [repo.id for repo in team_repos] | ||||||||||||||
|
|
||||||||||||||
| prs = pr_analytics.get_prs_merged_without_review(repo_ids, interval, pr_filter) | ||||||||||||||
|
|
||||||||||||||
| repo_id_repo_map = {repo.id: repo for repo in team_repos} | ||||||||||||||
| return get_non_paginated_pr_response(prs, repo_id_repo_map, len(prs)) | ||||||||||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,6 +1,8 @@ | ||||
| from mhq.store.models.code.filter import PRFilter | ||||
| from mhq.store.models.core.teams import Team | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove unused import. The Apply this diff to remove the unused import: -from mhq.store.models.core.teams import Team📝 Committable suggestion
Suggested change
🧰 Tools🪛 Flake8 (7.3.0)[error] 2-2: 'mhq.store.models.core.teams.Team' imported but unused (F401) 🤖 Prompt for AI Agents |
||||
| from mhq.utils.time import Interval | ||||
| from mhq.store.models.code import OrgRepo, PullRequest | ||||
| from mhq.store.repos.code import CodeRepoService | ||||
|
|
||||
| from typing import List, Optional | ||||
|
|
||||
|
|
||||
|
|
@@ -17,6 +19,8 @@ def get_team_repos(self, team_id: str) -> List[OrgRepo]: | |||
| def get_repo_by_id(self, repo_id: str) -> Optional[OrgRepo]: | ||||
| return self.code_repo_service.get_repo_by_id(repo_id) | ||||
|
|
||||
| def get_prs_merged_without_review(self, repo_ids: List[str], interval: Interval, pr_filter: PRFilter) -> List[PullRequest]: | ||||
| return self.code_repo_service.get_prs_merged_without_review(repo_ids, interval, pr_filter) | ||||
|
|
||||
| def get_pr_analytics_service(): | ||||
| return PullRequestAnalyticsService(CodeRepoService()) | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,8 +2,8 @@ | |
| from operator import and_ | ||
| from typing import Optional, List | ||
|
|
||
| from mhq.store.models.code.enums import CodeProvider | ||
| from sqlalchemy import or_ | ||
| from mhq.store.models.code.enums import CodeProvider, PullRequestEventType | ||
| from sqlalchemy import or_, not_, exists | ||
| from sqlalchemy.orm import defer | ||
| from mhq.store.models.core import Team | ||
|
|
||
|
|
@@ -283,6 +283,35 @@ def get_active_org_repos_by_ids(self, repo_ids: List[str]) -> List[OrgRepo]: | |
| .all() | ||
| ) | ||
|
|
||
| @rollback_on_exc | ||
| def get_prs_merged_without_review( | ||
| self, | ||
| repo_ids: List[str], | ||
| interval: Interval, | ||
| pr_filter: PRFilter = None, | ||
| ) -> List[PullRequest]: | ||
| query = self._db.session.query(PullRequest).options(defer(PullRequest.data)) | ||
|
|
||
| query = self._filter_prs_by_repo_ids(query, repo_ids) | ||
| query = self._filter_prs_merged_in_interval(query, interval) | ||
|
|
||
| query = self._filter_prs(query, pr_filter) | ||
|
|
||
| query = query.filter( | ||
| not_( | ||
| exists().where( | ||
| and_( | ||
| PullRequestEvent.pull_request_id == PullRequest.id, | ||
| PullRequestEvent.type == PullRequestEventType.REVIEW, | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
|
|
||
| query = query.order_by(PullRequest.state_changed_at.asc()) | ||
|
|
||
| return query.all() | ||
|
Comment on lines
+286
to
+313
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Query logic is correct, but clean up trailing whitespace. The implementation correctly identifies PRs merged without review using a NOT EXISTS subquery to exclude PRs with any REVIEW events. The approach is sound and follows established patterns in the codebase. However, there are trailing spaces on lines 288 and 308 that should be removed. Apply this diff to remove trailing whitespace: def get_prs_merged_without_review(
- self,
+ self,
repo_ids: List[str],
interval: Interval,
pr_filter: PRFilter = None,
) -> List[PullRequest]:
query = self._db.session.query(PullRequest).options(defer(PullRequest.data))
query = self._filter_prs_by_repo_ids(query, repo_ids)
query = self._filter_prs_merged_in_interval(query, interval)
query = self._filter_prs(query, pr_filter)
query = query.filter(
not_(
exists().where(
and_(
PullRequestEvent.pull_request_id == PullRequest.id,
PullRequestEvent.type == PullRequestEventType.REVIEW,
)
)
- )
+ )
)
query = query.order_by(PullRequest.state_changed_at.asc())
return query.all()🤖 Prompt for AI Agents |
||
|
|
||
| @rollback_on_exc | ||
| def get_prs_merged_in_interval( | ||
| self, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix return type annotation and parameter type hint.
The function has two type annotation issues:
List[PullRequest], but the function actually returns a dict (the result ofget_non_paginated_pr_response).pr_filterparameter should have an explicitOptionaltype hint or use union syntax.Apply this diff to fix the type annotations:
def get_prs_merged_without_review( team_id: str, from_time: datetime, to_time: datetime, - pr_filter: Dict = None, -) -> List[PullRequest]: + pr_filter: Dict | None = None, +):Note: The return type can be inferred from
get_non_paginated_pr_response, or you can explicitly type it asdictif preferred.📝 Committable suggestion
🧰 Tools
🪛 Ruff (0.14.2)
183-183: PEP 484 prohibits implicit
OptionalConvert to
T | None(RUF013)
🤖 Prompt for AI Agents