From 5cd6c5bfc0bbca7055ba5a49e8cf8b4dab2824af Mon Sep 17 00:00:00 2001
From: Savas Vedova <svedova@gitlab.com>
Date: Fri, 21 Jul 2023 12:01:35 +0000
Subject: [PATCH] Use component state
Remove v-model from widget as we can rely on the parent component
state and it is unnecessary to handle that logic inside the widget
component.
---
.../components/widget/widget.vue | 21 ++----
.../mr_widget_security_reports.vue | 69 +++++++++----------
.../components/widget/widget_spec.js | 48 +++++--------
3 files changed, 54 insertions(+), 84 deletions(-)
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/widget/widget.vue b/app/assets/javascripts/vue_merge_request_widget/components/widget/widget.vue
index 123fc8d90963c..5bfe903af9609 100644
--- a/app/assets/javascripts/vue_merge_request_widget/components/widget/widget.vue
+++ b/app/assets/javascripts/vue_merge_request_widget/components/widget/widget.vue
@@ -16,8 +16,6 @@ import DynamicContent from './dynamic_content.vue';
import StatusIcon from './status_icon.vue';
import ActionButtons from './action_buttons.vue';
-const FETCH_TYPE_COLLAPSED = 'collapsed';
-const FETCH_TYPE_EXPANDED = 'expanded';
const WIDGET_PREFIX = 'Widget';
const MISSING_RESPONSE_HEADERS =
'MR Widget: raesponse object should contain status and headers object. Make sure to include that in your `fetchCollapsedData` and `fetchExpandedData` functions.';
@@ -49,15 +47,6 @@ export default {
SafeHtml,
},
props: {
- /**
- * @param {value.collapsed} Object
- * @param {value.expanded} Object
- */
- value: {
- type: Object,
- required: false,
- default: () => ({}),
- },
loadingText: {
type: String,
required: false,
@@ -238,7 +227,7 @@ export default {
try {
if (this.fetchCollapsedData) {
- await this.fetch(this.fetchCollapsedData, FETCH_TYPE_COLLAPSED);
+ await this.fetch(this.fetchCollapsedData);
}
} catch {
this.summaryError = this.errorText;
@@ -271,7 +260,7 @@ export default {
this.contentError = null;
try {
- await this.fetch(this.fetchExpandedData, FETCH_TYPE_EXPANDED);
+ await this.fetch(this.fetchExpandedData);
} catch {
this.contentError = this.errorText;
@@ -282,7 +271,7 @@ export default {
this.isLoadingExpandedContent = false;
},
- fetch(handler, dataType) {
+ fetch(handler) {
const requests = this.multiPolling ? handler() : [handler];
const promises = requests.map((request) => {
@@ -319,9 +308,7 @@ export default {
});
});
- return Promise.all(promises).then((data) => {
- this.$emit('input', { ...this.value, [dataType]: this.multiPolling ? data : data[0] });
- });
+ return Promise.all(promises);
},
},
failedStatusIcon: EXTENSION_ICONS.failed,
diff --git a/ee/app/assets/javascripts/vue_merge_request_widget/extensions/security_reports/mr_widget_security_reports.vue b/ee/app/assets/javascripts/vue_merge_request_widget/extensions/security_reports/mr_widget_security_reports.vue
index e280bdcc668dc..34f210d497d22 100644
--- a/ee/app/assets/javascripts/vue_merge_request_widget/extensions/security_reports/mr_widget_security_reports.vue
+++ b/ee/app/assets/javascripts/vue_merge_request_widget/extensions/security_reports/mr_widget_security_reports.vue
@@ -60,10 +60,7 @@ export default {
isCreatingMergeRequest: false,
hasAtLeastOneReportWithMaxNewVulnerabilities: false,
modalData: null,
- vulnerabilities: {
- collapsed: null,
- expanded: null,
- },
+ collapsedData: {},
};
},
@@ -100,6 +97,7 @@ export default {
}
const issue = finding.issueLinks?.nodes.find((x) => x.linkType === 'CREATED')?.issue;
+
if (issue) {
this.$set(this.modalData.vulnerability, 'hasIssue', true);
@@ -125,6 +123,12 @@ export default {
},
computed: {
+ reports() {
+ return this.endpoints
+ .map(([, reportType]) => this.collapsedData[reportType])
+ .filter((r) => r);
+ },
+
helpPopovers() {
return {
SAST: {
@@ -172,21 +176,17 @@ export default {
},
isCollapsible() {
- if (!this.vulnerabilities.collapsed) {
- return false;
- }
-
return this.vulnerabilitiesCount > 0;
},
vulnerabilitiesCount() {
- return this.vulnerabilities.collapsed.reduce((counter, current) => {
+ return this.reports.reduce((counter, current) => {
return counter + current.numberOfNewFindings + (current.fixed?.length || 0);
}, 0);
},
highlights() {
- if (!this.vulnerabilities.collapsed) {
+ if (!this.reports.length) {
return {};
}
@@ -202,19 +202,13 @@ export default {
// { scanner: "DAST", added: [{ id: 15, severity: 'high' }] },
// ...
// ]
- this.vulnerabilities.collapsed.forEach((report) =>
- this.highlightsFromReport(report, highlights),
- );
+ this.reports.forEach((report) => this.highlightsFromReport(report, highlights));
return highlights;
},
totalNewVulnerabilities() {
- if (!this.vulnerabilities.collapsed) {
- return 0;
- }
-
- return this.vulnerabilities.collapsed.reduce((counter, current) => {
+ return this.reports.reduce((counter, current) => {
return counter + (current.numberOfNewFindings || 0);
}, 0);
},
@@ -308,26 +302,30 @@ export default {
this.hasAtLeastOneReportWithMaxNewVulnerabilities = true;
}
+ const report = {
+ ...props,
+ ...data,
+ added,
+ fixed,
+ findings: [...added, ...fixed],
+ numberOfNewFindings: added.length,
+ numberOfFixedFindings: fixed.length,
+ qaSelector: this.$options.qaSelectors[reportType],
+ };
+
+ this.$set(this.collapsedData, reportType, report);
+
return {
headers,
status,
- data: {
- ...props,
- ...data,
- added,
- fixed,
- findings: [...added, ...fixed],
- numberOfNewFindings: added.length,
- numberOfFixedFindings: fixed.length,
- qaSelector: this.$options.qaSelectors[reportType],
- },
+ data: report,
};
})
- .catch(({ headers = {}, status = 500 }) => ({
- headers,
- status,
- data: { ...props, error: true },
- }));
+ .catch(({ headers = {}, status = 500 }) => {
+ const report = { ...props, error: true };
+ this.$set(this.collapsedData, reportType, report);
+ return { headers, status, data: report };
+ });
});
},
@@ -396,11 +394,11 @@ export default {
})
.then(({ data }) => {
const url = getCreatedIssueForVulnerability(data).issue_url;
-
visitUrl(url);
})
.catch(() => {
this.isCreatingIssue = false;
+
this.modalData.error = s__(
'ciReport|There was an error creating the issue. Please try again.',
);
@@ -609,7 +607,6 @@ export default {
<template>
<mr-widget
v-if="shouldRenderMrWidget"
- v-model="vulnerabilities"
:error-text="$options.i18n.error"
:fetch-collapsed-data="fetchCollapsedData"
:status-icon-name="statusIconName"
@@ -662,7 +659,7 @@ export default {
:project-full-path="mr.sourceProjectFullPath"
/>
<mr-widget-row
- v-for="report in vulnerabilities.collapsed"
+ v-for="report in reports"
:key="report.reportType"
:widget-name="$options.name"
:level="2"
diff --git a/spec/frontend/vue_merge_request_widget/components/widget/widget_spec.js b/spec/frontend/vue_merge_request_widget/components/widget/widget_spec.js
index 9343a3a5e906e..18fdba32f5242 100644
--- a/spec/frontend/vue_merge_request_widget/components/widget/widget_spec.js
+++ b/spec/frontend/vue_merge_request_widget/components/widget/widget_spec.js
@@ -121,14 +121,15 @@ describe('~/vue_merge_request_widget/components/widget/widget.vue', () => {
});
describe('fetch', () => {
- it('sets the data.collapsed property after a successfull call - multiPolling: false', async () => {
+ it('calls fetchCollapsedData properly when multiPolling is false', async () => {
const mockData = { headers: {}, status: HTTP_STATUS_OK, data: { vulnerabilities: [] } };
- createComponent({ propsData: { fetchCollapsedData: () => Promise.resolve(mockData) } });
+ const fetchCollapsedData = jest.fn().mockResolvedValue(mockData);
+ createComponent({ propsData: { fetchCollapsedData } });
await waitForPromises();
- expect(wrapper.emitted('input')[0][0]).toEqual({ collapsed: mockData.data, expanded: null });
+ expect(fetchCollapsedData).toHaveBeenCalledTimes(1);
});
- it('sets the data.collapsed property after a successfull call - multiPolling: true', async () => {
+ it('calls fetchCollapsedData properly when multiPolling is true', async () => {
const mockData1 = {
headers: {},
status: HTTP_STATUS_OK,
@@ -140,22 +141,22 @@ describe('~/vue_merge_request_widget/components/widget/widget.vue', () => {
data: { vulnerabilities: [{ vuln: 2 }] },
};
+ const fetchCollapsedData = [
+ jest.fn().mockResolvedValue(mockData1),
+ jest.fn().mockResolvedValue(mockData2),
+ ];
+
createComponent({
propsData: {
multiPolling: true,
- fetchCollapsedData: () => [
- () => Promise.resolve(mockData1),
- () => Promise.resolve(mockData2),
- ],
+ fetchCollapsedData: () => fetchCollapsedData,
},
});
await waitForPromises();
- expect(wrapper.emitted('input')[0][0]).toEqual({
- collapsed: [mockData1.data, mockData2.data],
- expanded: null,
- });
+ expect(fetchCollapsedData[0]).toHaveBeenCalledTimes(1);
+ expect(fetchCollapsedData[1]).toHaveBeenCalledTimes(1);
});
it('throws an error when the handler does not include headers or status objects', async () => {
@@ -328,11 +329,12 @@ describe('~/vue_merge_request_widget/components/widget/widget.vue', () => {
};
const fetchExpandedData = jest.fn().mockResolvedValue(mockDataExpanded);
+ const fetchCollapsedData = jest.fn().mockResolvedValue(mockDataCollapsed);
await createComponent({
propsData: {
isCollapsible: true,
- fetchCollapsedData: () => Promise.resolve(mockDataCollapsed),
+ fetchCollapsedData,
fetchExpandedData,
},
});
@@ -340,17 +342,8 @@ describe('~/vue_merge_request_widget/components/widget/widget.vue', () => {
findToggleButton().vm.$emit('click');
await waitForPromises();
- // First fetches the collapsed data
- expect(wrapper.emitted('input')[0][0]).toEqual({
- collapsed: mockDataCollapsed.data,
- expanded: null,
- });
-
- // Then fetches the expanded data
- expect(wrapper.emitted('input')[1][0]).toEqual({
- collapsed: null,
- expanded: mockDataExpanded.data,
- });
+ expect(fetchCollapsedData).toHaveBeenCalledTimes(1);
+ expect(fetchExpandedData).toHaveBeenCalledTimes(1);
// Triggering a click does not call the expanded data again
findToggleButton().vm.$emit('click');
@@ -371,14 +364,7 @@ describe('~/vue_merge_request_widget/components/widget/widget.vue', () => {
findToggleButton().vm.$emit('click');
await waitForPromises();
- // First fetches the collapsed data
- expect(wrapper.emitted('input')[0][0]).toEqual({
- collapsed: undefined,
- expanded: null,
- });
-
expect(fetchExpandedData).toHaveBeenCalledTimes(1);
- expect(wrapper.emitted('input')).toHaveLength(1); // Should not an emit an input call because request failed
findToggleButton().vm.$emit('click');
await waitForPromises();
--
GitLab