From 90efdd7f9d74677f0f1a0eb7ccbe55f2b3cefa11 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Mon, 6 Nov 2023 15:45:30 +0100 Subject: [PATCH] PERF: cook message in background (#24227) This commit starts from a simple observation: cooking messages on the hot path can be slow. Especially with a lot of mentions. To move cooking from the hot path, this commit has made the following changes: - updating cooked, inserting mentions and notifying user of new mentions has been moved inside the `process_message` job. It happens right after the `Chat::MessageProcessor` run, which is where the cooking happens. - the similar existing code in `rebake!` has also been moved to rely on the `process_message`job only - refactored `create_mentions` and `update_mentions` into one single `upsert_mentions` which can be called invariably - allows services to decide if their job is ran inline or later. It avoids to need to know you have to use `Jobs.run_immediately!` in this case, in tests it will be inline per default - made various frontend changes to make the chat-channel component lifecycle clearer. we had to handle `did-update @channel` which was super awkward and creating bugs with listeners which the changes of the PR made clear in failing specs - adds a new `-processed` (and `-not-processed`) class on the chat message, this is made to have a good lifecyle hook in system specs --- .../chat/api/channel_messages_controller.rb | 4 +- .../chat/incoming_webhooks_controller.rb | 2 +- .../app/jobs/regular/chat/process_message.rb | 36 +++++++- plugins/chat/app/models/chat/message.rb | 26 ++---- .../chat/app/services/chat/create_message.rb | 59 ++++++++----- .../chat/app/services/chat/update_message.rb | 65 ++++++++------ .../discourse/components/chat-channel.hbs | 2 - .../discourse/components/chat-channel.js | 16 +--- .../discourse/components/chat-drawer.js | 6 +- .../components/chat-drawer/channel.hbs | 10 ++- .../discourse/components/chat-message.gjs | 1 + .../discourse/components/full-page-chat.hbs | 6 +- .../discourse/models/chat-channel.js | 1 + .../discourse/models/chat-message.js | 2 + .../discourse/models/chat-thread.js | 1 + .../chat-pane-base-subscriptions-manager.js | 4 +- plugins/chat/lib/chat/guardian_extensions.rb | 2 +- plugins/chat/lib/chat/message_processor.rb | 4 +- .../chat/spec/fabricators/chat_fabricator.rb | 35 +++++--- .../jobs/regular/chat/process_message_spec.rb | 22 +---- plugins/chat/spec/models/chat/message_spec.rb | 61 +++---------- plugins/chat/spec/plugin_helper.rb | 19 +++- .../api/channel_threads_controller_spec.rb | 8 +- .../spec/requests/chat_controller_spec.rb | 1 - .../spec/services/chat/create_message_spec.rb | 26 +++--- .../spec/services/chat/update_message_spec.rb | 87 +++++++++++-------- .../chat/spec/system/archive_channel_spec.rb | 9 +- .../spec/system/chat/composer/channel_spec.rb | 25 +++--- .../spec/system/chat/composer/thread_spec.rb | 2 +- plugins/chat/spec/system/chat_channel_spec.rb | 9 +- .../spec/system/chat_message/channel_spec.rb | 2 +- .../spec/system/chat_message/thread_spec.rb | 11 ++- .../chat/spec/system/deleted_channel_spec.rb | 2 +- .../chat/spec/system/deleted_message_spec.rb | 13 +-- plugins/chat/spec/system/drawer_spec.rb | 17 ++-- .../chat/spec/system/edited_message_spec.rb | 3 +- .../chat/spec/system/message_errors_spec.rb | 3 +- .../message_notifications_mobile_spec.rb | 82 +++++------------ ...message_notifications_with_sidebar_spec.rb | 1 - .../system/message_thread_indicator_spec.rb | 42 ++++----- .../system/page_objects/chat/chat_channel.rb | 3 +- .../system/page_objects/chat/chat_thread.rb | 3 +- .../page_objects/chat/components/message.rb | 18 ++-- .../system/reply_to_message/full_page_spec.rb | 8 +- .../spec/system/thread_list/full_page_spec.rb | 11 ++- plugins/chat/spec/system/transcript_spec.rb | 12 +-- 46 files changed, 388 insertions(+), 394 deletions(-) diff --git a/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb b/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb index 8f79131829b..80f614ca3b5 100644 --- a/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb +++ b/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb @@ -34,7 +34,7 @@ class Chat::Api::ChannelMessagesController < Chat::ApiController Chat::MessageRateLimiter.run!(current_user) with_service(Chat::CreateMessage) do - on_success { render json: success_json.merge(message_id: result[:message].id) } + on_success { render json: success_json.merge(message_id: result[:message_instance].id) } on_failed_policy(:no_silenced_user) { raise Discourse::InvalidAccess } on_model_not_found(:channel) { raise Discourse::NotFound } on_failed_policy(:allowed_to_join_channel) { raise Discourse::InvalidAccess } @@ -49,7 +49,7 @@ class Chat::Api::ChannelMessagesController < Chat::ApiController on_failed_policy(:ensure_thread_matches_parent) do render_json_error(I18n.t("chat.errors.thread_does_not_match_parent")) end - on_model_errors(:message) do |model| + on_model_errors(:message_instance) do |model| render_json_error(model.errors.map(&:full_message).join(", ")) end end diff --git a/plugins/chat/app/controllers/chat/incoming_webhooks_controller.rb b/plugins/chat/app/controllers/chat/incoming_webhooks_controller.rb index d4741400a34..52fe03e589f 100644 --- a/plugins/chat/app/controllers/chat/incoming_webhooks_controller.rb +++ b/plugins/chat/app/controllers/chat/incoming_webhooks_controller.rb @@ -80,7 +80,7 @@ module Chat on_failed_policy(:ensure_thread_matches_parent) do render_json_error(I18n.t("chat.errors.thread_does_not_match_parent")) end - on_model_errors(:message) do |model| + on_model_errors(:message_instance) do |model| render_json_error(model.errors.map(&:full_message).join(", ")) end end diff --git a/plugins/chat/app/jobs/regular/chat/process_message.rb b/plugins/chat/app/jobs/regular/chat/process_message.rb index 33fcc43b565..41810d3fb84 100644 --- a/plugins/chat/app/jobs/regular/chat/process_message.rb +++ b/plugins/chat/app/jobs/regular/chat/process_message.rb @@ -10,14 +10,44 @@ module Jobs ) do chat_message = ::Chat::Message.find_by(id: args[:chat_message_id]) return if !chat_message - processor = ::Chat::MessageProcessor.new(chat_message) + processor = + ::Chat::MessageProcessor.new( + chat_message, + { invalidate_oneboxes: args[:invalidate_oneboxes] }, + ) processor.run! - if args[:is_dirty] || processor.dirty? - chat_message.update( + if processor.dirty? + chat_message.update!( cooked: processor.html, cooked_version: ::Chat::Message::BAKED_VERSION, ) + chat_message.upsert_mentions + + if args[:edit_timestamp] + ::Chat::Publisher.publish_edit!(chat_message.chat_channel, chat_message) + ::Chat::Notifier.new(chat_message, args[:edit_timestamp]).notify_edit + DiscourseEvent.trigger( + :chat_message_edited, + chat_message, + chat_message.chat_channel, + chat_message.user, + ) + else + ::Chat::Publisher.publish_new!( + chat_message.chat_channel, + chat_message, + args[:staged_id], + ) + ::Chat::Notifier.new(chat_message, chat_message.created_at).notify_new + DiscourseEvent.trigger( + :chat_message_created, + chat_message, + chat_message.chat_channel, + chat_message.user, + ) + end + ::Chat::Publisher.publish_processed!(chat_message) end end diff --git a/plugins/chat/app/models/chat/message.rb b/plugins/chat/app/models/chat/message.rb index e81edec3fe4..8cbe3ff5d03 100644 --- a/plugins/chat/app/models/chat/message.rb +++ b/plugins/chat/app/models/chat/message.rb @@ -156,19 +156,8 @@ module Chat def rebake!(invalidate_oneboxes: false, priority: nil) ensure_last_editor_id - - previous_cooked = self.cooked - new_cooked = - self.class.cook( - message, - invalidate_oneboxes: invalidate_oneboxes, - user_id: self.last_editor_id, - ) - update_columns(cooked: new_cooked, cooked_version: BAKED_VERSION) - args = { chat_message_id: self.id } + args = { chat_message_id: self.id, invalidate_oneboxes: invalidate_oneboxes } args[:queue] = priority.to_s if priority && priority != :normal - args[:is_dirty] = true if previous_cooked != new_cooked - Jobs.enqueue(Jobs::Chat::ProcessMessage, args) end @@ -258,19 +247,14 @@ module Chat end end - def create_mentions - insert_mentions(parsed_mentions.all_mentioned_users_ids) - end - - def update_mentions + def upsert_mentions mentioned_user_ids = parsed_mentions.all_mentioned_users_ids - old_mentions = chat_mentions.pluck(:user_id) - updated_mentions = mentioned_user_ids - mentioned_user_ids_to_drop = old_mentions - updated_mentions - mentioned_user_ids_to_add = updated_mentions - old_mentions + mentioned_user_ids_to_drop = old_mentions - mentioned_user_ids delete_mentions(mentioned_user_ids_to_drop) + + mentioned_user_ids_to_add = mentioned_user_ids - old_mentions insert_mentions(mentioned_user_ids_to_add) end diff --git a/plugins/chat/app/services/chat/create_message.rb b/plugins/chat/app/services/chat/create_message.rb index c53e8fb7ea7..9f578f6e988 100644 --- a/plugins/chat/app/services/chat/create_message.rb +++ b/plugins/chat/app/services/chat/create_message.rb @@ -32,7 +32,8 @@ module Chat policy :ensure_valid_thread_for_channel policy :ensure_thread_matches_parent model :uploads, optional: true - model :message, :instantiate_message + model :message_instance, :instantiate_message + transaction do step :save_message step :delete_drafts @@ -54,6 +55,7 @@ module Chat attribute :upload_ids, :array attribute :thread_id, :string attribute :incoming_chat_webhook + attribute :process_inline, :boolean, default: Rails.env.test? validates :chat_channel_id, presence: true validates :message, presence: true, if: -> { upload_ids.blank? } @@ -127,38 +129,38 @@ module Chat ) end - def save_message(message:, **) - message.cook - message.save! - message.create_mentions + def save_message(message_instance:, **) + message_instance.save! end def delete_drafts(channel:, guardian:, **) Chat::Draft.where(user: guardian.user, chat_channel: channel).destroy_all end - def post_process_thread(thread:, message:, guardian:, **) + def post_process_thread(thread:, message_instance:, guardian:, **) return unless thread - thread.update!(last_message: message) + thread.update!(last_message: message_instance) thread.increment_replies_count_cache - thread.add(guardian.user).update!(last_read_message: message) + thread.add(guardian.user).update!(last_read_message: message_instance) thread.add(thread.original_message_user) end - def create_webhook_event(contract:, message:, **) + def create_webhook_event(contract:, message_instance:, **) return if contract.incoming_chat_webhook.blank? - message.create_chat_webhook_event(incoming_chat_webhook: contract.incoming_chat_webhook) + message_instance.create_chat_webhook_event( + incoming_chat_webhook: contract.incoming_chat_webhook, + ) end - def update_channel_last_message(channel:, message:, **) - return if message.in_thread? - channel.update!(last_message: message) + def update_channel_last_message(channel:, message_instance:, **) + return if message_instance.in_thread? + channel.update!(last_message: message_instance) end - def update_membership_last_read(channel_membership:, message:, **) - return if message.in_thread? - channel_membership.update!(last_read_message: message) + def update_membership_last_read(channel_membership:, message_instance:, **) + return if message_instance.in_thread? + channel_membership.update!(last_read_message: message_instance) end def process_direct_message_channel(channel_membership:, **) @@ -173,16 +175,25 @@ module Chat Chat::Publisher.publish_thread_created!(channel, reply, thread.id) end - def publish_new_message_events(channel:, message:, contract:, guardian:, **) - Chat::Publisher.publish_new!(channel, message, contract.staged_id) - Jobs.enqueue(Jobs::Chat::ProcessMessage, { chat_message_id: message.id }) - Chat::Notifier.notify_new(chat_message: message, timestamp: message.created_at) - DiscourseEvent.trigger(:chat_message_created, message, channel, guardian.user) + def publish_new_message_events(channel:, message_instance:, contract:, **) + staged_id = contract.staged_id + + if contract.process_inline + Jobs::Chat::ProcessMessage.new.execute( + { chat_message_id: message_instance.id, staged_id: staged_id }, + ) + else + Jobs.enqueue( + Jobs::Chat::ProcessMessage, + { chat_message_id: message_instance.id, staged_id: staged_id }, + ) + end end - def publish_user_tracking_state(message:, channel:, channel_membership:, guardian:, **) - message_to_publish = message - message_to_publish = channel_membership.last_read_message || message if message.in_thread? + def publish_user_tracking_state(message_instance:, channel:, channel_membership:, guardian:, **) + message_to_publish = message_instance + message_to_publish = + channel_membership.last_read_message || message_instance if message_instance.in_thread? Chat::Publisher.publish_user_tracking_state!(guardian.user, channel, message_to_publish) end end diff --git a/plugins/chat/app/services/chat/update_message.rb b/plugins/chat/app/services/chat/update_message.rb index 66356091a1b..545d475ee12 100644 --- a/plugins/chat/app/services/chat/update_message.rb +++ b/plugins/chat/app/services/chat/update_message.rb @@ -18,6 +18,7 @@ module Chat contract model :message model :uploads, optional: true + step :enforce_system_membership policy :can_modify_channel_message policy :can_modify_message @@ -30,34 +31,38 @@ module Chat class Contract attribute :message_id, :string + validates :message_id, presence: true + attribute :message, :string + validates :message, presence: true, if: -> { upload_ids.blank? } + attribute :upload_ids, :array - validates :message_id, presence: true - validates :message, presence: true, if: -> { upload_ids.blank? } + attribute :process_inline, :boolean, default: Rails.env.test? end private + def enforce_system_membership(guardian:, message:, **) + message.chat_channel.add(guardian.user) if guardian.user.is_system_user? + end + def fetch_message(contract:, **) - ::Chat::Message - .strict_loading - .includes( - :chat_mentions, - :bookmarks, - :chat_webhook_event, - :uploads, - :revisions, - reactions: [:user], - thread: [:channel, last_message: [:user]], - chat_channel: [ - :last_message, - :chat_channel_archive, - chatable: [:topic_only_relative_url, direct_message_users: [user: :user_option]], - ], - user: :user_status, - ) - .find_by(id: contract.message_id) + ::Chat::Message.includes( + :chat_mentions, + :bookmarks, + :chat_webhook_event, + :uploads, + :revisions, + reactions: [:user], + thread: [:channel, last_message: [:user]], + chat_channel: [ + :last_message, + :chat_channel_archive, + chatable: [:topic_only_relative_url, direct_message_users: [user: :user_option]], + ], + user: :user_status, + ).find_by(id: contract.message_id) end def fetch_uploads(contract:, guardian:, **) @@ -88,9 +93,7 @@ module Chat end def save_message(message:, **) - message.cook message.save! - message.update_mentions end def save_revision(message:, guardian:, **) @@ -127,13 +130,19 @@ module Chat chars_edited > max_edited_chars end - def publish(message:, guardian:, **) - ::Chat::Publisher.publish_edit!(message.chat_channel, message) - Jobs.enqueue(Jobs::Chat::ProcessMessage, { chat_message_id: message.id }) + def publish(message:, guardian:, contract:, **) + edit_timestamp = context.revision&.created_at&.iso8601(6) || Time.zone.now.iso8601(6) - edit_timestamp = context.revision&.created_at || Time.zone.now - ::Chat::Notifier.notify_edit(chat_message: message, timestamp: edit_timestamp) - DiscourseEvent.trigger(:chat_message_edited, message, message.chat_channel, guardian.user) + if contract.process_inline + Jobs::Chat::ProcessMessage.new.execute( + { chat_message_id: message.id, edit_timestamp: edit_timestamp }, + ) + else + Jobs.enqueue( + Jobs::Chat::ProcessMessage, + { chat_message_id: message.id, edit_timestamp: edit_timestamp }, + ) + end if message.thread.present? ::Chat::Publisher.publish_thread_original_message_metadata!(message.thread) diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-channel.hbs index 26a9dba1225..f7814be640f 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.hbs @@ -8,9 +8,7 @@ {{did-insert this.setUploadDropZone}} {{did-insert this.setupListeners}} {{will-destroy this.teardownListeners}} - {{did-update this.loadMessages @targetMessageId}} {{did-insert this.didUpdateChannel}} - {{did-update this.didUpdateChannel @channel.id}} {{did-insert this.addAutoFocusEventListener}} {{will-destroy this.removeAutoFocusEventListener}} data-id={{@channel.id}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js index c20b8e9cea9..ce8db2befc0 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js @@ -61,7 +61,6 @@ export default class ChatChannel extends Component { @tracked isScrolling = false; scrollable = null; - _loadedChannelId = null; _mentionWarningsSeen = {}; _unreachableGroupMentions = []; _overMembersLimitGroupMentions = []; @@ -98,7 +97,7 @@ export default class ChatChannel extends Component { teardownListeners() { this.#cancelHandlers(); removeOnPresenceChange(this.onPresenceChangeCallback); - this.unsubscribeToUpdates(this._loadedChannelId); + this.unsubscribeToUpdates(this.args.channel.id); } @action @@ -109,12 +108,6 @@ export default class ChatChannel extends Component { @action didUpdateChannel() { - this.#cancelHandlers(); - - if (!this.args.channel) { - return; - } - this.messagesManager.clear(); if ( @@ -124,12 +117,6 @@ export default class ChatChannel extends Component { this.chatChannelsManager.follow(this.args.channel); } - if (this._loadedChannelId !== this.args.channel.id) { - this.unsubscribeToUpdates(this._loadedChannelId); - this.pane.selectingMessages = false; - this._loadedChannelId = this.args.channel.id; - } - const existingDraft = this.chatDraftsManager.get({ channelId: this.args.channel.id, }); @@ -647,7 +634,6 @@ export default class ChatChannel extends Component { return; } - this.unsubscribeToUpdates(channel.id); this.messageBus.subscribe( `/chat/${channel.id}`, this.onMessage, diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-drawer.js b/plugins/chat/assets/javascripts/discourse/components/chat-drawer.js index 6cffa7c7449..6533b8fc137 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-drawer.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-drawer.js @@ -1,6 +1,6 @@ import Component from "@ember/component"; import { action } from "@ember/object"; -import { cancel, throttle } from "@ember/runloop"; +import { cancel, next, throttle } from "@ember/runloop"; import { inject as service } from "@ember/service"; import { htmlSafe } from "@ember/template"; import DiscourseURL from "discourse/lib/url"; @@ -188,11 +188,13 @@ export default Component.extend({ }, @action - openInFullPage() { + async openInFullPage() { this.chatStateManager.storeAppURL(); this.chatStateManager.prefersFullPage(); this.chat.activeChannel = null; + await new Promise((resolve) => next(resolve)); + return DiscourseURL.routeTo(this.chatStateManager.lastKnownChatURL); }, diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-drawer/channel.hbs b/plugins/chat/assets/javascripts/discourse/components/chat-drawer/channel.hbs index f78588d7632..7d80c277f61 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-drawer/channel.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-drawer/channel.hbs @@ -16,10 +16,12 @@ {{did-update this.fetchChannel @params.channelId}} > {{#if this.chat.activeChannel}} - + {{#each (array this.chat.activeChannel) as |channel|}} + + {{/each}} {{/if}} {{/if}} \ No newline at end of file diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs index 3a445d3d26a..8404d275af9 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message.gjs @@ -509,6 +509,7 @@ export default class ChatMessage extends Component { (if @message.highlighted "-highlighted") (if (eq @message.user.id this.currentUser.id) "is-by-current-user") (if @message.staged "-staged" "-persisted") + (if @message.processed "-processed" "-not-processed") (if this.hasActiveState "-active") (if @message.bookmark "-bookmarked") (if @message.deletedAt "-deleted") diff --git a/plugins/chat/assets/javascripts/discourse/components/full-page-chat.hbs b/plugins/chat/assets/javascripts/discourse/components/full-page-chat.hbs index 56c36f8f63d..74acc331eb5 100644 --- a/plugins/chat/assets/javascripts/discourse/components/full-page-chat.hbs +++ b/plugins/chat/assets/javascripts/discourse/components/full-page-chat.hbs @@ -1,3 +1,3 @@ -{{#if @channel.id}} - -{{/if}} \ No newline at end of file +{{#each (array @channel) as |channel|}} + +{{/each}} \ No newline at end of file diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index 731dbeb28fe..ccc6415b9a6 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js @@ -192,6 +192,7 @@ export default class ChatChannel { async stageMessage(message) { message.id = guid(); message.staged = true; + message.processed = false; message.draft = false; message.createdAt = new Date(); message.channel = this; diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-message.js b/plugins/chat/assets/javascripts/discourse/models/chat-message.js index 630889e7a27..dfd23414582 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-message.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-message.js @@ -26,6 +26,7 @@ export default class ChatMessage { @tracked selected; @tracked channel; @tracked staged; + @tracked processed; @tracked draftSaved; @tracked draft; @tracked createdAt; @@ -64,6 +65,7 @@ export default class ChatMessage { this.draftSaved = args.draftSaved || args.draft_saved || false; this.firstOfResults = args.firstOfResults || args.first_of_results || false; this.staged = args.staged || false; + this.processed = args.processed || true; this.edited = args.edited || false; this.editing = args.editing || false; this.availableFlags = args.availableFlags || args.available_flags; diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-thread.js b/plugins/chat/assets/javascripts/discourse/models/chat-thread.js index 095b82ebd1d..dcd059b61e2 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-thread.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-thread.js @@ -61,6 +61,7 @@ export default class ChatThread { async stageMessage(message) { message.id = guid(); message.staged = true; + message.processed = false; message.draft = false; message.createdAt ??= moment.utc().format(); message.thread = this; diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js index d0119e1e143..43692b0c8d0 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js @@ -16,7 +16,6 @@ export function handleStagedMessage(channel, messagesManager, data) { stagedMessage.excerpt = data.chat_message.excerpt; stagedMessage.channel = channel; stagedMessage.createdAt = new Date(data.chat_message.created_at); - stagedMessage.cooked = data.chat_message.cooked; return stagedMessage; } @@ -131,6 +130,7 @@ export default class ChatPaneBaseSubscriptionsManager extends Service { const message = this.messagesManager.findMessage(data.chat_message.id); if (message) { message.cooked = data.chat_message.cooked; + message.processed = true; } } @@ -144,8 +144,6 @@ export default class ChatPaneBaseSubscriptionsManager extends Service { handleEditMessage(data) { const message = this.messagesManager.findMessage(data.chat_message.id); if (message) { - message.message = data.chat_message.message; - message.cooked = data.chat_message.cooked; message.excerpt = data.chat_message.excerpt; message.uploads = cloneJSON(data.chat_message.uploads || []); message.edited = data.chat_message.edited; diff --git a/plugins/chat/lib/chat/guardian_extensions.rb b/plugins/chat/lib/chat/guardian_extensions.rb index c26506ebf1e..14116678535 100644 --- a/plugins/chat/lib/chat/guardian_extensions.rb +++ b/plugins/chat/lib/chat/guardian_extensions.rb @@ -209,7 +209,7 @@ module Chat end def can_edit_chat?(message) - message.user_id == @user.id && !@user.silenced? + (message.user_id == @user.id && !@user.silenced?) end def can_react? diff --git a/plugins/chat/lib/chat/message_processor.rb b/plugins/chat/lib/chat/message_processor.rb index ae663ecb624..9317dec377a 100644 --- a/plugins/chat/lib/chat/message_processor.rb +++ b/plugins/chat/lib/chat/message_processor.rb @@ -4,12 +4,12 @@ module Chat class MessageProcessor include ::CookedProcessorMixin - def initialize(chat_message) + def initialize(chat_message, opts = {}) @model = chat_message @previous_cooked = (chat_message.cooked || "").dup @with_secure_uploads = false @size_cache = {} - @opts = {} + @opts = opts cooked = Chat::Message.cook(chat_message.message, user_id: chat_message.last_editor_id) @doc = Loofah.html5_fragment(cooked) diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index 14b1d9ea0ae..b3c73a7f91c 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -63,10 +63,10 @@ end Fabricator(:chat_message_without_service, class_name: "Chat::Message") do user chat_channel - message { Faker::Lorem.paragraph_by_chars(number: 500).gsub("...", "…").gsub("..", "…") } + message { Faker::Lorem.words(number: 5).join(" ") } after_build { |message, attrs| message.cook } - after_create { |message, attrs| message.create_mentions } + after_create { |message, attrs| message.upsert_mentions } end Fabricator(:chat_message_with_service, class_name: "Chat::CreateMessage") do @@ -86,17 +86,26 @@ Fabricator(:chat_message_with_service, class_name: "Chat::CreateMessage") do Group.refresh_automatic_groups! channel.add(user) - resolved_class.call( - chat_channel_id: channel.id, - guardian: user.guardian, - message: - transients[:message] || - Faker::Lorem.paragraph_by_chars(number: 500).gsub("...", "…").gsub("..", "…"), - thread_id: transients[:thread]&.id, - in_reply_to_id: transients[:in_reply_to]&.id, - upload_ids: transients[:upload_ids], - incoming_chat_webhook: transients[:incoming_chat_webhook], - ).message + result = + resolved_class.call( + chat_channel_id: channel.id, + guardian: user.guardian, + message: transients[:message] || Faker::Lorem.words(number: 5).join(" "), + thread_id: transients[:thread]&.id, + in_reply_to_id: transients[:in_reply_to]&.id, + upload_ids: transients[:upload_ids], + incoming_chat_webhook: transients[:incoming_chat_webhook], + process_inline: true, + ) + + if result.failure? + raise RSpec::Expectations::ExpectationNotMetError.new( + "Service `#{resolved_class}` failed, see below for step details:\n\n" + + result.inspect_steps.inspect, + ) + end + + result.message_instance end end diff --git a/plugins/chat/spec/jobs/regular/chat/process_message_spec.rb b/plugins/chat/spec/jobs/regular/chat/process_message_spec.rb index 658bb2e3d1c..1e59252d3c6 100644 --- a/plugins/chat/spec/jobs/regular/chat/process_message_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat/process_message_spec.rb @@ -19,30 +19,12 @@ describe Jobs::Chat::ProcessMessage do ) end - context "when is_dirty args is true" do - fab!(:chat_message) { Fabricate(:chat_message, message: "a very lovely cat") } - + context "when the cooked message changed" do it "publishes the update" do + chat_message.update!(cooked: "another lovely cat") Chat::Publisher.expects(:publish_processed!).once - described_class.new.execute(chat_message_id: chat_message.id, is_dirty: true) - end - end - - context "when is_dirty args is not true" do - fab!(:chat_message) { Fabricate(:chat_message, message: "a very lovely cat") } - - it "doesn’t publish the update" do - Chat::Publisher.expects(:publish_processed!).never described_class.new.execute(chat_message_id: chat_message.id) end - - context "when the cooked message changed" do - it "publishes the update" do - chat_message.update!(cooked: "another lovely cat") - Chat::Publisher.expects(:publish_processed!).once - described_class.new.execute(chat_message_id: chat_message.id) - end - end end it "does not error when message is deleted" do diff --git a/plugins/chat/spec/models/chat/message_spec.rb b/plugins/chat/spec/models/chat/message_spec.rb index 9ad53bcf44e..8a268fa9a24 100644 --- a/plugins/chat/spec/models/chat/message_spec.rb +++ b/plugins/chat/spec/models/chat/message_spec.rb @@ -510,49 +510,23 @@ describe Chat::Message do it "keeps the same hashtags the user has permission to after rebake" do group.add(chat_message.user) - chat_message.update!( - message: + update_message( + chat_message, + user: chat_message.user, + text: "this is the message ##{category.slug} ##{secure_category.slug} ##{chat_message.chat_channel.slug}", ) - chat_message.cook - chat_message.save! expect(chat_message.reload.cooked).to include(secure_category.name) chat_message.rebake! + expect(chat_message.reload.cooked).to include(secure_category.name) end end end - describe "#create_mentions" do - fab!(:message) { Fabricate(:chat_message) } - fab!(:user1) { Fabricate(:user) } - fab!(:user2) { Fabricate(:user) } - - it "creates mentions for mentioned usernames" do - message.message = "Mentioning @#{user1.username} and @#{user2.username}" - message.cook - - message.create_mentions - message.reload - - expect(message.chat_mentions.pluck(:user_id)).to match_array([user1.id, user2.id]) - end - - it "ignores duplicated mentions" do - message.message = - "Mentioning @#{user1.username} @#{user1.username} @#{user1.username} @#{user1.username}" - message.cook - - message.create_mentions - message.reload - - expect(message.chat_mentions.pluck(:user_id)).to contain_exactly(user1.id) - end - end - - describe "#update_mentions" do + describe "#upsert_mentions" do fab!(:user1) { Fabricate(:user) } fab!(:user2) { Fabricate(:user) } fab!(:user3) { Fabricate(:user) } @@ -564,11 +538,11 @@ describe Chat::Message do it "creates newly added mentions" do existing_mention_ids = message.chat_mentions.pluck(:id) - message.message = message.message + " @#{user3.username} @#{user4.username} " - message.cook - - message.update_mentions - message.reload + update_message( + message, + user: message.user, + text: message.message + " @#{user3.username} @#{user4.username} ", + ) expect(message.chat_mentions.pluck(:user_id)).to match_array( [user1.id, user2.id, user3.id, user4.id], @@ -577,22 +551,15 @@ describe Chat::Message do end it "drops removed mentions" do - message.message = "Hey @#{user1.username}" # user 2 is not mentioned anymore - message.cook - - message.update_mentions - message.reload + # user 2 is not mentioned anymore + update_message(message, user: message.user, text: "Hey @#{user1.username}") expect(message.chat_mentions.pluck(:user_id)).to contain_exactly(user1.id) end it "changes nothing if passed mentions are identical to existing mentions" do existing_mention_ids = message.chat_mentions.pluck(:id) - message.message = message.message - message.cook - - message.update_mentions - message.reload + update_message(message, user: message.user, text: message.message) expect(message.chat_mentions.pluck(:user_id)).to match_array(already_mentioned) expect(message.chat_mentions.pluck(:id)).to include(*existing_mention_ids) # the mentions weren't recreated diff --git a/plugins/chat/spec/plugin_helper.rb b/plugins/chat/spec/plugin_helper.rb index 7ae9a782c51..8824f36bc70 100644 --- a/plugins/chat/spec/plugin_helper.rb +++ b/plugins/chat/spec/plugin_helper.rb @@ -8,7 +8,7 @@ module ChatSystemHelpers user.activate SiteSetting.chat_enabled = true - SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:trust_level_1] + SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone] channels_for_membership.each do |channel| membership = channel.add(user) @@ -42,11 +42,11 @@ module ChatSystemHelpers in_reply_to_id: in_reply_to, thread_id: thread_id, guardian: last_user.guardian, - message: Faker::Lorem.paragraph, + message: Faker::Lorem.words(number: 5).join(" "), ) raise "#{creator.inspect_steps.inspect}\n\n#{creator.inspect_steps.error}" if creator.failure? - last_message = creator.message + last_message = creator.message_instance end last_message.thread.set_replies_count_cache(messages_count - 1, update_db: true) @@ -67,6 +67,19 @@ module ChatSpecHelpers "Service failed, see below for step details:\n\n" + result.inspect_steps.inspect, ) end + + def update_message(message, text: nil, user: Discourse.system_user, upload_ids: nil) + result = + Chat::UpdateMessage.call( + guardian: user.guardian, + message_id: message.id, + upload_ids: upload_ids, + message: text, + process_inline: true, + ) + service_failed!(result) if result.failure? + result.message_instance + end end RSpec.configure do |config| diff --git a/plugins/chat/spec/requests/chat/api/channel_threads_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channel_threads_controller_spec.rb index cad5329c68a..9f3f3da1cc7 100644 --- a/plugins/chat/spec/requests/chat/api/channel_threads_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/channel_threads_controller_spec.rb @@ -120,11 +120,11 @@ RSpec.describe Chat::Api::ChannelThreadsController do end it "has preloaded chat mentions and users for the thread original message" do - thread_1.original_message.update!( - message: "@#{current_user.username} hello and @#{thread_2.original_message_user.username}!", + update_message( + thread_1.original_message, + user: thread_1.original_message.user, + text: "@#{current_user.username} hello and @#{thread_2.original_message_user.username}!", ) - thread_1.original_message.rebake! - thread_1.original_message.create_mentions get "/chat/api/channels/#{public_channel.id}/threads" expect(response.status).to eq(200) diff --git a/plugins/chat/spec/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb index e3d282c07dc..444c7901830 100644 --- a/plugins/chat/spec/requests/chat_controller_spec.rb +++ b/plugins/chat/spec/requests/chat_controller_spec.rb @@ -114,7 +114,6 @@ RSpec.describe Chat::ChatController do job: Jobs::Chat::ProcessMessage, args: { chat_message_id: chat_message.id, - is_dirty: true, }, ) do put "/chat/#{chat_channel.id}/#{chat_message.id}/rebake.json" diff --git a/plugins/chat/spec/services/chat/create_message_spec.rb b/plugins/chat/spec/services/chat/create_message_spec.rb index e6401b3fdca..c9a213aa18c 100644 --- a/plugins/chat/spec/services/chat/create_message_spec.rb +++ b/plugins/chat/spec/services/chat/create_message_spec.rb @@ -34,7 +34,7 @@ RSpec.describe Chat::CreateMessage do let(:params) do { guardian: guardian, chat_channel_id: channel.id, message: content, upload_ids: [upload.id] } end - let(:message) { result[:message].reload } + let(:message) { result[:message_instance].reload } shared_examples "creating a new message" do it "saves the message" do @@ -43,10 +43,12 @@ RSpec.describe Chat::CreateMessage do end it "cooks the message" do + Jobs.run_immediately! expect(message).to be_cooked end it "creates mentions" do + Jobs.run_immediately! expect { result }.to change { Chat::Mention.count }.by(1) end @@ -73,21 +75,15 @@ RSpec.describe Chat::CreateMessage do result end - it "enqueues a job to process message" do - result - expect_job_enqueued(job: Jobs::Chat::ProcessMessage, args: { chat_message_id: message.id }) + it "can enqueue a job to process message" do + params[:process_inline] = false + expect_enqueued_with(job: Jobs::Chat::ProcessMessage) { result } end - it "notifies the new message" do - result - expect_job_enqueued( - job: Jobs::Chat::SendMessageNotifications, - args: { - chat_message_id: message.id, - timestamp: message.created_at.iso8601(6), - reason: "new", - }, - ) + it "can process a message inline" do + params[:process_inline] = true + Jobs::Chat::ProcessMessage.any_instance.expects(:execute).once + expect_not_enqueued_with(job: Jobs::Chat::ProcessMessage) { result } end it "triggers a Discourse event" do @@ -340,7 +336,7 @@ RSpec.describe Chat::CreateMessage do context "when message is not valid" do let(:content) { "a" * (SiteSetting.chat_maximum_message_length + 1) } - it { is_expected.to fail_with_an_invalid_model(:message) } + it { is_expected.to fail_with_an_invalid_model(:message_instance) } end context "when message is valid" do diff --git a/plugins/chat/spec/services/chat/update_message_spec.rb b/plugins/chat/spec/services/chat/update_message_spec.rb index 6548a0b95ce..2cc566e1994 100644 --- a/plugins/chat/spec/services/chat/update_message_spec.rb +++ b/plugins/chat/spec/services/chat/update_message_spec.rb @@ -56,6 +56,7 @@ RSpec.describe Chat::UpdateMessage do user: user, message: message, upload_ids: upload_ids, + use_service: true, ) end @@ -153,18 +154,20 @@ RSpec.describe Chat::UpdateMessage do it "publishes updated message to message bus" do chat_message = create_chat_message(user1, "This will be changed", public_chat_channel) new_content = "New content" - messages = - MessageBus.track_publish("/chat/#{public_chat_channel.id}") do - described_class.call( - guardian: guardian, - message_id: chat_message.id, - message: new_content, - ) - end - expect(messages.count).to be(1) - message = messages[0].data - expect(message["chat_message"]["message"]).to eq(new_content) + processed_message = + MessageBus + .track_publish("/chat/#{public_chat_channel.id}") do + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: new_content, + ) + end + .detect { |m| m.data["type"] == "edit" } + .data + + expect(processed_message["chat_message"]["message"]).to eq(new_content) end context "with mentions" do @@ -281,20 +284,21 @@ RSpec.describe Chat::UpdateMessage do chat_message = create_chat_message(user1, "This will be updated", public_chat_channel) new_content = "Hey @#{user2.username}" - messages = - MessageBus.track_publish("/chat/#{public_chat_channel.id}") do - described_class.call( - guardian: guardian, - message_id: chat_message.id, - message: new_content, - ) - end + processed_message = + MessageBus + .track_publish("/chat/#{public_chat_channel.id}") do + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: new_content, + ) + end + .detect { |m| m.data["type"] == "edit" } + .data - expect(messages.count).to be(1) - message = messages[0].data - expect(message["chat_message"]["mentioned_users"].count).to eq(1) - mentioned_user = message["chat_message"]["mentioned_users"][0] + expect(processed_message["chat_message"]["mentioned_users"].count).to eq(1) + mentioned_user = processed_message["chat_message"]["mentioned_users"][0] expect(mentioned_user["id"]).to eq(user2.id) expect(mentioned_user["username"]).to eq(user2.username) expect(mentioned_user["status"]).to be_present @@ -305,21 +309,21 @@ RSpec.describe Chat::UpdateMessage do SiteSetting.enable_user_status = false user2.set_status!("dentist", "tooth") chat_message = create_chat_message(user1, "This will be updated", public_chat_channel) - new_content = "Hey @#{user2.username}" - messages = - MessageBus.track_publish("/chat/#{public_chat_channel.id}") do - described_class.call( - guardian: guardian, - message_id: chat_message.id, - message: new_content, - ) - end + processed_message = + MessageBus + .track_publish("/chat/#{public_chat_channel.id}") do + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "Hey @#{user2.username}", + ) + end + .detect { |m| m.data["type"] == "edit" } + .data - expect(messages.count).to be(1) - message = messages[0].data - expect(message["chat_message"]["mentioned_users"].count).to be(1) - mentioned_user = message["chat_message"]["mentioned_users"][0] + expect(processed_message["chat_message"]["mentioned_users"].count).to be(1) + mentioned_user = processed_message["chat_message"]["mentioned_users"][0] expect(mentioned_user["status"]).to be_blank end @@ -775,6 +779,17 @@ RSpec.describe Chat::UpdateMessage do expect { result }.to not_change { result.message.last_editor_id } end + + it "can enqueue a job to process message" do + params[:process_inline] = false + expect_enqueued_with(job: Jobs::Chat::ProcessMessage) { result } + end + + it "can process a message inline" do + params[:process_inline] = true + Jobs::Chat::ProcessMessage.any_instance.expects(:execute).once + expect_not_enqueued_with(job: Jobs::Chat::ProcessMessage) { result } + end end context "when params are not valid" do diff --git a/plugins/chat/spec/system/archive_channel_spec.rb b/plugins/chat/spec/system/archive_channel_spec.rb index 784a7f87cc7..b755295344b 100644 --- a/plugins/chat/spec/system/archive_channel_spec.rb +++ b/plugins/chat/spec/system/archive_channel_spec.rb @@ -68,8 +68,13 @@ RSpec.describe "Archive channel", type: :system do let(:other_user) { Fabricate(:user) } before do - Jobs.run_immediately! channel_1.add(current_user) + channel_1.add(other_user) + end + + it "clears unread indicators" do + Jobs.run_immediately! + Fabricate( :chat_message, chat_channel: channel_1, @@ -77,9 +82,7 @@ RSpec.describe "Archive channel", type: :system do message: "this is fine @#{current_user.username}", use_service: true, ) - end - it "clears unread indicators" do visit("/") expect(page.find(".chat-channel-unread-indicator")).to have_content(1) diff --git a/plugins/chat/spec/system/chat/composer/channel_spec.rb b/plugins/chat/spec/system/chat/composer/channel_spec.rb index 0143aae7107..f9b0b20e0b4 100644 --- a/plugins/chat/spec/system/chat/composer/channel_spec.rb +++ b/plugins/chat/spec/system/chat/composer/channel_spec.rb @@ -16,16 +16,21 @@ RSpec.describe "Chat | composer | channel", type: :system, js: true do end describe "reply to message" do - it "renders text in the details" do - message_1.update!(message: "not marked") - message_1.rebake! - chat_page.visit_channel(channel_1) - channel_page.reply_to(message_1) + context "when raw contains html" do + fab!(:message_1) do + Fabricate(:chat_message, chat_channel: channel_1, message: "not marked") + end - expect(channel_page.composer.message_details).to have_message( - id: message_1.id, - exact_text: "not marked", - ) + it "renders text in the details" do + chat_page.visit_channel(channel_1) + + channel_page.reply_to(message_1) + + expect(channel_page.composer.message_details).to have_message( + id: message_1.id, + exact_text: "not marked", + ) + end end context "when threading is disabled" do @@ -65,7 +70,7 @@ RSpec.describe "Chat | composer | channel", type: :system, js: true do channel_page.edit_message(message_1, "instant") expect(channel_page.messages).to have_message( - text: message_1.message + "instant", + text: message_1.message + " instant", persisted: false, ) ensure diff --git a/plugins/chat/spec/system/chat/composer/thread_spec.rb b/plugins/chat/spec/system/chat/composer/thread_spec.rb index c1c396efc36..591300be10f 100644 --- a/plugins/chat/spec/system/chat/composer/thread_spec.rb +++ b/plugins/chat/spec/system/chat/composer/thread_spec.rb @@ -34,7 +34,7 @@ RSpec.describe "Chat | composer | thread", type: :system, js: true do thread_page.edit_message(message_2, "instant") expect(thread_page.messages).to have_message( - text: message_2.message + "instant", + text: message_2.message + " instant", persisted: false, ) ensure diff --git a/plugins/chat/spec/system/chat_channel_spec.rb b/plugins/chat/spec/system/chat_channel_spec.rb index 2f4bb5484a0..cc664a8db69 100644 --- a/plugins/chat/spec/system/chat_channel_spec.rb +++ b/plugins/chat/spec/system/chat_channel_spec.rb @@ -92,6 +92,8 @@ RSpec.describe "Chat channel", type: :system do context "with two sessions opened on same channel" do it "syncs the messages" do + Jobs.run_immediately! + using_session(:tab_1) do sign_in(current_user) chat_page.visit_channel(channel_1) @@ -275,8 +277,11 @@ RSpec.describe "Chat channel", type: :system do end it "renders safe HTML like mentions (which are just links) in the reply-to" do - message_2.update!(message: "@#{other_user.username} not marked") - message_2.rebake! + update_message( + message_2, + user: other_user, + text: "@#{other_user.username} not marked", + ) chat_page.visit_channel(channel_1) expect(find(".chat-reply .chat-reply__excerpt")["innerHTML"].strip).to eq( diff --git a/plugins/chat/spec/system/chat_message/channel_spec.rb b/plugins/chat/spec/system/chat_message/channel_spec.rb index 396764a5905..eb9f21acf27 100644 --- a/plugins/chat/spec/system/chat_message/channel_spec.rb +++ b/plugins/chat/spec/system/chat_message/channel_spec.rb @@ -3,7 +3,7 @@ RSpec.describe "Chat message - channel", type: :system do fab!(:current_user) { Fabricate(:user) } fab!(:channel_1) { Fabricate(:chat_channel) } - fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, use_service: true) } let(:cdp) { PageObjects::CDP.new } let(:chat_page) { PageObjects::Pages::Chat.new } diff --git a/plugins/chat/spec/system/chat_message/thread_spec.rb b/plugins/chat/spec/system/chat_message/thread_spec.rb index 628cbe8d1ea..cef14247020 100644 --- a/plugins/chat/spec/system/chat_message/thread_spec.rb +++ b/plugins/chat/spec/system/chat_message/thread_spec.rb @@ -3,12 +3,15 @@ RSpec.describe "Chat message - thread", type: :system do fab!(:current_user) { Fabricate(:user) } fab!(:channel_1) { Fabricate(:chat_channel, threading_enabled: true) } + fab!(:thread_original_message) { Fabricate(:chat_message_with_service, chat_channel: channel_1) } fab!(:thread_message_1) do - message_1 = Fabricate(:chat_message, chat_channel: channel_1, use_service: true) - Fabricate(:chat_message, in_reply_to: message_1, use_service: true) + Fabricate( + :chat_message_with_service, + chat_channel: channel_1, + in_reply_to: thread_original_message, + ) end - let(:cdp) { PageObjects::CDP.new } let(:chat_page) { PageObjects::Pages::Chat.new } let(:thread_page) { PageObjects::Pages::ChatThread.new } @@ -31,6 +34,8 @@ RSpec.describe "Chat message - thread", type: :system do end context "when copying text of a message" do + let(:cdp) { PageObjects::CDP.new } + before { cdp.allow_clipboard } it "[mobile] copies the text of a single message", mobile: true do diff --git a/plugins/chat/spec/system/deleted_channel_spec.rb b/plugins/chat/spec/system/deleted_channel_spec.rb index 9534464b219..392203ac52b 100644 --- a/plugins/chat/spec/system/deleted_channel_spec.rb +++ b/plugins/chat/spec/system/deleted_channel_spec.rb @@ -15,7 +15,7 @@ RSpec.describe "Deleted channel", type: :system do it "redirects to homepage" do chat_page.visit_channel(channel_1) - expect(page).to have_current_path("/latest") + expect(page).to have_content("Not Found") # this is not a translated key end end end diff --git a/plugins/chat/spec/system/deleted_message_spec.rb b/plugins/chat/spec/system/deleted_message_spec.rb index 80375aab7c0..c4a0e12b08e 100644 --- a/plugins/chat/spec/system/deleted_message_spec.rb +++ b/plugins/chat/spec/system/deleted_message_spec.rb @@ -15,19 +15,14 @@ RSpec.describe "Deleted message", type: :system do end context "when deleting a message" do + fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) } + it "shows as deleted" do chat_page.visit_channel(channel_1) - channel_page.send_message - expect(page).to have_css(".-persisted") + channel_page.messages.delete(message_1) - last_message = find(".chat-message-container:last-child") - channel_page.messages.delete(OpenStruct.new(id: last_message["data-id"])) - - expect(channel_page.messages).to have_deleted_message( - OpenStruct.new(id: last_message["data-id"]), - count: 1, - ) + expect(channel_page.messages).to have_deleted_message(message_1, count: 1) end it "does not error when coming back to the channel from another channel" do diff --git a/plugins/chat/spec/system/drawer_spec.rb b/plugins/chat/spec/system/drawer_spec.rb index 6e6c072094a..3f7b68a8250 100644 --- a/plugins/chat/spec/system/drawer_spec.rb +++ b/plugins/chat/spec/system/drawer_spec.rb @@ -120,18 +120,19 @@ RSpec.describe "Drawer", type: :system do chat_page.minimize_full_page drawer_page.maximize - using_session("user_1") do |session| - sign_in(user_1) - chat_page.visit_channel(channel_1) - channel_page.send_message("onlyonce") - session.quit - end + Fabricate( + :chat_message, + chat_channel: channel_1, + user: user_1, + use_service: true, + message: "onlyonce", + ) - expect(page).to have_content("onlyonce", count: 1, wait: 20) + expect(page).to have_content("onlyonce", count: 1) chat_page.visit_channel(channel_2) - expect(page).to have_content("onlyonce", count: 0, wait: 20) + expect(page).to have_content("onlyonce", count: 0) end end diff --git a/plugins/chat/spec/system/edited_message_spec.rb b/plugins/chat/spec/system/edited_message_spec.rb index 5a4302f6584..7d3264de2e0 100644 --- a/plugins/chat/spec/system/edited_message_spec.rb +++ b/plugins/chat/spec/system/edited_message_spec.rb @@ -36,7 +36,8 @@ RSpec.describe "Edited message", type: :system do end it "runs decorators on the edited message" do - message_1 = Fabricate(:chat_message, chat_channel: channel_1, user: current_user) + message_1 = + Fabricate(:chat_message, chat_channel: channel_1, user: current_user, use_service: true) chat_page.visit_channel(channel_1) channel_page.edit_message(message_1, '[date=2025-03-10 timezone="Europe/Paris"]') diff --git a/plugins/chat/spec/system/message_errors_spec.rb b/plugins/chat/spec/system/message_errors_spec.rb index 98b15865011..d503bbeae29 100644 --- a/plugins/chat/spec/system/message_errors_spec.rb +++ b/plugins/chat/spec/system/message_errors_spec.rb @@ -11,8 +11,9 @@ RSpec.describe "Message errors", type: :system do context "when message is too long" do fab!(:channel) { Fabricate(:chat_channel) } + before { channel.add(current_user) } + it "only shows the error, not the message" do - channel.add(current_user) sign_in(current_user) chat_page.visit_channel(channel) diff --git a/plugins/chat/spec/system/message_notifications_mobile_spec.rb b/plugins/chat/spec/system/message_notifications_mobile_spec.rb index 49e8e8825e5..c975f928ef7 100644 --- a/plugins/chat/spec/system/message_notifications_mobile_spec.rb +++ b/plugins/chat/spec/system/message_notifications_mobile_spec.rb @@ -12,8 +12,8 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do chat_system_bootstrap end - def create_message(text: "this is fine", channel:, creator: Fabricate(:user)) - Fabricate(:chat_message_with_service, chat_channel: channel, message: text, user: creator) + def create_message(channel, text: "this is fine", user: Fabricate(:user)) + Fabricate(:chat_message_with_service, chat_channel: channel, message: text, user: user) end context "as a user" do @@ -30,13 +30,9 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do context "when not member of the channel" do context "when a message is created" do it "doesn't show anything" do - Jobs.run_immediately! - visit("/chat") - using_session(:user_1) do |session| - create_message(channel: channel_1, creator: user_1) - session.quit - end + + create_message(channel_1, user: user_1) expect(page).to have_no_css(".chat-header-icon .chat-channel-unread-indicator") expect(page).to have_no_css(channel_index_page.channel_row_selector(channel_1)) @@ -58,13 +54,9 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do end it "doesn’t show indicator in header" do - Jobs.run_immediately! - visit("/chat") - using_session(:user_1) do |session| - create_message(channel: channel_1, creator: user_1) - session.quit - end + + create_message(channel_1, user: user_1) expect(page).to have_css(".do-not-disturb-background") expect(page).to have_no_css(".chat-header-icon .chat-channel-unread-indicator") @@ -76,13 +68,9 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do context "when a message is created" do it "doesn't show anything" do - Jobs.run_immediately! - visit("/chat") - using_session(:user_1) do |session| - create_message(channel: channel_1, creator: user_1) - session.quit - end + + create_message(channel_1, user: user_1) expect(page).to have_no_css(".chat-header-icon .chat-channel-unread-indicator") expect(channel_index_page).to have_no_unread_channel(channel_1) @@ -92,13 +80,9 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do context "when a message is created" do it "correctly renders notifications" do - Jobs.run_immediately! - visit("/chat") - using_session(:user_1) do |session| - create_message(channel: channel_1, creator: user_1) - session.quit - end + + create_message(channel_1, user: user_1) expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator", text: "") expect(channel_index_page).to have_unread_channel(channel_1) @@ -110,13 +94,12 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do Jobs.run_immediately! visit("/chat") - using_session(:user_1) do - create_message( - channel: channel_1, - creator: user_1, - text: "hello @#{current_user.username} what's up?", - ) - end + + create_message( + channel_1, + user: user_1, + text: "hello @#{current_user.username} what's up?", + ) expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator") expect(channel_index_page).to have_unread_channel(channel_1, count: 1) @@ -135,13 +118,9 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do context "when a message is created" do it "correctly renders notifications" do - Jobs.run_immediately! - visit("/chat") - using_session(:user_1) do |session| - create_message(channel: dm_channel_1, creator: user_1) - session.quit - end + + create_message(dm_channel_1, user: user_1) expect(page).to have_css( ".chat-header-icon .chat-channel-unread-indicator", @@ -150,10 +129,7 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do ) expect(channel_index_page).to have_unread_channel(dm_channel_1, wait: 25) - using_session(:user_1) do |session| - create_message(channel: dm_channel_1, creator: user_1) - session.quit - end + create_message(dm_channel_1, user: user_1) expect(page).to have_css( ".chat-header-icon .chat-channel-unread-indicator", @@ -163,8 +139,6 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do end it "reorders channels" do - Jobs.run_immediately! - visit("/chat") expect(page).to have_css( @@ -174,10 +148,7 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do ".chat-channel-row:nth-child(2)[data-chat-channel-id=\"#{dm_channel_2.id}\"]", ) - using_session(:user_1) do |session| - create_message(channel: dm_channel_2, creator: user_2) - session.quit - end + create_message(dm_channel_2, user: user_2) expect(page).to have_css( ".chat-channel-row:nth-child(1)[data-chat-channel-id=\"#{dm_channel_2.id}\"]", @@ -202,21 +173,14 @@ RSpec.describe "Message notifications - mobile", type: :system, mobile: true do context "when messages are created" do it "correctly renders notifications" do - Jobs.run_immediately! - visit("/chat") - using_session(:user_1) do |session| - create_message(channel: channel_1, creator: user_1) - session.quit - end + + create_message(channel_1, user: user_1) expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator", text: "") expect(channel_index_page).to have_unread_channel(channel_1) - using_session(:user_1) do |session| - create_message(channel: dm_channel_1, creator: user_1) - session.quit - end + create_message(dm_channel_1, user: user_1) expect(channel_index_page).to have_unread_channel(dm_channel_1) expect(page).to have_css(".chat-header-icon .chat-channel-unread-indicator", text: "1") diff --git a/plugins/chat/spec/system/message_notifications_with_sidebar_spec.rb b/plugins/chat/spec/system/message_notifications_with_sidebar_spec.rb index 99778351aea..731be1b3de3 100644 --- a/plugins/chat/spec/system/message_notifications_with_sidebar_spec.rb +++ b/plugins/chat/spec/system/message_notifications_with_sidebar_spec.rb @@ -52,7 +52,6 @@ RSpec.describe "Message notifications - with sidebar", type: :system do end it "doesn’t show indicator in header" do - Jobs.run_immediately! visit("/") create_message(channel: channel_1, creator: user_1) diff --git a/plugins/chat/spec/system/message_thread_indicator_spec.rb b/plugins/chat/spec/system/message_thread_indicator_spec.rb index ab909012426..6ee0ca5093c 100644 --- a/plugins/chat/spec/system/message_thread_indicator_spec.rb +++ b/plugins/chat/spec/system/message_thread_indicator_spec.rb @@ -6,6 +6,7 @@ describe "Thread indicator for chat messages", type: :system do let(:chat_page) { PageObjects::Pages::Chat.new } let(:channel_page) { PageObjects::Pages::ChatChannel.new } + let(:thread_page) { PageObjects::Pages::ChatThread.new } let(:side_panel) { PageObjects::Pages::ChatSidePanel.new } let(:open_thread) { PageObjects::Pages::ChatThread.new } let(:chat_drawer_page) { PageObjects::Pages::ChatDrawer.new } @@ -123,50 +124,49 @@ describe "Thread indicator for chat messages", type: :system do end it "shows an excerpt of the last reply in the thread" do - thread_1.last_message.update!(message: "test for excerpt") - thread_1.last_message.rebake! + update_message( + thread_1.original_message, + user: thread_1.original_message.user, + text: "test for excerpt", + ) chat_page.visit_channel(channel) + expect( - channel_page.message_thread_indicator(thread_1.original_message).excerpt, - ).to have_content(thread_excerpt(thread_1.last_message)) + channel_page.message_thread_indicator(thread_1.original_message.reload).excerpt, + ).to have_content(thread_excerpt(thread_1.last_message.reload)) end it "updates the last reply excerpt and participants when a new message is added to the thread" do new_user = Fabricate(:user) chat_system_user_bootstrap(user: new_user, channel: channel) original_last_reply = thread_1.last_message - original_last_reply.update!(message: "test for excerpt") - original_last_reply.rebake! + update_message(original_last_reply, user: original_last_reply.user, text: "test for excerpt") chat_page.visit_channel(channel) - excerpt_text = thread_excerpt(original_last_reply) + excerpt_text = thread_excerpt(original_last_reply.reload) expect(channel_page.message_thread_indicator(thread_1.original_message)).to have_content( excerpt_text, ) - using_session(:new_user) do |session| - sign_in(new_user) - chat_page.visit_channel(channel) - channel_page.message_thread_indicator(thread_1.original_message).click - - expect(side_panel).to have_open_thread(thread_1) - - open_thread.send_message("wow i am happy to join this thread!") - end + new_message = + Fabricate( + :chat_message, + chat_channel: channel, + thread: thread_1, + user: new_user, + in_reply_to: thread_1.original_message, + use_service: true, + ) expect(channel_page.message_thread_indicator(thread_1.original_message)).to have_participant( new_user, ) - - new_user_reply = thread_1.replies.where(user: new_user).first - excerpt_text = thread_excerpt(new_user_reply) - expect( channel_page.message_thread_indicator(thread_1.original_message).excerpt, - ).to have_content(excerpt_text) + ).to have_content(thread_excerpt(new_message)) end end end diff --git a/plugins/chat/spec/system/page_objects/chat/chat_channel.rb b/plugins/chat/spec/system/page_objects/chat/chat_channel.rb index 275ffc744d3..b460be1c30c 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat_channel.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat_channel.rb @@ -98,7 +98,7 @@ module PageObjects def edit_message(message, text = nil) messages.edit(message) - send_message(message.message + text) if text + send_message(message.message + " " + text) if text end def send_message(text = nil) @@ -106,6 +106,7 @@ module PageObjects text = text.chomp if text.present? # having \n on the end of the string counts as an Enter keypress composer.fill_in(with: text) click_send_message + expect(page).to have_no_css(".chat-message.-not-processed") text end diff --git a/plugins/chat/spec/system/page_objects/chat/chat_thread.rb b/plugins/chat/spec/system/page_objects/chat/chat_thread.rb index 6dec29a6c7c..7ce96b3ebe9 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat_thread.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat_thread.rb @@ -106,6 +106,7 @@ module PageObjects text = text.chomp if text.present? # having \n on the end of the string counts as an Enter keypress composer.fill_in(with: text) click_send_message + expect(page).to have_no_css(".chat-message.-not-processed") click_composer text end @@ -141,7 +142,7 @@ module PageObjects def edit_message(message, text = nil) messages.edit(message) - send_message(message.message + text) if text + send_message(message.message + " " + text) if text end end end diff --git a/plugins/chat/spec/system/page_objects/chat/components/message.rb b/plugins/chat/spec/system/page_objects/chat/components/message.rb index d0d02f7b698..563d7aaeddd 100644 --- a/plugins/chat/spec/system/page_objects/chat/components/message.rb +++ b/plugins/chat/spec/system/page_objects/chat/components/message.rb @@ -32,7 +32,7 @@ module PageObjects def secondary_action(action) if page.has_css?("html.mobile-view", wait: 0) - component.click(delay: 0.4) + component.click(delay: 0.6) page.find(".chat-message-actions [data-id=\"#{action}\"]").click else open_more_menu @@ -52,14 +52,7 @@ module PageObjects return end - if page.has_css?("html.mobile-view", wait: 0) - component.click(delay: 0.6) - page.find(".chat-message-actions [data-id=\"select\"]").click - else - hover - click_more_button - page.find("[data-value='select']").click - end + secondary_action("select") end def find(**args) @@ -103,6 +96,13 @@ module PageObjects def build_selector(**args) selector = SELECTOR + + if args[:not_processed] + selector += ".-not-processed" + else + selector += ".-processed" + end + selector += "[data-id=\"#{args[:id]}\"]" if args[:id] selector += ".-selected" if args[:selected] selector += ".-persisted" if args[:persisted] diff --git a/plugins/chat/spec/system/reply_to_message/full_page_spec.rb b/plugins/chat/spec/system/reply_to_message/full_page_spec.rb index 55886cbea35..fbc1eaa90e8 100644 --- a/plugins/chat/spec/system/reply_to_message/full_page_spec.rb +++ b/plugins/chat/spec/system/reply_to_message/full_page_spec.rb @@ -13,6 +13,7 @@ RSpec.describe "Reply to message - channel - full page", type: :system do :chat_message, chat_channel: channel_1, message: "This is a message to reply to!", + user: current_user, use_service: true, ) end @@ -98,8 +99,11 @@ RSpec.describe "Reply to message - channel - full page", type: :system do it "renders safe HTML from the original message excerpt" do other_user = Fabricate(:user) - original_message.update!(message: "@#{other_user.username} not marked") - original_message.rebake! + update_message( + original_message, + user: current_user, + text: "@#{other_user.username} not marked", + ) chat_page.visit_channel(channel_1) channel_page.reply_to(original_message) diff --git a/plugins/chat/spec/system/thread_list/full_page_spec.rb b/plugins/chat/spec/system/thread_list/full_page_spec.rb index e9c9440a159..67ccfa8d9f9 100644 --- a/plugins/chat/spec/system/thread_list/full_page_spec.rb +++ b/plugins/chat/spec/system/thread_list/full_page_spec.rb @@ -39,7 +39,7 @@ describe "Thread list in side panel | full page", type: :system do it "does not show new threads in the channel in the thread list if the user is not tracking them" do chat_page.visit_channel(channel) - Fabricate(:chat_message_with_service, chat_channel: channel, in_reply_to: thread_om) + Fabricate(:chat_message, chat_channel: channel, in_reply_to: thread_om, use_service: true) channel_page.open_thread_list expect(page).to have_content(I18n.t("js.chat.threads.none")) @@ -89,9 +89,12 @@ describe "Thread list in side panel | full page", type: :system do expect(thread_list_page.item_by_id(thread_1.id)).to have_css("img.emoji[alt='hamburger']") end - it "shows an excerpt of the original message of the thread" do - thread_1.original_message.update!(message: "This is a long message for the excerpt") - thread_1.original_message.rebake! + it "shows an excerpt of the original message of the thread", inline: true do + update_message( + thread_1.original_message, + user: thread_1.original_message.user, + text: "This is a long message for the excerpt", + ) chat_page.visit_threads_list(channel) expect(thread_list_page.item_by_id(thread_1.id)).to have_content( diff --git a/plugins/chat/spec/system/transcript_spec.rb b/plugins/chat/spec/system/transcript_spec.rb index 619eb661583..ec93ebcaee2 100644 --- a/plugins/chat/spec/system/transcript_spec.rb +++ b/plugins/chat/spec/system/transcript_spec.rb @@ -41,7 +41,7 @@ RSpec.describe "Quoting chat message transcripts", type: :system do context "when quoting a single message into a topic" do fab!(:post_1) { Fabricate(:post) } - fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1, use_service: true) } it "quotes the message" do chat_page.visit_channel(chat_channel_1) @@ -62,8 +62,8 @@ RSpec.describe "Quoting chat message transcripts", type: :system do context "when quoting multiple messages into a topic" do fab!(:post_1) { Fabricate(:post) } - fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1) } - fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel_1) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1, use_service: true) } + fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel_1, use_service: true) } it "quotes the messages" do chat_page.visit_channel(chat_channel_1) @@ -85,7 +85,7 @@ RSpec.describe "Quoting chat message transcripts", type: :system do context "when quoting a message containing a onebox" do fab!(:post_1) { Fabricate(:post) } - fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1, use_service: true) } before do Oneboxer.stubs(:preview).returns( @@ -109,7 +109,7 @@ RSpec.describe "Quoting chat message transcripts", type: :system do end context "when quoting a message in another message" do - fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1, use_service: true) } it "quotes the message" do chat_page.visit_channel(chat_channel_1) @@ -125,7 +125,7 @@ RSpec.describe "Quoting chat message transcripts", type: :system do end context "when quoting into a topic directly" do - fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1, use_service: true) } let(:topic_title) { "Some topic title for testing" } it "opens the topic composer with correct state" do