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

Add confirm delete protected branch modal

Browse source
This commit is contained in:
Sam Rose 2017-05-08 07:41:58 +00:00 committed by Phil Hughes
parent 1a5e84febe
commit 1ebd9dad8e
21 changed files with 383 additions and 150 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

36
app/assets/javascripts/branches/branches_delete_modal.js Normal file
View file

@ -0,0 +1,36 @@
const MODAL_SELECTOR = '#modal-delete-branch';
class DeleteModal {
constructor() {
this.$modal = $(MODAL_SELECTOR);
this.$toggleBtns = $(`[data-target="${MODAL_SELECTOR}"]`);
this.$branchName = $('.js-branch-name', this.$modal);
this.$confirmInput = $('.js-delete-branch-input', this.$modal);
this.$deleteBtn = $('.js-delete-branch', this.$modal);
this.bindEvents();
}
bindEvents() {
this.$toggleBtns.on('click', this.setModalData.bind(this));
this.$confirmInput.on('input', this.setDeleteDisabled.bind(this));
}
setModalData(e) {
this.branchName = e.currentTarget.dataset.branchName || '';
this.deletePath = e.currentTarget.dataset.deletePath || '';
this.updateModal();
}
setDeleteDisabled(e) {
this.$deleteBtn.attr('disabled', e.currentTarget.value !== this.branchName);
}
updateModal() {
this.$branchName.text(this.branchName);
this.$confirmInput.val('');
this.$deleteBtn.attr('href', this.deletePath);
this.$deleteBtn.attr('disabled', true);
}
}
export default DeleteModal;

2
app/assets/javascripts/dispatcher.js
View file

@ -38,6 +38,7 @@
import Issue from './issue';
import BindInOut from './behaviors/bind_in_out';
import DeleteModal from './branches/branches_delete_modal';
import Group from './group';
import GroupName from './group_name';
import GroupsList from './groups_list';
@ -180,6 +181,7 @@ const ShortcutsBlob = require('./shortcuts_blob');
break;
case 'projects:branches:index':
gl.AjaxLoadingSpinner.init();
new DeleteModal();
break;
case 'projects:issues:new':
case 'projects:issues:edit':

1
app/assets/stylesheets/framework/lists.scss
View file

@ -152,6 +152,7 @@ ul.content-list {
margin-top: 3px;
margin-bottom: 4px;
&.has-tooltip,
&:last-child {
margin-right: 0;

12
app/controllers/projects/branches_controller.rb
View file

@ -73,13 +73,17 @@ class Projects::BranchesController < Projects::ApplicationController
def destroy
@branch_name = Addressable::URI.unescape(params[:id])
status = DeleteBranchService.new(project, current_user).execute(@branch_name)
result = DeleteBranchService.new(project, current_user).execute(@branch_name)
respond_to do |format|
format.html do
redirect_to namespace_project_branches_path(@project.namespace,
@project), status: 303
flash_type = result[:status] == :error ? :alert : :notice
flash[flash_type] = result[:message]
redirect_to namespace_project_branches_path(@project.namespace, @project), status: 303
end
format.js { render nothing: true, status: status[:return_code] }
format.js { render nothing: true, status: result[:return_code] }
end
end

4
app/controllers/projects/tags_controller.rb
View file

@ -48,7 +48,7 @@ class Projects::TagsController < Projects::ApplicationController
respond_to do |format|
if result[:status] == :success
format.html do
redirect_to namespace_project_tags_path(@project.namespace, @project)
redirect_to namespace_project_tags_path(@project.namespace, @project), status: 303
end
format.js
@ -57,7 +57,7 @@ class Projects::TagsController < Projects::ApplicationController
format.html do
redirect_to namespace_project_tags_path(@project.namespace, @project),
alert: @error
alert: @error, status: 303
end
format.js do

10
app/helpers/branches_helper.rb
View file

@ -1,14 +1,4 @@
module BranchesHelper
def can_remove_branch?(project, branch_name)
if ProtectedBranch.protected?(project, branch_name)
false
elsif branch_name == project.repository.root_ref
false
else
can?(current_user, :push_code, project)
end
end
def filter_branches_path(options = {})
exist_opts = {
search: params[:search],

6
app/policies/project_policy.rb
View file

@ -98,7 +98,7 @@ class ProjectPolicy < BasePolicy
end
def master_access!
can! :push_code_to_protected_branches
can! :delete_protected_branch
can! :update_project_snippet
can! :update_environment
can! :update_deployment
@ -173,7 +173,7 @@ class ProjectPolicy < BasePolicy
def archived_access!
cannot! :create_merge_request
cannot! :push_code
cannot! :push_code_to_protected_branches
cannot! :delete_protected_branch
cannot! :update_merge_request
cannot! :admin_merge_request
end
@ -211,7 +211,7 @@ class ProjectPolicy < BasePolicy
unless repository_enabled
cannot! :push_code
cannot! :push_code_to_protected_branches
cannot! :delete_protected_branch
cannot! :download_code
cannot! :fork_project
cannot! :read_commit_status

16
app/services/delete_branch_service.rb
View file

@ -3,22 +3,14 @@ class DeleteBranchService < BaseService
repository = project.repository
branch = repository.find_branch(branch_name)
unless branch
return error('No such branch', 404)
end
if branch_name == repository.root_ref
return error('Cannot remove HEAD branch', 405)
end
if ProtectedBranch.protected?(project, branch_name)
return error('Protected branch cant be removed', 405)
end
unless current_user.can?(:push_code, project)
return error('You dont have push access to repo', 405)
end
unless branch
return error('No such branch', 404)
end
if repository.rm_branch(current_user, branch_name)
success('Branch was removed')
else

35
app/views/projects/branches/_branch.html.haml
View file

@ -30,13 +30,34 @@
= render 'projects/buttons/download', project: @project, ref: branch.name, pipeline: @refs_pipelines[branch.name]
- if can?(current_user, :push_code, @project)
= link_to namespace_project_branch_path(@project.namespace, @project, branch.name),
class: "btn btn-remove remove-row js-ajax-loading-spinner #{can_remove_branch?(@project, branch.name) ? '' : 'disabled'}",
method: :delete,
data: { confirm: "Deleting the '#{branch.name}' branch cannot be undone. Are you sure?" },
remote: true,
"aria-label" => "Delete branch" do
= icon("trash-o")
- if branch.name == @project.repository.root_ref
%button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip disabled",
disabled: true,
title: "The default branch cannot be deleted" }
= icon("trash-o")
- elsif protected_branch?(@project, branch)
- if can?(current_user, :delete_protected_branch, @project)
%button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip",
title: "Delete protected branch",
data: { toggle: "modal",
target: "#modal-delete-branch",
delete_path: namespace_project_branch_path(@project.namespace, @project, branch.name),
branch_name: branch.name } }
= icon("trash-o")
- else
%button{ class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip disabled",
disabled: true,
title: "Only a project master or owner can delete a protected branch" }
= icon("trash-o")
- else
= link_to namespace_project_branch_path(@project.namespace, @project, branch.name),
class: "btn btn-remove remove-row js-ajax-loading-spinner has-tooltip",
title: "Delete branch",
method: :delete,
data: { confirm: "Deleting the '#{branch.name}' branch cannot be undone. Are you sure?" },
remote: true,
"aria-label" => "Delete branch" do
= icon("trash-o")
- if branch.name != @repository.root_ref
.divergence-graph{ title: "#{number_commits_ahead} commits ahead, #{number_commits_behind} commits behind #{@repository.root_ref}" }

34
app/views/projects/branches/_delete_protected_modal.html.haml Normal file
View file

@ -0,0 +1,34 @@
#modal-delete-branch.modal{ tabindex: -1 }
.modal-dialog
.modal-content
.modal-header
%button.close{ data: { dismiss: 'modal' } } ×
%h3.page-title
Delete protected branch
= surround "'", "'?" do
%span.js-branch-name>[branch name]
.modal-body
%p
You’re about to permanently delete the protected branch
= succeed '.' do
%strong.js-branch-name [branch name]
%p
Once you confirm and press
= succeed ',' do
%strong Delete protected branch
it cannot be undone or recovered.
%p
%strong To confirm, type
%kbd.js-branch-name [branch name]
.form-group
= text_field_tag 'delete_branch_input', '', class: 'form-control js-delete-branch-input'
.modal-footer
%button.btn{ data: { dismiss: 'modal' } } Cancel
= link_to 'Delete protected branch', '',
class: "btn btn-danger js-delete-branch",
title: 'Delete branch',
method: :delete,
"aria-label" => "Delete"

2
app/views/projects/branches/index.html.haml
View file

@ -37,3 +37,5 @@
= paginate @branches, theme: 'gitlab'
- else
.nothing-here-block No branches to show
= render 'projects/branches/delete_protected_modal'

68
lib/gitlab/checks/change_access.rb
View file

@ -1,7 +1,6 @@
module Gitlab
module Checks
class ChangeAccess
# protocol is currently used only in EE
attr_reader :user_access, :project, :skip_authorization, :protocol
def initialize(
@ -18,7 +17,9 @@ module Gitlab
end
def exec
error = push_checks || tag_checks || protected_branch_checks
return GitAccessStatus.new(true) if skip_authorization
error = push_checks || branch_checks || tag_checks
if error
GitAccessStatus.new(false, error)
@ -29,35 +30,59 @@ module Gitlab
protected
def protected_branch_checks
return if skip_authorization
def push_checks
if user_access.cannot_do_action?(:push_code)
"You are not allowed to push code to this project."
end
end
def branch_checks
return unless @branch_name
if deletion? && @branch_name == project.default_branch
return "The default branch of a project cannot be deleted."
end
protected_branch_checks
end
def protected_branch_checks
return unless ProtectedBranch.protected?(project, @branch_name)
if forced_push?
return "You are not allowed to force push code to a protected branch on this project."
elsif deletion?
return "You are not allowed to delete protected branches from this project."
end
if deletion?
protected_branch_deletion_checks
else
protected_branch_push_checks
end
end
def protected_branch_deletion_checks
unless user_access.can_delete_branch?(@branch_name)
return 'You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.'
end
unless protocol == 'web'
'You can only delete protected branches using the web interface.'
end
end
def protected_branch_push_checks
if matching_merge_request?
if user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name)
return
else
unless user_access.can_merge_to_branch?(@branch_name) || user_access.can_push_to_branch?(@branch_name)
"You are not allowed to merge code into protected branches on this project."
end
else
if user_access.can_push_to_branch?(@branch_name)
return
else
unless user_access.can_push_to_branch?(@branch_name)
"You are not allowed to push code to protected branches on this project."
end
end
end
def tag_checks
return if skip_authorization
return unless @tag_name
if tag_exists? && user_access.cannot_do_action?(:admin_project)
@ -68,7 +93,8 @@ module Gitlab
end
def protected_tag_checks
return unless tag_protected?
return unless ProtectedTag.protected?(project, @tag_name)
return "Protected tags cannot be updated." if update?
return "Protected tags cannot be deleted." if deletion?
@ -77,18 +103,6 @@ module Gitlab
end
end
def tag_protected?
ProtectedTag.protected?(project, @tag_name)
end
def push_checks
return if skip_authorization
if user_access.cannot_do_action?(:push_code)
"You are not allowed to push code to this project."
end
end
private
def tag_exists?

10
lib/gitlab/user_access.rb
View file

@ -38,6 +38,16 @@ module Gitlab
end
end
def can_delete_branch?(ref)
return false unless can_access_git?
if ProtectedBranch.protected?(project, ref)
user.can?(:delete_protected_branch, project)
else
user.can?(:push_code, project)
end
end
def can_push_to_branch?(ref)
return false unless can_access_git?

85
spec/features/projects/branches_spec.rb
View file

@ -4,7 +4,13 @@ describe 'Branches', feature: true do
let(:project) { create(:project, :public) }
let(:repository) { project.repository }
context 'logged in' do
def set_protected_branch_name(branch_name)
find(".js-protected-branch-select").click
find(".dropdown-input-field").set(branch_name)
click_on("Create wildcard #{branch_name}")
end
context 'logged in as developer' do
before do
login_as :user
project.team << [@user, :developer]
@ -38,6 +44,83 @@ describe 'Branches', feature: true do
expect(find('.all-branches')).to have_selector('li', count: 1)
end
end
describe 'Delete unprotected branch' do
it 'removes branch after confirmation', js: true do
visit namespace_project_branches_path(project.namespace, project)
fill_in 'branch-search', with: 'fix'
find('#branch-search').native.send_keys(:enter)
expect(page).to have_content('fix')
expect(find('.all-branches')).to have_selector('li', count: 1)
find('.js-branch-fix .btn-remove').trigger(:click)
expect(page).not_to have_content('fix')
expect(find('.all-branches')).to have_selector('li', count: 0)
end
end
describe 'Delete protected branch' do
before do
project.add_user(@user, :master)
visit namespace_project_protected_branches_path(project.namespace, project)
set_protected_branch_name('fix')
click_on "Protect"
within(".protected-branches-list") { expect(page).to have_content('fix') }
expect(ProtectedBranch.count).to eq(1)
project.add_user(@user, :developer)
end
it 'does not allow devleoper to removes protected branch', js: true do
visit namespace_project_branches_path(project.namespace, project)
fill_in 'branch-search', with: 'fix'
find('#branch-search').native.send_keys(:enter)
expect(page).to have_css('.btn-remove.disabled')
end
end
end
context 'logged in as master' do
before do
login_as :user
project.team << [@user, :master]
end
describe 'Delete protected branch' do
before do
visit namespace_project_protected_branches_path(project.namespace, project)
set_protected_branch_name('fix')
click_on "Protect"
within(".protected-branches-list") { expect(page).to have_content('fix') }
expect(ProtectedBranch.count).to eq(1)
end
it 'removes branch after modal confirmation', js: true do
visit namespace_project_branches_path(project.namespace, project)
fill_in 'branch-search', with: 'fix'
find('#branch-search').native.send_keys(:enter)
expect(page).to have_content('fix')
expect(find('.all-branches')).to have_selector('li', count: 1)
page.find('[data-target="#modal-delete-branch"]').trigger(:click)
expect(page).to have_css('.js-delete-branch[disabled]')
fill_in 'delete_branch_input', with: 'fix'
click_link 'Delete protected branch'
fill_in 'branch-search', with: 'fix'
find('#branch-search').native.send_keys(:enter)
expect(page).to have_content('No branches to show')
end
end
end
context 'logged out' do

99
spec/lib/gitlab/checks/change_access_spec.rb
View file

@ -96,40 +96,77 @@ describe Gitlab::Checks::ChangeAccess, lib: true do
end
end
context 'protected branches check' do
before do
allow(ProtectedBranch).to receive(:protected?).with(project, 'master').and_return(true)
end
it 'returns an error if the user is not allowed to do forced pushes to protected branches' do
expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
expect(subject.status).to be(false)
expect(subject.message).to eq('You are not allowed to force push code to a protected branch on this project.')
end
it 'returns an error if the user is not allowed to merge to protected branches' do
expect_any_instance_of(Gitlab::Checks::MatchingMergeRequest).to receive(:match?).and_return(true)
expect(user_access).to receive(:can_merge_to_branch?).and_return(false)
expect(user_access).to receive(:can_push_to_branch?).and_return(false)
expect(subject.status).to be(false)
expect(subject.message).to eq('You are not allowed to merge code into protected branches on this project.')
end
it 'returns an error if the user is not allowed to push to protected branches' do
expect(user_access).to receive(:can_push_to_branch?).and_return(false)
expect(subject.status).to be(false)
expect(subject.message).to eq('You are not allowed to push code to protected branches on this project.')
end
context 'branch deletion' do
context 'branches check' do
context 'trying to delete the default branch' do
let(:newrev) { '0000000000000000000000000000000000000000' }
let(:ref) { 'refs/heads/master' }
it 'returns an error if the user is not allowed to delete protected branches' do
it 'returns an error' do
expect(subject.status).to be(false)
expect(subject.message).to eq('You are not allowed to delete protected branches from this project.')
expect(subject.message).to eq('The default branch of a project cannot be deleted.')
end
end
context 'protected branches check' do
before do
allow(ProtectedBranch).to receive(:protected?).with(project, 'master').and_return(true)
allow(ProtectedBranch).to receive(:protected?).with(project, 'feature').and_return(true)
end
it 'returns an error if the user is not allowed to do forced pushes to protected branches' do
expect(Gitlab::Checks::ForcePush).to receive(:force_push?).and_return(true)
expect(subject.status).to be(false)
expect(subject.message).to eq('You are not allowed to force push code to a protected branch on this project.')
end
it 'returns an error if the user is not allowed to merge to protected branches' do
expect_any_instance_of(Gitlab::Checks::MatchingMergeRequest).to receive(:match?).and_return(true)
expect(user_access).to receive(:can_merge_to_branch?).and_return(false)
expect(user_access).to receive(:can_push_to_branch?).and_return(false)
expect(subject.status).to be(false)
expect(subject.message).to eq('You are not allowed to merge code into protected branches on this project.')
end
it 'returns an error if the user is not allowed to push to protected branches' do
expect(user_access).to receive(:can_push_to_branch?).and_return(false)
expect(subject.status).to be(false)
expect(subject.message).to eq('You are not allowed to push code to protected branches on this project.')
end
context 'branch deletion' do
let(:newrev) { '0000000000000000000000000000000000000000' }
let(:ref) { 'refs/heads/feature' }
context 'if the user is not allowed to delete protected branches' do
it 'returns an error' do
expect(subject.status).to be(false)
expect(subject.message).to eq('You are not allowed to delete protected branches from this project. Only a project master or owner can delete a protected branch.')
end
end
context 'if the user is allowed to delete protected branches' do
before do
project.add_master(user)
end
context 'through the web interface' do
let(:protocol) { 'web' }
it 'allows branch deletion' do
expect(subject.status).to be(true)
end
end
context 'over SSH or HTTP' do
it 'returns an error' do
expect(subject.status).to be(false)
expect(subject.message).to eq('You can only delete protected branches using the web interface.')
end
end
end
end
end
end

2
spec/lib/gitlab/git_access_spec.rb
View file

@ -1,7 +1,7 @@
require 'spec_helper'
describe Gitlab::GitAccess, lib: true do
let(:access) { Gitlab::GitAccess.new(actor, project, 'web', authentication_abilities: authentication_abilities) }
let(:access) { Gitlab::GitAccess.new(actor, project, 'ssh', authentication_abilities: authentication_abilities) }
let(:project) { create(:project, :repository) }
let(:user) { create(:user) }
let(:actor) { user }

48
spec/lib/gitlab/user_access_spec.rb
View file

@ -5,7 +5,7 @@ describe Gitlab::UserAccess, lib: true do
let(:project) { create(:project) }
let(:user) { create(:user) }
describe 'can_push_to_branch?' do
describe '#can_push_to_branch?' do
describe 'push to none protected branch' do
it 'returns true if user is a master' do
project.team << [user, :master]
@ -143,7 +143,7 @@ describe Gitlab::UserAccess, lib: true do
end
end
describe 'can_create_tag?' do
describe '#can_create_tag?' do
describe 'push to none protected tag' do
it 'returns true if user is a master' do
project.add_user(user, :master)
@ -211,4 +211,48 @@ describe Gitlab::UserAccess, lib: true do
end
end
end
describe '#can_delete_branch?' do
describe 'delete unprotected branch' do
it 'returns true if user is a master' do
project.add_user(user, :master)
expect(access.can_delete_branch?('random_branch')).to be_truthy
end
it 'returns true if user is a developer' do
project.add_user(user, :developer)
expect(access.can_delete_branch?('random_branch')).to be_truthy
end
it 'returns false if user is a reporter' do
project.add_user(user, :reporter)
expect(access.can_delete_branch?('random_branch')).to be_falsey
end
end
describe 'delete protected branch' do
let(:branch) { create(:protected_branch, project: project, name: "test") }
it 'returns true if user is a master' do
project.add_user(user, :master)
expect(access.can_delete_branch?(branch.name)).to be_truthy
end
it 'returns false if user is a developer' do
project.add_user(user, :developer)
expect(access.can_delete_branch?(branch.name)).to be_falsey
end
it 'returns false if user is a reporter' do
project.add_user(user, :reporter)
expect(access.can_delete_branch?(branch.name)).to be_falsey
end
end
end
end

2
spec/policies/project_policy_spec.rb
View file

@ -43,7 +43,7 @@ describe ProjectPolicy, models: true do
let(:master_permissions) do
%i[
push_code_to_protected_branches update_project_snippet update_environment
delete_protected_branch update_project_snippet update_environment
update_deployment admin_milestone admin_project_snippet
admin_project_member admin_note admin_wiki admin_project
admin_commit_status admin_build admin_container_image

13
spec/requests/api/branches_spec.rb
View file

@ -406,19 +406,6 @@ describe API::Branches do
delete api("/projects/#{project.id}/repository/branches/foobar", user)
expect(response).to have_http_status(404)
end
it "removes protected branch" do
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')
end
it "does not remove HEAD branch" do
delete api("/projects/#{project.id}/repository/branches/master", user)
expect(response).to have_http_status(405)
expect(json_response['message']).to eq('Cannot remove HEAD branch')
end
end
describe "DELETE /projects/:id/repository/merged_branches" do

13
spec/requests/api/v3/branches_spec.rb
View file

@ -47,19 +47,6 @@ describe API::V3::Branches do
delete v3_api("/projects/#{project.id}/repository/branches/foobar", user)
expect(response).to have_http_status(404)
end
it "removes protected branch" do
create(:protected_branch, project: project, name: branch_name)
delete v3_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')
end
it "does not remove HEAD branch" do
delete v3_api("/projects/#{project.id}/repository/branches/master", user)
expect(response).to have_http_status(405)
expect(json_response['message']).to eq('Cannot remove HEAD branch')
end
end
describe "DELETE /projects/:id/repository/merged_branches" do

35
spec/services/delete_merged_branches_service_spec.rb
View file

@ -6,33 +6,22 @@ describe DeleteMergedBranchesService, services: true do
let(:project) { create(:project, :repository) }
context '#execute' do
context 'unprotected branches' do
before do
service.execute
end
it 'deletes a branch that was merged' do
service.execute
it 'deletes a branch that was merged' do
expect(project.repository.branch_names).not_to include('improve/awesome')
end
it 'keeps branch that is unmerged' do
expect(project.repository.branch_names).to include('feature')
end
it 'keeps "master"' do
expect(project.repository.branch_names).to include('master')
end
expect(project.repository.branch_names).not_to include('improve/awesome')
end
context 'protected branches' do
before do
create(:protected_branch, name: 'improve/awesome', project: project)
service.execute
end
it 'keeps branch that is unmerged' do
service.execute
it 'keeps protected branch' do
expect(project.repository.branch_names).to include('improve/awesome')
end
expect(project.repository.branch_names).to include('feature')
end
it 'keeps "master"' do
service.execute
expect(project.repository.branch_names).to include('master')
end
context 'user without rights' do