Commit a4b564b6 authored by Michael Kozono's avatar Michael Kozono

Refactor based on code review

parent f25b5b7f
...@@ -46,7 +46,7 @@ class Namespace < ActiveRecord::Base ...@@ -46,7 +46,7 @@ class Namespace < ActiveRecord::Base
before_create :sync_share_with_group_lock_with_parent before_create :sync_share_with_group_lock_with_parent
before_update :sync_share_with_group_lock_with_parent, if: :parent_changed? 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 # Legacy Storage specific hooks
...@@ -225,7 +225,7 @@ class Namespace < ActiveRecord::Base ...@@ -225,7 +225,7 @@ class Namespace < ActiveRecord::Base
end end
def sync_share_with_group_lock_with_parent 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 self.share_with_group_lock = true
end end
end end
......
...@@ -15,7 +15,7 @@ class GroupPolicy < BasePolicy ...@@ -15,7 +15,7 @@ class GroupPolicy < BasePolicy
condition(:nested_groups_supported, scope: :global) { Group.supports_nested_groups? } 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(:can_change_parent_share_with_group_lock) { @subject.has_parent? && can?(:change_share_with_group_lock, @subject.parent) }
condition(:has_projects) do condition(:has_projects) do
......
module UpdateVisibilityLevel 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 # check that user is allowed to set specified visibility_level
if new_visibility && new_visibility.to_i != target.visibility_level if new_visibility && new_visibility.to_i != target.visibility_level
unless can?(current_user, :change_visibility_level, target) && unless can?(current_user, :change_visibility_level, target) &&
......
...@@ -5,7 +5,7 @@ module Groups ...@@ -5,7 +5,7 @@ module Groups
def execute def execute
reject_parent_id! 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? return false unless valid_share_with_group_lock_change?
......
...@@ -3,7 +3,7 @@ module Projects ...@@ -3,7 +3,7 @@ module Projects
include UpdateVisibilityLevel include UpdateVisibilityLevel
def execute 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!') return error('New visibility level not allowed!')
end end
......
...@@ -433,7 +433,7 @@ describe Namespace do ...@@ -433,7 +433,7 @@ describe Namespace do
let!(:subgroup) { create(:group, parent: root_group )} let!(:subgroup) { create(:group, parent: root_group )}
it 'the subgroup share lock becomes enabled' do 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 expect(subgroup.reload.share_with_group_lock).to be_truthy
end end
...@@ -446,7 +446,7 @@ describe Namespace do ...@@ -446,7 +446,7 @@ describe Namespace do
let(:subgroup) { create(:group, parent: root_group, share_with_group_lock: true )} let(:subgroup) { create(:group, parent: root_group, share_with_group_lock: true )}
it 'the subgroup share lock does not change' do 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 expect(subgroup.reload.share_with_group_lock).to be_truthy
end end
...@@ -456,7 +456,7 @@ describe Namespace do ...@@ -456,7 +456,7 @@ describe Namespace do
let(:subgroup) { create(:group, parent: root_group )} let(:subgroup) { create(:group, parent: root_group )}
it 'the subgroup share lock does not change' do 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 expect(subgroup.reload.share_with_group_lock?).to be_falsey
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