launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Richard Harding has proposed merging lp:~rharding/launchpad/limit_sharing_infotype_1083761 into lp:launchpad.

Commit message:
Limit the policy options for get(Bug|Branch|Specification)SharingPolicies based on the project information type.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1083761 in Launchpad itself: "oops changing the sharing policy for a proprietary project for anything other than proprietary"
  https://bugs.launchpad.net/launchpad/+bug/1083761

For more details, see:
https://code.launchpad.net/~rharding/launchpad/limit_sharing_infotype_1083761/+merge/137038

= Summary =

Per the bug, when a project is non-public then the sharing policies should
never show that you can change the sharing policy to something that is public.


== Pre Implementation ==

Talked with Deryck and Aaron to make sure I was understanding the policies. We
decided to add back in the embargoed and proprietary option into the UI even
though it's also part of the API.

See bug #1055617 for the reason it was originally removed, but it's no longer
an issue.


== Implementation Notes ==

To make things cleaner I added the private property that other models already
have. It makes sense to keep the api onto the products now that they have
information type as well.

Since we're allowing the display of the embargoed or proprietary I removed the
old check in the getBranchSharingPolicy method. See the bug referenced ^^.

The check is pretty simple. If it's a public product and you have a commercial
subscription you can set about any policy you want. However, if it's not
public then the only two policies that apply are proprietary and
embargoed/proprietary. We don't have a reverse policy for
proprietary/embargoed so it's not listed.

There's a new test method for each getXXXSharingPolicy and a tweak to an
existing test that changed/adjusted. Note that we had to specify the
information type because the factory will allow you to set a sharing policy of
proprietary while leaving the information type of the product as public.

This is a weakness of the factory and adding all the sanity checks to the
factory would be a bit crazy, so we just tweak the test itself.


== QA ==

Create a new public product with a proprietary license and you should get a
commercial subscription. From there, the sharing ui should allow you to select
the sharing policies available.

Create a non-public product and you should only be able to change the sharing
policies to proprietary or embargoed/proprietary from the ui.


== Tests ==

lib/lp/registry/services/tests/test_sharingservice.py




-- 
https://code.launchpad.net/~rharding/launchpad/limit_sharing_infotype_1083761/+merge/137038
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rharding/launchpad/limit_sharing_infotype_1083761 into lp:launchpad.

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-11-27 13:43:33 +0000
+++ lib/lp/registry/model/product.py	2012-11-29 20:29:22 +0000
@@ -1786,6 +1786,10 @@
     def people(self):
         return getUtility(IPersonSet)
 
+    @property
+    def private(self):
+        return self.information_type in PRIVATE_INFORMATION_TYPES
+
     @classmethod
     def latest(cls, user, quantity=5):
         """See `IProductSet`."""

=== modified file 'lib/lp/registry/services/sharingservice.py'
--- lib/lp/registry/services/sharingservice.py	2012-11-12 23:03:24 +0000
+++ lib/lp/registry/services/sharingservice.py	2012-11-29 20:29:22 +0000
@@ -439,17 +439,24 @@
         # default to Public.
         # If the branch sharing policy is EMBARGOED_OR_PROPRIETARY, then we
         # do not allow any other policies.
-        allowed_policies = []
-        if (pillar.branch_sharing_policy !=
-                BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY):
-            allowed_policies = [BranchSharingPolicy.PUBLIC]
-            # Commercial projects also allow proprietary branches.
-            if (IProduct.providedBy(pillar)
-                and pillar.has_current_commercial_subscription):
-                allowed_policies.extend([
+        allowed_policies = [BranchSharingPolicy.PUBLIC]
+        # Commercial projects also allow proprietary branches.
+        if (IProduct.providedBy(pillar)
+            and pillar.has_current_commercial_subscription):
+
+            if pillar.private:
+                allowed_policies = [
+                    BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY,
+                    BranchSharingPolicy.PROPRIETARY,
+                ]
+            else:
+                allowed_policies = [
+                    BranchSharingPolicy.PUBLIC,
                     BranchSharingPolicy.PUBLIC_OR_PROPRIETARY,
                     BranchSharingPolicy.PROPRIETARY_OR_PUBLIC,
-                    BranchSharingPolicy.PROPRIETARY])
+                    BranchSharingPolicy.PROPRIETARY,
+                ]
+
         if (pillar.branch_sharing_policy and
             not pillar.branch_sharing_policy in allowed_policies):
             allowed_policies.append(pillar.branch_sharing_policy)
@@ -464,10 +471,20 @@
         # Commercial projects also allow proprietary bugs.
         if (IProduct.providedBy(pillar)
             and pillar.has_current_commercial_subscription):
-            allowed_policies.extend([
-                BugSharingPolicy.PUBLIC_OR_PROPRIETARY,
-                BugSharingPolicy.PROPRIETARY_OR_PUBLIC,
-                BugSharingPolicy.PROPRIETARY])
+
+            if pillar.private:
+                allowed_policies = [
+                    BugSharingPolicy.EMBARGOED_OR_PROPRIETARY,
+                    BugSharingPolicy.PROPRIETARY,
+                ]
+            else:
+                allowed_policies = [
+                    BugSharingPolicy.PUBLIC,
+                    BugSharingPolicy.PUBLIC_OR_PROPRIETARY,
+                    BugSharingPolicy.PROPRIETARY_OR_PUBLIC,
+                    BugSharingPolicy.PROPRIETARY,
+                ]
+
         if (pillar.bug_sharing_policy and
             not pillar.bug_sharing_policy in allowed_policies):
             allowed_policies.append(pillar.bug_sharing_policy)
@@ -482,10 +499,20 @@
         # Commercial projects also allow proprietary specifications.
         if (IProduct.providedBy(pillar)
             and pillar.has_current_commercial_subscription):
-            allowed_policies.extend([
-                SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY,
-                SpecificationSharingPolicy.PROPRIETARY_OR_PUBLIC,
-                SpecificationSharingPolicy.PROPRIETARY])
+
+            if pillar.private:
+                allowed_policies = [
+                    SpecificationSharingPolicy.EMBARGOED_OR_PROPRIETARY,
+                    SpecificationSharingPolicy.PROPRIETARY,
+                ]
+            else:
+                allowed_policies = [
+                    SpecificationSharingPolicy.PUBLIC,
+                    SpecificationSharingPolicy.PUBLIC_OR_PROPRIETARY,
+                    SpecificationSharingPolicy.PROPRIETARY_OR_PUBLIC,
+                    SpecificationSharingPolicy.PROPRIETARY,
+                ]
+
         if (pillar.specification_sharing_policy and
             not pillar.specification_sharing_policy in allowed_policies):
             allowed_policies.append(pillar.specification_sharing_policy)

=== modified file 'lib/lp/registry/services/tests/test_sharingservice.py'
--- lib/lp/registry/services/tests/test_sharingservice.py	2012-10-12 14:53:10 +0000
+++ lib/lp/registry/services/tests/test_sharingservice.py	2012-11-29 20:29:22 +0000
@@ -192,6 +192,45 @@
              BranchSharingPolicy.PROPRIETARY_OR_PUBLIC,
              BranchSharingPolicy.PROPRIETARY])
 
+    def test_getBugSharingPolicies_non_public_product(self):
+        # When the product is non-public the policy options are limited to
+        # only proprietary or embargoed/proprietary.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY,
+            owner=owner,
+        )
+        with person_logged_in(owner):
+            self._assert_getBugSharingPolicies(
+                product, [BugSharingPolicy.EMBARGOED_OR_PROPRIETARY,
+                          BugSharingPolicy.PROPRIETARY])
+
+    def test_getBranchSharingPolicies_non_public_product(self):
+        # When the product is non-public the policy options are limited to
+        # only proprietary or embargoed/proprietary.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY,
+            owner=owner
+        )
+        with person_logged_in(owner):
+            self._assert_getBranchSharingPolicies(
+                product, [BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY,
+                          BranchSharingPolicy.PROPRIETARY])
+
+    def test_getSpecificationSharingPolicies_non_public_product(self):
+        # When the product is non-public the policy options are limited to
+        # only proprietary or embargoed/proprietary.
+        owner = self.factory.makePerson()
+        product = self.factory.makeProduct(
+            information_type=InformationType.PROPRIETARY,
+            owner=owner,
+        )
+        with person_logged_in(owner):
+            self._assert_getSpecificationSharingPolicies(
+                product, [SpecificationSharingPolicy.EMBARGOED_OR_PROPRIETARY,
+                          SpecificationSharingPolicy.PROPRIETARY])
+
     def test_getBranchSharingPolicies_disallowed_policy(self):
         # getBranchSharingPolicies includes a pillar's current policy even if
         # it is nominally not allowed.
@@ -204,12 +243,17 @@
             [BranchSharingPolicy.PUBLIC, BranchSharingPolicy.FORBIDDEN])
 
     def test_getBranchSharingPolicies_product_with_embargoed(self):
-        # If the current sharing policy is embargoed, that is all that is
-        # allowed.
+        # If the current sharing policy is embargoed, it can still be made
+        # proprietary.
+        owner = self.factory.makePerson()
         product = self.factory.makeProduct(
+            information_type=InformationType.EMBARGOED,
+            owner=owner,
             branch_sharing_policy=BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY)
-        self._assert_getBranchSharingPolicies(
-            product, [BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY])
+        with person_logged_in(owner):
+            self._assert_getBranchSharingPolicies(
+                product, [BranchSharingPolicy.EMBARGOED_OR_PROPRIETARY,
+                          BranchSharingPolicy.PROPRIETARY])
 
     def test_getBranchSharingPolicies_distro(self):
         distro = self.factory.makeDistribution()