Merge branch 'fix-todos-counters' into 'master'

Ensure Todos counters doesn't count Todos for projects pending delete

Use `TodosFinder` instead of `current_user.todos` to filter projects
pending delete on Todos counters helpers.

Counters should not reflect the number of Todos displayed on the tabs.

Fixes #18633

See merge request !4663

7 files changed, 51 insertions, 23 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 68287f0e6b9..e0c020997ab 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -127,6 +127,7 @@ v 8.9.0 (unreleased)
   - Use Git cached counters for branches and tags on project page
   - Filter parameters for request_uri value on instrumented transactions.
   - Cache user todo counts from TodoService
+  - Ensure Todos counters doesn't count Todos for projects pending delete
 
 v 8.8.5
   - Import GitHub repositories respecting the API rate limit !4166
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 7842fb9ce63..3a2db3e6eeb 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -1,5 +1,7 @@
 class Dashboard::TodosController < Dashboard::ApplicationController
-  before_action :find_todos, only: [:index, :destroy, :destroy_all]
+  include TodosHelper
+
+  before_action :find_todos, only: [:index, :destroy_all]
 
   def index
     @todos = @todos.page(params[:page])
@@ -8,14 +10,10 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   def destroy
     TodoService.new.mark_todos_as_done([todo], current_user)
 
-    todo_notice = 'Todo was successfully marked as done.'
-
     respond_to do |format|
-      format.html { redirect_to dashboard_todos_path, notice: todo_notice }
+      format.html { redirect_to dashboard_todos_path, notice: 'Todo was successfully marked as done.' }
       format.js { head :ok }
-      format.json do
-        render json: { count: @todos.size, done_count: current_user.todos_done_count }
-      end
+      format.json { render json: { count: todos_pending_count, done_count: todos_done_count } }
     end
   end
 
@@ -25,20 +23,17 @@ 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 do
-        find_todos
-        render json: { count: @todos.size, done_count: current_user.todos_done_count }
-      end
+      format.json { render json: { count: todos_pending_count, done_count: todos_done_count } }
     end
   end
 
   private
 
   def todo
-    @todo ||= current_user.todos.find(params[:id])
+    @todo ||= find_todos.find(params[:id])
   end
 
   def find_todos
-    @todos = TodosFinder.new(current_user, params).execute
+    @todos ||= TodosFinder.new(current_user, params).execute
   end
 end
diff --git a/app/finders/todos_finder.rb b/app/finders/todos_finder.rb
index aa47c6c157e..58a00f88af7 100644
--- a/app/finders/todos_finder.rb
+++ b/app/finders/todos_finder.rb
@@ -123,7 +123,7 @@ class TodosFinder
   end
 
   def by_state(items)
-    case params[:state]
+    case params[:state].to_s
     when 'done'
       items.done
     else
diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb
index c7aeed4b9fc..e1d8517f712 100644
--- a/app/helpers/todos_helper.rb
+++ b/app/helpers/todos_helper.rb
@@ -1,10 +1,10 @@
 module TodosHelper
   def todos_pending_count
-    current_user.todos_pending_count
+    TodosFinder.new(current_user, state: :pending).execute.count
   end
 
   def todos_done_count
-    current_user.todos_done_count
+    TodosFinder.new(current_user, state: :done).execute.count
   end
 
   def todo_action_name(todo)
diff --git a/features/dashboard/todos.feature b/features/dashboard/todos.feature
index 8677b450813..42f5d6d2af7 100644
--- a/features/dashboard/todos.feature
+++ b/features/dashboard/todos.feature
@@ -14,7 +14,12 @@ Feature: Dashboard Todos
   Scenario: I mark todos as done
     Then I should see todos assigned to me
     And I mark the todo as done
-    And I click on the "Done" tab
+    Then I should see the todo marked as done
+
+  @javascript
+  Scenario: I mark all todos as done
+    Then I should see todos assigned to me
+    And I mark all todos as done
     Then I should see all todos marked as done
 
   @javascript
diff --git a/features/steps/dashboard/todos.rb b/features/steps/dashboard/todos.rb
index 19fedfbfcdf..60152d3da55 100644
--- a/features/steps/dashboard/todos.rb
+++ b/features/steps/dashboard/todos.rb
@@ -26,14 +26,15 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
   end
 
   step 'I should see todos assigned to me' do
+    page.within('.todos-pending-count') { expect(page).to have_content '4' }
     expect(page).to have_content 'To do 4'
     expect(page).to have_content 'Done 0'
 
     expect(page).to have_link project.name_with_namespace
     should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference}", merge_request.title)
-    should_see_todo(2, "John Doe mentioned you on issue ##{issue.iid}", "#{current_user.to_reference} Wdyt?")
-    should_see_todo(3, "John Doe assigned you issue ##{issue.iid}", issue.title)
-    should_see_todo(4, "Mary Jane mentioned you on issue ##{issue.iid}", issue.title)
+    should_see_todo(2, "John Doe mentioned you on issue #{issue.to_reference}", "#{current_user.to_reference} Wdyt?")
+    should_see_todo(3, "John Doe assigned you issue #{issue.to_reference}", issue.title)
+    should_see_todo(4, "Mary Jane mentioned you on issue #{issue.to_reference}", issue.title)
   end
 
   step 'I mark the todo as done' do
@@ -41,18 +42,40 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
       click_link 'Done'
     end
 
+    page.within('.todos-pending-count') { expect(page).to have_content '3' }
     expect(page).to have_content 'To do 3'
     expect(page).to have_content 'Done 1'
     should_not_see_todo "John Doe assigned you merge request #{merge_request.to_reference}"
   end
 
-  step 'I click on the "Done" tab' do
+  step 'I mark all todos as done' do
+    click_link 'Mark all as done'
+
+    page.within('.todos-pending-count') { expect(page).to have_content '0' }
+    expect(page).to have_content 'To do 0'
+    expect(page).to have_content 'Done 4'
+    expect(page).not_to have_link project.name_with_namespace
+    should_not_see_todo "John Doe assigned you merge request #{merge_request.to_reference}"
+    should_not_see_todo "John Doe mentioned you on issue #{issue.to_reference}"
+    should_not_see_todo "John Doe assigned you issue #{issue.to_reference}"
+    should_not_see_todo "Mary Jane mentioned you on issue #{issue.to_reference}"
+  end
+
+  step 'I should see the todo marked as done' do
     click_link 'Done 1'
+
+    expect(page).to have_link project.name_with_namespace
+    should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference}", merge_request.title, false)
   end
 
   step 'I should see all todos marked as done' do
+    click_link 'Done 4'
+
     expect(page).to have_link project.name_with_namespace
     should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference}", merge_request.title, false)
+    should_see_todo(2, "John Doe mentioned you on issue #{issue.to_reference}", "#{current_user.to_reference} Wdyt?", false)
+    should_see_todo(3, "John Doe assigned you issue #{issue.to_reference}", issue.title, false)
+    should_see_todo(4, "Mary Jane mentioned you on issue #{issue.to_reference}", issue.title, false)
   end
 
   step 'I filter by "Enterprise"' do
@@ -76,7 +99,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
   end
 
   step 'I should not see todos related to "Mary Jane" in the list' do
-    should_not_see_todo "Mary Jane mentioned you on issue ##{issue.iid}"
+    should_not_see_todo "Mary Jane mentioned you on issue #{issue.to_reference}"
   end
 
   step 'I should not see todos related to "Merge Requests" in the list' do
@@ -85,7 +108,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
 
   step 'I should not see todos related to "Assignments" in the list' do
     should_not_see_todo "John Doe assigned you merge request #{merge_request.to_reference}"
-    should_not_see_todo "John Doe assigned you issue ##{issue.iid}"
+    should_not_see_todo "John Doe assigned you issue #{issue.to_reference}"
   end
 
   step 'I click on the todo' do
diff --git a/spec/features/todos/todos_spec.rb b/spec/features/todos/todos_spec.rb
index 8e1833a069e..0bdb1628c74 100644
--- a/spec/features/todos/todos_spec.rb
+++ b/spec/features/todos/todos_spec.rb
@@ -103,11 +103,15 @@ describe 'Dashboard Todos', feature: true do
       before do
         deleted_project = create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC, pending_delete: true)
         create(:todo, :mentioned, user: user, project: deleted_project, target: issue, author: author)
+        create(:todo, :mentioned, user: user, project: deleted_project, target: issue, author: author, state: :done)
         login_as(user)
         visit dashboard_todos_path
       end
 
       it 'shows "All done" message' do
+        within('.todos-pending-count') { expect(page).to have_content '0' }
+        expect(page).to have_content 'To do 0'
+        expect(page).to have_content 'Done 0'
         expect(page).to have_content "You're all done!"
       end
     end