From 29a8c6ee498999f114f933d2de2b46989a407660 Mon Sep 17 00:00:00 2001 From: Gary Pendergast Date: Thu, 20 Feb 2025 09:09:47 +1100 Subject: [PATCH] DEV: Add a new `type_source` field to the `Reviewable` model. (#31325) This change adds a new `type_source` field to the `Reviewable` model, indicating whether the Reviewable type was registered by `core`, a plugin, or an `unknown` source. When a plugin that registered a Reviewable type is disabled, this allows us to tell the user which plugin they need to re-enable to handle any orphan reviewable items. --- .../discourse/app/controllers/review-index.js | 2 + .../discourse/app/lib/constants.js | 2 + .../discourse/app/routes/review-index.js | 2 +- .../discourse/app/templates/review-index.hbs | 15 +++- .../fixtures/concerns/notification-types.js | 1 + app/controllers/reviewables_controller.rb | 2 +- app/models/reviewable.rb | 29 ++++++- app/models/reviewable_flagged_post.rb | 1 + app/models/reviewable_post.rb | 1 + app/models/reviewable_queued_post.rb | 1 + app/models/reviewable_user.rb | 1 + app/serializers/reviewable_serializer.rb | 1 + config/locales/client.en.yml | 2 + ...212045125_add_type_source_to_reviewable.rb | 27 ++++++ lib/discourse_plugin_registry.rb | 10 +++ lib/tasks/javascript.rake | 2 + spec/lib/discourse_plugin_registry_spec.rb | 15 ++++ spec/lib/plugin/instance_spec.rb | 11 ++- spec/models/reviewable_spec.rb | 83 +++++++++++++++++++ .../serializers/reviewable_serializer_spec.rb | 1 + spec/system/page_objects/pages/review.rb | 20 +++++ spec/system/reviewables_spec.rb | 11 ++- 22 files changed, 232 insertions(+), 8 deletions(-) create mode 100644 db/migrate/20250212045125_add_type_source_to_reviewable.rb diff --git a/app/assets/javascripts/discourse/app/controllers/review-index.js b/app/assets/javascripts/discourse/app/controllers/review-index.js index b5bf3596ff6..ab7b16928c7 100644 --- a/app/assets/javascripts/discourse/app/controllers/review-index.js +++ b/app/assets/javascripts/discourse/app/controllers/review-index.js @@ -6,6 +6,7 @@ import { underscore } from "@ember/string"; import { isPresent } from "@ember/utils"; import { ajax } from "discourse/lib/ajax"; import { popupAjaxError } from "discourse/lib/ajax-error"; +import { REVIEWABLE_UNKNOWN_TYPE_SOURCE } from "discourse/lib/constants"; import discourseComputed from "discourse/lib/decorators"; import { i18n } from "discourse-i18n"; @@ -44,6 +45,7 @@ export default class ReviewIndexController extends Controller { sort_order = null; additional_filters = null; filterScoreType = null; + unknownTypeSource = REVIEWABLE_UNKNOWN_TYPE_SOURCE; @discourseComputed("reviewableTypes") allTypes() { diff --git a/app/assets/javascripts/discourse/app/lib/constants.js b/app/assets/javascripts/discourse/app/lib/constants.js index 0aff2a48b0c..bdf1e8992e1 100644 --- a/app/assets/javascripts/discourse/app/lib/constants.js +++ b/app/assets/javascripts/discourse/app/lib/constants.js @@ -117,3 +117,5 @@ export const REPORT_MODES = { inline_table: "inline_table", storage_stats: "storage_stats", }; + +export const REVIEWABLE_UNKNOWN_TYPE_SOURCE = "unknown"; diff --git a/app/assets/javascripts/discourse/app/routes/review-index.js b/app/assets/javascripts/discourse/app/routes/review-index.js index f68afdd846f..8c3a43c9c97 100644 --- a/app/assets/javascripts/discourse/app/routes/review-index.js +++ b/app/assets/javascripts/discourse/app/routes/review-index.js @@ -39,7 +39,7 @@ export default class ReviewIndex extends DiscourseRoute { filterCategoryId: meta.category_id, filterPriority: meta.priority, reviewableTypes: meta.reviewable_types, - unknownReviewableTypes: meta.unknown_reviewable_types, + unknownReviewableTypes: meta.unknown_reviewable_types_and_sources, scoreTypes: meta.score_types, filterUsername: meta.username, filterReviewedBy: meta.reviewed_by, diff --git a/app/assets/javascripts/discourse/app/templates/review-index.hbs b/app/assets/javascripts/discourse/app/templates/review-index.hbs index cb685972c47..939e4f93344 100644 --- a/app/assets/javascripts/discourse/app/templates/review-index.hbs +++ b/app/assets/javascripts/discourse/app/templates/review-index.hbs @@ -6,8 +6,19 @@ }} {{htmlSafe diff --git a/app/assets/javascripts/discourse/tests/fixtures/concerns/notification-types.js b/app/assets/javascripts/discourse/tests/fixtures/concerns/notification-types.js index 83f912464ae..b328c544708 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/concerns/notification-types.js +++ b/app/assets/javascripts/discourse/tests/fixtures/concerns/notification-types.js @@ -42,6 +42,7 @@ export const NOTIFICATION_TYPES = { new_features: 37, admin_problems: 38, linked_consolidated: 39, + chat_watched_thread: 40, following: 800, following_created_topic: 801, following_replied: 802, diff --git a/app/controllers/reviewables_controller.rb b/app/controllers/reviewables_controller.rb index f54e2346e27..913eb60ae39 100644 --- a/app/controllers/reviewables_controller.rb +++ b/app/controllers/reviewables_controller.rb @@ -74,7 +74,7 @@ class ReviewablesController < ApplicationController total_rows_reviewables: total_rows, types: meta_types, reviewable_types: Reviewable.types, - unknown_reviewable_types: Reviewable.unknown_types, + unknown_reviewable_types_and_sources: Reviewable.unknown_types_and_sources, score_types: ReviewableScore .types diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index 34f6e1d7c87..054c12cdeb9 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -7,6 +7,8 @@ class Reviewable < ActiveRecord::Base ReviewableUser: BasicReviewableUserSerializer, } + UNKNOWN_TYPE_SOURCE = "unknown" + self.ignored_columns = [:reviewable_by_group_id] class UpdateConflict < StandardError @@ -42,6 +44,8 @@ class Reviewable < ActiveRecord::Base validates :reject_reason, length: { maximum: 2000 } + before_save :set_type_source + after_create { log_history(:created, created_by) } after_commit(on: :create) { DiscourseEvent.trigger(:reviewable_created, self) } @@ -77,6 +81,16 @@ class Reviewable < ActiveRecord::Base self.types.map(&:sti_name) end + def self.source_for(type) + type = type.sti_name if type.is_a?(Class) + return UNKNOWN_TYPE_SOURCE if Reviewable.sti_names.exclude?(type) + + DiscoursePluginRegistry + .reviewable_types_lookup + .find { |r| r[:klass].sti_name == type } + &.dig(:plugin) || "core" + end + def self.custom_filters @reviewable_filters ||= [] end @@ -89,6 +103,10 @@ class Reviewable < ActiveRecord::Base @reviewable_filters = [] end + def set_type_source + self.type_source = Reviewable.source_for(type) + end + def created_new! self.created_new = true self.topic = target.topic if topic.blank? && target.is_a?(Post) @@ -766,8 +784,14 @@ class Reviewable < ActiveRecord::Base ) end - def self.unknown_types - Reviewable.pending.distinct.pluck(:type) - Reviewable.sti_names + def self.unknown_types_and_sources + @known_sources ||= Reviewable.sti_names.map { |n| [n, Reviewable.source_for(n)] } + + known_unknowns = Reviewable.pending.distinct.pluck(:type, :type_source) - @known_sources + + known_unknowns + .map { |type, source| { type: type, source: source } } + .sort_by { |e| [e[:source] == UNKNOWN_TYPE_SOURCE ? 1 : 0, e[:source], e[:type]] } end def self.destroy_unknown_types! @@ -804,6 +828,7 @@ end # # id :bigint not null, primary key # type :string not null +# type_source :string default("unknown"), not null # status :integer default("pending"), not null # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null diff --git a/app/models/reviewable_flagged_post.rb b/app/models/reviewable_flagged_post.rb index 7e7a334a405..7d324b9a6da 100644 --- a/app/models/reviewable_flagged_post.rb +++ b/app/models/reviewable_flagged_post.rb @@ -399,6 +399,7 @@ end # # id :bigint not null, primary key # type :string not null +# type_source :string default("unknown"), not null # status :integer default("pending"), not null # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null diff --git a/app/models/reviewable_post.rb b/app/models/reviewable_post.rb index d72358e9c93..4d727b9e54c 100644 --- a/app/models/reviewable_post.rb +++ b/app/models/reviewable_post.rb @@ -142,6 +142,7 @@ end # # id :bigint not null, primary key # type :string not null +# type_source :string default("unknown"), not null # status :integer default("pending"), not null # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null diff --git a/app/models/reviewable_queued_post.rb b/app/models/reviewable_queued_post.rb index 342508912fe..13c6149b810 100644 --- a/app/models/reviewable_queued_post.rb +++ b/app/models/reviewable_queued_post.rb @@ -240,6 +240,7 @@ end # # id :bigint not null, primary key # type :string not null +# type_source :string default("unknown"), not null # status :integer default("pending"), not null # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index c7e2429ac76..4087c6c5d80 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -102,6 +102,7 @@ end # # id :bigint not null, primary key # type :string not null +# type_source :string default("unknown"), not null # status :integer default("pending"), not null # created_by_id :integer not null # reviewable_by_moderator :boolean default(FALSE), not null diff --git a/app/serializers/reviewable_serializer.rb b/app/serializers/reviewable_serializer.rb index a454cbd89e5..d4e5482b134 100644 --- a/app/serializers/reviewable_serializer.rb +++ b/app/serializers/reviewable_serializer.rb @@ -6,6 +6,7 @@ class ReviewableSerializer < ApplicationSerializer attributes( :id, :type, + :type_source, :topic_id, :topic_url, :target_url, diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 9d2cd54cf22..4e45d1fef0c 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -589,6 +589,8 @@ en: one: "You have pending reviewables from disabled plugin:" other: "You have pending reviewables from disabled plugins:" instruction: "They cannot be properly displayed until you enable the relevant plugin. Please enable the plugin and refresh the page. Alternatively, you can ignore them. Learn more..." + reviewable_unknown_source: "%{reviewableType} (unknown plugin)" + reviewable_known_source: "%{reviewableType} (from the '%{pluginName}' plugin)" ignore_all: "Ignore all" enable_plugins: "Enable plugins" delete_confirm: "Are you sure you want to delete all reviews created by disabled plugins?" diff --git a/db/migrate/20250212045125_add_type_source_to_reviewable.rb b/db/migrate/20250212045125_add_type_source_to_reviewable.rb new file mode 100644 index 00000000000..2be939f5e08 --- /dev/null +++ b/db/migrate/20250212045125_add_type_source_to_reviewable.rb @@ -0,0 +1,27 @@ +# frozen_string_literal: true +class AddTypeSourceToReviewable < ActiveRecord::Migration[7.2] + def change + add_column :reviewables, :type_source, :string, null: false, default: "unknown" + + # Migrate known reviewables to have a type_source + known_reviewables = { + "chat" => %w[ReviewableChatMessage], + "core" => %w[ReviewableFlaggedPost ReviewableQueuedPost ReviewableUser ReviewablePost], + "discourse-ai" => %w[ReviewableAiChatMessage ReviewableAiPost], + "discourse-akismet" => %w[ + ReviewableAkismetPost + ReviewableAkismetPostVotingComment + ReviewableAkismetUser + ], + "discourse-antivirus" => %w[ReviewableUpload], + "discourse-category-experts" => %w[ReviewableCategoryExpertSuggestion], + "discourse-post-voting" => %w[ReviewablePostVotingComment], + } + + known_reviewables.each do |plugin, types| + types.each do |type| + Reviewable.where(type: type, type_source: "unknown").update_all(type_source: plugin) + end + end + end +end diff --git a/lib/discourse_plugin_registry.rb b/lib/discourse_plugin_registry.rb index 042599fe541..7c8fcd44d20 100644 --- a/lib/discourse_plugin_registry.rb +++ b/lib/discourse_plugin_registry.rb @@ -50,6 +50,8 @@ class DiscoursePluginRegistry define_singleton_method("register_#{register_name.to_s.singularize}") do |value, plugin| public_send(:"_raw_#{register_name}") << { plugin: plugin, value: value } end + + yield(self) if block_given? end define_register :javascripts, Set @@ -129,6 +131,14 @@ class DiscoursePluginRegistry define_filtered_register :custom_filter_mappings + define_filtered_register :reviewable_types do |singleton| + singleton.define_singleton_method("reviewable_types_lookup") do + public_send(:"_raw_reviewable_types") + .filter_map { |h| { plugin: h[:plugin].name, klass: h[:value] } if h[:plugin].enabled? } + .uniq + end + end + def self.register_auth_provider(auth_provider) self.auth_providers << auth_provider end diff --git a/lib/tasks/javascript.rake b/lib/tasks/javascript.rake index 180cea69d61..1c99bf8788a 100644 --- a/lib/tasks/javascript.rake +++ b/lib/tasks/javascript.rake @@ -169,6 +169,8 @@ task "javascript:update_constants" => :environment do export const USER_FIELD_FLAGS = #{UserField::FLAG_ATTRIBUTES}; export const REPORT_MODES = #{Report::MODES.to_json}; + + export const REVIEWABLE_UNKNOWN_TYPE_SOURCE = "#{Reviewable::UNKNOWN_TYPE_SOURCE}"; JS pretty_notifications = Notification.types.map { |n| " #{n[0]}: #{n[1]}," }.join("\n") diff --git a/spec/lib/discourse_plugin_registry_spec.rb b/spec/lib/discourse_plugin_registry_spec.rb index f25b05cb195..d0799f76bd2 100644 --- a/spec/lib/discourse_plugin_registry_spec.rb +++ b/spec/lib/discourse_plugin_registry_spec.rb @@ -15,6 +15,7 @@ RSpec.describe DiscoursePluginRegistry do let(:plugin_class) do Class.new(Plugin::Instance) do attr_accessor :enabled + attr_accessor :name def enabled? @enabled end @@ -52,6 +53,20 @@ RSpec.describe DiscoursePluginRegistry do plugin.enabled = false expect(fresh_registry.test_things.length).to eq(0) end + + it "runs the callback block" do + fresh_registry.define_filtered_register(:test_other_things) do |singleton| + singleton.define_singleton_method(:my_fun_method) { true } + end + + fresh_registry.register_test_other_thing("mything", plugin) + + plugin.enabled = true + expect(fresh_registry.test_other_things).to contain_exactly("mything") + + expect(fresh_registry.methods.include?(:my_fun_method)).to eq(true) + expect(fresh_registry.my_fun_method).to eq(true) + end end end diff --git a/spec/lib/plugin/instance_spec.rb b/spec/lib/plugin/instance_spec.rb index 02d64d8d6ba..67ccbd4ea61 100644 --- a/spec/lib/plugin/instance_spec.rb +++ b/spec/lib/plugin/instance_spec.rb @@ -625,13 +625,22 @@ TEXT subject(:register_reviewable_type) { plugin_instance.register_reviewable_type(new_type) } context "when the provided class inherits from `Reviewable`" do - let(:new_type) { Class.new(Reviewable) } + let(:new_type) do + class MyReviewable < Reviewable + end + MyReviewable + end it "adds the provided class to the existing types" do expect { register_reviewable_type }.to change { Reviewable.types.size }.by(1) expect(Reviewable.types).to include(new_type) end + it "shows the correct source for the new type" do + register_reviewable_type + expect(Reviewable.source_for(new_type)).to eq("discourse-sample-plugin") + end + context "when the plugin is disabled" do before do register_reviewable_type diff --git a/spec/models/reviewable_spec.rb b/spec/models/reviewable_spec.rb index f6b22faf8a7..026054e86b6 100644 --- a/spec/models/reviewable_spec.rb +++ b/spec/models/reviewable_spec.rb @@ -341,6 +341,89 @@ RSpec.describe Reviewable, type: :model do expect(Reviewable.valid_type?("User")).to eq(false) end + describe ".source_for" do + it "returns the correct source" do + expect(Reviewable.source_for(ReviewablePost)).to eq("core") + expect(Reviewable.source_for(ReviewableFlaggedPost)).to eq("core") + expect(Reviewable.source_for(ReviewableQueuedPost)).to eq("core") + expect(Reviewable.source_for(ReviewableUser)).to eq("core") + expect(Reviewable.source_for("NonExistentType")).to eq("unknown") + end + end + + describe ".unknown_types_and_sources" do + it "returns an empty array when no unknown types are present" do + expect(Reviewable.unknown_types_and_sources).to eq([]) + end + + context "with reviewables of unknown type or sources" do + fab!(:core_type) do + type = Fabricate(:reviewable) + type.update_columns(type: "ReviewableDoesntExist", type_source: "core") + type + end + + fab!(:known_core_type) do + type = Fabricate(:reviewable) + type.update_columns(type: "ReviewableFlaggedPost", type_source: "core") + type + end + + fab!(:unknown_type) do + type = Fabricate(:reviewable) + type.update_columns(type: "UnknownType", type_source: "unknown") + type + end + + fab!(:plugin_type) do + type = Fabricate(:reviewable) + type.update_columns(type: "PluginReviewableDoesntExist", type_source: "my-plugin") + type + end + + fab!(:plugin_type2) do + type = Fabricate(:reviewable) + type.update_columns(type: "PluginReviewableStillDoesntExist", type_source: "my-plugin") + type + end + + fab!(:plugin_type3) do + type = Fabricate(:reviewable) + type.update_columns( + type: "AnotherPluginReviewableDoesntExist", + type_source: "another-plugin", + ) + type + end + + fab!(:plugin_type4) do + type = Fabricate(:reviewable) + type.update_columns(type: "ThisIsGettingSilly", type_source: "zzz-last-plugin") + type + end + + fab!(:unknown_type2) do + type = Fabricate(:reviewable) + type.update_columns(type: "AnotherUnknownType", type_source: "unknown") + type + end + + it "returns an array of unknown types, sorted by source (with 'unknown' always last), then by type" do + expect(Reviewable.unknown_types_and_sources).to eq( + [ + { type: "AnotherPluginReviewableDoesntExist", source: "another-plugin" }, + { type: "ReviewableDoesntExist", source: "core" }, + { type: "PluginReviewableDoesntExist", source: "my-plugin" }, + { type: "PluginReviewableStillDoesntExist", source: "my-plugin" }, + { type: "ThisIsGettingSilly", source: "zzz-last-plugin" }, + { type: "AnotherUnknownType", source: "unknown" }, + { type: "UnknownType", source: "unknown" }, + ], + ) + end + end + end + describe "events" do let!(:moderator) { Fabricate(:moderator) } let(:reviewable) { Fabricate(:reviewable) } diff --git a/spec/serializers/reviewable_serializer_spec.rb b/spec/serializers/reviewable_serializer_spec.rb index 7e064ba635b..b9088afd012 100644 --- a/spec/serializers/reviewable_serializer_spec.rb +++ b/spec/serializers/reviewable_serializer_spec.rb @@ -10,6 +10,7 @@ RSpec.describe ReviewableSerializer do expect(json[:id]).to eq(reviewable.id) expect(json[:status]).to eq(reviewable.status_for_database) expect(json[:type]).to eq(reviewable.type) + expect(json[:type_source]).to eq(reviewable.type_source) expect(json[:created_at]).to eq(reviewable.created_at) expect(json[:category_id]).to eq(reviewable.category_id) expect(json[:can_edit]).to eq(true) diff --git a/spec/system/page_objects/pages/review.rb b/spec/system/page_objects/pages/review.rb index d604ef11499..8046041ff11 100644 --- a/spec/system/page_objects/pages/review.rb +++ b/spec/system/page_objects/pages/review.rb @@ -92,6 +92,26 @@ module PageObjects page.has_no_css?(".unknown-reviewables") end + def has_listing_for_unknown_reviewables_plugin?(reviewable_type, plugin_name) + page.has_css?( + ".unknown-reviewables ul li", + text: + I18n.t( + "js.review.unknown.reviewable_known_source", + reviewableType: reviewable_type, + pluginName: plugin_name, + ), + ) + end + + def has_listing_for_unknown_reviewables_unknown_source?(reviewable_type) + page.has_css?( + ".unknown-reviewables ul li", + text: + I18n.t("js.review.unknown.reviewable_unknown_source", reviewableType: reviewable_type), + ) + end + private def reviewable_action_dropdown diff --git a/spec/system/reviewables_spec.rb b/spec/system/reviewables_spec.rb index 3926fb20cd9..f2995b416bd 100644 --- a/spec/system/reviewables_spec.rb +++ b/spec/system/reviewables_spec.rb @@ -216,12 +216,21 @@ describe "Reviewables", type: :system do describe "when there is an unknown plugin reviewable" do fab!(:reviewable) { Fabricate(:reviewable_flagged_post, target: long_post) } + fab!(:reviewable2) { Fabricate(:reviewable) } - before { reviewable.update_column(:type, "UnknownPlugin") } + before do + reviewable.update_columns(type: "UnknownPlugin", type_source: "some-plugin") + reviewable2.update_columns(type: "UnknownSource", type_source: "unknown") + end it "informs admin and allows to delete them" do visit("/review") expect(review_page).to have_information_about_unknown_reviewables_visible + expect(review_page).to have_listing_for_unknown_reviewables_plugin( + reviewable.type, + reviewable.type_source, + ) + expect(review_page).to have_listing_for_unknown_reviewables_unknown_source(reviewable2.type) review_page.click_ignore_all_unknown_reviewables expect(review_page).to have_no_information_about_unknown_reviewables_visible end