From 1051087ac4efc3dbf45bd075e36af647d2b66d62 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 30 Sep 2016 13:14:46 +0530 Subject: 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. --- app/services/protected_branches/api_create_service.rb | 6 +++--- app/services/protected_branches/api_update_service.rb | 10 +++++----- lib/api/branches.rb | 9 +++------ 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) -- cgit v1.2.1