fix(api): only require notes when the product actually requires them on POST /order/items (#360)
## Bug PR #345 (order_item_reason_policy wiring) accidentally broadened the legacy \`Notes is required for this product\` check to fire for every product whose POST body carried an empty/whitespace \`notes\` field. Mobile POS step 2 always posts the primary product (e.g. Sættevognstræk, product id 3) with \`notes: ''\` as part of \`syncCurrentTransactionToOrder\`. After #345, the API started returning 400 for that primary item. The frontend silently swallowed the 400 in the next-step click handler, and the operator saw **"Fuldfør doesn't continue"** with no feedback. ## Repro 1. Log in to the mobile POS (e.g. dept 12 / Taulov) 2. Scan / type a customer's plates (e.g. EP68666 + GG1876) 3. Long-press Sættevognstræk to add the service 4. Tap **Fuldfør** Before this fix: \`POST /order/items\` → 400 \`Notes is required for this product\`. Frontend catches and logs \`Next-step action was interrupted: AxiosError: Request failed with status code 400\`. Operator sees no error in the UI. After this fix: \`POST /order/items\` → 200 for the primary product; the order completes normally. ## Fix Scope the empty-notes rejection to products whose \`requires_note\` flag (or extraordinary-chemistry special case) is set, matching the existing PUT handler behaviour. Products that don't require notes can post \`notes=''\` without rejection. ## Lock-in tests Two Pest tests under \`Tests\\Api\\OrderItemsApiTest\`: - \`allows empty notes for primary products that do not require a note\` — \`requires_note=0\` product with \`notes=''\` returns 200 - \`still rejects empty notes for products whose requires_note flag is enabled\` — \`requires_note=1\` product with \`notes=' '\` returns 400 with the legacy message ## Verification PHP API suite: **292/292 passing** (11892 assertions). Local \`scripts/php-ci-test.sh api\`. ## Companion PR \`copenhagentruckwash/pleno-vue\` → \`fix/fuldfor-surface-order-item-error\` will surface order-item API errors in the UI so silent failures become visible. That PR is a follow-up; this one is the actual root cause fix. Co-authored-by: Truck Wash Agent <agent@copenhagentruckwash.local>
This commit is contained in:
co-authored by
Truck Wash Agent
parent
7174e3be6c
commit
43df3e4dca
@@ -104,18 +104,26 @@ class orderItemsRoute
|
|||||||
}
|
}
|
||||||
// Validation order for audited products:
|
// Validation order for audited products:
|
||||||
// 1. If reason_code is present, run reason validation first (most specific messages).
|
// 1. If reason_code is present, run reason validation first (most specific messages).
|
||||||
// 2. If notes is provided but empty/whitespace, return "Notes is required" (skip reason).
|
// 2. If the product requires an order-item note and notes are provided but
|
||||||
// 3. Otherwise run reason validation (covers missing reason_code and invalid combos).
|
// empty/whitespace, return "Notes is required" (the legacy message). Other products
|
||||||
|
// may carry an empty notes field without rejecting the request.
|
||||||
|
// 3. Otherwise run reason validation (covers missing reason_code on affected products).
|
||||||
$reasonFields = ['reason_code' => null, 'reason_label_snapshot' => null, 'reason_comment' => null];
|
$reasonFields = ['reason_code' => null, 'reason_label_snapshot' => null, 'reason_comment' => null];
|
||||||
$reasonCodeProvided = array_key_exists('reason_code', (array)$data) || array_key_exists('order_item_reason_code', (array)$data);
|
$reasonCodeProvided = array_key_exists('reason_code', (array)$data) || array_key_exists('order_item_reason_code', (array)$data);
|
||||||
|
|
||||||
|
$productRequiresOrderItemNote = $product->requiresOrderItemNote();
|
||||||
|
|
||||||
if ($reasonCodeProvided) {
|
if ($reasonCodeProvided) {
|
||||||
try {
|
try {
|
||||||
$reasonFields = \classes\order_item_reason_policy::validateForProduct((int)$data['product_id'], $data);
|
$reasonFields = \classes\order_item_reason_policy::validateForProduct((int)$data['product_id'], $data);
|
||||||
} catch (\InvalidArgumentException $e) {
|
} catch (\InvalidArgumentException $e) {
|
||||||
$response->error($e->getMessage(), 400);
|
$response->error($e->getMessage(), 400);
|
||||||
}
|
}
|
||||||
} elseif (array_key_exists('notes', (array)$data) && trim((string)($data['notes'] ?? '')) === '') {
|
} elseif (
|
||||||
|
$productRequiresOrderItemNote
|
||||||
|
&& array_key_exists('notes', (array)$data)
|
||||||
|
&& trim((string)($data['notes'] ?? '')) === ''
|
||||||
|
) {
|
||||||
$response->error('Notes is required for this product', 400);
|
$response->error('Notes is required for this product', 400);
|
||||||
} else {
|
} else {
|
||||||
try {
|
try {
|
||||||
|
|||||||
@@ -419,6 +419,79 @@ it('does not allow clearing notes for order items whose product requires notes',
|
|||||||
->assertMessage('Notes is required for this product');
|
->assertMessage('Notes is required for this product');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('allows empty notes for primary products that do not require a note', function (): void {
|
||||||
|
api_test_covers('POST /order/items', 'validation');
|
||||||
|
|
||||||
|
$customer = api_fixtures()->createUser(['display_name' => 'Empty Notes Customer']);
|
||||||
|
$department = api_fixtures()->createDepartment();
|
||||||
|
$cashier = api_fixtures()->createUser(['display_name' => 'Empty Notes Cashier']);
|
||||||
|
$order = api_fixtures()->createOrder([
|
||||||
|
'customer_id' => $customer['customer_number'],
|
||||||
|
'department_id' => $department['id'],
|
||||||
|
'cashier_id' => $cashier['id'],
|
||||||
|
'reference' => 'EMPTY-NOTES',
|
||||||
|
]);
|
||||||
|
// Primary product with requires_note = 0 and not the extraordinary chemistry product.
|
||||||
|
$product = api_fixtures()->createProduct([
|
||||||
|
'id' => 1701,
|
||||||
|
'name' => 'Standard Wash',
|
||||||
|
'price' => 929,
|
||||||
|
'requires_note' => 0,
|
||||||
|
]);
|
||||||
|
$session = api_fixtures()->createUserSession(['add_order_items', 'list_order_items', 'department_access_' . $department['id']]);
|
||||||
|
|
||||||
|
$response = api_client()->post('/order/items', [
|
||||||
|
'order_id' => $order['id'],
|
||||||
|
'product_id' => $product['id'],
|
||||||
|
'quantity' => 1,
|
||||||
|
'price' => 929,
|
||||||
|
'notes' => '',
|
||||||
|
], $session['headers']);
|
||||||
|
|
||||||
|
$response
|
||||||
|
->assertStatus(200)
|
||||||
|
->assertEnvelope()
|
||||||
|
->assertSuccess();
|
||||||
|
|
||||||
|
expect($response->data()['product_id'] ?? null)->toBe(1701);
|
||||||
|
expect($response->data()['notes'] ?? null)->toBe('');
|
||||||
|
expect($response->data()['reason_code'] ?? 'not_set')->toBe('not_set');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('still rejects empty notes for products whose requires_note flag is enabled', function (): void {
|
||||||
|
api_test_covers('POST /order/items', 'validation');
|
||||||
|
|
||||||
|
$customer = api_fixtures()->createUser(['display_name' => 'Notes Required Customer']);
|
||||||
|
$department = api_fixtures()->createDepartment();
|
||||||
|
$cashier = api_fixtures()->createUser(['display_name' => 'Notes Required Cashier']);
|
||||||
|
$order = api_fixtures()->createOrder([
|
||||||
|
'customer_id' => $customer['customer_number'],
|
||||||
|
'department_id' => $department['id'],
|
||||||
|
'cashier_id' => $cashier['id'],
|
||||||
|
'reference' => 'NOTES-REQUIRED',
|
||||||
|
]);
|
||||||
|
$product = api_fixtures()->createProduct([
|
||||||
|
'id' => 1702,
|
||||||
|
'name' => 'Requires-Note Wash',
|
||||||
|
'price' => 199,
|
||||||
|
'requires_note' => 1,
|
||||||
|
]);
|
||||||
|
$session = api_fixtures()->createUserSession(['add_order_items', 'list_order_items', 'department_access_' . $department['id']]);
|
||||||
|
|
||||||
|
api_client()
|
||||||
|
->post('/order/items', [
|
||||||
|
'order_id' => $order['id'],
|
||||||
|
'product_id' => $product['id'],
|
||||||
|
'quantity' => 1,
|
||||||
|
'price' => 199,
|
||||||
|
'notes' => ' ',
|
||||||
|
], $session['headers'])
|
||||||
|
->assertStatus(400)
|
||||||
|
->assertEnvelope()
|
||||||
|
->assertSuccess(false)
|
||||||
|
->assertMessage('Notes is required for this product');
|
||||||
|
});
|
||||||
|
|
||||||
it('returns the extraordinary chemistry product with requires_note enabled', function (): void {
|
it('returns the extraordinary chemistry product with requires_note enabled', function (): void {
|
||||||
api_test_covers('GET /products', 'happy');
|
api_test_covers('GET /products', 'happy');
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user