Validations are only run once

[joshleblanc]

With the recent change, validations are only run once.

This is due to this line: https://github.com/leastbad/optimism/commit/9711063a0488180282c710adbf5f6eaeab5ec1b8#diff-36b27dd62524e42bc61c16a76163fd30R41

After you validate once, the errors object will no longer be empty. Therefor, it will never re-validate the model after updating it.

[joshleblanc]

The more I think about this, the more I think broadcast_errors shouldn’t be running the validations at all.

It might be a good idea to just remove that line and have the programmer run validations before broadcast_errors

[leastbad]

Hey @joshleblanc, I really appreciate you still thinking about this.

I am a bit stumped as to the problem you're trying to solve. Admittedly, I run this library inside of controller actions like create and update, so in a very real way, the line you linked to is for you in a literal and immediate sense. If it's not working for you, it doesn't make sense to keep it.

Right now, I have the Optimism demo running on the latest version of the library with nested attributes and so far as I can tell, everything is currently working exactly as I would expect it to. I put in invalid data and submit it... errors appear. I correct the data and submit it... errors disappear.

Calling valid? was a really good suggestion that I wouldn't have considered without you. Why would I need to call it more than once, though? In what scenario is that a problem?

I've watched your animation for a long time and I'm not sure what you're illustrating with it. Please dumb it down for me. :)

[joshleblanc]

You’d be calling it more than once in the situation where you have the model in the session and you’re using it in a reflex.

In your demo, you’re operating on a brand new model. You hit submit, it constructs the model, tries to saves it (which runs validations), then broadcasts errors.

In my gif, I’m assigning the value in the text box to the model in the session, then running broadcast_errors. I’m not creating a new model, it’s the same model that was operated on before.

That means that it still contains the errors from the first validation, and therefore doesn’t validate again.

model = Foo.create(name: "")
broadcast_errors(model, :name)
# => broadcasts "Name must be at least 5 characters long" 
model.name = "12345"
broadcast_errors(model, :name)
# => broadcasts "Name must be at least 5 characters long" because validation are not re-run.

[leastbad]

I finally understand, thank you.

Long story short: at no point in the documentation or example do I ever suggest that this usage is supported - much less recommended practice - and if it’s absolutely necessary to store an AR model in your session, you’ll simply have to call valid? on it as you find necessary.

This library was designed to be called from a controller as part of the create/update loop, specifically to make error handling work for remote forms. What you’re trying to do is pretty left-field, honestly.

Storing complex objects in the session has always been a discouraged practice in Rails. It’s likely that there’s a better way to accomplish what you’re trying to do that is far less vulnerable to side-effects.

I’m glad that I added the extra check for valid? on your suggestion. I think it makes it a more resilient library. But I don’t want to repeatedly spam validations for an edge case that isn’t a technique I’d recommend. Many people have complex custom validations that call external services.

[joshleblanc]

I wasn't suggesting repeatedly spamming valid?, I was suggesting removing it 😄, for consistency's sake.

broadcast_errors suggests that you're broadcasting errors that already exist. So it would make sense to run validation yourself, then run broadcast_errors.

But, this is a non-issue. I can run the validations myself either way, and having the valid? call there does indeed improve resilience.

I'll go ahead and close this, thanks!