Commit 7edc37d1 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'ee-581-backport-changes' into 'master'

Backport changes from gitlab-org/gitlab-ee!581

## What does this MR do?

Backports changes that were made in gitlab-org/gitlab-ee!581, to avoid potential merge conflicts in the future.

## What are the relevant issue numbers?

- Related to gitlab-org/gitlab-ee!581

## Does this MR meet the acceptance criteria?

## Tasks

- [ ]  !5824 Backport changes from EE!581 to CE
    - [x]  Implementation
        - [x]  ::ProtectedBranches::CreateService.new
        - [x]  Can't remove `load_protected_branches_gon_variables`
        - [x]  `has_many` with count enforced
        - [x]  Extract from access levels
        - [x]  project.protected_branches.create(params)
        - [x]  Improve "access_levels.first"
        - [x]  Fix tests
    - [x]  Fix build
    - [x]  Assign to Douwe
    - [ ]  Wait for review/merge

See merge request !5824
parents 7f853e22 d9efc7d9
......@@ -44,8 +44,8 @@
// Enable submit button
const $branchInput = this.$wrap.find('input[name="protected_branch[name]"]');
const $allowedToMergeInput = this.$wrap.find('input[name="protected_branch[merge_access_level_attributes][access_level]"]');
const $allowedToPushInput = this.$wrap.find('input[name="protected_branch[push_access_level_attributes][access_level]"]');
const $allowedToMergeInput = this.$wrap.find('input[name="protected_branch[merge_access_levels_attributes][0][access_level]"]');
const $allowedToPushInput = this.$wrap.find('input[name="protected_branch[push_access_levels_attributes][0][access_level]"]');
if ($branchInput.val() && $allowedToMergeInput.val() && $allowedToPushInput.val()){
this.$form.find('input[type="submit"]').removeAttr('disabled');
......
......@@ -39,12 +39,14 @@
_method: 'PATCH',
id: this.$wrap.data('banchId'),
protected_branch: {
merge_access_level_attributes: {
merge_access_levels_attributes: [{
id: this.$allowedToMergeDropdown.data('access-level-id'),
access_level: $allowedToMergeInput.val()
},
push_access_level_attributes: {
}],
push_access_levels_attributes: [{
id: this.$allowedToPushDropdown.data('access-level-id'),
access_level: $allowedToPushInput.val()
}
}]
}
},
success: () => {
......
class AutocompleteController < ApplicationController
skip_before_action :authenticate_user!, only: [:users]
before_action :load_project, only: [:users]
before_action :find_users, only: [:users]
def users
......@@ -55,11 +56,8 @@ class AutocompleteController < ApplicationController
def find_users
@users =
if params[:project_id].present?
project = Project.find(params[:project_id])
return render_404 unless can?(current_user, :read_project, project)
project.team.users
if @project
@project.team.users
elsif params[:group_id].present?
group = Group.find(params[:group_id])
return render_404 unless can?(current_user, :read_group, group)
......@@ -71,4 +69,14 @@ class AutocompleteController < ApplicationController
User.none
end
end
def load_project
@project ||= begin
if params[:project_id].present?
project = Project.find(params[:project_id])
return render_404 unless can?(current_user, :read_project, project)
project
end
end
end
end
......@@ -9,16 +9,16 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
def index
@protected_branch = @project.protected_branches.new
load_protected_branches_gon_variables
load_gon_index
end
def create
@protected_branch = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params).execute
@protected_branch = ::ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params).execute
if @protected_branch.persisted?
redirect_to namespace_project_protected_branches_path(@project.namespace, @project)
else
load_protected_branches
load_protected_branches_gon_variables
load_gon_index
render :index
end
end
......@@ -28,7 +28,7 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
end
def update
@protected_branch = ProtectedBranches::UpdateService.new(@project, current_user, protected_branch_params).execute(@protected_branch)
@protected_branch = ::ProtectedBranches::UpdateService.new(@project, current_user, protected_branch_params).execute(@protected_branch)
if @protected_branch.valid?
respond_to do |format|
......@@ -58,17 +58,23 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
def protected_branch_params
params.require(:protected_branch).permit(:name,
merge_access_level_attributes: [:access_level],
push_access_level_attributes: [:access_level])
merge_access_levels_attributes: [:access_level, :id],
push_access_levels_attributes: [:access_level, :id])
end
def load_protected_branches
@protected_branches = @project.protected_branches.order(:name).page(params[:page])
end
def load_protected_branches_gon_variables
gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } },
push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } })
def access_levels_options
{
push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text, before_divider: true } },
merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text, before_divider: true } }
}
end
def load_gon_index
params = { open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } }
gon.push(params.merge(access_levels_options))
end
end
module ProtectedBranchAccess
extend ActiveSupport::Concern
def humanize
self.class.human_access_levels[self.access_level]
end
end
......@@ -5,11 +5,14 @@ class ProtectedBranch < ActiveRecord::Base
validates :name, presence: true
validates :project, presence: true
has_one :merge_access_level, dependent: :destroy
has_one :push_access_level, dependent: :destroy
has_many :merge_access_levels, dependent: :destroy
has_many :push_access_levels, dependent: :destroy
accepts_nested_attributes_for :push_access_level
accepts_nested_attributes_for :merge_access_level
validates_length_of :merge_access_levels, is: 1, message: "are restricted to a single instance per protected branch."
validates_length_of :push_access_levels, is: 1, message: "are restricted to a single instance per protected branch."
accepts_nested_attributes_for :push_access_levels
accepts_nested_attributes_for :merge_access_levels
def commit
project.commit(self.name)
......
class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
include ProtectedBranchAccess
belongs_to :protected_branch
delegate :project, to: :protected_branch
......@@ -17,8 +19,4 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
project.team.max_member_access(user.id) >= access_level
end
def humanize
self.class.human_access_levels[self.access_level]
end
end
class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
include ProtectedBranchAccess
belongs_to :protected_branch
delegate :project, to: :protected_branch
......@@ -20,8 +22,4 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
project.team.max_member_access(user.id) >= access_level
end
def humanize
self.class.human_access_levels[self.access_level]
end
end
......@@ -91,12 +91,12 @@ class GitPushService < BaseService
params = {
name: @project.default_branch,
push_access_level_attributes: {
push_access_levels_attributes: [{
access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
},
merge_access_level_attributes: {
}],
merge_access_levels_attributes: [{
access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
}
}]
}
ProtectedBranches::CreateService.new(@project, current_user, params).execute
......
......@@ -5,23 +5,7 @@ module ProtectedBranches
def execute
raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
protected_branch = project.protected_branches.new(params)
ProtectedBranch.transaction do
protected_branch.save!
if protected_branch.push_access_level.blank?
protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
end
if protected_branch.merge_access_level.blank?
protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
end
end
protected_branch
rescue ActiveRecord::RecordInvalid
protected_branch
project.protected_branches.create(params)
end
end
end
......@@ -5,6 +5,7 @@
Protect a branch
.panel-body
.form-horizontal
= form_errors(@protected_branch)
.form-group
= f.label :name, class: 'col-md-2 text-right' do
Branch:
......@@ -18,19 +19,19 @@
%code production/*
are supported
.form-group
%label.col-md-2.text-right{ for: 'merge_access_level_attributes' }
%label.col-md-2.text-right{ for: 'merge_access_levels_attributes' }
Allowed to merge:
.col-md-10
= dropdown_tag('Select',
options: { toggle_class: 'js-allowed-to-merge wide',
data: { field_name: 'protected_branch[merge_access_level_attributes][access_level]', input_id: 'merge_access_level_attributes' }})
data: { field_name: 'protected_branch[merge_access_levels_attributes][0][access_level]', input_id: 'merge_access_levels_attributes' }})
.form-group
%label.col-md-2.text-right{ for: 'push_access_level_attributes' }
%label.col-md-2.text-right{ for: 'push_access_levels_attributes' }
Allowed to push:
.col-md-10
= dropdown_tag('Select',
options: { toggle_class: 'js-allowed-to-push wide',
data: { field_name: 'protected_branch[push_access_level_attributes][access_level]', input_id: 'push_access_level_attributes' }})
data: { field_name: 'protected_branch[push_access_levels_attributes][0][access_level]', input_id: 'push_access_levels_attributes' }})
.panel-footer
= f.submit 'Protect', class: 'btn-create btn', disabled: true
......@@ -13,16 +13,9 @@
= time_ago_with_tooltip(commit.committed_date)
- else
(branch was removed from repository)
%td
= hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level
= dropdown_tag( (protected_branch.merge_access_level.humanize || 'Select') ,
options: { toggle_class: 'js-allowed-to-merge', dropdown_class: 'dropdown-menu-selectable js-allowed-to-merge-container',
data: { field_name: "allowed_to_merge_#{protected_branch.id}" }})
%td
= hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_level.access_level
= dropdown_tag( (protected_branch.push_access_level.humanize || 'Select') ,
options: { toggle_class: 'js-allowed-to-push', dropdown_class: 'dropdown-menu-selectable js-allowed-to-push-container',
data: { field_name: "allowed_to_push_#{protected_branch.id}" }})
= render partial: 'update_protected_branch', locals: { protected_branch: protected_branch }
- if can_admin_project
%td
= link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: 'btn btn-warning'
%td
= hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_levels.first.access_level
= dropdown_tag( (protected_branch.merge_access_levels.first.humanize || 'Select') ,
options: { toggle_class: 'js-allowed-to-merge', dropdown_class: 'dropdown-menu-selectable js-allowed-to-merge-container',
data: { field_name: "allowed_to_merge_#{protected_branch.id}", access_level_id: protected_branch.merge_access_levels.first.id }})
%td
= hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_levels.first.access_level
= dropdown_tag( (protected_branch.push_access_levels.first.humanize || 'Select') ,
options: { toggle_class: 'js-allowed-to-push', dropdown_class: 'dropdown-menu-selectable js-allowed-to-push-container',
data: { field_name: "allowed_to_push_#{protected_branch.id}", access_level_id: protected_branch.push_access_levels.first.id }})
......@@ -61,22 +61,27 @@ module API
name: @branch.name
}
unless developers_can_merge.nil?
protected_branch_params.merge!({
merge_access_level_attributes: {
access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
}
})
# If `developers_can_merge` is switched off, _all_ `DEVELOPER`
# merge_access_levels need to be deleted.
if developers_can_merge == false
protected_branch.merge_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all
end
unless developers_can_push.nil?
protected_branch_params.merge!({
push_access_level_attributes: {
access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
}
})
# If `developers_can_push` is switched off, _all_ `DEVELOPER`
# push_access_levels need to be deleted.
if developers_can_push == false
protected_branch.push_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all
end
protected_branch_params.merge!(
merge_access_levels_attributes: [{
access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
}],
push_access_levels_attributes: [{
access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
}]
)
if protected_branch
service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params)
service.execute(protected_branch)
......
......@@ -129,12 +129,14 @@ module API
expose :developers_can_push do |repo_branch, options|
project = options[:project]
project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_level.access_level == Gitlab::Access::DEVELOPER }
access_levels = project.protected_branches.matching(repo_branch.name).map(&:push_access_levels).flatten
access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER }
end
expose :developers_can_merge do |repo_branch, options|
project = options[:project]
project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_level.access_level == Gitlab::Access::DEVELOPER }
access_levels = project.protected_branches.matching(repo_branch.name).map(&:merge_access_levels).flatten
access_levels.any? { |access_level| access_level.access_level == Gitlab::Access::DEVELOPER }
end
end
......
......@@ -32,7 +32,7 @@ module Gitlab
if project.protected_branch?(ref)
return true if project.empty_repo? && project.user_can_push_to_empty_repo?(user)
access_levels = project.protected_branches.matching(ref).map(&:push_access_level)
access_levels = project.protected_branches.matching(ref).map(&:push_access_levels).flatten
access_levels.any? { |access_level| access_level.check_access(user) }
else
user.can?(:push_code, project)
......@@ -43,7 +43,7 @@ module Gitlab
return false unless user
if project.protected_branch?(ref)
access_levels = project.protected_branches.matching(ref).map(&:merge_access_level)
access_levels = project.protected_branches.matching(ref).map(&:merge_access_levels).flatten
access_levels.any? { |access_level| access_level.check_access(user) }
else
user.can?(:push_code, project)
......
......@@ -3,26 +3,26 @@ FactoryGirl.define do
name
project
after(:create) do |protected_branch|
protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
after(:build) do |protected_branch|
protected_branch.push_access_levels.new(access_level: Gitlab::Access::MASTER)
protected_branch.merge_access_levels.new(access_level: Gitlab::Access::MASTER)
end
trait :developers_can_push do
after(:create) do |protected_branch|
protected_branch.push_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
protected_branch.push_access_levels.first.update!(access_level: Gitlab::Access::DEVELOPER)
end
end
trait :developers_can_merge do
after(:create) do |protected_branch|
protected_branch.merge_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
protected_branch.merge_access_levels.first.update!(access_level: Gitlab::Access::DEVELOPER)
end
end
trait :no_one_can_push do
after(:create) do |protected_branch|
protected_branch.push_access_level.update!(access_level: Gitlab::Access::NO_ACCESS)
protected_branch.push_access_levels.first.update!(access_level: Gitlab::Access::NO_ACCESS)
end
end
end
......
......@@ -71,7 +71,10 @@ feature 'Projected Branches', feature: true, js: true do
project.repository.add_branch(user, 'production-stable', 'master')
project.repository.add_branch(user, 'staging-stable', 'master')
project.repository.add_branch(user, 'development', 'master')
create(:protected_branch, project: project, name: "*-stable")
visit namespace_project_protected_branches_path(project.namespace, project)
set_protected_branch_name('*-stable')
click_on "Protect"
visit namespace_project_protected_branches_path(project.namespace, project)
click_on "2 matching branches"
......@@ -90,13 +93,17 @@ feature 'Projected Branches', feature: true, js: true do
visit namespace_project_protected_branches_path(project.namespace, project)
set_protected_branch_name('master')
within('.new_protected_branch') do
find(".js-allowed-to-push").click
within(".dropdown.open .dropdown-menu") { click_on access_type_name }
allowed_to_push_button = find(".js-allowed-to-push")
unless allowed_to_push_button.text == access_type_name
allowed_to_push_button.click
within(".dropdown.open .dropdown-menu") { click_on access_type_name }
end
end
click_on "Protect"
expect(ProtectedBranch.count).to eq(1)
expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to eq([access_type_id])
end
it "allows updating protected branches so that #{access_type_name} can push to them" do
......@@ -112,7 +119,7 @@ feature 'Projected Branches', feature: true, js: true do
end
wait_for_ajax
expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(access_type_id)
end
end
......@@ -121,13 +128,17 @@ feature 'Projected Branches', feature: true, js: true do
visit namespace_project_protected_branches_path(project.namespace, project)
set_protected_branch_name('master')
within('.new_protected_branch') do
find(".js-allowed-to-merge").click
within(".dropdown.open .dropdown-menu") { click_on access_type_name }
allowed_to_merge_button = find(".js-allowed-to-merge")
unless allowed_to_merge_button.text == access_type_name
allowed_to_merge_button.click
within(".dropdown.open .dropdown-menu") { click_on access_type_name }
end
end
click_on "Protect"
expect(ProtectedBranch.count).to eq(1)
expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
expect(ProtectedBranch.last.merge_access_levels.map(&:access_level)).to eq([access_type_id])
end
it "allows updating protected branches so that #{access_type_name} can merge to them" do
......@@ -143,7 +154,7 @@ feature 'Projected Branches', feature: true, js: true do
end
wait_for_ajax
expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
expect(ProtectedBranch.last.merge_access_levels.map(&:access_level)).to include(access_type_id)
end
end
end
......
......@@ -1089,13 +1089,13 @@ describe Project, models: true do
let(:project) { create(:project) }
it 'returns true when the branch matches a protected branch via direct match' do
project.protected_branches.create!(name: 'foo')
create(:protected_branch, project: project, name: "foo")
expect(project.protected_branch?('foo')).to eq(true)
end
it 'returns true when the branch matches a protected branch via wildcard match' do
project.protected_branches.create!(name: 'production/*')
create(:protected_branch, project: project, name: "production/*")
expect(project.protected_branch?('production/some-branch')).to eq(true)
end
......@@ -1105,7 +1105,7 @@ describe Project, models: true do
end
it 'returns false when the branch does not match a protected branch via wildcard match' do
project.protected_branches.create!(name: 'production/*')
create(:protected_branch, project: project, name: "production/*")
expect(project.protected_branch?('staging/some-branch')).to eq(false)
end
......
......@@ -243,7 +243,7 @@ describe API::API, api: true do
end
it "removes protected branch" do
project.protected_branches.create(name: branch_name)
create(:protected_branch, project: project, name: branch_name)
delete api("/projects/#{project.id}/repository/branches/#{branch_name}", user)
expect(response).to have_http_status(405)
expect(json_response['message']).to eq('Protected branch cant be removed')
......
......@@ -227,8 +227,8 @@ describe GitPushService, services: true do
expect(project.default_branch).to eq("master")
execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
expect(project.protected_branches).not_to be_empty
expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER)
expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::MASTER)
expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
end
it "when pushing a branch for the first time with default branch protection disabled" do
......@@ -249,8 +249,8 @@ describe GitPushService, services: true do
execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
expect(project.protected_branches).not_to be_empty
expect(project.protected_branches.last.push_access_level.access_level).to eq(Gitlab::Access::DEVELOPER)
expect(project.protected_branches.last.merge_access_level.access_level).to eq(Gitlab::Access::MASTER)
expect(project.protected_branches.last.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER])
expect(project.protected_branches.last.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
end
it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do
......@@ -260,8 +260,8 @@ describe GitPushService, services: true do
expect(project.default_branch).to eq("master")
execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
expect(project.protected_branches).not_to be_empty
expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER)
expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::DEVELOPER)
expect(project.protected_branches.first.push_access_levels.map(&:access_level)).to eq([Gitlab::Access::MASTER])
expect(project.protected_branches.first.merge_access_levels.map(&:access_level)).to eq([Gitlab::Access::DEVELOPER])
end
it "when pushing new commits to existing branch" do
......
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