From 4cac93b334f3572e653197d25b2490fc43cc2930 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Feb 2020 12:20:44 -0700 Subject: [PATCH 1/5] 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. --- .../diffs/components/diff_expansion_cell.vue | 24 +++- app/assets/javascripts/diffs/store/utils.js | 17 +++ .../components/diff_expansion_cell_spec.js | 119 +++++++++++++++--- spec/javascripts/diffs/store/utils_spec.js | 33 +++++ 4 files changed, 169 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_expansion_cell.vue b/app/assets/javascripts/diffs/components/diff_expansion_cell.vue index 23fbfc2b74badd..4eae2e09c08dd7 100644 --- a/app/assets/javascripts/diffs/components/diff_expansion_cell.vue +++ b/app/assets/javascripts/diffs/components/diff_expansion_cell.vue @@ -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; diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 80972d2aeb8be3..062edefa0454cc 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -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') { @@ -112,6 +114,21 @@ 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); diff --git a/spec/javascripts/diffs/components/diff_expansion_cell_spec.js b/spec/javascripts/diffs/components/diff_expansion_cell_spec.js index e8ff6778512c06..3ff280986210ba 100644 --- a/spec/javascripts/diffs/components/diff_expansion_cell_spec.js +++ b/spec/javascripts/diffs/components/diff_expansion_cell_spec.js @@ -1,7 +1,10 @@ import Vue from 'vue'; +import { clone } 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'; @@ -9,56 +12,134 @@ const EXPAND_DOWN_CLASS = '.js-unfold-down'; const EXPAND_ALL_CLASS = '.js-unfold-all'; describe('DiffExpansionCell', () => { - const matchLine = diffFileMockData.highlighted_diff_lines[5]; + let mockFile; + let mockLine; + let store; + let vm; + + const getMockLineByViewType = type => { + const LINE_TO_USE = 5; + const sources = { + [INLINE_DIFF_VIEW_TYPE]: 'highlighted_diff_lines', + [PARALLEL_DIFF_VIEW_TYPE]: 'parallel_diff_lines', + }; + const linesHandlers = { + [INLINE_DIFF_VIEW_TYPE]: line => line, + [PARALLEL_DIFF_VIEW_TYPE]: line => line.right || line.left, + }; + const lines = mockFile[sources[type]]; + + mockLine = linesHandlers[type](lines[LINE_TO_USE]); + }; + + beforeEach(() => { + mockFile = clone(diffFileMockData); + getMockLineByViewType(INLINE_DIFF_VIEW_TYPE); + 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('default', () => { + [ + { 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(() => { + getMockLineByViewType(diffViewType); + 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', { + endpoint: 'contextLinesPath', + params: { + since: previousIndex, + to: newLineNumber - 1, + bottom: false, + offset: newLineNumber - oldLineNumber, + unfold: false, + view: diffViewType, + }, + lineNumbers: { + oldLineNumber, + newLineNumber, + }, + fileHash: mockFile.file_hash, + isExpandDown: false, + nextLineNumbers: {}, + }); + }); + }); }); }); }); diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 051820cedfa15c..07a9cf758efb9e 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -1,3 +1,4 @@ +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'; @@ -52,6 +54,37 @@ describe('DiffsStoreUtils', () => { }); }); + 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); + }); + }); + }); + }); + describe('removeMatchLine', () => { it('should remove match line properly by regarding the bottom parameter', () => { const diffFile = getDiffFileMock(); -- GitLab From cad3211e30679c73b5f46062cfe9a15772b8ea9a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 19 Feb 2020 01:48:04 -0700 Subject: [PATCH 2/5] Fix diff expansion expecting both view types to be present --- .../javascripts/diffs/store/mutations.js | 1 + app/assets/javascripts/diffs/store/utils.js | 47 ++++++++++++++----- .../javascripts/diffs/store/mutations_spec.js | 3 +- 3 files changed, 39 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index c26411af5d73a2..086a7872a5d37a 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -140,6 +140,7 @@ export default { addContextLines({ inlineLines: diffFile.highlighted_diff_lines, parallelLines: diffFile.parallel_diff_lines, + diffViewType: state.diffViewType, contextLines: lines, bottom, lineNumbers, diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 062edefa0454cc..29133c814ea31a 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -95,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, ); }; @@ -104,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 && @@ -134,8 +132,12 @@ export function removeMatchLine(diffFile, lineNumbers, bottom) { 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) { @@ -177,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, @@ -187,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 diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index cb89a89e2165fb..ffe5d89e615ef6 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -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, -- GitLab From f30f84e1136a55508de38bfb639f8ddf2e77d641 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 19 Feb 2020 01:48:29 -0700 Subject: [PATCH 3/5] Add tests for expanding up and down --- .../components/diff_expansion_cell_spec.js | 145 ++++++++++++++---- 1 file changed, 116 insertions(+), 29 deletions(-) diff --git a/spec/javascripts/diffs/components/diff_expansion_cell_spec.js b/spec/javascripts/diffs/components/diff_expansion_cell_spec.js index 3ff280986210ba..04566161d8e7a8 100644 --- a/spec/javascripts/diffs/components/diff_expansion_cell_spec.js +++ b/spec/javascripts/diffs/components/diff_expansion_cell_spec.js @@ -1,5 +1,5 @@ import Vue from 'vue'; -import { clone } from 'lodash'; +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'; @@ -10,6 +10,53 @@ 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', () => { let mockFile; @@ -17,24 +64,9 @@ describe('DiffExpansionCell', () => { let store; let vm; - const getMockLineByViewType = type => { - const LINE_TO_USE = 5; - const sources = { - [INLINE_DIFF_VIEW_TYPE]: 'highlighted_diff_lines', - [PARALLEL_DIFF_VIEW_TYPE]: 'parallel_diff_lines', - }; - const linesHandlers = { - [INLINE_DIFF_VIEW_TYPE]: line => line, - [PARALLEL_DIFF_VIEW_TYPE]: line => line.right || line.left, - }; - const lines = mockFile[sources[type]]; - - mockLine = linesHandlers[type](lines[LINE_TO_USE]); - }; - beforeEach(() => { - mockFile = clone(diffFileMockData); - getMockLineByViewType(INLINE_DIFF_VIEW_TYPE); + 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()); @@ -92,14 +124,14 @@ describe('DiffExpansionCell', () => { }); }); - describe('default', () => { + 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(() => { - getMockLineByViewType(diffViewType); + mockLine = getLine(mockFile, diffViewType, LINE_TO_USE); store.state.diffs.diffFiles = [{ ...mockFile, ...file }]; store.state.diffs.diffViewType = diffViewType; }); @@ -120,23 +152,78 @@ describe('DiffExpansionCell', () => { 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; + + const oldLineNumber = mockLine.meta_data.old_pos; + const newLineNumber = mockLine.meta_data.new_pos; + + createComponent(); + + findExpandDown().click(); + expect(store.dispatch).toHaveBeenCalledWith('diffs/loadMoreLines', { endpoint: 'contextLinesPath', params: { - since: previousIndex, - to: newLineNumber - 1, - bottom: false, - offset: newLineNumber - oldLineNumber, - unfold: false, + since: 1, + to: 21, // the load amount, plus 1 line + offset: 0, view: diffViewType, + unfold: true, + bottom: true, }, lineNumbers: { - oldLineNumber, - newLineNumber, + // 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: false, - nextLineNumbers: {}, + isExpandDown: true, }); }); }); -- GitLab From b789378b0d97eb0deb1231625220362fe3e2186f Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 19 Feb 2020 11:57:56 +0000 Subject: [PATCH 4/5] Added feature spec --- .../merge_request/user_views_diffs_spec.rb | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/spec/features/merge_request/user_views_diffs_spec.rb b/spec/features/merge_request/user_views_diffs_spec.rb index e0e4058dd47056..cd0cf1cc78afe7 100644 --- a/spec/features/merge_request/user_views_diffs_spec.rb +++ b/spec/features/merge_request/user_views_diffs_spec.rb @@ -7,10 +7,11 @@ 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 @@ 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 @@ 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') -- GitLab From 59b3ff3eb69078bec61c17f607bdf3ad71e64a3b Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 19 Feb 2020 13:41:38 +0000 Subject: [PATCH 5/5] Fixed failing karma specs --- .../components/diff_expansion_cell_spec.js | 3 - spec/javascripts/diffs/store/utils_spec.js | 91 +++++++++++-------- 2 files changed, 53 insertions(+), 41 deletions(-) diff --git a/spec/javascripts/diffs/components/diff_expansion_cell_spec.js b/spec/javascripts/diffs/components/diff_expansion_cell_spec.js index 04566161d8e7a8..9a5048d93326c8 100644 --- a/spec/javascripts/diffs/components/diff_expansion_cell_spec.js +++ b/spec/javascripts/diffs/components/diff_expansion_cell_spec.js @@ -199,9 +199,6 @@ describe('DiffExpansionCell', () => { 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(); findExpandDown().click(); diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 07a9cf758efb9e..223c4d7e40baf5 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -49,7 +49,7 @@ 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, []); }); }); }); @@ -113,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); + } + }); }); }); -- GitLab