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

[472] Allow arrays to be received for custom ransacker #486

Merged
merged 3 commits into from
Jan 4, 2015

Conversation

ideanl
Copy link
Contributor

@ideanl ideanl commented Jan 3, 2015

This is a try at fixing #472 and it works for me. Arel's update now has an Arel::Nodes::Casted type instead of the original array, so NULL gets passed into the SQL statement. This should cover all cases, and an if statement has been placed so this only will apply for when the Arel::Nodes::Casted type is used.

@jonatack
Copy link
Contributor

jonatack commented Jan 3, 2015

Thanks, @ideanl. The test suite is not passing. Did running the Ransack specs work for you with your PR?

The first step to take would be to write a test for the problem that works with Arel 5 and that fails with Arel 6 or Arel master. This would allow others to replicate the problem and then work efficiently to solve it.

@ideanl
Copy link
Contributor Author

ideanl commented Jan 3, 2015

Ok, I've updated the PR with a test that passes with Arel 5, but fails in Arel 6, and I've made all the tests pass by revising the code I added.

jonatack added a commit that referenced this pull request Jan 4, 2015
[472] Allow arrays to be received for ransacker with Rails 4.2/Arel 6
@jonatack jonatack merged commit b7f1401 into activerecord-hackery:master Jan 4, 2015
@jonatack
Copy link
Contributor

jonatack commented Jan 4, 2015

Thanks @ideanl, this improves compatibility with Rails 4.2 (Arel 6.0).

@jonatack
Copy link
Contributor

jonatack commented Jan 4, 2015

Would you like to update the change log?

It still does not work the way the OP wanted with searching on a string in #472, but yes, as you pointed out that's a separate issue.

jonatack added a commit that referenced this pull request Jan 4, 2015
the reason behind this patch and add a FIX ME comment to encourage PRs
to improve it.
ideanl added a commit to ideanl/ransack that referenced this pull request Jan 4, 2015
@ideanl
Copy link
Contributor Author

ideanl commented Jan 4, 2015

PR #487 is an update to the change log, now including this fix.

jonatack added a commit that referenced this pull request Jan 4, 2015
Update changelog with #486 [skip ci]
@jonatack
Copy link
Contributor

jonatack commented Jan 4, 2015

Thanks 😃

jonatack pushed a commit that referenced this pull request Jan 5, 2015
and tighten up the fix description for PRs #486 and #488.

[skip ci]
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.

2 participants