Merge branch '40871-todo-notification-count-shows-notification-without-having-a-todo' into 'master'

Resolve "Todo notification count shows notification without having a todo"

Closes #40871

See merge request gitlab-org/gitlab-ce!15807

10 files changed, 73 insertions, 18 deletions

diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb
index c3013884369..74a4f437dc8 100644
--- a/app/controllers/concerns/issuable_actions.rb
+++ b/app/controllers/concerns/issuable_actions.rb
@@ -55,7 +55,6 @@ module IssuableActions
 
   def destroy
     Issuable::DestroyService.new(issuable.project, current_user).execute(issuable)
-    TodoService.new.destroy_issuable(issuable, current_user)
 
     name = issuable.human_class_name
     flash[:notice] = "The #{name} was successfully deleted."
diff --git a/app/services/issuable/destroy_service.rb b/app/services/issuable/destroy_service.rb
index 0610b401213..7197a426a72 100644
--- a/app/services/issuable/destroy_service.rb
+++ b/app/services/issuable/destroy_service.rb
@@ -1,8 +1,10 @@
 module Issuable
   class DestroyService < IssuableBaseService
     def execute(issuable)
-      if issuable.destroy
-        issuable.update_project_counter_caches
+      TodoService.new.destroy_target(issuable) do |issuable|
+        if issuable.destroy
+          issuable.update_project_counter_caches
+        end
       end
     end
   end
diff --git a/app/services/notes/destroy_service.rb b/app/services/notes/destroy_service.rb
index b819bd17039..fb78420d324 100644
--- a/app/services/notes/destroy_service.rb
+++ b/app/services/notes/destroy_service.rb
@@ -1,7 +1,9 @@
 module Notes
   class DestroyService < BaseService
     def execute(note)
-      note.destroy
+      TodoService.new.destroy_target(note) do |note|
+        note.destroy
+      end
     end
   end
 end
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index 575853fd66b..c2ca404b179 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -31,12 +31,20 @@ class TodoService
     mark_pending_todos_as_done(issue, current_user)
   end
 
-  # When we destroy an issuable we should:
+  # When we destroy a todo target we should:
   #
-  #  * refresh the todos count cache for the current user
+  #  * refresh the todos count cache for all users with todos on the target
   #
-  def destroy_issuable(issuable, user)
-    user.update_todos_count_cache
+  # This needs to yield back to the caller to destroy the target, because it
+  # collects the todo users before the todos themselves are deleted, then
+  # updates the todo counts for those users.
+  #
+  def destroy_target(target)
+    todo_users = User.where(id: target.todos.pending.select(:user_id)).to_a
+
+    yield target
+
+    todo_users.each(&:update_todos_count_cache)
   end
 
   # When we reassign an issue we should:
diff --git a/changelogs/unreleased/40871-todo-notification-count-shows-notification-without-having-a-todo.yml b/changelogs/unreleased/40871-todo-notification-count-shows-notification-without-having-a-todo.yml
new file mode 100644
index 00000000000..ee196629def
--- /dev/null
+++ b/changelogs/unreleased/40871-todo-notification-count-shows-notification-without-having-a-todo.yml
@@ -0,0 +1,5 @@
+---
+title: Reset todo counters when the target is deleted
+merge_request: 15807
+author:
+type: fixed
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index c5d08cb0b9d..a2ef937609b 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -874,7 +874,7 @@ describe Projects::IssuesController do
       end
 
       it 'delegates the update of the todos count cache to TodoService' do
-        expect_any_instance_of(TodoService).to receive(:destroy_issuable).with(issue, owner).once
+        expect_any_instance_of(TodoService).to receive(:destroy_target).with(issue).once
 
         delete :destroy, namespace_id: project.namespace, project_id: project, id: issue.iid
       end
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index d03ecefe47f..58116e6e0fe 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -468,7 +468,7 @@ describe Projects::MergeRequestsController do
       end
 
       it 'delegates the update of the todos count cache to TodoService' do
-        expect_any_instance_of(TodoService).to receive(:destroy_issuable).with(merge_request, owner).once
+        expect_any_instance_of(TodoService).to receive(:destroy_target).with(merge_request).once
 
         delete :destroy, namespace_id: project.namespace, project_id: project, id: merge_request.iid
       end
diff --git a/spec/services/issuable/destroy_service_spec.rb b/spec/services/issuable/destroy_service_spec.rb
index d74d98c6079..0a3647a814f 100644
--- a/spec/services/issuable/destroy_service_spec.rb
+++ b/spec/services/issuable/destroy_service_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
 
 describe Issuable::DestroyService do
   let(:user) { create(:user) }
-  let(:project) { create(:project) }
+  let(:project) { create(:project, :public) }
 
   subject(:service) { described_class.new(project, user) }
 
@@ -19,6 +19,13 @@ describe Issuable::DestroyService do
 
         service.execute(issue)
       end
+
+      it 'updates the todo caches for users with todos on the issue' do
+        create(:todo, target: issue, user: user, author: user, project: project)
+
+        expect { service.execute(issue) }
+          .to change { user.todos_pending_count }.from(1).to(0)
+      end
     end
 
     context 'when issuable is a merge request' do
@@ -33,6 +40,13 @@ describe Issuable::DestroyService do
 
         service.execute(merge_request)
       end
+
+      it 'updates the todo caches for users with todos on the merge request' do
+        create(:todo, target: merge_request, user: user, author: user, project: project)
+
+        expect { service.execute(merge_request) }
+          .to change { user.todos_pending_count }.from(1).to(0)
+      end
     end
   end
 end
diff --git a/spec/services/notes/destroy_service_spec.rb b/spec/services/notes/destroy_service_spec.rb
index c9a99a43edb..64445be560e 100644
--- a/spec/services/notes/destroy_service_spec.rb
+++ b/spec/services/notes/destroy_service_spec.rb
@@ -1,15 +1,25 @@
 require 'spec_helper'
 
 describe Notes::DestroyService do
+  set(:project) { create(:project, :public) }
+  set(:issue) { create(:issue, project: project) }
+  let(:user) { issue.author }
+
   describe '#execute' do
     it 'deletes a note' do
-      project = create(:project)
-      issue = create(:issue, project: project)
       note = create(:note, project: project, noteable: issue)
 
-      described_class.new(project, note.author).execute(note)
+      described_class.new(project, user).execute(note)
 
       expect(project.issues.find(issue.id).notes).not_to include(note)
     end
+
+    it 'updates the todo counts for users with todos for the note' do
+      note = create(:note, project: project, noteable: issue)
+      create(:todo, note: note, target: issue, user: user, author: user, project: project)
+
+      expect { described_class.new(project, user).execute(note) }
+        .to change { user.todos_pending_count }.from(1).to(0)
+    end
   end
 end
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index dc2673abc73..88013acae0a 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -248,11 +248,26 @@ describe TodoService do
       end
     end
 
-    describe '#destroy_issuable' do
-      it 'refresh the todos count cache for the user' do
-        expect(john_doe).to receive(:update_todos_count_cache).and_call_original
+    describe '#destroy_target' do
+      it 'refreshes the todos count cache for users with todos on the target' do
+        create(:todo, target: issue, user: john_doe, author: john_doe, project: issue.project)
 
-        service.destroy_issuable(issue, john_doe)
+        expect_any_instance_of(User).to receive(:update_todos_count_cache).and_call_original
+
+        service.destroy_target(issue) { }
+      end
+
+      it 'does not refresh the todos count cache for users with only done todos on the target' do
+        create(:todo, :done, target: issue, user: john_doe, author: john_doe, project: issue.project)
+
+        expect_any_instance_of(User).not_to receive(:update_todos_count_cache)
+
+        service.destroy_target(issue) { }
+      end
+
+      it 'yields the target to the caller' do
+        expect { |b| service.destroy_target(issue, &b) }
+          .to yield_with_args(issue)
       end
     end