Cache todos pending/done dashboard query counts

6 files changed, 62 insertions, 38 deletions

diff --git a/CHANGELOG b/CHANGELOG
index ee3ee4c37d6..3758baa0a0a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -49,6 +49,7 @@ v 8.10.0 (unreleased)
   - Collapse large diffs by default (!4990)
   - Check for conflicts with existing Project's wiki path when creating a new project.
   - Show last push widget in upstream after push to fork
+  - Cache todos pending/done dashboard query counts.
   - Don't instantiate a git tree on Projects show default view
   - Bump Rinku to 2.0.0
   - Remove unused front-end variable -> default_issues_tracker
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 3a2db3e6eeb..19a76a5b5d8 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -1,6 +1,4 @@
 class Dashboard::TodosController < Dashboard::ApplicationController
-  include TodosHelper
-
   before_action :find_todos, only: [:index, :destroy_all]
 
   def index
@@ -13,7 +11,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
     respond_to do |format|
       format.html { redirect_to dashboard_todos_path, notice: 'Todo was successfully marked as done.' }
       format.js { head :ok }
-      format.json { render json: { count: todos_pending_count, done_count: todos_done_count } }
+      format.json { render json: todos_counts }
     end
   end
 
@@ -23,7 +21,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
     respond_to do |format|
       format.html { redirect_to dashboard_todos_path, notice: 'All todos were marked as done.' }
       format.js { head :ok }
-      format.json { render json: { count: todos_pending_count, done_count: todos_done_count } }
+      format.json { render json: todos_counts }
     end
   end
 
@@ -36,4 +34,11 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   def find_todos
     @todos ||= TodosFinder.new(current_user, params).execute
   end
+
+  def todos_counts
+    {
+      count: TodosFinder.new(current_user, state: :pending).execute.count,
+      done_count: TodosFinder.new(current_user, state: :done).execute.count
+    }
+  end
 end
diff --git a/app/controllers/projects/todos_controller.rb b/app/controllers/projects/todos_controller.rb
index 23868d986e9..5685d0f4e7c 100644
--- a/app/controllers/projects/todos_controller.rb
+++ b/app/controllers/projects/todos_controller.rb
@@ -5,7 +5,7 @@ class Projects::TodosController < Projects::ApplicationController
     todo = TodoService.new.mark_todo(issuable, current_user)
 
     render json: {
-      count: current_user.todos_pending_count,
+      count: TodosFinder.new(current_user, state: :pending).execute.count,
       delete_path: dashboard_todo_path(todo)
     }
   end
diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb
index 7806d9e4cc5..1a8f490355b 100644
--- a/app/finders/todos_finder.rb
+++ b/app/finders/todos_finder.rb
@@ -8,7 +8,7 @@
 #     action_id: integer
 #     author_id: integer
 #     project_id; integer
-#     state: 'pending' or 'done'
+#     state: 'pending' (default) or 'done'
 #     type: 'Issue' or 'MergeRequest'
 #
 
diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb
index a832a6c8df7..0925760e69c 100644
--- a/app/helpers/todos_helper.rb
+++ b/app/helpers/todos_helper.rb
@@ -1,10 +1,10 @@
 module TodosHelper
   def todos_pending_count
-    TodosFinder.new(current_user, state: :pending).execute.count
+    @todos_pending_count ||= TodosFinder.new(current_user, state: :pending).execute.count
   end
 
   def todos_done_count
-    TodosFinder.new(current_user, state: :done).execute.count
+    @todos_done_count ||= TodosFinder.new(current_user, state: :done).execute.count
   end
 
   def todo_action_name(todo)
diff --git a/spec/controllers/projects/todo_controller_spec.rb b/spec/controllers/projects/todo_controller_spec.rb
index 5a8bba28594..936320a3709 100644
--- a/spec/controllers/projects/todo_controller_spec.rb
+++ b/spec/controllers/projects/todo_controller_spec.rb
@@ -1,6 +1,8 @@
 require('spec_helper')
 
 describe Projects::TodosController do
+  include ApiHelpers
+
   let(:user)          { create(:user) }
   let(:project)       { create(:project) }
   let(:issue)         { create(:issue, project: project) }
@@ -8,43 +10,51 @@ describe Projects::TodosController do
 
   context 'Issues' do
     describe 'POST create' do
+      def go
+        post :create,
+          namespace_id: project.namespace.path,
+          project_id: project.path,
+          issuable_id: issue.id,
+          issuable_type: 'issue',
+          format: 'html'
+      end
+
       context 'when authorized' do
         before do
           sign_in(user)
           project.team << [user, :developer]
         end
 
-        it 'should create todo for issue' do
+        it 'creates todo for issue' do
           expect do
-            post(:create, namespace_id: project.namespace.path,
-                          project_id: project.path,
-                          issuable_id: issue.id,
-                          issuable_type: 'issue')
+            go
           end.to change { user.todos.count }.by(1)
 
           expect(response).to have_http_status(200)
         end
+
+        it 'returns todo path and pending count' do
+          go
+
+          expect(response).to have_http_status(200)
+          expect(json_response['count']).to eq 1
+          expect(json_response['delete_path']).to match(/\/dashboard\/todos\/\d{1}/)
+        end
       end
 
       context 'when not authorized' do
-        it 'should not create todo for issue that user has no access to' do
+        it 'does not create todo for issue that user has no access to' do
           sign_in(user)
           expect do
-            post(:create, namespace_id: project.namespace.path,
-                          project_id: project.path,
-                          issuable_id: issue.id,
-                          issuable_type: 'issue')
+            go
           end.to change { user.todos.count }.by(0)
 
           expect(response).to have_http_status(404)
         end
 
-        it 'should not create todo for issue when user not logged in' do
+        it 'does not create todo for issue when user not logged in' do
           expect do
-            post(:create, namespace_id: project.namespace.path,
-                          project_id: project.path,
-                          issuable_id: issue.id,
-                          issuable_type: 'issue')
+            go
           end.to change { user.todos.count }.by(0)
 
           expect(response).to have_http_status(302)
@@ -55,43 +65,51 @@ describe Projects::TodosController do
 
   context 'Merge Requests' do
     describe 'POST create' do
+      def go
+        post :create,
+          namespace_id: project.namespace.path,
+          project_id: project.path,
+          issuable_id: merge_request.id,
+          issuable_type: 'merge_request',
+          format: 'html'
+      end
+
       context 'when authorized' do
         before do
           sign_in(user)
           project.team << [user, :developer]
         end
 
-        it 'should create todo for merge request' do
+        it 'creates todo for merge request' do
           expect do
-            post(:create, namespace_id: project.namespace.path,
-                          project_id: project.path,
-                          issuable_id: merge_request.id,
-                          issuable_type: 'merge_request')
+            go
           end.to change { user.todos.count }.by(1)
 
           expect(response).to have_http_status(200)
         end
+
+        it 'returns todo path and pending count' do
+          go
+
+          expect(response).to have_http_status(200)
+          expect(json_response['count']).to eq 1
+          expect(json_response['delete_path']).to match(/\/dashboard\/todos\/\d{1}/)
+        end
       end
 
       context 'when not authorized' do
-        it 'should not create todo for merge request user has no access to' do
+        it 'does not create todo for merge request user has no access to' do
           sign_in(user)
           expect do
-            post(:create, namespace_id: project.namespace.path,
-                          project_id: project.path,
-                          issuable_id: merge_request.id,
-                          issuable_type: 'merge_request')
+            go
           end.to change { user.todos.count }.by(0)
 
           expect(response).to have_http_status(404)
         end
 
-        it 'should not create todo for merge request user has no access to' do
+        it 'does not create todo for merge request user has no access to' do
           expect do
-            post(:create, namespace_id: project.namespace.path,
-                          project_id: project.path,
-                          issuable_id: merge_request.id,
-                          issuable_type: 'merge_request')
+            go
           end.to change { user.todos.count }.by(0)
 
           expect(response).to have_http_status(302)