Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Refactor #sort_link to extract mutation and order-dependent
code into a single `initialize!` method and reduce what was previously a large, 100-line method with values mutating from start to finish, into a 5-line method that calls other small, order-independent, mutation-free methods. Consider this an experiment. It could be much better. There is a bit more to do and improve, and a few places where I think the code can be optimized to run faster. The two form_helper methods could probably benefit from much better documentation in the code itself, to help developers who would like to improve it and submit PRs.
- Loading branch information
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,21 +2,25 @@ module Ransack | |
| module Helpers | ||
| module FormHelper | ||
|
|
||
| # +search_form_for+ | ||
| # | ||
| # Example: <%= search_form_for(@q) do |f| %> | ||
| # | ||
| def search_form_for(record, options = {}, &proc) | ||
| if record.is_a? Ransack::Search | ||
| search = record | ||
| options[:url] ||= polymorphic_path( | ||
| search.klass, format: options.delete(:format) | ||
| ) | ||
| elsif record.is_a? Array && | ||
This comment has been minimized.
Sorry, something went wrong. |
||
| (search = record.detect { |o| o.is_a? Ransack::Search }) | ||
| options[:url] ||= polymorphic_path( | ||
| record.map { |o| o.is_a? Ransack::Search ? o.klass : o }, | ||
This comment has been minimized.
Sorry, something went wrong. 
daniel-rikowski
|
||
| format: options.delete(:format) | ||
| ) | ||
| else | ||
| raise ArgumentError, | ||
| 'No Ransack::Search object was provided to search_form_for!' | ||
| end | ||
| options[:html] ||= {} | ||
| html_options = { | ||
|
|
@@ -35,145 +39,149 @@ def search_form_for(record, options = {}, &proc) | |
| form_for(record, options, &proc) | ||
| end | ||
|
|
||
| # +sort_link+ | ||
| # | ||
| # Example: <%= sort_link(@q, :name, [:name, 'kind ASC'], 'Player Name') %> | ||
| # | ||
| def sort_link(search, attribute, *args) | ||
| @search_object, routing_proxy = extract_search_obj_and_routing(search) | ||
| raise TypeError, 'First argument must be a Ransack::Search!' unless | ||
| Search === @search_object | ||
| initialize!(search, attribute, args) | ||
| link_to(name, url(routing_proxy), html_options(args)) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| # All mutation and order-dependent code happens here. | ||
| # Ivars are not mutated outside of this method. | ||
| def initialize!(search, attribute, args) | ||
| @field_name = attribute.to_s | ||
| @sort_fields, args = extract_sort_fields(args) | ||
| @label_text, args = extract_label_text(args) | ||
| @options, args = extract_options(args) | ||
| @hide_indicator = @options.delete :hide_indicator | ||
| @default_order = @options.delete :default_order | ||
| if Hash === @default_order | ||
| @default_order = @default_order.with_indifferent_access | ||
| end | ||
| @sort_params = initialize_sort_params | ||
| @sort_params = @sort_params.first if @sort_params.size == 1 | ||
| @current_dir = existing_sort_direction | ||
| end | ||
|
|
||
| def name | ||
| [ERB::Util.h(@label_text), order_indicator] | ||
| .compact.join(Constants::NON_BREAKING_SPACE).html_safe | ||
| end | ||
|
|
||
| def url(routing_proxy) | ||
| if routing_proxy && respond_to?(routing_proxy) | ||
| send(routing_proxy).url_for(options_for_url(@options)) | ||
| else | ||
| url_for(options_for_url(@options)) | ||
| end | ||
| end | ||
|
|
||
| def options_for_url(options) | ||
| options[@search_object.context.search_key] = search_and_sort_params | ||
| params.merge(options) | ||
| end | ||
|
|
||
| def search_and_sort_params | ||
| search_params.merge(:s => @sort_params) | ||
| end | ||
|
|
||
| def search_params | ||
| params[@search_object.context.search_key].presence || | ||
| {}.with_indifferent_access | ||
| end | ||
|
|
||
| def html_options(args) | ||
| html_options = extract_options(args).first | ||
| html_options.merge(class: [css, html_options[:class]] | ||
| .compact.join(Constants::SPACE)) | ||
| end | ||
|
|
||
| def css | ||
| [Constants::SORT_LINK, @current_dir].compact.join(Constants::SPACE) | ||
| end | ||
|
|
||
| def extract_search_obj_and_routing(search) | ||
| if search.is_a? Array | ||
| [search.second, search.first] | ||
| else | ||
| [search, nil] | ||
| end | ||
| end | ||
|
|
||
| def extract_sort_fields(args) | ||
| if Array === args.first | ||
| [args.shift, args] | ||
| else | ||
| [Array(@field_name), args] | ||
| end | ||
| end | ||
|
|
||
| def extract_label_text(args) | ||
| if String === args.first | ||
| [args.shift, args] | ||
| else | ||
| [Translate.attribute(@field_name, :context => @search_object.context), args] | ||
| end | ||
| end | ||
|
|
||
| def extract_options(args) | ||
| if args.first.is_a? Hash | ||
| [args.shift, args] | ||
| else | ||
| [{}, args] | ||
| end | ||
| end | ||
|
|
||
| def initialize_sort_params | ||
| @sort_fields.each_with_object([]) do |field, _| | ||
| attr_name, new_dir = field.to_s.split(/\s+/) | ||
| if no_sort_direction_specified?(new_dir) | ||
| new_dir = detect_previous_sort_direction_and_invert_it(attr_name) | ||
| end | ||
| _ << "#{attr_name} #{new_dir}" | ||
| end | ||
| end | ||
|
|
||
| def detect_previous_sort_direction_and_invert_it(attr_name) | ||
| sort_dir = existing_sort_direction(attr_name) | ||
| if sort_dir | ||
| direction_text(sort_dir) | ||
| else | ||
| default_order(attr_name) || Constants::ASC | ||
| end | ||
| end | ||
|
|
||
| def existing_sort_direction(attr_name = @field_name) | ||
| if _ = @search_object.sorts.detect { |s| s.name == attr_name } | ||
| _.dir | ||
| end | ||
| end | ||
|
|
||
| def default_order(attr_name) | ||
| Hash === @default_order ? @default_order[attr_name] : @default_order | ||
| end | ||
|
|
||
| def order_indicator | ||
| if @hide_indicator || no_sort_direction_specified? | ||
| nil | ||
| else | ||
| direction_arrow | ||
| end | ||
| end | ||
|
|
||
| def no_sort_direction_specified?(dir = @current_dir) | ||
| Constants::ASC_DESC.none? { |d| d == dir } | ||
| end | ||
|
|
||
| def direction_arrow | ||
| if @current_dir == Constants::DESC | ||
| Constants::DESC_ARROW | ||
| else | ||
| Constants::ASC_ARROW | ||
|
|
||
The removal of the brackets changed the evaluation order. Now it is
which results in a type error because it returns a
Ransack::Searchinstance instead of theArrayclass. (i.e.&&is evaluated before theis_a?method call)To fix this the code should either be changed back or the
&&changed to the less strongand.