Skip to content

Commit

Permalink
Revert "feat: Add notification_uuid to digest URLs (#55951)"
Browse files Browse the repository at this point in the history
This reverts commit 2a46053.

Co-authored-by: scttcper <1400464+scttcper@users.noreply.github.com>
  • Loading branch information
getsentry-bot and scttcper committed Sep 13, 2023
1 parent 77f5e5b commit 6477a19
Show file tree
Hide file tree
Showing 9 changed files with 19 additions and 123 deletions.
14 changes: 4 additions & 10 deletions src/sentry/digests/notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

logger = logging.getLogger("sentry.digests")

Notification = namedtuple("Notification", "event rules notification_uuid")
Notification = namedtuple("Notification", "event rules")


def split_key(
Expand Down Expand Up @@ -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),
)

Expand Down Expand Up @@ -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,
)

Expand Down
4 changes: 2 additions & 2 deletions src/sentry/mail/adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
13 changes: 2 additions & 11 deletions src/sentry/notifications/notifications/digest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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,
}
Expand Down
20 changes: 2 additions & 18 deletions src/sentry/tasks/digests.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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
Expand All @@ -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(
Expand All @@ -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
5 changes: 1 addition & 4 deletions src/sentry/web/frontend/debug/mail.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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),
)
Expand All @@ -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)),
Expand All @@ -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)),
Expand Down
21 changes: 4 additions & 17 deletions tests/sentry/digests/test_notifications.py
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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(
Expand All @@ -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,
)

Expand All @@ -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]
Expand All @@ -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(
Expand Down
6 changes: 0 additions & 6 deletions tests/sentry/mail/test_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down
26 changes: 2 additions & 24 deletions tests/sentry/notifications/notifications/test_digests.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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)

Expand All @@ -209,5 +189,3 @@ def test_slack_digest_notification(self, digests, mock_func):
== f"<!date^{timestamp_secs}^2 issues detected {{date_pretty}} in| Digest Report for> <http://testserver/organizations/{self.organization.slug}/projects/{self.project.slug}/|{self.project.name}>"
)
assert len(attachments) == 2
assert "notification_uuid" in attachments[0]["title_link"]
assert "notification_uuid" in attachments[1]["title_link"]
33 changes: 2 additions & 31 deletions tests/sentry/tasks/test_digests.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 6477a19

Please sign in to comment.