Use the `{Push,Merge}AccessLevel` models in the UI.

1. Improve error handling while creating protected branches.

2. Modify coffeescript code so that the "Developers can *" checkboxes
   send a '1' or '0' even when using AJAX. This lets us keep the backend
   code simpler.

3. Use services for both creating and updating protected branches.
   Destruction is taken care of with `dependent: :destroy`

8 files changed, 89 insertions, 10 deletions

diff --git a/app/assets/javascripts/protected_branches.js.coffee b/app/assets/javascripts/protected_branches.js.coffee
index 14afef2e2ee..e9d1937cf71 100644
--- a/app/assets/javascripts/protected_branches.js.coffee
+++ b/app/assets/javascripts/protected_branches.js.coffee
@@ -3,7 +3,7 @@ $ ->
     name = $(this).attr("name")
     if name == "developers_can_push" || name == "developers_can_merge"
       id = $(this).val()
-      can_push = $(this).is(":checked")
+      can_push = if $(this).is(":checked") then "1" else "0"
       url = $(this).data("url")
       $.ajax
         type: "PATCH"
diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index 10dca47fded..fdbe0044d3c 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -3,19 +3,23 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   before_action :require_non_empty_project
   before_action :authorize_admin_project!
   before_action :load_protected_branch, only: [:show, :update, :destroy]
+  before_action :load_protected_branches, only: [:index, :create]
 
   layout "project_settings"
 
   def index
-    @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, title: br.name } } })
   end
 
   def create
-    @project.protected_branches.create(protected_branch_params)
-    redirect_to namespace_project_protected_branches_path(@project.namespace,
-                                                          @project)
+    service = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params)
+    if service.execute
+      redirect_to namespace_project_protected_branches_path(@project.namespace, @project)
+    else
+      @protected_branch = service.protected_branch
+      render :index
+    end
   end
 
   def show
@@ -23,13 +27,15 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   end
 
   def update
-    if @protected_branch && @protected_branch.update_attributes(protected_branch_params)
+    service = ProtectedBranches::UpdateService.new(@project, current_user, params[:id], protected_branch_params)
+
+    if service.execute
       respond_to do |format|
-        format.json { render json: @protected_branch, status: :ok }
+        format.json { render json: service.protected_branch, status: :ok }
       end
     else
       respond_to do |format|
-        format.json { render json: @protected_branch.errors, status: :unprocessable_entity }
+        format.json { render json: service.protected_branch.errors, status: :unprocessable_entity }
       end
     end
   end
@@ -52,4 +58,8 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   def protected_branch_params
     params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge)
   end
+
+  def load_protected_branches
+    @protected_branches = @project.protected_branches.order(:name).page(params[:page])
+  end
 end
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index a411cb417e2..b0fde6c6c1b 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -5,13 +5,21 @@ class ProtectedBranch < ActiveRecord::Base
   validates :name, presence: true
   validates :project, presence: true
 
-  has_one :merge_access_level
-  has_one :push_access_level
+  has_one :merge_access_level, dependent: :destroy
+  has_one :push_access_level, dependent: :destroy
 
   def commit
     project.commit(self.name)
   end
 
+  def developers_can_push
+    self.push_access_level && self.push_access_level.developers?
+  end
+
+  def developers_can_merge
+    self.merge_access_level && self.merge_access_level.developers?
+  end
+
   # Returns all protected branches that match the given branch name.
   # This realizes all records from the scope built up so far, and does
   # _not_ return a relation.
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index 78cec5bf566..cfaa9c166fe 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -1,3 +1,5 @@
 class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
+
+  enum access_level: [:masters, :developers]
 end
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index d53c4c391e3..4345dc4ede4 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -1,3 +1,5 @@
 class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
+
+  enum access_level: [:masters, :developers]
 end
diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb
new file mode 100644
index 00000000000..d4be8698a5f
--- /dev/null
+++ b/app/services/protected_branches/base_service.rb
@@ -0,0 +1,17 @@
+module ProtectedBranches
+  class BaseService < ::BaseService
+    def set_access_levels!
+      if params[:developers_can_push] == '0'
+        @protected_branch.push_access_level.masters!
+      elsif params[:developers_can_push] == '1'
+        @protected_branch.push_access_level.developers!
+      end
+
+      if params[:developers_can_merge] == '0'
+        @protected_branch.merge_access_level.masters!
+      elsif params[:developers_can_merge] == '1'
+        @protected_branch.merge_access_level.developers!
+      end
+    end
+  end
+end
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
new file mode 100644
index 00000000000..743f7bd2ce1
--- /dev/null
+++ b/app/services/protected_branches/create_service.rb
@@ -0,0 +1,19 @@
+class ProtectedBranches::CreateService < BaseService
+  attr_reader :protected_branch
+
+  def execute
+    ProtectedBranch.transaction do
+      @protected_branch = project.protected_branches.new(name: params[:name])
+      @protected_branch.save!
+
+      @protected_branch.create_push_access_level!
+      @protected_branch.create_merge_access_level!
+
+      set_access_levels!
+    end
+
+    true
+  rescue ActiveRecord::RecordInvalid
+    false
+  end
+end
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
new file mode 100644
index 00000000000..ed59b06b79a
--- /dev/null
+++ b/app/services/protected_branches/update_service.rb
@@ -0,0 +1,21 @@
+module ProtectedBranches
+  class UpdateService < BaseService
+    attr_reader :protected_branch
+
+    def initialize(project, current_user, id, params = {})
+      super(project, current_user, params)
+      @id = id
+    end
+
+    def execute
+      ProtectedBranch.transaction do
+        @protected_branch = ProtectedBranch.find(@id)
+        set_access_levels!
+      end
+
+      true
+    rescue ActiveRecord::RecordInvalid
+      false
+    end
+  end
+end