Merge branch 'notes_finder_updated_at' into 'master'

app/assets/javascripts/notes.js.coffee

 class Notes
   @interval: null
 
-  constructor: (notes_url, note_ids) ->
+  constructor: (notes_url, note_ids, last_fetched_at) ->
     @notes_url = notes_url
     @notes_url = gon.relative_url_root + @notes_url if gon.relative_url_root?
     @note_ids = note_ids
+    @last_fetched_at = last_fetched_at
     @initRefresh()
     @setupMainTargetNoteForm()
     @cleanBinding()
@@ -76,9 +77,11 @@ class Notes
   getContent: ->
     $.ajax
       url: @notes_url
+      data: "last_fetched_at=" + @last_fetched_at
       dataType: "json"
       success: (data) =>
         notes = data.notes
+        @last_fetched_at = data.last_fetched_at
         $.each notes, (i, note) =>
           @renderNote(note)
 

app/controllers/projects/notes_controller.rb

@@ -5,9 +5,10 @@ class Projects::NotesController < Projects::ApplicationController
   before_filter :authorize_admin_note!, only: [:update, :destroy]
 
   def index
+    current_fetched_at = Time.now.to_i
     @notes = NotesFinder.new.execute(project, current_user, params)
 
-    notes_json = { notes: [] }
+    notes_json = { notes: [], last_fetched_at: current_fetched_at }
 
     @notes.each do |note|
       notes_json[:notes] << {

app/finders/notes_finder.rb

 class NotesFinder
+  FETCH_OVERLAP = 5.seconds
+
   def execute(project, current_user, params)
     target_type = params[:target_type]
     target_id   = params[:target_id]
+    # Default to 0 to remain compatible with old clients
+    last_fetched_at = Time.at(params.fetch(:last_fetched_at, 0).to_i)
 
-    case target_type
+    notes = case target_type
     when "commit"
       project.notes.for_commit_id(target_id).not_inline.fresh
     when "issue"
@@ -12,6 +16,11 @@ class NotesFinder
       project.merge_requests.find(target_id).mr_and_commit_notes.inc_author.fresh
     when "snippet"
       project.snippets.find(target_id).notes.fresh
+    else
+      raise 'invalid target_type'
     end
+
+    # Use overlapping intervals to avoid worrying about race conditions
+    notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP)
   end
 end

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

@@ -7,4 +7,4 @@
   = render "projects/notes/form"
 
 :javascript
-  new Notes("#{project_notes_path(target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json})
+  new Notes("#{project_notes_path(target_id: @noteable.id, target_type: @noteable.class.name.underscore)}", #{@notes.map(&:id).to_json}, #{Time.now.to_i})

db/migrate/20140428105831_add_notes_index_updated_at.rb

+class AddNotesIndexUpdatedAt < ActiveRecord::Migration
+  def change
+    add_index :notes, :updated_at
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20140416185734) do
+ActiveRecord::Schema.define(version: 20140428105831) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -200,6 +200,7 @@ ActiveRecord::Schema.define(version: 20140416185734) do
   add_index "notes", ["noteable_type"], name: "index_notes_on_noteable_type", using: :btree
   add_index "notes", ["project_id", "noteable_type"], name: "index_notes_on_project_id_and_noteable_type", using: :btree
   add_index "notes", ["project_id"], name: "index_notes_on_project_id", using: :btree
+  add_index "notes", ["updated_at"], name: "index_notes_on_updated_at", using: :btree
 
   create_table "projects", force: true do |t|
     t.string   "name"

spec/finders/notes_finder_spec.rb

+require 'spec_helper'
+
+describe NotesFinder do
+  let(:user) { create :user }
+  let(:project) { create :project }
+  let(:note1) { create :note_on_commit, project: project }
+  let(:note2) { create :note_on_commit, project: project }
+  let(:commit) { note1.noteable }
+
+  before do
+    project.team << [user, :master]
+  end
+
+  describe :execute do
+    let(:params)  { { target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago.to_i } }
+
+    before do
+      note1
+      note2
+    end
+
+    it 'should find all notes' do
+      notes = NotesFinder.new.execute(project, user, params)
+      notes.size.should eq(2)
+    end
+
+    it 'should raise an exception for an invalid target_type' do
+      params.merge!(target_type: 'invalid')
+      expect { NotesFinder.new.execute(project, user, params) }.to raise_error('invalid target_type')
+    end
+
+    it 'filters out old notes' do
+      note2.update_attribute(:updated_at, 2.hours.ago)
+      notes = NotesFinder.new.execute(project, user, params)
+      notes.should eq([note1])
+    end
+  end
+end