launchpad-reviewers team mailing list archive

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

Данило Шеган has proposed merging lp:~danilo/launchpad/bug-1021196 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 targeted to the milestone"
  https://bugs.launchpad.net/launchpad/+bug/1021196

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

= Bug 1021196: take 2 =

Original MP up at https://code.launchpad.net/~danilo/launchpad/milestone-all-bps/+merge/113531

This fixes the OOPS as noticed by Steven in QA (https://oops.canonical.com/oops/?oopsid=OOPS-7ec83b58aab8ad14b1b194bf93116da6).

Only revisions r15585 and r15586 are relevant (r15587 is lint fixes) — diff containing both on https://pastebin.canonical.com/69731/.

== Tests ==

bin/test -cvvt TestMilestoneViews.test_distroseries_milestone

(or "bin/test -cvvt milestone" for all milestone related tests)

-- 
https://code.launchpad.net/~danilo/launchpad/bug-1021196/+merge/114157
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~danilo/launchpad/bug-1021196 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/tests/test_milestone.py'
--- lib/lp/registry/browser/tests/test_milestone.py	2012-06-11 00:47:38 +0000
+++ lib/lp/registry/browser/tests/test_milestone.py	2012-07-10 10:43:25 +0000
@@ -18,8 +18,6 @@
 from lp.services.config import config
 from lp.services.webapp import canonical_url
 from lp.testing import (
-    ANONYMOUS,
-    login,
     login_person,
     login_team,
     logout,
@@ -40,6 +38,25 @@
 
     layer = DatabaseFunctionalLayer
 
+    def test_distroseries_milestone(self):
+        # Distribution milestone with an untargeted blueprint containing
+        # work items targeted to the milestone lists this blueprint
+        # with a special note.
+        distro_series = self.factory.makeDistroSeries()
+        distribution = distro_series.distribution
+        milestone = self.factory.makeMilestone(distroseries=distro_series)
+        specification = self.factory.makeSpecification(
+            distribution=distribution)
+        self.factory.makeSpecificationWorkItem(
+            specification=specification, milestone=milestone)
+        view = create_initialized_view(milestone, '+index')
+        self.assertIn('some work for this milestone', view.render())
+
+
+class TestAddMilestoneViews(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
     def setUp(self):
         TestCaseWithFactory.setUp(self)
         self.product = self.factory.makeProduct()

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2012-07-09 03:15:32 +0000
+++ lib/lp/registry/model/milestone.py	2012-07-10 10:43:25 +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 == 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 == 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-07-09 03:15:32 +0000
+++ lib/lp/registry/templates/milestone-index.pt	2012-07-10 10:43:25 +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 to see if a blueprint is only
+                    partially targeted to this milestone.
+
+                    If a blueprint is untargeted, then it's partial as well.
+                  </tal:comment>
+                  <span tal:condition="
+                      python:(not spec.milestone or
+                              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-07-09 03:15:32 +0000
+++ lib/lp/registry/tests/test_milestone.py	2012-07-10 10:43:25 +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))