launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/shift-ap-creation into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1043366 in Launchpad itself: "Create Private and Private Security policies if sharing policy allows them"
  https://bugs.launchpad.net/launchpad/+bug/1043366

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/shift-ap-creation/+merge/121985

Currently, when a new product is created, createProduct() will create PRIVATESECURITY and USERDATA AccessPolicies. This does not make sense if the product has its branch and bug sharing policies set to PROPRIETARY since the two that were created will just be cleaned up by a garbo job.

As such, stop creating them in createProduct(), and ensure they exist in setB{ranch,ug}SharingPolicy(). I have also refactored those two methods so that all of the heavy lifting is done by a generic method.

I have also given a new argument to createProduct(), 'skip_sharing_policy', which will skip all of that setup for legacy products, rather than just setting the sharing policies to None. This had the neat side effect of pulling factory.makeLegacyProduct() down to one line.
-- 
https://code.launchpad.net/~stevenk/launchpad/shift-ap-creation/+merge/121985
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/shift-ap-creation into lp:launchpad.

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-08-29 03:19:38 +0000
+++ lib/lp/registry/model/product.py	2012-08-30 05:08:20 +0000
@@ -107,6 +107,9 @@
     )
 from lp.code.enums import BranchType
 from lp.code.interfaces.branch import DEFAULT_BRANCH_STATUS_IN_LISTING
+from lp.code.model.branchnamespace import (
+    POLICY_ALLOWED_TYPES as BRANCH_POLICY_ALLOWED_TYPES,
+    )
 from lp.code.model.branchvisibilitypolicy import BranchVisibilityPolicyMixin
 from lp.code.model.hasbranches import (
     HasBranchesMixin,
@@ -119,6 +122,7 @@
     BranchSharingPolicy,
     BugSharingPolicy,
     InformationType,
+    PRIVATE_INFORMATION_TYPES,
     )
 from lp.registry.errors import CommercialSubscribersOnly
 from lp.registry.interfaces.accesspolicy import (
@@ -560,28 +564,27 @@
         self.checkPrivateBugsTransitionAllowed(private_bugs, user)
         self.private_bugs = private_bugs
 
+    def _check_and_set_sharing_policy(self, var, enum, kind, allowed_types):
+        if var != enum.PUBLIC:
+            if not self.has_current_commercial_subscription:
+                raise CommercialSubscribersOnly(
+                    "A current commercial subscription is required to use "
+                    "proprietary %s." % kind)
+        required_policies = set(allowed_types[var]).intersection(
+            set(PRIVATE_INFORMATION_TYPES))
+        self._ensurePolicies(required_policies)
+
     def setBranchSharingPolicy(self, branch_sharing_policy):
         """See `IProductEditRestricted`."""
-        if branch_sharing_policy != BranchSharingPolicy.PUBLIC:
-            if not self.has_current_commercial_subscription:
-                raise CommercialSubscribersOnly(
-                    "A current commercial subscription is required to use "
-                    "proprietary branches.")
-            required_policies = [InformationType.PROPRIETARY]
-            if (branch_sharing_policy ==
-                BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY):
-                required_policies.append(InformationType.EMBARGOED)
-            self._ensurePolicies(required_policies)
+        self._check_and_set_sharing_policy(
+            branch_sharing_policy, BranchSharingPolicy, 'branches',
+            BRANCH_POLICY_ALLOWED_TYPES)
         self.branch_sharing_policy = branch_sharing_policy
 
     def setBugSharingPolicy(self, bug_sharing_policy):
         """See `IProductEditRestricted`."""
-        if bug_sharing_policy != BugSharingPolicy.PUBLIC:
-            if not self.has_current_commercial_subscription:
-                raise CommercialSubscribersOnly(
-                    "A current commercial subscription is required to use "
-                    "proprietary bugs.")
-            self._ensurePolicies([InformationType.PROPRIETARY])
+        self._check_and_set_sharing_policy(
+            bug_sharing_policy, BugSharingPolicy, 'bugs', POLICY_ALLOWED_TYPES)
         self.bug_sharing_policy = bug_sharing_policy
 
     def getAllowedBugInformationTypes(self):
@@ -1535,7 +1538,7 @@
                       sourceforgeproject=None, programminglang=None,
                       project_reviewed=False, mugshot=None, logo=None,
                       icon=None, licenses=None, license_info=None,
-                      registrant=None):
+                      registrant=None, skip_sharing_policy=False):
         """See `IProductSet`."""
         if registrant is None:
             registrant = owner
@@ -1552,20 +1555,21 @@
             project_reviewed=project_reviewed,
             icon=icon, logo=logo, mugshot=mugshot, license_info=license_info)
 
-        # Set up the sharing policies and product licence.
-        bug_sharing_policy_to_use = BugSharingPolicy.PUBLIC
-        branch_sharing_policy_to_use = BranchSharingPolicy.PUBLIC
-        if len(licenses) > 0:
-            product._setLicenses(licenses, reset_project_reviewed=False)
-            # By default, new non-proprietary projects use public bugs and
-            # branches. Proprietary projects are given a complimentary 30 day
-            # commercial subscription and so may use proprietary sharing
-            # policies.
-            if License.OTHER_PROPRIETARY in licenses:
-                bug_sharing_policy_to_use = BugSharingPolicy.PROPRIETARY
-                branch_sharing_policy_to_use = BranchSharingPolicy.PROPRIETARY
-        product.setBugSharingPolicy(bug_sharing_policy_to_use)
-        product.setBranchSharingPolicy(branch_sharing_policy_to_use)
+        if not skip_sharing_policy:
+            # Set up the sharing policies and product licence.
+            bug_sharing_policy = BugSharingPolicy.PUBLIC
+            branch_sharing_policy = BranchSharingPolicy.PUBLIC
+            if len(licenses) > 0:
+                product._setLicenses(licenses, reset_project_reviewed=False)
+                # By default, new non-proprietary projects use public bugs and
+                # branches. Proprietary projects are given a complimentary 30
+                # day commercial subscription and so may use proprietary
+                # sharing policies.
+                if License.OTHER_PROPRIETARY in licenses:
+                    bug_sharing_policy = BugSharingPolicy.PROPRIETARY
+                    branch_sharing_policy = BranchSharingPolicy.PROPRIETARY
+            product.setBugSharingPolicy(bug_sharing_policy)
+            product.setBranchSharingPolicy(branch_sharing_policy)
 
         # Create a default trunk series and set it as the development focus
         trunk = product.newSeries(
@@ -1574,10 +1578,6 @@
              'rather than a stable release branch. This is sometimes also '
              'called MAIN or HEAD.'))
         product.development_focus = trunk
-
-        # Add default AccessPolicies.
-        product._ensurePolicies((InformationType.USERDATA,
-                InformationType.PRIVATESECURITY))
         return product
 
     def forReview(self, search_text=None, active=None,

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-08-29 03:19:38 +0000
+++ lib/lp/registry/tests/test_product.py	2012-08-30 05:08:20 +0000
@@ -342,13 +342,11 @@
         product.setPrivateBugs(True, bug_supervisor)
         self.assertTrue(product.private_bugs)
 
-    def test_product_creation_creates_accesspolicies(self):
-        # Creating a new product also creates AccessPolicies for it.
-        product = self.factory.makeProduct()
-        ap = getUtility(IAccessPolicySource).findByPillar((product,))
-        expected = [
-            InformationType.USERDATA, InformationType.PRIVATESECURITY]
-        self.assertContentEqual(expected, [policy.type for policy in ap])
+    def test_product_creation_does_not_create_accesspolicies(self):
+        # Creating a new product does not create AccessPolicies for it.
+        product = self.factory.makeLegacyProduct()
+        ap = getUtility(IAccessPolicySource).findByPillar([product])
+        self.assertEqual([], list(ap))
 
     def test_product_creation_grants_maintainer_access(self):
         # Creating a new product creates an access grant for the maintainer
@@ -364,8 +362,8 @@
         self.assertEqual(expected_grantess, grantees)
 
     def test_open_product_creation_sharing_policies(self):
-        # Creating a new open (non-proprietary) product sets the bug and branch
-        # sharing polices to public.
+        # Creating a new open (non-proprietary) product sets the bug and
+        # branch sharing polices to public, and creates policies if required.
         owner = self.factory.makePerson()
         with person_logged_in(owner):
             product = getUtility(IProductSet).createProduct(
@@ -374,6 +372,10 @@
         self.assertEqual(BugSharingPolicy.PUBLIC, product.bug_sharing_policy)
         self.assertEqual(
             BranchSharingPolicy.PUBLIC, product.branch_sharing_policy)
+        aps = getUtility(IAccessPolicySource).findByPillar([product])
+        expected = [
+            InformationType.USERDATA, InformationType.PRIVATESECURITY]
+        self.assertContentEqual(expected, [policy.type for policy in aps])
 
     def test_proprietary_product_creation_sharing_policies(self):
         # Creating a new proprietary product sets the bug and branch sharing
@@ -387,6 +389,9 @@
             BugSharingPolicy.PROPRIETARY, product.bug_sharing_policy)
         self.assertEqual(
             BranchSharingPolicy.PROPRIETARY, product.branch_sharing_policy)
+        aps = getUtility(IAccessPolicySource).findByPillar([product])
+        expected = [InformationType.PROPRIETARY]
+        self.assertContentEqual(expected, [policy.type for policy in aps])
 
 
 class TestProductBugInformationTypes(TestCaseWithFactory):

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-08-29 03:19:38 +0000
+++ lib/lp/testing/factory.py	2012-08-30 05:08:20 +0000
@@ -956,7 +956,7 @@
         title=None, summary=None, official_malone=None,
         translations_usage=None, bug_supervisor=None, private_bugs=False,
         driver=None, icon=None, bug_sharing_policy=None,
-        branch_sharing_policy=None):
+        branch_sharing_policy=None, skip_sharing_policy=False):
         """Create and return a new, arbitrary Product."""
         if owner is None:
             owner = self.makePerson()
@@ -985,7 +985,8 @@
                 licenses=licenses,
                 project=project,
                 registrant=registrant,
-                icon=icon)
+                icon=icon,
+                skip_sharing_policy=skip_sharing_policy)
         naked_product = removeSecurityProxy(product)
         if official_malone is not None:
             naked_product.official_malone = official_malone
@@ -1015,10 +1016,7 @@
         # but we need to test for existing products which have not yet been
         # migrated.
         # XXX This method can be removed when branch visibility policy dies.
-        product = self.makeProduct(**kwargs)
-        removeSecurityProxy(product).bug_sharing_policy = None
-        removeSecurityProxy(product).branch_sharing_policy = None
-        return product
+        return self.makeProduct(skip_sharing_policy=True, **kwargs)
 
     def makeProductSeries(self, product=None, name=None, owner=None,
                           summary=None, date_created=None, branch=None):