From d49ccef8cb80d55be81f08af9fb992c7842f70e8 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Fri, 27 Oct 2023 16:47:00 -0400 Subject: [PATCH] address some minor direct paging backend issues (#3208) # Which issue(s) this PR fixes - Fixes an issue where if the user does not appear in the `UserHasNotification` query, we don't actually unpage the user and therefore they still show up in the `paged_users` array. (unpaging == creating a `AlertGroupLogRecord.TYPE_UNPAGE_USER` log record) - Fixes an issue where if a user is paged multiple times, they would currently show up in `paged_users` > 1 ## Checklist - [x] Unit, integration, and e2e (if applicable) tests updated - [x] 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) --- engine/apps/alerts/models/alert_group.py | 28 +++++++++---------- engine/apps/alerts/paging.py | 22 +++++++++------ engine/apps/alerts/tests/test_alert_group.py | 21 +++++++++++++- engine/apps/api/tests/test_alert_group.py | 29 +++++++++++++++----- 4 files changed, 69 insertions(+), 31 deletions(-) diff --git a/engine/apps/alerts/models/alert_group.py b/engine/apps/alerts/models/alert_group.py index 21c0040d..17051911 100644 --- a/engine/apps/alerts/models/alert_group.py +++ b/engine/apps/alerts/models/alert_group.py @@ -523,7 +523,7 @@ class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models. from apps.alerts.models import AlertGroupLogRecord user_ids: typing.Set[str] = set() - users: typing.List[PagedUser] = [] + users: typing.Dict[str, PagedUser] = {} log_records = self.log_records.filter( type__in=(AlertGroupLogRecord.TYPE_DIRECT_PAGING, AlertGroupLogRecord.TYPE_UNPAGE_USER) @@ -553,23 +553,21 @@ class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models. if user_id is not None and (user := user_map.get(user_id)) is not None: if log_record.type == AlertGroupLogRecord.TYPE_DIRECT_PAGING: # add the user - users.append( - { - "id": user.pk, - "pk": user.public_primary_key, - "name": user.name, - "username": user.username, - "avatar": user.avatar_url, - "avatar_full": user.avatar_full_url, - "important": important, - "teams": [{"pk": t.public_primary_key, "name": t.name} for t in user.teams.all()], - } - ) + users[user_id] = { + "id": user.pk, + "pk": user.public_primary_key, + "name": user.name, + "username": user.username, + "avatar": user.avatar_url, + "avatar_full": user.avatar_full_url, + "important": important, + "teams": [{"pk": t.public_primary_key, "name": t.name} for t in user.teams.all()], + } else: # user was unpaged at some point, remove them - users = [u for u in users if u["pk"] != user_id] + del users[user_id] - return users + return list(users.values()) def _get_response_time(self): """Return response_time based on current alert group status.""" diff --git a/engine/apps/alerts/paging.py b/engine/apps/alerts/paging.py index 20885688..694e88ed 100644 --- a/engine/apps/alerts/paging.py +++ b/engine/apps/alerts/paging.py @@ -155,7 +155,13 @@ def direct_paging( def unpage_user(alert_group: AlertGroup, user: User, from_user: User) -> None: - """Remove user from alert group escalation.""" + """ + Remove user from alert group escalation. + + An IndexError is raised (and caught) if the user had not been notified for some reason. + Regardless of whether or not the user was notified, we will always create an AlertGroupLogRecord of type + TYPE_UNPAGE_USER. + """ try: with transaction.atomic(): user_has_notification = UserHasNotification.objects.filter( @@ -163,15 +169,15 @@ def unpage_user(alert_group: AlertGroup, user: User, from_user: User) -> None: ).select_for_update()[0] user_has_notification.active_notification_policy_id = None user_has_notification.save(update_fields=["active_notification_policy_id"]) - # add log entry - alert_group.log_records.create( - type=AlertGroupLogRecord.TYPE_UNPAGE_USER, - author=from_user, - reason=f"{from_user.username} unpaged user {user.username}", - step_specific_info={"user": user.public_primary_key}, - ) except IndexError: return + finally: + alert_group.log_records.create( + type=AlertGroupLogRecord.TYPE_UNPAGE_USER, + author=from_user, + reason=f"{from_user.username} unpaged user {user.username}", + step_specific_info={"user": user.public_primary_key}, + ) def user_is_oncall(user: User) -> bool: diff --git a/engine/apps/alerts/tests/test_alert_group.py b/engine/apps/alerts/tests/test_alert_group.py index 927a5c0c..ab32c3ed 100644 --- a/engine/apps/alerts/tests/test_alert_group.py +++ b/engine/apps/alerts/tests/test_alert_group.py @@ -526,4 +526,23 @@ def test_alert_group_get_paged_users( _make_log_record(alert_group, other_user, AlertGroupLogRecord.TYPE_DIRECT_PAGING) - alert_group.get_paged_users()[0]["pk"] == other_user.public_primary_key + assert alert_group.get_paged_users()[0]["pk"] == other_user.public_primary_key + + # user was paged, unpaged, and then paged again - they should only show up once + alert_group = make_alert_group(alert_receive_channel) + _make_log_record(alert_group, user, AlertGroupLogRecord.TYPE_DIRECT_PAGING) + _make_log_record(alert_group, user, AlertGroupLogRecord.TYPE_UNPAGE_USER) + _make_log_record(alert_group, user, AlertGroupLogRecord.TYPE_DIRECT_PAGING) + + paged_users = alert_group.get_paged_users() + assert len(paged_users) == 1 + assert alert_group.get_paged_users()[0]["pk"] == user.public_primary_key + + # user was paged and then paged again - they should only show up once + alert_group = make_alert_group(alert_receive_channel) + _make_log_record(alert_group, user, AlertGroupLogRecord.TYPE_DIRECT_PAGING) + _make_log_record(alert_group, user, AlertGroupLogRecord.TYPE_DIRECT_PAGING) + + paged_users = alert_group.get_paged_users() + assert len(paged_users) == 1 + assert alert_group.get_paged_users()[0]["pk"] == user.public_primary_key diff --git a/engine/apps/api/tests/test_alert_group.py b/engine/apps/api/tests/test_alert_group.py index 5ecd4ebb..b293256c 100644 --- a/engine/apps/api/tests/test_alert_group.py +++ b/engine/apps/api/tests/test_alert_group.py @@ -11,6 +11,7 @@ from rest_framework.test import APIClient from apps.alerts.constants import ActionSource from apps.alerts.models import AlertGroup, AlertGroupLogRecord, ResolutionNote +from apps.alerts.paging import direct_paging from apps.alerts.tasks import wipe from apps.api.errors import AlertGroupAPIError from apps.api.permissions import LegacyAccessControlRole @@ -1356,17 +1357,31 @@ def test_unpage_user( make_user_auth_headers, ): client = APIClient() - user, token, alert_groups = alert_group_internal_api_setup - user_to_unpage = make_user(organization=user.organization) - _, _, new_alert_group, _ = alert_groups + user, token, _ = alert_group_internal_api_setup + other_user = make_user(organization=user.organization) - url = reverse("api-internal:alertgroup-unpage-user", kwargs={"pk": new_alert_group.public_primary_key}) - response = client.post( - url, data={"user_id": user_to_unpage.public_primary_key}, **make_user_auth_headers(user, token) - ) + alert_group = direct_paging(user.organization, user, "testtesttest", users=[(other_user, False)]) + paged_users = alert_group.get_paged_users() + + assert paged_users[0]["pk"] == other_user.public_primary_key + + url = reverse("api-internal:alertgroup-unpage-user", kwargs={"pk": alert_group.public_primary_key}) + response = client.post(url, data={"user_id": other_user.public_primary_key}, **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_200_OK + alert_group.refresh_from_db() + assert alert_group.silenced_until is None + assert alert_group.get_paged_users() == [] + + unpage_user_log_record = alert_group.log_records.get( + type=AlertGroupLogRecord.TYPE_UNPAGE_USER, + author=user, + ) + + assert unpage_user_log_record.reason == f"{user.username} unpaged user {other_user.username}" + assert unpage_user_log_record.step_specific_info == {"user": other_user.public_primary_key} + @pytest.mark.django_db def test_invalid_bulk_action(