Refactor profile refreshing (#14643)

* Refactor profile refreshing

* Fix guard in ProfileFields::Remove

* Add more comments to Profile

* Fix typo

* Rephrase comment

* Fix regular expression

* Clean up Profile

* Fix remove spec

* Also remove setter from store accessors

* Remove RUBY_THREAD_VM_STACK_SIZE workaround

* Travis, wake up

.travis.yml

@@ -26,7 +26,6 @@ addons:
 env:
   global:
     - RAILS_ENV=test
-    - RUBY_THREAD_VM_STACK_SIZE=5242880
     - DATABASE_URL=postgres://travis@localhost:5433/Forem_prod_test
     - DATABASE_NAME=Forem_prod_test
     - DATABASE_URL_TEST=postgres://travis@localhost:5433/Forem_test

app/models/profile.rb

@@ -6,28 +6,23 @@ class Profile < ApplicationRecord
   validates :website_url, url: { allow_blank: true, no_local: true, schemes: %w[https http] }
   validates_with ProfileValidator
 
+  ATTRIBUTE_NAME_REGEX = /(?<attribute_name>\w+)=?/
+  CACHE_KEY = "profile/attributes".freeze
   # Static fields are columns on the profiles table; they have no relationship
   # to a ProfileField record. These are columns we can safely assume exist for
   # any profile on a given Forem.
   STATIC_FIELDS = %w[summary location website_url].freeze
 
-  # Generates typed accessors for all currently defined profile fields.
+  # Update the Rails cache with the currently available attributes.
   def self.refresh_attributes!
-    return if ENV["ENV_AVAILABLE"] == "false"
-    return unless table_exists?
-
-    ProfileField.find_each do |field|
-      store_attribute :data, field.attribute_name.to_sym, field.type
-    end
+    Rails.cache.delete(CACHE_KEY)
+    attributes
   end
 
-  # Set up all profile attributes when this class loads so all store_attribute
-  # accessors get defined immediately.
-  refresh_attributes!
-
-  # Returns an array of all currently defined `store_attribute`s on `data`.
   def self.attributes
-    (stored_attributes[:data] || []).map(&:to_s)
+    Rails.cache.fetch(CACHE_KEY, expires_in: 24.hours) do
+      ProfileField.pluck(:attribute_name)
+    end
   end
 
   def self.static_fields
@@ -37,4 +32,26 @@ class Profile < ApplicationRecord
   def clear!
     update(data: {})
   end
+
+  # Lazily add accessors for profile fields on first use
+  def method_missing(method_name, *args, **kwargs, &block)
+    match = method_name.match(ATTRIBUTE_NAME_REGEX)
+    field = ProfileField.find_by(attribute_name: match[:attribute_name])
+    super unless field
+
+    self.class.instance_eval do
+      store_attribute :data, field.attribute_name.to_sym, field.type
+    end
+    public_send(method_name, *args, **kwargs, &block)
+  end
+
+  # Defining this is not only a good practice in general, it's also necessary
+  # for `update` to work since the `_assign_attribute` helper it uses performs
+  # an explicit `responds_to?` check.
+  def respond_to_missing?(method_name, include_private = false)
+    match = method_name.match(ATTRIBUTE_NAME_REGEX)
+    return true if match[:attribute_name].in?(self.class.attributes)
+
+    super
+  end
 end

app/services/profile_fields/remove.rb

@@ -16,8 +16,8 @@ module ProfileFields
     def call
       @profile_field = ProfileField.find(@id)
       if @profile_field.destroy
-        accessor = profile_field.attribute_name.to_s
-        Profile.undef_method(accessor) if accessor.in?(Profile.attributes)
+        remove_store_attributes
+        Profile.refresh_attributes!
         @success = true
       else
         @error_message = @profile_field.errors_as_sentence
@@ -28,5 +28,16 @@ module ProfileFields
     def success?
       @success
     end
+
+    private
+
+    def remove_store_attributes
+      accessor = @profile_field.attribute_name.to_sym
+      store_attributes = Profile.stored_attributes.fetch(:data) { [] }
+      return unless accessor.in?(store_attributes)
+
+      Profile.undef_method(accessor)
+      Profile.undef_method("#{accessor}=")
+    end
   end
 end

spec/models/profile_spec.rb

@@ -85,7 +85,6 @@ 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_attributes!
     end
 
     let(:profile) { described_class.new }

spec/requests/admin/profile_fields_spec.rb

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

spec/services/profile_fields/remove_spec.rb

@@ -4,24 +4,20 @@ 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_attributes!
       profile = create(:user).profile
 
-      expect(profile.respond_to?(:removed_field)).to be true
-      expect do
-        described_class.call(profile_field.id)
-      end.to change(ProfileField, :count).by(-1)
-      expect(profile.respond_to?(:removed_field)).to be false
+      expect { described_class.call(profile_field.id) }
+        .to change(ProfileField, :count).by(-1)
+        .and change { profile.respond_to?(:removed_field) }.from(true).to(false)
+        .and change { profile.respond_to?(:removed_field=) }.from(true).to(false)
     end
 
     it "returns the correct response object", :aggregate_failures do
       profile_field = create(:profile_field, label: "Another Removed field")
-      Profile.refresh_attributes!
 
-      add_response = described_class.call(profile_field.id)
-      expect(add_response.success?).to be true
-      expect(add_response.profile_field).to be_an_instance_of(ProfileField)
-      expect(add_response.error_message).to be_blank
+      result = described_class.call(profile_field.id)
+      expect(result.success?).to be true
+      expect(result.error_message).to be_blank
     end
   end
 
@@ -38,10 +34,10 @@ RSpec.describe ProfileFields::Remove, type: :service do
     end
 
     it "returns the correct response object", :aggregate_failures do
-      add_response = described_class.call(id)
-      expect(add_response.success?).to be false
-      expect(add_response.profile_field).to be_present
-      expect(add_response.error_message).to eq "Something went wrong"
+      result = described_class.call(id)
+      expect(result.success?).to be false
+      expect(result.profile_field).to be_present
+      expect(result.error_message).to eq "Something went wrong"
     end
   end
 end