launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14564
[Merge] lp:~abentley/launchpad/more-transition-checks into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/more-transition-checks into lp:launchpad.
Commit message:
Add more checks transitioning product to Proprietary.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1082422 in Launchpad itself: "Product information type transition checks are incomplete"
https://bugs.launchpad.net/launchpad/+bug/1082422
For more details, see:
https://code.launchpad.net/~abentley/launchpad/more-transition-checks/+merge/136284
= Summary =
Fix bug #1082422: Product information type transition checks are incomplete
== Proposed fix ==
Treat the following as validation errors:
- Bug supervisor is an inclusive-membership team
- Questions exist on the product
- Translations exist on the product
== Pre-implementation notes ==
None
== LOC Rationale ==
Part of private products
== Implementation details ==
To improve the user experience, all validation errors are displayed at once. This is done by doing the validation in a generator that yields exceptions instead of raising them. Model-validation can raise the first validation error it encounters and form validation code can display all errors.
Other than that, this is pretty straightforward. We simply query for the forbidden objects and emit exceptions if we find them. We use Storm instead of Product or ProductSet methods, because those methods typically omit some results.
== Tests ==
bin/test -t test_checkInformationType_bug_supervisor -t test_checkInformationType_questions -t test_checkInformationType_translations
== Demo and Q/A ==
Create a product, set a team with open membership as its bug supervisor, and add a question. Attempt to make it proprietary. Both issues should be listed. Change the team's membership policy to a restrictive one. Only the Question issue should remain listed. Assign the Question to another product. You should be able to make the product proprietary.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/browser/product.py
lib/lp/registry/interfaces/product.py
lib/lp/registry/browser/tests/test_product.py
lib/lp/registry/model/product.py
lib/lp/registry/tests/test_product.py
--
https://code.launchpad.net/~abentley/launchpad/more-transition-checks/+merge/136284
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/more-transition-checks into lp:launchpad.
=== modified file 'lib/lp/registry/browser/product.py'
--- lib/lp/registry/browser/product.py 2012-11-19 18:56:23 +0000
+++ lib/lp/registry/browser/product.py 2012-11-26 22:02:29 +0000
@@ -157,7 +157,6 @@
PillarViewMixin,
)
from lp.registry.browser.productseries import get_series_branch_error
-from lp.registry.errors import CannotChangeInformationType
from lp.registry.interfaces.pillar import IPillarNameSet
from lp.registry.interfaces.product import (
IProduct,
@@ -1434,6 +1433,23 @@
if not private_projects:
self.form_fields = self.form_fields.omit('information_type')
+ def validate(self, data):
+ """Validate 'licenses' and 'license_info'.
+
+ 'licenses' must not be empty unless the product already
+ exists and never has had a licence set.
+
+ 'license_info' must not be empty if "Other/Proprietary"
+ or "Other/Open Source" is checked.
+ """
+ super(ProductEditView, self).validate(data)
+ info_type = data.get('information_type')
+ if info_type is not None:
+ errors = [str(e) for e in
+ self.context.checkInformationType(info_type)]
+ if len(errors) > 0:
+ self.setFieldError('information_type', ' '.join(errors))
+
def showOptionalMarker(self, field_name):
"""See `LaunchpadFormView`."""
# This has the effect of suppressing the ": (Optional)" stuff for the
@@ -1446,10 +1462,7 @@
@action("Change", name='change')
def change_action(self, action, data):
- try:
- self.updateContextFromData(data)
- except CannotChangeInformationType as e:
- self.setFieldError('information_type', str(e))
+ self.updateContextFromData(data)
class ProductValidationMixin:
=== modified file 'lib/lp/registry/browser/tests/test_product.py'
--- lib/lp/registry/browser/tests/test_product.py 2012-11-26 08:33:03 +0000
+++ lib/lp/registry/browser/tests/test_product.py 2012-11-26 22:02:29 +0000
@@ -532,6 +532,23 @@
attrs={'class': 'message'})
self.assertThat(browser.contents, HTMLContains(tag))
+ def test_multiple_info_type_errors(self):
+ # Multiple information type errors are presented at once.
+ product = self.factory.makeProduct()
+ self.factory.makeBranch(product=product)
+ self.factory.makeSpecification(product=product)
+ self.useFixture(FeatureFixture(
+ {u'disclosure.private_projects.enabled': u'on'}))
+ browser = self.getViewBrowser(product, '+edit', user=product.owner)
+ info_type = browser.getControl(name='field.information_type')
+ info_type.value = ['PROPRIETARY']
+ browser.getControl('Change').click()
+ tag = Tag(
+ 'error', 'div', attrs={'class': 'message'},
+ text='Some blueprints are public. '
+ 'Some branches are neither proprietary nor embargoed.')
+ self.assertThat(browser.contents, HTMLContains(tag))
+
def test_change_information_type_public(self):
owner = self.factory.makePerson(name='pting')
product = self.factory.makeProduct(
=== modified file 'lib/lp/registry/interfaces/product.py'
--- lib/lp/registry/interfaces/product.py 2012-11-26 19:48:32 +0000
+++ lib/lp/registry/interfaces/product.py 2012-11-26 22:02:29 +0000
@@ -910,6 +910,14 @@
Checks authorization and entitlement.
"""
+ def checkInformationType(value):
+ """Check whether the information type change should be permitted.
+
+ Iterate through exceptions explaining why the type should not be
+ changed. Has the side-effect of creating a commercial subscription if
+ permitted.
+ """
+
class IProduct(
IBugTarget, IHasBugSupervisor, IHasDrivers, IProductEditRestricted,
=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py 2012-11-26 16:28:11 +0000
+++ lib/lp/registry/model/product.py 2012-11-26 22:02:29 +0000
@@ -61,6 +61,7 @@
FAQSearch,
)
from lp.answers.model.question import (
+ Question,
QuestionTargetMixin,
QuestionTargetSearch,
)
@@ -130,6 +131,7 @@
from lp.registry.enums import (
BranchSharingPolicy,
BugSharingPolicy,
+ INCLUSIVE_TEAM_POLICY,
SpecificationSharingPolicy,
)
from lp.registry.errors import (
@@ -454,13 +456,24 @@
pass
def _valid_product_information_type(self, attr, value):
+ for exception in self.checkInformationType(value):
+ raise exception
+ return value
+
+ def checkInformationType(self, value):
+ """Check whether the information type change should be permitted.
+
+ Iterate through exceptions explaining why the type should not be
+ changed. Has the side-effect of creating a commercial subscription if
+ permitted.
+ """
if value not in PUBLIC_PROPRIETARY_INFORMATION_TYPES:
- raise CannotChangeInformationType('Not supported for Projects.')
+ yield CannotChangeInformationType('Not supported for Projects.')
if value in PROPRIETARY_INFORMATION_TYPES:
if self.answers_usage == ServiceUsage.LAUNCHPAD:
- raise CannotChangeInformationType('Answers is enabled.')
+ yield CannotChangeInformationType('Answers is enabled.')
if self._SO_creating or value not in PROPRIETARY_INFORMATION_TYPES:
- return value
+ return
# Additional checks when transitioning an existing product to a
# proprietary type
# All specs located by an ALL search are public.
@@ -469,26 +482,35 @@
if not public_specs.is_empty():
# Unlike bugs and branches, specifications cannot be USERDATA or a
# security type.
- raise CannotChangeInformationType(
+ yield CannotChangeInformationType(
'Some blueprints are public.')
- non_proprietary_bugs = Store.of(self).find(Bug,
+ store = Store.of(self)
+ non_proprietary_bugs = store.find(Bug,
Not(Bug.information_type.is_in(PROPRIETARY_INFORMATION_TYPES)),
BugTask.bug == Bug.id, BugTask.product == self.id)
if not non_proprietary_bugs.is_empty():
- raise CannotChangeInformationType(
+ yield CannotChangeInformationType(
'Some bugs are neither proprietary nor embargoed.')
# Default returns all public branches.
- non_proprietary_branches = Store.of(self).find(
+ non_proprietary_branches = store.find(
Branch, Branch.product == self.id,
Not(Branch.information_type.is_in(PROPRIETARY_INFORMATION_TYPES))
)
if not non_proprietary_branches.is_empty():
- raise CannotChangeInformationType(
+ yield CannotChangeInformationType(
'Some branches are neither proprietary nor embargoed.')
+ questions = store.find(Question, Question.product == self.id)
+ if not questions.is_empty():
+ yield CannotChangeInformationType('This project has questions.')
+ templates = store.find(
+ POTemplate, ProductSeries.product == self.id,
+ POTemplate.productseries == ProductSeries.id)
+ if not templates.is_empty():
+ yield CannotChangeInformationType('This project has translations.')
if not self.packagings.is_empty():
- raise CannotChangeInformationType('Some series are packaged.')
+ yield CannotChangeInformationType('Some series are packaged.')
if self.translations_usage == ServiceUsage.LAUNCHPAD:
- raise CannotChangeInformationType('Translations are enabled.')
+ yield CannotChangeInformationType('Translations are enabled.')
# Proprietary check works only after creation, because during
# creation, has_commercial_subscription cannot give the right value
# and triggers an inappropriate DB flush.
@@ -503,10 +525,13 @@
# If you have a commercial subscription, but it's not current, you
# cannot set the information type to a PROPRIETARY type.
if not self.has_current_commercial_subscription:
- raise CommercialSubscribersOnly(
+ yield CommercialSubscribersOnly(
'A valid commercial subscription is required for private'
' Projects.')
- return value
+ if (self.bug_supervisor is not None and
+ self.bug_supervisor.membership_policy in INCLUSIVE_TEAM_POLICY):
+ yield CannotChangeInformationType(
+ 'Bug supervisor has inclusive membership.')
_information_type = EnumCol(
enum=InformationType, default=InformationType.PUBLIC,
=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py 2012-11-26 19:48:32 +0000
+++ lib/lp/registry/tests/test_product.py 2012-11-26 22:02:29 +0000
@@ -64,6 +64,7 @@
INCLUSIVE_TEAM_POLICY,
SharingPermission,
SpecificationSharingPolicy,
+ TeamMembershipPolicy,
)
from lp.registry.errors import (
CannotChangeInformationType,
@@ -497,6 +498,56 @@
SpecificationSharingPolicy.PROPRIETARY,
product.specification_sharing_policy)
+ def test_checkInformationType_bug_supervisor(self):
+ # Bug supervisors of proprietary products must not have inclusive
+ # membership policies.
+ team = self.factory.makeTeam()
+ product = self.factory.makeProduct(bug_supervisor=team)
+ for policy in (token.value for token in TeamMembershipPolicy):
+ with person_logged_in(team.teamowner):
+ team.membership_policy = policy
+ for info_type in PROPRIETARY_INFORMATION_TYPES:
+ with person_logged_in(product.owner):
+ errors = list(product.checkInformationType(info_type))
+ if policy in EXCLUSIVE_TEAM_POLICY:
+ self.assertEqual([], errors)
+ else:
+ with ExpectedException(
+ CannotChangeInformationType,
+ 'Bug supervisor has inclusive membership.'):
+ raise errors[0]
+
+ def test_checkInformationType_questions(self):
+ # Proprietary products must not have questions
+ product = self.factory.makeProduct()
+ for info_type in PROPRIETARY_INFORMATION_TYPES:
+ with person_logged_in(product.owner):
+ self.assertEqual([],
+ list(product.checkInformationType(info_type)))
+ self.factory.makeQuestion(target=product)
+ for info_type in PROPRIETARY_INFORMATION_TYPES:
+ with person_logged_in(product.owner):
+ error, = list(product.checkInformationType(info_type))
+ with ExpectedException(CannotChangeInformationType,
+ 'This project has questions.'):
+ raise error
+
+ def test_checkInformationType_translations(self):
+ # Proprietary products must not have translations
+ productseries = self.factory.makeProductSeries()
+ product = productseries.product
+ for info_type in PROPRIETARY_INFORMATION_TYPES:
+ with person_logged_in(product.owner):
+ self.assertEqual([],
+ list(product.checkInformationType(info_type)))
+ self.factory.makePOTemplate(productseries=productseries)
+ for info_type in PROPRIETARY_INFORMATION_TYPES:
+ with person_logged_in(product.owner):
+ error, = list(product.checkInformationType(info_type))
+ with ExpectedException(CannotChangeInformationType,
+ 'This project has translations.'):
+ raise error
+
def createProduct(self, information_type=None, license=None):
# convenience method for testing IProductSet.createProduct rather than
# self.factory.makeProduct
@@ -740,7 +791,7 @@
'launchpad.Edit': set((
'addOfficialBugTag', 'removeOfficialBugTag',
'setBranchSharingPolicy', 'setBugSharingPolicy',
- 'setSpecificationSharingPolicy')),
+ 'setSpecificationSharingPolicy', 'checkInformationType')),
'launchpad.Moderate': set((
'is_permitted', 'license_approved', 'project_reviewed',
'reviewer_whiteboard', 'setAliases')),
Follow ups
