Merge branch '28243-check-for-docker-images-before-renaming-group' into 'master'

Resolve "Check for docker images before renaming group"

See merge request gitlab-org/gitlab!19428

app/models/group.rb

@@ -263,8 +263,8 @@ class Group < Namespace
     members_with_parents.maintainers.exists?(user_id: user)
   end
 
-  def has_container_repositories?
-    container_repositories.exists?
+  def has_container_repository_including_subgroups?
+    ::ContainerRepository.for_group_and_its_subgroups(self).exists?
   end
 
   # @deprecated

app/services/groups/transfer_service.rb

@@ -75,7 +75,7 @@ module Groups
     # rubocop: enable CodeReuse/ActiveRecord
 
     def group_projects_contain_registry_images?
-      @group.has_container_repositories?
+      @group.has_container_repository_including_subgroups?
     end
 
     def update_group_attributes

app/services/groups/update_service.rb

@@ -43,8 +43,9 @@ module Groups
     def renaming_group_with_container_registry_images?
       new_path = params[:path]
 
-      new_path && new_path != group.path &&
-        group.has_container_repositories?
+      new_path &&
+        new_path != group.path &&
+        group.has_container_repository_including_subgroups?
     end
 
     def container_images_error

spec/finders/container_repositories_finder_spec.rb

@@ -8,7 +8,7 @@ describe ContainerRepositoriesFinder do
 
   let(:group) { create(:group) }
   let(:project) { create(:project, group: group) }
-  let(:project_repository) { create(:container_repository, project: project) }
+  let!(:project_repository) { create(:container_repository, project: project) }
 
   before do
     group.add_reporter(reporter)

spec/services/groups/transfer_service_spec.rb

@@ -427,21 +427,35 @@ describe Groups::TransferService do
       end
     end
 
-    context 'when a project in group has container images' do
+    context 'when a project has container images' do
       let(:group) { create(:group, :public, :nested) }
-      let!(:project) { create(:project, :repository, :public, namespace: group) }
+      let!(:container_repository) { create(:container_repository, project: project) }
+
+      subject { transfer_service.execute(new_parent_group) }
 
       before do
-        stub_container_registry_tags(repository: /image/, tags: %w[rc1])
-        create(:container_repository, project: project, name: :image)
-        create(:group_member, :owner, group: new_parent_group, user: user)
+        group.add_owner(user)
+        new_parent_group.add_owner(user)
       end
 
-      it 'does not allow group to be transferred' do
-        transfer_service.execute(new_parent_group)
+      context 'within group' do
+        let(:project) { create(:project, :repository, :public, namespace: group) }
 
+        it 'does not transfer' do
+          expect(subject).to be false
           expect(transfer_service.error).to match(/Docker images in their Container Registry/)
         end
       end
+
+      context 'within subgroup' do
+        let(:subgroup) { create(:group, parent: group) }
+        let(:project) { create(:project, :repository, :public, namespace: subgroup) }
+
+        it 'does not transfer' do
+          expect(subject).to be false
+          expect(transfer_service.error).to match(/Docker images in their Container Registry/)
+        end
+      end
+    end
   end
 end

spec/services/groups/update_service_spec.rb

@@ -32,6 +32,43 @@ describe Groups::UpdateService do
 
           expect(service.execute).to be_falsey
         end
+
+        context 'when a project has container images' do
+          let(:params) { { path: SecureRandom.hex } }
+          let!(:container_repository) { create(:container_repository, project: project) }
+
+          subject { described_class.new(public_group, user, params).execute }
+
+          context 'within group' do
+            let(:project) { create(:project, group: public_group) }
+
+            context 'with path updates' do
+              it 'does not allow the update' do
+                expect(subject).to be false
+                expect(public_group.errors[:base].first).to match(/Docker images in their Container Registry/)
+              end
+            end
+
+            context 'with name updates' do
+              let(:params) { { name: 'new-name' } }
+
+              it 'allows the update' do
+                expect(subject).to be true
+                expect(public_group.reload.name).to eq('new-name')
+              end
+            end
+          end
+
+          context 'within subgroup' do
+            let(:subgroup) { create(:group, parent: public_group) }
+            let(:project) { create(:project, group: subgroup) }
+
+            it 'does not allow path updates' do
+              expect(subject).to be false
+              expect(public_group.errors[:base].first).to match(/Docker images in their Container Registry/)
+            end
+          end
+        end
       end
 
       context "internal group with internal project" do
@@ -148,30 +185,6 @@ describe Groups::UpdateService do
     end
   end
 
-  context 'projects in group have container images' do
-    let(:service) { described_class.new(public_group, user, path: SecureRandom.hex) }
-    let(:project) { create(:project, :internal, group: public_group) }
-
-    before do
-      stub_container_registry_tags(repository: /image/, tags: %w[rc1])
-      create(:container_repository, project: project, name: :image)
-    end
-
-    it 'does not allow path to be changed' do
-      result = described_class.new(public_group, user, path: 'new-path').execute
-
-      expect(result).to eq false
-      expect(public_group.errors[:base].first).to match(/Docker images in their Container Registry/)
-    end
-
-    it 'allows other settings to be changed' do
-      result = described_class.new(public_group, user, name: 'new-name').execute
-
-      expect(result).to eq true
-      expect(public_group.reload.name).to eq('new-name')
-    end
-  end
-
   context 'for a subgroup' do
     let(:subgroup) { create(:group, :private, parent: private_group) }