Merge branch 'tc-no-todo-service-select' into 'master'

Avoid plucking Todo ids in TodoService

Closes #30374

See merge request !10845

3 files changed, 13 insertions, 9 deletions

diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 4d7d45787fc..2b76e57c06a 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -12,7 +12,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def destroy
-    TodoService.new.mark_todos_as_done_by_ids([params[:id]], current_user)
+    TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
 
     respond_to do |format|
       format.html { redirect_to dashboard_todos_path, notice: 'Todo was successfully marked as done.' }
@@ -32,7 +32,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def restore
-    TodoService.new.mark_todos_as_pending_by_ids([params[:id]], current_user)
+    TodoService.new.mark_todos_as_pending_by_ids(params[:id], current_user)
 
     render json: todos_counts
   end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index b071a398481..17cb71be5f6 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -260,7 +260,7 @@ class IssuableBaseService < BaseService
       todo_service.mark_todo(issuable, current_user)
     when 'done'
       todo = TodosFinder.new(current_user).execute.find_by(target: issuable)
-      todo_service.mark_todos_as_done([todo], current_user) if todo
+      todo_service.mark_todos_as_done_by_ids(todo, current_user) if todo
     end
   end
 
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index b6e88b0280f..a19283cccd3 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -170,20 +170,22 @@ class TodoService
 
   # When user marks some todos as done
   def mark_todos_as_done(todos, current_user)
-    update_todos_state_by_ids(todos.select(&:id), current_user, :done)
+    update_todos_state(todos, current_user, :done)
   end
 
   def mark_todos_as_done_by_ids(ids, current_user)
-    update_todos_state_by_ids(ids, current_user, :done)
+    todos = todos_by_ids(ids, current_user)
+    mark_todos_as_done(todos, current_user)
   end
 
   # When user marks some todos as pending
   def mark_todos_as_pending(todos, current_user)
-    update_todos_state_by_ids(todos.select(&:id), current_user, :pending)
+    update_todos_state(todos, current_user, :pending)
   end
 
   def mark_todos_as_pending_by_ids(ids, current_user)
-    update_todos_state_by_ids(ids, current_user, :pending)
+    todos = todos_by_ids(ids, current_user)
+    mark_todos_as_pending(todos, current_user)
   end
 
   # When user marks an issue as todo
@@ -198,9 +200,11 @@ class TodoService
 
   private
 
-  def update_todos_state_by_ids(ids, current_user, state)
-    todos = current_user.todos.where(id: ids)
+  def todos_by_ids(ids, current_user)
+    current_user.todos.where(id: Array(ids))
+  end
 
+  def update_todos_state(todos, current_user, state)
     # Only update those that are not really on that state
     todos = todos.where.not(state: state)
     todos_ids = todos.pluck(:id)