Code review refactor

* Replace utility API function to use existing API function
* Replace direct usage of glMrWidget object
* Update unit tests

Update action logic for fetching pipeline jobs

* Update error handling logic when one of two methods fail

app/assets/javascripts/api.js

@@ -777,52 +777,6 @@ const Api = {
         return { data, headers };
       });
   },
-  /**
-   * Generates a dynamic route URL based off a server provided stubbed URL
-   *
-   * Example:
-   *
-   * API: https://docs.gitlab.com/ee/api/job_artifacts.html#download-the-artifacts-archive
-   *
-   * User provides string to the frontend from:
-   *
-   * expose_path(api_v4_projects_pipelines_jobs_artifacts_path(id: project.id, ref_name: ':ref_name'))
-   *
-   * Rails route helper generated string:
-   *
-   * https://gitlab.example.com/api/v4/projects/1234/jobs/artifacts/:ref_name/download
-   *
-   * User calls:
-   *
-   *  getApiPath(this.artifactsPath, {
-   *   params { ':ref_name': 'master' },
-   *   query: {job: 'job-name'}
-   *  }
-   *
-   * Output is:
-   *
-   * https://gitlab.example.com/api/v4/projects/1234/jobs/artifacts/master/download?job=job-name
-   *
-   * @param {String} path The stubbed url
-   * @param {Object} params The stubbed values to replace
-   * @param {Object} query The url query params
-   * @returns {String} The dynamically generated URL
-   */
-  getApiPath(path = '', { params = {}, query = {} }) {
-    if (path) {
-      let outputPath = path;
-      const outputQuery = new URLSearchParams(query);
-
-      // Replace the :ruby_symbols
-      Object.entries(params).forEach(([key, value]) => {
-        outputPath = outputPath.replace(key, value);
-      });
-
-      // Construct the url query params
-      return Object.keys(query).length ? `${outputPath}?${outputQuery}` : outputPath;
-    }
-    return path;
-  },
 };
 
 export default Api;

ee/app/assets/javascripts/security_dashboard/store/modules/pipeline_jobs/actions.js

 import * as Sentry from '~/sentry/wrapper';
 import axios from '~/lib/utils/axios_utils';
+import Api from '~/api';
 import * as types from './mutation_types';
 
 export const setPipelineJobsPath = ({ commit }, path) => commit(types.SET_PIPELINE_JOBS_PATH, path);
 
 export const setProjectId = ({ commit }, id) => commit(types.SET_PROJECT_ID, id);
 
+export const setPipelineId = ({ commit }, id) => commit(types.SET_PIPELINE_ID, id);
+
 export const fetchPipelineJobs = ({ commit, state }) => {
-  if (!state.pipelineJobsPath) {
+  if (!state.pipelineJobsPath && !(state.projectId && state.pipelineId)) {
     return commit(types.RECEIVE_PIPELINE_JOBS_ERROR);
   }
   commit(types.REQUEST_PIPELINE_JOBS);
 
-  return axios({
-    method: 'GET',
-    url: state.pipelineJobsPath,
-  })
+  let requestPromise;
+
+  // Support existing usages that rely on server provided path,
+  // otherwise generate client side
+  if (state.pipelineJobsPath) {
+    requestPromise = axios({
+      method: 'GET',
+      url: state.pipelineJobsPath,
+    });
+  } else {
+    requestPromise = Api.pipelineJobs(state.projectId, state.pipelineId);
+  }
+
+  return requestPromise
     .then(response => {
       const { data } = response;
       commit(types.RECEIVE_PIPELINE_JOBS_SUCCESS, data);

ee/app/assets/javascripts/security_dashboard/store/modules/pipeline_jobs/mutation_types.js

 export const SET_PIPELINE_JOBS_PATH = 'SET_PIPELINE_JOBS_PATH';
 export const SET_PROJECT_ID = 'SET_PROJECT_ID ';
+export const SET_PIPELINE_ID = 'SET_PIPELINE_ID ';
 
 export const REQUEST_PIPELINE_JOBS = 'REQUEST_PIPELINE_JOBS';
 export const RECEIVE_PIPELINE_JOBS_SUCCESS = 'RECEIVE_PIPELINE_JOBS_SUCESS';

ee/app/assets/javascripts/security_dashboard/store/modules/pipeline_jobs/mutations.js

@@ -7,6 +7,9 @@ export default {
   [types.SET_PROJECT_ID](state, payload) {
     state.projectId = payload;
   },
+  [types.SET_PIPELINE_ID](state, payload) {
+    state.pipelineId = payload;
+  },
   [types.REQUEST_PIPELINE_JOBS](state) {
     state.isLoading = true;
   },

ee/app/assets/javascripts/security_dashboard/store/modules/pipeline_jobs/state.js

 export default () => ({
   projectId: undefined,
+  pipelineId: undefined,
   pipelineJobsPath: '',
   isLoading: false,
   pipelineJobs: [],

ee/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue

@@ -326,6 +326,7 @@ export default {
         :pipeline-path="mr.pipeline.path"
         :pipeline-id="mr.securityReportsPipelineId"
         :pipeline-iid="mr.securityReportsPipelineIid"
+        :project-id="mr.targetProjectId"
         :project-full-path="mr.sourceProjectFullPath"
         :diverged-commits-count="mr.divergedCommitsCount"
         :mr-state="mr.state"

ee/app/assets/javascripts/vue_shared/security_reports/grouped_security_reports_app.vue

@@ -18,7 +18,6 @@ import { mrStates } from '~/mr_popover/constants';
 import { fetchPolicies } from '~/lib/graphql';
 import securityReportSummaryQuery from './graphql/mr_security_report_summary.graphql';
 import SecuritySummary from './components/security_summary.vue';
-import Api from '~/api';
 
 export default {
   store: createStore(),
@@ -174,6 +173,11 @@ export default {
       required: false,
       default: '',
     },
+    projectId: {
+      type: Number,
+      required: false,
+      default: null,
+    },
     projectFullPath: {
       type: String,
       required: true,
@@ -194,7 +198,6 @@ export default {
       'isDismissingVulnerability',
       'isCreatingMergeRequest',
     ]),
-    ...mapState('pipelineJobs', ['projectId']),
     ...mapGetters([
       'groupedSummaryText',
       'summaryStatus',
@@ -279,7 +282,9 @@ export default {
       this.createVulnerabilityFeedbackMergeRequestPath,
     );
     this.setCreateVulnerabilityFeedbackDismissalPath(this.createVulnerabilityFeedbackDismissalPath);
+    this.setProjectId(this.projectId);
     this.setPipelineId(this.pipelineId);
+    this.setPipelineJobsId(this.pipelineId);
 
     const sastDiffEndpoint = gl?.mrWidgetData?.sast_comparison_path;
 
@@ -316,15 +321,10 @@ export default {
     }
 
     const coverageFuzzingDiffEndpoint = gl?.mrWidgetData?.coverage_fuzzing_comparison_path;
-    const pipelineJobsPath = Api.getApiPath(gl?.mrWidgetData?.pipeline_jobs_path, {
-      params: { ':pipeline_id': gl?.mrWidgetData?.pipeline_id },
-    });
 
     if (coverageFuzzingDiffEndpoint && this.hasCoverageFuzzingReports) {
       this.setCoverageFuzzingDiffEndpoint(coverageFuzzingDiffEndpoint);
       this.fetchCoverageFuzzingDiff();
-      this.setPipelineJobsPath(pipelineJobsPath);
-      this.setProjectId(gl?.mrWidgetData?.project_id);
       this.fetchPipelineJobs();
     }
   },
@@ -368,6 +368,9 @@ export default {
       fetchSastDiff: 'fetchDiff',
     }),
     ...mapActions('pipelineJobs', ['fetchPipelineJobs', 'setPipelineJobsPath', 'setProjectId']),
+    ...mapActions('pipelineJobs', {
+      setPipelineJobsId: 'setPipelineId',
+    }),
   },
   summarySlots: ['success', 'error', 'loading'],
 };

ee/app/views/projects/merge_requests/show.html.haml

@@ -20,5 +20,3 @@
     window.gl.mrWidgetData.sast_comparison_path = '#{sast_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:sast)}'
     window.gl.mrWidgetData.dast_comparison_path = '#{dast_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:dast)}'
     window.gl.mrWidgetData.secret_scanning_comparison_path = '#{secret_detection_reports_project_merge_request_path(@project, @merge_request) if @project.feature_available?(:secret_detection)}'
-    window.gl.mrWidgetData.pipeline_jobs_path = '#{expose_path(api_v4_projects_pipelines_jobs_path(id: @project.id, pipeline_id: ":pipeline_id"))}'
-    window.gl.mrWidgetData.project_id = #{@project.id}

ee/spec/frontend/security_dashboard/store/modules/pipeline_jobs/actions_spec.js

@@ -54,6 +54,26 @@ describe('pipeling jobs actions', () => {
     });
   });
 
+  describe('setPipelineId', () => {
+    const pipelineId = 123;
+
+    it('should commit the SET_PIPELINE_ID mutation', done => {
+      testAction(
+        actions.setPipelineId,
+        pipelineId,
+        state,
+        [
+          {
+            type: types.SET_PIPELINE_ID,
+            payload: pipelineId,
+          },
+        ],
+        [],
+        done,
+      );
+    });
+  });
+
   describe('fetchPipelineJobs', () => {
     let mock;
     const jobs = [{}, {}];
@@ -67,7 +87,7 @@ describe('pipeling jobs actions', () => {
       mock.restore();
     });
 
-    describe('on success', () => {
+    describe('on success with pipeline path', () => {
       beforeEach(() => {
         mock.onGet(state.pipelineJobsPath).replyOnce(200, jobs);
       });
@@ -90,6 +110,33 @@ describe('pipeling jobs actions', () => {
       });
     });
 
+    describe('on success with pipeline id and project id', () => {
+      beforeEach(() => {
+        mock.onGet('/api/undefined/projects/123/pipelines/321/jobs').replyOnce(200, jobs);
+      });
+
+      it('should commit the request and success mutations', done => {
+        state.pipelineJobsPath = '';
+        state.projectId = 123;
+        state.pipelineId = 321;
+
+        testAction(
+          actions.fetchPipelineJobs,
+          {},
+          state,
+          [
+            { type: types.REQUEST_PIPELINE_JOBS },
+            {
+              type: types.RECEIVE_PIPELINE_JOBS_SUCCESS,
+              payload: jobs,
+            },
+          ],
+          [],
+          done,
+        );
+      });
+    });
+
     describe('without pipelineJobsPath set', () => {
       beforeEach(() => {
         mock.onGet(state.pipelineJobsPath).replyOnce(200, jobs);
@@ -113,6 +160,31 @@ describe('pipeling jobs actions', () => {
       });
     });
 
+    describe('without projectId or pipelineId set', () => {
+      beforeEach(() => {
+        mock.onGet(state.pipelineJobsPath).replyOnce(200, jobs);
+      });
+
+      it('should commit RECEIVE_PIPELINE_JOBS_ERROR mutation', done => {
+        state.pipelineJobsPath = '';
+        state.projectId = undefined;
+        state.pipelineId = undefined;
+
+        testAction(
+          actions.fetchPipelineJobs,
+          {},
+          state,
+          [
+            {
+              type: types.RECEIVE_PIPELINE_JOBS_ERROR,
+            },
+          ],
+          [],
+          done,
+        );
+      });
+    });
+
     describe('with server error', () => {
       beforeEach(() => {
         mock.onGet(state.pipelineJobsPath).replyOnce(404);

ee/spec/frontend/security_dashboard/store/modules/pipeline_jobs/mutations_spec.js

@@ -26,6 +26,15 @@ describe('pipeline jobs module mutations', () => {
     });
   });
 
+  describe('SET_PIPELINE_ID', () => {
+    const pipelineId = 123;
+
+    it(`should set the pipelineId  to ${pipelineId}`, () => {
+      mutations[types.SET_PIPELINE_ID](state, pipelineId);
+      expect(state.pipelineId).toBe(pipelineId);
+    });
+  });
+
   describe('REQUEST_PIPELINE_JOBS', () => {
     it('should set the isLoading to true', () => {
       mutations[types.REQUEST_PIPELINE_JOBS](state);

ee/spec/frontend/vue_shared/security_reports/grouped_security_reports_app_spec.js

@@ -51,6 +51,7 @@ describe('Grouped security reports app', () => {
     vulnerabilityFeedbackHelpPath: 'path',
     coverageFuzzingHelpPath: 'path',
     pipelineId: 123,
+    projectId: 321,
     projectFullPath: 'path',
   };
 
@@ -113,7 +114,6 @@ describe('Grouped security reports app', () => {
       gl.mrWidgetData.sast_comparison_path = SAST_DIFF_ENDPOINT;
       gl.mrWidgetData.secret_scanning_comparison_path = SECRET_SCANNING_DIFF_ENDPOINT;
       gl.mrWidgetData.coverage_fuzzing_comparison_path = COVERAGE_FUZZING_DIFF_ENDPOINT;
-      gl.mrWidgetData.pipeline_jobs_path = PIPELINE_JOBS_ENDPOINT;
     });
 
     describe('with error', () => {