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

@@ -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

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

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
 

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

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.execute_at).to eq_time(Time.zone.now + 12.hours)
-            expect(topic_timer.created_at).to eq(Time.zone.now)
+            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

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

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

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",

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(

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

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
 

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 = now
-  time = Time.parse(now.to_s)
+  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

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