launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #814567 in Launchpad itself: "Need extra round-trip to create a private PPA via webservice"
  https://bugs.launchpad.net/launchpad/+bug/814567

For more details, see:
https://code.launchpad.net/~jml/launchpad/create-private-ppa-814567/+merge/69539

This branch makes it possible to create private PPAs over the webservice.

I couldn't find a good way to do the check on creation. Currently the ability to change privacy is controlled by a Zope attribute security check. I don't know how that works wrt object creation, and I *certainly* don't know how that works given that Archive.private is a property with side effects. 

Thus, I duplicated the check that takes place in the security permission.

I also couldn't find a good set of tests that I could repurpose to make sure that setting .private and specifying private=True behaved the same.

Note that setting .private over the API raises 401 Unauthorized, but specifying private to createPPA raises 400 Bad Request.


-- 
https://code.launchpad.net/~jml/launchpad/create-private-ppa-814567/+merge/69539
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/create-private-ppa-814567 into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2011-05-31 03:49:23 +0000
+++ lib/lp/registry/interfaces/person.py	2011-07-27 19:34:00 +0000
@@ -1400,16 +1400,20 @@
     @operation_parameters(
         name=TextLine(required=True, constraint=name_validator),
         displayname=TextLine(required=False),
-        description=TextLine(required=False))
+        description=TextLine(required=False),
+        private=Bool(required=False),
+        )
     @export_factory_operation(Interface, [])  # Really IArchive.
     @operation_for_version("beta")
-    def createPPA(name=None, displayname=None, description=None):
+    def createPPA(name=None, displayname=None, description=None, private=False):
         """Create a PPA.
 
         :param name: A string with the name of the new PPA to create. If
             not specified, defaults to 'ppa'.
         :param displayname: The displayname for the new PPA.
         :param description: The description for the new PPA.
+        :param private: Whether or not to create a private PPA. Defaults to
+            False, which means the PPA will be public.
         :raises: `PPACreationError` if an error is encountered
 
         :return: a PPA `IArchive` record.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-06-16 20:12:00 +0000
+++ lib/lp/registry/model/person.py	2011-07-27 19:34:00 +0000
@@ -2739,17 +2739,17 @@
         """See `IPerson`."""
         return getUtility(IArchiveSet).getPPAOwnedByPerson(self, name)
 
-    def createPPA(self, name=None, displayname=None, description=None):
+    def createPPA(self, name=None, displayname=None, description=None,
+                  private=False):
         """See `IPerson`."""
-        errors = Archive.validatePPA(self, name)
+        errors = Archive.validatePPA(self, name, private)
         if errors:
             raise PPACreationError(errors)
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
-        ppa = getUtility(IArchiveSet).new(
+        return getUtility(IArchiveSet).new(
             owner=self, purpose=ArchivePurpose.PPA,
             distribution=ubuntu, name=name, displayname=displayname,
             description=description)
-        return ppa
 
     def isBugContributor(self, user=None):
         """See `IPerson`."""

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2011-07-22 13:28:35 +0000
+++ lib/lp/soyuz/model/archive.py	2011-07-27 19:34:00 +0000
@@ -1947,8 +1947,12 @@
         self.enabled_restricted_families = restricted
 
     @classmethod
-    def validatePPA(self, person, proposed_name):
+    def validatePPA(self, person, proposed_name, private=False):
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+        if private:
+            commercial = getUtility(ILaunchpadCelebrities).commercial_admin
+            if not person.inTeam(commercial):
+                return '%s is not allowed to make private PPAs' % (person.name,)
         if person.isTeam() and (
             person.subscriptionpolicy in OPEN_TEAM_POLICY):
             return "Open teams cannot have PPAs."

=== modified file 'lib/lp/soyuz/stories/webservice/xx-person-createppa.txt'
--- lib/lp/soyuz/stories/webservice/xx-person-createppa.txt	2011-02-13 22:54:48 +0000
+++ lib/lp/soyuz/stories/webservice/xx-person-createppa.txt	2011-07-27 19:34:00 +0000
@@ -30,3 +30,31 @@
   Status: 400 Bad Request
   ...
   A PPA cannot have the same name as its distribution.
+
+== Creating private PPAs ==
+
+Our PPA owner now has a single PPA.
+
+  >>> print_self_link_of_entries(webservice.get(
+  ...     ppa_owner['ppas_collection_link']).jsonBody())
+  http://api.launchpad.dev/beta/~.../+archive/ppa
+
+They aren't a commercial admin, so they cannot create private PPAs.
+
+  >>> print ppa_owner_webservice.named_post(
+  ...     ppa_owner['self_link'], 'createPPA', {}, name='whatever',
+  ...     displayname='My secret new PPA', description='Secretness!',
+  ...     private=True,
+  ...     )
+  HTTP/1.1 400 Bad Request
+  Status: 400
+  ...
+  ... is not allowed to make private PPAs
+
+After attempting and failing to create a private PPA, they still have the same
+single PPA they had at the beginning:
+
+  >>> print_self_link_of_entries(webservice.get(
+  ...     ppa_owner['ppas_collection_link']).jsonBody())
+  http://api.launchpad.dev/beta/~.../+archive/ppa
+

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2011-07-22 13:31:35 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2011-07-27 19:34:00 +0000
@@ -1624,7 +1624,7 @@
             set(archive.getComponentsForUploader(person)))
 
 
-class TestvalidatePPA(TestCaseWithFactory):
+class TestValidatePPA(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer
 
@@ -1639,6 +1639,23 @@
             'A PPA cannot have the same name as its distribution.',
             Archive.validatePPA(ppa_owner, 'ubuntu'))
 
+    def test_private_ppa_non_commercial_admin(self):
+        ppa_owner = self.factory.makePerson()
+        self.assertEqual(
+            '%s is not allowed to make private PPAs' % (ppa_owner.name,),
+            Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
+                                private=True))
+
+    def test_private_ppa_commercial_admin(self):
+        ppa_owner = self.factory.makePerson()
+        with celebrity_logged_in('admin'):
+            comm = getUtility(ILaunchpadCelebrities).commercial_admin
+            comm.addMember(ppa_owner, comm.teamowner)
+        self.assertIs(
+            None,
+            Archive.validatePPA(ppa_owner, self.factory.getUniqueString(),
+                                private=True))
+
     def test_two_ppas(self):
         ppa = self.factory.makeArchive(name='ppa')
         self.assertEqual("You already have a PPA named 'ppa'.",