diff options
author | Filipa Lacerda <filipa@gitlab.com> | 2018-08-01 13:42:23 +0000 |
---|---|---|
committer | Filipa Lacerda <filipa@gitlab.com> | 2018-08-01 13:42:23 +0000 |
commit | 47244ad5ea4e887ecb6dffa9f7b96846adbf4b6f (patch) | |
tree | 823af4478325945020f36411fa315ae345918faf /app/assets/javascripts | |
parent | 5f742eb95a0080343167469ccabfeccd3630007d (diff) | |
parent | 09c1b008eb4b90c0a8becdf7ebb5723a8bd05468 (diff) | |
download | gitlab-ce-47244ad5ea4e887ecb6dffa9f7b96846adbf4b6f.tar.gz |
Merge branch 'andr3-remove-mr-regressions-fixes-from-master' into 'master'
Remove fixes for MR refactor regressions from master
See merge request gitlab-org/gitlab-ce!20920
Diffstat (limited to 'app/assets/javascripts')
22 files changed, 196 insertions, 400 deletions
diff --git a/app/assets/javascripts/autosave.js b/app/assets/javascripts/autosave.js index e8c59fab609..fa00a3cf386 100644 --- a/app/assets/javascripts/autosave.js +++ b/app/assets/javascripts/autosave.js @@ -53,8 +53,4 @@ 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 e64d5511d78..20483161033 100644 --- a/app/assets/javascripts/diffs/components/diff_discussions.vue +++ b/app/assets/javascripts/diffs/components/diff_discussions.vue @@ -30,7 +30,6 @@ 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 d184a76f038..ad838a32518 100644 --- a/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue +++ b/app/assets/javascripts/diffs/components/diff_line_gutter_content.vue @@ -71,18 +71,13 @@ 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']), + ...mapGetters(['isLoggedIn', 'discussionsByLineCode']), lineHref() { return this.lineCode ? `#${this.lineCode}` : '#'; }, @@ -92,19 +87,24 @@ export default { this.showCommentButton && !this.isMatchLine && !this.isContextLine && - !this.isMetaLine && - !this.hasDiscussions + !this.hasDiscussions && + !this.isMetaLine ); }, + 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) { - return false; + render = false; } - return this.hasDiscussions && this.showCommentButton; + return render; }, }, methods: { @@ -189,6 +189,7 @@ 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 cbe4551d06b..32f9516d332 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 '../../notes/mixins/autosave'; -import { DIFF_NOTE_TYPE } from '../constants'; +import Autosave from '../../autosave'; +import { DIFF_NOTE_TYPE, NOTE_TYPE } from '../constants'; export default { components: { noteForm, }, - mixins: [autosave], props: { diffFileHash: { type: String, @@ -41,35 +41,28 @@ export default { }, mounted() { if (this.isLoggedIn) { + const noteableData = this.getNoteableData; const keys = [ - this.noteableData.diff_head_sha, + NOTE_TYPE, + this.noteableType, + noteableData.id, + noteableData.diff_head_sha, DIFF_NOTE_TYPE, - this.noteableData.source_project_id, + noteableData.source_project_id, this.line.lineCode, ]; - this.initAutoSave(this.noteableData, keys); + this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys); } }, methods: { ...mapActions('diffs', ['cancelCommentForm']), ...mapActions(['saveNote', 'refetchDiscussionById']), - 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; - } - } - + handleCancelCommentForm() { + this.autosave.reset(); 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 e8e8ddc6c5e..5962f30d9bb 100644 --- a/app/assets/javascripts/diffs/components/diff_table_cell.vue +++ b/app/assets/javascripts/diffs/components/diff_table_cell.vue @@ -67,11 +67,6 @@ export default { required: false, default: false, }, - discussions: { - type: Array, - required: false, - default: () => [], - }, }, computed: { ...mapGetters(['isLoggedIn']), @@ -141,7 +136,6 @@ 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 1b5ae5e9f75..ca265dd892c 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 } from 'vuex'; +import { mapState, mapGetters } from 'vuex'; import diffDiscussions from './diff_discussions.vue'; import diffLineNoteForm from './diff_line_note_form.vue'; @@ -21,22 +21,18 @@ 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> @@ -57,7 +53,7 @@ export default { :discussions="discussions" /> <diff-line-note-form - v-if="hasCommentForm" + v-if="diffLineCommentForms[line.lineCode]" :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 32d65ff994f..0197a510ef1 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_table_row.vue @@ -33,11 +33,6 @@ export default { required: false, default: false, }, - discussions: { - type: Array, - required: false, - default: () => [], - }, }, data() { return { @@ -94,7 +89,6 @@ export default { :is-bottom="isBottom" :is-hover="isHover" :show-comment-button="true" - :discussions="discussions" class="diff-line-num old_line" /> <diff-table-cell @@ -104,10 +98,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 5f30cc57a59..9fd19b74cd7 100644 --- a/app/assets/javascripts/diffs/components/inline_diff_view.vue +++ b/app/assets/javascripts/diffs/components/inline_diff_view.vue @@ -20,11 +20,8 @@ export default { }, }, computed: { - ...mapGetters('diffs', [ - 'commitId', - 'shouldRenderInlineCommentRow', - 'singleDiscussionByLineCode', - ]), + ...mapGetters('diffs', ['commitId']), + ...mapGetters(['discussionsByLineCode']), ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), @@ -38,7 +35,18 @@ export default { return window.gon.user_color_scheme; }, }, - methods: {}, + 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); + }, + }, }; </script> @@ -57,15 +65,13 @@ export default { :line="line" :is-bottom="index + 1 === diffLinesLength" :key="line.lineCode" - :discussions="singleDiscussionByLineCode(line.lineCode)" /> <inline-diff-comment-row - v-if="shouldRenderInlineCommentRow(line)" + v-if="shouldRenderCommentRow(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 bb9a65c83fa..cc5248c25d9 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 } from 'vuex'; +import { mapState, mapGetters } from 'vuex'; import diffDiscussions from './diff_discussions.vue'; import diffLineNoteForm from './diff_line_note_form.vue'; @@ -21,51 +21,48 @@ 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.leftDiscussions; + const discussions = this.discussionsByLineCode[this.leftLineCode]; + return discussions ? discussions.every(discussion => discussion.expanded) : false; }, hasExpandedDiscussionOnRight() { - const discussions = this.rightDiscussions; + const discussions = this.discussionsByLineCode[this.rightLineCode]; + return discussions ? discussions.every(discussion => discussion.expanded) : false; }, hasAnyExpandedDiscussion() { return this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight; }, shouldRenderDiscussionsOnLeft() { - return this.leftDiscussions && this.hasExpandedDiscussionOnLeft; + return this.discussionsByLineCode[this.leftLineCode] && this.hasExpandedDiscussionOnLeft; }, shouldRenderDiscussionsOnRight() { - return this.rightDiscussions && this.hasExpandedDiscussionOnRight && this.line.right.type; - }, - showRightSideCommentForm() { - return this.line.right.type && this.diffLineCommentForms[this.rightLineCode]; + return ( + this.discussionsByLineCode[this.rightLineCode] && + this.hasExpandedDiscussionOnRight && + this.line.right.type + ); }, className() { - return this.leftDiscussions.length > 0 || this.rightDiscussions.length > 0 - ? '' - : 'js-temp-notes-holder'; + return this.hasDiscussion ? '' : 'js-temp-notes-holder'; }, }, }; @@ -83,12 +80,13 @@ export default { class="content" > <diff-discussions - v-if="leftDiscussions.length" - :discussions="leftDiscussions" + v-if="discussionsByLineCode[leftLineCode].length" + :discussions="discussionsByLineCode[leftLineCode]" /> </div> <diff-line-note-form - v-if="diffLineCommentForms[leftLineCode]" + v-if="diffLineCommentForms[leftLineCode] && + diffLineCommentForms[leftLineCode]" :diff-file-hash="diffFileHash" :line="line.left" :note-target-line="line.left" @@ -102,12 +100,13 @@ export default { class="content" > <diff-discussions - v-if="rightDiscussions.length" - :discussions="rightDiscussions" + v-if="discussionsByLineCode[rightLineCode].length" + :discussions="discussionsByLineCode[rightLineCode]" /> </div> <diff-line-note-form - v-if="showRightSideCommentForm" + v-if="diffLineCommentForms[rightLineCode] && + diffLineCommentForms[rightLineCode] && line.right.type" :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 d4e54c2bd00..ee5bb4d8d05 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_table_row.vue @@ -36,16 +36,6 @@ export default { required: false, default: false, }, - leftDiscussions: { - type: Array, - required: false, - default: () => [], - }, - rightDiscussions: { - type: Array, - required: false, - default: () => [], - }, }, data() { return { @@ -126,10 +116,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" @@ -147,10 +137,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 4d97cb6d15d..32528c9e7ab 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_view.vue @@ -21,11 +21,8 @@ export default { }, }, computed: { - ...mapGetters('diffs', [ - 'commitId', - 'singleDiscussionByLineCode', - 'shouldRenderParallelCommentRow', - ]), + ...mapGetters('diffs', ['commitId']), + ...mapGetters(['discussionsByLineCode']), ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), @@ -55,6 +52,32 @@ 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> @@ -75,17 +98,13 @@ 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="shouldRenderParallelCommentRow(line)" + v-if="shouldRenderCommentRow(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 fc1d15b8d54..9aec117c236 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -1,7 +1,5 @@ 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; @@ -66,87 +64,6 @@ 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 82082ac508a..d9589baa76e 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -173,24 +173,3 @@ 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 04a6948f1f1..198711cf427 100644 --- a/app/assets/javascripts/lib/utils/poll.js +++ b/app/assets/javascripts/lib/utils/poll.js @@ -63,7 +63,6 @@ export default class Poll { const headers = normalizeHeaders(response.headers); const pollInterval = parseInt(headers[this.intervalHeader], 10); if (pollInterval > 0 && successCodes.indexOf(response.status) !== -1 && this.canPoll) { - clearTimeout(this.timeoutID); this.timeoutID = setTimeout(() => { this.makeRequest(); }, pollInterval); diff --git a/app/assets/javascripts/notes/components/discussion_counter.vue b/app/assets/javascripts/notes/components/discussion_counter.vue index ad6e7cf501d..6385b75e557 100644 --- a/app/assets/javascripts/notes/components/discussion_counter.vue +++ b/app/assets/javascripts/notes/components/discussion_counter.vue @@ -5,20 +5,19 @@ 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 discussionNavigation from '../mixins/discussion_navigation'; +import { scrollToElement } from '../../lib/utils/common_utils'; import tooltip from '../../vue_shared/directives/tooltip'; export default { directives: { tooltip, }, - mixins: [discussionNavigation], computed: { ...mapGetters([ 'getUserData', 'getNoteableData', 'discussionCount', - 'firstUnresolvedDiscussionId', + 'unresolvedDiscussions', 'resolvedDiscussionCount', ]), isLoggedIn() { @@ -36,6 +35,11 @@ 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; @@ -46,10 +50,22 @@ export default { methods: { ...mapActions(['expandDiscussion']), jumpToFirstUnresolvedDiscussion() { - const diffTab = window.mrTabs.currentAction === 'diffs'; - const discussionId = this.firstUnresolvedDiscussionId(diffTab); + 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'); + } - this.jumpToDiscussion(discussionId); + if (el) { + this.expandDiscussion({ discussionId }); + scrollToElement(el); + } }, }, }; diff --git a/app/assets/javascripts/notes/components/note_form.vue b/app/assets/javascripts/notes/components/note_form.vue index abcd4422d7c..26482a02e00 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: 'NoteForm', + name: 'IssueNoteForm', components: { issueWarning, markdownField, diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index 0fe1c16854a..bee635398b3 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 } from '~/lib/utils/common_utils'; +import { convertObjectPropsToCamelCase, scrollToElement } 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,7 +20,6 @@ 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 { @@ -40,7 +39,7 @@ export default { directives: { tooltip, }, - mixins: [autosave, noteable, resolvable, discussionNavigation], + mixins: [autosave, noteable, resolvable], props: { discussion: { type: Object, @@ -61,11 +60,6 @@ export default { required: false, default: false, }, - discussionsByDiffOrder: { - type: Boolean, - required: false, - default: false, - }, }, data() { return { @@ -80,12 +74,7 @@ export default { 'discussionCount', 'resolvedDiscussionCount', 'allDiscussions', - 'unresolvedDiscussionsIdsByDiff', - 'unresolvedDiscussionsIdsByDate', 'unresolvedDiscussions', - 'unresolvedDiscussionsIdsOrdered', - 'nextUnresolvedDiscussionId', - 'isLastUnresolvedDiscussion', ]), transformedDiscussion() { return { @@ -136,10 +125,6 @@ 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; @@ -159,17 +144,19 @@ export default { return this.isDiffDiscussion ? '' : 'card discussion-wrapper'; }, }, - watch: { - isReplying() { - if (this.isReplying) { - this.$nextTick(() => { - // Pass an extra key to separate reply and note edit forms - this.initAutoSave(this.transformedDiscussion, ['Reply']); - }); + mounted() { + if (this.isReplying) { + this.initAutoSave(this.transformedDiscussion); + } + }, + updated() { + if (this.isReplying) { + if (!this.autosave) { + this.initAutoSave(this.transformedDiscussion); } else { - this.disposeAutoSave(); + this.setAutoSave(); } - }, + } }, created() { this.resolveDiscussionsSvg = resolveDiscussionsSvg; @@ -207,18 +194,16 @@ export default { showReplyForm() { this.isReplying = true; }, - cancelReplyForm(shouldConfirm, isDirty) { - if (shouldConfirm && isDirty) { - const msg = s__('Notes|Are you sure you want to cancel creating this comment?'); - + cancelReplyForm(shouldConfirm) { + if (shouldConfirm && this.$refs.noteForm.isDirty) { // eslint-disable-next-line no-alert - if (!window.confirm(msg)) { + if (!window.confirm('Are you sure you want to cancel creating this comment?')) { return; } } - this.isReplying = false; this.resetAutoSave(); + this.isReplying = false; }, saveReply(noteText, form, callback) { const postData = { @@ -256,10 +241,21 @@ Please check your network connection and try again.`; }); }, jumpToNextDiscussion() { - const nextId = - this.nextUnresolvedDiscussionId(this.discussion.id, this.discussionsByDiffOrder); + 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}"]`); - this.jumpToDiscussion(nextId); + if (el) { + this.expandDiscussion({ discussionId: nextId }); + scrollToElement(el); + } + } }, }, }; @@ -401,7 +397,7 @@ Please check your network connection and try again.`; </a> </div> <div - v-if="showJumpToNextDiscussion" + v-if="hasMultipleUnresolvedDiscussions" class="btn-group" role="group"> <button @@ -424,8 +420,7 @@ 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 4f45f912479..36cc8d5d056 100644 --- a/app/assets/javascripts/notes/mixins/autosave.js +++ b/app/assets/javascripts/notes/mixins/autosave.js @@ -4,18 +4,12 @@ import { capitalizeFirstCharacter } from '../../lib/utils/text_utility'; export default { methods: { - initAutoSave(noteable, extraKeys = []) { - let keys = [ + initAutoSave(noteable) { + this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), [ 'Note', - capitalizeFirstCharacter(noteable.noteable_type || noteable.noteableType), + capitalizeFirstCharacter(noteable.noteable_type), noteable.id, - ]; - - if (extraKeys) { - keys = keys.concat(extraKeys); - } - - this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys); + ]); }, resetAutoSave() { this.autosave.reset(); @@ -23,8 +17,5 @@ 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 deleted file mode 100644 index f7c4deee1f8..00000000000 --- a/app/assets/javascripts/notes/mixins/discussion_navigation.js +++ /dev/null @@ -1,29 +0,0 @@ -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 0d8d197bf71..5c65e1c3bb5 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -28,6 +28,18 @@ 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; @@ -70,9 +82,6 @@ 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 = {}; @@ -89,51 +98,6 @@ 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; @@ -150,42 +114,5 @@ 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 e1b159142c9..ab6a95e2601 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -174,19 +174,27 @@ 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); - Object.assign(comment, note); + noteObj.notes.splice(noteObj.notes.indexOf(comment), 1, note); } }, [types.UPDATE_DISCUSSION](state, noteData) { const note = noteData; - const selectedDiscussion = state.discussions.find(n => n.id === note.id); + let index = 0; + + state.discussions.forEach((n, i) => { + if (n.id === note.id) { + index = i; + } + }); + note.expanded = true; // override expand flag to prevent collapse - Object.assign(selectedDiscussion, note); + state.discussions.splice(index, 1, note); }, [types.CLOSE_ISSUE](state) { @@ -207,9 +215,12 @@ export default { [types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) { const discussion = utils.findNoteObjectById(state.discussions, discussionId); + const index = state.discussions.indexOf(discussion); - Object.assign(discussion, { + const discussionWithDiffLines = 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 c4a812c5af4..a0e096ebfaf 100644 --- a/app/assets/javascripts/notes/stores/utils.js +++ b/app/assets/javascripts/notes/stores/utils.js @@ -2,11 +2,13 @@ import AjaxCache from '~/lib/utils/ajax_cache'; const REGEX_QUICK_ACTIONS = /^\/\w+.*$/gm; -export const findNoteObjectById = (notes, id) => notes.find(n => n.id === id); +export const findNoteObjectById = (notes, id) => + notes.filter(n => n.id === id)[0]; 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}`); @@ -27,4 +29,5 @@ 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(); |