launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

Jonathan Lange has proposed merging lp:~jml/launchpad/create-commercial-ppa into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jml/launchpad/create-commercial-ppa/+merge/100635

This branch simply changes the createPPA API such that commercial PPAs can be created as well as private PPAs.

While doing this, we changed the login helpers so that one can easily get the logged in user.  This is particularly helpful when testing with celebrities.  We also added some unit tests for the current PPA creation behaviour, which didn't appear to have unit tests before.

We also discovered a logic bug in validatePPA. We have noted the bug in the code, and would be happy to fix it, but consider such a work separate to the work contained herein.

$ bzr di -r submit: | diffstat
Using parent branch file:///var/launchpad/test/,branch=stable/
 registry/interfaces/person.py        |    2 +-
 registry/model/person.py             |   14 ++++++++++----
 soyuz/interfaces/archive.py          |    2 +-
 soyuz/model/archive.py               |   26 ++++++++++++++++++++------
 soyuz/tests/test_person_createppa.py |   34 ++++++++++++++++++++++++++++++++--
 testing/_login.py                    |    5 +++--
 testing/tests/test_login.py          |   14 ++++++++++++++
 7 files changed, 81 insertions(+), 16 deletions(-)


Thanks,
jml & James Westby
-- 
https://code.launchpad.net/~jml/launchpad/create-commercial-ppa/+merge/100635
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/create-commercial-ppa into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-03-26 16:09:50 +0000
+++ lib/lp/registry/interfaces/person.py	2012-04-03 15:54:21 +0000
@@ -1491,7 +1491,7 @@
     @export_factory_operation(Interface, [])  # Really IArchive.
     @operation_for_version("beta")
     def createPPA(name=None, displayname=None, description=None,
-                  private=False):
+                  private=False, commercial=False):
         """Create a PPA.
 
         :param name: A string with the name of the new PPA to create. If

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-03-29 06:02:46 +0000
+++ lib/lp/registry/model/person.py	2012-04-03 15:54:21 +0000
@@ -2965,16 +2965,22 @@
         return getUtility(IArchiveSet).getPPAOwnedByPerson(self, name)
 
     def createPPA(self, name=None, displayname=None, description=None,
-                  private=False):
+                  private=False, commercial=False):
         """See `IPerson`."""
-        errors = Archive.validatePPA(self, name, private)
+        # XXX: We pass through the Person on whom the PPA is being created,
+        # but validatePPA assumes that that Person is also the one creating
+        # the PPA.  This is not true in general, and particularly not for
+        # teams.  Instead, both the acting user and the target of the PPA
+        # creation ought to be passed through.
+        errors = Archive.validatePPA(self, name, private, commercial)
         if errors:
-            raise PPACreationError(errors)
+           raise PPACreationError(errors)
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
         return getUtility(IArchiveSet).new(
             owner=self, purpose=ArchivePurpose.PPA,
             distribution=ubuntu, name=name, displayname=displayname,
-            description=description, private=private)
+            description=description, private=private,
+            commercial=commercial)
 
     def isBugContributor(self, user=None):
         """See `IPerson`."""

=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py	2012-02-28 05:09:39 +0000
+++ lib/lp/soyuz/interfaces/archive.py	2012-04-03 15:54:21 +0000
@@ -1742,7 +1742,7 @@
 
     def new(purpose, owner, name=None, displayname=None, distribution=None,
             description=None, enabled=True, require_virtualized=True,
-            private=False):
+            private=False, commercial=False):
         """Create a new archive.
 
         On named-ppa creation, the signing key for the default PPA for the

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-01-18 14:30:52 +0000
+++ lib/lp/soyuz/model/archive.py	2012-04-03 15:54:21 +0000
@@ -1974,18 +1974,30 @@
         self.enabled_restricted_families = restricted
 
     @classmethod
-    def validatePPA(self, person, proposed_name, private=False):
+    def validatePPA(self, person, proposed_name, private=False,
+                    commercial=False):
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
-        if private:
+        if private or commercial:
             # NOTE: This duplicates the policy in lp/soyuz/configure.zcml
             # which says that one needs 'launchpad.Commercial' permission to
             # set 'private', and the logic in `AdminByCommercialTeamOrAdmins`
             # which determines who is granted launchpad.Commercial
             # permissions.
-            commercial = getUtility(ILaunchpadCelebrities).commercial_admin
+            commercial_admin = getUtility(
+                ILaunchpadCelebrities).commercial_admin
             admin = getUtility(ILaunchpadCelebrities).admin
-            if not person.inTeam(commercial) and not person.inTeam(admin):
-                return '%s is not allowed to make private PPAs' % person.name
+            has_the_power = (
+                person.inTeam(commercial_admin)
+                or person.inTeam(admin))
+            if not has_the_power:
+                if private:
+                    return (
+                        '%s is not allowed to make private PPAs' % person.name)
+                if commercial:
+                    return (
+                        '%s is not allowed to make commercial PPAs'
+                        % person.name)
+                
         if person.is_team and (
             person.subscriptionpolicy in OPEN_TEAM_POLICY):
             return "Open teams cannot have PPAs."
@@ -2120,7 +2132,7 @@
 
     def new(self, purpose, owner, name=None, displayname=None,
             distribution=None, description=None, enabled=True,
-            require_virtualized=True, private=False):
+            require_virtualized=True, private=False, commercial=False):
         """See `IArchiveSet`."""
         if distribution is None:
             distribution = getUtility(ILaunchpadCelebrities).ubuntu
@@ -2197,6 +2209,8 @@
         else:
             new_archive.private = private
 
+        new_archive.commercial = commercial
+
         return new_archive
 
     def __iter__(self):

=== modified file 'lib/lp/soyuz/tests/test_person_createppa.py'
--- lib/lp/soyuz/tests/test_person_createppa.py	2012-01-01 02:58:52 +0000
+++ lib/lp/soyuz/tests/test_person_createppa.py	2012-04-03 15:54:21 +0000
@@ -5,7 +5,12 @@
 
 __metaclass__ = type
 
-from lp.testing import TestCaseWithFactory
+from lp.registry.errors import PPACreationError
+from lp.testing import (
+    celebrity_logged_in,
+    person_logged_in,
+    TestCaseWithFactory,
+    )
 from lp.testing.layers import DatabaseFunctionalLayer
 
 
@@ -14,7 +19,32 @@
 
     layer = DatabaseFunctionalLayer
 
-    def test_create_ppa(self):
+    def test_default_name(self):
         person = self.factory.makePerson()
         ppa = person.createPPA()
         self.assertEqual(ppa.name, 'ppa')
+
+    def test_private(self):
+        with celebrity_logged_in('commercial_admin') as person:
+            ppa = person.createPPA(private=True)
+            self.assertEqual(True, ppa.private)
+
+    def test_private_without_permission(self):
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            self.assertRaises(
+                PPACreationError, person.createPPA, private=True)
+
+    def test_commercial(self):
+        with celebrity_logged_in('commercial_admin') as person:
+            ppa = person.createPPA(commercial=True)
+            self.assertEqual(True, ppa.commercial)
+            
+    def test_commercial_without_permission(self):
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            self.assertRaises(
+                PPACreationError, person.createPPA, commercial=True)
+
+
+        

=== modified file 'lib/lp/testing/_login.py'
--- lib/lp/testing/_login.py	2012-02-02 13:49:33 +0000
+++ lib/lp/testing/_login.py	2012-04-03 15:54:21 +0000
@@ -84,6 +84,7 @@
             raise ValueError("Got team, expected person: %r" % (person,))
     participation = _test_login_impl(participation)
     setupInteractionForPerson(person, participation)
+    return person
 
 
 def login_team(team, participation=None):
@@ -140,9 +141,9 @@
 def _with_login(login_method, identifier):
     """Make a context manager that runs with a particular log in."""
     interaction = queryInteraction()
-    login_method(identifier)
+    person = login_method(identifier)
     try:
-        yield
+        yield person
     finally:
         if interaction is None:
             logout()

=== modified file 'lib/lp/testing/tests/test_login.py'
--- lib/lp/testing/tests/test_login.py	2012-01-03 11:51:30 +0000
+++ lib/lp/testing/tests/test_login.py	2012-04-03 15:54:21 +0000
@@ -240,6 +240,14 @@
             person = self.getLoggedInPerson()
         self.assertTrue(person.inTeam(team))
 
+    def test_team_logged_in_provides_person(self):
+        # person_logged_in makes the logged-in person available through
+        # the context manager.
+        team = self.factory.makeTeam()
+        with person_logged_in(team) as p:
+            person = self.getLoggedInPerson()
+        self.assertEqual(p, person)
+
     def test_celebrity_logged_in(self):
         # celebrity_logged_in runs in a context where a celebrity is logged
         # in.
@@ -248,6 +256,12 @@
             person = self.getLoggedInPerson()
         self.assertTrue(person.inTeam(vcs_imports))
 
+    def test_celebrity_logged_in_provides_person(self):
+        vcs_imports = getUtility(ILaunchpadCelebrities).vcs_imports
+        with celebrity_logged_in('vcs_imports') as p:
+            person = self.getLoggedInPerson()
+        self.assertEqual(p, person)
+
     def test_celebrity_logged_in_restores_person(self):
         # Once outside of the celebrity_logged_in context, the originally
         # logged-in person is re-logged in.