CodeCow/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2026-06-21 21:32:29 +08:00
Code Issues Actions Packages Projects Releases Wiki Activity

refactor: Extract build_bot_request().

Browse Source 
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.
This commit is contained in:
Steve Howell 2018-10-10 22:45:19 +00:00 committed by Tim Abbott
parent 16eff75e49
commit 69ee84bb14
4 changed files with 22 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
zerver/lib/outgoing_webhook.py
View File

@ -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:

17
zerver/tests/test_outgoing_webhook_interfaces.py
View File

@ -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:

13
zerver/tests/test_outgoing_webhook_system.py
View File

@ -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,

5
zerver/worker/queue_processors.py
View File

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