Merge lp:~cjwatson/launchpad/webhook-default-event-types into lp:launchpad

[Colin Watson]

Only offer webhook event types that are valid for the target, and set reasonable defaults.

Only offer webhook event types that are valid for the target, and set reasonable defaults (currently just all the valid event types, but in future it may be a subset in some cases).

[William Grant (wgrant)]

Events are currently webhook-specific, and those properties aren't exported, so I'd say valid_webhook_event_types and default_webhook_event_types.

review: Approve (code)

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2015-09-29 15:53:47 +0000
+++ lib/lp/code/model/branch.py	2015-10-05 11:12:53 +0000
@@ -205,6 +205,10 @@
         enum=InformationType, default=InformationType.PUBLIC)
 
     @property
+    def valid_webhook_event_types(self):
+        return ["bzr:push:0.1"]
+
+    @property
     def private(self):
         return self.information_type in PRIVATE_INFORMATION_TYPES
 
 
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2015-09-21 10:08:08 +0000
+++ lib/lp/code/model/gitrepository.py	2015-10-05 11:12:53 +0000
@@ -234,6 +234,10 @@
         self.owner_default = False
         self.target_default = False
 
+    @property
+    def valid_webhook_event_types(self):
+        return ["git:push:0.1"]
+
     # Marker for references to Git URL layouts: ##GITNAMESPACE##
     @property
     def unique_name(self):
 
=== modified file 'lib/lp/services/webhooks/browser.py'
--- lib/lp/services/webhooks/browser.py	2015-09-08 06:55:18 +0000
+++ lib/lp/services/webhooks/browser.py	2015-10-05 11:12:53 +0000
@@ -127,7 +127,10 @@
 
     @property
     def initial_values(self):
-        return {'active': True}
+        return {
+            'active': True,
+            'event_types': self.context.default_webhook_event_types,
+            }
 
     @property
     def cancel_url(self):
 
=== modified file 'lib/lp/services/webhooks/configure.zcml'
--- lib/lp/services/webhooks/configure.zcml	2015-08-10 06:39:16 +0000
+++ lib/lp/services/webhooks/configure.zcml	2015-10-05 11:12:53 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2015 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -22,12 +22,21 @@
         <allow interface="lp.services.webhooks.interfaces.IWebhookSet"/>
     </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">
+        name="AnyWebhookEventType"
+        component="lp.services.webhooks.interfaces.AnyWebhookEventTypeVocabulary"
+        provides="zope.schema.interfaces.IVocabularyFactory">
+        <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
+    </securedutility>
+    <class class="lp.services.webhooks.interfaces.AnyWebhookEventTypeVocabulary">
+        <allow interface="zope.schema.interfaces.IVocabularyTokenized"/>
+    </class>
+    <securedutility
+        name="ValidWebhookEventType"
+        component="lp.services.webhooks.interfaces.ValidWebhookEventTypeVocabulary"
+        provides="zope.schema.interfaces.IVocabularyFactory">
+        <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
+    </securedutility>
+    <class class="lp.services.webhooks.interfaces.ValidWebhookEventTypeVocabulary">
         <allow interface="zope.schema.interfaces.IVocabularyTokenized"/>
     </class>
 
 
=== modified file 'lib/lp/services/webhooks/interfaces.py'
--- lib/lp/services/webhooks/interfaces.py	2015-09-24 13:44:02 +0000
+++ lib/lp/services/webhooks/interfaces.py	2015-10-05 11:12:53 +0000
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 __all__ = [
+    'AnyWebhookEventTypeVocabulary',
     'IWebhook',
     'IWebhookClient',
     'IWebhookDeliveryJob',
@@ -14,10 +15,11 @@
     'IWebhookJobSource',
     'IWebhookSet',
     'IWebhookTarget',
+    'WEBHOOK_EVENT_TYPES',
     'WebhookDeliveryFailure',
     'WebhookDeliveryRetry',
     'WebhookFeatureDisabled',
-    'WebhookEventTypeVocabulary',
+    'ValidWebhookEventTypeVocabulary',
     ]
 
 import httplib
@@ -95,13 +97,28 @@
     pass
 
 
-class WebhookEventTypeVocabulary(SimpleVocabulary):
+class AnyWebhookEventTypeVocabulary(SimpleVocabulary):
 
     def __init__(self, context):
         terms = [
             self.createTerm(key, key, value)
             for key, value in WEBHOOK_EVENT_TYPES.iteritems()]
-        super(WebhookEventTypeVocabulary, self).__init__(terms)
+        super(AnyWebhookEventTypeVocabulary, self).__init__(terms)
+
+
+class ValidWebhookEventTypeVocabulary(SimpleVocabulary):
+
+    def __init__(self, context):
+        # When creating a webhook, the context is the target; when editing
+        # an existing webhook, the context is the webhook itself.
+        if IWebhook.providedBy(context):
+            target = context.target
+        else:
+            target = context
+        terms = [
+            self.createTerm(key, key, WEBHOOK_EVENT_TYPES[key])
+            for key in target.valid_webhook_event_types]
+        super(ValidWebhookEventTypeVocabulary, self).__init__(terms)
 
 
 class IWebhook(Interface):
@@ -115,7 +132,7 @@
         required=True, readonly=True,
         description=_("The object for which this webhook receives events.")))
     event_types = exported(List(
-        Choice(vocabulary='WebhookEventType'), title=_("Event types"),
+        Choice(vocabulary='ValidWebhookEventType'), title=_("Event types"),
         required=True, readonly=False))
     registrant = exported(Reference(
         title=_("Registrant"), schema=IPerson, required=True, readonly=True,
@@ -195,6 +212,19 @@
         value_type=Reference(schema=IWebhook),
         readonly=True)))
 
+    valid_webhook_event_types = List(
+        Choice(vocabulary='AnyWebhookEventType'), title=_("Valid event types"),
+        description=_("Valid event types for this object type."),
+        required=True, readonly=True)
+
+    default_webhook_event_types = List(
+        Choice(vocabulary='ValidWebhookEventType'),
+        title=_("Default event types"),
+        description=_(
+            "Default event types for new webhooks attached to this object "
+            "type."),
+        required=True, readonly=True)
+
     @call_with(registrant=REQUEST_USER)
     @export_factory_operation(
         IWebhook, ['delivery_url', 'active', 'event_types', 'secret'])
 
=== modified file 'lib/lp/services/webhooks/model.py'
--- lib/lp/services/webhooks/model.py	2015-09-24 13:44:02 +0000
+++ lib/lp/services/webhooks/model.py	2015-10-05 11:12:53 +0000
@@ -66,6 +66,7 @@
     IWebhookJob,
     IWebhookJobSource,
     IWebhookSet,
+    WEBHOOK_EVENT_TYPES,
     WebhookDeliveryFailure,
     WebhookDeliveryRetry,
     WebhookFeatureDisabled,
@@ -223,6 +224,14 @@
             getUtility(IWebhookSet).findByTarget(self),
             pre_iter_hook=preload_registrants)
 
+    @property
+    def valid_webhook_event_types(self):
+        return sorted(WEBHOOK_EVENT_TYPES)
+
+    @property
+    def default_webhook_event_types(self):
+        return self.valid_webhook_event_types
+
     def newWebhook(self, registrant, delivery_url, event_types, active=True,
                    secret=None):
         if not getFeatureFlag('webhooks.new.enabled'):
 
=== modified file 'lib/lp/services/webhooks/tests/test_browser.py'
--- lib/lp/services/webhooks/tests/test_browser.py	2015-09-24 13:44:02 +0000
+++ lib/lp/services/webhooks/tests/test_browser.py	2015-10-05 11:12:53 +0000
@@ -25,6 +25,7 @@
     )
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.matchers import HasQueryCount
+from lp.testing.pages import extract_text
 from lp.testing.views import create_view
 
 
@@ -51,6 +52,7 @@
 class GitRepositoryTestHelpers:
 
     event_type = "git:push:0.1"
+    expected_event_types = [("git:push:0.1", "Git push")]
 
     def makeTarget(self):
         return self.factory.makeGitRepository()
@@ -59,6 +61,7 @@
 class BranchTestHelpers:
 
     event_type = "bzr:push:0.1"
+    expected_event_types = [("bzr:push:0.1", "Bazaar push")]
 
     def makeTarget(self):
         return self.factory.makeBranch()
@@ -204,6 +207,18 @@
             ['delivery_url'], [error.field_name for error in view.errors])
         self.assertIs(None, self.target.webhooks.one())
 
+    def test_event_types(self):
+        # Only event types that are valid for the target are offered.
+        browser = self.getUserBrowser(
+            canonical_url(self.target, view_name="+new-webhook"),
+            user=self.owner)
+        event_types = browser.getControl(name="field.event_types")
+        display_options = [
+            extract_text(option) for option in event_types.displayOptions]
+        self.assertContentEqual(
+            self.expected_event_types,
+            zip(event_types.options, display_options))
+
 
 class TestWebhookAddViewGitRepository(
     TestWebhookAddViewBase, GitRepositoryTestHelpers, TestCaseWithFactory):
@@ -289,6 +304,17 @@
                 active=True,
                 event_types=[]))
 
+    def test_event_types(self):
+        # Only event types that are valid for the target are offered.
+        browser = self.getUserBrowser(
+            canonical_url(self.webhook, view_name="+index"), user=self.owner)
+        event_types = browser.getControl(name="field.event_types")
+        display_options = [
+            extract_text(option) for option in event_types.displayOptions]
+        self.assertContentEqual(
+            self.expected_event_types,
+            zip(event_types.options, display_options))
+
 
 class TestWebhookViewGitRepository(
     TestWebhookViewBase, GitRepositoryTestHelpers, TestCaseWithFactory):