diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index ca9eeb6c5e2..a432f77c96c 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 873c185babb..a3920f015c2 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 8026b5ebe24..4ae992a505b 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 4e8182522e7..1fab84e9972 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 8d362bb20f2..4070cc7ff22 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 afdd45e6fa0..1918205582f 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 1771f9b2efb..c3faed2b1e6 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