launchpad-reviewers team mailing list archive

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

addressed comments - improved validator error message, improved docstring for UI vocabulary class and added a static method to check if the event type is a subscope in Webhook class.

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

Added an explanation of what the class does and why we need this. Also mentioned that the ValidWebhookEventTypeVocabulary enables the full subscopes from the API.

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

adding a  static method is_subscope in the class Webhook, and calling it here for the filtering logic

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

rewritten to be clearer

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


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