Add code review remarks

Add cr remarks

Add cr remarks

Add cr remarks

app/models/group.rb

@@ -591,13 +591,13 @@ class Group < Namespace
   end
 
   def two_factor_authentication_allowed
-    return if parent_id.nil?
+    return unless has_parent?
     return unless require_two_factor_authentication
 
     ancestor_settings = ancestors.find_by(parent_id: nil).namespace_settings
     return if ancestor_settings.allow_mfa_for_subgroups
 
-    errors.add(:require_two_factor_authentication, "require two factor authentication is forbidden by a parent group")
+    errors.add(:require_two_factor_authentication, _('is forbidden by a top-level group'))
   end
 
   def members_from_self_and_ancestor_group_shares

app/models/namespace_setting.rb

@@ -4,7 +4,7 @@ class NamespaceSetting < ApplicationRecord
   belongs_to :namespace, inverse_of: :namespace_settings
 
   validate :default_branch_name_content
-  validate :allow_mfa_for_group, on: :update, if: :allow_mfa_for_subgroups_changed?
+  validate :allow_mfa_for_group
 
   NAMESPACE_SETTINGS_PARAMS = [:default_branch_name].freeze
 
@@ -19,8 +19,8 @@ class NamespaceSetting < ApplicationRecord
   end
 
   def allow_mfa_for_group
-    if namespace&.parent_id
-      errors.add(:allow_mfa_for_subgroups, "allow MFA setting is not allowed since group is not top-level group.")
+    if namespace&.subgroup? && allow_mfa_for_subgroups == false
+      errors.add(:allow_mfa_for_subgroups, _('is not allowed since the group is not top-level group.'))
     end
   end
 end

locale/gitlab.pot

@@ -30837,6 +30837,9 @@ msgstr ""
 msgid "is blocked by"
 msgstr ""
 
+msgid "is forbidden by a top-level group"
+msgstr ""
+
 msgid "is invalid because there is downstream lock"
 msgstr ""
 
@@ -30852,6 +30855,9 @@ msgstr ""
 msgid "is not a valid X509 certificate."
 msgstr ""
 
+msgid "is not allowed since the group is not top-level group."
+msgstr ""
+
 msgid "is not allowed. Try again with a different email address, or contact your GitLab admin."
 msgstr ""
 

spec/models/group_spec.rb

@@ -235,7 +235,7 @@ RSpec.describe Group do
       end
 
       context 'for a child group' do
-        let_it_be(:sub_group) { create(:group, parent: group) }
+        let(:sub_group) { create(:group, parent: group) }
 
         it 'is valid when parent group allows' do
           sub_group.require_two_factor_authentication = true
@@ -248,7 +248,7 @@ RSpec.describe Group do
           sub_group.require_two_factor_authentication = true
 
           expect(sub_group).to be_invalid
-          expect(sub_group.errors[:require_two_factor_authentication]).to include('require two factor authentication is forbidden by a parent group')
+          expect(sub_group.errors[:require_two_factor_authentication]).to include('is forbidden by a top-level group')
         end
       end
     end

spec/models/namespace_setting_spec.rb

@@ -47,19 +47,25 @@ RSpec.describe NamespaceSetting, type: :model do
     end
 
     describe '#allow_mfa_for_group' do
+      let(:settings) {  group.namespace_settings }
+
       context 'group is top-level group' do
         let(:group) { create(:group) }
 
-        it 'is valid when updated' do
-          expect(group.namespace_settings.update(allow_mfa_for_subgroups: false)).to eq(true)
+        it 'is valid' do
+          settings.allow_mfa_for_subgroups = false
+
+          expect(settings).to be_valid
         end
       end
 
       context 'group is a subgroup' do
         let(:group) { create(:group, parent: create(:group)) }
 
-        it 'is invalid when updated' do
-          expect(group.namespace_settings.update(allow_mfa_for_subgroups: false)).to eq(false)
+        it 'is invalid' do
+          settings.allow_mfa_for_subgroups = false
+
+          expect(settings).to be_invalid
         end
       end
     end