search

DavyJonesLocker / client_side_validations

Client Side Validations made easy for Ruby on Rails
MIT License
2.69k stars 404 forks source link

Bugfix #827 Fix nested forms `f.fields` #828

Closed AquisTech closed 3 years ago

AquisTech commented 3 years ago

Added fields method. This I am using in my project and is running well. But please review and suggest if you foresee any issues or any other changes are expected.

tagliala commented 3 years ago

Hi, thanks for the report and for the PR

Could you please

  1. Fix the RuboCop offence reported by Travis CI
  2. Add a failing spec
AquisTech commented 3 years ago

Hi, thanks for the report and for the PR

Could you please

  1. Fix the RuboCop offence reported by Travis CI
  2. Add a failing spec

Sure @tagliala

coveralls commented 3 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 46bb70393410a8ee50ab79b656d5d85c48ca9497 on AquisTech:bugfix-827-nested-form-breaks-for-fields into 34959f16be5d159a47af3525e967efe5d1f666c4 on DavyJonesLocker:main.

coveralls commented 3 years ago

Coverage Status

Coverage decreased (-8.5%) to 91.522% when pulling a0073c93fd869a65074f97ffa3796cbcfbe00e3c on AquisTech:bugfix-827-nested-form-breaks-for-fields into 2b8021eb3971502febcabee02de999539c5f5c7d on DavyJonesLocker:main.

tagliala commented 3 years ago

Amazing, thanks

Specs are still failing on CI for different reasons

Please take a look at the Travis Build

If this method is only used in Rails 6.1, all the previous versions of rails should not test it. You may want to provide conditional testing like it has been done with form_with

https://github.com/DavyJonesLocker/client_side_validations/blob/main/test/action_view/cases/test_form_with_helpers.rb#L5

tagliala commented 3 years ago

Please rebase on main, I've already cherry-picked 11597208feeeb0288d93687abe126ee398fbe91f, thanks

AquisTech commented 3 years ago

@tagliala I am trying to fix specs. Also I will add it conditionally. One thing I observed. Generation of ids for form_with and fields depends on Rails.application.config.action_view.form_with_generates_ids for a few versions of Rails id did not get generated by default. But for recent versions it gets generated by default. The code is working for me as I am latest version. For CSV we force to have ids as our JS depends on them. So any suggestions how should we force that behaviour? In my specs, I am able to get id for fields but its not prepending parent name in it.

Failure:
ClientSideValidations::FormWithActionViewHelpersTest#test_form_with_nested_fields_inherit_validation_settings [/home/anand/projects/client_side_validations/test/action_view/cases/test_form_with_helpers.rb:285]:
Expected: <form action="/posts" accept-charset="UTF-8" method="post" data-client-side-validations="{&quot;html_settings&quot;:{&quot;type&quot;:&quot;ActionView::Helpers::FormBuilder&quot;,&quot;input_tag&quot;:&quot;\u003cspan id=\&quot;input_tag\&quot;\u003e\u003c/span\u003e&quot;,&quot;label_tag&quot;:&quot;\u003clabel id=\&quot;label_tag\&quot;\u003e\u003c/label\u003e&quot;},&quot;number_format&quot;:{&quot;separator&quot;:&quot;.&quot;,&quot;delimiter&quot;:&quot;,&quot;},&quot;validators&quot;:{&quot;post[comment][title]&quot;:{&quot;presence&quot;:[{&quot;message&quot;:&quot;can&#39;t be blank&quot;}]}}}" novalidate="novalidate" data-remote="true"><input name="utf8" type="hidden" value="&#x2713;" /><input type="text" name="post[comment][title]" id="post_comment_title" /></form>
Actual: <form action="/posts" accept-charset="UTF-8" data-remote="true" novalidate="novalidate" data-client-side-validations="{&quot;html_settings&quot;:{&quot;type&quot;:&quot;ActionView::Helpers::FormBuilder&quot;,&quot;input_tag&quot;:&quot;\u003cspan id=\&quot;input_tag\&quot;\u003e\u003c/span\u003e&quot;,&quot;label_tag&quot;:&quot;\u003clabel id=\&quot;label_tag\&quot;\u003e\u003c/label\u003e&quot;},&quot;number_format&quot;:{&quot;separator&quot;:&quot;.&quot;,&quot;delimiter&quot;:&quot;,&quot;},&quot;validators&quot;:{}}" method="post"><input name="utf8" type="hidden" value="&#x2713;" /><input type="text" name="comment[title]" id="comment_title" /></form>

Difference is <input type="text" name="post[comment][title]" id="post_comment_title" /> and <input type="text" name="comment[title]" id="comment_title" />

tagliala commented 3 years ago

For CSV we force to have ids as our JS depends on them. So any suggestions how should we force that behaviour?

Is this question related to this PR?

tagliala commented 3 years ago

I can see that there are some errors like 

ClientSideValidations::FormWithActionViewHelpersTest#test_form_with_nested_fields_with_nested_attributes:
ArgumentError: wrong number of arguments (given 2, expected 0..1)

This happens because the specs are calling f.fields(:comments, [@comment]), which is incorrect (in other words: fields tests can't be a copy & paste of fields_for)

Here it is the signature of fields: https://apidock.com/rails/v5.1.7/ActionView/Helpers/FormHelper/fields

Since I do not use fields, please keep working on specs and then fix them

AquisTech commented 3 years ago

@tagliala Yes you are right about spec errors. I was fixing failures on local. So that signature is corrected but now there are failures due to mismatch between expected and actual behaviour in terms of ID being created. I will keep working to fix specs over weekend. Mainly I dont want to override whole method just to get the ID right on some Rails version. Can we add some note in README saying that "Rails.application.config.action_view.form_with_generates_ids this MUST be set to true in your rails app for CSV to work." First I will work on this weekend I will try to fix specs and then we can decide about the note in Readme

tagliala commented 3 years ago

expected and actual behaviour in terms of ID being created.

There is a helper called automatic_id that checks the version of rails and decides whether to add the id or not.

https://github.com/DavyJonesLocker/client_side_validations/blob/2b8021eb3971502febcabee02de999539c5f5c7d/test/action_view/cases/test_legacy_form_with_helpers.rb#L137-L139

But I think that if the ID is not provided, then there will be issues with the front-end validations (CSV uses ids to identify fields)

Can we add some note in README saying that "Rails.application.config.action_view.form_with_generates_ids this MUST be set to true in your rails app for CSV to work."

I guess this is an issue only for 5.0 and 5.1, and we should add to the readme this note, correct?

https://guides.rubyonrails.org/configuring.html

tagliala commented 3 years ago

@AquisTech I'm working on this and I've got the issue, thanks