diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index 4d7d45787fc48652224cc55c039ec0dfb0246978..2b76e57c06a7c9d930baaa357ddb0a49c3975538 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -12,7 +12,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 { redirect_to dashboard_todos_path, notice: 'Todo was successfully marked as done.' } @@ -32,7 +32,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/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index b071a3984811eb97a6437eb3462f0b4e26452b46..17cb71be5f60a603f9e569ef1df45e6168c46b8d 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -260,7 +260,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 b6e88b0280f228b2ebe4dc2f527ba3f63b785a94..a19283cccd3f1dc75ce3141b1a85f0592497c7b4 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/lib/api/todos.rb b/lib/api/todos.rb index d1f7e364029c8fc9bc751d2f7e98768ce778dd33..10aa64947fe6bfff518dd5aa5cafa5d96be8974d 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 + TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user) 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 desc 'Mark all todos as done' diff --git a/lib/api/v3/todos.rb b/lib/api/v3/todos.rb index e3b311d61cdbca094fce07dc453e52d249fb7bb0..3e2c61f6dbd45d8367fc0777e9ec559d337d8c62 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 + TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user) 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 desc 'Mark all todos as done' diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 89b3b6aad103c32cb008a8c991ef48d08ec93f40..801657d6c0ca8179f347584a03f323c51e136f34 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -336,7 +336,7 @@ describe TodoService, services: true 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, services: true 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 @@ -873,21 +873,15 @@ describe TodoService, services: true do create(:todo, :mentioned, user: john_doe, target: issue, project: project) 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) 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 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 context 'when some of the todos are done already' do @@ -895,23 +889,43 @@ describe TodoService, services: true do let!(:second_todo) { create(:todo, :mentioned, user: john_doe, target: another_issue, project: project) } 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 it 'returns an empty array if all are done' do - TodoService.new.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(issue, john_doe) + service.mark_pending_todos_as_done(another_issue, john_doe) - expect(TodoService.new.mark_todos_as_done(Todo.all, john_doe)).to eq([]) + 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) } + + 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 + + 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 it 'caches the number of todos of a user', :caching do 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)