Zugferd-Einstellungen vor Aktivierung prüfen (WIP)#571
Conversation
WalkthroughVor der ZUGFeRD-Aktivierung wird nun eine Plausibilitätsprüfung (checkStettings) durchgeführt, die bei fehlender Umsatzsteuer-ID die Aktivierung mit Fehlermeldung abbricht. Zusätzlich entfällt die Befüllung von paypentTerm aus payment_deadline, und das Frontend zeigt bei Fehlern einen Error-Toast an. ChangesZUGFeRD Aktivierungsprüfung und Fehler-Feedback
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant ZugferdSettingEdit
participant ZugferdSettingController
participant ZugferdService
ZugferdSettingEdit->>ZugferdSettingController: router.put (enable)
ZugferdSettingController->>ZugferdService: checkStettings()
ZugferdService-->>ZugferdSettingController: true oder Fehlerstring
alt Fehler
ZugferdSettingController-->>ZugferdSettingEdit: redirect back withErrors
ZugferdSettingEdit->>ZugferdSettingEdit: onError Toast anzeigen
else Erfolg
ZugferdSettingController-->>ZugferdSettingEdit: is_enabled aktualisiert
ZugferdSettingEdit->>ZugferdSettingEdit: onSuccess Toast anzeigen
end
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 (3)
app/Http/Controllers/App/Setting/ZugferdSettingController.php (1)
33-36: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFehlerübergabe via
withErrors($result)erzeugt untypischen numerischen Schlüssel.
$resultist ein einzelner String.RedirectResponse::withErrors()castet dies intern zu(array) $result, wodurch eineMessageBagmit dem numerischen Schlüssel0statt einem semantischen Feldnamen entsteht (z. B.seller_contact_id). Laut Inertia-Dokumentation werden Validierungsfehler normalerweise feldbasiert (errors.first_name) bereitgestellt – die numerische Schlüsselung ist unüblich und functioniert nur, weil das Frontend exakterrors[0]erwartet (sieheZugferdSettingEdit.tsx). Das koppelt beide Seiten eng an eine implizite Konvention statt an einen aussagekräftigen Schlüssel.♻️ Vorschlag mit semantischem Fehlerschlüssel
- $result = ZugferdService::checkStettings(); - if ($result !== true) { - return redirect()->back()->withErrors($result); - } + $result = ZugferdService::checkStettings(); + if ($result !== true) { + return redirect()->back()->withErrors(['zugferd' => $result]); + }🤖 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/ZugferdSettingController.php` around lines 33 - 36, The error handling in ZugferdSettingController’s settings check uses withErrors() with a plain string from ZugferdService::checkStettings(), which produces a numeric MessageBag key instead of a semantic field name. Update the redirect in this controller to pass the error under a meaningful key that matches the failing setting (for example the relevant contact or field identifier), and keep the frontend contract in ZugferdSettingEdit.tsx aligned with that named error key rather than relying on errors[0].app/Services/ZugferdService.php (1)
234-234: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMethodenname enthält Tippfehler.
checkStettingsstattcheckSettings– wird konsistent in Facade und Controller weiterverwendet, ist aber dauerhaft im öffentlichen API-Namen sichtbar.🤖 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/Services/ZugferdService.php` at line 234, The public method name has a typo and should be corrected from checkStettings to checkSettings in ZugferdService, and all call sites that reference this API in the Facade and Controller should be updated to use the new name consistently. Keep the behavior unchanged, but ensure the renamed method remains aligned across the service and any dependent references so the public API no longer exposes the misspelling.resources/js/Pages/App/Setting/ZugferdSetting/ZugferdSettingEdit.tsx (1)
78-85: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFragiler Zugriff
errors[0]mit Doppel-Cast, kein Fallback.Der Zugriff
errors[0] as unknown as stringfunktioniert nur, weil das Backend zufällig einen numerischen Schlüssel0liefert (sieheZugferdSettingController::enable()). Laut Inertia-Doku sind Validierungsfehler normalerweise feldbasiert (z. B.errors.first_name); ändert sich künftig die Fehlerstruktur (z. B. echte Feld-Validierung), zeigt der Toastundefinedan, da kein Fallback existiert. Der doppelte Type-Cast (as unknown as string) deutet zudem darauf hin, dass der eigentliche Typ vonerrorsnicht direkt zustringkompatibel ist und hier Typsicherheit umgangen wird.🛡️ Vorschlag mit Fallback
onError: (errors) =>{ toast({ id: toastId, type: 'error', title: 'ZUGFeRD konnte nicht aktiviert werden.', - message: errors[0] as unknown as string + message: Object.values(errors)[0] ?? 'ZUGFeRD konnte nicht aktiviert werden.' }) },Based on learnings, dieses Fehler-Schema hängt eng mit dem Backend-Vertrag aus
ZugferdSettingController::enable()zusammen – eine Anpassung dort (semantischer Schlüssel statt0) sollte hier entsprechend nachgezogen werden.🤖 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/Setting/ZugferdSetting/ZugferdSettingEdit.tsx` around lines 78 - 85, Der Toast in `ZugferdSettingEdit` greift fragil über `errors[0] as unknown as string` auf einen numerischen Fehlerkey zu und hat keinen Fallback. Ersetze den Direktzugriff im `onError`-Handler durch eine robuste Auswertung der von Inertia gelieferten Fehlerstruktur (z. B. erstes vorhandenes Feld/erstes Message-Objekt) und zeige einen Default-Text an, wenn keine Fehlermeldung vorhanden ist; entferne dabei den Doppel-Cast und halte die Typen im Handler passend zu `errors` sauber.
🤖 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/Services/ZugferdService.php`:
- Around line 234-246: `ZugferdService::checkStettings()` assumes
`Contact::find($this->settings->seller_contact_id)` always returns a model, but
`seller_contact_id` can be null so `$contact` may be null and `$contact->vat_id`
can fatal. Update `checkStettings()` to guard against a missing contact before
accessing `vat_id`, returning an appropriate validation message when no contact
is configured or found. Use the existing `ZugferdSettings` and `Contact` lookup
in `checkStettings()` as the place to add the null check.
---
Nitpick comments:
In `@app/Http/Controllers/App/Setting/ZugferdSettingController.php`:
- Around line 33-36: The error handling in ZugferdSettingController’s settings
check uses withErrors() with a plain string from
ZugferdService::checkStettings(), which produces a numeric MessageBag key
instead of a semantic field name. Update the redirect in this controller to pass
the error under a meaningful key that matches the failing setting (for example
the relevant contact or field identifier), and keep the frontend contract in
ZugferdSettingEdit.tsx aligned with that named error key rather than relying on
errors[0].
In `@app/Services/ZugferdService.php`:
- Line 234: The public method name has a typo and should be corrected from
checkStettings to checkSettings in ZugferdService, and all call sites that
reference this API in the Facade and Controller should be updated to use the new
name consistently. Keep the behavior unchanged, but ensure the renamed method
remains aligned across the service and any dependent references so the public
API no longer exposes the misspelling.
In `@resources/js/Pages/App/Setting/ZugferdSetting/ZugferdSettingEdit.tsx`:
- Around line 78-85: Der Toast in `ZugferdSettingEdit` greift fragil über
`errors[0] as unknown as string` auf einen numerischen Fehlerkey zu und hat
keinen Fallback. Ersetze den Direktzugriff im `onError`-Handler durch eine
robuste Auswertung der von Inertia gelieferten Fehlerstruktur (z. B. erstes
vorhandenes Feld/erstes Message-Objekt) und zeige einen Default-Text an, wenn
keine Fehlermeldung vorhanden ist; entferne dabei den Doppel-Cast und halte die
Typen im Handler passend zu `errors` sauber.
🪄 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: 1452c149-6e04-4051-a189-8d67eb210010
📒 Files selected for processing (3)
app/Http/Controllers/App/Setting/ZugferdSettingController.phpapp/Services/ZugferdService.phpresources/js/Pages/App/Setting/ZugferdSetting/ZugferdSettingEdit.tsx
| public function checkStettings(): string|bool | ||
| { | ||
| $this->settings = app(ZugferdSettings::class); | ||
|
|
||
| $contact = Contact::with(['mails', 'phones'])->find($this->settings->seller_contact_id); | ||
|
|
||
| if (! $contact->vat_id) { | ||
| return 'Umsatzsteuer-ID fehlt'; | ||
| } | ||
|
|
||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Null-Pointer-Risiko: $contact kann null sein.
seller_contact_id ist laut ZugferdSettings (app/Settings/ZugferdSettings.php:20) nullable (?int). Ist die Einstellung (noch) nicht gesetzt, liefert Contact::find(null) null zurück (Eloquent wandelt where('id', '=', null) intern in whereNull, was nie trifft). Der nachfolgende Zugriff $contact->vat_id löst dann einen fatalen Fehler aus – genau in dem Szenario, das die Plausibilitätsprüfung eigentlich sauber abfangen soll (fehlende Konfiguration vor Aktivierung).
🐛 Vorgeschlagener Fix
public function checkStettings(): string|bool
{
$this->settings = app(ZugferdSettings::class);
$contact = Contact::with(['mails', 'phones'])->find($this->settings->seller_contact_id);
- if (! $contact->vat_id) {
+ if (! $contact || ! $contact->vat_id) {
return 'Umsatzsteuer-ID fehlt';
}
return true;
}📝 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.
| public function checkStettings(): string|bool | |
| { | |
| $this->settings = app(ZugferdSettings::class); | |
| $contact = Contact::with(['mails', 'phones'])->find($this->settings->seller_contact_id); | |
| if (! $contact->vat_id) { | |
| return 'Umsatzsteuer-ID fehlt'; | |
| } | |
| return true; | |
| } | |
| public function checkStettings(): string|bool | |
| { | |
| $this->settings = app(ZugferdSettings::class); | |
| $contact = Contact::with(['mails', 'phones'])->find($this->settings->seller_contact_id); | |
| if (! $contact || ! $contact->vat_id) { | |
| return 'Umsatzsteuer-ID fehlt'; | |
| } | |
| return true; | |
| } |
🤖 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/Services/ZugferdService.php` around lines 234 - 246,
`ZugferdService::checkStettings()` assumes
`Contact::find($this->settings->seller_contact_id)` always returns a model, but
`seller_contact_id` can be null so `$contact` may be null and `$contact->vat_id`
can fatal. Update `checkStettings()` to guard against a missing contact before
accessing `vat_id`, returning an appropriate validation message when no contact
is configured or found. Use the existing `ZugferdSettings` and `Contact` lookup
in `checkStettings()` as the place to add the null check.
1 participant
Summary by CodeRabbit
Neue Funktionen
Fehlerbehebungen