From 4e80c9eb13ddb3d3661d919dbfadc44a71e2215d Mon Sep 17 00:00:00 2001
From: David Battersby <info@davidbattersby.com>
Date: Mon, 3 Jun 2024 12:11:49 +0400
Subject: [PATCH] FIX: chat direct message group user limit is off by 1
 (#27014)

This change allows the correct number of members to be added when creating a group direct message, based on the site setting chat_max_direct_message_users.

Previously we counted the current user within the max user limit and therefore the count was off by 1.
---
 .../api/channels_memberships_controller.rb    |  3 +++
 .../app/services/chat/add_users_to_channel.rb | 21 +++++++------------
 .../chat/message-creator/add-members.gjs      | 14 ++++++-------
 .../chat/message-creator/new-group.gjs        | 11 +++++-----
 .../chat/add_users_to_channel_spec.rb         | 14 +++++++------
 .../spec/system/chat_message_creator_spec.rb  |  7 ++++---
 6 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/plugins/chat/app/controllers/chat/api/channels_memberships_controller.rb b/plugins/chat/app/controllers/chat/api/channels_memberships_controller.rb
index 14ece64edcf..0a805df1a53 100644
--- a/plugins/chat/app/controllers/chat/api/channels_memberships_controller.rb
+++ b/plugins/chat/app/controllers/chat/api/channels_memberships_controller.rb
@@ -36,6 +36,9 @@ class Chat::Api::ChannelsMembershipsController < Chat::Api::ChannelsController
       on_failed_policy(:can_add_users_to_channel) do
         render_json_error(I18n.t("chat.errors.users_cant_be_added_to_channel"))
       end
+      on_failed_policy(:satisfies_dms_max_users_limit) do |policy|
+        render_json_dump({ error: policy.reason }, status: 400)
+      end
       on_failed_contract do |contract|
         render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
       end
diff --git a/plugins/chat/app/services/chat/add_users_to_channel.rb b/plugins/chat/app/services/chat/add_users_to_channel.rb
index 3785d1e62ff..b6ad6e4719b 100644
--- a/plugins/chat/app/services/chat/add_users_to_channel.rb
+++ b/plugins/chat/app/services/chat/add_users_to_channel.rb
@@ -25,10 +25,11 @@ module Chat
     contract
     model :channel
     policy :can_add_users_to_channel
-    model :users, optional: true
+    model :target_users, optional: true
+    policy :satisfies_dms_max_users_limit,
+           class_name: Chat::DirectMessageChannel::MaxUsersExcessPolicy
 
     transaction do
-      step :validate_user_count
       step :upsert_memberships
       step :recompute_users_count
       step :notice_channel
@@ -56,7 +57,7 @@ module Chat
         channel.direct_message_channel? && channel.chatable.group
     end
 
-    def fetch_users(contract:, channel:)
+    def fetch_target_users(contract:, channel:)
       ::Chat::UsersFromUsernamesAndGroupsQuery.call(
         usernames: contract.usernames,
         groups: contract.groups,
@@ -68,17 +69,11 @@ module Chat
       ::Chat::Channel.includes(:chatable).find_by(id: contract.channel_id)
     end
 
-    def validate_user_count(channel:, users:)
-      if channel.user_count + users.length > SiteSetting.chat_max_direct_message_users
-        fail!("should have less than #{SiteSetting.chat_max_direct_message_users} elements")
-      end
-    end
-
-    def upsert_memberships(channel:, users:)
+    def upsert_memberships(channel:, target_users:)
       always_level = ::Chat::UserChatChannelMembership::NOTIFICATION_LEVELS[:always]
 
       memberships =
-        users.map do |user|
+        target_users.map do |user|
           {
             user_id: user.id,
             chat_channel_id: channel.id,
@@ -128,8 +123,8 @@ module Chat
       )
     end
 
-    def notice_channel(guardian:, channel:, users:)
-      added_users = users.select { |u| context.added_user_ids.include?(u.id) }
+    def notice_channel(guardian:, channel:, target_users:)
+      added_users = target_users.select { |u| context.added_user_ids.include?(u.id) }
 
       return if added_users.blank?
 
diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/add-members.gjs b/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/add-members.gjs
index 2448d04261c..2512196516d 100644
--- a/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/add-members.gjs
+++ b/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/add-members.gjs
@@ -27,6 +27,10 @@ export default class AddMembers extends Component {
     return userCount + (this.args.channel?.membershipsCount ?? 0);
   }
 
+  get maxMembers() {
+    return this.siteSettings.chat_max_direct_message_users;
+  }
+
   @action
   async saveGroupMembers() {
     try {
@@ -60,10 +64,7 @@ export default class AddMembers extends Component {
   <template>
     <div class="chat-message-creator__add-members-container">
       <div class="chat-message-creator__add-members">
-        <MembersCount
-          @count={{this.membersCount}}
-          @max={{this.siteSettings.chat_max_direct_message_users}}
-        />
+        <MembersCount @count={{this.membersCount}} @max={{this.maxMembers}} />
 
         <MembersSelector
           @channel={{@channel}}
@@ -72,10 +73,7 @@ export default class AddMembers extends Component {
           @close={{@close}}
           @cancel={{@cancel}}
           @membersCount={{this.membersCount}}
-          @maxReached={{gte
-            this.membersCount
-            this.siteSettings.chat_max_direct_message_users
-          }}
+          @maxReached={{gte this.membersCount this.maxMembers}}
         />
 
         {{#if @members.length}}
diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/new-group.gjs b/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/new-group.gjs
index b3d4fba9e61..91074d836ca 100644
--- a/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/new-group.gjs
+++ b/plugins/chat/assets/javascripts/discourse/components/chat/message-creator/new-group.gjs
@@ -26,7 +26,11 @@ export default class NewGroup extends Component {
       } else {
         return acc + 1;
       }
-    }, 1);
+    }, 0);
+  }
+
+  get maxMembers() {
+    return this.siteSettings.chat_max_direct_message_users;
   }
 
   @action
@@ -82,10 +86,7 @@ export default class NewGroup extends Component {
           @close={{@close}}
           @cancel={{@cancel}}
           @membersCount={{this.membersCount}}
-          @maxReached={{gte
-            this.membersCount
-            this.siteSettings.chat_max_direct_message_users
-          }}
+          @maxReached={{gte this.membersCount this.maxMembers}}
         />
 
         {{#if @members.length}}
diff --git a/plugins/chat/spec/services/chat/add_users_to_channel_spec.rb b/plugins/chat/spec/services/chat/add_users_to_channel_spec.rb
index 1421095977a..3914f37943b 100644
--- a/plugins/chat/spec/services/chat/add_users_to_channel_spec.rb
+++ b/plugins/chat/spec/services/chat/add_users_to_channel_spec.rb
@@ -29,12 +29,12 @@ RSpec.describe Chat::AddUsersToChannel do
       before { channel.add(current_user) }
 
       it "fetches users to add" do
-        expect(result.users.map(&:username)).to contain_exactly(*users.map(&:username))
+        expect(result.target_users.map(&:username)).to contain_exactly(*users.map(&:username))
       end
 
       it "includes users from groups" do
         params.merge!(groups: [group.name])
-        expect(result.users.map(&:username)).to include(
+        expect(result.target_users.map(&:username)).to include(
           group_user_1.username,
           group_user_2.username,
         )
@@ -59,7 +59,9 @@ RSpec.describe Chat::AddUsersToChannel do
       it "doesn't include existing direct message users" do
         Chat::DirectMessageUser.create!(user: users.first, direct_message: direct_message)
 
-        expect(result.users.map(&:username)).to contain_exactly(*users[1..-1].map(&:username))
+        expect(result.target_users.map(&:username)).to contain_exactly(
+          *users[1..-1].map(&:username),
+        )
       end
 
       it "creates memberships" do
@@ -77,7 +79,7 @@ RSpec.describe Chat::AddUsersToChannel do
       end
 
       it "creates a chat message to show added users" do
-        added_users = result.users
+        added_users = result.target_users
 
         channel.chat_messages.last.tap do |message|
           expect(message.message).to eq(
@@ -93,10 +95,10 @@ RSpec.describe Chat::AddUsersToChannel do
       end
     end
 
-    context "when usernames exceeds chat_max_direct_message_users" do
+    context "when users exceed max direct message user limit" do
       before { SiteSetting.chat_max_direct_message_users = 4 }
 
-      it { is_expected.to fail_a_step(:validate_user_count) }
+      it { is_expected.to fail_a_policy(:satisfies_dms_max_users_limit) }
     end
 
     context "when channel is not found" do
diff --git a/plugins/chat/spec/system/chat_message_creator_spec.rb b/plugins/chat/spec/system/chat_message_creator_spec.rb
index 5d2fc1027ed..ab88a9e391f 100644
--- a/plugins/chat/spec/system/chat_message_creator_spec.rb
+++ b/plugins/chat/spec/system/chat_message_creator_spec.rb
@@ -111,15 +111,16 @@ RSpec.describe "Flag message", type: :system do
     user_1 = Fabricate(:user)
     user_2 = Fabricate(:user)
     user_3 = Fabricate(:user)
-    group = Fabricate(:public_group, users: [user_1, user_2])
+    user_4 = Fabricate(:user)
+    group = Fabricate(:public_group, users: [user_1, user_2, user_3])
 
     visit("/")
     chat_page.prefers_full_page
     chat_page.open_new_message
     chat_page.find("#new-group-chat").click
     chat_page.find(".chat-message-creator__new-group-header__input").fill_in(with: "hamsters")
-    chat_page.find(".chat-message-creator__members-input").fill_in(with: user_3.username)
-    chat_page.message_creator.click_row(user_3)
+    chat_page.find(".chat-message-creator__members-input").fill_in(with: user_4.username)
+    chat_page.message_creator.click_row(user_4)
     chat_page.find(".chat-message-creator__members-input").fill_in(with: group.name)
     chat_page.message_creator.click_row(group)