DEV: Make user field validation more specific (#16746)

- Only validate if custom_fields are loaded, so that we don't trigger a db query
- Only validate public user fields, not all custom_fields

This commit also reverts the unrelated spec changes in ba148e08, which were required to work around these issues

app/models/user.rb

@@ -114,7 +114,7 @@ class User < ActiveRecord::Base
   validates :name, user_full_name: true, if: :will_save_change_to_name?, length: { maximum: 255 }
   validates :ip_address, allowed_ip_address: { on: :create, message: :signup_not_allowed }
   validates :primary_email, presence: true
-  validates :custom_fields_values, watched_words: true
+  validates :public_user_field_values, watched_words: true, unless: :custom_fields_clean?
   validates_associated :primary_email, message: -> (_, user_email) { user_email[:value]&.errors[:email]&.first }
 
   after_initialize :add_trust_level
@@ -1245,6 +1245,11 @@ class User < ActiveRecord::Base
     end
   end
 
+  def public_user_field_values
+    @public_user_field_ids ||= UserField.public_fields.pluck(:id)
+    user_fields(@public_user_field_ids).values.join(" ")
+  end
+
   def set_user_field(field_id, value)
     custom_fields["#{USER_FIELD_PREFIX}#{field_id}"] = value
   end
@@ -1792,10 +1797,6 @@ class User < ActiveRecord::Base
       )
     SQL
   end
-
-  def custom_fields_values
-    custom_fields.values.join(" ")
-  end
 end
 
 # == Schema Information

app/models/user_field.rb

@@ -14,6 +14,8 @@ class UserField < ActiveRecord::Base
   before_save :sanitize_description
   after_save :queue_index_search
 
+  scope :public_fields, -> { where(show_on_profile: true).or(where(show_on_user_card: true)) }
+
   def self.max_length
     2048
   end

spec/jobs/activation_reminder_emails_spec.rb

@@ -12,9 +12,12 @@ describe Jobs::ActivationReminderEmails do
     expect { described_class.new.execute({}) }
       .to change { ActionMailer::Base.deliveries.size }.by(1)
       .and change { user.email_tokens.count }.by(1)
-      .and change { user.reload.custom_fields[:activation_reminder] }.to "t"
+
+    expect(user.custom_fields['activation_reminder']).to eq("t")
     expect { described_class.new.execute({}) }.to change { ActionMailer::Base.deliveries.size }.by(0)
-    expect { user.activate }.to change { user.reload.custom_fields[:activation_reminder] }.to nil
+
+    user.activate
+    expect(user.reload.custom_fields['activation_reminder']).to eq(nil)
   end
 
   it 'should not email active users' do

spec/jobs/bulk_invite_spec.rb

@@ -100,7 +100,7 @@ describe Jobs::BulkInvite do
       expect(User.where(staged: true).find_by_email('test@discourse.org')).to eq(nil)
       expect(user.user_fields[user_field.id.to_s]).to eq('value 1')
       expect(user.user_fields[user_field_color.id.to_s]).to eq('Blue')
-      expect(staged_user.reload.user_fields[user_field.id.to_s]).to eq('value 2')
+      expect(staged_user.user_fields[user_field.id.to_s]).to eq('value 2')
       expect(staged_user.user_fields[user_field_color.id.to_s]).to eq(nil)
       new_staged_user = User.where(staged: true).find_by_email('test2@discourse.org')
       expect(new_staged_user.user_fields[user_field.id.to_s]).to eq('value 3')

spec/models/user_spec.rb

@@ -138,7 +138,7 @@ RSpec.describe User do
     end
 
     describe "#user_fields" do
-      fab!(:user_field) { Fabricate(:user_field) }
+      fab!(:user_field) { Fabricate(:user_field, show_on_profile: true) }
       fab!(:watched_word) { Fabricate(:watched_word, word: "bad") }
 
       before { user.set_user_field(user_field.id, value) }
@@ -146,6 +146,7 @@ RSpec.describe User do
       context "when user fields contain watched words" do
         let(:value) { "bad user field value" }
 
+        context "when user field is public" do
           it "is not valid" do
             user.valid?
             expect(user.errors[:base].size).to eq(1)
@@ -153,6 +154,13 @@ RSpec.describe User do
           end
         end
 
+        context "when user field is private" do
+          before { user_field.update(show_on_profile: false) }
+
+          it { is_expected.to be_valid }
+        end
+      end
+
       context "when user fields do not contain watched words" do
         let(:value) { "good user field value" }
 

spec/services/user_anonymizer_spec.rb

@@ -289,9 +289,9 @@ describe UserAnonymizer do
           "user_field_#{field2.id}": "bar",
           "another_field": "456"
         }
-        user.save
 
-        expect { make_anonymous }.to change { user.reload.custom_fields }.to "some_field" => "123", "another_field" => "456"
+        expect { make_anonymous }.to change { user.custom_fields }
+        expect(user.reload.custom_fields).to eq("some_field" => "123", "another_field" => "456")
       end
     end
   end