Merge pull request #57 from blowmage/user_search_refactor

Refactor UserController#search_users

app/controllers/users_controller.rb

@@ -288,44 +288,14 @@ class UsersController < ApplicationController
   end
 
   def search_users
-
-    term = (params[:term] || "").strip.downcase
+    term = params[:term].to_s.strip
     topic_id = params[:topic_id]
     topic_id = topic_id.to_i if topic_id
 
-    sql = "select username, name, email from users u "
-    if topic_id
-      sql << "left join (select distinct p.user_id from posts p where topic_id = :topic_id) s on
-        s.user_id = u.id "
-    end
+    results = UserSearch.search term, topic_id
 
-    if term.length > 0
-      sql << "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')
-              ) "
-
-    end
-
-    sql << "order by case when username_lower = :term then 0 else 1 end asc, "
-    if topic_id
-      sql << " case when s.user_id is null then 0 else 1 end desc, "
-    end
-
-    sql << " case when last_seen_at is null then 0 else 1 end desc, last_seen_at desc, username asc limit(20)"
-
-    results = User.exec_sql(sql, topic_id: topic_id, term_like: "#{term}%", term: term)
-    results = results.map do |r|
-      r["avatar_template"] = User.avatar_template(r["email"])
-      r.delete("email")
-      r
-    end
-    render :json => results
+    render json: { users: results.as_json( only:    [ :username, :name ],
+                                           methods: :avatar_template ) }
   end
 
   private

app/models/user_search.rb

@@ -0,0 +1,43 @@
+class UserSearch
+
+  def self.search term, topic_id = nil
+    User.find_by_sql sql(term, topic_id)
+  end
+
+  private
+
+  def self.sql term, topic_id
+    sql = "select id, username, name, email from users u "
+    if topic_id
+      sql << "left join (select distinct p.user_id from posts p where topic_id = :topic_id) s on
+        s.user_id = u.id "
+    end
+
+    if term.present?
+      sql << "where username ilike :term_like or
+              to_tsvector('simple', name) @@
+              to_tsquery('simple',
+                regexp_replace(
+                  regexp_replace(
+                    cast(plainto_tsquery(:term) as text)
+                    ,'\''(?: |$)', ':*''', 'g'),
+                '''', '', 'g')
+              ) "
+
+    end
+
+    sql << "order by case when username_lower = :term then 0 else 1 end asc, "
+    if topic_id
+      sql << " case when s.user_id is null then 0 else 1 end desc, "
+    end
+
+    sql << " case when last_seen_at is null then 0 else 1 end desc, last_seen_at desc, username asc limit(20)"
+
+    sanitize_sql_array(sql, topic_id: topic_id, term_like: "#{term}%", term: term)
+  end
+
+  def self.sanitize_sql_array *args
+    ActiveRecord::Base.send(:sanitize_sql_array, args)
+  end
+
+end

spec/controllers/users_controller_spec.rb

@@ -1,13 +1,13 @@
 require 'spec_helper'
 
-describe UsersController do 
+describe UsersController do
 
   before do
     UsersController.any_instance.stubs(:honeypot_value).returns(nil)
     UsersController.any_instance.stubs(:challenge_value).returns(nil)
   end
 
-  describe '.show' do    
+  describe '.show' do
     let!(:user) { log_in }
 
     it 'returns success' do
@@ -52,7 +52,7 @@ describe UsersController do
 
       it 'sets a flash error' do
         flash[:error].should be_present
-      end      
+      end
     end
 
     context 'valid token' do
@@ -74,7 +74,7 @@ describe UsersController do
       it 'logs in as the user' do
         session[:current_user_id].should be_present
       end
-    end    
+    end
   end
 
   describe '.activate_account' do
@@ -90,7 +90,7 @@ describe UsersController do
 
       it 'sets a flash error' do
         flash[:error].should be_present
-      end      
+      end
     end
 
     context 'valid token' do
@@ -102,13 +102,13 @@ describe UsersController do
         end
 
         it 'enqueues a welcome message if the user object indicates so' do
-          user.send_welcome_message = true          
+          user.send_welcome_message = true
           user.expects(:enqueue_welcome_message).with('welcome_user')
           get :activate_account, token: 'asdfasdf'
         end
 
         it "doesn't enqueue the welcome message if the object returns false" do
-          user.send_welcome_message = false          
+          user.send_welcome_message = false
           user.expects(:enqueue_welcome_message).with('welcome_user').never
           get :activate_account, token: 'asdfasdf'
         end
@@ -131,12 +131,12 @@ describe UsersController do
 
         it 'logs in as the user' do
           session[:current_user_id].should be_present
-        end       
+        end
 
         it "doesn't set @needs_approval" do
           assigns[:needs_approval].should be_blank
-        end 
-        
+        end
+
       end
 
       context 'must_approve_users' do
@@ -166,7 +166,7 @@ describe UsersController do
     end
   end
 
-  describe '.change_email' do    
+  describe '.change_email' do
     let(:new_email) { 'bubblegum@adventuretime.ooo' }
 
     it "requires you to be logged in" do
@@ -223,11 +223,11 @@ describe UsersController do
 
       it 'sets a flash error' do
         flash[:error].should be_present
-      end      
+      end
 
       it "doesn't log in the user" do
         session[:current_user_id].should be_blank
-      end      
+      end
     end
 
     context 'valid token' do
@@ -259,17 +259,17 @@ describe UsersController do
         SiteSetting.expects(:must_approve_users?).returns(true)
         put :password_reset, token: 'asdfasdf', password: 'newpassword'
         session[:current_user_id].should be_blank
-      end       
+      end
     end
 
 
   end
 
 
-  describe '.create' do 
-    before do 
+  describe '.create' do
+    before do
       @user = Fabricate.build(:user)
-      @user.password = "strongpassword"      
+      @user.password = "strongpassword"
       Mothership.stubs(:register_nickname).returns([true, nil])
     end
 
@@ -282,7 +282,7 @@ describe UsersController do
       it "doesn't send a welcome email" do
         User.any_instance.expects(:enqueue_welcome_message).with('welcome_user').never
         xhr :post, :create, :name => @user.name, :username => @user.username, :password => "strongpassword", :email => @user.email
-      end    
+      end
     end
 
     context 'when creating an active user (confirmed email)' do
@@ -294,7 +294,7 @@ describe UsersController do
       it 'should enqueue a signup email' do
         User.any_instance.expects(:enqueue_welcome_message).with('welcome_user')
         xhr :post, :create, :name => @user.name, :username => @user.username, :password => "strongpassword", :email => @user.email
-      end      
+      end
 
       it "should be logged in" do
         User.any_instance.expects(:enqueue_welcome_message)
@@ -331,16 +331,16 @@ describe UsersController do
         xhr :post, :create, :name => @user.name, :username => @user.username, :password => "strongpassword", :email => @user.email
       end
 
-      it 'should succeed' do 
+      it 'should succeed' do
         should respond_with(:success)
       end
 
       it 'has the proper JSON' do
         json = JSON::parse(response.body)
-        json["success"].should be_true 
+        json["success"].should be_true
       end
 
-      it 'should not result in an active account' do 
+      it 'should not result in an active account' do
         User.where(username: @user.username).first.active.should be_false
       end
     end
@@ -391,7 +391,7 @@ describe UsersController do
       let(:new_username) { "#{user.username}1234" }
 
       it 'raises an error without a new_username param' do
-        lambda { xhr :put, :username, username: user.username }.should raise_error(Discourse::InvalidParameters) 
+        lambda { xhr :put, :username, username: user.username }.should raise_error(Discourse::InvalidParameters)
       end
 
       it 'raises an error when you don\'t have permission to change the user' do
@@ -402,7 +402,7 @@ describe UsersController do
 
       it 'raises an error when change_username fails' do
         User.any_instance.expects(:change_username).with(new_username).returns(false)
-        lambda { xhr :put, :username, username: user.username, new_username: new_username }.should raise_error(Discourse::InvalidParameters)        
+        lambda { xhr :put, :username, username: user.username, new_username: new_username }.should raise_error(Discourse::InvalidParameters)
       end
 
       it 'should succeed when the change_username returns true' do
@@ -671,4 +671,36 @@ describe UsersController do
 
   end
 
+  describe "search_users" do
+
+    let(:topic) { Fabricate :topic }
+    let(:user)  { Fabricate :user, username: "joecabot", name: "Lawrence Tierney" }
+
+    before do
+      Fabricate :post, user: user, topic: topic
+    end
+
+    it "searches when provided the term only" do
+      xhr :post, :search_users, term: user.name.split(" ").last
+      response.should be_success
+      json = JSON.parse(response.body)
+      json["users"].map { |u| u["username"] }.should include(user.username)
+    end
+
+    it "searches when provided the topic only" do
+      xhr :post, :search_users, topic_id: topic.id
+      response.should be_success
+      json = JSON.parse(response.body)
+      json["users"].map { |u| u["username"] }.should include(user.username)
+    end
+
+    it "searches when provided the term and topic" do
+      xhr :post, :search_users, term: user.name.split(" ").last, topic_id: topic.id
+      response.should be_success
+      json = JSON.parse(response.body)
+      json["users"].map { |u| u["username"] }.should include(user.username)
+    end
+
+  end
+
 end

spec/models/user_search_spec.rb

@@ -0,0 +1,83 @@
+require 'spec_helper'
+
+describe UserSearch do
+
+  let(:topic)  { Fabricate :topic }
+  let(:topic2) { Fabricate :topic }
+  let(:topic3) { Fabricate :topic }
+  let(:user1)  { Fabricate :user, username: "mrblonde", name: "Michael Madsen" }
+  let(:user2)  { Fabricate :user, username: "mrblue",   name: "Eddie Bunker" }
+  let(:user3)  { Fabricate :user, username: "mrorange", name: "Tim Roth" }
+  let(:user4)  { Fabricate :user, username: "mrpink",   name: "Steve Buscemi" }
+  let(:user5)  { Fabricate :user, username: "mrbrown",  name: "Quentin Tarantino" }
+  let(:user6)  { Fabricate :user, username: "mrwhite",  name: "Harvey Keitel" }
+
+  before do
+    Fabricate :post, user: user1, topic: topic
+    Fabricate :post, user: user2, topic: topic2
+    Fabricate :post, user: user3, topic: topic
+    Fabricate :post, user: user4, topic: topic
+    Fabricate :post, user: user5, topic: topic3
+    Fabricate :post, user: user6, topic: topic
+  end
+
+  context "all user search" do
+    it "searches the user's name" do
+      results = UserSearch.search user1.name.split(" ").first
+      results.size.should == 1
+      results.first.should == user1
+    end
+
+    it "searches the user's name case insensitive" do
+      results = UserSearch.search user1.name.split(" ").first.downcase
+      results.size.should == 1
+      results.first.should == user1
+    end
+
+    it "searches the user's username" do
+      results = UserSearch.search user4.username
+      results.size.should == 1
+      results.first.should == user4
+    end
+
+    it "searches the user's username case insensitive" do
+      results = UserSearch.search user4.username.upcase
+      results.size.should == 1
+      results.first.should == user4
+    end
+
+    it "searches the user's username substring" do
+      results = UserSearch.search "mr"
+      results.size.should == 6
+
+      results = UserSearch.search "mrb"
+      results.size.should == 3
+    end
+
+    it "searches the user's username substring upper case" do
+      results = UserSearch.search "MR"
+      results.size.should == 6
+
+      results = UserSearch.search "MRB"
+      results.size.should == 3
+    end
+  end
+
+  context "sort order respects users with posts on the topic" do
+    it "Mr. Blond is first when searching his topic" do
+      results = UserSearch.search "mrb", topic.id
+      results.first.should == user1
+    end
+
+    it "Mr. Blue is first when searching his topic" do
+      results = UserSearch.search "mrb", topic2.id
+      results.first.should == user2
+    end
+
+    it "Mr. Brown is first when searching his topic" do
+      results = UserSearch.search "mrb", topic3.id
+      results.first.should == user5
+    end
+  end
+
+end