Merge branch 'georgekoltsov/enhance-group-migration-resiliency' into 'master'

Add better error handling to BulkImports::GroupLoader

See merge request gitlab-org/gitlab!73191

lib/bulk_imports/groups/loaders/group_loader.rb

@@ -4,10 +4,21 @@ module BulkImports
   module Groups
     module Loaders
       class GroupLoader
+        GroupCreationError = Class.new(StandardError)
+
         def load(context, data)
-          return unless user_can_create_group?(context.current_user, data)
+          path = data['path']
+          current_user = context.current_user
+          destination_namespace = context.entity.destination_namespace
+
+          raise(GroupCreationError, 'Path is missing') unless path.present?
+          raise(GroupCreationError, 'Destination is not a group') if user_namespace_destination?(destination_namespace)
+          raise(GroupCreationError, 'User not allowed to create group') unless user_can_create_group?(current_user, data)
+          raise(GroupCreationError, 'Group exists') if group_exists?(destination_namespace, path)
+
+          group = ::Groups::CreateService.new(current_user, data).execute
 
-          group = ::Groups::CreateService.new(context.current_user, data).execute
+          raise(GroupCreationError, group.errors.full_messages.to_sentence) if group.errors.any?
 
           context.entity.update!(group: group)
 
@@ -25,6 +36,18 @@ module BulkImports
             Ability.allowed?(current_user, :create_group)
           end
         end
+
+        def group_exists?(destination_namespace, path)
+          full_path = destination_namespace.present? ? File.join(destination_namespace, path) : path
+
+          Group.find_by_full_path(full_path).present?
+        end
+
+        def user_namespace_destination?(destination_namespace)
+          return false unless destination_namespace.present?
+
+          Namespace.find_by_full_path(destination_namespace)&.user_namespace?
+        end
       end
     end
   end

spec/lib/bulk_imports/groups/loaders/group_loader_spec.rb

@@ -11,20 +11,66 @@ RSpec.describe BulkImports::Groups::Loaders::GroupLoader do
     let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) }
 
     let(:service_double) { instance_double(::Groups::CreateService) }
-    let(:data) { { foo: :bar } }
+    let(:data) { { 'path' => 'test' } }
 
     subject { described_class.new }
 
+    context 'when path is missing' do
+      it 'raises an error' do
+        expect { subject.load(context, {}) }.to raise_error(described_class::GroupCreationError, 'Path is missing')
+      end
+    end
+
+    context 'when destination namespace is not a group' do
+      it 'raises an error' do
+        entity.update!(destination_namespace: user.namespace.path)
+
+        expect { subject.load(context, data) }.to raise_error(described_class::GroupCreationError, 'Destination is not a group')
+      end
+    end
+
+    context 'when group exists' do
+      it 'raises an error' do
+        group1 = create(:group)
+        group2 = create(:group, parent: group1)
+        entity.update!(destination_namespace: group1.full_path)
+        data = { 'path' => group2.path }
+
+        expect { subject.load(context, data) }.to raise_error(described_class::GroupCreationError, 'Group exists')
+      end
+    end
+
+    context 'when there are other group errors' do
+      it 'raises an error with those errors' do
+        group = ::Group.new
+        group.validate
+        expected_errors = group.errors.full_messages.to_sentence
+
+        expect(::Groups::CreateService)
+          .to receive(:new)
+          .with(context.current_user, data)
+          .and_return(service_double)
+
+        expect(service_double).to receive(:execute).and_return(group)
+        expect(entity).not_to receive(:update!)
+
+        expect { subject.load(context, data) }.to raise_error(described_class::GroupCreationError, expected_errors)
+      end
+    end
+
     context 'when user can create group' do
       shared_examples 'calls Group Create Service to create a new group' do
         it 'calls Group Create Service to create a new group' do
+          group_double = instance_double(::Group)
+
           expect(::Groups::CreateService)
             .to receive(:new)
             .with(context.current_user, data)
             .and_return(service_double)
 
-          expect(service_double).to receive(:execute)
-          expect(entity).to receive(:update!)
+          expect(service_double).to receive(:execute).and_return(group_double)
+          expect(group_double).to receive(:errors).and_return([])
+          expect(entity).to receive(:update!).with(group: group_double)
 
           subject.load(context, data)
         end
@@ -40,7 +86,7 @@ RSpec.describe BulkImports::Groups::Loaders::GroupLoader do
 
       context 'when there is parent group' do
         let(:parent) { create(:group) }
-        let(:data) { { 'parent_id' => parent.id } }
+        let(:data) { { 'parent_id' => parent.id, 'path' => 'test' } }
 
         before do
           allow(Ability).to receive(:allowed?).with(user, :create_subgroup, parent).and_return(true)
@@ -55,7 +101,7 @@ RSpec.describe BulkImports::Groups::Loaders::GroupLoader do
         it 'does not create new group' do
           expect(::Groups::CreateService).not_to receive(:new)
 
-          subject.load(context, data)
+          expect { subject.load(context, data) }.to raise_error(described_class::GroupCreationError, 'User not allowed to create group')
         end
       end
 
@@ -69,7 +115,7 @@ RSpec.describe BulkImports::Groups::Loaders::GroupLoader do
 
       context 'when there is parent group' do
         let(:parent) { create(:group) }
-        let(:data) { { 'parent_id' => parent.id } }
+        let(:data) { { 'parent_id' => parent.id, 'path' => 'test' } }
 
         before do
           allow(Ability).to receive(:allowed?).with(user, :create_subgroup, parent).and_return(false)