Commit a64a5c04 authored by Phil Hughes's avatar Phil Hughes

Merge branch '2526-inline-codequality' into 'master'

Inline codequality [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57926
parents 36281d70 49f58214
......@@ -181,7 +181,6 @@ export default {
plainDiffPath: (state) => state.diffs.plainDiffPath,
emailPatchPath: (state) => state.diffs.emailPatchPath,
retrievingBatches: (state) => state.diffs.retrievingBatches,
codequalityDiff: (state) => state.diffs.codequalityDiff,
}),
...mapState('diffs', [
'showTreeList',
......
<script>
import { GlLoadingIcon } from '@gitlab/ui';
import { mapActions, mapGetters, mapState } from 'vuex';
import { mapInline, mapParallel } from 'ee_else_ce/diffs/components/diff_row_utils';
import DiffFileDrafts from '~/batch_comments/components/diff_file_drafts.vue';
import draftCommentsMixin from '~/diffs/mixins/draft_comments';
import { diffViewerModes } from '~/ide/constants';
......@@ -15,7 +16,6 @@ import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_
import { IMAGE_DIFF_POSITION_TYPE } from '../constants';
import { getDiffMode } from '../store/utils';
import DiffDiscussions from './diff_discussions.vue';
import { mapInline, mapParallel } from './diff_row_utils';
import DiffView from './diff_view.vue';
import ImageDiffOverlay from './image_diff_overlay.vue';
import InlineDiffView from './inline_diff_view.vue';
......@@ -55,6 +55,7 @@ export default {
'isParallelView',
'getCommentFormForDiffFile',
'diffLines',
'fileLineCodequality',
]),
...mapGetters(['getNoteableData', 'noteableType', 'getUserData']),
diffMode() {
......
......@@ -202,6 +202,9 @@ export default {
externalUrlLabel() {
return sprintf(__('View on %{url}'), { url: this.diffFile.formatted_external_url });
},
showCodequalityBadge() {
return this.codequalityDiff?.length > 0 && !this.glFeatures.codequalityMrDiffAnnotations;
},
},
methods: {
...mapActions('diffs', [
......@@ -334,7 +337,7 @@ export default {
/>
<code-quality-badge
v-if="codequalityDiff.length"
v-if="showCodequalityBadge"
:file-name="filePath"
:codequality-diff="codequalityDiff"
class="gl-mr-2"
......
......@@ -24,6 +24,8 @@ import * as utils from './diff_row_utils';
export default {
components: {
DiffGutterAvatars,
CodeQualityGutterIcon: () =>
import('ee_component/diffs/components/code_quality_gutter_icon.vue'),
},
directives: {
GlTooltip: GlTooltipDirective,
......@@ -89,6 +91,20 @@ export default {
if (!this.line.right) return {};
return this.fileLineCoverage(this.filePath, this.line.right.new_line);
},
showCodequalityLeft() {
return (
this.glFeatures.codequalityMrDiffAnnotations &&
this.inline &&
this.line.left?.codequality?.length > 0
);
},
showCodequalityRight() {
return (
this.glFeatures.codequalityMrDiffAnnotations &&
!this.inline &&
this.line.right?.codequality?.length > 0
);
},
classNameMapCellLeft() {
return utils.classNameMapCell({
line: this.line.left,
......@@ -269,6 +285,12 @@ export default {
:class="[...parallelViewLeftLineType, coverageStateLeft.class]"
class="diff-td line-coverage left-side"
></div>
<div class="diff-td line-codequality left-side" :class="[...parallelViewLeftLineType]">
<code-quality-gutter-icon
v-if="showCodequalityLeft"
:codequality="line.left.codequality"
/>
</div>
<div
:id="line.left.line_code"
:key="line.left.line_code"
......@@ -298,6 +320,11 @@ export default {
class="diff-td line-coverage left-side empty-cell"
:class="emptyCellLeftClassMap"
></div>
<div
v-if="inline"
class="diff-td line-codequality left-side empty-cell"
:class="emptyCellLeftClassMap"
></div>
<div
class="diff-td line_content with-coverage left-side empty-cell"
:class="[emptyCellLeftClassMap, { parallel: !inline }]"
......@@ -370,6 +397,15 @@ export default {
]"
class="diff-td line-coverage right-side"
></div>
<div
class="diff-td line-codequality right-side"
:class="[line.right.type, { hll: isHighlighted, hll: isCommented }]"
>
<code-quality-gutter-icon
v-if="showCodequalityRight"
:codequality="line.right.codequality"
/>
</div>
<div
:id="line.right.line_code"
:key="line.right.rich_text"
......@@ -405,6 +441,10 @@ export default {
class="diff-td line-coverage right-side empty-cell"
:class="emptyCellRightClassMap"
></div>
<div
class="diff-td line-codequality right-side empty-cell"
:class="emptyCellRightClassMap"
></div>
<div
class="diff-td line_content with-coverage right-side empty-cell"
:class="[emptyCellRightClassMap, { parallel: !inline }]"
......
......@@ -43,6 +43,7 @@ export default {
},
computed: {
...mapGetters('diffs', ['commitId']),
...mapState('diffs', ['codequalityDiff']),
...mapState({
selectedCommentPosition: ({ notes }) => notes.selectedCommentPosition,
selectedCommentPositionHover: ({ notes }) => notes.selectedCommentPositionHover,
......@@ -56,6 +57,9 @@ export default {
this.diffLines,
);
},
hasCodequalityChanges() {
return this.codequalityDiff?.files?.[this.diffFile.file_path]?.length > 0;
},
},
methods: {
...mapActions(['setSelectedCommentPosition']),
......@@ -98,7 +102,7 @@ export default {
<template>
<div
:class="[$options.userColorScheme, { inline }]"
:class="[$options.userColorScheme, { inline, 'with-codequality': hasCodequalityChanges }]"
:data-commit-id="commitId"
class="diff-grid diff-table code diff-wrap-lines js-syntax-highlight text-file"
>
......
......@@ -136,6 +136,11 @@ export const fileLineCoverage = (state) => (file, line) => {
return {};
};
// This function is overwritten for the inline codequality feature in EE
export const fileLineCodequality = () => () => {
return null;
};
/**
* Returns index of a currently selected diff in diffFiles
* @returns {number}
......
import * as actions from 'ee_else_ce/diffs/store/actions';
import * as getters from 'ee_else_ce/diffs/store/getters';
import createState from 'ee_else_ce/diffs/store/modules/diff_state';
import mutations from 'ee_else_ce/diffs/store/mutations';
import * as getters from '../getters';
export default () => ({
namespaced: true,
......
......@@ -598,7 +598,9 @@ table.code {
.diff-grid-left,
.diff-grid-right {
display: grid;
grid-template-columns: 50px 8px 1fr;
// Zero width column is a placeholder for the EE inline code quality diff
// see ee/.../diffs.scss for more details
grid-template-columns: 50px 8px 0 1fr;
}
.diff-grid-comments {
......@@ -628,7 +630,9 @@ table.code {
.diff-grid-left,
.diff-grid-right {
grid-template-columns: 50px 50px 8px 1fr;
// Zero width column is a placeholder for the EE inline code quality diff
// see ee/../diffs.scss for more details
grid-template-columns: 50px 50px 8px 0 1fr;
}
}
}
......
......@@ -131,6 +131,7 @@ $dark-il: #de935f;
.diff-td.diff-line-num.hll:not(.empty-cell),
.diff-td.line-coverage.hll:not(.empty-cell),
.diff-td.line-codequality.hll:not(.empty-cell),
.diff-td.line_content.hll:not(.empty-cell),
td.diff-line-num.hll:not(.empty-cell),
td.line-coverage.hll:not(.empty-cell),
......@@ -145,6 +146,7 @@ $dark-il: #de935f;
.diff-line-num.new,
.line-coverage.new,
.line-codequality.new,
.line_content.new {
@include diff-background($dark-new-bg, $dark-new-idiff, $dark-border);
......@@ -156,6 +158,7 @@ $dark-il: #de935f;
.diff-line-num.old,
.line-coverage.old,
.line-codequality.old,
.line_content.old {
@include diff-background($dark-old-bg, $dark-old-idiff, $dark-border);
......
......@@ -132,6 +132,7 @@ $monokai-gh: #75715e;
.diff-td.diff-line-num.hll:not(.empty-cell),
.diff-td.line-coverage.hll:not(.empty-cell),
.diff-td.line-codequality.hll:not(.empty-cell),
.diff-td.line_content.hll:not(.empty-cell),
td.diff-line-num.hll:not(.empty-cell),
td.line-coverage.hll:not(.empty-cell),
......@@ -146,6 +147,7 @@ $monokai-gh: #75715e;
.diff-line-num.new,
.line-coverage.new,
.line-codequality.new,
.line_content.new {
@include diff-background($monokai-new-bg, $monokai-new-idiff, $monokai-diff-border);
......@@ -157,6 +159,7 @@ $monokai-gh: #75715e;
.diff-line-num.old,
.line-coverage.old,
.line-codequality.old,
.line_content.old {
@include diff-background($monokai-old-bg, $monokai-old-idiff, $monokai-diff-border);
......
......@@ -56,7 +56,10 @@
.line-coverage {
@include line-coverage-border-color($green-500, $orange-500);
}
.line-coverage,
.line-codequality {
&.old,
&.new {
background-color: $white-normal;
......
......@@ -135,6 +135,7 @@ $solarized-dark-il: #2aa198;
.diff-td.diff-line-num.hll:not(.empty-cell),
.diff-td.line-coverage.hll:not(.empty-cell),
.diff-td.line-codequality.hll:not(.empty-cell),
.diff-td.line_content.hll:not(.empty-cell),
td.diff-line-num.hll:not(.empty-cell),
td.line-coverage.hll:not(.empty-cell),
......@@ -156,6 +157,7 @@ $solarized-dark-il: #2aa198;
.diff-line-num.new,
.line-coverage.new,
.line-codequality.new,
.line_content.new {
@include diff-background($solarized-dark-new-bg, $solarized-dark-new-idiff, $solarized-dark-border);
......@@ -167,6 +169,7 @@ $solarized-dark-il: #2aa198;
.diff-line-num.old,
.line-coverage.old,
.line-codequality.old,
.line_content.old {
@include diff-background($solarized-dark-old-bg, $solarized-dark-old-idiff, $solarized-dark-border);
......
......@@ -142,6 +142,7 @@ $solarized-light-il: #2aa198;
.diff-td.diff-line-num.hll:not(.empty-cell),
.diff-td.line-coverage.hll:not(.empty-cell),
.diff-td.line-codequality.hll:not(.empty-cell),
.diff-td.line_content.hll:not(.empty-cell),
td.diff-line-num.hll:not(.empty-cell),
td.line-coverage.hll:not(.empty-cell),
......@@ -156,6 +157,7 @@ $solarized-light-il: #2aa198;
.diff-line-num.new,
.line-coverage.new,
.line-codequality.new,
.line_content.new {
@include diff-background($solarized-light-new-bg,
$solarized-light-new-idiff, $solarized-light-border);
......@@ -175,6 +177,7 @@ $solarized-light-il: #2aa198;
.diff-line-num.old,
.line-coverage.old,
.line-codequality.old,
.line_content.old {
@include diff-background($solarized-light-old-bg, $solarized-light-old-idiff, $solarized-light-border);
......
......@@ -219,7 +219,10 @@ pre.code,
.line-coverage {
@include line-coverage-border-color($green-400, $red-400);
}
.line-coverage,
.line-codequality {
&.old {
background-color: $line-removed;
}
......
......@@ -43,6 +43,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
push_frontend_feature_flag(:usage_data_i_testing_summary_widget_total, @project, default_enabled: :yaml)
push_frontend_feature_flag(:improved_emoji_picker, project, default_enabled: :yaml)
push_frontend_feature_flag(:diffs_virtual_scrolling, project, default_enabled: :yaml)
push_frontend_feature_flag(:codequality_mr_diff_annotations, project, default_enabled: :yaml)
# Usage data feature flags
push_frontend_feature_flag(:users_expanding_widgets_usage_data, @project, default_enabled: :yaml)
......
---
name: codequality_mr_diff_annotations
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57926
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/330909
milestone: '14.0'
type: development
group: group::testing
default_enabled: false
<script>
import { GlTooltipDirective, GlIcon } from '@gitlab/ui';
import { SEVERITY_CLASSES, SEVERITY_ICONS } from '~/reports/codequality_report/constants';
export default {
components: {
GlIcon,
},
directives: {
GlTooltip: GlTooltipDirective,
},
props: {
codequality: {
type: Array,
required: false,
default: () => [],
},
},
computed: {
description() {
return this.codequality[0].description;
},
severity() {
return this.codequality[0].severity;
},
severityClass() {
return SEVERITY_CLASSES[this.severity] || SEVERITY_CLASSES.unknown;
},
severityIcon() {
return SEVERITY_ICONS[this.severity] || SEVERITY_ICONS.unknown;
},
},
};
</script>
<template>
<div v-gl-tooltip.hover :title="description">
<gl-icon :size="12" :name="severityIcon" :class="severityClass" />
</div>
</template>
import {
mapParallel as CEMapParallel,
mapInline as CEMapInline,
} from '~/diffs/components/diff_row_utils';
export const mapParallel = (content) => (line) => {
let { left, right } = line;
if (left) {
left = {
...left,
codequality: content.fileLineCodequality(content.diffFile.file_path, left.new_line),
};
}
if (right) {
right = {
...right,
codequality: content.fileLineCodequality(content.diffFile.file_path, right.new_line),
};
}
return {
...CEMapParallel(content)({
...line,
left,
right,
}),
};
};
export const mapInline = CEMapInline;
/* eslint-disable import/export */
export * from '~/diffs/store/getters';
// Returns the code quality degradations for a specific line of a given file
export const fileLineCodequality = (state) => (file, line) => {
const fileDiff = state.codequalityDiff.files?.[file] || [];
const lineDiff = fileDiff.filter((violation) => violation.line === line);
return lineDiff;
};
// Merge request diff grid layout modifications for inline code quality diff
// These grid templates build off of their equivalents in app/.../diffs.scss
// to expand the column only when the .with-codequality class is applied
.diff-grid.with-codequality {
.diff-grid-right {
grid-template-columns: 50px 8px 20px 1fr;
}
&.inline {
.diff-grid-left {
grid-template-columns: 50px 50px 8px 20px 1fr;
}
}
}
import { GlIcon } from '@gitlab/ui';
import { shallowMount } from '@vue/test-utils';
import Vue from 'vue';
import Vuex from 'vuex';
import CodeQualityGutterIcon from 'ee/diffs/components/code_quality_gutter_icon.vue';
import createDiffsStore from 'jest/diffs/create_diffs_store';
import { SEVERITY_CLASSES, SEVERITY_ICONS } from '~/reports/codequality_report/constants';
Vue.use(Vuex);
describe('EE CodeQualityGutterIcon', () => {
let store;
let wrapper;
let codequalityDiff;
const findIcon = () => wrapper.findComponent(GlIcon);
const createComponent = (props = {}, extendStore = () => {}) => {
store = createDiffsStore();
store.state.diffs.codequalityDiff = codequalityDiff;
extendStore(store);
wrapper = shallowMount(CodeQualityGutterIcon, {
propsData: { ...props },
store,
});
};
afterEach(() => {
wrapper.destroy();
});
it.each`
severity
${'info'}
${'minor'}
${'major'}
${'critical'}
${'blocker'}
${'unknown'}
`('shows icon for $severity degradation', ({ severity }) => {
createComponent({ codequality: [{ severity }] });
expect(findIcon().exists()).toBe(true);
expect(findIcon().attributes()).toEqual({
class: SEVERITY_CLASSES[severity],
name: SEVERITY_ICONS[severity],
size: '12',
});
});
});
......@@ -9,7 +9,7 @@ import createDiffsStore from '~/diffs/store/modules';
const getReadableFile = () => JSON.parse(JSON.stringify(diffFileMockDataReadable));
function createComponent({ withCodequality = true }) {
function createComponent({ withCodequality = true, provide = {} }) {
const file = getReadableFile();
const localVue = createLocalVue();
......@@ -48,6 +48,7 @@ function createComponent({ withCodequality = true }) {
isFirstFile: false,
isLastFile: false,
},
provide,
});
return {
......@@ -75,6 +76,19 @@ describe('EE DiffFile', () => {
});
});
describe('when the feature flag for the next iteration is enabled', () => {
beforeEach(() => {
({ wrapper } = createComponent({
withCodequality: true,
provide: { glFeatures: { codequalityMrDiffAnnotations: true } },
}));
});
it('does not show the code quality badge', () => {
expect(wrapper.find(CodeQualityBadge).exists()).toBe(false);
});
});
describe('when there is no diff data for the file', () => {
beforeEach(() => {
({ wrapper } = createComponent({ withCodequality: false }));
......
import { shallowMount } from '@vue/test-utils';
import Vue from 'vue';
import Vuex from 'vuex';
import CodeQualityGutterIcon from 'ee/diffs/components/code_quality_gutter_icon.vue';
import DiffRow from '~/diffs/components/diff_row.vue';
import diffsModule from '~/diffs/store/modules';
Vue.use(Vuex);
describe('EE DiffRow', () => {
let wrapper;
const findIcon = () => wrapper.findComponent(CodeQualityGutterIcon);
const defaultProps = {
fileHash: 'abc',
filePath: 'abc',
line: {},
index: 0,
};
const createComponent = ({ props, state, actions, isLoggedIn = true, provide }) => {
const diffs = diffsModule();
diffs.state = { ...diffs.state, ...state };
diffs.actions = { ...diffs.actions, ...actions };
const getters = { isLoggedIn: () => isLoggedIn };
const store = new Vuex.Store({
modules: { diffs },
getters,
});
wrapper = shallowMount(DiffRow, { propsData: { ...defaultProps, ...props }, store, provide });
};
afterEach(() => {
wrapper.destroy();
});
describe('with feature flag enabled', () => {
beforeEach(() => {
createComponent({
props: { line: { right: { codequality: [{ severity: 'critical' }] } } },
provide: { glFeatures: { codequalityMrDiffAnnotations: true } },
});
});
it('shows code quality gutter icon', () => {
expect(findIcon().exists()).toBe(true);
});
});
describe('without feature flag enabled', () => {
beforeEach(() => {
createComponent({
props: { line: { right: { codequality: [{ severity: 'critical' }] } } },
provide: { glFeatures: { codequalityMrDiffAnnotations: false } },
});
});
it('does not code quality gutter icon', () => {
expect(findIcon().exists()).toBe(false);
});
});
});
import { mount, createLocalVue } from '@vue/test-utils';
import Vuex from 'vuex';
import diffFileMockDataReadable from 'jest/diffs/mock_data/diff_file';
import DiffViewComponent from '~/diffs/components/diff_view.vue';
import createDiffsStore from '~/diffs/store/modules';
const getReadableFile = () => JSON.parse(JSON.stringify(diffFileMockDataReadable));
function createComponent({ withCodequality = true }) {
const diffFile = getReadableFile();
const localVue = createLocalVue();
localVue.use(Vuex);
const store = new Vuex.Store({
modules: {
diffs: createDiffsStore(),
},
});
store.state.diffs.diffFiles = [diffFile];
if (withCodequality) {
store.state.diffs.codequalityDiff = {
files: {
[diffFile.file_path]: [
{ line: 1, description: 'Unexpected alert.', severity: 'minor' },
{
line: 3,
description: 'Arrow function has too many statements (52). Maximum allowed is 30.',
severity: 'minor',
},
],
},
};
}
const wrapper = mount(DiffViewComponent, {
store,
localVue,
propsData: {
diffFile,
diffLines: [],
},
});
return {
localVue,
wrapper,
store,
};
}
describe('EE DiffView', () => {
let wrapper;
afterEach(() => {
wrapper.destroy();
});
describe('when there is diff data for the file', () => {
beforeEach(() => {
({ wrapper } = createComponent({ withCodequality: true }));
});
it('has the with-codequality class', () => {
expect(wrapper.classes('with-codequality')).toBe(true);
});
});
describe('when there is no diff data for the file', () => {
beforeEach(() => {
({ wrapper } = createComponent({ withCodequality: false }));
});
it('does not have the with-codequality class', () => {
expect(wrapper.classes('with-codequality')).toBe(false);
});
});
});
import * as getters from 'ee/diffs/store/getters';
import state from 'ee/diffs/store/modules/diff_state';
describe('EE Diffs Module Getters', () => {
let localState;
beforeEach(() => {
localState = state();
localState.codequalityDiff = {
files: {
'index.js': [
{
severity: 'minor',
description: 'Unexpected alert.',
line: 1,
},
{
severity: 'major',
description:
'Function `aVeryLongFunction` has 52 lines of code (exceeds 25 allowed). Consider refactoring.',
line: 3,
},
{
severity: 'minor',
description: 'Arrow function has too many statements (52). Maximum allowed is 30.',
line: 3,
},
],
},
};
});
describe('fileLineCodequality', () => {
it.each`
line | severity
${1} | ${'minor'}
${2} | ${'no'}
${3} | ${'major'}
${4} | ${'no'}
`('finds $severity degradation on line $line', ({ line, severity }) => {
if (severity === 'no') {
expect(getters.fileLineCodequality(localState)('index.js', line)).toEqual([]);
} else {
expect(getters.fileLineCodequality(localState)('index.js', line)[0]).toMatchObject({
line,
severity,
});
}
});
});
});
......@@ -73,6 +73,7 @@ describe('DiffContent', () => {
isParallelView: isParallelViewGetterMock,
getCommentFormForDiffFile: getCommentFormForDiffFileGetterMock,
diffLines: () => () => [...diffFileMockData.parallel_diff_lines],
fileLineCodequality: () => () => [],
},
actions: {
saveDiffDiscussion: saveDiffDiscussionMock,
......
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