From 5f232b5687b447e7eac40f58c56628da22580de6 Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Sun, 22 Feb 2015 16:01:49 -0700 Subject: [PATCH] Improve error messages when file editing fails Give more specific errors in API responses and web UI flash messages when a file update fails. --- CHANGELOG | 1 + app/services/base_service.rb | 7 +++-- app/services/files/update_service.rb | 14 ++++++---- lib/api/files.rb | 3 +- .../satellite/files/edit_file_action.rb | 28 +++++++++++++++---- lib/gitlab/satellite/satellite.rb | 4 +++ spec/requests/api/files_spec.rb | 28 ++++++++++++++++--- 7 files changed, 66 insertions(+), 19 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 6702ba2ba46..6571f0d1de0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,7 @@ v 7.9.0 (unreleased) - Move labels/milestones tabs to sidebar - Upgrade Rails gem to version 4.1.9. + - Improve error messages for file edit failures - Improve UI for commits, issues and merge request lists - Fix commit comments on first line of diff not rendering in Merge Request Discussion view. diff --git a/app/services/base_service.rb b/app/services/base_service.rb index bb51795df7c..52ab29f1492 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -37,11 +37,14 @@ class BaseService private - def error(message) - { + def error(message, http_status = nil) + result = { message: message, status: :error } + + result[:http_status] = http_status if http_status + result end def success diff --git a/app/services/files/update_service.rb b/app/services/files/update_service.rb index b4986e1c5c6..bcf0e7f3cee 100644 --- a/app/services/files/update_service.rb +++ b/app/services/files/update_service.rb @@ -20,17 +20,19 @@ module Files end edit_file_action = Gitlab::Satellite::EditFileAction.new(current_user, project, ref, path) - created_successfully = edit_file_action.commit!( + edit_file_action.commit!( params[:content], params[:commit_message], params[:encoding] ) - if created_successfully - success - else - error("Your changes could not be committed. Maybe the file was changed by another process or there was nothing to commit?") - end + success + rescue Gitlab::Satellite::CheckoutFailed => ex + error("Your changes could not be committed because ref '#{ref}' could not be checked out", 400) + rescue Gitlab::Satellite::CommitFailed => ex + error("Your changes could not be committed. Maybe there was nothing to commit?", 409) + rescue Gitlab::Satellite::PushFailed => ex + error("Your changes could not be committed. Maybe the file was changed by another process?", 409) end end end diff --git a/lib/api/files.rb b/lib/api/files.rb index e6e71bac367..3176ef0e256 100644 --- a/lib/api/files.rb +++ b/lib/api/files.rb @@ -117,7 +117,8 @@ module API branch_name: branch_name } else - render_api_error!(result[:message], 400) + http_status = result[:http_status] || 400 + render_api_error!(result[:message], http_status) end end diff --git a/lib/gitlab/satellite/files/edit_file_action.rb b/lib/gitlab/satellite/files/edit_file_action.rb index 2834b722b27..82d71ab9906 100644 --- a/lib/gitlab/satellite/files/edit_file_action.rb +++ b/lib/gitlab/satellite/files/edit_file_action.rb @@ -15,7 +15,11 @@ module Gitlab prepare_satellite!(repo) # create target branch in satellite at the corresponding commit from bare repo - repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}") + begin + repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}") + rescue Grit::Git::CommandFailed => ex + log_and_raise(CheckoutFailed, ex.message) + end # update the file in the satellite's working dir file_path_in_satellite = File.join(repo.working_dir, file_path) @@ -31,19 +35,31 @@ module Gitlab # commit the changes # will raise CommandFailed when commit fails - repo.git.commit(raise: true, timeout: true, a: true, m: commit_message) + begin + repo.git.commit(raise: true, timeout: true, a: true, m: commit_message) + rescue Grit::Git::CommandFailed => ex + log_and_raise(CommitFailed, ex.message) + end # push commit back to bare repo # will raise CommandFailed when push fails - repo.git.push({ raise: true, timeout: true }, :origin, ref) + begin + repo.git.push({ raise: true, timeout: true }, :origin, ref) + rescue Grit::Git::CommandFailed => ex + log_and_raise(PushFailed, ex.message) + end # everything worked true end - rescue Grit::Git::CommandFailed => ex - Gitlab::GitLogger.error(ex.message) - false + end + + private + + def log_and_raise(errorClass, message) + Gitlab::GitLogger.error(message) + raise(errorClass, message) end end end diff --git a/lib/gitlab/satellite/satellite.rb b/lib/gitlab/satellite/satellite.rb index 62d1bb364d3..70125d539da 100644 --- a/lib/gitlab/satellite/satellite.rb +++ b/lib/gitlab/satellite/satellite.rb @@ -1,5 +1,9 @@ module Gitlab module Satellite + class CheckoutFailed < StandardError; end + class CommitFailed < StandardError; end + class PushFailed < StandardError; end + class Satellite include Gitlab::Popen diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index cfac7d289ec..bab8888a631 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -98,13 +98,33 @@ describe API::API, api: true do expect(response.status).to eq(400) end - it "should return a 400 if satellite fails to create file" do - Gitlab::Satellite::EditFileAction.any_instance.stub( - commit!: false, - ) + it 'should return a 400 if the checkout fails' do + Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) + .and_raise(Gitlab::Satellite::CheckoutFailed) put api("/projects/#{project.id}/repository/files", user), valid_params expect(response.status).to eq(400) + + ref = valid_params[:branch_name] + expect(response.body).to match("ref '#{ref}' could not be checked out") + end + + it 'should return a 409 if the file was not modified' do + Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) + .and_raise(Gitlab::Satellite::CommitFailed) + + put api("/projects/#{project.id}/repository/files", user), valid_params + expect(response.status).to eq(409) + expect(response.body).to match("Maybe there was nothing to commit?") + end + + it 'should return a 409 if the push fails' do + Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!) + .and_raise(Gitlab::Satellite::PushFailed) + + put api("/projects/#{project.id}/repository/files", user), valid_params + expect(response.status).to eq(409) + expect(response.body).to match("Maybe the file was changed by another process?") end end