Make sure repository's removal work for legacy and hashed storages28283-uuid-storage

7 files changed, 67 insertions, 8 deletions

diff --git a/app/models/project.rb b/app/models/project.rb
index a13ad9dceec..37f4dd08355 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1477,6 +1477,10 @@ class Project < ActiveRecord::Base
     Projects::ForksCountService.new(self).count
   end
 
+  def legacy_storage?
+    self.storage_version.nil?
+  end
+
   private
 
   def storage
diff --git a/app/services/groups/destroy_service.rb b/app/services/groups/destroy_service.rb
index f565612a89d..e3f9d9ee95d 100644
--- a/app/services/groups/destroy_service.rb
+++ b/app/services/groups/destroy_service.rb
@@ -13,7 +13,7 @@ module Groups
         # Execute the destruction of the models immediately to ensure atomic cleanup.
         # Skip repository removal because we remove directory with namespace
         # that contain all these repositories
-        ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
+        ::Projects::DestroyService.new(project, current_user, skip_repo: project.legacy_storage?).execute
       end
 
       group.children.each do |group|
diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb
index 9d7237c2fbb..8e20de8dfa5 100644
--- a/app/services/users/destroy_service.rb
+++ b/app/services/users/destroy_service.rb
@@ -35,16 +35,18 @@ module Users
         Groups::DestroyService.new(group, current_user).execute
       end
 
+      namespace = user.namespace
+      namespace.prepare_for_destroy
+
       user.personal_projects.each do |project|
         # Skip repository removal because we remove directory with namespace
         # that contain all this repositories
-        ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
+        ::Projects::DestroyService.new(project, current_user, skip_repo: project.legacy_storage?).execute
       end
 
       MigrateToGhostUserService.new(user).execute unless options[:hard_delete]
 
       # Destroy the namespace after destroying the user since certain methods may depend on the namespace existing
-      namespace = user.namespace
       user_data = user.destroy
       namespace.really_destroy!
 
diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb
index c6f1da82f3c..9ebda0ba03b 100644
--- a/spec/factories/projects.rb
+++ b/spec/factories/projects.rb
@@ -82,7 +82,7 @@ FactoryGirl.define do
     end
 
     trait :hashed do
-      storage_version 1
+      storage_version Project::LATEST_STORAGE_VERSION
     end
 
     trait :access_requestable do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index c7d2f2063af..2e613c44357 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2342,6 +2342,14 @@ describe Project do
       end
     end
 
+    describe '#legacy_storage?' do
+      it 'returns true when storage_version is nil' do
+        project = build(:project)
+
+        expect(project.legacy_storage?).to be_truthy
+      end
+    end
+
     describe '#rename_repo' do
       before do
         # Project#gitlab_shell returns a new instance of Gitlab::Shell on every
diff --git a/spec/services/groups/destroy_service_spec.rb b/spec/services/groups/destroy_service_spec.rb
index 1b2ce3cd03e..ac4b9c02ba7 100644
--- a/spec/services/groups/destroy_service_spec.rb
+++ b/spec/services/groups/destroy_service_spec.rb
@@ -8,8 +8,8 @@ describe Groups::DestroyService do
   let!(:nested_group) { create(:group, parent: group) }
   let!(:project)      { create(:project, namespace: group) }
   let!(:notification_setting) { create(:notification_setting, source: group)}
-  let!(:gitlab_shell) { Gitlab::Shell.new }
-  let!(:remove_path)  { group.path + "+#{group.id}+deleted" }
+  let(:gitlab_shell) { Gitlab::Shell.new }
+  let(:remove_path)  { group.path + "+#{group.id}+deleted" }
 
   before do
     group.add_user(user, Gitlab::Access::OWNER)
@@ -134,4 +134,26 @@ describe Groups::DestroyService do
 
     it_behaves_like 'group destruction', false
   end
+
+  describe 'repository removal' do
+    before do
+      destroy_group(group, user, false)
+    end
+
+    context 'legacy storage' do
+      let!(:project) { create(:project, :empty_repo, namespace: group) }
+
+      it 'removes repository' do
+        expect(gitlab_shell.exists?(project.repository_storage_path, "#{project.disk_path}.git")).to be_falsey
+      end
+    end
+
+    context 'hashed storage' do
+      let!(:project) { create(:project, :hashed, :empty_repo, namespace: group) }
+
+      it 'removes repository' do
+        expect(gitlab_shell.exists?(project.repository_storage_path, "#{project.disk_path}.git")).to be_falsey
+      end
+    end
+  end
 end
diff --git a/spec/services/users/destroy_service_spec.rb b/spec/services/users/destroy_service_spec.rb
index a82567f6f43..58a5bede3de 100644
--- a/spec/services/users/destroy_service_spec.rb
+++ b/spec/services/users/destroy_service_spec.rb
@@ -4,9 +4,10 @@ describe Users::DestroyService do
   describe "Deletes a user and all their personal projects" do
     let!(:user)      { create(:user) }
     let!(:admin)     { create(:admin) }
-    let!(:namespace) { create(:namespace, owner: user) }
+    let!(:namespace) { user.namespace }
     let!(:project)   { create(:project, namespace: namespace) }
     let(:service)    { described_class.new(admin) }
+    let(:gitlab_shell) { Gitlab::Shell.new }
 
     context 'no options are given' do
       it 'deletes the user' do
@@ -14,7 +15,7 @@ describe Users::DestroyService do
 
         expect { user_data['email'].to eq(user.email) }
         expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
-        expect { Namespace.with_deleted.find(user.namespace.id) }.to raise_error(ActiveRecord::RecordNotFound)
+        expect { Namespace.with_deleted.find(namespace.id) }.to raise_error(ActiveRecord::RecordNotFound)
       end
 
       it 'will delete the project' do
@@ -165,5 +166,27 @@ describe Users::DestroyService do
         expect(Issue.exists?(issue.id)).to be_falsy
       end
     end
+
+    describe "user personal's repository removal" do
+      before do
+        Sidekiq::Testing.inline! { service.execute(user) }
+      end
+
+      context 'legacy storage' do
+        let!(:project) { create(:project, :empty_repo, namespace: user.namespace) }
+
+        it 'removes repository' do
+          expect(gitlab_shell.exists?(project.repository_storage_path, "#{project.disk_path}.git")).to be_falsey
+        end
+      end
+
+      context 'hashed storage' do
+        let!(:project) { create(:project, :empty_repo, :hashed, namespace: user.namespace) }
+
+        it 'removes repository' do
+          expect(gitlab_shell.exists?(project.repository_storage_path, "#{project.disk_path}.git")).to be_falsey
+        end
+      end
+    end
   end
 end