diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 1465db9cf3b1ee2d47f20fd6cd8b892c956dd7fe..02eac66f60907149bf843375dde93b62b06af415 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -31,6 +31,7 @@ import { useNotes } from '~/notes/store/legacy_notes'; import { useFindingsDrawer } from '~/mr_notes/store/findings_drawer'; import { useBatchComments } from '~/batch_comments/store'; import { querySelectionClosest } from '~/lib/utils/selection'; +import { useCodeReview } from '~/diffs/stores/code_review'; import { sortFindingsByFile } from '../utils/sort_findings_by_file'; import { ALERT_OVERFLOW_HIDDEN, @@ -51,7 +52,6 @@ import { } from '../constants'; import { isCollapsed } from '../utils/diff_file'; import diffsEventHub from '../event_hub'; -import { reviewStatuses } from '../utils/file_reviews'; import { diffsApp } from '../utils/performance'; import { updateChangesTabCount, extractFileHash } from '../utils/merge_request'; import { queueRedisHllEvents } from '../utils/queue_events'; @@ -238,7 +238,6 @@ export default { 'isTreeLoaded', 'hasConflicts', 'viewDiffsFileByFile', - 'mrReviews', 'renderTreeList', 'showWhitespace', 'addedLines', @@ -254,6 +253,7 @@ export default { ...mapState(useNotes, ['discussions', 'isNotesFetched', 'getNoteableData']), ...mapState(useFileBrowser, ['fileBrowserVisible']), ...mapState(useFindingsDrawer, ['activeDrawer']), + ...mapState(useCodeReview, ['reviewedIds']), diffs() { if (!this.viewDiffsFileByFile) { return this.diffFiles; @@ -307,9 +307,6 @@ export default { return visible; }, - fileReviews() { - return reviewStatuses(this.diffFiles, this.mrReviews); - }, renderFileTree() { return this.renderDiffFiles && this.fileBrowserVisible; }, @@ -469,7 +466,6 @@ export default { 'setDiffViewType', 'setShowWhitespace', 'goToFile', - 'reviewFile', 'setFileCollapsedByUser', 'toggleTreeOpen', ]), @@ -488,9 +484,12 @@ export default { const activeFile = this.diffFiles.find((file) => file.file_hash === this.currentDiffFileId); if (activeFile) { - const reviewed = !this.fileReviews[activeFile.id]; - this.reviewFile({ file: activeFile, reviewed }); - + const reviewed = !( + useCodeReview().reviewedIds[activeFile.code_review_id] || + useCodeReview().reviewedIds[activeFile.id] + ); + useCodeReview().removeId(activeFile.id); + useCodeReview().setReviewed(activeFile.code_review_id, reviewed); this.setFileCollapsedByUser({ filePath: activeFile.file_path, collapsed: reviewed }); } }, @@ -886,7 +885,7 @@ export default { :file="item" :codequality-data="codequalityData" :sast-data="sastData" - :reviewed="fileReviews[item.id]" + :reviewed="reviewedIds[item.code_review_id] || reviewedIds[item.id]" :is-first-file="index === 0" :is-last-file="index === diffFilesLength - 1" :help-page-path="helpPagePath" @@ -908,7 +907,7 @@ export default { :file="file" :codequality-data="codequalityData" :sast-data="sastData" - :reviewed="fileReviews[file.id]" + :reviewed="reviewedIds[file.code_review_id] || reviewedIds[file.id]" :is-first-file="index === 0" :is-last-file="index === diffFilesLength - 1" :help-page-path="helpPagePath" diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue index e87a4bb5074f507234a7b1c7d63f751178a7286f..0293c2c9b351a3a371a8c7352f81eac2b412e679 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -21,15 +21,14 @@ import { truncateSha } from '~/lib/utils/text_utility'; import { sanitize } from '~/lib/dompurify'; import { __, s__, sprintf } from '~/locale'; import ClipboardButton from '~/vue_shared/components/clipboard_button.vue'; - import { createFileUrl, fileContentsId } from '~/diffs/components/diff_row_utils'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { useNotes } from '~/notes/store/legacy_notes'; +import { useCodeReview } from '~/diffs/stores/code_review'; import { DIFF_FILE_AUTOMATIC_COLLAPSE } from '../constants'; import diffsEventHub from '../event_hub'; import { DIFF_FILE_HEADER } from '../i18n'; import { collapsedType, isCollapsed } from '../utils/diff_file'; -import { reviewable } from '../utils/file_reviews'; import DiffStats from './diff_stats.vue'; const createHotkeyHtml = (key) => ``; @@ -191,7 +190,7 @@ export default { ); }, isReviewable() { - return reviewable(this.diffFile); + return Boolean(this.diffFile.code_review_id) && Boolean(this.diffFile.file_identifier_hash); }, externalUrlLabel() { return sprintf(__('View on %{url}'), { url: this.diffFile.formatted_external_url }); @@ -263,7 +262,6 @@ export default { 'toggleFileDiscussionWrappers', 'toggleFullDiff', 'setCurrentFileHash', - 'reviewFile', 'setFileCollapsedByUser', 'setFileForcedOpen', 'toggleFileCommentForm', @@ -311,7 +309,11 @@ export default { const closed = !open; const reviewed = newReviewedStatus; - this.reviewFile({ file: this.diffFile, reviewed }); + // diffFile.id is generated on the frontend, code_review_id is generated on the backend + // we need to leave only code_review_id as our single source of truth + // this is done to prevent legacy id from persisting in the local storage + useCodeReview().removeId(this.diffFile.id); + useCodeReview().setReviewed(this.diffFile.code_review_id, reviewed); if (reviewed && autoCollapsed) { this.setFileCollapsedByUser({ diff --git a/app/assets/javascripts/diffs/components/diff_file_row.vue b/app/assets/javascripts/diffs/components/diff_file_row.vue index 061588373b4330cdec25848b57bdd551065eacaf..606f86a1fb67c84cd969b79fbb47b8f492f6a7bf 100644 --- a/app/assets/javascripts/diffs/components/diff_file_row.vue +++ b/app/assets/javascripts/diffs/components/diff_file_row.vue @@ -39,7 +39,8 @@ export default { return !this.hideFileStats && this.file.type === 'blob'; }, fileClasses() { - return this.file.type === 'blob' && !this.viewedFiles[this.file.id || this.file.fileHash] + return this.file.type === 'blob' && + !(this.viewedFiles[this.file.codeReviewId] || this.viewedFiles[this.file.id]) ? 'gl-font-bold' : 'gl-text-subtle'; }, diff --git a/app/assets/javascripts/diffs/components/tree_list.vue b/app/assets/javascripts/diffs/components/tree_list.vue index aaa7697632560f041708aafd862ed25eb8651a73..a9472ce292d3f49ae3b9c7789f9f85a44ad31e81 100644 --- a/app/assets/javascripts/diffs/components/tree_list.vue +++ b/app/assets/javascripts/diffs/components/tree_list.vue @@ -13,6 +13,7 @@ import { RecycleScroller } from 'vendor/vue-virtual-scroller'; import { isElementClipped } from '~/lib/utils/common_utils'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { MR_FOCUS_FILE_BROWSER } from '~/behaviors/shortcuts/keybindings'; +import { useCodeReview } from '~/diffs/stores/code_review'; import DiffFileRow from './diff_file_row.vue'; export default { @@ -61,12 +62,12 @@ export default { ...mapState(useLegacyDiffs, [ 'renderTreeList', 'currentDiffFileId', - 'viewedDiffFileIds', 'fileTree', 'allBlobs', 'linkedFile', 'flatBlobsList', ]), + ...mapState(useCodeReview, ['reviewedIds']), flatUngroupedList() { return this.flatBlobsList.reduce((acc, blob, index) => { const loading = this.isLoading(blob.fileHash); @@ -296,7 +297,7 @@ export default { { + this.reviewedIds[id] = true; + }); + this.reviewedIds = { ...this.reviewedIds }; + }, + restoreFromLegacyMrReviews() { + const reviewsForMr = localStorage.getItem(`${this.mrPath}-file-reviews`); + if (!reviewsForMr) return; + const reviews = JSON.parse(reviewsForMr); + Object.values(reviews).forEach((values) => { + values.forEach((value) => { + // eslint-disable-next-line @gitlab/require-i18n-strings + if (!value.startsWith('hash:')) this.reviewedIds[value] = true; + }); + }); + this.reviewedIds = { ...this.reviewedIds }; + localStorage.removeItem(`${this.mrPath}-file-reviews`); + this.autosave(); + }, + setReviewed(id, reviewed) { + this.reviewedIds = { ...this.reviewedIds, [id]: reviewed }; + this.autosave(); + }, + removeId(id) { + delete this.reviewedIds[id]; + }, + autosave() { + const ids = this.markedAsViewedIds; + if (ids.length) { + localStorage.setItem(this.autosaveKey, JSON.stringify(ids)); + } else { + localStorage.removeItem(this.autosaveKey); + } + }, + }, + getters: { + markedAsViewedIds() { + return Object.keys(this.reviewedIds).reduce((acc, key) => { + if (this.reviewedIds[key]) acc.push(key); + return acc; + }, []); + }, + autosaveKey() { + return `code-review-${this.mrPath}`; + }, + }, +}); diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js index 00327a853df9a9b7e2c7a47f1a4868ef2a1c4181..0f233795ff1db152365faee667e524b40fe177ff 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/actions.js @@ -67,7 +67,6 @@ import { ENCODED_FILE_PATHS_MESSAGE, } from '../../i18n'; import eventHub from '../../event_hub'; -import { markFileReview, setReviewsForMergeRequest } from '../../utils/file_reviews'; import { getDerivedMergeRequestInformation } from '../../utils/merge_request'; import { queueRedisHllEvents } from '../../utils/queue_events'; import * as types from '../../store/mutation_types'; @@ -95,7 +94,6 @@ export function setBaseConfig(options) { showSuggestPopover, defaultSuggestionCommitMessage, viewDiffsFileByFile, - mrReviews, diffViewType, perPage, } = options; @@ -111,16 +109,9 @@ export function setBaseConfig(options) { showSuggestPopover, defaultSuggestionCommitMessage, viewDiffsFileByFile, - mrReviews, diffViewType, perPage, }); - - Array.from(new Set(Object.values(mrReviews).flat())).forEach((id) => { - const viewedId = id.replace(/^hash:/, ''); - - this[types.SET_DIFF_FILE_VIEWED]({ id: viewedId, seen: true }); - }); } export async function prefetchSingleFile(treeEntry) { @@ -1085,16 +1076,6 @@ export function setFileByFile({ fileByFile }) { }); } -export function reviewFile({ file, reviewed = true }) { - const { mrPath } = getDerivedMergeRequestInformation({ endpoint: file.load_collapsed_diff_url }); - const reviews = markFileReview(this.mrReviews, file, reviewed); - - setReviewsForMergeRequest(mrPath, reviews); - - this[types.SET_DIFF_FILE_VIEWED]({ id: file.id, seen: reviewed }); - this[types.SET_MR_FILE_REVIEWS](reviews); -} - export function disableVirtualScroller() { this[types.DISABLE_VIRTUAL_SCROLLING](); } diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/index.js b/app/assets/javascripts/diffs/stores/legacy_diffs/index.js index 27b8f829cbb8943d51d07368c8bcdc3d7c024ee6..714f890f025a5034745daa62bad722b76929ad1e 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/index.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/index.js @@ -33,7 +33,6 @@ export const useLegacyDiffs = defineStore('legacyDiffs', { treeEntries: {}, currentDiffFileId: '', projectPath: '', - viewedDiffFileIds: {}, commentForms: [], highlightedRow: null, renderTreeList: true, @@ -43,7 +42,6 @@ export const useLegacyDiffs = defineStore('legacyDiffs', { dismissEndpoint: '', showSuggestPopover: true, defaultSuggestionCommitMessage: '', - mrReviews: {}, latestDiff: true, virtualScrollerDisabled: false, linkedFileHash: null, diff --git a/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js b/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js index ac417d6d3bfe900f5222e2dc9f7ee7cd7af45891..523027c9cc2e126a29a73f222c9e0511d8078e10 100644 --- a/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js +++ b/app/assets/javascripts/diffs/stores/legacy_diffs/mutations.js @@ -32,7 +32,6 @@ export default { showSuggestPopover, defaultSuggestionCommitMessage, viewDiffsFileByFile, - mrReviews, diffViewType, perPage, } = options; @@ -48,7 +47,6 @@ export default { showSuggestPopover, defaultSuggestionCommitMessage, viewDiffsFileByFile, - mrReviews, diffViewType, perPage, }); @@ -296,12 +294,6 @@ export default { [types.SET_CURRENT_DIFF_FILE](fileId) { this.currentDiffFileId = fileId; }, - [types.SET_DIFF_FILE_VIEWED]({ id, seen }) { - this.viewedDiffFileIds = { - ...this.viewedDiffFileIds, - [id]: seen, - }; - }, [types.OPEN_DIFF_FILE_COMMENT_FORM](formData) { this.commentForms.push({ ...formData, @@ -400,9 +392,6 @@ export default { [types.SET_FILE_BY_FILE](fileByFile) { this.viewDiffsFileByFile = fileByFile; }, - [types.SET_MR_FILE_REVIEWS](newReviews) { - this.mrReviews = newReviews; - }, [types.DISABLE_VIRTUAL_SCROLLING]() { this.virtualScrollerDisabled = true; }, diff --git a/app/assets/javascripts/diffs/utils/tree_worker_utils.js b/app/assets/javascripts/diffs/utils/tree_worker_utils.js index e1e3495a51f6c77734e822ab3108822dbf7cc5f2..e8b5a28ca11bf65b5bcf3ec1121ea48c122a3053 100644 --- a/app/assets/javascripts/diffs/utils/tree_worker_utils.js +++ b/app/assets/javascripts/diffs/utils/tree_worker_utils.js @@ -93,6 +93,7 @@ export const generateTreeList = (files) => { tempFile: file.new_file, deleted: file.deleted_file, fileHash: file.file_hash, + codeReviewId: file.code_review_id, addedLines: file.added_lines, removedLines: file.removed_lines, parentPath: parent ? `${parent.path}/` : '/', diff --git a/app/assets/javascripts/mr_notes/init.js b/app/assets/javascripts/mr_notes/init.js index 431b2f5f9b5162c153ca36e8bacf34bf320261b0..c526498d30198349b7892f807a7b98bbef1f773c 100644 --- a/app/assets/javascripts/mr_notes/init.js +++ b/app/assets/javascripts/mr_notes/init.js @@ -4,13 +4,13 @@ import eventHub from '~/notes/event_hub'; import { initDiscussionCounter } from '~/mr_notes/discussion_counter'; import { initOverviewTabCounter } from '~/mr_notes/init_count'; import { getDerivedMergeRequestInformation } from '~/diffs/utils/merge_request'; -import { getReviewsForMergeRequest } from '~/diffs/utils/file_reviews'; import { DIFF_VIEW_COOKIE_NAME, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants'; import { useNotes } from '~/notes/store/legacy_notes'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { useBatchComments } from '~/batch_comments/store'; import { useMrNotes } from '~/mr_notes/store/legacy_mr_notes'; import { pinia } from '~/pinia/instance'; +import { useCodeReview } from '~/diffs/stores/code_review'; function setupMrNotesState(notesDataset, diffsDataset = {}) { const noteableData = JSON.parse(notesDataset.noteableData); @@ -41,11 +41,13 @@ function setupMrNotesState(notesDataset, diffsDataset = {}) { showSuggestPopover: parseBoolean(diffsDataset.showSuggestPopover), viewDiffsFileByFile: parseBoolean(diffsDataset.fileByFileDefault), defaultSuggestionCommitMessage: diffsDataset.defaultSuggestionCommitMessage, - mrReviews: getReviewsForMergeRequest(mrPath), diffViewType: getParameterValues('view')[0] || getCookie(DIFF_VIEW_COOKIE_NAME) || INLINE_DIFF_VIEW_TYPE, perPage: Number(diffsDataset.perPage), }); + useCodeReview().setMrPath(mrPath); + useCodeReview().restoreFromAutosave(); + useCodeReview().restoreFromLegacyMrReviews(); } export function initMrStateLazyLoad() { diff --git a/ee/spec/frontend/diffs/components/app_spec.js b/ee/spec/frontend/diffs/components/app_spec.js index 21b51812593280ece205b11e4320b18aedd0d16b..0b47dd5d7958bf7229f6c4803a088cea5fba6a72 100644 --- a/ee/spec/frontend/diffs/components/app_spec.js +++ b/ee/spec/frontend/diffs/components/app_spec.js @@ -14,6 +14,7 @@ import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { useNotes } from '~/notes/store/legacy_notes'; import { useBatchComments } from '~/batch_comments/store'; import { useFindingsDrawer } from '~/mr_notes/store/findings_drawer'; +import { useCodeReview } from '~/diffs/stores/code_review'; import { codeQualityNewErrorsHandler, SASTParsedHandler, @@ -56,6 +57,7 @@ describe('diffs/components/app', () => { beforeEach(() => { pinia = createTestingPinia({ plugins: [globalAccessorPlugin] }); + useCodeReview(); store = useLegacyDiffs(); store.isLoading = false; diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 439b8d3d770bacd329742166283fbd2193497bff..cac9761347c53936945111577b68d828a948e42b 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -51,6 +51,7 @@ import { useNotes } from '~/notes/store/legacy_notes'; import { useBatchComments } from '~/batch_comments/store'; import { useFindingsDrawer } from '~/mr_notes/store/findings_drawer'; import { querySelectionClosest } from '~/lib/utils/selection'; +import { useCodeReview } from '~/diffs/stores/code_review'; import diffsMockData from '../mock_data/merge_request_diffs'; const TEST_ENDPOINT = `${TEST_HOST}/diff/endpoint`; @@ -102,6 +103,7 @@ describe('diffs/components/app', () => { beforeEach(() => { pinia = createTestingPinia({ plugins: [globalAccessorPlugin] }); + useCodeReview(); store = useLegacyDiffs(); store.isLoading = false; store.isTreeLoaded = true; @@ -332,24 +334,18 @@ describe('diffs/components/app', () => { beforeEach(() => { store.diffFiles = [ - { id: 1, file_hash: '111', file_path: '111.js' }, - { id: 2, file_hash: '222', file_path: '222.js' }, + { id: 1, file_hash: '111', file_path: '111.js', code_review_id: '111' }, + { id: 2, file_hash: '222', file_path: '222.js', code_review_id: '222' }, ]; createComponent(); }); it('marks active file as reviewed when triggering review toggle shortcut', () => { store.currentDiffFileId = '111'; - store.fileReviews = { 1: false }; toggleReview(); - expect(store.reviewFile).toHaveBeenCalledWith({ - file: store.diffFiles[0], - reviewed: true, - }); - expect(store.reviewFile).toHaveBeenCalledTimes(1); - + expect(useCodeReview().setReviewed).toHaveBeenCalledWith('111', true); expect(store.setFileCollapsedByUser).toHaveBeenCalledWith({ filePath: '111.js', collapsed: true, @@ -359,16 +355,11 @@ describe('diffs/components/app', () => { it('marks active file as unreviewed when triggering review toggle shortcut again', () => { store.currentDiffFileId = '222'; - jest.spyOn(wrapper.vm, 'fileReviews', 'get').mockReturnValue({ 2: true }); + useCodeReview().reviewedIds = { 222: true }; toggleReview(); - expect(store.reviewFile).toHaveBeenCalledWith({ - file: store.diffFiles[1], - reviewed: false, - }); - expect(store.reviewFile).toHaveBeenCalledTimes(1); - + expect(useCodeReview().setReviewed).toHaveBeenCalledWith('222', false); expect(store.setFileCollapsedByUser).toHaveBeenCalledWith({ filePath: '222.js', collapsed: false, @@ -381,7 +372,7 @@ describe('diffs/components/app', () => { toggleReview(); - expect(store.reviewFile).not.toHaveBeenCalled(); + expect(useCodeReview().setReviewed).not.toHaveBeenCalled(); expect(store.setFileCollapsedByUser).not.toHaveBeenCalled(); }); }); diff --git a/spec/frontend/diffs/components/diff_file_header_spec.js b/spec/frontend/diffs/components/diff_file_header_spec.js index 5fcdcc60b973d577366a843137d61b3d12a19e1e..56c12f988f8fe592005a0648d92774867a81ddfb 100644 --- a/spec/frontend/diffs/components/diff_file_header_spec.js +++ b/spec/frontend/diffs/components/diff_file_header_spec.js @@ -15,6 +15,7 @@ import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { globalAccessorPlugin } from '~/pinia/plugins'; import { useNotes } from '~/notes/store/legacy_notes'; import diffsEventHub from '~/diffs/event_hub'; +import { useCodeReview } from '~/diffs/stores/code_review'; import diffDiscussionsMockData from '../mock_data/diff_discussions'; jest.mock('~/lib/utils/common_utils', () => ({ @@ -45,6 +46,7 @@ const createDiffFile = () => ({ readable_text: true, icon: 'doc-text', }, + code_review_id: '123', }); Vue.use(PiniaVuePlugin); @@ -88,6 +90,7 @@ describe('DiffFileHeader component', () => { beforeEach(() => { pinia = createTestingPinia({ plugins: [globalAccessorPlugin] }); + useCodeReview(); useLegacyDiffs().diffFiles = [createDiffFile()]; useNotes(); }); @@ -541,28 +544,27 @@ describe('DiffFileHeader component', () => { describe('file reviews', () => { it('calls the action to set the new review', () => { jest.spyOn(document.activeElement, 'blur'); + const diffFile = { + ...createDiffFile(), + viewer: { + ...createDiffFile().viewer, + automaticallyCollapsed: false, + manuallyCollapsed: null, + }, + }; createComponent({ props: { - diffFile: { - ...createDiffFile(), - viewer: { - ...createDiffFile().viewer, - automaticallyCollapsed: false, - manuallyCollapsed: null, - }, - }, + diffFile, showLocalFileReviews: true, addMergeRequestButtons: true, }, }); - const file = wrapper.vm.diffFile; - findReviewFileCheckbox().vm.$emit('change', true); expect(document.activeElement.blur).toHaveBeenCalled(); - expect(useLegacyDiffs().reviewFile).toHaveBeenCalledWith({ file, reviewed: true }); + expect(useCodeReview().setReviewed).toHaveBeenCalledWith(diffFile.code_review_id, true); }); it.each` diff --git a/spec/frontend/diffs/components/diff_file_row_spec.js b/spec/frontend/diffs/components/diff_file_row_spec.js index 028bfb9b339759da575fd5a7b606b662d1ff36e6..bf1ff4c73e112451ce6dd2c60491dc7cb8a8e895 100644 --- a/spec/frontend/diffs/components/diff_file_row_spec.js +++ b/spec/frontend/diffs/components/diff_file_row_spec.js @@ -65,7 +65,7 @@ describe('Diff File Row component', () => { createComponent({ file: { type: fileType, - id: '#123456789', + codeReviewId: '#123456789', }, level: 0, hideFileStats: false, @@ -103,15 +103,4 @@ describe('Diff File Row component', () => { expect(wrapper.classes('is-active')).toBe(true); }); - - it('marks viewed files', () => { - createComponent({ - level: 0, - file: { fileHash: '123', type: 'blob' }, - viewedFiles: { 123: true }, - hideFileStats: false, - }); - - expect(wrapper.findComponent(FileRow).props('fileClasses')).toContain('gl-text-subtle'); - }); }); diff --git a/spec/frontend/diffs/components/tree_list_spec.js b/spec/frontend/diffs/components/tree_list_spec.js index a4ee48353b2191fe4ef24666b986e6bf480a070b..c104709a8e41d22ee88887a750600bc4d2a41828 100644 --- a/spec/frontend/diffs/components/tree_list_spec.js +++ b/spec/frontend/diffs/components/tree_list_spec.js @@ -12,6 +12,7 @@ import { isElementClipped } from '~/lib/utils/common_utils'; import { globalAccessorPlugin } from '~/pinia/plugins'; import { useLegacyDiffs } from '~/diffs/stores/legacy_diffs'; import { getDiffFileMock } from 'jest/diffs/mock_data/diff_file'; +import { useCodeReview } from '~/diffs/stores/code_review'; jest.mock('~/lib/utils/common_utils'); @@ -48,6 +49,7 @@ describe('Diffs tree list component', () => { beforeEach(() => { pinia = createTestingPinia({ plugins: [globalAccessorPlugin], stubActions: false }); + useCodeReview(); useLegacyDiffs().isTreeLoaded = true; useLegacyDiffs().diffFiles = [getDiffFileMock()]; useLegacyDiffs().addedLines = 10; @@ -304,18 +306,18 @@ describe('Diffs tree list component', () => { }); describe('with viewedDiffFileIds', () => { - const viewedDiffFileIds = { fileId: '#12345' }; + const reviewedIds = { 12345: true }; beforeEach(() => { setupFilesInState(); - useLegacyDiffs().viewedDiffFileIds = viewedDiffFileIds; + useCodeReview().reviewedIds = reviewedIds; }); - it('passes the viewedDiffFileIds to the FileTree', async () => { + it('passes the reviewedIds to the FileTree', async () => { createComponent(); await nextTick(); - expect(getFileRow().props('viewedFiles')).toBe(viewedDiffFileIds); + expect(getFileRow().props('viewedFiles')).toBe(reviewedIds); }); }); diff --git a/spec/frontend/diffs/stores/code_review_spec.js b/spec/frontend/diffs/stores/code_review_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..2aa7712ff01420309cb4876eda46f0e2c9ba4138 --- /dev/null +++ b/spec/frontend/diffs/stores/code_review_spec.js @@ -0,0 +1,107 @@ +import { createTestingPinia } from '@pinia/testing'; +import { useCodeReview } from '~/diffs/stores/code_review'; + +describe('CodeReview store', () => { + beforeEach(() => { + createTestingPinia({ stubActions: false }); + }); + + describe('#setMrPath', () => { + it('sets mr path', () => { + useCodeReview().setMrPath('foo'); + expect(useCodeReview().mrPath).toBe('foo'); + }); + }); + + describe('#restoreFromAutosave', () => { + it('restores from autosave', () => { + const items = ['bar']; + useCodeReview().setMrPath('foo'); + localStorage.setItem(useCodeReview().autosaveKey, JSON.stringify(items)); + useCodeReview().restoreFromAutosave(); + expect(useCodeReview().reviewedIds.bar).toBe(true); + }); + }); + + describe('#restoreFromLegacyMrReviews', () => { + it('restores from autosave', () => { + const items = { bar: ['baz'] }; + useCodeReview().setMrPath('foo'); + localStorage.setItem('foo-file-reviews', JSON.stringify(items)); + useCodeReview().restoreFromLegacyMrReviews(); + expect(useCodeReview().reviewedIds.baz).toBe(true); + }); + + it('does not restore hash: values', () => { + const items = { bar: ['hash:baz'] }; + useCodeReview().setMrPath('foo'); + localStorage.setItem('foo-file-reviews', JSON.stringify(items)); + useCodeReview().restoreFromLegacyMrReviews(); + expect(useCodeReview().reviewedIds.baz).toBe(undefined); + }); + + it('stores state', () => { + useCodeReview().setMrPath('foo'); + localStorage.setItem('foo-file-reviews', JSON.stringify({ bar: ['baz'] })); + useCodeReview().restoreFromLegacyMrReviews(); + const items = localStorage.getItem(useCodeReview().autosaveKey); + expect(JSON.parse(items)).toContain('baz'); + }); + }); + + describe('#setReviewed', () => { + it('marks file as reviewed', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().setReviewed('bar', true); + expect(useCodeReview().reviewedIds.bar).toBe(true); + }); + + it('stores state', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().setReviewed('bar', true); + const items = localStorage.getItem(useCodeReview().autosaveKey); + expect(JSON.parse(items)).toContain('bar'); + }); + }); + + describe('#removeId', () => { + it('removes existing ID', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().reviewedIds = { bar: true }; + useCodeReview().removeId('bar'); + expect(useCodeReview().reviewedIds.bar).toBe(undefined); + }); + }); + + describe('#autosave', () => { + it('stores state', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().reviewedIds = { bar: true }; + useCodeReview().autosave(); + const items = localStorage.getItem(useCodeReview().autosaveKey); + expect(JSON.parse(items)).toContain('bar'); + }); + + it('clears storage', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().autosave(); + expect(localStorage.getItem(useCodeReview().autosaveKey)).toBe(null); + }); + }); + + describe('#markedAsViewedIds', () => { + it('returns only viewed ids', () => { + useCodeReview().setMrPath('foo'); + useCodeReview().setReviewed('bar', false); + useCodeReview().setReviewed('baz', true); + expect(useCodeReview().markedAsViewedIds).toMatchObject(['baz']); + }); + }); + + describe('#autosaveKey', () => { + it('returns autosave key', () => { + useCodeReview().setMrPath('foo'); + expect(useCodeReview().autosaveKey).toBe('code-review-foo'); + }); + }); +}); diff --git a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js index e70f39fc77bbd386fc7c74df86273bd59ed71ceb..f1d74a2b1c140edbfde43280ea2d3bde3e03de89 100644 --- a/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js +++ b/spec/frontend/diffs/stores/legacy_diffs/actions_spec.js @@ -113,10 +113,6 @@ describe('legacyDiffs actions', () => { const projectPath = '/root/project'; const dismissEndpoint = '/-/user_callouts'; const showSuggestPopover = false; - const mrReviews = { - a: ['z', 'hash:a'], - b: ['y', 'hash:a'], - }; const diffViewType = 'inline'; return testAction( @@ -130,7 +126,6 @@ describe('legacyDiffs actions', () => { projectPath, dismissEndpoint, showSuggestPopover, - mrReviews, diffViewType, }, { @@ -155,22 +150,9 @@ describe('legacyDiffs actions', () => { projectPath, dismissEndpoint, showSuggestPopover, - mrReviews, diffViewType, }, }, - { - type: store[types.SET_DIFF_FILE_VIEWED], - payload: { id: 'z', seen: true }, - }, - { - type: store[types.SET_DIFF_FILE_VIEWED], - payload: { id: 'a', seen: true }, - }, - { - type: store[types.SET_DIFF_FILE_VIEWED], - payload: { id: 'y', seen: true }, - }, ], [], ); @@ -2205,37 +2187,6 @@ describe('legacyDiffs actions', () => { ); }); - 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', '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 }) => { - store.$patch({ mrReviews: { abc: ['123'] } }); - - store.reviewFile({ - 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(store[types.SET_MR_FILE_REVIEWS]).toHaveBeenCalledWith(reviews); - }, - ); - }); - describe('toggleFileCommentForm', () => { it('commits TOGGLE_FILE_COMMENT_FORM', () => { const file = getDiffFileMock(); diff --git a/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js b/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js index a74b674bcc5297baa2c24f67883664d294e5a594..c3954c03243fe25b5b70b2f610dce0bdb730aed0 100644 --- a/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js +++ b/spec/frontend/diffs/stores/legacy_diffs/mutations_spec.js @@ -990,22 +990,6 @@ describe('DiffsStoreMutations', () => { }); }); - describe('SET_DIFF_FILE_VIEWED', () => { - beforeEach(() => { - store.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 }) => { - store[types.SET_DIFF_FILE_VIEWED]({ id, seen: bool }); - - expect(store.viewedDiffFileIds).toEqual(outcome); - }); - }); - describe('Set highlighted row', () => { it('sets highlighted row', () => { store[types.SET_HIGHLIGHTED_ROW]('ABC_123'); @@ -1270,21 +1254,6 @@ describe('DiffsStoreMutations', () => { }); }); - describe('SET_MR_FILE_REVIEWS', () => { - it.each` - newReviews | oldReviews - ${{ abc: ['123'] }} | ${{}} - ${{ abc: [] }} | ${{ abc: ['123'] }} - ${{}} | ${{ abc: ['123'] }} - `('sets mrReviews to $newReviews', ({ newReviews, oldReviews }) => { - store.$patch({ mrReviews: oldReviews }); - - store[types.SET_MR_FILE_REVIEWS](newReviews); - - expect(store.mrReviews).toStrictEqual(newReviews); - }); - }); - describe('TOGGLE_FILE_COMMENT_FORM', () => { it('toggles diff files hasCommentForm', () => { store.$patch({ diffFiles: [{ file_path: 'path', hasCommentForm: false }] });