Search for duplicates on Namespace instead of Group31885-ability-to-transfer-groups-to-another-group

- Also changes controller response on transfer action
- Readjust css code since several features were failing

7 files changed, 41 insertions, 24 deletions

diff --git a/app/assets/stylesheets/pages/groups.scss b/app/assets/stylesheets/pages/groups.scss
index 7f03bce8d74..6ee8b33bd39 100644
--- a/app/assets/stylesheets/pages/groups.scss
+++ b/app/assets/stylesheets/pages/groups.scss
@@ -225,17 +225,15 @@
   font-weight: $gl-font-weight-normal;
 }
 
-.form-group {
-  .js-groups-dropdown {
-    width: 100%;
-  }
+.js-groups-dropdown {
+  width: 100%;
+}
+
+.dropdown-group-transfer {
+  bottom: 100%;
+  top: initial;
 
   .dropdown-content {
     overflow-y: unset;
   }
-
-  .dropdown-menu {
-    bottom: 100%;
-    top: initial;
-  }
 }
diff --git a/app/controllers/concerns/routable_actions.rb b/app/controllers/concerns/routable_actions.rb
index d5f817a10e0..f745deb083c 100644
--- a/app/controllers/concerns/routable_actions.rb
+++ b/app/controllers/concerns/routable_actions.rb
@@ -25,7 +25,7 @@ module RoutableActions
   end
 
   def ensure_canonical_path(routable, requested_full_path)
-    return unless request.get? && !flash[:alert].present?
+    return unless request.get?
 
     canonical_path = routable.full_path
     if canonical_path != requested_full_path
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 8112381d3db..79a6387fb61 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -100,11 +100,11 @@ class GroupsController < Groups::ApplicationController
 
     if service.execute(parent_group)
       flash[:notice] = "Group '#{@group.name}' was successfully transferred."
+      redirect_to group_path(@group.to_param)
     else
       flash[:alert] = service.error
+      render :edit
     end
-
-    redirect_to @group
   end
 
   protected
diff --git a/app/services/groups/transfer_service.rb b/app/services/groups/transfer_service.rb
index b778c74031c..92eb93428b3 100644
--- a/app/services/groups/transfer_service.rb
+++ b/app/services/groups/transfer_service.rb
@@ -2,7 +2,7 @@ module Groups
   class TransferService < Groups::BaseService
     ERROR_MESSAGES = {
       database_not_supported: 'Database is not supported.',
-      group_with_same_path: 'The parent group has a group with the same path.',
+      namespace_with_same_path: 'The parent group has a namespace 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."
@@ -23,6 +23,7 @@ module Groups
       proceed_to_transfer
 
     rescue TransferError, ActiveRecord::RecordInvalid, Gitlab::UpdatePathError => e
+      @group.errors.clear
       @error = "Transfer failed: " + e.message
       false
     end
@@ -40,7 +41,7 @@ module Groups
       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(:group_with_same_path) if group_with_same_path?
+      raise_transfer_error(:namespace_with_same_path) if namespace_with_same_path?
     end
 
     def group_is_already_root?
@@ -59,12 +60,10 @@ module Groups
       else
         can?(current_user, :create_group)
       end
-
-      true
     end
 
-    def group_with_same_path?
-      Group.exists?(path: @group.path, parent: @new_parent_group)
+    def namespace_with_same_path?
+      Namespace.exists?(path: @group.path, parent: @new_parent_group)
     end
 
     def update_group_attributes
@@ -79,6 +78,7 @@ module Groups
 
     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)
diff --git a/app/views/groups/edit.html.haml b/app/views/groups/edit.html.haml
index c214c672e16..86cd0759a2c 100644
--- a/app/views/groups/edit.html.haml
+++ b/app/views/groups/edit.html.haml
@@ -63,7 +63,7 @@
     .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', placeholder: "Search groups", data: { data: parent_group_options(@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
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index 7f4c6deb66c..8688fb33f0d 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -559,7 +559,7 @@ describe GroupsController do
       end
 
       it 'should redirect to the current path' do
-        expect(response).to redirect_to("/#{group.path}")
+        expect(response).to render_template(:edit)
       end
     end
 
diff --git a/spec/services/groups/transfer_service_spec.rb b/spec/services/groups/transfer_service_spec.rb
index b32953982c0..bd207777783 100644
--- a/spec/services/groups/transfer_service_spec.rb
+++ b/spec/services/groups/transfer_service_spec.rb
@@ -70,10 +70,10 @@ describe Groups::TransferService, :postgresql do
       end
 
       context 'when there is a group with the same path' do
-        let!(:group) { create(:group, :public, :nested, path: "not-unique") }
+        let!(:group) { create(:group, :public, :nested, path: 'not-unique') }
 
         before do
-          create(:group, path: "not-unique")
+          create(:group, path: 'not-unique')
         end
 
         it 'should return false' do
@@ -82,7 +82,7 @@ describe Groups::TransferService, :postgresql do
 
         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.')
+          expect(transfer_service.error).to eq('Transfer failed: The parent group has a namespace with the same path.')
         end
       end
 
@@ -158,7 +158,26 @@ describe Groups::TransferService, :postgresql do
 
         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.')
+          expect(transfer_service.error).to eq('Transfer failed: The parent group has a namespace with the same path.')
+        end
+      end
+
+      context 'when the parent group has a project with the same path' do
+        let!(:group) { create(:group, :public, :nested, path: 'foo') }
+
+        before do
+          create(:group_member, :owner, group: new_parent_group, user: user)
+          create(:project, path: 'foo', namespace: new_parent_group)
+          group.update_attribute(:path, 'foo')
+        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: Validation failed: Route path has already been taken, Route is invalid')
         end
       end