Merge ~pappacena/launchpad:snap-pillar-product-url into launchpad:master

[Thiago F. Pappacena]

Redirecting snaps with pillar to stay under ~user/pillar URL

[Colin Watson (cjwatson)]

review: Needs Information

[Thiago F. Pappacena (pappacena)]

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

[Colin Watson (cjwatson)]

review: Approve

[Thiago F. Pappacena (pappacena)]

Pushed the requested changes.

diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
index b15ec69..002b94a 100644
--- a/lib/lp/registry/browser/person.py
+++ b/lib/lp/registry/browser/person.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Person-related view classes."""
@@ -646,7 +646,11 @@ 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).getByPillarAndName(
+            self.context, None, name)
+        if snap is None:
+            raise NotFoundError(name)
+        return snap
 
 
 class PersonSetNavigation(Navigation):
diff --git a/lib/lp/registry/browser/personproduct.py b/lib/lp/registry/browser/personproduct.py
index a249b27..7d850e3 100644
--- a/lib/lp/registry/browser/personproduct.py
+++ b/lib/lp/registry/browser/personproduct.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Views, menus and traversal related to PersonProducts."""
@@ -30,6 +30,7 @@ from lp.services.webapp import (
     )
 from lp.services.webapp.breadcrumb import Breadcrumb
 from lp.services.webapp.interfaces import IMultiFacetedBreadcrumb
+from lp.snappy.interfaces.snap import ISnapSet
 
 
 class PersonProductNavigation(PersonTargetDefaultVCSNavigationMixin,
@@ -53,6 +54,13 @@ class PersonProductNavigation(PersonTargetDefaultVCSNavigationMixin,
         else:
             return branch
 
+    @stepthrough('+snap')
+    def traverse_snap(self, name):
+        return getUtility(ISnapSet).getByPillarAndName(
+            owner=self.context.person,
+            pillar=self.context.product,
+            name=name)
+
 
 @implementer(IMultiFacetedBreadcrumb)
 class PersonProductBreadcrumb(Breadcrumb):
diff --git a/lib/lp/snappy/browser/configure.zcml b/lib/lp/snappy/browser/configure.zcml
index b11f797..ebbbb66 100644
--- a/lib/lp/snappy/browser/configure.zcml
+++ b/lib/lp/snappy/browser/configure.zcml
@@ -11,8 +11,7 @@
     <facet facet="overview">
         <browser:url
             for="lp.snappy.interfaces.snap.ISnap"
-            path_expression="string:+snap/${name}"
-            attribute_to_parent="owner" />
+            urldata="lp.snappy.browser.snap.SnapURL" />
         <browser:defaultView
             for="lp.snappy.interfaces.snap.ISnap"
             name="+index" />
diff --git a/lib/lp/snappy/browser/snap.py b/lib/lp/snappy/browser/snap.py
index ab4ebf0..c51356e 100644
--- a/lib/lp/snappy/browser/snap.py
+++ b/lib/lp/snappy/browser/snap.py
@@ -28,7 +28,10 @@ from six.moves.urllib.parse import urlencode
 from zope.component import getUtility
 from zope.error.interfaces import IErrorReportingUtility
 from zope.formlib.widget import CustomWidgetFactory
-from zope.interface import Interface
+from zope.interface import (
+    implementer,
+    Interface,
+    )
 from zope.schema import (
     Choice,
     Dict,
@@ -61,6 +64,7 @@ from lp.code.browser.widgets.gitref import GitRefWidget
 from lp.code.interfaces.gitref import IGitRef
 from lp.registry.enums import VCSType
 from lp.registry.interfaces.person import IPersonSet
+from lp.registry.interfaces.personproduct import IPersonProductFactory
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.features import getFeatureFlag
 from lp.services.propertycache import cachedproperty
@@ -81,6 +85,7 @@ from lp.services.webapp.breadcrumb import (
     Breadcrumb,
     NameBreadcrumb,
     )
+from lp.services.webapp.interfaces import ICanonicalUrlData
 from lp.services.webapp.url import urlappend
 from lp.services.webhooks.browser import WebhookTargetNavigationMixin
 from lp.snappy.browser.widgets.snaparchive import SnapArchiveWidget
@@ -112,6 +117,27 @@ from lp.soyuz.browser.build import get_build_by_id_str
 from lp.soyuz.interfaces.archive import IArchive
 
 
+@implementer(ICanonicalUrlData)
+class SnapURL:
+    """Snap URL creation rules."""
+    rootsite = 'mainsite'
+
+    def __init__(self, snap):
+        self.snap = snap
+
+    @property
+    def inside(self):
+        owner = self.snap.owner
+        project = self.snap.project
+        if project is None:
+            return owner
+        return getUtility(IPersonProductFactory).create(owner, project)
+
+    @property
+    def path(self):
+        return "+snap/%s" % self.snap.name
+
+
 class SnapNavigation(WebhookTargetNavigationMixin, Navigation):
     usedfor = ISnap
 
diff --git a/lib/lp/snappy/browser/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py
index c57f412..f6a1cfc 100644
--- a/lib/lp/snappy/browser/tests/test_snap.py
+++ b/lib/lp/snappy/browser/tests/test_snap.py
@@ -572,9 +572,7 @@ class TestSnapAddView(BaseTestSnapView):
         self.assertEqual(303, int(browser.headers["Status"].split(" ", 1)[0]))
         parsed_location = urlsplit(browser.headers["Location"])
         self.assertEqual(
-            urlsplit(
-                canonical_url(snap, rootsite="code") +
-                "/+authorize/+login")[:3],
+            urlsplit(canonical_url(snap) + "/+authorize/+login")[:3],
             parsed_location[:3])
         expected_args = {
             "discharge_macaroon_action": ["field.actions.complete"],
@@ -1521,6 +1519,15 @@ class TestSnapView(BaseTestSnapView):
                         "snap breadcrumb", "li",
                         text=re.compile(r"\ssnap-name\s")))))
 
+    def test_snap_with_project_pillar_url(self):
+        project = self.factory.makeProduct()
+        snap = self.factory.makeSnap(project=project)
+        browser = self.getViewBrowser(snap)
+        with admin_logged_in():
+            expected_url = 'http://launchpad.test/~{}/{}/+snap/{}'.format(
+                snap.owner.name, project.name, snap.name)
+        self.assertEqual(expected_url, browser.url)
+
     def test_index_bzr(self):
         branch = self.factory.makePersonalBranch(
             owner=self.person, name="snap-branch")
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index 4bef0ef..28f0824 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -965,6 +965,9 @@ class ISnapSet(Interface):
     def getByName(owner, name):
         """Return the appropriate `ISnap` for the given objects."""
 
+    def getByPillarAndName(owner, pillar, name):
+        """Returns the appropriate `ISnap` for the given pillar and name."""
+
     @operation_parameters(
         owner=Reference(IPerson, title=_("Owner"), required=True))
     @operation_returns_collection_of(ISnap)
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 27f6694..5b1be23 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -1439,6 +1439,18 @@ class SnapSet:
             raise NoSuchSnap(name)
         return snap
 
+    def getByPillarAndName(self, owner, pillar, name):
+        conditions = [Snap.owner == owner, Snap.name == name]
+        if pillar is None:
+            # If we start supporting more pillars, remember to add the
+            # conditions here.
+            conditions.append(Snap.project == None)
+        elif IProduct.providedBy(pillar):
+            conditions.append(Snap.project == pillar)
+        else:
+            raise NotImplementedError("Unknown pillar for snap: %s" % pillar)
+        return IStore(Snap).find(Snap, *conditions).one()
+
     def _getSnapsFromCollection(self, collection, owner=None,
                                 visible_by_user=None):
         if IBranchCollection.providedBy(collection):
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 0c6f68e..5e31742 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -1684,6 +1684,22 @@ class TestSnapSet(TestCaseWithFactory):
             getUtility(ISnapSet).exists(self.factory.makePerson(), snap.name))
         self.assertFalse(getUtility(ISnapSet).exists(snap.owner, "different"))
 
+    def test_getByPillarAndName(self):
+        owner = self.factory.makePerson()
+        project = self.factory.makeProduct()
+        project_snap = self.factory.makeSnap(
+            name='proj-snap', owner=owner, registrant=owner, project=project)
+        no_project_snap = self.factory.makeSnap(
+            name='no-proj-snap', owner=owner, registrant=owner)
+
+        snap_set = getUtility(ISnapSet)
+        self.assertEqual(
+            project_snap,
+            snap_set.getByPillarAndName(owner, project, 'proj-snap'))
+        self.assertEqual(
+            no_project_snap,
+            snap_set.getByPillarAndName(owner, None, 'no-proj-snap'))
+
     def test_findByOwner(self):
         # ISnapSet.findByOwner returns all Snaps with the given owner.
         owners = [self.factory.makePerson() for i in range(2)]
@@ -2877,9 +2893,13 @@ class TestSnapWebservice(TestCaseWithFactory):
         branch = self.factory.makeAnyBranch(
             owner=self.person,
             information_type=InformationType.PRIVATESECURITY)
+        project = self.factory.makeProduct(
+            owner=self.person, registrant=self.person,
+            information_type=InformationType.PROPRIETARY,
+            branch_sharing_policy=BranchSharingPolicy.PROPRIETARY)
         snap = self.factory.makeSnap(
             registrant=self.person, owner=self.person, branch=branch,
-            private=True)
+            project=project, information_type=InformationType.PROPRIETARY)
         admin = getUtility(ILaunchpadCelebrities).admin.teamowner
         with person_logged_in(self.person):
             snap_url = api_url(snap)
@@ -2887,9 +2907,9 @@ class TestSnapWebservice(TestCaseWithFactory):
         admin_webservice = webservice_for_person(
             admin, permission=OAuthPermission.WRITE_PRIVATE)
         admin_webservice.default_api_version = "devel"
-        response = admin_webservice.patch(
-            snap_url, "application/json",
-            json.dumps({"information_type": 'Public'}))
+        data = json.dumps({"information_type": 'Public'})
+        content_type = "application/json"
+        response = admin_webservice.patch(snap_url, content_type, data)
         self.assertEqual(400, response.status)
         self.assertEqual(
             b"Snap recipe contains private information and cannot be public.",
diff --git a/lib/lp/snappy/tests/test_snapbuild.py b/lib/lp/snappy/tests/test_snapbuild.py
index 7acff54..ee67203 100644
--- a/lib/lp/snappy/tests/test_snapbuild.py
+++ b/lib/lp/snappy/tests/test_snapbuild.py
@@ -789,7 +789,8 @@ class TestSnapBuildWebservice(TestCaseWithFactory):
             self.factory.makePerson(), permission=OAuthPermission.WRITE_PUBLIC)
         unpriv_webservice.default_api_version = "devel"
         logout()
-        self.assertEqual(200, self.webservice.get(build_url).status)
+        response = self.webservice.get(build_url)
+        self.assertEqual(200, response.status)
         # 404 since we aren't allowed to know that the private team exists.
         self.assertEqual(404, unpriv_webservice.get(build_url).status)