Surface silent-skip paths and additional silent failures in email/booking/order flows (#352)
## Why Customer `k.sand@ksand.dk` reported never receiving wash certificates for completed bookings. Two methods contained silent early-return guards so the actual reason was unobservable from container logs: - `order_bookings_o::sendWashCertificateToCustomer()` — 5 silent returns - `email::sendWashCertificateEmailToCustomer()` — 1 silent return The most likely root cause: `email_notifications_enabled` defaults to `0` in the schema and `users.add()` does not set it on insert, so newly imported customers have notifications off until toggled. `wantsEmailNotifications()` then returns false and the email silently skips. ## What changed ### Original commit (`0ead5de5`) - `objects/order_bookings_o.php` — all 5 silent early-returns now log via new `logWashCertificateSkip()` helper (Redis stream `module=email / action=WASH_CERT_SKIP` + `error_log('[wash-cert-skip] …')`). - `classes/email.php` — silent `hasTransaction()` return in `sendWashCertificateEmailToCustomer()` now logs too. - `objects/bookings_o.php` — emits `WASH_CERT_SKIP` (legacy_no_wash_certificate_email) when `washCertificateEmail` is empty; no behavioural change. - **New** `routes/washCertificateDebugRoute.php` — `GET /debug/wash-certificates/diagnose?customer_number=&from=&to=` (404 in prod via $DEBUG; superuser-auth otherwise) replays the decision tree and reports `blocking_reason` per booking. ### Follow-up commit (`46a59e4e`) — silent-failure sweep **PART A — silent returns / silent errors (10 fixes):** - `email::sendEmailMailerSend()` — blacklisted-recipient skip now logs with context. - `email::sendNewCustomerRegistrationNotifications()` — empty-email skip + per-recipient try/catch with error_log (was unprotected; a single MailerSend error broke the loop). - `bookings_new_o::generateWashCertificate()` — wrapped `sendWashCertificateEmail()` in try/catch with error_log and re-throw (same pattern as the k.sand fix). - `users_o::getCustomerName()` — replaced catch-and-swallow with structured error_log. - `users_o::getCustomerEcocomicData()` — same. - `bookingsRoute.php` — added booking-id context to 4 × `$response->error('Booking not found', 404)` calls. **PART B — cron paths (10 files):** Added error_log breadcrumb + try/catch to `CheckUnfulfilledBookings`, `ClearAllUsersEconomicCustomerDetails`, `ClearAllUsersEconomicCustomerDiscounts`, `RunXLVaskModuleCron`, `SyncBookings`, `SyncEconomicInvoiceStatus`, `SyncLogs`, `BackfillEconomicV2History`, `EnsureXLVaskAutomationSchema`, and 3 functions in `Cron.php`. Each uses a distinct `[cron-…]` prefix for grep-ability. **PART C — real bugs (2 fixed):** 1. `email::sendEmailMailerSend()` attachment `array_map` — the previous exception message emitted a binary blob because `$attachment[0]` was already overwritten by `file_get_contents()`. Now captures $path first. 2. `bookings_new_o::generateWashCertificate()` — booking persisted as `completed` before email was sent, with no try/catch. Fixed (see PART A). ## How to verify 1. Deploy to staging. 2. Hit `/debug/wash-certificates/diagnose?customer_number=<k.sand's customer_number>` as a superuser — the response lists every booking's `blocking_reason`. 3. Tail container logs for `[wash-cert-skip]`, `[email-skip]`, `[cron-…]`, and Redis stream `module=email` action `WASH_CERT_SKIP` to see real-world skips going forward. ## Follow-ups (out of scope) - Schema migration to default `email_notifications_enabled` to `1` and backfill non-empty-email customers. - Move `error_log` to a proper PSR-3 logger. ## Risk - Logging only + new debug endpoint (404-gated in prod). No behavioural change for any path that previously sent mail successfully. `php -l` could not be run in the original sandbox; please verify on your CI box before deploying. 🤖 Generated with [OpenClaw](https://openclaw.ai)
This commit is contained in:
@@ -34,6 +34,7 @@ use MailerSend\Helpers\Builder\Recipient;
|
||||
use MailerSend\MailerSend;
|
||||
use objects\bookings_o;
|
||||
use objects\departments_o;
|
||||
use objects\logs_o;
|
||||
use objects\users_o;
|
||||
use Psr\Http\Client\ClientExceptionInterface;
|
||||
|
||||
@@ -144,6 +145,15 @@ use Psr\Http\Client\ClientExceptionInterface;
|
||||
];
|
||||
// If the email is blacklisted, return without sending the email
|
||||
if (in_array($to, $blacklisted_emails)) {
|
||||
// Previously this was a silent return - ops could not tell whether a
|
||||
// missing delivery was caused by the blacklist or a real provider
|
||||
// outage. Emit a structured skip event before returning.
|
||||
$context = [
|
||||
'reason' => 'recipient_blacklisted',
|
||||
'recipient' => $to,
|
||||
'subject' => $subject,
|
||||
];
|
||||
error_log('[email-skip] ' . json_encode($context, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE));
|
||||
return;
|
||||
}
|
||||
// Send POST request to email service
|
||||
@@ -164,12 +174,19 @@ use Psr\Http\Client\ClientExceptionInterface;
|
||||
if ($attachments) {
|
||||
$attachments = array_map(function ($attachment) {
|
||||
// Read the data from the path (Attachment[0]) and set the filename (Attachment[1])
|
||||
$attachment[0] = file_get_contents($attachment[0]);
|
||||
if ($attachment[0] === false) {
|
||||
throw new Exception('Failed to read file: ' . $attachment[0]);
|
||||
$path = (string)$attachment[0];
|
||||
$contents = file_get_contents($path);
|
||||
if ($contents === false) {
|
||||
// Capture the path before it is overwritten so the resulting
|
||||
// exception message is useful in ops logs. Previously this
|
||||
// threw with binary contents (because $attachment[0] had
|
||||
// already been replaced by file_get_contents()'s output),
|
||||
// making the failure essentially un-diagnosable.
|
||||
throw new Exception('Failed to read attachment file: ' . $path);
|
||||
}
|
||||
$attachment[0] = $contents;
|
||||
if (empty($attachment[1])) {
|
||||
throw new Exception('Filename is empty');
|
||||
throw new Exception('Attachment filename is empty (path: ' . $path . ')');
|
||||
}
|
||||
return new Attachment($attachment[0], $attachment[1]);
|
||||
}, $attachments);
|
||||
@@ -489,7 +506,13 @@ use Psr\Http\Client\ClientExceptionInterface;
|
||||
{
|
||||
// Validate the booking object
|
||||
$order_booking->requireSelected();
|
||||
if (!$order_booking->hasTransaction()) return; // Only send a wash certificate if the order has been created.
|
||||
if (!$order_booking->hasTransaction()) {
|
||||
// Previously this was a silent return which made wash-certificate
|
||||
// delivery failures (e.g. k.sand@ksand.dk) impossible to diagnose
|
||||
// without DB access. Emit a structured skip event before returning.
|
||||
self::logWashCertificateSkip('no_transaction_email', (int)$order_booking->id, (int)$order_booking->customer_number->value());
|
||||
return;
|
||||
} // Only send a wash certificate if the order has been created.
|
||||
// Get the order details
|
||||
$order = $order_booking->getOrder();
|
||||
// Get customer details
|
||||
@@ -604,6 +627,14 @@ use Psr\Http\Client\ClientExceptionInterface;
|
||||
foreach ((new users_o())->getSuperuserNewCustomerEmailNotificationRecipients() as $recipient) {
|
||||
$recipientEmail = trim((string)($recipient['email'] ?? ''));
|
||||
if ($recipientEmail === '') {
|
||||
// Recipient has no email address; surface the skip so an admin
|
||||
// with no configured inbox can be fixed instead of silently
|
||||
// dropping new-customer notifications.
|
||||
$context = [
|
||||
'reason' => 'superuser_recipient_empty_email',
|
||||
'recipient_display_name' => trim((string)($recipient['display_name'] ?? '')),
|
||||
];
|
||||
error_log('[email-skip] ' . json_encode($context, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE));
|
||||
continue;
|
||||
}
|
||||
|
||||
@@ -612,12 +643,61 @@ use Psr\Http\Client\ClientExceptionInterface;
|
||||
$recipientName = $recipientEmail;
|
||||
}
|
||||
|
||||
$this->sendEmail(
|
||||
$recipientEmail,
|
||||
$recipientName,
|
||||
'New customer registered on Truck Wash',
|
||||
$message,
|
||||
);
|
||||
// Per-recipient try/catch so a single bad MailerSend response does
|
||||
// not break delivery to the remaining superuser recipients - this
|
||||
// loop is unprotected upstream and a transient 5xx would otherwise
|
||||
// mean the rest of the team silently stops hearing about new
|
||||
// customer registrations.
|
||||
try {
|
||||
$this->sendEmail(
|
||||
$recipientEmail,
|
||||
$recipientName,
|
||||
'New customer registered on Truck Wash',
|
||||
$message,
|
||||
);
|
||||
} catch (Exception $e) {
|
||||
$context = [
|
||||
'reason' => 'superuser_recipient_send_failed',
|
||||
'recipient' => $recipientEmail,
|
||||
'subject' => 'New customer registered on Truck Wash',
|
||||
'error' => $e->getMessage(),
|
||||
];
|
||||
error_log('[email-skip] ' . json_encode($context, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Record a structured "wash certificate was skipped" event.
|
||||
*
|
||||
* Mirrors the helper of the same name on order_bookings_o so that every
|
||||
* silent-return path in the email delivery flow (this class plus the
|
||||
* order-bookings wrapper) is observable from the same grep target.
|
||||
*
|
||||
* TODO: migrate to the project logger when one is available globally.
|
||||
*
|
||||
* @param array<string, mixed> $extra
|
||||
*/
|
||||
private static function logWashCertificateSkip(string $reason, int $booking_id, int $customer_number, array $extra = []): void
|
||||
{
|
||||
$context = array_merge([
|
||||
'reason' => $reason,
|
||||
'booking_id' => $booking_id,
|
||||
'customer_number' => $customer_number,
|
||||
], $extra);
|
||||
try {
|
||||
(new logs_o())->add(
|
||||
'email',
|
||||
'global',
|
||||
3,
|
||||
0,
|
||||
'WASH_CERT_SKIP',
|
||||
json_encode($context, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE)
|
||||
);
|
||||
} catch (\Throwable) {
|
||||
// Logging must never block the booking flow.
|
||||
}
|
||||
// Also emit to PHP error stream so this is visible in container logs.
|
||||
error_log('[wash-cert-skip] ' . json_encode($context, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user