-
-
Notifications
You must be signed in to change notification settings - Fork 780
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
Check type before sending #value
message to predicate
#1468
base: main
Are you sure you want to change the base?
Check type before sending #value
message to predicate
#1468
Conversation
Issue: activerecord-hackery/ransack#1467 PR: activerecord-hackery/ransack#1468 Hopefully the PR will be merged soon so we can remove this patch.
Interestingly, CI is not running on the PR 🤷 |
Issue: activerecord-hackery/ransack#1467 PR: activerecord-hackery/ransack#1468 Hopefully the PR will be merged soon so we can remove this patch.
Issue: activerecord-hackery/ransack#1467 PR: activerecord-hackery/ransack#1468 Hopefully the PR will be merged soon so we can remove this patch.
Needs to be approved for the 1st run. |
@spaghetticode could you please add a test for this change? |
The previous implementation was giving for granted that every predicate respond to `#value`, but that doesn't seem to be the case at least when having a `Arel::SelectManager`. by simply inverting the terms of the existing AND check we can fix the issue without introducing unknown side effects. In order to test the change, a new ransacker has been added to the Person model.
e72e954
to
e7c8250
Compare
@scarroll32 I've just added a test that fails without the change. The method I modified is private, so it needs to be tested indirectly. I followed the existing examples, but I had to add a new ransacker to the |
Closes #1467
The previous implementation was giving for granted that every predicate respond to
#value
, which doesn't seem to be the case at least when having aArel::SelectManager
.by simply inverting the terms of the existing AND check we can fix the issue without introducing unknown side effects.