From 9b66aa6e04ab66af7ce11e26076ebf431ca938c5 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Mon, 19 Dec 2016 10:26:12 +0000 Subject: [PATCH 1/3] Prevent empty pagination when list is not empty --- app/controllers/concerns/kaminari_pagination.rb | 8 ++++++++ app/controllers/dashboard/todos_controller.rb | 4 +++- app/controllers/projects/issues_controller.rb | 3 ++- app/controllers/projects/merge_requests_controller.rb | 3 ++- app/controllers/projects/snippets_controller.rb | 3 ++- ...19988-prevent-empty-pagination-when-list-not-empty.yml | 4 ++++ 6 files changed, 21 insertions(+), 4 deletions(-) create mode 100644 app/controllers/concerns/kaminari_pagination.rb create mode 100644 changelogs/unreleased/19988-prevent-empty-pagination-when-list-not-empty.yml diff --git a/app/controllers/concerns/kaminari_pagination.rb b/app/controllers/concerns/kaminari_pagination.rb new file mode 100644 index 00000000000..b69340cac0e --- /dev/null +++ b/app/controllers/concerns/kaminari_pagination.rb @@ -0,0 +1,8 @@ +module KaminariPagination + extend ActiveSupport::Concern + + def bounded_pagination(items, page_number) + items = items.page(page_number) + items.to_a.empty? ? items.page(items.total_pages) : items + end +end diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index d425d0f9014..b2333a02392 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -1,9 +1,11 @@ class Dashboard::TodosController < Dashboard::ApplicationController + include KaminariPagination + before_action :find_todos, only: [:index, :destroy_all] def index @sort = params[:sort] - @todos = @todos.page(params[:page]) + @todos = bounded_pagination(@todos, params[:page]) end def destroy diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 4f66e01e0f7..ec09a5297fc 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -5,6 +5,7 @@ class Projects::IssuesController < Projects::ApplicationController include ToggleAwardEmoji include IssuableCollections include SpammableActions + include KaminariPagination before_action :redirect_to_external_issue_tracker, only: [:index, :new] before_action :module_enabled @@ -24,7 +25,7 @@ class Projects::IssuesController < Projects::ApplicationController def index @issues = issues_collection - @issues = @issues.page(params[:page]) + @issues = bounded_pagination(@issues, params[:page]) if params[:label_name].present? @labels = LabelsFinder.new(current_user, project_id: @project.id, title: params[:label_name]).execute diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 3abebdfd032..e25eea52723 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -6,6 +6,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController include NotesHelper include ToggleAwardEmoji include IssuableCollections + include KaminariPagination before_action :module_enabled before_action :merge_request, only: [ @@ -37,7 +38,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController def index @merge_requests = merge_requests_collection - @merge_requests = @merge_requests.page(params[:page]) + @merge_requests = bounded_pagination(@merge_requests, params[:page]) if params[:label_name].present? labels_params = { project_id: @project.id, title: params[:label_name] } diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index 0720be2e55d..7083b29b6a3 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/app/controllers/projects/snippets_controller.rb @@ -1,5 +1,6 @@ class Projects::SnippetsController < Projects::ApplicationController include ToggleAwardEmoji + include KaminariPagination before_action :module_enabled before_action :snippet, only: [:show, :edit, :destroy, :update, :raw, :toggle_award_emoji] @@ -25,7 +26,7 @@ class Projects::SnippetsController < Projects::ApplicationController project: @project, scope: params[:scope] ) - @snippets = @snippets.page(params[:page]) + @snippets = bounded_pagination(@snippets, params[:page]) end def new diff --git a/changelogs/unreleased/19988-prevent-empty-pagination-when-list-not-empty.yml b/changelogs/unreleased/19988-prevent-empty-pagination-when-list-not-empty.yml new file mode 100644 index 00000000000..5570ede4a9a --- /dev/null +++ b/changelogs/unreleased/19988-prevent-empty-pagination-when-list-not-empty.yml @@ -0,0 +1,4 @@ +--- +title: Prevent empty pagination when list is not empty +merge_request: 8172 +author: From 805bbe889328bff55b49290294721405148cc980 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Tue, 20 Dec 2016 18:52:09 +0000 Subject: [PATCH 2/3] adds specs for respective behaviour --- .../concerns/kaminari_pagination.rb | 8 ----- app/controllers/dashboard/todos_controller.rb | 7 ++-- app/controllers/projects/issues_controller.rb | 6 ++-- .../projects/merge_requests_controller.rb | 6 ++-- .../projects/snippets_controller.rb | 6 ++-- .../dashboard/todos_controller_spec.rb | 36 +++++++++++++++++++ .../projects/issues_controller_spec.rb | 29 +++++++++++++++ .../merge_requests_controller_spec.rb | 22 ++++++++++-- .../projects/snippets_controller_spec.rb | 22 ++++++++++++ 9 files changed, 123 insertions(+), 19 deletions(-) delete mode 100644 app/controllers/concerns/kaminari_pagination.rb create mode 100644 spec/controllers/dashboard/todos_controller_spec.rb diff --git a/app/controllers/concerns/kaminari_pagination.rb b/app/controllers/concerns/kaminari_pagination.rb deleted file mode 100644 index b69340cac0e..00000000000 --- a/app/controllers/concerns/kaminari_pagination.rb +++ /dev/null @@ -1,8 +0,0 @@ -module KaminariPagination - extend ActiveSupport::Concern - - def bounded_pagination(items, page_number) - items = items.page(page_number) - items.to_a.empty? ? items.page(items.total_pages) : items - end -end diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index b2333a02392..40652129f4c 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -1,11 +1,12 @@ class Dashboard::TodosController < Dashboard::ApplicationController - include KaminariPagination - before_action :find_todos, only: [:index, :destroy_all] def index @sort = params[:sort] - @todos = bounded_pagination(@todos, params[:page]) + @todos = @todos.page(params[:page]) + if @todos.out_of_range? && @todos.total_pages != 0 + redirect_to dashboard_todos_path(page: @todos.total_pages) + end end def destroy diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index ec09a5297fc..0efeec98570 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -5,7 +5,6 @@ class Projects::IssuesController < Projects::ApplicationController include ToggleAwardEmoji include IssuableCollections include SpammableActions - include KaminariPagination before_action :redirect_to_external_issue_tracker, only: [:index, :new] before_action :module_enabled @@ -25,7 +24,10 @@ class Projects::IssuesController < Projects::ApplicationController def index @issues = issues_collection - @issues = bounded_pagination(@issues, params[:page]) + @issues = @issues.page(params[:page]) + if @issues.out_of_range? && @issues.total_pages != 0 + return redirect_to namespace_project_issues_path(page: @issues.total_pages) + end if params[:label_name].present? @labels = LabelsFinder.new(current_user, project_id: @project.id, title: params[:label_name]).execute diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index e25eea52723..050d0ca77ae 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -6,7 +6,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController include NotesHelper include ToggleAwardEmoji include IssuableCollections - include KaminariPagination before_action :module_enabled before_action :merge_request, only: [ @@ -38,7 +37,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController def index @merge_requests = merge_requests_collection - @merge_requests = bounded_pagination(@merge_requests, params[:page]) + @merge_requests = @merge_requests.page(params[:page]) + if @merge_requests.out_of_range? && @merge_requests.total_pages != 0 + return redirect_to namespace_project_merge_requests_path(page: @merge_requests.total_pages) + end if params[:label_name].present? labels_params = { project_id: @project.id, title: params[:label_name] } diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index 7083b29b6a3..02a97c1c574 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/app/controllers/projects/snippets_controller.rb @@ -1,6 +1,5 @@ class Projects::SnippetsController < Projects::ApplicationController include ToggleAwardEmoji - include KaminariPagination before_action :module_enabled before_action :snippet, only: [:show, :edit, :destroy, :update, :raw, :toggle_award_emoji] @@ -26,7 +25,10 @@ class Projects::SnippetsController < Projects::ApplicationController project: @project, scope: params[:scope] ) - @snippets = bounded_pagination(@snippets, params[:page]) + @snippets = @snippets.page(params[:page]) + if @snippets.out_of_range? && @snippets.total_pages != 0 + redirect_to namespace_project_snippets_path(page: @snippets.total_pages) + end end def new diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb new file mode 100644 index 00000000000..a8c08a6e87b --- /dev/null +++ b/spec/controllers/dashboard/todos_controller_spec.rb @@ -0,0 +1,36 @@ +require 'spec_helper' + +describe Dashboard::TodosController do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:todo_service) { TodoService.new } + + describe 'GET #index' do + before do + sign_in(user) + project.team << [user, :developer] + end + + context 'when using pagination' do + let(:last_page) { user.todos.page().total_pages } + let!(:issues) { create_list(:issue, 30, project: project, assignee: user) } + + before do + issues.each { |issue| todo_service.new_issue(issue, user) } + end + + it 'redirects to last_page if page number is larger than number of pages' do + get :index, page: (last_page + 1).to_param + + expect(response).to redirect_to(dashboard_todos_path(page: last_page)) + end + + it 'redirects to correspondent page' do + get :index, page: last_page + + expect(assigns(:todos).current_page).to eq(last_page) + expect(response).to have_http_status(200) + end + end + end +end diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index dbe5ddccbcf..60b21cb4ace 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -52,6 +52,35 @@ describe Projects::IssuesController do expect(response).to have_http_status(404) end end + + context 'with page param' do + let(:last_page) { project.issues.page().total_pages } + let!(:issue_list) { create_list(:issue, 30, project: project) } + + before do + sign_in(user) + project.team << [user, :developer] + end + + it 'redirects to last_page if page number is larger than number of pages' do + get :index, + namespace_id: project.namespace.path.to_param, + project_id: project.path.to_param, + page: (last_page + 1).to_param + + expect(response).to redirect_to(namespace_project_issues_path(page: last_page)) + end + + it 'redirects to specified page' do + get :index, + namespace_id: project.namespace.path.to_param, + project_id: project.path.to_param, + page: last_page.to_param + + expect(assigns(:issues).current_page).to eq(last_page) + expect(response).to have_http_status(200) + end + end end describe 'GET #new' do diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 440b897ddc6..92392172af0 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -127,11 +127,29 @@ describe Projects::MergeRequestsController do end describe 'GET index' do - def get_merge_requests + let(:last_page) { project.merge_requests.page().total_pages } + let!(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } + + def get_merge_requests(page = nil) get :index, namespace_id: project.namespace.to_param, project_id: project.to_param, - state: 'opened' + state: 'opened', page: page.to_param + end + + context 'when page param' do + it 'redirects to last_page if page number is larger than number of pages' do + get_merge_requests(last_page + 1) + + expect(response).to redirect_to(namespace_project_merge_requests_path(page: last_page)) + end + + it 'redirects to specified page' do + get_merge_requests(last_page) + + expect(assigns(:merge_requests).current_page).to eq(last_page) + expect(response).to have_http_status(200) + end end context 'when filtering by opened state' do diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb index 72a3ebf2ebd..82e57e4d7dc 100644 --- a/spec/controllers/projects/snippets_controller_spec.rb +++ b/spec/controllers/projects/snippets_controller_spec.rb @@ -11,6 +11,28 @@ describe Projects::SnippetsController do end describe 'GET #index' do + context 'when page param' do + let(:last_page) { project.snippets.page().total_pages } + let!(:project_snippet) { create(:project_snippet, :public, project: project, author: user) } + + it 'redirects to last_page if page number is larger than number of pages' do + get :index, + namespace_id: project.namespace.path, + project_id: project.path, page: (last_page + 1).to_param + + expect(response).to redirect_to(namespace_project_snippets_path(page: last_page)) + end + + it 'redirects to specified page' do + get :index, + namespace_id: project.namespace.path, + project_id: project.path, page: (last_page).to_param + + expect(assigns(:snippets).current_page).to eq(last_page) + expect(response).to have_http_status(200) + end + end + context 'when the project snippet is private' do let!(:project_snippet) { create(:project_snippet, :private, project: project, author: user) } From d7a2e92ca0ae7fba4898f2f8ab722033b0721ec9 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Wed, 21 Dec 2016 15:02:05 +0000 Subject: [PATCH 3/3] applies url_for so that we dont lose filters when redirecting --- app/controllers/dashboard/todos_controller.rb | 2 +- app/controllers/projects/issues_controller.rb | 2 +- app/controllers/projects/merge_requests_controller.rb | 2 +- spec/controllers/dashboard/todos_controller_spec.rb | 3 ++- spec/controllers/projects/issues_controller_spec.rb | 5 +++-- .../projects/merge_requests_controller_spec.rb | 8 ++++---- spec/controllers/projects/snippets_controller_spec.rb | 2 +- 7 files changed, 13 insertions(+), 11 deletions(-) diff --git a/app/controllers/dashboard/todos_controller.rb b/app/controllers/dashboard/todos_controller.rb index 40652129f4c..e3933e3d7b1 100644 --- a/app/controllers/dashboard/todos_controller.rb +++ b/app/controllers/dashboard/todos_controller.rb @@ -5,7 +5,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController @sort = params[:sort] @todos = @todos.page(params[:page]) if @todos.out_of_range? && @todos.total_pages != 0 - redirect_to dashboard_todos_path(page: @todos.total_pages) + redirect_to url_for(params.merge(page: @todos.total_pages)) end end diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 0efeec98570..2beb0df8a07 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -26,7 +26,7 @@ class Projects::IssuesController < Projects::ApplicationController @issues = issues_collection @issues = @issues.page(params[:page]) if @issues.out_of_range? && @issues.total_pages != 0 - return redirect_to namespace_project_issues_path(page: @issues.total_pages) + return redirect_to url_for(params.merge(page: @issues.total_pages)) end if params[:label_name].present? diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 050d0ca77ae..fc8a289d49d 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -39,7 +39,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @merge_requests = merge_requests_collection @merge_requests = @merge_requests.page(params[:page]) if @merge_requests.out_of_range? && @merge_requests.total_pages != 0 - return redirect_to namespace_project_merge_requests_path(page: @merge_requests.total_pages) + return redirect_to url_for(params.merge(page: @merge_requests.total_pages)) end if params[:label_name].present? diff --git a/spec/controllers/dashboard/todos_controller_spec.rb b/spec/controllers/dashboard/todos_controller_spec.rb index a8c08a6e87b..288984cfba9 100644 --- a/spec/controllers/dashboard/todos_controller_spec.rb +++ b/spec/controllers/dashboard/todos_controller_spec.rb @@ -13,10 +13,11 @@ describe Dashboard::TodosController do context 'when using pagination' do let(:last_page) { user.todos.page().total_pages } - let!(:issues) { create_list(:issue, 30, project: project, assignee: user) } + let!(:issues) { create_list(:issue, 2, project: project, assignee: user) } before do issues.each { |issue| todo_service.new_issue(issue, user) } + allow(Kaminari.config).to receive(:default_per_page).and_return(1) end it 'redirects to last_page if page number is larger than number of pages' do diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 60b21cb4ace..e2321f2034b 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -55,11 +55,12 @@ describe Projects::IssuesController do context 'with page param' do let(:last_page) { project.issues.page().total_pages } - let!(:issue_list) { create_list(:issue, 30, project: project) } + let!(:issue_list) { create_list(:issue, 2, project: project) } before do sign_in(user) project.team << [user, :developer] + allow(Kaminari.config).to receive(:default_per_page).and_return(1) end it 'redirects to last_page if page number is larger than number of pages' do @@ -68,7 +69,7 @@ describe Projects::IssuesController do project_id: project.path.to_param, page: (last_page + 1).to_param - expect(response).to redirect_to(namespace_project_issues_path(page: last_page)) + expect(response).to redirect_to(namespace_project_issues_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope])) end it 'redirects to specified page' do diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 92392172af0..2a411d78395 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -127,9 +127,6 @@ describe Projects::MergeRequestsController do end describe 'GET index' do - let(:last_page) { project.merge_requests.page().total_pages } - let!(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } - def get_merge_requests(page = nil) get :index, namespace_id: project.namespace.to_param, @@ -138,10 +135,13 @@ describe Projects::MergeRequestsController do end context 'when page param' do + let(:last_page) { project.merge_requests.page().total_pages } + let!(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) } + it 'redirects to last_page if page number is larger than number of pages' do get_merge_requests(last_page + 1) - expect(response).to redirect_to(namespace_project_merge_requests_path(page: last_page)) + expect(response).to redirect_to(namespace_project_merge_requests_path(page: last_page, state: controller.params[:state], scope: controller.params[:scope])) end it 'redirects to specified page' do diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb index 82e57e4d7dc..32b0e42c3cd 100644 --- a/spec/controllers/projects/snippets_controller_spec.rb +++ b/spec/controllers/projects/snippets_controller_spec.rb @@ -26,7 +26,7 @@ describe Projects::SnippetsController do it 'redirects to specified page' do get :index, namespace_id: project.namespace.path, - project_id: project.path, page: (last_page).to_param + project_id: project.path, page: last_page.to_param expect(assigns(:snippets).current_page).to eq(last_page) expect(response).to have_http_status(200)