feat(pr_filtering): Initial implementation of PR filtering for Incidents #633
Conversation
WalkthroughThis update integrates pull request (PR) filtering into the incident management system. Several backend functions now accept an optional Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as TeamIncidentPRsFilter (React)
participant Redux as Redux Slice
participant API as Web API Endpoint
participant Service as IncidentService
participant Repo as CodeRepoService
U->>UI: Opens Incident PR Filter Modal
UI->>Redux: Dispatch fetchTeamIncidentPRsFilter action
Redux->>API: Send GET request for incident PR settings
API->>Service: Retrieve settings with optional pr_filter
Service->>Repo: Query PR data via get_prs_by_title_match_strings
Repo-->>Service: Return matching PR data
Service-->>API: Return combined incident and PR data
API-->>Redux: Send team incident PR settings
Redux-->>UI: Update and render filter settings
UI->>U: Display updated Incident PR Filter settings
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
backend/analytics_server/mhq/store/models/incidents/enums.py (1)
39-43: Regex patterns for PR filtering look good, but consider expanding themThe regex patterns are well-defined for the specified formats. However, you might want to consider adding more common patterns for referencing PRs or issues in GitHub/GitLab, such as "Fixes #1234", "Closes #1234", etc.
PR_FILTER_PATTERNS = { "fix #1234": r"(?i)fix #(\d+)", "fix(1234)": r"(?i)fix\((\d+)\)", "fix-1234": r"(?i)fix-(\d+)", + "fixes #1234": r"(?i)fixes #(\d+)", + "closes #1234": r"(?i)closes #(\d+)", + "resolves #1234": r"(?i)resolves #(\d+)", }backend/analytics_server/mhq/service/settings/models.py (1)
49-56: Consider adding default values for the IncidentPrsSetting attributesThe
IncidentPrsSettingclass looks good but would benefit from having default values for its attributes, especially for boolean fields likeinclude_revert_prs.@dataclass class IncidentPrsSetting(BaseSetting): - include_revert_prs: bool - title_filters: List[str] - head_branch_filters: List[str] - pr_mapping_field: str - pr_mapping_pattern: str + include_revert_prs: bool = False + title_filters: List[str] = None + head_branch_filters: List[str] = None + pr_mapping_field: str = "" + pr_mapping_pattern: str = "" + + def __post_init__(self): + self.title_filters = self.title_filters or [] + self.head_branch_filters = self.head_branch_filters or []This will prevent potential
Noneaccess issues and make the class more robust for new instances.web-server/pages/api/internal/team/[team_id]/incident_prs_filter.ts (1)
15-26: Consider refactoring Yup schema conditional validationThe schema validation for
pr_mapping_patternlooks good functionally but has a minor issue flagged by Biome static analysis.- pr_mapping_pattern: yup.string().when('pr_mapping_field', { - is: (pr_mapping_field: string) => pr_mapping_field !== '', - then: yup.string().required() - }) + pr_mapping_pattern: yup.string().when('pr_mapping_field', ([pr_mapping_field], schema) => + pr_mapping_field !== '' ? schema.required() : schema + )This alternative syntax avoids the
thenproperty which is causing the static analysis warning.🧰 Tools
🪛 Biome (1.9.4)
[error] 23-23: Do not add then to an object.
(lint/suspicious/noThenProperty)
web-server/src/types/resources.ts (1)
642-642: Consider expanding incident PR mapping example optionsThe current examples are good but could be expanded to cover more common patterns.
-export const IncidentPRMappingOptions = ['fix #1234', 'fix(1234)', 'fix-1234']; +export const IncidentPRMappingOptions = ['fix #1234', 'fix(1234)', 'fix-1234', 'fixes #1234', 'resolves #1234', 'closes #1234'];These additional patterns align with common GitHub issue linking conventions and would provide more comprehensive examples for users.
web-server/src/components/TeamIncidentPRsFilter.tsx (1)
138-147: Simplify boolean conversion in the IOSSwitch handler.The ternary expression used to convert the string value to a boolean is unnecessarily complex.
- const isEnabled = e.target.value === 'true' ? true : false; + const isEnabled = e.target.value === 'true'; setValue(`setting.include_revert_prs`, !isEnabled, { shouldValidate: true, shouldDirty: true });🧰 Tools
🪛 Biome (1.9.4)
[error] 141-141: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
backend/analytics_server/mhq/service/incidents/incidents.py (4)
116-120: Building the mapping dictionary.
Consider using adefaultdict(dict)to avoid repeated existence checks:- if str(pr.repo_id) not in repo_id_to_pr_number_to_pr_map: - repo_id_to_pr_number_to_pr_map[str(pr.repo_id)] = {} + from collections import defaultdict + repo_id_to_pr_number_to_pr_map = defaultdict(dict)
152-165: Avoid.keys()when checking membership.
At line 155, replaceif pr_number in repo_id_to_pr_number_to_pr_map[str(pr.repo_id)].keys():with
if pr_number in repo_id_to_pr_number_to_pr_map[str(pr.repo_id)]:This is more idiomatic and avoids generating a key view object.
🧰 Tools
🪛 Ruff (0.8.2)
155-155: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
169-193: Combine the nested if statements at lines 180-183
to reduce nesting. Also remove the.keys()usage. For instance:- if pr_number := self._extract_pr_number_from_pattern(...): - if pr_number in repo_id_to_pr_number_to_pr_map[str(pr.repo_id)].keys(): + if (pr_number := self._extract_pr_number_from_pattern(...)) and ( + pr_number in repo_id_to_pr_number_to_pr_map[str(pr.repo_id)] + ): ...🧰 Tools
🪛 Ruff (0.8.2)
180-183: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
183-183: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
237-259: UsingIncidentType.REVERT_PR.
If these incidents represent more general fix or revert scenarios, consider introducing additional incident types or making this configurable to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
backend/analytics_server/mhq/api/incidents.py(6 hunks)backend/analytics_server/mhq/api/resources/settings_resource.py(2 hunks)backend/analytics_server/mhq/service/incidents/incident_filter.py(2 hunks)backend/analytics_server/mhq/service/incidents/incidents.py(4 hunks)backend/analytics_server/mhq/service/settings/configuration_settings.py(8 hunks)backend/analytics_server/mhq/service/settings/default_settings_data.py(1 hunks)backend/analytics_server/mhq/service/settings/models.py(1 hunks)backend/analytics_server/mhq/service/settings/setting_type_validator.py(1 hunks)backend/analytics_server/mhq/store/models/incidents/enums.py(1 hunks)backend/analytics_server/mhq/store/models/settings/configuration_settings.py(1 hunks)backend/analytics_server/mhq/store/repos/code.py(1 hunks)web-server/pages/api/internal/team/[team_id]/incident_prs_filter.ts(1 hunks)web-server/src/components/DoraMetricsConfigurationSettings.tsx(4 hunks)web-server/src/components/TeamIncidentPRsFilter.tsx(1 hunks)web-server/src/slices/team.ts(5 hunks)web-server/src/types/resources.ts(1 hunks)web-server/src/utils/cockpitMetricUtils.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
web-server/pages/api/internal/team/[team_id]/incident_prs_filter.ts
[error] 23-23: Do not add then to an object.
(lint/suspicious/noThenProperty)
web-server/src/components/TeamIncidentPRsFilter.tsx
[error] 141-141: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
[error] 36-36: Do not add then to an object.
(lint/suspicious/noThenProperty)
🪛 Ruff (0.8.2)
backend/analytics_server/mhq/service/incidents/incidents.py
155-155: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
180-183: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
183-183: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🔇 Additional comments (75)
backend/analytics_server/mhq/service/settings/setting_type_validator.py (1)
22-24: Implementation looks good for the new setting type validatorThe validation logic follows the existing pattern of the other setting type validations.
backend/analytics_server/mhq/store/models/settings/configuration_settings.py (1)
19-19: Correctly added new SettingType enum valueThe enum value
INCIDENT_PRS_SETTINGfollows the naming convention of other setting types and is placed in the appropriate location.backend/analytics_server/mhq/service/settings/default_settings_data.py (1)
32-39: Implementation looks good!The new conditional block for
SettingType.INCIDENT_PRS_SETTINGcorrectly defines default values for PR filtering settings. The structure aligns well with the existing pattern in this file, and the default values (empty arrays and strings withinclude_revert_prsset to True) provide a sensible starting point.This implementation properly addresses the PR filtering requirements mentioned in the PR objectives.
backend/analytics_server/mhq/api/resources/settings_resource.py (2)
7-8: Clean import organizationThe imports are well-organized, with
IncidentPrsSettingadded in alphabetical order.
59-66: Good implementation of PR filter settings adapterThe new conditional block for handling
IncidentPrsSettingfollows the established pattern in this file. All necessary fields from the incident PR settings are correctly mapped to the response object.web-server/src/utils/cockpitMetricUtils.ts (2)
100-100: Parameter extraction looks goodThe
prFilterparameter is correctly extracted from the params object, maintaining the existing code structure.
114-115: Good implementation of PR filtering in API requestsThe
prFilterparameter is correctly spread into the params object for all API calls, ensuring consistent application of PR filters across all relevant requests. This implementation properly integrates with the backend changes for PR filtering.Also applies to: 120-123, 129-130, 136-138
backend/analytics_server/mhq/service/incidents/incident_filter.py (2)
2-2: Appropriate imports addedThe imports for
IncidentTypeandIncidentPrsSettingare necessary for the new functionality and are correctly placed in their respective import blocks.Also applies to: 8-8
112-125: Well-implemented feature for excluding revert PRsThe new conditional block correctly implements the logic to filter out revert PRs when
include_revert_prsis set to False. The implementation:
- Checks if the incident PRs setting is available
- Verifies the setting is of the right type
- Properly filters the incident types list when needed
The code is clean, readable, and follows the existing patterns in the codebase.
backend/analytics_server/mhq/store/repos/code.py (1)
244-265: Looks good - method aligns with PR filtering objectiveThe implementation of
get_prs_by_title_match_stringscorrectly follows the repository pattern and matches the existing implementation style. It properly filters PRs by title using case-insensitive matching and returns an ordered list.web-server/pages/api/internal/team/[team_id]/incident_prs_filter.ts (2)
30-42: GET handler looks goodThe GET endpoint correctly retrieves the team's incident PR settings from the configured endpoint, uses appropriate typing, and returns the expected response structure.
44-59: PUT handler looks goodThe PUT endpoint properly extracts payload data, makes the appropriate API call, and returns the updated settings in the response.
web-server/src/types/resources.ts (2)
623-629: Types align with PR filtering requirementsThe
IncidentPRsSettingstype includes all necessary fields for implementing the PR filtering functionality described in the PR objectives.
631-640: Response types properly structuredBoth API response types are well defined with appropriate fields for timestamps, team ID, and settings data.
web-server/src/slices/team.ts (5)
17-19: Import structure looks goodThe imports are properly organized and include the necessary types for the new functionality.
37-37: State type update is appropriateAdding the
teamIncidentPRsFiltersproperty to the state with proper typing is good practice.
53-53: Initial state correctly set to nullSetting the initial state to
nullfor the new property is appropriate since the data will be loaded asynchronously.
121-136: Redux reducers properly configuredThe reducer configuration for both fetch and update actions is correctly implemented using the utility function.
272-295: Thunk actions properly implementedBoth thunk actions are well implemented with appropriate typing, endpoint URLs, and request methods. The structure is consistent with other thunk actions in the file.
web-server/src/components/DoraMetricsConfigurationSettings.tsx (4)
6-16: New import and updated import structure added correctly.The code adds the necessary import for the new
TeamIncidentPRsFiltercomponent and restructures the team slice imports to include the newfetchTeamIncidentPRsFilterfunction.
25-28: Good addition of incident PR filter fetching.The
useEffecthook now properly fetches both production branches and incident PR filters when the component mounts or when the team ID changes, which ensures the necessary data is available for the new filter configuration functionality.
40-46: Well-structured modal callback implementation.The
openIncidentPRFilterModalcallback follows the same pattern as the existingopenProductionBranchSelectorModal, maintaining code consistency and providing a clean way to open the new incident PR filter configuration modal.
90-97: Good UI integration for the new feature.The new menu item is well-integrated into the existing menu structure, providing users with a clear way to access the new incident PR filter configuration feature.
web-server/src/components/TeamIncidentPRsFilter.tsx (5)
1-25: Appropriate imports for the new component.The imports are comprehensive and appropriate for the component's functionality, including form validation, UI components, and state management.
26-41: Well-structured form validation schema.The schema correctly defines the structure and validation rules for the incident PR filter settings, including conditional validation for the PR mapping pattern field.
🧰 Tools
🪛 Biome (1.9.4)
[error] 36-36: Do not add then to an object.
(lint/suspicious/noThenProperty)
47-74: Good component initialization and state management.The component properly initializes with Redux state and sets up form controls with appropriate default values from the team's stored incident PR filters.
77-113: Well-implemented save handler with proper error handling.The
handleSavefunction correctly updates the team's incident PR filter settings, refreshes related metrics, and provides appropriate user feedback through notifications.
115-266: Well-designed UI with appropriate form controls.The component's UI is well-structured with clear labeling and intuitive controls for configuring incident PR filters. The form includes appropriate validation feedback and disables the save button until valid changes are made.
🧰 Tools
🪛 Biome (1.9.4)
[error] 141-141: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
backend/analytics_server/mhq/service/settings/configuration_settings.py (8)
11-13: Appropriate import of the new IncidentPrsSetting class.The new
IncidentPrsSettingclass is properly imported alongside existing setting classes.
70-79: Well-implemented adapter method for incident PR settings.The
_adapt_incident_prs_setting_setting_from_setting_datamethod correctly extracts all necessary fields from the raw data and creates anIncidentPrsSettinginstance with appropriate default values.
103-107: Good integration with existing setting type handling.The new setting type is properly integrated into the
_handle_config_setting_from_db_settingmethod, ensuring it can be correctly loaded from database settings.
151-158: Excellent error handling for concurrent saves.The implementation of error handling in
get_or_set_default_settingsaddresses the issue of concurrent saving scenarios effectively. By catching unique constraint violations specifically related to the settings primary key, the code gracefully handles race conditions and returns the already-saved settings.
209-216: Consistent adapter implementation for JSON data.The
_adapt_incident_prs_setting_setting_from_jsonmethod follows the same pattern as other adapter methods, ensuring consistency in how settings are processed from API data.
240-242: Proper JSON data handler for the new setting type.The setting type is correctly integrated into the
_handle_config_setting_from_json_datamethod, ensuring API data can be properly transformed into the appropriate setting object.
281-290: Well-structured JSON data adapter for serialization.The
_adapt_incident_prs_setting_setting_json_datamethod correctly converts theIncidentPrsSettingobject back to a dictionary format suitable for JSON serialization.
323-327: Complete integration with DB setting handling.The new setting type is properly integrated with the
_handle_config_setting_to_db_settingmethod, ensuring thatIncidentPrsSettingobjects can be correctly serialized for database storage.backend/analytics_server/mhq/api/incidents.py (7)
39-45: Good API schema update for PR filtering.The
get_resolved_incidentsendpoint has been correctly updated to accept an optionalpr_filterparameter, maintaining backward compatibility with existing API clients.
51-59: Well-implemented PR filter application.The
pr_filteris properly processed using theapply_pr_filterfunction and passed to the incident service, ensuring consistent filter application throughout the system.
100-102: Consistent PR filter application for team incidents.The
get_team_incidentscall correctly includes the processed PR filter, maintaining consistency with other incident retrieval methods.
124-143: Good PR filter integration for MTTR metrics.The Mean Time To Recovery (MTTR) endpoint has been properly updated to accept and process the PR filter parameter, ensuring filtered metrics can be obtained.
154-174: Consistent PR filter handling for MTTR trends.The MTTR trends endpoint has been updated with the same PR filter pattern as other endpoints, maintaining consistency throughout the API.
220-222: Proper PR filter usage for change failure rate.The Change Failure Rate (CFR) calculations correctly use the filtered incident list, ensuring metrics consistency across the system.
266-268: Consistent PR filter application for CFR trends.The CFR trends endpoint correctly uses the filtered incidents list, maintaining consistency with other metric calculations.
backend/analytics_server/mhq/service/incidents/incidents.py (32)
3-4: No issues with these new imports.
They succinctly add needed type hints and introduce regex functionality for pattern matching.
5-5: Proper import usage.
ImportingIncidentPrsSettingaligns well with the new PR settings feature.
6-6: Neat addition ofPRFilter.
This is necessary for integrating the PR filtering logic.
7-7: ImportingPullRequest.
Required for creating or adapting PR-based incidents.
8-8: Usage ofIncidentStatusandIncidentType.
This import seems necessary for marking or categorizing incidents.
9-9: Inclusion ofCodeRepoService.
This allows querying and mapping PRs against incidents.
24-24:check_regeximport.
Helps validate the regex pattern before using it, preventing unexpected runtime errors.
32-32: ImportingPR_FILTER_PATTERNS.
Used for extracting PR numbers from known naming patterns.
40-40: Constructor acceptingcode_repo_service.
A clean extension that centralizes code repo logic for incidents.
44-44: Assigning_code_repo_service.
Storing this service instance is essential for subsequent PR lookups.
47-47: Newpr_filterparameter.
Extendingget_resolved_team_incidentswith PR filtering is consistent with the PR objectives.
55-55: IncludingSettingType.INCIDENT_PRS_SETTING.
Ensures we also load any settings that define PR-based incident behaviors.
58-58: Retrieving resolved incidents.
Leverages existing repo service logic. No issues found.
61-61: Combining resolved incidents with resolved PR incidents.
This addition supports merging different incident sources.
63-63: Returning a unified list.
Straightforward approach to aggregate both sets of incidents.
65-67: Enhancedget_team_incidentswithpr_filter.
Matches the pattern inget_resolved_team_incidents, ensuring consistency.
75-75: Again addingSettingType.INCIDENT_PRS_SETTING.
This maintains alignment with incident + PR setting filters.
82-84: Fetch and merge PR incidents.
Ensures the resulting incidents list factors in any PR-based items.
88-90: Newget_team_pr_incidentsmethod.
A dedicated approach that encapsulates logic for retrieving PRs as incidents.
91-97: Loading or initializingIncidentPrsSetting.
Usingget_or_set_default_settingsguarantees an existing setting object for PR management.
99-104: Early return if no PR filters or mapping.
Prevents unnecessary processing when no settings are configured.
107-113: Retrieving merged PRs in the given interval.
Efficiently narrows the scope to relevant PRs across the team’s repos.
114-115: Initializingrepo_id_to_pr_number_to_pr_map.
Maintains clarity on how PRs are indexed.
121-123:_get_mapped_incident_prscall.
Segregation of logic for mapped PRs is well-structured.
125-127:_filter_incident_prscall.
Good decomposition of separate filtering scenarios.
129-129: Concatenating final PR-based incidents.
Straightforward solution to unify mapped and filtered incident PRs.
131-137: New_get_mapped_incident_prsmethod.
Cleanly encapsulates the mapping logic of PR → Incident.
139-143: Checking for mapping field and pattern.
Returning early if absent avoids unnecessary processing.
166-168: Appending match strings for subsequent lookups.
No issues.
195-226: Substring-based matching in branches/titles.
Be aware thatbranch in current_pr.head_branchortitle in current_pr.titlecan match partial substrings. If partial matches are unintended, consider stricter regex filtering.
228-235:_extract_pr_number_from_patternlogic.
Uses a known pattern dict and then searches for matches. Good approach for flexible pattern-based extraction.
423-424: Returning a fully wiredIncidentService.
AddingCodeRepoServiceis consistent with new PR filtering features.
Linked Issue(s)
Ref #557
Acceptance Criteria fulfillment
fix(1234)which means it's a fix for PR number 1234get_or_set_default_settingsconcurrent savingProposed changes (including videos or screenshots)
Screen.recording.160325.mp4
Summary by CodeRabbit