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>
219 lines
8.6 KiB
PHP
219 lines
8.6 KiB
PHP
<?php
|
|
|
|
declare(strict_types=1);
|
|
|
|
use objects\products_o;
|
|
|
|
usesApiSuite();
|
|
|
|
/**
|
|
* Tests for TRU-94: product merging infrastructure.
|
|
*
|
|
* Verifies that:
|
|
* - Merging product A into B preserves historical order_items references (FK still points at A)
|
|
* - The source product's merged_into_product_id is set, and resolveActiveProductId() follows it
|
|
* - An audit row is written to product_merges
|
|
* - Price changes to the target (B) are what new orders will see (since reads resolve to the target)
|
|
* - The API endpoint POST /products/{id}/merge behaves as expected and validates inputs
|
|
* - The schema is additive and idempotent (running the bootstrap twice is safe)
|
|
*/
|
|
|
|
it('adds merged_into_product_id to products and product_merges table is idempotent', function (): void {
|
|
api_test_covers('schema', 'product-merges');
|
|
|
|
// Calling ensureTables on a clean test DB should be a no-op (column/table already exist
|
|
// from earlier schema runs in the same suite, or it should add them without error).
|
|
\classes\products_schema_bootstrap::ensureTables();
|
|
\classes\products_schema_bootstrap::ensureTables();
|
|
|
|
$db = api_test_runtime()->db();
|
|
$col = $db->query("SHOW COLUMNS FROM `products` LIKE 'merged_into_product_id'");
|
|
expect($col)->not->toBeFalse();
|
|
expect((int)$col->num_rows)->toBe(1);
|
|
|
|
$tbl = $db->query("SHOW TABLES LIKE 'product_merges'");
|
|
expect($tbl)->not->toBeFalse();
|
|
expect((int)$tbl->num_rows)->toBe(1);
|
|
});
|
|
|
|
it('resolveActiveProductId follows merged_into_product_id', function (): void {
|
|
$source = api_fixtures()->createProduct([
|
|
'name' => 'SF Source (Lastbil)',
|
|
'price' => 100,
|
|
]);
|
|
$target = api_fixtures()->createProduct([
|
|
'name' => 'SF Target (Lastbil)',
|
|
'price' => 150,
|
|
]);
|
|
|
|
$sourceObj = (new products_o())->select((int)$source['id']);
|
|
expect($sourceObj->exists())->toBeTrue();
|
|
expect($sourceObj->resolveActiveProductId())->toBe((int)$source['id']);
|
|
|
|
// No chain yet, and target is unchanged
|
|
$targetObj = (new products_o())->select((int)$target['id']);
|
|
expect($targetObj->resolveActiveProductId())->toBe((int)$target['id']);
|
|
|
|
// Perform the merge
|
|
$sourceObj->mergeInto((int)$target['id'], null, 'TRU-94 test merge');
|
|
|
|
expect((int)$sourceObj->merged_into_product_id->value())->toBe((int)$target['id']);
|
|
expect($sourceObj->resolveActiveProductId())->toBe((int)$target['id']);
|
|
|
|
// Reload from DB to confirm persistence
|
|
$reloaded = (new products_o())->select((int)$source['id']);
|
|
expect($reloaded->resolveActiveProductId())->toBe((int)$target['id']);
|
|
expect((int)$reloaded->merged_into_product_id->value())->toBe((int)$target['id']);
|
|
});
|
|
|
|
it('mergeInto preserves historical order_items references and writes an audit row', function (): void {
|
|
$source = api_fixtures()->createProduct([
|
|
'name' => 'Legacy SF',
|
|
'price' => 200,
|
|
]);
|
|
$target = api_fixtures()->createProduct([
|
|
'name' => 'New SF',
|
|
'price' => 250,
|
|
]);
|
|
|
|
// Create a historical order and order_item that points at the source.
|
|
$user = api_fixtures()->createUser(['name' => 'Merge Test User']);
|
|
$cashier = api_fixtures()->createUser(['name' => 'Merge Test Cashier']);
|
|
$department = api_fixtures()->createDepartment();
|
|
$order = api_fixtures()->createOrder([
|
|
'customer_id' => (int)$user['id'],
|
|
'department_id' => (int)$department['id'],
|
|
]);
|
|
$item = api_fixtures()->createOrderItem([
|
|
'order_id' => (int)$order['id'],
|
|
'product_id' => (int)$source['id'],
|
|
'cashier_id' => (int)$cashier['id'],
|
|
'price' => 200,
|
|
'quantity' => 1,
|
|
]);
|
|
|
|
expect((int)$item['product_id'])->toBe((int)$source['id']);
|
|
|
|
// Merge source into target
|
|
(new products_o())->select((int)$source['id'])->mergeInto((int)$target['id'], null, 'TRU-94 historical preservation');
|
|
|
|
// Historical order_items.product_id MUST still point at the source.
|
|
// (This is the whole point of the merge: we don't rewrite history.)
|
|
$db = api_test_runtime()->db();
|
|
$row = $db->query("SELECT product_id FROM order_items WHERE id = " . (int)$item['id'])->fetch_array();
|
|
expect((int)$row['product_id'])->toBe((int)$source['id']);
|
|
|
|
// Audit row exists
|
|
$audit = $db->query("SELECT * FROM product_merges WHERE source_product_id = " . (int)$source['id'])->fetch_array();
|
|
expect($audit)->not->toBeNull();
|
|
expect((int)$audit['source_product_id'])->toBe((int)$source['id']);
|
|
expect((int)$audit['target_product_id'])->toBe((int)$target['id']);
|
|
expect($audit['reason'])->toBe('TRU-94 historical preservation');
|
|
});
|
|
|
|
it('price change on the target is what new orders see (resolution goes to target)', function (): void {
|
|
$source = api_fixtures()->createProduct(['name' => 'SF Pre-Merge', 'price' => 100]);
|
|
$target = api_fixtures()->createProduct(['name' => 'SF Post-Merge', 'price' => 100]);
|
|
|
|
(new products_o())->select((int)$source['id'])->mergeInto((int)$target['id']);
|
|
|
|
// Simulate a price change on the target (the only product new orders can be placed against)
|
|
$targetObj = (new products_o())->select((int)$target['id']);
|
|
$targetObj->price->set(175);
|
|
|
|
// The source still resolves to the target, and a fresh read of the target shows the new price
|
|
$resolvedId = (new products_o())->select((int)$source['id'])->resolveActiveProductId();
|
|
expect($resolvedId)->toBe((int)$target['id']);
|
|
|
|
$reloaded = (new products_o())->select($resolvedId);
|
|
expect((int)$reloaded->price->value())->toBe(175);
|
|
});
|
|
|
|
it('POST /products/{id}/merge requires edit_product permission', function (): void {
|
|
$source = api_fixtures()->createProduct(['name' => 'Perm Source']);
|
|
$target = api_fixtures()->createProduct(['name' => 'Perm Target']);
|
|
|
|
// IMPORTANT: do NOT pass ['group_id' => 1] here. group_id 1 is a
|
|
// hardcoded superuser/admin in objects\users_o::hasPermission() and
|
|
// bypasses the groups_permissions check entirely, so the route would
|
|
// 200 instead of 403. createUserSession([], []) creates a fresh empty
|
|
// group (id > 1) with no permissions, which is what this test needs.
|
|
$session = api_fixtures()->createUserSession([], []);
|
|
$response = api_client()->post(
|
|
'/products/' . (int)$source['id'] . '/merge',
|
|
['target_id' => (int)$target['id']],
|
|
$session['headers']
|
|
);
|
|
|
|
$response
|
|
->assertStatus(403)
|
|
->assertEnvelope()
|
|
->assertSuccess(false);
|
|
});
|
|
|
|
it('POST /products/{id}/merge succeeds with edit_product permission', function (): void {
|
|
$source = api_fixtures()->createProduct(['name' => 'API Merge Source']);
|
|
$target = api_fixtures()->createProduct(['name' => 'API Merge Target']);
|
|
|
|
$session = api_fixtures()->createUserSession(['edit_product'], ['group_id' => 1]);
|
|
|
|
$response = api_client()->post(
|
|
'/products/' . (int)$source['id'] . '/merge',
|
|
['target_id' => (int)$target['id'], 'reason' => 'SENERE 2 — TRU-94'],
|
|
$session['headers']
|
|
);
|
|
|
|
$response
|
|
->assertStatus(200)
|
|
->assertEnvelope()
|
|
->assertSuccess();
|
|
|
|
expect($response->data())
|
|
->toBeArray()
|
|
->toHaveKey('source_product_id', (int)$source['id'])
|
|
->toHaveKey('target_product_id', (int)$target['id'])
|
|
->toHaveKey('merged_into_product_id', (int)$target['id']);
|
|
});
|
|
|
|
it('POST /products/{id}/merge rejects self-merge', function (): void {
|
|
$product = api_fixtures()->createProduct(['name' => 'Self Merge']);
|
|
$session = api_fixtures()->createUserSession(['edit_product'], ['group_id' => 1]);
|
|
|
|
$response = api_client()->post(
|
|
'/products/' . (int)$product['id'] . '/merge',
|
|
['target_id' => (int)$product['id']],
|
|
$session['headers']
|
|
);
|
|
|
|
$response
|
|
->assertStatus(400)
|
|
->assertEnvelope()
|
|
->assertSuccess(false);
|
|
});
|
|
|
|
it('POST /products/{id}/merge rejects double-merge', function (): void {
|
|
$a = api_fixtures()->createProduct(['name' => 'A']);
|
|
$b = api_fixtures()->createProduct(['name' => 'B']);
|
|
$c = api_fixtures()->createProduct(['name' => 'C']);
|
|
$session = api_fixtures()->createUserSession(['edit_product'], ['group_id' => 1]);
|
|
|
|
// First merge succeeds
|
|
$first = api_client()->post(
|
|
'/products/' . (int)$a['id'] . '/merge',
|
|
['target_id' => (int)$b['id']],
|
|
$session['headers']
|
|
);
|
|
$first->assertStatus(200)->assertEnvelope()->assertSuccess();
|
|
|
|
// Second merge of A into C should fail because A is already merged
|
|
$second = api_client()->post(
|
|
'/products/' . (int)$a['id'] . '/merge',
|
|
['target_id' => (int)$c['id']],
|
|
$session['headers']
|
|
);
|
|
$second
|
|
->assertStatus(400)
|
|
->assertEnvelope()
|
|
->assertSuccess(false);
|
|
});
|