Merge lp:~sinzui/launchpad/sane-definition-status-0 into lp:launchpad

[Curtis Hovey]

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

[Robert Collins (lifeless)]

review: Approve

=== 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 18:01:19 +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 18:01:19 +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 18:01:19 +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 18:01:19 +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 18:01:19 +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 18:01:19 +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 18:01:19 +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 18:01:19 +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