-
-
Notifications
You must be signed in to change notification settings - Fork 810
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 issue evaluating polymorphic joins #1077
Fix issue evaluating polymorphic joins #1077
Conversation
@deivid-rodriguez Thank you for taking a stab at this. Luckily I wrote tests wrapping our use cases here, so I was able to pull your branch and run a few of them. I also initially thought that removing Polyamorous for 5.2 would "just work", but somewhere along the way the condition to constrain I can get a failing test by adding this line to https://github.com/deivid-rodriguez/ransack/blob/fix_rails_52_polymorphic_joins/spec/ransack/search_spec.rb#L288
Failure:
My monkey-patch in the original issue accounted for this in the first
With this patch, the query in the failing test should look like:
Emphasis on the |
Thank you @PhilCoggins! So, the code the posted in the issue should be correct, and we also have a regression test for it, right? Can you create a PR and I'll close this one, I don't want to steal attribution, you did all the work here :) |
@deivid-rodriguez The code posted in the issue works with ActiveRecord v5.2.3, but fails on 5-2 stable, see rails/rails@54de9b1. I've spent nearly a full day going back and forth between ActiveRecord and Polyamorous and I'm afraid I just can't figure it out 😞. I appreciate you trying to attribute the fix to me, but I really just want someone who already understands the internals of this library to fix it. |
Ok! I'll try to dig in more and see if we can fix this. |
cbecf6e
to
dab2049
Compare
I plan to (at least try to) figure this out soon 👍 |
@deivid-rodriguez based on your latest comment this is not ready to merge? I'm wondering why the CI is passing. |
No, it's not. If I recall correctly, CI is passing because the assertion in my spec is too soft (needs to be strenghten by the assertion proposed by @PhilCoggins), and because tests against 5-2-stable and 6-0-stable are currently in the "allowed_failures" section, so failures there are "hidden". |
Hi guys. I'm sorry to be a PITA. We're trying to upgrade our project from rails 5.2.x to 6.0 and this requires us to upgrade ransack from 2.1.1 to 2.3.0. While doing that, some of our tests are failing, apparently because of the bug this patch might solve. Has anyone been able to use ransack 2.3.0 with rails 5.2.x and rails 6.0.x successfully without this patch? |
@oboxodo please see this comment #1081 (comment) |
Thanks for fixing this @PhilCoggins!! |
I have this "ArgumentError: wrong number of arguments (given 3, expected 2)" Don't sure it's related with one you all discuss |
Fixes #1039, inspired on @PhilCoggins's investigation in #1039 (comment), but being initially more aggressive.