launchpad-reviewers team mailing list archive

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

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1021196 in Launchpad itself: "Milestone page doesn't list BPs with only some workitems targetted to the milestone"
  https://bugs.launchpad.net/launchpad/+bug/1021196

For more details, see:
https://code.launchpad.net/~danilo/launchpad/milestone-all-bps/+merge/113531

= Bug #1021196: show partialy targeted blueprints in milestones =

When a certain blueprint has work items targeted to a milestone, but is itself not targeted to it, it currently doesn't show up in a milestone page.

It should, with a note how it's not targeted to be completed in a milestone.  This should make multi-milestone blueprints much more useful.

== Proposed fix ==

Refactor Milestone.specifications and ProjectMilestone.specifications to use Storm and look into SpecificationWorkItems as well.

Prejoins to Person table are replaced with LeftJoins and Persons are collected as well, but stripped out using DecoratedResultSet.

I've added a simple text "(some work for this milestone)" for blueprints which are not targeted to the milestone you are looking at, but I didn't add any pagetests for this (and since I dislike page tests, I'd rather not add them).  I've also used "python:" in TAL because I'd have to create a decorated Specification object to use if I wanted to put this in a simple tal:condition (which is too much work, but would allow easier testing).

== LOC Rationale ==

This is part of https://dev.launchpad.net/Projects/WorkItems

As documented in

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

we are still at -407 lines before this branch.

== Tests ==

For added tests:
  bin/test -cvvt MilestonesContainsPartialSpecifications

For all on milestones:
  bin/test -cvvt milestone

== Demo and Q/A ==

Create a project with two milestones ("other" and "this").
Add a project to project group.

Create a BP on the project with two workitems targeted to "this"
milestone.  Target a blueprint to "other" milestone.

Create another BP on the project with a single workitem and target
the blueprint to "this" milestone.

Load both project and project-group milestone pages for the "this" milestone
and ensure both BPs show up.

Load milestone pages for "other" milestone and ensure only the first BP
shows up.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/templates/milestone-index.pt
  lib/lp/registry/tests/test_milestone.py
  lib/lp/registry/model/milestone.py
-- 
https://code.launchpad.net/~danilo/launchpad/milestone-all-bps/+merge/113531
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/milestone-all-bps into lp:launchpad.

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2012-02-17 22:46:02 +0000
+++ lib/lp/registry/model/milestone.py	2012-07-05 10:00:13 +0000
@@ -23,12 +23,15 @@
     BoolCol,
     DateCol,
     ForeignKey,
-    SQLMultipleJoin,
     StringCol,
     )
-from storm.locals import (
+from storm.locals import Store
+from storm.expr import (
     And,
-    Store,
+    Desc,
+    Join,
+    LeftJoin,
+    Or,
     )
 from storm.zope import IResultSet
 from zope.component import getUtility
@@ -36,6 +39,7 @@
 
 from lp.app.errors import NotFoundError
 from lp.blueprints.model.specification import Specification
+from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
 from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
 from lp.bugs.interfaces.bugtarget import IHasBugs
 from lp.bugs.interfaces.bugtask import (
@@ -55,11 +59,9 @@
     IProjectGroupMilestone,
     )
 from lp.registry.model.productrelease import ProductRelease
+from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.lpstorm import IStore
-from lp.services.database.sqlbase import (
-    SQLBase,
-    sqlvalues,
-    )
+from lp.services.database.sqlbase import SQLBase
 from lp.services.webapp.sorting import expand_numbers
 
 
@@ -186,10 +188,31 @@
     summary = StringCol(notNull=False, default=None)
     code_name = StringCol(dbName='codename', notNull=False, default=None)
 
-    specifications = SQLMultipleJoin('Specification', joinColumn='milestone',
-        orderBy=['-priority', 'definition_status',
-                 'implementation_status', 'title'],
-        prejoins=['assignee'])
+    @property
+    def specifications(self):
+        from lp.registry.model.person import Person
+        store = Store.of(self)
+        origin = [
+            Specification,
+            LeftJoin(
+                SpecificationWorkItem,
+                SpecificationWorkItem.specification_id == Specification.id),
+            LeftJoin(Person, Specification.assigneeID == Person.id),
+            ]
+
+        results = store.using(*origin).find(
+            (Specification, Person),
+            Or(Specification.milestoneID == self.id,
+               SpecificationWorkItem.milestone_id == self.id),
+            Or(SpecificationWorkItem.deleted is None,
+               SpecificationWorkItem.deleted == False))
+        results.config(distinct=True)
+        ordered_results = results.order_by(Desc(Specification.priority),
+                                           Specification.definition_status,
+                                           Specification.implementation_status,
+                                           Specification.title)
+        mapper = lambda row: row[0]
+        return DecoratedResultSet(ordered_results, mapper)
 
     @property
     def target(self):
@@ -397,18 +420,35 @@
 
     @property
     def specifications(self):
-        """See `IMilestone`."""
-        return Specification.select(
-            """milestone IN
-                (SELECT milestone.id
-                    FROM Milestone, Product
-                    WHERE Milestone.Product = Product.id
-                    AND Milestone.name = %s
-                    AND Product.project = %s)
-            """ % sqlvalues(self.name, self.target),
-            orderBy=['-priority', 'definition_status',
-                     'implementation_status', 'title'],
-            prejoins=['assignee'])
+        """See `IMilestoneData`."""
+        from lp.registry.model.person import Person
+        from lp.registry.model.product import Product
+        store = Store.of(self.target)
+        origin = [
+            Specification,
+            LeftJoin(
+                SpecificationWorkItem,
+                SpecificationWorkItem.specification_id == Specification.id),
+            Join(Milestone,
+                 Or(Milestone.id == Specification.milestoneID,
+                    Milestone.id == SpecificationWorkItem.milestone_id)),
+            Join(Product, Product.id == Milestone.productID),
+            LeftJoin(Person, Specification.assigneeID == Person.id),
+            ]
+
+        results = store.using(*origin).find(
+            (Specification, Person),
+            Product.projectID == self.target.id,
+            Milestone.name == self.name,
+            Or(SpecificationWorkItem.deleted is None,
+               SpecificationWorkItem.deleted == False))
+        results.config(distinct=True)
+        ordered_results = results.order_by(Desc(Specification.priority),
+                                           Specification.definition_status,
+                                           Specification.implementation_status,
+                                           Specification.title)
+        mapper = lambda row: row[0]
+        return DecoratedResultSet(ordered_results, mapper)
 
     @property
     def displayname(self):

=== modified file 'lib/lp/registry/templates/milestone-index.pt'
--- lib/lp/registry/templates/milestone-index.pt	2012-06-11 00:03:25 +0000
+++ lib/lp/registry/templates/milestone-index.pt	2012-07-05 10:00:13 +0000
@@ -252,6 +252,16 @@
                          title spec/summary/fmt:shorten/400">Foo Bar Baz</a>
                   <img src="/@@/info" alt="Informational"
                        tal:condition="spec/informational" />
+                  <tal:comment condition="nothing">
+                    Compare milestone names (important for project group
+                    milestones, since they are not DB-backed Milestone objects
+                    and actually consist of a number of them with the same
+                    names) to see if a blueprint is only partially targeted to
+                    this milestone.
+                  </tal:comment>
+                  <span tal:condition="
+                      python:spec.milestone.name != context.name">
+                    (some work for this milestone)</span>
                 </td>
                 <td tal:condition="view/is_project_milestone">
                     <span class="sortkey" tal:content="spec/product/displayname" />

=== modified file 'lib/lp/registry/tests/test_milestone.py'
--- lib/lp/registry/tests/test_milestone.py	2012-02-28 04:24:19 +0000
+++ lib/lp/registry/tests/test_milestone.py	2012-07-05 10:00:13 +0000
@@ -175,3 +175,62 @@
             product=self.product,
             )
         self.assertContentEqual(specifications, self.milestone.specifications)
+
+
+class MilestonesContainsPartialSpecifications(TestCaseWithFactory):
+    """Milestones list specifications with some workitems targeted to it."""
+
+    layer = DatabaseFunctionalLayer
+
+    def _create_milestones_on_target(self, **kwargs):
+        """Create a milestone on a target with work targeted to it.
+
+        Target should be specified using either product or distribution
+        argument which is directly passed into makeMilestone call.
+        """
+        other_milestone = self.factory.makeMilestone(**kwargs)
+        target_milestone = self.factory.makeMilestone(**kwargs)
+        specification = self.factory.makeSpecification(
+            milestone=other_milestone, **kwargs)
+        # Create two workitems to ensure this doesn't cause
+        # two specifications to be returned.
+        self.factory.makeSpecificationWorkItem(
+            specification=specification, milestone=target_milestone)
+        self.factory.makeSpecificationWorkItem(
+            specification=specification, milestone=target_milestone)
+        return specification, target_milestone
+
+    def test_milestones_on_product(self):
+        specification, target_milestone = self._create_milestones_on_target(
+            product=self.factory.makeProduct())
+        self.assertEqual([specification],
+                         list(target_milestone.specifications))
+
+    def test_milestones_on_distribution(self):
+        specification, target_milestone = self._create_milestones_on_target(
+            distribution=self.factory.makeDistribution())
+        self.assertEqual([specification],
+                         list(target_milestone.specifications))
+
+    def test_milestones_on_project(self):
+        # A Project (Project Group) milestone contains all specifications
+        # targetted to contained Products (Projects) for milestones of
+        # a certain name.
+        projectgroup = self.factory.makeProject()
+        product = self.factory.makeProduct(project=projectgroup)
+        specification, target_milestone = self._create_milestones_on_target(
+            product=product)
+        milestone = projectgroup.getMilestone(name=target_milestone.name)
+        self.assertEqual([specification],
+                         list(milestone.specifications))
+
+    def test_milestones_with_deleted_workitems(self):
+        # Deleted work items do not cause the specification to show up
+        # in the milestone page.
+        milestone = self.factory.makeMilestone(
+            product=self.factory.makeProduct())
+        specification = self.factory.makeSpecification(
+            milestone=milestone, product=milestone.product)
+        self.factory.makeSpecificationWorkItem(
+            specification=specification, milestone=milestone, deleted=True)
+        self.assertEqual([], list(milestone.specifications))