Merge branch '21513-fix-branch-protection-api' into 'master'

Fix branch protection API.

- Fixes the branch protection API.
- Closes #21513
- EE Merge Request: gitlab-org/gitlab-ee!718

See merge request !6215
Signed-off-by: Rémy Coutable <remy@rymai.me>

CHANGELOG.md

@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 ## 8.13.1 (2016-10-25)
 
+  - Fix branch protection API. !6215
   - Fix hidden pipeline graph on commit and MR page. !6895
   - Fix Cycle analytics not showing correct data when filtering by date. !6906
   - Ensure custom provider tab labels don't break layout. !6993

app/models/concerns/protected_branch_access.rb

 module ProtectedBranchAccess
   extend ActiveSupport::Concern
 
+  included do
+    scope :master, -> { where(access_level: Gitlab::Access::MASTER) }
+    scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) }
+  end
+
   def humanize
     self.class.human_access_levels[self.access_level]
   end

app/services/protected_branches/api_create_service.rb

+# The protected branches API still uses the `developers_can_push` and `developers_can_merge`
+# flags for backward compatibility, and so performs translation between that format and the
+# internal data model (separate access levels). The translation code is non-trivial, and so
+# lives in this service.
+module ProtectedBranches
+  class ApiCreateService < BaseService
+    def execute
+      push_access_level =
+        if params.delete(:developers_can_push)
+          Gitlab::Access::DEVELOPER
+        else
+          Gitlab::Access::MASTER
+        end
+
+      merge_access_level =
+        if params.delete(:developers_can_merge)
+          Gitlab::Access::DEVELOPER
+        else
+          Gitlab::Access::MASTER
+        end
+
+      @params.merge!(push_access_levels_attributes: [{ access_level: push_access_level }],
+                     merge_access_levels_attributes: [{ access_level: merge_access_level }])
+
+      service = ProtectedBranches::CreateService.new(@project, @current_user, @params)
+      service.execute
+    end
+  end
+end

app/services/protected_branches/api_update_service.rb

+# The protected branches API still uses the `developers_can_push` and `developers_can_merge`
+# flags for backward compatibility, and so performs translation between that format and the
+# internal data model (separate access levels). The translation code is non-trivial, and so
+# lives in this service.
+module ProtectedBranches
+  class ApiUpdateService < BaseService
+    def execute(protected_branch)
+      @developers_can_push = params.delete(:developers_can_push)
+      @developers_can_merge = params.delete(:developers_can_merge)
+
+      @protected_branch = protected_branch
+
+      protected_branch.transaction do
+        delete_redundant_access_levels
+
+        case @developers_can_push
+        when true
+          params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
+        when false
+          params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
+        end
+
+        case @developers_can_merge
+        when true
+          params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
+        when false
+          params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
+        end
+
+        service = ProtectedBranches::UpdateService.new(@project, @current_user, @params)
+        service.execute(protected_branch)
+      end
+    end
+
+    private
+
+    def delete_redundant_access_levels
+      unless @developers_can_merge.nil?
+        @protected_branch.merge_access_levels.destroy_all
+      end
+
+      unless @developers_can_push.nil?
+        @protected_branch.push_access_levels.destroy_all
+      end
+    end
+  end
+end

lib/api/branches.rb

@@ -54,43 +54,25 @@ module API
         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
+          name: @branch.name,
+          developers_can_push: to_boolean(params[:developers_can_push]),
+          developers_can_merge: to_boolean(params[:developers_can_merge])
         }
 
-        # 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, protected_branch_params]
 
-        # 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 +105,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 +124,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
           {