Commit a74a6cfd authored by Jarka Kadlecova's avatar Jarka Kadlecova

Create todos only for new mentions

parent 76a15db4
......@@ -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')
......
......@@ -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')
......
......@@ -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
......
......@@ -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
......
---
title: Create todos only for new mentions
merge_request:
author:
......@@ -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
......
......@@ -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
......
......@@ -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
......
This diff is collapsed.
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