Skip to content
Merged
23 changes: 18 additions & 5 deletions src/sentry/integrations/slack/integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,12 @@ def send_notification(
client = self.get_client()
try:
client.chat_postMessage(
channel=target.resource_id, blocks=payload["blocks"], text=payload["text"]
channel=target.resource_id,
blocks=payload["blocks"] if len(payload["blocks"]) > 0 else None,
text=payload["text"],
attachments=payload.get("attachments"),
unfurl_links=False,
unfurl_media=False,
)
except SlackApiError as e:
translate_slack_api_error(e)
Comment thread
sentry[bot] marked this conversation as resolved.
Expand All @@ -132,8 +137,11 @@ def send_notification_with_threading(
)
kwargs: dict[str, Any] = dict(
channel=target.resource_id,
blocks=payload["blocks"],
blocks=payload["blocks"] if len(payload["blocks"]) > 0 else None,
text=payload["text"],
attachments=payload.get("attachments"),
unfurl_links=False,
unfurl_media=False,
)

if threading_context.thread_ts is not None:
Expand All @@ -159,8 +167,9 @@ def send_threaded_message(
try:
client.chat_postMessage(
channel=channel_id,
blocks=renderable["blocks"],
blocks=renderable["blocks"] if len(renderable["blocks"]) > 0 else None,
text=renderable["text"],
attachments=renderable.get("attachments"),
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.

Inconsistent unfurl flags across Slack message methods

Low Severity

This commit adds unfurl_links=False and unfurl_media=False to send_notification, send_notification_with_threading, and update_message, but omits them from send_threaded_message and send_threaded_ephemeral_message. Previously none of these methods set unfurl flags, so they were all consistent. Now threaded messages will still show link previews while non-threaded ones won't.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 0b40258. Configure here.

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.

Diverging code paths used by different callers with different expectations.

thread_ts=thread_ts,
)
except SlackApiError as e:
Expand All @@ -178,7 +187,8 @@ def send_threaded_ephemeral_message(
try:
client.chat_postEphemeral(
channel=channel_id,
blocks=renderable["blocks"],
blocks=renderable["blocks"] if len(renderable["blocks"]) > 0 else None,
attachments=renderable.get("attachments"),
text=renderable["text"],
thread_ts=thread_ts,
user=slack_user_id,
Expand All @@ -199,7 +209,10 @@ def update_message(
channel=channel_id,
ts=message_ts,
text=renderable["text"],
blocks=renderable["blocks"],
blocks=renderable["blocks"] if len(renderable["blocks"]) > 0 else None,
attachments=renderable.get("attachments"),
unfurl_links=False,
unfurl_media=False,
)
except SlackApiError as e:
translate_slack_api_error(e)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
from sentry.notifications.platform.email.provider import EmailRenderer
from sentry.notifications.platform.msteams.provider import MSTeamsRenderable, MSTeamsRenderer
from sentry.notifications.platform.registry import template_registry
from sentry.notifications.platform.slack.provider import SlackRenderer
from sentry.notifications.platform.slack.provider import SlackNotificationProvider
from sentry.notifications.platform.types import (
NotificationData,
NotificationProviderKey,
Expand Down Expand Up @@ -92,13 +92,14 @@ def serialize_slack_preview[T: NotificationData](
) -> dict[str, Any]:
data = template.example_data
rendered_template = template.render_example()
message = SlackRenderer.render(data=data, rendered_template=rendered_template)
renderer = SlackNotificationProvider.get_renderer(data=data, category=template.category)
message = renderer.render(data=data, rendered_template=rendered_template)

serialized_blocks = []
for block in message.get("blocks", []):
serialized_blocks.append(block.to_dict())
Comment thread
sentry-warden[bot] marked this conversation as resolved.

return {"blocks": serialized_blocks}
return {"blocks": serialized_blocks, "attachments": message.get("attachments")}


def serialize_discord_preview[T: NotificationData](
Expand Down
4 changes: 2 additions & 2 deletions src/sentry/notifications/platform/slack/provider.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from __future__ import annotations

from dataclasses import dataclass
from typing import TYPE_CHECKING, NotRequired, TypedDict
from typing import TYPE_CHECKING, Any, NotRequired, TypedDict

from slack_sdk.models.blocks import (
ActionsBlock,
Expand Down Expand Up @@ -58,8 +58,8 @@ class SlackProviderThreadingContext(ProviderThreadingContext):

class SlackRenderable(TypedDict):
blocks: list[Block]
attachments: NotRequired[list[dict[str, Any]]]
text: str
color: NotRequired[str]


class SlackRenderer(NotificationRenderer[SlackRenderable]):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,15 @@ def render[DataT: NotificationData](
*blocks, fallback_text=fallback_text, color=color
)

attachment_blocks = [{"blocks": slack_body.get("blocks", [])}]

if color:
attachment_blocks[0]["color"] = color

renderable = SlackRenderable(
blocks=slack_body.get("blocks", []),
blocks=[],
attachments=attachment_blocks,
text=slack_body.get("text", ""),
)
if (color := slack_body.get("color")) is not None:
renderable["color"] = color

return renderable
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ class MetricAlertNotificationData(NotificationData):
@template_registry.register(NotificationSource.METRIC_ALERT)
class MetricAlertNotificationTemplate(NotificationTemplate[MetricAlertNotificationData]):
category = NotificationCategory.METRIC_ALERT
hide_from_debugger = True
example_data = MetricAlertNotificationData(
group_id=1,
organization_id=1,
Expand Down
5 changes: 5 additions & 0 deletions tests/sentry/integrations/slack/test_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -544,6 +544,9 @@ def test_send_notification_success(self, mock_chat_post: MagicMock) -> None:
channel="C1234567890",
blocks=self.slack_renderable.get("blocks", []),
text="Mock Notification",
attachments=None,
unfurl_links=False,
unfurl_media=False,
)

@patch("sentry.integrations.slack.sdk_client.SlackSdkClient.chat_postMessage")
Expand All @@ -569,6 +572,7 @@ def test_send_threaded_message_success(self, mock_chat_post: MagicMock) -> None:
channel=self.channel_id,
thread_ts=self.thread_ts,
blocks=self.slack_renderable.get("blocks", []),
attachments=self.slack_renderable.get("attachments"),
text=self.slack_renderable.get("text", ""),
)

Expand Down Expand Up @@ -596,6 +600,7 @@ def test_send_threaded_ephemeral_message_success(self, mock_chat_ephemeral: Magi
mock_chat_ephemeral.assert_called_once_with(
channel=self.channel_id,
thread_ts=self.thread_ts,
attachments=self.slack_renderable.get("attachments"),
user=self.slack_user_id,
blocks=self.slack_renderable.get("blocks", []),
text=self.slack_renderable.get("text", ""),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,10 @@ def test_send_alert_via_np_sends_to_slack_channel(
mock_client_instance.chat_postMessage.assert_called_once()
call_kwargs = mock_client_instance.chat_postMessage.call_args.kwargs
assert call_kwargs["channel"] == "channel123"
blocks = call_kwargs["blocks"]
assert call_kwargs["attachments"] is not None
attachments: list[Any] = call_kwargs["attachments"]
assert len(attachments) == 1
blocks: list[Any] = attachments[0]["blocks"]
assert len(blocks) >= 1
assert blocks[0]["type"] == "section"
assert blocks[0]["text"]["type"] == "mrkdwn"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ def test_render_produces_blocks(self) -> None:
)

# Without a chart: exactly one section block
blocks: list[Any] = result["blocks"]
assert result.get("attachments") is not None
attachments: list[Any] = result["attachments"]
assert len(attachments) == 1
blocks: list[Any] = attachments[0]["blocks"]
assert len(blocks) == 1
assert blocks[0]["type"] == "section"
assert blocks[0]["text"]["type"] == "mrkdwn"
Expand All @@ -120,8 +123,10 @@ def test_render_includes_image_block_when_chart_url_set(self) -> None:
rendered_template=self.rendered_template,
)

assert result.get("attachments") is not None

# With a chart: section block + image block
blocks: list[Any] = result["blocks"]
blocks: list[Any] = result["attachments"][0]["blocks"]
assert len(blocks) == 2
assert blocks[0]["type"] == "section"
assert "123.45 events in the last minute" in blocks[0]["text"]["text"]
Expand All @@ -135,7 +140,10 @@ def test_render_without_chart_url(self) -> None:
rendered_template=self.rendered_template,
)

blocks: list[Any] = result["blocks"]
assert result.get("attachments") is not None
attachments: list[Any] = result["attachments"]
assert len(attachments) == 1
blocks: list[Any] = attachments[0]["blocks"]
assert len(blocks) == 1
assert blocks[0]["type"] == "section"

Expand Down
23 changes: 21 additions & 2 deletions tests/sentry/notifications/platform/slack/test_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,12 @@ def test_send_success(self, mock_slack_client: Mock) -> None:
assert isinstance(result, SendSuccessResult)
assert result.provider_message_id is None
mock_client_instance.chat_postMessage.assert_called_once_with(
channel="C1234567890", blocks=renderable["blocks"], text=renderable["text"]
channel="C1234567890",
blocks=renderable["blocks"],
text=renderable["text"],
attachments=None,
unfurl_links=False,
unfurl_media=False,
)

def test_send_invalid_target_class(self) -> None:
Expand Down Expand Up @@ -185,7 +190,12 @@ def test_send_to_direct_message(self, mock_slack_client: Mock) -> None:
SlackNotificationProvider.send(target=target, renderable=renderable)

mock_client_instance.chat_postMessage.assert_called_once_with(
channel="U1234567890", blocks=renderable["blocks"], text=renderable["text"]
channel="U1234567890",
blocks=renderable["blocks"],
text=renderable["text"],
attachments=None,
unfurl_links=False,
unfurl_media=False,
)


Expand Down Expand Up @@ -278,6 +288,9 @@ def test_send_with_thread_context_existing_thread(self, mock_slack_client: Mock)
blocks=renderable["blocks"],
text=renderable["text"],
thread_ts="1111111111.111111",
attachments=None,
unfurl_links=False,
unfurl_media=False,
)

@patch("sentry.integrations.slack.integration.SlackSdkClient")
Expand Down Expand Up @@ -316,6 +329,9 @@ def test_send_with_thread_context_reply_broadcast(self, mock_slack_client: Mock)
text=renderable["text"],
thread_ts="1111111111.111111",
reply_broadcast=True,
attachments=None,
unfurl_links=False,
unfurl_media=False,
)

@patch("sentry.integrations.slack.integration.SlackSdkClient")
Expand Down Expand Up @@ -349,6 +365,9 @@ def test_send_with_thread_context_no_existing_thread(self, mock_slack_client: Mo
channel="C1234567890",
blocks=renderable["blocks"],
text=renderable["text"],
attachments=None,
unfurl_links=False,
unfurl_media=False,
)

@patch("sentry.integrations.slack.integration.SlackSdkClient")
Expand Down
Loading