launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/oci/browser/ocirecipesubscription.py b/lib/lp/oci/browser/ocirecipesubscription.py
> new file mode 100644
> index 0000000..2b83696
> --- /dev/null
> +++ b/lib/lp/oci/browser/ocirecipesubscription.py
> @@ -0,0 +1,176 @@
> +# Copyright 2020-2021 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""OCI recipe subscription views."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> +    'OCIRecipePortletSubscribersContent'
> +]
> +
> +from lp.oci.interfaces.ocirecipesubscription import IOCIRecipeSubscription

format-imports?

> +from zope.component import getUtility
> +from zope.formlib.form import action
> +from zope.security.interfaces import ForbiddenAttribute
> +
> +from lp.app.browser.launchpadform import (
> +    LaunchpadEditFormView,
> +    LaunchpadFormView,
> +    )
> +from lp.registry.interfaces.person import IPersonSet
> +from lp.services.webapp import (
> +    canonical_url,
> +    LaunchpadView,
> +    )
> +from lp.services.webapp.authorization import (
> +    check_permission,
> +    precache_permission_for_objects,
> +    )
> +
> +
> +class OCIRecipePortletSubscribersContent(LaunchpadView):
> +    """View for the contents for the subscribers portlet."""
> +
> +    def subscriptions(self):
> +        """Return a decorated list of OCI recipe subscriptions."""
> +
> +        # Cache permissions so private subscribers can be rendered.
> +        # The security adaptor will do the job also but we don't want or
> +        # need the expense of running several complex SQL queries.
> +        subscriptions = list(self.context.subscriptions)
> +        person_ids = [sub.person.id for sub in subscriptions]
> +        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
> +            person_ids, need_validity=True))
> +        if self.user is not None:
> +            subscribers = [
> +                subscription.person for subscription in subscriptions]
> +            precache_permission_for_objects(
> +                self.request, "launchpad.LimitedView", subscribers)
> +
> +        visible_subscriptions = [
> +            subscription for subscription in subscriptions
> +            if check_permission("launchpad.LimitedView", subscription.person)]
> +        return sorted(
> +            visible_subscriptions,
> +            key=lambda subscription: subscription.person.displayname)
> +
> +
> +class RedirectToOCIRecipeMixin:
> +    @property
> +    def next_url(self):
> +        if self.ocirecipe.visibleByUser(self.user):
> +            return canonical_url(self.ocirecipe)
> +        # If the subscriber can no longer see the OCI recipe, tries to
> +        # redirect to the pillar page.
> +        try:
> +            pillar = self.ocirecipe.pillar
> +            if pillar is not None and pillar.userCanLimitedView(self.user):
> +                return canonical_url(pillar)
> +        except ForbiddenAttribute:
> +            pass
> +        # If not possible, redirect user back to its own page.
> +        return canonical_url(self.user)
> +
> +    cancel_url = next_url
> +
> +
> +class OCIRecipeSubscriptionEditView(RedirectToOCIRecipeMixin,
> +                                    LaunchpadEditFormView):
> +    """The view for editing OCI recipe subscriptions."""
> +    schema = IOCIRecipeSubscription
> +    field_names = []
> +
> +    @property
> +    def page_title(self):
> +        return (
> +            "Edit subscription to OCI recipe %s" %
> +            self.ocirecipe.displayname)
> +
> +    @property
> +    def label(self):
> +        return (
> +            "Edit subscription to OCI recipe for %s" %
> +            self.person.displayname)
> +
> +    def initialize(self):
> +        self.ocirecipe = self.context.recipe
> +        self.person = self.context.person
> +        super(OCIRecipeSubscriptionEditView, self).initialize()
> +
> +    @action("Unsubscribe", name="unsubscribe")
> +    def unsubscribe_action(self, action, data):
> +        """Unsubscribe the team from the OCI recipe."""
> +        self.ocirecipe.unsubscribe(self.person, self.user)
> +        self.request.response.addNotification(
> +            "%s has been unsubscribed from this OCI recipe."
> +            % self.person.displayname)
> +
> +
> +class _OCIRecipeSubscriptionCreationView(RedirectToOCIRecipeMixin,
> +                                         LaunchpadFormView):
> +    """Contains the common functionality of the Add and Edit views."""
> +
> +    schema = IOCIRecipeSubscription
> +    field_names = []
> +
> +    def initialize(self):
> +        self.ocirecipe = self.context
> +        super(_OCIRecipeSubscriptionCreationView, self).initialize()
> +
> +
> +class OCIRecipeSubscriptionAddView(_OCIRecipeSubscriptionCreationView):
> +
> +    page_title = label = "Subscribe to OCI recipe"
> +
> +    @action("Subscribe")
> +    def subscribe(self, action, data):
> +        # To catch the stale post problem, check that the user is not
> +        # subscribed before continuing.
> +        if self.context.getSubscription(self.user) is not None:
> +            self.request.response.addNotification(
> +                "You are already subscribed to this OCI recipe.")
> +        else:
> +            self.context.subscribe(self.user, self.user)
> +            self.request.response.addNotification(
> +                "You have subscribed to this OCI recipe.")
> +
> +
> +class OCIRecipeSubscriptionAddOtherView(_OCIRecipeSubscriptionCreationView):
> +    """View used to subscribe someone other than the current user."""
> +
> +    field_names = ["person"]
> +    for_input = True
> +
> +    # Since we are subscribing other people, the current user
> +    # is never considered subscribed.
> +    user_is_subscribed = False
> +
> +    page_title = label = "Subscribe to OCI recipe"
> +
> +    def validate(self, data):
> +        if "person" in data:
> +            person = data["person"]
> +            subscription = self.context.getSubscription(person)
> +            if (subscription is None
> +                    and not self.context.userCanBeSubscribed(person)):
> +                self.setFieldError(
> +                    "person",
> +                    "Open and delegated teams cannot be subscribed to "
> +                    "private OCI recipes.")
> +
> +    @action("Subscribe", name="subscribe_action")
> +    def subscribe_action(self, action, data):
> +        """Subscribe the specified user to the OCI recipe."""
> +        person = data["person"]
> +        subscription = self.context.getSubscription(person)
> +        if subscription is None:
> +            self.context.subscribe(person, self.user)
> +            self.request.response.addNotification(
> +                "%s has been subscribed to this OCI recipe." %
> +                person.displayname)
> +        else:
> +            self.request.response.addNotification(
> +                "%s was already subscribed to this OCI recipe." %
> +                person.displayname)
> diff --git a/lib/lp/oci/interfaces/ocirecipe.py b/lib/lp/oci/interfaces/ocirecipe.py
> index 0b006d2..8c946fb 100644
> --- a/lib/lp/oci/interfaces/ocirecipe.py
> +++ b/lib/lp/oci/interfaces/ocirecipe.py
> @@ -306,6 +306,10 @@ class IOCIRecipeView(Interface):
>          description=_("Use the credentials on a Distribution for "
>                        "registry upload"))
>  
> +    subscriptions = CollectionField(
> +        title=_("OCIRecipeSubscriptions associated with this snap recipe."),

s/snap recipe/OCI recipe/

> +        readonly=True, value_type=Reference(Interface))
> +
>      subscribers = CollectionField(
>          title=_("Persons subscribed to this snap recipe."),
>          readonly=True, value_type=Reference(IPerson))
> diff --git a/lib/lp/oci/templates/ocirecipe-portlet-privacy.pt b/lib/lp/oci/templates/ocirecipe-portlet-privacy.pt
> new file mode 100644
> index 0000000..a8d3dd5
> --- /dev/null
> +++ b/lib/lp/oci/templates/ocirecipe-portlet-privacy.pt
> @@ -0,0 +1,16 @@
> +<div
> +  xmlns:tal="http://xml.zope.org/namespaces/tal";
> +  xmlns:metal="http://xml.zope.org/namespaces/metal";
> +  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
> +  id="privacy"
> +  tal:attributes="
> +    class python: path('context/private') and 'portlet private' or 'portlet public'

Oh neat, I didn't know about `path()`.  But I think you might as well just write `context.private` instead of `path('context/private')`, unless I'm missing something?

I also always find that I have to think quite hard about "condition and expr1 or expr2" constructions as opposed to Python's normal ternary expression of "expr1 if condition else expr2" (even if the ordering is weird).

> +  "
> +>
> +    <span tal:attributes="class python: path('context/private') and 'sprite private' or 'sprite public'"
> +      >This OCI recipe contains <strong
> +        tal:content="python: path('context/private') and 'Private' or 'Public'"
> +      ></strong> information</span>
> +</div>
> +
> +


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/399750
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:ocirecipe-subscription.