Pin CloudReady's null-mailbox guard and share it with the trait
A mutation the reviewer ran (if ($box !== null) -> if (true)) left every test green: CloudReady hand-rolled the same null-mailbox guard as SendsFromMailbox, but nothing exercised its null branch. Added the missing test, then extracted the decision itself (null mailbox -> no sender fields, no_reply -> no Reply-To) into one shared method so an Envelope-shaped and a MailMessage-shaped consumer can no longer disagree.feat/mailboxes
parent
64a33c675e
commit
c899d41946
|
|
@ -20,18 +20,39 @@ trait SendsFromMailbox
|
|||
{
|
||||
protected function mailboxEnvelope(string $purpose, string $subject): Envelope
|
||||
{
|
||||
$box = app(MailboxResolver::class)->for($purpose);
|
||||
|
||||
if ($box === null) {
|
||||
// No mailbox configured yet — fall back to the framework default so
|
||||
// an installation mid-setup still sends rather than crashing.
|
||||
return new Envelope(subject: $subject);
|
||||
}
|
||||
[$from, $replyTo] = $this->mailboxAddresses($purpose);
|
||||
|
||||
return new Envelope(
|
||||
from: new Address($box->address, $box->display_name ?: null),
|
||||
replyTo: $box->no_reply ? [] : [new Address($box->address, $box->display_name ?: null)],
|
||||
from: $from,
|
||||
replyTo: $replyTo ? [$replyTo] : [],
|
||||
subject: $subject,
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* The From and Reply-To addresses for $purpose's mailbox — the ONE place
|
||||
* that decides "no configured mailbox means no sender fields at all" and
|
||||
* "no_reply means no Reply-To". Shared by every consumer of a purpose
|
||||
* mailbox regardless of the shape it builds — an Envelope here (a
|
||||
* Mailable), a MailMessage in CloudReady (a Notification, which cannot
|
||||
* use mailboxEnvelope() directly since it is not building an Envelope).
|
||||
* Two independent copies of this same decision is exactly the shape of
|
||||
* gap Task 4 found and fixed in MailboxTransport::resolution().
|
||||
*
|
||||
* @return array{0: ?Address, 1: ?Address} [from, replyTo]
|
||||
*/
|
||||
protected function mailboxAddresses(string $purpose): array
|
||||
{
|
||||
$box = app(MailboxResolver::class)->for($purpose);
|
||||
|
||||
if ($box === null) {
|
||||
// No mailbox configured yet — both null, so a caller falls back
|
||||
// to its own framework default rather than dereferencing null.
|
||||
return [null, null];
|
||||
}
|
||||
|
||||
$from = new Address($box->address, $box->display_name ?: null);
|
||||
|
||||
return [$from, $box->no_reply ? null : $from];
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,8 +2,8 @@
|
|||
|
||||
namespace App\Notifications;
|
||||
|
||||
use App\Mail\Concerns\SendsFromMailbox;
|
||||
use App\Models\Instance;
|
||||
use App\Services\Mail\MailboxResolver;
|
||||
use App\Services\Mail\MailPurpose;
|
||||
use Illuminate\Bus\Queueable;
|
||||
use Illuminate\Notifications\Messages\MailMessage;
|
||||
|
|
@ -22,7 +22,7 @@ use Illuminate\Support\Facades\Crypt;
|
|||
*/
|
||||
class CloudReady extends Notification
|
||||
{
|
||||
use Queueable;
|
||||
use Queueable, SendsFromMailbox;
|
||||
|
||||
public function __construct(
|
||||
public Instance $instance,
|
||||
|
|
@ -40,17 +40,16 @@ class CloudReady extends Notification
|
|||
{
|
||||
$url = 'https://'.$this->instance->subdomain.'.'.config('provisioning.dns.zone');
|
||||
|
||||
$box = app(MailboxResolver::class)
|
||||
->for(MailPurpose::PROVISIONING);
|
||||
[$from, $replyTo] = $this->mailboxAddresses(MailPurpose::PROVISIONING);
|
||||
|
||||
$message = (new MailMessage)->mailer('cp_'.MailPurpose::PROVISIONING);
|
||||
|
||||
if ($box !== null) {
|
||||
$message->from($box->address, $box->display_name ?: null);
|
||||
if ($from !== null) {
|
||||
$message->from($from->address, $from->name);
|
||||
}
|
||||
|
||||
if (! $box->no_reply) {
|
||||
$message->replyTo($box->address, $box->display_name ?: null);
|
||||
}
|
||||
if ($replyTo !== null) {
|
||||
$message->replyTo($replyTo->address, $replyTo->name);
|
||||
}
|
||||
|
||||
return $message
|
||||
|
|
|
|||
|
|
@ -137,3 +137,22 @@ it('sets Reply-To on the provisioning notification when its mailbox allows repli
|
|||
|
||||
expect($message->replyTo[0][0])->toBe('support@clupilot.com');
|
||||
});
|
||||
|
||||
it("leaves the provisioning notification's sender fields at MailMessage's defaults when no mailbox is configured", function () {
|
||||
// The mirror of the trait's own null-mailbox fallback test above, but for
|
||||
// CloudReady's own guard: neither a mailbox row nor a provisioning/system
|
||||
// Settings mapping exists, so MailboxResolver::for() returns null. Must
|
||||
// not throw dereferencing a null mailbox, and must leave from/replyTo
|
||||
// exactly as `new MailMessage` starts them — [] and [].
|
||||
$notification = new CloudReady(
|
||||
Instance::factory()->create(),
|
||||
'admin',
|
||||
Crypt::encryptString('geheim'),
|
||||
);
|
||||
|
||||
$message = $notification->toMail(new stdClass);
|
||||
|
||||
expect($message->from)->toBeEmpty()
|
||||
->and($message->replyTo)->toBeEmpty()
|
||||
->and($message->mailer)->toBe('cp_provisioning');
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue