FIX: Add users to user directory on account activation (#28505)

Follow-up to e3ae57ea7a

The previous commit added an `after_create` callback that triggers a refresh for the user directory whenever a `User` record is created. Theoretically, this approach should work, however, there's a gotcha in practice, because during a real user registration, when the `User` record is created in the database, it's not marked as active until the user verifies their email address and the user directory excludes inactive users, so the initial directory refresh triggered by the `after_create` callback becomes pointless.

To make a new user appear in the user directory immediately after sign up, we need to trigger a refresh via an `after_save` callback when they verify their email address and become active.

app/models/directory_item.rb

@@ -64,23 +64,7 @@ class DirectoryItem < ActiveRecord::Base
         period_type: period_types[period_type],
       )
 
-      # Create new records for users who don't have one yet
+      add_missing_users(period_type)
-
-      column_names =
-        DirectoryColumn.automatic_column_names + DirectoryColumn.plugin_directory_columns
-      DB.exec(
-        "INSERT INTO directory_items(period_type, user_id, #{column_names.map(&:to_s).join(", ")})
-                SELECT
-                    :period_type,
-                    u.id,
-                    #{Array.new(column_names.count) { |_| 0 }.join(", ")}
-                FROM users u
-                LEFT JOIN directory_items di ON di.user_id = u.id AND di.period_type = :period_type
-                WHERE di.id IS NULL AND u.id > 0 AND u.silenced_till IS NULL AND u.active
-                #{SiteSetting.must_approve_users ? "AND u.approved" : ""}
-              ",
-        period_type: period_types[period_type],
-      )
 
       # Calculate new values and update records
       #
@@ -163,6 +147,27 @@ class DirectoryItem < ActiveRecord::Base
         SQL
     end
   end
+
+  def self.add_missing_users_all_periods
+    period_types.each_key { |p| add_missing_users(p) }
+  end
+
+  def self.add_missing_users(period_type)
+    column_names = DirectoryColumn.automatic_column_names + DirectoryColumn.plugin_directory_columns
+    DB.exec(
+      "INSERT INTO directory_items(period_type, user_id, #{column_names.map(&:to_s).join(", ")})
+              SELECT
+                  :period_type,
+                  u.id,
+                  #{Array.new(column_names.count, 0).join(", ")}
+              FROM users u
+              LEFT JOIN directory_items di ON di.user_id = u.id AND di.period_type = :period_type
+              WHERE di.id IS NULL AND u.id > 0 AND u.silenced_till IS NULL AND u.active
+              #{SiteSetting.must_approve_users ? "AND u.approved" : ""}
+            ",
+      period_type: period_types[period_type],
+    )
+  end
 end
 
 # == Schema Information

app/models/user.rb

@@ -177,7 +177,6 @@ class User < ActiveRecord::Base
   after_create :set_default_categories_preferences
   after_create :set_default_tags_preferences
   after_create :set_default_sidebar_section_links
-  after_create :refresh_user_directory, if: Proc.new { SiteSetting.bootstrap_mode_enabled }
   after_update :set_default_sidebar_section_links, if: Proc.new { self.saved_change_to_staged? }
 
   after_update :trigger_user_updated_event,
@@ -191,6 +190,8 @@ class User < ActiveRecord::Base
   before_save :match_primary_group_changes
   before_save :check_if_title_is_badged_granted
   before_save :apply_watched_words, unless: :should_skip_user_fields_validation?
+  before_save :check_qualification_for_users_directory,
+              if: Proc.new { SiteSetting.bootstrap_mode_enabled }
 
   after_save :expire_tokens_if_password_changed
   after_save :clear_global_notice_if_needed
@@ -199,6 +200,8 @@ class User < ActiveRecord::Base
   after_save :expire_old_email_tokens
   after_save :index_search
   after_save :check_site_contact_username
+  after_save :add_to_user_directory,
+             if: Proc.new { SiteSetting.bootstrap_mode_enabled && @qualified_for_users_directory }
 
   after_save do
     if saved_change_to_uploaded_avatar_id?
@@ -2226,8 +2229,16 @@ class User < ActiveRecord::Base
     UserStatus.new(status).validate!
   end
 
-  def refresh_user_directory
+  def check_qualification_for_users_directory
-    DirectoryItem.refresh!
+    if (!self.active_was && self.active) || (!self.approved_was && self.approved) ||
+         (self.id_was.nil? && self.id.present?)
+      @qualified_for_users_directory = true
+    end
+  end
+
+  def add_to_user_directory
+    DirectoryItem.add_missing_users_all_periods
+    @qualified_for_users_directory = false
   end
 end
 

spec/models/user_spec.rb

@@ -129,27 +129,6 @@ RSpec.describe User do
         ) { user.update(name: "Batman") }
       end
     end
-
-    describe "#refresh_user_directory" do
-      context "when bootstrap mode is enabled" do
-        before { SiteSetting.bootstrap_mode_enabled = true }
-
-        it "creates directory items for a new user for all periods" do
-          expect do user = Fabricate(:user) end.to change { DirectoryItem.count }.by(
-            DirectoryItem.period_types.count,
-          )
-          expect(DirectoryItem.where(user_id: user.id)).to exist
-        end
-      end
-
-      context "when bootstrap mode is disabled" do
-        before { SiteSetting.bootstrap_mode_enabled = false }
-
-        it "doesn't create directory items for a new user" do
-          expect do Fabricate(:user) end.not_to change { DirectoryItem.count }
-        end
-      end
-    end
   end
 
   describe "Validations" do

spec/requests/users_controller_spec.rb

@@ -128,6 +128,34 @@ RSpec.describe UsersController do
           expect(session[:current_user_id]).to be_blank
         end
       end
+
+      context "when bootstrap mode is enabled" do
+        before { SiteSetting.bootstrap_mode_enabled = true }
+
+        it "adds the user to the user directory" do
+          token = Fabricate(:email_token, user: inactive_user)
+
+          expect do put "/u/activate-account/#{token.token}" end.to change {
+            DirectoryItem.where(user_id: inactive_user.id).count
+          }.by(DirectoryItem.period_types.count)
+
+          expect(response.status).to eq(200)
+        end
+      end
+
+      context "when bootstrap mode is disabled" do
+        before { SiteSetting.bootstrap_mode_enabled = false }
+
+        it "adds the user to the user directory" do
+          token = Fabricate(:email_token, user: inactive_user)
+
+          expect do put "/u/activate-account/#{token.token}" end.not_to change {
+            DirectoryItem.where(user_id: inactive_user.id).count
+          }
+
+          expect(response.status).to eq(200)
+        end
+      end
     end
 
     context "when cookies contains a destination URL" do