Backport EE refactorings for Protected Tag EE-only functionalityjej-backport-protected-tag-ee-role-refactorings

Improvements and refactorings were made while adding role based permissions for protected tags to EE. This doesn’t backport the feature, but should improve code quality and minimize divergence.

14 files changed, 48 insertions, 36 deletions

diff --git a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js
index 068e9698e1d..9d045886262 100644
--- a/app/assets/javascripts/protected_tags/protected_tag_dropdown.js
+++ b/app/assets/javascripts/protected_tags/protected_tag_dropdown.js
@@ -10,7 +10,7 @@ export default class ProtectedTagDropdown {
     this.$dropdown = options.$dropdown;
     this.$dropdownContainer = this.$dropdown.parent();
     this.$dropdownFooter = this.$dropdownContainer.find('.dropdown-footer');
-    this.$protectedTag = this.$dropdownContainer.find('.create-new-protected-tag');
+    this.$protectedTag = this.$dropdownContainer.find('.js-create-new-protected-tag');
 
     this.buildDropdown();
     this.bindEvents();
@@ -73,7 +73,7 @@ export default class ProtectedTagDropdown {
       };
 
       this.$dropdownContainer
-        .find('.create-new-protected-tag code')
+        .find('.js-create-new-protected-tag code')
         .text(tagName);
     }
 
diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss
index 24ab2bedea2..4719bf826dc 100644
--- a/app/assets/stylesheets/pages/projects.scss
+++ b/app/assets/stylesheets/pages/projects.scss
@@ -675,14 +675,16 @@ pre.light-well {
   }
 }
 
-.new_protected_branch {
+.new_protected_branch,
+.new-protected-tag {
   label {
     margin-top: 6px;
     font-weight: normal;
   }
 }
 
-.create-new-protected-branch-button {
+.create-new-protected-branch-button,
+.create-new-protected-tag-button {
   @include dropdown-link;
 
   width: 100%;
diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index ba24fa9acfe..d1719f12072 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -19,7 +19,7 @@ class Projects::ProtectedBranchesController < Projects::ProtectedRefsController
 
   def protected_ref_params
     params.require(:protected_branch).permit(:name,
-                                             merge_access_levels_attributes: [:access_level, :id],
-                                             push_access_levels_attributes: [:access_level, :id])
+                                             merge_access_levels_attributes: access_level_attributes,
+                                             push_access_levels_attributes: access_level_attributes)
   end
 end
diff --git a/app/controllers/projects/protected_refs_controller.rb b/app/controllers/projects/protected_refs_controller.rb
index 083a70968e5..b51bdf7aa78 100644
--- a/app/controllers/projects/protected_refs_controller.rb
+++ b/app/controllers/projects/protected_refs_controller.rb
@@ -44,4 +44,10 @@ class Projects::ProtectedRefsController < Projects::ApplicationController
       format.js { head :ok }
     end
   end
+
+  protected
+
+  def access_level_attributes
+    %i(access_level id)
+  end
 end
diff --git a/app/controllers/projects/protected_tags_controller.rb b/app/controllers/projects/protected_tags_controller.rb
index c61ddf145e6..a5dbd7e46ae 100644
--- a/app/controllers/projects/protected_tags_controller.rb
+++ b/app/controllers/projects/protected_tags_controller.rb
@@ -18,6 +18,6 @@ class Projects::ProtectedTagsController < Projects::ProtectedRefsController
   end
 
   def protected_ref_params
-    params.require(:protected_tag).permit(:name, create_access_levels_attributes: [:access_level, :id])
+    params.require(:protected_tag).permit(:name, create_access_levels_attributes: access_level_attributes)
   end
 end
diff --git a/app/models/concerns/protected_ref.rb b/app/models/concerns/protected_ref.rb
index 62eaec2407f..47e71c58557 100644
--- a/app/models/concerns/protected_ref.rb
+++ b/app/models/concerns/protected_ref.rb
@@ -8,32 +8,44 @@ module ProtectedRef
     validates :project, presence: true
 
     delegate :matching, :matches?, :wildcard?, to: :ref_matcher
+  end
+
+  def commit
+    project.commit(self.name)
+  end
+
+  class_methods do
+    def protected_ref_access_levels(*types)
+      types.each do |type|
+        has_many :"#{type}_access_levels", dependent: :destroy
+
+        validates :"#{type}_access_levels", length: { is: 1, message: "are restricted to a single instance per #{self.model_name.human}." }
 
-    def self.protected_ref_accessible_to?(ref, user, action:)
+        accepts_nested_attributes_for :"#{type}_access_levels", allow_destroy: true
+      end
+    end
+
+    def protected_ref_accessible_to?(ref, user, action:)
       access_levels_for_ref(ref, action: action).any? do |access_level|
         access_level.check_access(user)
       end
     end
 
-    def self.developers_can?(action, ref)
+    def developers_can?(action, ref)
       access_levels_for_ref(ref, action: action).any? do |access_level|
         access_level.access_level == Gitlab::Access::DEVELOPER
       end
     end
 
-    def self.access_levels_for_ref(ref, action:)
+    def access_levels_for_ref(ref, action:)
       self.matching(ref).map(&:"#{action}_access_levels").flatten
     end
 
-    def self.matching(ref_name, protected_refs: nil)
+    def matching(ref_name, protected_refs: nil)
       ProtectedRefMatcher.matching(self, ref_name, protected_refs: protected_refs)
     end
   end
 
-  def commit
-    project.commit(self.name)
-  end
-
   private
 
   def ref_matcher
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index 28b7d5ad072..5f0d0802ac9 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -2,14 +2,7 @@ class ProtectedBranch < ActiveRecord::Base
   include Gitlab::ShellAdapter
   include ProtectedRef
 
-  has_many :merge_access_levels, dependent: :destroy
-  has_many :push_access_levels, dependent: :destroy
-
-  validates :merge_access_levels, length: { is: 1, message: "are restricted to a single instance per protected branch." }
-  validates :push_access_levels, length: { 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
+  protected_ref_access_levels :merge, :push
 
   # Check if branch name is marked as protected in the system
   def self.protected?(project, ref_name)
diff --git a/app/models/protected_tag.rb b/app/models/protected_tag.rb
index 83964095516..f38109c0e52 100644
--- a/app/models/protected_tag.rb
+++ b/app/models/protected_tag.rb
@@ -2,11 +2,7 @@ class ProtectedTag < ActiveRecord::Base
   include Gitlab::ShellAdapter
   include ProtectedRef
 
-  has_many :create_access_levels, dependent: :destroy
-
-  validates :create_access_levels, length: { is: 1, message: "are restricted to a single instance per protected tag." }
-
-  accepts_nested_attributes_for :create_access_levels
+  protected_ref_access_levels :create
 
   def self.protected?(project, ref_name)
     self.matching(ref_name, protected_refs: project.protected_tags).present?
diff --git a/app/views/projects/protected_tags/_create_protected_tag.html.haml b/app/views/projects/protected_tags/_create_protected_tag.html.haml
index af9a080f0a2..dd5b346d922 100644
--- a/app/views/projects/protected_tags/_create_protected_tag.html.haml
+++ b/app/views/projects/protected_tags/_create_protected_tag.html.haml
@@ -1,4 +1,4 @@
-= form_for [@project.namespace.becomes(Namespace), @project, @protected_tag], html: { class: 'js-new-protected-tag' } do |f|
+= form_for [@project.namespace.becomes(Namespace), @project, @protected_tag], html: { class: 'new-protected-tag js-new-protected-tag' } do |f|
   .panel.panel-default
     .panel-heading
       %h3.panel-title
diff --git a/app/views/projects/protected_tags/_dropdown.html.haml b/app/views/projects/protected_tags/_dropdown.html.haml
index c8531f96f97..9b6923210f7 100644
--- a/app/views/projects/protected_tags/_dropdown.html.haml
+++ b/app/views/projects/protected_tags/_dropdown.html.haml
@@ -2,7 +2,7 @@
 
 = dropdown_tag('Select tag or create wildcard',
                options: { toggle_class: 'js-protected-tag-select js-filter-submit wide git-revision-dropdown-toggle',
-                          filter: true, dropdown_class: "dropdown-menu-selectable capitalize-header git-revision-dropdown", placeholder: "Search protected tag",
+                          filter: true, dropdown_class: "dropdown-menu-selectable capitalize-header git-revision-dropdown", placeholder: "Search protected tags",
                           footer_content: true,
                           data: { show_no: true, show_any: true, show_upcoming: true,
                                   selected: params[:protected_tag_name],
@@ -10,6 +10,6 @@
 
   %ul.dropdown-footer-list
     %li
-      = link_to '#', title: "New Protected Tag", class: "create-new-protected-tag" do
+      %button{ class: "create-new-protected-tag-button js-create-new-protected-tag", title: "New Protected Tag" }
         Create wildcard
         %code
diff --git a/app/views/projects/protected_tags/_index.html.haml b/app/views/projects/protected_tags/_index.html.haml
index 0bfb1ad191d..663cbd7cd64 100644
--- a/app/views/projects/protected_tags/_index.html.haml
+++ b/app/views/projects/protected_tags/_index.html.haml
@@ -4,13 +4,14 @@
 .row.prepend-top-default.append-bottom-default
   .col-lg-3
     %h4.prepend-top-0
-      Protected tags
+      Protected Tags
     %p.prepend-top-20
-      By default, Protected tags are designed to:
+      By default, protected tags are designed to:
       %ul
         %li Prevent tag creation by everybody except Masters
         %li Prevent <strong>anyone</strong> from updating the tag
         %li Prevent <strong>anyone</strong> from deleting the tag
+      %p.append-bottom-0 Read more about #{link_to "protected tags", help_page_path("user/project/protected_tags"), class: "underlined-link"}.
   .col-lg-9
     - if can? current_user, :admin_project, @project
       = render 'projects/protected_tags/create_protected_tag'
diff --git a/app/views/projects/protected_tags/_protected_tag.html.haml b/app/views/projects/protected_tags/_protected_tag.html.haml
index 54249ec0db1..f11ce0483a9 100644
--- a/app/views/projects/protected_tags/_protected_tag.html.haml
+++ b/app/views/projects/protected_tags/_protected_tag.html.haml
@@ -19,4 +19,4 @@
 
   - if can_admin_project
     %td
-      = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_tag], data: { confirm: 'tag will be writable for developers. Are you sure?' }, method: :delete, class: 'btn btn-warning'
+      = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_tag], data: { confirm: 'Tag will be writable for developers. Are you sure?' }, method: :delete, class: 'btn btn-warning'
diff --git a/app/views/projects/protected_tags/_tags_list.html.haml b/app/views/projects/protected_tags/_tags_list.html.haml
index 728afd75b50..d432a5c9113 100644
--- a/app/views/projects/protected_tags/_tags_list.html.haml
+++ b/app/views/projects/protected_tags/_tags_list.html.haml
@@ -1,4 +1,4 @@
-.panel.panel-default.protected-tags-list.js-protected-tags-list
+.panel.panel-default.protected-tags-list
   - if @protected_tags.empty?
     .panel-heading
       %h3.panel-title
@@ -13,6 +13,8 @@
         %col{ width: "25%" }
         %col{ width: "25%" }
         %col{ width: "50%" }
+        - if can_admin_project
+          %col
       %thead
         %tr
           %th Protected tag (#{@protected_tags.size})
diff --git a/app/views/projects/protected_tags/show.html.haml b/app/views/projects/protected_tags/show.html.haml
index 94c3612a449..16fc02fe9f4 100644
--- a/app/views/projects/protected_tags/show.html.haml
+++ b/app/views/projects/protected_tags/show.html.haml
@@ -5,7 +5,7 @@
     %h4.prepend-top-0.ref-name
       = @protected_ref.name
 
-  .col-lg-9
+  .col-lg-9.edit_protected_tag
     %h5 Matching Tags
     - if @matching_refs.present?
       .table-responsive