FIX: Trashing message should reset last read (#20912)

When a chat message is trashed and the message is used
for someone's UserChatChannelMembership#last_read_message_id,
the user would end up with some read state issues until
someone posted a new message in the channel, since we didn't
clear it like we did on bulk message delete.

This commit fixes the issue, and also takes the opportunity
to start a MessagesController in the API namespace, and move
the trash message functionality into the new service format.

plugins/chat/app/controllers/chat/api/channel_messages_controller.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class Chat::Api::ChannelMessagesController < Chat::ApiController
+  def destroy
+    with_service(Chat::TrashMessage) { on_model_not_found(:message) { raise Discourse::NotFound } }
+  end
+end

plugins/chat/app/controllers/chat/chat_controller.rb

@@ -224,14 +224,6 @@ module Chat
       render json: success_json
     end
 
-    def delete
-      guardian.ensure_can_delete_chat!(@message, @chatable)
-
-      Chat::MessageDestroyer.new.trash_message(@message, current_user)
-
-      head :ok
-    end
-
     def restore
       chat_channel = @message.chat_channel
       guardian.ensure_can_restore_chat!(@message, chat_channel.chatable)

plugins/chat/app/services/chat/message_destroyer.rb

@@ -12,19 +12,6 @@ module Chat
         end
     end
 
-    def trash_message(message, actor)
-      Chat::Message.transaction do
-        message.trash!(actor)
-        Chat::Mention.where(chat_message: message).destroy_all
-        DiscourseEvent.trigger(:chat_message_trashed, message, message.chat_channel, actor)
-
-        # FIXME: We should do something to prevent the blue/green bubble
-        # of other channel members from getting out of sync when a message
-        # gets deleted.
-        Chat::Publisher.publish_delete!(message.chat_channel, message)
-      end
-    end
-
     private
 
     def reset_last_read(message_ids)

plugins/chat/app/services/chat/trash_message.rb

@@ -0,0 +1,70 @@
+# frozen_string_literal: true
+
+module Chat
+  # Service responsible for trashing a chat message
+  # for a channel and ensuring that the client and read state is
+  # updated.
+  #
+  # @example
+  #  Chat::TrashMessage.call(message_id: 2, channel_id: 1, guardian: guardian)
+  #
+  class TrashMessage
+    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 :trash_message
+      step :destroy_mentions
+      step :update_tracking_state
+    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.includes(chat_channel: :chatable).find_by(
+        id: contract.message_id,
+        chat_channel_id: contract.channel_id,
+      )
+    end
+
+    def invalid_access(guardian:, message:, **)
+      guardian.can_delete_chat?(message, message.chat_channel.chatable)
+    end
+
+    def trash_message(message:, **)
+      message.trash!
+    end
+
+    def destroy_mentions(message:, **)
+      Chat::Mention.where(chat_message: message).destroy_all
+    end
+
+    def update_tracking_state(message:, **)
+      Chat::UserChatChannelMembership.where(last_read_message_id: message.id).update_all(
+        last_read_message_id: nil,
+      )
+    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)
+    end
+  end
+end

plugins/chat/assets/javascripts/discourse/components/chat-message.js

@@ -39,6 +39,7 @@ export default class ChatMessage extends Component {
   @service appEvents;
   @service capabilities;
   @service chat;
+  @service chatApi;
   @service chatEmojiReactionStore;
   @service chatEmojiPickerManager;
   @service chatChannelsManager;
@@ -640,12 +641,9 @@ export default class ChatMessage extends Component {
 
   @action
   deleteMessage() {
-    return ajax(
+    return this.chatApi
-      `/chat/${this.args.message.channelId}/${this.args.message.id}`,
+      .trashMessage(this.args.message.channelId, this.args.message.id)
-      {
+      .catch(popupAjaxError);
-        type: "DELETE",
-      }
-    ).catch(popupAjaxError);
   }
 
   @action

plugins/chat/assets/javascripts/discourse/services/chat-api.js

@@ -144,6 +144,16 @@ export default class ChatApi extends Service {
     });
   }
 
+  /**
+   * Trashes (soft deletes) a chat message.
+   * @param {number} channelId - ID of the channel.
+   * @param {number} messageId - ID of the message.
+   * @returns {Promise}
+   */
+  trashMessage(channelId, messageId) {
+    return this.#deleteRequest(`/channels/${channelId}/messages/${messageId}`);
+  }
+
   /**
    * Creates a channel archive.
    * @param {number} channelId - The ID of the channel.

plugins/chat/config/routes.rb

@@ -29,6 +29,8 @@ Chat::Engine.routes.draw do
     get "/mentions/groups" => "hints#check_group_mentions", :format => :json
 
     get "/channels/:channel_id/threads/:thread_id" => "channel_threads#show"
+
+    delete "/channels/:channel_id/messages/:message_id" => "channel_messages#destroy"
   end
 
   # direct_messages_controller routes
@@ -56,7 +58,6 @@ Chat::Engine.routes.draw do
   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"
-  delete "/:chat_channel_id/:message_id" => "chat#delete"
   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"

plugins/chat/spec/requests/chat/api/messages_controller_spec.rb

@@ -0,0 +1,109 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::Api::ChannelMessagesController do
+  fab!(:current_user) { Fabricate(:user) }
+  fab!(:admin) { Fabricate(:admin) }
+
+  before do
+    SiteSetting.chat_enabled = true
+    SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
+  end
+
+  describe "#delete" do
+    RSpec.shared_examples "chat_message_deletion" do
+      it "doesn't allow a user to delete another user's message" do
+        sign_in(other_user)
+
+        delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{message.id}.json"
+        expect(response.status).to eq(403)
+      end
+
+      it "doesn't allow a silenced user to delete their message" do
+        sign_in(other_user)
+        UserSilencer.new(other_user).silence
+
+        delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{other_user_message.id}.json"
+        expect(response.status).to eq(403)
+      end
+
+      it "allows admin to delete others' messages" do
+        sign_in(admin)
+
+        expect {
+          delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{message.id}.json"
+        }.to change { Chat::Message.count }.by(-1)
+        expect(response.status).to eq(200)
+      end
+
+      it "does not allow message delete when chat channel is read_only" do
+        sign_in(message.user)
+
+        chat_channel.update!(status: :read_only)
+        expect {
+          delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{message.id}.json"
+        }.not_to change { Chat::Message.count }
+        expect(response.status).to eq(403)
+
+        sign_in(admin)
+        delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{message.id}.json"
+        expect(response.status).to eq(403)
+      end
+
+      it "only allows admin to delete when chat channel is closed" do
+        sign_in(admin)
+
+        chat_channel.update!(status: :read_only)
+        expect {
+          delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{message.id}.json"
+        }.not_to change { Chat::Message.count }
+        expect(response.status).to eq(403)
+
+        chat_channel.update!(status: :closed)
+        expect {
+          delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{message.id}.json"
+        }.to change { Chat::Message.count }.by(-1)
+        expect(response.status).to eq(200)
+      end
+    end
+
+    describe "for category" do
+      fab!(:user_2) { Fabricate(:user) }
+      fab!(:chat_channel) { Fabricate(:chat_channel) }
+      fab!(:message) { Fabricate(:chat_message, chat_channel: chat_channel, user: current_user) }
+      fab!(:user_2_message) { Fabricate(:chat_message, chat_channel: chat_channel, user: user_2) }
+
+      it_behaves_like "chat_message_deletion" do
+        let(:other_user) { user_2 }
+        let(:other_user_message) { user_2_message }
+      end
+
+      it "allows users to delete their own messages" do
+        sign_in(current_user)
+        expect {
+          delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{message.id}.json"
+        }.to change { Chat::Message.count }.by(-1)
+        expect(response.status).to eq(200)
+      end
+    end
+
+    describe "for dm channel" do
+      fab!(:user_2) { Fabricate(:user) }
+      fab!(:chat_channel) { Fabricate(:direct_message_channel, users: [current_user, user_2]) }
+      fab!(:message) { Fabricate(:chat_message, chat_channel: chat_channel, user: current_user) }
+      fab!(:user_2_message) { Fabricate(:chat_message, chat_channel: chat_channel, user: user_2) }
+
+      it_behaves_like "chat_message_deletion" do
+        let(:other_user) { user_2 }
+        let(:other_user_message) { user_2_message }
+      end
+
+      it "allows users to delete their own messages" do
+        sign_in(current_user)
+        expect {
+          delete "/chat/api/channels/#{message.chat_channel_id}/messages/#{message.id}.json"
+        }.to change { Chat::Message.count }.by(-1)
+        expect(response.status).to eq(200)
+      end
+    end
+  end
+end

plugins/chat/spec/requests/chat_controller_spec.rb

@@ -559,96 +559,6 @@ RSpec.describe Chat::ChatController do
     end
   end
 
-  RSpec.shared_examples "chat_message_deletion" do
-    it "doesn't allow a user to delete another user's message" do
-      sign_in(other_user)
-
-      delete "/chat/#{chat_channel.id}/#{Chat::Message.last.id}.json"
-      expect(response.status).to eq(403)
-    end
-
-    it "doesn't allow a silenced user to delete their message" do
-      sign_in(other_user)
-      UserSilencer.new(other_user).silence
-
-      delete "/chat/#{other_user_message.chat_channel.id}/#{other_user_message.id}.json"
-      expect(response.status).to eq(403)
-    end
-
-    it "Allows admin to delete others' messages" do
-      sign_in(admin)
-
-      events = nil
-      expect do
-        events =
-          DiscourseEvent.track_events do
-            delete "/chat/#{chat_channel.id}/#{Chat::Message.last.id}.json"
-          end
-      end.to change { Chat::Message.count }.by(-1)
-
-      expect(response.status).to eq(200)
-      expect(events.map { |event| event[:event_name] }).to include(:chat_message_trashed)
-    end
-
-    it "does not allow message delete when chat channel is read_only" do
-      sign_in(Chat::Message.last.user)
-
-      chat_channel.update!(status: :read_only)
-      expect { delete "/chat/#{chat_channel.id}/#{Chat::Message.last.id}.json" }.not_to change {
-        Chat::Message.count
-      }
-      expect(response.status).to eq(403)
-
-      sign_in(admin)
-      delete "/chat/#{chat_channel.id}/#{Chat::Message.last.id}.json"
-      expect(response.status).to eq(403)
-    end
-
-    it "only allows admin to delete when chat channel is closed" do
-      sign_in(admin)
-
-      chat_channel.update!(status: :read_only)
-      expect { delete "/chat/#{chat_channel.id}/#{Chat::Message.last.id}.json" }.not_to change {
-        Chat::Message.count
-      }
-      expect(response.status).to eq(403)
-
-      chat_channel.update!(status: :closed)
-      expect { delete "/chat/#{chat_channel.id}/#{Chat::Message.last.id}.json" }.to change {
-        Chat::Message.count
-      }.by(-1)
-      expect(response.status).to eq(200)
-    end
-  end
-
-  describe "#delete" do
-    fab!(:second_user) { Fabricate(:user) }
-    fab!(:second_user_message) do
-      Fabricate(:chat_message, user: second_user, chat_channel: chat_channel)
-    end
-
-    before do
-      Chat::Message.create!(user: user, message: "this is a message", chat_channel: chat_channel)
-    end
-
-    describe "for category" do
-      fab!(:chat_channel) { Fabricate(:category_channel, chatable: category) }
-
-      it_behaves_like "chat_message_deletion" do
-        let(:other_user) { second_user }
-        let(:other_user_message) { second_user_message }
-      end
-
-      it "Allows users to delete their own messages" do
-        sign_in(user)
-        expect { delete "/chat/#{chat_channel.id}/#{Chat::Message.last.id}.json" }.to change {
-          Chat::Message.count
-        }.by(-1)
-        expect(response.status).to eq(200)
-      end
-    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)

plugins/chat/spec/services/chat/message_destroyer_spec.rb

@@ -43,56 +43,4 @@ RSpec.describe Chat::MessageDestroyer do
       expect(message_2.reload).to be_present
     end
   end
-
-  describe "#trash_message" do
-    fab!(:message_1) { Fabricate(:chat_message) }
-    fab!(:actor) { Discourse.system_user }
-
-    it "trashes the message" do
-      described_class.new.trash_message(message_1, actor)
-
-      expect(Chat::Message.find_by(id: message_1.id)).to be_blank
-      expect(Chat::Message.with_deleted.find_by(id: message_1.id)).to be_present
-    end
-
-    context "when the message has associated notifications" do
-      context "when notification has the chat_mention type" do
-        it "deletes associated notification and chat mention relations" do
-          notification =
-            Fabricate(:notification, notification_type: Notification.types[:chat_mention])
-          chat_mention =
-            Fabricate(:chat_mention, chat_message: message_1, notification: notification)
-
-          described_class.new.trash_message(message_1, actor)
-
-          expect { notification.reload }.to raise_error(ActiveRecord::RecordNotFound)
-          expect { chat_mention.reload }.to raise_error(ActiveRecord::RecordNotFound)
-        end
-      end
-    end
-
-    it "publishes a MB message to update clients" do
-      delete_message =
-        MessageBus
-          .track_publish("/chat/#{message_1.chat_channel_id}") do
-            described_class.new.trash_message(message_1, actor)
-          end
-          .first
-
-      expect(delete_message).to be_present
-      message_data = delete_message.data
-
-      expect(message_data[:type]).to eq("delete")
-      expect(message_data[:deleted_id]).to eq(message_1.id)
-      expect(message_data[:deleted_at]).to be_present
-    end
-
-    it "triggers a DiscourseEvent" do
-      delete_event =
-        DiscourseEvent.track_events { described_class.new.trash_message(message_1, actor) }.first
-
-      expect(delete_event[:event_name]).to eq(:chat_message_trashed)
-      expect(delete_event[:params]).to eq([message_1, message_1.chat_channel, actor])
-    end
-  end
 end

plugins/chat/spec/services/chat/trash_message_spec.rb

@@ -0,0 +1,97 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::TrashMessage do
+  fab!(:current_user) { Fabricate(:user) }
+  let!(:guardian) { Guardian.new(current_user) }
+  fab!(:message) { Fabricate(:chat_message, user: current_user) }
+
+  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 delete" 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 delete" do
+        it "sets the service result as successful" do
+          expect(result).to be_a_success
+        end
+
+        it "trashes the message" do
+          result
+          expect(Chat::Message.find_by(id: message.id)).to be_nil
+        end
+
+        it "destroys associated mentions" do
+          mention = Fabricate(:chat_mention, chat_message: message)
+          result
+          expect(Chat::Mention.find_by(id: mention.id)).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_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: Time.zone.now },
+          )
+        end
+
+        it "updates the tracking for users whose last_read_message_id was the trashed message" do
+          membership_1 =
+            Fabricate(
+              :user_chat_channel_membership,
+              chat_channel: message.chat_channel,
+              last_read_message: message,
+            )
+          membership_2 =
+            Fabricate(
+              :user_chat_channel_membership,
+              chat_channel: message.chat_channel,
+              last_read_message: message,
+            )
+          membership_3 =
+            Fabricate(
+              :user_chat_channel_membership,
+              chat_channel: message.chat_channel,
+              last_read_message: Fabricate(:chat_message, chat_channel: message.chat_channel),
+            )
+          result
+          expect(membership_1.reload.last_read_message_id).to be_nil
+          expect(membership_2.reload.last_read_message_id).to be_nil
+          expect(membership_3.reload.last_read_message_id).not_to be_nil
+        end
+
+        context "when message is already deleted" do
+          before { message.trash! }
+
+          it { is_expected.to fail_to_find_a_model(:message) }
+        end
+      end
+    end
+  end
+end