Commit 39993d82 authored by Robert May's avatar Robert May

Further refactoring for PageLimiter concern

Further simplifies the interface and removes some duplication.
parent 22ec2dc8
...@@ -12,13 +12,13 @@ ...@@ -12,13 +12,13 @@
# end # end
# #
# # You can override the default response # # You can override the default response
# def page_out_of_bounds # rescue_from PageOutOfBoundsError, with: :page_out_of_bounds
#
# def page_out_of_bounds(error)
# # Page limit number is available as error.message
# head :ok # head :ok
# end # end
# #
# # Or provide an entirely different handler:
# rescue_from PageOutOfBoundsError, with: :something
#
module PageLimiter module PageLimiter
extend ActiveSupport::Concern extend ActiveSupport::Concern
...@@ -29,39 +29,24 @@ module PageLimiter ...@@ -29,39 +29,24 @@ module PageLimiter
PageOutOfBoundsError = Class.new(PageLimiterError) PageOutOfBoundsError = Class.new(PageLimiterError)
included do included do
attr_accessor :max_page_number rescue_from PageOutOfBoundsError, with: :default_page_out_of_bounds_response
helper_method :max_page_number
rescue_from PageOutOfBoundsError, with: :page_out_of_bounds
end
def limit_pages(number)
set_max_page_number(number)
check_page_number
end end
# Override this method in your controller to customize the response def limit_pages(max_page_number)
def page_out_of_bounds check_page_number!(max_page_number)
default_page_out_of_bounds_response
end end
private private
def set_max_page_number(value) # If the page exceeds the defined maximum, raise a PageOutOfBoundsError
raise PageLimitNotANumberError unless value.is_a?(Integer) # If the page doesn't exceed the limit, it does nothing.
raise PageLimitNotSensibleError unless value > 0 def check_page_number!(max_page_number)
raise PageLimitNotANumberError unless max_page_number.is_a?(Integer)
self.max_page_number = value raise PageLimitNotSensibleError unless max_page_number > 0
end
# If the page exceeds the defined maximum, either call the provided
# block (if provided) or call the #page_out_of_bounds method to
# provide a response.
#
# If the page doesn't exceed the limit, it yields the controller action.
def check_page_number
if params[:page].present? && params[:page].to_i > max_page_number if params[:page].present? && params[:page].to_i > max_page_number
record_interception record_page_limit_interception
raise PageOutOfBoundsError raise PageOutOfBoundsError.new(max_page_number)
end end
end end
...@@ -71,7 +56,7 @@ module PageLimiter ...@@ -71,7 +56,7 @@ module PageLimiter
end end
# Record the page limit being hit in Prometheus # Record the page limit being hit in Prometheus
def record_interception def record_page_limit_interception
dd = DeviceDetector.new(request.user_agent) dd = DeviceDetector.new(request.user_agent)
Gitlab::Metrics.counter(:gitlab_page_out_of_bounds, Gitlab::Metrics.counter(:gitlab_page_out_of_bounds,
......
...@@ -10,6 +10,7 @@ class Explore::ProjectsController < Explore::ApplicationController ...@@ -10,6 +10,7 @@ class Explore::ProjectsController < Explore::ApplicationController
before_action :set_non_archived_param before_action :set_non_archived_param
before_action :set_sorting before_action :set_sorting
# Limit taken from https://gitlab.com/gitlab-org/gitlab/issues/38357
before_action only: [:index, :trending, :starred] do before_action only: [:index, :trending, :starred] do
limit_pages(200) limit_pages(200)
end end
...@@ -92,9 +93,9 @@ class Explore::ProjectsController < Explore::ApplicationController ...@@ -92,9 +93,9 @@ class Explore::ProjectsController < Explore::ApplicationController
Project::SORTING_PREFERENCE_FIELD Project::SORTING_PREFERENCE_FIELD
end end
# Overrides the default in the PageLimiter concern def page_out_of_bounds(error)
def page_out_of_bounds
load_project_counts load_project_counts
@max_page_number = error.message
respond_to do |format| respond_to do |format|
format.html do format.html do
......
...@@ -18,4 +18,4 @@ ...@@ -18,4 +18,4 @@
%h5= _("Maximum page reached") %h5= _("Maximum page reached")
%p= _("Sorry, you have exceeded the maximum browsable page number. Please use the API to explore further.") %p= _("Sorry, you have exceeded the maximum browsable page number. Please use the API to explore further.")
= link_to _("Back to page %{number}") % { number: max_page_number }, request.params.merge(page: max_page_number), class: 'btn btn-inverted' = link_to _("Back to page %{number}") % { number: @max_page_number }, request.params.merge(page: @max_page_number), class: 'btn btn-inverted'
...@@ -57,7 +57,7 @@ describe PageLimiter do ...@@ -57,7 +57,7 @@ describe PageLimiter do
it "returns the expected result" do it "returns the expected result" do
if result == PageLimiter::PageOutOfBoundsError if result == PageLimiter::PageOutOfBoundsError
expect(instance).to receive(:record_interception) expect(instance).to receive(:record_page_limit_interception)
expect { subject }.to raise_error(result) expect { subject }.to raise_error(result)
elsif result&.superclass == PageLimiter::PageLimiterError elsif result&.superclass == PageLimiter::PageLimiterError
expect { subject }.to raise_error(result) expect { subject }.to raise_error(result)
...@@ -68,18 +68,6 @@ describe PageLimiter do ...@@ -68,18 +68,6 @@ describe PageLimiter do
end end
end end
describe "#page_out_of_bounds" do
subject { instance.page_out_of_bounds }
after do
subject
end
it "returns a bad_request header" do
expect(instance).to receive(:head).with(:bad_request)
end
end
describe "#default_page_out_of_bounds_response" do describe "#default_page_out_of_bounds_response" do
subject { instance.send(:default_page_out_of_bounds_response) } subject { instance.send(:default_page_out_of_bounds_response) }
...@@ -92,8 +80,8 @@ describe PageLimiter do ...@@ -92,8 +80,8 @@ describe PageLimiter do
end end
end end
describe "#record_interception" do describe "#record_page_limit_interception" do
subject { instance.send(:record_interception) } subject { instance.send(:record_page_limit_interception) }
it "records a metric counter" do it "records a metric counter" do
expect(Gitlab::Metrics).to receive(:counter).with( expect(Gitlab::Metrics).to receive(:counter).with(
......
...@@ -73,6 +73,10 @@ describe Explore::ProjectsController do ...@@ -73,6 +73,10 @@ describe Explore::ProjectsController do
it { is_expected.to respond_with(:bad_request) } it { is_expected.to respond_with(:bad_request) }
it { is_expected.to render_template("explore/projects/page_out_of_bounds") } it { is_expected.to render_template("explore/projects/page_out_of_bounds") }
it "assigns the page number" do
expect(assigns[:max_page_number]).to eq(page_limit.to_s)
end
end end
describe "GET #{endpoint}.json" do describe "GET #{endpoint}.json" do
......
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