Commit 0c4b3472 authored by Douwe Maan's avatar Douwe Maan Committed by Douglas Barbosa Alexandre

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
parent 5b158b0b
......@@ -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
......
......@@ -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
......
......@@ -14,7 +14,13 @@ module Groups
group.assign_attributes(params)
begin
group.save
rescue Gitlab::UpdatePathError => e
group.errors.add(:base, e.message)
false
end
end
end
end
---
title: Fix 500 error renaming group
merge_request:
author:
module Gitlab
class UpdatePathError < StandardError; end
end
......@@ -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
......@@ -9,7 +9,7 @@ describe Groups::UpdateService, services: true do
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment