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

Avoid plucking Todo ids in TodoService - take 2

See merge request !11415

8 files changed, 62 insertions, 22 deletions

diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 59e5b5e4775..a8b2b93b458 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -13,7 +13,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 do
@@ -37,7 +37,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/finders/todos_finder.rb b/app/finders/todos_finder.rb
index 3fe37c75381..b276116f0c6 100644
--- a/app/finders/todos_finder.rb
+++ b/app/finders/todos_finder.rb
@@ -95,9 +95,18 @@ class TodosFinder
     @project
   end
 
+  def project_ids(items)
+    ids = items.except(:order).select(:project_id)
+    if Gitlab::Database.mysql?
+      # To make UPDATE work on MySQL, wrap it in a SELECT with an alias
+      ids = Todo.except(:order).select('*').from("(#{ids.to_sql}) AS t")
+    end
+
+    ids
+  end
+
   def projects(items)
-    item_project_ids = items.reorder(nil).select(:project_id)
-    ProjectsFinder.new(current_user: current_user, project_ids_relation: item_project_ids).execute
+    ProjectsFinder.new(current_user: current_user, project_ids_relation: project_ids(items)).execute
   end
 
   def type?
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index ea497729115..760a15e3ed0 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -288,7 +288,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 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)
diff --git a/changelogs/unreleased/tc-no-todo-service-select.yml b/changelogs/unreleased/tc-no-todo-service-select.yml
new file mode 100644
index 00000000000..ddcae334aa7
--- /dev/null
+++ b/changelogs/unreleased/tc-no-todo-service-select.yml
@@ -0,0 +1,4 @@
+---
+title: Avoid plucking Todo ids in TodoService
+merge_request: 10845
+author:
diff --git a/lib/api/todos.rb b/lib/api/todos.rb
index d1f7e364029..55191169dd4 100644
--- a/lib/api/todos.rb
+++ b/lib/api/todos.rb
@@ -59,10 +59,10 @@ module API
         requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
       end
       post ':id/mark_as_done' do
-        todo = current_user.todos.find(params[:id])
-        TodoService.new.mark_todos_as_done([todo], current_user)
+        TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
+        todo = Todo.find(params[:id])
 
-        present todo.reload, with: Entities::Todo, current_user: current_user
+        present todo, with: Entities::Todo, current_user: current_user
       end
 
       desc 'Mark all todos as done'
diff --git a/lib/api/v3/todos.rb b/lib/api/v3/todos.rb
index e3b311d61cd..2f2cf259987 100644
--- a/lib/api/v3/todos.rb
+++ b/lib/api/v3/todos.rb
@@ -11,10 +11,10 @@ module API
           requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
         end
         delete ':id' do
-          todo = current_user.todos.find(params[:id])
-          TodoService.new.mark_todos_as_done([todo], current_user)
+          TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
+          todo = Todo.find(params[:id])
 
-          present todo.reload, with: ::API::Entities::Todo, current_user: current_user
+          present todo, with: ::API::Entities::Todo, current_user: current_user
         end
 
         desc 'Mark all todos as done'
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index 534d3e65be2..80d05451e09 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -336,7 +336,7 @@ describe TodoService do
 
     describe '#mark_todos_as_done' do
       it_behaves_like 'updating todos state', :mark_todos_as_done, :pending, :done do
-        let(:collection) { [first_todo, second_todo] }
+        let(:collection) { Todo.all }
       end
     end
 
@@ -348,7 +348,7 @@ describe TodoService do
 
     describe '#mark_todos_as_pending' do
       it_behaves_like 'updating todos state', :mark_todos_as_pending, :done, :pending do
-        let(:collection) { [first_todo, second_todo] }
+        let(:collection) { Todo.all }
       end
     end
 
@@ -880,14 +880,16 @@ describe TodoService do
     it 'marks an array of todos as done' do
       todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
 
-      expect { described_class.new.mark_todos_as_done([todo], john_doe) }
+      todos = TodosFinder.new(john_doe, {}).execute
+      expect { described_class.new.mark_todos_as_done(todos, john_doe) }
         .to change { todo.reload.state }.from('pending').to('done')
     end
 
     it 'returns the ids of updated todos' do # Needed on API
       todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
 
-      expect(described_class.new.mark_todos_as_done([todo], john_doe)).to eq([todo.id])
+      todos = TodosFinder.new(john_doe, {}).execute
+      expect(described_class.new.mark_todos_as_done(todos, john_doe)).to eq([todo.id])
     end
 
     context 'when some of the todos are done already' do
@@ -907,11 +909,32 @@ describe TodoService do
         expect(described_class.new.mark_todos_as_done(Todo.all, john_doe)).to eq([])
       end
     end
+  end
+
+  describe '#mark_todos_as_done_by_ids' do
+    let(:issue) { create(:issue, project: project, author: author, assignees: [john_doe]) }
+    let(:another_issue) { create(:issue, project: project, author: author, assignees: [john_doe]) }
+
+    it 'marks an array of todo ids as done' do
+      todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
+      another_todo = create(:todo, :mentioned, user: john_doe, target: another_issue, project: project)
+
+      expect { described_class.new.mark_todos_as_done_by_ids([todo.id, another_todo.id], john_doe) }
+        .to change { john_doe.todos.done.count }.from(0).to(2)
+    end
+
+    it 'marks a single todo id as done' do
+      todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
+
+      expect { described_class.new.mark_todos_as_done_by_ids(todo.id, john_doe) }
+        .to change { todo.reload.state }.from('pending').to('done')
+    end
 
     it 'caches the number of todos of a user', :use_clean_rails_memory_store_caching do
       create(:todo, :mentioned, user: john_doe, target: issue, project: project)
       todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
-      described_class.new.mark_todos_as_done([todo], john_doe)
+
+      described_class.new.mark_todos_as_done_by_ids(todo, john_doe)
 
       expect_any_instance_of(TodosFinder).not_to receive(:execute)