launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/bugtask-distroseries-sort-by-version into lp:launchpad.

Commit message:
Sort bug tasks related to distribution series by series version rather than series name.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1681899 in Launchpad itself: "Series tasks in bugs should be sorted by series.version not series.name"
  https://bugs.launchpad.net/launchpad/+bug/1681899

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/bugtask-distroseries-sort-by-version/+merge/322467

I steered clear of doing this for ProductSeries-related tasks mostly out of conservatism: I wasn't sure if we can reliably expect versions to be sensibly comparable there.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/bugtask-distroseries-sort-by-version into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2016-07-29 16:13:36 +0000
+++ lib/lp/bugs/browser/bugtask.py	2017-04-12 15:39:07 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """IBugTask-related browser views."""
@@ -95,6 +95,7 @@
 from lp.app.enums import PROPRIETARY_INFORMATION_TYPES
 from lp.app.errors import NotFoundError
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.archivepublisher.debversion import Version
 from lp.bugs.browser.bug import (
     BugContextMenu,
     BugTextView,
@@ -1655,28 +1656,34 @@
     Designed to make sense when bugtargetdisplayname is shown.
     """
     if IDistribution.providedBy(bugtask.target):
-        return (
-            None, bugtask.target.displayname, None, None, None)
+        key = (None, bugtask.target.displayname, None, None, None)
     elif IDistroSeries.providedBy(bugtask.target):
-        return (
+        key = (
             None, bugtask.target.distribution.displayname,
             bugtask.target.name, None, None)
     elif IDistributionSourcePackage.providedBy(bugtask.target):
-        return (
+        key = (
             bugtask.target.sourcepackagename.name,
             bugtask.target.distribution.displayname, None, None, None)
     elif ISourcePackage.providedBy(bugtask.target):
-        return (
+        key = (
             bugtask.target.sourcepackagename.name,
             bugtask.target.distribution.displayname,
-            bugtask.target.distroseries.name, None, None)
+            Version(bugtask.target.distroseries.version), None, None)
     elif IProduct.providedBy(bugtask.target):
-        return (None, None, None, bugtask.target.displayname, None)
+        key = (None, None, None, bugtask.target.displayname, None)
     elif IProductSeries.providedBy(bugtask.target):
-        return (
+        key = (
             None, None, None, bugtask.target.product.displayname,
             bugtask.target.name)
-    raise AssertionError("No sort key for %r" % bugtask.target)
+    else:
+        raise AssertionError("No sort key for %r" % bugtask.target)
+
+    # Map elements of the sort key into a form where None is guaranteed to
+    # compare before non-None.  Values of arbitrary types are not guaranteed
+    # to compare greater than None in Python 2, and in Python 3 even asking
+    # the question results in a TypeError.
+    return [(0, None) if value is None else (1, value) for value in key]
 
 
 class BugTasksNominationsView(LaunchpadView):

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2016-07-29 16:41:58 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2017-04-12 15:39:07 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -797,7 +797,7 @@
     def test_bugtask_sorting(self):
         # Product tasks come first, sorted by product then series.
         # Distro tasks follow, sorted by package, distribution, then
-        # series.
+        # series (by version in the case of distribution series).
         foo = self.factory.makeProduct(displayname='Foo')
         self.factory.makeProductSeries(product=foo, name='2.0')
         self.factory.makeProductSeries(product=foo, name='1.0')
@@ -805,8 +805,12 @@
         self.factory.makeProductSeries(product=bar, name='0.0')
 
         barix = self.factory.makeDistribution(displayname='Barix')
-        self.factory.makeDistroSeries(distribution=barix, name='beta')
-        self.factory.makeDistroSeries(distribution=barix, name='alpha')
+        self.factory.makeDistroSeries(
+            distribution=barix, name='aaa-release', version='1.0')
+        self.factory.makeDistroSeries(
+            distribution=barix, name='beta', version='0.10')
+        self.factory.makeDistroSeries(
+            distribution=barix, name='alpha', version='0.9')
         fooix = self.factory.makeDistribution(displayname='Fooix')
         self.factory.makeDistroSeries(distribution=fooix, name='beta')
 
@@ -818,6 +822,7 @@
             foo, foo.getSeries('1.0'), foo.getSeries('2.0'),
             barix.getSourcePackage(bar_spn),
             barix.getSeries('beta').getSourcePackage(bar_spn),
+            barix.getSeries('aaa-release').getSourcePackage(bar_spn),
             fooix.getSourcePackage(bar_spn),
             fooix.getSeries('beta').getSourcePackage(bar_spn),
             barix.getSourcePackage(foo_spn),