diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index b83c3a872cf07bb9caaf583ae2fec7f83640fd94..5c7709ea013811e6f14d317d6f54a644cde69860 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -82,7 +82,10 @@ 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
- render action: "edit"
+ error = group.errors.full_messages.first
+ alert_message = "Group '#{@group.name}' cannot be updated: " + error
+
+ redirect_to edit_group_path(@group.reload), alert: alert_message
end
end
diff --git a/app/models/namespace.rb b/app/models/namespace.rb
index fd42f2328d88cf590abc9169cba951dc5994f056..b52f08c70813f94f5cdd46694429b5125e3fefac 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 fff2273f402df665a02b3649bfb2fe9c46774e67..4e878ec556a2e77f2ac571e6c45efa938f0e91aa 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 0000000000000000000000000000000000000000..e3d97ae39876e4ac1dee856ef700036ebaff6685
--- /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 0000000000000000000000000000000000000000..ce14cc887d0f4edfd674c9df14b2feeb2274d1e4
--- /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 a763e2c5ba852a31fc3c114fbdf81aaadb46bc86..4bb37bc52ee5d4c154bef7e64ee5414227e47330 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(response).to have_http_status(302)
+ expect(controller).to set_flash[:alert]
+ end
+ end
end
diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb
index 9c2331144a04d09d674d6b7f6c19ec7d53f4fa75..8ac5736cbb3034a7f53b29040098009ff409edd9 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,39 @@ 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
+ puts internal_group.errors.full_messages
+
+ 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
+ end
+ end
end