Merge branch 'riyad-discussions'

app/assets/javascripts/behaviors/details_behavior.coffee

+$ ->
+  $("body").on "click", ".js-details-target", ->
+    container = $(@).closest(".js-details-container")
+
+    container.toggleClass("open")

app/assets/javascripts/behaviors/toggler_behavior.coffee

+$ ->
+  $("body").on "click", ".js-toggler-target", ->
+    container = $(@).closest(".js-toggler-container")
+
+    container.toggleClass("on")

app/assets/javascripts/extensions/array.js

+Array.prototype.first = function() {
+  return this[0];
+}
+
+Array.prototype.last = function() {
+  return this[this.length-1];
+}
\ No newline at end of file

app/assets/stylesheets/application.scss

@@ -46,3 +46,8 @@
 @import "themes/ui_gray.scss";
 @import "themes/ui_color.scss";
+/**
+ * Styles for JS behaviors.
+ */
+@import "behaviors.scss";
+

app/assets/stylesheets/behaviors.scss

+// Details
+//--------
+.js-details-container .content { display: none; }
+.js-details-container .content.hide { display: block; }
+.js-details-container.open .content { display: block; }
+.js-details-container.open .content.hide { display: none; }
+
+
+// Toggler
+//--------
+.js-toggler-container .turn-on  { display: inherit; }
+.js-toggler-container .turn-off { display: none; }
+.js-toggler-container.on .turn-on  { display: none; }
+.js-toggler-container.on .turn-off { display: inherit; }

app/assets/stylesheets/common.scss

@@ -546,3 +546,9 @@ h1.http_status_code {
     }
   }
 }
+
+img.emoji {
+  height: 20px;
+  vertical-align: middle;
+  width: 20px;
+}

app/assets/stylesheets/sections/commits.scss

@@ -119,12 +119,14 @@
       }
     }
   }
-  .old_line, .new_line {
-    margin: 0px;
-    padding: 0px;
-    border: none;
-    background: #EEE;
-    color: #666;
+  .new_line,
+  .old_line,
+  .notes_line {
+    margin:0px;
+    padding:0px;
+    border:none;
+    background:#EEE;
+    color:#666;
     padding: 0px 5px;
     border-right: 1px solid #ccc;
     text-align: right;
@@ -134,6 +136,7 @@
     moz-user-select: none;
     -khtml-user-select: none;
     user-select: none;
+
     a {
       float: left;
       width: 35px;

app/assets/stylesheets/sections/notes.scss

 /**
  * Notes
- *
  */
-#notes-list,
-#new-notes-list {
+ul.notes {
   display: block;
   list-style: none;
   margin: 0px;
   padding: 0px;
-}
-.issue_notes,
-.wiki_notes {
-  .note_content {
-    float: left;
-    width: 400px;
-  }
-}
-/* Note textare */
-#note_note {
-  height: 80px;
-  width: 98%;
-  font-size: 14px;
-}
-#new_note {
-  .attach_holder {
-    display: none;
+  .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;
+      }
+    }
   }
-}
-.preview_note {
-  margin: 2px;
-  border: 1px solid #ddd;
-  padding: 10px;
-  min-height: 60px;
-  background: #f5f5f5;
-}
-.note {
-  padding: 8px 0;
-  overflow: hidden;
-  display: block;
-  position: relative;
-  img {float: left; margin-right: 10px;}
-  img.emoji {float: none;margin: 0;}
-  .note-author cite{font-style: italic;}
-  p { color: $style_color; }
-  .note-author { color: $style_color;}
-  .note-title { margin-left: 45px; padding-top: 5px;}
-  .avatar {
-    margin-top: 3px;
-  }
-  .delete-note {
-    display: none;
-    position: absolute;
-    right: 0;
-    top: 0;
+  .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 .notes {
+        /* reset */
+        background: inherit;
+        border: none;
+        @include box-shadow(none);
+      }
+      .discussion-hidden .note {
+        @extend .cgray;
+        padding: 8px;
+        text-align: center;
+      }
+      .notes .note {
+        border-color: #ddd;
+        padding: 8px;
+      }
+      .reply-btn {
+        margin-top: 8px;
+      }
+    }
   }
-  &:hover {
-    .delete-note { display: block; }
-  }
-}
-#notes-list:not(.reversed) .note,
-#new-notes-list:not(.reversed) .note {
-  border-bottom: 1px solid #eee;
-}
-#notes-list.reversed .note,
-#new-notes-list.reversed .note {
-  border-top: 1px solid #eee;
-}
-
-/* mark vote notes */
-.voting_notes .note {
-  padding: 8px 0;
-}
-.notes-status {
-  margin: 18px;
-}
-p.notify_controls input{
-  margin: 5px;
+  .note {
+    padding: 8px 0;
+    overflow: hidden;
+    display: block;
+    position:relative;
+    p { color: $style_color; }
+    .avatar {
+      margin-top: 3px;
+    }
+    .attachment {
+      font-size: 14px;
+      margin-top: -20px;
+      .icon-attachment {
+        @extend .icon-paper-clip;
+        font-size: 24px;
+        position: relative;
+        text-align: right;
+        top: 6px;
+      }
+    }
+    .note-body {
+      margin-left: 45px;
+    }
+    .note-header {
+      padding-bottom: 5px;
+    }
+  }
+  // paint top or bottom borders depending on notes direction
+  &:not(.reversed) .note,
+  &:not(.reversed) .discussion {
+    border-bottom: 1px solid #eee;
+  }
+  &.reversed .note,
+  &.reversed .discussion {
+    border-top: 1px solid #eee;
+  }
 }
-p.notify_controls span{
-  font-weight: 700;
-}
-tr.line_notes_row {
-  border-bottom: 1px solid #DDD;
-  border-left: 7px solid #2A79A3;
-  &.reply {
-    background: #eee;
-    border-left: 7px solid #2A79A3;
-    border-top: 1px solid #ddd;
-    td {
-      padding: 7px 10px;
+.diff_file .notes_holder {
+  font-family: $sansFontFamily;
+  font-size: 13px;
+  line-height: 18px;
+  td {
+    border: 1px solid #ddd;
+    border-left: none;
+    &.notes_line {
+      text-align: center;
+      padding: 10px 0;
     }
-    a.line_note_reply_link {
-      border: 1px solid #eaeaea;
-      @include border-radius(4px);
-      padding: 3px 10px;
-      margin-left: 5px;
-      color: white;
-      background: #2A79A3;
-      border-color: #2A79A3;
+    &.notes_content {
+      background-color: $white;
+      border-width: 1px 0;
+      padding-top: 0;
     }
   }
-  ul {
-    margin: 0;
-    li {
-      padding: 0;
-      border: none;
-    }
+
+  .reply-btn {
+    margin-top: 8px;
   }
 }
-.line_notes_row,  .per_line_form { font-family: "Helvetica Neue", Helvetica, Arial, sans-serif; }
-.per_line_form {
-  background: #f5f5f5;
-  border-top: 1px solid #eee;
-  form {  margin: 0; }
-  td {
-    border-bottom: 1px solid #ddd;
+
+/**
+ * Actions for Discussions/Notes
+ */
+
+.discussion,
+.note {
+  &.note:hover {
+    .note-actions { display: block; }
+  }
+  .discussion-header:hover {
+    .discussion-actions { display: block; }
   }
-  .note_actions {
-    margin: 0;
-    padding-top: 10px;
-    .buttons {
-      float: left;
-      width: 300px;
-    }
-    .options {
-      .labels {
-        float: left;
-        padding-left: 10px;
-        label {
-          padding: 6px 0;
-          margin: 0;
-          width: 120px;
-        }
+  .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; }
       }
     }
   }
 }
-td .line_note_link {
-  position: absolute;
-  margin-left:-70px;
-  margin-top:-10px;
-  z-index: 10;
-  background: url("comment_add.png") no-repeat left 0;
-  width: 32px;
-  height: 32px;
-  opacity: 0.0;
-  filter: alpha(opacity=0);
-  &:hover {
-    opacity: 1.0;
-    filter: alpha(opacity=100);
+.diff_file .note .note-actions {
+  right: 0;
+  top: 0;
+}
+
+
+
+/**
+ * Line note button on the side of diffs
+ */
+.diff_file tr.line_holder {
+  .add-diff-note {
+    background: url("diff_note_add.png") no-repeat left 0;
+    height: 22px;
+    margin-left: -65px;
+    position: absolute;
+    width: 22px;
+    z-index: 10;
+    // "hide" it by default
+    opacity: 0.0;
+    filter: alpha(opacity=0);
+    &:hover {
+      opacity: 1.0;
+      filter: alpha(opacity=100);
+    }
+  }
+
+  // "show" the icon also if we just hover somwhere over the line
+  &:hover > td {
+    background: $hover !important;
+
+    .add-diff-note {
+      opacity: 1.0;
+      filter: alpha(opacity=100);
+    }
   }
 }
-.diff_file_content tr.line_holder:hover > td { background: $hover !important; }
-.diff_file_content tr.line_holder:hover > td .line_note_link {
-  opacity: 1.0;
-  filter: alpha(opacity=100);
+
+
+/**
+ * Note Form
+ */
+
+.comment-btn,
+.reply-btn {
+  @extend .save-btn;
 }
-.new_note {
-  .input-file {
-    font: 500px monospace;
-    opacity: 0;
-    filter: alpha(opacity=0);
-    position: absolute;
-    z-index: 1;
-    top: 0;
-    right: 0;
-    padding: 0;
-    margin: 0;
+.diff_file,
+.discussion {
+  .new_note {
+    margin: 8px 5px 8px 0;
+    .note_options {
+      // because of the smaller width and the extra "cancel" button
+      margin-top: 8px;
+    }
   }
-  .note_advanced_opts {
+}
+.new_note {
+  display: none;
+  .buttons {
+    float: left;
+    margin-top: 8px;
+  }
+  .clearfix {
+    margin-bottom: 0;
+  }
+  .note_options {
     h6 {
-      line-height: 32px;
-      padding-right: 15px;
+      @extend .left;
+      line-height: 20px;
+      padding-right: 16px;
+      padding-bottom: 16px;
+    }
+    label {
+      padding: 0;
     }
-  }
-  .attachments {
-    position: relative;
-    width: 350px;
-    height: 50px;
-    overflow: hidden;
-    margin:0 0 5px !important;
-    .input_file {
-      .file_upload {
-        position: absolute;
-        right: 14px;
-        top: 7px;
+    .attachment {
+      @extend .right;
+      position: relative;
+      width: 350px;
+      height: 50px;
+      margin:0 0 5px !important;
+      // hide the actual file field
+      input {
+        display: none;
       }
-      .file_name {
-        line-height: 30px;
-        width: 240px;
-        height: 28px;
-        overflow: hidden;
-      }
-      .input-file {
-        width: 260px;
-        height: 41px;
+      .choose-btn {
         float: right;
       }
     }
+    .notify_options {
+      @extend .right;
+    }
   }
+  .note_text_and_preview {
+    // makes the "absolute" position for links relative to this
+    position: relative;
+
+    // preview/edit buttons
+    > a {
+      font-size: 24px;
+      padding: 4px;
+      position: absolute;
+      right: 10px;
+    }
+    .note_preview {
+      background: #f5f5f5;
+      border: 1px solid #ddd;
+      @include border-radius(4px);
+      min-height: 80px;
+      padding: 4px 6px;
+    }
+    .note_text {
+      border: 1px solid #DDD;
+      box-shadow: none;
+      font-size: 14px;
+      height: 80px;
+      width: 98.6%;
+    }
+  }
+}
+
+/* loading indicator */
+.notes-busy {
+  margin: 18px;
 }
-.note-text {
-  border: 1px solid #DDD;
-  box-shadow: none;
+.note-image-attach {
+  @extend .span4;
+  @extend .thumbnail;
+  margin-left: 45px;
 }

app/contexts/notes/create_context.rb

@@ -3,8 +3,8 @@ module Notes
     def execute
       note = project.notes.new(params[:note])
       note.author = current_user
-      note.notify = true if params[:notify] == '1'
-      note.notify_author = true if params[:notify_author] == '1'
+      note.notify = params[:notify].present?
+      note.notify_author = params[:notify_author].present?
       note.save
       note
     end

app/contexts/notes/load_context.rb

@@ -9,11 +9,11 @@ module Notes
       @notes = case target_type
                when "commit"
-                 project.notes.for_commit_id(target_id).not_inline.fresh.limit(20)
+                 project.notes.for_commit_id(target_id).not_inline.fresh
                when "issue"
-                 project.issues.find(target_id).notes.inc_author.fresh.limit(20)
+                 project.issues.find(target_id).notes.inc_author.fresh
                when "merge_request"
-                 project.merge_requests.find(target_id).mr_and_commit_notes.inc_author.fresh.limit(20)
+                 project.merge_requests.find(target_id).mr_and_commit_notes.inc_author.fresh
                when "snippet"
                  project.snippets.find(target_id).notes.fresh
                when "wall"

app/controllers/commit_controller.rb

@@ -13,11 +13,17 @@ class CommitController < ProjectResourceController
     @commit = result[:commit]
     git_not_found! unless @commit
-    @suppress_diff    = result[:suppress_diff]
-    @note             = result[:note]
-    @line_notes       = result[:line_notes]
-    @notes_count      = result[:notes_count]
-    @comments_allowed = true
+    @suppress_diff = result[:suppress_diff]
+
+    @note        = result[:note]
+    @line_notes  = result[:line_notes]
+    @notes_count = result[:notes_count]
+    @target_type = :commit
+    @target_id   = @commit.id
+
+    @comments_allowed = @reply_allowed = true
+    @comments_target  = { noteable_type: 'Commit',
+                          commit_id: @commit.id }
     respond_to do |format|
       format.html do

app/controllers/issues_controller.rb

@@ -35,6 +35,8 @@ class IssuesController < ProjectResourceController
   def show
     @note = @project.notes.new(noteable: @issue)
+    @target_type = :issue
+    @target_id = @issue.id
     respond_to do |format|
       format.html

app/controllers/merge_requests_controller.rb

@@ -18,6 +18,9 @@ class MergeRequestsController < ProjectResourceController
   end
   def show
+    @target_type = :merge_request
+    @target_id = @merge_request.id
+
     respond_to do |format|
       format.html
       format.js
@@ -31,7 +34,9 @@ class MergeRequestsController < ProjectResourceController
     @diffs = @merge_request.diffs
     @commit = @merge_request.last_commit
-    @comments_allowed = true
+    @comments_allowed = @reply_allowed = true
+    @comments_target  = { noteable_type: 'MergeRequest',
+                          noteable_id: @merge_request.id }
     @line_notes = @merge_request.notes.where("line_code is not null")
   end

app/controllers/notes_controller.rb

@@ -6,10 +6,12 @@ class NotesController < ProjectResourceController
   respond_to :js
   def index
-    notes
+    @notes = Notes::LoadContext.new(project, current_user, params).execute
+    @target_type = params[:target_type].camelize
+    @target_id = params[:target_id]
+
     if params[:target_type] == "merge_request"
-      @mixed_targets = true
-      @main_target_type = params[:target_type].camelize
+      @discussions   = discussions_from_notes
     end
     respond_with(@notes)
@@ -17,6 +19,8 @@ class NotesController < ProjectResourceController
   def create
     @note = Notes::CreateContext.new(project, current_user, params).execute
+    @target_type = params[:target_type].camelize
+    @target_id = params[:target_id]
     respond_to do |format|
       format.html {redirect_to :back}
@@ -40,7 +44,34 @@ class NotesController < ProjectResourceController
   protected
-  def notes
-    @notes = Notes::LoadContext.new(project, current_user, params).execute
+  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)
+    note.for_wall? ||
+      (@target_type.camelize == note.noteable_type && !note.for_diff_line?)
   end
 end

app/controllers/projects_controller.rb

@@ -80,7 +80,10 @@ class ProjectsController < ProjectResourceController
   def wall
     return render_404 unless @project.wall_enabled
-    @note = Note.new
+
+    @target_type = :wall
+    @target_id = nil
+    @note = @project.notes.new
     respond_to do |format|
       format.html

app/controllers/snippets_controller.rb

@@ -50,6 +50,8 @@ class SnippetsController < ProjectResourceController
   def show
     @note = @project.notes.new(noteable: @snippet)
+    @target_type = :snippet
+    @target_id = @snippet.id
   end
   def destroy

app/helpers/commits_helper.rb

@@ -9,11 +9,13 @@ module CommitsHelper
     end
   end
-  def build_line_anchor(index, line_new, line_old)
-    "#{index}_#{line_old}_#{line_new}"
+  def build_line_anchor(diff, line_new, line_old)
+    "#{hexdigest(diff.new_path)}_#{line_old}_#{line_new}"
   end
-  def each_diff_line(diff_arr, index)
+  def each_diff_line(diff, index)
+    diff_arr = diff.diff.lines.to_a
+
     line_old = 1
     line_new = 1
     type = nil
@@ -39,7 +41,7 @@ module CommitsHelper
         next
       else
         type = identification_type(line)
-        line_code = build_line_anchor(index, line_new, line_old)
+        line_code = build_line_anchor(diff, line_new, line_old)
         yield(full_line, type, line_code, line_new, line_old)
       end