Commit 27a75ea1 authored by Jan Provaznik's avatar Jan Provaznik

Backport 'Rebase' feature from EE to CE

When a project uses fast-forward merging strategy user has
to rebase MRs to target branch before it can be merged.
Now user can do rebase in UI by clicking 'Rebase' button
instead of doing rebase locally.

This feature was already present in EE, this is only backport
of the feature to CE. Couple of changes:
* removed rebase license check
* renamed migration (changed timestamp)

Closes #40301
parent 1560c234
<script>
import simplePoll from '../../../lib/utils/simple_poll';
import eventHub from '../../event_hub';
import statusIcon from '../mr_widget_status_icon';
import loadingIcon from '../../../vue_shared/components/loading_icon.vue';
import Flash from '../../../flash';
export default {
name: 'MRWidgetRebase',
props: {
mr: {
type: Object,
required: true,
},
service: {
type: Object,
required: true,
},
},
components: {
statusIcon,
loadingIcon,
},
data() {
return {
isMakingRequest: false,
rebasingError: null,
};
},
computed: {
status() {
if (this.mr.rebaseInProgress || this.isMakingRequest) {
return 'loading';
}
if (!this.mr.canPushToSourceBranch && !this.mr.rebaseInProgress) {
return 'warning';
}
return 'success';
},
showDisabledButton() {
return ['failed', 'loading'].includes(this.status);
},
},
methods: {
rebase() {
this.isMakingRequest = true;
this.rebasingError = null;
this.service.rebase()
.then(() => {
simplePoll(this.checkRebaseStatus);
})
.catch((error) => {
this.rebasingError = error.merge_error;
this.isMakingRequest = false;
Flash('Something went wrong. Please try again.');
});
},
checkRebaseStatus(continuePolling, stopPolling) {
this.service.poll()
.then(res => res.data)
.then((res) => {
if (res.rebase_in_progress) {
continuePolling();
} else {
this.isMakingRequest = false;
if (res.merge_error && 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();
});
},
},
};
</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-sm btn-reopen btn-success"
:disabled="isMakingRequest"
@click="rebase">
<loading-icon v-if="isMakingRequest" />
Rebase
</button>
<span
v-if="!rebasingError"
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>
...@@ -32,6 +32,7 @@ export { default as UnresolvedDiscussionsState } from './components/states/mr_wi ...@@ -32,6 +32,7 @@ export { default as UnresolvedDiscussionsState } from './components/states/mr_wi
export { default as PipelineBlockedState } from './components/states/mr_widget_pipeline_blocked'; export { default as PipelineBlockedState } from './components/states/mr_widget_pipeline_blocked';
export { default as PipelineFailedState } from './components/states/mr_widget_pipeline_failed'; export { default as PipelineFailedState } from './components/states/mr_widget_pipeline_failed';
export { default as MergeWhenPipelineSucceedsState } from './components/states/mr_widget_merge_when_pipeline_succeeds'; export { default as MergeWhenPipelineSucceedsState } from './components/states/mr_widget_merge_when_pipeline_succeeds';
export { default as RebaseState } from './components/states/mr_widget_rebase.vue';
export { default as AutoMergeFailed } from './components/states/mr_widget_auto_merge_failed'; export { default as AutoMergeFailed } from './components/states/mr_widget_auto_merge_failed';
export { default as CheckingState } from './components/states/mr_widget_checking'; export { default as CheckingState } from './components/states/mr_widget_checking';
export { default as MRWidgetStore } from './stores/mr_widget_store'; export { default as MRWidgetStore } from './stores/mr_widget_store';
......
...@@ -10,6 +10,7 @@ import { ...@@ -10,6 +10,7 @@ import {
MergedState, MergedState,
ClosedState, ClosedState,
MergingState, MergingState,
RebaseState,
WipState, WipState,
ArchivedState, ArchivedState,
ConflictsState, ConflictsState,
...@@ -79,6 +80,7 @@ export default { ...@@ -79,6 +80,7 @@ export default {
ciEnvironmentsStatusPath: store.ciEnvironmentsStatusPath, ciEnvironmentsStatusPath: store.ciEnvironmentsStatusPath,
statusPath: store.statusPath, statusPath: store.statusPath,
mergeActionsContentPath: store.mergeActionsContentPath, mergeActionsContentPath: store.mergeActionsContentPath,
rebasePath: store.rebasePath,
}; };
return new MRWidgetService(endpoints); return new MRWidgetService(endpoints);
}, },
...@@ -232,6 +234,7 @@ export default { ...@@ -232,6 +234,7 @@ export default {
'mr-widget-pipeline-failed': PipelineFailedState, 'mr-widget-pipeline-failed': PipelineFailedState,
'mr-widget-merge-when-pipeline-succeeds': MergeWhenPipelineSucceedsState, 'mr-widget-merge-when-pipeline-succeeds': MergeWhenPipelineSucceedsState,
'mr-widget-auto-merge-failed': AutoMergeFailed, 'mr-widget-auto-merge-failed': AutoMergeFailed,
'mr-widget-rebase': RebaseState,
}, },
template: ` template: `
<div class="mr-state-widget prepend-top-default"> <div class="mr-state-widget prepend-top-default">
......
...@@ -37,6 +37,10 @@ export default class MRWidgetService { ...@@ -37,6 +37,10 @@ export default class MRWidgetService {
return axios.get(this.endpoints.mergeActionsContentPath); return axios.get(this.endpoints.mergeActionsContentPath);
} }
rebase() {
return axios.post(this.endpoints.rebasePath);
}
static stopEnvironment(url) { static stopEnvironment(url) {
return axios.post(url); return axios.post(url);
} }
......
...@@ -25,6 +25,8 @@ export default function deviseState(data) { ...@@ -25,6 +25,8 @@ export default function deviseState(data) {
return this.mergeError ? stateKey.autoMergeFailed : stateKey.mergeWhenPipelineSucceeds; return this.mergeError ? stateKey.autoMergeFailed : stateKey.mergeWhenPipelineSucceeds;
} else if (!this.canMerge) { } else if (!this.canMerge) {
return stateKey.notAllowedToMerge; return stateKey.notAllowedToMerge;
} else if (this.shouldBeRebased) {
return stateKey.rebase;
} else if (this.canBeMerged) { } else if (this.canBeMerged) {
return stateKey.readyToMerge; return stateKey.readyToMerge;
} }
......
...@@ -26,6 +26,7 @@ export default class MergeRequestStore { ...@@ -26,6 +26,7 @@ export default class MergeRequestStore {
this.divergedCommitsCount = data.diverged_commits_count; this.divergedCommitsCount = data.diverged_commits_count;
this.pipeline = data.pipeline || {}; this.pipeline = data.pipeline || {};
this.deployments = this.deployments || data.deployments || []; this.deployments = this.deployments || data.deployments || [];
this.initRebase(data);
if (data.issues_links) { if (data.issues_links) {
const links = data.issues_links; const links = data.issues_links;
...@@ -124,6 +125,13 @@ export default class MergeRequestStore { ...@@ -124,6 +125,13 @@ export default class MergeRequestStore {
return this.state === stateKey.nothingToMerge; return this.state === stateKey.nothingToMerge;
} }
initRebase(data) {
this.canPushToSourceBranch = data.can_push_to_source_branch;
this.rebaseInProgress = data.rebase_in_progress;
this.approvalsLeft = !data.approved;
this.rebasePath = data.rebase_path;
}
static buildMetrics(metrics) { static buildMetrics(metrics) {
if (!metrics) { if (!metrics) {
return {}; return {};
......
...@@ -17,6 +17,7 @@ const stateToComponentMap = { ...@@ -17,6 +17,7 @@ const stateToComponentMap = {
failedToMerge: 'mr-widget-failed-to-merge', failedToMerge: 'mr-widget-failed-to-merge',
autoMergeFailed: 'mr-widget-auto-merge-failed', autoMergeFailed: 'mr-widget-auto-merge-failed',
shaMismatch: 'mr-widget-sha-mismatch', shaMismatch: 'mr-widget-sha-mismatch',
rebase: 'mr-widget-rebase',
}; };
const statesToShowHelpWidget = [ const statesToShowHelpWidget = [
...@@ -29,6 +30,7 @@ const statesToShowHelpWidget = [ ...@@ -29,6 +30,7 @@ const statesToShowHelpWidget = [
'pipelineFailed', 'pipelineFailed',
'pipelineBlocked', 'pipelineBlocked',
'autoMergeFailed', 'autoMergeFailed',
'rebase',
]; ];
export const stateKey = { export const stateKey = {
...@@ -46,6 +48,7 @@ export const stateKey = { ...@@ -46,6 +48,7 @@ export const stateKey = {
mergeWhenPipelineSucceeds: 'mergeWhenPipelineSucceeds', mergeWhenPipelineSucceeds: 'mergeWhenPipelineSucceeds',
notAllowedToMerge: 'notAllowedToMerge', notAllowedToMerge: 'notAllowedToMerge',
readyToMerge: 'readyToMerge', readyToMerge: 'readyToMerge',
rebase: 'rebase',
}; };
export default { export default {
......
...@@ -10,6 +10,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -10,6 +10,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort] before_action :authorize_update_issuable!, only: [:close, :edit, :update, :remove_wip, :sort]
before_action :set_issuables_index, only: [:index] before_action :set_issuables_index, only: [:index]
before_action :authenticate_user!, only: [:assign_related_issues] before_action :authenticate_user!, only: [:assign_related_issues]
before_action :check_user_can_push_to_source_branch!, only: [:rebase]
def index def index
@merge_requests = @issuables @merge_requests = @issuables
...@@ -223,6 +224,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -223,6 +224,12 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
render json: environments render json: environments
end end
def rebase
RebaseWorker.perform_async(@merge_request.id, current_user.id)
render nothing: true, status: 200
end
protected protected
alias_method :subscribable_resource, :merge_request alias_method :subscribable_resource, :merge_request
...@@ -322,4 +329,14 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -322,4 +329,14 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
@finder_type = MergeRequestsFinder @finder_type = MergeRequestsFinder
super super
end end
def check_user_can_push_to_source_branch!
return access_denied! unless @merge_request.source_branch_exists?
access_check = ::Gitlab::UserAccess
.new(current_user, project: @merge_request.source_project)
.can_push_to_branch?(@merge_request.source_branch)
access_denied! unless access_check
end
end end
...@@ -156,6 +156,13 @@ class MergeRequest < ActiveRecord::Base ...@@ -156,6 +156,13 @@ class MergeRequest < ActiveRecord::Base
'!' '!'
end end
def rebase_in_progress?
# The source project can be deleted
return false unless source_project
source_project.repository.rebase_in_progress?(id)
end
# Use this method whenever you need to make sure the head_pipeline is synced with the # Use this method whenever you need to make sure the head_pipeline is synced with the
# branch head commit, for example checking if a merge request can be merged. # branch head commit, for example checking if a merge request can be merged.
# For more information check: https://gitlab.com/gitlab-org/gitlab-ce/issues/40004 # For more information check: https://gitlab.com/gitlab-org/gitlab-ce/issues/40004
...@@ -607,7 +614,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -607,7 +614,7 @@ class MergeRequest < ActiveRecord::Base
check_if_can_be_merged check_if_can_be_merged
can_be_merged? can_be_merged? && !should_be_rebased?
end end
def mergeable_state?(skip_ci_check: false) def mergeable_state?(skip_ci_check: false)
......
...@@ -1099,6 +1099,13 @@ class Repository ...@@ -1099,6 +1099,13 @@ class Repository
@project.repository_storage_path @project.repository_storage_path
end end
def rebase(user, merge_request)
raw.rebase(user, merge_request.id, branch: merge_request.source_branch,
branch_sha: merge_request.source_branch_sha,
remote_repository: merge_request.target_project.repository.raw,
remote_branch: merge_request.target_branch)
end
private private
# TODO Generice finder, later split this on finders by Ref or Oid # TODO Generice finder, later split this on finders by Ref or Oid
......
...@@ -76,6 +76,12 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated ...@@ -76,6 +76,12 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end end
end end
def rebase_path
if !rebase_in_progress? && should_be_rebased? && user_can_push_to_source_branch?
rebase_project_merge_request_path(project, merge_request)
end
end
def target_branch_tree_path def target_branch_tree_path
if target_branch_exists? if target_branch_exists?
project_tree_path(project, target_branch) project_tree_path(project, target_branch)
...@@ -152,6 +158,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated ...@@ -152,6 +158,10 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
user_can_collaborate_with_project? && can_be_cherry_picked? user_can_collaborate_with_project? && can_be_cherry_picked?
end end
def can_push_to_source_branch?
source_branch_exists? && user_can_push_to_source_branch?
end
private private
def conflicts def conflicts
...@@ -174,6 +184,14 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated ...@@ -174,6 +184,14 @@ class MergeRequestPresenter < Gitlab::View::Presenter::Delegated
end.sort.to_sentence end.sort.to_sentence
end end
def user_can_push_to_source_branch?
return false unless source_branch_exists?
::Gitlab::UserAccess
.new(current_user, project: source_project)
.can_push_to_branch?(source_branch)
end
def user_can_collaborate_with_project? def user_can_collaborate_with_project?
can?(current_user, :push_code, project) || can?(current_user, :push_code, project) ||
(current_user && current_user.already_forked?(project)) (current_user && current_user.already_forked?(project))
......
...@@ -4,4 +4,5 @@ class MergeRequestBasicEntity < IssuableSidebarEntity ...@@ -4,4 +4,5 @@ class MergeRequestBasicEntity < IssuableSidebarEntity
expose :merge_error expose :merge_error
expose :state expose :state
expose :source_branch_exists?, as: :source_branch_exists expose :source_branch_exists?, as: :source_branch_exists
expose :rebase_in_progress?, as: :rebase_in_progress
end end
...@@ -23,6 +23,16 @@ class MergeRequestWidgetEntity < IssuableEntity ...@@ -23,6 +23,16 @@ class MergeRequestWidgetEntity < IssuableEntity
MergeRequestMetricsEntity.new(metrics).as_json MergeRequestMetricsEntity.new(metrics).as_json
end end
expose :rebase_commit_sha
expose :rebase_in_progress?, as: :rebase_in_progress
expose :can_push_to_source_branch do |merge_request|
presenter(merge_request).can_push_to_source_branch?
end
expose :rebase_path do |merge_request|
presenter(merge_request).rebase_path
end
# User entities # User entities
expose :merge_user, using: UserEntity expose :merge_user, using: UserEntity
......
module MergeRequests
class RebaseService < MergeRequests::WorkingCopyBaseService
def execute(merge_request)
@merge_request = merge_request
if rebase
success
else
error('Failed to rebase. Should be done manually')
end
end
def rebase
if merge_request.rebase_in_progress?
log_error('Rebase task canceled: Another rebase is already in progress', save_message_on_model: true)
return false
end
rebase_sha = repository.rebase(current_user, merge_request)
merge_request.update_attributes(rebase_commit_sha: rebase_sha)
true
rescue => e
log_error('Failed to rebase branch:')
log_error(e.message, save_message_on_model: true)
false
end
end
end
module MergeRequests
class WorkingCopyBaseService < MergeRequests::BaseService
attr_reader :merge_request
def source_project
@source_project ||= merge_request.source_project
end
def target_project
@target_project ||= merge_request.target_project
end
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
# Don't try to print expensive instance variables.
def inspect
"#<#{self.class} #{merge_request.to_reference(full: true)}>"
end
end
end
...@@ -10,4 +10,4 @@ ...@@ -10,4 +10,4 @@
No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded. No merge commits are created and all merges are fast-forwarded, which means that merging is only allowed if the branch could be fast-forwarded.
%br %br
%span.descr %span.descr
When fast-forward merge is not possible, the user must first rebase locally. When fast-forward merge is not possible, the user is given the option to rebase.
...@@ -10,4 +10,4 @@ ...@@ -10,4 +10,4 @@
This way you could make sure that if this merge request would build, after merging to target branch it would also build. This way you could make sure that if this merge request would build, after merging to target branch it would also build.
%br %br
%span.descr %span.descr
When fast-forward merge is not possible, the user must first rebase locally. When fast-forward merge is not possible, the user is given the option to rebase.
...@@ -89,6 +89,7 @@ ...@@ -89,6 +89,7 @@
- project_service - project_service
- propagate_service_template - propagate_service_template
- reactive_caching - reactive_caching
- rebase
- repository_fork - repository_fork
- repository_import - repository_import
- storage_migrator - storage_migrator
......
class RebaseWorker
include ApplicationWorker
def perform(merge_request_id, current_user_id)
current_user = User.find(current_user_id)
merge_request = MergeRequest.find(merge_request_id)
MergeRequests::RebaseService
.new(merge_request.source_project, current_user)
.execute(merge_request)
end
end
---
title: Allow user to rebase merge requests.
merge_request:
author:
type: added
...@@ -96,6 +96,7 @@ constraints(ProjectUrlConstrainer.new) do ...@@ -96,6 +96,7 @@ constraints(ProjectUrlConstrainer.new) do
post :toggle_subscription post :toggle_subscription
post :remove_wip post :remove_wip
post :assign_related_issues post :assign_related_issues
post :rebase
scope constraints: { format: nil }, action: :show do scope constraints: { format: nil }, action: :show do
get :commits, defaults: { tab: 'commits' } get :commits, defaults: { tab: 'commits' }
......
class AddRebaseCommitShaToMergeRequests < ActiveRecord::Migration
DOWNTIME = false
def change
add_column :merge_requests, :rebase_commit_sha, :string
end
end
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20171229225929) do ActiveRecord::Schema.define(version: 20171230123729) do
# These are extensions that must be enabled in order to support this database # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
...@@ -1099,6 +1099,7 @@ ActiveRecord::Schema.define(version: 20171229225929) do ...@@ -1099,6 +1099,7 @@ ActiveRecord::Schema.define(version: 20171229225929) do
t.string "merge_jid" t.string "merge_jid"
t.boolean "discussion_locked" t.boolean "discussion_locked"
t.integer "latest_merge_request_diff_id" t.integer "latest_merge_request_diff_id"
t.string "rebase_commit_sha"
end end
add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree add_index "merge_requests", ["assignee_id"], name: "index_merge_requests_on_assignee_id", using: :btree
......
...@@ -9,7 +9,7 @@ When the fast-forward merge ([`--ff-only`][ffonly]) setting is enabled, no merge ...@@ -9,7 +9,7 @@ When the fast-forward merge ([`--ff-only`][ffonly]) setting is enabled, no merge
commits will be created and all merges are fast-forwarded, which means that commits will be created and all merges are fast-forwarded, which means that
merging is only allowed if the branch could be fast-forwarded. merging is only allowed if the branch could be fast-forwarded.
When a fast-forward merge is not possible, the user must rebase the branch manually. When a fast-forward merge is not possible, the user is given the option to rebase.
## Use cases ## Use cases
...@@ -25,7 +25,7 @@ merge commits. In such cases, the fast-forward merge is the perfect candidate. ...@@ -25,7 +25,7 @@ merge commits. In such cases, the fast-forward merge is the perfect candidate.
Now, when you visit the merge request page, you will be able to accept it Now, when you visit the merge request page, you will be able to accept it
**only if a fast-forward merge is possible**. **only if a fast-forward merge is possible**.
![Fast forward merge request](img/ff_merge_mr.png) ![Fast forward merge request](img/ff_merge_rebase.png)
If the target branch is ahead of the source branch, you need to rebase the If the target branch is ahead of the source branch, you need to rebase the
source branch locally before you will be able to do a fast-forward merge. source branch locally before you will be able to do a fast-forward merge.
......
...@@ -22,3 +22,20 @@ Feature: Project Ff Merge Requests ...@@ -22,3 +22,20 @@ Feature: Project Ff Merge Requests
Then I should see ff-only merge button Then I should see ff-only merge button
When I accept this merge request When I accept this merge request
Then I should see merged request Then I should see merged request
@javascript
Scenario: I do rebase before ff-only merge
Given ff merge enabled
And rebase before merge enabled
When I visit merge request page "Bug NS-05"
Then I should see rebase button
When I press rebase button
Then I should see rebase in progress message
@javascript
Scenario: I do rebase before regular merge
Given rebase before merge enabled
When I visit merge request page "Bug NS-05"
Then I should see rebase button
When I press rebase button
Then I should see rebase in progress message
...@@ -17,6 +17,10 @@ class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps ...@@ -17,6 +17,10 @@ class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps
author: project.users.first) author: project.users.first)
end end
step 'merge request is mergeable' do
expect(page).to have_button 'Merge'
end
step 'I should see ff-only merge button' do step 'I should see ff-only merge button' do
expect(page).to have_content "Fast-forward merge without a merge commit" expect(page).to have_content "Fast-forward merge without a merge commit"
expect(page).to have_button 'Merge' expect(page).to have_button 'Merge'
...@@ -45,6 +49,10 @@ class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps ...@@ -45,6 +49,10 @@ class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps
project.save! project.save!
end end
step 'I should see rebase button' do
expect(page).to have_button "Rebase"
end
step 'merge request "Bug NS-05" is rebased' do step 'merge request "Bug NS-05" is rebased' do
merge_request.source_branch = 'flatten-dir' merge_request.source_branch = 'flatten-dir'
merge_request.target_branch = 'improve/awesome' merge_request.target_branch = 'improve/awesome'
...@@ -59,6 +67,20 @@ class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps ...@@ -59,6 +67,20 @@ class Spinach::Features::ProjectFfMergeRequests < Spinach::FeatureSteps
merge_request.save! merge_request.save!
end end
step 'rebase before merge enabled' do
project = merge_request.target_project
project.merge_requests_rebase_enabled = true
project.save!
end
step 'I press rebase button' do
click_button "Rebase"
end
step "I should see rebase in progress message" do
expect(page).to have_content("Rebase in progress")
end
def merge_request def merge_request
@merge_request ||= MergeRequest.find_by!(title: "Bug NS-05") @merge_request ||= MergeRequest.find_by!(title: "Bug NS-05")
end end
......
...@@ -97,6 +97,11 @@ module Gitlab ...@@ -97,6 +97,11 @@ module Gitlab
end end
end end
def update_branch(branch_name, newrev, oldrev)
ref = Gitlab::Git::BRANCH_REF_PREFIX + branch_name
update_ref_in_hooks(ref, newrev, oldrev)
end
private private
# Returns [newrev, should_run_after_create, should_run_after_create_branch] # Returns [newrev, should_run_after_create, should_run_after_create_branch]
......
...@@ -684,4 +684,62 @@ describe Projects::MergeRequestsController do ...@@ -684,4 +684,62 @@ describe Projects::MergeRequestsController do
format: :json format: :json
end end
end end
describe 'POST #rebase' do
let(:viewer) { user }
def post_rebase
post :rebase, namespace_id: project.namespace, project_id: project, id: merge_request
end
def expect_rebase_worker_for(user)
expect(RebaseWorker).to receive(:perform_async).with(merge_request.id, user.id)
end
context 'successfully' do
it 'enqeues a RebaseWorker' do
expect_rebase_worker_for(viewer)
post_rebase
expect(response.status).to eq(200)
end
end
context 'with a forked project' do
let(:fork_project) { create(:project, :repository, forked_from_project: project) }
let(:fork_owner) { fork_project.owner }
before do
merge_request.update!(source_project: fork_project)
fork_project.add_reporter(user)
end
context 'user cannot push to source branch' do
it 'returns 404' do
expect_rebase_worker_for(viewer).never
post_rebase
expect(response.status).to eq(404)
end
end
context 'user can push to source branch' do
before do
project.add_reporter(fork_owner)
sign_in(fork_owner)
end
it 'returns 200' do
expect_rebase_worker_for(fork_owner)
post_rebase
expect(response.status).to eq(200)
end
end
end
end
end end
...@@ -9,6 +9,7 @@ ...@@ -9,6 +9,7 @@
"human_time_estimate": { "type": ["string", "null"] }, "human_time_estimate": { "type": ["string", "null"] },
"human_total_time_spent": { "type": ["string", "null"] }, "human_total_time_spent": { "type": ["string", "null"] },
"merge_error": { "type": ["string", "null"] }, "merge_error": { "type": ["string", "null"] },
"rebase_in_progress": { "type": "boolean" },
"assignee_id": { "type": ["integer", "null"] }, "assignee_id": { "type": ["integer", "null"] },
"subscribed": { "type": ["boolean", "null"] }, "subscribed": { "type": ["boolean", "null"] },
"participants": { "type": "array" } "participants": { "type": "array" }
......
...@@ -103,7 +103,11 @@ ...@@ -103,7 +103,11 @@
"remove_source_branch": { "type": ["boolean", "null"] }, "remove_source_branch": { "type": ["boolean", "null"] },
"merge_ongoing": { "type": "boolean" }, "merge_ongoing": { "type": "boolean" },
"ff_only_enabled": { "type": ["boolean", false] }, "ff_only_enabled": { "type": ["boolean", false] },
"should_be_rebased": { "type": "boolean" } "should_be_rebased": { "type": "boolean" },
"rebase_commit_sha": { "type": ["string", "null"] },
"rebase_in_progress": { "type": "boolean" },
"can_push_to_source_branch": { "type": "boolean" },
"rebase_path": { "type": ["string", "null"] }
}, },
"additionalProperties": false "additionalProperties": false
} }
import Vue from 'vue';
import eventHub from '~/vue_merge_request_widget/event_hub';
import component from '~/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.');
});
});
describe('methods', () => {
it('checkRebaseStatus', (done) => {
spyOn(eventHub, '$emit');
vm = mountComponent(Component, {
mr: {},
service: {
rebase() {
return Promise.resolve();
},
poll() {
return Promise.resolve({
data: {
rebase_in_progress: false,
merge_error: null,
},
});
},
},
});
vm.rebase();
// Wait for the rebase request
vm.$nextTick()
// Wait for the polling request
.then(vm.$nextTick())
// Wait for the eventHub to be called
.then(vm.$nextTick())
.then(() => {
expect(eventHub.$emit).toHaveBeenCalledWith('MRWidgetUpdateRequested');
})
.then(done)
.catch(done.fail);
});
});
});
...@@ -1903,4 +1903,50 @@ describe MergeRequest do ...@@ -1903,4 +1903,50 @@ describe MergeRequest do
end end
end end
end end
describe '#should_be_rebased?' do
let(:project) { create(:project, :repository) }
it 'returns false for the same source and target branches' do
merge_request = create(:merge_request, source_project: project, target_project: project)
expect(merge_request.should_be_rebased?).to be_falsey
end
end
describe '#rebase_in_progress?' do
# Create merge request and project before we stub file calls
before do
subject
end
it 'returns true when there is a current rebase directory' do
allow(File).to receive(:exist?).and_return(true)
allow(File).to receive(:mtime).and_return(Time.now)
expect(subject.rebase_in_progress?).to be_truthy
end
it 'returns false when there is no rebase directory' do
allow(File).to receive(:exist?).and_return(false)
expect(subject.rebase_in_progress?).to be_falsey
end
it 'returns false when the rebase directory has expired' do
allow(File).to receive(:exist?).and_return(true)
allow(File).to receive(:mtime).and_return(20.minutes.ago)
expect(subject.rebase_in_progress?).to be_falsey
end
it 'returns false when the source project has been removed' do
allow(subject).to receive(:source_project).and_return(nil)
allow(File).to receive(:exist?).and_return(true)
allow(File).to receive(:mtime).and_return(Time.now)
expect(File).not_to have_received(:exist?)
expect(subject.rebase_in_progress?).to be_falsey
end
end
end end
...@@ -418,14 +418,21 @@ describe Project do ...@@ -418,14 +418,21 @@ describe Project do
end end
describe '#merge_method' do describe '#merge_method' do
it 'returns "ff" merge_method when ff is enabled' do using RSpec::Parameterized::TableSyntax
project = build(:project, merge_requests_ff_only_enabled: true)
expect(project.merge_method).to be :ff where(:ff, :rebase, :method) do
true | true | :ff
true | false | :ff
false | true | :rebase_merge
false | false | :merge
end end
it 'returns "merge" merge_method when ff is disabled' do with_them do
project = build(:project, merge_requests_ff_only_enabled: false) let(:project) { build(:project, merge_requests_rebase_enabled: rebase, merge_requests_ff_only_enabled: ff) }
expect(project.merge_method).to be :merge
subject { project.merge_method }
it { is_expected.to eq(method) }
end end
end end
......
...@@ -404,4 +404,67 @@ describe MergeRequestPresenter do ...@@ -404,4 +404,67 @@ describe MergeRequestPresenter do
.to eq("<a href=\"/#{resource.source_project.full_path}/tree/#{resource.source_branch}\">#{resource.source_branch}</a>") .to eq("<a href=\"/#{resource.source_project.full_path}/tree/#{resource.source_branch}\">#{resource.source_branch}</a>")
end end
end end
describe '#rebase_path' do
before do
allow(resource).to receive(:rebase_in_progress?) { rebase_in_progress }
allow(resource).to receive(:should_be_rebased?) { should_be_rebased }
allow_any_instance_of(Gitlab::UserAccess::RequestCacheExtension)
.to receive(:can_push_to_branch?)
.with(resource.source_branch)
.and_return(can_push_to_branch)
end
subject do
described_class.new(resource, current_user: user).rebase_path
end
context 'when can rebase' do
let(:rebase_in_progress) { false }
let(:can_push_to_branch) { true }
let(:should_be_rebased) { true }
before do
allow(resource).to receive(:source_branch_exists?) { true }
end
it 'returns path' do
is_expected
.to eq("/#{project.full_path}/merge_requests/#{resource.iid}/rebase")
end
end
context 'when cannot rebase' do
context 'when rebase in progress' do
let(:rebase_in_progress) { true }
let(:can_push_to_branch) { true }
let(:should_be_rebased) { true }
it 'returns nil' do
is_expected.to be_nil
end
end
context 'when user cannot merge' do
let(:rebase_in_progress) { false }
let(:can_push_to_branch) { false }
let(:should_be_rebased) { true }
it 'returns nil' do
is_expected.to be_nil
end
end
context 'should not be rebased' do
let(:rebase_in_progress) { false }
let(:can_push_to_branch) { true }
let(:should_be_rebased) { false }
it 'returns nil' do
is_expected.to be_nil
end
end
end
end
end end
...@@ -190,4 +190,20 @@ describe MergeRequestWidgetEntity do ...@@ -190,4 +190,20 @@ describe MergeRequestWidgetEntity do
end end
end end
end end
describe 'when source project is deleted' do
let(:project) { create(:project, :repository) }
let(:fork_project) { create(:project, :repository, forked_from_project: project) }
let(:merge_request) { create(:merge_request, source_project: fork_project, target_project: project) }
it 'returns a blank rebase_path' do
allow(merge_request).to receive(:should_be_rebased?).and_return(true)
fork_project.destroy
merge_request.reload
entity = described_class.new(merge_request, request: request).as_json
expect(entity[:rebase_path]).to be_nil
end
end
end end
require 'spec_helper'
describe MergeRequests::RebaseService do
include ProjectForksHelper
let(:user) { create(:user) }
let(:merge_request) do
create(:merge_request,
source_branch: 'feature_conflict',
target_branch: 'master')
end
let(:project) { merge_request.project }
let(:repository) { project.repository.raw }
subject(:service) { described_class.new(project, user, {}) }
before do
project.add_master(user)
end
describe '#execute' do
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(repository).to receive(:run_git!).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(repository).to receive(:run_git!).and_raise(Gitlab::Git::Repository::GitError, '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 'valid params' do
before do
service.execute(merge_request)
end
it 'rebases source branch' do
parent_sha = merge_request.source_project.repository.commit(merge_request.source_branch).parents.first.sha
target_branch_sha = merge_request.target_project.repository.commit(merge_request.target_branch).sha
expect(parent_sha).to eq(target_branch_sha)
end
it 'records the new SHA on the merge request' do
head_sha = merge_request.source_project.repository.commit(merge_request.source_branch).sha
expect(merge_request.reload.rebase_commit_sha).to eq(head_sha)
end
it 'logs correct author and commiter' do
head_commit = merge_request.source_project.repository.commit(merge_request.source_branch)
expect(head_commit.author_email).to eq('dmitriy.zaporozhets@gmail.com')
expect(head_commit.author_name).to eq('Dmitriy Zaporozhets')
expect(head_commit.committer_email).to eq(user.email)
expect(head_commit.committer_name).to eq(user.name)
end
context 'git commands' do
it 'sets GL_REPOSITORY env variable when calling git commands' do
expect(repository).to receive(:popen).exactly(3)
.with(anything, anything, hash_including('GL_REPOSITORY'))
.and_return(['', 0])
service.execute(merge_request)
end
end
context 'fork' do
let(:forked_project) do
fork_project(project, user, repository: true)
end
let(:merge_request_from_fork) do
forked_project.repository.create_file(
user,
'new-file-to-target',
'',
message: 'Add new file to target',
branch_name: 'master')
create(:merge_request,
source_branch: 'master', source_project: forked_project,
target_branch: 'master', target_project: project)
end
it 'rebases source branch' do
parent_sha = forked_project.repository.commit(merge_request_from_fork.source_branch).parents.first.sha
target_branch_sha = project.repository.commit(merge_request_from_fork.target_branch).sha
expect(parent_sha).to eq(target_branch_sha)
end
end
end
end
end
...@@ -54,6 +54,8 @@ describe 'projects/merge_requests/show.html.haml' do ...@@ -54,6 +54,8 @@ describe 'projects/merge_requests/show.html.haml' do
it 'closes the merge request if the source project does not exist' do it 'closes the merge request if the source project does not exist' do
closed_merge_request.update_attributes(state: 'open') closed_merge_request.update_attributes(state: 'open')
forked_project.destroy forked_project.destroy
# Reload merge request so MergeRequest#source_project turns to `nil`
closed_merge_request.reload
render render
......
require 'spec_helper'
describe RebaseWorker, '#perform' do
context 'when rebasing an MR from a fork where upstream has protected branches' do
let(:upstream_project) { create(:project, :repository) }
let(:fork_project) { create(:project, :repository) }
let(:merge_request) do
create(:merge_request,
source_project: fork_project,
source_branch: 'feature_conflict',
target_project: upstream_project,
target_branch: 'master')
end
before do
create(:forked_project_link, forked_to_project: fork_project, forked_from_project: upstream_project)
end
it 'sets the correct project for running hooks' do
expect(MergeRequests::RebaseService)
.to receive(:new).with(fork_project, merge_request.author).and_call_original
subject.perform(merge_request, merge_request.author)
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment