FIX: Deleting Users should work nicely with Reviewable Users

"Rejecting" a user in the queue is equivalent to deleting them, which
would then making it impossible to review rejected users. Now we store
information about the user in the payload so if they are deleted things
still display in the Rejected view.

Secondly, if a user is destroyed outside of the review queue, it will
now automatically "Reject" that queue item.

app/assets/javascripts/discourse/templates/components/reviewable-user.hbs

@@ -1,7 +1,7 @@
 <div class='reviewable-user-info'>
   <div class='reviewable-user-details'>
-    {{reviewable.username}}
-    {{reviewable.email}}
+    {{reviewable.payload.username}}
+    {{reviewable.payload.email}}
   </div>
 
   {{yield}}

app/jobs/regular/create_user_reviewable.rb

@@ -5,7 +5,16 @@ class Jobs::CreateUserReviewable < Jobs::Base
     if user = User.find_by(id: args[:user_id])
       return if user.approved?
 
-      reviewable = ReviewableUser.create!(target: user, created_by: Discourse.system_user, reviewable_by_moderator: true)
+      reviewable = ReviewableUser.create!(
+        target: user,
+        created_by: Discourse.system_user,
+        reviewable_by_moderator: true,
+        payload: {
+          username: user.username,
+          name: user.name,
+          email: user.email
+        }
+      )
       reviewable.add_score(
         Discourse.system_user,
         ReviewableScore.types[:needs_approval],

app/models/reviewable_user.rb

@@ -34,7 +34,7 @@ class ReviewableUser < Reviewable
   end
 
   def perform_reject(performed_by, args)
-    destroyer = UserDestroyer.new(performed_by)
+    destroyer = UserDestroyer.new(performed_by) unless args[:skip_delete]
 
     # If a user has posts, we won't delete them to preserve their content.
     # However the reviable record will be "rejected" and they will remain

app/models/user.rb

@@ -1220,7 +1220,6 @@ class User < ActiveRecord::Base
     Jobs.enqueue(:create_user_reviewable, user_id: self.id)
   end
 
-
   protected
 
   def badge_grant

app/serializers/reviewable_user_serializer.rb

@@ -1,6 +1,6 @@
 class ReviewableUserSerializer < ReviewableSerializer
 
-  target_attributes(
+  payload_attributes(
     :username,
     :email,
     :name

app/services/user_destroyer.rb

@@ -28,6 +28,10 @@ class UserDestroyer
       Draft.where(user_id: user.id).delete_all
       Reviewable.where(created_by_id: user.id).delete_all
 
+      if reviewable = Reviewable.find_by(target: user)
+        reviewable.perform(@actor, :reject, skip_delete: true)
+      end
+
       if opts[:delete_posts]
         user.posts.each do |post|
 

db/migrate/20190403202001_fix_reviewable_users.rb

@@ -0,0 +1,20 @@
+class FixReviewableUsers < ActiveRecord::Migration[5.2]
+  def up
+    execute(<<~SQL)
+      UPDATE reviewables
+      SET payload = json_build_object(
+        'username', u.username,
+        'name', u.name,
+        'email', ue.email
+      )::jsonb
+      FROM reviewables AS r
+      LEFT OUTER JOIN users AS u ON u.id = r.target_id
+      LEFT OUTER JOIN user_emails AS ue ON ue.user_id = u.id AND ue.primary
+      WHERE r.id = reviewables.id
+        AND r.type = 'ReviewableUser'
+    SQL
+  end
+
+  def down
+  end
+end

spec/jobs/create_user_reviewable_spec.rb

@@ -0,0 +1,19 @@
+require 'rails_helper'
+require_dependency 'jobs/regular/create_user_reviewable'
+
+describe Jobs::CreateUserReviewable do
+
+  let(:user) { Fabricate(:user) }
+
+  it "creates the reviewable" do
+    described_class.new.execute(user_id: user.id)
+
+    reviewable = Reviewable.find_by(target: user)
+    expect(reviewable).to be_present
+    expect(reviewable.pending?).to eq(true)
+    expect(reviewable.reviewable_scores).to be_present
+    expect(reviewable.payload['username']).to eq(user.username)
+    expect(reviewable.payload['name']).to eq(user.name)
+    expect(reviewable.payload['email']).to eq(user.email)
+  end
+end

spec/models/reviewable_user_spec.rb

@@ -36,6 +36,17 @@ RSpec.describe ReviewableUser, type: :model do
     end
   end
 
+  context "when a user is deleted" do
+    it "should reject the reviewable" do
+      Jobs::CreateUserReviewable.new.execute(user_id: user.id)
+      reviewable = Reviewable.find_by(target: user)
+      expect(reviewable.pending?).to eq(true)
+
+      UserDestroyer.new(Discourse.system_user).destroy(user)
+      expect(reviewable.reload.rejected?).to eq(true)
+    end
+  end
+
   context "perform" do
     let(:reviewable) { Fabricate(:reviewable) }
     context "approve" do

spec/serializers/reviewable_user_serializer_spec.rb

@@ -2,15 +2,18 @@ require 'rails_helper'
 
 describe ReviewableUserSerializer do
 
-  let(:reviewable) { Fabricate(:reviewable) }
+  let(:user) { Fabricate(:user) }
   let(:admin) { Fabricate(:admin) }
 
   it "includes the user fields for review" do
+    Jobs::CreateUserReviewable.new.execute(user_id: user.id)
+    reviewable = Reviewable.find_by(target: user)
+
     json = ReviewableUserSerializer.new(reviewable, scope: Guardian.new(admin), root: nil).as_json
     expect(json[:user_id]).to eq(reviewable.target_id)
-    expect(json[:username]).to eq(reviewable.target.username)
-    expect(json[:email]).to eq(reviewable.target.email)
-    expect(json[:name]).to eq(reviewable.target.name)
+    expect(json[:payload]['username']).to eq(user.username)
+    expect(json[:payload]['email']).to eq(user.email)
+    expect(json[:payload]['name']).to eq(user.name)
     expect(json[:topic_url]).to be_blank
   end