Merge branch 'fix/simplify-todo-destroy-queries' into 'master'

Simplify SQL queries of marking a todo as done

See merge request !5832

4 files changed, 27 insertions, 16 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 518436f3d90..8bbe1ed7810 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -132,6 +132,7 @@ v 8.11.0 (unreleased)
   - Ensure file editing in UI does not overwrite commited changes without warning user
   - Eliminate unneeded calls to Repository#blob_at when listing commits with no path
   - Update gitlab_git gem to 10.4.7
+  - Simplify SQL queries of marking a todo as done
 
 v 8.10.6
   - Upgrade Rails to 4.2.7.1 for security fixes. !5781
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index 1243bb96d4d..c8390af3b36 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -6,7 +6,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
   end
 
   def destroy
-    TodoService.new.mark_todos_as_done([todo], 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.' }
@@ -27,10 +27,6 @@ class Dashboard::TodosController < Dashboard::ApplicationController
 
   private
 
-  def todo
-    @todo ||= find_todos.find(params[:id])
-  end
-
   def find_todos
     @todos ||= TodosFinder.new(current_user, params).execute
   end
diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb
index eb833dd82ac..daf4339cb48 100644
--- a/app/services/todo_service.rb
+++ b/app/services/todo_service.rb
@@ -142,7 +142,11 @@ class TodoService
 
   # When user marks some todos as done
   def mark_todos_as_done(todos, current_user)
-    todos = current_user.todos.where(id: todos.map(&:id)) unless todos.respond_to?(:update_all)
+    mark_todos_as_done_by_ids(todos.select(&:id), current_user)
+  end
+
+  def mark_todos_as_done_by_ids(ids, current_user)
+    todos = current_user.todos.where(id: ids)
 
     marked_todos = todos.update_all(state: :done)
     current_user.update_todos_count_cache
diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb
index 6c3cbeae13c..a2a1d5e6d30 100644
--- a/spec/services/todo_service_spec.rb
+++ b/spec/services/todo_service_spec.rb
@@ -194,12 +194,12 @@ describe TodoService, services: true do
       end
     end
 
-    describe '#mark_todos_as_done' do
-      it 'marks related todos for the user as done' do
-        first_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author)
-        second_todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author)
+    shared_examples 'marking todos as done' do |meth|
+      let!(:first_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }
+      let!(:second_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }
 
-        service.mark_todos_as_done([first_todo, second_todo], john_doe)
+      it 'marks related todos for the user as done' do
+        service.send(meth, collection, john_doe)
 
         expect(first_todo.reload).to be_done
         expect(second_todo.reload).to be_done
@@ -207,20 +207,30 @@ describe TodoService, services: true do
 
       describe 'cached counts' do
         it 'updates when todos change' do
-          todo = create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author)
-
           expect(john_doe.todos_done_count).to eq(0)
-          expect(john_doe.todos_pending_count).to eq(1)
+          expect(john_doe.todos_pending_count).to eq(2)
           expect(john_doe).to receive(:update_todos_count_cache).and_call_original
 
-          service.mark_todos_as_done([todo], john_doe)
+          service.send(meth, collection, john_doe)
 
-          expect(john_doe.todos_done_count).to eq(1)
+          expect(john_doe.todos_done_count).to eq(2)
           expect(john_doe.todos_pending_count).to eq(0)
         end
       end
     end
 
+    describe '#mark_todos_as_done' do
+      it_behaves_like 'marking todos as done', :mark_todos_as_done do
+        let(:collection) { [first_todo, second_todo] }
+      end
+    end
+
+    describe '#mark_todos_as_done_by_ids' do
+      it_behaves_like 'marking todos as done', :mark_todos_as_done_by_ids do
+        let(:collection) { [first_todo, second_todo].map(&:id) }
+      end
+    end
+
     describe '#new_note' do
       let!(:first_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }
       let!(:second_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }