refactor: Parse JSON from bots in one place.

We move the JSON parsing step into the
higher level function: process_success_response().

In the unlikely event that we'll start integrating
with a solution that doesn't use JSON, we can deal
with that, and for now doing the parsing in one
place will help us make error reporting more
consistent.

In a subsequent commit we'll introduce better
error handling for malformed JSON.

zerver/lib/outgoing_webhook.py

@@ -45,7 +45,7 @@ class OutgoingWebhookServiceInterface:
     # The main use case for this function is to massage data from
     # various APIs to have similar data structures.
     # It also allows bots to explictly set response_not_required.
-    def process_success(self, response: Response,
+    def process_success(self, response_json: Dict[str, Any],
                         event: Dict[str, Any]) -> Optional[Dict[str, Any]]:
         raise NotImplementedError()
 
@@ -63,10 +63,8 @@ class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface):
                         "trigger": event['trigger']}
         return rest_operation, json.dumps(request_data)
 
-    def process_success(self, response: Response,
+    def process_success(self, response_json: Dict[str, Any],
                         event: Dict[str, Any]) -> Optional[Dict[str, Any]]:
-        response_json = json.loads(response.text)
-
         if "response_not_required" in response_json and response_json['response_not_required']:
             return None
 
@@ -111,10 +109,8 @@ class SlackOutgoingWebhookService(OutgoingWebhookServiceInterface):
 
         return rest_operation, request_data
 
-    def process_success(self, response: Response,
+    def process_success(self, response_json: Dict[str, Any],
                         event: Dict[str, Any]) -> Optional[Dict[str, Any]]:
-        response_json = json.loads(response.text)
-
         if "text" in response_json:
             content = response_json['text']
             success_data = dict(content=content)
@@ -258,7 +254,8 @@ def request_retry(event: Dict[str, Any],
 def process_success_response(event: Dict[str, Any],
                              service_handler: Any,
                              response: Response) -> None:
-    success_data = service_handler.process_success(response, event)
+    response_json = json.loads(response.text)
+    success_data = service_handler.process_success(response_json, event)
 
     if success_data is None:
         return

zerver/tests/test_outgoing_webhook_interfaces.py

@@ -1,10 +1,9 @@
 # -*- coding: utf-8 -*-
-from typing import Any
+from typing import Any, Dict
 
 import mock
 import json
 
-from requests.models import Response
 from zerver.lib.outgoing_webhook import GenericOutgoingWebhookService, \
     SlackOutgoingWebhookService
 from zerver.lib.test_classes import ZulipTestCase
@@ -36,16 +35,15 @@ class TestGenericOutgoingWebhookService(ZulipTestCase):
         self.assertEqual(request_data['message'], self.event['message'])
 
     def test_process_success(self) -> None:
-        response = mock.Mock(spec=Response)
+        response = dict(response_not_required=True)  # type: Dict[str, Any]
-        response.text = json.dumps({"response_not_required": True})
         success_response = self.handler.process_success(response, self.event)
         self.assertEqual(success_response, None)
 
-        response.text = json.dumps({"response_string": 'test_content'})
+        response = dict(response_string='test_content')
         success_response = self.handler.process_success(response, self.event)
         self.assertEqual(success_response, dict(content='test_content'))
 
-        response.text = json.dumps({})
+        response = dict()
         success_response = self.handler.process_success(response, self.event)
         self.assertEqual(success_response, None)
 
@@ -123,11 +121,10 @@ class TestSlackOutgoingWebhookService(ZulipTestCase):
         self.assertTrue(mock_fail_with_message.called)
 
     def test_process_success(self) -> None:
-        response = mock.Mock(spec=Response)
+        response = dict(response_not_required=True)  # type: Dict[str, Any]
-        response.text = json.dumps({"response_not_required": True})
         success_response = self.handler.process_success(response, self.stream_message_event)
         self.assertEqual(success_response, None)
 
-        response.text = json.dumps({"text": 'test_content'})
+        response = dict(text='test_content')
         success_response = self.handler.process_success(response, self.stream_message_event)
         self.assertEqual(success_response, dict(content='test_content'))

zerver/tests/test_outgoing_webhook_system.py

@@ -27,7 +27,7 @@ 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]) -> Optional[Dict[str, Any]]:
+    def process_success(self, response_json: Dict[str, Any], event: Dict[str, Any]) -> Optional[Dict[str, Any]]:
         # Our tests don't really look at the content yet.
         # They just ensure we use the "success" codepath.
         success_data = dict(