Commit c9041508 authored by Douwe Maan's avatar Douwe Maan

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
parents 34efcde7 ef454f68
...@@ -55,7 +55,6 @@ module IssuableActions ...@@ -55,7 +55,6 @@ module IssuableActions
def destroy def destroy
Issuable::DestroyService.new(issuable.project, current_user).execute(issuable) Issuable::DestroyService.new(issuable.project, current_user).execute(issuable)
TodoService.new.destroy_issuable(issuable, current_user)
name = issuable.human_class_name name = issuable.human_class_name
flash[:notice] = "The #{name} was successfully deleted." flash[:notice] = "The #{name} was successfully deleted."
......
module Issuable module Issuable
class DestroyService < IssuableBaseService class DestroyService < IssuableBaseService
def execute(issuable) def execute(issuable)
TodoService.new.destroy_target(issuable) do |issuable|
if issuable.destroy if issuable.destroy
issuable.update_project_counter_caches issuable.update_project_counter_caches
end end
end end
end end
end
end end
module Notes module Notes
class DestroyService < BaseService class DestroyService < BaseService
def execute(note) def execute(note)
TodoService.new.destroy_target(note) do |note|
note.destroy note.destroy
end end
end end
end
end end
...@@ -31,12 +31,20 @@ class TodoService ...@@ -31,12 +31,20 @@ class TodoService
mark_pending_todos_as_done(issue, current_user) mark_pending_todos_as_done(issue, current_user)
end 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) # This needs to yield back to the caller to destroy the target, because it
user.update_todos_count_cache # 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 end
# When we reassign an issue we should: # When we reassign an issue we should:
......
---
title: Reset todo counters when the target is deleted
merge_request: 15807
author:
type: fixed
...@@ -874,7 +874,7 @@ describe Projects::IssuesController do ...@@ -874,7 +874,7 @@ describe Projects::IssuesController do
end end
it 'delegates the update of the todos count cache to TodoService' do 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 delete :destroy, namespace_id: project.namespace, project_id: project, id: issue.iid
end end
......
...@@ -468,7 +468,7 @@ describe Projects::MergeRequestsController do ...@@ -468,7 +468,7 @@ describe Projects::MergeRequestsController do
end end
it 'delegates the update of the todos count cache to TodoService' do 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 delete :destroy, namespace_id: project.namespace, project_id: project, id: merge_request.iid
end end
......
...@@ -2,7 +2,7 @@ require 'spec_helper' ...@@ -2,7 +2,7 @@ require 'spec_helper'
describe Issuable::DestroyService do describe Issuable::DestroyService do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:project) { create(:project) } let(:project) { create(:project, :public) }
subject(:service) { described_class.new(project, user) } subject(:service) { described_class.new(project, user) }
...@@ -19,6 +19,13 @@ describe Issuable::DestroyService do ...@@ -19,6 +19,13 @@ describe Issuable::DestroyService do
service.execute(issue) service.execute(issue)
end 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 end
context 'when issuable is a merge request' do context 'when issuable is a merge request' do
...@@ -33,6 +40,13 @@ describe Issuable::DestroyService do ...@@ -33,6 +40,13 @@ describe Issuable::DestroyService do
service.execute(merge_request) service.execute(merge_request)
end 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 end
end end
require 'spec_helper' require 'spec_helper'
describe Notes::DestroyService do describe Notes::DestroyService do
set(:project) { create(:project, :public) }
set(:issue) { create(:issue, project: project) }
let(:user) { issue.author }
describe '#execute' do describe '#execute' do
it 'deletes a note' do it 'deletes a note' do
project = create(:project)
issue = create(:issue, project: project)
note = create(:note, project: project, noteable: issue) 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) expect(project.issues.find(issue.id).notes).not_to include(note)
end 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
end end
...@@ -248,11 +248,26 @@ describe TodoService do ...@@ -248,11 +248,26 @@ describe TodoService do
end end
end end
describe '#destroy_issuable' do describe '#destroy_target' do
it 'refresh the todos count cache for the user' do it 'refreshes the todos count cache for users with todos on the target' do
expect(john_doe).to receive(:update_todos_count_cache).and_call_original 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
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