Merge branch '1774-rebase-onto-master-silently-fails' into 'master'

Show errors when rebase onto target branch fails in the UI

Closes #1774

See merge request gitlab-org/gitlab-ee!2780

app/services/merge_requests/rebase_service.rb

@@ -12,7 +12,7 @@ module MergeRequests
 
     def rebase
       if merge_request.rebase_in_progress?
-        log_error('Rebase task canceled: Another rebase is already in progress')
+        log_error('Rebase task canceled: Another rebase is already in progress', save_message_on_model: true)
         return false
       end
 
@@ -52,7 +52,7 @@ module MergeRequests
       false
     rescue => e
       log_error('Failed to rebase branch:')
-      log_error(e)
+      log_error(e.message, save_message_on_model: true)
       false
     ensure
       clean_dir

app/services/merge_requests/working_copy_base_service.rb

@@ -24,7 +24,7 @@ module MergeRequests
           log_error("`#{command.join(' ')}` failed:")
         end
 
-        log_error(output)
+        log_error(output, save_message_on_model: true)
 
         raise GitCommandError
       end
@@ -40,8 +40,10 @@ module MergeRequests
       @target_project ||= merge_request.target_project
     end
 
-    def log_error(message)
+    def log_error(message, save_message_on_model: false)
       Gitlab::GitLogger.error("#{self.class.name} error (#{merge_request.to_reference(full: true)}): #{message}")
+
+      merge_request.update(merge_error: message) if save_message_on_model
     end
 
     def clean_dir

changelogs/unreleased-ee/1774-rebase-onto-master-silently-fails.yml

+---
+title: Show errors when rebase onto target branch fails in the UI
+merge_request:
+author:
+type: fixed

ee/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.js

-/* global Flash */
-
-import simplePoll from '~/lib/utils/simple_poll';
-import eventHub from '~/vue_merge_request_widget/event_hub';
-import statusIcon from '~/vue_merge_request_widget/components/mr_widget_status_icon';
-
-export default {
-  props: {
-    mr: {
-      type: Object,
-      required: true,
-    },
-    service: {
-      type: Object,
-      required: true,
-    },
-  },
-  components: {
-    statusIcon,
-  },
-  data() {
-    return {
-      isMakingRequest: false,
-    };
-  },
-  computed: {
-    status() {
-      if (this.mr.rebaseInProgress || this.isMakingRequest) {
-        return 'loading';
-      }
-      if (!this.mr.canPushToSourceBranch && !this.mr.rebaseInProgress) {
-        return 'failed';
-      }
-      return 'success';
-    },
-    showDisabledButton() {
-      return ['failed', 'loading'].includes(this.status);
-    },
-  },
-  methods: {
-    rebase() {
-      this.isMakingRequest = true;
-      this.service.rebase().then(() => {
-        simplePoll((continuePolling, stopPolling) => {
-          this.service.poll()
-            .then(res => res.json())
-            .then((res) => {
-              if (res.rebase_in_progress) {
-                continuePolling();
-              } else {
-                this.isMakingRequest = false;
-                eventHub.$emit('MRWidgetUpdateRequested');
-                stopPolling();
-              }
-            })
-            .catch(() => {
-              this.isMakingRequest = false;
-              new Flash('Something went wrong. Please try again.'); // eslint-disable-line
-              stopPolling();
-            });
-        });
-      }).catch(() => {
-        this.isMakingRequest = false;
-        new Flash('Something went wrong. Please try again.'); // eslint-disable-line
-      });
-    },
-  },
-  template: `
-    <div class="mr-widget-body media">
-      <status-icon :status="status" :showDisabledButton="showDisabledButton" />
-      <div class="rebase-state-find-class-convention media media-body space-children">
-        <template v-if="mr.rebaseInProgress || isMakingRequest">
-          <span class="bold">
-            Rebase in progress
-          </span>
-        </template>
-        <template v-if="!mr.rebaseInProgress && !mr.canPushToSourceBranch">
-          <span class="bold">
-            Fast-forward merge is not possible.
-            Rebase the source branch onto
-            <span class="label-branch">{{mr.targetBranch}}</span>
-            to allow this merge request to be merged.
-          </span>
-        </template>
-        <template v-if="!mr.rebaseInProgress && mr.canPushToSourceBranch && !isMakingRequest">
-          <div class="accept-merge-holder clearfix js-toggle-container accept-action media space-children">
-            <button
-              class="btn btn-small btn-reopen btn-success"
-              :disabled="isMakingRequest"
-              @click="rebase">
-              <i
-                v-if="isMakingRequest"
-                class="fa fa-spinner fa-spin"
-                aria-hidden="true" />
-              Rebase
-            </button>
-            <span class="bold">
-              Fast-forward merge is not possible.
-              Rebase the source branch onto the target branch or merge target
-              branch into source branch to allow this merge request to be merged.
-            </span>
-          </div>
-        </template>
-      </div>
-    </div>
-  `,
-};

ee/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue

+<script>
+  /* global Flash */
+
+  import simplePoll from '~/lib/utils/simple_poll';
+  import eventHub from '~/vue_merge_request_widget/event_hub';
+  import statusIcon from '~/vue_merge_request_widget/components/mr_widget_status_icon';
+  import loadingIcon from '~/vue_shared/components/loading_icon.vue';
+  import '~/flash';
+
+  export default {
+    props: {
+      mr: {
+        type: Object,
+        required: true,
+      },
+      service: {
+        type: Object,
+        required: true,
+      },
+    },
+    components: {
+      statusIcon,
+      loadingIcon,
+    },
+    data() {
+      return {
+        isMakingRequest: false,
+        rebasingError: '',
+      };
+    },
+    computed: {
+      status() {
+        if (this.mr.rebaseInProgress || this.isMakingRequest) {
+          return 'loading';
+        }
+        if (!this.mr.canPushToSourceBranch && !this.mr.rebaseInProgress) {
+          return 'failed';
+        }
+        return 'success';
+      },
+      showDisabledButton() {
+        return ['failed', 'loading'].includes(this.status);
+      },
+      hasRebasingError() {
+        return this.rebasingError.length;
+      },
+    },
+    methods: {
+      rebase() {
+        this.isMakingRequest = true;
+        this.rebasingError = '';
+
+        this.service.rebase()
+          .then(() => {
+            simplePoll((continuePolling, stopPolling) => {
+              this.service.poll()
+                .then(res => res.json())
+                .then((res) => {
+                  if (res.rebase_in_progress) {
+                    continuePolling();
+                  } else {
+                    this.isMakingRequest = false;
+
+                    if (res.merge_error.length) {
+                      this.rebasingError = res.merge_error;
+                      Flash('Something went wrong. Please try again.');
+                    }
+
+                    eventHub.$emit('MRWidgetUpdateRequested');
+                    stopPolling();
+                  }
+                })
+                .catch(() => {
+                  this.isMakingRequest = false;
+                  Flash('Something went wrong. Please try again.');
+                  stopPolling();
+                });
+            });
+          })
+          .catch((error) => {
+            this.rebasingError = error.merge_error;
+            this.isMakingRequest = false;
+            Flash('Something went wrong. Please try again.');
+          });
+      },
+    },
+  };
+</script>
+<template>
+  <div class="mr-widget-body media">
+    <status-icon
+      :status="status"
+      :show-disabled-button="showDisabledButton"
+      />
+
+    <div class="rebase-state-find-class-convention media media-body space-children">
+      <template v-if="mr.rebaseInProgress || isMakingRequest">
+        <span class="bold">
+          Rebase in progress
+        </span>
+      </template>
+      <template v-if="!mr.rebaseInProgress && !mr.canPushToSourceBranch">
+        <span class="bold">
+          Fast-forward merge is not possible.
+          Rebase the source branch onto
+          <span class="label-branch">{{mr.targetBranch}}</span>
+          to allow this merge request to be merged.
+        </span>
+      </template>
+      <template v-if="!mr.rebaseInProgress && mr.canPushToSourceBranch && !isMakingRequest">
+        <div class="accept-merge-holder clearfix js-toggle-container accept-action media space-children">
+          <button
+            type="button"
+            class="btn btn-small btn-reopen btn-success"
+            :disabled="isMakingRequest"
+            @click="rebase">
+            <loading-icon v-if="isMakingRequest" />
+            Rebase
+          </button>
+          <span
+            v-if="!hasRebasingError"
+            class="bold">
+            Fast-forward merge is not possible.
+            Rebase the source branch onto the target branch or merge target
+            branch into source branch to allow this merge request to be merged.
+          </span>
+          <span
+            v-else
+            class="bold danger">
+            {{rebasingError}}
+          </span>
+        </div>
+      </template>
+    </div>
+  </div>
+</template>

ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js

 import CEWidgetOptions from '~/vue_merge_request_widget/mr_widget_options';
 import WidgetApprovals from './components/approvals/mr_widget_approvals';
 import GeoSecondaryNode from './components/states/mr_widget_secondary_geo_node';
-import RebaseState from './components/states/mr_widget_rebase';
+import RebaseState from './components/states/mr_widget_rebase.vue';
 import WidgetCodeQuality from './components/mr_widget_code_quality.vue';
 
 export default {

spec/javascripts/vue_mr_widget/components/mr_widget_rebase_spec.js

+import Vue from 'vue';
+import component from 'ee/vue_merge_request_widget/components/states/mr_widget_rebase.vue';
+import mountComponent from '../../helpers/vue_mount_component_helper';
+
+describe('Merge request widget rebase component', () => {
+  let Component;
+  let vm;
+  beforeEach(() => {
+    Component = Vue.extend(component);
+  });
+
+  afterEach(() => {
+    vm.$destroy();
+  });
+
+  describe('While rebasing', () => {
+    it('should show progress message', () => {
+      vm = mountComponent(Component, {
+        mr: { rebaseInProgress: true },
+        service: {},
+      });
+
+      expect(
+        vm.$el.querySelector('.rebase-state-find-class-convention span').textContent.trim(),
+      ).toContain('Rebase in progress');
+    });
+  });
+
+  describe('With permissions', () => {
+    beforeEach(() => {
+      vm = mountComponent(Component, {
+        mr: {
+          rebaseInProgress: false,
+          canPushToSourceBranch: true,
+        },
+        service: {},
+      });
+    });
+
+    it('it should render rebase button and warning message', () => {
+      const text = vm.$el.querySelector('.rebase-state-find-class-convention span').textContent.trim();
+      expect(text).toContain('Fast-forward merge is not possible.');
+      expect(text).toContain('Rebase the source branch onto the target branch or merge target');
+      expect(text).toContain('branch into source branch to allow this merge request to be merged.');
+    });
+
+    it('it should render error message when it fails', (done) => {
+      vm.rebasingError = 'Something went wrong!';
+
+      Vue.nextTick(() => {
+        expect(
+          vm.$el.querySelector('.rebase-state-find-class-convention span').textContent.trim(),
+        ).toContain('Something went wrong!');
+        done();
+      });
+    });
+  });
+
+  describe('Without permissions', () => {
+    it('should render a message explaining user does not have permissions', () => {
+      vm = mountComponent(Component, {
+        mr: {
+          rebaseInProgress: false,
+          canPushToSourceBranch: false,
+          targetBranch: 'foo',
+        },
+        service: {},
+      });
+
+      const text = vm.$el.querySelector('.rebase-state-find-class-convention span').textContent.trim();
+
+      expect(text).toContain('Fast-forward merge is not possible.');
+      expect(text).toContain('Rebase the source branch onto');
+      expect(text).toContain('foo');
+      expect(text).toContain('to allow this merge request to be merged.');
+    });
+  });
+});

spec/services/merge_requests/rebase_service_spec.rb

@@ -9,14 +9,65 @@ describe MergeRequests::RebaseService do
   end
   let(:project) { merge_request.project }
 
+  subject(:service) { described_class.new(project, user, {}) }
+
   before do
     project.team << [user, :master]
   end
 
   describe '#execute' do
-    context 'valid params' do
-      let(:service) { described_class.new(project, user, {}) }
+    context 'when another rebase is already in progress' do
+      before do
+        allow(merge_request).to receive(:rebase_in_progress?).and_return(true)
+      end
+
+      it 'saves the error message' do
+        subject.execute(merge_request)
+
+        expect(merge_request.reload.merge_error).to eq 'Rebase task canceled: Another rebase is already in progress'
+      end
+
+      it 'returns an error' do
+        expect(service.execute(merge_request)).to match(status: :error,
+                                                        message: 'Failed to rebase. Should be done manually')
+      end
+    end
+
+    context 'when unexpected error occurs' do
+      before do
+        allow(service).to receive(:run_git_command).and_raise('Something went wrong')
+      end
+
+      it 'saves the error message' do
+        subject.execute(merge_request)
+
+        expect(merge_request.reload.merge_error).to eq 'Something went wrong'
+      end
+
+      it 'returns an error' do
+        expect(service.execute(merge_request)).to match(status: :error,
+                                                        message: 'Failed to rebase. Should be done manually')
+      end
+    end
 
+    context 'with git command failure' do
+      before do
+        allow(service).to receive(:popen).and_return(['Something went wrong', 1])
+      end
+
+      it 'saves the error message' do
+        subject.execute(merge_request)
+
+        expect(merge_request.reload.merge_error).to eq 'Something went wrong'
+      end
+
+      it 'returns an error' do
+        expect(service.execute(merge_request)).to match(status: :error,
+                                                        message: 'Failed to rebase. Should be done manually')
+      end
+    end
+
+    context 'valid params' do
       before do
         service.execute(merge_request)
       end
@@ -42,8 +93,6 @@ describe MergeRequests::RebaseService do
       end
 
       context 'git commands' do
-        let(:service) { described_class.new(project, user, {}) }
-
         it 'sets GL_REPOSITORY env variable when calling git commands' do
           expect_any_instance_of(described_class)
             .to receive(:run_git_command).exactly(4).with(

spec/services/merge_requests/squash_service_spec.rb

@@ -134,7 +134,7 @@ describe MergeRequests::SquashService do
 
         it 'logs the stage and output' do
           expect(service).to receive(:log_error).with(a_string_including(stage))
-          expect(service).to receive(:log_error).with(error)
+          expect(service).to receive(:log_error).with(error, save_message_on_model: true)
 
           service.execute(merge_request)
         end