diff options
author | Felipe Artur <fcardozo@gitlab.com> | 2018-07-27 18:25:53 +0000 |
---|---|---|
committer | Felipe Artur <fcardozo@gitlab.com> | 2018-07-27 18:25:53 +0000 |
commit | 0cf823558887aaf2ad041473ed9961755d5d5f3b (patch) | |
tree | cafd8d35271ddad4890ce05c88075816f8733e01 | |
parent | 923b26133b8d04e8c4da4183b8f0382eefcc8147 (diff) | |
parent | d1f09ec7cdab3b6892b1006b8c382c08be4cd9e0 (diff) | |
download | gitlab-ce-0cf823558887aaf2ad041473ed9961755d5d5f3b.tar.gz |
Merge branch '11-1-3-stable-fixes-mr-refactor-regressions' into '11-1-stable-patch-3'11-1-stable-patch-3
MR refactor fixes for 11.1.3
See merge request gitlab-org/gitlab-ce!20878
41 files changed, 816 insertions, 247 deletions
diff --git a/app/assets/javascripts/autosave.js b/app/assets/javascripts/autosave.js index fa00a3cf386..e8c59fab609 100644 --- a/app/assets/javascripts/autosave.js +++ b/app/assets/javascripts/autosave.js @@ -53,4 +53,8 @@ export default class Autosave { return window.localStorage.removeItem(this.key); } + + dispose() { + this.field.off('input'); + } } diff --git a/app/assets/javascripts/diffs/components/diff_discussions.vue b/app/assets/javascripts/diffs/components/diff_discussions.vue index 20483161033..e64d5511d78 100644 --- a/app/assets/javascripts/diffs/components/diff_discussions.vue +++ b/app/assets/javascripts/diffs/components/diff_discussions.vue @@ -30,6 +30,7 @@ export default { :render-header="false" :render-diff-file="false" :always-expanded="true" + :discussions-by-diff-order="true" /> </ul> </div> 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 ad838a32518..d184a76f038 100644 --- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue +++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue @@ -71,13 +71,18 @@ export default { required: false, default: false, }, + discussions: { + type: Array, + required: false, + default: () => [], + }, }, computed: { ...mapState({ diffViewType: state => state.diffs.diffViewType, diffFiles: state => state.diffs.diffFiles, }), - ...mapGetters(['isLoggedIn', 'discussionsByLineCode']), + ...mapGetters(['isLoggedIn']), lineHref() { return this.lineCode ? `#${this.lineCode}` : '#'; }, @@ -87,24 +92,19 @@ export default { this.showCommentButton && !this.isMatchLine && !this.isContextLine && - !this.hasDiscussions && - !this.isMetaLine + !this.isMetaLine && + !this.hasDiscussions ); }, - discussions() { - return this.discussionsByLineCode[this.lineCode] || []; - }, hasDiscussions() { return this.discussions.length > 0; }, shouldShowAvatarsOnGutter() { - let render = this.hasDiscussions && this.showCommentButton; - if (!this.lineType && this.linePosition === LINE_POSITION_RIGHT) { - render = false; + return false; } - return render; + return this.hasDiscussions && this.showCommentButton; }, }, methods: { @@ -189,7 +189,6 @@ export default { </button> <a v-if="lineNumber" - v-once :data-linenumber="lineNumber" :href="lineHref" > 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 32f9516d332..cbe4551d06b 100644 --- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue +++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue @@ -1,17 +1,17 @@ <script> -import $ from 'jquery'; import { mapState, mapGetters, mapActions } from 'vuex'; import createFlash from '~/flash'; import { s__ } from '~/locale'; import noteForm from '../../notes/components/note_form.vue'; import { getNoteFormData } from '../store/utils'; -import Autosave from '../../autosave'; -import { DIFF_NOTE_TYPE, NOTE_TYPE } from '../constants'; +import autosave from '../../notes/mixins/autosave'; +import { DIFF_NOTE_TYPE } from '../constants'; export default { components: { noteForm, }, + mixins: [autosave], props: { diffFileHash: { type: String, @@ -41,28 +41,35 @@ export default { }, mounted() { if (this.isLoggedIn) { - const noteableData = this.getNoteableData; const keys = [ - NOTE_TYPE, - this.noteableType, - noteableData.id, - noteableData.diff_head_sha, + this.noteableData.diff_head_sha, DIFF_NOTE_TYPE, - noteableData.source_project_id, + this.noteableData.source_project_id, this.line.lineCode, ]; - this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys); + this.initAutoSave(this.noteableData, keys); } }, methods: { ...mapActions('diffs', ['cancelCommentForm']), ...mapActions(['saveNote', 'refetchDiscussionById']), - handleCancelCommentForm() { - this.autosave.reset(); + handleCancelCommentForm(shouldConfirm, isDirty) { + if (shouldConfirm && isDirty) { + const msg = s__('Notes|Are you sure you want to cancel creating this comment?'); + + // eslint-disable-next-line no-alert + if (!window.confirm(msg)) { + return; + } + } + this.cancelCommentForm({ lineCode: this.line.lineCode, }); + this.$nextTick(() => { + this.resetAutoSave(); + }); }, handleSaveNote(note) { const selectedDiffFile = this.getDiffFileByHash(this.diffFileHash); diff --git a/app/assets/javascripts/diffs/components/diff_table_cell.vue b/app/assets/javascripts/diffs/components/diff_table_cell.vue index 5962f30d9bb..e8e8ddc6c5e 100644 --- a/app/assets/javascripts/diffs/components/diff_table_cell.vue +++ b/app/assets/javascripts/diffs/components/diff_table_cell.vue @@ -67,6 +67,11 @@ export default { required: false, default: false, }, + discussions: { + type: Array, + required: false, + default: () => [], + }, }, computed: { ...mapGetters(['isLoggedIn']), @@ -136,6 +141,7 @@ export default { :is-match-line="isMatchLine" :is-context-line="isContentLine" :is-meta-line="isMetaLine" + :discussions="discussions" /> </td> </template> 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 ca265dd892c..1b5ae5e9f75 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,5 @@ <script> -import { mapState, mapGetters } from 'vuex'; +import { mapState } from 'vuex'; import diffDiscussions from './diff_discussions.vue'; import diffLineNoteForm from './diff_line_note_form.vue'; @@ -21,18 +21,22 @@ export default { type: Number, required: true, }, + discussions: { + type: Array, + required: false, + default: () => [], + }, }, computed: { ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), - ...mapGetters(['discussionsByLineCode']), - discussions() { - return this.discussionsByLineCode[this.line.lineCode] || []; - }, className() { return this.discussions.length ? '' : 'js-temp-notes-holder'; }, + hasCommentForm() { + return this.diffLineCommentForms[this.line.lineCode]; + }, }, }; </script> @@ -53,7 +57,7 @@ export default { :discussions="discussions" /> <diff-line-note-form - v-if="diffLineCommentForms[line.lineCode]" + v-if="hasCommentForm" :diff-file-hash="diffFileHash" :line="line" :note-target-line="line" diff --git a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue index 0197a510ef1..32d65ff994f 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue @@ -33,6 +33,11 @@ export default { required: false, default: false, }, + discussions: { + type: Array, + required: false, + default: () => [], + }, }, data() { return { @@ -89,6 +94,7 @@ export default { :is-bottom="isBottom" :is-hover="isHover" :show-comment-button="true" + :discussions="discussions" class="diff-line-num old_line" /> <diff-table-cell @@ -98,10 +104,10 @@ export default { :line-type="newLineType" :is-bottom="isBottom" :is-hover="isHover" + :discussions="discussions" class="diff-line-num new_line" /> <td - v-once :class="line.type" class="line_content" v-html="line.richText" diff --git a/app/assets/javascripts/diffs/components/inline_diff_view.vue b/app/assets/javascripts/diffs/components/inline_diff_view.vue index 9fd19b74cd7..5f30cc57a59 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_view.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue @@ -20,8 +20,11 @@ export default { }, }, computed: { - ...mapGetters('diffs', ['commitId']), - ...mapGetters(['discussionsByLineCode']), + ...mapGetters('diffs', [ + 'commitId', + 'shouldRenderInlineCommentRow', + 'singleDiscussionByLineCode', + ]), ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), @@ -35,18 +38,7 @@ export default { return window.gon.user_color_scheme; }, }, - methods: { - shouldRenderCommentRow(line) { - if (this.diffLineCommentForms[line.lineCode]) return true; - - const lineDiscussions = this.discussionsByLineCode[line.lineCode]; - if (lineDiscussions === undefined) { - return false; - } - - return lineDiscussions.every(discussion => discussion.expanded); - }, - }, + methods: {}, }; </script> @@ -65,13 +57,15 @@ export default { :line="line" :is-bottom="index + 1 === diffLinesLength" :key="line.lineCode" + :discussions="singleDiscussionByLineCode(line.lineCode)" /> <inline-diff-comment-row - v-if="shouldRenderCommentRow(line)" + v-if="shouldRenderInlineCommentRow(line)" :diff-file-hash="diffFile.fileHash" :line="line" :line-index="index" :key="index" + :discussions="singleDiscussionByLineCode(line.lineCode)" /> </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 cc5248c25d9..bb9a65c83fa 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,5 @@ <script> -import { mapState, mapGetters } from 'vuex'; +import { mapState } from 'vuex'; import diffDiscussions from './diff_discussions.vue'; import diffLineNoteForm from './diff_line_note_form.vue'; @@ -21,48 +21,51 @@ export default { type: Number, required: true, }, + leftDiscussions: { + type: Array, + required: false, + default: () => [], + }, + rightDiscussions: { + type: Array, + required: false, + default: () => [], + }, }, computed: { ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), - ...mapGetters(['discussionsByLineCode']), leftLineCode() { return this.line.left.lineCode; }, rightLineCode() { return this.line.right.lineCode; }, - hasDiscussion() { - const discussions = this.discussionsByLineCode; - - return discussions[this.leftLineCode] || discussions[this.rightLineCode]; - }, hasExpandedDiscussionOnLeft() { - const discussions = this.discussionsByLineCode[this.leftLineCode]; - + const discussions = this.leftDiscussions; return discussions ? discussions.every(discussion => discussion.expanded) : false; }, hasExpandedDiscussionOnRight() { - const discussions = this.discussionsByLineCode[this.rightLineCode]; - + const discussions = this.rightDiscussions; return discussions ? discussions.every(discussion => discussion.expanded) : false; }, hasAnyExpandedDiscussion() { return this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight; }, shouldRenderDiscussionsOnLeft() { - return this.discussionsByLineCode[this.leftLineCode] && this.hasExpandedDiscussionOnLeft; + return this.leftDiscussions && this.hasExpandedDiscussionOnLeft; }, shouldRenderDiscussionsOnRight() { - return ( - this.discussionsByLineCode[this.rightLineCode] && - this.hasExpandedDiscussionOnRight && - this.line.right.type - ); + return this.rightDiscussions && this.hasExpandedDiscussionOnRight && this.line.right.type; + }, + showRightSideCommentForm() { + return this.line.right.type && this.diffLineCommentForms[this.rightLineCode]; }, className() { - return this.hasDiscussion ? '' : 'js-temp-notes-holder'; + return this.leftDiscussions.length > 0 || this.rightDiscussions.length > 0 + ? '' + : 'js-temp-notes-holder'; }, }, }; @@ -80,13 +83,12 @@ export default { class="content" > <diff-discussions - v-if="discussionsByLineCode[leftLineCode].length" - :discussions="discussionsByLineCode[leftLineCode]" + v-if="leftDiscussions.length" + :discussions="leftDiscussions" /> </div> <diff-line-note-form - v-if="diffLineCommentForms[leftLineCode] && - diffLineCommentForms[leftLineCode]" + v-if="diffLineCommentForms[leftLineCode]" :diff-file-hash="diffFileHash" :line="line.left" :note-target-line="line.left" @@ -100,13 +102,12 @@ export default { class="content" > <diff-discussions - v-if="discussionsByLineCode[rightLineCode].length" - :discussions="discussionsByLineCode[rightLineCode]" + v-if="rightDiscussions.length" + :discussions="rightDiscussions" /> </div> <diff-line-note-form - v-if="diffLineCommentForms[rightLineCode] && - diffLineCommentForms[rightLineCode] && line.right.type" + v-if="showRightSideCommentForm" :diff-file-hash="diffFileHash" :line="line.right" :note-target-line="line.right" diff --git a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue index ee5bb4d8d05..d4e54c2bd00 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue @@ -36,6 +36,16 @@ export default { required: false, default: false, }, + leftDiscussions: { + type: Array, + required: false, + default: () => [], + }, + rightDiscussions: { + type: Array, + required: false, + default: () => [], + }, }, data() { return { @@ -116,10 +126,10 @@ export default { :is-hover="isLeftHover" :show-comment-button="true" :diff-view-type="parallelDiffViewType" + :discussions="leftDiscussions" class="diff-line-num old_line" /> <td - v-once :id="line.left.lineCode" :class="parallelViewLeftLineType" class="line_content parallel left-side" @@ -137,10 +147,10 @@ export default { :is-hover="isRightHover" :show-comment-button="true" :diff-view-type="parallelDiffViewType" + :discussions="rightDiscussions" class="diff-line-num new_line" /> <td - v-once :id="line.right.lineCode" :class="line.right.type" class="line_content parallel right-side" diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue index 32528c9e7ab..4d97cb6d15d 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue @@ -21,8 +21,11 @@ export default { }, }, computed: { - ...mapGetters('diffs', ['commitId']), - ...mapGetters(['discussionsByLineCode']), + ...mapGetters('diffs', [ + 'commitId', + 'singleDiscussionByLineCode', + 'shouldRenderParallelCommentRow', + ]), ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), @@ -52,32 +55,6 @@ export default { return window.gon.user_color_scheme; }, }, - methods: { - shouldRenderCommentRow(line) { - const leftLineCode = line.left.lineCode; - const rightLineCode = line.right.lineCode; - const discussions = this.discussionsByLineCode; - const leftDiscussions = discussions[leftLineCode]; - const rightDiscussions = discussions[rightLineCode]; - const hasDiscussion = leftDiscussions || rightDiscussions; - - const hasExpandedDiscussionOnLeft = leftDiscussions - ? leftDiscussions.every(discussion => discussion.expanded) - : false; - const hasExpandedDiscussionOnRight = rightDiscussions - ? rightDiscussions.every(discussion => discussion.expanded) - : false; - - if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) { - return true; - } - - const hasCommentFormOnLeft = this.diffLineCommentForms[leftLineCode]; - const hasCommentFormOnRight = this.diffLineCommentForms[rightLineCode]; - - return hasCommentFormOnLeft || hasCommentFormOnRight; - }, - }, }; </script> @@ -98,13 +75,17 @@ export default { :line="line" :is-bottom="index + 1 === diffLinesLength" :key="index" + :left-discussions="singleDiscussionByLineCode(line.left.lineCode)" + :right-discussions="singleDiscussionByLineCode(line.right.lineCode)" /> <parallel-diff-comment-row - v-if="shouldRenderCommentRow(line)" + v-if="shouldRenderParallelCommentRow(line)" :key="`dcr-${index}`" :line="line" :diff-file-hash="diffFile.fileHash" :line-index="index" + :left-discussions="singleDiscussionByLineCode(line.left.lineCode)" + :right-discussions="singleDiscussionByLineCode(line.right.lineCode)" /> </template> </tbody> diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index 855de79adf8..c7b9b1a16e6 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -1,5 +1,7 @@ import _ from 'underscore'; +import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE } from '../constants'; +import { getDiffRefsByLineCode } from './utils'; export const isParallelView = state => state.diffViewType === PARALLEL_DIFF_VIEW_TYPE; @@ -56,6 +58,87 @@ export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) = discussion.diff_discussion && _.isEqual(discussion.diff_file.file_hash, diff.fileHash), ) || []; +/** + * Returns an Object with discussions by their diff line code + * To avoid rendering outdated discussions on the Changes tab we should do a bunch of SHA + * comparisions. `note.position.formatter` have the current version diff refs but + * `note.original_position.formatter` will have the first version's diff refs. + * If line diff refs matches with one of them, we should render it as a discussion on Changes tab. + * + * @param {Object} diff + * @returns {Array} + */ +export const discussionsByLineCode = (state, getters, rootState, rootGetters) => { + const diffRefsByLineCode = getDiffRefsByLineCode(state.diffFiles); + + return rootGetters.discussions.reduce((acc, note) => { + const isDiffDiscussion = note.diff_discussion; + const hasLineCode = note.line_code; + const isResolvable = note.resolvable; + + if (isDiffDiscussion && hasLineCode && isResolvable) { + const diffRefs = diffRefsByLineCode[note.line_code]; + if (diffRefs) { + const refs = convertObjectPropsToCamelCase(note.position.formatter); + const originalRefs = convertObjectPropsToCamelCase(note.original_position.formatter); + + if (_.isEqual(refs, diffRefs) || _.isEqual(originalRefs, diffRefs)) { + const lineCode = note.line_code; + + if (acc[lineCode]) { + acc[lineCode].push(note); + } else { + acc[lineCode] = [note]; + } + } + } + } + + return acc; + }, {}); +}; + +export const singleDiscussionByLineCode = (state, getters) => lineCode => { + if (!lineCode) return []; + const discussions = getters.discussionsByLineCode; + return discussions[lineCode] || []; +}; + +export const shouldRenderParallelCommentRow = (state, getters) => line => { + const leftLineCode = line.left.lineCode; + const rightLineCode = line.right.lineCode; + const leftDiscussions = getters.singleDiscussionByLineCode(leftLineCode); + const rightDiscussions = getters.singleDiscussionByLineCode(rightLineCode); + const hasDiscussion = leftDiscussions.length || rightDiscussions.length; + + const hasExpandedDiscussionOnLeft = leftDiscussions.length + ? leftDiscussions.every(discussion => discussion.expanded) + : false; + const hasExpandedDiscussionOnRight = rightDiscussions.length + ? rightDiscussions.every(discussion => discussion.expanded) + : false; + + if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) { + return true; + } + + const hasCommentFormOnLeft = state.diffLineCommentForms[leftLineCode]; + const hasCommentFormOnRight = state.diffLineCommentForms[rightLineCode]; + + return hasCommentFormOnLeft || hasCommentFormOnRight; +}; + +export const shouldRenderInlineCommentRow = (state, getters) => line => { + if (state.diffLineCommentForms[line.lineCode]) return true; + + const lineDiscussions = getters.singleDiscussionByLineCode(line.lineCode); + if (lineDiscussions.length === 0) { + return false; + } + + return lineDiscussions.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.fileHash === fileHash); diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index d9589baa76e..82082ac508a 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -173,3 +173,24 @@ export function trimFirstCharOfLineContent(line = {}) { return parsedLine; } + +export function getDiffRefsByLineCode(diffFiles) { + return diffFiles.reduce((acc, diffFile) => { + const { baseSha, headSha, startSha } = diffFile.diffRefs; + const { newPath, oldPath } = diffFile; + + // We can only use highlightedDiffLines to create the map of diff lines because + // highlightedDiffLines will also include every parallel diff line in it. + if (diffFile.highlightedDiffLines) { + diffFile.highlightedDiffLines.forEach(line => { + const { lineCode, oldLine, newLine } = line; + + if (lineCode) { + acc[lineCode] = { baseSha, headSha, startSha, newPath, oldPath, oldLine, newLine }; + } + }); + } + + return acc; + }, {}); +} diff --git a/app/assets/javascripts/lib/utils/poll.js b/app/assets/javascripts/lib/utils/poll.js index 7fca80c2fdb..91d8c30744f 100644 --- a/app/assets/javascripts/lib/utils/poll.js +++ b/app/assets/javascripts/lib/utils/poll.js @@ -38,7 +38,7 @@ import { normalizeHeaders } from './common_utils'; * } else { * poll.stop(); * } -* }); + * }); * * 1. Checks for response and headers before start polling * 2. Interval is provided by `Poll-Interval` header. @@ -51,8 +51,8 @@ export default class Poll { constructor(options = {}) { this.options = options; this.options.data = options.data || {}; - this.options.notificationCallback = options.notificationCallback || - function notificationCallback() {}; + this.options.notificationCallback = + options.notificationCallback || function notificationCallback() {}; this.intervalHeader = 'POLL-INTERVAL'; this.timeoutID = null; @@ -63,6 +63,7 @@ export default class Poll { const headers = normalizeHeaders(response.headers); const pollInterval = parseInt(headers[this.intervalHeader], 10); if (pollInterval > 0 && response.status === httpStatusCodes.OK && this.canPoll) { + clearTimeout(this.timeoutID); this.timeoutID = setTimeout(() => { this.makeRequest(); }, pollInterval); @@ -77,11 +78,11 @@ export default class Poll { notificationCallback(true); return resource[method](data) - .then((response) => { + .then(response => { this.checkConditions(response); notificationCallback(false); }) - .catch((error) => { + .catch(error => { notificationCallback(false); if (error.status === httpStatusCodes.ABORTED) { return; diff --git a/app/assets/javascripts/notes/components/discussion_counter.vue b/app/assets/javascripts/notes/components/discussion_counter.vue index 6385b75e557..ad6e7cf501d 100644 --- a/app/assets/javascripts/notes/components/discussion_counter.vue +++ b/app/assets/javascripts/notes/components/discussion_counter.vue @@ -5,19 +5,20 @@ import resolvedSvg from 'icons/_icon_status_success_solid.svg'; import mrIssueSvg from 'icons/_icon_mr_issue.svg'; import nextDiscussionSvg from 'icons/_next_discussion.svg'; import { pluralize } from '../../lib/utils/text_utility'; -import { scrollToElement } from '../../lib/utils/common_utils'; +import discussionNavigation from '../mixins/discussion_navigation'; import tooltip from '../../vue_shared/directives/tooltip'; export default { directives: { tooltip, }, + mixins: [discussionNavigation], computed: { ...mapGetters([ 'getUserData', 'getNoteableData', 'discussionCount', - 'unresolvedDiscussions', + 'firstUnresolvedDiscussionId', 'resolvedDiscussionCount', ]), isLoggedIn() { @@ -35,11 +36,6 @@ export default { resolveAllDiscussionsIssuePath() { return this.getNoteableData.create_issue_to_resolve_discussions_path; }, - firstUnresolvedDiscussionId() { - const item = this.unresolvedDiscussions[0] || {}; - - return item.id; - }, }, created() { this.resolveSvg = resolveSvg; @@ -50,22 +46,10 @@ export default { methods: { ...mapActions(['expandDiscussion']), jumpToFirstUnresolvedDiscussion() { - const discussionId = this.firstUnresolvedDiscussionId; - if (!discussionId) { - return; - } - - const el = document.querySelector(`[data-discussion-id="${discussionId}"]`); - const activeTab = window.mrTabs.currentAction; - - if (activeTab === 'commits' || activeTab === 'pipelines') { - window.mrTabs.activateTab('show'); - } + const diffTab = window.mrTabs.currentAction === 'diffs'; + const discussionId = this.firstUnresolvedDiscussionId(diffTab); - if (el) { - this.expandDiscussion({ discussionId }); - scrollToElement(el); - } + this.jumpToDiscussion(discussionId); }, }, }; diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue index 26482a02e00..abcd4422d7c 100644 --- a/app/assets/javascripts/notes/components/note_form.vue +++ b/app/assets/javascripts/notes/components/note_form.vue @@ -7,7 +7,7 @@ import issuableStateMixin from '../mixins/issuable_state'; import resolvable from '../mixins/resolvable'; export default { - name: 'IssueNoteForm', + name: 'NoteForm', components: { issueWarning, markdownField, diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index bee635398b3..0fe1c16854a 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -1,11 +1,11 @@ <script> -import _ from 'underscore'; import { mapActions, mapGetters } from 'vuex'; import resolveDiscussionsSvg from 'icons/_icon_mr_issue.svg'; import nextDiscussionsSvg from 'icons/_next_discussion.svg'; -import { convertObjectPropsToCamelCase, scrollToElement } from '~/lib/utils/common_utils'; +import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils'; import { truncateSha } from '~/lib/utils/text_utility'; import systemNote from '~/vue_shared/components/notes/system_note.vue'; +import { s__ } from '~/locale'; import Flash from '../../flash'; import { SYSTEM_NOTE } from '../constants'; import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue'; @@ -20,6 +20,7 @@ import placeholderSystemNote from '../../vue_shared/components/notes/placeholder 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 { @@ -39,7 +40,7 @@ export default { directives: { tooltip, }, - mixins: [autosave, noteable, resolvable], + mixins: [autosave, noteable, resolvable, discussionNavigation], props: { discussion: { type: Object, @@ -60,6 +61,11 @@ export default { required: false, default: false, }, + discussionsByDiffOrder: { + type: Boolean, + required: false, + default: false, + }, }, data() { return { @@ -74,7 +80,12 @@ export default { 'discussionCount', 'resolvedDiscussionCount', 'allDiscussions', + 'unresolvedDiscussionsIdsByDiff', + 'unresolvedDiscussionsIdsByDate', 'unresolvedDiscussions', + 'unresolvedDiscussionsIdsOrdered', + 'nextUnresolvedDiscussionId', + 'isLastUnresolvedDiscussion', ]), transformedDiscussion() { return { @@ -125,6 +136,10 @@ export default { hasMultipleUnresolvedDiscussions() { return this.unresolvedDiscussions.length > 1; }, + showJumpToNextDiscussion() { + return this.hasMultipleUnresolvedDiscussions && + !this.isLastUnresolvedDiscussion(this.discussion.id, this.discussionsByDiffOrder); + }, shouldRenderDiffs() { const { diffDiscussion, diffFile } = this.transformedDiscussion; @@ -144,19 +159,17 @@ export default { return this.isDiffDiscussion ? '' : 'card discussion-wrapper'; }, }, - mounted() { - if (this.isReplying) { - this.initAutoSave(this.transformedDiscussion); - } - }, - updated() { - if (this.isReplying) { - if (!this.autosave) { - this.initAutoSave(this.transformedDiscussion); + watch: { + isReplying() { + if (this.isReplying) { + this.$nextTick(() => { + // Pass an extra key to separate reply and note edit forms + this.initAutoSave(this.transformedDiscussion, ['Reply']); + }); } else { - this.setAutoSave(); + this.disposeAutoSave(); } - } + }, }, created() { this.resolveDiscussionsSvg = resolveDiscussionsSvg; @@ -194,16 +207,18 @@ export default { showReplyForm() { this.isReplying = true; }, - cancelReplyForm(shouldConfirm) { - if (shouldConfirm && this.$refs.noteForm.isDirty) { + cancelReplyForm(shouldConfirm, isDirty) { + if (shouldConfirm && isDirty) { + const msg = s__('Notes|Are you sure you want to cancel creating this comment?'); + // eslint-disable-next-line no-alert - if (!window.confirm('Are you sure you want to cancel creating this comment?')) { + if (!window.confirm(msg)) { return; } } - this.resetAutoSave(); this.isReplying = false; + this.resetAutoSave(); }, saveReply(noteText, form, callback) { const postData = { @@ -241,21 +256,10 @@ Please check your network connection and try again.`; }); }, jumpToNextDiscussion() { - const discussionIds = this.allDiscussions.map(d => d.id); - const unresolvedIds = this.unresolvedDiscussions.map(d => d.id); - const currentIndex = discussionIds.indexOf(this.discussion.id); - const remainingAfterCurrent = discussionIds.slice(currentIndex + 1); - const nextIndex = _.findIndex(remainingAfterCurrent, id => unresolvedIds.indexOf(id) > -1); - - if (nextIndex > -1) { - const nextId = remainingAfterCurrent[nextIndex]; - const el = document.querySelector(`[data-discussion-id="${nextId}"]`); + const nextId = + this.nextUnresolvedDiscussionId(this.discussion.id, this.discussionsByDiffOrder); - if (el) { - this.expandDiscussion({ discussionId: nextId }); - scrollToElement(el); - } - } + this.jumpToDiscussion(nextId); }, }, }; @@ -397,7 +401,7 @@ Please check your network connection and try again.`; </a> </div> <div - v-if="hasMultipleUnresolvedDiscussions" + v-if="showJumpToNextDiscussion" class="btn-group" role="group"> <button @@ -420,7 +424,8 @@ Please check your network connection and try again.`; :is-editing="false" save-button-title="Comment" @handleFormUpdate="saveReply" - @cancelForm="cancelReplyForm" /> + @cancelForm="cancelReplyForm" + /> <note-signed-out-widget v-if="!canReply" /> </div> </div> diff --git a/app/assets/javascripts/notes/mixins/autosave.js b/app/assets/javascripts/notes/mixins/autosave.js index 36cc8d5d056..4f45f912479 100644 --- a/app/assets/javascripts/notes/mixins/autosave.js +++ b/app/assets/javascripts/notes/mixins/autosave.js @@ -4,12 +4,18 @@ import { capitalizeFirstCharacter } from '../../lib/utils/text_utility'; export default { methods: { - initAutoSave(noteable) { - this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), [ + initAutoSave(noteable, extraKeys = []) { + let keys = [ 'Note', - capitalizeFirstCharacter(noteable.noteable_type), + capitalizeFirstCharacter(noteable.noteable_type || noteable.noteableType), noteable.id, - ]); + ]; + + if (extraKeys) { + keys = keys.concat(extraKeys); + } + + this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys); }, resetAutoSave() { this.autosave.reset(); @@ -17,5 +23,8 @@ export default { setAutoSave() { this.autosave.save(); }, + disposeAutoSave() { + this.autosave.dispose(); + }, }, }; diff --git a/app/assets/javascripts/notes/mixins/discussion_navigation.js b/app/assets/javascripts/notes/mixins/discussion_navigation.js new file mode 100644 index 00000000000..f7c4deee1f8 --- /dev/null +++ b/app/assets/javascripts/notes/mixins/discussion_navigation.js @@ -0,0 +1,29 @@ +import { scrollToElement } from '~/lib/utils/common_utils'; + +export default { + methods: { + jumpToDiscussion(id) { + if (id) { + const activeTab = window.mrTabs.currentAction; + const selector = + activeTab === 'diffs' + ? `ul.notes[data-discussion-id="${id}"]` + : `div.discussion[data-discussion-id="${id}"]`; + const el = document.querySelector(selector); + + if (activeTab === 'commits' || activeTab === 'pipelines') { + window.mrTabs.activateTab('show'); + } + + if (el) { + this.expandDiscussion({ discussionId: id }); + + scrollToElement(el); + return true; + } + } + + return false; + }, + }, +}; diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 5c65e1c3bb5..0d8d197bf71 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -28,18 +28,6 @@ export const notesById = state => return acc; }, {}); -export const discussionsByLineCode = state => - state.discussions.reduce((acc, note) => { - if (note.diff_discussion && note.line_code && note.resolvable) { - // For context about line notes: there might be multiple notes with the same line code - const items = acc[note.line_code] || []; - items.push(note); - - Object.assign(acc, { [note.line_code]: items }); - } - return acc; - }, {}); - export const noteableType = state => { const { ISSUE_NOTEABLE_TYPE, MERGE_REQUEST_NOTEABLE_TYPE, EPIC_NOTEABLE_TYPE } = constants; @@ -82,6 +70,9 @@ export const allDiscussions = (state, getters) => { return Object.values(resolved).concat(unresolved); }; +export const allResolvableDiscussions = (state, getters) => + getters.allDiscussions.filter(d => !d.individual_note && d.resolvable); + export const resolvedDiscussionsById = state => { const map = {}; @@ -98,6 +89,51 @@ export const resolvedDiscussionsById = state => { return map; }; +// Gets Discussions IDs ordered by the date of their initial note +export const unresolvedDiscussionsIdsByDate = (state, getters) => + getters.allResolvableDiscussions + .filter(d => !d.resolved) + .sort((a, b) => { + const aDate = new Date(a.notes[0].created_at); + const bDate = new Date(b.notes[0].created_at); + + if (aDate < bDate) { + return -1; + } + + return aDate === bDate ? 0 : 1; + }) + .map(d => d.id); + +// Gets Discussions IDs ordered by their position in the diff +// +// Sorts the array of resolvable yet unresolved discussions by +// comparing file names first. If file names are the same, compares +// line numbers. +export const unresolvedDiscussionsIdsByDiff = (state, getters) => + getters.allResolvableDiscussions + .filter(d => !d.resolved) + .sort((a, b) => { + if (!a.diff_file || !b.diff_file) { + return 0; + } + + // Get file names comparison result + const filenameComparison = a.diff_file.file_path.localeCompare(b.diff_file.file_path); + + // Get the line numbers, to compare within the same file + const aLines = [a.position.formatter.new_line, a.position.formatter.old_line]; + const bLines = [b.position.formatter.new_line, b.position.formatter.old_line]; + + return filenameComparison < 0 || + (filenameComparison === 0 && + // .max() because one of them might be zero (if removed/added) + Math.max(aLines[0], aLines[1]) < Math.max(bLines[0], bLines[1])) + ? -1 + : 1; + }) + .map(d => d.id); + export const resolvedDiscussionCount = (state, getters) => { const resolvedMap = getters.resolvedDiscussionsById; @@ -114,5 +150,42 @@ export const discussionTabCounter = state => { return all.length; }; +// Returns the list of discussion IDs ordered according to given parameter +// @param {Boolean} diffOrder - is ordered by diff? +export const unresolvedDiscussionsIdsOrdered = (state, getters) => diffOrder => { + if (diffOrder) { + return getters.unresolvedDiscussionsIdsByDiff; + } + return getters.unresolvedDiscussionsIdsByDate; +}; + +// Checks if a given discussion is the last in the current order (diff or date) +// @param {Boolean} discussionId - id of the discussion +// @param {Boolean} diffOrder - is ordered by diff? +export const isLastUnresolvedDiscussion = (state, getters) => (discussionId, diffOrder) => { + const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); + const lastDiscussionId = idsOrdered[idsOrdered.length - 1]; + + return lastDiscussionId === discussionId; +}; + +// Gets the ID of the discussion following the one provided, respecting order (diff or date) +// @param {Boolean} discussionId - id of the current discussion +// @param {Boolean} diffOrder - is ordered by diff? +export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => { + const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); + const currentIndex = idsOrdered.indexOf(discussionId); + + return idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0]; +}; + +// @param {Boolean} diffOrder - is ordered by diff? +export const firstUnresolvedDiscussionId = (state, getters) => diffOrder => { + if (diffOrder) { + return getters.unresolvedDiscussionsIdsByDiff[0]; + } + return getters.unresolvedDiscussionsIdsByDate[0]; +}; + // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index ab6a95e2601..e1b159142c9 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -174,27 +174,19 @@ export default { [types.UPDATE_NOTE](state, note) { const noteObj = utils.findNoteObjectById(state.discussions, note.discussion_id); - if (noteObj.individual_note) { noteObj.notes.splice(0, 1, note); } else { const comment = utils.findNoteObjectById(noteObj.notes, note.id); - noteObj.notes.splice(noteObj.notes.indexOf(comment), 1, note); + Object.assign(comment, note); } }, [types.UPDATE_DISCUSSION](state, noteData) { const note = noteData; - let index = 0; - - state.discussions.forEach((n, i) => { - if (n.id === note.id) { - index = i; - } - }); - + const selectedDiscussion = state.discussions.find(n => n.id === note.id); note.expanded = true; // override expand flag to prevent collapse - state.discussions.splice(index, 1, note); + Object.assign(selectedDiscussion, note); }, [types.CLOSE_ISSUE](state) { @@ -215,12 +207,9 @@ export default { [types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) { const discussion = utils.findNoteObjectById(state.discussions, discussionId); - const index = state.discussions.indexOf(discussion); - const discussionWithDiffLines = Object.assign({}, discussion, { + Object.assign(discussion, { truncated_diff_lines: diffLines, }); - - state.discussions.splice(index, 1, discussionWithDiffLines); }, }; diff --git a/app/assets/javascripts/notes/stores/utils.js b/app/assets/javascripts/notes/stores/utils.js index a0e096ebfaf..c4a812c5af4 100644 --- a/app/assets/javascripts/notes/stores/utils.js +++ b/app/assets/javascripts/notes/stores/utils.js @@ -2,13 +2,11 @@ import AjaxCache from '~/lib/utils/ajax_cache'; const REGEX_QUICK_ACTIONS = /^\/\w+.*$/gm; -export const findNoteObjectById = (notes, id) => - notes.filter(n => n.id === id)[0]; +export const findNoteObjectById = (notes, id) => notes.find(n => n.id === id); export const getQuickActionText = note => { let text = 'Applying command'; - const quickActions = - AjaxCache.get(gl.GfmAutoComplete.dataSources.commands) || []; + const quickActions = AjaxCache.get(gl.GfmAutoComplete.dataSources.commands) || []; const executedCommands = quickActions.filter(command => { const commandRegex = new RegExp(`/${command.name}`); @@ -29,5 +27,4 @@ export const getQuickActionText = note => { export const hasQuickActions = note => REGEX_QUICK_ACTIONS.test(note); -export const stripQuickActions = note => - note.replace(REGEX_QUICK_ACTIONS, '').trim(); +export const stripQuickActions = note => note.replace(REGEX_QUICK_ACTIONS, '').trim(); diff --git a/app/serializers/discussion_entity.rb b/app/serializers/discussion_entity.rb index 8a39a4950f5..7505bbdeb3d 100644 --- a/app/serializers/discussion_entity.rb +++ b/app/serializers/discussion_entity.rb @@ -4,6 +4,7 @@ class DiscussionEntity < Grape::Entity expose :id, :reply_id expose :position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? } + expose :original_position, if: -> (d, _) { d.diff_discussion? && !d.legacy_diff_discussion? } expose :line_code, if: -> (d, _) { d.diff_discussion? } expose :expanded?, as: :expanded expose :active?, as: :active, if: -> (d, _) { d.diff_discussion? } diff --git a/changelogs/unreleased/48817-fix-mr-changes-discussion-navigation.yml b/changelogs/unreleased/48817-fix-mr-changes-discussion-navigation.yml new file mode 100644 index 00000000000..ec4b843b863 --- /dev/null +++ b/changelogs/unreleased/48817-fix-mr-changes-discussion-navigation.yml @@ -0,0 +1,5 @@ +--- +title: Fix navigation to First and Next discussion on MR Changes tab +merge_request: 20434 +author: +type: fixed diff --git a/changelogs/unreleased/_acet-fix-expanding-context-lines.yml b/changelogs/unreleased/_acet-fix-expanding-context-lines.yml new file mode 100644 index 00000000000..41b4dbca5d6 --- /dev/null +++ b/changelogs/unreleased/_acet-fix-expanding-context-lines.yml @@ -0,0 +1,5 @@ +--- +title: Fix rendering of the context lines in MR diffs page +merge_request: 20642 +author: +type: fixed diff --git a/changelogs/unreleased/_acet-fix-mr-autosave.yml b/changelogs/unreleased/_acet-fix-mr-autosave.yml new file mode 100644 index 00000000000..f87b32f68e2 --- /dev/null +++ b/changelogs/unreleased/_acet-fix-mr-autosave.yml @@ -0,0 +1,5 @@ +--- +title: Fix autosave and ESC confirmation issues for MR discussions +merge_request: 20569 +author: +type: fixed diff --git a/changelogs/unreleased/_acet-fix-outdated-discussions.yml b/changelogs/unreleased/_acet-fix-outdated-discussions.yml new file mode 100644 index 00000000000..d31483b4765 --- /dev/null +++ b/changelogs/unreleased/_acet-fix-outdated-discussions.yml @@ -0,0 +1,5 @@ +--- +title: Fix showing outdated discussions on Changes tab +merge_request: 20445 +author: +type: fixed diff --git a/changelogs/unreleased/tz-mr-refactor-memory-reduction.yml b/changelogs/unreleased/tz-mr-refactor-memory-reduction.yml new file mode 100644 index 00000000000..16003fa9cad --- /dev/null +++ b/changelogs/unreleased/tz-mr-refactor-memory-reduction.yml @@ -0,0 +1,5 @@ +---
+title: Reduces the client side memory footprint on merge requests
+merge_request: 20744
+author:
+type: performance
diff --git a/locale/gitlab.pot b/locale/gitlab.pot index f2e4eda576f..59dfa04a19d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3190,6 +3190,9 @@ msgstr "" msgid "Note: Consider asking your GitLab administrator to configure %{github_integration_link}, which will allow login via GitHub and allow importing repositories without generating a Personal Access Token." msgstr "" +msgid "Notes|Are you sure you want to cancel creating this comment?" +msgstr "" + msgid "Notification events" 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 a0b9d6cb059..1c7aa674955 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 @@ -342,8 +342,9 @@ describe 'Merge request > User resolves diff notes and discussions', :js do end end - it 'shows jump to next discussion button' do - expect(page.all('.discussion-reply-holder', count: 2)).to all(have_selector('.discussion-next-btn')) + 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) end it 'displays next discussion even if hidden' do diff --git a/spec/javascripts/autosave_spec.js b/spec/javascripts/autosave_spec.js index 38ae5b7e00c..dcb1c781591 100644 --- a/spec/javascripts/autosave_spec.js +++ b/spec/javascripts/autosave_spec.js @@ -59,12 +59,10 @@ describe('Autosave', () => { Autosave.prototype.restore.call(autosave); - expect( - field.trigger, - ).toHaveBeenCalled(); + expect(field.trigger).toHaveBeenCalled(); }); - it('triggers native event', (done) => { + it('triggers native event', done => { autosave.field.get(0).addEventListener('change', () => { done(); }); @@ -81,9 +79,7 @@ describe('Autosave', () => { it('does not trigger event', () => { spyOn(field, 'trigger').and.callThrough(); - expect( - field.trigger, - ).not.toHaveBeenCalled(); + expect(field.trigger).not.toHaveBeenCalled(); }); }); }); diff --git a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js index 2d136a63c52..bdc94131fc2 100644 --- a/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js +++ b/spec/javascripts/diffs/components/diff_line_gutter_content_spec.js @@ -18,10 +18,12 @@ describe('DiffLineGutterContent', () => { }; const setDiscussions = component => { component.$store.dispatch('setInitialNotes', getDiscussionsMockData()); + component.$store.commit('diffs/SET_DIFF_DATA', { diffFiles: [getDiffFileMock()] }); }; const resetDiscussions = component => { component.$store.dispatch('setInitialNotes', []); + component.$store.commit('diffs/SET_DIFF_DATA', {}); }; describe('computed', () => { @@ -48,7 +50,11 @@ describe('DiffLineGutterContent', () => { it('should return discussions for the given lineCode', () => { const { lineCode } = getDiffFileMock().highlightedDiffLines[1]; - const component = createComponent({ lineCode, showCommentButton: true }); + const component = createComponent({ + lineCode, + showCommentButton: true, + discussions: getDiscussionsMockData(), + }); setDiscussions(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 4600aaea70b..6fe5fdaf7f9 100644 --- a/spec/javascripts/diffs/components/diff_line_note_form_spec.js +++ b/spec/javascripts/diffs/components/diff_line_note_form_spec.js @@ -3,6 +3,7 @@ import DiffLineNoteForm from '~/diffs/components/diff_line_note_form.vue'; import store from '~/mr_notes/stores'; import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; import diffFileMockData from '../mock_data/diff_file'; +import { noteableDataMock } from '../../notes/mock_data'; describe('DiffLineNoteForm', () => { let component; @@ -21,10 +22,9 @@ describe('DiffLineNoteForm', () => { noteTargetLine: diffLines[0], }); - Object.defineProperty(component, 'isLoggedIn', { - get() { - return true; - }, + Object.defineProperties(component, { + noteableData: { value: noteableDataMock }, + isLoggedIn: { value: true }, }); component.$mount(); @@ -32,12 +32,37 @@ describe('DiffLineNoteForm', () => { describe('methods', () => { describe('handleCancelCommentForm', () => { - it('should call cancelCommentForm with lineCode', () => { + it('should ask for confirmation when shouldConfirm and isDirty passed as truthy', () => { + spyOn(window, 'confirm').and.returnValue(false); + + component.handleCancelCommentForm(true, true); + expect(window.confirm).toHaveBeenCalled(); + }); + + it('should ask for confirmation when one of the params false', () => { + spyOn(window, 'confirm').and.returnValue(false); + + component.handleCancelCommentForm(true, false); + expect(window.confirm).not.toHaveBeenCalled(); + + component.handleCancelCommentForm(false, true); + expect(window.confirm).not.toHaveBeenCalled(); + }); + + it('should call cancelCommentForm with lineCode', done => { + spyOn(window, 'confirm'); spyOn(component, 'cancelCommentForm'); + spyOn(component, 'resetAutoSave'); component.handleCancelCommentForm(); - expect(component.cancelCommentForm).toHaveBeenCalledWith({ - lineCode: diffLines[0].lineCode, + expect(window.confirm).not.toHaveBeenCalled(); + component.$nextTick(() => { + expect(component.cancelCommentForm).toHaveBeenCalledWith({ + lineCode: diffLines[0].lineCode, + }); + expect(component.resetAutoSave).toHaveBeenCalled(); + + done(); }); }); }); @@ -66,7 +91,7 @@ describe('DiffLineNoteForm', () => { describe('mounted', () => { it('should init autosave', () => { - const key = 'autosave/Note/issue///DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1'; + const key = 'autosave/Note/Issue/98//DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1'; expect(component.autosave).toBeDefined(); expect(component.autosave.key).toEqual(key); diff --git a/spec/javascripts/diffs/components/inline_diff_view_spec.js b/spec/javascripts/diffs/components/inline_diff_view_spec.js index b02328dd359..90dfa5c5a58 100644 --- a/spec/javascripts/diffs/components/inline_diff_view_spec.js +++ b/spec/javascripts/diffs/components/inline_diff_view_spec.js @@ -33,6 +33,7 @@ describe('InlineDiffView', () => { it('should render discussions', done => { const el = component.$el; component.$store.dispatch('setInitialNotes', getDiscussionsMockData()); + component.$store.commit('diffs/SET_DIFF_DATA', { diffFiles: [getDiffFileMock()] }); Vue.nextTick(() => { expect(el.querySelectorAll('.notes_holder').length).toEqual(1); diff --git a/spec/javascripts/diffs/mock_data/diff_discussions.js b/spec/javascripts/diffs/mock_data/diff_discussions.js index 41d0dfd8939..8cd57d2248b 100644 --- a/spec/javascripts/diffs/mock_data/diff_discussions.js +++ b/spec/javascripts/diffs/mock_data/diff_discussions.js @@ -12,6 +12,17 @@ export default { head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13', }, }, + original_position: { + formatter: { + old_line: null, + new_line: 2, + old_path: 'CHANGELOG', + new_path: 'CHANGELOG', + base_sha: 'e63f41fe459e62e1228fcef60d7189127aeba95a', + start_sha: 'd9eaefe5a676b820c57ff18cf5b68316025f7962', + head_sha: 'c48ee0d1bf3b30453f5b32250ce03134beaa6d13', + }, + }, line_code: '1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2', expanded: true, notes: [ diff --git a/spec/javascripts/diffs/store/getters_spec.js b/spec/javascripts/diffs/store/getters_spec.js index 6210d4a7124..f5628a01a55 100644 --- a/spec/javascripts/diffs/store/getters_spec.js +++ b/spec/javascripts/diffs/store/getters_spec.js @@ -2,6 +2,7 @@ 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'; +import diffFile from '../mock_data/diff_file'; describe('Diffs Module Getters', () => { let localState; @@ -203,4 +204,38 @@ describe('Diffs Module Getters', () => { expect(getters.getDiffFileByHash(localState)('123')).toBeUndefined(); }); }); + + describe('discussionsByLineCode', () => { + let mockState; + + beforeEach(() => { + mockState = { diffFiles: [diffFile] }; + }); + + it('should return a map of diff lines with their line codes', () => { + const mockGetters = { discussions: [discussionMock] }; + + const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters); + expect(map['1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2']).toBeDefined(); + expect(Object.keys(map).length).toEqual(1); + }); + + it('should have the diff discussion on the map if the original position matches', () => { + discussionMock.position.formatter.base_sha = 'ff9200'; + const mockGetters = { discussions: [discussionMock] }; + + const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters); + expect(map['1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_2']).toBeDefined(); + expect(Object.keys(map).length).toEqual(1); + }); + + it('should not add an outdated diff discussion to the returned map', () => { + discussionMock.position.formatter.base_sha = 'ff9200'; + discussionMock.original_position.formatter.base_sha = 'ff9200'; + const mockGetters = { discussions: [discussionMock] }; + + const map = getters.discussionsByLineCode(mockState, {}, {}, mockGetters); + expect(Object.keys(map).length).toEqual(0); + }); + }); }); diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js index 32136d9ebff..8e7bd8afca4 100644 --- a/spec/javascripts/diffs/store/utils_spec.js +++ b/spec/javascripts/diffs/store/utils_spec.js @@ -207,4 +207,24 @@ describe('DiffsStoreUtils', () => { expect(utils.trimFirstCharOfLineContent()).toEqual({}); }); }); + + describe('getDiffRefsByLineCode', () => { + it('should return diffRefs for all highlightedDiffLines', () => { + const diffFile = getDiffFileMock(); + const map = utils.getDiffRefsByLineCode([diffFile]); + const { highlightedDiffLines } = diffFile; + const lineCodeCount = highlightedDiffLines.reduce( + (acc, line) => (line.lineCode ? acc + 1 : acc), + 0, + ); + + const { baseSha, headSha, startSha } = diffFile.diffRefs; + const targetLine = map[highlightedDiffLines[4].lineCode]; + + expect(Object.keys(map).length).toEqual(lineCodeCount); + expect(targetLine.baseSha).toEqual(baseSha); + expect(targetLine.headSha).toEqual(headSha); + expect(targetLine.startSha).toEqual(startSha); + }); + }); }); diff --git a/spec/javascripts/notes/components/discussion_counter_spec.js b/spec/javascripts/notes/components/discussion_counter_spec.js index a3869cc6498..d09bc5037ef 100644 --- a/spec/javascripts/notes/components/discussion_counter_spec.js +++ b/spec/javascripts/notes/components/discussion_counter_spec.js @@ -46,7 +46,7 @@ describe('DiscussionCounter component', () => { discussions, }); setFixtures(` - <div data-discussion-id="${firstDiscussionId}"></div> + <div class="discussion" data-discussion-id="${firstDiscussionId}"></div> `); vm.jumpToFirstUnresolvedDiscussion(); diff --git a/spec/javascripts/notes/components/noteable_discussion_spec.js b/spec/javascripts/notes/components/noteable_discussion_spec.js index 7da931fd9cb..2a01bd85520 100644 --- a/spec/javascripts/notes/components/noteable_discussion_spec.js +++ b/spec/javascripts/notes/components/noteable_discussion_spec.js @@ -14,6 +14,7 @@ describe('noteable_discussion component', () => { preloadFixtures(discussionWithTwoUnresolvedNotes); beforeEach(() => { + window.mrTabs = {}; store = createStore(); store.dispatch('setNoteableData', noteableDataMock); store.dispatch('setNotesData', notesDataMock); @@ -46,10 +47,15 @@ describe('noteable_discussion component', () => { it('should toggle reply form', done => { vm.$el.querySelector('.js-vue-discussion-reply').click(); + Vue.nextTick(() => { - expect(vm.$refs.noteForm).not.toBeNull(); expect(vm.isReplying).toEqual(true); - done(); + + // There is a watcher for `isReplying` which will init autosave in the next tick + Vue.nextTick(() => { + expect(vm.$refs.noteForm).not.toBeNull(); + done(); + }); }); }); @@ -101,33 +107,29 @@ describe('noteable_discussion component', () => { describe('methods', () => { describe('jumpToNextDiscussion', () => { - it('expands next unresolved discussion', () => { - spyOn(vm, 'expandDiscussion').and.stub(); - const discussions = [ - discussionMock, - { - ...discussionMock, - id: discussionMock.id + 1, - notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: true }], - }, - { - ...discussionMock, - id: discussionMock.id + 2, - notes: [{ ...discussionMock.notes[0], resolvable: true, resolved: false }], - }, - ]; - const nextDiscussionId = discussionMock.id + 2; - store.replaceState({ - ...store.state, - discussions, - }); - setFixtures(` - <div data-discussion-id="${nextDiscussionId}"></div> - `); + it('expands next unresolved discussion', done => { + const discussion2 = getJSONFixture(discussionWithTwoUnresolvedNotes)[0]; + discussion2.resolved = false; + discussion2.id = 'next'; // prepare this for being identified as next one (to be jumped to) + vm.$store.dispatch('setInitialNotes', [discussionMock, discussion2]); + window.mrTabs.currentAction = 'show'; + + Vue.nextTick() + .then(() => { + spyOn(vm, 'expandDiscussion').and.stub(); + + const nextDiscussionId = discussion2.id; + + setFixtures(` + <div class="discussion" data-discussion-id="${nextDiscussionId}"></div> + `); - vm.jumpToNextDiscussion(); + vm.jumpToNextDiscussion(); - expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId }); + expect(vm.expandDiscussion).toHaveBeenCalledWith({ discussionId: nextDiscussionId }); + }) + .then(done) + .catch(done.fail); }); }); }); diff --git a/spec/javascripts/notes/mock_data.js b/spec/javascripts/notes/mock_data.js index be2a8ba67fe..67f6a9629d9 100644 --- a/spec/javascripts/notes/mock_data.js +++ b/spec/javascripts/notes/mock_data.js @@ -1168,3 +1168,87 @@ export const collapsedSystemNotes = [ diff_discussion: false, }, ]; + +export const discussion1 = { + id: 'abc1', + resolvable: true, + resolved: false, + diff_file: { + file_path: 'about.md', + }, + position: { + formatter: { + new_line: 50, + old_line: null, + }, + }, + notes: [ + { + created_at: '2018-07-04T16:25:41.749Z', + }, + ], +}; + +export const resolvedDiscussion1 = { + id: 'abc1', + resolvable: true, + resolved: true, + diff_file: { + file_path: 'about.md', + }, + position: { + formatter: { + new_line: 50, + old_line: null, + }, + }, + notes: [ + { + created_at: '2018-07-04T16:25:41.749Z', + }, + ], +}; + +export const discussion2 = { + id: 'abc2', + resolvable: true, + resolved: false, + diff_file: { + file_path: 'README.md', + }, + position: { + formatter: { + new_line: null, + old_line: 20, + }, + }, + notes: [ + { + created_at: '2018-07-04T12:05:41.749Z', + }, + ], +}; + +export const discussion3 = { + id: 'abc3', + resolvable: true, + resolved: false, + diff_file: { + file_path: 'README.md', + }, + position: { + formatter: { + new_line: 21, + old_line: null, + }, + }, + notes: [ + { + created_at: '2018-07-05T17:25:41.749Z', + }, + ], +}; + +export const unresolvableDiscussion = { + resolvable: false, +}; diff --git a/spec/javascripts/notes/stores/getters_spec.js b/spec/javascripts/notes/stores/getters_spec.js index 41599e00122..7f8ede51508 100644 --- a/spec/javascripts/notes/stores/getters_spec.js +++ b/spec/javascripts/notes/stores/getters_spec.js @@ -5,6 +5,11 @@ import { noteableDataMock, individualNote, collapseNotesMock, + discussion1, + discussion2, + discussion3, + resolvedDiscussion1, + unresolvableDiscussion, } from '../mock_data'; const discussionWithTwoUnresolvedNotes = 'merge_requests/resolved_diff_discussion.json'; @@ -109,4 +114,154 @@ describe('Getters Notes Store', () => { expect(getters.isNotesFetched(state)).toBeFalsy(); }); }); + + describe('allResolvableDiscussions', () => { + it('should return only resolvable discussions in same order', () => { + const localGetters = { + allDiscussions: [ + discussion3, + unresolvableDiscussion, + discussion1, + unresolvableDiscussion, + discussion2, + ], + }; + + expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([ + discussion3, + discussion1, + discussion2, + ]); + }); + + it('should return empty array if there are no resolvable discussions', () => { + const localGetters = { + allDiscussions: [unresolvableDiscussion, unresolvableDiscussion], + }; + + expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([]); + }); + }); + + describe('unresolvedDiscussionsIdsByDiff', () => { + it('should return all discussions IDs in diff order', () => { + const localGetters = { + allResolvableDiscussions: [discussion3, discussion1, discussion2], + }; + + expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([ + 'abc1', + 'abc2', + 'abc3', + ]); + }); + + it('should return empty array if all discussions have been resolved', () => { + const localGetters = { + allResolvableDiscussions: [resolvedDiscussion1], + }; + + expect(getters.unresolvedDiscussionsIdsByDiff(state, localGetters)).toEqual([]); + }); + }); + + describe('unresolvedDiscussionsIdsByDate', () => { + it('should return all discussions in date ascending order', () => { + const localGetters = { + allResolvableDiscussions: [discussion3, discussion1, discussion2], + }; + + expect(getters.unresolvedDiscussionsIdsByDate(state, localGetters)).toEqual([ + 'abc2', + 'abc1', + 'abc3', + ]); + }); + + it('should return empty array if all discussions have been resolved', () => { + const localGetters = { + allResolvableDiscussions: [resolvedDiscussion1], + }; + + expect(getters.unresolvedDiscussionsIdsByDate(state, localGetters)).toEqual([]); + }); + }); + + describe('unresolvedDiscussionsIdsOrdered', () => { + const localGetters = { + unresolvedDiscussionsIdsByDate: ['123', '456'], + unresolvedDiscussionsIdsByDiff: ['abc', 'def'], + }; + + it('should return IDs ordered by diff when diffOrder param is true', () => { + expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(true)).toEqual([ + 'abc', + 'def', + ]); + }); + + it('should return IDs ordered by date when diffOrder param is not true', () => { + expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(false)).toEqual([ + '123', + '456', + ]); + expect(getters.unresolvedDiscussionsIdsOrdered(state, localGetters)(undefined)).toEqual([ + '123', + '456', + ]); + }); + }); + + describe('isLastUnresolvedDiscussion', () => { + const localGetters = { + unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], + }; + + it('should return true if the discussion id provided is the last', () => { + expect(getters.isLastUnresolvedDiscussion(state, localGetters)('789')).toBe(true); + }); + + it('should return false if the discussion id provided is not the last', () => { + expect(getters.isLastUnresolvedDiscussion(state, localGetters)('123')).toBe(false); + expect(getters.isLastUnresolvedDiscussion(state, localGetters)('456')).toBe(false); + }); + }); + + describe('nextUnresolvedDiscussionId', () => { + const localGetters = { + unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'], + }; + + 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); + }); + }); + + describe('firstUnresolvedDiscussionId', () => { + const localGetters = { + unresolvedDiscussionsIdsByDate: ['123', '456'], + unresolvedDiscussionsIdsByDiff: ['abc', 'def'], + }; + + it('should return the first discussion id by diff when diffOrder param is true', () => { + expect(getters.firstUnresolvedDiscussionId(state, localGetters)(true)).toBe('abc'); + }); + + it('should return the first discussion id by date when diffOrder param is not true', () => { + expect(getters.firstUnresolvedDiscussionId(state, localGetters)(false)).toBe('123'); + expect(getters.firstUnresolvedDiscussionId(state, localGetters)(undefined)).toBe('123'); + }); + + it('should be falsy if all discussions are resolved', () => { + const localGettersFalsy = { + unresolvedDiscussionsIdsByDiff: [], + unresolvedDiscussionsIdsByDate: [], + }; + + expect(getters.firstUnresolvedDiscussionId(state, localGettersFalsy)(true)).toBeFalsy(); + expect(getters.firstUnresolvedDiscussionId(state, localGettersFalsy)(false)).toBeFalsy(); + }); + }); }); |