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

Already on GitHub? Sign in to your account

Group select doesn't pick up default label/value #978

Closed
dnagir opened this Issue Feb 3, 2014 · 7 comments

Comments

Projects
None yet
3 participants

dnagir commented Feb 3, 2014

The following:

f.input :on_behalf_of, collection: agents, as: :grouped_select, group_label_method: :first, group_method: :last, include_blank: false

Produces incorrect results when one group is empty.

The simple_form has the following config entries (meaning id and name should be used as value/name methods in the example below):

config.collection_label_methods = [:to_label, :name, :title, :short_description, :description, :to_s]
config.collection_value_methods = [:id, :value, :to_s]

For example, this works fine:

X = Struct.new(:id, :name)
agents = [["First", [X.new(0, 'zero')]], ["Second", [X.new(1, 'one'), X.new(2, 'two')]]]

f.input :on_behalf_of, collection: agents, as: :grouped_select, group_label_method: :first, group_method: :last, include_blank: false

and produces the following:

<div class="form-group grouped_select optional reservation_update_on_behalf_of"><label class="grouped_select optional control-label col-md-4" for="agent_change_reservation_update_on_behalf_of">Reserve on behalf of...</label><div class="col-md-8">

<select class="grouped_select optional form-control" id="agent_change_reservation_update_on_behalf_of" name="reservation_update[on_behalf_of]">
<optgroup label="First"><option value="0">zero</option></optgroup>
<optgroup label="Second">
  <option value="1">one</option>
  <option value="2">two</option>
</optgroup>
</select>

</div></div>

but the following doesn't seem to use the default collection value/name methods (note the empty group):

X = Struct.new(:id, :name)
agents = [["First", []], ["Second", [X.new(1, 'one'), X.new(2, 'two')]]]

f.input :on_behalf_of, collection: agents, as: :grouped_select, group_label_method: :first, group_method: :last, include_blank: false

producing:

<div class="form-group grouped_select optional reservation_update_on_behalf_of"><label class="grouped_select optional control-label col-md-4" for="agent_change_reservation_update_on_behalf_of">Reserve on behalf of...</label><div class="col-md-8">

<select class="grouped_select optional form-control" id="agent_change_reservation_update_on_behalf_of" name="reservation_update[on_behalf_of]">
<optgroup label="First"></optgroup>
<optgroup label="Second">
  <option value="#&lt;struct ActionView::CompiledTemplates::X id=1, name=&quot;one&quot;&gt;">#&lt;struct ActionView::CompiledTemplates::X id=1, name=&quot;one&quot;&gt;</option>
  <option value="#&lt;struct ActionView::CompiledTemplates::X id=2, name=&quot;two&quot;&gt;">#&lt;struct ActionView::CompiledTemplates::X id=2, name=&quot;two&quot;&gt;</option>
</optgroup>
</select>

</div></div>

Reproducible in simple_form 3.0.0 and 3.0.1.

dnagir commented Feb 3, 2014

The workaround is obviously to apply the label_method: :name, value_method: :id directly.
But this shouldn't be necessary because it fails depending on the data that is given to the input which is the main problem.

Collaborator

nashby commented Feb 5, 2014

@dnagir yeah, that's because we use this as a sample collection to auto detect label and value methods. And since the first one is a blank array we can't detect proper methods and use .to_s. Not sure what's the best way to fix it though.

dnagir commented Feb 6, 2014

How about getting it from first non empty entry instead of just first?
On 5 Feb 2014 19:25, "Vasiliy Ermolovich" notifications@github.com wrote:

@dnagir https://github.com/dnagir yeah, that's because we use thishttps://github.com/plataformatec/simple_form/blob/master/lib/simple_form/inputs/grouped_collection_select_input.rb#L22as a sample collection to auto detect label and value methods. And since
the first one is a blank array we can't detect proper methods and use
.to_s. Not sure what's the best way to fix it though.


Reply to this email directly or view it on GitHubhttps://github.com/plataformatec/simple_form/issues/978#issuecomment-34145935
.

Collaborator

rafaelfranca commented Mar 20, 2014

I think this is too much magic than we like to have. Why not passing the value_method explicitly?

dnagir commented Mar 21, 2014

@rafaelfranca no problem applying it explicitly. The problem is that it is totally unexpected to do so when it is not necessary in all other cases. How come regular lists do pick it up and the grouped don't? This suggest the logic is very different and brings unexpected (by the user) behaviour.

Collaborator

rafaelfranca commented Mar 21, 2014

This is the problem with magic behavior, some cause it doesn't work. For these cases you always can workaround yourself. We don't need to implement every single possibility.

Collaborator

rafaelfranca commented Mar 21, 2014

But feel free to open a PR fixing it

@rafaelfranca rafaelfranca added Bug and removed wontfix labels Mar 21, 2014

@rafaelfranca rafaelfranca self-assigned this Apr 1, 2014

@rafaelfranca rafaelfranca closed this in #1016 Apr 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment