Merge lp:~danilo/launchpad/bug-1000787 into lp:launchpad

[Данило Шеган]

Upcoming work view should split out blueprint work items into appropriate milestone blocks.

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

== LOC rationale ==

All branches so far:
  https://docs.google.com/a/linaro.org/spreadsheet/ccc?key=0AvOsYPy8e7yUdGkyRmx2WGFwT3NnSjdHVW04Q1pvSmc

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

[Francesco Banconi (frankban)]

The code looks good, tests pass, thank you.
Just a suggestion: maybe the code in lines 9..11 could be simplified, e.g.::

    container = containers_by_spec.setdefault(milestone, {}).get(spec)

review: Approve (code*)

[Raphaël Badin (rvb)]

Looks good.

[0]

53 + self.assertContentEqual([spec],
54 + [container.spec

56 + self.assertContentEqual([current_workitem],
57 + [item.actual_workitem

63 + self.assertContentEqual([spec],
64 + [container.spec

66 + self.assertContentEqual([future_workitem],
67 + [item.actual_workitem

Weird line wrapping here.

review: Approve

[Данило Шеган (danilo)]

Francesco, thanks for the review, and indeed, that's good suggestion, I'll use that.

Raphael (thanks as well), what line wrapping would you suggest instead? I've used something that should be a bit more conservative now :)

Also, if the changes make you happy, can you please submit this for landing?

=== 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 16:44:19 +0000
@@ -4701,10 +4701,10 @@
             milestone = spec.milestone
         if milestone.dateexpected not in containers_by_date:
             containers_by_date[milestone.dateexpected] = []
-        container = containers_by_spec.get(spec)
+        container = containers_by_spec.setdefault(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 16:44:19 +0000
@@ -127,6 +127,48 @@
         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):