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
This commit is contained in:
parent
c39daf937b
commit
a723cba574
|
@ -13,7 +13,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
|
||||||
end
|
end
|
||||||
|
|
||||||
def destroy
|
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|
|
respond_to do |format|
|
||||||
format.html do
|
format.html do
|
||||||
|
@ -37,7 +37,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
|
||||||
end
|
end
|
||||||
|
|
||||||
def restore
|
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
|
render json: todos_counts
|
||||||
end
|
end
|
||||||
|
|
|
@ -288,7 +288,7 @@ class IssuableBaseService < BaseService
|
||||||
todo_service.mark_todo(issuable, current_user)
|
todo_service.mark_todo(issuable, current_user)
|
||||||
when 'done'
|
when 'done'
|
||||||
todo = TodosFinder.new(current_user).execute.find_by(target: issuable)
|
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
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -170,20 +170,22 @@ class TodoService
|
||||||
|
|
||||||
# When user marks some todos as done
|
# When user marks some todos as done
|
||||||
def mark_todos_as_done(todos, current_user)
|
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
|
end
|
||||||
|
|
||||||
def mark_todos_as_done_by_ids(ids, current_user)
|
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
|
end
|
||||||
|
|
||||||
# When user marks some todos as pending
|
# When user marks some todos as pending
|
||||||
def mark_todos_as_pending(todos, current_user)
|
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
|
end
|
||||||
|
|
||||||
def mark_todos_as_pending_by_ids(ids, current_user)
|
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
|
end
|
||||||
|
|
||||||
# When user marks an issue as todo
|
# When user marks an issue as todo
|
||||||
|
@ -198,9 +200,11 @@ class TodoService
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
def update_todos_state_by_ids(ids, current_user, state)
|
def todos_by_ids(ids, current_user)
|
||||||
todos = current_user.todos.where(id: ids)
|
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
|
# Only update those that are not really on that state
|
||||||
todos = todos.where.not(state: state)
|
todos = todos.where.not(state: state)
|
||||||
todos_ids = todos.pluck(:id)
|
todos_ids = todos.pluck(:id)
|
||||||
|
|
|
@ -0,0 +1,4 @@
|
||||||
|
---
|
||||||
|
title: Avoid plucking Todo ids in TodoService
|
||||||
|
merge_request: 10845
|
||||||
|
author:
|
|
@ -59,10 +59,10 @@ module API
|
||||||
requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
|
requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
|
||||||
end
|
end
|
||||||
post ':id/mark_as_done' do
|
post ':id/mark_as_done' do
|
||||||
todo = current_user.todos.find(params[:id])
|
TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
|
||||||
TodoService.new.mark_todos_as_done([todo], 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
|
end
|
||||||
|
|
||||||
desc 'Mark all todos as done'
|
desc 'Mark all todos as done'
|
||||||
|
|
|
@ -11,10 +11,10 @@ module API
|
||||||
requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
|
requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
|
||||||
end
|
end
|
||||||
delete ':id' do
|
delete ':id' do
|
||||||
todo = current_user.todos.find(params[:id])
|
TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
|
||||||
TodoService.new.mark_todos_as_done([todo], 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
|
end
|
||||||
|
|
||||||
desc 'Mark all todos as done'
|
desc 'Mark all todos as done'
|
||||||
|
|
|
@ -336,7 +336,7 @@ describe TodoService do
|
||||||
|
|
||||||
describe '#mark_todos_as_done' do
|
describe '#mark_todos_as_done' do
|
||||||
it_behaves_like 'updating todos state', :mark_todos_as_done, :pending, :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
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -348,7 +348,7 @@ describe TodoService do
|
||||||
|
|
||||||
describe '#mark_todos_as_pending' do
|
describe '#mark_todos_as_pending' do
|
||||||
it_behaves_like 'updating todos state', :mark_todos_as_pending, :done, :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
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@ -880,14 +880,16 @@ describe TodoService do
|
||||||
it 'marks an array of todos as done' do
|
it 'marks an array of todos as done' do
|
||||||
todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
|
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')
|
.to change { todo.reload.state }.from('pending').to('done')
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'returns the ids of updated todos' do # Needed on API
|
it 'returns the ids of updated todos' do # Needed on API
|
||||||
todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
|
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
|
end
|
||||||
|
|
||||||
context 'when some of the todos are done already' do
|
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([])
|
expect(described_class.new.mark_todos_as_done(Todo.all, john_doe)).to eq([])
|
||||||
end
|
end
|
||||||
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
|
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)
|
create(:todo, :mentioned, user: john_doe, target: issue, project: project)
|
||||||
todo = 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)
|
expect_any_instance_of(TodosFinder).not_to receive(:execute)
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue