From 84a4de32cc7681cb593ca789249d7a2e048104c9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C4=93teris=20Caune?= <cuu508@gmail.com>
Date: Fri, 27 Dec 2019 15:07:15 +0200
Subject: [PATCH] Remove legacy webhook formats (newline-separated fields and
 the post_data key) from the Channel model

---
 hc/api/models.py                   |  22 -----
 hc/api/tests/test_channel_model.py | 110 -----------------------
 hc/api/tests/test_notify.py        | 137 ++++++++++++++++++-----------
 hc/front/tests/test_channels.py    |  13 ++-
 hc/front/tests/test_log.py         |   9 +-
 5 files changed, 108 insertions(+), 183 deletions(-)

diff --git a/hc/api/models.py b/hc/api/models.py
index c09ab233..2a50a218 100644
--- a/hc/api/models.py
+++ b/hc/api/models.py
@@ -460,29 +460,7 @@ class Channel(models.Model):
     def webhook_spec(self, status):
         assert self.kind == "webhook"
 
-        if not self.value.startswith("{"):
-            parts = self.value.split("\n")
-            url_down = parts[0]
-            url_up = parts[1] if len(parts) > 1 else ""
-            post_data = parts[2] if len(parts) > 2 else ""
-
-            return {
-                "method": "POST" if post_data else "GET",
-                "url": url_down if status == "down" else url_up,
-                "body": post_data,
-                "headers": {},
-            }
-
         doc = json.loads(self.value)
-        if "post_data" in doc:
-            # Legacy "post_data" in doc -- use the legacy fields
-            return {
-                "method": "POST" if doc["post_data"] else "GET",
-                "url": doc["url_down"] if status == "down" else doc["url_up"],
-                "body": doc["post_data"],
-                "headers": doc["headers"],
-            }
-
         if status == "down" and "method_down" in doc:
             return {
                 "method": doc["method_down"],
diff --git a/hc/api/tests/test_channel_model.py b/hc/api/tests/test_channel_model.py
index 5c8e446a..9c106de7 100644
--- a/hc/api/tests/test_channel_model.py
+++ b/hc/api/tests/test_channel_model.py
@@ -5,116 +5,6 @@ from hc.test import BaseTestCase
 
 
 class ChannelModelTestCase(BaseTestCase):
-    def test_webhook_spec_handles_plain_single_address(self):
-        c = Channel(kind="webhook")
-        c.value = "http://example.org"
-        self.assertEqual(
-            c.down_webhook_spec,
-            {"method": "GET", "url": "http://example.org", "body": "", "headers": {}},
-        )
-
-        self.assertEqual(
-            c.up_webhook_spec, {"method": "GET", "url": "", "body": "", "headers": {}}
-        )
-
-    def test_webhook_spec_handles_plain_pair(self):
-        c = Channel(kind="webhook")
-        c.value = "http://example.org\nhttp://example.com/up/"
-        self.assertEqual(
-            c.down_webhook_spec,
-            {"method": "GET", "url": "http://example.org", "body": "", "headers": {}},
-        )
-
-        self.assertEqual(
-            c.up_webhook_spec,
-            {
-                "method": "GET",
-                "url": "http://example.com/up/",
-                "body": "",
-                "headers": {},
-            },
-        )
-
-    def test_webhook_spec_handles_plain_post(self):
-        c = Channel(kind="webhook")
-        c.value = "http://example.org\n\nhello world"
-        self.assertEqual(
-            c.down_webhook_spec,
-            {
-                "method": "POST",
-                "url": "http://example.org",
-                "body": "hello world",
-                "headers": {},
-            },
-        )
-
-        self.assertEqual(
-            c.up_webhook_spec,
-            {"method": "POST", "url": "", "body": "hello world", "headers": {}},
-        )
-
-    def test_webhook_spec_handles_legacy_get(self):
-        c = Channel(kind="webhook")
-        c.value = json.dumps(
-            {
-                "url_down": "http://example.org",
-                "url_up": "http://example.org/up/",
-                "headers": {"X-Name": "value"},
-                "post_data": "",
-            }
-        )
-
-        self.assertEqual(
-            c.down_webhook_spec,
-            {
-                "method": "GET",
-                "url": "http://example.org",
-                "body": "",
-                "headers": {"X-Name": "value"},
-            },
-        )
-
-        self.assertEqual(
-            c.up_webhook_spec,
-            {
-                "method": "GET",
-                "url": "http://example.org/up/",
-                "body": "",
-                "headers": {"X-Name": "value"},
-            },
-        )
-
-    def test_webhook_spec_handles_legacy_post(self):
-        c = Channel(kind="webhook")
-        c.value = json.dumps(
-            {
-                "url_down": "http://example.org",
-                "url_up": "http://example.org/up/",
-                "headers": {"X-Name": "value"},
-                "post_data": "hello world",
-            }
-        )
-
-        self.assertEqual(
-            c.down_webhook_spec,
-            {
-                "method": "POST",
-                "url": "http://example.org",
-                "body": "hello world",
-                "headers": {"X-Name": "value"},
-            },
-        )
-
-        self.assertEqual(
-            c.up_webhook_spec,
-            {
-                "method": "POST",
-                "url": "http://example.org/up/",
-                "body": "hello world",
-                "headers": {"X-Name": "value"},
-            },
-        )
-
     def test_webhook_spec_handles_mixed(self):
         c = Channel(kind="webhook")
         c.value = json.dumps(
diff --git a/hc/api/tests/test_notify.py b/hc/api/tests/test_notify.py
index 99cf092d..0d574941 100644
--- a/hc/api/tests/test_notify.py
+++ b/hc/api/tests/test_notify.py
@@ -28,7 +28,14 @@ class NotifyTestCase(BaseTestCase):
 
     @patch("hc.api.transports.requests.request")
     def test_webhook(self, mock_get):
-        self._setup_data("webhook", "http://example")
+        definition = {
+            "method_down": "GET",
+            "url_down": "http://example",
+            "body_down": "",
+            "headers_down": {},
+        }
+
+        self._setup_data("webhook", json.dumps(definition))
         mock_get.return_value.status_code = 200
 
         self.channel.notify(self.check)
@@ -41,7 +48,14 @@ class NotifyTestCase(BaseTestCase):
 
     @patch("hc.api.transports.requests.request", side_effect=Timeout)
     def test_webhooks_handle_timeouts(self, mock_get):
-        self._setup_data("webhook", "http://example")
+        definition = {
+            "method_down": "GET",
+            "url_down": "http://example",
+            "body_down": "",
+            "headers_down": {},
+        }
+
+        self._setup_data("webhook", json.dumps(definition))
         self.channel.notify(self.check)
 
         n = Notification.objects.get()
@@ -49,23 +63,28 @@ class NotifyTestCase(BaseTestCase):
 
     @patch("hc.api.transports.requests.request", side_effect=ConnectionError)
     def test_webhooks_handle_connection_errors(self, mock_get):
-        self._setup_data("webhook", "http://example")
+        definition = {
+            "method_down": "GET",
+            "url_down": "http://example",
+            "body_down": "",
+            "headers_down": {},
+        }
+        self._setup_data("webhook", json.dumps(definition))
         self.channel.notify(self.check)
 
         n = Notification.objects.get()
         self.assertEqual(n.error, "Connection failed")
 
-    @patch("hc.api.transports.requests.request")
-    def test_webhooks_ignore_up_events(self, mock_get):
-        self._setup_data("webhook", "http://example", status="up")
-        self.channel.notify(self.check)
-
-        self.assertFalse(mock_get.called)
-        self.assertEqual(Notification.objects.count(), 0)
-
     @patch("hc.api.transports.requests.request")
     def test_webhooks_handle_500(self, mock_get):
-        self._setup_data("webhook", "http://example")
+        definition = {
+            "method_down": "GET",
+            "url_down": "http://example",
+            "body_down": "",
+            "headers_down": {},
+        }
+
+        self._setup_data("webhook", json.dumps(definition))
         mock_get.return_value.status_code = 500
 
         self.channel.notify(self.check)
@@ -75,8 +94,14 @@ class NotifyTestCase(BaseTestCase):
 
     @patch("hc.api.transports.requests.request")
     def test_webhooks_support_variables(self, mock_get):
-        template = "http://host/$CODE/$STATUS/$TAG1/$TAG2/?name=$NAME"
-        self._setup_data("webhook", template)
+        definition = {
+            "method_down": "GET",
+            "url_down": "http://host/$CODE/$STATUS/$TAG1/$TAG2/?name=$NAME",
+            "body_down": "",
+            "headers_down": {},
+        }
+
+        self._setup_data("webhook", json.dumps(definition))
         self.check.name = "Hello World"
         self.check.tags = "foo bar"
         self.check.save()
@@ -93,7 +118,14 @@ class NotifyTestCase(BaseTestCase):
 
     @patch("hc.api.transports.requests.request")
     def test_webhooks_handle_variable_variables(self, mock_get):
-        self._setup_data("webhook", "http://host/$$NAMETAG1")
+        definition = {
+            "method_down": "GET",
+            "url_down": "http://host/$$NAMETAG1",
+            "body_down": "",
+            "headers_down": {},
+        }
+
+        self._setup_data("webhook", json.dumps(definition))
         self.check.tags = "foo bar"
         self.check.save()
 
@@ -105,8 +137,14 @@ class NotifyTestCase(BaseTestCase):
 
     @patch("hc.api.transports.requests.request")
     def test_webhooks_support_post(self, mock_request):
-        template = "http://example.com\n\nThe Time Is $NOW"
-        self._setup_data("webhook", template)
+        definition = {
+            "method_down": "POST",
+            "url_down": "http://example.com",
+            "body_down": "The Time Is $NOW",
+            "headers_down": {},
+        }
+
+        self._setup_data("webhook", json.dumps(definition))
         self.check.save()
 
         self.channel.notify(self.check)
@@ -122,9 +160,14 @@ class NotifyTestCase(BaseTestCase):
     def test_webhooks_dollarsign_escaping(self, mock_get):
         # If name or tag contains what looks like a variable reference,
         # that should be left alone:
+        definition = {
+            "method_down": "GET",
+            "url_down": "http://host/$NAME",
+            "body_down": "",
+            "headers_down": {},
+        }
 
-        template = "http://host/$NAME"
-        self._setup_data("webhook", template)
+        self._setup_data("webhook", json.dumps(definition))
         self.check.name = "$TAG1"
         self.check.tags = "foo"
         self.check.save()
@@ -137,8 +180,14 @@ class NotifyTestCase(BaseTestCase):
         )
 
     @patch("hc.api.transports.requests.request")
-    def test_webhook_fires_on_up_event(self, mock_get):
-        self._setup_data("webhook", "http://foo\nhttp://bar", status="up")
+    def test_webhooks_handle_up_events(self, mock_get):
+        definition = {
+            "method_up": "GET",
+            "url_up": "http://bar",
+            "body_up": "",
+            "headers_up": {},
+        }
+        self._setup_data("webhook", json.dumps(definition), status="up")
 
         self.channel.notify(self.check)
 
@@ -147,47 +196,37 @@ class NotifyTestCase(BaseTestCase):
         )
 
     @patch("hc.api.transports.requests.request")
-    def test_webhooks_handle_unicode_post_body(self, mock_request):
-        template = "http://example.com\n\n(╯°□°）╯︵ ┻━┻"
-        self._setup_data("webhook", template)
-        self.check.save()
+    def test_webhooks_handle_noop_up_events(self, mock_get):
+        definition = {
+            "method_up": "GET",
+            "url_up": "",
+            "body_up": "",
+            "headers_up": {},
+        }
 
+        self._setup_data("webhook", json.dumps(definition), status="up")
         self.channel.notify(self.check)
-        args, kwargs = mock_request.call_args
 
-        # unicode should be encoded into utf-8
-        self.assertIsInstance(kwargs["data"], bytes)
+        self.assertFalse(mock_get.called)
+        self.assertEqual(Notification.objects.count(), 0)
 
     @patch("hc.api.transports.requests.request")
-    def test_webhooks_handle_json_value(self, mock_request):
+    def test_webhooks_handle_unicode_post_body(self, mock_request):
         definition = {
-            "method_down": "GET",
+            "method_down": "POST",
             "url_down": "http://foo.com",
-            "body_down": "",
+            "body_down": "(╯°□°）╯︵ ┻━┻",
             "headers_down": {},
         }
-        self._setup_data("webhook", json.dumps(definition))
-        self.channel.notify(self.check)
-
-        headers = {"User-Agent": "healthchecks.io"}
-        mock_request.assert_called_with(
-            "get", "http://foo.com", headers=headers, timeout=5
-        )
 
-    @patch("hc.api.transports.requests.request")
-    def test_webhooks_handle_json_up_event(self, mock_request):
-        definition = {
-            "method_up": "GET",
-            "url_up": "http://bar",
-            "body_up": "",
-            "headers_up": {},
-        }
+        self._setup_data("webhook", json.dumps(definition))
+        self.check.save()
 
-        self._setup_data("webhook", json.dumps(definition), status="up")
         self.channel.notify(self.check)
+        args, kwargs = mock_request.call_args
 
-        headers = {"User-Agent": "healthchecks.io"}
-        mock_request.assert_called_with("get", "http://bar", headers=headers, timeout=5)
+        # unicode should be encoded into utf-8
+        self.assertIsInstance(kwargs["data"], bytes)
 
     @patch("hc.api.transports.requests.request")
     def test_webhooks_handle_post_headers(self, mock_request):
diff --git a/hc/front/tests/test_channels.py b/hc/front/tests/test_channels.py
index 5f0dd7a3..86ab1ec5 100644
--- a/hc/front/tests/test_channels.py
+++ b/hc/front/tests/test_channels.py
@@ -23,7 +23,18 @@ class ChannelsTestCase(BaseTestCase):
 
     def test_it_shows_webhook_post_data(self):
         ch = Channel(kind="webhook", project=self.project)
-        ch.value = "http://down.example.com\nhttp://up.example.com\nfoobar"
+        ch.value = json.dumps(
+            {
+                "method_down": "POST",
+                "url_down": "http://down.example.com",
+                "body_down": "foobar",
+                "headers_down": {},
+                "method_up": "GET",
+                "url_up": "http://up.example.com",
+                "body_up": "",
+                "headers_up": {},
+            }
+        )
         ch.save()
 
         self.client.login(username="alice@example.org", password="password")
diff --git a/hc/front/tests/test_log.py b/hc/front/tests/test_log.py
index 76e47a90..3c4eb1d6 100644
--- a/hc/front/tests/test_log.py
+++ b/hc/front/tests/test_log.py
@@ -74,7 +74,14 @@ class LogTestCase(BaseTestCase):
 
     def test_it_shows_webhook_notification(self):
         ch = Channel(kind="webhook", project=self.project)
-        ch.value = "foo/$NAME"
+        ch.value = json.dumps(
+            {
+                "method_down": "GET",
+                "url_down": "foo/$NAME",
+                "body_down": "",
+                "headers_down": {},
+            }
+        )
         ch.save()
 
         Notification(owner=self.check, channel=ch, check_status="down").save()