Merge branch '299178-implement-refresh_cache-for-issues-count-in-group-sidebar' into 'master'

Clear open issues count cache after bulk updating issues state

See merge request gitlab-org/gitlab!56386

app/services/groups/count_service.rb

@@ -19,13 +19,26 @@ module Groups
       cached_count = Rails.cache.read(cache_key)
       return cached_count unless cached_count.blank?
 
-      refreshed_count = uncached_count
-      update_cache_for_key(cache_key) { refreshed_count } if refreshed_count > CACHED_COUNT_THRESHOLD
-      refreshed_count
+      refresh_cache_over_threshold
     end
 
-    def cache_key
-      ['groups', "#{issuable_key}_count_service", VERSION, group.id, cache_key_name]
+    def refresh_cache_over_threshold(key = nil)
+      key ||= cache_key
+      new_count = uncached_count
+
+      if new_count > CACHED_COUNT_THRESHOLD
+        update_cache_for_key(key) { new_count }
+      else
+        delete_cache
+      end
+
+      new_count
+    end
+
+    def cache_key(key_name = nil)
+      key_name ||= cache_key_name
+
+      ['groups', "#{issuable_key}_count_service", VERSION, group.id, key_name]
     end
 
     private

app/services/groups/open_issues_count_service.rb

@@ -6,6 +6,12 @@ module Groups
     PUBLIC_COUNT_KEY = 'group_public_open_issues_count'
     TOTAL_COUNT_KEY = 'group_total_open_issues_count'
 
+    def clear_all_cache_keys
+      [cache_key(PUBLIC_COUNT_KEY), cache_key(TOTAL_COUNT_KEY)].each do |key|
+        Rails.cache.delete(key)
+      end
+    end
+
     private
 
     def cache_key_name
@@ -23,7 +29,14 @@ module Groups
     end
 
     def relation_for_count
-      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
 
     def issuable_key

app/services/issuable/bulk_update_service.rb

@@ -15,9 +15,13 @@ module Issuable
     def execute(type)
       ids = params.delete(:issuable_ids).split(",")
       set_update_params(type)
-      items = update_issuables(type, ids)
+      updated_issuables = update_issuables(type, ids)
 
-      response_success(payload: { count: items.size })
+      if updated_issuables.present? && requires_count_cache_reset?(type)
+        schedule_group_issues_count_reset(updated_issuables)
+      end
+
+      response_success(payload: { count: updated_issuables.size })
     rescue ArgumentError => e
       response_error(e.message, 422)
     end
@@ -81,6 +85,17 @@ module Issuable
     def response_error(message, http_status)
       ServiceResponse.error(message: message, http_status: http_status)
     end
+
+    def requires_count_cache_reset?(type)
+      type.to_sym == :issue && params.include?(:state_event)
+    end
+
+    def schedule_group_issues_count_reset(updated_issuables)
+      group_ids = updated_issuables.map(&:project).map(&:namespace_id)
+      return if group_ids.empty?
+
+      Issuables::ClearGroupsIssueCounterWorker.perform_async(group_ids)
+    end
   end
 end
 

app/workers/all_queues.yml

@@ -2119,6 +2119,15 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: issuables_clear_groups_issue_counter
+  :worker_name: Issuables::ClearGroupsIssueCounterWorker
+  :feature_category: :issue_tracking
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: issue_placement
   :worker_name: IssuePlacementWorker
   :feature_category: :issue_tracking

app/workers/issuables/clear_groups_issue_counter_worker.rb

+# frozen_string_literal: true
+
+module Issuables
+  class ClearGroupsIssueCounterWorker
+    include ApplicationWorker
+
+    idempotent!
+    urgency :low
+    feature_category :issue_tracking
+
+    def perform(group_ids = [])
+      return if group_ids.empty?
+
+      groups_with_ancestors = Gitlab::ObjectHierarchy
+        .new(Group.by_id(group_ids))
+        .base_and_ancestors
+
+      clear_cached_count(groups_with_ancestors)
+    end
+
+    private
+
+    def clear_cached_count(groups)
+      groups.each do |group|
+        Groups::OpenIssuesCountService.new(group).clear_all_cache_keys
+      end
+    end
+  end
+end

changelogs/unreleased/299178-implement-refresh_cache-for-issues-count-in-group-sidebar.yml

+---
+title: Clear group open issues count cache when bulk updating issues state
+merge_request: 56386
+author:
+type: added

config/sidekiq_queues.yml

@@ -190,6 +190,8 @@
   - 1
 - - issuable_export_csv
   - 1
+- - issuables_clear_groups_issue_counter
+  - 1
 - - issue_placement
   - 2
 - - issue_rebalancing

spec/services/groups/open_issues_count_service_spec.rb

@@ -57,4 +57,15 @@ RSpec.describe Groups::OpenIssuesCountService, :use_clean_rails_memory_store_cac
       it_behaves_like 'a counter caching service with threshold'
     end
   end
+
+  describe '#clear_all_cache_keys' do
+    it 'calls `Rails.cache.delete` with the correct keys' do
+      expect(Rails.cache).to receive(:delete)
+        .with(['groups', 'open_issues_count_service', 1, group.id, described_class::PUBLIC_COUNT_KEY])
+      expect(Rails.cache).to receive(:delete)
+        .with(['groups', 'open_issues_count_service', 1, group.id, described_class::TOTAL_COUNT_KEY])
+
+      subject.clear_all_cache_keys
+    end
+  end
 end

spec/services/issuable/bulk_update_service_spec.rb

@@ -101,6 +101,22 @@ RSpec.describe Issuable::BulkUpdateService do
     end
   end
 
+  shared_examples 'scheduling cached group count clear' do
+    it 'schedules worker' do
+      expect(Issuables::ClearGroupsIssueCounterWorker).to receive(:perform_async)
+
+      bulk_update(issuables, params)
+    end
+  end
+
+  shared_examples 'not scheduling cached group count clear' do
+    it 'does not schedule worker' do
+      expect(Issuables::ClearGroupsIssueCounterWorker).not_to receive(:perform_async)
+
+      bulk_update(issuables, params)
+    end
+  end
+
   context 'with issuables at a project level' do
     let(:parent) { project }
 
@@ -131,6 +147,11 @@ RSpec.describe Issuable::BulkUpdateService do
         expect(project.issues.opened).to be_empty
         expect(project.issues.closed).not_to be_empty
       end
+
+      it_behaves_like 'scheduling cached group count clear' do
+        let(:issuables) { issues }
+        let(:params) { { state_event: 'close' } }
+      end
     end
 
     describe 'reopen issues' do
@@ -149,6 +170,11 @@ RSpec.describe Issuable::BulkUpdateService do
         expect(project.issues.closed).to be_empty
         expect(project.issues.opened).not_to be_empty
       end
+
+      it_behaves_like 'scheduling cached group count clear' do
+        let(:issuables) { issues }
+        let(:params) { { state_event: 'reopen' } }
+      end
     end
 
     describe 'updating merge request assignee' do
@@ -231,6 +257,10 @@ RSpec.describe Issuable::BulkUpdateService do
       let(:milestone) { create(:milestone, project: project) }
 
       it_behaves_like 'updates milestones'
+
+      it_behaves_like 'not scheduling cached group count clear' do
+        let(:params) { { milestone_id: milestone.id } }
+      end
     end
 
     describe 'updating labels' do

spec/workers/issuables/clear_groups_issue_counter_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Issuables::ClearGroupsIssueCounterWorker do
+  describe '#perform' do
+    let_it_be(:user) { create(:user) }
+    let_it_be(:parent_group) { create(:group) }
+    let_it_be(:root_group) { create(:group, parent: parent_group) }
+    let_it_be(:subgroup) { create(:group, parent: root_group) }
+
+    let(:count_service) { Groups::OpenIssuesCountService }
+    let(:instance1) { instance_double(count_service) }
+    let(:instance2) { instance_double(count_service) }
+
+    it_behaves_like 'an idempotent worker' do
+      let(:job_args) { [[root_group.id]] }
+      let(:exec_times) { IdempotentWorkerHelper::WORKER_EXEC_TIMES }
+
+      it 'clears the cached issue count in given groups and ancestors' do
+        expect(count_service).to receive(:new)
+          .exactly(exec_times).times.with(root_group).and_return(instance1)
+        expect(count_service).to receive(:new)
+          .exactly(exec_times).times.with(parent_group).and_return(instance2)
+        expect(count_service).not_to receive(:new).with(subgroup)
+
+        [instance1, instance2].all? do |instance|
+          expect(instance).to receive(:clear_all_cache_keys).exactly(exec_times).times
+        end
+
+        subject
+      end
+    end
+
+    it 'does not call count service or rise error when group_ids is empty' do
+      expect(count_service).not_to receive(:new)
+      expect(Gitlab::ErrorTracking).not_to receive(:log_exception)
+
+      described_class.new.perform([])
+    end
+  end
+end