FIX: Keep ReviewableQueuedPosts even with user delete reviewable actions (#22501)

Performing a `Delete User`/`Delete and Block User` reviewable actions for a
queued post reviewable from the `review.show` route results in an error
popup even if the action completes successfully.

This happens because unlike other reviewable types, a user delete action
on a queued post reviewable results in the deletion of the reviewable
itself. A subsequent attempt to reload the reviewable record results in
404. The deletion happens as part of the call to `UserDestroyer` which
includes a step for destroying reviewables created by the user being
destroyed. At the root of this is the creator of the queued post
being set as the creator of the reviewable as instead of the system
user.

This change assigns the creator of the reviewable to the system user and
uses the more approapriate `target_created_by` column for the creator of the
post being queued.

app/assets/javascripts/discourse/app/components/reviewable-queued-post.hbs

@@ -13,12 +13,15 @@
 </ReviewableTopicLink>
 
 <div class="post-contents-wrapper">
-  <ReviewableCreatedBy @user={{this.reviewable.created_by}} @tagName="" />
+  <ReviewableCreatedBy
+    @user={{this.reviewable.target_created_by}}
+    @tagName=""
+  />
 
   <div class="post-contents">
     <ReviewablePostHeader
       @reviewable={{this.reviewable}}
-      @createdBy={{this.reviewable.created_by}}
+      @createdBy={{this.reviewable.target_created_by}}
       @tagName=""
     />
 

app/models/reviewable.rb

@@ -87,7 +87,7 @@ class Reviewable < ActiveRecord::Base
   def created_new!
     self.created_new = true
     self.topic = target.topic if topic.blank? && target.is_a?(Post)
-    self.target_created_by_id = target.is_a?(Post) ? target.user_id : nil
+    self.target_created_by_id ||= target.is_a?(Post) ? target.user_id : nil
     self.category_id = topic.category_id if category_id.blank? && topic.present?
   end
 
@@ -349,6 +349,12 @@ class Reviewable < ActiveRecord::Base
     result
   end
 
+  # Override this in specific reviewable type to include scores for
+  # non-pending reviewables
+  def updatable_reviewable_scores
+    reviewable_scores.pending
+  end
+
   def transition_to(status_symbol, performed_by)
     self.status = status_symbol
     save!
@@ -357,7 +363,7 @@ class Reviewable < ActiveRecord::Base
     DiscourseEvent.trigger(:reviewable_transitioned_to, status_symbol, self)
 
     if score_status = ReviewableScore.score_transitions[status_symbol]
-      reviewable_scores.pending.update_all(
+      updatable_reviewable_scores.update_all(
         status: score_status,
         reviewed_by_id: performed_by.id,
         reviewed_at: Time.zone.now,
@@ -494,10 +500,11 @@ class Reviewable < ActiveRecord::Base
     end
 
     # If a reviewable doesn't have a target, allow us to filter on who created that reviewable.
+    # A ReviewableQueuedPost may have a target_created_by_id even before a target get's assigned
     if user_id
       result =
         result.where(
-          "(reviewables.target_created_by_id IS NULL AND reviewables.created_by_id = :user_id)
+          "(reviewables.target_id IS NULL AND reviewables.created_by_id = :user_id)
         OR (reviewables.target_created_by_id = :user_id)",
           user_id: user_id,
         )

app/models/reviewable_queued_post.rb

@@ -16,6 +16,12 @@ class ReviewableQueuedPost < Reviewable
 
   after_commit :compute_user_stats, only: %i[create update]
 
+  def updatable_reviewable_scores
+    # Approvals are possible for already rejected queued posts. We need the
+    # scores to be updated when this happens.
+    reviewable_scores.pending.or(reviewable_scores.disagreed)
+  end
+
   def build_actions(actions, guardian, args)
     unless approved?
       if topic&.closed?
@@ -25,9 +31,11 @@ class ReviewableQueuedPost < Reviewable
           a.confirm_message = "reviewables.actions.approve_post.confirm_closed"
         end
       else
-        actions.add(:approve_post) do |a|
-          a.icon = "check"
-          a.label = "reviewables.actions.approve_post.title"
+        if target_created_by.present?
+          actions.add(:approve_post) do |a|
+            a.icon = "check"
+            a.label = "reviewables.actions.approve_post.title"
+          end
         end
       end
     end
@@ -39,7 +47,7 @@ class ReviewableQueuedPost < Reviewable
       end
     end
 
-    delete_user_actions(actions) if pending? && guardian.can_delete_user?(created_by)
+    delete_user_actions(actions) if pending? && guardian.can_delete_user?(target_created_by)
 
     actions.add(:delete) if guardian.can_delete?(self)
   end
@@ -79,7 +87,7 @@ class ReviewableQueuedPost < Reviewable
       )
     opts.merge!(guardian: Guardian.new(performed_by)) if performed_by.staff?
 
-    creator = PostCreator.new(created_by, opts)
+    creator = PostCreator.new(target_created_by, opts)
     created_post = creator.create
 
     unless created_post && creator.errors.blank?
@@ -90,7 +98,7 @@ class ReviewableQueuedPost < Reviewable
     self.topic_id = created_post.topic_id if topic_id.nil?
     save
 
-    UserSilencer.unsilence(created_by, performed_by) if created_by.silenced?
+    UserSilencer.unsilence(target_created_by, performed_by) if target_created_by.silenced?
 
     StaffActionLogger.new(performed_by).log_post_approved(created_post) if performed_by.staff?
 
@@ -99,7 +107,7 @@ class ReviewableQueuedPost < Reviewable
 
     Notification.create!(
       notification_type: Notification.types[:post_approved],
-      user_id: created_by.id,
+      user_id: target_created_by.id,
       data: { post_url: created_post.url }.to_json,
       topic_id: created_post.topic_id,
       post_number: created_post.post_number,
@@ -148,9 +156,12 @@ class ReviewableQueuedPost < Reviewable
   private
 
   def delete_user(performed_by, delete_options)
-    reviewable_ids = Reviewable.where(created_by: created_by).pluck(:id)
-    UserDestroyer.new(performed_by).destroy(created_by, delete_options)
-    create_result(:success) { |r| r.remove_reviewable_ids = reviewable_ids }
+    reviewable_ids = Reviewable.where(created_by: target_created_by).pluck(:id)
+
+    UserDestroyer.new(performed_by).destroy(target_created_by, delete_options)
+    update_column(:target_created_by_id, nil)
+
+    create_result(:success, :rejected) { |r| r.remove_reviewable_ids += reviewable_ids }
   end
 
   def delete_opts
@@ -164,7 +175,7 @@ class ReviewableQueuedPost < Reviewable
 
   def compute_user_stats
     return unless status_changed_from_or_to_pending?
-    created_by.user_stat.update_pending_posts
+    target_created_by&.user_stat&.update_pending_posts
   end
 
   def status_changed_from_or_to_pending?

app/models/user.rb

@@ -53,7 +53,7 @@ class User < ActiveRecord::Base
   has_many :pending_posts,
            -> { merge(Reviewable.pending) },
            class_name: "ReviewableQueuedPost",
-           foreign_key: :created_by_id
+           foreign_key: :target_created_by_id
 
   has_one :user_option, dependent: :destroy
   has_one :user_avatar, dependent: :destroy

app/services/staff_action_logger.rb

@@ -796,8 +796,8 @@ class StaffActionLogger
 
     topic = reviewable.topic || Topic.with_deleted.find_by(id: reviewable.topic_id)
     topic_title = topic&.title || I18n.t("staff_action_logs.not_found")
-    username = reviewable.created_by&.username || I18n.t("staff_action_logs.unknown")
-    name = reviewable.created_by&.name || I18n.t("staff_action_logs.unknown")
+    username = reviewable.target_created_by&.username || I18n.t("staff_action_logs.unknown")
+    name = reviewable.target_created_by&.name || I18n.t("staff_action_logs.unknown")
 
     details = [
       "created_at: #{reviewable.created_at}",

db/migrate/20230708011310_fix_reviewable_queued_posts_target_created_by_id.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+class FixReviewableQueuedPostsTargetCreatedById < ActiveRecord::Migration[7.0]
+  def up
+    execute <<~SQL
+      UPDATE reviewables
+      SET target_created_by_id = created_by_id,
+          created_by_id = #{Discourse::SYSTEM_USER_ID}
+      WHERE type = 'ReviewableQueuedPost' AND target_created_by_id IS NULL
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/new_post_manager.rb

@@ -259,10 +259,11 @@ class NewPostManager
 
     reviewable =
       ReviewableQueuedPost.new(
-        created_by: @user,
+        created_by: Discourse.system_user,
         payload: payload,
         topic_id: @args[:topic_id],
         reviewable_by_moderator: true,
+        target_created_by: @user,
       )
     reviewable.payload["title"] = @args[:title] if @args[:title].present?
     reviewable.category_id = args[:category] if args[:category].present?
@@ -299,7 +300,7 @@ class NewPostManager
     result.reviewable = reviewable
     result.reason = reason if reason
     result.check_errors(errors)
-    result.pending_count = ReviewableQueuedPost.where(created_by: @user).pending.count
+    result.pending_count = ReviewableQueuedPost.where(target_created_by: @user).pending.count
     result
   end
 

spec/fabricators/reviewable_fabricator.rb

@@ -16,6 +16,7 @@ Fabricator(:reviewable_queued_post_topic, class_name: :reviewable_queued_post) d
   reviewable_by_moderator true
   type "ReviewableQueuedPost"
   created_by { Fabricate(:user) }
+  target_created_by { Fabricate(:user) }
   category
   payload do
     {
@@ -32,6 +33,7 @@ Fabricator(:reviewable_queued_post) do
   reviewable_by_moderator true
   type "ReviewableQueuedPost"
   created_by { Fabricate(:user) }
+  target_created_by { Fabricate(:user) }
   topic
   payload do
     {

spec/models/reviewable_queued_post_spec.rb

@@ -63,7 +63,7 @@ RSpec.describe ReviewableQueuedPost, type: :model do
           expect(result.created_post.topic).to eq(topic)
           expect(result.created_post.custom_fields["hello"]).to eq("world")
           expect(result.created_post_topic).to eq(topic)
-          expect(result.created_post.user).to eq(reviewable.created_by)
+          expect(result.created_post.user).to eq(reviewable.target_created_by)
           expect(reviewable.target_id).to eq(result.created_post.id)
 
           expect(Topic.count).to eq(topic_count)
@@ -71,7 +71,7 @@ RSpec.describe ReviewableQueuedPost, type: :model do
 
           notifications =
             Notification.where(
-              user: reviewable.created_by,
+              user: reviewable.target_created_by,
               notification_type: Notification.types[:post_approved],
             )
           expect(notifications).to be_present
@@ -127,15 +127,17 @@ RSpec.describe ReviewableQueuedPost, type: :model do
 
       context "with delete_user" do
         it "deletes the user and rejects the post" do
-          other_reviewable = Fabricate(:reviewable_queued_post, created_by: reviewable.created_by)
+          other_reviewable =
+            Fabricate(:reviewable_queued_post, created_by: reviewable.target_created_by)
 
           result = reviewable.perform(moderator, :delete_user)
           expect(result.success?).to eq(true)
-          expect(User.find_by(id: reviewable.created_by)).to be_blank
+          expect(User.find_by(id: reviewable.target_created_by)).to be_blank
+
           expect(result.remove_reviewable_ids).to include(reviewable.id)
           expect(result.remove_reviewable_ids).to include(other_reviewable.id)
 
-          expect(ReviewableQueuedPost.where(id: reviewable.id)).to be_blank
+          expect(ReviewableQueuedPost.where(id: reviewable.id)).to be_present
           expect(ReviewableQueuedPost.where(id: other_reviewable.id)).to be_blank
         end
       end
@@ -222,7 +224,12 @@ RSpec.describe ReviewableQueuedPost, type: :model do
   describe "Callbacks" do
     context "when creating a new pending reviewable" do
       let(:reviewable) do
-        Fabricate.build(:reviewable_queued_post_topic, category: category, created_by: user)
+        Fabricate.build(
+          :reviewable_queued_post_topic,
+          category: category,
+          created_by: moderator,
+          target_created_by: user,
+        )
       end
       let(:user) { Fabricate(:user) }
       let(:user_stats) { user.user_stat }
@@ -235,7 +242,7 @@ RSpec.describe ReviewableQueuedPost, type: :model do
 
     context "when updating an existing reviewable" do
       let!(:reviewable) { Fabricate(:reviewable_queued_post_topic, category: category) }
-      let(:user_stats) { reviewable.created_by.user_stat }
+      let(:user_stats) { reviewable.target_created_by.user_stat }
 
       context "when status changes from 'pending' to something else" do
         it "updates user stats" do

spec/models/user_spec.rb

@@ -13,7 +13,7 @@ RSpec.describe User do
 
   it do
     is_expected.to have_many(:pending_posts).class_name("ReviewableQueuedPost").with_foreign_key(
-      :created_by_id,
+      :target_created_by_id,
     )
   end
 

spec/models/user_stat_spec.rb

@@ -284,7 +284,7 @@ RSpec.describe UserStat do
     subject(:update_pending_posts) { stat.update_pending_posts }
 
     let!(:reviewable) { Fabricate(:reviewable_queued_post) }
-    let(:user) { reviewable.created_by }
+    let(:user) { reviewable.target_created_by }
     let(:stat) { user.user_stat }
 
     before do

spec/requests/posts_controller_spec.rb

@@ -1105,7 +1105,7 @@ RSpec.describe PostsController do
           user.reload
           expect(user).to be_silenced
 
-          rp = ReviewableQueuedPost.find_by(created_by: user)
+          rp = ReviewableQueuedPost.find_by(target_created_by: user)
           expect(rp.payload["typing_duration_msecs"]).to eq(100)
           expect(rp.payload["composer_open_duration_msecs"]).to eq(204)
           expect(rp.payload["reply_to_post_number"]).to eq(123)
@@ -1199,7 +1199,7 @@ RSpec.describe PostsController do
         parsed = response.parsed_body
 
         expect(parsed["action"]).to eq("enqueued")
-        reviewable = ReviewableQueuedPost.find_by(created_by: user)
+        reviewable = ReviewableQueuedPost.find_by(target_created_by: user)
         score = reviewable.reviewable_scores.first
         expect(score.reason).to eq("auto_silence_regex")
 
@@ -1222,7 +1222,7 @@ RSpec.describe PostsController do
         parsed = response.parsed_body
 
         expect(parsed["action"]).to eq("enqueued")
-        reviewable = ReviewableQueuedPost.find_by(created_by: user)
+        reviewable = ReviewableQueuedPost.find_by(target_created_by: user)
         score = reviewable.reviewable_scores.first
         expect(score.reason).to eq("auto_silence_regex")
 

spec/system/page_objects/pages/review.rb

@@ -5,6 +5,22 @@ module PageObjects
     class Review < PageObjects::Pages::Base
       POST_BODY_TOGGLE_SELECTOR = ".post-body__toggle-btn"
       POST_BODY_COLLAPSED_SELECTOR = ".post-body.is-collapsed"
+      REVIEWABLE_ACTION_DROPDOWN = ".reviewable-action-dropdown"
+
+      def visit_reviewable(reviewable)
+        page.visit("/review/#{reviewable.id}")
+        self
+      end
+
+      def select_bundled_action(reviewable, value)
+        within(reviewable_by_id(reviewable.id)) do
+          reviewable_action_dropdown.select_row_by_value(value)
+        end
+      end
+
+      def reviewable_by_id(id)
+        find(".reviewable-item[data-reviewable-id=\"#{id}\"]")
+      end
 
       def click_post_body_toggle
         find(POST_BODY_TOGGLE_SELECTOR).click
@@ -25,6 +41,33 @@ module PageObjects
       def has_no_post_body_collapsed?
         page.has_no_css?(POST_BODY_COLLAPSED_SELECTOR)
       end
+
+      def has_reviewable_action_dropdown?
+        page.has_css?(REVIEWABLE_ACTION_DROPDOWN)
+      end
+
+      def has_reviewable_with_pending_status?(reviewable)
+        within(reviewable_by_id(reviewable.id)) { page.has_css?(".status .pending") }
+      end
+
+      def has_reviewable_with_rejected_status?(reviewable)
+        within(reviewable_by_id(reviewable.id)) { page.has_css?(".status .rejected") }
+      end
+
+      def has_error_dialog_visible?
+        page.has_css?(".dialog-container .dialog-content")
+      end
+
+      def has_no_error_dialog_visible?
+        !has_error_dialog_visible?
+      end
+
+      private
+
+      def reviewable_action_dropdown
+        @reviewable_action_dropdown ||=
+          PageObjects::Components::SelectKit.new(REVIEWABLE_ACTION_DROPDOWN)
+      end
     end
   end
 end

spec/system/reviewables_spec.rb

@@ -31,4 +31,27 @@ describe "Reviewables", type: :system do
       expect(review_page).to have_no_post_body_toggle
     end
   end
+
+  context "when performing a review action from the show route" do
+    context "with a ReviewableQueuedPost" do
+      fab!(:queued_post_reviewable) { Fabricate(:reviewable_queued_post) }
+
+      it "delete_user does not delete reviewable" do
+        review_page.visit_reviewable(queued_post_reviewable)
+
+        expect(queued_post_reviewable).to be_pending
+        expect(queued_post_reviewable.target_created_by).to be_present
+        expect(review_page).to have_reviewable_action_dropdown
+        expect(review_page).to have_reviewable_with_pending_status(queued_post_reviewable)
+
+        review_page.select_bundled_action(queued_post_reviewable, "delete_user")
+
+        expect(review_page).to have_no_error_dialog_visible
+        expect(review_page).to have_reviewable_with_rejected_status(queued_post_reviewable)
+        expect(review_page).not_to have_reviewable_action_dropdown
+        expect(queued_post_reviewable.reload).to be_rejected
+        expect(queued_post_reviewable.target_created_by).to be_nil
+      end
+    end
+  end
 end