Use `Gitlab::Access` to protected branch access levels.

1. It makes sense to reuse these constants since we had them duplicated
   in the previous enum implementation. This also simplifies our
   `check_access` implementation, because we can use
   `project.team.max_member_access` directly.

2. Use `accepts_nested_attributes_for` to create push/merge access
   levels. This was a bit fiddly to set up, but this simplifies our code
   by quite a large amount. We can even get rid of
   `ProtectedBranches::BaseService`.

3. Move API handling back into the API (previously in
   `ProtectedBranches::BaseService#translate_api_params`.

4. The protected branch services now return a `ProtectedBranch` rather
   than `true/false`.

5. Run `load_protected_branches` on-demand in the `create` action, to
   prevent it being called unneccessarily.

6. "Masters" is pre-selected as the default option for "Allowed to Push"
   and "Allowed to Merge".

7. These changes were based on a review from @rymai in !5081.

20 files changed, 152 insertions, 188 deletions

diff --git a/app/assets/javascripts/protected_branches_access_select.js.es6 b/app/assets/javascripts/protected_branches_access_select.js.es6
index 93b7d7755a7..e98312bbf37 100644
--- a/app/assets/javascripts/protected_branches_access_select.js.es6
+++ b/app/assets/javascripts/protected_branches_access_select.js.es6
@@ -1,27 +1,35 @@
 class ProtectedBranchesAccessSelect {
-  constructor(container, saveOnSelect) {
+  constructor(container, saveOnSelect, selectDefault) {
     this.container = container;
     this.saveOnSelect = saveOnSelect;
 
     this.container.find(".allowed-to-merge").each((i, element) => {
       var fieldName = $(element).data('field-name');
-      return $(element).glDropdown({
+      var dropdown = $(element).glDropdown({
         data: gon.merge_access_levels,
         selectable: true,
         fieldName: fieldName,
         clicked: _.chain(this.onSelect).partial(element).bind(this).value()
       });
+
+      if (selectDefault) {
+        dropdown.data('glDropdown').selectRowAtIndex(document.createEvent("Event"), 0);
+      }
     });
 
 
     this.container.find(".allowed-to-push").each((i, element) => {
       var fieldName = $(element).data('field-name');
-      return $(element).glDropdown({
+      var dropdown = $(element).glDropdown({
         data: gon.push_access_levels,
         selectable: true,
         fieldName: fieldName,
         clicked: _.chain(this.onSelect).partial(element).bind(this).value()
       });
+
+      if (selectDefault) {
+        dropdown.data('glDropdown').selectRowAtIndex(document.createEvent("Event"), 0);
+      }
     });
   }
 
@@ -36,7 +44,9 @@ class ProtectedBranchesAccessSelect {
           _method: 'PATCH',
           id: $(dropdown).data('id'),
           protected_branch: {
-            ["" + ($(dropdown).data('type'))]: selected.id
+            ["" + ($(dropdown).data('type')) + "_attributes"]: {
+              "access_level": selected.id
+            }
           }
         },
         success: function() {
diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index ddf1824ccb9..d28ec6e2eac 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -3,23 +3,22 @@ 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]
+  before_action :load_protected_branches, only: [:index]
 
   layout "project_settings"
 
   def index
     @protected_branch = @project.protected_branches.new
-    gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } },
-               push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
-               merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } })
+    load_protected_branches_gon_variables
   end
 
   def create
-    service = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params)
-    if service.execute
+    @protected_branch = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params).execute
+    if @protected_branch.persisted?
       redirect_to namespace_project_protected_branches_path(@project.namespace, @project)
     else
-      @protected_branch = service.protected_branch
+      load_protected_branches
+      load_protected_branches_gon_variables
       render :index
     end
   end
@@ -29,15 +28,15 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   end
 
   def update
-    service = ProtectedBranches::UpdateService.new(@project, current_user, params[:id], protected_branch_params)
+    @protected_branch = ProtectedBranches::UpdateService.new(@project, current_user, protected_branch_params).execute(@protected_branch)
 
-    if service.execute
+    if @protected_branch.valid?
       respond_to do |format|
-        format.json { render json: service.protected_branch, status: :ok }
+        format.json { render json: @protected_branch, status: :ok }
       end
     else
       respond_to do |format|
-        format.json { render json: service.protected_branch.errors, status: :unprocessable_entity }
+        format.json { render json: @protected_branch.errors, status: :unprocessable_entity }
       end
     end
   end
@@ -58,10 +57,18 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   end
 
   def protected_branch_params
-    params.require(:protected_branch).permit(:name, :allowed_to_push, :allowed_to_merge)
+    params.require(:protected_branch).permit(:name,
+                                             merge_access_level_attributes: [:access_level],
+                                             push_access_level_attributes: [:access_level])
   end
 
   def load_protected_branches
     @protected_branches = @project.protected_branches.order(:name).page(params[:page])
   end
+
+  def load_protected_branches_gon_variables
+    gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } },
+               push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
+               merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } })
+  end
 end
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index c0bee72b4d7..226b3f54342 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -8,18 +8,13 @@ class ProtectedBranch < ActiveRecord::Base
   has_one :merge_access_level, dependent: :destroy
   has_one :push_access_level, dependent: :destroy
 
+  accepts_nested_attributes_for :push_access_level
+  accepts_nested_attributes_for :merge_access_level
+
   def commit
     project.commit(self.name)
   end
 
-  def allowed_to_push
-    self.push_access_level && self.push_access_level.access_level
-  end
-
-  def allowed_to_merge
-    self.merge_access_level && self.merge_access_level.access_level
-  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 17a3a86c3e1..25a6ca6a8ee 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -2,25 +2,19 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
   delegate :project, to: :protected_branch
 
-  enum access_level: [:masters, :developers]
+  validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
+                                                             Gitlab::Access::DEVELOPER] }
 
   def self.human_access_levels
     {
-      "masters" => "Masters",
-      "developers" => "Developers + Masters"
+      Gitlab::Access::MASTER => "Masters",
+      Gitlab::Access::DEVELOPER => "Developers + Masters"
     }.with_indifferent_access
   end
 
   def check_access(user)
     return true if user.is_admin?
-
-    min_member_access = if masters?
-                          Gitlab::Access::MASTER
-                        elsif developers?
-                          Gitlab::Access::DEVELOPER
-                        end
-
-    project.team.max_member_access(user.id) >= min_member_access
+    project.team.max_member_access(user.id) >= access_level
   end
 
   def humanize
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index 22096b13300..1999316aa26 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -2,27 +2,22 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
   delegate :project, to: :protected_branch
 
-  enum access_level: [:masters, :developers, :no_one]
+  validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
+                                                             Gitlab::Access::DEVELOPER,
+                                                             Gitlab::Access::NO_ACCESS] }
 
   def self.human_access_levels
     {
-      "masters" => "Masters",
-      "developers" => "Developers + Masters",
-      "no_one" => "No one"
+      Gitlab::Access::MASTER => "Masters",
+      Gitlab::Access::DEVELOPER => "Developers + Masters",
+      Gitlab::Access::NO_ACCESS => "No one"
     }.with_indifferent_access
   end
 
   def check_access(user)
-    return false if no_one?
+    return false if access_level == Gitlab::Access::NO_ACCESS
     return true if user.is_admin?
-
-    min_member_access = if masters?
-                          Gitlab::Access::MASTER
-                        elsif developers?
-                          Gitlab::Access::DEVELOPER
-                        end
-
-    project.team.max_member_access(user.id) >= min_member_access
+    project.team.max_member_access(user.id) >= access_level
   end
 
   def humanize
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index 604737e6934..3f6a177bf3a 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -88,10 +88,17 @@ class GitPushService < BaseService
 
     # Set protection on the default branch if configured
     if current_application_settings.default_branch_protection != PROTECTION_NONE
-      allowed_to_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? 'developers' : 'masters'
-      allowed_to_merge = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? 'developers' : 'masters'
 
-      params = { name: @project.default_branch, allowed_to_push: allowed_to_push, allowed_to_merge: allowed_to_merge }
+      params = {
+        name: @project.default_branch,
+        push_access_level_attributes: {
+          access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+        },
+        merge_access_level_attributes: {
+          access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+        }
+      }
+
       ProtectedBranches::CreateService.new(@project, current_user, params).execute
     end
   end
diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb
deleted file mode 100644
index bdd175e8552..00000000000
--- a/app/services/protected_branches/base_service.rb
+++ /dev/null
@@ -1,67 +0,0 @@
-module ProtectedBranches
-  class BaseService < ::BaseService
-    def initialize(project, current_user, params = {})
-      super(project, current_user, params)
-      @allowed_to_push = params[:allowed_to_push]
-      @allowed_to_merge = params[:allowed_to_merge]
-    end
-
-    def set_access_levels!
-      translate_api_params!
-      set_merge_access_levels!
-      set_push_access_levels!
-    end
-
-    private
-
-    def set_merge_access_levels!
-      case @allowed_to_merge
-      when 'masters'
-        @protected_branch.merge_access_level.masters!
-      when 'developers'
-        @protected_branch.merge_access_level.developers!
-      end
-    end
-
-    def set_push_access_levels!
-      case @allowed_to_push
-      when 'masters'
-        @protected_branch.push_access_level.masters!
-      when 'developers'
-        @protected_branch.push_access_level.developers!
-      when 'no_one'
-        @protected_branch.push_access_level.no_one!
-      end
-    end
-
-    # The `branches` API still uses `developers_can_push` and `developers_can_merge`,
-    # which need to be translated to `allowed_to_push` and `allowed_to_merge`,
-    # respectively.
-    def translate_api_params!
-      @allowed_to_push ||=
-        case to_boolean(params[:developers_can_push])
-        when true
-          'developers'
-        when false
-          'masters'
-        end
-
-      @allowed_to_merge ||=
-        case to_boolean(params[:developers_can_merge])
-        when true
-          'developers'
-        when false
-          'masters'
-        end
-    end
-
-    protected
-
-    def to_boolean(value)
-      return true if value =~ /^(true|t|yes|y|1|on)$/i
-      return false if value =~ /^(false|f|no|n|0|off)$/i
-
-      nil
-    end
-  end
-end
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
index 36019906416..50f79f491ce 100644
--- a/app/services/protected_branches/create_service.rb
+++ b/app/services/protected_branches/create_service.rb
@@ -1,23 +1,27 @@
 module ProtectedBranches
-  class CreateService < ProtectedBranches::BaseService
+  class CreateService < BaseService
     attr_reader :protected_branch
 
     def execute
       raise Gitlab::Access::AccessDeniedError unless current_user.can?(:admin_project, project)
 
+      protected_branch = project.protected_branches.new(params)
+
       ProtectedBranch.transaction do
-        @protected_branch = project.protected_branches.new(name: params[:name])
-        @protected_branch.save!
+        protected_branch.save!
 
-        @protected_branch.create_push_access_level!
-        @protected_branch.create_merge_access_level!
+        if protected_branch.push_access_level.blank?
+          protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
+        end
 
-        set_access_levels!
+        if protected_branch.merge_access_level.blank?
+          protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
+        end
       end
 
-      true
+      protected_branch
     rescue ActiveRecord::RecordInvalid
-      false
+      protected_branch
     end
   end
 end
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
index 58f2f774bae..da4c96b3e5f 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -1,22 +1,13 @@
 module ProtectedBranches
-  class UpdateService < ProtectedBranches::BaseService
+  class UpdateService < BaseService
     attr_reader :protected_branch
 
-    def initialize(project, current_user, id, params = {})
-      super(project, current_user, params)
-      @protected_branch = ProtectedBranch.find(id)
-    end
-
-    def execute
+    def execute(protected_branch)
       raise Gitlab::Access::AccessDeniedError unless current_user.can?(:admin_project, project)
 
-      ProtectedBranch.transaction do
-        set_access_levels!
-      end
-
-      true
-    rescue ActiveRecord::RecordInvalid
-      false
+      @protected_branch = protected_branch
+      @protected_branch.update(params)
+      @protected_branch
     end
   end
 end
diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml
index a6956c8e69f..2498b57afb4 100644
--- a/app/views/projects/protected_branches/_branches_list.html.haml
+++ b/app/views/projects/protected_branches/_branches_list.html.haml
@@ -26,4 +26,4 @@
   = paginate @protected_branches, theme: 'gitlab'
 
 :javascript
-  new ProtectedBranchesAccessSelect($(".protected-branches-list"), true);
+  new ProtectedBranchesAccessSelect($(".protected-branches-list"), true, false);
diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml
index 2fc6081e448..498e412235e 100644
--- a/app/views/projects/protected_branches/_protected_branch.html.haml
+++ b/app/views/projects/protected_branches/_protected_branch.html.haml
@@ -18,12 +18,12 @@
     = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level
     = dropdown_tag(protected_branch.merge_access_level.humanize,
                    options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable merge',
-                   data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_merge" }})
+                   data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "merge_access_level" }})
   %td
     = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_level.access_level
     = dropdown_tag(protected_branch.push_access_level.humanize,
                    options: { title: "Allowed to push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable push',
-                   data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_push" }})
+                   data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "push_access_level" }})
   - 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 75c2063027a..8270da6cd27 100644
--- a/app/views/projects/protected_branches/index.html.haml
+++ b/app/views/projects/protected_branches/index.html.haml
@@ -32,20 +32,20 @@
             are supported.
 
         .form-group
-          = f.hidden_field :allowed_to_merge
-          = f.label :allowed_to_merge, "Allowed to merge: ", class: "label-light append-bottom-0"
+          = hidden_field_tag 'protected_branch[merge_access_level_attributes][access_level]'
+          = label_tag "Allowed to merge: ", nil, class: "label-light append-bottom-0"
           = dropdown_tag("<Make a selection>",
                          options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge',
                                     dropdown_class: 'dropdown-menu-selectable',
-                                    data: { field_name: "protected_branch[allowed_to_merge]" }})
+                                    data: { field_name: "protected_branch[merge_access_level_attributes][access_level]" }})
 
         .form-group
-          = f.hidden_field :allowed_to_push
-          = f.label :allowed_to_push, "Allowed to push: ", class: "label-light append-bottom-0"
+          = hidden_field_tag 'protected_branch[push_access_level_attributes][access_level]'
+          = label_tag "Allowed to push: ", nil, class: "label-light append-bottom-0"
           = dropdown_tag("<Make a selection>",
                          options: { title: "Allowed to push", toggle_class: 'allowed-to-push',
                                     dropdown_class: 'dropdown-menu-selectable',
-                                    data: { field_name: "protected_branch[allowed_to_push]" }})
+                                    data: { field_name: "protected_branch[push_access_level_attributes][access_level]" }})
 
 
         = f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true
@@ -54,4 +54,4 @@
     = render "branches_list"
 
 :javascript
-  new ProtectedBranchesAccessSelect($(".new_protected_branch"), false);
+  new ProtectedBranchesAccessSelect($(".new_protected_branch"), false, true);
diff --git a/db/migrate/20160705054938_add_protected_branches_push_access.rb b/db/migrate/20160705054938_add_protected_branches_push_access.rb
index 3031574fe2a..5c14d449e71 100644
--- a/db/migrate/20160705054938_add_protected_branches_push_access.rb
+++ b/db/migrate/20160705054938_add_protected_branches_push_access.rb
@@ -5,7 +5,9 @@ class AddProtectedBranchesPushAccess < ActiveRecord::Migration
   def change
     create_table :protected_branch_push_access_levels do |t|
       t.references :protected_branch, index: { name: "index_protected_branch_push_access" }, foreign_key: true, null: false
-      t.integer :access_level, default: 0, null: false
+
+      # Gitlab::Access::MASTER == 40
+      t.integer :access_level, default: 40, null: false
 
       t.timestamps null: false
     end
diff --git a/db/migrate/20160705054952_add_protected_branches_merge_access.rb b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
index cf1cdb8b3b6..789e3e04220 100644
--- a/db/migrate/20160705054952_add_protected_branches_merge_access.rb
+++ b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
@@ -5,7 +5,9 @@ class AddProtectedBranchesMergeAccess < ActiveRecord::Migration
   def change
     create_table :protected_branch_merge_access_levels do |t|
       t.references :protected_branch, index: { name: "index_protected_branch_merge_access" }, foreign_key: true, null: false
-      t.integer :access_level, default: 0, null: false
+
+      # Gitlab::Access::MASTER == 40
+      t.integer :access_level, default: 40, null: false
 
       t.timestamps null: false
     end
diff --git a/db/schema.rb b/db/schema.rb
index 7a5eded8e02..2d2ae5fd840 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -868,19 +868,19 @@ ActiveRecord::Schema.define(version: 20160722221922) do
   add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
 
   create_table "protected_branch_merge_access_levels", force: :cascade do |t|
-    t.integer  "protected_branch_id",             null: false
-    t.integer  "access_level",        default: 0, null: false
-    t.datetime "created_at",                      null: false
-    t.datetime "updated_at",                      null: false
+    t.integer  "protected_branch_id",              null: false
+    t.integer  "access_level",        default: 40, null: false
+    t.datetime "created_at",                       null: false
+    t.datetime "updated_at",                       null: false
   end
 
   add_index "protected_branch_merge_access_levels", ["protected_branch_id"], name: "index_protected_branch_merge_access", using: :btree
 
   create_table "protected_branch_push_access_levels", force: :cascade do |t|
-    t.integer  "protected_branch_id",             null: false
-    t.integer  "access_level",        default: 0, null: false
-    t.datetime "created_at",                      null: false
-    t.datetime "updated_at",                      null: false
+    t.integer  "protected_branch_id",              null: false
+    t.integer  "access_level",        default: 40, null: false
+    t.datetime "created_at",                       null: false
+    t.datetime "updated_at",                       null: false
   end
 
   add_index "protected_branch_push_access_levels", ["protected_branch_id"], name: "index_protected_branch_push_access", using: :btree
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index 4133a1f7a6b..a77afe634f6 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -53,19 +53,37 @@ module API
         @branch = user_project.repository.find_branch(params[:branch])
         not_found!('Branch') unless @branch
         protected_branch = user_project.protected_branches.find_by(name: @branch.name)
+
+        developers_can_merge = to_boolean(params[:developers_can_merge])
+        developers_can_push = to_boolean(params[:developers_can_push])
+
         protected_branch_params = {
-          name: @branch.name,
-          developers_can_push: params[:developers_can_push],
-          developers_can_merge: params[:developers_can_merge]
+          name: @branch.name
         }
 
-        service = if protected_branch
-                    ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch.id, protected_branch_params)
-                  else
-                    ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params)
-                  end
+        unless developers_can_merge.nil?
+          protected_branch_params.merge!({
+            merge_access_level_attributes: {
+              access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+            }
+          })
+        end
 
-        service.execute
+        unless developers_can_push.nil?
+          protected_branch_params.merge!({
+            push_access_level_attributes: {
+              access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+            }
+          })
+        end
+
+        if protected_branch
+          service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params)
+          service.execute(protected_branch)
+        else
+          service = ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params)
+          service.execute
+        end
 
         present @branch, with: Entities::RepoBranch, project: user_project
       end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index e51bee5c846..4eb95d8a215 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -127,12 +127,12 @@ module API
 
       expose :developers_can_push do |repo_branch, options|
         project = options[:project]
-        project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_level.developers? }
+        project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_level.access_level == Gitlab::Access::DEVELOPER }
       end
 
       expose :developers_can_merge do |repo_branch, options|
         project = options[:project]
-        project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_level.developers? }
+        project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_level.access_level == Gitlab::Access::DEVELOPER }
       end
     end
 
diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb
index 24a9b78f0c2..5575852c2d7 100644
--- a/spec/factories/protected_branches.rb
+++ b/spec/factories/protected_branches.rb
@@ -4,20 +4,26 @@ FactoryGirl.define do
     project
 
     after(:create) do |protected_branch|
-      protected_branch.create_push_access_level!(access_level: :masters)
-      protected_branch.create_merge_access_level!(access_level: :masters)
+      protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
+      protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
     end
 
     trait :developers_can_push do
-      after(:create) { |protected_branch| protected_branch.push_access_level.developers! }
+      after(:create) do |protected_branch|
+        protected_branch.push_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
+      end
     end
 
     trait :developers_can_merge do
-      after(:create) { |protected_branch| protected_branch.merge_access_level.developers! }
+      after(:create) do |protected_branch|
+        protected_branch.merge_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
+      end
     end
 
     trait :no_one_can_push do
-      after(:create) { |protected_branch| protected_branch.push_access_level.no_one! }
+      after(:create) do |protected_branch|
+        protected_branch.push_access_level.update!(access_level: Gitlab::Access::NO_ACCESS)
+      end
     end
   end
 end
diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb
index dac2bcf9efd..57734b33a44 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -91,12 +91,12 @@ feature 'Projected Branches', feature: true, js: true do
         set_protected_branch_name('master')
         within('.new_protected_branch') do
           find(".allowed-to-push").click
-          click_on access_type_name
+          within(".dropdown.open .dropdown-menu") { click_on access_type_name }
         end
         click_on "Protect"
 
         expect(ProtectedBranch.count).to eq(1)
-        expect(ProtectedBranch.last.allowed_to_push).to eq(access_type_id)
+        expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
       end
 
       it "allows updating protected branches so that #{access_type_name} can push to them" do
@@ -112,7 +112,7 @@ feature 'Projected Branches', feature: true, js: true do
         end
 
         wait_for_ajax
-        expect(ProtectedBranch.last.allowed_to_push).to eq(access_type_id)
+        expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
       end
     end
 
@@ -122,12 +122,12 @@ feature 'Projected Branches', feature: true, js: true do
         set_protected_branch_name('master')
         within('.new_protected_branch') do
           find(".allowed-to-merge").click
-          click_on access_type_name
+          within(".dropdown.open .dropdown-menu") { click_on access_type_name }
         end
         click_on "Protect"
 
         expect(ProtectedBranch.count).to eq(1)
-        expect(ProtectedBranch.last.allowed_to_merge).to eq(access_type_id)
+        expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
       end
 
       it "allows updating protected branches so that #{access_type_name} can merge to them" do
@@ -143,7 +143,7 @@ feature 'Projected Branches', feature: true, js: true do
         end
 
         wait_for_ajax
-        expect(ProtectedBranch.last.allowed_to_merge).to eq(access_type_id)
+        expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
       end
     end
   end
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 621eced83f6..ffa998dffc3 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -227,8 +227,8 @@ describe GitPushService, services: true do
         expect(project.default_branch).to eq("master")
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
         expect(project.protected_branches).not_to be_empty
-        expect(project.protected_branches.first.allowed_to_push).to eq('masters')
-        expect(project.protected_branches.first.allowed_to_merge).to eq('masters')
+        expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER)
+        expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::MASTER)
       end
 
       it "when pushing a branch for the first time with default branch protection disabled" do
@@ -249,8 +249,8 @@ describe GitPushService, services: true do
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
 
         expect(project.protected_branches).not_to be_empty
-        expect(project.protected_branches.last.allowed_to_push).to eq('developers')
-        expect(project.protected_branches.last.allowed_to_merge).to eq('masters')
+        expect(project.protected_branches.last.push_access_level.access_level).to eq(Gitlab::Access::DEVELOPER)
+        expect(project.protected_branches.last.merge_access_level.access_level).to eq(Gitlab::Access::MASTER)
       end
 
       it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do
@@ -260,8 +260,8 @@ describe GitPushService, services: true do
         expect(project.default_branch).to eq("master")
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
         expect(project.protected_branches).not_to be_empty
-        expect(project.protected_branches.first.allowed_to_push).to eq('masters')
-        expect(project.protected_branches.first.allowed_to_merge).to eq('developers')
+        expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER)
+        expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::DEVELOPER)
       end
 
       it "when pushing new commits to existing branch" do