Merge lp:~dooferlad/launchpad/workitems-bugfix into lp:launchpad

[James Tunnicliffe]

Bug Fix for bug #1004416. Work item editor now handles duplicate work items correctly.

-- 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.

[Brad Crittenden (bac)]

Hi James,

Thanks for making this change. I hadn't paid a lot of attention to the new work items feature of blueprints so it was interesting to learn a little about them.

* The logic to populate a dictionary with the count of items in a list is pretty generic and repeated. Perhaps you could factor that out into a method.

* Please change "if(" to "if (" at line 17 and elsewhere.

* I think "if not title in existing_title_count" reads better as "if title not in existing_title_count", as you'd written it previously.

* In the following

   if(new_wi['title'] not in existing_titles or
      new_wi['title'] in existing_title_count and
      existing_title_count[new_wi['title']] == 0):

the second line is redundant as you just established the title is in existing_titles so it must also appear in existing_title_count.

* I know you didn't write this line, but

for i in range(len(new_work_items)):
    new_wi = new_work_items[i]

would be easier to follow if it was done like

for i, new_wi in enumerate(new_work_items):

* You have a typo "anotehr".

* In your test, it might be easier to understand if your duplicate items were copied from the first one, i.e.

from copy import copy
new_w1_data = dict(...)
new_w2_data = copy(new_w1_data)
...
new_w3_data = copy(new_w1_data)

A reader will know instantly what is going on without having to look at every item in the dicts to verify they are the same.

Finally I'll note this test is very busy. Ideally each test would exercise one concept. Perhaps you could break this test up into one that shows adding duplicates works and another that proves deleting will work. That separation comes at the price of refactoring or (worse) duplicate set up code.

I'd like to go ahead and mark this proposal 'Approved', pending the mentioned changes being made.

Thanks for the nice branch.

review: Approve (code)

[James Tunnicliffe (dooferlad)]

I have pushed a change that addresses these concerns. I don't have commit access, so please kick off the test and check in cycle.

[Brad Crittenden (bac)]

Thanks for the changes James. I'll land this branch for you now.

=== 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-18 11:22:27 +0000
@@ -289,10 +289,25 @@
 
         Also set the sequence of those deleted work items to -1.
         """
+        title_counts = self._list_to_dict_of_frequency(titles)
+
         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 _list_to_dict_of_frequency(self, list):
+        dictionary = {}
+        for item in list:
+            if not item in dictionary:
+                dictionary[item] = 1
+            else:
+                dictionary[item] += 1
+        return dictionary
+
     def updateWorkItems(self, new_work_items):
         """See ISpecification."""
         # First mark work items with titles that are no longer present as
@@ -308,18 +323,20 @@
         # 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.
+        existing_title_count = self._list_to_dict_of_frequency(existing_titles)
+
+        for i, new_wi in enumerate(new_work_items):
+            if (new_wi['title'] not in existing_titles or
+                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-18 11:22:27 +0000
@@ -494,6 +494,57 @@
         for data, obj in zip(work_items, list(spec.work_items)):
             self.assertThat(obj, MatchesStructure.byEquality(**data))
 
+    def _dup_work_items_set_up(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)
+
+        # Create a duplicate and a near duplicate, insert into DB.
+        new_wi1_data = wi2_data.copy()
+        new_wi2_data = new_wi1_data.copy()
+        new_wi2_data['status'] = SpecificationWorkItemStatus.DONE
+        work_items = [new_wi1_data, wi1_data, new_wi2_data, wi2_data]
+        spec.updateWorkItems(work_items)
+
+        # Update our data dicts with the sequences to match data in DB
+        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))
+
+        return spec, work_items
+
+    def test_add_duplicate_work_item(self):
+        spec, work_items = self._dup_work_items_set_up()
+
+        # Test that we can insert another duplicate work item.
+        new_wi3_data = work_items[0].copy()
+        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))
+
+    def test_delete_duplicate_work_item(self):
+        spec, work_items = self._dup_work_items_set_up()
+
+        # Delete a duplicate work item
+        work_items.pop()
+        spec.updateWorkItems(work_items)
+
+        self.assertEqual(3, 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)