launchpad-reviewers team mailing list archive

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

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.