diff --git a/app/controllers/list_controller.rb b/app/controllers/list_controller.rb index 77ed9bf9aa4..ef1bee18c17 100644 --- a/app/controllers/list_controller.rb +++ b/app/controllers/list_controller.rb @@ -322,7 +322,8 @@ class ListController < ApplicationController define_method("top_#{period}") do |options = nil| top_options = build_topic_list_options top_options.merge!(options) if options - top_options[:per_page] = SiteSetting.topics_per_period_in_top_page + top_options[:per_page] = top_options[:per_page].presence || + SiteSetting.topics_per_period_in_top_page user = list_target_user list = TopicQuery.new(user, top_options).list_top_for(period) diff --git a/lib/topic_query.rb b/lib/topic_query.rb index 59660b970b9..5b6577add20 100644 --- a/lib/topic_query.rb +++ b/lib/topic_query.rb @@ -16,12 +16,14 @@ class TopicQuery begin int = lambda { |x| Integer === x || (String === x && x.match?(/\A-?[0-9]+\z/)) } zero_up_to_max_int = lambda { |x| int.call(x) && x.to_i.between?(0, PG_MAX_INT) } + one_up_to_one_hundred = lambda { |x| int.call(x) && x.to_i.between?(1, 100) } array_or_string = lambda { |x| Array === x || String === x } string = lambda { |x| String === x } true_or_false = lambda { |x| x == true || x == false || x == "true" || x == "false" } { page: zero_up_to_max_int, + per_page: one_up_to_one_hundred, before: zero_up_to_max_int, bumped_before: zero_up_to_max_int, topic_ids: array_or_string, @@ -59,6 +61,7 @@ class TopicQuery # For these to work in Ember, add them to `controllers/discovery/list.js` @public_valid_options ||= %i[ page + per_page before bumped_before topic_ids @@ -89,8 +92,6 @@ class TopicQuery %i[ except_topic_ids limit - page - per_page visible guardian no_definitions @@ -503,7 +504,7 @@ class TopicQuery unpinned_topics = topics.where("NOT ( #{pinned_clause} )") pinned_topics = topics.dup.offset(nil).where(pinned_clause).reorder(pinned_at: :desc) - per_page = options[:per_page] || per_page_setting + per_page = options[:per_page]&.to_i || per_page_setting limit = per_page unless options[:limit] == false page = options[:page].to_i @@ -561,7 +562,7 @@ class TopicQuery end list = TopicList.new(filter, @user, topics, options.merge(@options)) - list.per_page = options[:per_page] || per_page_setting + list.per_page = options[:per_page]&.to_i || per_page_setting list end @@ -836,7 +837,7 @@ class TopicQuery result = result.where("COALESCE(categories.topic_id, 0) <> topics.id") end - result = result.limit(options[:per_page]) unless options[:limit] == false + result = result.limit(options[:per_page]&.to_i) unless options[:limit] == false result = result.visible if options[:visible] result = result.where.not(topics: { id: options[:except_topic_ids] }).references(:topics) if options[ @@ -844,7 +845,7 @@ class TopicQuery ] if options[:page] - offset = options[:page].to_i * options[:per_page] + offset = options[:page].to_i * options[:per_page]&.to_i result = result.offset(offset) if offset > 0 end diff --git a/spec/lib/topic_query_spec.rb b/spec/lib/topic_query_spec.rb index 11af5744152..05cbb013b8c 100644 --- a/spec/lib/topic_query_spec.rb +++ b/spec/lib/topic_query_spec.rb @@ -89,6 +89,24 @@ RSpec.describe TopicQuery do end end + describe ".validate?" do + describe "per_page" do + it "only allows integers 1-100" do + # Invalid values + expect(TopicQuery.validate?(:per_page, -1)).to eq(false) + expect(TopicQuery.validate?(:per_page, 0)).to eq(false) + expect(TopicQuery.validate?(:per_page, 101)).to eq(false) + expect(TopicQuery.validate?(:per_page, "invalid")).to eq(false) + expect(TopicQuery.validate?(:per_page, [])).to eq(false) + + # Valid values + expect(TopicQuery.validate?(:per_page, 1)).to eq(true) + expect(TopicQuery.validate?(:per_page, 100)).to eq(true) + expect(TopicQuery.validate?(:per_page, "10")).to eq(true) + end + end + end + describe "#list_topics_by" do it "allows users to view their own invisible topics" do _topic = Fabricate(:topic, user: user) diff --git a/spec/requests/api/topics_spec.rb b/spec/requests/api/topics_spec.rb index df80458a237..141a9f15b4a 100644 --- a/spec/requests/api/topics_spec.rb +++ b/spec/requests/api/topics_spec.rb @@ -519,6 +519,12 @@ RSpec.describe "topics" do type: :string, description: "Defaults to `desc`, add `ascending=true` to sort asc", ) + parameter( + name: :per_page, + in: :query, + type: :integer, + description: "Maximum number of topics returned, between 1-100", + ) produces "application/json" response "200", "topic updated" do @@ -697,6 +703,7 @@ RSpec.describe "topics" do let(:order) { "default" } let(:ascending) { "false" } + let(:per_page) { 20 } run_test! end @@ -716,6 +723,12 @@ RSpec.describe "topics" do type: :string, description: "Enum: `all`, `yearly`, `quarterly`, `monthly`, `weekly`, `daily`", ) + parameter( + name: :per_page, + in: :query, + type: :integer, + description: "Maximum number of topics returned, between 1-100", + ) produces "application/json" response "200", "response" do @@ -896,6 +909,7 @@ RSpec.describe "topics" do } let(:period) { "all" } + let(:per_page) { 20 } run_test! end diff --git a/spec/requests/list_controller_spec.rb b/spec/requests/list_controller_spec.rb index 93c0b69c3a6..8c5afc40b28 100644 --- a/spec/requests/list_controller_spec.rb +++ b/spec/requests/list_controller_spec.rb @@ -726,6 +726,21 @@ RSpec.describe ListController do get "/top?period=decadely" expect(response.status).to eq(400) end + + describe "per page" do + it "uses value from params when present" do + get "/top.json?per_page=5" + + expect(response.parsed_body["topic_list"]["per_page"]).to eq(5) + end + + it "uses SiteSetting.topics_per_period_in_top_page when per_page param isn't present" do + SiteSetting.topics_per_period_in_top_page = 32 + get "/top.json" + + expect(response.parsed_body["topic_list"]["per_page"]).to eq(32) + end + end end describe "category" do