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.
This commit is contained in:
Timothy Andrew 2016-09-30 13:14:46 +05:30
parent b803bc7bb8
commit 1051087ac4
3 changed files with 11 additions and 14 deletions

View file

@ -4,10 +4,10 @@
# lives in this service. # lives in this service.
module ProtectedBranches module ProtectedBranches
class ApiCreateService < BaseService 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) super(project, user, params)
@developers_can_merge = developers_can_merge
@developers_can_push = developers_can_push
end end
def execute def execute

View file

@ -4,10 +4,10 @@
# lives in this service. # lives in this service.
module ProtectedBranches module ProtectedBranches
class ApiUpdateService < BaseService 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) super(project, user, params)
@developers_can_merge = developers_can_merge
@developers_can_push = developers_can_push
end end
def execute(protected_branch) def execute(protected_branch)
@ -38,11 +38,11 @@ module ProtectedBranches
private private
def delete_redundant_access_levels 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 @protected_branch.merge_access_levels.destroy_all
end end
if @developers_can_push || @developers_can_push == false unless @developers_can_push.nil?
@protected_branch.push_access_levels.destroy_all @protected_branch.push_access_levels.destroy_all
end end
end end

View file

@ -54,16 +54,13 @@ module API
not_found!('Branch') unless @branch not_found!('Branch') unless @branch
protected_branch = user_project.protected_branches.find_by(name: @branch.name) 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 = { 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])
} }
service_args = [user_project, current_user, protected_branch_params, service_args = [user_project, current_user, protected_branch_params]
developers_can_push: developers_can_push,
developers_can_merge: developers_can_merge]
protected_branch = if protected_branch protected_branch = if protected_branch
ProtectedBranches::ApiUpdateService.new(*service_args).execute(protected_branch) ProtectedBranches::ApiUpdateService.new(*service_args).execute(protected_branch)