From 2a5cb7ec5259123cbbecb0577b9b4afacaf7546a Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 16 Jun 2016 13:03:30 +0530
Subject: [PATCH] Modify the frontend for wildcard protected branches.

1. Allow entering any branch name for a protected branch.

    - Either pick from a list of options, or enter it manually
    - You can enter wildcards.

2. Display branches matching a protected branch.

    -  Add a `ProtectedBranches#show` page that displays the branches
       matching the given protected branch, or a message if there are no
       matches.

    - On the `index` page, display the last commit for an exact match,
      or the number of matching branches for a wildcard match.

    -  Add an `iid` column to `protected_branches` - this is what we use for
       the `show` page URL.

    -  On the off chance that this feature is unnecessary, this commit
       encapsulates it neatly, so it can be removed without affecting
       anything else.

3. Remove the "Last Commit" column from the list of protected branches.

    - There's no way to pull these for wildcard protected branches, so it's
      best left for the `show` page.

    - Rename the `@branches` instance variable to `@protected_branches`

    - Minor styling changes with the "Unprotect" button - floated right
      like the "Revoke" button for personal access tokens

4. Paginate the list of protected branches.

5. Move the instructions to the left side of the page.
---
 .../javascripts/protected_branches.js.coffee  |  3 +-
 .../projects/protected_branches_controller.rb | 26 ++++++++-------
 app/models/protected_branch.rb                | 11 +++++++
 .../_branches_list.html.haml                  | 33 +++++--------------
 .../_matching_branch.html.haml                |  9 +++++
 .../_protected_branch.html.haml               | 21 ++++++++++++
 .../protected_branches/index.html.haml        | 16 ++++++++-
 .../protected_branches/show.html.haml         | 25 ++++++++++++++
 config/routes.rb                              |  2 +-
 9 files changed, 108 insertions(+), 38 deletions(-)
 create mode 100644 app/views/projects/protected_branches/_matching_branch.html.haml
 create mode 100644 app/views/projects/protected_branches/_protected_branch.html.haml
 create mode 100644 app/views/projects/protected_branches/show.html.haml

diff --git a/app/assets/javascripts/protected_branches.js.coffee b/app/assets/javascripts/protected_branches.js.coffee
index 5753c9d4e72..79c2306e4d2 100644
--- a/app/assets/javascripts/protected_branches.js.coffee
+++ b/app/assets/javascripts/protected_branches.js.coffee
@@ -11,7 +11,8 @@ $ ->
         dataType: "json"
         data:
           id: id
-          developers_can_push: checked
+          protected_branch:
+            developers_can_push: checked
 
         success: ->
           row = $(e.target)
diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index efa7bf14d0f..026c5b74eb9 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -2,12 +2,14 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   # Authorize
   before_action :require_non_empty_project
   before_action :authorize_admin_project!
+  before_action :load_protected_branch, only: [:show, :update, :destroy]
 
   layout "project_settings"
 
   def index
-    @branches = @project.protected_branches.to_a
+    @protected_branches = @project.protected_branches.order(:name).page(params[:page])
     @protected_branch = @project.protected_branches.new
+    gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name } } })
   end
 
   def create
@@ -16,26 +18,24 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
                                                           @project)
   end
 
-  def update
-    protected_branch = @project.protected_branches.find(params[:id])
-
-    if protected_branch &&
-       protected_branch.update_attributes(
-         developers_can_push: params[:developers_can_push]
-       )
+  def show
+    @matching_branches = @protected_branch.matching(@project.repository.branches)
+  end
 
+  def update
+    if @protected_branch && @protected_branch.update_attributes(protected_branch_params)
       respond_to do |format|
-        format.json { render json: protected_branch, status: :ok }
+        format.json { render json: @protected_branch, status: :ok }
       end
     else
       respond_to do |format|
-        format.json { render json: protected_branch.errors, status: :unprocessable_entity }
+        format.json { render json: @protected_branch.errors, status: :unprocessable_entity }
       end
     end
   end
 
   def destroy
-    @project.protected_branches.find(params[:id]).destroy
+    @protected_branch.destroy
 
     respond_to do |format|
       format.html { redirect_to namespace_project_protected_branches_path }
@@ -45,6 +45,10 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
 
   private
 
+  def load_protected_branch
+    @protected_branch = @project.protected_branches.find(params[:id])
+  end
+
   def protected_branch_params
     params.require(:protected_branch).permit(:name, :developers_can_push)
   end
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index 3db1ab0e5f9..d3d5e1d98b2 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -19,6 +19,12 @@ class ProtectedBranch < ActiveRecord::Base
     (protected_branches || all).select { |protected_branch| protected_branch.matches?(branch_name) }
   end
 
+  # Returns all branches (among the given list of branches [`Gitlab::Git::Branch`])
+  # that match the current protected branch.
+  def matching(branches)
+    branches.select { |branch| self.matches?(branch.name) }
+  end
+
   # Checks if the protected branch matches the given branch name.
   def matches?(branch_name)
     return false if self.name.blank?
@@ -26,6 +32,11 @@ class ProtectedBranch < ActiveRecord::Base
     exact_match?(branch_name) || wildcard_match?(branch_name)
   end
 
+  # Checks if this protected branch contains a wildcard
+  def wildcard?
+    self.name.include?('*')
+  end
+
   protected
 
   def exact_match?(branch_name)
diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml
index 565905cbe7b..97cb1a9052b 100644
--- a/app/views/projects/protected_branches/_branches_list.html.haml
+++ b/app/views/projects/protected_branches/_branches_list.html.haml
@@ -1,6 +1,6 @@
 %h5.prepend-top-0
-  Already Protected (#{@branches.size})
-- if @branches.empty?
+  Already Protected (#{@protected_branches.size})
+- if @protected_branches.empty?
   %p.settings-message.text-center
     No branches are protected, protect a branch with the form above.
 - else
@@ -9,33 +9,18 @@
     %table.table.protected-branches-list
       %colgroup
         %col{ width: "30%" }
-        %col{ width: "30%" }
+        %col{ width: "25%" }
         %col{ width: "25%" }
         - if can_admin_project
           %col
       %thead
         %tr
-          %th Branch
-          %th Last commit
-          %th Developers can push
+          %th Protected Branch
+          %th Commit
+          %th Developers Can Push
           - if can_admin_project
             %th
       %tbody
-        - @branches.each do |branch|
-          - @url = namespace_project_protected_branch_path(@project.namespace, @project, branch)
-          %tr
-            %td
-              = link_to(branch.name, namespace_project_commits_path(@project.namespace, @project, branch.name))
-              - if @project.root_ref?(branch.name)
-                %span.label.label-info.prepend-left-5 default
-            %td
-              - if commit = branch.commit
-                = link_to(commit.short_id, namespace_project_commit_path(@project.namespace, @project, commit.id), class: 'commit_short_id')
-                #{time_ago_with_tooltip(commit.committed_date)}
-              - else
-                (branch was removed from repository)
-            %td
-              = check_box_tag("developers_can_push", branch.id, branch.developers_can_push, data: { url: @url })
-            - if can_admin_project
-              %td
-                = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm"
+        = render partial: @protected_branches, locals: { can_admin_project: can_admin_project }
+
+  = paginate @protected_branches, theme: 'gitlab'
diff --git a/app/views/projects/protected_branches/_matching_branch.html.haml b/app/views/projects/protected_branches/_matching_branch.html.haml
new file mode 100644
index 00000000000..8a5332ca5bb
--- /dev/null
+++ b/app/views/projects/protected_branches/_matching_branch.html.haml
@@ -0,0 +1,9 @@
+%tr
+  %td
+    = link_to matching_branch.name, namespace_project_tree_path(@project.namespace, @project, matching_branch.name)
+    - if @project.root_ref?(matching_branch.name)
+      %span.label.label-info.prepend-left-5 default
+  %td
+    - commit = @project.commit(matching_branch.name)
+    = link_to(commit.short_id, namespace_project_commit_path(@project.namespace, @project, commit.id), class: 'commit_short_id')
+    = time_ago_with_tooltip(commit.committed_date)
diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml
new file mode 100644
index 00000000000..474aec3a97c
--- /dev/null
+++ b/app/views/projects/protected_branches/_protected_branch.html.haml
@@ -0,0 +1,21 @@
+- url = namespace_project_protected_branch_path(@project.namespace, @project, protected_branch)
+%tr
+  %td
+    = protected_branch.name
+    - if @project.root_ref?(protected_branch.name)
+      %span.label.label-info.prepend-left-5 default
+  %td
+    - if protected_branch.wildcard?
+      - matching_branches = protected_branch.matching(repository.branches)
+      = link_to pluralize(matching_branches.count, "matching branch"), namespace_project_protected_branch_path(@project.namespace, @project, protected_branch)
+    - else
+      - if commit = protected_branch.commit
+        = link_to(commit.short_id, namespace_project_commit_path(@project.namespace, @project, commit.id), class: 'commit_short_id')
+        = time_ago_with_tooltip(commit.committed_date)
+      - else
+        (branch was removed from repository)
+  %td
+    = check_box_tag("developers_can_push", protected_branch.id, protected_branch.developers_can_push, data: { url: url })
+  - if can_admin_project
+    %td
+      = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right"
diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml
index c7d317dbaee..8eaef1f2904 100644
--- a/app/views/projects/protected_branches/index.html.haml
+++ b/app/views/projects/protected_branches/index.html.haml
@@ -21,7 +21,14 @@
 
         .form-group
           = f.label :name, "Branch", class: "label-light"
-          = f.select(:name, @project.open_branches.map { |br| [br.name, br.name] } , {include_blank: true}, {class: "select2", data: {placeholder: "Select branch"}})
+          = f.text_field(:name)
+          %p.help-block
+            Wildcards such as
+            %code *-stable
+            or
+            %code production/*
+            are supported.
+
         .form-group
           = f.check_box :developers_can_push, class: "pull-left"
           .prepend-left-20
@@ -31,3 +38,10 @@
         = f.submit "Protect", class: "btn-create btn"
     %hr
     = render "branches_list"
+
+:javascript
+  $("#protected_branch_name").select2({
+    placeholder: "Select branch",
+    createSearchChoice: function(term) { return { id: term, text: term }; },
+    data: gon.open_branches
+  })
diff --git a/app/views/projects/protected_branches/show.html.haml b/app/views/projects/protected_branches/show.html.haml
new file mode 100644
index 00000000000..4d8169815b3
--- /dev/null
+++ b/app/views/projects/protected_branches/show.html.haml
@@ -0,0 +1,25 @@
+- page_title @protected_branch.name, "Protected Branches"
+
+.row.prepend-top-default.append-bottom-default
+  .col-lg-3
+    %h4.prepend-top-0
+      = @protected_branch.name
+
+  .col-lg-9
+    %h5 Matching Branches
+    - if @matching_branches.present?
+      .table-responsive
+        %table.table.protected-branches-list
+          %colgroup
+            %col{ width: "30%" }
+            %col{ width: "30%" }
+          %thead
+            %tr
+              %th Branch
+              %th Last commit
+          %tbody
+            - @matching_branches.each do |matching_branch|
+              = render partial: "matching_branch", object: matching_branch
+    - else
+      %p.settings-message.text-center
+        Couldn't find any matching branches.
diff --git a/config/routes.rb b/config/routes.rb
index 1572656b8c5..18a4ead2b37 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -720,7 +720,7 @@ Rails.application.routes.draw do
           resource :release, only: [:edit, :update]
         end
 
-        resources :protected_branches, only: [:index, :create, :update, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex }
+        resources :protected_branches, only: [:index, :show, :create, :update, :destroy], constraints: { id: Gitlab::Regex.git_reference_regex }
         resources :variables, only: [:index, :show, :update, :create, :destroy]
         resources :triggers, only: [:index, :create, :destroy]
 
-- 
GitLab