From 225187733211efbcb6e51fb28a02999e1842a945 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 31 Oct 2014 09:40:35 +1100 Subject: [PATCH] FIX: "Dismiss Posts" corrupting read state REFACTOR: seen_post_count was a bad name, renamed to highest_seen_post_number --- app/models/topic.rb | 6 +++--- app/models/topic_user.rb | 14 +++++++------- .../20141030222425_rename_seen_post_count.rb | 5 +++++ lib/post_creator.rb | 2 +- lib/topics_bulk_action.rb | 2 +- lib/unread.rb | 6 +++--- spec/components/post_creator_spec.rb | 2 +- spec/components/post_destroyer_spec.rb | 2 +- spec/components/topics_bulk_action_spec.rb | 8 ++++++-- spec/components/unread_spec.rb | 14 +++++++------- spec/models/post_timing_spec.rb | 16 ++++++++-------- spec/models/topic_user_spec.rb | 5 +++-- 12 files changed, 46 insertions(+), 36 deletions(-) create mode 100644 db/migrate/20141030222425_rename_seen_post_count.rb diff --git a/app/models/topic.rb b/app/models/topic.rb index b80519aa008..b0ab6f24eb4 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -421,9 +421,9 @@ class Topic < ActiveRecord::Base WHEN last_read_post_number > :highest THEN :highest ELSE last_read_post_number END, - seen_post_count = CASE - WHEN seen_post_count > :highest THEN :highest - ELSE seen_post_count + highest_seen_post_number = CASE + WHEN highest_seen_post_number > :highest THEN :highest + ELSE highest_seen_post_number END WHERE topic_id = :topic_id", highest: highest_post_number, diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb index cc4497b2fa3..b252eca7307 100644 --- a/app/models/topic_user.rb +++ b/app/models/topic_user.rb @@ -152,8 +152,8 @@ class TopicUser < ActiveRecord::Base threshold: SiteSetting.auto_track_topics_after } - # In case anyone seens "seen_post_count" and gets confused, like I do. - # seen_post_count represents the highest_post_number of the topic when + # In case anyone seens "highest_seen_post_number" and gets confused, like I do. + # highest_seen_post_number represents the highest_post_number of the topic when # the user visited it. It may be out of alignment with last_read, meaning # ... user visited the topic but did not read the posts # @@ -161,7 +161,7 @@ class TopicUser < ActiveRecord::Base rows = exec_sql("UPDATE topic_users SET last_read_post_number = GREATEST(:post_number, tu.last_read_post_number), - seen_post_count = t.highest_post_number, + highest_seen_post_number = t.highest_post_number, total_msecs_viewed = LEAST(tu.total_msecs_viewed + :msecs,86400000), notification_level = case when tu.notifications_reason_id is null and (tu.total_msecs_viewed + :msecs) > @@ -210,7 +210,7 @@ class TopicUser < ActiveRecord::Base TopicTrackingState.publish_read(topic_id, post_number, user.id, args[:new_status]) user.update_posts_read!(post_number) - exec_sql("INSERT INTO topic_users (user_id, topic_id, last_read_post_number, seen_post_count, last_visited_at, first_visited_at, notification_level) + exec_sql("INSERT INTO topic_users (user_id, topic_id, last_read_post_number, highest_seen_post_number, last_visited_at, first_visited_at, notification_level) SELECT :user_id, :topic_id, :post_number, ft.highest_post_number, :now, :now, :new_status FROM topics AS ft JOIN users u on u.id = :user_id @@ -241,7 +241,7 @@ class TopicUser < ActiveRecord::Base UPDATE topic_users t SET last_read_post_number = LEAST(GREATEST(last_read, last_read_post_number), max_post_number), - seen_post_count = LEAST(max_post_number,GREATEST(t.seen_post_count, last_read)) + highest_seen_post_number = LEAST(max_post_number,GREATEST(t.highest_seen_post_number, last_read)) FROM ( SELECT topic_id, user_id, MAX(post_number) last_read FROM post_timings @@ -259,7 +259,7 @@ X.topic_id = t.topic_id AND X.user_id = t.user_id AND ( last_read_post_number <> LEAST(GREATEST(last_read, last_read_post_number), max_post_number) OR - seen_post_count <> LEAST(max_post_number,GREATEST(t.seen_post_count, last_read)) + highest_seen_post_number <> LEAST(max_post_number,GREATEST(t.highest_seen_post_number, last_read)) ) SQL @@ -281,7 +281,7 @@ end # starred :boolean default(FALSE), not null # posted :boolean default(FALSE), not null # last_read_post_number :integer -# seen_post_count :integer +# highest_seen_post_number :integer # starred_at :datetime # last_visited_at :datetime # first_visited_at :datetime diff --git a/db/migrate/20141030222425_rename_seen_post_count.rb b/db/migrate/20141030222425_rename_seen_post_count.rb new file mode 100644 index 00000000000..dac8d14467c --- /dev/null +++ b/db/migrate/20141030222425_rename_seen_post_count.rb @@ -0,0 +1,5 @@ +class RenameSeenPostCount < ActiveRecord::Migration + def change + rename_column :topic_users, :seen_post_count, :highest_seen_post_number + end +end diff --git a/lib/post_creator.rb b/lib/post_creator.rb index ae1c6fc178f..af0a1c3e7f8 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -298,7 +298,7 @@ class PostCreator @topic.id, posted: true, last_read_post_number: @post.post_number, - seen_post_count: @post.post_number) + highest_seen_post_number: @post.post_number) # assume it took us 5 seconds of reading time to make a post diff --git a/lib/topics_bulk_action.rb b/lib/topics_bulk_action.rb index fbdea220572..71defb0e3d7 100644 --- a/lib/topics_bulk_action.rb +++ b/lib/topics_bulk_action.rb @@ -22,7 +22,7 @@ class TopicsBulkAction def dismiss_posts sql = " UPDATE topic_users tu - SET seen_post_count = t.posts_count , last_read_post_number = highest_post_number + SET highest_seen_post_number = t.highest_post_number , last_read_post_number = highest_post_number FROM topics t WHERE t.id = tu.topic_id AND tu.user_id = :user_id AND t.id IN (:topic_ids) " diff --git a/lib/unread.rb b/lib/unread.rb index 04c20026026..a5a062d319a 100644 --- a/lib/unread.rb +++ b/lib/unread.rb @@ -10,17 +10,17 @@ class Unread def unread_posts return 0 if do_not_notify?(@topic_user.notification_level) - result = ((@topic_user.seen_post_count||0) - (@topic_user.last_read_post_number||0)) + result = ((@topic_user.highest_seen_post_number||0) - (@topic_user.last_read_post_number||0)) result = 0 if result < 0 result end def new_posts - return 0 if @topic_user.seen_post_count.blank? + return 0 if @topic_user.highest_seen_post_number.blank? return 0 if do_not_notify?(@topic_user.notification_level) return 0 if (@topic_user.last_read_post_number||0) > @topic.highest_post_number - new_posts = (@topic.highest_post_number - @topic_user.seen_post_count) + new_posts = (@topic.highest_post_number - @topic_user.highest_seen_post_number) new_posts = 0 if new_posts < 0 return new_posts end diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 6591e726e50..1cdcda9dd61 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -178,7 +178,7 @@ describe PostCreator do topic_user.should be_present topic_user.should be_posted topic_user.last_read_post_number.should == first_post.post_number - topic_user.seen_post_count.should == first_post.post_number + topic_user.highest_seen_post_number.should == first_post.post_number user2 = Fabricate(:coding_horror) user2.user_stat.topic_reply_count.should == 0 diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb index b0676533989..7785c2ef6c4 100644 --- a/spec/components/post_destroyer_spec.rb +++ b/spec/components/post_destroyer_spec.rb @@ -241,7 +241,7 @@ describe PostDestroyer do end it "sets the second user's last_read_post_number back to 1" do - topic_user.seen_post_count.should == 1 + topic_user.highest_seen_post_number.should == 1 end end diff --git a/spec/components/topics_bulk_action_spec.rb b/spec/components/topics_bulk_action_spec.rb index 325c6e402ff..533c876a27e 100644 --- a/spec/components/topics_bulk_action_spec.rb +++ b/spec/components/topics_bulk_action_spec.rb @@ -6,13 +6,17 @@ describe TopicsBulkAction do describe "dismiss_posts" do it "dismisses posts" do post1 = create_post + p = create_post(topic_id: post1.topic_id) create_post(topic_id: post1.topic_id) + PostDestroyer.new(Fabricate(:admin), p).destroy + TopicsBulkAction.new(post1.user, [post1.topic_id], type: 'dismiss_posts').perform! tu = TopicUser.find_by(user_id: post1.user_id, topic_id: post1.topic_id) - tu.last_read_post_number.should == 2 - tu.seen_post_count = 2 + + tu.last_read_post_number.should == 3 + tu.highest_seen_post_number.should == 3 end end diff --git a/spec/components/unread_spec.rb b/spec/components/unread_spec.rb index 45c11cc7f80..61a9b9bc4a2 100644 --- a/spec/components/unread_spec.rb +++ b/spec/components/unread_spec.rb @@ -16,19 +16,19 @@ describe Unread do describe 'unread_posts' do it 'should have 0 unread posts if the user has seen all posts' do @topic_user.stubs(:last_read_post_number).returns(13) - @topic_user.stubs(:seen_post_count).returns(13) + @topic_user.stubs(:highest_seen_post_number).returns(13) @unread.unread_posts.should == 0 end it 'should have 6 unread posts if the user has seen all but 6 posts' do @topic_user.stubs(:last_read_post_number).returns(5) - @topic_user.stubs(:seen_post_count).returns(11) + @topic_user.stubs(:highest_seen_post_number).returns(11) @unread.unread_posts.should == 6 end it 'should have 0 unread posts if the user has seen more posts than exist (deleted)' do @topic_user.stubs(:last_read_post_number).returns(100) - @topic_user.stubs(:seen_post_count).returns(13) + @topic_user.stubs(:highest_seen_post_number).returns(13) @unread.unread_posts.should == 0 end end @@ -40,23 +40,23 @@ describe Unread do end it 'returns 0 when the topic is the same length as when you last saw it' do - @topic_user.stubs(:seen_post_count).returns(13) + @topic_user.stubs(:highest_seen_post_number).returns(13) @unread.new_posts.should == 0 end it 'has 3 new posts if the user has read 10 posts' do - @topic_user.stubs(:seen_post_count).returns(10) + @topic_user.stubs(:highest_seen_post_number).returns(10) @unread.new_posts.should == 3 end it 'has 0 new posts if the user has read 10 posts but is not tracking' do - @topic_user.stubs(:seen_post_count).returns(10) + @topic_user.stubs(:highest_seen_post_number).returns(10) @topic_user.stubs(:notification_level).returns(TopicUser.notification_levels[:regular]) @unread.new_posts.should == 0 end it 'has 0 new posts if the user read more posts than exist (deleted)' do - @topic_user.stubs(:seen_post_count).returns(16) + @topic_user.stubs(:highest_seen_post_number).returns(16) @unread.new_posts.should == 0 end diff --git a/spec/models/post_timing_spec.rb b/spec/models/post_timing_spec.rb index 3cdedc9ca3e..c3694386d2b 100644 --- a/spec/models/post_timing_spec.rb +++ b/spec/models/post_timing_spec.rb @@ -18,12 +18,12 @@ describe PostTiming do PostTiming.create!(topic_id: topic_id, user_id: user_id, post_number: post_number, msecs: 0) end - def topic_user(user_id, last_read_post_number, seen_post_count) + def topic_user(user_id, last_read_post_number, highest_seen_post_number) TopicUser.create!( topic_id: topic_id, user_id: user_id, last_read_post_number: last_read_post_number, - seen_post_count: seen_post_count + highest_seen_post_number: highest_seen_post_number ) end @@ -35,9 +35,9 @@ describe PostTiming do timing(3,2) timing(3,3) - tu_one = topic_user(1,1,1) - tu_two = topic_user(2,2,2) - tu_three = topic_user(3,3,3) + _tu_one = topic_user(1,1,1) + _tu_two = topic_user(2,2,2) + _tu_three = topic_user(3,3,3) PostTiming.pretend_read(topic_id, 2, 3) @@ -47,15 +47,15 @@ describe PostTiming do tu = TopicUser.find_by(topic_id: topic_id, user_id: 1) tu.last_read_post_number.should == 1 - tu.seen_post_count.should == 1 + tu.highest_seen_post_number.should == 1 tu = TopicUser.find_by(topic_id: topic_id, user_id: 2) tu.last_read_post_number.should == 3 - tu.seen_post_count.should == 3 + tu.highest_seen_post_number.should == 3 tu = TopicUser.find_by(topic_id: topic_id, user_id: 3) tu.last_read_post_number.should == 3 - tu.seen_post_count.should == 3 + tu.highest_seen_post_number.should == 3 end end diff --git a/spec/models/topic_user_spec.rb b/spec/models/topic_user_spec.rb index 382f8e3ef6c..c1a62ac4d5a 100644 --- a/spec/models/topic_user_spec.rb +++ b/spec/models/topic_user_spec.rb @@ -254,7 +254,7 @@ describe TopicUser do p2 = Fabricate(:post, user: p1.user, topic: p1.topic, post_number: 2) p1.topic.notifier.watch_topic!(p1.user_id) - TopicUser.exec_sql("UPDATE topic_users set seen_post_count=100, last_read_post_number=0 + TopicUser.exec_sql("UPDATE topic_users set highest_seen_post_number=1, last_read_post_number=0 WHERE topic_id = :topic_id AND user_id = :user_id", topic_id: p1.topic_id, user_id: p1.user_id) [p1,p2].each do |p| @@ -265,7 +265,8 @@ describe TopicUser do tu = TopicUser.find_by(user_id: p1.user_id, topic_id: p1.topic_id) tu.last_read_post_number.should == p2.post_number - tu.seen_post_count.should == 2 + tu.highest_seen_post_number.should == 2 + end describe "mailing_list_mode" do