fix(insights): Fix get_prev_history to fetch the correct previous history row for a status

migrations_lockfile.txt

@@ -6,5 +6,5 @@ To resolve this, rebase against latest master and regenerate your migration. Thi
 will then be regenerated, and you should be able to merge without conflicts.
 
 nodestore: 0002_nodestore_no_dictfield
-sentry: 0239_drop_scheduleddeletion_aborted
+sentry: 0240_grouphistory_index
 social_auth: 0001_initial

src/sentry/migrations/0240_grouphistory_index.py

@@ -0,0 +1,36 @@
+# Generated by Django 2.2.24 on 2021-10-20 17:26
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+    # This flag is used to mark that a migration shouldn't be automatically run in
+    # production. We set this to True for operations that we think are risky and want
+    # someone from ops to run manually and monitor.
+    # General advice is that if in doubt, mark your migration as `is_dangerous`.
+    # Some things you should always mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that
+    #   they can be monitored. Since data migrations will now hold a transaction open
+    #   this is even more important.
+    # - Adding columns to highly active tables, even ones that are NULL.
+    is_dangerous = False
+
+    # This flag is used to decide whether to run this migration in a transaction or not.
+    # By default we prefer to run in a transaction, but for migrations where you want
+    # to `CREATE INDEX CONCURRENTLY` this needs to be set to False. Typically you'll
+    # want to create an index concurrently when adding one to an existing table.
+    # You'll also usually want to set this to `False` if you're writing a data
+    # migration, since we don't want the entire migration to run in one long-running
+    # transaction.
+    atomic = True
+
+    dependencies = [
+        ("sentry", "0239_drop_scheduleddeletion_aborted"),
+    ]
+
+    operations = [
+        migrations.AlterIndexTogether(
+            name="grouphistory",
+            index_together={("project", "status", "release"), ("group", "status")},
+        ),
+    ]

src/sentry/models/grouphistory.py

@@ -34,6 +34,29 @@ class GroupHistoryStatus:
     GroupHistoryStatus.DELETED_AND_DISCARDED,
 ]
 
+UNRESOLVED_STATUSES = (GroupHistoryStatus.UNRESOLVED, GroupHistoryStatus.REGRESSED)
+RESOLVED_STATUSES = (
+    GroupHistoryStatus.RESOLVED,
+    GroupHistoryStatus.SET_RESOLVED_IN_RELEASE,
+    GroupHistoryStatus.SET_RESOLVED_IN_COMMIT,
+    GroupHistoryStatus.SET_RESOLVED_IN_PULL_REQUEST,
+    GroupHistoryStatus.AUTO_RESOLVED,
+)
+
+PREVIOUS_STATUSES = {
+    GroupHistoryStatus.UNRESOLVED: RESOLVED_STATUSES,
+    GroupHistoryStatus.RESOLVED: UNRESOLVED_STATUSES,
+    GroupHistoryStatus.SET_RESOLVED_IN_RELEASE: UNRESOLVED_STATUSES,
+    GroupHistoryStatus.SET_RESOLVED_IN_COMMIT: UNRESOLVED_STATUSES,
+    GroupHistoryStatus.SET_RESOLVED_IN_PULL_REQUEST: UNRESOLVED_STATUSES,
+    GroupHistoryStatus.AUTO_RESOLVED: UNRESOLVED_STATUSES,
+    GroupHistoryStatus.IGNORED: (GroupHistoryStatus.UNIGNORED,),
+    GroupHistoryStatus.UNIGNORED: (GroupHistoryStatus.IGNORED,),
+    GroupHistoryStatus.ASSIGNED: (GroupHistoryStatus.UNASSIGNED,),
+    GroupHistoryStatus.UNASSIGNED: (GroupHistoryStatus.ASSIGNED,),
+    GroupHistoryStatus.REGRESSED: RESOLVED_STATUSES,
+}
+
 
 class GroupHistory(Model):
     """
@@ -82,16 +105,23 @@ class GroupHistory(Model):
     class Meta:
         db_table = "sentry_grouphistory"
         app_label = "sentry"
-        index_together = (("project", "status", "release"),)
+        index_together = (("project", "status", "release"), ("group", "status"))
 
     __repr__ = sane_repr("group_id", "release_id")
 
 
-def get_prev_history(group):
-    prev_histories = GroupHistory.objects.filter(group=group).order_by("date_added")
-    if prev_histories.exists():
-        return prev_histories.first()
-    return None
+def get_prev_history(group, status):
+    """
+    Finds the most recent row that is the inverse of this history row, if one exists.
+    """
+    previous_statuses = PREVIOUS_STATUSES.get(status)
+    if not previous_statuses:
+        return
+
+    prev_histories = GroupHistory.objects.filter(
+        group=group, status__in=previous_statuses
+    ).order_by("-date_added")
+    return prev_histories.first()
 
 
 def record_group_history_from_activity_type(group, activity_type, actor=None, release=None):
@@ -105,9 +135,9 @@ def record_group_history_from_activity_type(group, activity_type, actor=None, re
 
 
 def record_group_history(group, status, actor=None, release=None):
-    prev_history = get_prev_history(group)
     if not features.has("organizations:group-history", group.organization):
         return
+    prev_history = get_prev_history(group, status)
     return GroupHistory.objects.create(
         organization=group.project.organization,
         group=group,

src/sentry/testutils/factories.py

@@ -44,6 +44,7 @@
 )
 from sentry.models import (
     Activity,
+    Actor,
     Commit,
     CommitAuthor,
     CommitFileChange,
@@ -53,6 +54,7 @@
     ExternalIssue,
     File,
     Group,
+    GroupHistory,
     GroupLink,
     Identity,
     IdentityProvider,
@@ -1082,3 +1084,25 @@ def create_identity(
             status=IdentityStatus.VALID,
             scopes=[],
         )
+
+    @staticmethod
+    def create_group_history(
+        group: Group,
+        status: int,
+        release: Optional[Release] = None,
+        actor: Actor = None,
+        prev_history: GroupHistory = None,
+    ) -> GroupHistory:
+        prev_history_date = None
+        if prev_history:
+            prev_history_date = prev_history.date_added
+        return GroupHistory.objects.create(
+            organization=group.organization,
+            group=group,
+            project=group.project,
+            release=release,
+            actor=actor,
+            status=status,
+            prev_history=prev_history,
+            prev_history_date=prev_history_date,
+        )

src/sentry/testutils/fixtures.py

@@ -363,6 +363,9 @@ def create_identity(self, *args, **kwargs):
     def create_identity_provider(self, *args, **kwargs):
         return Factories.create_identity_provider(*args, **kwargs)
 
+    def create_group_history(self, *args, **kwargs):
+        return Factories.create_group_history(*args, **kwargs)
+
     @pytest.fixture(autouse=True)
     def _init_insta_snapshot(self, insta_snapshot):
         self.insta_snapshot = insta_snapshot

tests/sentry/models/test_grouphistory.py

@@ -0,0 +1,29 @@
+from sentry.models import GroupHistoryStatus, get_prev_history
+from sentry.testutils import TestCase
+
+
+class GetPrevHistoryTest(TestCase):
+    def test_no_history(self):
+        # Test both statuses with/without a previous status
+        assert get_prev_history(self.group, GroupHistoryStatus.UNRESOLVED) is None
+        assert get_prev_history(self.group, GroupHistoryStatus.DELETED) is None
+
+    def test_history(self):
+        prev_history = self.create_group_history(self.group, GroupHistoryStatus.UNRESOLVED)
+        assert get_prev_history(self.group, GroupHistoryStatus.RESOLVED) == prev_history
+        assert get_prev_history(self.group, GroupHistoryStatus.DELETED) is None
+
+    def test_multi_history(self):
+        other_group = self.create_group()
+        self.create_group_history(other_group, GroupHistoryStatus.UNRESOLVED)
+        assert get_prev_history(self.group, GroupHistoryStatus.UNRESOLVED) is None
+        prev_history = self.create_group_history(self.group, GroupHistoryStatus.UNRESOLVED)
+        assert get_prev_history(self.group, GroupHistoryStatus.RESOLVED) == prev_history
+        prev_history = self.create_group_history(
+            self.group, GroupHistoryStatus.RESOLVED, prev_history=prev_history
+        )
+        assert get_prev_history(self.group, GroupHistoryStatus.UNRESOLVED) == prev_history
+        prev_history = self.create_group_history(
+            self.group, GroupHistoryStatus.UNRESOLVED, prev_history=prev_history
+        )
+        assert get_prev_history(self.group, GroupHistoryStatus.RESOLVED) == prev_history