From 91c5658e9baaab0df581f53ab56dc784b152d5ec Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 10 May 2023 13:58:15 +0200 Subject: [PATCH] FIX: Handle deleted original message for thread index (#21470) Since we have channel message retention which deletes messages, we can end up with cases where the thread is still around but the message is deleted. We will handle the cascade delete in a different commit -- for now we will ensure the thread list lookup handles this case and doesn't error. --- plugins/chat/app/serializers/chat/thread_serializer.rb | 4 +++- plugins/chat/app/services/chat/lookup_channel_threads.rb | 4 +++- .../chat/assets/stylesheets/common/chat-thread-list.scss | 1 + .../chat/assets/stylesheets/mobile/chat-threads-list.scss | 4 ++++ .../chat/spec/services/chat/lookup_channel_threads_spec.rb | 7 ++++++- 5 files changed, 17 insertions(+), 3 deletions(-) diff --git a/plugins/chat/app/serializers/chat/thread_serializer.rb b/plugins/chat/app/serializers/chat/thread_serializer.rb index 76f6260e251..b5790415f67 100644 --- a/plugins/chat/app/serializers/chat/thread_serializer.rb +++ b/plugins/chat/app/serializers/chat/thread_serializer.rb @@ -10,7 +10,9 @@ module Chat def initialize(object, opts) super(object, opts) @opts = opts - original_message.thread = object + + # Avoids an N1 to re-load the thread in the serializer for original_message. + object.original_message.thread = object end def meta diff --git a/plugins/chat/app/services/chat/lookup_channel_threads.rb b/plugins/chat/app/services/chat/lookup_channel_threads.rb index 8f73e010142..7bab283f5e1 100644 --- a/plugins/chat/app/services/chat/lookup_channel_threads.rb +++ b/plugins/chat/app/services/chat/lookup_channel_threads.rb @@ -64,7 +64,9 @@ module Chat ) .where("chat_messages.user_id = ? OR chat_messages.user_id IS NULL", guardian.user.id) .where(channel_id: channel.id) - .where("original_messages.deleted_at IS NULL AND chat_messages.deleted_at IS NULL") + .where( + "original_messages.deleted_at IS NULL AND chat_messages.deleted_at IS NULL AND original_messages.id IS NOT NULL", + ) .group("chat_threads.id") .order("last_posted_at DESC NULLS LAST") .limit(50) diff --git a/plugins/chat/assets/stylesheets/common/chat-thread-list.scss b/plugins/chat/assets/stylesheets/common/chat-thread-list.scss index 32c14ab9165..1e785d8f810 100644 --- a/plugins/chat/assets/stylesheets/common/chat-thread-list.scss +++ b/plugins/chat/assets/stylesheets/common/chat-thread-list.scss @@ -24,5 +24,6 @@ &__no-threads { @include thread-list-item; + margin: 0.5rem 0rem 0.5rem 0.5rem; } } diff --git a/plugins/chat/assets/stylesheets/mobile/chat-threads-list.scss b/plugins/chat/assets/stylesheets/mobile/chat-threads-list.scss index f53e77e768b..ddc9ec0e52a 100644 --- a/plugins/chat/assets/stylesheets/mobile/chat-threads-list.scss +++ b/plugins/chat/assets/stylesheets/mobile/chat-threads-list.scss @@ -3,5 +3,9 @@ .chat-thread-list-item { margin: 0.5rem; } + + &__no-threads { + margin: 0.5rem; + } } } diff --git a/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb b/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb index 97d06d736d4..e0e9cc82089 100644 --- a/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb +++ b/plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb @@ -59,11 +59,16 @@ RSpec.describe Chat::LookupChannelThreads do expect(result.threads.map(&:id)).to eq([thread_3.id, thread_1.id, thread_2.id]) end - it "does not return threads where the original message is deleted" do + it "does not return threads where the original message is trashed" do thread_1.original_message.trash! expect(result.threads.map(&:id)).to eq([thread_3.id, thread_2.id]) end + it "does not return threads where the original message is deleted" do + thread_1.original_message.destroy + expect(result.threads.map(&:id)).to eq([thread_3.id, thread_2.id]) + end + it "does not count deleted messages for sort order" do Chat::Message.find_by(user: current_user, thread: thread_3).trash! expect(result.threads.map(&:id)).to eq([thread_1.id, thread_2.id])