From e805a6470031d942f7de604fdf7acfc7cf4f0b1a Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 16 Aug 2016 10:39:13 +0530 Subject: [PATCH] Backport changes from gitlab-org/gitlab-ee!581 to CE. !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. --- .../protected_branch_create.js.es6 | 4 +-- .../javascripts/protected_branch_edit.js.es6 | 10 ++++--- .../projects/protected_branches_controller.rb | 26 ++++++++++++------- .../concerns/protected_branch_access.rb | 7 +++++ app/models/protected_branch.rb | 11 +++++--- .../protected_branch/merge_access_level.rb | 6 ++--- .../protected_branch/push_access_level.rb | 6 ++--- .../protected_branches/create_service.rb | 18 +------------ .../_create_protected_branch.html.haml | 9 ++++--- .../_protected_branch.html.haml | 12 ++++----- lib/api/entities.rb | 4 +-- spec/factories/protected_branches.rb | 10 +++---- spec/features/protected_branches_spec.rb | 13 ++++++---- spec/services/git_push_service_spec.rb | 6 ++--- 14 files changed, 72 insertions(+), 70 deletions(-) create mode 100644 app/models/concerns/protected_branch_access.rb diff --git a/app/assets/javascripts/protected_branch_create.js.es6 b/app/assets/javascripts/protected_branch_create.js.es6 index 00e20a03b04..2efca2414dc 100644 --- a/app/assets/javascripts/protected_branch_create.js.es6 +++ b/app/assets/javascripts/protected_branch_create.js.es6 @@ -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'); diff --git a/app/assets/javascripts/protected_branch_edit.js.es6 b/app/assets/javascripts/protected_branch_edit.js.es6 index 8d42e268ebc..a59fcbfa082 100644 --- a/app/assets/javascripts/protected_branch_edit.js.es6 +++ b/app/assets/javascripts/protected_branch_edit.js.es6 @@ -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: () => { diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb index d28ec6e2eac..9a438d5512c 100644 --- a/app/controllers/projects/protected_branches_controller.rb +++ b/app/controllers/projects/protected_branches_controller.rb @@ -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 diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb new file mode 100644 index 00000000000..5a7b36070e7 --- /dev/null +++ b/app/models/concerns/protected_branch_access.rb @@ -0,0 +1,7 @@ +module ProtectedBranchAccess + extend ActiveSupport::Concern + + def humanize + self.class.human_access_levels[self.access_level] + end +end diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 226b3f54342..6240912a6e1 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -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) diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb index b1112ee737d..806b3ccd275 100644 --- a/app/models/protected_branch/merge_access_level.rb +++ b/app/models/protected_branch/merge_access_level.rb @@ -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 diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb index 6a5e49cf453..92e9c51d883 100644 --- a/app/models/protected_branch/push_access_level.rb +++ b/app/models/protected_branch/push_access_level.rb @@ -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 diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb index 6150a2a83c9..a84e335340d 100644 --- a/app/services/protected_branches/create_service.rb +++ b/app/services/protected_branches/create_service.rb @@ -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 diff --git a/app/views/projects/protected_branches/_create_protected_branch.html.haml b/app/views/projects/protected_branches/_create_protected_branch.html.haml index 85d0c494ba8..d4c6fa24768 100644 --- a/app/views/projects/protected_branches/_create_protected_branch.html.haml +++ b/app/views/projects/protected_branches/_create_protected_branch.html.haml @@ -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 diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml index e2e01ee78f8..eb4c67daa80 100644 --- a/app/views/projects/protected_branches/_protected_branch.html.haml +++ b/app/views/projects/protected_branches/_protected_branch.html.haml @@ -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' diff --git a/lib/api/entities.rb b/lib/api/entities.rb index ae74d14a4bb..7bce427adf6 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -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 diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb index 5575852c2d7..3b21174987f 100644 --- a/spec/factories/protected_branches.rb +++ b/spec/factories/protected_branches.rb @@ -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 diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb index 3499460c84d..5beb658b2f5 100644 --- a/spec/features/protected_branches_spec.rb +++ b/spec/features/protected_branches_spec.rb @@ -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 diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb index 80f6ebac86c..850b45f84f9 100644 --- a/spec/services/git_push_service_spec.rb +++ b/spec/services/git_push_service_spec.rb @@ -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