-
Notifications
You must be signed in to change notification settings - Fork 0
chore(aci milestone 3): open period -> incident serializer #123
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: master
Are you sure you want to change the base?
Conversation
|
Replace Adds a new implementation of Key Changes• Major rewrite of Affected Areas• This summary was automatically generated by @propel-code-bot |
| def get_incident_status(self, priority: int | None, date_ended: datetime | None) -> int: | ||
| if priority is None: | ||
| raise ValueError("Priority is required to get an incident status") | ||
|
|
||
| if date_ended: | ||
| return IncidentStatus.CLOSED.value | ||
|
|
||
| return self.priority_to_incident_status[priority] |
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.
[CriticalError]
Potential KeyError when priority is not in mapping: The get_incident_status method will raise a KeyError if priority is not HIGH or MEDIUM (e.g., LOW priority). This could cause runtime failures.
Suggested Change
| def get_incident_status(self, priority: int | None, date_ended: datetime | None) -> int: | |
| if priority is None: | |
| raise ValueError("Priority is required to get an incident status") | |
| if date_ended: | |
| return IncidentStatus.CLOSED.value | |
| return self.priority_to_incident_status[priority] | |
| def get_incident_status(self, priority: int | None, date_ended: datetime | None) -> int: | |
| if priority is None: | |
| raise ValueError("Priority is required to get an incident status") | |
| if date_ended: | |
| return IncidentStatus.CLOSED.value | |
| return self.priority_to_incident_status.get(priority, IncidentStatus.WARNING.value) |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Context for Agents
[**CriticalError**]
Potential KeyError when priority is not in mapping: The `get_incident_status` method will raise a `KeyError` if `priority` is not `HIGH` or `MEDIUM` (e.g., `LOW` priority). This could cause runtime failures.
<details>
<summary>Suggested Change</summary>
```suggestion
def get_incident_status(self, priority: int | None, date_ended: datetime | None) -> int:
if priority is None:
raise ValueError("Priority is required to get an incident status")
if date_ended:
return IncidentStatus.CLOSED.value
return self.priority_to_incident_status.get(priority, IncidentStatus.WARNING.value)
```
⚡ **Committable suggestion**
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
</details>
File: src/sentry/incidents/endpoints/serializers/workflow_engine_incident.py
Line: 50| open_periods_to_detectors = {} | ||
| for group in group_to_open_periods: | ||
| for op in group_to_open_periods[group]: | ||
| open_periods_to_detectors[op] = groups_to_detectors[group] |
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.
[CriticalError]
Potential KeyError when accessing missing group detector: Line 129 accesses groups_to_detectors[group] but if a group doesn't have a corresponding detector in the detector_groups queryset, this will raise a KeyError.
Suggested Change
| open_periods_to_detectors = {} | |
| for group in group_to_open_periods: | |
| for op in group_to_open_periods[group]: | |
| open_periods_to_detectors[op] = groups_to_detectors[group] | |
| open_periods_to_detectors = {} | |
| for group in group_to_open_periods: | |
| detector = groups_to_detectors.get(group) | |
| if detector: # Only process groups that have detectors | |
| for op in group_to_open_periods[group]: | |
| open_periods_to_detectors[op] = detector |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Context for Agents
[**CriticalError**]
Potential KeyError when accessing missing group detector: Line 129 accesses `groups_to_detectors[group]` but if a group doesn't have a corresponding detector in the `detector_groups` queryset, this will raise a `KeyError`.
<details>
<summary>Suggested Change</summary>
```suggestion
open_periods_to_detectors = {}
for group in group_to_open_periods:
detector = groups_to_detectors.get(group)
if detector: # Only process groups that have detectors
for op in group_to_open_periods[group]:
open_periods_to_detectors[op] = detector
```
⚡ **Committable suggestion**
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
</details>
File: src/sentry/incidents/endpoints/serializers/workflow_engine_incident.py
Line: 129
Previously, this serializer took in an open period, fetched the incident via the IGOP lookup table, and serialized the incident. With the incident no longer guaranteed to exist, create a serializer that will populate an incident response using only the open period model.
Copied from getsentry#103131
Original PR: getsentry#103131