comments addressed! thanks a lot for the feedback
Diff comments:
> 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",
It does :) renaming to ´WebhookCheckboxWidget´
> "PlainMultiCheckBoxWidget",
> ]
>
> 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.
docstring added and improved
> + """
> +
> 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 = []
I will still use most of my logic (for the reason you mention in the tests
comment) but use the helper method _createTerm, which seems clearer
> + 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) {
makes more sense, I created a webhooks/javascript/event_types.js with the
function and then call it from the two views.
> 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"]');
right! adding it to only parent checkboxes works fine
> + checkboxes.on('change', handleCheckboxChange);
> +
> + // Find all parent checkboxes that are checked
> + var checkedParents = Y.all('input[type="checkbox"]:checked');
works! good catch
> + 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:
renamed both
> + """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-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">
removed
> + </script>
> </metal:block>
> </head>
> <body>
> 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",
test passes too! will leave it like you mention
> + "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"],
> + )
--
https://code.launchpad.net/~alvarocs/launchpad/+git/launchpad/+merge/485628
Your team Launchpad code reviewers is requested to review the proposed merge of
~alvarocs/launchpad:mp-subscopes-ui into launchpad:master.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
