refactor: Have process_success return structured data.

Before this change subclasses of OutgoingWebhookServiceInterface
would return a raw string as the first element of its return
tuple in process_success().  This is not a very flexible
design, as it prevents the bot from passing extra data like
`widget_content`.

It's also possible in the future that we'll want to let outgoing
bots reply directly to senders who mention them on streams, and
again the original design was overly constrained for that.

This commit does not actually change any functionality yet.

zerver/lib/outgoing_webhook.py

@@ -41,12 +41,12 @@ class OutgoingWebhookServiceInterface:
         raise NotImplementedError()
 
     # Given a successful outgoing webhook REST operation, returns two-element tuple
-    # whose left-hand value contains a success message
+    # whose left-hand value contains a success dictionary
     # to sent back to the user (or None if no success message should be sent)
     # and right-hand value contains a failure message
     # to sent back to the user (or None if no failure message should be sent)
     def process_success(self, response: Response,
-                        event: Dict[str, Any]) -> Tuple[Optional[str], Optional[str]]:
+                        event: Dict[str, Any]) -> Tuple[Optional[Dict[str, Any]], Optional[str]]:
         raise NotImplementedError()
 
 class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface):
@@ -64,13 +64,16 @@ class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface):
         return rest_operation, json.dumps(request_data)
 
     def process_success(self, response: Response,
-                        event: Dict[str, Any]) -> Tuple[Optional[str], Optional[str]]:
+                        event: Dict[str, Any]) -> Tuple[Optional[Dict[str, Any]], Optional[str]]:
         response_json = json.loads(response.text)
 
         if "response_not_required" in response_json and response_json['response_not_required']:
             return None, None
         if "response_string" in response_json:
-            return str(response_json['response_string']), None
+            success_data = dict(
+                response_string=str(response_json['response_string']),
+            )
+            return success_data, None
         else:
             return None, None
 
@@ -103,10 +106,12 @@ class SlackOutgoingWebhookService(OutgoingWebhookServiceInterface):
         return rest_operation, request_data
 
     def process_success(self, response: Response,
-                        event: Dict[str, Any]) -> Tuple[Optional[str], Optional[str]]:
+                        event: Dict[str, Any]) -> Tuple[Optional[Dict[str, Any]], Optional[str]]:
         response_json = json.loads(response.text)
         if "text" in response_json:
-            return response_json["text"], None
+            response_string = response_json['text']
+            success_data = dict(response_string=response_string)
+            return success_data, None
         else:
             return None, None
 
@@ -225,9 +230,16 @@ def request_retry(event: Dict[str, Any],
 def process_success_response(event: Dict[str, Any],
                              service_handler: Any,
                              response: Response) -> None:
-    success_message, _ = service_handler.process_success(response, event)
-    if success_message is not None:
-        succeed_with_message(event, success_message)
+    success_data, _ = service_handler.process_success(response, event)
+
+    if success_data is None:
+        return
+
+    success_message = success_data.get('response_string')
+    if success_message is None:
+        return
+
+    succeed_with_message(event, success_message)
 
 def do_rest_call(rest_operation: Dict[str, Any],
                  request_data: Optional[Dict[str, Any]],

zerver/tests/test_outgoing_webhook_interfaces.py

@@ -43,7 +43,7 @@ class TestGenericOutgoingWebhookService(ZulipTestCase):
 
         response.text = json.dumps({"response_string": 'test_content'})
         success_response, _ = self.handler.process_success(response, self.event)
-        self.assertEqual(success_response, 'test_content')
+        self.assertEqual(success_response, dict(response_string='test_content'))
 
         response.text = json.dumps({})
         success_response, _ = self.handler.process_success(response, self.event)
@@ -130,4 +130,4 @@ class TestSlackOutgoingWebhookService(ZulipTestCase):
 
         response.text = json.dumps({"text": 'test_content'})
         success_response, _ = self.handler.process_success(response, self.stream_message_event)
-        self.assertEqual(success_response, 'test_content')
+        self.assertEqual(success_response, dict(response_string='test_content'))

zerver/tests/test_outgoing_webhook_system.py

@@ -27,8 +27,13 @@ def timeout_error(http_method: Any, final_url: Any, data: Any, **request_kwargs:
     raise requests.exceptions.Timeout("Time is up!")
 
 class MockServiceHandler(OutgoingWebhookServiceInterface):
-    def process_success(self, response: Response, event: Dict[str, Any]) -> Tuple[Optional[str], Optional[str]]:
-        return "Success!", None
+    def process_success(self, response: Response, event: Dict[str, Any]) -> Tuple[Optional[Dict[str, Any]], Optional[str]]:
+        # Our tests don't really look at the content yet.
+        # They just ensure we use the "success" codepath.
+        success_data = dict(
+            response_string="whatever",
+        )
+        return success_data, None
 
 service_handler = MockServiceHandler(None, None, None, None)