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

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')

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')

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

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

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]

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

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]

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

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

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