From 6ddd17a756407d14a572942894828044fb34420c Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 29 May 2023 09:37:10 +0200 Subject: [PATCH] DEV: Remove old ChatController routes for messages & lookup (#21723) Since 5cce829 and the new channel view builder, we have no need of these obsolete routes which have way too much logic in the controller, which has been superseded by the view builder anyway. Remove the routes and update the channel message loading to use it. --- .../app/controllers/chat/chat_controller.rb | 108 +---- .../chat/app/queries/chat/messages_query.rb | 18 +- .../app/serializers/chat/thread_serializer.rb | 2 +- .../app/services/chat/channel_view_builder.rb | 18 +- .../discourse/components/chat-channel.js | 31 +- .../discourse/components/chat-thread.js | 26 +- .../discourse/services/chat-api.js | 41 +- plugins/chat/config/routes.rb | 2 - .../chat/api/channels_controller_spec.rb | 393 ++++++++++++++++++ .../spec/requests/chat_controller_spec.rb | 325 --------------- .../chat/channel_view_builder_spec.rb | 20 +- 11 files changed, 486 insertions(+), 498 deletions(-) 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) }