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

Fix checking if a error was already added for nested validator #86

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shurikp
Copy link
Contributor

@shurikp shurikp commented Sep 2, 2022

Description

When

  • running validations several times (not sure why exactly, but that sometimes happens in Granite)
  • having :message set explicitly in the options

This check:

to.import(error, attribute: key) unless to.added?(key, error.type, error.options)

Doesn't do it's job well, since it gets to the strict matching in ActiveModel::Error:

options == @options.except(*CALLBACKS_OPTIONS + MESSAGE_OPTIONS)

and hence if we give :message in the options - it doesn't match it right.

This either seems a bug in ActiveModel or interface intended to be used without ever providing those keys in the options:

ActiveModel 6.1 and 7:

> errors = ActiveModel::Errors.new(nil)
> errors.add(:full_name, :weired, message: 'is weired')
> errors.added?(:full_name, :weired, message: 'is weired')
=> false
> errors.added?(:full_name, :weired)
=> true

ActiveModel 6.0 however behaves as one would expect:

> errors = ActiveModel::Errors.new(nil)
> errors.add(:full_name, :weired, message: 'is weired')
> errors.added?(:full_name, :weired, message: 'is weired')
=> true

How to test

I added a spec (and also explicitly set :message in the tested model), that fails if we don't omit those keys

Fixing ActiveModel?

Maybe I'll also make a PR in rails later, to my taste seems like a bug, if they merge it, this one won't be needed

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

Successfully merging this pull request may close these issues.

1 participant