From d284e597cdec4fa7fca179b46b9d65bb0a989eea Mon Sep 17 00:00:00 2001 From: Michael Kohl Date: Thu, 3 Sep 2020 20:19:25 +0700 Subject: [PATCH] [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 | 34 +++++++++++++-- app/models/user.rb | 48 ++++++--------------- app/services/profile_fields/add.rb | 2 +- app/services/profile_fields/remove.rb | 2 +- app/services/profiles/extract_data.rb | 43 +++--------------- app/services/profiles/update.rb | 42 ++++++++++++++++++ spec/models/profile_spec.rb | 2 +- spec/models/user_spec.rb | 15 ++++--- spec/requests/admin/profile_fields_spec.rb | 2 +- spec/services/profile_fields/remove_spec.rb | 4 +- spec/services/profiles/update_spec.rb | 39 +++++++++++++++++ 11 files changed, 144 insertions(+), 89 deletions(-) create mode 100644 app/services/profiles/update.rb create mode 100644 spec/services/profiles/update_spec.rb diff --git a/app/models/profile.rb b/app/models/profile.rb index e3ad58dea..f9f7b4ab6 100644 --- a/app/models/profile.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index b0c040f9e..988c3aa30 100644 --- a/app/models/user.rb +++ b/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 diff --git a/app/services/profile_fields/add.rb b/app/services/profile_fields/add.rb index eaa3dd3fa..e4e3c59b0 100644 --- a/app/services/profile_fields/add.rb +++ b/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 diff --git a/app/services/profile_fields/remove.rb b/app/services/profile_fields/remove.rb index a4172b75c..81d1691be 100644 --- a/app/services/profile_fields/remove.rb +++ b/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 diff --git a/app/services/profiles/extract_data.rb b/app/services/profiles/extract_data.rb index 943727c56..fc3ba803e 100644 --- a/app/services/profiles/extract_data.rb +++ b/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 diff --git a/app/services/profiles/update.rb b/app/services/profiles/update.rb new file mode 100644 index 000000000..74c472481 --- /dev/null +++ b/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 diff --git a/spec/models/profile_spec.rb b/spec/models/profile_spec.rb index 58c360726..359494d3b 100644 --- a/spec/models/profile_spec.rb +++ b/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 } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index b69de85f2..3b0d92446 100644 --- a/spec/models/user_spec.rb +++ b/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 diff --git a/spec/requests/admin/profile_fields_spec.rb b/spec/requests/admin/profile_fields_spec.rb index 70aacfff0..c2671c18d 100644 --- a/spec/requests/admin/profile_fields_spec.rb +++ b/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 diff --git a/spec/services/profile_fields/remove_spec.rb b/spec/services/profile_fields/remove_spec.rb index 4e4d75726..6d621917f 100644 --- a/spec/services/profile_fields/remove_spec.rb +++ b/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 diff --git a/spec/services/profiles/update_spec.rb b/spec/services/profiles/update_spec.rb new file mode 100644 index 000000000..747dcb224 --- /dev/null +++ b/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