From d4078b535c9854695e770cdfb5e0f4846a8cf64a Mon Sep 17 00:00:00 2001
From: Camil Staps <info@camilstaps.nl>
Date: Sat, 27 Jul 2019 08:26:53 +0200
Subject: [PATCH] Fix public/private starrers counts in special cases

---
 .../projects/starrers_controller.rb           | 23 +++++++--
 app/models/users_star_project.rb              |  1 +
 .../projects/starrers_controller_spec.rb      | 47 ++++++++++++++++---
 3 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/app/controllers/projects/starrers_controller.rb b/app/controllers/projects/starrers_controller.rb
index f69cff1b431..c8facea1d70 100644
--- a/app/controllers/projects/starrers_controller.rb
+++ b/app/controllers/projects/starrers_controller.rb
@@ -4,14 +4,31 @@ class Projects::StarrersController < Projects::ApplicationController
   include SortingHelper
 
   def index
-    @sort = params[:sort].presence || sort_value_name
-
     @starrers = UsersStarProjectsFinder.new(@project, params, current_user: @current_user).execute
 
+    # Normally the number of public starrers is equal to the number of visible
+    # starrers. We need to fix the counts in two cases: when the current user
+    # is an admin (and can see everything) and when the current user has a
+    # private profile and has starred the project (and can see itself).
+    @public_count =
+      if @current_user&.admin?
+        @starrers.with_public_profile.count
+      elsif @current_user&.private_profile && has_starred_project?(@starrers)
+        @starrers.size - 1
+      else
+        @starrers.size
+      end
+
     @total_count = @project.starrers.size
-    @public_count = @starrers.size
     @private_count = @total_count - @public_count
 
+    @sort = params[:sort].presence || sort_value_name
     @starrers = @starrers.sort_by_attribute(@sort).page(params[:page])
   end
+
+  private
+
+  def has_starred_project?(starrers)
+    starrers.first { |starrer| starrer.user_id == current_user.id }
+  end
 end
diff --git a/app/models/users_star_project.rb b/app/models/users_star_project.rb
index baf055dd6dd..3c7a805cc5c 100644
--- a/app/models/users_star_project.rb
+++ b/app/models/users_star_project.rb
@@ -16,6 +16,7 @@ class UsersStarProject < ApplicationRecord
   scope :order_user_name_desc, -> { joins(:user).merge(User.order_name_desc) }
   scope :by_project, -> (project) { where(project_id: project.id) }
   scope :with_visible_profile, -> (user) { joins(:user).merge(User.with_visible_profile(user)) }
+  scope :with_public_profile, -> { joins(:user).merge(User.with_public_profile) }
 
   class << self
     def sort_by_attribute(method)
diff --git a/spec/controllers/projects/starrers_controller_spec.rb b/spec/controllers/projects/starrers_controller_spec.rb
index 863929a2465..59d258e99ce 100644
--- a/spec/controllers/projects/starrers_controller_spec.rb
+++ b/spec/controllers/projects/starrers_controller_spec.rb
@@ -5,6 +5,7 @@ require 'spec_helper'
 describe Projects::StarrersController do
   let(:user) { create(:user) }
   let(:private_user) { create(:user, private_profile: true) }
+  let(:admin) { create(:user, admin: true) }
   let(:project) { create(:project, :public, :repository) }
 
   before do
@@ -26,25 +27,57 @@ describe Projects::StarrersController do
         project.update_attribute(:visibility_level, Project::PUBLIC)
       end
 
-      it 'only public starrers are visible for non logged in users' do
-        get_starrers
+      context 'when no user is logged in' do
+        before do
+          get_starrers
+        end
 
-        user_ids = assigns[:starrers].map { |s| s['user_id'] }
-        expect(user_ids).to include(user.id)
-        expect(user_ids).not_to include(private_user.id)
+        it 'only public starrers are visible' do
+          user_ids = assigns[:starrers].map { |s| s['user_id'] }
+          expect(user_ids).to include(user.id)
+          expect(user_ids).not_to include(private_user.id)
+        end
+
+        it 'public/private starrers counts are correct' do
+          expect(assigns[:public_count]).to eq(1)
+          expect(assigns[:private_count]).to eq(1)
+        end
       end
 
       context 'when private user is logged in' do
         before do
           sign_in(private_user)
+
+          get_starrers
         end
 
         it 'their star is also visible' do
-          get_starrers
-
           user_ids = assigns[:starrers].map { |s| s['user_id'] }
           expect(user_ids).to include(user.id, private_user.id)
         end
+
+        it 'public/private starrers counts are correct' do
+          expect(assigns[:public_count]).to eq(1)
+          expect(assigns[:private_count]).to eq(1)
+        end
+      end
+
+      context 'when admin is logged in' do
+        before do
+          sign_in(admin)
+
+          get_starrers
+        end
+
+        it 'all stars are visible' do
+          user_ids = assigns[:starrers].map { |s| s['user_id'] }
+          expect(user_ids).to include(user.id, private_user.id)
+        end
+
+        it 'public/private starrers counts are correct' do
+          expect(assigns[:public_count]).to eq(1)
+          expect(assigns[:private_count]).to eq(1)
+        end
       end
     end