From a0abc1cf57775071736ea1d27333e0c17495041a Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Tue, 19 Oct 2021 18:39:09 -0700 Subject: [PATCH 1/2] fix(insights): Fix `get_prev_history` to fetch the correct previous history row for a status Previously this function would always just fetch the oldest history for a given group. This pr fixes it to look for the most recent row with a status that is considered the inverse of the status we're using to create the new row. Unresolved and resolved are opposites, as an example, and not all statuses have an inverse. --- src/sentry/models/grouphistory.py | 44 ++++++++++++++++++++---- src/sentry/testutils/factories.py | 24 +++++++++++++ src/sentry/testutils/fixtures.py | 3 ++ tests/sentry/models/test_grouphistory.py | 29 ++++++++++++++++ 4 files changed, 93 insertions(+), 7 deletions(-) create mode 100644 tests/sentry/models/test_grouphistory.py diff --git a/src/sentry/models/grouphistory.py b/src/sentry/models/grouphistory.py index 08fbe46f38de30..dfef1b39e34df5 100644 --- a/src/sentry/models/grouphistory.py +++ b/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, diff --git a/src/sentry/testutils/factories.py b/src/sentry/testutils/factories.py index 2c5a54f68373c9..0d21996a110e66 100644 --- a/src/sentry/testutils/factories.py +++ b/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, + ) diff --git a/src/sentry/testutils/fixtures.py b/src/sentry/testutils/fixtures.py index e72d29b6b1cebd..92fb1184fd2fe8 100644 --- a/src/sentry/testutils/fixtures.py +++ b/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 diff --git a/tests/sentry/models/test_grouphistory.py b/tests/sentry/models/test_grouphistory.py new file mode 100644 index 00000000000000..0eeac92f665ea1 --- /dev/null +++ b/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 From a72e021fbd8887c3eddc846dd0a5f947a0e01bb1 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Wed, 20 Oct 2021 10:27:30 -0700 Subject: [PATCH 2/2] add migration --- migrations_lockfile.txt | 2 +- .../migrations/0240_grouphistory_index.py | 36 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 src/sentry/migrations/0240_grouphistory_index.py diff --git a/migrations_lockfile.txt b/migrations_lockfile.txt index 041e2730bee74f..6cbf15a7004a35 100644 --- a/migrations_lockfile.txt +++ b/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 diff --git a/src/sentry/migrations/0240_grouphistory_index.py b/src/sentry/migrations/0240_grouphistory_index.py new file mode 100644 index 00000000000000..5efe966e370af9 --- /dev/null +++ b/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")}, + ), + ]