Browse Source

fix(issues): Remove ignore rows when an issue is resolved (#45617)

We're leaving ignore rows around when we resolve an issue, which causes
problems with us displaying that an issue is not resolved in some cases.
Fixing this in two places:

- Added a signal handler so that whenever we resolve an issue, we'll
always remove any pending ignore rows since they're no longer relevant.
- To handle existing invalid ignore rows, fixed the
`clear_expired_snoozes` task to correctly remove expired snoozes even if
the related group doesn't have the `IGNORED` status.
Dan Fuller 2 years ago
parent
commit
5e901f316c

+ 1 - 0
src/sentry/conf/server.py

@@ -382,6 +382,7 @@ INSTALLED_APPS = (
     "sentry.eventstream",
     "sentry.auth.providers.google.apps.Config",
     "django.contrib.staticfiles",
+    "sentry.issues.apps.Config",
 )
 
 # Silence internal hints from Django's system checks

+ 8 - 0
src/sentry/issues/apps.py

@@ -0,0 +1,8 @@
+from django.apps import AppConfig
+
+
+class Config(AppConfig):  # type: ignore
+    name = "sentry.issues"
+
+    def ready(self) -> None:
+        from . import receivers  # NOQA

+ 16 - 0
src/sentry/issues/receivers.py

@@ -0,0 +1,16 @@
+from typing import Any
+
+from sentry.models import Group, GroupSnooze
+from sentry.signals import issue_resolved
+
+
+@issue_resolved.connect(weak=False)  # type: ignore
+def remove_ignores(group: Group, **kwargs: Any) -> None:
+    """
+    If an issue is resolved we should remove any pending ignore rows
+    """
+    try:
+        snooze = GroupSnooze.objects.get(group=group)
+        snooze.delete()
+    except GroupSnooze.DoesNotExist:
+        pass

+ 5 - 3
src/sentry/tasks/clear_expired_snoozes.py

@@ -8,16 +8,18 @@ from sentry.tasks.base import instrumented_task
 
 @instrumented_task(name="sentry.tasks.clear_expired_snoozes", time_limit=65, soft_time_limit=60)
 def clear_expired_snoozes():
-    group_list = list(
-        GroupSnooze.objects.filter(until__lte=timezone.now()).values_list("group", flat=True)
+    groupsnooze_list = list(
+        GroupSnooze.objects.filter(until__lte=timezone.now()).values_list("id", "group")
     )
+    group_snooze_ids = [gs[0] for gs in groupsnooze_list]
+    group_list = [gs[1] for gs in groupsnooze_list]
 
     ignored_groups = list(Group.objects.filter(id__in=group_list, status=GroupStatus.IGNORED))
     Group.objects.filter(id__in=group_list, status=GroupStatus.IGNORED).update(
         status=GroupStatus.UNRESOLVED
     )
 
-    GroupSnooze.objects.filter(group__in=group_list).delete()
+    GroupSnooze.objects.filter(id__in=group_snooze_ids).delete()
 
     for group in ignored_groups:
         record_group_history(group, GroupHistoryStatus.UNIGNORED)

+ 20 - 2
tests/sentry/tasks/test_clear_expired_snoozes.py

@@ -16,10 +16,14 @@ class ClearExpiredSnoozesTest(TestCase):
     @patch("sentry.signals.issue_unignored.send_robust")
     def test_simple(self, send_robust):
         group1 = self.create_group(status=GroupStatus.IGNORED)
-        GroupSnooze.objects.create(group=group1, until=timezone.now() - timedelta(minutes=1))
+        snooze1 = GroupSnooze.objects.create(
+            group=group1, until=timezone.now() - timedelta(minutes=1)
+        )
 
         group2 = self.create_group(status=GroupStatus.IGNORED)
-        GroupSnooze.objects.create(group=group2, until=timezone.now() + timedelta(minutes=1))
+        snooze2 = GroupSnooze.objects.create(
+            group=group2, until=timezone.now() + timedelta(minutes=1)
+        )
 
         clear_expired_snoozes()
 
@@ -27,6 +31,8 @@ class ClearExpiredSnoozesTest(TestCase):
 
         assert Group.objects.get(id=group2.id).status == GroupStatus.IGNORED
 
+        assert not GroupSnooze.objects.filter(id=snooze1.id).exists()
+        assert GroupSnooze.objects.filter(id=snooze2.id).exists()
         assert GroupHistory.objects.filter(
             group=group1, status=GroupHistoryStatus.UNIGNORED
         ).exists()
@@ -35,3 +41,15 @@ class ClearExpiredSnoozesTest(TestCase):
         ).exists()
 
         assert send_robust.called
+
+    def test_resolved_group(self):
+        group1 = self.create_group(status=GroupStatus.RESOLVED)
+        snooze1 = GroupSnooze.objects.create(
+            group=group1, until=timezone.now() - timedelta(minutes=1)
+        )
+
+        clear_expired_snoozes()
+
+        assert Group.objects.get(id=group1.id).status == GroupStatus.RESOLVED
+        # Validate that even though the group wasn't modified, we still remove the snooze
+        assert not GroupSnooze.objects.filter(id=snooze1.id).exists()

+ 18 - 0
tests/snuba/api/endpoints/test_organization_group_index.py

@@ -1950,6 +1950,24 @@ class GroupUpdateTest(APITestCase, SnubaTestCase):
         assert response.data == {"status": "resolved", "statusDetails": {}, "inbox": None}
         assert response.status_code == 200
 
+    def test_resolve_ignored(self):
+        group = self.create_group(status=GroupStatus.IGNORED)
+        snooze = GroupSnooze.objects.create(
+            group=group, until=timezone.now() - timedelta(minutes=1)
+        )
+
+        member = self.create_user()
+        self.create_member(
+            organization=self.organization, teams=group.project.teams.all(), user=member
+        )
+
+        self.login_as(user=member)
+        response = self.get_success_response(
+            qs_params={"id": group.id, "project": self.project.id}, status="resolved"
+        )
+        assert response.data == {"status": "resolved", "statusDetails": {}, "inbox": None}
+        assert not GroupSnooze.objects.filter(id=snooze.id).exists()
+
     def test_bulk_resolve(self):
         self.login_as(user=self.user)