fixed a pile of notification craziness

addes some tests around post timings

app/controllers/topics_controller.rb

@@ -129,49 +129,14 @@ class TopicsController < ApplicationController
   end
 
   def timings
-    # TODO: all this should be optimised, tested better
     
-    last_seen_key = "user-last-seen:#{current_user.id}"
-    last_seen = $redis.get(last_seen_key)
-    if last_seen.present?
-      diff = (Time.now.to_f - last_seen.to_f).round
-      if diff > 0
-        User.update_all ["time_read = time_read + ?", diff], ["id = ? and time_read = ?", current_user.id, current_user.time_read]
-      end
-    end
-    $redis.set(last_seen_key, Time.now.to_f)
-
-    original_unread = current_user.unread_notifications_by_type
-
-    topic_id = params["topic_id"].to_i
-    highest_seen = params["highest_seen"].to_i
-    added_time = 0
-
-
-    if params[:timings].present?
-      params[:timings].each do |post_number_str, t|
-        post_number = post_number_str.to_i
-
-        if post_number >= 0
-          if (highest_seen || 0) >= post_number
-            Notification.mark_post_read(current_user, topic_id, post_number)
-          end
-
-          PostTiming.record_timing(topic_id: topic_id,
-                                   post_number: post_number,
-                                   user_id: current_user.id,
-                                   msecs: t.to_i)
-        end
-      end
-    end
-
-    TopicUser.update_last_read(current_user, topic_id, highest_seen, params[:topic_time].to_i)
-
-    current_user.reload
-
-    if current_user.unread_notifications_by_type != original_unread
-      current_user.publish_notifications_state
-    end
+    PostTiming.process_timings(
+        current_user, 
+        params[:topic_id].to_i, 
+        params[:highest_seen].to_i, 
+        params[:topic_time].to_i, 
+        (params[:timings] || []).map{|post_number, t| [post_number.to_i, t.to_i]}
+    )
 
     render nothing: true
   end

app/models/notification.rb

@@ -27,8 +27,8 @@ class Notification < ActiveRecord::Base
     where(read: false)
   end
 
-  def self.mark_post_read(user, topic_id, post_number)
-    Notification.update_all "read = true", ["user_id = ? and topic_id = ? and post_number = ?", user.id, topic_id, post_number]
+  def self.mark_posts_read(user, topic_id, post_numbers)
+    Notification.update_all "read = 't'", ["user_id = ? and topic_id = ? and post_number in (?) and read = ?", user.id, topic_id, post_numbers, false]
   end
 
   def self.recent

app/models/post_timing.rb

@@ -39,4 +39,32 @@ class PostTiming < ActiveRecord::Base
     end
   end
 
+
+  def self.process_timings(current_user, topic_id, highest_seen, topic_time, timings)
+    current_user.update_time_read!
+   
+    original_unread = current_user.unread_notifications_by_type
+    timings.each do |post_number, time|
+      if post_number >= 0
+        PostTiming.record_timing(topic_id: topic_id,
+                                 post_number: post_number,
+                                 user_id: current_user.id,
+                                 msecs: time)
+      end
+    end
+
+    total_changed = 0
+    if timings.length > 0
+      total_changed = Notification.mark_posts_read(current_user, topic_id, timings.map{|t| t[0]})
+    end
+    
+    TopicUser.update_last_read(current_user, topic_id, highest_seen, topic_time)
+
+    if total_changed > 0
+      current_user.reload
+      current_user.publish_notifications_state
+    end
+
+  end
+
 end

app/models/user.rb

@@ -226,7 +226,7 @@ class User < ActiveRecord::Base
   end
 
   def publish_notifications_state
-    MessageBus.publish("/notification",
+    MessageBus.publish("/notification/#{self.id}",
         {unread_notifications: self.unread_notifications,
          unread_private_messages: self.unread_private_messages},
         user_ids: [self.id] # only publish the notification to this user
@@ -444,6 +444,20 @@ class User < ActiveRecord::Base
     end 
   end
 
+  MAX_TIME_READ_DIFF = 100
+  # attempt to add total read time to user based on previous time this was called
+  def update_time_read! 
+    last_seen_key = "user-last-seen:#{id}"
+    last_seen = $redis.get(last_seen_key)
+    if last_seen.present?
+      diff = (Time.now.to_f - last_seen.to_f).round
+      if diff > 0 && diff < MAX_TIME_READ_DIFF
+        User.update_all ["time_read = time_read + ?", diff], ["id = ? and time_read = ?", id, time_read]
+      end
+    end
+    $redis.set(last_seen_key, Time.now.to_f)
+  end
+
   protected
 
     def cook

spec/models/notification_spec.rb

@@ -108,4 +108,17 @@ describe Notification do
     end
   end
 
+  describe 'mark_posts_read' do 
+    it "marks multiple posts as read if needed" do 
+      user = Fabricate(:user)
+
+      notifications = (1..3).map do |i| 
+        Notification.create!(read: false, user_id: user.id, topic_id: 2, post_number: i, data: '[]', notification_type: 1)
+      end
+      Notification.create!(read: true, user_id: user.id, topic_id: 2, post_number: 4, data: '[]', notification_type: 1)
+
+      Notification.mark_posts_read(user,2,[1,2,3,4]).should == 3
+    end
+  end
+
 end

spec/models/post_timing_spec.rb

@@ -8,6 +8,28 @@ describe PostTiming do
   it { should validate_presence_of :post_number }
   it { should validate_presence_of :msecs }
 
+  describe 'process_timings' do 
+    
+    # integration test
+
+    it 'processes timings correctly' do 
+      post = Fabricate(:post)
+      user2 = Fabricate(:coding_horror)
+
+      PostAction.act(user2, post, PostActionType.Types[:like])
+      post.user.unread_notifications.should == 1
+      
+      post.user.unread_notifications_by_type.should == {Notification.Types[:liked] => 1}
+      
+      PostTiming.process_timings(post.user, post.topic_id, 1, 100, [[post.post_number, 100]])
+
+      post.user.reload
+      post.user.unread_notifications_by_type.should == {}
+      post.user.unread_notifications.should == 0
+      
+    end
+  end
+
   describe 'recording' do
     before do
       @post = Fabricate(:post)

spec/models/user_spec.rb

@@ -652,9 +652,6 @@ describe User do
       end
 
     end
-
-
-
   end
 
   describe '#create_for_email' do
@@ -691,4 +688,32 @@ describe User do
     end
   end
 
+
+  describe 'update_time_read!' do 
+    let(:user) { Fabricate(:user) }
+
+    it 'makes no changes if nothing is cached' do 
+      $redis.expects(:get).with("user-last-seen:#{user.id}").returns(nil)
+      user.update_time_read!
+      user.reload
+      user.time_read.should == 0
+    end
+
+    it 'makes a change if time read is below threshold' do 
+      $redis.expects(:get).with("user-last-seen:#{user.id}").returns(Time.now - 10.0)
+      user.update_time_read!
+      user.reload
+      user.time_read.should == 10
+    end
+
+    it 'makes no change if time read is above threshold' do 
+      t = Time.now - 1 - User::MAX_TIME_READ_DIFF
+      $redis.expects(:get).with("user-last-seen:#{user.id}").returns(t)
+      user.update_time_read!
+      user.reload
+      user.time_read.should == 0
+    end
+
+  end
+
 end