DEV: Refactor DM channel creation into new service pattern (#22144)

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 <loic@discourse.org>

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

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

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)

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

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

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<String>] 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

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);
   },
 

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);

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"
 

plugins/chat/lib/chat/direct_message_channel_creator.rb

@@ -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

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

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

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 =
+      result =
-        Chat::DirectMessageChannelCreator.create!(acting_user: user1, target_users: [user1, user2])
+        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(

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 =
+    result =
-      Chat::DirectMessageChannelCreator.create!(acting_user: user_1, target_users: [user_1, user_2])
+      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)

plugins/chat/spec/lib/chat/direct_message_channel_creator_spec.rb

@@ -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

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!(
+          result =
-            acting_user: user_1,
+            Chat::CreateDirectMessageChannel.call(
-            target_users: [user_1, user_2],
+              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) }

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 =
+          another_channel = create_dm_channel(user, [another_dm_user, user])
-            Chat::DirectMessageChannelCreator.create!(
-              acting_user: user,
-              target_users: [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 =
+            channel = create_dm_channel(sender, [sender, user])
-              Chat::DirectMessageChannelCreator.create!(
+
-                acting_user: sender,
-                target_users: [user, sender],
-              )
             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 =
+            channel = create_dm_channel(sender, [sender, user])
-              Chat::DirectMessageChannelCreator.create!(
-                acting_user: sender,
-                target_users: [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 =
+            channel = create_dm_channel(sender, [sender, user])
-              Chat::DirectMessageChannelCreator.create!(
-                acting_user: sender,
-                target_users: [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

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|

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

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

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

plugins/chat/spec/support/chat_helper.rb

@@ -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

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