Commit ef454f68 authored by Sean McGivern's avatar Sean McGivern

Reset todo counters when the target is deleted

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.
parent 9429e8ac
......@@ -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."
......
module Issuable
class DestroyService < IssuableBaseService
def execute(issuable)
TodoService.new.destroy_target(issuable) do |issuable|
if issuable.destroy
issuable.update_project_counter_caches
end
end
end
end
end
module Notes
class DestroyService < BaseService
def execute(note)
TodoService.new.destroy_target(note) do |note|
note.destroy
end
end
end
end
......@@ -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:
......
---
title: Reset todo counters when the target is deleted
merge_request: 15807
author:
type: fixed
......@@ -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
......
......@@ -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
......
......@@ -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
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
......@@ -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)
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
service.destroy_issuable(issue, john_doe)
it 'yields the target to the caller' do
expect { |b| service.destroy_target(issue, &b) }
.to yield_with_args(issue)
end
end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment