From f16377e7dc762462817dd0b34e36811c55988b10 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Wed, 5 Apr 2017 18:59:46 +0100 Subject: Protected Tags backend review changes Added changelog --- .../projects/protected_branches_controller.rb | 18 +++--------------- .../projects/protected_refs_controller.rb | 21 ++++++++++----------- .../projects/protected_tags_controller.rb | 18 +++--------------- app/helpers/branches_helper.rb | 4 ++++ app/models/concerns/protected_branch_access.rb | 1 + app/models/concerns/protected_ref.rb | 1 + app/models/concerns/protected_tag_access.rb | 1 + app/models/project.rb | 2 +- app/models/protectable_dropdown.rb | 11 +++++++++-- app/services/protected_branches/update_service.rb | 7 ++----- app/services/protected_tags/update_service.rb | 7 ++----- app/views/projects/branches/_branch.html.haml | 2 +- .../projects/protected_branches/show.html.haml | 8 ++++---- app/views/projects/protected_tags/show.html.haml | 8 ++++---- 14 files changed, 46 insertions(+), 63 deletions(-) (limited to 'app') diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb index c2a55c9500a..ba24fa9acfe 100644 --- a/app/controllers/projects/protected_branches_controller.rb +++ b/app/controllers/projects/protected_branches_controller.rb @@ -1,32 +1,20 @@ class Projects::ProtectedBranchesController < Projects::ProtectedRefsController 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 @project.repository.branches end - def create_service + def create_service_class ::ProtectedBranches::CreateService end - def update_service + def update_service_class ::ProtectedBranches::UpdateService end def load_protected_ref - self.protected_ref = @project.protected_branches.find(params[:id]) + @protected_ref = @project.protected_branches.find(params[:id]) end def protected_ref_params diff --git a/app/controllers/projects/protected_refs_controller.rb b/app/controllers/projects/protected_refs_controller.rb index 63f005124a9..083a70968e5 100644 --- a/app/controllers/projects/protected_refs_controller.rb +++ b/app/controllers/projects/protected_refs_controller.rb @@ -1,5 +1,6 @@ class Projects::ProtectedRefsController < Projects::ApplicationController include RepositorySettingsRedirect + # Authorize before_action :require_non_empty_project before_action :authorize_admin_project! @@ -12,33 +13,31 @@ class Projects::ProtectedRefsController < Projects::ApplicationController end 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? flash[:alert] = protected_ref.errors.full_messages.join(', ').html_safe end + redirect_to_repository_settings(@project) end def show - self.matching_refs = protected_ref.matching(project_refs) + @matching_refs = @protected_ref.matching(project_refs) end 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? - respond_to do |format| - format.json { render json: protected_ref, status: :ok } - end + if @protected_ref.valid? + render json: @protected_ref, status: :ok else - respond_to do |format| - format.json { render json: protected_ref.errors, status: :unprocessable_entity } - end + render json: @protected_ref.errors, status: :unprocessable_entity end end def destroy - protected_ref.destroy + @protected_ref.destroy respond_to do |format| format.html { redirect_to_repository_settings(@project) } diff --git a/app/controllers/projects/protected_tags_controller.rb b/app/controllers/projects/protected_tags_controller.rb index 0e00baedbdf..c61ddf145e6 100644 --- a/app/controllers/projects/protected_tags_controller.rb +++ b/app/controllers/projects/protected_tags_controller.rb @@ -1,32 +1,20 @@ class Projects::ProtectedTagsController < Projects::ProtectedRefsController 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 @project.repository.tags end - def create_service + def create_service_class ::ProtectedTags::CreateService end - def update_service + def update_service_class ::ProtectedTags::UpdateService end def load_protected_ref - self.protected_ref = @project.protected_tags.find(params[:id]) + @protected_ref = @project.protected_tags.find(params[:id]) end def protected_ref_params diff --git a/app/helpers/branches_helper.rb b/app/helpers/branches_helper.rb index a852b90c57e..b7a28b1b4a7 100644 --- a/app/helpers/branches_helper.rb +++ b/app/helpers/branches_helper.rb @@ -29,4 +29,8 @@ module BranchesHelper def project_branches options_for_select(@project.repository.branch_names, @project.default_branch) end + + def protected_branch?(project, branch) + ProtectedBranch.protected?(project, branch.name) + end end diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb index 06cae00249a..c41b807df8a 100644 --- a/app/models/concerns/protected_branch_access.rb +++ b/app/models/concerns/protected_branch_access.rb @@ -5,6 +5,7 @@ module ProtectedBranchAccess include ProtectedRefAccess belongs_to :protected_branch + delegate :project, to: :protected_branch end end diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb index 7c0183952a0..62eaec2407f 100644 --- a/app/models/concerns/protected_ref.rb +++ b/app/models/concerns/protected_ref.rb @@ -3,6 +3,7 @@ module ProtectedRef included do belongs_to :project + validates :name, presence: true validates :project, presence: true diff --git a/app/models/concerns/protected_tag_access.rb b/app/models/concerns/protected_tag_access.rb index 9b7d31a6fd5..ee65de24dd8 100644 --- a/app/models/concerns/protected_tag_access.rb +++ b/app/models/concerns/protected_tag_access.rb @@ -5,6 +5,7 @@ module ProtectedTagAccess include ProtectedRefAccess belongs_to :protected_tag + delegate :project, to: :protected_tag end end diff --git a/app/models/project.rb b/app/models/project.rb index 2469e6f8523..2c631050042 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -134,7 +134,7 @@ class Project < ActiveRecord::Base has_many :snippets, dependent: :destroy, class_name: 'ProjectSnippet' has_many :hooks, dependent: :destroy, class_name: 'ProjectHook' has_many :protected_branches, dependent: :destroy - has_many :protected_tags, dependent: :destroy + has_many :protected_tags, dependent: :destroy has_many :project_authorizations has_many :authorized_users, through: :project_authorizations, source: :user, class_name: 'User' diff --git a/app/models/protectable_dropdown.rb b/app/models/protectable_dropdown.rb index c9b2b213cd2..122fbce257d 100644 --- a/app/models/protectable_dropdown.rb +++ b/app/models/protectable_dropdown.rb @@ -6,8 +6,7 @@ class ProtectableDropdown # Tags/branches which are yet to be individually protected def protectable_ref_names - non_wildcard_protections = protections.reject(&:wildcard?) - refs.map(&:name) - non_wildcard_protections.map(&:name) + @protectable_ref_names ||= ref_names - non_wildcard_protected_ref_names end def hash @@ -20,7 +19,15 @@ class ProtectableDropdown @project.repository.public_send(@ref_type) end + def ref_names + refs.map(&:name) + end + def protections @project.public_send("protected_#{@ref_type}") end + + def non_wildcard_protected_ref_names + protections.reject(&:wildcard?).map(&:name) + end end diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb index 89d8ba60134..4b3337a5c9d 100644 --- a/app/services/protected_branches/update_service.rb +++ b/app/services/protected_branches/update_service.rb @@ -1,13 +1,10 @@ module ProtectedBranches class UpdateService < BaseService - attr_reader :protected_branch - def execute(protected_branch) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project) - @protected_branch = protected_branch - @protected_branch.update(params) - @protected_branch + protected_branch.update(params) + protected_branch end end end diff --git a/app/services/protected_tags/update_service.rb b/app/services/protected_tags/update_service.rb index 8a2419efd7b..aea6a48968d 100644 --- a/app/services/protected_tags/update_service.rb +++ b/app/services/protected_tags/update_service.rb @@ -1,13 +1,10 @@ module ProtectedTags class UpdateService < BaseService - attr_reader :protected_tag - def execute(protected_tag) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project) - @protected_tag = protected_tag - @protected_tag.update(params) - @protected_tag + protected_tag.update(params) + protected_tag end end end diff --git a/app/views/projects/branches/_branch.html.haml b/app/views/projects/branches/_branch.html.haml index d84fa9e55c0..931a5f920d3 100644 --- a/app/views/projects/branches/_branch.html.haml +++ b/app/views/projects/branches/_branch.html.haml @@ -15,7 +15,7 @@ %span.label.label-info.has-tooltip{ title: "Merged into #{@repository.root_ref}" } merged - - if ProtectedBranch.protected?(@project, branch.name) + - if protected_branch?(@project, branch) %span.label.label-success protected .controls.hidden-xs< diff --git a/app/views/projects/protected_branches/show.html.haml b/app/views/projects/protected_branches/show.html.haml index 4d8169815b3..f8cfe5e4b11 100644 --- a/app/views/projects/protected_branches/show.html.haml +++ b/app/views/projects/protected_branches/show.html.haml @@ -1,13 +1,13 @@ -- page_title @protected_branch.name, "Protected Branches" +- page_title @protected_ref.name, "Protected Branches" .row.prepend-top-default.append-bottom-default .col-lg-3 %h4.prepend-top-0 - = @protected_branch.name + = @protected_ref.name .col-lg-9 %h5 Matching Branches - - if @matching_branches.present? + - if @matching_refs.present? .table-responsive %table.table.protected-branches-list %colgroup @@ -18,7 +18,7 @@ %th Branch %th Last commit %tbody - - @matching_branches.each do |matching_branch| + - @matching_refs.each do |matching_branch| = render partial: "matching_branch", object: matching_branch - else %p.settings-message.text-center diff --git a/app/views/projects/protected_tags/show.html.haml b/app/views/projects/protected_tags/show.html.haml index 185807a7e8d..63743f28b3c 100644 --- a/app/views/projects/protected_tags/show.html.haml +++ b/app/views/projects/protected_tags/show.html.haml @@ -1,13 +1,13 @@ -- page_title @protected_tag.name, "Protected Tags" +- page_title @protected_ref.name, "Protected Tags" .row.prepend-top-default.append-bottom-default .col-lg-3 %h4.prepend-top-0 - = @protected_tag.name + = @protected_ref.name .col-lg-9 %h5 Matching Tags - - if @matching_tags.present? + - if @matching_refs.present? .table-responsive %table.table.protected-tags-list %colgroup @@ -18,7 +18,7 @@ %th Tag %th Last commit %tbody - - @matching_tags.each do |matching_tag| + - @matching_refs.each do |matching_tag| = render partial: "matching_tag", object: matching_tag - else %p.settings-message.text-center -- cgit v1.2.3