Skip to content

DEV: allows chat thread sdk to query more messages #32914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
2 changes: 1 addition & 1 deletion plugins/chat/app/queries/chat/messages_query.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down
8 changes: 0 additions & 8 deletions plugins/chat/spec/lib/chat_sdk/thread_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
6 changes: 6 additions & 0 deletions plugins/chat/spec/queries/chat/messages_query_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 15 additions & 14 deletions plugins/chat/spec/system/bookmark_message_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading