From ee5bdb343683f0cc2cfc44528cc18d1d77a81154 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Tue, 28 Nov 2023 18:24:09 +0100 Subject: [PATCH] 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(...)` --- .../api/channels_messages_flags_controller.rb | 12 ++ .../app/controllers/chat/chat_controller.rb | 37 +----- .../chat/app/services/chat/flag_message.rb | 86 ++++++++++++++ .../discourse/lib/chat-message-flag.js | 26 +++-- .../discourse/lib/chat-message-interactor.js | 4 +- .../discourse/services/chat-api.js | 23 ++++ plugins/chat/config/routes.rb | 4 +- ...channels_messages_flags_controller_spec.rb | 85 ++++++++++++++ .../spec/requests/chat_controller_spec.rb | 85 -------------- .../spec/services/chat/flag_message_spec.rb | 107 ++++++++++++++++++ 10 files changed, 336 insertions(+), 133 deletions(-) create mode 100644 plugins/chat/app/controllers/chat/api/channels_messages_flags_controller.rb create mode 100644 plugins/chat/app/services/chat/flag_message.rb create mode 100644 plugins/chat/spec/requests/chat/api/channels_messages_flags_controller_spec.rb create mode 100644 plugins/chat/spec/services/chat/flag_message_spec.rb diff --git a/plugins/chat/app/controllers/chat/api/channels_messages_flags_controller.rb b/plugins/chat/app/controllers/chat/api/channels_messages_flags_controller.rb new file mode 100644 index 00000000000..448c1ce2d75 --- /dev/null +++ b/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 diff --git a/plugins/chat/app/controllers/chat/chat_controller.rb b/plugins/chat/app/controllers/chat/chat_controller.rb index 2ba17b38562..fd13052c63e 100644 --- a/plugins/chat/app/controllers/chat/chat_controller.rb +++ b/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[ - respond - message_link - set_user_chat_status - dismiss_retention_reminder - flag - ] + except: %i[respond message_link set_user_chat_status dismiss_retention_reminder] 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 diff --git a/plugins/chat/app/services/chat/flag_message.rb b/plugins/chat/app/services/chat/flag_message.rb new file mode 100644 index 00000000000..ad9a07cef40 --- /dev/null +++ b/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 diff --git a/plugins/chat/assets/javascripts/discourse/lib/chat-message-flag.js b/plugins/chat/assets/javascripts/discourse/lib/chat-message-flag.js index 0c0e0272055..4662111a435 100644 --- a/plugins/chat/assets/javascripts/discourse/lib/chat-message-flag.js +++ b/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", { - method: "PUT", - data: { - chat_message_id: flagModal.args.model.flagModel.id, + const channelId = flagModal.args.model.flagModel.channel.id; + const messageId = 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); + } } } diff --git a/plugins/chat/assets/javascripts/discourse/lib/chat-message-interactor.js b/plugins/chat/assets/javascripts/discourse/lib/chat-message-interactor.js index 499d9d2e6fb..f2c1b90c134 100644 --- a/plugins/chat/assets/javascripts/discourse/lib/chat-message-interactor.js +++ b/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), }, diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-api.js b/plugins/chat/assets/javascripts/discourse/services/chat-api.js index 76c421c799b..ac3c2355cdf 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-api.js +++ b/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. diff --git a/plugins/chat/config/routes.rb b/plugins/chat/config/routes.rb index 8e5c16d05de..e66b76dfd78 100644 --- a/plugins/chat/config/routes.rb +++ b/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" diff --git a/plugins/chat/spec/requests/chat/api/channels_messages_flags_controller_spec.rb b/plugins/chat/spec/requests/chat/api/channels_messages_flags_controller_spec.rb new file mode 100644 index 00000000000..43609d14f1d --- /dev/null +++ b/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 diff --git a/plugins/chat/spec/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb index d2f3c106073..e619fcd2087 100644 --- a/plugins/chat/spec/requests/chat_controller_spec.rb +++ b/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)) diff --git a/plugins/chat/spec/services/chat/flag_message_spec.rb b/plugins/chat/spec/services/chat/flag_message_spec.rb new file mode 100644 index 00000000000..479d92023dd --- /dev/null +++ b/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