Merge branch 'fix-group-remove' into 'master'

Group improvements

* remove projects before removing group
* execute all hooks/events from project destroy when group removed
* log group create/remove
* delay remove of namespace directory (to prevent NFS issues)

Inspired by !759

See merge request !761

10 files changed, 93 insertions, 8 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 9e866af2fec..1fd938a34cd 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 2dfae13ac5c..4d3e48f7f81 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 34f0b257db3..2e381822e42 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 b4e908c5602..051c672cb33 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 211dfa76b81..03d2ab165ea 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 d259b4efca6..9017a63af3b 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 00000000000..d929a676293
--- /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 29e8ba347d4..403f419ec50 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 f768c750402..e88b6e31775 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 00000000000..24e439503e7
--- /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