Commit e6cf928e authored by Sarah Yasonik's avatar Sarah Yasonik Committed by Heinrich Lee Yu

Do not create additional alert todo for already-assigned user

parent d7239da8
...@@ -96,12 +96,12 @@ module AlertManagement ...@@ -96,12 +96,12 @@ module AlertManagement
end end
def handle_assignement(old_assignees) def handle_assignement(old_assignees)
assign_todo assign_todo(old_assignees)
add_assignee_system_note(old_assignees) add_assignee_system_note(old_assignees)
end end
def assign_todo def assign_todo(old_assignees)
todo_service.assign_alert(alert, current_user) todo_service.reassigned_assignable(alert, current_user, old_assignees)
end end
def add_assignee_system_note(old_assignees) def add_assignee_system_note(old_assignees)
......
...@@ -43,7 +43,7 @@ module Issues ...@@ -43,7 +43,7 @@ module Issues
if issue.assignees != old_assignees if issue.assignees != old_assignees
create_assignee_note(issue, old_assignees) create_assignee_note(issue, old_assignees)
notification_service.async.reassigned_issue(issue, current_user, old_assignees) notification_service.async.reassigned_issue(issue, current_user, old_assignees)
todo_service.reassigned_issuable(issue, current_user, old_assignees) todo_service.reassigned_assignable(issue, current_user, old_assignees)
end end
if issue.previous_changes.include?('confidential') if issue.previous_changes.include?('confidential')
......
...@@ -105,7 +105,7 @@ module MergeRequests ...@@ -105,7 +105,7 @@ module MergeRequests
def handle_assignees_change(merge_request, old_assignees) def handle_assignees_change(merge_request, old_assignees)
create_assignee_note(merge_request, old_assignees) create_assignee_note(merge_request, old_assignees)
notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees) notification_service.async.reassigned_merge_request(merge_request, current_user, old_assignees)
todo_service.reassigned_issuable(merge_request, current_user, old_assignees) todo_service.reassigned_assignable(merge_request, current_user, old_assignees)
end end
def create_branch_change_note(issuable, branch_type, old_branch, new_branch) def create_branch_change_note(issuable, branch_type, old_branch, new_branch)
......
...@@ -49,11 +49,11 @@ class TodoService ...@@ -49,11 +49,11 @@ class TodoService
todo_users.each(&:update_todos_count_cache) todo_users.each(&:update_todos_count_cache)
end end
# When we reassign an issuable we should: # When we reassign an assignable object (issuable, alert) we should:
# #
# * create a pending todo for new assignee if issuable is assigned # * create a pending todo for new assignee if object is assigned
# #
def reassigned_issuable(issuable, current_user, old_assignees = []) def reassigned_assignable(issuable, current_user, old_assignees = [])
create_assignment_todo(issuable, current_user, old_assignees) create_assignment_todo(issuable, current_user, old_assignees)
end end
...@@ -154,14 +154,6 @@ class TodoService ...@@ -154,14 +154,6 @@ class TodoService
resolve_todos_for_target(awardable, current_user) resolve_todos_for_target(awardable, current_user)
end end
# When assigning an alert we should:
#
# * create a pending todo for new assignee if alert is assigned
#
def assign_alert(alert, current_user)
create_assignment_todo(alert, current_user, [])
end
# When user marks a target as todo # When user marks a target as todo
def mark_todo(target, current_user) def mark_todo(target, current_user)
attributes = attributes_for_todo(target.project, target, current_user, Todo::MARKED) attributes = attributes_for_todo(target.project, target, current_user, Todo::MARKED)
......
...@@ -147,8 +147,7 @@ RSpec.describe AlertManagement::Alerts::UpdateService do ...@@ -147,8 +147,7 @@ RSpec.describe AlertManagement::Alerts::UpdateService do
end end
it_behaves_like 'does not add a system note' it_behaves_like 'does not add a system note'
# TODO: We should not add another todo in this scenario it_behaves_like 'does not add a todo'
it_behaves_like 'adds a todo'
end end
context 'with multiple users included' do context 'with multiple users included' do
......
...@@ -59,6 +59,10 @@ RSpec.describe TodoService do ...@@ -59,6 +59,10 @@ RSpec.describe TodoService do
should_not_create_todo(user: guest, target: addressed_target_assigned, action: Todo::DIRECTLY_ADDRESSED) should_not_create_todo(user: guest, target: addressed_target_assigned, action: Todo::DIRECTLY_ADDRESSED)
end end
it 'does not create a todo if already assigned' do
should_not_create_any_todo { service.send(described_method, target_assigned, author, [john_doe]) }
end
end end
describe 'Issues' do describe 'Issues' do
...@@ -573,10 +577,10 @@ RSpec.describe TodoService do ...@@ -573,10 +577,10 @@ RSpec.describe TodoService do
end end
end end
describe '#reassigned_issuable' do describe '#reassigned_assignable' do
let(:described_method) { :reassigned_issuable } let(:described_method) { :reassigned_assignable }
context 'issuable is a merge request' do context 'assignable is a merge request' do
it_behaves_like 'reassigned target' do it_behaves_like 'reassigned target' do
let(:target_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } let(:target_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") }
let(:addressed_target_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } let(:addressed_target_assigned) { create(:merge_request, source_project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") }
...@@ -584,13 +588,21 @@ RSpec.describe TodoService do ...@@ -584,13 +588,21 @@ RSpec.describe TodoService do
end end
end end
context 'issuable is an issue' do context 'assignable is an issue' do
it_behaves_like 'reassigned target' do it_behaves_like 'reassigned target' do
let(:target_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") } let(:target_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "- [ ] Task 1\n- [ ] Task 2 #{mentions}") }
let(:addressed_target_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") } let(:addressed_target_assigned) { create(:issue, project: project, author: author, assignees: [john_doe], description: "#{directly_addressed}\n- [ ] Task 1\n- [ ] Task 2") }
let(:target_unassigned) { create(:issue, project: project, author: author, assignees: []) } let(:target_unassigned) { create(:issue, project: project, author: author, assignees: []) }
end end
end end
context 'assignable is an alert' do
it_behaves_like 'reassigned target' do
let(:target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) }
let(:addressed_target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) }
let(:target_unassigned) { create(:alert_management_alert, project: project, assignees: []) }
end
end
end end
describe 'Merge Requests' do describe 'Merge Requests' do
...@@ -778,16 +790,6 @@ RSpec.describe TodoService do ...@@ -778,16 +790,6 @@ RSpec.describe TodoService do
end end
end end
describe '#assign_alert' do
let(:described_method) { :assign_alert }
it_behaves_like 'reassigned target' do
let(:target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) }
let(:addressed_target_assigned) { create(:alert_management_alert, project: project, assignees: [john_doe]) }
let(:target_unassigned) { create(:alert_management_alert, project: project, assignees: []) }
end
end
describe '#merge_request_build_failed' do describe '#merge_request_build_failed' do
let(:merge_participants) { [mr_unassigned.author, admin] } let(:merge_participants) { [mr_unassigned.author, admin] }
......
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