launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/webhook-default-event-types into lp:launchpad.

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #342729 in Launchpad itself: "Should support post-commit webhooks"
  https://bugs.launchpad.net/launchpad/+bug/342729

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/webhook-default-event-types/+merge/273054

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).
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/webhook-default-event-types into lp:launchpad.

=== 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-01 12:33:30 +0000
@@ -205,6 +205,10 @@
         enum=InformationType, default=InformationType.PUBLIC)
 
     @property
+    def valid_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-01 12:33:30 +0000
@@ -234,6 +234,10 @@
         self.owner_default = False
         self.target_default = False
 
+    @property
+    def valid_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-01 12:33:30 +0000
@@ -127,7 +127,10 @@
 
     @property
     def initial_values(self):
-        return {'active': True}
+        return {
+            'active': True,
+            'event_types': self.context.default_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-01 12:33:30 +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-01 12:33:30 +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_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_event_types = List(
+        Choice(vocabulary='AnyWebhookEventType'), title=_("Valid event types"),
+        description=_("Valid event types for this object type."),
+        required=True, readonly=True)
+
+    default_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-01 12:33:30 +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_event_types(self):
+        return sorted(WEBHOOK_EVENT_TYPES)
+
+    @property
+    def default_event_types(self):
+        return self.valid_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-01 12:33:30 +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):