Merge pull request #4721 from oblakeerickson/sort_admin_users_api

FEATURE: Add order logic to admin users controller

app/jobs/scheduled/pending_users_reminder.rb

@@ -7,7 +7,7 @@ module Jobs
 
     def execute(args)
       if SiteSetting.must_approve_users && SiteSetting.pending_users_reminder_delay >= 0
-        query = AdminUserIndexQuery.new({query: 'pending'}).find_users_query # default order is: users.created_at DESC
+        query = AdminUserIndexQuery.new({query: 'pending', stats: false}).find_users_query # default order is: users.created_at DESC
         if SiteSetting.pending_users_reminder_delay > 0
           query = query.where('users.created_at < ?', SiteSetting.pending_users_reminder_delay.hours.ago)
         end

lib/admin_user_index_query.rb

@@ -10,16 +10,31 @@ class AdminUserIndexQuery
 
   attr_reader :params, :trust_levels
 
+  SORTABLE_MAPPING = {
+    'created' => 'created_at',
+    'last_emailed' => "COALESCE(last_emailed_at, to_date('1970-01-01', 'YYYY-MM-DD'))",
+    'seen' => "COALESCE(last_seen_at, to_date('1970-01-01', 'YYYY-MM-DD'))",
+    'username' => 'username',
+    'email' => 'email',
+    'trust_level' => 'trust_level',
+    'days_visited' => 'user_stats.days_visited',
+    'posts_read' => 'user_stats.posts_read_count',
+    'topics_viewed' => 'user_stats.topics_entered',
+    'posts' => 'user_stats.post_count',
+    'read_time' => 'user_stats.time_read'
+  }
+
   def find_users(limit=100)
-    find_users_query.includes(:user_stat).limit(limit)
+    find_users_query.limit(limit)
   end
 
   def count_users
     find_users_query.count
   end
 
-  def self.orderable_columns
-    %w(created_at days_visited posts_read_count topics_entered post_count trust_level)
+  def custom_direction
+    asc = params[:ascending]
+    asc.present? && asc ? "ASC" : "DESC"
   end
 
   def initialize_query_with_order(klass)
@@ -27,12 +42,11 @@ class AdminUserIndexQuery
 
     custom_order = params[:order]
     if custom_order.present? &&
-      without_dir = custom_order.downcase.sub(/ (asc|desc)$/, '')
-      if AdminUserIndexQuery.orderable_columns.include?(without_dir)
-        order << custom_order
-      end
+      without_dir = SORTABLE_MAPPING[custom_order.downcase.sub(/ (asc|desc)$/, '')]
+      order << "#{without_dir} #{custom_direction}"
     end
 
+    if !custom_order.present?
       if params[:query] == "active"
         order << "COALESCE(last_seen_at, to_date('1970-01-01', 'YYYY-MM-DD')) DESC"
       else
@@ -40,8 +54,13 @@ class AdminUserIndexQuery
       end
 
       order << "users.username"
+    end
 
+    if params[:stats].present? && params[:stats] == false
       klass.order(order.reject(&:blank?).join(","))
+    else
+      klass.includes(:user_stat).order(order.reject(&:blank?).join(","))
+    end
   end
 
   def filter_by_trust

spec/components/admin_user_index_query_spec.rb

@@ -23,9 +23,24 @@ describe AdminUserIndexQuery do
     end
 
     it "allows custom ordering" do
-      query = ::AdminUserIndexQuery.new({ order: "trust_level DESC" })
+      query = ::AdminUserIndexQuery.new({ order: "trust_level" })
       expect(query.find_users_query.to_sql).to match("trust_level DESC")
     end
+    
+    it "allows custom ordering asc" do
+      query = ::AdminUserIndexQuery.new({ order: "trust_level", ascending: true })
+      expect(query.find_users_query.to_sql).to match("trust_level ASC" )
+    end
+
+    it "allows custom ordering for stats wtih default direction" do
+      query = ::AdminUserIndexQuery.new({ order: "topics_viewed" })
+      expect(query.find_users_query.to_sql).to match("topics_entered DESC")
+    end
+
+    it "allows custom ordering and direction for stats" do
+      query = ::AdminUserIndexQuery.new({ order: "topics_viewed", ascending: true })
+      expect(query.find_users_query.to_sql).to match("topics_entered ASC")
+    end
   end
 
   describe "no users with trust level" do
@@ -70,6 +85,28 @@ describe AdminUserIndexQuery do
 
   end
 
+  describe "correct order with nil values" do
+    before(:each) do
+      Fabricate(:user, email: "test2@example.com", last_emailed_at: 1.hour.ago)
+    end
+
+    it "shows nil values first with asc" do
+      users = ::AdminUserIndexQuery.new({ order: "last_emailed", ascending: true }).find_users
+
+      expect(users.count).to eq(2)
+      expect(users.first.username).to eq("system")
+      expect(users.first.last_emailed_at).to eq(nil)
+    end
+
+    it "shows nil values last with desc" do
+      users = ::AdminUserIndexQuery.new({ order: "last_emailed"}).find_users
+
+      expect(users.count).to eq(2)
+      expect(users.first.last_emailed_at).to_not eq(nil)
+    end
+
+  end
+
   describe "with an admin user" do
 
     let!(:user) { Fabricate(:user, admin: true) }