Merge branch '32449-fix-note-comparison-trailing-newline-edge-case' into 'master'

Fix note flicker with note you can't edit and trailing new line edge case Closes #32449 See merge request !11472
2017-05-18 09:39:29 +00:00 · 2017-05-18 09:39:29 +00:00 · 010e1dacab
commit 010e1dacab
parent 994532683d f21a315590
5 changed files with 156 additions and 75 deletions
--- a/app/assets/javascripts/notes.js
+++ b/app/assets/javascripts/notes.js
@ -307,7 +307,7 @@ const normalizeNewlines = function(str) {
      }

      const $note = $notesList.find(`#note_${noteEntity.id}`);
-      if (this.isNewNote(noteEntity)) {
+      if (Notes.isNewNote(noteEntity, this.note_ids)) {
        this.note_ids.push(noteEntity.id);

        const $newNote = Notes.animateAppendNote(noteEntity.html, $notesList);
@ -320,7 +320,7 @@ const normalizeNewlines = function(str) {
        return this.updateNotesCount(1);
      }
      // The server can send the same update multiple times so we need to make sure to only update once per actual update.
-      else if (this.isUpdatedNote(noteEntity, $note)) {
+      else if (Notes.isUpdatedNote(noteEntity, $note)) {
        const isEditing = $note.hasClass('is-editing');
        const initialContent = normalizeNewlines(
          $note.find('.original-note-content').text().trim()
@ -348,23 +348,6 @@ const normalizeNewlines = function(str) {
      }
    };

-    /*
-    Check if note does not exists on page
-     */
-
-    Notes.prototype.isNewNote = function(noteEntity) {
-      return $.inArray(noteEntity.id, this.note_ids) === -1;
-    };
-
-    Notes.prototype.isUpdatedNote = function(noteEntity, $note) {
-      // There can be CRLF vs LF mismatches if we don't sanitize and compare the same way
-      const sanitizedNoteNote = normalizeNewlines(noteEntity.note);
-      const currentNoteText = normalizeNewlines(
-        $note.find('.original-note-content').text().trim()
-      );
-      return sanitizedNoteNote !== currentNoteText;
-    };
-
    Notes.prototype.isParallelView = function() {
      return Cookies.get('diff_view') === 'parallel';
    };
@ -377,7 +360,7 @@ const normalizeNewlines = function(str) {

    Notes.prototype.renderDiscussionNote = function(noteEntity, $form) {
      var discussionContainer, form, row, lineType, diffAvatarContainer;
-      if (!this.isNewNote(noteEntity)) {
+      if (!Notes.isNewNote(noteEntity, this.note_ids)) {
        return;
      }
      this.note_ids.push(noteEntity.id);
@ -1138,6 +1121,25 @@ const normalizeNewlines = function(str) {
      return $form;
    };

+    /**
+     * Check if note does not exists on page
+     */
+    Notes.isNewNote = function(noteEntity, noteIds) {
+      return $.inArray(noteEntity.id, noteIds) === -1;
+    };
+
+    /**
+     * Check if $note already contains the `noteEntity` content
+     */
+    Notes.isUpdatedNote = function(noteEntity, $note) {
+      // There can be CRLF vs LF mismatches if we don't sanitize and compare the same way
+      const sanitizedNoteEntityText = normalizeNewlines(noteEntity.note.trim());
+      const currentNoteText = normalizeNewlines(
+        $note.find('.original-note-content').text().trim()
+      );
+      return sanitizedNoteEntityText !== currentNoteText;
+    };
+
    Notes.checkMergeRequestStatus = function() {
      if (gl.utils.getPagePath(1) === 'merge_requests') {
        gl.mrWidget.checkStatus();
--- a/app/views/shared/notes/_edit.html.haml
+++ b/app/views/shared/notes/_edit.html.haml
@ -1,3 +1 @@
-.original-note-content.hidden{ data: { post_url: note_url(note), target_id: note.noteable.id, target_type: note.noteable.class.name.underscore } }
-  #{note.note}
 %textarea.hidden.js-task-list-field.original-task-list{ data: {update_url: note_url(note) } }= note.note
--- a/app/views/shared/notes/_note.html.haml
+++ b/app/views/shared/notes/_note.html.haml
@ -43,6 +43,8 @@
        .note-text.md
          = note.redacted_note_html
        = edited_time_ago_with_tooltip(note, placement: 'bottom', html_class: 'note_edited_ago')
+        .original-note-content.hidden{ data: { post_url: note_url(note), target_id: note.noteable.id, target_type: note.noteable.class.name.underscore } }
+          #{note.note}
        - if note_editable
          = render 'shared/notes/edit', note: note
        .note-awards
--- a/spec/features/issues/note_polling_spec.rb
+++ b/spec/features/issues/note_polling_spec.rb
@ -18,58 +18,93 @@ feature 'Issue notes polling', :feature, :js do
  end

  describe 'updates' do
-    let(:user) { create(:user) }
-    let(:note_text) { "Hello World" }
-    let(:updated_text) { "Bye World" }
-    let!(:existing_note) { create(:note, noteable: issue, project: project, author: user, note: note_text) }
+    context 'when from own user' do
+      let(:user) { create(:user) }
+      let(:note_text) { "Hello World" }
+      let(:updated_text) { "Bye World" }
+      let!(:existing_note) { create(:note, noteable: issue, project: project, author: user, note: note_text) }

-    before do
-      login_as(user)
-      visit namespace_project_issue_path(project.namespace, project, issue)
+      before do
+        login_as(user)
+        visit namespace_project_issue_path(project.namespace, project, issue)
+      end
+
+      it 'has .original-note-content to compare against' do
+        expect(page).to have_selector("#note_#{existing_note.id}", text: note_text)
+        expect(page).to have_selector("#note_#{existing_note.id} .original-note-content", visible: false)
+
+        update_note(existing_note, updated_text)
+
+        expect(page).to have_selector("#note_#{existing_note.id}", text: updated_text)
+        expect(page).to have_selector("#note_#{existing_note.id} .original-note-content", visible: false)
+      end
+
+      it 'displays the updated content' do
+        expect(page).to have_selector("#note_#{existing_note.id}", text: note_text)
+
+        update_note(existing_note, updated_text)
+
+        expect(page).to have_selector("#note_#{existing_note.id}", text: updated_text)
+      end
+
+      it 'when editing but have not changed anything, and an update comes in, show the updated content in the textarea' do
+        find("#note_#{existing_note.id} .js-note-edit").click
+
+        expect(page).to have_field("note[note]", with: note_text)
+
+        update_note(existing_note, updated_text)
+
+        expect(page).to have_field("note[note]", with: updated_text)
+      end
+
+      it 'when editing but you changed some things, and an update comes in, show a warning' do
+        find("#note_#{existing_note.id} .js-note-edit").click
+
+        expect(page).to have_field("note[note]", with: note_text)
+
+        find("#note_#{existing_note.id} .js-note-text").set('something random')
+
+        update_note(existing_note, updated_text)
+
+        expect(page).to have_selector(".alert")
+      end
+
+      it 'when editing but you changed some things, an update comes in, and you press cancel, show the updated content' do
+        find("#note_#{existing_note.id} .js-note-edit").click
+
+        expect(page).to have_field("note[note]", with: note_text)
+
+        find("#note_#{existing_note.id} .js-note-text").set('something random')
+
+        update_note(existing_note, updated_text)
+
+        find("#note_#{existing_note.id} .note-edit-cancel").click
+
+        expect(page).to have_selector("#note_#{existing_note.id}", text: updated_text)
+      end
    end

-    it 'displays the updated content' do
-      expect(page).to have_selector("#note_#{existing_note.id}", text: note_text)
+    context 'when from another user' do
+      let(:user1) { create(:user) }
+      let(:user2) { create(:user) }
+      let(:note_text) { "Hello World" }
+      let(:updated_text) { "Bye World" }
+      let!(:existing_note) { create(:note, noteable: issue, project: project, author: user1, note: note_text) }

-      update_note(existing_note, updated_text)
+      before do
+        login_as(user2)
+        visit namespace_project_issue_path(project.namespace, project, issue)
+      end

-      expect(page).to have_selector("#note_#{existing_note.id}", text: updated_text)
-    end
+      it 'has .original-note-content to compare against' do
+        expect(page).to have_selector("#note_#{existing_note.id}", text: note_text)
+        expect(page).to have_selector("#note_#{existing_note.id} .original-note-content", visible: false)

-    it 'when editing but have not changed anything, and an update comes in, show the updated content in the textarea' do
-      find("#note_#{existing_note.id} .js-note-edit").click
+        update_note(existing_note, updated_text)

-      expect(page).to have_field("note[note]", with: note_text)
-
-      update_note(existing_note, updated_text)
-
-      expect(page).to have_field("note[note]", with: updated_text)
-    end
-
-    it 'when editing but you changed some things, and an update comes in, show a warning' do
-      find("#note_#{existing_note.id} .js-note-edit").click
-
-      expect(page).to have_field("note[note]", with: note_text)
-
-      find("#note_#{existing_note.id} .js-note-text").set('something random')
-
-      update_note(existing_note, updated_text)
-
-      expect(page).to have_selector(".alert")
-    end
-
-    it 'when editing but you changed some things, an update comes in, and you press cancel, show the updated content' do
-      find("#note_#{existing_note.id} .js-note-edit").click
-
-      expect(page).to have_field("note[note]", with: note_text)
-
-      find("#note_#{existing_note.id} .js-note-text").set('something random')
-
-      update_note(existing_note, updated_text)
-
-      find("#note_#{existing_note.id} .note-edit-cancel").click
-
-      expect(page).to have_selector("#note_#{existing_note.id}", text: updated_text)
+        expect(page).to have_selector("#note_#{existing_note.id}", text: updated_text)
+        expect(page).to have_selector("#note_#{existing_note.id} .original-note-content", visible: false)
+      end
    end
  end

--- a/spec/javascripts/notes_spec.js
+++ b/spec/javascripts/notes_spec.js
@ -99,8 +99,6 @@ import '~/notes';

        notes = jasmine.createSpyObj('notes', [
          'refresh',
-          'isNewNote',
-          'isUpdatedNote',
          'collapseLongCommitList',
          'updateNotesCount',
          'putConflictEditWarningInPlace'
@ -110,13 +108,15 @@ import '~/notes';
        notes.updatedNotesTrackingMap = {};

        spyOn(gl.utils, 'localTimeAgo');
+        spyOn(Notes, 'isNewNote').and.callThrough();
+        spyOn(Notes, 'isUpdatedNote').and.callThrough();
        spyOn(Notes, 'animateAppendNote').and.callThrough();
        spyOn(Notes, 'animateUpdateNote').and.callThrough();
      });

      describe('when adding note', () => {
        it('should call .animateAppendNote', () => {
-          notes.isNewNote.and.returnValue(true);
+          Notes.isNewNote.and.returnValue(true);
          Notes.prototype.renderNote.call(notes, note, null, $notesList);

          expect(Notes.animateAppendNote).toHaveBeenCalledWith(note.html, $notesList);
@ -125,7 +125,8 @@ import '~/notes';

      describe('when note was edited', () => {
        it('should call .animateUpdateNote', () => {
-          notes.isUpdatedNote.and.returnValue(true);
+          Notes.isNewNote.and.returnValue(false);
+          Notes.isUpdatedNote.and.returnValue(true);
          const $note = $('<div>');
          $notesList.find.and.returnValue($note);
          Notes.prototype.renderNote.call(notes, note, null, $notesList);
@ -135,7 +136,8 @@ import '~/notes';

        describe('while editing', () => {
          it('should update textarea if nothing has been touched', () => {
-            notes.isUpdatedNote.and.returnValue(true);
+            Notes.isNewNote.and.returnValue(false);
+            Notes.isUpdatedNote.and.returnValue(true);
            const $note = $(`<div class="is-editing">
              <div class="original-note-content">initial</div>
              <textarea class="js-note-text">initial</textarea>
@ -147,7 +149,8 @@ import '~/notes';
          });

          it('should call .putConflictEditWarningInPlace', () => {
-            notes.isUpdatedNote.and.returnValue(true);
+            Notes.isNewNote.and.returnValue(false);
+            Notes.isUpdatedNote.and.returnValue(true);
            const $note = $(`<div class="is-editing">
              <div class="original-note-content">initial</div>
              <textarea class="js-note-text">different</textarea>
@ -161,6 +164,47 @@ import '~/notes';
      });
    });

+    describe('isUpdatedNote', () => {
+      it('should consider same note text as the same', () => {
+        const result = Notes.isUpdatedNote(
+          {
+            note: 'initial'
+          },
+          $(`<div>
+            <div class="original-note-content">initial</div>
+          </div>`)
+        );
+
+        expect(result).toEqual(false);
+      });
+
+      it('should consider same note with trailing newline as the same', () => {
+        const result = Notes.isUpdatedNote(
+          {
+            note: 'initial\n'
+          },
+          $(`<div>
+            <div class="original-note-content">initial\n</div>
+          </div>`)
+        );
+
+        expect(result).toEqual(false);
+      });
+
+      it('should consider different notes as different', () => {
+        const result = Notes.isUpdatedNote(
+          {
+            note: 'foo'
+          },
+          $(`<div>
+            <div class="original-note-content">bar</div>
+          </div>`)
+        );
+
+        expect(result).toEqual(true);
+      });
+    });
+
    describe('renderDiscussionNote', () => {
      let discussionContainer;
      let note;
@ -180,15 +224,15 @@ import '~/notes';
        row = jasmine.createSpyObj('row', ['prevAll', 'first', 'find']);

        notes = jasmine.createSpyObj('notes', [
-          'isNewNote',
          'isParallelView',
          'updateNotesCount',
        ]);
        notes.note_ids = [];

        spyOn(gl.utils, 'localTimeAgo');
+        spyOn(Notes, 'isNewNote');
        spyOn(Notes, 'animateAppendNote');
-        notes.isNewNote.and.returnValue(true);
+        Notes.isNewNote.and.returnValue(true);
        notes.isParallelView.and.returnValue(false);
        row.prevAll.and.returnValue(row);
        row.first.and.returnValue(row);