launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-sort-tiebreaker into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #987110 in Launchpad itself: "Reversing the sort of a bug search doesn't reverse the tiebreaker"
  https://bugs.launchpad.net/launchpad/+bug/987110

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-sort-tiebreaker/+merge/103600

Bug searches default to sort by (importance DESC, bugtask ASC). The bugtask bit is a tiebreaker that's automatically added to ambiguous sort orders. When you reverse that listing, you get (importance ASC, bugtask ASC) rather than (importance ASC, bugtask DESC) -- the tiebreaker doesn't get inverted. This is potentially confusing (the full listing isn't reversed, just the order of the buckets), and it doubles the number of required indices.

This branch alters bugtasksearch._process_order_by to keep the tiebreaker in the opposite direction to the primary sort. This preserves the existing default order of (importance DESC, bugtask ASC), but flips its (little-used) reverse to (importance ASC, bugtask DESC), letting the same index efficiently satisfy both directions. A similar argument applies to the other indexed orders.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-sort-tiebreaker/+merge/103600
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-sort-tiebreaker into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-04-19 23:43:47 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-04-26 05:13:20 +0000
@@ -1006,14 +1006,18 @@
     if ambiguous:
         if use_flat:
             if in_unique_context:
-                orderby_arg.append(BugTaskFlat.bug_id)
+                disambiguator = BugTaskFlat.bug_id
             else:
-                orderby_arg.append(BugTaskFlat.bugtask_id)
+                disambiguator = BugTaskFlat.bugtask_id
         else:
             if in_unique_context:
-                orderby_arg.append(BugTask.bugID)
+                disambiguator = BugTask.bugID
             else:
-                orderby_arg.append(BugTask.id)
+                disambiguator = BugTask.id
+
+        if orderby_arg and not isinstance(orderby_arg[0], Desc):
+            disambiguator = Desc(disambiguator)
+        orderby_arg.append(disambiguator)
 
     return tuple(orderby_arg), extra_joins
 

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2012-04-19 04:48:44 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2012-04-26 05:13:20 +0000
@@ -23,6 +23,7 @@
     IBugTaskSet,
     )
 from lp.bugs.model.bugsummary import BugSummary
+from lp.bugs.model.bugtasksearch import _process_order_by
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distributionsourcepackage import (
     IDistributionSourcePackage,
@@ -31,6 +32,7 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.services.database.sqlbase import convert_storm_clause_to_string
 from lp.services.features.testing import FeatureFixture
 from lp.services.searchbuilder import (
     all,
@@ -39,6 +41,7 @@
     )
 from lp.testing import (
     person_logged_in,
+    TestCase,
     TestCaseWithFactory,
     )
 from lp.testing.layers import (
@@ -47,6 +50,54 @@
     )
 
 
+class TestProcessOrderBy(TestCase):
+
+    def assertOrderForParams(self, expected, user=None, product=None,
+                             distribution=None, **kwargs):
+        params = BugTaskSearchParams(user, **kwargs)
+        if product:
+            params.setProduct(product)
+        if distribution:
+            params.setProduct(distribution)
+        self.assertEqual(
+            expected,
+            convert_storm_clause_to_string(_process_order_by(params, True)[0]))
+
+    def test_tiebreaker(self):
+        # Requests for ambiguous sorts get a disambiguator of BugTask.id
+        # glued on.
+        self.assertOrderForParams(
+            'BugTaskFlat.importance DESC, BugTaskFlat.bugtask',
+            orderby='-importance')
+
+    def test_tiebreaker_direction(self):
+        # The tiebreaker direction is the reverse of the primary
+        # direction. This is mostly to retain the old default sort order
+        # of (-importance, bugtask), and could probably be reversed if
+        # someone wants to.
+        self.assertOrderForParams(
+            'BugTaskFlat.importance, BugTaskFlat.bugtask DESC',
+            orderby='importance')
+
+    def test_tiebreaker_in_unique_context(self):
+        # The tiebreaker is Bug.id if the context is unique, so we'll
+        # find no more than a single task for each bug. This applies to
+        # searches within a product, distribution source package, or
+        # source package.
+        self.assertOrderForParams(
+            'BugTaskFlat.importance DESC, BugTaskFlat.bug',
+            orderby='-importance',
+            product='foo')
+
+    def test_tiebreaker_in_duplicated_context(self):
+        # If the context can have multiple tasks for a single bug, we
+        # still use BugTask.id.
+        self.assertOrderForParams(
+            'BugTaskFlat.importance DESC, BugTaskFlat.bug',
+            orderby='-importance',
+            distribution='foo')
+
+
 class SearchTestBase:
 
     layer = LaunchpadFunctionalLayer