Rethink resource service DSLs

[jensljungblad]

Perhaps reduce DSLs further. Perhaps there are less magic ways to define attributes, scopes, filters and actions. Look at how https://github.com/thoughtbot/administrate use constants, for instance.

Also, reusability for scopes, filters etc would be good.

Defining regular attributes could look like this if we skip the macros:

class ArticleService 
  LIST_PAGE_ATTRIBUTES = [:id, :title]
  SHOW_PAGE_ATTRIBUTES = [:id, :title, :body]
  FORM_PAGE_ATTRIBUTES = [:title, :body]
end

When it comes to scopes, filters and actions things get more interesting, because they all take options. It would also be nice if you could specify the filter function instead of it having to be named filter_something. Same goes for things like the collection argument. Currently it needs to be a prop, but this could be more flexible. The most verbose approach would be:

class ArticleService 
  FILTERS = {
    title: { type: :string, filter: :filter_title },
    author: { type: :select, filter: :filter_author, collection: :filter_author_collection
  }

  def filter_title; end
  def filter_author; end
  def filter_author_collection; end
end

Another alternative would to replace filters with classes:

class ArticleService 
  FILTERS = [
    Filter::Title, 
    Filter::Author
  ]

  class Filter::Title
    def filter(articles); end
  end

  class Filter::Author
    def filter(articles); end
    def collection; end
  end
end

This would make it possible to extract and reuse filters. Perhaps we could even ship some:

class ArticleService 
  FILTERS = [
    Godmin::Filter::Wildcard.new(column: :title)
  ]
end

Some ideas that could be investigated...

[Linuus]

I like the one where we use classes for each filter. I'll experiment a bit today and see what I can come up with.

Perhaps it's hard to just keep FILTERS as an array, we may need to do something like:

FILTERS = {
  title: Filter::Title,
  author: Filter::Author
}

The reason for this is when we apply the filters we need to know what filters to run. Well, we don't need to know that, we can just call all filters, always, but then each filter needs to decide whether to actually do something or not, which may be a bit tedious.

class Filter::Title
  def call(scope, value, field)
    return scope unless field == "title"
    scope.where(title: value)
  end
end

Another approach would be to pass the field name when initializing the filter and we can provide a method by default that checks this explicitly. Something like this:

# In godmin
class Godmin::Filter::Base
  def initialize(field)
    @field = field
  end

  def filter_for?(field)
    field == @field
  end
end

# In my app
def Filter::Title < Godmin::Filter::Base
  def call(scope, value)
    scope.where(title: value)
  end
end

# In service
FILTERS = [
  Filter::Title.new(:title),
  Filter::Author.new(:author)
]

The #filter_for? method would be used by godmin to decide whether we should apply the filter with some params or not.

I think the last version is better, because if we want to ship some filters with Godmin we need to know the field anyway, as in your example.

[jensljungblad]

Could the Filter classes implement a to_param or something? I guess for a filter such as Filter::Title we just want it to map to title, but yeah it's trickier if we want to ship pre made filters. So Godmin::Filter::Text.new(column: :title) would also need to map to title.

I don't think this is terrible though:

FILTERS = {
  title: Filter::Title,
  author: Filter::Author
}

[Linuus]

This looks a bit inconsistent to me:

FILTERS = {
  title: Godmin::Filter::Text.new(field: :title),
  author: Filter::Author,
}

Also, this means we'd mix instances and class constants in the list which is a bit annoying... We would need to check what to instantiate and not.

Sure, we could do

FILTERS = {
  title: Godmin::Filter::Text,
  author: Filter::Author,
}

And then pass :title to the filter when instantiating it (and :author to Author) in Godmin.

However, if we want to be able to pass options to a filter it gets weirder... In Administrate you pass .with_options(...) which returns a Deferred object. This looks unnecessary complicated to me.

[jensljungblad]

What types of options would you want to pass?

Also, would it make sense to think about scopes and actions as well? Do we want the interface to be the same for those? Feels like it would be nice if they all worked pretty much the same. And what if we add attribute types like administrate has. If they need a with_options anyway, perhaps it would be consistent if filters use the same syntax?

But I agree. Seems a bit complicated to mix class constants and instances.

[Linuus]

I don't know :) There are probably a lot of cases where you'd want to pass options. Especially if you create a more general filter.

Example:

FILTERS = [
  Godmin::Filter::Wildcard.new(field: :title, wildcard: :pre),
  Filter::Author.new(:author)
]

class Godmin::Filter::Wildcard
  def initialize(field: field, wildcard: wildcard)
    @field = field
    @wildcard = wildcard
  end

  def call(scope, value)
    scope.where("#{@field} LIKE ?", wildcard_query(value))
  end

  private

  def wildcard_query(value)
    if @wildcard == :pre
      "%#{value}"
    elsif @wildcard == :post
      "#{value}%"
    else
      "%#{value}%"
    end
  end
end

Also, I guess these objects would be responsible for the options for the actual html fields, right? So we'd need to be able to pass those as well (include_blank, option_text etc).

[jensljungblad]

Our current thought is that the same syntax could be used for scopes and filters as well. Something like:

class ArticleDashboard
  SCOPES = [
    Godmin::Scope.new(:unpublished),
    Godmin::Scope.new(:published)
  ]

  FILTERS = [
    Godmin::Filter::Text.new(:title),
    Godmin::Filter::Select.new(:author, collection: Author.all)
  ]

  ACTIONS = [
    Godmin::Action.new(:destroy, confirm: true)
  ]
end

The first option would double as identifier and the default value of scope, column and action respectively. Godmin::Scope would call the provided scope on the resource relation, while Godmin::Action would call the provided action for each resource in the relation.

[benjamin-thomas]

I'd like to give my feedback if you're interested.

I'm all for less magic, but I found your use of metaprogramming very easy to understand (and I hardly ever use metaprogramming in my own code).

From my point of view, filters and scopes represent the right use case for metaprogramming and reduce boiler plate.

I personally found the administrate gem way too stiff and found your gem much nicer and better structured, when I looked at it a year ago.

So I wouldn't use them as inspiration IMHO.

Godmin's code is flexible enough that I implemented common filters easily, this way (the same approach works for scopes too).

module Admin
  module ServicesCommon
    module CommonFilters

      def filter_map
        filters = {
          author: {
            as: :select,
            collection: -> { Author.all },
            option_text: :to_s,
            option_value: :id,
          },
          title: {
            as: :string,
          },
          posted_on: {
            as: :string,
          },
        }

        filters.merge(super)
      end

      def filter_author(rel, id)
      end

      def filter_title(rel, id)
      end

      def filter_posted_on(rel, id)
      end

    end
  end
end

module Admin
  class BlogPostService
    include Admin::ServicesCommon::CommonFilters
  end
end

So I'm not sure this new syntax would be a major improvement. I personally found the macro syntax to be simple and effective.

[jensljungblad]

@benjamin-thomas Thanks! Yeah we definitely want feedback on these things. We're still basically experimenting and exploring different ideas.

I agree that our usage of macro methods is not a big problem. Although there is something to be said about the fact that it complicates the Godmin code, and makes it more difficult to understand for people who want to look at it.

As for administrate, I like parts of it. I think there are things we do better but also think they solved some parts better than we did. I like the way they've set up column customization.

Your way of doing shared filters work, but I would argue putting them in classes is preferable since it makes them easier to test. Testing one of your shared filters would require you to include that module in a service class first. Also, it feels like we're onto something with shipping some default filters. What are your opinions on that? Basically the fact that a lot of filters are just the same query over and over.

Oh, and also, if you don't want to override filter_map you could do:

module CommonFilters
  include ActiveSupport::Concern

  included do
    filter :title
    filter :author
  end

  ...
end

[benjamin-thomas]

Hello @jensljungblad

Sorry for the (really) late feedback.

Thanks for your suggesting including the Concern module, much better!

As far as testing goes, I think an integration test is the way to go, but I have to admit I'm not a very diligent tester (in the TDD sense).

Testing a filter would feel like testing a controller or route, I'm not sure much is gained by testing what I feel is more an implementation detail than anything.

Anyways what I like about your framework is that you can plug into it and override things with standard ruby techniques.

My 2 cents of course.

[jensljungblad]

No worries!

Yeah we definitely want to keep Godmin simple and easy to override using standard Ruby and Rails practices, that's why we built it in the first place. We'll keep discussing these things here, once we get back to it.