Merge lp:~adeuring/launchpad/bug-sorting into lp:launchpad

[Abel Deuring]

[r=deryck][no-qa] bugtask sorting by milestone name

This branch allows to sort bug searches results by milestone name.
(part of https://dev.launchpad.net/LEP/CustomBugListings)

This is basically a boilerplate addition of another entry to
the dictionary BugTaskSet._ORDERBY_COLUMN, which maps query strings
specifying the order to table columns.

Since the milestone table is normally not included in the FROM clause
of an SQL query, I added slightly more complex data to this dictionary:
Aside from the sort expression, the entry for "milestone_name" contains
also the class Milestone itself and a LeftJoin. This data is used to
include the join in the list join_tables, which in turn used to build
the FROM clause.

I also explicitly added a "second order" sorting, since milestones are
not the most fine grained sort option: By default, the "second level
sort order" would be bug.id or bugtask.id (depending on the search target),
and this is in many use cases probably not very helpful.

test:

./bin/test bugs -vvt test_bugtask_search.*test_sort_by_milestone_name

no lint

[Deryck Hodge (deryck)]

Looks good.

review: Approve (code)

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-11-01 05:30:11 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-11-14 21:39:30 +0000
@@ -2298,7 +2298,8 @@
                 "BugTask.datecreated > %s" % (
                     sqlvalues(params.created_since,)))
 
-        orderby_arg = self._processOrderBy(params)
+        orderby_arg, extra_joins = self._processOrderBy(params)
+        join_tables.extend(extra_joins)
 
         query = " AND ".join(extra_clauses)
 
@@ -3160,6 +3161,8 @@
 
     def getOrderByColumnDBName(self, col_name):
         """See `IBugTaskSet`."""
+        # Avoid circular imports.
+        from lp.registry.model.milestone import Milestone
         if BugTaskSet._ORDERBY_COLUMN is None:
             # Local import of Bug to avoid import loop.
             from lp.bugs.model.bug import Bug
@@ -3182,6 +3185,10 @@
                 "users_affected_count": Bug.users_affected_count,
                 "heat": BugTask.heat,
                 "latest_patch_uploaded": Bug.latest_patch_uploaded,
+                "milestone_name": (
+                    Milestone.name,
+                    (Milestone,
+                     LeftJoin(Milestone, BugTask.milestone == Milestone.id))),
                 }
         return BugTaskSet._ORDERBY_COLUMN[col_name]
 
@@ -3230,16 +3237,37 @@
 
         # Translate orderby keys into corresponding Table.attribute
         # strings.
+        extra_joins = []
         ambiguous = True
+        # Sorting by milestone only is a very "coarse" sort order.
+        # If no additional sort order is specified, add the bug task
+        # importance as a secondary sort order.
+        if len(orderby) == 1:
+            if orderby[0] == 'milestone_name':
+                # We want the most important bugtasks first; these have
+                # larger integer values.
+                orderby.append('-importance')
+            elif orderby[0] == '-milestone_name':
+                orderby.append('importance')
+            else:
+                # Other sort orders don't need tweaking.
+                pass
+
         for orderby_col in orderby:
             if isinstance(orderby_col, SQLConstant):
                 orderby_arg.append(orderby_col)
                 continue
             if orderby_col.startswith("-"):
                 col = self.getOrderByColumnDBName(orderby_col[1:])
+                if isinstance(col, tuple):
+                    extra_joins.append(col[1])
+                    col = col[0]
                 order_clause = Desc(col)
             else:
                 col = self.getOrderByColumnDBName(orderby_col)
+                if isinstance(col, tuple):
+                    extra_joins.append(col[1])
+                    col = col[0]
                 order_clause = col
             if col in unambiguous_cols:
                 ambiguous = False
@@ -3251,7 +3279,7 @@
             else:
                 orderby_arg.append(BugTask.id)
 
-        return tuple(orderby_arg)
+        return tuple(orderby_arg), extra_joins
 
     def getBugCountsForPackages(self, user, packages):
         """See `IBugTaskSet`."""

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2011-10-31 02:15:10 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2011-11-14 21:39:30 +0000
@@ -600,6 +600,16 @@
         params = self.getBugTaskSearchParams(user=None, has_cve=True)
         self.assertSearchFinds(params, self.bugtasks[:1])
 
+    def test_sort_by_milestone_name(self):
+        expected = self.setUpMilestoneSorting()
+        params = self.getBugTaskSearchParams(
+            user=None, orderby='milestone_name')
+        self.assertSearchFinds(params, expected)
+        expected.reverse()
+        params = self.getBugTaskSearchParams(
+            user=None, orderby='-milestone_name')
+        self.assertSearchFinds(params, expected)
+
 
 class DeactivatedProductBugTaskTestCase(TestCaseWithFactory):
 
@@ -791,6 +801,16 @@
         # main bug target.
         return self._findBugtaskForOtherProduct(bugtask, self.searchtarget)
 
+    def setUpMilestoneSorting(self):
+        with person_logged_in(self.owner):
+            milestone_1 = self.factory.makeMilestone(
+                product=self.searchtarget, name='1.0')
+            milestone_2 = self.factory.makeMilestone(
+                product=self.searchtarget, name='2.0')
+            self.bugtasks[1].transitionToMilestone(milestone_1, self.owner)
+            self.bugtasks[2].transitionToMilestone(milestone_2, self.owner)
+        return self.bugtasks[1:] + self.bugtasks[:1]
+
 
 class ProductSeriesTarget(BugTargetTestBase):
     """Use a product series as the bug target."""
@@ -838,6 +858,16 @@
         return self._findBugtaskForOtherProduct(
             bugtask, self.searchtarget.product)
 
+    def setUpMilestoneSorting(self):
+        with person_logged_in(self.owner):
+            milestone_1 = self.factory.makeMilestone(
+                productseries=self.searchtarget, name='1.0')
+            milestone_2 = self.factory.makeMilestone(
+                productseries=self.searchtarget, name='2.0')
+            self.bugtasks[1].transitionToMilestone(milestone_1, self.owner)
+            self.bugtasks[2].transitionToMilestone(milestone_2, self.owner)
+        return self.bugtasks[1:] + self.bugtasks[:1]
+
 
 class ProjectGroupTarget(BugTargetTestBase, BugTargetWithBugSuperVisor,
                          ProjectGroupAndDistributionTests):
@@ -947,6 +977,16 @@
             # With a flag set, no bugs are found.
             self.assertSearchFinds(params, [])
 
+    def setUpMilestoneSorting(self):
+        with person_logged_in(self.owner):
+            milestone_1 = self.factory.makeMilestone(
+                product=self.bugtasks[1].target, name='1.0')
+            milestone_2 = self.factory.makeMilestone(
+                product=self.bugtasks[2].target, name='2.0')
+            self.bugtasks[1].transitionToMilestone(milestone_1, self.owner)
+            self.bugtasks[2].transitionToMilestone(milestone_2, self.owner)
+        return self.bugtasks[1:] + self.bugtasks[:1]
+
 
 class MilestoneTarget(BugTargetTestBase):
     """Use a milestone as the bug target."""
@@ -992,6 +1032,15 @@
         # main bug target.
         return self._findBugtaskForOtherProduct(bugtask, self.product)
 
+    def setUpMilestoneSorting(self):
+        # Setup for a somewhat pointless test: All bugtasks are already
+        # assigned to same milestone. This means essentially that the
+        # search result should be ordered by the secondary sort order,
+        # BugTask.importance.
+        # Note that reversing the sort direction of milestone does not
+        # affect the sort direction of the bug ID.
+        return sorted(self.bugtasks, key=lambda bugtask: bugtask.importance)
+
 
 class DistributionTarget(BugTargetTestBase, ProductAndDistributionTests,
                          BugTargetWithBugSuperVisor,
@@ -1032,6 +1081,16 @@
                 subscriber, subscribed_by=subscriber)
         return subscriber
 
+    def setUpMilestoneSorting(self):
+        with person_logged_in(self.owner):
+            milestone_1 = self.factory.makeMilestone(
+                distribution=self.searchtarget, name='1.0')
+            milestone_2 = self.factory.makeMilestone(
+                distribution=self.searchtarget, name='2.0')
+            self.bugtasks[1].transitionToMilestone(milestone_1, self.owner)
+            self.bugtasks[2].transitionToMilestone(milestone_2, self.owner)
+        return self.bugtasks[1:] + self.bugtasks[:1]
+
 
 class DistroseriesTarget(BugTargetTestBase):
     """Use a distro series as the bug target."""
@@ -1055,6 +1114,16 @@
     def setBugParamsTarget(self, params, target):
         params.setDistroSeries(target)
 
+    def setUpMilestoneSorting(self):
+        with person_logged_in(self.owner):
+            milestone_1 = self.factory.makeMilestone(
+                distribution=self.searchtarget.distribution, name='1.0')
+            milestone_2 = self.factory.makeMilestone(
+                distribution=self.searchtarget.distribution, name='2.0')
+            self.bugtasks[1].transitionToMilestone(milestone_1, self.owner)
+            self.bugtasks[2].transitionToMilestone(milestone_2, self.owner)
+        return self.bugtasks[1:] + self.bugtasks[:1]
+
 
 class SourcePackageTarget(BugTargetTestBase):
     """Use a source package as the bug target."""
@@ -1091,6 +1160,16 @@
     def targetToGroup(self, target):
         return target.sourcepackagename.id
 
+    def setUpMilestoneSorting(self):
+        with person_logged_in(self.owner):
+            milestone_1 = self.factory.makeMilestone(
+                distribution=self.searchtarget.distribution, name='1.0')
+            milestone_2 = self.factory.makeMilestone(
+                distribution=self.searchtarget.distribution, name='2.0')
+            self.bugtasks[1].transitionToMilestone(milestone_1, self.owner)
+            self.bugtasks[2].transitionToMilestone(milestone_2, self.owner)
+        return self.bugtasks[1:] + self.bugtasks[:1]
+
 
 class DistributionSourcePackageTarget(BugTargetTestBase,
                                       BugTargetWithBugSuperVisor):
@@ -1126,6 +1205,16 @@
     def targetToGroup(self, target):
         return target.sourcepackagename.id
 
+    def setUpMilestoneSorting(self):
+        with person_logged_in(self.owner):
+            milestone_1 = self.factory.makeMilestone(
+                distribution=self.searchtarget.distribution, name='1.0')
+            milestone_2 = self.factory.makeMilestone(
+                distribution=self.searchtarget.distribution, name='2.0')
+            self.bugtasks[1].transitionToMilestone(milestone_1, self.owner)
+            self.bugtasks[2].transitionToMilestone(milestone_2, self.owner)
+        return self.bugtasks[1:] + self.bugtasks[:1]
+
 
 bug_targets_mixins = (
     DistributionTarget,