Zugferd aktivieren bzw. deaktivieren#569
Conversation
WalkthroughDie Zugferd-Einstellungsseite erhält Aktivieren/Deaktivieren-Funktionalität: neue Controller-Endpunkte und Routen im Backend, ein kontrollierter Switch mit Toast-Feedback im Frontend, eine erweiterte Toast-Komponente mit Loading-/Promise-Unterstützung sowie eine kleine Fokus-Style-Anpassung am Switch. ChangesZugferd Enable/Disable Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ZugferdSettingEdit
participant Toast
participant Router
participant ZugferdSettingController
participant ZugferdSettings
User->>ZugferdSettingEdit: Switch umschalten
ZugferdSettingEdit->>Toast: Loading-Toast anzeigen
ZugferdSettingEdit->>Router: router.put(enable/disable Route)
Router->>ZugferdSettingController: enable() oder disable()
ZugferdSettingController->>ZugferdSettings: is_enabled setzen, save()
ZugferdSettingController-->>ZugferdSettingEdit: redirect back
ZugferdSettingEdit->>Toast: Erfolgs-Toast aktualisieren
ZugferdSettingEdit->>ZugferdSettingEdit: isEnabled State aktualisieren
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: 2
🧹 Nitpick comments (4)
resources/js/Components/twc-ui/switch.tsx (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFokusring-Farbe weicht von bestehender Konvention ab.
Andere Komponenten wie
badge.tsxundcheckbox.tsxverwenden für den Fokuszustand konsistentring-ring/50. Der Wechsel zuring-primary/50nur bei Switch bricht dieses Muster und könnte den Fokusring visuell schwer vom "selected"-Zustand (bg-primary) unterscheidbar machen.Ist die Abweichung beabsichtigt (z.B. gezieltes Redesign) oder sollte hier
ring-ring/50beibehalten werden?♻️ Vorschlag zur Wiederherstellung der Konsistenz
isFocusVisible: { - true: 'border-ring ring-[3px] ring-primary/50' + true: 'border-ring ring-[3px] ring-ring/50' },🤖 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/switch.tsx` at line 25, The Switch focus ring in the twc-ui variant is inconsistent with the existing focus styling convention used by components like badge.tsx and checkbox.tsx. Update the focus state in switch.tsx (the switchVariants/variant definition that currently uses ring-primary/50) to match the shared pattern, likely ring-ring/50, unless the visual change is intentional and approved.Source: Coding guidelines
resources/js/Pages/App/Setting/ZugferdSetting/ZugferdSettingEdit.tsx (1)
62-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUngenutzten Debug-Code entfernen.
myPromisewird erzeugt, aber nie verwendet (auch der Platzhaltertext'My toast'deutet auf Test-/Debug-Code hin). Sollte vor dem Merge entfernt werden.🧹 Vorschlag
- const myPromise = new Promise<{ title: string }>(resolve => { - setTimeout(() => { - resolve({ title: 'My toast' }) - }, 3000) - }) - const toastId = toast({🤖 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 62 - 66, The debug-only promise in ZugferdSettingEdit should be removed because it is created but never used and appears to be leftover test code. Delete the unused myPromise block from the component and ensure the surrounding logic in ZugferdSettingEdit remains unchanged and clean of placeholder text like the toast title.app/Http/Controllers/App/Setting/ZugferdSettingController.php (2)
30-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDRY:
enable()/disable()sind fast identisch.Beide Methoden unterscheiden sich nur im gesetzten Boolean-Wert. Eine private Hilfsmethode reduziert Duplikation und Wartungsaufwand.
♻️ Vorschlag zur Konsolidierung
- public function enable(): RedirectResponse - { - $zugferdSettings = app(ZugferdSettings::class); - $zugferdSettings->is_enabled = true; - $zugferdSettings->save(); - - return redirect()->back(); - } - public function disable(): RedirectResponse - { - $zugferdSettings = app(ZugferdSettings::class); - $zugferdSettings->is_enabled = false; - $zugferdSettings->save(); - - return redirect()->back(); - } + public function enable(): RedirectResponse + { + return $this->setEnabled(true); + } + + public function disable(): RedirectResponse + { + return $this->setEnabled(false); + } + + private function setEnabled(bool $enabled): RedirectResponse + { + $zugferdSettings = app(ZugferdSettings::class); + $zugferdSettings->is_enabled = $enabled; + $zugferdSettings->save(); + + return redirect()->back(); + }🤖 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 30 - 45, `enable()` and `disable()` in `ZugferdSettingController` are nearly identical and only differ by the boolean value assigned to `ZugferdSettings::$is_enabled`. Refactor the duplicated save-and-redirect flow into a private helper method on `ZugferdSettingController` that accepts the desired enabled state, then have both `enable()` and `disable()` delegate to it using true/false respectively.
30-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFeature-Tests für die neuen Endpunkte fehlen.
Für die neuen
enable/disable-Aktionen sind in diesem PR keine Tests enthalten. Da diese Endpunkte einen produktionsrelevanten Umschalter steuern, wäre eine kurze Feature-Test-Abdeckung sinnvoll (z. B. Assertion aufis_enablednach dem Request).🤖 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 30 - 45, The new ZugferdSettingController enable and disable endpoints are missing feature-test coverage. Add a short feature test suite that exercises the enable() and disable() actions on ZugferdSettingController, asserting that the ZugferdSettings model’s is_enabled flag is updated correctly after each request and that the redirect response still succeeds.
🤖 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 `@resources/js/Components/twc-ui/sonner.tsx`:
- Around line 148-168: The object-based branch in toast() is dropping
isDismissible from ToastOptions, so <Toast> always falls back to its default
dismissible behavior. Update the toast() implementation in sonner.tsx to pass
props.isDismissible through to the <Toast> component alongside isLoading, title,
message, button, and type, so callers like the new loading toast flow get the
intended non-dismissible behavior.
In `@resources/js/Pages/App/Setting/ZugferdSetting/ZugferdSettingEdit.tsx`:
- Around line 54-89: The `handleEnabledChange` flow in `ZugferdSettingEdit`
handles success and finish for `router.put` but never handles failures, so add
error handling for the request. Prefer wrapping the `router.put` call in a
Promise and using `toast.promise` to match the existing loading/success/error
pattern, or at minimum add an `onError` callback that shows a failure toast and
leaves `setIsEnabled` unchanged while still clearing `setPendingStatusChange` in
`onFinish`.
---
Nitpick comments:
In `@app/Http/Controllers/App/Setting/ZugferdSettingController.php`:
- Around line 30-45: `enable()` and `disable()` in `ZugferdSettingController`
are nearly identical and only differ by the boolean value assigned to
`ZugferdSettings::$is_enabled`. Refactor the duplicated save-and-redirect flow
into a private helper method on `ZugferdSettingController` that accepts the
desired enabled state, then have both `enable()` and `disable()` delegate to it
using true/false respectively.
- Around line 30-45: The new ZugferdSettingController enable and disable
endpoints are missing feature-test coverage. Add a short feature test suite that
exercises the enable() and disable() actions on ZugferdSettingController,
asserting that the ZugferdSettings model’s is_enabled flag is updated correctly
after each request and that the redirect response still succeeds.
In `@resources/js/Components/twc-ui/switch.tsx`:
- Line 25: The Switch focus ring in the twc-ui variant is inconsistent with the
existing focus styling convention used by components like badge.tsx and
checkbox.tsx. Update the focus state in switch.tsx (the switchVariants/variant
definition that currently uses ring-primary/50) to match the shared pattern,
likely ring-ring/50, unless the visual change is intentional and approved.
In `@resources/js/Pages/App/Setting/ZugferdSetting/ZugferdSettingEdit.tsx`:
- Around line 62-66: The debug-only promise in ZugferdSettingEdit should be
removed because it is created but never used and appears to be leftover test
code. Delete the unused myPromise block from the component and ensure the
surrounding logic in ZugferdSettingEdit remains unchanged and clean of
placeholder text like the toast title.
🪄 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: 33b1319e-193f-4c98-843a-1983c0da2da8
📒 Files selected for processing (5)
app/Http/Controllers/App/Setting/ZugferdSettingController.phpresources/js/Components/twc-ui/sonner.tsxresources/js/Components/twc-ui/switch.tsxresources/js/Pages/App/Setting/ZugferdSetting/ZugferdSettingEdit.tsxroutes/tenant/settings.php
| const handleEnabledChange = async (value: boolean) => { | ||
| setPendingStatusChange(true) | ||
|
|
||
| const statusText = value ? 'aktiviert' : 'deaktiviert' | ||
| const routeName = value | ||
| ? 'app.setting.invoice.zugferd.enable' | ||
| : 'app.setting.invoice.zugferd.disable' | ||
|
|
||
| const myPromise = new Promise<{ title: string }>(resolve => { | ||
| setTimeout(() => { | ||
| resolve({ title: 'My toast' }) | ||
| }, 3000) | ||
| }) | ||
|
|
||
| const toastId = toast({ | ||
| isLoading: true, | ||
| type: 'default', | ||
| message: `ZUGFeRD wird ${statusText}.` | ||
| }) | ||
|
|
||
| router.put( | ||
| route(routeName), | ||
| {}, | ||
| { | ||
| onSuccess: () => { | ||
| toast({ | ||
| id: toastId, | ||
| type: 'success', | ||
| message: `ZUGFeRD wurde ${statusText}.` | ||
| }) | ||
| setIsEnabled(value) | ||
| }, | ||
| onFinish: () => setPendingStatusChange(false) | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Fehlende Fehlerbehandlung bei router.put.
onSuccess und onFinish werden behandelt, aber es gibt keinen onError-Handler. Schlägt der Request fehl (Server-/Netzwerkfehler), erhält der Nutzer keine Fehler-Rückmeldung, der Lade-Toast verschwindet lediglich nach der Default-Dauer von 10s (statt Infinity, wie es die neu implementierte toast.promise-Hilfsfunktion für genau diesen Fall vorsieht), und der Switch bleibt kommentarlos im alten Zustand.
Da toast.promise in diesem PR bereits für genau dieses Loading/Success/Error-Muster gebaut wurde, wäre es konsistenter, router.put in ein Promise zu wrappen und toast.promise zu nutzen, statt die Logik hier manuell (und unvollständig) zu reimplementieren.
🛡️ Minimaler Fix: `onError` ergänzen
onSuccess: () => {
toast({
id: toastId,
type: 'success',
message: `ZUGFeRD wurde ${statusText}.`
})
setIsEnabled(value)
},
+ onError: () => {
+ toast({
+ id: toastId,
+ type: 'error',
+ message: `ZUGFeRD konnte nicht ${statusText} werden.`
+ })
+ },
onFinish: () => setPendingStatusChange(false)📝 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.
| const handleEnabledChange = async (value: boolean) => { | |
| setPendingStatusChange(true) | |
| const statusText = value ? 'aktiviert' : 'deaktiviert' | |
| const routeName = value | |
| ? 'app.setting.invoice.zugferd.enable' | |
| : 'app.setting.invoice.zugferd.disable' | |
| const myPromise = new Promise<{ title: string }>(resolve => { | |
| setTimeout(() => { | |
| resolve({ title: 'My toast' }) | |
| }, 3000) | |
| }) | |
| const toastId = toast({ | |
| isLoading: true, | |
| type: 'default', | |
| message: `ZUGFeRD wird ${statusText}.` | |
| }) | |
| router.put( | |
| route(routeName), | |
| {}, | |
| { | |
| onSuccess: () => { | |
| toast({ | |
| id: toastId, | |
| type: 'success', | |
| message: `ZUGFeRD wurde ${statusText}.` | |
| }) | |
| setIsEnabled(value) | |
| }, | |
| onFinish: () => setPendingStatusChange(false) | |
| } | |
| ) | |
| } | |
| const handleEnabledChange = async (value: boolean) => { | |
| setPendingStatusChange(true) | |
| const statusText = value ? 'aktiviert' : 'deaktiviert' | |
| const routeName = value | |
| ? 'app.setting.invoice.zugferd.enable' | |
| : 'app.setting.invoice.zugferd.disable' | |
| const myPromise = new Promise<{ title: string }>(resolve => { | |
| setTimeout(() => { | |
| resolve({ title: 'My toast' }) | |
| }, 3000) | |
| }) | |
| const toastId = toast({ | |
| isLoading: true, | |
| type: 'default', | |
| message: `ZUGFeRD wird ${statusText}.` | |
| }) | |
| router.put( | |
| route(routeName), | |
| {}, | |
| { | |
| onSuccess: () => { | |
| toast({ | |
| id: toastId, | |
| type: 'success', | |
| message: `ZUGFeRD wurde ${statusText}.` | |
| }) | |
| setIsEnabled(value) | |
| }, | |
| onError: () => { | |
| toast({ | |
| id: toastId, | |
| type: 'error', | |
| message: `ZUGFeRD konnte nicht ${statusText} werden.` | |
| }) | |
| }, | |
| onFinish: () => setPendingStatusChange(false) | |
| } | |
| ) | |
| } |
🤖 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 54 - 89, The `handleEnabledChange` flow in `ZugferdSettingEdit` handles
success and finish for `router.put` but never handles failures, so add error
handling for the request. Prefer wrapping the `router.put` call in a Promise and
using `toast.promise` to match the existing loading/success/error pattern, or at
minimum add an `onError` callback that shows a failure toast and leaves
`setIsEnabled` unchanged while still clearing `setPendingStatusChange` in
`onFinish`.
There was a problem hiding this comment.
Kommt noch nach einem Check, ob Zugferd aktiviert werden kann.
There was a problem hiding this comment.
@dspangenberg Verstanden, danke für die Info. Ich lasse es dabei bewenden – wenn die Prüfung, ob ZUGFeRD aktiviert werden kann, ergänzt wird, kann die Fehlerbehandlung für router.put gleich mit berücksichtigt werden.
🧠 Learnings used
Learnt from: dspangenberg
Repo: dspangenberg/opsc PR: 309
File: resources/js/Pages/App/Bookkeeping/Receipt/ReceiptConfirm.tsx:151-157
Timestamp: 2026-03-02T21:09:25.796Z
Learning: When using React Aria Button components, use the isDisabled prop to disable the button. Do not use the native disabled prop on React Aria Button components; instead pass isDisabled={true/false}. Ensure consistent usage across all React Aria components in the codebase.
1 participant
Summary by CodeRabbit
Neue Funktionen
Bug Fixes