Commit bc9b3084 authored by Yorick Peterse's avatar Yorick Peterse

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

Simplify SQL queries of marking a todo as done

See merge request !5832
parents 9e7231bd 4ad028ae
...@@ -132,6 +132,7 @@ v 8.11.0 (unreleased) ...@@ -132,6 +132,7 @@ v 8.11.0 (unreleased)
- Ensure file editing in UI does not overwrite commited changes without warning user - 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 - Eliminate unneeded calls to Repository#blob_at when listing commits with no path
- Update gitlab_git gem to 10.4.7 - Update gitlab_git gem to 10.4.7
- Simplify SQL queries of marking a todo as done
v 8.10.6 v 8.10.6
- Upgrade Rails to 4.2.7.1 for security fixes. !5781 - Upgrade Rails to 4.2.7.1 for security fixes. !5781
......
...@@ -6,7 +6,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController ...@@ -6,7 +6,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
end end
def destroy 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| 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.' }
...@@ -27,10 +27,6 @@ class Dashboard::TodosController < Dashboard::ApplicationController ...@@ -27,10 +27,6 @@ class Dashboard::TodosController < Dashboard::ApplicationController
private private
def todo
@todo ||= find_todos.find(params[:id])
end
def find_todos def find_todos
@todos ||= TodosFinder.new(current_user, params).execute @todos ||= TodosFinder.new(current_user, params).execute
end end
......
...@@ -142,7 +142,11 @@ class TodoService ...@@ -142,7 +142,11 @@ 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)
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) marked_todos = todos.update_all(state: :done)
current_user.update_todos_count_cache current_user.update_todos_count_cache
......
...@@ -194,12 +194,12 @@ describe TodoService, services: true do ...@@ -194,12 +194,12 @@ describe TodoService, services: true do
end end
end end
describe '#mark_todos_as_done' do shared_examples 'marking todos as done' do |meth|
it 'marks related todos for the user as done' do let!(:first_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }
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) }
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(first_todo.reload).to be_done
expect(second_todo.reload).to be_done expect(second_todo.reload).to be_done
...@@ -207,20 +207,30 @@ describe TodoService, services: true do ...@@ -207,20 +207,30 @@ describe TodoService, services: true do
describe 'cached counts' do describe 'cached counts' do
it 'updates when todos change' 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_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 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) expect(john_doe.todos_pending_count).to eq(0)
end end
end 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 describe '#new_note' do
let!(:first_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) } 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) } let!(:second_todo) { create(:todo, :assigned, user: john_doe, project: project, target: issue, author: author) }
......
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