Merge branch 'notes_refactoring'

app/assets/javascripts/note.js

@@ -1,15 +1,20 @@
 var NoteList = {
 
+notes_path: null,
+target_params: null,
+target_id: 0,
+target_type: null,
 first_id: 0,
 last_id: 0,
-resource_name: null,
 disable:false,
 
 init:
-  function(resource_name, first_id, last_id) {
-    this.resource_name = resource_name;
-    this.first_id = first_id;
-    this.last_id = last_id;
+  function(tid, tt, path) {
+    this.notes_path = path + ".js";
+    this.target_id = tid;
+    this.target_type = tt;
+    this.target_params = "&target_type=" + this.target_type + "&target_id=" + this.target_id
+    this.refresh();
     this.initRefresh();
     this.initLoadMore();
   },
@@ -19,8 +24,8 @@ getOld:
     $('.loading').show();
     $.ajax({
       type: "GET",
-      url: location.href,
-      data: "first_id=" + this.first_id,
+      url: this.notes_path,
+      data: "first_id=" + this.first_id + this.target_params,
       complete: function(){ $('.loading').hide()},
       dataType: "script"});
   },
@@ -56,8 +61,8 @@ getNew:
     // refersh notes list
     $.ajax({
       type: "GET",
-      url: location.href,
-      data: "last_id=" + this.last_id,
+      url: this.notes_path,
+      data: "last_id=" + this.last_id + this.target_params,
       dataType: "script"});
   },
 
@@ -66,8 +71,8 @@ refresh:
     // refersh notes list
     $.ajax({
       type: "GET",
-      url: location.href,
-      data: "first_id=" + this.first_id + "&last_id=" + this.last_id,
+      url: this.notes_path,
+      data: "first_id=" + this.first_id + "&last_id=" + this.last_id + this.target_params,
       dataType: "script"});
   },
 

app/assets/stylesheets/common.scss

@@ -374,3 +374,10 @@ table a code {
 .btn.padded { 
   margin-right:3px;
 }
+
+.loading { 
+  margin:20px auto;
+  background: url(ajax-loader-facebook.gif) no-repeat center center;
+  width:40px;
+  height:40px;
+}

app/assets/stylesheets/notes.css.scss

@@ -30,7 +30,10 @@
   }
 }
 
-.note .delete-note { display:none; }
+.note .delete-note { 
+  display:none; 
+  float:right;
+}
 .note:hover .delete-note { display:block; }
 .note {padding: 10px 0; border-bottom: 1px solid #eee; overflow: hidden; display: block;}
 .note img{float: left; margin-right: 10px;}
@@ -53,6 +56,20 @@ p.notify_controls span{
 
 tr.line_notes_row { 
   border-bottom:1px solid #DDD;
+  &.reply { 
+    background:#eee;
+
+    td { 
+      padding:7px 10px;
+    }
+    a.line_note_reply_link { 
+      @include round-borders-all(4px);
+      border-color:#aaa;
+      background: #bbb;
+      padding: 3px 20px;
+      color: white;
+    }
+  }
   ul { 
     margin:0;
     li { 

app/controllers/application_controller.rb

@@ -95,18 +95,6 @@ class ApplicationController < ActionController::Base
     redirect_to @project unless @project.repo_exists? && @project.has_commits?
   end
 
-  def respond_with_notes
-    if params[:last_id] && params[:first_id]
-      @notes = @notes.where("id >= ?", params[:first_id])
-    elsif params[:last_id]
-      @notes = @notes.where("id > ?", params[:last_id])
-    elsif params[:first_id]
-      @notes = @notes.where("id < ?", params[:first_id])
-    else
-      nil
-    end
-  end
-
   def no_cache_headers
     response.headers["Cache-Control"] = "no-cache, no-store, max-age=0, must-revalidate"
     response.headers["Pragma"] = "no-cache"

app/controllers/commits_controller.rb

@@ -29,16 +29,9 @@ class CommitsController < ApplicationController
 
     git_not_found! and return unless @commit
 
-    @notes = project.commit_notes(@commit).fresh.limit(20)
     @note = @project.build_commit_note(@commit)
-
     @comments_allowed = true
     @line_notes = project.commit_line_notes(@commit)
-
-    respond_to do |format|
-      format.html
-      format.js { respond_with_notes }
-    end
   end
 
   def compare

app/controllers/issues_controller.rb

@@ -49,7 +49,6 @@ class IssuesController < ApplicationController
   end
 
   def show
-    @notes = @issue.notes.inc_author.order("created_at DESC").limit(20)
     @note = @project.notes.new(:noteable => @issue)
 
     @commits = if @issue.branch_name && @project.repo.heads.map(&:name).include?(@issue.branch_name)
@@ -61,7 +60,7 @@ class IssuesController < ApplicationController
 
     respond_to do |format|
       format.html
-      format.js { respond_with_notes }
+      format.js
     end
   end
 

app/controllers/merge_requests_controller.rb

@@ -39,7 +39,6 @@ class MergeRequestsController < ApplicationController
       git_not_found! and return 
     end
 
-    @notes = @merge_request.notes.inc_author.order("created_at DESC").limit(20)
     @note = @project.notes.new(:noteable => @merge_request)
 
     @commits = @project.repo.
@@ -52,14 +51,16 @@ class MergeRequestsController < ApplicationController
 
     respond_to do |format|
       format.html
-      format.js { respond_with_notes }
+      format.js
     end
   end
 
   def diffs
     @diffs = @merge_request.diffs
     @commit = @merge_request.last_commit
-    @line_notes = []
+
+    @comments_allowed = true
+    @line_notes = @merge_request.notes.where("line_code is not null")
   end
 
   def new

app/controllers/notes_controller.rb

@@ -9,6 +9,25 @@ class NotesController < ApplicationController
 
   respond_to :js
 
+  def index
+    @notes = case params[:target_type]
+             when "commit" 
+               then project.commit_notes(project.commit((params[:target_id]))).fresh.limit(20)
+             when "snippet"
+               then  project.snippets.find(params[:target_id]).notes
+             when "wall"
+               then project.common_notes.order("created_at DESC").fresh.limit(20)
+             when "issue"
+               then project.issues.find(params[:target_id]).notes.inc_author.order("created_at DESC").limit(20)
+             when "merge_request"
+               then project.merge_requests.find(params[:target_id]).notes.inc_author.order("created_at DESC").limit(20)
+             end
+
+    respond_to do |format|
+      format.js { respond_with_notes }
+    end
+  end
+
   def create
     @note = @project.notes.new(params[:note])
     @note.author = current_user
@@ -34,4 +53,17 @@ class NotesController < ApplicationController
     end
   end
 
+  protected 
+
+  def respond_with_notes
+    if params[:last_id] && params[:first_id]
+      @notes = @notes.where("id >= ?", params[:first_id])
+    elsif params[:last_id]
+      @notes = @notes.where("id > ?", params[:last_id])
+    elsif params[:first_id]
+      @notes = @notes.where("id < ?", params[:first_id])
+    else
+      nil
+    end
+  end
 end

app/controllers/projects_controller.rb

@@ -82,14 +82,10 @@ class ProjectsController < ApplicationController
 
   def wall
     return render_404 unless @project.wall_enabled
-
     @note = Note.new
-    @notes = @project.common_notes.order("created_at DESC")
-    @notes = @notes.fresh.limit(20)
 
     respond_to do |format|
       format.html
-      format.js { respond_with_notes }
     end
   end
 

app/controllers/snippets_controller.rb

@@ -55,7 +55,6 @@ class SnippetsController < ApplicationController
   end
 
   def show
-    @notes = @snippet.notes
     @note = @project.notes.new(:noteable => @snippet)
     render_full_content
   end

app/views/commits/_text_file.html.haml

@@ -16,6 +16,7 @@
         - if @comments_allowed
           - comments = @line_notes.select { |n| n.line_code == line_code }.sort_by(&:created_at).reverse
           - unless comments.empty?
-            - comments.each do |note|
+            - comments.each_with_index do |note, i|
+              = render "notes/reply_button", :line_code => line_code if i.zero?
               = render "notes/per_line_show", :note => note
               - @line_notes.reject!{ |n| n == note }

app/views/commits/show.html.haml

@@ -21,13 +21,13 @@
 %p.cgray
   Showing #{pluralize(@commit.diffs.count, "changed file")}
 = render "commits/diffs", :diffs => @commit.diffs
-= render "notes/notes"
+= render "notes/notes", :tid => @commit.id, :tt => "commit"
 = render "notes/per_line_form"
 
 
 :javascript
   $(document).ready(function(){
-    $(".line_note_link").live("click", function(e) { 
+    $(".line_note_link, .line_note_reply_link").live("click", function(e) { 
       var form = $(".per_line_form");
       $(this).parent().parent().after(form);
       form.find("#note_line_code").val($(this).attr("line_code"));

app/views/issues/show.html.haml

@@ -41,5 +41,4 @@
   %div= simple_format @issue.title
 
 
-.issue_notes= render "notes/notes"
-
+.issue_notes= render "notes/notes", :tid => @issue.id, :tt => "issue"

app/views/issues/show.js.haml

@@ -1 +0,0 @@
-= render "notes/load"

app/views/merge_requests/show.html.haml

@@ -63,10 +63,7 @@
     %img{:src => "/assets/ajax-loader-facebook.gif", :class => "dashboard-loader"}
 
 .merge-request-notes
-  .merge_request_notes= render "notes/notes"
-  .loading{ :style => "display:none;"}
-    %center= image_tag "ajax-loader.gif"
-  .clear
+  .merge_request_notes= render "notes/notes", :tid => @merge_request.id, :tt => "merge_request"
 
 .merge-request-diffs
 
@@ -75,3 +72,15 @@
   $(function(){
     MergeRequest.init();
   })
+
+= render "notes/per_line_form"
+:javascript
+  $(document).ready(function(){
+    $(".line_note_link, .line_note_reply_link").live("click", function(e) { 
+      var form = $(".per_line_form");
+      $(this).parent().parent().after(form);
+      form.find("#note_line_code").val($(this).attr("line_code"));
+      form.show();  
+      return false;
+    });
+  });

app/views/notes/_create_common.js.haml

@@ -0,0 +1,9 @@
+- if note.valid?
+  :plain
+    $("#new_note .errors").remove();
+    $('#new_note textarea').val("");
+    NoteList.prepend(#{note.id}, "#{escape_javascript(render :partial => "notes/show", :locals => {:note => note})}");
+- else
+  :plain
+    $("#new_note").replaceWith("#{escape_javascript(render('form'))}");
+

app/views/notes/_create_line.js.haml

@@ -0,0 +1,8 @@
+- if note.valid?
+  :plain
+    $(".per_line_form").hide();
+    $('#new_note textarea').val("");
+    $("a.line_note_reply_link[line_code='#{note.line_code}']").closest("tr").remove();
+    var trEl = $(".#{note.line_code}").parent();
+    trEl.after("#{escape_javascript(render :partial => "notes/per_line_show", :locals => {:note => note})}");
+    trEl.after("#{escape_javascript(render :partial => "notes/reply_button", :locals => {:line_code => note.line_code})}");

app/views/notes/_notes.html.haml

@@ -2,7 +2,9 @@
   = render "notes/form"
 .clear
 %hr
-%ul#notes-list= render "notes/notes_list"
+%ul#notes-list
+.loading
+
 
 :javascript
   $('.delete-note').live('ajax:success', function() {
@@ -22,5 +24,5 @@
       $('.attach_holder').show();
     });
 
-    NoteList.init("wall", #{@notes.last.try(:id) || 0}, #{@notes.first.try(:id) || 0});
+    NoteList.init("#{tid}", "#{tt}", "#{project_notes_path(@project)}");
   });

app/views/notes/_reply_button.html.haml

@@ -0,0 +1,3 @@
+%tr.line_notes_row.reply
+  %td{:colspan => 3} 
+    = link_to "Reply", "#", :class => "line_note_reply_link", "line_code" => line_code, :title => "Add note for this line" 

app/views/notes/_show.html.haml

@@ -6,7 +6,7 @@
       = time_ago_in_words(note.updated_at)
       ago
     - if(note.author_id == current_user.id) || can?(current_user, :admin_note, @project)
-      = link_to "Remove", [@project, note], :confirm => 'Are you sure?', :method => :delete, :remote => true, :class => "cred delete-note right"
+      %strong= link_to "Remove", [@project, note], :confirm => 'Are you sure?', :method => :delete, :remote => true, :class => "cred delete-note btn small"
 
   %div.note-title
     = markdown(note.note)

app/views/notes/create.js.haml

@@ -1,18 +1,8 @@
-- if @note.valid?
-  - if @note.line_code
-    :plain
-      $(".per_line_form").hide();
-      $('#new_note textarea').val("");
-      $(".#{@note.line_code}").parent().after("#{escape_javascript(render :partial => "notes/per_line_show", :locals => {:note => @note})}");
-  - else
-    :plain
-      $("#new_note .errors").remove();
-      $('#new_note textarea').val("");
-      NoteList.prepend(#{@note.id}, "#{escape_javascript(render :partial => "notes/show", :locals => {:note => @note})}");
-- else
-  - unless @note.line_code
-    :plain
-      $("#new_note").replaceWith("#{escape_javascript(render('form'))}");
+- if @note.line_code
+  = render "create_line", :note => @note
+- else 
+  = render "create_common", :note => @note
 
+-# Enable submit button
 :plain
   $("#submit_note").removeAttr("disabled");

app/views/projects/wall.html.haml

@@ -1,6 +1,2 @@
 %div.wall_page
-  = render "notes/notes"
-
-.loading{ :style => "display:none;"}
-  %center= image_tag "ajax-loader.gif"
-
+  = render "notes/notes", :tid => nil, :tt => "wall"

app/views/projects/wall.js.haml

@@ -1 +0,0 @@
-= render "notes/load"

app/views/snippets/show.html.haml

@@ -14,8 +14,5 @@
   .data.no-padding
     :erb
       <%= raw @snippet.colorize %>
-.clear
-%br
-.snippet_notes= render "notes/notes"
 
-.clear
+= render "notes/notes", :tid => @snippet.id, :tt => "snippet"

config/routes.rb

@@ -121,7 +121,7 @@ Gitlab::Application.routes.draw do
         get   :search
       end
     end
-    resources :notes, :only => [:create, :destroy]
+    resources :notes, :only => [:index, :create, :destroy]
   end
   root :to => "projects#index"
 end

spec/requests/projects_wall_spec.rb

@@ -8,7 +8,7 @@ describe "Projects", "Wall" do
     project.add_access(@user, :read, :write)
   end
 
-  describe "View notes on wall" do
+  describe "View notes on wall", :js => true do
     before do
       Factory :note, :project => project, :note => "Project specs", :author => @user
       visit wall_project_path(project)