Merge branch 'georgekoltsov/group-uploads-project-avatar' into 'master'

Add Project Avatar & Group Uploads to GitLab Migration

See merge request gitlab-org/gitlab!75249

app/models/bulk_imports/entity.rb

@@ -134,6 +134,10 @@ class BulkImports::Entity < ApplicationRecord
     source_type == 'group_entity'
   end
 
+  def update_service
+    "::#{pluralized_name.capitalize}::UpdateService".constantize
+  end
+
   private
 
   def validate_parent_is_a_group

app/models/bulk_imports/file_transfer/base_config.rb

@@ -5,6 +5,8 @@ module BulkImports
     class BaseConfig
       include Gitlab::Utils::StrongMemoize
 
+      UPLOADS_RELATION = 'uploads'
+
       def initialize(portable)
         @portable = portable
       end
@@ -78,7 +80,7 @@ module BulkImports
       end
 
       def file_relations
-        []
+        [UPLOADS_RELATION]
       end
 
       def skipped_relations

app/models/bulk_imports/file_transfer/project_config.rb

@@ -3,8 +3,6 @@
 module BulkImports
   module FileTransfer
     class ProjectConfig < BaseConfig
-      UPLOADS_RELATION = 'uploads'
-
       SKIPPED_RELATIONS = %w(
         project_members
         group_members
@@ -14,10 +12,6 @@ module BulkImports
         ::Gitlab::ImportExport.config_file
       end
 
-      def file_relations
-        [UPLOADS_RELATION]
-      end
-
       def skipped_relations
         SKIPPED_RELATIONS
       end

app/services/bulk_imports/uploads_export_service.rb

@@ -5,6 +5,7 @@ module BulkImports
     include Gitlab::ImportExport::CommandLineUtil
 
     BATCH_SIZE = 100
+    AVATAR_PATH = 'avatar'
 
     def initialize(portable, export_path)
       @portable = portable
@@ -34,7 +35,7 @@ module BulkImports
 
     def export_subdir_path(upload)
       subdir = if upload.path == avatar_path
-                 'avatar'
+                 AVATAR_PATH
                else
                  upload.try(:secret).to_s
                end

ee/spec/lib/ee/bulk_imports/groups/stage_spec.rb

@@ -6,7 +6,6 @@ RSpec.describe BulkImports::Groups::Stage do
   let(:pipelines) do
     [
       [0, BulkImports::Groups::Pipelines::GroupPipeline],
-      [1, BulkImports::Groups::Pipelines::GroupAvatarPipeline],
       [1, BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline],
       [1, BulkImports::Groups::Pipelines::MembersPipeline],
       [1, BulkImports::Common::Pipelines::LabelsPipeline],
@@ -17,6 +16,7 @@ RSpec.describe BulkImports::Groups::Stage do
       [2, BulkImports::Common::Pipelines::BoardsPipeline],
       [2, BulkImports::Groups::Pipelines::EpicsPipeline],
       [2, BulkImports::Common::Pipelines::WikiPipeline],
+      [2, BulkImports::Common::Pipelines::UploadsPipeline],
       [4, BulkImports::Common::Pipelines::EntityFinisher]
     ]
   end

lib/bulk_imports/common/pipelines/uploads_pipeline.rb

@@ -8,6 +8,9 @@ module BulkImports
         include Gitlab::ImportExport::CommandLineUtil
 
         FILENAME = 'uploads.tar.gz'
+        AVATAR_PATTERN = %r{.*\/#{BulkImports::UploadsExportService::AVATAR_PATH}\/(?<identifier>.*)}.freeze
+
+        AvatarLoadingError = Class.new(StandardError)
 
         def extract(context)
           download_service(tmp_dir, context).execute
@@ -18,14 +21,18 @@ module BulkImports
         end
 
         def load(context, file_path)
-          dynamic_path = FileUploader.extract_dynamic_path(file_path)
+          avatar_path = AVATAR_PATTERN.match(file_path)
+
+          return save_avatar(file_path) if avatar_path
+
+          dynamic_path = file_uploader.extract_dynamic_path(file_path)
 
           return unless dynamic_path
           return if File.directory?(file_path)
 
           named_captures = dynamic_path.named_captures.symbolize_keys
 
-          UploadService.new(context.portable, File.open(file_path, 'r'), FileUploader, **named_captures).execute
+          UploadService.new(context.portable, File.open(file_path, 'r'), file_uploader, **named_captures).execute
         end
 
         def after_run(_)
@@ -46,6 +53,24 @@ module BulkImports
         def tmp_dir
           @tmp_dir ||= Dir.mktmpdir('bulk_imports')
         end
+
+        def file_uploader
+          @file_uploader ||= if context.entity.group?
+                               NamespaceFileUploader
+                             else
+                               FileUploader
+                             end
+        end
+
+        def save_avatar(file_path)
+          File.open(file_path) do |avatar|
+            service = context.entity.update_service.new(portable, current_user, avatar: avatar)
+
+            unless service.execute
+              raise AvatarLoadingError, portable.errors.full_messages.to_sentence
+            end
+          end
+        end
       end
     end
   end

lib/bulk_imports/groups/pipelines/group_avatar_pipeline.rb

-# frozen_string_literal: true
-
-module BulkImports
-  module Groups
-    module Pipelines
-      class GroupAvatarPipeline
-        include Pipeline
-
-        ALLOWED_AVATAR_DOWNLOAD_TYPES = (AvatarUploader::MIME_WHITELIST + %w(application/octet-stream)).freeze
-
-        GroupAvatarLoadingError = Class.new(StandardError)
-
-        def extract(context)
-          context.extra[:tmpdir] = Dir.mktmpdir
-
-          filepath = BulkImports::FileDownloadService.new(
-            configuration: context.configuration,
-            relative_url: "/groups/#{context.entity.encoded_source_full_path}/avatar",
-            dir: context.extra[:tmpdir],
-            file_size_limit: Avatarable::MAXIMUM_FILE_SIZE,
-            allowed_content_types: ALLOWED_AVATAR_DOWNLOAD_TYPES
-          ).execute
-
-          BulkImports::Pipeline::ExtractedData.new(data: { filepath: filepath })
-        end
-
-        def load(context, data)
-          return if data.blank?
-
-          File.open(data[:filepath]) do |avatar|
-            service = ::Groups::UpdateService.new(
-              portable,
-              current_user,
-              avatar: avatar
-            )
-
-            unless service.execute
-              raise GroupAvatarLoadingError, portable.errors.full_messages.first
-            end
-          end
-        end
-
-        def after_run(_)
-          FileUtils.remove_entry(context.extra[:tmpdir]) if context.extra[:tmpdir].present?
-        end
-      end
-    end
-  end
-end

lib/bulk_imports/groups/stage.rb

@@ -11,10 +11,6 @@ module BulkImports
             pipeline: BulkImports::Groups::Pipelines::GroupPipeline,
             stage: 0
           },
-          avatar: {
-            pipeline: BulkImports::Groups::Pipelines::GroupAvatarPipeline,
-            stage: 1
-          },
           subgroups: {
             pipeline: BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline,
             stage: 1
@@ -39,6 +35,10 @@ module BulkImports
             pipeline: BulkImports::Common::Pipelines::BoardsPipeline,
             stage: 2
           },
+          uploads: {
+            pipeline: BulkImports::Common::Pipelines::UploadsPipeline,
+            stage: 2
+          },
           finisher: {
             pipeline: BulkImports::Common::Pipelines::EntityFinisher,
             stage: 3

spec/lib/bulk_imports/common/pipelines/uploads_pipeline_spec.rb

@@ -5,11 +5,12 @@ require 'spec_helper'
 RSpec.describe BulkImports::Common::Pipelines::UploadsPipeline do
   let_it_be(:tmpdir) { Dir.mktmpdir }
   let_it_be(:project) { create(:project) }
-  let_it_be(:entity) { create(:bulk_import_entity, :project_entity, project: project, source_full_path: 'test') }
-  let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) }
-  let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) }
-  let_it_be(:uploads_dir_path) { File.join(tmpdir, '72a497a02fe3ee09edae2ed06d390038') }
-  let_it_be(:upload_file_path) { File.join(uploads_dir_path, 'upload.txt')}
+  let_it_be(:group) { create(:group) }
+
+  let(:uploads_dir_path) { File.join(tmpdir, '72a497a02fe3ee09edae2ed06d390038') }
+  let(:upload_file_path) { File.join(uploads_dir_path, 'upload.txt')}
+  let(:tracker) { create(:bulk_import_tracker, entity: entity) }
+  let(:context) { BulkImports::Pipeline::Context.new(tracker) }
 
   subject(:pipeline) { described_class.new(context) }
 
@@ -24,57 +25,101 @@ RSpec.describe BulkImports::Common::Pipelines::UploadsPipeline do
     FileUtils.remove_entry(tmpdir) if Dir.exist?(tmpdir)
   end
 
-  describe '#run' do
-    it 'imports uploads into destination portable and removes tmpdir' do
-      allow(Dir).to receive(:mktmpdir).with('bulk_imports').and_return(tmpdir)
-      allow(pipeline).to receive(:extract).and_return(BulkImports::Pipeline::ExtractedData.new(data: [upload_file_path]))
+  shared_examples 'uploads import' do
+    describe '#run' do
+      before do
+        allow(Dir).to receive(:mktmpdir).with('bulk_imports').and_return(tmpdir)
+        allow(pipeline).to receive(:extract).and_return(BulkImports::Pipeline::ExtractedData.new(data: [upload_file_path]))
+      end
 
-      pipeline.run
+      it 'imports uploads into destination portable and removes tmpdir' do
+        pipeline.run
 
-      expect(project.uploads.map { |u| u.retrieve_uploader.filename }).to include('upload.txt')
+        expect(portable.uploads.map { |u| u.retrieve_uploader.filename }).to include('upload.txt')
 
-      expect(Dir.exist?(tmpdir)).to eq(false)
-    end
-  end
+        expect(Dir.exist?(tmpdir)).to eq(false)
+      end
 
-  describe '#extract' do
-    it 'downloads & extracts upload paths' do
-      allow(Dir).to receive(:mktmpdir).and_return(tmpdir)
-      expect(pipeline).to receive(:untar_zxf)
-      file_download_service = instance_double("BulkImports::FileDownloadService")
+      context 'when importing avatar' do
+        let(:uploads_dir_path) { File.join(tmpdir, 'avatar') }
 
-      expect(BulkImports::FileDownloadService)
-        .to receive(:new)
-        .with(
-          configuration: context.configuration,
-          relative_url: "/projects/test/export_relations/download?relation=uploads",
-          dir: tmpdir,
-          filename: 'uploads.tar.gz')
-        .and_return(file_download_service)
+        it 'imports avatar' do
+          FileUtils.touch(File.join(uploads_dir_path, 'avatar.png'))
 
-      expect(file_download_service).to receive(:execute)
+          expect_next_instance_of(entity.update_service) do |service|
+            expect(service).to receive(:execute)
+          end
 
-      extracted_data = pipeline.extract(context)
+          pipeline.run
+        end
 
-      expect(extracted_data.data).to contain_exactly(uploads_dir_path, upload_file_path)
-    end
-  end
+        context 'when something goes wrong' do
+          it 'raises exception' do
+            allow_next_instance_of(entity.update_service) do |service|
+              allow(service).to receive(:execute).and_return(nil)
+            end
+
+            pipeline.run
 
-  describe '#load' do
-    it 'creates a file upload' do
-      expect { pipeline.load(context, upload_file_path) }.to change { project.uploads.count }.by(1)
+            expect(entity.failures.first.exception_class).to include('AvatarLoadingError')
+          end
+        end
+      end
     end
 
-    context 'when dynamic path is nil' do
-      it 'returns' do
-        expect { pipeline.load(context, File.join(tmpdir, 'test')) }.not_to change { project.uploads.count }
+    describe '#extract' do
+      it 'downloads & extracts upload paths' do
+        allow(Dir).to receive(:mktmpdir).and_return(tmpdir)
+        expect(pipeline).to receive(:untar_zxf)
+        file_download_service = instance_double("BulkImports::FileDownloadService")
+
+        expect(BulkImports::FileDownloadService)
+          .to receive(:new)
+          .with(
+            configuration: context.configuration,
+            relative_url: "/#{entity.pluralized_name}/test/export_relations/download?relation=uploads",
+            dir: tmpdir,
+            filename: 'uploads.tar.gz')
+          .and_return(file_download_service)
+
+        expect(file_download_service).to receive(:execute)
+
+        extracted_data = pipeline.extract(context)
+
+        expect(extracted_data.data).to contain_exactly(uploads_dir_path, upload_file_path)
       end
     end
 
-    context 'when path is a directory' do
-      it 'returns' do
-        expect { pipeline.load(context, uploads_dir_path) }.not_to change { project.uploads.count }
+    describe '#load' do
+      it 'creates a file upload' do
+        expect { pipeline.load(context, upload_file_path) }.to change { portable.uploads.count }.by(1)
+      end
+
+      context 'when dynamic path is nil' do
+        it 'returns' do
+          expect { pipeline.load(context, File.join(tmpdir, 'test')) }.not_to change { portable.uploads.count }
+        end
+      end
+
+      context 'when path is a directory' do
+        it 'returns' do
+          expect { pipeline.load(context, uploads_dir_path) }.not_to change { portable.uploads.count }
+        end
       end
     end
   end
+
+  context 'when importing to group' do
+    let(:portable) { group }
+    let(:entity) { create(:bulk_import_entity, :group_entity, group: group, source_full_path: 'test') }
+
+    include_examples 'uploads import'
+  end
+
+  context 'when importing to project' do
+    let(:portable) { project }
+    let(:entity) { create(:bulk_import_entity, :project_entity, project: project, source_full_path: 'test') }
+
+    include_examples 'uploads import'
+  end
 end

spec/lib/bulk_imports/groups/pipelines/group_avatar_pipeline_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe BulkImports::Groups::Pipelines::GroupAvatarPipeline do
-  let_it_be(:user) { create(:user) }
-  let_it_be(:group) { create(:group) }
-  let_it_be(:bulk_import) { create(:bulk_import, user: user) }
-
-  let_it_be(:entity) do
-    create(
-      :bulk_import_entity,
-      group: group,
-      bulk_import: bulk_import,
-      source_full_path: 'source/full/path',
-      destination_name: 'My Destination Group',
-      destination_namespace: group.full_path
-    )
-  end
-
-  let_it_be(:tracker) { create(:bulk_import_tracker, entity: entity) }
-  let_it_be(:context) { BulkImports::Pipeline::Context.new(tracker) }
-
-  subject { described_class.new(context) }
-
-  describe '#run' do
-    it 'updates the group avatar' do
-      avatar_path = 'spec/fixtures/dk.png'
-      stub_file_download(
-        avatar_path,
-        configuration: context.configuration,
-        relative_url: "/groups/source%2Ffull%2Fpath/avatar",
-        dir: an_instance_of(String),
-        file_size_limit: Avatarable::MAXIMUM_FILE_SIZE,
-        allowed_content_types: described_class::ALLOWED_AVATAR_DOWNLOAD_TYPES
-      )
-
-      expect { subject.run }.to change(context.group, :avatar)
-
-      expect(context.group.avatar.filename).to eq(File.basename(avatar_path))
-    end
-
-    it 'raises an error when the avatar upload fails' do
-      avatar_path = 'spec/fixtures/aosp_manifest.xml'
-      stub_file_download(
-        avatar_path,
-        configuration: context.configuration,
-        relative_url: "/groups/source%2Ffull%2Fpath/avatar",
-        dir: an_instance_of(String),
-        file_size_limit: Avatarable::MAXIMUM_FILE_SIZE,
-        allowed_content_types: described_class::ALLOWED_AVATAR_DOWNLOAD_TYPES
-      )
-
-      expect_next_instance_of(Gitlab::Import::Logger) do |logger|
-        expect(logger).to receive(:error)
-          .with(
-            bulk_import_id: context.bulk_import.id,
-            bulk_import_entity_id: context.entity.id,
-            bulk_import_entity_type: context.entity.source_type,
-            context_extra: context.extra,
-            exception_class: "BulkImports::Groups::Pipelines::GroupAvatarPipeline::GroupAvatarLoadingError",
-            exception_message: "Avatar file format is not supported. Please try one of the following supported formats: image/png, image/jpeg, image/gif, image/bmp, image/tiff, image/vnd.microsoft.icon",
-            pipeline_class: "BulkImports::Groups::Pipelines::GroupAvatarPipeline",
-            pipeline_step: :loader
-          )
-      end
-
-      expect { subject.run }.to change(BulkImports::Failure, :count)
-    end
-  end
-
-  def stub_file_download(filepath = 'file/path.png', **params)
-    expect_next_instance_of(BulkImports::FileDownloadService, params.presence) do |downloader|
-      expect(downloader).to receive(:execute).and_return(filepath)
-    end
-  end
-end

spec/lib/bulk_imports/groups/stage_spec.rb

@@ -8,13 +8,13 @@ RSpec.describe BulkImports::Groups::Stage do
   let(:pipelines) do
     [
       [0, BulkImports::Groups::Pipelines::GroupPipeline],
-      [1, BulkImports::Groups::Pipelines::GroupAvatarPipeline],
       [1, BulkImports::Groups::Pipelines::SubgroupEntitiesPipeline],
       [1, BulkImports::Groups::Pipelines::MembersPipeline],
       [1, BulkImports::Common::Pipelines::LabelsPipeline],
       [1, BulkImports::Common::Pipelines::MilestonesPipeline],
       [1, BulkImports::Common::Pipelines::BadgesPipeline],
-      [2, BulkImports::Common::Pipelines::BoardsPipeline]
+      [2, BulkImports::Common::Pipelines::BoardsPipeline],
+      [2, BulkImports::Common::Pipelines::UploadsPipeline]
     ]
   end
 
@@ -24,7 +24,7 @@ RSpec.describe BulkImports::Groups::Stage do
 
   describe '.pipelines' do
     it 'list all the pipelines with their stage number, ordered by stage' do
-      expect(described_class.new(bulk_import).pipelines & pipelines).to eq(pipelines)
+      expect(described_class.new(bulk_import).pipelines & pipelines).to contain_exactly(*pipelines)
       expect(described_class.new(bulk_import).pipelines.last.last).to eq(BulkImports::Common::Pipelines::EntityFinisher)
     end
 

spec/models/bulk_imports/entity_spec.rb

@@ -298,4 +298,14 @@ RSpec.describe BulkImports::Entity, type: :model do
       expect(entity.wikis_url_path).to eq("/groups/#{entity.encoded_source_full_path}/wikis")
     end
   end
+
+  describe '#update_service' do
+    it 'returns correct update service class' do
+      group_entity = build(:bulk_import_entity)
+      project_entity = build(:bulk_import_entity, :project_entity)
+
+      expect(group_entity.update_service).to eq(::Groups::UpdateService)
+      expect(project_entity.update_service).to eq(::Projects::UpdateService)
+    end
+  end
 end

spec/services/bulk_imports/uploads_export_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe BulkImports::UploadsExportService do
+  let_it_be(:project) { create(:project, avatar: fixture_file_upload('spec/fixtures/rails_sample.png', 'image/png')) }
+  let_it_be(:upload) { create(:upload, :with_file, :issuable_upload, uploader: FileUploader, model: project) }
+  let_it_be(:export_path) { Dir.mktmpdir }
+
+  subject(:service) { described_class.new(project, export_path) }
+
+  after do
+    FileUtils.remove_entry(export_path) if Dir.exist?(export_path)
+  end
+
+  describe '#execute' do
+    it 'exports project uploads and avatar' do
+      subject.execute
+
+      expect(File.exist?(File.join(export_path, 'avatar', 'rails_sample.png'))).to eq(true)
+      expect(File.exist?(File.join(export_path, upload.secret, upload.retrieve_uploader.filename))).to eq(true)
+    end
+  end
+end