summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorSean McGivern <sean@gitlab.com>2017-10-04 17:37:38 +0100
committerSean McGivern <sean@gitlab.com>2017-10-04 17:37:38 +0100
commit063b9edc777e3df992e161c94572f4ff6ece9e02 (patch)
treea03c39fa42fbdbb312faf1e76ccb4a32eb728128
parent4a4f809353a2e7007f8c6d33bfb1e4d09ed5a560 (diff)
downloadgitlab-ce-save-a-query-on-todos-with-no-filters.tar.gz
Save a query on the todos index pagesave-a-query-on-todos-with-no-filters
When there are no filters, we can get the total todos count from the cached count on the user object, instead of performing a DB query. This query takes about 80ms for me on GitLab.com.
-rw-r--r--app/controllers/dashboard/todos_controller.rb30
-rw-r--r--changelogs/unreleased/save-a-query-on-todos-with-no-filters.yml5
-rw-r--r--spec/controllers/dashboard/todos_controller_spec.rb26
3 files changed, 56 insertions, 5 deletions
diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb
index a8b2b93b458..02c5857eea7 100644
--- a/app/controllers/dashboard/todos_controller.rb
+++ b/app/controllers/dashboard/todos_controller.rb
@@ -7,9 +7,8 @@ class Dashboard::TodosController < Dashboard::ApplicationController
def index
@sort = params[:sort]
@todos = @todos.page(params[:page])
- if @todos.out_of_range? && @todos.total_pages != 0
- redirect_to url_for(params.merge(page: @todos.total_pages, only_path: true))
- end
+
+ return if redirect_out_of_range(@todos)
end
def destroy
@@ -60,7 +59,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
end
def find_todos
- @todos ||= TodosFinder.new(current_user, params).execute
+ @todos ||= TodosFinder.new(current_user, todo_params).execute
end
def todos_counts
@@ -69,4 +68,27 @@ class Dashboard::TodosController < Dashboard::ApplicationController
done_count: number_with_delimiter(current_user.todos_done_count)
}
end
+
+ def todo_params
+ params.permit(:action_id, :author_id, :project_id, :type, :sort, :state)
+ end
+
+ def redirect_out_of_range(todos)
+ total_pages =
+ if todo_params.except(:sort, :page).empty?
+ (current_user.todos_pending_count / todos.limit_value).ceil
+ else
+ todos.total_pages
+ end
+
+ return false if total_pages.zero?
+
+ out_of_range = todos.current_page > total_pages
+
+ if out_of_range
+ redirect_to url_for(params.merge(page: total_pages, only_path: true))
+ end
+
+ out_of_range
+ end
end
diff --git a/changelogs/unreleased/save-a-query-on-todos-with-no-filters.yml b/changelogs/unreleased/save-a-query-on-todos-with-no-filters.yml
new file mode 100644
index 00000000000..c9fb042aa37
--- /dev/null
+++ b/changelogs/unreleased/save-a-query-on-todos-with-no-filters.yml
@@ -0,0 +1,5 @@
+---
+title: Remove a SQL query from the todos index page
+merge_request:
+author:
+type: other
diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb
index c8c6b9f41bf..9df4ebf2fa0 100644
--- a/spec/controllers/dashboard/todos_controller_spec.rb
+++ b/spec/controllers/dashboard/todos_controller_spec.rb
@@ -57,7 +57,7 @@ describe Dashboard::TodosController do
expect(response).to redirect_to(dashboard_todos_path(page: last_page))
end
- it 'redirects to correspondent page' do
+ it 'goes to the correct page' do
get :index, page: last_page
expect(assigns(:todos).current_page).to eq(last_page)
@@ -70,6 +70,30 @@ describe Dashboard::TodosController do
expect(response).to redirect_to(dashboard_todos_path(page: last_page))
end
+
+ context 'when providing no filters' do
+ it 'does not perform a query to get the page count, but gets that from the user' do
+ allow(controller).to receive(:current_user).and_return(user)
+
+ expect(user).to receive(:todos_pending_count).and_call_original
+
+ get :index, page: (last_page + 1).to_param, sort: :created_asc
+
+ expect(response).to redirect_to(dashboard_todos_path(page: last_page, sort: :created_asc))
+ end
+ end
+
+ context 'when providing filters' do
+ it 'performs a query to get the correct page count' do
+ allow(controller).to receive(:current_user).and_return(user)
+
+ expect(user).not_to receive(:todos_pending_count)
+
+ get :index, page: (last_page + 1).to_param, project_id: project.id
+
+ expect(response).to redirect_to(dashboard_todos_path(page: last_page, project_id: project.id))
+ end
+ end
end
end