Make note anchors actually work

Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com>

app/assets/javascripts/notes.js

@@ -13,9 +13,6 @@ var NoteList = {
 
     NoteList.setupMainTargetNoteForm();
 
-    // get initial set of notes
-    NoteList.getContent();
-
     // Unbind events to prevent firing twice
     $(document).off("click", ".js-add-diff-note-button");
     $(document).off("click", ".js-discussion-reply-button");

app/controllers/projects/commit_controller.rb

@@ -24,7 +24,8 @@ class Projects::CommitController < Projects::ApplicationController
     @line_notes  = result[:line_notes]
     @branches    = result[:branches]
     @notes_count = result[:notes_count]
-    @target_type = :commit
+    @target_type = "Commit"
+    @notes = project.notes.for_commit_id(@commit.id).not_inline.fresh
     @target_id   = @commit.id
 
     @comments_allowed = @reply_allowed = true

app/controllers/projects/issues_controller.rb

@@ -49,7 +49,8 @@ class Projects::IssuesController < Projects::ApplicationController
 
   def show
     @note = @project.notes.new(noteable: @issue)
-    @target_type = :issue
+    @notes = @issue.notes.inc_author.fresh
+    @target_type = 'Issue'
     @target_id = @issue.id
 
     respond_with(@issue)

app/controllers/projects/merge_requests_controller.rb

@@ -198,6 +198,10 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   def define_show_vars
     # Build a note object for comment form
     @note = @project.notes.new(noteable: @merge_request)
+    @notes = @merge_request.mr_and_commit_notes.inc_author.fresh
+    @discussions = Note.discussions_from_notes(@notes)
+    @target_type = 'MergeRequest'
+    @target_id = @merge_request.id
 
     # Get commits from repository
     # or from cache if already merged
@@ -205,9 +209,6 @@ class Projects::MergeRequestsController < Projects::ApplicationController
 
     @allowed_to_merge = allowed_to_merge?
     @show_merge_controls = @merge_request.opened? && @commits.any? && @allowed_to_merge
-
-    @target_type = :merge_request
-    @target_id = @merge_request.id
   end
 
   def allowed_to_merge?

app/controllers/projects/notes_controller.rb

@@ -11,7 +11,7 @@ class Projects::NotesController < Projects::ApplicationController
     @target_id = params[:target_id]
 
     if params[:target_type] == "merge_request"
-      @discussions   = discussions_from_notes
+      @discussions = Note.discussions_from_notes(@notes)
     end
 
     respond_to do |format|
@@ -76,36 +76,4 @@ class Projects::NotesController < Projects::ApplicationController
   def preview
     render text: view_context.markdown(params[:note])
   end
-
-  protected
-
-  def discussion_notes_for(note)
-    @notes.select do |other_note|
-      note.discussion_id == other_note.discussion_id
-    end
-  end
-
-  def discussions_from_notes
-    discussion_ids = []
-    discussions = []
-
-    @notes.each do |note|
-      next if discussion_ids.include?(note.discussion_id)
-
-      # don't group notes for the main target
-      if note_for_main_target?(note)
-        discussions << [note]
-      else
-        discussions << discussion_notes_for(note)
-        discussion_ids << note.discussion_id
-      end
-    end
-
-    discussions
-  end
-
-  # Helps to distinguish e.g. commit notes in mr notes list
-  def note_for_main_target?(note)
-    (@target_type.camelize == note.noteable_type && !note.for_diff_line?)
-  end
 end

app/controllers/projects/snippets_controller.rb

@@ -48,7 +48,8 @@ class Projects::SnippetsController < Projects::ApplicationController
 
   def show
     @note = @project.notes.new(noteable: @snippet)
-    @target_type = :snippet
+    @notes = @snippet.notes.fresh
+    @target_type = 'Snippet'
     @target_id = @snippet.id
   end
 

app/models/note.rb

@@ -56,7 +56,8 @@ class Note < ActiveRecord::Base
   serialize :st_diff
   before_create :set_diff, if: ->(n) { n.line_code.present? }
 
-  def self.create_status_change_note(noteable, project, author, status, source)
+  class << self
+    def create_status_change_note(noteable, project, author, status, source)
       body = "_Status changed to #{status}#{' by ' + source.gfm_reference if source}_"
 
       create({
@@ -70,7 +71,7 @@ class Note < ActiveRecord::Base
 
     # +noteable+ was referenced from +mentioner+, by including GFM in either +mentioner+'s description or an associated Note.
     # Create a system Note associated with +noteable+ with a GFM back-reference to +mentioner+.
-  def self.create_cross_reference_note(noteable, mentioner, author, project)
+    def create_cross_reference_note(noteable, mentioner, author, project)
       create({
         noteable: noteable,
         commit_id: (noteable.sha if noteable.respond_to? :sha),
@@ -81,6 +82,28 @@ class Note < ActiveRecord::Base
       }, without_protection: true)
     end
 
+    def discussions_from_notes(notes)
+      discussion_ids = []
+      discussions = []
+
+      notes.each do |note|
+        next if discussion_ids.include?(note.discussion_id)
+
+        # don't group notes for the main target
+        if !note.for_diff_line? && note.noteable_type == "MergeRequest"
+          discussions << [note]
+        else
+          discussions << notes.select do |other_note|
+            note.discussion_id == other_note.discussion_id
+          end
+          discussion_ids << note.discussion_id
+        end
+      end
+
+      discussions
+    end
+  end
+
   # Determine whether or not a cross-reference note already exists.
   def self.cross_reference_exists?(noteable, mentioner)
     where(noteable_id: noteable.id, system: true, note: "_mentioned in #{mentioner.gfm_reference}_").any?

app/views/projects/notes/_notes.html.haml

@@ -4,7 +4,7 @@
     - if note_for_main_target?(note)
       = render discussion_notes
     - else
-      = render 'discussion', discussion_notes: discussion_notes
+      = render 'projects/notes/discussion', discussion_notes: discussion_notes
 - else
   - @notes.each do |note|
     - next unless note.author

app/views/projects/notes/_notes_with_form.html.haml

 %ul#notes-list.notes
+  = render "projects/notes/notes"
 .js-notes-busy
 
 .js-main-target-form