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

@@ -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 <https://my.slack.com/services/new/outgoing-webhook>
    and click **Add Outgoing WebHooks integration**.

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")
 

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)