60222a7d91
## Summary
Fixes **TRU-128** ("Jeg kan ikke fakturere") — a customer in
#afdelingsansvarlige could not invoice because the customer-facing
invoice PUT endpoint returned a misleading 400 error.
## Root cause
`PUT /collected-invoices` in
`services/nginx/app/routes/userInvoicesRoute.php` had two related bugs:
1. **Misleading error message** — the 'both fields missing' guard
errored with
`'Missing required parameters: po_number, closed_at'`, which reads as
if BOTH fields are required. The actual condition (`&&`) only fires
when neither is set, so only one is required. Customers who tried
different combinations kept getting the same error and concluded the
system was broken.
2. **Inconsistent `closed_at` clearing** — the 'forbidden closed_at for
non-superusers' guard fired for ANY present `closed_at` key,
including `null` and `""`. That blocked customers from CLEARING a
previously-set `closed_at`, even though the handler further down
already nulls the field when it receives an empty value.
## Fix
- Reword the missing-fields error to state the actual contract:
*"At least one of po_number or closed_at must be provided"*.
- Narrow the forbidden guard to *non-empty* `closed_at`, so customers
can still pass `null` / `""` to clear a previously-set value.
The clear-on-null/empty logic further down in the handler is unchanged
— the guard now matches it.
## Test
`tests/Unit/Invoicing/UserCollectedInvoiceUpdateRouteValidationTest.php`
- Locks in the new error message.
- Locks in the new `$closed_at_is_non_empty` guard shape with the
`if (self::isParametersSet(['closed_at'])) { ... }` pre-check.
- Locks in the regression: the previous 'any present closed_at -> 403'
pattern is explicitly asserted to be absent.
## Files changed
- `services/nginx/app/routes/userInvoicesRoute.php`
-
`services/nginx/app/tests/Unit/Invoicing/UserCollectedInvoiceUpdateRouteValidationTest.php`
## Refs
- TRU-128
- Slack: #afdelingsansvarlige (kunde-rapport)
---------
Co-authored-by: OpenClaw Backend Agent <agent@openclaw.ai>
Co-authored-by: Jeppe B <jeppe@copenhagentruckwash.io>
Co-authored-by: jeppemaxclaw[bot] <bot@jeppemaxclaw.local>
Co-authored-by: Bugfix Subagent <bugfix-subagent@openclaw.local>
56 lines
3.6 KiB
PHP
56 lines
3.6 KiB
PHP
<?php
|
|
|
|
it('requires id and at least one mutable field for PUT /collected-invoices in user route', function (): void {
|
|
$routeFile = app_path('routes/userInvoicesRoute.php');
|
|
$content = file_get_contents($routeFile);
|
|
|
|
expect($content)->not->toBeFalse();
|
|
expect($content)->toContain("\$this->put('/collected-invoices'");
|
|
expect($content)->toContain("self::requireParameters(['id']);");
|
|
expect($content)->toContain("\$is_superuser = \$this->hasPermission('superuser');");
|
|
expect($content)->toContain("if (!self::isParametersSet(['po_number']) && !self::isParametersSet(['closed_at'])) {");
|
|
// TRU-128: The previous error message ("Missing required parameters:
|
|
// po_number, closed_at") read as if BOTH were required and confused
|
|
// customers trying to invoice. We now state the actual contract: at
|
|
// least one must be provided.
|
|
expect($content)->toContain("\$response->error('At least one of po_number or closed_at must be provided', 400);");
|
|
expect($content)->toContain("if (\$closed_at_is_non_empty && !\$is_superuser) {");
|
|
expect($content)->toContain("\$response->error('Forbidden: only superusers can update closed_at', 403);");
|
|
expect($content)->toContain("if ((int)\$invoice->customer_number->value() !== (int)\$user->customer_number->value() && !\$is_superuser) {");
|
|
});
|
|
|
|
it('supports independent po_number and closed_at updates for PUT /collected-invoices in user route', function (): void {
|
|
$routeFile = app_path('routes/userInvoicesRoute.php');
|
|
$content = file_get_contents($routeFile);
|
|
|
|
expect($content)->not->toBeFalse();
|
|
expect($content)->toContain("if (self::isParametersSet(['po_number'])) {");
|
|
expect($content)->toContain("\$invoice->po_number->set((string)self::getParameter('po_number'));");
|
|
|
|
expect($content)->toContain("if (self::isParametersSet(['closed_at'])) {");
|
|
expect($content)->toContain("if (\$closed_at !== null && \$closed_at !== '') {");
|
|
expect($content)->toContain("self::requireDateFormat((string)\$closed_at, self::FORMAT_DATE());");
|
|
expect($content)->toContain("\$invoice->closed_at->set(\$closed_at === null || \$closed_at === '' ? null : date('Y-m-d 23:59:59', strtotime((string)\$closed_at . ' 00:00:01')));");
|
|
});
|
|
|
|
it('locks in the TRU-128 bug fix: customers can clear closed_at with null/empty string', function (): void {
|
|
// TRU-128 / "Jeg kan ikke fakturere": a non-superuser could not pass
|
|
// closed_at at all (even null/empty) because isParametersSet() returns
|
|
// true for any present key. The route returned 403 Forbidden and the
|
|
// customer could not clear a previously-set closed_at either. The fix
|
|
// narrows the forbidden check to *non-empty* closed_at values, matching
|
|
// the existing clear-on-null/empty logic further down in the handler.
|
|
$routeFile = app_path('routes/userInvoicesRoute.php');
|
|
$content = file_get_contents($routeFile);
|
|
|
|
expect($content)->not->toBeFalse();
|
|
// The "present + non-empty" check must precede the 403 guard, so
|
|
// clearing closed_at (passing null or "") for a non-superuser is allowed.
|
|
expect($content)->toMatch(
|
|
'/\$closed_at_is_non_empty\s*=\s*false;\s*if\s*\(self::isParametersSet\(\[\'closed_at\'\]\)\)\s*\{[^}]*\$closed_at_is_non_empty\s*=\s*\(\$raw_closed_at\s*!==\s*null\s*&&\s*\$raw_closed_at\s*!==\s*\'\'\);[^}]*\}\s*if\s*\(\$closed_at_is_non_empty\s*&&\s*!\$is_superuser\)\s*\{[^}]*Forbidden:\s*only\s*superusers/s'
|
|
);
|
|
// The previous shape of the guard (which would always fire for any
|
|
// present closed_at, including null) must no longer be present.
|
|
expect($content)->not->toContain("if (self::isParametersSet(['closed_at']) && !\$is_superuser) {");
|
|
});
|