From 3f1024de7698fa357d5b517b37fcb96647fcbe6e Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 3 Jul 2023 10:18:37 +1000 Subject: [PATCH] DEV: Refactor DM channel creation into new service pattern (#22144) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will be used when we move the channel creation for DMs to happen when we first send a message in a DM channel to avoid a double-request. For now we can just have a new API endpoint for creating this that the existing frontend code can use, that uses the new service pattern. This also uses the new policy pattern for services where the policy can be defined in a class so a more dynamic reason for the policy failing can be sent to the controller. Co-authored-by: Loïc Guitaut --- lib/user_comm_screener.rb | 4 + .../chat/api/direct_messages_controller.rb | 32 ++ .../chat/direct_messages_controller.rb | 20 - .../can_communicate_all_parties_policy.rb | 74 ++++ .../max_users_excess_policy.rb | 28 ++ .../chat/create_direct_message_channel.rb | 98 +++++ .../chat-channel-selector-modal-inner.js | 4 +- .../javascripts/discourse/services/chat.js | 4 +- plugins/chat/config/routes.rb | 4 + .../chat/direct_message_channel_creator.rb | 133 ------ .../chat/spec/components/chat/mailer_spec.rb | 8 +- .../components/chat/message_creator_spec.rb | 8 +- .../components/chat/message_updater_spec.rb | 9 +- .../regular/chat/notify_mentioned_spec.rb | 11 +- .../direct_message_channel_creator_spec.rb | 380 ------------------ plugins/chat/spec/lib/chat/notifier_spec.rb | 11 +- .../spec/mailers/user_notifications_spec.rb | 37 +- plugins/chat/spec/plugin_helper.rb | 10 + .../api/direct_messages_controller_spec.rb | 121 ++++++ .../direct_messages_controller_spec.rb | 94 ----- .../create_direct_message_channel_spec.rb | 150 +++++++ plugins/chat/spec/support/chat_helper.rb | 19 - .../system/message_thread_indicator_spec.rb | 5 +- 23 files changed, 579 insertions(+), 685 deletions(-) create mode 100644 plugins/chat/app/controllers/chat/api/direct_messages_controller.rb create mode 100644 plugins/chat/app/policies/chat/direct_message_channel/can_communicate_all_parties_policy.rb create mode 100644 plugins/chat/app/policies/chat/direct_message_channel/max_users_excess_policy.rb create mode 100644 plugins/chat/app/services/chat/create_direct_message_channel.rb delete mode 100644 plugins/chat/lib/chat/direct_message_channel_creator.rb delete mode 100644 plugins/chat/spec/lib/chat/direct_message_channel_creator_spec.rb create mode 100644 plugins/chat/spec/requests/chat/api/direct_messages_controller_spec.rb create mode 100644 plugins/chat/spec/services/chat/create_direct_message_channel_spec.rb delete mode 100644 plugins/chat/spec/support/chat_helper.rb diff --git a/lib/user_comm_screener.rb b/lib/user_comm_screener.rb index 6593606dc8a..c1a451bf27a 100644 --- a/lib/user_comm_screener.rb +++ b/lib/user_comm_screener.rb @@ -180,6 +180,10 @@ class UserCommScreener actor_preferences[:disallowed_pms_from].include?(user_id) end + def actor_disallowing_any_pms?(user_ids) + user_ids.any? { |user_id| actor_disallowing_pms?(user_id) } + end + def actor_disallowing_all_pms? !acting_user.user_option.allow_private_messages end diff --git a/plugins/chat/app/controllers/chat/api/direct_messages_controller.rb b/plugins/chat/app/controllers/chat/api/direct_messages_controller.rb new file mode 100644 index 00000000000..c4fa7766286 --- /dev/null +++ b/plugins/chat/app/controllers/chat/api/direct_messages_controller.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +# TODO (martin) Remove this endpoint when we move to do the channel creation +# when a message is first sent to avoid double-request round trips for DMs. +class Chat::Api::DirectMessagesController < Chat::ApiController + def create + with_service(Chat::CreateDirectMessageChannel) do + on_success do + render_serialized( + result.channel, + Chat::ChannelSerializer, + root: "channel", + membership: result.membership, + ) + end + on_model_not_found(:target_users) { raise ActiveRecord::RecordNotFound } + on_failed_policy(:satisfies_dms_max_users_limit) do |policy| + raise Discourse::InvalidParameters.new(:target_usernames, policy.reason) + end + on_failed_policy(:actor_allows_dms) do + render_json_error(I18n.t("chat.errors.actor_disallowed_dms")) + end + on_failed_policy(:targets_allow_dms_from_user) { |policy| render_json_error(policy.reason) } + on_model_errors(:direct_message) do |model| + render_json_error(model, type: :record_invalid, status: 422) + end + on_model_errors(:channel) do |model| + render_json_error(model, type: :record_invalid, status: 422) + end + end + end +end diff --git a/plugins/chat/app/controllers/chat/direct_messages_controller.rb b/plugins/chat/app/controllers/chat/direct_messages_controller.rb index cb7d6981005..f20c647cfc5 100644 --- a/plugins/chat/app/controllers/chat/direct_messages_controller.rb +++ b/plugins/chat/app/controllers/chat/direct_messages_controller.rb @@ -2,26 +2,6 @@ module Chat class DirectMessagesController < ::Chat::BaseController - # NOTE: For V1 of chat channel archiving and deleting we are not doing - # anything for DM channels, their behaviour will stay as is. - def create - guardian.ensure_can_chat! - users = users_from_usernames(current_user, params) - - begin - chat_channel = - Chat::DirectMessageChannelCreator.create!(acting_user: current_user, target_users: users) - render_serialized( - chat_channel, - Chat::ChannelSerializer, - root: "channel", - membership: chat_channel.membership_for(current_user), - ) - rescue Chat::DirectMessageChannelCreator::NotAllowed => err - render_json_error(err.message) - end - end - def index guardian.ensure_can_chat! users = users_from_usernames(current_user, params) diff --git a/plugins/chat/app/policies/chat/direct_message_channel/can_communicate_all_parties_policy.rb b/plugins/chat/app/policies/chat/direct_message_channel/can_communicate_all_parties_policy.rb new file mode 100644 index 00000000000..76b7ef5a83f --- /dev/null +++ b/plugins/chat/app/policies/chat/direct_message_channel/can_communicate_all_parties_policy.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +class Chat::DirectMessageChannel::CanCommunicateAllPartiesPolicy < PolicyBase + delegate :target_users, :user_comm_screener, to: :context + + def call + acting_user_can_message_all_target_users? && + acting_user_not_preventing_messages_from_any_target_users? && + acting_user_not_ignoring_any_target_users? && acting_user_not_muting_any_target_users? + end + + def reason + if !acting_user_can_message_all_target_users? + I18n.t("chat.errors.not_accepting_dms", username: actor_cannot_message_user.username) + elsif !acting_user_not_preventing_messages_from_any_target_users? + I18n.t( + "chat.errors.actor_preventing_target_user_from_dm", + username: actor_disallowing_pm_user.username, + ) + elsif !acting_user_not_ignoring_any_target_users? + I18n.t("chat.errors.actor_ignoring_target_user", username: actor_ignoring_user.username) + elsif !acting_user_not_muting_any_target_users? + I18n.t("chat.errors.actor_muting_target_user", username: actor_muting_user.username) + end + end + + private + + def acting_user_can_message_all_target_users? + @acting_user_can_message_all_target_users ||= + user_comm_screener.preventing_actor_communication.none? + end + + def acting_user_not_preventing_messages_from_any_target_users? + @acting_user_not_preventing_messages_from_any_target_users ||= + !user_comm_screener.actor_disallowing_any_pms?(target_users_without_self.map(&:id)) + end + + def acting_user_not_ignoring_any_target_users? + @acting_user_not_ignoring_any_target_users ||= actor_ignoring_user.blank? + end + + def acting_user_not_muting_any_target_users? + @acting_user_not_muting_any_target_users ||= actor_muting_user.blank? + end + + def actor_cannot_message_user + target_users_without_self.find do |user| + user.id == user_comm_screener.preventing_actor_communication.first + end + end + + def actor_disallowing_pm_user + target_users_without_self.find do |target_user| + user_comm_screener.actor_disallowing_pms?(target_user.id) + end + end + + def actor_ignoring_user + target_users_without_self.find do |target_user| + user_comm_screener.actor_ignoring?(target_user.id) + end + end + + def actor_muting_user + target_users_without_self.find do |target_user| + user_comm_screener.actor_muting?(target_user.id) + end + end + + def target_users_without_self + @target_users_without_self ||= target_users.reject { |user| user.id == guardian.user.id } + end +end diff --git a/plugins/chat/app/policies/chat/direct_message_channel/max_users_excess_policy.rb b/plugins/chat/app/policies/chat/direct_message_channel/max_users_excess_policy.rb new file mode 100644 index 00000000000..86124ff2435 --- /dev/null +++ b/plugins/chat/app/policies/chat/direct_message_channel/max_users_excess_policy.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class Chat::DirectMessageChannel::MaxUsersExcessPolicy < PolicyBase + delegate :target_users, to: :context + + def call + guardian.is_staff? || + target_users_without_self.size <= SiteSetting.chat_max_direct_message_users + end + + def reason + return I18n.t("chat.errors.over_chat_max_direct_message_users_allow_self") if no_dm? + I18n.t( + "chat.errors.over_chat_max_direct_message_users", + count: SiteSetting.chat_max_direct_message_users + 1, # +1 for the acting user + ) + end + + private + + def no_dm? + SiteSetting.chat_max_direct_message_users.zero? + end + + def target_users_without_self + target_users.reject { |user| user.id == guardian.user.id } + end +end diff --git a/plugins/chat/app/services/chat/create_direct_message_channel.rb b/plugins/chat/app/services/chat/create_direct_message_channel.rb new file mode 100644 index 00000000000..d49fc7e6f5e --- /dev/null +++ b/plugins/chat/app/services/chat/create_direct_message_channel.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +module Chat + # Service responsible for creating a new direct message chat channel. + # The guardian passed in is the "acting user" when creating the channel + # and deciding whether the actor can communicate with the users that + # are passed in. + # + # @example + # Service::Chat::CreateDirectMessageChannel.call( + # guardian: guardian, + # target_usernames: ["bob", "alice"] + # ) + # + class CreateDirectMessageChannel + include Service::Base + + # @!method call(guardian:, **params_to_create) + # @param [Guardian] guardian + # @param [Hash] params_to_create + # @option params_to_create [Array] target_usernames + # @return [Service::Base::Context] + + policy :can_create_direct_message + contract + model :target_users + policy :satisfies_dms_max_users_limit, + class_name: Chat::DirectMessageChannel::MaxUsersExcessPolicy + model :user_comm_screener + policy :actor_allows_dms + policy :targets_allow_dms_from_user, + class_name: Chat::DirectMessageChannel::CanCommunicateAllPartiesPolicy + model :direct_message, :fetch_or_create_direct_message + model :channel, :fetch_or_create_channel + step :update_memberships + step :publish_channel + + # @!visibility private + class Contract + attribute :target_usernames, :array + validates :target_usernames, presence: true + end + + private + + def can_create_direct_message(guardian:, **) + guardian.can_create_direct_message? + end + + def fetch_target_users(guardian:, contract:, **) + User.where(username: [guardian.user.username, *contract.target_usernames]).to_a + end + + def fetch_user_comm_screener(target_users:, guardian:, **) + UserCommScreener.new(acting_user: guardian.user, target_user_ids: target_users.map(&:id)) + end + + def actor_allows_dms(user_comm_screener:, **) + !user_comm_screener.actor_disallowing_all_pms? + end + + def fetch_or_create_direct_message(target_users:, **) + Chat::DirectMessage.for_user_ids(target_users.map(&:id)) || + Chat::DirectMessage.create(user_ids: target_users.map(&:id)) + end + + def fetch_or_create_channel(direct_message:, **) + Chat::DirectMessageChannel.find_or_create_by(chatable: direct_message) + end + + def update_memberships(guardian:, channel:, target_users:, **) + always_level = Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:always] + + memberships = + target_users.map do |user| + { + user_id: user.id, + chat_channel_id: channel.id, + muted: false, + following: true, + desktop_notification_level: always_level, + mobile_notification_level: always_level, + created_at: Time.zone.now, + updated_at: Time.zone.now, + } + end + + Chat::UserChatChannelMembership.upsert_all( + memberships, + unique_by: %i[user_id chat_channel_id], + ) + end + + def publish_channel(channel:, target_users:, **) + Chat::Publisher.publish_new_channel(channel, target_users) + end + end +end diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel-selector-modal-inner.js b/plugins/chat/assets/javascripts/discourse/components/chat-channel-selector-modal-inner.js index 19cf7f873b0..bb904b0f1b9 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel-selector-modal-inner.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel-selector-modal-inner.js @@ -181,9 +181,9 @@ export default Component.extend({ @action fetchOrCreateChannelForUser(user) { - return ajax("/chat/direct_messages/create.json", { + return ajax("/chat/api/direct-message-channels.json", { method: "POST", - data: { usernames: [user.username] }, + data: { target_usernames: [user.username] }, }).catch(popupAjaxError); }, diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js index 24048aa574b..a676d67dd4e 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat.js @@ -380,9 +380,9 @@ export default class Chat extends Service { // channel for. The current user will automatically be included in the channel // when it is created. upsertDmChannelForUsernames(usernames) { - return ajax("/chat/direct_messages/create.json", { + return ajax("/chat/api/direct-message-channels.json", { method: "POST", - data: { usernames: usernames.uniq() }, + data: { target_usernames: usernames.uniq() }, }) .then((response) => { const channel = this.chatChannelsManager.store(response.channel); diff --git a/plugins/chat/config/routes.rb b/plugins/chat/config/routes.rb index eb3965132c3..96e2189bf75 100644 --- a/plugins/chat/config/routes.rb +++ b/plugins/chat/config/routes.rb @@ -35,6 +35,10 @@ Chat::Engine.routes.draw do put "/channels/:channel_id/threads/:thread_id/notifications-settings/me" => "channel_threads_current_user_notifications_settings#update" + # TODO (martin) Remove this when we refactor the DM channel creation to happen + # via message creation in a different API controller. + post "/direct-message-channels" => "direct_messages#create" + put "/channels/:channel_id/messages/:message_id/restore" => "channel_messages#restore" delete "/channels/:channel_id/messages/:message_id" => "channel_messages#destroy" diff --git a/plugins/chat/lib/chat/direct_message_channel_creator.rb b/plugins/chat/lib/chat/direct_message_channel_creator.rb deleted file mode 100644 index 9da6f35d327..00000000000 --- a/plugins/chat/lib/chat/direct_message_channel_creator.rb +++ /dev/null @@ -1,133 +0,0 @@ -# frozen_string_literal: true - -module Chat - class DirectMessageChannelCreator - class NotAllowed < StandardError - end - - def self.create!(acting_user:, target_users:) - Guardian.new(acting_user).ensure_can_create_direct_message! - target_users.uniq! - direct_message = Chat::DirectMessage.for_user_ids(target_users.map(&:id)) - - if direct_message - chat_channel = Chat::Channel.find_by!(chatable: direct_message) - else - enforce_max_direct_message_users!(acting_user, target_users) - ensure_actor_can_communicate!(acting_user, target_users) - direct_message = Chat::DirectMessage.create!(user_ids: target_users.map(&:id)) - chat_channel = direct_message.create_chat_channel! - end - - update_memberships(acting_user, target_users, chat_channel.id) - Chat::Publisher.publish_new_channel(chat_channel, target_users) - - chat_channel - end - - private - - def self.enforce_max_direct_message_users!(acting_user, target_users) - # We never want to prevent the actor from communicating with themselves - target_users = target_users.reject { |user| user.id == acting_user.id } - - if !acting_user.staff? && target_users.size > SiteSetting.chat_max_direct_message_users - if SiteSetting.chat_max_direct_message_users == 0 - raise NotAllowed.new(I18n.t("chat.errors.over_chat_max_direct_message_users_allow_self")) - else - raise NotAllowed.new( - I18n.t( - "chat.errors.over_chat_max_direct_message_users", - count: SiteSetting.chat_max_direct_message_users + 1, # +1 for the acting_user - ), - ) - end - end - end - - def self.update_memberships(acting_user, target_users, chat_channel_id) - sql_params = { - acting_user_id: acting_user.id, - user_ids: target_users.map(&:id), - chat_channel_id: chat_channel_id, - always_notification_level: Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:always], - } - - DB.exec(<<~SQL, sql_params) - INSERT INTO user_chat_channel_memberships( - user_id, - chat_channel_id, - muted, - following, - desktop_notification_level, - mobile_notification_level, - created_at, - updated_at - ) - VALUES( - unnest(array[:user_ids]), - :chat_channel_id, - false, - false, - :always_notification_level, - :always_notification_level, - NOW(), - NOW() - ) - ON CONFLICT (user_id, chat_channel_id) DO NOTHING; - - UPDATE user_chat_channel_memberships - SET following = true - WHERE user_id = :acting_user_id AND chat_channel_id = :chat_channel_id; - SQL - end - - def self.ensure_actor_can_communicate!(acting_user, target_users) - # We never want to prevent the actor from communicating with themselves - target_users = target_users.reject { |user| user.id == acting_user.id } - - screener = - UserCommScreener.new(acting_user: acting_user, target_user_ids: target_users.map(&:id)) - - # People blocking the actor. - screener.preventing_actor_communication.each do |user_id| - raise NotAllowed.new( - I18n.t( - "chat.errors.not_accepting_dms", - username: target_users.find { |user| user.id == user_id }.username, - ), - ) - end - - # The actor cannot start DMs with people if they are not allowing anyone - # to start DMs with them, that's no fair! - if screener.actor_disallowing_all_pms? - raise NotAllowed.new(I18n.t("chat.errors.actor_disallowed_dms")) - end - - # People the actor is blocking. - target_users.each do |target_user| - if screener.actor_disallowing_pms?(target_user.id) - raise NotAllowed.new( - I18n.t( - "chat.errors.actor_preventing_target_user_from_dm", - username: target_user.username, - ), - ) - end - - if screener.actor_ignoring?(target_user.id) - raise NotAllowed.new( - I18n.t("chat.errors.actor_ignoring_target_user", username: target_user.username), - ) - end - - if screener.actor_muting?(target_user.id) - raise NotAllowed.new( - I18n.t("chat.errors.actor_muting_target_user", username: target_user.username), - ) - end - end - end - end -end diff --git a/plugins/chat/spec/components/chat/mailer_spec.rb b/plugins/chat/spec/components/chat/mailer_spec.rb index cfc06500e2d..dec8390e20a 100644 --- a/plugins/chat/spec/components/chat/mailer_spec.rb +++ b/plugins/chat/spec/components/chat/mailer_spec.rb @@ -18,7 +18,13 @@ describe Chat::Mailer do end fab!(:private_chat_channel) do Group.refresh_automatic_groups! - Chat::DirectMessageChannelCreator.create!(acting_user: sender, target_users: [sender, user_1]) + result = + Chat::CreateDirectMessageChannel.call( + guardian: sender.guardian, + target_usernames: [sender.username, user_1.username], + ) + service_failed!(result) if result.failure? + result.channel end before do diff --git a/plugins/chat/spec/components/chat/message_creator_spec.rb b/plugins/chat/spec/components/chat/message_creator_spec.rb index 5701e71d938..dfaec6dd54d 100644 --- a/plugins/chat/spec/components/chat/message_creator_spec.rb +++ b/plugins/chat/spec/components/chat/message_creator_spec.rb @@ -32,7 +32,13 @@ describe Chat::MessageCreator do ) end let(:direct_message_channel) do - Chat::DirectMessageChannelCreator.create!(acting_user: user1, target_users: [user1, user2]) + result = + Chat::CreateDirectMessageChannel.call( + guardian: user1.guardian, + target_usernames: [user1.username, user2.username], + ) + service_failed!(result) if result.failure? + result.channel end before do diff --git a/plugins/chat/spec/components/chat/message_updater_spec.rb b/plugins/chat/spec/components/chat/message_updater_spec.rb index 3398394f49c..dd611f72d46 100644 --- a/plugins/chat/spec/components/chat/message_updater_spec.rb +++ b/plugins/chat/spec/components/chat/message_updater_spec.rb @@ -237,8 +237,13 @@ describe Chat::MessageUpdater do end it "doesn't create mention notification in direct message for users without access" do - direct_message_channel = - Chat::DirectMessageChannelCreator.create!(acting_user: user1, target_users: [user1, user2]) + result = + Chat::CreateDirectMessageChannel.call( + guardian: user1.guardian, + target_usernames: [user1.username, user2.username], + ) + service_failed!(result) if result.failure? + direct_message_channel = result.channel message = create_chat_message(user1, "ping nobody", direct_message_channel) Chat::MessageUpdater.update( diff --git a/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb b/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb index db432fbdcda..5f2c6f507d2 100644 --- a/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb +++ b/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb @@ -15,8 +15,15 @@ describe Jobs::Chat::NotifyMentioned do user_2.reload @chat_group = Fabricate(:group, users: [user_1, user_2]) - @personal_chat_channel = - Chat::DirectMessageChannelCreator.create!(acting_user: user_1, target_users: [user_1, user_2]) + result = + Chat::CreateDirectMessageChannel.call( + guardian: user_1.guardian, + target_usernames: [user_1.username, user_2.username], + ) + + service_failed!(result) if result.failure? + + @personal_chat_channel = result.channel [user_1, user_2].each do |u| Fabricate(:user_chat_channel_membership, chat_channel: public_channel, user: u) diff --git a/plugins/chat/spec/lib/chat/direct_message_channel_creator_spec.rb b/plugins/chat/spec/lib/chat/direct_message_channel_creator_spec.rb deleted file mode 100644 index dae249a2d52..00000000000 --- a/plugins/chat/spec/lib/chat/direct_message_channel_creator_spec.rb +++ /dev/null @@ -1,380 +0,0 @@ -# frozen_string_literal: true - -require "rails_helper" - -describe Chat::DirectMessageChannelCreator do - fab!(:user_1) { Fabricate(:user) } - fab!(:user_2) { Fabricate(:user) } - fab!(:user_3) { Fabricate(:user) } - - before { Group.refresh_automatic_groups! } - - context "with an existing direct message channel" do - fab!(:dm_chat_channel) do - Fabricate(:direct_message_channel, users: [user_1, user_2, user_3], with_membership: false) - end - fab!(:own_chat_channel) do - Fabricate(:direct_message_channel, users: [user_1], with_membership: false) - end - - it "doesn't create a new chat channel" do - existing_channel = nil - expect { - existing_channel = - described_class.create!(acting_user: user_1, target_users: [user_1, user_2, user_3]) - }.not_to change { Chat::Channel.count } - expect(existing_channel).to eq(dm_chat_channel) - end - - it "creates Chat::UserChatChannelMembership records and sets their notification levels, and only updates creator membership to following" do - Fabricate( - :user_chat_channel_membership, - user: user_2, - chat_channel: dm_chat_channel, - following: false, - muted: true, - desktop_notification_level: Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:never], - mobile_notification_level: Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:never], - ) - Fabricate( - :user_chat_channel_membership, - user: user_3, - chat_channel: dm_chat_channel, - following: false, - muted: true, - desktop_notification_level: Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:never], - mobile_notification_level: Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:never], - ) - - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2, user_3]) - }.to change { Chat::UserChatChannelMembership.count }.by(1) - - user_1_membership = - Chat::UserChatChannelMembership.find_by( - user_id: user_1.id, - chat_channel_id: dm_chat_channel, - ) - expect(user_1_membership.last_read_message_id).to eq(nil) - expect(user_1_membership.desktop_notification_level).to eq("always") - expect(user_1_membership.mobile_notification_level).to eq("always") - expect(user_1_membership.muted).to eq(false) - expect(user_1_membership.following).to eq(true) - - user_2_membership = - Chat::UserChatChannelMembership.find_by( - user_id: user_2.id, - chat_channel_id: dm_chat_channel, - ) - expect(user_2_membership.last_read_message_id).to eq(nil) - expect(user_2_membership.desktop_notification_level).to eq("never") - expect(user_2_membership.mobile_notification_level).to eq("never") - expect(user_2_membership.muted).to eq(true) - expect(user_2_membership.following).to eq(false) - - user_3_membership = - Chat::UserChatChannelMembership.find_by( - user_id: user_3.id, - chat_channel_id: dm_chat_channel, - ) - expect(user_3_membership.last_read_message_id).to eq(nil) - expect(user_3_membership.desktop_notification_level).to eq("never") - expect(user_3_membership.mobile_notification_level).to eq("never") - expect(user_3_membership.muted).to eq(true) - expect(user_3_membership.following).to eq(false) - end - - it "publishes the new DM channel message bus message for each user not following yet" do - messages = - MessageBus - .track_publish do - described_class.create!(acting_user: user_1, target_users: [user_1, user_2, user_3]) - end - .filter { |m| m.channel == "/chat/new-channel" } - - expect(messages.count).to eq(3) - expect(messages.first[:data]).to be_kind_of(Hash) - expect(messages.map { |m| m.dig(:data, :channel, :id) }).to eq( - [dm_chat_channel.id, dm_chat_channel.id, dm_chat_channel.id], - ) - end - - it "allows a user to create a direct message to themselves, without creating a new channel" do - existing_channel = nil - expect { - existing_channel = described_class.create!(acting_user: user_1, target_users: [user_1]) - }.to not_change { Chat::Channel.count }.and change { - Chat::UserChatChannelMembership.count - }.by(1) - expect(existing_channel).to eq(own_chat_channel) - end - - it "deduplicates target_users" do - existing_channel = nil - expect { - existing_channel = - described_class.create!(acting_user: user_1, target_users: [user_1, user_1]) - }.to not_change { Chat::Channel.count }.and change { - Chat::UserChatChannelMembership.count - }.by(1) - expect(existing_channel).to eq(own_chat_channel) - end - - context "when the user is not a member of direct_message_enabled_groups" do - before { SiteSetting.direct_message_enabled_groups = Group::AUTO_GROUPS[:trust_level_4] } - - it "raises an error and does not change membership or channel counts" do - channel_count = Chat::Channel.count - membership_count = Chat::UserChatChannelMembership.count - expect { - existing_channel = - described_class.create!(acting_user: user_1, target_users: [user_1, user_1]) - }.to raise_error(Discourse::InvalidAccess) - expect(Chat::Channel.count).to eq(channel_count) - expect(Chat::UserChatChannelMembership.count).to eq(membership_count) - end - - context "when user is staff" do - before { user_1.update!(admin: true) } - - it "doesn't create an error and returns the existing channel" do - existing_channel = nil - expect { - existing_channel = - described_class.create!(acting_user: user_1, target_users: [user_1, user_2, user_3]) - }.not_to change { Chat::Channel.count } - expect(existing_channel).to eq(dm_chat_channel) - end - end - end - end - - context "with non existing direct message channel" do - it "creates a new chat channel" do - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2]) - }.to change { Chat::Channel.count }.by(1) - end - - it "creates Chat::UserChatChannelMembership records and sets their notification levels" do - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2]) - }.to change { Chat::UserChatChannelMembership.count }.by(2) - - chat_channel = Chat::Channel.last - user_1_membership = - Chat::UserChatChannelMembership.find_by(user_id: user_1.id, chat_channel_id: chat_channel) - expect(user_1_membership.last_read_message_id).to eq(nil) - expect(user_1_membership.desktop_notification_level).to eq("always") - expect(user_1_membership.mobile_notification_level).to eq("always") - expect(user_1_membership.muted).to eq(false) - expect(user_1_membership.following).to eq(true) - end - - it "publishes the new DM channel message bus message for each user" do - messages = - MessageBus - .track_publish do - described_class.create!(acting_user: user_1, target_users: [user_1, user_2]) - end - .filter { |m| m.channel == "/chat/new-channel" } - - chat_channel = Chat::Channel.last - expect(messages.count).to eq(2) - expect(messages.first[:data]).to be_kind_of(Hash) - expect(messages.map { |m| m.dig(:data, :channel, :id) }).to eq( - [chat_channel.id, chat_channel.id], - ) - end - - it "allows a user to create a direct message to themselves" do - expect { described_class.create!(acting_user: user_1, target_users: [user_1]) }.to change { - Chat::Channel.count - }.by(1).and change { Chat::UserChatChannelMembership.count }.by(1) - end - - it "deduplicates target_users" do - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_1]) - }.to change { Chat::Channel.count }.by(1).and change { - Chat::UserChatChannelMembership.count - }.by(1) - end - - context "when number of users is over the limit" do - before { SiteSetting.chat_max_direct_message_users = 1 } - - it "raises an error" do - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2, user_3]) - }.to raise_error( - Chat::DirectMessageChannelCreator::NotAllowed, - I18n.t("chat.errors.over_chat_max_direct_message_users", count: 2), - ) - end - - context "when acting user is staff" do - fab!(:admin) { Fabricate(:admin) } - - it "creates a new chat channel" do - expect { - described_class.create!(acting_user: admin, target_users: [admin, user_1, user_2]) - }.to change { Chat::Channel.count }.by(1) - end - end - - context "when limit is zero" do - before { SiteSetting.chat_max_direct_message_users = 0 } - - it "raises an error" do - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2]) - }.to raise_error( - Chat::DirectMessageChannelCreator::NotAllowed, - I18n.t("chat.errors.over_chat_max_direct_message_users_allow_self"), - ) - end - end - end - - context "when number of users is at the limit" do - before { SiteSetting.chat_max_direct_message_users = 0 } - - it "creates a new chat channel" do - expect { described_class.create!(acting_user: user_1, target_users: [user_1]) }.to change { - Chat::Channel.count - }.by(1) - end - end - - context "when number of users is under the limit" do - before { SiteSetting.chat_max_direct_message_users = 1 } - - it "creates a new chat channel" do - expect { described_class.create!(acting_user: user_1, target_users: [user_1]) }.to change { - Chat::Channel.count - }.by(1) - end - end - - context "when the user is not a member of direct_message_enabled_groups" do - before { SiteSetting.direct_message_enabled_groups = Group::AUTO_GROUPS[:trust_level_4] } - - it "raises an error and does not change membership or channel counts" do - channel_count = Chat::Channel.count - membership_count = Chat::UserChatChannelMembership.count - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2]) - }.to raise_error(Discourse::InvalidAccess) - expect(Chat::Channel.count).to eq(channel_count) - expect(Chat::UserChatChannelMembership.count).to eq(membership_count) - end - - context "when user is staff" do - before { user_1.update!(admin: true) } - - it "creates a new chat channel" do - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2]) - }.to change { Chat::Channel.count }.by(1) - end - end - end - end - - describe "ignoring, muting, and preventing DMs from other users" do - context "when any of the users that the acting user is open in a DM with are ignoring the acting user" do - before do - Fabricate(:ignored_user, user: user_2, ignored_user: user_1, expiring_at: 1.day.from_now) - end - - it "raises an error with a helpful message" do - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2, user_3]) - }.to raise_error( - Chat::DirectMessageChannelCreator::NotAllowed, - I18n.t("chat.errors.not_accepting_dms", username: user_2.username), - ) - end - - it "does not let the ignoring user create a DM either and raises an error with a helpful message" do - expect { - described_class.create!(acting_user: user_2, target_users: [user_2, user_1, user_3]) - }.to raise_error( - Chat::DirectMessageChannelCreator::NotAllowed, - I18n.t("chat.errors.actor_ignoring_target_user", username: user_1.username), - ) - end - end - - context "when any of the users that the acting user is open in a DM with are muting the acting user" do - before { Fabricate(:muted_user, user: user_2, muted_user: user_1) } - - it "raises an error with a helpful message" do - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2, user_3]) - }.to raise_error( - Chat::DirectMessageChannelCreator::NotAllowed, - I18n.t("chat.errors.not_accepting_dms", username: user_2.username), - ) - end - - it "does not let the muting user create a DM either and raises an error with a helpful message" do - expect { - described_class.create!(acting_user: user_2, target_users: [user_2, user_1, user_3]) - }.to raise_error( - Chat::DirectMessageChannelCreator::NotAllowed, - I18n.t("chat.errors.actor_muting_target_user", username: user_1.username), - ) - end - end - - context "when any of the users that the acting user is open in a DM with is preventing private/direct messages" do - before { user_2.user_option.update(allow_private_messages: false) } - - it "raises an error with a helpful message" do - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2, user_3]) - }.to raise_error( - Chat::DirectMessageChannelCreator::NotAllowed, - I18n.t("chat.errors.not_accepting_dms", username: user_2.username), - ) - end - - it "does not let the user who is preventing PM/DM create a DM either and raises an error with a helpful message" do - expect { - described_class.create!(acting_user: user_2, target_users: [user_2, user_1, user_3]) - }.to raise_error( - Chat::DirectMessageChannelCreator::NotAllowed, - I18n.t("chat.errors.actor_disallowed_dms"), - ) - end - end - - context "when any of the users that the acting user is open in a DM with only allow private/direct messages from certain users" do - before { user_2.user_option.update!(enable_allowed_pm_users: true) } - - it "raises an error with a helpful message" do - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2, user_3]) - }.to raise_error(Chat::DirectMessageChannelCreator::NotAllowed) - end - - it "does not raise an error if the acting user is allowed to send the PM" do - AllowedPmUser.create!(user: user_2, allowed_pm_user: user_1) - expect { - described_class.create!(acting_user: user_1, target_users: [user_1, user_2, user_3]) - }.to change { Chat::Channel.count }.by(1) - end - - it "does not let the user who is preventing PM/DM create a DM either and raises an error with a helpful message" do - expect { - described_class.create!(acting_user: user_2, target_users: [user_2, user_1, user_3]) - }.to raise_error( - Chat::DirectMessageChannelCreator::NotAllowed, - I18n.t("chat.errors.actor_preventing_target_user_from_dm", username: user_1.username), - ) - end - end - end -end diff --git a/plugins/chat/spec/lib/chat/notifier_spec.rb b/plugins/chat/spec/lib/chat/notifier_spec.rb index 48ee215777c..f892bcca6a4 100644 --- a/plugins/chat/spec/lib/chat/notifier_spec.rb +++ b/plugins/chat/spec/lib/chat/notifier_spec.rb @@ -408,10 +408,13 @@ describe Chat::Notifier do context "when in a personal message" do let(:personal_chat_channel) do Group.refresh_automatic_groups! - Chat::DirectMessageChannelCreator.create!( - acting_user: user_1, - target_users: [user_1, user_2], - ) + result = + Chat::CreateDirectMessageChannel.call( + guardian: user_1.guardian, + target_usernames: [user_1.username, user_2.username], + ) + service_failed!(result) if result.failure? + result.channel end before { @chat_group.add(user_3) } diff --git a/plugins/chat/spec/mailers/user_notifications_spec.rb b/plugins/chat/spec/mailers/user_notifications_spec.rb index 9ef6eca5e30..734687c81c1 100644 --- a/plugins/chat/spec/mailers/user_notifications_spec.rb +++ b/plugins/chat/spec/mailers/user_notifications_spec.rb @@ -22,7 +22,7 @@ describe UserNotifications do context "with private channel" do fab!(:channel) do refresh_auto_groups - Chat::DirectMessageChannelCreator.create!(acting_user: sender, target_users: [sender, user]) + create_dm_channel(sender, [sender, user]) end it "calls guardian can_join_chat_channel?" do @@ -76,11 +76,7 @@ describe UserNotifications do another_dm_user = Fabricate(:user, group_ids: [chatters_group.id]) refresh_auto_groups another_dm_user.reload - another_channel = - Chat::DirectMessageChannelCreator.create!( - acting_user: user, - target_users: [another_dm_user, user], - ) + another_channel = create_dm_channel(user, [another_dm_user, user]) Fabricate(:chat_message, user: another_dm_user, chat_channel: another_channel) Fabricate(:chat_message, user: sender, chat_channel: channel) email = described_class.chat_summary(user, {}) @@ -107,11 +103,8 @@ describe UserNotifications do refresh_auto_groups sender.reload senders << sender - channel = - Chat::DirectMessageChannelCreator.create!( - acting_user: sender, - target_users: [user, sender], - ) + channel = create_dm_channel(sender, [sender, user]) + user .user_chat_channel_memberships .where(chat_channel_id: channel.id) @@ -337,11 +330,7 @@ describe UserNotifications do context "with both unread DM messages and mentions" do before do refresh_auto_groups - channel = - Chat::DirectMessageChannelCreator.create!( - acting_user: sender, - target_users: [sender, user], - ) + channel = create_dm_channel(sender, [sender, user]) Fabricate(:chat_message, user: sender, chat_channel: channel) notification = Fabricate(:notification) Fabricate( @@ -462,11 +451,7 @@ describe UserNotifications do it "returns an email when the user has unread private messages" do user_membership.update!(last_read_message_id: chat_message.id) refresh_auto_groups - channel = - Chat::DirectMessageChannelCreator.create!( - acting_user: sender, - target_users: [sender, user], - ) + channel = create_dm_channel(sender, [sender, user]) Fabricate(:chat_message, user: sender, chat_channel: channel) email = described_class.chat_summary(user, {}) @@ -628,4 +613,14 @@ describe UserNotifications do end end end + + def create_dm_channel(sender, target_users) + result = + Chat::CreateDirectMessageChannel.call( + guardian: sender.guardian, + target_usernames: target_users.map(&:username), + ) + service_failed!(result) if result.failure? + result.channel + end end diff --git a/plugins/chat/spec/plugin_helper.rb b/plugins/chat/spec/plugin_helper.rb index 378ff17921d..657646b905c 100644 --- a/plugins/chat/spec/plugin_helper.rb +++ b/plugins/chat/spec/plugin_helper.rb @@ -61,8 +61,18 @@ module ChatSystemHelpers end end +module ChatSpecHelpers + def service_failed!(result) + raise RSpec::Expectations::ExpectationNotMetError.new( + "Service failed, see below for step details:\n\n" + result.inspect_steps.inspect, + ) + end +end + RSpec.configure do |config| config.include ChatSystemHelpers, type: :system + config.include ChatSpecHelpers + config.include Chat::WithServiceHelper config.include Chat::ServiceMatchers config.expect_with :rspec do |c| diff --git a/plugins/chat/spec/requests/chat/api/direct_messages_controller_spec.rb b/plugins/chat/spec/requests/chat/api/direct_messages_controller_spec.rb new file mode 100644 index 00000000000..b3458d47c23 --- /dev/null +++ b/plugins/chat/spec/requests/chat/api/direct_messages_controller_spec.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +RSpec.describe Chat::Api::DirectMessagesController do + fab!(:current_user) { Fabricate(:user) } + fab!(:user1) { Fabricate(:user) } + fab!(:user2) { Fabricate(:user) } + fab!(:user3) { Fabricate(:user) } + + before do + SiteSetting.chat_enabled = true + SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone] + sign_in(current_user) + end + + def create_dm_channel(user_ids) + direct_messages_channel = Chat::DirectMessage.create! + user_ids.each do |user_id| + direct_messages_channel.direct_message_users.create!(user_id: user_id) + end + Chat::DirectMessageChannel.create!(chatable: direct_messages_channel) + end + + describe "#create" do + before { Group.refresh_automatic_groups! } + + shared_examples "creating dms" do + it "creates a new dm channel with username(s) provided" do + expect { + post "/chat/api/direct-message-channels.json", params: { target_usernames: [usernames] } + }.to change { Chat::DirectMessage.count }.by(1) + expect(Chat::DirectMessage.last.direct_message_users.map(&:user_id)).to match_array( + direct_message_user_ids, + ) + end + + it "returns existing dm channel if one exists for username(s)" do + create_dm_channel(direct_message_user_ids) + expect { + post "/chat/api/direct-message-channels.json", params: { target_usernames: [usernames] } + }.not_to change { Chat::DirectMessage.count } + end + end + + describe "dm with one other user" do + let(:usernames) { user1.username } + let(:direct_message_user_ids) { [current_user.id, user1.id] } + + include_examples "creating dms" + end + + describe "dm with myself" do + let(:usernames) { [current_user.username] } + let(:direct_message_user_ids) { [current_user.id] } + + include_examples "creating dms" + end + + describe "dm with two other users" do + let(:usernames) { [user1, user2, user3].map(&:username) } + let(:direct_message_user_ids) { [current_user.id, user1.id, user2.id, user3.id] } + + include_examples "creating dms" + end + + it "creates Chat::UserChatChannelMembership records" do + users = [user2, user3] + usernames = users.map(&:username) + expect { + post "/chat/api/direct-message-channels.json", params: { target_usernames: usernames } + }.to change { Chat::UserChatChannelMembership.count }.by(3) + end + + context "when one of the users I am messaging has ignored, muted, or prevented DMs from the acting user creating the channel" do + let(:usernames) { [user1, user2, user3].map(&:username) } + let(:direct_message_user_ids) { [current_user.id, user1.id, user2.id, user3.id] } + + shared_examples "creating dms with communication error" do + it "responds with a friendly error" do + expect { + post "/chat/api/direct-message-channels.json", params: { target_usernames: [usernames] } + }.not_to change { Chat::DirectMessage.count } + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to eq( + [I18n.t("chat.errors.not_accepting_dms", username: user1.username)], + ) + end + end + + describe "user ignoring the actor" do + before do + Fabricate( + :ignored_user, + user: user1, + ignored_user: current_user, + expiring_at: 1.day.from_now, + ) + end + + include_examples "creating dms with communication error" + end + + describe "user muting the actor" do + before { Fabricate(:muted_user, user: user1, muted_user: current_user) } + + include_examples "creating dms with communication error" + end + + describe "user preventing all DMs" do + before { user1.user_option.update(allow_private_messages: false) } + + include_examples "creating dms with communication error" + end + + describe "user only allowing DMs from certain users" do + before { user1.user_option.update(enable_allowed_pm_users: true) } + + include_examples "creating dms with communication error" + end + end + end +end diff --git a/plugins/chat/spec/requests/direct_messages_controller_spec.rb b/plugins/chat/spec/requests/direct_messages_controller_spec.rb index 73a0f40eefc..3c9017bc202 100644 --- a/plugins/chat/spec/requests/direct_messages_controller_spec.rb +++ b/plugins/chat/spec/requests/direct_messages_controller_spec.rb @@ -68,98 +68,4 @@ RSpec.describe Chat::DirectMessagesController do end end end - - describe "#create" do - before { Group.refresh_automatic_groups! } - - shared_examples "creating dms" do - it "creates a new dm channel with username(s) provided" do - expect { - post "/chat/direct_messages/create.json", params: { usernames: [usernames] } - }.to change { Chat::DirectMessage.count }.by(1) - expect(Chat::DirectMessage.last.direct_message_users.map(&:user_id)).to match_array( - direct_message_user_ids, - ) - end - - it "returns existing dm channel if one exists for username(s)" do - create_dm_channel(direct_message_user_ids) - expect { - post "/chat/direct_messages/create.json", params: { usernames: [usernames] } - }.not_to change { Chat::DirectMessage.count } - end - end - - describe "dm with one other user" do - let(:usernames) { user1.username } - let(:direct_message_user_ids) { [user.id, user1.id] } - - include_examples "creating dms" - end - - describe "dm with myself" do - let(:usernames) { [user.username] } - let(:direct_message_user_ids) { [user.id] } - - include_examples "creating dms" - end - - describe "dm with two other users" do - let(:usernames) { [user1, user2, user3].map(&:username) } - let(:direct_message_user_ids) { [user.id, user1.id, user2.id, user3.id] } - - include_examples "creating dms" - end - - it "creates Chat::UserChatChannelMembership records" do - users = [user2, user3] - usernames = users.map(&:username) - expect { - post "/chat/direct_messages/create.json", params: { usernames: usernames } - }.to change { Chat::UserChatChannelMembership.count }.by(3) - end - - context "when one of the users I am messaging has ignored, muted, or prevented DMs from the acting user creating the channel" do - let(:usernames) { [user1, user2, user3].map(&:username) } - let(:direct_message_user_ids) { [user.id, user1.id, user2.id, user3.id] } - - shared_examples "creating dms with communication error" do - it "responds with a friendly error" do - expect { - post "/chat/direct_messages/create.json", params: { usernames: [usernames] } - }.not_to change { Chat::DirectMessage.count } - expect(response.status).to eq(422) - expect(response.parsed_body["errors"]).to eq( - [I18n.t("chat.errors.not_accepting_dms", username: user1.username)], - ) - end - end - - describe "user ignoring the actor" do - before do - Fabricate(:ignored_user, user: user1, ignored_user: user, expiring_at: 1.day.from_now) - end - - include_examples "creating dms with communication error" - end - - describe "user muting the actor" do - before { Fabricate(:muted_user, user: user1, muted_user: user) } - - include_examples "creating dms with communication error" - end - - describe "user preventing all DMs" do - before { user1.user_option.update(allow_private_messages: false) } - - include_examples "creating dms with communication error" - end - - describe "user only allowing DMs from certain users" do - before { user1.user_option.update(enable_allowed_pm_users: true) } - - include_examples "creating dms with communication error" - end - end - end end diff --git a/plugins/chat/spec/services/chat/create_direct_message_channel_spec.rb b/plugins/chat/spec/services/chat/create_direct_message_channel_spec.rb new file mode 100644 index 00000000000..48e99a0fbe4 --- /dev/null +++ b/plugins/chat/spec/services/chat/create_direct_message_channel_spec.rb @@ -0,0 +1,150 @@ +# frozen_string_literal: true + +RSpec.describe Chat::CreateDirectMessageChannel do + describe Chat::CreateDirectMessageChannel::Contract, type: :model do + subject(:contract) { described_class.new(params) } + + let(:params) { { target_usernames: %w[lechuck elaine] } } + + it { is_expected.to validate_presence_of :target_usernames } + + context "when the target_usernames argument is a string" do + let(:params) { { target_usernames: "lechuck,elaine" } } + + it "splits it into an array" do + contract.validate + expect(contract.target_usernames).to eq(%w[lechuck elaine]) + end + end + end + + describe ".call" do + subject(:result) { described_class.call(params) } + + fab!(:current_user) { Fabricate(:user, username: "guybrush") } + fab!(:user_1) { Fabricate(:user, username: "lechuck") } + fab!(:user_2) { Fabricate(:user, username: "elaine") } + + let(:guardian) { Guardian.new(current_user) } + let(:params) { { guardian: guardian, target_usernames: %w[lechuck elaine] } } + + before { Group.refresh_automatic_groups! } + + context "when all steps pass" do + it "sets the service result as successful" do + expect(result).to be_a_success + end + + it "creates the channel" do + expect { result }.to change { Chat::Channel.count } + expect(result.channel.chatable).to have_attributes( + user_ids: match_array([current_user.id, user_1.id, user_2.id]), + ) + end + + it "makes all target users a member of the channel and updates all users to following" do + expect { result }.to change { Chat::UserChatChannelMembership.count }.by(3) + expect(result.channel.user_chat_channel_memberships.pluck(:user_id)).to match_array( + [current_user.id, user_1.id, user_2.id], + ) + result.channel.user_chat_channel_memberships.each do |membership| + expect(membership).to have_attributes( + following: true, + muted: false, + desktop_notification_level: "always", + mobile_notification_level: "always", + ) + end + end + + it "publishes the new channel" do + messages = + MessageBus.track_publish(Chat::Publisher::NEW_CHANNEL_MESSAGE_BUS_CHANNEL) { result } + expect(messages.first.data[:channel][:title]).to eq("@elaine, @lechuck") + end + + context "when there is an existing direct message channel for the target users" do + before { described_class.call(params) } + + it "does not create a channel" do + expect { result }.to not_change { Chat::Channel.count }.and not_change { + Chat::DirectMessage.count + } + end + + it "does not double-insert the channel memberships" do + expect { result }.not_to change { Chat::UserChatChannelMembership.count } + end + end + end + + context "when the current user cannot make direct messages" do + fab!(:current_user) { Fabricate(:user) } + + before { SiteSetting.direct_message_enabled_groups = Fabricate(:group).id } + + it { is_expected.to fail_a_policy(:can_create_direct_message) } + end + + context "when the number of target users exceeds chat_max_direct_message_users" do + before { SiteSetting.chat_max_direct_message_users = 1 } + + it { is_expected.to fail_a_policy(:satisfies_dms_max_users_limit) } + + context "when the user is staff" do + fab!(:current_user) { Fabricate(:admin) } + + it { is_expected.not_to fail_a_policy(:satisfies_dms_max_users_limit) } + end + end + + context "when the actor is not allowing anyone to message them" do + before { current_user.user_option.update!(allow_private_messages: false) } + + it { is_expected.to fail_a_policy(:actor_allows_dms) } + end + + context "when one of the target users is ignoring the current user" do + before do + IgnoredUser.create!(user: user_1, ignored_user: current_user, expiring_at: 1.day.from_now) + end + + it { is_expected.to fail_a_policy(:targets_allow_dms_from_user) } + end + + context "when one of the target users is muting the current user" do + before { MutedUser.create!(user: user_1, muted_user: current_user) } + + it { is_expected.to fail_a_policy(:targets_allow_dms_from_user) } + end + + context "when one of the target users is disallowing messages" do + before { user_1.user_option.update!(allow_private_messages: false) } + + it { is_expected.to fail_a_policy(:targets_allow_dms_from_user) } + end + + context "when the current user is allowing messages from all but one of the target users" do + before do + current_user.user_option.update!(enable_allowed_pm_users: true) + AllowedPmUser.create!(user: current_user, allowed_pm_user: user_2) + end + + it { is_expected.to fail_a_policy(:targets_allow_dms_from_user) } + end + + context "when the current user is ignoring one of the target users" do + before do + IgnoredUser.create!(user: current_user, ignored_user: user_1, expiring_at: 1.day.from_now) + end + + it { is_expected.to fail_a_policy(:targets_allow_dms_from_user) } + end + + context "when the current user is muting one of the target users" do + before { MutedUser.create!(user: current_user, muted_user: user_1) } + + it { is_expected.to fail_a_policy(:targets_allow_dms_from_user) } + end + end +end diff --git a/plugins/chat/spec/support/chat_helper.rb b/plugins/chat/spec/support/chat_helper.rb deleted file mode 100644 index a1c4f51f2c8..00000000000 --- a/plugins/chat/spec/support/chat_helper.rb +++ /dev/null @@ -1,19 +0,0 @@ -# frozen_string_literal: true - -module ChatHelper - def self.make_messages!(chatable, users, count) - users = [users] unless Array === users - raise ArgumentError if users.length <= 0 - - chatable = Fabricate(:category) unless chatable - chat_channel = Fabricate(:chat_channel, chatable: chatable) - - count.times do |n| - Chat::Message.new( - chat_channel: chat_channel, - user: users[n % users.length], - message: "Chat message for test #{n}", - ).save! - end - end -end diff --git a/plugins/chat/spec/system/message_thread_indicator_spec.rb b/plugins/chat/spec/system/message_thread_indicator_spec.rb index 27fc5aa367a..3c41cabae09 100644 --- a/plugins/chat/spec/system/message_thread_indicator_spec.rb +++ b/plugins/chat/spec/system/message_thread_indicator_spec.rb @@ -131,12 +131,9 @@ describe "Thread indicator for chat messages", type: :system do thread_1.last_reply.rebake! chat_page.visit_channel(channel) - - excerpt_text = thread_excerpt(thread_1.last_reply) - expect( channel_page.message_thread_indicator(thread_1.original_message).excerpt, - ).to have_content(excerpt_text) + ).to have_content(thread_excerpt(thread_1.last_reply)) end it "updates the last reply excerpt and participants when a new message is added to the thread" do