From 1682504edf6d6e310bc956edbefe86591f043cf8 Mon Sep 17 00:00:00 2001
From: Jannik Lehmann <jlehmann@gitlab.com>
Date: Wed, 13 Dec 2023 19:16:48 +0000
Subject: [PATCH] Add Slideshow for SAST and CodeQuality Inline Findings
This commit is meant to solve: https://gitlab.com/gitlab-org/gitlab/-/issues/417033
It introduces a slideshow on the Inline findings
drawer when there are multiple findings.
EE: true
---
.../components/shared/findings_drawer.vue | 99 +++++++++++----
.../inline_findings_gutter_icon_dropdown.vue | 28 +++--
...line_findings_gutter_icon_dropdown_spec.js | 51 ++++++++
.../findings_drawer_spec.js.snap | 14 ++-
.../components/shared/findings_drawer_spec.js | 113 ++++++++++++++----
.../diffs/mock_data/findings_drawer.js | 42 +++++++
6 files changed, 278 insertions(+), 69 deletions(-)
diff --git a/app/assets/javascripts/diffs/components/shared/findings_drawer.vue b/app/assets/javascripts/diffs/components/shared/findings_drawer.vue
index 6a141ee28bab..854f6910fa1b 100644
--- a/app/assets/javascripts/diffs/components/shared/findings_drawer.vue
+++ b/app/assets/javascripts/diffs/components/shared/findings_drawer.vue
@@ -1,5 +1,5 @@
<script>
-import { GlBadge, GlDrawer, GlIcon, GlLink } from '@gitlab/ui';
+import { GlBadge, GlDrawer, GlLink, GlButton, GlIcon } from '@gitlab/ui';
import { __, s__ } from '~/locale';
import { DRAWER_Z_INDEX } from '~/lib/utils/constants';
import { getSeverity } from '~/ci/reports/utils';
@@ -27,7 +27,7 @@ export const codeQuality = 'codeQuality';
export default {
i18n,
codeQuality,
- components: { GlBadge, GlDrawer, GlIcon, GlLink, DrawerItem },
+ components: { GlBadge, GlDrawer, GlLink, GlButton, GlIcon, DrawerItem },
props: {
drawer: {
type: Object,
@@ -39,22 +39,50 @@ export default {
default: () => {},
},
},
+ data() {
+ return {
+ activeIndex: 0,
+ };
+ },
computed: {
getDrawerHeaderHeight() {
return getContentWrapperHeight();
},
isCodeQuality() {
- return this.drawer.scale === this.$options.codeQuality;
+ return this.activeElement.scale === this.$options.codeQuality;
+ },
+ activeElement() {
+ return this.drawer.findings[this.activeIndex];
},
findingsStatus() {
- return this.drawer.state === SAST_FINDING_DISMISSED ? 'muted' : 'warning';
+ return this.activeElement.state === SAST_FINDING_DISMISSED ? 'muted' : 'warning';
},
},
DRAWER_Z_INDEX,
+ watch: {
+ drawer(newVal) {
+ this.activeIndex = newVal.index;
+ },
+ },
methods: {
getSeverity,
+ prev() {
+ if (this.activeIndex === 0) {
+ this.activeIndex = this.drawer.findings.length - 1;
+ } else {
+ this.activeIndex -= 1;
+ }
+ },
+ next() {
+ if (this.activeIndex === this.drawer.findings.length - 1) {
+ this.activeIndex = 0;
+ } else {
+ this.activeIndex += 1;
+ }
+ },
+
concatIdentifierName(name, index) {
- return name + (index !== this.drawer.identifiers.length - 1 ? ', ' : '');
+ return name + (index !== this.activeElement.identifiers.length - 1 ? ', ' : '');
},
},
};
@@ -68,38 +96,51 @@ export default {
@close="$emit('close')"
>
<template #title>
- <h2 class="drawer-heading gl-font-base gl-mt-0 gl-mb-0">
+ <h2 class="drawer-heading gl-font-base gl-mt-0 gl-mb-0 gl-w-28">
<gl-icon
:size="12"
- :name="getSeverity(drawer).name"
- :class="getSeverity(drawer).class"
+ :name="getSeverity(activeElement).name"
+ :class="getSeverity(activeElement).class"
class="inline-findings-severity-icon gl-vertical-align-baseline!"
/>
- <span class="drawer-heading-severity">{{ drawer.severity }}</span>
+ <span class="drawer-heading-severity">{{ activeElement.severity }}</span>
{{ isCodeQuality ? $options.i18n.codeQualityFinding : $options.i18n.sastFinding }}
</h2>
+ <div v-if="drawer.findings.length > 1">
+ <gl-button data-testid="findings-drawer-prev-button" class="gl-p-1!" @click="prev">
+ <gl-icon :size="24" name="chevron-left" />
+ </gl-button>
+ <gl-button class="gl-p-1!" @click="next">
+ <gl-icon data-testid="findings-drawer-next-button" :size="24" name="chevron-right" />
+ </gl-button>
+ </div>
</template>
<template #default>
<ul class="gl-list-style-none gl-border-b-initial gl-mb-0 gl-pb-0!">
- <drawer-item v-if="drawer.title" :description="$options.i18n.name" :value="drawer.title" />
+ <drawer-item
+ v-if="activeElement.title"
+ :description="$options.i18n.name"
+ :value="activeElement.title"
+ data-testid="findings-drawer-title"
+ />
- <drawer-item v-if="drawer.state" :description="$options.i18n.status">
+ <drawer-item v-if="activeElement.state" :description="$options.i18n.status">
<template #value>
<gl-badge :variant="findingsStatus" class="text-capitalize">{{
- drawer.state
+ activeElement.state
}}</gl-badge>
</template>
</drawer-item>
<drawer-item
- v-if="drawer.description"
+ v-if="activeElement.description"
:description="$options.i18n.description"
- :value="drawer.description"
+ :value="activeElement.description"
/>
<drawer-item
- v-if="project && drawer.scale !== $options.codeQuality"
+ v-if="project && activeElement.scale !== $options.codeQuality"
:description="$options.i18n.project"
>
<template #value>
@@ -107,23 +148,31 @@ export default {
</template>
</drawer-item>
- <drawer-item v-if="drawer.location || drawer.webUrl" :description="$options.i18n.file">
+ <drawer-item
+ v-if="activeElement.location || activeElement.webUrl"
+ :description="$options.i18n.file"
+ >
<template #value>
- <span v-if="drawer.webUrl && drawer.filePath && drawer.line">
- <gl-link :href="drawer.webUrl">{{ drawer.filePath }}:{{ drawer.line }}</gl-link>
+ <span v-if="activeElement.webUrl && activeElement.filePath && activeElement.line">
+ <gl-link :href="activeElement.webUrl"
+ >{{ activeElement.filePath }}:{{ activeElement.line }}</gl-link
+ >
</span>
- <span v-else-if="drawer.location">
- {{ drawer.location.file }}:{{ drawer.location.startLine }}
+ <span v-else-if="activeElement.location">
+ {{ activeElement.location.file }}:{{ activeElement.location.startLine }}
</span>
</template>
</drawer-item>
<drawer-item
- v-if="drawer.identifiers && drawer.identifiers.length"
+ v-if="activeElement.identifiers && activeElement.identifiers.length"
:description="$options.i18n.identifiers"
>
<template #value>
- <span v-for="(identifier, index) in drawer.identifiers" :key="identifier.externalId">
+ <span
+ v-for="(identifier, index) in activeElement.identifiers"
+ :key="identifier.externalId"
+ >
<gl-link v-if="identifier.url" :href="identifier.url">
{{ concatIdentifierName(identifier.name, index) }}
</gl-link>
@@ -135,15 +184,15 @@ export default {
</drawer-item>
<drawer-item
- v-if="drawer.scale"
+ v-if="activeElement.scale"
:description="$options.i18n.tool"
:value="isCodeQuality ? $options.i18n.codeQuality : $options.i18n.sast"
/>
<drawer-item
- v-if="drawer.engineName"
+ v-if="activeElement.engineName"
:description="$options.i18n.engine"
- :value="drawer.engineName"
+ :value="activeElement.engineName"
/>
</ul>
</template>
diff --git a/ee/app/assets/javascripts/diffs/components/inline_findings_gutter_icon_dropdown.vue b/ee/app/assets/javascripts/diffs/components/inline_findings_gutter_icon_dropdown.vue
index ed5587ecd1a2..e966ed966672 100644
--- a/ee/app/assets/javascripts/diffs/components/inline_findings_gutter_icon_dropdown.vue
+++ b/ee/app/assets/javascripts/diffs/components/inline_findings_gutter_icon_dropdown.vue
@@ -77,18 +77,22 @@ export default {
});
}
- groupedFindings.forEach((e) => {
- e.items.map((arr) => {
- // enhance groupedFindings to match GlDisclosureDropdown validator
- // https://gitlab-org.gitlab.io/gitlab-ui/?path=/docs/base-new-dropdowns-disclosure--docs#setting-disclosure-dropdown-items
- const enhancedGroupedFindings = arr;
- enhancedGroupedFindings.text = arr.description;
- enhancedGroupedFindings.action = () => this.toggleDrawer(arr);
+ const allLineFindings = this.flatFindings;
- return enhancedGroupedFindings;
- });
+ // Enhance each finding with the correct index and action
+ allLineFindings.forEach((finding, index) => {
+ /* eslint-disable no-param-reassign */
+ finding.action = () => this.toggleDrawer(allLineFindings, index);
+ // enhance to match GlDisclosureDropdown validator
+ // https://gitlab-org.gitlab.io/gitlab-ui/?path=/docs/base-new-dropdowns-disclosure--docs#setting-disclosure-dropdown-items
+ finding.text = finding.description;
+ /* eslint-enable no-param-reassign */
});
- return groupedFindings;
+
+ return groupedFindings.map((group) => ({
+ ...group,
+ items: group.items.map((item) => allLineFindings.find((f) => f === item)),
+ }));
},
showMoreCount() {
return this.moreCount && this.isHoveringFirstIcon;
@@ -106,8 +110,8 @@ export default {
},
},
methods: {
- toggleDrawer(finding) {
- this.setDrawer(finding);
+ toggleDrawer(findings, index) {
+ this.setDrawer({ findings, index });
},
...mapActions('findingsDrawer', ['setDrawer']),
},
diff --git a/ee/spec/frontend/diffs/components/inline_findings_gutter_icon_dropdown_spec.js b/ee/spec/frontend/diffs/components/inline_findings_gutter_icon_dropdown_spec.js
index a5c393eb7e84..4c3da1e83c0a 100644
--- a/ee/spec/frontend/diffs/components/inline_findings_gutter_icon_dropdown_spec.js
+++ b/ee/spec/frontend/diffs/components/inline_findings_gutter_icon_dropdown_spec.js
@@ -109,6 +109,57 @@ describe('EE inlineFindingsGutterIconDropdown', () => {
await itemElements.at(1).trigger('click');
expect(mockSetDrawer).toHaveBeenCalledTimes(2);
});
+
+ it('calls setDrawer action with correct allLineFindings and index when an item action is triggered', async () => {
+ createComponent({
+ filePath,
+ codeQuality: singularCodeQualityFinding,
+ sast: singularSastFinding,
+ });
+
+ const itemElements = findDropdownItems();
+ await itemElements.at(0).trigger('click');
+ const firstCallFirstArg = mockSetDrawer.mock.calls[0][1];
+
+ expect(firstCallFirstArg).toEqual({
+ findings: [
+ {
+ ...singularCodeQualityFinding[0],
+ action: expect.any(Function),
+ class: 'gl-text-orange-300',
+ name: 'severity-low',
+ },
+ {
+ ...singularSastFinding[0],
+ action: expect.any(Function),
+ class: 'gl-text-orange-300',
+ name: 'severity-low',
+ },
+ ],
+ index: 0,
+ });
+
+ await itemElements.at(1).trigger('click');
+ const secondCall = mockSetDrawer.mock.calls[1][1];
+
+ expect(secondCall).toEqual({
+ findings: [
+ {
+ ...singularCodeQualityFinding[0],
+ action: expect.any(Function),
+ class: 'gl-text-orange-300',
+ name: 'severity-low',
+ },
+ {
+ ...singularSastFinding[0],
+ action: expect.any(Function),
+ class: 'gl-text-orange-300',
+ name: 'severity-low',
+ },
+ ],
+ index: 1,
+ });
+ });
});
it('sets "isHoveringFirstIcon" to true when mouse enters the first icon', async () => {
diff --git a/spec/frontend/diffs/components/shared/__snapshots__/findings_drawer_spec.js.snap b/spec/frontend/diffs/components/shared/__snapshots__/findings_drawer_spec.js.snap
index d7364e805423..33a268c06ccc 100644
--- a/spec/frontend/diffs/components/shared/__snapshots__/findings_drawer_spec.js.snap
+++ b/spec/frontend/diffs/components/shared/__snapshots__/findings_drawer_spec.js.snap
@@ -1,6 +1,6 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
-exports[`FindingsDrawer matches the snapshot with detected badge 1`] = `
+exports[`FindingsDrawer General Rendering matches the snapshot with detected badge 1`] = `
<transition-stub
class="findings-drawer"
name="gl-drawer"
@@ -16,7 +16,7 @@ exports[`FindingsDrawer matches the snapshot with detected badge 1`] = `
class="gl-drawer-title"
>
<h2
- class="drawer-heading gl-font-base gl-mb-0 gl-mt-0"
+ class="drawer-heading gl-font-base gl-mb-0 gl-mt-0 gl-w-28"
>
<svg
aria-hidden="true"
@@ -61,6 +61,7 @@ exports[`FindingsDrawer matches the snapshot with detected badge 1`] = `
>
<li
class="gl-mb-4"
+ data-testid="findings-drawer-title"
>
<p
class="gl-line-height-20"
@@ -219,7 +220,7 @@ exports[`FindingsDrawer matches the snapshot with detected badge 1`] = `
</transition-stub>
`;
-exports[`FindingsDrawer matches the snapshot with dismissed badge 1`] = `
+exports[`FindingsDrawer General Rendering matches the snapshot with dismissed badge 1`] = `
<transition-stub
class="findings-drawer"
name="gl-drawer"
@@ -235,7 +236,7 @@ exports[`FindingsDrawer matches the snapshot with dismissed badge 1`] = `
class="gl-drawer-title"
>
<h2
- class="drawer-heading gl-font-base gl-mb-0 gl-mt-0"
+ class="drawer-heading gl-font-base gl-mb-0 gl-mt-0 gl-w-28"
>
<svg
aria-hidden="true"
@@ -280,6 +281,7 @@ exports[`FindingsDrawer matches the snapshot with dismissed badge 1`] = `
>
<li
class="gl-mb-4"
+ data-testid="findings-drawer-title"
>
<p
class="gl-line-height-20"
@@ -310,9 +312,9 @@ exports[`FindingsDrawer matches the snapshot with dismissed badge 1`] = `
Status
</span>
<span
- class="badge badge-muted badge-pill gl-badge md text-capitalize"
+ class="badge badge-pill badge-warning gl-badge md text-capitalize"
>
- dismissed
+ detected
</span>
</p>
</li>
diff --git a/spec/frontend/diffs/components/shared/findings_drawer_spec.js b/spec/frontend/diffs/components/shared/findings_drawer_spec.js
index a6528978e3bf..00b4ca262be5 100644
--- a/spec/frontend/diffs/components/shared/findings_drawer_spec.js
+++ b/spec/frontend/diffs/components/shared/findings_drawer_spec.js
@@ -1,3 +1,4 @@
+import { nextTick } from 'vue';
import { GlDrawer } from '@gitlab/ui';
import FindingsDrawer from '~/diffs/components/shared/findings_drawer.vue';
import { mountExtended } from 'helpers/vue_test_utils_helper';
@@ -5,41 +6,101 @@ import {
mockFindingDismissed,
mockFindingDetected,
mockProject,
+ mockFindingsMultiple,
} from '../../mock_data/findings_drawer';
-let wrapper;
-const getDrawer = () => wrapper.findComponent(GlDrawer);
-const closeEvent = 'close';
+describe('FindingsDrawer', () => {
+ let wrapper;
-const createWrapper = (finding = mockFindingDismissed) => {
- return mountExtended(FindingsDrawer, {
- propsData: {
- drawer: finding,
- project: mockProject,
- },
- });
-};
+ const findPreviousButton = () => wrapper.findByTestId('findings-drawer-prev-button');
+ const findNextButton = () => wrapper.findByTestId('findings-drawer-next-button');
+ const findTitle = () => wrapper.findByTestId('findings-drawer-title');
+ const createWrapper = (
+ drawer = { findings: [mockFindingDetected], index: 0 },
+ project = mockProject,
+ ) => {
+ return mountExtended(FindingsDrawer, {
+ propsData: {
+ drawer,
+ project,
+ },
+ });
+ };
-describe('FindingsDrawer', () => {
- it('renders without errors', () => {
- wrapper = createWrapper();
- expect(wrapper.exists()).toBe(true);
- });
+ describe('General Rendering', () => {
+ beforeEach(() => {
+ wrapper = createWrapper();
+ });
+ it('renders without errors', () => {
+ expect(wrapper.exists()).toBe(true);
+ });
- it('emits close event when gl-drawer emits close event', () => {
- wrapper = createWrapper();
+ it('emits close event when gl-drawer emits close event', () => {
+ wrapper.findComponent(GlDrawer).vm.$emit('close');
+ expect(wrapper.emitted('close')).toHaveLength(1);
+ });
- getDrawer().vm.$emit(closeEvent);
- expect(wrapper.emitted(closeEvent)).toHaveLength(1);
+ it('matches the snapshot with dismissed badge', () => {
+ expect(wrapper.element).toMatchSnapshot();
+ });
+
+ it('matches the snapshot with detected badge', () => {
+ expect(wrapper.element).toMatchSnapshot();
+ });
});
- it('matches the snapshot with dismissed badge', () => {
- wrapper = createWrapper();
- expect(wrapper.element).toMatchSnapshot();
+ describe('Prev/Next Buttons with Multiple Items', () => {
+ it('renders prev/next buttons when there are multiple items', () => {
+ wrapper = createWrapper({ findings: mockFindingsMultiple, index: 0 });
+ expect(findPreviousButton().exists()).toBe(true);
+ expect(findNextButton().exists()).toBe(true);
+ });
+
+ it('does not render prev/next buttons when there is only one item', () => {
+ wrapper = createWrapper({ findings: [mockFindingDismissed], index: 0 });
+ expect(findPreviousButton().exists()).toBe(false);
+ expect(findNextButton().exists()).toBe(false);
+ });
+
+ it('calls prev method on prev button click and loops correct activeIndex', async () => {
+ wrapper = createWrapper({ findings: mockFindingsMultiple, index: 0 });
+ expect(findTitle().text()).toBe(`Name ${mockFindingsMultiple[0].title}`);
+
+ await findPreviousButton().trigger('click');
+ await nextTick();
+ expect(findTitle().text()).toBe(`Name ${mockFindingsMultiple[2].title}`);
+
+ await findPreviousButton().trigger('click');
+ await nextTick();
+ expect(findTitle().text()).toBe(`Name ${mockFindingsMultiple[1].title}`);
+ });
+
+ it('calls next method on next button click', async () => {
+ wrapper = createWrapper({ findings: mockFindingsMultiple, index: 0 });
+ expect(findTitle().text()).toBe(`Name ${mockFindingsMultiple[0].title}`);
+
+ await findNextButton().trigger('click');
+ await nextTick();
+ expect(findTitle().text()).toBe(`Name ${mockFindingsMultiple[1].title}`);
+
+ await findNextButton().trigger('click');
+ await nextTick();
+ expect(findTitle().text()).toBe(`Name ${mockFindingsMultiple[2].title}`);
+
+ await findNextButton().trigger('click');
+ await nextTick();
+ expect(findTitle().text()).toBe(`Name ${mockFindingsMultiple[0].title}`);
+ });
});
- it('matches the snapshot with detected badge', () => {
- wrapper = createWrapper(mockFindingDetected);
- expect(wrapper.element).toMatchSnapshot();
+ describe('Active Index Handling', () => {
+ it('watcher sets active index on drawer prop change', async () => {
+ wrapper = createWrapper();
+ const newFinding = { findings: mockFindingsMultiple, index: 2 };
+
+ await wrapper.setProps({ drawer: newFinding });
+ await nextTick();
+ expect(findTitle().text()).toBe(`Name ${mockFindingsMultiple[2].title}`);
+ });
});
});
diff --git a/spec/frontend/diffs/mock_data/findings_drawer.js b/spec/frontend/diffs/mock_data/findings_drawer.js
index f6fc33b1dcfb..257a3b3e499a 100644
--- a/spec/frontend/diffs/mock_data/findings_drawer.js
+++ b/spec/frontend/diffs/mock_data/findings_drawer.js
@@ -31,3 +31,45 @@ export const mockProject = {
nameWithNamespace: 'testname',
fullPath: 'testpath',
};
+
+export const mockFindingsMultiple = [
+ {
+ ...mockFindingDismissed,
+ title: 'Finding 1',
+ severity: 'critical',
+ engineName: 'Engine 1',
+ identifiers: [
+ {
+ ...mockFindingDismissed.identifiers[0],
+ name: 'identifier 1',
+ url: 'https://example.com/identifier1',
+ },
+ ],
+ },
+ {
+ ...mockFindingDetected,
+ title: 'Finding 2',
+ severity: 'medium',
+ engineName: 'Engine 2',
+ identifiers: [
+ {
+ ...mockFindingDetected.identifiers[0],
+ name: 'identifier 2',
+ url: 'https://example.com/identifier2',
+ },
+ ],
+ },
+ {
+ ...mockFindingDetected,
+ title: 'Finding 3',
+ severity: 'medium',
+ engineName: 'Engine 3',
+ identifiers: [
+ {
+ ...mockFindingDetected.identifiers[0],
+ name: 'identifier 3',
+ url: 'https://example.com/identifier3',
+ },
+ ],
+ },
+];
--
GitLab