Merge branch '28799-todo-creation' into 'master'

Create todos only for new mentiones

Closes #28799

See merge request !10279

app/services/issues/update_service.rb

@@ -19,7 +19,7 @@ module Issues
 
       if issue.previous_changes.include?('title') ||
           issue.previous_changes.include?('description')
-        todo_service.update_issue(issue, current_user)
+        todo_service.update_issue(issue, current_user, old_mentioned_users)
       end
 
       if issue.previous_changes.include?('milestone_id')

app/services/merge_requests/update_service.rb

@@ -28,7 +28,7 @@ module MergeRequests
 
       if merge_request.previous_changes.include?('title') ||
           merge_request.previous_changes.include?('description')
-        todo_service.update_merge_request(merge_request, current_user)
+        todo_service.update_merge_request(merge_request, current_user, old_mentioned_users)
       end
 
       if merge_request.previous_changes.include?('target_branch')

app/services/notes/update_service.rb

@@ -3,11 +3,13 @@ module Notes
     def execute(note)
       return note unless note.editable?
 
+      old_mentioned_users = note.mentioned_users.to_a
+
       note.update_attributes(params.merge(updated_by: current_user))
       note.create_new_cross_references!(current_user)
 
       if note.previous_changes.include?('note')
-        TodoService.new.update_note(note, current_user)
+        TodoService.new.update_note(note, current_user, old_mentioned_users)
       end
 
       note

app/services/todo_service.rb

@@ -19,8 +19,8 @@ class TodoService
   #
   #  * mark all pending todos related to the issue for the current user as done
   #
-  def update_issue(issue, current_user)
-    update_issuable(issue, current_user)
+  def update_issue(issue, current_user, skip_users = [])
+    update_issuable(issue, current_user, skip_users)
   end
 
   # When close an issue we should:
@@ -60,8 +60,8 @@ class TodoService
   #
   #  * create a todo for each mentioned user on merge request
   #
-  def update_merge_request(merge_request, current_user)
-    update_issuable(merge_request, current_user)
+  def update_merge_request(merge_request, current_user, skip_users = [])
+    update_issuable(merge_request, current_user, skip_users)
   end
 
   # When close a merge request we should:
@@ -123,7 +123,7 @@ class TodoService
     mark_pending_todos_as_done(merge_request, merge_request.author)
     mark_pending_todos_as_done(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds?
   end
-  
+
   # When a merge request could not be automatically merged due to its unmergeable state we should:
   #
   #  * create a todo for a merge_user
@@ -131,7 +131,7 @@ class TodoService
   def merge_request_became_unmergeable(merge_request)
     create_unmergeable_todo(merge_request, merge_request.merge_user) if merge_request.merge_when_pipeline_succeeds?
   end
-  
+
   # When create a note we should:
   #
   #  * mark all pending todos related to the noteable for the note author as done
@@ -146,8 +146,8 @@ class TodoService
   #  * mark all pending todos related to the noteable for the current user as done
   #  * create a todo for each new user mentioned on note
   #
-  def update_note(note, current_user)
-    handle_note(note, current_user)
+  def update_note(note, current_user, skip_users = [])
+    handle_note(note, current_user, skip_users)
   end
 
   # When an emoji is awarded we should:
@@ -223,11 +223,11 @@ class TodoService
     create_mention_todos(issuable.project, issuable, author)
   end
 
-  def update_issuable(issuable, author)
+  def update_issuable(issuable, author, skip_users = [])
     # Skip toggling a task list item in a description
     return if toggling_tasks?(issuable)
 
-    create_mention_todos(issuable.project, issuable, author)
+    create_mention_todos(issuable.project, issuable, author, nil, skip_users)
   end
 
   def destroy_issuable(issuable, user)
@@ -239,7 +239,7 @@ class TodoService
       issuable.tasks? && issuable.updated_tasks.any?
   end
 
-  def handle_note(note, author)
+  def handle_note(note, author, skip_users = [])
     # Skip system notes, and notes on project snippet
     return if note.system? || note.for_snippet?
 
@@ -247,7 +247,7 @@ class TodoService
     target  = note.noteable
 
     mark_pending_todos_as_done(target, author)
-    create_mention_todos(project, target, author, note)
+    create_mention_todos(project, target, author, note, skip_users)
   end
 
   def create_assignment_todo(issuable, author)
@@ -257,14 +257,14 @@ class TodoService
     end
   end
 
-  def create_mention_todos(project, target, author, note = nil)
+  def create_mention_todos(project, target, author, note = nil, skip_users = [])
     # Create Todos for directly addressed users
-    directly_addressed_users = filter_directly_addressed_users(project, note || target, author)
+    directly_addressed_users = filter_directly_addressed_users(project, note || target, author, skip_users)
     attributes = attributes_for_todo(project, target, author, Todo::DIRECTLY_ADDRESSED, note)
     create_todos(directly_addressed_users, attributes)
 
     # Create Todos for mentioned users
-    mentioned_users = filter_mentioned_users(project, note || target, author)
+    mentioned_users = filter_mentioned_users(project, note || target, author, skip_users)
     attributes = attributes_for_todo(project, target, author, Todo::MENTIONED, note)
     create_todos(mentioned_users, attributes)
   end
@@ -307,13 +307,13 @@ class TodoService
     reject_users_without_access(users, project, target).uniq
   end
 
-  def filter_mentioned_users(project, target, author)
-    mentioned_users = target.mentioned_users(author)
+  def filter_mentioned_users(project, target, author, skip_users = [])
+    mentioned_users = target.mentioned_users(author) - skip_users
     filter_todo_users(mentioned_users, project, target)
   end
 
-  def filter_directly_addressed_users(project, target, author)
-    directly_addressed_users = target.directly_addressed_users(author)
+  def filter_directly_addressed_users(project, target, author, skip_users = [])
+    directly_addressed_users = target.directly_addressed_users(author) - skip_users
     filter_todo_users(directly_addressed_users, project, target)
   end
 

changelogs/unreleased/28799-todo-creation.yml

+---
+title: Create todos only for new mentions
+merge_request:
+author:

spec/services/issues/update_service_spec.rb

@@ -13,6 +13,7 @@ describe Issues::UpdateService, services: true do
 
   let(:issue) do
     create(:issue, title: 'Old title',
+                   description: "for #{user2.to_reference}",
                    assignee_id: user3.id,
                    project: project)
   end
@@ -182,16 +183,24 @@ describe Issues::UpdateService, services: true do
         it 'marks pending todos as done' do
           expect(todo.reload.done?).to eq true
         end
+
+        it 'does not create any new todos' do
+          expect(Todo.count).to eq(1)
+        end
       end
 
       context 'when the description change' do
         before do
-          update_issue(description: 'Also please fix')
+          update_issue(description: "Also please fix #{user2.to_reference} #{user3.to_reference}")
         end
 
         it 'marks todos as done' do
           expect(todo.reload.done?).to eq true
         end
+
+        it 'creates only 1 new todo' do
+          expect(Todo.count).to eq(2)
+        end
       end
 
       context 'when is reassigned' do

spec/services/merge_requests/update_service_spec.rb

@@ -12,6 +12,7 @@ describe MergeRequests::UpdateService, services: true do
 
   let(:merge_request) do
     create(:merge_request, :simple, title: 'Old title',
+                                    description: "FYI #{user2.to_reference}",
                                     assignee_id: user3.id,
                                     source_project: project)
   end
@@ -225,16 +226,24 @@ describe MergeRequests::UpdateService, services: true do
         it 'marks pending todos as done' do
           expect(pending_todo.reload).to be_done
         end
+
+        it 'does not create any new todos' do
+          expect(Todo.count).to eq(1)
+        end
       end
 
       context 'when the description change' do
         before do
-          update_merge_request({ description: 'Also please fix' })
+          update_merge_request({ description: "Also please fix #{user2.to_reference} #{user3.to_reference}" })
         end
 
         it 'marks pending todos as done' do
           expect(pending_todo.reload).to be_done
         end
+
+        it 'creates only 1 new todo' do
+          expect(Todo.count).to eq(2)
+        end
       end
 
       context 'when is reassigned' do

spec/services/notes/update_service_spec.rb

@@ -4,12 +4,14 @@ describe Notes::UpdateService, services: true do
   let(:project) { create(:empty_project) }
   let(:user) { create(:user) }
   let(:user2) { create(:user) }
+  let(:user3) { create(:user) }
   let(:issue) { create(:issue, project: project) }
-  let(:note) { create(:note, project: project, noteable: issue, author: user, note: 'Old note') }
+  let(:note) { create(:note, project: project, noteable: issue, author: user, note: "Old note #{user2.to_reference}") }
 
   before do
     project.team << [user, :master]
     project.team << [user2, :developer]
+    project.team << [user3, :developer]
   end
 
   describe '#execute' do
@@ -23,22 +25,30 @@ describe Notes::UpdateService, services: true do
 
       context 'when the note change' do
         before do
-          update_note({ note: 'New note' })
+          update_note({ note: "New note #{user2.to_reference} #{user3.to_reference}" })
         end
 
         it 'marks todos as done' do
           expect(todo.reload).to be_done
         end
+
+        it 'creates only 1 new todo' do
+          expect(Todo.count).to eq(2)
+        end
       end
 
       context 'when the note does not change' do
         before do
-          update_note({ note: 'Old note' })
+          update_note({ note: "Old note #{user2.to_reference}" })
         end
 
         it 'keep todos' do
           expect(todo.reload).to be_pending
         end
+
+        it 'does not create any new todos' do
+          expect(Todo.count).to eq(1)
+        end
       end
     end
   end