launchpad-reviewers team mailing list archive

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

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:snap-pillar-product-url into launchpad:master with ~pappacena/launchpad:snap-pillar-list-filters as a prerequisite.

Commit message:
Redirecting snaps with pillar to stay under ~user/pillar URL

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/399025
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:snap-pillar-product-url into launchpad:master.

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
@@ -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."""
@@ -69,6 +69,7 @@ import six
 from six.moves.urllib.parse import (
     quote,
     urlencode,
+    urljoin,
     )
 from storm.zope.interfaces import IResultSet
 from zope.browserpage import ViewPageTemplateFile
@@ -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.
+        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/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/tests/test_snap.py b/lib/lp/snappy/browser/tests/test_snap.py
index 61f514a..a8586a2 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,
     PersonVisibility,
     TeamMembershipPolicy,
     )
@@ -1521,6 +1522,15 @@ class TestSnapView(BaseTestSnapView):
                         "snap breadcrumb", "li",
                         text=re.compile(r"\ssnap-name\s")))))
 
+    def test_snap_with_project_pillar_redirects(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/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
+from six.moves.urllib.parse import urljoin
+from zope.component._api import getUtility
 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)
+            else:
+                return canonical_url(snap)
+
     def test_subscribe_self(self):
         snap = self.makeSnap()
         another_user = self.factory.makePerson(name="another-user")
@@ -87,8 +101,7 @@ class TestPublicSnapSubscriptionViews(BaseTestSnapView):
         browser.getControl("Subscribe").click()
 
         # We should be redirected back to snap page.
-        with admin_logged_in():
-            self.assertEqual(canonical_url(snap), browser.url)
+        self.assertEqual(self.getSnapURL(snap), browser.url)
 
         # And the new user should be listed in the subscribers list.
         self.assertTextMatchesExpressionIgnoreWhitespace(r"""
@@ -148,8 +161,7 @@ class TestPublicSnapSubscriptionViews(BaseTestSnapView):
         browser.getControl("Subscribe").click()
 
         # We should be redirected back to snap page.
-        with admin_logged_in():
-            self.assertEqual(canonical_url(snap), browser.url)
+        self.assertEqual(self.getSnapURL(snap), browser.url)
 
         # And the new user should be listed in the subscribers list.
         self.assertTextMatchesExpressionIgnoreWhitespace(r"""
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index c33ea82..e2d6c14 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -966,6 +966,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 f0cbf07..fd92f93 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -1437,6 +1437,14 @@ class SnapSet:
             raise NoSuchSnap(name)
         return snap
 
+    def getByPillarAndName(self, owner, pillar, name):
+        conditions = [Snap.owner == owner, Snap.name == name]
+        if 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 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,
     PersonVisibility,
     TeamMembershipPolicy,
     )
@@ -2858,9 +2859,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)
@@ -2868,9 +2873,13 @@ 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)
+        # If it's a redirect, try again.
+        if response.status == 301:
+            location = urlsplit(response.getheader('location')).path
+            response = admin_webservice.patch(location, 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..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
 from testtools.matchers import (
     ContainsDict,
     Equals,
@@ -789,7 +790,11 @@ 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)
+        if response.status == 301:
+            location = urlsplit(response.getheader('location')).path
+            response = self.webservice.get(location)
+        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)