Merge branch 'jdb/sync-showwhitespace-changes' into 'master'

Use user preferences as SSOT for diffs whitespace

See merge request gitlab-org/gitlab!63927

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

@@ -36,7 +36,7 @@ export default {
       this.setFileByFile({ fileByFile: !this.viewDiffsFileByFile });
     },
     toggleWhitespace(updatedSetting) {
-      this.setShowWhitespace({ showWhitespace: updatedSetting, pushState: true });
+      this.setShowWhitespace({ showWhitespace: updatedSetting });
     },
   },
 };

app/assets/javascripts/diffs/constants.js

-// The backend actually uses "hide_whitespace" while the frontend
-// uses "show whitspace" so these values are opposite what you might expect
-export const NO_SHOW_WHITESPACE = '1';
-export const SHOW_WHITESPACE = '0';
 export const INLINE_DIFF_VIEW_TYPE = 'inline';
 export const PARALLEL_DIFF_VIEW_TYPE = 'parallel';
 export const MATCH_LINE_TYPE = 'match';

app/assets/javascripts/diffs/index.js

@@ -98,10 +98,23 @@ export default function initDiffsApp(store) {
       this.setRenderTreeList(renderTreeList);
-      // Set whitespace default as per user preferences unless cookie is already set
-      if (!Cookies.get(DIFF_WHITESPACE_COOKIE_NAME)) {
-        const hideWhitespace = this.showWhitespaceDefault ? '0' : '1';
-        this.setShowWhitespace({ showWhitespace: hideWhitespace !== '1' });
+      // NOTE: A "true" or "checked" value for `showWhitespace` is '0' not '1'.
+      // Check for cookie and save that setting for future use.
+      // Then delete the cookie as we are phasing it out and using the database as SSOT.
+      // NOTE: This can/should be removed later
+      if (Cookies.get(DIFF_WHITESPACE_COOKIE_NAME)) {
+        const hideWhitespace = Cookies.get(DIFF_WHITESPACE_COOKIE_NAME);
+        this.setShowWhitespace({
+          url: this.endpointUpdateUser,
+          showWhitespace: hideWhitespace !== '1',
+        });
+        Cookies.remove(DIFF_WHITESPACE_COOKIE_NAME);
+      } else {
+        // This is only to set the the user preference in Vuex for use later
+        this.setShowWhitespace({
+          showWhitespace: this.showWhitespaceDefault,
+          updateDatabase: false,
+        });
       }
     },
     methods: {

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

@@ -26,9 +26,6 @@ import {
   START_RENDERING_INDEX,
   INLINE_DIFF_LINES_KEY,
   DIFFS_PER_PAGE,
-  DIFF_WHITESPACE_COOKIE_NAME,
-  SHOW_WHITESPACE,
-  NO_SHOW_WHITESPACE,
   DIFF_FILE_MANUAL_COLLAPSE,
   DIFF_FILE_AUTOMATIC_COLLAPSE,
   EVT_PERF_MARK_FILE_TREE_START,
@@ -569,16 +566,15 @@ export const setRenderTreeList = ({ commit }, renderTreeList) => {
   }
 };
-export const setShowWhitespace = ({ commit }, { showWhitespace, pushState = false }) => {
-  commit(types.SET_SHOW_WHITESPACE, showWhitespace);
-  const w = showWhitespace ? SHOW_WHITESPACE : NO_SHOW_WHITESPACE;
-
-  Cookies.set(DIFF_WHITESPACE_COOKIE_NAME, w);
-
-  if (pushState) {
-    historyPushState(mergeUrlParams({ w }, window.location.href));
+export const setShowWhitespace = async (
+  { state, commit },
+  { url, showWhitespace, updateDatabase = true },
+) => {
+  if (updateDatabase) {
+    await axios.put(url || state.endpointUpdateUser, { show_whitespace_in_diffs: showWhitespace });
   }
+  commit(types.SET_SHOW_WHITESPACE, showWhitespace);
   notesEventHub.$emit('refetchDiffData');
   if (window.gon?.features?.diffSettingsUsageData) {

app/assets/javascripts/diffs/store/modules/diff_state.js

 import Cookies from 'js-cookie';
 import { getParameterValues } from '~/lib/utils/url_utility';
-import {
-  INLINE_DIFF_VIEW_TYPE,
-  DIFF_VIEW_COOKIE_NAME,
-  DIFF_WHITESPACE_COOKIE_NAME,
-} from '../../constants';
+import { INLINE_DIFF_VIEW_TYPE, DIFF_VIEW_COOKIE_NAME } from '../../constants';
 import { fileByFile } from '../../utils/preferences';
-import { getDefaultWhitespace } from '../utils';
 const getViewTypeFromQueryString = () => getParameterValues('view')[0];
 const viewTypeFromCookie = Cookies.get(DIFF_VIEW_COOKIE_NAME);
 const defaultViewType = INLINE_DIFF_VIEW_TYPE;
-const whiteSpaceFromQueryString = getParameterValues('w')[0];
-const whiteSpaceFromCookie = Cookies.get(DIFF_WHITESPACE_COOKIE_NAME);
 export default () => ({
   isLoading: true,
@@ -42,7 +35,7 @@ export default () => ({
   commentForms: [],
   highlightedRow: null,
   renderTreeList: true,
-  showWhitespace: getDefaultWhitespace(whiteSpaceFromQueryString, whiteSpaceFromCookie),
+  showWhitespace: true,
   viewDiffsFileByFile: fileByFile(),
   fileFinderVisible: false,
   dismissEndpoint: '',

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

@@ -11,8 +11,6 @@ import {
   MATCH_LINE_TYPE,
   LINES_TO_BE_RENDERED_DIRECTLY,
   INLINE_DIFF_LINES_KEY,
-  SHOW_WHITESPACE,
-  NO_SHOW_WHITESPACE,
   CONFLICT_OUR,
   CONFLICT_THEIR,
   CONFLICT_MARKER,
@@ -559,10 +557,3 @@ export const allDiscussionWrappersExpanded = (diff) => {
   return discussionsExpanded;
 };
-
-export const getDefaultWhitespace = (queryString, cookie) => {
-  // Querystring should override stored cookie value
-  if (queryString) return queryString === SHOW_WHITESPACE;
-  if (cookie === NO_SHOW_WHITESPACE) return false;
-  return true;
-};

doc/user/project/merge_requests/changes.md

@@ -91,10 +91,6 @@ specific commit page.
 ![MR diff](img/merge_request_diff.png)
-NOTE:
-You can append `?w=1` while on the diffs page of a merge request to ignore any
-whitespace changes.
-
 ## Mark files as viewed
 > - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/51513) in GitLab 13.9.

spec/frontend/diffs/components/settings_dropdown_spec.js

@@ -142,7 +142,6 @@ describe('Diff settings dropdown component', () => {
       expect(store.dispatch).toHaveBeenCalledWith('diffs/setShowWhitespace', {
         showWhitespace: !checked,
-        pushState: true,
       });
     });
   });

spec/frontend/diffs/store/actions_spec.js

@@ -9,8 +9,6 @@ import {
   INLINE_DIFF_VIEW_TYPE,
   PARALLEL_DIFF_VIEW_TYPE,
   DIFFS_PER_PAGE,
-  DIFF_WHITESPACE_COOKIE_NAME,
-  SHOW_WHITESPACE,
 } from '~/diffs/constants';
 import {
   setBaseConfig,
@@ -1019,14 +1017,26 @@ describe('DiffsStoreActions', () => {
   });
   describe('setShowWhitespace', () => {
+    const endpointUpdateUser = 'user/prefs';
+    let putSpy;
+    let mock;
+
     beforeEach(() => {
+      mock = new MockAdapter(axios);
+      putSpy = jest.spyOn(axios, 'put');
+
+      mock.onPut(endpointUpdateUser).reply(200, {});
       jest.spyOn(eventHub, '$emit').mockImplementation();
     });
+    afterEach(() => {
+      mock.restore();
+    });
+
     it('commits SET_SHOW_WHITESPACE', (done) => {
       testAction(
         setShowWhitespace,
-        { showWhitespace: true },
+        { showWhitespace: true, updateDatabase: false },
         {},
         [{ type: types.SET_SHOW_WHITESPACE, payload: true }],
         [],
@@ -1034,32 +1044,20 @@ describe('DiffsStoreActions', () => {
       );
     });
-    it('sets cookie', () => {
-      setShowWhitespace({ commit() {} }, { showWhitespace: true });
-
-      expect(Cookies.get(DIFF_WHITESPACE_COOKIE_NAME)).toEqual(SHOW_WHITESPACE);
-    });
-
-    it('calls history pushState', () => {
-      setShowWhitespace({ commit() {} }, { showWhitespace: true, pushState: true });
-
-      expect(window.history.pushState).toHaveBeenCalled();
-    });
-
-    it('calls history pushState with merged params', () => {
-      window.history.pushState({}, '', '?test=1');
-
-      setShowWhitespace({ commit() {} }, { showWhitespace: true, pushState: true });
-
-      expect(
-        window.history.pushState.mock.calls[window.history.pushState.mock.calls.length - 1][2],
-      ).toMatch(/(.*)\?test=1&w=0/);
-      window.history.pushState({}, '', '?');
+    it('saves to the database', async () => {
+      await setShowWhitespace(
+        { state: { endpointUpdateUser }, commit() {} },
+        { showWhitespace: true, updateDatabase: true },
+      );
+      expect(putSpy).toHaveBeenCalledWith(endpointUpdateUser, { show_whitespace_in_diffs: true });
     });
-    it('emits eventHub event', () => {
-      setShowWhitespace({ commit() {} }, { showWhitespace: true, pushState: true });
+    it('emits eventHub event', async () => {
+      await setShowWhitespace(
+        { state: {}, commit() {} },
+        { showWhitespace: true, updateDatabase: false },
+      );
       expect(eventHub.$emit).toHaveBeenCalledWith('refetchDiffData');
     });

spec/frontend/diffs/store/utils_spec.js

@@ -752,28 +752,6 @@ describe('DiffsStoreUtils', () => {
     });
   });
-  describe('getDefaultWhitespace', () => {
-    it('defaults to true if querystring and cookie are undefined', () => {
-      expect(utils.getDefaultWhitespace()).toBe(true);
-    });
-
-    it('returns false if querystring is `1`', () => {
-      expect(utils.getDefaultWhitespace('1', '0')).toBe(false);
-    });
-
-    it('returns true if querystring is `0`', () => {
-      expect(utils.getDefaultWhitespace('0', undefined)).toBe(true);
-    });
-
-    it('returns false if cookie is `1`', () => {
-      expect(utils.getDefaultWhitespace(undefined, '1')).toBe(false);
-    });
-
-    it('returns true if cookie is `0`', () => {
-      expect(utils.getDefaultWhitespace(undefined, '0')).toBe(true);
-    });
-  });
-
   describe('isAdded', () => {
     it.each`
       type               | expected