newrelic: Added owner field and cleaned up code.

I reformatted the tests and view to include information about who
acknowledged and closed the alert. Only includes the information about
the owner if there was an owner.

Made a few small changes to the refactored bit as requested in review.

zerver/lib/webhooks/common.py

@@ -14,6 +14,7 @@ from zerver.lib.actions import (
 from zerver.lib.exceptions import ErrorCode, JsonableError, StreamDoesNotExistError
 from zerver.lib.request import REQ, has_request_variables
 from zerver.lib.send_email import FromAddress
+from zerver.lib.timestamp import timestamp_to_datetime
 from zerver.models import UserProfile
 
 MISSING_EVENT_HEADER_MESSAGE = """
@@ -167,8 +168,7 @@ def unix_milliseconds_to_timestamp(milliseconds: Any, webhook: str) -> datetime:
     Returns a datetime representing the time."""
     try:
         # timestamps are in milliseconds so divide by 1000
-        return datetime.fromtimestamp((milliseconds) / 1000)
-    except ValueError:
-        raise JsonableError(_("The {} webhook expects time in milleseconds.").format(webhook))
-    except TypeError:
+        seconds = milliseconds / 1000
+        return timestamp_to_datetime(seconds)
+    except (ValueError, TypeError):
         raise JsonableError(_("The {} webhook expects time in milleseconds.").format(webhook))

zerver/webhooks/newrelic/fixtures/incident_acknowledged.json

@@ -6,5 +6,6 @@
     "condition_name": "Server Down",
     "timestamp": 1605133931151,
     "current_state": "acknowledged",
+    "owner": "Alice",
     "incident_url": "https://alerts.newrelic.com/accounts/2941966/incidents/1234"
 }

zerver/webhooks/newrelic/fixtures/incident_closed.json

@@ -6,5 +6,6 @@
     "condition_name": "Server Down",
     "timestamp": 1605133931151,
     "current_state": "closed",
+    "owner": "",
     "incident_url": "https://alerts.newrelic.com/accounts/2941966/incidents/1234"
 }

zerver/webhooks/newrelic/fixtures/incident_malformatted_time.json

@@ -6,5 +6,6 @@
     "condition_name": "Server Down",
     "timestamp": "1969-12-31 23:59:55",
     "current_state": "open",
+    "owner": "",
     "incident_url": "https://alerts.newrelic.com/accounts/2941966/incidents/1234"
 }

zerver/webhooks/newrelic/fixtures/incident_missing_current_state.json

@@ -5,5 +5,6 @@
     "policy_name": "Test policy name",
     "condition_name": "Server Down",
     "timestamp": 1605133931151,
+    "owner": "Alice",
     "incident_url": "https://alerts.newrelic.com/accounts/2941966/incidents/1234"
 }

zerver/webhooks/newrelic/fixtures/incident_missing_timestamp.json

@@ -5,5 +5,6 @@
     "policy_name": "Test policy name",
     "condition_name": "Server Down",
     "current_state": "open",
+    "owner": "",
     "incident_url": "https://alerts.newrelic.com/accounts/2941966/incidents/1234"
 }

zerver/webhooks/newrelic/fixtures/incident_opened.json

@@ -6,5 +6,6 @@
     "condition_name": "Server Down",
     "timestamp": 1605133931151,
     "current_state": "open",
+    "owner": "",
     "incident_url": "https://alerts.newrelic.com/accounts/2941966/incidents/1234"
 }

zerver/webhooks/newrelic/fixtures/incident_state_not_recognized.json

@@ -6,5 +6,6 @@
     "condition_name": "Server Down",
     "timestamp": 1605133931151,
     "current_state": "hello world",
+    "owner": "Alice",
     "incident_url": "https://alerts.newrelic.com/accounts/2941966/incidents/1234"
 }

zerver/webhooks/newrelic/fixtures/incident_time_too_large.json

@@ -6,5 +6,6 @@
     "condition_name": "Server Down",
     "timestamp": 160513393115100000,
     "current_state": "open",
+    "owner": "",
     "incident_url": "https://alerts.newrelic.com/accounts/2941966/incidents/1234"
 }

zerver/webhooks/newrelic/tests.py

@@ -9,7 +9,7 @@ class NewRelicHookTests(WebhookTestCase):
     def test_open(self) -> None:
         expected_topic = "Test policy name (1234)"
         expected_message = """
-[Incident](https://alerts.newrelic.com/accounts/2941966/incidents/1234) **opened** for condition: **Server Down** at <time:2020-11-11 22:32:11.151000>
+[Incident](https://alerts.newrelic.com/accounts/2941966/incidents/1234) **opened** for condition: **Server Down** at <time:2020-11-11 22:32:11.151000+00:00>
 ``` quote
 Violation description test.
 ```
@@ -38,7 +38,7 @@ Violation description test.
     def test_acknowledged(self) -> None:
         expected_topic = "Test policy name (1234)"
         expected_message = """
-[Incident](https://alerts.newrelic.com/accounts/2941966/incidents/1234) **acknowledged** for condition: **Server Down**
+[Incident](https://alerts.newrelic.com/accounts/2941966/incidents/1234) **acknowledged** by **Alice** for condition: **Server Down**
 """.strip()
 
         self.check_webhook(
@@ -61,7 +61,7 @@ Violation description test.
     def test_missing_fields(self) -> None:
         expected_topic = "Unknown Policy (Unknown ID)"
         expected_message = """
-[Incident](https://alerts.newrelic.com) **opened** for condition: **Unknown condition** at <time:2020-11-11 22:32:11.151000>
+[Incident](https://alerts.newrelic.com) **opened** for condition: **Unknown condition** at <time:2020-11-11 22:32:11.151000+00:00>
 ``` quote
 No details.
 ```

zerver/webhooks/newrelic/view.py

@@ -17,7 +17,7 @@ OPEN_TEMPLATE = """
 ```
 """.strip()
 
-DEFAULT_TEMPLATE = """[Incident]({incident_url}) **{status}** for condition: **{condition_name}**""".strip()
+DEFAULT_TEMPLATE = """[Incident]({incident_url}) **{status}** {owner}for condition: **{condition_name}**""".strip()
 
 TOPIC_TEMPLATE = """{policy_name} ({incident_id})""".strip()
 
@@ -36,6 +36,7 @@ def api_newrelic_webhook(
         "incident_acknowledge_url": payload.get('incident_acknowledge_url', 'https://alerts.newrelic.com'),
         "status": payload.get('current_state', 'None'),
         "iso_timestamp": '',
+        "owner": payload.get('owner', ''),
     }
 
     unix_time = payload.get('timestamp', None)
@@ -44,6 +45,10 @@ def api_newrelic_webhook(
 
     info['iso_timestamp'] = unix_milliseconds_to_timestamp(unix_time, "newrelic")
 
+    # Add formatting to the owner field if owner is present
+    if info['owner'] != '':
+        info['owner'] = 'by **{}** '.format(info['owner'])
+
     # These are the three promised current_state values
     if 'open' in info['status']:
         content = OPEN_TEMPLATE.format(**info)