PERF: run user merging task in a background job. (#10961)

* PERF: run user merging task in a background job.

Currently, admin page is timing out while merging the users with lots of posts.

app/assets/javascripts/admin/addon/models/admin-user.js

@@ -9,6 +9,7 @@ import bootbox from "bootbox";
 import discourseComputed from "discourse-common/utils/decorators";
 import getURL from "discourse-common/lib/get-url";
 import { iconHTML } from "discourse-common/lib/icon-library";
+import messageBus from "message-bus-client";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import { propertyNotEqual } from "discourse/lib/computed";
 
@@ -493,7 +494,7 @@ const AdminUser = User.extend({
     const user = this;
     const location = document.location.pathname;
 
-    bootbox.dialog(I18n.t("admin.user.merging_user"));
+    const bootboxDiv = bootbox.dialog(I18n.t("admin.user.merging_user"));
     let formData = { context: location };
 
     if (opts && opts.targetUsername) {
@@ -504,20 +505,25 @@ const AdminUser = User.extend({
       type: "POST",
       data: formData,
     })
-      .then((data) => {
+      .then((response) => {
-        if (data.merged) {
+        if (response.success) {
-          if (/^\/admin\/users\/list\//.test(location)) {
+          messageBus.subscribe("/merge_user", (data) => {
-            DiscourseURL.redirectTo(location);
+            if (data.merged) {
-          } else {
+              if (/^\/admin\/users\/list\//.test(location)) {
-            DiscourseURL.redirectTo(
+                DiscourseURL.redirectTo(location);
-              `/admin/users/${data.user.id}/${data.user.username}`
+              } else {
-            );
+                DiscourseURL.redirectTo(
-          }
+                  `/admin/users/${data.user.id}/${data.user.username}`
+                );
+              }
+            } else if (data.message) {
+              bootboxDiv.find(".modal-body").html(data.message);
+            } else if (data.failed) {
+              bootbox.alert(I18n.t("admin.user.merge_failed"));
+            }
+          });
         } else {
           bootbox.alert(I18n.t("admin.user.merge_failed"));
-          if (data.user) {
-            user.setProperties(data.user);
-          }
         }
       })
       .catch(() => {

app/controllers/admin/users_controller.rb

@@ -502,14 +502,9 @@ class Admin::UsersController < Admin::AdminController
     raise Discourse::NotFound if target_user.blank?
 
     guardian.ensure_can_merge_users!(@user, target_user)
-    serializer_opts = { root: false, scope: guardian }
 
-    if user = UserMerger.new(@user, target_user, current_user).merge!
+    Jobs.enqueue(:merge_user, user_id: @user.id, target_user_id: target_user.id, current_user_id: current_user.id)
-      user_json = AdminDetailedUserSerializer.new(user, serializer_opts).as_json
+    render json: success_json
-      render json: success_json.merge(merged: true, user: user_json)
-    else
-      render json: failed_json.merge(user: AdminDetailedUserSerializer.new(@user, serializer_opts).as_json)
-    end
   end
 
   def reset_bounce_score

app/jobs/regular/merge_user.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module Jobs
+  class MergeUser < ::Jobs::Base
+
+    def execute(args)
+      target_user_id = args[:target_user_id]
+      current_user_id = args[:current_user_id]
+
+      user = User.find_by(id: args[:user_id])
+      target_user = User.find_by(id: args[:target_user_id])
+      current_user = User.find_by(id: args[:current_user_id])
+      guardian = Guardian.new(current_user)
+      serializer_opts = { root: false, scope: guardian }
+
+      if user = UserMerger.new(user, target_user, current_user).merge!
+        user_json = AdminDetailedUserSerializer.new(user, serializer_opts).as_json
+        ::MessageBus.publish '/merge_user', { success: 'OK' }.merge(merged: true, user: user_json), user_ids: [current_user.id]
+      else
+        ::MessageBus.publish '/merge_user', { failed: 'FAILED' }.merge(user: AdminDetailedUserSerializer.new(@user, serializer_opts).as_json), user_ids: [current_user.id]
+      end
+    end
+  end
+end

app/services/user_merger.rb

@@ -5,6 +5,7 @@ class UserMerger
     @source_user = source_user
     @target_user = target_user
     @acting_user = acting_user
+    @user_id = source_user.id
     @source_primary_email = source_user.email
   end
 
@@ -30,6 +31,9 @@ class UserMerger
   protected
 
   def update_username
+    return if @source_user.username == @target_user.username
+
+    ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.updating_username") }, user_ids: [@acting_user.id] if @acting_user
     UsernameChanger.update_username(user_id: @source_user.id,
                                     old_username: @source_user.username,
                                     new_username: @target_user.username,
@@ -43,6 +47,10 @@ class UserMerger
       .order(:topic_id, :post_number)
       .pluck(:topic_id, :id)
 
+    return if posts.count == 0
+
+    ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.changing_post_ownership") }, user_ids: [@acting_user.id] if @acting_user
+
     last_topic_id = nil
     post_ids = []
 
@@ -70,6 +78,8 @@ class UserMerger
   end
 
   def merge_given_daily_likes
+    ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.merging_given_daily_likes") }, user_ids: [@acting_user.id] if @acting_user
+
     sql = <<~SQL
       INSERT INTO given_daily_likes AS g (user_id, likes_given, given_date, limit_reached)
         SELECT
@@ -102,6 +112,8 @@ class UserMerger
   end
 
   def merge_post_timings
+    ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.merging_post_timings") }, user_ids: [@acting_user.id] if @acting_user
+
     update_user_id(:post_timings, conditions: ["x.topic_id = y.topic_id",
                                                "x.post_number = y.post_number"])
     sql = <<~SQL
@@ -116,6 +128,8 @@ class UserMerger
   end
 
   def merge_user_visits
+    ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.merging_user_visits") }, user_ids: [@acting_user.id] if @acting_user
+
     update_user_id(:user_visits, conditions: "x.visited_at = y.visited_at")
 
     sql = <<~SQL
@@ -132,6 +146,8 @@ class UserMerger
   end
 
   def update_site_settings
+    ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.updating_site_settings") }, user_ids: [@acting_user.id] if @acting_user
+
     SiteSetting.all_settings(true).each do |setting|
       if setting[:type] == "username" && setting[:value] == @source_user.username
         SiteSetting.set_and_log(setting[:setting], @target_user.username)
@@ -140,6 +156,8 @@ class UserMerger
   end
 
   def update_user_stats
+    ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.updating_user_stats") }, user_ids: [@acting_user.id] if @acting_user
+
     # topics_entered
     DB.exec(<<~SQL, target_user_id: @target_user.id)
       UPDATE user_stats
@@ -194,6 +212,8 @@ class UserMerger
   end
 
   def merge_user_attributes
+    ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.merging_user_attributes") }, user_ids: [@acting_user.id] if @acting_user
+
     DB.exec(<<~SQL, source_user_id: @source_user.id, target_user_id: @target_user.id)
       UPDATE users AS t
       SET created_at              = LEAST(t.created_at, s.created_at),
@@ -235,6 +255,8 @@ class UserMerger
   end
 
   def update_user_ids
+    ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.updating_user_ids") }, user_ids: [@acting_user.id] if @acting_user
+
     Category.where(user_id: @source_user.id).update_all(user_id: @target_user.id)
 
     update_user_id(:category_users, conditions: ["x.category_id = y.category_id"])
@@ -356,6 +378,8 @@ class UserMerger
   end
 
   def delete_source_user
+    ::MessageBus.publish '/merge_user', { message: I18n.t("admin.user.merge_user.deleting_source_user") }, user_ids: [@acting_user.id] if @acting_user
+
     @source_user.reload
 
     @source_user.skip_email_validation = true

config/locales/server.en.yml

@@ -2496,7 +2496,18 @@ en:
   admin:
     email:
       sent_test: "sent!"
-
+    user:
+      merge_user:
+        updating_username: "Updating username..."
+        changing_post_ownership: "Changing post ownership..."
+        merging_given_daily_likes: "Merging given daily likes..."
+        merging_post_timings: "Merging post timings..."
+        merging_user_visits: "Merging user visits..."
+        updating_site_settings: "Updating site settings..."
+        updating_user_stats: "Updating user stats..."
+        merging_user_attributes: "Merging user attributes..."
+        updating_user_ids: "Updating user ids..."
+        deleting_source_user: "Deleting source user..."
   user:
     deactivated: "Was deactivated due to too many bounced emails to '%{email}'."
     deactivated_by_staff: "Deactivated by staff"

spec/requests/admin/users_controller_spec.rb

@@ -1079,13 +1079,12 @@ RSpec.describe Admin::UsersController do
     fab!(:first_post) { Fabricate(:post, topic: topic, user: user) }
 
     it 'should merge source user to target user' do
+      Jobs.run_immediately!
       post "/admin/users/#{user.id}/merge.json", params: {
         target_username: target_user.username
       }
 
       expect(response.status).to eq(200)
-      expect(response.parsed_body["merged"]).to be_truthy
-      expect(response.parsed_body["user"]["id"]).to eq(target_user.id)
       expect(topic.reload.user_id).to eq(target_user.id)
       expect(first_post.reload.user_id).to eq(target_user.id)
     end