STI: add `add_available_filters` method in order to DRY

[vmeyet]

following conversation at https://github.com/jhund/filterrific/commit/00ed11b8d613d930a3e2e2020c616390c143438d

When using single-table-inheritance (STI),

• one might expect that the GirlClass have the parents available_filters when not overriding them

class Daddy < ActiveRecord::Base
  filterrific(available_filters: [:one, :two])
end

class Girl < Daddy
end
# we expect Girl to have [:one, :two] as available_filters

• It would be really helpful, to have a method to "add" filters instead of overriding the filter and redefining 80% of them

class Daddy < ActiveRecord::Base
  filterrific(available_filters: [:one, :two])
end

class Girl1 < Daddy
  filterrific(add_available_filters: [:three, :four])
end
# we expect Girl1 to have [:one, :two, :three, :four] as available_filters

class Girl2 < Daddy
  filterrific(available_filters: [:three, :four])
end
# we expect Girl2 to only have [:three, :four] as available_filters

Sadly i think given the implementation it might be not trivial, given:

• http://www.railstips.org/blog/archives/2006/11/18/class-and-instance-variables-in-ruby/ (search for ClassLevelInheritableAttributes)
• https://www.ruby-forum.com/topic/197051

we could do (not so great but I've done it quickly):

module Filterrific
  module ActiveRecordExtension
    ...
    def filterrific(opts)
      class << self
        attr_accessor :filterrific_available_filters
        attr_accessor :filterrific_default_filter_params

        def filterrific_available_filters
          @filterrific_available_filters || (superclass.filterrific_available_filters if superclass.respond_to?(:filterrific_available_filters))
        end

        def filterrific_default_filter_params
          @filterrific_default_filter_params || (superclass.filterrific_default_filter_params if superclass.respond_to?(:filterrific_default_filter_params))
        end
      end
      self.filterrific_available_filters = []
      ...
    end
    ...
  end
 ...
end

What do you think of it of the feature?

[hachpai]

In the specific case whose mine, it seems coherent and could avoid me a dirty hack. For the moment, in a way to keep being DRY, I do:

class Daddy < ActiveRecord::Base
  def self.filterrific_default_filter_params
    { sorted_by: 'name',with_status:nil, production:0, deploiement:0, deleted:0}
  end
  def self.filterrific_available_filters
    [:production,:deploiement,:deleted,:sorted_by,:with_status]
  end
end

class Girl < Daddy
filterrific(
  default_filter_params: filterrific_default_filter_params.merge({girl_filter1:value}) ,
  available_filters: filterrific_available_filters + [: girl_filter1]
  )
end

[jhund]

I agree with the expected behavior that subclasses should be able to build on available_filters already defined in the superclass and not having to re-declare them. I'm just not sure about the implementation yet.

@vmeyet your example code likely won't work: You use @filterrific_available_filters || (superclass... to use settings on instance, or fall back to parent class. However in the same method you initialize self.filterrific_available_filters = [], so it is not nil and will always be used in the || based fallback in def filterrific_available_filters. We can fix this easily.

I'd like to explore the idea of adding an :add_available_filters key that will add to the parent class' available_filters. My question: Is adding to the parent class available_filters the only valid use case? Are there situations where you would want to subtract or intersect? If so, then we'd have to think about a different way of pulling this off.

Related to this, I have been considering developing a block form and DSL to configure filterrific:

filterrific do
  filter :with_country, belongs_to: :country
  sort_by :name, default_direction: :asc
end

Doing configuration this way may be easier to implement inheritable filterrific settings.

ActiveSupport's class_attribute may do the trick here.

[vmeyet]

Good catch for the issue in the code.

I like the DSL idea that might make it more extensible. it might simplify settings with inheritance depending on implementation.

For the new methods the two methods that makes the more sense to me would be add_available_filters and remove_available_filters. Also it might be implemented using a Set instead of an Array. And we could use the set operator |, &, ^, - and +. (even though array also has set operators)

Something like

filterrific do
  # available_filters is parent's available_filters #<Set: {:a, :b}>
  self.available_filters += [:a, :c]
  # available_filters is now #<Set: {:a, :b, :c}>
  self.available_filters &= [:c, :d]
  # available_filters is now #<Set: {:c}>
end

just a thought. But in short, IMO, the inheritance of filters + a way to add, remove filters from the inherited filters is what is really needed.

[jhund]

@vmeyet I'd like to stick with the Open Closed principle which in a nutshell says that a superclass should be replaceable with any of its subclasses. So I don't think we should support removal of available filters as that would break the above assumption, however adding available filters doesn't.

[hachpai]

I agree @vmeyet, that was what I expected before reporting the issue. Then, calling available_filters should concatenate the given list to the existing one. Furthermore, we should make filters available through the class hierarchy for children.

[hachpai]

To keep the actual implementation, we could make something like that:

  def filterrific(opts)
      class << self
        attr_accessor :filterrific_available_filters
        attr_accessor :filterrific_default_filter_params
      end
      self.filterrific_available_filters ||= []

      recursively_append_available_filters

      # define_sorted_by_scope(opts['sorted_by'])  if opts['sorted_by']
      # define_search_query_scope(opts['search_query'])  if opts['search_query']

      assign_filterrific_available_filters(opts)
      validate_filterrific_available_filters
      assign_filterrific_default_filter_params(opts)
      validate_filterrific_default_filter_params

    end

    #this method check recursively the class hierarchy for finding already defined filters, until it encounters ActiveRecord::Base 
    def recursively_append_available_filters(actual_class=self)
      parent=actual_class.ancestors[1]
      if parent != ActiveRecord::Base
        self.filterrific_available_filters += parent.filterrific_available_filters || [] if parent.respond_to?(:filterrific_available_filters)
        recursively_append_available_filters(actual_class.ancestors[1])
      else
      []
      end
    end

[hachpai]

For the moment I've implemented a method in my superclass to cumulate the filters in the subclasses.

def self.ancestors_available_filters(actual_class=self)
    parent=actual_class.superclass
    if parent != ActiveRecord::Base
      if parent.respond_to?(:filterrific_available_filters)
        parent.filterrific_available_filters + ancestors_available_filters(parent)
      else
        ancestors_available_filters(parent)
      end
    else
      []
    end
  end

We could integrate it easily in the lib. My previous version with ancestors doesn't work and costs a lot.

[vmeyet]

@jhund, that make sense to not break the OpenClose principle. That would mean not having ways to subtract or intersect and we only and always want to add available filters.

However I've worked on project using filterrific where the OC principle was broken due to architecture choices. And not having ways to override parents filter would mean, for the dev, to monkey patch the lib to workaround this.

Does the following code make sense (saving the OC principle)?

module Filterrific
  module ActiveRecordExtension
    def filterrific(opts)
      class << self
        attr_accessor :filterrific_available_filters
        attr_accessor :filterrific_default_filter_params
      end

      # We no longer need to set filterrific_available_filters here
      # self.filterrific_available_filters = []
      ...
    end

  end

  protected

  def assign_filterrific_available_filters(opts)
    # default on parent available_filters if not defined yet
    self.filterrific_available_filters ||= superclass.try(:filterrific_available_filters) || []
    # union with available_filters if given
    self.filterrific_available_filters |= opts['available_filters'] if opts['available_filters']
    # no need for #uniq thanks to the union operator
    self.filterrific_available_filters.map!(&:to_s).sort!
  end

  def assign_filterrific_default_filter_params(opts)
    # default on parent available_filters if not defined yet
    self.filterrific_default_filter_params ||= superclass.try(:filterrific_default_filter_params) || {}

    # merge with default_filter_params if given
    self.filterrific_default_filter_params.merge(
      default_filter_params: opts['default_filter_params']
    ) if opts['default_filter_params']

    self.filterrific_default_filter_params.stringify_keys!
  end
end

I'm assuming only one parent, since we're talking about ActiveRecord and STI, we cannot have filterrificable mixin, so that's ok.

@hachpai your method to accumulate available_filters work. but if each class has all its ancestors filters, there is no need for recursion. Moreover you might want to add a #uniq when adding the filter or (better) use the union operator (|)

If we want the whole hierarchy we can still do something along the line of: ancestors.select { |a| a.kind_of? ActiveRecord::Base }.map { |a| a.respond_to?(method) ? method : nil }.compact.uniq.sort

[hachpai]

Agree with you for uniqueness, but recursion ensure that even if an intermediate class doesn't invoke filterrific (and then doesn't respond to the filterrific_available_filters method), the accumulation will pass over it. Anyway there's still this problem, without invoking the filterrific method, the class doesn't heritate any filter.