Merge ~cjwatson/launchpad:refactor-searchPPAs into launchpad:master

[Colin Watson]

Refactor Distribution.searchPPAs to use get_enabled_archive_filter

This doesn't behave quite identically to the previous code, but I think the changes are slight improvements: search results for commercial admins include private archives to reflect their view privileges, and search results for ordinary users include archives where they have upload permissions even if they don't own them.

Those are niche changes, though, and this is mainly just intended as a refactoring. My motivations here are to get rid of some more SQLObject-flavoured code, and that it's easier to understand code when it uses well-established helper functions for moderately complex tasks rather than having open-coded near-equivalents for them.

[Ines Almeida (ines-almeida)]

The changes make sense to me, and look good code wise.

The only thing I see "missing" is test changes, which suggests that maybe we don't really have a lot of coverage on the `searchPPAs` function specifically. I think it would benefit from some edge cases tests that reflects these changes.

review: Approve

cb167cf... by Colin Watson on 2023-09-11

Add a few unit tests for Distribution.searchPPAs

[Colin Watson (cjwatson)]

Yeah, that's a reasonable point. There are some doctests, but those are annoying to extend. How's this?

[Ines Almeida (ines-almeida)]

Tests look good to me, thanks!

review: Approve

diff --git a/lib/lp/registry/model/distribution.py b/lib/lp/registry/model/distribution.py
index 44085f9..bd3e4fe 100644
--- a/lib/lp/registry/model/distribution.py
+++ b/lib/lp/registry/model/distribution.py
@@ -17,6 +17,7 @@ from storm.expr import (
     SQL,
     And,
     Coalesce,
+    Column,
     Desc,
     Exists,
     Func,
@@ -26,6 +27,7 @@ from storm.expr import (
     Not,
     Or,
     Select,
+    Table,
 )
 from storm.info import ClassAlias
 from storm.locals import Int, List, Reference
@@ -183,7 +185,7 @@ from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.soyuz.interfaces.binarypackagename import IBinaryPackageNameSet
 from lp.soyuz.interfaces.publishing import active_publishing_status
-from lp.soyuz.model.archive import Archive
+from lp.soyuz.model.archive import Archive, get_enabled_archive_filter
 from lp.soyuz.model.archivefile import ArchiveFile
 from lp.soyuz.model.distributionsourcepackagerelease import (
     DistributionSourcePackageRelease,
@@ -1847,55 +1849,38 @@ class Distribution(
 
     def searchPPAs(self, text=None, show_inactive=False, user=None):
         """See `IDistribution`."""
+        ValidPersonOrTeamCache = Table("ValidPersonOrTeamCache")
         clauses = [
-            """
-        Archive.purpose = %s AND
-        Archive.distribution = %s AND
-        Archive.owner = ValidPersonOrTeamCache.id
-        """
-            % sqlvalues(ArchivePurpose.PPA, self)
+            Archive.distribution == self,
+            Archive.owner == Column("id", ValidPersonOrTeamCache),
         ]
 
-        clauseTables = ["ValidPersonOrTeamCache"]
-        orderBy = ["Archive.displayname"]
+        order_by = [Archive.displayname]
 
         if not show_inactive:
             clauses.append(
-                """
-            Archive.id IN (
-                SELECT archive FROM SourcepackagePublishingHistory
-                WHERE status IN %s)
-            """
-                % sqlvalues(active_publishing_status)
+                Archive.id.is_in(
+                    Select(
+                        SourcePackagePublishingHistory.archive_id,
+                        SourcePackagePublishingHistory.status.is_in(
+                            active_publishing_status
+                        ),
+                    )
+                )
             )
 
         if text:
-            orderBy.insert(0, rank_by_fti(Archive, text))
+            order_by.insert(0, rank_by_fti(Archive, text))
             clauses.append(fti_search(Archive, text))
 
-        if user is not None:
-            if not user.inTeam(getUtility(ILaunchpadCelebrities).admin):
-                clauses.append(
-                    """
-                ((Archive.private = FALSE AND Archive.enabled = TRUE) OR
-                 Archive.owner = %s OR
-                 %s IN (SELECT TeamParticipation.person
-                        FROM TeamParticipation
-                        WHERE TeamParticipation.person = %s AND
-                              TeamParticipation.team = Archive.owner)
-                )
-                """
-                    % sqlvalues(user, user, user)
-                )
-        else:
-            clauses.append(
-                "Archive.private = FALSE AND Archive.enabled = TRUE"
+        clauses.append(
+            get_enabled_archive_filter(
+                user, purpose=ArchivePurpose.PPA, include_public=True
             )
-
-        return Archive.select(
-            And(*clauses), orderBy=orderBy, clauseTables=clauseTables
         )
 
+        return IStore(Archive).find(Archive, *clauses).order_by(order_by)
+
     def getPendingAcceptancePPAs(self):
         """See `IDistribution`."""
         query = """
diff --git a/lib/lp/registry/tests/test_distribution.py b/lib/lp/registry/tests/test_distribution.py
index cce72bf..10810e7 100644
--- a/lib/lp/registry/tests/test_distribution.py
+++ b/lib/lp/registry/tests/test_distribution.py
@@ -79,6 +79,7 @@ from lp.services.webapp.interfaces import OAuthPermission
 from lp.services.worlddata.interfaces.country import ICountrySet
 from lp.soyuz.enums import ArchivePurpose, PackagePublishingStatus
 from lp.soyuz.interfaces.archive import IArchiveSet
+from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
 from lp.soyuz.interfaces.distributionsourcepackagerelease import (
     IDistributionSourcePackageRelease,
 )
@@ -2848,3 +2849,72 @@ class TestDistributionPublishedSources(TestCaseWithFactory):
         # source packages to them should not affect the number of queries
         # executed here.
         self.assertThat(recorder2, HasQueryCount.byEquality(recorder1))
+
+
+class TestDistributionSearchPPAs(TestCaseWithFactory):
+    # XXX cjwatson 2023-09-11: See also lib/lp/soyuz/doc/distribution.rst
+    # and lib/lp/soyuz/doc/distribution.rst for related doctests which
+    # haven't yet been turned into unit tests.
+
+    layer = DatabaseFunctionalLayer
+
+    def test_private_invisible_by_unprivileged_user(self):
+        distribution = self.factory.makeDistribution()
+        archive = self.factory.makeArchive(
+            distribution=distribution, private=True
+        )
+        with person_logged_in(self.factory.makePerson()) as user:
+            self.assertNotIn(
+                archive, distribution.searchPPAs(user=user, show_inactive=True)
+            )
+
+    def test_private_visible_by_owner(self):
+        distribution = self.factory.makeDistribution()
+        owner = self.factory.makePerson()
+        archive = self.factory.makeArchive(
+            distribution=distribution, owner=owner, private=True
+        )
+        with person_logged_in(owner):
+            self.assertIn(
+                archive,
+                distribution.searchPPAs(user=owner, show_inactive=True),
+            )
+
+    def test_private_visible_by_uploader(self):
+        distribution = self.factory.makeDistribution()
+        archive = self.factory.makeArchive(
+            distribution=distribution, private=True
+        )
+        uploader = self.factory.makePerson()
+        getUtility(IArchivePermissionSet).newComponentUploader(
+            archive, uploader, "main"
+        )
+        with person_logged_in(uploader):
+            self.assertIn(
+                archive,
+                distribution.searchPPAs(user=uploader, show_inactive=True),
+            )
+
+    def test_private_visible_by_commercial_admin(self):
+        distribution = self.factory.makeDistribution()
+        archive = self.factory.makeArchive(
+            distribution=distribution, private=True
+        )
+        with celebrity_logged_in("commercial_admin") as commercial_admin:
+            self.assertIn(
+                archive,
+                distribution.searchPPAs(
+                    user=commercial_admin, show_inactive=True
+                ),
+            )
+
+    def test_private_visible_by_admin(self):
+        distribution = self.factory.makeDistribution()
+        archive = self.factory.makeArchive(
+            distribution=distribution, private=True
+        )
+        with celebrity_logged_in("admin") as admin:
+            self.assertIn(
+                archive,
+                distribution.searchPPAs(user=admin, show_inactive=True),
+            )