FIX: Better tracking of topic visibility changes (#26709)

This commit introduces a few changes as a result of
customer issues with finding why a topic was relisted.
In one case, if a user edited the OP of a topic that was
unlisted and hidden because of too many flags, the topic
would get relisted by directly changing topic.visible,
instead of going via TopicStatusUpdater.

To improve tracking we:

* Introduce a visibility_reason_id to topic which functions
  in a similar way to hidden_reason_id on post, this column is
  set from the various places we change topic visibility
* Fix Post#unhide! which was directly modifying topic.visible,
  instead we use TopicStatusUpdater which sets visibility_reason_id
  and also makes a small action post
* Show the reason topic visibility changed when hovering the
  unlisted icon in topic status on topic titles

app/assets/javascripts/discourse/app/components/topic-status.js

@@ -82,7 +82,17 @@ export default Component.extend({
 
   _set(name, icon, key, iconArgs) {
     this.set(`${name}Icon`, htmlSafe(iconHTML(`${icon}`, iconArgs)));
-    this.set(`${name}Title`, I18n.t(`topic_statuses.${key}.help`));
+
+    const translationParams = {};
+
+    if (name === "invisible") {
+      translationParams.unlistedReason = this.topic.visibilityReasonTranslated;
+    }
+
+    this.set(
+      `${name}Title`,
+      I18n.t(`topic_statuses.${key}.help`, translationParams)
+    );
     return true;
   },
 

app/assets/javascripts/discourse/app/lib/constants.js

@@ -70,3 +70,13 @@ export const AUTO_GROUPS = {
 };
 
 export const MAX_NOTIFICATIONS_LIMIT_PARAMS = 60;
+
+export const TOPIC_VISIBILITY_REASONS = {
+  op_flag_threshold_reached: 0,
+  op_unhidden: 1,
+  embedded_topic: 2,
+  manually_unlisted: 3,
+  manually_relisted: 4,
+  bulk_action: 5,
+  unknown: 99,
+};

app/assets/javascripts/discourse/app/models/topic.js

@@ -5,6 +5,7 @@ import { resolveShareUrl } from "discourse/helpers/share-url";
 import { ajax } from "discourse/lib/ajax";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import { fmt, propertyEqual } from "discourse/lib/computed";
+import { TOPIC_VISIBILITY_REASONS } from "discourse/lib/constants";
 import { longDate } from "discourse/lib/formatter";
 import { applyModelTransformations } from "discourse/lib/model-transformers";
 import PreloadStore from "discourse/lib/preload-store";
@@ -461,6 +462,21 @@ export default class Topic extends RestModel {
     return visible !== undefined ? !visible : undefined;
   }
 
+  @discourseComputed("visibility_reason_id")
+  visibilityReasonTranslated() {
+    if (
+      this.visibility_reason_id &&
+      this.visibility_reason_id !== TOPIC_VISIBILITY_REASONS.unknown
+    ) {
+      const reasonKey = Object.keys(TOPIC_VISIBILITY_REASONS).find(
+        (key) => TOPIC_VISIBILITY_REASONS[key] === this.visibility_reason_id
+      );
+      return I18n.t(`topic_statuses.visibility_reasons.${reasonKey}`);
+    }
+
+    return "";
+  }
+
   @discourseComputed("id")
   searchContext(id) {
     return { type: "topic", id };

app/assets/javascripts/discourse/app/raw-views/topic-status.js

@@ -67,7 +67,17 @@ export default EmberObject.extend({
     }
 
     results.forEach((result) => {
-      result.title = I18n.t(`topic_statuses.${result.key}.help`);
+      const translationParams = {};
+
+      if (result.key === "unlisted") {
+        translationParams.unlistedReason = topic.visibilityReasonTranslated;
+      }
+
+      result.title = I18n.t(
+        `topic_statuses.${result.key}.help`,
+        translationParams
+      );
+
       if (
         this.currentUser &&
         (result.key === "pinned" || result.key === "unpinned")

app/controllers/topics_controller.rb

@@ -497,6 +497,18 @@ class TopicsController < ApplicationController
         Topic.find_by(id: topic_id)
       end
 
+    status_opts = { until: params[:until].presence }
+
+    if status == "visible"
+      status_opts[:visibility_reason_id] = (
+        if enabled
+          Topic.visibility_reasons[:manually_relisted]
+        else
+          Topic.visibility_reasons[:manually_unlisted]
+        end
+      )
+    end
+
     case status
     when "closed"
       guardian.ensure_can_close_topic!(@topic)
@@ -510,9 +522,7 @@ class TopicsController < ApplicationController
       guardian.ensure_can_moderate!(@topic)
     end
 
-    params[:until] === "" ? params[:until] = nil : params[:until]
+    @topic.update_status(status, enabled, current_user, status_opts)
-
-    @topic.update_status(status, enabled, current_user, until: params[:until])
 
     render json:
              success_json.merge!(

app/jobs/regular/make_embedded_topic_visible.rb

@@ -6,7 +6,12 @@ module Jobs
       raise Discourse::InvalidParameters.new(:topic_id) if args[:topic_id].blank?
 
       if topic = Topic.find_by(id: args[:topic_id])
-        topic.update_status("visible", true, topic.user)
+        topic.update_status(
+          "visible",
+          true,
+          topic.user,
+          { visibility_reason_id: Topic.visibility_reasons[:embedded_topic] },
+        )
       end
     end
   end

app/models/post.rb

@@ -612,15 +612,26 @@ class Post < ActiveRecord::Base
 
     Post.transaction do
       self.skip_validation = true
+      should_update_user_stat = true
 
       update!(hidden: true, hidden_at: Time.zone.now, hidden_reason_id: reason)
 
-      Topic.where(
+      any_visible_posts_in_topic =
-        "id = :topic_id AND NOT EXISTS(SELECT 1 FROM POSTS WHERE topic_id = :topic_id AND NOT hidden)",
+        Post.exists?(topic_id: topic_id, hidden: false, post_type: Post.types[:regular])
-        topic_id: topic_id,
-      ).update_all(visible: false)
 
-      UserStatCountUpdater.decrement!(self)
+      if !any_visible_posts_in_topic
+        self.topic.update_status(
+          "visible",
+          false,
+          Discourse.system_user,
+          { visibility_reason_id: Topic.visibility_reasons[:op_flag_threshold_reached] },
+        )
+        should_update_user_stat = false
+      end
+
+      # We need to do this because TopicStatusUpdater also does the decrement
+      # and we don't want to double count for the OP.
+      UserStatCountUpdater.decrement!(self) if should_update_user_stat
     end
 
     # inform user
@@ -652,8 +663,29 @@ class Post < ActiveRecord::Base
   def unhide!
     Post.transaction do
       self.update!(hidden: false)
-      self.topic.update(visible: true) if is_first_post?
+      should_update_user_stat = true
-      UserStatCountUpdater.increment!(self)
+
+      # NOTE: We have to consider `nil` a valid reason here because historically
+      # topics didn't have a visibility_reason_id, if we didn't do this we would
+      # break backwards compat since we cannot backfill data.
+      hidden_because_of_op_flagging =
+        self.topic.visibility_reason_id == Topic.visibility_reasons[:op_flag_threshold_reached] ||
+          self.topic.visibility_reason_id.nil?
+
+      if is_first_post? && hidden_because_of_op_flagging
+        self.topic.update_status(
+          "visible",
+          true,
+          Discourse.system_user,
+          { visibility_reason_id: Topic.visibility_reasons[:op_unhidden] },
+        )
+        should_update_user_stat = false
+      end
+
+      # We need to do this because TopicStatusUpdater also does the increment
+      # and we don't want to double count for the OP.
+      UserStatCountUpdater.increment!(self) if should_update_user_stat
+
       save(validate: false)
     end
 

app/models/topic.rb

@@ -43,6 +43,19 @@ class Topic < ActiveRecord::Base
     [self.share_thumbnail_size] + DiscoursePluginRegistry.topic_thumbnail_sizes
   end
 
+  def self.visibility_reasons
+    @visible_reasons ||=
+      Enum.new(
+        op_flag_threshold_reached: 0,
+        op_unhidden: 1,
+        embedded_topic: 2,
+        manually_unlisted: 3,
+        manually_relisted: 4,
+        bulk_action: 5,
+        unknown: 99,
+      )
+  end
+
   def thumbnail_job_redis_key(sizes)
     "generate_topic_thumbnail_enqueue_#{id}_#{sizes.inspect}"
   end
@@ -2178,6 +2191,7 @@ end
 #  slow_mode_seconds         :integer          default(0), not null
 #  bannered_until            :datetime
 #  external_id               :string
+#  visibility_reason_id      :integer
 #
 # Indexes
 #

app/serializers/listable_topic_serializer.rb

@@ -26,7 +26,8 @@ class ListableTopicSerializer < BasicTopicSerializer
              :liked,
              :unicode_title,
              :unread_by_group_member,
-             :thumbnails
+             :thumbnails,
+             :visibility_reason_id
 
   has_one :last_poster, serializer: BasicUserSerializer, embed: :objects
 
@@ -159,6 +160,10 @@ class ListableTopicSerializer < BasicTopicSerializer
     !!object.topic_list&.publish_read_state
   end
 
+  def include_visibility_reason_id?
+    object.visibility_reason_id.present?
+  end
+
   protected
 
   def unread_helper

app/serializers/topic_view_serializer.rb

@@ -43,6 +43,7 @@ class TopicViewSerializer < ApplicationSerializer
     :image_url,
     :slow_mode_seconds,
     :external_id,
+    :visibility_reason_id,
   )
 
   attributes(
@@ -321,4 +322,8 @@ class TopicViewSerializer < ApplicationSerializer
   def include_categories?
     scope.can_lazy_load_categories?
   end
+
+  def include_visibility_reason_id?
+    object.topic.visibility_reason_id.present?
+  end
 end

app/services/topic_status_updater.rb

@@ -55,6 +55,12 @@ TopicStatusUpdater =
         )
       end
 
+      if status.visible?
+        topic.update(
+          visibility_reason_id: opts[:visibility_reason_id] || Topic.visibility_reasons[:unknown],
+        )
+      end
+
       if @topic_timer
         if status.manually_closing_topic? || status.closing_topic?
           topic.delete_topic_timer(TopicTimer.types[:close])

config/locales/client.en.yml

@@ -4084,10 +4084,17 @@ en:
         title: "Pinned"
         help: "This topic is pinned for you; it will display at the top of its category"
       unlisted:
-        help: "This topic is unlisted; it will not be displayed in topic lists, and can only be accessed via a direct link"
+        help: "This topic is unlisted; it will not be displayed in topic lists, and can only be accessed via a direct link. %{unlistedReason}"
       personal_message:
         title: "This topic is a personal message"
         help: "This topic is a personal message"
+      visibility_reasons:
+        op_flag_threshold_reached: "This topic was automatically unlisted because it reached the flag threshold"
+        op_unhidden: "This topic was relisted by the author"
+        embedded_topic: "This topic was relisted because it is an embedded topic"
+        manually_unlisted: "This topic was manually unlisted by an admin or moderator"
+        manually_relisted: "This topic was manually relisted by an admin or moderator"
+        bulk_action: "This topic's visibility was changed because of a bulk action performed by a user"
     posts: "Posts"
     pending_posts:
       label: "Pending"

db/migrate/20240423013808_add_visibility_reason_id_to_topics.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddVisibilityReasonIdToTopics < ActiveRecord::Migration[7.0]
+  def change
+    add_column :topics, :visibility_reason_id, :integer
+  end
+end

lib/post_action_creator.rb

@@ -235,7 +235,14 @@ class PostActionCreator
     return if not_auto_action_flag_type && !@queue_for_review
 
     if @queue_for_review
-      @post.topic.update_status("visible", false, @created_by) if @post.is_first_post?
+      if @post.is_first_post?
+        @post.topic.update_status(
+          "visible",
+          false,
+          @created_by,
+          { visibility_reason_id: Topic.visibility_reasons[:op_flag_threshold_reached] },
+        )
+      end
 
       @post.hide!(
         @post_action_type_id,

lib/tasks/javascript.rake

@@ -162,6 +162,8 @@ task "javascript:update_constants" => :environment do
     export const AUTO_GROUPS = #{auto_groups.to_json};
 
     export const MAX_NOTIFICATIONS_LIMIT_PARAMS = #{NotificationsController::INDEX_LIMIT};
+
+    export const TOPIC_VISIBILITY_REASONS = #{Topic.visibility_reasons.to_json};
   JS
 
   pretty_notifications = Notification.types.map { |n| "  #{n[0]}: #{n[1]}," }.join("\n")

lib/topics_bulk_action.rb

@@ -183,7 +183,12 @@ class TopicsBulkAction
   def unlist
     topics.each do |t|
       if guardian.can_moderate?(t)
-        t.update_status("visible", false, @user)
+        t.update_status(
+          "visible",
+          false,
+          @user,
+          { visibility_reason_id: Topic.visibility_reasons[:bulk_action] },
+        )
         @changed_ids << t.id
       end
     end
@@ -192,7 +197,12 @@ class TopicsBulkAction
   def relist
     topics.each do |t|
       if guardian.can_moderate?(t)
-        t.update_status("visible", true, @user)
+        t.update_status(
+          "visible",
+          true,
+          @user,
+          { visibility_reason_id: Topic.visibility_reasons[:bulk_action] },
+        )
         @changed_ids << t.id
       end
     end

spec/models/post_action_spec.rb

@@ -544,6 +544,9 @@ RSpec.describe PostAction do
       expect(post.hidden_at).to be_present
       expect(post.hidden_reason_id).to eq(Post.hidden_reasons[:flag_threshold_reached])
       expect(post.topic.visible).to eq(false)
+      expect(post.topic.visibility_reason_id).to eq(
+        Topic.visibility_reasons[:op_flag_threshold_reached],
+      )
 
       post.revise(post.user, raw: post.raw + " ha I edited it ")
 
@@ -553,6 +556,7 @@ RSpec.describe PostAction do
       expect(post.hidden_reason_id).to eq(Post.hidden_reasons[:flag_threshold_reached]) # keep most recent reason
       expect(post.hidden_at).to be_present # keep the most recent hidden_at time
       expect(post.topic.visible).to eq(true)
+      expect(post.topic.visibility_reason_id).to eq(Topic.visibility_reasons[:op_unhidden])
 
       PostActionCreator.spam(eviltrout, post)
       PostActionCreator.off_topic(walterwhite, post)
@@ -567,6 +571,9 @@ RSpec.describe PostAction do
       expect(post.hidden_at).to be_present
       expect(post.hidden_reason_id).to eq(Post.hidden_reasons[:flag_threshold_reached_again])
       expect(post.topic.visible).to eq(false)
+      expect(post.topic.visibility_reason_id).to eq(
+        Topic.visibility_reasons[:op_flag_threshold_reached],
+      )
 
       post.revise(post.user, raw: post.raw + " ha I edited it again ")
 
@@ -575,7 +582,10 @@ RSpec.describe PostAction do
       expect(post.hidden).to eq(true)
       expect(post.hidden_at).to be_present
       expect(post.hidden_reason_id).to eq(Post.hidden_reasons[:flag_threshold_reached_again])
-      expect(post.topic.visible).to eq(false)
+      expect(post.topic.reload.visible).to eq(false)
+      expect(post.topic.visibility_reason_id).to eq(
+        Topic.visibility_reasons[:op_flag_threshold_reached],
+      )
     end
 
     it "doesn't fail when post has nil user" do

spec/models/post_spec.rb

@@ -1540,6 +1540,25 @@ RSpec.describe Post do
 
       expect(post.hidden).to eq(false)
       expect(hidden_topic.visible).to eq(true)
+      expect(hidden_topic.visibility_reason_id).to eq(Topic.visibility_reasons[:op_unhidden])
+    end
+
+    it "will not unhide the topic if the topic visibility_reason_id is not op_flag_threshold_reached" do
+      hidden_topic =
+        Fabricate(
+          :topic,
+          visible: false,
+          visibility_reason_id: Topic.visibility_reasons[:manually_unlisted],
+        )
+      post = create_post(topic: hidden_topic)
+      post.update_columns(hidden: true, hidden_at: Time.now, hidden_reason_id: 1)
+      post.reload
+
+      expect(post.hidden).to eq(true)
+      post.unhide!
+
+      hidden_topic.reload
+      expect(hidden_topic.visible).to eq(false)
     end
 
     it "should increase user_stat topic_count for first post" do

spec/requests/topics_controller_spec.rb

@@ -1224,6 +1224,9 @@ RSpec.describe TopicsController do
 
         expect(response.status).to eq(200)
         expect(topic.reload.visible).to eq(false)
+        expect(topic.reload.visibility_reason_id).to eq(
+          Topic.visibility_reasons[:manually_unlisted],
+        )
         expect(topic.posts.last.action_code).to eq("visible.disabled")
       end
 
@@ -1234,6 +1237,9 @@ RSpec.describe TopicsController do
 
         expect(response.status).to eq(200)
         expect(topic.reload.visible).to eq(true)
+        expect(topic.reload.visibility_reason_id).to eq(
+          Topic.visibility_reasons[:manually_relisted],
+        )
         expect(topic.posts.last.action_code).to eq("visible.enabled")
       end
     end

spec/services/topic_status_updater_spec.rb

@@ -204,5 +204,24 @@ RSpec.describe TopicStatusUpdater do
       expect(updated).to eq(false)
       expect(topic.posts.where(post_type: Post.types[:small_action]).count).to eq(2)
     end
+
+    it "sets visibility_reason_id" do
+      topic = Fabricate(:topic)
+
+      updated = TopicStatusUpdater.new(topic, admin).update!("visible", false)
+      expect(updated).to eq(true)
+      expect(topic.visible).to eq(false)
+      expect(topic.visibility_reason_id).to eq(Topic.visibility_reasons[:unknown])
+
+      updated =
+        TopicStatusUpdater.new(topic, admin).update!(
+          "visible",
+          true,
+          { visibility_reason_id: Topic.visibility_reasons[:manually_relisted] },
+        )
+      expect(updated).to eq(true)
+      expect(topic.visible).to eq(true)
+      expect(topic.visibility_reason_id).to eq(Topic.visibility_reasons[:manually_relisted])
+    end
   end
 end