Check type before sending #value message to predicate
#1468
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spaghetticode
added a commit
to peterberkenbosch/solidus
that referenced
this pull request
Dec 15, 2023
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 🤷 |
elia
pushed a commit
to peterberkenbosch/solidus
that referenced
this pull request
Dec 20, 2023
Issue: activerecord-hackery/ransack#1467 PR: activerecord-hackery/ransack#1468 Hopefully the PR will be merged soon so we can remove this patch.
elia
pushed a commit
to peterberkenbosch/solidus
that referenced
this pull request
Dec 20, 2023
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.
spaghetticode
force-pushed
the
fix-casted_array-method
branch
from
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 |
MadelineCollier
added a commit
to MadelineCollier/solidus
that referenced
this pull request
Aug 7, 2024
Solidus core's gemspec already required that ransack be '~> 4.0', but the latest version of ransack, v4.2.0, released July 10 2024, introduces a bug. The previous implementation was taking for granted that every predicate would respond to #value, which doesn't seem to be the case when the predicate is an instance of a Arel::SelectManager. This has already been flagged by @spaghetticode in his PR against ransack: activerecord-hackery/ransack#1468 Since there has been little movement on this PR since January, we should lock to a version that works for us since currently many of our product specs are failing. (eg. spec/models/spree/product_spec.rb:659) We can remove this lock once the PR is merged and once the above test (and the others that are failing) are able to pass in ransack v4.2.0 or subsequent versions.
|
I am also having this issue, will there be any movement on this PR @scarroll32 ? For now I have locked my gemfile to |
3 tasks
MadelineCollier
added a commit
to MadelineCollier/solidus
that referenced
this pull request
Aug 8, 2024
Solidus core's gemspec already required that ransack be '~> 4.0', but the latest version of ransack, v4.2.0, released July 10 2024, introduces a bug. The previous implementation was taking for granted that every predicate would respond to #value, which doesn't seem to be the case when the predicate is an instance of a Arel::SelectManager. This has already been flagged by @spaghetticode in his PR against ransack: activerecord-hackery/ransack#1468 Since there has been little movement on this PR since January, we should lock to a version that works for us since currently many of our product specs are failing. (eg. spec/models/spree/product_spec.rb:659) We can remove this lock once the PR is merged and once the above test (and the others that are failing) are able to pass in ransack v4.2.0 or subsequent versions.
tvdeyen
pushed a commit
to tvdeyen/solidus
that referenced
this pull request
Aug 27, 2024
Solidus core's gemspec already required that ransack be '~> 4.0', but the latest version of ransack, v4.2.0, released July 10 2024, introduces a bug. The previous implementation was taking for granted that every predicate would respond to #value, which doesn't seem to be the case when the predicate is an instance of a Arel::SelectManager. This has already been flagged by @spaghetticode in his PR against ransack: activerecord-hackery/ransack#1468 Since there has been little movement on this PR since January, we should lock to a version that works for us since currently many of our product specs are failing. (eg. spec/models/spree/product_spec.rb:659) We can remove this lock once the PR is merged and once the above test (and the others that are failing) are able to pass in ransack v4.2.0 or subsequent versions. (cherry picked from commit 1664d10) # Conflicts: # core/solidus_core.gemspec
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.