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

Multiple fields causing validation not to run properly #300

Closed
methyl opened this issue Sep 18, 2015 · 11 comments
Closed

Multiple fields causing validation not to run properly #300

methyl opened this issue Sep 18, 2015 · 11 comments
Assignees

Comments

@methyl
Copy link

methyl commented Sep 18, 2015

Let's have an Interactor:

class Test1 < ActiveInteraction::Base
  string :tag
  validates :tag,    presence: true

  def execute
  end
end

after passing empty string as a tag to it, I should get an error and this is exactly what happens:

Test1.run(tag: '').errors.messages # => {:tag=>["can't be blank"]}

Although when I have multiple fields in the interactor, as per:

class Test2 < ActiveInteraction::Base
  string :tag
  symbol :emails
  validates :tag,    presence: true

  def execute
  end
end

Error on tag field is not there anymore:

Test2.run(tag: '').errors.messages # => {:emails=>["is required"]}

As a reference, here's related Gemfile.lock part:

active_interaction (2.1.2)
      activemodel (>= 3.2, < 5)

It's reproducible on isolated Ruby 2.2.2 instance as well.

Thanks!

@tfausak tfausak added the bug label Sep 18, 2015
@tfausak tfausak self-assigned this Sep 18, 2015
@tfausak
Copy link
Collaborator

tfausak commented Sep 18, 2015

This is actually expected behavior. The validations section of the readme covers this. In particular:

First ActiveInteraction will type check your inputs. Then ActiveModel will validate them.

We do this so your validators can know that the inputs have been type checked already. See #196 for some discussion about that.

@methyl
Copy link
Author

methyl commented Sep 18, 2015

@tfausak let's say I have interactor which I pass to form_for. How do I properly handle this case, eg. how to display errors for empty fields in that case?

@tfausak
Copy link
Collaborator

tfausak commented Sep 18, 2015

Unfortunately I don't think you can. It would be nice if you could get both type checking errors (like "emails is required") and validation errors (like "tag can't be blank") all at once. But in general it is not possible to know which attributes a validator validates. Consider the following example:

string :x, :y

validate do
  if x.downcase == y.downcase
    errors.add(:x, "can't be equal to y")
    errors.add(:y, "can't be equal to x")
  end
end

The first validator uses both x and y, and can add errors to both of them. It also assumes that they're strings. So if you ran that with .run(x: '', y: nil), what would you expect the output to be?

@methyl
Copy link
Author

methyl commented Sep 18, 2015

Okay, so let's consider another example:

I have a form in which I have number input. I also have corresponding integer field in my Interactor. If I leave the input blank, it will pass an empty string to the interactor causing validation to fail, as an empty string is not a number. Is there any way to mitigate this issue?

What if we have an option, which converts empty strings to nils before type check? Is there any way to do it currently?

@tfausak
Copy link
Collaborator

tfausak commented Sep 18, 2015

Is there any way to do it currently?

There is! See this comment for an example: #195 (comment)

I think the semantics have changes a little since then (because we introduced the type_check callback before validate), but you should be able to do what you want using callbacks. Here's an example that works with the most recent version:

class Example < ActiveInteraction::Base
  integer :x,
    default: nil

  set_callback :type_check, :before, lambda { |interaction|
    interaction.x = interaction.x.presence
  }

  def execute
    inputs
  end
end

Example.run!(x: '')
# => {:x=>nil}

Edited to add: We cover callbacks in the readme: https://github.com/orgsync/active_interaction/blob/v2.1.2/README.md#callbacks

@methyl
Copy link
Author

methyl commented Sep 18, 2015

@tfausak I figured out something like this:

  def process_inputs(inputs)
    super(Hash[inputs.map {|k, v| [k, v.presence]}])
  end

Works as expected, not sure if it's better/worse than a callback though. Any way to make callback work for every input at once?

@methyl
Copy link
Author

methyl commented Sep 18, 2015

@tfausak this works:


  set_callback :type_check, :before, lambda { |interaction|
    interaction.inputs.keys.each { |key|
      interaction.send("#{key}=", interaction.send(key).presence)
    }
  }

Still process_inputs seems to be cleaner, which one would you recommend?

@tfausak
Copy link
Collaborator

tfausak commented Sep 18, 2015

Are you monkey patching ActiveInteraction::Base with that? Or are you creating a subclass and defining it on there?

Either way, that's going to be less future proof than using callbacks. process_inputs is not part of our publicly versioned API, which means it could change at any time for any reason. Callbacks are part of our publicly versioned API, so they can only change when the major version number does.

I would recommend putting this behavior in a concern and including it when you want this behavior.

module InputMangler
  extend ActiveSupport::Concern

  included do
    set_callback :type_check, :before, lambda { |interaction|
      interaction.inputs.keys.each do |input|
        interaction.public_send(
          "#{input}=", interaction.public_send(input).presence)
      end
    }
  end
end

class Example < ActiveInteraction::Base
  include InputMangler

  integer :x,
    default: nil

  def execute
    inputs
  end
end

Example.run!(x: '')
# => {:x=>nil}

@methyl
Copy link
Author

methyl commented Sep 18, 2015

Looks good, I think we can close this now. Anyway, I would recommend including such concern in ActiveInteraction itself, as my problem is an issue for every Interactor used together with form_for

@tfausak
Copy link
Collaborator

tfausak commented Sep 18, 2015

Can you create a new issue for that? I am open to discussing that addition.

@tfausak tfausak closed this as completed Sep 18, 2015
@methyl
Copy link
Author

methyl commented Sep 18, 2015

@tfausak I will try to figure something out and open PR

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

2 participants