launchpad-reviewers team mailing list archive

[Данило Шеган <danilo@xxxxxxxxxxxxx>]

Данило Шеган has proposed merging lp:~danilo/launchpad/bug-1000787 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1000787 in Launchpad itself: "ZeroDivisionError on  launchpad.net/~linaro/+upcomingwork"
  https://bugs.launchpad.net/launchpad/+bug/1000787

For more details, see:
https://code.launchpad.net/~danilo/launchpad/bug-1000787/+merge/107041

= Bug 1000787: incorrect grouping of workitems in different milestones =

Initially, this bug was about a ZeroDivisionError, where we were dividing by zero as a total number of work items in a single "date grouping".  That should have been impossible, so James Tunnicliffe just guarded against division-by-zero in one of his other branches, and that helped uncover where the bug was.

A single blueprint can have workitems targetted to multiple milestones.  However, due to code structure, all of them ended up in the same blueprint-container that could only go under one single date.

== Proposed fix ==

The change is to make by-blueprint (specification) grouping be also by milestone first, so we would create a spec-container in each of the milestones that require it.

== Tests ==

bin/test -cvvt person_upcoming

== Demo and Q/A ==

 1. Create a BP, target it to a certain milestone (with the date set)
 2. Add two workitems, one in default milestone; another in a different milestone with a different date (use "Work items for milestone-name:" in the work-items edit box to do that)
 3. Assign the BP to a person/team
 4. Go to that person's/team's upcomingwork page
    This should show up work-items/blueprints appropriately split over two milestones

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/browser/tests/test_person_upcomingwork.py
  lib/lp/registry/browser/person.py
-- 
https://code.launchpad.net/~danilo/launchpad/bug-1000787/+merge/107041
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-1000787 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2012-05-22 15:04:48 +0000
+++ lib/lp/registry/browser/person.py	2012-05-23 15:21:21 +0000
@@ -4701,10 +4701,12 @@
             milestone = spec.milestone
         if milestone.dateexpected not in containers_by_date:
             containers_by_date[milestone.dateexpected] = []
-        container = containers_by_spec.get(spec)
+        if containers_by_spec.get(milestone) is None:
+            containers_by_spec[milestone] = {}
+        container = containers_by_spec.get(milestone).get(spec)
         if container is None:
             container = SpecWorkItemContainer(spec)
-            containers_by_spec[spec] = container
+            containers_by_spec[milestone][spec] = container
             containers_by_date[milestone.dateexpected].append(container)
         container.append(GenericWorkItem.from_workitem(workitem))
 

=== modified file 'lib/lp/registry/browser/tests/test_person_upcomingwork.py'
--- lib/lp/registry/browser/tests/test_person_upcomingwork.py	2012-05-22 15:23:25 +0000
+++ lib/lp/registry/browser/tests/test_person_upcomingwork.py	2012-05-23 15:21:21 +0000
@@ -127,6 +127,49 @@
         self.assertEqual(1, len(container.items))
         self.assertEqual(current_wi, container.items[0].actual_workitem)
 
+    def test_multiple_milestone_separation(self):
+        # A single blueprint with workitems targetted to multiple
+        # milestones is processed so that the same blueprint appears
+        # in both with only the relevant work items.
+        spec = self.factory.makeSpecification(
+            product=self.current_milestone.product,
+            assignee=self.team.teamowner)
+        current_workitem = self.factory.makeSpecificationWorkItem(
+            title=u'workitem 1', specification=spec,
+            milestone=self.current_milestone)
+        future_workitem = self.factory.makeSpecificationWorkItem(
+            title=u'workitem 2', specification=spec,
+            milestone=self.future_milestone)
+
+        workitems = getWorkItemsDueBefore(
+            self.team, self.future_milestone.dateexpected, user=None)
+
+        # Both milestone dates are present in the returned results.
+        self.assertContentEqual(
+            [self.current_milestone.dateexpected,
+             self.future_milestone.dateexpected],
+            workitems.keys())
+
+        # Current milestone date has a single specification
+        # with only the matching work item.
+        containers_current = workitems[self.current_milestone.dateexpected]
+        self.assertContentEqual([spec],
+                                [container.spec
+                                 for container in containers_current])
+        self.assertContentEqual([current_workitem],
+                                [item.actual_workitem
+                                 for item in containers_current[0].items])
+
+        # Future milestone date has the same specification
+        # containing only the work item targetted to future.
+        containers_future = workitems[self.future_milestone.dateexpected]
+        self.assertContentEqual([spec],
+                                [container.spec
+                                 for container in containers_future])
+        self.assertContentEqual([future_workitem],
+                                [item.actual_workitem
+                                 for item in containers_future[0].items])
+
 
 class TestGenericWorkItem(TestCaseWithFactory):