launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #19206
[Merge] lp:~wgrant/launchpad/webhook-event-types into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/webhook-event-types into lp:launchpad.
Commit message:
Improve Webhook.event_types widget and value checking.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/webhook-event-types/+merge/267470
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/webhook-event-types into lp:launchpad.
=== 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(
Follow ups
