Merge branch 'g.hickman-bot-comment-default-approvals-required' into 'master'

Update default bot comment text when approvals required

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/143717



Merged-by: Sashi Kumar Kumaresan <skumar@gitlab.com>
Approved-by: Russell Dickenson <rdickenson@gitlab.com>
Approved-by: Dominic Bauer <dbauer@gitlab.com>
Approved-by: Sashi Kumar Kumaresan <skumar@gitlab.com>
Reviewed-by: Sashi Kumar Kumaresan <skumar@gitlab.com>
Co-authored-by: Russell Dickenson <rdickenson@gitlab.com>
Co-authored-by: Martin Čavoj <mcavoj@gitlab.com>
Co-authored-by: Grant Hickman <ghickman@gitlab.com>

ee/app/services/security/scan_result_policies/generate_policy_violation_comment_service.rb

@@ -44,7 +44,7 @@ def exclusive_lock_key
       end
 
       def comment
-        @comment ||= PolicyViolationComment.new(existing_comment).tap do |violation_comment|
+        @comment ||= PolicyViolationComment.new(existing_comment, project).tap do |violation_comment|
           violation_comment.remove_report_type(report_type)
           violation_comment.add_report_type(report_type, requires_approval) if violated_policy
         end

ee/lib/security/scan_result_policies/policy_violation_comment.rb

@@ -3,6 +3,8 @@
 module Security
   module ScanResultPolicies
     class PolicyViolationComment
+      include Rails.application.routes.url_helpers
+
       MESSAGE_HEADER = '<!-- policy_violation_comment -->'
       VIOLATED_REPORTS_HEADER_PATTERN = /<!-- violated_reports: ([a-z_,]+)/
       OPTIONAL_APPROVALS_HEADER_PATTERN = /<!-- optional_approvals: ([a-z_,]+)/
@@ -11,11 +13,17 @@ class PolicyViolationComment
         scan_finding: 'scan_finding',
         any_merge_request: 'any_merge_request'
       }.freeze
-      MESSAGE_REQUIRES_APPROVAL = <<~TEXT.squish
+      MESSAGE_REQUIRES_APPROVAL = <<~MARKDOWN
         Security and compliance scanners enforced by your organization have completed and identified that approvals
         are required due to one or more policy violations.
         Review the policy's rules in the MR widget and assign reviewers to proceed.
-      TEXT
+
+        Several factors can lead to a violation or required approval in your merge request:
+
+        - If merge request approval policies enforced on your project include a scanner in the conditions, the scanner must be properly configured to run in both the source and target branch, the required jobs must complete, and a job artifact containing the scan results must be produced (even if empty).
+        - If any violation of a merge request approval policy's rules are found, approval is required.
+- Approvals are assumed required until all pipelines associated with the merge base commit in the target branch, and all pipelines associated with the latest commit in the source branch, are complete and it's confirmed that no policy violations have occurred.
+      MARKDOWN
 
       MESSAGE_REQUIRES_NO_APPROVAL = <<~TEXT.squish
         Security and compliance scanners enforced by your organization have completed and identified one or more
@@ -23,12 +31,13 @@ class PolicyViolationComment
         Consider including optional reviewers based on the policy rules in the MR widget.
       TEXT
 
-      attr_reader :reports, :optional_approval_reports, :existing_comment
+      attr_reader :reports, :optional_approval_reports, :existing_comment, :project
 
-      def initialize(existing_comment)
+      def initialize(existing_comment, project)
         @existing_comment = existing_comment
         @reports = Set.new
         @optional_approval_reports = Set.new
+        @project = project
 
         return unless existing_comment
 
@@ -71,20 +80,33 @@ def fixed_note_body
         'Security policy violations have been resolved.'
       end
 
+      def links_approvals_required
+        <<~MARKDOWN
+
+          [View policies enforced on your project](#{project_security_policies_url(project)}).<br>
+          [View further troubleshooting guidance](#{help_page_url('user/application_security/policies/index', anchor: 'troubleshooting-common-issues-configuring-security-policies')}).
+        MARKDOWN
+      end
+
       def body_message
         return fixed_note_body if reports.empty?
 
-        message = reports == optional_approval_reports ? MESSAGE_REQUIRES_NO_APPROVAL : MESSAGE_REQUIRES_APPROVAL
+        message, links = if reports == optional_approval_reports
+                           [MESSAGE_REQUIRES_NO_APPROVAL, '']
+                         else
+                           [MESSAGE_REQUIRES_APPROVAL, links_approvals_required]
+                         end
+
         optional_approvals_sorted_list = optional_approval_reports.sort.join(',')
         <<~MARKDOWN
           <!-- violated_reports: #{reports.sort.join(',')} -->
           #{"<!-- optional_approvals: #{optional_approvals_sorted_list} -->" if optional_approval_reports.any?}
-          | :warning: **Policy violation(s) detected**|
-          | ----------------------------------------- |
-          | #{message}                                |
+          :warning: **Policy violation(s) detected**
+
+          #{message}#{links}
 
           #{format('Learn more about [Security and Compliance policies](%{url}).',
-            url: Rails.application.routes.url_helpers.help_page_url('user/application_security/policies/index'))}
+            url: help_page_url('user/application_security/policies/index'))}
         MARKDOWN
       end
     end

ee/spec/lib/security/scan_result_policies/policy_violation_comment_spec.rb

@@ -5,7 +5,8 @@
 RSpec.describe Security::ScanResultPolicies::PolicyViolationComment, feature_category: :security_policy_management do
   using RSpec::Parameterized::TableSyntax
 
-  let(:comment) { described_class.new(existing_comment) }
+  let_it_be(:project) { create(:project) }
+  let(:comment) { described_class.new(existing_comment, project) }
 
   def build_comment(reports: [], optional_approvals: [])
     build(:note,
@@ -104,9 +105,9 @@ def build_comment(reports: [], optional_approvals: [])
   describe '#body' do
     subject { comment.body }
 
-    let_it_be(:violations_resolved) { 'Security policy violations have been resolved.' }
-    let_it_be(:violations_detected) { 'Policy violation(s) detected' }
-    let_it_be(:optional_approvals_detected) { 'Consider including optional reviewers' }
+    let_it_be(:violations_resolved) { ['Security policy violations have been resolved.'] }
+    let_it_be(:violations_detected) { ['Policy violation(s) detected', 'View policies enforced on your project'] }
+    let_it_be(:optional_approvals_detected) { ['Consider including optional reviewers'] }
 
     context 'when there is no existing comment and no reports' do
       let(:existing_comment) { nil }
@@ -133,7 +134,7 @@ def build_comment(reports: [], optional_approvals: [])
       end
 
       it { is_expected.to start_with(described_class::MESSAGE_HEADER) }
-      it { is_expected.to include(expected_body) }
+      it { is_expected.to include(*expected_body) }
     end
   end
 end