Commit 12d4d89f authored by Mark Florian's avatar Mark Florian Committed by Kushal Pandya

Check all pipeline jobs for security artifacts

This corrects an oversight where the basic security MR widget was only
checking for security artifacts from the first page of jobs returned by
the API. This meant that, for a pipeline with a lot of jobs, the MR
widget might incorrectly not be shown.

This change makes sure that additional pages are requested until either
the last page has been reached, or one or more security artifacts have
been found.

Addresses https://gitlab.com/gitlab-org/gitlab/-/issues/276440.
parent 5f55a666
...@@ -616,12 +616,12 @@ const Api = { ...@@ -616,12 +616,12 @@ const Api = {
return axios.get(url); return axios.get(url);
}, },
pipelineJobs(projectId, pipelineId) { pipelineJobs(projectId, pipelineId, params) {
const url = Api.buildUrl(this.pipelineJobsPath) const url = Api.buildUrl(this.pipelineJobsPath)
.replace(':id', encodeURIComponent(projectId)) .replace(':id', encodeURIComponent(projectId))
.replace(':pipeline_id', encodeURIComponent(pipelineId)); .replace(':pipeline_id', encodeURIComponent(pipelineId));
return axios.get(url); return axios.get(url, { params });
}, },
// Return all pipelines for a project or filter by query params // Return all pipelines for a project or filter by query params
......
...@@ -3,6 +3,7 @@ import { GlIcon, GlLink, GlSprintf } from '@gitlab/ui'; ...@@ -3,6 +3,7 @@ import { GlIcon, GlLink, GlSprintf } from '@gitlab/ui';
import ReportSection from '~/reports/components/report_section.vue'; import ReportSection from '~/reports/components/report_section.vue';
import { status } from '~/reports/constants'; import { status } from '~/reports/constants';
import { s__ } from '~/locale'; import { s__ } from '~/locale';
import { normalizeHeaders, parseIntPagination } from '~/lib/utils/common_utils';
import Flash from '~/flash'; import Flash from '~/flash';
import Api from '~/api'; import Api from '~/api';
...@@ -52,12 +53,27 @@ export default { ...@@ -52,12 +53,27 @@ export default {
}); });
}, },
methods: { methods: {
checkHasSecurityReports(reportTypes) { async checkHasSecurityReports(reportTypes) {
return Api.pipelineJobs(this.projectId, this.pipelineId).then(({ data: jobs }) => let page = 1;
jobs.some(({ artifacts = [] }) => while (page) {
// eslint-disable-next-line no-await-in-loop
const { data: jobs, headers } = await Api.pipelineJobs(this.projectId, this.pipelineId, {
per_page: 100,
page,
});
const hasSecurityReports = jobs.some(({ artifacts = [] }) =>
artifacts.some(({ file_type }) => reportTypes.includes(file_type)), artifacts.some(({ file_type }) => reportTypes.includes(file_type)),
),
); );
if (hasSecurityReports) {
return true;
}
page = parseIntPagination(normalizeHeaders(headers)).nextPage;
}
return false;
}, },
activatePipelinesTab() { activatePipelinesTab() {
if (window.mrTabs) { if (window.mrTabs) {
......
---
title: Ensure security report is displayed correctly in merge requests with a lot of CI jobs
merge_request: 46870
author:
type: fixed
...@@ -710,7 +710,9 @@ describe('Api', () => { ...@@ -710,7 +710,9 @@ describe('Api', () => {
}); });
describe('pipelineJobs', () => { describe('pipelineJobs', () => {
it('fetches the jobs for a given pipeline', done => { it.each([undefined, {}, { foo: true }])(
'fetches the jobs for a given pipeline given %p params',
async params => {
const projectId = 123; const projectId = 123;
const pipelineId = 456; const pipelineId = 456;
const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/projects/${projectId}/pipelines/${pipelineId}/jobs`; const expectedUrl = `${dummyUrlRoot}/api/${dummyApiVersion}/projects/${projectId}/pipelines/${pipelineId}/jobs`;
...@@ -719,15 +721,12 @@ describe('Api', () => { ...@@ -719,15 +721,12 @@ describe('Api', () => {
name: 'test', name: 'test',
}, },
]; ];
mock.onGet(expectedUrl).reply(httpStatus.OK, payload); mock.onGet(expectedUrl, { params }).reply(httpStatus.OK, payload);
Api.pipelineJobs(projectId, pipelineId) const { data } = await Api.pipelineJobs(projectId, pipelineId, params);
.then(({ data }) => {
expect(data).toEqual(payload); expect(data).toEqual(payload);
}) },
.then(done) );
.catch(done.fail);
});
}); });
describe('createBranch', () => { describe('createBranch', () => {
......
...@@ -5,7 +5,7 @@ import SecurityReportsApp from '~/vue_shared/security_reports/security_reports_a ...@@ -5,7 +5,7 @@ import SecurityReportsApp from '~/vue_shared/security_reports/security_reports_a
jest.mock('~/flash'); jest.mock('~/flash');
describe('Grouped security reports app', () => { describe('Security reports app', () => {
let wrapper; let wrapper;
let mrTabsMock; let mrTabsMock;
...@@ -21,6 +21,8 @@ describe('Grouped security reports app', () => { ...@@ -21,6 +21,8 @@ describe('Grouped security reports app', () => {
}); });
}; };
const anyParams = expect.any(Object);
const findPipelinesTabAnchor = () => wrapper.find('[data-testid="show-pipelines"]'); const findPipelinesTabAnchor = () => wrapper.find('[data-testid="show-pipelines"]');
const findHelpLink = () => wrapper.find('[data-testid="help"]'); const findHelpLink = () => wrapper.find('[data-testid="help"]');
const setupMrTabsMock = () => { const setupMrTabsMock = () => {
...@@ -43,10 +45,12 @@ describe('Grouped security reports app', () => { ...@@ -43,10 +45,12 @@ describe('Grouped security reports app', () => {
window.mrTabs = { tabShown: jest.fn() }; window.mrTabs = { tabShown: jest.fn() };
setupMockJobArtifact(reportType); setupMockJobArtifact(reportType);
createComponent(); createComponent();
return wrapper.vm.$nextTick();
}); });
it('calls the pipelineJobs API correctly', () => { it('calls the pipelineJobs API correctly', () => {
expect(Api.pipelineJobs).toHaveBeenCalledWith(props.projectId, props.pipelineId); expect(Api.pipelineJobs).toHaveBeenCalledTimes(1);
expect(Api.pipelineJobs).toHaveBeenCalledWith(props.projectId, props.pipelineId, anyParams);
}); });
it('renders the expected message', () => { it('renders the expected message', () => {
...@@ -75,10 +79,12 @@ describe('Grouped security reports app', () => { ...@@ -75,10 +79,12 @@ describe('Grouped security reports app', () => {
beforeEach(() => { beforeEach(() => {
setupMockJobArtifact('foo'); setupMockJobArtifact('foo');
createComponent(); createComponent();
return wrapper.vm.$nextTick();
}); });
it('calls the pipelineJobs API correctly', () => { it('calls the pipelineJobs API correctly', () => {
expect(Api.pipelineJobs).toHaveBeenCalledWith(props.projectId, props.pipelineId); expect(Api.pipelineJobs).toHaveBeenCalledTimes(1);
expect(Api.pipelineJobs).toHaveBeenCalledWith(props.projectId, props.pipelineId, anyParams);
}); });
it('renders nothing', () => { it('renders nothing', () => {
...@@ -86,6 +92,42 @@ describe('Grouped security reports app', () => { ...@@ -86,6 +92,42 @@ describe('Grouped security reports app', () => {
}); });
}); });
describe('security artifacts on last page of multi-page response', () => {
const numPages = 3;
beforeEach(() => {
jest
.spyOn(Api, 'pipelineJobs')
.mockImplementation(async (projectId, pipelineId, { page }) => {
const requestedPage = parseInt(page, 10);
if (requestedPage < numPages) {
return {
// Some jobs with no relevant artifacts
data: [{}, {}],
headers: { 'x-next-page': String(requestedPage + 1) },
};
} else if (requestedPage === numPages) {
return {
data: [{ artifacts: [{ file_type: SecurityReportsApp.reportTypes[0] }] }],
};
}
throw new Error('Test failed due to request of non-existent jobs page');
});
createComponent();
return wrapper.vm.$nextTick();
});
it('fetches all pages', () => {
expect(Api.pipelineJobs).toHaveBeenCalledTimes(numPages);
});
it('renders the expected message', () => {
expect(wrapper.text()).toMatchInterpolatedText(SecurityReportsApp.i18n.scansHaveRun);
});
});
describe('given an error from the API', () => { describe('given an error from the API', () => {
let error; let error;
...@@ -93,10 +135,12 @@ describe('Grouped security reports app', () => { ...@@ -93,10 +135,12 @@ describe('Grouped security reports app', () => {
error = new Error('an error'); error = new Error('an error');
jest.spyOn(Api, 'pipelineJobs').mockRejectedValue(error); jest.spyOn(Api, 'pipelineJobs').mockRejectedValue(error);
createComponent(); createComponent();
return wrapper.vm.$nextTick();
}); });
it('calls the pipelineJobs API correctly', () => { it('calls the pipelineJobs API correctly', () => {
expect(Api.pipelineJobs).toHaveBeenCalledWith(props.projectId, props.pipelineId); expect(Api.pipelineJobs).toHaveBeenCalledTimes(1);
expect(Api.pipelineJobs).toHaveBeenCalledWith(props.projectId, props.pipelineId, anyParams);
}); });
it('renders nothing', () => { it('renders nothing', () => {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment