Commit c3c465ac authored by Robert Speicher's avatar Robert Speicher

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

Avoid plucking Todo ids in TodoService

Closes #30374

See merge request !10845
parents 15eaca81 a204d14c
...@@ -12,7 +12,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController ...@@ -12,7 +12,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 { redirect_to dashboard_todos_path, notice: 'Todo was successfully marked as done.' } format.html { redirect_to dashboard_todos_path, notice: 'Todo was successfully marked as done.' }
...@@ -32,7 +32,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController ...@@ -32,7 +32,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
......
...@@ -260,7 +260,7 @@ class IssuableBaseService < BaseService ...@@ -260,7 +260,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 ...@@ -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 ...@@ -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)
......
...@@ -59,10 +59,10 @@ module API ...@@ -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
TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
todo = current_user.todos.find(params[:id]) todo = current_user.todos.find(params[:id])
TodoService.new.mark_todos_as_done([todo], current_user)
present todo.reload, with: 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'
......
...@@ -11,10 +11,10 @@ module API ...@@ -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
TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
todo = current_user.todos.find(params[:id]) todo = current_user.todos.find(params[:id])
TodoService.new.mark_todos_as_done([todo], current_user)
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, services: true do ...@@ -336,7 +336,7 @@ describe TodoService, services: true 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, services: true do ...@@ -348,7 +348,7 @@ describe TodoService, services: true 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
...@@ -873,21 +873,15 @@ describe TodoService, services: true do ...@@ -873,21 +873,15 @@ describe TodoService, services: true do
create(:todo, :mentioned, user: john_doe, target: issue, project: project) create(:todo, :mentioned, user: john_doe, target: issue, project: project)
todos = TodosFinder.new(john_doe, {}).execute todos = TodosFinder.new(john_doe, {}).execute
expect { TodoService.new.mark_todos_as_done(todos, john_doe) } expect { service.mark_todos_as_done(todos, john_doe) }
.to change { john_doe.todos.done.count }.from(0).to(1) .to change { john_doe.todos.done.count }.from(0).to(1)
end end
it 'marks an array of todos as done' do
todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
expect { TodoService.new.mark_todos_as_done([todo], john_doe) }
.to change { todo.reload.state }.from('pending').to('done')
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(TodoService.new.mark_todos_as_done([todo], john_doe)).to eq([todo.id]) todos = TodosFinder.new(john_doe, {}).execute
expect(service.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
...@@ -895,23 +889,43 @@ describe TodoService, services: true do ...@@ -895,23 +889,43 @@ describe TodoService, services: true do
let!(:second_todo) { create(:todo, :mentioned, user: john_doe, target: another_issue, project: project) } let!(:second_todo) { create(:todo, :mentioned, user: john_doe, target: another_issue, project: project) }
it 'returns the ids of those still pending' do it 'returns the ids of those still pending' do
TodoService.new.mark_pending_todos_as_done(issue, john_doe) service.mark_pending_todos_as_done(issue, john_doe)
expect(TodoService.new.mark_todos_as_done(Todo.all, john_doe)).to eq([second_todo.id]) expect(service.mark_todos_as_done(Todo.all, john_doe)).to eq([second_todo.id])
end end
it 'returns an empty array if all are done' do it 'returns an empty array if all are done' do
TodoService.new.mark_pending_todos_as_done(issue, john_doe) service.mark_pending_todos_as_done(issue, john_doe)
TodoService.new.mark_pending_todos_as_done(another_issue, john_doe) service.mark_pending_todos_as_done(another_issue, john_doe)
expect(service.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, assignee: john_doe) }
let(:another_issue) { create(:issue, project: project, author: author, assignee: john_doe) }
expect(TodoService.new.mark_todos_as_done(Todo.all, john_doe)).to eq([]) 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 { service.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 end
it 'marks a single todo id as done' do
todo = create(:todo, :mentioned, user: john_doe, target: issue, project: project)
expect { service.mark_todos_as_done_by_ids(todo.id, john_doe) }
.to change { todo.reload.state }.from('pending').to('done')
end end
it 'caches the number of todos of a user', :caching do it 'caches the number of todos of a user', :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)
TodoService.new.mark_todos_as_done([todo], john_doe) service.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)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment