Fix branch protection API.

1. Previously, we were not removing existing access levels before
   creating new ones. This is not a problem for EE, but _is_ for CE,
   since we restrict the number of access levels in CE to 1.

2. The correct approach is:

    CE -> delete all access levels before updating a protected branch
    EE -> delete developer access levels if "developers_can_{merge,push}" is switched off

3. The dispatch is performed by checking if a "length: 1" validation is
   present on the access levels or not.

4. Another source of problems was that we didn't put multiple queries in
   a transaction. If the `destroy_all` passes, but the `update` fails,
   we should have a rollback.

5. Modifying the API to provide users direct access to CRUD access
   levels will make things a lot simpler.

6. Create `create/update` services separately for this API, which
   perform the necessary data translation, before calling the regular
   `create/update` services. The translation code was getting too large
   for the API endpoint itself, so this move makes sense.

1 files changed, 16 insertions, 31 deletions

diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index b615703df93..6941cc4aca6 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -57,40 +57,25 @@ module API
         developers_can_merge = to_boolean(params[:developers_can_merge])
         developers_can_push = to_boolean(params[:developers_can_push])
 
-        protected_branch_params = {
-          name: @branch.name
+        params = {
+          name: @branch.name,
         }
 
-        # If `developers_can_merge` is switched off, _all_ `DEVELOPER`
-        # merge_access_levels need to be deleted.
-        if developers_can_merge == false
-          protected_branch.merge_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all
-        end
+        service_args = [user_project, current_user, params,
+                        developers_can_push: developers_can_push,
+                        developers_can_merge: developers_can_merge]
 
-        # If `developers_can_push` is switched off, _all_ `DEVELOPER`
-        # push_access_levels need to be deleted.
-        if developers_can_push == false
-          protected_branch.push_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all
-        end
+        protected_branch = if protected_branch
+                             ProtectedBranches::ApiUpdateService.new(*service_args).execute(protected_branch)
+                           else
+                             ProtectedBranches::ApiCreateService.new(*service_args).execute
+                           end
 
-        protected_branch_params.merge!(
-          merge_access_levels_attributes: [{
-            access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
-          }],
-          push_access_levels_attributes: [{
-            access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
-          }]
-        )
-
-        if protected_branch
-          service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params)
-          service.execute(protected_branch)
+        if protected_branch.valid?
+          present @branch, with: Entities::RepoBranch, project: user_project
         else
-          service = ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params)
-          service.execute
+          render_api_error!(protected_branch.errors.full_messages, 422)
         end
-
-        present @branch, with: Entities::RepoBranch, project: user_project
       end
 
       # Unprotect a single branch
@@ -123,7 +108,7 @@ module API
       post ":id/repository/branches" do
         authorize_push_project
         result = CreateBranchService.new(user_project, current_user).
-          execute(params[:branch_name], params[:ref])
+                 execute(params[:branch_name], params[:ref])
 
         if result[:status] == :success
           present result[:branch],
@@ -142,10 +127,10 @@ module API
       # Example Request:
       #   DELETE /projects/:id/repository/branches/:branch
       delete ":id/repository/branches/:branch",
-          requirements: { branch: /.+/ } do
+             requirements: { branch: /.+/ } do
         authorize_push_project
         result = DeleteBranchService.new(user_project, current_user).
-          execute(params[:branch])
+                 execute(params[:branch])
 
         if result[:status] == :success
           {