Merge branch 'ee-31885-ability-to-transfer-groups-to-another-group' into 'master'

Port to EE: 31885 Ability to transfer a single group to another group

See merge request gitlab-org/gitlab-ee!4391

app/assets/javascripts/groups/transfer_dropdown.js

+export default class TransferDropdown {
+  constructor() {
+    this.groupDropdown = $('.js-groups-dropdown');
+    this.parentInput = $('#new_parent_group_id');
+    this.data = this.groupDropdown.data('data');
+    this.init();
+  }
+
+  init() {
+    this.buildDropdown();
+  }
+
+  buildDropdown() {
+    const extraOptions = [{ id: '', text: 'No parent group' }, 'divider'];
+
+    this.groupDropdown.glDropdown({
+      selectable: true,
+      filterable: true,
+      toggleLabel: item => item.text,
+      search: { fields: ['text'] },
+      data: extraOptions.concat(this.data),
+      text: item => item.text,
+      clicked: (options) => {
+        const { e } = options;
+        e.preventDefault();
+        this.assignSelected(options.selectedObj);
+      },
+    });
+  }
+
+  assignSelected(selected) {
+    this.parentInput.val(selected.id);
+  }
+}

app/assets/javascripts/pages/groups/edit/index.js

 import groupAvatar from '~/group_avatar';
+import TransferDropdown from '~/groups/transfer_dropdown';
 
-export default groupAvatar;
+export default () => {
+  groupAvatar();
+  new TransferDropdown(); // eslint-disable-line no-new
+};

app/assets/stylesheets/pages/groups.scss

@@ -273,3 +273,16 @@ table.pipeline-project-metrics tr td {
   border-radius: $label-border-radius;
   font-weight: $gl-font-weight-normal;
 }
+
+.js-groups-dropdown {
+  width: 100%;
+}
+
+.dropdown-group-transfer {
+  bottom: 100%;
+  top: initial;
+
+  .dropdown-content {
+    overflow-y: unset;
+  }
+}

app/controllers/groups_controller.rb

@@ -11,7 +11,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,6 +95,19 @@ class GroupsController < Groups::ApplicationController
     redirect_to root_path, status: 302, alert: "Group '#{@group.name}' was scheduled for deletion."
   end
 
+  def transfer
+    parent_group = Group.find_by(id: params[:new_parent_group_id])
+    service = ::Groups::TransferService.new(@group, current_user)
+
+    if service.execute(parent_group)
+      flash[:notice] = "Group '#{@group.name}' was successfully transferred."
+      redirect_to group_path(@group)
+    else
+      flash.now[:alert] = service.error
+      render :edit
+    end
+  end
+
   protected
 
   def authorize_create_group!

app/helpers/groups_helper.rb

@@ -94,6 +94,19 @@ module GroupsHelper
     end
   end
 
+  def parent_group_options(current_group)
+    groups = current_user.owned_groups.sort_by(&:human_name).map do |group|
+      { id: group.id, text: group.human_name }
+    end
+
+    groups.delete_if { |group| group[:id] == current_group.id }
+    groups.to_json
+  end
+
+  def supports_nested_groups?
+    Group.supports_nested_groups?
+  end
+
   private
 
   def group_title_link(group, hidable: false, show_avatar: false, for_dropdown: false)

app/models/concerns/storage/legacy_namespace.rb

@@ -14,7 +14,11 @@ module Storage
         # Ensure old directory exists before moving it
         gitlab_shell.add_namespace(repository_storage_path, full_path_was)
 
+        # Ensure new directory exists before moving it (if there's a parent)
+        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

app/models/namespace.rb

@@ -41,7 +41,6 @@ class Namespace < ActiveRecord::Base
     namespace_path: true
 
   validate :nesting_level_allowed
-  validate :allowed_path_by_redirects
 
   delegate :name, to: :owner, allow_nil: true, prefix: true
 
@@ -54,7 +53,7 @@ class Namespace < ActiveRecord::Base
 
   # Legacy Storage specific hooks
 
-  after_update :move_dir, if: :path_changed?
+  after_update :move_dir, if: :path_or_parent_changed?
   before_destroy(prepend: true) { prepare_for_destroy }
   after_destroy :rm_dir
 
@@ -236,9 +235,12 @@ class Namespace < ActiveRecord::Base
   end
 
   def full_path_was
-    return path_was unless has_parent?
-
-    "#{parent.full_path}/#{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
 
   # Exports belonging to projects with legacy storage are placed in a common
@@ -255,6 +257,10 @@ class Namespace < ActiveRecord::Base
 
   private
 
+  def path_or_parent_changed?
+    path_changed? || parent_changed?
+  end
+
   def refresh_access_of_projects_invited_groups
     Group
       .joins(project_group_links: :project)
@@ -291,16 +297,6 @@ class Namespace < ActiveRecord::Base
       .update_all(share_with_group_lock: true)
   end
 
-  def allowed_path_by_redirects
-    return if path.nil?
-
-    errors.add(:path, "#{path} has been taken before. Please use another one") if namespace_previously_created_with_same_path?
-  end
-
-  def namespace_previously_created_with_same_path?
-    RedirectRoute.permanent.exists?(path: path)
-  end
-
   def write_projects_repository_config
     all_projects.find_each do |project|
       project.expires_full_path_cache # we need to clear cache to validate renames correctly

app/services/groups/transfer_service.rb

+module Groups
+  class TransferService < Groups::BaseService
+    ERROR_MESSAGES = {
+      database_not_supported: 'Database is not supported.',
+      namespace_with_same_path: 'The parent group already has a subgroup 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 = {})
+      super
+      @error = nil
+    end
+
+    def execute(new_parent_group)
+      @new_parent_group = new_parent_group
+      ensure_allowed_transfer
+      proceed_to_transfer
+
+    rescue TransferError, ActiveRecord::RecordInvalid, Gitlab::UpdatePathError => e
+      @group.errors.clear
+      @error = "Transfer failed: " + e.message
+      false
+    end
+
+    private
+
+    def proceed_to_transfer
+      Group.transaction do
+        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(:invalid_policies) unless valid_policies?
+      raise_transfer_error(:namespace_with_same_path) if namespace_with_same_path?
+    end
+
+    def group_is_already_root?
+      !@new_parent_group && !@group.has_parent?
+    end
+
+    def same_parent?
+      @new_parent_group && @new_parent_group.id == @group.parent_id
+    end
+
+    def valid_policies?
+      return false unless can?(current_user, :admin_group, @group)
+
+      if @new_parent_group
+        can?(current_user, :create_subgroup, @new_parent_group)
+      else
+        can?(current_user, :create_group)
+      end
+    end
+
+    def namespace_with_same_path?
+      Namespace.exists?(path: @group.path, parent: @new_parent_group)
+    end
+
+    def update_group_attributes
+      if @new_parent_group && @new_parent_group.visibility_level < @group.visibility_level
+        update_children_and_projects_visibility
+        @group.visibility_level = @new_parent_group.visibility_level
+      end
+
+      @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)
+
+      Group
+        .where(id: descendants.select(:id))
+        .update_all(visibility_level: @new_parent_group.visibility_level)
+
+      @group
+        .all_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

app/views/groups/edit.html.haml

@@ -61,4 +61,20 @@
       .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 do |f|
+        .form-group
+          = dropdown_tag('Select parent group', options: { toggle_class: 'js-groups-dropdown', title: 'Parent Group', filter: true, dropdown_class: 'dropdown-open-top dropdown-group-transfer', placeholder: "Search groups", data: { data: parent_group_options(@group) } })
+          = hidden_field_tag 'new_parent_group_id'
+
+        %ul
+          %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 If the parent group's visibility is lower than the group current visibility, visibility levels for subgroups and projects will be changed to match the new parent group's visibility.
+        = f.submit 'Transfer group', class: "btn btn-warning"
+
 = render 'shared/confirm_modal', phrase: @group.path

changelogs/unreleased/31885-ability-to-transfer-groups-to-another-group.yml

+---
+title: Add ability to transfer a group into another group
+merge_request: 16302
+author:
+type: added

config/routes/group.rb

@@ -15,6 +15,7 @@ constraints(GroupUrlConstrainer.new) do
       get :projects, as: :projects_group
       get :activity, as: :activity_group
       get :subgroups, as: :subgroups_group ## EE-specific
+      put :transfer, as: :transfer_group
     end
 
     get '/', action: :show, as: :group_canonical

doc/user/group/index.md

@@ -189,6 +189,20 @@ milestones.
 
 [Learn more about Epics.](epics/index.md)
 
+## 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.
+- If the parent group's visibility is lower than the group current visibility, visibility levels for subgroups and projects will be changed to match the new parent group's visibility.
+
 ## Group settings
 
 Once you have created a group, you can manage its settings by navigating to

doc/user/project/index.md

@@ -132,8 +132,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

spec/controllers/groups_controller_spec.rb

@@ -535,4 +535,87 @@ describe GroupsController do
       "Group '#{redirect_route.path}' was moved to '#{group.full_path}'. Please update any links and bookmarks that may still have the old path."
     end
   end
+
+  describe 'PUT transfer', :postgresql do
+    before do
+      sign_in(user)
+    end
+
+    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
+        put :transfer,
+          id: group.to_param,
+          new_parent_group_id: new_parent_group.id
+      end
+
+      it 'should return a notice' do
+        expect(flash[:notice]).to eq("Group '#{group.name}' was successfully transferred.")
+      end
+
+      it 'should redirect to the new path' do
+        expect(response).to redirect_to("/#{new_parent_group.path}/#{group.path}")
+      end
+    end
+
+    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
+        put :transfer,
+          id: group.to_param,
+          new_parent_group_id: ''
+      end
+
+      it 'should return a notice' do
+        expect(flash[:notice]).to eq("Group '#{group.name}' was successfully transferred.")
+      end
+
+      it 'should redirect to the new path' do
+        expect(response).to redirect_to("/#{group.path}")
+      end
+    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) }
+
+      before do
+        allow_any_instance_of(::Groups::TransferService).to receive(:proceed_to_transfer).and_raise(Gitlab::UpdatePathError, 'namespace directory cannot be moved')
+
+        put :transfer,
+          id: group.to_param,
+          new_parent_group_id: new_parent_group.id
+      end
+
+      it 'should return an alert' do
+        expect(flash[:alert]).to eq "Transfer failed: namespace directory cannot be moved"
+      end
+
+      it 'should redirect to the current path' do
+        expect(response).to render_template(:edit)
+      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

spec/models/group_spec.rb

@@ -602,4 +602,20 @@ describe Group do
       end
     end
   end
+
+  describe '#has_parent?' do
+    context 'when the group has a parent' do
+      it 'should be truthy' do
+        group = create(:group, :nested)
+        expect(group.has_parent?).to be_truthy
+      end
+    end
+
+    context 'when the group has no parent' do
+      it 'should be falsy' do
+        group = create(:group, parent: nil)
+        expect(group.has_parent?).to be_falsy
+      end
+    end
+  end
 end

spec/models/namespace_spec.rb

@@ -704,36 +704,6 @@ describe Namespace do
     end
   end
 
-  describe "#allowed_path_by_redirects" do
-    let(:namespace1) { create(:namespace, path: 'foo') }
-
-    context "when the path has been taken before" do
-      before do
-        namespace1.path = 'bar'
-        namespace1.save!
-      end
-
-      it 'should be invalid' do
-        namespace2 = build(:group, path: 'foo')
-        expect(namespace2).to be_invalid
-      end
-
-      it 'should return an error on path' do
-        namespace2 = build(:group, path: 'foo')
-        namespace2.valid?
-        expect(namespace2.errors.messages[:path].first).to eq('foo has been taken before. Please use another one')
-      end
-    end
-
-    context "when the path has not been taken before" do
-      it 'should be valid' do
-        expect(RedirectRoute.count).to eq(0)
-        namespace = build(:namespace)
-        expect(namespace).to be_valid
-      end
-    end
-  end
-
   describe '#remove_exports' do
     let(:legacy_project) { create(:project, :with_export, namespace: namespace) }
     let(:hashed_project) { create(:project, :with_export, :hashed, namespace: namespace) }
@@ -753,4 +723,44 @@ describe Namespace do
       expect(File.exist?(hashed_export)).to be_falsy
     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

spec/models/user_spec.rb

@@ -2720,7 +2720,7 @@ describe User do
 
       it 'should raise an ActiveRecord::RecordInvalid exception' do
         user2 = build(:user, username: 'foo')
-        expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Path foo has been taken before/)
+        expect { user2.save! }.to raise_error(ActiveRecord::RecordInvalid, /Route path foo has been taken before. Please use another one, Route is invalid/)
       end
     end