Fix sidebar issue countissue_38418

5 files changed, 84 insertions, 18 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index 534a0e630af..f227ee11cb0 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1417,8 +1417,8 @@ class Project < ActiveRecord::Base
     self.runners_token && ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.runners_token)
   end
 
-  def open_issues_count
-    Projects::OpenIssuesCountService.new(self).count
+  def open_issues_count(current_user = nil)
+    Projects::OpenIssuesCountService.new(self, current_user).count
   end
 
   def open_merge_requests_count
diff --git a/app/services/projects/open_issues_count_service.rb b/app/services/projects/open_issues_count_service.rb
index a975a06a05c..0a004677417 100644
--- a/app/services/projects/open_issues_count_service.rb
+++ b/app/services/projects/open_issues_count_service.rb
@@ -2,14 +2,42 @@ module Projects
   # Service class for counting and caching the number of open issues of a
   # project.
   class OpenIssuesCountService < Projects::CountService
+    include Gitlab::Utils::StrongMemoize
+
+    def initialize(project, user = nil)
+      @user = user
+
+      super(project)
+    end
+
     def cache_key_name
-      'open_issues_count'
+      public_only? ? 'public_open_issues_count' : 'total_open_issues_count'
+    end
+
+    def public_only?
+      !user_is_at_least_reporter?
+    end
+
+    def relation_for_count
+      self.class.query(@project, public_only: public_only?)
+    end
+
+    def user_is_at_least_reporter?
+      strong_memoize(:user_is_at_least_reporter) do
+        @user && @project.team.member?(@user, Gitlab::Access::REPORTER)
+      end
     end
 
-    def self.query(project_ids)
-      # We don't include confidential issues in this number since this would
-      # expose the number of confidential issues to non project members.
-      Issue.opened.public_only.where(project: project_ids)
+    # We only show total issues count for reporters
+    # which are allowed to view confidential issues
+    # This will still show a discrepancy on issues number but should be less than before.
+    # Check https://gitlab.com/gitlab-org/gitlab-ce/issues/38418 description.
+    def self.query(projects, public_only: true)
+      if public_only
+        Issue.opened.public_only.where(project: projects)
+      else
+        Issue.opened.where(project: projects)
+      end
     end
   end
 end
diff --git a/app/views/layouts/nav/sidebar/_project.html.haml b/app/views/layouts/nav/sidebar/_project.html.haml
index c3ea592a6b5..3223dc903f5 100644
--- a/app/views/layouts/nav/sidebar/_project.html.haml
+++ b/app/views/layouts/nav/sidebar/_project.html.haml
@@ -89,7 +89,7 @@
               = _('Issues')
             - if @project.issues_enabled?
               %span.badge.count.issue_counter
-                = number_with_delimiter(@project.open_issues_count)
+                = number_with_delimiter(@project.open_issues_count(current_user))
 
           %ul.sidebar-sub-level-items
             = nav_link(controller: :issues, html_options: { class: "fly-out-top-item" } ) do
@@ -98,7 +98,7 @@
                   = _('Issues')
                 - if @project.issues_enabled?
                   %span.badge.count.issue_counter.fly-out-badge
-                    = number_with_delimiter(@project.open_issues_count)
+                    = number_with_delimiter(@project.open_issues_count(current_user))
             %li.divider.fly-out-top-item
             = nav_link(controller: :issues, action: :index) do
               = link_to project_issues_path(@project), title: 'Issues' do
diff --git a/changelogs/unreleased/issue_38418.yml b/changelogs/unreleased/issue_38418.yml
new file mode 100644
index 00000000000..79452b27e4b
--- /dev/null
+++ b/changelogs/unreleased/issue_38418.yml
@@ -0,0 +1,5 @@
+---
+title: Fix issue count on sidebar
+merge_request:
+author:
+type: other
diff --git a/spec/services/projects/open_issues_count_service_spec.rb b/spec/services/projects/open_issues_count_service_spec.rb
index f964f9972cd..06b470849b3 100644
--- a/spec/services/projects/open_issues_count_service_spec.rb
+++ b/spec/services/projects/open_issues_count_service_spec.rb
@@ -2,20 +2,53 @@ require 'spec_helper'
 
 describe Projects::OpenIssuesCountService do
   describe '#count' do
-    it 'returns the number of open issues' do
-      project = create(:project)
-      create(:issue, :opened, project: project)
+    let(:project) { create(:project) }
 
-      expect(described_class.new(project).count).to eq(1)
+    context 'when user is nil' do
+      it 'does not include confidential issues in the issue count' do
+        create(:issue, :opened, project: project)
+        create(:issue, :opened, confidential: true, project: project)
+
+        expect(described_class.new(project).count).to eq(1)
+      end
     end
 
-    it 'does not include confidential issues in the issue count' do
-      project = create(:project)
+    context 'when user is provided' do
+      let(:user) { create(:user) }
+
+      context 'when user can read confidential issues' do
+        before do
+          project.add_reporter(user)
+        end
+
+        it 'returns the right count with confidential issues' do
+          create(:issue, :opened, project: project)
+          create(:issue, :opened, confidential: true, project: project)
+
+          expect(described_class.new(project, user).count).to eq(2)
+        end
+
+        it 'uses total_open_issues_count cache key' do
+          expect(described_class.new(project, user).cache_key_name).to eq('total_open_issues_count')
+        end
+      end
+
+      context 'when user cannot read confidential issues' do
+        before do
+          project.add_guest(user)
+        end
+
+        it 'does not include confidential issues' do
+          create(:issue, :opened, project: project)
+          create(:issue, :opened, confidential: true, project: project)
 
-      create(:issue, :opened, project: project)
-      create(:issue, :opened, confidential: true, project: project)
+          expect(described_class.new(project, user).count).to eq(1)
+        end
 
-      expect(described_class.new(project).count).to eq(1)
+        it 'uses public_open_issues_count cache key' do
+          expect(described_class.new(project, user).cache_key_name).to eq('public_open_issues_count')
+        end
+      end
     end
   end
 end