Merge pull request #1630 from riyad/show-commit-notes-with-mr-notes

Mix commit notes with merge request notes on MR show page

app/assets/javascripts/notes.js

@@ -230,7 +230,7 @@ var NoteList = {
   updateVotes:
     function() {
       var votes = $("#votes .votes");
-      var notes = $("#notes-list, #new-notes-list").find(".note.vote");
+      var notes = $("#notes-list, #new-notes-list").find(".note .vote");
 
       // only update if there is a vote display
       if (votes.size()) {

app/contexts/notes/load_context.rb

@@ -13,7 +13,7 @@ module Notes
                when "issue"
                  project.issues.find(target_id).notes.inc_author.fresh.limit(20)
                when "merge_request"
-                 project.merge_requests.find(target_id).notes.inc_author.fresh.limit(20)
+                 project.merge_requests.find(target_id).mr_and_commit_notes.inc_author.fresh.limit(20)
                when "snippet"
                  project.snippets.find(target_id).notes.fresh
                when "wall"

app/controllers/notes_controller.rb

@@ -7,6 +7,11 @@ class NotesController < ProjectResourceController
 
   def index
     notes
+    if params[:target_type] == "merge_request"
+      @mixed_targets = true
+      @main_target_type = params[:target_type].camelize
+    end
+
     respond_with(@notes)
   end
 

app/helpers/notes_helper.rb

@@ -7,11 +7,20 @@ module NotesHelper
     params[:loading_new].present?
   end
 
-  def note_vote_class(note)
-    if note.upvote?
-      "vote upvote"
-    elsif note.downvote?
-      "vote downvote"
-    end
+   # Helps to distinguish e.g. commit notes in mr notes list
+  def note_for_main_target?(note)
+    !@mixed_targets || @main_target_type == note.noteable_type
+  end
+
+  def link_to_commit_diff_line_note(note)
+    return unless note.line_note?
+
+    commit = note.target
+    diff_index, diff_old_line, diff_new_line = note.line_code.split('_')
+
+    link_file = commit.diffs[diff_index.to_i].new_path
+    link_line = diff_new_line
+
+    link_to "#{link_file}:L#{link_line}", project_commit_path(@project, commit, anchor: note.line_code)
   end
 end

app/models/note.rb

@@ -49,7 +49,7 @@ class Note < ActiveRecord::Base
   end
 
   def target
-    if noteable_type == "Commit"
+    if commit?
       project.commit(noteable_id)
     else
       noteable
@@ -82,6 +82,10 @@ class Note < ActiveRecord::Base
     noteable_type == "Commit"
   end
 
+  def line_note?
+    line_code.present?
+  end
+
   def commit_author
     @commit_author ||=
       project.users.find_by_email(target.author_email) ||

app/views/commits/_text_file.html.haml

@@ -4,7 +4,7 @@
 
 %table{class: "#{'hide' if too_big}"}
   - each_diff_line(diff.diff.lines.to_a, index) do |line, type, line_code, line_new, line_old|
-    %tr.line_holder
+    %tr.line_holder{ id: line_code }
       - if type == "match"
         %td.old_line= "..."
         %td.new_line= "..."

app/views/notes/_note.html.haml

@@ -1,4 +1,4 @@
-%li{id: dom_id(note), class: "note #{note_vote_class(note)}"}
+%li{id: dom_id(note), class: "note"}
   = image_tag gravatar_icon(note.author.email), class: "avatar s32"
   %div.note-author
     %strong= note.author_name
@@ -6,14 +6,25 @@
       %cite.cgray
         = time_ago_in_words(note.updated_at)
         ago
-    - if note.upvote?
-      %span.label.label-success
-        %i.icon-thumbs-up
-        \+1
-    - if note.downvote?
-      %span.label.label-error
-        %i.icon-thumbs-down
-        \-1
+
+    - unless note_for_main_target?(note)
+      - if note.commit?
+        %span.cgray
+          on #{link_to note.target.short_id, project_commit_path(@project, note.target)}
+          = link_to_commit_diff_line_note(note) if note.line_note?
+
+    -# only show vote if it's a note for the main target
+    - if note_for_main_target?(note)
+      - if note.upvote?
+        %span.vote.upvote.label.label-success
+          %i.icon-thumbs-up
+          \+1
+      - if note.downvote?
+        %span.vote.downvote.label.label-error
+          %i.icon-thumbs-down
+          \-1
+
+    -# remove button
     - if(note.author_id == current_user.id) || can?(current_user, :admin_note, @project)
       = link_to [@project, note], confirm: 'Are you sure?', method: :delete, remote: true, class: "cred delete-note btn very_small" do
         %i.icon-trash