launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
> index 8311bd1..91da499 100644
> --- a/lib/lp/registry/browser/person.py
> +++ b/lib/lp/registry/browser/person.py
> @@ -646,7 +646,12 @@ 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, this is not the right place
> +        # to traverse. The correct URL should be under IPersonProduct.
> +        if snap.project:
> +            raise NotFoundError(name)

I'd prefer something like `return getUtility(ISnapSet).getByPillarAndName(self.context, None, name)` (which I realize would need a bit of work in that method).

> +        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..25383d9 100644
> --- a/lib/lp/snappy/browser/tests/test_snap.py
> +++ b/lib/lp/snappy/browser/tests/test_snap.py
> @@ -1521,6 +1519,15 @@ class TestSnapView(BaseTestSnapView):
>                          "snap breadcrumb", "li",
>                          text=re.compile(r"\ssnap-name\s")))))
>  
> +    def test_snap_with_project_pillar_redirects(self):

This test name no longer seems quite right.

> +        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/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
> index 2a522f5..604aacc 100644
> --- a/lib/lp/snappy/tests/test_snap.py
> +++ b/lib/lp/snappy/tests/test_snap.py
> @@ -2887,9 +2891,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)

Is this still needed, now that you're no longer redirecting?

>          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..e12eb61 100644
> --- a/lib/lp/snappy/tests/test_snapbuild.py
> +++ b/lib/lp/snappy/tests/test_snapbuild.py
> @@ -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)

Is this still needed, now that you're no longer redirecting?

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


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