launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code db



Diff comments:

> 
> === modified file 'lib/lp/snappy/tests/test_snap.py'
> --- lib/lp/snappy/tests/test_snap.py	2019-07-15 17:15:25 +0000
> +++ lib/lp/snappy/tests/test_snap.py	2019-07-24 12:45:34 +0000
> @@ -1674,6 +1674,24 @@
>              [snaps[0], snaps[2], snaps[4], snaps[6]],
>              getUtility(ISnapSet).findByURLPrefixes(prefixes, owner=owners[0]))
>  
> +    def test_findByStoreName(self):
> +        # ISnapSet.findByStoreName returns visible Snaps with the given
> +        # store name.
> +        store_names = ["foo", "bar"]
> +        owners = [self.factory.makePerson() for i in range(2)]
> +        snaps = []
> +        for store_name in store_names:
> +            for owner in owners:
> +                snaps.append(self.factory.makeSnap(
> +                    registrant=owner, owner=owner, store_name=store_name))
> +        snaps.append(self.factory.makeSnap())
> +        self.assertContentEqual(
> +            snaps[:2], getUtility(ISnapSet).findByStoreName(store_names[0]))
> +        self.assertContentEqual(
> +            [snaps[0]],
> +            getUtility(ISnapSet).findByStoreName(
> +                store_names[0], owner=owners[0]))

Can you add a visible_by_user test too?

> +
>      def test_getSnapcraftYaml_bzr_snap_snapcraft_yaml(self):
>          def getInventory(unique_name, dirname, *args, **kwargs):
>              if dirname == "snap":
> @@ -3004,6 +3022,78 @@
>              [ws_snaps[i] for i in (0, 1, 4, 5, 8, 9, 12, 13)],
>              [entry["self_link"] for entry in response.jsonBody()["entries"]])
>  
> +    def test_findByStoreName(self):
> +        # lp.snaps.findByStoreName returns visible Snaps with the given
> +        # store name.
> +        persons = [self.factory.makePerson(), self.factory.makePerson()]
> +        store_names = ["foo", "bar"]
> +        snaps = []
> +        for store_name in store_names:
> +            for person in persons:
> +                for private in (False, True):
> +                    snaps.append(self.factory.makeSnap(
> +                        registrant=person, owner=person, private=private,
> +                        store_name=store_name))
> +        with admin_logged_in():
> +            person_urls = [api_url(person) for person in persons]
> +            ws_snaps = [
> +                self.webservice.getAbsoluteUrl(api_url(snap))
> +                for snap in snaps]
> +        commercial_admin = (
> +            getUtility(ILaunchpadCelebrities).commercial_admin.teamowner)
> +        logout()
> +        # Anonymous requests can only see public snaps.
> +        anon_webservice = LaunchpadWebServiceCaller("test", "")
> +        response = anon_webservice.named_get(
> +            "/+snaps", "findByStoreName", store_name=store_names[0],
> +            api_version="devel")
> +        self.assertEqual(200, response.status)
> +        self.assertContentEqual(
> +            [ws_snaps[0], ws_snaps[2]],
> +            [entry["self_link"] for entry in response.jsonBody()["entries"]])
> +        response = anon_webservice.named_get(
> +            "/+snaps", "findByStoreName", store_name=store_names[0],
> +            owner=person_urls[0], api_version="devel")
> +        self.assertEqual(200, response.status)
> +        self.assertContentEqual(
> +            [ws_snaps[0]],
> +            [entry["self_link"] for entry in response.jsonBody()["entries"]])
> +        # persons[0] can see both public snaps with this store name, as well
> +        # as their own private snap.
> +        webservice = webservice_for_person(
> +            persons[0], permission=OAuthPermission.READ_PRIVATE)
> +        response = webservice.named_get(
> +            "/+snaps", "findByStoreName", store_name=store_names[0],
> +            api_version="devel")
> +        self.assertEqual(200, response.status)
> +        self.assertContentEqual(
> +            ws_snaps[:3],
> +            [entry["self_link"] for entry in response.jsonBody()["entries"]])
> +        response = webservice.named_get(
> +            "/+snaps", "findByStoreName", store_name=store_names[0],
> +            owner=person_urls[0], api_version="devel")
> +        self.assertEqual(200, response.status)
> +        self.assertContentEqual(
> +            ws_snaps[:2],
> +            [entry["self_link"] for entry in response.jsonBody()["entries"]])
> +        # Admins can see all snaps with this store name.
> +        commercial_admin_webservice = webservice_for_person(
> +            commercial_admin, permission=OAuthPermission.READ_PRIVATE)
> +        response = commercial_admin_webservice.named_get(
> +            "/+snaps", "findByStoreName", store_name=store_names[0],
> +            api_version="devel")
> +        self.assertEqual(200, response.status)
> +        self.assertContentEqual(
> +            ws_snaps[:4],
> +            [entry["self_link"] for entry in response.jsonBody()["entries"]])
> +        response = commercial_admin_webservice.named_get(
> +            "/+snaps", "findByStoreName", store_name=store_names[0],
> +            owner=person_urls[0], api_version="devel")
> +        self.assertEqual(200, response.status)
> +        self.assertContentEqual(
> +            ws_snaps[:2],
> +            [entry["self_link"] for entry in response.jsonBody()["entries"]])

I feel like this could use some vertical whitespace.

> +
>      def setProcessors(self, user, snap, names):
>          ws = webservice_for_person(
>              user, permission=OAuthPermission.WRITE_PUBLIC)


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-find-by-store-name/+merge/370562
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.