From 9ebabc1de868d2db0ba52cdae92d5b23079cba1e Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Thu, 28 Mar 2019 17:28:01 +1100 Subject: [PATCH] FEATURE: unconditionally update Topic updated_at when posts change in topic Previously we would bypass touching `Topic.updated_at` for whispers and post recovery / deletions. This meant that certain types of caching can not be done where we rely on this information for cache accuracy. For example if we know we have zero unread topics as of yesterday and whisper is made I need to bump this date so the cache remains accurate This is only half of a larger change but provides the groundwork. Confirmed none of our serializers leak out Topic.updated_at so this is safe spot for this info At the moment edits still do not change this but it is not relevant for the unread cache. This commit also cleans up some specs to use the new `eq_time` matcher for millisecond fidelity comparison of times Previously `freeze_time` would fudge this which is not that clean. --- lib/post_creator.rb | 6 ++-- lib/post_destroyer.rb | 25 ++++++++++++- .../default_current_user_provider_spec.rb | 2 +- .../concern/second_factor_manager_spec.rb | 2 +- spec/components/post_creator_spec.rb | 6 ++-- spec/integration/user_api_keys_spec.rb | 2 +- spec/jobs/toggle_topic_closed_spec.rb | 2 +- .../shared_examples_for_backup_store.rb | 4 ++- spec/models/post_action_spec.rb | 2 +- spec/models/post_spec.rb | 36 +++++++++++++++++++ spec/models/topic_spec.rb | 2 +- spec/rails_helper.rb | 13 +++++-- spec/support/time_matcher.rb | 9 +++++ 13 files changed, 96 insertions(+), 15 deletions(-) diff --git a/lib/post_creator.rb b/lib/post_creator.rb index c494ba660a3..68f23ea59e1 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -419,16 +419,18 @@ class PostCreator end def update_topic_stats + attrs = { updated_at: Time.now } + if @post.post_type != Post.types[:whisper] - attrs = {} attrs[:last_posted_at] = @post.created_at attrs[:last_post_user_id] = @post.user_id attrs[:word_count] = (@topic.word_count || 0) + @post.word_count attrs[:excerpt] = @post.excerpt_for_topic if new_topic? attrs[:bumped_at] = @post.created_at unless @post.no_bump - attrs[:updated_at] = Time.now @topic.update_columns(attrs) end + + @topic.update_columns(attrs) end def update_topic_auto_close diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index 16e91f4c586..04512fcbc8e 100644 --- a/lib/post_destroyer.rb +++ b/lib/post_destroyer.rb @@ -89,6 +89,8 @@ class PostDestroyer def staff_recovered @post.recover! + mark_topic_changed + if @post.topic && !@post.topic.private_message? if author = @post.user if @post.is_first_post? @@ -121,6 +123,7 @@ class PostDestroyer @post.trash!(@user) if @post.topic make_previous_post_the_last_one + mark_topic_changed clear_user_posted_flag Topic.reset_highest(@post.topic_id) end @@ -184,13 +187,33 @@ class PostDestroyer private + # we need topics to change if ever a post in them is deleted or created + # this ensures users relying on this information can keep unread tracking + # working as desired + def mark_topic_changed + # make this as fast as possible, can bypass everything + DB.exec(<<~SQL, updated_at: Time.now, id: @post.topic_id) + UPDATE topics + SET updated_at = :updated_at + WHERE id = :id + SQL + end + def make_previous_post_the_last_one - last_post = Post.where("topic_id = ? and id <> ?", @post.topic_id, @post.id).order('created_at desc').limit(1).first + last_post = Post + .select(:created_at, :user_id, :post_number) + .where("topic_id = ? and id <> ?", @post.topic_id, @post.id) + .order('created_at desc') + .limit(1) + .first + if last_post.present? && @post.topic.present? topic = @post.topic topic.last_posted_at = last_post.created_at topic.last_post_user_id = last_post.user_id topic.highest_post_number = last_post.post_number + + # we go via save here cause we need to run hooks topic.save!(validate: false) end end diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index 5d188b66e3c..7f73209161d 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -392,7 +392,7 @@ describe Auth::DefaultCurrentUserProvider do provider2 = provider("/", "HTTP_COOKIE" => "_t=#{unhashed_token}") u = provider2.current_user u.reload - expect(u.last_seen_at).to eq(Time.now) + expect(u.last_seen_at).to eq_time(Time.now) freeze_time 20.minutes.from_now diff --git a/spec/components/concern/second_factor_manager_spec.rb b/spec/components/concern/second_factor_manager_spec.rb index 0080f2ff675..4e6dcd2e7ac 100644 --- a/spec/components/concern/second_factor_manager_spec.rb +++ b/spec/components/concern/second_factor_manager_spec.rb @@ -53,7 +53,7 @@ RSpec.describe SecondFactorManager do token = user.totp.now expect(user.authenticate_totp(token)).to eq(true) - expect(user.user_second_factors.totp.last_used).to eq(DateTime.now) + expect(user.user_second_factors.totp.last_used).to eq_time(DateTime.now) expect(user.authenticate_totp(token)).to eq(false) end end diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index ffdf2cc1e7b..7deb2c2f7a7 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -374,8 +374,8 @@ describe PostCreator do topic_timer.reload - expect(topic_timer.execute_at).to eq(Time.zone.now + 12.hours) - expect(topic_timer.created_at).to eq(Time.zone.now) + expect(topic_timer.execute_at).to eq_time(Time.zone.now + 12.hours) + expect(topic_timer.created_at).to eq_time(Time.zone.now) end describe "when auto_close_topics_post_count has been reached" do @@ -401,7 +401,7 @@ describe PostCreator do )) expect(topic.closed).to eq(true) - expect(topic_timer.reload.deleted_at).to eq(Time.zone.now) + expect(topic_timer.reload.deleted_at).to eq_time(Time.zone.now) end end end diff --git a/spec/integration/user_api_keys_spec.rb b/spec/integration/user_api_keys_spec.rb index d0f0648266f..1e43f53c065 100644 --- a/spec/integration/user_api_keys_spec.rb +++ b/spec/integration/user_api_keys_spec.rb @@ -11,6 +11,6 @@ describe 'user api keys integration' do get '/session/current.json', headers: { HTTP_USER_API_KEY: user_api_key.key, } - expect(user_api_key.reload.last_used_at).to eq(Time.zone.now) + expect(user_api_key.reload.last_used_at).to eq_time(Time.zone.now) end end diff --git a/spec/jobs/toggle_topic_closed_spec.rb b/spec/jobs/toggle_topic_closed_spec.rb index 24b9d3c307b..22873f13b70 100644 --- a/spec/jobs/toggle_topic_closed_spec.rb +++ b/spec/jobs/toggle_topic_closed_spec.rb @@ -64,7 +64,7 @@ describe Jobs::ToggleTopicClosed do topic_timer = topic.public_topic_timer expect(topic_timer.status_type).to eq(TopicTimer.types[:close]) - expect(topic_timer.execute_at).to eq(5.hours.from_now) + expect(topic_timer.execute_at).to eq_time(5.hours.from_now) end end end diff --git a/spec/lib/backup_restore/shared_examples_for_backup_store.rb b/spec/lib/backup_restore/shared_examples_for_backup_store.rb index 07ed5d481eb..cd5f5430db6 100644 --- a/spec/lib/backup_restore/shared_examples_for_backup_store.rb +++ b/spec/lib/backup_restore/shared_examples_for_backup_store.rb @@ -215,7 +215,9 @@ shared_examples "remote backup store" do describe "#upload_file" do def upload_file - freeze_time + # time has fidelity issues freeze a time that is not going to be prone + # to that + freeze_time(Time.now.to_s) backup = BackupFile.new( filename: "foo.tar.gz", diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 923ec3fd740..1b16944a8b0 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -946,7 +946,7 @@ describe PostAction do expect(topic.reload.closed).to eq(true) timer = TopicTimer.last - expect(timer.execute_at).to eq(1.hour.from_now) + expect(timer.execute_at).to eq_time(1.hour.from_now) freeze_time timer.execute_at Jobs.expects(:enqueue_in).with( diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 2f18227d199..ae4d32bcd01 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -1311,4 +1311,40 @@ describe Post do end end + context 'topic updated_at' do + let :topic do + create_post.topic + end + + def updates_topic_updated_at + + freeze_time 1.day.from_now + time = Time.now + + result = yield + + topic.reload + expect(topic.updated_at).to eq_time(time) + + result + end + + it "will update topic updated_at for all topic related events" do + SiteSetting.enable_whispers = true + + post = updates_topic_updated_at do + create_post(topic_id: topic.id, post_type: Post.types[:whisper]) + end + + updates_topic_updated_at do + PostDestroyer.new(Discourse.system_user, post).destroy + end + + updates_topic_updated_at do + PostDestroyer.new(Discourse.system_user, post).recover + end + + end + end + end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 65a9be83bbe..2fb6a681fd7 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1582,7 +1582,7 @@ describe Topic do closing_topic.set_or_create_timer(TopicTimer.types[:open], nil) topic_timer = closing_topic.public_topic_timer - expect(topic_timer.execute_at).to eq(5.hours.from_now) + expect(topic_timer.execute_at).to eq_time(5.hours.from_now) expect(topic_timer.status_type).to eq(TopicTimer.types[:close]) end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 6eaa2a2934a..7cca874f3c2 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -278,8 +278,17 @@ def set_cdn_url(cdn_url) end def freeze_time(now = Time.now) - datetime = DateTime.parse(now.to_s) - time = Time.parse(now.to_s) + time = now + datetime = now + + if Time === now + datetime = now.to_datetime + elsif DateTime === now + time = now.to_time + else + datetime = DateTime.parse(now.to_s) + time = Time.parse(now.to_s) + end if block_given? raise "nested freeze time not supported" if TrackTimeStub.stubbed diff --git a/spec/support/time_matcher.rb b/spec/support/time_matcher.rb index eb7ec2d80a2..02a6fd859bc 100644 --- a/spec/support/time_matcher.rb +++ b/spec/support/time_matcher.rb @@ -6,3 +6,12 @@ RSpec::Matchers.define :be_within_one_second_of do |expected_time| "#{actual_time} is not within 1 second of #{expected_time}" end end + +RSpec::Matchers.define :eq_time do |expected_time| + match do |actual_time| + (actual_time - expected_time).abs < 0.001 + end + failure_message do |actual_time| + "#{actual_time} is not within 1 millisecond of #{expected_time}" + end +end