From ebfe7ac806353de474cccf3ea085d9fe0f19b879 Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Thu, 2 May 2024 12:28:29 -0700 Subject: [PATCH 01/12] refactor(slack): Code cleanup --- src/sentry/notifications/utils/actions.py | 40 ++++++++++------------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/src/sentry/notifications/utils/actions.py b/src/sentry/notifications/utils/actions.py index effcd8da7856f5..878681ebcd8687 100644 --- a/src/sentry/notifications/utils/actions.py +++ b/src/sentry/notifications/utils/actions.py @@ -5,40 +5,34 @@ from typing import Any, Literal -@dataclass -class MessageAction: - name: str - - # Optional label. This falls back to name. - label: str | None = None +@dataclass(kw_only=True) +class _BaseMessageAction: + """ + Base class used to hold the fields for a notification message action. + TODO(ecosystem): this class seems to only be used for Slack notifications, can we move this? + """ + name: str type: Literal["button", "select"] = "button" - - # If this is a button type, a url is required. + # If the message action is a button type, the url is required url: str | None = None - - # If this is a select type, the selected value. + # If the message action is a select type, this is the selected value value: str | None = None - # Denotes the type of action action_id: str | None = None + block_id: str | None = None + selected_options: Sequence[Mapping[str, Any]] | None = None - style: Literal["primary", "danger", "default"] | None = None - # TODO(mgaeta): Refactor this to be provider-agnostic - selected_options: Sequence[Mapping[str, Any]] | None = None +@dataclass +class MessageAction(_BaseMessageAction): + # Label is optional, if empty it falls back to name + label: str | None = None + style: Literal["primary", "danger", "default"] | None = None option_groups: Sequence[Mapping[str, Any]] | None = None - block_id: str | None = None elements: Sequence[Mapping[str, Any]] | None = None @dataclass -class BlockKitMessageAction: - name: str +class BlockKitMessageAction(_BaseMessageAction): label: str - type: Literal["button", "select"] = "button" - url: str | None = None - value: str | None = None - action_id: str | None = None - block_id: str | None = None - selected_options: Sequence[Mapping[str, Any]] | None = None From fe9208e9db44d4e85f601d44ac915c88431ea27d Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Thu, 2 May 2024 14:50:04 -0700 Subject: [PATCH 02/12] remove unused code --- .../slack/message_builder/base/base.py | 31 ++----------------- src/sentry/notifications/utils/actions.py | 3 +- 2 files changed, 3 insertions(+), 31 deletions(-) diff --git a/src/sentry/integrations/slack/message_builder/base/base.py b/src/sentry/integrations/slack/message_builder/base/base.py index 4b696b7ba3472e..b5971b3ed82333 100644 --- a/src/sentry/integrations/slack/message_builder/base/base.py +++ b/src/sentry/integrations/slack/message_builder/base/base.py @@ -1,31 +1,10 @@ from __future__ import annotations from abc import ABC -from collections.abc import Mapping, MutableMapping -from typing import Any from sentry.eventstore.models import Event, GroupEvent from sentry.integrations.slack.message_builder import SlackBody from sentry.models.group import Group -from sentry.notifications.utils.actions import MessageAction - - -def get_slack_button(action: MessageAction) -> Mapping[str, Any]: - kwargs: MutableMapping[str, Any] = { - "text": action.label or action.name, - "name": action.name, - "type": action.type, - } - for field in ("style", "url", "value", "action_id"): - value = getattr(action, field, None) - if value: - kwargs[field] = value - - if action.type == "select": - kwargs["selected_options"] = action.selected_options or [] - kwargs["option_groups"] = action.option_groups or [] - - return kwargs class SlackMessageBuilder(ABC): @@ -33,17 +12,11 @@ def build(self) -> SlackBody: """Abstract `build` method that all inheritors must implement.""" raise NotImplementedError - def build_fallback_text(self, obj: Group | Event | GroupEvent, project_slug: str) -> str: + @classmethod + def build_fallback_text(cls, obj: Group | Event | GroupEvent, project_slug: str) -> str: """Fallback text is used in the message preview popup.""" title = obj.title if isinstance(obj, GroupEvent) and obj.occurrence is not None: title = obj.occurrence.issue_title return f"[{project_slug}] {title}" - - @property - def escape_text(self) -> bool: - """ - Returns True if we need to escape the text in the message. - """ - return False diff --git a/src/sentry/notifications/utils/actions.py b/src/sentry/notifications/utils/actions.py index 878681ebcd8687..617e8c310ffa1f 100644 --- a/src/sentry/notifications/utils/actions.py +++ b/src/sentry/notifications/utils/actions.py @@ -8,8 +8,7 @@ @dataclass(kw_only=True) class _BaseMessageAction: """ - Base class used to hold the fields for a notification message action. - TODO(ecosystem): this class seems to only be used for Slack notifications, can we move this? + Base class used to hold the fields for a notification message action """ name: str From c28ac44a30cfa65e80282edbc17858e6f7fc2513 Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Thu, 2 May 2024 15:07:13 -0700 Subject: [PATCH 03/12] move pieces together, wip --- .../slack/message_builder/base/block.py | 38 ------------------- .../slack/message_builder/issues.py | 2 +- .../message_builder/notifications/base.py | 2 +- src/sentry/notifications/utils/actions.py | 29 ++++++++++++++ 4 files changed, 31 insertions(+), 40 deletions(-) diff --git a/src/sentry/integrations/slack/message_builder/base/block.py b/src/sentry/integrations/slack/message_builder/base/block.py index a5ac988fdb8e30..4c0d37876fcd2d 100644 --- a/src/sentry/integrations/slack/message_builder/base/block.py +++ b/src/sentry/integrations/slack/message_builder/base/block.py @@ -7,7 +7,6 @@ from sentry.integrations.slack.message_builder import SlackBlock from sentry.integrations.slack.message_builder.base.base import SlackMessageBuilder -from sentry.notifications.utils.actions import MessageAction class BlockSlackMessageBuilder(SlackMessageBuilder, ABC): @@ -83,43 +82,6 @@ def get_external_select_action(action, initial_option): return action - @staticmethod - def get_button_action(action: MessageAction) -> SlackBlock: - button_text = action.label or action.name - button = { - "type": "button", - "text": {"type": "plain_text", "text": button_text}, - } - if action.value: - button["action_id"] = action.value - button["value"] = action.value - - if action.action_id: - button["action_id"] = action.action_id - - if action.url: - button["url"] = action.url - button["value"] = "link_clicked" - - return button - - @staticmethod - def get_link_button(action: MessageAction) -> SlackBlock: - return { - "type": "section", - "text": { - "type": "mrkdwn", - "text": action.label, - }, - "accessory": { - "type": "button", - "text": {"type": "plain_text", "text": action.name, "emoji": True}, - "style": action.style, - "value": action.value, - "url": action.url, - }, - } - @staticmethod def get_action_block(actions: Sequence[tuple[str, str | None, str]]) -> SlackBlock: class SlackBlockType(TypedDict): diff --git a/src/sentry/integrations/slack/message_builder/issues.py b/src/sentry/integrations/slack/message_builder/issues.py index 1b9f9854538204..150a2ee942d239 100644 --- a/src/sentry/integrations/slack/message_builder/issues.py +++ b/src/sentry/integrations/slack/message_builder/issues.py @@ -596,7 +596,7 @@ def build(self, notification_uuid: str | None = None) -> SlackBlock: "Unresolve", "Resolve...", ): - actions.append(self.get_button_action(action)) + actions.append(action.get_button()) elif action.name == "assign": actions.append( self.get_external_select_action( diff --git a/src/sentry/integrations/slack/message_builder/notifications/base.py b/src/sentry/integrations/slack/message_builder/notifications/base.py index 1d81c2f7d78009..14a3b41ed724cf 100644 --- a/src/sentry/integrations/slack/message_builder/notifications/base.py +++ b/src/sentry/integrations/slack/message_builder/notifications/base.py @@ -59,7 +59,7 @@ def build(self) -> SlackBlock: actions_block = [] for action in actions: - actions_block.append(self.get_button_action(action)) + actions_block.append(action.get_button()) if actions_block: blocks.append({"type": "actions", "elements": [action for action in actions_block]}) diff --git a/src/sentry/notifications/utils/actions.py b/src/sentry/notifications/utils/actions.py index 617e8c310ffa1f..5001437f6fb2e6 100644 --- a/src/sentry/notifications/utils/actions.py +++ b/src/sentry/notifications/utils/actions.py @@ -4,6 +4,8 @@ from dataclasses import dataclass from typing import Any, Literal +from sentry.integrations.slack import SlackBlock + @dataclass(kw_only=True) class _BaseMessageAction: @@ -22,6 +24,27 @@ class _BaseMessageAction: block_id: str | None = None selected_options: Sequence[Mapping[str, Any]] | None = None + def _get_button_text(self) -> str: + return self.name + + def get_button(self) -> SlackBlock: + button = { + "type": "button", + "text": {"type": "plain_text", "text": self._get_button_text()}, + } + if self.value: + button["action_id"] = self.value + button["value"] = self.value + + if self.action_id: + button["action_id"] = self.action_id + + if self.url: + button["url"] = self.url + button["value"] = "link_clicked" + + return button + @dataclass class MessageAction(_BaseMessageAction): @@ -31,7 +54,13 @@ class MessageAction(_BaseMessageAction): option_groups: Sequence[Mapping[str, Any]] | None = None elements: Sequence[Mapping[str, Any]] | None = None + def _get_button_text(self) -> str: + return self.label or self.name + @dataclass class BlockKitMessageAction(_BaseMessageAction): label: str + + def _get_button_text(self) -> str: + return self.label or self.name From 0c7da1aaa58dd9c005a52f874a3d7851be681ea8 Mon Sep 17 00:00:00 2001 From: Cathy Teng Date: Thu, 2 May 2024 15:12:18 -0700 Subject: [PATCH 04/12] remove unused MessageBuilder --- src/sentry/integrations/slack/__init__.py | 1 - .../slack/message_builder/event.py | 19 ------------------- 2 files changed, 20 deletions(-) delete mode 100644 src/sentry/integrations/slack/message_builder/event.py diff --git a/src/sentry/integrations/slack/__init__.py b/src/sentry/integrations/slack/__init__.py index 3b598f2faeb93e..dcb38d1d7c1f9a 100644 --- a/src/sentry/integrations/slack/__init__.py +++ b/src/sentry/integrations/slack/__init__.py @@ -10,7 +10,6 @@ from .message_builder.base.block import * # noqa: F401,F403 from .message_builder.disconnected import * # noqa: F401,F403 from .message_builder.discover import * # noqa: F401,F403 -from .message_builder.event import * # noqa: F401,F403 from .message_builder.help import * # noqa: F401,F403 from .message_builder.incidents import * # noqa: F401,F403 from .message_builder.issues import * # noqa: F401,F403 diff --git a/src/sentry/integrations/slack/message_builder/event.py b/src/sentry/integrations/slack/message_builder/event.py deleted file mode 100644 index bf12da7c39fa64..00000000000000 --- a/src/sentry/integrations/slack/message_builder/event.py +++ /dev/null @@ -1,19 +0,0 @@ -from collections.abc import Sequence - -from sentry.integrations.slack.message_builder import SlackBlock -from sentry.integrations.slack.message_builder.help import SlackHelpMessageBuilder - -from ..utils import logger -from .help import UNKNOWN_COMMAND_MESSAGE - - -class SlackEventMessageBuilder(SlackHelpMessageBuilder): - def get_header_blocks(self) -> Sequence[SlackBlock]: - blocks = [] - if self.command and self.command != "help": - logger.info("slack.event.unknown-command", extra={"command": self.command}) - blocks.append( - self.get_markdown_block(UNKNOWN_COMMAND_MESSAGE.format(command=self.command)) - ) - - return blocks From 0072cf2ee37b61d55a72d307462264e88d3c78fd Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Thu, 2 May 2024 16:32:47 -0700 Subject: [PATCH 05/12] small refactor --- .../integrations/slack/webhooks/action.py | 77 ++++++++++--------- src/sentry/notifications/utils/actions.py | 10 +-- 2 files changed, 46 insertions(+), 41 deletions(-) diff --git a/src/sentry/integrations/slack/webhooks/action.py b/src/sentry/integrations/slack/webhooks/action.py index 081949139a12d3..47264f402718a1 100644 --- a/src/sentry/integrations/slack/webhooks/action.py +++ b/src/sentry/integrations/slack/webhooks/action.py @@ -737,47 +737,54 @@ def get_action_option(cls, slack_request: SlackActionRequest) -> str | None: return action_option @classmethod - def get_action_list( - cls, slack_request: SlackActionRequest, use_block_kit: bool + def _get_default_action_list( + cls, slack_request_actions: list[dict[str, Any]] ) -> list[MessageAction]: - action_data = slack_request.data.get("actions") - if use_block_kit and action_data: - # XXX(CEO): this is here for backwards compatibility - if a user performs an action with an "older" - # style issue alert but the block kit flag is enabled, we don't want to fall into this code path - if action_data[0].get("action_id"): - action_list = [] - for action_data in action_data: - if action_data.get("type") in ("static_select", "external_select"): - action = BlockKitMessageAction( - name=action_data["action_id"], - label=action_data["selected_option"]["text"]["text"], - type=action_data["type"], - value=action_data["selected_option"]["value"], - action_id=action_data["action_id"], - block_id=action_data["block_id"], - selected_options=[ - {"value": action_data.get("selected_option", {}).get("value")} - ], - ) - # TODO: selected_options is kinda ridiculous, I think this is built to handle multi-select? - else: - action = BlockKitMessageAction( - name=action_data["action_id"], - label=action_data["text"]["text"], - type=action_data["type"], - value=action_data["value"], - action_id=action_data["action_id"], - block_id=action_data["block_id"], - ) - action_list.append(action) - - return action_list return [ MessageAction(**action_data) - for action_data in action_data or [] + for action_data in slack_request_actions if "name" in action_data ] + @classmethod + def get_action_list( + cls, slack_request: SlackActionRequest, use_block_kit: bool + ) -> list[MessageAction] | list[BlockKitMessageAction]: + slack_request_actions = slack_request.data.get("actions", []) + if not use_block_kit or not slack_request_actions: + return cls._get_default_action_list(slack_request_actions=slack_request_actions) + + # XXX(CEO): this is here for backwards compatibility - if a user performs an action with an "older" + # style issue alert but the block kit flag is enabled, we don't want to fall into this code path + if not slack_request_actions[0].get("action_id"): + return cls._get_default_action_list(slack_request_actions=slack_request_actions) + + action_list = [] + for slack_request_actions in slack_request_actions: + if slack_request_actions.get("type") in ("static_select", "external_select"): + label = slack_request_actions["selected_option"]["text"]["text"] + value = slack_request_actions["selected_option"]["value"] + selected_options = [ + {"value": slack_request_actions.get("selected_option", {}).get("value")} + ] + # TODO(CEO): selected_options is kinda ridiculous, I think this is built to handle multi-select? + else: + label = slack_request_actions["text"]["text"] + value = slack_request_actions["value"] + selected_options = None + + action = BlockKitMessageAction( + name=slack_request_actions["action_id"], + label=label, + type=slack_request_actions["type"], + value=value, + action_id=slack_request_actions["action_id"], + block_id=slack_request_actions["block_id"], + selected_options=selected_options, + ) + action_list.append(action) + return action_list + def post(self, request: Request) -> Response: try: slack_request = self.slack_request_class(request) diff --git a/src/sentry/notifications/utils/actions.py b/src/sentry/notifications/utils/actions.py index 5001437f6fb2e6..3492253805377b 100644 --- a/src/sentry/notifications/utils/actions.py +++ b/src/sentry/notifications/utils/actions.py @@ -4,11 +4,9 @@ from dataclasses import dataclass from typing import Any, Literal -from sentry.integrations.slack import SlackBlock - @dataclass(kw_only=True) -class _BaseMessageAction: +class BaseMessageAction: """ Base class used to hold the fields for a notification message action """ @@ -27,7 +25,7 @@ class _BaseMessageAction: def _get_button_text(self) -> str: return self.name - def get_button(self) -> SlackBlock: + def get_button(self) -> Mapping[str, Any]: button = { "type": "button", "text": {"type": "plain_text", "text": self._get_button_text()}, @@ -47,7 +45,7 @@ def get_button(self) -> SlackBlock: @dataclass -class MessageAction(_BaseMessageAction): +class MessageAction(BaseMessageAction): # Label is optional, if empty it falls back to name label: str | None = None style: Literal["primary", "danger", "default"] | None = None @@ -59,7 +57,7 @@ def _get_button_text(self) -> str: @dataclass -class BlockKitMessageAction(_BaseMessageAction): +class BlockKitMessageAction(BaseMessageAction): label: str def _get_button_text(self) -> str: From 5d01565628ed39a375a6835baaf9217be543f3b0 Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Thu, 2 May 2024 16:51:26 -0700 Subject: [PATCH 06/12] move code around --- .../integrations/slack/actions/message_action.py | 11 +++++++++++ src/sentry/integrations/slack/webhooks/action.py | 3 ++- src/sentry/notifications/utils/actions.py | 8 -------- 3 files changed, 13 insertions(+), 9 deletions(-) create mode 100644 src/sentry/integrations/slack/actions/message_action.py diff --git a/src/sentry/integrations/slack/actions/message_action.py b/src/sentry/integrations/slack/actions/message_action.py new file mode 100644 index 00000000000000..3fd5b7b65dc79f --- /dev/null +++ b/src/sentry/integrations/slack/actions/message_action.py @@ -0,0 +1,11 @@ +from dataclasses import dataclass + +from sentry.notifications.utils.actions import BaseMessageAction + + +@dataclass +class BlockKitMessageAction(BaseMessageAction): + label: str + + def _get_button_text(self) -> str: + return self.label or self.name diff --git a/src/sentry/integrations/slack/webhooks/action.py b/src/sentry/integrations/slack/webhooks/action.py index 47264f402718a1..f69fd32d1aed18 100644 --- a/src/sentry/integrations/slack/webhooks/action.py +++ b/src/sentry/integrations/slack/webhooks/action.py @@ -19,6 +19,7 @@ from sentry.api.helpers.group_index import update_groups from sentry.auth.access import from_member from sentry.exceptions import UnableToAcceptMemberInvitationException +from sentry.integrations.slack.actions.message_action import BlockKitMessageAction from sentry.integrations.slack.client import SlackClient from sentry.integrations.slack.message_builder import SlackBody from sentry.integrations.slack.message_builder.issues import SlackIssuesMessageBuilder @@ -31,7 +32,7 @@ from sentry.models.group import Group from sentry.models.organizationmember import InviteStatus, OrganizationMember from sentry.models.rule import Rule -from sentry.notifications.utils.actions import BlockKitMessageAction, MessageAction +from sentry.notifications.utils.actions import MessageAction from sentry.services.hybrid_cloud.integration import integration_service from sentry.services.hybrid_cloud.notifications import notifications_service from sentry.services.hybrid_cloud.organization import organization_service diff --git a/src/sentry/notifications/utils/actions.py b/src/sentry/notifications/utils/actions.py index 3492253805377b..12f22a3b106a28 100644 --- a/src/sentry/notifications/utils/actions.py +++ b/src/sentry/notifications/utils/actions.py @@ -54,11 +54,3 @@ class MessageAction(BaseMessageAction): def _get_button_text(self) -> str: return self.label or self.name - - -@dataclass -class BlockKitMessageAction(BaseMessageAction): - label: str - - def _get_button_text(self) -> str: - return self.label or self.name From 45c0293d6a8b6d50597c51a589a2263ede21c136 Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Thu, 2 May 2024 17:14:15 -0700 Subject: [PATCH 07/12] put back dependent code --- .../slack/message_builder/base/block.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/sentry/integrations/slack/message_builder/base/block.py b/src/sentry/integrations/slack/message_builder/base/block.py index 4c0d37876fcd2d..92ba0768c2dbc5 100644 --- a/src/sentry/integrations/slack/message_builder/base/block.py +++ b/src/sentry/integrations/slack/message_builder/base/block.py @@ -7,6 +7,7 @@ from sentry.integrations.slack.message_builder import SlackBlock from sentry.integrations.slack.message_builder.base.base import SlackMessageBuilder +from sentry.notifications.utils.actions import MessageAction class BlockSlackMessageBuilder(SlackMessageBuilder, ABC): @@ -82,6 +83,23 @@ def get_external_select_action(action, initial_option): return action + @staticmethod + def get_link_button(action: MessageAction) -> SlackBlock: + return { + "type": "section", + "text": { + "type": "mrkdwn", + "text": action.label, + }, + "accessory": { + "type": "button", + "text": {"type": "plain_text", "text": action.name, "emoji": True}, + "style": action.style, + "value": action.value, + "url": action.url, + }, + } + @staticmethod def get_action_block(actions: Sequence[tuple[str, str | None, str]]) -> SlackBlock: class SlackBlockType(TypedDict): From 5a17034b2c2790a56be8bbbfdda64737d89cdd70 Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Fri, 3 May 2024 09:46:36 -0700 Subject: [PATCH 08/12] put back used method in getsentry --- .../slack/message_builder/base/block.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/sentry/integrations/slack/message_builder/base/block.py b/src/sentry/integrations/slack/message_builder/base/block.py index 92ba0768c2dbc5..a5ac988fdb8e30 100644 --- a/src/sentry/integrations/slack/message_builder/base/block.py +++ b/src/sentry/integrations/slack/message_builder/base/block.py @@ -83,6 +83,26 @@ def get_external_select_action(action, initial_option): return action + @staticmethod + def get_button_action(action: MessageAction) -> SlackBlock: + button_text = action.label or action.name + button = { + "type": "button", + "text": {"type": "plain_text", "text": button_text}, + } + if action.value: + button["action_id"] = action.value + button["value"] = action.value + + if action.action_id: + button["action_id"] = action.action_id + + if action.url: + button["url"] = action.url + button["value"] = "link_clicked" + + return button + @staticmethod def get_link_button(action: MessageAction) -> SlackBlock: return { From 51def70f798d764c161281ca8203c45c845e15e9 Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Fri, 3 May 2024 13:37:32 -0700 Subject: [PATCH 09/12] add more tests, move stuff around, add logging and helper funcs --- .../slack/actions/message_action.py | 61 +++++- .../slack/message_builder/issues.py | 89 +++++---- .../message_builder/notifications/base.py | 89 ++++++--- .../integrations/slack/webhooks/action.py | 6 +- src/sentry/notifications/utils/actions.py | 27 +-- .../slack/actions/message_action/__init__.py | 0 .../test_slack_message_action.py | 176 ++++++++++++++++++ 7 files changed, 350 insertions(+), 98 deletions(-) create mode 100644 tests/sentry/integrations/slack/actions/message_action/__init__.py create mode 100644 tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py diff --git a/src/sentry/integrations/slack/actions/message_action.py b/src/sentry/integrations/slack/actions/message_action.py index 3fd5b7b65dc79f..378336debef5f0 100644 --- a/src/sentry/integrations/slack/actions/message_action.py +++ b/src/sentry/integrations/slack/actions/message_action.py @@ -1,11 +1,66 @@ +from __future__ import annotations + +from collections.abc import Mapping from dataclasses import dataclass +from typing import Any from sentry.notifications.utils.actions import BaseMessageAction @dataclass -class BlockKitMessageAction(BaseMessageAction): - label: str +class SlackMessageAction(BaseMessageAction): + """ + Class that holds information about a Slack message action. + Has helper functions that can provide Slack specific outputs for the particular message action + """ + + @staticmethod + def to_slack_message_action(original: BaseMessageAction) -> SlackMessageAction: + """ + Converts the original message into a specific SlackMessageAction object with the proper defaults + """ + return SlackMessageAction( + name=original.name, + type=original.type, + label=original.label, + url=original.url, + value=original.value, + action_id=original.action_id, + block_id=original.block_id, + option_groups=original.option_groups, + selected_options=original.selected_options, + ) - def _get_button_text(self) -> str: + def _get_button_text_value(self) -> str: + """ + Returns the proper text value for the button that should be displayed to the user. + Favors the label field if it is set, otherwise falls back to the name of the message action + """ return self.label or self.name + + def _get_button_text(self) -> dict[str, str]: + """ + Returns the proper structure for the button text that Slack expects to display + """ + return {"type": "plain_text", "text": self._get_button_text_value()} + + def get_button(self) -> Mapping[str, Any]: + """ + Create a block kit supported button for this action to be used in a Slack message + """ + button = { + "type": "button", + "text": self._get_button_text(), + } + if self.value: + button["action_id"] = self.value + button["value"] = self.value + + if self.action_id: + button["action_id"] = self.action_id + + if self.url: + button["url"] = self.url + button["value"] = "link_clicked" + + return button diff --git a/src/sentry/integrations/slack/message_builder/issues.py b/src/sentry/integrations/slack/message_builder/issues.py index 150a2ee942d239..2ea3a85a08d789 100644 --- a/src/sentry/integrations/slack/message_builder/issues.py +++ b/src/sentry/integrations/slack/message_builder/issues.py @@ -20,6 +20,7 @@ format_actor_options, get_title_link, ) +from sentry.integrations.slack.actions.message_action import SlackMessageAction from sentry.integrations.slack.message_builder import ( ACTION_EMOJI, ACTIONED_CATEGORY_TO_EMOJI, @@ -372,13 +373,52 @@ def get_action_text(text: str, actions: Sequence[Any], identity: RpcIdentity) -> return action_text +def _ignore_action(group: Group) -> SlackMessageAction | None: + if group.issue_category == GroupCategory.FEEDBACK: + return None + + if group.get_status() == GroupStatus.IGNORED: + return SlackMessageAction( + name="status", label="Mark as Ongoing", value="unresolved:ongoing" + ) + + return SlackMessageAction(name="status", label="Archive", value="archive_dialog") + + +def _resolve_action(group: Group, project: Project) -> SlackMessageAction: + if group.get_status() == GroupStatus.RESOLVED: + return SlackMessageAction( + name="unresolved:ongoing", label="Unresolve", value="unresolved:ongoing" + ) + if not project.flags.has_releases: + return SlackMessageAction(name="status", label="Resolve", value="resolved") + + return SlackMessageAction( + name="status", + label="Resolve", + value="resolve_dialog", + ) + + +def _assign_action(group: Group) -> SlackMessageAction: + assignee = group.get_assignee() + assign_button = SlackMessageAction( + name="assign", + label="Select Assignee...", + type="select", + selected_options=format_actor_options([assignee], True) if assignee else [], + option_groups=get_option_groups_block_kit(group), + ) + return assign_button + + def build_actions( group: Group, project: Project, text: str, actions: Sequence[MessageAction] | None = None, identity: RpcIdentity | None = None, -) -> tuple[Sequence[MessageAction], str, bool]: +) -> tuple[Sequence[SlackMessageAction], str, bool]: """Having actions means a button will be shown on the Slack message e.g. ignore, resolve, assign.""" if actions and identity: text = get_action_text(text, actions, identity) @@ -387,50 +427,9 @@ def build_actions( return [], text, True return [], text, False - status = group.get_status() - - def _ignore_button() -> MessageAction | None: - if group.issue_category == GroupCategory.FEEDBACK: - return None - if status == GroupStatus.IGNORED: - return MessageAction(name="status", label="Mark as Ongoing", value="unresolved:ongoing") - - return MessageAction(name="status", label="Archive", value="archive_dialog") - - def _resolve_button() -> MessageAction: - if status == GroupStatus.RESOLVED: - return MessageAction( - name="unresolved:ongoing", label="Unresolve", value="unresolved:ongoing" - ) - if not project.flags.has_releases: - return MessageAction(name="status", label="Resolve", value="resolved") - - return MessageAction( - name="status", - label="Resolve", - value="resolve_dialog", - ) - - def _assign_button() -> MessageAction: - assignee = group.get_assignee() - assign_button = MessageAction( - name="assign", - label="Select Assignee...", - type="select", - selected_options=format_actor_options([assignee], True) if assignee else [], - option_groups=get_option_groups_block_kit(group), - ) - return assign_button - - action_list = [ - a - for a in [ - _resolve_button(), - _ignore_button(), - _assign_button(), - ] - if a is not None - ] + action_list = [_resolve_action(group=group, project=project), _assign_action(group=group)] + if (ignore_action := _ignore_action(group=group)) is not None: + action_list.append(ignore_action) return action_list, text, False diff --git a/src/sentry/integrations/slack/message_builder/notifications/base.py b/src/sentry/integrations/slack/message_builder/notifications/base.py index 14a3b41ed724cf..527f4c2b7be85f 100644 --- a/src/sentry/integrations/slack/message_builder/notifications/base.py +++ b/src/sentry/integrations/slack/message_builder/notifications/base.py @@ -1,8 +1,10 @@ from __future__ import annotations +import logging from collections.abc import Mapping from typing import Any +from sentry.integrations.slack.actions.message_action import SlackMessageAction from sentry.integrations.slack.message_builder import SlackBlock from sentry.integrations.slack.message_builder.base.block import BlockSlackMessageBuilder from sentry.integrations.slack.utils.escape import escape_slack_text @@ -11,6 +13,8 @@ from sentry.types.integrations import ExternalProviders from sentry.utils import json +_default_logger = logging.getLogger(__name__) + class SlackNotificationsMessageBuilder(BlockSlackMessageBuilder): def __init__( @@ -24,22 +28,36 @@ def __init__( self.context = context self.recipient = recipient - def build(self) -> SlackBlock: - callback_id_raw = self.notification.get_callback_data() - title = self.notification.build_attachment_title(self.recipient) - title_link = self.notification.get_title_link(self.recipient, ExternalProviders.SLACK) - text = self.notification.get_message_description(self.recipient, ExternalProviders.SLACK) - footer = self.notification.build_notification_footer( - self.recipient, ExternalProviders.SLACK - ) - actions = self.notification.get_message_actions(self.recipient, ExternalProviders.SLACK) - callback_id = ( - json.dumps_experimental("integrations.slack.enable-orjson", callback_id_raw) - if callback_id_raw - else None - ) + def _get_callback_id(self) -> str | None: + """ + Helper method to get the callback id used for the message sent to Slack + """ + callback_id_raw = None + try: + callback_id_raw = self.notification.get_callback_data() + callback_id = ( + json.dumps_experimental("integrations.slack.enable-orjson", callback_id_raw) + if callback_id_raw + else None + ) + except Exception as err: + _default_logger.info( + "There was an error trying to get the callback id", + exc_info=err, + extra={"raw_callback_id": callback_id_raw, "err_message": str(err)}, + ) + raise + return callback_id + + def _get_first_block_text(self) -> str: + """ + Helper method to get the first block text used for the message sent to Slack + """ first_block_text = "" + + title_link = self.notification.get_title_link(self.recipient, ExternalProviders.SLACK) + title = self.notification.build_attachment_title(self.recipient) if title_link: if title: first_block_text += f"<{title_link}|*{escape_slack_text(title)}*> \n" @@ -48,22 +66,49 @@ def build(self) -> SlackBlock: elif title: # ie. "ZeroDivisionError", first_block_text += f"*{escape_slack_text(title)}* \n" + text = self.notification.get_message_description(self.recipient, ExternalProviders.SLACK) if text: # ie. "division by zero", comments first_block_text += text + return first_block_text + + def _get_actions_block(self) -> SlackBlock | None: + """ + Helper method to get the actions block associated with the current Slack action message. + If no actions are found, returns None. + """ + actions_block = [] + actions = self.notification.get_message_actions(self.recipient, ExternalProviders.SLACK) + for action in actions: + slack_action_message = SlackMessageAction.to_slack_message_action(action) + actions_block.append(slack_action_message.get_button()) + + if not actions_block: + return None + + return {"type": "actions", "elements": actions_block} + + def build(self) -> SlackBlock: + """ + Method that builds the Slack block message. Leverages other helper methods to create smaller block parts + required for the message. + """ + # Holds the individual block elements that we want attached to our final Slack message blocks = [] - if first_block_text: + + if first_block_text := self._get_first_block_text(): blocks.append(self.get_markdown_block(text=first_block_text)) + + footer = self.notification.build_notification_footer( + self.recipient, ExternalProviders.SLACK + ) if footer: blocks.append(self.get_context_block(text=footer)) - actions_block = [] - for action in actions: - actions_block.append(action.get_button()) - - if actions_block: - blocks.append({"type": "actions", "elements": [action for action in actions_block]}) + if actions_block := self._get_actions_block(): + blocks.append(actions_block) + text = self.notification.get_message_description(self.recipient, ExternalProviders.SLACK) return self._build_blocks( - *blocks, fallback_text=text if text else None, callback_id=callback_id + *blocks, fallback_text=text if text else None, callback_id=self._get_callback_id() ) diff --git a/src/sentry/integrations/slack/webhooks/action.py b/src/sentry/integrations/slack/webhooks/action.py index f69fd32d1aed18..11f7520b18f263 100644 --- a/src/sentry/integrations/slack/webhooks/action.py +++ b/src/sentry/integrations/slack/webhooks/action.py @@ -19,7 +19,7 @@ from sentry.api.helpers.group_index import update_groups from sentry.auth.access import from_member from sentry.exceptions import UnableToAcceptMemberInvitationException -from sentry.integrations.slack.actions.message_action import BlockKitMessageAction +from sentry.integrations.slack.actions.message_action import SlackMessageAction from sentry.integrations.slack.client import SlackClient from sentry.integrations.slack.message_builder import SlackBody from sentry.integrations.slack.message_builder.issues import SlackIssuesMessageBuilder @@ -750,7 +750,7 @@ def _get_default_action_list( @classmethod def get_action_list( cls, slack_request: SlackActionRequest, use_block_kit: bool - ) -> list[MessageAction] | list[BlockKitMessageAction]: + ) -> list[MessageAction] | list[SlackMessageAction]: slack_request_actions = slack_request.data.get("actions", []) if not use_block_kit or not slack_request_actions: return cls._get_default_action_list(slack_request_actions=slack_request_actions) @@ -774,7 +774,7 @@ def get_action_list( value = slack_request_actions["value"] selected_options = None - action = BlockKitMessageAction( + action = SlackMessageAction( name=slack_request_actions["action_id"], label=label, type=slack_request_actions["type"], diff --git a/src/sentry/notifications/utils/actions.py b/src/sentry/notifications/utils/actions.py index 12f22a3b106a28..9e9ef627f954b6 100644 --- a/src/sentry/notifications/utils/actions.py +++ b/src/sentry/notifications/utils/actions.py @@ -13,6 +13,7 @@ class BaseMessageAction: name: str type: Literal["button", "select"] = "button" + label: str # If the message action is a button type, the url is required url: str | None = None # If the message action is a select type, this is the selected value @@ -20,37 +21,13 @@ class BaseMessageAction: # Denotes the type of action action_id: str | None = None block_id: str | None = None + option_groups: Sequence[Mapping[str, Any]] | None = None selected_options: Sequence[Mapping[str, Any]] | None = None - def _get_button_text(self) -> str: - return self.name - - def get_button(self) -> Mapping[str, Any]: - button = { - "type": "button", - "text": {"type": "plain_text", "text": self._get_button_text()}, - } - if self.value: - button["action_id"] = self.value - button["value"] = self.value - - if self.action_id: - button["action_id"] = self.action_id - - if self.url: - button["url"] = self.url - button["value"] = "link_clicked" - - return button - @dataclass class MessageAction(BaseMessageAction): # Label is optional, if empty it falls back to name label: str | None = None style: Literal["primary", "danger", "default"] | None = None - option_groups: Sequence[Mapping[str, Any]] | None = None elements: Sequence[Mapping[str, Any]] | None = None - - def _get_button_text(self) -> str: - return self.label or self.name diff --git a/tests/sentry/integrations/slack/actions/message_action/__init__.py b/tests/sentry/integrations/slack/actions/message_action/__init__.py new file mode 100644 index 00000000000000..e69de29bb2d1d6 diff --git a/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py b/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py new file mode 100644 index 00000000000000..02c82d42566792 --- /dev/null +++ b/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py @@ -0,0 +1,176 @@ +from sentry.integrations.slack.actions.message_action import SlackMessageAction +from sentry.notifications.utils.actions import BaseMessageAction +from sentry.testutils.cases import TestCase + + +class TestToSlackMessageAction(TestCase): + def setUp(self) -> None: + self.base_action = BaseMessageAction( + name="test_action", + type="button", + label="Test Button", + url="https://example.com", + value="test_value", + action_id="test_action_id", + block_id="test_block_id", + option_groups={"test_group": "test_option_group"}, + selected_options={"test_option": "test_option_value"}, + ) + + def test_to_slack_message_action(self) -> None: + slack_action = SlackMessageAction.to_slack_message_action(self.base_action) + assert isinstance(slack_action, SlackMessageAction) + + def test_name_conversion(self) -> None: + slack_action = SlackMessageAction.to_slack_message_action(self.base_action) + assert slack_action.name == self.base_action.name + + def test_type_conversion(self) -> None: + slack_action = SlackMessageAction.to_slack_message_action(self.base_action) + assert slack_action.type == self.base_action.type + + def test_label_conversion(self) -> None: + slack_action = SlackMessageAction.to_slack_message_action(self.base_action) + assert slack_action.label == self.base_action.label + + def test_url_conversion(self) -> None: + slack_action = SlackMessageAction.to_slack_message_action(self.base_action) + assert slack_action.url == self.base_action.url + + def test_value_conversion(self) -> None: + slack_action = SlackMessageAction.to_slack_message_action(self.base_action) + assert slack_action.value == self.base_action.value + + def test_action_id_conversion(self) -> None: + slack_action = SlackMessageAction.to_slack_message_action(self.base_action) + assert slack_action.action_id == self.base_action.action_id + + def test_block_id_conversion(self) -> None: + slack_action = SlackMessageAction.to_slack_message_action(self.base_action) + assert slack_action.block_id == self.base_action.block_id + + def test_option_groups_conversion(self) -> None: + slack_action = SlackMessageAction.to_slack_message_action(self.base_action) + assert slack_action.option_groups == self.base_action.option_groups + + def test_selected_options_conversion(self) -> None: + slack_action = SlackMessageAction.to_slack_message_action(self.base_action) + assert slack_action.selected_options == self.base_action.selected_options + + +class TestGetButtonTextValue(TestCase): + def test_uses_label_when_populated(self) -> None: + label = "Test Button" + slack_action = SlackMessageAction( + name="test_action", + type="button", + label=label, + url="https://example.com", + value="test_value", + action_id="test_action_id", + block_id="test_block_id", + option_groups={"test_group": "test_option_group"}, + selected_options={"test_option": "test_option_value"}, + ) + button_text = slack_action._get_button_text_value() + assert button_text == label + + def test_uses_name_when_label_is_empty(self) -> None: + name = "test_action" + slack_action = SlackMessageAction( + name=name, + type="button", + label="", + url="https://example.com", + value="test_value", + action_id="test_action_id", + block_id="test_block_id", + option_groups={"test_group": "test_option_group"}, + selected_options={"test_option": "test_option_value"}, + ) + button_text = slack_action._get_button_text_value() + assert button_text == name + + +class _BaseTestSlackMessageAction(TestCase): + def setUp(self) -> None: + self.default_slack_action = SlackMessageAction( + name="test_action", + type="button", + label="Test Button", + url="https://example.com", + value="test_value", + action_id="test_action_id", + block_id="test_block_id", + option_groups={"test_group": "test_option_group"}, + selected_options={"test_option": "test_option_value"}, + ) + + +class TestGetButtonTest(_BaseTestSlackMessageAction): + def test_has_type_key(self) -> None: + text_obj = self.default_slack_action._get_button_text() + assert "type" in text_obj + + def test_has_correct_type_value(self) -> None: + text_obj = self.default_slack_action._get_button_text() + assert text_obj["type"] == "plain_text" + + def test_has_text_key(self) -> None: + text_obj = self.default_slack_action._get_button_text() + assert "text" in text_obj + + def test_has_correct_text_value(self) -> None: + text_obj = self.default_slack_action._get_button_text() + assert text_obj["text"] == self.default_slack_action._get_button_text_value() + + +class TestGetButton(_BaseTestSlackMessageAction): + def test_type_key_exists(self) -> None: + button = self.default_slack_action.get_button() + assert "type" in button + + def test_type_is_button(self) -> None: + button = self.default_slack_action.get_button() + button_type = button["type"] + assert button_type == "button" + + def test_text_key_exists(self) -> None: + button = self.default_slack_action.get_button() + assert "text" in button + + def test_button_action_id_overwrites_value(self) -> None: + slack_action = SlackMessageAction( + name="test_action", + type="button", + label="Test Button", + url=None, + value="test_value", + action_id="test_action_id", + block_id="test_block_id", + option_groups={"test_group": "test_option_group"}, + selected_options={"test_option": "test_option_value"}, + ) + button = slack_action.get_button() + assert button["action_id"] == "test_action_id" + + def test_button_url_overwrites_value(self) -> None: + slack_action = SlackMessageAction( + name="test_action", + type="button", + label="Test Button", + url="https://example.com", + value="test_value", + action_id=None, + block_id="test_block_id", + option_groups={"test_group": "test_option_group"}, + selected_options={"test_option": "test_option_value"}, + ) + button = slack_action.get_button() + assert button["value"] == "link_clicked" + + def test_all_overwrites_at_once(self) -> None: + button = self.default_slack_action.get_button() + assert button["url"] == "https://example.com" + assert button["value"] == "link_clicked" + assert button["action_id"] == "test_action_id" From 2dd875288505803717a4ec25555931430d775f29 Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Fri, 3 May 2024 13:48:23 -0700 Subject: [PATCH 10/12] fix typing --- .../message_action/test_slack_message_action.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py b/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py index 02c82d42566792..28b0e30e4ef63a 100644 --- a/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py +++ b/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py @@ -13,8 +13,8 @@ def setUp(self) -> None: value="test_value", action_id="test_action_id", block_id="test_block_id", - option_groups={"test_group": "test_option_group"}, - selected_options={"test_option": "test_option_value"}, + option_groups=[{"test_group": "test_option_group"}], + selected_options=[{"test_option": "test_option_value"}], ) def test_to_slack_message_action(self) -> None: @@ -102,8 +102,8 @@ def setUp(self) -> None: value="test_value", action_id="test_action_id", block_id="test_block_id", - option_groups={"test_group": "test_option_group"}, - selected_options={"test_option": "test_option_value"}, + option_groups=[{"test_group": "test_option_group"}], + selected_options=[{"test_option": "test_option_value"}], ) @@ -148,8 +148,8 @@ def test_button_action_id_overwrites_value(self) -> None: value="test_value", action_id="test_action_id", block_id="test_block_id", - option_groups={"test_group": "test_option_group"}, - selected_options={"test_option": "test_option_value"}, + option_groups=[{"test_group": "test_option_group"}], + selected_options=[{"test_option": "test_option_value"}], ) button = slack_action.get_button() assert button["action_id"] == "test_action_id" @@ -163,8 +163,8 @@ def test_button_url_overwrites_value(self) -> None: value="test_value", action_id=None, block_id="test_block_id", - option_groups={"test_group": "test_option_group"}, - selected_options={"test_option": "test_option_value"}, + option_groups=[{"test_group": "test_option_group"}], + selected_options=[{"test_option": "test_option_value"}], ) button = slack_action.get_button() assert button["value"] == "link_clicked" From 0253ce3d3f9e44ed82fa2e9333935b3f3cb367d9 Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Fri, 3 May 2024 13:56:53 -0700 Subject: [PATCH 11/12] forgot typing --- .../actions/message_action/test_slack_message_action.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py b/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py index 28b0e30e4ef63a..1337c0456c3d1b 100644 --- a/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py +++ b/tests/sentry/integrations/slack/actions/message_action/test_slack_message_action.py @@ -69,8 +69,8 @@ def test_uses_label_when_populated(self) -> None: value="test_value", action_id="test_action_id", block_id="test_block_id", - option_groups={"test_group": "test_option_group"}, - selected_options={"test_option": "test_option_value"}, + option_groups=[{"test_group": "test_option_group"}], + selected_options=[{"test_option": "test_option_value"}], ) button_text = slack_action._get_button_text_value() assert button_text == label @@ -85,8 +85,8 @@ def test_uses_name_when_label_is_empty(self) -> None: value="test_value", action_id="test_action_id", block_id="test_block_id", - option_groups={"test_group": "test_option_group"}, - selected_options={"test_option": "test_option_value"}, + option_groups=[{"test_group": "test_option_group"}], + selected_options=[{"test_option": "test_option_value"}], ) button_text = slack_action._get_button_text_value() assert button_text == name From c4cc46ec41ab1361a3d7006aaece1b5e97985347 Mon Sep 17 00:00:00 2001 From: Yash Kamothi Date: Fri, 3 May 2024 15:54:46 -0700 Subject: [PATCH 12/12] fix typing inheritance --- src/sentry/integrations/slack/actions/message_action.py | 4 +++- src/sentry/notifications/utils/actions.py | 5 ++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/sentry/integrations/slack/actions/message_action.py b/src/sentry/integrations/slack/actions/message_action.py index 378336debef5f0..26a786b8fa68d5 100644 --- a/src/sentry/integrations/slack/actions/message_action.py +++ b/src/sentry/integrations/slack/actions/message_action.py @@ -14,6 +14,8 @@ class SlackMessageAction(BaseMessageAction): Has helper functions that can provide Slack specific outputs for the particular message action """ + label: str + @staticmethod def to_slack_message_action(original: BaseMessageAction) -> SlackMessageAction: """ @@ -22,7 +24,7 @@ def to_slack_message_action(original: BaseMessageAction) -> SlackMessageAction: return SlackMessageAction( name=original.name, type=original.type, - label=original.label, + label=original.label if original.label else "", url=original.url, value=original.value, action_id=original.action_id, diff --git a/src/sentry/notifications/utils/actions.py b/src/sentry/notifications/utils/actions.py index 9e9ef627f954b6..96aff6146cb1d7 100644 --- a/src/sentry/notifications/utils/actions.py +++ b/src/sentry/notifications/utils/actions.py @@ -13,7 +13,8 @@ class BaseMessageAction: name: str type: Literal["button", "select"] = "button" - label: str + # Label is optional, if empty it falls back to name + label: str | None = None # If the message action is a button type, the url is required url: str | None = None # If the message action is a select type, this is the selected value @@ -27,7 +28,5 @@ class BaseMessageAction: @dataclass class MessageAction(BaseMessageAction): - # Label is optional, if empty it falls back to name - label: str | None = None style: Literal["primary", "danger", "default"] | None = None elements: Sequence[Mapping[str, Any]] | None = None