Merge branch '443557-check-placeholder-references-loaded-before-finishing' into 'master'

Check references all saved before finishing import

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



Merged-by: Rodrigo Tomonari <rtomonari@gitlab.com>
Approved-by: Sam Word <sword@gitlab.com>
Approved-by: Rodrigo Tomonari <rtomonari@gitlab.com>
Co-authored-by: Luke Duncalfe <lduncalfe@eml.cc>

app/presenters/import/pending_reassignment_alert_presenter.rb

@@ -9,11 +9,13 @@ class PendingReassignmentAlertPresenter < Gitlab::View::Presenter::Simple
     presents ::BulkImport, as: :bulk_import
 
     def show_alert?
-      Feature.enabled?(:importer_user_mapping, current_user) && groups_awaiting_placeholder_assignment.any?
+      Feature.enabled?(:importer_user_mapping, current_user) &&
+        Feature.enabled?(:bulk_import_importer_user_mapping, current_user) &&
+        groups_awaiting_placeholder_assignment.any?
     end
 
     def groups_awaiting_placeholder_assignment
-      return [] unless bulk_import
+      return [] unless bulk_import&.finished?
 
       namespaces = bulk_import.namespaces_with_unassigned_placeholders
       namespaces.select do |namespace|

app/services/bulk_imports/process_service.rb

@@ -15,7 +15,7 @@ def execute
       return unless bulk_import
       return if bulk_import.completed?
       return bulk_import.fail_op! if all_entities_failed?
-      return bulk_import.finish! if all_entities_processed? && bulk_import.started?
+      return bulk_import.finish! if all_entities_processed? && bulk_import.started? && placeholder_references_loaded?
       return re_enqueue if max_batch_size_exceeded? # Do not start more jobs if max allowed are already running
 
       process_bulk_import
@@ -54,6 +54,25 @@ def all_entities_failed?
       entities.all?(&:failed?)
     end
 
+    def placeholder_references_loaded?
+      return true unless importer_user_mapping_enabled?
+
+      store = Import::PlaceholderReferences::Store.new(
+        import_source: Import::SOURCE_DIRECT_TRANSFER,
+        import_uid: bulk_import.id
+      )
+
+      return true if store.empty?
+
+      logger.info(
+        message: 'Placeholder references not finished loading to database',
+        bulk_import_id: bulk_import.id,
+        placeholder_reference_store_count: store.count
+      )
+
+      false
+    end
+
     # A new BulkImportWorker job is enqueued to either
     #   - Process the new BulkImports::Entity created during import (e.g. for the subgroups)
     #   - Or to mark the `bulk_import` as finished
@@ -69,6 +88,10 @@ def max_batch_size_exceeded?
       started_entities.count >= DEFAULT_BATCH_SIZE
     end
 
+    def importer_user_mapping_enabled?
+      Import::BulkImports::EphemeralData.new(bulk_import.id).importer_user_mapping_enabled?
+    end
+
     def next_batch_size
       [DEFAULT_BATCH_SIZE - started_entities.count, 0].max
     end

spec/presenters/import/pending_reassignment_alert_presenter_spec.rb

@@ -6,7 +6,7 @@
   include SafeFormatHelper
 
   let_it_be(:user) { build_stubbed(:user) }
-  let_it_be(:bulk_import) { build_stubbed(:bulk_import, :with_configuration) }
+  let(:bulk_import) { build_stubbed(:bulk_import, :with_configuration, :finished) }
   let(:presenter) { described_class.new(bulk_import, current_user: user) }
   let_it_be(:namespaces) { [] }
 
@@ -44,7 +44,7 @@
   context 'with no top level groups' do
     let_it_be(:namespaces) { [] }
 
-    it 'presents the import values' do
+    it 'does not present the import values' do
       expect(presenter.show_alert?).to eq(false)
     end
   end
@@ -68,7 +68,27 @@
         stub_feature_flags(importer_user_mapping: false)
       end
 
-      it 'presents the import values' do
+      it 'does not present the import values' do
+        expect(presenter.show_alert?).to eq(false)
+      end
+    end
+
+    context 'when bulk_import_importer_user_mapping feature flag is disabled' do
+      before do
+        stub_feature_flags(bulk_import_importer_user_mapping: false)
+      end
+
+      it 'does not present the import values' do
+        expect(presenter.show_alert?).to eq(false)
+      end
+    end
+
+    context 'when import has not finished' do
+      before do
+        bulk_import.status = 1
+      end
+
+      it 'does not present the import values' do
         expect(presenter.show_alert?).to eq(false)
       end
     end

spec/services/bulk_imports/process_service_spec.rb

@@ -45,15 +45,55 @@
     end
 
     context 'when all entities are processed' do
-      it 'marks bulk import as finished' do
+      before do
         bulk_import.update!(status: 1)
         create(:bulk_import_entity, :finished, bulk_import: bulk_import)
         create(:bulk_import_entity, :failed, bulk_import: bulk_import)
+      end
 
+      it 'marks bulk import as finished' do
         subject.execute
 
         expect(bulk_import.reload.finished?).to eq(true)
       end
+
+      context 'when placeholder references have not finished being loaded to the database' do
+        before do
+          allow_next_instance_of(Import::PlaceholderReferences::Store) do |store|
+            allow(store).to receive(:empty?).and_return(false)
+            allow(store).to receive(:count).and_return(1)
+          end
+        end
+
+        it 'marks bulk import as finished' do
+          subject.execute
+
+          expect(bulk_import.reload.finished?).to eq(true)
+        end
+
+        context 'when importer_user_mapping_enabled is enabled' do
+          before do
+            allow_next_instance_of(Import::BulkImports::EphemeralData) do |ephemeral_data|
+              allow(ephemeral_data).to receive(:importer_user_mapping_enabled?).and_return(true)
+            end
+          end
+
+          it 'logs and re-enqueues the worker' do
+            expect(BulkImportWorker).to receive(:perform_in).with(described_class::PERFORM_DELAY, bulk_import.id)
+            expect_next_instance_of(BulkImports::Logger) do |logger|
+              expect(logger).to receive(:info).with(
+                message: 'Placeholder references not finished loading to database',
+                bulk_import_id: bulk_import.id,
+                placeholder_reference_store_count: 1
+              )
+            end
+
+            subject.execute
+
+            expect(bulk_import.reload.started?).to eq(true)
+          end
+        end
+      end
     end
 
     context 'when all entities are failed' do