From 87ec11e298e950203966a988fae4d1a9e197f9d7 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Fri, 17 Nov 2017 16:08:31 -0500 Subject: [PATCH] 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 | 6 +++- app/models/topic_user.rb | 11 ++++--- spec/components/topic_query_spec.rb | 20 +++++------ spec/models/post_mover_spec.rb | 2 +- spec/models/post_timing_spec.rb | 33 ++++++++++++++++++- spec/models/topic_user_spec.rb | 12 +++---- .../serializers/topic_view_serializer_spec.rb | 2 +- 7 files changed, 62 insertions(+), 24 deletions(-) diff --git a/app/models/post_timing.rb b/app/models/post_timing.rb index 0e29207dc4d..b32c80ccfcc 100644 --- a/app/models/post_timing.rb +++ b/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 diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb index a141de281df..f620c4648a4 100644 --- a/app/models/topic_user.rb +++ b/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 diff --git a/spec/components/topic_query_spec.rb b/spec/components/topic_query_spec.rb index ab682837745..d2a1c28ca32 100644 --- a/spec/components/topic_query_spec.rb +++ b/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(admin, 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, 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, fully_read.id, 1, 0) + TopicUser.update_last_read(user, partially_read.id, 0, 0, 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, fully_read.id, 1, 0) - TopicUser.update_last_read(user, fully_read_closed.id, 1, 0) - TopicUser.update_last_read(user, fully_read_archived.id, 1, 0) + TopicUser.update_last_read(user, partially_read.id, 0, 0, 0) + TopicUser.update_last_read(user, fully_read.id, 1, 1, 0) + TopicUser.update_last_read(user, fully_read_closed.id, 1, 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 diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index afc58a72b72..0fdcf26da5e 100644 --- a/spec/models/post_mover_spec.rb +++ b/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 diff --git a/spec/models/post_timing_spec.rb b/spec/models/post_timing_spec.rb index 889524911b6..7027c6b0902 100644 --- a/spec/models/post_timing_spec.rb +++ b/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 diff --git a/spec/models/topic_user_spec.rb b/spec/models/topic_user_spec.rb index e1d15e030f5..1c47b31a8b2 100644 --- a/spec/models/topic_user_spec.rb +++ b/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 diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb index 21b75d19e3f..d4449c39ceb 100644 --- a/spec/serializers/topic_view_serializer_spec.rb +++ b/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