Improve dry integration #388

fran-worley · 2016-08-03T11:50:14Z

This enables you to either validate collection via a separate validation block within the collection or via dry-v's each { schema { # code... } }

Note:
Error messages aren't being generated correctly, need to check what the messages hash should look like in AMV first.

this just looks nicer: Reform::Form::DrySchema => becomes => Reform::Form::Dry::Schema

fran-worley · 2016-08-04T15:28:10Z

@apotonick can you have a look at this? The failing test is due to the key format of errors on a collection. I just need to have a look at how AM-V output them and then we should be good to go.

fran-worley · 2016-08-09T11:43:46Z

Ok so I've sorted out how the errors should map to objects and return consistently with the AM-V api. #389

My problem now is that for collections I need the fragment index in order to map the dry error to the correct collection item. @apotonick is there a way that we can store the fragment index in a collection form object for me to hook into?

Currently you have to pass it into the params hash for it to work which isn't ideal.

apotonick · 2016-08-13T17:29:13Z

@fran-worley Re. the index, you could "hack" it with

form.parent.songs.index(form)

BUT... we can make that simpler and set the index on the form itself or something like that.

rosskevin · 2016-08-26T22:04:22Z

Just updated, getting the following error:

expected Trailblazer::Operation::InvalidContract, got #<NoMethodError: undefined method `fragment_index' for #<#<Class:0x007fab3a6c3d28>:0x007fab3c9b8298>> with backtrace:
  # gems/reform-c4fc02510914/lib/reform/form/dry.rb:83:in `block (2 levels) in add_nested_error_message'
  # gems/disposable-0.4.0/lib/disposable/twin/property_processor.rb:24:in `block in collection!'
  # gems/disposable-0.4.0/lib/disposable/twin/property_processor.rb:24:in `collect'
  # gems/disposable-0.4.0/lib/disposable/twin/property_processor.rb:24:in `collection!'
  # gems/disposable-0.4.0/lib/disposable/twin/property_processor.rb:15:in `call'
  # gems/reform-c4fc02510914/lib/reform/form/dry.rb:81:in `block in add_nested_error_message'

Do you need any more info from me?

fran-worley · 2016-09-06T10:52:02Z

@rosskevin because I'd forgotten you were actually using this branch and that change is a bit of a hack right now. I'll sort this week and hopefully merge.

fran-worley · 2016-09-06T11:10:54Z

@rosskevin I've removed the hack you will need to make sure that you're adding:

require "disposable/twin/parent" 
feature Disposable::Twin::Parent

to your forms with collections or you'll get errors about an unknown parent method.

fran-worley · 2016-09-06T12:47:33Z

~~As I've changed the API for groups (again :)) we will need to merge this PR trailblazer/reform-rails#23 in reform-rails aswell.~~

fran-worley · 2016-09-06T13:04:27Z

So cool feature for reform/dry-v added. You can now inject your own dependencies into a schema without having to make them available via a method on your form. You can do so by passing in a :with option to your validation group as follows:

class SomeForm < Reform::Form
  validation :default, with: { user: current_user } do
    configure do
      option :user # as per dry-v docs, you must declare injected options in the configure block

      def users_name
        user.name # I can now access my user Yey!
      end
    end
    required(:username).filled(eql?: users_name)
  end
end

So I was being a twit. This saves us having to change the API and therefore reform-rails and also simplifies the call method

1) Fix incorrect assertion discovered on dry update 2) we won’t release 2.3.0 until dry-validation 0.10.0 is released

1) Store input params on the form 2) add a context option to validation groups (default to object) 3) if context is `:params` then we pass the raw input params to dry instead of the deserialised form object params. WARNING: You should make sure that handle coercion appropriately in dry-v if using this option, providing a custom schema if necessary.

fran-worley · 2016-10-25T15:51:30Z

Hi Kevin,

As per: #388 (comment) you'll need to include/ feature Parent when using collection validation via dry-v.

rosskevin · 2016-10-25T16:02:43Z

Thank you @fran-worley, I'm sorry I missed that.

I included it and now receive this:

/Users/kross/.rvm/gems/ruby-2.3.1@af/gems/disposable-0.4.0/lib/disposable/twin/parent.rb:3:in `included': private method `property' called for #<Class:0x007f8cc9cf07f8> (NoMethodError)
Did you mean?  properties
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/declarative-0.0.8/lib/declarative/schema.rb:72:in `include'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/declarative-0.0.8/lib/declarative/schema.rb:72:in `block in feature'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/declarative-0.0.8/lib/declarative/schema.rb:71:in `each'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/declarative-0.0.8/lib/declarative/schema.rb:71:in `feature'
    from /Users/kross/projects/af/app/concepts/af/commerce/base_itemized_document/operation.rb:13:in `block in <class:Create>'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/trailblazer-1.1.0/lib/trailblazer/operation.rb:56:in `class_eval'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/trailblazer-1.1.0/lib/trailblazer/operation.rb:56:in `contract'
    from /Users/kross/projects/af/app/concepts/af/commerce/base_itemized_document/operation.rb:12:in `<class:Create>'
    from /Users/kross/projects/af/app/concepts/af/commerce/base_itemized_document/operation.rb:7:in `<class:BaseItemizedDocument>'
    from /Users/kross/projects/af/app/concepts/af/commerce/base_itemized_document/operation.rb:6:in `<module:Commerce>'
    from /Users/kross/projects/af/app/concepts/af/commerce/base_itemized_document/operation.rb:5:in `<module:Af>'
    from /Users/kross/projects/af/app/concepts/af/commerce/base_itemized_document/operation.rb:4:in `<top (required)>'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:293:in `require'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:293:in `block in require'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:259:in `load_dependency'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:293:in `require'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:380:in `block in require_or_load'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:37:in `block in load_interlock'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies/interlock.rb:12:in `block in loading'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/concurrency/share_lock.rb:117:in `exclusive'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies/interlock.rb:11:in `loading'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:37:in `load_interlock'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:358:in `require_or_load'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:336:in `depend_on'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:252:in `require_dependency'
    from /Users/kross/projects/af/app/concepts/af/commerce/base_order/operation.rb:2:in `<top (required)>'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:293:in `require'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:293:in `block in require'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:259:in `load_dependency'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:293:in `require'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:380:in `block in require_or_load'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:37:in `block in load_interlock'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies/interlock.rb:12:in `block in loading'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/concurrency/share_lock.rb:117:in `exclusive'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies/interlock.rb:11:in `loading'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:37:in `load_interlock'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:358:in `require_or_load'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:336:in `depend_on'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/dependencies.rb:252:in `require_dependency'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/bundler/gems/trailblazer-rails-39db11b99f1e/lib/trailblazer/rails/railtie.rb:16:in `block in load_for'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/trailblazer-loader-0.1.0/lib/trailblazer/loader.rb:75:in `block in load_files'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/trailblazer-loader-0.1.0/lib/trailblazer/loader.rb:75:in `each'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/trailblazer-loader-0.1.0/lib/trailblazer/loader.rb:75:in `load_files'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/trailblazer-loader-0.1.0/lib/trailblazer/loader.rb:38:in `call'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/bundler/gems/trailblazer-rails-39db11b99f1e/lib/trailblazer/rails/railtie.rb:16:in `load_for'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/bundler/gems/trailblazer-rails-39db11b99f1e/lib/trailblazer/rails/railtie.rb:11:in `block in load_concepts'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/bundler/gems/trailblazer-rails-39db11b99f1e/lib/trailblazer/rails/railtie.rb:10:in `each'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/bundler/gems/trailblazer-rails-39db11b99f1e/lib/trailblazer/rails/railtie.rb:10:in `load_concepts'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/bundler/gems/trailblazer-rails-39db11b99f1e/lib/trailblazer/rails/railtie.rb:29:in `block (2 levels) in <class:Railtie>'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:396:in `instance_exec'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:396:in `block in make_lambda'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:169:in `block (2 levels) in halting'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:547:in `block (2 levels) in default_terminator'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:546:in `catch'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:546:in `block in default_terminator'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:170:in `block in halting'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:454:in `block in call'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:454:in `each'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:454:in `call'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:101:in `__run_callbacks__'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:750:in `_run_prepare_callbacks'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/callbacks.rb:90:in `run_callbacks'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/activesupport-5.0.0.1/lib/active_support/reloader.rb:87:in `prepare!'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/railties-5.0.0.1/lib/rails/application/finisher.rb:53:in `block in <module:Finisher>'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/railties-5.0.0.1/lib/rails/initializable.rb:30:in `instance_exec'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/railties-5.0.0.1/lib/rails/initializable.rb:30:in `run'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/railties-5.0.0.1/lib/rails/initializable.rb:55:in `block in run_initializers'
    from /Users/kross/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/tsort.rb:228:in `block in tsort_each'
    from /Users/kross/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/tsort.rb:350:in `block (2 levels) in each_strongly_connected_component'
    from /Users/kross/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/tsort.rb:431:in `each_strongly_connected_component_from'
    from /Users/kross/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/tsort.rb:349:in `block in each_strongly_connected_component'
    from /Users/kross/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/tsort.rb:347:in `each'
    from /Users/kross/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/tsort.rb:347:in `call'
    from /Users/kross/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/tsort.rb:347:in `each_strongly_connected_component'
    from /Users/kross/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/tsort.rb:226:in `tsort_each'
    from /Users/kross/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/tsort.rb:205:in `tsort_each'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/railties-5.0.0.1/lib/rails/initializable.rb:54:in `run_initializers'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/railties-5.0.0.1/lib/rails/application.rb:352:in `initialize!'
    from /Users/kross/projects/af/spec/dummy/config/environment.rb:5:in `<top (required)>'
    from /Users/kross/projects/af/lib/af/test/test_helper.rb:32:in `require'
    from /Users/kross/projects/af/lib/af/test/test_helper.rb:32:in `<top (required)>'
    from /Users/kross/projects/af/lib/af/spec/spec_helper.rb:2:in `require'
    from /Users/kross/projects/af/lib/af/spec/spec_helper.rb:2:in `<top (required)>'
    from /Users/kross/projects/af/spec/spec_helper.rb:1:in `require'
    from /Users/kross/projects/af/spec/spec_helper.rb:1:in `<top (required)>'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1394:in `require'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1394:in `block in requires='
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1394:in `each'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1394:in `requires='
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/configuration_options.rb:112:in `block in process_options_into'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/configuration_options.rb:111:in `each'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/configuration_options.rb:111:in `process_options_into'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/configuration_options.rb:21:in `configure'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:99:in `setup'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:86:in `run'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:71:in `run'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:45:in `invoke'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/gems/rspec-core-3.5.4/exe/rspec:4:in `<top (required)>'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/bin/rspec:22:in `load'
    from /Users/kross/.rvm/gems/ruby-2.3.1@af/bin/rspec:22:in `<top (required)>'
    from -e:1:in `load'
    from -e:1:in `<main>'

Here's a contract snippet:

        contract do
          feature Disposable::Twin::Parent
          collection :line_items, populate_if_empty: LineItem do
            property :product_id
            property :quantity
            property :period_start_at
            property :period_end_at
          end
        end

fran-worley · 2016-10-25T16:31:10Z

@rosskevin how weird this test passes fine:
https://github.com/apotonick/reform/blob/improve-dry-integration/test/validation/dry_validation_test.rb#L247-L356

Going to have to call in the big Disposable guns here... Any ideas @apotonick ??

rosskevin · 2016-10-25T19:13:07Z

Just to be sure I'm not leaving out anything useful, it is breaking on the line that calls feature Disposable::Twin::Parent, which wouldn't make sense if the test is running. I'll check out this branch, run tests, and look at dependency versions.

rosskevin · 2016-10-25T19:39:05Z

Tests run locally and nothing sticks out on the surface dependency-wise. I updated the error above with the full stack. I have to yank the call to feature Disposable::Twin::Parent because this error prevents anything else from running.

# Conflicts: # .travis.yml # gemfiles/Gemfile.disposable-0.4 # test/parse_pipeline_test.rb # test/validation/dry_validation_test.rb

This would allow users to declare their schema in a separate file if desired and just pass in as an option where desired.

With the improvements in dry, this shouldn’t be needed as often and as it causes all rules to be rebuilt we should really do this as little as possible. Also makes dry even more transparent to the user

fran-worley · 2016-11-18T15:23:53Z

Latest commit removes the default :form dependency injection into dry schemas. It also removes the Reform::Form::Schema class as it was no longer needed.

To manually inject your form instance into your schema, you just need to add with: { form: true } option to your validation call. We now inject your option :key into the schema configure block automatically.

E.g.

validation with: { form: true } do
  configure do
    def some_form_reliant_method?
      form.method?
    end
  end

  required(:some_key).filled(:some_form_reliant_method?)
end

JoeWoodward · 2016-12-13T15:26:05Z

test/validation/dry_validation_test.rb

+      )
+
+      result.must_equal false
+      form.band.errors.full_messages.inspect.must_equal %({:name=>["Name must be filled"], :\"label.name\"=>[\"Label Name must be filled\"]})


What's the reasoning for straying from the rails standard of full_messages?

Was this not added to keep it compatible with rails conventions?

Rails does not include the "property" key in the full messages. Instead it would be an array of compiled messages

e.g.

form.band.errors.full_messages.must_equal ["Name must be filled", "Label Name must be filled"]

Thanks for pointing this out. I'll confess that I've never actually used ActiveRecords full messages and couldn't remember exactly how they came out.

Does this look better? https://github.com/trailblazer/reform/tree/fix-arstyle-full-messages

Hey Fran, thanks for hopping on that so quickly. I don't see any changes in the branch you linked to? Did you forget to push?

I've just looked at the implementation and realised that I think it's actually not your fault at all. It's due to the way dry-validation formats the full_messages

reform/lib/reform/form/dry.rb

Line 13 in 330f6b6

Reform::Contract::Errors.new(self)

http://dry-rb.org/gems/dry-validation/error-messages/

Shoot sorry, trying to do to many things at once. Changes now pushed.

Sadly it is entirely my fault as we handle full errors in Reform and completely ignore what dry comes up with for many reasons. Essentially we take the regular dry messages and inject the property details into them.

The errors API included with native Reform will be changing to be in the dry-v style and the active record style will migrate to reform-rails. I'm just trying to make sure that we get our AR style messages when using dry correct before we go any further so thanks for your input here 😸

Hi Fran,

Sorry, feel asleep last night. I'm based in Bangkok so quite a time difference from you.

I left a note on the branch. The full_messages format is correct now

contentfree · 2017-02-04T03:33:05Z

So do usages of optional() in a validation scheme get treated as expected from dry-v docs? (I saw the issue got rolled into this one, but it looks like it was unresolved.)

apotonick · 2017-02-04T09:11:37Z

@contentfree Any example how that would be used and expected outcome?

fran-worley · 2017-02-04T13:44:24Z

@apotonick this was the purpose of the context option I added.

The issue with Reform is that if the property is declared on the form the key gets included regardless of whether it was passed in as a param.

If the key is then passed to dry then the optional key concept in dry is unusable with Reform.

apotonick · 2017-02-04T15:54:07Z

The solution here is passing only submitted attributes to dry?

fran-worley · 2017-02-04T21:01:40Z

But then what about default properties? Making it configurable might be better?

contentfree · 2017-02-08T22:51:04Z

I think the least-surprising experience for a dev would be for the optional/required abilities stated in the dry-v docs to work as described and for properties with defaults to work as expected.

siassaj · 2017-08-21T05:05:27Z

I think the least-surprising experience for a dev would be for the optional/required abilities stated in the dry-v docs to work as described and for properties with defaults to work as expected.

Thing is, we're either validating input params or the result of the form being populated. Can't really have it both ways with 1 set of validations. To be honest I'm close to declaring that reform::forms can have their own validation block which makes sure the data makes sense before sync, and operations can have their own validation block which makes sure the passed params make sense.

EDIT: Even more weirdly, if the params don't have the key, the populator isn't executed but the property's key is still passed into validation. If the params hash does have the key, the populator is executed and the mutated data is sent into validations.

Is the validation block in Reform::Form for validating the input params, or the transformed data in the form?

@apotonick whatcha think?

siassaj · 2017-08-21T05:19:26Z

As a follow up; this terrible stuff:

NOTE: I understand this a very contrived example but it does illustrate that we're validating the final shape of the form, not the user input.

property :location_id, populator: -> (opts) { self.location_id = nil }
required(:location_id).filled(:str?)

Order::Checkout.({location_id: "hi"})[:form].errors
=> #<Reform::Contract::Errors:0x00556d20c1db48
 @errors=
  {:location_id=>["must be filled"],

and

property :location_id, populator: -> (opts) { self.location_id = 4 }
required(:location_id).filled(:str?)

Order::Checkout.({location_id: "hi"})[:form].errors
=> #<Reform::Contract::Errors:0x00556d268bae50
 @errors=
  {:location_id=>["must be a string"],

apotonick · 2017-08-21T06:09:42Z

The idea in Reform has always been to validate the "final shape of the form" (good point @siassaj ), not the user input. This now is a problem because of the way dry-v works. Solution is to only pass changed/defaulted (good point @contentfree ) params to the dry-v engine.
With regards to orchestrating the flow of a validation, I have a massive addition to Reform planned, where the whole validation/deserialization/coercion/etc. will be an operation itself that can be reordered and the like. This will dramatically simplify how Reform works. Since TRB 2.1 is around the corner, I am expecting to find some time crafting a draft (and a draught) next week.

siassaj · 2017-08-21T06:23:39Z

I did a big edit cos the previous stuff i wrote was a bit hard to read.

The idea in Reform has always been to validate the "final shape of the form"

So I'm a bit torn;

Ultimately we expect to sync the form data into another domain, whereupon that domain should take care of what's valid or not anyway. (simple example, activerecord validations for what ultimately should/shouldn't go into db, or even better postgres triggers and rules etc.).

Then comes the stuff that flows into the form and is combined into the final shape of the form via populators, defaults, etc. In the simplest case this may be a combination of a couple populator lambdas, some defaults and the final params passed into the form.

In my thinking these sources of data ought to either be valid already, or validated by the form. This is where I'm torn;

These form objects back the actual form the user fills in, i want to validate the users input. It's the only source of data that could be invalid in almost every case I see. Incorrect defaults/populators and other sources of data ... they feel like something to be tested for. In any case those don't represent the inputs that the user fills in that will need errors displayed on.

It's not a trivial situation; we have domain objects that need to be valid before we use/store them. The form being in a valid shape before we sync makes sense. But we have a mismatch against validating user input as it comes. That user input is what I use the form for...

apotonick · 2017-08-21T06:28:08Z

That's exactly the "dilemma" we're in right now! Dry-v is really only meant for asserting input. Reform's statement is that we need higher-level business validations, too (e.g. if cart.items.count > 3 total amount needs to be > $100). To solve this, those two might have to get separated, which is why I want to introduce the validation "flow".

siassaj · 2017-08-21T06:31:06Z

Yep makes sense, We need validation flow.

I'm gonna wait for you to write that :P in the meantime i'll just hack reform::form to have a 2 step validation process :D

Basically, forms should validate user input, contracts should validate business logic. Contracts should do both

Whycomeyoumadecontract==form!\

contentfree · 2017-09-01T05:16:16Z

You can always make two contracts: One for the input and one for the model. Though I do agree with something @siassaj implied: ActiveRecord objects should be where its validation should reside. Ultimately, it's the thing responsible for syncing your data store (in your app… validate at the DB, too)

fran-worley added 4 commits August 3, 2016 12:46

rename base Dry-Schema class

306926c

this just looks nicer: Reform::Form::DrySchema => becomes => Reform::Form::Dry::Schema

expand test cases and hash symbolise method

58130ea

Merge branch 'master' into improve-dry-integration

214bfb5

only symbolize contents of array if hash

750a974

WIP error messages for nested dry-schemas

c4fc025

fran-worley mentioned this pull request Aug 9, 2016

Dry-Validation error messages #389

Closed

fran-worley added this to the v 2.3 milestone Aug 9, 2016

fran-worley added the dry-validation integration label Aug 9, 2016

add a spec for custom classes in type predicate (dry)

bfcd349

use parent instead of fragment index hack

2d04e4c

fran-worley added 2 commits September 6, 2016 12:34

ensure that form is accessible in dynamic args

c30d924

API CHANGE - support injection of custom args to dry

6ee0bd4

fran-worley added 9 commits September 6, 2016 19:49

Merge branch 'master' into improve-dry-integration

a4ecc34

Reverse API change: Even better 😄

615db6f

So I was being a twit. This saves us having to change the API and therefore reform-rails and also simplifies the call method

Use new validation API

bd7fa4c

WARN FIX: instance variable not initialised

c549128

Use now popular args API

bb95146

WARN FIX: Stop shadowing outer variables

8cdf920

WARN FIX: Unused variable

db61eca

Test with dry master (will be 0.10.0)

d6c0536

1) Fix incorrect assertion discovered on dry update 2) we won’t release 2.3.0 until dry-validation 0.10.0 is released

fran-worley mentioned this pull request Sep 14, 2016

Optional properties are treated as required #390

Closed

fran-worley added 5 commits November 11, 2016 14:48

Merge branch 'master' into improve-dry-integration

37f2734

# Conflicts: # .travis.yml # gemfiles/Gemfile.disposable-0.4 # test/parse_pipeline_test.rb # test/validation/dry_validation_test.rb

remove unused gem file

8949b4e

update for new dry configure api

fabcbde

Add a test for separate schema i.e. no validation block

764e87a

This would allow users to declare their schema in a separate file if desired and just pass in as an option where desired.

Stop including form by default

be13559

With the improvements in dry, this shouldn’t be needed as often and as it causes all rules to be rebuilt we should really do this as little as possible. Also makes dry even more transparent to the user

fran-worley added 3 commits November 24, 2016 18:14

simplify nested property errors

f5297cf

auto inject keys to schema config

e56a6cd

add support for full messages

5e8af01

fran-worley merged commit 330f6b6 into master Nov 24, 2016

fran-worley deleted the improve-dry-integration branch November 29, 2016 18:53

JoeWoodward reviewed Dec 13, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve dry integration #388

Improve dry integration #388

fran-worley commented Aug 3, 2016

fran-worley commented Aug 4, 2016

fran-worley commented Aug 9, 2016 •

edited

apotonick commented Aug 13, 2016

rosskevin commented Aug 26, 2016 •

edited

fran-worley commented Sep 6, 2016

fran-worley commented Sep 6, 2016

fran-worley commented Sep 6, 2016 •

edited

fran-worley commented Sep 6, 2016

fran-worley commented Oct 25, 2016

rosskevin commented Oct 25, 2016 •

edited

fran-worley commented Oct 25, 2016 •

edited

rosskevin commented Oct 25, 2016 •

edited

rosskevin commented Oct 25, 2016 •

edited

fran-worley commented Nov 18, 2016 •

edited

JoeWoodward Dec 13, 2016

fran-worley Dec 13, 2016

JoeWoodward Dec 13, 2016

JoeWoodward Dec 13, 2016

fran-worley Dec 13, 2016 •

edited

JoeWoodward Dec 14, 2016 •

edited

contentfree commented Feb 4, 2017

apotonick commented Feb 4, 2017

fran-worley commented Feb 4, 2017

apotonick commented Feb 4, 2017

fran-worley commented Feb 4, 2017

contentfree commented Feb 8, 2017

siassaj commented Aug 21, 2017 •

edited

siassaj commented Aug 21, 2017 •

edited

apotonick commented Aug 21, 2017

siassaj commented Aug 21, 2017 •

edited

apotonick commented Aug 21, 2017

siassaj commented Aug 21, 2017 •

edited

contentfree commented Sep 1, 2017

Improve dry integration #388

Improve dry integration #388

Conversation

fran-worley commented Aug 3, 2016

fran-worley commented Aug 4, 2016

fran-worley commented Aug 9, 2016 • edited

apotonick commented Aug 13, 2016

rosskevin commented Aug 26, 2016 • edited

fran-worley commented Sep 6, 2016

fran-worley commented Sep 6, 2016

fran-worley commented Sep 6, 2016 • edited

fran-worley commented Sep 6, 2016

fran-worley commented Oct 25, 2016

rosskevin commented Oct 25, 2016 • edited

fran-worley commented Oct 25, 2016 • edited

rosskevin commented Oct 25, 2016 • edited

rosskevin commented Oct 25, 2016 • edited

fran-worley commented Nov 18, 2016 • edited

JoeWoodward Dec 13, 2016

Choose a reason for hiding this comment

fran-worley Dec 13, 2016

Choose a reason for hiding this comment

JoeWoodward Dec 13, 2016

Choose a reason for hiding this comment

JoeWoodward Dec 13, 2016

Choose a reason for hiding this comment

fran-worley Dec 13, 2016 • edited

Choose a reason for hiding this comment

JoeWoodward Dec 14, 2016 • edited

Choose a reason for hiding this comment

contentfree commented Feb 4, 2017

apotonick commented Feb 4, 2017

fran-worley commented Feb 4, 2017

apotonick commented Feb 4, 2017

fran-worley commented Feb 4, 2017

contentfree commented Feb 8, 2017

siassaj commented Aug 21, 2017 • edited

siassaj commented Aug 21, 2017 • edited

apotonick commented Aug 21, 2017

siassaj commented Aug 21, 2017 • edited

apotonick commented Aug 21, 2017

siassaj commented Aug 21, 2017 • edited

contentfree commented Sep 1, 2017

fran-worley commented Aug 9, 2016 •

edited

rosskevin commented Aug 26, 2016 •

edited

fran-worley commented Sep 6, 2016 •

edited

rosskevin commented Oct 25, 2016 •

edited

fran-worley commented Oct 25, 2016 •

edited

rosskevin commented Oct 25, 2016 •

edited

rosskevin commented Oct 25, 2016 •

edited

fran-worley commented Nov 18, 2016 •

edited

fran-worley Dec 13, 2016 •

edited

JoeWoodward Dec 14, 2016 •

edited

siassaj commented Aug 21, 2017 •

edited

siassaj commented Aug 21, 2017 •

edited

siassaj commented Aug 21, 2017 •

edited

siassaj commented Aug 21, 2017 •

edited