Skip to content
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

use-associations-validations #787

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MichalRemis
Copy link
Contributor

I need this change to make checkboxes/radiobuttons work in CSV-simple_form.

Currently in CSV validations are added to CSV hash (via collection_check_boxes/collection_radio_buttons overrides). Events aren't attached to radiobuttons (filtered out). For checkboxes, events are attached but when JS validating, their validations are NOT applied because of special names of association checkbox inputs (e.g. user[:roles][]). Not sure if this was an intention, but it is allright because standard presence validation wouldn't work for checkboxes anyway, and as I understand they are not going to be supported in main CSV.

But to make collections work in CSV-simple_form I need to use (overridden) validations and therefore need to clean input names user[:roles][]->user[:roles]. Couldn't do it in CSV-simple_form therefore updating it here in main. Maybe you have some better idea how to achieve this.

@MichalRemis MichalRemis force-pushed the attach-correctly-associations-validations branch from 1664e6a to d548184 Compare April 30, 2020 13:15
@coveralls
Copy link

coveralls commented Apr 30, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling d548184 on MichalRemis:attach-correctly-associations-validations into c7468a6 on DavyJonesLocker:master.

@tagliala
Copy link
Contributor

@MichalRemis can you please paste the generated HTML code of a page showing the issue, as well as the ruby template?

Maybe with both collection_check_boxes and collection_radio_buttons, so I can take a look of the actual behavior

@MichalRemis
Copy link
Contributor Author

MichalRemis commented Apr 30, 2020

Sure:

ruby:

class Box < ApplicationRecord
  validates :colors, presence: true
  validates :material, presence: true

  belongs_to :material
  has_and_belongs_to_many :colors
end
<%= form_for @box, validate: true do |f| %>
    <%= f.collection_radio_buttons :material, Material.all, :id, :name %>
    <%= f.collection_check_boxes :colors, Color.all, :id, :name %>

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

HTML:

<form class="new_box" id="new_box" novalidate="novalidate" data-client-side-validations="{&quot;html_settings&quot;:{&quot;type&quot;:&quot;ActionView::Helpers::FormBuilder&quot;,&quot;input_tag&quot;:&quot;\u003cdiv class=\&quot;field_with_errors\&quot;\u003e\u003cspan id=\&quot;input_tag\&quot;\u003e\u003c/span\u003e\u003c/div\u003e&quot;,&quot;label_tag&quot;:&quot;\u003cdiv class=\&quot;field_with_errors\&quot;\u003e\u003clabel id=\&quot;label_tag\&quot;\u003e\u003c/label\u003e\u003c/div\u003e&quot;},&quot;number_format&quot;:{&quot;separator&quot;:&quot;.&quot;,&quot;delimiter&quot;:&quot;,&quot;},&quot;validators&quot;:{&quot;box[material]&quot;:{&quot;presence&quot;:[{&quot;message&quot;:&quot;can't be blank&quot;},{&quot;message&quot;:&quot;must exist&quot;}]},&quot;box[colors]&quot;:{&quot;length&quot;:[{&quot;messages&quot;:{&quot;minimum&quot;:&quot;is too short (minimum is 1 character)&quot;,&quot;maximum&quot;:&quot;is too long (maximum is 2 characters)&quot;},&quot;minimum&quot;:1,&quot;maximum&quot;:2}]}}}" action="/boxes" accept-charset="UTF-8" method="post">
  <input name="utf8" type="hidden" value="✓">
  <input type="hidden" name="authenticity_token" value="6TvRPJ5x9rYO1hE4n3kwWBraopOEOmXI9YU93cNFbXVnaFk7RuUgCv8BzP6naBoHRO5qapfWkJppgf9tIN3ITg==">
  
  <input type="hidden" name="box[material]" value="" data-validate="true">
  <input type="radio" value="1" name="box[material]" id="box_material_1" data-validate="true">
  <label for="box_material_1">Wood</label>
  <input type="radio" value="2" name="box[material]" id="box_material_2" data-validate="true">
  <label for="box_material_2">Plastic</label>
  <input type="radio" value="3" name="box[material]" id="box_material_3" data-validate="true">
  <label for="box_material_3">Glass</label>
  
  <input type="hidden" name="box[colors][]" value="">
  <input type="checkbox" value="1" name="box[colors][]" id="box_colors_1" data-validate="true">
  <label for="box_colors_1">Red</label>
  <input type="checkbox" value="2" name="box[colors][]" id="box_colors_2" data-validate="true">
  <label for="box_colors_2">Green</label>
  <input type="checkbox" value="3" name="box[colors][]" id="box_colors_3" data-validate="true">
  <label for="box_colors_3">Blue</label>

  <input type="submit" name="commit" value="Create Box" data-disable-with="Create Box">
</form>

Problem with plain Rails radio/checkbox collections is that they don't have a wrapper around them like simple_form does.

Regarding this PR. You can see validations for box[colors] are in the CSV hash but currently when JS validating checkbox input,

$.fn.isValid = function (validators) {
const obj = $(this[0])
if (obj.is('form')) {
return validateForm(obj, validators)
} else {
return validateElement(obj, validatorsFor(this[0].name, validators))
}
}

it doesn't use box[colors] JS validator because name of input is box[colors][]. This function:

const cleanElementName = (elementName, validators) => {
and my PR's change in it cleans the name to box[colors] to apply validators correctly.

@tagliala
Copy link
Contributor

tagliala commented Apr 30, 2020

Thanks

Got the need for different names

we should do three things:

  1. Move /\[(\w+_attributes)\]\[[\da-z_]+\](?=\[(?:\w+_attributes)\])/g to a self-explaining constant. I don't remember what this regexp ecaxtly do, we need to go back in history. -- SEPARATE PR/COMMIT
  2. See if we can safely omit the trailing [] when checking for validations.
  3. Find another solution (we can't check for the formbuilder's name)

@MichalRemis
Copy link
Contributor Author

MichalRemis commented Apr 30, 2020

  1. Move /\[(\w+_attributes)\]\[[\da-z_]+\](?=\[(?:\w+_attributes)\])/g to a self-explaining constant. I don't remember what this regexp ecaxtly do, we need to go back in history. -- SEPARATE PR/COMMIT

Look's like it normalizes input name for nested attributes https://api.rubyonrails.org/classes/ActiveRecord/NestedAttributes/ClassMethods.html

  1. See if we can safely omit the trailing [] when checking for validations.

I don't think we may omit it safely for mainCSV, because JS validators, e.g.:

export const presenceLocalValidator = (element, options) => {
if (!valueIsPresent(element.val())) {
return options.message
}
}

aren't ready for checkboxes and would report false errors for association checkbox's inputs (not tested though). That's why I added SimpleForm::FormBuilder condition. It would be safe if you tweak validators to work with checkboxes, something like https://github.com/MichalRemis/client_side_validations-simple_form/blob/add-checkboxes-and-radios-support/src/validator_overrides/presence.js

  1. Find another solution (we can't check for the formbuilder's name)

If we can't check formbuilder's name and you have no other idea or won't implement working collection_checkboxes in main CSV, I could implement it in CSV_simple-form, but it would require a lot of code copy & pasting from main - I am not sure if that's a good idea.

Base automatically changed from master to main February 13, 2021 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants