Repository: getsentry/sentry
Pull request: feat(issue-states): Take issue age into account when issues are manually unarchived
Take issue age into account when issues are manually unarchived so that issues < 3 days old are marked as NEW 

Fixes https://github.com/getsentry/sentry/issues/48761

Problem statement:
Unignoring an issue should revert to NEW or ONGOING
When an issue is unignored/unarchived, we should take the issue age into account and set the substatus to NEW or ONGOING accordingly (< 3 days = NEW). 

These issues should also be added back into GroupInbox

Hints from issue discussion:

Commit message: Add type:ignore

Patch to review:
diff --git a/src/sentry/issues/escalating.py b/src/sentry/issues/escalating.py
index 3f1e92cb9fc56a..13aea667fd0abd 100644
--- a/src/sentry/issues/escalating.py
+++ b/src/sentry/issues/escalating.py
@@ -3,7 +3,7 @@
 """
 import logging
 from collections import defaultdict
-from datetime import datetime, timedelta
+from datetime import datetime, timedelta, timezone
 from typing import Any, Dict, List, Mapping, Optional, Sequence, Tuple, TypedDict
 
 from django.db.models.signals import post_save
@@ -343,20 +343,25 @@ def manage_issue_states(
             add_group_to_inbox(group, GroupInboxReason.ONGOING, snooze_details)
             record_group_history(group, GroupHistoryStatus.ONGOING)
     elif group_inbox_reason == GroupInboxReason.UNIGNORED:
-        updated = Group.objects.filter(
+        groups = Group.objects.filter(
             id=group.id, status__in=[GroupStatus.RESOLVED, GroupStatus.IGNORED]
-        ).update(status=GroupStatus.UNRESOLVED, substatus=GroupSubStatus.ONGOING)
-        if updated:
-            group.status = GroupStatus.UNRESOLVED
-            group.substatus = GroupSubStatus.ONGOING
-            post_save.send(
-                sender=Group,
-                instance=group,
-                created=False,
-                update_fields=["status", "substatus"],
-            )
-            add_group_to_inbox(group, GroupInboxReason.UNIGNORED, snooze_details)
-            record_group_history(group, GroupHistoryStatus.UNIGNORED)
+        )
+        updated = False
+        for group in groups:
+            is_new_group = group.first_seen > datetime.now(timezone.utc) - timedelta(days=3)
+            substatus = GroupSubStatus.NEW if is_new_group else GroupSubStatus.ONGOING
+            updated = group.update(status=GroupStatus.UNRESOLVED, substatus=substatus)
+            if updated:
+                group.status = GroupStatus.UNRESOLVED
+                group.substatus = substatus
+                post_save.send(
+                    sender=Group,
+                    instance=group,
+                    created=False,
+                    update_fields=["status", "substatus"],
+                )
+                add_group_to_inbox(group, GroupInboxReason.UNIGNORED, snooze_details)
+                record_group_history(group, GroupHistoryStatus.UNIGNORED)
 
     else:
         raise NotImplementedError(
diff --git a/tests/sentry/issues/test_escalating.py b/tests/sentry/issues/test_escalating.py
index 0963d24ae33523..6c80e8153d756f 100644
--- a/tests/sentry/issues/test_escalating.py
+++ b/tests/sentry/issues/test_escalating.py
@@ -3,6 +3,7 @@
 from unittest.mock import patch
 from uuid import uuid4
 
+import pytest
 from freezegun import freeze_time
 
 from sentry.eventstore.models import Event
@@ -11,15 +12,19 @@
     _start_and_end_dates,
     get_group_hourly_count,
     is_escalating,
+    manage_issue_states,
     query_groups_past_counts,
 )
 from sentry.issues.escalating_group_forecast import EscalatingGroupForecast
 from sentry.issues.grouptype import GroupCategory, ProfileFileIOGroupType
 from sentry.models import Group
+from sentry.models.activity import Activity
 from sentry.models.group import GroupStatus
-from sentry.models.groupinbox import GroupInbox
+from sentry.models.grouphistory import GroupHistory, GroupHistoryStatus
+from sentry.models.groupinbox import GroupInbox, GroupInboxReason
 from sentry.testutils import SnubaTestCase, TestCase
 from sentry.testutils.cases import PerformanceIssueTestCase
+from sentry.types.activity import ActivityType
 from sentry.types.group import GroupSubStatus
 from sentry.utils.cache import cache
 from sentry.utils.snuba import to_start_of_hour
@@ -275,3 +280,57 @@ def test_hourly_count_query(self) -> None:
 
         # Events are aggregated in the hourly count query by date rather than the last 24hrs
         assert get_group_hourly_count(group) == 1
+
+
+class ManageIssueStatesTest(TestCase):  # type: ignore[misc]
+    def test_status_changes(self) -> None:
+        state_change_details = [
+            {
+                "substatus": GroupSubStatus.ESCALATING,
+                "reason": GroupInboxReason.ESCALATING,
+                "history_status": GroupHistoryStatus.ESCALATING,
+                "first_seen": datetime.now(),
+            },
+            {
+                "substatus": GroupSubStatus.ONGOING,
+                "reason": GroupInboxReason.ONGOING,
+                "history_status": GroupHistoryStatus.ONGOING,
+                "first_seen": datetime.now(),
+            },
+            {
+                "substatus": GroupSubStatus.NEW,
+                "reason": GroupInboxReason.UNIGNORED,
+                "history_status": GroupHistoryStatus.UNIGNORED,
+                "first_seen": datetime.now(),
+            },
+            {
+                "substatus": GroupSubStatus.ONGOING,
+                "reason": GroupInboxReason.UNIGNORED,
+                "history_status": GroupHistoryStatus.UNIGNORED,
+                "first_seen": datetime.now() - timedelta(days=4),
+            },
+        ]
+
+        for state_change in state_change_details:
+            self.group = self.create_group(
+                status=GroupStatus.IGNORED, first_seen=state_change["first_seen"]
+            )
+            manage_issue_states(self.group, state_change["reason"])
+
+            self.group.refresh_from_db()
+
+            assert self.group.status == GroupStatus.UNRESOLVED
+            assert self.group.substatus == state_change["substatus"]
+
+            assert GroupInbox.objects.filter(group=self.group).exists()
+            assert GroupHistory.objects.filter(
+                group=self.group, status=state_change["history_status"]
+            ).exists()
+            assert Activity.objects.filter(
+                group=self.group, type=ActivityType.SET_UNRESOLVED.value
+            ).exists()
+
+    def test_invalid_status_changes(self) -> None:
+        self.group = self.create_group()
+        with pytest.raises(NotImplementedError):
+            manage_issue_states(self.group, GroupInboxReason.REPROCESSED)
diff --git a/tests/sentry/tasks/test_clear_expired_snoozes.py b/tests/sentry/tasks/test_clear_expired_snoozes.py
index 74249e76b18635..e41285d5406461 100644
--- a/tests/sentry/tasks/test_clear_expired_snoozes.py
+++ b/tests/sentry/tasks/test_clear_expired_snoozes.py
@@ -32,7 +32,7 @@ def test_simple(self, send_robust):
         group2.refresh_from_db()
 
         assert group1.status == GroupStatus.UNRESOLVED
-        assert group1.substatus == GroupSubStatus.ONGOING
+        assert group1.substatus == GroupSubStatus.NEW
 
         # Check if unexpired snooze got cleared
         assert group2.status == GroupStatus.IGNORED
@@ -48,6 +48,24 @@ def test_simple(self, send_robust):
 
         assert send_robust.called
 
+    @patch("sentry.signals.issue_unignored.send_robust")
+    def test_simple_with_old_issue(self, send_robust):
+        group1 = self.create_group(status=GroupStatus.IGNORED)
+        snooze1 = GroupSnooze.objects.create(group=group1, until=timezone.now() - timedelta(days=4))
+
+        clear_expired_snoozes()
+
+        group1.refresh_from_db()
+        assert group1.status == GroupStatus.UNRESOLVED
+        assert group1.substatus == GroupSubStatus.NEW
+
+        assert not GroupSnooze.objects.filter(id=snooze1.id).exists()
+        assert GroupHistory.objects.filter(
+            group=group1, status=GroupHistoryStatus.UNIGNORED
+        ).exists()
+
+        assert send_robust.called
+
     @with_feature("organizations:escalating-issues")
     @patch("sentry.signals.issue_unignored.send_robust")
     def test_simple_with_escalating_issues(self, send_robust):


Code review comments:
diff_hunk: @@ -343,20 +343,25 @@ def manage_issue_states(
             add_group_to_inbox(group, GroupInboxReason.ONGOING, snooze_details)
             record_group_history(group, GroupHistoryStatus.ONGOING)
     elif group_inbox_reason == GroupInboxReason.UNIGNORED:
-        updated = Group.objects.filter(
+        groups = Group.objects.filter(
             id=group.id, status__in=[GroupStatus.RESOLVED, GroupStatus.IGNORED]
-        ).update(status=GroupStatus.UNRESOLVED, substatus=GroupSubStatus.ONGOING)
-        if updated:
-            group.status = GroupStatus.UNRESOLVED
-            group.substatus = GroupSubStatus.ONGOING
-            post_save.send(
-                sender=Group,
-                instance=group,
-                created=False,
-                update_fields=["status", "substatus"],
-            )
-            add_group_to_inbox(group, GroupInboxReason.UNIGNORED, snooze_details)
-            record_group_history(group, GroupHistoryStatus.UNIGNORED)
+        )
path: src/sentry/issues/escalating.py
text: @user1:
I'm not sure it would be possible to get multiple groups when we're filtering on `group.id`?

@author:
Changed the logic to grab the first entry from the `.filter`.

@user1:
Could we use a conditional update (https://docs.djangoproject.com/en/4.2/ref/models/conditional-expressions/#conditional-update) here instead of retrieving the first? 

The reason why we wanna do the update chained with the filter is we want the resulting query to run all in one shot in postgres. I'm not too sure how likely it is to run into race conditions for updating the `status` and `substatus` values for the same group but if we break them up its possible to update the state when it was previously updated already.

@user1:
Hmm, now that I think about it if we do a conditional update, we can't retrieve the `substatus` field value at all and won't be able to call `add_group_to_inbox` appropriately. 

I suppose an alternative approach is we break this up into two parts:
1. 
```
ongoing_set = Group.objects.filter(
    id=group.id, status__in=[GroupStatus.RESOLVED, GroupStatus.IGNORED]
).update(
    status=Case(
        When(
            first_seen__lt=datetime.now(timezone.utc) - timedelta(days=3),
            then=Value(GroupStatus.UNRESOLVED),
        ),
        default=F('status'),
    ),
    substatus=Case(
        When(
            first_seen__lt=datetime.now(timezone.utc) - timedelta(days=3),
            then=Value(GroupSubStatus.ONGOING),
        ),
        default=F('substatus')
    ),
)
```
2. 
```
new_set = Group.objects.filter(
    id=group.id, status__in=[GroupStatus.RESOLVED, GroupStatus.IGNORED]
).update(
    status=Case(
        When(
            first_seen__gte=datetime.now(timezone.utc) - timedelta(days=3),
            then=Value(GroupStatus.UNRESOLVED),
        ),
        default=F('status'),
    ),
    substatus=Case(
        When(
            first_seen__gte=datetime.now(timezone.utc) - timedelta(days=3),
            then=Value(GroupSubStatus.NEW),
        ),
        default=F('substatus')
    ),
)
```

Then fire the appropriate methods based on:
```
updated = new_set or ongoing_set

if not ongoing_set and not new_set: # this group isn't resolved or ongoing, skip it
  return

if new_set:
  # New things

if ongoing_set:
  # ongoing things

```