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 5496aae6e9c..067d4a9213d 100644 --- a/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb +++ b/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb @@ -4,10 +4,4 @@ class Chat::Api::ChannelMessagesController < Chat::ApiController def destroy with_service(Chat::TrashMessage) { on_model_not_found(:message) { raise Discourse::NotFound } } end - - def restore - with_service(Chat::RestoreMessage) do - on_model_not_found(:message) { raise Discourse::NotFound } - end - end end diff --git a/plugins/chat/app/controllers/chat/chat_controller.rb b/plugins/chat/app/controllers/chat/chat_controller.rb index 0a5ec268eb8..db3ae4bcf47 100644 --- a/plugins/chat/app/controllers/chat/chat_controller.rb +++ b/plugins/chat/app/controllers/chat/chat_controller.rb @@ -12,7 +12,8 @@ module Chat # these endpoints require a standalone find because they need to be # able to get deleted channels and recover them. before_action :find_chatable, only: %i[enable_chat disable_chat] - before_action :find_chat_message, only: %i[lookup_message edit_message rebake message_link] + before_action :find_chat_message, + only: %i[delete restore lookup_message edit_message rebake message_link] before_action :set_channel_and_chatable_with_access_check, except: %i[ respond @@ -232,6 +233,18 @@ module Chat render json: success_json end + def restore + chat_channel = @message.chat_channel + guardian.ensure_can_restore_chat!(@message, chat_channel.chatable) + updated = @message.recover! + if updated + Chat::Publisher.publish_restore!(chat_channel, @message) + render json: success_json + else + render_json_error(@message) + end + end + def rebake guardian.ensure_can_rebake_chat_message!(@message) @message.rebake!(invalidate_oneboxes: true) diff --git a/plugins/chat/app/jobs/regular/chat/update_thread_reply_count.rb b/plugins/chat/app/jobs/regular/chat/update_thread_reply_count.rb deleted file mode 100644 index ce362866690..00000000000 --- a/plugins/chat/app/jobs/regular/chat/update_thread_reply_count.rb +++ /dev/null @@ -1,22 +0,0 @@ -# frozen_string_literal: true - -module Jobs - module Chat - class UpdateThreadReplyCount < Jobs::Base - def execute(args = {}) - thread = ::Chat::Thread.find_by(id: args[:thread_id]) - return if thread.blank? - return if thread.replies_count_cache_recently_updated? - - Discourse.redis.setex( - ::Chat::Thread.replies_count_cache_updated_at_redis_key(thread.id), - 5.minutes.from_now.to_i, - Time.zone.now.to_i, - ) - thread.set_replies_count_cache(thread.replies.count, update_db: true) - - ::Chat::Publisher.publish_thread_original_message_metadata!(thread) - end - end - end -end diff --git a/plugins/chat/app/models/chat/thread.rb b/plugins/chat/app/models/chat/thread.rb index f98f15d5748..6b1eafedaea 100644 --- a/plugins/chat/app/models/chat/thread.rb +++ b/plugins/chat/app/models/chat/thread.rb @@ -4,8 +4,6 @@ module Chat class Thread < ActiveRecord::Base EXCERPT_LENGTH = 150 - include Chat::ThreadCache - self.table_name = "chat_threads" belongs_to :channel, foreign_key: "channel_id", class_name: "Chat::Channel" @@ -13,11 +11,7 @@ module Chat belongs_to :original_message, foreign_key: "original_message_id", class_name: "Chat::Message" has_many :chat_messages, - -> { - where("deleted_at IS NULL").order( - "chat_messages.created_at ASC, chat_messages.id ASC", - ) - }, + -> { order("chat_messages.created_at ASC, chat_messages.id ASC") }, foreign_key: :thread_id, primary_key: :id, class_name: "Chat::Message" @@ -67,7 +61,7 @@ module Chat # # It is updated eventually via Jobs::Chat::PeriodicalUpdates. In # future we may want to update this more frequently. - updated_thread_ids = DB.query_single <<~SQL + DB.exec <<~SQL UPDATE chat_threads threads SET replies_count = subquery.replies_count FROM ( @@ -78,9 +72,7 @@ module Chat ) subquery WHERE threads.id = subquery.thread_id AND subquery.replies_count != threads.replies_count - RETURNING threads.id AS thread_id; SQL - self.clear_caches!(updated_thread_ids) end end end diff --git a/plugins/chat/app/models/concerns/chat/thread_cache.rb b/plugins/chat/app/models/concerns/chat/thread_cache.rb deleted file mode 100644 index 4a67ea39daa..00000000000 --- a/plugins/chat/app/models/concerns/chat/thread_cache.rb +++ /dev/null @@ -1,69 +0,0 @@ -# frozen_string_literal: true - -module Chat - module ThreadCache - extend ActiveSupport::Concern - - class_methods do - def replies_count_cache_updated_at_redis_key(id) - "chat_thread:replies_count_cache_updated_at:#{id}" - end - - def replies_count_cache_redis_key(id) - "chat_thread:replies_count_cache:#{id}" - end - - def clear_caches!(ids = nil) - return Discourse.redis.delete_prefixed("chat_thread:") if ids.blank? - ids = Array.wrap(ids) - - keys_to_delete = - ids - .map do |id| - [replies_count_cache_redis_key(id), replies_count_cache_updated_at_redis_key(id)] - end - .flatten - Discourse.redis.del(keys_to_delete) - end - end - - def replies_count_cache_recently_updated? - replies_count_cache_updated_at.after?(5.minutes.ago) - end - - def replies_count_cache_updated_at - Time.at( - Discourse.redis.get(Chat::Thread.replies_count_cache_updated_at_redis_key(self.id)).to_i, - in: Time.zone, - ) - end - - def replies_count_cache - redis_cache = Discourse.redis.get(Chat::Thread.replies_count_cache_redis_key(self.id))&.to_i - if redis_cache.present? && redis_cache != self.replies_count - redis_cache - else - self.replies_count - end - end - - def set_replies_count_cache(value, update_db: false) - self.update!(replies_count: value) if update_db - Discourse.redis.setex( - Chat::Thread.replies_count_cache_redis_key(self.id), - 5.minutes.from_now.to_i, - value, - ) - Jobs.enqueue_in(5.seconds, Jobs::Chat::UpdateThreadReplyCount, thread_id: self.id) - ::Chat::Publisher.publish_thread_original_message_metadata!(self) - end - - def increment_replies_count_cache - self.set_replies_count_cache(self.replies_count_cache + 1) - end - - def decrement_replies_count_cache - self.set_replies_count_cache(self.replies_count_cache - 1) - end - end -end diff --git a/plugins/chat/app/serializers/chat/message_serializer.rb b/plugins/chat/app/serializers/chat/message_serializer.rb index 6c91a6f3f61..58d32b85edb 100644 --- a/plugins/chat/app/serializers/chat/message_serializer.rb +++ b/plugins/chat/app/serializers/chat/message_serializer.rb @@ -166,7 +166,7 @@ module Chat end def thread_reply_count - object.thread&.replies_count_cache || 0 + object.thread&.replies_count || 0 end end end diff --git a/plugins/chat/app/services/chat/publisher.rb b/plugins/chat/app/services/chat/publisher.rb index a14bc288dcd..6d63f395b73 100644 --- a/plugins/chat/app/services/chat/publisher.rb +++ b/plugins/chat/app/services/chat/publisher.rb @@ -3,7 +3,7 @@ module Chat module Publisher def self.new_messages_message_bus_channel(chat_channel_id) - "#{root_message_bus_channel(chat_channel_id)}/new-messages" + "/chat/#{chat_channel_id}/new-messages" end def self.root_message_bus_channel(chat_channel_id) @@ -35,11 +35,31 @@ module Chat def self.publish_new!(chat_channel, chat_message, staged_id) message_bus_targets = calculate_publish_targets(chat_channel, chat_message) - publish_to_targets!( - message_bus_targets, - chat_channel, - serialize_message_with_type(chat_message, :sent).merge(staged_id: staged_id), - ) + + content = + Chat::MessageSerializer.new( + chat_message, + { scope: anonymous_guardian, root: :chat_message }, + ).as_json + content[:type] = :sent + content[:staged_id] = staged_id + permissions = permissions(chat_channel) + + message_bus_targets.each do |message_bus_channel| + MessageBus.publish(message_bus_channel, content.as_json, permissions) + end + + if chat_message.thread_reply? + MessageBus.publish( + root_message_bus_channel(chat_channel.id), + { + type: :update_thread_original_message, + original_message_id: chat_message.thread.original_message_id, + action: :increment_reply_count, + }.as_json, + permissions, + ) + end # NOTE: This means that the read count is only updated in the client # for new messages in the main channel stream, maybe in future we want to @@ -54,67 +74,84 @@ module Chat username: chat_message.user.username, thread_id: chat_message.thread_id, }, - permissions(chat_channel), + permissions, ) end end - def self.publish_thread_original_message_metadata!(thread) - publish_to_channel!( - thread.channel, - { - type: :update_thread_original_message, - original_message_id: thread.original_message_id, - replies_count: thread.replies_count_cache, - }, - ) - end - def self.publish_thread_created!(chat_channel, chat_message) - publish_to_channel!(chat_channel, serialize_message_with_type(chat_message, :thread_created)) + content = + Chat::MessageSerializer.new( + chat_message, + { scope: anonymous_guardian, root: :chat_message }, + ).as_json + content[:type] = :thread_created + permissions = permissions(chat_channel) + + MessageBus.publish(root_message_bus_channel(chat_channel.id), content.as_json, permissions) end def self.publish_processed!(chat_message) chat_channel = chat_message.chat_channel message_bus_targets = calculate_publish_targets(chat_channel, chat_message) - publish_to_targets!( - message_bus_targets, - chat_channel, - { type: :processed, chat_message: { id: chat_message.id, cooked: chat_message.cooked } }, - ) + + content = { + type: :processed, + chat_message: { + id: chat_message.id, + cooked: chat_message.cooked, + }, + } + + message_bus_targets.each do |message_bus_channel| + MessageBus.publish(message_bus_channel, content.as_json, permissions(chat_channel)) + end end def self.publish_edit!(chat_channel, chat_message) message_bus_targets = calculate_publish_targets(chat_channel, chat_message) - publish_to_targets!( - message_bus_targets, - chat_channel, - serialize_message_with_type(chat_message, :edit), - ) + + content = + Chat::MessageSerializer.new( + chat_message, + { scope: anonymous_guardian, root: :chat_message }, + ).as_json + content[:type] = :edit + + message_bus_targets.each do |message_bus_channel| + MessageBus.publish(message_bus_channel, content.as_json, permissions(chat_channel)) + end end def self.publish_refresh!(chat_channel, chat_message) message_bus_targets = calculate_publish_targets(chat_channel, chat_message) - publish_to_targets!( - message_bus_targets, - chat_channel, - serialize_message_with_type(chat_message, :refresh), - ) + + content = + Chat::MessageSerializer.new( + chat_message, + { scope: anonymous_guardian, root: :chat_message }, + ).as_json + content[:type] = :refresh + + message_bus_targets.each do |message_bus_channel| + MessageBus.publish(message_bus_channel, content.as_json, permissions(chat_channel)) + end end def self.publish_reaction!(chat_channel, chat_message, action, user, emoji) message_bus_targets = calculate_publish_targets(chat_channel, chat_message) - publish_to_targets!( - message_bus_targets, - chat_channel, - { - action: action, - user: BasicUserSerializer.new(user, root: false).as_json, - emoji: emoji, - type: :reaction, - chat_message_id: chat_message.id, - }, - ) + + content = { + action: action, + user: BasicUserSerializer.new(user, root: false).as_json, + emoji: emoji, + type: :reaction, + chat_message_id: chat_message.id, + } + + message_bus_targets.each do |message_bus_channel| + MessageBus.publish(message_bus_channel, content.as_json, permissions(chat_channel)) + end end def self.publish_presence!(chat_channel, user, typ) @@ -123,26 +160,28 @@ module Chat def self.publish_delete!(chat_channel, chat_message) message_bus_targets = calculate_publish_targets(chat_channel, chat_message) - publish_to_targets!( - message_bus_targets, - chat_channel, - { type: "delete", deleted_id: chat_message.id, deleted_at: chat_message.deleted_at }, - ) + + message_bus_targets.each do |message_bus_channel| + MessageBus.publish( + message_bus_channel, + { type: "delete", deleted_id: chat_message.id, deleted_at: chat_message.deleted_at }, + permissions(chat_channel), + ) + end end def self.publish_bulk_delete!(chat_channel, deleted_message_ids) - channel_permissions = permissions(chat_channel) Chat::Thread .grouped_messages(message_ids: deleted_message_ids) .each do |group| MessageBus.publish( thread_message_bus_channel(chat_channel.id, group.thread_id), { - type: :bulk_delete, + type: "bulk_delete", deleted_ids: group.thread_message_ids, deleted_at: Time.zone.now, }, - channel_permissions, + permissions(chat_channel), ) # Don't need to publish to the main channel if the messages deleted @@ -154,72 +193,52 @@ module Chat return if deleted_message_ids.empty? - publish_to_channel!( - chat_channel, - { type: :bulk_delete, deleted_ids: deleted_message_ids, deleted_at: Time.zone.now }, + MessageBus.publish( + root_message_bus_channel(chat_channel.id), + { type: "bulk_delete", deleted_ids: deleted_message_ids, deleted_at: Time.zone.now }, + permissions(chat_channel), ) end def self.publish_restore!(chat_channel, chat_message) message_bus_targets = calculate_publish_targets(chat_channel, chat_message) - publish_to_targets!( - message_bus_targets, - chat_channel, - serialize_message_with_type(chat_message, :restore), - ) + + content = + Chat::MessageSerializer.new( + chat_message, + { scope: anonymous_guardian, root: :chat_message }, + ).as_json + content[:type] = :restore + + message_bus_targets.each do |message_bus_channel| + MessageBus.publish(message_bus_channel, content.as_json, permissions(chat_channel)) + end end def self.publish_flag!(chat_message, user, reviewable, score) message_bus_targets = calculate_publish_targets(chat_message.chat_channel, chat_message) - # Publish to user who created flag - publish_to_targets!( - message_bus_targets, - chat_message.chat_channel, - { - type: :self_flagged, - user_flag_status: score.status_for_database, - chat_message_id: chat_message.id, - }, - permissions: { - user_ids: [user.id], - }, - ) - - # Publish flag with link to reviewable to staff - publish_to_targets!( - message_bus_targets, - chat_message.chat_channel, - { type: :flag, chat_message_id: chat_message.id, reviewable_id: reviewable.id }, - permissions: { - group_ids: [Group::AUTO_GROUPS[:staff]], - }, - ) - end - - def self.publish_to_channel!(channel, payload) - MessageBus.publish( - root_message_bus_channel(channel.id), - payload.as_json, - permissions(channel), - ) - end - - def self.publish_to_targets!(targets, channel, payload, permissions: nil) - targets.each do |message_bus_channel| + message_bus_targets.each do |message_bus_channel| + # Publish to user who created flag MessageBus.publish( message_bus_channel, - payload.as_json, - permissions || permissions(channel), + { + type: "self_flagged", + user_flag_status: score.status_for_database, + chat_message_id: chat_message.id, + }.as_json, + user_ids: [user.id], ) end - end - def self.serialize_message_with_type(chat_message, type) - Chat::MessageSerializer - .new(chat_message, { scope: anonymous_guardian, root: :chat_message }) - .as_json - .merge(type: type) + message_bus_targets.each do |message_bus_channel| + # Publish flag with link to reviewable to staff + MessageBus.publish( + message_bus_channel, + { type: "flag", chat_message_id: chat_message.id, reviewable_id: reviewable.id }.as_json, + group_ids: [Group::AUTO_GROUPS[:staff]], + ) + end end def self.user_tracking_state_message_bus_channel(user_id) diff --git a/plugins/chat/app/services/chat/restore_message.rb b/plugins/chat/app/services/chat/restore_message.rb deleted file mode 100644 index fa1a6b173b8..00000000000 --- a/plugins/chat/app/services/chat/restore_message.rb +++ /dev/null @@ -1,63 +0,0 @@ -# frozen_string_literal: true - -module Chat - # Service responsible for restoring a trashed chat message - # for a channel and ensuring that the client and read state is - # updated. - # - # @example - # Chat::RestoreMessage.call(message_id: 2, channel_id: 1, guardian: guardian) - # - class RestoreMessage - include Service::Base - - # @!method call(message_id:, channel_id:, guardian:) - # @param [Integer] message_id - # @param [Integer] channel_id - # @param [Guardian] guardian - # @return [Service::Base::Context] - - contract - model :message - policy :invalid_access - transaction do - step :restore_message - step :update_thread_reply_cache - end - step :publish_events - - # @!visibility private - class Contract - attribute :message_id, :integer - attribute :channel_id, :integer - validates :message_id, presence: true - validates :channel_id, presence: true - end - - private - - def fetch_message(contract:, **) - Chat::Message - .with_deleted - .includes(chat_channel: :chatable) - .find_by(id: contract.message_id, chat_channel_id: contract.channel_id) - end - - def invalid_access(guardian:, message:, **) - guardian.can_restore_chat?(message, message.chat_channel.chatable) - end - - def restore_message(message:, **) - message.recover! - end - - def update_thread_reply_cache(message:, **) - message.thread&.increment_replies_count_cache - end - - def publish_events(guardian:, message:, **) - DiscourseEvent.trigger(:chat_message_restored, message, message.chat_channel, guardian.user) - Chat::Publisher.publish_restore!(message.chat_channel, message) - end - end -end diff --git a/plugins/chat/app/services/chat/trash_message.rb b/plugins/chat/app/services/chat/trash_message.rb index 71bf33a7b63..2139a8a0f77 100644 --- a/plugins/chat/app/services/chat/trash_message.rb +++ b/plugins/chat/app/services/chat/trash_message.rb @@ -24,7 +24,6 @@ module Chat step :trash_message step :destroy_mentions step :update_tracking_state - step :update_thread_reply_cache end step :publish_events @@ -63,10 +62,6 @@ module Chat ) end - def update_thread_reply_cache(message:, **) - message.thread&.decrement_replies_count_cache - end - def publish_events(guardian:, message:, **) DiscourseEvent.trigger(:chat_message_trashed, message, message.chat_channel, guardian.user) Chat::Publisher.publish_delete!(message.chat_channel, message) diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-api.js b/plugins/chat/assets/javascripts/discourse/services/chat-api.js index 7be02b5ed3c..f459a5faab5 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-api.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-api.js @@ -350,9 +350,10 @@ export default class ChatApi extends Service { * @param {number} messageId - The ID of the message being restored. */ restoreMessage(channelId, messageId) { - return this.#putRequest( - `/channels/${channelId}/messages/${messageId}/restore` - ); + // TODO (martin) Not ideal, this should have a chat API controller endpoint. + return ajax(`/chat/${channelId}/restore/${messageId}`, { + type: "PUT", + }); } /** diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js index 93a218eb2af..e8d1080baa5 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js @@ -37,8 +37,11 @@ export default class ChatChannelPaneSubscriptionsManager extends ChatPaneBaseSub handleThreadOriginalMessageUpdate(data) { const message = this.messagesManager.findMessage(data.original_message_id); if (message) { - if (data.replies_count) { - message.threadReplyCount = data.replies_count; + if (data.action === "increment_reply_count") { + // TODO (martin) In future we should use a replies_count delivered + // from the server and simply update the message accordingly, for + // now we don't have an accurate enough count for this. + message.threadReplyCount += 1; } } } diff --git a/plugins/chat/config/routes.rb b/plugins/chat/config/routes.rb index 53d1a723fb6..4fed17209d4 100644 --- a/plugins/chat/config/routes.rb +++ b/plugins/chat/config/routes.rb @@ -30,7 +30,6 @@ Chat::Engine.routes.draw do get "/channels/:channel_id/threads/:thread_id" => "channel_threads#show" - put "/channels/:channel_id/messages/:message_id/restore" => "channel_messages#restore" delete "/channels/:channel_id/messages/:message_id" => "channel_messages#destroy" end @@ -62,6 +61,7 @@ Chat::Engine.routes.draw do put "/:chat_channel_id/:message_id/rebake" => "chat#rebake" post "/:chat_channel_id/:message_id/flag" => "chat#flag" post "/:chat_channel_id/quote" => "chat#quote_messages" + put "/:chat_channel_id/restore/:message_id" => "chat#restore" get "/lookup/:message_id" => "chat#lookup_message" put "/:chat_channel_id/read/:message_id" => "chat#update_user_last_read" put "/user_chat_enabled/:user_id" => "chat#set_user_chat_status" diff --git a/plugins/chat/lib/chat/message_creator.rb b/plugins/chat/lib/chat/message_creator.rb index 9a608bd51e4..d5fc84a941c 100644 --- a/plugins/chat/lib/chat/message_creator.rb +++ b/plugins/chat/lib/chat/message_creator.rb @@ -58,7 +58,6 @@ module Chat @chat_message.attach_uploads(uploads) Chat::Draft.where(user_id: @user.id, chat_channel_id: @chat_channel.id).destroy_all Chat::Publisher.publish_new!(@chat_channel, @chat_message, @staged_id) - resolved_thread&.increment_replies_count_cache Jobs.enqueue(Jobs::Chat::ProcessMessage, { chat_message_id: @chat_message.id }) Chat::Notifier.notify_new(chat_message: @chat_message, timestamp: @chat_message.created_at) @chat_channel.touch(:last_message_sent_at) @@ -207,9 +206,5 @@ module Chat WHERE thread_id IS NULL AND chat_messages.id = thread_updater.id SQL end - - def resolved_thread - @existing_thread || @chat_message.thread - end end end diff --git a/plugins/chat/spec/integration/thread_replies_count_cache_accuracy_spec.rb b/plugins/chat/spec/integration/thread_replies_count_cache_accuracy_spec.rb deleted file mode 100644 index 693c3c5c7c3..00000000000 --- a/plugins/chat/spec/integration/thread_replies_count_cache_accuracy_spec.rb +++ /dev/null @@ -1,63 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe "Chat::Thread replies_count cache accuracy" do - include ActiveSupport::Testing::TimeHelpers - - fab!(:user) { Fabricate(:user) } - fab!(:thread) { Fabricate(:chat_thread) } - - it "keeps an accurate replies_count cache" do - freeze_time - Jobs.run_immediately! - - expect(thread.replies_count).to eq(0) - expect(thread.replies_count_cache).to eq(0) - - # Create 5 replies - 5.times do |i| - Chat::MessageCreator.create( - chat_channel: thread.channel, - user: user, - thread_id: thread.id, - content: "Hello world #{i}", - ) - end - - # The job only runs to completion if the cache has not been recently - # updated, so the DB count will only be 1. - expect(thread.replies_count_cache).to eq(5) - expect(thread.reload.replies_count).to eq(1) - - # Travel to the future so the cache expires. - travel_to 6.minutes.from_now - Chat::MessageCreator.create( - chat_channel: thread.channel, - user: user, - thread_id: thread.id, - content: "Hello world now that time has passed", - ) - expect(thread.replies_count_cache).to eq(6) - expect(thread.reload.replies_count).to eq(6) - - # Lose the cache intentionally. - Chat::Thread.clear_caches! - message_to_destroy = thread.replies.last - Chat::TrashMessage.call( - message_id: message_to_destroy.id, - channel_id: thread.channel_id, - guardian: Guardian.new(user), - ) - expect(thread.replies_count_cache).to eq(5) - expect(thread.reload.replies_count).to eq(5) - - # Lose the cache intentionally. - Chat::Thread.clear_caches! - Chat::RestoreMessage.call( - message_id: message_to_destroy.id, - channel_id: thread.channel_id, - guardian: Guardian.new(user), - ) - expect(thread.replies_count_cache).to eq(6) - expect(thread.reload.replies_count).to eq(6) - end -end diff --git a/plugins/chat/spec/jobs/regular/update_thread_reply_count_spec.rb b/plugins/chat/spec/jobs/regular/update_thread_reply_count_spec.rb deleted file mode 100644 index 2d5d7443118..00000000000 --- a/plugins/chat/spec/jobs/regular/update_thread_reply_count_spec.rb +++ /dev/null @@ -1,46 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe Jobs::Chat::UpdateThreadReplyCount do - fab!(:thread) { Fabricate(:chat_thread) } - fab!(:message_1) { Fabricate(:chat_message, thread: thread) } - fab!(:message_2) { Fabricate(:chat_message, thread: thread) } - - before { Chat::Thread.clear_caches! } - - it "does not error if the thread is deleted" do - id = thread.id - thread.destroy! - expect { described_class.new.execute(thread_id: id) }.not_to raise_error - end - - it "does not set the reply count in the DB if it has been changed recently" do - described_class.new.execute(thread_id: thread.id) - expect(thread.reload.replies_count).to eq(2) - Fabricate(:chat_message, thread: thread) - described_class.new.execute(thread_id: thread.id) - expect(thread.reload.replies_count).to eq(2) - end - - it "sets the updated_at cache to the current time" do - freeze_time - described_class.new.execute(thread_id: thread.id) - expect(thread.replies_count_cache_updated_at).to eq_time( - Time.at(Time.zone.now.to_i, in: Time.zone), - ) - end - - it "publishes the thread original message metadata" do - messages = - MessageBus.track_publish("/chat/#{thread.channel_id}") do - described_class.new.execute(thread_id: thread.id) - end - - expect(messages.first.data).to eq( - { - "original_message_id" => thread.original_message_id, - "replies_count" => 2, - "type" => "update_thread_original_message", - }, - ) - end -end diff --git a/plugins/chat/spec/lib/chat/message_mover_spec.rb b/plugins/chat/spec/lib/chat/message_mover_spec.rb index aef48ddb241..c694b246d9d 100644 --- a/plugins/chat/spec/lib/chat/message_mover_spec.rb +++ b/plugins/chat/spec/lib/chat/message_mover_spec.rb @@ -76,9 +76,9 @@ describe Chat::MessageMover do deleted_messages = Chat::Message.with_deleted.where(id: move_message_ids).order(:id) expect(deleted_messages.count).to eq(3) expect(messages.first.channel).to eq("/chat/#{source_channel.id}") - expect(messages.first.data["type"]).to eq("bulk_delete") - expect(messages.first.data["deleted_ids"]).to eq(deleted_messages.map(&:id)) - expect(messages.first.data["deleted_at"]).not_to eq(nil) + expect(messages.first.data[:type]).to eq("bulk_delete") + expect(messages.first.data[:deleted_ids]).to eq(deleted_messages.map(&:id)) + expect(messages.first.data[:deleted_at]).not_to eq(nil) end it "creates a message in the source channel to indicate that the messages have been moved" do diff --git a/plugins/chat/spec/lib/chat/review_queue_spec.rb b/plugins/chat/spec/lib/chat/review_queue_spec.rb index beed58049cf..672b7dff64d 100644 --- a/plugins/chat/spec/lib/chat/review_queue_spec.rb +++ b/plugins/chat/spec/lib/chat/review_queue_spec.rb @@ -280,9 +280,9 @@ describe Chat::ReviewQueue do end .map(&:data) - delete_msg = messages.detect { |m| m["type"] == "delete" } + delete_msg = messages.detect { |m| m[:type] == "delete" } - expect(delete_msg["deleted_id"]).to eq(message.id) + expect(delete_msg[:deleted_id]).to eq(message.id) end it "agrees with other flags on the same message" do diff --git a/plugins/chat/spec/models/chat/thread_spec.rb b/plugins/chat/spec/models/chat/thread_spec.rb index fa458eb71e0..f48a4f04f94 100644 --- a/plugins/chat/spec/models/chat/thread_spec.rb +++ b/plugins/chat/spec/models/chat/thread_spec.rb @@ -40,20 +40,6 @@ RSpec.describe Chat::Thread do described_class.ensure_consistency! expect(thread_1.reload.replies_count).to eq(0) end - - it "clears the affected replies_count caches" do - thread_1.set_replies_count_cache(100) - expect(thread_1.replies_count_cache).to eq(100) - expect(thread_1.replies_count_cache_updated_at).not_to eq(nil) - - described_class.ensure_consistency! - expect(Discourse.redis.get(Chat::Thread.replies_count_cache_redis_key(thread_1.id))).to eq( - nil, - ) - expect( - Discourse.redis.get(Chat::Thread.replies_count_cache_updated_at_redis_key(thread_1.id)), - ).to eq(nil) - end end end diff --git a/plugins/chat/spec/requests/chat/api/messages_controller_spec.rb b/plugins/chat/spec/requests/chat/api/messages_controller_spec.rb index 749c760f631..c29c25ff596 100644 --- a/plugins/chat/spec/requests/chat/api/messages_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/messages_controller_spec.rb @@ -106,95 +106,4 @@ RSpec.describe Chat::Api::ChannelMessagesController do end end end - - describe "#restore" do - RSpec.shared_examples "chat_message_restoration" do - it "doesn't allow a user to restore another user's message" do - sign_in(other_user) - - put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json" - expect(response.status).to eq(403) - end - - it "allows a user to restore their own messages" do - sign_in(current_user) - - put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json" - expect(response.status).to eq(200) - expect(deleted_message.reload.deleted_at).to be_nil - end - - it "allows admin to restore others' messages" do - sign_in(admin) - - put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json" - expect(response.status).to eq(200) - expect(deleted_message.reload.deleted_at).to be_nil - end - - it "does not allow message restore when channel is read_only" do - sign_in(current_user) - - chat_channel.update!(status: :read_only) - - put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json" - expect(response.status).to eq(403) - expect(deleted_message.reload.deleted_at).not_to be_nil - - sign_in(admin) - put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json" - expect(response.status).to eq(403) - end - - it "only allows admin to restore when channel is closed" do - sign_in(admin) - - chat_channel.update!(status: :read_only) - - put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json" - expect(response.status).to eq(403) - expect(deleted_message.reload.deleted_at).not_to be_nil - - chat_channel.update!(status: :closed) - put "/chat/api/channels/#{chat_channel.id}/messages/#{deleted_message.id}/restore.json" - expect(response.status).to eq(200) - expect(deleted_message.reload.deleted_at).to be_nil - end - end - - fab!(:admin) { Fabricate(:admin) } - fab!(:second_user) { Fabricate(:user) } - - before do - message = - Chat::Message.create( - user: current_user, - message: "this is a message", - chat_channel: chat_channel, - ) - message.trash! - end - - let(:deleted_message) do - Chat::Message.unscoped.where(user: current_user, chat_channel: chat_channel).last - end - - describe "for category" do - fab!(:category) { Fabricate(:category) } - fab!(:chat_channel) { Fabricate(:category_channel, chatable: category) } - - it_behaves_like "chat_message_restoration" do - let(:other_user) { second_user } - end - end - - describe "for dm channel" do - fab!(:user_2) { Fabricate(:user) } - fab!(:chat_channel) { Fabricate(:direct_message_channel, users: [current_user, user_2]) } - - it_behaves_like "chat_message_restoration" do - let(:other_user) { user_2 } - end - end - end end diff --git a/plugins/chat/spec/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb index 573474f8a5d..abaa63a34ce 100644 --- a/plugins/chat/spec/requests/chat_controller_spec.rb +++ b/plugins/chat/spec/requests/chat_controller_spec.rb @@ -612,6 +612,82 @@ RSpec.describe Chat::ChatController do end end + RSpec.shared_examples "chat_message_restoration" do + it "doesn't allow a user to restore another user's message" do + sign_in(other_user) + + put "/chat/#{chat_channel.id}/restore/#{Chat::Message.unscoped.last.id}.json" + expect(response.status).to eq(403) + end + + it "allows a user to restore their own posts" do + sign_in(user) + + deleted_message = Chat::Message.unscoped.last + put "/chat/#{chat_channel.id}/restore/#{deleted_message.id}.json" + expect(response.status).to eq(200) + expect(deleted_message.reload.deleted_at).to be_nil + end + + it "allows admin to restore others' posts" do + sign_in(admin) + + deleted_message = Chat::Message.unscoped.last + put "/chat/#{chat_channel.id}/restore/#{deleted_message.id}.json" + expect(response.status).to eq(200) + expect(deleted_message.reload.deleted_at).to be_nil + end + + it "does not allow message restore when chat channel is read_only" do + sign_in(Chat::Message.last.user) + + chat_channel.update!(status: :read_only) + + deleted_message = Chat::Message.unscoped.last + put "/chat/#{chat_channel.id}/restore/#{deleted_message.id}.json" + expect(response.status).to eq(403) + expect(deleted_message.reload.deleted_at).not_to be_nil + + sign_in(admin) + put "/chat/#{chat_channel.id}/restore/#{deleted_message.id}.json" + expect(response.status).to eq(403) + end + + it "only allows admin to restore when chat channel is closed" do + sign_in(admin) + + chat_channel.update!(status: :read_only) + + deleted_message = Chat::Message.unscoped.last + put "/chat/#{chat_channel.id}/restore/#{deleted_message.id}.json" + expect(response.status).to eq(403) + expect(deleted_message.reload.deleted_at).not_to be_nil + + chat_channel.update!(status: :closed) + put "/chat/#{chat_channel.id}/restore/#{deleted_message.id}.json" + expect(response.status).to eq(200) + expect(deleted_message.reload.deleted_at).to be_nil + end + end + + describe "#restore" do + fab!(:second_user) { Fabricate(:user) } + + before do + message = + Chat::Message.create(user: user, message: "this is a message", chat_channel: chat_channel) + message.trash! + end + + describe "for category" do + fab!(:chat_channel) { Fabricate(:category_channel, chatable: category) } + + it_behaves_like "chat_message_restoration" do + let(:other_user) { second_user } + end + end + end + describe "react" do fab!(:chat_channel) { Fabricate(:category_channel) } fab!(:chat_message) { Fabricate(:chat_message, chat_channel: chat_channel, user: user) } diff --git a/plugins/chat/spec/services/chat/restore_message_spec.rb b/plugins/chat/spec/services/chat/restore_message_spec.rb deleted file mode 100644 index 80a1c077abd..00000000000 --- a/plugins/chat/spec/services/chat/restore_message_spec.rb +++ /dev/null @@ -1,74 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe Chat::RestoreMessage do - fab!(:current_user) { Fabricate(:user) } - let!(:guardian) { Guardian.new(current_user) } - fab!(:message) { Fabricate(:chat_message, user: current_user) } - - before { message.trash! } - - describe ".call" do - subject(:result) { described_class.call(params) } - - context "when params are not valid" do - let(:params) { { guardian: guardian } } - - it { is_expected.to fail_a_contract } - end - - context "when params are valid" do - let(:params) do - { guardian: guardian, message_id: message.id, channel_id: message.chat_channel_id } - end - - context "when the user does not have permission to restore" do - before { message.update!(user: Fabricate(:admin)) } - - it { is_expected.to fail_a_policy(:invalid_access) } - end - - context "when the channel does not match the message" do - let(:params) do - { guardian: guardian, message_id: message.id, channel_id: Fabricate(:chat_channel).id } - end - - it { is_expected.to fail_to_find_a_model(:message) } - end - - context "when the user has permission to restore" do - it "sets the service result as successful" do - expect(result).to be_a_success - end - - it "restores the message" do - result - expect(Chat::Message.find_by(id: message.id)).not_to be_nil - end - - it "publishes associated Discourse and MessageBus events" do - freeze_time - messages = nil - event = - DiscourseEvent.track_events { messages = MessageBus.track_publish { result } }.first - expect(event[:event_name]).to eq(:chat_message_restored) - expect(event[:params]).to eq([message, message.chat_channel, current_user]) - expect( - messages.find { |m| m.channel == "/chat/#{message.chat_channel_id}" }.data, - ).to include({ "type" => "restore" }) - end - - context "when the message has a thread" do - fab!(:thread) { Fabricate(:chat_thread, channel: message.chat_channel) } - - before { message.update!(thread: thread) } - - it "increments the thread reply count" do - thread.set_replies_count_cache(1) - result - expect(thread.replies_count_cache).to eq(2) - end - end - end - end - end -end diff --git a/plugins/chat/spec/services/chat/trash_message_spec.rb b/plugins/chat/spec/services/chat/trash_message_spec.rb index b8d045b600b..68902d36843 100644 --- a/plugins/chat/spec/services/chat/trash_message_spec.rb +++ b/plugins/chat/spec/services/chat/trash_message_spec.rb @@ -57,11 +57,7 @@ RSpec.describe Chat::TrashMessage do expect(event[:event_name]).to eq(:chat_message_trashed) expect(event[:params]).to eq([message, message.chat_channel, current_user]) expect(messages.find { |m| m.channel == "/chat/#{message.chat_channel_id}" }.data).to eq( - { - "type" => "delete", - "deleted_id" => message.id, - "deleted_at" => message.reload.deleted_at.iso8601(3), - }, + { type: "delete", deleted_id: message.id, deleted_at: Time.zone.now }, ) end @@ -90,18 +86,6 @@ RSpec.describe Chat::TrashMessage do expect(membership_3.reload.last_read_message_id).not_to be_nil end - context "when the message has a thread" do - fab!(:thread) { Fabricate(:chat_thread, channel: message.chat_channel) } - - before { message.update!(thread: thread) } - - it "decrements the thread reply count" do - thread.set_replies_count_cache(5) - result - expect(thread.replies_count_cache).to eq(4) - end - end - context "when message is already deleted" do before { message.trash! }