Commit 792ab063 authored by Bob Van Landuyt's avatar Bob Van Landuyt

Allow a user to select `allow maintainer to push`

When a project is not private, and the source branch not protected the
user can now select the option to allow maintainers to push to this
branch
parent 9ba0052c
<script>
export default {
name: 'MRWidgetMaintainerEdit',
props: {
maintainerEditAllowed: {
type: Boolean,
default: false,
required: false,
},
},
};
</script>
<template>
<section class="mr-info-list mr-maintainer-edit">
<p v-if="maintainerEditAllowed">
{{ s__("mrWidget|Allows edits from maintainers") }}
</p>
</section>
</template>
...@@ -15,6 +15,7 @@ export { default as WidgetHeader } from './components/mr_widget_header.vue'; ...@@ -15,6 +15,7 @@ export { default as WidgetHeader } from './components/mr_widget_header.vue';
export { default as WidgetMergeHelp } from './components/mr_widget_merge_help.vue'; export { default as WidgetMergeHelp } from './components/mr_widget_merge_help.vue';
export { default as WidgetPipeline } from './components/mr_widget_pipeline.vue'; export { default as WidgetPipeline } from './components/mr_widget_pipeline.vue';
export { default as WidgetDeployment } from './components/mr_widget_deployment'; export { default as WidgetDeployment } from './components/mr_widget_deployment';
export { default as WidgetMaintainerEdit } from './components/mr_widget_maintainer_edit.vue';
export { default as WidgetRelatedLinks } from './components/mr_widget_related_links.vue'; export { default as WidgetRelatedLinks } from './components/mr_widget_related_links.vue';
export { default as MergedState } from './components/states/mr_widget_merged.vue'; export { default as MergedState } from './components/states/mr_widget_merged.vue';
export { default as FailedToMerge } from './components/states/mr_widget_failed_to_merge.vue'; export { default as FailedToMerge } from './components/states/mr_widget_failed_to_merge.vue';
......
...@@ -6,6 +6,7 @@ import { ...@@ -6,6 +6,7 @@ import {
WidgetMergeHelp, WidgetMergeHelp,
WidgetPipeline, WidgetPipeline,
WidgetDeployment, WidgetDeployment,
WidgetMaintainerEdit,
WidgetRelatedLinks, WidgetRelatedLinks,
MergedState, MergedState,
ClosedState, ClosedState,
...@@ -211,6 +212,7 @@ export default { ...@@ -211,6 +212,7 @@ export default {
'mr-widget-merge-help': WidgetMergeHelp, 'mr-widget-merge-help': WidgetMergeHelp,
'mr-widget-pipeline': WidgetPipeline, 'mr-widget-pipeline': WidgetPipeline,
'mr-widget-deployment': WidgetDeployment, 'mr-widget-deployment': WidgetDeployment,
'mr-widget-maintainer-edit': WidgetMaintainerEdit,
'mr-widget-related-links': WidgetRelatedLinks, 'mr-widget-related-links': WidgetRelatedLinks,
'mr-widget-merged': MergedState, 'mr-widget-merged': MergedState,
'mr-widget-closed': ClosedState, 'mr-widget-closed': ClosedState,
...@@ -251,11 +253,12 @@ export default { ...@@ -251,11 +253,12 @@ export default {
:is="componentName" :is="componentName"
:mr="mr" :mr="mr"
:service="service" /> :service="service" />
<mr-widget-maintainer-edit
:maintainerEditAllowed="mr.maintainerEditAllowed" />
<mr-widget-related-links <mr-widget-related-links
v-if="shouldRenderRelatedLinks" v-if="shouldRenderRelatedLinks"
:state="mr.state" :state="mr.state"
:related-links="mr.relatedLinks" :related-links="mr.relatedLinks" />
/>
</div> </div>
<div <div
class="mr-widget-footer" class="mr-widget-footer"
......
...@@ -76,6 +76,7 @@ export default class MergeRequestStore { ...@@ -76,6 +76,7 @@ export default class MergeRequestStore {
this.canBeMerged = data.can_be_merged || false; this.canBeMerged = data.can_be_merged || false;
this.isMergeAllowed = data.mergeable || false; this.isMergeAllowed = data.mergeable || false;
this.mergeOngoing = data.merge_ongoing; this.mergeOngoing = data.merge_ongoing;
this.maintainerEditAllowed = data.allow_maintainer_to_push;
// Cherry-pick and Revert actions related // Cherry-pick and Revert actions related
this.canCherryPickInCurrentMR = currentUser.can_cherry_pick_on_current_merge_request || false; this.canCherryPickInCurrentMR = currentUser.can_cherry_pick_on_current_merge_request || false;
......
...@@ -453,7 +453,8 @@ ...@@ -453,7 +453,8 @@
} }
} }
.mr-links { .mr-links,
.mr-maintainer-edit {
padding-left: $status-icon-size + $status-icon-margin; padding-left: $status-icon-size + $status-icon-margin;
} }
......
...@@ -15,6 +15,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont ...@@ -15,6 +15,7 @@ class Projects::MergeRequests::ApplicationController < Projects::ApplicationCont
def merge_request_params_attributes def merge_request_params_attributes
[ [
:allow_maintainer_to_push,
:assignee_id, :assignee_id,
:description, :description,
:force_remove_source_branch, :force_remove_source_branch,
......
...@@ -125,6 +125,19 @@ module MergeRequestsHelper ...@@ -125,6 +125,19 @@ module MergeRequestsHelper
link_to(url[merge_request.project, merge_request], data: data_attrs, &block) link_to(url[merge_request.project, merge_request], data: data_attrs, &block)
end end
def allow_maintainer_push_unavailable_reason(merge_request)
return if merge_request.can_allow_maintainer_to_push?(current_user)
minimum_visibility = [merge_request.target_project.visibility_level,
merge_request.source_project.visibility_level].min
if minimum_visibility < Gitlab::VisibilityLevel::INTERNAL
_('Not available for private projects')
elsif ProtectedBranch.protected?(merge_request.source_project, merge_request.source_branch)
_('Not available for protected branches')
end
end
def merge_params_ee(merge_request) def merge_params_ee(merge_request)
{} {}
end end
......
...@@ -1087,4 +1087,22 @@ class MergeRequest < ActiveRecord::Base ...@@ -1087,4 +1087,22 @@ class MergeRequest < ActiveRecord::Base
project.merge_requests.merged.where(author_id: author_id).empty? project.merge_requests.merged.where(author_id: author_id).empty?
end end
def allow_maintainer_to_push
maintainer_push_possible? && super
end
alias_method :allow_maintainer_to_push?, :allow_maintainer_to_push
def maintainer_push_possible?
source_project.present? && for_fork? &&
target_project.visibility_level > Gitlab::VisibilityLevel::PRIVATE &&
source_project.visibility_level > Gitlab::VisibilityLevel::PRIVATE &&
!ProtectedBranch.protected?(source_project, source_branch)
end
def can_allow_maintainer_to_push?(user)
maintainer_push_possible? &&
Ability.allowed?(user, :push_code, source_project)
end
end end
...@@ -11,6 +11,7 @@ class MergeRequestWidgetEntity < IssuableEntity ...@@ -11,6 +11,7 @@ class MergeRequestWidgetEntity < IssuableEntity
expose :source_project_id expose :source_project_id
expose :target_branch expose :target_branch
expose :target_project_id expose :target_project_id
expose :allow_maintainer_to_push
expose :should_be_rebased?, as: :should_be_rebased expose :should_be_rebased?, as: :should_be_rebased
expose :ff_only_enabled do |merge_request| expose :ff_only_enabled do |merge_request|
......
...@@ -35,6 +35,14 @@ module MergeRequests ...@@ -35,6 +35,14 @@ module MergeRequests
end end
end end
def filter_params(merge_request)
super
unless merge_request.can_allow_maintainer_to_push?(current_user)
params.delete(:allow_maintainer_to_push)
end
end
def merge_request_metrics_service(merge_request) def merge_request_metrics_service(merge_request)
MergeRequestMetricsService.new(merge_request.metrics) MergeRequestMetricsService.new(merge_request.metrics)
end end
......
...@@ -6,6 +6,7 @@ module MergeRequests ...@@ -6,6 +6,7 @@ module MergeRequests
@params_issue_iid = params.delete(:issue_iid) @params_issue_iid = params.delete(:issue_iid)
self.merge_request = MergeRequest.new(params) self.merge_request = MergeRequest.new(params)
merge_request.author = current_user
merge_request.compare_commits = [] merge_request.compare_commits = []
merge_request.source_project = find_source_project merge_request.source_project = find_source_project
merge_request.target_project = find_target_project merge_request.target_project = find_target_project
......
...@@ -33,6 +33,8 @@ ...@@ -33,6 +33,8 @@
= render 'shared/issuable/form/merge_params', issuable: issuable = render 'shared/issuable/form/merge_params', issuable: issuable
= render 'shared/issuable/form/contribution', issuable: issuable, form: form
- if @merge_request_to_resolve_discussions_of - if @merge_request_to_resolve_discussions_of
.form-group .form-group
.col-sm-10.col-sm-offset-2 .col-sm-10.col-sm-offset-2
......
- issuable = local_assigns.fetch(:issuable)
- form = local_assigns.fetch(:form)
- return unless issuable.is_a?(MergeRequest)
- return unless issuable.for_fork?
- return unless can?(current_user, :push_code, issuable.source_project)
%hr
.form-group
.control-label
= _('Contribution')
.col-sm-10
.checkbox
= form.label :allow_maintainer_to_push do
= form.check_box :allow_maintainer_to_push, disabled: !issuable.can_allow_maintainer_to_push?(current_user)
= _('Allow edits from maintainers')
.help-block
= allow_maintainer_push_unavailable_reason(issuable)
---
title: Allow maintainers to push to forks of their projects when a merge request is open
merge_request: 17395
author:
type: added
require 'spec_helper'
describe 'create a merge request that allows maintainers to push', :js do
include ProjectForksHelper
let(:user) { create(:user) }
let(:target_project) { create(:project, :public, :repository) }
let(:source_project) { fork_project(target_project, user, repository: true, namespace: user.namespace) }
def visit_new_merge_request
visit project_new_merge_request_path(
source_project,
merge_request: {
source_project_id: source_project.id,
target_project_id: target_project.id,
source_branch: 'fix',
target_branch: 'master'
})
end
before do
sign_in(user)
end
it 'allows setting maintainer push possible' do
visit_new_merge_request
check 'Allow edits from maintainers'
click_button 'Submit merge request'
wait_for_requests
expect(page).to have_content('Allows edits from maintainers')
end
it 'shows a message when one of the projects is private' do
source_project.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
visit_new_merge_request
expect(page).to have_content('Not available for private projects')
end
it 'shows a message when the source branch is protected' do
create(:protected_branch, project: source_project, name: 'fix')
visit_new_merge_request
expect(page).to have_content('Not available for protected branches')
end
context 'when the merge request is being created within the same project' do
let(:source_project) { target_project }
it 'hides the checkbox if the merge request is being created within the same project' do
target_project.add_developer(user)
visit_new_merge_request
expect(page).not_to have_content('Allows edits from maintainers')
end
end
context 'when a maintainer tries to edit the option' do
let(:maintainer) { create(:user) }
let(:merge_request) do
create(:merge_request,
source_project: source_project,
target_project: target_project,
source_branch: 'fixes')
end
before do
target_project.add_master(maintainer)
sign_in(maintainer)
end
it 'it hides the option from maintainers' do
visit edit_project_merge_request_path(target_project, merge_request)
expect(page).not_to have_content('Allows edits from maintainers')
end
end
end
...@@ -12,7 +12,8 @@ ...@@ -12,7 +12,8 @@
"rebase_in_progress": { "type": "boolean" }, "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" },
"allow_maintainer_to_push": { "type": "boolean"}
}, },
"additionalProperties": false "additionalProperties": false
} }
...@@ -30,6 +30,7 @@ ...@@ -30,6 +30,7 @@
"source_project_id": { "type": "integer" }, "source_project_id": { "type": "integer" },
"target_branch": { "type": "string" }, "target_branch": { "type": "string" },
"target_project_id": { "type": "integer" }, "target_project_id": { "type": "integer" },
"allow_maintainer_to_push": { "type": "boolean"},
"metrics": { "metrics": {
"oneOf": [ "oneOf": [
{ "type": "null" }, { "type": "null" },
......
import Vue from 'vue';
import maintainerEditComponent from '~/vue_merge_request_widget/components/mr_widget_maintainer_edit.vue';
import mountComponent from 'spec/helpers/vue_mount_component_helper';
describe('MRWidgetAuthor', () => {
let vm;
beforeEach(() => {
const Component = Vue.extend(maintainerEditComponent);
vm = mountComponent(Component, {
maintainerEditAllowed: true,
});
});
afterEach(() => {
vm.$destroy();
});
it('renders the message when maintainers are allowed to edit', () => {
expect(vm.$el.textContent.trim()).toEqual('Allows edits from maintainers');
});
});
...@@ -349,6 +349,7 @@ describe('mrWidgetOptions', () => { ...@@ -349,6 +349,7 @@ describe('mrWidgetOptions', () => {
expect(comps['mr-widget-pipeline-blocked']).toBeDefined(); expect(comps['mr-widget-pipeline-blocked']).toBeDefined();
expect(comps['mr-widget-pipeline-failed']).toBeDefined(); expect(comps['mr-widget-pipeline-failed']).toBeDefined();
expect(comps['mr-widget-merge-when-pipeline-succeeds']).toBeDefined(); expect(comps['mr-widget-merge-when-pipeline-succeeds']).toBeDefined();
expect(comps['mr-widget-maintainer-edit']).toBeDefined();
}); });
}); });
......
...@@ -168,6 +168,7 @@ MergeRequest: ...@@ -168,6 +168,7 @@ MergeRequest:
- last_edited_by_id - last_edited_by_id
- head_pipeline_id - head_pipeline_id
- discussion_locked - discussion_locked
- allow_maintainer_to_push
MergeRequestDiff: MergeRequestDiff:
- id - id
- state - state
......
...@@ -2084,4 +2084,82 @@ describe MergeRequest do ...@@ -2084,4 +2084,82 @@ describe MergeRequest do
it_behaves_like 'checking whether a rebase is in progress' it_behaves_like 'checking whether a rebase is in progress'
end end
end end
describe '#allow_maintainer_to_push' do
let(:merge_request) do
build(:merge_request, source_branch: 'fixes', allow_maintainer_to_push: true)
end
it 'is false when pushing by a maintainer is not possible' do
expect(merge_request).to receive(:maintainer_push_possible?) { false }
expect(merge_request.allow_maintainer_to_push).to be_falsy
end
it 'is true when pushing by a maintainer is possible' do
expect(merge_request).to receive(:maintainer_push_possible?) { true }
expect(merge_request.allow_maintainer_to_push).to be_truthy
end
end
describe '#maintainer_push_possible?' do
let(:merge_request) do
build(:merge_request, source_branch: 'fixes')
end
before do
allow(ProtectedBranch).to receive(:protected?) { false }
end
it 'does not allow maintainer to push if the source project is the same as the target' do
merge_request.target_project = merge_request.source_project = create(:project, :public)
expect(merge_request.maintainer_push_possible?).to be_falsy
end
it 'allows maintainer to push when both source and target are public' do
merge_request.target_project = build(:project, :public)
merge_request.source_project = build(:project, :public)
expect(merge_request.maintainer_push_possible?).to be_truthy
end
it 'is not available for protected branches' do
merge_request.target_project = build(:project, :public)
merge_request.source_project = build(:project, :public)
expect(ProtectedBranch).to receive(:protected?)
.with(merge_request.source_project, 'fixes')
.and_return(true)
expect(merge_request.maintainer_push_possible?).to be_falsy
end
end
describe '#can_allow_maintainer_to_push?' do
let(:target_project) { create(:project, :public) }
let(:source_project) { fork_project(target_project) }
let(:merge_request) do
create(:merge_request,
source_project: source_project,
source_branch: 'fixes',
target_project: target_project)
end
let(:user) { create(:user) }
before do
allow(merge_request).to receive(:maintainer_push_possible?) { true }
end
it 'is false if the user does not have push access to the source project' do
expect(merge_request.can_allow_maintainer_to_push?(user)).to be_falsy
end
it 'is true when the user has push access to the source project' do
source_project.add_developer(user)
expect(merge_request.can_allow_maintainer_to_push?(user)).to be_truthy
end
end
end end
require 'spec_helper' require 'spec_helper'
describe MergeRequests::UpdateService, :mailer do describe MergeRequests::UpdateService, :mailer do
include ProjectForksHelper
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
let(:user) { create(:user) } let(:user) { create(:user) }
let(:user2) { create(:user) } let(:user2) { create(:user) }
...@@ -538,5 +540,40 @@ describe MergeRequests::UpdateService, :mailer do ...@@ -538,5 +540,40 @@ describe MergeRequests::UpdateService, :mailer do
let(:open_issuable) { merge_request } let(:open_issuable) { merge_request }
let(:closed_issuable) { create(:closed_merge_request, source_project: project) } let(:closed_issuable) { create(:closed_merge_request, source_project: project) }
end end
context 'setting `allow_maintainer_to_push`' do
let(:target_project) { create(:project, :public) }
let(:source_project) { fork_project(target_project) }
let(:user) { create(:user) }
let(:merge_request) do
create(:merge_request,
source_project: source_project,
source_branch: 'fixes',
target_project: target_project)
end
before do
allow(ProtectedBranch).to receive(:protected?).with(source_project, 'fixes') { false }
end
it 'does not allow a maintainer of the target project to set `allow_maintainer_to_push`' do
target_project.add_developer(user)
update_merge_request(allow_maintainer_to_push: true, title: 'Updated title')
expect(merge_request.title).to eq('Updated title')
expect(merge_request.allow_maintainer_to_push).to be_falsy
end
it 'is allowed by a user that can push to the source and can update the merge request' do
merge_request.update!(assignee: user)
source_project.add_developer(user)
update_merge_request(allow_maintainer_to_push: true, title: 'Updated title')
expect(merge_request.title).to eq('Updated title')
expect(merge_request.allow_maintainer_to_push).to be_truthy
end
end
end 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