4 files changed, 157 insertions, 106 deletions

diff --git a/spec/services/pages/delete_services_spec.rb b/spec/services/pages/delete_service_spec.rb
index f1edf93b0c1..a79c89a1c35 100644
--- a/spec/services/pages/delete_services_spec.rb
+++ b/spec/services/pages/delete_service_spec.rb
@@ -16,7 +16,10 @@ RSpec.describe Pages::DeleteService do
   it 'deletes published pages', :sidekiq_inline do
     expect(project.pages_deployed?).to be(true)
 
-    expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true
+    expect_next_instance_of(Gitlab::PagesTransfer) do |pages_transfer|
+      expect(pages_transfer).to receive(:rename_project).and_return true
+    end
+
     expect(PagesWorker).to receive(:perform_in).with(5.minutes, :remove, project.namespace.full_path, anything)
 
     service.execute
@@ -24,11 +27,10 @@ RSpec.describe Pages::DeleteService do
     expect(project.pages_deployed?).to be(false)
   end
 
-  it "doesn't remove anything from the legacy storage if updates on it are disabled", :sidekiq_inline do
-    stub_feature_flags(pages_update_legacy_storage: false)
+  it "doesn't remove anything from the legacy storage", :sidekiq_inline do
+    allow(Settings.pages.local_store).to receive(:enabled).and_return(false)
 
     expect(project.pages_deployed?).to be(true)
-
     expect(PagesWorker).not_to receive(:perform_in)
 
     service.execute
@@ -69,7 +71,9 @@ RSpec.describe Pages::DeleteService do
     expect(project.pages_deployed?).to eq(false)
     expect(project.pages_domains.count).to eq(0)
 
-    expect_any_instance_of(Gitlab::PagesTransfer).to receive(:rename_project).and_return true
+    expect_next_instance_of(Gitlab::PagesTransfer) do |pages_transfer|
+      expect(pages_transfer).to receive(:rename_project).and_return true
+    end
 
     Sidekiq::Worker.drain_all
   end
diff --git a/spec/services/pages/migrate_from_legacy_storage_service_spec.rb b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb
index 4ec57044912..d058324f3bb 100644
--- a/spec/services/pages/migrate_from_legacy_storage_service_spec.rb
+++ b/spec/services/pages/migrate_from_legacy_storage_service_spec.rb
@@ -3,90 +3,135 @@
 require 'spec_helper'
 
 RSpec.describe Pages::MigrateFromLegacyStorageService do
-  let(:service) { described_class.new(Rails.logger, migration_threads: 3, batch_size: 10, ignore_invalid_entries: false) }
+  let(:batch_size) { 10 }
+  let(:mark_projects_as_not_deployed) { false }
+  let(:service) { described_class.new(Rails.logger, ignore_invalid_entries: false, mark_projects_as_not_deployed: mark_projects_as_not_deployed) }
 
-  it 'does not try to migrate pages if pages are not deployed' do
-    expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new)
+  shared_examples "migrates projects properly" do
+    it 'does not try to migrate pages if pages are not deployed' do
+      expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new)
 
-    expect(service.execute).to eq(migrated: 0, errored: 0)
-  end
+      is_expected.to eq(migrated: 0, errored: 0)
+    end
 
-  it 'uses multiple threads' do
-    projects = create_list(:project, 20)
-    projects.each do |project|
-      project.mark_pages_as_deployed
+    context 'when pages are marked as deployed' do
+      let(:project) { create(:project) }
 
-      FileUtils.mkdir_p File.join(project.pages_path, "public")
-      File.open(File.join(project.pages_path, "public/index.html"), "w") do |f|
-        f.write("Hello!")
+      before do
+        project.mark_pages_as_deployed
       end
-    end
-
-    service = described_class.new(Rails.logger, migration_threads: 3, batch_size: 2, ignore_invalid_entries: false)
 
-    threads = Concurrent::Set.new
+      context 'when pages directory does not exist' do
+        context 'when mark_projects_as_not_deployed is set' do
+          let(:mark_projects_as_not_deployed) { true }
 
-    expect(service).to receive(:migrate_project).exactly(20).times.and_wrap_original do |m, *args|
-      threads.add(Thread.current)
+          it 'counts project as migrated' do
+            expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: true) do |service|
+              expect(service).to receive(:execute).and_call_original
+            end
 
-      # sleep to be 100% certain that once thread can't consume all the queue
-      # it works without it, but I want to avoid making this test flaky
-      sleep(0.01)
+            is_expected.to eq(migrated: 1, errored: 0)
+          end
+        end
 
-      m.call(*args)
-    end
+        it 'counts project as errored' do
+          expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false) do |service|
+            expect(service).to receive(:execute).and_call_original
+          end
 
-    expect(service.execute).to eq(migrated: 20, errored: 0)
-    expect(threads.length).to eq(3)
-  end
+          is_expected.to eq(migrated: 0, errored: 1)
+        end
+      end
 
-  context 'when pages are marked as deployed' do
-    let(:project) { create(:project) }
+      context 'when pages directory exists on disk' do
+        before do
+          FileUtils.mkdir_p File.join(project.pages_path, "public")
+          File.open(File.join(project.pages_path, "public/index.html"), "w") do |f|
+            f.write("Hello!")
+          end
+        end
 
-    before do
-      project.mark_pages_as_deployed
-    end
+        it 'migrates pages projects without deployments' do
+          expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false) do |service|
+            expect(service).to receive(:execute).and_call_original
+          end
 
-    context 'when pages directory does not exist' do
-      it 'tries to migrate the project, but does not crash' do
-        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service|
-          expect(service).to receive(:execute).and_call_original
+          expect(project.pages_metadatum.reload.pages_deployment).to eq(nil)
+          expect(subject).to eq(migrated: 1, errored: 0)
+          expect(project.pages_metadatum.reload.pages_deployment).to be
         end
 
-        expect(service.execute).to eq(migrated: 0, errored: 1)
+        context 'when deployed already exists for the project' do
+          before do
+            deployment = create(:pages_deployment, project: project)
+            project.set_first_pages_deployment!(deployment)
+          end
+
+          it 'does not try to migrate project' do
+            expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new)
+
+            is_expected.to eq(migrated: 0, errored: 0)
+          end
+        end
       end
     end
+  end
 
-    context 'when pages directory exists on disk' do
-      before do
-        FileUtils.mkdir_p File.join(project.pages_path, "public")
-        File.open(File.join(project.pages_path, "public/index.html"), "w") do |f|
-          f.write("Hello!")
+  describe '#execute_with_threads' do
+    subject { service.execute_with_threads(threads: 3, batch_size: batch_size) }
+
+    include_examples "migrates projects properly"
+
+    context 'when there is work for multiple threads' do
+      let(:batch_size) { 2 } # override to force usage of multiple threads
+
+      it 'uses multiple threads' do
+        projects = create_list(:project, 20)
+        projects.each do |project|
+          project.mark_pages_as_deployed
+
+          FileUtils.mkdir_p File.join(project.pages_path, "public")
+          File.open(File.join(project.pages_path, "public/index.html"), "w") do |f|
+            f.write("Hello!")
+          end
         end
-      end
 
-      it 'migrates pages projects without deployments' do
-        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false) do |service|
-          expect(service).to receive(:execute).and_call_original
+        threads = Concurrent::Set.new
+
+        expect(service).to receive(:migrate_project).exactly(20).times.and_wrap_original do |m, *args|
+          threads.add(Thread.current)
+
+          # sleep to be 100% certain that once thread can't consume all the queue
+          # it works without it, but I want to avoid making this test flaky
+          sleep(0.01)
+
+          m.call(*args)
         end
 
-        expect do
-          expect(service.execute).to eq(migrated: 1, errored: 0)
-        end.to change { project.pages_metadatum.reload.pages_deployment }.from(nil)
+        is_expected.to eq(migrated: 20, errored: 0)
+        expect(threads.length).to eq(3)
       end
+    end
+  end
 
-      context 'when deployed already exists for the project' do
-        before do
-          deployment = create(:pages_deployment, project: project)
-          project.set_first_pages_deployment!(deployment)
-        end
+  describe "#execute_for_batch" do
+    subject { service.execute_for_batch(Project.ids) }
 
-        it 'does not try to migrate project' do
-          expect(::Pages::MigrateLegacyStorageToDeploymentService).not_to receive(:new)
+    include_examples "migrates projects properly"
 
-          expect(service.execute).to eq(migrated: 0, errored: 0)
+    it 'only tries to migrate projects with passed ids' do
+      projects = create_list(:project, 5)
+
+      projects.each(&:mark_pages_as_deployed)
+      projects_to_migrate = projects.first(3)
+
+      projects_to_migrate.each do |project|
+        expect_next_instance_of(::Pages::MigrateLegacyStorageToDeploymentService, project, ignore_invalid_entries: false, mark_projects_as_not_deployed: false) do |service|
+          expect(service).to receive(:execute).and_call_original
         end
       end
+
+      expect(service.execute_for_batch(projects_to_migrate.pluck(:id))).to eq(migrated: 0, errored: 3)
     end
   end
 end
diff --git a/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb b/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb
index d95303c3e85..25f571a73d1 100644
--- a/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb
+++ b/spec/services/pages/migrate_legacy_storage_to_deployment_service_spec.rb
@@ -14,44 +14,44 @@ RSpec.describe Pages::MigrateLegacyStorageToDeploymentService do
     expect(described_class.new(project, ignore_invalid_entries: true).execute[:status]).to eq(:error)
   end
 
-  it 'marks pages as not deployed if public directory is absent' do
-    project.mark_pages_as_deployed
-
-    expect(project.pages_metadatum.reload.deployed).to eq(true)
+  context 'when mark_projects_as_not_deployed is passed' do
+    let(:service) { described_class.new(project, mark_projects_as_not_deployed: true) }
 
-    expect(service.execute).to(
-      eq(status: :error,
-         message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}")
-    )
+    it 'marks pages as not deployed if public directory is absent and invalid entries are ignored' do
+      project.mark_pages_as_deployed
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
 
-    expect(project.pages_metadatum.reload.deployed).to eq(false)
-  end
+      expect(service.execute).to(
+        eq(status: :success,
+           message: "Archive not created. Missing public directory in #{project.pages_path}? Marked project as not deployed")
+      )
 
-  it 'does not mark pages as not deployed if public directory is absent but pages_deployment exists' do
-    deployment = create(:pages_deployment, project: project)
-    project.update_pages_deployment!(deployment)
-    project.mark_pages_as_deployed
+      expect(project.pages_metadatum.reload.deployed).to eq(false)
+    end
 
-    expect(project.pages_metadatum.reload.deployed).to eq(true)
+    it 'does not mark pages as not deployed if public directory is absent but pages_deployment exists' do
+      deployment = create(:pages_deployment, project: project)
+      project.update_pages_deployment!(deployment)
+      project.mark_pages_as_deployed
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
 
-    expect(service.execute).to(
-      eq(status: :error,
-         message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}")
-    )
+      expect(service.execute).to(
+        eq(status: :success,
+           message: "Archive not created. Missing public directory in #{project.pages_path}? Marked project as not deployed")
+      )
 
-    expect(project.pages_metadatum.reload.deployed).to eq(true)
+      expect(project.pages_metadatum.reload.deployed).to eq(true)
+    end
   end
 
-  it 'does not mark pages as not deployed if public directory is absent but feature is disabled' do
-    stub_feature_flags(pages_migration_mark_as_not_deployed: false)
-
+  it 'does not mark pages as not deployed if public directory is absent but invalid entries are not ignored' do
     project.mark_pages_as_deployed
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)
 
     expect(service.execute).to(
       eq(status: :error,
-         message: "Can't create zip archive: Can not find valid public dir in #{project.pages_path}")
+         message: "Archive not created. Missing public directory in #{project.pages_path}")
     )
 
     expect(project.pages_metadatum.reload.deployed).to eq(true)
diff --git a/spec/services/pages/zip_directory_service_spec.rb b/spec/services/pages/zip_directory_service_spec.rb
index 9de68dd62bb..9cce90c6c0d 100644
--- a/spec/services/pages/zip_directory_service_spec.rb
+++ b/spec/services/pages/zip_directory_service_spec.rb
@@ -12,8 +12,10 @@ RSpec.describe Pages::ZipDirectoryService do
 
   let(:ignore_invalid_entries) { false }
 
+  let(:service_directory) { @work_dir }
+
   let(:service) do
-    described_class.new(@work_dir, ignore_invalid_entries: ignore_invalid_entries)
+    described_class.new(service_directory, ignore_invalid_entries: ignore_invalid_entries)
   end
 
   let(:result) do
@@ -25,32 +27,32 @@ RSpec.describe Pages::ZipDirectoryService do
   let(:archive) { result[:archive_path] }
   let(:entries_count) { result[:entries_count] }
 
-  it 'returns error if project pages dir does not exist' do
-    expect(Gitlab::ErrorTracking).not_to receive(:track_exception)
-
-    expect(
-      described_class.new("/tmp/not/existing/dir").execute
-    ).to eq(status: :error, message: "Can not find valid public dir in /tmp/not/existing/dir")
+  shared_examples 'handles invalid public directory' do
+    it 'returns success' do
+      expect(status).to eq(:success)
+      expect(archive).to be_nil
+      expect(entries_count).to be_nil
+    end
   end
 
-  it 'returns nils if there is no public directory and does not leave archive' do
-    expect(status).to eq(:error)
-    expect(message).to eq("Can not find valid public dir in #{@work_dir}")
-    expect(archive).to eq(nil)
-    expect(entries_count).to eq(nil)
+  context "when work direcotry doesn't exist" do
+    let(:service_directory) { "/tmp/not/existing/dir" }
 
-    expect(File.exist?(File.join(@work_dir, '@migrated.zip'))).to eq(false)
+    include_examples 'handles invalid public directory'
   end
 
-  it 'returns nils if public directory is a symlink' do
-    create_dir('target')
-    create_file('./target/index.html', 'hello')
-    create_link("public", "./target")
+  context 'when public directory is absent' do
+    include_examples 'handles invalid public directory'
+  end
+
+  context 'when public directory is a symlink' do
+    before do
+      create_dir('target')
+      create_file('./target/index.html', 'hello')
+      create_link("public", "./target")
+    end
 
-    expect(status).to eq(:error)
-    expect(message).to eq("Can not find valid public dir in #{@work_dir}")
-    expect(archive).to eq(nil)
-    expect(entries_count).to eq(nil)
+    include_examples 'handles invalid public directory'
   end
 
   context 'when there is a public directory' do