launchpad-reviewers team mailing list archive

[Jonathan Lange <jml@xxxxxxxxxxxxx>]

Jonathan Lange has proposed merging lp:~jml/launchpad/p3a-commercial-subscription into lp:launchpad with lp:~jml/launchpad/p3a-private-team as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jml/launchpad/p3a-commercial-subscription/+merge/109602

We want to be able to create private PPAs without having commercial admin privileges. After an implementation discussion with sinzui (summarized https://code.launchpad.net/~jml/launchpad/db-distro-level-ppa-privacy/+merge/109336/comments/234987), we decided that folk with commercial subscriptions should be allowed to create private PPAs on public teams. This branch enables that change, via the createPPA API call.

What it does *not* do is allow people with commercial subscription to change privacy of existing PPAs. This is because I can't figure out how to separate the 'private' attribute from the rest of the attributes requiring 'launchpad.Commercial' and because I don't want to grant 'launchpad.Commercial' privileges to anyone with a commercial subscription, as the permission seems to grant low-level admin privileges than rather than things one would expect a any paying user to be able to do.

Also, this branch does not update the web UI.
-- 
https://code.launchpad.net/~jml/launchpad/p3a-commercial-subscription/+merge/109602
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jml/launchpad/p3a-commercial-subscription into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/test_team.py'
--- lib/lp/registry/browser/tests/test_team.py	2012-06-11 10:37:38 +0000
+++ lib/lp/registry/browser/tests/test_team.py	2012-06-11 10:37:38 +0000
@@ -501,8 +501,7 @@
         personset = getUtility(IPersonSet)
         team = self.factory.makeTeam(
             subscription_policy=TeamSubscriptionPolicy.MODERATED)
-        product = self.factory.makeProduct(owner=team)
-        self.factory.makeCommercialSubscription(product)
+        self.factory.grantCommercialSubscription(team)
         with person_logged_in(team.teamowner):
             view = create_initialized_view(
                 personset, name=self.view_name, principal=team.teamowner)
@@ -514,8 +513,7 @@
         personset = getUtility(IPersonSet)
         team = self.factory.makeTeam(
             subscription_policy=TeamSubscriptionPolicy.MODERATED)
-        product = self.factory.makeProduct(owner=team)
-        self.factory.makeCommercialSubscription(product)
+        self.factory.grantCommercialSubscription(team)
         team_name = self.factory.getUniqueString()
         form = {
             'field.name': team_name,
@@ -550,8 +548,7 @@
         team = self.factory.makeTeam(
             subscription_policy=TeamSubscriptionPolicy.RESTRICTED,
             visibility=PersonVisibility.PRIVATE, owner=owner)
-        product = self.factory.makeProduct(owner=owner)
-        self.factory.makeCommercialSubscription(product)
+        self.factory.grantCommercialSubscription(team)
         with person_logged_in(owner):
             url = canonical_url(team)
         browser = self.getUserBrowser(url, user=owner)

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2012-06-11 10:37:38 +0000
+++ lib/lp/soyuz/model/archive.py	2012-06-11 10:37:38 +0000
@@ -66,10 +66,7 @@
     validate_person,
     )
 from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.registry.interfaces.role import (
-    IHasOwner,
-    IPersonRoles,
-    )
+from lp.registry.interfaces.role import IHasOwner
 from lp.registry.interfaces.sourcepackagename import ISourcePackageNameSet
 from lp.registry.model.sourcepackagename import SourcePackageName
 from lp.registry.model.teammembership import TeamParticipation
@@ -2065,13 +2062,13 @@
     creator = getUtility(ILaunchBag).user
     ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
     if private:
-        # 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.
-        role = IPersonRoles(creator)
-        if not (owner.private or role.in_admin or role.in_commercial_admin):
+        # 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. The
+        # difference is that here we grant ability to set 'private' to people
+        # with a commercial subscription.
+        if not (owner.private or creator.checkAllowVisibility()):
             return '%s is not allowed to make private PPAs' % creator.name
     elif owner.private:
         return 'Private teams may not have public archives.'

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2012-06-11 10:37:38 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2012-06-11 10:37:38 +0000
@@ -1666,7 +1666,7 @@
             'A PPA cannot have the same name as its distribution.',
             validate_ppa(ppa_owner, 'ubuntu'))
 
-    def test_private_ppa_non_commercial_admin(self):
+    def test_private_ppa_standard_user(self):
         ppa_owner = self.factory.makePerson()
         with person_logged_in(ppa_owner):
             errors = validate_ppa(
@@ -1675,6 +1675,13 @@
             '%s is not allowed to make private PPAs' % (ppa_owner.name,),
             errors)
 
+    def test_private_ppa_commercial_subscription(self):
+        owner = self.factory.makePerson()
+        self.factory.grantCommercialSubscription(owner)
+        with person_logged_in(owner):
+            errors = validate_ppa(owner, 'ppa', private=True)
+        self.assertIsNone(errors)
+
     def test_private_ppa_commercial_admin(self):
         ppa_owner = self.factory.makePerson()
         with celebrity_logged_in('admin'):

=== modified file 'lib/lp/soyuz/tests/test_archive_privacy.py'
--- lib/lp/soyuz/tests/test_archive_privacy.py	2012-05-25 22:14:21 +0000
+++ lib/lp/soyuz/tests/test_archive_privacy.py	2012-06-11 10:37:38 +0000
@@ -8,6 +8,7 @@
 from lp.soyuz.interfaces.archive import CannotSwitchPrivacy
 from lp.soyuz.tests.test_publishing import SoyuzTestPublisher
 from lp.testing import (
+    celebrity_logged_in,
     person_logged_in,
     TestCaseWithFactory,
     )
@@ -37,24 +38,46 @@
         with person_logged_in(subscriber):
             self.assertEqual(ppa.description, "Foo")
 
-
-class TestArchivePrivacySwitching(TestCaseWithFactory):
+    def test_owner_changing_privacy(self):
+        ppa = self.factory.makeArchive()
+        with person_logged_in(ppa.owner):
+            self.assertRaises(Unauthorized, setattr, ppa, 'private', True)
+
+    def test_owner_with_commercial_subscription_changing_privacy(self):
+        ppa = self.factory.makeArchive()
+        self.factory.grantCommercialSubscription(ppa.owner)
+        with person_logged_in(ppa.owner):
+            # XXX: jml 2012-06-11: We actually want this to be allowed, but I
+            # can't think of any way to grant this without also granting other
+            # attributes that have launchpad.Commercial.
+            self.assertRaises(Unauthorized, setattr, ppa, 'private', True)
+
+    def test_admin_changing_privacy(self):
+        ppa = self.factory.makeArchive()
+        with celebrity_logged_in('admin'):
+            ppa.private = True
+        self.assertEqual(True, ppa.private)
+
+    def test_commercial_admin_changing_privacy(self):
+        ppa = self.factory.makeArchive()
+        with celebrity_logged_in('commercial_admin'):
+            ppa.private = True
+        self.assertEqual(True, ppa.private)
+
+
+class TestPrivacySwitching(TestCaseWithFactory):
 
     layer = LaunchpadZopelessLayer
 
-    def set_ppa_privacy(self, ppa, private):
-        """Helper method to privatise a ppa."""
-        ppa.private = private
-
     def test_switch_privacy_no_pubs_succeeds(self):
         # Changing the privacy is fine if there are no publishing
         # records.
         public_ppa = self.factory.makeArchive()
-        self.set_ppa_privacy(public_ppa, private=True)
+        public_ppa.private = True
         self.assertTrue(public_ppa.private)
 
         private_ppa = self.factory.makeArchive(private=True)
-        self.set_ppa_privacy(private_ppa, private=False)
+        private_ppa.private = False
         self.assertFalse(private_ppa.private)
 
     def test_switch_privacy_with_pubs_fails(self):
@@ -69,19 +92,17 @@
         publisher.getPubSource(archive=private_ppa)
 
         self.assertRaises(
-            CannotSwitchPrivacy,
-            self.set_ppa_privacy, public_ppa, private=True)
+            CannotSwitchPrivacy, setattr, public_ppa, 'private', True)
 
         self.assertRaises(
-            CannotSwitchPrivacy,
-            self.set_ppa_privacy, private_ppa, private=False)
+            CannotSwitchPrivacy, setattr, private_ppa, 'private', False)
 
     def test_buildd_secret_was_generated(self):
         public_ppa = self.factory.makeArchive()
-        self.set_ppa_privacy(public_ppa, private=True)
+        public_ppa.private = True
         self.assertNotEqual(public_ppa.buildd_secret, None)
 
     def test_discard_buildd_was_discarded(self):
         private_ppa = self.factory.makeArchive(private=True)
-        self.set_ppa_privacy(private_ppa, private=False)
+        private_ppa.private = False
         self.assertEqual(private_ppa.buildd_secret, None)

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-06-08 06:01:50 +0000
+++ lib/lp/testing/factory.py	2012-06-11 10:37:38 +0000
@@ -4338,6 +4338,12 @@
             sales_system_id='new',
             whiteboard='')
 
+    def grantCommercialSubscription(self, person, months=12):
+        """Give 'person' a commercial subscription."""
+        product = self.makeProduct(owner=person)
+        product.redeemSubscriptionVoucher(
+            self.getUniqueString(), person, person, months)
+
 
 # Some factory methods return simple Python types. We don't add
 # security wrappers for them, as well as for objects created by