Nested `simple_fields_for` no longer inherits `validate: true` (since v4.2.5) #680

janraasch · 2016-11-08T08:58:07Z

Hi there,

we have a simple_fields_for call inside a simple_form_for-form with validate: true passed to the form. Before v4.2.5 the required nested simple_fields_for attributes would be added to the @validators automatically.

Example

Say we have a User model and an Address model. Now, the Address requires a street, and the User of course has an address.

A sign-up form for a User thus would look like the following

= simple_form_for(@user, url: create_user_path, method: :post, validate: true) do |user|
      %h3.strong= t('contact_details')
      = user.input :first_name
      = user.input :last_name

      = user.simple_fields_for :address do |address|
        = address.input :street

      = user.input :phone
      = user.input :email

Expected Behavior

Before v4.2.5 this would cause the street-Input to be validated on the client-side. As of v4.2.5 the street-Input is not validated on the client-side.

Temporary Fix

This can be fixed by adding validate: true to the user.simple_fields_for :address call.

Versions

Works as expected on v4.2.4. Breaks on all later versions, including v4.2.7. We are running client_side_validations-simple_form v3.2.4, and upgrading to v3.3.0 also does not help; which is why I suspect the problem to originate from client_side_validationsitself.

Supposedly this is a regression, or it is a breaking change which should have been part of a major update.

Let me know, if you need more information.

The text was updated successfully, but these errors were encountered:

tagliala · 2016-11-08T17:11:47Z

Doesn't work for me with 4.2.0+ neither. I'm investigating

Gemfile:

gem 'client_side_validations'
gem 'rails', '4.2.7.1'

User Model

class User < ActiveRecord::Base
  has_one :address
end

Address model

class Address < ActiveRecord::Base
  belongs_to :user

  validates :street, presence: true
end

erb

<%= simple_form_for @user, validate: true do |f| %>
  <%= f.simple_fields_for :address do |address| %>
    <%= address.input :street %>
  <% end %>
<% end %>

output html

<form data-validate="true" novalidate="novalidate" class="new_user" id="new_user" action="/users" accept-charset="UTF-8" method="post"><input name="utf8" type="hidden" value="&#x2713;" /><input type="hidden" name="authenticity_token" value="fqlMT7YMfgk5RWPMi38krHHmB8lFE7JlhX+w1Ja2eu0iyP6CUPZrvVzBTkVQIl/q6X3BmYDTMn4u5P30aQwqLQ==" />

    <input type="text" name="user[address][street]" id="user_address_street" />
</form><script>
//<![CDATA[
if(window.ClientSideValidations===undefined)window.ClientSideValidations={};window.ClientSideValidations.disabled_validators=["uniqueness"];window.ClientSideValidations.number_format={"separator":".","delimiter":","};if(window.ClientSideValidations.patterns===undefined)window.ClientSideValidations.patterns = {};window.ClientSideValidations.patterns.numericality=/^(-|\+)?(?:\d+|\d{1,3}(?:\,\d{3})+)(?:\.\d*)?$/;if(window.ClientSideValidations.forms===undefined)window.ClientSideValidations.forms={};window.ClientSideValidations.forms['new_user'] = {"type":"ActionView::Helpers::FormBuilder","input_tag":"\u003cdiv class=\"field_with_errors\"\u003e\u003cspan id=\"input_tag\" /\u003e\u003c/div\u003e","label_tag":"\u003cdiv class=\"field_with_errors\"\u003e\u003clabel id=\"label_tag\" /\u003e\u003c/div\u003e","validators":{}};
//]]>
</script>

The problem is that tests for fields_for pass 😐

janraasch · 2016-11-08T17:26:16Z

Thank you for looking into this, @tagliala.

tagliala · 2016-11-08T18:40:35Z

The problem is that address is nil at the moment that fields_for is executing, and CSV is not able to create validation fields for that object. Please add @user.build_address in your new controller and check that the object is instantiated in the edit/update action.

Hope it helps

janraasch · 2017-05-17T11:42:19Z

The problem is still present in the latest version (v9.3.1). I can of course try the @user.build_address workaround as per your suggestion. However I am wondering what caused this change in behavior. Do you have any idea? And if so, could we just make it work like before?

tagliala · 2017-05-17T11:47:46Z

#680 (comment)

Doesn't work for me with 4.2.0+ neither.

Problem is that I cannot replicate on older versions

If you think there is a regression in a specific version (eg: 4.2.4 works, 4.2.5 doesn't), please write a failing test case and use git bisect to find the offending commit

janraasch · 2017-05-22T15:19:35Z

Hi @tagliala, I finally took the time to git-bisect this puppy. Luckily the offending commit is quite small: 77799b3.

I also found out that it is enough to reverse

def fields_for_with_client_side_validations(record_name, record_object = nil, fields_options = {}, &block)
  fields_options[:validate] ||= @options[:validate] if @options[:validate] && !fields_options.key?(:validate)
  fields_for_without_client_side_validations(record_name, record_object, fields_options, &block)
end

to

def fields_for_with_client_side_validations(record_or_name_or_array, *args, &block)
  options = args.extract_options!
  options[:validate] ||= @options[:validate] if @options[:validate] && !options.key?(:validate)
  fields_for_without_client_side_validations(record_or_name_or_array, *(args << options), &block)
end

get rid of the problem.

This got me all excited and I tried to debug what is happening here some more, but this is deep into the FormBuilder stuff and I am having trouble understanding what is going on.

I did however also see that there is a fallback in rails (v4.2) for the old arguments notation:

# File actionview/lib/action_view/helpers/form_helper.rb, line 1573
def fields_for(record_name, record_object = nil, fields_options = {}, &block)
  fields_options, record_object = record_object, nil if record_object.is_a?(Hash) && record_object.extractable_options?

so #665 is not necessary for fields_for and interestingly enough using

def fields_for_with_client_side_validations(record_name, record_object = nil, fields_options = {}, &block)
  fields_options[:validate] ||= @options[:validate] if @options[:validate] && !fields_options.key?(:validate)
  fields_for_without_client_side_validations(record_name, *([] << fields_options), &block)
end

also fixes the problem for our use case.

Let me know, if any of this makes sense to you and I would be very interested to here where I need to dig deeper to finally get to the bottom of this.

Thank you very much for your time.

tagliala · 2017-05-22T19:01:22Z

@janraasch thanks for your effort.

The problem is that I still cannot replicate this issue. I can't see a regression from 4.2.4 to 4.2.5.

I would like to see a failing test case, otherwise I cannot help

Models

class User < ActiveRecord::Base
  has_one :address
end

class Address < ActiveRecord::Base
  belongs_to :user

  validates :street, presence: true
end

Controller

class UsersController < ApplicationController
  def new
    @user = User.new
  end
end

View

<%- require 'client_side_validations/version' %>

CSV Version: <%= ClientSideValidations::VERSION %>
<%= simple_form_for @user, validate: true do |f| %>
  <%= f.simple_fields_for :address do |address| %>
    <%= address.input :street %>
  <% end %>
<% end %>

Output (CSV 4.2.4)

CSV Version: 4.2.4
<form data-validate="true" novalidate="novalidate" class="simple_form new_user" id="new_user" action="/users" accept-charset="UTF-8" method="post"><input name="utf8" type="hidden" value="&#x2713;" /><input type="hidden" name="authenticity_token" value="hGKezNyzaS0/lEBf3xFdKVfDXoVkQcbRabHWvcljWmXYAywBOkl8mVoQbdYETCZvz1iY1aGBRsrCKpudNtkKpQ==" />
  
    <div class="input string required user_address_street"><label class="string required" for="user_address_street"><abbr title="required">*</abbr> Street</label><input class="string required" type="text" name="user[address][street]" id="user_address_street" /></div>
</form><script>//<![CDATA[
if(window.ClientSideValidations===undefined)window.ClientSideValidations={};window.ClientSideValidations.disabled_validators=[];window.ClientSideValidations.number_format={"separator":".","delimiter":","};if(window.ClientSideValidations.patterns===undefined)window.ClientSideValidations.patterns = {};window.ClientSideValidations.patterns.numericality=/^(-|\+)?(?:\d+|\d{1,3}(?:\,\d{3})+)(?:\.\d*)?$/;if(window.ClientSideValidations.forms===undefined)window.ClientSideValidations.forms={};window.ClientSideValidations.forms['new_user'] = {"type":"SimpleForm::FormBuilder","input_tag":"\u003cspan id=\"input_tag\" /\u003e","label_tag":"\u003clabel id=\"label_tag\" /\u003e","validators":{}};
//]]></script>

Output (CSV 4.2.5)

CSV Version: 4.2.5
<form data-validate="true" novalidate="novalidate" class="simple_form new_user" id="new_user" action="/users" accept-charset="UTF-8" method="post"><input name="utf8" type="hidden" value="&#x2713;" /><input type="hidden" name="authenticity_token" value="t45bUGJmMaHlod+HTCu+oD2XFIm6elI7SxSs58T0Bx3r7+mdhJwkFYAl8g6XdsXmpQzS2X+60iDgj+HHO05X3Q==" />
  
    <div class="input string required user_address_street"><label class="string required" for="user_address_street"><abbr title="required">*</abbr> Street</label><input class="string required" type="text" name="user[address][street]" id="user_address_street" /></div>
</form><script>
//<![CDATA[
if(window.ClientSideValidations===undefined)window.ClientSideValidations={};window.ClientSideValidations.disabled_validators=[];window.ClientSideValidations.number_format={"separator":".","delimiter":","};if(window.ClientSideValidations.patterns===undefined)window.ClientSideValidations.patterns = {};window.ClientSideValidations.patterns.numericality=/^(-|\+)?(?:\d+|\d{1,3}(?:\,\d{3})+)(?:\.\d*)?$/;if(window.ClientSideValidations.forms===undefined)window.ClientSideValidations.forms={};window.ClientSideValidations.forms['new_user'] = {"type":"SimpleForm::FormBuilder","input_tag":"\u003cspan id=\"input_tag\" /\u003e","label_tag":"\u003clabel id=\"label_tag\" /\u003e","validators":{}};
//]]>
</script>

tagliala · 2017-05-22T19:16:33Z

btw I could definitely see that something may go wrong when

fields_options, record_object = record_object, nil if record_object.is_a?(Hash) && record_object.extractable_options?

will overwrite the actual fields_options parameter, but I cannot replicate this behaviour

janraasch · 2017-05-26T13:00:07Z

So now I have at least produced a working example you can have a look at.

The code is basically as you suggested:

Models

class User < ActiveRecord::Base
  belongs_to :company

  validates :first_name, presence: true
  validates :last_name, presence: true

  accepts_nested_attributes_for :company
end

class Company < ActiveRecord::Base
  validates :name, presence: true
end

Notice the accepts_nested_attributes_for-call here.

Controller

class UsersController < ApplicationController
  def new
    @user = User.new
    @user.build_company
  end
end

View

<%- require 'client_side_validations/version' %>

CSV Version: <%= ClientSideValidations::VERSION %>
<%= simple_form_for @user, method: :post, validate: true do |f| %>
  <%= f.input :first_name %>
  <%= f.input :last_name %>

  <%= f.simple_fields_for :company do |company_form| %>
    <%= company_form.input :name %>
  <% end %>

  <%= f.button :submit %>
<% end %>

Output (CSV 4.2.4)

<body>
CSV Version: 4.2.4
<form data-validate="true" novalidate="novalidate" class="simple_form new_user" id="new_user" action="/users" accept-charset="UTF-8" method="post"><input name="utf8" type="hidden" value="✓"><input type="hidden" name="authenticity_token" value="FZLVBXFyXJ3KoJFGnTDJ3Gi/Zrka5szbL3h29daWFTKkI6XRXucz+Es1OEb3I8PNoWSgYPv3ZDPDzTdgoEBYEQ==">
  <div class="input string required user_first_name"><label class="string required" for="user_first_name"><abbr title="required">*</abbr> First name</label><input class="string required" required="required" aria-required="true" type="text" name="user[first_name]" id="user_first_name"></div>
  <div class="input string required user_last_name"><label class="string required" for="user_last_name"><abbr title="required">*</abbr> Last name</label><input class="string required" required="required" aria-required="true" type="text" name="user[last_name]" id="user_last_name"></div>

  
    <div class="input string required user_company_name"><label class="string required" for="user_company_attributes_name"><abbr title="required">*</abbr> Name</label><input class="string required" required="required" aria-required="true" type="text" name="user[company_attributes][name]" id="user_company_attributes_name"></div>

  <input type="submit" name="commit" value="Create User" class="button">
</form><script>//<![CDATA[
if(window.ClientSideValidations===undefined)window.ClientSideValidations={};window.ClientSideValidations.disabled_validators=["uniqueness"];window.ClientSideValidations.number_format={"separator":".","delimiter":","};if(window.ClientSideValidations.patterns===undefined)window.ClientSideValidations.patterns = {};window.ClientSideValidations.patterns.numericality=/^(-|\+)?(?:\d+|\d{1,3}(?:\,\d{3})+)(?:\.\d*)?$/;if(window.ClientSideValidations.forms===undefined)window.ClientSideValidations.forms={};window.ClientSideValidations.forms['new_user'] = {"type":"SimpleForm::FormBuilder","error_class":"error","error_tag":"span","wrapper_error_class":"field_with_errors","wrapper_tag":"div","wrapper_class":"input","wrapper":"default","validators":{"user[company_attributes][name]":{"presence":[{"message":"can't be blank"}]},"user[first_name]":{"presence":[{"message":"can't be blank"}]},"user[last_name]":{"presence":[{"message":"can't be blank"}]}}};
//]]></script>
</body>

To reproduce this locally you may check out https://github.com/janraasch/client_side_validations_debugging/commit/520636625fe82704fa2814c10c796c66ffe038c0.

Output (CSV 4.2.5)

<body>
CSV Version: 4.2.5
<form data-validate="true" novalidate="novalidate" class="simple_form new_user" id="new_user" action="/users" accept-charset="UTF-8" method="post"><input name="utf8" type="hidden" value="✓"><input type="hidden" name="authenticity_token" value="lCrwNh7IvHn1b7+FJQ1sxCigmyFkET102JpJyZkol28lm4DiMV3THHT6FoVPHmbV4Xtd+IUAlZw0Lwhc7/7aTA==">
  <div class="input string required user_first_name"><label class="string required" for="user_first_name"><abbr title="required">*</abbr> First name</label><input class="string required" required="required" aria-required="true" type="text" name="user[first_name]" id="user_first_name"></div>
  <div class="input string required user_last_name"><label class="string required" for="user_last_name"><abbr title="required">*</abbr> Last name</label><input class="string required" required="required" aria-required="true" type="text" name="user[last_name]" id="user_last_name"></div>

  
    <div class="input string required user_company_name"><label class="string required" for="user_company_attributes_name"><abbr title="required">*</abbr> Name</label><input class="string required" required="required" aria-required="true" type="text" name="user[company_attributes][name]" id="user_company_attributes_name"></div>

  <input type="submit" name="commit" value="Create User" class="button">
</form><script>
//<![CDATA[
if(window.ClientSideValidations===undefined)window.ClientSideValidations={};window.ClientSideValidations.disabled_validators=["uniqueness"];window.ClientSideValidations.number_format={"separator":".","delimiter":","};if(window.ClientSideValidations.patterns===undefined)window.ClientSideValidations.patterns = {};window.ClientSideValidations.patterns.numericality=/^(-|\+)?(?:\d+|\d{1,3}(?:\,\d{3})+)(?:\.\d*)?$/;if(window.ClientSideValidations.forms===undefined)window.ClientSideValidations.forms={};window.ClientSideValidations.forms['new_user'] = {"type":"SimpleForm::FormBuilder","error_class":"error","error_tag":"span","wrapper_error_class":"field_with_errors","wrapper_tag":"div","wrapper_class":"input","wrapper":"default","validators":{"user[first_name]":{"presence":[{"message":"can't be blank"}]},"user[last_name]":{"presence":[{"message":"can't be blank"}]}}};
//]]>
</script>
</body>

To reproduce this locally you may check out https://github.com/janraasch/client_side_validations_debugging/commit/e9e54941a81d39058a03d887f8e1df23d34d67b8.

Comparing the script tags

As you can see, there is no entry of {"user[company_attributes][name]":{"presence":[{"message":"can't be blank"}]} in window.ClientSideValidations.forms['new_user']['validators'] on version v4.2.5, while on v4.2.4 the validation is present. As I mentioned before I already git-bisected the issue at found 77799b3 to be the offending commit.

To reproduce this on your machine, please use the code I provided on https://github.com/janraasch/client_side_validations_debugging. I simply generated a rails app and added the models, controllers and views as described above. You simply need to checkout the repository. Run bundle, rake db:migrate and rails s, then navigate to http://localhost:3000/users/new to see the rendered view.

janraasch · 2017-05-26T15:09:50Z

FYI. I added a form_for example (w/o SimpleForm) to the mix with https://github.com/janraasch/client_side_validations_debugging/commit/dd928df2c2b991cf719fcf083f83789329a74c1a. Interestingly enough the form_for version works correctly. So this problem does originate from the 77799b3 commit of this repository/gem, but can only be exhibited when using simple_form_for.

tagliala · 2017-05-26T15:42:06Z

So this problem does originate from the 77799b3 commit of this repository/gem, but can only be exhibited when using simple_form_for.

I was going to just remove the simple_form dependency to check this issue.

So we are in the wrong repo and I should add a failing test to CSV-simple_form

tagliala · 2017-05-26T16:08:42Z

Yo!

janraasch · 2017-05-26T16:18:57Z

Yo? :)

So we are in the wrong repo and I should add a failing test to CSV-simple_form

That is for you to decide. So far I have been unable to reproduce this in a unit test.

The issue is definitely because of the

fields_options, record_object = record_object, nil if record_object.is_a?(Hash) && record_object.extractable_options?

business I mentioned earlier. simple_form sports that same line in their implementation of simple_fields_for.

This FormBuilder-stuff is crazy interdependent if you ask me.

tagliala · 2017-05-26T16:22:33Z

please add

          fields_options, record_object = record_object, nil if record_object.is_a?(Hash) && record_object.extractable_options?

before

          fields_options[:validate] ||= @options[:validate] if @options[:validate] && !fields_options.key?(:validate)

and it should do the job.

I'm trying to write a failing spec on CSV-simple_form and fix this in CSV

Starting from v4.2.5, fields_for has been suffering from a regression when the record_object passed to the form was a hash. This is particularly true with simple_form and has_one nested models. Fix: #680

tagliala · 2017-05-26T17:44:01Z

@janraasch I was been finally able to reproduce the issue inside a spec and fix that

Thanks for your effort

I'm going to release 9.3.2 tomorrow, but I don't know if I will backport this to the 4.2 branch

janraasch · 2017-05-26T20:37:06Z

Having a working version of v9.x is sufficient for my use case. Thank you very much.

It should avoid reintroducing an issue with simple_fields_for in the future. Ref: DavyJonesLocker/client_side_validations#680

tagliala · 2017-05-26T23:55:34Z

@janraasch thanks for being so patient, it really took a lot of time to understand what was going on.

I've just released CSV 9.3.2 and CSV-simple_form 6.3.0.

The issue has been fixed in CSV, so you should be fine by just updating the CSV gem.

I took the opportunity to release CSV-simple_form 6.3.0 which contains another spec to test simple_fields_for hoping that it would avoid regressions in the future and bumps the minimum required version of CSV to '~> 9.3', '>= 9.3.2' and simple_form to '~> 3.5'

tagliala added bug ruby labels Nov 8, 2016

tagliala closed this as completed Nov 8, 2016

tagliala added question and removed bug labels Nov 8, 2016

tagliala modified the milestone: 4.2.8 Nov 10, 2016

tagliala mentioned this issue May 23, 2017

Nested fields validation #705

Closed

tagliala reopened this May 26, 2017

tagliala added bug and removed question labels May 26, 2017

tagliala closed this as completed in 95cf293 May 26, 2017

tagliala added this to the 9.3.2 milestone May 26, 2017

tagliala added a commit to DavyJonesLocker/client_side_validations-simple_form that referenced this issue May 26, 2017

Add spec for simple_fields_for

6ea2db6

It should avoid reintroducing an issue with simple_fields_for in the future. Ref: DavyJonesLocker/client_side_validations#680

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested `simple_fields_for` no longer inherits `validate: true` (since v4.2.5) #680

Nested `simple_fields_for` no longer inherits `validate: true` (since v4.2.5) #680

janraasch commented Nov 8, 2016 •

edited

Loading

tagliala commented Nov 8, 2016

janraasch commented Nov 8, 2016

tagliala commented Nov 8, 2016

janraasch commented May 17, 2017

tagliala commented May 17, 2017 •

edited

Loading

janraasch commented May 22, 2017

tagliala commented May 22, 2017 •

edited

Loading

tagliala commented May 22, 2017 •

edited

Loading

janraasch commented May 26, 2017 •

edited

Loading

janraasch commented May 26, 2017

tagliala commented May 26, 2017

tagliala commented May 26, 2017

janraasch commented May 26, 2017

tagliala commented May 26, 2017 •

edited

Loading

tagliala commented May 26, 2017

janraasch commented May 26, 2017

tagliala commented May 26, 2017

Nested simple_fields_for no longer inherits validate: true (since v4.2.5) #680

Nested simple_fields_for no longer inherits validate: true (since v4.2.5) #680

Comments

janraasch commented Nov 8, 2016 • edited Loading

Example

Expected Behavior

Temporary Fix

Versions

tagliala commented Nov 8, 2016

janraasch commented Nov 8, 2016

tagliala commented Nov 8, 2016

janraasch commented May 17, 2017

tagliala commented May 17, 2017 • edited Loading

janraasch commented May 22, 2017

tagliala commented May 22, 2017 • edited Loading

Models

Controller

View

Output (CSV 4.2.4)

Output (CSV 4.2.5)

tagliala commented May 22, 2017 • edited Loading

janraasch commented May 26, 2017 • edited Loading

Models

Controller

View

Output (CSV 4.2.4)

Output (CSV 4.2.5)

Comparing the script tags

janraasch commented May 26, 2017

tagliala commented May 26, 2017

tagliala commented May 26, 2017

janraasch commented May 26, 2017

tagliala commented May 26, 2017 • edited Loading

tagliala commented May 26, 2017

janraasch commented May 26, 2017

tagliala commented May 26, 2017

Nested `simple_fields_for` no longer inherits `validate: true` (since v4.2.5) #680

Nested `simple_fields_for` no longer inherits `validate: true` (since v4.2.5) #680

janraasch commented Nov 8, 2016 •

edited

Loading

tagliala commented May 17, 2017 •

edited

Loading

tagliala commented May 22, 2017 •

edited

Loading

tagliala commented May 22, 2017 •

edited

Loading

janraasch commented May 26, 2017 •

edited

Loading

tagliala commented May 26, 2017 •

edited

Loading