Merge branch 'fix-comment-on-diff-ajax-loading'

6 files changed, 69 insertions, 52 deletions

diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee
index 2bfc5cb2d9c..53d72be66e3 100644
--- a/app/assets/javascripts/notes.js.coffee
+++ b/app/assets/javascripts/notes.js.coffee
@@ -15,6 +15,8 @@ class @Notes
     @last_fetched_at = last_fetched_at
     @view = view
     @noteable_url = document.URL
+    @notesCountBadge ||= $(".issuable-details").find(".notes-tab .badge")
+
     @initRefresh()
     @setupMainTargetNoteForm()
     @cleanBinding()
@@ -89,7 +91,7 @@ class @Notes
     , 15000
 
   refresh: ->
-    unless document.hidden or (@noteable_url != document.URL)
+    if not document.hidden and document.URL.indexOf(@noteable_url) is 0
       @getContent()
 
   getContent: ->
@@ -101,7 +103,10 @@ class @Notes
         notes = data.notes
         @last_fetched_at = data.last_fetched_at
         $.each notes, (i, note) =>
-          @renderNote(note)
+          if note.discussion_with_diff_html?
+            @renderDiscussionNote(note)
+          else
+            @renderNote(note)
 
 
   ###
@@ -116,18 +121,21 @@ class @Notes
         flash.pinTo('.header-content')
       return
 
+    if note.award
+      awards_handler.addAwardToEmojiBar(note.note)
+      awards_handler.scrollToAwards()
+
     # render note if it not present in loaded list
     # or skip if rendered
-    if @isNewNote(note) && !note.award
+    else if @isNewNote(note)
       @note_ids.push(note.id)
-      $('ul.main-notes-list').
-        append(note.html).
-        syntaxHighlight()
+
+      $('ul.main-notes-list')
+        .append(note.html)
+        .syntaxHighlight()
       @initTaskList()
+      @updateNotesCount(1)
 
-    if note.award
-      awards_handler.addAwardToEmojiBar(note.note)
-      awards_handler.scrollToAwards()
 
   ###
   Check if note does not exists on page
@@ -144,34 +152,39 @@ class @Notes
   Note: for rendering inline notes use renderDiscussionNote
   ###
   renderDiscussionNote: (note) ->
+    return unless @isNewNote(note)
+
     @note_ids.push(note.id)
-    form = $("form[rel='" + note.discussion_id + "']")
+    form = $("#new-discussion-note-form-#{note.discussion_id}")
     row = form.closest("tr")
     note_html = $(note.html)
     note_html.syntaxHighlight()
 
     # is this the first note of discussion?
-    if row.is(".js-temp-notes-holder")
+    discussionContainer = $(".notes[data-discussion-id='" + note.discussion_id + "']")
+    if discussionContainer.length is 0
       # insert the note and the reply button after the temp row
       row.after note.discussion_html
 
       # remove the note (will be added again below)
       row.next().find(".note").remove()
 
+      # Before that, the container didn't exist
+      discussionContainer = $(".notes[data-discussion-id='" + note.discussion_id + "']")
+
       # Add note to 'Changes' page discussions
-      $(".notes[rel='" + note.discussion_id + "']").append note_html
+      discussionContainer.append note_html
 
       # Init discussion on 'Discussion' page if it is merge request page
-      if $('body').attr('data-page').indexOf('projects:merge_request') == 0
-        discussion_html = $(note.discussion_with_diff_html)
-        discussion_html.syntaxHighlight()
-        $('ul.main-notes-list').append(discussion_html)
+      if $('body').attr('data-page').indexOf('projects:merge_request') is 0
+        $('ul.main-notes-list')
+          .append(note.discussion_with_diff_html)
+          .syntaxHighlight()
     else
       # append new note to all matching discussions
-      $(".notes[rel='" + note.discussion_id + "']").append note_html
+      discussionContainer.append note_html
 
-    # cleanup after successfully creating a diff/discussion note
-    @removeDiscussionNoteForm(form)
+    @updateNotesCount(1)
 
   ###
   Called in response the main target form has been successfully submitted.
@@ -278,6 +291,9 @@ class @Notes
   addDiscussionNote: (xhr, note, status) =>
     @renderDiscussionNote(note)
 
+    # cleanup after successfully creating a diff/discussion note
+    @removeDiscussionNoteForm($("#new-discussion-note-form-#{note.discussion_id}"))
+
   ###
   Called in response to the edit note form being submitted
 
@@ -349,30 +365,32 @@ class @Notes
   Removes the actual note from view.
   Removes the whole discussion if the last note is being removed.
   ###
-  removeNote: ->
-    note = $(this).closest(".note")
-    note_id = note.attr('id')
+  removeNote: (e) =>
+    noteId = $(e.currentTarget)
+               .closest(".note")
+               .attr("id")
 
-    $('.note[id="' + note_id + '"]').each ->
-      note = $(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.
+    $(".note[id='#{noteId}']").each (i, el) =>
+      note  = $(el)
       notes = note.closest(".notes")
-      count = notes.closest(".issuable-details").find(".notes-tab .badge")
 
       # check if this is the last note for this line
       if notes.find(".note").length is 1
 
-        # for discussions
-        notes.closest(".discussion").remove()
+        # "Discussions" tab
+        notes.closest(".timeline-entry").remove()
 
-        # for diff lines
+        # "Changes" tab / commit view
         notes.closest("tr").remove()
 
-      # update notes count
-      oldNum = parseInt(count.text())
-      count.text(oldNum - 1)
-
       note.remove()
 
+    # Decrement the "Discussions" counter only once
+    @updateNotesCount(-1)
+
   ###
   Called in response to clicking the delete attachment link
 
@@ -412,7 +430,7 @@ class @Notes
   ###
   setupDiscussionNoteForm: (dataHolder, form) =>
     # setup note target
-    form.attr "rel", dataHolder.data("discussionId")
+    form.attr 'id', "new-discussion-note-form-#{dataHolder.data("discussionId")}"
     form.find("#line_type").val dataHolder.data("lineType")
     form.find("#note_commit_id").val dataHolder.data("commitId")
     form.find("#note_line_code").val dataHolder.data("lineCode")
@@ -542,3 +560,6 @@ class @Notes
 
   updateTaskList: ->
     $('form', this).submit()
+
+  updateNotesCount: (updateCount) ->
+    @notesCountBadge.text(parseInt(@notesCountBadge.text()) + updateCount)
diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index 6f1e186d408..4a2599dda37 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -11,11 +11,9 @@ class Projects::NotesController < Projects::ApplicationController
     notes_json = { notes: [], last_fetched_at: current_fetched_at }
 
     @notes.each do |note|
-      notes_json[:notes] << {
-        id: note.id,
-        html: note_to_html(note),
-        valid: note.valid?
-      }
+      next if note.cross_reference_not_visible_for?(current_user)
+
+      notes_json[:notes] << note_json(note)
     end
 
     render json: notes_json
@@ -25,7 +23,7 @@ class Projects::NotesController < Projects::ApplicationController
     @note = Notes::CreateService.new(project, current_user, note_params).execute
 
     respond_to do |format|
-      format.json { render_note_json(@note) }
+      format.json { render json: note_json(@note) }
       format.html { redirect_back_or_default }
     end
   end
@@ -34,7 +32,7 @@ class Projects::NotesController < Projects::ApplicationController
     @note = Notes::UpdateService.new(project, current_user, note_params).execute(note)
 
     respond_to do |format|
-      format.json { render_note_json(@note) }
+      format.json { render json: note_json(@note) }
       format.html { redirect_back_or_default }
     end
   end
@@ -99,6 +97,8 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def note_to_discussion_html(note)
+    return unless note.for_diff_line?
+
     if params[:view] == 'parallel'
       template = "projects/notes/_diff_notes_with_reply_parallel"
       locals =
@@ -131,9 +131,9 @@ class Projects::NotesController < Projects::ApplicationController
     )
   end
 
-  def render_note_json(note)
+  def note_json(note)
     if note.valid?
-      render json: {
+      {
         valid: true,
         id: note.id,
         discussion_id: note.discussion_id,
@@ -144,7 +144,7 @@ class Projects::NotesController < Projects::ApplicationController
         discussion_with_diff_html: note_to_discussion_with_diff_html(note)
       }
     else
-      render json: {
+      {
         valid: false,
         award: note.is_award,
         errors: note.errors
@@ -163,8 +163,6 @@ class Projects::NotesController < Projects::ApplicationController
     )
   end
 
-  private
-
   def find_current_user_notes
     @notes = NotesFinder.new.execute(project, current_user, params)
   end
diff --git a/app/views/projects/notes/_diff_notes_with_reply.html.haml b/app/views/projects/notes/_diff_notes_with_reply.html.haml
index c731baf0a65..11f9859a90f 100644
--- a/app/views/projects/notes/_diff_notes_with_reply.html.haml
+++ b/app/views/projects/notes/_diff_notes_with_reply.html.haml
@@ -7,7 +7,7 @@
         %i.fa.fa-comment
         = notes.count
     %td.notes_content
-      %ul.notes{ rel: note.discussion_id }
+      %ul.notes{ data: { discussion_id: note.discussion_id } }
         = render notes
       .discussion-reply-holder
         = link_to_reply_diff(note)
diff --git a/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml b/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml
index c6726cbafa3..bb761ed2f94 100644
--- a/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml
+++ b/app/views/projects/notes/_diff_notes_with_reply_parallel.html.haml
@@ -8,7 +8,7 @@
         %i.fa.fa-comment
         = notes_left.count
     %td.notes_content.parallel.old
-      %ul.notes{ rel: note1.discussion_id }
+      %ul.notes{ data: { discussion_id: note1.discussion_id } }
         = render notes_left
 
       .discussion-reply-holder
@@ -23,7 +23,7 @@
         %i.fa.fa-comment
         = notes_right.count
     %td.notes_content.parallel.new
-      %ul.notes{ rel: note2.discussion_id }
+      %ul.notes{ data: { discussion_id: note2.discussion_id } }
         = render notes_right
 
       .discussion-reply-holder
@@ -31,4 +31,3 @@
   - else
     %td.notes_line.new= ""
     %td.notes_content.parallel.new= ""
-
diff --git a/app/views/projects/notes/_note.html.haml b/app/views/projects/notes/_note.html.haml
index 922535e5c4a..e858c412836 100644
--- a/app/views/projects/notes/_note.html.haml
+++ b/app/views/projects/notes/_note.html.haml
@@ -1,4 +1,4 @@
-%li.timeline-entry{ id: dom_id(note), class: [dom_class(note), "note-row-#{note.id}", ('system-note' if note.system)], data: { discussion: note.discussion_id } }
+%li.timeline-entry{ id: dom_id(note), class: [dom_class(note), "note-row-#{note.id}", ('system-note' if note.system)] }
   .timeline-entry-inner
     .timeline-icon
       %a{href: user_path(note.author)}
diff --git a/app/views/projects/notes/discussions/_commit.html.haml b/app/views/projects/notes/discussions/_commit.html.haml
index 6903fad4a0a..3da2f2060b8 100644
--- a/app/views/projects/notes/discussions/_commit.html.haml
+++ b/app/views/projects/notes/discussions/_commit.html.haml
@@ -20,8 +20,7 @@
       = render "projects/notes/discussions/diff", discussion_notes: discussion_notes, note: note
     - else
       .panel.panel-default
-        .notes{ rel: discussion_notes.first.discussion_id }
+        .notes{ data: { discussion_id: discussion_notes.first.discussion_id } }
           = render discussion_notes
         .discussion-reply-holder
           = link_to_reply_diff(discussion_notes.first)
-