launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/limit-product-info-types into lp:launchpad.

Commit message:
Ensure only valid Product.information_type values are used.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~abentley/launchpad/limit-product-info-types/+merge/127814

= Summary =
Ensure only valid information_types are used for Product.

== Proposed fix ==
Use a Storm validator for Product.information_type.

== Pre-implementation notes ==
Discussed with deryck

== LOC Rationale ==
Part of Private Projects

== Implementation details ==
The storm validator ensures that values outside PUBLIC_PROPRIETARY are not accepted.  For mutations, it ensures that PROPRIETARY values can only be set if there is a commercial subscription.  For creation, createProject ensures that PROPRIETARY values are only used with Other/Proprietary licenses, which imply a commercial subscription.

== Tests ==
bin/test -t TestProduct registry.tests.test_product

== Demo and Q/A ==
Modifying an existing Product should not complain about a NULL information_type.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/configure.zcml
  lib/lp/registry/interfaces/product.py
  lib/lp/app/interfaces/informationtype.py
  lib/lp/app/model/launchpad.py
  lib/lp/testing/factory.py
  lib/lp/scripts/garbo.py
  lib/lp/blueprints/model/specification.py
  lib/lp/scripts/tests/test_garbo.py
  lib/lp/bugs/model/bug.py
  lib/lp/registry/model/product.py
  lib/lp/registry/tests/test_product.py
-- 
https://code.launchpad.net/~abentley/launchpad/limit-product-info-types/+merge/127814
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/limit-product-info-types into lp:launchpad.

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-10-03 13:05:31 +0000
+++ lib/lp/registry/model/product.py	2012-10-03 16:15:41 +0000
@@ -69,6 +69,8 @@
     FREE_INFORMATION_TYPES,
     InformationType,
     PRIVATE_INFORMATION_TYPES,
+    PUBLIC_PROPRIETARY_INFORMATION_TYPES,
+    PROPRIETARY_INFORMATION_TYPES,
     service_uses_launchpad,
     ServiceUsage,
     )
@@ -127,7 +129,10 @@
     BugSharingPolicy,
     SpecificationSharingPolicy,
     )
-from lp.registry.errors import CommercialSubscribersOnly
+from lp.registry.errors import (
+    CannotChangeInformationType,
+    CommercialSubscribersOnly,
+    )
 from lp.registry.interfaces.accesspolicy import (
     IAccessPolicyArtifactSource,
     IAccessPolicyGrantSource,
@@ -415,8 +420,22 @@
         """
         pass
 
+    def _valid_product_information_type(self, attr, value):
+        if value not in PUBLIC_PROPRIETARY_INFORMATION_TYPES:
+            raise CannotChangeInformationType('Not supported for Projects.')
+        # Proprietary check works only after creation, because during
+        # creation, has_commercial_subscription cannot give the right value
+        # and triggers an inappropriate DB flush.
+        if (not self._SO_creating and value in PROPRIETARY_INFORMATION_TYPES
+            and not self.has_current_commercial_subscription):
+            raise CommercialSubscribersOnly(
+                'A valid commercial subscription is required for private'
+                ' Projects.')
+        return value
+
     information_type = EnumCol(
-        enum=InformationType, default=InformationType.PUBLIC)
+        enum=InformationType, default=InformationType.PUBLIC,
+        storm_validator=_valid_product_information_type)
 
     security_contact = None
 
@@ -1603,6 +1622,15 @@
             licenses = set()
         if information_type is None:
             information_type = InformationType.PUBLIC
+        if information_type in PROPRIETARY_INFORMATION_TYPES:
+            # This check is skipped in _valid_product_information_type during
+            # creation, so done here.  It predicts whether a commercial
+            # subscription will be generated based on the selected license,
+            # duplicating product._setLicenses
+            if License.OTHER_PROPRIETARY not in licenses:
+                raise CommercialSubscribersOnly(
+                    'A valid commercial subscription is required for private'
+                    ' Projects.')
         product = Product(
             owner=owner, registrant=registrant, name=name,
             displayname=displayname, title=title, project=project,

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2012-10-02 18:44:49 +0000
+++ lib/lp/registry/tests/test_product.py	2012-10-03 16:15:41 +0000
@@ -9,6 +9,7 @@
 import pytz
 from storm.locals import Store
 from testtools.matchers import MatchesAll
+from testtools.testcase import ExpectedException
 import transaction
 from zope.component import getUtility
 from zope.lifecycleevent.interfaces import IObjectModifiedEvent
@@ -19,6 +20,8 @@
 from lp.app.enums import (
     FREE_INFORMATION_TYPES,
     InformationType,
+    PUBLIC_PROPRIETARY_INFORMATION_TYPES,
+    PROPRIETARY_INFORMATION_TYPES,
     ServiceUsage,
     )
 from lp.app.interfaces.launchpad import (
@@ -41,6 +44,7 @@
     SpecificationSharingPolicy,
     )
 from lp.registry.errors import (
+    CannotChangeInformationType,
     CommercialSubscribersOnly,
     InclusiveTeamLinkageError,
     )
@@ -393,10 +397,25 @@
         expected = [InformationType.PROPRIETARY]
         self.assertContentEqual(expected, [policy.type for policy in aps])
 
+    def createProduct(self, information_type=None, license=None):
+        # convenience method for testing IProductSet.createProduct rather than
+        # self.factory.makeProduct
+        owner = self.factory.makePerson()
+        kwargs = {}
+        if information_type is not None:
+            kwargs['information_type'] = information_type
+        if license is not None:
+            kwargs['licenses'] = [license]
+        with person_logged_in(owner):
+            return getUtility(IProductSet).createProduct(
+                owner, self.factory.getUniqueString('product'),
+                'Fnord', 'Fnord', 'test 1', 'test 2', **kwargs)
+
     def test_product_information_type(self):
         # Product is created with specified information_type
-        product = self.factory.makeProduct(
-            information_type=InformationType.EMBARGOED)
+        product = self.createProduct(
+            information_type=InformationType.EMBARGOED,
+            license=License.OTHER_PROPRIETARY)
         self.assertEqual(InformationType.EMBARGOED, product.information_type)
         # Owner can set information_type
         with person_logged_in(product.owner):
@@ -411,11 +430,58 @@
 
     def test_product_information_type_default(self):
         # Default information_type is PUBLIC
-        owner = self.factory.makePerson()
-        product = getUtility(IProductSet).createProduct(
-            owner, 'fnord', 'Fnord', 'Fnord', 'test 1', 'test 2')
+        product = self.createProduct()
         self.assertEqual(InformationType.PUBLIC, product.information_type)
 
+    invalid_information_types = [info_type for info_type in
+            InformationType.items if info_type not in
+            PUBLIC_PROPRIETARY_INFORMATION_TYPES]
+
+    def test_product_information_type_init_invalid_values(self):
+        # Cannot create Product.information_type with invalid values.
+        for info_type in self.invalid_information_types:
+            with ExpectedException(
+                CannotChangeInformationType, 'Not supported for Projects.'):
+                self.createProduct(information_type=info_type)
+
+    def test_product_information_type_set_invalid_values(self):
+        # Cannot set Product.information_type to invalid values.
+        product = self.factory.makeProduct()
+        for info_type in self.invalid_information_types:
+            with ExpectedException(
+                CannotChangeInformationType, 'Not supported for Projects.'):
+                with person_logged_in(product.owner):
+                    product.information_type = info_type
+
+    def test_product_information_set_proprietary_requires_commercial(self):
+        # Cannot set Product.information_type to proprietary values if no
+        # commercial subscription.
+        product = self.factory.makeProduct()
+        self.useContext(person_logged_in(product.owner))
+        for info_type in PROPRIETARY_INFORMATION_TYPES:
+            with ExpectedException(
+                CommercialSubscribersOnly,
+                'A valid commercial subscription is required for private'
+                ' Projects.'):
+                product.information_type = info_type
+        product.redeemSubscriptionVoucher(
+            'hello', product.owner, product.owner, 1)
+        for info_type in PROPRIETARY_INFORMATION_TYPES:
+            product.information_type = info_type
+
+    def test_product_information_init_proprietary_requires_commercial(self):
+        # Cannot create a product with proprietary types without specifying
+        # Other/Proprietary license.
+        for info_type in PROPRIETARY_INFORMATION_TYPES:
+            with ExpectedException(
+                CommercialSubscribersOnly,
+                'A valid commercial subscription is required for private'
+                ' Projects.'):
+                self.createProduct(info_type)
+        for info_type in PROPRIETARY_INFORMATION_TYPES:
+            product = self.createProduct(info_type, License.OTHER_PROPRIETARY)
+            self.assertEqual(info_type, product.information_type)
+
 
 class TestProductBugInformationTypes(TestCaseWithFactory):