-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
Feature flexible resource filters #2091
Feature flexible resource filters #2091
Conversation
@robinboening rebasing with latest |
9991158
to
6188a1b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. What a great change and improvement 👍🏻
I really like that and only have some naming nits and naive questions.
Regarding the remaining failing specs I think we can solve them by wrapping the expectations in a within
block to narrow down the potential nodes.
end | ||
|
||
def sanitize_filter_params! | ||
search_filter_params[:filter].reject! do |_,v| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true
*) TIP: in order to not ever need to think of those formatting things again: https://github.com/ruby-formatter/rufo
app/models/alchemy/picture.rb
Outdated
[ | ||
{ | ||
name: :filter, | ||
values: %w(recent last_upload without_tag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about all the Rubocop comments. The easiest way to get rid of them is to install rufo
, and autoformat the file changes with it.
@robinboening btw. extremely good work on the PR description 👏🏻 |
FYI, I haven't had the time to revisit this, but I'll most likely find some time in the next days or next week. |
3494c8e
to
08cdb0e
Compare
this commit introduces a breaking change for apps using the class method `alchemy_resource_filters` in their models used in custom modules. The change introduces a new data structure for defining filters. It allows to set up multiple filters in two different ways: * where the filter name is assumed to be a scope on the model and the values are passed as the argument * where the filter values are assumed to be scopes on the model (basically as it was until now) Example: ``` class Mymodel < ApplicationRecord scope :by_language, ->(lang) { where(language: lang) } scope :published, -> { where(published: true) } scope :drafts, -> { not.published } def self.alchemy_resource_filters [ { name: :by_language, values: distinct.pluck(:language) }, { name: :status, values: %w[published drafts] } ] end end ``` Both, the name and the values can be translated. Since the filter values are passed into the `options_for_select` helper, they can be provided in two ways: array of strings/symbols, or as an array of arrays. In the first case Alchemy will lookup a translation in `alchemy.filters.<name_of_filter>.values` whereas in the second case the provided values will just be passed to into `options_for_select` directly. There are two security measures in place against filter param tempering: 1. Arriving filter values must match the values defined in the model. If the value doesn't exist, the parameter gets rejected and the value won't be send to the model. 2. Rails' strong parameters is used for the case where someone tempered with the parameter names. If a name and doesn't match one of the defined filter names in the model it won't get the permission to pass.
3298d99
to
69c2552
Compare
@tvdeyen I finally got back to this PR and covered all the comments with either changes or a comment, except for one: The headline change when auto filtering after a picture upload. I'd like you to check the current state first so I can start fresh on the last item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow! This is really impressive work and is an improvement to the current resourceful admin views.
I think with the recent renaming of the picture filters to misc
(I agree that this is a good name btw.) we need to update some views.
Thanks again! 🚀
69c2552
to
20aad20
Compare
I just thought can we maybe find a way to still support the old resource filter usage by checking the contents of the filters array and pass it to miscellaneous filters if they are not a Hash, then print out a deprecation warning? |
Sounds feasible. I'll check how that would look like. |
1c5607e
to
7c163a1
Compare
@tvdeyen I pushed another commit for supporting the legacy filter structure. Now off to bed... 😴 |
7c163a1
to
ace7ac6
Compare
This commit is supposed to be reverted after Alchemy v6.0 is released.
ace7ac6
to
4af2611
Compare
@tvdeyen I requested another review, hopefully the last one now 🤞 |
@robinboening left one comment. If this is not easy to achieve now, we can add that later in another PR. This is already is major improvement. Thank you for all this great work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive work
What is this pull request for?
This PR enhances the resource filtering feature. Models can now define multiple and more flexible filters in the
alchemy_resource_filters
class method. It closes #2085Disclaimer
This PR grew to a much bigger size as I initially imagined. This is mainly caused by the number of internal models having individually implemented filters as an addition to the also used
alchemy_resource_filters
feature which wasn't sufficient enough to cover their needs.I consider this PR almost done, but not 100%. Hence the WiP in the title.
What's missing: It should add some tests covering some cases with multiple filters. Though, the changes made many tests fail which I was able to fix already, so the basic feature is actually still covered quite okay. Anyhow, even after fixing many tests there are still two select2 feature tests I haven't been able to fix yet. Any help fixing those two is much appreciated.
Notable changes
The changes allow to set up multiple filters in a unified but new data structure. Filters are still utilising model scopes, but now they not only support normal scopes but scopes with an argument as well.
Introducing a new data structure also means introducing a breaking change. Existing Alchemy apps having
alchemy_resource_filters
defined will need to adopt the new structure.Scope with argument
The filter
name
is assumed to be a scope on the model and thevalues
are passed as the argument. Thevalues
will probably almost always dynamically determined (most likely database records).Scope without argument
The filter
values
are assumed to be scopes on the model (basically as before) and are usually hard-coded as the scope names are known and don't change.Example
In this example you'll see two filters. The
by_language
filter fetches its values dynamically from the database and the according scope expects an argument. Thestatus
filter has hard-coded values and those are assumed to be existing scopes without arguments.Both, the name and the values can be translated. Since filter values are passed into
options_for_select
they can be provided in two formats:[:value1, :value2, ...]
[["Translated value1", :value1], ["Translated value2", :value2], ...]
In the first case Alchemy will lookup a translation in
alchemy.filters.<model_name>.<name_of_filter>.values
whereas in the second case the provided values will just be passed to intooptions_for_select
directly.Security
There are two security measures in place against filter param tempering:
Misc
While adopting the new filter feature by the internal models I was forced to refactor a few areas.
Picture.search_by(params, ...)
,Picture.next(params)
andPicture.previous(params)
methods into the controller level. This eliminates the need to pass params into the model and allows to reuse existing code.Screenshots
From the screenshots you can see it plays well together with other filters and search.
Checklist