Commit b8166a20 authored by Thomas Randolph's avatar Thomas Randolph Committed by Phil Hughes

Fix expansion cell previousLine relying on invalid data

This component (and indeed all of the diffs app)
used to rely on `highlighted_diff_lines` whether you
were looking at the inline view or not.

When the split diffs was being written, most places
that did this were caught. This is one place that slipped
through the cracks since there were no tests checking
the behavior.

The update adds a generic "getPreviousLine" that uses
the diff view type to switch, and adds a bunch of tests
that will fail if the code is checking the wrong line type.
parent 891f27a5
......@@ -3,7 +3,7 @@ import { mapState, mapActions } from 'vuex';
import createFlash from '~/flash';
import { s__ } from '~/locale';
import Icon from '~/vue_shared/components/icon.vue';
import { UNFOLD_COUNT } from '../constants';
import { UNFOLD_COUNT, INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE } from '../constants';
import * as utils from '../store/utils';
import tooltip from '../../vue_shared/directives/tooltip';
......@@ -11,6 +11,16 @@ const EXPAND_ALL = 0;
const EXPAND_UP = 1;
const EXPAND_DOWN = 2;
const lineNumberByViewType = (viewType, diffLine) => {
const numberGetters = {
[INLINE_DIFF_VIEW_TYPE]: line => line?.new_line,
[PARALLEL_DIFF_VIEW_TYPE]: line => (line?.right || line?.left)?.new_line,
};
const numberGetter = numberGetters[viewType];
return numberGetter && numberGetter(diffLine);
};
export default {
directives: {
tooltip,
......@@ -67,12 +77,16 @@ export default {
...mapActions('diffs', ['loadMoreLines']),
getPrevLineNumber(oldLineNumber, newLineNumber) {
const diffFile = utils.findDiffFile(this.diffFiles, this.fileHash);
const indexForInline = utils.findIndexInInlineLines(diffFile.highlighted_diff_lines, {
const lines = {
[INLINE_DIFF_VIEW_TYPE]: diffFile.highlighted_diff_lines,
[PARALLEL_DIFF_VIEW_TYPE]: diffFile.parallel_diff_lines,
};
const index = utils.getPreviousLineIndex(this.diffViewType, diffFile, {
oldLineNumber,
newLineNumber,
});
const prevLine = diffFile.highlighted_diff_lines[indexForInline - 2];
return (prevLine && prevLine.new_line) || 0;
return lineNumberByViewType(this.diffViewType, lines[this.diffViewType][index - 2]) || 0;
},
callLoadMoreLines(
endpoint,
......@@ -114,7 +128,7 @@ export default {
this.handleExpandAllLines(expandOptions);
}
},
handleExpandUpLines(expandOptions = EXPAND_ALL) {
handleExpandUpLines(expandOptions) {
const { endpoint, fileHash, view, oldLineNumber, newLineNumber, offset } = expandOptions;
const bottom = this.isBottom;
......
......@@ -140,6 +140,7 @@ export default {
addContextLines({
inlineLines: diffFile.highlighted_diff_lines,
parallelLines: diffFile.parallel_diff_lines,
diffViewType: state.diffViewType,
contextLines: lines,
bottom,
lineNumbers,
......
......@@ -13,6 +13,8 @@ import {
LINES_TO_BE_RENDERED_DIRECTLY,
MAX_LINES_TO_BE_RENDERED,
TREE_TYPE,
INLINE_DIFF_VIEW_TYPE,
PARALLEL_DIFF_VIEW_TYPE,
} from '../constants';
export function findDiffFile(files, match, matchKey = 'file_hash') {
......@@ -93,8 +95,7 @@ export function getNoteFormData(params) {
export const findIndexInInlineLines = (lines, lineNumbers) => {
const { oldLineNumber, newLineNumber } = lineNumbers;
return _.findIndex(
lines,
return lines.findIndex(
line => line.old_line === oldLineNumber && line.new_line === newLineNumber,
);
};
......@@ -102,8 +103,7 @@ export const findIndexInInlineLines = (lines, lineNumbers) => {
export const findIndexInParallelLines = (lines, lineNumbers) => {
const { oldLineNumber, newLineNumber } = lineNumbers;
return _.findIndex(
lines,
return lines.findIndex(
line =>
line.left &&
line.right &&
......@@ -112,13 +112,32 @@ export const findIndexInParallelLines = (lines, lineNumbers) => {
);
};
const indexGettersByViewType = {
[INLINE_DIFF_VIEW_TYPE]: findIndexInInlineLines,
[PARALLEL_DIFF_VIEW_TYPE]: findIndexInParallelLines,
};
export const getPreviousLineIndex = (diffViewType, file, lineNumbers) => {
const findIndex = indexGettersByViewType[diffViewType];
const lines = {
[INLINE_DIFF_VIEW_TYPE]: file.highlighted_diff_lines,
[PARALLEL_DIFF_VIEW_TYPE]: file.parallel_diff_lines,
};
return findIndex && findIndex(lines[diffViewType], lineNumbers);
};
export function removeMatchLine(diffFile, lineNumbers, bottom) {
const indexForInline = findIndexInInlineLines(diffFile.highlighted_diff_lines, lineNumbers);
const indexForParallel = findIndexInParallelLines(diffFile.parallel_diff_lines, lineNumbers);
const factor = bottom ? 1 : -1;
diffFile.highlighted_diff_lines.splice(indexForInline + factor, 1);
diffFile.parallel_diff_lines.splice(indexForParallel + factor, 1);
if (indexForInline > -1) {
diffFile.highlighted_diff_lines.splice(indexForInline + factor, 1);
}
if (indexForParallel > -1) {
diffFile.parallel_diff_lines.splice(indexForParallel + factor, 1);
}
}
export function addLineReferences(lines, lineNumbers, bottom, isExpandDown, nextLineNumbers) {
......@@ -160,8 +179,8 @@ export function addLineReferences(lines, lineNumbers, bottom, isExpandDown, next
return linesWithNumbers;
}
export function addContextLines(options) {
const { inlineLines, parallelLines, contextLines, lineNumbers, isExpandDown } = options;
function addParallelContextLines(options) {
const { parallelLines, contextLines, lineNumbers, isExpandDown } = options;
const normalizedParallelLines = contextLines.map(line => ({
left: line,
right: line,
......@@ -170,17 +189,40 @@ export function addContextLines(options) {
const factor = isExpandDown ? 1 : 0;
if (!isExpandDown && options.bottom) {
inlineLines.push(...contextLines);
parallelLines.push(...normalizedParallelLines);
} else {
const inlineIndex = findIndexInInlineLines(inlineLines, lineNumbers);
const parallelIndex = findIndexInParallelLines(parallelLines, lineNumbers);
inlineLines.splice(inlineIndex + factor, 0, ...contextLines);
parallelLines.splice(parallelIndex + factor, 0, ...normalizedParallelLines);
}
}
function addInlineContextLines(options) {
const { inlineLines, contextLines, lineNumbers, isExpandDown } = options;
const factor = isExpandDown ? 1 : 0;
if (!isExpandDown && options.bottom) {
inlineLines.push(...contextLines);
} else {
const inlineIndex = findIndexInInlineLines(inlineLines, lineNumbers);
inlineLines.splice(inlineIndex + factor, 0, ...contextLines);
}
}
export function addContextLines(options) {
const { diffViewType } = options;
const contextLineHandlers = {
[INLINE_DIFF_VIEW_TYPE]: addInlineContextLines,
[PARALLEL_DIFF_VIEW_TYPE]: addParallelContextLines,
};
const contextLineHandler = contextLineHandlers[diffViewType];
if (contextLineHandler) {
contextLineHandler(options);
}
}
/**
* Trims the first char of the `richText` property when it's either a space or a diff symbol.
* @param {Object} line
......
......@@ -7,10 +7,11 @@ describe 'User views diffs', :js do
create(:merge_request_with_diffs, source_project: project, target_project: project, source_branch: 'merge-test')
end
let(:project) { create(:project, :public, :repository) }
let(:view) { 'inline' }
before do
stub_feature_flags(diffs_batch_load: false)
visit(diffs_project_merge_request_path(project, merge_request))
visit(diffs_project_merge_request_path(project, merge_request, view: view))
wait_for_requests
......@@ -20,12 +21,20 @@ describe 'User views diffs', :js do
shared_examples 'unfold diffs' do
it 'unfolds diffs upwards' do
first('.js-unfold').click
expect(find('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"] .text-file')).to have_content('.bundle')
page.within('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"]') do
expect(find('.text-file')).to have_content('.bundle')
expect(page).to have_selector('.new_line [data-linenumber="1"]', count: 1)
end
end
it 'unfolds diffs to the start' do
first('.js-unfold-all').click
expect(find('.file-holder[id="a5cc2925ca8258af241be7e5b0381edf30266302"] .text-file')).to have_content('.rbc')
it 'unfolds diffs in the middle' do
page.within('.file-holder[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd"]') do
all('.js-unfold-all')[1].click
expect(page).to have_selector('.new_line [data-linenumber="24"]', count: 1)
expect(page).not_to have_selector('.new_line [data-linenumber="1"]')
end
end
it 'unfolds diffs downwards' do
......@@ -66,13 +75,7 @@ describe 'User views diffs', :js do
end
context 'when in the side-by-side view' do
before do
find('.js-show-diff-settings').click
click_button 'Side-by-side'
wait_for_requests
end
let(:view) { 'parallel' }
it 'shows diffs in parallel' do
expect(page).to have_css('.parallel')
......
import Vue from 'vue';
import { cloneDeep } from 'lodash';
import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
import { createStore } from '~/mr_notes/stores';
import DiffExpansionCell from '~/diffs/components/diff_expansion_cell.vue';
import { getPreviousLineIndex } from '~/diffs/store/utils';
import { INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE } from '~/diffs/constants';
import diffFileMockData from '../mock_data/diff_file';
const EXPAND_UP_CLASS = '.js-unfold';
const EXPAND_DOWN_CLASS = '.js-unfold-down';
const EXPAND_ALL_CLASS = '.js-unfold-all';
const LINE_TO_USE = 5;
const lineSources = {
[INLINE_DIFF_VIEW_TYPE]: 'highlighted_diff_lines',
[PARALLEL_DIFF_VIEW_TYPE]: 'parallel_diff_lines',
};
const lineHandlers = {
[INLINE_DIFF_VIEW_TYPE]: line => line,
[PARALLEL_DIFF_VIEW_TYPE]: line => line.right || line.left,
};
function makeLoadMoreLinesPayload({
sinceLine,
toLine,
oldLineNumber,
diffViewType,
fileHash,
nextLineNumbers = {},
unfold = false,
bottom = false,
isExpandDown = false,
}) {
return {
endpoint: 'contextLinesPath',
params: {
since: sinceLine,
to: toLine,
offset: toLine + 1 - oldLineNumber,
view: diffViewType,
unfold,
bottom,
},
lineNumbers: {
oldLineNumber,
newLineNumber: toLine + 1,
},
nextLineNumbers,
fileHash,
isExpandDown,
};
}
function getLine(file, type, index) {
const source = lineSources[type];
const handler = lineHandlers[type];
return handler(file[source][index]);
}
describe('DiffExpansionCell', () => {
const matchLine = diffFileMockData.highlighted_diff_lines[5];
let mockFile;
let mockLine;
let store;
let vm;
beforeEach(() => {
mockFile = cloneDeep(diffFileMockData);
mockLine = getLine(mockFile, INLINE_DIFF_VIEW_TYPE, LINE_TO_USE);
store = createStore();
store.state.diffs.diffFiles = [mockFile];
spyOn(store, 'dispatch').and.returnValue(Promise.resolve());
});
const createComponent = (options = {}) => {
const cmp = Vue.extend(DiffExpansionCell);
const defaults = {
fileHash: diffFileMockData.file_hash,
fileHash: mockFile.file_hash,
contextLinesPath: 'contextLinesPath',
line: matchLine,
line: mockLine,
isTop: false,
isBottom: false,
};
const props = Object.assign({}, defaults, options);
return createComponentWithStore(cmp, createStore(), props).$mount();
vm = createComponentWithStore(cmp, store, props).$mount();
};
const findExpandUp = () => vm.$el.querySelector(EXPAND_UP_CLASS);
const findExpandDown = () => vm.$el.querySelector(EXPAND_DOWN_CLASS);
const findExpandAll = () => vm.$el.querySelector(EXPAND_ALL_CLASS);
describe('top row', () => {
it('should have "expand up" and "show all" option', () => {
const vm = createComponent({
createComponent({
isTop: true,
});
const el = vm.$el;
expect(el.querySelector(EXPAND_UP_CLASS)).not.toBe(null);
expect(el.querySelector(EXPAND_DOWN_CLASS)).toBe(null);
expect(el.querySelector(EXPAND_ALL_CLASS)).not.toBe(null);
expect(findExpandUp()).not.toBe(null);
expect(findExpandDown()).toBe(null);
expect(findExpandAll()).not.toBe(null);
});
});
describe('middle row', () => {
it('should have "expand down", "show all", "expand up" option', () => {
const vm = createComponent();
const el = vm.$el;
createComponent();
expect(el.querySelector(EXPAND_UP_CLASS)).not.toBe(null);
expect(el.querySelector(EXPAND_DOWN_CLASS)).not.toBe(null);
expect(el.querySelector(EXPAND_ALL_CLASS)).not.toBe(null);
expect(findExpandUp()).not.toBe(null);
expect(findExpandDown()).not.toBe(null);
expect(findExpandAll()).not.toBe(null);
});
});
describe('bottom row', () => {
it('should have "expand down" and "show all" option', () => {
const vm = createComponent({
createComponent({
isBottom: true,
});
const el = vm.$el;
expect(el.querySelector(EXPAND_UP_CLASS)).toBe(null);
expect(el.querySelector(EXPAND_DOWN_CLASS)).not.toBe(null);
expect(el.querySelector(EXPAND_ALL_CLASS)).not.toBe(null);
expect(findExpandUp()).toBe(null);
expect(findExpandDown()).not.toBe(null);
expect(findExpandAll()).not.toBe(null);
});
});
describe('any row', () => {
[
{ diffViewType: INLINE_DIFF_VIEW_TYPE, file: { parallel_diff_lines: [] } },
{ diffViewType: PARALLEL_DIFF_VIEW_TYPE, file: { highlighted_diff_lines: [] } },
].forEach(({ diffViewType, file }) => {
describe(`with diffViewType (${diffViewType})`, () => {
beforeEach(() => {
mockLine = getLine(mockFile, diffViewType, LINE_TO_USE);
store.state.diffs.diffFiles = [{ ...mockFile, ...file }];
store.state.diffs.diffViewType = diffViewType;
});
it('does not initially dispatch anything', () => {
expect(store.dispatch).not.toHaveBeenCalled();
});
it('on expand all clicked, dispatch loadMoreLines', () => {
const oldLineNumber = mockLine.meta_data.old_pos;
const newLineNumber = mockLine.meta_data.new_pos;
const previousIndex = getPreviousLineIndex(diffViewType, mockFile, {
oldLineNumber,
newLineNumber,
});
createComponent();
findExpandAll().click();
expect(store.dispatch).toHaveBeenCalledWith(
'diffs/loadMoreLines',
makeLoadMoreLinesPayload({
fileHash: mockFile.file_hash,
toLine: newLineNumber - 1,
sinceLine: previousIndex,
oldLineNumber,
diffViewType,
}),
);
});
it('on expand up clicked, dispatch loadMoreLines', () => {
mockLine.meta_data.old_pos = 200;
mockLine.meta_data.new_pos = 200;
const oldLineNumber = mockLine.meta_data.old_pos;
const newLineNumber = mockLine.meta_data.new_pos;
createComponent();
findExpandUp().click();
expect(store.dispatch).toHaveBeenCalledWith(
'diffs/loadMoreLines',
makeLoadMoreLinesPayload({
fileHash: mockFile.file_hash,
toLine: newLineNumber - 1,
sinceLine: 179,
oldLineNumber,
diffViewType,
unfold: true,
}),
);
});
it('on expand down clicked, dispatch loadMoreLines', () => {
mockFile[lineSources[diffViewType]][LINE_TO_USE + 1] = cloneDeep(
mockFile[lineSources[diffViewType]][LINE_TO_USE],
);
const nextLine = getLine(mockFile, diffViewType, LINE_TO_USE + 1);
nextLine.meta_data.old_pos = 300;
nextLine.meta_data.new_pos = 300;
mockLine.meta_data.old_pos = 200;
mockLine.meta_data.new_pos = 200;
createComponent();
findExpandDown().click();
expect(store.dispatch).toHaveBeenCalledWith('diffs/loadMoreLines', {
endpoint: 'contextLinesPath',
params: {
since: 1,
to: 21, // the load amount, plus 1 line
offset: 0,
view: diffViewType,
unfold: true,
bottom: true,
},
lineNumbers: {
// when expanding down, these are based on the previous line, 0, in this case
oldLineNumber: 0,
newLineNumber: 0,
},
nextLineNumbers: { old_line: 200, new_line: 200 },
fileHash: mockFile.file_hash,
isExpandDown: true,
});
});
});
});
});
});
......@@ -167,7 +167,7 @@ describe('DiffsStoreMutations', () => {
highlighted_diff_lines: [],
parallel_diff_lines: [],
};
const state = { diffFiles: [diffFile] };
const state = { diffFiles: [diffFile], diffViewType: 'viewType' };
const lines = [{ old_line: 1, new_line: 1 }];
const findDiffFileSpy = spyOnDependency(mutations, 'findDiffFile').and.returnValue(diffFile);
......@@ -195,6 +195,7 @@ describe('DiffsStoreMutations', () => {
expect(addContextLinesSpy).toHaveBeenCalledWith({
inlineLines: diffFile.highlighted_diff_lines,
parallelLines: diffFile.parallel_diff_lines,
diffViewType: 'viewType',
contextLines: options.contextLines,
bottom: options.params.bottom,
lineNumbers: options.lineNumbers,
......
import { clone } from 'lodash';
import * as utils from '~/diffs/store/utils';
import {
LINE_POSITION_LEFT,
......@@ -8,6 +9,7 @@ import {
NEW_LINE_TYPE,
OLD_LINE_TYPE,
MATCH_LINE_TYPE,
INLINE_DIFF_VIEW_TYPE,
PARALLEL_DIFF_VIEW_TYPE,
} from '~/diffs/constants';
import { MERGE_REQUEST_NOTEABLE_TYPE } from '~/notes/constants';
......@@ -47,7 +49,38 @@ describe('DiffsStoreUtils', () => {
describe('findIndexInParallelLines', () => {
it('should return correct index for given line numbers', () => {
expectSet(utils.findIndexInParallelLines, getDiffFileMock().parallel_diff_lines, {});
expectSet(utils.findIndexInParallelLines, getDiffFileMock().parallel_diff_lines, []);
});
});
});
describe('getPreviousLineIndex', () => {
[
{ diffViewType: INLINE_DIFF_VIEW_TYPE, file: { parallel_diff_lines: [] } },
{ diffViewType: PARALLEL_DIFF_VIEW_TYPE, file: { highlighted_diff_lines: [] } },
].forEach(({ diffViewType, file }) => {
describe(`with diffViewType (${diffViewType}) in split diffs`, () => {
let diffFile;
beforeEach(() => {
diffFile = { ...clone(diffFileMockData), ...file };
});
it('should return the correct previous line number', () => {
const emptyLines =
diffViewType === INLINE_DIFF_VIEW_TYPE
? diffFile.parallel_diff_lines
: diffFile.highlighted_diff_lines;
// This expectation asserts that we cannot possibly be using the opposite view type lines in the next expectation
expect(emptyLines.length).toBe(0);
expect(
utils.getPreviousLineIndex(diffViewType, diffFile, {
oldLineNumber: 3,
newLineNumber: 5,
}),
).toBe(4);
});
});
});
});
......@@ -80,44 +113,59 @@ describe('DiffsStoreUtils', () => {
});
describe('addContextLines', () => {
it('should add context lines', () => {
const diffFile = getDiffFileMock();
const inlineLines = diffFile.highlighted_diff_lines;
const parallelLines = diffFile.parallel_diff_lines;
const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 };
const contextLines = [{ lineNumber: 42, line_code: '123' }];
const options = { inlineLines, parallelLines, contextLines, lineNumbers };
const inlineIndex = utils.findIndexInInlineLines(inlineLines, lineNumbers);
const parallelIndex = utils.findIndexInParallelLines(parallelLines, lineNumbers);
const normalizedParallelLine = {
left: options.contextLines[0],
right: options.contextLines[0],
line_code: '123',
};
utils.addContextLines(options);
expect(inlineLines[inlineIndex]).toEqual(contextLines[0]);
expect(parallelLines[parallelIndex]).toEqual(normalizedParallelLine);
});
it('should add context lines properly with bottom parameter', () => {
const diffFile = getDiffFileMock();
const inlineLines = diffFile.highlighted_diff_lines;
const parallelLines = diffFile.parallel_diff_lines;
const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 };
const contextLines = [{ lineNumber: 42, line_code: '123' }];
const options = { inlineLines, parallelLines, contextLines, lineNumbers, bottom: true };
const normalizedParallelLine = {
left: options.contextLines[0],
right: options.contextLines[0],
line_code: '123',
};
utils.addContextLines(options);
[INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE].forEach(diffViewType => {
it(`should add context lines for ${diffViewType}`, () => {
const diffFile = getDiffFileMock();
const inlineLines = diffFile.highlighted_diff_lines;
const parallelLines = diffFile.parallel_diff_lines;
const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 };
const contextLines = [{ lineNumber: 42, line_code: '123' }];
const options = { inlineLines, parallelLines, contextLines, lineNumbers, diffViewType };
const inlineIndex = utils.findIndexInInlineLines(inlineLines, lineNumbers);
const parallelIndex = utils.findIndexInParallelLines(parallelLines, lineNumbers);
const normalizedParallelLine = {
left: options.contextLines[0],
right: options.contextLines[0],
line_code: '123',
};
utils.addContextLines(options);
if (diffViewType === INLINE_DIFF_VIEW_TYPE) {
expect(inlineLines[inlineIndex]).toEqual(contextLines[0]);
} else {
expect(parallelLines[parallelIndex]).toEqual(normalizedParallelLine);
}
});
expect(inlineLines[inlineLines.length - 1]).toEqual(contextLines[0]);
expect(parallelLines[parallelLines.length - 1]).toEqual(normalizedParallelLine);
it(`should add context lines properly with bottom parameter for ${diffViewType}`, () => {
const diffFile = getDiffFileMock();
const inlineLines = diffFile.highlighted_diff_lines;
const parallelLines = diffFile.parallel_diff_lines;
const lineNumbers = { oldLineNumber: 3, newLineNumber: 5 };
const contextLines = [{ lineNumber: 42, line_code: '123' }];
const options = {
inlineLines,
parallelLines,
contextLines,
lineNumbers,
bottom: true,
diffViewType,
};
const normalizedParallelLine = {
left: options.contextLines[0],
right: options.contextLines[0],
line_code: '123',
};
utils.addContextLines(options);
if (diffViewType === INLINE_DIFF_VIEW_TYPE) {
expect(inlineLines[inlineLines.length - 1]).toEqual(contextLines[0]);
} else {
expect(parallelLines[parallelLines.length - 1]).toEqual(normalizedParallelLine);
}
});
});
});
......
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