From 6477a1917e735d6e5451fe5bee9d2fc7a62d2683 Mon Sep 17 00:00:00 2001 From: getsentry-bot Date: Wed, 13 Sep 2023 21:04:43 +0000 Subject: [PATCH] Revert "feat: Add notification_uuid to digest URLs (#55951)" This reverts commit 2a46053ca9f2995d9e0db4795166588b42ee62d1. Co-authored-by: scttcper <1400464+scttcper@users.noreply.github.com> --- src/sentry/digests/notifications.py | 14 +++----- src/sentry/mail/adapter.py | 4 +-- .../notifications/notifications/digest.py | 13 ++------ src/sentry/tasks/digests.py | 20 ++--------- src/sentry/web/frontend/debug/mail.py | 5 +-- tests/sentry/digests/test_notifications.py | 21 +++--------- tests/sentry/mail/test_adapter.py | 6 ---- .../notifications/test_digests.py | 26 ++------------- tests/sentry/tasks/test_digests.py | 33 ++----------------- 9 files changed, 19 insertions(+), 123 deletions(-) diff --git a/src/sentry/digests/notifications.py b/src/sentry/digests/notifications.py index 8c0d79c41d7f9f..96542b5b64af58 100644 --- a/src/sentry/digests/notifications.py +++ b/src/sentry/digests/notifications.py @@ -17,7 +17,7 @@ logger = logging.getLogger("sentry.digests") -Notification = namedtuple("Notification", "event rules notification_uuid") +Notification = namedtuple("Notification", "event rules") def split_key( @@ -57,15 +57,13 @@ def unsplit_key( return f"mail:p:{project.id}:{target_type.value}:{target_str}:{fallthrough}" -def event_to_record( - event: Event, rules: Sequence[Rule], notification_uuid: str | None = None -) -> Record: +def event_to_record(event: Event, rules: Sequence[Rule]) -> Record: if not rules: logger.warning(f"Creating record for {event} that does not contain any rules!") return Record( event.event_id, - Notification(event, [rule.id for rule in rules], notification_uuid), + Notification(event, [rule.id for rule in rules]), to_timestamp(event.datetime), ) @@ -147,11 +145,7 @@ def rewrite_record( return Record( record.key, - Notification( - event, - [_f for _f in [rules.get(id) for id in record.value.rules] if _f], - record.value.notification_uuid, - ), + Notification(event, [_f for _f in [rules.get(id) for id in record.value.rules] if _f]), record.timestamp, ) diff --git a/src/sentry/mail/adapter.py b/src/sentry/mail/adapter.py index 72febaa7c0b654..5363db16c43aeb 100644 --- a/src/sentry/mail/adapter.py +++ b/src/sentry/mail/adapter.py @@ -77,12 +77,12 @@ def get_digest_option(key): extra["digest_key"] = digest_key immediate_delivery = digests.add( digest_key, - event_to_record(event, rules, notification_uuid=notification_uuid), + event_to_record(event, rules), increment_delay=get_digest_option("increment_delay"), maximum_delay=get_digest_option("maximum_delay"), ) if immediate_delivery: - deliver_digest.delay(digest_key, notification_uuid=notification_uuid) + deliver_digest.delay(digest_key) else: log_event = "digested" diff --git a/src/sentry/notifications/notifications/digest.py b/src/sentry/notifications/notifications/digest.py index ef8e5d83e7183f..5ab046288df190 100644 --- a/src/sentry/notifications/notifications/digest.py +++ b/src/sentry/notifications/notifications/digest.py @@ -102,11 +102,7 @@ def reference(self) -> Model | None: def get_context(self) -> MutableMapping[str, Any]: rule_details = get_rules(list(self.digest.keys()), self.project.organization, self.project) context = DigestNotification.build_context( - self.digest, - self.project, - self.project.organization, - rule_details, - notification_uuid=self.notification_uuid, + self.digest, self.project, self.project.organization, rule_details ) sentry_query_params = self.get_sentry_query_params(ExternalProviders.EMAIL) @@ -129,7 +125,6 @@ def build_context( organization: Organization, rule_details: Sequence[NotificationRuleDetails], alert_timestamp: int | None = None, - notification_uuid: str | None = None, ) -> MutableMapping[str, Any]: has_session_replay = features.has("organizations:session-replay", organization) show_replay_link = features.has("organizations:session-replay-issue-emails", organization) @@ -140,11 +135,7 @@ def build_context( "slack_link": get_integration_link(organization, "slack"), "rules_details": {rule.id: rule for rule in rule_details}, "link_params_for_rule": get_email_link_extra_params( - "digest_email", - None, - rule_details, - alert_timestamp, - notification_uuid=notification_uuid, + "digest_email", None, rule_details, alert_timestamp ), "show_replay_links": has_session_replay and show_replay_link, } diff --git a/src/sentry/tasks/digests.py b/src/sentry/tasks/digests.py index 4787db31d925d8..3279c76104568d 100644 --- a/src/sentry/tasks/digests.py +++ b/src/sentry/tasks/digests.py @@ -1,8 +1,7 @@ import logging import time -from typing import List, Optional -from sentry.digests import Record, get_option_key +from sentry.digests import get_option_key from sentry.digests.backends.base import InvalidState from sentry.digests.notifications import build_digest, split_key from sentry.models import Project, ProjectOption @@ -43,7 +42,7 @@ def schedule_digests(): queue="digests.delivery", silo_mode=SiloMode.REGION, ) -def deliver_digest(key, schedule_timestamp=None, notification_uuid: Optional[str] = None): +def deliver_digest(key, schedule_timestamp=None): from sentry import digests from sentry.mail import mail_adapter @@ -62,9 +61,6 @@ def deliver_digest(key, schedule_timestamp=None, notification_uuid: Optional[str try: with digests.digest(key, minimum_delay=minimum_delay) as records: digest, logs = build_digest(project, records) - - if not notification_uuid: - notification_uuid = get_notification_uuid_from_records(records) except InvalidState as error: logger.info(f"Skipped digest delivery: {error}", exc_info=True) return @@ -76,7 +72,6 @@ def deliver_digest(key, schedule_timestamp=None, notification_uuid: Optional[str target_type, target_identifier, fallthrough_choice=fallthrough_choice, - notification_uuid=notification_uuid, ) else: logger.info( @@ -89,14 +84,3 @@ def deliver_digest(key, schedule_timestamp=None, notification_uuid: Optional[str "fallthrough_choice": fallthrough_choice.value if fallthrough_choice else None, }, ) - - -def get_notification_uuid_from_records(records: List[Record]) -> Optional[str]: - for record in records: - try: - notification_uuid = record.value.notification_uuid - if notification_uuid: # Take the first existing notification_uuid - return notification_uuid - except Exception: - return None - return None diff --git a/src/sentry/web/frontend/debug/mail.py b/src/sentry/web/frontend/debug/mail.py index 0ac73463104e57..8a908954bf7fcb 100644 --- a/src/sentry/web/frontend/debug/mail.py +++ b/src/sentry/web/frontend/debug/mail.py @@ -479,7 +479,7 @@ def digest(request): } records = [] group_generator = make_group_generator(random, project) - notification_uuid = str(uuid.uuid4()) + for _ in range(random.randint(1, 30)): group = next(group_generator) state["groups"][group.id] = group @@ -511,7 +511,6 @@ def digest(request): random.sample( list(state["rules"].keys()), random.randint(1, len(state["rules"])) ), - notification_uuid, ), to_timestamp(event.datetime), ) @@ -535,7 +534,6 @@ def digest(request): random.sample( list(state["rules"].keys()), random.randint(1, len(state["rules"])) ), - notification_uuid, ), # this is required for acceptance tests to pass as the EventManager won't accept a timestamp in the past to_timestamp(datetime(2016, 6, 22, 16, 16, 0, tzinfo=timezone.utc)), @@ -559,7 +557,6 @@ def digest(request): random.sample( list(state["rules"].keys()), random.randint(1, len(state["rules"])) ), - notification_uuid, ), # this is required for acceptance tests to pass as the EventManager won't accept a timestamp in the past to_timestamp(datetime(2016, 6, 22, 16, 16, 0, tzinfo=timezone.utc)), diff --git a/tests/sentry/digests/test_notifications.py b/tests/sentry/digests/test_notifications.py index 2d8de50ade1ee7..3a00b2492c3c0a 100644 --- a/tests/sentry/digests/test_notifications.py +++ b/tests/sentry/digests/test_notifications.py @@ -1,6 +1,5 @@ from __future__ import annotations -import uuid from collections import defaultdict from functools import cached_property, reduce from typing import Mapping, MutableMapping, MutableSequence @@ -24,16 +23,13 @@ @region_silo_test(stable=True) class RewriteRecordTestCase(TestCase): - def setUp(self): - self.notification_uuid = str(uuid.uuid4()) - @cached_property def rule(self): return self.event.project.rule_set.all()[0] @cached_property def record(self): - return event_to_record(self.event, (self.rule,), self.notification_uuid) + return event_to_record(self.event, (self.rule,)) def test_success(self): assert rewrite_record( @@ -43,7 +39,7 @@ def test_success(self): rules={self.rule.id: self.rule}, ) == Record( self.record.key, - Notification(self.record.value.event, [self.rule], self.notification_uuid), + Notification(self.record.value.event, [self.rule]), self.record.timestamp, ) @@ -63,17 +59,12 @@ def test_filters_invalid_rules(self): groups={self.event.group.id: self.event.group}, rules={}, ) == Record( - self.record.key, - Notification(self.record.value.event, [], self.notification_uuid), - self.record.timestamp, + self.record.key, Notification(self.record.value.event, []), self.record.timestamp ) @region_silo_test(stable=True) class GroupRecordsTestCase(TestCase): - def setUp(self): - self.notification_uuid = str(uuid.uuid4()) - @cached_property def rule(self): return self.project.rule_set.all()[0] @@ -85,11 +76,7 @@ def test_success(self): ] group = events[0].group records = [ - Record( - event.event_id, - Notification(event, [self.rule], self.notification_uuid), - event.datetime, - ) + Record(event.event_id, Notification(event, [self.rule]), event.datetime) for event in events ] results: MutableMapping[str, Mapping[str, MutableSequence[Record]]] = defaultdict( diff --git a/tests/sentry/mail/test_adapter.py b/tests/sentry/mail/test_adapter.py index d6348ea591c9c8..31034d69f62688 100644 --- a/tests/sentry/mail/test_adapter.py +++ b/tests/sentry/mail/test_adapter.py @@ -1248,9 +1248,6 @@ def test_notify_digest(self, notify): message = mail.outbox[0] assert "List-ID" in message.message() - assert isinstance(message, EmailMultiAlternatives) - assert isinstance(message.alternatives[0][0], str) - assert "notification_uuid" in message.alternatives[0][0] @mock.patch.object(mail_adapter, "notify", side_effect=mail_adapter.notify, autospec=True) def test_notify_digest_replay_id(self, notify): @@ -1300,9 +1297,6 @@ def test_notify_digest_replay_id(self, notify): message = mail.outbox[0] assert "View Replays" in message.message().as_string() - assert isinstance(message, EmailMultiAlternatives) - assert isinstance(message.alternatives[0][0], str) - assert "notification_uuid" in message.alternatives[0][0] @mock.patch.object(mail_adapter, "notify", side_effect=mail_adapter.notify, autospec=True) def test_dont_notify_digest_snoozed(self, notify): diff --git a/tests/sentry/notifications/notifications/test_digests.py b/tests/sentry/notifications/notifications/test_digests.py index b3dec2ca6e4d84..4bb53a3b2ce87b 100644 --- a/tests/sentry/notifications/notifications/test_digests.py +++ b/tests/sentry/notifications/notifications/test_digests.py @@ -5,7 +5,6 @@ import responses from django.core import mail -from django.core.mail.message import EmailMultiAlternatives import sentry from sentry.digests.backends.base import Backend @@ -104,10 +103,6 @@ def test_sends_digest_to_every_member(self, mock_record): assert "N+1 Query" in mail.outbox[0].body assert "oh no" in mail.outbox[0].body assert self.build_occurrence_data()["issue_title"] in mail.outbox[0].body - message = mail.outbox[0] - assert isinstance(message, EmailMultiAlternatives) - assert isinstance(message.alternatives[0][0], str) - assert "notification_uuid" in message.alternatives[0][0] mock_record.assert_any_call( "integrations.email.notification_sent", category="digest", @@ -140,10 +135,6 @@ def test_sends_alert_rule_notification_to_each_member(self): # It is an alert rule notification, not a digest assert "new alerts since" not in mail.outbox[0].subject - message = mail.outbox[0] - assert isinstance(message, EmailMultiAlternatives) - assert isinstance(message.alternatives[0][0], str) - assert "notification_uuid" in message.alternatives[0][0] class DigestSlackNotification(SlackActivityNotificationTest): @@ -183,19 +174,8 @@ def test_slack_digest_notification(self, digests, mock_func): }, project_id=self.project.id, ) - notification_uuid = str(uuid.uuid4()) - backend.add( - key, - event_to_record(event, [rule], notification_uuid), - increment_delay=0, - maximum_delay=0, - ) - backend.add( - key, - event_to_record(event2, [rule], notification_uuid), - increment_delay=0, - maximum_delay=0, - ) + backend.add(key, event_to_record(event, [rule]), increment_delay=0, maximum_delay=0) + backend.add(key, event_to_record(event2, [rule]), increment_delay=0, maximum_delay=0) with self.tasks(): deliver_digest(key) @@ -209,5 +189,3 @@ def test_slack_digest_notification(self, digests, mock_func): == f" " ) assert len(attachments) == 2 - assert "notification_uuid" in attachments[0]["title_link"] - assert "notification_uuid" in attachments[1]["title_link"] diff --git a/tests/sentry/tasks/test_digests.py b/tests/sentry/tasks/test_digests.py index b66d4c6cd26a5d..f3deec29a4fc01 100644 --- a/tests/sentry/tasks/test_digests.py +++ b/tests/sentry/tasks/test_digests.py @@ -1,8 +1,6 @@ -import uuid from unittest import mock from django.core import mail -from django.core.mail.message import EmailMultiAlternatives import sentry from sentry.digests.backends.redis import RedisBackend @@ -32,51 +30,24 @@ def run_test(self, key: str) -> None: data={"timestamp": iso_format(before_now(days=1)), "fingerprint": ["group-2"]}, project_id=self.project.id, ) - notification_uuid = str(uuid.uuid4()) - backend.add( - key, - event_to_record(event, [rule], notification_uuid), - increment_delay=0, - maximum_delay=0, - ) - backend.add( - key, - event_to_record(event_2, [rule], notification_uuid), - increment_delay=0, - maximum_delay=0, - ) + backend.add(key, event_to_record(event, [rule]), increment_delay=0, maximum_delay=0) + backend.add(key, event_to_record(event_2, [rule]), increment_delay=0, maximum_delay=0) with self.tasks(): deliver_digest(key) assert "2 new alerts since" in mail.outbox[0].subject def test_old_key(self): self.run_test(f"mail:p:{self.project.id}") - message = mail.outbox[0] - assert isinstance(message, EmailMultiAlternatives) - assert isinstance(message.alternatives[0][0], str) - assert "notification_uuid" in message.alternatives[0][0] def test_new_key(self): self.run_test(f"mail:p:{self.project.id}:IssueOwners:") - message = mail.outbox[0] - assert isinstance(message, EmailMultiAlternatives) - assert isinstance(message.alternatives[0][0], str) - assert "notification_uuid" in message.alternatives[0][0] @with_feature("organizations:issue-alert-fallback-targeting") def test_fallthrough_choice_key(self): self.run_test(f"mail:p:{self.project.id}:IssueOwners::AllMembers") - message = mail.outbox[0] - assert isinstance(message, EmailMultiAlternatives) - assert isinstance(message.alternatives[0][0], str) - assert "notification_uuid" in message.alternatives[0][0] def test_member_key(self): self.run_test(f"mail:p:{self.project.id}:Member:{self.user.id}") - message = mail.outbox[0] - assert isinstance(message, EmailMultiAlternatives) - assert isinstance(message.alternatives[0][0], str) - assert "notification_uuid" in message.alternatives[0][0] def test_no_records(self): # This shouldn't error if no records are present