From 3b98adcc75f82f4e5e469da5c164467da02b0f0c Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 17 Feb 2016 20:04:14 -0200 Subject: [PATCH] Create a pending task when a user is mentioned when edit a issue/mr/note --- app/services/issues/update_service.rb | 2 +- app/services/merge_requests/update_service.rb | 2 +- app/services/task_service.rb | 79 ++++++++-- spec/factories/tasks.rb | 8 +- spec/services/issues/close_service_spec.rb | 4 +- spec/services/issues/update_service_spec.rb | 4 +- .../merge_requests/close_service_spec.rb | 4 +- .../merge_requests/update_service_spec.rb | 4 +- spec/services/notes/update_service_spec.rb | 6 +- spec/services/task_service_spec.rb | 145 +++++++++--------- 10 files changed, 152 insertions(+), 106 deletions(-) diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 9e39847dc54..042b567b25f 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -6,7 +6,7 @@ module Issues def handle_changes(issue, options = {}) if have_changes?(issue, options) - task_service.mark_pending_tasks_as_done(issue, current_user) + task_service.update_issue(issue, current_user) 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 38ae067877a..87949f0a9b8 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -16,7 +16,7 @@ module MergeRequests def handle_changes(merge_request, options = {}) if have_changes?(merge_request, options) - task_service.mark_pending_tasks_as_done(merge_request, current_user) + task_service.update_merge_request(merge_request, current_user) end if merge_request.previous_changes.include?('target_branch') diff --git a/app/services/task_service.rb b/app/services/task_service.rb index c9b30bcb19c..48678763f31 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -8,12 +8,22 @@ class TaskService # When create an issue we should: # - # * creates a pending task for assignee if issue is assigned + # * create a task for assignee if issue is assigned + # * create a task for each mentioned user on issue # def new_issue(issue, current_user) new_issuable(issue, current_user) end + # When update an issue we should: + # + # * mark all pending tasks related to the issue for the current user as done + # * create a task for each new user mentioned on issue + # + def update_issue(issue, current_user) + update_issuable(issue, current_user) + end + # When close an issue we should: # # * mark all pending tasks related to the target for the current user as done @@ -24,7 +34,7 @@ class TaskService # When we reassign an issue we should: # - # * creates a pending task for new assignee if issue is assigned + # * create a pending task for new assignee if issue is assigned # def reassigned_issue(issue, current_user) reassigned_issuable(issue, current_user) @@ -33,11 +43,21 @@ class TaskService # When create a merge request we should: # # * creates a pending task for assignee if merge request is assigned + # * create a task for each mentioned user on merge request # def new_merge_request(merge_request, current_user) new_issuable(merge_request, current_user) end + # When update a merge request we should: + # + # * mark all pending tasks related to the merge request for the current user as done + # * create a task for each new user mentioned on merge request + # + def update_merge_request(merge_request, current_user) + update_issuable(merge_request, current_user) + end + # When close a merge request we should: # # * mark all pending tasks related to the target for the current user as done @@ -62,17 +82,10 @@ class TaskService mark_pending_tasks_as_done(merge_request, current_user) end - # When we mark a task as done we should: - # - # * mark all pending tasks related to the target for the user as done - # - def mark_pending_tasks_as_done(target, user) - pending_tasks(user, target.project, target).update_all(state: :done) - end - # When create a note we should: # # * mark all pending tasks related to the noteable for the note author as done + # * create a task for each mentioned user on note # def new_note(note) # Skip system notes, like status changes and cross-references @@ -94,11 +107,24 @@ class TaskService # When update a note we should: # # * mark all pending tasks related to the noteable for the current user as done + # * create a task for each new user mentioned on note # def update_note(note, current_user) # Skip system notes, like status changes and cross-references unless note.system - mark_pending_tasks_as_done(note.noteable, current_user) + project = note.project + target = note.noteable + author = current_user + + mark_pending_tasks_as_done(target, author) + + mentioned_users = build_mentioned_users(project, note, author) + + mentioned_users.each do |user| + unless pending_tasks(mentioned_user, project, target, note: note, action: Task::MENTIONED).exists? + create_task(project, target, author, user, Task::MENTIONED, note) + end + end end end @@ -128,8 +154,17 @@ class TaskService mentioned_users.uniq end - def pending_tasks(user, project, target) - user.tasks.pending.where(project: project, target: target) + def mark_pending_tasks_as_done(target, user) + pending_tasks(user, target.project, target).update_all(state: :done) + end + + def pending_tasks(user, project, target, options = {}) + options.reverse_merge({ + project: project, + target: target + }) + + user.tasks.pending.where(options) end def new_issuable(issuable, current_user) @@ -148,8 +183,24 @@ class TaskService end end + def update_issuable(issuable, current_user) + project = issuable.project + target = issuable + author = current_user + + mark_pending_tasks_as_done(target, author) + + mentioned_users = build_mentioned_users(project, target, author) + + mentioned_users.each do |mentioned_user| + unless pending_tasks(mentioned_user, project, target, action: Task::MENTIONED).exists? + create_task(project, target, author, mentioned_user, Task::MENTIONED) + end + end + end + def reassigned_issuable(issuable, current_user) - if issuable.is_assigned? && issuable.assignee != current_user + if issuable.assignee && issuable.assignee != current_user create_task(issuable.project, issuable, current_user, issuable.assignee, Task::ASSIGNED) end end diff --git a/spec/factories/tasks.rb b/spec/factories/tasks.rb index 710b8324f1f..b31db8a7d8b 100644 --- a/spec/factories/tasks.rb +++ b/spec/factories/tasks.rb @@ -20,14 +20,12 @@ FactoryGirl.define do author user - factory :pending_assigned_task, traits: [:assgined, :pending] - - trait :assgined do + trait :assigned do action { Task::ASSIGNED } end - trait :pending do - state { :pending } + trait :mentioned do + action { Task::MENTIONED } end end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index 7a587a96ddc..fdf0fd819b2 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -5,9 +5,7 @@ describe Issues::CloseService, services: true do let(:user2) { create(:user) } let(:issue) { create(:issue, assignee: user2) } let(:project) { issue.project } - let!(:pending_task) do - create(:pending_assigned_task, user: user, project: project, target: issue, author: user2) - end + let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: issue, author: user2) } before do project.team << [user, :master] diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 1b7d5b45168..5674b9f1e9b 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -81,9 +81,7 @@ describe Issues::UpdateService, services: true do end context 'task queue' do - let!(:pending_task) do - create(:pending_assigned_task, user: user, project: project, target: issue, author: user2) - end + let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: issue, author: user2) } context 'when the title change' do before do diff --git a/spec/services/merge_requests/close_service_spec.rb b/spec/services/merge_requests/close_service_spec.rb index 3e72be2dc25..13872a1a2dd 100644 --- a/spec/services/merge_requests/close_service_spec.rb +++ b/spec/services/merge_requests/close_service_spec.rb @@ -5,9 +5,7 @@ describe MergeRequests::CloseService, services: true do let(:user2) { create(:user) } let(:merge_request) { create(:merge_request, assignee: user2) } let(:project) { merge_request.project } - let!(:pending_task) do - create(:pending_assigned_task, user: user, project: project, target: merge_request, author: user2) - end + let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: merge_request, author: user2) } before do project.team << [user, :master] diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb index 77c661fd293..6d70e8ec16a 100644 --- a/spec/services/merge_requests/update_service_spec.rb +++ b/spec/services/merge_requests/update_service_spec.rb @@ -99,9 +99,7 @@ describe MergeRequests::UpdateService, services: true do end context 'task queue' do - let!(:pending_task) do - create(:pending_assigned_task, user: user, project: project, target: merge_request, author: user2) - end + let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: merge_request, author: user2) } context 'when the title change' do before do diff --git a/spec/services/notes/update_service_spec.rb b/spec/services/notes/update_service_spec.rb index e6670143951..a8b3e0d825d 100644 --- a/spec/services/notes/update_service_spec.rb +++ b/spec/services/notes/update_service_spec.rb @@ -19,7 +19,7 @@ describe Notes::UpdateService, services: true do end context 'task queue' do - let!(:task) { create(:pending_assigned_task, user: user, project: project, target: issue, author: user2) } + let!(:pending_task) { create(:task, :assigned, user: user, project: project, target: issue, author: user2) } context 'when the note change' do before do @@ -27,7 +27,7 @@ describe Notes::UpdateService, services: true do end it 'marks pending tasks as done' do - expect(task.reload).to be_done + expect(pending_task.reload).to be_done end end @@ -37,7 +37,7 @@ describe Notes::UpdateService, services: true do end it 'keep pending tasks' do - expect(task.reload).to be_pending + expect(pending_task.reload).to be_pending end end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index 18ad02dd2df..a5a497c7002 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -16,7 +16,7 @@ describe TaskService, services: true do end describe 'Issues' do - let(:issue) { create(:issue, project: project, assignee: john_doe, author: author) } + let(:issue) { create(:issue, project: project, assignee: john_doe, author: author, description: mentions) } let(:unassigned_issue) { create(:issue, project: project, assignee: nil) } describe '#new_issue' do @@ -35,8 +35,6 @@ describe TaskService, services: true do end it 'creates a task for each valid mentioned user' do - issue.update_attribute(:description, mentions) - service.new_issue(issue, author) should_create_task(user: michael, target: issue, action: Task::MENTIONED) @@ -46,20 +44,39 @@ describe TaskService, services: true do end end + describe '#update_issue' do + it 'marks pending tasks to the issue for the user as done' do + pending_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + service.update_issue(issue, john_doe) + + expect(pending_task.reload).to be_done + end + + it 'creates a task for each valid mentioned user' do + service.update_issue(issue, author) + + should_create_task(user: michael, target: issue, action: Task::MENTIONED) + should_not_create_task(user: author, target: issue, action: Task::MENTIONED) + should_not_create_task(user: john_doe, target: issue, action: Task::MENTIONED) + should_not_create_task(user: stranger, target: issue, action: Task::MENTIONED) + end + + it 'does not create a task if user was already mentioned' do + create(:task, :mentioned, user: michael, project: project, target: issue, author: author) + + should_not_create_any_task { service.update_issue(issue, author) } + end + end + describe '#close_issue' do - let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) - end - - let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) - end - it 'marks related pending tasks to the target for the user as done' do + first_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + second_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + service.close_issue(issue, john_doe) - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done end end @@ -84,60 +101,38 @@ describe TaskService, services: true do end end - describe '#mark_pending_tasks_as_done' do - let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) - end - - let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) - end - - it 'marks related pending tasks to the target for the user as done' do - service.mark_pending_tasks_as_done(issue, john_doe) - - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done - end - end - describe '#new_note' do - let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) - end - - let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author) - end - - let(:note) { create(:note, project: project, noteable: issue, author: john_doe) } + let!(:first_task) { create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) } + let!(:second_task) { create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) } + let(:note) { create(:note, project: project, noteable: issue, author: john_doe, note: mentions) } let(:award_note) { create(:note, :award, project: project, noteable: issue, author: john_doe, note: 'thumbsup') } let(:system_note) { create(:system_note, project: project, noteable: issue) } it 'mark related pending tasks to the noteable for the note author as done' do + first_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + second_task = create(:task, :assigned, user: john_doe, project: project, target: issue, author: author) + service.new_note(note) - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done end it 'mark related pending tasks to the noteable for the award note author as done' do service.new_note(award_note) - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done end it 'does not mark related pending tasks it is a system note' do service.new_note(system_note) - expect(first_pending_task.reload).to be_pending - expect(second_pending_task.reload).to be_pending + expect(first_task.reload).to be_pending + expect(second_task.reload).to be_pending end it 'creates a task for each valid mentioned user' do - note.update_attribute(:note, mentions) - service.new_note(note) should_create_task(user: michael, target: issue, author: john_doe, action: Task::MENTIONED, note: note) @@ -149,7 +144,7 @@ describe TaskService, services: true do end describe 'Merge Requests' do - let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe) } + let(:mr_assigned) { create(:merge_request, source_project: project, author: author, assignee: john_doe, description: mentions) } let(:mr_unassigned) { create(:merge_request, source_project: project, author: author, assignee: nil) } describe '#new_merge_request' do @@ -168,8 +163,6 @@ describe TaskService, services: true do end it 'creates a task for each valid mentioned user' do - mr_assigned.update_attribute(:description, mentions) - service.new_merge_request(mr_assigned, author) should_create_task(user: michael, target: mr_assigned, action: Task::MENTIONED) @@ -179,20 +172,38 @@ describe TaskService, services: true do end end + describe '#update_merge_request' do + it 'marks pending tasks to the merge request for the user as done' do + pending_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) + service.update_merge_request(mr_assigned, john_doe) + + expect(pending_task.reload).to be_done + end + + it 'creates a task for each valid mentioned user' do + service.update_merge_request(mr_assigned, author) + + should_create_task(user: michael, target: mr_assigned, action: Task::MENTIONED) + should_not_create_task(user: author, target: mr_assigned, action: Task::MENTIONED) + should_not_create_task(user: john_doe, target: mr_assigned, action: Task::MENTIONED) + should_not_create_task(user: stranger, target: mr_assigned, action: Task::MENTIONED) + end + + it 'does not create a task if user was already mentioned' do + create(:task, :mentioned, user: michael, project: project, target: mr_assigned, author: author) + + should_not_create_any_task { service.update_merge_request(mr_assigned, author) } + end + end + describe '#close_merge_request' do - let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) - end - - let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) - end - it 'marks related pending tasks to the target for the user as done' do + first_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) + second_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) service.close_merge_request(mr_assigned, john_doe) - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done end end @@ -218,19 +229,13 @@ describe TaskService, services: true do end describe '#merge_merge_request' do - let!(:first_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) - end - - let!(:second_pending_task) do - create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author) - end - it 'marks related pending tasks to the target for the user as done' do + first_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) + second_task = create(:task, :assigned, user: john_doe, project: project, target: mr_assigned, author: author) service.merge_merge_request(mr_assigned, john_doe) - expect(first_pending_task.reload).to be_done - expect(second_pending_task.reload).to be_done + expect(first_task.reload).to be_done + expect(second_task.reload).to be_done end end end