Merge branch '221259-move-packages-service-transfer-to-core' into 'master'

Move transfer service package logic specs to core [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!37264

app/models/group.rb

@@ -172,6 +172,10 @@ class Group < Namespace
     notification_settings(hierarchy_order: hierarchy_order).where(user: user)
   end
 
+  def packages_feature_enabled?
+    ::Gitlab.config.packages.enabled
+  end
+
   def notification_email_for(user)
     # Finds the closest notification_setting with a `notification_email`
     notification_settings = notification_settings_for(user, hierarchy_order: :asc)

app/models/project.rb

@@ -838,6 +838,10 @@ class Project < ApplicationRecord
     auto_devops_config[:scope] != :project && !auto_devops_config[:status]
   end
 
+  def has_packages?(package_type)
+    packages.where(package_type: package_type).exists?
+  end
+
   def first_auto_devops_config
     return namespace.first_auto_devops_config if auto_devops&.enabled.nil?
 

app/services/groups/transfer_service.rb

@@ -46,6 +46,21 @@ module Groups
       raise_transfer_error(:namespace_with_same_path) if namespace_with_same_path?
       raise_transfer_error(:group_contains_images) if group_projects_contain_registry_images?
       raise_transfer_error(:cannot_transfer_to_subgroup) if transfer_to_subgroup?
+      raise_transfer_error(:group_contains_npm_packages) if group_with_npm_packages?
+    end
+
+    def group_with_npm_packages?
+      return false unless group.packages_feature_enabled?
+
+      npm_packages = ::Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute
+
+      return true if different_root_ancestor? && npm_packages.exists?
+
+      false
+    end
+
+    def different_root_ancestor?
+      group.root_ancestor != new_parent_group&.root_ancestor
     end
 
     def group_is_already_root?
@@ -133,7 +148,8 @@ module Groups
         same_parent_as_current: s_('TransferGroup|Group is already associated to the parent group.'),
         invalid_policies: s_("TransferGroup|You don't have enough permissions."),
         group_contains_images: s_('TransferGroup|Cannot update the path because there are projects under this group that contain Docker images in their Container Registry. Please remove the images from your projects first and try again.'),
-        cannot_transfer_to_subgroup: s_('TransferGroup|Cannot transfer group to one of its subgroup.')
+        cannot_transfer_to_subgroup: s_('TransferGroup|Cannot transfer group to one of its subgroup.'),
+        group_contains_npm_packages: s_('TransferGroup|Group contains projects with NPM packages.')
       }.freeze
     end
   end

app/services/groups/update_service.rb

@@ -17,6 +17,8 @@ module Groups
 
       return false unless valid_share_with_group_lock_change?
 
+      return false unless valid_path_change_with_npm_packages?
+
       before_assignment_hook(group, params)
 
       group.assign_attributes(params)
@@ -36,6 +38,20 @@ module Groups
 
     private
 
+    def valid_path_change_with_npm_packages?
+      return true unless group.packages_feature_enabled?
+      return true if params[:path].blank?
+      return true if !group.has_parent? && group.path == params[:path]
+
+      npm_packages = ::Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute
+      if npm_packages.exists?
+        group.errors.add(:path, s_('GroupSettings|cannot change when group contains projects with NPM packages'))
+        return
+      end
+
+      true
+    end
+
     def before_assignment_hook(group, params)
       # overridden in EE
     end

app/services/projects/transfer_service.rb

@@ -55,10 +55,18 @@ module Projects
         raise TransferError.new(s_('TransferProject|Project cannot be transferred, because tags are present in its container registry'))
       end
 
+      if project.has_packages?(:npm) && !new_namespace_has_same_root?(project)
+        raise TransferError.new(s_("TransferProject|Root namespace can't be updated if project has NPM packages"))
+      end
+
       attempt_transfer_transaction
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    def new_namespace_has_same_root?(project)
+      new_namespace.root_ancestor == project.namespace.root_ancestor
+    end
+
     def attempt_transfer_transaction
       Project.transaction do
         project.expire_caches_before_rename(@old_path)

ee/app/models/ee/group.rb

@@ -304,7 +304,7 @@ module EE
     end
 
     def packages_feature_available?
-      ::Gitlab.config.packages.enabled && feature_available?(:packages)
+      packages_feature_enabled? && feature_available?(:packages)
     end
 
     def dependency_proxy_feature_available?

ee/app/models/ee/project.rb

@@ -638,12 +638,6 @@ module EE
         .joins(:deletion_schedule).first
     end
 
-    def has_packages?(package_type)
-      return false unless feature_available?(:packages)
-
-      packages.where(package_type: package_type).exists?
-    end
-
     def disable_overriding_approvers_per_merge_request
       return super unless License.feature_available?(:admin_merge_request_approvers_rules)
       return super unless has_regulated_settings?

ee/app/services/ee/groups/transfer_service.rb

@@ -5,12 +5,6 @@ module EE
     module TransferService
       extend ::Gitlab::Utils::Override
 
-      def localized_ee_error_messages
-        {
-          group_contains_npm_packages: s_('TransferGroup|Group contains projects with NPM packages.')
-        }.freeze
-      end
-
       def update_group_attributes
         ::Epic.nullify_lost_group_parents(group.self_and_descendants, lost_groups)
 
@@ -19,17 +13,6 @@ module EE
 
       private
 
-      override :ensure_allowed_transfer
-      def ensure_allowed_transfer
-        super
-        return unless group.packages_feature_available?
-
-        npm_packages = ::Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute
-        if different_root_ancestor? && npm_packages.exists?
-          raise_ee_transfer_error(:group_contains_npm_packages)
-        end
-      end
-
       override :post_update_hooks
       def post_update_hooks(updated_project_ids)
         ::Project.id_in(updated_project_ids).find_each do |project|
@@ -37,14 +20,6 @@ module EE
         end
       end
 
-      def raise_ee_transfer_error(message)
-        raise ::Groups::TransferService::TransferError, localized_ee_error_messages[message]
-      end
-
-      def different_root_ancestor?
-        group.root_ancestor != new_parent_group&.root_ancestor
-      end
-
       def lost_groups
         ancestors = group.ancestors
 

ee/app/services/ee/groups/update_service.rb

@@ -12,8 +12,6 @@ module EE
           return false if group.errors.present?
         end
 
-        return false unless valid_path_change_with_npm_packages?
-
         handle_changes
 
         remove_insight_if_insight_project_absent
@@ -113,20 +111,6 @@ module EE
         AllowedEmailDomains::UpdateService.new(current_user, group, comma_separated_domains).execute
       end
 
-      def valid_path_change_with_npm_packages?
-        return true unless group.packages_feature_available?
-        return true if params[:path].blank?
-        return true if !group.has_parent? && group.path == params[:path]
-
-        npm_packages = ::Packages::GroupPackagesFinder.new(current_user, group, package_type: :npm).execute
-        if npm_packages.exists?
-          group.errors.add(:path, s_('GroupSettings|cannot change when group contains projects with NPM packages'))
-          return
-        end
-
-        true
-      end
-
       def log_audit_event
         EE::Audit::GroupChangesAuditor.new(current_user, group).execute
       end

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

@@ -19,19 +19,6 @@ module EE
           old_path_with_namespace: old_path
         ).create!
       end
-
-      override :transfer
-      def transfer(project)
-        if project.feature_available?(:packages) && project.has_packages?(:npm) && !new_namespace_has_same_root?(project)
-          raise ::Projects::TransferService::TransferError.new(s_("TransferProject|Root namespace can't be updated if project has NPM packages"))
-        end
-
-        super
-      end
-
-      def new_namespace_has_same_root?(project)
-        new_namespace.root_ancestor == project.namespace.root_ancestor
-      end
     end
   end
 end

ee/spec/models/project_spec.rb

@@ -2406,63 +2406,6 @@ RSpec.describe Project do
     end
   end
 
-  describe '#has_packages?' do
-    let(:project) { create(:project, :public) }
-
-    subject { project.has_packages?(package_type) }
-
-    shared_examples 'returning true examples' do
-      let!(:package) { create("#{package_type}_package", project: project) }
-
-      it { is_expected.to be true }
-    end
-
-    shared_examples 'returning false examples' do
-      it { is_expected.to be false }
-    end
-
-    context 'with packages disabled' do
-      before do
-        stub_licensed_features(packages: false)
-      end
-
-      it_behaves_like 'returning false examples' do
-        let!(:package) { create(:maven_package, project: project) }
-        let(:package_type) { :maven }
-      end
-    end
-
-    context 'with packages enabled' do
-      before do
-        stub_licensed_features(packages: true)
-      end
-
-      context 'with maven packages' do
-        it_behaves_like 'returning true examples' do
-          let(:package_type) { :maven }
-        end
-      end
-
-      context 'with npm packages' do
-        it_behaves_like 'returning true examples' do
-          let(:package_type) { :npm }
-        end
-      end
-
-      context 'with conan packages' do
-        it_behaves_like 'returning true examples' do
-          let(:package_type) { :conan }
-        end
-      end
-
-      context 'with no package type' do
-        it_behaves_like 'returning false examples' do
-          let(:package_type) { nil }
-        end
-      end
-    end
-  end
-
   describe 'caculate template repositories' do
     let(:group1) { create(:group) }
     let(:group2) { create(:group) }

ee/spec/services/groups/transfer_service_spec.rb

@@ -15,63 +15,6 @@ RSpec.describe Groups::TransferService, '#execute' do
     new_group&.add_owner(user)
   end
 
-  context 'with an npm package' do
-    before do
-      create(:npm_package, project: project)
-    end
-
-    shared_examples 'transfer not allowed' do
-      it 'does not allow transfer when there is a root namespace change' do
-        transfer_service.execute(new_group)
-
-        expect(transfer_service.error).to eq('Transfer failed: Group contains projects with NPM packages.')
-      end
-    end
-
-    it_behaves_like 'transfer not allowed'
-
-    context 'with a project within subgroup' do
-      let(:root_group) { create(:group) }
-      let(:group) { create(:group, parent: root_group) }
-
-      before do
-        root_group.add_owner(user)
-      end
-
-      it_behaves_like 'transfer not allowed'
-
-      context 'without a root namespace change' do
-        let(:new_group) { create(:group, parent: root_group) }
-
-        it 'allows transfer' do
-          transfer_service.execute(new_group)
-
-          expect(transfer_service.error).not_to be
-          expect(group.parent).to eq(new_group)
-        end
-      end
-
-      context 'when transferring a group into a root group' do
-        let(:new_group) { nil }
-
-        it_behaves_like 'transfer not allowed'
-      end
-    end
-  end
-
-  context 'without an npm package' do
-    context 'when transferring a group into a root group' do
-      let(:group) { create(:group, parent: create(:group)) }
-
-      it 'allows transfer' do
-        transfer_service.execute(nil)
-
-        expect(transfer_service.error).not_to be
-        expect(group.parent).to be_nil
-      end
-    end
-  end
-
   context 'when visibility changes' do
     let(:new_group) { create(:group, :private) }
 

ee/spec/services/groups/update_service_spec.rb

@@ -132,50 +132,6 @@ RSpec.describe Groups::UpdateService, '#execute' do
     end
   end
 
-  context 'with project' do
-    let(:project) { create(:project, namespace: group) }
-
-    shared_examples 'with packages' do
-      before do
-        stub_licensed_features(packages: true)
-        group.add_owner(user)
-      end
-
-      context 'with npm packages' do
-        let!(:package) { create(:npm_package, project: project) }
-
-        it 'does not allow a path update' do
-          expect(update_group(group, user, path: 'updated')).to be false
-          expect(group.errors[:path]).to include('cannot change when group contains projects with NPM packages')
-        end
-
-        it 'allows name update' do
-          expect(update_group(group, user, name: 'Updated')).to be true
-          expect(group.errors).to be_empty
-          expect(group.name).to eq('Updated')
-        end
-      end
-    end
-
-    it_behaves_like 'with packages'
-
-    context 'located in a subgroup' do
-      let(:subgroup) { create(:group, parent: group) }
-      let!(:project) { create(:project, namespace: subgroup) }
-
-      before do
-        subgroup.add_owner(user)
-      end
-
-      it_behaves_like 'with packages'
-
-      it 'does allow a path update if there is not a root namespace change' do
-        expect(update_group(subgroup, user, path: 'updated')).to be true
-        expect(subgroup.errors[:path]).to be_empty
-      end
-    end
-  end
-
   context 'repository_size_limit assignment as Bytes' do
     let(:group) { create(:group, :public, repository_size_limit: 0) }
 

ee/spec/services/projects/transfer_service_spec.rb

@@ -52,35 +52,4 @@ RSpec.describe Projects::TransferService do
       end
     end
   end
-
-  context 'with npm packages' do
-    let!(:package) { create(:npm_package, project: project) }
-
-    before do
-      stub_licensed_features(packages: true)
-    end
-
-    context 'with a root namespace change' do
-      it 'does not allow the transfer' do
-        expect(subject.execute(group)).to be false
-        expect(project.errors[:new_namespace]).to include("Root namespace can't be updated if project has NPM packages")
-      end
-    end
-
-    context 'without a root namespace change' do
-      let(:root) { create(:group) }
-      let(:group) { create(:group, parent: root) }
-      let(:other_group) { create(:group, parent: root) }
-      let(:project) { create(:project, :repository, namespace: group) }
-
-      before do
-        other_group.add_owner(user)
-      end
-
-      it 'does allow the transfer' do
-        expect(subject.execute(other_group)).to be true
-        expect(project.errors[:new_namespace]).to be_empty
-      end
-    end
-  end
 end

spec/models/project_spec.rb

@@ -6154,6 +6154,46 @@ RSpec.describe Project do
     end
   end
 
+  describe '#has_packages?' do
+    let(:project) { create(:project, :public) }
+
+    subject { project.has_packages?(package_type) }
+
+    shared_examples 'returning true examples' do
+      let!(:package) { create("#{package_type}_package", project: project) }
+
+      it { is_expected.to be true }
+    end
+
+    shared_examples 'returning false examples' do
+      it { is_expected.to be false }
+    end
+
+    context 'with maven packages' do
+      it_behaves_like 'returning true examples' do
+        let(:package_type) { :maven }
+      end
+    end
+
+    context 'with npm packages' do
+      it_behaves_like 'returning true examples' do
+        let(:package_type) { :npm }
+      end
+    end
+
+    context 'with conan packages' do
+      it_behaves_like 'returning true examples' do
+        let(:package_type) { :conan }
+      end
+    end
+
+    context 'with no package type' do
+      it_behaves_like 'returning false examples' do
+        let(:package_type) { nil }
+      end
+    end
+  end
+
   describe '#environments_for_scope' do
     let_it_be(:project, reload: true) { create(:project) }
 

spec/services/groups/transfer_service_spec.rb

@@ -8,6 +8,74 @@ RSpec.describe Groups::TransferService do
   let!(:group_member) { create(:group_member, :owner, group: group, user: user) }
   let(:transfer_service) { described_class.new(group, user) }
 
+  context 'handling packages' do
+    let(:group) { create(:group, :public) }
+    let(:project) { create(:project, :public, namespace: group) }
+    let(:new_group) { create(:group, :public) }
+
+    before do
+      group.add_owner(user)
+      new_group&.add_owner(user)
+    end
+
+    context 'with an npm package' do
+      before do
+        create(:npm_package, project: project)
+      end
+
+      shared_examples 'transfer not allowed' do
+        it 'does not allow transfer when there is a root namespace change' do
+          transfer_service.execute(new_group)
+
+          expect(transfer_service.error).to eq('Transfer failed: Group contains projects with NPM packages.')
+        end
+      end
+
+      it_behaves_like 'transfer not allowed'
+
+      context 'with a project within subgroup' do
+        let(:root_group) { create(:group) }
+        let(:group) { create(:group, parent: root_group) }
+
+        before do
+          root_group.add_owner(user)
+        end
+
+        it_behaves_like 'transfer not allowed'
+
+        context 'without a root namespace change' do
+          let(:new_group) { create(:group, parent: root_group) }
+
+          it 'allows transfer' do
+            transfer_service.execute(new_group)
+
+            expect(transfer_service.error).not_to be
+            expect(group.parent).to eq(new_group)
+          end
+        end
+
+        context 'when transferring a group into a root group' do
+          let(:new_group) { nil }
+
+          it_behaves_like 'transfer not allowed'
+        end
+      end
+    end
+
+    context 'without an npm package' do
+      context 'when transferring a group into a root group' do
+        let(:group) { create(:group, parent: create(:group)) }
+
+        it 'allows transfer' do
+          transfer_service.execute(nil)
+
+          expect(transfer_service.error).not_to be
+          expect(group.parent).to be_nil
+        end
+      end
+    end
+  end
+
   shared_examples 'ensuring allowed transfer for a group' do
     context "when there's an exception on GitLab shell directories" do
       let(:new_parent_group) { create(:group, :public) }

spec/services/groups/update_service_spec.rb

@@ -9,6 +9,50 @@ RSpec.describe Groups::UpdateService do
   let!(:public_group) { create(:group, :public) }
 
   describe "#execute" do
+    shared_examples 'with packages' do
+      before do
+        group.add_owner(user)
+      end
+
+      context 'with npm packages' do
+        let!(:package) { create(:npm_package, project: project) }
+
+        it 'does not allow a path update' do
+          expect(update_group(group, user, path: 'updated')).to be false
+          expect(group.errors[:path]).to include('cannot change when group contains projects with NPM packages')
+        end
+
+        it 'allows name update' do
+          expect(update_group(group, user, name: 'Updated')).to be true
+          expect(group.errors).to be_empty
+          expect(group.name).to eq('Updated')
+        end
+      end
+    end
+
+    context 'with project' do
+      let!(:group) { create(:group, :public) }
+      let(:project) { create(:project, namespace: group) }
+
+      it_behaves_like 'with packages'
+
+      context 'located in a subgroup' do
+        let(:subgroup) { create(:group, parent: group) }
+        let!(:project) { create(:project, namespace: subgroup) }
+
+        before do
+          subgroup.add_owner(user)
+        end
+
+        it_behaves_like 'with packages'
+
+        it 'does allow a path update if there is not a root namespace change' do
+          expect(update_group(subgroup, user, path: 'updated')).to be true
+          expect(subgroup.errors[:path]).to be_empty
+        end
+      end
+    end
+
     context "project visibility_level validation" do
       context "public group with public projects" do
         let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL) }
@@ -238,4 +282,8 @@ RSpec.describe Groups::UpdateService do
       end
     end
   end
+
+  def update_group(group, user, opts)
+    Groups::UpdateService.new(group, user, opts).execute
+  end
 end

spec/services/projects/transfer_service_spec.rb

@@ -11,6 +11,39 @@ RSpec.describe Projects::TransferService do
 
   subject(:execute_transfer) { described_class.new(project, user).execute(group) }
 
+  context 'with npm packages' do
+    before do
+      group.add_owner(user)
+    end
+
+    subject(:transfer_service) { described_class.new(project, user) }
+
+    let!(:package) { create(:npm_package, project: project) }
+
+    context 'with a root namespace change' do
+      it 'does not allow the transfer' do
+        expect(transfer_service.execute(group)).to be false
+        expect(project.errors[:new_namespace]).to include("Root namespace can't be updated if project has NPM packages")
+      end
+    end
+
+    context 'without a root namespace change' do
+      let(:root) { create(:group) }
+      let(:group) { create(:group, parent: root) }
+      let(:other_group) { create(:group, parent: root) }
+      let(:project) { create(:project, :repository, namespace: group) }
+
+      before do
+        other_group.add_owner(user)
+      end
+
+      it 'does allow the transfer' do
+        expect(transfer_service.execute(other_group)).to be true
+        expect(project.errors[:new_namespace]).to be_empty
+      end
+    end
+  end
+
   context 'namespace -> namespace' do
     before do
       allow_next_instance_of(Gitlab::UploadsTransfer) do |service|