Merge branch 'fix/group-path-rename-error' into 'master'

Fix error 500 renaming group. Also added specs and changelog.

Closes #17922 and #23223

See merge request !8201

7 files changed, 83 insertions, 10 deletions

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index b83c3a872cf..efe9c001bcf 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -82,6 +82,8 @@ class GroupsController < Groups::ApplicationController
     if Groups::UpdateService.new(@group, current_user, group_params).execute
       redirect_to edit_group_path(@group), notice: "Group '#{@group.name}' was successfully updated."
     else
+      @group.reset_path!
+
       render action: "edit"
     end
   end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index fd42f2328d8..b52f08c7081 100644
--- a/app/models/namespace.rb
+++ b/app/models/namespace.rb
@@ -98,7 +98,7 @@ class Namespace < ActiveRecord::Base
 
   def move_dir
     if any_project_has_container_registry_tags?
-      raise Exception.new('Namespace cannot be moved, because at least one project has tags in container registry')
+      raise Gitlab::UpdatePathError.new('Namespace cannot be moved, because at least one project has tags in container registry')
     end
 
     # Move the namespace directory in all storages paths used by member projects
@@ -111,7 +111,7 @@ class Namespace < ActiveRecord::Base
 
         # if we cannot move namespace directory we should rollback
         # db changes in order to prevent out of sync between db and fs
-        raise Exception.new('namespace directory cannot be moved')
+        raise Gitlab::UpdatePathError.new('namespace directory cannot be moved')
       end
     end
 
diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb
index 99ad12b1003..f228256533a 100644
--- a/app/services/groups/update_service.rb
+++ b/app/services/groups/update_service.rb
@@ -14,7 +14,13 @@ module Groups
 
       group.assign_attributes(params)
 
-      group.save
+      begin
+        group.save
+      rescue Gitlab::UpdatePathError => e
+        group.errors.add(:base, e.message)
+
+        false
+      end
     end
   end
 end
diff --git a/changelogs/unreleased/fix-group-path-rename-error.yml b/changelogs/unreleased/fix-group-path-rename-error.yml
new file mode 100644
index 00000000000..e3d97ae3987
--- /dev/null
+++ b/changelogs/unreleased/fix-group-path-rename-error.yml
@@ -0,0 +1,4 @@
+---
+title: Fix 500 error renaming group
+merge_request: 
+author: 
diff --git a/lib/gitlab/update_path_error.rb b/lib/gitlab/update_path_error.rb
new file mode 100644
index 00000000000..ce14cc887d0
--- /dev/null
+++ b/lib/gitlab/update_path_error.rb
@@ -0,0 +1,3 @@
+module Gitlab
+  class UpdatePathError < StandardError; end
+end
diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb
index a763e2c5ba8..98dfb3e5216 100644
--- a/spec/controllers/groups_controller_spec.rb
+++ b/spec/controllers/groups_controller_spec.rb
@@ -105,4 +105,25 @@ describe GroupsController do
       end
     end
   end
+
+  describe 'PUT update' do
+    before do
+      sign_in(user)
+    end
+
+    it 'updates the path succesfully' do
+      post :update, id: group.to_param, group: { path: 'new_path' }
+
+      expect(response).to have_http_status(302)
+      expect(controller).to set_flash[:notice]
+    end
+
+    it 'does not update the path on error' do
+      allow_any_instance_of(Group).to receive(:move_dir).and_raise(Gitlab::UpdatePathError)
+      post :update, id: group.to_param, group: { path: 'new_path' }
+
+      expect(assigns(:group).errors).not_to be_empty
+      expect(assigns(:group).path).not_to eq('new_path')
+    end
+  end
 end
diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb
index 9c2331144a0..531180e48a1 100644
--- a/spec/services/groups/update_service_spec.rb
+++ b/spec/services/groups/update_service_spec.rb
@@ -1,15 +1,15 @@
 require 'spec_helper'
 
 describe Groups::UpdateService, services: true do
-  let!(:user)             { create(:user) }
-  let!(:private_group)    { create(:group, :private) }
-  let!(:internal_group)   { create(:group, :internal) }
-  let!(:public_group)     { create(:group, :public) }
+  let!(:user) { create(:user) }
+  let!(:private_group) { create(:group, :private) }
+  let!(:internal_group) { create(:group, :internal) }
+  let!(:public_group) { create(:group, :public) }
 
   describe "#execute" do
     context "project visibility_level validation" do
       context "public group with public projects" do
-        let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL ) }
+        let!(:service) { described_class.new(public_group, user, visibility_level: Gitlab::VisibilityLevel::INTERNAL) }
 
         before do
           public_group.add_user(user, Gitlab::Access::MASTER)
@@ -23,7 +23,7 @@ describe Groups::UpdateService, services: true do
       end
 
       context "internal group with internal project" do
-        let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE ) }
+        let!(:service) { described_class.new(internal_group, user, visibility_level: Gitlab::VisibilityLevel::PRIVATE) }
 
         before do
           internal_group.add_user(user, Gitlab::Access::MASTER)
@@ -39,7 +39,7 @@ describe Groups::UpdateService, services: true do
   end
 
   context "unauthorized visibility_level validation" do
-    let!(:service) { described_class.new(internal_group, user, visibility_level: 99 ) }
+    let!(:service) { described_class.new(internal_group, user, visibility_level: 99) }
     before do
       internal_group.add_user(user, Gitlab::Access::MASTER)
     end
@@ -49,4 +49,41 @@ describe Groups::UpdateService, services: true do
       expect(internal_group.errors.count).to eq(1)
     end
   end
+
+  context 'rename group' do
+    let!(:service) { described_class.new(internal_group, user, path: 'new_path') }
+
+    before do
+      internal_group.add_user(user, Gitlab::Access::MASTER)
+      create(:project, :internal, group: internal_group)
+    end
+
+    it 'returns true' do
+      expect(service.execute).to eq(true)
+    end
+
+    context 'error moving group' do
+      before do
+        allow(internal_group).to receive(:move_dir).and_raise(Gitlab::UpdatePathError)
+      end
+
+      it 'does not raise an error' do
+        expect { service.execute }.not_to raise_error
+      end
+
+      it 'returns false' do
+        expect(service.execute).to eq(false)
+      end
+
+      it 'has the right error' do
+        service.execute
+
+        expect(internal_group.errors.full_messages.first).to eq('Gitlab::UpdatePathError')
+      end
+
+      it "hasn't changed the path" do
+        expect { service.execute}.not_to change { internal_group.reload.path}
+      end
+    end
+  end
 end