FIX: more accurate counting of posts read. Skipping to the end of a topic does not count all posts as read in user stats.

app/models/post_timing.rb

@@ -1,3 +1,5 @@
+require_dependency 'archetype'
+
 class PostTiming < ActiveRecord::Base
   belongs_to :topic
   belongs_to :user
@@ -80,6 +82,7 @@ class PostTiming < ActiveRecord::Base
     max_time_per_post = MAX_READ_TIME_PER_BATCH if max_time_per_post > MAX_READ_TIME_PER_BATCH
 
     highest_seen = 1
+    new_posts_read = 0
 
     join_table = []
 
@@ -114,6 +117,7 @@ SQL
       result = exec_sql(sql)
       result.type_map = SqlBuilder.pg_type_map
       existing = Set.new(result.column_values(0))
+      new_posts_read = timings.size - existing.size if Topic.where(id: topic_id, archetype: Archetype.default).exists?
 
       timings.each_with_index do |(post_number, time), index|
         unless existing.include?(index)
@@ -132,7 +136,7 @@ SQL
 
     topic_time = max_time_per_post if topic_time > max_time_per_post
 
-    TopicUser.update_last_read(current_user, topic_id, highest_seen, topic_time, opts)
+    TopicUser.update_last_read(current_user, topic_id, highest_seen, new_posts_read, topic_time, opts)
 
     if total_changed > 0
       current_user.reload

app/models/topic_user.rb

@@ -248,7 +248,7 @@ SQL
 
     INSERT_TOPIC_USER_SQL_STAFF = INSERT_TOPIC_USER_SQL.gsub("highest_post_number", "highest_staff_post_number")
 
-    def update_last_read(user, topic_id, post_number, msecs, opts = {})
+    def update_last_read(user, topic_id, post_number, new_posts_read, msecs, opts = {})
       return if post_number.blank?
       msecs = 0 if msecs.to_i < 0
 
@@ -284,7 +284,10 @@ SQL
         if before_last_read < post_number
           # The user read at least one new post
           TopicTrackingState.publish_read(topic_id, post_number, user.id, after)
-          user.update_posts_read!(post_number - before_last_read, mobile: opts[:mobile])
+        end
+
+        if new_posts_read > 0
+          user.update_posts_read!(new_posts_read, mobile: opts[:mobile])
         end
 
         if before != after
@@ -300,7 +303,7 @@ SQL
         end
         TopicTrackingState.publish_read(topic_id, post_number, user.id, args[:new_status])
 
-        user.update_posts_read!(post_number, mobile: opts[:mobile])
+        user.update_posts_read!(new_posts_read, mobile: opts[:mobile])
 
         begin
           if user.staff?
@@ -315,7 +318,7 @@ SQL
             raise
           else
             opts[:retry] = true
-            update_last_read(user, topic_id, post_number, msecs, opts)
+            update_last_read(user, topic_id, post_number, new_posts_read, msecs, opts)
           end
         end
 

spec/components/topic_query_spec.rb

@@ -465,8 +465,8 @@ describe TopicQuery do
 
         topic_id = first.topic.id
 
-        TopicUser.update_last_read(user, topic_id, first.post_number, 1)
+        TopicUser.update_last_read(user, topic_id, first.post_number, 1, 1)
-        TopicUser.update_last_read(admin, topic_id, first.post_number, 1)
+        TopicUser.update_last_read(admin, topic_id, first.post_number, 1, 1)
 
         TopicUser.change(user.id, topic_id, notification_level: TopicUser.notification_levels[:tracking])
         TopicUser.change(admin.id, topic_id, notification_level: TopicUser.notification_levels[:tracking])
@@ -481,8 +481,8 @@ describe TopicQuery do
       let!(:fully_read) { Fabricate(:post, user: creator).topic }
 
       before do
-        TopicUser.update_last_read(user, partially_read.id, 0, 0)
+        TopicUser.update_last_read(user, partially_read.id, 0, 0, 0)
-        TopicUser.update_last_read(user, fully_read.id, 1, 0)
+        TopicUser.update_last_read(user, fully_read.id, 1, 1, 0)
       end
 
       context 'list_unread' do
@@ -633,7 +633,7 @@ describe TopicQuery do
       context "but interacted with" do
 
         it "is not included if read" do
-          TopicUser.update_last_read(user, other_users_topic.id, 0, 0)
+          TopicUser.update_last_read(user, other_users_topic.id, 0, 0, 0)
 
           expect(topics).to be_blank
         end
@@ -680,7 +680,7 @@ describe TopicQuery do
     end
 
     def read(user, topic, post_number)
-      TopicUser.update_last_read(user, topic, post_number, 10000)
+      TopicUser.update_last_read(user, topic, post_number, post_number, 10000)
     end
 
     it 'returns the correct suggestions' do
@@ -848,10 +848,10 @@ describe TopicQuery do
         before do
           user.user_option.auto_track_topics_after_msecs = 0
           user.user_option.save
-          TopicUser.update_last_read(user, partially_read.id, 0, 0)
+          TopicUser.update_last_read(user, partially_read.id, 0, 0, 0)
-          TopicUser.update_last_read(user, fully_read.id, 1, 0)
+          TopicUser.update_last_read(user, fully_read.id, 1, 1, 0)
-          TopicUser.update_last_read(user, fully_read_closed.id, 1, 0)
+          TopicUser.update_last_read(user, fully_read_closed.id, 1, 1, 0)
-          TopicUser.update_last_read(user, fully_read_archived.id, 1, 0)
+          TopicUser.update_last_read(user, fully_read_archived.id, 1, 1, 0)
           fully_read_closed.closed = true
           fully_read_closed.save
           fully_read_archived.archived = true

spec/models/post_mover_spec.rb

@@ -102,7 +102,7 @@ describe PostMover do
 
     context "successfully moved" do
       before do
-        TopicUser.update_last_read(user, topic.id, p4.post_number, 0)
+        TopicUser.update_last_read(user, topic.id, p4.post_number, p4.post_number, 0)
         TopicLink.extract_from(p2)
       end
 

spec/models/post_timing_spec.rb

@@ -78,12 +78,15 @@ describe PostTiming do
 
   describe 'process_timings' do
 
-    # integration test
+    # integration tests
 
     it 'processes timings correctly' do
       PostActionNotifier.enable
 
       post = Fabricate(:post)
+      (2..5).each do |i|
+        Fabricate(:post, topic: post.topic, post_number: i)
+      end
       user2 = Fabricate(:coding_horror, created_at: 1.day.ago)
 
       PostAction.act(user2, post, PostActionType.types[:like])
@@ -97,6 +100,34 @@ describe PostTiming do
 
       PostTiming.process_timings(post.user, post.topic_id, 1, [[post.post_number, 1.day]])
 
+      user_visit = post.user.user_visits.order('id DESC').first
+      expect(user_visit.posts_read).to eq(1)
+
+      # Skip to bottom
+      PostTiming.process_timings(post.user, post.topic_id, 1, [[5, 100]])
+      expect(user_visit.reload.posts_read).to eq(2)
+
+      # Scroll up
+      PostTiming.process_timings(post.user, post.topic_id, 1, [[4, 100]])
+      expect(user_visit.reload.posts_read).to eq(3)
+      PostTiming.process_timings(post.user, post.topic_id, 1, [[2, 100], [3, 100]])
+      expect(user_visit.reload.posts_read).to eq(5)
+    end
+
+    it 'does not count private message posts read' do
+      pm = Fabricate(:private_message_topic, user: Fabricate(:admin))
+      user1, user2 = pm.topic_allowed_users.map(&:user)
+
+      (1..3).each do |i|
+        Fabricate(:post, topic: pm, user: user1)
+      end
+
+      PostTiming.process_timings(user2, pm.id, 10, [[1, 100]])
+      user_visit = user2.user_visits.last
+      expect(user_visit.posts_read).to eq(0)
+
+      PostTiming.process_timings(user2, pm.id, 10, [[2, 100], [3, 100]])
+      expect(user_visit.reload.posts_read).to eq(0)
     end
   end
 

spec/models/topic_user_spec.rb

@@ -206,7 +206,7 @@ describe TopicUser do
       it 'should create a new record for a visit' do
         freeze_time yesterday
 
-        TopicUser.update_last_read(user, topic.id, 1, 0)
+        TopicUser.update_last_read(user, topic.id, 1, 1, 0)
 
         expect(topic_user.last_read_post_number).to eq(1)
         expect(topic_user.last_visited_at.to_i).to eq(yesterday.to_i)
@@ -218,13 +218,13 @@ describe TopicUser do
         today = Time.zone.now
         freeze_time Time.zone.now
 
-        TopicUser.update_last_read(user, topic.id, 1, 0)
+        TopicUser.update_last_read(user, topic.id, 1, 1, 0)
 
         tomorrow = 1.day.from_now
         freeze_time tomorrow
 
         Fabricate(:post, topic: topic, user: user)
-        TopicUser.update_last_read(user, topic.id, 2, 0)
+        TopicUser.update_last_read(user, topic.id, 2, 1, 0)
         topic_user = TopicUser.get(topic, user)
 
         expect(topic_user.last_read_post_number).to eq(2)
@@ -249,7 +249,7 @@ describe TopicUser do
       let(:post_creator) { PostCreator.new(new_user, raw: Fabricate.build(:post).raw, topic_id: topic.id) }
 
       before do
-        TopicUser.update_last_read(new_user, topic.id, 2, 0)
+        TopicUser.update_last_read(new_user, topic.id, 2, 2, 0)
       end
 
       it 'should automatically track topics you reply to' do
@@ -311,13 +311,13 @@ describe TopicUser do
 
       it 'should automatically track topics after they are read for long enough' do
         expect(topic_new_user.notification_level).to eq(TopicUser.notification_levels[:regular])
-        TopicUser.update_last_read(new_user, topic.id, 2, SiteSetting.default_other_auto_track_topics_after_msecs + 1)
+        TopicUser.update_last_read(new_user, topic.id, 2, 2, SiteSetting.default_other_auto_track_topics_after_msecs + 1)
         expect(TopicUser.get(topic, new_user).notification_level).to eq(TopicUser.notification_levels[:tracking])
       end
 
       it 'should not automatically track topics after they are read for long enough if changed manually' do
         TopicUser.change(new_user, topic, notification_level: TopicUser.notification_levels[:regular])
-        TopicUser.update_last_read(new_user, topic, 2, SiteSetting.default_other_auto_track_topics_after_msecs + 1)
+        TopicUser.update_last_read(new_user, topic, 2, 2, SiteSetting.default_other_auto_track_topics_after_msecs + 1)
         expect(topic_new_user.notification_level).to eq(TopicUser.notification_levels[:regular])
       end
     end

spec/serializers/topic_view_serializer_spec.rb

@@ -8,7 +8,7 @@ describe TopicViewSerializer do
     let(:topic2) { Fabricate(:topic) }
 
     before do
-      TopicUser.update_last_read(user, topic2.id, 0, 0)
+      TopicUser.update_last_read(user, topic2.id, 0, 0, 0)
     end
 
     describe 'when loading last chunk' do