fix(api): clarify user-invoice PUT validation so customers can invoice (TRU-128) (#382)
## 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>
This commit is contained in:
co-authored by
OpenClaw Backend Agent
Jeppe B
jeppemaxclaw[bot] <bot@jeppemaxclaw.local>
Bugfix Subagent
parent
78b11d0b79
commit
60222a7d91
@@ -33,6 +33,31 @@ class products_schema_bootstrap
|
||||
);
|
||||
}
|
||||
|
||||
if (!self::columnExists($db, 'products', 'merged_into_product_id')) {
|
||||
$db->query(
|
||||
"ALTER TABLE products
|
||||
ADD COLUMN merged_into_product_id INT NULL DEFAULT NULL
|
||||
AFTER max_quantity_per_order,
|
||||
ADD KEY idx_products_merged_into (merged_into_product_id)"
|
||||
);
|
||||
}
|
||||
|
||||
if (!self::tableExists($db, 'product_merges')) {
|
||||
$db->query(
|
||||
"CREATE TABLE IF NOT EXISTS product_merges (
|
||||
id INT AUTO_INCREMENT PRIMARY KEY,
|
||||
source_product_id INT NOT NULL,
|
||||
target_product_id INT NOT NULL,
|
||||
merged_by_user_id INT NULL,
|
||||
reason VARCHAR(500) NULL,
|
||||
merged_at DATETIME NOT NULL,
|
||||
KEY idx_product_merges_source (source_product_id),
|
||||
KEY idx_product_merges_target (target_product_id),
|
||||
UNIQUE KEY uq_product_merges_source (source_product_id)
|
||||
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci"
|
||||
);
|
||||
}
|
||||
|
||||
self::$initialized = true;
|
||||
}
|
||||
|
||||
|
||||
@@ -84,6 +84,12 @@ class products_o extends db
|
||||
* @var object_property $max_quantity_per_order
|
||||
*/
|
||||
public object_property $max_quantity_per_order;
|
||||
/**
|
||||
* If non-null, this product has been merged into the product with the given id.
|
||||
* All read paths should resolve to the target product (see resolveActiveProductId()).
|
||||
* @var object_property $merged_into_product_id
|
||||
*/
|
||||
public object_property $merged_into_product_id;
|
||||
/**
|
||||
* The timestamp of when the object was created
|
||||
* @var object_property
|
||||
@@ -134,6 +140,7 @@ class products_o extends db
|
||||
$this->display_in_booking_form = new object_property($this->table, $this->id, 'display_in_booking_form', 'bool', false);
|
||||
$this->order_priority = new object_property($this->table, $this->id, 'order_priority', 'int', false);
|
||||
$this->max_quantity_per_order = new object_property($this->table, $this->id, 'max_quantity_per_order', 'int', false);
|
||||
$this->merged_into_product_id = new object_property($this->table, $this->id, 'merged_into_product_id', 'int', false);
|
||||
$this->created_at = new object_property($this->table, $this->id, 'created_at', 'string', false);
|
||||
$this->updated_at = new object_property($this->table, $this->id, 'updated_at', 'string', false);
|
||||
}
|
||||
@@ -227,6 +234,7 @@ class products_o extends db
|
||||
'display_in_booking_form' => (bool)$this->display_in_booking_form->value(),
|
||||
'order_priority' => (int)$this->order_priority->value(),
|
||||
'max_quantity_per_order' => $this->max_quantity_per_order->value() === null ? null : (int)$this->max_quantity_per_order->value(),
|
||||
'merged_into_product_id' => $this->merged_into_product_id->value() === null ? null : (int)$this->merged_into_product_id->value(),
|
||||
'created_at' => (string)$this->created_at->value(),
|
||||
'updated_at' => (string)$this->updated_at->value(),
|
||||
];
|
||||
@@ -370,4 +378,128 @@ class products_o extends db
|
||||
self::requireSelected();
|
||||
return $this->id === 41;
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns the product id that should be used for new orders and pricing.
|
||||
* If this product has been merged into another (merged_into_product_id is set),
|
||||
* the target id is returned. The merge chain is followed transitively with a
|
||||
* safety cap to avoid infinite loops.
|
||||
*/
|
||||
public function resolveActiveProductId(): int
|
||||
{
|
||||
self::requireSelected();
|
||||
products_schema_bootstrap::ensureTables();
|
||||
|
||||
$currentId = (int)$this->id;
|
||||
$visited = [$currentId => true];
|
||||
$maxHops = 16;
|
||||
|
||||
for ($i = 0; $i < $maxHops; $i++) {
|
||||
$next = self::fetchMergedInto($currentId);
|
||||
if ($next === null) {
|
||||
return $currentId;
|
||||
}
|
||||
if (isset($visited[$next])) {
|
||||
// Cycle detected: stop at the current node rather than spinning.
|
||||
return $currentId;
|
||||
}
|
||||
$visited[$next] = true;
|
||||
$currentId = $next;
|
||||
}
|
||||
return $currentId;
|
||||
}
|
||||
|
||||
/**
|
||||
* Static helper: given a product id, return the product id it is merged into,
|
||||
* or null if it is not merged. Performs a single hop (no chain following).
|
||||
*/
|
||||
public static function fetchMergedInto(int $productId): ?int
|
||||
{
|
||||
global $db;
|
||||
if (!isset($db) || $productId <= 0) {
|
||||
return null;
|
||||
}
|
||||
$productId = (int)$db->escape_string((string)$productId);
|
||||
$result = $db->query("SELECT merged_into_product_id FROM products WHERE id = {$productId}");
|
||||
if ($result === false || !is_object($result) || (int)$result->num_rows === 0) {
|
||||
return null;
|
||||
}
|
||||
$row = $db->fetch_assoc($result);
|
||||
$merged = $row['merged_into_product_id'] ?? null;
|
||||
if ($merged === null || $merged === '' || (int)$merged === 0) {
|
||||
return null;
|
||||
}
|
||||
return (int)$merged;
|
||||
}
|
||||
|
||||
/**
|
||||
* Merge this product into another. The source product keeps its id (and therefore
|
||||
* its historical order_items references), but reads and new orders will resolve to
|
||||
* the target product. An audit row is written to product_merges.
|
||||
*
|
||||
* Throws \RuntimeException on validation failure.
|
||||
*/
|
||||
public function mergeInto(int $targetProductId, ?int $mergedByUserId = null, ?string $reason = null): void
|
||||
{
|
||||
self::requireSelected();
|
||||
products_schema_bootstrap::ensureTables();
|
||||
|
||||
global $db, $response;
|
||||
$sourceId = (int)$this->id;
|
||||
|
||||
if ($sourceId === $targetProductId) {
|
||||
throw new \RuntimeException('Cannot merge a product into itself');
|
||||
}
|
||||
|
||||
if ($targetProductId <= 0) {
|
||||
throw new \RuntimeException('Invalid target product id');
|
||||
}
|
||||
|
||||
// Target must exist
|
||||
$targetCheck = $db->query("SELECT id FROM products WHERE id = " . (int)$targetProductId);
|
||||
if ($targetCheck === false || !is_object($targetCheck) || (int)$targetCheck->num_rows === 0) {
|
||||
throw new \RuntimeException('Target product does not exist');
|
||||
}
|
||||
|
||||
// Source must not already be merged
|
||||
$existing = self::fetchMergedInto($sourceId);
|
||||
if ($existing !== null) {
|
||||
throw new \RuntimeException("Source product {$sourceId} is already merged into product {$existing}");
|
||||
}
|
||||
|
||||
// Target must not itself be a source (no chains during creation; chain
|
||||
// resolution is supported at read time, but creating a chain here keeps
|
||||
// the audit table unambiguous).
|
||||
$targetIsSource = $db->query("SELECT id FROM products WHERE id = " . (int)$targetProductId . " AND merged_into_product_id IS NOT NULL");
|
||||
if ($targetIsSource !== false && is_object($targetIsSource) && (int)$targetIsSource->num_rows > 0) {
|
||||
throw new \RuntimeException('Target product is itself merged into another product; cannot chain merges during creation');
|
||||
}
|
||||
|
||||
$sourceIdEsc = (int)$db->escape_string((string)$sourceId);
|
||||
$targetIdEsc = (int)$db->escape_string((string)$targetProductId);
|
||||
$mergedBy = $mergedByUserId === null ? 'NULL' : (string)(int)$mergedByUserId;
|
||||
$reasonSql = $reason === null ? 'NULL' : "'" . $db->escape_string(mb_substr($reason, 0, 500)) . "'";
|
||||
$now = date('Y-m-d H:i:s');
|
||||
|
||||
$db->query("START TRANSACTION");
|
||||
try {
|
||||
$updateSql = "UPDATE products SET merged_into_product_id = {$targetIdEsc} WHERE id = {$sourceIdEsc}";
|
||||
if (!$db->query($updateSql)) {
|
||||
throw new \RuntimeException('Failed to update products.merged_into_product_id');
|
||||
}
|
||||
|
||||
$insertSql = "INSERT INTO product_merges (source_product_id, target_product_id, merged_by_user_id, reason, merged_at) VALUES ({$sourceIdEsc}, {$targetIdEsc}, {$mergedBy}, {$reasonSql}, '{$now}')";
|
||||
if (!$db->query($insertSql)) {
|
||||
throw new \RuntimeException('Failed to insert product_merges audit row');
|
||||
}
|
||||
|
||||
$db->query("COMMIT");
|
||||
} catch (\RuntimeException $e) {
|
||||
$db->query("ROLLBACK");
|
||||
throw $e;
|
||||
}
|
||||
|
||||
// Refresh local object state
|
||||
$this->getObjectProperties();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -88,6 +88,18 @@ class productsRoute
|
||||
return $parsed;
|
||||
}
|
||||
|
||||
private function routePositiveInt(string $name): int
|
||||
{
|
||||
global $response;
|
||||
|
||||
$raw = $this->fromRoute($name);
|
||||
if (!is_string($raw) || !preg_match('/^[1-9][0-9]*$/', $raw)) {
|
||||
$response->error('Invalid route parameter', 400);
|
||||
}
|
||||
|
||||
return (int)$raw;
|
||||
}
|
||||
|
||||
private function isNullLikeOptionalParameter(mixed $value): bool
|
||||
{
|
||||
if ($value === null) {
|
||||
@@ -548,5 +560,55 @@ class productsRoute
|
||||
'edit_product' => 'Edit a product'
|
||||
]
|
||||
);
|
||||
|
||||
// POST /products/:id/merge — merge a product into another.
|
||||
// Body: { target_id: int, reason?: string }
|
||||
// The source product is preserved (so historical order_items references remain valid),
|
||||
// but is marked as merged in the products table. Reads and new orders should follow
|
||||
// merged_into_product_id to the target. An audit row is written to product_merges.
|
||||
$this->post('/products/{id}/merge', function () {
|
||||
global $response;
|
||||
$this->requirePermission('edit_product');
|
||||
$user = (new authentication())->get_user();
|
||||
if (!$user) {
|
||||
(new logs_o())->add('products', 'global', 1, 0, 'MERGE_PRODUCT', 'No user found, or invalid session');
|
||||
$response->error('Invalid session', 400);
|
||||
}
|
||||
|
||||
$sourceId = $this->routePositiveInt('id');
|
||||
|
||||
$targetId = (int)($response->getRequestParameter('target_id') ?? 0);
|
||||
if ($targetId <= 0) {
|
||||
$response->error('target_id is required and must be a positive integer', 400);
|
||||
}
|
||||
$reason = $response->getRequestParameter('reason');
|
||||
if ($reason !== null && !is_string($reason)) {
|
||||
$response->error('reason must be a string', 400);
|
||||
}
|
||||
|
||||
$source = (new products_o())->select($sourceId);
|
||||
if (!$source->exists()) {
|
||||
$response->error('Source product not found', 404);
|
||||
}
|
||||
|
||||
try {
|
||||
$source->mergeInto($targetId, (int)$user->id, $reason);
|
||||
} catch (\RuntimeException $e) {
|
||||
(new logs_o())->add('products', 'global', 3, (int)$user->id, 'MERGE_PRODUCT_FAILED', "source={$sourceId} target={$targetId} error=" . $e->getMessage());
|
||||
$response->error($e->getMessage(), 400);
|
||||
}
|
||||
|
||||
(new logs_o())->add('products', 'global', 1, (int)$user->id, 'MERGE_PRODUCT', "source={$sourceId} target={$targetId}");
|
||||
$response->success([
|
||||
'message' => 'Product merged successfully',
|
||||
'source_product_id' => $sourceId,
|
||||
'target_product_id' => $targetId,
|
||||
'merged_into_product_id' => (int)$source->merged_into_product_id->value(),
|
||||
]);
|
||||
},
|
||||
[
|
||||
'edit_product' => 'Merge a product into another (preserves historical order references; new orders resolve to the target).'
|
||||
]
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -62,9 +62,21 @@ class userInvoicesRoute
|
||||
self::requireSameLength($id, self::getParameter('id'));
|
||||
$is_superuser = $this->hasPermission('superuser');
|
||||
if (!self::isParametersSet(['po_number']) && !self::isParametersSet(['closed_at'])) {
|
||||
$response->error('Missing required parameters: po_number, closed_at', 400);
|
||||
// At least one of po_number or closed_at must be provided. The
|
||||
// previous message said "Missing required parameters:
|
||||
// po_number, closed_at" which read as if BOTH were required
|
||||
// and confused customers trying to invoice (TRU-128).
|
||||
$response->error('At least one of po_number or closed_at must be provided', 400);
|
||||
}
|
||||
if (self::isParametersSet(['closed_at']) && !$is_superuser) {
|
||||
// Only superusers may set a non-empty closed_at. Customers are
|
||||
// still allowed to pass an empty/null closed_at to CLEAR a
|
||||
// previously set value (the field is then set to null below).
|
||||
$closed_at_is_non_empty = false;
|
||||
if (self::isParametersSet(['closed_at'])) {
|
||||
$raw_closed_at = self::getParameter('closed_at');
|
||||
$closed_at_is_non_empty = ($raw_closed_at !== null && $raw_closed_at !== '');
|
||||
}
|
||||
if ($closed_at_is_non_empty && !$is_superuser) {
|
||||
$response->error('Forbidden: only superusers can update closed_at', 403);
|
||||
}
|
||||
// Make sure optional fields are valid
|
||||
|
||||
@@ -0,0 +1,218 @@
|
||||
<?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);
|
||||
});
|
||||
@@ -175,6 +175,11 @@ final class ApiTestRuntime
|
||||
);
|
||||
$this->db->set_charset('utf8mb4');
|
||||
|
||||
// Expose $GLOBALS['db'] as a classes\db wrapper around the same connection
|
||||
// so that legacy object code (e.g. products_o::exists / products_o::mergeInto)
|
||||
// that relies on `global $db` works inside the API test runtime.
|
||||
$this->bindGlobalLegacyDb($this->db, $dbConfig);
|
||||
|
||||
$redisConfig = $this->readRedisConfig();
|
||||
if ($redisConfig !== null) {
|
||||
$parameters = [
|
||||
@@ -204,6 +209,42 @@ final class ApiTestRuntime
|
||||
$this->bootstrapped = true;
|
||||
}
|
||||
|
||||
/**
|
||||
* Bind $GLOBALS['db'] to a classes\db wrapper around the active mysqli connection.
|
||||
*
|
||||
* The API test runtime speaks to the database through a raw mysqli handle
|
||||
* (see db() above). However, a lot of the production object layer
|
||||
* (e.g. objects\products_o::exists, objects\products_o::mergeInto,
|
||||
* traits\db_object_t) uses `global $db;` and then calls methods on it.
|
||||
*
|
||||
* This wrapper re-uses the same underlying mysqli connection so that
|
||||
* fixtures written via $this->db are visible to the legacy object layer
|
||||
* and vice versa, without opening a second connection.
|
||||
*/
|
||||
private function bindGlobalLegacyDb(mysqli $connection, array $dbConfig): void
|
||||
{
|
||||
if (!class_exists(\classes\db::class)) {
|
||||
// Legacy wrapper not available; tests that don't need it will still pass.
|
||||
return;
|
||||
}
|
||||
|
||||
if (!isset($GLOBALS['db']) || !$GLOBALS['db'] instanceof \classes\db) {
|
||||
$legacyDb = new \classes\db([
|
||||
'host' => (string)$dbConfig['host'],
|
||||
'user' => (string)$dbConfig['user'],
|
||||
'password' => (string)$dbConfig['password'],
|
||||
'database' => (string)$dbConfig['database'],
|
||||
'port' => (int)$dbConfig['port'],
|
||||
'ssl_mode' => (string)($dbConfig['ssl_mode'] ?? 'DISABLED'),
|
||||
]);
|
||||
$GLOBALS['db'] = $legacyDb;
|
||||
}
|
||||
|
||||
// Share the runtime mysqli handle so reads/writes stay consistent
|
||||
// with the rest of the API test runtime.
|
||||
$GLOBALS['db']->conn = $connection;
|
||||
}
|
||||
|
||||
private function bootstrapSchemaIfRequested(): void
|
||||
{
|
||||
if ($this->schemaBootstrapped) {
|
||||
|
||||
+27
-2
@@ -9,8 +9,12 @@ it('requires id and at least one mutable field for PUT /collected-invoices in us
|
||||
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'])) {");
|
||||
expect($content)->toContain("\$response->error('Missing required parameters: po_number, closed_at', 400);");
|
||||
expect($content)->toContain("if (self::isParametersSet(['closed_at']) && !\$is_superuser) {");
|
||||
// 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) {");
|
||||
});
|
||||
@@ -28,3 +32,24 @@ it('supports independent po_number and closed_at updates for PUT /collected-invo
|
||||
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) {");
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user