Grouped collection select documentation error? #623

Closed
tfwright opened this Issue Jul 18, 2012 · 11 comments

Comments

Projects
None yet
5 participants
@tfwright

According to the documentation, it should be possible to pass in a lambda/proc to the value_method option to collection selects, but when I do so using a grouped collection on Rails 3.2.6 I get the following error: ActionView::Template::Error: ... (lambda)> is not a symbol

It seems to be Rails throwing the error so it seems like the options are being sent straight to the helper without being called. Briefly looking through the source, it indeed looks like if the options are present they are not inspected to see if they need to be called

https://github.com/plataformatec/simple_form/blob/master/lib/simple_form/inputs/grouped_collection_select_input.rb#L5
https://github.com/plataformatec/simple_form/blob/master/lib/simple_form/inputs/collection_input.rb#L53

Am I missing something here, or is the documentation out of date?

Thanks.

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 18, 2012

Collaborator

The collection helpers (select, check boxes, radio buttons) do accept a lambda/proc, but unfortunately the grouped one does not. And the select one only work because we monkey patch Rails to add support for that.

If you want to try out and make it work for grouped select as well, you'd more than welcome. Please feel free to ask any questions, we can help as needed. I'll leave this opened as "feature request".

Thanks!

Collaborator

carlosantoniodasilva commented Jul 18, 2012

The collection helpers (select, check boxes, radio buttons) do accept a lambda/proc, but unfortunately the grouped one does not. And the select one only work because we monkey patch Rails to add support for that.

If you want to try out and make it work for grouped select as well, you'd more than welcome. Please feel free to ask any questions, we can help as needed. I'll leave this opened as "feature request".

Thanks!

@gauthier-delacroix

This comment has been minimized.

Show comment Hide comment
@gauthier-delacroix

gauthier-delacroix Jul 19, 2012

Same issue here (for label_method).

I'll try to write the monkey patch if it's close to yours but I presume you'd already have done it if it was so simple...

Same issue here (for label_method).

I'll try to write the monkey patch if it's close to yours but I presume you'd already have done it if it was so simple...

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Jul 19, 2012

Collaborator

@porecreat we will work on it later, but feel free to open a pull request if you want.

Collaborator

rafaelfranca commented Jul 19, 2012

@porecreat we will work on it later, but feel free to open a pull request if you want.

@tfwright

This comment has been minimized.

Show comment Hide comment
@tfwright

tfwright Jul 19, 2012

For me the work around was to just use the Rails helper with grouped_options_for_select. This is a little more flexible than grouped_options_from_collection_for_select--I hadn't looked into it yet but I was wondering whether an easy/cleaner solution would be to just switch to the former in the background if a lambda/proc is provided.

For me the work around was to just use the Rails helper with grouped_options_for_select. This is a little more flexible than grouped_options_from_collection_for_select--I hadn't looked into it yet but I was wondering whether an easy/cleaner solution would be to just switch to the former in the background if a lambda/proc is provided.

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jul 19, 2012

Collaborator

@tfwright the same is true for select vs collection_select, but we do stick with the collection one. Let us know if you find anything to improve on this side.

@porecreat it's more likely that we forgot about the grouped helper, since it was added later (and this is the first issue raised about this subjet).

As @rafaelfranca said, anyone is free to help and send us a pull request, otherwise we'll work on it when we have some time. Thanks!

Collaborator

carlosantoniodasilva commented Jul 19, 2012

@tfwright the same is true for select vs collection_select, but we do stick with the collection one. Let us know if you find anything to improve on this side.

@porecreat it's more likely that we forgot about the grouped helper, since it was added later (and this is the first issue raised about this subjet).

As @rafaelfranca said, anyone is free to help and send us a pull request, otherwise we'll work on it when we have some time. Thanks!

@gauthier-delacroix

This comment has been minimized.

Show comment Hide comment
@gauthier-delacroix

gauthier-delacroix Jul 19, 2012

I've found that overriding "options_from_collection_for_select" does the job for grouped and basic select (both call this method to render options).

This quick hack works (it's actually simpler than yours ;o) ):

module ActionView::Helpers
  module FormOptionsHelper

    def value_for_collection(item, value) #:nodoc:
        value.respond_to?(:call) ? value.call(item) : item.send(value)
    end

    # Override default Rails options_from_collection_for_select helper to handle lambdas/procs in
    # text and value methods, so it works the same way as collection_radio_buttons
    # and collection_check_boxes in SimpleForm. If none of text/value methods is a
    # callable object, then it just delegates back to original collection select.
    #
    alias :original_options_from_collection_for_select :options_from_collection_for_select
    def options_from_collection_for_select(collection, value_method, text_method, selected = nil)
      if value_method.respond_to?(:call) || text_method.respond_to?(:call)
        collection = collection.map do |item|
          value = value_for_collection(item, value_method)
          text  = value_for_collection(item, text_method)

          [value, text]
        end

        value_method, text_method = :first, :last
      end

      original_options_from_collection_for_select(collection, value_method, text_method, selected)
    end
  end
end

I've copied "value_for_collection" since I had an error when overriding "class FormBuilder" like you did and value_for_collection wasn't found in "module FormOptionsHelper", even by including "SimpleForm::ActionViewExtensions::Builder"

I've found that overriding "options_from_collection_for_select" does the job for grouped and basic select (both call this method to render options).

This quick hack works (it's actually simpler than yours ;o) ):

module ActionView::Helpers
  module FormOptionsHelper

    def value_for_collection(item, value) #:nodoc:
        value.respond_to?(:call) ? value.call(item) : item.send(value)
    end

    # Override default Rails options_from_collection_for_select helper to handle lambdas/procs in
    # text and value methods, so it works the same way as collection_radio_buttons
    # and collection_check_boxes in SimpleForm. If none of text/value methods is a
    # callable object, then it just delegates back to original collection select.
    #
    alias :original_options_from_collection_for_select :options_from_collection_for_select
    def options_from_collection_for_select(collection, value_method, text_method, selected = nil)
      if value_method.respond_to?(:call) || text_method.respond_to?(:call)
        collection = collection.map do |item|
          value = value_for_collection(item, value_method)
          text  = value_for_collection(item, text_method)

          [value, text]
        end

        value_method, text_method = :first, :last
      end

      original_options_from_collection_for_select(collection, value_method, text_method, selected)
    end
  end
end

I've copied "value_for_collection" since I had an error when overriding "class FormBuilder" like you did and value_for_collection wasn't found in "module FormOptionsHelper", even by including "SimpleForm::ActionViewExtensions::Builder"

@gauthier-delacroix

This comment has been minimized.

Show comment Hide comment
@gauthier-delacroix

gauthier-delacroix Jul 19, 2012

Sorry for not sending this through a pull request: I'm not very comfortable with Github yet. I'll try to do it but it needs to be a bit more clean (I'm currently trying this in an initializer).

Sorry for not sending this through a pull request: I'm not very comfortable with Github yet. I'll try to do it but it needs to be a bit more clean (I'm currently trying this in an initializer).

@svendahlstrand

This comment has been minimized.

Show comment Hide comment
@svendahlstrand

svendahlstrand Nov 12, 2012

Contributor

@carlosantoniodasilva You've seem to have fixed this problem in Rails 4: rails/rails@9035324#actionpack/lib/action_view/helpers/form_options_helper.rb. Will you still accept a patch to get it working in rails 3.x?

Contributor

svendahlstrand commented Nov 12, 2012

@carlosantoniodasilva You've seem to have fixed this problem in Rails 4: rails/rails@9035324#actionpack/lib/action_view/helpers/form_options_helper.rb. Will you still accept a patch to get it working in rails 3.x?

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Nov 12, 2012

Collaborator

@svendahlstrand I'm not sure that fixes the problem for grouped collection select, does that? I remember it was about collection select only, but I may be wrong. Also, I didn't have time to get back to this, unfortunatelly 😄

In any case, I'm unsure we can/should backport such a change to 3-2 since it's more a feature addition than a bug fix at this point, and lately we're only applying bug fixes to 3-2 branch.

Collaborator

carlosantoniodasilva commented Nov 12, 2012

@svendahlstrand I'm not sure that fixes the problem for grouped collection select, does that? I remember it was about collection select only, but I may be wrong. Also, I didn't have time to get back to this, unfortunatelly 😄

In any case, I'm unsure we can/should backport such a change to 3-2 since it's more a feature addition than a bug fix at this point, and lately we're only applying bug fixes to 3-2 branch.

@svendahlstrand

This comment has been minimized.

Show comment Hide comment
@svendahlstrand

svendahlstrand Nov 12, 2012

Contributor

@carlosantoniodasilva I think it will! :) option_groups_from_collection_for_select is calling options_from_collection_for_select: https://github.com/rails/rails/blob/master/actionpack/lib/action_view/helpers/form_options_helper.rb#L442.

What I meant about a patch: will you accept a pull request for simple_form to get it working in Rails < 4? :)

Contributor

svendahlstrand commented Nov 12, 2012

@carlosantoniodasilva I think it will! :) option_groups_from_collection_for_select is calling options_from_collection_for_select: https://github.com/rails/rails/blob/master/actionpack/lib/action_view/helpers/form_options_helper.rb#L442.

What I meant about a patch: will you accept a pull request for simple_form to get it working in Rails < 4? :)

@carlosantoniodasilva

This comment has been minimized.

Show comment Hide comment
@carlosantoniodasilva

carlosantoniodasilva Nov 12, 2012

Collaborator

@svendahlstrand ah, pull request to SimpleForm 👍, will review when I find some time here, thanks!

Collaborator

carlosantoniodasilva commented Nov 12, 2012

@svendahlstrand ah, pull request to SimpleForm 👍, will review when I find some time here, thanks!

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