FEATURE: Only list watching group messages in messages notifications panel (#20630)

Why is this change required?

Prior to this change, we would list all group messages that a user
has access to in the user menu messages notifications panel dropdown.
However, this did not respect the topic's notification level setting and
group messages which the user has set to 'normal' notification level were
being displayed

What does this commit do?

With this commit, we no longer display all group messages that a user
has access to. Instead, we only display group messages that a user is
watching in the user menu messages notifications panel dropdown.

Internal Ref: /t/94392

app/controllers/users_controller.rb

@@ -1842,16 +1842,21 @@ class UsersController < ApplicationController
     if unread_notifications.size < USER_MENU_LIST_LIMIT
       exclude_topic_ids = unread_notifications.filter_map(&:topic_id).uniq
       limit = USER_MENU_LIST_LIMIT - unread_notifications.size
+
       messages_list =
         TopicQuery
           .new(current_user, per_page: limit)
-          .list_private_messages_direct_and_groups(current_user) do |query|
+          .list_private_messages_direct_and_groups(
+            current_user,
+            groups_messages_notification_level: :watching,
+          ) do |query|
             if exclude_topic_ids.present?
               query.where("topics.id NOT IN (?)", exclude_topic_ids)
             else
               query
             end
           end
+
       read_notifications =
         Notification
           .for_user_menu(current_user.id, limit: limit)

lib/topic_query/private_message_lists.rb

@@ -3,15 +3,20 @@
 class TopicQuery
   module PrivateMessageLists
     def list_private_messages(user, &blk)
-      list = private_messages_for(user, :user)
+      list = user_personal_private_messages(user)
       list = not_archived(list, user)
       list = have_posts_from_others(list, user)
 
       create_list(:private_messages, {}, list, &blk)
     end
 
-    def list_private_messages_direct_and_groups(user, &blk)
+    def list_private_messages_direct_and_groups(user, groups_messages_notification_level: nil, &blk)
-      list = private_messages_for(user, :all)
+      list =
+        user_personal_and_groups_private_messages(
+          user,
+          groups_messages_notification_level: groups_messages_notification_level,
+        )
+
       list = not_archived(list, user)
       list = not_archived_in_groups(list)
       list = have_posts_from_others(list, user)
@@ -20,14 +25,16 @@ class TopicQuery
     end
 
     def list_private_messages_archive(user)
-      list = private_messages_for(user, :user)
+      list = user_personal_private_messages(user)
+
       list =
         list.joins(:user_archived_messages).where("user_archived_messages.user_id = ?", user.id)
+
       create_list(:private_messages, {}, list)
     end
 
     def list_private_messages_sent(user)
-      list = private_messages_for(user, :user)
+      list = user_personal_private_messages(user)
 
       list = list.where(<<~SQL, user.id)
       EXISTS (
@@ -54,7 +61,7 @@ class TopicQuery
     end
 
     def list_private_messages_group(user)
-      list = private_messages_for(user, :group)
+      list = user_groups_private_messages(user)
 
       list = list.joins(<<~SQL)
       LEFT JOIN group_archived_messages gm
@@ -68,7 +75,7 @@ class TopicQuery
     end
 
     def list_private_messages_group_archive(user)
-      list = private_messages_for(user, :group)
+      list = user_groups_private_messages(user)
 
       list = list.joins(<<~SQL)
       INNER JOIN group_archived_messages gm
@@ -96,7 +103,7 @@ class TopicQuery
     end
 
     def list_private_messages_warnings(user)
-      list = private_messages_for(user, :user)
+      list = user_personal_private_messages(user)
       list = list.where("topics.subtype = ?", TopicSubtype.moderator_warning)
       # Exclude official warnings that the user created, instead of received
       list = list.where("topics.user_id <> ?", user.id)
@@ -104,79 +111,24 @@ class TopicQuery
     end
 
     def private_messages_for(user, type)
-      options = @options
-      options.reverse_merge!(per_page: per_page_setting)
-
-      result = Topic.includes(:allowed_users)
-      result = result.includes(:tags) if SiteSetting.tagging_enabled
-
       if type == :group
-        result =
+        user_groups_private_messages(user)
-          result.joins(
-            "INNER JOIN topic_allowed_groups tag ON tag.topic_id = topics.id AND tag.group_id IN (SELECT id FROM groups WHERE LOWER(name) = '#{PG::Connection.escape_string(@options[:group_name].downcase)}')",
-          )
-
-        unless user.admin?
-          result =
-            result.joins(
-              "INNER JOIN group_users gu ON gu.group_id = tag.group_id AND gu.user_id = #{user.id.to_i}",
-            )
-        end
       elsif type == :user
-        result =
+        user_personal_private_messages(user)
-          result.where(
-            "topics.id IN (SELECT topic_id FROM topic_allowed_users WHERE user_id = ?)",
-            user.id.to_i,
-          )
       elsif type == :all
-        group_ids = group_with_messages_ids(user)
+        user_personal_and_groups_private_messages(user)
-
-        result =
-          if group_ids.present?
-            result.where(<<~SQL, user.id.to_i, group_ids)
-            topics.id IN (
-              SELECT topic_id
-              FROM topic_allowed_users
-              WHERE user_id = ?
-              UNION ALL
-              SELECT topic_id FROM topic_allowed_groups
-              WHERE group_id IN (?)
-            )
-          SQL
-          else
-            result.joins(<<~SQL)
-          INNER JOIN topic_allowed_users tau
-            ON tau.topic_id = topics.id
-            AND tau.user_id = #{user.id.to_i}
-          SQL
-          end
       end
-
-      result =
-        result
-          .joins(
-            "LEFT OUTER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{user.id.to_i})",
-          )
-          .order("topics.bumped_at DESC")
-          .private_messages
-
-      result = result.limit(options[:per_page]) unless options[:limit] == false
-      result = result.visible if options[:visible] || @user.nil? || @user.regular?
-
-      if options[:page]
-        offset = options[:page].to_i * options[:per_page]
-        result = result.offset(offset) if offset > 0
-      end
-      result
     end
 
     def list_private_messages_tag(user)
-      list = private_messages_for(user, :all)
+      list = user_personal_and_groups_private_messages(user)
+
       list =
         list.joins(
           "JOIN topic_tags tt ON tt.topic_id = topics.id
                         JOIN tags t ON t.id = tt.tag_id AND t.name = '#{@options[:tags][0]}'",
         )
+
       create_list(:private_messages, {}, list)
     end
 
@@ -296,5 +248,98 @@ class TopicQuery
 
       @group_with_messages_ids[user.id] = user.groups.where(has_messages: true).pluck(:id)
     end
+
+    private
+
+    def private_messages_default_scope(user)
+      options = @options
+      options.reverse_merge!(per_page: per_page_setting)
+
+      result =
+        Topic
+          .private_messages
+          .includes(:allowed_users)
+          .joins(
+            "LEFT OUTER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{user.id.to_i})",
+          )
+          .order("topics.bumped_at DESC")
+
+      result = result.includes(:tags) if SiteSetting.tagging_enabled
+      result = result.limit(options[:per_page]) unless options[:limit] == false
+      result = result.visible if options[:visible] || @user.nil? || @user.regular?
+
+      if options[:page]
+        offset = options[:page].to_i * options[:per_page]
+        result = result.offset(offset) if offset > 0
+      end
+
+      result
+    end
+
+    def user_groups_private_messages(user)
+      result = private_messages_default_scope(user)
+
+      result =
+        result.joins(
+          "INNER JOIN topic_allowed_groups tag ON tag.topic_id = topics.id AND tag.group_id IN (SELECT id FROM groups WHERE LOWER(name) = '#{PG::Connection.escape_string(@options[:group_name].downcase)}')",
+        )
+
+      unless user.admin?
+        result =
+          result.joins(
+            "INNER JOIN group_users gu ON gu.group_id = tag.group_id AND gu.user_id = #{user.id.to_i}",
+          )
+      end
+
+      result
+    end
+
+    def user_personal_private_messages(user)
+      result = private_messages_default_scope(user)
+
+      result.where(
+        "topics.id IN (SELECT topic_id FROM topic_allowed_users WHERE user_id = ?)",
+        user.id.to_i,
+      )
+    end
+
+    def user_personal_and_groups_private_messages(user, groups_messages_notification_level: nil)
+      result = private_messages_default_scope(user)
+      group_ids = group_with_messages_ids(user)
+
+      topic_allowed_groups_scope =
+        if groups_messages_notification_level.present? &&
+             notification_level =
+               NotificationLevels.topic_levels[groups_messages_notification_level]
+          <<~SQL
+          SELECT topic_allowed_groups.topic_id
+          FROM topic_allowed_groups
+          INNER JOIN topic_users ON topic_users.topic_id = topic_allowed_groups.topic_id AND topic_users.user_id = :user_id
+          WHERE group_id IN (:group_ids)
+          AND topic_users.notification_level >= #{notification_level.to_i}
+          SQL
+        else
+          "SELECT topic_id FROM topic_allowed_groups WHERE group_id IN (:group_ids)"
+        end
+
+      result =
+        if group_ids.present?
+          result.where(<<~SQL, user_id: user.id.to_i, group_ids: group_ids)
+          topics.id IN (
+            SELECT topic_id
+            FROM topic_allowed_users
+            WHERE user_id = :user_id
+            UNION ALL
+            #{topic_allowed_groups_scope}
+          )
+          SQL
+        else
+          result.joins(<<~SQL)
+          INNER JOIN topic_allowed_users tau
+          ON tau.topic_id = topics.id
+          AND tau.user_id = #{user.id.to_i}
+          SQL
+        end
+    end
   end
 end

spec/lib/topic_query/private_message_lists_spec.rb

@@ -320,4 +320,38 @@ RSpec.describe TopicQuery::PrivateMessageLists do
       expect(TopicQuery.new(user_4).private_messages_for(user_4, :all)).to eq([])
     end
   end
+
+  describe "#list_private_messages_direct_and_groups" do
+    it "returns a list of all personal and group private messages for a given user" do
+      expect(
+        TopicQuery.new(user_2).list_private_messages_direct_and_groups(user_2).topics,
+      ).to contain_exactly(private_message, group_message)
+    end
+
+    it "returns a list of personal private messages and user watching group private messages for a given user when the `groups_notification_level` option is set" do
+      expect(
+        TopicQuery
+          .new(user_2)
+          .list_private_messages_direct_and_groups(
+            user_2,
+            groups_messages_notification_level: :watching,
+          )
+          .topics,
+      ).to contain_exactly(private_message, group_message)
+
+      TopicUser.find_by(user: user_2, topic: group_message).update!(
+        notification_level: NotificationLevels.topic_levels[:regular],
+      )
+
+      expect(
+        TopicQuery
+          .new(user_2)
+          .list_private_messages_direct_and_groups(
+            user_2,
+            groups_messages_notification_level: :watching,
+          )
+          .topics,
+      ).to contain_exactly(private_message)
+    end
+  end
 end

spec/requests/users_controller_spec.rb

@@ -6526,24 +6526,31 @@ RSpec.describe UsersController do
         )
       end
 
-      it "responds with an array of PM topics that are not associated with any of the unread private_message notifications" do
+      it "responds with an array of personal messages and user watching group messages that are not associated with any of the unread private_message notifications" do
         group_message1.update!(bumped_at: 1.minutes.ago)
         message_without_notification.update!(bumped_at: 3.minutes.ago)
         group_message2.update!(bumped_at: 6.minutes.ago)
         message_with_read_notification.update!(bumped_at: 10.minutes.ago)
         read_group_message_summary_notification.destroy!
 
+        TopicUser.create!(
+          user: user,
+          topic: group_message1,
+          notification_level: TopicUser.notification_levels[:watching],
+        )
+        TopicUser.create!(
+          user: user,
+          topic: group_message2,
+          notification_level: TopicUser.notification_levels[:regular],
+        )
+
         get "/u/#{user.username}/user-menu-private-messages"
         expect(response.status).to eq(200)
 
         topics = response.parsed_body["topics"]
+
         expect(topics.map { |topic| topic["id"] }).to eq(
-          [
+          [group_message1.id, message_without_notification.id, message_with_read_notification.id],
-            group_message1.id,
-            message_without_notification.id,
-            group_message2.id,
-            message_with_read_notification.id,
-          ],
         )
       end