launchpad-reviewers team mailing list archive

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

Pushed the requested changes. This should be ready for another round of review.

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.

Ok! I'm changing <browser:url /> definition for the Snap, which should make this traverse to not be used when the snap has a pillar, but I'll raise a 404 here just in case.

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

Probably some merge confusion. Fixing it.

>      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

Ok!

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

pycharm really likes importing this from _api... fixing it.

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

I agree. Fixing the URL mapping, and removing this method in favor of raw `canonical_url`.

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

Ok!

>      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

Ok!

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