From 5eb7d6d9c026f9790be0fe32e7a3e15fbee3b5c2 Mon Sep 17 00:00:00 2001 From: Krzysztof Kotlarek Date: Wed, 5 Feb 2025 14:38:45 +1100 Subject: [PATCH] FEATURE: Gracefully handle unhandled reviewables (#31118) Plugins like for example AI or Akismet create reviewable items. When the plugin is disabled, then we cannot properly handle those items. In that situation, we should display warnings about unhandled types. Instruct admin to reenable plugins. In addition, we should allow the admin to delete all pending reviews from disabled plugins. --- .../discourse/app/controllers/review-index.js | 31 ++++++++++++++++ .../discourse/app/routes/review-index.js | 1 + .../discourse/app/templates/review-index.hbs | 31 ++++++++++++++++ .../stylesheets/common/base/reviewables.scss | 4 ++ .../admin/unknown_reviewables_controller.rb | 8 ++++ app/controllers/reviewables_controller.rb | 1 + app/models/reviewable.rb | 13 +++++++ config/locales/client.en.yml | 9 +++++ config/routes.rb | 2 + spec/models/reviewable_spec.rb | 9 ++++- .../unknown_reviewables_controller_spec.rb | 37 +++++++++++++++++++ spec/system/page_objects/pages/review.rb | 13 +++++++ spec/system/reviewables_spec.rb | 21 +++++++++++ 13 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 app/controllers/admin/unknown_reviewables_controller.rb create mode 100644 spec/requests/admin/unknown_reviewables_controller_spec.rb diff --git a/app/assets/javascripts/discourse/app/controllers/review-index.js b/app/assets/javascripts/discourse/app/controllers/review-index.js index 0bd293905a4..b5bf3596ff6 100644 --- a/app/assets/javascripts/discourse/app/controllers/review-index.js +++ b/app/assets/javascripts/discourse/app/controllers/review-index.js @@ -1,12 +1,18 @@ import Controller from "@ember/controller"; import { action } from "@ember/object"; import { next } from "@ember/runloop"; +import { service } from "@ember/service"; import { underscore } from "@ember/string"; import { isPresent } from "@ember/utils"; +import { ajax } from "discourse/lib/ajax"; +import { popupAjaxError } from "discourse/lib/ajax-error"; import discourseComputed from "discourse/lib/decorators"; import { i18n } from "discourse-i18n"; export default class ReviewIndexController extends Controller { + @service currentUser; + @service dialog; + @service toasts; queryParams = [ "priority", "type", @@ -106,6 +112,11 @@ export default class ReviewIndexController extends Controller { next(() => this.send("refreshRoute")); } + @discourseComputed("unknownReviewableTypes") + displayUnknownReviewableTypesWarning(unknownReviewableTypes) { + return unknownReviewableTypes?.length > 0 && this.currentUser.admin; + } + @action remove(ids) { if (!ids) { @@ -129,6 +140,26 @@ export default class ReviewIndexController extends Controller { this.refreshModel(); } + @action + ignoreAllUnknownTypes() { + return this.dialog.deleteConfirm({ + message: i18n("review.unknown.delete_confirm"), + didConfirm: async () => { + try { + await ajax("/admin/unknown_reviewables/destroy", { + type: "delete", + }); + this.set("unknownReviewableTypes", []); + this.toasts.success({ + data: { message: i18n("review.unknown.ignore_success") }, + }); + } catch (e) { + popupAjaxError(e); + } + }, + }); + } + @action refresh() { const currentStatus = this.status; diff --git a/app/assets/javascripts/discourse/app/routes/review-index.js b/app/assets/javascripts/discourse/app/routes/review-index.js index 76633080eb9..f68afdd846f 100644 --- a/app/assets/javascripts/discourse/app/routes/review-index.js +++ b/app/assets/javascripts/discourse/app/routes/review-index.js @@ -39,6 +39,7 @@ export default class ReviewIndex extends DiscourseRoute { filterCategoryId: meta.category_id, filterPriority: meta.priority, reviewableTypes: meta.reviewable_types, + unknownReviewableTypes: meta.unknown_reviewable_types, 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 64f542a590e..cb685972c47 100644 --- a/app/assets/javascripts/discourse/app/templates/review-index.hbs +++ b/app/assets/javascripts/discourse/app/templates/review-index.hbs @@ -1,3 +1,34 @@ +{{#if this.displayUnknownReviewableTypesWarning}} +
+ {{i18n + "review.unknown.title" + count=this.unknownReviewableTypes.length + }} + + + {{htmlSafe + (i18n + "review.unknown.instruction" url="https://meta.discourse.org/t/350179" + ) + }} +
+ + {{d-icon "puzzle-piece"}} + {{i18n "review.unknown.enable_plugins"}} + + +
+
+{{/if}}
{{#if this.reviewables}} diff --git a/app/assets/stylesheets/common/base/reviewables.scss b/app/assets/stylesheets/common/base/reviewables.scss index ed9b457416b..e23d3b0b63b 100644 --- a/app/assets/stylesheets/common/base/reviewables.scss +++ b/app/assets/stylesheets/common/base/reviewables.scss @@ -639,3 +639,7 @@ margin-bottom: 0.5em; } } + +.unknown-reviewables__options { + margin-top: 1em; +} diff --git a/app/controllers/admin/unknown_reviewables_controller.rb b/app/controllers/admin/unknown_reviewables_controller.rb new file mode 100644 index 00000000000..9778ac18027 --- /dev/null +++ b/app/controllers/admin/unknown_reviewables_controller.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class Admin::UnknownReviewablesController < Admin::AdminController + def destroy + Reviewable.destroy_unknown_types! + render json: success_json + end +end diff --git a/app/controllers/reviewables_controller.rb b/app/controllers/reviewables_controller.rb index 34fb5ccd11e..f54e2346e27 100644 --- a/app/controllers/reviewables_controller.rb +++ b/app/controllers/reviewables_controller.rb @@ -74,6 +74,7 @@ class ReviewablesController < ApplicationController total_rows_reviewables: total_rows, types: meta_types, reviewable_types: Reviewable.types, + unknown_reviewable_types: Reviewable.unknown_types, score_types: ReviewableScore .types diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index f9a68e68dd3..34f6e1d7c87 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -73,6 +73,10 @@ class Reviewable < ActiveRecord::Base [ReviewableFlaggedPost, ReviewableQueuedPost, ReviewableUser, ReviewablePost] end + def self.sti_names + self.types.map(&:sti_name) + end + def self.custom_filters @reviewable_filters ||= [] end @@ -550,6 +554,7 @@ class Reviewable < ActiveRecord::Base "LEFT JOIN reviewable_claimed_topics rct ON reviewables.topic_id = rct.topic_id", ).where("rct.user_id IS NULL OR rct.user_id = ?", user.id) end + result = result.where(type: Reviewable.sti_names) result = result.limit(limit) if limit result = result.offset(offset) if offset result @@ -761,6 +766,14 @@ class Reviewable < ActiveRecord::Base ) end + def self.unknown_types + Reviewable.pending.distinct.pluck(:type) - Reviewable.sti_names + end + + def self.destroy_unknown_types! + Reviewable.pending.where.not(type: Reviewable.sti_names).delete_all + end + private def update_flag_stats(status:, user_ids:) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index b04c8ebf778..fb8d0e5d7f1 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -584,6 +584,15 @@ en: date_filter: "Posted between" in_reply_to: "in reply to" filtered_flagged_by: "Flagged by" + unknown: + title: + 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..." + ignore_all: "Ignore all" + enable_plugins: "Enable plugins" + delete_confirm: "Are you sure you want to delete all reviews created by disabled plugins?" + ignore_success: "All reviews created by disabled plugins have been deleted." explain: why: "explain why this item ended up in the queue" title: "Reviewable Scoring" diff --git a/config/routes.rb b/config/routes.rb index 767b9c20f17..dec00caed17 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -449,6 +449,8 @@ Discourse::Application.routes.draw do get "section/:section_id" => "section#show", :constraints => AdminConstraint.new resources :admin_notices, only: %i[destroy], constraints: AdminConstraint.new + + delete "unknown_reviewables/destroy" => "unknown_reviewables#destroy" end # admin namespace get "email/unsubscribe/:key" => "email#unsubscribe", :as => "email_unsubscribe" diff --git a/spec/models/reviewable_spec.rb b/spec/models/reviewable_spec.rb index 387e4f65115..e79e2c8b7a6 100644 --- a/spec/models/reviewable_spec.rb +++ b/spec/models/reviewable_spec.rb @@ -188,13 +188,20 @@ RSpec.describe Reviewable, type: :model do expect(reviewables).to contain_exactly(reviewable) end - it "Does not filter by status when status parameter is set to all" do + it "does not filter by status when status parameter is set to all" do rejected_reviewable = Fabricate(:reviewable, target: post, status: Reviewable.statuses[:rejected]) reviewables = Reviewable.list_for(user, status: :all) expect(reviewables).to match_array [reviewable, rejected_reviewable] end + it "does not include reviewables of unknown type" do + unknown_reviewable = Fabricate(:reviewable, target: post) + unknown_reviewable.update_column(:type, "UnknownPlugin") + reviewables = Reviewable.list_for(user, status: :all) + expect(reviewables).to match_array [reviewable] + end + it "supports sorting" do r0 = Fabricate(:reviewable, score: 100, created_at: 3.months.ago) r1 = Fabricate(:reviewable, score: 999, created_at: 1.month.ago) diff --git a/spec/requests/admin/unknown_reviewables_controller_spec.rb b/spec/requests/admin/unknown_reviewables_controller_spec.rb new file mode 100644 index 00000000000..7e005277e56 --- /dev/null +++ b/spec/requests/admin/unknown_reviewables_controller_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +require "rails_helper" + +RSpec.describe Admin::UnknownReviewablesController do + let(:admin) { Fabricate(:admin) } + let(:user) { Fabricate(:user) } + fab!(:reviewable) + fab!(:unknown_reviewable) { Fabricate(:reviewable, type: "ReviewablePost") } + + describe "#destroy" do + context "when user is an admin" do + before do + sign_in admin + + allow(Reviewable).to receive(:types).and_return([ReviewableUser]) + end + + it "destroys all pending reviewables of specified types" do + delete "/admin/unknown_reviewables/destroy.json" + expect(response.code).to eq("200") + + reviewable.reload + expect { unknown_reviewable.reload }.to raise_error(ActiveRecord::RecordNotFound) + end + end + + context "when user is not an admin" do + before { sign_in user } + + it "raises Discourse::InvalidAccess" do + delete "/admin/unknown_reviewables/destroy.json" + expect(response.code).to eq("404") + end + end + end +end diff --git a/spec/system/page_objects/pages/review.rb b/spec/system/page_objects/pages/review.rb index 86578428fd6..d604ef11499 100644 --- a/spec/system/page_objects/pages/review.rb +++ b/spec/system/page_objects/pages/review.rb @@ -79,6 +79,19 @@ module PageObjects page.has_no_css?("dialog-container .dialog-content") end + def click_ignore_all_unknown_reviewables + find(".unknown-reviewables__options button").click + find(".dialog-footer .btn-danger").click + end + + def has_information_about_unknown_reviewables_visible? + page.has_css?(".unknown-reviewables") + end + + def has_no_information_about_unknown_reviewables_visible? + page.has_no_css?(".unknown-reviewables") + end + private def reviewable_action_dropdown diff --git a/spec/system/reviewables_spec.rb b/spec/system/reviewables_spec.rb index af01733b90c..3926fb20cd9 100644 --- a/spec/system/reviewables_spec.rb +++ b/spec/system/reviewables_spec.rb @@ -7,6 +7,7 @@ describe "Reviewables", type: :system do fab!(:long_post) { Fabricate(:post_with_very_long_raw_content) } fab!(:post) let(:composer) { PageObjects::Components::Composer.new } + let(:moderator) { Fabricate(:moderator) } before { sign_in(admin) } @@ -212,4 +213,24 @@ describe "Reviewables", type: :system do end end end + + describe "when there is an unknown plugin reviewable" do + fab!(:reviewable) { Fabricate(:reviewable_flagged_post, target: long_post) } + + before { reviewable.update_column(:type, "UnknownPlugin") } + + it "informs admin and allows to delete them" do + visit("/review") + expect(review_page).to have_information_about_unknown_reviewables_visible + review_page.click_ignore_all_unknown_reviewables + expect(review_page).to have_no_information_about_unknown_reviewables_visible + end + + it "does not inform moderator about them" do + sign_in(moderator) + + visit("/review") + expect(review_page).to have_no_information_about_unknown_reviewables_visible + end + end end