diff --git a/CHANGELOG b/CHANGELOG
index 9e866af2fec63cf843012d6910836726cacc967e..1fd938a34cd4b826d2c2fa42ae931d70fcabc7b7 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -43,6 +43,7 @@ v 7.12.0 (unreleased)
- Better performance for web editor (switched from satellites to rugged)
- GitLab CI service sends .gitlab-ci.yaml in each push call
- When remove project - move repository and schedule it removal
+ - Improve group removing logic
v 7.11.4
- Fix missing bullets when creating lists
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 2dfae13ac5c0e10f980fa78668b6aa5143df2ad1..4d3e48f7f817a16fd22e577af09d827e68a034e8 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -47,7 +47,7 @@ class Admin::GroupsController < Admin::ApplicationController
end
def destroy
- @group.destroy
+ DestroyGroupService.new(@group, current_user).execute
redirect_to admin_groups_path, notice: 'Group was successfully deleted.'
end
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 34f0b257db3b48efc784c39e0a4b177b3b5c4450..2e381822e421bd4b15f58339cd959ea994f0b210 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -82,7 +82,7 @@ class GroupsController < Groups::ApplicationController
end
def destroy
- @group.destroy
+ DestroyGroupService.new(@group, current_user).execute
redirect_to root_path, notice: 'Group was removed.'
end
diff --git a/app/models/group.rb b/app/models/group.rb
index b4e908c5602d23b577ee4ac1c9362627c4f06b70..051c672cb33b7c08c4c2b51b0ba07a636a48b118 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -101,10 +101,14 @@ class Group < Namespace
end
def post_create_hook
+ Gitlab::AppLogger.info("Group \"#{name}\" was created")
+
system_hook_service.execute_hooks_for(self, :create)
end
def post_destroy_hook
+ Gitlab::AppLogger.info("Group \"#{name}\" was removed")
+
system_hook_service.execute_hooks_for(self, :destroy)
end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index 211dfa76b811f34ffe9f8763d4f7b185d057d95a..03d2ab165eacfdc0d8e0e6854eca402a8a920342 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -72,7 +72,7 @@ class Namespace < ActiveRecord::Base
path.gsub!(/[^a-zA-Z0-9_\-\.]/, "")
# Users with the great usernames of "." or ".." would end up with a blank username.
- # Work around that by setting their username to "blank", followed by a counter.
+ # Work around that by setting their username to "blank", followed by a counter.
path = "blank" if path.blank?
counter = 0
@@ -99,7 +99,18 @@ class Namespace < ActiveRecord::Base
end
def rm_dir
- gitlab_shell.rm_namespace(path)
+ # Move namespace directory into trash.
+ # We will remove it later async
+ new_path = "#{path}+#{id}+deleted"
+
+ if gitlab_shell.mv_namespace(path, new_path)
+ message = "Namespace directory \"#{path}\" moved to \"#{new_path}\""
+ Gitlab::AppLogger.info message
+
+ # Remove namespace directroy async with delay so
+ # GitLab has time to remove all projects first
+ GitlabShellWorker.perform_in(5.minutes, :rm_namespace, new_path)
+ end
end
def move_dir
diff --git a/app/services/delete_user_service.rb b/app/services/delete_user_service.rb
index d259b4efca63fe6bbb25a150a556ac97bef9a9e5..9017a63af3b3c8905a7107db17f09775fd414fcd 100644
--- a/app/services/delete_user_service.rb
+++ b/app/services/delete_user_service.rb
@@ -4,6 +4,12 @@ class DeleteUserService
user.errors[:base] << 'You must transfer ownership or delete groups before you can remove user'
user
else
+ 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
+ end
+
user.destroy
end
end
diff --git a/app/services/destroy_group_service.rb b/app/services/destroy_group_service.rb
new file mode 100644
index 0000000000000000000000000000000000000000..d929a6762933d0d928eb1bc170186a3dd4610dad
--- /dev/null
+++ b/app/services/destroy_group_service.rb
@@ -0,0 +1,17 @@
+class DestroyGroupService
+ attr_accessor :group, :current_user
+
+ def initialize(group, user)
+ @group, @current_user = group, user
+ end
+
+ def execute
+ @group.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
+ end
+
+ @group.destroy
+ end
+end
diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb
index 29e8ba347d4d804d22b77b6157298cfca85b6fcc..403f419ec508b0043c1086779064d5737649077f 100644
--- a/app/services/projects/destroy_service.rb
+++ b/app/services/projects/destroy_service.rb
@@ -36,9 +36,11 @@ module Projects
private
def remove_repository(path)
- unless gitlab_shell.exists?(path + '.git')
- return true
- end
+ # Skip repository removal. We use this flag when remove user or group
+ return true if params[:skip_repo] == true
+
+ # There is a possibility project does not have repository or wiki
+ return true unless gitlab_shell.exists?(path + '.git')
new_path = removal_path(path)
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index f768c750402b367a034350bdf3b160b8c320ec16..e88b6e31775340a214d06146566dd5c57c187d96 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -62,7 +62,7 @@ module API
delete ":id" do
group = find_group(params[:id])
authorize! :admin_group, group
- group.destroy
+ DestroyGroupService.new(group, current_user).execute
end
# Transfer a project to the Group namespace
diff --git a/spec/services/destroy_group_service_spec.rb b/spec/services/destroy_group_service_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..24e439503e772fe8d438e5d75da7b278b78cfa57
--- /dev/null
+++ b/spec/services/destroy_group_service_spec.rb
@@ -0,0 +1,44 @@
+require 'spec_helper'
+
+describe DestroyGroupService do
+ let!(:user) { create(:user) }
+ let!(:group) { create(:group) }
+ let!(:project) { create(:project, namespace: group) }
+ let!(:gitlab_shell) { Gitlab::Shell.new }
+ let!(:remove_path) { group.path + "+#{group.id}+deleted" }
+
+ context 'database records' do
+ before do
+ destroy_group(group, user)
+ end
+
+ it { Group.all.should_not include(group) }
+ it { Project.all.should_not 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) }
+ end
+
+ it { gitlab_shell.exists?(group.path).should be_falsey }
+ it { gitlab_shell.exists?(remove_path).should 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) }
+ end
+
+ it { gitlab_shell.exists?(group.path).should be_falsey }
+ it { gitlab_shell.exists?(remove_path).should be_truthy }
+ end
+ end
+
+ def destroy_group(group, user)
+ DestroyGroupService.new(group, user).execute
+ end
+end