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
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
