[IP-480]: Send invoice reminders for overdue invoices. - #698
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds overdue invoice reminders with editable recipient, subject, and body fields. The feature queues a PDF-attached mailable, records each send in ChangesInvoice reminder flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SendReminderAction
participant InvoiceService
participant InvoiceReminderMailable
participant MailQueue
User->>SendReminderAction: Open reminder action
SendReminderAction->>InvoiceService: Resolve reminder defaults
User->>SendReminderAction: Submit recipient, subject, and body
SendReminderAction->>InvoiceService: Send reminder
InvoiceService->>InvoiceReminderMailable: Queue PDF-attached email
InvoiceService->>MailQueue: Store reminder type and sent_at
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 2
🧹 Nitpick comments (7)
Modules/Core/Database/Seeders/EmailTemplatesSeeder.php (1)
31-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReminder template content differs from the company-bootstrap default.
This
invoice_remindertemplate's subject and body differ from the one created inCompanyDefaultsBootstrapService::bootstrap()for the same title. Companies backfilled through this seeder and companies created afterward will get different default reminder wording.🤖 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 `@Modules/Core/Database/Seeders/EmailTemplatesSeeder.php` around lines 31 - 36, Update the invoice_reminder entry in EmailTemplatesSeeder to use the same subject and body as the invoice_reminder template defined by CompanyDefaultsBootstrapService::bootstrap(), keeping the title and company_id assignments unchanged.Modules/Core/Services/CompanyDefaultsBootstrapService.php (2)
62-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
from_emailcomputation.The
from_emailexpression at line 72 is identical to the one at line 56 forinvoice_sent. Compute it once and reuse it for both templates.♻️ Proposed fix
+ $fromEmail = 'billing@' . mb_strtolower(preg_replace('/[^A-Za-z0-9]/', '', $company->name)) . '.com'; + // Create default email template EmailTemplate::query()->firstOrCreate( [ 'company_id' => $company->id, 'title' => 'invoice_sent', ], [ 'subject' => 'Invoice #{{ invoice.number }}', 'body' => 'Please find your invoice #{{ invoice.number }} attached.', 'from_name' => $company->name, - 'from_email' => 'billing@' . mb_strtolower(preg_replace('/[^A-Za-z0-9]/', '', $company->name)) . '.com', + 'from_email' => $fromEmail, 'cc' => null, 'bcc' => null, ] ); // Create default reminder email template EmailTemplate::query()->firstOrCreate( [ 'company_id' => $company->id, 'title' => 'invoice_reminder', ], [ 'subject' => 'Payment reminder — Invoice #{{ invoice.number }}', 'body' => 'Invoice #{{ invoice.number }} is overdue. Please arrange payment at your earliest convenience.', 'from_name' => $company->name, - 'from_email' => 'billing@' . mb_strtolower(preg_replace('/[^A-Za-z0-9]/', '', $company->name)) . '.com', + 'from_email' => $fromEmail, 'cc' => null, 'bcc' => null, ] );🤖 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 `@Modules/Core/Services/CompanyDefaultsBootstrapService.php` around lines 62 - 77, Extract the shared from_email computation before the invoice_sent and invoice_reminder template creation in CompanyDefaultsBootstrapService, assign it to a local variable, and reuse that variable in both EmailTemplate::query()->firstOrCreate calls. Remove both duplicated inline expressions while preserving the resulting email value.
68-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReminder template body is missing placeholders used elsewhere.
This body only uses
{{ invoice.number }}.InvoiceService::resolveReminderDefaults()also fillscustomer.name,invoice.total_formatted,invoice.due_date_formatted, andcompany.name. Companies created after this change get a sparser reminder than the one seeded throughEmailTemplatesSeederfor the same title.🤖 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 `@Modules/Core/Services/CompanyDefaultsBootstrapService.php` around lines 68 - 76, Update the reminder template body in CompanyDefaultsBootstrapService to include the placeholders populated by InvoiceService::resolveReminderDefaults(): customer.name, invoice.total_formatted, invoice.due_date_formatted, and company.name, matching the seeded template for the same subject.Modules/Invoices/Services/InvoiceService.php (1)
275-346: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winExtract shared reminder/email defaults-resolution logic.
resolveReminderDefaults()duplicatesresolveEmailDefaults()almost line for line, differing only in the template title constant and the translation key for the default subject.sendReminder()also re-fetches the sameEmailTemplaterow (line 326-328) thatresolveReminderDefaults()(called at line 316) already looked up internally, causing a redundant query on every send.Extract a private helper, e.g.
resolveDefaultsForTemplate(Invoice $invoice, string $templateTitle, string $defaultSubjectKey): arraythat returns both the rendered defaults and the resolvedEmailTemplate, and have bothresolveEmailDefaults()/resolveReminderDefaults()andsendReminder()reuse it.🤖 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 `@Modules/Invoices/Services/InvoiceService.php` around lines 275 - 346, Extract the shared template/default resolution used by resolveEmailDefaults() and resolveReminderDefaults() into a private helper such as resolveDefaultsForTemplate(), parameterized by the template title and default-subject translation key, returning both rendered defaults and the resolved EmailTemplate. Update both public resolvers to use it, and change sendReminder() to reuse the helper result instead of querying EmailTemplate again.Modules/Invoices/Filament/Company/Actions/SendReminderAction.php (1)
28-56: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid resolving reminder defaults twice per row for
disabled()/tooltip().
disabled()andtooltip()each independently callresolveReminderDefaults($record), which does a full template lookup and customer/contact traversal just to check whether the recipient is blank. In a table listing many overdue invoices, this doubles the query load per row.Resolve the recipient once and reuse it, or expose a lighter check (e.g. a public
hasReminderRecipient(Invoice $invoice): boolwrapping the existing private recipient-resolution logic) fordisabled()/tooltip(), reserving the fullresolveReminderDefaults()call for the modalschema().♻️ Proposed direction
- ->disabled(fn (Invoice $record): bool => blank(app(InvoiceService::class)->resolveReminderDefaults($record)['recipient'])) - ->tooltip(fn (Invoice $record): ?string => blank(app(InvoiceService::class)->resolveReminderDefaults($record)['recipient']) - ? trans('ip.customer_has_no_email') - : null) + ->disabled(fn (Invoice $record): bool => ! app(InvoiceService::class)->hasReminderRecipient($record)) + ->tooltip(fn (Invoice $record): ?string => app(InvoiceService::class)->hasReminderRecipient($record) + ? null + : trans('ip.customer_has_no_email'))🤖 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 `@Modules/Invoices/Filament/Company/Actions/SendReminderAction.php` around lines 28 - 56, Update SendReminderAction::make so disabled() and tooltip() do not each call InvoiceService::resolveReminderDefaults($record). Reuse a single recipient-resolution result or add a lightweight public InvoiceService::hasReminderRecipient(Invoice $invoice) method backed by the existing recipient logic, and use it for both callbacks while retaining the full resolveReminderDefaults() call only in schema().Modules/Invoices/Mail/InvoiceReminderMailable.php (1)
19-24: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAvoid serializing the full PDF binary into the queued job payload.
pdfBinaryis a public property on aShouldQueuemailable, so the entire rendered PDF gets serialized into the queue payload (database row, Redis value, or SQS message, depending on driver) instead of being generated when the job actually runs. Large invoices can produce sizeable PDFs, which bloats queue storage and risks exceeding payload limits on size-constrained queue backends (e.g., SQS's 256 KB limit).Pass the
Invoiceonly, and generate the PDF lazily insideattachments()(or a queued job) at send time instead of eagerly in the caller.Please confirm which queue connection this application uses in production (
config/queue.php), since driver-specific payload limits affect how severe this risk is.Also applies to: 40-48
🤖 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 `@Modules/Invoices/Mail/InvoiceReminderMailable.php` around lines 19 - 24, Remove the public pdfBinary dependency from InvoiceReminderMailable and its constructor, retaining only the Invoice and email content needed by the queued mailable. Update the caller and attachments() flow so the PDF is generated lazily from the Invoice when the message is sent, rather than serialized into the queue payload; verify the production queue connection in config/queue.php to account for its payload limits.Modules/Core/Models/MailQueue.php (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a backed enum for
typeinstead of a raw string.
typestores a categorical value (currently only'reminder', defined as a bare string constant inInvoiceService::REMINDER_MAIL_TYPE). Other classification fields in this codebase (InvoiceStatus,CommunicationType,PaymentMethod) use backed enums cast via$casts. Introduce aMailTypeenum and casttypeto it, then updateInvoiceServiceto reference the enum case instead of a plain string constant.As per coding guidelines, "Use
$castsin models for enum fields instead of JSON or ENUM database columns."Also applies to: 23-23, 34-34
🤖 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 `@Modules/Core/Models/MailQueue.php` around lines 7 - 13, Introduce a string-backed MailType enum for the mail queue categories, replace the raw type declaration with the enum and add the corresponding enum cast in MailQueue::$casts. Update InvoiceService::REMINDER_MAIL_TYPE usages to reference the MailType reminder case, removing the bare-string constant while preserving the existing stored value.Source: Coding guidelines
🤖 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 `@Modules/Invoices/Tests/Feature/SendReminderActionTest.php`:
- Around line 30-46: Add the required /* Arrange */ block comment before the
EmailTemplate update setup in
it_prefills_the_modal_from_the_companys_reminder_email_template, while
preserving the existing explanatory comment and the following /* Act */ label.
- Around line 107-131: The test's not-yet-due invoice uses a fixed date that can
become past due; update the invoice_due_at value in
it_hides_the_send_reminder_action_for_an_invoice_that_is_not_yet_due to a future
date relative to test execution, such as one year from now, while preserving the
existing overdue invoice case.
---
Nitpick comments:
In `@Modules/Core/Database/Seeders/EmailTemplatesSeeder.php`:
- Around line 31-36: Update the invoice_reminder entry in EmailTemplatesSeeder
to use the same subject and body as the invoice_reminder template defined by
CompanyDefaultsBootstrapService::bootstrap(), keeping the title and company_id
assignments unchanged.
In `@Modules/Core/Models/MailQueue.php`:
- Around line 7-13: Introduce a string-backed MailType enum for the mail queue
categories, replace the raw type declaration with the enum and add the
corresponding enum cast in MailQueue::$casts. Update
InvoiceService::REMINDER_MAIL_TYPE usages to reference the MailType reminder
case, removing the bare-string constant while preserving the existing stored
value.
In `@Modules/Core/Services/CompanyDefaultsBootstrapService.php`:
- Around line 62-77: Extract the shared from_email computation before the
invoice_sent and invoice_reminder template creation in
CompanyDefaultsBootstrapService, assign it to a local variable, and reuse that
variable in both EmailTemplate::query()->firstOrCreate calls. Remove both
duplicated inline expressions while preserving the resulting email value.
- Around line 68-76: Update the reminder template body in
CompanyDefaultsBootstrapService to include the placeholders populated by
InvoiceService::resolveReminderDefaults(): customer.name,
invoice.total_formatted, invoice.due_date_formatted, and company.name, matching
the seeded template for the same subject.
In `@Modules/Invoices/Filament/Company/Actions/SendReminderAction.php`:
- Around line 28-56: Update SendReminderAction::make so disabled() and tooltip()
do not each call InvoiceService::resolveReminderDefaults($record). Reuse a
single recipient-resolution result or add a lightweight public
InvoiceService::hasReminderRecipient(Invoice $invoice) method backed by the
existing recipient logic, and use it for both callbacks while retaining the full
resolveReminderDefaults() call only in schema().
In `@Modules/Invoices/Mail/InvoiceReminderMailable.php`:
- Around line 19-24: Remove the public pdfBinary dependency from
InvoiceReminderMailable and its constructor, retaining only the Invoice and
email content needed by the queued mailable. Update the caller and attachments()
flow so the PDF is generated lazily from the Invoice when the message is sent,
rather than serialized into the queue payload; verify the production queue
connection in config/queue.php to account for its payload limits.
In `@Modules/Invoices/Services/InvoiceService.php`:
- Around line 275-346: Extract the shared template/default resolution used by
resolveEmailDefaults() and resolveReminderDefaults() into a private helper such
as resolveDefaultsForTemplate(), parameterized by the template title and
default-subject translation key, returning both rendered defaults and the
resolved EmailTemplate. Update both public resolvers to use it, and change
sendReminder() to reuse the helper result instead of querying EmailTemplate
again.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3feb0264-ef75-4ae4-9083-3f0c717bec12
📒 Files selected for processing (12)
Modules/Core/Database/Migrations/2026_07_31_000001_add_type_and_sent_at_to_mail_queue_table.phpModules/Core/Database/Seeders/EmailTemplatesSeeder.phpModules/Core/Models/MailQueue.phpModules/Core/Services/CompanyDefaultsBootstrapService.phpModules/Invoices/Filament/Company/Actions/SendReminderAction.phpModules/Invoices/Filament/Company/Resources/Invoices/Pages/EditInvoice.phpModules/Invoices/Filament/Company/Resources/Invoices/Schemas/InvoiceForm.phpModules/Invoices/Filament/Company/Resources/Invoices/Tables/InvoicesTable.phpModules/Invoices/Mail/InvoiceReminderMailable.phpModules/Invoices/Services/InvoiceService.phpModules/Invoices/Tests/Feature/SendReminderActionTest.phpresources/lang/en/ip.php
| public function it_hides_the_send_reminder_action_for_an_invoice_that_is_not_yet_due(): void | ||
| { | ||
| /* Arrange */ | ||
| $customer = Relation::factory()->for($this->company)->customer()->create(); | ||
| $numbering = Numbering::factory()->for($this->company)->create(); | ||
| $notYetDue = $this->makeInvoice($customer, $numbering, [ | ||
| 'invoice_number' => 'INV-NOT-DUE', | ||
| 'invoice_status' => InvoiceStatus::SENT->value, | ||
| 'invoice_due_at' => '2026-06-09', | ||
| ]); | ||
| $overdue = $this->makeInvoice($customer, $numbering, [ | ||
| 'invoice_number' => 'INV-OVERDUE', | ||
| 'invoice_status' => InvoiceStatus::SENT->value, | ||
| 'invoice_due_at' => '2025-06-09', | ||
| ]); | ||
|
|
||
| /* Act */ | ||
| $component = $this->listInvoices(); | ||
|
|
||
| /* Assert */ | ||
| $component | ||
| ->assertSuccessful() | ||
| ->assertActionHidden(TestAction::make('send_reminder')->table($notYetDue)) | ||
| ->assertActionVisible(TestAction::make('send_reminder')->table($overdue)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether AbstractCompanyPanelTestCase (or a parent) freezes the clock in tests.
fd AbstractCompanyPanelTestCase --exec cat -n {}
fd AbstractTestCase --exec rg -n "setTestNow|travelTo" {} \;Repository: InvoicePlane/InvoicePlane-v2
Length of output: 2082
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- repository files availability ---"
if [ -d .git ]; then
echo "repository present"
git rev-parse --show-toplevel
git status --short
else
echo "no .git directory"
pwd
ls -la
fi
echo "--- locate target test and base cases ---"
fd -a "SendReminderActionTest.php|AbstractCompanyPanelTestCase.php|AbstractTestCase.php" . || true
echo "--- inspect target test around due dates/time mocking ---"
TARGET="$(fd "SendReminderActionTest.php"_modules/Invoices/Tests/Feature || true)"
if [ -n "${TARGET:-}" ] && [ "$(printf '%s' "$TARGET" | wc -l)" -eq 1 ]; then
cat -n "$TARGET" | sed -n '1,180p'
fi
echo "--- inspect base case names and time mocking references ---"
for f in $(fd "AbstractCompanyPanelTestCase.php|AbstractTestCase.php" . | wc -l | tr '\n' ' '); do
true
done
fd "AbstractCompanyPanelTestCase.php|AbstractTestCase.php" . --depth 5 --exec sh -c '
echo "===== $1 ====="
rg -n "setTestNow|travelTo|testNow|refreshDatabase|databaseTransactions" "$1" || true
sed -n "1,220p" "$1"
' sh {}Repository: InvoicePlane/InvoicePlane-v2
Length of output: 2053
Keep the not yet due invoice due date relative to test execution.
'invoice_due_at' => '2026-06-09' evaluates against the current date. If the test runs after June 9, 2026, SendReminderAction::isOverdue($notYetDue) becomes true, so the hidden action assertion can fail. Use a relative date such as now()->addYear()->toDateString() unless the test freezes time. Use a future relative date unless the test has a time freeze.
🤖 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 `@Modules/Invoices/Tests/Feature/SendReminderActionTest.php` around lines 107 -
131, The test's not-yet-due invoice uses a fixed date that can become past due;
update the invoice_due_at value in
it_hides_the_send_reminder_action_for_an_invoice_that_is_not_yet_due to a future
date relative to test execution, such as one year from now, while preserving the
existing overdue invoice case.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Align invoice_reminder template content between EmailTemplatesSeeder and CompanyDefaultsBootstrapService, and extract the duplicated from_email computation. - Extract InvoiceService::resolveTemplateDefaults() shared by resolveEmailDefaults()/resolveReminderDefaults()/sendReminder(), removing a redundant EmailTemplate query on every reminder send. - Add InvoiceService::hasReminderRecipient() and use it in SendReminderAction's disabled()/tooltip() instead of each re-resolving full template defaults. - Make InvoiceReminderMailable render the PDF lazily in attachments() at send time instead of serializing a pre-rendered binary into the queue payload. - Introduce a MailType backed enum for MailQueue::type, replacing the bare-string InvoiceService::REMINDER_MAIL_TYPE constant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@KhawarMehfooz thanks man! Merged! |
2 participants
Pull Request Checklist
Checklist
Description
Adds a "Send Reminder" action for overdue invoices. From the invoice list or the invoice edit page, a user with the
email-invoicespermission can send a payment reminder for any invoice that isSent/Overdueand past its due date. The action opens a modal pre-filled from the company'sinvoice_reminderemail template (recipient resolved the same way as the existing "Email Invoice" action, subject/body rendered from the template), lets the user edit before sending, attaches the invoice PDF, and logs aMailQueuerow (type = reminder,sent_atpopulated) so every reminder is auditable. The invoice edit page now shows a "Last reminder sent" field sourced from that history. The action is disabled with a tooltip when the client has no resolvable email, and repeated sends create separateMailQueuerows rather than overwriting.Also adds the
typeandsent_atcolumns tomail_queue(previously missing) and seeds a defaultinvoice_reminderemail template per company, alongside the existinginvoice_senttemplate.Known limitation: PDF attachment depends on
dompdf/dompdf, which is declared incomposer.lockbut isn't present in the CI/test vendor tree (a pre-existing, documented gap — seeInvoicePdfAndCreditNoteTest). The three tests that exercise real PDF generation andMailQueuelogging are tagged#[Group('failing')], consistent with how the existing PDF-download test is already excluded from the default suite.Related Issue(s)
Fixes #480
Motivation and Context
Chasing overdue invoices by hand is time-consuming and easy to forget. This gives users a one-click way to send a reminder without leaving InvoicePlane, using the customer's existing email and invoice details, and keeps an auditable record of every reminder sent — as specified in #480's acceptance criteria.
Issue Type (Check one or more)
Screenshots (If Applicable)
Summary by CodeRabbit