diff options
| -rw-r--r-- | app/assets/javascripts/diffs/components/diff_file.vue | 4 | ||||
| -rw-r--r-- | app/assets/javascripts/diffs/components/diff_file_header.vue | 26 | ||||
| -rw-r--r-- | app/assets/javascripts/diffs/store/actions.js | 27 | ||||
| -rw-r--r-- | app/assets/javascripts/diffs/store/getters.js | 50 | ||||
| -rw-r--r-- | app/assets/javascripts/notes/stores/actions.js | 2 | ||||
| -rw-r--r-- | app/assets/javascripts/notes/stores/mutation_types.js | 7 | ||||
| -rw-r--r-- | app/assets/javascripts/notes/stores/mutations.js | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/48237-toggle-file-comments.yml | 5 | ||||
| -rw-r--r-- | spec/features/projects/merge_requests/user_comments_on_diff_spec.rb | 6 | ||||
| -rw-r--r-- | spec/javascripts/diffs/components/diff_file_header_spec.js | 144 | ||||
| -rw-r--r-- | spec/javascripts/diffs/store/actions_spec.js | 44 | ||||
| -rw-r--r-- | spec/javascripts/diffs/store/getters_spec.js | 123 | ||||
| -rw-r--r-- | spec/javascripts/notes/stores/actions_spec.js | 13 | ||||
| -rw-r--r-- | spec/javascripts/notes/stores/mutation_spec.js | 14 |
14 files changed, 378 insertions, 92 deletions
diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index 060386c3ecb..a61e368249a 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -31,9 +31,6 @@ export default { }; }, computed: { - isDiscussionsExpanded() { - return true; // TODO: @fatihacet - Fix this. - }, isCollapsed() { return this.file.collapsed || false; }, @@ -131,7 +128,6 @@ export default { :diff-file="file" :collapsible="true" :expanded="!isCollapsed" - :discussions-expanded="isDiscussionsExpanded" :add-merge-request-buttons="true" class="js-file-title file-title" @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 1957698c6c1..c5abd0a9568 100644 --- a/app/assets/javascripts/diffs/components/diff_file_header.vue +++ b/app/assets/javascripts/diffs/components/diff_file_header.vue @@ -1,5 +1,6 @@ <script> import _ from 'underscore'; +import { mapActions, mapGetters } from 'vuex'; import ClipboardButton from '~/vue_shared/components/clipboard_button.vue'; import Icon from '~/vue_shared/components/icon.vue'; import FileIcon from '~/vue_shared/components/file_icon.vue'; @@ -38,11 +39,6 @@ export default { required: false, default: true, }, - discussionsExpanded: { - type: Boolean, - required: false, - default: true, - }, currentUser: { type: Object, required: true, @@ -54,6 +50,10 @@ export default { }; }, computed: { + ...mapGetters('diffs', ['diffHasExpandedDiscussions']), + hasExpandedDiscussions() { + return this.diffHasExpandedDiscussions(this.diffFile); + }, icon() { if (this.diffFile.submodule) { return 'archive'; @@ -88,9 +88,6 @@ export default { collapseIcon() { return this.expanded ? 'chevron-down' : 'chevron-right'; }, - isDiscussionsExpanded() { - return this.discussionsExpanded && this.expanded; - }, viewFileButtonText() { const truncatedContentSha = _.escape(truncateSha(this.diffFile.contentSha)); return sprintf( @@ -113,7 +110,8 @@ export default { }, }, methods: { - handleToggle(e, checkTarget) { + ...mapActions('diffs', ['toggleFileDiscussions']), + handleToggleFile(e, checkTarget) { if ( !checkTarget || e.target === this.$refs.header || @@ -125,6 +123,9 @@ export default { showForkMessage() { this.$emit('showForkMessage'); }, + handleToggleDiscussions() { + this.toggleFileDiscussions(this.diffFile); + }, }, }; </script> @@ -133,7 +134,7 @@ export default { <div ref="header" class="js-file-title file-title file-title-flex-parent" - @click="handleToggle($event, true)" + @click="handleToggleFile($event, true)" > <div class="file-header-content"> <icon @@ -216,10 +217,11 @@ export default { v-if="diffFile.blob && diffFile.blob.readableText" > <button - :class="{ active: isDiscussionsExpanded }" + :class="{ active: hasExpandedDiscussions }" :title="s__('MergeRequests|Toggle comments for this file')" - class="btn js-toggle-diff-comments" + class="js-btn-vue-toggle-comments btn" type="button" + @click="handleToggleDiscussions" > <icon name="comment" /> </button> diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 5bfe42618c2..27001142257 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -82,5 +82,32 @@ export const expandAllFiles = ({ commit }) => { commit(types.EXPAND_ALL_FILES); }; +/** + * Toggles the file discussions after user clicked on the toggle discussions button. + * + * Gets the discussions for the provided diff. + * + * If all discussions are expanded, it will collapse all of them + * If all discussions are collapsed, it will expand all of them + * If some discussions are open and others closed, it will expand the closed ones. + * + * @param {Object} diff + */ +export const toggleFileDiscussions = ({ getters, dispatch }, diff) => { + const discussions = getters.getDiffFileDiscussions(diff); + const shouldCloseAll = getters.diffHasAllExpandedDiscussions(diff); + const shouldExpandAll = getters.diffHasAllCollpasedDiscussions(diff); + + discussions.forEach(discussion => { + const data = { discussionId: discussion.id }; + + if (shouldCloseAll) { + dispatch('collapseDiscussion', data, { root: true }); + } else if (shouldExpandAll || (!shouldCloseAll && !shouldExpandAll && !discussion.expanded)) { + dispatch('expandDiscussion', data, { root: true }); + } + }); +}; + // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index f3c2d7427e7..f89acb73ed8 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -1,3 +1,4 @@ +import _ from 'underscore'; import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '../constants'; export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW_TYPE; @@ -8,5 +9,52 @@ export const areAllFilesCollapsed = state => state.diffFiles.every(file => file. export const commitId = state => (state.commit && state.commit.id ? state.commit.id : null); -// prevent babel-plugin-rewire from generating an invalid default during karma tests +/** + * Checks if the diff has all discussions expanded + * @param {Object} diff + * @returns {Boolean} + */ +export const diffHasAllExpandedDiscussions = (state, getters) => diff => { + const discussions = getters.getDiffFileDiscussions(diff); + + return (discussions.length && discussions.every(discussion => discussion.expanded)) || false; +}; + +/** + * Checks if the diff has all discussions collpased + * @param {Object} diff + * @returns {Boolean} + */ +export const diffHasAllCollpasedDiscussions = (state, getters) => diff => { + const discussions = getters.getDiffFileDiscussions(diff); + + return (discussions.length && discussions.every(discussion => !discussion.expanded)) || false; +}; + +/** + * Checks if the diff has any open discussions + * @param {Object} diff + * @returns {Boolean} + */ +export const diffHasExpandedDiscussions = (state, getters) => diff => { + const discussions = getters.getDiffFileDiscussions(diff); + + return ( + (discussions.length && discussions.find(discussion => discussion.expanded) !== undefined) || + false + ); +}; + +/** + * Returns an array with the discussions of the given diff + * @param {Object} diff + * @returns {Array} + */ +export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) => diff => + rootGetters.discussions.filter( + discussion => + discussion.diff_discussion && _.isEqual(discussion.diff_file.file_hash, diff.fileHash), + ) || []; + +// prevent babel-plugin-rewire from generating an invalid default during karma∂ tests export default () => {}; diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index b2bf86eea56..3eefbe11c37 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -15,6 +15,8 @@ let eTagPoll; export const expandDiscussion = ({ commit }, data) => commit(types.EXPAND_DISCUSSION, data); +export const collapseDiscussion = ({ commit }, data) => commit(types.COLLAPSE_DISCUSSION, data); + export const setNotesData = ({ commit }, data) => commit(types.SET_NOTES_DATA, data); export const setNoteableData = ({ commit }, data) => commit(types.SET_NOTEABLE_DATA, data); diff --git a/app/assets/javascripts/notes/stores/mutation_types.js b/app/assets/javascripts/notes/stores/mutation_types.js index a25098fbc06..6f374f78691 100644 --- a/app/assets/javascripts/notes/stores/mutation_types.js +++ b/app/assets/javascripts/notes/stores/mutation_types.js @@ -1,7 +1,6 @@ export const ADD_NEW_NOTE = 'ADD_NEW_NOTE'; export const ADD_NEW_REPLY_TO_DISCUSSION = 'ADD_NEW_REPLY_TO_DISCUSSION'; export const DELETE_NOTE = 'DELETE_NOTE'; -export const EXPAND_DISCUSSION = 'EXPAND_DISCUSSION'; export const REMOVE_PLACEHOLDER_NOTES = 'REMOVE_PLACEHOLDER_NOTES'; export const SET_NOTES_DATA = 'SET_NOTES_DATA'; export const SET_NOTEABLE_DATA = 'SET_NOTEABLE_DATA'; @@ -11,12 +10,16 @@ export const SET_LAST_FETCHED_AT = 'SET_LAST_FETCHED_AT'; export const SET_TARGET_NOTE_HASH = 'SET_TARGET_NOTE_HASH'; export const SHOW_PLACEHOLDER_NOTE = 'SHOW_PLACEHOLDER_NOTE'; export const TOGGLE_AWARD = 'TOGGLE_AWARD'; -export const TOGGLE_DISCUSSION = 'TOGGLE_DISCUSSION'; export const UPDATE_NOTE = 'UPDATE_NOTE'; export const UPDATE_DISCUSSION = 'UPDATE_DISCUSSION'; export const SET_DISCUSSION_DIFF_LINES = 'SET_DISCUSSION_DIFF_LINES'; export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE'; +// DISCUSSION +export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION'; +export const EXPAND_DISCUSSION = 'EXPAND_DISCUSSION'; +export const TOGGLE_DISCUSSION = 'TOGGLE_DISCUSSION'; + // Issue export const CLOSE_ISSUE = 'CLOSE_ISSUE'; export const REOPEN_ISSUE = 'REOPEN_ISSUE'; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index a1849269010..ab6a95e2601 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -58,6 +58,11 @@ export default { discussion.expanded = true; }, + [types.COLLAPSE_DISCUSSION](state, { discussionId }) { + const discussion = utils.findNoteObjectById(state.discussions, discussionId); + discussion.expanded = false; + }, + [types.REMOVE_PLACEHOLDER_NOTES](state) { const { discussions } = state; diff --git a/changelogs/unreleased/48237-toggle-file-comments.yml b/changelogs/unreleased/48237-toggle-file-comments.yml new file mode 100644 index 00000000000..2e893aad0b2 --- /dev/null +++ b/changelogs/unreleased/48237-toggle-file-comments.yml @@ -0,0 +1,5 @@ +--- +title: Fixes toggle discussion button not expanding collapsed discussions +merge_request: 20452 +author: +type: fixed diff --git a/spec/features/projects/merge_requests/user_comments_on_diff_spec.rb b/spec/features/projects/merge_requests/user_comments_on_diff_spec.rb index b1b62d04ac2..441b080bee5 100644 --- a/spec/features/projects/merge_requests/user_comments_on_diff_spec.rb +++ b/spec/features/projects/merge_requests/user_comments_on_diff_spec.rb @@ -31,7 +31,7 @@ describe 'User comments on a diff', :js do page.within('.files > div:nth-child(3)') do expect(page).to have_content('Line is wrong') - find('.js-toggle-diff-comments').click + find('.js-btn-vue-toggle-comments').click expect(page).not_to have_content('Line is wrong') end @@ -64,7 +64,7 @@ describe 'User comments on a diff', :js do # Hide the comment. page.within('.files > div:nth-child(3)') do - find('.js-toggle-diff-comments').click + find('.js-btn-vue-toggle-comments').click expect(page).not_to have_content('Line is wrong') end @@ -77,7 +77,7 @@ describe 'User comments on a diff', :js do # Show the comment. page.within('.files > div:nth-child(3)') do - find('.js-toggle-diff-comments').click + find('.js-btn-vue-toggle-comments').click end # Now both the comments should be shown. diff --git a/spec/javascripts/diffs/components/diff_file_header_spec.js b/spec/javascripts/diffs/components/diff_file_header_spec.js index 05f5d47ce42..0f3a95da5bf 100644 --- a/spec/javascripts/diffs/components/diff_file_header_spec.js +++ b/spec/javascripts/diffs/components/diff_file_header_spec.js @@ -1,7 +1,10 @@ import Vue from 'vue'; +import Vuex from 'vuex'; +import diffsModule from '~/diffs/store/modules'; +import notesModule from '~/notes/stores/modules'; import DiffFileHeader from '~/diffs/components/diff_file_header.vue'; import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; -import mountComponent from 'spec/helpers/vue_mount_component_helper'; +import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; const discussionFixture = 'merge_requests/diff_discussion.json'; @@ -9,6 +12,12 @@ describe('diff_file_header', () => { let vm; let props; const Component = Vue.extend(DiffFileHeader); + const store = new Vuex.Store({ + modules: { + diffs: diffsModule, + notes: notesModule, + }, + }); beforeEach(() => { const diffDiscussionMock = getJSONFixture(discussionFixture)[0]; @@ -26,13 +35,13 @@ describe('diff_file_header', () => { describe('computed', () => { describe('icon', () => { beforeEach(() => { - props.diffFile.blob.icon = 'dummy icon'; + props.diffFile.blob.icon = 'file-text-o'; }); it('returns the blob icon for files', () => { props.diffFile.submodule = false; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.icon).toBe(props.diffFile.blob.icon); }); @@ -40,7 +49,7 @@ describe('diff_file_header', () => { it('returns the archive icon for submodules', () => { props.diffFile.submodule = true; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.icon).toBe('archive'); }); @@ -58,7 +67,7 @@ describe('diff_file_header', () => { it('returns the fileHash for files', () => { props.diffFile.submodule = false; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.titleLink).toBe(`#${props.diffFile.fileHash}`); }); @@ -66,7 +75,7 @@ describe('diff_file_header', () => { it('returns the submoduleTreeUrl for submodules', () => { props.diffFile.submodule = true; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.titleLink).toBe(props.diffFile.submoduleTreeUrl); }); @@ -77,7 +86,7 @@ describe('diff_file_header', () => { submoduleTreeUrl: null, }); - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.titleLink).toBe(props.diffFile.submoduleLink); }); @@ -94,7 +103,7 @@ describe('diff_file_header', () => { it('returns the filePath for files', () => { props.diffFile.submodule = false; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.filePath).toBe(props.diffFile.filePath); }); @@ -102,7 +111,7 @@ describe('diff_file_header', () => { it('appends the truncated blob id for submodules', () => { props.diffFile.submodule = true; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.filePath).toBe( `${props.diffFile.filePath} @ ${props.diffFile.blob.id.substr(0, 8)}`, @@ -114,7 +123,7 @@ describe('diff_file_header', () => { it('returns a link tag if fileHash is set', () => { props.diffFile.fileHash = 'some hash'; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.titleTag).toBe('a'); }); @@ -122,7 +131,7 @@ describe('diff_file_header', () => { it('returns a span tag if fileHash is not set', () => { props.diffFile.fileHash = null; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.titleTag).toBe('span'); }); @@ -137,7 +146,7 @@ describe('diff_file_header', () => { }); it('returns true if file is stored in LFS', () => { - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.isUsingLfs).toBe(true); }); @@ -145,7 +154,7 @@ describe('diff_file_header', () => { it('returns false if file is not stored externally', () => { props.diffFile.storedExternally = false; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.isUsingLfs).toBe(false); }); @@ -153,7 +162,7 @@ describe('diff_file_header', () => { it('returns false if file is not stored in LFS', () => { props.diffFile.externalStorage = 'not lfs'; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.isUsingLfs).toBe(false); }); @@ -163,7 +172,7 @@ describe('diff_file_header', () => { it('returns chevron-down if the diff is expanded', () => { props.expanded = true; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.collapseIcon).toBe('chevron-down'); }); @@ -171,49 +180,18 @@ describe('diff_file_header', () => { it('returns chevron-right if the diff is collapsed', () => { props.expanded = false; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.collapseIcon).toBe('chevron-right'); }); }); - describe('isDiscussionsExpanded', () => { - beforeEach(() => { - Object.assign(props, { - discussionsExpanded: true, - expanded: true, - }); - }); - - it('returns true if diff and discussion are expanded', () => { - vm = mountComponent(Component, props); - - expect(vm.isDiscussionsExpanded).toBe(true); - }); - - it('returns false if discussion is collapsed', () => { - props.discussionsExpanded = false; - - vm = mountComponent(Component, props); - - expect(vm.isDiscussionsExpanded).toBe(false); - }); - - it('returns false if diff is collapsed', () => { - props.expanded = false; - - vm = mountComponent(Component, props); - - expect(vm.isDiscussionsExpanded).toBe(false); - }); - }); - describe('viewFileButtonText', () => { it('contains the truncated content SHA', () => { const dummySha = 'deebd00f is no SHA'; props.diffFile.contentSha = dummySha; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.viewFileButtonText).not.toContain(dummySha); expect(vm.viewFileButtonText).toContain(dummySha.substr(0, 8)); @@ -225,7 +203,7 @@ describe('diff_file_header', () => { const dummySha = 'deadabba sings no more'; props.diffFile.diffRefs.baseSha = dummySha; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.viewReplacedFileButtonText).not.toContain(dummySha); expect(vm.viewReplacedFileButtonText).toContain(dummySha.substr(0, 8)); @@ -234,25 +212,25 @@ describe('diff_file_header', () => { }); describe('methods', () => { - describe('handleToggle', () => { + describe('handleToggleFile', () => { beforeEach(() => { spyOn(vm, '$emit').and.stub(); }); it('emits toggleFile if checkTarget is false', () => { - vm.handleToggle(null, false); + vm.handleToggleFile(null, false); expect(vm.$emit).toHaveBeenCalledWith('toggleFile'); }); it('emits toggleFile if checkTarget is true and event target is header', () => { - vm.handleToggle({ target: vm.$refs.header }, true); + vm.handleToggleFile({ target: vm.$refs.header }, true); expect(vm.$emit).toHaveBeenCalledWith('toggleFile'); }); it('does not emit toggleFile if checkTarget is true and event target is not header', () => { - vm.handleToggle({ target: 'not header' }, true); + vm.handleToggleFile({ target: 'not header' }, true); expect(vm.$emit).not.toHaveBeenCalled(); }); @@ -266,7 +244,7 @@ describe('diff_file_header', () => { it('is visible if collapsible is true', () => { props.collapsible = true; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(collapseToggle()).not.toBe(null); }); @@ -274,14 +252,14 @@ describe('diff_file_header', () => { it('is hidden if collapsible is false', () => { props.collapsible = false; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(collapseToggle()).toBe(null); }); }); it('displays an file icon in the title', () => { - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.$el.querySelector('svg.js-file-icon use').getAttribute('xlink:href')).toContain( 'ruby', ); @@ -293,7 +271,7 @@ describe('diff_file_header', () => { it('displays the path of a added file', () => { props.diffFile.renamedFile = false; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(filePaths()).toHaveLength(1); expect(filePaths()[0]).toHaveText(props.diffFile.filePath); @@ -303,7 +281,7 @@ describe('diff_file_header', () => { props.diffFile.renamedFile = false; props.diffFile.deletedFile = true; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(filePaths()).toHaveLength(1); expect(filePaths()[0]).toHaveText(`${props.diffFile.filePath} deleted`); @@ -312,7 +290,7 @@ describe('diff_file_header', () => { it('displays old and new path if the file was renamed', () => { props.diffFile.renamedFile = true; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(filePaths()).toHaveLength(2); expect(filePaths()[0]).toHaveText(props.diffFile.oldPath); @@ -321,7 +299,7 @@ describe('diff_file_header', () => { }); it('displays a copy to clipboard button', () => { - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); const button = vm.$el.querySelector('.btn-clipboard'); expect(button).not.toBe(null); @@ -332,7 +310,7 @@ describe('diff_file_header', () => { it('it displays old and new file mode if it changed', () => { props.diffFile.modeChanged = true; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); const { fileMode } = vm.$refs; expect(fileMode).not.toBe(undefined); @@ -343,7 +321,7 @@ describe('diff_file_header', () => { it('does not display the file mode if it has not changed', () => { props.diffFile.modeChanged = false; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); const { fileMode } = vm.$refs; expect(fileMode).toBe(undefined); @@ -359,7 +337,7 @@ describe('diff_file_header', () => { externalStorage: 'lfs', }); - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(lfsLabel()).not.toBe(null); expect(lfsLabel()).toHaveText('LFS'); @@ -368,7 +346,7 @@ describe('diff_file_header', () => { it('does not display the LFS label for files stored in repository', () => { props.diffFile.storedExternally = false; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(lfsLabel()).toBe(null); }); @@ -376,7 +354,7 @@ describe('diff_file_header', () => { describe('edit button', () => { it('should not render edit button if addMergeRequestButtons is not true', () => { - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.$el.querySelector('.js-edit-blob')).toEqual(null); }); @@ -384,7 +362,7 @@ describe('diff_file_header', () => { it('should show edit button when file is editable', () => { props.addMergeRequestButtons = true; props.diffFile.editPath = '/'; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.$el.querySelector('.js-edit-blob')).toContainText('Edit'); }); @@ -393,7 +371,7 @@ describe('diff_file_header', () => { props.addMergeRequestButtons = true; props.diffFile.deletedFile = true; props.diffFile.editPath = '/'; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.$el.querySelector('.js-edit-blob')).toEqual(null); }); @@ -413,7 +391,7 @@ describe('diff_file_header', () => { props.diffFile.externalUrl = url; props.diffFile.formattedExternalUrl = title; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.$el.querySelector(`a[href="${url}"]`)).not.toBe(null); expect(vm.$el.querySelector(`a[data-original-title="View on ${title}"]`)).not.toBe(null); @@ -423,11 +401,39 @@ describe('diff_file_header', () => { props.diffFile.externalUrl = ''; props.diffFile.formattedExternalUrl = title; - vm = mountComponent(Component, props); + vm = mountComponentWithStore(Component, { props, store }); expect(vm.$el.querySelector(`a[data-original-title="View on ${title}"]`)).toBe(null); }); }); }); + + describe('handles toggle discussions', () => { + it('dispatches toggleFileDiscussions when user clicks on toggle discussions button', () => { + const propsCopy = Object.assign({}, props); + propsCopy.diffFile.submodule = false; + propsCopy.diffFile.blob = { + id: '848ed9407c6730ff16edb3dd24485a0eea24292a', + path: 'lib/base.js', + name: 'base.js', + mode: '100644', + readableText: true, + icon: 'file-text-o', + }; + propsCopy.addMergeRequestButtons = true; + propsCopy.diffFile.deletedFile = true; + + vm = mountComponentWithStore(Component, { + props: propsCopy, + store, + }); + + spyOn(vm, 'toggleFileDiscussions'); + + vm.$el.querySelector('.js-btn-vue-toggle-comments').click(); + + expect(vm.toggleFileDiscussions).toHaveBeenCalled(); + }); + }); }); }); diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index 6829c1e956a..c1560dac1a0 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -191,4 +191,48 @@ describe('DiffsStoreActions', () => { ); }); }); + + describe('toggleFileDiscussions', () => { + it('should dispatch collapseDiscussion when all discussions are expanded', () => { + const getters = { + getDiffFileDiscussions: jasmine.createSpy().and.returnValue([{ id: 1 }]), + diffHasAllExpandedDiscussions: jasmine.createSpy().and.returnValue(true), + diffHasAllCollpasedDiscussions: jasmine.createSpy().and.returnValue(false), + }; + + const dispatch = jasmine.createSpy('dispatch'); + + actions.toggleFileDiscussions({ getters, dispatch }); + + expect(dispatch).toHaveBeenCalledWith('collapseDiscussion', { discussionId: 1 }, { root: true }); + }); + + it('should dispatch expandDiscussion when all discussions are collapsed', () => { + const getters = { + getDiffFileDiscussions: jasmine.createSpy().and.returnValue([{ id: 1 }]), + diffHasAllExpandedDiscussions: jasmine.createSpy().and.returnValue(false), + diffHasAllCollpasedDiscussions: jasmine.createSpy().and.returnValue(true), + }; + + const dispatch = jasmine.createSpy(); + + actions.toggleFileDiscussions({ getters, dispatch }); + + expect(dispatch).toHaveBeenCalledWith('expandDiscussion', { discussionId: 1 }, { root: true }); + }); + + it('should dispatch expandDiscussion when some discussions are collapsed and others are expanded for the collapsed discussion', () => { + const getters = { + getDiffFileDiscussions: jasmine.createSpy().and.returnValue([{ expanded: false, id: 1 }]), + diffHasAllExpandedDiscussions: jasmine.createSpy().and.returnValue(false), + diffHasAllCollpasedDiscussions: jasmine.createSpy().and.returnValue(false), + }; + + const dispatch = jasmine.createSpy(); + + actions.toggleFileDiscussions({ getters, dispatch }); + + expect(dispatch).toHaveBeenCalledWith('expandDiscussion', { discussionId: 1 }, { root: true }); + }); + }); }); diff --git a/spec/javascripts/diffs/store/getters_spec.js b/spec/javascripts/diffs/store/getters_spec.js index 7a94f18778b..919b612bb6a 100644 --- a/spec/javascripts/diffs/store/getters_spec.js +++ b/spec/javascripts/diffs/store/getters_spec.js @@ -1,12 +1,24 @@ import * as getters from '~/diffs/store/getters'; import state from '~/diffs/store/modules/diff_state'; import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants'; +import discussion from '../mock_data/diff_discussions'; -describe('DiffsStoreGetters', () => { +describe('Diffs Module Getters', () => { let localState; + let discussionMock; + let discussionMock1; + + const diffFileMock = { + fileHash: '9732849daca6ae818696d9575f5d1207d1a7f8bb', + }; beforeEach(() => { localState = state(); + discussionMock = Object.assign({}, discussion); + discussionMock.diff_file.file_hash = diffFileMock.fileHash; + + discussionMock1 = Object.assign({}, discussion); + discussionMock1.diff_file.file_hash = diffFileMock.fileHash; }); describe('isParallelView', () => { @@ -63,4 +75,113 @@ describe('DiffsStoreGetters', () => { expect(getters.commitId(localState)).toEqual(null); }); }); + + describe('diffHasAllExpandedDiscussions', () => { + it('returns true when all discussions are expanded', () => { + expect( + getters.diffHasAllExpandedDiscussions(localState, { + getDiffFileDiscussions: () => [discussionMock, discussionMock], + })(diffFileMock), + ).toEqual(true); + }); + + it('returns false when there are no discussions', () => { + expect( + getters.diffHasAllExpandedDiscussions(localState, { + getDiffFileDiscussions: () => [], + })(diffFileMock), + ).toEqual(false); + }); + + it('returns false when one discussions is collapsed', () => { + discussionMock1.expanded = false; + + expect( + getters.diffHasAllExpandedDiscussions(localState, { + getDiffFileDiscussions: () => [discussionMock, discussionMock1], + })(diffFileMock), + ).toEqual(false); + }); + }); + + describe('diffHasAllCollpasedDiscussions', () => { + it('returns true when all discussions are collapsed', () => { + discussionMock.diff_file.file_hash = diffFileMock.fileHash; + discussionMock.expanded = false; + + expect( + getters.diffHasAllCollpasedDiscussions(localState, { + getDiffFileDiscussions: () => [discussionMock], + })(diffFileMock), + ).toEqual(true); + }); + + it('returns false when there are no discussions', () => { + expect( + getters.diffHasAllCollpasedDiscussions(localState, { + getDiffFileDiscussions: () => [], + })(diffFileMock), + ).toEqual(false); + }); + + it('returns false when one discussions is expanded', () => { + discussionMock1.expanded = false; + + expect( + getters.diffHasAllCollpasedDiscussions(localState, { + getDiffFileDiscussions: () => [discussionMock, discussionMock1], + })(diffFileMock), + ).toEqual(false); + }); + }); + + describe('diffHasExpandedDiscussions', () => { + it('returns true when one of the discussions is expanded', () => { + discussionMock1.expanded = false; + + expect( + getters.diffHasExpandedDiscussions(localState, { + getDiffFileDiscussions: () => [discussionMock, discussionMock], + })(diffFileMock), + ).toEqual(true); + }); + + it('returns false when there are no discussions', () => { + expect( + getters.diffHasExpandedDiscussions(localState, { getDiffFileDiscussions: () => [] })( + diffFileMock, + ), + ).toEqual(false); + }); + + it('returns false when no discussion is expanded', () => { + discussionMock.expanded = false; + discussionMock1.expanded = false; + + expect( + getters.diffHasExpandedDiscussions(localState, { + getDiffFileDiscussions: () => [discussionMock, discussionMock1], + })(diffFileMock), + ).toEqual(false); + }); + }); + + describe('getDiffFileDiscussions', () => { + it('returns an array with discussions when fileHash matches and the discussion belongs to a diff', () => { + discussionMock.diff_file.file_hash = diffFileMock.fileHash; + + expect( + getters.getDiffFileDiscussions(localState, {}, {}, { discussions: [discussionMock] })( + diffFileMock, + ).length, + ).toEqual(1); + }); + + it('returns an empty array when no discussions are found in the given diff', () => { + expect( + getters.getDiffFileDiscussions(localState, {}, {}, { discussions: [] })(diffFileMock) + .length, + ).toEqual(0); + }); + }); }); diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js index 71ef3aa9b03..b66e8e1ceb3 100644 --- a/spec/javascripts/notes/stores/actions_spec.js +++ b/spec/javascripts/notes/stores/actions_spec.js @@ -128,6 +128,19 @@ describe('Actions Notes Store', () => { }); }); + describe('collapseDiscussion', () => { + it('should commit collapse discussion', done => { + testAction( + actions.collapseDiscussion, + { discussionId: discussionMock.id }, + { notes: [discussionMock] }, + [{ type: 'COLLAPSE_DISCUSSION', payload: { discussionId: discussionMock.id } }], + [], + done, + ); + }); + }); + describe('async methods', () => { const interceptor = (request, next) => { next( diff --git a/spec/javascripts/notes/stores/mutation_spec.js b/spec/javascripts/notes/stores/mutation_spec.js index ccc7328447b..a15ff1a5888 100644 --- a/spec/javascripts/notes/stores/mutation_spec.js +++ b/spec/javascripts/notes/stores/mutation_spec.js @@ -74,6 +74,20 @@ describe('Notes Store mutations', () => { }); }); + describe('COLLAPSE_DISCUSSION', () => { + it('should collpase an expanded discussion', () => { + const discussion = Object.assign({}, discussionMock, { expanded: true }); + + const state = { + discussions: [discussion], + }; + + mutations.COLLAPSE_DISCUSSION(state, { discussionId: discussion.id }); + + expect(state.discussions[0].expanded).toEqual(false); + }); + }); + describe('REMOVE_PLACEHOLDER_NOTES', () => { it('should remove all placeholder notes in indivudal notes and discussion', () => { const placeholderNote = Object.assign({}, individualNote, { isPlaceholderNote: true }); |