Fix up some notes tech debtfix-up-notes-technical-debt

Fix up tech debt comments from @jschatz1

 - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10837#note_28757436
 - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10837#note_28757905
 - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10837#note_28757952
 - https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10837#note_28758013

2 files changed, 115 insertions, 109 deletions

diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js
index b0b1cfd6c8a..c10fcded26a 100644
--- a/app/assets/javascripts/notes.js
+++ b/app/assets/javascripts/notes.js
@@ -1,4 +1,10 @@
-/* eslint-disable no-restricted-properties, func-names, space-before-function-paren, no-var, prefer-rest-params, wrap-iife, no-use-before-define, camelcase, no-unused-expressions, quotes, max-len, one-var, one-var-declaration-per-line, default-case, prefer-template, consistent-return, no-alert, no-return-assign, no-param-reassign, prefer-arrow-callback, no-else-return, comma-dangle, no-new, brace-style, no-lonely-if, vars-on-top, no-unused-vars, no-sequences, no-shadow, newline-per-chained-call, no-useless-escape */
+/* eslint-disable no-restricted-properties, func-names, space-before-function-paren,
+no-var, prefer-rest-params, wrap-iife, no-use-before-define, camelcase,
+no-unused-expressions, quotes, max-len, one-var, one-var-declaration-per-line,
+default-case, prefer-template, consistent-return, no-alert, no-return-assign,
+no-param-reassign, prefer-arrow-callback, no-else-return, comma-dangle, no-new,
+brace-style, no-lonely-if, vars-on-top, no-unused-vars, no-sequences, no-shadow,
+newline-per-chained-call, no-useless-escape */
 /* global Flash */
 /* global Autosave */
 /* global ResolveService */
@@ -57,7 +63,7 @@ const normalizeNewlines = function(str) {
       this.updatedNotesTrackingMap = {};
       this.last_fetched_at = last_fetched_at;
       this.noteable_url = document.URL;
-      this.notesCountBadge || (this.notesCountBadge = $(".issuable-details").find(".notes-tab .badge"));
+      this.notesCountBadge || (this.notesCountBadge = $('.issuable-details').find('.notes-tab .badge'));
       this.basePollingInterval = 15000;
       this.maxPollingSteps = 4;
       this.flashErrors = [];
@@ -87,61 +93,61 @@ const normalizeNewlines = function(str) {
 
     Notes.prototype.addBinding = function() {
       // Edit note link
-      $(document).on("click", ".js-note-edit", this.showEditForm.bind(this));
-      $(document).on("click", ".note-edit-cancel", this.cancelEdit);
+      $(document).on('click', '.js-note-edit', this.showEditForm.bind(this));
+      $(document).on('click', '.note-edit-cancel', this.cancelEdit);
       // Reopen and close actions for Issue/MR combined with note form submit
-      $(document).on("click", ".js-comment-submit-button", this.postComment);
-      $(document).on("click", ".js-comment-save-button", this.updateComment);
-      $(document).on("keyup input", ".js-note-text", this.updateTargetButtons);
+      $(document).on('click', '.js-comment-submit-button', this.postComment);
+      $(document).on('click', '.js-comment-save-button', this.updateComment);
+      $(document).on('keyup input', '.js-note-text', this.updateTargetButtons);
       // resolve a discussion
       $(document).on('click', '.js-comment-resolve-button', this.postComment);
       // remove a note (in general)
-      $(document).on("click", ".js-note-delete", this.removeNote);
+      $(document).on('click', '.js-note-delete', this.removeNote);
       // delete note attachment
-      $(document).on("click", ".js-note-attachment-delete", this.removeAttachment);
+      $(document).on('click', '.js-note-attachment-delete', this.removeAttachment);
       // reset main target form when clicking discard
-      $(document).on("click", ".js-note-discard", this.resetMainTargetForm);
+      $(document).on('click', '.js-note-discard', this.resetMainTargetForm);
       // update the file name when an attachment is selected
-      $(document).on("change", ".js-note-attachment-input", this.updateFormAttachment);
+      $(document).on('change', '.js-note-attachment-input', this.updateFormAttachment);
       // reply to diff/discussion notes
-      $(document).on("click", ".js-discussion-reply-button", this.onReplyToDiscussionNote);
+      $(document).on('click', '.js-discussion-reply-button', this.onReplyToDiscussionNote);
       // add diff note
-      $(document).on("click", ".js-add-diff-note-button", this.onAddDiffNote);
+      $(document).on('click', '.js-add-diff-note-button', this.onAddDiffNote);
       // hide diff note form
-      $(document).on("click", ".js-close-discussion-note-form", this.cancelDiscussionForm);
+      $(document).on('click', '.js-close-discussion-note-form', this.cancelDiscussionForm);
       // toggle commit list
-      $(document).on("click", '.system-note-commit-list-toggler', this.toggleCommitList);
+      $(document).on('click', '.system-note-commit-list-toggler', this.toggleCommitList);
       // fetch notes when tab becomes visible
-      $(document).on("visibilitychange", this.visibilityChange);
+      $(document).on('visibilitychange', this.visibilityChange);
       // when issue status changes, we need to refresh data
-      $(document).on("issuable:change", this.refresh);
+      $(document).on('issuable:change', this.refresh);
       // ajax:events that happen on Form when actions like Reopen, Close are performed on Issues and MRs.
-      $(document).on("ajax:success", ".js-main-target-form", this.addNote);
-      $(document).on("ajax:success", ".js-discussion-note-form", this.addDiscussionNote);
-      $(document).on("ajax:success", ".js-main-target-form", this.resetMainTargetForm);
-      $(document).on("ajax:complete", ".js-main-target-form", this.reenableTargetFormSubmitButton);
+      $(document).on('ajax:success', '.js-main-target-form', this.addNote);
+      $(document).on('ajax:success', '.js-discussion-note-form', this.addDiscussionNote);
+      $(document).on('ajax:success', '.js-main-target-form', this.resetMainTargetForm);
+      $(document).on('ajax:complete', '.js-main-target-form', this.reenableTargetFormSubmitButton);
       // when a key is clicked on the notes
-      return $(document).on("keydown", ".js-note-text", this.keydownNoteText);
+      return $(document).on('keydown', '.js-note-text', this.keydownNoteText);
     };
 
     Notes.prototype.cleanBinding = function() {
-      $(document).off("click", ".js-note-edit");
-      $(document).off("click", ".note-edit-cancel");
-      $(document).off("click", ".js-note-delete");
-      $(document).off("click", ".js-note-attachment-delete");
-      $(document).off("click", ".js-discussion-reply-button");
-      $(document).off("click", ".js-add-diff-note-button");
-      $(document).off("visibilitychange");
-      $(document).off("keyup input", ".js-note-text");
-      $(document).off("click", ".js-note-target-reopen");
-      $(document).off("click", ".js-note-target-close");
-      $(document).off("click", ".js-note-discard");
-      $(document).off("keydown", ".js-note-text");
+      $(document).off('click', '.js-note-edit');
+      $(document).off('click', '.note-edit-cancel');
+      $(document).off('click', '.js-note-delete');
+      $(document).off('click', '.js-note-attachment-delete');
+      $(document).off('click', '.js-discussion-reply-button');
+      $(document).off('click', '.js-add-diff-note-button');
+      $(document).off('visibilitychange');
+      $(document).off('keyup input', '.js-note-text');
+      $(document).off('click', '.js-note-target-reopen');
+      $(document).off('click', '.js-note-target-close');
+      $(document).off('click', '.js-note-discard');
+      $(document).off('keydown', '.js-note-text');
       $(document).off('click', '.js-comment-resolve-button');
-      $(document).off("click", '.system-note-commit-list-toggler');
-      $(document).off("ajax:success", ".js-main-target-form");
-      $(document).off("ajax:success", ".js-discussion-note-form");
-      $(document).off("ajax:complete", ".js-main-target-form");
+      $(document).off('click', '.system-note-commit-list-toggler');
+      $(document).off('ajax:success', '.js-main-target-form');
+      $(document).off('ajax:success', '.js-discussion-note-form');
+      $(document).off('ajax:complete', '.js-main-target-form');
     };
 
     Notes.initCommentTypeToggle = function (form) {
@@ -231,8 +237,8 @@ const normalizeNewlines = function(str) {
       this.refreshing = true;
       return $.ajax({
         url: this.notes_url,
-        headers: { "X-Last-Fetched-At": this.last_fetched_at },
-        dataType: "json",
+        headers: { 'X-Last-Fetched-At': this.last_fetched_at },
+        dataType: 'json',
         success: (function(_this) {
           return function(data) {
             var notes;
@@ -303,7 +309,7 @@ const normalizeNewlines = function(str) {
      */
 
     Notes.prototype.renderNote = function(noteEntity, $form, $notesList = $('.main-notes-list')) {
-      if (noteEntity.discussion_html != null) {
+      if (noteEntity.discussion_html) {
         return this.renderDiscussionNote(noteEntity, $form);
       }
 
@@ -368,8 +374,8 @@ const normalizeNewlines = function(str) {
         return;
       }
       this.note_ids.push(noteEntity.id);
-      form = $form || $(".js-discussion-note-form[data-discussion-id='" + noteEntity.discussion_id + "']");
-      row = form.closest("tr");
+      form = $form || $(`.js-discussion-note-form[data-discussion-id="${noteEntity.discussion_id}"]`);
+      row = form.closest('tr');
       lineType = this.isParallelView() ? form.find('#line_type').val() : 'old';
       diffAvatarContainer = row.prevAll('.line_holder').first().find('.js-avatar-container.' + lineType + '_line');
       // is this the first note of discussion?
@@ -386,7 +392,7 @@ const normalizeNewlines = function(str) {
             row.after($discussion);
           } else {
             // Merge new discussion HTML in
-            var $notes = $discussion.find('.notes[data-discussion-id="' + noteEntity.discussion_id + '"]');
+            var $notes = $discussion.find(`.notes[data-discussion-id="${noteEntity.discussion_id}"]`);
             var contentContainerClass = '.' + $notes.closest('.notes_content')
               .attr('class')
               .split(' ')
@@ -397,7 +403,7 @@ const normalizeNewlines = function(str) {
         }
         // Init discussion on 'Discussion' page if it is merge request page
         const page = $('body').attr('data-page');
-        if ((page && page.indexOf('projects:merge_request') === 0) || !noteEntity.diff_discussion_html) {
+        if ((page && page.indexOf('projects:merge_request') !== -1) || !noteEntity.diff_discussion_html) {
           Notes.animateAppendNote(noteEntity.discussion_html, $('.main-notes-list'));
         }
       } else {
@@ -450,13 +456,13 @@ const normalizeNewlines = function(str) {
 
     Notes.prototype.resetMainTargetForm = function(e) {
       var form;
-      form = $(".js-main-target-form");
+      form = $('.js-main-target-form');
       // remove validation errors
-      form.find(".js-errors").remove();
+      form.find('.js-errors').remove();
       // reset text and preview
-      form.find(".js-md-write-button").click();
-      form.find(".js-note-text").val("").trigger("input");
-      form.find(".js-note-text").data("autosave").reset();
+      form.find('.js-md-write-button').click();
+      form.find('.js-note-text').val('').trigger('input');
+      form.find('.js-note-text').data('autosave').reset();
 
       var event = document.createEvent('Event');
       event.initEvent('autosize:update', true, false);
@@ -467,8 +473,8 @@ const normalizeNewlines = function(str) {
 
     Notes.prototype.reenableTargetFormSubmitButton = function() {
       var form;
-      form = $(".js-main-target-form");
-      return form.find(".js-note-text").trigger("input");
+      form = $('.js-main-target-form');
+      return form.find('.js-note-text').trigger('input');
     };
 
     /*
@@ -480,18 +486,18 @@ const normalizeNewlines = function(str) {
     Notes.prototype.setupMainTargetNoteForm = function() {
       var form;
       // find the form
-      form = $(".js-new-note-form");
+      form = $('.js-new-note-form');
       // Set a global clone of the form for later cloning
       this.formClone = form.clone();
       // show the form
       this.setupNoteForm(form);
       // fix classes
-      form.removeClass("js-new-note-form");
-      form.addClass("js-main-target-form");
-      form.find("#note_line_code").remove();
-      form.find("#note_position").remove();
-      form.find("#note_type").val('');
-      form.find("#in_reply_to_discussion_id").remove();
+      form.removeClass('js-new-note-form');
+      form.addClass('js-main-target-form');
+      form.find('#note_line_code').remove();
+      form.find('#note_position').remove();
+      form.find('#note_type').val('');
+      form.find('#in_reply_to_discussion_id').remove();
       form.find('.js-comment-resolve-button').closest('comment-and-resolve-btn').remove();
       this.parentTimeline = form.parents('.timeline');
 
@@ -512,20 +518,20 @@ const normalizeNewlines = function(str) {
     Notes.prototype.setupNoteForm = function(form) {
       var textarea, key;
       new gl.GLForm(form, this.enableGFM);
-      textarea = form.find(".js-note-text");
+      textarea = form.find('.js-note-text');
       key = [
-        "Note",
-        form.find("#note_noteable_type").val(),
-        form.find("#note_noteable_id").val(),
-        form.find("#note_commit_id").val(),
-        form.find("#note_type").val(),
-        form.find("#in_reply_to_discussion_id").val(),
+        'Note',
+        form.find('#note_noteable_type').val(),
+        form.find('#note_noteable_id').val(),
+        form.find('#note_commit_id').val(),
+        form.find('#note_type').val(),
+        form.find('#in_reply_to_discussion_id').val(),
 
         // LegacyDiffNote
-        form.find("#note_line_code").val(),
+        form.find('#note_line_code').val(),
 
         // DiffNote
-        form.find("#note_position").val()
+        form.find('#note_position').val()
       ];
       return new Autosave(textarea, key);
     };
@@ -670,7 +676,8 @@ const normalizeNewlines = function(str) {
         const $newNote = $(this.updatedNotesTrackingMap[noteId].html);
         $note.replaceWith($newNote);
         this.setupNewNote($newNote);
-        this.updatedNotesTrackingMap[noteId] = null;
+        // Now that we have taken care of the update, clear it out
+        delete this.updatedNotesTrackingMap[noteId];
       }
       else {
         $note.find('.js-finish-edit-warning').hide();
@@ -722,14 +729,14 @@ const normalizeNewlines = function(str) {
       lineHolder = $(e.currentTarget).closest('.notes[data-discussion-id]')
         .closest('.notes_holder')
         .prev('.line_holder');
-      $(".note[id='" + noteElId + "']").each((function(_this) {
+      $(`.note[id="${noteElId}"]`).each((function(_this) {
         // 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.
+        // to remove all. Using $('.note[id='noteId']') ensure we get all the notes,
+        // where $('#noteId') would return only one.
         return function(i, el) {
           var $note, $notes;
           $note = $(el);
-          $notes = $note.closest(".discussion-notes");
+          $notes = $note.closest('.discussion-notes');
 
           if (typeof gl.diffNotesCompileComponents !== 'undefined') {
             if (gl.diffNoteApps[noteElId]) {
@@ -740,11 +747,11 @@ const normalizeNewlines = function(str) {
           $note.remove();
 
           // check if this is the last note for this line
-          if ($notes.find(".note").length === 0) {
-            var notesTr = $notes.closest("tr");
+          if ($notes.find('.note').length === 0) {
+            var notesTr = $notes.closest('tr');
 
             // "Discussions" tab
-            $notes.closest(".timeline-entry").remove();
+            $notes.closest('.timeline-entry').remove();
 
             // The notes tr can contain multiple lists of notes, like on the parallel diff
             if (notesTr.find('.discussion-notes').length > 1) {
@@ -768,11 +775,11 @@ const normalizeNewlines = function(str) {
      */
 
     Notes.prototype.removeAttachment = function() {
-      const $note = $(this).closest(".note");
-      $note.find(".note-attachment").remove();
-      $note.find(".note-body > .note-text").show();
-      $note.find(".note-header").show();
-      return $note.find(".current-note-edit-form").remove();
+      const $note = $(this).closest('.note');
+      $note.find('.note-attachment').remove();
+      $note.find('.note-body > .note-text').show();
+      $note.find('.note-header').show();
+      return $note.find('.current-note-edit-form').remove();
     };
 
     /*
@@ -788,7 +795,7 @@ const normalizeNewlines = function(str) {
     Notes.prototype.replyToDiscussionNote = function(target) {
       var form, replyLink;
       form = this.cleanForm(this.formClone.clone());
-      replyLink = $(target).closest(".js-discussion-reply-button");
+      replyLink = $(target).closest('.js-discussion-reply-button');
       // insert the form after the button
       replyLink
         .closest('.discussion-reply-holder')
@@ -808,26 +815,26 @@ const normalizeNewlines = function(str) {
 
     Notes.prototype.setupDiscussionNoteForm = function(dataHolder, form) {
       // setup note target
-      var discussionID = dataHolder.data("discussionId");
+      var discussionID = dataHolder.data('discussionId');
 
       if (discussionID) {
-        form.attr("data-discussion-id", discussionID);
-        form.find("#in_reply_to_discussion_id").val(discussionID);
+        form.attr('data-discussion-id', discussionID);
+        form.find('#in_reply_to_discussion_id').val(discussionID);
       }
 
-      form.attr("data-line-code", dataHolder.data("lineCode"));
-      form.find("#line_type").val(dataHolder.data("lineType"));
+      form.attr('data-line-code', dataHolder.data('lineCode'));
+      form.find('#line_type').val(dataHolder.data('lineType'));
 
-      form.find("#note_noteable_type").val(dataHolder.data("noteableType"));
-      form.find("#note_noteable_id").val(dataHolder.data("noteableId"));
-      form.find("#note_commit_id").val(dataHolder.data("commitId"));
-      form.find("#note_type").val(dataHolder.data("noteType"));
+      form.find('#note_noteable_type').val(dataHolder.data('noteableType'));
+      form.find('#note_noteable_id').val(dataHolder.data('noteableId'));
+      form.find('#note_commit_id').val(dataHolder.data('commitId'));
+      form.find('#note_type').val(dataHolder.data('noteType'));
 
       // LegacyDiffNote
-      form.find("#note_line_code").val(dataHolder.data("lineCode"));
+      form.find('#note_line_code').val(dataHolder.data('lineCode'));
 
       // DiffNote
-      form.find("#note_position").val(dataHolder.attr("data-position"));
+      form.find('#note_position').val(dataHolder.attr('data-position'));
 
       form.find('.js-note-discard').show().removeClass('js-note-discard').addClass('js-close-discussion-note-form').text(form.find('.js-close-discussion-note-form').data('cancel-text'));
       form.find('.js-note-target-close').remove();
@@ -836,7 +843,7 @@ const normalizeNewlines = function(str) {
 
       form
         .removeClass('js-main-target-form')
-        .addClass("discussion-form js-discussion-note-form");
+        .addClass('discussion-form js-discussion-note-form');
 
       if (typeof gl.diffNotesCompileComponents !== 'undefined') {
         var $commentBtn = form.find('comment-and-resolve-btn');
@@ -845,7 +852,7 @@ const normalizeNewlines = function(str) {
         gl.diffNotesCompileComponents();
       }
 
-      form.find(".js-note-text").focus();
+      form.find('.js-note-text').focus();
       form
         .find('.js-comment-resolve-button')
         .attr('data-discussion-id', discussionID);
@@ -878,21 +885,21 @@ const normalizeNewlines = function(str) {
     }) {
       var $link, addForm, hasNotes, newForm, noteForm, replyButton, row, rowCssToAdd, targetContent, isDiffCommentAvatar;
       $link = $(target);
-      row = $link.closest("tr");
+      row = $link.closest('tr');
       const nextRow = row.next();
       let targetRow = row;
       if (nextRow.is('.notes_holder')) {
         targetRow = nextRow;
       }
 
-      hasNotes = targetRow.is(".notes_holder");
+      hasNotes = nextRow.is('.notes_holder');
       addForm = false;
       let lineTypeSelector = '';
-      rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line\" colspan=\"2\"></td><td class=\"notes_content\"><div class=\"content\"></div></td></tr>";
+      rowCssToAdd = '<tr class="notes_holder js-temp-notes-holder"><td class="notes_line" colspan="2"></td><td class="notes_content"><div class="content"></div></td></tr>';
       // In parallel view, look inside the correct left/right pane
       if (this.isParallelView()) {
         lineTypeSelector = `.${lineType}`;
-        rowCssToAdd = "<tr class=\"notes_holder js-temp-notes-holder\"><td class=\"notes_line old\"></td><td class=\"notes_content parallel old\"><div class=\"content\"></div></td><td class=\"notes_line new\"></td><td class=\"notes_content parallel new\"><div class=\"content\"></div></td></tr>";
+        rowCssToAdd = '<tr class="notes_holder js-temp-notes-holder"><td class="notes_line old"></td><td class="notes_content parallel old"><div class="content"></div></td><td class="notes_line new"></td><td class="notes_content parallel new"><div class="content"></div></td></tr>';
       }
       const notesContentSelector = `.notes_content${lineTypeSelector} .content`;
       let notesContent = targetRow.find(notesContentSelector);
@@ -902,12 +909,12 @@ const normalizeNewlines = function(str) {
         notesContent = targetRow.find(notesContentSelector);
         if (notesContent.length) {
           notesContent.show();
-          replyButton = notesContent.find(".js-discussion-reply-button:visible");
+          replyButton = notesContent.find('.js-discussion-reply-button:visible');
           if (replyButton.length) {
             this.replyToDiscussionNote(replyButton[0]);
           } else {
             // In parallel view, the form may not be present in one of the panes
-            noteForm = notesContent.find(".js-discussion-note-form");
+            noteForm = notesContent.find('.js-discussion-note-form');
             if (noteForm.length === 0) {
               addForm = true;
             }
@@ -945,15 +952,15 @@ const normalizeNewlines = function(str) {
 
     Notes.prototype.removeDiscussionNoteForm = function(form) {
       var glForm, row;
-      row = form.closest("tr");
+      row = form.closest('tr');
       glForm = form.data('gl-form');
       glForm.destroy();
-      form.find(".js-note-text").data("autosave").reset();
+      form.find('.js-note-text').data('autosave').reset();
       // show the reply button (will only work for replies)
       form
         .prev('.discussion-reply-holder')
         .show();
-      if (row.is(".js-temp-notes-holder")) {
+      if (row.is('.js-temp-notes-holder')) {
         // remove temporary row for diff lines
         return row.remove();
       } else {
@@ -965,7 +972,7 @@ const normalizeNewlines = function(str) {
     Notes.prototype.cancelDiscussionForm = function(e) {
       var form;
       e.preventDefault();
-      form = $(e.target).closest(".js-discussion-note-form");
+      form = $(e.target).closest('.js-discussion-note-form');
       return this.removeDiscussionNoteForm(form);
     };
 
@@ -977,10 +984,10 @@ const normalizeNewlines = function(str) {
 
     Notes.prototype.updateFormAttachment = function() {
       var filename, form;
-      form = $(this).closest("form");
+      form = $(this).closest('form');
       // get only the basename
-      filename = $(this).val().replace(/^.*[\\\/]/, "");
-      return form.find(".js-attachment-filename").text(filename);
+      filename = $(this).val().replace(/^.*[\\\/]/, '');
+      return form.find('.js-attachment-filename').text(filename);
     };
 
     /*
diff --git a/spec/javascripts/notes_spec.js b/spec/javascripts/notes_spec.js
index 025f08ee332..04cf0fe2bf8 100644
--- a/spec/javascripts/notes_spec.js
+++ b/spec/javascripts/notes_spec.js
@@ -128,7 +128,6 @@ import '~/notes';
       beforeEach(() => {
         note = {
           id: 1,
-          discussion_html: null,
           valid: true,
           note: 'heya',
           html: '<div>heya</div>',