diff options
author | Sean McGivern <sean@gitlab.com> | 2017-12-08 12:17:22 +0000 |
---|---|---|
committer | Sean McGivern <sean@gitlab.com> | 2017-12-18 12:23:00 +0000 |
commit | ef454f68e837e4e7360fe1518686dd56adbbb0a9 (patch) | |
tree | 72e67bbf1a2222d8a1007298478350b5cc9c02bd | |
parent | 9429e8ac60a10436a0469d7d206d3f74a2c966c7 (diff) | |
download | gitlab-ce-ef454f68e837e4e7360fe1518686dd56adbbb0a9.tar.gz |
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.
-rw-r--r-- | app/controllers/concerns/issuable_actions.rb | 1 | ||||
-rw-r--r-- | app/services/issuable/destroy_service.rb | 6 | ||||
-rw-r--r-- | app/services/notes/destroy_service.rb | 4 | ||||
-rw-r--r-- | app/services/todo_service.rb | 16 | ||||
-rw-r--r-- | changelogs/unreleased/40871-todo-notification-count-shows-notification-without-having-a-todo.yml | 5 | ||||
-rw-r--r-- | spec/controllers/projects/issues_controller_spec.rb | 2 | ||||
-rw-r--r-- | spec/controllers/projects/merge_requests_controller_spec.rb | 2 | ||||
-rw-r--r-- | spec/services/issuable/destroy_service_spec.rb | 16 | ||||
-rw-r--r-- | spec/services/notes/destroy_service_spec.rb | 16 | ||||
-rw-r--r-- | spec/services/todo_service_spec.rb | 23 |
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 |