Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 2 additions & 17 deletions src/sentry/notifications/notifications/digest.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from collections.abc import Mapping, MutableMapping, Sequence
from datetime import UTC, tzinfo
from typing import TYPE_CHECKING, Any
from urllib.parse import urlencode

import sentry_sdk

Expand Down Expand Up @@ -154,22 +153,8 @@ def get_context(self) -> MutableMapping[str, Any]:
notification_uuid=self.notification_uuid,
)

sentry_query_params = self.get_sentry_query_params(ExternalProviders.EMAIL)

if not features.has("organizations:workflow-engine-ui", self.project.organization):
# TODO(iamrajjoshi): This actually mutes a rule for a user, something we have not ported over in the new system
# By not including this context, the template will not show the mute button
snooze_alert = len(rule_details) > 0
snooze_alert_urls = {
rule.id: f"{rule.status_url}{sentry_query_params}&{urlencode({'mute': '1'})}"
for rule in rule_details
}

context["snooze_alert"] = snooze_alert
context["snooze_alert_urls"] = snooze_alert_urls
else:
context["snooze_alert"] = False
context["snooze_alert_urls"] = {}
context["snooze_alert"] = False
context["snooze_alert_urls"] = {}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we don't even need this going forward since there is no snoozing?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah! I think we'll be able to start removing this stuff. 🎉


return context

Expand Down
18 changes: 3 additions & 15 deletions src/sentry/notifications/notifications/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
get_integration_link,
get_issue_replay_link,
get_rules,
get_snooze_url,
)
from sentry.notifications.utils.participants import get_owner_reason, get_send_to
from sentry.notifications.utils.rules import get_rule_or_workflow_id
Expand Down Expand Up @@ -270,20 +269,9 @@ def get_context(self) -> MutableMapping[str, Any]:
},
)

# We don't show the snooze alert if the organization has not enabled the workflow engine UI because in the new UI/system a user can't individually disable a workflow
if not features.has("organizations:workflow-engine-ui", self.organization):
if len(self.rules) > 0:
context["snooze_alert"] = True
context["snooze_alert_url"] = get_snooze_url(
self.rules[0],
self.organization,
self.project,
sentry_query_params,
self.group.type,
)
else:
context["snooze_alert"] = False
context["snooze_alert_url"] = None
# We don't show the snooze alert because in the new UI/system a user can't individually disable a workflow
context["snooze_alert"] = False
context["snooze_alert_url"] = None

if isinstance(self.event, GroupEvent) and self.event.occurrence:
context["issue_title"] = self.event.occurrence.issue_title
Expand Down
7 changes: 5 additions & 2 deletions tests/acceptance/test_incidents.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"organizations:incidents",
"organizations:performance-view",
"organizations:visibility-explore-view",
"organizations:workflow-engine-ui",
]


Expand Down Expand Up @@ -59,7 +60,9 @@ def test_incidents_list(self) -> None:
details_url = f'[href="/organizations/{self.organization.slug}/issues/alerts/rules/details/{alert_rule.id}/?alert={incident.id}'
self.browser.wait_until(details_url)
self.browser.click(details_url)
self.browser.wait_until_not('[data-test-id="loading-indicator"]')
self.browser.wait_until_test_id("incident-rule-title")

self.browser.wait_until_script_execution(
f'return window.location.pathname === "/organizations/{self.organization.slug}/issues/{group.id}/"'
)
self.browser.wait_until_not('[data-test-id="loading-indicator"]')
self.browser.wait_until_not('[data-test-id="loading-placeholder"]')
3 changes: 0 additions & 3 deletions tests/sentry/incidents/test_charts.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ def test_eap_alert(self, mock_client_get: MagicMock, mock_generate_chart: MagicM

@patch("sentry.charts.backend.generate_chart", return_value="chart-url")
@patch("sentry.incidents.charts.client.get")
@with_feature("organizations:workflow-engine-ui")
def test_eap_log_alert(
self, mock_client_get: MagicMock, mock_generate_chart: MagicMock
) -> None:
Expand Down Expand Up @@ -274,7 +273,6 @@ def test_pending_deletion_detector_not_resolved_to_alert_rule(self) -> None:

@freeze_time(frozen_time)
@with_feature("organizations:incidents")
@with_feature("organizations:workflow-engine-ui")
def test_use_open_period_serializer(self) -> None:
detector = self.create_detector(project=self.project)
group = self.create_group(type=MetricIssue.type_id, priority=PriorityLevel.HIGH)
Expand Down Expand Up @@ -307,7 +305,6 @@ def test_use_open_period_serializer(self) -> None:

@freeze_time(frozen_time)
@with_feature("organizations:incidents")
@with_feature("organizations:workflow-engine-ui")
def test_use_open_period_serializer_with_offset(self) -> None:
group = self.create_group(type=MetricIssue.type_id, priority=PriorityLevel.HIGH)

Expand Down
63 changes: 0 additions & 63 deletions tests/sentry/mail/test_adapter.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
assert_last_analytics_event,
)
from sentry.testutils.helpers.datetime import before_now
from sentry.testutils.helpers.features import with_feature
from sentry.testutils.silo import assume_test_silo_mode
from sentry.testutils.skips import requires_snuba
from sentry.types.activity import ActivityType
Expand Down Expand Up @@ -560,68 +559,6 @@ def test_notify_users_does_email(self, mock_logger, mock_func) -> None:
self.assertEqual(notification.reference, group)
assert notification.get_subject() == "BAR-1 - hello world"

assert notification.get_context()["snooze_alert"] is True

assert group
mock_logger.info.assert_called_with(
"mail.adapter.notify",
extra={
"target_type": "IssueOwners",
"target_identifier": None,
"group": group.id,
"project_id": group.project.id,
"organization": group.organization.id,
"fallthrough_choice": "ActiveMembers",
"notification_uuid": mock.ANY,
},
)

@mock_notify
@mock.patch("sentry.notifications.notifications.rules.logger")
@with_feature("organizations:workflow-engine-ui") # snooze

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the only test we had for workflow_engine + email? if so, instead of removing it, should we update the test to only use the workflow models / calls to ensure the email is sent?

def test_notify_users_does_email_workflow_engine_ui(self, mock_logger, mock_func) -> None:
self.create_user_option(user=self.user, key="timezone", value="Europe/Vienna")
event_manager = EventManager({"message": "hello world", "level": "error"})
event_manager.normalize()
event_data = event_manager.get_data()
event_type = get_event_type(event_data)
event_data["type"] = event_type.key
event_data["metadata"] = event_type.get_metadata(event_data)

event = event_manager.save(self.project.id)
group = event.group

self.create_notification_settings_provider(
user_id=self.user.id,
scope_type="user",
scope_identifier=self.user.id,
provider="slack",
type="alerts",
value="never",
)
ProjectOwnership.objects.create(project_id=self.project.id, fallthrough=True)
rule = self.create_project_rule(
project=self.project, action_data=[{"workflow_id": "1234567890"}]
)
with self.tasks():
AlertRuleNotification(
Notification(event=event, rules=[rule]),
ActionTargetType.ISSUE_OWNERS,
fallthrough_choice=FallthroughChoiceType.ACTIVE_MEMBERS,
).send()

assert mock_func.call_count == 1

args, kwargs = mock_func.call_args
notification = args[1]

recipient_context = notification.get_recipient_context(Actor.from_orm_user(self.user), {})
assert recipient_context["timezone"] == zoneinfo.ZoneInfo("Europe/Vienna")

self.assertEqual(notification.project, self.project)
self.assertEqual(notification.reference, group)
assert notification.get_subject() == "BAR-1 - hello world"

# Because we are using the workflow engine, the snooze_alert context should be False
# This is because a user cannot snooze a workflow for themselves
assert notification.get_context()["snooze_alert"] is False
Expand Down
3 changes: 1 addition & 2 deletions tests/sentry/notifications/notifications/test_digests.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,7 @@ def test_sends_alert_rule_notification_to_each_member(self) -> None:
assert "notification_uuid" in message.alternatives[0][0]

def test_sends_digest_with_workflow_engine_ui(self) -> None:
with self.feature("organizations:workflow-engine-ui"):
self.run_test(event_count=2)
self.run_test(event_count=2)
assert "new alerts since" in mail.outbox[0].subject

def test_digest_email_uses_user_timezone(self) -> None:
Expand Down
Loading