Address some sections of first code review

app/controllers/admin/push_rules_controller.rb

@@ -24,9 +24,19 @@ class Admin::PushRulesController < Admin::ApplicationController
   end
 
   def push_rule_params
-    params.require(:push_rule).permit(:deny_delete_tag, :delete_branch_regex,
-      :commit_message_regex, :branch_name_regex, :force_push_regex, :author_email_regex, :member_check,
-      :file_name_regex, :max_file_size, :prevent_secrets, :reject_unsigned_commits, :commit_author_check)
+    allowed_fields = %i[deny_delete_tag delete_branch_regex commit_message_regex
+                        branch_name_regex force_push_regex author_email_regex
+                        member_check file_name_regex max_file_size prevent_secrets]
+
+    if @push_rule.available?(:reject_unsigned_commits)
+      allowed_fields << :reject_unsigned_commits
+    end
+
+    if @push_rule.available?(:commit_author_check)
+      allowed_fields << :commit_author_check
+    end
+
+    params.require(:push_rule).permit(allowed_fields)
   end
 
   def push_rule

app/controllers/projects/push_rules_controller.rb

@@ -29,19 +29,14 @@ class Projects::PushRulesController < Projects::ApplicationController
                         branch_name_regex force_push_regex author_email_regex
                         member_check file_name_regex max_file_size prevent_secrets]
 
-    allowed_fields.concat(extra_allowed_fields)
-
-    params.require(:push_rule).permit(allowed_fields)
-  end
-
-  def extra_allowed_fields
-    abilities_and_fields = {
-      change_reject_unsigned_commits: :reject_unsigned_commits,
-      change_commit_author_check: :commit_author_check
-    }
+    if can?(current_user, :change_reject_unsigned_commits, project)
+      allowed_fields << :reject_unsigned_commits
+    end
 
-    abilities_and_fields.each_with_object([]) do |(ability, field), fields|
-      fields << field if can?(current_user, ability, project)
+    if can?(current_user, :change_commit_author_check, project)
+      allowed_fields << :commit_author_check
     end
+
+    params.require(:push_rule).permit(allowed_fields)
   end
 end

app/helpers/push_rules_helper.rb

@@ -2,25 +2,24 @@ module PushRulesHelper
   def reject_unsigned_commits_description(push_rule)
     message = [s_("ProjectSettings|Only signed commits can be pushed to this repository.")]
 
-    annotate_with_update_message(message, push_rule,
-                                 enabled_globally: PushRule.global&.reject_unsigned_commits,
-                                 enabled_in_project: push_rule.reject_unsigned_commits)
+    push_rule_update_description(message, push_rule, :reject_unsigned_commits)
   end
 
   def commit_author_check_description(push_rule)
     message = [s_("ProjectSettings|Only the author of a commit can push changes to this repository.")]
 
-    annotate_with_update_message(message, push_rule,
-                                 enabled_globally: PushRule.global&.commit_author_check,
-                                 enabled_in_project: push_rule.commit_author_check)
+    push_rule_update_description(message, push_rule, :commit_author_check)
   end
 
   private
 
-  def annotate_with_update_message(message, push_rule, enabled_globally:, enabled_in_project:)
+  def push_rule_update_description(message, push_rule, rule)
     if push_rule.global?
       message << s_("ProjectSettings|This setting will be applied to all projects unless overridden by an admin.")
     else
+      enabled_globally = PushRule.global&.public_send(rule) # rubocop:disable GitlabSecurity/PublicSend
+      enabled_in_project = push_rule.public_send(rule) # rubocop:disable GitlabSecurity/PublicSend
+
       if enabled_globally
         message << if enabled_in_project
                      s_("ProjectSettings|This setting is applied on the server level and can be overridden by an admin.")

app/models/push_rule.rb

@@ -10,31 +10,6 @@ class PushRule < ActiveRecord::Base
     commit_author_check
   ].freeze
 
-  SETTINGS_WITH_GLOBAL_DEFAULT.each do |setting|
-    define_method(setting) do
-      value = super()
-
-      # return if value is true/false or if current object is the global setting
-      return value if global? || !value.nil?
-
-      PushRule.global&.public_send(setting)
-    end
-    alias_method "#{setting}?", setting
-
-    define_method("#{setting}=") do |value|
-      enabled_globally = PushRule.global&.public_send(setting)
-      is_disabled = !Gitlab::Utils.to_boolean(value)
-
-      # If setting is globally disabled and user disable it at project level,
-      # reset the attr so we can use the default global if required later.
-      if !enabled_globally && is_disabled
-        super(nil)
-      else
-        super(value)
-      end
-    end
-  end
-
   def self.global
     find_by(is_sample: true)
   end
@@ -97,6 +72,24 @@ class PushRule < ActiveRecord::Base
     end
   end
 
+  def reject_unsigned_commits
+    read_setting_with_global_default(:reject_unsigned_commits)
+  end
+  alias_method :reject_unsigned_commits?, :reject_unsigned_commits
+
+  def reject_unsigned_commits=(value)
+    write_setting_with_global_default(:reject_unsigned_commits, value)
+  end
+
+  def commit_author_check
+    read_setting_with_global_default(:commit_author_check)
+  end
+  alias_method :commit_author_check?, :commit_author_check
+
+  def commit_author_check=(value)
+    write_setting_with_global_default(:commit_author_check, value)
+  end
+
   private
 
   def data_match?(data, regex)
@@ -106,4 +99,26 @@ class PushRule < ActiveRecord::Base
       true
     end
   end
+
+  def read_setting_with_global_default(setting)
+    value = read_attribute(setting)
+
+    # return if value is true/false or if current object is the global setting
+    return value if global? || !value.nil?
+
+    PushRule.global&.public_send(setting)
+  end
+
+  def write_setting_with_global_default(setting, value)
+    enabled_globally = PushRule.global&.public_send(setting)
+    is_disabled = !Gitlab::Utils.to_boolean(value)
+
+    # If setting is globally disabled and user disable it at project level,
+    # reset the attr so we can use the default global if required later.
+    if !enabled_globally && is_disabled
+      write_attribute(setting, nil)
+    else
+      write_attribute(setting, value)
+    end
+  end
 end