FIX: Deleting Users should work nicely with Reviewable Users

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.

diff --git a/app/assets/javascripts/discourse/templates/components/reviewable-user.hbs b/app/assets/javascripts/discourse/templates/components/reviewable-user.hbs
index 0afc052..a0f6143 100644
--- a/app/assets/javascripts/discourse/templates/components/reviewable-user.hbs
+++ b/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}}
diff --git a/app/jobs/regular/create_user_reviewable.rb b/app/jobs/regular/create_user_reviewable.rb
index 911728d..2546f7f 100644
--- a/app/jobs/regular/create_user_reviewable.rb
+++ b/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],
diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb
index 1f4f7a0..f1ebf49 100644
--- a/app/models/reviewable_user.rb
+++ b/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
diff --git a/app/models/user.rb b/app/models/user.rb
index 566ddeb..f1f7989 100644
--- a/app/models/user.rb
+++ b/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
diff --git a/app/serializers/reviewable_user_serializer.rb b/app/serializers/reviewable_user_serializer.rb
index 1991a4f..f41320a 100644
--- a/app/serializers/reviewable_user_serializer.rb
+++ b/app/serializers/reviewable_user_serializer.rb
@@ -1,6 +1,6 @@
 class ReviewableUserSerializer < ReviewableSerializer
 
-  target_attributes(
+  payload_attributes(
     :username,
     :email,
     :name
diff --git a/app/services/user_destroyer.rb b/app/services/user_destroyer.rb
index be4e91e..dc554f3 100644
--- a/app/services/user_destroyer.rb
+++ b/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|
 
diff --git a/db/migrate/20190403202001_fix_reviewable_users.rb b/db/migrate/20190403202001_fix_reviewable_users.rb
new file mode 100644
index 0000000..eb95326
--- /dev/null
+++ b/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
diff --git a/spec/jobs/create_user_reviewable_spec.rb b/spec/jobs/create_user_reviewable_spec.rb
new file mode 100644
index 0000000..ffb2c3b
--- /dev/null
+++ b/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
diff --git a/spec/models/reviewable_user_spec.rb b/spec/models/reviewable_user_spec.rb
index 0782137..95e6c73 100644
--- a/spec/models/reviewable_user_spec.rb
+++ b/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
diff --git a/spec/serializers/reviewable_user_serializer_spec.rb b/spec/serializers/reviewable_user_serializer_spec.rb
index 5b117d0..88991ea 100644
--- a/spec/serializers/reviewable_user_serializer_spec.rb
+++ b/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

GitHub sha: 111a5022

This commit has been mentioned on Discourse Meta. There might be relevant details there: