diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index b920e0411358e1a1eb88fe46e4f261675fd441be..bd85105ccb4c71f17b037e55d3dbf13c00508aa0 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -111,15 +111,22 @@ export const fetchDiffFilesBatch = ({ commit, state }) => { commit(types.SET_BATCH_LOADING, true); commit(types.SET_RETRIEVING_BATCHES, true); - const getBatch = page => + const getBatch = (page = 1) => axios .get(state.endpointBatch, { - params: { ...urlParams, page }, + params: { + ...urlParams, + page, + }, }) .then(({ data: { pagination, diff_files } }) => { commit(types.SET_DIFF_DATA_BATCH, { diff_files }); commit(types.SET_BATCH_LOADING, false); - if (!pagination.next_page) commit(types.SET_RETRIEVING_BATCHES, false); + + if (!pagination.next_page) { + commit(types.SET_RETRIEVING_BATCHES, false); + } + return pagination.next_page; }) .then(nextPage => nextPage && getBatch(nextPage)) @@ -132,6 +139,11 @@ export const fetchDiffFilesBatch = ({ commit, state }) => { export const fetchDiffFilesMeta = ({ commit, state }) => { const worker = new TreeWorker(); + const urlParams = {}; + + if (state.useSingleDiffStyle) { + urlParams.view = state.diffViewType; + } commit(types.SET_LOADING, true); @@ -142,16 +154,17 @@ export const fetchDiffFilesMeta = ({ commit, state }) => { }); return axios - .get(state.endpointMetadata) + .get(mergeUrlParams(urlParams, state.endpointMetadata)) .then(({ data }) => { const strippedData = { ...data }; + delete strippedData.diff_files; commit(types.SET_LOADING, false); commit(types.SET_MERGE_REQUEST_DIFFS, data.merge_request_diffs || []); commit(types.SET_DIFF_DATA, strippedData); - prepareDiffData(data); - worker.postMessage(data.diff_files); + worker.postMessage(prepareDiffData(data, state.diffFiles)); + return data; }) .catch(() => worker.terminate()); @@ -226,7 +239,7 @@ export const startRenderDiffsQueue = ({ state, commit }) => { const nextFile = state.diffFiles.find( file => !file.renderIt && - (file.viewer && (!file.viewer.collapsed || !file.viewer.name === diffViewerModes.text)), + (file.viewer && (!file.viewer.collapsed || file.viewer.name !== diffViewerModes.text)), ); if (nextFile) { diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 1505be1a0b2f8eba6cc53244d5f5fa0a85d8872d..c26411af5d73a27a7a23d6f256394cdc4afc55a0 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -148,8 +148,8 @@ export default { }, [types.ADD_COLLAPSED_DIFFS](state, { file, data }) { - prepareDiffData(data); - const [newFileData] = data.diff_files.filter(f => f.file_hash === file.file_hash); + const files = prepareDiffData(data); + const [newFileData] = files.filter(f => f.file_hash === file.file_hash); const selectedFile = state.diffFiles.find(f => f.file_hash === file.file_hash); Object.assign(selectedFile, { ...newFileData }); }, diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index b379f1fabef839873c2bfc40e299187dff6c3078..80972d2aeb8be3ec7d2dee897b7fe5c107be3f19 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -217,30 +217,19 @@ function diffFileUniqueId(file) { return `${file.content_sha}-${file.file_hash}`; } -function combineDiffFilesWithPriorFiles(files, prior = []) { - files.forEach(file => { - const id = diffFileUniqueId(file); - const oldMatch = prior.find(oldFile => diffFileUniqueId(oldFile) === id); - - if (oldMatch) { - const missingInline = !file.highlighted_diff_lines; - const missingParallel = !file.parallel_diff_lines; - - if (missingInline) { - Object.assign(file, { - highlighted_diff_lines: oldMatch.highlighted_diff_lines, - }); - } +function mergeTwoFiles(target, source) { + const originalInline = target.highlighted_diff_lines; + const originalParallel = target.parallel_diff_lines; + const missingInline = !originalInline.length; + const missingParallel = !originalParallel.length; - if (missingParallel) { - Object.assign(file, { - parallel_diff_lines: oldMatch.parallel_diff_lines, - }); - } - } - }); - - return files; + return { + ...target, + highlighted_diff_lines: missingInline ? source.highlighted_diff_lines : originalInline, + parallel_diff_lines: missingParallel ? source.parallel_diff_lines : originalParallel, + renderIt: source.renderIt, + collapsed: source.collapsed, + }; } function ensureBasicDiffFileLines(file) { @@ -260,13 +249,16 @@ function cleanRichText(text) { } function prepareLine(line) { - return Object.assign(line, { - rich_text: cleanRichText(line.rich_text), - discussionsExpanded: true, - discussions: [], - hasForm: false, - text: undefined, - }); + if (!line.alreadyPrepared) { + Object.assign(line, { + rich_text: cleanRichText(line.rich_text), + discussionsExpanded: true, + discussions: [], + hasForm: false, + text: undefined, + alreadyPrepared: true, + }); + } } function prepareDiffFileLines(file) { @@ -288,11 +280,11 @@ function prepareDiffFileLines(file) { parallelLinesCount += 1; prepareLine(line.right); } + }); - Object.assign(file, { - inlineLinesCount: inlineLines.length, - parallelLinesCount, - }); + Object.assign(file, { + inlineLinesCount: inlineLines.length, + parallelLinesCount, }); return file; @@ -318,11 +310,26 @@ function finalizeDiffFile(file) { return file; } -export function prepareDiffData(diffData, priorFiles) { - return combineDiffFilesWithPriorFiles(diffData.diff_files, priorFiles) +function deduplicateFilesList(files) { + const dedupedFiles = files.reduce((newList, file) => { + const id = diffFileUniqueId(file); + + return { + ...newList, + [id]: newList[id] ? mergeTwoFiles(newList[id], file) : file, + }; + }, {}); + + return Object.values(dedupedFiles); +} + +export function prepareDiffData(diff, priorFiles = []) { + const cleanedFiles = (diff.diff_files || []) .map(ensureBasicDiffFileLines) .map(prepareDiffFileLines) .map(finalizeDiffFile); + + return deduplicateFilesList([...priorFiles, ...cleanedFiles]); } export function getDiffPositionByLineCode(diffFiles, useSingleDiffStyle) { diff --git a/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb b/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..92d90926c0aeec8ddf293f8519333b2f49498014 --- /dev/null +++ b/spec/features/merge_request/user_interacts_with_batched_mr_diffs_spec.rb @@ -0,0 +1,111 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Batch diffs', :js do + include MergeRequestDiffHelpers + include RepoHelpers + + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'master', target_branch: 'empty-branch') } + + before do + stub_feature_flags(single_mr_diff_view: true) + stub_feature_flags(diffs_batch_load: true) + + sign_in(project.owner) + + visit diffs_project_merge_request_path(merge_request.project, merge_request) + wait_for_requests + + # Add discussion to first line of first file + click_diff_line(find('.diff-file.file-holder:first-of-type tr.line_holder.new:first-of-type')) + page.within('.js-discussion-note-form') do + fill_in('note_note', with: 'First Line Comment') + click_button('Comment') + end + + # Add discussion to first line of last file + click_diff_line(find('.diff-file.file-holder:last-of-type tr.line_holder.new:first-of-type')) + page.within('.js-discussion-note-form') do + fill_in('note_note', with: 'Last Line Comment') + click_button('Comment') + end + + wait_for_requests + end + + it 'assigns discussions to diff files across multiple batch pages' do + # Reload so we know the discussions are persisting across batch loads + visit page.current_url + + # Wait for JS to settle + wait_for_requests + + expect(page).to have_selector('.diff-files-holder .file-holder', count: 39) + + # Confirm discussions are applied to appropriate files (should be contained in multiple diff pages) + page.within('.diff-file.file-holder:first-of-type .notes .timeline-entry .note .note-text') do + expect(page).to have_content('First Line Comment') + end + + page.within('.diff-file.file-holder:last-of-type .notes .timeline-entry .note .note-text') do + expect(page).to have_content('Last Line Comment') + end + end + + context 'when user visits a URL with a link directly to to a discussion' do + context 'which is in the first batched page of diffs' do + it 'scrolls to the correct discussion' do + page.within('.diff-file.file-holder:first-of-type') do + click_link('just now') + end + + visit page.current_url + + wait_for_requests + + # Confirm scrolled to correct UI element + expect(page.find('.diff-file.file-holder:first-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_falsey + expect(page.find('.diff-file.file-holder:last-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_truthy + end + end + + context 'which is in at least page 2 of the batched pages of diffs' do + it 'scrolls to the correct discussion' do + page.within('.diff-file.file-holder:last-of-type') do + click_link('just now') + end + + visit page.current_url + + wait_for_requests + + # Confirm scrolled to correct UI element + expect(page.find('.diff-file.file-holder:first-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_truthy + expect(page.find('.diff-file.file-holder:last-of-type .discussion-notes .timeline-entry li.note[id]').obscured?).to be_falsey + end + end + end + + context 'when user switches view styles' do + before do + find('.js-show-diff-settings').click + click_button 'Side-by-side' + + wait_for_requests + end + + it 'has the correct discussions applied to files across batched pages' do + expect(page).to have_selector('.diff-files-holder .file-holder', count: 39) + + page.within('.diff-file.file-holder:first-of-type .notes .timeline-entry .note .note-text') do + expect(page).to have_content('First Line Comment') + end + + page.within('.diff-file.file-holder:last-of-type .notes .timeline-entry .note .note-text') do + expect(page).to have_content('Last Line Comment') + end + end + end +end diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index af2dd7b4f93c2aba488fed29d0ce6bb17bfe76b9..ff17d8ec1580234ae56948e082a84b6e89c2a80a 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -158,16 +158,19 @@ describe('DiffsStoreActions', () => { const res1 = { diff_files: [], pagination: { next_page: 2 } }; const res2 = { diff_files: [], pagination: {} }; mock - .onGet(endpointBatch, { params: { page: undefined, per_page: DIFFS_PER_PAGE, w: '1' } }) - .reply(200, res1); - mock - .onGet(endpointBatch, { params: { page: 2, per_page: DIFFS_PER_PAGE, w: '1' } }) + .onGet(endpointBatch, { + params: { page: 1, per_page: DIFFS_PER_PAGE, w: '1', view: 'inline' }, + }) + .reply(200, res1) + .onGet(endpointBatch, { + params: { page: 2, per_page: DIFFS_PER_PAGE, w: '1', view: 'inline' }, + }) .reply(200, res2); testAction( fetchDiffFilesBatch, {}, - { endpointBatch }, + { endpointBatch, useSingleDiffStyle: true, diffViewType: 'inline' }, [ { type: types.SET_BATCH_LOADING, payload: true }, { type: types.SET_RETRIEVING_BATCHES, payload: true }, @@ -188,7 +191,7 @@ describe('DiffsStoreActions', () => { describe('fetchDiffFilesMeta', () => { it('should fetch diff meta information', done => { - const endpointMetadata = '/fetch/diffs_meta'; + const endpointMetadata = '/fetch/diffs_meta?view=inline'; const mock = new MockAdapter(axios); const data = { diff_files: [] }; const res = { data }; @@ -213,6 +216,108 @@ describe('DiffsStoreActions', () => { }); }); + describe('when the single diff view feature flag is off', () => { + describe('fetchDiffFiles', () => { + it('should fetch diff files', done => { + const endpoint = '/fetch/diff/files?w=1'; + const mock = new MockAdapter(axios); + const res = { diff_files: 1, merge_request_diffs: [] }; + mock.onGet(endpoint).reply(200, res); + + testAction( + fetchDiffFiles, + {}, + { + endpoint, + diffFiles: [], + showWhitespace: false, + diffViewType: 'inline', + useSingleDiffStyle: false, + }, + [ + { type: types.SET_LOADING, payload: true }, + { type: types.SET_LOADING, payload: false }, + { type: types.SET_MERGE_REQUEST_DIFFS, payload: res.merge_request_diffs }, + { type: types.SET_DIFF_DATA, payload: res }, + ], + [], + () => { + mock.restore(); + done(); + }, + ); + + fetchDiffFiles({ state: { endpoint }, commit: () => null }) + .then(data => { + expect(data).toEqual(res); + done(); + }) + .catch(done.fail); + }); + }); + + describe('fetchDiffFilesBatch', () => { + it('should fetch batch diff files', done => { + const endpointBatch = '/fetch/diffs_batch'; + const mock = new MockAdapter(axios); + const res1 = { diff_files: [], pagination: { next_page: 2 } }; + const res2 = { diff_files: [], pagination: {} }; + mock + .onGet(endpointBatch, { params: { page: 1, per_page: DIFFS_PER_PAGE, w: '1' } }) + .reply(200, res1) + .onGet(endpointBatch, { params: { page: 2, per_page: DIFFS_PER_PAGE, w: '1' } }) + .reply(200, res2); + + testAction( + fetchDiffFilesBatch, + {}, + { endpointBatch, useSingleDiffStyle: false }, + [ + { type: types.SET_BATCH_LOADING, payload: true }, + { type: types.SET_RETRIEVING_BATCHES, payload: true }, + { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: res1.diff_files } }, + { type: types.SET_BATCH_LOADING, payload: false }, + { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: [] } }, + { type: types.SET_BATCH_LOADING, payload: false }, + { type: types.SET_RETRIEVING_BATCHES, payload: false }, + ], + [], + () => { + mock.restore(); + done(); + }, + ); + }); + }); + + describe('fetchDiffFilesMeta', () => { + it('should fetch diff meta information', done => { + const endpointMetadata = '/fetch/diffs_meta?'; + const mock = new MockAdapter(axios); + const data = { diff_files: [] }; + const res = { data }; + mock.onGet(endpointMetadata).reply(200, res); + + testAction( + fetchDiffFilesMeta, + {}, + { endpointMetadata, useSingleDiffStyle: false }, + [ + { type: types.SET_LOADING, payload: true }, + { type: types.SET_LOADING, payload: false }, + { type: types.SET_MERGE_REQUEST_DIFFS, payload: [] }, + { type: types.SET_DIFF_DATA, payload: { data } }, + ], + [], + () => { + mock.restore(); + done(); + }, + ); + }); + }); + }); + describe('setHighlightedRow', () => { it('should mark currently selected diff and set lineHash and fileHash of highlightedRow', () => { testAction(setHighlightedRow, 'ABC_123', {}, [ diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index 24405dcc7968cd810fd6f3908bcd5c0b61abc7de..cb89a89e2165fb226cded87a1135d1544d6b3603 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -55,8 +55,8 @@ describe('DiffsStoreMutations', () => { const state = { diffFiles: [ { - content_sha: diffFileMockData.content_sha, - file_hash: diffFileMockData.file_hash, + ...diffFileMockData, + parallel_diff_lines: [], }, ], }; diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 638b4510221692c320fe3e64a2e599f76dbe19b6..051820cedfa15c322dcecf5972e878235afb4f15 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -333,10 +333,10 @@ describe('DiffsStoreUtils', () => { diff_files: [Object.assign({}, mock, { highlighted_diff_lines: undefined })], }; - utils.prepareDiffData(preparedDiff); - utils.prepareDiffData(splitInlineDiff); - utils.prepareDiffData(splitParallelDiff); - utils.prepareDiffData(completedDiff, [mock]); + preparedDiff.diff_files = utils.prepareDiffData(preparedDiff); + splitInlineDiff.diff_files = utils.prepareDiffData(splitInlineDiff); + splitParallelDiff.diff_files = utils.prepareDiffData(splitParallelDiff); + completedDiff.diff_files = utils.prepareDiffData(completedDiff, [mock]); }); it('sets the renderIt and collapsed attribute on files', () => { @@ -390,6 +390,37 @@ describe('DiffsStoreUtils', () => { expect(completedDiff.diff_files[0].parallel_diff_lines.length).toBeGreaterThan(0); expect(completedDiff.diff_files[0].highlighted_diff_lines.length).toBeGreaterThan(0); }); + + it('leaves files in the existing state', () => { + const priorFiles = [mock]; + const fakeNewFile = { + ...mock, + content_sha: 'ABC', + file_hash: 'DEF', + }; + const updatedFilesList = utils.prepareDiffData({ diff_files: [fakeNewFile] }, priorFiles); + + expect(updatedFilesList).toEqual([mock, fakeNewFile]); + }); + + it('completes an existing split diff without overwriting existing diffs', () => { + // The current state has a file that has only loaded inline lines + const priorFiles = [{ ...mock, parallel_diff_lines: [] }]; + // The next (batch) load loads two files: the other half of that file, and a new file + const fakeBatch = [ + { ...mock, highlighted_diff_lines: undefined }, + { ...mock, highlighted_diff_lines: undefined, content_sha: 'ABC', file_hash: 'DEF' }, + ]; + const updatedFilesList = utils.prepareDiffData({ diff_files: fakeBatch }, priorFiles); + + expect(updatedFilesList).toEqual([ + mock, + jasmine.objectContaining({ + content_sha: 'ABC', + file_hash: 'DEF', + }), + ]); + }); }); describe('isDiscussionApplicableToLine', () => {