Merge branch 'refactor/notes' of /home/git/repositories/gitlab/gitlabhq

app/assets/stylesheets/sections/notes.scss

@@ -257,12 +257,12 @@ ul.notes {
 .file,
 .discussion {
   .new_note {
-    margin: 8px 5px 8px 0;
+    margin: 0;
+    border: none;
   }
 }
 .new_note {
   display: none;
-
   .buttons {
     float: left;
     margin-top: 8px;

app/controllers/projects/commit_controller.rb

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

app/controllers/projects/issues_controller.rb

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

app/controllers/projects/merge_requests_controller.rb

@@ -198,6 +198,9 @@ 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)
+    @noteable = @merge_request
 
     # Get commits from repository
     # or from cache if already merged
@@ -205,9 +208,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

@@ -2,71 +2,54 @@ class Projects::NotesController < Projects::ApplicationController
   # Authorize
   before_filter :authorize_read_note!
   before_filter :authorize_write_note!, only: [:create]
-
-  respond_to :js
+  before_filter :authorize_admin_note!, only: [:update, :destroy]
 
   def index
     @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"
-      @discussions   = discussions_from_notes
-    end
+    notes_json = { notes: [] }
 
-    respond_to do |format|
-      format.html { redirect_to :back }
-      format.json do
-        render json: {
-          html: view_to_html_string("projects/notes/_notes")
-        }
-      end
+    @notes.each do |note|
+      notes_json[:notes] << {
+        id: note.id,
+        html: note_to_html(note)
+      }
     end
+
+    render json: notes_json
   end
 
   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}
-      format.js
+      format.json { render_note_json(@note) }
+      format.html { redirect_to :back }
     end
   end
 
-  def destroy
-    @note = @project.notes.find(params[:id])
-    return access_denied! unless can?(current_user, :admin_note, @note)
-    @note.destroy
-    @note.reset_events_cache
+  def update
+    note.update_attributes(params[:note])
+    note.reset_events_cache
 
     respond_to do |format|
-      format.js { render nothing: true }
+      format.json { render_note_json(note) }
+      format.html { redirect_to :back }
     end
   end
 
-  def update
-    @note = @project.notes.find(params[:id])
-    return access_denied! unless can?(current_user, :admin_note, @note)
-
-    @note.update_attributes(params[:note])
-    @note.reset_events_cache
+  def destroy
+    note.destroy
+    note.reset_events_cache
 
     respond_to do |format|
-      format.js do
-        render js: { success: @note.valid?, id: @note.id, note: view_context.markdown(@note.note) }.to_json
-      end
-      format.html do
-        redirect_to :back
-      end
+      format.js { render nothing: true }
     end
   end
 
   def delete_attachment
-    @note = @project.notes.find(params[:id])
-    @note.remove_attachment!
-    @note.update_attribute(:attachment, nil)
+    note.remove_attachment!
+    note.update_attribute(:attachment, nil)
 
     respond_to do |format|
       format.js { render nothing: true }
@@ -77,35 +60,40 @@ class Projects::NotesController < Projects::ApplicationController
     render text: view_context.markdown(params[:note])
   end
 
-  protected
+  private
 
-  def discussion_notes_for(note)
-    @notes.select do |other_note|
-      note.discussion_id == other_note.discussion_id
-    end
+  def note
+    @note ||= @project.notes.find(params[:id])
   end
 
-  def discussions_from_notes
-    discussion_ids = []
-    discussions = []
+  def note_to_html(note)
+    render_to_string(
+      "projects/notes/_note",
+      layout: false,
+      formats: [:html],
+      locals: { note: note }
+    )
+  end
 
-    @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
+  def note_to_discussion_html(note)
+    render_to_string(
+      "projects/notes/_diff_notes_with_reply",
+      layout: false,
+      formats: [:html],
+      locals: { notes: [note] }
+    )
+  end
 
-    discussions
+  def render_note_json(note)
+    render json: {
+      id: note.id,
+      discussion_id: note.discussion_id,
+      html: note_to_html(note),
+      discussion_html: note_to_discussion_html(note)
+    }
   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?)
+  def authorize_admin_note!
+    return access_denied! unless can?(current_user, :admin_note, note)
   end
 end

app/controllers/projects/snippets_controller.rb

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

app/helpers/notes_helper.rb

 module NotesHelper
    # 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?)
+    (@noteable.class.name == note.noteable_type && !note.for_diff_line?)
   end
 
   def note_target_fields
@@ -21,14 +21,6 @@ module NotesHelper
     end
   end
 
-  def loading_more_notes?
-    params[:loading_more].present?
-  end
-
-  def loading_new_notes?
-    params[:loading_new].present?
-  end
-
   def note_timestamp(note)
     # Shows the created at time and the updated at time if different
     ts = "#{time_ago_with_tooltip(note.created_at, 'bottom', 'note_created_ago')} ago"
@@ -41,4 +33,13 @@ module NotesHelper
     end
     ts.html_safe
   end
+
+  def noteable_json(noteable)
+    {
+      id: noteable.id,
+      class: noteable.class.name,
+      resources: noteable.class.table_name,
+      project_id: noteable.project.id,
+    }.to_json
+  end
 end

app/models/note.rb

@@ -56,29 +56,52 @@ 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)
-    body = "_Status changed to #{status}#{' by ' + source.gfm_reference if source}_"
-
-    create({
-      noteable: noteable,
-      project: project,
-      author: author,
-      note: body,
-      system: true
-    }, without_protection: true)
-  end
+  class << self
+    def create_status_change_note(noteable, project, author, status, source)
+      body = "_Status changed to #{status}#{' by ' + source.gfm_reference if source}_"
+
+      create({
+        noteable: noteable,
+        project: project,
+        author: author,
+        note: body,
+        system: true
+      }, without_protection: true)
+    end
+
+    # +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 create_cross_reference_note(noteable, mentioner, author, project)
+      create({
+        noteable: noteable,
+        commit_id: (noteable.sha if noteable.respond_to? :sha),
+        project: project,
+        author: author,
+        note: "_mentioned in #{mentioner.gfm_reference}_",
+        system: true
+      }, without_protection: true)
+    end
 
-  # +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)
-    create({
-      noteable: noteable,
-      commit_id: (noteable.sha if noteable.respond_to? :sha),
-      project: project,
-      author: author,
-      note: "_mentioned in #{mentioner.gfm_reference}_",
-      system: true
-    }, without_protection: true)
+    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.
@@ -89,7 +112,7 @@ class Note < ActiveRecord::Base
   def commit_author
     @commit_author ||=
       project.users.find_by_email(noteable.author_email) ||
-        project.users.find_by_name(noteable.author_name)
+      project.users.find_by_name(noteable.author_name)
   rescue
     nil
   end

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

-= form_for [@project, @note], remote: true, html: { multipart: true, id: nil, class: "new_note js-new-note-form common-note-form" }, authenticity_token: true do |f|
-
+= form_for [@project, @note], remote: true, html: { :'data-type' => 'json', multipart: true, id: nil, class: "new_note js-new-note-form common-note-form" }, authenticity_token: true do |f|
   = note_target_fields
   = f.hidden_field :commit_id
   = f.hidden_field :line_code

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

@@ -34,7 +34,7 @@
         = markdown(note.note)
 
     .note-edit-form
-      = form_for note, url: project_note_path(@project, note), method: :put, remote: true do |f|
+      = form_for note, url: project_note_path(@project, note), method: :put, remote: true, authenticity_token: true do |f|
         = f.text_area :note, class: 'note_text js-note-text js-gfm-input turn-on'
 
         .form-actions

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
+%ul#notes-list.notes.main-notes-list
+  = render "projects/notes/notes"
 .js-notes-busy
 
 .js-main-target-form
@@ -6,4 +7,4 @@
   = render "projects/notes/form"
 
 :javascript
-  NoteList.init("#{@target_id}", "#{@target_type}", "#{project_notes_path(@project)}");
+  new Notes("#{project_notes_path(target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json})

app/views/projects/notes/create.js.haml

-- if @note.valid?
-  var noteHtml = "#{escape_javascript(render @note)}";
-
-  - if note_for_main_target?(@note)
-    NoteList.appendNewNote(#{@note.id}, noteHtml);
-  - else
-    :plain
-      var firstDiscussionNoteHtml = "#{escape_javascript(render "projects/notes/diff_notes_with_reply", notes: [@note])}";
-      NoteList.appendNewDiscussionNote("#{@note.discussion_id}",
-                                       firstDiscussionNoteHtml,
-                                       noteHtml);
-
-- else
-  var errorsHtml = "#{escape_javascript(render 'projects/notes/form_errors', note: @note)}";
-  - if note_for_main_target?(@note)
-    NoteList.errorsOnForm(errorsHtml);
-  - else
-    NoteList.errorsOnForm(errorsHtml, "#{@note.discussion_id}");

features/steps/project/project_merge_requests.rb

@@ -115,19 +115,26 @@ class ProjectMergeRequests < Spinach::FeatureSteps
   And 'I leave a comment on the diff page' do
     init_diff_note
 
-    within('.js-temp-notes-holder') do
+    within('.js-discussion-note-form') do
       fill_in "note_note", with: "One comment to rule them all"
       click_button "Add Comment"
     end
+
+    within ".note-text" do
+      page.should have_content "One comment to rule them all"
+    end
   end
 
   And 'I leave a comment like "Line is wrong" on line 185 of the first file' do
     init_diff_note
 
-    within(".js-temp-notes-holder") do
+    within(".js-discussion-note-form") do
       fill_in "note_note", with: "Line is wrong"
       click_button "Add Comment"
-      sleep 0.05
+    end
+
+    within ".note-text" do
+      page.should have_content "Line is wrong"
     end
   end
 

spec/features/notes_on_merge_requests_spec.rb

@@ -108,7 +108,7 @@ describe "On a merge request", js: true do
 
         within("#note_#{note.id}") do
           should have_css(".note-last-update small")
-          find(".note-last-update small").text.should match(/Edited just now/)
+          find(".note-last-update small").text.should match(/Edited less than a minute ago/)
         end
       end
     end