diff options
-rw-r--r-- | app/controllers/groups_controller.rb | 4 | ||||
-rw-r--r-- | app/helpers/groups_helper.rb | 4 | ||||
-rw-r--r-- | app/services/groups/transfer_service.rb | 36 | ||||
-rw-r--r-- | app/views/groups/edit.html.haml | 3 | ||||
-rw-r--r-- | doc/user/group/index.md | 14 | ||||
-rw-r--r-- | doc/user/project/index.md | 3 | ||||
-rw-r--r-- | spec/controllers/groups_controller_spec.rb | 16 | ||||
-rw-r--r-- | spec/services/groups/transfer_service_spec.rb | 54 |
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 |