Ensure that we only have one task per issue/mr

4 files changed, 82 insertions, 110 deletions

diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb
index 042b567b25f..d2620750305 100644
--- a/app/services/issues/update_service.rb
+++ b/app/services/issues/update_service.rb
@@ -6,6 +6,11 @@ module Issues
 
     def handle_changes(issue, options = {})
       if have_changes?(issue, options)
+        task_service.mark_pending_tasks_as_done(issue, current_user)
+      end
+
+      if issue.previous_changes.include?('title') ||
+         issue.previous_changes.include?('description')
         task_service.update_issue(issue, current_user)
       end
 
diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb
index 87949f0a9b8..a89daf9821e 100644
--- a/app/services/merge_requests/update_service.rb
+++ b/app/services/merge_requests/update_service.rb
@@ -16,6 +16,11 @@ 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)
+      end
+
+      if merge_request.previous_changes.include?('title') ||
+         merge_request.previous_changes.include?('description')
         task_service.update_merge_request(merge_request, current_user)
       end
 
diff --git a/app/services/task_service.rb b/app/services/task_service.rb
index bfd724a0f51..c4479fe6382 100644
--- a/app/services/task_service.rb
+++ b/app/services/task_service.rb
@@ -18,10 +18,9 @@ class TaskService
   # 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)
+    create_mention_tasks(issue.project, issue, current_user)
   end
 
   # When close an issue we should:
@@ -37,7 +36,7 @@ class TaskService
   #  * create a pending task for new assignee if issue is assigned
   #
   def reassigned_issue(issue, current_user)
-    reassigned_issuable(issue, current_user)
+    create_assignment_task(issue, current_user)
   end
 
   # When create a merge request we should:
@@ -51,11 +50,10 @@ class TaskService
 
   # 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
+  #  * create a task for each mentioned user on merge request
   #
   def update_merge_request(merge_request, current_user)
-    update_issuable(merge_request, current_user)
+    create_mention_tasks(merge_request.project, merge_request, current_user)
   end
 
   # When close a merge request we should:
@@ -71,7 +69,7 @@ class TaskService
   #  * creates a pending task for new assignee if merge request is assigned
   #
   def reassigned_merge_request(merge_request, current_user)
-    reassigned_issuable(merge_request, current_user)
+    create_assignment_task(merge_request, current_user)
   end
 
   # When merge a merge request we should:
@@ -87,21 +85,8 @@ class TaskService
   #  * 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
-    return if note.system
-
-    project = note.project
-    target  = note.noteable
-    author  = note.author
-
-    mark_pending_tasks_as_done(target, author)
-
-    mentioned_users = build_mentioned_users(project, note, author)
-
-    mentioned_users.each do |user|
-      create_task(project, target, author, user, Task::MENTIONED, note)
-    end
+  def new_note(note, current_user)
+    handle_note(note, current_user)
   end
 
   # When update a note we should:
@@ -110,41 +95,63 @@ class TaskService
   #  * create a task for each new user mentioned on note
   #
   def update_note(note, current_user)
+    handle_note(note, current_user)
+  end
+
+  # When marking pending tasks as done we should:
+  #
+  #  * mark all pending tasks related to the target for the current user as done
+  #
+  def mark_pending_tasks_as_done(target, user)
+    pending_tasks(user, target.project, target).update_all(state: :done)
+  end
+
+  private
+
+  def create_tasks(project, target, author, users, action, note = nil)
+    Array(users).each do |user|
+      next if pending_tasks(user, project, target).exists?
+
+      Task.create(
+        project: project,
+        user_id: user.id,
+        author_id: author.id,
+        target_id: target.id,
+        target_type: target.class.name,
+        action: action,
+        note: note
+      )
+    end
+  end
+
+  def new_issuable(issuable, author)
+    create_assignment_task(issuable, author)
+    create_mention_tasks(issuable.project, issuable, author)
+  end
+
+  def handle_note(note, author)
     # Skip system notes, like status changes and cross-references
     return if note.system
 
     project = note.project
     target  = note.noteable
-    author  = current_user
 
     mark_pending_tasks_as_done(target, author)
+    create_mention_tasks(project, target, author, note)
+  end
 
-    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
+  def create_assignment_task(issuable, author)
+    if issuable.assignee && issuable.assignee != author
+      create_tasks(issuable.project, issuable, author, issuable.assignee, Task::ASSIGNED)
     end
   end
 
-  private
-
-  def create_task(project, target, author, user, action, note = nil)
-    attributes = {
-      project: project,
-      user_id: user.id,
-      author_id: author.id,
-      target_id: target.id,
-      target_type: target.class.name,
-      action: action,
-      note: note
-    }
-
-    Task.create(attributes)
+  def create_mention_tasks(project, issuable, author, note = nil)
+    mentioned_users = filter_mentioned_users(project, note || issuable, author)
+    create_tasks(project, issuable, author, mentioned_users, Task::MENTIONED, note)
   end
 
-  def build_mentioned_users(project, target, author)
+  def filter_mentioned_users(project, target, author)
     mentioned_users = target.mentioned_users.select do |user|
       user.can?(:read_project, project)
     end
@@ -153,54 +160,11 @@ class TaskService
     mentioned_users.uniq
   end
 
-  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
+  def pending_tasks(user, project, target)
+    user.tasks.pending.where(
+      project_id: project.id,
+      target_id: target.id,
+      target_type: target.class.name
     )
-
-    user.tasks.pending.where(options)
-  end
-
-  def new_issuable(issuable, current_user)
-    project = issuable.project
-    target  = issuable
-    author  = issuable.author
-
-    if target.is_assigned? && target.assignee != current_user
-      create_task(project, target, author, target.assignee, Task::ASSIGNED)
-    end
-
-    mentioned_users = build_mentioned_users(project, target, author)
-    mentioned_users.delete(issuable.assignee)
-
-    mentioned_users.each do |mentioned_user|
-      create_task(project, target, author, mentioned_user, Task::MENTIONED)
-    end
-  end
-
-  def update_issuable(issuable, current_user)
-    project = issuable.project
-    author  = current_user
-
-    mark_pending_tasks_as_done(issuable, author)
-
-    mentioned_users = build_mentioned_users(project, issuable, author)
-
-    mentioned_users.each do |mentioned_user|
-      unless pending_tasks(mentioned_user, project, issuable, action: Task::MENTIONED).exists?
-        create_task(project, issuable, author, mentioned_user, Task::MENTIONED)
-      end
-    end
-  end
-
-  def reassigned_issuable(issuable, current_user)
-    if issuable.assignee && issuable.assignee != current_user
-      create_task(issuable.project, issuable, current_user, issuable.assignee, Task::ASSIGNED)
-    end
   end
 end
diff --git a/spec/services/task_service_spec.rb b/spec/services/task_service_spec.rb
index 75498514093..43022ca1604 100644
--- a/spec/services/task_service_spec.rb
+++ b/spec/services/task_service_spec.rb
@@ -45,13 +45,6 @@ describe TaskService, services: true do
     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)
 
@@ -101,6 +94,18 @@ describe TaskService, services: true do
       end
     end
 
+    describe '#mark_pending_tasks_as_done' 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.mark_pending_tasks_as_done(issue, john_doe)
+
+        expect(first_task.reload).to be_done
+        expect(second_task.reload).to be_done
+      end
+    end
+
     describe '#new_note' do
       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) }
@@ -112,28 +117,28 @@ describe TaskService, services: true 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)
+        service.new_note(note, john_doe)
 
         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)
+        service.new_note(award_note, john_doe)
 
         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)
+        service.new_note(system_note, john_doe)
 
         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
-        service.new_note(note)
+        service.new_note(note, john_doe)
 
         should_create_task(user: michael, target: issue, author: john_doe, action: Task::MENTIONED, note: note)
         should_create_task(user: author, target: issue, author: john_doe, action: Task::MENTIONED, note: note)
@@ -173,13 +178,6 @@ describe TaskService, services: true do
     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)