Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

♻️ ref: refactor notification building utils to prevent circular dependencies #88030

Merged
Merged
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
7 changes: 6 additions & 1 deletion src/sentry/integrations/discord/message_builder/issues.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,12 @@ def build(self, notification_uuid: str | None = None) -> dict[str, object]:
color=LEVEL_TO_COLOR[get_color(event_for_tags, self.notification, self.group)],
# We can't embed urls in Discord embed footers.
footer=DiscordMessageEmbedFooter(
build_footer(self.group, project, self.rules, "{text}")
build_footer(
group=self.group,
project=project,
url_format="{text}",
rules=self.rules,
)
),
fields=build_tag_fields(event_for_tags, self.tags),
timestamp=timestamp,
Expand Down
9 changes: 3 additions & 6 deletions src/sentry/integrations/messaging/message_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

from sentry import features
from sentry.eventstore.models import Event, GroupEvent
from sentry.integrations.slack.message_builder.types import LEVEL_TO_COLOR, SLACK_URL_FORMAT
from sentry.integrations.messaging.types import LEVEL_TO_COLOR
from sentry.integrations.types import EXTERNAL_PROVIDERS, ExternalProviders
from sentry.issues.grouptype import GroupCategory
from sentry.models.environment import Environment
Expand Down Expand Up @@ -173,7 +173,7 @@ def build_attachment_text(group: Group, event: Event | GroupEvent | None = None)


def build_attachment_replay_link(
group: Group, event: Event | GroupEvent | None = None, url_format: str = SLACK_URL_FORMAT
group: Group, url_format: str, event: Event | GroupEvent | None = None
Copy link
Member Author

Choose a reason for hiding this comment

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

enforcing u pass a url_format

) -> str | None:
has_replay = features.has("organizations:session-replay", group.organization)
has_slack_links = features.has(
Expand All @@ -199,8 +199,8 @@ def build_rule_url(rule: Any, group: Group, project: Project) -> str:
def build_footer(
group: Group,
project: Project,
url_format: str,
rules: Sequence[Rule] | None = None,
url_format: str = SLACK_URL_FORMAT,
) -> str:
footer = f"{group.qualified_short_id}"
if rules:
Expand All @@ -210,9 +210,6 @@ def build_footer(
text = rules[0].label if rules[0].label else "Test Alert"
footer += f" via {url_format.format(text=text, url=rule_url)}"

if url_format == SLACK_URL_FORMAT:
Copy link
Member Author

Choose a reason for hiding this comment

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

slack is clearly doing its own thing, so lets give it its own specific method instead of having this 1 tiny function do all the heavy lifting

footer = url_format.format(text=text, url=rule_url)

if len(rules) > 1:
footer += f" (+{len(rules) - 1} other)"

Expand Down
11 changes: 11 additions & 0 deletions src/sentry/integrations/messaging/types.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# Attachment colors used for issues with no actions take.

LEVEL_TO_COLOR = {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is generic, so lets not keep it with Slack

"_actioned_issue": "#EDEEEF",
"_incident_resolved": "#4DC771",
"debug": "#FBE14F",
"error": "#E03E2F",
"fatal": "#FA4747",
"info": "#2788CE",
"warning": "#FFC227",
}
7 changes: 6 additions & 1 deletion src/sentry/integrations/msteams/card_builder/issues.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,12 @@ def build_group_footer(self) -> ColumnSetBlock:

image_column = create_footer_logo_block()

text = build_footer(self.group, project, self.rules, MSTEAMS_URL_FORMAT)
text = build_footer(
group=self.group,
project=project,
url_format=MSTEAMS_URL_FORMAT,
rules=self.rules,
)

text_column = create_footer_column_block(create_footer_text_block(text))

Expand Down
7 changes: 2 additions & 5 deletions src/sentry/integrations/slack/message_builder/incidents.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
from datetime import datetime

from sentry.incidents.typings.metric_detector import AlertContext, MetricIssueContext
from sentry.integrations.messaging.types import LEVEL_TO_COLOR
from sentry.integrations.metric_alerts import incident_attachment_info
from sentry.integrations.slack.message_builder.base.block import BlockSlackMessageBuilder
from sentry.integrations.slack.message_builder.types import (
INCIDENT_COLOR_MAPPING,
LEVEL_TO_COLOR,
SlackBody,
)
from sentry.integrations.slack.message_builder.types import INCIDENT_COLOR_MAPPING, SlackBody
from sentry.integrations.slack.utils.escape import escape_slack_text
from sentry.models.organization import Organization

Expand Down
14 changes: 11 additions & 3 deletions src/sentry/integrations/slack/message_builder/issues.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
build_attachment_replay_link,
build_attachment_text,
build_attachment_title,
build_footer,
format_actor_option_slack,
format_actor_options_slack,
get_title_link,
Expand All @@ -32,6 +31,7 @@
SLACK_URL_FORMAT,
SlackBlock,
)
from sentry.integrations.slack.message_builder.util import build_slack_footer
from sentry.integrations.slack.utils.escape import escape_slack_markdown_text, escape_slack_text
from sentry.integrations.time_utils import get_approx_start_time, time_since
from sentry.integrations.types import ExternalProviders
Expand Down Expand Up @@ -506,7 +506,11 @@ def get_suggested_assignees_block(self, suggested_assignees: list[str]) -> Slack

def get_footer(self) -> SlackBlock:
# This link does not contain user input (it's a static label and a url), must not escape it.
replay_link = build_attachment_replay_link(self.group, self.event)
replay_link = build_attachment_replay_link(
group=self.group,
url_format=SLACK_URL_FORMAT,
event=self.event,
)

timestamp = None
if not self.issue_details:
Expand All @@ -517,7 +521,11 @@ def get_footer(self) -> SlackBlock:
footer = (
self.notification.build_notification_footer(self.recipient, ExternalProviders.SLACK)
if self.notification and self.recipient
else build_footer(self.group, project, self.rules, SLACK_URL_FORMAT)
else build_slack_footer(
group=self.group,
project=project,
rules=self.rules,
)
)

if not self.notification:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,9 @@
from sentry.incidents.models.alert_rule import AlertRule
from sentry.incidents.models.incident import Incident, IncidentStatus
from sentry.integrations.messaging.types import LEVEL_TO_COLOR
from sentry.integrations.metric_alerts import metric_alert_unfurl_attachment_info
from sentry.integrations.slack.message_builder.base.block import BlockSlackMessageBuilder
from sentry.integrations.slack.message_builder.types import (
INCIDENT_COLOR_MAPPING,
LEVEL_TO_COLOR,
SlackBody,
)
from sentry.integrations.slack.message_builder.types import INCIDENT_COLOR_MAPPING, SlackBody
from sentry.integrations.slack.utils.escape import escape_slack_text


Expand Down
11 changes: 0 additions & 11 deletions src/sentry/integrations/slack/message_builder/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,6 @@
SlackBlock = dict[str, Any]
SlackBody = Union[SlackAttachment, SlackBlock]

# Attachment colors used for issues with no actions take.
Copy link
Member Author

Choose a reason for hiding this comment

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

moved outside of slack since its generic

LEVEL_TO_COLOR = {
"_actioned_issue": "#EDEEEF",
"_incident_resolved": "#4DC771",
"debug": "#FBE14F",
"error": "#E03E2F",
"fatal": "#FA4747",
"info": "#2788CE",
"warning": "#FFC227",
}

INCIDENT_COLOR_MAPPING = {
"Resolved": "_incident_resolved",
"Warning": "warning",
Expand Down
27 changes: 27 additions & 0 deletions src/sentry/integrations/slack/message_builder/util.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
from collections.abc import Sequence

from sentry.integrations.messaging.message_builder import build_rule_url
from sentry.integrations.slack.message_builder.types import SLACK_URL_FORMAT
from sentry.models.group import Group
from sentry.models.project import Project
from sentry.models.rule import Rule


def build_slack_footer(
Copy link
Member Author

Choose a reason for hiding this comment

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

slack gets its own method thats specific to it since it styles things differently

group: Group,
project: Project,
rules: Sequence[Rule] | None = None,
) -> str:
footer = f"{group.qualified_short_id}"

if rules:
rule_url = build_rule_url(rules[0], group, project)
# If this notification is triggered via the "Send Test Notification"
# button then the label is not defined, but the url works.
text = rules[0].label if rules[0].label else "Test Alert"
footer = SLACK_URL_FORMAT.format(text=text, url=rule_url)

if len(rules) > 1:
footer += f" (+{len(rules) - 1} other)"
Comment on lines +16 to +25
Copy link
Member

Choose a reason for hiding this comment

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

A new unit test on this util would be appreciated.


return footer
2 changes: 1 addition & 1 deletion src/sentry_plugins/slack/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from sentry import tagstore
from sentry.integrations.base import FeatureDescription, IntegrationFeatures
from sentry.integrations.slack.message_builder.types import LEVEL_TO_COLOR
from sentry.integrations.messaging.types import LEVEL_TO_COLOR
from sentry.plugins.base.structs import Notification
from sentry.plugins.bases import notify
from sentry.shared_integrations.exceptions import ApiError
Expand Down
2 changes: 1 addition & 1 deletion tests/sentry/integrations/discord/test_issue_alert.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ def test_basic(self, mock_record):
notification_uuid=notification_uuid,
),
"color": LEVEL_TO_COLOR["error"],
"footer": {"text": build_footer(self.event.group, self.event.project, None, "{text}")},
Copy link
Member Author

Choose a reason for hiding this comment

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

reorder required because of optional args

"footer": {"text": build_footer(self.event.group, self.event.project, "{text}", None)},
"fields": [],
"timestamp": self.event.timestamp,
}
Expand Down
2 changes: 1 addition & 1 deletion tests/sentry/integrations/slack/test_message_builder.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
build_attachment_text,
build_attachment_title,
)
from sentry.integrations.messaging.types import LEVEL_TO_COLOR
from sentry.integrations.slack.message_builder.incidents import SlackIncidentsMessageBuilder
from sentry.integrations.slack.message_builder.issues import (
SlackIssuesMessageBuilder,
Expand All @@ -32,7 +33,6 @@
get_tags,
)
from sentry.integrations.slack.message_builder.metric_alerts import SlackMetricAlertMessageBuilder
from sentry.integrations.slack.message_builder.types import LEVEL_TO_COLOR
from sentry.integrations.time_utils import time_since
from sentry.issues.grouptype import (
FeedbackGroup,
Expand Down
2 changes: 1 addition & 1 deletion tests/sentry_plugins/slack/test_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import pytest
import responses

from sentry.integrations.slack.message_builder.types import LEVEL_TO_COLOR
from sentry.integrations.messaging.types import LEVEL_TO_COLOR
from sentry.models.rule import Rule
from sentry.plugins.base import Notification
from sentry.shared_integrations.exceptions import ApiError
Expand Down
Loading