Merge branch...

Merge branch 'kassio/bulkimports-skip-epics-subrelations-pipelines-when-group-has-no-epics' into 'master'

BulkImports: Remove code duplication on Epic dependent pipelines

See merge request gitlab-org/gitlab!57402

ee/lib/ee/bulk_imports/groups/loaders/epic_award_emoji_loader.rb

-# frozen_string_literal: true
-
-module EE
-  module BulkImports
-    module Groups
-      module Loaders
-        class EpicAwardEmojiLoader
-          NotAllowedError = Class.new(StandardError)
-
-          # rubocop: disable CodeReuse/ActiveRecord
-          def load(context, data)
-            return unless data
-
-            epic = context.group.epics.find_by(iid: context.extra[:epic_iid])
-
-            return if award_emoji_exists?(epic, data)
-
-            raise NotAllowedError unless Ability.allowed?(context.current_user, :award_emoji, epic)
-
-            epic.award_emoji.create!(data)
-          end
-
-          private
-
-          def award_emoji_exists?(epic, data)
-            epic.award_emoji.exists?(user_id: data['user_id'], name: data['name'])
-          end
-          # rubocop: enable CodeReuse/ActiveRecord
-        end
-      end
-    end
-  end
-end

ee/lib/ee/bulk_imports/groups/pipelines/epic_award_emoji_pipeline.rb

@@ -4,38 +4,30 @@ module EE
   module BulkImports
     module Groups
       module Pipelines
-        class EpicAwardEmojiPipeline
-          include ::BulkImports::Pipeline
-
+        class EpicAwardEmojiPipeline < EE::BulkImports::Pipeline::EpicBase
           extractor ::BulkImports::Common::Extractors::GraphqlExtractor,
             query: EE::BulkImports::Groups::Graphql::GetEpicAwardEmojiQuery
 
           transformer ::BulkImports::Common::Transformers::ProhibitedAttributesTransformer
           transformer ::BulkImports::Common::Transformers::UserReferenceTransformer
 
-          loader EE::BulkImports::Groups::Loaders::EpicAwardEmojiLoader
-
           # rubocop: disable CodeReuse/ActiveRecord
-          def initialize(context)
-            @context = context
-            @group = context.group
-            @epic_iids = @group.epics.order(iid: :desc).pluck(:iid)
+          def load(context, data)
+            return unless data
 
-            set_next_epic
-          end
+            epic = context.group.epics.find_by(iid: context.extra[:epic_iid])
 
-          private
+            return if award_emoji_exists?(epic, data)
 
-          def after_run(extracted_data)
-            set_next_epic unless extracted_data.has_next_page?
+            raise NotAllowedError unless Ability.allowed?(context.current_user, :award_emoji, epic)
 
-            if extracted_data.has_next_page? || context.extra[:epic_iid]
-              run
-            end
+            epic.award_emoji.create!(data)
           end
 
-          def set_next_epic
-            context.extra[:epic_iid] = @epic_iids.pop
+          private
+
+          def award_emoji_exists?(epic, data)
+            epic.award_emoji.exists?(user_id: data['user_id'], name: data['name'])
           end
           # rubocop: enable CodeReuse/ActiveRecord
         end

ee/lib/ee/bulk_imports/groups/pipelines/epic_events_pipeline.rb

@@ -4,23 +4,13 @@ module EE
   module BulkImports
     module Groups
       module Pipelines
-        class EpicEventsPipeline
-          include ::BulkImports::Pipeline
-
+        class EpicEventsPipeline < EE::BulkImports::Pipeline::EpicBase
           extractor ::BulkImports::Common::Extractors::GraphqlExtractor,
                     query: EE::BulkImports::Groups::Graphql::GetEpicEventsQuery
 
           transformer ::BulkImports::Common::Transformers::ProhibitedAttributesTransformer
           transformer ::BulkImports::Common::Transformers::UserReferenceTransformer, reference: 'author'
 
-          def initialize(context)
-            @context = context
-            @group = context.group
-            @epic_iids = @group.epics.order(iid: :desc).pluck(:iid) # rubocop: disable CodeReuse/ActiveRecord
-
-            set_next_epic
-          end
-
           def transform(context, data)
             # Only create 'reopened' & 'closed' events.
             # 'created' event get created when epic is persisted.
@@ -49,18 +39,6 @@ module EE
 
           private
 
-          def after_run(extracted_data)
-            set_next_epic unless extracted_data.has_next_page?
-
-            if extracted_data.has_next_page? || context.extra[:epic_iid]
-              run
-            end
-          end
-
-          def set_next_epic
-            context.extra[:epic_iid] = @epic_iids.pop
-          end
-
           def create_event!(epic, data)
             epic.events.create!(data)
           end

ee/lib/ee/bulk_imports/pipeline/epic_base.rb

+# frozen_string_literal: true
+
+module EE
+  module BulkImports
+    module Pipeline
+      class EpicBase
+        include ::BulkImports::Pipeline
+
+        def initialize(context)
+          super(context)
+
+          @epic_iids = context.group.epics.order(iid: :desc).pluck(:iid) # rubocop: disable CodeReuse/ActiveRecord
+
+          set_next_epic
+        end
+
+        def run
+          return skip!('Skipping because group has no epics') if current_epic_iid.blank?
+
+          super
+        end
+
+        private
+
+        attr_reader :epic_iids
+
+        def after_run(extracted_data)
+          set_next_epic unless extracted_data.has_next_page?
+
+          if has_next_page_or_next_epic?(extracted_data)
+            run
+          end
+        end
+
+        def set_next_epic
+          context.extra[:epic_iid] = epic_iids.pop
+        end
+
+        def has_next_page_or_next_epic?(extracted_data)
+          extracted_data.has_next_page? || current_epic_iid
+        end
+
+        def current_epic_iid
+          context.extra[:epic_iid]
+        end
+      end
+    end
+  end
+end

ee/spec/lib/ee/bulk_imports/groups/loaders/epic_award_emoji_loader_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe EE::BulkImports::Groups::Loaders::EpicAwardEmojiLoader do
-  describe '#load' do
-    let_it_be(:user) { create(:user) }
-    let_it_be(:group) { create(:group) }
-    let_it_be(:epic) { create(:epic, group: group, iid: 1) }
-    let_it_be(:bulk_import) { create(:bulk_import, user: user) }
-    let_it_be(:entity) { create(:bulk_import_entity, bulk_import: bulk_import, group: group) }
-    let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) }
-    let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) }
-
-    let_it_be(:data) do
-      {
-        'name' => 'banana',
-        'user_id' => user.id
-      }
-    end
-
-    before do
-      stub_licensed_features(epics: true)
-      context.extra[:epic_iid] = epic.iid
-      group.add_developer(user)
-    end
-
-    context 'when emoji does not exist' do
-      it 'creates new emoji' do
-        expect { subject.load(context, data) }.to change(::AwardEmoji, :count).by(1)
-
-        epic = group.epics.last
-        emoji = epic.award_emoji.first
-
-        expect(emoji.name).to eq(data['name'])
-        expect(emoji.user).to eq(user)
-      end
-    end
-
-    context 'when same emoji exists' do
-      it 'does not create a new emoji' do
-        epic.award_emoji.create!(data)
-
-        expect { subject.load(context, data) }.not_to change(::AwardEmoji, :count)
-      end
-    end
-
-    context 'when user is not allowed to award emoji' do
-      before do
-        allow(Ability).to receive(:allowed?).with(user, :award_emoji, epic).and_return(false)
-      end
-
-      it 'raises NotAllowedError exception' do
-        expect { subject.load(context, data) }.to raise_error(described_class::NotAllowedError)
-      end
-    end
-  end
-end

ee/spec/lib/ee/bulk_imports/groups/pipelines/epic_award_emoji_pipeline_spec.rb

@@ -48,48 +48,80 @@ RSpec.describe EE::BulkImports::Groups::Pipelines::EpicAwardEmojiPipeline do
       expect { subject.run }.to change(::AwardEmoji, :count).by(1)
       expect(epic.award_emoji.first.name).to eq('thumbsup')
     end
-  end
 
-  context 'when extracted data many pages' do
-    it 'runs pipeline for the second page' do
-      first_page = extracted_data(has_next_page: true)
-      last_page = extracted_data
+    context 'when extracted data many pages' do
+      it 'runs pipeline for the second page' do
+        first_page = extracted_data(has_next_page: true)
+        last_page = extracted_data
 
-      allow_next_instance_of(BulkImports::Common::Extractors::GraphqlExtractor) do |extractor|
-        allow(extractor)
-          .to receive(:extract)
-          .and_return(first_page, last_page)
+        allow_next_instance_of(BulkImports::Common::Extractors::GraphqlExtractor) do |extractor|
+          allow(extractor)
+            .to receive(:extract)
+            .and_return(first_page, last_page)
+        end
+
+        subject.run
       end
+    end
 
-      subject.run
+    context 'when there is many epics to import' do
+      let_it_be(:second_epic) { create(:epic, group: group) }
+
+      it 'runs the pipeline for the next epic' do
+        allow_next_instance_of(BulkImports::Common::Extractors::GraphqlExtractor) do |extractor|
+          allow(extractor)
+            .to receive(:extract)
+            .twice # for each epic
+            .and_return(extracted_data)
+        end
+
+        expect(context.extra)
+          .to receive(:[]=)
+          .with(:epic_iid, epic.iid)
+          .and_call_original
+        expect(context.extra)
+          .to receive(:[]=)
+          .with(:epic_iid, second_epic.iid)
+          .and_call_original
+        expect(context.extra)
+          .to receive(:[]=)
+          .with(:epic_iid, nil)
+          .and_call_original
+
+        subject.run
+      end
     end
   end
 
-  context 'when there is many epics to import' do
-    let_it_be(:second_epic) { create(:epic, group: group) }
+  describe '#load' do
+    let(:data) { { 'name' => 'thumbsup', 'user_id' => user.id } }
 
-    it 'runs the pipeline for the next epic' do
-      allow_next_instance_of(BulkImports::Common::Extractors::GraphqlExtractor) do |extractor|
-        allow(extractor)
-          .to receive(:extract)
-          .twice # for each epic
-          .and_return(extracted_data)
+    context 'when emoji does not exist' do
+      it 'creates new emoji' do
+        expect { subject.load(context, data) }.to change(::AwardEmoji, :count).by(1)
+
+        epic = group.epics.last
+        emoji = epic.award_emoji.first
+
+        expect(emoji.name).to eq(data['name'])
+        expect(emoji.user).to eq(user)
       end
+    end
 
-      expect(context.extra)
-        .to receive(:[]=)
-        .with(:epic_iid, epic.iid)
-        .and_call_original
-      expect(context.extra)
-        .to receive(:[]=)
-        .with(:epic_iid, second_epic.iid)
-        .and_call_original
-      expect(context.extra)
-        .to receive(:[]=)
-        .with(:epic_iid, nil)
-        .and_call_original
-
-      subject.run
+    context 'when same emoji exists' do
+      it 'does not create a new emoji' do
+        epic.award_emoji.create!(data)
+
+        expect { subject.load(context, data) }.not_to change(::AwardEmoji, :count)
+      end
+    end
+
+    context 'when user is not allowed to award emoji' do
+      it 'raises NotAllowedError exception' do
+        allow(Ability).to receive(:allowed?).with(user, :award_emoji, epic).and_return(false)
+
+        expect { subject.load(context, data) }.to raise_error(described_class::NotAllowedError)
+      end
     end
   end
 
@@ -114,10 +146,6 @@ RSpec.describe EE::BulkImports::Groups::Pipelines::EpicAwardEmojiPipeline do
           { klass: BulkImports::Common::Transformers::UserReferenceTransformer, options: nil }
         )
     end
-
-    it 'has loaders' do
-      expect(described_class.get_loader).to eq(klass: EE::BulkImports::Groups::Loaders::EpicAwardEmojiLoader, options: nil)
-    end
   end
 
   def extracted_data(has_next_page: false)

ee/spec/lib/ee/bulk_imports/pipeline/epic_base_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe EE::BulkImports::Pipeline::EpicBase do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:entity) { create(:bulk_import_entity, group: group) }
+  let_it_be(:pipeline_tracker) { create(:bulk_import_tracker, entity: entity) }
+  let_it_be(:context) { BulkImports::Pipeline::Context.new(pipeline_tracker) }
+
+  let(:pipeline_class) do
+    Class.new(EE::BulkImports::Pipeline::EpicBase) do
+      def extract(_)
+        ::BulkImports::Pipeline::ExtractedData.new
+      end
+
+      def transform(_, data)
+        data
+      end
+
+      def load(_, data)
+        data
+      end
+    end
+  end
+
+  subject { MyPipeline.new(context) }
+
+  before do
+    stub_const('MyPipeline', pipeline_class)
+  end
+
+  context 'when the group has no epics' do
+    it 'skips the pipeline' do
+      expect_next_instance_of(Gitlab::Import::Logger) do |logger|
+        expect(logger).to receive(:warn)
+          .with(
+            bulk_import_id: entity.bulk_import.id,
+            bulk_import_entity_id: entity.id,
+            bulk_import_entity_type: entity.source_type,
+            context_extra: { epic_iid: nil },
+            message: 'Skipping because group has no epics',
+            pipeline_class: 'MyPipeline'
+          )
+      end
+
+      expect { subject.run }
+        .to change(pipeline_tracker, :status_name).to(:skipped)
+    end
+  end
+
+  context 'when the group has one epic with one page of data' do
+    it 'runs the pipeline for each epic' do
+      create(:epic, group: group)
+
+      expect(subject)
+        .to receive(:run)
+        .once
+        .and_call_original
+
+      subject.run
+    end
+  end
+
+  context 'when the group has one epic with multiple page of data' do
+    it 'runs the pipeline for page' do
+      create(:epic, group: group)
+
+      first_page = ::BulkImports::Pipeline::ExtractedData.new(page_info: {
+        'has_next_page' => true,
+        'next_page' => 'page'
+      })
+      last_page = ::BulkImports::Pipeline::ExtractedData.new(page_info: {
+        'has_next_page' => false
+      })
+
+      expect(subject)
+        .to receive(:extract)
+        .and_return(first_page, last_page)
+
+      expect(subject)
+        .to receive(:run)
+        .twice
+        .and_call_original
+
+      subject.run
+    end
+  end
+
+  context 'when the group has multiple epics with one page of data' do
+    it 'runs the pipeline once for each epic' do
+      create(:epic, group: group)
+      create(:epic, group: group)
+
+      expect(subject)
+        .to receive(:run)
+        .twice
+        .and_call_original
+
+      subject.run
+    end
+  end
+
+  context 'when the group has multiple epics with multiple pages of data' do
+    it 'runs the pipeline once for each page of each epic' do
+      create(:epic, group: group)
+      create(:epic, group: group)
+
+      first_page = ::BulkImports::Pipeline::ExtractedData.new(page_info: {
+        'has_next_page' => true,
+        'next_page' => 'page'
+      })
+      last_page = ::BulkImports::Pipeline::ExtractedData.new(page_info: {
+        'has_next_page' => false
+      })
+
+      expect(subject)
+        .to receive(:extract)
+        .and_return(
+          first_page, # first epic
+          last_page,  # first epic
+          first_page, # last epic
+          last_page   # last epic
+        )
+
+      expect(subject)
+        .to receive(:run)
+        .exactly(4).times
+        .and_call_original
+
+      subject.run
+    end
+  end
+end