From 4dd80bed1fb1fe7872850fd09c530493276b338e Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Wed, 11 Sep 2024 09:56:54 -0700 Subject: [PATCH 1/4] :recycle: ref: fix slack webhook actions typing --- pyproject.toml | 1 - src/sentry/api/helpers/group_index/update.py | 2 +- .../slack/message_builder/issues.py | 4 +- .../integrations/slack/webhooks/action.py | 154 +++++++++++------- 4 files changed, 94 insertions(+), 67 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 3e4430f3f31053..27f73a0ec2ab46 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -278,7 +278,6 @@ module = [ "sentry.integrations.slack.unfurl.discover", "sentry.integrations.slack.utils.channel", "sentry.integrations.slack.utils.users", - "sentry.integrations.slack.webhooks.action", "sentry.integrations.slack.webhooks.command", "sentry.integrations.slack.webhooks.event", "sentry.integrations.utils.commit_context", diff --git a/src/sentry/api/helpers/group_index/update.py b/src/sentry/api/helpers/group_index/update.py index b10045644d557b..1962ee68746813 100644 --- a/src/sentry/api/helpers/group_index/update.py +++ b/src/sentry/api/helpers/group_index/update.py @@ -170,7 +170,7 @@ def update_groups( projects: Sequence[Project], organization_id: int, search_fn: SearchFunction | None, - user: User | None = None, + user: RpcUser | User | None = None, data: Mapping[str, Any] | None = None, ) -> Response: # If `user` and `data` are passed as parameters then they should override diff --git a/src/sentry/integrations/slack/message_builder/issues.py b/src/sentry/integrations/slack/message_builder/issues.py index ef01d320e0ab49..9c4de0bd1762fc 100644 --- a/src/sentry/integrations/slack/message_builder/issues.py +++ b/src/sentry/integrations/slack/message_builder/issues.py @@ -51,7 +51,7 @@ from sentry.models.rule import Rule from sentry.models.team import Team from sentry.notifications.notifications.base import ProjectNotification -from sentry.notifications.utils.actions import MessageAction +from sentry.notifications.utils.actions import BlockKitMessageAction, MessageAction from sentry.notifications.utils.participants import ( dedupe_suggested_assignees, get_suspect_commit_users, @@ -421,7 +421,7 @@ def __init__( event: Event | GroupEvent | None = None, tags: set[str] | None = None, identity: RpcIdentity | None = None, - actions: Sequence[MessageAction] | None = None, + actions: Sequence[MessageAction | BlockKitMessageAction] | None = None, rules: list[Rule] | None = None, link_to_event: bool = False, issue_details: bool = False, diff --git a/src/sentry/integrations/slack/webhooks/action.py b/src/sentry/integrations/slack/webhooks/action.py index 3ac68d16a782bd..25c8bad989e4f3 100644 --- a/src/sentry/integrations/slack/webhooks/action.py +++ b/src/sentry/integrations/slack/webhooks/action.py @@ -208,22 +208,40 @@ def api_error( channel_id = data.get("channel_id") response_url = data.get("orig_response_url") - if error.status_code == 403: + user_id = slack_request.user_id + channel = channel_id or slack_request.channel_id + resp_url = response_url or slack_request.response_url + + if user_id is None or channel is None or resp_url is None: + text = DEFAULT_ERROR_MESSAGE + # keeping this separate from above since its a different condition + elif error.status_code != 403: + text = DEFAULT_ERROR_MESSAGE + else: text = UNLINK_IDENTITY_MESSAGE.format( associate_url=build_unlinking_url( slack_request.integration.id, - slack_request.user_id, - channel_id or slack_request.channel_id, - response_url or slack_request.response_url, + user_id, + channel, + resp_url, ), user_email=user.email, org_name=group.organization.name, ) - else: - text = DEFAULT_ERROR_MESSAGE return self.respond_ephemeral(text) + def get_first_error_text(self, error_detail) -> str: + if isinstance(error_detail, dict): + # If it's a dictionary, get the first value + return str(next(iter(error_detail.values()))) + elif isinstance(error_detail, list): + # If it's a list, get the first item + return str(error_detail[0]) if error_detail else "" + else: + # If it's neither, return an empty string or raise an exception + return "" + def validation_error( self, slack_request: SlackActionRequest, @@ -240,11 +258,15 @@ def validation_error( }, ) - text: str = list(*error.detail.values())[0] + text: str = self.get_first_error_text(error.detail) return self.respond_ephemeral(text) def on_assign( - self, request: Request, user: RpcUser, group: Group, action: MessageAction + self, + request: Request, + user: RpcUser, + group: Group, + action: MessageAction | BlockKitMessageAction, ) -> None: if not (action.selected_options and len(action.selected_options)): # Short-circuit if action is invalid @@ -269,7 +291,7 @@ def on_status( request: Request, user: RpcUser, group: Group, - action: MessageAction, + action: MessageAction | BlockKitMessageAction, ) -> None: status_data = (action.value or "").split(":", 2) if not len(status_data): @@ -408,11 +430,11 @@ def _update_modal( }, ) # The modal was not found, so we need to open a new one - self._open_view(slack_client, modal_payload, slack_request) + self._open_modal(slack_client, modal_payload, slack_request) else: raise - def _open_view( + def _open_modal( self, slack_client: SlackSdkClient, modal_payload: View, slack_request: SlackActionRequest ) -> None: # Error handling is done in the calling function @@ -451,10 +473,11 @@ def open_resolve_dialog(self, slack_request: SlackActionRequest, group: Group) - # We passed this in control when we sent the loading modal to beat the 3 second timeout external_id = slack_request.get_action_ts() - if options.get("send-slack-response-from-control-silo"): - self._update_modal(slack_client, external_id, modal_payload, slack_request) + if not external_id or not options.get("send-slack-response-from-control-silo"): + # If we don't have an external_id or option is disabled we need to open a new modal + self._open_modal(slack_client, modal_payload, slack_request) else: - self._open_view(slack_client, modal_payload, slack_request) + self._update_modal(slack_client, external_id, modal_payload, slack_request) metrics.incr( SLACK_WEBHOOK_GROUP_ACTIONS_SUCCESS_DATADOG_METRIC, @@ -503,10 +526,11 @@ def open_archive_dialog(self, slack_request: SlackActionRequest, group: Group) - # We passed this in control when we sent the loading modal to beat the 3 second timeout external_id = slack_request.get_action_ts() - if options.get("send-slack-response-from-control-silo"): - self._update_modal(slack_client, external_id, modal_payload, slack_request) + if not external_id or not options.get("send-slack-response-from-control-silo"): + # If we don't have an external_id or option is disabled we need to open a new modal + self._open_modal(slack_client, modal_payload, slack_request) else: - self._open_view(slack_client, modal_payload, slack_request) + self._update_modal(slack_client, external_id, modal_payload, slack_request) metrics.incr( SLACK_WEBHOOK_GROUP_ACTIONS_SUCCESS_DATADOG_METRIC, @@ -549,7 +573,7 @@ def _handle_group_actions( self, slack_request: SlackActionRequest, request: Request, - action_list: Sequence[MessageAction], + action_list: Sequence[BlockKitMessageAction], ) -> Response: from sentry.integrations.slack.views.link_identity import build_linking_url @@ -564,6 +588,10 @@ def _handle_group_actions( identity_user = slack_request.get_identity_user() if not identity or not identity_user: + # if we don't have user_id or channel_id, we can't link the identity + if not slack_request.user_id or not slack_request.channel_id: + return self.respond_ephemeral(NO_IDENTITY_MESSAGE) + associate_url = build_linking_url( integration=slack_request.integration, slack_id=slack_request.user_id, @@ -594,10 +622,10 @@ def _handle_group_actions( if not selection: return self.respond() - action = MessageAction(name="status", value=selection) + status_action = MessageAction(name="status", value=selection) try: - self.on_status(request, identity_user, group, action) + self.on_status(request, identity_user, group, status_action) except client.ApiError as error: return self.api_error(slack_request, group, identity_user, error, "status_dialog") @@ -608,7 +636,7 @@ def _handle_group_actions( blocks = SlackIssuesMessageBuilder( group, identity=identity, - actions=[action], + actions=[status_action], tags=original_tags_from_request, rules=[rule] if rule else None, issue_details=True, @@ -616,11 +644,10 @@ def _handle_group_actions( ).build() # use the original response_url to update the link attachment - json_blocks = orjson.dumps(blocks.get("blocks")).decode() try: webhook_client = WebhookClient(private_metadata["orig_response_url"]) webhook_client.send( - blocks=json_blocks, delete_original=False, replace_original=True + blocks=blocks.get("blocks"), delete_original=False, replace_original=True ) metrics.incr( SLACK_WEBHOOK_GROUP_ACTIONS_SUCCESS_DATADOG_METRIC, @@ -700,18 +727,20 @@ def _handle_group_actions( return self.respond() response_url = slack_request.data["response_url"] - json_blocks = orjson.dumps(response.get("blocks")).decode() webhook_client = WebhookClient(response_url) try: webhook_client.send( - blocks=json_blocks, + blocks=response.get("blocks"), text=response.get("text"), delete_original=False, replace_original=True, ) _logger.info( "slack.webhook.update_status.success", - extra={"integration_id": slack_request.integration.id, "blocks": json_blocks}, + extra={ + "integration_id": slack_request.integration.id, + "blocks": response.get("blocks"), + }, ) metrics.incr( SLACK_WEBHOOK_GROUP_ACTIONS_SUCCESS_DATADOG_METRIC, @@ -758,44 +787,42 @@ def get_action_option(cls, slack_request: SlackActionRequest) -> str | None: return action_option @classmethod - def get_action_list(cls, slack_request: SlackActionRequest) -> list[MessageAction]: + def get_action_list(cls, slack_request: SlackActionRequest) -> list[BlockKitMessageAction]: action_data = slack_request.data.get("actions") - if 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 [] - if "name" in action_data - ] + if ( + not action_data + or not isinstance(action_data, list) + or not action_data[0].get("action_id") + ): + return [] + + 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 def post(self, request: Request) -> Response: try: @@ -903,6 +930,7 @@ def handle_member_approval(self, slack_request: SlackActionRequest, action: str) except Exception: # shouldn't error but if it does, respond to the user _logger.exception( + "slack.action.member-invitation-error", extra={ "organization_id": organization.id, "member_id": member.id, From 97d6bd8220ce66c978c0c1601a1892d3b97c0087 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Wed, 11 Sep 2024 10:14:32 -0700 Subject: [PATCH 2/4] :mag: nit: some nits and mor typing fixes --- .../discord/webhooks/message_component.py | 2 +- .../integrations/slack/message_builder/issues.py | 6 ++++-- src/sentry/integrations/slack/webhooks/action.py | 12 ++++++------ 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/sentry/integrations/discord/webhooks/message_component.py b/src/sentry/integrations/discord/webhooks/message_component.py index e6fb124ee259ce..de920038248619 100644 --- a/src/sentry/integrations/discord/webhooks/message_component.py +++ b/src/sentry/integrations/discord/webhooks/message_component.py @@ -232,7 +232,7 @@ def update_group(self, data: Mapping[str, object]) -> None: projects=[self.group.project], organization_id=self.group.organization.id, search_fn=None, - user=self.user, # type: ignore[arg-type] + user=self.user, data=data, ) diff --git a/src/sentry/integrations/slack/message_builder/issues.py b/src/sentry/integrations/slack/message_builder/issues.py index 9c4de0bd1762fc..62736ffad7c529 100644 --- a/src/sentry/integrations/slack/message_builder/issues.py +++ b/src/sentry/integrations/slack/message_builder/issues.py @@ -135,7 +135,9 @@ def build_assigned_text(identity: RpcIdentity, assignee: str) -> str | None: return f"*Issue assigned to {assignee_text} by <@{identity.external_id}>*" -def build_action_text(identity: RpcIdentity, action: MessageAction) -> str | None: +def build_action_text( + identity: RpcIdentity, action: MessageAction | BlockKitMessageAction +) -> str | None: if action.name == "assign": selected_options = action.selected_options or [] if not len(selected_options): @@ -356,7 +358,7 @@ def build_actions( group: Group, project: Project, text: str, - actions: Sequence[MessageAction] | None = None, + actions: Sequence[MessageAction | BlockKitMessageAction] | None = None, identity: RpcIdentity | None = None, ) -> tuple[Sequence[MessageAction], str, bool]: """Having actions means a button will be shown on the Slack message e.g. ignore, resolve, assign.""" diff --git a/src/sentry/integrations/slack/webhooks/action.py b/src/sentry/integrations/slack/webhooks/action.py index 25c8bad989e4f3..f98ca845f6996b 100644 --- a/src/sentry/integrations/slack/webhooks/action.py +++ b/src/sentry/integrations/slack/webhooks/action.py @@ -221,9 +221,9 @@ def api_error( text = UNLINK_IDENTITY_MESSAGE.format( associate_url=build_unlinking_url( slack_request.integration.id, - user_id, - channel, - resp_url, + slack_id=user_id, + channel_id=channel, + response_url=resp_url, ), user_email=user.email, org_name=group.organization.name, @@ -233,13 +233,13 @@ def api_error( def get_first_error_text(self, error_detail) -> str: if isinstance(error_detail, dict): - # If it's a dictionary, get the first value + # if it's a dictionary, get the first value return str(next(iter(error_detail.values()))) elif isinstance(error_detail, list): - # If it's a list, get the first item + # if it's a list, get the first item return str(error_detail[0]) if error_detail else "" else: - # If it's neither, return an empty string or raise an exception + # if it's neither, return an empty string return "" def validation_error( From f6ca079a280fd76166525339a676a648c68667e5 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Mon, 16 Sep 2024 13:05:17 -0700 Subject: [PATCH 3/4] :bug: fix: fix the error text and tests --- .../integrations/slack/webhooks/action.py | 18 +++---- .../slack/webhooks/actions/test_status.py | 52 +++++++++---------- 2 files changed, 32 insertions(+), 38 deletions(-) diff --git a/src/sentry/integrations/slack/webhooks/action.py b/src/sentry/integrations/slack/webhooks/action.py index f98ca845f6996b..c8346ca591b3a3 100644 --- a/src/sentry/integrations/slack/webhooks/action.py +++ b/src/sentry/integrations/slack/webhooks/action.py @@ -231,17 +231,6 @@ def api_error( return self.respond_ephemeral(text) - def get_first_error_text(self, error_detail) -> str: - if isinstance(error_detail, dict): - # if it's a dictionary, get the first value - return str(next(iter(error_detail.values()))) - elif isinstance(error_detail, list): - # if it's a list, get the first item - return str(error_detail[0]) if error_detail else "" - else: - # if it's neither, return an empty string - return "" - def validation_error( self, slack_request: SlackActionRequest, @@ -258,7 +247,12 @@ def validation_error( }, ) - text: str = self.get_first_error_text(error.detail) + text: str = "" + if isinstance(error.detail, dict): + text = list(*error.detail.values())[0] + else: + text = str(error.detail[0]) + return self.respond_ephemeral(text) def on_assign( diff --git a/tests/sentry/integrations/slack/webhooks/actions/test_status.py b/tests/sentry/integrations/slack/webhooks/actions/test_status.py index bcb92d27fa2d47..77bf8b0ef400b7 100644 --- a/tests/sentry/integrations/slack/webhooks/actions/test_status.py +++ b/tests/sentry/integrations/slack/webhooks/actions/test_status.py @@ -243,7 +243,7 @@ def test_archive_issue_until_escalating(self, mock_tags): assert self.group.get_status() == GroupStatus.IGNORED assert self.group.substatus == GroupSubStatus.UNTIL_ESCALATING - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags @@ -263,7 +263,7 @@ def test_archive_issue_until_escalating_through_unfurl(self, mock_tags): assert self.group.get_status() == GroupStatus.IGNORED assert self.group.substatus == GroupSubStatus.UNTIL_ESCALATING - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue archived by <@{self.external_id}>*" @@ -281,7 +281,7 @@ def test_archive_issue_until_condition_met(self, mock_tags): group_snooze = GroupSnooze.objects.get(group=self.group) assert group_snooze.count == 10 - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue archived by <@{self.external_id}>*" @@ -302,7 +302,7 @@ def test_archive_issue_until_condition_met_through_unfurl(self, mock_tags): group_snooze = GroupSnooze.objects.get(group=self.group) assert group_snooze.count == 100 - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue archived by <@{self.external_id}>*" @@ -318,7 +318,7 @@ def test_archive_issue_forever(self, mock_tags): assert self.group.get_status() == GroupStatus.IGNORED assert self.group.substatus == GroupSubStatus.FOREVER - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue archived by <@{self.external_id}>*" @@ -349,7 +349,7 @@ def test_archive_issue_forever_through_unfurl(self, mock_tags): assert self.group.get_status() == GroupStatus.IGNORED assert self.group.substatus == GroupSubStatus.FOREVER - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue archived by <@{self.external_id}>*" @@ -373,7 +373,7 @@ def test_archive_issue_with_additional_user_auth(self): assert self.group.get_status() == GroupStatus.IGNORED assert self.group.substatus == GroupSubStatus.FOREVER - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] expect_status = f"*Issue archived by <@{self.external_id}>*" assert self.notification_text in blocks[1]["text"]["text"] @@ -396,7 +396,7 @@ def test_archive_issue_with_additional_user_auth_through_unfurl(self): assert self.group.get_status() == GroupStatus.IGNORED assert self.group.substatus == GroupSubStatus.FOREVER - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] expect_status = f"*Issue archived by <@{self.external_id}>*" assert self.notification_text in blocks[1]["text"]["text"] @@ -420,7 +420,7 @@ def test_unarchive_issue(self, mock_tags): assert self.group.get_status() == GroupStatus.UNRESOLVED assert self.group.substatus == GroupSubStatus.NEW # the issue is less than 7 days old - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue re-opened by <@{self.external_id}>*" @@ -446,7 +446,7 @@ def test_unarchive_issue_through_unfurl(self, mock_tags): assert self.group.get_status() == GroupStatus.UNRESOLVED assert self.group.substatus == GroupSubStatus.NEW # the issue is less than 7 days old - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue re-opened by <@{self.external_id}>*" @@ -464,7 +464,7 @@ def test_assign_issue(self, mock_tags): assert GroupAssignee.objects.filter(group=self.group, user_id=user2.id).exists() assert mock_tags.call_args.kwargs["tags"] == self.tags - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] text = self.mock_post.call_args.kwargs["text"] expect_status = f"*Issue assigned to {user2.get_display_name()} by <@{self.external_id}>*" assert self.notification_text in blocks[1]["text"]["text"] @@ -476,7 +476,7 @@ def test_assign_issue(self, mock_tags): assert GroupAssignee.objects.filter(group=self.group, team=self.team).exists() assert mock_tags.call_args.kwargs["tags"] == self.tags - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] text = self.mock_post.call_args.kwargs["text"] expect_status = f"*Issue assigned to #{self.team.slug} by <@{self.external_id}>*" assert self.notification_text in blocks[1]["text"]["text"] @@ -546,7 +546,7 @@ def test_assign_issue_through_unfurl(self): # Assign to user self.assign_issue(original_message, user2, payload_data) assert GroupAssignee.objects.filter(group=self.group, user_id=user2.id).exists() - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] text = self.mock_post.call_args.kwargs["text"] expect_status = f"*Issue assigned to {user2.get_display_name()} by <@{self.external_id}>*" assert self.notification_text in blocks[1]["text"]["text"] @@ -555,7 +555,7 @@ def test_assign_issue_through_unfurl(self): # Assign to team self.assign_issue(original_message, self.team, payload_data) assert GroupAssignee.objects.filter(group=self.group, team=self.team).exists() - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] text = self.mock_post.call_args.kwargs["text"] expect_status = f"*Issue assigned to #{self.team.slug} by <@{self.external_id}>*" assert self.notification_text in blocks[1]["text"]["text"] @@ -615,7 +615,7 @@ def test_assign_issue_user_has_identity(self): self.assign_issue(original_message, user2) assert GroupAssignee.objects.filter(group=self.group, user_id=user2.id).exists() - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] text = self.mock_post.call_args.kwargs["text"] expect_status = ( @@ -638,7 +638,7 @@ def test_assign_issue_user_has_identity_through_unfurl(self): self.assign_issue(original_message, user2, payload_data) assert GroupAssignee.objects.filter(group=self.group, user_id=user2.id).exists() - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] text = self.mock_post.call_args.kwargs["text"] expect_status = ( f"*Issue assigned to <@{user2_identity.external_id}> by <@{self.external_id}>*" @@ -664,7 +664,7 @@ def test_assign_user_with_multiple_identities(self): self.assign_issue(original_message, self.user) assert GroupAssignee.objects.filter(group=self.group, user_id=self.user.id).exists() - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] text = self.mock_post.call_args.kwargs["text"] expect_status = "*Issue assigned to <@{assignee}> by <@{assignee}>*".format( assignee=self.external_id @@ -691,7 +691,7 @@ def test_assign_user_with_multiple_identities_through_unfurl(self): self.assign_issue(original_message, self.user, payload_data) assert GroupAssignee.objects.filter(group=self.group, user_id=self.user.id).exists() - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] text = self.mock_post.call_args.kwargs["text"] expect_status = "*Issue assigned to <@{assignee}> by <@{assignee}>*".format( assignee=self.external_id @@ -708,7 +708,7 @@ def test_resolve_issue(self, mock_tags): assert self.group.get_status() == GroupStatus.RESOLVED assert not GroupResolution.objects.filter(group=self.group) - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue resolved by <@{self.external_id}>*" @@ -738,7 +738,7 @@ def test_resolve_perf_issue(self, mock_tags): assert self.group.get_status() == GroupStatus.RESOLVED assert not GroupResolution.objects.filter(group=self.group) - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue resolved by <@{self.external_id}>*" @@ -759,7 +759,7 @@ def test_resolve_issue_through_unfurl(self, mock_tags): assert self.group.get_status() == GroupStatus.RESOLVED assert not GroupResolution.objects.filter(group=self.group) - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue resolved by <@{self.external_id}>*" @@ -783,7 +783,7 @@ def test_resolve_issue_in_current_release(self, mock_tags): assert resolution.type == GroupResolution.Type.in_release assert resolution.release == release - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue resolved by <@{self.external_id}>*" @@ -808,7 +808,7 @@ def test_resolve_issue_in_current_release_through_unfurl(self, mock_tags): assert resolution.type == GroupResolution.Type.in_release assert resolution.release == release - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue resolved by <@{self.external_id}>*" @@ -831,7 +831,7 @@ def test_resolve_in_next_release(self, mock_tags): assert resolution.type == GroupResolution.Type.in_next_release assert resolution.release == release - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue resolved by <@{self.external_id}>*" @@ -855,7 +855,7 @@ def test_resolve_in_next_release_through_unfurl(self, mock_tags): assert resolution.type == GroupResolution.Type.in_next_release assert resolution.release == release - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] assert mock_tags.call_args.kwargs["tags"] == self.tags expect_status = f"*Issue resolved by <@{self.external_id}>*" @@ -907,7 +907,7 @@ def test_response_differs_on_bot_message(self, mock_views_open): assert self.group.get_status() == GroupStatus.IGNORED assert self.group.substatus == GroupSubStatus.FOREVER - blocks = orjson.loads(self.mock_post.call_args.kwargs["blocks"]) + blocks = self.mock_post.call_args.kwargs["blocks"] expect_status = f"*Issue archived by <@{self.external_id}>*" assert self.notification_text in blocks[1]["text"]["text"] From 094a247e510d0363b53ab845b944eda9bb55d7d1 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Mon, 23 Sep 2024 12:07:04 -0700 Subject: [PATCH 4/4] :bug: fix: using ryan's helper function --- .../integrations/slack/webhooks/action.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/sentry/integrations/slack/webhooks/action.py b/src/sentry/integrations/slack/webhooks/action.py index c8346ca591b3a3..1b998f67f26cb1 100644 --- a/src/sentry/integrations/slack/webhooks/action.py +++ b/src/sentry/integrations/slack/webhooks/action.py @@ -231,6 +231,17 @@ def api_error( return self.respond_ephemeral(text) + @staticmethod + def _unpack_error_text(validation_error: serializers.ValidationError) -> str: + detail = validation_error.detail + while True: + if isinstance(detail, dict): + detail = list(detail.values()) + element = detail[0] + if isinstance(element, str): + return element + detail = element + def validation_error( self, slack_request: SlackActionRequest, @@ -247,12 +258,7 @@ def validation_error( }, ) - text: str = "" - if isinstance(error.detail, dict): - text = list(*error.detail.values())[0] - else: - text = str(error.detail[0]) - + text: str = self._unpack_error_text(error) return self.respond_ephemeral(text) def on_assign(