varvet / godmin

Admin framework for Rails 5+
http://godmin-sandbox.herokuapp.com
MIT License
486 stars 47 forks source link

[WIP] New filter objects #232

Closed Linuus closed 5 years ago

Linuus commented 7 years ago

This is a PR where I will work on #218. I will first focus on the filters and maybe I'll do the actions, scopes and so on afterwards.

New way of defining filters in services

Instead of using a meta programming as before we use constant FILTERS. It is just an array which lists all filters you want. The filters themselves are now separated from the service as individual objects. There are a few good things about this separation.

  1. It allows filters to be shared between resources easily (even projects if you'd like).
  2. Godmin can ship with some nice default filters which, hopefully, can cover most of the cases.
  3. Easy to test

Example:

module Admin
  class ArticleService
    include Godmin::Resources::ResourceService

    FILTERS = [
      Godmin::Filter::String.new(:my_identifier, column: :title, wildcard: :post),
      Godmin::Filter::Boolean.new(:published, collection: [["Published", 1], ["Unpublished", 0]])
    ]
  end
end

Update Clarification of Godmin::Filter::String.new(:my_identifier, column: :title, wildcard: :post) The first parameter to the filters is the identifier. This is used as the name of the filter, used when finding what filter to apply with what filter params etc. By default this value is also used as the name of the column to filter, so I could have done this: Godmin::Filter::String.new(:title, wildcard: :post) which would use :title as both the identifier and column name.

Creating custom filters

Creating custom filters would be quite easy. You only need to create a class and implement a call method that takes a value and a scope.

Example: (this filter doesn't make much sense but anyway 😸 )

# Always filters out records with matching column and are cats.
module Filter
  class Cat < Godmin::Filter::Base
    def call(value, scope)
      scope.where(column => value).where(cat: true)
    end
  end
end

module Admin
  class ArticleService
    include Godmin::Resources::ResourceService

    FILTERS = [
      Filter::Cat.new(:title)
    ]
  end
end

Views

Each filter defines a to_partial_path which makes it easy to render with rails built in render method. This method renders a partial like filters/_string.html.erb by default (string is the name of the filter). So, the cat filter above would renderfilters/_cat.html.erb.

The views are simple, looking like this:

<%
# Local variables:
#  `form` is the filter form object.
#  `filter` is the filter being rendered
%>

<%= form.string_filter_field(filter) %>

To the *_filter_field methods you can pass options as usual with rails. See: https://github.com/varvet/godmin/blob/04abc4cb7a8f070a1115394e384e8ee049b09d18/lib/godmin/helpers/filters.rb

To think about

What default filters do we want to ship with Godmin?

Let's start with:

View overriding

Out of the box it's possible to override a filter partial by just putting your own partial in the appropriate place. To override the string filter partial for instance: Godmin location: views/godmin/filters/_string.html.erb Your location: views/admin/filters/_string.html.erb ("admin" is your engine name if you have any)

However, this would override it for all filters in your app. Perhaps you want to override for a specific resource. We have two options here, that I can think of.

  1. Use our custom override_partial-thing as we do with other partials at the moment.
  2. Make the user define their own filter for this.

Let's say I want a custom view for a my article string filter.

module Admin
  module Filter
    class ArticleString < Godmin::Filter::String
    end
  end
end

# put view in: view/admin/filters/_article_string.html.erb

Or if you want the view somewhere else, override to_partial_path:

module Admin
  module Filter
    class ArticleString < Godmin::Filter::String
      def to_partial_path
        "article/filters/string"
      end
    end
  end
end

# put view in: view/admin/articles/filters/_string.html.erb

Any thoughts here @jensljungblad ?

jensljungblad commented 7 years ago

Some thoughts:

  1. Clarify in the first example what is meant by my_identifier.

  2. Some nitpicks regarding the filter class syntax. Reason for naming the variable scope? Currently it's resources I believe. I don't feel strongly about it, just curious. Also, order of the arguments, currently it's (resources, value). Reason for changing it? Again, just curious.

  3. Regarding filters to ship. How about String, Select and Multiselect? That maps to what we have now. Or is there something different about belongs to and has many relationships? Also not sure Boolean is useful if we have Select, especially if you still need to pass a collection as in your example.

  4. I think option 2 would be nice. Would be cool if we could get rid of our own partial override thing altogether. I don't think I have ever needed to override a filter anyway, seems like a very rare use case.

jensljungblad commented 7 years ago

I like the wildcard: :post thing! I assume there would be wildcard: :pre and wildcard: :both as well.

Linuus commented 7 years ago
  1. Done

Actually, I can't really come up with a good use case for the identifier to be something other than the column... We talked about this yesterday though :)

One case is if you want to filter two columns and create a custom filter.

Filter::MyFilter.new(:title_or_name, column: ....)

But if you do that you will override the call method anyway and don't have to care about the column value at all. Perhaps it's not needed?

  1. No reason. I just named them something :) I will change it.

  2. Sure, maybe that's enough. I was thinking if we could do something more for the user with specific filters for associations, but perhaps we can add that later if we need to. Also, you don't need to pass anything to the Boolean filter, it has a default, but I just wanted to show that it was possible :)

Actually, the reason I did the Boolean filter was because I couldn't get it to work without some specific conversions, but maybe I just messed something up. See: https://github.com/varvet/godmin/blob/04abc4cb7a8f070a1115394e384e8ee049b09d18/lib/godmin/filter/boolean.rb#L12

I had to explicitly convert "0" and "1" to false/true values before querying. I thought rails handled that somehow. Shouldn't it do that?

  1. Coolio.

And yes, the options for wildcard is :pre, :post, :both with a default of :none at the moment :)

jensljungblad commented 7 years ago

Regarding the boolean conversion, yeah I recognise that as well...

I guess Boolean could inherit from Select then? That's a good question actually. Would it pick up the select filter partial if there is no boolean partial? I guess not, but then I guess the Boolean filter could implement to_partial_path and explicitly render the select partial.

Linuus commented 7 years ago

Yes, Boolean can inherit from select, it can just do the conversion and call super.

Linuus commented 7 years ago

OK, so now I've implemented 4 simple filters.

First options is always an identifier. The :column option is to set a specific column. Defaults to identifier. Not sure if we need to keep this option actually. I will not use it in all examples below.

String filter

Example usage:

Godmin::Filter::String.new(:title, column: :title, wildcard: :post)

Options:

Select filter

Example usage:

Godmin::Filter::Select.new(:author_id, collection: Author.all, option_text: :to_s, option_value: :id)

Options:

MultiSelect filter

This inherits from the Select filter. It just renders a different partial which in turn renders a multi select instead of a regular select box.

Example usage:

Godmin::Filter::MultiSelect.new(:status, collection: Status.all, option_text: :to_s, option_value: :id)

Options:

Boolean filter

This is a variant of a select filter. Essentially it just allows the user to specify the true and false option texts, converts the value coming from the form (1 = true, 0 = false) and delegates to the select filter.

Example usage:

Godmin::Filter::Boolean.new(:published, true_label: "Published", false_label: "Unpublished")

Options:

Now that I write this, maybe I should rename true_label to true_text to be consistent with option_text?

Thoughts

Can we make the select filters above support many-to-many associations or do we just let the user implement those by themselves?

I mean this case:

We have Article and Tag. An article can have many tags, a tag may have many articles. I guess we would like to make a query like

def call(resources, value)
  resources.includes(:tags).where(tags: { name: value })
end

So, the filter needs to know the association name, as well as the column name.

What do you think @jensljungblad ?