From daa97c40ced1c8b265c18a97dd111bab6fa1c945 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Wed, 26 Apr 2017 10:51:36 +0800
Subject: [PATCH] FIX: Clean up unused staged users job not completing.

---
 .../scheduled/clean_up_unused_staged_users.rb | 20 +++++++---
 spec/jobs/clean_up_unused_staged_users.rb     | 37 +++++++++++++++++++
 2 files changed, 52 insertions(+), 5 deletions(-)
 create mode 100644 spec/jobs/clean_up_unused_staged_users.rb

diff --git a/app/jobs/scheduled/clean_up_unused_staged_users.rb b/app/jobs/scheduled/clean_up_unused_staged_users.rb
index f11ec67c1ec..074b09e9780 100644
--- a/app/jobs/scheduled/clean_up_unused_staged_users.rb
+++ b/app/jobs/scheduled/clean_up_unused_staged_users.rb
@@ -6,11 +6,21 @@ module Jobs
     def execute(args)
       destroyer = UserDestroyer.new(Discourse.system_user)
 
-      User.joins(:user_stat)
-          .where(staged: true)
-          .where("users.created_at < ?", 1.year.ago)
-          .where("user_stats.post_count = 0")
-          .find_each { |user| destroyer.destroy(user) }
+      User.joins("LEFT JOIN posts ON posts.user_id = users.id")
+        .where("posts.user_id IS NULL")
+        .where(staged: true)
+        .where("users.created_at < ?", 1.year.ago)
+        .find_each do |user|
+
+        begin
+          destroyer.destroy(user)
+        rescue => e
+          Discourse.handle_job_exception(e,
+            message: "Cleaning up unused staged user",
+            extra: { user_id: user.id }
+          )
+        end
+      end
     end
 
   end
diff --git a/spec/jobs/clean_up_unused_staged_users.rb b/spec/jobs/clean_up_unused_staged_users.rb
new file mode 100644
index 00000000000..2fd222c0e8a
--- /dev/null
+++ b/spec/jobs/clean_up_unused_staged_users.rb
@@ -0,0 +1,37 @@
+require 'rails_helper'
+
+RSpec.describe Jobs::CleanUpUnusedStagedUsers do
+  let(:user) { Fabricate(:user) }
+  let(:staged_user) { Fabricate(:user, staged: true) }
+
+  context 'when staged user is unused' do
+    it 'should clean up the staged user' do
+      user
+      staged_user.update!(created_at: 2.years.ago)
+
+      expect { described_class.new.execute({}) }.to change { User.count }.by(-1)
+      expect(User.find_by(id: staged_user.id)).to eq(nil)
+    end
+
+    describe 'when staged user is not old enough' do
+      it 'should not clean up the staged user' do
+        user
+        staged_user.update!(created_at: 5.months.ago)
+
+        expect { described_class.new.execute({}) }.to_not change { User.count }
+        expect(User.find_by(id: staged_user.id)).to eq(staged_user)
+      end
+    end
+  end
+
+  context 'when staged user is not unused' do
+    it 'should not clean up the staged user' do
+      user
+      Fabricate(:post, user: staged_user)
+      user.update!(created_at: 2.years.ago)
+
+      expect { described_class.new.execute({}) }.to_not change { User.count }
+      expect(User.find_by(id: staged_user.id)).to eq(staged_user)
+    end
+  end
+end