From 77a2d8ccc4367b33d6e1bfac7f5530379e3ea6d2 Mon Sep 17 00:00:00 2001
From: Sam Saffron <sam.saffron@gmail.com>
Date: Mon, 25 Feb 2013 18:42:42 +1100
Subject: [PATCH] fixed a pile of notification craziness addes some tests
around post timings
---
app/controllers/topics_controller.rb | 51 +++++-----------------------
app/models/notification.rb | 4 +--
app/models/post_timing.rb | 28 +++++++++++++++
app/models/user.rb | 16 ++++++++-
spec/models/notification_spec.rb | 13 +++++++
spec/models/post_timing_spec.rb | 22 ++++++++++++
spec/models/user_spec.rb | 31 +++++++++++++++--
7 files changed, 116 insertions(+), 49 deletions(-)
diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index ca9eeb6c5..a432f77c9 100644
--- a/app/controllers/topics_controller.rb
+++ b/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
diff --git a/app/models/notification.rb b/app/models/notification.rb
index 873c185ba..a3920f015 100644
--- a/app/models/notification.rb
+++ b/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
diff --git a/app/models/post_timing.rb b/app/models/post_timing.rb
index 8026b5ebe..4ae992a50 100644
--- a/app/models/post_timing.rb
+++ b/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
diff --git a/app/models/user.rb b/app/models/user.rb
index 4e8182522..1fab84e99 100644
--- a/app/models/user.rb
+++ b/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
diff --git a/spec/models/notification_spec.rb b/spec/models/notification_spec.rb
index 8d362bb20..4070cc7ff 100644
--- a/spec/models/notification_spec.rb
+++ b/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
diff --git a/spec/models/post_timing_spec.rb b/spec/models/post_timing_spec.rb
index afdd45e6f..191820558 100644
--- a/spec/models/post_timing_spec.rb
+++ b/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)
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 1771f9b2e..c3faed2b1 100644
--- a/spec/models/user_spec.rb
+++ b/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