Merge lp:~stevenk/launchpad/shift-ap-creation into lp:launchpad

[Steve Kowalik]

No longer create AccessPolicies in IProductSet.createProduct(), do it in IProduct.setB{ranch,ug}SharingPolicy().

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 deleted a test that no longer makes sense, since creating a product itself won't create APs.

[William Grant (wgrant)]

+ def _check_and_set_sharing_policy(self, var, enum, kind, allowed_types):

It doesn't actually set the sharing policy. It just prepares for it.

27 + if var != enum.PUBLIC:
28 + if not self.has_current_commercial_subscription:

This can be a single if statement now.

208 + return self.makeProduct(skip_sharing_policy=True, **kwargs)

Privacy relies on the APs existing, so you can't set skip_sharing_policy unconditionally here. I think it'd be better to have the one or two tests that need it remove the APs themselves, rather than preventing them from being created in the first place.

review: Approve (code)

=== 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 23:47: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,26 @@
         self.checkPrivateBugsTransitionAllowed(private_bugs, user)
         self.private_bugs = private_bugs
 
+    def _prepare_to_set_sharing_policy(self, var, enum, kind, allowed_types):
+        if var != enum.PUBLIC and 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._prepare_to_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._prepare_to_set_sharing_policy(
+            bug_sharing_policy, BugSharingPolicy, 'bugs', POLICY_ALLOWED_TYPES)
         self.bug_sharing_policy = bug_sharing_policy
 
     def getAllowedBugInformationTypes(self):
@@ -1574,10 +1576,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 23:47:20 +0000
@@ -342,14 +342,6 @@
         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_grants_maintainer_access(self):
         # Creating a new product creates an access grant for the maintainer
         # for all default policies.
@@ -364,8 +356,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 +366,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 +383,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-30 11:48:26 +0000
+++ lib/lp/testing/factory.py	2012-08-30 23:47:20 +0000
@@ -1017,6 +1017,9 @@
         # migrated.
         # XXX This method can be removed when branch visibility policy dies.
         product = self.makeProduct(**kwargs)
+        # Since createProduct() doesn't create PRIVATESECURITY/USERDATA.
+        removeSecurityProxy(product)._ensurePolicies([
+            InformationType.PRIVATESECURITY, InformationType.USERDATA])
         removeSecurityProxy(product).bug_sharing_policy = None
         removeSecurityProxy(product).branch_sharing_policy = None
         return product