From 063b9edc777e3df992e161c94572f4ff6ece9e02 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Wed, 4 Oct 2017 17:37:38 +0100
Subject: [PATCH] Save a query on the todos index page

When there are no filters, we can get the total todos count from the cached
count on the user object, instead of performing a DB query. This query takes
about 80ms for me on GitLab.com.
---
 app/controllers/dashboard/todos_controller.rb | 30 ++++++++++++++++---
 .../save-a-query-on-todos-with-no-filters.yml |  5 ++++
 .../dashboard/todos_controller_spec.rb        | 26 +++++++++++++++-
 3 files changed, 56 insertions(+), 5 deletions(-)
 create mode 100644 changelogs/unreleased/save-a-query-on-todos-with-no-filters.yml

diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index a8b2b93b458..02c5857eea7 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -7,9 +7,8 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   def index
     @sort = params[:sort]
     @todos = @todos.page(params[:page])
-    if @todos.out_of_range? && @todos.total_pages != 0
-      redirect_to url_for(params.merge(page: @todos.total_pages, only_path: true))
-    end
+
+    return if redirect_out_of_range(@todos)
   end
 
   def destroy
@@ -60,7 +59,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def find_todos
-    @todos ||= TodosFinder.new(current_user, params).execute
+    @todos ||= TodosFinder.new(current_user, todo_params).execute
   end
 
   def todos_counts
@@ -69,4 +68,27 @@ class Dashboard::TodosController < Dashboard::ApplicationController
       done_count: number_with_delimiter(current_user.todos_done_count)
     }
   end
+
+  def todo_params
+    params.permit(:action_id, :author_id, :project_id, :type, :sort, :state)
+  end
+
+  def redirect_out_of_range(todos)
+    total_pages =
+      if todo_params.except(:sort, :page).empty?
+        (current_user.todos_pending_count / todos.limit_value).ceil
+      else
+        todos.total_pages
+      end
+
+    return false if total_pages.zero?
+
+    out_of_range = todos.current_page > total_pages
+
+    if out_of_range
+      redirect_to url_for(params.merge(page: total_pages, only_path: true))
+    end
+
+    out_of_range
+  end
 end
diff --git a/changelogs/unreleased/save-a-query-on-todos-with-no-filters.yml b/changelogs/unreleased/save-a-query-on-todos-with-no-filters.yml
new file mode 100644
index 00000000000..c9fb042aa37
--- /dev/null
+++ b/changelogs/unreleased/save-a-query-on-todos-with-no-filters.yml
@@ -0,0 +1,5 @@
+---
+title: Remove a SQL query from the todos index page
+merge_request:
+author:
+type: other
diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb
index c8c6b9f41bf..9df4ebf2fa0 100644
--- a/spec/controllers/dashboard/todos_controller_spec.rb
+++ b/spec/controllers/dashboard/todos_controller_spec.rb
@@ -57,7 +57,7 @@ describe Dashboard::TodosController do
         expect(response).to redirect_to(dashboard_todos_path(page: last_page))
       end
 
-      it 'redirects to correspondent page' do
+      it 'goes to the correct page' do
         get :index, page: last_page
 
         expect(assigns(:todos).current_page).to eq(last_page)
@@ -70,6 +70,30 @@ describe Dashboard::TodosController do
 
         expect(response).to redirect_to(dashboard_todos_path(page: last_page))
       end
+
+      context 'when providing no filters' do
+        it 'does not perform a query to get the page count, but gets that from the user' do
+          allow(controller).to receive(:current_user).and_return(user)
+
+          expect(user).to receive(:todos_pending_count).and_call_original
+
+          get :index, page: (last_page + 1).to_param, sort: :created_asc
+
+          expect(response).to redirect_to(dashboard_todos_path(page: last_page, sort: :created_asc))
+        end
+      end
+
+      context 'when providing filters' do
+        it 'performs a query to get the correct page count' do
+          allow(controller).to receive(:current_user).and_return(user)
+
+          expect(user).not_to receive(:todos_pending_count)
+
+          get :index, page: (last_page + 1).to_param, project_id: project.id
+
+          expect(response).to redirect_to(dashboard_todos_path(page: last_page, project_id: project.id))
+        end
+      end
     end
   end