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

Ransackable scopes #57

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Ransackable scopes #57

wants to merge 3 commits into from

Conversation

stas
Copy link
Owner

@stas stas commented Aug 4, 2021

What is the current behavior?

ActiveRecord scopes can't be used in filters. This supersedes the #54

What is the new behavior?

You can use scopes for filtering as well, to enable scopes, use the option
flags:

options = { allowed_scopes: User.ransackable_scopes }
jsonapi_filter(User.all, [:created_before], options) do |filtered|
  render jsonapi: result.group('id').to_a
end

Assuming your model User has the following scope defined:

class User < ActiveRecord::Base
  scope :created_before, ->(date) { where('created_at < ?', date) }
  def self.ransackable_scopes(_auth_object = nil)
    [:created_before]
  end
end

This allows you to run queries like:

$ curl -X GET /api/resources?filter[created_before]='2021-01-29'

Checklist

Please make sure the following requirements are complete:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes /
    features)
  • All automated checks pass (CI/CD)

fluxsaas and others added 3 commits June 24, 2021 17:40
Scopes are special in that they don't have a predicate. This removes the
check for the predicate, and thus makes scopes as filtering options work
with JSONAPI.rb.
@stas stas mentioned this pull request Aug 4, 2021
2 tasks
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking this over! I like your solution, but I have a question regarding requiring the scope's name to be an allowed_field.

Also, thank you for maintaining these gems, they really help us!

allow = predicates.any? || (
predicates.none? &&
scopes.include?(requested_field) &&
allowed_fields.include?(requested_field)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, given that we handle scopes separately already, I don't really see why we should add the scopes to the allowed_fields array as well. Most scopes will have names that don't directly match the name of a field, so I think this would be a little confusing.

However, I might be missing something as well!

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most scopes will have names that don't directly match the name of a field, so I think this would be a little confusing.

Yup, I was thinking about it too and the only reason I'd leave it explicit, is because there might be scopes that can conflict with the field name. My concern is that we might accidentally enable a scope as filterable without the user being aware of it.

@fluxsaas would be nice to get your opinion on this too. 🙇

@stas stas added this to the v1.8 milestone Sep 1, 2021
@curtis741
Copy link

Any updates on this idea?

Was just trying to do this today.

@tvdeyen
Copy link

tvdeyen commented Feb 15, 2023

@stas we really would like to have this. Is there anything we can do to make it happen? Happy to contribute. I actually know @mamhoff personally and we work on the same app :)

@barberj
Copy link

barberj commented Feb 15, 2023

Also just ran into this

@BenAkroyd
Copy link

Dealing with this now, nice solution!

@stas
Copy link
Owner Author

stas commented Feb 16, 2023

I'll have to make it backwards compatible, but definitely could have it released by next week. Ty ty for bringing it all up!

@fastfedora
Copy link

Checking in on this status as well. Any updates?

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.

8 participants