Commit cef10ef7 authored by Timothy Andrew's avatar Timothy Andrew

Implement review comments from @dbalexandre.

1. Don't have any EE-only code in CE. Ok to have CE-only code in EE.

2. Use `case` instead of `if/elsif`
parent b116df7e
...@@ -14,25 +14,19 @@ module ProtectedBranches ...@@ -14,25 +14,19 @@ module ProtectedBranches
@protected_branch = protected_branch @protected_branch = protected_branch
protected_branch.transaction do protected_branch.transaction do
# If a protected branch can have only a single access level (CE), delete it to delete_redundant_access_levels
# make room for the new access level. If a protected branch can have more than
# one access level (EE), only remove the relevant access levels. If we don't do this,
# we'll have a failed validation.
if protected_branch_restricted_to_single_access_level?
delete_redundant_ce_access_levels
else
delete_redundant_ee_access_levels
end
if @developers_can_push case @developers_can_push
when true
params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }]) params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
elsif @developers_can_push == false when false
params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }]) params.merge!(push_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
end end
if @developers_can_merge case @developers_can_merge
when true
params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }]) params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::DEVELOPER }])
elsif @developers_can_merge == false when false
params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }]) params.merge!(merge_access_levels_attributes: [{ access_level: Gitlab::Access::MASTER }])
end end
...@@ -43,7 +37,7 @@ module ProtectedBranches ...@@ -43,7 +37,7 @@ module ProtectedBranches
private private
def delete_redundant_ce_access_levels def delete_redundant_access_levels
if @developers_can_merge || @developers_can_merge == false if @developers_can_merge || @developers_can_merge == false
@protected_branch.merge_access_levels.destroy_all @protected_branch.merge_access_levels.destroy_all
end end
...@@ -52,28 +46,5 @@ module ProtectedBranches ...@@ -52,28 +46,5 @@ module ProtectedBranches
@protected_branch.push_access_levels.destroy_all @protected_branch.push_access_levels.destroy_all
end end
end end
def delete_redundant_ee_access_levels
if @developers_can_merge
@protected_branch.merge_access_levels.developer.destroy_all
elsif @developers_can_merge == false
@protected_branch.merge_access_levels.developer.destroy_all
@protected_branch.merge_access_levels.master.destroy_all
end
if @developers_can_push
@protected_branch.push_access_levels.developer.destroy_all
elsif @developers_can_push == false
@protected_branch.push_access_levels.developer.destroy_all
@protected_branch.push_access_levels.master.destroy_all
end
end
def protected_branch_restricted_to_single_access_level?
length_validator = ProtectedBranch.validators_on(:push_access_levels).find do |validator|
validator.is_a? ActiveModel::Validations::LengthValidator
end
length_validator.options[:is] == 1 if length_validator
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment