Merge branch 'feature/file-reviews/store-setup' into 'master'

Add File Review implementation to Diffs app

See merge request gitlab-org/gitlab!50033

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

@@ -124,6 +124,11 @@ export default {
       required: false,
       default: false,
     },
+    mrReviews: {
+      type: Object,
+      required: false,
+      default: () => ({}),
+    },
   },
   data() {
     const treeWidth =
@@ -161,7 +166,12 @@ export default {
       'hasConflicts',
       'viewDiffsFileByFile',
     ]),
-    ...mapGetters('diffs', ['whichCollapsedTypes', 'isParallelView', 'currentDiffIndex']),
+    ...mapGetters('diffs', [
+      'whichCollapsedTypes',
+      'isParallelView',
+      'currentDiffIndex',
+      'fileReviews',
+    ]),
     ...mapGetters(['isNotesFetched', 'getNoteableData']),
     diffs() {
       if (!this.viewDiffsFileByFile) {
@@ -261,6 +271,7 @@ export default {
       dismissEndpoint: this.dismissEndpoint,
       showSuggestPopover: this.showSuggestPopover,
       viewDiffsFileByFile: fileByFile(this.fileByFileUserPreference),
+      mrReviews: this.mrReviews || {},
     });
 
     if (this.shouldShow) {
@@ -519,6 +530,7 @@ export default {
               v-for="(file, index) in diffs"
               :key="file.newPath"
               :file="file"
+              :reviewed="fileReviews[index]"
               :is-first-file="index === 0"
               :is-last-file="index === diffs.length - 1"
               :help-page-path="helpPagePath"

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

@@ -37,6 +37,11 @@ export default {
       type: Object,
       required: true,
     },
+    reviewed: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
     isFirstFile: {
       type: Boolean,
       required: false,

app/assets/javascripts/diffs/index.js

@@ -5,6 +5,10 @@ import { parseBoolean } from '~/lib/utils/common_utils';
 import FindFile from '~/vue_shared/components/file_finder/index.vue';
 import eventHub from '../notes/event_hub';
 import diffsApp from './components/app.vue';
+
+import { getDerivedMergeRequestInformation } from './utils/merge_request';
+import { getReviewsForMergeRequest } from './utils/file_reviews';
+
 import { TREE_LIST_STORAGE_KEY, DIFF_WHITESPACE_COOKIE_NAME } from './constants';
 
 export default function initDiffsApp(store) {
@@ -102,6 +106,8 @@ export default function initDiffsApp(store) {
       ...mapActions('diffs', ['setRenderTreeList', 'setShowWhitespace']),
     },
     render(createElement) {
+      const { mrPath } = getDerivedMergeRequestInformation({ endpoint: this.endpoint });
+
       return createElement('diffs-app', {
         props: {
           endpoint: this.endpoint,
@@ -117,6 +123,7 @@ export default function initDiffsApp(store) {
           dismissEndpoint: this.dismissEndpoint,
           showSuggestPopover: this.showSuggestPopover,
           fileByFileUserPreference: this.viewDiffsFileByFile,
+          mrReviews: getReviewsForMergeRequest(mrPath),
         },
       });
     },

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

@@ -50,6 +50,8 @@ import {
 } from '../constants';
 import { diffViewerModes } from '~/ide/constants';
 import { isCollapsed } from '../utils/diff_file';
+import { getDerivedMergeRequestInformation } from '../utils/merge_request';
+import { markFileReview, setReviewsForMergeRequest } from '../utils/file_reviews';
 
 export const setBaseConfig = ({ commit }, options) => {
   const {
@@ -61,6 +63,7 @@ export const setBaseConfig = ({ commit }, options) => {
     dismissEndpoint,
     showSuggestPopover,
     viewDiffsFileByFile,
+    mrReviews,
   } = options;
   commit(types.SET_BASE_CONFIG, {
     endpoint,
@@ -71,6 +74,7 @@ export const setBaseConfig = ({ commit }, options) => {
     dismissEndpoint,
     showSuggestPopover,
     viewDiffsFileByFile,
+    mrReviews,
   });
 };
 
@@ -741,3 +745,13 @@ export const setFileByFile = ({ commit }, { fileByFile }) => {
     mergeUrlParams({ [DIFF_FILE_BY_FILE_COOKIE_NAME]: fileViewMode }, window.location.href),
   );
 };
+
+export function reviewFile({ commit, state, getters }, { file, reviewed = true }) {
+  const { mrPath } = getDerivedMergeRequestInformation({ endpoint: file.load_collapsed_diff_url });
+  const reviews = setReviewsForMergeRequest(
+    mrPath,
+    markFileReview(getters.fileReviews(state), file, reviewed),
+  );
+
+  commit(types.SET_MR_FILE_REVIEWS, reviews);
+}

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

 import { __, n__ } from '~/locale';
 import { parallelizeDiffLines } from './utils';
+import { isFileReviewed } from '../utils/file_reviews';
 import {
   PARALLEL_DIFF_VIEW_TYPE,
   INLINE_DIFF_VIEW_TYPE,
@@ -149,3 +150,7 @@ export const diffLines = state => (file, unifiedDiffComponents) => {
     state.diffViewType === INLINE_DIFF_VIEW_TYPE,
   );
 };
+
+export function fileReviews(state) {
+  return state.diffFiles.map(file => isFileReviewed(state.mrReviews, file));
+}

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

@@ -45,4 +45,5 @@ export default () => ({
   fileFinderVisible: false,
   dismissEndpoint: '',
   showSuggestPopover: true,
+  mrReviews: {},
 });

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

@@ -7,6 +7,8 @@ export const SET_DIFF_METADATA = 'SET_DIFF_METADATA';
 export const SET_DIFF_DATA_BATCH = 'SET_DIFF_DATA_BATCH';
 export const SET_DIFF_FILES = 'SET_DIFF_FILES';
 
+export const SET_MR_FILE_REVIEWS = 'SET_MR_FILE_REVIEWS';
+
 export const SET_DIFF_VIEW_TYPE = 'SET_DIFF_VIEW_TYPE';
 export const SET_COVERAGE_DATA = 'SET_COVERAGE_DATA';
 export const SET_MERGE_REQUEST_DIFFS = 'SET_MERGE_REQUEST_DIFFS';

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

@@ -37,6 +37,7 @@ export default {
       dismissEndpoint,
       showSuggestPopover,
       viewDiffsFileByFile,
+      mrReviews,
     } = options;
     Object.assign(state, {
       endpoint,
@@ -47,6 +48,7 @@ export default {
       dismissEndpoint,
       showSuggestPopover,
       viewDiffsFileByFile,
+      mrReviews,
     });
   },
 
@@ -353,4 +355,7 @@ export default {
   [types.SET_FILE_BY_FILE](state, fileByFile) {
     state.viewDiffsFileByFile = fileByFile;
   },
+  [types.SET_MR_FILE_REVIEWS](state, newReviews) {
+    state.mrReviews = newReviews;
+  },
 };

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

+function getFileReviewsKey(mrPath) {
+  return `${mrPath}-file-reviews`;
+}
+
+export function getReviewsForMergeRequest(mrPath) {
+  const reviewsForMr = localStorage.getItem(getFileReviewsKey(mrPath));
+  let reviews = {};
+
+  if (reviewsForMr) {
+    try {
+      reviews = JSON.parse(reviewsForMr);
+    } catch (err) {
+      reviews = {};
+    }
+  }
+
+  return reviews;
+}
+
+export function setReviewsForMergeRequest(mrPath, reviews) {
+  localStorage.setItem(getFileReviewsKey(mrPath), JSON.stringify(reviews));
+
+  return reviews;
+}
+
+export function isFileReviewed(reviews, file) {
+  const fileReviews = reviews[file.file_identifier_hash];
+
+  return file?.id && fileReviews?.length ? new Set(fileReviews).has(file.id) : false;
+}
+
+export function reviewable(file) {
+  return Boolean(file.id) && Boolean(file.file_identifier_hash);
+}
+
+export function markFileReview(reviews, file, reviewed = true) {
+  const usableReviews = { ...(reviews || {}) };
+  let updatedReviews = usableReviews;
+  let fileReviews;
+
+  if (reviewable(file)) {
+    fileReviews = new Set([...(usableReviews[file.file_identifier_hash] || [])]);
+
+    if (reviewed) {
+      fileReviews.add(file.id);
+    } else {
+      fileReviews.delete(file.id);
+    }
+
+    updatedReviews = {
+      ...usableReviews,
+      [file.file_identifier_hash]: Array.from(fileReviews),
+    };
+
+    if (updatedReviews[file.file_identifier_hash].length === 0) {
+      delete updatedReviews[file.file_identifier_hash];
+    }
+  }
+
+  return updatedReviews;
+}

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

+export function getDerivedMergeRequestInformation({ endpoint } = {}) {
+  const mrPath = endpoint
+    ?.split('/')
+    .slice(0, -1)
+    .join('/');
+
+  return {
+    mrPath,
+  };
+}

spec/frontend/diffs/store/actions_spec.js

@@ -49,6 +49,7 @@ import {
   setCurrentDiffFileIdFromNote,
   navigateToDiffFileIndex,
   setFileByFile,
+  reviewFile,
 } from '~/diffs/store/actions';
 import eventHub from '~/notes/event_hub';
 import * as types from '~/diffs/store/mutation_types';
@@ -1472,4 +1473,46 @@ describe('DiffsStoreActions', () => {
       );
     });
   });
+
+  describe('reviewFile', () => {
+    const file = {
+      id: '123',
+      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}
+    `(
+      'sets reviews ($reviews) to localStorage and state for file $file if it is marked reviewed=$reviewed',
+      ({ reviews, diffFile, reviewed }) => {
+        const commitSpy = jest.fn();
+        const getterSpy = jest.fn().mockReturnValue([]);
+
+        reviewFile(
+          {
+            commit: commitSpy,
+            getters: {
+              fileReviews: getterSpy,
+            },
+            state: {
+              mrReviews: { abc: ['123'] },
+            },
+          },
+          {
+            file: diffFile,
+            reviewed,
+          },
+        );
+
+        expect(localStorage.setItem).toHaveBeenCalledTimes(1);
+        expect(localStorage.setItem).toHaveBeenCalledWith(
+          'gitlab-org/gitlab-test/-/merge_requests/1-file-reviews',
+          JSON.stringify(reviews),
+        );
+        expect(commitSpy).toHaveBeenCalledWith(types.SET_MR_FILE_REVIEWS, reviews);
+      },
+    );
+  });
 });

spec/frontend/diffs/store/getters_spec.js

@@ -372,4 +372,26 @@ describe('Diffs Module Getters', () => {
       });
     });
   });
+
+  describe('fileReviews', () => {
+    const file1 = { id: '123', file_identifier_hash: 'abc' };
+    const file2 = { id: '098', file_identifier_hash: 'abc' };
+
+    it.each`
+      reviews                           | files             | fileReviews
+      ${{}}                             | ${[file1, file2]} | ${[false, false]}
+      ${{ abc: ['123'] }}               | ${[file1, file2]} | ${[true, false]}
+      ${{ abc: ['098'] }}               | ${[file1, file2]} | ${[false, true]}
+      ${{ def: ['123'] }}               | ${[file1, file2]} | ${[false, false]}
+      ${{ abc: ['123'], def: ['098'] }} | ${[]}             | ${[]}
+    `(
+      'returns $fileReviews based on the diff files in state and the existing reviews $reviews',
+      ({ reviews, files, fileReviews }) => {
+        localState.diffFiles = files;
+        localState.mrReviews = reviews;
+
+        expect(getters.fileReviews(localState)).toStrictEqual(fileReviews);
+      },
+    );
+  });
 });

spec/frontend/diffs/store/mutations_spec.js

@@ -906,4 +906,19 @@ describe('DiffsStoreMutations', () => {
       expect(state.viewDiffsFileByFile).toBe(value);
     });
   });
+
+  describe('SET_MR_FILE_REVIEWS', () => {
+    it.each`
+      newReviews          | oldReviews
+      ${{ abc: ['123'] }} | ${{}}
+      ${{ abc: [] }}      | ${{ abc: ['123'] }}
+      ${{}}               | ${{ abc: ['123'] }}
+    `('sets mrReviews to $newReviews', ({ newReviews, oldReviews }) => {
+      const state = { mrReviews: oldReviews };
+
+      mutations[types.SET_MR_FILE_REVIEWS](state, newReviews);
+
+      expect(state.mrReviews).toStrictEqual(newReviews);
+    });
+  });
 });

spec/frontend/diffs/utils/file_reviews_spec.js

+import { useLocalStorageSpy } from 'helpers/local_storage_helper';
+
+import {
+  getReviewsForMergeRequest,
+  setReviewsForMergeRequest,
+  isFileReviewed,
+  markFileReview,
+  reviewable,
+} from '~/diffs/utils/file_reviews';
+
+function getDefaultReviews() {
+  return {
+    abc: ['123', '098'],
+  };
+}
+
+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 storedValue = JSON.stringify(getDefaultReviews());
+  let reviews;
+
+  useLocalStorageSpy();
+
+  beforeEach(() => {
+    reviews = getDefaultReviews();
+    localStorage.clear();
+  });
+
+  describe('getReviewsForMergeRequest', () => {
+    it('fetches the appropriate stored reviews from localStorage', () => {
+      getReviewsForMergeRequest(mrPath);
+
+      expect(localStorage.getItem).toHaveBeenCalledTimes(1);
+      expect(localStorage.getItem).toHaveBeenCalledWith(storageKey);
+    });
+
+    it('returns an empty object if there have never been stored reviews for this MR', () => {
+      expect(getReviewsForMergeRequest(mrPath)).toStrictEqual({});
+    });
+
+    it.each`
+      data
+      ${'+++'}
+      ${'{ lookinGood: "yeah!", missingClosingBrace: "yeah :(" '}
+    `(
+      "returns an empty object if the stored reviews are corrupted/aren't parseable as JSON (like: $data)",
+      ({ data }) => {
+        localStorage.getItem.mockReturnValueOnce(data);
+
+        expect(getReviewsForMergeRequest(mrPath)).toStrictEqual({});
+      },
+    );
+
+    it('fetches the reviews for the MR if they exist', () => {
+      localStorage.setItem(storageKey, storedValue);
+
+      expect(getReviewsForMergeRequest(mrPath)).toStrictEqual(reviews);
+    });
+  });
+
+  describe('setReviewsForMergeRequest', () => {
+    it('sets the new value to localStorage', () => {
+      setReviewsForMergeRequest(mrPath, reviews);
+
+      expect(localStorage.setItem).toHaveBeenCalledTimes(1);
+      expect(localStorage.setItem).toHaveBeenCalledWith(storageKey, storedValue);
+    });
+
+    it('returns the new value for chainability', () => {
+      expect(setReviewsForMergeRequest(mrPath, reviews)).toStrictEqual(reviews);
+    });
+  });
+
+  describe('isFileReviewed', () => {
+    it.each`
+      description                            | diffFile                      | fileReviews
+      ${'the file does not have an `id`'}    | ${{ ...file, id: undefined }} | ${getDefaultReviews()}
+      ${'there are no reviews for the file'} | ${file}                       | ${{ ...getDefaultReviews(), abc: undefined }}
+    `('returns `false` if $description', ({ diffFile, fileReviews }) => {
+      expect(isFileReviewed(fileReviews, diffFile)).toBe(false);
+    });
+
+    it("returns `true` for a file if it's available in the provided reviews", () => {
+      expect(isFileReviewed(reviews, file)).toBe(true);
+    });
+  });
+
+  describe('reviewable', () => {
+    it.each`
+      response | diffFile                                        | description
+      ${true}  | ${file}                                         | ${'has an `.id` and a `.file_identifier_hash`'}
+      ${false} | ${{ file_identifier_hash: 'abc' }}              | ${'does not have an `.id`'}
+      ${false} | ${{ ...file, id: undefined }}                   | ${'has an undefined `.id`'}
+      ${false} | ${{ ...file, id: null }}                        | ${'has a null `.id`'}
+      ${false} | ${{ ...file, id: 0 }}                           | ${'has an `.id` set to 0'}
+      ${false} | ${{ ...file, id: false }}                       | ${'has an `.id` set to false'}
+      ${false} | ${{ id: '123' }}                                | ${'does not have a `.file_identifier_hash`'}
+      ${false} | ${{ ...file, file_identifier_hash: undefined }} | ${'has an undefined `.file_identifier_hash`'}
+      ${false} | ${{ ...file, file_identifier_hash: null }}      | ${'has a null `.file_identifier_hash`'}
+      ${false} | ${{ ...file, file_identifier_hash: 0 }}         | ${'has a `.file_identifier_hash` set to 0'}
+      ${false} | ${{ ...file, file_identifier_hash: false }}     | ${'has a `.file_identifier_hash` set to false'}
+    `('returns `$response` when the file $description`', ({ response, diffFile }) => {
+      expect(reviewable(diffFile)).toBe(response);
+    });
+  });
+
+  describe('markFileReview', () => {
+    it("adds a review when there's nothing that already exists", () => {
+      expect(markFileReview(null, file)).toStrictEqual({ abc: ['123'] });
+    });
+
+    it("overwrites an existing review if it's for the same file (identifier hash)", () => {
+      expect(markFileReview(reviews, file)).toStrictEqual(getDefaultReviews());
+    });
+
+    it('removes a review from the list when `reviewed` is `false`', () => {
+      expect(markFileReview(reviews, file, false)).toStrictEqual({ abc: ['098'] });
+    });
+
+    it('adds a new review if the file ID is new', () => {
+      const updatedFile = { ...file, id: '098' };
+      const allReviews = markFileReview({ abc: ['123'] }, updatedFile);
+
+      expect(allReviews).toStrictEqual(getDefaultReviews());
+      expect(allReviews.abc).toStrictEqual(['123', '098']);
+    });
+
+    it.each`
+      description                            | diffFile
+      ${'missing an `.id`'}                  | ${{ file_identifier_hash: 'abc' }}
+      ${'missing a `.file_identifier_hash`'} | ${{ id: '123' }}
+    `("doesn't modify the reviews if the file is $description", ({ diffFile }) => {
+      expect(markFileReview(reviews, diffFile)).toStrictEqual(getDefaultReviews());
+    });
+
+    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);
+
+      expect(updated).toStrictEqual({});
+    });
+  });
+});

spec/frontend/diffs/utils/merge_request_spec.js

+import { getDerivedMergeRequestInformation } from '~/diffs/utils/merge_request';
+import { diffMetadata } from '../mock_data/diff_metadata';
+
+describe('Merge Request utilities', () => {
+  describe('getDerivedMergeRequestInformation', () => {
+    const endpoint = `${diffMetadata.latest_version_path}.json?searchParam=irrelevant`;
+    const mrPath = diffMetadata.latest_version_path.replace(/\/diffs$/, '');
+
+    it.each`
+      argument                   | response
+      ${{ endpoint }}            | ${{ mrPath }}
+      ${{}}                      | ${{ mrPath: undefined }}
+      ${{ endpoint: undefined }} | ${{ mrPath: undefined }}
+      ${{ endpoint: null }}      | ${{ mrPath: undefined }}
+    `('generates the correct derived results based on $argument', ({ argument, response }) => {
+      expect(getDerivedMergeRequestInformation(argument)).toStrictEqual(response);
+    });
+  });
+});