launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Addressed all the comments. Since there were quite a lot of comments, it might worth another quick round of review (at least in the new diff).

Diff comments:

> diff --git a/lib/lp/security.py b/lib/lp/security.py
> index 35c8b8c..8192755 100644
> --- a/lib/lp/security.py
> +++ b/lib/lp/security.py
> @@ -3298,17 +3299,18 @@ class ViewSnap(AuthorizationBase):
>      permission = 'launchpad.View'
>      usedfor = ISnap
>  
> -    def checkUnauthenticated(self):
> -        return not self.obj.private
> -
>      def checkAuthenticated(self, user):
> -        if not self.obj.private:
> +        # Check user visibility first: public snaps should be visible to
> +        # anyone immediately. Doing this check first can save us some
> +        # queries done by the not-so-common cases checked below.

Indeed, it can. God knows why I've made it like this. I'm changing it.

> +        if self.obj.visibleByUser(user.person):
> +            return True
> +        if user.isOwner(self.obj) or user.in_commercial_admin or user.in_admin:
>              return True
> +        return False
>  
> -        return (
> -            user.isOwner(self.obj) or
> -            user.in_commercial_admin or
> -            user.in_admin)
> +    def checkUnauthenticated(self):
> +        return self.obj.visibleByUser(None)
>  
>  
>  class EditSnap(AuthorizationBase):
> diff --git a/lib/lp/snappy/browser/snap.py b/lib/lp/snappy/browser/snap.py
> index 7a62458..bd3926c 100644
> --- a/lib/lp/snappy/browser/snap.py
> +++ b/lib/lp/snappy/browser/snap.py
> @@ -182,12 +191,31 @@ class SnapContextMenu(ContextMenu):
>  
>      facet = 'overview'
>  
> -    links = ('request_builds',)
> +    links = ('request_builds', 'add_subscriber', 'subscription')
>  
>      @enabled_with_permission('launchpad.Edit')
>      def request_builds(self):
>          return Link('+request-builds', 'Request builds', icon='add')
>  
> +    @enabled_with_permission("launchpad.AnyPerson")
> +    def subscription(self):
> +        if self.context.hasSubscription(self.user):
> +            url = "+subscription/%s" % self.user.name
> +            text = "Edit your subscription"
> +            icon = "edit"
> +        elif self.context.userCanBeSubscribed(self.user):

It makes sense. Changing it.

> +            url = "+subscribe"
> +            text = "Subscribe yourself"
> +            icon = "add"
> +        else:
> +            return None
> +        return Link(url, text, icon=icon)
> +
> +    @enabled_with_permission("launchpad.Edit")

Not particularly. I don't oppose to be a bit less restrictive here.

> +    def add_subscriber(self):
> +        text = "Subscribe someone else"
> +        return Link("+addsubscriber", text, icon="add")
> +
>  
>  class SnapView(LaunchpadView):
>      """Default view of a Snap."""
> diff --git a/lib/lp/snappy/browser/snapsubscription.py b/lib/lp/snappy/browser/snapsubscription.py
> new file mode 100644
> index 0000000..a11cbc2
> --- /dev/null
> +++ b/lib/lp/snappy/browser/snapsubscription.py
> @@ -0,0 +1,180 @@
> +# Copyright 2020-2021 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Snap subscription views."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> +    'SnapPortletSubscribersContent'
> +]
> +
> +from zope.component._api 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,
> +    )
> +from lp.snappy.interfaces.snapsubscription import ISnapSubscription
> +
> +
> +class SnapPortletSubscribersContent(LaunchpadView):
> +    """View for the contents for the subscribers portlet."""
> +
> +    def subscriptions(self):
> +        """Return a decorated list of Snap 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 RedirectToSnapMixin:
> +    @property
> +    def next_url(self):
> +        if self.snap.visibleByUser(self.user):
> +            return canonical_url(self.snap)
> +        # If the subscriber can no longer see the Snap recipe, tries to
> +        # redirect to the project page.
> +        try:
> +            project = self.snap.project
> +            if project is not None and project.userCanLimitedView(self.user):
> +                return canonical_url(self.snap.project)
> +        except ForbiddenAttribute:
> +            pass
> +        # If not possible, redirect user back to its own page.
> +        return canonical_url(self.user)
> +
> +    cancel_url = next_url
> +
> +
> +class SnapSubscriptionEditView(RedirectToSnapMixin, LaunchpadEditFormView):
> +    """The view for editing Snap recipe subscriptions."""
> +    schema = ISnapSubscription
> +    field_names = []
> +
> +    @property
> +    def page_title(self):
> +        return (
> +            "Edit subscription to Snap recipe %s" %

Ok! Changing it.

> +            self.snap.displayname)
> +
> +    @property
> +    def label(self):
> +        return (
> +            "Edit subscription to Snap recipe for %s" %
> +            self.person.displayname)
> +
> +    def initialize(self):
> +        self.snap = self.context.snap
> +        self.person = self.context.person
> +        super(SnapSubscriptionEditView, self).initialize()
> +
> +    @action("Unsubscribe", name="unsubscribe")
> +    def unsubscribe_action(self, action, data):
> +        """Unsubscribe the team from the Snap recipe."""
> +        self.snap.unsubscribe(self.person, self.user)
> +        self.request.response.addNotification(
> +            "%s has been unsubscribed from this Snap recipe."
> +            % self.person.displayname)
> +
> +
> +class _SnapSubscriptionCreationView(RedirectToSnapMixin, LaunchpadFormView):
> +    """Contains the common functionality of the Add and Edit views."""
> +
> +    schema = ISnapSubscription
> +    field_names = []
> +
> +    def initialize(self):
> +        self.snap = self.context
> +        super(_SnapSubscriptionCreationView, self).initialize()
> +
> +
> +class SnapSubscriptionAddView(_SnapSubscriptionCreationView):
> +
> +    page_title = label = "Subscribe to Snap 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.hasSubscription(self.user):
> +            self.request.response.addNotification(
> +                "You are already subscribed to this Snap recipe.")
> +        else:
> +            self.context.subscribe(self.user, self.user)
> +
> +            self.request.response.addNotification(
> +                "You have subscribed to this Snap recipe.")
> +
> +
> +class SnapSubscriptionAddOtherView(_SnapSubscriptionCreationView):
> +    """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 Snap 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 Snap recipes.")
> +
> +    @action("Subscribe", name="subscribe_action")
> +    def subscribe_action(self, action, data):
> +        """Subscribe the specified user to the Snap recipe.
> +
> +        The user must be a member of a team in order to subscribe that team
> +        to the Snap recipe. Launchpad Admins are special and they can
> +        subscribe any team.

It's possible. I'll remove this comment in all 3 places.

> +        """
> +        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 Snap recipe." %
> +                person.displayname)
> +        else:
> +            self.request.response.addNotification(
> +                "%s was already subscribed to this Snap recipe with." %

Ok!

> +                person.displayname)
> diff --git a/lib/lp/snappy/configure.zcml b/lib/lp/snappy/configure.zcml
> index a16c664..05529f8 100644
> --- a/lib/lp/snappy/configure.zcml
> +++ b/lib/lp/snappy/configure.zcml
> @@ -42,6 +42,15 @@
>          <allow interface="lp.snappy.interfaces.snap.ISnapSet" />
>      </securedutility>
>  
> +    <!-- SnapSubscription -->
> +
> +    <class class="lp.snappy.model.snapsubscription.SnapSubscription">
> +      <allow interface="lp.snappy.interfaces.snapsubscription.ISnapSubscription"/>
> +      <require
> +          permission="zope.Public"

Changing also lp.security.SnapSubscriptionView accordingly, it seems to work out just fine. I'll try to do the same adjustment on Branch/GitRepository in another branch after.

> +          set_schema="lp.snappy.interfaces.snapsubscription.ISnapSubscription"/>
> +    </class>
> +
>      <!-- SnapBuildRequest -->
>      <class class="lp.snappy.model.snap.SnapBuildRequest">
>          <require
> diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
> index b423263..c33ea82 100644
> --- a/lib/lp/snappy/interfaces/snap.py
> +++ b/lib/lp/snappy/interfaces/snap.py
> @@ -571,10 +571,25 @@ class ISnapView(Interface):
>          # Really ISnapBuild, patched in lp.snappy.interfaces.webservice.
>          value_type=Reference(schema=Interface), readonly=True)))
>  
> +    subscriptions = CollectionField(
> +        title=_("SnapSubscriptions associated with this repository."),

Ok!

> +        readonly=True,
> +        # Really IGitSubscription, patched in _schema_circular_imports.py.

I'm removing the comment here. Once we do the API for this, we can fix it.

> +        value_type=Reference(Interface))
> +
>      subscribers = CollectionField(
> -        title=_("Persons subscribed to this repository."),
> +        title=_("Persons subscribed to this snap recipe."),
>          readonly=True, value_type=Reference(IPerson))
>  
> +    def getSubscription(person):
> +        """Returns the person's snap subscription for this snap recipe."""
> +
> +    def hasSubscription(person):
> +        """Is this person subscribed to the snap recipe?"""
> +
> +    def userCanBeSubscribed(person):
> +        """Checks if the given person can be subscribed to this snap recipe."""
> +
>      def visibleByUser(user):
>          """Can the specified user see this snap recipe?"""
>  
> @@ -584,6 +599,9 @@ class ISnapView(Interface):
>          If the user is a Launchpad admin, any type is acceptable.
>          """
>  
> +    def unsubscribe(person, unsubscribed_by):
> +        """Unsubscribe a person to this snap recipe."""

Oops

> +
>  
>  class ISnapEdit(IWebhookTarget):
>      """`ISnap` methods that require launchpad.Edit permission."""
> diff --git a/lib/lp/snappy/templates/snap-index.pt b/lib/lp/snappy/templates/snap-index.pt
> index 7d3083e..5c51a5d 100644
> --- a/lib/lp/snappy/templates/snap-index.pt
> +++ b/lib/lp/snappy/templates/snap-index.pt
> @@ -58,10 +59,13 @@
>        <dl id="source"
>            tal:define="source context/source" tal:condition="source">
>          <dt>Source:</dt>
> -        <dd>
> +        <dd tal:condition="view/user_can_see_source">
>            <a tal:replace="structure source/fmt:link"/>
>            <a tal:replace="structure view/menu:overview/edit/fmt:icon"/>
>          </dd>
> +        <dd tal:condition="not: view/user_can_see_source">
> +            <span style="font-style: italic">&lt;Redacted&gt;</span>

Ok!

> +        </dd>
>        </dl>
>        <dl id="build_source_tarball"
>            tal:define="build_source_tarball context/build_source_tarball">


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/398319
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:snap-pillar-subscribe-removal-job.