Marks pending tasks for an user as done when he edit an issue

7 files changed, 130 insertions, 14 deletions

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
+
+      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
 
-        expect(Task.where(attributes).count).to eq 1
+        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 = {})