summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--CHANGELOG1
-rw-r--r--app/controllers/admin/groups_controller.rb4
-rw-r--r--app/controllers/groups_controller.rb4
-rw-r--r--app/models/namespace.rb2
-rw-r--r--app/services/delete_user_service.rb7
-rw-r--r--app/services/destroy_group_service.rb16
-rw-r--r--app/workers/group_destroy_worker.rb17
-rw-r--r--db/migrate/20140407135544_fix_namespaces.rb10
-rw-r--r--db/migrate/20160805041956_add_deleted_at_to_namespaces.rb12
-rw-r--r--db/schema.rb2
-rw-r--r--spec/controllers/admin/groups_controller_spec.rb24
-rw-r--r--spec/controllers/groups_controller_spec.rb29
-rw-r--r--spec/requests/api/users_spec.rb2
-rw-r--r--spec/services/delete_user_service_spec.rb6
-rw-r--r--spec/services/destroy_group_service_spec.rb58
-rw-r--r--spec/workers/group_destroy_worker_spec.rb19
16 files changed, 179 insertions, 34 deletions
diff --git a/CHANGELOG b/CHANGELOG
index 6fe1720796d..96965a20f69 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -93,6 +93,7 @@ v 8.11.0 (unreleased)
- Bump gitlab_git to lazy load compare commits
- Reduce number of queries made for merge_requests/:id/diffs
- Sensible state specific default sort order for issues and merge requests !5453 (tomb0y)
+ - Fix bug where destroying a namespace would not always destroy projects
- Fix RequestProfiler::Middleware error when code is reloaded in development
- Catch what warden might throw when profiling requests to re-throw it
- Avoid commit lookup on diff_helper passing existing local variable to the helper method
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index f3a88a8e6c8..4ce18321649 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -48,9 +48,9 @@ class Admin::GroupsController < Admin::ApplicationController
end
def destroy
- DestroyGroupService.new(@group, current_user).execute
+ DestroyGroupService.new(@group, current_user).async_execute
- redirect_to admin_groups_path, notice: 'Group was successfully deleted.'
+ redirect_to admin_groups_path, alert: "Group '#{@group.name}' was scheduled for deletion."
end
private
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 6780a6d4d87..cb82d62616c 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -87,9 +87,9 @@ class GroupsController < Groups::ApplicationController
end
def destroy
- DestroyGroupService.new(@group, current_user).execute
+ DestroyGroupService.new(@group, current_user).async_execute
- redirect_to root_path, alert: "Group '#{@group.name}' was successfully deleted."
+ redirect_to root_path, alert: "Group '#{@group.name}' was scheduled for deletion."
end
protected
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 8b52cc824cd..7c29d27ce97 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -1,4 +1,6 @@
class Namespace < ActiveRecord::Base
+ acts_as_paranoid
+
include Sortable
include Gitlab::ShellAdapter
diff --git a/app/services/delete_user_service.rb b/app/services/delete_user_service.rb
index 2f237de813c..eaff88d6463 100644
--- a/app/services/delete_user_service.rb
+++ b/app/services/delete_user_service.rb
@@ -21,6 +21,11 @@ class DeleteUserService
::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
end
- user.destroy
+ # 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!
+
+ user_data
end
end
diff --git a/app/services/destroy_group_service.rb b/app/services/destroy_group_service.rb
index a4ebccb5606..0081364b8aa 100644
--- a/app/services/destroy_group_service.rb
+++ b/app/services/destroy_group_service.rb
@@ -5,13 +5,23 @@ class DestroyGroupService
@group, @current_user = group, user
end
+ def async_execute
+ group.transaction do
+ # Soft delete via paranoia gem
+ group.destroy
+ job_id = GroupDestroyWorker.perform_async(group.id, current_user.id)
+ Rails.logger.info("User #{current_user.id} scheduled a deletion of group ID #{group.id} with job ID #{job_id}")
+ end
+ end
+
def execute
group.projects.each do |project|
+ # Execute the destruction of the models immediately to ensure atomic cleanup.
# Skip repository removal because we remove directory with namespace
- # that contain all this repositories
- ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute
+ # that contain all these repositories
+ ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute
end
- group.destroy
+ group.really_destroy!
end
end
diff --git a/app/workers/group_destroy_worker.rb b/app/workers/group_destroy_worker.rb
new file mode 100644
index 00000000000..5048746f09b
--- /dev/null
+++ b/app/workers/group_destroy_worker.rb
@@ -0,0 +1,17 @@
+class GroupDestroyWorker
+ include Sidekiq::Worker
+
+ sidekiq_options queue: :default
+
+ def perform(group_id, user_id)
+ begin
+ group = Group.with_deleted.find(group_id)
+ rescue ActiveRecord::RecordNotFound
+ return
+ end
+
+ user = User.find(user_id)
+
+ DestroyGroupService.new(group, user).execute
+ end
+end
diff --git a/db/migrate/20140407135544_fix_namespaces.rb b/db/migrate/20140407135544_fix_namespaces.rb
index 91374966698..0026ce645a6 100644
--- a/db/migrate/20140407135544_fix_namespaces.rb
+++ b/db/migrate/20140407135544_fix_namespaces.rb
@@ -1,8 +1,14 @@
# rubocop:disable all
class FixNamespaces < ActiveRecord::Migration
+ DOWNTIME = false
+
def up
- Namespace.where('name <> path and type is null').each do |namespace|
- namespace.update_attribute(:name, namespace.path)
+ namespaces = exec_query('SELECT id, path FROM namespaces WHERE name <> path and type is null')
+
+ namespaces.each do |row|
+ id = row['id']
+ path = row['path']
+ exec_query("UPDATE namespaces SET name = '#{path}' WHERE id = #{id}")
end
end
diff --git a/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb b/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb
new file mode 100644
index 00000000000..a853de3abfb
--- /dev/null
+++ b/db/migrate/20160805041956_add_deleted_at_to_namespaces.rb
@@ -0,0 +1,12 @@
+class AddDeletedAtToNamespaces < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ disable_ddl_transaction!
+
+ def change
+ add_column :namespaces, :deleted_at, :datetime
+ add_concurrent_index :namespaces, :deleted_at
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 6c85e1e9dba..1de2cdcf026 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -640,9 +640,11 @@ ActiveRecord::Schema.define(version: 20160810142633) do
t.boolean "share_with_group_lock", default: false
t.integer "visibility_level", default: 20, null: false
t.boolean "request_access_enabled", default: true, null: false
+ t.datetime "deleted_at"
end
add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree
+ add_index "namespaces", ["deleted_at"], name: "index_namespaces_on_deleted_at", using: :btree
add_index "namespaces", ["name"], name: "index_namespaces_on_name", unique: true, using: :btree
add_index "namespaces", ["name"], name: "index_namespaces_on_name_trigram", using: :gin, opclasses: {"name"=>"gin_trgm_ops"}
add_index "namespaces", ["owner_id"], name: "index_namespaces_on_owner_id", using: :btree
diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb
new file mode 100644
index 00000000000..0239aea47fb
--- /dev/null
+++ b/spec/controllers/admin/groups_controller_spec.rb
@@ -0,0 +1,24 @@
+require 'spec_helper'
+
+describe Admin::GroupsController do
+ let(:group) { create(:group) }
+ let(:project) { create(:project, namespace: group) }
+ let(:admin) { create(:admin) }
+
+ before do
+ sign_in(admin)
+ Sidekiq::Testing.fake!
+ end
+
+ describe 'DELETE #destroy' do
+ it 'schedules a group destroy' do
+ expect { delete :destroy, id: project.group.path }.to change(GroupDestroyWorker.jobs, :size).by(1)
+ end
+
+ it 'redirects to the admin group path' do
+ delete :destroy, id: project.group.path
+
+ expect(response).to redirect_to(admin_groups_path)
+ end
+ end
+end
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index cd98fecd0c7..4ae6364207b 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -75,4 +75,33 @@ describe GroupsController do
end
end
end
+
+ describe 'DELETE #destroy' do
+ context 'as another user' do
+ it 'returns 404' do
+ sign_in(create(:user))
+
+ delete :destroy, id: group.path
+
+ expect(response.status).to eq(404)
+ end
+ end
+
+ context 'as the group owner' do
+ before do
+ Sidekiq::Testing.fake!
+ sign_in(user)
+ end
+
+ it 'schedules a group destroy' do
+ expect { delete :destroy, id: group.path }.to change(GroupDestroyWorker.jobs, :size).by(1)
+ end
+
+ it 'redirects to the root path' do
+ delete :destroy, id: group.path
+
+ expect(response).to redirect_to(root_path)
+ end
+ end
+ end
end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index e0e041b4e15..0bbba64a6d5 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -564,12 +564,14 @@ describe API::API, api: true do
end
describe "DELETE /users/:id" do
+ let!(:namespace) { user.namespace }
before { admin }
it "deletes user" do
delete api("/users/#{user.id}", admin)
expect(response).to have_http_status(200)
expect { User.find(user.id) }.to raise_error ActiveRecord::RecordNotFound
+ expect { Namespace.find(namespace.id) }.to raise_error ActiveRecord::RecordNotFound
expect(json_response['email']).to eq(user.email)
end
diff --git a/spec/services/delete_user_service_spec.rb b/spec/services/delete_user_service_spec.rb
index 630458f9efc..418a12a83a9 100644
--- a/spec/services/delete_user_service_spec.rb
+++ b/spec/services/delete_user_service_spec.rb
@@ -9,9 +9,11 @@ describe DeleteUserService, services: true do
context 'no options are given' do
it 'deletes the user' do
- DeleteUserService.new(current_user).execute(user)
+ user_data = DeleteUserService.new(current_user).execute(user)
- expect { User.find(user.id) }.to raise_error(ActiveRecord::RecordNotFound)
+ 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)
end
it 'will delete the project in the near future' do
diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/destroy_group_service_spec.rb
index eca8ddd8ea4..da724643604 100644
--- a/spec/services/destroy_group_service_spec.rb
+++ b/spec/services/destroy_group_service_spec.rb
@@ -7,38 +7,52 @@ describe DestroyGroupService, services: true do
let!(:gitlab_shell) { Gitlab::Shell.new }
let!(:remove_path) { group.path + "+#{group.id}+deleted" }
- context 'database records' do
- before do
- destroy_group(group, user)
+ shared_examples 'group destruction' do |async|
+ context 'database records' do
+ before do
+ destroy_group(group, user, async)
+ end
+
+ it { expect(Group.all).not_to include(group) }
+ it { expect(Project.all).not_to include(project) }
end
- it { expect(Group.all).not_to include(group) }
- it { expect(Project.all).not_to include(project) }
- end
+ context 'file system' do
+ context 'Sidekiq inline' do
+ before do
+ # Run sidekiq immediatly to check that renamed dir will be removed
+ Sidekiq::Testing.inline! { destroy_group(group, user, async) }
+ end
- context 'file system' do
- context 'Sidekiq inline' do
- before do
- # Run sidekiq immediatly to check that renamed dir will be removed
- Sidekiq::Testing.inline! { destroy_group(group, user) }
+ it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
+ it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey }
end
- it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
- it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_falsey }
- end
+ context 'Sidekiq fake' do
+ before do
+ # Dont run sidekiq to check if renamed repository exists
+ Sidekiq::Testing.fake! { destroy_group(group, user, async) }
+ end
- context 'Sidekiq fake' do
- before do
- # Dont run sidekiq to check if renamed repository exists
- Sidekiq::Testing.fake! { destroy_group(group, user) }
+ it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
+ it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy }
end
+ end
- it { expect(gitlab_shell.exists?(project.repository_storage_path, group.path)).to be_falsey }
- it { expect(gitlab_shell.exists?(project.repository_storage_path, remove_path)).to be_truthy }
+ def destroy_group(group, user, async)
+ if async
+ DestroyGroupService.new(group, user).async_execute
+ else
+ DestroyGroupService.new(group, user).execute
+ end
end
end
- def destroy_group(group, user)
- DestroyGroupService.new(group, user).execute
+ describe 'asynchronous delete' do
+ it_behaves_like 'group destruction', true
+ end
+
+ describe 'synchronous delete' do
+ it_behaves_like 'group destruction', false
end
end
diff --git a/spec/workers/group_destroy_worker_spec.rb b/spec/workers/group_destroy_worker_spec.rb
new file mode 100644
index 00000000000..4e4eaf9b2f7
--- /dev/null
+++ b/spec/workers/group_destroy_worker_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe GroupDestroyWorker do
+ let(:group) { create(:group) }
+ let(:user) { create(:admin) }
+ let!(:project) { create(:project, namespace: group) }
+
+ subject { GroupDestroyWorker.new }
+
+ describe "#perform" do
+ it "deletes the project" do
+ subject.perform(group.id, user.id)
+
+ expect(Group.all).not_to include(group)
+ expect(Project.all).not_to include(project)
+ expect(Dir.exist?(project.path)).to be_falsey
+ end
+ end
+end
View Lorry Controller Status