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

Add a hidden field on the collection_radio_buttons #18303

Merged
merged 2 commits into from Sep 26, 2015

Conversation

Projects
None yet
4 participants
@maurogeorge
Contributor

maurogeorge commented Jan 2, 2015

The same behavior that occurs on file_field(#17947) occurs with the collection_radio_buttons.

@rafaelfranca rafaelfranca self-assigned this Jan 2, 2015

Show outdated Hide outdated actionview/lib/action_view/helpers/tags/collection_radio_buttons.rb
@@ -14,7 +14,8 @@ def radio_button(extra_html_options={})
end
def render(&block)
render_collection do |item, value, text, default_html_options|
options = @options.stringify_keys

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

I think we can simplify the implementation of this method and collection_check_boxes moving it render_collection. Also I don't think we need to stringify in this case.

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

I think we can simplify the implementation of this method and collection_check_boxes moving it render_collection. Also I don't think we need to stringify in this case.

Show outdated Hide outdated actionview/lib/action_view/helpers/tags/collection_radio_buttons.rb
@@ -30,6 +39,18 @@ def render(&block)
def render_component(builder)
builder.radio_button + builder.label
end
def hidden_field

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

This should go to a shared place too.

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

This should go to a shared place too.

Show outdated Hide outdated actionview/CHANGELOG.md
@@ -12,5 +12,9 @@
*Angelo Capilleri*
* Add a `hidden_field` on the `collection_radio_buttons` to avoid raise a error

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

It should be at top of the file

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

It should be at top of the file

Show outdated Hide outdated actionview/test/template/form_collections_helper_test.rb
collection = [Category.new(1, 'Category 1'), Category.new(2, 'Category 2')]
with_collection_radio_buttons :user, :category_ids, collection, :id, :name
assert_select "input[type=hidden][name='user[category_ids][]'][value='']", :count => 1

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

Use new hash syntax

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

Use new hash syntax

Show outdated Hide outdated actionview/test/template/form_collections_helper_test.rb
test 'collection radio buttons generates a hidden field using the given :name in :html_options' do
collection = [Category.new(1, 'Category 1'), Category.new(2, 'Category 2')]
with_collection_radio_buttons :user, :category_ids, collection, :id, :name, {}, {name: "user[other_category_ids][]"}

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

space after the { and before } at the last hash.

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

space after the { and before } at the last hash.

Show outdated Hide outdated actionview/test/template/form_collections_helper_test.rb
collection = [Category.new(1, 'Category 1'), Category.new(2, 'Category 2')]
with_collection_radio_buttons :user, :category_ids, collection, :id, :name, {}, {name: "user[other_category_ids][]"}
assert_select "input[type=hidden][name='user[other_category_ids][]'][value='']", :count => 1

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

New hash syntax

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

New hash syntax

test 'collection radio buttons does not generate a hidden field if include_hidden option is false with key as string' do
collection = [Category.new(1, 'Category 1'), Category.new(2, 'Category 2')]
with_collection_radio_buttons :user, :category_ids, collection, :id, :name, 'include_hidden' => false

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

Not sure if this is needed. Does collection_check_box have this test?

@rafaelfranca

rafaelfranca Jan 2, 2015

Member

Not sure if this is needed. Does collection_check_box have this test?

Show outdated Hide outdated actionview/lib/action_view/helpers/form_options_helper.rb
# The HTML specification says when nothing is select on a collection of radio buttons
# web browsers do not send any value to server.
# Unfortunately this introduces a gotcha:
# if an +User+ model has a +category_ids+ field, and in the form none category is selected no +category_ids+ parameter is sent. So,

This comment has been minimized.

@eileencodes

eileencodes Jan 5, 2015

Member

This should be "If a +User+" because the way "user" is pronounced is an exception to the article "an" before a vowel rule because it makes the same sound as "y" in "you".

@eileencodes

eileencodes Jan 5, 2015

Member

This should be "If a +User+" because the way "user" is pronounced is an exception to the article "an" before a vowel rule because it makes the same sound as "y" in "you".

This comment has been minimized.

@maurogeorge

maurogeorge Jan 6, 2015

Contributor

Thanks, I will update.

@maurogeorge

maurogeorge Jan 6, 2015

Contributor

Thanks, I will update.

@maurogeorge

This comment has been minimized.

Show comment
Hide comment
@maurogeorge

maurogeorge Jan 6, 2015

Contributor

@rafaelfranca I updated the code following your suggestions.

About collection_check_box it dont have the test of include_hidden as string, I added the missing test, now both works with the string. I added the stringfy to fix this.

Contributor

maurogeorge commented Jan 6, 2015

@rafaelfranca I updated the code following your suggestions.

About collection_check_box it dont have the test of include_hidden as string, I added the missing test, now both works with the string. I added the stringfy to fix this.

@maurogeorge

This comment has been minimized.

Show comment
Hide comment
@maurogeorge

maurogeorge Jan 20, 2015

Contributor

@rafaelfranca when you have some time could you please review 😄

Contributor

maurogeorge commented Jan 20, 2015

@rafaelfranca when you have some time could you please review 😄

@maurogeorge

This comment has been minimized.

Show comment
Hide comment
@maurogeorge

maurogeorge Mar 28, 2015

Contributor

@rafaelfranca fixed the conflicts, when you have some time could you please review

Contributor

maurogeorge commented Mar 28, 2015

@rafaelfranca fixed the conflicts, when you have some time could you please review

@maurogeorge

This comment has been minimized.

Show comment
Hide comment
@maurogeorge

maurogeorge May 7, 2015

Contributor

@rafaelfranca please review when you have some time 😄

Contributor

maurogeorge commented May 7, 2015

@rafaelfranca please review when you have some time 😄

@maurogeorge

This comment has been minimized.

Show comment
Hide comment
@maurogeorge

maurogeorge Jun 13, 2015

Contributor

@rafaelfranca I rebased again to fix the conflicts, please review when you have some time.Is there anything I can do to help? @carlosantoniodasilva maybe you can review this 😄

Contributor

maurogeorge commented Jun 13, 2015

@rafaelfranca I rebased again to fix the conflicts, please review when you have some time.Is there anything I can do to help? @carlosantoniodasilva maybe you can review this 😄

end
end
def hidden_field #:nodoc:

This comment has been minimized.

@claudiob

claudiob Sep 3, 2015

Member

Hello @maurogeorge – I'm 👍 with this PR 😀

Since you added def hidden_field, could the one already defined in collection_check_boxes.rb be removed?

@claudiob

claudiob Sep 3, 2015

Member

Hello @maurogeorge – I'm 👍 with this PR 😀

Since you added def hidden_field, could the one already defined in collection_check_boxes.rb be removed?

This comment has been minimized.

@maurogeorge

maurogeorge Sep 3, 2015

Contributor

Hi @claudiob great to hear this 😄

You are right I can remove the mentioned hidden_field. I updated the code and rebased.

@maurogeorge

maurogeorge Sep 3, 2015

Contributor

Hi @claudiob great to hear this 😄

You are right I can remove the mentioned hidden_field. I updated the code and rebased.

@template_object.check_box(@object_name, @method_name, html_options, @value, nil)
end
end
def render(&block)
rendered_collection = render_collection do |item, value, text, default_html_options|
default_html_options[:multiple] = true

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 3, 2015

Member

Multiple is not being handled anymore with your refactoring.

@rafaelfranca

rafaelfranca Sep 3, 2015

Member

Multiple is not being handled anymore with your refactoring.

This comment has been minimized.

@maurogeorge

maurogeorge Sep 4, 2015

Contributor

@rafaelfranca I moved the multiple options to the check_box it is covered by the tests, if I remove this line I got some broken tests

@maurogeorge

maurogeorge Sep 4, 2015

Contributor

@rafaelfranca I moved the multiple options to the check_box it is covered by the tests, if I remove this line I got some broken tests

Show outdated Hide outdated actionview/lib/action_view/helpers/tags/collection_helpers.rb
def hidden_field #:nodoc:
hidden_name = @html_options[:name]
hidden_name ||= if @options.has_key?(:index)

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 3, 2015

Member

Why this is not using tag_name?

@rafaelfranca

rafaelfranca Sep 3, 2015

Member

Why this is not using tag_name?

This comment has been minimized.

@maurogeorge

maurogeorge Sep 4, 2015

Contributor

Thanks for the tip, I updated the code to use tag_name.

@maurogeorge

maurogeorge Sep 4, 2015

Contributor

Thanks for the tip, I updated the code to use tag_name.

This comment has been minimized.

@rafaelfranca

rafaelfranca Sep 7, 2015

Member

Does not the previous implementation work? Does it need to check :index option?

@rafaelfranca

rafaelfranca Sep 7, 2015

Member

Does not the previous implementation work? Does it need to check :index option?

Show outdated Hide outdated actionview/CHANGELOG.md
Example:
* Add a `hidden_field` on the `collection_radio_buttons` to avoid raise a error

This comment has been minimized.

@claudiob

claudiob Sep 4, 2015

Member

There is something strange in this change… it should only show your new lines, not replace the existing one.
Did you accidentally edit the next CHANGELOG message too?

If you review this, also fix avoid raise to avoid raising 😉

@claudiob

claudiob Sep 4, 2015

Member

There is something strange in this change… it should only show your new lines, not replace the existing one.
Did you accidentally edit the next CHANGELOG message too?

If you review this, also fix avoid raise to avoid raising 😉

# every collection of radio buttons. The hidden field has the same name as collection radio button and blank value.
#
# In case if you don't want the helper to generate this hidden field you can specify
# <tt>include_hidden: false</tt> option.

This comment has been minimized.

@claudiob

claudiob Sep 4, 2015

Member

Hello @maurogeorge – I think this comment is incorrect.

In the case of a collection of radio button, you don't have category_ids (plural) and mass assignments.

Instead, the effect of this commit is to tackle the case where you have a form that contains only a collection of radio buttons (and no other field type).

For instance, if you have a User#update form, where all you can do is to select the category through radio buttons.

If a user submits such a form without selecting any radio button, then you expect the params to contain {user: {category_id: nil}. This would let you save the object or optionally raise a validation error for the absence of category. This is the behavior that your commit adds.

The current behavior is that the params will not contain {user: ...} at all.
Therefore if your controller requires strong parameters like params.require(:user).permit(...), the require part will fail and the user will see a 400 error.

@claudiob

claudiob Sep 4, 2015

Member

Hello @maurogeorge – I think this comment is incorrect.

In the case of a collection of radio button, you don't have category_ids (plural) and mass assignments.

Instead, the effect of this commit is to tackle the case where you have a form that contains only a collection of radio buttons (and no other field type).

For instance, if you have a User#update form, where all you can do is to select the category through radio buttons.

If a user submits such a form without selecting any radio button, then you expect the params to contain {user: {category_id: nil}. This would let you save the object or optionally raise a validation error for the absence of category. This is the behavior that your commit adds.

The current behavior is that the params will not contain {user: ...} at all.
Therefore if your controller requires strong parameters like params.require(:user).permit(...), the require part will fail and the user will see a 400 error.

maurogeorge added some commits Jan 2, 2015

Add a hidden field on the collection_radio_buttons
This will avoid a error be raised when the only input on the form is the
`collection_radio_buttons`.
@maurogeorge

This comment has been minimized.

Show comment
Hide comment
@maurogeorge

maurogeorge Sep 24, 2015

Contributor

@claudiob @rafaelfranca thanks a lot for the review ❤️ 😍
Sorry for the delay, I was involved with other things 😢
I updated the code, please take a look when you have some time 😄

Contributor

maurogeorge commented Sep 24, 2015

@claudiob @rafaelfranca thanks a lot for the review ❤️ 😍
Sorry for the delay, I was involved with other things 😢
I updated the code, please take a look when you have some time 😄

rafaelfranca added a commit that referenced this pull request Sep 26, 2015

Merge pull request #18303 from maurogeorge/collection_radio_buttons_h…
…idden_field

Add a hidden field on the collection_radio_buttons

@rafaelfranca rafaelfranca merged commit ae1f295 into rails:master Sep 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@meinac meinac referenced this pull request Sep 27, 2015

Merged

minor doc fix [ci skip] #21785

@maurogeorge

This comment has been minimized.

Show comment
Hide comment
@maurogeorge

maurogeorge Sep 28, 2015

Contributor

Thanks for the merge ❤️

Contributor

maurogeorge commented Sep 28, 2015

Thanks for the merge ❤️

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