sort_link class: does not get passed correctly when no label argument passed (SortLink.new)

[Ancez]

Hello, hope you're all well.

My colleague has encountered a minor issue with the sort_link method. We are using the latest version of ransack (2.5.0).

The issue appears to be caused by ransack/helpers/form_helper.rb:53, SortLink.new. After debugging it further, I think the issue is how the URL options are extracted within the SortLink#extract_options_and_mutate_args! method as it extracts the classes as URL options.

Outline

• when no label passed, the classes are passed into the query string and not into the link_to class: attribute like expected

Screenshots showcasing the issue

This issue is not urgent, but I think this will be useful to other developers who might encounter this unexpected behaviour.

Excited to hear your thoughts on this, and thanks for any input.

[deivid-rodriguez]

Can you try out this patch?

diff --git a/lib/ransack/helpers/form_helper.rb b/lib/ransack/helpers/form_helper.rb
index fa8f82a..9927ab7 100644
--- a/lib/ransack/helpers/form_helper.rb
+++ b/lib/ransack/helpers/form_helper.rb
@@ -51,7 +51,7 @@ module Ransack
         end
         args[args.first.is_a?(Array) ? 1 : 0, 0] = capture(&block) if block_given?
         s = SortLink.new(search, attribute, args, params, &block)
-        link_to(s.name, url(routing_proxy, s.url_options), s.html_options(args))
+        link_to(s.name, url(routing_proxy, s.url_options), s.html_options)
       end
 
       # +sort_url+
@@ -130,14 +130,13 @@ module Ransack
 
         def url_options
           @params.merge(
-            @options.merge(
+            @options.except(:class).merge(
               @search.context.search_key => search_and_sort_params))
         end
 
-        def html_options(args)
-          html_options = extract_options_and_mutate_args!(args)
-          html_options.merge(
-            class: [['sort_link'.freeze, @current_dir], html_options[:class]]
+        def html_options
+          @options.merge(
+            class: [['sort_link'.freeze, @current_dir], @options[:class]]
                    .compact.join(' '.freeze)
           )
         end
diff --git a/spec/ransack/helpers/form_helper_spec.rb b/spec/ransack/helpers/form_helper_spec.rb
index 0b1d338..d538ea0 100644
--- a/spec/ransack/helpers/form_helper_spec.rb
+++ b/spec/ransack/helpers/form_helper_spec.rb
@@ -726,6 +726,17 @@ module Ransack
         it { should match /Block label ▼/ }
       end
 
+      describe '#sort_link with class option' do
+        subject { @controller.view_context
+          .sort_link(
+            [:main_app, Person.ransack(sorts: ['name desc'])],
+            :name,
+            class: 'people', controller: 'people'
+          )
+        }
+        it { should match /class="sort_link desc people"/ }
+      end
+
       describe '#search_form_for with default format' do

[Ancez]

extract_options_and_mutate_args

This is brilliant! Thank you so much for providing this patch, it works like a charm! 🥳

After some testing, it occured to me that other users might be bypassing this just as we are on our apps by specifying the second argument as an empty hash. e.g.

<%= sort_link(@q, :invitation_sent_at, 'invited at date', {}, class: 'block px-6 py-4 text-xs font-medium uppercase text-so-blue-500 text-gray-500 cursor-pointer tracking-wider') %>

By specifying an empty hash as the first argument, the above patch will not detect the class: argument and will therefore not use the classes specified so anyone who would be updating their ransack version which includes this patch, would have to additionally update all their sort_link's to exclude the empty hash {} so the above would have to change to:

<%= sort_link(@q, :invitation_sent_at, 'invited at date', class: 'block px-6 py-4 text-xs font-medium uppercase text-so-blue-500 text-gray-500 cursor-pointer tracking-wider') %>

The code responsible for this is located at ransack/helpers/form_helper.rb:109, done via the extract_options_and_mutate_args! method which returns args.shift.with_indifferent_access if args[0].is_a?(Hash). I don't think we should be modifying this method as it would just add extra complexity that's unnecessary. I think making the users of ransack aware of this bugfix would be more appropriate.

Let me know what you think @deivid-rodriguez, I can PR this as well if you'd like me to

[deivid-rodriguez]

mmmm, not sure if I understood correctly. You're pointing out that this fix will break a workaround some users were using before, right?

What's your suggestion, use my patch and let things break, or are you suggesting further changes in order to let the above workaround still work as before?

[Ancez]

I think we should use your patch and let things break. The workaround is what I wasn't sure on, was thinking about it but no. Users can lock their versions.

It's detectable through changelog, tests, and CI

[deivid-rodriguez]

We have a pending 3.0 release, so we could include this there to be on the safe side and warn people that this might break some previously working usages.

[Ancez]

That'll be perfect, including this in a major release will do the trick 🎉 a note in the readme about this should suffice

[deivid-rodriguez]

@Ancez In the end I decided to make the fix backwards compatible, see #1288. I will deprecate the workaround in another PR.

[deivid-rodriguez]

I merged #1288, I will create one more PR on top of it to deprecate to trick to workaround this issue and sneak it into the 3.0.0 release.

[deivid-rodriguez]

#1289 deprecates the workaround 👍.