AR - Order clause has issues with multiple parameters

[tagliala]

Hi,

I'm opening this issue to track an issue I've found with order clause on multiple columns

Context

ActiveRecord 7.0
Mobility 1.2.6

Issue 1 - 1st parameter is not translatable

Expected Behavior

Orders by translated attribute

Foo.i18n.order(:priority, :name).to_sql
 => "SELECT \"foos\".* FROM \"foos\" ORDER BY \"foos\".\"priority\" ASC, \"foos\".\"name_en\" ASC" 

Actual Behavior

Does not order by translated attribute

Foo.i18n.order(:priority, :name).to_sql
 => "SELECT \"foos\".* FROM \"foos\" ORDER BY \"foos\".\"priority\" ASC, \"foos\".\"name\" ASC" 

Issue 2 - 1st parameter is translatable

Expected Behavior

Keeps in account nth parameter where n >= 2

Foo.i18n.order(:name, :priority).to_sql
 => "SELECT \"foos\".* FROM \"foos\" ORDER BY \"foos\".\"name_en\" ASC, \"foos\".\"priority\" ASC" 

Actual Behavior

Discards nth parameter where n >= 2

Foo.i18n.order(:name, :priority).to_sql
 => "SELECT \"foos\".* FROM \"foos\" ORDER BY \"foos\".\"name_en\" ASC" 

Workaround

Concatenate order clauses

Foo.i18n.order(:priority).order(:name).to_sql
 => "SELECT \"foos\".* FROM \"foos\" ORDER BY \"foos\".\"priority\" ASC, \"foos\".\"name_en\" ASC" 

Possible Fix

mobility/lib/mobility/plugins/active_record/query.rb

Lines 142 to 158 in 7683433

	case opts
	when Symbol, String
	@klass.mobility_attribute?(opts) ? order({ opts => :asc }, *rest) : super
	when ::Hash
	i18n_keys, keys = opts.keys.partition(&@klass.method(:mobility_attribute?))
	return super if i18n_keys.empty?
	base = keys.empty? ? self : super(opts.slice(keys))
	i18n_keys.inject(base) do |query, key|
	backend_class = @klass.mobility_backend_class(key)
	dir, node = opts[key], backend_node(key)
	backend_class.apply_scope(query, node).order(node.send(dir.downcase))
	end
	else
	super
	end

Add support to array of fields change the implementation to correctly manage args

[shioyama]

Seems like a genuine issue, I'll try to have a look.

[shioyama]

This should be a straightforward fix, if anyone wants to have a go at it I'll review any PRs. Otherwise will probably take me a few weeks, I'm very busy until after RailsConf.

[tagliala]

Hi, thanks fro the feedback.

I've tried myself to fix but it isn't so easy for me.

The problem is that multiple symbols are not passed as array, and the previous scope is not maintained in the Hash branch, together with attributes being rearranged by putting untranslated ones at the beginning:

mobility/lib/mobility/plugins/active_record/query.rb

Line 146 in 7683433

i18n_keys, keys = opts.keys.partition(&@klass.method(:mobility_attribute?))

I'm trying to use the same approach as pluck group select for order too, without success for the moment

I've added some failing specs at tagliala@91d827a

[tagliala]

Hi, from time to time I'm still attempting to fix this issue

I've added more failing specs, and I think that Mobility needs something like preprocess_order_args

[shioyama]

Thanks @tagliala, yes this seems to be a bit more complicated than when I originally added support for order... 😭

You're right that preprocessor_order_args is the right reference point. I tried quickly to pull together something now but it's a bit tricky.