Backport EE refactorings for Protected Tag EE-only functionality

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.

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);
     }
 

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%;

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],
+                                             merge_access_levels_attributes: access_level_attributes,
-                                             push_access_levels_attributes: [:access_level, :id])
+                                             push_access_levels_attributes: access_level_attributes)
   end
 end

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

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

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 self.protected_ref_accessible_to?(ref, user, action:)
+  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}." }
+
+        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

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
+  protected_ref_access_levels :merge, :push
-  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
 
   # Check if branch name is marked as protected in the system
   def self.protected?(project, ref_name)

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
+  protected_ref_access_levels :create
-
-  validates :create_access_levels, length: { is: 1, message: "are restricted to a single instance per protected tag." }
-
-  accepts_nested_attributes_for :create_access_levels
 
   def self.protected?(project, ref_name)
     self.matching(ref_name, protected_refs: project.protected_tags).present?

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

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

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'

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'

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})

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

spec/lib/gitlab/import_export/all_models.yml

@@ -144,7 +144,9 @@ merge_access_levels:
 push_access_levels:
 - protected_branch
 create_access_levels:
+- user
 - protected_tag
+- group
 container_repositories:
 - project
 - name