diff options
author | Rémy Coutable <remy@rymai.me> | 2017-02-16 10:18:59 +0000 |
---|---|---|
committer | Rémy Coutable <remy@rymai.me> | 2017-02-16 10:18:59 +0000 |
commit | 1b6a671eb5c9b4a440a07e56aad8e30b1dfe852f (patch) | |
tree | e398a5323e23a784de13dd9a0e264bec7a1dfeb6 | |
parent | 93ae3063ad0455a0bd6b9994f718d20357a31e28 (diff) | |
parent | 7a9d3a3c9043d391fc890befc6ed8117aa94c8ad (diff) | |
download | gitlab-ce-1b6a671eb5c9b4a440a07e56aad8e30b1dfe852f.tar.gz |
Merge branch '28124-mrs-don-t-show-all-merge-errors' into 'master'
Show merge errors in merge request widget
Closes #28124 and gitlab-ee#1652
See merge request !9229
5 files changed, 50 insertions, 18 deletions
diff --git a/app/assets/javascripts/merge_request_widget.js.es6 b/app/assets/javascripts/merge_request_widget.js.es6 index 69aed77c83d..4ab33420e59 100644 --- a/app/assets/javascripts/merge_request_widget.js.es6 +++ b/app/assets/javascripts/merge_request_widget.js.es6 @@ -110,7 +110,7 @@ require('./smart_interval'); urlSuffix = deleteSourceBranch ? '?deleted_source_branch=true' : ''; return window.location.href = window.location.pathname + urlSuffix; } else if (data.merge_error) { - return _this.$widgetBody.html("<h4>" + data.merge_error + "</h4>"); + return $('.mr-widget-body').html("<h4>" + data.merge_error + "</h4>"); } else { callback = function() { return merge_request_widget.mergeInProgress(deleteSourceBranch); diff --git a/app/services/merge_requests/merge_service.rb b/app/services/merge_requests/merge_service.rb index 5ca6fec962d..177b714b734 100644 --- a/app/services/merge_requests/merge_service.rb +++ b/app/services/merge_requests/merge_service.rb @@ -11,18 +11,20 @@ module MergeRequests def execute(merge_request) @merge_request = merge_request - return log_merge_error('Merge request is not mergeable', true) unless @merge_request.mergeable? + unless @merge_request.mergeable? + return log_merge_error('Merge request is not mergeable', save_message_on_model: true) + end @source = find_merge_source - return log_merge_error('No source for merge', true) unless @source + unless @source + log_merge_error('No source for merge', save_message_on_model: true) + end merge_request.in_locked_state do if commit after_merge success - else - log_merge_error('Can not merge changes', true) end end end @@ -43,11 +45,11 @@ module MergeRequests if commit_id merge_request.update(merge_commit_sha: commit_id) else - merge_request.update(merge_error: 'Conflicts detected during merge') + log_merge_error('Conflicts detected during merge', save_message_on_model: true) false end rescue GitHooksService::PreReceiveError => e - merge_request.update(merge_error: e.message) + log_merge_error(e.message, save_message_on_model: true) false rescue StandardError => e merge_request.update(merge_error: "Something went wrong during merge: #{e.message}") @@ -70,10 +72,10 @@ module MergeRequests @merge_request.force_remove_source_branch? ? @merge_request.author : current_user end - def log_merge_error(message, http_error = false) + def log_merge_error(message, save_message_on_model: false) Rails.logger.error("MergeService ERROR: #{merge_request_info} - #{message}") - error(message) if http_error + @merge_request.update(merge_error: message) if save_message_on_model end def merge_request_info diff --git a/changelogs/unreleased/28124-mrs-don-t-show-all-merge-errors.yml b/changelogs/unreleased/28124-mrs-don-t-show-all-merge-errors.yml new file mode 100644 index 00000000000..cd61c38e1bc --- /dev/null +++ b/changelogs/unreleased/28124-mrs-don-t-show-all-merge-errors.yml @@ -0,0 +1,4 @@ +--- +title: Show merge errors in merge request widget +merge_request: 9229 +author: diff --git a/spec/features/merge_requests/widget_spec.rb b/spec/features/merge_requests/widget_spec.rb index 957e913bf95..4ad944366c8 100644 --- a/spec/features/merge_requests/widget_spec.rb +++ b/spec/features/merge_requests/widget_spec.rb @@ -52,4 +52,19 @@ describe 'Merge request', :feature, :js do end end end + + context 'merge error' do + before do + allow_any_instance_of(Repository).to receive(:merge).and_return(false) + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + click_button 'Accept Merge Request' + wait_for_ajax + end + + it 'updates the MR widget' do + page.within('.mr-widget-body') do + expect(page).to have_content('Conflicts detected during merge') + end + end + end end diff --git a/spec/services/merge_requests/merge_service_spec.rb b/spec/services/merge_requests/merge_service_spec.rb index 5a89acc96a4..d96f819e66a 100644 --- a/spec/services/merge_requests/merge_service_spec.rb +++ b/spec/services/merge_requests/merge_service_spec.rb @@ -149,35 +149,46 @@ describe MergeRequests::MergeService, services: true do context "error handling" do let(:service) { MergeRequests::MergeService.new(project, user, commit_message: 'Awesome message') } - it 'saves error if there is an exception' do - allow(service).to receive(:repository).and_raise("error message") + before do + allow(Rails.logger).to receive(:error) + end + it 'logs and saves error if there is an exception' do + error_message = 'error message' + + allow(service).to receive(:repository).and_raise("error message") allow(service).to receive(:execute_hooks) service.execute(merge_request) - expect(merge_request.merge_error).to eq("Something went wrong during merge: error message") + expect(merge_request.merge_error).to include(error_message) + expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) end - it 'saves error if there is an PreReceiveError exception' do - allow(service).to receive(:repository).and_raise(GitHooksService::PreReceiveError, "error") + it 'logs and saves error if there is an PreReceiveError exception' do + error_message = 'error message' + allow(service).to receive(:repository).and_raise(GitHooksService::PreReceiveError, error_message) allow(service).to receive(:execute_hooks) service.execute(merge_request) - expect(merge_request.merge_error).to eq("error") + expect(merge_request.merge_error).to include(error_message) + expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) end - it 'aborts if there is a merge conflict' do + it 'logs and saves error if there is a merge conflict' do + error_message = 'Conflicts detected during merge' + allow_any_instance_of(Repository).to receive(:merge).and_return(false) allow(service).to receive(:execute_hooks) service.execute(merge_request) - expect(merge_request.open?).to be_truthy + expect(merge_request).to be_open expect(merge_request.merge_commit_sha).to be_nil - expect(merge_request.merge_error).to eq("Conflicts detected during merge") + expect(merge_request.merge_error).to include(error_message) + expect(Rails.logger).to have_received(:error).with(a_string_matching(error_message)) end end end |