launchpad-reviewers team mailing list archive

[Ines Almeida <mp+485489@xxxxxxxxxxxxxxxxxx>]

LGTM, mostly needing comments and clarity into why some of the things are the way they are so that the next people that go into that code won't be confused :)

Diff comments:

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

This tells someone what this is doing, but doesn't really help with "why". I understand that this is supposed to be temporary while there isn't a UI for it, but it's still important to make it clear for the next people that read the code.

I would add a sentence explaining why it is this is excluding subscopes from the UI, and why this new class is needed when there is already a `ValidWebhookEventTypeVocabulary` class

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

Someone coming back to this would be quite confused as to what this is about. Ideally there would be a `isSubscope(key)` method we could call that would make it easy to understand what it is we are checking. It could be a staticmethod in the Webhook class or something like it.

> +                terms.append(
> +                    self.createTerm(key, key, WEBHOOK_EVENT_TYPES[key])
> +                )
> +        super().__init__(terms)
>  
>  
>  class WebhookNavigation(Navigation):
> 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
> @@ -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 "

I think this error message could be clearer, it's a little confusing atm. Could you please update it to something like "Please, select either the parent event type ('{parent}') or its subscopes - not both".

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

Extra "" here?

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


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