From 410790a355a6389bfa372e0834c31541b8ac8e4f Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 28 Apr 2014 11:48:18 +0200
Subject: [PATCH 1/9] Add NotesFinder spec

---
 spec/finders/notes_finder_spec.rb | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 spec/finders/notes_finder_spec.rb

diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb
new file mode 100644
index 0000000000..f0588f56b4
--- /dev/null
+++ b/spec/finders/notes_finder_spec.rb
@@ -0,0 +1,26 @@
+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.commit }
+
+  before do
+    project.team << [user, :master]
+  end
+
+  describe :execute do
+    before do
+      note1
+      note2
+    end
+
+    it 'should find all notes' do
+      params = { target_id: commit.id, target_type: 'commit' }
+      notes = NotesFinder.new.execute(project, user, params)
+      notes.size.should eq(2)
+    end
+  end
+end

From e5cf5f4f98464543ec2f06415e071b8110368cc7 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 28 Apr 2014 11:50:05 +0200
Subject: [PATCH 2/9] Notes have noteables but no commits

---
 spec/finders/notes_finder_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb
index f0588f56b4..3b28070ffa 100644
--- a/spec/finders/notes_finder_spec.rb
+++ b/spec/finders/notes_finder_spec.rb
@@ -5,7 +5,7 @@ describe NotesFinder do
   let(:project) { create :project }
   let(:note1) { create :note_on_commit, project: project }
   let(:note2) { create :note_on_commit, project: project }
-  let(:commit) { note1.commit }
+  let(:commit) { note1.noteable }
 
   before do
     project.team << [user, :master]

From 7339464e7701c0778cca12c12ace83ebd8ffe2f7 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 28 Apr 2014 11:53:37 +0200
Subject: [PATCH 3/9] Fail faster on an invalid target_type

---
 app/finders/notes_finder.rb       | 2 ++
 spec/finders/notes_finder_spec.rb | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index 384316e14b..4e80bd8175 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -12,6 +12,8 @@ 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
   end
 end
diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb
index 3b28070ffa..27eaba8dfa 100644
--- a/spec/finders/notes_finder_spec.rb
+++ b/spec/finders/notes_finder_spec.rb
@@ -22,5 +22,10 @@ describe NotesFinder 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 = { target_id: commit.id, target_type: 'invalid' }
+      expect { NotesFinder.new.execute(project, user, params) }.to raise_error('invalid target_type')
+    end
   end
 end

From 0b615eb0e2b5cca7685360c0cae72484741d672e Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 28 Apr 2014 12:13:29 +0200
Subject: [PATCH 4/9] Filter out old notes in NotesFinder

---
 app/finders/notes_finder.rb       | 8 +++++++-
 spec/finders/notes_finder_spec.rb | 7 +++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index 4e80bd8175..38d78f3a2d 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -1,9 +1,12 @@
 class NotesFinder
+  FETCH_OVERLAP = 5.seconds
+
   def execute(project, current_user, params)
     target_type = params[:target_type]
     target_id   = params[:target_id]
+    last_fetched_at = params.fetch(:last_fetched_at)
 
-    case target_type
+    notes = case target_type
     when "commit"
       project.notes.for_commit_id(target_id).not_inline.fresh
     when "issue"
@@ -15,5 +18,8 @@ class NotesFinder
     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/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb
index 27eaba8dfa..ffd3f5db81 100644
--- a/spec/finders/notes_finder_spec.rb
+++ b/spec/finders/notes_finder_spec.rb
@@ -27,5 +27,12 @@ describe NotesFinder do
       params = { target_id: commit.id, 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)
+      params = { target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago }
+      notes = NotesFinder.new.execute(project, user, params)
+      notes.should eq([note1])
+    end
   end
 end

From bbfa4a771ab8ca4745419b8d660af89249e088f4 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 28 Apr 2014 12:16:34 +0200
Subject: [PATCH 5/9] Always set last_fetched_at in NotesFinder spec

---
 spec/finders/notes_finder_spec.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb
index ffd3f5db81..80d6a36c31 100644
--- a/spec/finders/notes_finder_spec.rb
+++ b/spec/finders/notes_finder_spec.rb
@@ -12,25 +12,25 @@ describe NotesFinder do
   end
 
   describe :execute do
+    let(:params)  { { target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago } }
+
     before do
       note1
       note2
     end
 
     it 'should find all notes' do
-      params = { target_id: commit.id, target_type: 'commit' }
       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 = { target_id: commit.id, target_type: 'invalid' }
+      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)
-      params = { target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago }
       notes = NotesFinder.new.execute(project, user, params)
       notes.should eq([note1])
     end

From 7ec5ff4dbae71d147f413da5cea64116e7eb305d Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 28 Apr 2014 12:21:49 +0200
Subject: [PATCH 6/9] Pass last_fetched_at for notes to javascript

---
 app/assets/javascripts/notes.js.coffee              | 5 ++++-
 app/controllers/projects/notes_controller.rb        | 3 ++-
 app/views/projects/notes/_notes_with_form.html.haml | 2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

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..3826515d22 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
     @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/views/projects/notes/_notes_with_form.html.haml b/app/views/projects/notes/_notes_with_form.html.haml
index 3bd592e398..bdcecd8a39 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)

From 285926918b95f1d771bf4e9c84972d4e55b41ea9 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 28 Apr 2014 12:42:01 +0200
Subject: [PATCH 7/9] Serialize last_fetched_at as a string with seconds

---
 app/controllers/projects/notes_controller.rb        | 2 +-
 app/finders/notes_finder.rb                         | 2 +-
 app/views/projects/notes/_notes_with_form.html.haml | 2 +-
 spec/finders/notes_finder_spec.rb                   | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/controllers/projects/notes_controller.rb b/app/controllers/projects/notes_controller.rb
index 3826515d22..b5b0446b43 100644
--- a/app/controllers/projects/notes_controller.rb
+++ b/app/controllers/projects/notes_controller.rb
@@ -5,7 +5,7 @@ class Projects::NotesController < Projects::ApplicationController
   before_filter :authorize_admin_note!, only: [:update, :destroy]
 
   def index
-    current_fetched_at = Time.now
+    current_fetched_at = Time.now.to_i
     @notes = NotesFinder.new.execute(project, current_user, params)
 
     notes_json = { notes: [], last_fetched_at: current_fetched_at }
diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index 38d78f3a2d..0b9affb716 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -4,7 +4,7 @@ class NotesFinder
   def execute(project, current_user, params)
     target_type = params[:target_type]
     target_id   = params[:target_id]
-    last_fetched_at = params.fetch(:last_fetched_at)
+    last_fetched_at = Time.at(params.fetch(:last_fetched_at).to_i)
 
     notes = case target_type
     when "commit"
diff --git a/app/views/projects/notes/_notes_with_form.html.haml b/app/views/projects/notes/_notes_with_form.html.haml
index bdcecd8a39..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}, Time.now)
+  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/spec/finders/notes_finder_spec.rb b/spec/finders/notes_finder_spec.rb
index 80d6a36c31..4f8a5f909d 100644
--- a/spec/finders/notes_finder_spec.rb
+++ b/spec/finders/notes_finder_spec.rb
@@ -12,7 +12,7 @@ describe NotesFinder do
   end
 
   describe :execute do
-    let(:params)  { { target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago } }
+    let(:params)  { { target_id: commit.id, target_type: 'commit', last_fetched_at: 1.hour.ago.to_i } }
 
     before do
       note1

From c685024951b8fed73ab05039baa170859ddf7b12 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 28 Apr 2014 13:02:05 +0200
Subject: [PATCH 8/9] Add an index for Note#updated_at

---
 db/migrate/20140428105831_add_notes_index_updated_at.rb | 5 +++++
 db/schema.rb                                            | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)
 create mode 100644 db/migrate/20140428105831_add_notes_index_updated_at.rb

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"

From bd8b2b7fd98faf3308cb1f722426ff8f7c39f1d5 Mon Sep 17 00:00:00 2001
From: Jacob Vosmaer <contact@jacobvosmaer.nl>
Date: Mon, 28 Apr 2014 16:37:19 +0200
Subject: [PATCH 9/9] Default last_fetched_at to 0 for old clients

Users who have not refreshed their browser tab will poll GitLab using
outdated JS. This change makes the server fall back to the old behavior
(send all comments) for old clients, instead of throwing an exception
for old clients.
---
 app/finders/notes_finder.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index 0b9affb716..ea055694cd 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -4,7 +4,8 @@ class NotesFinder
   def execute(project, current_user, params)
     target_type = params[:target_type]
     target_id   = params[:target_id]
-    last_fetched_at = Time.at(params.fetch(:last_fetched_at).to_i)
+    # Default to 0 to remain compatible with old clients
+    last_fetched_at = Time.at(params.fetch(:last_fetched_at, 0).to_i)
 
     notes = case target_type
     when "commit"