Skip to content

G
gitlab-ce
Commit adf8ad9e authored by Phil Hughes's avatar Phil Hughes
Browse files
Options

Improve discussion rendering performance 

Improve the renderign of new and existing discussions
by reducing the number of watchers on each object & array.
Previously every discussion change would trigger an update for every
discussion component.

Also tidied up some components to get them closer to our docs.

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