Merge branch '422955-remove-dup-epics-in-grp-transfer' into 'master'

Add a worker to remove duplicate epic on group transfer

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/134524



Merged-by: Dmitry Gruzd <dgruzd@gitlab.com>
Approved-by: Ravi Kumar <rkumar@gitlab.com>
Reviewed-by: Dmitry Gruzd <dgruzd@gitlab.com>
Reviewed-by: Terri Chu <tchu@gitlab.com>
Co-authored-by: Siddharth Dungarwal <sdungarwal@gitlab.com>

ee/app/services/ee/groups/transfer_service.rb

@@ -60,6 +60,8 @@ def post_update_hooks(updated_project_ids, old_root_ancestor_id)
         end
 
         process_wikis(group)
+
+        process_epics(old_root_ancestor_id, group)
       end
 
       def update_project_settings(updated_project_ids)
@@ -85,6 +87,24 @@ def process_elasticsearch_project(project, elasticsearch_limit_indexing_enabled)
         ::Elastic::ProcessInitialBookkeepingService.backfill_projects!(project) if project.maintaining_elasticsearch?
       end
 
+      def process_epics(old_root_ancestor_id, group)
+        return unless group.use_elasticsearch? && ::Epic.elasticsearch_available?
+
+        epics_found = false
+        group.self_and_descendants.each_batch do |group_batch|
+          ::Epic.in_selected_groups(group_batch).each_batch do |epics|
+            epics_found ||= true
+            ::Elastic::ProcessInitialBookkeepingService.track!(*epics)
+          end
+        end
+
+        return unless epics_found
+        return if old_root_ancestor_id == group.root_ancestor.id && group.licensed_feature_available?(:epics)
+
+        ::Search::ElasticGroupAssociationDeletionWorker.perform_async(group.id, old_root_ancestor_id,
+          { include_descendants: true })
+      end
+
       def process_wikis(group)
         return unless ::Wiki.use_separate_indices?
 

ee/app/workers/search/elastic_group_association_deletion_worker.rb

@@ -13,14 +13,28 @@ class ElasticGroupAssociationDeletionWorker
     urgency :throttled
     idempotent!
 
-    def perform(group_id, ancestor_id)
-      remove_epics(group_id, ancestor_id) if Epic.elasticsearch_available?
+    def perform(group_id, ancestor_id, options = {})
+      return unless ::Epic.elasticsearch_available?
+
+      return remove_epics(group_id, ancestor_id) unless options[:include_descendants]
+
+      # We have the return condition here because we still want to remove the deleted group epics in the above call
+      group = Group.find_by_id(group_id)
+      return if group.nil?
+
+      # rubocop: disable CodeReuse/ActiveRecord -- We need only the ids of self_and_descendants groups
+      group.self_and_descendants.each_batch { |groups| remove_epics(groups.pluck(:id), ancestor_id) }
+      # rubocop: enable CodeReuse/ActiveRecord
     end
 
     private
 
-    def remove_epics(group_id, ancestor_id)
-      Gitlab::Search::Client.new.delete_by_query(
+    def client
+      @client ||= ::Gitlab::Search::Client.new
+    end
+
+    def remove_epics(group_ids, ancestor_id)
+      client.delete_by_query(
         {
           index: ::Elastic::Latest::EpicConfig.index_name,
           routing: "group_#{ancestor_id}",
@@ -29,11 +43,7 @@ def remove_epics(group_id, ancestor_id)
           body: {
             query: {
               bool: {
-                filter: {
-                  term: {
-                    group_id: group_id
-                  }
-                }
+                filter: { terms: { group_id: Array.wrap(group_ids) } }
               }
             }
           }

ee/spec/elastic_integration/epic_index_spec.rb

@@ -173,11 +173,14 @@
     context 'when the group is transferred', :sidekiq_inline do
       it 'tracks the epic via Elastic::NamespaceUpdateWorker' do
         expect(Elastic::NamespaceUpdateWorker).to receive(:perform_async).with(group.id).and_call_original
+        expect(Search::ElasticGroupAssociationDeletionWorker).to receive(:perform_async)
+          .with(group.id, parent_group.id, { include_descendants: true }).once
         expect(ElasticAssociationIndexerWorker).to receive(:perform_async)
           .with(anything, group.id, [:epics])
           .and_call_original
 
         expect(::Elastic::ProcessBookkeepingService).to receive(:track!).with(epic).once
+        expect(::Elastic::ProcessInitialBookkeepingService).to receive(:track!).with(epic).once
         Groups::TransferService.new(group, user).execute(another_group)
       end
     end

ee/spec/services/groups/transfer_service_spec.rb

@@ -3,6 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe Groups::TransferService, '#execute', feature_category: :groups_and_projects do
+  include ElasticsearchHelpers
   let_it_be(:user) { create(:user) }
 
   let(:group) { create(:group, :public) }
@@ -286,6 +287,8 @@
   context 'with epics' do
     context 'when epics feature is disabled' do
       it 'transfers a group successfully' do
+        expect(::Search::ElasticGroupAssociationDeletionWorker).not_to receive(:perform_async)
+        expect(::Elastic::ProcessInitialBookkeepingService).not_to receive(:track!)
         transfer_service.execute(new_group)
 
         expect(group.parent).to eq(new_group)
@@ -308,12 +311,15 @@
 
       before do
         root_group.add_owner(user)
-
+        set_elasticsearch_migration_to :create_epic_index, including: true
+        stub_ee_application_setting(elasticsearch_indexing: true)
         stub_licensed_features(epics: true)
       end
 
       context 'when group is moved completely out of the main group' do
         it 'keeps relations between all epics' do
+          expect(::Search::ElasticGroupAssociationDeletionWorker).to receive(:perform_async).with(subgroup_group_level_1.id, root_group.id, { include_descendants: true }).once
+          expect(::Elastic::ProcessInitialBookkeepingService).to receive(:track!).once
           described_class.new(subgroup_group_level_1, user).execute(new_group)
 
           expect(level_1_epic_2.reload.parent).to eq(level_1_epic_1)
@@ -327,6 +333,10 @@
 
       context 'when group is moved some levels up' do
         it 'keeps relations between all epics' do
+          expect(::Search::ElasticGroupAssociationDeletionWorker).not_to receive(:perform_async)
+          expect(::Elastic::ProcessInitialBookkeepingService).to receive(:track!) do |*args|
+            expect(args).to match_array(subgroup_group_level_2.self_and_descendants.flat_map(&:epics))
+          end
           described_class.new(subgroup_group_level_2, user).execute(root_group)
 
           expect(level_1_epic_1.reload.parent).to eq(root_epic)

ee/spec/workers/search/elastic_group_association_deletion_worker_spec.rb

@@ -8,6 +8,7 @@
 
     let_it_be(:parent_group) { create(:group) }
     let_it_be(:group) { create(:group, parent: parent_group) }
+    let_it_be(:other_group) { create(:group, parent: group) }
     let(:epic_index) { Epic.__elasticsearch__.index_name }
     let(:helper) { Gitlab::Elastic::Helper.default }
     let(:client) { helper.client }
@@ -35,6 +36,26 @@
           allow(Epic).to receive(:elasticsearch_available?).and_return(true)
         end
 
+        it 'deletes epics belonging to the group and its escendants' do
+          group_epic = create(:epic, group: group)
+          other_group_epic = create(:epic, group: other_group)
+          create(:epic)
+
+          ensure_elasticsearch_index!
+
+          expect(epics_in_index.count).to eq(3)
+          expect(epics_in_index).to include(group_epic.id)
+          expect(epics_in_index).to include(other_group_epic.id)
+
+          described_class.new.perform(group.id, parent_group.id, { include_descendants: true })
+
+          helper.refresh_index(index_name: epic_index)
+
+          expect(epics_in_index.count).to eq(1)
+          expect(epics_in_index).not_to include(group_epic.id)
+          expect(epics_in_index).not_to include(other_group_epic.id)
+        end
+
         it 'deletes epics belonging to the group' do
           group_epic = create(:epic, group: group)
           create(:epic)