activerecord-hackery/ransack

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?

avit commented

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)
.