From edec941a8790b3a2a5f0f5a825d7329577cc15dc Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 29 Apr 2024 10:34:46 +1000 Subject: [PATCH] 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 --- .../discourse/app/components/topic-status.js | 12 ++++- .../discourse/app/lib/constants.js | 10 ++++ .../javascripts/discourse/app/models/topic.js | 16 +++++++ .../discourse/app/raw-views/topic-status.js | 12 ++++- app/controllers/topics_controller.rb | 16 +++++-- .../regular/make_embedded_topic_visible.rb | 7 ++- app/models/post.rb | 46 ++++++++++++++++--- app/models/topic.rb | 14 ++++++ app/serializers/listable_topic_serializer.rb | 7 ++- app/serializers/topic_view_serializer.rb | 5 ++ app/services/topic_status_updater.rb | 6 +++ config/locales/client.en.yml | 9 +++- ...3808_add_visibility_reason_id_to_topics.rb | 7 +++ lib/post_action_creator.rb | 9 +++- lib/tasks/javascript.rake | 2 + lib/topics_bulk_action.rb | 14 +++++- spec/models/post_action_spec.rb | 12 ++++- spec/models/post_spec.rb | 19 ++++++++ spec/requests/topics_controller_spec.rb | 6 +++ spec/services/topic_status_updater_spec.rb | 19 ++++++++ 20 files changed, 229 insertions(+), 19 deletions(-) create mode 100644 db/migrate/20240423013808_add_visibility_reason_id_to_topics.rb diff --git a/app/assets/javascripts/discourse/app/components/topic-status.js b/app/assets/javascripts/discourse/app/components/topic-status.js index e3cbe00f957..74438ed5e5f 100644 --- a/app/assets/javascripts/discourse/app/components/topic-status.js +++ b/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; }, diff --git a/app/assets/javascripts/discourse/app/lib/constants.js b/app/assets/javascripts/discourse/app/lib/constants.js index 6b447258089..dd0830974ab 100644 --- a/app/assets/javascripts/discourse/app/lib/constants.js +++ b/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, +}; diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js index 4e735e86a23..3bf788d3e70 100644 --- a/app/assets/javascripts/discourse/app/models/topic.js +++ b/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 }; diff --git a/app/assets/javascripts/discourse/app/raw-views/topic-status.js b/app/assets/javascripts/discourse/app/raw-views/topic-status.js index b4b025a2663..e432854fd4c 100644 --- a/app/assets/javascripts/discourse/app/raw-views/topic-status.js +++ b/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") diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index c64e36c15b0..a28c7726f2b 100644 --- a/app/controllers/topics_controller.rb +++ b/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, until: params[:until]) + @topic.update_status(status, enabled, current_user, status_opts) render json: success_json.merge!( diff --git a/app/jobs/regular/make_embedded_topic_visible.rb b/app/jobs/regular/make_embedded_topic_visible.rb index 0d8e90a6185..ee3031b92d5 100644 --- a/app/jobs/regular/make_embedded_topic_visible.rb +++ b/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 diff --git a/app/models/post.rb b/app/models/post.rb index 7445c8de9c0..0d103d15e73 100644 --- a/app/models/post.rb +++ b/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( - "id = :topic_id AND NOT EXISTS(SELECT 1 FROM POSTS WHERE topic_id = :topic_id AND NOT hidden)", - topic_id: topic_id, - ).update_all(visible: false) + any_visible_posts_in_topic = + Post.exists?(topic_id: topic_id, hidden: false, post_type: Post.types[:regular]) - 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? - UserStatCountUpdater.increment!(self) + should_update_user_stat = true + + # 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 diff --git a/app/models/topic.rb b/app/models/topic.rb index 053380d95f1..233a9aed103 100644 --- a/app/models/topic.rb +++ b/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 # diff --git a/app/serializers/listable_topic_serializer.rb b/app/serializers/listable_topic_serializer.rb index de117bc1026..8dfd5fb3d79 100644 --- a/app/serializers/listable_topic_serializer.rb +++ b/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 diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index f23f87cba19..3e9fa319de7 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/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 diff --git a/app/services/topic_status_updater.rb b/app/services/topic_status_updater.rb index 04998dfe40c..0a7f02fb63c 100644 --- a/app/services/topic_status_updater.rb +++ b/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]) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 4af28846ad5..729aff4c311 100644 --- a/config/locales/client.en.yml +++ b/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" diff --git a/db/migrate/20240423013808_add_visibility_reason_id_to_topics.rb b/db/migrate/20240423013808_add_visibility_reason_id_to_topics.rb new file mode 100644 index 00000000000..6f74d9002ea --- /dev/null +++ b/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 diff --git a/lib/post_action_creator.rb b/lib/post_action_creator.rb index df36e57af43..1c905e2285c 100644 --- a/lib/post_action_creator.rb +++ b/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, diff --git a/lib/tasks/javascript.rake b/lib/tasks/javascript.rake index 1acbb36d20b..5b077fb661e 100644 --- a/lib/tasks/javascript.rake +++ b/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") diff --git a/lib/topics_bulk_action.rb b/lib/topics_bulk_action.rb index 97f02666974..20174c0d200 100644 --- a/lib/topics_bulk_action.rb +++ b/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 diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index ad311caa914..b1b7bafcd74 100644 --- a/spec/models/post_action_spec.rb +++ b/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 diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index fe9648aa5d5..bd82b76d990 100644 --- a/spec/models/post_spec.rb +++ b/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 diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 27e92128b73..9c1c3d1bc2b 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/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 diff --git a/spec/services/topic_status_updater_spec.rb b/spec/services/topic_status_updater_spec.rb index 6117e873682..b1f5cf3f9d9 100644 --- a/spec/services/topic_status_updater_spec.rb +++ b/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