diff --git a/plugins/chat/app/controllers/chat/chat_controller.rb b/plugins/chat/app/controllers/chat/chat_controller.rb index f49bbbe95e8..57ac5ab7bd3 100644 --- a/plugins/chat/app/controllers/chat/chat_controller.rb +++ b/plugins/chat/app/controllers/chat/chat_controller.rb @@ -12,14 +12,13 @@ module Chat # these endpoints require a standalone find because they need to be # able to get deleted channels and recover them. before_action :find_chatable, only: %i[enable_chat disable_chat] - before_action :find_chat_message, only: %i[lookup_message edit_message rebake message_link] + before_action :find_chat_message, only: %i[edit_message rebake message_link] before_action :set_channel_and_chatable_with_access_check, except: %i[ respond enable_chat disable_chat message_link - lookup_message set_user_chat_status dismiss_retention_reminder flag @@ -169,59 +168,6 @@ module Chat render json: success_json end - # TODO: (martin) This endpoint is deprecated -- the /api/channels/:id?include_messages=true - # endpoint should be used instead. We need to remove this and any JS references. - def messages - page_size = params[:page_size]&.to_i || 1000 - direction = params[:direction].to_s - message_id = params[:message_id] - if page_size > 100 || - ( - message_id.blank? ^ direction.blank? && - (direction.present? && !CHAT_DIRECTIONS.include?(direction)) - ) - raise Discourse::InvalidParameters - end - - messages = preloaded_chat_message_query.where(chat_channel: @chat_channel) - messages = messages.with_deleted if guardian.can_moderate_chat?(@chatable) - messages = messages.where(thread_id: params[:thread_id]) if params[:thread_id] - messages = exclude_thread_messages(messages) if !include_thread_messages? - - if message_id.present? - condition = direction == PAST ? "<" : ">" - messages = messages.where("id #{condition} ?", message_id.to_i) - end - - # NOTE: This order is reversed when we return the Chat::View below if the direction - # is not FUTURE. - order = direction == FUTURE ? "ASC" : "DESC" - messages = messages.order("created_at #{order}, id #{order}").limit(page_size).to_a - - can_load_more_past = nil - can_load_more_future = nil - - if direction == FUTURE - can_load_more_future = messages.size == page_size - elsif direction == PAST - can_load_more_past = messages.size == page_size - else - # When direction is blank, we'll return the latest messages. - can_load_more_future = false - can_load_more_past = messages.size == page_size - end - - chat_view = - Chat::View.new( - chat_channel: @chat_channel, - chat_messages: direction == FUTURE ? messages : messages.reverse, - user: current_user, - can_load_more_past: can_load_more_past, - can_load_more_future: can_load_more_future, - ) - render_serialized(chat_view, Chat::ViewSerializer, root: false) - end - def react params.require(%i[message_id emoji react_action]) guardian.ensure_can_react! @@ -252,43 +198,6 @@ module Chat ) end - # TODO: (martin) This endpoint is deprecated -- the /api/channels/:id?target_message_id=:message_id - # endpoint should be used instead. We need to remove this and any JS references. - def lookup_message - set_channel_and_chatable_with_access_check(chat_channel_id: @message.chat_channel_id) - - messages = preloaded_chat_message_query.where(chat_channel: @chat_channel) - messages = messages.with_deleted if guardian.can_moderate_chat?(@chatable) - messages = messages.where(thread_id: params[:thread_id]) if params[:thread_id] - messages = exclude_thread_messages(messages) if !include_thread_messages? - - past_messages = - messages - .where("created_at < ?", @message.created_at) - .order(created_at: :desc) - .limit(PAST_MESSAGE_LIMIT) - - future_messages = - messages - .where("created_at > ?", @message.created_at) - .order(created_at: :asc) - .limit(FUTURE_MESSAGE_LIMIT) - - can_load_more_past = past_messages.count == PAST_MESSAGE_LIMIT - can_load_more_future = future_messages.count == FUTURE_MESSAGE_LIMIT - looked_up_message = !include_thread_messages? && @message.thread_reply? ? [] : [@message] - messages = [past_messages.reverse, looked_up_message, future_messages].reduce([], :concat) - chat_view = - Chat::View.new( - chat_channel: @chat_channel, - chat_messages: messages, - user: current_user, - can_load_more_past: can_load_more_past, - can_load_more_future: can_load_more_future, - ) - render_serialized(chat_view, Chat::ViewSerializer, root: false) - end - def set_user_chat_status params.require(:chat_enabled) @@ -423,11 +332,6 @@ module Chat query end - def include_thread_messages? - params[:thread_id].present? || !SiteSetting.enable_experimental_chat_threaded_discussions || - !@chat_channel.threading_enabled - end - def find_chatable @chatable = Category.find_by(id: params[:chatable_id]) guardian.ensure_can_moderate_chat!(@chatable) @@ -441,15 +345,5 @@ module Chat @message = @message.find_by(id: params[:message_id]) raise Discourse::NotFound unless @message end - - def exclude_thread_messages(messages) - messages.where(<<~SQL, channel_id: @chat_channel.id) - chat_messages.thread_id IS NULL OR chat_messages.id IN ( - SELECT original_message_id - FROM chat_threads - WHERE chat_threads.channel_id = :channel_id - ) - SQL - end end end diff --git a/plugins/chat/app/queries/chat/messages_query.rb b/plugins/chat/app/queries/chat/messages_query.rb index c733fb887a0..a5746037b68 100644 --- a/plugins/chat/app/queries/chat/messages_query.rb +++ b/plugins/chat/app/queries/chat/messages_query.rb @@ -21,6 +21,7 @@ module Chat PAST = "past" FUTURE = "future" VALID_DIRECTIONS = [PAST, FUTURE] + MAX_PAGE_SIZE = 100 # @param channel [Chat::Channel] The channel to query messages within. # @param guardian [Guardian] The guardian to use for permission checks. @@ -57,10 +58,10 @@ module Chat ) SQL - if target_message_id.present? + if target_message_id.present? && direction.blank? query_around_target(target_message_id, channel, messages) else - query_paginated_messages(direction, page_size, channel, messages) + query_paginated_messages(direction, page_size, channel, messages, target_message_id) end end @@ -117,7 +118,18 @@ module Chat } end - def self.query_paginated_messages(direction, page_size, channel, messages) + def self.query_paginated_messages( + direction, + page_size, + channel, + messages, + target_message_id = nil + ) + if target_message_id.present? + condition = direction == PAST ? "<" : ">" + messages = messages.where("id #{condition} ?", target_message_id.to_i) + end + order = direction == FUTURE ? "ASC" : "DESC" messages = messages.order("created_at #{order}, id #{order}").limit(page_size).to_a diff --git a/plugins/chat/app/serializers/chat/thread_serializer.rb b/plugins/chat/app/serializers/chat/thread_serializer.rb index 8c01dc01601..66c73d315c2 100644 --- a/plugins/chat/app/serializers/chat/thread_serializer.rb +++ b/plugins/chat/app/serializers/chat/thread_serializer.rb @@ -19,7 +19,7 @@ module Chat @opts = opts # Avoids an N1 to re-load the thread in the serializer for original_message. - object.original_message.thread = object + object.original_message&.thread = object @current_user_membership = opts[:membership] end diff --git a/plugins/chat/app/services/chat/channel_view_builder.rb b/plugins/chat/app/services/chat/channel_view_builder.rb index 61861eaa910..bf519c3ece1 100644 --- a/plugins/chat/app/services/chat/channel_view_builder.rb +++ b/plugins/chat/app/services/chat/channel_view_builder.rb @@ -57,6 +57,18 @@ module Chat 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 + + validate :page_size_present, if: -> { target_message_id.blank? && !fetch_from_last_read } + + def page_size_present + errors.add(:page_size, :blank) if page_size.blank? + end end private @@ -77,7 +89,11 @@ module Chat def target_message_exists(contract:, guardian:, **) return true if contract.target_message_id.blank? - target_message = Chat::Message.unscoped.find_by(id: contract.target_message_id) + target_message = + Chat::Message.with_deleted.find_by( + id: contract.target_message_id, + chat_channel_id: contract.channel_id, + ) return false if target_message.blank? return true if !target_message.trashed? target_message.user_id == guardian.user.id || guardian.is_staff? diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js index cd2d59b2ddb..4fe4574f98a 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js @@ -266,11 +266,11 @@ export default class ChatLivePane extends Component { }; return this.chatApi - .messages(this.args.channel.id, findArgs) - .then((results) => { + .channel(this.args.channel.id, findArgs) + .then((result) => { if ( this._selfDeleted || - this.args.channel.id !== results.meta.channel_id || + this.args.channel.id !== result.meta.channel_id || !this.scrollable ) { return; @@ -284,14 +284,29 @@ export default class ChatLivePane extends Component { const [messages, meta] = this.afterFetchCallback( this.args.channel, - results + result ); + if (result.threads) { + result.threads.forEach((thread) => { + this.args.channel.threadsManager.store(this.args.channel, thread); + }); + } + + if (result.thread_tracking_overview) { + result.thread_tracking_overview.forEach((threadId) => { + if (!this.args.channel.threadTrackingOverview.includes(threadId)) { + this.args.channel.threadTrackingOverview.push(threadId); + } + }); + } + + this.args.channel.details = meta; + if (!messages?.length) { return; } - this.args.channel.details = meta; this.args.channel.addMessages(messages); // Edge case for IOS to avoid blank screens @@ -331,11 +346,11 @@ export default class ChatLivePane extends Component { } @bind - afterFetchCallback(channel, results) { + afterFetchCallback(channel, result) { const messages = []; let foundFirstNew = false; - results.chat_messages.forEach((messageData, index) => { + result.chat_messages.forEach((messageData, index) => { if (index === 0) { messageData.firstOfResults = true; } @@ -378,7 +393,7 @@ export default class ChatLivePane extends Component { messages.push(message); }); - return [messages, results.meta]; + return [messages, result.meta]; } @debounce(100) diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-thread.js b/plugins/chat/assets/javascripts/discourse/components/chat-thread.js index 5cda322ed60..1ee591f5a9b 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-thread.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-thread.js @@ -154,22 +154,22 @@ export default class ChatThreadPanel extends Component { this.loading = true; - const findArgs = { pageSize: PAGE_SIZE, threadId: this.thread.id }; + const findArgs = { + pageSize: PAGE_SIZE, + threadId: this.thread.id, + includeMessages: true, + }; return this.chatApi - .messages(this.channel.id, findArgs) - .then((results) => { - if (this._selfDeleted || this.channel.id !== results.meta.channel_id) { - this.router.transitionTo( - "chat.channel", - "-", - results.meta.channel_id - ); + .channel(this.channel.id, findArgs) + .then((result) => { + if (this._selfDeleted || this.channel.id !== result.meta.channel_id) { + this.router.transitionTo("chat.channel", "-", result.meta.channel_id); } const [messages, meta] = this.afterFetchCallback( this.channel, this.thread, - results + result ); this.thread.messagesManager.addMessages(messages); this.thread.details = meta; @@ -186,10 +186,10 @@ export default class ChatThreadPanel extends Component { } @bind - afterFetchCallback(channel, thread, results) { + afterFetchCallback(channel, thread, result) { const messages = []; - results.chat_messages.forEach((messageData) => { + result.chat_messages.forEach((messageData) => { // If a message has been hidden it is because the current user is ignoring // the user who sent it, so we want to unconditionally hide it, even if // we are going directly to the target @@ -205,7 +205,7 @@ export default class ChatThreadPanel extends Component { messages.push(message); }); - return [messages, results.meta]; + return [messages, result.meta]; } // NOTE: At some point we want to do this based on visible messages diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-api.js b/plugins/chat/assets/javascripts/discourse/services/chat-api.js index 012b4c03346..426c28357d7 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-api.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-api.js @@ -32,6 +32,10 @@ export default class ChatApi extends Service { } else { args.page_size = data.pageSize; + if (data.direction) { + args.direction = data.direction; + } + if (data.includeMessages) { args.include_messages = true; } @@ -273,43 +277,6 @@ export default class ChatApi extends Service { ); } - /** - * Returns messages of a channel, from the last message or a specified target. - * @param {number} channelId - The ID of the channel. - * @param {object} data - Params of the query. - * @param {integer} data.targetMessageId - ID of the targeted message. - * @param {integer} data.messageId - ID of the targeted message. - * @param {integer} data.direction - Fetch past or future messages. - * @param {integer} data.pageSize - Max number of messages to fetch. - * @returns {Promise} - */ - messages(channelId, data = {}) { - let path; - const args = {}; - - if (data.targetMessageId) { - path = `/chat/lookup/${data.targetMessageId}`; - args.chat_channel_id = channelId; - } else { - args.page_size = data.pageSize; - path = `/chat/${channelId}/messages`; - - if (data.messageId) { - args.message_id = data.messageId; - } - - if (data.direction) { - args.direction = data.direction; - } - - if (data.threadId) { - args.thread_id = data.threadId; - } - } - - return ajax(path, { data: args }); - } - /** * Update notifications settings of current user for a channel. * @param {number} channelId - The ID of the channel. diff --git a/plugins/chat/config/routes.rb b/plugins/chat/config/routes.rb index 736ad850e88..6ea17804cc3 100644 --- a/plugins/chat/config/routes.rb +++ b/plugins/chat/config/routes.rb @@ -58,14 +58,12 @@ Chat::Engine.routes.draw do post "/enable" => "chat#enable_chat" post "/disable" => "chat#disable_chat" post "/dismiss-retention-reminder" => "chat#dismiss_retention_reminder" - get "/:chat_channel_id/messages" => "chat#messages" get "/message/:message_id" => "chat#message_link" put ":chat_channel_id/edit/:message_id" => "chat#edit_message" put ":chat_channel_id/react/:message_id" => "chat#react" put "/:chat_channel_id/:message_id/rebake" => "chat#rebake" post "/:chat_channel_id/:message_id/flag" => "chat#flag" post "/:chat_channel_id/quote" => "chat#quote_messages" - get "/lookup/:message_id" => "chat#lookup_message" put "/:chat_channel_id/read/:message_id" => "chat#update_user_last_read" put "/user_chat_enabled/:user_id" => "chat#set_user_chat_status" put "/:chat_channel_id/invite" => "chat#invite_users" diff --git a/plugins/chat/spec/requests/chat/api/channels_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channels_controller_spec.rb index f22349422ab..fdb06a4f5b2 100644 --- a/plugins/chat/spec/requests/chat/api/channels_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/channels_controller_spec.rb @@ -159,6 +159,395 @@ RSpec.describe Chat::Api::ChannelsController do end end end + + context "when include_messages is true" do + fab!(:current_user) { Fabricate(:user) } + fab!(:channel_1) { Fabricate(:category_channel) } + fab!(:other_user) { Fabricate(:user) } + + describe "target message lookup" do + let!(:message) { Fabricate(:chat_message, chat_channel: channel_1) } + let(:chatable) { channel_1.chatable } + + before { sign_in(current_user) } + + context "when the message doesn’t belong to the channel" do + let!(:message) { Fabricate(:chat_message) } + + it "returns a 404" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + target_message_id: message.id, + include_messages: true, + } + + expect(response.status).to eq(404) + end + end + + context "when the chat channel is for a category" do + it "ensures the user can access that category" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + target_message_id: message.id, + include_messages: true, + } + expect(response.status).to eq(200) + expect(response.parsed_body["chat_messages"][0]["id"]).to eq(message.id) + + group = Fabricate(:group) + chatable.update!(read_restricted: true) + Fabricate(:category_group, group: group, category: chatable) + get "/chat/api/channels/#{channel_1.id}.json", + params: { + target_message_id: message.id, + include_messages: true, + } + expect(response.status).to eq(403) + + GroupUser.create!(user: current_user, group: group) + get "/chat/api/channels/#{channel_1.id}.json", + params: { + target_message_id: message.id, + include_messages: true, + } + expect(response.status).to eq(200) + expect(response.parsed_body["chat_messages"][0]["id"]).to eq(message.id) + end + end + + context "when the chat channel is for a direct message channel" do + let(:channel_1) { Fabricate(:direct_message_channel) } + + it "ensures the user can access that direct message channel" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + target_message_id: message.id, + include_messages: true, + } + expect(response.status).to eq(403) + + Chat::DirectMessageUser.create!(user: current_user, direct_message: chatable) + get "/chat/api/channels/#{channel_1.id}.json", + params: { + target_message_id: message.id, + include_messages: true, + } + expect(response.status).to eq(200) + expect(response.parsed_body["chat_messages"][0]["id"]).to eq(message.id) + end + end + end + + describe "messages pagination and direction" do + let(:page_size) { 30 } + + message_count = 70 + message_count.times do |n| + fab!("message_#{n}") do + Fabricate( + :chat_message, + chat_channel: channel_1, + user: other_user, + message: "message #{n}", + ) + end + end + + before do + sign_in(current_user) + Group.refresh_automatic_groups! + end + + it "errors for user when they are not allowed to chat" do + SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:staff] + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + expect(response.status).to eq(403) + end + + it "errors when page size is over the maximum" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: Chat::MessagesQuery::MAX_PAGE_SIZE + 1, + } + expect(response.status).to eq(400) + expect(response.parsed_body["errors"]).to include( + "Page size must be less than or equal to #{Chat::MessagesQuery::MAX_PAGE_SIZE}", + ) + end + + it "errors when page size is nil" do + get "/chat/api/channels/#{channel_1.id}.json", params: { include_messages: true } + expect(response.status).to eq(400) + expect(response.parsed_body["errors"]).to include("Page size can't be blank") + end + + it "returns the latest messages in created_at, id order" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + messages = response.parsed_body["chat_messages"] + expect(messages.count).to eq(page_size) + expect(messages.first["created_at"].to_time).to be < messages.last["created_at"].to_time + end + + it "returns `can_flag=true` for public channels" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + expect(response.parsed_body["meta"]["can_flag"]).to be true + end + + it "returns `can_flag=true` for DM channels" do + dm_chat_channel = Fabricate(:direct_message_channel, users: [current_user, other_user]) + get "/chat/api/channels/#{dm_chat_channel.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + expect(response.parsed_body["meta"]["can_flag"]).to be true + end + + it "returns `can_moderate=true` based on whether the user can moderate the chatable" do + 1.upto(4) do |n| + current_user.update!(trust_level: n) + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + expect(response.parsed_body["meta"]["can_moderate"]).to be false + end + + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + expect(response.parsed_body["meta"]["can_moderate"]).to be false + + current_user.update!(admin: true) + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + expect(response.parsed_body["meta"]["can_moderate"]).to be true + current_user.update!(admin: false) + + SiteSetting.enable_category_group_moderation = true + group = Fabricate(:group) + group.add(current_user) + channel_1.category.update!(reviewable_by_group: group) + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + expect(response.parsed_body["meta"]["can_moderate"]).to be true + end + + it "serializes `user_flag_status` for user who has a pending flag" do + chat_message = channel_1.chat_messages.last + reviewable = flag_message(chat_message, current_user) + score = reviewable.reviewable_scores.last + + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + + expect(response.parsed_body["chat_messages"].last["user_flag_status"]).to eq( + score.status_for_database, + ) + end + + it "doesn't serialize `reviewable_ids` for non-staff" do + reviewable = flag_message(channel_1.chat_messages.last, Fabricate(:admin)) + + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + + expect(response.parsed_body["chat_messages"].last["reviewable_id"]).to be_nil + end + + it "serializes `reviewable_ids` correctly for staff" do + admin = Fabricate(:admin) + sign_in(admin) + reviewable = flag_message(channel_1.chat_messages.last, admin) + + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + expect(response.parsed_body["chat_messages"].last["reviewable_id"]).to eq(reviewable.id) + end + + it "correctly marks reactions as 'reacted' for the current_user" do + heart_emoji = ":heart:" + smile_emoji = ":smile" + last_message = channel_1.chat_messages.last + last_message.reactions.create(user: current_user, emoji: heart_emoji) + last_message.reactions.create(user: Fabricate(:admin), emoji: smile_emoji) + + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + + reactions = response.parsed_body["chat_messages"].last["reactions"] + heart_reaction = reactions.find { |r| r["emoji"] == heart_emoji } + expect(heart_reaction["reacted"]).to be true + smile_reaction = reactions.find { |r| r["emoji"] == smile_emoji } + expect(smile_reaction["reacted"]).to be false + end + + it "sends the last message bus id for the channel" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + page_size: page_size, + } + expect(response.parsed_body["meta"]["channel_message_bus_last_id"]).not_to eq(nil) + end + + describe "scrolling to the past" do + it "returns the correct messages in created_at, id order" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + target_message_id: message_40.id, + page_size: page_size, + direction: Chat::MessagesQuery::PAST, + } + messages = response.parsed_body["chat_messages"] + expect(messages.count).to eq(page_size) + expect(messages.first["created_at"].to_time).to eq_time(message_10.created_at) + expect(messages.last["created_at"].to_time).to eq_time(message_39.created_at) + end + + it "returns 'can_load...' properly when there are more past messages" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + target_message_id: message_40.id, + page_size: page_size, + direction: Chat::MessagesQuery::PAST, + } + expect(response.parsed_body["meta"]["can_load_more_past"]).to be true + expect(response.parsed_body["meta"]["can_load_more_future"]).to be_nil + end + + it "returns 'can_load...' properly when there are no past messages" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + target_message_id: message_3.id, + page_size: page_size, + direction: Chat::MessagesQuery::PAST, + } + expect(response.parsed_body["meta"]["can_load_more_past"]).to be false + expect(response.parsed_body["meta"]["can_load_more_future"]).to be_nil + end + end + + describe "scrolling to the future" do + it "returns the correct messages in created_at, id order when there are many after" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + target_message_id: message_10.id, + page_size: page_size, + direction: Chat::MessagesQuery::FUTURE, + } + messages = response.parsed_body["chat_messages"] + expect(messages.count).to eq(page_size) + expect(messages.first["created_at"].to_time).to eq_time(message_11.created_at) + expect(messages.last["created_at"].to_time).to eq_time(message_40.created_at) + end + + it "return 'can_load..' properly when there are future messages" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + target_message_id: message_10.id, + page_size: page_size, + direction: Chat::MessagesQuery::FUTURE, + } + expect(response.parsed_body["meta"]["can_load_more_past"]).to be_nil + expect(response.parsed_body["meta"]["can_load_more_future"]).to be true + end + + it "returns 'can_load..' properly when there are no future messages" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + include_messages: true, + target_message_id: message_60.id, + page_size: page_size, + direction: Chat::MessagesQuery::FUTURE, + } + expect(response.parsed_body["meta"]["can_load_more_past"]).to be_nil + expect(response.parsed_body["meta"]["can_load_more_future"]).to be false + end + end + + describe "without direction (latest messages)" do + it "signals there are no future messages" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + page_size: page_size, + include_messages: true, + } + + expect(response.parsed_body["meta"]["can_load_more_future"]).to eq(false) + end + + it "signals there are more messages in the past" do + get "/chat/api/channels/#{channel_1.id}.json", + params: { + page_size: page_size, + include_messages: true, + } + + expect(response.parsed_body["meta"]["can_load_more_past"]).to eq(true) + end + + it "signals there are no more messages" do + new_channel = Fabricate(:category_channel) + Fabricate( + :chat_message, + chat_channel: new_channel, + user: other_user, + message: "message", + ) + chat_messages_qty = 1 + + get "/chat/api/channels/#{new_channel.id}.json", + params: { + page_size: chat_messages_qty + 1, + include_messages: true, + } + + expect(response.parsed_body["meta"]["can_load_more_past"]).to eq(false) + end + end + end + end end describe "#destroy" do @@ -569,4 +958,8 @@ RSpec.describe Chat::Api::ChannelsController do end end end + + def flag_message(message, flagger, flag_type: ReviewableScore.types[:off_topic]) + Chat::ReviewQueue.new.flag_message(message, Guardian.new(flagger), flag_type)[:reviewable] + end end diff --git a/plugins/chat/spec/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb index 31c95de2c30..2d2233499f2 100644 --- a/plugins/chat/spec/requests/chat_controller_spec.rb +++ b/plugins/chat/spec/requests/chat_controller_spec.rb @@ -32,272 +32,6 @@ RSpec.describe Chat::ChatController do Chat::ReviewQueue.new.flag_message(message, Guardian.new(flagger), flag_type)[:reviewable] end - describe "#messages" do - let(:page_size) { 30 } - - before do - sign_in(user) - Group.refresh_automatic_groups! - end - - it "errors for user when they are not allowed to chat" do - SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:staff] - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - expect(response.status).to eq(403) - end - - it "errors when page size is over 100" do - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: 200 } - expect(response.status).to eq(400) - end - - it "errors when page size is nil" do - get "/chat/#{chat_channel.id}/messages.json" - expect(response.status).to eq(400) - end - - it "returns the latest messages in created_at, id order" do - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - messages = response.parsed_body["chat_messages"] - expect(messages.count).to eq(page_size) - expect(messages.first["created_at"].to_time).to be < messages.last["created_at"].to_time - end - - it "returns `can_flag=true` for public channels" do - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - expect(response.parsed_body["meta"]["can_flag"]).to be true - end - - it "returns `can_flag=true` for DM channels" do - get "/chat/#{dm_chat_channel.id}/messages.json", params: { page_size: page_size } - expect(response.parsed_body["meta"]["can_flag"]).to be true - end - - it "returns `can_moderate=true` based on whether the user can moderate the chatable" do - 1.upto(4) do |n| - user.update!(trust_level: n) - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - expect(response.parsed_body["meta"]["can_moderate"]).to be false - end - - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - expect(response.parsed_body["meta"]["can_moderate"]).to be false - - user.update!(admin: true) - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - expect(response.parsed_body["meta"]["can_moderate"]).to be true - user.update!(admin: false) - - SiteSetting.enable_category_group_moderation = true - group = Fabricate(:group) - group.add(user) - category.update!(reviewable_by_group: group) - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - expect(response.parsed_body["meta"]["can_moderate"]).to be true - end - - it "serializes `user_flag_status` for user who has a pending flag" do - chat_message = chat_channel.chat_messages.last - reviewable = flag_message(chat_message, user) - score = reviewable.reviewable_scores.last - - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - - expect(response.parsed_body["chat_messages"].last["user_flag_status"]).to eq( - score.status_for_database, - ) - end - - it "doesn't serialize `reviewable_ids` for non-staff" do - reviewable = flag_message(chat_channel.chat_messages.last, admin) - - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - - expect(response.parsed_body["chat_messages"].last["reviewable_id"]).to be_nil - end - - it "serializes `reviewable_ids` correctly for staff" do - sign_in(admin) - reviewable = flag_message(chat_channel.chat_messages.last, admin) - - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - expect(response.parsed_body["chat_messages"].last["reviewable_id"]).to eq(reviewable.id) - end - - it "correctly marks reactions as 'reacted' for the current_user" do - heart_emoji = ":heart:" - smile_emoji = ":smile" - last_message = chat_channel.chat_messages.last - last_message.reactions.create(user: user, emoji: heart_emoji) - last_message.reactions.create(user: admin, emoji: smile_emoji) - - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - - reactions = response.parsed_body["chat_messages"].last["reactions"] - heart_reaction = reactions.find { |r| r["emoji"] == heart_emoji } - expect(heart_reaction["reacted"]).to be true - smile_reaction = reactions.find { |r| r["emoji"] == smile_emoji } - expect(smile_reaction["reacted"]).to be false - end - - it "sends the last message bus id for the channel" do - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - expect(response.parsed_body["meta"]["channel_message_bus_last_id"]).not_to eq(nil) - end - - context "with mentions" do - it "returns mentioned users" do - last_message = chat_channel.chat_messages.last - user1 = Fabricate(:user) - user2 = Fabricate(:user) - Fabricate(:chat_mention, user: user1, chat_message: last_message) - Fabricate(:chat_mention, user: user2, chat_message: last_message) - - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - - mentioned_users = response.parsed_body["chat_messages"].last["mentioned_users"] - expect(mentioned_users[0]["id"]).to eq(user1.id) - expect(mentioned_users[0]["username"]).to eq(user1.username) - expect(mentioned_users[1]["id"]).to eq(user2.id) - expect(mentioned_users[1]["username"]).to eq(user2.username) - end - - it "returns an empty list if no one was mentioned" do - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - - last_message = response.parsed_body["chat_messages"].last - expect(last_message).to have_key("mentioned_users") - expect(last_message["mentioned_users"]).to be_empty - end - - context "with user status" do - fab!(:status) { Fabricate(:user_status) } - fab!(:user1) { Fabricate(:user, user_status: status) } - fab!(:chat_mention) do - Fabricate(:chat_mention, user: user1, chat_message: chat_channel.chat_messages.last) - end - - it "returns status if enabled in settings" do - SiteSetting.enable_user_status = true - - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - - mentioned_user = response.parsed_body["chat_messages"].last["mentioned_users"][0] - expect(mentioned_user).to have_key("status") - expect(mentioned_user["status"]["emoji"]).to eq(status.emoji) - expect(mentioned_user["status"]["description"]).to eq(status.description) - end - - it "doesn't return status if disabled in settings" do - SiteSetting.enable_user_status = false - - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - - mentioned_user = response.parsed_body["chat_messages"].last["mentioned_users"][0] - expect(mentioned_user).not_to have_key("status") - end - end - end - - describe "scrolling to the past" do - it "returns the correct messages in created_at, id order" do - get "/chat/#{chat_channel.id}/messages.json", - params: { - message_id: message_40.id, - direction: described_class::PAST, - page_size: page_size, - } - messages = response.parsed_body["chat_messages"] - expect(messages.count).to eq(page_size) - expect(messages.first["created_at"].to_time).to eq_time(message_10.created_at) - expect(messages.last["created_at"].to_time).to eq_time(message_39.created_at) - end - - it "returns 'can_load...' properly when there are more past messages" do - get "/chat/#{chat_channel.id}/messages.json", - params: { - message_id: message_40.id, - direction: described_class::PAST, - page_size: page_size, - } - expect(response.parsed_body["meta"]["can_load_more_past"]).to be true - expect(response.parsed_body["meta"]["can_load_more_future"]).to be_nil - end - - it "returns 'can_load...' properly when there are no past messages" do - get "/chat/#{chat_channel.id}/messages.json", - params: { - message_id: message_3.id, - direction: described_class::PAST, - page_size: page_size, - } - expect(response.parsed_body["meta"]["can_load_more_past"]).to be false - expect(response.parsed_body["meta"]["can_load_more_future"]).to be_nil - end - end - - describe "scrolling to the future" do - it "returns the correct messages in created_at, id order when there are many after" do - get "/chat/#{chat_channel.id}/messages.json", - params: { - message_id: message_10.id, - direction: described_class::FUTURE, - page_size: page_size, - } - messages = response.parsed_body["chat_messages"] - expect(messages.count).to eq(page_size) - expect(messages.first["created_at"].to_time).to eq_time(message_11.created_at) - expect(messages.last["created_at"].to_time).to eq_time(message_40.created_at) - end - - it "return 'can_load..' properly when there are future messages" do - get "/chat/#{chat_channel.id}/messages.json", - params: { - message_id: message_10.id, - direction: described_class::FUTURE, - page_size: page_size, - } - expect(response.parsed_body["meta"]["can_load_more_past"]).to be_nil - expect(response.parsed_body["meta"]["can_load_more_future"]).to be true - end - - it "returns 'can_load..' properly when there are no future messages" do - get "/chat/#{chat_channel.id}/messages.json", - params: { - message_id: message_60.id, - direction: described_class::FUTURE, - page_size: page_size, - } - expect(response.parsed_body["meta"]["can_load_more_past"]).to be_nil - expect(response.parsed_body["meta"]["can_load_more_future"]).to be false - end - end - - describe "without direction (latest messages)" do - it "signals there are no future messages" do - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - - expect(response.parsed_body["meta"]["can_load_more_future"]).to eq(false) - end - - it "signals there are more messages in the past" do - get "/chat/#{chat_channel.id}/messages.json", params: { page_size: page_size } - - expect(response.parsed_body["meta"]["can_load_more_past"]).to eq(true) - end - - it "signals there are no more messages" do - new_channel = Fabricate(:category_channel) - Fabricate(:chat_message, chat_channel: new_channel, user: other_user, message: "message") - chat_messages_qty = 1 - - get "/chat/#{new_channel.id}/messages.json", params: { page_size: chat_messages_qty + 1 } - - expect(response.parsed_body["meta"]["can_load_more_past"]).to eq(false) - end - end - end - describe "#enable_chat" do context "with category as chatable" do let!(:category) { Fabricate(:category) } @@ -1193,63 +927,4 @@ RSpec.describe Chat::ChatController do get "/chat/message/#{message.id}.json" end end - - describe "#lookup_message" do - let!(:message) { Fabricate(:chat_message, chat_channel: channel) } - let(:channel) { Fabricate(:direct_message_channel) } - let(:chatable) { channel.chatable } - fab!(:user) { Fabricate(:user) } - - before { sign_in(user) } - - it "ensures message's channel can be seen" do - Guardian.any_instance.expects(:can_join_chat_channel?).with(channel) - get "/chat/lookup/#{message.id}.json", params: { chat_channel_id: channel.id } - end - - context "when the message doesn’t belong to the channel" do - let!(:message) { Fabricate(:chat_message) } - - it "returns a 404" do - get "/chat/lookup/#{message.id}.json", params: { chat_channel_id: channel.id } - - expect(response.status).to eq(404) - end - end - - context "when the chat channel is for a category" do - let(:channel) { Fabricate(:category_channel) } - - it "ensures the user can access that category" do - get "/chat/lookup/#{message.id}.json", params: { chat_channel_id: channel.id } - expect(response.status).to eq(200) - expect(response.parsed_body["chat_messages"][0]["id"]).to eq(message.id) - - group = Fabricate(:group) - chatable.update!(read_restricted: true) - Fabricate(:category_group, group: group, category: chatable) - get "/chat/lookup/#{message.id}.json", params: { chat_channel_id: channel.id } - expect(response.status).to eq(403) - - GroupUser.create!(user: user, group: group) - get "/chat/lookup/#{message.id}.json", params: { chat_channel_id: channel.id } - expect(response.status).to eq(200) - expect(response.parsed_body["chat_messages"][0]["id"]).to eq(message.id) - end - end - - context "when the chat channel is for a direct message channel" do - let(:channel) { Fabricate(:direct_message_channel) } - - it "ensures the user can access that direct message channel" do - get "/chat/lookup/#{message.id}.json", params: { chat_channel_id: channel.id } - expect(response.status).to eq(403) - - Chat::DirectMessageUser.create!(user: user, direct_message: chatable) - get "/chat/lookup/#{message.id}.json", params: { chat_channel_id: channel.id } - expect(response.status).to eq(200) - expect(response.parsed_body["chat_messages"][0]["id"]).to eq(message.id) - end - end - end end diff --git a/plugins/chat/spec/services/chat/channel_view_builder_spec.rb b/plugins/chat/spec/services/chat/channel_view_builder_spec.rb index 92e551b61e5..60e4efc9417 100644 --- a/plugins/chat/spec/services/chat/channel_view_builder_spec.rb +++ b/plugins/chat/spec/services/chat/channel_view_builder_spec.rb @@ -17,7 +17,7 @@ RSpec.describe Chat::ChannelViewBuilder do let(:channel_id) { channel.id } let(:guardian) { current_user.guardian } let(:target_message_id) { nil } - let(:page_size) { nil } + let(:page_size) { 10 } let(:direction) { nil } let(:thread_id) { nil } let(:fetch_from_last_read) { nil } @@ -87,6 +87,12 @@ RSpec.describe Chat::ChannelViewBuilder do expect(subject.view).to be_a(Chat::View) end + context "when page_size is null" do + let(:page_size) { nil } + + it { is_expected.to fail_a_contract } + end + context "when channel has threading_enabled and enable_experimental_chat_threaded_discussions is true" do before do channel.update!(threading_enabled: true) @@ -210,6 +216,12 @@ RSpec.describe Chat::ChannelViewBuilder do msg end + context "when page_size is null" do + let(:page_size) { nil } + + it { is_expected.not_to fail_a_contract } + end + context "if the user is not a member of the channel" do it "does not error and still returns messages" do expect(subject.view.chat_messages).to eq([past_message_2, past_message_1, message]) @@ -257,6 +269,12 @@ RSpec.describe Chat::ChannelViewBuilder do expect(subject.view.chat_messages).to eq([past_message, message, future_message]) end + context "when page_size is null" do + let(:page_size) { nil } + + it { is_expected.not_to fail_a_contract } + end + context "when the target message is a thread reply" do fab!(:thread) { Fabricate(:chat_thread, channel: channel) }