From 4b49e358fb0dc6da3ed1e93a1ef4404c8ed3bb06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Thu, 22 Aug 2024 11:35:13 +0200 Subject: [PATCH] FIX: skip 1:1s when chat search returns users (#28464) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we use CTRL/CMD + K to search, on the server we run 4 queries: - 1 for users - 1 for groups - 1 for category channels - 1 for direct message channels The server returns up to 10 results per type and the client concatenate all the results (in the order I described) and then takes the first 10 or so. Let's say you've had 1:1s with john1, john2, and john3. Searching for “john” would return all the johns as “users”. It doesn’t make sense to also return them as 1:1 dms. That’s what this fixes. When the search returns some users, we skip all 1:1s because we assume they will show up as the results of the “users” query. We’ll always return matching direct messages with more than 2 users (aka. “group chats”). All 3 other queries (users, groups, and category channels) are unaffected. Internal ref - t/136079 --- .../chat/app/services/chat/search_chatable.rb | 91 ++++++++++--------- 1 file changed, 47 insertions(+), 44 deletions(-) diff --git a/plugins/chat/app/services/chat/search_chatable.rb b/plugins/chat/app/services/chat/search_chatable.rb index e67a131890e..ec9a575cddb 100644 --- a/plugins/chat/app/services/chat/search_chatable.rb +++ b/plugins/chat/app/services/chat/search_chatable.rb @@ -9,6 +9,8 @@ module Chat class SearchChatable include Service::Base + SEARCH_RESULT_LIMIT ||= 10 + # @!method call(term:, guardian:) # @param [String] term # @param [Guardian] guardian @@ -35,7 +37,7 @@ module Chat private def clean_term(contract:) - context.term = contract.term.downcase&.gsub(/^#+/, "")&.gsub(/^@+/, "")&.strip + context.term = contract.term&.downcase&.strip&.gsub(/^[@#]+/, "") end def fetch_memberships(guardian:) @@ -45,57 +47,29 @@ module Chat def fetch_users(guardian:, contract:) return unless contract.include_users return unless guardian.can_create_direct_message? - search_users(context, guardian, contract) + search_users(context, guardian) end def fetch_groups(guardian:, contract:) return unless contract.include_groups return unless guardian.can_create_direct_message? - search_groups(context, guardian, contract) + search_groups(context, guardian) end def fetch_category_channels(guardian:, contract:) return unless contract.include_category_channels - return if !SiteSetting.enable_public_channels - - ::Chat::ChannelFetcher.secured_public_channel_search( - guardian, - filter_on_category_name: false, - match_filter_on_starts_with: false, - filter: context.term, - status: :open, - limit: 10, - ) + return unless SiteSetting.enable_public_channels + search_category_channels(context, guardian) end - def fetch_direct_message_channels(guardian:, users:, contract:, **args) + def fetch_direct_message_channels(guardian:, contract:, users:, **args) return unless contract.include_direct_message_channels - - channels = - ::Chat::ChannelFetcher.secured_direct_message_channels_search( - guardian.user.id, - guardian, - limit: 10, - match_filter_on_starts_with: false, - filter: context.term, - ) || [] - - if users && contract.include_users - user_ids = users.map(&:id) - channels = - channels.reject do |channel| - channel_user_ids = channel.allowed_user_ids - [guardian.user.id] - channel.allowed_user_ids.length == 1 && - user_ids.include?(channel.allowed_user_ids.first) || - channel_user_ids.length == 1 && user_ids.include?(channel_user_ids.first) - end - end - - channels + return unless guardian.can_create_direct_message? + search_direct_message_channels(context, guardian, contract, users) end - def search_users(context, guardian, contract) - user_search = ::UserSearch.new(context.term, limit: 10) + def search_users(context, guardian) + user_search = ::UserSearch.new(context.term, limit: SEARCH_RESULT_LIMIT) if context.term.blank? user_search = user_search.scoped_users @@ -112,11 +86,7 @@ module Chat if context.excluded_memberships_channel_id user_search = user_search.where( - "NOT EXISTS ( - SELECT 1 - FROM user_chat_channel_memberships - WHERE user_chat_channel_memberships.user_id = users.id AND user_chat_channel_memberships.chat_channel_id = ? - )", + "NOT EXISTS (SELECT 1 FROM user_chat_channel_memberships WHERE user_id = users.id AND chat_channel_id = ?)", context.excluded_memberships_channel_id, ) end @@ -124,7 +94,7 @@ module Chat user_search end - def search_groups(context, guardian, contract) + def search_groups(context, guardian) Group .visible_groups(guardian.user) .includes(users: :user_option) @@ -132,6 +102,39 @@ module Chat "groups.name ILIKE :term_like OR groups.full_name ILIKE :term_like", term_like: "%#{context.term}%", ) + .limit(SEARCH_RESULT_LIMIT) + end + + def search_category_channels(context, guardian) + ::Chat::ChannelFetcher.secured_public_channel_search( + guardian, + status: :open, + filter: context.term, + filter_on_category_name: false, + match_filter_on_starts_with: false, + limit: SEARCH_RESULT_LIMIT, + ) + end + + def search_direct_message_channels(context, guardian, contract, users) + channels = + ::Chat::ChannelFetcher.secured_direct_message_channels_search( + guardian.user.id, + guardian, + filter: context.term, + match_filter_on_starts_with: false, + limit: SEARCH_RESULT_LIMIT, + ) || [] + + # skip 1:1s when search returns users + if contract.include_users && users.present? + channels.reject! do |channel| + other_user_ids = channel.allowed_user_ids - [guardian.user.id] + other_user_ids.size <= 1 + end + end + + channels end end end