Email/SMTP test button + loan due-date visibility & notifications (#238)#253
Open
fabiodalez-dev wants to merge 9 commits into
Open
Email/SMTP test button + loan due-date visibility & notifications (#238)#253fabiodalez-dev wants to merge 9 commits into
fabiodalez-dev wants to merge 9 commits into
Conversation
…Mailer (#238) Nikola had no way to tell whether SMTP was actually working. Adds a 'Send test email' button in Settings → Email that sends a test message with the current config and reports success or the SPECIFIC error. Also fixes a latent bug: the 'smtp' driver used a hand-rolled SMTP client that never checked server replies and returned true even when the server rejected the message (auth/recipient) — the classic 'SMTP looks fine but no mail arrives'. Both 'smtp' and 'phpmailer' now go through PHPMailer, which verifies the handshake/auth/recipient and surfaces the real error. New Mailer::sendTest() returns {ok, error}; SettingsController::sendTestEmail exposes it as JSON. Verified end-to-end against Mailpit: valid SMTP → success + mail delivered; bad host → success:false with 'Could not connect to SMTP host'. PHPStan L5 clean; 13 strings added to all four locales.
…ashboard (#238) Nikola asked for loan due dates to be more visible. Two changes: 1. Loans List (/admin/loans) now has a dedicated 'Scadenza' (Due date) column. Previously the due date was only shown squeezed under the loan-start date. The due date turns red (text-red-600 font-semibold) when an active loan (in_corso/in_ritardo) is due today or overdue — the same red-highlight rule the Physical Copies list uses. The server-side endpoint now returns data_scadenza; the DataTable computes the highlight on the local date to match the stored dates. 2. Dashboard 'Prestiti in Corso' list applies the same red date highlight for due-today/overdue loans. The status badge stays faithful to the real loan state (driven by p.stato): a loan due today reads 'In corso' with a red date (act-now cue), while a genuinely late one reads 'In Ritardo'. Previously the badge was hard-coded 'In corso' even for overdue loans. Verified in the browser against seeded loans: a due-today loan and an overdue loan both render the due date in red in both places; badges are accurate. PHPStan L5 clean; no new locale strings (all reused).
…ify on app timezone (#238) Follow-up refinements to the due-date visibility work: - NotificationService: the expiration-warning query matched data_scadenza exactly at 'today + N days', which silently skipped a loan whose due date is today (Nikola's exact repro). Widen it to BETWEEN today AND today + N so due-today and due-soon loans both get warned; the in-app notification now reads 'scade oggi' when zero days remain. Clamp the configured horizon to >= 0. - PrestitiApiController: the DataTables column map still assumed the old layout (index 3 = stato) after the Due-date column was inserted at index 3, so clicking the 'Scadenza' / 'Stato' headers sorted by the wrong SQL column. Remap 3 -> p.data_scadenza, 4 -> p.stato. - Loans list + dashboard: compute the due-today/overdue highlight against the application's configured date (DateHelper::today(), passed to the DataTable as applicationToday) instead of the browser's or PHP process's local date, and only highlight active loans. Consistent with how loan dates are compared elsewhere and avoids timezone-edge flicker. - Tests: E2E B.9b registers a loan due today and asserts the warning email is delivered and warning_sent flips; a new static unit test guards the query shape, the column map, the timezone source, and locale parity. PHPStan L5 clean; unit test 9/9; browser-verified the red highlight for due-today and overdue loans; locale parity 6541 keys across all four.
…ication pipeline (#238) Extend the due-date visibility work to the mobile API so the Android app and its push notifications agree with the web/email side: - Hourly cron: dispatch native push (mobile_api.dispatch_push) on the same pass as the email reminders. The daily full-maintenance job also fires it, but 06:00 alone misses loans created later in the day and can always land in a user's quiet-hours window. PushDispatcher deduplicates event keys, so daily + hourly are safe together. - PushDispatcher + ActionsController: read the due-soon horizon from the real setting (advanced.days_before_expiry_warning) instead of the non-existent loans.reminder_days_before key, which silently pinned the feed to its 3-day fallback even after an admin changed it. Clamp to >= 0 (0 = today only), matching NotificationService. - PushDispatcher: derive 'today' from DateHelper::today() (application timezone) and bind it into the due/overdue sweeps, instead of forcing the DB session to UTC and using CURDATE() — so the day boundary matches the web/email pipeline around the library's midnight. sweepLoanOverdue is now a prepared statement. - Loan API: add a server-authoritative 'due_attention' boolean (active loan due today or earlier) so the Android client doesn't compute 'today' from its own device timezone; documented in the OpenAPI schema. - Tests: mobile-api E2E for due_attention (future vs due-today) and the zero-day notification horizon; a static unit test guards the setting key, timezone source, bind params, the new field, and the hourly dispatch. PHPStan L5 clean; unit test 5/5; mobile-api plugin hook wiring verified.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIl PR aggiorna la gestione delle scadenze nelle notifiche, nelle viste, nell’API mobile e nei push usando la data applicativa. Aggiunge inoltre ChangesGestione delle scadenze
Invio email di prova
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant SettingsView
participant SettingsController
participant Mailer
participant PHPMailer
Admin->>SettingsView: avvia test email
SettingsView->>SettingsController: POST /admin/settings/email/test
SettingsController->>Mailer: sendTest(destinatario)
Mailer->>PHPMailer: configura e invia email
PHPMailer-->>Mailer: esito o errore
Mailer-->>SettingsController: risultato strutturato
SettingsController-->>SettingsView: JSON di successo o errore
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/Support/Mailer.php (1)
114-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicazione tra
sendPHPMailer()esendTest().I due metodi costruiscono lo stesso messaggio PHPMailer (addAddress/Subject/Body/AltBody/send/catch) quasi identicamente. Rischio concreto: se domani si modifica la logica di invio reale (es. si aggiunge CC/BCC, si cambia l'AltBody) e ci si scorda di sincronizzare
sendTest(), il test diagnostico non rappresenterà più fedelmente l'invio vero — vanificando lo scopo della feature.♻️ Suggerimento: estrarre un helper condiviso
- private static function sendPHPMailer(string $to, string $subject, string $htmlBody, ?string $textBody, string $fromEmail, string $fromName): bool - { - if (!class_exists('PHPMailer\\PHPMailer\\PHPMailer')) { - return self::sendMail($to, $subject, $htmlBody, $fromEmail, $fromName); - } - try { - $mailer = new \PHPMailer\PHPMailer\PHPMailer(true); - self::configureMailer($mailer, $fromEmail, $fromName); - $mailer->addAddress($to); - $mailer->Subject = $subject; - $mailer->Body = $htmlBody; - $mailer->AltBody = $textBody ?: strip_tags($htmlBody); - return $mailer->send(); - } catch (\Throwable $e) { - SecureLogger::warning('Email send failed: ' . $e->getMessage()); - return false; - } - } + private static function sendPHPMailer(string $to, string $subject, string $htmlBody, ?string $textBody, string $fromEmail, string $fromName): bool + { + if (!class_exists('PHPMailer\\PHPMailer\\PHPMailer')) { + return self::sendMail($to, $subject, $htmlBody, $fromEmail, $fromName); + } + $result = self::dispatchPHPMailer($to, $subject, $htmlBody, $textBody, $fromEmail, $fromName); + if (!$result['ok']) { + SecureLogger::warning('Email send failed: ' . $result['error']); + } + return $result['ok']; + } + + /** `@return` array{ok: bool, error: string} */ + private static function dispatchPHPMailer(string $to, string $subject, string $htmlBody, ?string $textBody, string $fromEmail, string $fromName): array + { + try { + $mailer = new \PHPMailer\PHPMailer\PHPMailer(true); + self::configureMailer($mailer, $fromEmail, $fromName); + $mailer->addAddress($to); + $mailer->Subject = $subject; + $mailer->Body = $htmlBody; + $mailer->AltBody = $textBody ?: strip_tags($htmlBody); + $mailer->send(); + return ['ok' => true, 'error' => '']; + } catch (\Throwable $e) { + return ['ok' => false, 'error' => $e->getMessage()]; + } + }Poi
sendTest()chiamadispatchPHPMailer()direttamente.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Support/Mailer.php` around lines 114 - 176, Estrai la costruzione e l’invio condivisi di PHPMailer da sendPHPMailer() e sendTest() in un helper privato, ad esempio dispatchPHPMailer(), che riceva destinatario, oggetto, corpo HTML/testo e mittente, esegua configureMailer(), addAddress(), impostazione dei contenuti e send(), propagando l’eccezione. Aggiorna entrambi i metodi per usare questo helper, mantenendo in sendPHPMailer() il fallback/logging esistente e in sendTest() la restituzione del messaggio d’errore reale.app/Support/NotificationService.php (1)
428-449: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAllineare
loan_expiring_warningal caso di scadenza odierna
Nel body del templategiorni_rimastiviene stampato così com’è: con0l’email mostra “0 giorni” invece di “oggi”. Aggiungi un ramo dedicato o passa un testo già formattato.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Support/NotificationService.php` around lines 428 - 449, Aggiorna la preparazione delle variabili nel flusso che invoca sendWithRetry per loan_expiring_warning, formattando giorni_rimasti come “oggi” quando il valore è 0 e mantenendo il numero di giorni negli altri casi. Usa il valore formattato anche nel template email, senza modificare la logica delle notifiche in-app.storage/plugins/mobile-api/src/Controllers/ActionsController.php (1)
706-716: 📐 Maintainability & Code Quality | 🔵 TrivialLogica di calcolo
dueSoonDaysduplicata conPushDispatcher::dueSoonDays().Stessa lettura di
advanced.days_before_expiry_warningcon fallback3e clampmax(0, ...), ripetuta identica in due file. È esattamente il tipo di drift che questa PR sta correggendo (la vecchia chiaveloans.reminder_days_beforemai esistita). Vedi commento consolidato.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@storage/plugins/mobile-api/src/Controllers/ActionsController.php` around lines 706 - 716, Rimuovi la lettura duplicata di advanced.days_before_expiry_warning da questo blocco e riutilizza PushDispatcher::dueSoonDays() per ottenere il valore già normalizzato con fallback 3 e clamp a zero. Mantieni invariato il calcolo di dueSoonLimit e il comportamento del feed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Routes/web.php`:
- Around line 574-579: Add the existing dedicated rate-limiting middleware or
throttle configuration used by the costly admin routes such as sync-covers and
bulk-enrich/start to the /admin/settings/email/test route. Preserve the current
CsrfMiddleware and AdminAuthMiddleware protections while ensuring repeated
test-email requests are throttled.
In `@app/Views/dashboard/index.php`:
- Around line 654-664: Update the $dueSoonOrOverdue calculation in the dashboard
view to require an active loan and a stato of in_corso or in_ritardo, matching
the isDueNowOrOverdue conditions in prestiti/index.php. Preserve the existing
due-date comparison and empty-date guard while preventing returned or otherwise
inactive loans from being highlighted.
In `@app/Views/settings/index.php`:
- Line 353: Replace the direct PHP translation interpolations in the JavaScript
assignments around btn.innerHTML at both affected locations with
json_encode(__(...), JSON_HEX_TAG), preserving the existing spinner markup and
translated text while safely escaping apostrophes and HTML-sensitive characters.
In `@storage/plugins/mobile-api/src/Push/PushDispatcher.php`:
- Around line 575-586: Consolida la logica duplicata di dueSoonDays() con la
versione inline in ActionsController.php: riutilizza un’unica implementazione
condivisa per leggere advanced.days_before_expiry_warning, applicare il fallback
a 3 e mantenere il clamp minimo a 0, preservando zero come valore valido.
---
Outside diff comments:
In `@app/Support/Mailer.php`:
- Around line 114-176: Estrai la costruzione e l’invio condivisi di PHPMailer da
sendPHPMailer() e sendTest() in un helper privato, ad esempio
dispatchPHPMailer(), che riceva destinatario, oggetto, corpo HTML/testo e
mittente, esegua configureMailer(), addAddress(), impostazione dei contenuti e
send(), propagando l’eccezione. Aggiorna entrambi i metodi per usare questo
helper, mantenendo in sendPHPMailer() il fallback/logging esistente e in
sendTest() la restituzione del messaggio d’errore reale.
In `@app/Support/NotificationService.php`:
- Around line 428-449: Aggiorna la preparazione delle variabili nel flusso che
invoca sendWithRetry per loan_expiring_warning, formattando giorni_rimasti come
“oggi” quando il valore è 0 e mantenendo il numero di giorni negli altri casi.
Usa il valore formattato anche nel template email, senza modificare la logica
delle notifiche in-app.
In `@storage/plugins/mobile-api/src/Controllers/ActionsController.php`:
- Around line 706-716: Rimuovi la lettura duplicata di
advanced.days_before_expiry_warning da questo blocco e riutilizza
PushDispatcher::dueSoonDays() per ottenere il valore già normalizzato con
fallback 3 e clamp a zero. Mantieni invariato il calcolo di dueSoonLimit e il
comportamento del feed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 62299552-a79f-46b2-b8ad-cbdf05c55edc
📒 Files selected for processing (20)
app/Controllers/PrestitiApiController.phpapp/Controllers/SettingsController.phpapp/Routes/web.phpapp/Support/Mailer.phpapp/Support/NotificationService.phpapp/Views/dashboard/index.phpapp/Views/prestiti/index.phpapp/Views/settings/index.phpcron/automatic-notifications.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonstorage/plugins/mobile-api/src/Controllers/ActionsController.phpstorage/plugins/mobile-api/src/Controllers/OpenApiController.phpstorage/plugins/mobile-api/src/Push/PushDispatcher.phptests/email-notifications.spec.jstests/loan-due-visibility-email.unit.phptests/mobile-api-behaviors.spec.jstests/mobile-due-push-alignment.unit.php
- web.php: throttle the new /admin/settings/email/test route with RateLimitMiddleware(5, 60, 'email_test'). It triggers an external SMTP handshake with an admin-supplied recipient, so without a limit it is a spam / SMTP-saturation vector — the other costly admin routes (sync-covers, bulk-enrich/start) already throttle. Verified end-to-end: the route returns 200 + success and Mailpit receives the message; the dev vhost's E2E bypass flag is why the throttle doesn't trip locally, prod (no flag) throttles. - settings/index.php: escape the two PHP->JS translation injections in the test-email script with json_encode(..., JSON_HEX_TAG); an apostrophe in a translation (common in it/fr) would otherwise break the inline JS and silently disable the button, per the project's PHP->JS escaping rule. - dashboard/index.php: the due-date red highlight now also requires attivo=1 and stato in (in_corso, in_ritardo), matching prestiti/index.php, so a returned/inactive loan can never be highlighted. - SettingsRepository::daysBeforeExpiryWarning(): single source of truth for the expiry-warning horizon (key + fallback + clamp); PushDispatcher and ActionsController now delegate to it instead of duplicating the logic. Unit test updated accordingly. PHPStan L5 clean (full tree); unit tests 5/5 + 9/9; send flow verified against Mailpit.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Views/settings/index.php (1)
358-358: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUsa
json_encodeper le stringhe PHP iniettate in JS.All'interno dei tag
<script>, le funzioni comehtmlspecialchars()codificano i caratteri in entità HTML (es.'), che il motore JavaScript interpreterà letteralmente, causando bug se l'URL contenesse caratteri speciali. Come da linee guida, qualsiasi dato PHP inserito in JS deve essere formattato usandojson_encode().🛡️ Fix proposto
- fetch('<?= htmlspecialchars(url('/admin/settings/email/test'), ENT_QUOTES, 'UTF-8') ?>', { + fetch(<?= json_encode(url('/admin/settings/email/test'), JSON_HEX_TAG) ?>, {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/settings/index.php` at line 358, Replace the htmlspecialchars-based URL interpolation in the fetch call with json_encode so the PHP-generated string is encoded as a valid JavaScript string literal, preserving special characters correctly.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/Views/settings/index.php`:
- Line 358: Replace the htmlspecialchars-based URL interpolation in the fetch
call with json_encode so the PHP-generated string is encoded as a valid
JavaScript string literal, preserving special characters correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d3960a4-6d4b-4dd7-8984-47b71d13be2f
📒 Files selected for processing (7)
app/Models/SettingsRepository.phpapp/Routes/web.phpapp/Views/dashboard/index.phpapp/Views/settings/index.phpstorage/plugins/mobile-api/src/Controllers/ActionsController.phpstorage/plugins/mobile-api/src/Push/PushDispatcher.phptests/mobile-due-push-alignment.unit.php
…bbit) CodeRabbit's re-review flagged the last PHP->JS injection in the test-email script: the fetch() URL still used htmlspecialchars(url(...)) inside a JS string literal. In a <script> context that emits HTML entities (e.g. ') which the JS engine reads literally, so a base path with a special character would break the request. Switched to json_encode(url(...), JSON_HEX_TAG), which emits a valid JS string literal — matching the project's PHP->JS rule and the two strings fixed in the previous commit. Verified end-to-end in the browser: clicking the button sends the test email (green success message) and Mailpit receives it; the emitted literal parses as valid JS (node --check).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Views/settings/index.php (1)
365-365: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLocalizzare i fallback
OKeError.Se la risposta JSON non contiene
message, l’interfaccia mostra testo inglese anche nelle installazioni localizzate. Usare stringhe tradotte tramitejson_encode(...)oppure garantire che l’endpoint restituisca sempre un messaggio localizzato.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/settings/index.php` at line 365, Localize the fallback strings in the response handling around res.j.message, replacing the hardcoded “OK” and “Error” values with translated strings emitted via json_encode(...). Preserve the existing preference for res.j.message when the endpoint provides one.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/Views/settings/index.php`:
- Line 365: Localize the fallback strings in the response handling around
res.j.message, replacing the hardcoded “OK” and “Error” values with translated
strings emitted via json_encode(...). Preserve the existing preference for
res.j.message when the endpoint provides one.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf817d0e-3eec-4d83-b025-e0f5ac3d5985
📒 Files selected for processing (1)
app/Views/settings/index.php
The response handler fell back to hardcoded 'OK'/'Error' when res.j.message was absent, which would show English on localized installs. The controller always returns a localized message so this is a defensive path, but localize it anyway via json_encode(__(...)), reusing 'Operazione completata' / 'Errore' — both already present in all four locales, so no new keys and no parity change. php -l clean; emitted ternary parses as valid JS (node --check).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Views/settings/index.php (1)
349-371: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAggiungere un timeout alla richiesta di test email.
fetch()non ha alcun timeout oAbortController: se l’handshake SMTP resta in attesa, laPromisenon termina,finally()non viene eseguito e il bottone rimane disabilitato con lo spinner attivo. Aggiungere un timeout lato client e verificare anche un timeout bounded nel mailer backend.Fix proposto
+ var controller = new AbortController(); + var timeoutId = window.setTimeout(function () { + controller.abort(); + }, 30000); fetch(<?= json_encode(url('/admin/settings/email/test'), JSON_HEX_TAG) ?>, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', 'X-Requested-With': 'XMLHttpRequest' }, - body: body + body: body, + signal: controller.signal }).then(function (r) { return r.json().then(function (j) { return { ok: r.ok, j: j }; }); }) @@ - .finally(function () { btn.disabled = false; btn.innerHTML = original; }); + .finally(function () { + window.clearTimeout(timeoutId); + btn.disabled = false; + btn.innerHTML = original; + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/settings/index.php` around lines 349 - 371, Aggiungi un timeout client alla richiesta fetch nel listener click del test email usando AbortController e un timer, passando il relativo signal e annullando sempre il timer quando la richiesta termina; considera il timeout come errore nel catch così il blocco finally riabilita il pulsante e ripristina il testo. Verifica inoltre che il mailer backend usato dall’endpoint di test imponga un timeout bounded per l’handshake SMTP.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/Views/settings/index.php`:
- Around line 349-371: Aggiungi un timeout client alla richiesta fetch nel
listener click del test email usando AbortController e un timer, passando il
relativo signal e annullando sempre il timer quando la richiesta termina;
considera il timeout come errore nel catch così il blocco finally riabilita il
pulsante e ripristina il testo. Verifica inoltre che il mailer backend usato
dall’endpoint di test imponga un timeout bounded per l’handshake SMTP.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a954b3e-6fb0-4885-a82a-806b3eb20c07
📒 Files selected for processing (1)
app/Views/settings/index.php
…oday email, fetch timeout)
- Mailer: extract dispatchPHPMailer() as the single build+send path returning
{ok,error}. sendPHPMailer() wraps it (fallback + logging), sendTest() calls it
directly — so the diagnostic test can never drift from the real send (CC/BCC/
AltBody changes stay in sync). The SMTP handshake already has a bounded timeout
(mailer.Timeout = mail.smtp.timeout, default 10s).
- NotificationService: the expiring-loan email printed 'Giorni rimasti: 0' for a
due-today loan. Format the giorni_rimasti template variable as 'oggi' when zero
(localized, new key added to all four locales), the number otherwise — matching
the in-app notification wording.
- settings/index.php: the test-email fetch had no timeout, so a stalled SMTP
handshake would leave the button disabled with the spinner forever. Add an
AbortController (30s) + clearTimeout in finally; an abort rejects into the
existing catch, so the button always re-enables.
PHPStan L5 clean (full tree); unit tests 9/9 + 5/5; locale parity 6542 keys x4;
button re-verified end-to-end (green success + Mailpit delivery).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Support/NotificationService.php`:
- Around line 428-436: Update the giorni_rimasti translation in the notification
variables built by the surrounding email-rendering method so “oggi” is resolved
using I18n::getInstallationLocale(), matching the locale used to send the
template; preserve the existing numeric days behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 664b60c0-de4a-4143-ab7c-1c90b8e36b0a
📒 Files selected for processing (7)
app/Support/Mailer.phpapp/Support/NotificationService.phpapp/Views/settings/index.phplocale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.json
…(CodeRabbit) The expiring-loan email template renders in the installation locale (see sendWithRetry), but the giorni_rimasti 'oggi' label was resolved with __() in the caller's current locale. These diverge when the run is triggered from the admin-login maintenance path (session locale) rather than cron, leaking the wrong language into the email. Resolve 'oggi' via a temporary switch to I18n::getInstallationLocale() — the same idiom the button translations already use — computed once per batch. The in-app notification keeps using __() (the admin's UI locale), which is correct for that surface. PHPStan L5 clean; unit test 9/9.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the Email/SMTP Testing and Due Date Visibility feedback in discussion #238.
Three related gaps: no way to tell whether SMTP works, no due-date visibility in the loans list/dashboard, and no notification for loans due today.
1. Send test email + robust SMTP
smtpdriver used a hand-rolled SMTP client that never checked server replies and returned success even when the message was rejected (the classic "SMTP looks fine but no mail arrives"). Bothsmtpandphpmailernow go through PHPMailer, which verifies the handshake/auth/recipient and surfaces the real error.2. Due-date visibility
text-red-600 font-semibold) when an active loan is due today or overdue — the same rule the Physical Copies list uses.DateHelper::today()(application timezone), consistent with the rest of the loan pipeline, instead of the browser/PHP-process local date.3. Notifications for due-today / overdue
data_scadenzaexactly attoday + N, which silently skipped a loan due today (the exact repro). Widened toBETWEEN today AND today + N; the in-app notification reads "scade oggi" at zero days.advanced.days_before_expiry_warningsetting (it previously read a key that never existed and stayed pinned to the 3-day fallback), derives "today" from the application timezone, and the loan payload exposes a server-authoritativedue_attentionflag so the Android client doesn't compute "today" from its own device timezone.Verification
success:falsewith the connection error.B.9b); mobile-apidue_attentionfor future vs due-today, and the zero-day notification horizon.Docker deployments also need an in-container scheduler for the automatic jobs to run at all — that lives in the companion
pinakes-dockerPR.Summary by CodeRabbit
due_attentionper prestiti che scadono “oggi” o sono già scaduti.