-
-
Notifications
You must be signed in to change notification settings - Fork 17
Support all ransack predicates #25
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
base: main
Are you sure you want to change the base?
Conversation
6b049a9 to
7d9478a
Compare
|
Hey @goosys Thank you very much for your contribution here.
Since it passed quite some time, I'll evaluate to create specific PRs based on this one 👍 |
blocknotes
left a comment
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.
As anticipated, I prefer to create more scoped PRs, to avoid breaking changes for now (but soon I would like also to consider breaking changes too), to avoid accessing private instance variables.
| <% association = model.reflections[field.to_s] %> | ||
| <% if association %> | ||
| <% field_key = model.ransackable_scopes.include?(field) ? field : "#{field}_id_eq" %> | ||
| <% desc = association.klass.method_defined?(:admin_label) ? :admin_label : :to_s %> |
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.
This is a breaking change, I prefer to have a specific PR for it. And it can be merged only with the next major version.
| class << self | ||
| def ransack?(model, params = {}, options = {}) | ||
| ransack = model.ransack(params, **options) | ||
| ransack.instance_variable_get(:@scope_args).present? || ransack.base.c.present? |
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.
The general idea is good but I prefer to avoid accessing internal Ransack details (like @scope_args instance var) because they do not belong to the public API of the gem.
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.
I believe this is a feature currently missing in Ransack, and I'm hoping it will eventually be implemented there — though it hasn't happened yet (there are some related PRs, though).
It might be faster to contribute directly to Ransack, but if we want to avoid that for now, how about something like the following as a workaround?
def ransack?(model, params, **options)
begin
model.ransack!(params, **options)
true
rescue StandardError
false
end
endThere 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.
Mmm... perhaps you could try something like:
def ransack?(model, field)
ctx = Ransack::Context.for(model)
ctx.ransackable_attribute?(field, model) ||
ctx.ransackable_association?(field, model) ||
ctx.ransackable_scope?(field, model)
endThere 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.
No, that only checks the model definition, but it doesn’t reflect which field names can actually be used in Ransack.
One of the goals of this PR is to support Ransack-style search conditions like name_or_description_or_email_or_articles_title_cont.
To do that, we need to determine whether a given field is usable after it has gone through all of Ransack’s initialization processes, including things like groupings.
|
I also think the general idea is good, though I believe some of the details still need further discussion. Administrate still supports Rails 6.0, but since this gem has already dropped it, we could consider using |
Those fields' view does their job for me, but any improvement could be nice (if proposed in a specific PR). Do you mean having something similar to view components? |
|
The ViewComponent library is separate from Rails itself — Rails only provides support for Here’s a simplified example. A lot is omitted, but this is the general idea: # lib/administrate_ransack/view/field/base.rb
module AdministrateRansack
module View
module Field
class Base
def initialize(); end
def render_in(view_context); end
def render?; end
private
def ransack?; end
# lib/administrate_ransack/view/field/has_many.rb
module AdministrateRansack
module View
module Field
class HasMany < Base
def render_in(view_context)
@view_context = view_context
return unless render?
view_context.render template_name, local_assigns
end
def render?
super && model.reflections[field.to_s]
end
private
def template_name
"components/field_has_many"
end
def local_assigns
{
form: @form,
field_key: field_key,
resource_field: resource_field,
collection: collection
}
end
...<%# app/views/administrate_ransack/_filters.html.erb %>
...
<% component = AdministrateRansack::FILTERS[input_type] || 'field_other' %>
<% view_component = <<Some logics to find view class like `AdministrateRansack::View::Field::HasMany`>> %>
<div class="filter filter-...">
<%= render view_component.new(
form: f, model: model, field: field, label: label, type: type, options: options[field]
) %><%# app/views/administrate_ransack/components/_field_has_many.html.erb %>
<%= form.label(label || field_key, class: 'filter-label') %>
<%= form.select(field_key, collection, {}, multiple: true) %>What do you think? Do you think it's worth discussing further? |
Yep, the approach is similar to view components. I'll evaluate better your proposal, but to be honest I'm thinking to go in a different direction at the moment. In this way I'll be able to have simpler views and more freedom on the filters customization: RANSACK_SEARCH = {
title: {
type: :string,
param: :title_cont
},
published: :boolean,
position: :number,
by_category: {
type: :string,
scope: true
},
dt: :date,
}I do not exclude the possibility to use renderable / view components / Phlex in the future. |
I made the following changes to enable the use of Ransack's extensive predicates expressions, in addition to ransackable_scope.
Please review and let me know if any further changes are needed.
Changes
AdministrateRansack.ransack?helper to check if a field name with predicates is valid.Breaking changes
admin_scopeoptionadmin_scopeis no longer necessary as updated to useField.associated_resourceto get the collection.Field::BelongsTo.with_options(scope: ->{ Post.published })Field::HasManydoes not have ascopeoption, it cannot be done in the same way, but it can be achieved using theField::ScopedHasManyplugin.admin_labeloptiondisplay_resourceas the label to match the title or heading of each admin pages.