FIX Read-Modify-Write issue with dynamic fields (#290)

* FIX Read-Modify-Write issue with dynamic fields

Ruby caching + queueing updates might cause some dynamic fields to not be updated.

* Small fix for true dynamic fields

* Add suite of tests for new settings page

* Treat nil values as deletions to keep the hash clean

* Test fix

app/controllers/settings/providers_controller.rb

@@ -28,6 +28,9 @@ class Settings::ProvidersController < ApplicationController
 
     updated_fields = []
 
+    # This hash will store only the updates for dynamic (non-declared) fields
+    dynamic_updates = {}
+
     # Perform all updates within a transaction for consistency
     Setting.transaction do
       provider_params.each do |param_key, param_value|
@@ -44,10 +47,42 @@ class Settings::ProvidersController < ApplicationController
           next
         end
 
-        # Set the value using dynamic hash-style access
-        Setting[field.setting_key] = value
+        key_str = field.setting_key.to_s
+
+        # Check if the setting is a declared field in setting.rb
+        # Use method_defined? to check if the setter actually exists on the singleton class,
+        # not just respond_to? which returns true for dynamic fields due to respond_to_missing?
+        if Setting.singleton_class.method_defined?("#{key_str}=")
+          # If it's a declared field (e.g., openai_model), set it directly.
+          # This is safe and uses the proper setter.
+          Setting.public_send("#{key_str}=", value)
+        else
+          # If it's a dynamic field, add it to our batch hash
+          # to avoid the Read-Modify-Write conflict.
+          dynamic_updates[key_str] = value
+        end
+
         updated_fields << param_key
       end
+
+      # Now, if we have any dynamic updates, apply them all at once
+      if dynamic_updates.any?
+        # 1. READ the current hash once
+        current_dynamic = Setting.dynamic_fields.dup
+
+        # 2. MODIFY by merging changes
+        # Treat nil values as deletions to keep the hash clean
+        dynamic_updates.each do |key, value|
+          if value.nil?
+            current_dynamic.delete(key)
+          else
+            current_dynamic[key] = value
+          end
+        end
+
+        # 3. WRITE the complete, merged hash back once
+        Setting.dynamic_fields = current_dynamic
+      end
     end
 
     if updated_fields.any?
@@ -61,6 +96,9 @@ class Settings::ProvidersController < ApplicationController
   rescue => error
     Rails.logger.error("Failed to update provider settings: #{error.message}")
     flash.now[:alert] = "Failed to update provider settings: #{error.message}"
+    # Set @provider_configurations so the view can render properly
+    Provider::Factory.ensure_adapters_loaded
+    @provider_configurations = Provider::ConfigurationRegistry.all
     render :show, status: :unprocessable_entity
   end
 

app/models/setting.rb

@@ -70,10 +70,14 @@ class Setting < RailsSettings::Base
       if respond_to?("#{key_str}=")
         public_send("#{key_str}=", value)
       else
-        # Otherwise, store in dynamic_fields hash
+        # Otherwise, manage in dynamic_fields hash
         current_dynamic = dynamic_fields.dup
-        current_dynamic[key_str] = value
-        self.dynamic_fields = current_dynamic
+        if value.nil?
+          current_dynamic.delete(key_str)          # treat nil as delete
+        else
+          current_dynamic[key_str] = value
+        end
+        self.dynamic_fields = current_dynamic      # persists & busts cache
       end
     end
 

test/controllers/settings/providers_controller_test.rb

@@ -0,0 +1,335 @@
+require "test_helper"
+
+class Settings::ProvidersControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    sign_in users(:family_admin)
+
+    # Ensure provider adapters are loaded for all tests
+    Provider::Factory.ensure_adapters_loaded
+  end
+
+  test "cannot access when self hosting is disabled" do
+    with_env_overrides SELF_HOSTED: "false" do
+      get settings_providers_url
+      assert_response :forbidden
+
+      patch settings_providers_url, params: { setting: { plaid_client_id: "test123" } }
+      assert_response :forbidden
+    end
+  end
+
+  test "should get show when self hosting is enabled" do
+    with_self_hosting do
+      get settings_providers_url
+      assert_response :success
+    end
+  end
+
+  test "correctly identifies declared vs dynamic fields" do
+    # All current provider fields are dynamic, but the logic should correctly
+    # distinguish between declared and dynamic fields
+    with_self_hosting do
+      # plaid_client_id is a dynamic field (not defined in Setting)
+      refute Setting.singleton_class.method_defined?(:plaid_client_id=),
+        "plaid_client_id= should NOT be defined on Setting's singleton class"
+
+      # openai_model IS a declared field (defined in Setting)
+      # but it's not a provider field, so it won't go through this controller
+      assert Setting.singleton_class.method_defined?(:openai_model=),
+        "openai_model= should be defined on Setting's singleton class"
+    end
+  end
+
+  test "updates dynamic provider fields using batch update" do
+    # plaid_client_id is a dynamic field, so it should go through dynamic_fields hash
+    with_self_hosting do
+      # Clear any existing plaid settings
+      Setting.dynamic_fields = {}
+
+      patch settings_providers_url, params: {
+        setting: { plaid_client_id: "test_client_id" }
+      }
+
+      assert_redirected_to settings_providers_url
+      assert_equal "test_client_id", Setting["plaid_client_id"]
+      assert_equal "test_client_id", Setting.dynamic_fields["plaid_client_id"]
+    end
+  end
+
+  test "batches multiple dynamic fields from same provider atomically" do
+    # Test that multiple fields from Plaid are updated together in one write operation
+    with_self_hosting do
+      Setting.dynamic_fields = {}
+
+      patch settings_providers_url, params: {
+        setting: {
+          plaid_client_id: "new_client_id",
+          plaid_secret: "new_secret",
+          plaid_environment: "production"
+        }
+      }
+
+      assert_redirected_to settings_providers_url
+
+      # All three should be present in dynamic_fields
+      assert_equal "new_client_id", Setting["plaid_client_id"]
+      assert_equal "new_secret", Setting["plaid_secret"]
+      assert_equal "production", Setting["plaid_environment"]
+    end
+  end
+
+  test "batches dynamic fields from multiple providers atomically" do
+    # Test that fields from different providers are all batched together
+    with_self_hosting do
+      Setting.dynamic_fields = {}
+
+      patch settings_providers_url, params: {
+        setting: {
+          plaid_client_id: "plaid_client",
+          plaid_secret: "plaid_secret",
+          plaid_eu_client_id: "plaid_eu_client",
+          plaid_eu_secret: "plaid_eu_secret",
+          simplefin_setup_token: "simplefin_token"
+        }
+      }
+
+      assert_redirected_to settings_providers_url
+
+      # All fields should be present
+      assert_equal "plaid_client", Setting["plaid_client_id"]
+      assert_equal "plaid_secret", Setting["plaid_secret"]
+      assert_equal "plaid_eu_client", Setting["plaid_eu_client_id"]
+      assert_equal "plaid_eu_secret", Setting["plaid_eu_secret"]
+      assert_equal "simplefin_token", Setting["simplefin_setup_token"]
+    end
+  end
+
+  test "preserves existing dynamic fields when updating new ones" do
+    # Test that updating some fields doesn't overwrite other existing fields
+    with_self_hosting do
+      # Set initial fields
+      Setting.dynamic_fields = {
+        "existing_field_1" => "value1",
+        "plaid_client_id" => "old_client_id"
+      }
+
+      # Update one field and add a new one
+      patch settings_providers_url, params: {
+        setting: {
+          plaid_client_id: "new_client_id",
+          plaid_secret: "new_secret"
+        }
+      }
+
+      assert_redirected_to settings_providers_url
+
+      # Existing unrelated field should still be there
+      assert_equal "value1", Setting["existing_field_1"]
+
+      # Updated field should have new value
+      assert_equal "new_client_id", Setting["plaid_client_id"]
+
+      # New field should be added
+      assert_equal "new_secret", Setting["plaid_secret"]
+    end
+  end
+
+  test "skips placeholder values for secret fields" do
+    with_self_hosting do
+      # Set an initial secret value
+      Setting["plaid_secret"] = "real_secret"
+
+      # Try to update with placeholder
+      patch settings_providers_url, params: {
+        setting: {
+          plaid_client_id: "new_client_id",
+          plaid_secret: "********"  # Placeholder value
+        }
+      }
+
+      assert_redirected_to settings_providers_url
+
+      # Client ID should be updated
+      assert_equal "new_client_id", Setting["plaid_client_id"]
+
+      # Secret should remain unchanged
+      assert_equal "real_secret", Setting["plaid_secret"]
+    end
+  end
+
+  test "converts blank values to nil and removes from dynamic_fields" do
+    with_self_hosting do
+      # Set initial values
+      Setting["plaid_client_id"] = "old_value"
+      assert_equal "old_value", Setting["plaid_client_id"]
+      assert Setting.dynamic_fields.key?("plaid_client_id")
+
+      patch settings_providers_url, params: {
+        setting: { plaid_client_id: "  " }  # Blank string with spaces
+      }
+
+      assert_redirected_to settings_providers_url
+      assert_nil Setting["plaid_client_id"]
+      # Key should be removed from hash, not just set to nil
+      refute Setting.dynamic_fields.key?("plaid_client_id"),
+        "nil values should delete the key from dynamic_fields"
+    end
+  end
+
+  test "handles sequential updates to different dynamic fields safely" do
+    # This test simulates what would happen if two requests tried to update
+    # different dynamic fields sequentially. With the batch update approach,
+    # all changes should be preserved.
+    with_self_hosting do
+      Setting.dynamic_fields = { "existing_field" => "existing_value" }
+
+      # Simulate first request updating plaid fields
+      patch settings_providers_url, params: {
+        setting: {
+          plaid_client_id: "client_id_1",
+          plaid_secret: "secret_1"
+        }
+      }
+
+      # Existing field should still be there
+      assert_equal "existing_value", Setting["existing_field"]
+
+      # New fields should be added
+      assert_equal "client_id_1", Setting["plaid_client_id"]
+      assert_equal "secret_1", Setting["plaid_secret"]
+
+      # Simulate second request updating simplefin fields
+      patch settings_providers_url, params: {
+        setting: {
+          simplefin_setup_token: "token_1"
+        }
+      }
+
+      # All previously set fields should still be there
+      assert_equal "existing_value", Setting["existing_field"]
+      assert_equal "client_id_1", Setting["plaid_client_id"]
+      assert_equal "secret_1", Setting["plaid_secret"]
+      assert_equal "token_1", Setting["simplefin_setup_token"]
+    end
+  end
+
+  test "only processes valid configuration fields" do
+    with_self_hosting do
+      # Try to update a field that doesn't exist in any provider configuration
+      patch settings_providers_url, params: {
+        setting: {
+          plaid_client_id: "valid_field",
+          fake_field_that_does_not_exist: "should_be_ignored"
+        }
+      }
+
+      assert_redirected_to settings_providers_url
+
+      # Valid field should be updated
+      assert_equal "valid_field", Setting["plaid_client_id"]
+
+      # Invalid field should not be stored
+      assert_nil Setting["fake_field_that_does_not_exist"]
+    end
+  end
+
+  test "calls reload_configuration on updated providers" do
+    with_self_hosting do
+      # Mock the adapter class to verify reload_configuration is called
+      Provider::PlaidAdapter.expects(:reload_configuration).once
+
+      patch settings_providers_url, params: {
+        setting: { plaid_client_id: "new_client_id" }
+      }
+
+      assert_redirected_to settings_providers_url
+    end
+  end
+
+  test "reloads configuration for multiple providers when updated" do
+    with_self_hosting do
+      # Both providers should have their configuration reloaded
+      Provider::PlaidAdapter.expects(:reload_configuration).once
+      Provider::SimplefinAdapter.expects(:reload_configuration).once
+
+      patch settings_providers_url, params: {
+        setting: {
+          plaid_client_id: "plaid_client",
+          simplefin_setup_token: "simplefin_token"
+        }
+      }
+
+      assert_redirected_to settings_providers_url
+    end
+  end
+
+  test "logs errors when update fails" do
+    with_self_hosting do
+      # Test that errors during update are properly logged and handled gracefully
+      # We'll force an error by making the dynamic_fields= setter raise
+      Setting.expects(:dynamic_fields=).raises(StandardError.new("Database error")).once
+
+      # Mock logger to verify error is logged
+      Rails.logger.expects(:error).with(regexp_matches(/Failed to update provider settings.*Database error/)).once
+
+      patch settings_providers_url, params: {
+        setting: { plaid_client_id: "test" }
+      }
+
+      # Controller should handle the error gracefully
+      assert_response :unprocessable_entity
+      assert_equal "Failed to update provider settings: Database error", flash[:alert]
+    end
+  end
+
+  test "shows no changes message when no fields are updated" do
+    with_self_hosting do
+      # Only send a secret field with placeholder value (which gets skipped)
+      Setting["plaid_secret"] = "existing_secret"
+
+      patch settings_providers_url, params: {
+        setting: { plaid_secret: "********" }
+      }
+
+      assert_redirected_to settings_providers_url
+      assert_equal "No changes were made", flash[:notice]
+    end
+  end
+
+  test "non-admin users cannot update providers" do
+    with_self_hosting do
+      sign_in users(:family_member)
+
+      patch settings_providers_url, params: {
+        setting: { plaid_client_id: "test" }
+      }
+
+      assert_redirected_to settings_providers_path
+      assert_equal "Not authorized", flash[:alert]
+
+      # Value should not have changed
+      assert_nil Setting["plaid_client_id"]
+    end
+  end
+
+  test "uses singleton_class method_defined to detect declared fields" do
+    with_self_hosting do
+      # This test verifies the difference between respond_to? and singleton_class.method_defined?
+
+      # openai_model is a declared field
+      assert Setting.singleton_class.method_defined?(:openai_model=),
+        "openai_model= should be defined on Setting's singleton class"
+      assert Setting.respond_to?(:openai_model=),
+        "respond_to? should return true for declared field"
+
+      # plaid_client_id is a dynamic field
+      refute Setting.singleton_class.method_defined?(:plaid_client_id=),
+        "plaid_client_id= should NOT be defined on Setting's singleton class"
+      refute Setting.respond_to?(:plaid_client_id=),
+        "respond_to? should return false for dynamic field"
+
+      # Both methods currently return the same result, but singleton_class.method_defined?
+      # is more explicit and reliable for checking if a method is actually defined
+    end
+  end
+end