[deploy] Migrate profile data (#9750)

* Add temporary Rake task for creating profile fields

* Fix problems after splitting branch

* Add profile migration Rake task

* Add unique index to profiles

* Add migration code to users

* Be smarter about updating profiles

* Update spec

* Update Bullet config

* Fix typo

* Change temporary rake task to data update script

* Re-add profile factory and update spec

* Change private declaration to make CodeClimate happy

* Update comment

* Drop validation in favor of DB default

* Update spec

app/models/profile.rb

@@ -1,6 +1,8 @@
 class Profile < ApplicationRecord
   belongs_to :user
 
+  validates :user_id, uniqueness: true
+
   # This method generates typed accessors for all active profile fields
   def self.refresh_store_accessors!
     ProfileField.active.find_each do |field|
@@ -9,6 +11,4 @@ class Profile < ApplicationRecord
   end
 
   refresh_store_accessors!
-
-  validates :data, presence: true
 end

app/models/user.rb

@@ -5,6 +5,61 @@ class User < ApplicationRecord
   include Searchable
   include Storext.model
 
+  # NOTE: @citizen428 This is temporary code during profile migration and will
+  # be removed.
+  # rubocop:disable Metrics/BlockLength
+  concerning :ProfileMigration do
+    included do
+      PROFIE_FIELDS =
+        (Profiles::ExtractData::DIRECT_ATTRIBUTES + Profiles::ExtractData::MAPPED_ATTRIBUTES.values).freeze
+
+      # NOTE: There are rare cases were we want to skip this callback, primarily
+      # in tests. `skip_callback` modifies global state, which is not thread-safe
+      # and can cause hard to track down bugs. We use an instance-level attribute
+      # instead. See `spec/factories/profiles.rb` for an example.
+      attr_accessor :_skip_creating_profile
+
+      # All new users should automatically have a profile
+      after_create_commit :create_profile, unless: :_skip_creating_profile
+
+      # Keep saving changes locally for the time being, but propagate them to profiles.
+      after_update_commit do
+        if (previous_changes.keys.map(&:to_sym) & User::PROFIE_FIELDS).present?
+          profile.update(data: Profiles::ExtractData.call(self))
+        end
+      end
+
+      # Define wrapped getters for profile attributes. We first try to get the
+      # value from the profile and if it doesn't exist there we move retrieve it
+      # from here.
+      Profiles::ExtractData::DIRECT_ATTRIBUTES.each do |attribute|
+        define_method(attribute) do
+          if profile.respond_to?(attribute)
+            profile.public_send(attribute)
+          else
+            self[attribute]
+          end
+        end
+      end
+
+      Profiles::ExtractData::MAPPED_ATTRIBUTES.each do |profile_attribute, user_attribute|
+        define_method(user_attribute) do
+          if profile.respond_to?(profile_attribute)
+            profile.public_send(profile_attribute)
+          else
+            self[user_attribute]
+          end
+        end
+      end
+
+      def create_profile
+        Profile.create(user: self, data: Profiles::ExtractData.call(self))
+      end
+      private :create_profile
+    end
+  end
+  # rubocop:enable Metrics/BlockLength
+
   BEHANCE_URL_REGEXP = %r{\A(http(s)?://)?(www.behance.net|behance.net)/.*\z}.freeze
   COLOR_HEX_REGEXP = /\A#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})\z/.freeze
   DRIBBBLE_URL_REGEXP = %r{\A(http(s)?://)?(www.dribbble.com|dribbble.com)/.*\z}.freeze

app/services/profiles/extract_data.rb

@@ -0,0 +1,47 @@
+module Profiles
+  module ExtractData
+    DIRECT_ATTRIBUTES = %i[
+      available_for
+      behance_url
+      currently_hacking_on
+      currently_learning
+      dribble_url
+      education
+      employer_name
+      employer_url
+      employment_title
+      facebook_url
+      instagram_url
+      location
+      looking_for_work
+      mastodon_url
+      medium_url
+      mostly_work_with
+      name
+      summary
+      twitch_url
+      website_url
+      youtube_url
+    ].freeze
+
+    MAPPED_ATTRIBUTES = {
+      brand_color1: :bg_color_hex,
+      brand_color2: :text_color_hex,
+      display_email_on_profile: :email_public,
+      display_looking_for_work_on_profile: :looking_for_work_publicly,
+      git_lab_url: :gitlab_url,
+      linked_in_url: :linkedin_url,
+      recruiters_can_contact_me_about_job_opportunities: :contact_consent,
+      stack_overflow_url: :stackoverflow_url
+    }.freeze
+
+    def self.call(user)
+      user_attributes = user.attributes.symbolize_keys!
+
+      direct_data = user_attributes.extract!(*DIRECT_ATTRIBUTES)
+      mapped_data = MAPPED_ATTRIBUTES.keys.zip(user_attributes.values_at(*MAPPED_ATTRIBUTES.values)).to_h
+
+      direct_data.merge(mapped_data)
+    end
+  end
+end

config/environments/development.rb

@@ -128,6 +128,8 @@ Rails.application.configure do
     # Supress incorrect warnings from Bullet due to included columns: https://github.com/flyerhzm/bullet/issues/147
     Bullet.add_whitelist(type: :unused_eager_loading, class_name: "Article", association: :top_comments)
     Bullet.add_whitelist(type: :unused_eager_loading, class_name: "Comment", association: :user)
+    # NOTE: @citizen428 Temporarily ignoring this while working out user - profile relationship
+    Bullet.add_whitelist(type: :n_plus_one_query, class_name: "User", association: :profile)
 
     # Check if there are any data update scripts to run during startup
     if %w[c console runner s server].include?(ENV["COMMAND"])

config/environments/test.rb

@@ -75,6 +75,8 @@ Rails.application.configure do
     # Supress incorrect warnings from Bullet due to included columns: https://github.com/flyerhzm/bullet/issues/147
     Bullet.add_whitelist(type: :unused_eager_loading, class_name: "Article", association: :top_comments)
     Bullet.add_whitelist(type: :unused_eager_loading, class_name: "Comment", association: :user)
+    # NOTE: @citizen428 Temporarily ignoring this while working out user - profile relationship
+    Bullet.add_whitelist(type: :n_plus_one_query, class_name: "User", association: :profile)
   end
 end
 # rubocop:enable Metrics/BlockLength

db/migrate/20200813031851_add_unique_index_to_profiles.rb

@@ -0,0 +1,8 @@
+class AddUniqueIndexToProfiles < ActiveRecord::Migration[6.0]
+  disable_ddl_transaction!
+
+  def change
+    remove_index :profiles, column: :user_id, algorithm: :concurrently
+    add_index :profiles, :user_id, unique: true, algorithm: :concurrently
+  end
+end

db/schema.rb

@@ -951,7 +951,7 @@ ActiveRecord::Schema.define(version: 2020_08_18_101700) do
     t.jsonb "data", default: {}, null: false
     t.datetime "updated_at", precision: 6, null: false
     t.bigint "user_id", null: false
-    t.index ["user_id"], name: "index_profiles_on_user_id"
+    t.index ["user_id"], name: "index_profiles_on_user_id", unique: true
   end
 
   create_table "rating_votes", force: :cascade do |t|

lib/data_update_scripts/20200819025131_migrate_profile_data.rb

@@ -0,0 +1,13 @@
+module DataUpdateScripts
+  class MigrateProfileData
+    def run
+      User.find_each do |user|
+        # NOTE: no production users have profiles yet, but we want this script
+        # to be idempotent.
+        next if user.profile.present?
+
+        Profile.create(user: user, data: Profiles::ExtractData.call(user))
+      end
+    end
+  end
+end

lib/tasks/temporary/create_profile_fields.rake

@@ -1,7 +0,0 @@
-namespace :temporary do
-  desc "Create profile fields based on DEV profile" 
-  task create_profile_fields: :environment do
-    csv = Rails.root.join("lib/data/dev_profile_fields.csv")
-    ProfileFields::ImportFromCsv.call(csv)
-  end
-end

spec/factories/profiles.rb

@@ -1,6 +1,5 @@
 FactoryBot.define do
   factory :profile do
-    association :user, factory: :user, strategy: :create
-    data { { name: "John Doe" } }
+    user { association(:user, _skip_creating_profile: true) }
   end
 end

spec/factories/users.rb

@@ -26,8 +26,6 @@ FactoryBot.define do
     bg_color_hex                 { Faker::Color.hex_color }
     text_color_hex               { Faker::Color.hex_color }
 
-    after(:create) { |user| create(:profile, user: user) }
-
     trait :with_identity do
       transient { identities { Authentication::Providers.available } }
 
@@ -161,5 +159,9 @@ FactoryBot.define do
       instagram_url { "www.instagram.com/example" }
       twitch_username { "Example007" }
     end
+
+    trait :without_profile do
+      _skip_creating_profile { true }
+    end
   end
 end

spec/lib/data_update_scripts/migrate_profile_data_spec.rb

@@ -0,0 +1,16 @@
+require "rails_helper"
+require Rails.root.join("lib/data_update_scripts/20200819025131_migrate_profile_data.rb")
+
+describe DataUpdateScripts::MigrateProfileData do
+  it "creates a profile for users who don't have one" do
+    user = create(:user, :without_profile)
+    expect do
+      described_class.new.run
+    end.to change { user.reload.profile }.from(nil).to(an_instance_of(Profile))
+  end
+
+  it "does nothing for users with existing profiles" do
+    user = create(:user)
+    expect { described_class.new.run }.not_to change { user.reload.profile }
+  end
+end

spec/models/profile_spec.rb

@@ -1,7 +1,11 @@
 require "rails_helper"
 
 RSpec.describe Profile, type: :model do
-  it { is_expected.to validate_presence_of(:data) }
+  describe "validations" do
+    subject { create(:profile) }
+
+    it { is_expected.to validate_uniqueness_of(:user_id) }
+  end
 
   context "when accessing profile fields" do
     before do

spec/models/user_spec.rb

@@ -1181,4 +1181,33 @@ RSpec.describe User, type: :model do
       expect(user.authenticated_with_all_providers?).to be(true)
     end
   end
+
+  describe "profiles" do
+    before do
+      create(:profile_field, label: "Available for")
+      Profile.refresh_store_accessors!
+    end
+
+    it "automatically creates a profile for new users", :aggregate_failures do
+      user = create(:user)
+      expect(user.profile).to be_present
+      expect(user.profile).to respond_to(:available_for)
+    end
+
+    it "propagates changes to the profile model", :aggregate_failures do
+      expect do
+        user.update(available_for: "profile migrations")
+      end.to change { user.profile.reload.available_for }.from(nil).to("profile migrations")
+
+      # Changes were also persisted in the users table
+      expect(user.reload.available_for).to eq "profile migrations"
+    end
+
+    it "reads from the profile model, not the user", :aggregate_failures do
+      user.profile.update(available_for: "Well, actually...")
+
+      expect(user.available_for).to eq "Well, actually..."
+      expect(user[:available_for]).to be nil
+    end
+  end
 end