Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support grouped collection for collection_select #41562

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 32 additions & 6 deletions actionview/lib/action_view/helpers/form_options_helper.rb
Expand Up @@ -170,6 +170,10 @@ def select(object, method, choices = nil, options = {}, html_options = {}, &bloc
# as a +proc+, that will be called for each member of the +collection+ to
# retrieve the value/text.
#
# The members of +collection+ may also be grouped by a key (as by +Enumerable#group_by+), and an
# <tt><optgroup></tt> tag will be rendered for each group, using each group's key as the +label+
# attribute.
#
# Example object structure for use with this method:
#
# class Post < ActiveRecord::Base
Expand All @@ -179,21 +183,43 @@ def select(object, method, choices = nil, options = {}, html_options = {}, &bloc
# class Author < ActiveRecord::Base
# has_many :posts
#
# def name_with_initial
# "#{first_name.first}. #{last_name}"
# def full_name
# "#{first_name} #{last_name}"
# end
#
# def first_initial
# first_name.first
# end
# end
#
# Sample usage (selecting the associated Author for an instance of Post, <tt>@post</tt>):
#
# collection_select(:post, :author_id, Author.all, :id, :name_with_initial, prompt: true)
# collection_select(:post, :author_id, Author.all, :id, :full_name, prompt: true)
#
# If <tt>@post.author_id</tt> is already <tt>1</tt>, this would return:
#
# <select name="post[author_id]" id="post_author_id">
# <option value="">Please select</option>
# <option value="1" selected="selected">D. Heinemeier Hansson</option>
# <option value="2">D. Thomas</option>
# <option value="3">M. Clark</option>
# <option value="1" selected="selected">David Heinemeier Hansson</option>
# <option value="2">Dave Thomas</option>
# <option value="3">Mike Clark</option>
# </select>
#
# Choices may also be grouped by a key:
#
# collection_select(:post, :author_id, Author.order(:first_name).group_by(&:first_initial), :id, :full_name, prompt: true)
#
# In the same context, this would return:
#
# <select name="post[author_id]" id="post_author_id">
# <option value="">Please select</option>
# <optgroup label="D">
# <option value="2">Dave Thomas</option>
# <option value="1" selected="selected">David Heinemeier Hansson</option>
# </optgroup>
# <optgroup label="M">
# <option value="3">Mike Clark</option>
# </optgroup>
# </select>
def collection_select(object, method, collection, value_method, text_method, options = {}, html_options = {})
Tags::CollectionSelect.new(object, method, self, collection, value_method, text_method, options, html_options).render
Expand Down
26 changes: 17 additions & 9 deletions actionview/lib/action_view/helpers/tags/collection_select.rb
Expand Up @@ -14,16 +14,24 @@ def initialize(object_name, method_name, template_object, collection, value_meth
end

def render
option_tags_options = {
selected: @options.fetch(:selected) { value },
disabled: @options[:disabled]
}
Comment on lines -17 to -20
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines are currently (almost) identical to their counterparts in the Select tag helper class:

option_tags_options = {
selected: @options.fetch(:selected) { value.nil? ? "" : value },
disabled: @options[:disabled]
}

I don't think we should refactor them, and hide the equivalence, without a good reason.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I noticed the same pattern in GroupedCollectionSelect, too.

In #44335, I factored this out into a SelectRenderer module. (So Select would look like this, and CollectionSelect would match.) I was planning to merge that PR first, and then rebase this PR on top of it. Any objections?


select_content_tag(
options_from_collection_for_select(@collection, @value_method, @text_method, option_tags_options),
@options, @html_options
)
select_content_tag(option_tags, @options, @html_options)
end

private
def collection_grouped?
entry = @collection.first if @collection.is_a?(Hash) || @collection.is_a?(Array)
entry.respond_to?(:last) && entry.last.is_a?(Array)
end
Comment on lines +21 to +24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost equivalent to the implementation for select:

def grouped_choices?
!@choices.blank? && @choices.first.respond_to?(:last) && Array === @choices.first.last
end

What do you think of matching it exactly? My concern is consistent edge-case behaviour, e.g. passing a set of arrays of arrays will work for select, but won't work here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! The reason I limited this behavior to Hash and Array was because I did not realize I could use blank? to ensure that an AR relation was loaded. I had assumed blank? would execute a limited query, just like empty?. It looks like this behavior was explicitly implemented in #5461 (and tests were added in #5469), so I will change this method to mirror Select. 👍


def option_tags
html_options = { selected: @options.fetch(:selected) { value }, disabled: @options[:disabled] }

if collection_grouped?
option_groups_from_collection_for_select(@collection, :last, :first, @value_method, @text_method, html_options)
else
options_from_collection_for_select(@collection, @value_method, @text_method, html_options)
end
end
end
end
end
Expand Down
20 changes: 20 additions & 0 deletions actionview/test/template/form_options_helper_test.rb
Expand Up @@ -1019,6 +1019,26 @@ def test_collection_select
)
end

def test_collection_select_with_grouped_collection_as_nested_array
@post = Post.new
@post.origin = "dk"

assert_dom_equal(
%Q{<select id="post_origin" name="post[origin]"><optgroup label="&lt;Africa&gt;"><option value="&lt;sa&gt;">&lt;South Africa&gt;</option>\n<option value="so">Somalia</option></optgroup><optgroup label="Europe"><option value="dk" selected="selected">Denmark</option>\n<option value="ie">Ireland</option></optgroup></select>},
collection_select("post", "origin", dummy_continents.pluck(:continent_name, :countries), :country_id, :country_name)
)
end

def test_collection_select_with_grouped_collection_as_hash
@post = Post.new
@post.origin = "dk"

assert_dom_equal(
%Q{<select id="post_origin" name="post[origin]"><optgroup label="&lt;Africa&gt;"><option value="&lt;sa&gt;">&lt;South Africa&gt;</option>\n<option value="so">Somalia</option></optgroup><optgroup label="Europe"><option value="dk" selected="selected">Denmark</option>\n<option value="ie">Ireland</option></optgroup></select>},
collection_select("post", "origin", dummy_continents.pluck(:continent_name, :countries).to_h, :country_id, :country_name)
)
end

def test_collection_select_under_fields_for
@post = Post.new
@post.author_name = "Babe"
Expand Down