Merge branch '340491-handle-move-project-or-group' into 'master'

Handle ProjectNamespaces when transfering a project or a group

See merge request gitlab-org/gitlab!70378

app/models/concerns/routable.rb

@@ -41,7 +41,7 @@ module Routable
     has_one :route, as: :source, autosave: true, dependent: :destroy, inverse_of: :source # rubocop:disable Cop/ActiveRecordDependent
     has_many :redirect_routes, as: :source, autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
-    validates :route, presence: true
+    validates :route, presence: true, unless: -> { is_a?(Namespaces::ProjectNamespace) }
 
     scope :with_route, -> { includes(:route) }
 
@@ -185,6 +185,7 @@ module Routable
 
   def prepare_route
     return unless full_path_changed? || full_name_changed?
+    return if is_a?(Namespaces::ProjectNamespace)
 
     route || build_route(source: self)
     route.path = build_full_path

app/models/namespace.rb

@@ -105,7 +105,7 @@ class Namespace < ApplicationRecord
 
   # Legacy Storage specific hooks
 
-  after_update :move_dir, if: :saved_change_to_path_or_parent?
+  after_update :move_dir, if: :saved_change_to_path_or_parent?, unless: -> { is_a?(Namespaces::ProjectNamespace) }
   before_destroy(prepend: true) { prepare_for_destroy }
   after_destroy :rm_dir
   after_commit :expire_child_caches, on: :update, if: -> {

app/models/project.rb

@@ -98,6 +98,7 @@ class Project < ApplicationRecord
   before_validation :mark_remote_mirrors_for_removal, if: -> { RemoteMirror.table_exists? }
 
   before_save :ensure_runners_token
+  before_save :ensure_project_namespace_in_sync
 
   after_save :update_project_statistics, if: :saved_change_to_namespace_id?
 
@@ -146,7 +147,7 @@ class Project < ApplicationRecord
   belongs_to :namespace
   # Sync deletion via DB Trigger to ensure we do not have
   # a project without a project_namespace (or vice-versa)
-  belongs_to :project_namespace, class_name: 'Namespaces::ProjectNamespace', foreign_key: 'project_namespace_id', inverse_of: :project
+  belongs_to :project_namespace, autosave: true, class_name: 'Namespaces::ProjectNamespace', foreign_key: 'project_namespace_id', inverse_of: :project
   alias_method :parent, :namespace
   alias_attribute :parent_id, :namespace_id
 
@@ -2883,6 +2884,15 @@ class Project < ApplicationRecord
   def online_runners_with_tags
     @online_runners_with_tags ||= active_runners.with_tags.online
   end
+
+  def ensure_project_namespace_in_sync
+    if changes.keys & [:name, :path, :namespace_id, :visibility_level] && project_namespace.present?
+      project_namespace.name = name
+      project_namespace.path = path
+      project_namespace.parent = namespace
+      project_namespace.visibility_level = visibility_level
+    end
+  end
 end
 
 Project.prepend_mod_with('Project')

app/services/groups/transfer_service.rb

@@ -140,6 +140,10 @@ module Groups
       # these records again.
       @updated_project_ids = projects_to_update.pluck(:id)
 
+      Namespaces::ProjectNamespace
+        .where(id: projects_to_update.select(:project_namespace_id))
+        .update_all(visibility_level: @new_parent_group.visibility_level)
+
       projects_to_update
         .update_all(visibility_level: @new_parent_group.visibility_level)
     end

spec/factories/namespaces/project_namespaces.rb

@@ -3,10 +3,11 @@
 FactoryBot.define do
   factory :project_namespace, class: 'Namespaces::ProjectNamespace' do
     project
+    parent { project.namespace }
+    visibility_level { project.visibility_level }
     name { project.name }
     path { project.path }
     type { Namespaces::ProjectNamespace.sti_name }
     owner { nil }
-    parent factory: :group
   end
 end

spec/services/groups/transfer_service_spec.rb

@@ -3,6 +3,19 @@
 require 'spec_helper'
 
 RSpec.describe Groups::TransferService do
+  shared_examples 'project namespace path is in sync with project path' do
+    it 'keeps project and project namespace attributes in sync' do
+      projects_with_project_namespace.each do |project|
+        project.reload
+
+        expect(project.full_path).to eq("#{group_full_path}/#{project.path}")
+        expect(project.project_namespace.full_path).to eq(project.full_path)
+        expect(project.project_namespace.parent).to eq(project.namespace)
+        expect(project.project_namespace.visibility_level).to eq(project.visibility_level)
+      end
+    end
+  end
+
   let_it_be(:user) { create(:user) }
   let_it_be(:new_parent_group) { create(:group, :public) }
 
@@ -169,6 +182,18 @@ RSpec.describe Groups::TransferService do
             expect(project.full_path).to eq("#{group.path}/#{project.path}")
           end
         end
+
+        context 'when projects have project namespaces' do
+          let_it_be(:project1) { create(:project, :private, namespace: group) }
+          let_it_be(:project_namespace1) { create(:project_namespace, project: project1) }
+          let_it_be(:project2) { create(:project, :private, namespace: group) }
+          let_it_be(:project_namespace2) { create(:project_namespace, project: project2) }
+
+          it_behaves_like 'project namespace path is in sync with project path' do
+            let(:group_full_path) { "#{group.path}" }
+            let(:projects_with_project_namespace) { [project1, project2] }
+          end
+        end
       end
     end
 
@@ -222,10 +247,10 @@ RSpec.describe Groups::TransferService do
 
       context 'when the parent group has a project with the same path' do
         let_it_be_with_reload(:group) { create(:group, :public, :nested, path: 'foo') }
+        let_it_be(:membership) { create(:group_member, :owner, group: new_parent_group, user: user) }
+        let_it_be(:project) { create(:project, path: 'foo', namespace: new_parent_group) }
 
         before do
-          create(:group_member, :owner, group: new_parent_group, user: user)
-          create(:project, path: 'foo', namespace: new_parent_group)
           group.update_attribute(:path, 'foo')
         end
 
@@ -237,6 +262,19 @@ RSpec.describe Groups::TransferService do
           transfer_service.execute(new_parent_group)
           expect(transfer_service.error).to eq('Transfer failed: Validation failed: Group URL has already been taken')
         end
+
+        context 'when projects have project namespaces' do
+          let!(:project_namespace) { create(:project_namespace, project: project) }
+
+          before do
+            transfer_service.execute(new_parent_group)
+          end
+
+          it_behaves_like 'project namespace path is in sync with project path' do
+            let(:group_full_path) { "#{new_parent_group.full_path}" }
+            let(:projects_with_project_namespace) { [project] }
+          end
+        end
       end
 
       context 'when the group is allowed to be transferred' do
@@ -407,6 +445,8 @@ RSpec.describe Groups::TransferService do
       context 'when transferring a group with project descendants' do
         let!(:project1) { create(:project, :repository, :private, namespace: group) }
         let!(:project2) { create(:project, :repository, :internal, namespace: group) }
+        let!(:project_namespace1) { create(:project_namespace, project: project1) }
+        let!(:project_namespace2) { create(:project_namespace, project: project2) }
 
         before do
           TestEnv.clean_test_path
@@ -432,18 +472,30 @@ RSpec.describe Groups::TransferService do
             expect(project1.private?).to be_truthy
             expect(project2.internal?).to be_truthy
           end
+
+          it_behaves_like 'project namespace path is in sync with project path' do
+            let(:group_full_path) { "#{new_parent_group.path}/#{group.path}" }
+            let(:projects_with_project_namespace) { [project1, project2] }
+          end
         end
 
         context 'when the new parent has a lower visibility than the projects' do
           let!(:project1) { create(:project, :repository, :public, namespace: group) }
           let!(:project2) { create(:project, :repository, :public, namespace: group) }
-          let(:new_parent_group) { create(:group, :private) }
+          let!(:new_parent_group) { create(:group, :private) }
+          let!(:project_namespace1) { create(:project_namespace, project: project1) }
+          let!(:project_namespace2) { create(:project_namespace, project: project2) }
 
           it 'updates projects visibility to match the new parent' do
             group.projects.each do |project|
               expect(project.private?).to be_truthy
             end
           end
+
+          it_behaves_like 'project namespace path is in sync with project path' do
+            let(:group_full_path) { "#{new_parent_group.path}/#{group.path}" }
+            let(:projects_with_project_namespace) { [project1, project2] }
+          end
         end
       end
 
@@ -452,6 +504,8 @@ RSpec.describe Groups::TransferService do
         let!(:project2) { create(:project, :repository, :internal, namespace: group) }
         let!(:subgroup1) { create(:group, :private, parent: group) }
         let!(:subgroup2) { create(:group, :internal, parent: group) }
+        let!(:project_namespace1) { create(:project_namespace, project: project1) }
+        let!(:project_namespace2) { create(:project_namespace, project: project2) }
 
         before do
           TestEnv.clean_test_path
@@ -480,6 +534,11 @@ RSpec.describe Groups::TransferService do
           expect(project1.redirect_routes.count).to eq(1)
           expect(project2.redirect_routes.count).to eq(1)
         end
+
+        it_behaves_like 'project namespace path is in sync with project path' do
+          let(:group_full_path) { "#{new_parent_group.path}/#{group.path}" }
+          let(:projects_with_project_namespace) { [project1, project2] }
+        end
       end
 
       context 'when transferring a group with nested groups and projects' do

spec/services/projects/transfer_service_spec.rb

@@ -64,6 +64,33 @@ RSpec.describe Projects::TransferService do
       expect(transfer_result).to be_truthy
       expect(project.namespace).to eq(group)
     end
+
+    context 'when project has an associated project namespace' do
+      let!(:project_namespace) { create(:project_namespace, project: project) }
+
+      it 'keeps project namespace in sync with project' do
+        transfer_result = execute_transfer
+
+        expect(transfer_result).to be_truthy
+
+        project_namespace_in_sync(group)
+      end
+
+      context 'when project is transferred to a deeper nested group' do
+        let(:parent_group) { create(:group) }
+        let(:sub_group) { create(:group, parent: parent_group) }
+        let(:sub_sub_group) { create(:group, parent: sub_group) }
+        let(:group) { sub_sub_group }
+
+        it 'keeps project namespace in sync with project' do
+          transfer_result = execute_transfer
+
+          expect(transfer_result).to be_truthy
+
+          project_namespace_in_sync(sub_sub_group)
+        end
+      end
+    end
   end
 
   context 'when transfer succeeds' do
@@ -243,6 +270,16 @@ RSpec.describe Projects::TransferService do
         expect(unrelated_pending_build.namespace_traversal_ids).to eq(other_project.namespace.traversal_ids)
       end
     end
+
+    context 'when project has an associated project namespace' do
+      let!(:project_namespace) { create(:project_namespace, project: project) }
+
+      it 'keeps project namespace in sync with project' do
+        attempt_project_transfer
+
+        project_namespace_in_sync(user.namespace)
+      end
+    end
   end
 
   context 'namespace -> no namespace' do
@@ -255,6 +292,18 @@ RSpec.describe Projects::TransferService do
       expect(project.namespace).to eq(user.namespace)
       expect(project.errors.messages[:new_namespace].first).to eq 'Please select a new namespace for your project.'
     end
+
+    context 'when project has an associated project namespace' do
+      let!(:project_namespace) { create(:project_namespace, project: project) }
+
+      it 'keeps project namespace in sync with project' do
+        transfer_result = execute_transfer
+
+        expect(transfer_result).to be false
+
+        project_namespace_in_sync(user.namespace)
+      end
+    end
   end
 
   context 'disallow transferring of project with tags' do
@@ -655,4 +704,13 @@ RSpec.describe Projects::TransferService do
   def rugged_config
     rugged_repo(project.repository).config
   end
+
+  def project_namespace_in_sync(group)
+    project.reload
+    expect(project.namespace).to eq(group)
+    expect(project.project_namespace.visibility_level).to eq(project.visibility_level)
+    expect(project.project_namespace.path).to eq(project.path)
+    expect(project.project_namespace.parent).to eq(project.namespace)
+    expect(project.project_namespace.traversal_ids).to eq([*project.namespace.traversal_ids, project.project_namespace.id])
+  end
 end