From 7a3ca41abbab89f5e126012abed075db817ef78f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 17 Dec 2020 01:19:35 -0700 Subject: [PATCH 1/3] Add utilities for file reviews Also adds helpers for getting MR information only available within hard-coded URLs --- .../javascripts/diffs/utils/file_reviews.js | 61 ++++++++ .../javascripts/diffs/utils/merge_request.js | 10 ++ .../frontend/diffs/utils/file_reviews_spec.js | 146 ++++++++++++++++++ .../diffs/utils/merge_request_spec.js | 19 +++ 4 files changed, 236 insertions(+) create mode 100644 app/assets/javascripts/diffs/utils/file_reviews.js create mode 100644 app/assets/javascripts/diffs/utils/merge_request.js create mode 100644 spec/frontend/diffs/utils/file_reviews_spec.js create mode 100644 spec/frontend/diffs/utils/merge_request_spec.js diff --git a/app/assets/javascripts/diffs/utils/file_reviews.js b/app/assets/javascripts/diffs/utils/file_reviews.js new file mode 100644 index 00000000000000..0047955643aec3 --- /dev/null +++ b/app/assets/javascripts/diffs/utils/file_reviews.js @@ -0,0 +1,61 @@ +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; +} diff --git a/app/assets/javascripts/diffs/utils/merge_request.js b/app/assets/javascripts/diffs/utils/merge_request.js new file mode 100644 index 00000000000000..10e2a4b2cf2fac --- /dev/null +++ b/app/assets/javascripts/diffs/utils/merge_request.js @@ -0,0 +1,10 @@ +export function getDerivedMergeRequestInformation({ endpoint } = {}) { + const mrPath = endpoint + ?.split('/') + .slice(0, -1) + .join('/'); + + return { + mrPath, + }; +} diff --git a/spec/frontend/diffs/utils/file_reviews_spec.js b/spec/frontend/diffs/utils/file_reviews_spec.js new file mode 100644 index 00000000000000..819426ee75f6f9 --- /dev/null +++ b/spec/frontend/diffs/utils/file_reviews_spec.js @@ -0,0 +1,146 @@ +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({}); + }); + }); +}); diff --git a/spec/frontend/diffs/utils/merge_request_spec.js b/spec/frontend/diffs/utils/merge_request_spec.js new file mode 100644 index 00000000000000..e949a70dfe7d56 --- /dev/null +++ b/spec/frontend/diffs/utils/merge_request_spec.js @@ -0,0 +1,19 @@ +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); + }); + }); +}); -- GitLab From 6865570d5243d3eb7fd039631756cbf66d26214c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 17 Dec 2020 12:05:34 -0700 Subject: [PATCH 2/3] Add MR File Reviews to Vuex Also adds a getter to sort the reviews into an array organized in the same order as diffFiles. --- app/assets/javascripts/diffs/store/actions.js | 14 ++++++ app/assets/javascripts/diffs/store/getters.js | 5 +++ .../diffs/store/modules/diff_state.js | 1 + .../javascripts/diffs/store/mutation_types.js | 2 + .../javascripts/diffs/store/mutations.js | 5 +++ spec/frontend/diffs/store/actions_spec.js | 43 +++++++++++++++++++ spec/frontend/diffs/store/getters_spec.js | 22 ++++++++++ spec/frontend/diffs/store/mutations_spec.js | 15 +++++++ 8 files changed, 107 insertions(+) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 5b410051705887..f2d9d86997d30e 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/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); +} diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index baf54188932dbe..6afc6f91fec2ba 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -1,5 +1,6 @@ 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)); +} diff --git a/app/assets/javascripts/diffs/store/modules/diff_state.js b/app/assets/javascripts/diffs/store/modules/diff_state.js index c331e52c887ebd..2aa971374ec2c3 100644 --- a/app/assets/javascripts/diffs/store/modules/diff_state.js +++ b/app/assets/javascripts/diffs/store/modules/diff_state.js @@ -45,4 +45,5 @@ export default () => ({ fileFinderVisible: false, dismissEndpoint: '', showSuggestPopover: true, + mrReviews: {}, }); diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js index 30097239aaa230..4641731c4b65f8 100644 --- a/app/assets/javascripts/diffs/store/mutation_types.js +++ b/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'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 19122c3096f0c3..d3827a924263e2 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/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; + }, }; diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index fef7676e7950f2..55e744e573b846 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/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); + }, + ); + }); }); diff --git a/spec/frontend/diffs/store/getters_spec.js b/spec/frontend/diffs/store/getters_spec.js index 7e936c561fc097..8cbcefa28d16a4 100644 --- a/spec/frontend/diffs/store/getters_spec.js +++ b/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); + }, + ); + }); }); diff --git a/spec/frontend/diffs/store/mutations_spec.js b/spec/frontend/diffs/store/mutations_spec.js index 13e7cad835dc0d..be187c49a206c7 100644 --- a/spec/frontend/diffs/store/mutations_spec.js +++ b/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); + }); + }); }); -- GitLab From eb2ea6867bea68c545416e8097f667d6a22c70db Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 17 Dec 2020 04:31:21 -0700 Subject: [PATCH 3/3] Provide file reviews to Diff Files in the Diffs App --- app/assets/javascripts/diffs/components/app.vue | 14 +++++++++++++- .../javascripts/diffs/components/diff_file.vue | 5 +++++ app/assets/javascripts/diffs/index.js | 7 +++++++ 3 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 7827c78b658f9e..1825e1185754e7 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/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" diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index ed94cabe124c4d..343cee7ad833c1 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/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, diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js index 587220488beea0..6cc9cbf750d85e 100644 --- a/app/assets/javascripts/diffs/index.js +++ b/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), }, }); }, -- GitLab