diff --git a/plugins/chat/app/controllers/chat/api/channel_thread_messages_controller.rb b/plugins/chat/app/controllers/chat/api/channel_thread_messages_controller.rb index d5b4e5a4331f8..53866b9875dfd 100644 --- a/plugins/chat/app/controllers/chat/api/channel_thread_messages_controller.rb +++ b/plugins/chat/app/controllers/chat/api/channel_thread_messages_controller.rb @@ -1,8 +1,12 @@ # frozen_string_literal: true class Chat::Api::ChannelThreadMessagesController < Chat::ApiController + MAX_PAGE_SIZE = 50 + def index - ::Chat::ListChannelThreadMessages.call(service_params) do |result| + ::Chat::ListChannelThreadMessages.call( + service_params.deep_merge(params: { page_size: MAX_PAGE_SIZE }), + ) do |result| on_success do render_serialized( result, diff --git a/plugins/chat/app/queries/chat/messages_query.rb b/plugins/chat/app/queries/chat/messages_query.rb index 5b1d2b82d9b66..40b41c2090f21 100644 --- a/plugins/chat/app/queries/chat/messages_query.rb +++ b/plugins/chat/app/queries/chat/messages_query.rb @@ -21,7 +21,7 @@ class MessagesQuery PAST = "past" FUTURE = "future" VALID_DIRECTIONS = [PAST, FUTURE] - MAX_PAGE_SIZE = 50 + MAX_PAGE_SIZE = 500 # @param channel [Chat::Channel] The channel to query messages within. # @param guardian [Guardian] The guardian to use for permission checks. diff --git a/plugins/chat/app/services/chat/list_channel_thread_messages.rb b/plugins/chat/app/services/chat/list_channel_thread_messages.rb index 4b32e1da75224..db66866383211 100644 --- a/plugins/chat/app/services/chat/list_channel_thread_messages.rb +++ b/plugins/chat/app/services/chat/list_channel_thread_messages.rb @@ -34,12 +34,7 @@ class ListChannelThreadMessages in: Chat::MessagesQuery::VALID_DIRECTIONS, }, allow_nil: true - validates :page_size, - numericality: { - less_than_or_equal_to: Chat::MessagesQuery::MAX_PAGE_SIZE, - only_integer: true, - }, - allow_nil: true + validates :page_size, numericality: { only_integer: true }, allow_nil: true end model :thread @@ -94,7 +89,7 @@ def fetch_messages(thread:, guardian:, params:) guardian: guardian, target_message_id: context.target_message_id, thread_id: thread.id, - page_size: params.page_size || Chat::MessagesQuery::MAX_PAGE_SIZE, + page_size: params.page_size, direction: params.direction, target_date: params.target_date, include_target_message_id: diff --git a/plugins/chat/spec/lib/chat_sdk/thread_spec.rb b/plugins/chat/spec/lib/chat_sdk/thread_spec.rb index 2483e6f3ffe97..94438a420179e 100644 --- a/plugins/chat/spec/lib/chat_sdk/thread_spec.rb +++ b/plugins/chat/spec/lib/chat_sdk/thread_spec.rb @@ -126,14 +126,6 @@ end end - context "when page_size is too large" do - it "fails" do - expect { described_class.messages(**params, page_size: 9999) }.to raise_error( - "Page size must be less than or equal to 50", - ) - end - end - context "when target_message doesn’t exist" do it "fails" do expect { described_class.messages(**params, target_message_id: -999) }.to raise_error( diff --git a/plugins/chat/spec/queries/chat/messages_query_spec.rb b/plugins/chat/spec/queries/chat/messages_query_spec.rb index fbd36de1409e9..2164525c9d4c9 100644 --- a/plugins/chat/spec/queries/chat/messages_query_spec.rb +++ b/plugins/chat/spec/queries/chat/messages_query_spec.rb @@ -106,6 +106,12 @@ stub_const(described_class, "MAX_PAGE_SIZE", 1) { expect(query[:messages].length).to eq(1) } end + it "limits results to MAX_PAGE_SIZE" do + options[:page_size] = 2 + options[:target_message_id] = nil + stub_const(described_class, "MAX_PAGE_SIZE", 1) { expect(query[:messages].length).to eq(1) } + end + describe "when some messages are in threads" do fab!(:thread) { Fabricate(:chat_thread, channel: channel) } diff --git a/plugins/chat/spec/requests/chat/api/channel_thread_messages_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channel_thread_messages_controller_spec.rb index 46eb2f4d7f9f9..792961bc7722e 100644 --- a/plugins/chat/spec/requests/chat/api/channel_thread_messages_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/channel_thread_messages_controller_spec.rb @@ -60,9 +60,22 @@ end end + context "when page_size is a above limit" do + fab!(:message_3) { Fabricate(:chat_message, thread: thread, chat_channel: thread.channel) } + + it "clamps it to the max" do + stub_const(Chat::Api::ChannelThreadMessagesController, "MAX_PAGE_SIZE", 1) do + get "/chat/api/channels/#{thread.channel.id}/threads/#{thread.id}/messages?page_size=9999" + + expect(response.status).to eq(200) + expect(response.parsed_body["messages"].length).to eq(1) + end + end + end + context "when params are invalid" do it "returns a 400" do - get "/chat/api/channels/#{thread.channel.id}/threads/#{thread.id}/messages?page_size=9999" + get "/chat/api/channels/#{thread.channel.id}/threads/#{thread.id}/messages?direction=yolo" expect(response.status).to eq(400) end diff --git a/plugins/chat/spec/services/chat/list_channel_thread_messages_spec.rb b/plugins/chat/spec/services/chat/list_channel_thread_messages_spec.rb index 90357a0fa406d..5e766d2492863 100644 --- a/plugins/chat/spec/services/chat/list_channel_thread_messages_spec.rb +++ b/plugins/chat/spec/services/chat/list_channel_thread_messages_spec.rb @@ -11,7 +11,6 @@ it do is_expected.to allow_values(Chat::MessagesQuery::MAX_PAGE_SIZE, 1, "1", nil).for(:page_size) end - it { is_expected.not_to allow_values(Chat::MessagesQuery::MAX_PAGE_SIZE + 1).for(:page_size) } end describe ".call" do diff --git a/plugins/chat/spec/system/bookmark_message_spec.rb b/plugins/chat/spec/system/bookmark_message_spec.rb index 235acc3a6b988..1655657cfc27a 100644 --- a/plugins/chat/spec/system/bookmark_message_spec.rb +++ b/plugins/chat/spec/system/bookmark_message_spec.rb @@ -28,25 +28,26 @@ expect(channel_page).to have_bookmarked_message(message_1) end + context "when in a long thread" do + it "supports linking to a bookmark in a long thread" do + category_channel_1.update!(threading_enabled: true) + category_channel_1.add(current_user) - it "supports linking to a bookmark in a long thread" do - category_channel_1.update!(threading_enabled: true) - category_channel_1.add(current_user) + thread = + chat_thread_chain_bootstrap( + channel: category_channel_1, + users: [current_user, Fabricate(:user)], + messages_count: 51, + ) - thread = - chat_thread_chain_bootstrap( - channel: category_channel_1, - users: [current_user, Fabricate(:user)], - messages_count: Chat::MessagesQuery::MAX_PAGE_SIZE + 1, - ) - - first_message = thread.replies.first + first_message = thread.replies.first - bookmark = Bookmark.create!(bookmarkable: first_message, user: current_user) + bookmark = Bookmark.create!(bookmarkable: first_message, user: current_user) - visit bookmark.bookmarkable.url + visit bookmark.bookmarkable.url - expect(thread_page).to have_bookmarked_message(first_message) + expect(thread_page).to have_bookmarked_message(first_message) + end end context "in drawer mode" do