diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 08306312c2e7a6c42d34a7bb2b8cfb40a2e94148..dcf2595edb862f73b2e57efaaa03ad9822cdaa01 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -42,7 +42,7 @@ import { import diffsEventHub from '../event_hub'; import { reviewStatuses } from '../utils/file_reviews'; import { diffsApp } from '../utils/performance'; -import { updateChangesTabCount } from '../utils/merge_request'; +import { updateChangesTabCount, extractFileHash } from '../utils/merge_request'; import { queueRedisHllEvents } from '../utils/queue_events'; import FindingsDrawer from './shared/findings_drawer.vue'; import CollapsedFilesWarning from './collapsed_files_warning.vue'; @@ -344,12 +344,13 @@ export default { ...mapActions(['startTaskList']), ...mapActions('diffs', [ 'moveToNeighboringCommit', - 'setBaseConfig', 'setCodequalityEndpoint', 'setSastEndpoint', 'fetchDiffFilesMeta', 'fetchDiffFilesBatch', 'fetchFileByFile', + 'loadCollapsedDiff', + 'setFileForcedOpen', 'fetchCoverageFiles', 'fetchCodequality', 'fetchSast', @@ -373,15 +374,34 @@ export default { notesEventHub.$on('refetchDiffData', this.refetchDiffData); notesEventHub.$on('fetchedNotesData', this.rereadNoteHash); diffsEventHub.$on('diffFilesModified', this.setDiscussions); + diffsEventHub.$on('doneLoadingBatches', this.autoScroll); diffsEventHub.$on(EVT_MR_PREPARED, this.fetchData); }, unsubscribeFromEvents() { diffsEventHub.$off(EVT_MR_PREPARED, this.fetchData); + diffsEventHub.$off('doneLoadingBatches', this.autoScroll); diffsEventHub.$off('diffFilesModified', this.setDiscussions); notesEventHub.$off('fetchedNotesData', this.rereadNoteHash); notesEventHub.$off('refetchDiffData', this.refetchDiffData); notesEventHub.$off('fetchDiffData', this.fetchData); }, + autoScroll() { + const lineCode = window.location.hash; + const sha1InHash = extractFileHash({ input: lineCode }); + + if (sha1InHash) { + const idx = this.diffs.findIndex((diffFile) => diffFile.file_hash === sha1InHash); + const file = this.diffs[idx]; + + this.loadCollapsedDiff({ file }) + .then(() => { + this.setDiscussions(); + this.scrollVirtualScrollerToIndex(idx); + this.setFileForcedOpen({ filePath: file.new_path }); + }) + .catch(() => {}); + } + }, navigateToDiffFileNumber(number) { this.navigateToDiffFileIndex(number - 1); }, diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index f99edced36164a17bc8b9400f2a3252f78ed7d70..13274129f1d3b8ea44527930d5de4f6557c4487c 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -161,6 +161,9 @@ export default { manuallyCollapsed() { return collapsedType(this.file) === DIFF_FILE_MANUAL_COLLAPSE; }, + forcedOpen() { + return this.file.viewer.forceOpen; + }, showBody() { return !this.isCollapsed || this.automaticallyCollapsed; }, @@ -174,6 +177,10 @@ export default { return Boolean(gon.current_user_id); }, isCollapsed() { + if (this.forcedOpen) { + return false; + } + if (collapsedType(this.file) !== DIFF_FILE_MANUAL_COLLAPSE) { return this.viewDiffsFileByFile ? false : this.file.viewer?.automaticallyCollapsed; } @@ -201,6 +208,11 @@ export default { this.manageViewedEffects(); }, }, + 'file.viewer.forceOpen': { + handler: function fileForcedOpenHandler() { + this.handleToggle(); + }, + }, 'file.file_hash': { handler: function hashChangeWatch(newHash, oldHash) { if ( diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue index d62d0e11bff13628be90f17d7770f84d7b9131de..b27afb2d5c2f77a2b0889a403b568a75c3320731 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -232,10 +232,15 @@ export default { 'setCurrentFileHash', 'reviewFile', 'setFileCollapsedByUser', + 'setFileForcedOpen', 'setGenerateTestFilePath', 'toggleFileCommentForm', ]), handleToggleFile() { + this.setFileForcedOpen({ + filePath: this.diffFile.file_path, + forced: false, + }); this.$emit('toggleFile'); }, showForkMessage(e) { @@ -278,6 +283,10 @@ export default { } if ((open && reviewed) || (closed && !reviewed)) { + this.setFileForcedOpen({ + filePath: this.diffFile.file_path, + forced: false, + }); this.$emit('toggleFile'); } }, diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 7c68b5f69f1ac42c398be6efc63d6a136a85e80e..ed8ae795bdafccac80412daed5e69ffa008c356a 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -254,6 +254,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => { if (totalLoaded === pagination.total_pages || pagination.total_pages === null) { commit(types.SET_RETRIEVING_BATCHES, false); + eventHub.$emit('doneLoadingBatches'); // We need to check that the currentDiffFileId points to a file that exists if ( @@ -879,6 +880,7 @@ export function switchToFullDiffFromRenamedFile({ commit }, { diffFile }) { ...diffFile.alternate_viewer, automaticallyCollapsed: false, manuallyCollapsed: false, + forceOpen: false, }, }); commit(types.SET_CURRENT_VIEW_DIFF_FILE_LINES, { filePath: diffFile.file_path, lines }); @@ -893,6 +895,10 @@ export const setFileCollapsedAutomatically = ({ commit }, { filePath, collapsed commit(types.SET_FILE_COLLAPSED, { filePath, collapsed, trigger: DIFF_FILE_AUTOMATIC_COLLAPSE }); }; +export function setFileForcedOpen({ commit }, { filePath, forced }) { + commit(types.SET_FILE_FORCED_OPEN, { filePath, forced }); +} + export const setSuggestPopoverDismissed = ({ commit, state }) => axios .post(state.dismissEndpoint, { diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js index 3df491503a43525a25fec7f362f82da633fcd7f7..c2177bacbcc717a974445ff3a4dfa977552c0059 100644 --- a/app/assets/javascripts/diffs/store/mutation_types.js +++ b/app/assets/javascripts/diffs/store/mutation_types.js @@ -39,6 +39,7 @@ export const REQUEST_FULL_DIFF = 'REQUEST_FULL_DIFF'; export const RECEIVE_FULL_DIFF_SUCCESS = 'RECEIVE_FULL_DIFF_SUCCESS'; export const RECEIVE_FULL_DIFF_ERROR = 'RECEIVE_FULL_DIFF_ERROR'; export const SET_FILE_COLLAPSED = 'SET_FILE_COLLAPSED'; +export const SET_FILE_FORCED_OPEN = 'SET_FILE_FORCED_OPEN'; export const SET_CURRENT_VIEW_DIFF_FILE_LINES = 'SET_CURRENT_VIEW_DIFF_FILE_LINES'; export const ADD_CURRENT_VIEW_DIFF_FILE_LINES = 'ADD_CURRENT_VIEW_DIFF_FILE_LINES'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 3af2d6ee6b14f8bd712cba3e51047c279fe898a5..31369b169f522893588f24e63dda3980623a34c5 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -349,6 +349,11 @@ export default { } } }, + [types.SET_FILE_FORCED_OPEN](state, { filePath, forced = true }) { + const file = state.diffFiles.find((f) => f.file_path === filePath); + + Vue.set(file.viewer, 'forceOpen', forced); + }, [types.SET_CURRENT_VIEW_DIFF_FILE_LINES](state, { filePath, lines }) { const file = state.diffFiles.find((f) => f.file_path === filePath); diff --git a/app/assets/javascripts/diffs/utils/diff_file.js b/app/assets/javascripts/diffs/utils/diff_file.js index 98e1c1cc8493d6429cea7aec253557e6ae33aa11..f20ae6464aec4b290882dda2a2955e8c5aa7a69d 100644 --- a/app/assets/javascripts/diffs/utils/diff_file.js +++ b/app/assets/javascripts/diffs/utils/diff_file.js @@ -35,6 +35,7 @@ function collapsed(file) { return { automaticallyCollapsed: viewer.automaticallyCollapsed || viewer.collapsed || false, manuallyCollapsed: null, + forceOpen: false, }; } diff --git a/app/assets/javascripts/diffs/utils/merge_request.js b/app/assets/javascripts/diffs/utils/merge_request.js index bc81c0b0a05407711c3ef524cce511c65b82fb00..a74c9fe7facd9fa71138b7d37d3d24e7354409a6 100644 --- a/app/assets/javascripts/diffs/utils/merge_request.js +++ b/app/assets/javascripts/diffs/utils/merge_request.js @@ -1,6 +1,7 @@ import { ZERO_CHANGES_ALT_DISPLAY } from '../constants'; const endpointRE = /^(\/?(.+\/)+(.+)\/-\/merge_requests\/(\d+)).*$/i; +const SHA1RE = /([a-f0-9]{40})/g; function getVersionInfo({ endpoint } = {}) { const dummyRoot = 'https://gitlab.com'; @@ -51,3 +52,9 @@ export function getDerivedMergeRequestInformation({ endpoint } = {}) { startSha, }; } + +export function extractFileHash({ input = '' } = {}) { + const matches = input.match(SHA1RE); + + return matches?.[0]; +} diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index e10aad6214cd140bab56c89f5d0500bd5daec183..212def72b90808782a4e1477fecdc0fa368c0163 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -6,6 +6,7 @@ import Vue, { nextTick } from 'vue'; import Vuex from 'vuex'; import setWindowLocation from 'helpers/set_window_location_helper'; import { TEST_HOST } from 'spec/test_constants'; + import App from '~/diffs/components/app.vue'; import CommitWidget from '~/diffs/components/commit_widget.vue'; import CompareVersions from '~/diffs/components/compare_versions.vue'; @@ -17,6 +18,8 @@ import DiffsFileTree from '~/diffs/components/diffs_file_tree.vue'; import CollapsedFilesWarning from '~/diffs/components/collapsed_files_warning.vue'; import HiddenFilesWarning from '~/diffs/components/hidden_files_warning.vue'; +import eventHub from '~/diffs/event_hub'; + import axios from '~/lib/utils/axios_utils'; import { HTTP_STATUS_OK } from '~/lib/utils/http_status'; import { Mousetrap } from '~/lib/mousetrap'; @@ -760,4 +763,29 @@ describe('diffs/components/app', () => { ); }); }); + + describe('autoscroll', () => { + let loadSpy; + + beforeEach(() => { + createComponent(); + loadSpy = jest.spyOn(wrapper.vm, 'loadCollapsedDiff').mockResolvedValue('resolved'); + }); + + it('does nothing if the location hash does not include a file hash', () => { + window.location.hash = 'not_a_file_hash'; + + eventHub.$emit('doneLoadingBatches'); + + expect(loadSpy).not.toHaveBeenCalled(); + }); + + it('requests that the correct file be loaded', () => { + window.location.hash = '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_0_1'; + + eventHub.$emit('doneLoadingBatches'); + + expect(loadSpy).toHaveBeenCalledWith({ file: store.state.diffs.diffFiles[0] }); + }); + }); }); diff --git a/spec/frontend/diffs/components/diff_file_header_spec.js b/spec/frontend/diffs/components/diff_file_header_spec.js index b089825090b082aa64a9c9fcd0021c5e7f30885d..b0d98e0e4a698bbd88c374117b7851d3aa96f174 100644 --- a/spec/frontend/diffs/components/diff_file_header_spec.js +++ b/spec/frontend/diffs/components/diff_file_header_spec.js @@ -8,8 +8,12 @@ import { mockTracking, triggerEvent } from 'helpers/tracking_helper'; import DiffFileHeader from '~/diffs/components/diff_file_header.vue'; import { DIFF_FILE_AUTOMATIC_COLLAPSE, DIFF_FILE_MANUAL_COLLAPSE } from '~/diffs/constants'; -import { reviewFile } from '~/diffs/store/actions'; -import { SET_DIFF_FILE_VIEWED, SET_MR_FILE_REVIEWS } from '~/diffs/store/mutation_types'; +import { reviewFile, setFileForcedOpen } from '~/diffs/store/actions'; +import { + SET_DIFF_FILE_VIEWED, + SET_MR_FILE_REVIEWS, + SET_FILE_FORCED_OPEN, +} from '~/diffs/store/mutation_types'; import { diffViewerModes } from '~/ide/constants'; import { scrollToElement } from '~/lib/utils/common_utils'; import { truncateSha } from '~/lib/utils/text_utility'; @@ -67,6 +71,7 @@ describe('DiffFileHeader component', () => { toggleFullDiff: jest.fn(), setCurrentFileHash: jest.fn(), setFileCollapsedByUser: jest.fn(), + setFileForcedOpen: jest.fn(), reviewFile: jest.fn(), }, }, @@ -138,6 +143,19 @@ describe('DiffFileHeader component', () => { expect(wrapper.emitted().toggleFile).toBeDefined(); }); + it('when header is clicked it triggers the action that removes the value that forces a file to be uncollapsed', () => { + createComponent(); + findHeader().trigger('click'); + + return testAction( + setFileForcedOpen, + { filePath: diffFile.file_path, forced: false }, + {}, + [{ type: SET_FILE_FORCED_OPEN, payload: { filePath: diffFile.file_path, forced: false } }], + [], + ); + }); + it('when collapseIcon is clicked emits toggleFile', async () => { createComponent({ props: { collapsible: true } }); findCollapseButton().vm.$emit('click', new Event('click')); @@ -643,6 +661,44 @@ describe('DiffFileHeader component', () => { expect(Boolean(wrapper.emitted().toggleFile)).toBe(fires); }, ); + + it('removes the property that forces a file to be shown when the file review is toggled', () => { + createComponent({ + props: { + diffFile: { + ...diffFile, + viewer: { + ...diffFile.viewer, + automaticallyCollapsed: false, + manuallyCollapsed: null, + }, + }, + showLocalFileReviews: true, + addMergeRequestButtons: true, + expanded: false, + }, + }); + + findReviewFileCheckbox().vm.$emit('change', true); + + testAction( + setFileForcedOpen, + { filePath: diffFile.file_path, forced: false }, + {}, + [{ type: SET_FILE_FORCED_OPEN, payload: { filePath: diffFile.file_path, forced: false } }], + [], + ); + + findReviewFileCheckbox().vm.$emit('change', false); + + testAction( + setFileForcedOpen, + { filePath: diffFile.file_path, forced: false }, + {}, + [{ type: SET_FILE_FORCED_OPEN, payload: { filePath: diffFile.file_path, forced: false } }], + [], + ); + }); }); it('should render the comment on files button', () => { diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js index 53f135471b7e7fb76ae70b6547d9890f71e5581c..13efd3584b41587aa610286849b4ae17fcae0b31 100644 --- a/spec/frontend/diffs/components/diff_file_spec.js +++ b/spec/frontend/diffs/components/diff_file_spec.js @@ -324,6 +324,22 @@ describe('DiffFile', () => { }); describe('collapsing', () => { + describe('forced open', () => { + it('should have content even when it is automatically collapsed', () => { + makeFileAutomaticallyCollapsed(store); + + expect(findDiffContentArea(wrapper).element.children.length).toBe(1); + expect(wrapper.classes('has-body')).toBe(true); + }); + + it('should have content even when it is manually collapsed', () => { + makeFileManuallyCollapsed(store); + + expect(findDiffContentArea(wrapper).element.children.length).toBe(1); + expect(wrapper.classes('has-body')).toBe(true); + }); + }); + describe(`\`${EVT_EXPAND_ALL_FILES}\` event`, () => { beforeEach(() => { jest.spyOn(wrapper.vm, 'handleToggle').mockImplementation(() => {}); diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 387407a7e4d6cb08c6775f76968d9c336e26cdb5..18e81232b5c0f9c4386d8ba904c42c9a95d6f457 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -1627,6 +1627,7 @@ describe('DiffsStoreActions', () => { name: updatedViewerName, automaticallyCollapsed: false, manuallyCollapsed: false, + forceOpen: false, }; const testData = [{ rich_text: 'test' }, { rich_text: 'file2' }]; let renamedFile; @@ -1673,7 +1674,7 @@ describe('DiffsStoreActions', () => { }); }); - describe('setFileUserCollapsed', () => { + describe('setFileCollapsedByUser', () => { it('commits SET_FILE_COLLAPSED', () => { return testAction( diffActions.setFileCollapsedByUser, @@ -1690,6 +1691,17 @@ describe('DiffsStoreActions', () => { }); }); + describe('setFileForcedOpen', () => { + it('commits SET_FILE_FORCED_OPEN', () => { + return testAction(diffActions.setFileForcedOpen, { filePath: 'test', forced: true }, null, [ + { + type: types.SET_FILE_FORCED_OPEN, + payload: { filePath: 'test', forced: true }, + }, + ]); + }); + }); + describe('setExpandedDiffLines', () => { beforeEach(() => { utils.idleCallback.mockImplementation((cb) => { diff --git a/spec/frontend/diffs/store/mutations_spec.js b/spec/frontend/diffs/store/mutations_spec.js index e87c5d0a9b1e230c456894c6ce37ccf5d19e2c49..fdcf7c3eeab8aec8edbd7886f9b712e1ba2ae075 100644 --- a/spec/frontend/diffs/store/mutations_spec.js +++ b/spec/frontend/diffs/store/mutations_spec.js @@ -1055,4 +1055,14 @@ describe('DiffsStoreMutations', () => { expect(state.diffFiles[0].drafts[0]).toEqual('test'); }); }); + + describe('SET_FILE_FORCED_OPEN', () => { + it('sets the forceOpen property of a diff file viewer correctly', () => { + const state = { diffFiles: [{ file_path: 'abc', viewer: { forceOpen: 'not-a-boolean' } }] }; + + mutations[types.SET_FILE_FORCED_OPEN](state, { filePath: 'abc', force: true }); + + expect(state.diffFiles[0].viewer.forceOpen).toBe(true); + }); + }); }); diff --git a/spec/frontend/diffs/utils/merge_request_spec.js b/spec/frontend/diffs/utils/merge_request_spec.js index 11c0efb9a9c4d1c3f91177749ec6329aa125b1c8..f5145b3c4c78fecc3caa605b787ceb865d48f80b 100644 --- a/spec/frontend/diffs/utils/merge_request_spec.js +++ b/spec/frontend/diffs/utils/merge_request_spec.js @@ -1,6 +1,7 @@ import { updateChangesTabCount, getDerivedMergeRequestInformation, + extractFileHash, } from '~/diffs/utils/merge_request'; import { ZERO_CHANGES_ALT_DISPLAY } from '~/diffs/constants'; import { diffMetadata } from '../mock_data/diff_metadata'; @@ -128,4 +129,19 @@ describe('Merge Request utilities', () => { }); }); }); + + describe('extractFileHash', () => { + const sha1Like = 'abcdef1234567890abcdef1234567890abcdef12'; + const sha1LikeToo = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'; + + it('returns undefined when a SHA1-like string cannot be found in the input', () => { + expect(extractFileHash({ input: 'something' })).toBe(undefined); + }); + + it('returns the first matching string of SHA1-like characters in the input', () => { + const fullString = `#${sha1Like}_34_42--${sha1LikeToo}`; + + expect(extractFileHash({ input: fullString })).toBe(sha1Like); + }); + }); });