diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index a444c78b609..b7fe5cb168b 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -19,7 +19,7 @@ module Issues if issue.previous_changes.include?('title') || issue.previous_changes.include?('description') - todo_service.update_issue(issue, current_user) + todo_service.update_issue(issue, current_user, old_mentioned_users) end if issue.previous_changes.include?('milestone_id') diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 3cb9aae83f6..ab7fcf3b6e2 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -28,7 +28,7 @@ module MergeRequests if merge_request.previous_changes.include?('title') || merge_request.previous_changes.include?('description') - todo_service.update_merge_request(merge_request, current_user) + todo_service.update_merge_request(merge_request, current_user, old_mentioned_users) end if merge_request.previous_changes.include?('target_branch') diff --git a/app/services/notes/update_service.rb b/app/services/notes/update_service.rb index 75a4b3ed826..75fd08ea0a9 100644 --- a/app/services/notes/update_service.rb +++ b/app/services/notes/update_service.rb @@ -3,11 +3,13 @@ module Notes def execute(note) return note unless note.editable? + old_mentioned_users = note.mentioned_users.to_a + note.update_attributes(params.merge(updated_by: current_user)) note.create_new_cross_references!(current_user) if note.previous_changes.include?('note') - TodoService.new.update_note(note, current_user) + TodoService.new.update_note(note, current_user, old_mentioned_users) end note diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index bf7e76ec59e..2c56cb4c680 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -19,8 +19,8 @@ class TodoService # # * mark all pending todos related to the issue for the current user as done # - def update_issue(issue, current_user) - update_issuable(issue, current_user) + def update_issue(issue, current_user, skip_users = []) + update_issuable(issue, current_user, skip_users) end # When close an issue we should: @@ -60,8 +60,8 @@ class TodoService # # * create a todo for each mentioned user on merge request # - def update_merge_request(merge_request, current_user) - update_issuable(merge_request, current_user) + def update_merge_request(merge_request, current_user, skip_users = []) + update_issuable(merge_request, current_user, skip_users) end # When close a merge request we should: @@ -123,7 +123,7 @@ class TodoService mark_pending_todos_as_done(merge_request, merge_request.author) mark_pending_todos_as_done(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds? end - + # When a merge request could not be automatically merged due to its unmergeable state we should: # # * create a todo for a merge_user @@ -131,7 +131,7 @@ class TodoService def merge_request_became_unmergeable(merge_request) create_unmergeable_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds? end - + # When create a note we should: # # * mark all pending todos related to the noteable for the note author as done @@ -146,8 +146,8 @@ class TodoService # * mark all pending todos related to the noteable for the current user as done # * create a todo for each new user mentioned on note # - def update_note(note, current_user) - handle_note(note, current_user) + def update_note(note, current_user, skip_users = []) + handle_note(note, current_user, skip_users) end # When an emoji is awarded we should: @@ -223,11 +223,11 @@ class TodoService create_mention_todos(issuable.project, issuable, author) end - def update_issuable(issuable, author) + def update_issuable(issuable, author, skip_users = []) # Skip toggling a task list item in a description return if toggling_tasks?(issuable) - create_mention_todos(issuable.project, issuable, author) + create_mention_todos(issuable.project, issuable, author, nil, skip_users) end def destroy_issuable(issuable, user) @@ -239,7 +239,7 @@ class TodoService issuable.tasks? && issuable.updated_tasks.any? end - def handle_note(note, author) + def handle_note(note, author, skip_users = []) # Skip system notes, and notes on project snippet return if note.system? || note.for_snippet? @@ -247,7 +247,7 @@ class TodoService target = note.noteable mark_pending_todos_as_done(target, author) - create_mention_todos(project, target, author, note) + create_mention_todos(project, target, author, note, skip_users) end def create_assignment_todo(issuable, author) @@ -257,14 +257,14 @@ class TodoService end end - def create_mention_todos(project, target, author, note = nil) + def create_mention_todos(project, target, author, note = nil, skip_users = []) # Create Todos for directly addressed users - directly_addressed_users = filter_directly_addressed_users(project, note || target, author) + directly_addressed_users = filter_directly_addressed_users(project, note || target, author, skip_users) attributes = attributes_for_todo(project, target, author, Todo::DIRECTLY_ADDRESSED, note) create_todos(directly_addressed_users, attributes) # Create Todos for mentioned users - mentioned_users = filter_mentioned_users(project, note || target, author) + mentioned_users = filter_mentioned_users(project, note || target, author, skip_users) attributes = attributes_for_todo(project, target, author, Todo::MENTIONED, note) create_todos(mentioned_users, attributes) end @@ -307,13 +307,13 @@ class TodoService reject_users_without_access(users, project, target).uniq end - def filter_mentioned_users(project, target, author) - mentioned_users = target.mentioned_users(author) + def filter_mentioned_users(project, target, author, skip_users = []) + mentioned_users = target.mentioned_users(author) - skip_users filter_todo_users(mentioned_users, project, target) end - def filter_directly_addressed_users(project, target, author) - directly_addressed_users = target.directly_addressed_users(author) + def filter_directly_addressed_users(project, target, author, skip_users = []) + directly_addressed_users = target.directly_addressed_users(author) - skip_users filter_todo_users(directly_addressed_users, project, target) end diff --git a/changelogs/unreleased/28799-todo-creation.yml b/changelogs/unreleased/28799-todo-creation.yml new file mode 100644 index 00000000000..c6e05468568 --- /dev/null +++ b/changelogs/unreleased/28799-todo-creation.yml @@ -0,0 +1,4 @@ +--- +title: Create todos only for new mentions +merge_request: +author: diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index fa472f3e2c3..5b324f3c706 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -13,6 +13,7 @@ describe Issues::UpdateService, services: true do let(:issue) do create(:issue, title: 'Old title', + description: "for #{user2.to_reference}", assignee_id: user3.id, project: project) end @@ -182,16 +183,24 @@ describe Issues::UpdateService, services: true do it 'marks pending todos as done' do expect(todo.reload.done?).to eq true end + + it 'does not create any new todos' do + expect(Todo.count).to eq(1) + end end context 'when the description change' do before do - update_issue(description: 'Also please fix') + update_issue(description: "Also please fix #{user2.to_reference} #{user3.to_reference}") end it 'marks todos as done' do expect(todo.reload.done?).to eq true end + + it 'creates only 1 new todo' do + expect(Todo.count).to eq(2) + end end context 'when is reassigned' do diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index ad3d767f193..f2ca1e6fcbd 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -12,6 +12,7 @@ describe MergeRequests::UpdateService, services: true do let(:merge_request) do create(:merge_request, :simple, title: 'Old title', + description: "FYI #{user2.to_reference}", assignee_id: user3.id, source_project: project) end @@ -225,16 +226,24 @@ describe MergeRequests::UpdateService, services: true do it 'marks pending todos as done' do expect(pending_todo.reload).to be_done end + + it 'does not create any new todos' do + expect(Todo.count).to eq(1) + end end context 'when the description change' do before do - update_merge_request({ description: 'Also please fix' }) + update_merge_request({ description: "Also please fix #{user2.to_reference} #{user3.to_reference}" }) end it 'marks pending todos as done' do expect(pending_todo.reload).to be_done end + + it 'creates only 1 new todo' do + expect(Todo.count).to eq(2) + end end context 'when is reassigned' do diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index dde4bde7dc2..905e2f46bde 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -4,12 +4,14 @@ describe Notes::UpdateService, services: true do let(:project) { create(:empty_project) } let(:user) { create(:user) } let(:user2) { create(:user) } + let(:user3) { create(:user) } let(:issue) { create(:issue, project: project) } - let(:note) { create(:note, project: project, noteable: issue, author: user, note: 'Old note') } + let(:note) { create(:note, project: project, noteable: issue, author: user, note: "Old note #{user2.to_reference}") } before do project.team << [user, :master] project.team << [user2, :developer] + project.team << [user3, :developer] end describe '#execute' do @@ -23,22 +25,30 @@ describe Notes::UpdateService, services: true do context 'when the note change' do before do - update_note({ note: 'New note' }) + update_note({ note: "New note #{user2.to_reference} #{user3.to_reference}" }) end it 'marks todos as done' do expect(todo.reload).to be_done end + + it 'creates only 1 new todo' do + expect(Todo.count).to eq(2) + end end context 'when the note does not change' do before do - update_note({ note: 'Old note' }) + update_note({ note: "Old note #{user2.to_reference}" }) end it 'keep todos' do expect(todo.reload).to be_pending end + + it 'does not create any new todos' do + expect(Todo.count).to eq(1) + end end end end diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index f9e432bb216..89b3b6aad10 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -8,10 +8,12 @@ describe TodoService, services: true do let(:guest) { create(:user) } let(:admin) { create(:admin) } let(:john_doe) { create(:user) } + let(:skipped) { create(:user) } + let(:skip_users) { [skipped] } let(:project) { create(:empty_project) } - let(:mentions) { 'FYI: ' + [author, assignee, john_doe, member, guest, non_member, admin].map(&:to_reference).join(' ') } - let(:directly_addressed) { [author, assignee, john_doe, member, guest, non_member, admin].map(&:to_reference).join(' ') } - let(:directly_addressed_and_mentioned) { member.to_reference + ", what do you think? cc: " + [guest, admin].map(&:to_reference).join(' ') } + let(:mentions) { 'FYI: ' + [author, assignee, john_doe, member, guest, non_member, admin, skipped].map(&:to_reference).join(' ') } + let(:directly_addressed) { [author, assignee, john_doe, member, guest, non_member, admin, skipped].map(&:to_reference).join(' ') } + let(:directly_addressed_and_mentioned) { member.to_reference + ", what do you think? cc: " + [guest, admin, skipped].map(&:to_reference).join(' ') } let(:service) { described_class.new } before do @@ -19,6 +21,7 @@ describe TodoService, services: true do project.team << [author, :developer] project.team << [member, :developer] project.team << [john_doe, :developer] + project.team << [skipped, :developer] end describe 'Issues' do @@ -119,46 +122,61 @@ describe TodoService, services: true do end describe '#update_issue' do - it 'creates a todo for each valid mentioned user' do - service.update_issue(issue, author) + it 'creates a todo for each valid mentioned user not included in skip_users' do + service.update_issue(issue, author, skip_users) should_create_todo(user: member, target: issue, action: Todo::MENTIONED) should_create_todo(user: guest, target: issue, action: Todo::MENTIONED) should_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED) should_create_todo(user: author, target: issue, action: Todo::MENTIONED) should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: skipped, target: issue, action: Todo::MENTIONED) end - it 'creates a todo for each valid user based on the type of mention' do + it 'creates a todo for each valid user not included in skip_users based on the type of mention' do issue.update(description: directly_addressed_and_mentioned) - service.update_issue(issue, author) + service.update_issue(issue, author, skip_users) should_create_todo(user: member, target: issue, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: guest, target: issue, action: Todo::MENTIONED) should_create_todo(user: admin, target: issue, action: Todo::MENTIONED) + should_not_create_todo(user: skipped, target: issue) end - it 'creates a directly addressed todo for each valid addressed user' do - service.update_issue(addressed_issue, author) + it 'creates a directly addressed todo for each valid addressed user not included in skip_users' do + service.update_issue(addressed_issue, author, skip_users) should_create_todo(user: member, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: guest, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: john_doe, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: author, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED) should_not_create_todo(user: non_member, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED) + should_not_create_todo(user: skipped, target: addressed_issue, action: Todo::DIRECTLY_ADDRESSED) end - it 'does not create a todo if user was already mentioned' do + it 'does not create a todo if user was already mentioned and todo is pending' do create(:todo, :mentioned, user: member, project: project, target: issue, author: author) - expect { service.update_issue(issue, author) }.not_to change(member.todos, :count) + expect { service.update_issue(issue, author, skip_users) }.not_to change(member.todos, :count) end - it 'does not create a directly addressed todo if user was already mentioned or addressed' do + it 'does not create a todo if user was already mentioned and todo is done' do + create(:todo, :mentioned, :done, user: skipped, project: project, target: issue, author: author) + + expect { service.update_issue(issue, author, skip_users) }.not_to change(skipped.todos, :count) + end + + it 'does not create a directly addressed todo if user was already mentioned or addressed and todo is pending' do create(:todo, :directly_addressed, user: member, project: project, target: addressed_issue, author: author) - expect { service.update_issue(addressed_issue, author) }.not_to change(member.todos, :count) + expect { service.update_issue(addressed_issue, author, skip_users) }.not_to change(member.todos, :count) + end + + it 'does not create a directly addressed todo if user was already mentioned or addressed and todo is done' do + create(:todo, :directly_addressed, :done, user: skipped, project: project, target: addressed_issue, author: author) + + expect { service.update_issue(addressed_issue, author, skip_users) }.not_to change(skipped.todos, :count) end it 'does not create todo if user can not see the issue when issue is confidential' do @@ -521,47 +539,62 @@ describe TodoService, services: true do end describe '#update_merge_request' do - it 'creates a todo for each valid mentioned user' do - service.update_merge_request(mr_assigned, author) + it 'creates a todo for each valid mentioned user not included in skip_users' do + service.update_merge_request(mr_assigned, author, skip_users) should_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED) should_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED) should_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: skipped, target: mr_assigned, action: Todo::MENTIONED) end - it 'creates a todo for each valid user based on the type of mention' do + it 'creates a todo for each valid user not included in skip_users based on the type of mention' do mr_assigned.update(description: directly_addressed_and_mentioned) - service.update_merge_request(mr_assigned, author) + service.update_merge_request(mr_assigned, author, skip_users) should_create_todo(user: member, target: mr_assigned, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: admin, target: mr_assigned, action: Todo::MENTIONED) + should_not_create_todo(user: skipped, target: mr_assigned) end - it 'creates a directly addressed todo for each valid addressed user' do - service.update_merge_request(addressed_mr_assigned, author) + it 'creates a directly addressed todo for each valid addressed user not included in skip_users' do + service.update_merge_request(addressed_mr_assigned, author, skip_users) should_create_todo(user: member, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED) should_not_create_todo(user: guest, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: john_doe, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED) should_create_todo(user: author, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED) should_not_create_todo(user: non_member, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED) + should_not_create_todo(user: skipped, target: addressed_mr_assigned, action: Todo::DIRECTLY_ADDRESSED) end - it 'does not create a todo if user was already mentioned' do + it 'does not create a todo if user was already mentioned and todo is pending' do create(:todo, :mentioned, user: member, project: project, target: mr_assigned, author: author) expect { service.update_merge_request(mr_assigned, author) }.not_to change(member.todos, :count) end - it 'does not create a directly addressed todo if user was already mentioned or addressed' do + it 'does not create a todo if user was already mentioned and todo is done' do + create(:todo, :mentioned, :done, user: skipped, project: project, target: mr_assigned, author: author) + + expect { service.update_merge_request(mr_assigned, author, skip_users) }.not_to change(skipped.todos, :count) + end + + it 'does not create a directly addressed todo if user was already mentioned or addressed and todo is pending' do create(:todo, :directly_addressed, user: member, project: project, target: addressed_mr_assigned, author: author) expect{ service.update_merge_request(addressed_mr_assigned, author) }.not_to change(member.todos, :count) end + it 'does not create a directly addressed todo if user was already mentioned or addressed and todo is done' do + create(:todo, :directly_addressed, user: skipped, project: project, target: addressed_mr_assigned, author: author) + + expect{ service.update_merge_request(addressed_mr_assigned, author, skip_users) }.not_to change(skipped.todos, :count) + end + context 'with a task list' do it 'does not create todo when tasks are marked as completed' do mr_assigned.update(description: "- [x] Task 1\n- [X] Task 2 #{mentions}") @@ -757,6 +790,69 @@ describe TodoService, services: true do end end + describe '#update_note' do + let(:noteable) { create(:issue, project: project) } + let(:note) { create(:note, project: project, note: mentions, noteable: noteable) } + let(:addressed_note) { create(:note, project: project, note: "#{directly_addressed}", noteable: noteable) } + + it 'creates a todo for each valid mentioned user not included in skip_users' do + service.update_note(note, author, skip_users) + + should_create_todo(user: member, target: noteable, action: Todo::MENTIONED) + should_create_todo(user: guest, target: noteable, action: Todo::MENTIONED) + should_create_todo(user: john_doe, target: noteable, action: Todo::MENTIONED) + should_create_todo(user: author, target: noteable, action: Todo::MENTIONED) + should_not_create_todo(user: non_member, target: noteable, action: Todo::MENTIONED) + should_not_create_todo(user: skipped, target: noteable, action: Todo::MENTIONED) + end + + it 'creates a todo for each valid user not included in skip_users based on the type of mention' do + note.update(note: directly_addressed_and_mentioned) + + service.update_note(note, author, skip_users) + + should_create_todo(user: member, target: noteable, action: Todo::DIRECTLY_ADDRESSED) + should_create_todo(user: guest, target: noteable, action: Todo::MENTIONED) + should_create_todo(user: admin, target: noteable, action: Todo::MENTIONED) + should_not_create_todo(user: skipped, target: noteable) + end + + it 'creates a directly addressed todo for each valid addressed user not included in skip_users' do + service.update_note(addressed_note, author, skip_users) + + should_create_todo(user: member, target: noteable, action: Todo::DIRECTLY_ADDRESSED) + should_create_todo(user: guest, target: noteable, action: Todo::DIRECTLY_ADDRESSED) + should_create_todo(user: john_doe, target: noteable, action: Todo::DIRECTLY_ADDRESSED) + should_create_todo(user: author, target: noteable, action: Todo::DIRECTLY_ADDRESSED) + should_not_create_todo(user: non_member, target: noteable, action: Todo::DIRECTLY_ADDRESSED) + should_not_create_todo(user: skipped, target: noteable, action: Todo::DIRECTLY_ADDRESSED) + end + + it 'does not create a todo if user was already mentioned and todo is pending' do + create(:todo, :mentioned, user: member, project: project, target: noteable, author: author) + + expect { service.update_note(note, author, skip_users) }.not_to change(member.todos, :count) + end + + it 'does not create a todo if user was already mentioned and todo is done' do + create(:todo, :mentioned, :done, user: skipped, project: project, target: noteable, author: author) + + expect { service.update_note(note, author, skip_users) }.not_to change(skipped.todos, :count) + end + + it 'does not create a directly addressed todo if user was already mentioned or addressed and todo is pending' do + create(:todo, :directly_addressed, user: member, project: project, target: noteable, author: author) + + expect { service.update_note(addressed_note, author, skip_users) }.not_to change(member.todos, :count) + end + + it 'does not create a directly addressed todo if user was already mentioned or addressed and todo is done' do + create(:todo, :directly_addressed, :done, user: skipped, project: project, target: noteable, author: author) + + expect { service.update_note(addressed_note, author, skip_users) }.not_to change(skipped.todos, :count) + end + end + it 'updates cached counts when a todo is created' do issue = create(:issue, project: project, assignee: john_doe, author: author, description: mentions)