From a4b564b6200582b6d3700a5e77e0c11fc5cac011 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Tue, 5 Sep 2017 17:10:30 -0700 Subject: [PATCH] Refactor based on code review --- app/models/namespace.rb | 4 ++-- app/policies/group_policy.rb | 2 +- app/services/concerns/update_visibility_level.rb | 2 +- app/services/groups/update_service.rb | 2 +- app/services/projects/update_service.rb | 2 +- spec/models/namespace_spec.rb | 6 +++--- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index aeda829415a..4a9a23fea1f 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -46,7 +46,7 @@ class Namespace < ActiveRecord::Base before_create :sync_share_with_group_lock_with_parent before_update :sync_share_with_group_lock_with_parent, if: :parent_changed? - after_commit :force_share_with_group_lock_on_descendants, on: :update, if: -> { previous_changes.key?('share_with_group_lock') && share_with_group_lock? } + after_update :force_share_with_group_lock_on_descendants, if: -> { share_with_group_lock_changed? && share_with_group_lock? } # Legacy Storage specific hooks @@ -225,7 +225,7 @@ class Namespace < ActiveRecord::Base end def sync_share_with_group_lock_with_parent - if has_parent? && parent.share_with_group_lock? + if parent&.share_with_group_lock? self.share_with_group_lock = true end end diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 45416bb7149..e1420de57b8 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -15,7 +15,7 @@ class GroupPolicy < BasePolicy condition(:nested_groups_supported, scope: :global) { Group.supports_nested_groups? } - condition(:parent_share_locked) { @subject.has_parent? && @subject.parent.share_with_group_lock? } + condition(:parent_share_locked, scope: :subject) { @subject.parent&.share_with_group_lock? } condition(:can_change_parent_share_with_group_lock) { @subject.has_parent? && can?(:change_share_with_group_lock, @subject.parent) } condition(:has_projects) do diff --git a/app/services/concerns/update_visibility_level.rb b/app/services/concerns/update_visibility_level.rb index f67b5474627..536fcc6acce 100644 --- a/app/services/concerns/update_visibility_level.rb +++ b/app/services/concerns/update_visibility_level.rb @@ -1,5 +1,5 @@ module UpdateVisibilityLevel - def visibility_level_allowed?(target, new_visibility) + def valid_visibility_level_change?(target, new_visibility) # check that user is allowed to set specified visibility_level if new_visibility && new_visibility.to_i != target.visibility_level unless can?(current_user, :change_visibility_level, target) && diff --git a/app/services/groups/update_service.rb b/app/services/groups/update_service.rb index 90da1b3d1fe..7955dc32d52 100644 --- a/app/services/groups/update_service.rb +++ b/app/services/groups/update_service.rb @@ -5,7 +5,7 @@ module Groups def execute reject_parent_id! - return false unless visibility_level_allowed?(group, params[:visibility_level]) + return false unless valid_visibility_level_change?(group, params[:visibility_level]) return false unless valid_share_with_group_lock_change? diff --git a/app/services/projects/update_service.rb b/app/services/projects/update_service.rb index b3d62c9b84d..cb4ffcab778 100644 --- a/app/services/projects/update_service.rb +++ b/app/services/projects/update_service.rb @@ -3,7 +3,7 @@ module Projects include UpdateVisibilityLevel def execute - unless visibility_level_allowed?(project, params[:visibility_level]) + unless valid_visibility_level_change?(project, params[:visibility_level]) return error('New visibility level not allowed!') end diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 3f38316ceae..6e583dc3e93 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -433,7 +433,7 @@ describe Namespace do let!(:subgroup) { create(:group, parent: root_group )} it 'the subgroup share lock becomes enabled' do - root_group.update(share_with_group_lock: true) + root_group.update!(share_with_group_lock: true) expect(subgroup.reload.share_with_group_lock).to be_truthy end @@ -446,7 +446,7 @@ describe Namespace do let(:subgroup) { create(:group, parent: root_group, share_with_group_lock: true )} it 'the subgroup share lock does not change' do - root_group.update(share_with_group_lock: false) + root_group.update!(share_with_group_lock: false) expect(subgroup.reload.share_with_group_lock).to be_truthy end @@ -456,7 +456,7 @@ describe Namespace do let(:subgroup) { create(:group, parent: root_group )} it 'the subgroup share lock does not change' do - root_group.update(share_with_group_lock: false) + root_group.update!(share_with_group_lock: false) expect(subgroup.reload.share_with_group_lock?).to be_falsey end