diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 2556f06e2d3..63025c10fb0 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -54,7 +54,7 @@ class IssuableBaseService < BaseService if params.present? && issuable.update_attributes(params.merge(updated_by: current_user)) issuable.reset_events_cache handle_common_system_notes(issuable, old_labels: old_labels) - handle_changes(issuable) + handle_changes(issuable, old_labels: old_labels) issuable.create_new_cross_references!(current_user) execute_hooks(issuable, 'update') end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index e6afcb91652..fb9571247cc 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -4,7 +4,11 @@ module Issues update(issue) end - def handle_changes(issue) + def handle_changes(issue, options = {}) + if have_changes?(issue, options) + task_service.mark_as_done(issue, current_user) + end + if issue.previous_changes.include?('milestone_id') create_milestone_note(issue) end @@ -23,5 +27,22 @@ module Issues def close_service Issues::CloseService end + + private + + def have_changes?(issue, options = {}) + valid_attrs = [:title, :description, :assignee_id, :milestone_id] + + attrs_changed = valid_attrs.any? do |attr| + issue.previous_changes.include?(attr.to_s) + end + + old_labels = options[:old_labels] + labels_changed = old_labels && issue.labels != old_labels + + if attrs_changed || labels_changed + task_service.mark_as_done(issue, current_user) + end + end end end diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 5ff2cc03dda..55322bf47bb 100644 --- a/app/services/merge_requests/update_service.rb +++ b/app/services/merge_requests/update_service.rb @@ -14,7 +14,7 @@ module MergeRequests update(merge_request) end - def handle_changes(merge_request) + def handle_changes(merge_request, options = {}) if merge_request.previous_changes.include?('target_branch') create_branch_change_note(merge_request, 'target', merge_request.previous_changes['target_branch'].first, diff --git a/app/services/task_service.rb b/app/services/task_service.rb index 35d258a70c2..2c312331ae9 100644 --- a/app/services/task_service.rb +++ b/app/services/task_service.rb @@ -26,6 +26,15 @@ class TaskService end 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_as_done(target, user) + pending_tasks = pending_tasks_for(user, target.project, target) + pending_tasks.update_all(state: :done) + end + private def create_task(project, target, author, user, action) @@ -40,4 +49,8 @@ class TaskService Task.create(attributes) end + + def pending_tasks_for(user, project, target) + user.tasks.pending.where(project: project, target: target) + end end diff --git a/spec/factories/tasks.rb b/spec/factories/tasks.rb index cd04405774a..710b8324f1f 100644 --- a/spec/factories/tasks.rb +++ b/spec/factories/tasks.rb @@ -19,5 +19,15 @@ FactoryGirl.define do project author user + + factory :pending_assigned_task, traits: [:assgined, :pending] + + trait :assgined do + action { Task::ASSIGNED } + end + + trait :pending do + state { :pending } + end end end diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 8f654517bce..1b7d5b45168 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -78,18 +78,74 @@ describe Issues::UpdateService, services: true do expect(note).not_to be_nil expect(note.note).to eq 'Title changed from **Old title** to **New title**' end + end - it 'creates a pending task if being reassigned' do - attributes = { - project: project, - author: user, - user: user2, - target: issue, - action: Task::ASSIGNED, - state: :pending - } + context 'task queue' do + let!(:pending_task) do + create(:pending_assigned_task, user: user, project: project, target: issue, author: user2) + end - expect(Task.where(attributes).count).to eq 1 + context 'when the title change' do + before do + update_issue({ title: 'New title' }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload.done?).to eq true + end + end + + context 'when the description change' do + before do + update_issue({ description: 'Also please fix' }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload.done?).to eq true + end + end + + context 'when is reassigned' do + before do + update_issue({ assignee: user2 }) + end + + it 'marks previous assignee pending tasks as done' do + expect(pending_task.reload.done?).to eq true + end + + it 'creates a pending task for new assignee' do + attributes = { + project: project, + author: user, + user: user2, + target: issue, + action: Task::ASSIGNED, + state: :pending + } + + expect(Task.where(attributes).count).to eq 1 + end + end + + context 'when the milestone change' do + before do + update_issue({ milestone: create(:milestone) }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload.done?).to eq true + end + end + + context 'when the labels change' do + before do + update_issue({ label_ids: [label.id] }) + end + + it 'marks pending tasks as done' do + expect(pending_task.reload.done?).to eq true + end end end @@ -157,6 +213,5 @@ describe Issues::UpdateService, services: true do end end end - end end diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb index db9f77fd12d..e8920c59fee 100644 --- a/spec/services/task_service_spec.rb +++ b/spec/services/task_service_spec.rb @@ -45,6 +45,23 @@ describe TaskService, services: true do is_expected_to_not_create_task { service.reassigned_issue(assigned_issue, author) } end end + + describe '#mark_as_done' do + let!(:first_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + end + + let!(:second_pending_task) do + create(:pending_assigned_task, user: john_doe, project: project, target: assigned_issue, author: author) + end + + it 'marks related pending tasks to the target for the user as done' do + service.mark_as_done(assigned_issue, john_doe) + + expect(first_pending_task.reload.done?).to eq true + expect(second_pending_task.reload.done?).to eq true + end + end end def is_expected_to_create_pending_task(attributes = {})