feat(pr_filtering): Added Incident PRs Setting, handle race condition and Revert PR filter #650
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new configuration setting type called Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant SettingsService
participant DB
Client->>API: Request configuration settings
API->>SettingsService: Fetch settings
SettingsService->>DB: Query settings by type
DB-->>SettingsService: Return settings data
SettingsService-->>API: Adapt settings (including IncidentPrsSetting)
API-->>Client: Return settings (with IncidentPrsSetting fields)
sequenceDiagram
participant IncidentFilter
participant SettingsService
IncidentFilter->>SettingsService: Get IncidentPrsSetting
SettingsService-->>IncidentFilter: Return IncidentPrsSetting (include_revert_prs)
IncidentFilter->>IncidentFilter: Filter incident types (exclude REVERT_PR if needed)
Poem
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (2)
backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py (2)
58-80: Test name doesn't match assertion behavior.The test name indicates "returns_empty_list" but the assertion checks for
Nonerather than an empty list. This creates a discrepancy between the test name and its actual behavior.-def test_get_incident_types_when_only_prs_setting_present_returns_empty_list(): +def test_get_incident_types_when_only_prs_setting_present_returns_none():
1-144: Consider adding an edge case test.The tests cover most scenarios well, but consider adding a test for when
IncidentTypesSettingcontains onlyREVERT_PRandinclude_revert_prsis False, which should result in an empty list of incident types.def test_get_incident_types_when_only_revert_pr_type_and_not_includes_revert_prs(): setting_types = [ SettingType.INCIDENT_TYPES_SETTING, SettingType.INCIDENT_PRS_SETTING, ] incident_prs_setting = IncidentPrsSetting( include_revert_prs=False, title_filters=[], head_branch_filters=[], pr_mapping_field="", pr_mapping_pattern="", ) setting_type_to_settings_map = { SettingType.INCIDENT_TYPES_SETTING: IncidentTypesSetting( incident_types=[IncidentType.REVERT_PR] ), SettingType.INCIDENT_PRS_SETTING: incident_prs_setting, } incident_filter = ConfigurationsIncidentFilterProcessor( incident_filter=IncidentFilter(), entity_type=EntityType.TEAM, entity_id="team_id", setting_types=setting_types, setting_type_to_settings_map=setting_type_to_settings_map, ).apply() # Should be empty list as REVERT_PR is excluded assert incident_filter.incident_types == []
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: All file linting
🔇 Additional comments (5)
backend/analytics_server/tests/service/Incidents/test_incident_types_setting.py (5)
1-10: Imports look good and appropriate.The necessary imports for testing the
ConfigurationsIncidentFilterProcessorwith the newIncidentPrsSettingfeature are correctly included.
12-39: Test case correctly verifies behavior with only IncidentTypesSetting.This test properly verifies that when only the
IncidentTypesSettingis present, the incident types are directly passed through to the filter.
41-56: Test case correctly handles empty incident types setting.Good test case confirming that an empty list in the settings produces an empty list in the filter.
82-112: Test correctly verifies 'include_revert_prs=True' behavior.This test properly confirms that when both settings are present and
include_revert_prsis True, the REVERT_PR type is included in the incident filter.
114-144: Test correctly verifies 'include_revert_prs=False' behavior.The test properly confirms that when
include_revert_prsis False, the REVERT_PR type is excluded from the incident filter, which is a key requirement from the PR objectives.
backend/analytics_server/mhq/api/resources/settings_resource.py
Outdated
Show resolved
Hide resolved
adnanhashmi09
merged commit b5c9275
into
middlewarehq:incidents-pr-filtering
Linked Issue(s)
Ref #557
Acceptance Criteria fulfillment
include_revert_prsto false.Summary by CodeRabbit
New Features
Bug Fixes