github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2024-11-22 09:12:45 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

Revert "FEATURE: Better thread reply counter cache (#21108)" (#21192)

Browse Source 
This reverts commit 180e3e11d1.

Per internal discussions, this is a temporary revert, to investigate if this is causing a performance regression.
This commit is contained in:
Daniel Waterworth 2023-04-20 15:09:47 -05:00 committed by GitHub
parent a4d51810e2
commit 38cebd3ed5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
22 changed files with 232 additions and 602 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
plugins/chat/app/controllers/chat/api/channel_messages_controller.rb
View File

@ -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

15
plugins/chat/app/controllers/chat/chat_controller.rb
View File

@ -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)

22
plugins/chat/app/jobs/regular/chat/update_thread_reply_count.rb
View File

@ -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

12
plugins/chat/app/models/chat/thread.rb
View File

@ -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

69
plugins/chat/app/models/concerns/chat/thread_cache.rb
View File

@ -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

2
plugins/chat/app/serializers/chat/message_serializer.rb
View File

@ -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

227
plugins/chat/app/services/chat/publisher.rb
View File

@ -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)

63
plugins/chat/app/services/chat/restore_message.rb
View File

@ -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

5
plugins/chat/app/services/chat/trash_message.rb
View File

@ -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)

7
plugins/chat/assets/javascripts/discourse/services/chat-api.js
View File

@ -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",
});
}
/**

7
plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js
View File

@ -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;
}
}
}

2
plugins/chat/config/routes.rb
View File

@ -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"

5
plugins/chat/lib/chat/message_creator.rb
View File

@ -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

63
plugins/chat/spec/integration/thread_replies_count_cache_accuracy_spec.rb
View File

@ -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

46
plugins/chat/spec/jobs/regular/update_thread_reply_count_spec.rb
View File

@ -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

6
plugins/chat/spec/lib/chat/message_mover_spec.rb
View File

@ -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

4
plugins/chat/spec/lib/chat/review_queue_spec.rb
View File

@ -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

14
plugins/chat/spec/models/chat/thread_spec.rb
View File

@ -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

91
plugins/chat/spec/requests/chat/api/messages_controller_spec.rb
View File

@ -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

76
plugins/chat/spec/requests/chat_controller_spec.rb
View File

@ -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) }

74
plugins/chat/spec/services/chat/restore_message_spec.rb
View File

@ -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

18
plugins/chat/spec/services/chat/trash_message_spec.rb
View File

@ -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! }