From cdb06474f6f487ebfd567a609f59ab9f71fc641e Mon Sep 17 00:00:00 2001 From: Mayra Cabrera Date: Fri, 19 Jan 2018 18:10:37 -0600 Subject: 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 --- app/controllers/groups_controller.rb | 19 +- app/helpers/groups_helper.rb | 4 - app/models/concerns/storage/legacy_namespace.rb | 1 + app/models/group.rb | 13 +- app/services/groups/convert_to_root_service.rb | 35 --- app/services/groups/convert_to_subgroup_service.rb | 66 ---- app/services/groups/transfer_error_messages.rb | 26 -- app/services/groups/transfer_service.rb | 85 ++++- app/views/groups/convert_to_root.js.haml | 2 - app/views/groups/edit.html.haml | 14 +- app/views/groups/transfer.js.haml | 2 - ...ability-to-transfer-groups-to-another-group.yml | 5 - ...ability-to-transfer-groups-to-another-group.yml | 5 + config/routes/group.rb | 1 - spec/controllers/groups_controller_spec.rb | 129 ++------ spec/models/group_spec.rb | 40 +++ .../groups/convert_to_root_service_spec.rb | 65 ---- .../groups/convert_to_subgroup_service_spec.rb | 213 ------------- spec/services/groups/transfer_service_spec.rb | 341 +++++++++++++++++++++ 19 files changed, 512 insertions(+), 554 deletions(-) delete mode 100644 app/services/groups/convert_to_root_service.rb delete mode 100644 app/services/groups/convert_to_subgroup_service.rb delete mode 100644 app/services/groups/transfer_error_messages.rb delete mode 100644 app/views/groups/convert_to_root.js.haml delete mode 100644 app/views/groups/transfer.js.haml delete mode 100644 changelogs/unreleased/30547-ability-to-transfer-groups-to-another-group.yml create mode 100644 changelogs/unreleased/31885-ability-to-transfer-groups-to-another-group.yml delete mode 100644 spec/services/groups/convert_to_root_service_spec.rb delete mode 100644 spec/services/groups/convert_to_subgroup_service_spec.rb create mode 100644 spec/services/groups/transfer_service_spec.rb 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 -- cgit v1.2.1