launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Information



Diff comments:

> diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
> index 31c29ed..f1bb302 100644
> --- a/lib/lp/registry/browser/person.py
> +++ b/lib/lp/registry/browser/person.py
> @@ -647,7 +648,17 @@ class PersonNavigation(BranchTraversalMixin, Navigation):
>      @stepthrough('+snap')
>      def traverse_snap(self, name):
>          """Traverse to this person's snap packages."""
> -        return getUtility(ISnapSet).getByName(self.context, name)
> +        snap = getUtility(ISnapSet).getByName(self.context, name)
> +        # If it's a snap attached to a pillar, redirect to the Snap
> +        # URL under pillar's URL.

The redirection is a bit unusual; we do it for some things like bug tasks and questions that are commonly moved around, but this sort of thing would normally be expected to stay where it's put.  Can we just require Person:+snap/name to traverse only to pillarless snaps?

> +        if snap.project:
> +            person_product = getUtility(IPersonProductFactory).create(
> +                self.context, snap.project)
> +            project_url = canonical_url(person_product)
> +            snap_url = urljoin(project_url + '/', '+snap/')
> +            snap_url = urljoin(snap_url, snap.name)
> +            return self.redirectSubTree(snap_url)
> +        return snap
>  
>  
>  class PersonSetNavigation(Navigation):
> diff --git a/lib/lp/snappy/browser/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py
> index 7b8981b..a969292 100644
> --- a/lib/lp/snappy/browser/tests/test_snap.py
> +++ b/lib/lp/snappy/browser/tests/test_snap.py
> @@ -58,6 +58,7 @@ from lp.code.tests.helpers import (
>      )
>  from lp.registry.enums import (
>      BranchSharingPolicy,
> +    BranchSharingPolicy,

Duplicate import.

>      PersonVisibility,
>      TeamMembershipPolicy,
>      )
> diff --git a/lib/lp/snappy/browser/tests/test_snapsubscription.py b/lib/lp/snappy/browser/tests/test_snapsubscription.py
> index ef021d9..b091e95 100644
> --- a/lib/lp/snappy/browser/tests/test_snapsubscription.py
> +++ b/lib/lp/snappy/browser/tests/test_snapsubscription.py
> @@ -9,6 +9,9 @@ __metaclass__ = type
>  
>  
>  from fixtures import FakeLogger
> +from lp.registry.interfaces.personproduct import IPersonProductFactory

Could use a format-imports here.

> +from six.moves.urllib.parse import urljoin
> +from zope.component._api import getUtility

Just zope.component.

>  from zope.security.interfaces import Unauthorized
>  
>  from lp.app.enums import InformationType
> @@ -65,6 +68,17 @@ class BaseTestSnapView(BrowserTestCase):
>  
>  class TestPublicSnapSubscriptionViews(BaseTestSnapView):
>  
> +    def getSnapURL(self, snap):
> +        with admin_logged_in():
> +            if snap.project:
> +                person_product = getUtility(IPersonProductFactory).create(
> +                    snap.owner, snap.project)
> +                project_url = canonical_url(person_product)
> +                snap_url = urljoin(project_url + '/', '+snap/')
> +                return urljoin(snap_url, snap.name)

This seems quite circuitous; generally speaking canonical_url should just work, and if it doesn't then we need to fix it.

> +            else:
> +                return canonical_url(snap)
> +
>      def test_subscribe_self(self):
>          snap = self.makeSnap()
>          another_user = self.factory.makePerson(name="another-user")
> diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
> index 80bcaf4..528c0fa 100644
> --- a/lib/lp/snappy/tests/test_snap.py
> +++ b/lib/lp/snappy/tests/test_snap.py
> @@ -69,6 +69,7 @@ from lp.code.tests.helpers import (
>      )
>  from lp.registry.enums import (
>      BranchSharingPolicy,
> +    BranchSharingPolicy,

Duplicate import.

>      PersonVisibility,
>      TeamMembershipPolicy,
>      )
> diff --git a/lib/lp/snappy/tests/test_snapbuild.py b/lib/lp/snappy/tests/test_snapbuild.py
> index 7acff54..8404c86 100644
> --- a/lib/lp/snappy/tests/test_snapbuild.py
> +++ b/lib/lp/snappy/tests/test_snapbuild.py
> @@ -17,6 +17,7 @@ from pymacaroons import Macaroon
>  import pytz
>  import six
>  from six.moves.urllib.request import urlopen
> +from six.moves.urllib.parse import urlsplit

Could you sort these?

>  from testtools.matchers import (
>      ContainsDict,
>      Equals,


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/399025
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:snap-pillar-list-filters.