From 4b0ac0853c0e655040887659c2b7e51a83812104 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 8 Mar 2023 01:26:13 -0700 Subject: [PATCH 1/6] Add action to fetch a single file --- app/assets/javascripts/diffs/store/actions.js | 43 ++++++++ spec/frontend/diffs/store/actions_spec.js | 103 ++++++++++++++++++ 2 files changed, 146 insertions(+) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 9236e14beb195b..20a67914c95fd6 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -62,6 +62,8 @@ import { idleCallback, allDiscussionWrappersExpanded, prepareLineForRenamedFile, + parseUrlHashAsFileHash, + isUrlHashNoteLink, } from './utils'; export const setBaseConfig = ({ commit }, options) => { @@ -101,6 +103,47 @@ export const setBaseConfig = ({ commit }, options) => { }); }; +export const fetchFileByFile = async ({ state, getters, commit }) => { + const isNoteLink = isUrlHashNoteLink(window?.location?.hash); + const id = parseUrlHashAsFileHash(window?.location?.hash, state.currentDiffFileId); + const treeEntry = id + ? getters.flatBlobsList.find((file) => file.fileHash === id) + : getters.flatBlobsList[0]; + + if (treeEntry && !treeEntry.diffLoaded) { + // Overloading "batch" loading indicators so the UI stays mostly the same + commit(types.SET_BATCH_LOADING_STATE, 'loading'); + commit(types.SET_RETRIEVING_BATCHES, true); + + const urlParams = { + old_path: treeEntry.filePaths.old, + new_path: treeEntry.filePaths.new, + w: state.showWhitespace ? '0' : '1', + view: 'inline', + }; + + axios + .get(mergeUrlParams({ ...urlParams }, state.endpointDiffForPath)) + .then(({ data: diffData }) => { + commit(types.SET_DIFF_DATA_BATCH, { diff_files: diffData.diff_files }); + + if (!isNoteLink && !state.currentDiffFileId) { + commit(types.SET_CURRENT_DIFF_FILE, state.diffFiles[0]?.file_hash); + } + + commit(types.SET_BATCH_LOADING_STATE, 'loaded'); + + eventHub.$emit('diffFilesModified'); + }) + .catch(() => { + commit(types.SET_BATCH_LOADING_STATE, 'error'); + }) + .finally(() => { + commit(types.SET_RETRIEVING_BATCHES, false); + }); + } +}; + export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => { let perPage = state.viewDiffsFileByFile ? 1 : 5; let increaseAmount = 1.4; diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index b00076504e3b34..4326f097cb0d27 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -1,3 +1,4 @@ +import { nextTick } from 'vue'; import MockAdapter from 'axios-mock-adapter'; import Cookies from '~/lib/utils/cookies'; import { useLocalStorageSpy } from 'helpers/local_storage_helper'; @@ -24,6 +25,7 @@ import { } from '~/lib/utils/http_status'; import { mergeUrlParams } from '~/lib/utils/url_utility'; import eventHub from '~/notes/event_hub'; +import diffsEventHub from '~/diffs/event_hub'; import { diffMetadata } from '../mock_data/diff_metadata'; jest.mock('~/alert'); @@ -135,6 +137,107 @@ describe('DiffsStoreActions', () => { }); }); + describe('fetchFileByFile', () => { + beforeEach(() => { + window.location.hash = '#e334a2a10f036c00151a04cea7938a5d4213a818'; + }); + + it('should do nothing if there is no tree entry for the file ID', () => { + return testAction(diffActions.fetchFileByFile, {}, { flatBlobsList: [] }, [], []); + }); + + it('should do nothing if the tree entry for the file ID has already been marked as loaded', () => { + return testAction( + diffActions.fetchFileByFile, + {}, + { + flatBlobsList: [ + { fileHash: 'e334a2a10f036c00151a04cea7938a5d4213a818', diffLoaded: true }, + ], + }, + [], + [], + ); + }); + + describe('when a tree entry exists for the file, but it has not been marked as loaded', () => { + const endpointDiffForPath = '/diffs/set/endpoint/path'; + const diffForPath = mergeUrlParams( + { + old_path: 'old/123', + new_path: 'new/123', + w: '1', + view: 'inline', + }, + endpointDiffForPath, + ); + const treeEntry = { + fileHash: 'e334a2a10f036c00151a04cea7938a5d4213a818', + filePaths: { old: 'old/123', new: 'new/123' }, + }; + const fileResult = { + diff_files: [{ file_hash: 'e334a2a10f036c00151a04cea7938a5d4213a818' }], + }; + const getters = { + flatBlobsList: [treeEntry], + }; + let state; + let commit; + let hubSpy; + + beforeEach(() => { + commit = jest.fn(); + state = { + endpointDiffForPath, + diffFiles: [fileResult], + }; + getters.flatBlobsList = [treeEntry]; + hubSpy = jest.spyOn(diffsEventHub, '$emit'); + }); + + it('does some standard work every time', async () => { + mock.onGet(diffForPath).reply(HTTP_STATUS_OK, fileResult); + + await diffActions.fetchFileByFile({ state, getters, commit }); + + expect(commit).toHaveBeenCalledWith(types.SET_BATCH_LOADING_STATE, 'loading'); + expect(commit).toHaveBeenCalledWith(types.SET_RETRIEVING_BATCHES, true); + + // wait for the mocked network request to return and start processing the .then + // This always requires 5 ticks + await nextTick(); + await nextTick(); + await nextTick(); + await nextTick(); + await nextTick(); + + expect(commit).toHaveBeenCalledWith(types.SET_DIFF_DATA_BATCH, fileResult); + expect(commit).toHaveBeenCalledWith(types.SET_BATCH_LOADING_STATE, 'loaded'); + + expect(hubSpy).toHaveBeenCalledWith('diffFilesModified'); + }); + + it("sets the current file to the first diff file if it's not a note hash and there isn't a current ID set", async () => { + mock.onGet(diffForPath).reply(HTTP_STATUS_OK, fileResult); + + await diffActions.fetchFileByFile({ state, getters, commit }); + + // wait for the mocked network request to return and start processing the .then + // This always requires 5 ticks + await nextTick(); + await nextTick(); + await nextTick(); + await nextTick(); + await nextTick(); + + expect(commit).toHaveBeenCalledWith( + types.SET_CURRENT_DIFF_FILE, + state.diffFiles[0].file_hash, + ); + }); + }); + }); + describe('fetchDiffFilesBatch', () => { it('should fetch batch diff files', () => { const endpointBatch = '/fetch/diffs_batch'; -- GitLab From e647f7bb48413adaececf34b7c58addb5494f29a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 14 Mar 2023 16:03:24 -0600 Subject: [PATCH 2/6] Do a secondary check for a file being loaded In one scenario, a batch of files could load before the metadata loads, which would mean they would never get marked as having been loaded. To avoid expensive deep looping over the meta+files every time each one changes, we're deferring the "fallback" check until the last possible moment (right before the actual load occurs). This is in response to and resolves this discussion thread: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/113941/diffs#note_1308132367 --- app/assets/javascripts/diffs/store/actions.js | 2 +- spec/frontend/diffs/store/actions_spec.js | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 20a67914c95fd6..490e964f38a1c8 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -110,7 +110,7 @@ export const fetchFileByFile = async ({ state, getters, commit }) => { ? getters.flatBlobsList.find((file) => file.fileHash === id) : getters.flatBlobsList[0]; - if (treeEntry && !treeEntry.diffLoaded) { + if (treeEntry && !treeEntry.diffLoaded && !getters.getDiffFileByHash(id)) { // Overloading "batch" loading indicators so the UI stays mostly the same commit(types.SET_BATCH_LOADING_STATE, 'loading'); commit(types.SET_RETRIEVING_BATCHES, true); diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 4326f097cb0d27..f55855c00749b7 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -139,7 +139,7 @@ describe('DiffsStoreActions', () => { describe('fetchFileByFile', () => { beforeEach(() => { - window.location.hash = '#e334a2a10f036c00151a04cea7938a5d4213a818'; + window.location.hash = 'e334a2a10f036c00151a04cea7938a5d4213a818'; }); it('should do nothing if there is no tree entry for the file ID', () => { @@ -180,6 +180,7 @@ describe('DiffsStoreActions', () => { }; const getters = { flatBlobsList: [treeEntry], + getDiffFileByHash: () => undefined, }; let state; let commit; @@ -195,6 +196,19 @@ describe('DiffsStoreActions', () => { hubSpy = jest.spyOn(diffsEventHub, '$emit'); }); + it('does nothing if the file already exists in the loaded diff files', () => { + return testAction( + diffActions.fetchFileByFile, + {}, + { + flatBlobsList: [], + getDiffFileByHash: () => fileResult.diff_files[0], + }, + [], + [], + ); + }); + it('does some standard work every time', async () => { mock.onGet(diffForPath).reply(HTTP_STATUS_OK, fileResult); -- GitLab From 2a08c663d1d0f1572344de584c379b449ca96211 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 20 Mar 2023 15:11:27 -0600 Subject: [PATCH 3/6] Add default string values where not having them could fatal [Review response] --- app/assets/javascripts/diffs/store/utils.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 3739ef0cd55c8a..4ca353333b7ade 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -559,19 +559,19 @@ export const allDiscussionWrappersExpanded = (diff) => { return discussionsExpanded; }; -export function isUrlHashNoteLink(urlHash) { +export function isUrlHashNoteLink(urlHash = '') { const id = urlHash.replace(/^#/, ''); return id.startsWith('note'); } -export function isUrlHashFileHeader(urlHash) { +export function isUrlHashFileHeader(urlHash = '') { const id = urlHash.replace(/^#/, ''); return id.startsWith('diff-content'); } -export function parseUrlHashAsFileHash(urlHash, currentDiffFileId = '') { +export function parseUrlHashAsFileHash(urlHash = '', currentDiffFileId = '') { const isNoteLink = isUrlHashNoteLink(urlHash); let id = urlHash.replace(/^#/, ''); -- GitLab From ec8d3ccb259a089e1a4bbf913da486f146a0ed60 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 20 Mar 2023 15:18:17 -0600 Subject: [PATCH 4/6] Use object expansion to get a parameter property to test [Review response] --- app/assets/javascripts/diffs/store/actions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 490e964f38a1c8..9dbd07f68cc724 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -107,7 +107,7 @@ export const fetchFileByFile = async ({ state, getters, commit }) => { const isNoteLink = isUrlHashNoteLink(window?.location?.hash); const id = parseUrlHashAsFileHash(window?.location?.hash, state.currentDiffFileId); const treeEntry = id - ? getters.flatBlobsList.find((file) => file.fileHash === id) + ? getters.flatBlobsList.find(({ fileHash }) => fileHash === id) : getters.flatBlobsList[0]; if (treeEntry && !treeEntry.diffLoaded && !getters.getDiffFileByHash(id)) { -- GitLab From 2a929cd18a928b13e61753aa08a8a0e432ee8cc9 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 20 Mar 2023 15:35:55 -0600 Subject: [PATCH 5/6] If we don't have diff files, make sure the diff file ID is a string [Review response] --- app/assets/javascripts/diffs/store/actions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 9dbd07f68cc724..9c5f2043933d77 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -128,7 +128,7 @@ export const fetchFileByFile = async ({ state, getters, commit }) => { commit(types.SET_DIFF_DATA_BATCH, { diff_files: diffData.diff_files }); if (!isNoteLink && !state.currentDiffFileId) { - commit(types.SET_CURRENT_DIFF_FILE, state.diffFiles[0]?.file_hash); + commit(types.SET_CURRENT_DIFF_FILE, state.diffFiles[0]?.file_hash || ''); } commit(types.SET_BATCH_LOADING_STATE, 'loaded'); -- GitLab From 7e3630aa174cf92b303fbfacc108379265cd7e1a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 20 Mar 2023 17:35:25 -0600 Subject: [PATCH 6/6] Refactor tests Avoids fully-mocked internal calls, which tests more cases [Review response] --- spec/frontend/diffs/store/actions_spec.js | 67 ++++++++++------------- 1 file changed, 29 insertions(+), 38 deletions(-) diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index f55855c00749b7..3067f3791c530d 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -1,6 +1,6 @@ -import { nextTick } from 'vue'; import MockAdapter from 'axios-mock-adapter'; import Cookies from '~/lib/utils/cookies'; +import waitForPromises from 'helpers/wait_for_promises'; import { useLocalStorageSpy } from 'helpers/local_storage_helper'; import { TEST_HOST } from 'helpers/test_constants'; import testAction from 'helpers/vuex_action_helper'; @@ -161,6 +161,9 @@ describe('DiffsStoreActions', () => { }); describe('when a tree entry exists for the file, but it has not been marked as loaded', () => { + let state; + let commit; + let hubSpy; const endpointDiffForPath = '/diffs/set/endpoint/path'; const diffForPath = mergeUrlParams( { @@ -180,33 +183,25 @@ describe('DiffsStoreActions', () => { }; const getters = { flatBlobsList: [treeEntry], - getDiffFileByHash: () => undefined, + getDiffFileByHash(hash) { + return state.diffFiles?.find((entry) => entry.file_hash === hash); + }, }; - let state; - let commit; - let hubSpy; beforeEach(() => { commit = jest.fn(); state = { endpointDiffForPath, - diffFiles: [fileResult], + diffFiles: [], }; getters.flatBlobsList = [treeEntry]; hubSpy = jest.spyOn(diffsEventHub, '$emit'); }); it('does nothing if the file already exists in the loaded diff files', () => { - return testAction( - diffActions.fetchFileByFile, - {}, - { - flatBlobsList: [], - getDiffFileByHash: () => fileResult.diff_files[0], - }, - [], - [], - ); + state.diffFiles = fileResult.diff_files; + + return testAction(diffActions.fetchFileByFile, state, getters, [], []); }); it('does some standard work every time', async () => { @@ -218,12 +213,7 @@ describe('DiffsStoreActions', () => { expect(commit).toHaveBeenCalledWith(types.SET_RETRIEVING_BATCHES, true); // wait for the mocked network request to return and start processing the .then - // This always requires 5 ticks - await nextTick(); - await nextTick(); - await nextTick(); - await nextTick(); - await nextTick(); + await waitForPromises(); expect(commit).toHaveBeenCalledWith(types.SET_DIFF_DATA_BATCH, fileResult); expect(commit).toHaveBeenCalledWith(types.SET_BATCH_LOADING_STATE, 'loaded'); @@ -231,24 +221,25 @@ describe('DiffsStoreActions', () => { expect(hubSpy).toHaveBeenCalledWith('diffFilesModified'); }); - it("sets the current file to the first diff file if it's not a note hash and there isn't a current ID set", async () => { - mock.onGet(diffForPath).reply(HTTP_STATUS_OK, fileResult); + it.each` + urlHash | diffFiles | expected + ${treeEntry.fileHash} | ${[]} | ${''} + ${'abcdef1234567890'} | ${fileResult.diff_files} | ${'e334a2a10f036c00151a04cea7938a5d4213a818'} + `( + "sets the current file to the first diff file ('$id') if it's not a note hash and there isn't a current ID set", + async ({ urlHash, diffFiles, expected }) => { + window.location.hash = urlHash; + mock.onGet(diffForPath).reply(HTTP_STATUS_OK, fileResult); + state.diffFiles = diffFiles; - await diffActions.fetchFileByFile({ state, getters, commit }); + await diffActions.fetchFileByFile({ state, getters, commit }); - // wait for the mocked network request to return and start processing the .then - // This always requires 5 ticks - await nextTick(); - await nextTick(); - await nextTick(); - await nextTick(); - await nextTick(); - - expect(commit).toHaveBeenCalledWith( - types.SET_CURRENT_DIFF_FILE, - state.diffFiles[0].file_hash, - ); - }); + // wait for the mocked network request to return and start processing the .then + await waitForPromises(); + + expect(commit).toHaveBeenCalledWith(types.SET_CURRENT_DIFF_FILE, expected); + }, + ); }); }); -- GitLab