named_filter_issues.rb

############### Named filter issues.

#
# 1) The table aliases for explicit joins are hacky. Obviously this is an easy fix, but we should 
# talk about the best alternative. In this example, the resulting alias should not be "posts_blogs".
# 
# => Agreed, although I can't think of anything really clever at the moment...

      Post.filter do
        join(Blog, :left, :posts_blogs) do
          on(:blog_id => :id)
          with(:name, 'Test Name')
        end
      end.inspect

      Post.last_find[:joins].should == %q(LEFT JOIN "blogs" AS posts_blogs ON "posts".blog_id = posts_blogs.id)

#
# 2) Join type support is spotty. Right now we have inner, left, left_outer, and outer. We just 
# need to make sure that we support all of the reasonable types.
#
# => To my knowledge, the only types of joins regularly used in SQL queries are INNER, LEFT OUTER,
#    and RIGHT OUTER. RIGHT and LEFT are shorthand for the outer options. So we might actually
#    want to support fewer join types, not more!

#
# 3) Custom SQL isn't supported. Worth discussing the correct API for this, given the large number
# of ways in which you might want to use it.
#
# => Yeah, agreed. We might want to hold off on that until we have more experience using it, and
#    possibly feedback from other users.

#
# 4) Accessing local variables can be clumsy with name conflicts. Since the methods called inside
# of filters and named_filters are scoped to the filter DSL, there can be name conflicts when trying
# to use local variables with the same name. I think think this is just a fact of life, but worth
# thinking about. Take this example, from a content block presenter, where we have to alias the 'limited'
# value. Unfortunately, this is a general case for these presenters.
#
# => That's kind of a fact of life with using instance_eval'd blocks, and I'd say the benefits outweigh the
#    costs. By the way, it is possible to do `limit=limit` outside the filter, which is strange looking but
#    sets a new local variable called 'limit' to the return value of the 'limit' method.
#    One other option would be to allow two contexts for evaluation - either the existing instance_eval, or,
#    if the block takes an argument (block.arity > 0) pass the DSL as the argument instead. I'm doing this
#    in Sunspot.
#    And finally, I'd say the below is a strong motivator for inline named filters!

class FeaturedListingsContentBlockPresenter < ContentBlockPresenter
  def items
    @items ||= begin
      scope = current_site.listings.featured.published
      scope = scope.for_category(category) if category
      scope = scope_for_deduplication(scope, Listing)
      limited = limit
      scope.filter do
        order({ :feature => :published_at }, :desc)
        limit(limited)
      end
    end
  end
end

#
# 5) The explicit join API can be clunky. Consider this case... I found the combination of hashes and
# argument lists a bit confusing when creating them. It obviously makes sense but isn't necessarily
# intuitive. It's also not clear which column name should go on the left of the hash declaration
# and which one on the right, and the fact that the argument list form switches them isn't easy on the
# eyes (hey, I still need to read that wikipedia article on joins, obviously).
#
# => Agreed, but I can't think of a really good way to improve it...

      named_filter(:for_category) do |category|
        join(ClassificationsObject, :inner) do
          on(:id => :classifiable_id)
          on(:classifiable_type, filter_class.name)
          on(:classification_type, 'Category')
          on(:deleted_at, nil)
          join(Category, :inner) do
            on(:classification_id => :id)
            with(:lft).between(category.lft, category.rgt)
          end
        end
      end

#
# 6) Also in the code example above, I added the filter_class method so that we can get access to the
# class being filtered, but it's not very intuitive to use (or that you should have to use it). Once nice
# thing about named scopes is that because they are evaled in context of the filtering class they don't
# have to do this. I might look into delegating all methods that aren't in the DSL API to the underlying
# model class to fix this.
#
# => Not a big fan of delegating methods in that way, actually - seems like a mixed namespace metaphor.
#    Probably giving access to the filter_class is as good a solution as any.

#
# 7) We need to make sure we're handling all the wacky cases for AR associations. There are a lot of
# ways to customize the associations, and we are probably only handling a small percentage of them.
#
# => Word. This seems much more of a hassle to write specs/mocks for than to actually code up, but
#    anyway it must be done.

#
# 8) The pending test about using named filters within named filters is still pending. I can take
# another look at implementing that now that I have a better grip on things than I did last time I
# looked.
#
# => Yes, this would be a killer feature. Maybe I'll look into implementing that - it would be a
#    good way for me to get familiarized with the code in its current form as well...