From 7a9d3a3c9043d391fc890befc6ed8117aa94c8ad Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Tue, 14 Feb 2017 14:13:35 +0000
Subject: [PATCH] Show merge errors in merge request widget
There were two problems here:
1. On the JS side, the reference to $widgetBody didn't refer to the
right DOM element any more. This might be because it was replaced by
the `getMergeStatus` method. Even if it wasn't, ensuring we have the
right element means that the content gets updated.
2. On the Ruby side, the `log_merge_error` method didn't update the
`merge_error` column of the merge request. Change that to update if
requested, and update in the most common cases by default.
Additionally, this would sometimes return an error hash, but it
doesn't look like this was ever used (the return value of
`MergeService#execute` appears to be unused everywhere).
---
.../javascripts/merge_request_widget.js.es6 | 2 +-
app/services/merge_requests/merge_service.rb | 18 +++++++-----
.../28124-mrs-don-t-show-all-merge-errors.yml | 4 +++
spec/features/merge_requests/widget_spec.rb | 15 ++++++++++
.../merge_requests/merge_service_spec.rb | 29 +++++++++++++------
5 files changed, 50 insertions(+), 18 deletions(-)
create mode 100644 changelogs/unreleased/28124-mrs-don-t-show-all-merge-errors.yml
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
--
GitLab