Skip to content

Commit

Permalink
fix(group-inbox): Update GroupInbox when using issue platform to upda…
Browse files Browse the repository at this point in the history
…te statuses (#79708)

Currently, updating a group's status via the issue platform doesn't
add/remove the group from GroupInbox. Since we use GroupInbox to
populate issues in the `is:for_review` query, we're seeing resolved
groups in the stream that should be otherwise hidden.
[Example](https://sentry.sentry.io/issues/?project=1&query=is%3Afor_review%20%21issue.category%3Aerror%20issue%3ASENTRY-3H3K&referrer=issue-list&sort=date&statsPeriod=90d&viewId=54006)

Fixing this behavior in this PR to make sure the GroupInbox is updated
with each status change.
  • Loading branch information
snigdhas authored Oct 24, 2024
1 parent 67ffead commit 96a4abd
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 2 deletions.
16 changes: 15 additions & 1 deletion src/sentry/issues/status_change_consumer.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,12 @@
from sentry.issues.status_change_message import StatusChangeMessageData
from sentry.models.group import Group, GroupStatus
from sentry.models.grouphash import GroupHash
from sentry.models.groupinbox import GroupInboxReason
from sentry.models.groupinbox import (
GroupInboxReason,
GroupInboxRemoveAction,
add_group_to_inbox,
remove_group_from_inbox,
)
from sentry.models.organization import Organization
from sentry.models.project import Project
from sentry.types.activity import ActivityType
Expand Down Expand Up @@ -58,6 +63,8 @@ def update_status(group: Group, status_change: StatusChangeMessageData) -> None:
substatus=new_substatus,
activity_type=ActivityType.SET_RESOLVED,
)
remove_group_from_inbox(group, action=GroupInboxRemoveAction.RESOLVED)

elif new_status == GroupStatus.IGNORED:
# The IGNORED status supports 3 substatuses. For UNTIL_ESCALATING and
# UNTIL_CONDITION_MET, we expect the caller to monitor the conditions/escalating
Expand All @@ -75,13 +82,19 @@ def update_status(group: Group, status_change: StatusChangeMessageData) -> None:
substatus=new_substatus,
activity_type=ActivityType.SET_IGNORED,
)
remove_group_from_inbox(group, action=GroupInboxRemoveAction.IGNORED)
elif new_status == GroupStatus.UNRESOLVED and new_substatus == GroupSubStatus.ESCALATING:
# Update the group status, priority, and add the group to the inbox
manage_issue_states(group=group, group_inbox_reason=GroupInboxReason.ESCALATING)
elif new_status == GroupStatus.UNRESOLVED:
activity_type = None
group_inbox_reason = None
if new_substatus == GroupSubStatus.REGRESSED:
activity_type = ActivityType.SET_REGRESSION
group_inbox_reason = GroupInboxReason.REGRESSION

elif new_substatus == GroupSubStatus.ONGOING:
group_inbox_reason = GroupInboxReason.ONGOING
if group.substatus == GroupSubStatus.ESCALATING:
# If the group was previously escalating, update the priority via AUTO_SET_ONGOING
activity_type = ActivityType.AUTO_SET_ONGOING
Expand All @@ -104,6 +117,7 @@ def update_status(group: Group, status_change: StatusChangeMessageData) -> None:
activity_type=activity_type,
from_substatus=group.substatus,
)
add_group_to_inbox(group, group_inbox_reason)
else:
logger.error(
"group.update_status.unsupported_status",
Expand Down
18 changes: 17 additions & 1 deletion tests/sentry/issues/test_status_change_consumer.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from sentry.models.activity import Activity
from sentry.models.group import Group, GroupStatus
from sentry.models.grouphistory import GroupHistory, GroupHistoryStatus
from sentry.models.groupinbox import GroupInbox, GroupInboxReason
from sentry.testutils.pytest.fixtures import django_db_all
from sentry.types.activity import ActivityType
from sentry.types.group import GroupSubStatus, PriorityLevel
Expand Down Expand Up @@ -52,6 +53,7 @@ def _assert_statuses_set(
group_history_status: int,
activity_type: ActivityType,
priority: int | None = None,
group_inbox_reason: GroupInboxReason | None = None,
) -> None:
self.group.refresh_from_db()
assert self.group.status == status
Expand All @@ -66,6 +68,13 @@ def _assert_statuses_set(
group_id=self.group.id, type=ActivityType.SET_PRIORITY.value
).exists()

if group_inbox_reason:
assert GroupInbox.objects.filter(
group=self.group, reason=group_inbox_reason.value
).exists()
else:
assert not GroupInbox.objects.filter(group=self.group).exists()

@django_db_all
def test_valid_payload_resolved(self) -> None:
message = get_test_message_status_change(self.project.id, fingerprint=["touch-id"])
Expand All @@ -77,7 +86,11 @@ def test_valid_payload_resolved(self) -> None:
group.refresh_from_db()

self._assert_statuses_set(
GroupStatus.RESOLVED, None, GroupHistoryStatus.RESOLVED, ActivityType.SET_RESOLVED
GroupStatus.RESOLVED,
None,
GroupHistoryStatus.RESOLVED,
ActivityType.SET_RESOLVED,
group_inbox_reason=None,
)

def test_valid_payload_archived_forever(self) -> None:
Expand All @@ -99,6 +112,7 @@ def test_valid_payload_archived_forever(self) -> None:
GroupSubStatus.FOREVER,
GroupHistoryStatus.ARCHIVED_FOREVER,
ActivityType.SET_IGNORED,
group_inbox_reason=None,
)

def test_valid_payload_unresolved_escalating(self) -> None:
Expand Down Expand Up @@ -126,6 +140,7 @@ def test_valid_payload_unresolved_escalating(self) -> None:
GroupHistoryStatus.ESCALATING,
ActivityType.SET_ESCALATING,
PriorityLevel.HIGH,
group_inbox_reason=GroupInboxReason.ESCALATING,
)

def test_valid_payload_auto_ongoing(self) -> None:
Expand Down Expand Up @@ -156,6 +171,7 @@ def test_valid_payload_auto_ongoing(self) -> None:
GroupHistoryStatus.ONGOING,
ActivityType.AUTO_SET_ONGOING,
PriorityLevel.MEDIUM,
group_inbox_reason=GroupInboxReason.ONGOING,
)


Expand Down

0 comments on commit 96a4abd

Please sign in to comment.