Add custom profile fields (#10202)

* Add custom_profile_fields table

* Add CustomProfileField model and refactor

* Remove trailing whitespace

* Stop ignoring removed column

* Add explanatory comment

* Update service object

* Fix bug in service object

* Refactor and fix specs

* Increase limit from 3 to 5

* Update comment to reflect code changes

app/models/concerns/acts_as_profile_field.rb

@@ -0,0 +1,17 @@
+# Used for sharing behavior between ProfileField and CustomProfileField
+concern :ActsAsProfileField do
+  included do
+    before_create :generate_attribute_name
+
+    WORD_REGEX = /\w+/.freeze
+
+    validates :label, presence: true, uniqueness: { case_sensitive: false }
+    validates :attribute_name, presence: true, on: :update
+  end
+
+  private
+
+  def generate_attribute_name
+    self.attribute_name = label.titleize.scan(WORD_REGEX).join.underscore
+  end
+end

app/models/custom_profile_field.rb

@@ -0,0 +1,16 @@
+class CustomProfileField < ApplicationRecord
+  include ActsAsProfileField
+
+  belongs_to :profile
+
+  validate :validate_maximum_count
+
+  private
+
+  # We allow a maximum of 5 custom profile fields per user
+  def validate_maximum_count
+    return if profile.custom_profile_fields.count < 5
+
+    errors.add(:profile_id, "maximum number of custom profile fields exceeded")
+  end
+end

app/models/profile.rb

@@ -3,6 +3,10 @@ class Profile < ApplicationRecord
 
   validates :user_id, uniqueness: true
 
+  has_many :custom_profile_fields, dependent: :destroy
+
+  store_attribute :data, :custom_attributes, :json, default: {}
+
   # NOTE: @citizen428 This is a temporary mapping so we don't break DEV during
   # profile migration/generalization work.
   MAPPED_ATTRIBUTES = {
@@ -19,13 +23,13 @@ class Profile < ApplicationRecord
   # Generates typed accessors for all currently defined profile fields.
   def self.refresh_attributes!
     ProfileField.find_each do |field|
-      store_attribute :data, field.attribute_name, field.type
+      store_attribute :data, field.attribute_name.to_sym, field.type
     end
   end
 
   # Returns an array of all currently defined `store_attribute`s on `data`.
   def self.attributes
-    stored_attributes[:data] || []
+    (stored_attributes[:data] || []).map(&:to_s)
   end
 
   # Forces a reload before returning attributes
@@ -39,4 +43,8 @@ class Profile < ApplicationRecord
   def self.mapped_attributes
     attributes!.map { |attribute| MAPPED_ATTRIBUTES.fetch(attribute, attribute).to_s }
   end
+
+  def custom_profile_attributes
+    custom_profile_fields.pluck(:attribute_name)
+  end
 end

app/models/profile_field.rb

@@ -1,9 +1,5 @@
 class ProfileField < ApplicationRecord
-  self.ignored_columns = ["group"]
-
-  before_create :generate_attribute_name
-
-  WORD_REGEX = /\w+/.freeze
+  include ActsAsProfileField
 
   # Key names follow the Rails form helpers
   enum input_type: {
@@ -20,8 +16,6 @@ class ProfileField < ApplicationRecord
 
   belongs_to :profile_field_group, optional: true
 
-  validates :label, presence: true, uniqueness: { case_sensitive: false }
-  validates :attribute_name, presence: true, on: :update
   validates :show_in_onboarding, inclusion: { in: [true, false] }
 
   def type
@@ -29,10 +23,4 @@ class ProfileField < ApplicationRecord
 
     :string
   end
-
-  private
-
-  def generate_attribute_name
-    self.attribute_name = label.titleize.scan(WORD_REGEX).join.underscore
-  end
 end

app/services/profiles/update.rb

@@ -16,6 +16,10 @@ module Profiles
       # Ensure we have up to date attributes
       Profile.refresh_attributes!
 
+      # Handle user specific custom profile fields
+      custom_attributes = @updated_attributes.extract!(*@profile.custom_profile_attributes)
+      @updated_attributes[:custom_attributes] = custom_attributes
+
       # We don't update `data` directly. This uses the defined store_attributes
       # so we can make use of their typecasting.
       @profile.assign_attributes(@updated_attributes)
@@ -26,17 +30,23 @@ module Profiles
       return unless @profile.save
 
       # Propagate changes back to the `users` table
-      user_attributes = @profile.data.transform_keys do |key|
-        Profile::MAPPED_ATTRIBUTES.fetch(key, key).to_s
-      end
-      @profile.user._skip_profile_sync = true
-      @success = true if @profile.user.update(user_attributes)
-      @profile.user._skip_profile_sync = false
+      sync_to_user
       self
     end
 
     def success?
       @success
     end
+
+    private
+
+    def sync_to_user
+      user_attributes = @profile.data.transform_keys do |key|
+        Profile::MAPPED_ATTRIBUTES.fetch(key, key).to_s
+      end
+      @profile.user._skip_profile_sync = true
+      @success = true if @profile.user.update(user_attributes.except("custom_attributes"))
+      @profile.user._skip_profile_sync = false
+    end
   end
 end

db/migrate/20200904040009_create_custom_profile_fields.rb

@@ -0,0 +1,18 @@
+class CreateCustomProfileFields < ActiveRecord::Migration[6.0]
+  def change
+    safety_assured do
+      create_table :custom_profile_fields do |t|
+        t.references :profile, null: false
+
+        t.string "attribute_name", null: false
+        t.string "description"
+        t.citext "label", null: false
+
+        t.timestamps
+      end
+
+      add_foreign_key :custom_profile_fields, :profiles, on_delete: :cascade
+      add_index :custom_profile_fields, [:label, :profile_id], unique: true
+    end
+  end
+end

db/schema.rb

@@ -444,6 +444,17 @@ ActiveRecord::Schema.define(version: 2020_09_04_151734) do
     t.index ["spent"], name: "index_credits_on_spent"
   end
 
+  create_table "custom_profile_fields", force: :cascade do |t|
+    t.string "attribute_name", null: false
+    t.datetime "created_at", precision: 6, null: false
+    t.string "description"
+    t.citext "label", null: false
+    t.bigint "profile_id", null: false
+    t.datetime "updated_at", precision: 6, null: false
+    t.index ["label", "profile_id"], name: "index_custom_profile_fields_on_label_and_profile_id", unique: true
+    t.index ["profile_id"], name: "index_custom_profile_fields_on_profile_id"
+  end
+
   create_table "data_update_scripts", force: :cascade do |t|
     t.datetime "created_at", null: false
     t.string "file_name"
@@ -1388,6 +1399,7 @@ ActiveRecord::Schema.define(version: 2020_09_04_151734) do
   add_foreign_key "comments", "users", on_delete: :cascade
   add_foreign_key "credits", "organizations", on_delete: :restrict
   add_foreign_key "credits", "users", on_delete: :cascade
+  add_foreign_key "custom_profile_fields", "profiles", on_delete: :cascade
   add_foreign_key "display_ad_events", "display_ads", on_delete: :cascade
   add_foreign_key "display_ad_events", "users", on_delete: :cascade
   add_foreign_key "display_ads", "organizations", on_delete: :cascade

spec/factories/custom_profile_fields.rb

@@ -0,0 +1,7 @@
+FactoryBot.define do
+  factory :custom_profile_field do
+    profile
+    sequence(:label) { |n| "Email #{n}" }
+    description { "some description" }
+  end
+end

spec/models/custom_profile_field_spec.rb

@@ -0,0 +1,19 @@
+require "rails_helper"
+
+RSpec.describe CustomProfileField, type: :model do
+  it_behaves_like "a profile field"
+
+  describe "#validate_maximum_count" do
+    it "validates that user's can't have more than 3 custom profile fields" do
+      profile = create(:profile)
+      create_list(:custom_profile_field, 5, profile: profile)
+
+      custom_profile_field = build(:custom_profile_field, profile: profile)
+      custom_profile_field.save
+
+      expect(custom_profile_field).not_to be_valid
+      expect(custom_profile_field.errors[:profile_id])
+        .to include("maximum number of custom profile fields exceeded")
+    end
+  end
+end

spec/models/profile_field_spec.rb

@@ -1,24 +1,12 @@
 require "rails_helper"
 
 RSpec.describe ProfileField, type: :model do
-  let(:profile_field) { create(:profile_field) }
+  it_behaves_like "a profile field"
 
   describe "validations" do
     describe "builtin validations" do
-      subject { profile_field }
-
       it { is_expected.to belong_to(:profile_field_group).optional(true) }
-      it { is_expected.to validate_presence_of(:label) }
-      it { is_expected.to validate_uniqueness_of(:label).case_insensitive }
-      it { is_expected.to validate_presence_of(:attribute_name).on(:update) }
       it { is_expected.to validate_inclusion_of(:show_in_onboarding).in_array([true, false]) }
     end
   end
-
-  describe "callbacks" do
-    it "automatically generates an attribute name" do
-      pf = create(:profile_field, label: "Is this a test? This is a test! 1")
-      expect(pf.attribute_name).to eq "is_this_a_test_this_is_a_test1"
-    end
-  end
 end

spec/models/shared_examples/a_profile_field.rb

@@ -0,0 +1,20 @@
+RSpec.shared_examples "a profile field" do
+  let(:profile_field_class) { described_class.name.underscore }
+
+  describe "validations" do
+    describe "builtin validations" do
+      subject { create(profile_field_class) }
+
+      it { is_expected.to validate_presence_of(:label) }
+      it { is_expected.to validate_uniqueness_of(:label).case_insensitive }
+      it { is_expected.to validate_presence_of(:attribute_name).on(:update) }
+    end
+  end
+
+  describe "callbacks" do
+    it "automatically generates an attribute name" do
+      field = create(profile_field_class, label: "Test? Test! 1")
+      expect(field.attribute_name).to eq "test_test1"
+    end
+  end
+end

spec/services/profiles/update_spec.rb

@@ -5,19 +5,12 @@ RSpec.describe Profiles::Update, type: :service do
     create(:profile, data: { name: "Sloan Doe", looking_for_work: true, removed: "Bla" })
   end
 
-  # rubocop:disable RSpec/BeforeAfterAll
-  before(:all) do
+  before do
     create(:profile_field, label: "Name", input_type: :text_field)
     create(:profile_field, label: "Looking for work", input_type: :check_box)
     Profile.refresh_attributes!
   end
 
-  after(:all) do
-    ProfileField.destroy_all
-    Profile.refresh_attributes!
-  end
-  # rubocop:enable RSpec/BeforeAfterAll
-
   it "correctly typecasts new attributes", :aggregate_failures do
     described_class.call(profile, name: 123, looking_for_work: "false")
     expect(profile.name).to eq "123"
@@ -36,4 +29,13 @@ RSpec.describe Profiles::Update, type: :service do
     expect(profile.name).to eq new_name
     expect(profile.user[:name]).to eq new_name
   end
+
+  it "sets custom attributes for the user" do
+    custom_profile_field = create(:custom_profile_field, profile: profile)
+    custom_attribute = custom_profile_field.attribute_name
+
+    described_class.call(profile, custom_attribute => "Test")
+
+    expect(profile.custom_attributes[custom_attribute]).to eq "Test"
+  end
 end