From b82fdf6257255b720526ccef716759892e88de09 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Tue, 20 Dec 2016 17:52:27 +0100 Subject: [PATCH 1/4] Fix error 500 renaming group. Also added specs and changelog. --- app/controllers/groups_controller.rb | 5 +- app/models/namespace.rb | 4 +- app/services/groups/update_service.rb | 8 ++- .../fix-group-path-rename-error.yml | 4 ++ lib/gitlab/update_path_error.rb | 3 ++ spec/controllers/groups_controller_spec.rb | 21 ++++++++ spec/services/groups/update_service_spec.rb | 49 ++++++++++++++++--- 7 files changed, 83 insertions(+), 11 deletions(-) create mode 100644 changelogs/unreleased/fix-group-path-rename-error.yml create mode 100644 lib/gitlab/update_path_error.rb diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index b83c3a872cf..5c7709ea013 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 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 fff2273f402..4e878ec556a 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..4bb37bc52ee 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 9c2331144a0..8ac5736cbb3 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 From fd5062a848d63b89248c384e0171c6f1af833a49 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 21 Dec 2016 11:50:31 +0100 Subject: [PATCH 2/4] update controller action to render error in form --- app/controllers/groups_controller.rb | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 5c7709ea013..b83c3a872cf 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -82,10 +82,7 @@ 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 - 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 + render action: "edit" end end From 4c3ec579e5660daab23d48bd4b3e21050735f726 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 21 Dec 2016 13:29:27 +0100 Subject: [PATCH 3/4] added more specs --- app/controllers/groups_controller.rb | 2 ++ spec/controllers/groups_controller_spec.rb | 4 ++-- spec/services/groups/update_service_spec.rb | 6 ++++-- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index b83c3a872cf..1e499199f82 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.reload + render action: "edit" end end diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index 4bb37bc52ee..98dfb3e5216 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/spec/controllers/groups_controller_spec.rb @@ -122,8 +122,8 @@ describe GroupsController 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] + 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 8ac5736cbb3..531180e48a1 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -59,8 +59,6 @@ describe Groups::UpdateService, services: true do end it 'returns true' do - puts internal_group.errors.full_messages - expect(service.execute).to eq(true) end @@ -82,6 +80,10 @@ describe Groups::UpdateService, services: true do 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 From b902784dbff64d1f746fc38249a814debb8ed325 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Wed, 21 Dec 2016 14:21:18 +0100 Subject: [PATCH 4/4] fix reset path --- app/controllers/groups_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 1e499199f82..efe9c001bcf 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -82,7 +82,7 @@ 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.reload + @group.reset_path! render action: "edit" end