Apply review suggestions

- Update Worker name
- Remove type argument from worker
- Refresh cache if existing value stored

app/services/groups/count_service.rb

@@ -22,8 +22,8 @@ module Groups
       refresh_cache_over_threshold
     end
 
-    def refresh_cache_over_threshold(new_count = nil)
-      new_count ||= uncached_count
+    def refresh_cache_over_threshold
+      new_count = uncached_count
 
       if new_count > CACHED_COUNT_THRESHOLD
         update_cache_for_key(cache_key) { new_count }

app/services/issuable/bulk_update_service.rb

@@ -17,8 +17,8 @@ module Issuable
       set_update_params(type)
       updated_issuables = update_issuables(type, ids)
 
-      if !updated_issuables.empty? && requires_count_cache_refresh?(type)
-        schedule_group_count_refresh(type, updated_issuables)
+      if updated_issuables.present? && requires_count_cache_refresh?(type)
+        schedule_group_issues_count_refresh(updated_issuables)
       end
 
       response_success(payload: { count: updated_issuables.size })
@@ -90,11 +90,11 @@ module Issuable
       type.to_sym == :issue && params.include?(:state_event)
     end
 
-    def schedule_group_count_refresh(issuable_type, updated_issuables)
-      group_ids = updated_issuables.map(&:project).map { |project| project.group&.id }.uniq
-      return unless group_ids.any?
+    def schedule_group_issues_count_refresh(updated_issuables)
+      group_ids = updated_issuables.map(&:project).map { |project| project.namespace_id }
+      return if group_ids.empty?
 
-      Issuables::RefreshGroupsCounterWorker.perform_async(issuable_type, current_user.id, group_ids)
+      Issuables::RefreshGroupsIssueCounterWorker.perform_async(current_user.id, group_ids)
     end
   end
 end

app/workers/all_queues.yml

@@ -2110,7 +2110,7 @@
   :weight: 1
   :idempotent:
   :tags: []
-- :name: issuables_refresh_groups_counter
+- :name: issuables_refresh_groups_issue_counter
   :feature_category: :issue_tracking
   :has_external_dependencies:
   :urgency: :low

app/workers/issuables/refresh_groups_counter_worker.rb → app/workers/issuables/refresh_groups_issue_counter_worker.rb

 # frozen_string_literal: true
 
 module Issuables
-  class RefreshGroupsCounterWorker
+  class RefreshGroupsIssueCounterWorker
     include ApplicationWorker
 
     idempotent!
     urgency :low
     feature_category :issue_tracking
 
-    # rubocop: disable CodeReuse/ActiveRecord
-    def perform(type, current_user_id, group_ids = [])
-      return unless group_ids.any? && issue_type?(type)
+    def perform(current_user_id, group_ids = [])
+      return if group_ids.empty?
 
       current_user = User.find(current_user_id)
-      groups_with_ancestors = Gitlab::ObjectHierarchy.new(Group.where(id: group_ids)).base_and_ancestors
+      groups_with_ancestors = Gitlab::ObjectHierarchy
+        .new(Group.by_id(group_ids))
+        .base_and_ancestors
+        .with_route
 
-      refresh_cached_count(type, current_user, groups_with_ancestors)
+      refresh_cached_count(current_user, groups_with_ancestors)
     rescue ActiveRecord::RecordNotFound => e
       Gitlab::ErrorTracking.log_exception(e, user_id: current_user_id)
     end
-    # rubocop: enable CodeReuse/ActiveRecord
 
     private
 
-    def refresh_cached_count(type, user, groups)
+    def refresh_cached_count(user, groups)
       groups.each do |group|
-        count_service = count_service_class(type)&.new(group, user)
-        next unless count_service&.count_stored?
-
-        count_service.refresh_cache_over_threshold
+        Groups::OpenIssuesCountService.new(group, user).refresh_cache_over_threshold
       end
     end
-
-    def count_service_class(type)
-      Groups::OpenIssuesCountService if issue_type?(type)
-    end
-
-    def issue_type?(type)
-      type.to_sym == :issue
-    end
   end
 end

config/sidekiq_queues.yml

@@ -190,7 +190,7 @@
   - 1
 - - issuable_export_csv
   - 1
-- - issuables_refresh_groups_counter
+- - issuables_refresh_groups_issue_counter
   - 1
 - - issue_placement
   - 2

spec/services/issuable/bulk_update_service_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Issuable::BulkUpdateService, :clean_gitlab_redis_cache do
+RSpec.describe Issuable::BulkUpdateService do
   let_it_be(:user)    { create(:user) }
   let_it_be(:project) { create(:project, :repository, namespace: user.namespace) }
 
@@ -101,6 +101,22 @@ RSpec.describe Issuable::BulkUpdateService, :clean_gitlab_redis_cache do
     end
   end
 
+  shared_examples 'scheduling cached group count refresh' do
+    it 'schedules worker' do
+      expect(Issuables::RefreshGroupsIssueCounterWorker).to receive(:perform_async)
+
+      bulk_update(issuables, params)
+    end
+  end
+
+  shared_examples 'not scheduling cached group count refresh' do
+    it 'does not schedule worker' do
+      expect(Issuables::RefreshGroupsIssueCounterWorker).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, :clean_gitlab_redis_cache do
         expect(project.issues.opened).to be_empty
         expect(project.issues.closed).not_to be_empty
       end
+
+      it_behaves_like 'scheduling cached group count refresh' do
+        let(:issuables) { issues }
+        let(:params) { { state_event: 'close' } }
+      end
     end
 
     describe 'reopen issues' do
@@ -149,6 +170,11 @@ RSpec.describe Issuable::BulkUpdateService, :clean_gitlab_redis_cache do
         expect(project.issues.closed).to be_empty
         expect(project.issues.opened).not_to be_empty
       end
+
+      it_behaves_like 'scheduling cached group count refresh' 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, :clean_gitlab_redis_cache do
       let(:milestone) { create(:milestone, project: project) }
 
       it_behaves_like 'updates milestones'
+
+      it_behaves_like 'not scheduling cached group count refresh' do
+        let(:params) { { milestone_id: milestone.id } }
+      end
     end
 
     describe 'updating labels' do
@@ -280,73 +310,6 @@ RSpec.describe Issuable::BulkUpdateService, :clean_gitlab_redis_cache do
         expect(issue2.reload.assignees).to be_empty
       end
     end
-
-    describe 'updating issuables cached count' do
-      shared_examples 'scheduling cached group count refresh' do
-        it 'schedules worker' do
-          expect(Issuables::RefreshGroupsCounterWorker).to receive(:perform_async)
-
-          bulk_update(issuables, params)
-        end
-      end
-
-      shared_examples 'not scheduling cached group count refresh' do
-        it 'does not schedule worker' do
-          expect(Issuables::RefreshGroupsCounterWorker).not_to receive(:perform_async)
-
-          bulk_update(issuables, params)
-        end
-      end
-
-      context 'when project belongs to a group' do
-        let_it_be(:group_project) { create(:project, :repository, group: create(:group)) }
-        let_it_be(:issues) { create_list(:issue, 2, :opened, project: group_project) }
-        let_it_be(:milestone) { create(:milestone, project: project) }
-        let(:parent) { group_project }
-        let(:issuables) { issues }
-
-        before do
-          group_project.add_reporter(user)
-        end
-
-        context 'when updating issues state' do
-          it_behaves_like 'scheduling cached group count refresh' do
-            let(:params) { { state_event: 'closed' } }
-          end
-
-          it_behaves_like 'scheduling cached group count refresh' do
-            let(:params) { { state_event: 'reopened' } }
-          end
-        end
-
-        context 'when state is not updated' do
-          it_behaves_like 'not scheduling cached group count refresh' do
-            let(:params) { { milestone_id: milestone.id } }
-          end
-        end
-
-        context 'when issuable type is not :issue' do
-          it_behaves_like 'not scheduling cached group count refresh' do
-            let(:params) { { state_event: 'closed' } }
-            let(:issuables) { [create(:merge_request, source_project: project, source_branch: 'branch-1')] }
-          end
-        end
-      end
-
-      context 'when project belongs to a user namespace' do
-        let(:issuables) { create_list(:issue, 2, :opened, project: project) }
-
-        context 'when updating issues state' do
-          it_behaves_like 'not scheduling cached group count refresh' do
-            let(:params) { { state_event: 'closed' } }
-          end
-
-          it_behaves_like 'not scheduling cached group count refresh' do
-            let(:params) { { state_event: 'reopened' } }
-          end
-        end
-      end
-    end
   end
 
   context 'with issuables at a group level' do

spec/workers/issuables/refresh_groups_counter_worker_spec.rb → spec/workers/issuables/refresh_groups_issue_counter_worker_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Issuables::RefreshGroupsCounterWorker do
+RSpec.describe Issuables::RefreshGroupsIssueCounterWorker do
   describe '#perform' do
     let_it_be(:user) { create(:user) }
     let_it_be(:parent_group) { create(:group) }
@@ -10,81 +10,47 @@ RSpec.describe Issuables::RefreshGroupsCounterWorker do
     let_it_be(:subgroup) { create(:group, parent: root_group) }
 
     let(:count_service) { Groups::OpenIssuesCountService }
-    let(:type) { 'issue' }
     let(:group_ids) { [root_group.id] }
 
-    shared_examples 'a worker that takes no action' do
-      it 'does not call count service or rise error' do
-        expect(count_service).not_to receive(:new)
-        expect(Gitlab::ErrorTracking).not_to receive(:log_exception)
-
-        described_class.new.perform(type, user.id, group_ids)
-      end
-    end
-
     it 'anticipates the inability to find the issue' do
       expect(Gitlab::ErrorTracking).to receive(:log_exception)
         .with(ActiveRecord::RecordNotFound, include(user_id: -1))
       expect(count_service).not_to receive(:new)
 
-      described_class.new.perform(type, -1, group_ids)
+      described_class.new.perform(-1, group_ids)
     end
 
     context 'when group_ids is empty' do
       let(:group_ids) { [] }
 
-      it_behaves_like 'a worker that takes no action'
-    end
-
-    context 'when type is not issue' do
-      let(:type) { 'merge_request' }
+      it 'does not call count service or rise error' do
+        expect(count_service).not_to receive(:new)
+        expect(Gitlab::ErrorTracking).not_to receive(:log_exception)
 
-      it_behaves_like 'a worker that takes no action'
+        described_class.new.perform(user.id, group_ids)
+      end
     end
 
     context 'when updating cache' do
       let(:instance1) { instance_double(count_service) }
       let(:instance2) { instance_double(count_service) }
 
-      context 'with existing cached value' do
-        before do
-          allow(instance1).to receive(:count_stored?).and_return(true)
-          allow(instance2).to receive(:count_stored?).and_return(true)
-        end
-
-        it_behaves_like 'an idempotent worker' do
-          let(:job_args) { [type, user.id, group_ids] }
-          let(:exec_times) { IdempotentWorkerHelper::WORKER_EXEC_TIMES }
-
-          it 'refreshes the issue count in given groups and ancestors' do
-            expect(count_service).to receive(:new)
-              .exactly(exec_times).times.with(root_group, user).and_return(instance1)
-            expect(count_service).to receive(:new)
-              .exactly(exec_times).times.with(parent_group, user).and_return(instance2)
-            expect(count_service).not_to receive(:new).with(subgroup, user)
-
-            [instance1, instance2].all? do |instance|
-              expect(instance).to receive(:refresh_cache_over_threshold).exactly(exec_times).times
-            end
-
-            subject
-          end
-        end
-      end
-
-      context 'with no cached value' do
-        before do
-          allow(instance1).to receive(:count_stored?).and_return(false)
-          allow(instance2).to receive(:count_stored?).and_return(false)
-        end
+      it_behaves_like 'an idempotent worker' do
+        let(:job_args) { [user.id, group_ids] }
+        let(:exec_times) { IdempotentWorkerHelper::WORKER_EXEC_TIMES }
 
         it 'refreshes the issue count in given groups and ancestors' do
-          expect(count_service).to receive(:new).with(root_group, user).and_return(instance1)
-          expect(count_service).to receive(:new).with(parent_group, user).and_return(instance2)
-
-          [instance1, instance2].all? {|i| expect(i).not_to receive(:refresh_cache_over_threshold) }
+          expect(count_service).to receive(:new)
+            .exactly(exec_times).times.with(root_group, user).and_return(instance1)
+          expect(count_service).to receive(:new)
+            .exactly(exec_times).times.with(parent_group, user).and_return(instance2)
+          expect(count_service).not_to receive(:new).with(subgroup, user)
+
+          [instance1, instance2].all? do |instance|
+            expect(instance).to receive(:refresh_cache_over_threshold).exactly(exec_times).times
+          end
 
-          described_class.new.perform(type, user.id, group_ids)
+          subject
         end
       end
     end