Merge branch 'fix-deleting-project-again' into 'master'

Skip or retain project while deleting the project:

Closes #15005

See merge request !8960

3 files changed, 50 insertions, 24 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 44d4fb9d8d8..3c1a1ae5933 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -41,7 +41,7 @@ module Ci
 
     before_save :update_artifacts_size, if: :artifacts_file_changed?
     before_save :ensure_token
-    before_destroy { project }
+    before_destroy { unscoped_project }
 
     after_create :execute_hooks
     after_save :update_project_statistics, if: :artifacts_size_changed?
@@ -416,16 +416,23 @@ module Ci
     # This method returns old path to artifacts only if it already exists.
     #
     def artifacts_path
+      # We need the project even if it's soft deleted, because whenever
+      # we're really deleting the project, we'll also delete the builds,
+      # and in order to delete the builds, we need to know where to find
+      # the artifacts, which is depending on the data of the project.
+      # We need to retain the project in this case.
+      the_project = project || unscoped_project
+
       old = File.join(created_at.utc.strftime('%Y_%m'),
-                      project.ci_id.to_s,
+                      the_project.ci_id.to_s,
                       id.to_s)
 
       old_store = File.join(ArtifactUploader.artifacts_path, old)
-      return old if project.ci_id && File.directory?(old_store)
+      return old if the_project.ci_id && File.directory?(old_store)
 
       File.join(
         created_at.utc.strftime('%Y_%m'),
-        project.id.to_s,
+        the_project.id.to_s,
         id.to_s
       )
     end
@@ -560,6 +567,10 @@ module Ci
       self.update(erased_by: user, erased_at: Time.now, artifacts_expire_at: nil)
     end
 
+    def unscoped_project
+      @unscoped_project ||= Project.unscoped.find_by(id: gl_project_id)
+    end
+
     def predefined_variables
       variables = [
         { key: 'CI', value: 'true', public: true },
@@ -598,6 +609,8 @@ module Ci
     end
 
     def update_project_statistics
+      return unless project
+
       ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size])
     end
   end
diff --git a/changelogs/unreleased/fix-deleting-project-again.yml b/changelogs/unreleased/fix-deleting-project-again.yml
new file mode 100644
index 00000000000..e13215f22a7
--- /dev/null
+++ b/changelogs/unreleased/fix-deleting-project-again.yml
@@ -0,0 +1,4 @@
+---
+title: Fix deleting projects with pipelines and builds
+merge_request: 8960
+author:
diff --git a/spec/services/projects/destroy_service_spec.rb b/spec/services/projects/destroy_service_spec.rb
index 90771825f5c..3faa88c00a1 100644
--- a/spec/services/projects/destroy_service_spec.rb
+++ b/spec/services/projects/destroy_service_spec.rb
@@ -9,12 +9,27 @@ describe Projects::DestroyService, services: true do
 
   shared_examples 'deleting the project' do
     it 'deletes the project' do
-      expect(Project.all).not_to include(project)
+      expect(Project.unscoped.all).not_to include(project)
       expect(Dir.exist?(path)).to be_falsey
       expect(Dir.exist?(remove_path)).to be_falsey
     end
   end
 
+  shared_examples 'deleting the project with pipeline and build' do
+    context 'with pipeline and build' do # which has optimistic locking
+      let!(:pipeline) { create(:ci_pipeline, project: project) }
+      let!(:build) { create(:ci_build, :artifacts, pipeline: pipeline) }
+
+      before do
+        perform_enqueued_jobs do
+          destroy_project(project, user, {})
+        end
+      end
+
+      it_behaves_like 'deleting the project'
+    end
+  end
+
   context 'Sidekiq inline' do
     before do
       # Run sidekiq immediatly to check that renamed repository will be removed
@@ -35,30 +50,24 @@ describe Projects::DestroyService, services: true do
     it { expect(Dir.exist?(remove_path)).to be_truthy }
   end
 
-  context 'async delete of project with private issue visibility' do
-    let!(:async) { true }
+  context 'with async_execute' do
+    let(:async) { true }
 
-    before do
-      project.project_feature.update_attribute("issues_access_level", ProjectFeature::PRIVATE)
-      # Run sidekiq immediately to check that renamed repository will be removed
-      Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
+    context 'async delete of project with private issue visibility' do
+      before do
+        project.project_feature.update_attribute("issues_access_level", ProjectFeature::PRIVATE)
+        # Run sidekiq immediately to check that renamed repository will be removed
+        Sidekiq::Testing.inline! { destroy_project(project, user, {}) }
+      end
+
+      it_behaves_like 'deleting the project'
     end
 
-    it_behaves_like 'deleting the project'
+    it_behaves_like 'deleting the project with pipeline and build'
   end
 
-  context 'delete with pipeline' do # which has optimistic locking
-    let!(:pipeline) { create(:ci_pipeline, project: project) }
-
-    before do
-      expect(project).to receive(:destroy!).and_call_original
-
-      perform_enqueued_jobs do
-        destroy_project(project, user, {})
-      end
-    end
-
-    it_behaves_like 'deleting the project'
+  context 'with execute' do
+    it_behaves_like 'deleting the project with pipeline and build'
   end
 
   context 'container registry' do