launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/getInformationTypes-respect-policies-1043368 into lp:launchpad with lp:~stevenk/launchpad/shift-ap-creation as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1043368 in Launchpad itself: "SharingService.getInformationTypes must respect sharing policies"
  https://bugs.launchpad.net/launchpad/+bug/1043368

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/getInformationTypes-respect-policies-1043368/+merge/121998

== Implementation ==

Sharing service uses a getInformationTypes() method to determine which information types to show for selection in the sharing picker. This method was written early before the work to properly implement bug and branch sharing policies and it doesn't return the correct information. So this branch refactors the implementation to use the new infrastructure.

IProduct and IDistribution get new getAllowedBranchInformationTypes() methods analogous to the existing getAllowedBugInformationTypes() method. The allowed types becomes the union of the allowed private types.

The implementation uses POLICY_ALLOWED_TYPES defined separately for bugs and branches in different modules. The imports are aliased for now, but a future branch will rename them.

A followup branch is required to correctly refresh the data displayed on the +sharing page when the bug/branch sharing policies are changed.

== Tests ==

Add new tests for the new getAllowedBranchInformationTypes() methods and updated tests for the getInformationTypes() method.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/interfaces/distribution.py
  lib/lp/registry/interfaces/product.py
  lib/lp/registry/interfaces/sharingservice.py
  lib/lp/registry/model/distribution.py
  lib/lp/registry/model/product.py
  lib/lp/registry/services/sharingservice.py
  lib/lp/registry/services/tests/test_sharingservice.py
  lib/lp/registry/tests/test_distribution.py
  lib/lp/registry/tests/test_product.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/getInformationTypes-respect-policies-1043368/+merge/121998
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/getInformationTypes-respect-policies-1043368 into lp:launchpad.

=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py	2012-08-21 00:34:02 +0000
+++ lib/lp/registry/interfaces/distribution.py	2012-08-30 07:20:24 +0000
@@ -628,6 +628,12 @@
         :return: The `InformationType`.
         """
 
+    def getAllowedBranchInformationTypes():
+        """Get the information types that a branch in this distro can have.
+
+        :return: A sequence of `InformationType`s.
+        """
+
     def userCanEdit(user):
         """Can the user edit this distribution?"""
 

=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py	2012-08-23 02:52:31 +0000
+++ lib/lp/registry/interfaces/product.py	2012-08-30 07:20:24 +0000
@@ -797,6 +797,12 @@
         :return: The `InformationType`.
         """
 
+    def getAllowedBranchInformationTypes():
+        """Get the information types that a branch in this project can have.
+
+        :return: A sequence of `InformationType`s.
+        """
+
     def getVersionSortedSeries(statuses=None, filter_statuses=None):
         """Return all the series sorted by the name field as a version.
 

=== modified file 'lib/lp/registry/interfaces/sharingservice.py'
--- lib/lp/registry/interfaces/sharingservice.py	2012-08-14 04:48:36 +0000
+++ lib/lp/registry/interfaces/sharingservice.py	2012-08-30 07:20:24 +0000
@@ -112,7 +112,13 @@
         """
 
     def getInformationTypes(pillar):
-        """Return the allowed information types for the given pillar."""
+        """Return the allowed information types for the given pillar.
+
+        The allowed information types are those for which bugs and branches
+        may be created. This does not mean that there will necessarily be bugs
+        and branches of these types; the result is used to populate the allowed
+        choices in the grantee sharing pillar and other similar things.
+        """
 
     def getBugSharingPolicies(pillar):
         """Return the allowed bug sharing policies for the given pillar."""

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2012-08-21 04:28:11 +0000
+++ lib/lp/registry/model/distribution.py	2012-08-30 07:20:24 +0000
@@ -91,6 +91,7 @@
     IFindOfficialBranchLinks,
     )
 from lp.registry.enums import (
+    FREE_INFORMATION_TYPES,
     InformationType,
     PRIVATE_INFORMATION_TYPES,
     PUBLIC_INFORMATION_TYPES,
@@ -1574,15 +1575,16 @@
 
     def getAllowedBugInformationTypes(self):
         """See `IDistribution.`"""
-        types = set(InformationType.items)
-        types.discard(InformationType.PROPRIETARY)
-        types.discard(InformationType.EMBARGOED)
-        return types
+        return FREE_INFORMATION_TYPES
 
     def getDefaultBugInformationType(self):
         """See `IDistribution.`"""
         return InformationType.PUBLIC
 
+    def getAllowedBranchInformationTypes(self):
+        """See `IDistribution.`"""
+        return FREE_INFORMATION_TYPES
+
     def userCanEdit(self, user):
         """See `IDistribution`."""
         if user is None:

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-08-30 07:20:24 +0000
+++ lib/lp/registry/model/product.py	2012-08-30 07:20:24 +0000
@@ -91,7 +91,7 @@
 from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
 from lp.bugs.interfaces.bugsupervisor import IHasBugSupervisor
 from lp.bugs.interfaces.bugtarget import (
-    POLICY_ALLOWED_TYPES,
+    POLICY_ALLOWED_TYPES as BUG_POLICY_ALLOWED_TYPES,
     POLICY_DEFAULT_TYPES,
     )
 from lp.bugs.interfaces.bugtaskfilter import OrderedBugTask
@@ -121,6 +121,7 @@
 from lp.registry.enums import (
     BranchSharingPolicy,
     BugSharingPolicy,
+    FREE_PRIVATE_INFORMATION_TYPES,
     InformationType,
     PRIVATE_INFORMATION_TYPES,
     )
@@ -583,18 +584,15 @@
     def setBugSharingPolicy(self, bug_sharing_policy):
         """See `IProductEditRestricted`."""
         self._prepare_to_set_sharing_policy(
-            bug_sharing_policy, BugSharingPolicy, 'bugs', POLICY_ALLOWED_TYPES)
+            bug_sharing_policy, BugSharingPolicy, 'bugs',
+            BUG_POLICY_ALLOWED_TYPES)
         self.bug_sharing_policy = bug_sharing_policy
 
     def getAllowedBugInformationTypes(self):
         """See `IProduct.`"""
         if self.bug_sharing_policy is not None:
-            return POLICY_ALLOWED_TYPES[self.bug_sharing_policy]
-
-        types = set(InformationType.items)
-        types.discard(InformationType.PROPRIETARY)
-        types.discard(InformationType.EMBARGOED)
-        return types
+            return BUG_POLICY_ALLOWED_TYPES[self.bug_sharing_policy]
+        return FREE_PRIVATE_INFORMATION_TYPES
 
     def getDefaultBugInformationType(self):
         """See `IDistribution.`"""
@@ -605,6 +603,12 @@
         else:
             return InformationType.PUBLIC
 
+    def getAllowedBranchInformationTypes(self):
+        """See `IProduct.`"""
+        if self.branch_sharing_policy is not None:
+            return BRANCH_POLICY_ALLOWED_TYPES[self.branch_sharing_policy]
+        return FREE_PRIVATE_INFORMATION_TYPES
+
     def _ensurePolicies(self, information_types):
         # Ensure that the product has access policies for the specified
         # information types.

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-08-28 23:25:43 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-08-30 07:20:24 +0000
@@ -32,7 +32,7 @@
 from lp.registry.enums import (
     BranchSharingPolicy,
     BugSharingPolicy,
-    InformationType,
+    PRIVATE_INFORMATION_TYPES,
     SharingPermission,
     )
 from lp.registry.interfaces.accesspolicy import (
@@ -255,16 +255,11 @@
 
     def getInformationTypes(self, pillar):
         """See `ISharingService`."""
-        allowed_types = [
-            InformationType.PRIVATESECURITY,
-            InformationType.USERDATA]
-        # Products with current commercial subscriptions are also allowed to
-        # have a PROPRIETARY information type.
-        if (IProduct.providedBy(pillar) and
-                pillar.has_current_commercial_subscription):
-            allowed_types.append(InformationType.PROPRIETARY)
-
-        return self._makeEnumData(allowed_types)
+        allowed_types = set(pillar.getAllowedBugInformationTypes()).union(
+            pillar.getAllowedBranchInformationTypes())
+        allowed_private_types = allowed_types.intersection(
+            PRIVATE_INFORMATION_TYPES)
+        return self._makeEnumData(allowed_private_types)
 
     def getBranchSharingPolicies(self, pillar):
         """See `ISharingService`."""

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-08-29 06:12:08 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-08-30 07:20:24 +0000
@@ -139,13 +139,20 @@
             [InformationType.PRIVATESECURITY, InformationType.USERDATA])
 
     def test_getInformationTypes_commercial_product(self):
-        product = self.factory.makeProduct()
-        self.factory.makeCommercialSubscription(product)
-        self._assert_getInformationTypes(
-            product,
-            [InformationType.PRIVATESECURITY,
-             InformationType.USERDATA,
-             InformationType.PROPRIETARY])
+        product = self.factory.makeProduct(
+            branch_sharing_policy=BranchSharingPolicy.PROPRIETARY,
+            bug_sharing_policy=BugSharingPolicy.PROPRIETARY)
+        self._assert_getInformationTypes(
+            product,
+            [InformationType.PROPRIETARY])
+
+    def test_getInformationTypes_product_with_embargoed(self):
+        product = self.factory.makeProduct(
+            branch_sharing_policy=BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY,
+            bug_sharing_policy=BugSharingPolicy.PROPRIETARY)
+        self._assert_getInformationTypes(
+            product,
+            [InformationType.PROPRIETARY, InformationType.EMBARGOED])
 
     def test_getInformationTypes_distro(self):
         distro = self.factory.makeDistribution()

=== modified file 'lib/lp/registry/tests/test_distribution.py'
--- lib/lp/registry/tests/test_distribution.py	2012-08-21 04:04:47 +0000
+++ lib/lp/registry/tests/test_distribution.py	2012-08-30 07:20:24 +0000
@@ -276,6 +276,14 @@
             InformationType.PUBLIC,
             self.factory.makeDistribution().getDefaultBugInformationType())
 
+    def test_getAllowedBranchInformationTypes(self):
+        # All distros currently support just the non-proprietary
+        # information types.
+        self.assertContentEqual(
+            [InformationType.PUBLIC, InformationType.PUBLICSECURITY,
+             InformationType.PRIVATESECURITY, InformationType.USERDATA],
+            self.factory.makeDistribution().getAllowedBranchInformationTypes())
+
 
 class TestDistributionCurrentSourceReleases(
     CurrentSourceReleasesMixin, TestCase):

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-08-30 07:20:24 +0000
+++ lib/lp/registry/tests/test_product.py	2012-08-30 07:20:24 +0000
@@ -459,6 +459,50 @@
             product.getDefaultBugInformationType())
 
 
+class TestProductBranchInformationTypes(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_no_policy(self):
+        # New projects can only use the non-proprietary information
+        # types.
+        product = self.factory.makeProduct()
+        self.assertContentEqual(
+            FREE_INFORMATION_TYPES, product.getAllowedBranchInformationTypes())
+
+    def test_sharing_policy_public_or_proprietary(self):
+        # branch_sharing_policy can enable Proprietary.
+        product = self.factory.makeProduct(
+            branch_sharing_policy=BranchSharingPolicy.PUBLIC_OR_PROPRIETARY)
+        self.assertContentEqual(
+            FREE_INFORMATION_TYPES + (InformationType.PROPRIETARY,),
+            product.getAllowedBranchInformationTypes())
+
+    def test_sharing_policy_proprietary_or_public(self):
+        # branch_sharing_policy can enable Proprietary.
+        product = self.factory.makeProduct(
+            branch_sharing_policy=BranchSharingPolicy.PROPRIETARY_OR_PUBLIC)
+        self.assertContentEqual(
+            FREE_INFORMATION_TYPES + (InformationType.PROPRIETARY,),
+            product.getAllowedBranchInformationTypes())
+
+    def test_sharing_policy_proprietary(self):
+        # branch_sharing_policy can enable only Proprietary.
+        product = self.factory.makeProduct(
+            branch_sharing_policy=BranchSharingPolicy.PROPRIETARY)
+        self.assertContentEqual(
+            [InformationType.PROPRIETARY],
+            product.getAllowedBranchInformationTypes())
+
+    def test_sharing_policy_embargoed_or_proprietary(self):
+        # branch_sharing_policy can enable Embargoed or Proprietary.
+        product = self.factory.makeProduct(
+            branch_sharing_policy=BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY)
+        self.assertContentEqual(
+            [InformationType.PROPRIETARY, InformationType.EMBARGOED],
+            product.getAllowedBranchInformationTypes())
+
+
 class ProductPermissionTestCase(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer