Commit 7f2fdfaa authored by Lukas 'Eipi' Eipert's avatar Lukas 'Eipi' Eipert Committed by Jose Ivan Vargas

Fix deletion of deprecated notes

Notes in Snippets / Commits are deleted from the DOM even before the
user confirmed the deletion. With `bootstrap_confirmation_modals`
enabled this actually caused a bug, because the element is
progamatically clicked after confirmation. But as the element is removed
from the DOM already, Rails/UJS didn't intercept the click properly.

If we switch from a click handler to an ajax:success handler _and_
remove the note from the DOM after a successful deletion, this problem
is resolved.
parent ce45fe71
...@@ -143,7 +143,7 @@ export default class Notes { ...@@ -143,7 +143,7 @@ export default class Notes {
// resolve a discussion // resolve a discussion
this.$wrapperEl.on('click', '.js-comment-resolve-button', this.postComment); this.$wrapperEl.on('click', '.js-comment-resolve-button', this.postComment);
// remove a note (in general) // remove a note (in general)
this.$wrapperEl.on('click', '.js-note-delete', this.removeNote); this.$wrapperEl.on('ajax:success', '.js-note-delete', this.removeNote);
// delete note attachment // delete note attachment
this.$wrapperEl.on('click', '.js-note-attachment-delete', this.removeAttachment); this.$wrapperEl.on('click', '.js-note-attachment-delete', this.removeAttachment);
// update the file name when an attachment is selected // update the file name when an attachment is selected
...@@ -188,7 +188,7 @@ export default class Notes { ...@@ -188,7 +188,7 @@ export default class Notes {
cleanBinding() { cleanBinding() {
this.$wrapperEl.off('click', '.js-note-edit'); this.$wrapperEl.off('click', '.js-note-edit');
this.$wrapperEl.off('click', '.note-edit-cancel'); this.$wrapperEl.off('click', '.note-edit-cancel');
this.$wrapperEl.off('click', '.js-note-delete'); this.$wrapperEl.off('ajax:success', '.js-note-delete');
this.$wrapperEl.off('click', '.js-note-attachment-delete'); this.$wrapperEl.off('click', '.js-note-attachment-delete');
this.$wrapperEl.off('click', '.js-discussion-reply-button'); this.$wrapperEl.off('click', '.js-discussion-reply-button');
this.$wrapperEl.off('click', '.js-add-diff-note-button'); this.$wrapperEl.off('click', '.js-add-diff-note-button');
...@@ -827,50 +827,53 @@ export default class Notes { ...@@ -827,50 +827,53 @@ export default class Notes {
*/ */
removeNote(e) { removeNote(e) {
const $note = $(e.currentTarget).closest('.note'); const $note = $(e.currentTarget).closest('.note');
const noteElId = $note.attr('id');
$(`.note[id="${noteElId}"]`).each((i, el) => {
// A same note appears in the "Discussion" and in the "Changes" tab, we have
// to remove all. Using $('.note[id='noteId']') ensure we get all the notes,
// where $('#noteId') would return only one.
const $note = $(el);
const $notes = $note.closest('.discussion-notes');
const discussionId = $('.notes', $notes).data('discussionId');
$note.remove();
// check if this is the last note for this line
if ($notes.find('.note').length === 0) {
const notesTr = $notes.closest('tr');
// "Discussions" tab
$notes.closest('.timeline-entry').remove();
$(`.js-diff-avatars-${discussionId}`).trigger('remove.vue');
// The notes tr can contain multiple lists of notes, like on the parallel diff
// notesTr does not exist for image diffs
if (notesTr.find('.discussion-notes').length > 1 || notesTr.length === 0) {
const $diffFile = $notes.closest('.diff-file');
if ($diffFile.length > 0) {
const removeBadgeEvent = new CustomEvent('removeBadge.imageDiff', {
detail: {
// badgeNumber's start with 1 and index starts with 0
badgeNumber: $notes.index() + 1,
},
});
$diffFile[0].dispatchEvent(removeBadgeEvent); $note.one('ajax:complete', () => {
} const noteElId = $note.attr('id');
$(`.note[id="${noteElId}"]`).each((i, el) => {
// A same note appears in the "Discussion" and in the "Changes" tab, we have
// to remove all. Using $('.note[id='noteId']') ensure we get all the notes,
// where $('#noteId') would return only one.
const $note = $(el);
const $notes = $note.closest('.discussion-notes');
const discussionId = $('.notes', $notes).data('discussionId');
$note.remove();
// check if this is the last note for this line
if ($notes.find('.note').length === 0) {
const notesTr = $notes.closest('tr');
// "Discussions" tab
$notes.closest('.timeline-entry').remove();
$(`.js-diff-avatars-${discussionId}`).trigger('remove.vue');
// The notes tr can contain multiple lists of notes, like on the parallel diff
// notesTr does not exist for image diffs
if (notesTr.find('.discussion-notes').length > 1 || notesTr.length === 0) {
const $diffFile = $notes.closest('.diff-file');
if ($diffFile.length > 0) {
const removeBadgeEvent = new CustomEvent('removeBadge.imageDiff', {
detail: {
// badgeNumber's start with 1 and index starts with 0
badgeNumber: $notes.index() + 1,
},
});
$diffFile[0].dispatchEvent(removeBadgeEvent);
}
$notes.remove(); $notes.remove();
} else if (notesTr.length > 0) { } else if (notesTr.length > 0) {
notesTr.remove(); notesTr.remove();
}
} }
} });
});
Notes.checkMergeRequestStatus(); Notes.checkMergeRequestStatus();
return this.updateNotesCount(-1); return this.updateNotesCount(-1);
});
} }
/** /**
......
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