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 e8f760dc738..8f79131829b 100644 --- a/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb +++ b/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb @@ -20,6 +20,16 @@ class Chat::Api::ChannelMessagesController < Chat::ApiController end end + def update + with_service(Chat::UpdateMessage) do + on_success { render json: success_json.merge(message_id: result[:message].id) } + on_model_not_found(:message) { raise Discourse::NotFound } + on_model_errors(:message) do |model| + render_json_error(model.errors.map(&:full_message).join(", ")) + end + end + end + def create Chat::MessageRateLimiter.run!(current_user) diff --git a/plugins/chat/app/controllers/chat/chat_controller.rb b/plugins/chat/app/controllers/chat/chat_controller.rb index 134503b4064..16ae84dca92 100644 --- a/plugins/chat/app/controllers/chat/chat_controller.rb +++ b/plugins/chat/app/controllers/chat/chat_controller.rb @@ -12,7 +12,7 @@ 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[edit_message rebake message_link] + before_action :find_chat_message, only: %i[rebake message_link] before_action :set_channel_and_chatable_with_access_check, except: %i[ respond @@ -77,20 +77,6 @@ module Chat end end - def edit_message - chat_message_updater = - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: @message, - new_content: params[:new_message], - upload_ids: params[:upload_ids] || [], - ) - - return render_json_error(chat_message_updater.error) if chat_message_updater.failed? - - render json: success_json - end - def react params.require(%i[message_id emoji react_action]) guardian.ensure_can_react! diff --git a/plugins/chat/app/serializers/chat/message_serializer.rb b/plugins/chat/app/serializers/chat/message_serializer.rb index f9fbea84dda..9c0a55a86cb 100644 --- a/plugins/chat/app/serializers/chat/message_serializer.rb +++ b/plugins/chat/app/serializers/chat/message_serializer.rb @@ -37,6 +37,7 @@ module Chat def mentioned_users object .chat_mentions + .includes(:user) .map(&:user) .compact .sort_by(&:id) diff --git a/plugins/chat/app/services/chat/update_message.rb b/plugins/chat/app/services/chat/update_message.rb new file mode 100644 index 00000000000..66356091a1b --- /dev/null +++ b/plugins/chat/app/services/chat/update_message.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +module Chat + # Service responsible for updating a message. + # + # @example + # Chat::UpdateMessage.call(message_id: 2, guardian: guardian, message: "A new message") + # + class UpdateMessage + include Service::Base + + # @!method call(message_id:, guardian:, message:, upload_ids:) + # @param guardian [Guardian] + # @param message_id [Integer] + # @param message [String] + # @param upload_ids [Array] IDs of uploaded documents + + contract + model :message + model :uploads, optional: true + policy :can_modify_channel_message + policy :can_modify_message + + transaction do + step :modify_message + step :save_message + step :save_revision + step :publish + end + + class Contract + attribute :message_id, :string + attribute :message, :string + attribute :upload_ids, :array + + validates :message_id, presence: true + validates :message, presence: true, if: -> { upload_ids.blank? } + end + + private + + 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) + end + + def fetch_uploads(contract:, guardian:, **) + return if !SiteSetting.chat_allow_uploads + guardian.user.uploads.where(id: contract.upload_ids) + end + + def can_modify_channel_message(guardian:, message:, **) + guardian.can_modify_channel_message?(message.chat_channel) + end + + def can_modify_message(guardian:, message:, **) + guardian.can_edit_chat?(message) + end + + def modify_message(contract:, message:, guardian:, uploads:, **) + message.message = contract.message + message.last_editor_id = guardian.user.id + + return if uploads&.size != contract.upload_ids.to_a.size + + new_upload_ids = uploads.map(&:id) + existing_upload_ids = message.upload_ids + difference = (existing_upload_ids + new_upload_ids) - (existing_upload_ids & new_upload_ids) + return if !difference.any? + + message.upload_ids = new_upload_ids + end + + def save_message(message:, **) + message.cook + message.save! + message.update_mentions + end + + def save_revision(message:, guardian:, **) + prev_message = message.message_before_last_save || message.message_was + return if !should_create_revision(message, prev_message, guardian) + + context.revision = + message.revisions.create!( + old_message: prev_message, + new_message: message.message, + user_id: guardian.user.id, + ) + end + + def should_create_revision(new_message, prev_message, guardian) + max_seconds = SiteSetting.chat_editing_grace_period + seconds_since_created = Time.now.to_i - new_message&.created_at.iso8601.to_time.to_i + return true if seconds_since_created > max_seconds + + max_edited_chars = + ( + if guardian.user.has_trust_level?(TrustLevel[2]) + SiteSetting.chat_editing_grace_period_max_diff_high_trust + else + SiteSetting.chat_editing_grace_period_max_diff_low_trust + end + ) + chars_edited = + ONPDiff + .new(prev_message, new_message.message) + .short_diff + .sum { |str, type| type == :common ? 0 : str.size } + + 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 }) + + 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 message.thread.present? + ::Chat::Publisher.publish_thread_original_message_metadata!(message.thread) + end + end + end +end diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js index ff8225a121b..8945c022072 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.js @@ -526,7 +526,7 @@ export default class ChatChannel extends Component { this.pane.sending = true; const data = { - new_message: message.message, + message: message.message, upload_ids: message.uploads.map((upload) => upload.id), }; diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-thread.js b/plugins/chat/assets/javascripts/discourse/components/chat-thread.js index 2a0e9a418a0..f144d9c91fa 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-thread.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-thread.js @@ -391,7 +391,7 @@ export default class ChatThread extends Component { this.chatThreadPane.sending = true; const data = { - new_message: message.message, + message: message.message, upload_ids: message.uploads.map((upload) => upload.id), }; diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-api.js b/plugins/chat/assets/javascripts/discourse/services/chat-api.js index f4ab3c79068..4212a76b0fc 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-api.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-api.js @@ -386,10 +386,10 @@ export default class ChatApi extends Service { * @param {Array} data.upload_ids - The uploads attached to the message after editing. */ editMessage(channelId, messageId, data) { - return ajax(`/chat/${channelId}/edit/${messageId}`, { - type: "PUT", - data, - }); + return this.#putRequest( + `/channels/${channelId}/messages/${messageId}`, + data + ); } /** 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 3e2e158a971..d0119e1e143 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 @@ -148,7 +148,7 @@ export default class ChatPaneBaseSubscriptionsManager extends Service { message.cooked = data.chat_message.cooked; message.excerpt = data.chat_message.excerpt; message.uploads = cloneJSON(data.chat_message.uploads || []); - message.edited = true; + message.edited = data.chat_message.edited; } } diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml index 3090c5c7dba..63709634e9d 100644 --- a/plugins/chat/config/locales/server.en.yml +++ b/plugins/chat/config/locales/server.en.yml @@ -22,6 +22,9 @@ en: max_mentions_per_chat_message: "Maximum number of @name notifications a user can use in a chat message." chat_max_direct_message_users: "Users cannot add more than this number of other users when creating a new direct message. Set to 0 to only allow messages to oneself. Staff are exempt from this setting." chat_allow_archiving_channels: "Allow staff to archive messages to a topic when closing a channel." + chat_editing_grace_period: "For (n) seconds after sending a chat, editing will not display the (edited) tag by the chat message." + chat_editing_grace_period_max_diff_low_trust: "Maximum number of character changes allowed in chat editing grace period, if more changed display the (edited) tag by the chat message (trust level 0 and 1)." + chat_editing_grace_period_max_diff_high_trust: "Maximum number of character changes allowed in chat editing grace period, if more changed display the (edited) tag by the chat message (trust level 2 and up)." errors: chat_default_channel: "The default chat channel must be a public channel." direct_message_enabled_groups_invalid: "You must specify at least one group for this setting. If you do not want anyone except staff to send direct messages, choose the staff group." diff --git a/plugins/chat/config/routes.rb b/plugins/chat/config/routes.rb index 7404c0a59d4..5d7f5a86375 100644 --- a/plugins/chat/config/routes.rb +++ b/plugins/chat/config/routes.rb @@ -13,6 +13,7 @@ Chat::Engine.routes.draw do get "/channels/:channel_id" => "channels#show" put "/channels/:channel_id/status" => "channels_status#update" get "/channels/:channel_id/messages" => "channel_messages#index" + put "/channels/:channel_id/messages/:message_id" => "channel_messages#update" post "/channels/:channel_id/messages/moves" => "channels_messages_moves#create" post "/channels/:channel_id/archives" => "channels_archives#create" get "/channels/:channel_id/memberships" => "channels_memberships#index" @@ -72,7 +73,6 @@ Chat::Engine.routes.draw do post "/disable" => "chat#disable_chat" post "/dismiss-retention-reminder" => "chat#dismiss_retention_reminder" 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" diff --git a/plugins/chat/config/settings.yml b/plugins/chat/config/settings.yml index dd5e54ecb7c..15ff53917f8 100644 --- a/plugins/chat/config/settings.yml +++ b/plugins/chat/config/settings.yml @@ -121,3 +121,18 @@ chat: default: "never" type: enum enum: "Chat::SeparateSidebarModeSiteSetting" + chat_editing_grace_period: + client: true + type: integer + default: 30 + min: 0 + chat_editing_grace_period_max_diff_low_trust: + client: true + type: integer + default: 10 + min: 0 + chat_editing_grace_period_max_diff_high_trust: + client: true + type: integer + default: 40 + min: 0 diff --git a/plugins/chat/lib/chat/message_updater.rb b/plugins/chat/lib/chat/message_updater.rb deleted file mode 100644 index 04eb7ba5e6b..00000000000 --- a/plugins/chat/lib/chat/message_updater.rb +++ /dev/null @@ -1,94 +0,0 @@ -# frozen_string_literal: true - -module Chat - class MessageUpdater - attr_reader :error - - def self.update(opts) - instance = new(**opts) - instance.update - instance - end - - def initialize(guardian:, chat_message:, new_content:, upload_ids: nil) - @guardian = guardian - @user = guardian.user - @chat_message = chat_message - @old_message_content = chat_message.message - @chat_channel = @chat_message.chat_channel - @new_content = new_content - @upload_ids = upload_ids - @error = nil - end - - def update - begin - validate_channel_status! - @guardian.ensure_can_edit_chat!(@chat_message) - @chat_message.message = @new_content - @chat_message.last_editor_id = @user.id - upload_info = get_upload_info - @chat_message.uploads = upload_info[:uploads] if upload_info[:changed] - validate_message! - @chat_message.cook - @chat_message.save! - - @chat_message.update_mentions - revision = save_revision! - - @chat_message.reload - Chat::Publisher.publish_edit!(@chat_channel, @chat_message) - Jobs.enqueue(Jobs::Chat::ProcessMessage, { chat_message_id: @chat_message.id }) - Chat::Notifier.notify_edit(chat_message: @chat_message, timestamp: revision.created_at) - DiscourseEvent.trigger(:chat_message_edited, @chat_message, @chat_channel, @user) - - if @chat_message.thread.present? - Chat::Publisher.publish_thread_original_message_metadata!(@chat_message.thread) - end - rescue => error - @error = error - end - end - - def failed? - @error.present? - end - - private - - def validate_channel_status! - return if @guardian.can_modify_channel_message?(@chat_channel) - raise StandardError.new( - I18n.t("chat.errors.channel_modify_message_disallowed.#{@chat_channel.status}"), - ) - end - - def validate_message! - return if @chat_message.valid? - raise StandardError.new(@chat_message.errors.map(&:full_message).join(", ")) - end - - def get_upload_info - return { uploads: [] } if @upload_ids.nil? || !SiteSetting.chat_allow_uploads - - uploads = ::Upload.where(id: @upload_ids, user_id: @user.id) - if uploads.count != @upload_ids.count - # User is passing upload_ids for uploads that they don't own. Don't change anything. - return { uploads: @chat_message.uploads, changed: false } - end - - new_upload_ids = uploads.map(&:id) - existing_upload_ids = @chat_message.upload_ids - difference = (existing_upload_ids + new_upload_ids) - (existing_upload_ids & new_upload_ids) - { uploads: uploads, changed: difference.any? } - end - - def save_revision! - @chat_message.revisions.create!( - old_message: @old_message_content, - new_message: @chat_message.message, - user_id: @user.id, - ) - end - end -end diff --git a/plugins/chat/spec/components/chat/message_updater_spec.rb b/plugins/chat/spec/components/chat/message_updater_spec.rb deleted file mode 100644 index ba498506915..00000000000 --- a/plugins/chat/spec/components/chat/message_updater_spec.rb +++ /dev/null @@ -1,720 +0,0 @@ -# frozen_string_literal: true - -require "rails_helper" - -describe Chat::MessageUpdater do - let(:guardian) { Guardian.new(user1) } - fab!(:admin1) { Fabricate(:admin) } - fab!(:admin2) { Fabricate(:admin) } - fab!(:user1) { Fabricate(:user) } - fab!(:user2) { Fabricate(:user) } - fab!(:user3) { Fabricate(:user) } - fab!(:user4) { Fabricate(:user) } - fab!(:admin_group) do - Fabricate( - :public_group, - users: [admin1, admin2], - mentionable_level: Group::ALIAS_LEVELS[:everyone], - ) - end - fab!(:user_without_memberships) { Fabricate(:user) } - fab!(:public_chat_channel) { Fabricate(:category_channel) } - - before do - SiteSetting.chat_enabled = true - SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone] - SiteSetting.chat_duplicate_message_sensitivity = 0 - Jobs.run_immediately! - - [admin1, admin2, user1, user2, user3, user4].each do |user| - Fabricate(:user_chat_channel_membership, chat_channel: public_chat_channel, user: user) - end - Group.refresh_automatic_groups! - end - - def create_chat_message(user, message, channel, upload_ids: nil) - Fabricate( - :chat_message, - chat_channel: channel, - user: user, - message: message, - upload_ids: upload_ids, - ) - end - - it "errors when length is less than `chat_minimum_message_length`" do - SiteSetting.chat_minimum_message_length = 10 - og_message = "This won't be changed!" - chat_message = create_chat_message(user1, og_message, public_chat_channel) - new_message = "2 short" - - updater = - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: new_message, - ) - expect(updater.failed?).to eq(true) - expect(updater.error.message).to match( - I18n.t( - "chat.errors.minimum_length_not_met", - { count: SiteSetting.chat_minimum_message_length }, - ), - ) - expect(chat_message.reload.message).to eq(og_message) - end - - it "errors when length is greater than `chat_maximum_message_length`" do - SiteSetting.chat_maximum_message_length = 100 - og_message = "This won't be changed!" - chat_message = create_chat_message(user1, og_message, public_chat_channel) - new_message = "2 long" * 100 - - updater = - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: new_message, - ) - expect(updater.failed?).to eq(true) - expect(updater.error.message).to match( - I18n.t("chat.errors.message_too_long", { count: SiteSetting.chat_maximum_message_length }), - ) - expect(chat_message.reload.message).to eq(og_message) - end - - it "errors when a blank message is sent" do - og_message = "This won't be changed!" - chat_message = create_chat_message(user1, og_message, public_chat_channel) - new_message = " " - - updater = - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: new_message, - ) - expect(updater.failed?).to eq(true) - expect(updater.error.message).to match( - I18n.t( - "chat.errors.minimum_length_not_met", - { count: SiteSetting.chat_minimum_message_length }, - ), - ) - expect(chat_message.reload.message).to eq(og_message) - end - - it "errors if a user other than the message user is trying to edit the message" do - og_message = "This won't be changed!" - chat_message = create_chat_message(user1, og_message, public_chat_channel) - new_message = "2 short" - updater = - Chat::MessageUpdater.update( - guardian: Guardian.new(Fabricate(:user)), - chat_message: chat_message, - new_content: new_message, - ) - expect(updater.failed?).to eq(true) - expect(updater.error).to match(Discourse::InvalidAccess) - end - - it "it updates a messages content" do - chat_message = create_chat_message(user1, "This will be changed", public_chat_channel) - new_message = "Change to this!" - - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: new_message, - ) - expect(chat_message.reload.message).to eq(new_message) - end - - it "publishes a DiscourseEvent for updated messages" do - chat_message = create_chat_message(user1, "This will be changed", public_chat_channel) - events = - DiscourseEvent.track_events do - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "Change to this!", - ) - end - expect(events.map { _1[:event_name] }).to include(:chat_message_edited) - end - - 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.update( - guardian: guardian, - chat_message: chat_message, - new_content: new_content, - ) - end - - expect(messages.count).to be(1) - message = messages[0].data - expect(message["chat_message"]["message"]).to eq(new_content) - end - - context "with mentions" do - it "sends notifications if a message was updated with new mentions" do - message = create_chat_message(user1, "Mentioning @#{user2.username}", public_chat_channel) - - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: message, - new_content: "Mentioning @#{user2.username} and @#{user3.username}", - ) - - mention = user3.chat_mentions.where(chat_message: message).first - expect(mention.notification).to be_present - end - - it "doesn't create mentions for already mentioned users" do - message = "ping @#{user2.username} @#{user3.username}" - chat_message = create_chat_message(user1, message, public_chat_channel) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: message + " editedddd", - ) - }.not_to change { Chat::Mention.count } - end - - it "doesn't create mention notification for users without access" do - message = "ping" - chat_message = create_chat_message(user1, message, public_chat_channel) - - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: message + " @#{user_without_memberships.username}", - ) - - mention = user_without_memberships.chat_mentions.where(chat_message: chat_message).first - expect(mention.notification).to be_nil - end - - it "destroys mentions that should be removed" do - chat_message = - create_chat_message( - user1, - "ping @#{user2.username} @#{user3.username}", - public_chat_channel, - ) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "ping @#{user3.username}", - ) - }.to change { user2.chat_mentions.count }.by(-1).and not_change { user3.chat_mentions.count } - end - - it "creates new, leaves existing, and removes old mentions all at once" do - chat_message = - create_chat_message( - user1, - "ping @#{user2.username} @#{user3.username}", - public_chat_channel, - ) - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "ping @#{user3.username} @#{user4.username}", - ) - - expect(user2.chat_mentions.where(chat_message: chat_message)).not_to be_present - expect(user3.chat_mentions.where(chat_message: chat_message)).to be_present - expect(user4.chat_mentions.where(chat_message: chat_message)).to be_present - end - - it "doesn't create mention notification in direct message for users without access" do - result = - Chat::CreateDirectMessageChannel.call( - guardian: user1.guardian, - target_usernames: [user1.username, user2.username], - ) - service_failed!(result) if result.failure? - direct_message_channel = result.channel - message = create_chat_message(user1, "ping nobody", direct_message_channel) - - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: message, - new_content: "ping @#{admin1.username}", - ) - - mention = admin1.chat_mentions.where(chat_message: message).first - expect(mention.notification).to be_nil - end - - it "creates a chat_mention record without notification when self mentioning" do - chat_message = create_chat_message(user1, "I will mention myself soon", public_chat_channel) - new_content = "hello @#{user1.username}" - - described_class.update( - guardian: guardian, - chat_message: chat_message, - new_content: new_content, - ) - - mention = user1.chat_mentions.where(chat_message: chat_message).first - expect(mention).to be_present - expect(mention.notification).to be_nil - end - - it "adds mentioned user and their status to the message bus message" do - SiteSetting.enable_user_status = true - status = { description: "dentist", emoji: "tooth" } - user2.set_status!(status[:description], status[:emoji]) - 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.update( - guardian: guardian, - chat_message: chat_message, - new_content: new_content, - ) - end - - 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(mentioned_user["id"]).to eq(user2.id) - expect(mentioned_user["username"]).to eq(user2.username) - expect(mentioned_user["status"]).to be_present - expect(mentioned_user["status"].slice(:description, :emoji)).to eq(status) - end - - it "doesn't add mentioned user's status to the message bus message when status is disabled" 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.update( - guardian: guardian, - chat_message: chat_message, - new_content: new_content, - ) - end - - 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(mentioned_user["status"]).to be_blank - end - - context "when updating a mentioned user" do - it "updates the mention record" do - chat_message = create_chat_message(user1, "ping @#{user2.username}", public_chat_channel) - - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "ping @#{user3.username}", - ) - - user2_mentions = user2.chat_mentions.where(chat_message: chat_message) - expect(user2_mentions.length).to be(0) - - user3_mentions = user3.chat_mentions.where(chat_message: chat_message) - expect(user3_mentions.length).to be(1) - end - end - - context "when there are duplicate mentions" do - it "creates a single mention record per user" do - chat_message = create_chat_message(user1, "ping @#{user2.username}", public_chat_channel) - - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "ping @#{user2.username} @#{user2.username} edited", - ) - - expect(user2.chat_mentions.where(chat_message: chat_message).count).to eq(1) - end - end - - describe "with group mentions" do - it "creates group mentions on update" do - chat_message = create_chat_message(user1, "ping nobody", public_chat_channel) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "ping @#{admin_group.name}", - ) - }.to change { Chat::Mention.where(chat_message: chat_message).count }.by(2) - - expect(admin1.chat_mentions.where(chat_message: chat_message)).to be_present - expect(admin2.chat_mentions.where(chat_message: chat_message)).to be_present - end - - it "doesn't duplicate mentions when the user is already direct mentioned and then group mentioned" do - chat_message = create_chat_message(user1, "ping @#{admin2.username}", public_chat_channel) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "ping @#{admin_group.name} @#{admin2.username}", - ) - }.to change { admin1.chat_mentions.count }.by(1).and not_change { - admin2.chat_mentions.count - } - end - - it "deletes old mentions when group mention is removed" do - chat_message = create_chat_message(user1, "ping @#{admin_group.name}", public_chat_channel) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "ping nobody anymore!", - ) - }.to change { Chat::Mention.where(chat_message: chat_message).count }.by(-2) - - expect(admin1.chat_mentions.where(chat_message: chat_message)).not_to be_present - expect(admin2.chat_mentions.where(chat_message: chat_message)).not_to be_present - end - end - end - - it "creates a chat_message_revision record and sets last_editor_id for the message" do - old_message = "It's a thrsday!" - new_message = "It's a thursday!" - chat_message = create_chat_message(user1, old_message, public_chat_channel) - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: new_message, - ) - revision = chat_message.revisions.last - expect(revision.old_message).to eq(old_message) - expect(revision.new_message).to eq(new_message) - expect(revision.user_id).to eq(guardian.user.id) - expect(chat_message.reload.last_editor_id).to eq(guardian.user.id) - end - - describe "duplicates" do - fab!(:upload1) { Fabricate(:upload, user: user1) } - fab!(:upload2) { Fabricate(:upload, user: user1) } - - before do - SiteSetting.chat_duplicate_message_sensitivity = 1.0 - public_chat_channel.update!(user_count: 50) - end - - it "errors when editing the message to be the same as one that was posted recently" do - chat_message_1 = create_chat_message(user1, "this is some chat message", public_chat_channel) - chat_message_2 = - create_chat_message( - Fabricate(:user), - "another different chat message here", - public_chat_channel, - ) - - chat_message_1.update!(created_at: 30.seconds.ago) - chat_message_2.update!(created_at: 20.seconds.ago) - - updater = - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message_1, - new_content: "another different chat message here", - ) - expect(updater.failed?).to eq(true) - expect(updater.error.message).to eq(I18n.t("chat.errors.duplicate_message")) - end - - it "does not count the message as a duplicate when editing leaves the message the same but changes uploads" do - chat_message = - create_chat_message( - user1, - "this is some chat message", - public_chat_channel, - upload_ids: [upload1.id, upload2.id], - ) - chat_message.update!(created_at: 30.seconds.ago) - - updater = - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "this is some chat message", - upload_ids: [upload2.id], - ) - expect(updater.failed?).to eq(false) - expect(chat_message.reload.uploads.count).to eq(1) - end - end - - describe "uploads" do - fab!(:upload1) { Fabricate(:upload, user: user1) } - fab!(:upload2) { Fabricate(:upload, user: user1) } - - it "does nothing if the passed in upload_ids match the existing upload_ids" do - chat_message = - create_chat_message( - user1, - "something", - public_chat_channel, - upload_ids: [upload1.id, upload2.id], - ) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "I guess this is different", - upload_ids: [upload2.id, upload1.id], - ) - }.to not_change { UploadReference.count } - end - - it "removes uploads that should be removed" do - chat_message = - create_chat_message( - user1, - "something", - public_chat_channel, - upload_ids: [upload1.id, upload2.id], - ) - - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "I guess this is different", - upload_ids: [upload1.id], - ) - }.to change { UploadReference.where(upload_id: upload2.id).count }.by(-1) - end - - it "removes all uploads if they should be removed" do - chat_message = - create_chat_message( - user1, - "something", - public_chat_channel, - upload_ids: [upload1.id, upload2.id], - ) - - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "I guess this is different", - upload_ids: [], - ) - }.to change { UploadReference.where(target: chat_message).count }.by(-2) - end - - it "adds one upload if none exist" do - chat_message = create_chat_message(user1, "something", public_chat_channel) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "I guess this is different", - upload_ids: [upload1.id], - ) - }.to change { UploadReference.where(target: chat_message).count }.by(1) - end - - it "adds multiple uploads if none exist" do - chat_message = create_chat_message(user1, "something", public_chat_channel) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "I guess this is different", - upload_ids: [upload1.id, upload2.id], - ) - }.to change { UploadReference.where(target: chat_message).count }.by(2) - end - - it "doesn't remove existing uploads when upload ids that do not exist are passed in" do - chat_message = - create_chat_message(user1, "something", public_chat_channel, upload_ids: [upload1.id]) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "I guess this is different", - upload_ids: [0], - ) - }.to not_change { UploadReference.where(target: chat_message).count } - end - - it "doesn't add uploads if `chat_allow_uploads` is false" do - SiteSetting.chat_allow_uploads = false - chat_message = create_chat_message(user1, "something", public_chat_channel) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "I guess this is different", - upload_ids: [upload1.id, upload2.id], - ) - }.to not_change { UploadReference.where(target: chat_message).count } - end - - it "doesn't remove existing uploads if `chat_allow_uploads` is false" do - SiteSetting.chat_allow_uploads = false - chat_message = - create_chat_message( - user1, - "something", - public_chat_channel, - upload_ids: [upload1.id, upload2.id], - ) - expect { - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "I guess this is different", - upload_ids: [], - ) - }.to not_change { UploadReference.where(target: chat_message).count } - end - - it "updates if upload is present even if length is less than `chat_minimum_message_length`" do - chat_message = - create_chat_message( - user1, - "something", - public_chat_channel, - upload_ids: [upload1.id, upload2.id], - ) - SiteSetting.chat_minimum_message_length = 10 - new_message = "hi :)" - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: new_message, - upload_ids: [upload1.id], - ) - expect(chat_message.reload.message).to eq(new_message) - end - end - - context "when the message is in a thread" do - fab!(:message) do - Fabricate( - :chat_message, - user: user1, - chat_channel: public_chat_channel, - thread: Fabricate(:chat_thread, channel: public_chat_channel), - ) - end - - it "publishes a MessageBus event to update the original message metadata" do - messages = - MessageBus.track_publish("/chat/#{public_chat_channel.id}") do - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: message, - new_content: "some new updated content", - ) - end - expect(messages.find { |m| m.data["type"] == "update_thread_original_message" }).to be_present - end - end - - describe "watched words" do - fab!(:watched_word) { Fabricate(:watched_word) } - - it "errors when a blocked word is present" do - chat_message = create_chat_message(user1, "something", public_chat_channel) - updater = - Chat::MessageUpdater.update( - guardian: guardian, - chat_message: chat_message, - new_content: "bad word - #{watched_word.word}", - ) - expect(updater.failed?).to eq(true) - expect(updater.error.message).to match( - I18n.t("contains_blocked_word", { word: watched_word.word }), - ) - end - end - - describe "channel statuses" do - fab!(:message) { Fabricate(:chat_message, user: user1, chat_channel: public_chat_channel) } - - def update_message(user) - message.update(user: user) - Chat::MessageUpdater.update( - guardian: Guardian.new(user), - chat_message: message, - new_content: "I guess this is different", - ) - end - - context "when channel is closed" do - before { public_chat_channel.update(status: :closed) } - - it "errors when trying to update the message for non-staff" do - updater = update_message(user1) - expect(updater.failed?).to eq(true) - expect(updater.error.message).to eq( - I18n.t("chat.errors.channel_modify_message_disallowed.closed"), - ) - end - - it "does not error when trying to create a message for staff" do - update_message(admin1) - expect(message.reload.message).to eq("I guess this is different") - end - end - - context "when channel is read_only" do - before { public_chat_channel.update(status: :read_only) } - - it "errors when trying to update the message for all users" do - updater = update_message(user1) - expect(updater.failed?).to eq(true) - expect(updater.error.message).to eq( - I18n.t("chat.errors.channel_modify_message_disallowed.read_only"), - ) - updater = update_message(admin1) - expect(updater.failed?).to eq(true) - expect(updater.error.message).to eq( - I18n.t("chat.errors.channel_modify_message_disallowed.read_only"), - ) - end - end - - context "when channel is archived" do - before { public_chat_channel.update(status: :archived) } - - it "errors when trying to update the message for all users" do - updater = update_message(user1) - expect(updater.failed?).to eq(true) - expect(updater.error.message).to eq( - I18n.t("chat.errors.channel_modify_message_disallowed.archived"), - ) - updater = update_message(admin1) - expect(updater.failed?).to eq(true) - expect(updater.error.message).to eq( - I18n.t("chat.errors.channel_modify_message_disallowed.archived"), - ) - end - end - end -end diff --git a/plugins/chat/spec/integration/outgoing_web_hooks_spec.rb b/plugins/chat/spec/integration/outgoing_web_hooks_spec.rb index a951dc6fd7c..18c8ef67c4b 100644 --- a/plugins/chat/spec/integration/outgoing_web_hooks_spec.rb +++ b/plugins/chat/spec/integration/outgoing_web_hooks_spec.rb @@ -129,9 +129,9 @@ RSpec.describe "Outgoing chat webhooks" do end it "triggers a webhook when a chat message is edited" do - put "/chat/#{chat_channel.id}/edit/#{chat_message.id}.json", + put "/chat/api/channels/#{chat_channel.id}/messages/#{chat_message.id}.json", params: { - new_message: new_message_content, + message: new_message_content, } expect_response_to_be_successful @@ -195,9 +195,9 @@ RSpec.describe "Outgoing chat webhooks" do end it "triggers a webhook when a chat message is edited" do - put "/chat/#{direct_message_channel.id}/edit/#{chat_message.id}.json", + put "/chat/api/channels/#{direct_message_channel.id}/messages/#{chat_message.id}.json", params: { - new_message: new_message_content, + message: new_message_content, } expect_response_to_be_successful diff --git a/plugins/chat/spec/lib/chat/notifier_spec.rb b/plugins/chat/spec/lib/chat/notifier_spec.rb index 88c9ddde707..b92a257b134 100644 --- a/plugins/chat/spec/lib/chat/notifier_spec.rb +++ b/plugins/chat/spec/lib/chat/notifier_spec.rb @@ -145,10 +145,10 @@ describe Chat::Notifier do Jobs.run_immediately! msg = build_cooked_msg(mention, user_1) - Chat::MessageUpdater.update( + Chat::UpdateMessage.call( guardian: user_1.guardian, - chat_message: msg, - new_content: "hello @all", + message_id: msg.id, + message: "hello @all", ) described_class.new(msg, msg.created_at).notify_edit diff --git a/plugins/chat/spec/lib/chat/review_queue_spec.rb b/plugins/chat/spec/lib/chat/review_queue_spec.rb index 59f9fcaddcd..90faf80ca82 100644 --- a/plugins/chat/spec/lib/chat/review_queue_spec.rb +++ b/plugins/chat/spec/lib/chat/review_queue_spec.rb @@ -116,10 +116,10 @@ describe Chat::ReviewQueue do end it "ignores the cooldown window when the message is edited" do - Chat::MessageUpdater.update( + Chat::UpdateMessage.call( guardian: Guardian.new(message.user), - chat_message: message, - new_content: "I'm editing this message. Please flag it.", + message_id: message.id, + message: "I'm editing this message. Please flag it.", ) expect(second_flag_result).to include success: true diff --git a/plugins/chat/spec/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb index 45df5cb9c3e..54ef2de8eea 100644 --- a/plugins/chat/spec/requests/chat_controller_spec.rb +++ b/plugins/chat/spec/requests/chat_controller_spec.rb @@ -158,7 +158,7 @@ RSpec.describe Chat::ChatController do end end - describe "#edit_message" do + xdescribe "#edit_message" do fab!(:chat_message) { Fabricate(:chat_message, chat_channel: chat_channel, user: user) } context "when current user is silenced" do diff --git a/plugins/chat/spec/services/chat/update_message_spec.rb b/plugins/chat/spec/services/chat/update_message_spec.rb new file mode 100644 index 00000000000..6548a0b95ce --- /dev/null +++ b/plugins/chat/spec/services/chat/update_message_spec.rb @@ -0,0 +1,843 @@ +# frozen_string_literal: true + +RSpec.describe Chat::UpdateMessage do + describe described_class::Contract, type: :model do + subject(:contract) { described_class.new(upload_ids: upload_ids) } + + let(:upload_ids) { nil } + + it { is_expected.to validate_presence_of :message_id } + + context "when uploads are not provided" do + it { is_expected.to validate_presence_of :message } + end + + context "when uploads are provided" do + let(:upload_ids) { "2,3" } + + it { is_expected.not_to validate_presence_of :message } + end + end + + describe "with validation" do + let(:guardian) { Guardian.new(user1) } + fab!(:admin1) { Fabricate(:admin) } + fab!(:admin2) { Fabricate(:admin) } + fab!(:user1) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + fab!(:user3) { Fabricate(:user) } + fab!(:user4) { Fabricate(:user) } + fab!(:admin_group) do + Fabricate( + :public_group, + users: [admin1, admin2], + mentionable_level: Group::ALIAS_LEVELS[:everyone], + ) + end + fab!(:user_without_memberships) { Fabricate(:user) } + fab!(:public_chat_channel) { Fabricate(:category_channel) } + + before do + SiteSetting.chat_enabled = true + SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone] + SiteSetting.chat_duplicate_message_sensitivity = 0 + Jobs.run_immediately! + + [admin1, admin2, user1, user2, user3, user4].each do |user| + Fabricate(:user_chat_channel_membership, chat_channel: public_chat_channel, user: user) + end + Group.refresh_automatic_groups! + end + + def create_chat_message(user, message, channel, upload_ids: nil) + Fabricate( + :chat_message, + chat_channel: channel, + user: user, + message: message, + upload_ids: upload_ids, + ) + end + + it "errors when length is less than `chat_minimum_message_length`" do + SiteSetting.chat_minimum_message_length = 10 + og_message = "This won't be changed!" + chat_message = create_chat_message(user1, og_message, public_chat_channel) + new_message = "2 short" + + expect do + update = + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: new_message, + ) + end.to raise_error(ActiveRecord::RecordInvalid).with_message( + "Validation failed: " + + I18n.t( + "chat.errors.minimum_length_not_met", + { count: SiteSetting.chat_minimum_message_length }, + ), + ) + + expect(chat_message.reload.message).to eq(og_message) + end + + it "errors when length is greater than `chat_maximum_message_length`" do + SiteSetting.chat_maximum_message_length = 100 + og_message = "This won't be changed!" + chat_message = create_chat_message(user1, og_message, public_chat_channel) + new_message = "2 long" * 100 + + expect do + described_class.call(guardian: guardian, message_id: chat_message.id, message: new_message) + end.to raise_error(ActiveRecord::RecordInvalid).with_message( + "Validation failed: " + + I18n.t( + "chat.errors.message_too_long", + { count: SiteSetting.chat_maximum_message_length }, + ), + ) + + expect(chat_message.reload.message).to eq(og_message) + end + + it "errors when a blank message is sent" do + og_message = "This won't be changed!" + chat_message = create_chat_message(user1, og_message, public_chat_channel) + new_message = " " + + updater = + described_class.call(guardian: guardian, message_id: chat_message.id, message: new_message) + + expect(updater.contract).not_to be_valid + expect(updater.contract.errors.added?(:message, :blank)).to be_truthy + expect(chat_message.reload.message).to eq(og_message) + end + + it "errors if a user other than the message user is trying to edit the message" do + og_message = "This won't be changed!" + chat_message = create_chat_message(user1, og_message, public_chat_channel) + new_message = "2 short" + updater = + described_class.call( + guardian: Guardian.new(Fabricate(:user)), + message_id: chat_message.id, + message: new_message, + ) + + expect(updater.message.reload.message).not_to eq(new_message) + end + + it "updates a message's content" do + chat_message = create_chat_message(user1, "This will be changed", public_chat_channel) + new_message = "Change to this!" + + described_class.call(guardian: guardian, message_id: chat_message.id, message: new_message) + expect(chat_message.reload.message).to eq(new_message) + end + + it "publishes a DiscourseEvent for updated messages" do + chat_message = create_chat_message(user1, "This will be changed", public_chat_channel) + events = + DiscourseEvent.track_events do + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "Change to this!", + ) + end + expect(events.map { _1[:event_name] }).to include(:chat_message_edited) + end + + 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) + end + + context "with mentions" do + it "sends notifications if a message was updated with new mentions" do + message = create_chat_message(user1, "Mentioning @#{user2.username}", public_chat_channel) + + described_class.call( + guardian: guardian, + message_id: message.id, + message: "Mentioning @#{user2.username} and @#{user3.username}", + ) + + mention = user3.chat_mentions.where(chat_message: message.id).first + expect(mention.notification).to be_present + end + + it "doesn't create mentions for already mentioned users" do + message = "ping @#{user2.username} @#{user3.username}" + chat_message = create_chat_message(user1, message, public_chat_channel) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: message + " editedddd", + ) + }.not_to change { Chat::Mention.count } + end + + it "doesn't create mention notification for users without access" do + message = "ping" + chat_message = create_chat_message(user1, message, public_chat_channel) + + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: message + " @#{user_without_memberships.username}", + ) + + mention = user_without_memberships.chat_mentions.where(chat_message: chat_message).first + expect(mention.notification).to be_nil + end + + it "destroys mentions that should be removed" do + chat_message = + create_chat_message( + user1, + "ping @#{user2.username} @#{user3.username}", + public_chat_channel, + ) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "ping @#{user3.username}", + ) + }.to change { user2.chat_mentions.count }.by(-1).and not_change { + user3.chat_mentions.count + } + end + + it "creates new, leaves existing, and removes old mentions all at once" do + chat_message = + create_chat_message( + user1, + "ping @#{user2.username} @#{user3.username}", + public_chat_channel, + ) + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "ping @#{user3.username} @#{user4.username}", + ) + + expect(user2.chat_mentions.where(chat_message: chat_message)).not_to be_present + expect(user3.chat_mentions.where(chat_message: chat_message)).to be_present + expect(user4.chat_mentions.where(chat_message: chat_message)).to be_present + end + + it "doesn't create mention notification in direct message for users without access" do + result = + Chat::CreateDirectMessageChannel.call( + guardian: user1.guardian, + target_usernames: [user1.username, user2.username], + ) + service_failed!(result) if result.failure? + direct_message_channel = result.channel + message = create_chat_message(user1, "ping nobody", direct_message_channel) + + described_class.call( + guardian: guardian, + message_id: message.id, + message: "ping @#{admin1.username}", + ) + + mention = admin1.chat_mentions.where(chat_message_id: message.id).first + expect(mention.notification).to be_nil + end + + it "creates a chat_mention record without notification when self mentioning" do + chat_message = create_chat_message(user1, "I will mention myself soon", public_chat_channel) + new_content = "hello @#{user1.username}" + + described_class.call(guardian: guardian, message_id: chat_message.id, message: new_content) + + mention = user1.chat_mentions.where(chat_message: chat_message).first + expect(mention).to be_present + expect(mention.notification).to be_nil + end + + it "adds mentioned user and their status to the message bus message" do + SiteSetting.enable_user_status = true + status = { description: "dentist", emoji: "tooth" } + user2.set_status!(status[:description], status[:emoji]) + 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 + + 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(mentioned_user["id"]).to eq(user2.id) + expect(mentioned_user["username"]).to eq(user2.username) + expect(mentioned_user["status"]).to be_present + expect(mentioned_user["status"].slice(:description, :emoji)).to eq(status) + end + + it "doesn't add mentioned user's status to the message bus message when status is disabled" 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 + + 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(mentioned_user["status"]).to be_blank + end + + context "when updating a mentioned user" do + it "updates the mention record" do + chat_message = create_chat_message(user1, "ping @#{user2.username}", public_chat_channel) + + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "ping @#{user3.username}", + ) + + user2_mentions = user2.chat_mentions.where(chat_message: chat_message) + expect(user2_mentions.length).to be(0) + + user3_mentions = user3.chat_mentions.where(chat_message: chat_message) + expect(user3_mentions.length).to be(1) + end + end + + context "when there are duplicate mentions" do + it "creates a single mention record per user" do + chat_message = create_chat_message(user1, "ping @#{user2.username}", public_chat_channel) + + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "ping @#{user2.username} @#{user2.username} edited", + ) + + expect(user2.chat_mentions.where(chat_message: chat_message).count).to eq(1) + end + end + + describe "with group mentions" do + it "creates group mentions on update" do + chat_message = create_chat_message(user1, "ping nobody", public_chat_channel) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "ping @#{admin_group.name}", + ) + }.to change { Chat::Mention.where(chat_message: chat_message).count }.by(2) + + expect(admin1.chat_mentions.where(chat_message: chat_message)).to be_present + expect(admin2.chat_mentions.where(chat_message: chat_message)).to be_present + end + + it "doesn't duplicate mentions when the user is already direct mentioned and then group mentioned" do + chat_message = create_chat_message(user1, "ping @#{admin2.username}", public_chat_channel) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "ping @#{admin_group.name} @#{admin2.username}", + ) + }.to change { admin1.chat_mentions.count }.by(1).and not_change { + admin2.chat_mentions.count + } + end + + it "deletes old mentions when group mention is removed" do + chat_message = + create_chat_message(user1, "ping @#{admin_group.name}", public_chat_channel) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "ping nobody anymore!", + ) + }.to change { Chat::Mention.where(chat_message: chat_message).count }.by(-2) + + expect(admin1.chat_mentions.where(chat_message: chat_message)).not_to be_present + expect(admin2.chat_mentions.where(chat_message: chat_message)).not_to be_present + end + end + end + + it "creates a chat_message_revision record and sets last_editor_id for the message" do + SiteSetting.chat_editing_grace_period = 10 + SiteSetting.chat_editing_grace_period_max_diff_low_trust = 5 + + old_message = "It's a thrsday!" + new_message = "Today is Thursday, it's almost the weekend already!" + chat_message = create_chat_message(user1, old_message, public_chat_channel) + updater = + described_class.call(guardian: guardian, message_id: chat_message.id, message: new_message) + + revision = chat_message.revisions.last + expect(revision.old_message).to eq(old_message) + expect(revision.new_message).to eq(new_message) + expect(revision.user_id).to eq(guardian.user.id) + expect(chat_message.reload.last_editor_id).to eq(guardian.user.id) + end + + describe "duplicates" do + fab!(:upload1) { Fabricate(:upload, user: user1) } + fab!(:upload2) { Fabricate(:upload, user: user1) } + + before do + SiteSetting.chat_duplicate_message_sensitivity = 1.0 + public_chat_channel.update!(user_count: 50) + end + + it "errors when editing the message to be the same as one that was posted recently" do + chat_message_1 = + create_chat_message(user1, "this is some chat message", public_chat_channel) + chat_message_2 = + create_chat_message( + Fabricate(:user), + "another different chat message here", + public_chat_channel, + ) + + chat_message_1.update!(created_at: 30.seconds.ago) + chat_message_2.update!(created_at: 20.seconds.ago) + + expect do + described_class.call( + guardian: guardian, + message_id: chat_message_1.id, + message: "another different chat message here", + ) + end.to raise_error(ActiveRecord::RecordInvalid).with_message( + "Validation failed: " + I18n.t("chat.errors.duplicate_message"), + ) + end + + it "does not count the message as a duplicate when editing leaves the message the same but changes uploads" do + chat_message = + create_chat_message( + user1, + "this is some chat message", + public_chat_channel, + upload_ids: [upload1.id, upload2.id], + ) + chat_message.update!(created_at: 30.seconds.ago) + + updater = + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "this is some chat message", + upload_ids: [upload2.id], + ) + expect(updater.message).to be_valid + expect(chat_message.reload.uploads.count).to eq(1) + end + end + + describe "uploads" do + fab!(:upload1) { Fabricate(:upload, user: user1) } + fab!(:upload2) { Fabricate(:upload, user: user1) } + + it "does nothing if the passed in upload_ids match the existing upload_ids" do + chat_message = + create_chat_message( + user1, + "something", + public_chat_channel, + upload_ids: [upload1.id, upload2.id], + ) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "I guess this is different", + upload_ids: [upload2.id, upload1.id], + ) + }.to not_change { UploadReference.count } + end + + it "removes uploads that should be removed" do + chat_message = + create_chat_message( + user1, + "something", + public_chat_channel, + upload_ids: [upload1.id, upload2.id], + ) + + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "I guess this is different", + upload_ids: [upload1.id], + ) + }.to change { UploadReference.where(upload_id: upload2.id).count }.by(-1) + end + + it "removes all uploads if they should be removed" do + chat_message = + create_chat_message( + user1, + "something", + public_chat_channel, + upload_ids: [upload1.id, upload2.id], + ) + + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "I guess this is different", + upload_ids: [], + ) + }.to change { UploadReference.where(target: chat_message).count }.by(-2) + end + + it "adds one upload if none exist" do + chat_message = create_chat_message(user1, "something", public_chat_channel) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "I guess this is different", + upload_ids: [upload1.id], + ) + }.to change { UploadReference.where(target: chat_message).count }.by(1) + end + + it "adds multiple uploads if none exist" do + chat_message = create_chat_message(user1, "something", public_chat_channel) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "I guess this is different", + upload_ids: [upload1.id, upload2.id], + ) + }.to change { UploadReference.where(target: chat_message).count }.by(2) + end + + it "doesn't remove existing uploads when upload ids that do not exist are passed in" do + chat_message = + create_chat_message(user1, "something", public_chat_channel, upload_ids: [upload1.id]) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message, + message: "I guess this is different", + upload_ids: [0], + ) + }.to not_change { UploadReference.where(target: chat_message).count } + end + + it "doesn't add uploads if `chat_allow_uploads` is false" do + SiteSetting.chat_allow_uploads = false + chat_message = create_chat_message(user1, "something", public_chat_channel) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "I guess this is different", + upload_ids: [upload1.id, upload2.id], + ) + }.to not_change { UploadReference.where(target: chat_message).count } + end + + it "doesn't remove existing uploads if `chat_allow_uploads` is false" do + SiteSetting.chat_allow_uploads = false + chat_message = + create_chat_message( + user1, + "something", + public_chat_channel, + upload_ids: [upload1.id, upload2.id], + ) + expect { + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "I guess this is different", + upload_ids: [], + ) + }.to not_change { UploadReference.where(target: chat_message).count } + end + + it "updates if upload is present even if length is less than `chat_minimum_message_length`" do + chat_message = + create_chat_message( + user1, + "something", + public_chat_channel, + upload_ids: [upload1.id, upload2.id], + ) + SiteSetting.chat_minimum_message_length = 10 + new_message = "hi :)" + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: new_message, + upload_ids: [upload1.id], + ) + expect(chat_message.reload.message).to eq(new_message) + end + end + + context "when the message is in a thread" do + fab!(:message) do + Fabricate( + :chat_message, + user: user1, + chat_channel: public_chat_channel, + thread: Fabricate(:chat_thread, channel: public_chat_channel), + ) + end + + it "publishes a MessageBus event to update the original message metadata" do + messages = + MessageBus.track_publish("/chat/#{public_chat_channel.id}") do + described_class.call( + guardian: guardian, + message_id: message.id, + message: "some new updated content", + ) + end + expect( + messages.find { |m| m.data["type"] == "update_thread_original_message" }, + ).to be_present + end + end + + describe "watched words" do + fab!(:watched_word) { Fabricate(:watched_word) } + + it "errors when a blocked word is present" do + chat_message = create_chat_message(user1, "something", public_chat_channel) + msg = "Validation failed: " + I18n.t("contains_blocked_word", { word: watched_word.word }) + + expect do + described_class.call( + guardian: guardian, + message_id: chat_message.id, + message: "bad word - #{watched_word.word}", + ) + end.to raise_error(ActiveRecord::RecordInvalid).with_message(msg) + + expect(chat_message.reload.message).not_to eq("bad word - #{watched_word.word}") + end + end + + describe "channel statuses" do + fab!(:message) { Fabricate(:chat_message, user: user1, chat_channel: public_chat_channel) } + + def update_message(user) + message.update!(user: user) + described_class.call( + guardian: Guardian.new(user), + message_id: message.id, + message: "I guess this is different", + ) + end + + context "when channel is closed" do + before { public_chat_channel.update(status: :closed) } + + it "errors when trying to update the message for non-staff" do + updater = update_message(user1) + expect(message.reload.message).not_to eq("I guess this is different") + end + + it "does not error when trying to create a message for staff" do + update_message(admin1) + expect(message.reload.message).to eq("I guess this is different") + end + end + + context "when channel is read_only" do + before { public_chat_channel.update(status: :read_only) } + + it "errors when trying to update the message for all users" do + updater = update_message(user1) + expect(message.reload.message).not_to eq("I guess this is different") + + updater = update_message(admin1) + expect(message.reload.message).not_to eq("I guess this is different") + end + end + + context "when channel is archived" do + before { public_chat_channel.update(status: :archived) } + + it "errors when trying to update the message for all users" do + updater = update_message(user1) + expect(message.reload.message).not_to eq("I guess this is different") + + updater = update_message(admin1) + expect(message.reload.message).not_to eq("I guess this is different") + end + end + end + end + + describe ".call" do + subject(:result) { described_class.call(params) } + + fab!(:current_user) { Fabricate(:user) } + fab!(:channel_1) { Fabricate(:chat_channel) } + fab!(:upload_1) { Fabricate(:upload, user: current_user) } + fab!(:message_1) do + Fabricate( + :chat_message, + chat_channel_id: channel_1.id, + message: "old", + upload_ids: [upload_1.id], + user: current_user, + ) + end + + let(:guardian) { current_user.guardian } + let(:message) { "new" } + let(:message_id) { message_1.id } + let(:upload_ids) { [upload_1.id] } + let(:params) do + { guardian: guardian, message_id: message_id, message: message, upload_ids: upload_ids } + end + + before do + SiteSetting.chat_editing_grace_period = 10 + SiteSetting.chat_editing_grace_period_max_diff_low_trust = 10 + SiteSetting.chat_editing_grace_period_max_diff_high_trust = 40 + end + + context "when all steps pass" do + it "sets the service result as successful" do + expect(result).to run_service_successfully + end + + it "updates the message" do + expect(result.message.message).to eq("new") + end + + it "updates the uploads" do + upload_1 = Fabricate(:upload, user: current_user) + upload_2 = Fabricate(:upload, user: current_user) + params[:upload_ids] = [upload_1.id, upload_2.id] + + expect(result.message.upload_ids).to contain_exactly(upload_1.id, upload_2.id) + end + + it "keeps the existing uploads" do + expect(result.message.upload_ids).to eq([upload_1.id]) + end + + it "does not update last editor" do + # message can only be updated by the original author + message_1.update!(last_editor: Discourse.system_user) + + expect { result }.to not_change { result.message.last_editor_id } + end + end + + context "when params are not valid" do + before { params.delete(:message_id) } + + it { is_expected.to fail_a_contract } + end + + context "when user can't modify a channel message" do + before { channel_1.update!(status: :read_only) } + + it { is_expected.to fail_a_policy(:can_modify_channel_message) } + end + + context "when user can't modify this message" do + let(:message_id) { Fabricate(:chat_message).id } + + it { is_expected.to fail_a_policy(:can_modify_message) } + end + + context "when edit grace period" do + let(:low_trust_char_limit) { SiteSetting.chat_editing_grace_period_max_diff_low_trust } + let(:high_trust_char_limit) { SiteSetting.chat_editing_grace_period_max_diff_high_trust } + + it "does not create a revision when under (n) seconds" do + freeze_time 5.seconds.from_now + message_1.update!(message: "hello") + + expect { result }.to not_change { Chat::MessageRevision.count } + end + + it "does not create a revision when under (n) chars" do + message_1.update!(message: "hi :)") + + expect { result }.to not_change { Chat::MessageRevision.count } + end + + it "creates a revision when over (n) seconds" do + freeze_time 30.seconds.from_now + message_1.update!(message: "welcome") + + expect { result }.to change { Chat::MessageRevision.count }.by(1) + end + + it "creates a revision when over (n) chars" do + message_1.update!(message: "a" * (low_trust_char_limit + 1)) + + expect { result }.to change { Chat::MessageRevision.count }.by(1) + end + + it "allows trusted users to make larger edits without creating revision" do + current_user.update!(trust_level: TrustLevel[4]) + message_1.update!(message: "a" * (low_trust_char_limit + 1)) + + expect { result }.to not_change { Chat::MessageRevision.count } + end + + it "creates a revision when over (n) chars for high trust users" do + current_user.update!(trust_level: TrustLevel[4]) + + message_1.update!(message: "a" * (high_trust_char_limit + 1)) + expect { result }.to change { Chat::MessageRevision.count }.by(1) + end + end + end +end diff --git a/plugins/chat/spec/support/chat_service_matcher.rb b/plugins/chat/spec/support/chat_service_matcher.rb index e3ca0ac067d..77c0cbd262e 100644 --- a/plugins/chat/spec/support/chat_service_matcher.rb +++ b/plugins/chat/spec/support/chat_service_matcher.rb @@ -2,6 +2,20 @@ module Chat module ServiceMatchers + class RunServiceSuccessfully + attr_reader :result + + def matches?(result) + @result = result + result.success? + end + + def failure_message + inspector = StepsInspector.new(result) + "Expected to run the service sucessfully but failed:\n\n#{inspector.inspect}\n\n#{inspector.error}" + end + end + class FailStep attr_reader :name, :result @@ -124,6 +138,10 @@ module Chat FailStep.new(name) end + def run_service_successfully + RunServiceSuccessfully.new + end + def inspect_steps(result) inspector = Chat::StepsInspector.new(result) puts "Steps:" diff --git a/plugins/chat/spec/system/uploads_spec.rb b/plugins/chat/spec/system/uploads_spec.rb index 3181e96ca1a..102228d3421 100644 --- a/plugins/chat/spec/system/uploads_spec.rb +++ b/plugins/chat/spec/system/uploads_spec.rb @@ -115,9 +115,10 @@ describe "Uploading files in chat messages", type: :system do channel_page.messages.edit(message_2) find(".chat-composer-upload").hover find(".chat-composer-upload__remove-btn").click + expect(channel_page.message_by_id(message_2.id)).to have_no_css(".chat-uploads") + channel_page.click_send_message - expect(channel_page.message_by_id(message_2.id)).not_to have_css(".chat-uploads") - try_until_success(timeout: 5) { expect(message_2.reload.uploads).to be_empty } + try_until_success(timeout: 5) { expect(message_2.reload.upload_ids).to be_empty } end it "allows adding more uploads" do