diff --git a/app/models/profile.rb b/app/models/profile.rb index c34e8a5b7..dab8789f4 100644 --- a/app/models/profile.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index e51785d54..42e49c006 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/services/profiles/extract_data.rb b/app/services/profiles/extract_data.rb new file mode 100644 index 000000000..943727c56 --- /dev/null +++ b/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 diff --git a/config/environments/development.rb b/config/environments/development.rb index 6c7cdb5db..757110e10 100644 --- a/config/environments/development.rb +++ b/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"]) diff --git a/config/environments/test.rb b/config/environments/test.rb index 664e2b8e1..f92bf1e48 100644 --- a/config/environments/test.rb +++ b/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 diff --git a/db/migrate/20200813031851_add_unique_index_to_profiles.rb b/db/migrate/20200813031851_add_unique_index_to_profiles.rb new file mode 100644 index 000000000..f7470c3d9 --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index 9391faddf..7af19f871 100644 --- a/db/schema.rb +++ b/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| diff --git a/lib/data_update_scripts/20200819025131_migrate_profile_data.rb b/lib/data_update_scripts/20200819025131_migrate_profile_data.rb new file mode 100644 index 000000000..4a0abc283 --- /dev/null +++ b/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 diff --git a/lib/tasks/temporary/create_profile_fields.rake b/lib/tasks/temporary/create_profile_fields.rake deleted file mode 100644 index 7788cd371..000000000 --- a/lib/tasks/temporary/create_profile_fields.rake +++ /dev/null @@ -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 diff --git a/spec/factories/profiles.rb b/spec/factories/profiles.rb index fe7e5eda0..38787e973 100644 --- a/spec/factories/profiles.rb +++ b/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 diff --git a/spec/factories/users.rb b/spec/factories/users.rb index afed97bec..b81b57c80 100644 --- a/spec/factories/users.rb +++ b/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 diff --git a/spec/lib/data_update_scripts/migrate_profile_data_spec.rb b/spec/lib/data_update_scripts/migrate_profile_data_spec.rb new file mode 100644 index 000000000..320d12cd4 --- /dev/null +++ b/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 diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index 0cce155ca..a0c74b4b4 100644 --- a/spec/models/profile_spec.rb +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 6603ff925..5390a01ac 100644 --- a/spec/models/user_spec.rb +++ b/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