diff --git a/app/controllers/settings/providers_controller.rb b/app/controllers/settings/providers_controller.rb index c843d445c..69a52c87c 100644 --- a/app/controllers/settings/providers_controller.rb +++ b/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 diff --git a/app/models/setting.rb b/app/models/setting.rb index 043b17895..dc1b025db 100644 --- a/app/models/setting.rb +++ b/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 diff --git a/test/controllers/settings/providers_controller_test.rb b/test/controllers/settings/providers_controller_test.rb new file mode 100644 index 000000000..7f97d6de0 --- /dev/null +++ b/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