DEV: refactor flag message (#24604)

- Uses a chat service: `Chat::FlatMessage`
- Moves logic inside chat api controllers
- Create a javascript chat api helper: `chatApi.flagMessage(...)`

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

@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class Chat::Api::ChannelsMessagesFlagsController < Chat::ApiController
+  def create
+    RateLimiter.new(current_user, "flag_chat_message", 4, 1.minutes).performed!
+
+    with_service(Chat::FlagMessage) do
+      on_model_not_found(:message) { raise Discourse::NotFound }
+      on_failed_policy(:can_flag_message_in_channel) { raise Discourse::InvalidAccess }
+    end
+  end
+end

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

@@ -7,13 +7,7 @@ module Chat
     # able to get deleted channels and recover them.
     before_action :find_chat_message, only: %i[rebake message_link]
     before_action :set_channel_and_chatable_with_access_check,
-                  except: %i[
+                  except: %i[respond message_link set_user_chat_status dismiss_retention_reminder]
-                    respond
-                    message_link
-                    set_user_chat_status
-                    dismiss_retention_reminder
-                    flag
-                  ]
 
     def respond
       render
@@ -88,35 +82,6 @@ module Chat
       render json: success_json.merge(markdown: markdown)
     end
 
-    def flag
-      RateLimiter.new(current_user, "flag_chat_message", 4, 1.minutes).performed!
-
-      permitted_params =
-        params.permit(
-          %i[chat_message_id flag_type_id message is_warning take_action queue_for_review],
-        )
-
-      chat_message =
-        Chat::Message.includes(:chat_channel, :revisions).find(permitted_params[:chat_message_id])
-
-      flag_type_id = permitted_params[:flag_type_id].to_i
-
-      if !ReviewableScore.types.values.include?(flag_type_id)
-        raise Discourse::InvalidParameters.new(:flag_type_id)
-      end
-
-      set_channel_and_chatable_with_access_check(chat_channel_id: chat_message.chat_channel_id)
-
-      result =
-        Chat::ReviewQueue.new.flag_message(chat_message, guardian, flag_type_id, permitted_params)
-
-      if result[:success]
-        render json: success_json
-      else
-        render_json_error(result[:errors])
-      end
-    end
-
     private
 
     def preloaded_chat_message_query

plugins/chat/app/services/chat/flag_message.rb

@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+
+module Chat
+  # Service responsible to flag a message.
+  #
+  # @example
+  #  ::Chat::FlagMessage.call(
+  #    guardian: guardian,
+  #    channel_id: 1,
+  #    message_id: 43,
+  #  )
+  #
+  class FlagMessage
+    include Service::Base
+
+    # @!method call(guardian:, channel_id:, data:)
+    #   @param [Guardian] guardian
+    #   @param [Integer] channel_id of the channel
+    #   @param [Integer] message_id of the message
+    #   @param [Integer] flag_type_id - Type of flag to create
+    #   @param [String] optional message - Used when the flag type is notify_user or notify_moderators and we have to create
+    #     a separate PM.
+    #   @param [Boolean] optional is_warning - Staff can send warnings when using the notify_user flag.
+    #   @param [Boolean] optional take_action - Automatically approves the created reviewable and deletes the chat message.
+    #   @param [Boolean] optional queue_for_review - Adds a special reason to the reviewable score and creates the reviewable using
+    #     the force_review option.
+
+    #   @return [Service::Base::Context]
+    contract
+    model :message
+    policy :can_flag_message_in_channel
+    step :flag_message
+
+    # @!visibility private
+    class Contract
+      attribute :message_id, :integer
+      validates :message_id, presence: true
+
+      attribute :channel_id, :integer
+      validates :channel_id, presence: true
+
+      attribute :flag_type_id, :integer
+      validates :flag_type_id, inclusion: { in: ::ReviewableScore.types.values }
+
+      attribute :message, :string
+      attribute :is_warning, :boolean
+      attribute :take_action, :boolean
+      attribute :queue_for_review, :boolean
+    end
+
+    private
+
+    def fetch_message(contract:, **)
+      Chat::Message.includes(:chat_channel, :revisions).find_by(
+        id: contract.message_id,
+        chat_channel_id: contract.channel_id,
+      )
+    end
+
+    def can_flag_message_in_channel(guardian:, contract:, message:, **)
+      guardian.can_join_chat_channel?(message.chat_channel) &&
+        guardian.can_flag_chat_message?(message) &&
+        guardian.can_flag_message_as?(
+          message,
+          contract.flag_type_id,
+          {
+            queue_for_review: contract.queue_for_review,
+            take_action: contract.take_action,
+            is_warning: contract.is_warning,
+          },
+        )
+    end
+
+    def flag_message(message:, contract:, guardian:, **)
+      Chat::ReviewQueue.new.flag_message(
+        message,
+        guardian,
+        contract.flag_type_id,
+        message: contract.message,
+        is_warning: contract.is_warning,
+        take_action: contract.take_action,
+        queue_for_review: contract.queue_for_review,
+      )
+    end
+  end
+end

plugins/chat/assets/javascripts/discourse/lib/chat-message-flag.js

@@ -1,9 +1,16 @@
-import { ajax } from "discourse/lib/ajax";
+import { setOwner } from "@ember/application";
+import { inject as service } from "@ember/service";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import getURL from "discourse-common/lib/get-url";
 import I18n from "discourse-i18n";
 
 export default class ChatMessageFlag {
+  @service chatApi;
+
+  constructor(owner) {
+    setOwner(this, owner);
+  }
+
   title() {
     return "flagging.title";
   }
@@ -57,19 +64,22 @@ export default class ChatMessageFlag {
     return this._rewriteFlagDescriptions(flagsAvailable);
   }
 
-  create(flagModal, opts) {
+  async create(flagModal, opts) {
     flagModal.args.closeModal();
 
-    return ajax("/chat/flag", {
+    const channelId = flagModal.args.model.flagModel.channel.id;
-      method: "PUT",
+    const messageId = flagModal.args.model.flagModel.id;
-      data: {
+
-        chat_message_id: flagModal.args.model.flagModel.id,
+    try {
+      await this.chatApi.flagMessage(channelId, messageId, {
         flag_type_id: flagModal.selected.id,
         message: opts.message,
         is_warning: opts.isWarning,
         take_action: opts.takeAction,
         queue_for_review: opts.queue_for_review,
-      },
+      });
-    }).catch((error) => popupAjaxError(error));
+    } catch (error) {
+      popupAjaxError(error);
+    }
   }
 }

plugins/chat/assets/javascripts/discourse/lib/chat-message-interactor.js

@@ -1,5 +1,5 @@
 import { tracked } from "@glimmer/tracking";
-import { setOwner } from "@ember/application";
+import { getOwner, setOwner } from "@ember/application";
 import { action } from "@ember/object";
 import { inject as service } from "@ember/service";
 import BookmarkModal from "discourse/components/modal/bookmark";
@@ -360,7 +360,7 @@ export default class ChatMessageInteractor {
     model.user_id = this.message.user?.id;
     this.modal.show(FlagModal, {
       model: {
-        flagTarget: new ChatMessageFlag(),
+        flagTarget: new ChatMessageFlag(getOwner(this)),
         flagModel: model,
         setHidden: () => model.set("hidden", true),
       },

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

@@ -33,6 +33,29 @@ export default class ChatApi extends Service {
     );
   }
 
+  /**
+   * Flags a message in a channel.
+   * @param {number} channelId - The ID of the channel.
+   * @param {number} messageId - The ID of the message to flag.
+   * @param {object} params - Params of the flag.
+   * @param {integer} params.flag_type_id
+   * @param {string} [params.message]
+   * @param {boolean} [params.is_warning]
+   * @param {boolean} [params.queue_for_review]
+   * @param {boolean} [params.take_action]
+   * @returns {Promise}
+   *
+   * @example
+   *
+   *    this.chatApi.flagMessage(5, 1);
+   */
+  flagMessage(channelId, messageId, params = {}) {
+    return this.#postRequest(
+      `/channels/${channelId}/messages/${messageId}/flags`,
+      params
+    );
+  }
+
   /**
    * Get a thread in a channel by its ID.
    * @param {number} channelId - The ID of the channel.

plugins/chat/config/routes.rb

@@ -8,6 +8,7 @@ Chat::Engine.routes.draw do
     post "/channels" => "channels#create"
     put "/channels/read/" => "reads#update_all"
     put "/channels/:channel_id/read/:message_id" => "reads#update"
+    post "/channels/:channel_id/messages/:message_id/flags" => "channels_messages_flags#create"
     post "/channels/:channel_id/drafts" => "channels_drafts#create"
     delete "/channels/:channel_id" => "channels#destroy"
     put "/channels/:channel_id" => "channels#update"
@@ -77,11 +78,10 @@ Chat::Engine.routes.draw do
   get "/message/:message_id" => "chat#message_link"
   put ":chat_channel_id/react/:message_id" => "chat#react"
   put "/:chat_channel_id/:message_id/rebake" => "chat#rebake"
-  post "/:chat_channel_id/:message_id/flag" => "chat#flag"
   post "/:chat_channel_id/quote" => "chat#quote_messages"
   put "/user_chat_enabled/:user_id" => "chat#set_user_chat_status"
   post "/:chat_channel_id" => "api/channel_messages#create"
-  put "/flag" => "chat#flag"
+
   get "/emojis" => "emojis#index"
 
   base_c_route = "/c/:channel_title/:channel_id"

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

@@ -0,0 +1,85 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::Api::ChannelsMessagesFlagsController do
+  fab!(:current_user) { Fabricate(:user) }
+  fab!(:message_1) { Fabricate(:chat_message) }
+
+  let(:params) { { flag_type_id: ::ReviewableScore.types[:off_topic] } }
+
+  before do
+    SiteSetting.chat_enabled = true
+    SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
+    SiteSetting.chat_message_flag_allowed_groups = [Group::AUTO_GROUPS[:everyone]]
+    sign_in(current_user)
+  end
+
+  describe "#create" do
+    it "ratelimits flagging" do
+      RateLimiter.enable
+
+      Fabricate
+        .times(4, :chat_message)
+        .each do |message|
+          post "/chat/api/channels/#{message.chat_channel.id}/messages/#{message.id}/flags",
+               params: params
+
+          expect(response.status).to eq(200)
+        end
+
+      post "/chat/api/channels/#{message_1.chat_channel.id}/messages/#{message_1.id}/flags",
+           params: params
+
+      expect(response.status).to eq(429)
+    ensure
+      RateLimiter.disable
+    end
+
+    describe "success" do
+      it "works" do
+        post "/chat/api/channels/#{message_1.chat_channel.id}/messages/#{message_1.id}/flags",
+             params: params
+
+        expect(response.status).to eq(200)
+      end
+    end
+
+    context "when user can’t flag message" do
+      before { UserSilencer.new(current_user).silence }
+
+      it "returns a 403" do
+        post "/chat/api/channels/#{message_1.chat_channel.id}/messages/#{message_1.id}/flags",
+             params: params
+
+        expect(response.status).to eq(403)
+        expect(response.parsed_body["errors"].first).to eq(I18n.t("invalid_access"))
+      end
+    end
+
+    context "when channel is not found" do
+      it "returns a 404" do
+        post "/chat/api/channels/-999/messages/#{message_1.id}/flags", params: params
+
+        expect(response.status).to eq(404)
+      end
+    end
+
+    context "when message is trashed" do
+      before { trash_message!(message_1) }
+
+      it "returns a 403" do
+        post "/chat/api/channels/#{message_1.chat_channel.id}/messages/#{message_1.id}/flags",
+             params: params
+
+        expect(response.status).to eq(404)
+      end
+    end
+
+    context "when message is not found" do
+      it "returns a 404" do
+        post "/chat/api/channels/#{message_1.chat_channel.id}/messages/-999/flags", params: params
+
+        expect(response.status).to eq(404)
+      end
+    end
+  end
+end

plugins/chat/spec/requests/chat_controller_spec.rb

@@ -454,91 +454,6 @@ RSpec.describe Chat::ChatController do
     end
   end
 
-  describe "#flag" do
-    fab!(:admin_chat_message) { Fabricate(:chat_message, user: admin, chat_channel: chat_channel) }
-    fab!(:user_chat_message) { Fabricate(:chat_message, user: user, chat_channel: chat_channel) }
-
-    fab!(:admin_dm_message) { Fabricate(:chat_message, user: admin, chat_channel: dm_chat_channel) }
-
-    before do
-      sign_in(user)
-      Group.refresh_automatic_groups!
-    end
-
-    it "creates reviewable" do
-      expect {
-        put "/chat/flag.json",
-            params: {
-              chat_message_id: admin_chat_message.id,
-              flag_type_id: ReviewableScore.types[:off_topic],
-            }
-      }.to change { Chat::ReviewableMessage.where(target: admin_chat_message).count }.by(1)
-      expect(response.status).to eq(200)
-    end
-
-    it "errors for silenced users" do
-      UserSilencer.new(user).silence
-
-      put "/chat/flag.json",
-          params: {
-            chat_message_id: admin_chat_message.id,
-            flag_type_id: ReviewableScore.types[:off_topic],
-          }
-      expect(response.status).to eq(403)
-    end
-
-    it "doesn't allow flagging your own message" do
-      put "/chat/flag.json",
-          params: {
-            chat_message_id: user_chat_message.id,
-            flag_type_id: ReviewableScore.types[:off_topic],
-          }
-      expect(response.status).to eq(403)
-    end
-
-    it "doesn't allow flagging messages in a read_only channel" do
-      user_chat_message.chat_channel.update(status: :read_only)
-      put "/chat/flag.json",
-          params: {
-            chat_message_id: admin_chat_message.id,
-            flag_type_id: ReviewableScore.types[:off_topic],
-          }
-
-      expect(response.status).to eq(403)
-    end
-
-    it "doesn't allow flagging staff if SiteSetting.allow_flagging_staff is false" do
-      SiteSetting.allow_flagging_staff = false
-      put "/chat/flag.json",
-          params: {
-            chat_message_id: admin_chat_message.id,
-            flag_type_id: ReviewableScore.types[:off_topic],
-          }
-      expect(response.status).to eq(403)
-    end
-
-    it "returns a 429 when the user attempts to flag more than 4 messages  in 1 minute" do
-      RateLimiter.enable
-
-      [message_1, message_2, message_3, message_4].each do |message|
-        put "/chat/flag.json",
-            params: {
-              chat_message_id: message.id,
-              flag_type_id: ReviewableScore.types[:off_topic],
-            }
-        expect(response.status).to eq(200)
-      end
-
-      put "/chat/flag.json",
-          params: {
-            chat_message_id: message_5.id,
-            flag_type_id: ReviewableScore.types[:off_topic],
-          }
-
-      expect(response.status).to eq(429)
-    end
-  end
-
   describe "#message_link" do
     it "ensures message's channel can be seen" do
       channel = Fabricate(:category_channel, chatable: Fabricate(:category))

plugins/chat/spec/services/chat/flag_message_spec.rb

@@ -0,0 +1,107 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::FlagMessage do
+  describe described_class::Contract, type: :model do
+    subject(:contract) { described_class.new }
+
+    it { is_expected.to validate_presence_of(:channel_id) }
+    it { is_expected.to validate_presence_of(:message_id) }
+    it do
+      is_expected.to validate_inclusion_of(:flag_type_id).in_array(ReviewableScore.types.values)
+    end
+  end
+
+  describe ".call" do
+    subject(:result) { described_class.call(params) }
+
+    fab!(:current_user) { Fabricate(:user) }
+    fab!(:channel_1) { Fabricate(:chat_channel) }
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
+
+    let(:guardian) { Guardian.new(current_user) }
+    let(:channel_id) { channel_1.id }
+    let(:message_id) { message_1.id }
+    let(:flag_type_id) { ReviewableScore.types.values.first }
+    let(:message) { nil }
+    let(:is_warning) { nil }
+    let(:take_action) { nil }
+
+    before { SiteSetting.direct_message_enabled_groups = Group::AUTO_GROUPS[:everyone] }
+
+    let(:params) do
+      {
+        guardian: guardian,
+        channel_id: channel_id,
+        message_id:,
+        flag_type_id: flag_type_id,
+        message: message,
+        is_warning: is_warning,
+        take_action: take_action,
+      }
+    end
+
+    context "when all steps pass" do
+      fab!(:current_user) { Fabricate(:admin) }
+
+      it "flags the message" do
+        expect { result }.to change { Reviewable.count }.by(1)
+
+        reviewable = Reviewable.last
+        expect(reviewable.target_type).to eq("ChatMessage")
+        expect(reviewable.created_by_id).to eq(current_user.id)
+        expect(reviewable.target_created_by_id).to eq(message_1.user.id)
+        expect(reviewable.target_id).to eq(message_1.id)
+        expect(reviewable.payload).to eq("message_cooked" => message_1.cooked)
+      end
+    end
+
+    context "when channel is not found" do
+      before { params[:channel_id] = -999 }
+
+      it { is_expected.to fail_to_find_a_model(:message) }
+    end
+
+    context "when user is silenced" do
+      before { UserSilencer.new(current_user).silence }
+
+      it { is_expected.to fail_a_policy(:can_flag_message_in_channel) }
+    end
+
+    context "when channel is in read only mode" do
+      before { channel_1.update!(status: Chat::Channel.statuses[:read_only]) }
+
+      it { is_expected.to fail_a_policy(:can_flag_message_in_channel) }
+    end
+
+    context "when flagging staff message is not allowed" do
+      before { SiteSetting.allow_flagging_staff = false }
+
+      fab!(:message_1) do
+        Fabricate(:chat_message, chat_channel: channel_1, user: Fabricate(:admin))
+      end
+
+      it { is_expected.to fail_a_policy(:can_flag_message_in_channel) }
+    end
+
+    context "when flagging its own message" do
+      fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: current_user) }
+
+      before { UserSilencer.new(current_user).silence }
+
+      it { is_expected.to fail_a_policy(:can_flag_message_in_channel) }
+    end
+
+    context "when message is not found" do
+      before { params[:message_id] = -999 }
+
+      it { is_expected.to fail_to_find_a_model(:message) }
+    end
+
+    context "when user doesn't have access to channel" do
+      fab!(:channel_1) { Fabricate(:private_category_channel, group: Fabricate(:group)) }
+      fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1) }
+
+      it { is_expected.to fail_a_policy(:can_flag_message_in_channel) }
+    end
+  end
+end