Reset todo counters when the target is deleted40871-todo-notification-count-shows-notification-without-having-a-todo

When the target is deleted, todos are destroyed, but we did not reset the todo
cache for users with todos on the deleted target. This would only update after
the next time the todo cache was updated for that user.

10 files changed, 73 insertions, 18 deletions

diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb
index 744e448e8df..141c34ee5ee 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 4dbbaecdd6d..b7447f654c4 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -861,7 +861,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 51d5d6a52b3..97a5018d714 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