Merge ~pelpsi/launchpad:alphabetical-sort-orderd-unhelpful-codenames into launchpad:master

[Simone Pelosi]

Current alphabetical sort order for Ubuntu codenames is unhelpful

translation sorted by distroseries.version instead of (distroseries.name, package.name)

LP: #1923378

[Jürgen Gmach (jugmac00)]

Have you noticed that Colin suggested to replace `p.distroseries.name` with `Version(p.distroseries.version)` in the related bug in a comment?

If you plan to do otherwise, what are your reasons?

Also, while it seem daunting to find corresponding tests, there are a couple of strategies I use.

First, as you modify something in `product.py`, it is highly likely that there are unit tests in a `test_product.py` near the same location of the former file.

You are modifying the `translatable_packages` property, so the first I would do is search for that name in the above test file. If that does not show up, I would look where `translatable_packages` is used and then look for the corresponding tests.

Also, sometimes we do have doctests, which are a bit harder to find.

Running the whole test suite to find the correct tests is usually off limits except for the most severe changesets, as it takes so long ( 6 hours on my laptop, though mine seems to be on the slower end ).

tl/dr Although we do not measure test coverage, we seem to have a pretty high coverage. Please have a look whether you could create a test, and in case you need help, please do not hesitate to reach out.

[Colin Watson (cjwatson)]

I don't think it was right to remove the source package name from the sort keys here. Can you explain your thinking there? My inclination would be to put it back, because there's nothing technically to prevent the same upstream product being packaged in more than one source package (it happens in practice sometimes due to renames), and we should make the sort order be deterministic in that case.

It should be straightforward to add some test coverage for this, so I'd definitely like to see that before landing this (if nothing else because it will be some practice with the test suite). An untested sketch of the setup part of a unit test might look something like this (probably in `lp.registry.tests.test_product.TestProduct`):

    productseries = self.factory.makeProductSeries()
    zesty = self.factory.makeUbuntuDistroSeries(version="17.04", name="zesty")
    artful = self.factory.makeUbuntuDistroSeries(version="17.10", name="artful")
    spns = [self.factory.makeSourcePackageName("package-%d" % i) for i in range(2)]
    for distroseries in zesty, artful:
        for spn in spns:
            self.factory.makePackagingLink(productseries=productseries, sourcepackagename=spn, distroseries=distroseries)
            self.factory.makePOTemplate(distroseries=distroseries, sourcepackagename=spn)

(Normally I prefer to use entirely synthetic data in tests, but in this case the real Ubuntu "zesty" and "artful" series make a good illustration of the problem we're trying to avoid.)

You'd then want to assert that `productseries.product.translatable_packages` is in the right order, which in this case would be `(artful, spns[0]), (artful, spns[1]), (zesty, spns[0]), (zesty, spns[1])`.

review: Needs Fixing

[Colin Watson (cjwatson)]

(And I agree with Jürgen that this should wrap `Version()` around the series version, so that it compares using version semantics rather than purely lexicographically.)

18a42c7... by Simone Pelosi on 2023-03-17

Added a test case for translated packages order and fixed the sorting order

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/registry/interfaces/product.py b/lib/lp/registry/interfaces/product.py
index a27236b..e28f68c 100644
--- a/lib/lp/registry/interfaces/product.py
+++ b/lib/lp/registry/interfaces/product.py
@@ -920,7 +920,8 @@ class IProductView(
 
     translatable_packages = Attribute(
         "A list of the source packages for this product that can be "
-        "translated sorted by distroseries.name and sourcepackage.name."
+        "translated sorted by distroseries.version (descending order) "
+        "and package.name (ascending order)."
     )
 
     translatable_series = Attribute(
diff --git a/lib/lp/registry/model/product.py b/lib/lp/registry/model/product.py
index f7c3b99..8faf3cf 100644
--- a/lib/lp/registry/model/product.py
+++ b/lib/lp/registry/model/product.py
@@ -57,6 +57,7 @@ from lp.app.errors import NotFoundError, ServiceUsageForbidden
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.app.interfaces.services import IService
 from lp.app.model.launchpad import InformationTypeMixin
+from lp.archivepublisher.debversion import Version
 from lp.blueprints.enums import SpecificationFilter
 from lp.blueprints.model.specification import (
     SPECIFICATION_POLICY_ALLOWED_TYPES,
@@ -1270,8 +1271,14 @@ class Product(
             if package.has_current_translation_templates
         }
 
-        # Sort packages by distroseries.name and package.name
-        return sorted(packages, key=lambda p: (p.distroseries.name, p.name))
+        # Sort packages by distroseries.version (descending order)
+        # and package.name (ascending order)
+        sorted_by_names = sorted(packages, key=lambda p: p.name)
+        return sorted(
+            sorted_by_names,
+            key=lambda p: Version(p.distroseries.version),
+            reverse=True,
+        )
 
     @property
     def translatable_series(self):
diff --git a/lib/lp/registry/tests/test_product.py b/lib/lp/registry/tests/test_product.py
index 6f28d7b..5401861 100644
--- a/lib/lp/registry/tests/test_product.py
+++ b/lib/lp/registry/tests/test_product.py
@@ -256,6 +256,45 @@ class TestProduct(TestCaseWithFactory):
             [series.name for series in product.getVersionSortedSeries()],
         )
 
+    def test_translatable_packages_order(self):
+        # The product translatable packages should be ordered
+        # by Version(distroseries.version) and package.name in
+        # descending order.
+        productseries = self.factory.makeProductSeries()
+        zesty = self.factory.makeUbuntuDistroSeries(
+            version="17.04", name="zesty"
+        )
+        artful = self.factory.makeUbuntuDistroSeries(
+            version="17.10", name="artful"
+        )
+        spns = [
+            self.factory.makeSourcePackageName("package-%d" % i)
+            for i in range(2)
+        ]
+
+        for distroseries in zesty, artful:
+            for spn in spns:
+                self.factory.makePackagingLink(
+                    productseries=productseries,
+                    sourcepackagename=spn,
+                    distroseries=distroseries,
+                )
+                self.factory.makePOTemplate(
+                    distroseries=distroseries, sourcepackagename=spn
+                )
+        self.assertEqual(
+            [
+                ("artful", "package-0"),
+                ("artful", "package-1"),
+                ("zesty", "package-0"),
+                ("zesty", "package-1"),
+            ],
+            [
+                (p.distroseries.name, p.name)
+                for p in productseries.product.translatable_packages
+            ],
+        )
+
     def test_getVersionSortedSeries_with_specific_statuses(self):
         # The obsolete series should be included in the results if
         # statuses=[SeriesStatus.OBSOLETE]. The development focus will