From c39a4de139ed9bba4696d44b15cb7569038b5fde Mon Sep 17 00:00:00 2001 From: David Battersby Date: Fri, 24 May 2024 12:12:49 +0400 Subject: [PATCH] FIX: load existing chat dm channel via url (#26998) When users click a link that points to an existing group chat, we should reopen that chat instead of creating a new group chat so users can more easily continue ongoing conversations. --- .../chat/app/models/chat/direct_message.rb | 4 +-- .../chat/create_direct_message_channel.rb | 10 ++++-- .../chat/message-creator/new-group.gjs | 4 +-- .../javascripts/discourse/services/chat.js | 13 ++++++-- .../api/direct_messages_controller_spec.rb | 18 +++++++++-- .../chat/spec/system/chat_new_message_spec.rb | 32 +++++++++++++++++-- 6 files changed, 65 insertions(+), 16 deletions(-) diff --git a/plugins/chat/app/models/chat/direct_message.rb b/plugins/chat/app/models/chat/direct_message.rb index c45038e1ff8..30f4797eb66 100644 --- a/plugins/chat/app/models/chat/direct_message.rb +++ b/plugins/chat/app/models/chat/direct_message.rb @@ -17,9 +17,9 @@ module Chat class << self def polymorphic_class_mapping = { "DirectMessage" => Chat::DirectMessage } - def for_user_ids(user_ids) + def for_user_ids(user_ids, group: false) joins(:users) - .where(group: false) + .where(group: group) .group("direct_message_channels.id") .having("ARRAY[?] = ARRAY_AGG(users.id ORDER BY users.id)", user_ids.sort) .first diff --git a/plugins/chat/app/services/chat/create_direct_message_channel.rb b/plugins/chat/app/services/chat/create_direct_message_channel.rb index 7cd9bd2f1c5..292e9116a90 100644 --- a/plugins/chat/app/services/chat/create_direct_message_channel.rb +++ b/plugins/chat/app/services/chat/create_direct_message_channel.rb @@ -20,6 +20,7 @@ module Chat # @param [Hash] params_to_create # @option params_to_create [Array] target_usernames # @option params_to_create [Array] target_groups + # @option params_to_create [Boolean] upsert # @return [Service::Base::Context] contract @@ -42,6 +43,7 @@ module Chat attribute :name, :string attribute :target_usernames, :array attribute :target_groups, :array + attribute :upsert, :boolean, default: false validate :target_presence @@ -78,11 +80,13 @@ module Chat def fetch_or_create_direct_message(target_users:, contract:) ids = target_users.map(&:id) + is_group = ids.size > 2 || contract.name.present? - if ids.size > 2 || contract.name.present? - ::Chat::DirectMessage.create(user_ids: ids, group: true) + if contract.upsert || !is_group + ::Chat::DirectMessage.for_user_ids(ids, group: is_group) || + ::Chat::DirectMessage.create(user_ids: ids, group: is_group) else - ::Chat::DirectMessage.for_user_ids(ids) || ::Chat::DirectMessage.create(user_ids: ids) + ::Chat::DirectMessage.create(user_ids: ids, group: is_group) end end 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 a60ca3c132a..b3d4fba9e61 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 @@ -40,9 +40,9 @@ export default class NewGroup extends Component { .filter((member) => member.type === "group") .mapBy("model.name"); - const channel = await this.chat.upsertDmChannel( + const channel = await this.chat.createDmChannel( { usernames, groups }, - this.newGroupTitle + { name: this.newGroupTitle } ); if (!channel) { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat.js b/plugins/chat/assets/javascripts/discourse/services/chat.js index e6ee02e7472..d84881f7352 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat.js @@ -355,14 +355,17 @@ export default class Chat extends Service { // channel for. The current user will automatically be included in the channel when it is created. // @param {array} [targets.usernames] - The usernames to include in the direct message channel. // @param {array} [targets.groups] - The groups to include in the direct message channel. - // @param {string|null} [name=null] - Optional name for the direct message channel. - upsertDmChannel(targets, name = null) { + // @param {object} opts - Optional values when fetching or creating the direct message channel. + // @param {string|null} [opts.name] - Name for the direct message channel. + // @param {boolean} [opts.upsert] - Should we attempt to fetch existing channel before creating a new one. + createDmChannel(targets, opts = { name: null, upsert: false }) { return ajax("/chat/api/direct-message-channels.json", { method: "POST", data: { target_usernames: targets.usernames?.uniq(), target_groups: targets.groups?.uniq(), - name, + upsert: opts.upsert, + name: opts.name, }, }) .then((response) => { @@ -373,6 +376,10 @@ export default class Chat extends Service { .catch(popupAjaxError); } + upsertDmChannel(targets, name = null) { + return this.createDmChannel(targets, { name, upsert: true }); + } + // @param {array} usernames - The usernames to fetch the direct message // channel for. The current user will automatically be included as a // participant to fetch the channel for. 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 index ef87c9e1920..c980f6e2a84 100644 --- a/plugins/chat/spec/requests/chat/api/direct_messages_controller_spec.rb +++ b/plugins/chat/spec/requests/chat/api/direct_messages_controller_spec.rb @@ -13,7 +13,7 @@ RSpec.describe Chat::Api::DirectMessagesController do end def create_dm_channel(user_ids) - direct_messages_channel = Chat::DirectMessage.create! + direct_messages_channel = Chat::DirectMessage.create!(group: user_ids.length > 2) user_ids.each do |user_id| direct_messages_channel.direct_message_users.create!(user_id: user_id) end @@ -65,7 +65,7 @@ RSpec.describe Chat::Api::DirectMessagesController do end end - describe "dm with two other users" do + describe "dm with multiple users" do let(:usernames) { [user1, user2, user3].map(&:username) } let(:direct_message_user_ids) { [current_user.id, user1.id, user2.id, user3.id] } @@ -78,13 +78,25 @@ RSpec.describe Chat::Api::DirectMessagesController do ) end - it "createsa new dm channel" do + it "creates a new dm channel" do create_dm_channel(direct_message_user_ids) expect { post "/chat/api/direct-message-channels.json", params: { target_usernames: [usernames] } }.to change { Chat::DirectMessage.count }.by(1) end + + it "returns existing dm channel when upsert is true" do + create_dm_channel(direct_message_user_ids) + + expect { + post "/chat/api/direct-message-channels.json", + params: { + target_usernames: [usernames], + upsert: true, + } + }.not_to change { Chat::DirectMessage.count } + end end it "creates Chat::UserChatChannelMembership records" do diff --git a/plugins/chat/spec/system/chat_new_message_spec.rb b/plugins/chat/spec/system/chat_new_message_spec.rb index dc11f2043bf..41301c65537 100644 --- a/plugins/chat/spec/system/chat_new_message_spec.rb +++ b/plugins/chat/spec/system/chat_new_message_spec.rb @@ -35,12 +35,38 @@ RSpec.describe "Chat New Message from params", type: :system do end context "with multiple users" do - it "creates a dm channel with multiple users" do - chat_page.visit_new_message([user_1, user_2]) + fab!(:group_dm) do + Fabricate(:direct_message_channel, users: [current_user, user_1, user_2], group: true) + end + fab!(:user_3) { Fabricate(:user) } + it "loads existing dm channel when one exists" do + expect { chat_page.visit_new_message([user_1, user_2]) }.not_to change { Chat::Channel.count } users = [user_1.username, user_2.username].permutation.map { |u| u.join("-") }.join("|") - expect(page).to have_current_path(%r{/chat/c/(#{users})/#{Chat::Channel.last.id}}) + expect(page).to have_current_path(%r{/chat/c/(#{users})/#{group_dm.id}}) + end + + it "creates a dm channel when none exists" do + expect { chat_page.visit_new_message([user_1, user_3]) }.to change { Chat::Channel.count }.by( + 1, + ) + + expect(page).to have_current_path( + "/chat/c/#{user_1.username}-#{user_3.username}/#{Chat::Channel.last.id}", + ) + end + + context "when user has chat disabled" do + before { user_3.user_option.update!(chat_enabled: false) } + + it "loads channel without the chat disabled user" do + expect { chat_page.visit_new_message([user_1, user_3]) }.not_to change { + Chat::Channel.count + } + + expect(page).to have_current_path("/chat/c/#{user_1.username}/#{user_1_channel.id}") + end end end end