From 79166d64943431fdc5d87eaef79eae3be48fda27 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 1 Dec 2020 18:35:33 -0700 Subject: [PATCH 01/10] Add fileByFile setting to the diffs state Mix the user setting with the URL parameter and the cookie --- app/assets/javascripts/diffs/constants.js | 3 ++ .../diffs/store/modules/diff_state.js | 3 ++ .../javascripts/diffs/utils/preferences.js | 22 ++++++++++ spec/frontend/diffs/utils/preferences_spec.js | 40 +++++++++++++++++++ 4 files changed, 68 insertions(+) create mode 100644 app/assets/javascripts/diffs/utils/preferences.js create mode 100644 spec/frontend/diffs/utils/preferences_spec.js diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index 79f8c08e389d01..2120848e55b073 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -77,6 +77,9 @@ export const ALERT_COLLAPSED_FILES = 'collapsed'; export const DIFF_FILE_AUTOMATIC_COLLAPSE = 'automatic'; export const DIFF_FILE_MANUAL_COLLAPSE = 'manual'; +// Diff view single file mode +export const DIFF_FILE_BY_FILE_COOKIE_NAME = 'fileViewMode'; +export const DIFF_VIEW_FILE_BY_FILE = 'single'; // State machine states export const STATE_IDLING = 'idle'; export const STATE_LOADING = 'loading'; diff --git a/app/assets/javascripts/diffs/store/modules/diff_state.js b/app/assets/javascripts/diffs/store/modules/diff_state.js index 001d9d9f5947d9..c331e52c887ebd 100644 --- a/app/assets/javascripts/diffs/store/modules/diff_state.js +++ b/app/assets/javascripts/diffs/store/modules/diff_state.js @@ -5,6 +5,8 @@ import { DIFF_VIEW_COOKIE_NAME, DIFF_WHITESPACE_COOKIE_NAME, } from '../../constants'; + +import { fileByFile } from '../../utils/preferences'; import { getDefaultWhitespace } from '../utils'; const viewTypeFromQueryString = getParameterValues('view')[0]; @@ -39,6 +41,7 @@ export default () => ({ highlightedRow: null, renderTreeList: true, showWhitespace: getDefaultWhitespace(whiteSpaceFromQueryString, whiteSpaceFromCookie), + viewDiffsFileByFile: fileByFile(), fileFinderVisible: false, dismissEndpoint: '', showSuggestPopover: true, diff --git a/app/assets/javascripts/diffs/utils/preferences.js b/app/assets/javascripts/diffs/utils/preferences.js new file mode 100644 index 00000000000000..e440de3350ae0d --- /dev/null +++ b/app/assets/javascripts/diffs/utils/preferences.js @@ -0,0 +1,22 @@ +import Cookies from 'js-cookie'; +import { getParameterValues } from '~/lib/utils/url_utility'; + +import { DIFF_FILE_BY_FILE_COOKIE_NAME, DIFF_VIEW_FILE_BY_FILE } from '../constants'; + +export function fileByFile(pref = false) { + const search = getParameterValues(DIFF_FILE_BY_FILE_COOKIE_NAME)?.[0]; + const cookie = Cookies.get(DIFF_FILE_BY_FILE_COOKIE_NAME); + let viewFileByFile = pref; + + // use the cookie first, if it exists + if (cookie) { + viewFileByFile = cookie === DIFF_VIEW_FILE_BY_FILE; + } + + // the search parameter of the URL should override, if it exists + if (search) { + viewFileByFile = search === DIFF_VIEW_FILE_BY_FILE; + } + + return viewFileByFile; +} diff --git a/spec/frontend/diffs/utils/preferences_spec.js b/spec/frontend/diffs/utils/preferences_spec.js new file mode 100644 index 00000000000000..a48db1d7512409 --- /dev/null +++ b/spec/frontend/diffs/utils/preferences_spec.js @@ -0,0 +1,40 @@ +import Cookies from 'js-cookie'; +import { getParameterValues } from '~/lib/utils/url_utility'; + +import { fileByFile } from '~/diffs/utils/preferences'; +import { + DIFF_FILE_BY_FILE_COOKIE_NAME, + DIFF_VIEW_FILE_BY_FILE, + DIFF_VIEW_ALL_FILES, +} from '~/diffs/constants'; + +jest.mock('~/lib/utils/url_utility'); + +describe('diffs preferences', () => { + describe('fileByFile', () => { + it.each` + result | preference | cookie | searchParam + ${false} | ${false} | ${undefined} | ${undefined} + ${true} | ${true} | ${undefined} | ${undefined} + ${true} | ${false} | ${DIFF_VIEW_FILE_BY_FILE} | ${undefined} + ${false} | ${true} | ${DIFF_VIEW_ALL_FILES} | ${undefined} + ${true} | ${false} | ${undefined} | ${[DIFF_VIEW_FILE_BY_FILE]} + ${false} | ${true} | ${undefined} | ${[DIFF_VIEW_ALL_FILES]} + ${true} | ${false} | ${DIFF_VIEW_FILE_BY_FILE} | ${[DIFF_VIEW_FILE_BY_FILE]} + ${true} | ${true} | ${DIFF_VIEW_ALL_FILES} | ${[DIFF_VIEW_FILE_BY_FILE]} + ${false} | ${false} | ${DIFF_VIEW_ALL_FILES} | ${[DIFF_VIEW_ALL_FILES]} + ${false} | ${true} | ${DIFF_VIEW_FILE_BY_FILE} | ${[DIFF_VIEW_ALL_FILES]} + `( + 'should return $result when { preference: $preference, cookie: $cookie, search: $searchParam }', + ({ result, preference, cookie, searchParam }) => { + if (cookie) { + Cookies.set(DIFF_FILE_BY_FILE_COOKIE_NAME, cookie); + } + + getParameterValues.mockReturnValue(searchParam); + + expect(fileByFile(preference)).toBe(result); + }, + ); + }); +}); -- GitLab From ecae140fe573db53b8f4278f9b84fac4297d7c7a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Sat, 14 Nov 2020 14:33:11 -0700 Subject: [PATCH 02/10] Add mutation to change fileByFile setting --- .../javascripts/diffs/store/mutation_types.js | 1 + app/assets/javascripts/diffs/store/mutations.js | 5 +++++ spec/frontend/diffs/store/mutations_spec.js | 14 ++++++++++++++ 3 files changed, 20 insertions(+) diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js index ed694419ab1100..251840287993ee 100644 --- a/app/assets/javascripts/diffs/store/mutation_types.js +++ b/app/assets/javascripts/diffs/store/mutation_types.js @@ -28,6 +28,7 @@ export const SET_HIGHLIGHTED_ROW = 'SET_HIGHLIGHTED_ROW'; export const SET_TREE_DATA = 'SET_TREE_DATA'; export const SET_RENDER_TREE_LIST = 'SET_RENDER_TREE_LIST'; export const SET_SHOW_WHITESPACE = 'SET_SHOW_WHITESPACE'; +export const SET_FILE_BY_FILE = 'SET_FILE_BY_FILE'; export const TOGGLE_FILE_FINDER_VISIBLE = 'TOGGLE_FILE_FINDER_VISIBLE'; export const REQUEST_FULL_DIFF = 'REQUEST_FULL_DIFF'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 7b08c5e75e1353..69ae3f705e3326 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -36,6 +36,7 @@ export default { projectPath, dismissEndpoint, showSuggestPopover, + viewDiffsFileByFile, } = options; Object.assign(state, { endpoint, @@ -45,6 +46,7 @@ export default { projectPath, dismissEndpoint, showSuggestPopover, + viewDiffsFileByFile, }); }, @@ -352,4 +354,7 @@ export default { [types.SET_SHOW_SUGGEST_POPOVER](state) { state.showSuggestPopover = false; }, + [types.SET_FILE_BY_FILE](state, fileByFile) { + state.viewDiffsFileByFile = fileByFile; + }, }; diff --git a/spec/frontend/diffs/store/mutations_spec.js b/spec/frontend/diffs/store/mutations_spec.js index 2dda9a0ad7199d..c061c5c25906c5 100644 --- a/spec/frontend/diffs/store/mutations_spec.js +++ b/spec/frontend/diffs/store/mutations_spec.js @@ -892,4 +892,18 @@ describe('DiffsStoreMutations', () => { expect(state.showSuggestPopover).toBe(false); }); }); + + describe('SET_FILE_BY_FILE', () => { + it.each` + value | opposite + ${true} | ${false} + ${false} | ${true} + `('sets viewDiffsFileByFile to $value', ({ value, opposite }) => { + const state = { viewDiffsFileByFile: opposite }; + + mutations[types.SET_FILE_BY_FILE](state, value); + + expect(state.viewDiffsFileByFile).toBe(value); + }); + }); }); -- GitLab From 37c2c4a27ace8a6eb1080f6dba3339362bc490d4 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 1 Dec 2020 17:32:44 -0700 Subject: [PATCH 03/10] Add action to change the fileByFile setting --- app/assets/javascripts/diffs/constants.js | 2 ++ app/assets/javascripts/diffs/store/actions.js | 16 ++++++++++++++++ spec/frontend/diffs/store/actions_spec.js | 17 +++++++++++++++++ 3 files changed, 35 insertions(+) diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index 2120848e55b073..8532119dc63855 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -80,6 +80,8 @@ export const DIFF_FILE_MANUAL_COLLAPSE = 'manual'; // Diff view single file mode export const DIFF_FILE_BY_FILE_COOKIE_NAME = 'fileViewMode'; export const DIFF_VIEW_FILE_BY_FILE = 'single'; +export const DIFF_VIEW_ALL_FILES = 'all'; + // State machine states export const STATE_IDLING = 'idle'; export const STATE_LOADING = 'loading'; diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 1a0e65bbb3eb90..51d9fe12102b9a 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -44,6 +44,9 @@ import { EVT_PERF_MARK_FILE_TREE_START, EVT_PERF_MARK_FILE_TREE_END, EVT_PERF_MARK_DIFF_FILES_START, + DIFF_VIEW_FILE_BY_FILE, + DIFF_VIEW_ALL_FILES, + DIFF_FILE_BY_FILE_COOKIE_NAME, } from '../constants'; import { diffViewerModes } from '~/ide/constants'; import { isCollapsed } from '../diff_file'; @@ -57,6 +60,7 @@ export const setBaseConfig = ({ commit }, options) => { projectPath, dismissEndpoint, showSuggestPopover, + viewDiffsFileByFile, } = options; commit(types.SET_BASE_CONFIG, { endpoint, @@ -66,6 +70,7 @@ export const setBaseConfig = ({ commit }, options) => { projectPath, dismissEndpoint, showSuggestPopover, + viewDiffsFileByFile, }); }; @@ -694,3 +699,14 @@ export const navigateToDiffFileIndex = ({ commit, state }, index) => { commit(types.VIEW_DIFF_FILE, fileHash); }; + +export const setFileByFile = ({ commit }, { fileByFile }) => { + const fileViewMode = fileByFile ? DIFF_VIEW_FILE_BY_FILE : DIFF_VIEW_ALL_FILES; + commit(types.SET_FILE_BY_FILE, fileByFile); + + Cookies.set(DIFF_FILE_BY_FILE_COOKIE_NAME, fileViewMode); + + historyPushState( + mergeUrlParams({ [DIFF_FILE_BY_FILE_COOKIE_NAME]: fileViewMode }, window.location.href), + ); +}; diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 0032fe926d0a48..c1a7844d301ee8 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -48,6 +48,7 @@ import { moveToNeighboringCommit, setCurrentDiffFileIdFromNote, navigateToDiffFileIndex, + setFileByFile, } from '~/diffs/store/actions'; import eventHub from '~/notes/event_hub'; import * as types from '~/diffs/store/mutation_types'; @@ -1455,4 +1456,20 @@ describe('DiffsStoreActions', () => { ); }); }); + + describe('setFileByFile', () => { + it.each` + value + ${true} + ${false} + `('commits SET_FILE_BY_FILE with the new value $value', ({ value }) => { + return testAction( + setFileByFile, + { fileByFile: value }, + { viewDiffsFileByFile: null }, + [{ type: types.SET_FILE_BY_FILE, payload: value }], + [], + ); + }); + }); }); -- GitLab From 775f909399b6aeef2b345fdd93abaede49f05366 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 16 Nov 2020 03:13:11 -0700 Subject: [PATCH 04/10] Set the fileByFile state based on user preference ... when the app loads and the pref is available --- .../javascripts/diffs/components/app.vue | 6 ++-- app/assets/javascripts/diffs/index.js | 2 +- spec/frontend/diffs/components/app_spec.js | 34 ++++++++++++------- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 9218cc00303e25..0fe81198d5375d 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -22,6 +22,7 @@ import MergeConflictWarning from './merge_conflict_warning.vue'; import CollapsedFilesWarning from './collapsed_files_warning.vue'; import { diffsApp } from '../utils/performance'; +import { fileByFile } from '../utils/preferences'; import { TREE_LIST_WIDTH_STORAGE_KEY, @@ -114,7 +115,7 @@ export default { required: false, default: false, }, - viewDiffsFileByFile: { + fileByFileUserPreference: { type: Boolean, required: false, default: false, @@ -154,6 +155,7 @@ export default { 'conflictResolutionPath', 'canMerge', 'hasConflicts', + 'viewDiffsFileByFile', ]), ...mapGetters('diffs', ['whichCollapsedTypes', 'isParallelView', 'currentDiffIndex']), ...mapGetters(['isNotesFetched', 'getNoteableData']), @@ -254,7 +256,7 @@ export default { projectPath: this.projectPath, dismissEndpoint: this.dismissEndpoint, showSuggestPopover: this.showSuggestPopover, - viewDiffsFileByFile: this.viewDiffsFileByFile, + viewDiffsFileByFile: fileByFile(this.fileByFileUserPreference), }); if (this.shouldShow) { diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js index 06a138b1e13e7b..587220488beea0 100644 --- a/app/assets/javascripts/diffs/index.js +++ b/app/assets/javascripts/diffs/index.js @@ -116,7 +116,7 @@ export default function initDiffsApp(store) { isFluidLayout: this.isFluidLayout, dismissEndpoint: this.dismissEndpoint, showSuggestPopover: this.showSuggestPopover, - viewDiffsFileByFile: this.viewDiffsFileByFile, + fileByFileUserPreference: this.viewDiffsFileByFile, }, }); }, diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 01c789270dacbc..79bfd5ec63f0d9 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -19,8 +19,8 @@ import diffsMockData from '../mock_data/merge_request_diffs'; const mergeRequestDiff = { version_index: 1 }; const TEST_ENDPOINT = `${TEST_HOST}/diff/endpoint`; -const COMMIT_URL = '[BASE URL]/OLD'; -const UPDATED_COMMIT_URL = '[BASE URL]/NEW'; +const COMMIT_URL = `${TEST_HOST}/COMMIT/OLD`; +const UPDATED_COMMIT_URL = `${TEST_HOST}/COMMIT/NEW`; function getCollapsedFilesWarning(wrapper) { return wrapper.find(CollapsedFilesWarning); @@ -61,7 +61,7 @@ describe('diffs/components/app', () => { changesEmptyStateIllustration: '', dismissEndpoint: '', showSuggestPopover: true, - viewDiffsFileByFile: false, + fileByFileUserPreference: false, ...props, }, provide, @@ -700,12 +700,14 @@ describe('diffs/components/app', () => { }); describe('file-by-file', () => { - it('renders a single diff', () => { - createComponent({ viewDiffsFileByFile: true }, ({ state }) => { + it('renders a single diff', async () => { + createComponent({ fileByFileUserPreference: true }, ({ state }) => { state.diffs.diffFiles.push({ file_hash: '123' }); state.diffs.diffFiles.push({ file_hash: '312' }); }); + await wrapper.vm.$nextTick(); + expect(wrapper.findAll(DiffFile).length).toBe(1); }); @@ -713,31 +715,37 @@ describe('diffs/components/app', () => { const fileByFileNav = () => wrapper.find('[data-testid="file-by-file-navigation"]'); const paginator = () => fileByFileNav().find(GlPagination); - it('sets previous button as disabled', () => { - createComponent({ viewDiffsFileByFile: true }, ({ state }) => { + it('sets previous button as disabled', async () => { + createComponent({ fileByFileUserPreference: true }, ({ state }) => { state.diffs.diffFiles.push({ file_hash: '123' }, { file_hash: '312' }); }); + await wrapper.vm.$nextTick(); + expect(paginator().attributes('prevpage')).toBe(undefined); expect(paginator().attributes('nextpage')).toBe('2'); }); - it('sets next button as disabled', () => { - createComponent({ viewDiffsFileByFile: true }, ({ state }) => { + it('sets next button as disabled', async () => { + createComponent({ fileByFileUserPreference: true }, ({ state }) => { state.diffs.diffFiles.push({ file_hash: '123' }, { file_hash: '312' }); state.diffs.currentDiffFileId = '312'; }); + await wrapper.vm.$nextTick(); + expect(paginator().attributes('prevpage')).toBe('1'); expect(paginator().attributes('nextpage')).toBe(undefined); }); - it("doesn't display when there's fewer than 2 files", () => { - createComponent({ viewDiffsFileByFile: true }, ({ state }) => { + it("doesn't display when there's fewer than 2 files", async () => { + createComponent({ fileByFileUserPreference: true }, ({ state }) => { state.diffs.diffFiles.push({ file_hash: '123' }); state.diffs.currentDiffFileId = '123'; }); + await wrapper.vm.$nextTick(); + expect(fileByFileNav().exists()).toBe(false); }); @@ -748,11 +756,13 @@ describe('diffs/components/app', () => { `( 'it calls navigateToDiffFileIndex with $index when $link is clicked', async ({ currentDiffFileId, targetFile }) => { - createComponent({ viewDiffsFileByFile: true }, ({ state }) => { + createComponent({ fileByFileUserPreference: true }, ({ state }) => { state.diffs.diffFiles.push({ file_hash: '123' }, { file_hash: '312' }); state.diffs.currentDiffFileId = currentDiffFileId; }); + await wrapper.vm.$nextTick(); + jest.spyOn(wrapper.vm, 'navigateToDiffFileIndex'); paginator().vm.$emit('input', targetFile); -- GitLab From 66e502c6fbbbd6870819399c490c9180ab3f5796 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Sat, 14 Nov 2020 14:41:53 -0700 Subject: [PATCH 05/10] Rename app eventHub to correct notesEventHub --- app/assets/javascripts/diffs/components/app.vue | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 0fe81198d5375d..0854d41efb0eef 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -10,7 +10,8 @@ import PanelResizer from '~/vue_shared/components/panel_resizer.vue'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { isSingleViewStyle } from '~/helpers/diffs_helper'; import { updateHistory } from '~/lib/utils/url_utility'; -import eventHub from '../../notes/event_hub'; + +import notesEventHub from '../../notes/event_hub'; import CompareVersions from './compare_versions.vue'; import DiffFile from './diff_file.vue'; import NoChanges from './no_changes.vue'; @@ -280,8 +281,8 @@ export default { created() { this.adjustView(); - eventHub.$once('fetchDiffData', this.fetchData); - eventHub.$on('refetchDiffData', this.refetchDiffData); + notesEventHub.$once('fetchDiffData', this.fetchData); + notesEventHub.$on('refetchDiffData', this.refetchDiffData); this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES; this.unwatchDiscussions = this.$watch( @@ -302,8 +303,9 @@ export default { beforeDestroy() { diffsApp.deinstrument(); - eventHub.$off('fetchDiffData', this.fetchData); - eventHub.$off('refetchDiffData', this.refetchDiffData); + notesEventHub.$off('refetchDiffData', this.refetchDiffData); + notesEventHub.$off('fetchDiffData', this.fetchData); + this.removeEventListeners(); }, methods: { @@ -373,7 +375,7 @@ export default { } if (!this.isNotesFetched) { - eventHub.$emit('fetchNotesData'); + notesEventHub.$emit('fetchNotesData'); } }, setDiscussions() { -- GitLab From 64c7f99aebf112e6bfe51c10aef483404fd75723 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 1 Dec 2020 18:35:47 -0700 Subject: [PATCH 06/10] Respond to diffs app hub requests to change the fileByFile setting --- .../javascripts/diffs/components/app.vue | 10 ++++++++ app/assets/javascripts/diffs/constants.js | 1 + spec/frontend/diffs/components/app_spec.js | 23 +++++++++++++++++++ 3 files changed, 34 insertions(+) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 0854d41efb0eef..53eb8cd8eb8576 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -12,6 +12,8 @@ import { isSingleViewStyle } from '~/helpers/diffs_helper'; import { updateHistory } from '~/lib/utils/url_utility'; import notesEventHub from '../../notes/event_hub'; +import eventHub from '../event_hub'; + import CompareVersions from './compare_versions.vue'; import DiffFile from './diff_file.vue'; import NoChanges from './no_changes.vue'; @@ -36,6 +38,7 @@ import { ALERT_OVERFLOW_HIDDEN, ALERT_MERGE_CONFLICT, ALERT_COLLAPSED_FILES, + EVT_VIEW_FILE_BY_FILE, } from '../constants'; export default { @@ -283,6 +286,8 @@ export default { notesEventHub.$once('fetchDiffData', this.fetchData); notesEventHub.$on('refetchDiffData', this.refetchDiffData); + eventHub.$on(EVT_VIEW_FILE_BY_FILE, this.fileByFileListener); + this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES; this.unwatchDiscussions = this.$watch( @@ -303,6 +308,7 @@ export default { beforeDestroy() { diffsApp.deinstrument(); + eventHub.$off(EVT_VIEW_FILE_BY_FILE, this.fileByFileListener); notesEventHub.$off('refetchDiffData', this.refetchDiffData); notesEventHub.$off('fetchDiffData', this.fetchData); @@ -323,7 +329,11 @@ export default { 'scrollToFile', 'setShowTreeList', 'navigateToDiffFileIndex', + 'setFileByFile', ]), + fileByFileListener({ setting } = {}) { + this.setFileByFile({ fileByFile: setting }); + }, navigateToDiffFileNumber(number) { this.navigateToDiffFileIndex(number - 1); }, diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index 8532119dc63855..07e27bd8e47324 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -103,6 +103,7 @@ export const RENAMED_DIFF_TRANSITIONS = { // MR Diffs known events export const EVT_EXPAND_ALL_FILES = 'mr:diffs:expandAllFiles'; +export const EVT_VIEW_FILE_BY_FILE = 'mr:diffs:preference:fileByFile'; export const EVT_PERF_MARK_FILE_TREE_START = 'mr:diffs:perf:fileTreeStart'; export const EVT_PERF_MARK_FILE_TREE_END = 'mr:diffs:perf:fileTreeEnd'; export const EVT_PERF_MARK_DIFF_FILES_START = 'mr:diffs:perf:filesStart'; diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 79bfd5ec63f0d9..416564b72c39d7 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -17,6 +17,10 @@ import axios from '~/lib/utils/axios_utils'; import * as urlUtils from '~/lib/utils/url_utility'; import diffsMockData from '../mock_data/merge_request_diffs'; +import { EVT_VIEW_FILE_BY_FILE } from '~/diffs/constants'; + +import eventHub from '~/diffs/event_hub'; + const mergeRequestDiff = { version_index: 1 }; const TEST_ENDPOINT = `${TEST_HOST}/diff/endpoint`; const COMMIT_URL = `${TEST_HOST}/COMMIT/OLD`; @@ -773,5 +777,24 @@ describe('diffs/components/app', () => { }, ); }); + + describe('control via event stream', () => { + it.each` + setting + ${true} + ${false} + `( + 'triggers the action with the new fileByFile setting - $setting - when the event with that setting is received', + async ({ setting }) => { + createComponent(); + await wrapper.vm.$nextTick(); + + eventHub.$emit(EVT_VIEW_FILE_BY_FILE, { setting }); + await wrapper.vm.$nextTick(); + + expect(store.state.diffs.viewDiffsFileByFile).toBe(setting); + }, + ); + }); }); }); -- GitLab From 1f2204890a74ee0f76f32f516faf7a54abbf78d3 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 1 Dec 2020 14:40:47 -0700 Subject: [PATCH 07/10] Add UI to toggle fileByFile setting --- .../diffs/components/settings_dropdown.vue | 34 ++++++- app/assets/javascripts/diffs/i18n.js | 4 + .../components/settings_dropdown_spec.js | 95 +++++++++++++++---- 3 files changed, 113 insertions(+), 20 deletions(-) diff --git a/app/assets/javascripts/diffs/components/settings_dropdown.vue b/app/assets/javascripts/diffs/components/settings_dropdown.vue index 590b2127e6bae6..b8904de8049287 100644 --- a/app/assets/javascripts/diffs/components/settings_dropdown.vue +++ b/app/assets/javascripts/diffs/components/settings_dropdown.vue @@ -1,16 +1,38 @@ @@ -84,5 +109,10 @@ export default { {{ __('Show whitespace changes') }} +
+ + {{ $options.i18n.fileByFile }} + +
diff --git a/app/assets/javascripts/diffs/i18n.js b/app/assets/javascripts/diffs/i18n.js index 4ec24d452bf4ad..c4ac99ead9152e 100644 --- a/app/assets/javascripts/diffs/i18n.js +++ b/app/assets/javascripts/diffs/i18n.js @@ -16,3 +16,7 @@ export const DIFF_FILE = { autoCollapsed: __('Files with large changes are collapsed by default.'), expand: __('Expand file'), }; + +export const SETTINGS_DROPDOWN = { + fileByFile: __('Show one file at a time'), +}; diff --git a/spec/frontend/diffs/components/settings_dropdown_spec.js b/spec/frontend/diffs/components/settings_dropdown_spec.js index 72330d8efba93c..a9b1c81d00e886 100644 --- a/spec/frontend/diffs/components/settings_dropdown_spec.js +++ b/spec/frontend/diffs/components/settings_dropdown_spec.js @@ -2,12 +2,18 @@ import { mount, createLocalVue } from '@vue/test-utils'; import Vuex from 'vuex'; import diffModule from '~/diffs/store/modules'; import SettingsDropdown from '~/diffs/components/settings_dropdown.vue'; -import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants'; +import { + EVT_VIEW_FILE_BY_FILE, + PARALLEL_DIFF_VIEW_TYPE, + INLINE_DIFF_VIEW_TYPE, +} from '~/diffs/constants'; +import eventHub from '~/diffs/event_hub'; const localVue = createLocalVue(); localVue.use(Vuex); describe('Diff settings dropdown component', () => { + let wrapper; let vm; let actions; @@ -25,10 +31,15 @@ describe('Diff settings dropdown component', () => { extendStore(store); - vm = mount(SettingsDropdown, { + wrapper = mount(SettingsDropdown, { localVue, store, }); + vm = wrapper.vm; + } + + function getFileByFileCheckbox(vueWrapper) { + return vueWrapper.find('[data-testid="file-by-file"]'); } beforeEach(() => { @@ -41,14 +52,14 @@ describe('Diff settings dropdown component', () => { }); afterEach(() => { - vm.destroy(); + wrapper.destroy(); }); describe('tree view buttons', () => { it('list view button dispatches setRenderTreeList with false', () => { createComponent(); - vm.find('.js-list-view').trigger('click'); + wrapper.find('.js-list-view').trigger('click'); expect(actions.setRenderTreeList).toHaveBeenCalledWith(expect.anything(), false); }); @@ -56,7 +67,7 @@ describe('Diff settings dropdown component', () => { it('tree view button dispatches setRenderTreeList with true', () => { createComponent(); - vm.find('.js-tree-view').trigger('click'); + wrapper.find('.js-tree-view').trigger('click'); expect(actions.setRenderTreeList).toHaveBeenCalledWith(expect.anything(), true); }); @@ -68,8 +79,8 @@ describe('Diff settings dropdown component', () => { }); }); - expect(vm.find('.js-list-view').classes('selected')).toBe(true); - expect(vm.find('.js-tree-view').classes('selected')).toBe(false); + expect(wrapper.find('.js-list-view').classes('selected')).toBe(true); + expect(wrapper.find('.js-tree-view').classes('selected')).toBe(false); }); it('sets tree button as selected when renderTreeList is true', () => { @@ -79,8 +90,8 @@ describe('Diff settings dropdown component', () => { }); }); - expect(vm.find('.js-list-view').classes('selected')).toBe(false); - expect(vm.find('.js-tree-view').classes('selected')).toBe(true); + expect(wrapper.find('.js-list-view').classes('selected')).toBe(false); + expect(wrapper.find('.js-tree-view').classes('selected')).toBe(true); }); }); @@ -92,8 +103,8 @@ describe('Diff settings dropdown component', () => { }); }); - expect(vm.find('.js-inline-diff-button').classes('selected')).toBe(true); - expect(vm.find('.js-parallel-diff-button').classes('selected')).toBe(false); + expect(wrapper.find('.js-inline-diff-button').classes('selected')).toBe(true); + expect(wrapper.find('.js-parallel-diff-button').classes('selected')).toBe(false); }); it('sets parallel button as selected', () => { @@ -103,14 +114,14 @@ describe('Diff settings dropdown component', () => { }); }); - expect(vm.find('.js-inline-diff-button').classes('selected')).toBe(false); - expect(vm.find('.js-parallel-diff-button').classes('selected')).toBe(true); + expect(wrapper.find('.js-inline-diff-button').classes('selected')).toBe(false); + expect(wrapper.find('.js-parallel-diff-button').classes('selected')).toBe(true); }); it('calls setInlineDiffViewType when clicking inline button', () => { createComponent(); - vm.find('.js-inline-diff-button').trigger('click'); + wrapper.find('.js-inline-diff-button').trigger('click'); expect(actions.setInlineDiffViewType).toHaveBeenCalled(); }); @@ -118,7 +129,7 @@ describe('Diff settings dropdown component', () => { it('calls setParallelDiffViewType when clicking parallel button', () => { createComponent(); - vm.find('.js-parallel-diff-button').trigger('click'); + wrapper.find('.js-parallel-diff-button').trigger('click'); expect(actions.setParallelDiffViewType).toHaveBeenCalled(); }); @@ -132,7 +143,7 @@ describe('Diff settings dropdown component', () => { }); }); - expect(vm.find('#show-whitespace').element.checked).toBe(false); + expect(wrapper.find('#show-whitespace').element.checked).toBe(false); }); it('sets as checked when showWhitespace is true', () => { @@ -142,13 +153,13 @@ describe('Diff settings dropdown component', () => { }); }); - expect(vm.find('#show-whitespace').element.checked).toBe(true); + expect(wrapper.find('#show-whitespace').element.checked).toBe(true); }); it('calls setShowWhitespace on change', () => { createComponent(); - const checkbox = vm.find('#show-whitespace'); + const checkbox = wrapper.find('#show-whitespace'); checkbox.element.checked = true; checkbox.trigger('change'); @@ -159,4 +170,52 @@ describe('Diff settings dropdown component', () => { }); }); }); + + describe('file-by-file toggle', () => { + beforeEach(() => { + jest.spyOn(eventHub, '$emit'); + }); + + it.each` + fileByFile | checked + ${true} | ${true} + ${false} | ${false} + `( + 'sets { checked: $checked } if the fileByFile setting is $fileByFile', + async ({ fileByFile, checked }) => { + createComponent(store => { + Object.assign(store.state.diffs, { + viewDiffsFileByFile: fileByFile, + }); + }); + + await vm.$nextTick(); + + expect(vm.checked).toBe(checked); + }, + ); + + it.each` + start | emit + ${true} | ${false} + ${false} | ${true} + `( + 'when the file by file setting starts as $start, toggling the checkbox should emit an event set to $emit', + async ({ start, emit }) => { + createComponent(store => { + Object.assign(store.state.diffs, { + viewDiffsFileByFile: start, + }); + }); + + await vm.$nextTick(); + + getFileByFileCheckbox(wrapper).trigger('click'); + + await vm.$nextTick(); + + expect(eventHub.$emit).toHaveBeenCalledWith(EVT_VIEW_FILE_BY_FILE, { setting: emit }); + }, + ); + }); }); -- GitLab From 1b7c5b89289456d9ad86d48be367eef574032ffe Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Sat, 14 Nov 2020 14:51:06 -0700 Subject: [PATCH 08/10] Add changelog for new MR settings dropdown feature --- changelogs/unreleased/feature-mr-file-by-file.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/feature-mr-file-by-file.yml diff --git a/changelogs/unreleased/feature-mr-file-by-file.yml b/changelogs/unreleased/feature-mr-file-by-file.yml new file mode 100644 index 00000000000000..9d0f17b90592c3 --- /dev/null +++ b/changelogs/unreleased/feature-mr-file-by-file.yml @@ -0,0 +1,5 @@ +--- +title: Toggle File-By-File setting from the MR settings dropdown +merge_request: 47726 +author: +type: added -- GitLab From 48fede6fd6ca0551a693a0023caf17561d113e11 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Sat, 14 Nov 2020 15:25:37 -0700 Subject: [PATCH 09/10] Update translations with new text --- locale/gitlab.pot | 3 +++ 1 file changed, 3 insertions(+) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index b089c3e8e0a1c3..0e469715559c89 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -25056,6 +25056,9 @@ msgstr "" msgid "Show me the basics" msgstr "" +msgid "Show one file at a time" +msgstr "" + msgid "Show only direct members" msgstr "" -- GitLab From 143c0158ad6bc8666359d76d121ee7c58ccde295 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 18 Nov 2020 20:41:05 -0700 Subject: [PATCH 10/10] Add documentation for new MR dropdown toggle --- .../reviewing_and_managing_merge_requests.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md b/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md index 4968945b3c7c3d..1dbee52f6db8b9 100644 --- a/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md +++ b/doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md @@ -92,6 +92,17 @@ From there, when reviewing merge requests' **Changes** tab, you will see only on ![File-by-file diff navigation](img/file_by_file_v13_2.png) +From [GitLab 13.7](https://gitlab.com/gitlab-org/gitlab/-/issues/233898) onwards, if you want to change +this behavior, you can do so from your **User preferences** (as explained above) or directly in a +merge request: + +1. Go to the merge request's **Changes** tab. +1. Click the cog icon (**{settings}**) to reveal the merge request's settings dropdown. +1. Select or unselect the checkbox **Show one file at a time** to change the setting accordingly. + +This change overrides the choice you made in your user preferences and persists until you clear your +browser's cookies or change this behavior again. + #### Enable or disable file-by-file diff navigation **(CORE ONLY)** File-by-file diff navigation is under development but ready for production use. It is -- GitLab