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

Recent scopes addition broke #search(params[:q]) API #403

Closed
ohaibbq opened this issue Aug 1, 2014 · 8 comments · Fixed by #460
Closed

Recent scopes addition broke #search(params[:q]) API #403

ohaibbq opened this issue Aug 1, 2014 · 8 comments · Fixed by #460

Comments

@ohaibbq
Copy link
Contributor

ohaibbq commented Aug 1, 2014

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.

@jonatack
Copy link
Contributor

jonatack commented Aug 1, 2014

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.

@ohaibbq
Copy link
Contributor Author

ohaibbq commented Aug 1, 2014

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
Copy link
Contributor

avit commented Sep 5, 2014

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

@ohaibbq
Copy link
Contributor Author

ohaibbq commented Sep 11, 2014

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 added a commit to shekibobo/ransack that referenced this issue Oct 30, 2014
shekibobo added a commit to shekibobo/ransack that referenced this issue Oct 30, 2014
shekibobo added a commit to shekibobo/ransack that referenced this issue Oct 30, 2014
shekibobo added a commit to shekibobo/ransack that referenced this issue Oct 31, 2014
jonatack added a commit that referenced this issue Oct 31, 2014
Allow passing stringy booleans as scope args. Fixes #403.
@ohaibbq
Copy link
Contributor Author

ohaibbq commented Nov 11, 2014

@shekibobo wooooo!! thanks for taking the time to do this :)

@otaviomedeiros
Copy link

Is this feature coming in version 1.5.2?

@jonatack
Copy link
Contributor

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

@otaviomedeiros
Copy link

Thanks @jonatack

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 a pull request may close this issue.

4 participants