Refactor policy bot comment generation

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

The following discussion from !194271 (merged) should be addressed:

• @imam_h started a discussion: (+4 comments)

  question: I was thinking if we can simplify this check if policy_bot_comment is enabled like below? WDYT?

  The check return true if merge_request.policy_bot_comment.present? handles the case when we need to update the policy bot comment with all violations resolved message.

  Git diff

  diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb
  index b01576f7ac1d..efd19df91acf 100644
  --- a/ee/app/models/ee/merge_request.rb
  +++ b/ee/app/models/ee/merge_request.rb
  @@ -680,6 +680,12 @@ def squash_option
       end
       strong_memoize_attr :squash_option
   
  +    def policy_bot_comment
  +      notes
  +        .authored_by(::Users::Internal.security_bot)
  +        .note_starting_with(Security::ScanResultPolicies::PolicyViolationComment::MESSAGE_HEADER).first
  +    end
  +
       private
   
       def security_comparision?(service_class)
  diff --git a/ee/app/models/security/scan_result_policy_read.rb b/ee/app/models/security/scan_result_policy_read.rb
  index cf7d6ac9c554..dc73cde8da89 100644
  --- a/ee/app/models/security/scan_result_policy_read.rb
  +++ b/ee/app/models/security/scan_result_policy_read.rb
  @@ -90,6 +90,10 @@ def bot_message_disabled?
         send_bot_message['enabled'] == false
       end
   
  +    def bot_message_enabled?
  +      send_bot_message.fetch('enabled', true)
  +    end
  +
       def approval_policy_rule
         return super if approval_policy_rule_id.present?
         return if real_policy_index < 0
  diff --git a/ee/app/services/concerns/security/scan_result_policies/policy_violation_comment_generator.rb b/ee/app/services/concerns/security/scan_result_policies/policy_violation_comment_generator.rb
  index 42183b78785d..03e9782f291f 100644
  --- a/ee/app/services/concerns/security/scan_result_policies/policy_violation_comment_generator.rb
  +++ b/ee/app/services/concerns/security/scan_result_policies/policy_violation_comment_generator.rb
  @@ -6,7 +6,7 @@ module PolicyViolationCommentGenerator
         private
   
         def generate_policy_bot_comment(merge_request)
  -        return if bot_message_disabled?(merge_request)
  +        return unless bot_message_enabled?(merge_request)
           return unless violations_populated?(merge_request)
   
           Security::GeneratePolicyViolationCommentWorker.perform_async(merge_request.id)
  @@ -16,23 +16,19 @@ def violations_populated?(merge_request)
           !merge_request.scan_result_policy_violations.without_violation_data.exists?
         end
   
  -      def bot_message_disabled?(merge_request)
  +      def bot_message_enabled?(merge_request)
           project = merge_request.project
   
  -        return true if project.archived?
  +        return false if project.archived?
  +        return true if merge_request.policy_bot_comment.present?
   
  -        applicable_rules_policy_ids = merge_request.approval_rules.report_approver
  -                                           .applicable_to_branch(merge_request.target_branch)
  -                                           .filter_map(&:scan_result_policy_id)
  -        violated_policy_ids = merge_request.scan_result_policy_violations.filter_map(&:scan_result_policy_id)
  -        # Only check `bot_message_disabled?` for policies that are both applicable to the branch and have violations
  -        policies_requiring_bot_message_check = (applicable_rules_policy_ids & violated_policy_ids)
  -        return false if policies_requiring_bot_message_check.blank?
  +        policy_violations = merge_request.scan_result_policy_violations.including_scan_result_policy_reads
  +        return false if policy_violations.blank?
   
  -        policies = project.scan_result_policy_reads.id_in(policies_requiring_bot_message_check)
  +        policies = policy_violations.filter_map(&:scan_result_policy_read)
           return false if policies.blank?
   
  -        policies.all?(&:bot_message_disabled?)
  +        policies.any?(&:bot_message_enabled?)
         end
       end
     end

• Check if the current method for generating the comment can be simplified

  • Keep in mind that applicable_to_branch is likely still required to ensure that no bot comment is posted when merge request targets a branch that's not protected by approval policies
  • We could achieve simplification if we don't create violations for MRs that target a branch which is not covered by approval policies. We just have to ensure that we reevaluate all policies when the branch changes.

• Changing the method from bot_message_disabled? to bot_message_enabled? could improve readability

• Check if we can utilize the event ::MergeRequests::ViolationsUpdatedEvent

• Use FF for larger changes to reduce the risk