Skip to content

G
gitlab-ce
Commit e64780db authored by Justin Boyson's avatar Justin Boyson Committed by Mark Florian
Browse files
Options

Refactor compare dropdowns 

We're splitting `compare_versions_dropdown` into 3 separate components
A "layout" component and two "higher order" compoments

This should greatly simplify the code, and testing.
parent 3824edb1
Hide whitespace changes
Inline Side-by-side
Showing with 334 additions and 246 deletions
app/assets/javascripts/diffs/components/app.vue
View file @ e64780db
......@@ -112,7 +112,6 @@ export default {
mergeRequestDiffs: state => state.diffs.mergeRequestDiffs,
mergeRequestDiff: state => state.diffs.mergeRequestDiff,
commit: state => state.diffs.commit,
targetBranchName: state => state.diffs.targetBranchName,
renderOverflowWarning: state => state.diffs.renderOverflowWarning,
numTotalFiles: state => state.diffs.realSize,
numVisibleFiles: state => state.diffs.size,
......@@ -123,19 +122,9 @@ export default {
...mapState('diffs', ['showTreeList', 'isLoading', 'startVersion']),
...mapGetters('diffs', ['isParallelView', 'currentDiffIndex']),
...mapGetters(['isNotesFetched', 'getNoteableData']),
targetBranch() {
return {
branchName: this.targetBranchName,
versionIndex: -1,
path: '',
};
},
canCurrentUserFork() {
return this.currentUser.can_fork === true && this.currentUser.can_create_merge_request;
},
showCompareVersions() {
return this.mergeRequestDiffs && this.mergeRequestDiff;
},
renderDiffFiles() {
return (
this.diffFiles.length > 0 ||
......@@ -369,8 +358,6 @@ export default {
<div v-else id="diffs" :class="{ active: shouldShow }" class="diffs tab-pane">
<compare-versions
:merge-request-diffs="mergeRequestDiffs"
:merge-request-diff="mergeRequestDiff"
:target-branch="targetBranch"
:is-limited-container="isLimitedContainer"
:diff-files-length="diffFilesLength"
/>
......
app/assets/javascripts/diffs/components/compare_versions_dropdown.vue → app/assets/javascripts/diffs/components/compare_dropdown_layout.vue
View file @ e64780db
<script>
import Icon from '~/vue_shared/components/icon.vue';
import { n__, __, sprintf } from '~/locale';
import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
import TimeAgo from '~/vue_shared/components/time_ago_tooltip.vue';
export default {
......@@ -10,98 +8,14 @@ export default {
TimeAgo,
},
props: {
otherVersions: {
versions: {
type: Array,
required: false,
default: () => [],
},
mergeRequestVersion: {
type: Object,
required: false,
default: null,
},
startVersion: {
type: Object,
required: false,
default: null,
},
targetBranch: {
type: Object,
required: false,
default: null,
},
showCommitCount: {
type: Boolean,
required: false,
default: false,
},
baseVersionPath: {
type: String,
required: false,
default: null,
required: true,
},
},
computed: {
targetVersions() {
if (this.mergeRequestVersion) {
return this.otherVersions;
}
return [...this.otherVersions, this.targetBranch];
},
selectedVersionName() {
const selectedVersion = this.startVersion || this.targetBranch || this.mergeRequestVersion;
return this.versionName(selectedVersion);
},
},
methods: {
commitsText(version) {
return n__(`%d commit,`, `%d commits,`, version.commits_count);
},
href(version) {
if (this.isBase(version)) {
return this.baseVersionPath;
}
if (this.showCommitCount) {
return version.version_path;
}
return version.compare_path;
},
versionName(version) {
if (this.isLatest(version)) {
return __('latest version');
}
if (this.targetBranch && (this.isBase(version) || !version)) {
return this.targetBranch.branchName;
}
return sprintf(__(`version %{versionIndex}`), { versionIndex: version.version_index });
},
isActive(version) {
if (!version) {
return false;
}
if (this.targetBranch) {
return (
(this.isBase(version) && !this.startVersion) ||
(this.startVersion && this.startVersion.version_index === version.version_index)
);
}
return version.version_index === this.mergeRequestVersion.version_index;
},
isBase(version) {
if (!version || !this.targetBranch) {
return false;
}
return version.versionIndex === -1;
},
isHead() {
return parseBoolean(getParameterByName('diff_head'));
},
isLatest(version) {
return (
this.mergeRequestVersion && version.version_index === this.targetVersions[0].version_index
);
return this.versions.find(x => x.selected)?.versionName || '';
},
},
};
......@@ -120,13 +34,15 @@ export default {
<div class="dropdown-menu dropdown-select dropdown-menu-selectable">
<div class="dropdown-content">
<ul>
<li v-for="version in targetVersions" :key="version.id">
<a :class="{ 'is-active': isActive(version) }" :href="href(version)">
<li v-for="version in versions" :key="version.id">
<a :class="{ 'is-active': version.selected }" :href="version.href">
<div>
<strong>
{{ versionName(version) }}
<template v-if="isHead()">{{ s__('DiffsCompareBaseBranch|(HEAD)') }}</template>
<template v-else-if="isBase(version)">{{
{{ version.versionName }}
<template v-if="version.isHead">{{
s__('DiffsCompareBaseBranch|(HEAD)')
}}</template>
<template v-else-if="version.isBase">{{
s__('DiffsCompareBaseBranch|(base)')
}}</template>
</strong>
......@@ -136,8 +52,8 @@ export default {
</div>
<div>
<small>
<template v-if="showCommitCount">
{{ commitsText(version) }}
<template v-if="version.commitsText">
{{ version.commitsText }}
</template>
<time-ago
v-if="version.created_at"
......
app/assets/javascripts/diffs/components/compare_versions.vue
View file @ e64780db
......@@ -4,14 +4,14 @@ import { GlTooltipDirective, GlLink, GlDeprecatedButton, GlSprintf } from '@gitl
import { __ } from '~/locale';
import { polyfillSticky } from '~/lib/utils/sticky';
import Icon from '~/vue_shared/components/icon.vue';
import CompareVersionsDropdown from './compare_versions_dropdown.vue';
import CompareDropdownLayout from './compare_dropdown_layout.vue';
import SettingsDropdown from './settings_dropdown.vue';
import DiffStats from './diff_stats.vue';
import { CENTERED_LIMITED_CONTAINER_CLASSES } from '../constants';
export default {
components: {
CompareVersionsDropdown,
CompareDropdownLayout,
Icon,
GlLink,
GlDeprecatedButton,
......@@ -27,16 +27,6 @@ export default {
type: Array,
required: true,
},
mergeRequestDiff: {
type: Object,
required: false,
default: () => ({}),
},
targetBranch: {
type: Object,
required: false,
default: null,
},
isLimitedContainer: {
type: Boolean,
required: false,
......@@ -48,7 +38,11 @@ export default {
},
},
computed: {
...mapGetters('diffs', ['hasCollapsedFile']),
...mapGetters('diffs', [
'hasCollapsedFile',
'diffCompareDropdownTargetVersions',
'diffCompareDropdownSourceVersions',
]),
...mapState('diffs', [
'commit',
'showTreeList',
......@@ -57,18 +51,12 @@ export default {
'addedLines',
'removedLines',
]),
comparableDiffs() {
return this.mergeRequestDiffs.slice(1);
},
showDropdowns() {
return !this.commit && this.mergeRequestDiffs.length;
},
toggleFileBrowserTitle() {
return this.showTreeList ? __('Hide file browser') : __('Show file browser');
},
baseVersionPath() {
return this.mergeRequestDiff.base_version_path;
},
},
created() {
this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES;
......@@ -113,19 +101,14 @@ export default {
:message="s__('MergeRequest|Compare %{source} and %{target}')"
>
<template #source>
<compare-versions-dropdown
:other-versions="mergeRequestDiffs"
:merge-request-version="mergeRequestDiff"
:show-commit-count="true"
<compare-dropdown-layout
:versions="diffCompareDropdownSourceVersions"
class="mr-version-dropdown"
/>
</template>
<template #target>
<compare-versions-dropdown
:other-versions="comparableDiffs"
:base-version-path="baseVersionPath"
:start-version="startVersion"
:target-branch="targetBranch"
<compare-dropdown-layout
:versions="diffCompareDropdownTargetVersions"
class="mr-version-compare-dropdown"
/>
</template>
......
app/assets/javascripts/diffs/constants.js
View file @ e64780db
......@@ -58,3 +58,5 @@ export const START_RENDERING_INDEX = 200;
export const INLINE_DIFF_LINES_KEY = 'highlighted_diff_lines';
export const PARALLEL_DIFF_LINES_KEY = 'parallel_diff_lines';
export const DIFFS_PER_PAGE = 20;
export const DIFF_COMPARE_BASE_VERSION_INDEX = -1;
app/assets/javascripts/diffs/store/getters.js
View file @ e64780db
import { __, n__ } from '~/locale';
import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '../constants';
export * from './getters_versions_dropdowns';
export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW_TYPE;
export const isInlineView = state => state.diffViewType === INLINE_DIFF_VIEW_TYPE;
......
app/assets/javascripts/diffs/store/getters_versions_dropdowns.js 0 → 100644
View file @ e64780db
import { __, n__, sprintf } from '~/locale';
import { DIFF_COMPARE_BASE_VERSION_INDEX } from '../constants';
export const selectedTargetIndex = state =>
state.startVersion?.version_index || DIFF_COMPARE_BASE_VERSION_INDEX;
export const selectedSourceIndex = state => state.mergeRequestDiff.version_index;
export const diffCompareDropdownTargetVersions = (state, getters) => {
// startVersion only exists if the user has selected a version other
// than "base" so if startVersion is null then base must be selected
const baseVersion = {
versionName: state.targetBranchName,
version_index: DIFF_COMPARE_BASE_VERSION_INDEX,
href: state.mergeRequestDiff.base_version_path,
isBase: true,
selected: !state.startVersion,
};
// Appended properties here are to make the compare_dropdown_layout easier to reason about
const formatVersion = v => {
return {
href: v.compare_path,
versionName: sprintf(__(`version %{versionIndex}`), { versionIndex: v.version_index }),
selected: v.version_index === getters.selectedTargetIndex,
...v,
};
};
return [...state.mergeRequestDiffs.slice(1).map(formatVersion), baseVersion];
};
export const diffCompareDropdownSourceVersions = (state, getters) => {
// Appended properties here are to make the compare_dropdown_layout easier to reason about
return state.mergeRequestDiffs.map((v, i) => ({
...v,
href: v.version_path,
commitsText: n__(`%d commit,`, `%d commits,`, v.commits_count),
versionName:
i === 0
? __('latest version')
: sprintf(__(`version %{versionIndex}`), { versionIndex: v.version_index }),
selected: v.version_index === getters.selectedSourceIndex,
}));
};
app/assets/javascripts/diffs/store/modules/diff_state.js
View file @ e64780db
......@@ -15,7 +15,7 @@ export default () => ({
endpoint: '',
basePath: '',
commit: null,
startVersion: null,
startVersion: null, // Null unless a target diff is selected for comparison that is not the "base" diff
diffFiles: [],
coverageFiles: {},
mergeRequestDiffs: [],
......
spec/frontend/diffs/components/app_spec.js
View file @ e64780db
......@@ -613,13 +613,7 @@ describe('diffs/components/app', () => {
expect(wrapper.contains(CompareVersions)).toBe(true);
expect(wrapper.find(CompareVersions).props()).toEqual(
expect.objectContaining({
targetBranch: {
branchName: 'target-branch',
versionIndex: -1,
path: '',
},
mergeRequestDiffs: diffsMockData,
mergeRequestDiff,
}),
);
});
......
spec/frontend/diffs/components/compare_dropdown_layout_spec.js 0 → 100644
View file @ e64780db
import { shallowMount } from '@vue/test-utils';
import { trimText } from 'helpers/text_helper';
import TimeAgo from '~/vue_shared/components/time_ago_tooltip.vue';
import CompareDropdownLayout from '~/diffs/components/compare_dropdown_layout.vue';
const TEST_COMMIT_TEXT = '1 commit';
const TEST_CREATED_AT = '2018-10-23T11:49:16.611Z';
describe('CompareDropdownLayout', () => {
let wrapper;
const createVersion = ({ id, isHead, isBase, selected, commitsText, createdAt }) => ({
id,
href: `version/${id}`,
versionName: `version ${id}`,
isHead,
isBase,
short_commit_sha: `abcdef${id}`,
commitsText,
created_at: createdAt,
selected,
});
const createComponent = (propsData = {}) => {
wrapper = shallowMount(CompareDropdownLayout, {
propsData: {
...propsData,
},
});
};
const findListItems = () => wrapper.findAll('li');
const findListItemsData = () =>
findListItems().wrappers.map(listItem => ({
href: listItem.find('a').attributes('href'),
text: trimText(listItem.text()),
createdAt: listItem.findAll(TimeAgo).wrappers[0]?.props('time'),
isActive: listItem.find('a.is-active').exists(),
}));
afterEach(() => {
wrapper.destroy();
wrapper = null;
});
describe('with versions', () => {
beforeEach(() => {
const versions = [
createVersion({
id: 1,
isHead: false,
isBase: true,
selected: true,
commitsText: TEST_COMMIT_TEXT,
createdAt: TEST_CREATED_AT,
}),
createVersion({ id: 2, isHead: true, isBase: false, selected: false }),
createVersion({ id: 3, isHead: false, isBase: false, selected: false }),
];
createComponent({ versions });
});
it('renders the selected version name', () => {
expect(wrapper.text()).toContain('version 1');
});
it('renders versions in order', () => {
expect(findListItemsData()).toEqual([
{
href: 'version/1',
text: 'version 1 (base) abcdef1 1 commit',
createdAt: TEST_CREATED_AT,
isActive: true,
},
{
href: 'version/2',
text: 'version 2 (HEAD) abcdef2',
createdAt: undefined,
isActive: false,
},
{
href: 'version/3',
text: 'version 3 abcdef3',
createdAt: undefined,
isActive: false,
},
]);
});
});
});
spec/frontend/diffs/components/compare_versions_dropdown_spec.js deleted 100644 → 0
View file @ 3824edb1
import { shallowMount, createLocalVue } from '@vue/test-utils';
import CompareVersionsDropdown from '~/diffs/components/compare_versions_dropdown.vue';
import diffsMockData from '../mock_data/merge_request_diffs';
import TimeAgo from '~/vue_shared/components/time_ago_tooltip.vue';
import { TEST_HOST } from 'helpers/test_constants';
const localVue = createLocalVue();
const targetBranch = { branchName: 'tmp-wine-dev', versionIndex: -1 };
const startVersion = { version_index: 4 };
const mergeRequestVersion = {
version_path: '123',
};
const baseVersionPath = '/gnuwget/wget2/-/merge_requests/6/diffs?diff_id=37';
describe('CompareVersionsDropdown', () => {
let wrapper;
const findSelectedVersion = () => wrapper.find('.dropdown-menu-toggle');
const findVersionsListElements = () => wrapper.findAll('li');
const findLinkElement = index =>
findVersionsListElements()
.at(index)
.find('a');
const findLastLink = () => findLinkElement(findVersionsListElements().length - 1);
const createComponent = (props = {}) => {
wrapper = shallowMount(localVue.extend(CompareVersionsDropdown), {
localVue,
propsData: { ...props },
});
};
afterEach(() => {
wrapper.destroy();
});
describe('selected version name', () => {
it('shows latest version when latest is selected', () => {
createComponent({
mergeRequestVersion,
startVersion,
otherVersions: diffsMockData,
});
expect(findSelectedVersion().text()).toBe('latest version');
});
it('shows target branch name for base branch', () => {
createComponent({
targetBranch,
});
expect(findSelectedVersion().text()).toBe('tmp-wine-dev');
});
it('shows correct version for non-base and non-latest branches', () => {
createComponent({
startVersion,
targetBranch,
});
expect(findSelectedVersion().text()).toBe(`version ${startVersion.version_index}`);
});
});
describe('target versions list', () => {
it('should have the same length as otherVersions if merge request version is present', () => {
createComponent({
mergeRequestVersion,
otherVersions: diffsMockData,
});
expect(findVersionsListElements().length).toEqual(diffsMockData.length);
});
it('should have an otherVersions length plus 1 if no merge request version is present', () => {
createComponent({
targetBranch,
otherVersions: diffsMockData,
});
expect(findVersionsListElements().length).toEqual(diffsMockData.length + 1);
});
it('should have base branch link as active on base branch', () => {
createComponent({
targetBranch,
otherVersions: diffsMockData,
});
expect(findLastLink().classes()).toContain('is-active');
});
it('should have correct branch link as active if start version present', () => {
createComponent({
targetBranch,
startVersion,
otherVersions: diffsMockData,
});
expect(findLinkElement(0).classes()).toContain('is-active');
});
it('should render a correct base version link', () => {
createComponent({
baseVersionPath,
otherVersions: diffsMockData.slice(1),
targetBranch,
});
expect(findLastLink().attributes('href')).toEqual(baseVersionPath);
expect(findLastLink().text()).toContain('(base)');
expect(findLastLink().text()).not.toContain('(HEAD)');
});
it('should render a correct head version link', () => {
Object.defineProperty(window, 'location', {
writable: true,
value: { href: `${TEST_HOST}?diff_head=true` },
});
createComponent({
baseVersionPath,
otherVersions: diffsMockData.slice(1),
targetBranch,
});
expect(findLastLink().attributes('href')).toEqual(baseVersionPath);
expect(findLastLink().text()).not.toContain('(base)');
expect(findLastLink().text()).toContain('(HEAD)');
});
it('should not render commits count if no showCommitsCount is passed', () => {
createComponent({
otherVersions: diffsMockData,
targetBranch,
});
const commitsCount = diffsMockData[0].commits_count;
expect(findLinkElement(0).text()).not.toContain(`${commitsCount} commit`);
});
it('should render correct commits count if showCommitsCount is passed', () => {
createComponent({
otherVersions: diffsMockData,
targetBranch,
showCommitCount: true,
});
const commitsCount = diffsMockData[0].commits_count;
expect(findLinkElement(0).text()).toContain(`${commitsCount} commit`);
});
it('should render correct commit sha', () => {
createComponent({
otherVersions: diffsMockData,
targetBranch,
});
const commitShaElement = findLinkElement(0).find('.commit-sha');
expect(commitShaElement.text()).toBe(diffsMockData[0].short_commit_sha);
});
it('should render correct time-ago ', () => {
createComponent({
otherVersions: diffsMockData,
targetBranch,
});
const timeAgoElement = findLinkElement(0).find(TimeAgo);
expect(timeAgoElement.exists()).toBe(true);
expect(timeAgoElement.props('time')).toBe(diffsMockData[0].created_at);
});
});
});
spec/frontend/diffs/components/compare_versions_spec.js
View file @ e64780db
......@@ -12,23 +12,25 @@ localVue.use(Vuex);
describe('CompareVersions', () => {
let wrapper;
const targetBranch = { branchName: 'tmp-wine-dev', versionIndex: -1 };
const targetBranchName = 'tmp-wine-dev';
const createWrapper = props => {
const store = createStore();
const mergeRequestDiff = diffsMockData[0];
store.state.diffs.addedLines = 10;
store.state.diffs.removedLines = 20;
store.state.diffs.diffFiles.push('test');
store.state.diffs.targetBranchName = targetBranchName;
store.state.diffs.mergeRequestDiff = mergeRequestDiff;
store.state.diffs.mergeRequestDiffs = diffsMockData;
wrapper = mount(CompareVersionsComponent, {
localVue,
store,
propsData: {
mergeRequestDiffs: diffsMockData,
mergeRequestDiff: diffsMockData[0],
diffFilesLength: 0,
targetBranch,
...props,
},
});
......@@ -59,7 +61,7 @@ describe('CompareVersions', () => {
expect(sourceDropdown.exists()).toBe(true);
expect(targetDropdown.exists()).toBe(true);
expect(sourceDropdown.find('a span').html()).toContain('latest version');
expect(targetDropdown.find('a span').html()).toContain(targetBranch.branchName);
expect(targetDropdown.find('a span').html()).toContain(targetBranchName);
});
it('should not render comparison dropdowns if no mergeRequestDiffs are specified', () => {
......@@ -119,21 +121,6 @@ describe('CompareVersions', () => {
});
});
describe('comparableDiffs', () => {
it('should not contain the first item in the mergeRequestDiffs property', () => {
const { comparableDiffs } = wrapper.vm;
const comparableDiffsMock = diffsMockData.slice(1);
expect(comparableDiffs).toEqual(comparableDiffsMock);
});
});
describe('baseVersionPath', () => {
it('should be set correctly from mergeRequestDiff', () => {
expect(wrapper.vm.baseVersionPath).toEqual(wrapper.vm.mergeRequestDiff.base_version_path);
});
});
describe('commit', () => {
beforeEach(done => {
wrapper.vm.$store.state.diffs.commit = getDiffWithCommit().commit;
......
spec/frontend/diffs/store/getters_versions_dropdowns_spec.js 0 → 100644
View file @ e64780db
import * as getters from '~/diffs/store/getters';
import state from '~/diffs/store/modules/diff_state';
import { DIFF_COMPARE_BASE_VERSION_INDEX } from '~/diffs/constants';
import diffsMockData from '../mock_data/merge_request_diffs';
describe('Compare diff version dropdowns', () => {
let localState;
beforeEach(() => {
localState = state();
localState.mergeRequestDiff = {
base_version_path: 'basePath',
head_version_path: 'headPath',
version_index: 1,
};
localState.targetBranchName = 'baseVersion';
localState.mergeRequestDiffs = diffsMockData;
});
describe('selectedTargetIndex', () => {
it('without startVersion', () => {
expect(getters.selectedTargetIndex(localState)).toEqual(DIFF_COMPARE_BASE_VERSION_INDEX);
});
it('with startVersion', () => {
const startVersion = { version_index: 1 };
localState.startVersion = startVersion;
expect(getters.selectedTargetIndex(localState)).toEqual(startVersion.version_index);
});
});
it('selectedSourceIndex', () => {
expect(getters.selectedSourceIndex(localState)).toEqual(
localState.mergeRequestDiff.version_index,
);
});
describe('diffCompareDropdownTargetVersions', () => {
// diffCompareDropdownTargetVersions slices the array at the first position
// and appends a "base" version which is why we use diffsMockData[1] below
// This is to display "base" at the end of the target dropdown
const expectedFirstVersion = {
...diffsMockData[1],
href: expect.any(String),
versionName: expect.any(String),
};
const expectedBaseVersion = {
versionName: 'baseVersion',
version_index: DIFF_COMPARE_BASE_VERSION_INDEX,
href: 'basePath',
isBase: true,
};
it('base version selected', () => {
expectedFirstVersion.selected = false;
expectedBaseVersion.selected = true;
const targetVersions = getters.diffCompareDropdownTargetVersions(localState, {
selectedTargetIndex: DIFF_COMPARE_BASE_VERSION_INDEX,
});
const lastVersion = targetVersions[targetVersions.length - 1];
expect(targetVersions[0]).toEqual(expectedFirstVersion);
expect(lastVersion).toEqual(expectedBaseVersion);
});
it('first version selected', () => {
expectedFirstVersion.selected = true;
expectedBaseVersion.selected = false;
localState.startVersion = expectedFirstVersion;
const targetVersions = getters.diffCompareDropdownTargetVersions(localState, {
selectedTargetIndex: expectedFirstVersion.version_index,
});
const lastVersion = targetVersions[targetVersions.length - 1];
expect(targetVersions[0]).toEqual(expectedFirstVersion);
expect(lastVersion).toEqual(expectedBaseVersion);
});
});
it('diffCompareDropdownSourceVersions', () => {
const firstDiff = localState.mergeRequestDiffs[0];
const expectedShape = {
...firstDiff,
href: firstDiff.version_path,
commitsText: `${firstDiff.commits_count} commits,`,
versionName: 'latest version',
selected: true,
};
const sourceVersions = getters.diffCompareDropdownSourceVersions(localState, {
selectedSourceIndex: expectedShape.version_index,
});
expect(sourceVersions[0]).toEqual(expectedShape);
expect(sourceVersions[1].selected).toBe(false);
});
});
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7