From 9222b48d8699a1ae50141fa7a8697a21790d1872 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 5 Mar 2021 18:13:10 -0700 Subject: [PATCH 1/4] Use the Vuex store as the primary source of reviews Previously, the localStorage storage was the authoritative source. Now, the Vuex store is the authoritative source, with the localStorage store being the "backup"/persistent store. That means we need to push the localStorage values into the app, but reserve the `mrReviews` property for the mapped state. We do this by renaming the initial hydrator to "rehydratedMrReviews". --- app/assets/javascripts/diffs/components/app.vue | 5 +++-- app/assets/javascripts/diffs/index.js | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 46ed06fc3ab319..a0fd420c0caa19 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -125,7 +125,7 @@ export default { required: false, default: '', }, - mrReviews: { + rehydratedMrReviews: { type: Object, required: false, default: () => ({}), @@ -164,6 +164,7 @@ export default { 'canMerge', 'hasConflicts', 'viewDiffsFileByFile', + 'mrReviews', ]), ...mapGetters('diffs', ['whichCollapsedTypes', 'isParallelView', 'currentDiffIndex']), ...mapGetters(['isNotesFetched', 'getNoteableData']), @@ -268,7 +269,7 @@ export default { showSuggestPopover: this.showSuggestPopover, viewDiffsFileByFile: fileByFile(this.fileByFileUserPreference), defaultSuggestionCommitMessage: this.defaultSuggestionCommitMessage, - mrReviews: this.mrReviews || {}, + mrReviews: this.rehydratedMrReviews, }); if (this.shouldShow) { diff --git a/app/assets/javascripts/diffs/index.js b/app/assets/javascripts/diffs/index.js index 68fe204d955a50..87e9af174e5ab5 100644 --- a/app/assets/javascripts/diffs/index.js +++ b/app/assets/javascripts/diffs/index.js @@ -124,7 +124,7 @@ export default function initDiffsApp(store) { showSuggestPopover: this.showSuggestPopover, fileByFileUserPreference: this.viewDiffsFileByFile, defaultSuggestionCommitMessage: this.defaultSuggestionCommitMessage, - mrReviews: getReviewsForMergeRequest(mrPath), + rehydratedMrReviews: getReviewsForMergeRequest(mrPath), }, }); }, -- GitLab From 9b9a38144846b6d3a09898750549f97890d4e7e2 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 5 Mar 2021 19:51:25 -0700 Subject: [PATCH 2/4] Use actual file IDs to retrieve reviews It's not very robust to retrieve a value by its position in an array. Instead, because the files all have unique IDs we return an object where each key is a file ID and it's value is whether that file has been marked as reviewed. This fixes the issue where the list of files in single-file mode always has a length of 1, which means the fetched review is always the first file in the MR. --- app/assets/javascripts/diffs/components/app.vue | 2 +- app/assets/javascripts/diffs/utils/file_reviews.js | 7 ++++++- spec/frontend/diffs/utils/file_reviews_spec.js | 10 +++++----- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index a0fd420c0caa19..253e1e3b70e84e 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -514,7 +514,7 @@ export default { v-for="(file, index) in diffs" :key="file.newPath" :file="file" - :reviewed="fileReviews[index]" + :reviewed="fileReviews[file.id]" :is-first-file="index === 0" :is-last-file="index === diffFilesLength - 1" :help-page-path="helpPagePath" diff --git a/app/assets/javascripts/diffs/utils/file_reviews.js b/app/assets/javascripts/diffs/utils/file_reviews.js index 5fafc1714ae75c..7a4b1aa6b17f5b 100644 --- a/app/assets/javascripts/diffs/utils/file_reviews.js +++ b/app/assets/javascripts/diffs/utils/file_reviews.js @@ -9,7 +9,12 @@ export function isFileReviewed(reviews, file) { } export function reviewStatuses(files, reviews) { - return files.map((file) => isFileReviewed(reviews, file)); + return files.reduce((flat, file) => { + return { + ...flat, + [file.id]: isFileReviewed(reviews, file), + }; + }, {}); } export function getReviewsForMergeRequest(mrPath) { diff --git a/spec/frontend/diffs/utils/file_reviews_spec.js b/spec/frontend/diffs/utils/file_reviews_spec.js index a58c19a72455ae..230ec12409c65a 100644 --- a/spec/frontend/diffs/utils/file_reviews_spec.js +++ b/spec/frontend/diffs/utils/file_reviews_spec.js @@ -49,11 +49,11 @@ describe('File Review(s) utilities', () => { it.each` mrReviews | 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'] }} | ${[]} | ${[]} + ${{}} | ${[file1, file2]} | ${{ 123: false, '098': false }} + ${{ abc: ['123'] }} | ${[file1, file2]} | ${{ 123: true, '098': false }} + ${{ abc: ['098'] }} | ${[file1, file2]} | ${{ 123: false, '098': true }} + ${{ def: ['123'] }} | ${[file1, file2]} | ${{ 123: false, '098': false }} + ${{ abc: ['123'], def: ['098'] }} | ${[]} | ${{}} `( 'returns $fileReviews based on the diff files in state and the existing reviews $reviews', ({ mrReviews, files, fileReviews }) => { -- GitLab From f710d4e04295185185d75b34b0fb5e6bc59f238d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 5 Mar 2021 18:57:37 -0700 Subject: [PATCH 3/4] Update the collapse state when the file changes, too When the file loads, it's collapsed automatically if the user has reviewed it already. Unfortunately, this only happens when the component mounts. To make the component reactive - specifically in file-by-file mode - we add the check for the collapse state into a watcher on the file ID. Critically, we leave the `mounted` check in place, too. The first time the component is mounted, it has the initial file already present - the watcher won't trigger on that first render. --- .../javascripts/diffs/components/diff_file.vue | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index f77c8d7406b01e..ca4543f7002b99 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -150,6 +150,11 @@ export default { }, }, watch: { + 'file.id': { + handler: function fileIdHandler() { + this.manageViewedEffects(); + }, + }, 'file.file_hash': { handler: function hashChangeWatch(newHash, oldHash) { this.isCollapsed = isCollapsed(this.file); @@ -186,9 +191,7 @@ export default { this.postRender(); } - if (this.reviewed && !this.isCollapsed && this.showLocalFileReviews) { - this.handleToggle(); - } + this.manageViewedEffects(); }, beforeDestroy() { eventHub.$off(EVT_EXPAND_ALL_FILES, this.expandAllListener); @@ -200,6 +203,11 @@ export default { 'setRenderIt', 'setFileCollapsedByUser', ]), + manageViewedEffects() { + if (this.reviewed && !this.isCollapsed && this.showLocalFileReviews) { + this.handleToggle(); + } + }, expandAllListener() { if (this.isCollapsed) { this.handleToggle(); -- GitLab From 3f329d5621b7a5410bf60de5ecd73021b2f48f82 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 5 Mar 2021 18:31:17 -0700 Subject: [PATCH 4/4] Add changelog for the viewed checkbox fix --- .../tor-defect-file-viewed-reviews-convert-to-vuex.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/tor-defect-file-viewed-reviews-convert-to-vuex.yml diff --git a/changelogs/unreleased/tor-defect-file-viewed-reviews-convert-to-vuex.yml b/changelogs/unreleased/tor-defect-file-viewed-reviews-convert-to-vuex.yml new file mode 100644 index 00000000000000..1d9fc46b875982 --- /dev/null +++ b/changelogs/unreleased/tor-defect-file-viewed-reviews-convert-to-vuex.yml @@ -0,0 +1,5 @@ +--- +title: Fix 'viewed' checkbox in single-file view mode +merge_request: 55922 +author: +type: fixed -- GitLab