From 69ee84bb148fa27e5d9934dbda741892436c793e Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Wed, 10 Oct 2018 22:45:19 +0000 Subject: [PATCH] refactor: Extract build_bot_request(). This fixes a couple things: * process_event() is a pretty vague name * returning tuples should generally be avoided * we were producing the same REST parameters in both subclasses * relative_url_path was always blank * request_kwargs was always empty Now process_event() is called build_bot_request(), and it only returns request data, not a tuple of `rest_operation` and `request_data`. By no longer returning `rest_operation`, there are fewer moving parts. We just have `do_rest_call` make a POST call. --- zerver/lib/outgoing_webhook.py | 47 ++++--------------- .../tests/test_outgoing_webhook_interfaces.py | 17 +++---- zerver/tests/test_outgoing_webhook_system.py | 13 ++--- zerver/worker/queue_processors.py | 5 +- 4 files changed, 22 insertions(+), 60 deletions(-) diff --git a/zerver/lib/outgoing_webhook.py b/zerver/lib/outgoing_webhook.py index f79fbfb976..175dd151c3 100644 --- a/zerver/lib/outgoing_webhook.py +++ b/zerver/lib/outgoing_webhook.py @@ -27,15 +27,7 @@ class OutgoingWebhookServiceInterface: self.user_profile = user_profile # type: UserProfile self.service_name = service_name # type: str - # Given an event that triggers an outgoing webhook operation, returns: - # - The REST operation that should be performed - # - The body of the request - # - # The REST operation is a dictionary with the following keys: - # - method - # - relative_url_path - # - request_kwargs - def process_event(self, event: Dict[str, Any]) -> Tuple[Dict[str, Any], Any]: + def build_bot_request(self, event: Dict[str, Any]) -> Optional[Any]: raise NotImplementedError() # Given a successful outgoing webhook REST operation, return @@ -49,16 +41,13 @@ class OutgoingWebhookServiceInterface: class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface): - def process_event(self, event: Dict[str, Any]) -> Tuple[Dict[str, Any], Any]: - rest_operation = {'method': 'POST', - 'relative_url_path': '', - 'request_kwargs': {}} + def build_bot_request(self, event: Dict[str, Any]) -> Optional[Any]: request_data = {"data": event['command'], "message": event['message'], "bot_email": self.user_profile.email, "token": self.token, "trigger": event['trigger']} - return rest_operation, json.dumps(request_data) + return json.dumps(request_data) def process_success(self, response_json: Dict[str, Any], event: Dict[str, Any]) -> Optional[Dict[str, Any]]: @@ -80,15 +69,11 @@ class GenericOutgoingWebhookService(OutgoingWebhookServiceInterface): class SlackOutgoingWebhookService(OutgoingWebhookServiceInterface): - def process_event(self, event: Dict[str, Any]) -> Tuple[Dict[str, Any], Any]: - rest_operation = {'method': 'POST', - 'relative_url_path': '', - 'request_kwargs': {}} - + def build_bot_request(self, event: Dict[str, Any]) -> Optional[Any]: if event['message']['type'] == 'private': failure_message = "Slack outgoing webhooks don't support private messages." fail_with_message(event, failure_message) - return None, None + return None request_data = [("token", self.token), ("team_id", event['message']['sender_realm_str']), @@ -103,7 +88,7 @@ class SlackOutgoingWebhookService(OutgoingWebhookServiceInterface): ("service_id", event['user_profile_id']), ] - return rest_operation, request_data + return request_data def process_success(self, response_json: Dict[str, Any], event: Dict[str, Any]) -> Optional[Dict[str, Any]]: @@ -273,28 +258,12 @@ def process_success_response(event: Dict[str, Any], send_response_message(bot_id=bot_id, message_info=message_info, response_data=response_data) def do_rest_call(base_url: str, - rest_operation: Dict[str, Any], - request_data: Optional[Dict[str, Any]], + request_data: Any, event: Dict[str, Any], service_handler: Any, timeout: Any=None) -> None: - rest_operation_validator = check_dict([ - ('method', check_string), - ('relative_url_path', check_string), - ('request_kwargs', check_dict([])), - ]) - - error = rest_operation_validator('rest_operation', rest_operation) - if error: - raise JsonableError(error) - - http_method = rest_operation['method'] - final_url = urllib.parse.urljoin(base_url, rest_operation['relative_url_path']) - request_kwargs = rest_operation['request_kwargs'] - request_kwargs['timeout'] = timeout - try: - response = requests.request(http_method, final_url, data=request_data, **request_kwargs) + response = requests.request('POST', base_url, data=request_data, timeout=timeout) if str(response.status_code).startswith('2'): process_success_response(event, service_handler, response) else: diff --git a/zerver/tests/test_outgoing_webhook_interfaces.py b/zerver/tests/test_outgoing_webhook_interfaces.py index 287fd43ded..7bd34f2e96 100644 --- a/zerver/tests/test_outgoing_webhook_interfaces.py +++ b/zerver/tests/test_outgoing_webhook_interfaces.py @@ -59,12 +59,11 @@ class TestGenericOutgoingWebhookService(ZulipTestCase): ) self.assertTrue(m.called) - def test_process_event(self) -> None: - rest_operation, request_data = self.handler.process_event(self.event) + def test_build_bot_request(self) -> None: + request_data = self.handler.build_bot_request(self.event) request_data = json.loads(request_data) self.assertEqual(request_data['data'], "@**test**") self.assertEqual(request_data['token'], "abcdef") - self.assertEqual(rest_operation['method'], "POST") self.assertEqual(request_data['message'], self.event['message']) def test_process_success(self) -> None: @@ -125,10 +124,9 @@ class TestSlackOutgoingWebhookService(ZulipTestCase): user_profile=None, service_name='test-service') - def test_process_event_stream_message(self) -> None: - rest_operation, request_data = self.handler.process_event(self.stream_message_event) + def test_build_bot_request_stream_message(self) -> None: + request_data = self.handler.build_bot_request(self.stream_message_event) - self.assertEqual(rest_operation['method'], 'POST') self.assertEqual(request_data[0][1], "abcdef") # token self.assertEqual(request_data[1][1], "zulip") # team_id self.assertEqual(request_data[2][1], "zulip.com") # team_domain @@ -143,12 +141,11 @@ class TestSlackOutgoingWebhookService(ZulipTestCase): @mock.patch('zerver.lib.outgoing_webhook.get_service_profile', return_value=mock_service) @mock.patch('zerver.lib.outgoing_webhook.fail_with_message') - def test_process_event_private_message(self, mock_fail_with_message: mock.Mock, - mock_get_service_profile: mock.Mock) -> None: + def test_build_bot_request_private_message(self, mock_fail_with_message: mock.Mock, + mock_get_service_profile: mock.Mock) -> None: - rest_operation, request_data = self.handler.process_event(self.private_message_event) + request_data = self.handler.build_bot_request(self.private_message_event) self.assertIsNone(request_data) - self.assertIsNone(rest_operation) self.assertTrue(mock_fail_with_message.called) def test_process_success(self) -> None: diff --git a/zerver/tests/test_outgoing_webhook_system.py b/zerver/tests/test_outgoing_webhook_system.py index 5085ab0d81..a57cf834a0 100644 --- a/zerver/tests/test_outgoing_webhook_system.py +++ b/zerver/tests/test_outgoing_webhook_system.py @@ -58,9 +58,6 @@ class DoRestCallTests(ZulipTestCase): 'command': '', 'service_name': ''} - self.rest_operation = {'method': "POST", - 'relative_url_path': "", - 'request_kwargs': {}} self.bot_user = self.example_user('outgoing_webhook_bot') logging.disable(logging.WARNING) @@ -68,7 +65,7 @@ class DoRestCallTests(ZulipTestCase): def test_successful_request(self, mock_send: mock.Mock) -> None: response = ResponseMock(200) with mock.patch('requests.request', return_value=response): - do_rest_call('', self.rest_operation, None, self.mock_event, service_handler, None) + do_rest_call('', None, self.mock_event, service_handler, None) self.assertTrue(mock_send.called) def test_retry_request(self: mock.Mock) -> None: @@ -76,7 +73,7 @@ class DoRestCallTests(ZulipTestCase): self.mock_event['failed_tries'] = 3 with mock.patch('requests.request', return_value=response): - do_rest_call('', self.rest_operation, None, self.mock_event, service_handler, None) + do_rest_call('', None, self.mock_event, service_handler, None) bot_owner_notification = self.get_last_message() self.assertEqual(bot_owner_notification.content, '''[A message](http://zulip.testserver/#narrow/stream/999-Verona/subject/Foo/near/) triggered an outgoing webhook. @@ -88,7 +85,7 @@ The webhook got a response with status code *500*.''') def test_fail_request(self, mock_fail_with_message: mock.Mock) -> None: response = ResponseMock(400) with mock.patch('requests.request', return_value=response): - do_rest_call('', self.rest_operation, None, self.mock_event, service_handler, None) + do_rest_call('', None, self.mock_event, service_handler, None) bot_owner_notification = self.get_last_message() self.assertTrue(mock_fail_with_message.called) self.assertEqual(bot_owner_notification.content, @@ -100,7 +97,7 @@ The webhook got a response with status code *400*.''') def helper(side_effect: Any, error_text: str) -> None: with mock.patch('logging.info'): with mock.patch('requests.request', side_effect=side_effect): - do_rest_call('', self.rest_operation, None, self.mock_event, service_handler, None) + do_rest_call('', None, self.mock_event, service_handler, None) bot_owner_notification = self.get_last_message() self.assertIn(error_text, bot_owner_notification.content) self.assertIn('triggered', bot_owner_notification.content) @@ -114,7 +111,7 @@ The webhook got a response with status code *400*.''') @mock.patch('zerver.lib.outgoing_webhook.fail_with_message') def test_request_exception(self, mock_fail_with_message: mock.Mock, mock_requests_request: mock.Mock, mock_logger: mock.Mock) -> None: - do_rest_call('', self.rest_operation, None, self.mock_event, service_handler, None) + do_rest_call('', None, self.mock_event, service_handler, None) bot_owner_notification = self.get_last_message() self.assertTrue(mock_fail_with_message.called) self.assertEqual(bot_owner_notification.content, diff --git a/zerver/worker/queue_processors.py b/zerver/worker/queue_processors.py index 9e62b94db0..558add8bd9 100644 --- a/zerver/worker/queue_processors.py +++ b/zerver/worker/queue_processors.py @@ -481,10 +481,9 @@ class OutgoingWebhookWorker(QueueProcessingWorker): for service in services: dup_event['service_name'] = str(service.name) service_handler = get_outgoing_webhook_service_handler(service) - rest_operation, request_data = service_handler.process_event(dup_event) - if rest_operation: + request_data = service_handler.build_bot_request(dup_event) + if request_data: do_rest_call(service.base_url, - rest_operation, request_data, dup_event, service_handler)