From 036991134b27cb8931f5816f6c1b0e82b75104fd Mon Sep 17 00:00:00 2001 From: Wabo Date: Wed, 3 Jun 2026 16:11:50 -0400 Subject: [PATCH] Fix SMS settings page: toggle-driven visibility, password merging, background save - Wrap provider section in #sms_provider_section, hidden when SMS disabled - Extend embedded JS to sync section visibility with enable toggle - Add Turbo frame attribute for background saves without full page reload - Fix password field merging: preserve existing secrets when blank submitted - Expand system spec with behavioral tests: toggle visibility, provider switching, saving - Add request spec for controller actions: auth, password retention, test_message Co-Authored-By: Claude Sonnet 4.6 --- app/controllers/sms_settings_controller.rb | 15 +- app/views/sms_settings/index.html.erb | 208 +++++++++++---------- spec/requests/sms_settings_spec.rb | 170 +++++++++++++++++ spec/system/sms_settings_spec.rb | 128 +++++++++++++ 4 files changed, 423 insertions(+), 98 deletions(-) create mode 100644 spec/requests/sms_settings_spec.rb diff --git a/app/controllers/sms_settings_controller.rb b/app/controllers/sms_settings_controller.rb index 3406e617..814c7615 100644 --- a/app/controllers/sms_settings_controller.rb +++ b/app/controllers/sms_settings_controller.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true class SmsSettingsController < ApplicationController + PASSWORD_FIELDS = %w[basic_auth_token twilio_auth_token voipms_api_password signalwire_api_token].freeze + private_constant :PASSWORD_FIELDS + before_action :load_encrypted_config authorize_resource :encrypted_config, only: :index authorize_resource :encrypted_config, parent: false, only: %i[create test_message] @@ -46,7 +49,7 @@ class SmsSettingsController < ApplicationController end def build_sms_value - params.require(:encrypted_config).require(:value).permit( + permitted = params.require(:encrypted_config).require(:value).permit( :enabled, :provider, :basic_auth_token, @@ -62,6 +65,14 @@ class SmsSettingsController < ApplicationController :signalwire_project_id, :signalwire_api_token, :signalwire_from - ) + ).to_h + + existing = @encrypted_config.value.is_a?(Hash) ? @encrypted_config.value : {} + + PASSWORD_FIELDS.each do |field| + permitted[field] = existing[field] if permitted[field].blank? && existing[field].present? + end + + permitted end end diff --git a/app/views/sms_settings/index.html.erb b/app/views/sms_settings/index.html.erb index 055b1fa6..75e6c6f3 100644 --- a/app/views/sms_settings/index.html.erb +++ b/app/views/sms_settings/index.html.erb @@ -60,7 +60,9 @@ <% end %> - <%= form_for @encrypted_config, url: settings_sms_path, method: :post, html: { autocomplete: 'off', class: 'space-y-4', id: 'sms_settings_form' } do |f| %> + <%= form_for @encrypted_config, url: settings_sms_path, method: :post, + html: { autocomplete: 'off', class: 'space-y-4', id: 'sms_settings_form' }, + data: { turbo_frame: :_top } do |f| %> <%= f.fields_for :value do |ff| %>
-
- <%= ff.label :provider, 'Provider', class: 'label' %> - <%= ff.select :provider, - Sms::SUPPORTED_PROVIDERS.map { |p| [provider_labels[p] || p, p] }, - { selected: selected_provider }, - class: 'base-select', - id: 'sms_provider_select' %> -
- -
-
- <%= ff.label :basic_auth_token, 'BulkVS Basic Auth Token', class: 'label' %> - <%= ff.password_field :basic_auth_token, value: '', class: 'base-input', placeholder: value['basic_auth_token'].present? ? '*************' : 'Paste from BulkVS portal' %> - <% if value['basic_auth_token'].present? %> - Leave blank to keep the saved token. - <% else %> - In the BulkVS portal, open the API tab and copy the pre-encoded Basic Auth header value (do not include "Basic "). - <% end %> -
-
- <%= ff.label :from_number, 'From Number', class: 'label' %> - <%= ff.text_field :from_number, value: value['from_number'], class: 'base-input', placeholder: '15551234567' %> - E.164 format (digits only, country code first; e.g. 15551234567). -
+
- <%= ff.label :delivery_webhook_url, 'Delivery Status Webhook (optional)', class: 'label' %> - <%= ff.url_field :delivery_webhook_url, value: value['delivery_webhook_url'], class: 'base-input', placeholder: 'https://your-app.example/webhooks/sms' %> - If set, BulkVS will POST delivery-status events here for each message. + <%= ff.label :provider, 'Provider', class: 'label' %> + <%= ff.select :provider, + Sms::SUPPORTED_PROVIDERS.map { |p| [provider_labels[p] || p, p] }, + { selected: selected_provider }, + class: 'base-select', + id: 'sms_provider_select' %>
-
-
-
- <%= ff.label :twilio_account_sid, 'Twilio Account SID', class: 'label' %> - <%= ff.text_field :twilio_account_sid, value: value['twilio_account_sid'], class: 'base-input', placeholder: 'AC...' %> - From your Twilio Console "Account Info" panel. -
-
- <%= ff.label :twilio_auth_token, 'Twilio Auth Token', class: 'label' %> - <%= ff.password_field :twilio_auth_token, value: '', class: 'base-input', placeholder: value['twilio_auth_token'].present? ? '*************' : 'Click "show" in the Console to reveal' %> - <% if value['twilio_auth_token'].present? %> - Leave blank to keep the saved token. - <% else %> - Found next to the Account SID in the Twilio Console. - <% end %> -
-
- <%= ff.label :twilio_from, 'From Number', class: 'label' %> - <%= ff.text_field :twilio_from, value: value['twilio_from'], class: 'base-input', placeholder: '+15551234567' %> - Twilio number purchased in Phone Numbers → Manage. Use full E.164 with leading +. +
+
+ <%= ff.label :basic_auth_token, 'BulkVS Basic Auth Token', class: 'label' %> + <%= ff.password_field :basic_auth_token, value: '', class: 'base-input', placeholder: value['basic_auth_token'].present? ? '*************' : 'Paste from BulkVS portal' %> + <% if value['basic_auth_token'].present? %> + Leave blank to keep the saved token. + <% else %> + In the BulkVS portal, open the API tab and copy the pre-encoded Basic Auth header value (do not include "Basic "). + <% end %> +
+
+ <%= ff.label :from_number, 'From Number', class: 'label' %> + <%= ff.text_field :from_number, value: value['from_number'], class: 'base-input', placeholder: '15551234567' %> + E.164 format (digits only, country code first; e.g. 15551234567). +
+
+ <%= ff.label :delivery_webhook_url, 'Delivery Status Webhook (optional)', class: 'label' %> + <%= ff.url_field :delivery_webhook_url, value: value['delivery_webhook_url'], class: 'base-input', placeholder: 'https://your-app.example/webhooks/sms' %> + If set, BulkVS will POST delivery-status events here for each message. +
-
-
-
- <%= ff.label :voipms_api_username, 'API Username', class: 'label' %> - <%= ff.text_field :voipms_api_username, value: value['voipms_api_username'], class: 'base-input', placeholder: 'your-account@example.com' %> - Your VoIP.ms portal login email. -
-
- <%= ff.label :voipms_api_password, 'API Password', class: 'label' %> - <%= ff.password_field :voipms_api_password, value: '', class: 'base-input', placeholder: value['voipms_api_password'].present? ? '*************' : 'Set this at voip.ms/m/api.php' %> - <% if value['voipms_api_password'].present? %> - Leave blank to keep the saved password. - <% else %> - Set the dedicated API password at voip.ms/m/api.php — this is not your portal login password. On the same page, enable API access and whitelist this server's egress IP, or every call will fail with ip_not_authorized. - <% end %> -
-
- <%= ff.label :voipms_did, 'DID (Sending Number)', class: 'label' %> - <%= ff.text_field :voipms_did, value: value['voipms_did'], class: 'base-input', placeholder: '5551234567' %> - An SMS-enabled DID from Manage DIDs. Digits only, no +. The DID must have the SMS feature enabled. +
+
+ <%= ff.label :twilio_account_sid, 'Twilio Account SID', class: 'label' %> + <%= ff.text_field :twilio_account_sid, value: value['twilio_account_sid'], class: 'base-input', placeholder: 'AC...' %> + From your Twilio Console "Account Info" panel. +
+
+ <%= ff.label :twilio_auth_token, 'Twilio Auth Token', class: 'label' %> + <%= ff.password_field :twilio_auth_token, value: '', class: 'base-input', placeholder: value['twilio_auth_token'].present? ? '*************' : 'Click "show" in the Console to reveal' %> + <% if value['twilio_auth_token'].present? %> + Leave blank to keep the saved token. + <% else %> + Found next to the Account SID in the Twilio Console. + <% end %> +
+
+ <%= ff.label :twilio_from, 'From Number', class: 'label' %> + <%= ff.text_field :twilio_from, value: value['twilio_from'], class: 'base-input', placeholder: '+15551234567' %> + Twilio number purchased in Phone Numbers → Manage. Use full E.164 with leading +. +
-
-
-
- <%= ff.label :signalwire_space_url, 'Space URL', class: 'label' %> - <%= ff.text_field :signalwire_space_url, value: value['signalwire_space_url'], class: 'base-input', placeholder: 'yourname.signalwire.com' %> - From Dashboard → API. Omit https://. -
-
- <%= ff.label :signalwire_project_id, 'Project ID', class: 'label' %> - <%= ff.text_field :signalwire_project_id, value: value['signalwire_project_id'], class: 'base-input', placeholder: '00000000-0000-0000-0000-000000000000' %> - The UUID labelled "Your Project ID" on the API tab. -
-
- <%= ff.label :signalwire_api_token, 'API Token', class: 'label' %> - <%= ff.password_field :signalwire_api_token, value: '', class: 'base-input', placeholder: value['signalwire_api_token'].present? ? '*************' : 'PT...' %> - <% if value['signalwire_api_token'].present? %> - Leave blank to keep the saved token. - <% else %> - Generate on the API tab. The token must have the Messaging scope enabled or sends return 401. - <% end %> +
+
+ <%= ff.label :voipms_api_username, 'API Username', class: 'label' %> + <%= ff.text_field :voipms_api_username, value: value['voipms_api_username'], class: 'base-input', placeholder: 'your-account@example.com' %> + Your VoIP.ms portal login email. +
+
+ <%= ff.label :voipms_api_password, 'API Password', class: 'label' %> + <%= ff.password_field :voipms_api_password, value: '', class: 'base-input', placeholder: value['voipms_api_password'].present? ? '*************' : 'Set this at voip.ms/m/api.php' %> + <% if value['voipms_api_password'].present? %> + Leave blank to keep the saved password. + <% else %> + Set the dedicated API password at voip.ms/m/api.php — this is not your portal login password. On the same page, enable API access and whitelist this server's egress IP, or every call will fail with ip_not_authorized. + <% end %> +
+
+ <%= ff.label :voipms_did, 'DID (Sending Number)', class: 'label' %> + <%= ff.text_field :voipms_did, value: value['voipms_did'], class: 'base-input', placeholder: '5551234567' %> + An SMS-enabled DID from Manage DIDs. Digits only, no +. The DID must have the SMS feature enabled. +
-
- <%= ff.label :signalwire_from, 'From Number', class: 'label' %> - <%= ff.text_field :signalwire_from, value: value['signalwire_from'], class: 'base-input', placeholder: '+15551234567' %> - A SignalWire number from Phone Numbers. Full E.164 with leading +. + +
+
+ <%= ff.label :signalwire_space_url, 'Space URL', class: 'label' %> + <%= ff.text_field :signalwire_space_url, value: value['signalwire_space_url'], class: 'base-input', placeholder: 'yourname.signalwire.com' %> + From Dashboard → API. Omit https://. +
+
+ <%= ff.label :signalwire_project_id, 'Project ID', class: 'label' %> + <%= ff.text_field :signalwire_project_id, value: value['signalwire_project_id'], class: 'base-input', placeholder: '00000000-0000-0000-0000-000000000000' %> + The UUID labelled "Your Project ID" on the API tab. +
+
+ <%= ff.label :signalwire_api_token, 'API Token', class: 'label' %> + <%= ff.password_field :signalwire_api_token, value: '', class: 'base-input', placeholder: value['signalwire_api_token'].present? ? '*************' : 'PT...' %> + <% if value['signalwire_api_token'].present? %> + Leave blank to keep the saved token. + <% else %> + Generate on the API tab. The token must have the Messaging scope enabled or sends return 401. + <% end %> +
+
+ <%= ff.label :signalwire_from, 'From Number', class: 'label' %> + <%= ff.text_field :signalwire_from, value: value['signalwire_from'], class: 'base-input', placeholder: '+15551234567' %> + A SignalWire number from Phone Numbers. Full E.164 with leading +. +
<% end %> @@ -206,16 +210,28 @@ } } ready(function() { - var select = document.getElementById('sms_provider_select'); + var toggle = document.getElementById('encrypted_config_value_enabled'); + var section = document.getElementById('sms_provider_section'); + var select = document.getElementById('sms_provider_select'); if (!select) return; - function sync() { + + function syncSection() { + if (!section || !toggle) return; + section.classList.toggle('hidden', !toggle.checked); + } + + function syncProvider() { var v = select.value; document.querySelectorAll('[data-provider-block]').forEach(function(b) { b.classList.toggle('hidden', b.dataset.providerBlock !== v); }); } - select.addEventListener('change', sync); - sync(); + + if (toggle) toggle.addEventListener('change', syncSection); + select.addEventListener('change', syncProvider); + + syncSection(); + syncProvider(); }); })(); diff --git a/spec/requests/sms_settings_spec.rb b/spec/requests/sms_settings_spec.rb new file mode 100644 index 00000000..67c16233 --- /dev/null +++ b/spec/requests/sms_settings_spec.rb @@ -0,0 +1,170 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'SMS Settings', type: :request do + let!(:account) { create(:account) } + let!(:admin) { create(:user, account:, role: User::ADMIN_ROLE, email: 'admin@wabo.cc') } + let!(:editor) { create(:user, account:, role: User::EDITOR_ROLE, email: 'editor@wabo.cc') } + + describe 'GET /settings/sms' do + it 'renders ok for admin' do + sign_in admin + get settings_sms_path + + expect(response).to have_http_status(:ok) + end + + it 'redirects editor to root' do + sign_in editor + get settings_sms_path + + expect(response).to redirect_to(root_path) + end + end + + describe 'POST /settings/sms (create)' do + before { sign_in admin } + + it 'creates a new SMS config and redirects with notice' do + expect { + post settings_sms_path, params: { + encrypted_config: { + value: { + enabled: '1', + provider: 'bulkvs', + basic_auth_token: 'tok123', + from_number: '15551234567', + delivery_webhook_url: '' + } + } + } + }.to change(EncryptedConfig, :count).by(1) + + expect(response).to redirect_to(settings_sms_path) + follow_redirect! + expect(response.body).to include(I18n.t('changes_have_been_saved')) + + config = EncryptedConfig.find_by(account:, key: EncryptedConfig::SMS_CONFIGS_KEY) + expect(config.value['basic_auth_token']).to eq('tok123') + expect(config.value['from_number']).to eq('15551234567') + end + + it 'preserves existing Twilio auth token when blank is submitted' do + create(:encrypted_config, account:, key: EncryptedConfig::SMS_CONFIGS_KEY, + value: { 'enabled' => true, 'provider' => 'twilio', + 'twilio_account_sid' => 'AC123', + 'twilio_auth_token' => 'keep_me', + 'twilio_from' => '+15551234567' }) + + post settings_sms_path, params: { + encrypted_config: { + value: { + enabled: '1', + provider: 'twilio', + twilio_account_sid: 'AC123', + twilio_auth_token: '', + twilio_from: '+15551234567' + } + } + } + + config = EncryptedConfig.find_by(account:, key: EncryptedConfig::SMS_CONFIGS_KEY) + expect(config.value['twilio_auth_token']).to eq('keep_me') + end + + it 'preserves existing BulkVS basic_auth_token when blank is submitted' do + create(:encrypted_config, account:, key: EncryptedConfig::SMS_CONFIGS_KEY, + value: { 'enabled' => true, 'provider' => 'bulkvs', + 'basic_auth_token' => 'keep_me', + 'from_number' => '15551234567' }) + + post settings_sms_path, params: { + encrypted_config: { + value: { enabled: '1', provider: 'bulkvs', basic_auth_token: '', from_number: '15551234567' } + } + } + + config = EncryptedConfig.find_by(account:, key: EncryptedConfig::SMS_CONFIGS_KEY) + expect(config.value['basic_auth_token']).to eq('keep_me') + end + + it 'preserves existing VoIP.ms API password when blank is submitted' do + create(:encrypted_config, account:, key: EncryptedConfig::SMS_CONFIGS_KEY, + value: { 'enabled' => true, 'provider' => 'voipms', + 'voipms_api_username' => 'user@example.com', + 'voipms_api_password' => 'keep_me', + 'voipms_did' => '5551234567' }) + + post settings_sms_path, params: { + encrypted_config: { + value: { + enabled: '1', provider: 'voipms', + voipms_api_username: 'user@example.com', voipms_api_password: '', voipms_did: '5551234567' + } + } + } + + config = EncryptedConfig.find_by(account:, key: EncryptedConfig::SMS_CONFIGS_KEY) + expect(config.value['voipms_api_password']).to eq('keep_me') + end + + it 'preserves existing SignalWire API token when blank is submitted' do + create(:encrypted_config, account:, key: EncryptedConfig::SMS_CONFIGS_KEY, + value: { 'enabled' => true, 'provider' => 'signalwire', + 'signalwire_space_url' => 'test.signalwire.com', + 'signalwire_project_id' => 'uuid-1234', + 'signalwire_api_token' => 'keep_me', + 'signalwire_from' => '+15551234567' }) + + post settings_sms_path, params: { + encrypted_config: { + value: { + enabled: '1', provider: 'signalwire', + signalwire_space_url: 'test.signalwire.com', signalwire_project_id: 'uuid-1234', + signalwire_api_token: '', signalwire_from: '+15551234567' + } + } + } + + config = EncryptedConfig.find_by(account:, key: EncryptedConfig::SMS_CONFIGS_KEY) + expect(config.value['signalwire_api_token']).to eq('keep_me') + end + + it 'redirects editor to root' do + sign_in editor + post settings_sms_path, params: { + encrypted_config: { value: { enabled: '1', provider: 'bulkvs' } } + } + + expect(response).to redirect_to(root_path) + end + end + + describe 'POST /settings/sms/test_message' do + before { sign_in admin } + + it 'redirects with alert when phone is blank' do + post test_message_settings_sms_path, params: { phone: '' } + + expect(response).to redirect_to(settings_sms_path) + follow_redirect! + expect(response.body).to include('Enter a phone number') + end + + it 'redirects with alert when SMS is not configured' do + post test_message_settings_sms_path, params: { phone: '15551234567' } + + expect(response).to redirect_to(settings_sms_path) + follow_redirect! + expect(response.body).to include('Test failed') + end + + it 'redirects editor to root' do + sign_in editor + post test_message_settings_sms_path, params: { phone: '15551234567' } + + expect(response).to redirect_to(root_path) + end + end +end diff --git a/spec/system/sms_settings_spec.rb b/spec/system/sms_settings_spec.rb index 12fdb434..bb992c3a 100644 --- a/spec/system/sms_settings_spec.rb +++ b/spec/system/sms_settings_spec.rb @@ -42,4 +42,132 @@ RSpec.describe 'SMS Settings' do expect(page).to have_content('SMS is enabled') expect(page).to have_content('Send a test SMS') end + + describe 'enable toggle visibility' do + context 'when SMS is disabled (no saved config)' do + it 'hides the provider section on page load' do + visit settings_sms_path + + expect(page).to have_css('#sms_provider_section.hidden', visible: :hidden) + end + end + + context 'when SMS is enabled' do + before do + create(:encrypted_config, account:, key: EncryptedConfig::SMS_CONFIGS_KEY, + value: { 'enabled' => true, 'provider' => 'bulkvs', + 'basic_auth_token' => 'tok', 'from_number' => '15551234567' }) + end + + it 'shows the provider section on page load' do + visit settings_sms_path + + expect(page).to have_css('#sms_provider_section', visible: :visible) + end + end + + it 'shows the provider section when the toggle is turned on' do + visit settings_sms_path + + expect(page).to have_css('#sms_provider_section.hidden', visible: :hidden) + + find('#encrypted_config_value_enabled').click + + expect(page).to have_css('#sms_provider_section', visible: :visible) + end + + it 'hides the provider section when the toggle is turned off' do + create(:encrypted_config, account:, key: EncryptedConfig::SMS_CONFIGS_KEY, + value: { 'enabled' => true, 'provider' => 'bulkvs', + 'basic_auth_token' => 'tok', 'from_number' => '15551234567' }) + + visit settings_sms_path + + expect(page).to have_css('#sms_provider_section', visible: :visible) + + find('#encrypted_config_value_enabled').click + + expect(page).to have_css('#sms_provider_section.hidden', visible: :hidden) + end + end + + describe 'provider switching' do + before do + create(:encrypted_config, account:, key: EncryptedConfig::SMS_CONFIGS_KEY, + value: { 'enabled' => true, 'provider' => 'bulkvs', + 'basic_auth_token' => 'tok', 'from_number' => '15551234567' }) + visit settings_sms_path + end + + it 'shows only the BulkVS block when BulkVS is the saved provider' do + expect(page).to have_css('[data-provider-block="bulkvs"]', visible: :visible) + expect(page).to have_css('[data-provider-block="twilio"]', visible: :hidden) + expect(page).to have_css('[data-provider-block="voipms"]', visible: :hidden) + expect(page).to have_css('[data-provider-block="signalwire"]', visible: :hidden) + end + + it 'switches to Twilio fields when Twilio is selected' do + select 'Twilio', from: 'encrypted_config[value][provider]' + + expect(page).to have_css('[data-provider-block="twilio"]', visible: :visible) + expect(page).to have_css('[data-provider-block="bulkvs"]', visible: :hidden) + end + + it 'switches to VoIP.ms fields when VoIP.ms is selected' do + select 'VoIP.ms', from: 'encrypted_config[value][provider]' + + expect(page).to have_css('[data-provider-block="voipms"]', visible: :visible) + expect(page).to have_css('[data-provider-block="twilio"]', visible: :hidden) + end + + it 'switches to SignalWire fields when SignalWire is selected' do + select 'SignalWire', from: 'encrypted_config[value][provider]' + + expect(page).to have_css('[data-provider-block="signalwire"]', visible: :visible) + expect(page).to have_css('[data-provider-block="bulkvs"]', visible: :hidden) + end + end + + describe 'saving settings' do + it 'saves BulkVS configuration and shows success flash' do + visit settings_sms_path + + find('#encrypted_config_value_enabled').click + + within('[data-provider-block="bulkvs"]') do + fill_in 'BulkVS Basic Auth Token', with: 'mytoken123' + fill_in 'From Number', with: '15551234567' + end + + expect { click_button 'Save' }.to change(EncryptedConfig, :count).by(1) + + expect(page).to have_content('Changes have been saved') + + config = EncryptedConfig.find_by(account:, key: EncryptedConfig::SMS_CONFIGS_KEY) + expect(config.value['enabled']).to eq('1') + expect(config.value['basic_auth_token']).to eq('mytoken123') + expect(config.value['from_number']).to eq('15551234567') + end + + it 'retains existing Twilio auth token when left blank on re-save' do + create(:encrypted_config, account:, key: EncryptedConfig::SMS_CONFIGS_KEY, + value: { 'enabled' => true, 'provider' => 'twilio', + 'twilio_account_sid' => 'AC123', + 'twilio_auth_token' => 'secret_token', + 'twilio_from' => '+15551234567' }) + + visit settings_sms_path + + within('[data-provider-block="twilio"]') do + fill_in 'Twilio Account SID', with: 'AC123' + fill_in 'From Number', with: '+15551234567' + # Auth Token intentionally left blank — should be preserved from saved value + end + + click_button 'Save' + + config = EncryptedConfig.find_by(account:, key: EncryptedConfig::SMS_CONFIGS_KEY) + expect(config.value['twilio_auth_token']).to eq('secret_token') + end + end end