Commit fb11e8fd authored by Douwe Maan's avatar Douwe Maan

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

31885 - Ability to transfer a single group to another group

Closes #31885

See merge request gitlab-org/gitlab-ce!16302
parents 976413ad 68a419c8
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);
}
}
import groupAvatar from '~/group_avatar'; import groupAvatar from '~/group_avatar';
import TransferDropdown from '~/groups/transfer_dropdown';
export default groupAvatar; export default () => {
groupAvatar();
new TransferDropdown(); // eslint-disable-line no-new
};
...@@ -224,3 +224,16 @@ ...@@ -224,3 +224,16 @@
border-radius: $label-border-radius; border-radius: $label-border-radius;
font-weight: $gl-font-weight-normal; font-weight: $gl-font-weight-normal;
} }
.js-groups-dropdown {
width: 100%;
}
.dropdown-group-transfer {
bottom: 100%;
top: initial;
.dropdown-content {
overflow-y: unset;
}
}
...@@ -10,7 +10,7 @@ class GroupsController < Groups::ApplicationController ...@@ -10,7 +10,7 @@ class GroupsController < Groups::ApplicationController
before_action :group, except: [:index, :new, :create] before_action :group, except: [:index, :new, :create]
# Authorize # 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 :authorize_create_group!, only: [:new]
before_action :group_projects, only: [:projects, :activity, :issues, :merge_requests] before_action :group_projects, only: [:projects, :activity, :issues, :merge_requests]
...@@ -94,6 +94,19 @@ class GroupsController < Groups::ApplicationController ...@@ -94,6 +94,19 @@ class GroupsController < Groups::ApplicationController
redirect_to root_path, status: 302, alert: "Group '#{@group.name}' was scheduled for deletion." redirect_to root_path, status: 302, alert: "Group '#{@group.name}' was scheduled for deletion."
end 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 protected
def authorize_create_group! def authorize_create_group!
......
...@@ -88,6 +88,19 @@ module GroupsHelper ...@@ -88,6 +88,19 @@ module GroupsHelper
end end
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 private
def group_title_link(group, hidable: false, show_avatar: false, for_dropdown: false) def group_title_link(group, hidable: false, show_avatar: false, for_dropdown: false)
......
...@@ -14,7 +14,11 @@ module Storage ...@@ -14,7 +14,11 @@ module Storage
# Ensure old directory exists before moving it # Ensure old directory exists before moving it
gitlab_shell.add_namespace(repository_storage_path, full_path_was) 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) 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}" 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 # if we cannot move namespace directory we should rollback
......
...@@ -40,7 +40,6 @@ class Namespace < ActiveRecord::Base ...@@ -40,7 +40,6 @@ class Namespace < ActiveRecord::Base
namespace_path: true namespace_path: true
validate :nesting_level_allowed validate :nesting_level_allowed
validate :allowed_path_by_redirects
delegate :name, to: :owner, allow_nil: true, prefix: true delegate :name, to: :owner, allow_nil: true, prefix: true
...@@ -52,7 +51,7 @@ class Namespace < ActiveRecord::Base ...@@ -52,7 +51,7 @@ class Namespace < ActiveRecord::Base
# Legacy Storage specific hooks # 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 } before_destroy(prepend: true) { prepare_for_destroy }
after_destroy :rm_dir after_destroy :rm_dir
...@@ -222,9 +221,12 @@ class Namespace < ActiveRecord::Base ...@@ -222,9 +221,12 @@ class Namespace < ActiveRecord::Base
end end
def full_path_was def full_path_was
return path_was unless has_parent? if parent_id_was.nil?
path_was
"#{parent.full_path}/#{path_was}" else
previous_parent = Group.find_by(id: parent_id_was)
previous_parent.full_path + '/' + path_was
end
end end
# Exports belonging to projects with legacy storage are placed in a common # Exports belonging to projects with legacy storage are placed in a common
...@@ -241,6 +243,10 @@ class Namespace < ActiveRecord::Base ...@@ -241,6 +243,10 @@ class Namespace < ActiveRecord::Base
private private
def path_or_parent_changed?
path_changed? || parent_changed?
end
def refresh_access_of_projects_invited_groups def refresh_access_of_projects_invited_groups
Group Group
.joins(project_group_links: :project) .joins(project_group_links: :project)
...@@ -271,16 +277,6 @@ class Namespace < ActiveRecord::Base ...@@ -271,16 +277,6 @@ class Namespace < ActiveRecord::Base
.update_all(share_with_group_lock: true) .update_all(share_with_group_lock: true)
end 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 def write_projects_repository_config
all_projects.find_each do |project| all_projects.find_each do |project|
project.expires_full_path_cache # we need to clear cache to validate renames correctly project.expires_full_path_cache # we need to clear cache to validate renames correctly
......
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
...@@ -57,4 +57,20 @@ ...@@ -57,4 +57,20 @@
.form-actions .form-actions
= button_to 'Remove group', '#', class: "btn btn-remove js-confirm-danger", data: { "confirm-danger-message" => remove_group_message(@group) } = 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 = render 'shared/confirm_modal', phrase: @group.path
---
title: Add ability to transfer a group into another group
merge_request: 16302
author:
type: added
...@@ -14,6 +14,7 @@ constraints(GroupUrlConstrainer.new) do ...@@ -14,6 +14,7 @@ constraints(GroupUrlConstrainer.new) do
get :merge_requests, as: :merge_requests_group get :merge_requests, as: :merge_requests_group
get :projects, as: :projects_group get :projects, as: :projects_group
get :activity, as: :activity_group get :activity, as: :activity_group
put :transfer, as: :transfer_group
end end
get '/', action: :show, as: :group_canonical get '/', action: :show, as: :group_canonical
......
...@@ -168,6 +168,20 @@ Alternatively, you can [lock the sharing with group feature](#share-with-group-l ...@@ -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. 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. 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.
- 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 ## Group settings
Once you have created a group, you can manage its settings by navigating to Once you have created a group, you can manage its settings by navigating to
......
...@@ -128,8 +128,7 @@ and Git push/pull redirects. ...@@ -128,8 +128,7 @@ and Git push/pull redirects.
Depending on the situation, different things apply. Depending on the situation, different things apply.
When [transferring a project](settings/index.md#transferring-an-existing-project-into-another-namespace), When [renaming a user](../profile/index.md#changing-your-username) or
or [renaming a user](../profile/index.md#changing-your-username) or
[changing a group path](../group/index.md#changing-a-group-s-path): [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 - **The redirect to the new URL is permanent**, which means that the original
......
...@@ -496,4 +496,87 @@ describe GroupsController do ...@@ -496,4 +496,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." "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
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 end
...@@ -582,4 +582,20 @@ describe Group do ...@@ -582,4 +582,20 @@ describe Group do
end end
end 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 end
...@@ -567,36 +567,6 @@ describe Namespace do ...@@ -567,36 +567,6 @@ describe Namespace do
end end
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 describe '#remove_exports' do
let(:legacy_project) { create(:project, :with_export, namespace: namespace) } let(:legacy_project) { create(:project, :with_export, namespace: namespace) }
let(:hashed_project) { create(:project, :with_export, :hashed, namespace: namespace) } let(:hashed_project) { create(:project, :with_export, :hashed, namespace: namespace) }
...@@ -616,4 +586,44 @@ describe Namespace do ...@@ -616,4 +586,44 @@ describe Namespace do
expect(File.exist?(hashed_export)).to be_falsy expect(File.exist?(hashed_export)).to be_falsy
end 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 end
...@@ -2617,7 +2617,7 @@ describe User do ...@@ -2617,7 +2617,7 @@ describe User do
it 'should raise an ActiveRecord::RecordInvalid exception' do it 'should raise an ActiveRecord::RecordInvalid exception' do
user2 = build(:user, username: 'foo') 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
end end
......
This diff is collapsed.
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment