diff --git a/app/assets/javascripts/notes.js.coffee b/app/assets/javascripts/notes.js.coffee index d200d962ca..043e4f6266 100644 --- a/app/assets/javascripts/notes.js.coffee +++ b/app/assets/javascripts/notes.js.coffee @@ -1,10 +1,11 @@ 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) diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb index 85d042a89b..b5b0446b43 100644 --- a/app/controllers/projects/notes_controller.rb +++ b/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] << { diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb index 384316e14b..ea055694cd 100644 --- a/app/finders/notes_finder.rb +++ b/app/finders/notes_finder.rb @@ -1,9 +1,13 @@ 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 diff --git a/app/views/projects/notes/_notes_with_form.html.haml b/app/views/projects/notes/_notes_with_form.html.haml index 3bd592e398..052661962e 100644 --- a/app/views/projects/notes/_notes_with_form.html.haml +++ b/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}) diff --git a/db/migrate/20140428105831_add_notes_index_updated_at.rb b/db/migrate/20140428105831_add_notes_index_updated_at.rb new file mode 100644 index 0000000000..6c25570f12 --- /dev/null +++ b/db/migrate/20140428105831_add_notes_index_updated_at.rb @@ -0,0 +1,5 @@ +class AddNotesIndexUpdatedAt < ActiveRecord::Migration + def change + add_index :notes, :updated_at + end +end diff --git a/db/schema.rb b/db/schema.rb index 17ca752db7..0a31eb7e3c 100644 --- a/db/schema.rb +++ b/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" diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb new file mode 100644 index 0000000000..4f8a5f909d --- /dev/null +++ b/spec/finders/notes_finder_spec.rb @@ -0,0 +1,38 @@ +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