Merge branch 'cache-todos-finder-calls' into 'master'

Cache todos pending/done dashboard query counts

See #19273

See merge request !5175

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

app/controllers/dashboard/todos_controller.rb

 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

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

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'
 #
 

app/helpers/todos_helper.rb

 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)

spec/controllers/projects/todo_controller_spec.rb

 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)