Fixes MR suggestions

- Makes code more self descriptive
- Add documentation for transferring groups

8 files changed, 107 insertions, 27 deletions

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 54c0bb4acb8..8112381d3db 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -10,7 +10,7 @@ class GroupsController < Groups::ApplicationController
   before_action :group, except: [:index, :new, :create]
 
   # Authorize
-  before_action :authorize_admin_group!, only: [:edit, :update, :destroy, :projects]
+  before_action :authorize_admin_group!, only: [:edit, :update, :destroy, :projects, :transfer]
   before_action :authorize_create_group!, only: [:new]
 
   before_action :group_projects, only: [:projects, :activity, :issues, :merge_requests]
@@ -95,8 +95,6 @@ class GroupsController < Groups::ApplicationController
   end
 
   def transfer
-    return access_denied! unless can?(current_user, :admin_group, @group)
-
     parent_group = Group.find_by(id: params[:new_parent_group_id])
     service = ::Groups::TransferService.new(@group, current_user)
 
diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb
index 1401ffdf1ea..d6807604804 100644
--- a/app/helpers/groups_helper.rb
+++ b/app/helpers/groups_helper.rb
@@ -89,8 +89,8 @@ module GroupsHelper
   end
 
   def parent_group_options
-    groups = current_user.owned_groups
-    options_for_select(groups.map { |group| [group.full_path, group.id] })
+    groups = current_user.owned_groups.sort_by(&:human_name)
+    options_for_select(groups.map { |group| [group.human_name, group.id] })
   end
 
   def supports_nested_groups?
diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb
index 6f6f33c2903..fcf50e2439b 100644
--- a/app/services/groups/transfer_service.rb
+++ b/app/services/groups/transfer_service.rb
@@ -31,46 +31,44 @@ module Groups
 
     def proceed_to_transfer
       Group.transaction do
-        update_children_and_projects_visibility if @new_parent_group.present?
+        update_children_and_projects_visibility if @new_parent_group
         update_group_attributes
       end
     end
 
     def ensure_allowed_transfer
-      raise_transfer_error(:group_is_already_root) if group_is_already_root
+      raise_transfer_error(:group_is_already_root) if group_is_already_root?
       raise_transfer_error(:database_not_supported) unless Group.supports_nested_groups?
       raise_transfer_error(:same_parent_as_current) if same_parent?
-      raise_transfer_error(:group_with_same_path) if group_with_same_path?
       raise_transfer_error(:invalid_policies) unless valid_policies?
+      raise_transfer_error(:group_with_same_path) if group_with_same_path?
     end
 
-    def group_is_already_root
-      @new_parent_group.blank? && @group.parent.nil?
+    def group_is_already_root?
+      !@new_parent_group && !@group.has_parent?
     end
 
     def same_parent?
-      @new_parent_group.present? && @new_parent_group.id == @group.parent_id
+      @new_parent_group && @new_parent_group.id == @group.parent_id
     end
 
     def valid_policies?
-      if @new_parent_group.present?
-        can?(current_user, :admin_group, @group) &&
-          can?(current_user, :create_subgroup, @new_parent_group)
-      else
-        can?(current_user, :admin_group, @group)
-      end
+      return false unless can?(current_user, :admin_group, @group)
+      return false if @new_parent_group && !can?(current_user, :create_subgroup, @new_parent_group)
+      return false if !@new_parent_group && !can?(current_user, :create_group)
+
+      true
     end
 
     def group_with_same_path?
-      if @new_parent_group.present?
-        @new_parent_group.children.exists?(path: @group.path)
-      else
-        Group.exists?(path: @group.path, parent: nil)
-      end
+      Group.exists?(path: @group.path, parent: @new_parent_group)
     end
 
     def update_group_attributes
-      @group.visibility_level = @new_parent_group.visibility_level if @new_parent_group.present?
+      if @new_parent_group && @new_parent_group.visibility_level < @group.visibility_level
+        @group.visibility_level = @new_parent_group.visibility_level
+      end
+
       @group.parent = @new_parent_group
       @group.save!
     end
@@ -82,7 +80,7 @@ module Groups
       end
 
       @group
-        .projects
+        .all_projects
         .where("visibility_level > ?", @new_parent_group.visibility_level)
         .update_all(visibility_level: @new_parent_group.visibility_level)
     end
diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml
index 6d844c3e428..e5a2822fc5a 100644
--- a/app/views/groups/edit.html.haml
+++ b/app/views/groups/edit.html.haml
@@ -68,7 +68,8 @@
           .form-group
             = select_tag :new_parent_group_id, parent_group_options, include_blank: 'No parent group', class: 'select2'
           %ul
-            %li Be careful. Changing a group's parent can have unintended side effects.
+            %li 
+              = "Be careful. Changing a group's parent can have unintended #{link_to 'side effects', 'https://docs.gitlab.com/ce/user/project/index.html#redirects-when-changing-repository-paths', target: 'blank'}."
             %li You can only transfer the group to a group you manage.
             %li You will need to update your local repositories to point to the new location.
             %li Group and project visibility levels will be changed to match the new parent group's values.
diff --git a/doc/user/group/index.md b/doc/user/group/index.md
index 7f77a33aadc..daa0345db0a 100644
--- a/doc/user/group/index.md
+++ b/doc/user/group/index.md
@@ -168,6 +168,20 @@ Alternatively, you can [lock the sharing with group feature](#share-with-group-l
 In GitLab Enterprise Edition it is possible to manage GitLab group memberships using LDAP groups.
 See [the GitLab Enterprise Edition documentation](../../integration/ldap.md) for more information.
 
+## Transfer groups to another group
+
+From 10.5 there are two different ways to transfer a group:
+
+- Either by transferring a group into another group (making it a subgroup of that group).
+- Or by converting a subgroup into a root group (a group with no parent).
+
+Please make sure to understand that:
+
+- Changing a group's parent can have unintended side effects. See [Redirects when changing repository paths](https://docs.gitlab.com/ce/user/project/index.html#redirects-when-changing-repository-paths)
+- You can only transfer the group to a group you manage.
+- You will need to update your local repositories to point to the new location.
+- Group and project visibility levels will be changed to match the new parent group's values.
+
 ## Group settings
 
 Once you have created a group, you can manage its settings by navigating to
diff --git a/doc/user/project/index.md b/doc/user/project/index.md
index 4c772c62f8d..77eba8eda7c 100644
--- a/doc/user/project/index.md
+++ b/doc/user/project/index.md
@@ -128,8 +128,7 @@ and Git push/pull redirects.
 
 Depending on the situation, different things apply.
 
-When [transferring a project](settings/index.md#transferring-an-existing-project-into-another-namespace),
-or [renaming a user](../profile/index.md#changing-your-username) or
+When [renaming a user](../profile/index.md#changing-your-username) or
 [changing a group path](../group/index.md#changing-a-group-s-path):
 
 - **The redirect to the new URL is permanent**, which means that the original
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index 87441a282fa..7f4c6deb66c 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -562,5 +562,21 @@ describe GroupsController do
         expect(response).to redirect_to("/#{group.path}")
       end
     end
+
+    context 'when the user is not allowed to transfer the group' do
+      let(:new_parent_group) { create(:group, :public) }
+      let!(:group_member) { create(:group_member, :guest, group: group, user: user) }
+      let!(:new_parent_group_member) { create(:group_member, :guest, group: new_parent_group, user: user) }
+
+      before do
+        put :transfer,
+          id: group.to_param,
+          new_parent_group_id: new_parent_group.id
+      end
+
+      it 'should be denied' do
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
   end
 end
diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb
index b525e11590e..b32953982c0 100644
--- a/spec/services/groups/transfer_service_spec.rb
+++ b/spec/services/groups/transfer_service_spec.rb
@@ -168,6 +168,28 @@ describe Groups::TransferService, :postgresql do
           transfer_service.execute(new_parent_group)
         end
 
+        context 'when the group has a lower visibility than the parent group' do
+          let(:new_parent_group) { create(:group, :public) }
+          let(:group) { create(:group, :private, :nested) }
+
+          it 'should not update the visibility for the group' do
+            group.reload
+            expect(group.private?).to be_truthy
+            expect(group.visibility_level).not_to eq(new_parent_group.visibility_level)
+          end
+        end
+
+        context 'when the group has a higher visibility than the parent group' do
+          let(:new_parent_group) { create(:group, :private) }
+          let(:group) { create(:group, :public, :nested) }
+
+          it 'should update visibility level based on the parent group' do
+            group.reload
+            expect(group.private?).to be_truthy
+            expect(group.visibility_level).to eq(new_parent_group.visibility_level)
+          end
+        end
+
         it 'should update visibility for the group based on the parent group' do
           expect(group.visibility_level).to eq(new_parent_group.visibility_level)
         end
@@ -180,6 +202,10 @@ describe Groups::TransferService, :postgresql do
           expect(new_parent_group.children.count).to eq(1)
           expect(new_parent_group.children.first).to eq(group)
         end
+
+        it 'should create a permanent redirect for the group' do
+          expect(group.redirect_routes.permanent.count).to eq(1)
+        end
       end
 
       context 'when transferring a group with group descendants' do
@@ -198,6 +224,12 @@ describe Groups::TransferService, :postgresql do
           end
         end
 
+        it 'should create permanent redirects for the subgroups' do
+          expect(group.redirect_routes.permanent.count).to eq(1)
+          expect(subgroup1.redirect_routes.permanent.count).to eq(1)
+          expect(subgroup2.redirect_routes.permanent.count).to eq(1)
+        end
+
         context 'when the new parent has a higher visibility than the children' do
           it 'should not update the children visibility' do
             expect(subgroup1.private?).to be_truthy
@@ -235,6 +267,12 @@ describe Groups::TransferService, :postgresql do
           end
         end
 
+        it 'should create permanent redirects for the projects' do
+          expect(group.redirect_routes.permanent.count).to eq(1)
+          expect(project1.redirect_routes.permanent.count).to eq(1)
+          expect(project2.redirect_routes.permanent.count).to eq(1)
+        end
+
         context 'when the new parent has a higher visibility than the projects' do
           it 'should not update projects visibility' do
             expect(project1.private?).to be_truthy
@@ -280,6 +318,14 @@ describe Groups::TransferService, :postgresql do
             expect(project.full_path).to eq("#{new_parent_path}/#{group.path}/#{project.name}")
           end
         end
+
+        it 'should create permanent redirect for the subgroups and projects' do
+          expect(group.redirect_routes.permanent.count).to eq(1)
+          expect(subgroup1.redirect_routes.permanent.count).to eq(1)
+          expect(subgroup2.redirect_routes.permanent.count).to eq(1)
+          expect(project1.redirect_routes.permanent.count).to eq(1)
+          expect(project2.redirect_routes.permanent.count).to eq(1)
+        end
       end
 
       context 'when transfering a group with nested groups and projects' do
@@ -314,6 +360,14 @@ describe Groups::TransferService, :postgresql do
             expect(project.full_path).to eq(project_full_path)
           end
         end
+
+        it 'should create permanent redirect for the subgroups and projects' do
+          expect(group.redirect_routes.permanent.count).to eq(1)
+          expect(project1.redirect_routes.permanent.count).to eq(1)
+          expect(subgroup1.redirect_routes.permanent.count).to eq(1)
+          expect(nested_subgroup.redirect_routes.permanent.count).to eq(1)
+          expect(nested_project.redirect_routes.permanent.count).to eq(1)
+        end
       end
 
       context 'when updating the group goes wrong' do