Show reaction counts and user names only for reactions visible to the logged-in user (#42630).

Patch by Katsuya HIDAKA (user:hidakatsuya).


git-svn-id: https://svn.redmine.org/redmine/trunk@23768 e93f8b46-1217-0410-a6f0-8f06a7374b81

5 files changed, 15 insertions, 26 deletions

diff --git a/app/models/reaction.rb b/app/models/reaction.rb
index 84c982043..184ed2d6e 100644
--- a/app/models/reaction.rb
+++ b/app/models/reaction.rb
@@ -25,39 +25,34 @@ class Reaction < ApplicationRecord
 
   scope :by, ->(user) { where(user: user) }
   scope :for_reactable, ->(reactable) { where(reactable: reactable) }
+  scope :visible, ->(user) { where(user: User.visible(user)) }
 
   # Represents reaction details for a reactable object
   Detail = Struct.new(
-    # Total number of reactions
-    :reaction_count,
     # Users who reacted and are visible to the target user
     :visible_users,
     # Reaction of the target user
     :user_reaction
   ) do
-    def initialize(reaction_count: 0, visible_users: [], user_reaction: nil)
+    def initialize(visible_users: [], user_reaction: nil)
       super
     end
+
+    def reaction_count = visible_users.size
   end
 
   def self.build_detail_map_for(reactables, user)
-    reactions = preload(:user)
+    reactions = visible(user)
                   .for_reactable(reactables)
+                  .preload(:user)
                   .select(:id, :reactable_id, :user_id)
                   .order(id: :desc)
 
-    # Prepare IDs of users who reacted and are visible to the user
-    visible_user_ids = User.visible(user)
-                         .joins(:reactions)
-                         .where(reactions: for_reactable(reactables))
-                         .pluck(:id).to_set
-
     reactions.each_with_object({}) do |reaction, m|
       m[reaction.reactable_id] ||= Detail.new
 
       m[reaction.reactable_id].then do |detail|
-        detail.reaction_count += 1
-        detail.visible_users << reaction.user if visible_user_ids.include?(reaction.user.id)
+        detail.visible_users << reaction.user
         detail.user_reaction = reaction if reaction.user == user
       end
     end
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index b7e0321d4..1230ec1eb 100644
--- a/test/functional/issues_controller_test.rb
+++ b/test/functional/issues_controller_test.rb
@@ -3347,8 +3347,8 @@ class IssuesControllerTest < Redmine::ControllerTest
       # The current_user can only see members who belong to projects that the current_user has access to.
       # Since the Redmine Admin user does not belong to any projects visible to the current_user,
       # the Redmine Admin user's name is not displayed in the reaction user list. Instead, "1 other" is shown.
-      assert_select 'a.reaction-button[title=?]', 'Dave Lopper, John Smith, and 1 other' do
-        assert_select 'span.icon-label', '3'
+      assert_select 'a.reaction-button[title=?]', 'Dave Lopper and John Smith' do
+        assert_select 'span.icon-label', '2'
       end
     end
 
diff --git a/test/helpers/reactions_helper_test.rb b/test/helpers/reactions_helper_test.rb
index f3a4e38d8..ab722e3ca 100644
--- a/test/helpers/reactions_helper_test.rb
+++ b/test/helpers/reactions_helper_test.rb
@@ -106,12 +106,10 @@ class ReactionsHelperTest < ActionView::TestCase
     assert_select_in result, 'a.reaction-button[title=?]', expected_tooltip
   end
 
-  test 'reaction_button displays non-visible users as "X other" in the tooltip' do
+  test 'reaction_button should not count and display non-visible users' do
     issue2 = issues(:issues_002)
 
     issue2.reaction_detail = Reaction::Detail.new(
-      # The remaining 3 users are non-visible users
-      reaction_count: 5,
       visible_users: users(:users_002, :users_003)
     )
 
@@ -119,11 +117,10 @@ class ReactionsHelperTest < ActionView::TestCase
       reaction_button(issue2)
     end
 
-    assert_select_in result, 'a.reaction-button[title=?]', 'John Smith, Dave Lopper, and 3 others'
+    assert_select_in result, 'a.reaction-button[title=?]', 'John Smith and Dave Lopper'
 
     # When all users are non-visible users
     issue2.reaction_detail = Reaction::Detail.new(
-      reaction_count: 2,
       visible_users: []
     )
 
@@ -131,7 +128,10 @@ class ReactionsHelperTest < ActionView::TestCase
       reaction_button(issue2)
     end
 
-    assert_select_in result, 'a.reaction-button[title=?]', '2 others'
+    assert_select_in result, 'a.reaction-button[title]', false
+    assert_select_in result, 'a.reaction-button' do
+      assert_select 'span.icon-label', '0'
+    end
   end
 
   test 'reaction_button formats the tooltip content based on the support.array settings of each locale' do
diff --git a/test/unit/lib/redmine/reaction_test.rb b/test/unit/lib/redmine/reaction_test.rb
index bed4600d0..7b2bd574b 100644
--- a/test/unit/lib/redmine/reaction_test.rb
+++ b/test/unit/lib/redmine/reaction_test.rb
@@ -42,7 +42,6 @@ class Redmine::ReactionTest < ActiveSupport::TestCase
     Issue.preload_reaction_details([issue1, issue2])
 
     expected_issue1_reaction_detail = Reaction::Detail.new(
-      reaction_count: 3,
       visible_users: [users(:users_003), users(:users_002), users(:users_001)],
       user_reaction: reactions(:reaction_002)
     )
@@ -53,7 +52,6 @@ class Redmine::ReactionTest < ActiveSupport::TestCase
 
       # Even when an object has no reactions, an empty ReactionDetail is set.
       assert_equal Reaction::Detail.new(
-        reaction_count: 0,
         visible_users: [],
         user_reaction: nil
       ), issue2.reaction_detail
@@ -107,7 +105,6 @@ class Redmine::ReactionTest < ActiveSupport::TestCase
     assert_nil message7.instance_variable_get(:@reaction_detail)
 
     assert_equal Reaction::Detail.new(
-      reaction_count: 1,
       visible_users: [users(:users_002)],
       user_reaction: reactions(:reaction_009)
     ), message7.reaction_detail
@@ -122,7 +119,6 @@ class Redmine::ReactionTest < ActiveSupport::TestCase
     comment1.load_reaction_detail
 
     assert_equal Reaction::Detail.new(
-      reaction_count: 1,
       visible_users: [users(:users_002)],
       user_reaction: nil
     ), comment1.reaction_detail
diff --git a/test/unit/reaction_test.rb b/test/unit/reaction_test.rb
index 2690da351..9b3da0738 100644
--- a/test/unit/reaction_test.rb
+++ b/test/unit/reaction_test.rb
@@ -75,12 +75,10 @@ class ReactionTest < ActiveSupport::TestCase
 
     expected = {
       1 => Reaction::Detail.new(
-        reaction_count: 3,
         visible_users: [users(:users_003), users(:users_002), users(:users_001)],
         user_reaction: reactions(:reaction_003)
       ),
       6 => Reaction::Detail.new(
-        reaction_count: 1,
         visible_users: [users(:users_002)],
         user_reaction: nil
       )