launchpad-reviewers team mailing list archive

[Alvaro Crespo Serrano <mp+485489@xxxxxxxxxxxxxxxxxx>]

Alvaro Crespo Serrano has proposed merging ~alvarocs/launchpad:validate-mp-subscopes into launchpad:master.

Commit message:
Enable merge proposal subscopes through Launchpad API

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~alvarocs/launchpad/+git/launchpad/+merge/485489
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~alvarocs/launchpad:validate-mp-subscopes into launchpad:master.

diff --git a/lib/lp/code/model/branch.py b/lib/lp/code/model/branch.py
index 20fdf26..ab07555 100644
--- a/lib/lp/code/model/branch.py
+++ b/lib/lp/code/model/branch.py
@@ -220,7 +220,16 @@ class Branch(StormBase, WebhookTargetMixin, BzrIdentityMixin):
 
     @property
     def valid_webhook_event_types(self):
-        return ["bzr:push:0.1", "merge-proposal:0.1"]
+        return [
+            "bzr:push:0.1",
+            "merge-proposal:0.1",
+            "merge-proposal:0.1::create",
+            "merge-proposal:0.1::push",
+            "merge-proposal:0.1::review",
+            "merge-proposal:0.1::edit",
+            "merge-proposal:0.1::status-change",
+            "merge-proposal:0.1::delete",
+        ]
 
     @property
     def default_webhook_event_types(self):
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 440b270..483f986 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -417,7 +417,17 @@ class GitRepository(
 
     @property
     def valid_webhook_event_types(self):
-        return ["ci:build:0.1", "git:push:0.1", "merge-proposal:0.1"]
+        return [
+            "ci:build:0.1",
+            "git:push:0.1",
+            "merge-proposal:0.1",
+            "merge-proposal:0.1::create",
+            "merge-proposal:0.1::push",
+            "merge-proposal:0.1::review",
+            "merge-proposal:0.1::edit",
+            "merge-proposal:0.1::status-change",
+            "merge-proposal:0.1::delete",
+        ]
 
     @property
     def default_webhook_event_types(self):
diff --git a/lib/lp/services/webhooks/browser.py b/lib/lp/services/webhooks/browser.py
index 00be986..2763f2c 100644
--- a/lib/lp/services/webhooks/browser.py
+++ b/lib/lp/services/webhooks/browser.py
@@ -12,6 +12,8 @@ from lazr.restful.interface import use_template
 from lazr.restful.interfaces import IJSONRequestCache
 from zope.component import getUtility
 from zope.interface import Interface
+from zope.schema import Choice, List
+from zope.schema.vocabulary import SimpleVocabulary
 
 from lp.app.browser.launchpadform import (
     LaunchpadEditFormView,
@@ -33,7 +35,29 @@ from lp.services.webapp.batching import (
     get_batch_properties_for_json_cache,
 )
 from lp.services.webapp.breadcrumb import Breadcrumb
-from lp.services.webhooks.interfaces import IWebhook, IWebhookSet
+from lp.services.webhooks.interfaces import (
+    WEBHOOK_EVENT_TYPES,
+    IWebhook,
+    IWebhookSet,
+)
+
+
+class UIValidWebhookEventTypeVocabulary(SimpleVocabulary):
+    """A UI-specific vocabulary that excludes subscopes for form views."""
+
+    def __init__(self, context):
+        if IWebhook.providedBy(context):
+            target = context.target
+        else:
+            target = context
+
+        terms = []
+        for key in target.valid_webhook_event_types:
+            if "::" not in key:
+                terms.append(
+                    self.createTerm(key, key, WEBHOOK_EVENT_TYPES[key])
+                )
+        super().__init__(terms)
 
 
 class WebhookNavigation(Navigation):
@@ -104,13 +128,18 @@ class WebhookEditSchema(Interface):
         IWebhook,
         include=[
             "delivery_url",
-            "event_types",
             "active",
             "secret",
             "git_ref_pattern",
         ],
     )
 
+    event_types = List(
+        title="Event types",
+        required=True,
+        value_type=Choice(vocabulary="UIValidWebhookEventType"),
+    )
+
 
 class WebhookAddView(LaunchpadFormView):
     page_title = label = "Add webhook"
diff --git a/lib/lp/services/webhooks/configure.zcml b/lib/lp/services/webhooks/configure.zcml
index 6dbf8d6..de2dda9 100644
--- a/lib/lp/services/webhooks/configure.zcml
+++ b/lib/lp/services/webhooks/configure.zcml
@@ -41,6 +41,17 @@
     <class class="lp.services.webhooks.interfaces.ValidWebhookEventTypeVocabulary">
         <allow interface="zope.schema.interfaces.IVocabularyTokenized"/>
     </class>
+    
+    <lp:securedutility
+        name="UIValidWebhookEventType"
+        component="lp.services.webhooks.browser.UIValidWebhookEventTypeVocabulary"
+        provides="zope.schema.interfaces.IVocabularyFactory">
+        <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
+    </lp:securedutility>
+
+    <class class="lp.services.webhooks.browser.UIValidWebhookEventTypeVocabulary">
+        <allow interface="zope.schema.interfaces.IVocabularyTokenized"/>
+    </class>
 
     <lp:securedutility
         component="lp.services.webhooks.model.WebhookJob"
diff --git a/lib/lp/services/webhooks/interfaces.py b/lib/lp/services/webhooks/interfaces.py
index 69b5978..789f7f0 100644
--- a/lib/lp/services/webhooks/interfaces.py
+++ b/lib/lp/services/webhooks/interfaces.py
@@ -38,6 +38,7 @@ from zope.schema import Bool, Choice, Datetime, Dict, Int, List, TextLine
 from zope.schema.vocabulary import SimpleVocabulary
 
 from lp import _
+from lp.app.validators import LaunchpadValidationError
 from lp.registry.interfaces.person import IPerson
 from lp.services.fields import URIField
 from lp.services.job.interfaces.job import IJob, IJobSource, IRunnableJob
@@ -57,12 +58,12 @@ WEBHOOK_EVENT_TYPES = {
     "livefs:build:0.1": "Live filesystem build",
     "merge-proposal:0.1": "Merge proposal",
     # Merge proposal subscopes
-    "merge-proposal:0.1::create": "Create",
-    "merge-proposal:0.1::push": "Push",
-    "merge-proposal:0.1::review": "Review",
-    "merge-proposal:0.1::edit": "Edit",
-    "merge-proposal:0.1::status-change": "Status Change",
-    "merge-proposal:0.1::delete": "Delete",
+    "merge-proposal:0.1::create": "Merge proposal Created",
+    "merge-proposal:0.1::push": "Merge proposal Pushed",
+    "merge-proposal:0.1::review": "Merge proposal Reviewed",
+    "merge-proposal:0.1::edit": "Merge proposal Edited",
+    "merge-proposal:0.1::status-change": "Merge proposal Status Changed",
+    "merge-proposal:0.1::delete": "Merge proposal Deleted",
     "oci-recipe:build:0.1": "OCI recipe build",
     "snap:build:0.1": "Snap build",
     "craft-recipe:build:0.1": "Craft recipe build",
@@ -105,6 +106,19 @@ class ValidWebhookEventTypeVocabulary(SimpleVocabulary):
         super().__init__(terms)
 
 
+def validate_event_type_parent_subscope(event_types):
+    """Validator to handle parent and its subscopes selection."""
+    parents = {et.split("::")[0] for et in event_types if "::" in et}
+    for parent in parents:
+        if parent in event_types:
+            raise LaunchpadValidationError(
+                f"Both the parent {parent} and any of its subscopes cannot "
+                "be selected. Please, select either the parent or one or "
+                "more subscopes."
+            )
+    return True
+
+
 @exported_as_webservice_entry(as_of="beta")
 class IWebhook(Interface):
     id = Int(title=_("ID"), readonly=True, required=True)
@@ -126,6 +140,7 @@ class IWebhook(Interface):
             title=_("Event types"),
             required=True,
             readonly=False,
+            constraint=validate_event_type_parent_subscope,
         )
     )
     registrant = exported(
diff --git a/lib/lp/services/webhooks/tests/test_webservice.py b/lib/lp/services/webhooks/tests/test_webservice.py
index 53f7e93..033f815 100644
--- a/lib/lp/services/webhooks/tests/test_webservice.py
+++ b/lib/lp/services/webhooks/tests/test_webservice.py
@@ -260,6 +260,62 @@ class TestWebhook(TestCaseWithFactory):
         with person_logged_in(self.owner):
             self.assertIs(None, self.webhook.secret)
 
+    def test_patch_event_types_allows_only_parent_scope(self):
+        # Parent scope alone should be accepted
+        patch = json.dumps({"event_types": ["merge-proposal:0.1"]})
+        response = self.webservice.patch(
+            self.webhook_url, "application/json", patch, api_version="devel"
+        )
+        self.assertEqual(209, response.status)
+        representation = self.webservice.get(
+            self.webhook_url, api_version="devel"
+        ).jsonBody()
+        self.assertEqual(["merge-proposal:0.1"], representation["event_types"])
+
+    def test_patch_event_types_allows_only_subscopes(self):
+        # Subscopes alone should be accepted
+        patch = json.dumps(
+            {
+                "event_types": [
+                    "" "merge-proposal:0.1::create",
+                    "merge-proposal:0.1::edit",
+                ]
+            }
+        )
+        response = self.webservice.patch(
+            self.webhook_url, "application/json", patch, api_version="devel"
+        )
+        self.assertEqual(209, response.status)
+        representation = self.webservice.get(
+            self.webhook_url, api_version="devel"
+        ).jsonBody()
+        self.assertEqual(
+            ["merge-proposal:0.1::create", "merge-proposal:0.1::edit"],
+            representation["event_types"],
+        )
+
+    def test_patch_event_types_rejects_parent_and_subscopes(self):
+        # A parent and one of its subscopes cannot be selected together
+        patch = json.dumps(
+            {
+                "event_types": [
+                    "merge-proposal:0.1",
+                    "merge-proposal:0.1::create",
+                ]
+            }
+        )
+        response = self.webservice.patch(
+            self.webhook_url,
+            "application/json",
+            patch,
+            api_version="devel",
+        )
+        self.assertEqual(400, response.status)
+        self.assertIn(
+            b"Both the parent merge-proposal:0.1 and any of its subscopes",
+            response.body,
+        )
+
 
 class TestWebhookDelivery(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer