launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/entitle-branch-sharing into lp:launchpad with lp:~sinzui/launchpad/project-branch-permissions as a prerequisite.

Requested reviews:
  William Grant (wgrant)
Related bugs:
  Bug #750871 in Launchpad itself: "only commercial admins can setup branch privacy policies"
  https://bugs.launchpad.net/launchpad/+bug/750871

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/entitle-branch-sharing/+merge/120225

Launchpad must let users reconfigure their projects to use sharing
which enters beta next week.

--------------------------------------------------------------------

RULES

    Pre-implementation: wgrant
    * Remove the guard in setBranchSharingPolicy() that enforces the
      deprecated rule that restricts branch policies to be set by a
      commercial admin.
    * Simplify tests that use a commmercial admin to setup the conditions.


QA

    As a non-commercial admin
    * Visit http://qastaging.launchpad.net/lp-dev-utils/+sharing
    * Verify you can change the branch sharing policy.


LINT

    lib/lp/code/browser/tests/test_branch.py
    lib/lp/code/model/tests/test_branch.py
    lib/lp/code/model/tests/test_branchnamespace.py
    lib/lp/registry/model/product.py


TEST

    ./bin/test -vvc -t transitionToInformationType -t methods_smoketest  lp.code.model.tests.test_branch
    ./bin/test -vvc -t WithInformationType lp.code.model.tests.test_branchnamespace
    ./bin/test -vvc -t EditViewInformationTypes lp.code.browser.tests.test_branch


IMPLEMENTATION

Remove the commerncial admin guard from setBranchSharingPolicy().
    lib/lp/registry/model/product.py

Update tests to use the project owner instead of a commecial admin to
setup the test conditions.
    lib/lp/code/browser/tests/test_branch.py
    lib/lp/code/model/tests/test_branch.py
    lib/lp/code/model/tests/test_branchnamespace.py
-- 
https://code.launchpad.net/~sinzui/launchpad/entitle-branch-sharing/+merge/120225
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2012-08-16 06:24:32 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2012-08-17 19:32:22 +0000
@@ -41,7 +41,6 @@
     )
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.person import (
-    IPersonSet,
     PersonVisibility,
     )
 from lp.services.config import config
@@ -72,7 +71,6 @@
     setupBrowser,
     setupBrowserForUser,
     )
-from lp.testing.sampledata import COMMERCIAL_ADMIN_EMAIL
 from lp.testing.views import create_initialized_view
 
 
@@ -1086,10 +1084,9 @@
         owner = self.factory.makePerson()
         product = self.factory.makeProduct(owner=owner)
         self.factory.makeCommercialSubscription(product=product)
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
-        product.setBranchSharingPolicy(
-            BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY, comadmin)
         with person_logged_in(owner):
+            product.setBranchSharingPolicy(
+                BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY, owner)
             branch = self.factory.makeBranch(
                 product=product, owner=owner,
                 information_type=InformationType.PROPRIETARY)
@@ -1102,10 +1099,9 @@
         owner = self.factory.makePerson()
         product = self.factory.makeProduct(owner=owner)
         self.factory.makeCommercialSubscription(product=product)
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
-        product.setBranchSharingPolicy(
-            BranchSharingPolicy.PROPRIETARY, comadmin)
         with person_logged_in(owner):
+            product.setBranchSharingPolicy(
+                BranchSharingPolicy.PROPRIETARY, owner)
             branch = self.factory.makeBranch(
                 product=product, owner=owner,
                 information_type=InformationType.PROPRIETARY)

=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py	2012-08-17 19:32:22 +0000
+++ lib/lp/code/model/tests/test_branch.py	2012-08-17 19:32:22 +0000
@@ -2601,10 +2601,9 @@
     def test_methods_smoketest(self):
         # Users with launchpad.Moderate can call transitionToInformationType.
         branch = self.factory.makeProductBranch()
-        with celebrity_logged_in('commercial_admin') as admin:
+        with person_logged_in(branch.product.owner):
             branch.product.setBranchSharingPolicy(
-                BranchSharingPolicy.PUBLIC, admin)
-        with person_logged_in(branch.product.owner):
+                BranchSharingPolicy.PUBLIC, branch.product.owner)
             branch.transitionToInformationType(
                 InformationType.PRIVATESECURITY, branch.product.owner)
         self.assertEqual(
@@ -3283,12 +3282,9 @@
         """Test transitionToInformationType() API arguments."""
         product = self.factory.makeProduct()
         self.factory.makeCommercialSubscription(product)
-        with celebrity_logged_in('commercial_admin') as admin:
-            # XXX sinzui 2012-08-16: setBranchSharingPolicy() is guarded
-            # at this moment.
+        with person_logged_in(product.owner):
             product.setBranchSharingPolicy(
-                BranchSharingPolicy.PUBLIC_OR_PROPRIETARY, admin)
-        with person_logged_in(product.owner):
+                BranchSharingPolicy.PUBLIC_OR_PROPRIETARY, product.owner)
             db_branch = self.factory.makeBranch(product=product)
             launchpad = launchpadlib_for('test', db_branch.owner,
                 service_root=self.layer.appserver_root_url('api'))

=== modified file 'lib/lp/code/model/tests/test_branchnamespace.py'
--- lib/lp/code/model/tests/test_branchnamespace.py	2012-08-17 04:48:04 +0000
+++ lib/lp/code/model/tests/test_branchnamespace.py	2012-08-17 19:32:22 +0000
@@ -53,7 +53,6 @@
     )
 from lp.registry.interfaces.distribution import NoSuchDistribution
 from lp.registry.interfaces.person import (
-    IPersonSet,
     NoSuchPerson,
     )
 from lp.registry.interfaces.product import NoSuchProduct
@@ -64,7 +63,6 @@
     TestCaseWithFactory,
     )
 from lp.testing.layers import DatabaseFunctionalLayer
-from lp.testing.sampledata import COMMERCIAL_ADMIN_EMAIL
 
 
 class NamespaceMixin:
@@ -470,8 +468,7 @@
             person = self.factory.makePerson()
         product = self.factory.makeProduct()
         self.factory.makeCommercialSubscription(product=product)
-        comadmin = getUtility(IPersonSet).getByEmail(COMMERCIAL_ADMIN_EMAIL)
-        product.setBranchSharingPolicy(sharing_policy, comadmin)
+        product.setBranchSharingPolicy(sharing_policy, product.owner)
         namespace = ProductNamespace(person, product)
         return namespace
 

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-08-17 05:05:37 +0000
+++ lib/lp/registry/model/product.py	2012-08-17 19:32:22 +0000
@@ -563,10 +563,6 @@
 
     def setBranchSharingPolicy(self, branch_sharing_policy, user):
         """See `IProductPublic`."""
-        if not user or not IPersonRoles(user).in_commercial_admin:
-            raise Unauthorized(
-                "Only commercial admins can configure sharing policies right "
-                "now.")
         if branch_sharing_policy != BranchSharingPolicy.PUBLIC:
             if not self.has_current_commercial_subscription:
                 raise CommercialSubscribersOnly(