launchpad-reviewers team mailing list archive

[Ines Almeida <mp+485628@xxxxxxxxxxxxxxxxxx>]

Good work! It's not easy to get into Launchpad's UI intricacies :)
Left a few comments

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",

This naming might come from my draft, but it sounds a little weird.
Maybe we can just call it 'WebhookCheckboxWidget' since it's quite related to webhooks due to the subscopes by now?

>      "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.

Might add something like "This is particularly relevant for UI purposes as it ensures event types are displayed orderly"

> +    """
> +
>      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 think this can be slightly simplified.

How about

        scopes = defaultdict(list)
        for key in target.valid_webhook_event_types:
            # Group subscopes under their parent
            if Webhook.is_subscope(key):
                parent = Webhook.parent_of(key)
                scopes[parent].append(key)
            elif key not in scopes:
                scopes[key] = []

        terms = []
        # Create terms for parents and for their corresponding subscopes
        for parent, subscopes in scopes.items():
            terms.append(self._createTerm(parent))
            for subscope in subscopes:
                terms.append(self._createTerm(subscope))


And have something like

    def _createTerm(self, scope):
        return self.createTerm(scope, scope, WEBHOOK_EVENT_TYPES[scope])

> +        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) {

Minor, but the name of the file is refering to the `webhook` deliveries, which doesnt feel related to the event types. Given this is not needed in the `add-webhook` view, maybe might as well create a new similar file and call it in the 2 views that need it?

>      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"]');

Hmmm, we might as well only add this event listener to the parent checkboxes.
I'm assuming something like this would work? We would minimize the times we trigger the function

 var checkboxes = Y.all('input[type="checkbox"]'):not([data-parent]);

> +    checkboxes.on('change', handleCheckboxChange);
> +
> +    // Find all parent checkboxes that are checked
> +    var checkedParents = Y.all('input[type="checkbox"]:checked');

Well, this seems to do the right thing, but this is getting all checkboxes whic isnt in line with the name of the variable. Can also add that `:not([data-parent])` here (if it works)

> +    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:

This makes it sound like it gets the parent of a webhook due to naming.
The `is_subscope` at least refers to something more specific.
How about naming these to something more specific:
 - is_event_subscope
 - event_parent_scope

Can be something else, but `parent_of` sounds too generic

> +        """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">

You can remove this

> +    </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",

Might make sense for this test to unorder them like
+                "merge-proposal:0.1::create",
+                "merge-proposal:0.1",
+                "merge-proposal:0.1::push",
+                "git:push:0.1",

and see that they are ordered in the end

> +                "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",

I see now that my suggestion to simplify the code above might not lead to this outcome. It should never happen that there are subscopes without scopes, but might as well have this behavior. Feel free to take the parts that make sense from the simplification suggestion above

> +                "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.