Merge branch 'tor/feature/file-tree-viewed-check' into 'master'

Toggle MR diffs File Tree "viewed" status (bold) based on file header Viewed checkbox

See merge request gitlab-org/gitlab!73688

app/assets/javascripts/batch_comments/components/preview_dropdown.vue

@@ -15,14 +15,14 @@ export default {
     ...mapGetters('batchComments', ['draftsCount', 'sortedDrafts']),
   },
   methods: {
-    ...mapActions('diffs', ['toggleActiveFileByHash']),
+    ...mapActions('diffs', ['setCurrentFileHash']),
     ...mapActions('batchComments', ['scrollToDraft']),
     isLast(index) {
       return index === this.sortedDrafts.length - 1;
     },
     async onClickDraft(draft) {
       if (this.viewDiffsFileByFile && draft.file_hash) {
-        await this.toggleActiveFileByHash(draft.file_hash);
+        await this.setCurrentFileHash(draft.file_hash);
       }
 
       await this.scrollToDraft(draft);

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

@@ -221,7 +221,7 @@ export default {
       'toggleFileDiscussions',
       'toggleFileDiscussionWrappers',
       'toggleFullDiff',
-      'toggleActiveFileByHash',
+      'setCurrentFileHash',
       'reviewFile',
       'setFileCollapsedByUser',
     ]),
@@ -244,7 +244,7 @@ export default {
         scrollToElement(document.querySelector(selector));
         window.location.hash = selector;
         if (!this.viewDiffsFileByFile) {
-          this.toggleActiveFileByHash(this.diffFile.file_hash);
+          this.setCurrentFileHash(this.diffFile.file_hash);
         }
       }
     },

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

@@ -85,6 +85,12 @@ export const setBaseConfig = ({ commit }, options) => {
     viewDiffsFileByFile,
     mrReviews,
   });
+
+  Array.from(new Set(Object.values(mrReviews).flat())).forEach((id) => {
+    const viewedId = id.replace(/^hash:/, '');
+
+    commit(types.SET_DIFF_FILE_VIEWED, { id: viewedId, seen: true });
+  });
 };
 
 export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
@@ -127,7 +133,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
         }
 
         if (!isNoteLink && !state.currentDiffFileId) {
-          commit(types.VIEW_DIFF_FILE, diff_files[0]?.file_hash);
+          commit(types.SET_CURRENT_DIFF_FILE, diff_files[0]?.file_hash);
         }
 
         if (isNoteLink) {
@@ -143,7 +149,7 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
             !state.diffFiles.some((f) => f.file_hash === state.currentDiffFileId) &&
             !isNoteLink
           ) {
-            commit(types.VIEW_DIFF_FILE, state.diffFiles[0].file_hash);
+            commit(types.SET_CURRENT_DIFF_FILE, state.diffFiles[0].file_hash);
           }
 
           if (state.diffFiles?.length) {
@@ -248,7 +254,7 @@ export const fetchCoverageFiles = ({ commit, state }) => {
 export const setHighlightedRow = ({ commit }, lineCode) => {
   const fileHash = lineCode.split('_')[0];
   commit(types.SET_HIGHLIGHTED_ROW, lineCode);
-  commit(types.VIEW_DIFF_FILE, fileHash);
+  commit(types.SET_CURRENT_DIFF_FILE, fileHash);
 
   handleLocationHash();
 };
@@ -514,8 +520,8 @@ export const toggleTreeOpen = ({ commit }, path) => {
   commit(types.TOGGLE_FOLDER_OPEN, path);
 };
 
-export const toggleActiveFileByHash = ({ commit }, hash) => {
-  commit(types.VIEW_DIFF_FILE, hash);
+export const setCurrentFileHash = ({ commit }, hash) => {
+  commit(types.SET_CURRENT_DIFF_FILE, hash);
 };
 
 export const scrollToFile = ({ state, commit, getters }, { path, setHash = true }) => {
@@ -523,7 +529,7 @@ export const scrollToFile = ({ state, commit, getters }, { path, setHash = true
 
   const { fileHash } = state.treeEntries[path];
 
-  commit(types.VIEW_DIFF_FILE, fileHash);
+  commit(types.SET_CURRENT_DIFF_FILE, fileHash);
 
   if (getters.isVirtualScrollingEnabled) {
     eventHub.$emit('scrollToFileHash', fileHash);
@@ -806,7 +812,7 @@ export const setCurrentDiffFileIdFromNote = ({ commit, state, rootGetters }, not
   const fileHash = rootGetters.getDiscussion(note.discussion_id).diff_file?.file_hash;
 
   if (fileHash && state.diffFiles.some((f) => f.file_hash === fileHash)) {
-    commit(types.VIEW_DIFF_FILE, fileHash);
+    commit(types.SET_CURRENT_DIFF_FILE, fileHash);
   }
 };
 
@@ -814,7 +820,7 @@ export const navigateToDiffFileIndex = ({ commit, state }, index) => {
   const fileHash = state.diffFiles[index].file_hash;
   document.location.hash = fileHash;
 
-  commit(types.VIEW_DIFF_FILE, fileHash);
+  commit(types.SET_CURRENT_DIFF_FILE, fileHash);
 };
 
 export const setFileByFile = ({ state, commit }, { fileByFile }) => {
@@ -850,6 +856,8 @@ export function reviewFile({ commit, state }, { file, reviewed = true }) {
   const reviews = markFileReview(state.mrReviews, file, reviewed);
 
   setReviewsForMergeRequest(mrPath, reviews);
+
+  commit(types.SET_DIFF_FILE_VIEWED, { id: file.file_hash, seen: reviewed });
   commit(types.SET_MR_FILE_REVIEWS, reviews);
 }
 

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

@@ -20,7 +20,8 @@ export const SET_LINE_DISCUSSIONS_FOR_FILE = 'SET_LINE_DISCUSSIONS_FOR_FILE';
 export const REMOVE_LINE_DISCUSSIONS_FOR_FILE = 'REMOVE_LINE_DISCUSSIONS_FOR_FILE';
 export const TOGGLE_FOLDER_OPEN = 'TOGGLE_FOLDER_OPEN';
 export const SET_SHOW_TREE_LIST = 'SET_SHOW_TREE_LIST';
-export const VIEW_DIFF_FILE = 'VIEW_DIFF_FILE';
+export const SET_CURRENT_DIFF_FILE = 'SET_CURRENT_DIFF_FILE';
+export const SET_DIFF_FILE_VIEWED = 'SET_DIFF_FILE_VIEWED';
 
 export const OPEN_DIFF_FILE_COMMENT_FORM = 'OPEN_DIFF_FILE_COMMENT_FORM';
 export const UPDATE_DIFF_FILE_COMMENT_FORM = 'UPDATE_DIFF_FILE_COMMENT_FORM';

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

@@ -254,9 +254,11 @@ export default {
   [types.SET_SHOW_TREE_LIST](state, showTreeList) {
     state.showTreeList = showTreeList;
   },
-  [types.VIEW_DIFF_FILE](state, fileId) {
+  [types.SET_CURRENT_DIFF_FILE](state, fileId) {
     state.currentDiffFileId = fileId;
-    Vue.set(state.viewedDiffFileIds, fileId, true);
+  },
+  [types.SET_DIFF_FILE_VIEWED](state, { id, seen }) {
+    Vue.set(state.viewedDiffFileIds, id, seen);
   },
   [types.OPEN_DIFF_FILE_COMMENT_FORM](state, formData) {
     state.commentForms.push({

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

@@ -52,8 +52,10 @@ export function markFileReview(reviews, file, reviewed = true) {
 
     if (reviewed) {
       fileReviews.add(file.id);
+      fileReviews.add(`hash:${file.file_hash}`);
     } else {
       fileReviews.delete(file.id);
+      fileReviews.delete(`hash:${file.file_hash}`);
     }
 
     updatedReviews[file.file_identifier_hash] = Array.from(fileReviews);

spec/frontend/batch_comments/components/preview_dropdown_spec.js

@@ -7,7 +7,7 @@ Vue.use(Vuex);
 
 let wrapper;
 
-const toggleActiveFileByHash = jest.fn();
+const setCurrentFileHash = jest.fn();
 const scrollToDraft = jest.fn();
 
 function factory({ viewDiffsFileByFile = false, draftsCount = 1, sortedDrafts = [] } = {}) {
@@ -16,7 +16,7 @@ function factory({ viewDiffsFileByFile = false, draftsCount = 1, sortedDrafts =
       diffs: {
         namespaced: true,
         actions: {
-          toggleActiveFileByHash,
+          setCurrentFileHash,
         },
         state: {
           viewDiffsFileByFile,
@@ -51,7 +51,7 @@ describe('Batch comments preview dropdown', () => {
 
       await Vue.nextTick();
 
-      expect(toggleActiveFileByHash).toHaveBeenCalledWith(expect.anything(), 'hash');
+      expect(setCurrentFileHash).toHaveBeenCalledWith(expect.anything(), 'hash');
       expect(scrollToDraft).toHaveBeenCalledWith(expect.anything(), { id: 1, file_hash: 'hash' });
     });
 

spec/frontend/diffs/components/diff_file_header_spec.js

@@ -7,7 +7,7 @@ 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_MR_FILE_REVIEWS } from '~/diffs/store/mutation_types';
+import { SET_DIFF_FILE_VIEWED, SET_MR_FILE_REVIEWS } from '~/diffs/store/mutation_types';
 import { diffViewerModes } from '~/ide/constants';
 import { scrollToElement } from '~/lib/utils/common_utils';
 import { truncateSha } from '~/lib/utils/text_utility';
@@ -23,6 +23,7 @@ jest.mock('~/lib/utils/common_utils');
 const diffFile = Object.freeze(
   Object.assign(diffDiscussionsMockData.diff_file, {
     id: '123',
+    file_hash: 'xyz',
     file_identifier_hash: 'abc',
     edit_path: 'link:/to/edit/path',
     blob: {
@@ -58,7 +59,7 @@ describe('DiffFileHeader component', () => {
           toggleFileDiscussions: jest.fn(),
           toggleFileDiscussionWrappers: jest.fn(),
           toggleFullDiff: jest.fn(),
-          toggleActiveFileByHash: jest.fn(),
+          setCurrentFileHash: jest.fn(),
           setFileCollapsedByUser: jest.fn(),
           reviewFile: jest.fn(),
         },
@@ -553,7 +554,13 @@ describe('DiffFileHeader component', () => {
         reviewFile,
         { file, reviewed: true },
         {},
-        [{ type: SET_MR_FILE_REVIEWS, payload: { [file.file_identifier_hash]: [file.id] } }],
+        [
+          { type: SET_DIFF_FILE_VIEWED, payload: { id: file.file_hash, seen: true } },
+          {
+            type: SET_MR_FILE_REVIEWS,
+            payload: { [file.file_identifier_hash]: [file.id, `hash:${file.file_hash}`] },
+          },
+        ],
         [],
       );
     });

spec/frontend/diffs/store/actions_spec.js

@@ -99,6 +99,10 @@ describe('DiffsStoreActions', () => {
       const projectPath = '/root/project';
       const dismissEndpoint = '/-/user_callouts';
       const showSuggestPopover = false;
+      const mrReviews = {
+        a: ['z', 'hash:a'],
+        b: ['y', 'hash:a'],
+      };
 
       testAction(
         setBaseConfig,
@@ -110,6 +114,7 @@ describe('DiffsStoreActions', () => {
           projectPath,
           dismissEndpoint,
           showSuggestPopover,
+          mrReviews,
         },
         {
           endpoint: '',
@@ -131,8 +136,21 @@ describe('DiffsStoreActions', () => {
               projectPath,
               dismissEndpoint,
               showSuggestPopover,
+              mrReviews,
             },
           },
+          {
+            type: types.SET_DIFF_FILE_VIEWED,
+            payload: { id: 'z', seen: true },
+          },
+          {
+            type: types.SET_DIFF_FILE_VIEWED,
+            payload: { id: 'a', seen: true },
+          },
+          {
+            type: types.SET_DIFF_FILE_VIEWED,
+            payload: { id: 'y', seen: true },
+          },
         ],
         [],
         done,
@@ -190,10 +208,10 @@ describe('DiffsStoreActions', () => {
           { type: types.SET_RETRIEVING_BATCHES, payload: true },
           { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: res1.diff_files } },
           { type: types.SET_BATCH_LOADING_STATE, payload: 'loaded' },
-          { type: types.VIEW_DIFF_FILE, payload: 'test' },
+          { type: types.SET_CURRENT_DIFF_FILE, payload: 'test' },
           { type: types.SET_DIFF_DATA_BATCH, payload: { diff_files: res2.diff_files } },
           { type: types.SET_BATCH_LOADING_STATE, payload: 'loaded' },
-          { type: types.VIEW_DIFF_FILE, payload: 'test2' },
+          { type: types.SET_CURRENT_DIFF_FILE, payload: 'test2' },
           { type: types.SET_RETRIEVING_BATCHES, payload: false },
           { type: types.SET_BATCH_LOADING_STATE, payload: 'error' },
         ],
@@ -307,7 +325,7 @@ describe('DiffsStoreActions', () => {
     it('should mark currently selected diff and set lineHash and fileHash of highlightedRow', () => {
       testAction(setHighlightedRow, 'ABC_123', {}, [
         { type: types.SET_HIGHLIGHTED_ROW, payload: 'ABC_123' },
-        { type: types.VIEW_DIFF_FILE, payload: 'ABC' },
+        { type: types.SET_CURRENT_DIFF_FILE, payload: 'ABC' },
       ]);
     });
   });
@@ -895,7 +913,7 @@ describe('DiffsStoreActions', () => {
       expect(document.location.hash).toBe('#test');
     });
 
-    it('commits VIEW_DIFF_FILE', () => {
+    it('commits SET_CURRENT_DIFF_FILE', () => {
       const state = {
         treeEntries: {
           path: {
@@ -906,7 +924,7 @@ describe('DiffsStoreActions', () => {
 
       scrollToFile({ state, commit, getters }, { path: 'path' });
 
-      expect(commit).toHaveBeenCalledWith(types.VIEW_DIFF_FILE, 'test');
+      expect(commit).toHaveBeenCalledWith(types.SET_CURRENT_DIFF_FILE, 'test');
     });
   });
 
@@ -1428,7 +1446,7 @@ describe('DiffsStoreActions', () => {
   });
 
   describe('setCurrentDiffFileIdFromNote', () => {
-    it('commits VIEW_DIFF_FILE', () => {
+    it('commits SET_CURRENT_DIFF_FILE', () => {
       const commit = jest.fn();
       const state = { diffFiles: [{ file_hash: '123' }] };
       const rootGetters = {
@@ -1438,10 +1456,10 @@ describe('DiffsStoreActions', () => {
 
       setCurrentDiffFileIdFromNote({ commit, state, rootGetters }, '1');
 
-      expect(commit).toHaveBeenCalledWith(types.VIEW_DIFF_FILE, '123');
+      expect(commit).toHaveBeenCalledWith(types.SET_CURRENT_DIFF_FILE, '123');
     });
 
-    it('does not commit VIEW_DIFF_FILE when discussion has no diff_file', () => {
+    it('does not commit SET_CURRENT_DIFF_FILE when discussion has no diff_file', () => {
       const commit = jest.fn();
       const state = { diffFiles: [{ file_hash: '123' }] };
       const rootGetters = {
@@ -1454,7 +1472,7 @@ describe('DiffsStoreActions', () => {
       expect(commit).not.toHaveBeenCalled();
     });
 
-    it('does not commit VIEW_DIFF_FILE when diff file does not exist', () => {
+    it('does not commit SET_CURRENT_DIFF_FILE when diff file does not exist', () => {
       const commit = jest.fn();
       const state = { diffFiles: [{ file_hash: '123' }] };
       const rootGetters = {
@@ -1469,12 +1487,12 @@ describe('DiffsStoreActions', () => {
   });
 
   describe('navigateToDiffFileIndex', () => {
-    it('commits VIEW_DIFF_FILE', (done) => {
+    it('commits SET_CURRENT_DIFF_FILE', (done) => {
       testAction(
         navigateToDiffFileIndex,
         0,
         { diffFiles: [{ file_hash: '123' }] },
-        [{ type: types.VIEW_DIFF_FILE, payload: '123' }],
+        [{ type: types.SET_CURRENT_DIFF_FILE, payload: '123' }],
         [],
         done,
       );
@@ -1523,13 +1541,14 @@ describe('DiffsStoreActions', () => {
   describe('reviewFile', () => {
     const file = {
       id: '123',
+      file_hash: 'xyz',
       file_identifier_hash: 'abc',
       load_collapsed_diff_url: 'gitlab-org/gitlab-test/-/merge_requests/1/diffs',
     };
     it.each`
-      reviews             | diffFile | reviewed
-      ${{ abc: ['123'] }} | ${file}  | ${true}
-      ${{}}               | ${file}  | ${false}
+      reviews                         | diffFile | reviewed
+      ${{ abc: ['123', 'hash:xyz'] }} | ${file}  | ${true}
+      ${{}}                           | ${file}  | ${false}
     `(
       'sets reviews ($reviews) to localStorage and state for file $file if it is marked reviewed=$reviewed',
       ({ reviews, diffFile, reviewed }) => {

spec/frontend/diffs/store/mutations_spec.js

@@ -633,16 +633,36 @@ describe('DiffsStoreMutations', () => {
     });
   });
 
-  describe('VIEW_DIFF_FILE', () => {
+  describe('SET_CURRENT_DIFF_FILE', () => {
     it('updates currentDiffFileId', () => {
       const state = createState();
 
-      mutations[types.VIEW_DIFF_FILE](state, 'somefileid');
+      mutations[types.SET_CURRENT_DIFF_FILE](state, 'somefileid');
 
       expect(state.currentDiffFileId).toBe('somefileid');
     });
   });
 
+  describe('SET_DIFF_FILE_VIEWED', () => {
+    let state;
+
+    beforeEach(() => {
+      state = {
+        viewedDiffFileIds: { 123: true },
+      };
+    });
+
+    it.each`
+      id       | bool     | outcome
+      ${'abc'} | ${true}  | ${{ 123: true, abc: true }}
+      ${'123'} | ${false} | ${{ 123: false }}
+    `('sets the viewed files list to $bool for the id $id', ({ id, bool, outcome }) => {
+      mutations[types.SET_DIFF_FILE_VIEWED](state, { id, seen: bool });
+
+      expect(state.viewedDiffFileIds).toEqual(outcome);
+    });
+  });
+
   describe('Set highlighted row', () => {
     it('sets highlighted row', () => {
       const state = createState();

spec/frontend/diffs/utils/file_reviews_spec.js

@@ -11,14 +11,14 @@ import {
 
 function getDefaultReviews() {
   return {
-    abc: ['123', '098'],
+    abc: ['123', 'hash:xyz', '098', 'hash:uvw'],
   };
 }
 
 describe('File Review(s) utilities', () => {
   const mrPath = 'my/fake/mr/42';
   const storageKey = `${mrPath}-file-reviews`;
-  const file = { id: '123', file_identifier_hash: 'abc' };
+  const file = { id: '123', file_hash: 'xyz', file_identifier_hash: 'abc' };
   const storedValue = JSON.stringify(getDefaultReviews());
   let reviews;
 
@@ -44,14 +44,14 @@ describe('File Review(s) utilities', () => {
   });
 
   describe('reviewStatuses', () => {
-    const file1 = { id: '123', file_identifier_hash: 'abc' };
-    const file2 = { id: '098', file_identifier_hash: 'abc' };
+    const file1 = { id: '123', hash: 'xyz', file_identifier_hash: 'abc' };
+    const file2 = { id: '098', hash: 'uvw', file_identifier_hash: 'abc' };
 
     it.each`
       mrReviews                         | files             | fileReviews
       ${{}}                             | ${[file1, file2]} | ${{ 123: false, '098': false }}
-      ${{ abc: ['123'] }}               | ${[file1, file2]} | ${{ 123: true, '098': false }}
-      ${{ abc: ['098'] }}               | ${[file1, file2]} | ${{ 123: false, '098': true }}
+      ${{ abc: ['123', 'hash:xyz'] }}   | ${[file1, file2]} | ${{ 123: true, '098': false }}
+      ${{ abc: ['098', 'hash:uvw'] }}   | ${[file1, file2]} | ${{ 123: false, '098': true }}
       ${{ def: ['123'] }}               | ${[file1, file2]} | ${{ 123: false, '098': false }}
       ${{ abc: ['123'], def: ['098'] }} | ${[]}             | ${{}}
     `(
@@ -128,7 +128,7 @@ describe('File Review(s) utilities', () => {
 
   describe('markFileReview', () => {
     it("adds a review when there's nothing that already exists", () => {
-      expect(markFileReview(null, file)).toStrictEqual({ abc: ['123'] });
+      expect(markFileReview(null, file)).toStrictEqual({ abc: ['123', 'hash:xyz'] });
     });
 
     it("overwrites an existing review if it's for the same file (identifier hash)", () => {
@@ -136,15 +136,15 @@ describe('File Review(s) utilities', () => {
     });
 
     it('removes a review from the list when `reviewed` is `false`', () => {
-      expect(markFileReview(reviews, file, false)).toStrictEqual({ abc: ['098'] });
+      expect(markFileReview(reviews, file, false)).toStrictEqual({ abc: ['098', 'hash:uvw'] });
     });
 
     it('adds a new review if the file ID is new', () => {
-      const updatedFile = { ...file, id: '098' };
-      const allReviews = markFileReview({ abc: ['123'] }, updatedFile);
+      const updatedFile = { ...file, id: '098', file_hash: 'uvw' };
+      const allReviews = markFileReview({ abc: ['123', 'hash:xyz'] }, updatedFile);
 
       expect(allReviews).toStrictEqual(getDefaultReviews());
-      expect(allReviews.abc).toStrictEqual(['123', '098']);
+      expect(allReviews.abc).toStrictEqual(['123', 'hash:xyz', '098', 'hash:uvw']);
     });
 
     it.each`
@@ -158,7 +158,7 @@ describe('File Review(s) utilities', () => {
     it('removes the file key if there are no more reviews for it', () => {
       let updated = markFileReview(reviews, file, false);
 
-      updated = markFileReview(updated, { ...file, id: '098' }, false);
+      updated = markFileReview(updated, { ...file, id: '098', file_hash: 'uvw' }, false);
 
       expect(updated).toStrictEqual({});
     });