Protected Tags backend review changes

Added changelog

14 files changed, 46 insertions, 63 deletions

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