When sorting addons from a collection by name, one of extension is dissappearing

[vcarciu]

Steps to reproduce: 1.Open this collection https://addons.mozilla.org/en-US/firefox/collections/15194964/test/edit/?page=1&collection_sort=-added 2.Sort addons by name

Expected results: Collection are ordered by name and it shows 4 webextensions

Actual results: After ordering by name, one of webextensions is dissappearing

Please see a video with the issue :

[kumar303]

The Network panel confirms that this is an API issue. The response contains the wrong number of items.

[diox]

@vcarciu can you figure out STR on dev? It doesn't happen with all collections, it depends on the add-ons inside it. That missing add-on only has a translation in it for its name (which matches its default locale, also it), I suspect it's part of the problem.

[vcarciu]

@diox : I managed to reproduce it on -dev with https://addons-dev.allizom.org/ro/firefox/collections/10641615/testcollect/?page=1&collection_sort=-added using same steps and this time, 2 addons are missing, probably related to what you said in the comment from above : https://addons-dev.allizom.org/ro/firefox/addon/corrector-ortografico-aragones/?src=collection and https://addons-dev.allizom.org/ro/firefox/addon/corrector-de-galego/?src=collection

[diox]

I think this happens when the add-ons have no translation in the current language and no translation in en-US either, regardless of their default locale (if they don't have a translation in their default locale that's a different problem entirely, something we actually do try to prevent - and off-topic here).

With that in mind, https://addons-dev.allizom.org/ca/firefox/collections/10641615/testcollect/?page=1&collection_sort=name shows 4 add-ons because one of them has translations in ca and es - so it's absent when you do the request in most languages, like ro above, but does show up with ca.

I'm still unclear why this is happening, but at least that's progress: I did manage to reproduce locally with that information.

[diox]

I figured it out. Like the regular query made to fetch translations, the query made to sort by translation is trying to find the translation in the current language, and falls back to the field returned by the model get_fallback(), or just settings.LANGUAGE_CODE (which is en-US).

The problem is, here, we're not ordering an Addon queryset, but a CollectionAddon queryset. The CollectionAddon queryset doesn't have get_fallback() method (or a default_locale field), so we fall back to settings.LANGUAGE_CODE instead. That means add-ons that don't have a translation in the current language or en-US are excluded.

>>> order_by_translation(CollectionAddon.objects.filter(collection=collection).exclude(addon__status=amo.STATUS_DELETED), 'name', Addon).count()                                                                              
3

It wouldn't be a problem with an Addon queryset:

>>> order_by_translation(collection.addons.all().exclude(status=amo.STATUS_DELETED), 'name', Addon).count()                                                                              
5

order_by_translation() already gets passed an optional model argument so that it knows the field to sort on belongs to a different model (), we need to somehow make it use the get_fallback() method from that* model instead of the one from the queryset.

The patch is kinda convoluted because of how this is all implemented relying on django internal ORM compiler stuff, but does appear to work, I need to write tests now:

diff --git a/src/olympia/translations/query.py b/src/olympia/translations/query.py
index a38160551c..03904fa591 100644
--- a/src/olympia/translations/query.py
+++ b/src/olympia/translations/query.py
@@ -41,6 +41,7 @@ def order_by_translation(qs, fieldname, model=None):
     # INNER JOINs)
     qs.query = qs.query.clone()
     qs.query.__class__ = TranslationQuery
+    qs.query.fallback_model = model
     t1 = qs.query.join(
         Join(field.remote_field.model._meta.db_table, model._meta.db_table,
              None, LOUTER, field, True),
@@ -99,8 +100,9 @@ class SQLCompiler(compiler.SQLCompiler):

         # fallback could be a string locale or a model field.
         params.append(translation_utils.get_language())
-        if hasattr(self.query.model, 'get_fallback'):
-            fallback = self.query.model.get_fallback()
+        model = getattr(self.query, 'fallback_model', self.query.model)
+        if hasattr(model, 'get_fallback'):
+            fallback = model.get_fallback()
         else:
             fallback = settings.LANGUAGE_CODE
         if not isinstance(fallback, models.Field):
@@ -110,15 +112,15 @@ class SQLCompiler(compiler.SQLCompiler):
         # Django had in query.tables, but that seems to be ok.
         for field, aliases in self.query.translation_aliases.items():
             t1, t2 = aliases
-            joins.append(self.join_with_locale(t1, None, old_map))
-            joins.append(self.join_with_locale(t2, fallback, old_map))
+            joins.append(self.join_with_locale(t1, None, old_map, model))
+            joins.append(self.join_with_locale(t2, fallback, old_map, model))

         if old_tables:
             self.query.tables = old_tables
         self.query.alias_map = old_map
         return joins, params

-    def join_with_locale(self, alias, fallback, alias_map):
+    def join_with_locale(self, alias, fallback, alias_map, model):
         # This is all lifted from the real sql.compiler.get_from_clause(),
         # except for the extra AND clause.  Fun project: fix Django to use Q
         # objects here instead of a bunch of strings.
@@ -132,7 +134,7 @@ class SQLCompiler(compiler.SQLCompiler):
             else ' %s' % join.table_alias)

         if isinstance(fallback, models.Field):
-            fallback_str = '%s.%s' % (qn(self.query.model._meta.db_table),
+            fallback_str = '%s.%s' % (qn(model._meta.db_table),
                                       qn(fallback.column))
         else:
             fallback_str = '%s'

(*) side note: it only works because we're also filtering on something in the Addon model. Otherwise the join to the addons table wouldn't be made!

[diox]

Amusingly, because we don't use order_by_translation that much, and you need to make a query against a relation to trigger it, the bug only shows up in collections. If we somehow wanted to display your add-ons sorted by name alongside your role in each of them, we'd see it too, but we don't do that. Essentially the bug has been present in the translations code since the beginning of time but was limited in impact.

That explains why this issue has been reported before with collections in legacy frontend (it used the same queryset) but never occurred in other places where we order by translations (though I'm sure we could find other bugs with ordering by translations if we looked for them).

[vcarciu]

Verified as fixed in AMO-dev :