feat: Backend API for PRs Merged Without Review #691
Conversation
|
|
WalkthroughA new backend API endpoint is added to fetch merged pull requests without review for a team within a specified time interval. The implementation spans three layers: the HTTP API endpoint, the analytics service, and the repository data access layer with filtering logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API Endpoint
participant Service as Analytics Service
participant Repo as Code Repo Service
participant DB as Database
Client->>API: GET /teams/{team_id}/prs/merged_without_review<br/>(from_time, to_time, pr_filter)
API->>API: Validate interval<br/>Fetch team & repos
API->>Service: get_prs_merged_without_review<br/>(repo_ids, interval, pr_filter)
Service->>Repo: get_prs_merged_without_review<br/>(repo_ids, interval, pr_filter)
Repo->>DB: Query PRs merged without REVIEW events<br/>within interval & repos
DB-->>Repo: PullRequest records
Repo-->>Service: List[PullRequest]
Service-->>API: List[PullRequest]
API-->>Client: Non-paginated PR response (JSON)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
backend/analytics_server/mhq/api/pull_requests.py (2)
171-178: Consider consistent formatting for the schema.The schema definition is correct, but line 175 has inconsistent spacing compared to lines 174 and 176.
Apply this diff for consistent formatting:
Schema({ Required("from_time"): All(str, Coerce(datetime.fromisoformat)), - Required("to_time"): All(str, Coerce(datetime.fromisoformat)), + Required("to_time"): All(str, Coerce(datetime.fromisoformat)), Optional("pr_filter"): All(str, Coerce(json.loads)), })
185-204: Consider adding team validation for consistency.The implementation correctly fetches PRs merged without review. However, comparing to other endpoints in this file (e.g., line 80 in
get_lead_time_prs), you might want to add team validation for consistency and to provide early error feedback if the team doesn't exist.If you want to add team validation, apply this diff:
query_validator = get_query_validator() pr_analytics = get_pr_analytics_service() interval: Interval = query_validator.interval_validator(from_time, to_time) + team: Team = query_validator.team_validator(team_id) pr_filter: PRFilter = apply_pr_filter( pr_filter, EntityType.TEAM, team_id, [SettingType.EXCLUDED_PRS_SETTING] )Note: This would require adding the Team import if not already present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/analytics_server/mhq/api/pull_requests.py(1 hunks)backend/analytics_server/mhq/service/code/pr_analytics.py(2 hunks)backend/analytics_server/mhq/store/repos/code.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
backend/analytics_server/mhq/api/pull_requests.py (8)
backend/analytics_server/mhq/api/request_utils.py (1)
queryschema(17-37)backend/analytics_server/mhq/service/code/pr_analytics.py (3)
get_prs_merged_without_review(22-23)get_pr_analytics_service(25-26)get_team_repos(16-17)backend/analytics_server/mhq/store/repos/code.py (2)
get_prs_merged_without_review(287-313)get_team_repos(401-416)backend/analytics_server/mhq/utils/time.py (3)
from_time(26-27)to_time(30-31)Interval(14-96)backend/analytics_server/mhq/service/query_validator.py (2)
get_query_validator(76-77)interval_validator(44-56)backend/analytics_server/mhq/store/models/code/filter.py (1)
PRFilter(10-108)backend/analytics_server/mhq/service/code/pr_filter.py (1)
apply_pr_filter(11-34)backend/analytics_server/mhq/api/resources/code_resouces.py (1)
get_non_paginated_pr_response(64-109)
backend/analytics_server/mhq/store/repos/code.py (5)
backend/analytics_server/mhq/store/models/code/enums.py (1)
PullRequestEventType(30-51)backend/analytics_server/mhq/store/__init__.py (1)
rollback_on_exc(27-36)backend/analytics_server/mhq/utils/time.py (1)
Interval(14-96)backend/analytics_server/mhq/store/models/code/filter.py (1)
PRFilter(10-108)backend/analytics_server/mhq/store/models/code/pull_requests.py (2)
PullRequest(12-76)PullRequestEvent(79-103)
backend/analytics_server/mhq/service/code/pr_analytics.py (5)
backend/analytics_server/mhq/store/models/code/filter.py (1)
PRFilter(10-108)backend/analytics_server/mhq/utils/time.py (1)
Interval(14-96)backend/analytics_server/mhq/api/pull_requests.py (1)
get_prs_merged_without_review(179-204)backend/analytics_server/mhq/store/repos/code.py (1)
get_prs_merged_without_review(287-313)backend/analytics_server/mhq/store/models/code/pull_requests.py (1)
PullRequest(12-76)
🪛 Flake8 (7.3.0)
backend/analytics_server/mhq/service/code/pr_analytics.py
[error] 2-2: 'mhq.store.models.core.teams.Team' imported but unused
(F401)
🪛 Ruff (0.14.2)
backend/analytics_server/mhq/api/pull_requests.py
183-183: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🔇 Additional comments (2)
backend/analytics_server/mhq/service/code/pr_analytics.py (1)
22-23: LGTM!The service method correctly delegates to the repository layer and follows the established pattern in this class.
backend/analytics_server/mhq/store/repos/code.py (1)
5-6: LGTM!The new imports are correctly added to existing import statements and are properly used in the new method.
| def get_prs_merged_without_review( | ||
| team_id: str, | ||
| from_time: datetime, | ||
| to_time: datetime, | ||
| pr_filter: Dict = None, | ||
| ) -> List[PullRequest]: |
There was a problem hiding this comment.
Fix return type annotation and parameter type hint.
The function has two type annotation issues:
- The return type is annotated as
List[PullRequest], but the function actually returns a dict (the result ofget_non_paginated_pr_response). - The
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 as dict if preferred.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_prs_merged_without_review( | |
| team_id: str, | |
| from_time: datetime, | |
| to_time: datetime, | |
| pr_filter: Dict = None, | |
| ) -> List[PullRequest]: | |
| def get_prs_merged_without_review( | |
| team_id: str, | |
| from_time: datetime, | |
| to_time: datetime, | |
| pr_filter: Dict | None = None, | |
| ): |
🧰 Tools
🪛 Ruff (0.14.2)
183-183: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for AI Agents
In backend/analytics_server/mhq/api/pull_requests.py around lines 179 to 184,
the function signature incorrectly annotates the return type as
List[PullRequest] and leaves pr_filter without an explicit Optional type; update
the signature so the return type matches the actual return value (e.g., dict or
the specific type returned by get_non_paginated_pr_response) and change
pr_filter: Dict = None to pr_filter: Optional[Dict] = None (or pr_filter: Dict |
None = None), and add the necessary typing import (Optional) if not already
present.
| ) -> List[PullRequest]: | ||
| query_validator = get_query_validator() | ||
|
|
||
| pr_analytics = get_pr_analytics_service() |
There was a problem hiding this comment.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pr_analytics = get_pr_analytics_service() | |
| pr_analytics = get_pr_analytics_service() |
🤖 Prompt for AI Agents
In backend/analytics_server/mhq/api/pull_requests.py around line 187, the line
"pr_analytics = get_pr_analytics_service() " contains trailing whitespace;
remove the extra space at the end of the line so it reads "pr_analytics =
get_pr_analytics_service()" and run a quick linter/formatter to ensure no other
trailing whitespace remains.
| team_repos = pr_analytics.get_team_repos(team_id) | ||
| if not team_repos: | ||
| return [] |
There was a problem hiding this comment.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| team_repos = pr_analytics.get_team_repos(team_id) | |
| if not team_repos: | |
| return [] | |
| team_repos = pr_analytics.get_team_repos(team_id) | |
| if not team_repos: | |
| return [] |
🤖 Prompt for AI Agents
In backend/analytics_server/mhq/api/pull_requests.py around lines 195 to 197,
the early return is correct but line 197 contains trailing whitespace; remove
the trailing space after the return [] so the line ends exactly with return []
and save the file (run the formatter/linter if you have one to ensure no other
trailing whitespace remains).
| @@ -1,6 +1,8 @@ | |||
| from mhq.store.models.code.filter import PRFilter | |||
| from mhq.store.models.core.teams import Team | |||
There was a problem hiding this comment.
Remove unused import.
The Team import is not used anywhere in this file.
Apply this diff to remove the unused import:
-from mhq.store.models.core.teams import Team📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from mhq.store.models.core.teams import Team |
🧰 Tools
🪛 Flake8 (7.3.0)
[error] 2-2: 'mhq.store.models.core.teams.Team' imported but unused
(F401)
🤖 Prompt for AI Agents
In backend/analytics_server/mhq/service/code/pr_analytics.py around line 2, the
imported symbol Team from mhq.store.models.core.teams is unused; remove the line
"from mhq.store.models.core.teams import Team" to eliminate the unused import
and keep the file imports minimal, then run the linter/tests to confirm no other
references break.
| @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() |
There was a problem hiding this comment.
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
In backend/analytics_server/mhq/store/repos/code.py around lines 286 to 313,
there are trailing whitespace characters on lines 288 and 308; remove those
trailing spaces so the lines end cleanly (no extra spaces) and save the file to
eliminate the whitespace-only diffs.
2 participants
Linked Issue(s)
Closes #207
Acceptance Criteria Fulfillment
Proposed Changes
/teams/<team_id>/prs/merged_without_review— retrieves all pull requests merged without any review activity.API Response Example
Below is an example response from the new API:
UI Reference (For Context Only)
The following screenshots illustrate how this data could be visualized in the UI.
Note: This PR only includes backend API changes the UI is shown for reference.
Summary by CodeRabbit