launchpad-reviewers team mailing list archive

[James Tunnicliffe <launchpad@xxxxxxxxxxxxx>]

James Tunnicliffe has proposed merging lp:~dooferlad/launchpad/workitems-bugfix into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1004416 in Launchpad itself: "Work Items not allowing users to edit them properly"
  https://bugs.launchpad.net/launchpad/+bug/1004416

For more details, see:
https://code.launchpad.net/~dooferlad/launchpad/workitems-bugfix/+merge/110563

-- Summary
Bug Fix for #1004416
The existing work item editor code assumed that work items would be uniquely identifiable by title alone. When inserting a new work item you could insert a duplicate, but any changes to it would only apply to the first work item that the code found. Deleting one of those work items would fail - you had to delete both.

-- Proposed fix
Modify logic that takes work item updates to not just use work item name as the only identifier.

-- Pre-implementation notes
None.

-- Implementation details
Since the work item editor is still a text box, the work item name is still used to identify a work item, but the code takes care to only match a work item for update or deletion once. This resolves the problems seen.

-- LOC Rationale
Added quite a few lines, though. These will be more than offset by:
https://code.launchpad.net/~danilo/launchpad/kill-feedback-requests/+merge/106119

-- Tests
test_duplicate_work_items in lib/lp/blueprints/model/tests/test_specificatin.py

-- Demo and Q/A
1. In a dev instance run http://paste.ubuntu.com/992291/ to generate some work items
2. Visit https://blueprints.launchpad.dev/linaro/+spec/name-100098 and create a copy of the last work item. Previously this wouldn’t work. Now delete it. It should be deleted. Previously you had to delete all work items with the same name.

-- Lint
None.
-- 
https://code.launchpad.net/~dooferlad/launchpad/workitems-bugfix/+merge/110563
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~dooferlad/launchpad/workitems-bugfix into lp:launchpad.

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-05-17 07:46:56 +0000
+++ lib/lp/blueprints/model/specification.py	2012-06-15 16:19:22 +0000
@@ -289,10 +289,21 @@
 
         Also set the sequence of those deleted work items to -1.
         """
+        title_counts = {}
+        for title in titles:
+            if title not in title_counts:
+                title_counts[title] = 1
+            else:
+                title_counts[title] += 1
+
         for work_item in self.work_items:
-            if work_item.title not in titles:
+            if(work_item.title not in title_counts or
+               title_counts[work_item.title] == 0):
                 work_item.deleted = True
 
+            elif title_counts[work_item.title] > 0:
+                title_counts[work_item.title] -= 1
+
     def updateWorkItems(self, new_work_items):
         """See ISpecification."""
         # First mark work items with titles that are no longer present as
@@ -308,18 +319,27 @@
         # ones.
         to_insert = []
         existing_titles = [wi.title for wi in work_items]
+        existing_title_count = {}
+        for title in existing_titles:
+            if not title in existing_title_count:
+                existing_title_count[title] = 1
+            else:
+                existing_title_count[title] += 1
+
         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.
+            if(new_wi['title'] not in existing_titles or
+               new_wi['title'] in existing_title_count and
+               existing_title_count[new_wi['title']] == 0):
                 to_insert.append((i, new_wi))
             else:
-                # Get the existing work item with the same title and update
+                existing_title_count[new_wi['title']] -= 1
+                # Get an 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'])]
+                existing_wi_index = existing_titles.index(new_wi['title'])
+                existing_wi = work_items[existing_wi_index]
+                # Mark a work item as dirty - don't use it again this update.
+                existing_titles[existing_wi_index] = None
                 # Update the sequence to match its current position on the
                 # list entered by the user.
                 existing_wi.sequence = i

=== modified file 'lib/lp/blueprints/model/tests/test_specification.py'
--- lib/lp/blueprints/model/tests/test_specification.py	2012-03-26 14:23:39 +0000
+++ lib/lp/blueprints/model/tests/test_specification.py	2012-06-15 16:19:22 +0000
@@ -494,6 +494,57 @@
         for data, obj in zip(work_items, list(spec.work_items)):
             self.assertThat(obj, MatchesStructure.byEquality(**data))
 
+    def test_duplicate_work_items(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)
+
+        # These are the work items we'll be inserting.
+        new_wi1_data = dict(title=u'Some Title', assignee=None, milestone=None,
+                            status=SpecificationWorkItemStatus.TODO)
+        new_wi2_data = dict(title=u'Some Title', assignee=None, milestone=None,
+                            status=SpecificationWorkItemStatus.DONE)
+
+        # 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))
+
+        # Test that we can insert anotehr duplicate work item.
+        new_wi3_data = dict(title=u'Some Title', assignee=None, milestone=None,
+                                      status=SpecificationWorkItemStatus.TODO)
+        new_wi3_data['sequence'] = 4
+        work_items.append(new_wi3_data)
+        spec.updateWorkItems(work_items)
+
+        self.assertEqual(5, spec.work_items.count())
+        for data, obj in zip(work_items, list(spec.work_items)):
+            self.assertThat(obj, MatchesStructure.byEquality(**data))
+
+        # Now delete one of the duplicated work items. It, and only it,
+        # should be deleted from the work item list.
+        work_items.pop()
+        spec.updateWorkItems(work_items)
+
+        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)