From eef4af939c729c73376a2d11eb23584b187e6755 Mon Sep 17 00:00:00 2001 From: Snigdha Sharma Date: Fri, 14 Apr 2023 23:44:57 -0700 Subject: [PATCH 1/2] Move is_subscribed to new method --- src/sentry/api/helpers/group_index/update.py | 54 +++++++++++--------- tests/sentry/api/helpers/test_group_index.py | 14 +++++ 2 files changed, 45 insertions(+), 23 deletions(-) diff --git a/src/sentry/api/helpers/group_index/update.py b/src/sentry/api/helpers/group_index/update.py index 66ee0cc3a9df3f..3ddf71aea3c1ab 100644 --- a/src/sentry/api/helpers/group_index/update.py +++ b/src/sentry/api/helpers/group_index/update.py @@ -1,7 +1,7 @@ from __future__ import annotations from collections import defaultdict -from typing import Any, Mapping, MutableMapping, Sequence +from typing import Any, Dict, Mapping, MutableMapping, Sequence import rest_framework from django.db import IntegrityError, transaction @@ -655,29 +655,10 @@ def update_groups( elif result.get("isBookmarked") is False: GroupBookmark.objects.filter(group__in=group_ids, user_id=acting_user.id).delete() - # TODO(dcramer): we could make these more efficient by first - # querying for rich rows are present (if N > 2), flipping the flag - # on those rows, and then creating the missing rows if result.get("isSubscribed") in (True, False): - is_subscribed = result["isSubscribed"] - for group in group_list: - # NOTE: Subscribing without an initiating event (assignment, - # commenting, etc.) clears out the previous subscription reason - # to avoid showing confusing messaging as a result of this - # action. It'd be jarring to go directly from "you are not - # subscribed" to "you were subscribed due since you were - # assigned" just by clicking the "subscribe" button (and you - # may no longer be assigned to the issue anyway.) - GroupSubscription.objects.create_or_update( - user_id=acting_user.id, - group=group, - project=project_lookup[group.project_id], - values={"is_active": is_subscribed, "reason": GroupSubscriptionReason.unknown}, - ) - - result["subscriptionDetails"] = { - "reason": SUBSCRIPTION_REASON_MAP.get(GroupSubscriptionReason.unknown, "unknown") - } + result["subscriptionDetails"] = handle_is_subscribed( + result["isSubscribed"], group_list, project_lookup, acting_user + ) if "isPublic" in result: # We always want to delete an existing share, because triggering @@ -738,3 +719,30 @@ def update_groups( result["inbox"] = inbox return Response(result) + + +def handle_is_subscribed( + is_subscribed: Any, + group_list: Sequence[Group], + project_lookup: Dict[int, Any], + acting_user: User | None, +) -> Dict[str, str]: + # TODO(dcramer): we could make these more efficient by first + # querying for rich rows are present (if N > 2), flipping the flag + # on those rows, and then creating the missing rows + for group in group_list: + # NOTE: Subscribing without an initiating event (assignment, + # commenting, etc.) clears out the previous subscription reason + # to avoid showing confusing messaging as a result of this + # action. It'd be jarring to go directly from "you are not + # subscribed" to "you were subscribed due since you were + # assigned" just by clicking the "subscribe" button (and you + # may no longer be assigned to the issue anyway.) + GroupSubscription.objects.create_or_update( + user_id=acting_user.id if acting_user else None, + group=group, + project=project_lookup[group.project_id], + values={"is_active": is_subscribed, "reason": GroupSubscriptionReason.unknown}, + ) + + return {"reason": SUBSCRIPTION_REASON_MAP.get(GroupSubscriptionReason.unknown, "unknown")} diff --git a/tests/sentry/api/helpers/test_group_index.py b/tests/sentry/api/helpers/test_group_index.py index 57eeee9c79a4f0..8ef29ff818a10e 100644 --- a/tests/sentry/api/helpers/test_group_index.py +++ b/tests/sentry/api/helpers/test_group_index.py @@ -8,11 +8,13 @@ update_groups, validate_search_filter_permissions, ) +from sentry.api.helpers.group_index.update import handle_is_subscribed from sentry.api.issue_search import parse_search_query from sentry.models import ( GroupInbox, GroupInboxReason, GroupStatus, + GroupSubscription, GroupSubStatus, add_group_to_inbox, ) @@ -200,3 +202,15 @@ def test_ignore_with_substatus_until_escalating(self, send_robust): assert group.substatus == GroupSubStatus.UNTIL_ESCALATING assert send_robust.called assert not GroupInbox.objects.filter(group=group).exists() + + +class TestHandleIsSubscribed(TestCase): + def test_is_subscribed(self): + self.group = self.create_group() + self.group_list = [self.group] + self.project_lookup = {self.group.project_id: self.group.project} + + resp = handle_is_subscribed(True, self.group_list, self.project_lookup, self.user) + + assert GroupSubscription.objects.filter(group=self.group, user_id=self.user.id).exists() + assert resp["reason"] == "unknown" From 26a8b0c15aa94592b9569fcb5bc5d7603c59df38 Mon Sep 17 00:00:00 2001 From: Snigdha Sharma Date: Tue, 18 Apr 2023 15:33:52 -0700 Subject: [PATCH 2/2] Apply changes from code review --- src/sentry/api/helpers/group_index/update.py | 20 ++++++++++---------- tests/sentry/api/helpers/test_group_index.py | 15 ++++++++++++++- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/sentry/api/helpers/group_index/update.py b/src/sentry/api/helpers/group_index/update.py index 3ddf71aea3c1ab..ae25186432c508 100644 --- a/src/sentry/api/helpers/group_index/update.py +++ b/src/sentry/api/helpers/group_index/update.py @@ -1,7 +1,7 @@ from __future__ import annotations from collections import defaultdict -from typing import Any, Dict, Mapping, MutableMapping, Sequence +from typing import Any, Mapping, MutableMapping, Sequence import rest_framework from django.db import IntegrityError, transaction @@ -722,24 +722,24 @@ def update_groups( def handle_is_subscribed( - is_subscribed: Any, + is_subscribed: bool, group_list: Sequence[Group], - project_lookup: Dict[int, Any], - acting_user: User | None, -) -> Dict[str, str]: + project_lookup: dict[int, Any], + acting_user: User, +) -> dict[str, str]: # TODO(dcramer): we could make these more efficient by first - # querying for rich rows are present (if N > 2), flipping the flag - # on those rows, and then creating the missing rows + # querying for which `GroupSubscription` rows are present (if N > 2), + # flipping the flag on those rows, and then creating the missing rows for group in group_list: # NOTE: Subscribing without an initiating event (assignment, # commenting, etc.) clears out the previous subscription reason # to avoid showing confusing messaging as a result of this # action. It'd be jarring to go directly from "you are not - # subscribed" to "you were subscribed due since you were + # subscribed" to "you were subscribed since you were # assigned" just by clicking the "subscribe" button (and you - # may no longer be assigned to the issue anyway.) + # may no longer be assigned to the issue anyway). GroupSubscription.objects.create_or_update( - user_id=acting_user.id if acting_user else None, + user_id=acting_user.id, group=group, project=project_lookup[group.project_id], values={"is_active": is_subscribed, "reason": GroupSubscriptionReason.unknown}, diff --git a/tests/sentry/api/helpers/test_group_index.py b/tests/sentry/api/helpers/test_group_index.py index 8ef29ff818a10e..ab4414d5e542b1 100644 --- a/tests/sentry/api/helpers/test_group_index.py +++ b/tests/sentry/api/helpers/test_group_index.py @@ -205,12 +205,25 @@ def test_ignore_with_substatus_until_escalating(self, send_robust): class TestHandleIsSubscribed(TestCase): - def test_is_subscribed(self): + def setUp(self) -> None: self.group = self.create_group() self.group_list = [self.group] self.project_lookup = {self.group.project_id: self.group.project} + def test_is_subscribed(self) -> None: resp = handle_is_subscribed(True, self.group_list, self.project_lookup, self.user) assert GroupSubscription.objects.filter(group=self.group, user_id=self.user.id).exists() assert resp["reason"] == "unknown" + + def test_is_subscribed_updates(self) -> None: + GroupSubscription.objects.create( + group=self.group, project=self.group.project, user_id=self.user.id, is_active=False + ) + + resp = handle_is_subscribed(True, self.group_list, self.project_lookup, self.user) + + subscription = GroupSubscription.objects.filter(group=self.group, user_id=self.user.id) + assert subscription.exists() + assert subscription.first().is_active + assert resp["reason"] == "unknown"