Avoid plucking Todo ids and use sub-queries instead

TodoService should not call `.select(&:id)` on todos, because this is
bad performance. So instead use sub-queries, which will result in a
single SQL query to the database.

https://docs.gitlab.com/ee/development/sql.html#plucking-ids

1 files changed, 10 insertions, 6 deletions

diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index 322c6286365..6ee96d6a0f8 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)