Merge branch '42410-transfer-service-support-for-design-repositories' into 'master'

Support design repo in project namespace transfer

See merge request gitlab-org/gitlab!23573

app/services/projects/transfer_service.rb

@@ -13,8 +13,6 @@ module Projects
     include Gitlab::ShellAdapter
     TransferError = Class.new(StandardError)
 
-    attr_reader :new_namespace
-
     def execute(new_namespace)
       @new_namespace = new_namespace
 
@@ -39,6 +37,8 @@ module Projects
 
     private
 
+    attr_reader :old_path, :new_path, :new_namespace
+
     # rubocop: disable CodeReuse/ActiveRecord
     def transfer(project)
       @old_path = project.full_path
@@ -132,6 +132,8 @@ module Projects
     end
 
     def rollback_folder_move
+      return if project.hashed_storage?(:repository)
+
       move_repo_folder(@new_path, @old_path)
       move_repo_folder("#{@new_path}.wiki", "#{@old_path}.wiki")
     end

ee/app/services/ee/projects/transfer_service.rb

@@ -16,7 +16,7 @@ module EE
         ::Geo::RepositoryRenamedEventStore.new(
           project,
           old_path: project.path,
-          old_path_with_namespace: @old_path # rubocop:disable Gitlab/ModuleWithInstanceVariables
+          old_path_with_namespace: old_path
         ).create!
       end
 
@@ -29,9 +29,35 @@ module EE
         super
       end
 
+      override :move_project_folders
+      def move_project_folders(project)
+        super
+
+        return if project.hashed_storage?(:repository)
+
+        move_repo_folder(old_design_repo_path, new_design_repo_path)
+      end
+
+      override :rollback_folder_move
+      def rollback_folder_move
+        super
+
+        return if project.hashed_storage?(:repository)
+
+        move_repo_folder(new_design_repo_path, old_design_repo_path)
+      end
+
       def new_namespace_has_same_root?(project)
         new_namespace.root_ancestor == project.namespace.root_ancestor
       end
+
+      def old_design_repo_path
+        "#{old_path}#{EE::Gitlab::GlRepository::DESIGN.path_suffix}"
+      end
+
+      def new_design_repo_path
+        "#{new_path}#{EE::Gitlab::GlRepository::DESIGN.path_suffix}"
+      end
     end
   end
 end

ee/changelogs/unreleased/42410-transfer-service-support-for-design-repositories.yml

+---
+title: Support moving the design repository of a project when the project is transferred
+  to a new namespace
+merge_request: 23573
+author:
+type: changed

ee/spec/services/projects/transfer_service_spec.rb

@@ -5,9 +5,9 @@ require 'spec_helper'
 describe Projects::TransferService do
   include EE::GeoHelpers
 
-  let(:user) { create(:user) }
-  let(:group) { create(:group) }
-  let(:project) { create(:project, :repository, namespace: user.namespace) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let(:project) { create(:project, :repository, :legacy_storage, namespace: user.namespace) }
 
   subject { described_class.new(project, user) }
 
@@ -82,4 +82,82 @@ describe Projects::TransferService do
       end
     end
   end
+
+  describe 'transferring a design repository' do
+    def design_repository
+      project.design_repository
+    end
+
+    it 'does not create a design repository' do
+      expect(subject.execute(group)).to be true
+
+      project.clear_memoization(:design_repository)
+
+      expect(design_repository.exists?).to be false
+    end
+
+    describe 'when the project has a design repository' do
+      let(:project_repo_path) { "#{project.path}#{EE::Gitlab::GlRepository::DESIGN.path_suffix}" }
+      let(:old_full_path) { "#{user.namespace.full_path}/#{project_repo_path}" }
+      let(:new_full_path) { "#{group.full_path}/#{project_repo_path}" }
+
+      context 'with legacy storage' do
+        let(:project) { create(:project, :repository, :legacy_storage, :design_repo, namespace: user.namespace) }
+
+        it 'moves the repository' do
+          expect(subject.execute(group)).to be true
+
+          project.clear_memoization(:design_repository)
+
+          expect(design_repository).to have_attributes(
+            disk_path: new_full_path,
+            full_path: new_full_path
+          )
+        end
+
+        it 'does not move the repository when an error occurs', :aggregate_failures do
+          allow(subject).to receive(:execute_system_hooks).and_raise('foo')
+          expect { subject.execute(group) }.to raise_error('foo')
+
+          project.clear_memoization(:design_repository)
+
+          expect(design_repository).to have_attributes(
+            disk_path: old_full_path,
+            full_path: old_full_path
+          )
+        end
+      end
+
+      context 'with hashed storage' do
+        let(:project) { create(:project, :repository, namespace: user.namespace) }
+
+        it 'does not move the repository' do
+          old_disk_path = design_repository.disk_path
+
+          expect(subject.execute(group)).to be true
+
+          project.clear_memoization(:design_repository)
+
+          expect(design_repository).to have_attributes(
+            disk_path: old_disk_path,
+            full_path: new_full_path
+          )
+        end
+
+        it 'does not move the repository when an error occurs' do
+          old_disk_path = design_repository.disk_path
+
+          allow(subject).to receive(:execute_system_hooks).and_raise('foo')
+          expect { subject.execute(group) }.to raise_error('foo')
+
+          project.clear_memoization(:design_repository)
+
+          expect(design_repository).to have_attributes(
+            disk_path: old_disk_path,
+            full_path: old_full_path
+          )
+        end
+      end
+    end
+  end
 end

spec/services/projects/transfer_service_spec.rb

@@ -47,11 +47,12 @@ describe Projects::TransferService do
       end
     end
 
-    it 'disk path has moved' do
+    it 'moves the disk path', :aggregate_failures do
       old_path = project.repository.disk_path
       old_full_path = project.repository.full_path
 
       transfer_project(project, user, group)
+      project.reload_repository!
 
       expect(project.repository.disk_path).not_to eq(old_path)
       expect(project.repository.full_path).not_to eq(old_full_path)
@@ -298,22 +299,41 @@ describe Projects::TransferService do
   end
 
   context 'when hashed storage in use' do
-    let(:hashed_project) { create(:project, :repository, namespace: user.namespace) }
+    let!(:hashed_project) { create(:project, :repository, namespace: user.namespace) }
+    let!(:old_disk_path) { hashed_project.repository.disk_path }
 
     before do
       group.add_owner(user)
     end
 
-    it 'does not move the directory' do
-      old_path = hashed_project.repository.disk_path
-      old_full_path = hashed_project.repository.full_path
+    it 'does not move the disk path', :aggregate_failures do
+      new_full_path = "#{group.full_path}/#{hashed_project.path}"
 
       transfer_project(hashed_project, user, group)
-      project.reload
+      hashed_project.reload_repository!
 
-      expect(hashed_project.repository.disk_path).to eq(old_path)
-      expect(hashed_project.repository.full_path).to eq(old_full_path)
-      expect(hashed_project.disk_path).to eq(old_path)
+      expect(hashed_project.repository).to have_attributes(
+        disk_path: old_disk_path,
+        full_path: new_full_path
+      )
+      expect(hashed_project.disk_path).to eq(old_disk_path)
+    end
+
+    it 'does not move the disk path when the transfer fails', :aggregate_failures do
+      old_full_path = hashed_project.full_path
+
+      expect_next_instance_of(described_class) do |service|
+        allow(service).to receive(:execute_system_hooks).and_raise('foo')
+      end
+      expect { transfer_project(hashed_project, user, group) }.to raise_error('foo')
+
+      hashed_project.reload_repository!
+
+      expect(hashed_project.repository).to have_attributes(
+        disk_path: old_disk_path,
+        full_path: old_full_path
+      )
+      expect(hashed_project.disk_path).to eq(old_disk_path)
     end
   end