Commit 0a8aeb46 authored by Timothy Andrew's avatar Timothy Andrew

Use `Gitlab::Access` to protected branch access levels.

1. It makes sense to reuse these constants since we had them duplicated
   in the previous enum implementation. This also simplifies our
   `check_access` implementation, because we can use
   `project.team.max_member_access` directly.

2. Use `accepts_nested_attributes_for` to create push/merge access
   levels. This was a bit fiddly to set up, but this simplifies our code
   by quite a large amount. We can even get rid of
   `ProtectedBranches::BaseService`.

3. Move API handling back into the API (previously in
   `ProtectedBranches::BaseService#translate_api_params`.

4. The protected branch services now return a `ProtectedBranch` rather
   than `true/false`.

5. Run `load_protected_branches` on-demand in the `create` action, to
   prevent it being called unneccessarily.

6. "Masters" is pre-selected as the default option for "Allowed to Push"
   and "Allowed to Merge".

7. These changes were based on a review from @rymai in !5081.
parent c93a895a
class ProtectedBranchesAccessSelect {
constructor(container, saveOnSelect) {
constructor(container, saveOnSelect, selectDefault) {
this.container = container;
this.saveOnSelect = saveOnSelect;
this.container.find(".allowed-to-merge").each((i, element) => {
var fieldName = $(element).data('field-name');
return $(element).glDropdown({
var dropdown = $(element).glDropdown({
data: gon.merge_access_levels,
selectable: true,
fieldName: fieldName,
clicked: _.chain(this.onSelect).partial(element).bind(this).value()
});
if (selectDefault) {
dropdown.data('glDropdown').selectRowAtIndex(document.createEvent("Event"), 0);
}
});
this.container.find(".allowed-to-push").each((i, element) => {
var fieldName = $(element).data('field-name');
return $(element).glDropdown({
var dropdown = $(element).glDropdown({
data: gon.push_access_levels,
selectable: true,
fieldName: fieldName,
clicked: _.chain(this.onSelect).partial(element).bind(this).value()
});
if (selectDefault) {
dropdown.data('glDropdown').selectRowAtIndex(document.createEvent("Event"), 0);
}
});
}
......@@ -36,7 +44,9 @@ class ProtectedBranchesAccessSelect {
_method: 'PATCH',
id: $(dropdown).data('id'),
protected_branch: {
["" + ($(dropdown).data('type'))]: selected.id
["" + ($(dropdown).data('type')) + "_attributes"]: {
"access_level": selected.id
}
}
},
success: function() {
......
......@@ -3,23 +3,22 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
before_action :require_non_empty_project
before_action :authorize_admin_project!
before_action :load_protected_branch, only: [:show, :update, :destroy]
before_action :load_protected_branches, only: [:index, :create]
before_action :load_protected_branches, only: [:index]
layout "project_settings"
def index
@protected_branch = @project.protected_branches.new
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 } } })
load_protected_branches_gon_variables
end
def create
service = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params)
if service.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
@protected_branch = service.protected_branch
load_protected_branches
load_protected_branches_gon_variables
render :index
end
end
......@@ -29,15 +28,15 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
end
def update
service = ProtectedBranches::UpdateService.new(@project, current_user, params[:id], protected_branch_params)
@protected_branch = ProtectedBranches::UpdateService.new(@project, current_user, protected_branch_params).execute(@protected_branch)
if service.execute
if @protected_branch.valid?
respond_to do |format|
format.json { render json: service.protected_branch, status: :ok }
format.json { render json: @protected_branch, status: :ok }
end
else
respond_to do |format|
format.json { render json: service.protected_branch.errors, status: :unprocessable_entity }
format.json { render json: @protected_branch.errors, status: :unprocessable_entity }
end
end
end
......@@ -58,10 +57,18 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
end
def protected_branch_params
params.require(:protected_branch).permit(:name, :allowed_to_push, :allowed_to_merge)
params.require(:protected_branch).permit(:name,
merge_access_level_attributes: [:access_level],
push_access_level_attributes: [:access_level])
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 } } })
end
end
......@@ -8,18 +8,13 @@ class ProtectedBranch < ActiveRecord::Base
has_one :merge_access_level, dependent: :destroy
has_one :push_access_level, dependent: :destroy
accepts_nested_attributes_for :push_access_level
accepts_nested_attributes_for :merge_access_level
def commit
project.commit(self.name)
end
def allowed_to_push
self.push_access_level && self.push_access_level.access_level
end
def allowed_to_merge
self.merge_access_level && self.merge_access_level.access_level
end
# Returns all protected branches that match the given branch name.
# This realizes all records from the scope built up so far, and does
# _not_ return a relation.
......
......@@ -2,25 +2,19 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
belongs_to :protected_branch
delegate :project, to: :protected_branch
enum access_level: [:masters, :developers]
validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
Gitlab::Access::DEVELOPER] }
def self.human_access_levels
{
"masters" => "Masters",
"developers" => "Developers + Masters"
Gitlab::Access::MASTER => "Masters",
Gitlab::Access::DEVELOPER => "Developers + Masters"
}.with_indifferent_access
end
def check_access(user)
return true if user.is_admin?
min_member_access = if masters?
Gitlab::Access::MASTER
elsif developers?
Gitlab::Access::DEVELOPER
end
project.team.max_member_access(user.id) >= min_member_access
project.team.max_member_access(user.id) >= access_level
end
def humanize
......
......@@ -2,27 +2,22 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
belongs_to :protected_branch
delegate :project, to: :protected_branch
enum access_level: [:masters, :developers, :no_one]
validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
Gitlab::Access::DEVELOPER,
Gitlab::Access::NO_ACCESS] }
def self.human_access_levels
{
"masters" => "Masters",
"developers" => "Developers + Masters",
"no_one" => "No one"
Gitlab::Access::MASTER => "Masters",
Gitlab::Access::DEVELOPER => "Developers + Masters",
Gitlab::Access::NO_ACCESS => "No one"
}.with_indifferent_access
end
def check_access(user)
return false if no_one?
return false if access_level == Gitlab::Access::NO_ACCESS
return true if user.is_admin?
min_member_access = if masters?
Gitlab::Access::MASTER
elsif developers?
Gitlab::Access::DEVELOPER
end
project.team.max_member_access(user.id) >= min_member_access
project.team.max_member_access(user.id) >= access_level
end
def humanize
......
......@@ -88,10 +88,17 @@ class GitPushService < BaseService
# Set protection on the default branch if configured
if current_application_settings.default_branch_protection != PROTECTION_NONE
allowed_to_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? 'developers' : 'masters'
allowed_to_merge = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? 'developers' : 'masters'
params = { name: @project.default_branch, allowed_to_push: allowed_to_push, allowed_to_merge: allowed_to_merge }
params = {
name: @project.default_branch,
push_access_level_attributes: {
access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
},
merge_access_level_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
end
end
......
module ProtectedBranches
class BaseService < ::BaseService
def initialize(project, current_user, params = {})
super(project, current_user, params)
@allowed_to_push = params[:allowed_to_push]
@allowed_to_merge = params[:allowed_to_merge]
end
def set_access_levels!
translate_api_params!
set_merge_access_levels!
set_push_access_levels!
end
private
def set_merge_access_levels!
case @allowed_to_merge
when 'masters'
@protected_branch.merge_access_level.masters!
when 'developers'
@protected_branch.merge_access_level.developers!
end
end
def set_push_access_levels!
case @allowed_to_push
when 'masters'
@protected_branch.push_access_level.masters!
when 'developers'
@protected_branch.push_access_level.developers!
when 'no_one'
@protected_branch.push_access_level.no_one!
end
end
# The `branches` API still uses `developers_can_push` and `developers_can_merge`,
# which need to be translated to `allowed_to_push` and `allowed_to_merge`,
# respectively.
def translate_api_params!
@allowed_to_push ||=
case to_boolean(params[:developers_can_push])
when true
'developers'
when false
'masters'
end
@allowed_to_merge ||=
case to_boolean(params[:developers_can_merge])
when true
'developers'
when false
'masters'
end
end
protected
def to_boolean(value)
return true if value =~ /^(true|t|yes|y|1|on)$/i
return false if value =~ /^(false|f|no|n|0|off)$/i
nil
end
end
end
module ProtectedBranches
class CreateService < ProtectedBranches::BaseService
class CreateService < BaseService
attr_reader :protected_branch
def execute
raise Gitlab::Access::AccessDeniedError unless current_user.can?(:admin_project, project)
protected_branch = project.protected_branches.new(params)
ProtectedBranch.transaction do
@protected_branch = project.protected_branches.new(name: params[:name])
@protected_branch.save!
protected_branch.save!
@protected_branch.create_push_access_level!
@protected_branch.create_merge_access_level!
if protected_branch.push_access_level.blank?
protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
end
set_access_levels!
if protected_branch.merge_access_level.blank?
protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
end
end
true
protected_branch
rescue ActiveRecord::RecordInvalid
false
protected_branch
end
end
end
module ProtectedBranches
class UpdateService < ProtectedBranches::BaseService
class UpdateService < BaseService
attr_reader :protected_branch
def initialize(project, current_user, id, params = {})
super(project, current_user, params)
@protected_branch = ProtectedBranch.find(id)
end
def execute
def execute(protected_branch)
raise Gitlab::Access::AccessDeniedError unless current_user.can?(:admin_project, project)
ProtectedBranch.transaction do
set_access_levels!
end
true
rescue ActiveRecord::RecordInvalid
false
@protected_branch = protected_branch
@protected_branch.update(params)
@protected_branch
end
end
end
......@@ -26,4 +26,4 @@
= paginate @protected_branches, theme: 'gitlab'
:javascript
new ProtectedBranchesAccessSelect($(".protected-branches-list"), true);
new ProtectedBranchesAccessSelect($(".protected-branches-list"), true, false);
......@@ -18,12 +18,12 @@
= hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level
= dropdown_tag(protected_branch.merge_access_level.humanize,
options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable merge',
data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_merge" }})
data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "merge_access_level" }})
%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,
options: { title: "Allowed to push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable push',
data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_push" }})
data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "push_access_level" }})
- 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 btn-sm pull-right"
......@@ -32,20 +32,20 @@
are supported.
.form-group
= f.hidden_field :allowed_to_merge
= f.label :allowed_to_merge, "Allowed to merge: ", class: "label-light append-bottom-0"
= hidden_field_tag 'protected_branch[merge_access_level_attributes][access_level]'
= label_tag "Allowed to merge: ", nil, class: "label-light append-bottom-0"
= dropdown_tag("<Make a selection>",
options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge',
dropdown_class: 'dropdown-menu-selectable',
data: { field_name: "protected_branch[allowed_to_merge]" }})
data: { field_name: "protected_branch[merge_access_level_attributes][access_level]" }})
.form-group
= f.hidden_field :allowed_to_push
= f.label :allowed_to_push, "Allowed to push: ", class: "label-light append-bottom-0"
= hidden_field_tag 'protected_branch[push_access_level_attributes][access_level]'
= label_tag "Allowed to push: ", nil, class: "label-light append-bottom-0"
= dropdown_tag("<Make a selection>",
options: { title: "Allowed to push", toggle_class: 'allowed-to-push',
dropdown_class: 'dropdown-menu-selectable',
data: { field_name: "protected_branch[allowed_to_push]" }})
data: { field_name: "protected_branch[push_access_level_attributes][access_level]" }})
= f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true
......@@ -54,4 +54,4 @@
= render "branches_list"
:javascript
new ProtectedBranchesAccessSelect($(".new_protected_branch"), false);
new ProtectedBranchesAccessSelect($(".new_protected_branch"), false, true);
......@@ -5,7 +5,9 @@ class AddProtectedBranchesPushAccess < ActiveRecord::Migration
def change
create_table :protected_branch_push_access_levels do |t|
t.references :protected_branch, index: { name: "index_protected_branch_push_access" }, foreign_key: true, null: false
t.integer :access_level, default: 0, null: false
# Gitlab::Access::MASTER == 40
t.integer :access_level, default: 40, null: false
t.timestamps null: false
end
......
......@@ -5,7 +5,9 @@ class AddProtectedBranchesMergeAccess < ActiveRecord::Migration
def change
create_table :protected_branch_merge_access_levels do |t|
t.references :protected_branch, index: { name: "index_protected_branch_merge_access" }, foreign_key: true, null: false
t.integer :access_level, default: 0, null: false
# Gitlab::Access::MASTER == 40
t.integer :access_level, default: 40, null: false
t.timestamps null: false
end
......
......@@ -868,19 +868,19 @@ ActiveRecord::Schema.define(version: 20160722221922) do
add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
create_table "protected_branch_merge_access_levels", force: :cascade do |t|
t.integer "protected_branch_id", null: false
t.integer "access_level", default: 0, null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.integer "protected_branch_id", null: false
t.integer "access_level", default: 40, null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
end
add_index "protected_branch_merge_access_levels", ["protected_branch_id"], name: "index_protected_branch_merge_access", using: :btree
create_table "protected_branch_push_access_levels", force: :cascade do |t|
t.integer "protected_branch_id", null: false
t.integer "access_level", default: 0, null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.integer "protected_branch_id", null: false
t.integer "access_level", default: 40, null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
end
add_index "protected_branch_push_access_levels", ["protected_branch_id"], name: "index_protected_branch_push_access", using: :btree
......
......@@ -53,19 +53,37 @@ module API
@branch = user_project.repository.find_branch(params[:branch])
not_found!('Branch') unless @branch
protected_branch = user_project.protected_branches.find_by(name: @branch.name)
developers_can_merge = to_boolean(params[:developers_can_merge])
developers_can_push = to_boolean(params[:developers_can_push])
protected_branch_params = {
name: @branch.name,
developers_can_push: params[:developers_can_push],
developers_can_merge: params[:developers_can_merge]
name: @branch.name
}
service = if protected_branch
ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch.id, protected_branch_params)
else
ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params)
end
unless developers_can_merge.nil?
protected_branch_params.merge!({
merge_access_level_attributes: {
access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
}
})
end
service.execute
unless developers_can_push.nil?
protected_branch_params.merge!({
push_access_level_attributes: {
access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
}
})
end
if protected_branch
service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params)
service.execute(protected_branch)
else
service = ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params)
service.execute
end
present @branch, with: Entities::RepoBranch, project: user_project
end
......
......@@ -127,12 +127,12 @@ 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.developers? }
project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_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.developers? }
project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_level.access_level == Gitlab::Access::DEVELOPER }
end
end
......
......@@ -4,20 +4,26 @@ FactoryGirl.define do
project
after(:create) do |protected_branch|
protected_branch.create_push_access_level!(access_level: :masters)
protected_branch.create_merge_access_level!(access_level: :masters)
protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
end
trait :developers_can_push do
after(:create) { |protected_branch| protected_branch.push_access_level.developers! }
after(:create) do |protected_branch|
protected_branch.push_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
end
end
trait :developers_can_merge do
after(:create) { |protected_branch| protected_branch.merge_access_level.developers! }
after(:create) do |protected_branch|
protected_branch.merge_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
end
end
trait :no_one_can_push do
after(:create) { |protected_branch| protected_branch.push_access_level.no_one! }
after(:create) do |protected_branch|
protected_branch.push_access_level.update!(access_level: Gitlab::Access::NO_ACCESS)
end
end
end
end
......@@ -91,12 +91,12 @@ feature 'Projected Branches', feature: true, js: true do
set_protected_branch_name('master')
within('.new_protected_branch') do
find(".allowed-to-push").click
click_on access_type_name
within(".dropdown.open .dropdown-menu") { click_on access_type_name }
end
click_on "Protect"
expect(ProtectedBranch.count).to eq(1)
expect(ProtectedBranch.last.allowed_to_push).to eq(access_type_id)
expect(ProtectedBranch.last.push_access_level.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 +112,7 @@ feature 'Projected Branches', feature: true, js: true do
end
wait_for_ajax
expect(ProtectedBranch.last.allowed_to_push).to eq(access_type_id)
expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
end
end
......@@ -122,12 +122,12 @@ feature 'Projected Branches', feature: true, js: true do
set_protected_branch_name('master')
within('.new_protected_branch') do
find(".allowed-to-merge").click
click_on access_type_name
within(".dropdown.open .dropdown-menu") { click_on access_type_name }
end
click_on "Protect"
expect(ProtectedBranch.count).to eq(1)
expect(ProtectedBranch.last.allowed_to_merge).to eq(access_type_id)
expect(ProtectedBranch.last.merge_access_level.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 +143,7 @@ feature 'Projected Branches', feature: true, js: true do
end
wait_for_ajax
expect(ProtectedBranch.last.allowed_to_merge).to eq(access_type_id)
expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
end
end
end
......
......@@ -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.allowed_to_push).to eq('masters')
expect(project.protected_branches.first.allowed_to_merge).to eq('masters')
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)
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.allowed_to_push).to eq('developers')
expect(project.protected_branches.last.allowed_to_merge).to eq('masters')
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)
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.allowed_to_push).to eq('masters')
expect(project.protected_branches.first.allowed_to_merge).to eq('developers')
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)
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