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

Custom validation context is missing #1318

Open
molfar opened this issue Nov 1, 2020 · 6 comments
Open

Custom validation context is missing #1318

molfar opened this issue Nov 1, 2020 · 6 comments

Comments

@molfar
Copy link

molfar commented Nov 1, 2020

Required labels missing when model has specific validation context, different from save update create.

next false if (validator_on.exclude?(:save)) && ((object.new_record? && validator_on.exclude?(:create)) || (!object.new_record? && validator_on.exclude?(:update)))

As result, such validators are skipped. I think there should be some context option for semantic_form_for

@mikz
Copy link
Contributor

mikz commented Nov 5, 2020

What validation context? Could you give an example of how that should work? Links to Rails documentation?

@molfar
Copy link
Author

molfar commented Nov 7, 2020

What validation context? Could you give an example of how that should work? Links to Rails documentation?

https://guides.rubyonrails.org/active_record_validations.html#on

When rendering form, fields that has presence validators with :on option, are displayed without required label.

@mikz
Copy link
Contributor

mikz commented Nov 7, 2020

(
  validator_on.exclude?(:save)) && ( # there is no on: :save
    (object.new_record? && validator_on.exclude?(:create)) # it is new record and there is no on :create
    ||
     (!object.new_record? && validator_on.exclude?(:update) # it is existing object and there is no :update
  )
)

I see the logic as sound.

And I see it tested.

context 'with options[:on] as symbol' do
context 'with save context' do
let (:validator) { double(options: {on: :save}, kind: :presence) }
it 'is required' do
expect(instance.required?).to be_truthy
end
end
context 'with create context' do
let (:validator) { double(options: {on: :create}, kind: :presence) }
it 'is required for new records' do
allow(model).to receive(:new_record?).and_return(true)
expect(instance.required?).to be_truthy
end
it 'is not required for existing records' do
allow(model).to receive(:new_record?).and_return(false)
expect(instance.required?).to be_falsey
end
end
context 'with update context' do
let (:validator) { double(options: {on: :update}, kind: :presence) }
it 'is not required for new records' do
allow(model).to receive(:new_record?).and_return(true)
expect(instance.required?).to be_falsey
end
it 'is required for existing records' do
allow(model).to receive(:new_record?).and_return(false)
expect(instance.required?).to be_truthy
end
end
end
context 'with options[:on] as array' do
context 'with save context' do
let (:validator) { double(options: {on: [:save]}, kind: :presence) }
it 'is required' do
expect(instance.required?).to be_truthy
end
end
context 'with create context' do
let (:validator) { double(options: {on: [:create]}, kind: :presence) }
it 'is required for new records' do
allow(model).to receive(:new_record?).and_return(true)
expect(instance.required?).to be_truthy
end
it 'is not required for existing records' do
allow(model).to receive(:new_record?).and_return(false)
expect(instance.required?).to be_falsey
end
end
context 'with update context' do
let (:validator) { double(options: {on: [:update]}, kind: :presence) }
it 'is not required for new records' do
allow(model).to receive(:new_record?).and_return(true)
expect(instance.required?).to be_falsey
end
it 'is required for existing records' do
allow(model).to receive(:new_record?).and_return(false)
expect(instance.required?).to be_truthy
end
end
context 'with save and create context' do
let (:validator) { double(options: {on: [:save, :create]}, kind: :presence) }
it 'is required for new records' do
allow(model).to receive(:new_record?).and_return(true)
expect(instance.required?).to be_truthy
end
it 'is required for existing records' do
allow(model).to receive(:new_record?).and_return(false)
expect(instance.required?).to be_truthy
end
end
context 'with save and update context' do
let (:validator) { double(options: {on: [:save, :create]}, kind: :presence) }
it 'is required for new records' do
allow(model).to receive(:new_record?).and_return(true)
expect(instance.required?).to be_truthy
end
it 'is required for existing records' do
allow(model).to receive(:new_record?).and_return(false)
expect(instance.required?).to be_truthy
end
end
context 'with create and update context' do
let (:validator) { double(options: {on: [:create, :update]}, kind: :presence) }
it 'is required for new records' do
allow(model).to receive(:new_record?).and_return(true)
expect(instance.required?).to be_truthy
end
it 'is required for existing records' do
allow(model).to receive(:new_record?).and_return(false)
expect(instance.required?).to be_truthy
end
end
context 'with save and other context' do
let (:validator) { double(options: {on: [:save, :foo]}, kind: :presence) }
it 'is required for new records' do
allow(model).to receive(:new_record?).and_return(true)
expect(instance.required?).to be_truthy
end
it 'is required for existing records' do
allow(model).to receive(:new_record?).and_return(false)
expect(instance.required?).to be_truthy
end
end
context 'with create and other context' do
let (:validator) { double(options: {on: [:create, :foo]}, kind: :presence) }
it 'is required for new records' do
allow(model).to receive(:new_record?).and_return(true)
expect(instance.required?).to be_truthy
end
it 'is not required for existing records' do
allow(model).to receive(:new_record?).and_return(false)
expect(instance.required?).to be_falsey
end
end
context 'with update and other context' do
let (:validator) { double(options: {on: [:update, :foo]}, kind: :presence) }
it 'is not required for new records' do
allow(model).to receive(:new_record?).and_return(true)
expect(instance.required?).to be_falsey
end
it 'is required for existing records' do
allow(model).to receive(:new_record?).and_return(false)
expect(instance.required?).to be_truthy
end
end
end
end

If it is not working for your case please try to contribute some example or a test case that shows what is wrong.

@pduersteler
Copy link

I just stumbled upon the same thing. My case is that I have a User model with a multi-step onboarding wizard where I validate the fields based on the current step the user is in. There are multiple edit views for the user that split up form fields by their logic (e. g. address, profile information).

# Contextual validations for step 1 in form wizard.
with_options on: %i[basic_information create] do
  validates :name, presence: true
  validates :country, presence: true, country: true
end

# Contextual validations for edit profile action.
with_options on: %i[edit_profile] do
  validates :name, presence: true
end

This will render the name field in a form as "optional" because the context is missing. Of course it can be circumvented by manually putting required: true to those fields but the magic is lost then.

@mikz
Copy link
Contributor

mikz commented Jun 1, 2021

Ok, and how would you want the form to look like?

semantic_form_for user, validation_context: :basic_information do |f|

Or do it per input? Or for some inputs at ones?

I think this could make sense if done properly, so it also makes it easier for you to hook into determining what is required.

Maybe extracting the this:

if validator.options.key?(:on)
validator_on = Array(validator.options[:on])
next false if (validator_on.exclude?(:save)) && ((object.new_record? && validator_on.exclude?(:create)) || (!object.new_record? && validator_on.exclude?(:update)))
end
case validator.kind
when :presence
true
when :inclusion
validator.options[:allow_blank] != true
when :length
validator.options[:allow_blank] != true &&
validator.options[:minimum].to_i > 0 ||
validator.options[:within].try(:first).to_i > 0
else
false
end

Into a method that is easily overridable in your custom form builder.

@lostapathy
Copy link
Contributor

Just stumbled into wanting this myself. Yes, the example syntax floated above by @mikz seems like it'd be great.

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

No branches or pull requests

4 participants