From 01236e0c0bab4206f516d811dd69078171d4c44a Mon Sep 17 00:00:00 2001
From: Rodrigo Tomonari <rtomonari@gitlab.com>
Date: Mon, 4 Jul 2022 14:46:23 +0000
Subject: [PATCH] Fix group name conflict when migrating groups via BulkImport
Modify BulkImport to make group names unique as a namespace can not
have subgroups with the same name.
Changelog: fixed
---
.../group_attributes_transformer.rb | 28 ++++++--
.../group_attributes_transformer_spec.rb | 65 ++++++++++++++++---
2 files changed, 79 insertions(+), 14 deletions(-)
diff --git a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb
index df27275b664b1..3067e0997c243 100644
--- a/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb
+++ b/lib/bulk_imports/groups/transformers/group_attributes_transformer.rb
@@ -7,10 +7,15 @@ class GroupAttributesTransformer
def transform(context, data)
import_entity = context.entity
+ if import_entity.destination_namespace.present?
+ namespace = Namespace.find_by_full_path(import_entity.destination_namespace)
+ end
+
data
+ .then { |data| transform_name(import_entity, namespace, data) }
.then { |data| transform_path(import_entity, data) }
.then { |data| transform_full_path(data) }
- .then { |data| transform_parent(context, import_entity, data) }
+ .then { |data| transform_parent(context, import_entity, namespace, data) }
.then { |data| transform_visibility_level(data) }
.then { |data| transform_project_creation_level(data) }
.then { |data| transform_subgroup_creation_level(data) }
@@ -18,6 +23,20 @@ def transform(context, data)
private
+ def transform_name(import_entity, namespace, data)
+ if namespace.present?
+ namespace_children_names = namespace.children.pluck(:name) # rubocop: disable CodeReuse/ActiveRecord
+
+ if namespace_children_names.include?(data['name'])
+ data['name'] = Uniquify.new(1).string(-> (counter) { "#{data['name']}(#{counter})" }) do |base|
+ namespace_children_names.include?(base)
+ end
+ end
+ end
+
+ data
+ end
+
def transform_path(import_entity, data)
data['path'] = import_entity.destination_name.parameterize
data
@@ -28,11 +47,8 @@ def transform_full_path(data)
data
end
- def transform_parent(context, import_entity, data)
- unless import_entity.destination_namespace.blank?
- namespace = Namespace.find_by_full_path(import_entity.destination_namespace)
- data['parent_id'] = namespace.id
- end
+ def transform_parent(context, import_entity, namespace, data)
+ data['parent_id'] = namespace.id if namespace.present?
data
end
diff --git a/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb b/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb
index c42ca9bef3be6..d775cf6b02679 100644
--- a/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb
+++ b/spec/lib/bulk_imports/groups/transformers/group_attributes_transformer_spec.rb
@@ -4,12 +4,12 @@
RSpec.describe BulkImports::Groups::Transformers::GroupAttributesTransformer do
describe '#transform' do
- let_it_be(:user) { create(:user) }
let_it_be(:parent) { create(:group) }
- let_it_be(:bulk_import) { create(:bulk_import, user: user) }
- let_it_be(:entity) do
- create(
+ let(:bulk_import) { build_stubbed(:bulk_import) }
+
+ let(:entity) do
+ build_stubbed(
:bulk_import_entity,
bulk_import: bulk_import,
source_full_path: 'source/full/path',
@@ -18,8 +18,8 @@
)
end
- let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) }
- let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) }
+ let(:tracker) { build_stubbed(:bulk_import_tracker, entity: entity) }
+ let(:context) { BulkImports::Pipeline::Context.new(tracker) }
let(:data) do
{
@@ -87,14 +87,63 @@
end
context 'when destination namespace is empty' do
- it 'does not set parent id' do
- entity.update!(destination_namespace: '')
+ before do
+ entity.destination_namespace = ''
+ end
+ it 'does not set parent id' do
transformed_data = subject.transform(context, data)
expect(transformed_data).not_to have_key('parent_id')
end
end
end
+
+ describe 'group name transformation' do
+ context 'when destination namespace is empty' do
+ before do
+ entity.destination_namespace = ''
+ end
+
+ it 'does not transform name' do
+ transformed_data = subject.transform(context, data)
+
+ expect(transformed_data['name']).to eq('Source Group Name')
+ end
+ end
+
+ context 'when destination namespace is present' do
+ context 'when destination namespace does not have a group with same name' do
+ it 'does not transform name' do
+ transformed_data = subject.transform(context, data)
+
+ expect(transformed_data['name']).to eq('Source Group Name')
+ end
+ end
+
+ context 'when destination namespace already have a group with the same name' do
+ before do
+ create(:group, parent: parent, name: 'Source Group Name', path: 'group_1')
+ create(:group, parent: parent, name: 'Source Group Name(1)', path: 'group_2')
+ create(:group, parent: parent, name: 'Source Group Name(2)', path: 'group_3')
+ create(:group, parent: parent, name: 'Source Group Name(1)(1)', path: 'group_4')
+ end
+
+ it 'makes the name unique by appeding a counter', :aggregate_failures do
+ transformed_data = subject.transform(context, data.merge('name' => 'Source Group Name'))
+ expect(transformed_data['name']).to eq('Source Group Name(3)')
+
+ transformed_data = subject.transform(context, data.merge('name' => 'Source Group Name(2)'))
+ expect(transformed_data['name']).to eq('Source Group Name(2)(1)')
+
+ transformed_data = subject.transform(context, data.merge('name' => 'Source Group Name(1)'))
+ expect(transformed_data['name']).to eq('Source Group Name(1)(2)')
+
+ transformed_data = subject.transform(context, data.merge('name' => 'Source Group Name(1)(1)'))
+ expect(transformed_data['name']).to eq('Source Group Name(1)(1)(1)')
+ end
+ end
+ end
+ end
end
end
--
GitLab