launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32482
[Merge] ~alvarocs/launchpad:mp-subscopes-ui into launchpad:master
Alvaro Crespo Serrano has proposed merging ~alvarocs/launchpad:mp-subscopes-ui into launchpad:master.
Commit message:
Enable merge proposal subscopes through the UI
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~alvarocs/launchpad/+git/launchpad/+merge/485628
Update Launchpad webhook UI to properly show the webhook event type subscopes properly. The UI now indents subscopes and disables/unchecks them when the parent is checked.
Code changes:
-style.css: add the .indentation CSS class for subscopes.
-itemswidgets.py: new MultilevelLabeledMultiCheckBoxWidget to handle subscopes.
-webhooks/browser.py: drop previous vocabulary class, use new widget in add and edit views. Point the Add view to a new js template.
-webhooks/interfaces.py: rewrite ValidWebhookEventTypeVocabulary class to enforce order of parent and subscopes.
-webhooks/model.py: new helper method parent_of
-webhooks/javascript/deliveries.js: add common namespace.initScopeCheckboxes() function
-webhooks/templates/webhook-add.pt: template for Add webhook view
-webhooks/templates/webhook-index: import the new JS function from deliveries.js script
Tests:
-webhooks/test_browser.py: update event-type expected with new subscopes
-webhooks/test_interfaces.py: unit tests for ValidWebhookEventTypeVocabulary.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~alvarocs/launchpad:mp-subscopes-ui into launchpad:master.
diff --git a/lib/canonical/launchpad/icing/style.css b/lib/canonical/launchpad/icing/style.css
index a66400f..a5d5400 100644
--- a/lib/canonical/launchpad/icing/style.css
+++ b/lib/canonical/launchpad/icing/style.css
@@ -918,6 +918,9 @@ table.listing tr.webhook-delivery:hover {
cursor: pointer;
}
+.indentation {
+ padding-left: 1rem;
+}
/* ====== Content area styles ====== */
diff --git a/lib/lp/app/widgets/itemswidgets.py b/lib/lp/app/widgets/itemswidgets.py
index 54f6c90..492715e 100644
--- a/lib/lp/app/widgets/itemswidgets.py
+++ b/lib/lp/app/widgets/itemswidgets.py
@@ -10,6 +10,7 @@ __all__ = [
"LaunchpadDropdownWidget",
"LaunchpadRadioWidget",
"LaunchpadRadioWidgetWithDescription",
+ "MultilevelLabeledMultiCheckBoxWidget",
"PlainMultiCheckBoxWidget",
]
@@ -23,6 +24,7 @@ from zope.schema.interfaces import IChoice
from zope.schema.vocabulary import SimpleVocabulary
from lp.services.webapp.escaping import html_escape
+from lp.services.webhooks.model import Webhook
class LaunchpadDropdownWidget(DropdownWidget):
@@ -111,6 +113,47 @@ class LabeledMultiCheckBoxWidget(PlainMultiCheckBoxWidget):
return self._joinButtonToMessageTemplate % (option_id, elem, text)
+class MultilevelLabeledMultiCheckBoxWidget(PlainMultiCheckBoxWidget):
+ """MultiCheckBoxWidget which wraps option labels with proper
+ <label> elements.
+ """
+
+ SUBSCOPE_CSS = "indentation"
+
+ def _renderItem(self, index, text, value, name, cssClass, checked=False):
+ """Render a checkbox and text in a label with a style attribute."""
+
+ kw = {}
+
+ label_class = ""
+ if Webhook.is_subscope(value):
+ label_class = f' class="{self.SUBSCOPE_CSS}"'
+ kw["data-parent"] = Webhook.parent_of(value)
+
+ _label = (
+ '<label for="%s"%s style="font-weight: normal">%s %s</label> '
+ )
+
+ if checked:
+ kw["checked"] = "checked"
+ if value in self.disabled_items:
+ kw["disabled"] = "disabled"
+ value = html_escape(value)
+ text = html_escape(text)
+ id = "%s.%s" % (name, index)
+ elem = renderElement(
+ "input",
+ value=value,
+ name=name,
+ id=id,
+ cssClass=cssClass,
+ type="checkbox",
+ **kw,
+ )
+ option_id = "%s.%s" % (self.name, index)
+ return _label % (option_id, label_class, elem, text)
+
+
# XXX Brad Bollenbach 2006-08-10 bugs=56062: This is a hack to
# workaround Zope's RadioWidget not properly selecting the default value.
class LaunchpadRadioWidget(RadioWidget):
diff --git a/lib/lp/services/webhooks/browser.py b/lib/lp/services/webhooks/browser.py
index bafba3d..226c498 100644
--- a/lib/lp/services/webhooks/browser.py
+++ b/lib/lp/services/webhooks/browser.py
@@ -12,15 +12,13 @@ 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,
LaunchpadFormView,
action,
)
-from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget
+from lp.app.widgets.itemswidgets import MultilevelLabeledMultiCheckBoxWidget
from lp.code.interfaces.gitrepository import IGitRepository
from lp.services.propertycache import cachedproperty
from lp.services.webapp import (
@@ -35,38 +33,7 @@ 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 (
- WEBHOOK_EVENT_TYPES,
- IWebhook,
- IWebhookSet,
-)
-from lp.services.webhooks.model import Webhook
-
-
-class UIValidWebhookEventTypeVocabulary(SimpleVocabulary):
- """A UI-specific vocabulary that excludes subscopes.
-
- This is used in form views to present only top-level
- event types (e.g., 'merge-proposal:0.1'), hiding subscopes,
- which currently have no UI support.
-
- The full list including subscopes is still available via
- the Launchpad API through ValidWebhookEventTypeVocabulary.
- """
-
- 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 Webhook.is_subscope(key):
- terms.append(
- self.createTerm(key, key, WEBHOOK_EVENT_TYPES[key])
- )
- super().__init__(terms)
+from lp.services.webhooks.interfaces import IWebhook, IWebhookSet
class WebhookNavigation(Navigation):
@@ -138,23 +105,18 @@ class WebhookEditSchema(Interface):
include=[
"delivery_url",
"active",
+ "event_types",
"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"
schema = WebhookEditSchema
- custom_widget_event_types = LabeledMultiCheckBoxWidget
+ custom_widget_event_types = MultilevelLabeledMultiCheckBoxWidget
next_url = None
@property
@@ -199,7 +161,7 @@ class WebhookView(LaunchpadEditFormView):
label = "Manage webhook"
schema = WebhookEditSchema
- custom_widget_event_types = LabeledMultiCheckBoxWidget
+ custom_widget_event_types = MultilevelLabeledMultiCheckBoxWidget
@property
def field_names(self):
diff --git a/lib/lp/services/webhooks/configure.zcml b/lib/lp/services/webhooks/configure.zcml
index de2dda9..3c7ad5c 100644
--- a/lib/lp/services/webhooks/configure.zcml
+++ b/lib/lp/services/webhooks/configure.zcml
@@ -41,18 +41,6 @@
<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"
provides="lp.services.webhooks.interfaces.IWebhookJobSource">
@@ -108,7 +96,7 @@
name="+new-webhook"
permission="launchpad.Edit"
class="lp.services.webhooks.browser.WebhookAddView"
- template="../../app/templates/generic-edit.pt" />
+ template="templates/webhook-add.pt" />
<adapter
provides="lp.services.webapp.interfaces.IBreadcrumb"
diff --git a/lib/lp/services/webhooks/interfaces.py b/lib/lp/services/webhooks/interfaces.py
index 307dccd..d0fb52c 100644
--- a/lib/lp/services/webhooks/interfaces.py
+++ b/lib/lp/services/webhooks/interfaces.py
@@ -92,6 +92,10 @@ class AnyWebhookEventTypeVocabulary(SimpleVocabulary):
class ValidWebhookEventTypeVocabulary(SimpleVocabulary):
+ """Vocabulary that returns the valid webhook event types in
+ parent-subscopes order.
+ """
+
def __init__(self, context):
# When creating a webhook, the context is the target; when editing
# an existing webhook, the context is the webhook itself.
@@ -99,10 +103,27 @@ class ValidWebhookEventTypeVocabulary(SimpleVocabulary):
target = context.target
else:
target = context
- terms = [
- self.createTerm(key, key, WEBHOOK_EVENT_TYPES[key])
- for key in target.valid_webhook_event_types
- ]
+ subscopes_by_parent = {}
+ top_level = []
+ for key in target.valid_webhook_event_types:
+ # Group subscopes under their parent
+ if "::" in key:
+ parent = key.split("::")[0]
+ subscopes_by_parent.setdefault(parent, []).append(key)
+ else:
+ top_level.append(key)
+ terms = []
+ # Create terms for parents and for their corresponding subscopes
+ for parent in top_level:
+ terms.append(
+ self.createTerm(parent, parent, WEBHOOK_EVENT_TYPES[parent])
+ )
+ for subscope in subscopes_by_parent.get(parent, []):
+ terms.append(
+ self.createTerm(
+ subscope, subscope, WEBHOOK_EVENT_TYPES[subscope]
+ )
+ )
super().__init__(terms)
diff --git a/lib/lp/services/webhooks/javascript/deliveries.js b/lib/lp/services/webhooks/javascript/deliveries.js
index 81aea30..d3d6eb8 100644
--- a/lib/lp/services/webhooks/javascript/deliveries.js
+++ b/lib/lp/services/webhooks/javascript/deliveries.js
@@ -301,7 +301,48 @@ namespace.retry_delivery = function(deliveries_widget, delivery_url) {
var lp_client = new Y.lp.client.Launchpad();
lp_client.named_post(delivery_url, 'retry', config);
};
+/**
+ * Activate the 'parent-subscope' behaviour for the check‑boxes
+ * rendered by MultilevelLabeledMultiCheckBoxWidget.
+ */
+namespace.initScopeCheckboxes = function () {
+ // Handle checkbox parent-child relationships
+ var handleCheckboxChange = function (e) {
+ var checkbox = e.currentTarget;
+ var value = checkbox.get('value');
+ var isChecked = checkbox.get('checked');
+ // If this is a parent checkbox, handle children
+ var childCheckboxes =
+ Y.all('input[type="checkbox"][data-parent="' + value + '"]');
+
+ childCheckboxes.each(function (childCheckbox) {
+ if (isChecked) {
+ // If parent is checked, uncheck and disable child
+ childCheckbox.set('checked', false);
+ childCheckbox.set('disabled', true);
+ } else {
+ // If parent is unchecked, enable child
+ childCheckbox.set('disabled', false);
+ }
+ });
+ };
+ // Set up event listeners for all checkboxes
+ var checkboxes = Y.all('input[type="checkbox"]');
+ checkboxes.on('change', handleCheckboxChange);
+
+ // Find all parent checkboxes that are checked
+ var checkedParents = Y.all('input[type="checkbox"]:checked');
+ checkedParents.each(function (parentCheckbox) {
+ var value = parentCheckbox.get('value');
+ var childCheckboxes =
+ Y.all('input[type="checkbox"][data-parent="' + value + '"]');
+ childCheckboxes.each(function (childCheckbox) {
+ childCheckbox.set('checked', false);
+ childCheckbox.set('disabled', true);
+ });
+ });
+};
}, "0.1", {"requires": ["event", "node", "widget", "lp.app.date",
"lp.app.listing_navigator", "lp.client",
"lp.mustache"]});
diff --git a/lib/lp/services/webhooks/model.py b/lib/lp/services/webhooks/model.py
index 994f72d..8027fe5 100644
--- a/lib/lp/services/webhooks/model.py
+++ b/lib/lp/services/webhooks/model.py
@@ -209,6 +209,11 @@ class Webhook(StormBase):
"""Return True if the event_type is a subscope (contains '::')."""
return "::" in event_type
+ @staticmethod
+ def parent_of(event_type: str) -> str:
+ """Return the top‑level parent of a subscope event type."""
+ return event_type.split("::", 1)[0]
+
@implementer(IWebhookSet)
class WebhookSet:
diff --git a/lib/lp/services/webhooks/templates/webhook-add.pt b/lib/lp/services/webhooks/templates/webhook-add.pt
new file mode 100644
index 0000000..3a0189c
--- /dev/null
+++ b/lib/lp/services/webhooks/templates/webhook-add.pt
@@ -0,0 +1,25 @@
+<html
+ xmlns="http://www.w3.org/1999/xhtml";
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+ metal:use-macro="view/macro:page/main_only"
+ i18n:domain="launchpad">
+
+ <head>
+ <metal:script fill-slot="head_epilogue">
+ <script tal:content="structure string:
+ LPJS.use('base','node','event','lp.services.webhooks.deliveries',
+ function (Y) {
+ Y.lp.services.webhooks.deliveries.initScopeCheckboxes();
+ });
+ "/>
+ </metal:script>
+ </head>
+
+ <body>
+ <div metal:fill-slot="main">
+ <div metal:use-macro="context/@@launchpad_form/form" />
+ </div>
+ </body>
+</html>
\ No newline at end of file
diff --git a/lib/lp/services/webhooks/templates/webhook-index.pt b/lib/lp/services/webhooks/templates/webhook-index.pt
index ce04e54..a4ae1cc 100644
--- a/lib/lp/services/webhooks/templates/webhook-index.pt
+++ b/lib/lp/services/webhooks/templates/webhook-index.pt
@@ -52,7 +52,13 @@
deliveries_widget.render();
});
});
+ LPJS.use('base','node','event','lp.services.webhooks.deliveries',
+ function (Y) {
+ Y.lp.services.webhooks.deliveries.initScopeCheckboxes();
+ });
"/>
+ <script type="text/javascript">
+ </script>
</metal:block>
</head>
<body>
diff --git a/lib/lp/services/webhooks/tests/test_browser.py b/lib/lp/services/webhooks/tests/test_browser.py
index 283a009..ce6ebe6 100644
--- a/lib/lp/services/webhooks/tests/test_browser.py
+++ b/lib/lp/services/webhooks/tests/test_browser.py
@@ -66,6 +66,12 @@ class GitRepositoryTestHelpers:
("ci:build:0.1", "CI build"),
("git:push:0.1", "Git push"),
("merge-proposal:0.1", "Merge proposal"),
+ ("merge-proposal:0.1::create", "Merge proposal Created"),
+ ("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"),
+ ("merge-proposal:0.1::push", "Merge proposal Pushed"),
+ ("merge-proposal:0.1::review", "Merge proposal Reviewed"),
]
def makeTarget(self):
@@ -80,6 +86,12 @@ class BranchTestHelpers:
expected_event_types = [
("bzr:push:0.1", "Bazaar push"),
("merge-proposal:0.1", "Merge proposal"),
+ ("merge-proposal:0.1::create", "Merge proposal Created"),
+ ("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"),
+ ("merge-proposal:0.1::push", "Merge proposal Pushed"),
+ ("merge-proposal:0.1::review", "Merge proposal Reviewed"),
]
def makeTarget(self):
diff --git a/lib/lp/services/webhooks/tests/test_interfaces.py b/lib/lp/services/webhooks/tests/test_interfaces.py
new file mode 100644
index 0000000..9b3568e
--- /dev/null
+++ b/lib/lp/services/webhooks/tests/test_interfaces.py
@@ -0,0 +1,59 @@
+import unittest
+
+from zope.interface import implementer
+
+from lp.services.webhooks.interfaces import (
+ IWebhookTarget,
+ ValidWebhookEventTypeVocabulary,
+)
+
+
+@implementer(IWebhookTarget)
+class FakeTarget:
+ def __init__(self, valid_event_types):
+ self.valid_webhook_event_types = valid_event_types
+
+
+class TestValidWebhookEventTypeVocabulary(unittest.TestCase):
+ def test_ordering_with_parent_and_subscopes(self):
+ target = FakeTarget(
+ [
+ "merge-proposal:0.1",
+ "merge-proposal:0.1::create",
+ "merge-proposal:0.1::push",
+ "git:push:0.1",
+ ]
+ )
+ vocab = ValidWebhookEventTypeVocabulary(target)
+ self.assertEqual(
+ [term.token for term in vocab],
+ [
+ "merge-proposal:0.1",
+ "merge-proposal:0.1::create",
+ "merge-proposal:0.1::push",
+ "git:push:0.1",
+ ],
+ )
+
+ def test_skips_subscope_without_parent(self):
+ target = FakeTarget(
+ [
+ "merge-proposal:0.1::review",
+ "git:push:0.1",
+ ]
+ )
+ vocab = ValidWebhookEventTypeVocabulary(target)
+ self.assertEqual([term.token for term in vocab], ["git:push:0.1"])
+
+ def test_parent_only(self):
+ target = FakeTarget(
+ [
+ "git:push:0.1",
+ "merge-proposal:0.1",
+ ]
+ )
+ vocab = ValidWebhookEventTypeVocabulary(target)
+ self.assertEqual(
+ [term.token for term in vocab],
+ ["git:push:0.1", "merge-proposal:0.1"],
+ )
Follow ups
