outgoing_webhook: Join build_bot_request and send_data_to_server.

The existing organization, of returning an opaque blob from
`build_bot_request`, which was later consumed by
`send_data_to_server`, is not particularly sensible; the steps become
oddly split between the OutgoingWebhookWorker, `do_rest_call`, and the
`OutgoingWebhookServiceInterface`.

Make the `OutgoingWebhookServiceInterface` in charge of building,
making, and returning the request in one method; another method
handles extracting content from a successful response.  `do_rest_call`
is responsible for calling both halves of this, and doing common error
handling.

zerver/lib/outgoing_webhook.py

@@ -32,11 +32,7 @@ class OutgoingWebhookServiceInterface(metaclass=abc.ABCMeta):
         self.service_name: str = service_name
 
     @abc.abstractmethod
-    def build_bot_request(self, event: Dict[str, Any]) -> Optional[Any]:
+    def make_request(self, base_url: str, event: Dict[str, Any]) -> Optional[Response]:
-        raise NotImplementedError
-
-    @abc.abstractmethod
-    def send_data_to_server(self, base_url: str, request_data: Any) -> Response:
         raise NotImplementedError
 
     @abc.abstractmethod
@@ -45,7 +41,7 @@ class OutgoingWebhookServiceInterface(metaclass=abc.ABCMeta):
 
 
 class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface):
-    def build_bot_request(self, event: Dict[str, Any]) -> Optional[Any]:
+    def make_request(self, base_url: str, event: Dict[str, Any]) -> Optional[Response]:
         """
         We send a simple version of the message to outgoing
         webhooks, since most of them really only need
@@ -69,9 +65,7 @@ class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface):
             "token": self.token,
             "trigger": event["trigger"],
         }
-        return json.dumps(request_data)
 
-    def send_data_to_server(self, base_url: str, request_data: Any) -> Response:
         user_agent = "ZulipOutgoingWebhook/" + ZULIP_VERSION
         headers = {
             "User-Agent": user_agent,
@@ -99,7 +93,7 @@ class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface):
 
 
 class SlackOutgoingWebhookService(OutgoingWebhookServiceInterface):
-    def build_bot_request(self, event: Dict[str, Any]) -> Optional[Any]:
+    def make_request(self, base_url: str, event: Dict[str, Any]) -> Optional[Response]:
         if event["message"]["type"] == "private":
             failure_message = "Slack outgoing webhooks don't support private messages."
             fail_with_message(event, failure_message)
@@ -118,12 +112,7 @@ class SlackOutgoingWebhookService(OutgoingWebhookServiceInterface):
             ("trigger_word", event["trigger"]),
             ("service_id", event["user_profile_id"]),
         ]
-
+        return requests.request("POST", base_url, data=request_data)
-        return request_data
-
-    def send_data_to_server(self, base_url: str, request_data: Any) -> Response:
-        response = requests.request("POST", base_url, data=request_data)
-        return response
 
     def process_success(self, response_json: Dict[str, Any]) -> Optional[Dict[str, Any]]:
         if "text" in response_json:
@@ -311,14 +300,16 @@ def process_success_response(
 
 
 def do_rest_call(
-    base_url: str, request_data: Any, event: Dict[str, Any], service_handler: Any
+    base_url: str, event: Dict[str, Any], service_handler: Any
 ) -> Optional[Response]:
     """Returns response of call if no exception occurs."""
     try:
-        response = service_handler.send_data_to_server(
+        response = service_handler.make_request(
-            base_url=base_url,
+            base_url,
-            request_data=request_data,
+            event,
         )
+        if not response:
+            return None
         if str(response.status_code).startswith("2"):
             process_success_response(event, service_handler, response)
         else:

zerver/tests/test_outgoing_webhook_interfaces.py

@@ -55,7 +55,7 @@ class TestGenericOutgoingWebhookService(ZulipTestCase):
             )
         self.assertTrue(m.called)
 
-    def test_build_bot_request(self) -> None:
+    def test_make_request(self) -> None:
         othello = self.example_user("othello")
         stream = get_stream("Denmark", othello.realm)
         message_id = self.send_stream_message(
@@ -102,8 +102,25 @@ class TestGenericOutgoingWebhookService(ZulipTestCase):
             "trigger": "mention",
         }
 
-        request_data = self.handler.build_bot_request(event)
+        test_url = "https://example.com/example"
-        request_data = json.loads(request_data)
+        response = mock.Mock(spec=requests.Response)
+        response.status_code = 200
+        expect_200 = mock.patch("requests.request", return_value=response)
+        with expect_200 as mock_request:
+            self.handler.make_request(
+                test_url,
+                event,
+            )
+            mock_request.assert_called_once()
+            self.assertEqual(
+                mock_request.call_args[0],
+                (
+                    "POST",
+                    test_url,
+                ),
+            )
+            request_data = mock_request.call_args[1]["json"]
+
         validate_against_openapi_schema(request_data, "/zulip-outgoing-webhook", "post", "200")
         self.assertEqual(request_data["data"], "@**test**")
         self.assertEqual(request_data["token"], "abcdef")
@@ -180,8 +197,22 @@ class TestSlackOutgoingWebhookService(ZulipTestCase):
         service_class = get_service_interface_class(SLACK_INTERFACE)
         self.handler = service_class(token="abcdef", user_profile=None, service_name="test-service")
 
-    def test_build_bot_request_stream_message(self) -> None:
+    def test_make_request_stream_message(self) -> None:
-        request_data = self.handler.build_bot_request(self.stream_message_event)
+        test_url = "https://example.com/example"
+        with mock.patch("requests.request") as mock_request:
+            self.handler.make_request(
+                test_url,
+                self.stream_message_event,
+            )
+            mock_request.assert_called_once()
+            self.assertEqual(
+                mock_request.call_args[0],
+                (
+                    "POST",
+                    test_url,
+                ),
+            )
+            request_data = mock_request.call_args[1]["data"]
 
         self.assertEqual(request_data[0][1], "abcdef")  # token
         self.assertEqual(request_data[1][1], "zulip")  # team_id
@@ -196,10 +227,15 @@ class TestSlackOutgoingWebhookService(ZulipTestCase):
         self.assertEqual(request_data[10][1], 12)  # user_profile_id
 
     @mock.patch("zerver.lib.outgoing_webhook.fail_with_message")
-    def test_build_bot_request_private_message(self, mock_fail_with_message: mock.Mock) -> None:
+    def test_make_request_private_message(self, mock_fail_with_message: mock.Mock) -> None:
-
+        test_url = "https://example.com/example"
-        request_data = self.handler.build_bot_request(self.private_message_event)
+        with mock.patch("requests.request") as mock_request:
-        self.assertIsNone(request_data)
+            response = self.handler.make_request(
+                test_url,
+                self.private_message_event,
+            )
+            mock_request.assert_not_called()
+        self.assertIsNone(response)
         self.assertTrue(mock_fail_with_message.called)
 
     def test_process_success(self) -> None:

zerver/tests/test_outgoing_webhook_system.py

@@ -47,10 +47,23 @@ class DoRestCallTests(ZulipTestCase):
             "message": {
                 "display_recipient": "Verona",
                 "stream_id": 999,
+                "sender_id": bot_user.id,
+                "sender_email": bot_user.email,
+                "sender_realm_id": bot_user.realm.id,
+                "sender_realm_str": bot_user.realm.string_id,
+                "sender_delivery_email": bot_user.delivery_email,
+                "sender_full_name": bot_user.full_name,
+                "sender_avatar_source": UserProfile.AVATAR_FROM_GRAVATAR,
+                "sender_avatar_version": 1,
+                "recipient_type": "stream",
+                "recipient_type_id": 999,
+                "sender_is_mirror_dummy": False,
                 TOPIC_NAME: "Foo",
                 "id": "",
                 "type": "stream",
+                "timestamp": 1,
             },
+            "trigger": "mention",
             "user_profile_id": bot_user.id,
             "command": "",
             "service_name": "",
@@ -66,14 +79,14 @@ class DoRestCallTests(ZulipTestCase):
 
         expect_send_response = mock.patch("zerver.lib.outgoing_webhook.send_response_message")
         with expect_200, expect_send_response as mock_send:
-            do_rest_call("", None, mock_event, service_handler)
+            do_rest_call("", mock_event, service_handler)
         self.assertTrue(mock_send.called)
 
         for service_class in [GenericOutgoingWebhookService, SlackOutgoingWebhookService]:
             handler = service_class("token", bot_user, "service")
 
             with expect_200:
-                do_rest_call("", None, mock_event, handler)
+                do_rest_call("", mock_event, handler)
 
             # TODO: assert something interesting here?
 
@@ -86,7 +99,7 @@ class DoRestCallTests(ZulipTestCase):
         with mock.patch("requests.request", return_value=response), self.assertLogs(
             level="WARNING"
         ) as m:
-            final_response = do_rest_call("", None, mock_event, service_handler)
+            final_response = do_rest_call("", mock_event, service_handler)
             assert final_response is not None
 
             self.assertEqual(
@@ -115,7 +128,7 @@ The webhook got a response with status code *500*.""",
         expect_fail = mock.patch("zerver.lib.outgoing_webhook.fail_with_message")
 
         with expect_400, expect_fail as mock_fail, self.assertLogs(level="WARNING") as m:
-            final_response = do_rest_call("", None, mock_event, service_handler)
+            final_response = do_rest_call("", mock_event, service_handler)
             assert final_response is not None
 
             self.assertEqual(
@@ -144,9 +157,7 @@ The webhook got a response with status code *400*.""",
 
         with mock.patch("requests.sessions.Session.send") as mock_send:
             mock_send.return_value = ResponseMock(200)
-            final_response = do_rest_call(
+            final_response = do_rest_call("https://example.com/", mock_event, service_handler)
-                "https://example.com/", "payload-stub", mock_event, service_handler
-            )
             assert final_response is not None
 
             mock_send.assert_called_once()
@@ -167,7 +178,7 @@ The webhook got a response with status code *400*.""",
         def helper(side_effect: Any, error_text: str) -> None:
 
             with mock.patch("requests.request", side_effect=side_effect):
-                do_rest_call("", None, mock_event, service_handler)
+                do_rest_call("", mock_event, service_handler)
 
             bot_owner_notification = self.get_last_message()
             self.assertIn(error_text, bot_owner_notification.content)
@@ -202,7 +213,7 @@ The webhook got a response with status code *400*.""",
         # Don't think that we should catch and assert whole log output(which is actually a very big error traceback).
         # We are already asserting bot_owner_notification.content which verifies exception did occur.
         with expect_request_exception, expect_logging_exception, expect_fail as mock_fail:
-            do_rest_call("", None, mock_event, service_handler)
+            do_rest_call("", mock_event, service_handler)
 
         self.assertTrue(mock_fail.called)
 
@@ -260,7 +271,8 @@ class TestOutgoingWebhookMessaging(ZulipTestCase):
 
         sender = self.example_user("hamlet")
 
-        with mock.patch("zerver.worker.queue_processors.do_rest_call") as m:
+        with mock.patch("requests.request") as mock_request:
+            mock_request.return_value = ResponseMock(200)
             self.send_personal_message(
                 sender,
                 bot,
@@ -268,10 +280,11 @@ class TestOutgoingWebhookMessaging(ZulipTestCase):
             )
 
         url_token_tups = set()
-        for item in m.call_args_list:
+        for item in mock_request.call_args_list:
             args = item[0]
-            base_url = args[0]
+            base_url = args[1]
-            request_data = orjson.loads(args[1])
+            kwargs = item[1]
+            request_data = kwargs["json"]
             tup = (base_url, request_data["token"])
             url_token_tups.add(tup)
             message_data = request_data["message"]

zerver/worker/queue_processors.py

@@ -742,9 +742,7 @@ class OutgoingWebhookWorker(QueueProcessingWorker):
         for service in services:
             event["service_name"] = str(service.name)
             service_handler = get_outgoing_webhook_service_handler(service)
-            request_data = service_handler.build_bot_request(event)
+            do_rest_call(service.base_url, event, service_handler)
-            if request_data:
-                do_rest_call(service.base_url, request_data, event, service_handler)
 
 
 @assign_queue("embedded_bots")