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
This commit is contained in:
commit
6e8ba3b79f
|
@ -82,6 +82,8 @@ class GroupsController < Groups::ApplicationController
|
||||||
if Groups::UpdateService.new(@group, current_user, group_params).execute
|
if Groups::UpdateService.new(@group, current_user, group_params).execute
|
||||||
redirect_to edit_group_path(@group), notice: "Group '#{@group.name}' was successfully updated."
|
redirect_to edit_group_path(@group), notice: "Group '#{@group.name}' was successfully updated."
|
||||||
else
|
else
|
||||||
|
@group.reset_path!
|
||||||
|
|
||||||
render action: "edit"
|
render action: "edit"
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -98,7 +98,7 @@ class Namespace < ActiveRecord::Base
|
||||||
|
|
||||||
def move_dir
|
def move_dir
|
||||||
if any_project_has_container_registry_tags?
|
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
|
end
|
||||||
|
|
||||||
# Move the namespace directory in all storages paths used by member projects
|
# 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
|
# if we cannot move namespace directory we should rollback
|
||||||
# db changes in order to prevent out of sync between db and fs
|
# 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
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -14,7 +14,13 @@ module Groups
|
||||||
|
|
||||||
group.assign_attributes(params)
|
group.assign_attributes(params)
|
||||||
|
|
||||||
group.save
|
begin
|
||||||
|
group.save
|
||||||
|
rescue Gitlab::UpdatePathError => e
|
||||||
|
group.errors.add(:base, e.message)
|
||||||
|
|
||||||
|
false
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -0,0 +1,4 @@
|
||||||
|
---
|
||||||
|
title: Fix 500 error renaming group
|
||||||
|
merge_request:
|
||||||
|
author:
|
|
@ -0,0 +1,3 @@
|
||||||
|
module Gitlab
|
||||||
|
class UpdatePathError < StandardError; end
|
||||||
|
end
|
|
@ -105,4 +105,25 @@ describe GroupsController do
|
||||||
end
|
end
|
||||||
end
|
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
|
end
|
||||||
|
|
|
@ -1,15 +1,15 @@
|
||||||
require 'spec_helper'
|
require 'spec_helper'
|
||||||
|
|
||||||
describe Groups::UpdateService, services: true do
|
describe Groups::UpdateService, services: true do
|
||||||
let!(:user) { create(:user) }
|
let!(:user) { create(:user) }
|
||||||
let!(:private_group) { create(:group, :private) }
|
let!(:private_group) { create(:group, :private) }
|
||||||
let!(:internal_group) { create(:group, :internal) }
|
let!(:internal_group) { create(:group, :internal) }
|
||||||
let!(:public_group) { create(:group, :public) }
|
let!(:public_group) { create(:group, :public) }
|
||||||
|
|
||||||
describe "#execute" do
|
describe "#execute" do
|
||||||
context "project visibility_level validation" do
|
context "project visibility_level validation" do
|
||||||
context "public group with public projects" 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
|
before do
|
||||||
public_group.add_user(user, Gitlab::Access::MASTER)
|
public_group.add_user(user, Gitlab::Access::MASTER)
|
||||||
|
@ -23,7 +23,7 @@ describe Groups::UpdateService, services: true do
|
||||||
end
|
end
|
||||||
|
|
||||||
context "internal group with internal project" do
|
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
|
before do
|
||||||
internal_group.add_user(user, Gitlab::Access::MASTER)
|
internal_group.add_user(user, Gitlab::Access::MASTER)
|
||||||
|
@ -39,7 +39,7 @@ describe Groups::UpdateService, services: true do
|
||||||
end
|
end
|
||||||
|
|
||||||
context "unauthorized visibility_level validation" do
|
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
|
before do
|
||||||
internal_group.add_user(user, Gitlab::Access::MASTER)
|
internal_group.add_user(user, Gitlab::Access::MASTER)
|
||||||
end
|
end
|
||||||
|
@ -49,4 +49,41 @@ describe Groups::UpdateService, services: true do
|
||||||
expect(internal_group.errors.count).to eq(1)
|
expect(internal_group.errors.count).to eq(1)
|
||||||
end
|
end
|
||||||
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
|
end
|
||||||
|
|
Loading…
Reference in New Issue