Do not cache over threshold count values

- Fix OpenIssuesCountService specs
- Add group helper specs

app/services/groups/open_issues_count_service.rb

@@ -11,21 +11,30 @@ module Groups
     CACHED_COUNT_THRESHOLD = 1000
     EXPIRATION_TIME = 24.hours
 
+    attr_reader :group, :user
+
     def initialize(group, user = nil)
       @group = group
       @user = user
     end
 
+    # Reads count value from cache and return it if present.
+    # If empty or expired, #uncached_count will calculate the issues count for the group and
+    # compare it with the threshold. If it is greater, it will be written to the cache and returned.
+    # If below, it will be returned without being cached.
+    # This results in only caching large counts and calculating the rest with every call to maintain
+    # accuracy.
     def count
       cached_count = Rails.cache.read(cache_key)
+      return cached_count unless cached_count.blank?
 
-      if cached_count && cached_count >= CACHED_COUNT_THRESHOLD
-        cached_count
-      else
-        new_count = uncached_count
-        update_cache_for_key(cache_key) { new_count }
-        new_count
+      refreshed_count = uncached_count
+      update_cache_for_key(cache_key) { refreshed_count } if refreshed_count > CACHED_COUNT_THRESHOLD
+      refreshed_count
     end
+
+    def cache_key(key = nil)
+      ['groups', 'open_issues_count_service', VERSION, group.id, cache_key_name]
     end
 
     private
@@ -34,10 +43,6 @@ module Groups
       super.merge({ expires_in: EXPIRATION_TIME })
     end
 
-    def cache_key(key = nil)
-      ['groups', 'open_issues_count_service', VERSION, @group.id, cache_key_name]
-    end
-
     def cache_key_name
       public_only? ? PUBLIC_COUNT_KEY : TOTAL_COUNT_KEY
     end
@@ -48,16 +53,12 @@ module Groups
 
     def user_is_at_least_reporter?
       strong_memoize(:user_is_at_least_reporter) do
-        @user && @group.member?(@user, Gitlab::Access::REPORTER)
+        group.member?(user, Gitlab::Access::REPORTER)
       end
     end
 
     def relation_for_count
-      self.class.query(@group, user: @user, public_only: public_only?)
-    end
-
-    def self.query(group, user: nil, public_only: true)
-      IssuesFinder.new(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: public_only).execute
+      IssuesFinder.new(user, group_id: group.id, state: 'opened', non_archived: true, include_subgroups: true, public_only: public_only?).execute
     end
   end
 end

spec/helpers/groups_helper_spec.rb

@@ -445,6 +445,42 @@ RSpec.describe GroupsHelper do
     end
   end
 
+  describe '#group_open_issues_count' do
+    let_it_be(:current_user) { create(:user) }
+    let_it_be(:group) { create(:group, :public) }
+    let_it_be(:count_service) { Groups::OpenIssuesCountService }
+
+    before do
+      allow(helper).to receive(:current_user) { current_user }
+    end
+
+    context 'when cached_sidebar_open_issues_count feature flag is enabled' do
+      before do
+        stub_feature_flags(cached_sidebar_open_issues_count: true)
+      end
+
+      it 'returns count value from cache' do
+        allow_next_instance_of(count_service) do |service|
+          allow(service).to receive(:count).and_return(2500)
+        end
+
+        expect(helper.group_open_issues_count(group)).to eq('2.5k')
+      end
+    end
+
+    context 'when cached_sidebar_open_issues_count feature flag is disabled' do
+      before do
+        stub_feature_flags(cached_sidebar_open_issues_count: false)
+      end
+
+      it 'returns not cached issues count' do
+        allow(helper).to receive(:group_issues_count).and_return(2500)
+
+        expect(helper.group_open_issues_count(group)).to eq('2,500')
+      end
+    end
+  end
+
   describe '#cached_open_group_issues_count' do
     let_it_be(:current_user) { create(:user) }
     let_it_be(:group) { create(:group, name: 'group') }

spec/services/groups/open_issues_count_service_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac
 
   subject { described_class.new(group, user) }
 
-  describe '#self.query' do
+  describe '#relation_for_count' do
     before do
       allow(IssuesFinder).to receive(:new).and_call_original
     end
@@ -34,72 +34,70 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac
     end
 
     context 'when user is provided' do
-      let(:total_count_key) { subject.cache_key(described_class::TOTAL_COUNT_KEY) }
-
       context 'when user can read confidential issues' do
         before do
           group.add_reporter(user)
-          project.add_reporter(user)
         end
 
         it 'returns the right count with confidential issues' do
           expect(subject.count).to eq(2)
         end
-
-        it 'uses total_open_issues_count cache key' do
-          expect(subject.cache_key_name).to eq('group_total_open_issues_count')
       end
 
-        context 'when cache is empty' do
+      context 'when user cannot read confidential issues' do
         before do
-            Rails.cache.delete(total_count_key)
+          group.add_guest(user)
         end
 
-          it 'refreshes cache keys correctly' do
-            subject.count
-
-            expect(Rails.cache.read(total_count_key)).to eq(2)
+        it 'does not include confidential issues' do
+          expect(subject.count).to eq(1)
         end
       end
 
-        context 'when count is over the threshold value' do
+      context 'with different cache values' do
+        let(:public_count_key) { subject.cache_key(described_class::PUBLIC_COUNT_KEY) }
+        let(:under_threshold) { described_class::CACHED_COUNT_THRESHOLD - 1 }
+        let(:over_threshold) { described_class::CACHED_COUNT_THRESHOLD + 1 }
+
+        context 'when cache is empty' do
           before do
-            Rails.cache.write(total_count_key, 12345)
+            Rails.cache.delete(public_count_key)
           end
 
-          it 'does not refresh cache' do
-            expect(Rails.cache).not_to receive(:write)
+          it 'refreshes cache if value over threshold' do
+            allow(subject).to receive(:uncached_count).and_return(over_threshold)
 
-            subject.count
+            expect(subject.count).to eq(over_threshold)
+            expect(Rails.cache.read(public_count_key)).to eq(over_threshold)
           end
+
+          it 'does not refresh cache if value under threshold' do
+            allow(subject).to receive(:uncached_count).and_return(under_threshold)
+
+            expect(subject.count).to eq(under_threshold)
+            expect(Rails.cache.read(public_count_key)).to be_nil
           end
         end
 
-      context 'when user cannot read confidential issues' do
+        context 'when cached count is under the threshold value' do
           before do
-          group.add_guest(user)
-          project.add_guest(user)
+            Rails.cache.write(public_count_key, under_threshold)
           end
 
-        it 'does not include confidential issues' do
-          expect(subject.count).to eq(1)
+          it 'does not refresh cache' do
+            expect(Rails.cache).not_to receive(:write)
+            expect(subject.count).to eq(under_threshold)
           end
-
-        it 'uses public_open_issues_count cache key' do
-          expect(subject.cache_key_name).to eq('group_public_open_issues_count')
         end
 
-        context 'when cache is empty' do
-          let(:public_count_key) { subject.cache_key(described_class::PUBLIC_COUNT_KEY) }
-
+        context 'when cached count is over the threshold value' do
           before do
-            Rails.cache.delete(public_count_key)
+            Rails.cache.write(public_count_key, over_threshold)
           end
 
-          it 'refreshes cache keys correctly' do
-            subject.count
-
-            expect(Rails.cache.read(public_count_key)).to eq(1)
+          it 'does not refresh cache' do
+            expect(Rails.cache).not_to receive(:write)
+            expect(subject.count).to eq(over_threshold)
           end
         end
       end