From 06ff1e1e31e4fb6316674b63819455a65884d572 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Fri, 6 Nov 2026 13:44:03 +0100 Subject: [PATCH] fix(email): keep saved provider on failed switch, fall back to SMTP sender for Resend - Restore the saved SMTP/Resend flags when a provider switch fails validation, so a later event toggle cannot save a half-configured Resend (team and instance pages). - Resend uses smtp_from_* when resend_from_* is blank, as before the Resend sender fields. - Require the Resend sender only when Resend is enabled. - Replace wire:confirm in the SMTP server picker with x-modal-confirmation. --- app/Livewire/Notifications/Email.php | 37 +++++- app/Livewire/SettingsEmail.php | 37 +++++- bootstrap/helpers/notifications.php | 15 ++- .../partials/smtp-server-picker.blade.php | 20 ++- tests/Feature/EmailProviderSelectionTest.php | 121 ++++++++++++++++++ tests/Feature/InstanceSettingsAuditTest.php | 4 +- .../SettingsEmailProviderExclusivityTest.php | 4 +- .../SettingsEmailSendTestPlacementTest.php | 5 +- tests/Feature/SettingsEmailSmtpSetupTest.php | 3 +- tests/Unit/MailFromIdentityTest.php | 32 +++++ 10 files changed, 245 insertions(+), 33 deletions(-) create mode 100644 tests/Feature/EmailProviderSelectionTest.php diff --git a/app/Livewire/Notifications/Email.php b/app/Livewire/Notifications/Email.php index d48343f720..a81d4249b3 100644 --- a/app/Livewire/Notifications/Email.php +++ b/app/Livewire/Notifications/Email.php @@ -244,10 +244,12 @@ class Email extends Component try { $this->resetErrorBag(); if (! $this->useInstanceEmailSettings) { - $this->validateEnabledEmailProvider(); + $this->enableSelectedEmailProvider(); } $this->saveModel(); } catch (\Throwable $e) { + $this->restoreSavedEmailProvider(); + return handleError($e, $this); } } @@ -258,11 +260,8 @@ class Email extends Component */ public function selectEmailProvider() { - $this->smtpEnabled = $this->emailProvider === 'smtp'; - $this->resendEnabled = $this->emailProvider === 'resend'; - try { - $this->validateEnabledEmailProvider(); + $this->enableSelectedEmailProvider(); } catch (ValidationException) { // Not complete yet: show the fields of the provider and the "Not active yet" notice, without an error. $this->resetErrorBag(); @@ -282,6 +281,30 @@ class Email extends Component return enabled_email_provider($this->settings) ?? 'none'; } + /** + * Turn on the provider selected in the provider list. When its settings are not valid, the saved + * provider stays on, so that a later save of another field cannot turn off the working provider. + */ + private function enableSelectedEmailProvider(): void + { + $this->smtpEnabled = $this->emailProvider === 'smtp'; + $this->resendEnabled = $this->emailProvider === 'resend'; + + try { + $this->validateEnabledEmailProvider(); + } catch (ValidationException $e) { + $this->restoreSavedEmailProvider(); + + throw $e; + } + } + + private function restoreSavedEmailProvider(): void + { + $this->smtpEnabled = (bool) $this->settings->getOriginal('smtp_enabled'); + $this->resendEnabled = (bool) $this->settings->getOriginal('resend_enabled'); + } + private function validateEnabledEmailProvider(): void { match (true) { @@ -483,8 +506,8 @@ class Email extends Component $this->validate([ 'resendEnabled' => 'boolean', 'resendApiKey' => $this->resendEnabled ? 'required|string' : 'nullable|string', - 'resendFromAddress' => 'required|email', - 'resendFromName' => 'required|string', + 'resendFromAddress' => $this->resendEnabled ? 'required|email' : 'nullable|email', + 'resendFromName' => $this->resendEnabled ? 'required|string' : 'nullable|string', ], [ 'resendApiKey.required' => 'Resend API Key is required.', 'resendFromAddress.required' => 'From Address is required.', diff --git a/app/Livewire/SettingsEmail.php b/app/Livewire/SettingsEmail.php index 2bf53bec41..bb7083dcb9 100644 --- a/app/Livewire/SettingsEmail.php +++ b/app/Livewire/SettingsEmail.php @@ -150,10 +150,12 @@ class SettingsEmail extends Component try { $this->authorize('update', $this->settings); $this->resetErrorBag(); - $this->validateEnabledEmailProvider(); + $this->enableSelectedEmailProvider(); $this->syncData(true); $this->dispatch('success', 'Transactional email settings updated.'); } catch (\Throwable $e) { + $this->restoreSavedEmailProvider(); + return handleError($e, $this); } } @@ -164,11 +166,8 @@ class SettingsEmail extends Component */ public function selectEmailProvider() { - $this->smtpEnabled = $this->emailProvider === 'smtp'; - $this->resendEnabled = $this->emailProvider === 'resend'; - try { - $this->validateEnabledEmailProvider(); + $this->enableSelectedEmailProvider(); } catch (ValidationException) { // Not complete yet: show the fields of the provider and the "Not active yet" notice, without an error. $this->resetErrorBag(); @@ -179,6 +178,30 @@ class SettingsEmail extends Component return $this->submit(); } + /** + * Turn on the provider selected in the provider list. When its settings are not valid, the saved + * provider stays on, so that a later save of another field cannot turn off the working provider. + */ + private function enableSelectedEmailProvider(): void + { + $this->smtpEnabled = $this->emailProvider === 'smtp'; + $this->resendEnabled = $this->emailProvider === 'resend'; + + try { + $this->validateEnabledEmailProvider(); + } catch (ValidationException $e) { + $this->restoreSavedEmailProvider(); + + throw $e; + } + } + + private function restoreSavedEmailProvider(): void + { + $this->smtpEnabled = (bool) $this->settings->getOriginal('smtp_enabled'); + $this->resendEnabled = (bool) $this->settings->getOriginal('resend_enabled'); + } + private function validateEnabledEmailProvider(): void { match (true) { @@ -352,8 +375,8 @@ class SettingsEmail extends Component $this->validate([ 'resendEnabled' => 'boolean', 'resendApiKey' => $this->resendEnabled ? 'required|string' : 'nullable|string', - 'resendFromAddress' => 'required|email', - 'resendFromName' => 'required|string', + 'resendFromAddress' => $this->resendEnabled ? 'required|email' : 'nullable|email', + 'resendFromName' => $this->resendEnabled ? 'required|string' : 'nullable|string', ], [ 'resendApiKey.required' => 'Resend API Key is required.', 'resendFromAddress.required' => 'From Address is required.', diff --git a/bootstrap/helpers/notifications.php b/bootstrap/helpers/notifications.php index f2f97b196b..2a250197fe 100644 --- a/bootstrap/helpers/notifications.php +++ b/bootstrap/helpers/notifications.php @@ -52,24 +52,29 @@ function enabled_email_provider(?object $settings): ?string function mail_from_identity(object $settings, bool $asNotification = false): array { // Each provider has its own sender, because each one sends from its own verified domain. - $prefix = data_get($settings, 'resend_enabled') ? 'resend' : 'smtp'; + // Resend uses the SMTP sender when it has none, as before the Resend sender fields existed. + $usesResend = (bool) data_get($settings, 'resend_enabled'); + $providerSender = static function (string $field) use ($settings, $usesResend): string { + $value = trim((string) ($usesResend ? data_get($settings, "resend_{$field}") : '')); + + return $value !== '' ? $value : trim((string) data_get($settings, "smtp_{$field}")); + }; if ($asNotification && filled(data_get($settings, 'notification_from_address'))) { $address = (string) data_get($settings, 'notification_from_address'); $name = trim((string) data_get($settings, 'notification_from_name', '')); } else { - $address = data_get($settings, "{$prefix}_from_address"); + $address = $providerSender('from_address'); - if (blank($address)) { + if ($address === '') { throw new InvalidArgumentException('Transactional email sender address is not configured.'); } - $address = (string) $address; $name = ''; } if ($name === '') { - $name = trim((string) data_get($settings, "{$prefix}_from_name", '')); + $name = $providerSender('from_name'); } return [ diff --git a/resources/views/livewire/partials/smtp-server-picker.blade.php b/resources/views/livewire/partials/smtp-server-picker.blade.php index 33f923820d..70c6911dc0 100644 --- a/resources/views/livewire/partials/smtp-server-picker.blade.php +++ b/resources/views/livewire/partials/smtp-server-picker.blade.php @@ -38,12 +38,20 @@ @if (count($settings->savedSmtpServers()) > 0) - + {{-- Keyed by the server name, so that the ignored modal shows the server in use after a switch. --}} + + + + + @endif diff --git a/tests/Feature/EmailProviderSelectionTest.php b/tests/Feature/EmailProviderSelectionTest.php new file mode 100644 index 0000000000..6d0a11f93a --- /dev/null +++ b/tests/Feature/EmailProviderSelectionTest.php @@ -0,0 +1,121 @@ + true, + 'smtp_host' => 'smtp.example.com', + 'smtp_port' => '587', + 'smtp_encryption' => 'starttls', + 'smtp_from_address' => 'alerts@example.com', + 'smtp_from_name' => 'Coolify', + 'resend_enabled' => false, + 'resend_api_key' => null, + ]; +} + +beforeEach(function () { + $this->rootTeam = Team::find(0) ?? Team::factory()->create(['id' => 0]); + InstanceSettings::forceCreate(['id' => 0, ...workingSmtpSettings()]); + Once::flush(); +}); + +test('an unsaved resend selection is not saved by a later team notification event toggle', function () { + $team = Team::factory()->create(); + $user = User::factory()->create(); + $team->members()->attach($user->id, ['role' => 'admin']); + $team->emailNotificationSettings->update([...workingSmtpSettings(), 'use_instance_email_settings' => false]); + + $this->actingAs($user); + session(['currentTeam' => $team]); + + Livewire::test(NotificationEmail::class) + ->set('emailProvider', 'resend') + ->call('selectEmailProvider') + ->assertSet('emailProvider', 'resend') + ->call('toggleEvent', 'deploymentSuccessEmailNotifications') + ->call('instantSaveDeploymentCommitDetails'); + + $settings = $team->emailNotificationSettings->fresh(); + + expect($settings->smtp_enabled)->toBeTrue() + ->and($settings->resend_enabled)->toBeFalse() + ->and($settings->deployment_success_email_notifications)->toBeTrue(); +}); + +test('an unsaved resend selection is not saved by a later instance provider toggle', function () { + $user = User::factory()->create(); + $this->rootTeam->members()->attach($user->id, ['role' => 'admin']); + + $this->actingAs($user); + session(['currentTeam' => ['id' => 0]]); + + Livewire::test(SettingsEmail::class) + ->set('emailProvider', 'resend') + ->call('selectEmailProvider') + ->call('toggleResend'); + + $settings = InstanceSettings::find(0); + + expect($settings->smtp_enabled)->toBeTrue() + ->and($settings->resend_enabled)->toBeFalse(); +}); + +test('a completed resend selection is saved on submit', function () { + $team = Team::factory()->create(); + $user = User::factory()->create(); + $team->members()->attach($user->id, ['role' => 'admin']); + $team->emailNotificationSettings->update([...workingSmtpSettings(), 'use_instance_email_settings' => false]); + + $this->actingAs($user); + session(['currentTeam' => $team]); + + Livewire::test(NotificationEmail::class) + ->set('emailProvider', 'resend') + ->call('selectEmailProvider') + ->set('resendApiKey', 're_test_key') + ->set('resendFromAddress', 'resend@example.com') + ->set('resendFromName', 'Coolify') + ->call('submit') + ->assertHasNoErrors(); + + $settings = $team->emailNotificationSettings->fresh(); + + expect($settings->resend_enabled)->toBeTrue() + ->and($settings->smtp_enabled)->toBeFalse() + ->and($settings->resend_from_address)->toBe('resend@example.com'); +}); + +test('turning team resend off does not require a resend sender', function () { + $team = Team::factory()->create(); + $user = User::factory()->create(); + $team->members()->attach($user->id, ['role' => 'admin']); + $team->emailNotificationSettings->update([ + 'use_instance_email_settings' => false, + 'resend_enabled' => true, + 'resend_api_key' => 're_test_key', + 'smtp_from_address' => 'alerts@example.com', + ]); + + $this->actingAs($user); + session(['currentTeam' => $team]); + + Livewire::test(NotificationEmail::class) + ->set('resendEnabled', false) + ->call('submitResend') + ->assertHasNoErrors() + ->assertNotDispatched('error'); + + expect($team->emailNotificationSettings->fresh()->resend_enabled)->toBeFalse(); +}); diff --git a/tests/Feature/InstanceSettingsAuditTest.php b/tests/Feature/InstanceSettingsAuditTest.php index 6441be372b..4b1de8169f 100644 --- a/tests/Feature/InstanceSettingsAuditTest.php +++ b/tests/Feature/InstanceSettingsAuditTest.php @@ -150,8 +150,8 @@ test('resend settings save is audited without the api key', function () { Livewire::test(SettingsEmail::class) ->set('resendEnabled', true) ->set('resendApiKey', 're_secret_api_key') - ->set('smtpFromAddress', 'from@example.com') - ->set('smtpFromName', 'Coolify') + ->set('resendFromAddress', 'from@example.com') + ->set('resendFromName', 'Coolify') ->call('submitResend'); $event = instanceSettingsAuditEvents('ui.settings.email.updated')->sole(); diff --git a/tests/Feature/SettingsEmailProviderExclusivityTest.php b/tests/Feature/SettingsEmailProviderExclusivityTest.php index c63df9fe40..7b0d5524a0 100644 --- a/tests/Feature/SettingsEmailProviderExclusivityTest.php +++ b/tests/Feature/SettingsEmailProviderExclusivityTest.php @@ -54,8 +54,8 @@ test('enabling Resend disables SMTP in storage', function () { Livewire::test(SettingsEmail::class) ->set('resendApiKey', 're_test_key') - ->set('smtpFromAddress', 'from@example.com') - ->set('smtpFromName', 'Coolify') + ->set('resendFromAddress', 'from@example.com') + ->set('resendFromName', 'Coolify') ->call('toggleResend'); $this->settings->refresh(); diff --git a/tests/Feature/SettingsEmailSendTestPlacementTest.php b/tests/Feature/SettingsEmailSendTestPlacementTest.php index 76f557a782..d3ad49577a 100644 --- a/tests/Feature/SettingsEmailSendTestPlacementTest.php +++ b/tests/Feature/SettingsEmailSendTestPlacementTest.php @@ -41,7 +41,7 @@ test('send test is available when transactional email is enabled', function () { ->assertSee('Send test'); }); -test('send test is not rendered when transactional email is disabled', function () { +test('send test is disabled when transactional email is disabled', function () { $rootTeam = Team::find(0) ?? Team::factory()->create(['id' => 0]); InstanceSettings::forceCreate([ 'id' => 0, @@ -58,5 +58,6 @@ test('send test is not rendered when transactional email is disabled', function Livewire::test(SettingsEmail::class) ->assertOk() - ->assertDontSee('Send test'); + ->assertSee('Save an email provider first.') + ->assertDontSee('Send Test Email'); }); diff --git a/tests/Feature/SettingsEmailSmtpSetupTest.php b/tests/Feature/SettingsEmailSmtpSetupTest.php index f5f72c92df..ce3d5985d0 100644 --- a/tests/Feature/SettingsEmailSmtpSetupTest.php +++ b/tests/Feature/SettingsEmailSmtpSetupTest.php @@ -110,8 +110,7 @@ test('enabling smtp delivery does not require a resend api key when resend is di Livewire::test(SettingsEmail::class) ->fill(smtpSetupPayload()) - ->set('smtpEnabled', true) - ->call('instantSaveSmtp') + ->call('toggleSmtp') ->assertHasNoErrors() ->assertNotDispatched('error'); diff --git a/tests/Unit/MailFromIdentityTest.php b/tests/Unit/MailFromIdentityTest.php index d9587f9c7f..f5a6c8f7a9 100644 --- a/tests/Unit/MailFromIdentityTest.php +++ b/tests/Unit/MailFromIdentityTest.php @@ -85,3 +85,35 @@ it('rejects enabled email settings without a from address', function () { 'smtp_from_name' => 'Coolify', ]); })->throws(InvalidArgumentException::class, 'Transactional email sender address is not configured.'); + +it('uses the resend sender when resend is enabled', function () { + $identity = mail_from_identity((object) [ + 'resend_enabled' => true, + 'resend_from_address' => 'resend@example.com', + 'resend_from_name' => 'Resend Sender', + 'smtp_from_address' => 'smtp@example.com', + 'smtp_from_name' => 'SMTP Sender', + ]); + + expect($identity)->toBe(['address' => 'resend@example.com', 'name' => 'Resend Sender']); +}); + +it('falls back to the smtp sender when resend is enabled without a resend sender', function () { + $identity = mail_from_identity((object) [ + 'resend_enabled' => true, + 'resend_from_address' => null, + 'resend_from_name' => '', + 'smtp_from_address' => 'smtp@example.com', + 'smtp_from_name' => 'SMTP Sender', + ]); + + expect($identity)->toBe(['address' => 'smtp@example.com', 'name' => 'SMTP Sender']); +}); + +it('rejects resend settings without a resend or smtp from address', function () { + mail_from_identity((object) [ + 'resend_enabled' => true, + 'resend_from_address' => null, + 'smtp_from_address' => null, + ]); +})->throws(InvalidArgumentException::class, 'Transactional email sender address is not configured.');