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.

4 files changed, 27 insertions, 92 deletions

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