Merge branch 'ph/292515/fileByFileStopUpdatingURL' into 'master'

Toggling file-by-file no longer updates the URL

See merge request gitlab-org/gitlab!54796

app/assets/javascripts/diffs/components/app.vue

@@ -23,9 +23,7 @@ import {
   ALERT_OVERFLOW_HIDDEN,
   ALERT_MERGE_CONFLICT,
   ALERT_COLLAPSED_FILES,
-  EVT_VIEW_FILE_BY_FILE,
 } from '../constants';
-import eventHub from '../event_hub';
 
 import { reviewStatuses } from '../utils/file_reviews';
 import { diffsApp } from '../utils/performance';
@@ -332,16 +330,11 @@ export default {
     subscribeToEvents() {
       notesEventHub.$once('fetchDiffData', this.fetchData);
       notesEventHub.$on('refetchDiffData', this.refetchDiffData);
-      eventHub.$on(EVT_VIEW_FILE_BY_FILE, this.fileByFileListener);
     },
     unsubscribeFromEvents() {
-      eventHub.$off(EVT_VIEW_FILE_BY_FILE, this.fileByFileListener);
       notesEventHub.$off('refetchDiffData', this.refetchDiffData);
       notesEventHub.$off('fetchDiffData', this.fetchData);
     },
-    fileByFileListener({ setting } = {}) {
-      this.setFileByFile({ fileByFile: setting });
-    },
     navigateToDiffFileNumber(number) {
       this.navigateToDiffFileIndex(number - 1);
     },

app/assets/javascripts/diffs/components/settings_dropdown.vue

 <script>
 import { GlButtonGroup, GlButton, GlDropdown, GlFormCheckbox } from '@gitlab/ui';
 import { mapActions, mapGetters, mapState } from 'vuex';
-
-import { EVT_VIEW_FILE_BY_FILE } from '../constants';
-import eventHub from '../event_hub';
 import { SETTINGS_DROPDOWN } from '../i18n';
 
 export default {
@@ -24,9 +21,10 @@ export default {
       'setParallelDiffViewType',
       'setRenderTreeList',
       'setShowWhitespace',
+      'setFileByFile',
     ]),
     toggleFileByFile() {
-      eventHub.$emit(EVT_VIEW_FILE_BY_FILE, { setting: !this.viewDiffsFileByFile });
+      this.setFileByFile({ fileByFile: !this.viewDiffsFileByFile });
     },
   },
 };

app/assets/javascripts/diffs/constants.js

@@ -103,7 +103,6 @@ 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';

app/assets/javascripts/diffs/store/actions.js

@@ -741,12 +741,7 @@ export const navigateToDiffFileIndex = ({ commit, state }, index) => {
 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),
-  );
 };
 
 export function reviewFile({ commit, state }, { file, reviewed = true }) {

app/assets/javascripts/diffs/utils/preferences.js

 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 cookie === DIFF_VIEW_FILE_BY_FILE;
   }
 
-  return viewFileByFile;
+  return pref;
 }

spec/frontend/diffs/components/app_spec.js

@@ -14,9 +14,6 @@ import HiddenFilesWarning from '~/diffs/components/hidden_files_warning.vue';
 import NoChanges from '~/diffs/components/no_changes.vue';
 import TreeList from '~/diffs/components/tree_list.vue';
 
-import { EVT_VIEW_FILE_BY_FILE } from '~/diffs/constants';
-
-import eventHub from '~/diffs/event_hub';
 import axios from '~/lib/utils/axios_utils';
 import * as urlUtils from '~/lib/utils/url_utility';
 import createDiffsStore from '../create_diffs_store';
@@ -699,24 +696,5 @@ 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 nextTick();
-
-          eventHub.$emit(EVT_VIEW_FILE_BY_FILE, { setting });
-          await nextTick();
-
-          expect(store.state.diffs.viewDiffsFileByFile).toBe(setting);
-        },
-      );
-    });
   });
 });

spec/frontend/diffs/components/settings_dropdown_spec.js

 import { mount, createLocalVue } from '@vue/test-utils';
 import Vuex from 'vuex';
 import SettingsDropdown from '~/diffs/components/settings_dropdown.vue';
-import {
-  EVT_VIEW_FILE_BY_FILE,
-  PARALLEL_DIFF_VIEW_TYPE,
-  INLINE_DIFF_VIEW_TYPE,
-} from '~/diffs/constants';
+import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants';
 import eventHub from '~/diffs/event_hub';
 import diffModule from '~/diffs/store/modules';
 
@@ -48,6 +44,7 @@ describe('Diff settings dropdown component', () => {
       setParallelDiffViewType: jest.fn(),
       setRenderTreeList: jest.fn(),
       setShowWhitespace: jest.fn(),
+      setFileByFile: jest.fn(),
     };
   });
 
@@ -196,12 +193,12 @@ describe('Diff settings dropdown component', () => {
     );
 
     it.each`
-      start    | emit
+      start    | setting
       ${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 }) => {
+      'when the file by file setting starts as $start, toggling the checkbox should call setFileByFile with $setting',
+      async ({ start, setting }) => {
         createComponent((store) => {
           Object.assign(store.state.diffs, {
             viewDiffsFileByFile: start,
@@ -214,7 +211,9 @@ describe('Diff settings dropdown component', () => {
 
         await vm.$nextTick();
 
-        expect(eventHub.$emit).toHaveBeenCalledWith(EVT_VIEW_FILE_BY_FILE, { setting: emit });
+        expect(actions.setFileByFile).toHaveBeenLastCalledWith(expect.anything(), {
+          fileByFile: setting,
+        });
       },
     );
   });

spec/frontend/diffs/utils/preferences_spec.js

@@ -5,32 +5,25 @@ import {
   DIFF_VIEW_ALL_FILES,
 } from '~/diffs/constants';
 import { fileByFile } from '~/diffs/utils/preferences';
-import { getParameterValues } from '~/lib/utils/url_utility';
-
-jest.mock('~/lib/utils/url_utility');
 
 describe('diffs preferences', () => {
   describe('fileByFile', () => {
+    afterEach(() => {
+      Cookies.remove(DIFF_FILE_BY_FILE_COOKIE_NAME);
+    });
+
     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]}
+      result   | preference | cookie
+      ${true}  | ${false}   | ${DIFF_VIEW_FILE_BY_FILE}
+      ${false} | ${true}    | ${DIFF_VIEW_ALL_FILES}
+      ${true}  | ${false}   | ${DIFF_VIEW_FILE_BY_FILE}
+      ${false} | ${true}    | ${DIFF_VIEW_ALL_FILES}
+      ${false} | ${false}   | ${DIFF_VIEW_ALL_FILES}
+      ${true}  | ${true}    | ${DIFF_VIEW_FILE_BY_FILE}
     `(
-      '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);
+      'should return $result when { preference: $preference, cookie: $cookie }',
+      ({ result, preference, cookie }) => {
+        Cookies.set(DIFF_FILE_BY_FILE_COOKIE_NAME, cookie);
 
         expect(fileByFile(preference)).toBe(result);
       },