launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02826
[Merge] lp:~sinzui/launchpad/sane-definition-status-0 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/sane-definition-status-0 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#121608 DB constraint triggered adding a new blueprint with status obsolete or superseded
https://bugs.launchpad.net/bugs/121608
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/sane-definition-status-0/+merge/51932
Do not allow closed definition status when creating a specification.
Launchpad bug: https://bugs.launchpad.net/bugs/121608
Pre-implementation: thumper
Test command: ./bin/test -vv -m lp.blueprints
DB constraint triggered adding a new blueprint with status obsolete or
superseded: IntegrityError ERROR: new row for relation "specification"
violates check constraint "specification_completion_recorded_chk".
You cannot create a obsolete or superseded specification because the
form does not capture the all the status and people to actually set the
specification to a closed statues. Users should not be creating closed
specifications. This issue can also happen over API.
--------------------------------------------------------------------
RULES
* Create an enum that does not contain obsolete or superseded
definition status.
* Update INewSpecification to use it.
* ADDENDUM: copy the original definition_status to ISpecification to
preserve the rules for life cycle events.
* Add an adaption block to new() to convert the new enum to the life
cycle enum.
* This is because The two enum items are not equivalent, not through
inheritance or via use_template.
* As with branches, use the name of the item to adapt.
* I discussed this design issue with Tim. We may solve this issue in
lazr.enum and Lp's storm enumcol, but that is a separate issue.
QA
* Visit https://blueprints.qastaging.launchpad.net/gdp/+addspec
* Verify that obsolete or superseded are not in the definition
status field.
* Create a blueprint with any status.
* Change it's definition status to obsolete
LINT
lib/lp/blueprints/enums.py
lib/lp/blueprints/interfaces/specification.py
lib/lp/blueprints/model/specification.py
lib/lp/blueprints/tests/test_hasspecifications.py
lib/lp/blueprints/tests/test_specification.py
lib/lp/blueprints/tests/test_webservice.py
lib/lp/code/model/tests/test_branch.py
lib/lp/testing/factory.py
IMPLEMENTATION
Added a new enum using the original as a template. Updated the interface
to use the enum. Updated ISpecificationSet.new() to adapt the new enum to
the enum used by lifecycle events.
lib/lp/blueprints/enums.py
lib/lp/blueprints/interfaces/specification.py
lib/lp/blueprints/model/specification.py
lib/lp/blueprints/tests/test_hasspecifications.py
lib/lp/blueprints/tests/test_specification.py
Updated tests to import the enums form the true location.
lib/lp/blueprints/tests/test_webservice.py
lib/lp/code/model/tests/test_branch.py
Updated the factory to work with the life cycle rules.
lib/lp/testing/factory.py
--
https://code.launchpad.net/~sinzui/launchpad/sane-definition-status-0/+merge/51932
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/sane-definition-status-0 into lp:launchpad.
=== modified file 'lib/lp/blueprints/enums.py'
--- lib/lp/blueprints/enums.py 2010-11-01 03:57:52 +0000
+++ lib/lp/blueprints/enums.py 2011-03-02 17:56:37 +0000
@@ -5,6 +5,7 @@
__metaclass__ = type
__all__ = [
+ 'NewSpecificationDefinitionStatus',
'SpecificationDefinitionStatus',
'SpecificationFilter',
'SpecificationGoalStatus',
@@ -21,6 +22,7 @@
DBItem,
EnumeratedType,
Item,
+ use_template,
)
@@ -433,6 +435,22 @@
""")
+class NewSpecificationDefinitionStatus(DBEnumeratedType):
+ """The Initial status of a Specification.
+
+ The initial status to define the feature and get approval for the
+ implementation plan.
+ """
+ use_template(SpecificationDefinitionStatus, include=(
+ 'NEW',
+ 'DISCUSSION',
+ 'DRAFT',
+ 'PENDINGREVIEW',
+ 'PENDINGAPPROVAL',
+ 'APPROVED',
+ ))
+
+
class SpecificationGoalStatus(DBEnumeratedType):
"""The target status for this specification.
=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py 2011-02-24 15:30:54 +0000
+++ lib/lp/blueprints/interfaces/specification.py 2011-03-02 17:56:37 +0000
@@ -47,6 +47,7 @@
from canonical.launchpad.interfaces.validation import valid_webref
from lp.app.validators import LaunchpadValidationError
from lp.blueprints.enums import (
+ NewSpecificationDefinitionStatus,
SpecificationDefinitionStatus,
SpecificationGoalStatus,
SpecificationImplementationStatus,
@@ -168,8 +169,8 @@
definition_status = exported(
Choice(
title=_('Definition Status'),
- vocabulary=SpecificationDefinitionStatus,
- default=SpecificationDefinitionStatus.NEW,
+ vocabulary=NewSpecificationDefinitionStatus,
+ default=NewSpecificationDefinitionStatus.NEW,
description=_(
"The current status of the process to define the "
"feature and get approval for the implementation plan.")),
@@ -275,6 +276,18 @@
# referencing it.
id = Int(title=_("Database ID"), required=True, readonly=True)
+ # Redefine definition_status to support all definition statuses for
+ # life cycle events.
+ definition_status = exported(
+ Choice(
+ title=_('Definition Status'),
+ vocabulary=SpecificationDefinitionStatus,
+ default=SpecificationDefinitionStatus.NEW,
+ description=_(
+ "The current status of the process to define the "
+ "feature and get approval for the implementation plan.")),
+ ('devel', dict(exported=True, readonly=True)), exported=False)
+
priority = exported(
Choice(
title=_('Priority'), vocabulary=SpecificationPriority,
=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py 2010-12-02 16:13:51 +0000
+++ lib/lp/blueprints/model/specification.py 2011-03-02 17:56:37 +0000
@@ -52,6 +52,7 @@
)
from lp.blueprints.adapters import SpecificationDelta
from lp.blueprints.enums import (
+ NewSpecificationDefinitionStatus,
SpecificationDefinitionStatus,
SpecificationFilter,
SpecificationGoalStatus,
@@ -689,6 +690,7 @@
"""
# Circular import.
from lp.registry.model.person import Person
+
def cache_people(rows):
# Find the people we need:
person_ids = set()
@@ -717,6 +719,7 @@
column = row[index]
index += 1
decorator(person, column)
+
results = Store.of(self).find(
Specification,
SQL(query),
@@ -900,6 +903,15 @@
drafter=None, whiteboard=None,
priority=SpecificationPriority.UNDEFINED):
"""See ISpecificationSet."""
+ # Adapt the NewSpecificationDefinitionStatus item to a
+ # SpecificationDefinitionStatus item.
+ status_name = definition_status.name
+ status_names = NewSpecificationDefinitionStatus.items.mapping.keys()
+ if status_name not in status_names:
+ raise AssertionError(
+ "definition_status must an item found in "
+ "NewSpecificationDefinitionStatus.")
+ definition_status = SpecificationDefinitionStatus.items[status_name]
return Specification(name=name, title=title, specurl=specurl,
summary=summary, priority=priority,
definition_status=definition_status, owner=owner,
=== modified file 'lib/lp/blueprints/tests/test_hasspecifications.py'
--- lib/lp/blueprints/tests/test_hasspecifications.py 2011-01-13 14:26:14 +0000
+++ lib/lp/blueprints/tests/test_hasspecifications.py 2011-03-02 17:56:37 +0000
@@ -6,9 +6,7 @@
__metaclass__ = type
from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.blueprints.interfaces.specification import (
- SpecificationDefinitionStatus,
- )
+from lp.blueprints.enums import SpecificationDefinitionStatus
from lp.blueprints.interfaces.specificationtarget import (
IHasSpecifications,
)
=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py 2010-12-17 13:29:40 +0000
+++ lib/lp/blueprints/tests/test_specification.py 2011-03-02 17:56:37 +0000
@@ -6,13 +6,19 @@
__metaclass__ = type
+from zope.component import getUtility
from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
from canonical.launchpad.webapp.authorization import check_permission
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.blueprints.errors import TargetAlreadyHasSpecification
-from lp.blueprints.interfaces.specification import SpecificationGoalStatus
+from lp.blueprints.enums import (
+ NewSpecificationDefinitionStatus,
+ SpecificationDefinitionStatus,
+ SpecificationGoalStatus,
+ )
+from lp.blueprints.interfaces.specification import ISpecificationSet
from lp.testing import (
login_person,
TestCaseWithFactory,
@@ -101,3 +107,39 @@
product=self.factory.makeProduct())
self.assertRaises(
Unauthorized, getattr, specification, 'setTarget')
+
+
+class TestSpecificationSet(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestSpecificationSet, self).setUp()
+ self.specification_set = getUtility(ISpecificationSet)
+ self.new_names = NewSpecificationDefinitionStatus.items.mapping.keys()
+
+ def test_new_with_open_definition_status_creates_specification(self):
+ # Calling new() with an open definition status will will create
+ # a specification.
+ self.assertTrue(
+ SpecificationDefinitionStatus.NEW.name in self.new_names)
+ product = self.factory.makeProduct()
+ spec = self.specification_set.new(
+ name='plane', title='Place', specurl='http://eg.org/plane',
+ summary='summary', owner=product.owner, product=product,
+ definition_status=SpecificationDefinitionStatus.NEW)
+ self.assertEqual(
+ SpecificationDefinitionStatus.NEW, spec.definition_status)
+
+ def test_new_with_closed_definition_status_raises_error(self):
+ # Calling new() with a obsolete or superseded definition status
+ # raises an error.
+ self.assertTrue(
+ SpecificationDefinitionStatus.OBSOLETE.name not in self.new_names)
+ product = self.factory.makeProduct()
+ args = dict(
+ name='plane', title='Place', specurl='http://eg.org/plane',
+ summary='summary', owner=product.owner, product=product,
+ definition_status=SpecificationDefinitionStatus.OBSOLETE)
+ self.assertRaises(
+ AssertionError, self.specification_set.new, **args)
=== modified file 'lib/lp/blueprints/tests/test_webservice.py'
--- lib/lp/blueprints/tests/test_webservice.py 2011-02-03 19:01:10 +0000
+++ lib/lp/blueprints/tests/test_webservice.py 2011-03-02 17:56:37 +0000
@@ -9,9 +9,7 @@
from canonical.testing import DatabaseFunctionalLayer
from canonical.launchpad.testing.pages import webservice_for_person
-from lp.blueprints.interfaces.specification import (
- SpecificationDefinitionStatus,
- )
+from lp.blueprints.enums import SpecificationDefinitionStatus
from lp.testing import (
launchpadlib_for,
person_logged_in,
=== modified file 'lib/lp/code/model/tests/test_branch.py'
--- lib/lp/code/model/tests/test_branch.py 2010-12-02 16:13:51 +0000
+++ lib/lp/code/model/tests/test_branch.py 2011-03-02 17:56:37 +0000
@@ -35,7 +35,7 @@
DatabaseFunctionalLayer,
LaunchpadZopelessLayer,
)
-from lp.blueprints.enums import SpecificationDefinitionStatus
+from lp.blueprints.enums import NewSpecificationDefinitionStatus
from lp.blueprints.interfaces.specification import ISpecificationSet
from lp.blueprints.model.specificationbranch import SpecificationBranch
from lp.bugs.interfaces.bug import (
@@ -1100,7 +1100,7 @@
spec = getUtility(ISpecificationSet).new(
name='some-spec', title='Some spec', product=self.product,
owner=self.user, summary='', specurl=None,
- definition_status=SpecificationDefinitionStatus.NEW)
+ definition_status=NewSpecificationDefinitionStatus.NEW)
spec.linkBranch(self.branch, self.user)
self.assertEqual(self.branch.canBeDeleted(), False,
"A branch linked to a spec is not deletable.")
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-03-01 09:43:35 +0000
+++ lib/lp/testing/factory.py 2011-03-02 17:56:37 +0000
@@ -104,6 +104,7 @@
from lp.archiveuploader.dscfile import DSCFile
from lp.archiveuploader.uploadpolicy import BuildDaemonUploadPolicy
from lp.blueprints.enums import (
+ NewSpecificationDefinitionStatus,
SpecificationDefinitionStatus,
SpecificationPriority,
)
@@ -1882,7 +1883,7 @@
def makeSpecification(self, product=None, title=None, distribution=None,
name=None, summary=None, owner=None,
- status=SpecificationDefinitionStatus.NEW,
+ status=NewSpecificationDefinitionStatus.NEW,
implementation_status=None, goal=None, specurl=None,
assignee=None, drafter=None, approver=None,
priority=None, whiteboard=None, milestone=None):
@@ -1903,12 +1904,18 @@
owner = self.makePerson()
if priority is None:
priority = SpecificationPriority.UNDEFINED
+ status_names = NewSpecificationDefinitionStatus.items.mapping.keys()
+ if status.name in status_names:
+ definition_status = status
+ else:
+ # This is to satisfy life cycle requirements.
+ definition_status = NewSpecificationDefinitionStatus.NEW
spec = getUtility(ISpecificationSet).new(
name=name,
title=title,
specurl=None,
summary=summary,
- definition_status=status,
+ definition_status=definition_status,
whiteboard=whiteboard,
owner=owner,
assignee=assignee,
@@ -1918,6 +1925,9 @@
distribution=distribution,
priority=priority)
naked_spec = removeSecurityProxy(spec)
+ if status.name not in status_names:
+ # Set the closed status after the status has a sane initial state.
+ naked_spec.definition_status = status
if status == SpecificationDefinitionStatus.OBSOLETE:
# This is to satisfy a DB constraint of obsolete specs.
naked_spec.completer = owner
