launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/getPPAOwnedByPerson-cross-distro into lp:launchpad.

Commit message:
ArchiveSet.getPPAOwnedByUser now requires a distribution when filtering by name.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/getPPAOwnedByPerson-cross-distro/+merge/225419

The first bit of non-Ubuntu PPAs: ArchiveSet.getPPAOwnedByUser now requires a distribution when filtering by name.

I also prevented it from raising NoSuchPPA, as that was a special case when a non-existent name was specified. Instead, Person.getPPAByName raises the exception itself. This makes the tests nicer.
-- 
https://code.launchpad.net/~wgrant/launchpad/getPPAOwnedByPerson-cross-distro/+merge/225419
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/getPPAOwnedByPerson-cross-distro into lp:launchpad.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2014-07-02 06:25:53 +0000
+++ lib/lp/registry/model/person.py	2014-07-03 02:03:29 +0000
@@ -309,7 +309,10 @@
     ArchivePurpose,
     ArchiveStatus,
     )
-from lp.soyuz.interfaces.archive import IArchiveSet
+from lp.soyuz.interfaces.archive import (
+    IArchiveSet,
+    NoSuchPPA,
+    )
 from lp.soyuz.interfaces.archivesubscriber import IArchiveSubscriberSet
 from lp.soyuz.model.archive import (
     Archive,
@@ -3060,7 +3063,12 @@
 
     def getPPAByName(self, name):
         """See `IPerson`."""
-        return getUtility(IArchiveSet).getPPAOwnedByPerson(self, name)
+        ppa = getUtility(IArchiveSet).getPPAOwnedByPerson(
+            self, distribution=getUtility(ILaunchpadCelebrities).ubuntu,
+            name=name)
+        if ppa is None:
+            raise NoSuchPPA(name)
+        return ppa
 
     def createPPA(self, name=None, displayname=None, description=None,
                   private=False, suppress_subscription_notifications=False):

=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt	2013-05-10 06:28:17 +0000
+++ lib/lp/soyuz/doc/archive.txt	2014-07-03 02:03:29 +0000
@@ -1200,60 +1200,6 @@
     >>> jblack_ppas.count()
     0
 
-Another similar method, getPPAOwnedByPerson(), will return the named PPA
-owned by the person, or if a name is not supplied will default to the
-first PPA that the person created.
-
-    >>> ppa_owner = factory.makePerson(
-    ...     name="ppa-owner", displayname="PPA Owner")
-    >>> ignored = login_person(ppa_owner)
-
-If no PPAs match the search criteria, and a name is not given, then
-None is returned.
-
-    >>> print archive_set.getPPAOwnedByPerson(ppa_owner)
-    None
-
-    >>> zoobuntu = factory.makeDistribution(name='zoobuntu')
-    >>> ppa1 = factory.makeArchive(
-    ...     owner=ppa_owner, name="first-ppa", distribution=zoobuntu)
-    >>> ppa2 = factory.makeArchive(
-    ...     owner=ppa_owner, name="second-ppa", distribution=zoobuntu)
-
-    >>> print archive_set.getPPAOwnedByPerson(ppa_owner).displayname
-    PPA named first-ppa for PPA Owner
-
-    >>> print archive_set.getPPAOwnedByPerson(
-    ...     ppa_owner, name="second-ppa").displayname
-    PPA named second-ppa for PPA Owner
-
-If the named PPA does not exist, a NoSuchPPA exception is raised.
-
-    >>> print archive_set.getPPAOwnedByPerson(
-    ...     ppa_owner, name="goat").displayname
-    Traceback (most recent call last):
-    ...
-    NoSuchPPA: No such ppa: 'goat'.
-
-A list of statuses may be optionally used, which cause the search to
-only select PPAs with one of those statuses.  The first PPA will not
-be selected and the second is returned.
-
-    >>> ppa1.status = ArchiveStatus.DELETED
-    >>> ppa2.status = ArchiveStatus.ACTIVE
-    >>> print archive_set.getPPAOwnedByPerson(
-    ...     ppa_owner, statuses=[ArchiveStatus.ACTIVE]).displayname
-    PPA named second-ppa for PPA Owner
-
-The parameter has_packages can be specified to filter based on whether
-the PPA has published packages.
-
-    >>> spph = factory.makeSourcePackagePublishingHistory(
-    ...     archive=ppa2, status=PackagePublishingStatus.PUBLISHED)
-    >>> print archive_set.getPPAOwnedByPerson(
-    ...     ppa_owner, has_packages=True).displayname
-    PPA named second-ppa for PPA Owner
-
 The method getPrivatePPAs() will return a result set of all PPAs that are
 private.
 

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2014-06-30 13:51:15 +0000
+++ lib/lp/soyuz/model/archive.py	2014-07-03 02:03:29 +0000
@@ -2279,25 +2279,25 @@
         """See `IArchiveSet`."""
         return iter(Archive.select())
 
-    def getPPAOwnedByPerson(self, person, name=None, statuses=None,
-                            has_packages=False):
+    def getPPAOwnedByPerson(self, person, distribution=None, name=None,
+                            statuses=None, has_packages=False):
         """See `IArchiveSet`."""
         # See Person._members which also directly queries this.
         store = Store.of(person)
         clause = [
             Archive.purpose == ArchivePurpose.PPA,
             Archive.owner == person]
+        if distribution is not None:
+            clause.append(Archive.distribution == distribution)
         if name is not None:
+            assert distribution is not None
             clause.append(Archive.name == name)
         if statuses is not None:
             clause.append(Archive.status.is_in(statuses))
         if has_packages:
             clause.append(
                     SourcePackagePublishingHistory.archive == Archive.id)
-        result = store.find(Archive, *clause).order_by(Archive.id).first()
-        if name is not None and result is None:
-            raise NoSuchPPA(name)
-        return result
+        return store.find(Archive, *clause).order_by(Archive.id).first()
 
     def getPPAsForUser(self, user):
         """See `IArchiveSet`."""

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2014-04-24 03:24:53 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2014-07-03 02:03:29 +0000
@@ -2901,6 +2901,63 @@
             distribution=boingolinux, name=boingolinux.name)
 
 
+class TestGetPPAOwnedByPerson(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestGetPPAOwnedByPerson, self).setUp()
+        self.set = getUtility(IArchiveSet)
+
+    def test_person(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        random = self.factory.makePerson()
+        self.assertEqual(archive, self.set.getPPAOwnedByPerson(archive.owner))
+        self.assertIs(None, self.set.getPPAOwnedByPerson(random))
+
+    def test_distribution_and_name(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        self.assertEqual(
+            archive,
+            self.set.getPPAOwnedByPerson(
+                archive.owner, archive.distribution, archive.name))
+        self.assertIs(
+            None,
+            self.set.getPPAOwnedByPerson(
+                archive.owner, archive.distribution, archive.name + u'lol'))
+        self.assertIs(
+            None,
+            self.set.getPPAOwnedByPerson(
+                archive.owner, self.factory.makeDistribution(), archive.name))
+
+    def test_statuses(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        self.assertEqual(
+            archive,
+            self.set.getPPAOwnedByPerson(
+                archive.owner, statuses=(ArchiveStatus.ACTIVE,)))
+        self.assertIs(
+            None,
+            self.set.getPPAOwnedByPerson(
+                archive.owner, statuses=(ArchiveStatus.DELETING,)))
+        with person_logged_in(archive.owner):
+            archive.delete(archive.owner)
+        self.assertEqual(
+            archive,
+            self.set.getPPAOwnedByPerson(
+                archive.owner, statuses=(ArchiveStatus.DELETING,)))
+
+    def test_has_packages(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        self.assertIs(
+            None,
+            self.set.getPPAOwnedByPerson(archive.owner, has_packages=True))
+        self.factory.makeSourcePackagePublishingHistory(archive=archive)
+        self.assertEqual(
+            archive,
+            self.set.getPPAOwnedByPerson(archive.owner, has_packages=True))
+
+
 class TestPPALookup(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer