From 5c2f6d7f050222d2601218a0bec1dadcee5fcfa0 Mon Sep 17 00:00:00 2001
From: Riyad Preukschas <riyad@informatik.uni-bremen.de>
Date: Sun, 2 Dec 2012 20:53:50 +0100
Subject: [PATCH] Update notes views to support discussions

---
 app/assets/javascripts/notes.js               |  13 +-
 app/assets/stylesheets/sections/notes.scss    | 144 +++++++++++++++++-
 app/controllers/notes_controller.rb           |  12 +-
 app/helpers/notes_helper.rb                   |   7 +-
 app/models/note.rb                            |   2 +-
 app/views/merge_requests/diffs.html.haml      |   5 -
 app/views/notes/_discussion.html.haml         |  46 ++++++
 app/views/notes/_discussion_diff.html.haml    |  24 +++
 app/views/notes/_note.html.haml               |  36 ++---
 app/views/notes/_notes.html.haml              |  15 +-
 app/views/notes/_per_line_form.html.haml      |  10 +-
 app/views/notes/_per_line_note_link.html.haml |   6 +-
 .../notes/_per_line_reply_button.html.haml    |   6 +-
 app/views/notes/index.js.haml                 |   4 +
 14 files changed, 270 insertions(+), 60 deletions(-)
 create mode 100644 app/views/notes/_discussion.html.haml
 create mode 100644 app/views/notes/_discussion_diff.html.haml

diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js
index b6f65b7aa5e..91215fdbcbe 100644
--- a/app/assets/javascripts/notes.js
+++ b/app/assets/javascripts/notes.js
@@ -20,7 +20,7 @@ var NoteList = {
       // get initial set of notes
       this.getContent();
 
-      $("#notes-list, #new-notes-list").on("ajax:success", ".delete-note", function() {
+      $("#notes-list, #new-notes-list").on("ajax:success", ".js-note-delete", function() {
         $(this).closest('li').fadeOut(function() {
           $(this).remove();
           NoteList.updateVotes();
@@ -275,16 +275,23 @@ var NoteList = {
 var PerLineNotes = {
   init:
     function() {
+      $(".per_line_form .hide-button").on("click", function(){
+        $(this).closest(".per_line_form").hide();
+        return false;
+      });
+
       /**
        * Called when clicking on the "add note" or "reply" button for a diff line.
        *
        * Shows the note form below the line.
        * Sets some hidden fields in the form.
        */
-      $(".diff_file_content").on("click", ".line_note_link, .line_note_reply_link", function(e) {
+      $(".diff_file_content").on("click", ".js-note-add-to-diff-line", function(e) {
         var form = $(".per_line_form");
         $(this).closest("tr").after(form);
         form.find("#note_line_code").val($(this).data("lineCode"));
+        form.find("#note_noteable_type").val($(this).data("noteableType"));
+        form.find("#note_noteable_id").val($(this).data("noteableId"));
         form.show();
         e.preventDefault();
       });
@@ -297,7 +304,7 @@ var PerLineNotes = {
        * Removes the actual note from view.
        * Removes the reply button if the last note for that line has been removed.
        */
-      $(".diff_file_content").on("ajax:success", ".delete-note", function() {
+      $(".diff_file_content").on("ajax:success", ".js-note-delete", function() {
         var trNote = $(this).closest("tr");
         trNote.fadeOut(function() {
           $(this).remove();
diff --git a/app/assets/stylesheets/sections/notes.scss b/app/assets/stylesheets/sections/notes.scss
index 0c2a56d62f5..18c17433c03 100644
--- a/app/assets/stylesheets/sections/notes.scss
+++ b/app/assets/stylesheets/sections/notes.scss
@@ -1,6 +1,5 @@
 /**
  * Notes
- *
  */
 #notes-list,
 #new-notes-list {
@@ -8,6 +7,133 @@
   list-style: none;
   margin: 0px;
   padding: 0px;
+
+  .discussion-header,
+  .note-header {
+    @extend .cgray;
+    padding-top: 5px;
+    padding-bottom: 15px;
+
+    .avatar {
+      float: left;
+      margin-right: 10px;
+    }
+
+    .discussion-last-update,
+    .note-last-update {
+      font-style: italic;
+    }
+    .note-author {
+      color: $style_color;
+      font-weight: bold;
+      &:hover {
+        color: $primary_color;
+      }
+    }
+  }
+
+  .discussion {
+    padding: 8px 0;
+    overflow: hidden;
+    display: block;
+    position:relative;
+
+    .discussion-body {
+      margin-left: 50px;
+
+      .diff_file,
+      .discussion-hidden,
+      .notes {
+        @extend .borders;
+        background-color: #F9F9F9;
+      }
+      .diff_file .note {
+        border-bottom: 0px;
+        padding: 0px;
+      }
+      .discussion-hidden .note {
+        @extend .cgray;
+        padding: 8px;
+        text-align: center;
+      }
+      .notes .note {
+        border-color: #ddd;
+        padding: 8px;
+      }
+    }
+  }
+
+  .note {
+    padding: 8px 0;
+    overflow: hidden;
+    display: block;
+    position:relative;
+    p { color: $style_color; }
+
+    .avatar {
+      margin-top:3px;
+    }
+    .note-body {
+      margin-left:45px;
+      padding-top: 5px;
+    }
+    .note-header {
+      padding-bottom: 5px;
+    }
+  }
+}
+
+#notes-list:not(.reversed) .note,
+#notes-list:not(.reversed) .discussion,
+#new-notes-list:not(.reversed) .note,
+#new-notes-list:not(.reversed) .discussion {
+  border-bottom: 1px solid #eee;
+}
+#notes-list.reversed .note,
+#notes-list.reversed .discussion,
+#new-notes-list.reversed .note,
+#new-notes-list.reversed .discussion {
+  border-top: 1px solid #eee;
+}
+
+
+/**
+ * Discussion/Note Actions
+ */
+.discussion,
+.note {
+  &.note:hover {
+    .note-actions { display: block; }
+  }
+  .discussion-header:hover {
+    .discussion-actions { display: block; }
+  }
+
+  .discussion-actions,
+  .note-actions {
+    display: none;
+    float: right;
+
+    [class^="icon-"],
+    [class*="icon-"] {
+      font-size: 16px;
+      line-height: 16px;
+      vertical-align: middle;
+    }
+
+    a {
+      @extend .cgray;
+
+      &:hover {
+        color: $primary_color;
+        &.danger { @extend .cred; }
+      }
+    }
+  }
+}
+.diff_file .note .note-actions {
+  right: 0;
+  top: 0;
 }
 
 .issue_notes,
@@ -18,13 +144,19 @@
   }
 }
 
-/* Note textare */
-#note_note {
-  height: 80px;
-  width: 99%;
-  font-size: 14px;
+/*
+ * New Note Form
+ */
+.new_note {
+  /* Note textare */
+  #note_note {
+    height:80px;
+    width:99%;
+    font-size:14px;
+  }
 }
 
+
 #new_note {
   .attach_holder {
     display: none;
diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb
index 79e8bcc7866..ca22af0de83 100644
--- a/app/controllers/notes_controller.rb
+++ b/app/controllers/notes_controller.rb
@@ -6,13 +6,15 @@ class NotesController < ProjectResourceController
   respond_to :js
 
   def index
+    @target_note = Note.new(noteable_type: params[:target_type].camelize,
+                            noteable_id: params[:target_id])
+    @target = @target_note.noteable
     @notes = Notes::LoadContext.new(project, current_user, params).execute
 
     if params[:target_type] == "merge_request"
-      @mixed_targets    = true
-      @main_target_type = params[:target_type].camelize
-      @discussions      = discussions_from_notes
-      @has_diff         = true
+      @has_diff      = true
+      @mixed_targets = true
+      @discussions   = discussions_from_notes
     elsif params[:target_type] == "commit"
       @has_diff = true
     end
@@ -72,6 +74,6 @@ class NotesController < ProjectResourceController
 
   # Helps to distinguish e.g. commit notes in mr notes list
   def for_main_target?(note)
-    !@mixed_targets || (@main_target_type == note.noteable_type && !note.for_diff_line?)
+    !@mixed_targets || (@target.class.name == note.noteable_type && !note.for_diff_line?)
   end
 end
diff --git a/app/helpers/notes_helper.rb b/app/helpers/notes_helper.rb
index 5cada379c3c..11d3a2ecc6c 100644
--- a/app/helpers/notes_helper.rb
+++ b/app/helpers/notes_helper.rb
@@ -9,13 +9,18 @@ module NotesHelper
 
    # 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 && !note.for_diff_line?)
+    !@mixed_targets || (@target.class.name == note.noteable_type && !note.for_diff_line?)
   end
 
   def link_to_commit_diff_line_note(note)
     if note.for_commit_diff_line?
       link_to "#{note.diff_file_name}:L#{note.diff_new_line}", project_commit_path(@project, note.noteable, anchor: note.line_code)
     end
+  end
 
+  def link_to_merge_request_diff_line_note(note)
+    if note.for_merge_request_diff_line?
+      link_to "#{note.diff_file_name}:L#{note.diff_new_line}", diffs_project_merge_request_path(note.project, note.noteable_id, anchor: note.line_code)
+    end
   end
 end
diff --git a/app/models/note.rb b/app/models/note.rb
index 6708fbc3758..a7bde1c5d8c 100644
--- a/app/models/note.rb
+++ b/app/models/note.rb
@@ -79,7 +79,7 @@ class Note < ActiveRecord::Base
   end
 
   def discussion_id
-    @discussion_id ||= [noteable_type, noteable_id, line_code].join.underscore.to_sym
+    @discussion_id ||= [:discussion, noteable_type.underscore, noteable_id, line_code].join("-").to_sym
   end
 
   # Returns true if this is a downvote note,
diff --git a/app/views/merge_requests/diffs.html.haml b/app/views/merge_requests/diffs.html.haml
index a755491c42e..2a5b8b1441e 100644
--- a/app/views/merge_requests/diffs.html.haml
+++ b/app/views/merge_requests/diffs.html.haml
@@ -1,6 +1 @@
 = render "show"
-
-:javascript
-  $(function(){
-    PerLineNotes.init();
-  });
diff --git a/app/views/notes/_discussion.html.haml b/app/views/notes/_discussion.html.haml
new file mode 100644
index 00000000000..8c050216b82
--- /dev/null
+++ b/app/views/notes/_discussion.html.haml
@@ -0,0 +1,46 @@
+- note = discussion_notes.first
+.discussion.js-details-container.js-toggler-container.open{ class: note.discussion_id }
+  .discussion-header
+    .discussion-actions
+      = link_to "javascript:;", class: "js-details-target turn-on js-toggler-target" do
+        %i.icon-eye-close
+        Hide discussion
+      = link_to "javascript:;", class: "js-details-target turn-off js-toggler-target" do
+        %i.icon-eye-open
+        Show discussion
+    = image_tag gravatar_icon(note.author.email), class: "avatar s32"
+    %div
+      = link_to note.author_name, project_team_member_path(@project, @project.team_member_by_id(note.author)), class: "note-author"
+      - if note.for_merge_request?
+        started a discussion on this merge request diff
+        = link_to_merge_request_diff_line_note(note)
+      - elsif note.for_commit?
+        started a discussion on commit
+        #{link_to note.noteable.short_id, project_commit_path(@project, note.noteable)}
+        = link_to_commit_diff_line_note(note) if note.for_diff_line?
+      - else
+        %cite.cgray started a discussion
+    %div
+      - if discussion_notes.size > 1
+        - last_note = discussion_notes.last
+        last updated by
+        = link_to last_note.author_name, project_team_member_path(@project, @project.team_member_by_id(last_note.author)), class: "note-author"
+        %span.discussion-last-update
+          = time_ago_in_words(last_note.updated_at)
+          ago
+  .discussion-body
+    - if note.for_diff_line?
+      .diff_file.content
+        = render "notes/discussion_diff", discussion_notes: discussion_notes, note: note
+    - else
+      .notes.content
+        = render discussion_notes
+
+    -# will be shown when the other one is hidden
+    .discussion-hidden.content.hide
+      .note
+        %em Hidden discussion.
+        = link_to "javascript:;", class: "js-details-target js-toggler-target" do
+          %i.icon-eye-open
+          Show
+
diff --git a/app/views/notes/_discussion_diff.html.haml b/app/views/notes/_discussion_diff.html.haml
new file mode 100644
index 00000000000..4cd227d68c6
--- /dev/null
+++ b/app/views/notes/_discussion_diff.html.haml
@@ -0,0 +1,24 @@
+- diff = note.diff
+.diff_file_header
+  %i.icon-file
+  - if diff.deleted_file
+    %span{id: "#{diff.a_path}"}= diff.a_path
+  - else
+    %span{id: "#{diff.b_path}"}= diff.b_path
+  %br/
+.diff_file_content
+  %table
+    - each_diff_line(diff.diff.lines.to_a, note.diff_file_index) do |line, type, line_code, line_new, line_old|
+      %tr.line_holder{ id: line_code }
+        - if type == "match"
+          %td.old_line= "..."
+          %td.new_line= "..."
+          %td.line_content.matched= line
+        - else
+          %td.old_line= raw(type == "new" ? "&nbsp;" : line_old)
+          %td.new_line= raw(type == "old" ? "&nbsp;" : line_new)
+          %td.line_content{class: "noteable_line #{type} #{line_code}", "line_code" => line_code}= raw "#{line} &nbsp;"
+
+          - if line_code == note.line_code
+            = render "notes/per_line_notes_with_reply", notes: discussion_notes
+            - break # cut off diff after notes
diff --git a/app/views/notes/_note.html.haml b/app/views/notes/_note.html.haml
index 70baa212d10..cf88d2912e0 100644
--- a/app/views/notes/_note.html.haml
+++ b/app/views/notes/_note.html.haml
@@ -1,17 +1,18 @@
-%li{id: dom_id(note), class: "note"}
-  = image_tag gravatar_icon(note.author.email), class: "avatar s32"
-  %div.note-author
-    %strong= note.author_name
-    = link_to "##{dom_id(note)}", name: dom_id(note) do
-      %cite.cgray
-        = time_ago_in_words(note.updated_at)
-        ago
-
-    - unless note_for_main_target?(note)
-      - if note.for_commit?
-        %span.cgray
-          on #{link_to note.noteable.short_id, project_commit_path(@project, note.noteable)}
-          = link_to_commit_diff_line_note(note) if note.for_diff_line?
+%li{ id: dom_id(note), class: dom_class(note), data: { discussion: note.discussion_id } }
+  .note-header
+    .note-actions
+      = link_to "##{dom_id(note)}", name: dom_id(note) do
+        %i.icon-link
+        Link here
+      &nbsp;
+      - if(note.author_id == current_user.id) || can?(current_user, :admin_note, @project)
+        = link_to project_note_path(@project, note), method: :delete, confirm: 'Are you sure?', remote: true, class: "danger js-note-delete" do
+          %i.icon-remove-circle
+    = image_tag gravatar_icon(note.author.email), class: "avatar s32"
+    = link_to note.author_name, project_team_member_path(@project, @project.team_member_by_id(note.author)), class: "note-author"
+    %span.note-last-update
+      = time_ago_in_words(note.updated_at)
+      ago
 
     -# only show vote if it's a note for the main target
     - if note_for_main_target?(note)
@@ -24,13 +25,8 @@
           %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
-        Remove
 
-  %div.note-title
+  .note-body
     = preserve do
       = markdown(note.note)
     - if note.attachment.url
diff --git a/app/views/notes/_notes.html.haml b/app/views/notes/_notes.html.haml
index adb5dfcbf18..4904249aeff 100644
--- a/app/views/notes/_notes.html.haml
+++ b/app/views/notes/_notes.html.haml
@@ -1,4 +1,11 @@
-- @notes.each do |note|
-  - next unless note.author
-  = render "note", note: note
-
+- if @discussions.present?
+  - @discussions.each do |discussion_notes|
+    - note = discussion_notes.first
+    - if note_for_main_target?(note)
+      = render discussion_notes
+    - else
+      = render 'discussion', discussion_notes: discussion_notes
+- else
+  - @notes.each do |note|
+    - next unless note.author
+    = render 'note', note: note
diff --git a/app/views/notes/_per_line_form.html.haml b/app/views/notes/_per_line_form.html.haml
index 460d49522bf..9210be977f6 100644
--- a/app/views/notes/_per_line_form.html.haml
+++ b/app/views/notes/_per_line_form.html.haml
@@ -17,7 +17,7 @@
             .note_actions
               .buttons
                 = f.submit 'Add Comment', class: "btn save-btn submit_note submit_inline_note", id: "submit_note"
-                = link_to "Cancel", "#", class: "btn hide-button"
+                = link_to "Cancel", "javascript:;", class: "btn hide-button"
               .options
                 %h6.left Notify via email:
                 .labels
@@ -29,11 +29,3 @@
                     = label_tag :notify_author do
                       = check_box_tag :notify_author, 1 , @note.noteable_type == "Commit"
                       %span Commit author
-
-:javascript
-  $(function(){
-    $(".per_line_form .hide-button").bind("click", function(){
-      $('.per_line_form').hide();
-      return false;
-    });
-  });
diff --git a/app/views/notes/_per_line_note_link.html.haml b/app/views/notes/_per_line_note_link.html.haml
index 16db3d809cb..d25577eac5a 100644
--- a/app/views/notes/_per_line_note_link.html.haml
+++ b/app/views/notes/_per_line_note_link.html.haml
@@ -1,6 +1,6 @@
 = link_to "",
           "#",
-          id: "line-note-#{line_code}",
-          class: "line_note_link",
+          id: "add-diff-line-note-#{line_code}",
+          class: "line_note_link js-note-add-to-diff-line",
           data: @comments_target.merge({ line_code: line_code }),
-          title: "Add comment on line #{line_code[/[0-9]+$/]}"
+          title: "Add a comment to this line"
diff --git a/app/views/notes/_per_line_reply_button.html.haml b/app/views/notes/_per_line_reply_button.html.haml
index c9d2696675e..edd84eaef2d 100644
--- a/app/views/notes/_per_line_reply_button.html.haml
+++ b/app/views/notes/_per_line_reply_button.html.haml
@@ -1,10 +1,10 @@
 %tr.line_notes_row.reply
   %td{colspan: 3}
-    = link_to "#",
-              class: "line_note_reply_link",
+    = link_to "javascript:;",
+              class: "line_note_reply_link js-note-add-to-diff-line",
               data: { line_code:     note.line_code,
                       noteable_type: note.noteable_type,
                       noteable_id:   note.noteable_id },
-              title: "Add note for this line" do
+              title: "Add a comment to this line" do
       %i.icon-comment
       Reply
diff --git a/app/views/notes/index.js.haml b/app/views/notes/index.js.haml
index 3814dbd46a2..99da619c649 100644
--- a/app/views/notes/index.js.haml
+++ b/app/views/notes/index.js.haml
@@ -15,3 +15,7 @@
   - if loading_more_notes?
     :plain
       NoteList.finishedLoadingMore();
+
+- if @has_diff
+  :plain
+    PerLineNotes.init();