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.

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;

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,

app/assets/javascripts/discourse/app/templates/review-index.hbs

@@ -1,3 +1,34 @@
+{{#if this.displayUnknownReviewableTypesWarning}}
+  <div class="alert alert-info unknown-reviewables">
+    <span class="text">{{i18n
+        "review.unknown.title"
+        count=this.unknownReviewableTypes.length
+      }}</span>
+
+    <ul>
+      {{#each this.unknownReviewableTypes as |type|}}
+        <li>{{type}}</li>
+      {{/each}}
+    </ul>
+    <span class="text">{{htmlSafe
+        (i18n
+          "review.unknown.instruction" url="https://meta.discourse.org/t/350179"
+        )
+      }}</span>
+    <div class="unknown-reviewables__options">
+      <LinkTo @route="adminPlugins.index" class="btn">
+        {{d-icon "puzzle-piece"}}
+        <span>{{i18n "review.unknown.enable_plugins"}}</span>
+      </LinkTo>
+      <DButton
+        @label="review.unknown.ignore_all"
+        @icon="trash-can"
+        @action={{this.ignoreAllUnknownTypes}}
+        class="btn-default"
+      />
+    </div>
+  </div>
+{{/if}}
 <div class="reviewable-container">
   <div class="reviewable-list">
     {{#if this.reviewables}}

app/assets/stylesheets/common/base/reviewables.scss

@@ -639,3 +639,7 @@
     margin-bottom: 0.5em;
   }
 }
+
+.unknown-reviewables__options {
+  margin-top: 1em;
+}

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

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

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:)

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. <a href='%{url}' target='_blank'>Learn more...</a>"
+        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"

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"

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)

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

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

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