[IP-32]: implement payments options on invoice list#684
Conversation
Ran vendor/bin/pint on develop: import ordering, binary-operator spacing, and empty-body brace style. Purely cosmetic, no behavior change.
|
Warning Review limit reached
Next review available in: 59 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 (32)
📝 WalkthroughWalkthroughAdds payable invoice filtering, outstanding-balance calculation, transactional payment creation, invoice status reconciliation, invoice-list payment action coverage, and payment-form selector tests. ChangesInvoice payment workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant InvoicesTable
participant PaymentService
participant Database
User->>InvoicesTable: Open enter_payment action
InvoicesTable->>PaymentService: createPayment(payment data)
PaymentService->>Database: Create payment in transaction
PaymentService->>Database: Recalculate linked invoice status
Database-->>InvoicesTable: Payment and invoice updates
InvoicesTable-->>User: Show successful payment entry
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Modules/Payments/Filament/Company/Resources/Payments/Schemas/PaymentForm.php (1)
46-57: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPrevent N+1 query issue in the invoice selector.
By retrieving the results with
->get()and immediately calling->pluck('invoice_number', 'id'), the loaded models and eager-loadedcustomerrelationship data are discarded. Consequently, the subsequentmapWithKeysblock iterates over the array and executes a newfind($id)database query for every single returned record.You can fix this severe N+1 issue by iterating over the collection of models directly.
⚡ Proposed fix for the N+1 query
return Invoice::with('customer') ->payable() ->where(fn ($q) => $q ->where('invoice_number', 'like', "%{$search}%") ->orWhereHas('customer', fn ($q) => $q->where('company_name', 'like', "%{$search}%")) ) ->limit(50) ->get() - ->pluck('invoice_number', 'id') - ->mapWithKeys(fn ($number, $id) => [ - $id => "{$number} – " . Invoice::query()->find($id)->customer?->company_name, + ->mapWithKeys(fn (Invoice $invoice) => [ + $invoice->id => "{$invoice->invoice_number} – " . $invoice->customer?->company_name, ]) ->toArray();🤖 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/Payments/Filament/Company/Resources/Payments/Schemas/PaymentForm.php` around lines 46 - 57, Update the invoice selector’s collection transformation after get() to iterate over the retrieved Invoice models directly instead of plucking IDs and re-querying with Invoice::query()->find($id). Reuse each model’s invoice_number and already-loaded customer relationship when building the keyed options, preserving the existing labels and limit.
🧹 Nitpick comments (1)
Modules/Payments/Services/PaymentService.php (1)
31-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove commented-out code.
As a best practice for maintainability, dead or commented-out code should be removed rather than committed.
🧹 Proposed cleanup
- /* if ($payment->merchant_client_id) { - dispatch(new ProcessMerchantPaymentJob($payment)); - } */ -🤖 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/Payments/Services/PaymentService.php` around lines 31 - 34, Remove the commented-out ProcessMerchantPaymentJob dispatch block from the payment handling code, leaving the surrounding PaymentService logic unchanged.
🤖 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/Models/Invoice.php`:
- Around line 169-180: Update getOutstandingBalanceAttribute to use the loaded
payments relationship collection, filtering it for completed PaymentStatus
values and summing payment_amount without invoking the payments() query builder.
Preserve the existing outstanding-balance calculation and non-negative result.
In `@Modules/Payments/Services/PaymentService.php`:
- Around line 52-69: Update syncInvoiceStatusFromPayments so an invoice with
current status OVERDUE is not changed to PARTIALLY_PAID when paidTotal is below
invoice_total; preserve OVERDUE until the invoice reaches the existing PAID
branch, while retaining current behavior for other statuses.
---
Outside diff comments:
In
`@Modules/Payments/Filament/Company/Resources/Payments/Schemas/PaymentForm.php`:
- Around line 46-57: Update the invoice selector’s collection transformation
after get() to iterate over the retrieved Invoice models directly instead of
plucking IDs and re-querying with Invoice::query()->find($id). Reuse each
model’s invoice_number and already-loaded customer relationship when building
the keyed options, preserving the existing labels and limit.
---
Nitpick comments:
In `@Modules/Payments/Services/PaymentService.php`:
- Around line 31-34: Remove the commented-out ProcessMerchantPaymentJob dispatch
block from the payment handling code, leaving the surrounding PaymentService
logic unchanged.
🪄 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
Run ID: 45799a81-3c0a-4eb9-a713-ecb0f1e33c31
📒 Files selected for processing (8)
Modules/Invoices/Enums/InvoiceStatus.phpModules/Invoices/Filament/Company/Resources/Invoices/Tables/InvoicesTable.phpModules/Invoices/Models/Invoice.phpModules/Invoices/Tests/Feature/EnterPaymentActionTest.phpModules/Payments/Filament/Company/Resources/Payments/Schemas/PaymentForm.phpModules/Payments/Services/PaymentService.phpModules/Payments/Tests/Feature/PaymentInvoiceSelectorTest.phpresources/lang/en/ip.php
| /** | ||
| * The invoice total minus all completed payments recorded against it. | ||
| */ | ||
| public function getOutstandingBalanceAttribute(): float | ||
| { | ||
| $paid = $this->payments() | ||
| ->where('payment_status', \Modules\Payments\Enums\PaymentStatus::COMPLETED->value) | ||
| ->sum('payment_amount'); | ||
|
|
||
| return max(0, (float) $this->invoice_total - (float) $paid); | ||
| } | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Prevent N+1 queries in the outstanding balance accessor.
Using the payments() query builder relationship method executes a new database query every time this accessor is accessed. If the payments relationship has been eager-loaded, you can prevent N+1 query issues by filtering the loaded collection directly instead of hitting the database again.
⚡ Proposed optimization
public function getOutstandingBalanceAttribute(): float
{
- $paid = $this->payments()
- ->where('payment_status', \Modules\Payments\Enums\PaymentStatus::COMPLETED->value)
- ->sum('payment_amount');
+ $paid = $this->relationLoaded('payments')
+ ? $this->payments->where('payment_status', \Modules\Payments\Enums\PaymentStatus::COMPLETED->value)->sum('payment_amount')
+ : $this->payments()
+ ->where('payment_status', \Modules\Payments\Enums\PaymentStatus::COMPLETED->value)
+ ->sum('payment_amount');
return max(0, (float) $this->invoice_total - (float) $paid);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * The invoice total minus all completed payments recorded against it. | |
| */ | |
| public function getOutstandingBalanceAttribute(): float | |
| { | |
| $paid = $this->payments() | |
| ->where('payment_status', \Modules\Payments\Enums\PaymentStatus::COMPLETED->value) | |
| ->sum('payment_amount'); | |
| return max(0, (float) $this->invoice_total - (float) $paid); | |
| } | |
| /** | |
| * The invoice total minus all completed payments recorded against it. | |
| */ | |
| public function getOutstandingBalanceAttribute(): float | |
| { | |
| $paid = $this->relationLoaded('payments') | |
| ? $this->payments->where('payment_status', \Modules\Payments\Enums\PaymentStatus::COMPLETED->value)->sum('payment_amount') | |
| : $this->payments() | |
| ->where('payment_status', \Modules\Payments\Enums\PaymentStatus::COMPLETED->value) | |
| ->sum('payment_amount'); | |
| return max(0, (float) $this->invoice_total - (float) $paid); | |
| } |
🤖 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/Models/Invoice.php` around lines 169 - 180, Update
getOutstandingBalanceAttribute to use the loaded payments relationship
collection, filtering it for completed PaymentStatus values and summing
payment_amount without invoking the payments() query builder. Preserve the
existing outstanding-balance calculation and non-negative result.
| protected function syncInvoiceStatusFromPayments(Invoice $invoice): void | ||
| { | ||
| $paidTotal = $invoice->payments() | ||
| ->where('payment_status', PaymentStatus::COMPLETED->value) | ||
| ->sum('payment_amount'); | ||
|
|
||
| if ((float) $paidTotal <= 0) { | ||
| return; | ||
| } | ||
|
|
||
| if ((float) $paidTotal >= (float) $invoice->invoice_total) { | ||
| $invoice->update(['invoice_status' => InvoiceStatus::PAID->value]); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| $invoice->update(['invoice_status' => InvoiceStatus::PARTIALLY_PAID->value]); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the OVERDUE invoice status.
This method unconditionally sets the status to PARTIALLY_PAID if the invoice isn't fully paid. Based on the domain rules (and handled correctly by the existing syncInvoiceStatus method), an overdue invoice must remain OVERDUE until it is fully settled.
Please check the current status before making the downgrade to avoid corrupting the aging metrics.
🛡️ Proposed fix
if ((float) $paidTotal >= (float) $invoice->invoice_total) {
$invoice->update(['invoice_status' => InvoiceStatus::PAID->value]);
return;
}
- $invoice->update(['invoice_status' => InvoiceStatus::PARTIALLY_PAID->value]);
+ if (in_array($invoice->invoice_status, [InvoiceStatus::SENT, InvoiceStatus::VIEWED], true)) {
+ $invoice->update(['invoice_status' => InvoiceStatus::PARTIALLY_PAID->value]);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function syncInvoiceStatusFromPayments(Invoice $invoice): void | |
| { | |
| $paidTotal = $invoice->payments() | |
| ->where('payment_status', PaymentStatus::COMPLETED->value) | |
| ->sum('payment_amount'); | |
| if ((float) $paidTotal <= 0) { | |
| return; | |
| } | |
| if ((float) $paidTotal >= (float) $invoice->invoice_total) { | |
| $invoice->update(['invoice_status' => InvoiceStatus::PAID->value]); | |
| return; | |
| } | |
| $invoice->update(['invoice_status' => InvoiceStatus::PARTIALLY_PAID->value]); | |
| } | |
| protected function syncInvoiceStatusFromPayments(Invoice $invoice): void | |
| { | |
| $paidTotal = $invoice->payments() | |
| ->where('payment_status', PaymentStatus::COMPLETED->value) | |
| ->sum('payment_amount'); | |
| if ((float) $paidTotal <= 0) { | |
| return; | |
| } | |
| if ((float) $paidTotal >= (float) $invoice->invoice_total) { | |
| $invoice->update(['invoice_status' => InvoiceStatus::PAID->value]); | |
| return; | |
| } | |
| if (in_array($invoice->invoice_status, [InvoiceStatus::SENT, InvoiceStatus::VIEWED], true)) { | |
| $invoice->update(['invoice_status' => InvoiceStatus::PARTIALLY_PAID->value]); | |
| } | |
| } |
🤖 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/Payments/Services/PaymentService.php` around lines 52 - 69, Update
syncInvoiceStatusFromPayments so an invoice with current status OVERDUE is not
changed to PARTIALLY_PAID when paidTotal is below invoice_total; preserve
OVERDUE until the invoice reaches the existing PAID branch, while retaining
current behavior for other statuses.
…ed_at Expense, Payment, Project, and Task all set $timestamps = false and have no created_at column, but the RecentExpensesWidget/RecentPaymentsWidget/ RecentProjectsWidget/RecentTasksWidget dashboard widgets called ->latest() with no argument, which defaults to ordering by created_at regardless of $timestamps. MySQL (CI) raises a hard 1054 Unknown column error; SQLite silently reinterprets the double-quoted "created_at" identifier as a string literal when it can't resolve it, so the bug never surfaced in local sqlite-backed test runs.
Local tests silently diverging from CI's MariaDB (via a documented SQLite .env.testing fallback) has repeatedly masked real bugs this session — ->latest() defaulting to a nonexistent created_at column, and identifier quoting differences, both passed locally on SQLite and only failed on CI. - docker-compose.yml: cli service now injects DB_CONNECTION=mysql/DB_HOST=db etc. itself and depends_on db, so `docker compose run --rm cli php artisan test` works against real MariaDB with zero per-developer .env.testing edits - Add docker-resources/mariadb/init/01-create-test-db.sql to provision a dedicated invoiceplane_test database alongside the dev one on first boot - Fix db service: the named `database` volume was declared but never mounted, so all local dev/test data was lost on every container recreate - docker-resources/php-cli/Dockerfile: rebuild on Debian (php:8.4-cli) with the minimal proven extension set, matching the ip2-test-php:8.4 image this session used successfully throughout — see InvoicePlane#689 for a still-open false- failure issue found with a fresh cli image build, flagged in the docs - Update AGENTS.md/CLAUDE.md/README.md/.github/DOCKER.md/Makefile to point at the compose db/cli path instead of the SQLite instructions - Note throughout: use `php artisan test`, not raw vendor/bin/phpunit — the two were observed to behave differently for this app's Livewire form tests
5e50805 to
80e7422
Compare
1 participant
Closes #32
Summary by CodeRabbit
New Features
Bug Fixes