Commit 5ba542b1 authored by Tim Zallmann's avatar Tim Zallmann

Merge branch '_acet-fix-mr-autosave' into 'master'

Fix autosave issues for MR discussions

Closes #48876 and #48877

See merge request gitlab-org/gitlab-ce!20569
parents 32c831ea 9e24e93e
...@@ -53,4 +53,8 @@ export default class Autosave { ...@@ -53,4 +53,8 @@ export default class Autosave {
return window.localStorage.removeItem(this.key); return window.localStorage.removeItem(this.key);
} }
dispose() {
this.field.off('input');
}
} }
<script> <script>
import $ from 'jquery';
import { mapState, mapGetters, mapActions } from 'vuex'; import { mapState, mapGetters, mapActions } from 'vuex';
import createFlash from '~/flash'; import createFlash from '~/flash';
import { s__ } from '~/locale'; import { s__ } from '~/locale';
import noteForm from '../../notes/components/note_form.vue'; import noteForm from '../../notes/components/note_form.vue';
import { getNoteFormData } from '../store/utils'; import { getNoteFormData } from '../store/utils';
import Autosave from '../../autosave'; import autosave from '../../notes/mixins/autosave';
import { DIFF_NOTE_TYPE, NOTE_TYPE } from '../constants'; import { DIFF_NOTE_TYPE } from '../constants';
export default { export default {
components: { components: {
noteForm, noteForm,
}, },
mixins: [autosave],
props: { props: {
diffFileHash: { diffFileHash: {
type: String, type: String,
...@@ -41,28 +41,35 @@ export default { ...@@ -41,28 +41,35 @@ export default {
}, },
mounted() { mounted() {
if (this.isLoggedIn) { if (this.isLoggedIn) {
const noteableData = this.getNoteableData;
const keys = [ const keys = [
NOTE_TYPE, this.noteableData.diff_head_sha,
this.noteableType,
noteableData.id,
noteableData.diff_head_sha,
DIFF_NOTE_TYPE, DIFF_NOTE_TYPE,
noteableData.source_project_id, this.noteableData.source_project_id,
this.line.lineCode, this.line.lineCode,
]; ];
this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys); this.initAutoSave(this.noteableData, keys);
} }
}, },
methods: { methods: {
...mapActions('diffs', ['cancelCommentForm']), ...mapActions('diffs', ['cancelCommentForm']),
...mapActions(['saveNote', 'refetchDiscussionById']), ...mapActions(['saveNote', 'refetchDiscussionById']),
handleCancelCommentForm() { handleCancelCommentForm(shouldConfirm, isDirty) {
this.autosave.reset(); if (shouldConfirm && isDirty) {
const msg = s__('Notes|Are you sure you want to cancel creating this comment?');
// eslint-disable-next-line no-alert
if (!window.confirm(msg)) {
return;
}
}
this.cancelCommentForm({ this.cancelCommentForm({
lineCode: this.line.lineCode, lineCode: this.line.lineCode,
}); });
this.$nextTick(() => {
this.resetAutoSave();
});
}, },
handleSaveNote(note) { handleSaveNote(note) {
const selectedDiffFile = this.getDiffFileByHash(this.diffFileHash); const selectedDiffFile = this.getDiffFileByHash(this.diffFileHash);
......
...@@ -7,7 +7,7 @@ import issuableStateMixin from '../mixins/issuable_state'; ...@@ -7,7 +7,7 @@ import issuableStateMixin from '../mixins/issuable_state';
import resolvable from '../mixins/resolvable'; import resolvable from '../mixins/resolvable';
export default { export default {
name: 'IssueNoteForm', name: 'NoteForm',
components: { components: {
issueWarning, issueWarning,
markdownField, markdownField,
......
...@@ -6,6 +6,7 @@ import nextDiscussionsSvg from 'icons/_next_discussion.svg'; ...@@ -6,6 +6,7 @@ import nextDiscussionsSvg from 'icons/_next_discussion.svg';
import { convertObjectPropsToCamelCase, scrollToElement } from '~/lib/utils/common_utils'; import { convertObjectPropsToCamelCase, scrollToElement } from '~/lib/utils/common_utils';
import { truncateSha } from '~/lib/utils/text_utility'; import { truncateSha } from '~/lib/utils/text_utility';
import systemNote from '~/vue_shared/components/notes/system_note.vue'; import systemNote from '~/vue_shared/components/notes/system_note.vue';
import { s__ } from '~/locale';
import Flash from '../../flash'; import Flash from '../../flash';
import { SYSTEM_NOTE } from '../constants'; import { SYSTEM_NOTE } from '../constants';
import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue'; import userAvatarLink from '../../vue_shared/components/user_avatar/user_avatar_link.vue';
...@@ -144,19 +145,17 @@ export default { ...@@ -144,19 +145,17 @@ export default {
return this.isDiffDiscussion ? '' : 'card discussion-wrapper'; return this.isDiffDiscussion ? '' : 'card discussion-wrapper';
}, },
}, },
mounted() { watch: {
if (this.isReplying) { isReplying() {
this.initAutoSave(this.transformedDiscussion); if (this.isReplying) {
} this.$nextTick(() => {
}, // Pass an extra key to separate reply and note edit forms
updated() { this.initAutoSave(this.transformedDiscussion, ['Reply']);
if (this.isReplying) { });
if (!this.autosave) {
this.initAutoSave(this.transformedDiscussion);
} else { } else {
this.setAutoSave(); this.disposeAutoSave();
} }
} },
}, },
created() { created() {
this.resolveDiscussionsSvg = resolveDiscussionsSvg; this.resolveDiscussionsSvg = resolveDiscussionsSvg;
...@@ -194,16 +193,18 @@ export default { ...@@ -194,16 +193,18 @@ export default {
showReplyForm() { showReplyForm() {
this.isReplying = true; this.isReplying = true;
}, },
cancelReplyForm(shouldConfirm) { cancelReplyForm(shouldConfirm, isDirty) {
if (shouldConfirm && this.$refs.noteForm.isDirty) { if (shouldConfirm && isDirty) {
const msg = s__('Notes|Are you sure you want to cancel creating this comment?');
// eslint-disable-next-line no-alert // eslint-disable-next-line no-alert
if (!window.confirm('Are you sure you want to cancel creating this comment?')) { if (!window.confirm(msg)) {
return; return;
} }
} }
this.resetAutoSave();
this.isReplying = false; this.isReplying = false;
this.resetAutoSave();
}, },
saveReply(noteText, form, callback) { saveReply(noteText, form, callback) {
const postData = { const postData = {
...@@ -420,7 +421,8 @@ Please check your network connection and try again.`; ...@@ -420,7 +421,8 @@ Please check your network connection and try again.`;
:is-editing="false" :is-editing="false"
save-button-title="Comment" save-button-title="Comment"
@handleFormUpdate="saveReply" @handleFormUpdate="saveReply"
@cancelForm="cancelReplyForm" /> @cancelForm="cancelReplyForm"
/>
<note-signed-out-widget v-if="!canReply" /> <note-signed-out-widget v-if="!canReply" />
</div> </div>
</div> </div>
......
...@@ -4,12 +4,18 @@ import { capitalizeFirstCharacter } from '../../lib/utils/text_utility'; ...@@ -4,12 +4,18 @@ import { capitalizeFirstCharacter } from '../../lib/utils/text_utility';
export default { export default {
methods: { methods: {
initAutoSave(noteable) { initAutoSave(noteable, extraKeys = []) {
this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), [ let keys = [
'Note', 'Note',
capitalizeFirstCharacter(noteable.noteable_type), capitalizeFirstCharacter(noteable.noteable_type || noteable.noteableType),
noteable.id, noteable.id,
]); ];
if (extraKeys) {
keys = keys.concat(extraKeys);
}
this.autosave = new Autosave($(this.$refs.noteForm.$refs.textarea), keys);
}, },
resetAutoSave() { resetAutoSave() {
this.autosave.reset(); this.autosave.reset();
...@@ -17,5 +23,8 @@ export default { ...@@ -17,5 +23,8 @@ export default {
setAutoSave() { setAutoSave() {
this.autosave.save(); this.autosave.save();
}, },
disposeAutoSave() {
this.autosave.dispose();
},
}, },
}; };
---
title: Fix autosave and ESC confirmation issues for MR discussions
merge_request: 20569
author:
type: fixed
...@@ -3523,6 +3523,9 @@ msgstr "" ...@@ -3523,6 +3523,9 @@ msgstr ""
msgid "Note: Consider asking your GitLab administrator to configure %{github_integration_link}, which will allow login via GitHub and allow importing repositories without generating a Personal Access Token." msgid "Note: Consider asking your GitLab administrator to configure %{github_integration_link}, which will allow login via GitHub and allow importing repositories without generating a Personal Access Token."
msgstr "" msgstr ""
msgid "Notes|Are you sure you want to cancel creating this comment?"
msgstr ""
msgid "Notification events" msgid "Notification events"
msgstr "" msgstr ""
......
...@@ -59,12 +59,10 @@ describe('Autosave', () => { ...@@ -59,12 +59,10 @@ describe('Autosave', () => {
Autosave.prototype.restore.call(autosave); Autosave.prototype.restore.call(autosave);
expect( expect(field.trigger).toHaveBeenCalled();
field.trigger,
).toHaveBeenCalled();
}); });
it('triggers native event', (done) => { it('triggers native event', done => {
autosave.field.get(0).addEventListener('change', () => { autosave.field.get(0).addEventListener('change', () => {
done(); done();
}); });
...@@ -81,9 +79,7 @@ describe('Autosave', () => { ...@@ -81,9 +79,7 @@ describe('Autosave', () => {
it('does not trigger event', () => { it('does not trigger event', () => {
spyOn(field, 'trigger').and.callThrough(); spyOn(field, 'trigger').and.callThrough();
expect( expect(field.trigger).not.toHaveBeenCalled();
field.trigger,
).not.toHaveBeenCalled();
}); });
}); });
}); });
......
...@@ -3,6 +3,7 @@ import DiffLineNoteForm from '~/diffs/components/diff_line_note_form.vue'; ...@@ -3,6 +3,7 @@ import DiffLineNoteForm from '~/diffs/components/diff_line_note_form.vue';
import store from '~/mr_notes/stores'; import store from '~/mr_notes/stores';
import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper'; import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
import diffFileMockData from '../mock_data/diff_file'; import diffFileMockData from '../mock_data/diff_file';
import { noteableDataMock } from '../../notes/mock_data';
describe('DiffLineNoteForm', () => { describe('DiffLineNoteForm', () => {
let component; let component;
...@@ -21,10 +22,9 @@ describe('DiffLineNoteForm', () => { ...@@ -21,10 +22,9 @@ describe('DiffLineNoteForm', () => {
noteTargetLine: diffLines[0], noteTargetLine: diffLines[0],
}); });
Object.defineProperty(component, 'isLoggedIn', { Object.defineProperties(component, {
get() { noteableData: { value: noteableDataMock },
return true; isLoggedIn: { value: true },
},
}); });
component.$mount(); component.$mount();
...@@ -32,12 +32,37 @@ describe('DiffLineNoteForm', () => { ...@@ -32,12 +32,37 @@ describe('DiffLineNoteForm', () => {
describe('methods', () => { describe('methods', () => {
describe('handleCancelCommentForm', () => { describe('handleCancelCommentForm', () => {
it('should call cancelCommentForm with lineCode', () => { it('should ask for confirmation when shouldConfirm and isDirty passed as truthy', () => {
spyOn(window, 'confirm').and.returnValue(false);
component.handleCancelCommentForm(true, true);
expect(window.confirm).toHaveBeenCalled();
});
it('should ask for confirmation when one of the params false', () => {
spyOn(window, 'confirm').and.returnValue(false);
component.handleCancelCommentForm(true, false);
expect(window.confirm).not.toHaveBeenCalled();
component.handleCancelCommentForm(false, true);
expect(window.confirm).not.toHaveBeenCalled();
});
it('should call cancelCommentForm with lineCode', done => {
spyOn(window, 'confirm');
spyOn(component, 'cancelCommentForm'); spyOn(component, 'cancelCommentForm');
spyOn(component, 'resetAutoSave');
component.handleCancelCommentForm(); component.handleCancelCommentForm();
expect(component.cancelCommentForm).toHaveBeenCalledWith({ expect(window.confirm).not.toHaveBeenCalled();
lineCode: diffLines[0].lineCode, component.$nextTick(() => {
expect(component.cancelCommentForm).toHaveBeenCalledWith({
lineCode: diffLines[0].lineCode,
});
expect(component.resetAutoSave).toHaveBeenCalled();
done();
}); });
}); });
}); });
...@@ -66,7 +91,7 @@ describe('DiffLineNoteForm', () => { ...@@ -66,7 +91,7 @@ describe('DiffLineNoteForm', () => {
describe('mounted', () => { describe('mounted', () => {
it('should init autosave', () => { it('should init autosave', () => {
const key = 'autosave/Note/issue///DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1'; const key = 'autosave/Note/Issue/98//DiffNote//1c497fbb3a46b78edf04cc2a2fa33f67e3ffbe2a_1_1';
expect(component.autosave).toBeDefined(); expect(component.autosave).toBeDefined();
expect(component.autosave.key).toEqual(key); expect(component.autosave.key).toEqual(key);
......
...@@ -46,10 +46,15 @@ describe('noteable_discussion component', () => { ...@@ -46,10 +46,15 @@ describe('noteable_discussion component', () => {
it('should toggle reply form', done => { it('should toggle reply form', done => {
vm.$el.querySelector('.js-vue-discussion-reply').click(); vm.$el.querySelector('.js-vue-discussion-reply').click();
Vue.nextTick(() => { Vue.nextTick(() => {
expect(vm.$refs.noteForm).not.toBeNull();
expect(vm.isReplying).toEqual(true); expect(vm.isReplying).toEqual(true);
done();
// There is a watcher for `isReplying` which will init autosave in the next tick
Vue.nextTick(() => {
expect(vm.$refs.noteForm).not.toBeNull();
done();
});
}); });
}); });
......
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