From b662cb6c02f645740759f6480eed8fa81b4bb5c4 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 4 Jul 2013 11:47:12 +1000 Subject: [PATCH] If a user read to the end of an auto closing topic, when it is closes just pretend they read the close message. --- app/models/post_timing.rb | 23 +++++++++++ app/models/topic_status_update.rb | 16 ++++++- app/models/topic_user.rb | 26 ++++++++---- spec/models/post_timing_spec.rb | 55 +++++++++++++++++++++++++ spec/models/topic_status_update_spec.rb | 29 +++++++++++++ 5 files changed, 139 insertions(+), 10 deletions(-) create mode 100644 spec/models/topic_status_update_spec.rb diff --git a/app/models/post_timing.rb b/app/models/post_timing.rb index aad81a054..62a473e23 100644 --- a/app/models/post_timing.rb +++ b/app/models/post_timing.rb @@ -6,6 +6,29 @@ class PostTiming < ActiveRecord::Base validates_presence_of :msecs + def self.pretend_read(topic_id, actual_read_post_number, pretend_read_post_number) + # This is done in SQL cause the logic is quite tricky and we want to do this in one db hit + # + exec_sql("INSERT INTO post_timings(topic_id, user_id, post_number, msecs) + SELECT :topic_id, user_id, :pretend_read_post_number, 1 + FROM post_timings pt + WHERE topic_id = :topic_id AND + post_number = :actual_read_post_number AND + NOT EXISTS ( + SELECT 1 FROM post_timings pt1 + WHERE pt1.topic_id = pt.topic_id AND + pt1.post_number = :pretend_read_post_number AND + pt1.user_id = pt.user_id + ) + ", + pretend_read_post_number: pretend_read_post_number, + topic_id: topic_id, + actual_read_post_number: actual_read_post_number + ) + + TopicUser.ensure_consistency!(topic_id) + end + # Increases a timer if a row exists, otherwise create it def self.record_timing(args) rows = exec_sql_row_count("UPDATE post_timings diff --git a/app/models/topic_status_update.rb b/app/models/topic_status_update.rb index cb890cfbf..a9769df3c 100644 --- a/app/models/topic_status_update.rb +++ b/app/models/topic_status_update.rb @@ -3,8 +3,11 @@ TopicStatusUpdate = Struct.new(:topic, :user) do status = Status.new(status, enabled) Topic.transaction do - change status - create_moderator_post_for status + change(status) + highest_post_number = topic.highest_post_number + + create_moderator_post_for(status) + update_read_state_for(status, highest_post_number) end end @@ -26,6 +29,15 @@ TopicStatusUpdate = Struct.new(:topic, :user) do def create_moderator_post_for(status) topic.add_moderator_post(user, message_for(status), options_for(status)) + topic.reload + end + + def update_read_state_for(status, old_highest_read) + if status.autoclosed? + # let's pretend all the people that read up to the autoclose message + # actually read the topic + PostTiming.pretend_read(topic.id, old_highest_read, topic.highest_post_number) + end end def message_for(status) diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb index 008196e89..e0c331056 100644 --- a/app/models/topic_user.rb +++ b/app/models/topic_user.rb @@ -204,8 +204,8 @@ class TopicUser < ActiveRecord::Base end end - def self.ensure_consistency! - exec_sql < last_read OR - seen_post_count <> LEAST(max_post_number,GREATEST(t.seen_post_count, last_read)) - ) +/*where*/ SQL + + builder.where < last_read OR + seen_post_count <> LEAST(max_post_number,GREATEST(t.seen_post_count, last_read)) +) +SQL + + if topic_id + builder.where("t.topic_id = :topic_id", topic_id: topic_id) + end + + builder.exec end end diff --git a/spec/models/post_timing_spec.rb b/spec/models/post_timing_spec.rb index 37642c44b..e1ca8a84e 100644 --- a/spec/models/post_timing_spec.rb +++ b/spec/models/post_timing_spec.rb @@ -8,6 +8,61 @@ describe PostTiming do it { should validate_presence_of :post_number } it { should validate_presence_of :msecs } + describe 'pretend_read' do + let!(:p1) { Fabricate(:post) } + let!(:p2) { Fabricate(:post, topic: p1.topic, user: p1.user) } + let!(:p3) { Fabricate(:post, topic: p1.topic, user: p1.user) } + + let :topic_id do + p1.topic_id + end + + def timing(user_id, post_number) + 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) + TopicUser.create!( + topic_id: topic_id, + user_id: user_id, + last_read_post_number: last_read_post_number, + seen_post_count: seen_post_count + ) + end + + it 'works correctly' do + timing(1,1) + timing(2,1) + timing(2,2) + timing(3,1) + 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) + + PostTiming.pretend_read(topic_id, 2, 3) + + PostTiming.where(topic_id: topic_id, user_id: 1, post_number: 3).count.should == 0 + PostTiming.where(topic_id: topic_id, user_id: 2, post_number: 3).count.should == 1 + PostTiming.where(topic_id: topic_id, user_id: 3, post_number: 3).count.should == 1 + + tu = TopicUser.where(topic_id: topic_id, user_id: 1).first + tu.last_read_post_number.should == 1 + tu.seen_post_count.should == 1 + + tu = TopicUser.where(topic_id: topic_id, user_id: 2).first + tu.last_read_post_number.should == 3 + tu.seen_post_count.should == 3 + + tu = TopicUser.where(topic_id: topic_id, user_id: 3).first + tu.last_read_post_number.should == 3 + tu.seen_post_count.should == 3 + + end + end + describe 'process_timings' do # integration test diff --git a/spec/models/topic_status_update_spec.rb b/spec/models/topic_status_update_spec.rb new file mode 100644 index 000000000..ff5e2ca9b --- /dev/null +++ b/spec/models/topic_status_update_spec.rb @@ -0,0 +1,29 @@ +# encoding: UTF-8 + +require 'spec_helper' +require_dependency 'post_destroyer' + +describe TopicStatusUpdate do + it "avoids notifying on automatically closed topics" do + # TODO: TopicStatusUpdate should supress message bus updates from the users it "pretends to read" + user = Fabricate(:user) + post = PostCreator.create(user, + raw: "this is a test post 123 this is a test post", + title: "hello world title", + ) + # TODO needed so counts sync up, + # PostCreator really should not give back out-of-date Topic + post.topic.reload + + # TODO: also annoying PostTiming is not logged + PostTiming.create!(topic_id: post.topic_id, user_id: user.id, post_number: 1, msecs: 0) + + admin = Fabricate(:admin) + TopicStatusUpdate.new(post.topic, admin).update!("autoclosed", true) + + post.topic.posts.count.should == 2 + + tu = TopicUser.where(user_id: user.id).first + tu.last_read_post_number.should == 2 + end +end