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

Cannot validate form when using Composition and dry-validation #451

Open
cmrichards opened this issue Jan 11, 2018 · 4 comments
Open

Cannot validate form when using Composition and dry-validation #451

cmrichards opened this issue Jan 11, 2018 · 4 comments
Milestone

Comments

@cmrichards
Copy link

cmrichards commented Jan 11, 2018

Complete Description of Issue

When using dry-validation for validation and using Composition, I cannot get the form to validate.

Steps to reproduce

class TestForm < Reform::Form 
  include Composition
  property :title, on: :first
  property :first_name, on: :second
  validation do 
    required(:title).filled(:str?)
    required(:first_name).filled(:str?)
  end
end

# First try with strings as keys
f = TestForm.new(first: OpenStruct.new, second: OpenStruct.new)
f.validate({"title": "Mr", "first_name": "Bob"})
=> false

# Then try with symbols as keys
f = TestForm.new(first: OpenStruct.new, second: OpenStruct.new)
f.validate({title: "Mr", first_name: "Bob"})
=> false

Expected behavior

It should validate

Actual behavior

It does not validate

System configuration

reform 2.2.4
dry-validation 0.11.1
rails 5.2.0.beta2

@orbanbotond
Copy link

We have the same here:

  class RetailerOnboardInput < Reform::Form
    include Composition
    feature Reform::Form::Dry

    property :location_country, on: :investor
    property :first_name, on: :user
    property :last_name, on: :user

    validation :default do
      required(:location_country).filled(:str?)
      required(:first_name).filled(:str?)
      required(:last_name).filled(:str?)
    end
form.valid?
=> false
[2] pry(#<RSpec::ExampleGroups::InvestorRetailerOnboardInput::PositiveCase>)> form.errors
=> #<Reform::Contract::Errors:0x007fa56ebf61c8
 @errors={:location_country=>["is missing"], :first_name=>["is missing"], :last_name=>["is missing"]}>
[3] pry(#<RSpec::ExampleGroups::InvestorRetailerOnboardInput::PositiveCase>)> form.first_name
=> "Tommie"

@siassaj
Copy link

siassaj commented Apr 12, 2018

We're having this too.

Essentially, it's very similar to
#438

If you do

    validation :default do
      required(:investor).schema do
        required(:location_country).filled(:str?)
      end
      required(:user).schema do
        required(:first_name).filled(:str?)
        required(:last_name).filled(:str?)
      end
    end

Then your validation will work, but the errors hash produced will not be usable with rails style form builders.

So we have a problem of what hash is being passed to dry-v

@siassaj
Copy link

siassaj commented Apr 12, 2018

More info, issue is here:

In file lib/disposable/twin/composition.rb in the Disposable library (0.4.3 for me) we have

     def to_nested_hash(*)
        hash = {}

        @model.each do |name, model| # TODO: provide list of composee attributes in Composition.
          part_properties = schema.find_all { |dfn| dfn[:on] == name }.collect{ |dfn| dfn[:name].to_sym }
          hash[name] = self.class.nested_hash_representer.new(self).to_hash(include: part_properties)
        end

        hash
      end

The method is geared to iterate over the models.

But we need to use a non nested hash in reform::form...
reform form needs a normalized_hash or.... actually a the_hash_i_expected

The fix depends on whether Composition in twin is doing the wrong thing with nested_hash (which looks right to me) or if reform's supposed to use a flat hash...

@siassaj
Copy link

siassaj commented Apr 12, 2018

In 2.1.0,
reform/lib/reform/validation.rb

  def valid?
    Groups::Result.new(self.class.validation_groups).(@fields, errors, self)
  end

But in 2.2.4

  def valid?
    Groups::Result.new(self.class.validation_groups).(to_nested_hash, errors, self)
  end

I think it should be @fields, not to_nested_hash

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

No branches or pull requests

4 participants