diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 32822fe1fe8489e0a38c4a95ffaffb682298cc78..2f20277c6ae3af8688beffb02ffc2bd03d8cd20f 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -26,6 +26,7 @@ import CollapsedFilesWarning from './collapsed_files_warning.vue'; import { diffsApp } from '../utils/performance'; import { fileByFile } from '../utils/preferences'; +import { reviewStatuses } from '../utils/file_reviews'; import { TREE_LIST_WIDTH_STORAGE_KEY, @@ -169,12 +170,7 @@ export default { 'hasConflicts', 'viewDiffsFileByFile', ]), - ...mapGetters('diffs', [ - 'whichCollapsedTypes', - 'isParallelView', - 'currentDiffIndex', - 'fileReviews', - ]), + ...mapGetters('diffs', ['whichCollapsedTypes', 'isParallelView', 'currentDiffIndex']), ...mapGetters(['isNotesFetched', 'getNoteableData']), diffs() { if (!this.viewDiffsFileByFile) { @@ -232,6 +228,9 @@ export default { return visible; }, + fileReviews() { + return reviewStatuses(this.diffFiles, this.mrReviews); + }, }, watch: { commit(newCommit, oldCommit) { diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index e613b684345bb059644014a6e34f928b1a1755dd..f5a3d69843ae61402b442b524c41983eecdbd415 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -10,7 +10,9 @@ import notesEventHub from '../../notes/event_hub'; import DiffFileHeader from './diff_file_header.vue'; import DiffContent from './diff_content.vue'; import { diffViewerErrors } from '~/ide/constants'; + import { collapsedType, isCollapsed } from '../utils/diff_file'; + import { DIFF_FILE_AUTOMATIC_COLLAPSE, DIFF_FILE_MANUAL_COLLAPSE, @@ -144,6 +146,12 @@ export default { showContent() { return !this.isCollapsed && !this.isFileTooLarge; }, + showLocalFileReviews() { + const loggedIn = Boolean(gon.current_user_id); + const featureOn = this.glFeatures.localFileReviews; + + return loggedIn && featureOn; + }, }, watch: { 'file.file_hash': { @@ -181,6 +189,10 @@ export default { if (this.hasDiff) { this.postRender(); } + + if (this.reviewed && !this.isCollapsed && this.showLocalFileReviews) { + this.handleToggle(); + } }, beforeDestroy() { eventHub.$off(EVT_EXPAND_ALL_FILES, this.expandAllListener); @@ -273,9 +285,11 @@ export default { :can-current-user-fork="canCurrentUserFork" :diff-file="file" :collapsible="true" + :reviewed="reviewed" :expanded="!isCollapsed" :add-merge-request-buttons="true" :view-diffs-file-by-file="viewDiffsFileByFile" + :show-local-file-reviews="showLocalFileReviews" class="js-file-title file-title gl-border-1 gl-border-solid gl-border-gray-100" :class="hasBodyClasses.header" @toggleFile="handleToggle" diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue index 53d1383b82e6704190e2248d51734dbcf6f4340d..4fb97df7d6043792fefae568e073794f8328084f 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -1,6 +1,6 @@ @@ -289,6 +334,19 @@ export default { class="file-actions d-flex align-items-center gl-ml-auto gl-align-self-start" > + + + {{ $options.i18n.fileReviewLabel }} + + { ); }; -export function reviewFile({ commit, state, getters }, { file, reviewed = true }) { +export function reviewFile({ commit, state }, { file, reviewed = true }) { const { mrPath } = getDerivedMergeRequestInformation({ endpoint: file.load_collapsed_diff_url }); - const reviews = setReviewsForMergeRequest( - mrPath, - markFileReview(getters.fileReviews(state), file, reviewed), - ); + const reviews = markFileReview(state.mrReviews, file, reviewed); + setReviewsForMergeRequest(mrPath, reviews); 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 a167b6d4694d1b60d75a6ea77f6e05be97043237..f9a466f1899782ce94e92392c4d0054b19b96f8a 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -1,6 +1,5 @@ import { __, n__ } from '~/locale'; import { parallelizeDiffLines } from './utils'; -import { isFileReviewed } from '../utils/file_reviews'; import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE, @@ -155,7 +154,3 @@ 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 aa89c74cef0110ef4fbca4240d30a3b0880d32f5..f93435363ec1071bf957f5b0dd0a7b5409c648f6 100644 --- a/app/assets/javascripts/diffs/store/modules/diff_state.js +++ b/app/assets/javascripts/diffs/store/modules/diff_state.js @@ -47,4 +47,5 @@ export default () => ({ showSuggestPopover: true, defaultSuggestionCommitMessage: '', mrReviews: {}, + latestDiff: true, }); diff --git a/app/assets/javascripts/diffs/utils/file_reviews.js b/app/assets/javascripts/diffs/utils/file_reviews.js index 0047955643aec3df987c026c31d3847ad8f79119..5fafc1714ae75caede2a58bfcc2d4b748dc40d6a 100644 --- a/app/assets/javascripts/diffs/utils/file_reviews.js +++ b/app/assets/javascripts/diffs/utils/file_reviews.js @@ -2,6 +2,16 @@ function getFileReviewsKey(mrPath) { return `${mrPath}-file-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 reviewStatuses(files, reviews) { + return files.map((file) => isFileReviewed(reviews, file)); +} + export function getReviewsForMergeRequest(mrPath) { const reviewsForMr = localStorage.getItem(getFileReviewsKey(mrPath)); let reviews = {}; @@ -23,23 +33,17 @@ export function setReviewsForMergeRequest(mrPath, 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; + const updatedReviews = usableReviews; let fileReviews; if (reviewable(file)) { - fileReviews = new Set([...(usableReviews[file.file_identifier_hash] || [])]); + fileReviews = new Set(usableReviews[file.file_identifier_hash] || []); if (reviewed) { fileReviews.add(file.id); @@ -47,10 +51,7 @@ export function markFileReview(reviews, file, reviewed = true) { fileReviews.delete(file.id); } - updatedReviews = { - ...usableReviews, - [file.file_identifier_hash]: Array.from(fileReviews), - }; + updatedReviews[file.file_identifier_hash] = Array.from(fileReviews); if (updatedReviews[file.file_identifier_hash].length === 0) { delete updatedReviews[file.file_identifier_hash]; diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index d452a5e02e2ccafdaf93834ac9c3a58d63741c76..98d002c9664af7cfbd3683c60de3e0a112f91af6 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -43,6 +43,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo push_frontend_feature_flag(:diffs_gradual_load, @project, default_enabled: true) push_frontend_feature_flag(:codequality_mr_diff, @project) push_frontend_feature_flag(:suggestions_custom_commit, @project) + push_frontend_feature_flag(:local_file_reviews, default_enabled: :yaml) record_experiment_user(:invite_members_version_a) record_experiment_user(:invite_members_version_b) diff --git a/changelogs/unreleased/feature-file-review.yml b/changelogs/unreleased/feature-file-review.yml new file mode 100644 index 0000000000000000000000000000000000000000..2b41cc0d5ac7d0ddffbfde9e0ad91408873f9e66 --- /dev/null +++ b/changelogs/unreleased/feature-file-review.yml @@ -0,0 +1,5 @@ +--- +title: Mark files as reviewed locally +merge_request: 51513 +author: +type: added diff --git a/config/feature_flags/development/local_file_reviews.yml b/config/feature_flags/development/local_file_reviews.yml new file mode 100644 index 0000000000000000000000000000000000000000..8b62b497c1cfe8486d7e5c8c9007f8ae63ba3827 --- /dev/null +++ b/config/feature_flags/development/local_file_reviews.yml @@ -0,0 +1,8 @@ +--- +name: local_file_reviews +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48976 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/296674 +milestone: '13.8' +type: development +group: group::code review +default_enabled: false diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 77fb16512958103eb283097074dd6e24d6b2eee4..43d5a134f1ace8852d4729c733c1d3edc55ed127 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -7078,6 +7078,9 @@ msgstr "" msgid "Collapse sidebar" msgstr "" +msgid "Collapses this file (only for you) until it’s changed again." +msgstr "" + msgid "Collector hostname" msgstr "" @@ -31335,6 +31338,9 @@ msgstr "" msgid "View users statistics" msgstr "" +msgid "Viewed" +msgstr "" + msgid "Viewing commit" msgstr "" diff --git a/spec/frontend/diffs/components/diff_file_header_spec.js b/spec/frontend/diffs/components/diff_file_header_spec.js index e9a63e861edc753b69fe432161adf51ff1c5b3aa..8487a3ea8184d01ee062332088957407c98f1c19 100644 --- a/spec/frontend/diffs/components/diff_file_header_spec.js +++ b/spec/frontend/diffs/components/diff_file_header_spec.js @@ -13,10 +13,18 @@ import { diffViewerModes } from '~/ide/constants'; import { __, sprintf } from '~/locale'; import { scrollToElement } from '~/lib/utils/common_utils'; +import testAction from '../../__helpers__/vuex_action_helper'; + +import { SET_MR_FILE_REVIEWS } from '~/diffs/store/mutation_types'; +import { reviewFile } from '~/diffs/store/actions'; +import { DIFF_FILE_AUTOMATIC_COLLAPSE, DIFF_FILE_MANUAL_COLLAPSE } from '~/diffs/constants'; + jest.mock('~/lib/utils/common_utils'); const diffFile = Object.freeze( Object.assign(diffDiscussionsMockData.diff_file, { + id: '123', + file_identifier_hash: 'abc', edit_path: 'link:/to/edit/path', blob: { id: '848ed9407c6730ff16edb3dd24485a0eea24292a', @@ -52,6 +60,8 @@ describe('DiffFileHeader component', () => { toggleFileDiscussionWrappers: jest.fn(), toggleFullDiff: jest.fn(), toggleActiveFileByHash: jest.fn(), + setFileCollapsedByUser: jest.fn(), + reviewFile: jest.fn(), }, }, }, @@ -79,10 +89,11 @@ describe('DiffFileHeader component', () => { const findViewFileButton = () => wrapper.find({ ref: 'viewButton' }); const findCollapseIcon = () => wrapper.find({ ref: 'collapseIcon' }); const findEditButton = () => wrapper.find({ ref: 'editButton' }); + const findReviewFileCheckbox = () => wrapper.find("[data-testid='fileReviewCheckbox']"); - const createComponent = (props) => { + const createComponent = ({ props, options = {} } = {}) => { mockStoreConfig = cloneDeep(defaultMockStoreConfig); - const store = new Vuex.Store(mockStoreConfig); + const store = new Vuex.Store({ ...mockStoreConfig, ...(options.store || {}) }); wrapper = shallowMount(DiffFileHeader, { propsData: { @@ -91,6 +102,7 @@ describe('DiffFileHeader component', () => { viewDiffsFileByFile: false, ...props, }, + ...options, localVue, store, }); @@ -101,7 +113,7 @@ describe('DiffFileHeader component', () => { ${'visible'} | ${true} ${'hidden'} | ${false} `('collapse toggle is $visibility if collapsible is $collapsible', ({ collapsible }) => { - createComponent({ collapsible }); + createComponent({ props: { collapsible } }); expect(findCollapseIcon().exists()).toBe(collapsible); }); @@ -110,7 +122,7 @@ describe('DiffFileHeader component', () => { ${true} | ${'chevron-down'} ${false} | ${'chevron-right'} `('collapse icon is $icon if expanded is $expanded', ({ icon, expanded }) => { - createComponent({ expanded, collapsible: true }); + createComponent({ props: { expanded, collapsible: true } }); expect(findCollapseIcon().props('name')).toBe(icon); }); @@ -124,7 +136,7 @@ describe('DiffFileHeader component', () => { }); it('when collapseIcon is clicked emits toggleFile', () => { - createComponent({ collapsible: true }); + createComponent({ props: { collapsible: true } }); findCollapseIcon().vm.$emit('click', new Event('click')); return wrapper.vm.$nextTick().then(() => { expect(wrapper.emitted().toggleFile).toBeDefined(); @@ -132,7 +144,7 @@ describe('DiffFileHeader component', () => { }); it('when other element in header is clicked does not emits toggleFile', () => { - createComponent({ collapsible: true }); + createComponent({ props: { collapsible: true } }); findTitleLink().trigger('click'); return wrapper.vm.$nextTick().then(() => { @@ -171,10 +183,12 @@ describe('DiffFileHeader component', () => { it('prefers submodule_tree_url over submodule_link for href', () => { const submoduleTreeUrl = 'some://tree/url'; createComponent({ - discussionLink: 'discussionLink', - diffFile: { - ...submoduleDiffFile, - submodule_tree_url: 'some://tree/url', + props: { + discussionLink: 'discussionLink', + diffFile: { + ...submoduleDiffFile, + submodule_tree_url: 'some://tree/url', + }, }, }); @@ -184,8 +198,10 @@ describe('DiffFileHeader component', () => { it('uses submodule_link for href if submodule_tree_url does not exists', () => { const submoduleLink = 'link://to/submodule'; createComponent({ - discussionLink: 'discussionLink', - diffFile: submoduleDiffFile, + props: { + discussionLink: 'discussionLink', + diffFile: submoduleDiffFile, + }, }); expect(findTitleLink().attributes('href')).toBe(submoduleLink); @@ -193,7 +209,9 @@ describe('DiffFileHeader component', () => { it('uses file_path + SHA as link text', () => { createComponent({ - diffFile: submoduleDiffFile, + props: { + diffFile: submoduleDiffFile, + }, }); expect(findTitleLink().text()).toContain( @@ -203,15 +221,19 @@ describe('DiffFileHeader component', () => { it('does not render file actions', () => { createComponent({ - diffFile: submoduleDiffFile, - addMergeRequestButtons: true, + props: { + diffFile: submoduleDiffFile, + addMergeRequestButtons: true, + }, }); expect(findFileActions().exists()).toBe(false); }); it('renders submodule icon', () => { createComponent({ - diffFile: submoduleDiffFile, + props: { + diffFile: submoduleDiffFile, + }, }); expect(wrapper.find(FileIcon).props('submodule')).toBe(true); @@ -223,13 +245,15 @@ describe('DiffFileHeader component', () => { it('for mode_changed file mode displays mode changes', () => { createComponent({ - diffFile: { - ...diffFile, - a_mode: 'old-mode', - b_mode: 'new-mode', - viewer: { - ...diffFile.viewer, - name: diffViewerModes.mode_changed, + props: { + diffFile: { + ...diffFile, + a_mode: 'old-mode', + b_mode: 'new-mode', + viewer: { + ...diffFile.viewer, + name: diffViewerModes.mode_changed, + }, }, }, }); @@ -240,13 +264,15 @@ describe('DiffFileHeader component', () => { 'for %s file mode does not display mode changes', (mode) => { createComponent({ - diffFile: { - ...diffFile, - a_mode: 'old-mode', - b_mode: 'new-mode', - viewer: { - ...diffFile.viewer, - name: diffViewerModes[mode], + props: { + diffFile: { + ...diffFile, + a_mode: 'old-mode', + b_mode: 'new-mode', + viewer: { + ...diffFile.viewer, + name: diffViewerModes[mode], + }, }, }, }); @@ -256,32 +282,38 @@ describe('DiffFileHeader component', () => { it('displays the LFS label for files stored in LFS', () => { createComponent({ - diffFile: { ...diffFile, stored_externally: true, external_storage: 'lfs' }, + props: { + diffFile: { ...diffFile, stored_externally: true, external_storage: 'lfs' }, + }, }); expect(findLfsLabel().exists()).toBe(true); }); it('does not display the LFS label for files stored in repository', () => { createComponent({ - diffFile: { ...diffFile, stored_externally: false }, + props: { + diffFile: { ...diffFile, stored_externally: false }, + }, }); expect(findLfsLabel().exists()).toBe(false); }); it('does not render view replaced file button if no replaced view path is present', () => { createComponent({ - diffFile: { ...diffFile, replaced_view_path: null }, + props: { + diffFile: { ...diffFile, replaced_view_path: null }, + }, }); expect(findReplacedFileButton().exists()).toBe(false); }); describe('when addMergeRequestButtons is false', () => { it('does not render file actions', () => { - createComponent({ addMergeRequestButtons: false }); + createComponent({ props: { addMergeRequestButtons: false } }); expect(findFileActions().exists()).toBe(false); }); it('should not render edit button', () => { - createComponent({ addMergeRequestButtons: false }); + createComponent({ props: { addMergeRequestButtons: false } }); expect(findEditButton().exists()).toBe(false); }); }); @@ -290,7 +322,7 @@ describe('DiffFileHeader component', () => { describe('without discussions', () => { it('does not render a toggle discussions button', () => { diffHasDiscussionsResultMock.mockReturnValue(false); - createComponent({ addMergeRequestButtons: true }); + createComponent({ props: { addMergeRequestButtons: true } }); expect(findToggleDiscussionsButton().exists()).toBe(false); }); }); @@ -298,7 +330,7 @@ describe('DiffFileHeader component', () => { describe('with discussions', () => { it('dispatches toggleFileDiscussionWrappers when user clicks on toggle discussions button', () => { diffHasDiscussionsResultMock.mockReturnValue(true); - createComponent({ addMergeRequestButtons: true }); + createComponent({ props: { addMergeRequestButtons: true } }); expect(findToggleDiscussionsButton().exists()).toBe(true); findToggleDiscussionsButton().vm.$emit('click'); expect( @@ -309,7 +341,9 @@ describe('DiffFileHeader component', () => { it('should show edit button', () => { createComponent({ - addMergeRequestButtons: true, + props: { + addMergeRequestButtons: true, + }, }); expect(findEditButton().exists()).toBe(true); }); @@ -319,25 +353,27 @@ describe('DiffFileHeader component', () => { const externalUrl = 'link://to/external'; const formattedExternalUrl = 'link://formatted'; createComponent({ - diffFile: { - ...diffFile, - external_url: externalUrl, - formatted_external_url: formattedExternalUrl, + props: { + diffFile: { + ...diffFile, + external_url: externalUrl, + formatted_external_url: formattedExternalUrl, + }, + addMergeRequestButtons: true, }, - addMergeRequestButtons: true, }); expect(findExternalLink().exists()).toBe(true); }); it('is hidden by default', () => { - createComponent({ addMergeRequestButtons: true }); + createComponent({ props: { addMergeRequestButtons: true } }); expect(findExternalLink().exists()).toBe(false); }); }); describe('without file blob', () => { beforeEach(() => { - createComponent({ diffFile: { ...diffFile, blob: false } }); + createComponent({ props: { diffFile: { ...diffFile, blob: false } } }); }); it('should not render toggle discussions button', () => { @@ -352,8 +388,10 @@ describe('DiffFileHeader component', () => { it('should render correct file view button', () => { const viewPath = 'link://view-path'; createComponent({ - diffFile: { ...diffFile, view_path: viewPath }, - addMergeRequestButtons: true, + props: { + diffFile: { ...diffFile, view_path: viewPath }, + addMergeRequestButtons: true, + }, }); expect(findViewFileButton().attributes('href')).toBe(viewPath); expect(findViewFileButton().text()).toEqual( @@ -367,9 +405,11 @@ describe('DiffFileHeader component', () => { describe('when diff is fully expanded', () => { it('is not rendered', () => { createComponent({ - diffFile: { - ...diffFile, - is_fully_expanded: true, + props: { + diffFile: { + ...diffFile, + is_fully_expanded: true, + }, }, }); expect(findExpandButton().exists()).toBe(false); @@ -387,17 +427,17 @@ describe('DiffFileHeader component', () => { }; it('renders expand to full file button if not showing full file already', () => { - createComponent(fullyNotExpandedFileProps); + createComponent({ props: fullyNotExpandedFileProps }); expect(findExpandButton().exists()).toBe(true); }); it('renders loading icon when loading full file', () => { - createComponent(fullyNotExpandedFileProps); + createComponent({ props: fullyNotExpandedFileProps }); expect(findExpandButton().exists()).toBe(true); }); it('toggles full diff on click', () => { - createComponent(fullyNotExpandedFileProps); + createComponent({ props: fullyNotExpandedFileProps }); findExpandButton().vm.$emit('click'); expect(mockStoreConfig.modules.diffs.actions.toggleFullDiff).toHaveBeenCalled(); }); @@ -407,7 +447,9 @@ describe('DiffFileHeader component', () => { it('uses discussionPath for link if it is defined', () => { const discussionPath = 'link://to/discussion'; createComponent({ - discussionPath, + props: { + discussionPath, + }, }); expect(findTitleLink().attributes('href')).toBe(discussionPath); }); @@ -436,21 +478,21 @@ describe('DiffFileHeader component', () => { describe('for new file', () => { it('displays the path', () => { - createComponent({ diffFile: { ...diffFile, new_file: true } }); + createComponent({ props: { diffFile: { ...diffFile, new_file: true } } }); expect(findTitleLink().text()).toBe(diffFile.file_path); }); }); describe('for deleted file', () => { it('displays the path', () => { - createComponent({ diffFile: { ...diffFile, deleted_file: true } }); + createComponent({ props: { diffFile: { ...diffFile, deleted_file: true } } }); expect(findTitleLink().text()).toBe( sprintf(__('%{filePath} deleted'), { filePath: diffFile.file_path }, false), ); }); it('does not show edit button', () => { - createComponent({ diffFile: { ...diffFile, deleted_file: true } }); + createComponent({ props: { diffFile: { ...diffFile, deleted_file: true } } }); expect(findEditButton().exists()).toBe(false); }); }); @@ -458,11 +500,13 @@ describe('DiffFileHeader component', () => { describe('for renamed file', () => { it('displays old and new path if the file was renamed', () => { createComponent({ - diffFile: { - ...diffFile, - renamed_file: true, - old_path_html: 'old', - new_path_html: 'new', + props: { + diffFile: { + ...diffFile, + renamed_file: true, + old_path_html: 'old', + new_path_html: 'new', + }, }, }); expect(findTitleLink().text()).toMatch(/^old.+new/s); @@ -473,13 +517,132 @@ describe('DiffFileHeader component', () => { it('renders view replaced file button', () => { const replacedViewPath = 'some/path'; createComponent({ - diffFile: { - ...diffFile, - replaced_view_path: replacedViewPath, + props: { + diffFile: { + ...diffFile, + replaced_view_path: replacedViewPath, + }, + addMergeRequestButtons: true, }, - addMergeRequestButtons: true, }); expect(findReplacedFileButton().exists()).toBe(true); }); }); + + describe('file reviews', () => { + it('calls the action to set the new review', () => { + createComponent({ + props: { + diffFile: { + ...diffFile, + viewer: { + ...diffFile.viewer, + automaticallyCollapsed: false, + manuallyCollapsed: null, + }, + }, + showLocalFileReviews: true, + addMergeRequestButtons: true, + }, + }); + + const file = wrapper.vm.diffFile; + + findReviewFileCheckbox().vm.$emit('change', true); + + return testAction( + reviewFile, + { file, reviewed: true }, + {}, + [{ type: SET_MR_FILE_REVIEWS, payload: { [file.file_identifier_hash]: [file.id] } }], + [], + ); + }); + + it.each` + description | newReviewedStatus | collapseType | aCollapse | mCollapse | callAction + ${'does nothing'} | ${true} | ${DIFF_FILE_MANUAL_COLLAPSE} | ${false} | ${true} | ${false} + ${'does nothing'} | ${false} | ${DIFF_FILE_AUTOMATIC_COLLAPSE} | ${true} | ${null} | ${false} + ${'does nothing'} | ${true} | ${'not collapsed'} | ${false} | ${null} | ${false} + ${'does nothing'} | ${false} | ${'not collapsed'} | ${false} | ${null} | ${false} + ${'collapses the file'} | ${true} | ${DIFF_FILE_AUTOMATIC_COLLAPSE} | ${true} | ${null} | ${true} + `( + "$description if the new review status is reviewed = $newReviewedStatus and the file's collapse type is collapse = $collapseType", + ({ newReviewedStatus, aCollapse, mCollapse, callAction }) => { + createComponent({ + props: { + diffFile: { + ...diffFile, + viewer: { + ...diffFile.viewer, + automaticallyCollapsed: aCollapse, + manuallyCollapsed: mCollapse, + }, + }, + showLocalFileReviews: true, + addMergeRequestButtons: true, + }, + }); + + findReviewFileCheckbox().vm.$emit('change', newReviewedStatus); + + if (callAction) { + expect(mockStoreConfig.modules.diffs.actions.setFileCollapsedByUser).toHaveBeenCalled(); + } else { + expect( + mockStoreConfig.modules.diffs.actions.setFileCollapsedByUser, + ).not.toHaveBeenCalled(); + } + }, + ); + + it.each` + description | show | visible + ${'shows'} | ${true} | ${true} + ${'hides'} | ${false} | ${false} + `( + '$description the file review feature given { showLocalFileReviewsProp: $show }', + ({ show, visible }) => { + createComponent({ + props: { + showLocalFileReviews: show, + addMergeRequestButtons: true, + }, + }); + + expect(findReviewFileCheckbox().exists()).toEqual(visible); + }, + ); + + it.each` + open | status | fires + ${true} | ${true} | ${true} + ${false} | ${false} | ${true} + ${true} | ${false} | ${false} + ${false} | ${true} | ${false} + `( + 'toggles appropriately when { fileExpanded: $open, newReviewStatus: $status }', + ({ open, status, fires }) => { + createComponent({ + props: { + diffFile: { + ...diffFile, + viewer: { + ...diffFile.viewer, + automaticallyCollapsed: false, + manuallyCollapsed: null, + }, + }, + showLocalFileReviews: true, + addMergeRequestButtons: true, + expanded: open, + }, + }); + + findReviewFileCheckbox().vm.$emit('change', status); + + expect(Boolean(wrapper.emitted().toggleFile)).toBe(fires); + }, + ); + }); }); diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js index c715d7799868b7f62deef91b4d70dc890897d232..57f6e50e68ae0f26994423c679c6b75c3306874a 100644 --- a/spec/frontend/diffs/components/diff_file_spec.js +++ b/spec/frontend/diffs/components/diff_file_spec.js @@ -66,7 +66,7 @@ function markFileToBeRendered(store, index = 0) { }); } -function createComponent({ file, first = false, last = false }) { +function createComponent({ file, first = false, last = false, options = {}, props = {} }) { const localVue = createLocalVue(); localVue.use(Vuex); @@ -89,7 +89,9 @@ function createComponent({ file, first = false, last = false }) { viewDiffsFileByFile: false, isFirstFile: first, isLastFile: last, + ...props, }, + ...options, }); return { @@ -220,6 +222,53 @@ describe('DiffFile', () => { }); }); + describe('computed', () => { + describe('showLocalFileReviews', () => { + let gon; + + function setLoggedIn(bool) { + window.gon.current_user_id = bool; + } + + beforeAll(() => { + gon = window.gon; + window.gon = {}; + }); + + afterEach(() => { + window.gon = gon; + }); + + it.each` + loggedIn | featureOn | bool + ${true} | ${true} | ${true} + ${false} | ${true} | ${false} + ${true} | ${false} | ${false} + ${false} | ${false} | ${false} + `( + 'should be $bool when { userIsLoggedIn: $loggedIn, featureEnabled: $featureOn }', + ({ loggedIn, featureOn, bool }) => { + setLoggedIn(loggedIn); + + ({ wrapper } = createComponent({ + options: { + provide: { + glFeatures: { + localFileReviews: featureOn, + }, + }, + }, + props: { + file: store.state.diffs.diffFiles[0], + }, + })); + + expect(wrapper.vm.showLocalFileReviews).toBe(bool); + }, + ); + }); + }); + describe('collapsing', () => { describe(`\`${EVT_EXPAND_ALL_FILES}\` event`, () => { beforeEach(() => { diff --git a/spec/frontend/diffs/store/getters_spec.js b/spec/frontend/diffs/store/getters_spec.js index 4d7f861ac22472fabd3cee8e5bfde4732822bb88..85dc52f8bf70f20c807a28cd0bf92d36526afa5d 100644 --- a/spec/frontend/diffs/store/getters_spec.js +++ b/spec/frontend/diffs/store/getters_spec.js @@ -375,26 +375,4 @@ 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/utils/file_reviews_spec.js b/spec/frontend/diffs/utils/file_reviews_spec.js index 819426ee75f6f92b98046a75c372e456cbb3a22d..a58c19a72455ae963643af2dc10e5ee1b502cfd5 100644 --- a/spec/frontend/diffs/utils/file_reviews_spec.js +++ b/spec/frontend/diffs/utils/file_reviews_spec.js @@ -5,6 +5,7 @@ import { setReviewsForMergeRequest, isFileReviewed, markFileReview, + reviewStatuses, reviewable, } from '~/diffs/utils/file_reviews'; @@ -28,6 +29,39 @@ describe('File Review(s) utilities', () => { localStorage.clear(); }); + 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('reviewStatuses', () => { + const file1 = { id: '123', file_identifier_hash: 'abc' }; + const file2 = { id: '098', file_identifier_hash: 'abc' }; + + 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'] }} | ${[]} | ${[]} + `( + 'returns $fileReviews based on the diff files in state and the existing reviews $reviews', + ({ mrReviews, files, fileReviews }) => { + expect(reviewStatuses(files, mrReviews)).toStrictEqual(fileReviews); + }, + ); + }); + describe('getReviewsForMergeRequest', () => { it('fetches the appropriate stored reviews from localStorage', () => { getReviewsForMergeRequest(mrPath); @@ -73,20 +107,6 @@ describe('File Review(s) utilities', () => { }); }); - 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