Merge branch 'fix-toggling-task-should-not-generate-todo' into 'master'

Fix error when editing an issuable with a task list

Closes #18712

See merge request !4751

2 files changed, 40 insertions, 19 deletions

diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index e1f9ea64dc4..5c0c1ba27bf 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -161,11 +161,16 @@ class TodoService
 
   def update_issuable(issuable, author)
     # Skip toggling a task list item in a description
-    return if issuable.tasks? && issuable.updated_tasks.any?
+    return if toggling_tasks?(issuable)
 
     create_mention_todos(issuable.project, issuable, author)
   end
 
+  def toggling_tasks?(issuable)
+    issuable.previous_changes.include?('description') &&
+      issuable.tasks? && issuable.updated_tasks.any?
+  end
+
   def handle_note(note, author)
     # Skip system notes, and notes on project snippet
     return if note.system? || note.for_snippet?
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index 26f09cdbaf9..a09413692d9 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -108,17 +108,25 @@ describe TodoService, services: true do
         should_not_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED)
       end
 
-      it 'does not create todo when when tasks are marked as completed' do
-        issue.update(description: "- [x] Task 1\n- [X] Task 2 #{mentions}")
+      context 'issues with a task list' do
+        it 'does not create todo when tasks are marked as completed' do
+          issue.update(description: "- [x] Task 1\n- [X] Task 2 #{mentions}")
+
+          service.update_issue(issue, author)
+
+          should_not_create_todo(user: admin, target: issue, action: Todo::MENTIONED)
+          should_not_create_todo(user: assignee, target: issue, action: Todo::MENTIONED)
+          should_not_create_todo(user: author, target: issue, action: Todo::MENTIONED)
+          should_not_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED)
+          should_not_create_todo(user: member, target: issue, action: Todo::MENTIONED)
+          should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED)
+        end
 
-        service.update_issue(issue, author)
+        it 'does not raise an error when description not change' do
+          issue.update(title: 'Sample')
 
-        should_not_create_todo(user: admin, target: issue, action: Todo::MENTIONED)
-        should_not_create_todo(user: assignee, target: issue, action: Todo::MENTIONED)
-        should_not_create_todo(user: author, target: issue, action: Todo::MENTIONED)
-        should_not_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED)
-        should_not_create_todo(user: member, target: issue, action: Todo::MENTIONED)
-        should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED)
+          expect { service.update_issue(issue, author) }.not_to raise_error
+        end
       end
     end
 
@@ -285,17 +293,25 @@ describe TodoService, services: true do
         expect { service.update_merge_request(mr_assigned, author) }.not_to change(member.todos, :count)
       end
 
-      it 'does not create todo when when tasks are marked as completed' do
-        mr_assigned.update(description: "- [x] Task 1\n- [X] Task 2 #{mentions}")
+      context 'with a task list' do
+        it 'does not create todo when tasks are marked as completed' do
+          mr_assigned.update(description: "- [x] Task 1\n- [X] Task 2 #{mentions}")
 
-        service.update_merge_request(mr_assigned, author)
+          service.update_merge_request(mr_assigned, author)
 
-        should_not_create_todo(user: admin, target: mr_assigned, action: Todo::MENTIONED)
-        should_not_create_todo(user: assignee, target: mr_assigned, action: Todo::MENTIONED)
-        should_not_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED)
-        should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
-        should_not_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED)
-        should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
+          should_not_create_todo(user: admin, target: mr_assigned, action: Todo::MENTIONED)
+          should_not_create_todo(user: assignee, target: mr_assigned, action: Todo::MENTIONED)
+          should_not_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED)
+          should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED)
+          should_not_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED)
+          should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED)
+        end
+
+        it 'does not raise an error when description not change' do
+          mr_assigned.update(title: 'Sample')
+
+          expect { service.update_merge_request(mr_assigned, author) }.not_to raise_error
+        end
       end
     end