Merge branch 'remove-duplicate-orders' into 'master'

Remove duplicate orders

This changes the default order from `created_at DESC, id DESC` to just `id DESC` as this achieves the same results without the overhead of having to sort data twice (we've seen queries go from 200ms to just a few ms by removing the double sort).

cc @jacobvosmaer @dzaporozhets @rspeicher @DouweM 

See merge request !1735

3 files changed, 21 insertions, 11 deletions

diff --git a/app/models/concerns/sortable.rb b/app/models/concerns/sortable.rb
index 0ad2654867d..913c747a1c3 100644
--- a/app/models/concerns/sortable.rb
+++ b/app/models/concerns/sortable.rb
@@ -8,12 +8,12 @@ module Sortable
   included do
     # By default all models should be ordered
     # by created_at field starting from newest
-    default_scope { order(created_at: :desc, id: :desc) }
+    default_scope { order(id: :desc) }
 
-    scope :order_created_desc, -> { reorder(created_at: :desc, id: :desc) }
-    scope :order_created_asc, -> { reorder(created_at: :asc, id: :asc) }
-    scope :order_updated_desc, -> { reorder(updated_at: :desc, id: :desc) }
-    scope :order_updated_asc, -> { reorder(updated_at: :asc, id: :asc) }
+    scope :order_created_desc, -> { reorder(created_at: :desc) }
+    scope :order_created_asc, -> { reorder(created_at: :asc) }
+    scope :order_updated_desc, -> { reorder(updated_at: :desc) }
+    scope :order_updated_asc, -> { reorder(updated_at: :asc) }
     scope :order_name_asc, -> { reorder(name: :asc) }
     scope :order_name_desc, -> { reorder(name: :desc) }
   end
diff --git a/spec/benchmarks/models/user_spec.rb b/spec/benchmarks/models/user_spec.rb
index 4cdba66939b..1be7a8d3ed9 100644
--- a/spec/benchmarks/models/user_spec.rb
+++ b/spec/benchmarks/models/user_spec.rb
@@ -1,6 +1,16 @@
 require 'spec_helper'
 
 describe User, benchmark: true do
+  describe '.all' do
+    before do
+      10.times { create(:user) }
+    end
+
+    benchmark_subject { User.all.to_a }
+
+    it { is_expected.to iterate_per_second(500) }
+  end
+
   describe '.by_login' do
     before do
       %w{Alice Bob Eve}.each do |name|
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index c71cfb3ebe3..49e0bfdd2ec 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -663,24 +663,24 @@ describe User do
       @user1 = create :user, created_at: Date.today - 1, last_sign_in_at: Date.today - 1, name: 'Omega'
     end
 
-    it "sorts users as recently_signed_in" do
+    it "sorts users by the recent sign-in time" do
       expect(User.sort('recent_sign_in').first).to eq(@user)
     end
 
-    it "sorts users as late_signed_in" do
+    it "sorts users by the oldest sign-in time" do
       expect(User.sort('oldest_sign_in').first).to eq(@user1)
     end
 
-    it "sorts users as recently_created" do
+    it "sorts users in descending order by their creation time" do
       expect(User.sort('created_desc').first).to eq(@user)
     end
 
-    it "sorts users as late_created" do
+    it "sorts users in ascending order by their creation time" do
       expect(User.sort('created_asc').first).to eq(@user1)
     end
 
-    it "sorts users by name when nil is passed" do
-      expect(User.sort(nil).first).to eq(@user)
+    it "sorts users by id in descending order when nil is passed" do
+      expect(User.sort(nil).first).to eq(@user1)
     end
   end