diff options
37 files changed, 484 insertions, 541 deletions
diff --git a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue index c02561b7599..aecdd133bf8 100644 --- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue +++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue @@ -99,7 +99,7 @@ export default { methods: { ...mapActions('diffs', ['loadMoreLines', 'showCommentForm']), handleCommentButton() { - this.showCommentForm({ lineCode: this.line.line_code }); + this.showCommentForm({ lineCode: this.line.line_code, fileHash: this.fileHash }); }, handleLoadMoreLines() { if (this.isRequesting) { @@ -160,7 +160,7 @@ export default { > <template v-else> <button - v-if="shouldShowCommentButton" + v-show="shouldShowCommentButton" type="button" class="add-diff-note js-add-diff-note-button qa-diff-comment" title="Add a comment to this line" diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue index c7cef74fe40..9fd02acbd6e 100644 --- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue +++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue @@ -73,6 +73,7 @@ export default { this.cancelCommentForm({ lineCode: this.line.line_code, + fileHash: this.diffFileHash, }); this.$nextTick(() => { this.resetAutoSave(); diff --git a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue index 91b87fb042c..aa40b24950a 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_comment_row.vue @@ -1,5 +1,4 @@ <script> -import { mapState } from 'vuex'; import diffDiscussions from './diff_discussions.vue'; import diffLineNoteForm from './diff_line_note_form.vue'; @@ -17,29 +16,31 @@ export default { type: String, required: true, }, - lineIndex: { - type: Number, - required: true, - }, }, computed: { - ...mapState({ - diffLineCommentForms: state => state.diffs.diffLineCommentForms, - }), className() { return this.line.discussions.length ? '' : 'js-temp-notes-holder'; }, + shouldRender() { + if (this.line.hasForm) return true; + + if (!this.line.discussions || !this.line.discussions.length) { + return false; + } + + return this.line.discussions.every(discussion => discussion.expanded); + }, }, }; </script> <template> - <tr :class="className" class="notes_holder"> + <tr v-if="shouldRender" :class="className" class="notes_holder"> <td class="notes_content" colspan="3"> <div class="content"> <diff-discussions v-if="line.discussions.length" :discussions="line.discussions" /> <diff-line-note-form - v-if="diffLineCommentForms[line.line_code]" + v-if="line.hasForm" :diff-file-hash="diffFileHash" :line="line" :note-target-line="line" diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue index fafc1649ce7..6a0ce760e6d 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_view.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue @@ -1,5 +1,5 @@ <script> -import { mapGetters, mapState } from 'vuex'; +import { mapGetters } from 'vuex'; import inlineDiffTableRow from './inline_diff_table_row.vue'; import inlineDiffCommentRow from './inline_diff_comment_row.vue'; @@ -19,23 +19,18 @@ export default { }, }, computed: { - ...mapGetters('diffs', ['commitId', 'shouldRenderInlineCommentRow']), - ...mapState({ - diffLineCommentForms: state => state.diffs.diffLineCommentForms, - }), + ...mapGetters('diffs', ['commitId']), diffLinesLength() { return this.diffLines.length; }, - userColorScheme() { - return window.gon.user_color_scheme; - }, }, + userColorScheme: window.gon.user_color_scheme, }; </script> <template> <table - :class="userColorScheme" + :class="$options.userColorScheme" :data-commit-id="commitId" class="code diff-wrap-lines js-syntax-highlight text-file js-diff-inline-view" > @@ -49,11 +44,9 @@ export default { :is-bottom="index + 1 === diffLinesLength" /> <inline-diff-comment-row - v-if="shouldRenderInlineCommentRow(line)" - :key="index" + :key="`icr-${index}`" :diff-file-hash="diffFile.file_hash" :line="line" - :line-index="index" /> </template> </tbody> diff --git a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue index c6b50983277..b98463d3dd3 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue @@ -1,5 +1,4 @@ <script> -import { mapState } from 'vuex'; import diffDiscussions from './diff_discussions.vue'; import diffLineNoteForm from './diff_line_note_form.vue'; @@ -23,22 +22,13 @@ export default { }, }, computed: { - ...mapState({ - diffLineCommentForms: state => state.diffs.diffLineCommentForms, - }), - leftLineCode() { - return this.line.left && this.line.left.line_code; - }, - rightLineCode() { - return this.line.right && this.line.right.line_code; - }, hasExpandedDiscussionOnLeft() { - return this.line.left && this.line.left.discussions + return this.line.left && this.line.left.discussions.length ? this.line.left.discussions.every(discussion => discussion.expanded) : false; }, hasExpandedDiscussionOnRight() { - return this.line.right && this.line.right.discussions + return this.line.right && this.line.right.discussions.length ? this.line.right.discussions.every(discussion => discussion.expanded) : false; }, @@ -57,9 +47,10 @@ export default { ); }, showRightSideCommentForm() { - return ( - this.line.right && this.line.right.type && this.diffLineCommentForms[this.rightLineCode] - ); + return this.line.right && this.line.right.type && this.line.right.hasForm; + }, + showLeftSideCommentForm() { + return this.line.left && this.line.left.hasForm; }, className() { return (this.left && this.line.left.discussions.length > 0) || @@ -67,12 +58,30 @@ export default { ? '' : 'js-temp-notes-holder'; }, + shouldRender() { + const { line } = this; + const hasDiscussion = + (line.left && line.left.discussions && line.left.discussions.length) || + (line.right && line.right.discussions && line.right.discussions.length); + + if ( + hasDiscussion && + (this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight) + ) { + return true; + } + + const hasCommentFormOnLeft = line.left && line.left.hasForm; + const hasCommentFormOnRight = line.right && line.right.hasForm; + + return hasCommentFormOnLeft || hasCommentFormOnRight; + }, }, }; </script> <template> - <tr :class="className" class="notes_holder"> + <tr v-if="shouldRender" :class="className" class="notes_holder"> <td class="notes_content parallel old" colspan="2"> <div v-if="shouldRenderDiscussionsOnLeft" class="content"> <diff-discussions @@ -81,7 +90,7 @@ export default { /> </div> <diff-line-note-form - v-if="diffLineCommentForms[leftLineCode]" + v-if="showLeftSideCommentForm" :diff-file-hash="diffFileHash" :line="line.left" :note-target-line="line.left" diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue index 771b8a80352..9a6e0e82529 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue @@ -1,5 +1,5 @@ <script> -import { mapState, mapGetters } from 'vuex'; +import { mapGetters } from 'vuex'; import parallelDiffTableRow from './parallel_diff_table_row.vue'; import parallelDiffCommentRow from './parallel_diff_comment_row.vue'; @@ -19,23 +19,18 @@ export default { }, }, computed: { - ...mapGetters('diffs', ['commitId', 'shouldRenderParallelCommentRow']), - ...mapState({ - diffLineCommentForms: state => state.diffs.diffLineCommentForms, - }), + ...mapGetters('diffs', ['commitId']), diffLinesLength() { return this.diffLines.length; }, - userColorScheme() { - return window.gon.user_color_scheme; - }, }, + userColorScheme: window.gon.user_color_scheme, }; </script> <template> <div - :class="userColorScheme" + :class="$options.userColorScheme" :data-commit-id="commitId" class="code diff-wrap-lines js-syntax-highlight text-file" > @@ -50,7 +45,6 @@ export default { :is-bottom="index + 1 === diffLinesLength" /> <parallel-diff-comment-row - v-if="shouldRenderParallelCommentRow(line)" :key="`dcr-${index}`" :line="line" :diff-file-hash="diffFile.file_hash" diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index a3de058b20e..0899d793c91 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -99,12 +99,12 @@ export const setParallelDiffViewType = ({ commit }) => { historyPushState(url); }; -export const showCommentForm = ({ commit }, params) => { - commit(types.ADD_COMMENT_FORM_LINE, params); +export const showCommentForm = ({ commit }, { lineCode, fileHash }) => { + commit(types.TOGGLE_LINE_HAS_FORM, { lineCode, fileHash, hasForm: true }); }; -export const cancelCommentForm = ({ commit }, params) => { - commit(types.REMOVE_COMMENT_FORM_LINE, params); +export const cancelCommentForm = ({ commit }, { lineCode, fileHash }) => { + commit(types.TOGGLE_LINE_HAS_FORM, { lineCode, fileHash, hasForm: false }); }; export const loadMoreLines = ({ commit }, options) => { @@ -191,8 +191,8 @@ export const saveDiffDiscussion = ({ dispatch }, { note, formData }) => { return dispatch('saveNote', postData, { root: true }) .then(result => dispatch('updateDiscussion', result.discussion, { root: true })) .then(discussion => dispatch('assignDiscussionsToDiff', [discussion])) + .then(() => dispatch('updateResolvableDiscussonsCounts', null, { root: true })) .then(() => dispatch('closeDiffFileCommentForm', formData.diffFile.file_hash)) - .then(() => dispatch('startTaskList', null, { root: true })) .catch(() => createFlash(s__('MergeRequests|Saving the comment failed'))); }; diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index 6a87b712b48..fdf1efbb10e 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -70,40 +70,6 @@ export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) = discussion => discussion.diff_discussion && discussion.diff_file.file_hash === diff.file_hash, ) || []; -export const shouldRenderParallelCommentRow = state => line => { - const hasDiscussion = - (line.left && line.left.discussions && line.left.discussions.length) || - (line.right && line.right.discussions && line.right.discussions.length); - - const hasExpandedDiscussionOnLeft = - line.left && line.left.discussions && line.left.discussions.length - ? line.left.discussions.every(discussion => discussion.expanded) - : false; - const hasExpandedDiscussionOnRight = - line.right && line.right.discussions && line.right.discussions.length - ? line.right.discussions.every(discussion => discussion.expanded) - : false; - - if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) { - return true; - } - - const hasCommentFormOnLeft = line.left && state.diffLineCommentForms[line.left.line_code]; - const hasCommentFormOnRight = line.right && state.diffLineCommentForms[line.right.line_code]; - - return hasCommentFormOnLeft || hasCommentFormOnRight; -}; - -export const shouldRenderInlineCommentRow = state => line => { - if (state.diffLineCommentForms[line.line_code]) return true; - - if (!line.discussions || line.discussions.length === 0) { - return false; - } - - return line.discussions.every(discussion => discussion.expanded); -}; - // prevent babel-plugin-rewire from generating an invalid default during karma∂ tests export const getDiffFileByHash = state => fileHash => state.diffFiles.find(file => file.file_hash === fileHash); diff --git a/app/assets/javascripts/diffs/store/modules/diff_state.js b/app/assets/javascripts/diffs/store/modules/diff_state.js index 2f59a3822f4..8fb2f0e17ac 100644 --- a/app/assets/javascripts/diffs/store/modules/diff_state.js +++ b/app/assets/javascripts/diffs/store/modules/diff_state.js @@ -18,7 +18,6 @@ export default () => ({ diffFiles: [], mergeRequestDiffs: [], mergeRequestDiff: null, - diffLineCommentForms: {}, diffViewType: viewTypeFromQueryString || viewTypeFromCookie || defaultViewType, tree: [], treeEntries: {}, diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js index e011031e72c..74961a74899 100644 --- a/app/assets/javascripts/diffs/store/mutation_types.js +++ b/app/assets/javascripts/diffs/store/mutation_types.js @@ -3,8 +3,7 @@ export const SET_LOADING = 'SET_LOADING'; export const SET_DIFF_DATA = 'SET_DIFF_DATA'; export const SET_DIFF_VIEW_TYPE = 'SET_DIFF_VIEW_TYPE'; export const SET_MERGE_REQUEST_DIFFS = 'SET_MERGE_REQUEST_DIFFS'; -export const ADD_COMMENT_FORM_LINE = 'ADD_COMMENT_FORM_LINE'; -export const REMOVE_COMMENT_FORM_LINE = 'REMOVE_COMMENT_FORM_LINE'; +export const TOGGLE_LINE_HAS_FORM = 'TOGGLE_LINE_HAS_FORM'; export const ADD_CONTEXT_LINES = 'ADD_CONTEXT_LINES'; export const ADD_COLLAPSED_DIFFS = 'ADD_COLLAPSED_DIFFS'; export const EXPAND_ALL_FILES = 'EXPAND_ALL_FILES'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 2133cfe4825..7baf2ed17e6 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -1,4 +1,3 @@ -import Vue from 'vue'; import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; import { sortTree } from '~/ide/stores/utils'; import { @@ -49,12 +48,30 @@ export default { Object.assign(state, { diffViewType }); }, - [types.ADD_COMMENT_FORM_LINE](state, { lineCode }) { - Vue.set(state.diffLineCommentForms, lineCode, true); - }, + [types.TOGGLE_LINE_HAS_FORM](state, { lineCode, fileHash, hasForm }) { + const diffFile = state.diffFiles.find(f => f.file_hash === fileHash); + + if (!diffFile) return; + + if (diffFile.highlighted_diff_lines) { + diffFile.highlighted_diff_lines.find(l => l.line_code === lineCode).hasForm = hasForm; + } + + if (diffFile.parallel_diff_lines) { + const line = diffFile.parallel_diff_lines.find(l => { + const { left, right } = l; - [types.REMOVE_COMMENT_FORM_LINE](state, { lineCode }) { - Vue.delete(state.diffLineCommentForms, lineCode); + return (left && left.line_code === lineCode) || (right && right.line_code === lineCode); + }); + + if (line.left && line.left.line_code === lineCode) { + line.left.hasForm = hasForm; + } + + if (line.right && line.right.line_code === lineCode) { + line.right.hasForm = hasForm; + } + } }, [types.ADD_CONTEXT_LINES](state, options) { @@ -68,6 +85,7 @@ export default { ...line, line_code: line.line_code || `${fileHash}_${line.old_line}_${line.new_line}`, discussions: line.discussions || [], + hasForm: false, })); addContextLines({ diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index d9d3c0f2ca2..54b9ee4d2d6 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -209,9 +209,11 @@ export function prepareDiffData(diffData) { const line = file.parallel_diff_lines[u]; if (line.left) { line.left = trimFirstCharOfLineContent(line.left); + line.left.hasForm = false; } if (line.right) { line.right = trimFirstCharOfLineContent(line.right); + line.right.hasForm = false; } } } @@ -220,7 +222,7 @@ export function prepareDiffData(diffData) { const linesLength = file.highlighted_diff_lines.length; for (let u = 0; u < linesLength; u += 1) { const line = file.highlighted_diff_lines[u]; - Object.assign(line, { ...trimFirstCharOfLineContent(line) }); + Object.assign(line, { ...trimFirstCharOfLineContent(line), hasForm: false }); } showingLines += file.parallel_diff_lines.length; } diff --git a/app/assets/javascripts/notes/components/diff_with_note.vue b/app/assets/javascripts/notes/components/diff_with_note.vue index 8e8bd150647..af821df0fd2 100644 --- a/app/assets/javascripts/notes/components/diff_with_note.vue +++ b/app/assets/javascripts/notes/components/diff_with_note.vue @@ -4,7 +4,9 @@ import DiffFileHeader from '~/diffs/components/diff_file_header.vue'; import DiffViewer from '~/vue_shared/components/diff_viewer/diff_viewer.vue'; import ImageDiffOverlay from '~/diffs/components/image_diff_overlay.vue'; import { GlSkeletonLoading } from '@gitlab/ui'; -import { trimFirstCharOfLineContent, getDiffMode } from '~/diffs/store/utils'; +import { getDiffMode } from '~/diffs/store/utils'; + +const FIRST_CHAR_REGEX = /^(\+|-| )/; export default { components: { @@ -26,46 +28,16 @@ export default { }, computed: { ...mapState({ - noteableData: state => state.notes.noteableData, projectPath: state => state.diffs.projectPath, }), diffMode() { - return getDiffMode(this.diffFile); + return getDiffMode(this.discussion.diff_file); }, hasTruncatedDiffLines() { return ( this.discussion.truncated_diff_lines && this.discussion.truncated_diff_lines.length !== 0 ); }, - isDiscussionsExpanded() { - return true; // TODO: @fatihacet - Fix this. - }, - isCollapsed() { - return this.diffFile.collapsed || false; - }, - isImageDiff() { - return !this.diffFile.text; - }, - diffFileClass() { - const { text } = this.diffFile; - return text ? 'text-file' : 'js-image-file'; - }, - diffFile() { - return this.discussion.diff_file; - }, - imageDiffHtml() { - return this.discussion.image_diff_html; - }, - userColorScheme() { - return window.gon.user_color_scheme; - }, - normalizedDiffLines() { - if (this.discussion.truncated_diff_lines) { - return this.discussion.truncated_diff_lines.map(line => trimFirstCharOfLineContent(line)); - } - - return []; - }, }, mounted() { if (!this.hasTruncatedDiffLines) { @@ -74,9 +46,6 @@ export default { }, methods: { ...mapActions(['fetchDiscussionDiffLines']), - rowTag(html) { - return html.outerHTML ? 'tr' : 'template'; - }, fetchDiff() { this.error = false; this.fetchDiscussionDiffLines(this.discussion) @@ -85,31 +54,45 @@ export default { this.error = true; }); }, + trimChar(line) { + return line.replace(FIRST_CHAR_REGEX, ''); + }, }, + userColorSchemeClass: window.gon.user_color_scheme, }; </script> <template> - <div ref="fileHolder" :class="diffFileClass" class="diff-file file-holder"> + <div :class="{ 'text-file': discussion.diff_file.text }" class="diff-file file-holder"> <diff-file-header :discussion-path="discussion.discussion_path" - :diff-file="diffFile" + :diff-file="discussion.diff_file" :can-current-user-fork="false" - :discussions-expanded="isDiscussionsExpanded" - :expanded="!isCollapsed" + :expanded="!discussion.diff_file.collapsed" /> - <div v-if="diffFile.text" :class="userColorScheme" class="diff-content code"> + <div + v-if="discussion.diff_file.text" + :class="$options.userColorSchemeClass" + class="diff-content code" + > <table> - <tr v-for="line in normalizedDiffLines" :key="line.line_code" class="line_holder"> - <td class="diff-line-num old_line">{{ line.old_line }}</td> - <td class="diff-line-num new_line">{{ line.new_line }}</td> - <td :class="line.type" class="line_content" v-html="line.rich_text"></td> - </tr> + <template v-if="hasTruncatedDiffLines"> + <tr + v-for="line in discussion.truncated_diff_lines" + v-once + :key="line.line_code" + class="line_holder" + > + <td class="diff-line-num old_line">{{ line.old_line }}</td> + <td class="diff-line-num new_line">{{ line.new_line }}</td> + <td :class="line.type" class="line_content" v-html="trimChar(line.rich_text)"></td> + </tr> + </template> <tr v-if="!hasTruncatedDiffLines" class="line_holder line-holder-placeholder"> <td class="old_line diff-line-num"></td> <td class="new_line diff-line-num"></td> <td v-if="error" class="js-error-lazy-load-diff diff-loading-error-block"> - Unable to load the diff + {{ error }} Unable to load the diff <button class="btn-link btn-link-retry btn-no-padding js-toggle-lazy-diff-retry-button" @click="fetchDiff" @@ -131,17 +114,17 @@ export default { <div v-else> <diff-viewer :diff-mode="diffMode" - :new-path="diffFile.new_path" - :new-sha="diffFile.diff_refs.head_sha" - :old-path="diffFile.old_path" - :old-sha="diffFile.diff_refs.base_sha" - :file-hash="diffFile.file_hash" + :new-path="discussion.diff_file.new_path" + :new-sha="discussion.diff_file.diff_refs.head_sha" + :old-path="discussion.diff_file.old_path" + :old-sha="discussion.diff_file.diff_refs.base_sha" + :file-hash="discussion.diff_file.file_hash" :project-path="projectPath" > <image-diff-overlay slot="image-overlay" :discussions="discussion" - :file-hash="diffFile.file_hash" + :file-hash="discussion.diff_file.file_hash" :show-comment-icon="true" :should-toggle-discussion="false" badge-class="image-comment-badge" diff --git a/app/assets/javascripts/notes/components/discussion_counter.vue b/app/assets/javascripts/notes/components/discussion_counter.vue index ee79ecbf9b3..c7cfc0f0f3b 100644 --- a/app/assets/javascripts/notes/components/discussion_counter.vue +++ b/app/assets/javascripts/notes/components/discussion_counter.vue @@ -1,13 +1,12 @@ <script> import { mapActions, mapGetters } from 'vuex'; +import { GlTooltipDirective } from '@gitlab/ui'; import Icon from '~/vue_shared/components/icon.vue'; -import { pluralize } from '../../lib/utils/text_utility'; import discussionNavigation from '../mixins/discussion_navigation'; -import tooltip from '../../vue_shared/directives/tooltip'; export default { directives: { - tooltip, + GlTooltip: GlTooltipDirective, }, components: { Icon, @@ -17,9 +16,9 @@ export default { ...mapGetters([ 'getUserData', 'getNoteableData', - 'discussionCount', + 'resolvableDiscussionsCount', 'firstUnresolvedDiscussionId', - 'resolvedDiscussionCount', + 'unresolvedDiscussionsCount', ]), isLoggedIn() { return this.getUserData.id; @@ -27,15 +26,15 @@ export default { hasNextButton() { return this.isLoggedIn && !this.allResolved; }, - countText() { - return pluralize('discussion', this.discussionCount); - }, allResolved() { - return this.resolvedDiscussionCount === this.discussionCount; + return this.unresolvedDiscussionsCount === 0; }, resolveAllDiscussionsIssuePath() { return this.getNoteableData.create_issue_to_resolve_discussions_path; }, + resolvedDiscussionsCount() { + return this.resolvableDiscussionsCount - this.unresolvedDiscussionsCount; + }, }, methods: { ...mapActions(['expandDiscussion']), @@ -50,7 +49,7 @@ export default { </script> <template> - <div v-if="discussionCount > 0" class="line-resolve-all-container prepend-top-8"> + <div v-if="resolvableDiscussionsCount > 0" class="line-resolve-all-container prepend-top-8"> <div> <div :class="{ 'has-next-btn': hasNextButton }" class="line-resolve-all"> <span @@ -61,15 +60,15 @@ export default { <icon name="check-circle" /> </span> <span class="line-resolve-text"> - {{ resolvedDiscussionCount }}/{{ discussionCount }} {{ countText }} resolved + {{ resolvedDiscussionsCount }}/{{ resolvableDiscussionsCount }} + {{ n__('discussion resolved', 'discussions resolved', resolvableDiscussionsCount) }} </span> </div> <div v-if="resolveAllDiscussionsIssuePath && !allResolved" class="btn-group" role="group"> <a - v-tooltip + v-gl-tooltip :href="resolveAllDiscussionsIssuePath" :title="s__('Resolve all discussions in new issue')" - data-container="body" class="new-issue-for-discussion btn btn-default discussion-create-issue-btn" > <icon name="issue-new" /> @@ -77,9 +76,8 @@ export default { </div> <div v-if="isLoggedIn && !allResolved" class="btn-group" role="group"> <button - v-tooltip + v-gl-tooltip title="Jump to first unresolved discussion" - data-container="body" class="btn btn-default discussion-next-btn" @click="jumpToFirstUnresolvedDiscussion" > diff --git a/app/assets/javascripts/notes/components/note_actions.vue b/app/assets/javascripts/notes/components/note_actions.vue index 9a5817890c9..d99694b06e9 100644 --- a/app/assets/javascripts/notes/components/note_actions.vue +++ b/app/assets/javascripts/notes/components/note_actions.vue @@ -1,8 +1,7 @@ <script> import { mapGetters } from 'vuex'; import Icon from '~/vue_shared/components/icon.vue'; -import tooltip from '~/vue_shared/directives/tooltip'; -import { GlLoadingIcon } from '@gitlab/ui'; +import { GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui'; export default { name: 'NoteActions', @@ -11,7 +10,7 @@ export default { GlLoadingIcon, }, directives: { - tooltip, + GlTooltip: GlTooltipDirective, }, props: { authorId: { @@ -119,10 +118,10 @@ export default { <template> <div class="note-actions"> - <span v-if="accessLevel" class="note-role user-access-role"> {{ accessLevel }} </span> + <span v-if="accessLevel" class="note-role user-access-role">{{ accessLevel }}</span> <div v-if="canResolve" class="note-actions-item"> <button - v-tooltip + v-gl-tooltip :class="{ 'is-disabled': !resolvable, 'is-active': isResolved }" :title="resolveButtonTitle" :aria-label="resolveButtonTitle" @@ -138,12 +137,10 @@ export default { </div> <div v-if="canAwardEmoji" class="note-actions-item"> <a - v-tooltip + v-gl-tooltip.bottom :class="{ 'js-user-authored': isAuthoredByCurrentUser }" class="note-action-button note-emoji-button js-add-award js-note-emoji" data-position="right" - data-placement="bottom" - data-container="body" href="#" title="Add reaction" > @@ -158,12 +155,10 @@ export default { </div> <div v-if="canEdit" class="note-actions-item"> <button - v-tooltip + v-gl-tooltip.bottom type="button" title="Edit comment" class="note-action-button js-note-edit btn btn-transparent" - data-container="body" - data-placement="bottom" @click="onEdit" > <icon name="pencil" css-classes="link-highlight" /> @@ -171,12 +166,10 @@ export default { </div> <div v-if="showDeleteAction" class="note-actions-item"> <button - v-tooltip + v-gl-tooltip.bottom type="button" title="Delete comment" class="note-action-button js-note-delete btn btn-transparent" - data-container="body" - data-placement="bottom" @click="onDelete" > <icon name="remove" class="link-highlight" /> @@ -184,19 +177,17 @@ export default { </div> <div v-else-if="shouldShowActionsDropdown" class="dropdown more-actions note-actions-item"> <button - v-tooltip + v-gl-tooltip.bottom type="button" title="More actions" class="note-action-button more-actions-toggle btn btn-transparent" data-toggle="dropdown" - data-container="body" - data-placement="bottom" > <icon css-classes="icon" name="ellipsis_v" /> </button> <ul class="dropdown-menu more-actions-dropdown dropdown-open-left"> <li v-if="canReportAsAbuse"> - <a :href="reportAbusePath"> {{ __('Report abuse to GitLab') }} </a> + <a :href="reportAbusePath">{{ __('Report abuse to GitLab') }}</a> </li> <li v-if="noteUrl"> <button @@ -213,7 +204,7 @@ export default { type="button" @click.prevent="onDelete" > - <span class="text-danger"> {{ __('Delete comment') }} </span> + <span class="text-danger">{{ __('Delete comment') }}</span> </button> </li> </ul> diff --git a/app/assets/javascripts/notes/components/note_awards_list.vue b/app/assets/javascripts/notes/components/note_awards_list.vue index 4aba2e65edb..3d60eb02db8 100644 --- a/app/assets/javascripts/notes/components/note_awards_list.vue +++ b/app/assets/javascripts/notes/components/note_awards_list.vue @@ -1,16 +1,16 @@ <script> import { mapActions, mapGetters } from 'vuex'; +import { GlTooltipDirective } from '@gitlab/ui'; import Icon from '~/vue_shared/components/icon.vue'; import Flash from '../../flash'; import { glEmojiTag } from '../../emoji'; -import tooltip from '../../vue_shared/directives/tooltip'; export default { components: { Icon, }, directives: { - tooltip, + GlTooltip: GlTooltipDirective, }, props: { awards: { @@ -167,21 +167,19 @@ export default { <button v-for="(awardList, awardName, index) in groupedAwards" :key="index" - v-tooltip + v-gl-tooltip.bottom="{ boundary: 'viewport' }" :class="getAwardClassBindings(awardList)" :title="awardTitle(awardList)" class="btn award-control" - data-boundary="viewport" - data-placement="bottom" type="button" @click="handleAward(awardName);" > <span v-html="getAwardHTML(awardName)"></span> - <span class="award-control-text js-counter"> {{ awardList.length }} </span> + <span class="award-control-text js-counter">{{ awardList.length }}</span> </button> <div v-if="canAwardEmoji" class="award-menu-holder"> <button - v-tooltip + v-gl-tooltip :class="{ 'js-user-authored': isAuthoredByMe }" class="award-control btn js-add-award" title="Add reaction" diff --git a/app/assets/javascripts/notes/components/note_header.vue b/app/assets/javascripts/notes/components/note_header.vue index 8b7450783c9..e1a58e7cb26 100644 --- a/app/assets/javascripts/notes/components/note_header.vue +++ b/app/assets/javascripts/notes/components/note_header.vue @@ -73,7 +73,7 @@ export default { {{ __('Toggle discussion') }} </button> </div> - <a v-if="hasAuthor" :href="author.path"> + <a v-if="hasAuthor" v-once :href="author.path"> <span class="note-header-author-name">{{ author.name }}</span> <span v-if="author.status_tooltip_html" v-html="author.status_tooltip_html"></span> <span class="note-headline-light"> @{{ author.username }} </span> diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index 29740ddf6ae..36388e4b3cf 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -1,7 +1,8 @@ <script> import { mapActions, mapGetters } from 'vuex'; +import { GlTooltipDirective } from '@gitlab/ui'; import { truncateSha } from '~/lib/utils/text_utility'; -import { s__ } from '~/locale'; +import { s__, __ } from '~/locale'; import systemNote from '~/vue_shared/components/notes/system_note.vue'; import icon from '~/vue_shared/components/icon.vue'; import Flash from '../../flash'; @@ -20,14 +21,12 @@ import autosave from '../mixins/autosave'; import noteable from '../mixins/noteable'; import resolvable from '../mixins/resolvable'; import discussionNavigation from '../mixins/discussion_navigation'; -import tooltip from '../../vue_shared/directives/tooltip'; export default { name: 'NoteableDiscussion', components: { icon, noteableNote, - diffWithNote, userAvatarLink, noteHeader, noteSignedOutWidget, @@ -39,7 +38,7 @@ export default { systemNote, }, directives: { - tooltip, + GlTooltip: GlTooltipDirective, }, mixins: [autosave, noteable, resolvable, discussionNavigation], props: { @@ -74,33 +73,12 @@ export default { computed: { ...mapGetters([ 'getNoteableData', - 'discussionCount', - 'resolvedDiscussionCount', - 'allDiscussions', - 'unresolvedDiscussionsIdsByDiff', - 'unresolvedDiscussionsIdsByDate', - 'unresolvedDiscussions', - 'unresolvedDiscussionsIdsOrdered', 'nextUnresolvedDiscussionId', - 'isLastUnresolvedDiscussion', + 'unresolvedDiscussionsCount', + 'hasUnresolvedDiscussions', ]), - transformedDiscussion() { - return { - ...this.discussion.notes[0], - truncated_diff_lines: this.discussion.truncated_diff_lines || [], - truncated_diff_lines_path: this.discussion.truncated_diff_lines_path, - diff_file: this.discussion.diff_file, - diff_discussion: this.discussion.diff_discussion, - active: this.discussion.active, - discussion_path: this.discussion.discussion_path, - resolved: this.discussion.resolved, - resolved_by: this.discussion.resolved_by, - resolved_by_push: this.discussion.resolved_by_push, - resolved_at: this.discussion.resolved_at, - }; - }, author() { - return this.transformedDiscussion.author; + return this.initialDiscussion.author; }, canReply() { return this.getNoteableData.current_user.can_create_note; @@ -136,29 +114,13 @@ export default { return null; }, resolvedText() { - return this.transformedDiscussion.resolved_by_push ? 'Automatically resolved' : 'Resolved'; - }, - hasMultipleUnresolvedDiscussions() { - return this.unresolvedDiscussions.length > 1; - }, - showJumpToNextDiscussion() { - return ( - this.hasMultipleUnresolvedDiscussions && - !this.isLastUnresolvedDiscussion(this.discussion.id, this.discussionsByDiffOrder) - ); + return this.discussion.resolved_by_push ? __('Automatically resolved') : __('Resolved'); }, shouldRenderDiffs() { - return ( - this.transformedDiscussion.diff_discussion && - this.transformedDiscussion.diff_file && - this.renderDiffFile - ); + return this.discussion.diff_discussion && this.renderDiffFile; }, shouldGroupReplies() { - return !this.shouldRenderDiffs && !this.transformedDiscussion.diff_discussion; - }, - shouldRenderHeader() { - return this.shouldRenderDiffs; + return !this.shouldRenderDiffs && !this.discussion.diff_discussion; }, wrapperComponent() { return this.shouldRenderDiffs ? diffWithNote : 'div'; @@ -170,9 +132,6 @@ export default { return {}; }, - wrapperClass() { - return this.isDiffDiscussion ? '' : 'card discussion-wrapper'; - }, componentClassName() { if (this.shouldRenderDiffs) { if (!this.lastUpdatedAt && !this.discussion.resolved) { @@ -183,11 +142,10 @@ export default { return ''; }, shouldShowDiscussions() { - const isExpanded = this.discussion.expanded; - const { resolved } = this.transformedDiscussion; - const isResolvedNonDiffDiscussion = !this.transformedDiscussion.diff_discussion && resolved; + const { expanded, resolved } = this.discussion; + const isResolvedNonDiffDiscussion = !this.discussion.diff_discussion && resolved; - return isExpanded || this.alwaysExpanded || isResolvedNonDiffDiscussion; + return expanded || this.alwaysExpanded || isResolvedNonDiffDiscussion; }, isRepliesCollapsed() { const { discussion, isRepliesToggledByUser } = this; @@ -204,7 +162,7 @@ export default { if (this.isReplying) { this.$nextTick(() => { // Pass an extra key to separate reply and note edit forms - this.initAutoSave(this.transformedDiscussion, ['Reply']); + this.initAutoSave({ ...this.initialDiscussion, ...this.discussion }, ['Reply']); }); } else { this.disposeAutoSave(); @@ -314,12 +272,9 @@ Please check your network connection and try again.`; <li class="note note-discussion timeline-entry" :class="componentClassName"> <div class="timeline-entry-inner"> <div class="timeline-content"> - <div - :data-discussion-id="transformedDiscussion.discussion_id" - class="discussion js-discussion-container" - > - <div v-if="shouldRenderHeader" class="discussion-header note-wrapper"> - <div class="timeline-icon"> + <div :data-discussion-id="discussion.id" class="discussion js-discussion-container"> + <div v-if="shouldRenderDiffs" class="discussion-header note-wrapper"> + <div v-once class="timeline-icon"> <user-avatar-link v-if="author" :link-href="author.path" @@ -330,35 +285,35 @@ Please check your network connection and try again.`; </div> <note-header :author="author" - :created-at="transformedDiscussion.created_at" - :note-id="transformedDiscussion.id" + :created-at="initialDiscussion.created_at" + :note-id="initialDiscussion.id" :include-toggle="true" :expanded="discussion.expanded" @toggleHandler="toggleDiscussionHandler" > - <template v-if="transformedDiscussion.diff_discussion"> + <template v-if="discussion.diff_discussion"> started a discussion on - <a :href="transformedDiscussion.discussion_path"> - <template v-if="transformedDiscussion.active"> - the diff - </template> - <template v-else> - an old version of the diff - </template> + <a :href="discussion.discussion_path"> + <template v-if="discussion.active" + >the diff</template + > + <template v-else + >an old version of the diff</template + > </a> </template> <template v-else-if="discussion.for_commit"> started a discussion on commit - <a :href="discussion.discussion_path"> {{ truncateSha(discussion.commit_id) }} </a> - </template> - <template v-else> - started a discussion + <a :href="discussion.discussion_path">{{ truncateSha(discussion.commit_id) }}</a> </template> + <template v-else + >started a discussion</template + > </note-header> <note-edited-text - v-if="transformedDiscussion.resolved" - :edited-at="transformedDiscussion.resolved_at" - :edited-by="transformedDiscussion.resolved_by" + v-if="discussion.resolved" + :edited-at="discussion.resolved_at" + :edited-by="discussion.resolved_by" :action-text="resolvedText" class-name="discussion-headline-light js-discussion-headline" /> @@ -371,7 +326,11 @@ Please check your network connection and try again.`; /> </div> <div v-if="shouldShowDiscussions" class="discussion-body"> - <component :is="wrapperComponent" v-bind="wrapperComponentProps" :class="wrapperClass"> + <component + :is="wrapperComponent" + v-bind="wrapperComponentProps" + class="card discussion-wrapper" + > <div class="discussion-notes"> <ul class="notes"> <template v-if="shouldGroupReplies"> @@ -380,7 +339,7 @@ Please check your network connection and try again.`; :note="componentData(initialDiscussion)" @handleDeleteNote="deleteNoteHandler" > - <slot slot="avatar-badge" name="avatar-badge"> </slot> + <slot slot="avatar-badge" name="avatar-badge"></slot> </component> <toggle-replies-widget v-if="hasReplies" @@ -406,7 +365,7 @@ Please check your network connection and try again.`; :note="componentData(note)" @handleDeleteNote="deleteNoteHandler" > - <slot v-if="index === 0" slot="avatar-badge" name="avatar-badge"> </slot> + <slot v-if="index === 0" slot="avatar-badge" name="avatar-badge"></slot> </component> </template> </ul> @@ -446,22 +405,19 @@ Please check your network connection and try again.`; > <div v-if="!discussionResolved" class="btn-group" role="group"> <a - v-tooltip + v-gl-tooltip :href="discussion.resolve_with_issue_path" :title="s__('MergeRequests|Resolve this discussion in a new issue')" - class="new-issue-for-discussion btn - btn-default discussion-create-issue-btn" - data-container="body" + class="new-issue-for-discussion btn btn-default discussion-create-issue-btn" > <icon name="issue-new" /> </a> </div> - <div v-if="showJumpToNextDiscussion" class="btn-group" role="group"> + <div v-if="hasUnresolvedDiscussions" class="btn-group" role="group"> <button - v-tooltip + v-gl-tooltip class="btn btn-default discussion-next-btn" title="Jump to next unresolved discussion" - data-container="body" @click="jumpToNextDiscussion" > <icon name="comment-next" /> diff --git a/app/assets/javascripts/notes/components/noteable_note.vue b/app/assets/javascripts/notes/components/noteable_note.vue index c2e49f8b23f..fc37fe1c7af 100644 --- a/app/assets/javascripts/notes/components/noteable_note.vue +++ b/app/assets/javascripts/notes/components/noteable_note.vue @@ -177,7 +177,7 @@ export default { class="note timeline-entry note-wrapper" > <div class="timeline-entry-inner"> - <div class="timeline-icon"> + <div v-once class="timeline-icon"> <user-avatar-link :link-href="author.path" :img-src="author.avatar_url" @@ -190,6 +190,7 @@ export default { <div class="timeline-content"> <div class="note-header"> <note-header + v-once :author="author" :created-at="note.created_at" :note-id="note.id" diff --git a/app/assets/javascripts/notes/components/notes_app.vue b/app/assets/javascripts/notes/components/notes_app.vue index 79ece036e69..6e6efb04753 100644 --- a/app/assets/javascripts/notes/components/notes_app.vue +++ b/app/assets/javascripts/notes/components/notes_app.vue @@ -22,6 +22,7 @@ export default { commentForm, placeholderNote, placeholderSystemNote, + skeletonLoadingContainer, }, props: { noteableData: { @@ -59,7 +60,6 @@ export default { 'isNotesFetched', 'discussions', 'getNotesDataByProp', - 'discussionCount', 'isLoading', 'commentsDisabled', ]), @@ -109,39 +109,22 @@ export default { this.$nextTick(() => highlightCurrentUser(this.$el.querySelectorAll('.gfm-project_member'))); }, methods: { - ...mapActions({ - setLoadingState: 'setLoadingState', - fetchDiscussions: 'fetchDiscussions', - poll: 'poll', - actionToggleAward: 'toggleAward', - scrollToNoteIfNeeded: 'scrollToNoteIfNeeded', - setNotesData: 'setNotesData', - setNoteableData: 'setNoteableData', - setUserData: 'setUserData', - setLastFetchedAt: 'setLastFetchedAt', - setTargetNoteHash: 'setTargetNoteHash', - toggleDiscussion: 'toggleDiscussion', - setNotesFetchedState: 'setNotesFetchedState', - startTaskList: 'startTaskList', - }), - getComponentName(discussion) { - if (discussion.isSkeletonNote) { - return skeletonLoadingContainer; - } - if (discussion.isPlaceholderNote) { - if (discussion.placeholderType === constants.SYSTEM_NOTE) { - return placeholderSystemNote; - } - return placeholderNote; - } else if (discussion.individual_note) { - return discussion.notes[0].system ? systemNote : noteableNote; - } - - return noteableDiscussion; - }, - getComponentData(discussion) { - return discussion.individual_note ? { note: discussion.notes[0] } : { discussion }; - }, + ...mapActions([ + 'setLoadingState', + 'fetchDiscussions', + 'poll', + 'toggleAward', + 'scrollToNoteIfNeeded', + 'setNotesData', + 'setNoteableData', + 'setUserData', + 'setLastFetchedAt', + 'setTargetNoteHash', + 'toggleDiscussion', + 'setNotesFetchedState', + 'expandDiscussion', + 'startTaskList', + ]), fetchNotes() { if (this.isFetching) return null; @@ -181,31 +164,46 @@ export default { const noteId = hash && hash.replace(/^note_/, ''); if (noteId) { - this.discussions.forEach(discussion => { - if (discussion.notes) { - discussion.notes.forEach(note => { - if (`${note.id}` === `${noteId}`) { - // FIXME: this modifies the store state without using a mutation/action - Object.assign(discussion, { expanded: true }); - } - }); - } - }); + const discussion = this.discussions.find(d => d.notes.some(({ id }) => id === noteId)); + + if (discussion) { + this.expandDiscussion({ discussionId: discussion.id }); + } } }, }, + systemNote: constants.SYSTEM_NOTE, }; </script> <template> <div v-show="shouldShow" id="notes"> <ul id="notes-list" class="notes main-notes-list timeline"> - <component - :is="getComponentName(discussion)" - v-for="discussion in allDiscussions" - :key="discussion.id" - v-bind="getComponentData(discussion)" - /> + <template v-for="discussion in allDiscussions"> + <skeleton-loading-container v-if="discussion.isSkeletonNote" :key="discussion.id" /> + <template v-else-if="discussion.isPlaceholderNote"> + <placeholder-system-note + v-if="discussion.placeholderType === $options.systemNote" + :key="discussion.id" + :note="discussion.notes[0]" + /> + <placeholder-note v-else :key="discussion.id" :note="discussion.notes[0]" /> + </template> + <template v-else-if="discussion.individual_note"> + <system-note + v-if="discussion.notes[0].system" + :key="discussion.id" + :note="discussion.notes[0]" + /> + <noteable-note v-else :key="discussion.id" :note="discussion.notes[0]" /> + </template> + <noteable-discussion + v-else + :key="discussion.id" + :discussion="discussion" + :render-diff-file="true" + /> + </template> </ul> <comment-form diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 5b2f0540020..b4befdd6e4a 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -11,7 +11,7 @@ import * as constants from '../constants'; import service from '../services/notes_service'; import loadAwardsHandler from '../../awards_handler'; import sidebarTimeTrackingEventHub from '../../sidebar/event_hub'; -import { isInViewport, scrollToElement } from '../../lib/utils/common_utils'; +import { isInViewport, scrollToElement, isInMRPage } from '../../lib/utils/common_utils'; import mrWidgetEventHub from '../../vue_merge_request_widget/event_hub'; import { __ } from '~/locale'; @@ -39,12 +39,13 @@ export const setNotesFetchedState = ({ commit }, state) => export const toggleDiscussion = ({ commit }, data) => commit(types.TOGGLE_DISCUSSION, data); -export const fetchDiscussions = ({ commit }, { path, filter }) => +export const fetchDiscussions = ({ commit, dispatch }, { path, filter }) => service .fetchDiscussions(path, filter) .then(res => res.json()) .then(discussions => { commit(types.SET_INITIAL_DISCUSSIONS, discussions); + dispatch('updateResolvableDiscussonsCounts'); }); export const updateDiscussion = ({ commit, state }, discussion) => { @@ -53,11 +54,18 @@ export const updateDiscussion = ({ commit, state }, discussion) => { return utils.findNoteObjectById(state.discussions, discussion.id); }; -export const deleteNote = ({ commit, dispatch }, note) => +export const deleteNote = ({ commit, dispatch, state }, note) => service.deleteNote(note.path).then(() => { + const discussion = state.discussions.find(({ id }) => id === note.discussion_id); + commit(types.DELETE_NOTE, note); dispatch('updateMergeRequestWidget'); + dispatch('updateResolvableDiscussonsCounts'); + + if (isInMRPage()) { + dispatch('diffs/removeDiscussionsFromDiff', discussion); + } }); export const updateNote = ({ commit, dispatch }, { endpoint, note }) => @@ -89,6 +97,7 @@ export const createNewNote = ({ commit, dispatch }, { endpoint, data }) => dispatch('updateMergeRequestWidget'); dispatch('startTaskList'); + dispatch('updateResolvableDiscussonsCounts'); } return res; }); @@ -104,6 +113,8 @@ export const toggleResolveNote = ({ commit, dispatch }, { endpoint, isResolved, commit(mutationType, res); + dispatch('updateResolvableDiscussonsCounts'); + dispatch('updateMergeRequestWidget'); }); @@ -385,5 +396,8 @@ export const startTaskList = ({ dispatch }) => }), ); +export const updateResolvableDiscussonsCounts = ({ commit }) => + commit(types.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS); + // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 980d79605d7..2ed8aac059a 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -53,30 +53,15 @@ export const getCurrentUserLastNote = state => export const getDiscussionLastNote = state => discussion => reverseNotes(discussion.notes).find(el => isLastNote(el, state)); -export const discussionCount = state => { - const filteredDiscussions = state.discussions.filter(n => !n.individual_note && n.resolvable); - - return filteredDiscussions.length; -}; - -export const unresolvedDiscussions = (state, getters) => { - const resolvedMap = getters.resolvedDiscussionsById; - - return state.discussions.filter(n => !n.individual_note && !resolvedMap[n.id]); -}; - -export const allDiscussions = (state, getters) => { - const resolved = getters.resolvedDiscussionsById; - const unresolved = getters.unresolvedDiscussions; - - return Object.values(resolved).concat(unresolved); -}; +export const unresolvedDiscussionsCount = state => state.unresolvedDiscussionsCount; +export const resolvableDiscussionsCount = state => state.resolvableDiscussionsCount; +export const hasUnresolvedDiscussions = state => state.hasUnresolvedDiscussions; export const isDiscussionResolved = (state, getters) => discussionId => getters.resolvedDiscussionsById[discussionId] !== undefined; -export const allResolvableDiscussions = (state, getters) => - getters.allDiscussions.filter(d => !d.individual_note && d.resolvable); +export const allResolvableDiscussions = state => + state.discussions.filter(d => !d.individual_note && d.resolvable); export const resolvedDiscussionsById = state => { const map = {}; @@ -147,15 +132,12 @@ export const resolvedDiscussionCount = (state, getters) => { return Object.keys(resolvedMap).length; }; -export const discussionTabCounter = state => { - let all = []; - - state.discussions.forEach(discussion => { - all = all.concat(discussion.notes.filter(note => !note.system && !note.placeholder)); - }); - - return all.length; -}; +export const discussionTabCounter = state => + state.discussions.reduce( + (acc, discussion) => + acc + discussion.notes.filter(note => !note.system && !note.placeholder).length, + 0, + ); // Returns the list of discussion IDs ordered according to given parameter // @param {Boolean} diffOrder - is ordered by diff? @@ -182,8 +164,10 @@ export const isLastUnresolvedDiscussion = (state, getters) => (discussionId, dif export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => { const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); const currentIndex = idsOrdered.indexOf(discussionId); + const slicedIds = idsOrdered.slice(currentIndex + 1, currentIndex + 2); - return idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0]; + // Get the first ID if there is none after the currentIndex + return slicedIds.length ? idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0] : idsOrdered[0]; }; // @param {Boolean} diffOrder - is ordered by diff? diff --git a/app/assets/javascripts/notes/stores/modules/index.js b/app/assets/javascripts/notes/stores/modules/index.js index 8aea269ea7d..b5fe8bdb1d3 100644 --- a/app/assets/javascripts/notes/stores/modules/index.js +++ b/app/assets/javascripts/notes/stores/modules/index.js @@ -22,6 +22,9 @@ export default () => ({ current_user: {}, }, commentsDisabled: false, + resolvableDiscussionsCount: 0, + unresolvedDiscussionsCount: 0, + hasUnresolvedDiscussions: false, }, actions, getters, diff --git a/app/assets/javascripts/notes/stores/mutation_types.js b/app/assets/javascripts/notes/stores/mutation_types.js index dfbf3b7b34b..9c68ab67a8c 100644 --- a/app/assets/javascripts/notes/stores/mutation_types.js +++ b/app/assets/javascripts/notes/stores/mutation_types.js @@ -21,6 +21,7 @@ export const DISABLE_COMMENTS = 'DISABLE_COMMENTS'; export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION'; export const EXPAND_DISCUSSION = 'EXPAND_DISCUSSION'; export const TOGGLE_DISCUSSION = 'TOGGLE_DISCUSSION'; +export const UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS = 'UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS'; // Issue export const CLOSE_ISSUE = 'CLOSE_ISSUE'; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index f6054e0be87..667c8a97cf3 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -24,6 +24,7 @@ export default { noteData.resolved = false; noteData.resolve_path = note.resolve_path; noteData.resolve_with_issue_path = note.resolve_with_issue_path; + noteData.diff_discussion = false; } state.discussions.push(noteData); @@ -97,33 +98,36 @@ export default { }, [types.SET_INITIAL_DISCUSSIONS](state, discussionsData) { - const discussions = []; + const discussions = discussionsData.reduce((acc, d) => { + const discussion = { ...d }; + const diffData = {}; - discussionsData.forEach(discussion => { if (discussion.diff_file) { - Object.assign(discussion, { - file_hash: discussion.diff_file.file_hash, - truncated_diff_lines: discussion.truncated_diff_lines || [], - }); + diffData.file_hash = discussion.diff_file.file_hash; + diffData.truncated_diff_lines = discussion.truncated_diff_lines || []; } // To support legacy notes, should be very rare case. if (discussion.individual_note && discussion.notes.length > 1) { discussion.notes.forEach(n => { - discussions.push({ + acc.push({ ...discussion, + ...diffData, notes: [n], // override notes array to only have one item to mimick individual_note }); }); } else { const oldNote = utils.findNoteObjectById(state.discussions, discussion.id); - discussions.push({ + acc.push({ ...discussion, + ...diffData, expanded: oldNote ? oldNote.expanded : discussion.expanded, }); } - }); + + return acc; + }, []); Object.assign(state, { discussions }); }, @@ -195,7 +199,9 @@ export default { const selectedDiscussion = state.discussions.find(disc => disc.id === note.id); note.expanded = true; // override expand flag to prevent collapse if (note.diff_file) { - Object.assign(note, { file_hash: note.diff_file.file_hash }); + Object.assign(note, { + file_hash: note.diff_file.file_hash, + }); } Object.assign(selectedDiscussion, { ...note }); }, @@ -229,4 +235,16 @@ export default { [types.DISABLE_COMMENTS](state, value) { state.commentsDisabled = value; }, + [types.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS](state) { + state.resolvableDiscussionsCount = state.discussions.filter( + discussion => !discussion.individual_note && discussion.resolvable, + ).length; + state.unresolvedDiscussionsCount = state.discussions.filter( + discussion => + !discussion.individual_note && + discussion.resolvable && + discussion.notes.some(note => !note.resolved), + ).length; + state.hasUnresolvedDiscussions = state.unresolvedDiscussionsCount > 1; + }, }; diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 172a0dc5e91..948ace91a48 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -804,6 +804,9 @@ msgstr "" msgid "Automatically marked as default internal user" msgstr "" +msgid "Automatically resolved" +msgstr "" + msgid "Available" msgstr "" @@ -5465,6 +5468,9 @@ msgstr "" msgid "Resolve discussion" msgstr "" +msgid "Resolved" +msgstr "" + msgid "Response metrics (AWS ELB)" msgstr "" @@ -7571,6 +7577,11 @@ msgstr "" msgid "disabled" msgstr "" +msgid "discussion resolved" +msgid_plural "discussions resolved" +msgstr[0] "" +msgstr[1] "" + msgid "done" msgstr "" diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb index 87613a4fdce..328f96e6ed7 100644 --- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb +++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb @@ -50,7 +50,7 @@ describe 'Merge request > User resolves diff notes and discussions', :js do find('.line-resolve-btn').click expect(page).to have_selector('.line-resolve-btn.is-active') - expect(find('.line-resolve-btn')['data-original-title']).to eq("Resolved by #{user.name}") + expect(find('.line-resolve-btn')['aria-label']).to eq("Resolved by #{user.name}") end page.within '.diff-content' do @@ -243,7 +243,7 @@ describe 'Merge request > User resolves diff notes and discussions', :js do resolve_button.click wait_for_requests - expect(resolve_button['data-original-title']).to eq("Resolved by #{user.name}") + expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}") end end @@ -266,7 +266,7 @@ describe 'Merge request > User resolves diff notes and discussions', :js do wait_for_requests - expect(first('.line-resolve-btn')['data-original-title']).to eq("Resolved by #{user.name}") + expect(first('.line-resolve-btn')['aria-label']).to eq("Resolved by #{user.name}") end expect(page).to have_content('Last updated') @@ -285,7 +285,7 @@ describe 'Merge request > User resolves diff notes and discussions', :js do wait_for_requests resolve_buttons.each do |button| - expect(button['data-original-title']).to eq("Resolved by #{user.name}") + expect(button['aria-label']).to eq("Resolved by #{user.name}") end page.within '.line-resolve-all-container' do @@ -357,13 +357,12 @@ describe 'Merge request > User resolves diff notes and discussions', :js do resolve_button.click wait_for_requests - expect(resolve_button['data-original-title']).to eq("Resolved by #{user.name}") + expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}") end end - it 'shows jump to next discussion button, apart from the last one' do - expect(page).to have_selector('.discussion-reply-holder', count: 2) - expect(page).to have_selector('.discussion-reply-holder .discussion-next-btn', count: 1) + it 'shows jump to next discussion button' do + expect(page.all('.discussion-reply-holder', count: 2)).to all(have_selector('.discussion-next-btn')) end it 'displays next discussion even if hidden' do diff --git a/spec/javascripts/diffs/components/diff_file_header_spec.js b/spec/javascripts/diffs/components/diff_file_header_spec.js index 9530b50c729..b77907ff26f 100644 --- a/spec/javascripts/diffs/components/diff_file_header_spec.js +++ b/spec/javascripts/diffs/components/diff_file_header_spec.js @@ -464,7 +464,11 @@ describe('diff_file_header', () => { propsCopy.addMergeRequestButtons = true; propsCopy.diffFile.deleted_file = true; - const discussionGetter = () => [diffDiscussionMock]; + const discussionGetter = () => [ + { + ...diffDiscussionMock, + }, + ]; const notesModuleMock = notesModule(); notesModuleMock.getters.discussions = discussionGetter; vm = mountComponentWithStore(Component, { diff --git a/spec/javascripts/diffs/components/diff_line_note_form_spec.js b/spec/javascripts/diffs/components/diff_line_note_form_spec.js index 81b66cf7c9b..b983dc35a57 100644 --- a/spec/javascripts/diffs/components/diff_line_note_form_spec.js +++ b/spec/javascripts/diffs/components/diff_line_note_form_spec.js @@ -62,6 +62,7 @@ describe('DiffLineNoteForm', () => { component.$nextTick(() => { expect(component.cancelCommentForm).toHaveBeenCalledWith({ lineCode: diffLines[0].line_code, + fileHash: component.diffFileHash, }); expect(component.resetAutoSave).toHaveBeenCalled(); diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index acd95a3dd8b..43d8d950bed 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -310,13 +310,13 @@ describe('DiffsStoreActions', () => { describe('showCommentForm', () => { it('should call mutation to show comment form', done => { - const payload = { lineCode: 'lineCode' }; + const payload = { lineCode: 'lineCode', fileHash: 'hash' }; testAction( showCommentForm, payload, {}, - [{ type: types.ADD_COMMENT_FORM_LINE, payload }], + [{ type: types.TOGGLE_LINE_HAS_FORM, payload: { ...payload, hasForm: true } }], [], done, ); @@ -325,13 +325,13 @@ describe('DiffsStoreActions', () => { describe('cancelCommentForm', () => { it('should call mutation to cancel comment form', done => { - const payload = { lineCode: 'lineCode' }; + const payload = { lineCode: 'lineCode', fileHash: 'hash' }; testAction( cancelCommentForm, payload, {}, - [{ type: types.REMOVE_COMMENT_FORM_LINE, payload }], + [{ type: types.TOGGLE_LINE_HAS_FORM, payload: { ...payload, hasForm: false } }], [], done, ); diff --git a/spec/javascripts/diffs/store/getters_spec.js b/spec/javascripts/diffs/store/getters_spec.js index eef95c823fb..582535e0a53 100644 --- a/spec/javascripts/diffs/store/getters_spec.js +++ b/spec/javascripts/diffs/store/getters_spec.js @@ -186,77 +186,6 @@ describe('Diffs Module Getters', () => { }); }); - describe('shouldRenderParallelCommentRow', () => { - let line; - - beforeEach(() => { - line = {}; - - discussionMock.expanded = true; - - line.left = { - line_code: 'ABC', - discussions: [discussionMock], - }; - - line.right = { - line_code: 'DEF', - discussions: [discussionMock1], - }; - }); - - it('returns true when discussion is expanded', () => { - expect(getters.shouldRenderParallelCommentRow(localState)(line)).toEqual(true); - }); - - it('returns false when no discussion was found', () => { - line.left.discussions = []; - line.right.discussions = []; - - localState.diffLineCommentForms.ABC = false; - localState.diffLineCommentForms.DEF = false; - - expect(getters.shouldRenderParallelCommentRow(localState)(line)).toEqual(false); - }); - - it('returns true when discussionForm was found', () => { - localState.diffLineCommentForms.ABC = {}; - - expect(getters.shouldRenderParallelCommentRow(localState)(line)).toEqual(true); - }); - }); - - describe('shouldRenderInlineCommentRow', () => { - let line; - - beforeEach(() => { - discussionMock.expanded = true; - - line = { - lineCode: 'ABC', - discussions: [discussionMock], - }; - }); - - it('returns true when diffLineCommentForms has form', () => { - localState.diffLineCommentForms.ABC = {}; - - expect(getters.shouldRenderInlineCommentRow(localState)(line)).toEqual(true); - }); - - it('returns false when no line discussions were found', () => { - line.discussions = []; - - expect(getters.shouldRenderInlineCommentRow(localState)(line)).toEqual(false); - }); - - it('returns true if all found discussions are expanded', () => { - discussionMock.expanded = true; - - expect(getters.shouldRenderInlineCommentRow(localState)(line)).toEqual(true); - }); - }); - describe('getDiffFileDiscussions', () => { it('returns an array with discussions when fileHash matches and the discussion belongs to a diff', () => { discussionMock.diff_file.file_hash = diffFileMock.file_hash; diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index 598d723c940..d1040ace5ca 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -55,32 +55,6 @@ describe('DiffsStoreMutations', () => { }); }); - describe('ADD_COMMENT_FORM_LINE', () => { - it('should set a truthy reference for the given line code in diffLineCommentForms', () => { - const state = { diffLineCommentForms: {} }; - const lineCode = 'FDE'; - - mutations[types.ADD_COMMENT_FORM_LINE](state, { lineCode }); - - expect(state.diffLineCommentForms[lineCode]).toBeTruthy(); - }); - }); - - describe('REMOVE_COMMENT_FORM_LINE', () => { - it('should remove given reference from diffLineCommentForms', () => { - const state = { diffLineCommentForms: {} }; - const lineCode = 'FDE'; - - mutations[types.ADD_COMMENT_FORM_LINE](state, { lineCode }); - - expect(state.diffLineCommentForms[lineCode]).toBeTruthy(); - - mutations[types.REMOVE_COMMENT_FORM_LINE](state, { lineCode }); - - expect(state.diffLineCommentForms[lineCode]).toBeUndefined(); - }); - }); - describe('EXPAND_ALL_FILES', () => { it('should change the collapsed prop from diffFiles', () => { const diffFile = { @@ -98,7 +72,9 @@ describe('DiffsStoreMutations', () => { it('should call utils.addContextLines with proper params', () => { const options = { lineNumbers: { oldLineNumber: 1, newLineNumber: 2 }, - contextLines: [{ old_line: 1, new_line: 1, line_code: 'ff9200_1_1', discussions: [] }], + contextLines: [ + { old_line: 1, new_line: 1, line_code: 'ff9200_1_1', discussions: [], hasForm: false }, + ], fileHash: 'ff9200', params: { bottom: true, @@ -383,4 +359,35 @@ describe('DiffsStoreMutations', () => { expect(state.currentDiffFileId).toBe('somefileid'); }); }); + + describe('TOGGLE_LINE_HAS_FORM', () => { + it('sets hasForm on lines', () => { + const file = { + file_hash: 'hash', + parallel_diff_lines: [ + { left: { line_code: '123', hasForm: false }, right: {} }, + { left: {}, right: { line_code: '124', hasForm: false } }, + ], + highlighted_diff_lines: [ + { line_code: '123', hasForm: false }, + { line_code: '124', hasForm: false }, + ], + }; + const state = { + diffFiles: [file], + }; + + mutations[types.TOGGLE_LINE_HAS_FORM](state, { + lineCode: '123', + hasForm: true, + fileHash: 'hash', + }); + + expect(file.highlighted_diff_lines[0].hasForm).toBe(true); + expect(file.highlighted_diff_lines[1].hasForm).toBe(false); + + expect(file.parallel_diff_lines[0].left.hasForm).toBe(true); + expect(file.parallel_diff_lines[1].right.hasForm).toBe(false); + }); + }); }); diff --git a/spec/javascripts/notes/components/diff_with_note_spec.js b/spec/javascripts/notes/components/diff_with_note_spec.js index 95461396f10..0752bd05904 100644 --- a/spec/javascripts/notes/components/diff_with_note_spec.js +++ b/spec/javascripts/notes/components/diff_with_note_spec.js @@ -17,7 +17,7 @@ describe('diff_with_note', () => { }; const selectors = { get container() { - return vm.$refs.fileHolder; + return vm.$el; }, get diffTable() { return this.container.querySelector('.diff-content table'); @@ -70,7 +70,6 @@ describe('diff_with_note', () => { it('shows image diff', () => { vm = mountComponentWithStore(Component, { props, store }); - expect(selectors.container).toHaveClass('js-image-file'); expect(selectors.diffTable).not.toExist(); }); }); diff --git a/spec/javascripts/notes/components/noteable_discussion_spec.js b/spec/javascripts/notes/components/noteable_discussion_spec.js index 4b4403689d9..76e9cd03d2d 100644 --- a/spec/javascripts/notes/components/noteable_discussion_spec.js +++ b/spec/javascripts/notes/components/noteable_discussion_spec.js @@ -80,43 +80,6 @@ describe('noteable_discussion component', () => { }); describe('computed', () => { - describe('hasMultipleUnresolvedDiscussions', () => { - it('is false if there are no unresolved discussions', done => { - spyOnProperty(vm, 'unresolvedDiscussions').and.returnValue([]); - - Vue.nextTick() - .then(() => { - expect(vm.hasMultipleUnresolvedDiscussions).toBe(false); - }) - .then(done) - .catch(done.fail); - }); - - it('is false if there is one unresolved discussion', done => { - spyOnProperty(vm, 'unresolvedDiscussions').and.returnValue([discussionMock]); - - Vue.nextTick() - .then(() => { - expect(vm.hasMultipleUnresolvedDiscussions).toBe(false); - }) - .then(done) - .catch(done.fail); - }); - - it('is true if there are two unresolved discussions', done => { - const discussion = getJSONFixture(discussionWithTwoUnresolvedNotes)[0]; - discussion.notes[0].resolved = false; - vm.$store.dispatch('setInitialNotes', [discussion, discussion]); - - Vue.nextTick() - .then(() => { - expect(vm.hasMultipleUnresolvedDiscussions).toBe(true); - }) - .then(done) - .catch(done.fail); - }); - }); - describe('isRepliesCollapsed', () => { it('should return false for diff discussions', done => { const diffDiscussion = getJSONFixture(diffDiscussionFixture)[0]; diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js index fcdd834e4a0..24c2b3e6570 100644 --- a/spec/javascripts/notes/stores/actions_spec.js +++ b/spec/javascripts/notes/stores/actions_spec.js @@ -1,4 +1,5 @@ import Vue from 'vue'; +import $ from 'jquery'; import _ from 'underscore'; import { headersInterceptor } from 'spec/helpers/vue_resource_helper'; import * as actions from '~/notes/stores/actions'; @@ -330,10 +331,14 @@ describe('Actions Notes Store', () => { beforeEach(() => { Vue.http.interceptors.push(interceptor); + + $('body').attr('data-page', ''); }); afterEach(() => { Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor); + + $('body').attr('data-page', ''); }); it('commits DELETE_NOTE and dispatches updateMergeRequestWidget', done => { @@ -353,6 +358,39 @@ describe('Actions Notes Store', () => { { type: 'updateMergeRequestWidget', }, + { + type: 'updateResolvableDiscussonsCounts', + }, + ], + done, + ); + }); + + it('dispatches removeDiscussionsFromDiff on merge request page', done => { + const note = { path: `${gl.TEST_HOST}`, id: 1 }; + + $('body').attr('data-page', 'projects:merge_requests:show'); + + testAction( + actions.deleteNote, + note, + store.state, + [ + { + type: 'DELETE_NOTE', + payload: note, + }, + ], + [ + { + type: 'updateMergeRequestWidget', + }, + { + type: 'updateResolvableDiscussonsCounts', + }, + { + type: 'diffs/removeDiscussionsFromDiff', + }, ], done, ); @@ -399,6 +437,9 @@ describe('Actions Notes Store', () => { { type: 'startTaskList', }, + { + type: 'updateResolvableDiscussonsCounts', + }, ], done, ); @@ -472,6 +513,9 @@ describe('Actions Notes Store', () => { ], [ { + type: 'updateResolvableDiscussonsCounts', + }, + { type: 'updateMergeRequestWidget', }, ], @@ -494,6 +538,9 @@ describe('Actions Notes Store', () => { ], [ { + type: 'updateResolvableDiscussonsCounts', + }, + { type: 'updateMergeRequestWidget', }, ], @@ -525,4 +572,17 @@ describe('Actions Notes Store', () => { ); }); }); + + describe('updateResolvableDiscussonsCounts', () => { + it('commits UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS', done => { + testAction( + actions.updateResolvableDiscussonsCounts, + null, + {}, + [{ type: 'UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS' }], + [], + done, + ); + }); + }); }); diff --git a/spec/javascripts/notes/stores/getters_spec.js b/spec/javascripts/notes/stores/getters_spec.js index f853f9ff088..c066975a43b 100644 --- a/spec/javascripts/notes/stores/getters_spec.js +++ b/spec/javascripts/notes/stores/getters_spec.js @@ -117,17 +117,15 @@ describe('Getters Notes Store', () => { describe('allResolvableDiscussions', () => { it('should return only resolvable discussions in same order', () => { - const localGetters = { - allDiscussions: [ - discussion3, - unresolvableDiscussion, - discussion1, - unresolvableDiscussion, - discussion2, - ], - }; + state.discussions = [ + discussion3, + unresolvableDiscussion, + discussion1, + unresolvableDiscussion, + discussion2, + ]; - expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([ + expect(getters.allResolvableDiscussions(state)).toEqual([ discussion3, discussion1, discussion2, @@ -135,11 +133,9 @@ describe('Getters Notes Store', () => { }); it('should return empty array if there are no resolvable discussions', () => { - const localGetters = { - allDiscussions: [unresolvableDiscussion, unresolvableDiscussion], - }; + state.discussions = [unresolvableDiscussion, unresolvableDiscussion]; - expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([]); + expect(getters.allResolvableDiscussions(state)).toEqual([]); }); }); @@ -236,7 +232,7 @@ describe('Getters Notes Store', () => { it('should return the ID of the discussion after the ID provided', () => { expect(getters.nextUnresolvedDiscussionId(state, localGetters)('123')).toBe('456'); expect(getters.nextUnresolvedDiscussionId(state, localGetters)('456')).toBe('789'); - expect(getters.nextUnresolvedDiscussionId(state, localGetters)('789')).toBe(undefined); + expect(getters.nextUnresolvedDiscussionId(state, localGetters)('789')).toBe('123'); }); }); diff --git a/spec/javascripts/notes/stores/mutation_spec.js b/spec/javascripts/notes/stores/mutation_spec.js index 461de5a3106..1c4449d1055 100644 --- a/spec/javascripts/notes/stores/mutation_spec.js +++ b/spec/javascripts/notes/stores/mutation_spec.js @@ -437,4 +437,51 @@ describe('Notes Store mutations', () => { expect(state.commentsDisabled).toEqual(true); }); }); + + describe('UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS', () => { + it('updates resolvableDiscussionsCount', () => { + const state = { + discussions: [ + { individual_note: false, resolvable: true, notes: [] }, + { individual_note: true, resolvable: true, notes: [] }, + { individual_note: false, resolvable: false, notes: [] }, + ], + resolvableDiscussionsCount: 0, + }; + + mutations.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS(state); + + expect(state.resolvableDiscussionsCount).toBe(1); + }); + + it('updates unresolvedDiscussionsCount', () => { + const state = { + discussions: [ + { individual_note: false, resolvable: true, notes: [{ resolved: false }] }, + { individual_note: true, resolvable: true, notes: [{ resolved: false }] }, + { individual_note: false, resolvable: false, notes: [{ resolved: false }] }, + ], + unresolvedDiscussionsCount: 0, + }; + + mutations.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS(state); + + expect(state.unresolvedDiscussionsCount).toBe(1); + }); + + it('updates hasUnresolvedDiscussions', () => { + const state = { + discussions: [ + { individual_note: false, resolvable: true, notes: [{ resolved: false }] }, + { individual_note: false, resolvable: true, notes: [{ resolved: false }] }, + { individual_note: false, resolvable: false, notes: [{ resolved: false }] }, + ], + hasUnresolvedDiscussions: 0, + }; + + mutations.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS(state); + + expect(state.hasUnresolvedDiscussions).toBe(true); + }); + }); }); |