Merge pull request #1931 from codidact/0valt/1930/comments-last-activity

Oaphi · web-flow · commit a93b782f97bd · 2026-01-09T17:57:13.000Z
Follow / unfollow thread actions should bump its last activity date &amp; time + minor general fixes
diff --git a/.rubocop.yml b/.rubocop.yml
@@ -58,7 +58,7 @@ Naming/BlockForwarding:
   EnforcedStyle: explicit
 Naming/PredicateMethod:
   Mode: conservative
-  AllowedPatterns: ['verify_.*', 'check_.*', 'enforce_.*', 'setup_.*']
+  AllowedPatterns: ['verify_.*', 'check_.*', 'enforce_.*', 'setup_.*','add_.*', 'remove_.*']
   AllowedMethods:
     - post_sign_in # returns a boolean but is a necessary after-sign-in method for both normal and SAML flows
     - comment_rate_limited? # returns a tuple with the boolean as the first value
diff --git a/app/assets/javascripts/comments.js b/app/assets/javascripts/comments.js
@@ -250,11 +250,32 @@ $(() => {
 
     QPixel.handleJSONResponse(data, () => {
       const wrapper = getCommentThreadWrapper($tgt);
-      const inline = isInlineCommentThread(wrapper);
-      openThread(wrapper, threadID, { inline });
+
+      if (wrapper) {
+        const inline = isInlineCommentThread(wrapper);
+        openThread(wrapper, threadID, { inline });
+      }
     });
   });
 
+  $(document).on('click', '.js--unfollow-thread', async (ev) => {
+    ev.preventDefault();
+
+    const $tgt = $(ev.target);
+    const threadID = $tgt.data('thread');
+
+    const data = await QPixel.unfollowThread(threadID);
+
+    QPixel.handleJSONResponse(data, () => {
+      const wrapper = getCommentThreadWrapper($tgt);
+
+      if (wrapper) {
+        const inline = isInlineCommentThread(wrapper);
+        openThread(wrapper, threadID, { inline });
+      }
+    });
+  })
+
   /**
    * @param {Element} target
    */
diff --git a/app/assets/javascripts/qpixel_api.js b/app/assets/javascripts/qpixel_api.js
@@ -501,6 +501,14 @@ window.QPixel = {
     return QPixel.parseJSONResponse(resp, 'Failed to follow thread');
   },
 
+  unfollowThread: async (id) => {
+    const resp = await QPixel.fetchJSON(`/comments/thread/${id}/unfollow`, {}, {
+      headers: { 'Accept': 'application/json' },
+    });
+
+    return QPixel.parseJSONResponse(resp, 'Failed to unfollow thread');
+  },
+
   lockThread: async (id, duration) => {
     const resp = await QPixel.fetchJSON(`/comments/thread/${id}/lock`, {
       duration,
diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb
@@ -12,6 +12,7 @@ class CommentsController < ApplicationController
                                     :archive_thread,
                                     :delete_thread,
                                     :follow_thread,
+                                    :unfollow_thread,
                                     :lock_thread,
                                     :thread_unrestrict,
                                     :thread_followers]
@@ -188,14 +189,16 @@ def show
   end
 
   def thread
-    respond_to do |format|
-      format.html { render 'comments/thread' }
-      format.json { render json: @comment_thread }
+    if stale?(last_modified: @comment_thread.last_activity.utc)
+      respond_to do |format|
+        format.html { render 'comments/thread' }
+        format.json { render json: @comment_thread }
+      end
     end
   end
 
   def thread_content
-    if stale?(last_modified: @comment_thread.last_activity_at.utc)
+    if stale?(last_modified: @comment_thread.last_activity.utc)
       render partial: 'comment_threads/expanded',
              locals: { inline: params[:inline] == 'true',
                        show_deleted: params[:show_deleted_comments] == '1',
@@ -264,7 +267,7 @@ def delete_thread
   end
 
   def follow_thread
-    status = ThreadFollower.create(comment_thread: @comment_thread, user: current_user)
+    status = @comment_thread.add_follower(current_user)
     restrict_thread_response(@comment_thread, status)
   end
 
@@ -293,7 +296,7 @@ def undelete_thread
   end
 
   def unfollow_thread
-    status = ThreadFollower.find_by(comment_thread: @comment_thread, user: current_user)&.destroy
+    status = @comment_thread.remove_follower(current_user)
     restrict_thread_response(@comment_thread, status)
   end
 
@@ -311,8 +314,6 @@ def thread_unrestrict
       unarchive_thread
     when 'delete'
       undelete_thread
-    when 'follow'
-      unfollow_thread
     else
       not_found!
     end
diff --git a/app/jobs/clean_up_thread_followers_job.rb b/app/jobs/clean_up_thread_followers_job.rb
@@ -0,0 +1,24 @@
+class CleanUpThreadFollowersJob < ApplicationJob
+  queue_as :default
+
+  def perform
+    sql = File.read(Rails.root.join('db/scripts/threads_with_duplicate_followers.sql'))
+    threads = ActiveRecord::Base.connection.execute(sql).to_a
+
+    threads.each do |thread|
+      user_id, thread_id = thread
+
+      followers = ThreadFollower.where(comment_thread_id: thread_id, user_id: user_id)
+
+      next unless followers.many?
+
+      duplicate = followers.first
+      result = duplicate.destroy
+
+      unless result
+        puts "failed to destroy thread follower duplicate \"#{duplicate.id}\""
+        duplicate.errors.each { |e| puts e.full_message }
+      end
+    end
+  end
+end
diff --git a/app/models/comment.rb b/app/models/comment.rb
@@ -17,16 +17,12 @@ class Comment < ApplicationRecord
   after_create :create_follower
   after_update :delete_thread
 
+  before_save :bump_last_activity
+
   counter_culture :comment_thread, column_name: proc { |model| model.deleted? ? nil : 'reply_count' }, touch: true
 
   validate :content_length
 
-  # Gets last activity date and time on the comment
-  # @return [DateTime] last activity date and time
-  def last_activity_at
-    [created_at, updated_at].compact.max
-  end
-
   def root
     # If parent_question is nil, the comment is already on a question, so we can just return post.
     parent_question || post
@@ -41,12 +37,28 @@ def content_length
     end
   end
 
+  # Gets last activity date and time on the comment
+  # @return [DateTime] last activity date and time
+  def last_activity
+    [created_at, updated_at, last_activity_at].compact.max
+  end
+
   def pings
     pingable = comment_thread.pingable
     matches = content.scan(USER_PING_REG_EXP)
     matches.flatten.select { |m| pingable.include?(m.to_i) }.map(&:to_i)
   end
 
+  # Directly bumps the comment's last activity date & time
+  # @param persist_changes [Boolean] if set to +true+, will persist the changes
+  def bump_last_activity(persist_changes: false)
+    self.last_activity_at = DateTime.now
+
+    if persist_changes
+      save
+    end
+  end
+
   private
 
   def create_follower
diff --git a/app/models/comment_thread.rb b/app/models/comment_thread.rb
@@ -16,6 +16,8 @@ class CommentThread < ApplicationRecord
 
   after_create :create_follower
 
+  before_save :bump_last_activity
+
   # Gets threads appropriately scoped for a given user & post
   # @param user [User, nil] user to check
   # @para post [Post] post to check
@@ -51,9 +53,9 @@ def can_access?(user)
 
   # Gets last activity date and time on the thread
   # @return [DateTime] last activity date and time
-  def last_activity_at
-    last_comment_activity_at = comments.map(&:last_activity_at).max
-    [created_at, locked_at, updated_at, last_comment_activity_at].compact.max
+  def last_activity
+    last_comment_activity = comments.map(&:last_activity).compact.max
+    [created_at, updated_at, last_activity_at, last_comment_activity].compact.max
   end
 
   # Gets a list of user IDs who should be pingable in the thread.
@@ -86,6 +88,40 @@ def maximum_title_length
     end
   end
 
+  # Registers a given user as a follower of the thread
+  # @param user [User] user to register as a follower
+  # @return [Boolean] status of the operation
+  def add_follower(user)
+    if ThreadFollower.where(comment_thread: self, user: user).any?
+      bump_last_activity(persist_changes: true)
+      return true
+    end
+
+    ThreadFollower.create(comment_thread: self, user: user)
+  end
+
+  # Directly bumps the thread's last activity date & time
+  # @param persist_changes [Boolean] if set to +true+, will persist the changes
+  def bump_last_activity(persist_changes: false)
+    self.last_activity_at = DateTime.now
+
+    if persist_changes
+      save
+    end
+  end
+
+  # Removes a given user from the thread's followers
+  # @param user [User] user to remove from followers
+  # @return [Boolean] status of the operation
+  def remove_follower(user)
+    if ThreadFollower.where(comment_thread: self, user: user).none?
+      bump_last_activity(persist_changes: true)
+      return true
+    end
+
+    ThreadFollower.where(comment_thread: self, user: user).destroy_all.any?
+  end
+
   private
 
   # Comment author and post author are automatically followed to the thread. Question author is NOT
diff --git a/app/models/thread_follower.rb b/app/models/thread_follower.rb
@@ -3,10 +3,17 @@ class ThreadFollower < ApplicationRecord
   belongs_to :post, optional: true
   belongs_to :user
 
+  after_create :bump_thread_last_activity
+  before_destroy :bump_thread_last_activity
+
   validate :thread_or_post
 
   private
 
+  def bump_thread_last_activity
+    comment_thread&.bump_last_activity(persist_changes: true)
+  end
+
   def thread_or_post
     if comment_thread.nil? && post.nil?
       errors.add(:base, 'Must refer to either a comment thread or a post.')
diff --git a/app/models/user.rb b/app/models/user.rb
@@ -483,13 +483,13 @@ def active_flags(post)
   end
 
   # Anonymizes the user (f.e., for the purpose of soft deletion)
-  # @param persist_changes [Boolean] if set to +false+, will persist the changes
+  # @param persist_changes [Boolean] if set to +true+, will persist the changes
   def anonymize(persist_changes: false)
     assign_attributes(username: "user#{id}",
                       email: "#{id}@deleted.localhost",
                       password: SecureRandom.hex(32))
 
-    unless persist_changes
+    if persist_changes
       skip_reconfirmation!
       save
     end
@@ -505,7 +505,7 @@ def soft_delete(attribute_to)
                       deleted_by_id: attribute_to.id,
                       deleted_at: DateTime.now)
 
-    anonymize(persist_changes: true)
+    anonymize(persist_changes: false)
 
     skip_reconfirmation!
     save
diff --git a/app/views/comments/_thread_follow_link.html.erb b/app/views/comments/_thread_follow_link.html.erb
@@ -8,8 +8,7 @@
 
 <% if thread.followed_by?(user) %>
   <a href="#"
-     class="widget--header-link js--unrestrict-thread"
-     data-action="follow"
+     class="widget--header-link js--unfollow-thread"
      data-thread="<%= thread.id %>"
      title="You are following this thread and will be notified of every response. You can unfollow at any time."
      role="button"
diff --git a/config/routes.rb b/config/routes.rb
@@ -247,6 +247,7 @@
     post   'thread/:id/archive',           to: 'comments#archive_thread', as: :archive_comment_thread
     post   'thread/:id/delete',            to: 'comments#delete_thread', as: :delete_comment_thread
     post   'thread/:id/follow',            to: 'comments#follow_thread', as: :follow_comment_thread
+    post   'thread/:id/unfollow',          to: 'comments#unfollow_thread', as: :unfollow_comment_thread
     post   'thread/:id/lock',              to: 'comments#lock_thread', as: :lock_comment_thread
 
     post   'thread/:id/unrestrict',        to: 'comments#thread_unrestrict', as: :unrestrict_comment_thread
diff --git a/config/schedule.rb b/config/schedule.rb
@@ -26,6 +26,10 @@
   runner 'scripts/run_complaints_closure.rb'
 end
 
+every 7.days, at: '02:35' do
+  runner 'scripts/run_thread_followers_cleanup.rb'
+end
+
 every 6.hours do
   runner 'scripts/recalc_abilities.rb'
 end
diff --git a/db/migrate/20251221140930_add_last_activity_at_to_comments.rb b/db/migrate/20251221140930_add_last_activity_at_to_comments.rb
@@ -0,0 +1,5 @@
+class AddLastActivityAtToComments < ActiveRecord::Migration[7.2]
+  def change
+    add_column :comments, :last_activity_at, :datetime
+  end
+end
diff --git a/db/migrate/20251221142105_add_last_activity_at_to_comment_threads.rb b/db/migrate/20251221142105_add_last_activity_at_to_comment_threads.rb
@@ -0,0 +1,5 @@
+class AddLastActivityAtToCommentThreads < ActiveRecord::Migration[7.2]
+  def change
+    add_column :comment_threads, :last_activity_at, :datetime
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.2].define(version: 2025_10_15_121326) do
+ActiveRecord::Schema[7.2].define(version: 2025_12_21_142105) do
   create_table "abilities", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
     t.bigint "community_id"
     t.string "name"
@@ -176,6 +176,7 @@
     t.datetime "updated_at", precision: nil, null: false
     t.bigint "community_id", null: false
     t.datetime "locked_at", precision: nil
+    t.datetime "last_activity_at"
     t.index ["archived_by_id"], name: "index_comment_threads_on_archived_by_id"
     t.index ["community_id"], name: "index_comment_threads_on_community_id"
     t.index ["deleted_by_id"], name: "index_comment_threads_on_deleted_by_id"
@@ -195,6 +196,7 @@
     t.boolean "has_reference", default: false, null: false
     t.text "reference_text"
     t.bigint "references_comment_id"
+    t.datetime "last_activity_at"
     t.index ["comment_thread_id"], name: "index_comments_on_comment_thread_id"
     t.index ["community_id"], name: "index_comments_on_community_id"
     t.index ["post_id"], name: "index_comments_on_post_type_and_post_id"
diff --git a/db/scripts/threads_with_duplicate_followers.sql b/db/scripts/threads_with_duplicate_followers.sql
@@ -0,0 +1,13 @@
+select
+  user_id,
+  comment_thread_id,
+  count(*) as count
+from
+  thread_followers
+where
+  post_id is null
+group by
+  user_id,
+  comment_thread_id
+having
+  count > 1;
diff --git a/global.d.ts b/global.d.ts
@@ -601,6 +601,13 @@ interface QPixel {
    */
   followThread?: (id: string) => Promise<QPixelResponseJSON>
 
+  /**
+   * Attempts to unfollow a comment thread
+   * @param id id of the thread to unfollow
+   * @returns result of the operation
+   */
+  unfollowThread?: (id: string) => Promise<QPixelResponseJSON>
+
   /**
    * Attempts to start following comments on a given post
    * @param postId id of the post to follow comments on
diff --git a/scripts/run_thread_followers_cleanup.rb b/scripts/run_thread_followers_cleanup.rb
@@ -0,0 +1 @@
+CleanUpThreadFollowersJob.perform_later
diff --git a/test/comments_test_helpers.rb b/test/comments_test_helpers.rb
@@ -68,7 +68,7 @@ def try_follow_thread(thread)
   # Attempts to unfollow a given comment thread
   # @param thread [CommentThread] thread to unfollow
   def try_unfollow_thread(thread)
-    post :thread_unrestrict, params: { id: thread.id, type: 'follow' }
+    post :unfollow_thread, params: { id: thread.id }
   end
 
   # Attempts to follow new threads on a given post
diff --git a/test/fixtures/comment_threads.yml b/test/fixtures/comment_threads.yml
diff --git a/test/fixtures/comments.yml b/test/fixtures/comments.yml
diff --git a/test/jobs/clean_up_thread_followers_job_test.rb b/test/jobs/clean_up_thread_followers_job_test.rb
diff --git a/test/models/comment_test.rb b/test/models/comment_test.rb
diff --git a/test/models/comment_thread_test.rb b/test/models/comment_thread_test.rb
diff --git a/test/models/user_test.rb b/test/models/user_test.rb
