Fix ordering by collection name in AddonCollectionViewSet. (#8472)

* order_by_translation can now be applied a specific `model` to join on,
e.g in case of a m2n-table where the method couldn't determine the
correct joins correctly.

Fixes #8354

docs/topics/api/collections.rst

@@ -179,6 +179,7 @@ This endpoint lists the add-ons in a collection, together with collector's notes
 
 All sort parameters can be reversed, e.g. '-added' for descending dates.
 The default sorting is by popularity, descending ('-popularity').
+There can only be one sort parameter, multiple orderings are not supported.
 
 
 .. _collection-addon-filtering-param:

src/olympia/bandwagon/tests/test_views.py

@@ -1869,6 +1869,17 @@ class TestCollectionAddonViewSetList(CollectionAddonViewSetMixin, TestCase):
         self.addon_deleted = addon_factory(name=u'buffalo_deleted')
         self.addon_pending = addon_factory(name=u'pelican_pending')
 
+        # Set a few more languages on our add-ons to test sorting
+        # a bit better. https://github.com/mozilla/addons-server/issues/8354
+        self.addon_a.name = {'de': u'Ameisenbär'}
+        self.addon_a.save()
+
+        self.addon_b.name = {'de': u'Pavian'}
+        self.addon_b.save()
+
+        self.addon_c.name = {'de': u'Gepard'}
+        self.addon_c.save()
+
         self.collection.add_addon(self.addon_a)
         self.collection.add_addon(self.addon_disabled)
         self.collection.add_addon(self.addon_b)
@@ -1919,14 +1930,17 @@ class TestCollectionAddonViewSetList(CollectionAddonViewSetMixin, TestCase):
         self.addon_c.update(weekly_downloads=100)
 
         self.client.login_api(self.user)
+
         # First default sort
         self.check_result_order(
             self.client.get(self.url),
             self.addon_b, self.addon_a, self.addon_c)
+
         # Popularity ascending
         self.check_result_order(
             self.client.get(self.url + '?sort=popularity'),
             self.addon_c, self.addon_a, self.addon_b)
+
         # Popularity descending (same as default)
         self.check_result_order(
             self.client.get(self.url + '?sort=-popularity'),
@@ -1946,6 +1960,7 @@ class TestCollectionAddonViewSetList(CollectionAddonViewSetMixin, TestCase):
         self.check_result_order(
             self.client.get(self.url + '?sort=added'),
             self.addon_b, self.addon_c, self.addon_a)
+
         # Added descending
         self.check_result_order(
             self.client.get(self.url + '?sort=-added'),
@@ -1955,11 +1970,30 @@ class TestCollectionAddonViewSetList(CollectionAddonViewSetMixin, TestCase):
         self.check_result_order(
             self.client.get(self.url + '?sort=name'),
             self.addon_a, self.addon_b, self.addon_c)
+
         # Name descending
         self.check_result_order(
             self.client.get(self.url + '?sort=-name'),
             self.addon_c, self.addon_b, self.addon_a)
 
+        # Name ascending, German
+        self.check_result_order(
+            self.client.get(self.url + '?sort=name&lang=de'),
+            self.addon_a, self.addon_c, self.addon_b)
+
+        # Name descending, German
+        self.check_result_order(
+            self.client.get(self.url + '?sort=-name&lang=de'),
+            self.addon_b, self.addon_c, self.addon_a)
+
+    def test_only_one_sort_parameter_supported(self):
+        response = self.client.get(self.url + '?sort=popularity,name')
+
+        assert response.status_code == 400
+        assert response.data == [
+            'You can only specify one "sort" argument. Multiple orderings '
+            'are not supported']
+
     def test_with_deleted_or_with_hidden(self):
         response = self.send(self.url)
         assert response.status_code == 200

src/olympia/bandwagon/views.py

@@ -14,6 +14,7 @@ from django.views.decorators.csrf import csrf_protect
 from django.views.decorators.http import require_POST
 
 from django_statsd.clients import statsd
+from rest_framework import serializers
 from rest_framework.viewsets import ModelViewSet
 
 import olympia.core.logger
@@ -703,14 +704,34 @@ class CollectionViewSet(ModelViewSet):
         return qs.order_by(sort)[:limit]
 
 
+class TranslationAwareOrderingAliasFilter(OrderingAliasFilter):
+    def filter_queryset(self, request, queryset, view):
+        ordering = self.get_ordering(request, queryset, view)
+
+        if len(ordering) > 1:
+            # We can't support multiple orderings easily because of
+            # how order_by_translation works.
+            raise serializers.ValidationError(
+                'You can only specify one "sort" argument. Multiple '
+                'orderings are not supported')
+
+        order_by = ordering[0]
+
+        if order_by in ('name', '-name'):
+            return order_by_translation(queryset, order_by, Addon)
+
+        sup = super(TranslationAwareOrderingAliasFilter, self)
+        return sup.filter_queryset(request, queryset, view)
+
+
 class CollectionAddonViewSet(ModelViewSet):
     permission_classes = []  # We don't need extra permissions.
     serializer_class = CollectionAddonSerializer
     lookup_field = 'addon'
-    filter_backends = (OrderingAliasFilter,)
+    filter_backends = (TranslationAwareOrderingAliasFilter,)
     ordering_fields = ()
     ordering_field_aliases = {'popularity': 'addon__weekly_downloads',
-                              'name': 'addon__name__localized_string',
+                              'name': 'name',
                               'added': 'created'}
     ordering = ('-addon__weekly_downloads',)
 
@@ -740,6 +761,7 @@ class CollectionAddonViewSet(ModelViewSet):
                                     self.action != 'list')
         # If deleted addons are requested, that implies all addons.
         include_all = filter_param == 'all' or include_all_with_deleted
+
         if not include_all:
             qs = qs.filter(
                 addon__status=amo.STATUS_PUBLIC, addon__disabled_by_user=False)

src/olympia/translations/query.py

@@ -8,7 +8,7 @@ from django.db.models.sql.datastructures import Join
 from django.utils import translation as translation_utils
 
 
-def order_by_translation(qs, fieldname):
+def order_by_translation(qs, fieldname, model=None):
     """
     Order the QuerySet by the translated field, honoring the current and
     fallback locales.  Returns a new QuerySet.
@@ -23,7 +23,7 @@ def order_by_translation(qs, fieldname):
         desc = False
 
     qs = qs.all()
-    model = qs.model
+    model = model or qs.model
     field = model._meta.get_field(fieldname)
 
     # Doing the manual joins is flying under Django's radar, so we need to make