From 3a03e149381ddc89b2856df49c741d51c142c04e Mon Sep 17 00:00:00 2001 From: Lauryn Menard Date: Thu, 15 Feb 2024 12:25:57 +0100 Subject: [PATCH] integrations: Update Slack webhook to use check_send_webhook_message. Due to the channel_map_to_topics URL parameter in the Slack webhook, it was not migrated to use the check_send_webhook_message. By using check_send_webhook_message, any topic parameter in the webhook URL will be prioritized over mapping Slack channels to topics, e.g. when channel_map_to_topics is true. This is because the default behaviour for incoming webhooks is to send a default topic as a parameter to check_send_webhook_message in case there is no topic specified in the URL. In contrast, we can override the stream passed in the URL when channel_map_to_topics is false by passing the Slack channel name to check_send_webhook_message. The default behaviour for incoming webhooks is to send a direct message if there is no specified stream in the URL, so a default stream is not generally passed to check_send_webhook_message. Fixes #27601. --- zerver/webhooks/slack/doc.md | 22 ++++++------ zerver/webhooks/slack/tests.py | 64 +++++++++++++++++++++++++++------- zerver/webhooks/slack/view.py | 50 ++++++++++++++++++-------- 3 files changed, 98 insertions(+), 38 deletions(-) diff --git a/zerver/webhooks/slack/doc.md b/zerver/webhooks/slack/doc.md index 3343a4344b..74e54744b8 100644 --- a/zerver/webhooks/slack/doc.md +++ b/zerver/webhooks/slack/doc.md @@ -7,20 +7,20 @@ See also the [Slack-compatible webhook](/integrations/doc/slack_incoming). 1. {!create-an-incoming-webhook.md!} - Construct the URL for the {{ integration_display_name }} - bot using the bot's API key: +1. {!generate-integration-url.md!} - `{{ api_url }}{{ integration_url }}?api_key=abcdefgh` - - If you'd like to map Slack channels to different topics within the - same stream, add `&channels_map_to_topics=1` to the end of the URL. - This is the default behavior of the integration. + If you'd like to map Slack channels to different topics within the same + stream, add `&channels_map_to_topics=1` to the end of the URL. Note that + this should be used instead of specifying a topic in the URL. If a topic + is specified in the URL, then it will be prioritized over the channel to + topic mapping. If you'd like to map Slack channels to different streams, add - `&channels_map_to_topics=0` to the end of the URL. Make sure you - create streams for all your public Slack channels, and that the name - of each stream is the same as the name of the Slack channel it maps - to. + `&channels_map_to_topics=0` to the end of the URL. Make sure you create + streams for all your public Slack channels *(see step 1)*, and that the + name of each stream is the same as the name of the Slack channel it maps + to. Note that in this case, the channel to stream mapping will be + prioritized over the stream specified in the URL. 1. Go to and click **Add Outgoing WebHooks integration**. diff --git a/zerver/webhooks/slack/tests.py b/zerver/webhooks/slack/tests.py index 40b61de981..eb5bdb80d0 100644 --- a/zerver/webhooks/slack/tests.py +++ b/zerver/webhooks/slack/tests.py @@ -2,31 +2,71 @@ from typing_extensions import override from zerver.lib.test_classes import WebhookTestCase +EXPECTED_TOPIC = "Message from Slack" +EXPECTED_MESSAGE = "**slack_user**: test" + class SlackWebhookTests(WebhookTestCase): STREAM_NAME = "slack" URL_TEMPLATE = "/api/v1/external/slack?stream={stream}&api_key={api_key}" WEBHOOK_DIR_NAME = "slack" - def test_slack_channel_to_topic(self) -> None: - expected_topic_name = "channel: general" - expected_message = "**slack_user**: test" + def test_slack_only_stream_parameter(self) -> None: self.check_webhook( "message_info", - expected_topic_name, - expected_message, + EXPECTED_TOPIC, + EXPECTED_MESSAGE, content_type="application/x-www-form-urlencoded", ) - def test_slack_channel_to_stream(self) -> None: - self.STREAM_NAME = "general" - self.url = "{}{}".format(self.url, "&channels_map_to_topics=0") - expected_topic_name = "Message from Slack" - expected_message = "**slack_user**: test" + def test_slack_with_user_specified_topic(self) -> None: + self.url = self.build_webhook_url(topic="test") + expected_topic_name = "test" self.check_webhook( "message_info", expected_topic_name, - expected_message, + EXPECTED_MESSAGE, + content_type="application/x-www-form-urlencoded", + ) + + def test_slack_channels_map_to_topics_true(self) -> None: + self.url = self.build_webhook_url(channels_map_to_topics="1") + expected_topic_name = "channel: general" + self.check_webhook( + "message_info", + expected_topic_name, + EXPECTED_MESSAGE, + content_type="application/x-www-form-urlencoded", + ) + + def test_slack_channels_map_to_topics_true_and_user_specified_topic(self) -> None: + self.url = self.build_webhook_url(topic="test", channels_map_to_topics="1") + expected_topic_name = "test" + self.check_webhook( + "message_info", + expected_topic_name, + EXPECTED_MESSAGE, + content_type="application/x-www-form-urlencoded", + ) + + def test_slack_channels_map_to_topics_false(self) -> None: + self.STREAM_NAME = "general" + self.url = self.build_webhook_url(channels_map_to_topics="0") + self.check_webhook( + "message_info", + EXPECTED_TOPIC, + EXPECTED_MESSAGE, + content_type="application/x-www-form-urlencoded", + ) + + def test_slack_channels_map_to_topics_false_and_user_specified_topic(self) -> None: + self.STREAM_NAME = "general" + self.url = self.build_webhook_url(topic="test", channels_map_to_topics="0") + expected_topic_name = "test" + self.check_webhook( + "message_info", + expected_topic_name, + EXPECTED_MESSAGE, content_type="application/x-www-form-urlencoded", ) @@ -50,7 +90,7 @@ class SlackWebhookTests(WebhookTestCase): def test_invalid_channels_map_to_topics(self) -> None: payload = self.get_body("message_info") - url = "{}{}".format(self.url, "&channels_map_to_topics=abc") + url = self.build_webhook_url(channels_map_to_topics="abc") result = self.client_post(url, payload, content_type="application/x-www-form-urlencoded") self.assert_json_error(result, "Error: channels_map_to_topics parameter other than 0 or 1") diff --git a/zerver/webhooks/slack/view.py b/zerver/webhooks/slack/view.py index 4cf130d799..6622098701 100644 --- a/zerver/webhooks/slack/view.py +++ b/zerver/webhooks/slack/view.py @@ -1,13 +1,14 @@ +from typing import Optional + from django.http import HttpRequest from django.http.response import HttpResponse from django.utils.translation import gettext as _ -from zerver.actions.message_send import check_send_stream_message from zerver.decorator import webhook_view from zerver.lib.exceptions import JsonableError -from zerver.lib.request import RequestNotes from zerver.lib.response import json_success from zerver.lib.typed_endpoint import typed_endpoint +from zerver.lib.webhooks.common import check_send_webhook_message from zerver.models import UserProfile ZULIP_MESSAGE_TEMPLATE = "**{message_sender}**: {text}" @@ -23,20 +24,39 @@ def api_slack_webhook( user_name: str, text: str, channel_name: str, - stream: str = "slack", - channels_map_to_topics: str = "1", + channels_map_to_topics: Optional[str] = None, ) -> HttpResponse: - if channels_map_to_topics not in VALID_OPTIONS.values(): + content = ZULIP_MESSAGE_TEMPLATE.format(message_sender=user_name, text=text) + topic_name = "Message from Slack" + + if channels_map_to_topics is None: + check_send_webhook_message( + request, + user_profile, + topic_name, + content, + ) + elif channels_map_to_topics == VALID_OPTIONS["SHOULD_BE_MAPPED"]: + # If the webhook URL has a user_specified_topic, + # then this topic-channel mapping will not be used. + topic_name = f"channel: {channel_name}" + check_send_webhook_message( + request, + user_profile, + topic_name, + content, + ) + elif channels_map_to_topics == VALID_OPTIONS["SHOULD_NOT_BE_MAPPED"]: + # This stream-channel mapping will be used even if + # there is a stream specified in the webhook URL. + check_send_webhook_message( + request, + user_profile, + topic_name, + content, + stream=channel_name, + ) + else: raise JsonableError(_("Error: channels_map_to_topics parameter other than 0 or 1")) - if channels_map_to_topics == VALID_OPTIONS["SHOULD_BE_MAPPED"]: - topic_name = f"channel: {channel_name}" - else: - stream = channel_name - topic_name = _("Message from Slack") - - content = ZULIP_MESSAGE_TEMPLATE.format(message_sender=user_name, text=text) - client = RequestNotes.get_notes(request).client - assert client is not None - check_send_stream_message(user_profile, client, stream, topic_name, content) return json_success(request)