[deploy] Add Profiles::Update service object (#9912)

* Add Profiles::Update service object

* Refactor

* Rename Profile.fields to Profile.attributes

* Keep sync from user -> profile

* And and update comments

* Rename Profile.refresh_store_accessors!

* Make forwarding getters in user dynamic

* Add more explanation

* Fix typo

* Update service object

* Fix return value

* Fix specs

* Travis, let's be friends again?

* Remove Travis change

* Add require_dependency in Profile

* Refactor

* More refactoring

* Avoid double sync

* Fix specs

* Fix mapped attributes sync

app/models/profile.rb

@@ -3,12 +3,40 @@ class Profile < ApplicationRecord
 
   validates :user_id, uniqueness: true
 
-  # This method generates typed accessors for all profile fields
-  def self.refresh_store_accessors!
+  # NOTE: @citizen428 This is a temporary mapping so we don't break DEV during
+  # profile migration/generalization work.
+  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
+  }.with_indifferent_access.freeze
+
+  # 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
     end
   end
 
-  refresh_store_accessors!
+  # Returns an array of all currently defined `store_attribute`s on `data`.
+  def self.attributes
+    stored_attributes[:data] || []
+  end
+
+  # Forces a reload before returning attributes
+  def self.attributes!
+    refresh_attributes!
+    attributes
+  end
+
+  # NOTE: @citizen428 This is a temporary mapping so we don't break DEV during
+  # profile migration/generalization work.
+  def self.mapped_attributes
+    attributes!.map { |attribute| MAPPED_ATTRIBUTES.fetch(attribute, attribute).to_s }
+  end
 end

app/models/user.rb

@@ -7,58 +7,34 @@ class User < ApplicationRecord
 
   # 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
 
+      # NOTE: used for not sync-ing back data to profiles when the profile got
+      # updated first. This will eventually be removed:
+      attr_accessor :_skip_profile_sync
+
       # All new users should automatically have a profile
-      after_create_commit :create_profile, unless: :_skip_creating_profile
+      after_create_commit -> { Profile.create(user: self, data: Profiles::ExtractData.call(self)) },
+                          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
+      after_update_commit :sync_profile
 
-      # 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
+      def sync_profile
+        return if _skip_profile_sync
+        return unless previous_changes.keys.any? { |attribute| attribute.in?(Profile.mapped_attributes) }
 
-      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
+        profile.update(data: Profiles::ExtractData.call(self))
       end
-
-      def create_profile
-        Profile.create(user: self, data: Profiles::ExtractData.call(self))
-      end
-      private :create_profile
+      private :sync_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

app/services/profile_fields/add.rb

@@ -17,7 +17,7 @@ module ProfileFields
       @profile_field = ProfileField.create(@attributes)
       if @profile_field.persisted?
         @success = true
-        Profile.refresh_store_accessors!
+        Profile.refresh_attributes!
       else
         @error_message = @profile_field.errors_as_sentence
       end

app/services/profile_fields/remove.rb

@@ -17,7 +17,7 @@ module ProfileFields
       @profile_field = ProfileField.find(@id)
       if @profile_field.destroy
         accessor = profile_field.attribute_name.to_s
-        Profile.undef_method(accessor) if accessor.in?(Profile.stored_attributes[:data])
+        Profile.undef_method(accessor) if accessor.in?(Profile.attributes)
         @success = true
       else
         @error_message = @profile_field.errors_as_sentence

app/services/profiles/extract_data.rb

@@ -1,45 +1,12 @@
 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!
+      user_attributes = user.attributes
 
-      direct_data = user_attributes.extract!(*DIRECT_ATTRIBUTES)
-      mapped_data = MAPPED_ATTRIBUTES.keys.zip(user_attributes.values_at(*MAPPED_ATTRIBUTES.values)).to_h
+      mapped_attributes = Profile::MAPPED_ATTRIBUTES.transform_values(&:to_s)
+      direct_attributes = Profile.attributes! - mapped_attributes.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

app/services/profiles/update.rb

@@ -0,0 +1,42 @@
+module Profiles
+  class Update
+    def self.call(profile, updated_attributes = {})
+      new(profile, updated_attributes).call
+    end
+
+    attr_reader
+
+    def initialize(profile, updated_attributes)
+      @profile = profile
+      @updated_attributes = updated_attributes
+      @success = false
+    end
+
+    def call
+      # Ensure we have up to date attributes
+      Profile.refresh_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)
+
+      # Before saving, filter out obsolete profile fields
+      @profile.data.slice!(*Profile.attributes)
+
+      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
+      self
+    end
+
+    def success?
+      @success
+    end
+  end
+end

spec/models/profile_spec.rb

@@ -11,7 +11,7 @@ RSpec.describe Profile, type: :model do
     before do
       create(:profile_field, label: "Test 1")
       create(:profile_field, label: "Test 2", input_type: :check_box)
-      described_class.refresh_store_accessors!
+      described_class.refresh_attributes!
     end
 
     let(:profile) { described_class.new }

spec/models/user_spec.rb

@@ -1195,7 +1195,8 @@ RSpec.describe User, type: :model do
   describe "profiles" do
     before do
       create(:profile_field, label: "Available for")
-      Profile.refresh_store_accessors!
+      create(:profile_field, label: "Brand Color 1")
+      Profile.refresh_attributes!
     end
 
     it "automatically creates a profile for new users", :aggregate_failures do
@@ -1204,7 +1205,7 @@ RSpec.describe User, type: :model do
       expect(user.profile).to respond_to(:available_for)
     end
 
-    it "propagates changes to the profile model", :aggregate_failures do
+    it "propagates changes of unmapped attributes 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")
@@ -1213,11 +1214,13 @@ RSpec.describe User, type: :model do
       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...")
+    it "propagates changes of mapped attributes to the profile model", :aggregate_failures do
+      expect do
+        user.update(bg_color_hex: "#abcdef")
+      end.to change { user.profile.reload.brand_color1 }.to("#abcdef")
 
-      expect(user.available_for).to eq "Well, actually..."
-      expect(user[:available_for]).to be nil
+      # Changes were also persisted in the users table
+      expect(user.reload.bg_color_hex).to eq "#abcdef"
     end
   end
 end

spec/requests/admin/profile_fields_spec.rb

@@ -73,7 +73,7 @@ RSpec.describe "/admin/profile_fields", type: :request do
 
   describe "DELETE /admin/profile_fields/:id" do
     let(:profile_field) do
-      create(:profile_field).tap { Profile.refresh_store_accessors! }
+      create(:profile_field).tap { Profile.refresh_attributes! }
     end
 
     it "redirects successfully" do

spec/services/profile_fields/remove_spec.rb

@@ -4,7 +4,7 @@ RSpec.describe ProfileFields::Remove, type: :service do
   context "when successfully removing a profile field" do
     it "removes the profile field and store accessor", :aggregate_failures do
       profile_field = create(:profile_field, label: "Removed field")
-      Profile.refresh_store_accessors!
+      Profile.refresh_attributes!
       profile = create(:user).profile
 
       expect(profile.respond_to?(:removed_field)).to be true
@@ -16,7 +16,7 @@ RSpec.describe ProfileFields::Remove, type: :service do
 
     it "returns the correct response object", :aggregate_failures do
       profile_field = create(:profile_field, label: "Another Removed field")
-      Profile.refresh_store_accessors!
+      Profile.refresh_attributes!
 
       add_response = described_class.call(profile_field.id)
       expect(add_response.success?).to be true

spec/services/profiles/update_spec.rb

@@ -0,0 +1,39 @@
+require "rails_helper"
+
+RSpec.describe Profiles::Update, type: :service do
+  let(:profile) do
+    create(:profile, data: { name: "Sloan Doe", looking_for_work: true, removed: "Bla" })
+  end
+
+  # rubocop:disable RSpec/BeforeAfterAll
+  before(:all) 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"
+    expect(profile.looking_for_work).to be false
+  end
+
+  it "removes old attributes from the profile" do
+    expect do
+      described_class.call(profile, {})
+    end.to change { profile.data.key?("removed") }.to(false)
+  end
+
+  it "propagates changes to user", :agregate_failures do
+    new_name = "Sloan Doe"
+    described_class.call(profile, name: new_name)
+    expect(profile.name).to eq new_name
+    expect(profile.user[:name]).to eq new_name
+  end
+end