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

@@ -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."""

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:

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

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(