FEATURE: unconditionally update Topic updated_at when posts change in topic

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.

diff --git a/lib/post_creator.rb b/lib/post_creator.rb
index c494ba6..68f23ea 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 16e91f4..04512fc 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 5d188b6..7f73209 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 0080f2f..4e6dcd2 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 ffdf2cc..7deb2c2 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 d0f0648..1e43f53 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 24b9d3c..22873f1 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 07ed5d4..cd5f543 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 923ec3f..1b16944 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 2f18227..ae4d32b 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

[... diff too long, it was truncated ...]

GitHub sha: 9ebabc1d

2 Likes

DEV: use eq_time which allows for 1ms fidelity

DEV: correct spec to test for correct fidelity