Merge lp:~linaro-infrastructure/launchpad/workitems-parsing-and-updating into lp:launchpad

[Guilherme Salgado]

[r=adeuring][bug=940446] new method Specification.updateWorkItems()

This branch adds a new method on ISpecification which will take a list of dictionaries describing work items and update/delete/create work items in the DB to match that. This is going to be used in the new UI for editing work items (lp:~linaro-infrastructure/launchpad/workitems-widget), which will consist of a text area widget identical to the one currently used for the whiteboard.

In lp:~linaro-infrastructure/launchpad/workitems-widget we're implementing a new field class which will parse the work items entered by the user into the list of dicts expected by updateWorkItems()

[Abel Deuring (adeuring)]

Hi Salgado,

the changes look good overall, I just have a few formal nitpicks, see below.

> + def updateWorkItems(new_work_items):
> + """Update the existing work items to match the given ones.
> +
> + First, for every existing work item that is not present on the new
> + list, mark it as deleted. Then, for every tuple in the given list,

s/tuple/dictionary/

> + lookup an existing work item with the same title and update its
> + status, assignee, milestone and sequence (position on the work-items
> + list). If there's no existing work items with that title, we create a
> + new one.
> +
> + :param new_work_items: A list of dictionaries containing the following
> + keys: title, status, assignee and milestone.
> + """

> + def _deleteWorkItemsNotMatching(self, titles):
> + """Delete all work items whose title does not match the given ones.
> +
> + Also set the sequence of those deleted work items to -1.
> + """

Since the work item will not literally be deleted, I think "Mark all
work items as deleted" is a bit more precise than "Delete all work items"

> + for work_item in self.work_items:
> + if work_item.title not in titles:
> + work_item.deleted = True
> +
> + def updateWorkItems(self, new_work_items):
> + """See ISpecification."""
> + # First mark work items with titles that are no longer present as
> + # deleted.
> + self._deleteWorkItemsNotMatching(
> + [wi['title'] for wi in new_work_items])
> + work_items = Store.of(self).find(
> + SpecificationWorkItem, specification=self, deleted=False)
> + work_items = list(work_items.order_by("sequence"))
> + # At this point the list of new_work_items is necessarily the same
> + # size (or longer) than the list of existing ones, so we can just
> + # iterate over it updating the existing items and creating any new
> + # ones.
> + to_insert = []
> + existing_titles = [wi.title for wi in work_items]
> + for i in range(len(new_work_items)):

Our style guide does not allow single-letter variable names. WHat about
"for position in..." or "for index in..."?

review: Approve (code)

[Guilherme Salgado (salgado)]

On 24/02/12 12:43, Abel Deuring wrote:
> Review: Approve code
>
> Hi Salgado,
>
> the changes look good overall, I just have a few formal nitpicks, see below.

Thanks, Abel!

>
>> + def updateWorkItems(new_work_items):
>> + """Update the existing work items to match the given ones.
>> +
>> + First, for every existing work item that is not present on the new
>> + list, mark it as deleted. Then, for every tuple in the given list,
>
> s/tuple/dictionary/

Oops, fixed.

>
>> + lookup an existing work item with the same title and update its
>> + status, assignee, milestone and sequence (position on the work-items
>> + list). If there's no existing work items with that title, we create a
>> + new one.
>> +
>> + :param new_work_items: A list of dictionaries containing the following
>> + keys: title, status, assignee and milestone.
>> + """
>
>> + def _deleteWorkItemsNotMatching(self, titles):
>> + """Delete all work items whose title does not match the given ones.
>> +
>> + Also set the sequence of those deleted work items to -1.
>> + """
>
> Since the work item will not literally be deleted, I think "Mark all
> work items as deleted" is a bit more precise than "Delete all work items"

Good point, changed.

>
>
>> + for work_item in self.work_items:
>> + if work_item.title not in titles:
>> + work_item.deleted = True
>> +
>> + def updateWorkItems(self, new_work_items):
>> + """See ISpecification."""
>> + # First mark work items with titles that are no longer present as
>> + # deleted.
>> + self._deleteWorkItemsNotMatching(
>> + [wi['title'] for wi in new_work_items])
>> + work_items = Store.of(self).find(
>> + SpecificationWorkItem, specification=self, deleted=False)
>> + work_items = list(work_items.order_by("sequence"))
>> + # At this point the list of new_work_items is necessarily the same
>> + # size (or longer) than the list of existing ones, so we can just
>> + # iterate over it updating the existing items and creating any new
>> + # ones.
>> + to_insert = []
>> + existing_titles = [wi.title for wi in work_items]
>> + for i in range(len(new_work_items)):
>
> Our style guide does not allow single-letter variable names. WHat about
> "for position in..." or "for index in..."?

I don't think this should apply to a counter variable used in a for
loop, but I'm happy to rename it to index. :)

=== modified file 'lib/lp/blueprints/interfaces/specification.py'
--- lib/lp/blueprints/interfaces/specification.py	2012-02-09 01:19:00 +0000
+++ lib/lp/blueprints/interfaces/specification.py	2012-02-24 14:50:29 +0000
@@ -41,6 +41,7 @@
     Choice,
     Datetime,
     Int,
+    List,
     Text,
     TextLine,
     )
@@ -63,6 +64,9 @@
     IHasSpecifications,
     ISpecificationTarget,
     )
+from lp.blueprints.interfaces.specificationworkitem import (
+    ISpecificationWorkItem
+    )
 from lp.blueprints.interfaces.sprint import ISprint
 from lp.bugs.interfaces.buglink import IBugLinkTarget
 from lp.code.interfaces.branchlink import IHasLinkedBranches
@@ -289,6 +293,10 @@
     date_goal_decided = Attribute("The date the spec was approved "
         "or declined as a goal.")
 
+    work_items = List(
+        description=_("All non-deleted work items for this spec, sorted by "
+                      "their 'sequence'"),
+        value_type=Reference(schema=ISpecificationWorkItem), readonly=True)
     whiteboard = exported(
         Text(title=_('Status Whiteboard'), required=False,
              description=_(
@@ -575,6 +583,20 @@
                     milestone=None):
         """Create a new SpecificationWorkItem."""
 
+    def updateWorkItems(new_work_items):
+        """Update the existing work items to match the given ones.
+
+        First, for every existing work item that is not present on the new
+        list, mark it as deleted. Then, for every tuple in the given list,
+        lookup an existing work item with the same title and update its
+        status, assignee, milestone and sequence (position on the work-items
+        list). If there's no existing work items with that title, we create a
+        new one.
+
+        :param new_work_items: A list of dictionaries containing the following
+            keys: title, status, assignee and milestone.
+        """
+
     def setTarget(target):
         """Set this specification's target.
 

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-02-07 01:43:29 +0000
+++ lib/lp/blueprints/model/specification.py	2012-02-24 14:50:29 +0000
@@ -234,10 +234,73 @@
                     status=SpecificationWorkItemStatus.TODO, assignee=None,
                     milestone=None):
         """See ISpecification."""
+        if milestone is not None:
+            assert milestone.target == self.target, (
+                "%s does not belong to this spec's target (%s)" %
+                    (milestone.displayname, self.target.name))
         return SpecificationWorkItem(
             title=title, status=status, specification=self, assignee=assignee,
             milestone=milestone, sequence=sequence)
 
+    @property
+    def work_items(self):
+        """See ISpecification."""
+        return Store.of(self).find(
+            SpecificationWorkItem, specification=self,
+            deleted=False).order_by("sequence")
+
+    def _deleteWorkItemsNotMatching(self, titles):
+        """Delete all work items whose title does not match the given ones.
+
+        Also set the sequence of those deleted work items to -1.
+        """
+        for work_item in self.work_items:
+            if work_item.title not in titles:
+                work_item.deleted = True
+
+    def updateWorkItems(self, new_work_items):
+        """See ISpecification."""
+        # First mark work items with titles that are no longer present as
+        # deleted.
+        self._deleteWorkItemsNotMatching(
+            [wi['title'] for wi in new_work_items])
+        work_items = Store.of(self).find(
+            SpecificationWorkItem, specification=self, deleted=False)
+        work_items = list(work_items.order_by("sequence"))
+        # At this point the list of new_work_items is necessarily the same
+        # size (or longer) than the list of existing ones, so we can just
+        # iterate over it updating the existing items and creating any new
+        # ones.
+        to_insert = []
+        existing_titles = [wi.title for wi in work_items]
+        for i in range(len(new_work_items)):
+            new_wi = new_work_items[i]
+            if new_wi['title'] not in existing_titles:
+                # This is a new work item, so we insert it with 'i' as its
+                # sequence because that's the position it is on the list
+                # entered by the user.
+                to_insert.append((i, new_wi))
+            else:
+                # Get the existing work item with the same title and update
+                # it to match what we have now.
+                existing_wi = work_items[
+                    existing_titles.index(new_wi['title'])]
+                # Update the sequence to match its current position on the
+                # list entered by the user.
+                existing_wi.sequence = i
+                existing_wi.status = new_wi['status']
+                existing_wi.assignee = new_wi['assignee']
+                milestone = new_wi['milestone']
+                if milestone is not None:
+                    assert milestone.target == self.target, (
+                        "%s does not belong to this spec's target (%s)" %
+                            (milestone.displayname, self.target.name))
+                existing_wi.milestone = milestone
+
+        for sequence, item in to_insert:
+            self.newWorkItem(item['title'], sequence, item['status'],
+                             item['assignee'], item['milestone'])
+
     def setTarget(self, target):
         """See ISpecification."""
         if IProduct.providedBy(target):

=== modified file 'lib/lp/blueprints/model/tests/test_specification.py'
--- lib/lp/blueprints/model/tests/test_specification.py	2012-02-10 17:32:41 +0000
+++ lib/lp/blueprints/model/tests/test_specification.py	2012-02-24 14:50:29 +0000
@@ -5,7 +5,10 @@
 
 __metaclass__ = type
 
-from testtools.matchers import Equals
+from testtools.matchers import (
+    Equals,
+    MatchesStructure,
+    )
 
 from lp.app.validators import LaunchpadValidationError
 from lp.blueprints.interfaces.specification import ISpecification
@@ -145,6 +148,7 @@
 
 
 class TestSpecificationWorkItems(TestCaseWithFactory):
+    """Test the Workitem-related methods of ISpecification."""
 
     layer = DatabaseFunctionalLayer
 
@@ -163,7 +167,7 @@
         title = u'new-work-item'
         spec = self.factory.makeSpecification()
         assignee = self.factory.makePerson()
-        milestone = self.factory.makeMilestone()
+        milestone = self.factory.makeMilestone(product=spec.product)
         status = SpecificationWorkItemStatus.DONE
         login_person(spec.owner)
         work_item = spec.newWorkItem(
@@ -174,3 +178,146 @@
         self.assertEqual(status, work_item.status)
         self.assertEqual(title, work_item.title)
         self.assertEqual(milestone, work_item.milestone)
+
+    def test_work_items_property(self):
+        spec = self.factory.makeSpecification()
+        wi1 = self.factory.makeSpecificationWorkItem(
+            specification=spec, sequence=2)
+        wi2 = self.factory.makeSpecificationWorkItem(
+            specification=spec, sequence=1)
+        # This work item won't be included in the results of spec.work_items
+        # because it is deleted.
+        self.factory.makeSpecificationWorkItem(
+            specification=spec, sequence=3, deleted=True)
+        # This work item belongs to a different spec so it won't be returned
+        # by spec.work_items.
+        self.factory.makeSpecificationWorkItem()
+        self.assertEqual([wi2, wi1], list(spec.work_items))
+
+    def test_updateWorkItems_no_existing_items(self):
+        """When there are no existing work items, updateWorkItems will create
+        a new entry for every element in the list given to it.
+        """
+        spec = self.factory.makeSpecification(
+            product=self.factory.makeProduct())
+        milestone = self.factory.makeMilestone(product=spec.product)
+        work_item1_data = dict(
+            title=u'Foo Bar', status=SpecificationWorkItemStatus.DONE,
+            assignee=spec.owner, milestone=None)
+        work_item2_data = dict(
+            title=u'Bar Foo', status=SpecificationWorkItemStatus.TODO,
+            assignee=None, milestone=milestone)
+
+        # We start with no work items.
+        self.assertEquals([], list(spec.work_items))
+
+        login_person(spec.owner)
+        spec.updateWorkItems([work_item1_data, work_item2_data])
+
+        # And after calling updateWorkItems() we have 2 work items.
+        self.assertEqual(2, spec.work_items.count())
+
+        # The data dicts we pass to updateWorkItems() have no sequence because
+        # that's taken from their position on the list, so we update our data
+        # dicts with the sequence we expect our work items to have.
+        work_item1_data['sequence'] = 0
+        work_item2_data['sequence'] = 1
+
+        # Assert that the work items ultimately inserted in the DB are exactly
+        # what we expect them to be.
+        created_wi1, created_wi2 = list(spec.work_items)
+        self.assertThat(
+            created_wi1, MatchesStructure.byEquality(**work_item1_data))
+        self.assertThat(
+            created_wi2, MatchesStructure.byEquality(**work_item2_data))
+
+    def test_updateWorkItems_merges_with_existing_ones(self):
+        spec = self.factory.makeSpecification(
+            product=self.factory.makeProduct())
+        login_person(spec.owner)
+        # Create two work-items in our database.
+        wi1_data = self._createWorkItemAndReturnDataDict(spec)
+        wi2_data = self._createWorkItemAndReturnDataDict(spec)
+        self.assertEqual(2, spec.work_items.count())
+
+        # These are the work items we'll be inserting.
+        new_wi1_data = dict(
+            title=u'Some Title', status=SpecificationWorkItemStatus.TODO,
+            assignee=None, milestone=None)
+        new_wi2_data = dict(
+            title=u'Other title', status=SpecificationWorkItemStatus.TODO,
+            assignee=None, milestone=None)
+
+        # We want to insert the two work items above in the first and third
+        # positions respectively, so the existing ones to be moved around
+        # (e.g. have their sequence updated).
+        work_items = [new_wi1_data, wi1_data, new_wi2_data, wi2_data]
+        spec.updateWorkItems(work_items)
+
+        # Update our data dicts with the sequences we expect the work items in
+        # our DB to have.
+        new_wi1_data['sequence'] = 0
+        wi1_data['sequence'] = 1
+        new_wi2_data['sequence'] = 2
+        wi2_data['sequence'] = 3
+
+        self.assertEqual(4, spec.work_items.count())
+        for data, obj in zip(work_items, list(spec.work_items)):
+            self.assertThat(obj, MatchesStructure.byEquality(**data))
+
+    def test_updateWorkItems_updates_existing_ones(self):
+        spec = self.factory.makeSpecification()
+        login_person(spec.owner)
+        # Create a work-item in our database.
+        wi_data = self._createWorkItemAndReturnDataDict(spec)
+        self.assertEqual(1, spec.work_items.count())
+
+        # This time we're only changing the existing work item; we'll change
+        # its assignee and status.
+        wi_data.update(dict(status=SpecificationWorkItemStatus.DONE,
+                            assignee=spec.owner))
+        spec.updateWorkItems([wi_data])
+
+        self.assertEqual(1, spec.work_items.count())
+        self.assertThat(
+            spec.work_items[0], MatchesStructure.byEquality(**wi_data))
+
+    def test_updateWorkItems_deletes_all_if_given_empty_list(self):
+        work_item = self.factory.makeSpecificationWorkItem()
+        spec = work_item.specification
+        self.assertEqual(1, spec.work_items.count())
+        spec.updateWorkItems([])
+        self.assertEqual(0, spec.work_items.count())
+
+    def test_updateWorkItems_marks_removed_ones_as_deleted(self):
+        spec = self.factory.makeSpecification()
+        self._createWorkItemAndReturnDataDict(spec)
+        wi2_data = self._createWorkItemAndReturnDataDict(spec)
+        self.assertEqual(2, spec.work_items.count())
+        login_person(spec.owner)
+
+        # We have two work items in the DB but now we want to update them to
+        # keep just the second one. The first will be deleted and the sequence
+        # of the second will be changed.
+        spec.updateWorkItems([wi2_data])
+        self.assertEqual(1, spec.work_items.count())
+        wi2_data['sequence'] = 0
+        self.assertThat(
+            spec.work_items[0], MatchesStructure.byEquality(**wi2_data))
+
+    def _createWorkItemAndReturnDataDict(self, spec):
+        """Create a new work item for the given spec using the next available
+        sequence number.
+
+        Return a dict with the title, status, assignee, milestone and sequence
+        attributes of the spec.
+        """
+        if spec.work_items.count() == 0:
+            sequence = 0
+        else:
+            sequence = max(wi.sequence for wi in spec.work_items) + 1
+        wi = self.factory.makeSpecificationWorkItem(
+            specification=spec, sequence=sequence)
+        return dict(
+            title=wi.title, status=wi.status, assignee=wi.assignee,
+            milestone=wi.milestone, sequence=sequence)

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2012-01-16 00:07:45 +0000
+++ lib/lp/registry/model/milestone.py	2012-02-24 14:50:29 +0000
@@ -366,7 +366,7 @@
     """A virtual milestone implementation for project.
 
     The current database schema has no formal concept of milestones related to
-    projects. A milestone named `milestone` is considererd to belong to
+    projects. A milestone named `milestone` is considered to belong to
     a project if the project contains at least one product with a milestone
     of the same name. A project milestone is considered to be active if at
     least one product milestone with the same name is active.  The