Merge branch 'pages-transfer-worker-guard' into 'master'

Only schedule PagesTransferWorker when project or namespace has pages

See merge request gitlab-org/gitlab!40603

app/models/concerns/storage/legacy_namespace.rb

@@ -25,8 +25,10 @@ module Storage
         Gitlab::UploadsTransfer.new.move_namespace(path, former_parent_full_path, parent_full_path)
 
         if ::Feature.enabled?(:async_pages_move_namespace_transfer, self)
-          run_after_commit do
-            Gitlab::PagesTransfer.new.async.move_namespace(path, former_parent_full_path, parent_full_path)
+          if any_project_with_pages_deployed?
+            run_after_commit do
+              Gitlab::PagesTransfer.new.async.move_namespace(path, former_parent_full_path, parent_full_path)
+            end
           end
         else
           Gitlab::PagesTransfer.new.move_namespace(path, former_parent_full_path, parent_full_path)
@@ -35,10 +37,12 @@ module Storage
         Gitlab::UploadsTransfer.new.rename_namespace(full_path_before_last_save, full_path)
 
         if ::Feature.enabled?(:async_pages_move_namespace_rename, self)
-          full_path_was = full_path_before_last_save
+          if any_project_with_pages_deployed?
+            full_path_was = full_path_before_last_save
 
-          run_after_commit do
-            Gitlab::PagesTransfer.new.async.rename_namespace(full_path_was, full_path)
+            run_after_commit do
+              Gitlab::PagesTransfer.new.async.rename_namespace(full_path_was, full_path)
+            end
           end
         else
           Gitlab::PagesTransfer.new.rename_namespace(full_path_before_last_save, full_path)

app/models/namespace.rb

@@ -353,6 +353,10 @@ class Namespace < ApplicationRecord
     )
   end
 
+  def any_project_with_pages_deployed?
+    all_projects.with_pages_deployed.any?
+  end
+
   def closest_setting(name)
     self_and_ancestors(hierarchy_order: :asc)
       .find { |n| !n.read_attribute(name).nil? }

app/services/projects/after_rename_service.rb

@@ -97,16 +97,18 @@ module Projects
       end
 
       if ::Feature.enabled?(:async_pages_move_project_rename, project)
-        # Block will be evaluated in the context of project so we need
-        # to bind to a local variable to capture it, as the instance
-        # variable and method aren't available on Project
-        path_before_local = @path_before
-
-        project.run_after_commit_or_now do
-          Gitlab::PagesTransfer
-            .new
-            .async
-            .rename_project(path_before_local, path, namespace.full_path)
+        if project.pages_deployed?
+          # Block will be evaluated in the context of project so we need
+          # to bind to a local variable to capture it, as the instance
+          # variable and method aren't available on Project
+          path_before_local = @path_before
+
+          project.run_after_commit_or_now do
+            Gitlab::PagesTransfer
+              .new
+              .async
+              .rename_project(path_before_local, path, namespace.full_path)
+          end
         end
       else
         Gitlab::PagesTransfer

spec/models/namespace_spec.rb

@@ -391,14 +391,14 @@ RSpec.describe Namespace do
         let(:uploads_dir) { FileUploader.root }
         let(:pages_dir) { File.join(TestEnv.pages_path) }
 
-        def expect_project_directories_at(namespace_path)
+        def expect_project_directories_at(namespace_path, with_pages: true)
           expected_repository_path = File.join(TestEnv.repos_path, namespace_path, 'the-project.git')
           expected_upload_path = File.join(uploads_dir, namespace_path, 'the-project')
           expected_pages_path = File.join(pages_dir, namespace_path, 'the-project')
 
           expect(File.directory?(expected_repository_path)).to be_truthy
           expect(File.directory?(expected_upload_path)).to be_truthy
-          expect(File.directory?(expected_pages_path)).to be_truthy
+          expect(File.directory?(expected_pages_path)).to be(with_pages)
         end
 
         before do
@@ -412,7 +412,7 @@ RSpec.describe Namespace do
           FileUtils.remove_entry(File.join(TestEnv.repos_path, new_parent.full_path), true)
           FileUtils.remove_entry(File.join(TestEnv.repos_path, child.full_path), true)
           FileUtils.remove_entry(File.join(uploads_dir, project.full_path), true)
-          FileUtils.remove_entry(File.join(pages_dir, project.full_path), true)
+          FileUtils.remove_entry(pages_dir, true)
         end
 
         context 'renaming child' do
@@ -426,10 +426,22 @@ RSpec.describe Namespace do
             end
           end
 
-          it 'correctly moves the repository, uploads and pages', :sidekiq_inline do
-            child.update!(path: 'renamed')
+          context 'when no projects have pages deployed' do
+            it 'moves the repository and uploads', :sidekiq_inline do
+              project.pages_metadatum.update!(deployed: false)
+              child.update!(path: 'renamed')
+
+              expect_project_directories_at('parent/renamed', with_pages: false)
+            end
+          end
+
+          context 'when the project has pages deployed' do
+            it 'correctly moves the repository, uploads and pages', :sidekiq_inline do
+              project.pages_metadatum.update!(deployed: true)
+              child.update!(path: 'renamed')
 
-            expect_project_directories_at('parent/renamed')
+              expect_project_directories_at('parent/renamed')
+            end
           end
         end
 
@@ -444,10 +456,22 @@ RSpec.describe Namespace do
             end
           end
 
-          it 'correctly moves the repository, uploads and pages', :sidekiq_inline do
-            parent.update!(path: 'renamed')
+          context 'when no projects have pages deployed' do
+            it 'moves the repository and uploads', :sidekiq_inline do
+              project.pages_metadatum.update!(deployed: false)
+              parent.update!(path: 'renamed')
+
+              expect_project_directories_at('renamed/child', with_pages: false)
+            end
+          end
+
+          context 'when the project has pages deployed' do
+            it 'correctly moves the repository, uploads and pages', :sidekiq_inline do
+              project.pages_metadatum.update!(deployed: true)
+              parent.update!(path: 'renamed')
 
-            expect_project_directories_at('renamed/child')
+              expect_project_directories_at('renamed/child')
+            end
           end
         end
 
@@ -462,10 +486,22 @@ RSpec.describe Namespace do
             end
           end
 
-          it 'correctly moves the repository, uploads and pages', :sidekiq_inline do
-            child.update!(parent: new_parent)
+          context 'when no projects have pages deployed' do
+            it 'moves the repository and uploads', :sidekiq_inline do
+              project.pages_metadatum.update!(deployed: false)
+              child.update!(parent: new_parent)
+
+              expect_project_directories_at('new_parent/child', with_pages: false)
+            end
+          end
+
+          context 'when the project has pages deployed' do
+            it 'correctly moves the repository, uploads and pages', :sidekiq_inline do
+              project.pages_metadatum.update!(deployed: true)
+              child.update!(parent: new_parent)
 
-            expect_project_directories_at('new_parent/child')
+              expect_project_directories_at('new_parent/child')
+            end
           end
         end
 
@@ -480,10 +516,22 @@ RSpec.describe Namespace do
             end
           end
 
-          it 'correctly moves the repository, uploads and pages', :sidekiq_inline do
-            child.update!(parent: nil)
+          context 'when no projects have pages deployed' do
+            it 'moves the repository and uploads', :sidekiq_inline do
+              project.pages_metadatum.update!(deployed: false)
+              child.update!(parent: nil)
+
+              expect_project_directories_at('child', with_pages: false)
+            end
+          end
+
+          context 'when the project has pages deployed' do
+            it 'correctly moves the repository, uploads and pages', :sidekiq_inline do
+              project.pages_metadatum.update!(deployed: true)
+              child.update!(parent: nil)
 
-            expect_project_directories_at('child')
+              expect_project_directories_at('child')
+            end
           end
         end
 
@@ -498,10 +546,22 @@ RSpec.describe Namespace do
             end
           end
 
-          it 'correctly moves the repository, uploads and pages', :sidekiq_inline do
-            parent.update!(parent: new_parent)
+          context 'when no projects have pages deployed' do
+            it 'moves the repository and uploads', :sidekiq_inline do
+              project.pages_metadatum.update!(deployed: false)
+              parent.update!(parent: new_parent)
 
-            expect_project_directories_at('new_parent/parent/child')
+              expect_project_directories_at('new_parent/parent/child', with_pages: false)
+            end
+          end
+
+          context 'when the project has pages deployed' do
+            it 'correctly moves the repository, uploads and pages', :sidekiq_inline do
+              project.pages_metadatum.update!(deployed: true)
+              parent.update!(parent: new_parent)
+
+              expect_project_directories_at('new_parent/parent/child')
+            end
           end
         end
       end
@@ -1174,6 +1234,27 @@ RSpec.describe Namespace do
     end
   end
 
+  describe '#any_project_with_pages_deployed?' do
+    it 'returns true if any project nested under the group has pages deployed' do
+      parent_1 = create(:group) # Three projects, one with pages
+      child_1_1 = create(:group, parent: parent_1) # Two projects, one with pages
+      child_1_2 = create(:group, parent: parent_1) # One project, no pages
+      parent_2 = create(:group) # No projects
+
+      create(:project, group: child_1_1).tap do |project|
+        project.pages_metadatum.update!(deployed: true)
+      end
+
+      create(:project, group: child_1_1)
+      create(:project, group: child_1_2)
+
+      expect(parent_1.any_project_with_pages_deployed?).to be(true)
+      expect(child_1_1.any_project_with_pages_deployed?).to be(true)
+      expect(child_1_2.any_project_with_pages_deployed?).to be(false)
+      expect(parent_2.any_project_with_pages_deployed?).to be(false)
+    end
+  end
+
   describe '#has_parent?' do
     it 'returns true when the group has a parent' do
       group = create(:group, :nested)

spec/services/projects/after_rename_service_spec.rb

@@ -75,10 +75,25 @@ RSpec.describe Projects::AfterRenameService do
           end
         end
 
-        it 'schedules a move of the pages directory' do
-          expect(PagesTransferWorker).to receive(:perform_async).with('rename_project', anything)
+        context 'when the project has pages deployed' do
+          it 'schedules a move of the pages directory' do
+            allow(project).to receive(:pages_deployed?).and_return(true)
 
-          service_execute
+            expect(PagesTransferWorker).to receive(:perform_async).with('rename_project', anything)
+
+            service_execute
+          end
+        end
+
+        context 'when the project does not have pages deployed' do
+          it 'does nothing with the pages directory' do
+            allow(project).to receive(:pages_deployed?).and_return(false)
+
+            expect(PagesTransferWorker).not_to receive(:perform_async)
+            expect(Gitlab::PagesTransfer).not_to receive(:new)
+
+            service_execute
+          end
         end
       end
 
@@ -180,10 +195,25 @@ RSpec.describe Projects::AfterRenameService do
           end
         end
 
-        it 'schedules a move of the pages directory' do
-          expect(PagesTransferWorker).to receive(:perform_async).with('rename_project', anything)
+        context 'when the project has pages deployed' do
+          it 'schedules a move of the pages directory' do
+            allow(project).to receive(:pages_deployed?).and_return(true)
 
-          service_execute
+            expect(PagesTransferWorker).to receive(:perform_async).with('rename_project', anything)
+
+            service_execute
+          end
+        end
+
+        context 'when the project does not have pages deployed' do
+          it 'does nothing with the pages directory' do
+            allow(project).to receive(:pages_deployed?).and_return(false)
+
+            expect(PagesTransferWorker).not_to receive(:perform_async)
+            expect(Gitlab::PagesTransfer).not_to receive(:new)
+
+            service_execute
+          end
         end
       end