Create a pending task when a user is mentioned when edit a issue/mr/note

10 files changed, 146 insertions, 100 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 '#close_issue' do
-      let!(:first_pending_task) do
-        create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author)
+    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
 
-      let!(:second_pending_task) do
-        create(:pending_assigned_task, user: john_doe, project: project, target: issue, author: author)
+      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
       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 '#close_merge_request' do
-      let!(:first_pending_task) do
-        create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author)
+    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
 
-      let!(:second_pending_task) do
-        create(:pending_assigned_task, user: john_doe, project: project, target: mr_assigned, author: author)
+      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
       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