feat: Allow length validation on associations #1569
Conversation
| if array_column? | ||
| ['x'] * length | ||
| elsif association? | ||
| Array.new(length) { association.klass.new } | ||
| else | ||
| 'x' * length | ||
| end |
There was a problem hiding this comment.
My initial idea was to multiply by length the return of the condition, like the example bellow.
if array_column?
['x']
elsif association?
[association.klass.new]
else
'x'
end * lengthHowever, this causes the array of the associated object to have the same object_id, and Rails prevents the same object from being associated twice check on collection_association class on active record gem for more context. That's why I needed to change this logic, so we can have different object_ids on the array of associations.
matsales28
force-pushed
the
feat/allow-length-validation-on-associations
branch
from
28f06fe
to
e8e1303
Compare
lib/shoulda/matchers/active_model/validate_length_of_matcher.rb
Outdated
Show resolved
Hide resolved
| message = <<-MESSAGE | ||
| Expected Parent not to validate that the length of :children is at least | ||
| 4, but this could not be proved. | ||
| After setting :children to ‹[#<Child id: nil>, #<Child id: nil>, |
There was a problem hiding this comment.
This is out of scope for this PR, but this error message seems strange to me. If a model doesn't validate that an association matches a certain length, then it should allow a value that would have been invalid with the validation in place. I don't see why having four children should be invalid in this case. If anything having three children should be valid. Of course that counts as breaking behavior, and it also could be inconsistent with how "negative" versions of other matchers work. It would have to be changed in concert with other matchers, probably. So I'm not eager to change it now, but I thought I'd point it out.
There was a problem hiding this comment.
I share your sentiment. The "negative" message sounded very strange to me when I first started working on shoulda-matchers. However, I attributed this perception to my non-native English background. Reading it in Portuguese, my primary language, sounded weird. Should we open an issue to discuss this?
There was a problem hiding this comment.
Yeah, that sounds like a good idea!
This commit allows the length matcher to be used on associations. It does this by checking if the attribute is an association, and if so, it uses the associations as the attribute to validate. This commit also test for the length matcher on associations (has_many and has_many through). I want to give credit to @prashantjois for the initial work on this feature. I took his work and expanded on it. Closes thoughtbot#1007
matsales28
force-pushed
the
feat/allow-length-validation-on-associations
branch
from
e8e1303
to
a98fb2d
Compare
This commit allows the length matcher to be used on associations. It does this by checking if the attribute is an association, and if so, it uses the associations as the attribute to validate.
This commit also adds tests for the length matcher on associations (has_many and has_many through).
I want to credit @prashantjois for the initial work on this feature #1124. I took his work and expanded on it.
If/When this PR gets merged, it'll close #1007 and #1124.