Implement second round of review comments from @DouweM.

- Pass `developers_and_merge` and `developers_can_push` in `params`
  instead of using keyword arguments.

- Refactor a slightly complex boolean check to a simple `nil?` check.

3 files changed, 11 insertions, 14 deletions

diff --git a/app/services/protected_branches/api_create_service.rb b/app/services/protected_branches/api_create_service.rb
index cbb99ede9f3..d714a8aaf01 100644
--- a/app/services/protected_branches/api_create_service.rb
+++ b/app/services/protected_branches/api_create_service.rb
@@ -4,10 +4,10 @@
 # lives in this service.
 module ProtectedBranches
   class ApiCreateService < BaseService
-    def initialize(project, user, params, developers_can_push:, developers_can_merge:)
+    def initialize(project, user, params)
+      @developers_can_merge = params.delete(:developers_can_merge)
+      @developers_can_push = params.delete(:developers_can_push)
       super(project, user, params)
-      @developers_can_merge = developers_can_merge
-      @developers_can_push = developers_can_push
     end
 
     def execute
diff --git a/app/services/protected_branches/api_update_service.rb b/app/services/protected_branches/api_update_service.rb
index 1c27d32aad8..c28bffee2f4 100644
--- a/app/services/protected_branches/api_update_service.rb
+++ b/app/services/protected_branches/api_update_service.rb
@@ -4,10 +4,10 @@
 # lives in this service.
 module ProtectedBranches
   class ApiUpdateService < BaseService
-    def initialize(project, user, params, developers_can_push:, developers_can_merge:)
+    def initialize(project, user, params)
+      @developers_can_merge = params.delete(:developers_can_merge)
+      @developers_can_push = params.delete(:developers_can_push)
       super(project, user, params)
-      @developers_can_merge = developers_can_merge
-      @developers_can_push = developers_can_push
     end
 
     def execute(protected_branch)
@@ -38,11 +38,11 @@ module ProtectedBranches
     private
 
     def delete_redundant_access_levels
-      if @developers_can_merge || @developers_can_merge == false
+      unless @developers_can_merge.nil?
         @protected_branch.merge_access_levels.destroy_all
       end
 
-      if @developers_can_push || @developers_can_push == false
+      unless @developers_can_push.nil?
         @protected_branch.push_access_levels.destroy_all
       end
     end
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index 7feb47784b7..6d827448994 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -54,16 +54,13 @@ 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,
+          developers_can_push: to_boolean(params[:developers_can_push]),
+          developers_can_merge: to_boolean(params[:developers_can_merge])
         }
 
-        service_args = [user_project, current_user, protected_branch_params,
-                        developers_can_push: developers_can_push,
-                        developers_can_merge: developers_can_merge]
+        service_args = [user_project, current_user, protected_branch_params]
 
         protected_branch = if protected_branch
                              ProtectedBranches::ApiUpdateService.new(*service_args).execute(protected_branch)