Commit f16377e7 authored by James Edwards-Jones's avatar James Edwards-Jones

Protected Tags backend review changes

Added changelog
parent 18506d4b
class Projects::ProtectedBranchesController < Projects::ProtectedRefsController class Projects::ProtectedBranchesController < Projects::ProtectedRefsController
protected protected
def protected_ref
@protected_branch
end
def protected_ref=(val)
@protected_branch = val
end
def matching_refs=(val)
@matching_branches = val
end
def project_refs def project_refs
@project.repository.branches @project.repository.branches
end end
def create_service def create_service_class
::ProtectedBranches::CreateService ::ProtectedBranches::CreateService
end end
def update_service def update_service_class
::ProtectedBranches::UpdateService ::ProtectedBranches::UpdateService
end end
def load_protected_ref def load_protected_ref
self.protected_ref = @project.protected_branches.find(params[:id]) @protected_ref = @project.protected_branches.find(params[:id])
end end
def protected_ref_params def protected_ref_params
......
class Projects::ProtectedRefsController < Projects::ApplicationController class Projects::ProtectedRefsController < Projects::ApplicationController
include RepositorySettingsRedirect include RepositorySettingsRedirect
# Authorize # Authorize
before_action :require_non_empty_project before_action :require_non_empty_project
before_action :authorize_admin_project! before_action :authorize_admin_project!
...@@ -12,33 +13,31 @@ class Projects::ProtectedRefsController < Projects::ApplicationController ...@@ -12,33 +13,31 @@ class Projects::ProtectedRefsController < Projects::ApplicationController
end end
def create def create
self.protected_ref = create_service.new(@project, current_user, protected_ref_params).execute protected_ref = create_service_class.new(@project, current_user, protected_ref_params).execute
unless protected_ref.persisted? unless protected_ref.persisted?
flash[:alert] = protected_ref.errors.full_messages.join(', ').html_safe flash[:alert] = protected_ref.errors.full_messages.join(', ').html_safe
end end
redirect_to_repository_settings(@project) redirect_to_repository_settings(@project)
end end
def show def show
self.matching_refs = protected_ref.matching(project_refs) @matching_refs = @protected_ref.matching(project_refs)
end end
def update def update
self.protected_ref = update_service.new(@project, current_user, protected_ref_params).execute(protected_ref) @protected_ref = update_service_class.new(@project, current_user, protected_ref_params).execute(@protected_ref)
if protected_ref.valid? if @protected_ref.valid?
respond_to do |format| render json: @protected_ref, status: :ok
format.json { render json: protected_ref, status: :ok }
end
else else
respond_to do |format| render json: @protected_ref.errors, status: :unprocessable_entity
format.json { render json: protected_ref.errors, status: :unprocessable_entity }
end
end end
end end
def destroy def destroy
protected_ref.destroy @protected_ref.destroy
respond_to do |format| respond_to do |format|
format.html { redirect_to_repository_settings(@project) } format.html { redirect_to_repository_settings(@project) }
......
class Projects::ProtectedTagsController < Projects::ProtectedRefsController class Projects::ProtectedTagsController < Projects::ProtectedRefsController
protected protected
def protected_ref
@protected_tag
end
def protected_ref=(val)
@protected_tag = val
end
def matching_refs=(val)
@matching_tags = val
end
def project_refs def project_refs
@project.repository.tags @project.repository.tags
end end
def create_service def create_service_class
::ProtectedTags::CreateService ::ProtectedTags::CreateService
end end
def update_service def update_service_class
::ProtectedTags::UpdateService ::ProtectedTags::UpdateService
end end
def load_protected_ref def load_protected_ref
self.protected_ref = @project.protected_tags.find(params[:id]) @protected_ref = @project.protected_tags.find(params[:id])
end end
def protected_ref_params def protected_ref_params
......
...@@ -29,4 +29,8 @@ module BranchesHelper ...@@ -29,4 +29,8 @@ module BranchesHelper
def project_branches def project_branches
options_for_select(@project.repository.branch_names, @project.default_branch) options_for_select(@project.repository.branch_names, @project.default_branch)
end end
def protected_branch?(project, branch)
ProtectedBranch.protected?(project, branch.name)
end
end end
...@@ -5,6 +5,7 @@ module ProtectedBranchAccess ...@@ -5,6 +5,7 @@ module ProtectedBranchAccess
include ProtectedRefAccess include ProtectedRefAccess
belongs_to :protected_branch belongs_to :protected_branch
delegate :project, to: :protected_branch delegate :project, to: :protected_branch
end end
end end
...@@ -3,6 +3,7 @@ module ProtectedRef ...@@ -3,6 +3,7 @@ module ProtectedRef
included do included do
belongs_to :project belongs_to :project
validates :name, presence: true validates :name, presence: true
validates :project, presence: true validates :project, presence: true
......
...@@ -5,6 +5,7 @@ module ProtectedTagAccess ...@@ -5,6 +5,7 @@ module ProtectedTagAccess
include ProtectedRefAccess include ProtectedRefAccess
belongs_to :protected_tag belongs_to :protected_tag
delegate :project, to: :protected_tag delegate :project, to: :protected_tag
end end
end end
...@@ -134,7 +134,7 @@ class Project < ActiveRecord::Base ...@@ -134,7 +134,7 @@ class Project < ActiveRecord::Base
has_many :snippets, dependent: :destroy, class_name: 'ProjectSnippet' has_many :snippets, dependent: :destroy, class_name: 'ProjectSnippet'
has_many :hooks, dependent: :destroy, class_name: 'ProjectHook' has_many :hooks, dependent: :destroy, class_name: 'ProjectHook'
has_many :protected_branches, dependent: :destroy has_many :protected_branches, dependent: :destroy
has_many :protected_tags, dependent: :destroy has_many :protected_tags, dependent: :destroy
has_many :project_authorizations has_many :project_authorizations
has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User' has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User'
......
...@@ -6,8 +6,7 @@ class ProtectableDropdown ...@@ -6,8 +6,7 @@ class ProtectableDropdown
# Tags/branches which are yet to be individually protected # Tags/branches which are yet to be individually protected
def protectable_ref_names def protectable_ref_names
non_wildcard_protections = protections.reject(&:wildcard?) @protectable_ref_names ||= ref_names - non_wildcard_protected_ref_names
refs.map(&:name) - non_wildcard_protections.map(&:name)
end end
def hash def hash
...@@ -20,7 +19,15 @@ class ProtectableDropdown ...@@ -20,7 +19,15 @@ class ProtectableDropdown
@project.repository.public_send(@ref_type) @project.repository.public_send(@ref_type)
end end
def ref_names
refs.map(&:name)
end
def protections def protections
@project.public_send("protected_#{@ref_type}") @project.public_send("protected_#{@ref_type}")
end end
def non_wildcard_protected_ref_names
protections.reject(&:wildcard?).map(&:name)
end
end end
module ProtectedBranches module ProtectedBranches
class UpdateService < BaseService class UpdateService < BaseService
attr_reader :protected_branch
def execute(protected_branch) def execute(protected_branch)
raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
@protected_branch = protected_branch protected_branch.update(params)
@protected_branch.update(params) protected_branch
@protected_branch
end end
end end
end end
module ProtectedTags module ProtectedTags
class UpdateService < BaseService class UpdateService < BaseService
attr_reader :protected_tag
def execute(protected_tag) def execute(protected_tag)
raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
@protected_tag = protected_tag protected_tag.update(params)
@protected_tag.update(params) protected_tag
@protected_tag
end end
end end
end end
...@@ -15,7 +15,7 @@ ...@@ -15,7 +15,7 @@
%span.label.label-info.has-tooltip{ title: "Merged into #{@repository.root_ref}" } %span.label.label-info.has-tooltip{ title: "Merged into #{@repository.root_ref}" }
merged merged
- if ProtectedBranch.protected?(@project, branch.name) - if protected_branch?(@project, branch)
%span.label.label-success %span.label.label-success
protected protected
.controls.hidden-xs< .controls.hidden-xs<
......
- page_title @protected_branch.name, "Protected Branches" - page_title @protected_ref.name, "Protected Branches"
.row.prepend-top-default.append-bottom-default .row.prepend-top-default.append-bottom-default
.col-lg-3 .col-lg-3
%h4.prepend-top-0 %h4.prepend-top-0
= @protected_branch.name = @protected_ref.name
.col-lg-9 .col-lg-9
%h5 Matching Branches %h5 Matching Branches
- if @matching_branches.present? - if @matching_refs.present?
.table-responsive .table-responsive
%table.table.protected-branches-list %table.table.protected-branches-list
%colgroup %colgroup
...@@ -18,7 +18,7 @@ ...@@ -18,7 +18,7 @@
%th Branch %th Branch
%th Last commit %th Last commit
%tbody %tbody
- @matching_branches.each do |matching_branch| - @matching_refs.each do |matching_branch|
= render partial: "matching_branch", object: matching_branch = render partial: "matching_branch", object: matching_branch
- else - else
%p.settings-message.text-center %p.settings-message.text-center
......
- page_title @protected_tag.name, "Protected Tags" - page_title @protected_ref.name, "Protected Tags"
.row.prepend-top-default.append-bottom-default .row.prepend-top-default.append-bottom-default
.col-lg-3 .col-lg-3
%h4.prepend-top-0 %h4.prepend-top-0
= @protected_tag.name = @protected_ref.name
.col-lg-9 .col-lg-9
%h5 Matching Tags %h5 Matching Tags
- if @matching_tags.present? - if @matching_refs.present?
.table-responsive .table-responsive
%table.table.protected-tags-list %table.table.protected-tags-list
%colgroup %colgroup
...@@ -18,7 +18,7 @@ ...@@ -18,7 +18,7 @@
%th Tag %th Tag
%th Last commit %th Last commit
%tbody %tbody
- @matching_tags.each do |matching_tag| - @matching_refs.each do |matching_tag|
= render partial: "matching_tag", object: matching_tag = render partial: "matching_tag", object: matching_tag
- else - else
%p.settings-message.text-center %p.settings-message.text-center
......
---
title: Protected Tags feature
merge_request: 10356
author:
class CreateProtectedTags < ActiveRecord::Migration class CreateProtectedTags < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers include Gitlab::Database::MigrationHelpers
# Set this constant to true if this migration requires downtime.
DOWNTIME = false DOWNTIME = false
GITLAB_ACCESS_MASTER = 40 GITLAB_ACCESS_MASTER = 40
......
...@@ -70,14 +70,8 @@ module Gitlab ...@@ -70,14 +70,8 @@ module Gitlab
def protected_tag_checks def protected_tag_checks
return unless tag_protected? return unless tag_protected?
return "Protected tags cannot be updated." if update?
if update? return "Protected tags cannot be deleted." if deletion?
return "Protected tags cannot be updated."
end
if deletion?
return "Protected tags cannot be deleted."
end
unless user_access.can_create_tag?(@tag_name) unless user_access.can_create_tag?(@tag_name)
return "You are not allowed to create this tag as it is protected." return "You are not allowed to create this tag as it is protected."
......
...@@ -3,6 +3,7 @@ require('spec_helper') ...@@ -3,6 +3,7 @@ require('spec_helper')
describe Projects::ProtectedBranchesController do describe Projects::ProtectedBranchesController do
describe "GET #index" do describe "GET #index" do
let(:project) { create(:project_empty_repo, :public) } let(:project) { create(:project_empty_repo, :public) }
it "redirects empty repo to projects page" do it "redirects empty repo to projects page" do
get(:index, namespace_id: project.namespace.to_param, project_id: project) get(:index, namespace_id: project.namespace.to_param, project_id: project)
end end
......
...@@ -3,6 +3,7 @@ require('spec_helper') ...@@ -3,6 +3,7 @@ require('spec_helper')
describe Projects::ProtectedTagsController do describe Projects::ProtectedTagsController do
describe "GET #index" do describe "GET #index" do
let(:project) { create(:project_empty_repo, :public) } let(:project) { create(:project_empty_repo, :public) }
it "redirects empty repo to projects page" do it "redirects empty repo to projects page" do
get(:index, namespace_id: project.namespace.to_param, project_id: project) get(:index, namespace_id: project.namespace.to_param, project_id: project)
end end
......
...@@ -2,7 +2,9 @@ RSpec.shared_examples "protected branches > access control > CE" do ...@@ -2,7 +2,9 @@ RSpec.shared_examples "protected branches > access control > CE" do
ProtectedBranch::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| ProtectedBranch::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
it "allows creating protected branches that #{access_type_name} can push to" do it "allows creating protected branches that #{access_type_name} can push to" do
visit namespace_project_protected_branches_path(project.namespace, project) visit namespace_project_protected_branches_path(project.namespace, project)
set_protected_branch_name('master') set_protected_branch_name('master')
within('.new_protected_branch') do within('.new_protected_branch') do
allowed_to_push_button = find(".js-allowed-to-push") allowed_to_push_button = find(".js-allowed-to-push")
...@@ -11,6 +13,7 @@ RSpec.shared_examples "protected branches > access control > CE" do ...@@ -11,6 +13,7 @@ RSpec.shared_examples "protected branches > access control > CE" do
within(".dropdown.open .dropdown-menu") { click_on access_type_name } within(".dropdown.open .dropdown-menu") { click_on access_type_name }
end end
end end
click_on "Protect" click_on "Protect"
expect(ProtectedBranch.count).to eq(1) expect(ProtectedBranch.count).to eq(1)
...@@ -19,7 +22,9 @@ RSpec.shared_examples "protected branches > access control > CE" do ...@@ -19,7 +22,9 @@ RSpec.shared_examples "protected branches > access control > CE" do
it "allows updating protected branches so that #{access_type_name} can push to them" do it "allows updating protected branches so that #{access_type_name} can push to them" do
visit namespace_project_protected_branches_path(project.namespace, project) visit namespace_project_protected_branches_path(project.namespace, project)
set_protected_branch_name('master') set_protected_branch_name('master')
click_on "Protect" click_on "Protect"
expect(ProtectedBranch.count).to eq(1) expect(ProtectedBranch.count).to eq(1)
...@@ -34,6 +39,7 @@ RSpec.shared_examples "protected branches > access control > CE" do ...@@ -34,6 +39,7 @@ RSpec.shared_examples "protected branches > access control > CE" do
end end
wait_for_ajax wait_for_ajax
expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(access_type_id) expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(access_type_id)
end end
end end
...@@ -41,7 +47,9 @@ RSpec.shared_examples "protected branches > access control > CE" do ...@@ -41,7 +47,9 @@ RSpec.shared_examples "protected branches > access control > CE" do
ProtectedBranch::MergeAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| ProtectedBranch::MergeAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
it "allows creating protected branches that #{access_type_name} can merge to" do it "allows creating protected branches that #{access_type_name} can merge to" do
visit namespace_project_protected_branches_path(project.namespace, project) visit namespace_project_protected_branches_path(project.namespace, project)
set_protected_branch_name('master') set_protected_branch_name('master')
within('.new_protected_branch') do within('.new_protected_branch') do
allowed_to_merge_button = find(".js-allowed-to-merge") allowed_to_merge_button = find(".js-allowed-to-merge")
...@@ -50,6 +58,7 @@ RSpec.shared_examples "protected branches > access control > CE" do ...@@ -50,6 +58,7 @@ RSpec.shared_examples "protected branches > access control > CE" do
within(".dropdown.open .dropdown-menu") { click_on access_type_name } within(".dropdown.open .dropdown-menu") { click_on access_type_name }
end end
end end
click_on "Protect" click_on "Protect"
expect(ProtectedBranch.count).to eq(1) expect(ProtectedBranch.count).to eq(1)
...@@ -58,7 +67,9 @@ RSpec.shared_examples "protected branches > access control > CE" do ...@@ -58,7 +67,9 @@ RSpec.shared_examples "protected branches > access control > CE" do
it "allows updating protected branches so that #{access_type_name} can merge to them" do it "allows updating protected branches so that #{access_type_name} can merge to them" do
visit namespace_project_protected_branches_path(project.namespace, project) visit namespace_project_protected_branches_path(project.namespace, project)
set_protected_branch_name('master') set_protected_branch_name('master')
click_on "Protect" click_on "Protect"
expect(ProtectedBranch.count).to eq(1) expect(ProtectedBranch.count).to eq(1)
...@@ -73,6 +84,7 @@ RSpec.shared_examples "protected branches > access control > CE" do ...@@ -73,6 +84,7 @@ RSpec.shared_examples "protected branches > access control > CE" do
end end
wait_for_ajax wait_for_ajax
expect(ProtectedBranch.last.merge_access_levels.map(&:access_level)).to include(access_type_id) expect(ProtectedBranch.last.merge_access_levels.map(&:access_level)).to include(access_type_id)
end end
end end
......
...@@ -2,7 +2,9 @@ RSpec.shared_examples "protected tags > access control > CE" do ...@@ -2,7 +2,9 @@ RSpec.shared_examples "protected tags > access control > CE" do
ProtectedTag::CreateAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)| ProtectedTag::CreateAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
it "allows creating protected tags that #{access_type_name} can create" do it "allows creating protected tags that #{access_type_name} can create" do
visit namespace_project_protected_tags_path(project.namespace, project) visit namespace_project_protected_tags_path(project.namespace, project)
set_protected_tag_name('master') set_protected_tag_name('master')
within('.new_protected_tag') do within('.new_protected_tag') do
allowed_to_create_button = find(".js-allowed-to-create") allowed_to_create_button = find(".js-allowed-to-create")
...@@ -11,6 +13,7 @@ RSpec.shared_examples "protected tags > access control > CE" do ...@@ -11,6 +13,7 @@ RSpec.shared_examples "protected tags > access control > CE" do
within(".dropdown.open .dropdown-menu") { click_on access_type_name } within(".dropdown.open .dropdown-menu") { click_on access_type_name }
end end
end end
click_on "Protect" click_on "Protect"
expect(ProtectedTag.count).to eq(1) expect(ProtectedTag.count).to eq(1)
...@@ -19,7 +22,9 @@ RSpec.shared_examples "protected tags > access control > CE" do ...@@ -19,7 +22,9 @@ RSpec.shared_examples "protected tags > access control > CE" do
it "allows updating protected tags so that #{access_type_name} can create them" do it "allows updating protected tags so that #{access_type_name} can create them" do
visit namespace_project_protected_tags_path(project.namespace, project) visit namespace_project_protected_tags_path(project.namespace, project)
set_protected_tag_name('master') set_protected_tag_name('master')
click_on "Protect" click_on "Protect"
expect(ProtectedTag.count).to eq(1) expect(ProtectedTag.count).to eq(1)
...@@ -34,6 +39,7 @@ RSpec.shared_examples "protected tags > access control > CE" do ...@@ -34,6 +39,7 @@ RSpec.shared_examples "protected tags > access control > CE" do
end end
wait_for_ajax wait_for_ajax
expect(ProtectedTag.last.create_access_levels.map(&:access_level)).to include(access_type_id) expect(ProtectedTag.last.create_access_levels.map(&:access_level)).to include(access_type_id)
end end
end end
......
...@@ -18,6 +18,7 @@ describe ProtectableDropdown, models: true do ...@@ -18,6 +18,7 @@ describe ProtectableDropdown, models: true do
create(:protected_branch, name: 'feat*', project: project) create(:protected_branch, name: 'feat*', project: project)
subject = described_class.new(project.reload, :branches) subject = described_class.new(project.reload, :branches)
expect(subject.protectable_ref_names).to include('feature') expect(subject.protectable_ref_names).to include('feature')
end end
end end
......
require 'spec_helper' require 'spec_helper'
describe ProtectedTag, models: true do describe ProtectedTag, models: true do
subject { build_stubbed(:protected_branch) }
describe 'Associations' do describe 'Associations' do
it { is_expected.to belong_to(:project) } it { is_expected.to belong_to(:project) }
end end
......
require 'spec_helper'
describe ProtectedBranches::UpdateService, services: true do
let(:protected_branch) { create(:protected_branch) }
let(:project) { protected_branch.project }
let(:user) { project.owner }
let(:params) { { name: 'new protected branch name' } }
describe '#execute' do
subject(:service) { described_class.new(project, user, params) }
it 'updates a protected branch' do
result = service.execute(protected_branch)
expect(result.reload.name).to eq(params[:name])
end
context 'without admin_project permissions' do
let(:user) { create(:user) }
it "raises error" do
expect{ service.execute(protected_branch) }.to raise_error(Gitlab::Access::AccessDeniedError)
end
end
end
end
require 'spec_helper'
describe ProtectedTags::UpdateService, services: true do
let(:protected_tag) { create(:protected_tag) }
let(:project) { protected_tag.project }
let(:user) { project.owner }
let(:params) { { name: 'new protected tag name' } }
describe '#execute' do
subject(:service) { described_class.new(project, user, params) }
it 'updates a protected tag' do
result = service.execute(protected_tag)
expect(result.reload.name).to eq(params[:name])
end
context 'without admin_project permissions' do
let(:user) { create(:user) }
it "raises error" do
expect{ service.execute(protected_tag) }.to raise_error(Gitlab::Access::AccessDeniedError)
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