Merge branch 'jdb/save-whitespace-setting' into 'master'

Save show whitespace choice

See merge request gitlab-org/gitlab!35806

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';
@@ -20,6 +24,7 @@ export const LINE_SIDE_LEFT = 'left-side';
 export const LINE_SIDE_RIGHT = 'right-side';
 
 export const DIFF_VIEW_COOKIE_NAME = 'diff_view';
+export const DIFF_WHITESPACE_COOKIE_NAME = 'diff_whitespace';
 export const LINE_HOVER_CLASS_NAME = 'is-over';
 export const LINE_UNFOLD_CLASS_NAME = 'unfold js-unfold';
 export const CONTEXT_LINE_CLASS_NAME = 'diff-expanded';
@@ -35,7 +40,6 @@ export const MR_TREE_SHOW_KEY = 'mr_tree_show';
 
 export const TREE_TYPE = 'tree';
 export const TREE_LIST_STORAGE_KEY = 'mr_diff_tree_list';
-export const WHITESPACE_STORAGE_KEY = 'mr_show_whitespace';
 export const TREE_LIST_WIDTH_STORAGE_KEY = 'mr_tree_list_width';
 
 export const INITIAL_TREE_WIDTH = 320;

app/assets/javascripts/diffs/index.js

 import Vue from 'vue';
 import { mapActions, mapState, mapGetters } from 'vuex';
 import { parseBoolean } from '~/lib/utils/common_utils';
-import { getParameterValues } from '~/lib/utils/url_utility';
 import FindFile from '~/vue_shared/components/file_finder/index.vue';
 import eventHub from '../notes/event_hub';
 import diffsApp from './components/app.vue';
-import { TREE_LIST_STORAGE_KEY } from './constants';
+import { TREE_LIST_STORAGE_KEY, DIFF_WHITESPACE_COOKIE_NAME } from './constants';
+import Cookies from 'js-cookie';
 
 export default function initDiffsApp(store) {
   const fileFinderEl = document.getElementById('js-diff-file-finder');
@@ -86,15 +86,16 @@ export default function initDiffsApp(store) {
       }),
     },
     created() {
-      let hideWhitespace = getParameterValues('w')[0];
       const treeListStored = localStorage.getItem(TREE_LIST_STORAGE_KEY);
       const renderTreeList = treeListStored !== null ? parseBoolean(treeListStored) : true;
 
       this.setRenderTreeList(renderTreeList);
-      if (!hideWhitespace) {
-        hideWhitespace = this.showWhitespaceDefault ? '0' : '1';
+
+      // 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' });
       }
-      this.setShowWhitespace({ showWhitespace: hideWhitespace !== '1' });
     },
     methods: {
       ...mapActions('diffs', ['setRenderTreeList', 'setShowWhitespace']),
@@ -114,7 +115,6 @@ export default function initDiffsApp(store) {
           isFluidLayout: this.isFluidLayout,
           dismissEndpoint: this.dismissEndpoint,
           showSuggestPopover: this.showSuggestPopover,
-          showWhitespaceDefault: this.showWhitespaceDefault,
         },
       });
     },

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

@@ -25,7 +25,6 @@ import {
   DIFF_VIEW_COOKIE_NAME,
   MR_TREE_SHOW_KEY,
   TREE_LIST_STORAGE_KEY,
-  WHITESPACE_STORAGE_KEY,
   TREE_LIST_WIDTH_STORAGE_KEY,
   OLD_LINE_KEY,
   NEW_LINE_KEY,
@@ -38,6 +37,9 @@ import {
   INLINE_DIFF_LINES_KEY,
   PARALLEL_DIFF_LINES_KEY,
   DIFFS_PER_PAGE,
+  DIFF_WHITESPACE_COOKIE_NAME,
+  SHOW_WHITESPACE,
+  NO_SHOW_WHITESPACE,
 } from '../constants';
 import { diffViewerModes } from '~/ide/constants';
 
@@ -484,11 +486,12 @@ 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;
 
-  localStorage.setItem(WHITESPACE_STORAGE_KEY, showWhitespace);
+  Cookies.set(DIFF_WHITESPACE_COOKIE_NAME, w);
 
   if (pushState) {
-    historyPushState(mergeUrlParams({ w: showWhitespace ? '0' : '1' }, window.location.href));
+    historyPushState(mergeUrlParams({ w }, window.location.href));
   }
 
   eventHub.$emit('refetchDiffData');

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 } from '../../constants';
+import {
+  INLINE_DIFF_VIEW_TYPE,
+  DIFF_VIEW_COOKIE_NAME,
+  DIFF_WHITESPACE_COOKIE_NAME,
+} from '../../constants';
+import { getDefaultWhitespace } from '../utils';
 
 const viewTypeFromQueryString = 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,
@@ -29,7 +36,7 @@ export default () => ({
   commentForms: [],
   highlightedRow: null,
   renderTreeList: true,
-  showWhitespace: true,
+  showWhitespace: getDefaultWhitespace(whiteSpaceFromQueryString, whiteSpaceFromCookie),
   fileFinderVisible: false,
   dismissEndpoint: '',
   showSuggestPopover: true,

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

@@ -15,6 +15,8 @@ import {
   TREE_TYPE,
   INLINE_DIFF_VIEW_TYPE,
   PARALLEL_DIFF_VIEW_TYPE,
+  SHOW_WHITESPACE,
+  NO_SHOW_WHITESPACE,
 } from '../constants';
 
 export function findDiffFile(files, match, matchKey = 'file_hash') {
@@ -701,3 +703,10 @@ 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;
+};

changelogs/unreleased/jdb-save-whitespace-setting.yml

+---
+title: Save show whitespace changes
+merge_request: 35806
+author:
+type: fixed

spec/features/merge_request/user_toggles_whitespace_changes_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe 'Merge request > User toggles whitespace changes', :js do
 
       find('.js-show-diff-settings').click
 
-      expect(find('#show-whitespace')).to be_checked
+      expect(find('#show-whitespace')).not_to be_checked
     end
   end
 end

spec/frontend/diffs/store/actions_spec.js

@@ -6,6 +6,8 @@ import {
   INLINE_DIFF_VIEW_TYPE,
   PARALLEL_DIFF_VIEW_TYPE,
   DIFFS_PER_PAGE,
+  DIFF_WHITESPACE_COOKIE_NAME,
+  SHOW_WHITESPACE,
 } from '~/diffs/constants';
 import {
   setBaseConfig,
@@ -1187,10 +1189,10 @@ describe('DiffsStoreActions', () => {
       );
     });
 
-    it('sets localStorage', () => {
+    it('sets cookie', () => {
       setShowWhitespace({ commit() {} }, { showWhitespace: true });
 
-      expect(localStorage.setItem).toHaveBeenCalledWith('mr_show_whitespace', true);
+      expect(Cookies.get(DIFF_WHITESPACE_COOKIE_NAME)).toEqual(SHOW_WHITESPACE);
     });
 
     it('calls history pushState', () => {

spec/frontend/diffs/store/utils_spec.js

@@ -1090,4 +1090,26 @@ 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);
+    });
+  });
 });