Refactor based on code review

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