From cda6fddf7489c3b37c8b74d18d15838cc64a61ca Mon Sep 17 00:00:00 2001
From: Kassio Borges <kborges@gitlab.com>
Date: Mon, 9 Nov 2020 09:48:48 +0000
Subject: [PATCH] Simplify subgroups pipeline on BulkImports

To better leverage the BulkImports pipeline structure, instead of
creating a nested array, for the subgroups pages, prefer to create a
flat array and let the `Pipeline#run` handle the iteration of the
extracted data.
---
 .../common/loaders/entities_loader.rb         | 19 ---------------
 .../common/loaders/entity_loader.rb           | 15 ++++++++++++
 .../groups/extractors/subgroups_extractor.rb  |  7 ++----
 .../pipelines/subgroup_entities_pipeline.rb   |  4 ++--
 .../subgroup_to_entity_transformer.rb         | 21 +++++++++++++++++
 .../subgroups_to_entities_transformer.rb      | 23 -------------------
 ...s_loader_spec.rb => entity_loader_spec.rb} |  6 ++---
 .../subgroup_entities_pipeline_spec.rb        |  6 ++---
 ...=> subgroup_to_entity_transformer_spec.rb} | 14 +++++------
 9 files changed, 52 insertions(+), 63 deletions(-)
 delete mode 100644 lib/bulk_imports/common/loaders/entities_loader.rb
 create mode 100644 lib/bulk_imports/common/loaders/entity_loader.rb
 create mode 100644 lib/bulk_imports/groups/transformers/subgroup_to_entity_transformer.rb
 delete mode 100644 lib/bulk_imports/groups/transformers/subgroups_to_entities_transformer.rb
 rename spec/lib/bulk_imports/common/loaders/{entities_loader_spec.rb => entity_loader_spec.rb} (92%)
 rename spec/lib/bulk_imports/groups/transformers/{subgroups_to_entities_transformer_spec.rb => subgroup_to_entity_transformer_spec.rb} (65%)

diff --git a/lib/bulk_imports/common/loaders/entities_loader.rb b/lib/bulk_imports/common/loaders/entities_loader.rb
deleted file mode 100644
index 19926fa9a3e..00000000000
--- a/lib/bulk_imports/common/loaders/entities_loader.rb
+++ /dev/null
@@ -1,19 +0,0 @@
-# frozen_string_literal: true
-
-module BulkImports
-  module Common
-    module Loaders
-      class EntitiesLoader
-        def initialize(*args); end
-
-        def load(context, entities)
-          bulk_import = context.entity.bulk_import
-
-          entities.each do |entity|
-            bulk_import.entities.create!(entity)
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/lib/bulk_imports/common/loaders/entity_loader.rb b/lib/bulk_imports/common/loaders/entity_loader.rb
new file mode 100644
index 00000000000..4540b892c88
--- /dev/null
+++ b/lib/bulk_imports/common/loaders/entity_loader.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+module BulkImports
+  module Common
+    module Loaders
+      class EntityLoader
+        def initialize(*args); end
+
+        def load(context, entity)
+          context.entity.bulk_import.entities.create!(entity)
+        end
+      end
+    end
+  end
+end
diff --git a/lib/bulk_imports/groups/extractors/subgroups_extractor.rb b/lib/bulk_imports/groups/extractors/subgroups_extractor.rb
index c33bae6ada4..5c5e686cec5 100644
--- a/lib/bulk_imports/groups/extractors/subgroups_extractor.rb
+++ b/lib/bulk_imports/groups/extractors/subgroups_extractor.rb
@@ -9,12 +9,9 @@ module BulkImports
         def extract(context)
           encoded_parent_path = ERB::Util.url_encode(context.entity.source_full_path)
 
-          subgroups = []
           http_client(context.entity.bulk_import.configuration)
-            .each_page(:get, "groups/#{encoded_parent_path}/subgroups") do |page|
-              subgroups << page
-            end
-          subgroups
+            .each_page(:get, "groups/#{encoded_parent_path}/subgroups")
+            .flat_map(&:itself)
         end
 
         private
diff --git a/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline.rb b/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline.rb
index 30b46a3fa24..6384e9d5972 100644
--- a/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline.rb
+++ b/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline.rb
@@ -7,8 +7,8 @@ module BulkImports
         include Pipeline
 
         extractor BulkImports::Groups::Extractors::SubgroupsExtractor
-        transformer BulkImports::Groups::Transformers::SubgroupsToEntitiesTransformer
-        loader BulkImports::Common::Loaders::EntitiesLoader
+        transformer BulkImports::Groups::Transformers::SubgroupToEntityTransformer
+        loader BulkImports::Common::Loaders::EntityLoader
       end
     end
   end
diff --git a/lib/bulk_imports/groups/transformers/subgroup_to_entity_transformer.rb b/lib/bulk_imports/groups/transformers/subgroup_to_entity_transformer.rb
new file mode 100644
index 00000000000..6c3c299c2d2
--- /dev/null
+++ b/lib/bulk_imports/groups/transformers/subgroup_to_entity_transformer.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+module BulkImports
+  module Groups
+    module Transformers
+      class SubgroupToEntityTransformer
+        def initialize(*args); end
+
+        def transform(context, entry)
+          {
+            source_type: :group_entity,
+            source_full_path: entry['full_path'],
+            destination_name: entry['name'],
+            destination_namespace: context.entity.group.full_path,
+            parent_id: context.entity.id
+          }
+        end
+      end
+    end
+  end
+end
diff --git a/lib/bulk_imports/groups/transformers/subgroups_to_entities_transformer.rb b/lib/bulk_imports/groups/transformers/subgroups_to_entities_transformer.rb
deleted file mode 100644
index 090b3552338..00000000000
--- a/lib/bulk_imports/groups/transformers/subgroups_to_entities_transformer.rb
+++ /dev/null
@@ -1,23 +0,0 @@
-# frozen_string_literal: true
-
-module BulkImports
-  module Groups
-    module Transformers
-      class SubgroupsToEntitiesTransformer
-        def initialize(*args); end
-
-        def transform(context, data)
-          data.map do |entry|
-            {
-              source_type: :group_entity,
-              source_full_path: entry['full_path'],
-              destination_name: entry['name'],
-              destination_namespace: context.entity.group.full_path,
-              parent_id: context.entity.id
-            }
-          end
-        end
-      end
-    end
-  end
-end
diff --git a/spec/lib/bulk_imports/common/loaders/entities_loader_spec.rb b/spec/lib/bulk_imports/common/loaders/entity_loader_spec.rb
similarity index 92%
rename from spec/lib/bulk_imports/common/loaders/entities_loader_spec.rb
rename to spec/lib/bulk_imports/common/loaders/entity_loader_spec.rb
index 9699fc00074..4de7d95172f 100644
--- a/spec/lib/bulk_imports/common/loaders/entities_loader_spec.rb
+++ b/spec/lib/bulk_imports/common/loaders/entity_loader_spec.rb
@@ -2,20 +2,20 @@
 
 require 'spec_helper'
 
-RSpec.describe BulkImports::Common::Loaders::EntitiesLoader do
+RSpec.describe BulkImports::Common::Loaders::EntityLoader do
   describe '#load' do
     it "creates entities for the given data" do
       group = create(:group, path: "imported-group")
       parent_entity = create(:bulk_import_entity, group: group, bulk_import: create(:bulk_import))
       context = instance_double(BulkImports::Pipeline::Context, entity: parent_entity)
 
-      data = [{
+      data = {
         source_type: :group_entity,
         source_full_path: "parent/subgroup",
         destination_name: "subgroup",
         destination_namespace: parent_entity.group.full_path,
         parent_id: parent_entity.id
-      }]
+      }
 
       expect { subject.load(context, data) }.to change(BulkImports::Entity, :count).by(1)
 
diff --git a/spec/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline_spec.rb b/spec/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline_spec.rb
index 98ae8d9c53e..60a4a796682 100644
--- a/spec/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline_spec.rb
+++ b/spec/lib/bulk_imports/groups/pipelines/subgroup_entities_pipeline_spec.rb
@@ -35,7 +35,7 @@ RSpec.describe BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline do
 
     before do
       allow_next_instance_of(BulkImports::Groups::Extractors::SubgroupsExtractor) do |extractor|
-        allow(extractor).to receive(:extract).and_return([subgroup_data])
+        allow(extractor).to receive(:extract).and_return(subgroup_data)
       end
 
       parent.add_owner(user)
@@ -67,14 +67,14 @@ RSpec.describe BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline do
 
     it 'has transformers' do
       expect(described_class.transformers).to contain_exactly(
-        klass: BulkImports::Groups::Transformers::SubgroupsToEntitiesTransformer,
+        klass: BulkImports::Groups::Transformers::SubgroupToEntityTransformer,
         options: nil
       )
     end
 
     it 'has loaders' do
       expect(described_class.loaders).to contain_exactly(
-        klass: BulkImports::Common::Loaders::EntitiesLoader,
+        klass: BulkImports::Common::Loaders::EntityLoader,
         options: nil
       )
     end
diff --git a/spec/lib/bulk_imports/groups/transformers/subgroups_to_entities_transformer_spec.rb b/spec/lib/bulk_imports/groups/transformers/subgroup_to_entity_transformer_spec.rb
similarity index 65%
rename from spec/lib/bulk_imports/groups/transformers/subgroups_to_entities_transformer_spec.rb
rename to spec/lib/bulk_imports/groups/transformers/subgroup_to_entity_transformer_spec.rb
index bcd65e218f6..2f97a5721e7 100644
--- a/spec/lib/bulk_imports/groups/transformers/subgroups_to_entities_transformer_spec.rb
+++ b/spec/lib/bulk_imports/groups/transformers/subgroup_to_entity_transformer_spec.rb
@@ -2,20 +2,18 @@
 
 require 'spec_helper'
 
-RSpec.describe BulkImports::Groups::Transformers::SubgroupsToEntitiesTransformer do
+RSpec.describe BulkImports::Groups::Transformers::SubgroupToEntityTransformer do
   describe "#transform" do
     it "transforms subgroups data in entity params" do
       parent = create(:group)
       parent_entity = instance_double(BulkImports::Entity, group: parent, id: 1)
       context = instance_double(BulkImports::Pipeline::Context, entity: parent_entity)
-      subgroup_data = [
-        {
-          "name" => "subgroup",
-          "full_path" => "parent/subgroup"
-        }
-      ]
+      subgroup_data = {
+        "name" => "subgroup",
+        "full_path" => "parent/subgroup"
+      }
 
-      expect(subject.transform(context, subgroup_data)).to contain_exactly(
+      expect(subject.transform(context, subgroup_data)).to eq(
         source_type: :group_entity,
         source_full_path: "parent/subgroup",
         destination_name: "subgroup",
-- 
2.30.9