Skip to content

Commit

Permalink
Don't update alert group metrics when deleting an alert group (#3544)
Browse files Browse the repository at this point in the history
# Which issue(s) this PR fixes

Fixes grafana/oncall-private#2376

## Checklist

- [x] Unit, integration, and e2e (if applicable) tests updated
- [ ] Documentation added (or `pr:no public docs` PR label added if not
required)
- [x] `CHANGELOG.md` updated (or `pr:no changelog` PR label added if not
required)
  • Loading branch information
joeyorlando authored Dec 11, 2023
1 parent 58de3f8 commit 16ba87b
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 5 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed

- Fix schedules invalid dates issue ([#support-escalations/issues/8084](https://github.com/grafana/support-escalations/issues/8084))
- Fix issue related to updating alert group metrics when deleting an alert group via the public API by @joeyorlando ([#3544](https://github.com/grafana/oncall/pull/3544))

## v1.3.76 (2023-12-11)

Expand Down
7 changes: 2 additions & 5 deletions engine/apps/alerts/models/alert_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -1191,8 +1191,6 @@ def wipe_by_user(self, user: User) -> None:
def delete_by_user(self, user: User):
from apps.alerts.models import AlertGroupLogRecord

initial_state = self.state

self.stop_escalation()
# prevent creating multiple logs
# filter instead of get_or_create cause it can be multiple logs of this type due deleting error
Expand Down Expand Up @@ -1222,10 +1220,9 @@ def delete_by_user(self, user: User):
dependent_alerts = list(self.dependent_alert_groups.all())

self.hard_delete()
# Update alert group state metric cache
self._update_metrics(organization_id=user.organization_id, previous_state=initial_state, state=None)

for dependent_alert_group in dependent_alerts: # unattach dependent incidents
# unattach dependent incidents
for dependent_alert_group in dependent_alerts:
dependent_alert_group.un_attach_by_delete()

def hard_delete(self):
Expand Down
2 changes: 2 additions & 0 deletions engine/apps/alerts/tasks/delete_alert_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ def delete_alert_group(alert_group_pk, user_pk):
logger.debug("User not found, skipping delete_alert_group")
return

logger.debug(f"User {user} is deleting alert group {alert_group} (channel: {alert_group.channel})")

try:
alert_group.delete_by_user(user)
except SlackAPIRatelimitError as e:
Expand Down
44 changes: 44 additions & 0 deletions engine/apps/alerts/tests/test_alert_group.py
Original file line number Diff line number Diff line change
Expand Up @@ -577,3 +577,47 @@ def test_filter_active_alert_groups(
assert active_alert_groups.count() == 2
assert alert_group_active in active_alert_groups
assert alert_group_active_silenced in active_alert_groups


@patch("apps.alerts.models.AlertGroup.hard_delete")
@patch("apps.alerts.models.AlertGroup.un_attach_by_delete")
@patch("apps.alerts.models.AlertGroup.stop_escalation")
@patch("apps.alerts.models.alert_group.alert_group_action_triggered_signal")
@pytest.mark.django_db
def test_delete_by_user(
mock_alert_group_action_triggered_signal,
_mock_stop_escalation,
_mock_un_attach_by_delete,
_mock_hard_delete,
make_organization_and_user,
make_alert_receive_channel,
make_alert_group,
):
organization, user = make_organization_and_user()
alert_receive_channel = make_alert_receive_channel(organization)

alert_group = make_alert_group(alert_receive_channel)

# make a few dependent alert groups
dependent_alert_groups = [make_alert_group(alert_receive_channel, root_alert_group=alert_group) for _ in range(3)]

assert alert_group.log_records.filter(type=AlertGroupLogRecord.TYPE_DELETED).count() == 0

alert_group.delete_by_user(user)

assert alert_group.log_records.filter(type=AlertGroupLogRecord.TYPE_DELETED).count() == 1
deleted_log_record = alert_group.log_records.get(type=AlertGroupLogRecord.TYPE_DELETED)

alert_group.stop_escalation.assert_called_once_with()

mock_alert_group_action_triggered_signal.send.assert_called_once_with(
sender=alert_group.delete_by_user,
log_record=deleted_log_record.pk,
action_source=None,
force_sync=True,
)

alert_group.hard_delete.assert_called_once_with()

for dependent_alert_group in dependent_alert_groups:
dependent_alert_group.un_attach_by_delete.assert_called_with()

0 comments on commit 16ba87b

Please sign in to comment.