Feature/536 investigate association rule issue #650
Conversation
|
Sure ^^ I will see to it today or tomorrow :D |
Fix an issue that caused associations not to work in their usage as a subject for a rule
|
I added some tests to verify that this also works with |
lib/cancan/conditions_matcher.rb
Outdated
| @@ -78,7 +78,8 @@ def condition_match?(attribute, value) | |||
|
|
|||
| def hash_condition_match?(attribute, value) | |||
| if attribute.is_a?(Array) || (defined?(ActiveRecord) && attribute.is_a?(ActiveRecord::Relation)) | |||
| attribute.any? { |element| matches_conditions_hash?(element, value) } | |||
| match_results = attribute.map { |element| matches_conditions_hash?(element, value) } | |||
| match_results.any? | |||
There was a problem hiding this comment.
rather than the multi-line solution, how do you feel about attribute.to_a.any? { |element| matches_conditions_hash?(element, value) }
if attribute is already an array, to_a will do nothing. if it's not, it'll force the relation to be loaded.
i just think that code makes a bit more sense than what you have here - if i saw this, i'd want to refactor it back to the old version.
There was a problem hiding this comment.
That absolutely makes a lot of sense ^^ Your change looks a lot cleaner and thus I'm happy to revert to just adding to_a to force the relation loading
ghost
removed their assignment
ghost
requested a review
from
| @@ -78,7 +78,7 @@ def condition_match?(attribute, value) | |||
|
|
|||
| def hash_condition_match?(attribute, value) | |||
| if attribute.is_a?(Array) || (defined?(ActiveRecord) && attribute.is_a?(ActiveRecord::Relation)) | |||
| attribute.any? { |element| matches_conditions_hash?(element, value) } | |||
| attribute.to_a.any? { |element| matches_conditions_hash?(element, value) } | |||
There was a problem hiding this comment.
@Liberatys if I remove the .to_a here, all tests still pass locally for me.
Do you know if I am missing something?
I've tried testing against AR 5.2 and 6.0 on sqlite and postgres.
There was a problem hiding this comment.
@ghiculescu the tests are mostly written to ensure / prove the functionality. The issue prominently arose in the setting of the example provided in the original issue.
The discussion in the ticket outlined the issue and this change was made to address the behavior that was described in the example repo :)
There was a problem hiding this comment.
Got it. My concern is that if the tests pass even with the fix removed, then they aren't preventing us from regressing again.
I'm doing some refactoring of this code as part of #653 and I don't want to accidentally break it.
Do you think there is a test missing that I can include in that PR to ensure I don't regress?
Fix the issue outlined in #536 and validate behaviour in test