Merge lp:~wgrant/launchpad/webhook-event-types into lp:launchpad

[William Grant]

Improve Webhook.event_types widget and value checking.

Improve Webhook.event_types widget and value checking.

Rather than a weird Zopey text list widget, there's now a set of checkboxes for each known event type. I've eschewed a normal enum due to the non-DB-column and potentially fluid nature of event types; a custom vocabulary is easy and makes migration easier when we change things later.

=== modified file 'lib/lp/services/webhooks/browser.py'
--- lib/lp/services/webhooks/browser.py	2015-08-04 13:18:24 +0000
+++ lib/lp/services/webhooks/browser.py	2015-08-10 06:17:10 +0000
@@ -16,9 +16,11 @@
 
 from lp.app.browser.launchpadform import (
     action,
+    custom_widget,
     LaunchpadEditFormView,
     LaunchpadFormView,
     )
+from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
     canonical_url,
@@ -103,8 +105,7 @@
 
 
 class WebhookEditSchema(Interface):
-    # XXX wgrant 2015-08-04: Need custom widgets for secret and
-    # event_types.
+    # XXX wgrant 2015-08-04: Need custom widget for secret.
     use_template(IWebhook, include=['delivery_url', 'event_types', 'active'])
 
 
@@ -113,6 +114,7 @@
     page_title = label = "Add webhook"
 
     schema = WebhookEditSchema
+    custom_widget('event_types', LabeledMultiCheckBoxWidget)
 
     @property
     def inside_breadcrumb(self):
@@ -136,10 +138,11 @@
 
 class WebhookView(LaunchpadEditFormView):
 
-    schema = WebhookEditSchema
-
     label = "Manage webhook"
 
+    schema = WebhookEditSchema
+    custom_widget('event_types', LabeledMultiCheckBoxWidget)
+
     @property
     def next_url(self):
         # The edit form is the default view, so the URL doesn't need the
 
=== modified file 'lib/lp/services/webhooks/configure.zcml'
--- lib/lp/services/webhooks/configure.zcml	2015-08-04 11:02:02 +0000
+++ lib/lp/services/webhooks/configure.zcml	2015-08-10 06:17:10 +0000
@@ -21,6 +21,15 @@
         provides="lp.services.webhooks.interfaces.IWebhookSource">
         <allow interface="lp.services.webhooks.interfaces.IWebhookSource"/>
     </securedutility>
+    <securedutility
+        name="WebhookEventType"
+        component="lp.services.webhooks.interfaces.WebhookEventTypeVocabulary"
+        provides="zope.schema.interfaces.IVocabularyFactory">
+        <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
+    </securedutility>
+    <class class="lp.services.webhooks.interfaces.WebhookEventTypeVocabulary">
+        <allow interface="zope.schema.interfaces.IVocabularyTokenized"/>
+    </class>
 
     <securedutility
         component="lp.services.webhooks.model.WebhookJob"
 
=== modified file 'lib/lp/services/webhooks/interfaces.py'
--- lib/lp/services/webhooks/interfaces.py	2015-08-04 13:59:25 +0000
+++ lib/lp/services/webhooks/interfaces.py	2015-08-10 06:17:10 +0000
@@ -17,6 +17,7 @@
     'WebhookDeliveryFailure',
     'WebhookDeliveryRetry',
     'WebhookFeatureDisabled',
+    'WebhookEventTypeVocabulary',
     ]
 
 import httplib
@@ -45,12 +46,14 @@
     )
 from zope.schema import (
     Bool,
+    Choice,
     Datetime,
     Dict,
     Int,
     List,
     TextLine,
     )
+from zope.schema.vocabulary import SimpleVocabulary
 
 from lp import _
 from lp.registry.interfaces.person import IPerson
@@ -67,6 +70,11 @@
     )
 
 
+WEBHOOK_EVENT_TYPES = {
+    "git:push:0.1": "Git push",
+    }
+
+
 @error_status(httplib.UNAUTHORIZED)
 class WebhookFeatureDisabled(Exception):
     """Only certain users can create new Git repositories."""
@@ -86,6 +94,15 @@
     pass
 
 
+class WebhookEventTypeVocabulary(SimpleVocabulary):
+
+    def __init__(self, context):
+        terms = [
+            self.createTerm(key, key, value)
+            for key, value in WEBHOOK_EVENT_TYPES.iteritems()]
+        super(WebhookEventTypeVocabulary, self).__init__(terms)
+
+
 class IWebhook(Interface):
 
     export_as_webservice_entry(as_of='beta')
@@ -97,9 +114,7 @@
         required=True, readonly=True,
         description=_("The object for which this webhook receives events.")))
     event_types = exported(List(
-        TextLine(), title=_("Event types"),
-        description=_(
-            "The event types for which this webhook receives events."),
+        Choice(vocabulary='WebhookEventType'), title=_("Event types"),
         required=True, readonly=False))
     registrant = exported(Reference(
         title=_("Registrant"), schema=IPerson, required=True, readonly=True,
 
=== modified file 'lib/lp/services/webhooks/model.py'
--- lib/lp/services/webhooks/model.py	2015-08-04 13:59:25 +0000
+++ lib/lp/services/webhooks/model.py	2015-08-10 06:17:10 +0000
@@ -140,6 +140,8 @@
     @event_types.setter
     def event_types(self, event_types):
         updated_data = self.json_data or {}
+        # The correctness of the values is also checked by zope.schema,
+        # but best to be safe.
         assert isinstance(event_types, (list, tuple))
         assert all(isinstance(v, basestring) for v in event_types)
         updated_data['event_types'] = event_types
 
=== modified file 'lib/lp/services/webhooks/tests/test_browser.py'
--- lib/lp/services/webhooks/tests/test_browser.py	2015-08-04 13:18:24 +0000
+++ lib/lp/services/webhooks/tests/test_browser.py	2015-08-10 06:17:10 +0000
@@ -149,7 +149,8 @@
             "+new-webhook", method="POST",
             form={
                 "field.delivery_url": "http://example.com/test",
-                "field.active": "on", "field.event_types.count": "0",
+                "field.active": "on", "field.event_types-empty-marker": "1",
+                "field.event_types": "git:push:0.1",
                 "field.actions.new": "Add webhook"})
         self.assertEqual([], view.errors)
         hook = self.target.webhooks.one()
@@ -160,7 +161,7 @@
                 registrant=self.owner,
                 delivery_url="http://example.com/test",
                 active=True,
-                event_types=[]))
+                event_types=["git:push:0.1"]))
 
     def test_rejects_bad_scheme(self):
         transaction.commit()
@@ -168,7 +169,7 @@
             "+new-webhook", method="POST",
             form={
                 "field.delivery_url": "ftp://example.com/test",
-                "field.active": "on", "field.event_types.count": "0",
+                "field.active": "on", "field.event_types-empty-marker": "1",
                 "field.actions.new": "Add webhook"})
         self.assertEqual(
             ['delivery_url'], [error.field_name for error in view.errors])
@@ -220,7 +221,7 @@
             "+index", method="POST",
             form={
                 "field.delivery_url": "http://example.com/edited",
-                "field.active": "off", "field.event_types.count": "0",
+                "field.active": "off", "field.event_types-empty-marker": "1",
                 "field.actions.save": "Save webhook"})
         self.assertEqual([], view.errors)
         self.assertThat(
@@ -236,7 +237,7 @@
             "+index", method="POST",
             form={
                 "field.delivery_url": "ftp://example.com/edited",
-                "field.active": "off", "field.event_types.count": "0",
+                "field.active": "off", "field.event_types-empty-marker": "1",
                 "field.actions.save": "Save webhook"})
         self.assertEqual(
             ['delivery_url'], [error.field_name for error in view.errors])
 
=== modified file 'lib/lp/services/webhooks/tests/test_model.py'
--- lib/lp/services/webhooks/tests/test_model.py	2015-08-04 05:46:54 +0000
+++ lib/lp/services/webhooks/tests/test_model.py	2015-08-10 06:17:10 +0000
@@ -22,6 +22,7 @@
 from lp.testing import (
     admin_logged_in,
     anonymous_logged_in,
+    ExpectedException,
     login_person,
     person_logged_in,
     StormStatementRecorder,
@@ -49,6 +50,24 @@
                 webhook.date_last_modified,
                 GreaterThan(old_mtime))
 
+    def test_event_types(self):
+        # Webhook.event_types is a list of event type strings.
+        webhook = self.factory.makeWebhook()
+        with admin_logged_in():
+            self.assertContentEqual([], webhook.event_types)
+            webhook.event_types = ['foo', 'bar']
+            self.assertContentEqual(['foo', 'bar'], webhook.event_types)
+
+    def test_event_types_bad_structure(self):
+        # It's not possible to set Webhook.event_types to a list of the
+        # wrong type.
+        webhook = self.factory.makeWebhook()
+        with admin_logged_in():
+            self.assertContentEqual([], webhook.event_types)
+            with ExpectedException(AssertionError, '.*'):
+                webhook.event_types = ['foo', [1]]
+            self.assertContentEqual([], webhook.event_types)
+
 
 class TestWebhookPermissions(TestCaseWithFactory):
 
 
=== modified file 'lib/lp/services/webhooks/tests/test_webservice.py'
--- lib/lp/services/webhooks/tests/test_webservice.py	2015-08-04 05:44:42 +0000
+++ lib/lp/services/webhooks/tests/test_webservice.py	2015-08-10 06:17:10 +0000
@@ -16,6 +16,7 @@
     Is,
     KeysEqual,
     MatchesAll,
+    MatchesStructure,
     Not,
     )
 from zope.security.proxy import removeSecurityProxy
@@ -73,7 +74,7 @@
         old_mtime = representation['date_last_modified']
         patch = json.dumps(
             {'active': False, 'delivery_url': 'http://example.com/ep2',
-             'event_types': ['foo', 'bar']})
+             'event_types': ['git:push:0.1']})
         self.webservice.patch(
             self.webhook_url, 'application/json', patch, api_version='devel')
         representation = self.webservice.get(
@@ -84,7 +85,33 @@
                 {'active': Equals(False),
                  'delivery_url': Equals('http://example.com/ep2'),
                  'date_last_modified': GreaterThan(old_mtime),
-                 'event_types': Equals(['foo', 'bar'])}))
+                 'event_types': Equals(['git:push:0.1'])}))
+
+    def test_patch_event_types(self):
+        representation = self.webservice.get(
+            self.webhook_url, api_version='devel').jsonBody()
+        self.assertThat(
+            representation, ContainsDict({'event_types': Equals([])}))
+
+        # Including a valid type in event_types works.
+        response = self.webservice.patch(
+            self.webhook_url, 'application/json',
+            json.dumps({'event_types': ['git:push:0.1']}), api_version='devel')
+        self.assertEqual(209, response.status)
+        representation = self.webservice.get(
+            self.webhook_url, api_version='devel').jsonBody()
+        self.assertThat(
+            representation,
+            ContainsDict({'event_types': Equals(['git:push:0.1'])}))
+
+        # But an unknown type is rejected.
+        response = self.webservice.patch(
+            self.webhook_url, 'application/json',
+            json.dumps({'event_types': ['hg:push:0.1']}), api_version='devel')
+        self.assertThat(response,
+            MatchesStructure.byEquality(
+                status=400,
+                body="event_types: u'hg:push:0.1' isn't a valid token"))
 
     def test_anon_forbidden(self):
         response = LaunchpadWebServiceCaller().get(
@@ -279,14 +306,14 @@
         self.useFixture(FeatureFixture({'webhooks.new.enabled': 'true'}))
         response = self.webservice.named_post(
             self.target_url, 'newWebhook',
-            delivery_url='http://example.com/ep', event_types=['foo', 'bar'],
-            api_version='devel')
+            delivery_url='http://example.com/ep',
+            event_types=['git:push:0.1'], api_version='devel')
         self.assertEqual(201, response.status)
 
         representation = self.webservice.get(
             self.target_url + '/webhooks', api_version='devel').jsonBody()
         self.assertContentEqual(
-            [('http://example.com/ep', ['foo', 'bar'], True)],
+            [('http://example.com/ep', ['git:push:0.1'], True)],
             [(entry['delivery_url'], entry['event_types'], entry['active'])
              for entry in representation['entries']])
 
@@ -294,7 +321,7 @@
         self.useFixture(FeatureFixture({'webhooks.new.enabled': 'true'}))
         response = self.webservice.named_post(
             self.target_url, 'newWebhook',
-            delivery_url='http://example.com/ep', event_types=['foo', 'bar'],
+            delivery_url='http://example.com/ep', event_types=['git:push:0.1'],
             secret='sekrit', api_version='devel')
         self.assertEqual(201, response.status)
 
@@ -310,7 +337,7 @@
         webservice = LaunchpadWebServiceCaller()
         response = webservice.named_post(
             self.target_url, 'newWebhook',
-            delivery_url='http://example.com/ep', event_types=['foo', 'bar'],
+            delivery_url='http://example.com/ep', event_types=['git:push:0.1'],
             api_version='devel')
         self.assertEqual(401, response.status)
         self.assertIn('launchpad.Edit', response.body)
@@ -318,7 +345,7 @@
     def test_newWebhook_feature_flag_guard(self):
         response = self.webservice.named_post(
             self.target_url, 'newWebhook',
-            delivery_url='http://example.com/ep', event_types=['foo', 'bar'],
+            delivery_url='http://example.com/ep', event_types=['git:push:0.1'],
             api_version='devel')
         self.assertEqual(401, response.status)
         self.assertEqual(