Fix diff comments loaded by AJAX to load comment with diff in discussion tab

This commits also fixes two minor issues:
- Ensure notes that the current user is not allowed to see are not
  returned in the AJAX notes loading
- Ensure the notes counter badge is decremented of 1 instead of 2

CHANGELOG

@@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 v 8.5.0 (unreleased)
 
 v 8.4.0 (unreleased)
+  - Fix diff comments loaded by AJAX to load comment with diff in discussion tab
   - Ensure Gravatar host looks like an actual host
   - Consider re-assign as a mention from a notification point of view
   - Add pagination headers to already paginated API resources

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,6 +103,9 @@ class @Notes
         notes = data.notes
         @last_fetched_at = data.last_fetched_at
         $.each notes, (i, 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()
       @initTaskList()
+      @incrementNotesCount()
 
-    if note.award
-      awards_handler.addAwardToEmojiBar(note.note)
-      awards_handler.scrollToAwards()
 
   ###
   Check if note does not exists on page
@@ -144,6 +152,8 @@ 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 + "']")
     row = form.closest("tr")
@@ -151,27 +161,30 @@ class @Notes
     note_html.syntaxHighlight()
 
     # is this the first note of discussion?
-    if row.is(".js-temp-notes-holder")
+    discussionContainer = $(".notes[rel='" + 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[rel='" + 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)
+    @incrementNotesCount()
 
   ###
   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($("form[rel='" + note.discussion_id + "']"))
+
   ###
   Called in response to the edit note form being submitted
 
@@ -349,14 +365,12 @@ 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)
+    $('.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
@@ -367,12 +381,10 @@ class @Notes
         # for diff lines
         notes.closest("tr").remove()
 
-      # update notes count
-      oldNum = parseInt(count.text())
-      count.text(oldNum - 1)
-
       note.remove()
 
+    @decrementNotesCount()
+
   ###
   Called in response to clicking the delete attachment link
 
@@ -542,3 +554,9 @@ class @Notes
 
   updateTaskList: ->
     $('form', this).submit()
+
+  incrementNotesCount: (incrementStep = 1) ->
+    @notesCountBadge.text parseInt(@notesCountBadge.text()) + incrementStep
+
+  decrementNotesCount: ->
+    @incrementNotesCount(-1)

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

features/project/merge_requests.feature

@@ -83,6 +83,15 @@ Feature: Project Merge Requests
     And I leave a comment like "Line is wrong" on diff
     And I switch to the merge request's comments tab
     Then I should see a discussion has started on diff
+    And I should see a badge of "1" next to the discussion link
+
+  @javascript
+  Scenario: I see a new comment on merge request diff from another user in the discussion tab
+    Given project "Shop" have "Bug NS-05" open merge request with diffs inside
+    And I visit merge request page "Bug NS-05"
+    And user "John Doe" leaves a comment like "Line is wrong" on diff
+    Then I should see a discussion by user "John Doe" has started on diff
+    And I should see a badge of "1" next to the discussion link
 
   @javascript
   Scenario: I edit a comment on a merge request diff
@@ -100,9 +109,11 @@ Feature: Project Merge Requests
     And I visit merge request page "Bug NS-05"
     And I click on the Changes tab
     And I leave a comment like "Line is wrong" on diff
+    And I should see a badge of "1" next to the discussion link
     And I delete the comment "Line is wrong" on diff
     And I click on the Discussion tab
     Then I should not see any discussion
+    And I should see a badge of "0" next to the discussion link
 
   @javascript
   Scenario: I comment on a line of a commit in merge request

features/steps/project/merge_requests.rb

@@ -181,6 +181,15 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     leave_comment "Line is wrong"
   end
 
+  step 'user "John Doe" leaves a comment like "Line is wrong" on diff' do
+    mr = MergeRequest.find_by(title: "Bug NS-05")
+    create(:note_on_merge_request_diff, project: project,
+                                        noteable_id: mr.id,
+                                        author: user_exists("John Doe"),
+                                        line_code: sample_commit.line_code,
+                                        note: 'Line is wrong')
+  end
+
   step 'I leave a comment like "Line is wrong" on diff in commit' do
     click_diff_line(sample_commit.line_code)
     leave_comment "Line is wrong"
@@ -238,6 +247,22 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     end
   end
 
+  step 'I should see a discussion by user "John Doe" has started on diff' do
+    page.within(".notes .discussion") do
+      page.should have_content "#{user_exists("John Doe").name} started a discussion"
+      page.should have_content sample_commit.line_code_path
+      page.should have_content "Line is wrong"
+    end
+  end
+
+  step 'I should see a badge of "1" next to the discussion link' do
+    expect_discussion_badge_to_have_counter("1")
+  end
+
+  step 'I should see a badge of "0" next to the discussion link' do
+    expect_discussion_badge_to_have_counter("0")
+  end
+
   step 'I should see a discussion has started on commit diff' do
     page.within(".notes .discussion") do
       page.should have_content "#{current_user.name} started a discussion on commit"
@@ -444,4 +469,10 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   def have_visible_content (text)
     have_css("*", text: text, visible: true)
   end
+
+  def expect_discussion_badge_to_have_counter(value)
+    page.within(".notes-tab .badge") do
+      page.should have_content value
+    end
+  end
 end