Skip to content

Verify and apply filters using a callback name#613

Merged
lgebhardt merged 1 commit intoJSONAPI-Resources:masterfrom
kenzai:filter-apply-verify-symbol
Feb 17, 2016
Merged

Verify and apply filters using a callback name#613
lgebhardt merged 1 commit intoJSONAPI-Resources:masterfrom
kenzai:filter-apply-verify-symbol

Conversation

@davidstosik
Copy link
Copy Markdown
Contributor

What?

At the moment, the only way to customise the process of verifying or applying a filter is to write a callable inline, in the filter declaration.
My suggestion is to accept, as apply: and verify: parameter values, either a callable, or a method name (string or symbol).

How?

Using the same apply: option as previously, we could also pass a callback name, that would need to be defined on the resource class.

filter :public, apply: :apply_public_filter

# (...)

def self.apply_public_filter(records, value, _options)
  records.where(public: (value == "public"))
end

Same goes with verify:

filter :user_id, verify: :verify_filter_user_id

# (...)

def self.verify_filter_user_id(values, _context)
  current_user = context ? context[:current_user] : nil
  values.map do |id|
    id == "me" ? current_user.try(:id) : id
  end
end

Why?

Although the existing solution is a progress from the previous suggestion to override the apply_filters and verify_filters methods, it does not allow code factorisation, and sometimes makes the code harder to read. I personally ended up with a lot of, in my opinion disgraceful declarations like the one below:

filter :custom, apply: ->(*args) { my_custom_filter_callback(*args) }

The solution above makes it easier to factorise, and follows a pattern that I think is quite common.

lgebhardt added a commit that referenced this pull request Feb 17, 2016
Verify and apply filters using a callback name
@lgebhardt lgebhardt merged commit 9caf694 into JSONAPI-Resources:master Feb 17, 2016
@lgebhardt
Copy link
Copy Markdown
Contributor

@davidstosik Thanks, that's a nice addition. And thanks for the excellent write up on the problem and solution! I think this should be the model for how to submit a PR.

@davidstosik
Copy link
Copy Markdown
Contributor Author

Thanks, but I merely copied what looked like a nice template from the original issue thread #526. :)
Thanks for the merge, this is going to help!

@lgebhardt
Copy link
Copy Markdown
Contributor

I thought it look vaguely familiar. Glad you used it, and thanks to @iamvery for using it in #526

@iamvery
Copy link
Copy Markdown
Contributor

iamvery commented Feb 17, 2016

❤️

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.

3 participants