UserSearch refactor

user_search.rb

class UserSearch
  def self.search(term, topic_id = nil)
    scope = User.scoped 

    if topic_id
      scope = scope.joins("
        LEFT JOIN (
          SELECT DISTINCT(posts.user_id)
          FROM posts
          WHERE topic_id = #{topic_id.to_i}
        ) s ON s.user_id = users.id
      ")

      scope = scope.order('s.user_id IS NULL')
    end

    scope = scope.order('
      last_seen_at IS NULL ASC,
      last_seen_at DESC,
      username ASC
    ')

    scope = scope.where("
      username_lower LIKE :term_like OR
      TO_TSVECTOR('simple', name) @@
      TO_TSQUERY('simple',
        REGEXP_REPLACE(
          REGEXP_REPLACE(
            CAST(PLAINTO_TSQUERY(:term) AS text)
            ,'\''(?: |$)', ':*''', 'g'),
        '''', '', 'g')
    )", term: term, term_like: "#{term.downcase}%")
    
    scope.limit(20)
  end
end

not going to get into the rbenv vs rvm religious argument, not touching that with a ten foot pole.

1. Agree, slow tests need to be improved, test suite is slow.
2. schema.rb is impossible due to http://stackoverflow.com/questions/11894180/how-does-one-correctly-add-custom-sql-dml-in-migrations , we should probably find a way to nuke it for good
3. "No discernable coding style, respect for line length." will come with time , we need to establish this as a community
4. The refactor omits a whole bunch of stuff so is broken as is: eg: sql.order_by "case when username_lower = :term then 0 else 1 end asc", and a pile of ordering rules so it totally broken as is.
5. To me: "WHERE topic_id = #{topic_id.to_i}" is ugly, should be passed in as a param, but then you have weird locality issues
6. "sql_builder is just a reinvention of the ActiveRecord wheel. It's bad and it's everywhere." That is completely a statement of taste, to my taste the very tight dsl is way easier to understand and very flexible, we are talking less than 100 lines of code. To me "scope = scope.joins ("LEFT JOIN (" ... reads very badly. Its simply an extension of find_by_sql , a supported existing AR concept.
7. The glob is indeed ugly, but I needed it, if there is a cleaner way of achieving the same I would be more than happy to take it.
8. Rails handles all sanitization for you transparently. sql_builder uses the rails sanitizer
9. My philosophy is that if we need a scope we introduce it, we don't introduce it just in case we are ever going to need it.
10. We do much of our debugging in better_errors
11. The refactor omits the fact I only need a few columns, so it needs a .select so it becomes rather odd to return a scope with both a select an limit

Agree

1. Check to see if fabricator has an equivalent of FactoryGirl's build_stubbed. Reducing database interactions is the best way to get a fast test suite.
2. This is fair to some degree, but you're racking up a ton of technical debt and it's much easier to educate developers on the way in than herd cats once you hit scale.
3. It's tough to even read -- are you just regexping out noise and then doing a fulltext? If so, you can move the sanitization to Ruby and then stuff this complexity below a class method.
4. It does.
5. Yes, but contributors may not. That's one of the tools I think most Rails developers take as a given.
6. I'll take another stab at it when I have time.

Disagree

0. There is debate around rvm vs rbenv but there is zero debate around the fact that you need to choose one of the two. As it is, you're totally at the mercy of whatever insane local configuration each developer has, and you WILL cause conflicts and hard-to-track bugs given your current (lack of) approach.
1. The lack of an ability to export a partial index is not an excuse for letting schema.rb be out of date. You're missing entire tables, and that will be quite painful for future developers.
2. Egh, it's not in the tests. I'll take another look later.
3. Entirely subjective. I hear you, though, that .joins(...) should behave more like .where(...).
4. find_by_sql is not a "supported active record concept." It's a code smell and only to be used in really, really dire cases and when implementing gems. The official Rails docs are explicit about this:

If you’re used to using raw SQL to find database records, then you will generally find that there are better ways to carry out the same operations in Rails. Active Record insulates you from the need to use SQL in most cases.

http://guides.rubyonrails.org/active_record_querying.html

And, no, stating that it's "just a reinvention of the ActiveRecord wheel" is emphatically not a statement of taste, whatever you think about it. Every single method you've implemented has a corresponding method in ActiveRecord with the SAME NAME that does the SAME THING. Your library is a strict subset of the built-in ORM that lacks the flexibility and community testing that ActiveRecord has received over the years.

Don't get me wrong, AR is not perfect. It can be improved and it has shortcomings. However, I see absolutely nothing in your library that indicates you hit a wall with AR or saw some huge shortcoming.

9. I never suggested that you introduce scopes that are not needed. I suggested refactoring this method further so the scopes are implemented as classmethods (faux scopes). I suspect you will get some reuse out of them down the road, but the arguments for testability, granularity and readability are sufficient without reuse, in my opinion.
10. Generally Rails apps don't specify the select clause because it introduces undesirable coupling between the views and models, especially since there's no security or performance argument for doing so (that I'm aware of).

PS Just to be clear, I think it's great that you're open-sourcing this project and using Rails. I strongly prefer to be direct in the interest of time.

I think @blowmage's fork is an improvement over what I produced. See it here:

https://gist.github.com/blowmage/4755700