Recent scopes addition broke #search(params[:q]) API
Closed this issue · 8 comments
It appears that the recent additions of adding scopes (which is great!) doesn't support the API of passing unmodified ActionController::Parameters
.
Assuming:
class Person < ActiveRecord::Base
scope :domestic, -> { where(country: 'US') }
def self.ransackable_scopes(auth_object=nil); [:domestic]; end
end
Here's a few examples:
Person.search(domestic: true) # scopes just perfectly
Form helper using a hidden input
<%= f.hidden_field :domestic %>
If the input value is set to true
, it is deserialized from the request as "true"
.
Calling Person.search(params[:q])
will raise ArgumentError: wrong number of arguments (1 for 0)
as Ransack tries to pass "true"
to the scope.
If the hidden value is set to ""
, Ransack skips applying the scope.
For now, we are able to manually deserialize these parameters in to the correct values, but just wanted to give you a heads up.
I should be able to look in to ransack's code some more this weekend.
Thanks @ohaibbq, I have to admit that I haven't been using the new scopes
feature so thank you for the heads up. Looks like we are missing some test coverage.
If you have the chance to submit a fix and a test that would be great.
No problem.. This is definitely a design call that I don't feel that comfortable making.
Should we accept "true"
as true
? Additionally, are we forcing users who wish to pass true
to their scope to pass it in an array?
For what it's worth, we have this workaround, maybe worth adding?
Ransack.configure do |config|
# If a boolean predicate expects a TRUE_VALUE and it got an empty string
# (shame on you RansackUI!), we should still allow it:
Ransack::Constants::TRUE_VALUES << ""
end
The problem is that currently there's no way to distinguish between the intents:
params = {"custom_scope" => "true"}
Model.search(params)
#=> Model.custom_scope
#=> Model.custom_scope("true")
#=> Model.custom_scope(true)
I assume a bare "true"
or true
should mean no arguments to the scope. If you want to pass that value to the scope, it should be in an array:
Model.search({"custom_scope" => "true"})
#=> Model.custom_scope
Model.search({"custom_scope" => ["true"]}
#=> Model.custom_scope("true")
As for type conversion, once it's treated as a parameter value inside the array, it's not our place to decide how it gets handled: if you need to write your scope to treat "false"
as false
you can do so. ("true"
is already truthy so the positive case is fine.)
Thanks for the response!
I'll look in to this workaround soon.
I agree that we should make users pass booleans to scopes by wrapping it in an array. Maybe we can justify interpreting string booleans as booleans for applying scopes and add that functionality in to Ransack.
@shekibobo wooooo!! thanks for taking the time to do this :)
Is this feature coming in version 1.5.2?
Yes.
You can use it now with the master branch, please don't hesitate to try it
out and report if any issues.
On Monday, November 17, 2014, otaviomedeiros notifications@github.com
wrote:
Is this feature coming in version 1.5.2?
—
Reply to this email directly or view it on GitHub
#403 (comment)
.
Thanks @jonatack