Skip to content

Refactor sastReportsInInlineDiff handling

What does this MR do and why?

This MR is an iteration on solving: #417007 (closed)

It refactors the handling of the sast_reports_in_inline_diff flag (rollout Issue) on the client side.

Why would we want to do that? The required UI changes described in the Issue above make it necessary that the findings-detail information will be living as a direct child from the gutter icon in the future. Currently the detail information are living on the same level next to the findings-gutter icon.

  • This MR refactors the SAST inline findings out of the expanded-view, since they will be shown in a dropdown (to be added in follow-up MR)
  • adds a new flag-toggle in the diff_row component so that we can make the switch between the two UX components easily and only have a single source of truth.
  • This makes it easy to remove the expanded section completely once the flag will be removed

Adding the new Findings UI will be done in a separate MR to keep the size of the MR's at a manageable level.

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

with sast_reports_in_inline_diff false

no visual changes

Before After
MR1_flag_off_before MR1_flag_off_after

with sast_reports_in_inline_diff true

no expanded section, popover will be introduced in follow-up MR

Before After
MR1_flag_on_before MR1_flag_on_after

How to set up and validate locally

  1. Clone this repo: https://gitlab.com/jannik_lehmann/sast-inline-findings-example
  2. Recreate this MR: jannik_lehmann/sast-inline-findings-example!1 (merged)
  3. Let the Pipeline finish and see the Security findings in action
  4. Clone this repo: https://gitlab.com/jannik_lehmann/code-quality-test
  5. Recreate this MR: jannik_lehmann/code-quality-test!4 (diffs)
  6. Let the pipeline finish and see that the CodeQuality Findings are still working.
  7. Enable this flag: sast_reports_in_inline_diff
  8. Apply the patch below
  9. Go to recreated MR, see CQ and (mocked)-Security finding on same line
diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js
index b9cf26827f26..f487574edcf9 100644
--- a/app/assets/javascripts/diffs/index.js
+++ b/app/assets/javascripts/diffs/index.js
@@ -32,7 +32,8 @@ export default function initDiffsApp(store = notesStore) {
       return {
         endpointCoverage: dataset.endpointCoverage || '',
         endpointCodequality: dataset.endpointCodequality || '',
-        endpointSast: dataset.endpointSast || '',
+        // endpointSast: dataset.endpointSast || '',
+        endpointSast: 'mockedEndpoint',
         helpPagePath: dataset.helpPagePath,
         currentUser: JSON.parse(dataset.currentUserData) || {},
         changesEmptyStateIllustration: dataset.changesEmptyStateIllustration,
diff --git a/ee/app/assets/javascripts/diffs/store/actions.js b/ee/app/assets/javascripts/diffs/store/actions.js
index 3c91c1fc3482..b70aac7b3976 100644
--- a/ee/app/assets/javascripts/diffs/store/actions.js
+++ b/ee/app/assets/javascripts/diffs/store/actions.js
@@ -100,6 +100,23 @@ export const setGenerateTestFilePath = ({ commit }, path) =>
   commit(types.SET_GENERATE_TEST_FILE_PATH, path);
 
 export const fetchSast = ({ commit, state, dispatch }) => {
+  commit(types.SET_SAST_DATA, {
+    added: [
+      {
+        severity: "medium",
+        description:
+          "Markup escaping disabled. This can be used with some template engines to escape\ndisabling of HTML entities, which can lead to XSS attacks.\n",
+        location: {
+          file: "noise.rb",
+          start_line: 7,
+        },
+      },
+    ],
+    fixed: [],
+  });
+
+  return;
+
   let retryCount = 0;
   sastPoll = new Poll({
     resource: {

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Jannik Lehmann

Merge request reports

Loading