Combine two services into a single one

- Removes extra code and just use a generic transfer service class
- Readjust UI to new design
- Fixes specs to adjust to new code
- Adapt full_path_was on Group model for parent/no parent scenarios

19 files changed, 512 insertions, 554 deletions

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index e5c3046a097..54c0bb4acb8 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -98,20 +98,15 @@ class GroupsController < Groups::ApplicationController
     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)
 
-    service = ::Groups::ConvertToSubgroupService.new(@group, current_user)
-    service.execute(parent_group)
-
-    flash[:alert] = service.error if service.error.present?
-  end
-
-  def convert_to_root
-    return access_denied! unless can?(current_user, :admin_group, @group)
-
-    service = ::Groups::ConvertToRootService.new(@group, current_user)
-    service.execute
+    if service.execute(parent_group)
+      flash[:notice] = "Group '#{@group.name}' was successfully transferred."
+    else
+      flash[:alert] = service.error
+    end
 
-    flash[:alert] = service.error if service.error.present?
+    redirect_to @group
   end
 
   protected
diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb
index bf85c97e4c4..1401ffdf1ea 100644
--- a/app/helpers/groups_helper.rb
+++ b/app/helpers/groups_helper.rb
@@ -97,10 +97,6 @@ module GroupsHelper
     Group.supports_nested_groups?
   end
 
-  def group_can_become_root?(group)
-    group.has_parent? && can?(current_user, :create_group)
-  end
-
   private
 
   def group_title_link(group, hidable: false, show_avatar: false, for_dropdown: false)
diff --git a/app/models/concerns/storage/legacy_namespace.rb b/app/models/concerns/storage/legacy_namespace.rb
index f3535a23702..67a988addbe 100644
--- a/app/models/concerns/storage/legacy_namespace.rb
+++ b/app/models/concerns/storage/legacy_namespace.rb
@@ -18,6 +18,7 @@ module Storage
         gitlab_shell.add_namespace(repository_storage_path, parent.full_path) if parent
 
         unless gitlab_shell.mv_namespace(repository_storage_path, full_path_was, full_path)
+
           Rails.logger.error "Exception moving path #{repository_storage_path} from #{full_path_was} to #{full_path}"
 
           # if we cannot move namespace directory we should rollback
diff --git a/app/models/group.rb b/app/models/group.rb
index cb46302af62..8d17c8befee 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -274,6 +274,15 @@ class Group < Namespace
     list_of_ids.reverse.map { |group| variables[group.id] }.compact.flatten
   end
 
+  def full_path_was
+    if parent_id_was.nil?
+      path_was
+    else
+      previous_parent = Group.find_by(id: parent_id_was)
+      previous_parent.full_path + '/' + path_was
+    end
+  end
+
   def group_member(user)
     if group_members.loaded?
       group_members.find { |gm| gm.user_id == user.id }
@@ -286,10 +295,6 @@ class Group < Namespace
     false
   end
 
-  def has_parent?
-    !parent_id.nil?
-  end
-
   private
 
   def update_two_factor_requirement
diff --git a/app/services/groups/convert_to_root_service.rb b/app/services/groups/convert_to_root_service.rb
deleted file mode 100644
index 7efbfd97b3b..00000000000
--- a/app/services/groups/convert_to_root_service.rb
+++ /dev/null
@@ -1,35 +0,0 @@
-module Groups
-  class ConvertToRootService < TransferService
-    def execute
-      with_transfer_error_handling do
-        proceed_to_transfer
-      end
-    end
-
-    private
-
-    def proceed_to_transfer
-      ensure_policies
-      update_group_attributes
-    end
-
-    def ensure_policies
-      raise_transfer_error(:invalid_policies) unless valid_policies?
-      raise_transfer_error(:already_a_root_group) if root_group?
-    end
-
-    def valid_policies?
-      can?(current_user, :admin_group, @group) &&
-        can?(current_user, :create_group)
-    end
-
-    def root_group?
-      !@group.has_parent?
-    end
-
-    def update_group_attributes
-      @group.parent = nil
-      @group.save!
-    end
-  end
-end
diff --git a/app/services/groups/convert_to_subgroup_service.rb b/app/services/groups/convert_to_subgroup_service.rb
deleted file mode 100644
index 786aadac948..00000000000
--- a/app/services/groups/convert_to_subgroup_service.rb
+++ /dev/null
@@ -1,66 +0,0 @@
-module Groups
-  class ConvertToSubgroupService < TransferService
-    def execute(new_parent_group)
-      @new_parent_group = new_parent_group
-
-      with_transfer_error_handling do
-        proceed_to_transfer
-      end
-    end
-
-    private
-
-    def proceed_to_transfer
-      ensure_allowed_transfer
-      update_visibility(new_visibility_level)
-      update_group_attributes
-    end
-
-    def ensure_allowed_transfer
-      raise_transfer_error(:database_not_supported) unless Group.supports_nested_groups?
-      raise_transfer_error(:missing_parent) if @new_parent_group.blank?
-      raise_transfer_error(:same_parent_as_current) if same_parent?
-      raise_transfer_error(:invalid_policies) unless valid_policies?
-      raise_transfer_error(:group_with_same_path) if group_with_same_path?
-      raise_transfer_error(visibility_error_for(@group, @new_parent_group)) unless allowed_visibility?
-    end
-
-    def same_parent?
-      @new_parent_group.id == @group.parent_id
-    end
-
-    def valid_policies?
-      can?(current_user, :admin_group, @group) &&
-        can?(current_user, :create_subgroup, @new_parent_group)
-    end
-
-    def allowed_visibility?
-      @group.visibility_level <= new_visibility_level
-    end
-
-    def update_visibility(visibility_level)
-      @group.projects.update_all(visibility_level: visibility_level)
-      @group.self_and_descendants.each do |subgroup|
-        subgroup.update_attribute(:visibility_level, visibility_level)
-      end
-      @group.all_projects.update_all(visibility_level: visibility_level)
-    end
-
-    def update_group_attributes
-      old_visibility_level = @group.visibility_level
-      @group.visibility_level = new_visibility_level
-      @group.parent = @new_parent_group
-      @group.save!
-    rescue ActiveRecord::RecordInvalid
-      update_visibility(old_visibility_level)
-    end
-
-    def group_with_same_path?
-      @new_parent_group.children.exists?(path: @group.path)
-    end
-
-    def new_visibility_level
-      @new_parent_group.visibility_level
-    end
-  end
-end
diff --git a/app/services/groups/transfer_error_messages.rb b/app/services/groups/transfer_error_messages.rb
deleted file mode 100644
index d3f50d7fc36..00000000000
--- a/app/services/groups/transfer_error_messages.rb
+++ /dev/null
@@ -1,26 +0,0 @@
-module Groups
-  module TransferErrorMessages
-    def friendly_update_path_error
-      "Oops, there was a problem transferring the group, this is not your fault. Please contact an admin."
-    end
-
-    def error_messages
-      {
-        already_a_root_group: "Group is already a root group.",
-        database_not_supported: "Database is not supported.",
-        group_with_same_path: "The parent group has a group with the same path.",
-        missing_parent: "Please select a new parent for your group.",
-        same_parent_as_current: "Group is already associated to the parent group.",
-        invalid_policies: "You don't have enough permissions."
-      }
-    end
-
-    def visibility_error_for(group, new_parent_group)
-      "#{gitlab_visibility_level(group.visibility_level)} #{group.path} cannot be transferred because the parent visibility is #{gitlab_visibility_level(new_parent_group.visibility_level)}."
-    end
-
-    def gitlab_visibility_level(visibility_level)
-      Gitlab::VisibilityLevel.string_level(visibility_level)
-    end
-  end
-end
diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb
index 58e8cca04bb..6f6f33c2903 100644
--- a/app/services/groups/transfer_service.rb
+++ b/app/services/groups/transfer_service.rb
@@ -1,8 +1,15 @@
 module Groups
   class TransferService < Groups::BaseService
-    include TransferErrorMessages
+    ERROR_MESSAGES = {
+      database_not_supported: 'Database is not supported.',
+      group_with_same_path: 'The parent group has a group with the same path.',
+      group_is_already_root: 'Group is already a root group.',
+      same_parent_as_current: 'Group is already associated to the parent group.',
+      invalid_policies: "You don't have enough permissions."
+    }.freeze
 
     TransferError = Class.new(StandardError)
+
     attr_reader :error
 
     def initialize(group, user, params = {})
@@ -10,24 +17,78 @@ module Groups
       @error = nil
     end
 
-    private
+    def execute(new_parent_group)
+      @new_parent_group = new_parent_group
+      ensure_allowed_transfer
+      proceed_to_transfer
 
-    def with_transfer_error_handling
-      yield
-    rescue TransferError => e
+    rescue TransferError, ActiveRecord::RecordInvalid, Gitlab::UpdatePathError => e
       @error = "Transfer failed: " + e.message
       false
-    rescue Gitlab::UpdatePathError
-      @error = friendly_update_path_error
-      false
     end
 
-    def raise_transfer_error(message)
-      if message.is_a?(Symbol)
-        raise TransferError, error_messages[message]
+    private
+
+    def proceed_to_transfer
+      Group.transaction do
+        update_children_and_projects_visibility if @new_parent_group.present?
+        update_group_attributes
+      end
+    end
+
+    def ensure_allowed_transfer
+      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?
+    end
+
+    def group_is_already_root
+      @new_parent_group.blank? && @group.parent.nil?
+    end
+
+    def same_parent?
+      @new_parent_group.present? && @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
-        raise TransferError, message
+        can?(current_user, :admin_group, @group)
+      end
+    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
+    end
+
+    def update_group_attributes
+      @group.visibility_level = @new_parent_group.visibility_level if @new_parent_group.present?
+      @group.parent = @new_parent_group
+      @group.save!
+    end
+
+    def update_children_and_projects_visibility
+      descendants = @group.descendants.where("visibility_level > ?", @new_parent_group.visibility_level)
+      descendants.each do |subgroup|
+        subgroup.update_column(:visibility_level, @new_parent_group.visibility_level)
       end
+
+      @group
+        .projects
+        .where("visibility_level > ?", @new_parent_group.visibility_level)
+        .update_all(visibility_level: @new_parent_group.visibility_level)
+    end
+
+    def raise_transfer_error(message)
+      raise TransferError, ERROR_MESSAGES[message]
     end
   end
 end
diff --git a/app/views/groups/convert_to_root.js.haml b/app/views/groups/convert_to_root.js.haml
deleted file mode 100644
index efe60f4791b..00000000000
--- a/app/views/groups/convert_to_root.js.haml
+++ /dev/null
@@ -1,2 +0,0 @@
-:plain
-  location.href = "#{edit_group_path(@group)}";
diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml
index 913eaf278b2..6d844c3e428 100644
--- a/app/views/groups/edit.html.haml
+++ b/app/views/groups/edit.html.haml
@@ -57,27 +57,21 @@
       .form-actions
         = button_to 'Remove group', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_group_message(@group) }
 
-
 - if supports_nested_groups?
   .panel.panel-warning
     .panel-heading Transfer group
     .panel-body
-      = form_for @group, url: transfer_group_path(@group), method: :put, remote: true do |f|
+      = form_for @group, url: transfer_group_path(@group), method: :put do |f|
         .form-group
           = label_tag :new_parent_group_id, nil, class: 'label-light' do
             %span Parent Group
           .form-group
-            = select_tag :new_parent_group_id, parent_group_options, include_blank: 'Select', class: 'select2'
+            = 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 You can only transfer the group to a group you owned.
+            %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.
-            %li Permissions of the members of the group will not be transferred. You will need to change them manually.
-        = f.submit 'Transfer Group', class: "btn btn-warning"
-        - if group_can_become_root?(@group)
-          or
-          = link_to 'Convert to root group', convert_to_root_group_path(@group), remote: true, method: :put, class: "btn btn-warning"
-
+        = f.submit 'Transfer group', class: "btn btn-warning"
 
 = render 'shared/confirm_modal', phrase: @group.path
diff --git a/app/views/groups/transfer.js.haml b/app/views/groups/transfer.js.haml
deleted file mode 100644
index efe60f4791b..00000000000
--- a/app/views/groups/transfer.js.haml
+++ /dev/null
@@ -1,2 +0,0 @@
-:plain
-  location.href = "#{edit_group_path(@group)}";
diff --git a/changelogs/unreleased/30547-ability-to-transfer-groups-to-another-group.yml b/changelogs/unreleased/30547-ability-to-transfer-groups-to-another-group.yml
deleted file mode 100644
index 1829d54a797..00000000000
--- a/changelogs/unreleased/30547-ability-to-transfer-groups-to-another-group.yml
+++ /dev/null
@@ -1,5 +0,0 @@
----
-title: Added ability to transfer a single group to another group
-merge_request:
-author: Mayra Cabrera
-type: added
diff --git a/changelogs/unreleased/31885-ability-to-transfer-groups-to-another-group.yml b/changelogs/unreleased/31885-ability-to-transfer-groups-to-another-group.yml
new file mode 100644
index 00000000000..d2a5802af64
--- /dev/null
+++ b/changelogs/unreleased/31885-ability-to-transfer-groups-to-another-group.yml
@@ -0,0 +1,5 @@
+---
+title: Add ability to transfer a group into another group
+merge_request: 16302
+author:
+type: added
diff --git a/config/routes/group.rb b/config/routes/group.rb
index 185feb1356c..b17611d8623 100644
--- a/config/routes/group.rb
+++ b/config/routes/group.rb
@@ -15,7 +15,6 @@ constraints(GroupUrlConstrainer.new) do
       get :projects, as: :projects_group
       get :activity, as: :activity_group
       put :transfer, as: :transfer_group
-      put :convert_to_root, as: :convert_to_root_group
     end
 
     get '/', action: :show, as: :group_canonical
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index 9f6b7510585..87441a282fa 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -497,134 +497,69 @@ describe GroupsController do
     end
   end
 
-  describe "PUT transfer" do
-    let(:new_parent_group) { create(:group, :public) }
-
+  describe 'PUT transfer', :postgresql do
     before do
       sign_in(user)
     end
 
-    context "When the current user has valid policies" do
-      before do
-        create(:group_member, :owner, group: new_parent_group, user: user)
-
-        put :transfer,
-          id: group.path,
-          new_parent_group_id: new_parent_group.id,
-          format: :js
-
-        group.reload
-      end
-
-      it "should be success" do
-        expect(response).to be_success
-      end
-
-      it "should update the parent for the group" do
-        expect(group.parent).to eq(new_parent_group)
-      end
-    end
-
-    context "When the current user has no valid policies" do
-      let(:previous_parent) { create(:group, :public) }
+    context 'when transfering to a subgroup goes right' do
+      let(:new_parent_group) { create(:group, :public) }
+      let!(:group_member) { create(:group_member, :owner, group: group, user: user) }
+      let!(:new_parent_group_member) { create(:group_member, :owner, group: new_parent_group, user: user) }
 
       before do
-        create(:group_member, :guest, group: new_parent_group, user: user)
-        group.update_attribute(:parent_id, previous_parent.id)
-
         put :transfer,
-          id: group.path,
-          new_parent_group_id: new_parent_group.id,
-          format: :js
-
-        group.reload
+          id: group.to_param,
+          new_parent_group_id: new_parent_group.id
       end
 
-      it "should not update the parent for the group" do
-        expect(group.parent).not_to be_nil
-        expect(group.parent).to eq(previous_parent)
+      it 'should return a notice' do
+        expect(flash[:notice]).to eq("Group '#{group.name}' was successfully transferred.")
       end
 
-      it "should be success" do
-        expect(response).to be_success
-      end
-
-      it "should return an alert" do
-        expect(flash[:alert]).to eq "Transfer failed: You don't have enough permissions."
+      it 'should redirect to the new path' do
+        expect(response).to redirect_to("/#{new_parent_group.path}/#{group.path}")
       end
     end
 
-    context "When parent_group is empty" do
-      let(:previous_parent) { create(:group, :public) }
+    context 'when converting to a root group goes right' do
+      let(:group) { create(:group, :public, :nested) }
+      let!(:group_member) { create(:group_member, :owner, group: group, user: user) }
 
       before do
-        group.update_attribute(:parent_id, previous_parent.id)
-
         put :transfer,
-          id: group.path,
-          new_parent_group_id: nil,
-          format: :js
-
-        group.reload
+          id: group.to_param,
+          new_parent_group_id: ''
       end
 
-      it "should not update the namespace for the group" do
-        expect(group.parent).not_to be_nil
-        expect(group.parent).to eq(previous_parent)
+      it 'should return a notice' do
+        expect(flash[:notice]).to eq("Group '#{group.name}' was successfully transferred.")
       end
 
-      it "should be success" do
-        expect(response).to be_success
-      end
-
-      it "should return an alert" do
-        expect(flash[:alert]).to eq "Transfer failed: Please select a new parent for your group."
+      it 'should redirect to the new path' do
+        expect(response).to redirect_to("/#{group.path}")
       end
     end
-  end
-
-  describe "PUT convert_to_root" do
-    before do
-      sign_in(user)
-    end
-
-    context "When the user has no valid policies" do
-      let!(:group_member) { create(:group_member, :guest, group: group, user: user) }
-
-      before do
-        put :convert_to_root,
-          id: group.path,
-          format: :js
-      end
 
-      it "should not be success" do
-        expect(response).not_to be_success
-      end
+    context 'When the transfer goes wrong' do
+      let(:new_parent_group) { create(:group, :public) }
+      let!(:group_member) { create(:group_member, :owner, group: group, user: user) }
+      let!(:new_parent_group_member) { create(:group_member, :owner, group: new_parent_group, user: user) }
 
-      it "should render access denied" do
-        expect(response).to render_template("errors/access_denied")
-      end
-    end
-
-    context "When the user has valid policies" do
       before do
-        put :convert_to_root,
-          id: group.path,
-          format: :js
-
-        group.reload
-      end
+        allow_any_instance_of(::Groups::TransferService).to receive(:proceed_to_transfer).and_raise(Gitlab::UpdatePathError, 'namespace directory cannot be moved')
 
-      it "should be success" do
-        expect(response).to be_success
+        put :transfer,
+          id: group.to_param,
+          new_parent_group_id: new_parent_group.id
       end
 
-      it "should update group attributes" do
-        expect(group.parent).to be_nil
+      it 'should return an alert' do
+        expect(flash[:alert]).to eq "Transfer failed: namespace directory cannot be moved"
       end
 
-      it "should convert group to root group" do
-        expect(group.full_path).to eq(group.name)
+      it 'should redirect to the current path' do
+        expect(response).to redirect_to("/#{group.path}")
       end
     end
   end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 5ea4acb6687..8cfbb864d10 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -598,4 +598,44 @@ describe Group do
       end
     end
   end
+
+  describe '#full_path_was' do
+    context 'when the group has no parent' do
+      it 'should return the path was' do
+        group = create(:group, parent: nil)
+        expect(group.full_path_was).to eq(group.path_was)
+      end
+    end
+
+    context 'when a parent is assigned to a group with no previous parent' do
+      it 'should return the path was' do
+        group = create(:group, parent: nil)
+
+        parent = create(:group)
+        group.parent = parent
+
+        expect(group.full_path_was).to eq("#{group.path_was}")
+      end
+    end
+
+    context 'when a parent is removed from the group' do
+      it 'should return the parent full path' do
+        parent = create(:group)
+        group = create(:group, parent: parent)
+        group.parent = nil
+
+        expect(group.full_path_was).to eq("#{parent.full_path}/#{group.path}")
+      end
+    end
+
+    context 'when changing parents' do
+      it 'should return the previous parent full path' do
+        parent = create(:group)
+        group = create(:group, parent: parent)
+        new_parent = create(:group)
+        group.parent = new_parent
+        expect(group.full_path_was).to eq("#{parent.full_path}/#{group.path}")
+      end
+    end
+  end
 end
diff --git a/spec/services/groups/convert_to_root_service_spec.rb b/spec/services/groups/convert_to_root_service_spec.rb
deleted file mode 100644
index 466f9eb221e..00000000000
--- a/spec/services/groups/convert_to_root_service_spec.rb
+++ /dev/null
@@ -1,65 +0,0 @@
-require 'rails_helper'
-
-describe Groups::ConvertToRootService do
-  let(:user) { create(:user) }
-  let(:group) { create(:group, :public, :nested) }
-  let(:convert_to_root_service) { described_class.new(group, user) }
-
-  describe "#execute" do
-    context "when the user does not have enough permissions" do
-      it "should return false" do
-        expect(convert_to_root_service.execute).to be_falsy
-      end
-
-      it "should add an error to group" do
-        convert_to_root_service.execute
-        expect(convert_to_root_service.error).to eq("Transfer failed: You don't have enough permissions.")
-      end
-    end
-
-    context "when the group is a root group" do
-      let!(:group_member) { create(:group_member, :owner, group: group, user: user) }
-
-      before do
-        group.update_attribute(:parent_id, nil)
-      end
-
-      it "should return false" do
-        expect(convert_to_root_service.execute).to be_falsy
-      end
-
-      it "should add an error to group" do
-        convert_to_root_service.execute
-        expect(convert_to_root_service.error).to eq("Transfer failed: Group is already a root group.")
-      end
-    end
-
-    context "when the user does have enough permissions" do
-      let!(:group_member) { create(:group_member, :owner, group: group, user: user) }
-
-      before do
-        create_list(:project, 2, :repository, namespace: group)
-        create_list(:group, 2, :public, parent: group)
-        convert_to_root_service.execute
-      end
-
-      it "should update group attributes" do
-        expect(group.parent).to be_nil
-      end
-
-      it "should transfer group's content to new root namespace" do
-        pending
-        group.projects.each do |project|
-          expect(project.full_path).to eq("#{group.name}/#{project.name}")
-        end
-      end
-
-      it "should transfer group's content to new directory" do
-        group.projects.each do |project|
-          full_path = project.repository_storage_path + '/' + project.full_path + '.git'
-          expect(File.exist?(full_path)).to be_truthy
-        end
-      end
-    end
-  end
-end
diff --git a/spec/services/groups/convert_to_subgroup_service_spec.rb b/spec/services/groups/convert_to_subgroup_service_spec.rb
deleted file mode 100644
index eb2be79192e..00000000000
--- a/spec/services/groups/convert_to_subgroup_service_spec.rb
+++ /dev/null
@@ -1,213 +0,0 @@
-require 'rails_helper'
-
-describe Groups::ConvertToSubgroupService do
-  let(:user) { create(:user) }
-  let(:group) { create(:group, :public, :nested) }
-  let(:new_parent_group) { create(:group, :public) }
-  let!(:group_member) { create(:group_member, :owner, group: group, user: user) }
-  let(:convert_to_subgroup_service) { described_class.new(group, user) }
-  let(:previous_parent) { create(:group, :public) }
-
-  describe "#execute" do
-    context "with other database than postgresql" do
-      before do
-        allow(Group).to receive(:supports_nested_groups?).and_return(false)
-      end
-
-      it "should return false" do
-        expect(convert_to_subgroup_service.execute(new_parent_group)).to be_falsy
-      end
-
-      it "should add an error on group" do
-        convert_to_subgroup_service.execute(new_parent_group)
-        expect(convert_to_subgroup_service.error).to eq('Transfer failed: Database is not supported.')
-      end
-    end
-
-    context "when the new parent group is the same as the previous parent group" do
-      it "should return false" do
-        new_parent_group = group.parent
-        expect(convert_to_subgroup_service.execute(new_parent_group)).to be_falsy
-      end
-
-      it "should add an error on group" do
-        new_parent_group = group.parent
-        convert_to_subgroup_service.execute(new_parent_group)
-        expect(convert_to_subgroup_service.error).to eq('Transfer failed: Group is already associated to the parent group.')
-      end
-    end
-
-    context "when the user does not have the right policies" do
-      let!(:group_member) { create(:group_member, :guest, group: group, user: user) }
-
-      it "should return false" do
-        expect(convert_to_subgroup_service.execute(new_parent_group)).to be_falsy
-      end
-
-      it "should add an error on group" do
-        convert_to_subgroup_service.execute(new_parent_group)
-        expect(convert_to_subgroup_service.error).to eq("Transfer failed: You don't have enough permissions.")
-      end
-    end
-
-    context "with no parent_group given" do
-      it "should return false" do
-        expect(convert_to_subgroup_service.execute(nil)).to be_falsy
-      end
-
-      it "should add an error on group" do
-        convert_to_subgroup_service.execute(nil)
-        expect(convert_to_subgroup_service.error).to eq('Transfer failed: Please select a new parent for your group.')
-      end
-    end
-
-    context "when the parent has higher visibility" do
-      let(:group) { create(:group, :public, :nested) }
-      let(:new_parent_group) { create(:group, :private) }
-      let(:convert_to_subgroup_service) { described_class.new(group, user) }
-
-      before do
-        create(:group_member, :owner, group: new_parent_group, user: user)
-      end
-
-      it "should return false" do
-        expect(convert_to_subgroup_service.execute(new_parent_group)).to be_falsy
-      end
-
-      it "should add an error on group" do
-        convert_to_subgroup_service.execute(new_parent_group)
-        expect(convert_to_subgroup_service.error).to eq("Transfer failed: public #{group.path} cannot be transferred because the parent visibility is private.")
-      end
-    end
-
-    context "when the parent has a group with the same name" do
-      before do
-        create(:group_member, :owner, group: new_parent_group, user: user)
-        group.update_attribute(:path, "not-unique")
-        create(:group, path: "not-unique", parent: new_parent_group)
-      end
-
-      it "should return false" do
-        expect(convert_to_subgroup_service.execute(new_parent_group)).to be_falsy
-      end
-
-      it "should add an error on group" do
-        convert_to_subgroup_service.execute(new_parent_group)
-        expect(convert_to_subgroup_service.error).to eq('Transfer failed: The parent group has a group with the same path.')
-      end
-    end
-
-    context "when there's an exception on Gitlab shell directories" do
-      let(:new_parent_group) { create(:group, :public) }
-
-      before do
-        allow_any_instance_of(described_class).to receive(:update_group_attributes).and_raise(Gitlab::UpdatePathError)
-        create(:group_member, :owner, group: new_parent_group, user: user)
-      end
-
-      it "should return false" do
-        expect(convert_to_subgroup_service.execute(new_parent_group)).to be_falsy
-      end
-
-      it "should add an error on group" do
-        convert_to_subgroup_service.execute(new_parent_group)
-        expect(convert_to_subgroup_service.error).to eq('Oops, there was a problem transferring the group, this is not your fault. Please contact an admin.')
-      end
-    end
-
-    context "with valid policies" do
-      let(:new_parent_group) { create(:group, :public) }
-
-      before do
-        create_list(:project, 2, :repository, namespace: group)
-        create_list(:project, 2, :repository, namespace: new_parent_group)
-        create_list(:group, 2, :public, parent: group)
-        create(:group_member, :owner, group: new_parent_group, user: user)
-        convert_to_subgroup_service.execute(new_parent_group)
-      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
-
-      it "should update visibility for the group projects" do
-        group.projects.each do |project|
-          expect(project.visibility_level).to eq(new_parent_group.visibility_level)
-        end
-      end
-
-      it "should update visibility for the group children" do
-        group.children.each do |g|
-          expect(g.visibility_level).to eq(new_parent_group.visibility_level)
-        end
-      end
-
-      it "should update visibility for the projects of the group children" do
-        group.children.each do |g|
-          g.projects.each do |project|
-            expect(project.visibility_level).to eq(new_parent_group.visibility_level)
-          end
-        end
-      end
-
-      it "should set new parent group as the parent" do
-        expect(group.parent).to eq(new_parent_group)
-      end
-
-      it "should transfer the projects to the new namespace" do
-        pending
-        group.projects.each do |project|
-          expect(project.full_path).to eq("#{new_parent_group.name}/#{group.name}/#{project.name}")
-        end
-      end
-
-      it "should transfer group's content to new directory" do
-        group.projects.each do |project|
-          full_path = project.repository_storage_path + '/' + project.full_path + '.git'
-          expect(File.exist?(full_path)).to be_truthy
-        end
-      end
-    end
-
-    context "when the parent group does not have a directory created" do
-      let(:new_parent_group) { create(:group, :public) }
-
-      before do
-        allow(described_class).to receive(:gitlab_storage_path).and_return(Rails.root.to_s + '/' + Gitlab.config.repositories["storages"]["default"]["path"])
-        create_list(:project, 2, :repository, namespace: group)
-        create_list(:group, 2, :public, parent: group)
-        create(:group_member, :owner, group: new_parent_group, user: user)
-        convert_to_subgroup_service.execute(new_parent_group)
-      end
-
-      it "should set the new parent group as the parent" do
-        expect(group.parent).to eq(new_parent_group)
-      end
-    end
-
-    context "when updating the group and this one is invalid" do
-      let(:group) { create(:group, :private, :nested) }
-
-      let(:new_parent_group) { create(:group, :public) }
-
-      before do
-        create_list(:project, 2, :repository, :private, namespace: group)
-        create(:group_member, :owner, group: new_parent_group, user: user)
-        @old_visibility = group.visibility_level
-        allow(group).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new(group))
-        convert_to_subgroup_service.execute(new_parent_group)
-        group.reload
-      end
-
-      it "should not update visilibility level for the group" do
-        expect(group.visibility_level).to eq(@old_visibility)
-      end
-
-      it "should restore the old visibility level for group's children" do
-        group.projects.each do |project|
-          expect(project.visibility_level).to eq(@old_visibility)
-        end
-      end
-    end
-  end
-end
diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb
new file mode 100644
index 00000000000..b525e11590e
--- /dev/null
+++ b/spec/services/groups/transfer_service_spec.rb
@@ -0,0 +1,341 @@
+require 'rails_helper'
+
+describe Groups::TransferService, :postgresql do
+  let(:user) { create(:user) }
+  let(:new_parent_group) { create(:group, :public) }
+  let!(:group_member) { create(:group_member, :owner, group: group, user: user) }
+  let(:transfer_service) { described_class.new(group, user) }
+
+  shared_examples 'ensuring allowed transfer for a group' do
+    context 'with other database than PostgreSQL' do
+      before do
+        allow(Group).to receive(:supports_nested_groups?).and_return(false)
+      end
+
+      it 'should return false' do
+        expect(transfer_service.execute(new_parent_group)).to be_falsy
+      end
+
+      it 'should add an error on group' do
+        transfer_service.execute(new_parent_group)
+        expect(transfer_service.error).to eq('Transfer failed: Database is not supported.')
+      end
+    end
+
+    context "when there's an exception on Gitlab shell directories" do
+      let(:new_parent_group) { create(:group, :public) }
+
+      before do
+        allow_any_instance_of(described_class).to receive(:update_group_attributes).and_raise(Gitlab::UpdatePathError, 'namespace directory cannot be moved')
+        create(:group_member, :owner, group: new_parent_group, user: user)
+      end
+
+      it 'should return false' do
+        expect(transfer_service.execute(new_parent_group)).to be_falsy
+      end
+
+      it 'should add an error on group' do
+        transfer_service.execute(new_parent_group)
+        expect(transfer_service.error).to eq('Transfer failed: namespace directory cannot be moved')
+      end
+    end
+  end
+
+  describe '#execute' do
+    context 'when transforming a group into a root group' do
+      let!(:group) { create(:group, :public, :nested) }
+
+      it_behaves_like 'ensuring allowed transfer for a group'
+
+      context 'when the group is already a root group' do
+        let(:group) { create(:group, :public) }
+
+        it 'should add an error on group' do
+          transfer_service.execute(nil)
+          expect(transfer_service.error).to eq('Transfer failed: Group is already a root group.')
+        end
+      end
+
+      context 'when the user does not have the right policies' do
+        let!(:group_member) { create(:group_member, :guest, group: group, user: user) }
+
+        it "should return false" do
+          expect(transfer_service.execute(nil)).to be_falsy
+        end
+
+        it "should add an error on group" do
+          transfer_service.execute(new_parent_group)
+          expect(transfer_service.error).to eq("Transfer failed: You don't have enough permissions.")
+        end
+      end
+
+      context 'when there is a group with the same path' do
+        let!(:group) { create(:group, :public, :nested, path: "not-unique") }
+
+        before do
+          create(:group, path: "not-unique")
+        end
+
+        it 'should return false' do
+          expect(transfer_service.execute(nil)).to be_falsy
+        end
+
+        it 'should add an error on group' do
+          transfer_service.execute(nil)
+          expect(transfer_service.error).to eq('Transfer failed: The parent group has a group with the same path.')
+        end
+      end
+
+      context 'when the group is a subgroup and the transfer is valid' do
+        let!(:subgroup1) { create(:group, :private, parent: group) }
+        let!(:subgroup2) { create(:group, :internal, parent: group) }
+        let!(:project1) { create(:project, :repository, :private, namespace: group) }
+
+        before do
+          transfer_service.execute(nil)
+          group.reload
+        end
+
+        it 'should update group attributes' do
+          expect(group.parent).to be_nil
+        end
+
+        it 'should update group children path' do
+          group.children.each do |subgroup|
+            expect(subgroup.full_path).to eq("#{group.path}/#{subgroup.path}")
+          end
+        end
+
+        it 'should update group projects path' do
+          group.projects.each do |project|
+            expect(project.full_path).to eq("#{group.path}/#{project.path}")
+          end
+        end
+      end
+    end
+
+    context 'when transferring a subgroup into another group' do
+      let(:group) { create(:group, :public, :nested) }
+
+      it_behaves_like 'ensuring allowed transfer for a group'
+
+      context 'when the new parent group is the same as the previous parent group' do
+        let(:group) { create(:group, :public, :nested, parent: new_parent_group) }
+
+        it 'should return false' do
+          expect(transfer_service.execute(new_parent_group)).to be_falsy
+        end
+
+        it 'should add an error on group' do
+          transfer_service.execute(new_parent_group)
+          expect(transfer_service.error).to eq('Transfer failed: Group is already associated to the parent group.')
+        end
+      end
+
+      context 'when the user does not have the right policies' do
+        let!(:group_member) { create(:group_member, :guest, group: group, user: user) }
+
+        it "should return false" do
+          expect(transfer_service.execute(new_parent_group)).to be_falsy
+        end
+
+        it "should add an error on group" do
+          transfer_service.execute(new_parent_group)
+          expect(transfer_service.error).to eq("Transfer failed: You don't have enough permissions.")
+        end
+      end
+
+      context 'when the parent has a group with the same path' do
+        before do
+          create(:group_member, :owner, group: new_parent_group, user: user)
+          group.update_attribute(:path, "not-unique")
+          create(:group, path: "not-unique", parent: new_parent_group)
+        end
+
+        it 'should return false' do
+          expect(transfer_service.execute(new_parent_group)).to be_falsy
+        end
+
+        it 'should add an error on group' do
+          transfer_service.execute(new_parent_group)
+          expect(transfer_service.error).to eq('Transfer failed: The parent group has a group with the same path.')
+        end
+      end
+
+      context 'when the group is allowed to be transferred' do
+        before do
+          create(:group_member, :owner, group: new_parent_group, user: user)
+          transfer_service.execute(new_parent_group)
+        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
+
+        it 'should update parent group to the new parent ' do
+          expect(group.parent).to eq(new_parent_group)
+        end
+
+        it 'should return the group as children of the new parent' do
+          expect(new_parent_group.children.count).to eq(1)
+          expect(new_parent_group.children.first).to eq(group)
+        end
+      end
+
+      context 'when transferring a group with group descendants' do
+        let!(:subgroup1) { create(:group, :private, parent: group) }
+        let!(:subgroup2) { create(:group, :internal, parent: group) }
+
+        before do
+          create(:group_member, :owner, group: new_parent_group, user: user)
+          transfer_service.execute(new_parent_group)
+        end
+
+        it 'should update subgroups path' do
+          new_parent_path = new_parent_group.path
+          group.children.each do |subgroup|
+            expect(subgroup.full_path).to eq("#{new_parent_path}/#{group.path}/#{subgroup.path}")
+          end
+        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
+            expect(subgroup2.internal?).to be_truthy
+          end
+        end
+
+        context 'when the new parent has a lower visibility than the children' do
+          let!(:subgroup1) { create(:group, :public, parent: group) }
+          let!(:subgroup2) { create(:group, :public, parent: group) }
+          let(:new_parent_group) { create(:group, :private) }
+
+          it 'should update children visibility to match the new parent' do
+            group.children.each do |subgroup|
+              expect(subgroup.private?).to be_truthy
+            end
+          end
+        end
+      end
+
+      context 'when transferring a group with project descendants' do
+        let!(:project1) { create(:project, :repository, :private, namespace: group) }
+        let!(:project2) { create(:project, :repository, :internal, namespace: group) }
+
+        before do
+          TestEnv.clean_test_path
+          create(:group_member, :owner, group: new_parent_group, user: user)
+          transfer_service.execute(new_parent_group)
+        end
+
+        it 'should update projects path' do
+          new_parent_path = new_parent_group.path
+          group.projects.each do |project|
+            expect(project.full_path).to eq("#{new_parent_path}/#{group.path}/#{project.name}")
+          end
+        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
+            expect(project2.internal?).to be_truthy
+          end
+        end
+
+        context 'when the new parent has a lower visibility than the projects' do
+          let!(:project1) { create(:project, :repository, :public, namespace: group) }
+          let!(:project2) { create(:project, :repository, :public, namespace: group) }
+          let(:new_parent_group) { create(:group, :private) }
+
+          it 'should update projects visibility to match the new parent' do
+            group.projects.each do |project|
+              expect(project.private?).to be_truthy
+            end
+          end
+        end
+      end
+
+      context 'when transferring a group with subgroups & projects descendants' do
+        let!(:project1) { create(:project, :repository, :private, namespace: group) }
+        let!(:project2) { create(:project, :repository, :internal, namespace: group) }
+        let!(:subgroup1) { create(:group, :private, parent: group) }
+        let!(:subgroup2) { create(:group, :internal, parent: group) }
+
+        before do
+          TestEnv.clean_test_path
+          create(:group_member, :owner, group: new_parent_group, user: user)
+          transfer_service.execute(new_parent_group)
+        end
+
+        it 'should update subgroups path' do
+          new_parent_path = new_parent_group.path
+          group.children.each do |subgroup|
+            expect(subgroup.full_path).to eq("#{new_parent_path}/#{group.path}/#{subgroup.path}")
+          end
+        end
+
+        it 'should update projects path' do
+          new_parent_path = new_parent_group.path
+          group.projects.each do |project|
+            expect(project.full_path).to eq("#{new_parent_path}/#{group.path}/#{project.name}")
+          end
+        end
+      end
+
+      context 'when transfering a group with nested groups and projects' do
+        let!(:group) { create(:group, :public) }
+        let!(:project1) { create(:project, :repository, :private, namespace: group) }
+        let!(:subgroup1) { create(:group, :private, parent: group) }
+        let!(:nested_subgroup) { create(:group, :private, parent: subgroup1) }
+        let!(:nested_project) { create(:project, :repository, :private, namespace: subgroup1) }
+
+        before do
+          TestEnv.clean_test_path
+          create(:group_member, :owner, group: new_parent_group, user: user)
+          transfer_service.execute(new_parent_group)
+        end
+
+        it 'should update subgroups path' do
+          new_base_path = "#{new_parent_group.path}/#{group.path}"
+          group.children.each do |children|
+            expect(children.full_path).to eq("#{new_base_path}/#{children.path}")
+          end
+
+          new_base_path = "#{new_parent_group.path}/#{group.path}/#{subgroup1.path}"
+          subgroup1.children.each do |children|
+            expect(children.full_path).to eq("#{new_base_path}/#{children.path}")
+          end
+        end
+
+        it 'should update projects path' do
+          new_parent_path = "#{new_parent_group.path}/#{group.path}"
+          subgroup1.projects.each do |project|
+            project_full_path = "#{new_parent_path}/#{project.namespace.path}/#{project.name}"
+            expect(project.full_path).to eq(project_full_path)
+          end
+        end
+      end
+
+      context 'when updating the group goes wrong' do
+        let!(:subgroup1) { create(:group, :public, parent: group) }
+        let!(:subgroup2) { create(:group, :public, parent: group) }
+        let(:new_parent_group) { create(:group, :private) }
+        let!(:project1) { create(:project, :repository, :public, namespace: group) }
+
+        before do
+          allow(group).to receive(:save!).and_raise(ActiveRecord::RecordInvalid.new(group))
+          TestEnv.clean_test_path
+          create(:group_member, :owner, group: new_parent_group, user: user)
+          transfer_service.execute(new_parent_group)
+        end
+
+        it 'should restore group and projects visibility' do
+          subgroup1.reload
+          project1.reload
+          expect(subgroup1.public?).to be_truthy
+          expect(project1.public?).to be_truthy
+        end
+      end
+    end
+  end
+end