kotovalexarian-likes-gitlab/gitlab-org--gitlab-foss
1
0
Fork 0
Code Releases Activity

Backport changes from gitlab-org/gitlab-ee!581 to CE.

Browse source 
!581 has a lot of changes that would cause merge conflicts if not
properly backported to CE. This commit/MR serves as a better
foundation for gitlab-org/gitlab-ee!581.

= Changes =

1. Move from `has_one {merge,push}_access_level` to `has_many`, with the
   `length` of the association limited to `1`. This is _effectively_ a
   `has_one` association, but should cause less conflicts with EE, which
   is set to `has_many`. This has a number of related changes in the
   views, specs, and factories.

2. Make `gon` variable loading more consistent (with EE!581) in the
   `ProtectedBranchesController`. Also use `::` to prefix the
   `ProtectedBranches` services, because this is required in EE.

3. Extract a `ProtectedBranchAccess` concern from the two access level
   models. This concern only has a single `humanize` method here, but
   will have more methods in EE.

4. Add `form_errors` to the protected branches creation form. This is
   not strictly required for EE compatibility, but was an oversight
   nonetheless.
This commit is contained in:
Timothy Andrew 2016-08-16 10:39:13 +05:30
parent 5a4ecb9825
commit e805a64700
14 changed files with 72 additions and 70 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
app/assets/javascripts/protected_branch_create.js.es6
View file

@ -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');

10
app/assets/javascripts/protected_branch_edit.js.es6
View file

@ -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: () => {

26
app/controllers/projects/protected_branches_controller.rb
View file

@ -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

7
app/models/concerns/protected_branch_access.rb Normal file
View file

@ -0,0 +1,7 @@
module ProtectedBranchAccess
extend ActiveSupport::Concern
def humanize
self.class.human_access_levels[self.access_level]
end
end

11
app/models/protected_branch.rb
View file

@ -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)

6
app/models/protected_branch/merge_access_level.rb
View file

@ -1,4 +1,6 @@
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

6
app/models/protected_branch/push_access_level.rb
View file

@ -1,4 +1,6 @@
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

18
app/services/protected_branches/create_service.rb
View file

@ -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

9
app/views/projects/protected_branches/_create_protected_branch.html.haml
View file

@ -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

12
app/views/projects/protected_branches/_protected_branch.html.haml
View file

@ -14,15 +14,15 @@
- 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') ,
= 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}" }})
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_level.access_level
= dropdown_tag( (protected_branch.push_access_level.humanize || 'Select') ,
= 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}" }})
data: { field_name: "allowed_to_push_#{protected_branch.id}", access_level_id: protected_branch.push_access_levels.first.id }})
- 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'

4
lib/api/entities.rb
View file

@ -129,12 +129,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.access_level == Gitlab::Access::DEVELOPER }
project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_levels.first.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 }
project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_levels.first.access_level == Gitlab::Access::DEVELOPER }
end
end

10
spec/factories/protected_branches.rb
View file

@ -4,25 +4,25 @@ FactoryGirl.define do
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)
protected_branch.push_access_levels.create!(access_level: Gitlab::Access::MASTER)
protected_branch.merge_access_levels.create!(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

13
spec/features/protected_branches_spec.rb
View file

@ -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"
@ -96,7 +99,7 @@ feature 'Projected Branches', feature: true, js: true do
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.first.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 +115,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.first.access_level).to eq(access_type_id)
end
end
@ -127,7 +130,7 @@ feature 'Projected Branches', feature: true, js: true do
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.first.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 +146,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.first.access_level).to eq(access_type_id)
end
end
end

6
spec/services/git_push_service_spec.rb
View file

@ -228,7 +228,7 @@ 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.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.merge_access_levels.first.access_level).to eq(Gitlab::Access::MASTER)
end
it "when pushing a branch for the first time with default branch protection disabled" do
@ -250,7 +250,7 @@ describe GitPushService, services: true do
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.merge_access_levels.first.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
@ -261,7 +261,7 @@ 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.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.merge_access_levels.first.access_level).to eq(Gitlab::Access::DEVELOPER)
end
it "when pushing new commits to existing branch" do