Optimierungen für Rechnungen; Prüfung der Rechnung vor freigabe (WIP)#564
Conversation
WalkthroughDie Änderungen führen eine Vorprüfung (checkForRelease) vor der Rechnungsfreigabe ein, passen die Standardkonto-Bestimmung bei Bankkonten an, erweitern die Sonner-Toast-Komponente um einen Schließen-Button und einen optionalen Titel, und ersetzen den Bestätigungsdialog beim Freigeben von Rechnungen durch direktes Toast-Feedback. ChangesRechnungsfreigabe-Validierung
Bankkonto-Standardkonto-Logik
Toast-Komponente und Typ-Erweiterung
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.2.2)PHPStan was skipped because the config uses disallowed 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: 1
🧹 Nitpick comments (6)
app/Http/Controllers/App/Setting/BankAccountController.php (1)
91-102: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRace condition beim Setzen des Standardkontos & fehlende
is_closed-Prüfung.Zwei potenzielle Probleme in der neuen Logik:
- Check-then-act ohne Sperre: Zwei parallele
store()-Aufrufe können beide$defaultAccountalsnulllesen und anschließend beideis_default = truesetzen, wodurch mehrere Standardkonten entstehen.setDefault()verwendet dafür bereits eineDB::transaction.- Ein neu angelegtes, aber sofort geschlossenes Konto (
is_closed = true) kann trotzdem als Standardkonto gesetzt werden – im Frontend (BankAccountIndexColumns.tsx, Zeile 26) ist „Als Standardkonto setzen“ für geschlossene Konten jedoch explizit deaktiviert.🔧 Vorschlag
public function store(BankAccountRequest $request): RedirectResponse { $bankAccount = BankAccount::create($request->validated()); - $defaultAccount = BankAccount::query()->where('is_default', true)->first(); - - if (! $defaultAccount && ! $bankAccount->is_paypal) { - $bankAccount->is_default = true; - $bankAccount->save(); - } + DB::transaction(function () use ($bankAccount) { + $hasDefault = BankAccount::query()->where('is_default', true)->lockForUpdate()->exists(); + + if (! $hasDefault && ! $bankAccount->is_paypal && ! $bankAccount->is_closed) { + $bankAccount->is_default = true; + $bankAccount->save(); + } + }); return redirect()->route('app.bookkeeping.bank-account.index'); }🤖 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/Http/Controllers/App/Setting/BankAccountController.php` around lines 91 - 102, The BankAccountController::store flow has a race condition when deciding the default account and it also ignores the closed-account rule. Update the default-setting logic to use the existing BankAccount::setDefault() transaction pattern (or equivalent locking/transactional check) instead of a check-then-act query on BankAccount::query()->where('is_default', true)->first(). Also ensure a newly created account is not promoted to default when it is closed by checking the is_closed state before setting is_default, matching the frontend behavior for closed accounts.resources/js/Components/twc-ui/sonner.tsx (1)
113-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClose-Button reimplementiert statt vorhandene
Button-Komponente zu nutzen.Der Aktions-Button direkt darüber (Zeilen 104-111) verwendet die geteilte
Button-Komponente, der neue Close-Button ist jedoch ein rohes<button>-Element mit manuell dupliziertem Styling. Das widerspricht der Konvention, bestehende Komponenten wiederzuverwenden, und erschwert künftige Style-Anpassungen (z. B. Fokus-Ring, Theme-Farben), die sonst zentral inButtongepflegt werden.♻️ Vorschlag: `Button`-Komponente wiederverwenden
- <button - onClick={() => sonnerToast.dismiss(id)} - type="button" - className="absolute -top-2 -left-2 flex size-5 items-center justify-center rounded-full border border-border bg-background text-muted-foreground pressed:shadow-black/20 shadow-sm transition-colors hover:bg-accent hover:text-foreground active:translate-y-px active:shadow-inner active:brightness-95" - aria-label="Close" - > - <Icon icon={Cancel01Icon} className="size-3" /> - </button> + <Button + variant="outline" + size="icon" + icon={Cancel01Icon} + onClick={() => sonnerToast.dismiss(id)} + aria-label="Close" + className="absolute -top-2 -left-2 size-5 rounded-full" + />🤖 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 `@resources/js/Components/twc-ui/sonner.tsx` around lines 113 - 120, The close button in the Sonner toast is reimplemented as a raw button with duplicated styling instead of reusing the shared Button component. Update the toast rendering in sonner.tsx to use the existing Button component for the close action, matching the pattern already used by the action button above, and pass the same dismiss handler, label, and icon through that component so centralized styling and behavior stay consistent.Source: Coding guidelines
resources/js/Layouts/AppLayout.tsx (1)
80-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerzweigung ist überflüssig —
toast()unterstützt bereits ein optionalestitle.Die Objekt-Form von
toast()insonner.tsxakzeptierttitlebereits als optionales Feld (title={props.title}, im JSX nur bei Wahrheitswert gerendert). Derif/else-Zweig hier kann daher entfallen,toastData.titleist ohnehinundefined, wenn es fehlt.♻️ Vorschlag: Verzweigung vereinfachen
- if (toastData.title) { - toast({ - title: toastData.title, - message: toastData.message, - type: toastData.type - }) - } else { - toast(toastData.message, toastData.type) - } + toast({ + title: toastData.title, + message: toastData.message, + type: toastData.type + })🤖 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 `@resources/js/Layouts/AppLayout.tsx` around lines 80 - 88, The conditional in AppLayout around the toast call is unnecessary because the toast API already accepts an optional title. Simplify the existing toast handling by always calling toast with the object form from toastData, passing title, message, and type directly, and remove the if/else branch entirely in AppLayout where toastData is used.app/Models/Invoice.php (1)
381-390: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocblock beschreibt Prüfungen, die im Code (noch) nicht umgesetzt sind.
Der Kommentar erwähnt Adress- und Umsatzsteuer-ID-Prüfungen bei Debitor bzw. Stornorechnungen, die im Methodenkörper nicht implementiert sind (es wird nur Bankkonto und Leistungszeitraum geprüft). Da die PR als „WIP“ markiert ist, ist das vermutlich beabsichtigt – trotzdem sollte der Kommentar den aktuellen Implementierungsstand widerspiegeln, um künftige Entwickler nicht in die Irre zu führen. Als per coding guidelines PHPDoc-Blöcke gegenüber Inline-Kommentaren bevorzugt werden sollten, könnte dieser Block zudem als PHPDoc über der Methode statt als Inline-Kommentar im Methodenkörper stehen.
🤖 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/Models/Invoice.php` around lines 381 - 390, Update the comment near the invoice completion check so it matches the current implementation in Invoice::... and does not claim unimplemented Debitor/address/VAT-ID validations; either trim it to only the checks actually performed (standard bank account and service period) or mark the remaining items clearly as TODO/WIP. If this block documents the method behavior, move it into a proper PHPDoc above the relevant method instead of leaving it as an inline comment inside the body.Source: Coding guidelines
app/Http/Controllers/App/InvoiceController.php (1)
381-392: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winErwäge einen benannten Schlüssel statt impliziertem Index
0beiwithErrors().
withErrors($result)mit einem reinen String wird von Laravel intern zu(array) $result(also[0 => $result]), wodurch das Frontend den Fehler über den unklaren numerischen Schlüsselerrors[0]abgreifen muss (sieheInvoiceDetailsLayout.tsx). Das funktioniert aktuell nur, weil Inertias Laravel-Adapter standardmäßig pro Feld die erste Fehlermeldung als String zurückgibt. Ein sprechender Schlüssel macht den Contract zwischen Controller und Frontend robuster und selbsterklärender, besonders fallscheckForRelease()später mehrere Fehler zurückgeben soll.♻️ Vorschlag
$result = $invoice->checkForRelease(); if ($result !== true) { - return redirect()->back()->withErrors($result); + return redirect()->back()->withErrors(['release' => $result]); }Im Frontend entsprechend
errors[0]durcherrors.releaseersetzen.🤖 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/Http/Controllers/App/InvoiceController.php` around lines 381 - 392, The release action in InvoiceController currently passes a plain string to withErrors(), which becomes an implicit numeric error key and couples the frontend to errors[0]. Update the release() flow to use a named error key when checkForRelease() returns a failure, and keep the response contract explicit so InvoiceDetailsLayout.tsx can read a stable field like errors.release instead of a numeric index. Ensure the change is localized around release() and the withErrors() call.resources/js/Pages/App/Invoice/InvoiceDetailsLayout.tsx (1)
114-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnsicherer Double-Cast
as unknown as stringbei der Fehlermeldung.
errors[0] as unknown as stringdeutet darauf hin, dass der TS-Typ vonerrors[0]nicht direkt alsstringkompatibel ist (vermutlichstring | undefined). Der Cast umgeht die Typprüfung, statt denundefined-Fall abzufangen. Außerdem hängt der Zugriff über den numerischen Index0am serverseitigenwithErrors($result)-Aufruf inInvoiceController::release()– falls dort ein sprechender Key verwendet wird (siehe Kommentar dort), sollte hiererrors.releasestatterrors[0]verwendet werden.♻️ Vorschlag
onError: errors => { toast({ type: 'error', title: `Die Rechnung kann nicht abgeschlossen werden.`, - message: errors[0] as unknown as string + message: errors[0] ?? 'Unbekannter Fehler.' }) }🤖 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 `@resources/js/Pages/App/Invoice/InvoiceDetailsLayout.tsx` around lines 114 - 119, The error handling in InvoiceDetailsLayout’s onError callback is using an unsafe double-cast on errors[0], which should be removed. Update the toast message to handle the error type directly by checking for a defined value or using the typed key from the server-side error payload if available. If InvoiceController::release returns a named validation error, switch this callback to use that named property instead of the numeric index so the message stays type-safe and avoids the undefined case.
🤖 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/Models/Invoice.php`:
- Around line 397-405: Fix the validation logic in Invoice::servicePeriod
validation so it checks both $this->service_period_begin and
$this->service_period_end instead of testing service_period_end twice, ensuring
the lines fallback runs when either bound is missing. Also guard the
$this->type->key access by handling a missing type relation safely in the same
block, so a missing InvoiceType record does not cause a null-pointer error.
---
Nitpick comments:
In `@app/Http/Controllers/App/InvoiceController.php`:
- Around line 381-392: The release action in InvoiceController currently passes
a plain string to withErrors(), which becomes an implicit numeric error key and
couples the frontend to errors[0]. Update the release() flow to use a named
error key when checkForRelease() returns a failure, and keep the response
contract explicit so InvoiceDetailsLayout.tsx can read a stable field like
errors.release instead of a numeric index. Ensure the change is localized around
release() and the withErrors() call.
In `@app/Http/Controllers/App/Setting/BankAccountController.php`:
- Around line 91-102: The BankAccountController::store flow has a race condition
when deciding the default account and it also ignores the closed-account rule.
Update the default-setting logic to use the existing BankAccount::setDefault()
transaction pattern (or equivalent locking/transactional check) instead of a
check-then-act query on BankAccount::query()->where('is_default',
true)->first(). Also ensure a newly created account is not promoted to default
when it is closed by checking the is_closed state before setting is_default,
matching the frontend behavior for closed accounts.
In `@app/Models/Invoice.php`:
- Around line 381-390: Update the comment near the invoice completion check so
it matches the current implementation in Invoice::... and does not claim
unimplemented Debitor/address/VAT-ID validations; either trim it to only the
checks actually performed (standard bank account and service period) or mark the
remaining items clearly as TODO/WIP. If this block documents the method
behavior, move it into a proper PHPDoc above the relevant method instead of
leaving it as an inline comment inside the body.
In `@resources/js/Components/twc-ui/sonner.tsx`:
- Around line 113-120: The close button in the Sonner toast is reimplemented as
a raw button with duplicated styling instead of reusing the shared Button
component. Update the toast rendering in sonner.tsx to use the existing Button
component for the close action, matching the pattern already used by the action
button above, and pass the same dismiss handler, label, and icon through that
component so centralized styling and behavior stay consistent.
In `@resources/js/Layouts/AppLayout.tsx`:
- Around line 80-88: The conditional in AppLayout around the toast call is
unnecessary because the toast API already accepts an optional title. Simplify
the existing toast handling by always calling toast with the object form from
toastData, passing title, message, and type directly, and remove the if/else
branch entirely in AppLayout where toastData is used.
In `@resources/js/Pages/App/Invoice/InvoiceDetailsLayout.tsx`:
- Around line 114-119: The error handling in InvoiceDetailsLayout’s onError
callback is using an unsafe double-cast on errors[0], which should be removed.
Update the toast message to handle the error type directly by checking for a
defined value or using the typed key from the server-side error payload if
available. If InvoiceController::release returns a named validation error,
switch this callback to use that named property instead of the numeric index so
the message stays type-safe and avoids the undefined case.
🪄 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: CHILL
Plan: Pro
Run ID: 7b19a722-8966-41da-898b-2a63e9dff2a9
📒 Files selected for processing (9)
app/Http/Controllers/App/InvoiceController.phpapp/Http/Controllers/App/Setting/BankAccountController.phpapp/Models/Invoice.phpresources/js/Components/twc-ui/sonner.tsxresources/js/Layouts/AppLayout.tsxresources/js/Pages/App/Invoice/InvoiceDetailsLayout.tsxresources/js/Pages/App/Setting/BankAccount/BankAccountEdit.tsxresources/js/Pages/App/Setting/BankAccount/BankAccountIndexColumns.tsxresources/js/Types/global.d.ts
1 participant
Summary by CodeRabbit
Neue Funktionen
Fehlerbehebungen