From a1994122cac9a79df8730a38bd6e07de41807fd9 Mon Sep 17 00:00:00 2001 From: Eeshan Garg Date: Sat, 12 Jan 2019 20:11:51 -0330 Subject: [PATCH] webhooks/pagerduty: Refactor and make minor improvements. This commit improves a couple of things: * All of the message templates are now at the top, a convention we follow in a lot of our webhooks. * Messages are not prefixed with any emojis. We don't do this in any of our other webhooks. Plus, the emojis were outdated. * Remove some superfluous code. * Use ```quote ``` style formatting for quoted text instead of the `>` character. --- zerver/webhooks/pagerduty/tests.py | 44 ++++++------ zerver/webhooks/pagerduty/view.py | 109 +++++++++++++++++------------ 2 files changed, 87 insertions(+), 66 deletions(-) diff --git a/zerver/webhooks/pagerduty/tests.py b/zerver/webhooks/pagerduty/tests.py index 42f60935f8..fc85f9856a 100644 --- a/zerver/webhooks/pagerduty/tests.py +++ b/zerver/webhooks/pagerduty/tests.py @@ -8,48 +8,48 @@ class PagerDutyHookTests(WebhookTestCase): FIXTURE_DIR_NAME = 'pagerduty' def test_trigger(self) -> None: - expected_message = ':imp: Incident [3](https://zulip-test.pagerduty.com/incidents/P140S4Y) triggered by [Test service](https://zulip-test.pagerduty.com/services/PIL5CUQ) and assigned to [armooo@](https://zulip-test.pagerduty.com/users/POBCFRJ)\n\n>foo' - self.send_and_test_stream_message('trigger', u"incident 3", expected_message) + expected_message = 'Incident [3](https://zulip-test.pagerduty.com/incidents/P140S4Y) triggered by [Test service](https://zulip-test.pagerduty.com/services/PIL5CUQ) (assigned to [armooo](https://zulip-test.pagerduty.com/users/POBCFRJ))\n\n``` quote\nfoo\n```' + self.send_and_test_stream_message('trigger', u"Incident 3", expected_message) def test_trigger_v2(self) -> None: - expected_message = ':imp: Incident [33](https://webdemo.pagerduty.com/incidents/PRORDTY) triggered by [Production XDB Cluster](https://webdemo.pagerduty.com/services/PN49J75) and assigned to [Laura Haley@](https://webdemo.pagerduty.com/users/P553OPV)\n\n>My new incident' - self.send_and_test_stream_message('trigger_v2', u'incident 33', expected_message) + expected_message = 'Incident [33](https://webdemo.pagerduty.com/incidents/PRORDTY) triggered by [Production XDB Cluster](https://webdemo.pagerduty.com/services/PN49J75) (assigned to [Laura Haley](https://webdemo.pagerduty.com/users/P553OPV))\n\n``` quote\nMy new incident\n```' + self.send_and_test_stream_message('trigger_v2', u'Incident 33', expected_message) def test_trigger_without_assignee_v2(self) -> None: - expected_message = ':imp: Incident [33](https://webdemo.pagerduty.com/incidents/PRORDTY) triggered by [Production XDB Cluster](https://webdemo.pagerduty.com/services/PN49J75) and assigned to [nobody@]()\n\n>My new incident' - self.send_and_test_stream_message('trigger_without_assignee_v2', u'incident 33', expected_message) + expected_message = 'Incident [33](https://webdemo.pagerduty.com/incidents/PRORDTY) triggered by [Production XDB Cluster](https://webdemo.pagerduty.com/services/PN49J75) (assigned to nobody)\n\n``` quote\nMy new incident\n```' + self.send_and_test_stream_message('trigger_without_assignee_v2', u'Incident 33', expected_message) def test_unacknowledge(self) -> None: - expected_message = ':imp: Incident [3](https://zulip-test.pagerduty.com/incidents/P140S4Y) unacknowledged by [Test service](https://zulip-test.pagerduty.com/services/PIL5CUQ) and assigned to [armooo@](https://zulip-test.pagerduty.com/users/POBCFRJ)\n\n>foo' - self.send_and_test_stream_message('unacknowledge', u"incident 3", expected_message) + expected_message = 'Incident [3](https://zulip-test.pagerduty.com/incidents/P140S4Y) unacknowledged by [Test service](https://zulip-test.pagerduty.com/services/PIL5CUQ) (assigned to [armooo](https://zulip-test.pagerduty.com/users/POBCFRJ))\n\n``` quote\nfoo\n```' + self.send_and_test_stream_message('unacknowledge', u"Incident 3", expected_message) def test_resolved(self) -> None: - expected_message = ':grinning: Incident [1](https://zulip-test.pagerduty.com/incidents/PO1XIJ5) resolved by [armooo@](https://zulip-test.pagerduty.com/users/POBCFRJ)\n\n>It is on fire' - self.send_and_test_stream_message('resolved', u"incident 1", expected_message) + expected_message = 'Incident [1](https://zulip-test.pagerduty.com/incidents/PO1XIJ5) resolved by [armooo](https://zulip-test.pagerduty.com/users/POBCFRJ)\n\n``` quote\nIt is on fire\n```' + self.send_and_test_stream_message('resolved', u"Incident 1", expected_message) def test_resolved_v2(self) -> None: - expected_message = ':grinning: Incident [33](https://webdemo.pagerduty.com/incidents/PRORDTY) resolved by [Laura Haley@](https://webdemo.pagerduty.com/users/P553OPV)\n\n>My new incident' - self.send_and_test_stream_message('resolve_v2', 'incident 33', expected_message) + expected_message = 'Incident [33](https://webdemo.pagerduty.com/incidents/PRORDTY) resolved by [Laura Haley](https://webdemo.pagerduty.com/users/P553OPV)\n\n``` quote\nMy new incident\n```' + self.send_and_test_stream_message('resolve_v2', 'Incident 33', expected_message) def test_auto_resolved(self) -> None: - expected_message = ':grinning: Incident [2](https://zulip-test.pagerduty.com/incidents/PX7K9J2) resolved\n\n>new' - self.send_and_test_stream_message('auto_resolved', u"incident 2", expected_message) + expected_message = 'Incident [2](https://zulip-test.pagerduty.com/incidents/PX7K9J2) resolved\n\n``` quote\nnew\n```' + self.send_and_test_stream_message('auto_resolved', u"Incident 2", expected_message) def test_acknowledge(self) -> None: - expected_message = ':no_good: Incident [1](https://zulip-test.pagerduty.com/incidents/PO1XIJ5) acknowledged by [armooo@](https://zulip-test.pagerduty.com/users/POBCFRJ)\n\n>It is on fire' - self.send_and_test_stream_message('acknowledge', u"incident 1", expected_message) + expected_message = 'Incident [1](https://zulip-test.pagerduty.com/incidents/PO1XIJ5) acknowledged by [armooo](https://zulip-test.pagerduty.com/users/POBCFRJ)\n\n``` quote\nIt is on fire\n```' + self.send_and_test_stream_message('acknowledge', u"Incident 1", expected_message) def test_acknowledge_v2(self) -> None: - expected_message = ':no_good: Incident [33](https://webdemo.pagerduty.com/incidents/PRORDTY) acknowledged by [Laura Haley@](https://webdemo.pagerduty.com/users/P553OPV)\n\n>My new incident' - self.send_and_test_stream_message('acknowledge_v2', 'incident 33', expected_message) + expected_message = 'Incident [33](https://webdemo.pagerduty.com/incidents/PRORDTY) acknowledged by [Laura Haley](https://webdemo.pagerduty.com/users/P553OPV)\n\n``` quote\nMy new incident\n```' + self.send_and_test_stream_message('acknowledge_v2', 'Incident 33', expected_message) def test_incident_assigned_v2(self) -> None: - expected_message = ':no_good: Incident [33](https://webdemo.pagerduty.com/incidents/PRORDTY) assigned to [Wiley Jacobson@](https://webdemo.pagerduty.com/users/PFBSJ2Z)\n\n>My new incident' - self.send_and_test_stream_message('assign_v2', 'incident 33', expected_message) + expected_message = 'Incident [33](https://webdemo.pagerduty.com/incidents/PRORDTY) assigned to [Wiley Jacobson](https://webdemo.pagerduty.com/users/PFBSJ2Z)\n\n``` quote\nMy new incident\n```' + self.send_and_test_stream_message('assign_v2', 'Incident 33', expected_message) def test_no_subject(self) -> None: - expected_message = u':grinning: Incident [48219](https://dropbox.pagerduty.com/incidents/PJKGZF9) resolved\n\n>mp_error_block_down_critical\u2119\u01b4' - self.send_and_test_stream_message('mp_fail', u"incident 48219", expected_message) + expected_message = u'Incident [48219](https://dropbox.pagerduty.com/incidents/PJKGZF9) resolved\n\n``` quote\nmp_error_block_down_critical\u2119\u01b4\n```' + self.send_and_test_stream_message('mp_fail', u"Incident 48219", expected_message) def test_bad_message(self) -> None: expected_message = 'Unknown pagerduty message\n```\n{\n "type":"incident.triggered"\n}\n```' diff --git a/zerver/webhooks/pagerduty/view.py b/zerver/webhooks/pagerduty/view.py index c678bf08e4..c6d6dfdb7c 100644 --- a/zerver/webhooks/pagerduty/view.py +++ b/zerver/webhooks/pagerduty/view.py @@ -30,10 +30,46 @@ PAGER_DUTY_EVENT_NAMES_V2 = { 'incident.assign': 'assigned', } -def build_pagerduty_formatdict(message: Dict[str, Any]) -> Dict[str, Any]: - # Normalize the message dict, after this all keys will exist. I would - # rather some strange looking messages than dropping pages. +ASSIGNEE_TEMPLATE = '[{username}]({url})' +INCIDENT_WITH_SERVICE_AND_ASSIGNEE = ( + 'Incident [{incident_num}]({incident_url}) {action} by [{service_name}]' + '({service_url}) (assigned to {assignee_info})\n\n``` quote\n{trigger_message}\n```' +) + +INCIDENT_WITH_ASSIGNEE = """ +Incident [{incident_num}]({incident_url}) {action} by {assignee_info} + +``` quote +{trigger_message} +``` +""".strip() + +INCIDENT_ASSIGNED = """ +Incident [{incident_num}]({incident_url}) {action} to {assignee_info} + +``` quote +{trigger_message} +``` +""".strip() + +INCIDENT_RESOLVED_WITH_AGENT = """ +Incident [{incident_num}]({incident_url}) resolved by {resolving_agent_info} + +``` quote +{trigger_message} +``` +""".strip() + +INCIDENT_RESOLVED = """ +Incident [{incident_num}]({incident_url}) resolved + +``` quote +{trigger_message} +``` +""".strip() + +def build_pagerduty_formatdict(message: Dict[str, Any]) -> Dict[str, Any]: format_dict = {} # type: Dict[str, Any] format_dict['action'] = PAGER_DUTY_EVENT_NAMES[message['type']] @@ -44,27 +80,21 @@ def build_pagerduty_formatdict(message: Dict[str, Any]) -> Dict[str, Any]: format_dict['service_name'] = message['data']['incident']['service']['name'] format_dict['service_url'] = message['data']['incident']['service']['html_url'] - # This key can be missing on null if message['data']['incident'].get('assigned_to_user', None): assigned_to_user = message['data']['incident']['assigned_to_user'] - format_dict['assigned_to_email'] = assigned_to_user['email'] - format_dict['assigned_to_username'] = assigned_to_user['email'].split('@')[0] - format_dict['assigned_to_url'] = assigned_to_user['html_url'] + format_dict['assignee_info'] = ASSIGNEE_TEMPLATE.format( + username=assigned_to_user['email'].split('@')[0], + url=assigned_to_user['html_url'], + ) else: - format_dict['assigned_to_email'] = 'nobody' - format_dict['assigned_to_username'] = 'nobody' - format_dict['assigned_to_url'] = '' + format_dict['assignee_info'] = 'nobody' - # This key can be missing on null if message['data']['incident'].get('resolved_by_user', None): resolved_by_user = message['data']['incident']['resolved_by_user'] - format_dict['resolved_by_email'] = resolved_by_user['email'] - format_dict['resolved_by_username'] = resolved_by_user['email'].split('@')[0] - format_dict['resolved_by_url'] = resolved_by_user['html_url'] - else: - format_dict['resolved_by_email'] = 'nobody' - format_dict['resolved_by_username'] = 'nobody' - format_dict['resolved_by_url'] = '' + format_dict['resolving_agent_info'] = ASSIGNEE_TEMPLATE.format( + username=resolved_by_user['email'].split('@')[0], + url=resolved_by_user['html_url'], + ) trigger_message = [] trigger_subject = message['data']['incident']['trigger_summary_data'].get('subject', '') @@ -90,16 +120,17 @@ def build_pagerduty_formatdict_v2(message: Dict[str, Any]) -> Dict[str, Any]: assignments = message['incident']['assignments'] if assignments: assignee = assignments[0]['assignee'] - format_dict['assigned_to_username'] = assignee['summary'] - format_dict['assigned_to_url'] = assignee['html_url'] + format_dict['assignee_info'] = ASSIGNEE_TEMPLATE.format( + username=assignee['summary'], url=assignee['html_url']) else: - format_dict['assigned_to_username'] = 'nobody' - format_dict['assigned_to_url'] = '' + format_dict['assignee_info'] = 'nobody' last_status_change_by = message['incident'].get('last_status_change_by') if last_status_change_by is not None: - format_dict['resolved_by_username'] = last_status_change_by['summary'] - format_dict['resolved_by_url'] = last_status_change_by['html_url'] + format_dict['resolving_agent_info'] = ASSIGNEE_TEMPLATE.format( + username=last_status_change_by['summary'], + url=last_status_change_by['html_url'], + ) trigger_description = message['incident'].get('description') if trigger_description is not None: @@ -123,28 +154,18 @@ def send_formated_pagerduty(request: HttpRequest, message_type: str, format_dict: Dict[str, Any]) -> None: if message_type in ('incident.trigger', 'incident.unacknowledge'): - template = (u':imp: Incident ' - u'[{incident_num}]({incident_url}) {action} by ' - u'[{service_name}]({service_url}) and assigned to ' - u'[{assigned_to_username}@]({assigned_to_url})\n\n>{trigger_message}') - - elif message_type == 'incident.resolve' and format_dict['resolved_by_url']: - template = (u':grinning: Incident ' - u'[{incident_num}]({incident_url}) resolved by ' - u'[{resolved_by_username}@]({resolved_by_url})\n\n>{trigger_message}') - elif message_type == 'incident.resolve' and not format_dict['resolved_by_url']: - template = (u':grinning: Incident ' - u'[{incident_num}]({incident_url}) resolved\n\n>{trigger_message}') + template = INCIDENT_WITH_SERVICE_AND_ASSIGNEE + elif message_type == 'incident.resolve' and format_dict.get('resolving_agent_info') is not None: + template = INCIDENT_RESOLVED_WITH_AGENT + elif message_type == 'incident.resolve' and format_dict.get('resolving_agent_info') is None: + template = INCIDENT_RESOLVED elif message_type == 'incident.assign': - template = (u':no_good: Incident [{incident_num}]({incident_url}) ' - u'{action} to [{assigned_to_username}@]({assigned_to_url})\n\n>{trigger_message}') + template = INCIDENT_ASSIGNED else: - template = (u':no_good: Incident [{incident_num}]({incident_url}) ' - u'{action} by [{assigned_to_username}@]({assigned_to_url})\n\n>{trigger_message}') + template = INCIDENT_WITH_ASSIGNEE - subject = u'incident {incident_num}'.format(**format_dict) + subject = u'Incident {incident_num}'.format(**format_dict) body = template.format(**format_dict) - check_send_webhook_message(request, user_profile, subject, body) @@ -160,7 +181,7 @@ def api_pagerduty_webhook( # If the message has no "type" key, then this payload came from a # Pagerduty Webhook V2. if message_type is None: - continue + break if message_type not in PAGER_DUTY_EVENT_NAMES: send_raw_pagerduty_json(request, user_profile, message) @@ -178,7 +199,7 @@ def api_pagerduty_webhook( # If the message has no "event" key, then this payload came from a # Pagerduty Webhook V1. if event is None: - continue + break if event not in PAGER_DUTY_EVENT_NAMES_V2: raise UnexpectedWebhookEventType('Pagerduty', event)