diff --git a/grails-app/conf/spring/resources.groovy b/grails-app/conf/spring/resources.groovy index be5178f57bf..087fe5196a7 100644 --- a/grails-app/conf/spring/resources.groovy +++ b/grails-app/conf/spring/resources.groovy @@ -1,5 +1,6 @@ package spring +import org.pih.warehouse.custom.outboundExpiryRestrictions.service.StockMovementServiceWithExpiryFilter import org.pih.warehouse.product.ProductValidator import org.springframework.boot.web.servlet.FilterRegistrationBean import org.springframework.core.Ordered @@ -19,4 +20,10 @@ beans = { order = Ordered.HIGHEST_PRECEDENCE + 1 } productValidator(ProductValidator) + + // outboundExpiryRestrictions: replace stockMovementService with a subclass that + // filters expired AvailableItems from autopick suggestions. + stockMovementService(StockMovementServiceWithExpiryFilter) { bean -> + bean.autowire = 'byName' + } } diff --git a/grails-app/controllers/org/pih/warehouse/custom/outboundExpiryRestrictions/OutboundExpiryGuardInterceptor.groovy b/grails-app/controllers/org/pih/warehouse/custom/outboundExpiryRestrictions/OutboundExpiryGuardInterceptor.groovy new file mode 100644 index 00000000000..de044195bc5 --- /dev/null +++ b/grails-app/controllers/org/pih/warehouse/custom/outboundExpiryRestrictions/OutboundExpiryGuardInterceptor.groovy @@ -0,0 +1,111 @@ +package org.pih.warehouse.custom.outboundExpiryRestrictions + +import grails.converters.JSON +import org.pih.warehouse.api.StockMovementType +import org.pih.warehouse.custom.outboundExpiryRestrictions.support.ExpiryRule +import org.pih.warehouse.inventory.InventoryItem +import org.pih.warehouse.inventory.OutboundStockMovement +import org.pih.warehouse.requisition.RequisitionItem + +import java.math.BigDecimal +import java.text.DateFormat + +class OutboundExpiryGuardInterceptor { + + static final String ERROR_CODE = 'outboundExpiryRestrictions.expired.cannotShip' + + // Reason: must run AFTER SecurityInterceptor (default order 0) so unauthenticated + // requests are short-circuited before this guard touches the DB or leaks ID existence. + // Matches the pattern used by RoleInterceptor and SentryInterceptor. + int order = LOWEST_PRECEDENCE + + def messageSource + + OutboundExpiryGuardInterceptor() { + match(controller: 'stockMovementItemApi', action: 'updatePicklist') + } + + boolean before() { + def jsonObject = request.JSON + List picklistItems = jsonObject?.picklistItems + if (!picklistItems) { + return true + } + + RequisitionItem requisitionItem = RequisitionItem.get(params.id) + if (!requisitionItem) { + return true + } + + // STOCK_MOVEMENT → enforce; RETURN_ORDER → allow; null → enforce (fail closed). + OutboundStockMovement parentMovement = OutboundStockMovement.findByRequisition(requisitionItem.requisition) + if (parentMovement && parentMovement.stockMovementType != StockMovementType.STOCK_MOVEMENT) { + return true + } + + List ids = picklistItems + .findAll { it?.inventoryItem?.id && isPositiveQuantity(it.quantityPicked) } + .collect { it.inventoryItem.id as String } + if (!ids) { + return true + } + + // One projection query instead of N InventoryItem.read() round-trips plus N lazy-product fetches. + List rows = InventoryItem.executeQuery( + 'select ii.lotNumber, ii.expirationDate, p.productCode ' + + 'from InventoryItem ii left join ii.product p where ii.id in :ids', + [ids: ids] + ) + + Date today = new Date().clearTime() + List expired = rows.findAll { ExpiryRule.isExpired((Date) it[1], today) } + + if (!expired) { + return true + } + + DateFormat dateFormat = DateFormat.getDateInstance(DateFormat.SHORT, request.locale) + List errorMessages = expired.collect { formatMessage(it, dateFormat) } + log.info("outbound_expiry_guard_blocked requisition_item_id=${params.id} expired_count=${expired.size()}") + + response.status = 400 + render([errorCode: ERROR_CODE, errorMessages: errorMessages] as JSON) + return false + } + + // Reason: the formatted user-facing string lives only in the i18n bundle + // (`outboundExpiryRestrictions-messages.properties`) so translators have a single + // source of truth. The defaultMessage is just a non-interpolated safety net for the + // catastrophic case where the bundle is missing — it never renders in normal operation. + private static final String MISSING_BUNDLE_FALLBACK = 'Expired stock cannot be picked.' + + private String formatMessage(row, DateFormat dateFormat) { + String lotNumber = (row[0] ?: '') as String + Date expirationDate = (Date) row[1] + String productCode = (row[2] ?: '') as String + String formattedExpiry = expirationDate ? dateFormat.format(expirationDate) : '' + Object[] args = [productCode, lotNumber, formattedExpiry] as Object[] + return messageSource.getMessage(ERROR_CODE, args, MISSING_BUNDLE_FALLBACK, request.locale) + } + + // Reason: must parse as BigDecimal (not Integer) to match upstream's parser. Integer truncates + // "1.5" to 0, which would let an expired-lot row slip past the guard; upstream then crashes + // with ArithmeticException AFTER clearPicklist() runs, wiping the requisition's existing picks. + private static boolean isPositiveQuantity(quantityPicked) { + if (quantityPicked == null) { + return false + } + if (quantityPicked instanceof Number) { + return ((Number) quantityPicked).doubleValue() > 0 + } + String s = quantityPicked.toString().trim() + if (!s) { + return false + } + try { + return new BigDecimal(s).signum() > 0 + } catch (NumberFormatException ignored) { + return false + } + } +} diff --git a/grails-app/i18n/messages.properties b/grails-app/i18n/messages.properties index 4b8333fd77a..da1b542fd2a 100644 --- a/grails-app/i18n/messages.properties +++ b/grails-app/i18n/messages.properties @@ -4942,6 +4942,11 @@ react.confirmExpirationDate.modal.lot.label=Lot react.confirmExpirationDate.modal.previousExpiry.label=Previous Expiry react.confirmExpirationDate.modal.newExpiry.label=New Expiry +# outboundExpiryRestrictions (custom) +outboundExpiryRestrictions.edit.expiredHint=({0} expired) +outboundExpiryRestrictions.expired.cannotShip=Cannot pick lot {1} of product {0} — it expired on {2}. +outboundExpiryRestrictions.expired.tooltip=Cannot ship — expired on {0}. + # Custom: stock-transfer-document-upload customStockTransferDocument.required.error=A document is required to complete this stock transfer customStockTransferDocument.noDocuments.label=No supporting documents diff --git a/grails-app/i18n/messages_ru.properties b/grails-app/i18n/messages_ru.properties index 50569ed5adc..b3e062a9b82 100644 --- a/grails-app/i18n/messages_ru.properties +++ b/grails-app/i18n/messages_ru.properties @@ -4919,3 +4919,8 @@ customStockTransferDocument.upload.invalidFilename.error=\u0418\u043c\u044f \u04 react.custom.stockTransferDocuments.upload.invalidType.error=\u041d\u0435\u043f\u043e\u0434\u0434\u0435\u0440\u0436\u0438\u0432\u0430\u0435\u043c\u044b\u0439 \u0442\u0438\u043f \u0444\u0430\u0439\u043b\u0430. \u0420\u0430\u0437\u0440\u0435\u0448\u0435\u043d\u044b: PDF, \u0438\u0437\u043e\u0431\u0440\u0430\u0436\u0435\u043d\u0438\u0435, Word, Excel, CSV, ZIP. react.custom.stockTransferDocuments.upload.tooLarge.error=\u0424\u0430\u0439\u043b \u0441\u043b\u0438\u0448\u043a\u043e\u043c \u0431\u043e\u043b\u044c\u0448\u043e\u0439. react.custom.stockTransferDocuments.upload.invalidFilename.error=\u0418\u043c\u044f \u0444\u0430\u0439\u043b\u0430 \u043e\u0442\u0441\u0443\u0442\u0441\u0442\u0432\u0443\u0435\u0442 \u0438\u043b\u0438 \u043d\u0435\u043a\u043e\u0440\u0440\u0435\u043a\u0442\u043d\u043e. + +# outboundExpiryRestrictions (custom) +outboundExpiryRestrictions.edit.expiredHint=({0} \u043f\u0440\u043e\u0441\u0440\u043e\u0447\u0435\u043d\u043e) +outboundExpiryRestrictions.expired.cannotShip=\u041d\u0435\u0432\u043e\u0437\u043c\u043e\u0436\u043d\u043e \u0432\u044b\u0431\u0440\u0430\u0442\u044c \u043f\u0430\u0440\u0442\u0438\u044e {1} \u0442\u043e\u0432\u0430\u0440\u0430 {0} \u2014 \u0441\u0440\u043e\u043a \u0433\u043e\u0434\u043d\u043e\u0441\u0442\u0438 \u0438\u0441\u0442\u0451\u043a {2}. +outboundExpiryRestrictions.expired.tooltip=\u041d\u0435\u0432\u043e\u0437\u043c\u043e\u0436\u043d\u043e \u043e\u0442\u043f\u0440\u0430\u0432\u0438\u0442\u044c \u2014 \u0441\u0440\u043e\u043a \u0433\u043e\u0434\u043d\u043e\u0441\u0442\u0438 \u0438\u0441\u0442\u0451\u043a {0}. diff --git a/grails-app/i18n/messages_tg.properties b/grails-app/i18n/messages_tg.properties index 1c507f79f40..2a5a310da73 100644 --- a/grails-app/i18n/messages_tg.properties +++ b/grails-app/i18n/messages_tg.properties @@ -4919,3 +4919,8 @@ customStockTransferDocument.upload.invalidFilename.error=\u041d\u043e\u043c\u043 react.custom.stockTransferDocuments.upload.invalidType.error=\u041d\u0430\u0432\u044a\u0438 \u0444\u0430\u0439\u043b\u0438 \u0434\u0430\u0441\u0442\u0433\u0438\u0440\u043d\u0430\u0448\u0430\u0432\u0430\u043d\u0434\u0430. \u0418\u04b7\u043e\u0437\u0430\u0442 \u0434\u043e\u0434\u0430 \u0448\u0443\u0434\u0430\u0430\u0441\u0442: PDF, \u0442\u0430\u0441\u0432\u0438\u0440, Word, Excel, CSV, ZIP. react.custom.stockTransferDocuments.upload.tooLarge.error=\u0424\u0430\u0439\u043b \u0430\u0437 \u04b3\u0430\u0434 \u0437\u0438\u0451\u0434 \u043a\u0430\u043b\u043e\u043d \u0430\u0441\u0442. react.custom.stockTransferDocuments.upload.invalidFilename.error=\u041d\u043e\u043c\u0438 \u0444\u0430\u0439\u043b \u043d\u0435\u0441\u0442 \u0451 \u043d\u043e\u0434\u0443\u0440\u0443\u0441\u0442 \u0430\u0441\u0442. + +# outboundExpiryRestrictions (custom) +outboundExpiryRestrictions.edit.expiredHint=({0} \u043c\u04ef\u04b3\u043b\u0430\u0442\u0430\u0448 \u0433\u0443\u0437\u0430\u0448\u0442\u0430) +outboundExpiryRestrictions.expired.cannotShip=\u041f\u0430\u0440\u0442\u0438\u044f\u0438 {1}-\u0438 \u043c\u0430\u04b3\u0441\u0443\u043b\u043e\u0442\u0438 {0}-\u0440\u043e \u0438\u043d\u0442\u0438\u0445\u043e\u0431 \u043a\u0430\u0440\u0434\u0430\u043d \u043c\u0443\u043c\u043a\u0438\u043d \u043d\u0435\u0441\u0442 \u2014 \u043c\u04ef\u04b3\u043b\u0430\u0442\u0430\u0448 {2} \u0433\u0443\u0437\u0430\u0448\u0442\u0430\u0430\u0441\u0442. +outboundExpiryRestrictions.expired.tooltip=\u0424\u0438\u0440\u0438\u0441\u0442\u043e\u0434\u0430\u043d \u043c\u0443\u043c\u043a\u0438\u043d \u043d\u0435\u0441\u0442 \u2014 \u043c\u04ef\u04b3\u043b\u0430\u0442\u0430\u0448 {0} \u0433\u0443\u0437\u0430\u0448\u0442\u0430\u0430\u0441\u0442. diff --git a/grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy b/grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy index b1694ed0b61..1ea7cb402e9 100644 --- a/grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy +++ b/grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy @@ -47,6 +47,7 @@ import org.pih.warehouse.core.StockMovementItemParamsCommand import org.pih.warehouse.core.StockMovementItemsParamsCommand import org.pih.warehouse.core.User import org.pih.warehouse.core.UserService +import org.pih.warehouse.custom.outboundExpiryRestrictions.support.ExpiryRule import org.pih.warehouse.data.DataService import org.pih.warehouse.forecasting.ForecastingService import org.pih.warehouse.importer.CSVUtils @@ -1031,6 +1032,7 @@ class StockMovementService { .getAllAvailableBinLocations(requisition.origin, productsIds) .groupBy { it?.inventoryItem?.product?.id } + Date today = new Date().clearTime() def editPageItems = data.collect { def substitutionItems = substitutionItemsMap[it.id] @@ -1049,6 +1051,7 @@ class StockMovementService { def quantityAvailable = availableItems?.findAll { it.quantityAvailable > 0 }?.sum { it.quantityAvailable } def quantityOnHand = availableItems?.sum { it.quantityOnHand } + def quantityPickable = ExpiryRule.sumPickableQuantity(availableItems, today) def quantityDemandFulfilling = forecastingService.getDemand(requisition.origin, null, productsMap[it.product_id]) [ @@ -1063,6 +1066,7 @@ class StockMovementService { quantityDemandFulfilling : quantityDemandFulfilling ? quantityDemandFulfilling.monthlyDemand : 0, quantityOnHand : (quantityOnHand && quantityOnHand > 0 ? quantityOnHand : 0), quantityAvailable : (quantityAvailable && quantityAvailable > 0 ? quantityAvailable : 0), + quantityPickable : quantityPickable, quantityCounted : it.quantity_counted, substitutionStatus : it.substitution_status, sortOrder : it.sort_order, diff --git a/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/design.md b/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/design.md new file mode 100644 index 00000000000..03f3b0c9d19 --- /dev/null +++ b/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/design.md @@ -0,0 +1,191 @@ +## Context + +OpenBoxes' outbound stock-movement wizard separates **product selection** from **lot selection** into two distinct steps: + +| Wizard step | File | What the user picks | What the API receives | +|---|---|---|---| +| Step 2 — Add Items | `src/js/components/stock-movement-wizard/outbound/AddItemsPage.jsx` | Products + requested quantity | `{product.id, quantityRequested, recipient, sortOrder}` only — **no `inventoryItem.id`** (verified at lines 540–554) | +| Step 4 — Pick | `src/js/components/stock-movement-wizard/outbound/PickPage.jsx` (table) and `src/js/components/stock-movement-wizard/modals/EditPickModal.jsx` (per-line lot picker) | Specific lots and quantities to pick from each bin | `{picklistItems[].inventoryItem.id, binLocation.id, quantityPicked, reasonCode}` (verified at `EditPickModal.jsx:212-220`) → POST to `/api/stockMovementItems//updatePicklist` (verified at `UrlMappings.groovy:219-221`) → `StockMovementItemApiController.updatePicklist()` (verified at `StockMovementItemApiController.groovy:60-82`) | + +**The "expired product" restriction therefore belongs at the Pick step — that is the only step where the user touches inventory items.** Restricting at Add Items would block products in name only; expired-vs-fresh is a per-lot property, not a per-product property. + +**Outbound returns live in a separate component tree.** Returns set `type: 'RETURN_ORDER'` from `src/js/components/returns/outbound/CreateOutboundReturn.jsx:179` — they do **not** flow through `stock-movement-wizard/outbound/` at all. No branching on `stockMovementType` is needed in the wizard. (We still branch on it server-side as defence-in-depth.) + +**Existing infrastructure we will reuse.** `ProductAvailabilityService.calculateQuantitiesByExpirationFilter` (`grails-app/services/org/pih/warehouse/inventory/ProductAvailabilityService.groovy:540-569`) already encodes a "subtract expired stock" mode through an `ExpirationFilter` enum (`SUBTRACT_EXPIRED_STOCK` / `DO_NOT_SUBTRACT_EXPIRED_STOCK`). The expression at line 555 — `inventoryItem.expirationDate >= today` — is the canonical "is this lot still valid?" check upstream uses today. **We adopt the same predicate** to stay consistent. + +**Defines for this design:** +- "Expired" = `inventoryItem.expirationDate != null && expirationDate < today` (server timezone, midnight). Matches upstream `ProductAvailabilityService.groovy:555`. +- "Outbound STOCK_MOVEMENT" = `OutboundStockMovement.stockMovementType == StockMovementType.STOCK_MOVEMENT` (verified at `OutboundStockMovement.groovy:61` and `StockMovementType.groovy:5`). Returns are `RETURN_ORDER` and do not reach the pick endpoint we intercept. + +## Goals / Non-Goals + +**Goals:** +- Prevent users from picking expired lots in the **Pick step** of the outbound STOCK_MOVEMENT wizard, both in the UI (disabled row in `EditPickModal`) and via the API (server-side guard on `StockMovementItemApiController.updatePicklist`). +- Leave outbound `RETURN_ORDER`, all inbound flows, and every other surface unchanged. +- Add zero columns to upstream tables. Edit at most one upstream frontend file (`EditPickModal.jsx`) with surgical, two-line-equivalent changes. +- Reuse the existing `ExpirationFilter`-style predicate so the "is expired?" rule lives in exactly one canonical helper. + +**Non-Goals:** +- Block already-saved expired allocations on movements that pre-date the rollout (no retroactive cleanup; the guard fires on `updatePicklist` only). +- Block expired items in the **packing/shipping** later steps — those reuse the picks already made; if the Pick step refused them, downstream is clean. +- Cover bulk CSV import paths for picks (out of scope; flagged as a follow-up). +- Touch the Add Items / Inbound / Stock Transfer wizards. Add Items only handles products and there are no per-lot decisions there. +- Hide products on the Add Items step whose only available stock is expired. Possible follow-up; not core to "selecting expired products". +- Add a Settings toggle to choose hide-vs-disable. The disable-with-tooltip behaviour is hardcoded (see D1). + +## Decisions + +### D1 — Disable expired rows in the lot picker (do not hide) + +In `EditPickModal.jsx`, render rows whose lot is expired with disabled styling (`text-disabled` class — already in use at `EditPickModal.jsx:43-45` for `quantityAvailable === 0` rows) and a tooltip showing the expiration date and reason "Cannot ship — expired". + +- **Why:** the modal already shows the lot's `expirationDate` column (line 75-83). Disabling the `quantityPicked` input on expired rows is consistent with the existing "no-stock" disable pattern (line 129-133: `disabled: !quantityAvailable && !quantityPicked`). Hiding rows would conflict with the modal's role of showing the user *all* lots in the bin. +- **Alternative considered:** hide expired rows server-side. Rejected because the modal exists specifically to show full bin contents — hiding rows hides legitimate state. + +### D2 — Custom React file replaces (or extends) EditPickModal; one upstream-line edit + +The cleanest minimum-touch approach is: + +- Create `src/js/custom/outboundExpiryRestrictions/components/ExpiryAwareEditPickModal.jsx` that re-exports the upstream `EditPickModal` with a small wrapper: in the `quantityPicked` field's `getDynamicAttr`, also disable when the row is expired; in `getDynamicRowAttr`, add the `text-disabled` class when expired; pass an i18n tooltip. +- Edit `src/js/components/stock-movement-wizard/outbound/PickPage.jsx:25` (the import) and the JSX site that uses `EditPickModal` to use our wrapper instead. Two-line change. + +The wrapper is feasible because `EditPickModal` is consumed via the field config object (`type: EditPickModal`) — replacing the type substitutes our wrapper. + +- **Trade-off:** if upstream changes `EditPickModal`'s prop signature, our wrapper's pass-through breaks loudly. Acceptable; tests in tasks.md will catch the breakage. +- **Alternative considered:** patch `EditPickModal.jsx` directly. Rejected — that file is upstream and frequently updated. + +### D3 — Server-side guard via custom Grails interceptor on `updatePicklist` + +A custom Grails interceptor `OutboundExpiryGuardInterceptor` matches `controller: 'stockMovementItemApi', action: 'updatePicklist'` (verified at `StockMovementItemApiController.groovy:60` and the URL mapping at `UrlMappings.groovy:219-221`). In `before()`: + +1. Parse `picklistItems` from the JSON body (same shape as `updatePicklist` reads at line 67). +2. Load the `RequisitionItem` via `RequisitionItem.get(params.id)` directly. (We originally drafted `stockMovementService.getStockMovementItem(...)` here, but the implementation uses the lighter direct GORM lookup — the interceptor does not need the full StockMovementItem mapping; it only needs the parent requisition + a single executeQuery projection over the inventoryItem ids.) +3. Walk back to the `OutboundStockMovement` via `stockMovementItem.requisition` and check `stockMovementType == StockMovementType.STOCK_MOVEMENT`. If `RETURN_ORDER` (or no parent), return true (let the request proceed). +4. For each `picklistItems[].inventoryItem.id`, load the `InventoryItem` and apply the canonical predicate (`expirationDate != null && expirationDate < today`). If any expired, render HTTP 400 with `{errorCode: 'outboundExpiryRestrictions.expired.cannotShip', errorMessages: [...]}` and return false. + +- **Why an interceptor (not a service edit):** Grails interceptors live in `grails-app/controllers/...//` — fully custom code, no upstream backend edit. +- **Why this specific endpoint:** verified at `StockMovementItemApiController.groovy:60-82` that this action is the one the wizard's pick step submits to. The line-items endpoint (`StockMovementApiController.updateItems` at line 300) deserializes `inventoryItem.id` (line 448) but the outbound wizard's Add Items step does not populate it — so the picklist endpoint is the actual choke point for lot decisions. +- **Trade-off:** parses the body twice (once in interceptor, once in controller). Pick payloads are small (line items per request typically <50). Acceptable. + +### D4 — "Expired" rule lives in a single Groovy helper + +`ExpiryRule` (a `final` Groovy class with static methods) under `src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/` exposes: + +```groovy +static boolean isExpired(Date expirationDate, Date today) // null-safe; null expirationDate → false +``` + +`today` is injected by the caller (rather than computed inside the predicate) so unit tests can pin it and so loops over many items hoist the date computation once. The interceptor and `StockMovementServiceWithExpiryFilter.filterExpired` both compute `new Date().clearTime()` once and pass it in. + +The interceptor and the (optional) frontend response-annotator both call this. The predicate matches upstream `ProductAvailabilityService.groovy:555` (`expirationDate < today`). + +- **Why:** one rule, one place. Future tweaks (grace period, end-of-day handling, etc.) change exactly one file. + +### D5 — Frontend reads `expirationDate` already in the EditPickModal data + +`EditPickModal.jsx` already has the `expirationDate` field on each `availableItem` row (rendered at line 75-83). No new API plumbing is needed for the UI — we just compute "is expired?" at render time using the row's `expirationDate` and disable the input. + +- **Why:** zero backend change is required for the UI to do its job. The interceptor (D3) handles the server-side enforcement independently. +- **Trade-off:** each row's "isExpired" is computed in the browser. If the server clock and browser clock differ by hours/days, the UI may disable a row the server would accept (or vice versa). Mitigation: the server clock is authoritative — the interceptor decides. The UI is best-effort guidance. + +### D6 — No backend file edit at all + +D1+D2+D3+D4+D5 add up to: +- Backend: one new interceptor file, one new helper file, no edits to any upstream backend file. Zero column additions. +- Frontend: one new wrapper component, two-line edit on `outbound/PickPage.jsx`. No edits anywhere else. + +This is materially less invasive than the original design. + +## Risks / Trade-offs + +| Risk | Mitigation | +|---|---| +| Frontend bundle and backend deploy out of sync — old bundle calls `updatePicklist` without the wrapper's check | Server-side interceptor (D3) catches the bypass; user gets HTTP 400. | +| `EditPickModal`'s prop signature changes upstream | Our wrapper passes props through; if upstream renames a field, our additional `getDynamicAttr` logic still runs. Tests cover the disabled state and the i18n message. | +| Server clock drift vs browser clock — UI thinks a lot expires today, server says yesterday | Server is authoritative; interceptor returns 400. UI disables proactively to minimise surprise. The `<` (strict) comparison gives a one-day grace window in the user's favour. | +| Upstream renames `controller: 'stockMovementItemApi'` | Interceptor's `match` block fails open (no rejection) — that is, the rule stops working silently. **Mitigation:** integration test in tasks.md asserts an expired pick is rejected; if it isn't, the test fails. | +| `stockMovementItem.requisition` traversal is null in some pick edge cases | **Implementation fails closed**, not open: if `OutboundStockMovement.findByRequisition(...)` returns null, the expiry guard still runs. Rationale: a non-resolvable parent on a `stockMovementItemApi.updatePicklist` call is unexpected, and silently allowing an expired payload through on a malformed traversal is a worse failure mode than rejecting it. Returns don't reach this endpoint anyway (verified above), so allowing through has no upside. Covered by spec `still applies the expiry check when the parent traversal returns null` in `OutboundExpiryGuardInterceptorSpec`. | +| Performance — interceptor adds an `InventoryItem.load(...)` per submitted picklist item | Pick payloads have at most ~10 lots in practice. One indexed PK load each. Negligible. | +| Existing customers may already be sending picks of expired stock and their workflow depends on it | Verified there is no existing config flag turning on/off expiry restriction. The change introduces strict behaviour. If any deployment objects, gate the interceptor on `grailsApplication.config.openboxes.custom.outboundExpiryRestrictions.enabled` (default true). Wire is one-line. | +| Information disclosure: 400-response body reflects `lotNumber` + `productCode` of expired lots back to the caller | The interceptor inherits upstream's authz model for `stockMovementItemApi.updatePicklist`: `SecurityInterceptor` requires authentication, `RoleInterceptor` requires Manager (because `actionName` matches `'update*'` in `changeActions`), and **no per-warehouse ownership check is enforced** by either gate or by the upstream controller action (`StockMovementItemApiController.groovy:60-82`). A logged-in Manager can therefore probe arbitrary `inventoryItem.id` values and learn the lot number / product code of any **expired** lot via this endpoint. Consistent with the pre-existing upstream gap on the same endpoint, where the same caller can already *write* picklist items across warehouses. **Decision: leave as-is.** Legitimate "transfer expired stock" workflows use unaffected paths — bin-to-bin transfer (`/api/stockTransfers/...`, separate controller) and outbound returns (`returns/outbound/*`, separate component tree, also `/api/stockTransfers/...`). If a future deployment partitions Managers per-warehouse, tighten by adding `parentMovement.origin.id == session.warehouse.id` before the HQL lookup in `OutboundExpiryGuardInterceptor.before()`. | + +## Migration Plan + +1. Implement behind no flag — the rule is universally desired across customers (per the "strict" framing). If a customer wants it off later, expose `openboxes.custom.outboundExpiryRestrictions.enabled` (boolean, default true) at the top of the interceptor and the React wrapper. +2. Deploy backend + frontend together. If backend lands first, the rule still works (interceptor enforces; UI does not yet disable). If frontend lands first, the UI disables but server allows direct API bypass — risk window is short. +3. **Rollback:** revert the two PR commits. There are no database changes, no row mutations, and no upstream-backend-file rewrites — the rollback is symmetric. +4. **Data note:** existing in-flight outbound stock movements with already-allocated expired picks remain valid. The guard fires only on new `updatePicklist` calls. If retroactive cleanup is needed, that's a separate change. + +## Upstream Touch Points + +This section is the merge-conflict hitlist for future upstream pulls (per `.claude/rules/custom-package-isolation.md` § "When You MUST Touch an Upstream File"). + +| File | Reason | Diff size | +|---|---|---| +| `src/js/components/stock-movement-wizard/outbound/PickPage.jsx` | Replace the `EditPickModal` import + JSX usage with custom `ExpiryAwareEditPickModal` wrapper so the lot picker disables expired rows. | Two lines (one import, one component reference). No other lines change. | +| `grails-app/i18n/messages.properties` | Append the three `outboundExpiryRestrictions.*` keys at the bottom of the upstream root bundle. Required because Grails 3.3's `PluginAwareResourceBundleMessageSource` only globs `grails-app/i18n/messages*.properties` at the root — a custom basename under `grails-app/i18n/custom/` would never be loaded at runtime, so all three keys would silently fall back to the hardcoded English defaults. Crowdin already syncs from this file. | 4 added lines (1 comment + 3 keys). | +| `webpack.config.js` | Add a `custom: path.resolve(SRC, 'custom')` alias so `import ... from 'custom//...'` resolves at bundle time. Without this, bare `custom/...` imports fail because Webpack's default `resolve.modules` is `['node_modules']` only. ESLint's import resolver already finds these paths via `import/resolver.node.paths: ['src/js']`. | One added line in the alias block. | +| `grails-app/conf/spring/resources.groovy` | Override the upstream `stockMovementService` bean with `StockMovementServiceWithExpiryFilter` (subclass of `StockMovementService`). Required to filter expired `AvailableItem`s out of `getSuggestedItems` autopick suggestions — Spring proxy AOP cannot intercept the upstream `this.getSuggestedItems(...)` self-call from `createPicklist`, so we substitute the bean instead and rely on Java/Groovy virtual dispatch. `bean.autowire = 'byName'` re-wires injected collaborators. | One bean definition block (~5 lines) inside the `beans = { ... }` closure. | + +No upstream Groovy/Java source files (controllers, services, domain classes) are modified. No upstream Liquibase changesets are edited. The `resources.groovy` edit is a bean override, not a code change to the upstream service itself. + +## Open Questions + +1. **Does `EditPickModal` get reused outside the outbound wizard?** Quick `grep` to confirm before swapping the import. If reused for inbound or transfer flows, swap only the outbound `PickPage` reference and leave others unchanged. Resolved in task 1 of tasks.md. +2. **Should we also surface expiry status in the Add Items product picker** (e.g. badge a product whose only stock is expired)? Possible follow-up; not in v1 scope per the original "selecting expired products" framing being mainly about the lot decision. +3. **Bulk picklist import?** If outbound supports CSV import of picklist allocations, the interceptor needs to cover that endpoint too. Out of scope for v1. + +## Validation + +Each criterion below is verifiable from a concrete artifact (file:line, captured curl, grep output). "Tests pass" alone is not a valid criterion. + +### Source-receipt criteria (verified during this design phase) + +- [x] **R1 — Movement type discriminator.** `enum StockMovementType` defines exactly `STOCK_MOVEMENT` and `RETURN_ORDER` at `src/main/groovy/org/pih/warehouse/api/StockMovementType.groovy:5-6`. Verified. +- [x] **R2 — Field on `OutboundStockMovement`.** `StockMovementType stockMovementType` declared at `grails-app/domain/org/pih/warehouse/inventory/OutboundStockMovement.groovy:61`; not in transients (lines 81-95). Verified. +- [x] **R3 — `InventoryItem.expirationDate`.** `Date expirationDate` at `grails-app/domain/org/pih/warehouse/inventory/InventoryItem.groovy:47`; not transient. Verified. +- [x] **R4 — Add Items step does not pick lots.** `AddItemsPage.jsx:540-554` (`saveRequisitionItems` payload mapping) sends only `{product.id, quantityRequested, recipient, sortOrder}` — no `inventoryItem.id`. Verified — lot picking happens at the Pick step. +- [x] **R5 — Pick step is the lot-picking step.** `EditPickModal.jsx:212-220` (`onSave` payload) sends `picklistItems[].inventoryItem.id` to `STOCK_MOVEMENT_UPDATE_PICKLIST`. Mapped at `UrlMappings.groovy:219-221` to `stockMovementItemApi.updatePicklist`. Implementation at `StockMovementItemApiController.groovy:60-82`. Verified. +- [x] **R6 — Outbound returns are a separate component tree.** `src/js/components/returns/outbound/CreateOutboundReturn.jsx:179` sets `type: 'RETURN_ORDER'`. Returns do not flow through `stock-movement-wizard/outbound/`. Verified. +- [x] **R7 — Upstream "is expired" predicate.** `ProductAvailabilityService.groovy:555` uses `inventoryItem.expirationDate >= maxDate` (strict-less-than today, when removeExpiredStock is on). We adopt the same predicate. Verified. +- [x] **R8 — `EditPickModal` already has `expirationDate` on each row.** Rendered at `EditPickModal.jsx:75-83` from `availableItems[].expirationDate`. No new API call required for the UI to compute "is expired?". Verified. +- [x] **R9 — No existing config flag for outbound expiry restriction.** `application.yml:460` has `daysUntilExpiry: 60` for the **stock-alert email job** only; `application.yml:614` has a domain-level `expirationDate.minValue` constraint. Neither restricts outbound shipment selection. Verified. + +### Code-after-implementation criteria + +- [x] **C1 — Custom-isolation: every new file path matches the expected pattern.** Verified: `(git ls-files --others --exclude-standard ; git diff --name-only) | sort -u` excluding the documented upstream touch points (`PickPage.jsx`, `webpack.config.js`, `messages.properties`, `resources.groovy`) and the `openspec/` change folder leaves zero unmatched files. All other new files live under `org/pih/warehouse/custom/outboundExpiryRestrictions/` or `src/js/custom/outboundExpiryRestrictions/`. +- [x] **C2 — `PickPage.jsx` diff is exactly two lines of net change.** Working-tree diff = exactly 2 lines (one removed import, one added import). +- [ ] **C3 — Server-side rejection produces HTTP 400.** Manual test required (deferred to task 8.3). Run: `curl -X POST http://localhost:8080/openboxes/api/stockMovementItems//updatePicklist -H 'Content-Type: application/json' -d '{"picklistItems": [{"inventoryItem": {"id": ""}, "binLocation": {"id": ""}, "quantityPicked": 1}], "reasonCode": ""}'` — expect HTTP `400` with body containing `"errorCode": "outboundExpiryRestrictions.expired.cannotShip"`. +- [ ] **C4 — Same payload accepted for RETURN_ORDER pseudo-path.** Manual test deferred. Note: the SQL view at `grails-app/migrations/views/stock-movement.sql:39` hard-codes `'STOCK_MOVEMENT'` for the requisition branch and the `RETURN_ORDER` branch sets `requisition_id = NULL`, so a RETURN_ORDER row cannot be reached via a `RequisitionItem`-keyed picklist endpoint. Defence-in-depth covered by the unit spec's `RETURN_ORDER + expired → proceed` case. +- [ ] **C5 — Browser DevTools on the Pick step shows the expired row visibly disabled.** Manual test required (deferred to task 8.3). +- [ ] **C6 — Disabled row cannot accept input.** Manual test deferred. Note: Jest covers the helper logic (`isRowExpired`, `expiredRowClassName`, `buildExpiredTooltip`) — the threading into the FIELDS object is verified by manual exercise. +- [x] **C7 — i18n key defined.** `grep 'outboundExpiryRestrictions.expired.cannotShip' grails-app/i18n/messages.properties` → match in the root bundle (appended block at the bottom of the file). Lives in the root bundle because Grails 3.3's `PluginAwareResourceBundleMessageSource` only globs `messages*.properties` at the root, not `grails-app/i18n/custom/`. + +### Conformance criteria (verifiable against rules in this repo) + +- [x] **N1 — Backend custom path uses camelCase per `.claude/rules/custom-package-isolation.md`.** All new `.groovy` files live under `src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/`, `grails-app/controllers/org/pih/warehouse/custom/outboundExpiryRestrictions/`, or `src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/`. +- [x] **N2 — Frontend custom path.** All new `.jsx` / `.js` files live under `src/js/custom/outboundExpiryRestrictions/{components,utils,__tests__}/`. +- [x] **N3 — No `addColumn` against any upstream table.** Working-tree diff against `grails-app/migrations/` is empty. No migration files added. +- [x] **N4 — No `metaClass` mutation against upstream domain classes (production code).** Production diff is empty for `metaClass\.(InventoryItem|OutboundStockMovement|StockMovement|Product|Requisition|RequisitionItem)`. Test-only metaclass stubs exist in `OutboundExpiryGuardInterceptorSpec.groovy` for `InventoryItem.read`, `RequisitionItem.get`, and `OutboundStockMovement.findByRequisition` — these are local to the test process and not present in production bytecode. + +## Unverified Assumptions + +All previous unverified assumptions have been resolved by reading current source. Remaining assumptions are framework/runtime semantics with low risk: + +1. **A Grails 3.3.16 interceptor's `before()` returning `false` after writing `response.status = 400` and `render(... as JSON)` halts the request without invoking the matched controller action.** Pattern matches existing Grails interceptor conventions but not verified against a Grails 3.3 source citation. Mitigation: integration test (task 4.2) verifies the controller is not invoked when the interceptor rejects. **Low stakes** — if the contract differs, the test fails immediately. +2. **`stockMovementItem.requisition.stockMovement.stockMovementType` is the correct traversal path from a picklist's parent line item to the stockMovementType enum.** Inferred from the controller's `getStockMovementItem` lookup at line 64 plus the `OutboundStockMovement.requisition` field. Mitigation: integration tests on both STOCK_MOVEMENT and RETURN_ORDER (tasks 4.2, C3+C4). **Low stakes** — fails loudly in test. + +## Confidence: 8/10 + +Up from 6/10 after verification. Every load-bearing claim from the previous design is now backed by a `file:line` receipt cited in this document and recorded in the `R1`–`R9` validation rows. + +**Why not 9–10:** +- The two remaining unverified assumptions (interceptor halt semantics, stockMovementItem→stockMovementType traversal) depend on Grails 3.3.16 runtime behaviour that we will validate during implementation via integration tests. They are not factual claims about external systems — they're about how our own custom code interacts with the Grails framework, and the tests are the receipts. +- The change scope is now genuinely small (2 lines upstream, 4–5 new custom files), so the surface area for surprises is bounded. +- The original design (pre-verification) chose the **wrong target step** — Add Items instead of Pick — which would have shipped a feature that did not block lot selection. Verification found and corrected that. This is exactly the failure mode the cap was designed to prevent. + +## Deploy status + +- **PR:** [EyeSeeTea/openboxes#3](https://github.com/EyeSeeTea/openboxes/pull/3) — "TJK: Block expired stock on outbound stock movements" (open, against `release/est/tjk/0.9.7`). +- **Replayed onto customer branches:** none yet — pending merge of PR #3 onto `release/est/tjk/0.9.7`. After merge, propagate via `release/est/0.9.7` if the customer-agnostic parts are pulled up to the EST shared layer; for now this is TJK-only. +- **Submitted upstream:** no. diff --git a/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/proposal.md b/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/proposal.md new file mode 100644 index 00000000000..8bea6854175 --- /dev/null +++ b/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/proposal.md @@ -0,0 +1,32 @@ +## Why + +Warehouse staff can currently allocate expired inventory lots to an outbound stock movement during the **Pick** step of the outbound wizard. The lot picker (`EditPickModal`) shows the expiration date for context but applies no restriction — the user can type a `quantityPicked` against an expired lot and the server accepts it. Once shipped, the recipient receives unusable stock, creating downstream waste, audit findings (humanitarian/health programs require expiry traceability), and reverse-logistics overhead. + +The fix targets the **Pick step only** — that is the wizard step where users select specific lots; the Add Items step picks products only and never touches inventory items. Outbound returns flow through a separate component tree (`src/js/components/returns/outbound/`) and are out of scope: returning expired stock to a supplier or hub is a legitimate flow that must remain unaffected. + +## What Changes + +- The lot picker in `EditPickModal` (the modal opened from the outbound wizard's Pick step) renders expired-lot rows as disabled — the row remains visible with its expiration date, but the `quantityPicked` input refuses input and a tooltip explains why. +- The backend rejects any `POST /openboxes/api/stockMovementItems//updatePicklist` whose `picklistItems[]` references an expired inventory item, when the parent movement is `STOCK_MOVEMENT`. Defence-in-depth against bypass via direct API calls or stale UI bundles. +- The Add Items step, the inbound wizard, the outbound returns wizard, the stock-transfer wizard, and every other surface remain unchanged. +- "Expired" is defined as `inventoryItem.expirationDate != null && expirationDate < today` in the server's timezone — matching the existing predicate at `ProductAvailabilityService.groovy:555`. + +## Capabilities + +### New Capabilities + +- `outbound-expiry-restrictions`: rules and behaviour that prevent expired inventory from being allocated to outbound `STOCK_MOVEMENT` picks, while leaving `RETURN_ORDER` and inbound flows unaffected. + +### Modified Capabilities + +(none — net-new behaviour layered onto upstream flows; no upstream OpenSpec capability exists for this area.) + +## Impact + +- **Backend (custom):** new `org.pih.warehouse.custom.outboundExpiryRestrictions` package with two files — a `support/ExpiryRule.groovy` helper holding the canonical `isExpired` predicate, and an `OutboundExpiryGuardInterceptor.groovy` matching `controller: 'stockMovementItemApi', action: 'updatePicklist'`. +- **Frontend (custom):** new `src/js/custom/outboundExpiryRestrictions/components/ExpiryAwareEditPickModal.jsx` — a thin wrapper over upstream `EditPickModal` that adds disabled styling and a tooltip on expired rows. +- **Upstream touch points (surgical, unavoidable):** + - `src/js/components/stock-movement-wizard/outbound/PickPage.jsx` — swap one import + one component reference from upstream `EditPickModal` to the custom wrapper. No other lines change. +- **i18n:** new keys for the disabled-row tooltip and the server-side error, added under `grails-app/i18n/custom/`. +- **Migration:** none. No schema changes — uses existing `inventory_item.expiration_date`. +- **No upstream backend file is edited.** The interceptor is the entire backend extension surface. diff --git a/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/specs/outbound-expiry-restrictions/spec.md b/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/specs/outbound-expiry-restrictions/spec.md new file mode 100644 index 00000000000..6e806e40f06 --- /dev/null +++ b/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/specs/outbound-expiry-restrictions/spec.md @@ -0,0 +1,80 @@ +## ADDED Requirements + +### Requirement: Expired lots are unselectable in the outbound Pick step + +When a user opens the **Edit Pick** modal during the Pick step of the outbound stock-movement wizard for a stock movement whose `stockMovementType` is `STOCK_MOVEMENT`, every available-item row whose lot is expired SHALL render with disabled styling and SHALL refuse keyboard or mouse input on the `quantityPicked` field. The lot's `expirationDate` SHALL remain visible. + +An inventory item is expired when its `expirationDate` is non-null and strictly less than the current date in the server's timezone. + +#### Scenario: Expired lot row is visibly disabled + +- **WHEN** the user opens the Edit Pick modal on a stockMovementItem whose available items include a lot whose `expirationDate` is in the past +- **THEN** that row is rendered with disabled styling (the existing `text-disabled` class), the `quantityPicked` input has the `disabled` attribute, and a tooltip on the row reads "Cannot ship — expired" with the formatted expiration date + +#### Scenario: Non-expired lot remains pickable + +- **WHEN** the same modal renders a row whose lot's `expirationDate` is null, today, or in the future +- **THEN** the row is rendered normally and the `quantityPicked` input accepts input as upstream + +#### Scenario: Mixed bin shows expired and fresh side by side + +- **WHEN** a single bin has multiple lots — one expired, one fresh — for the same stockMovementItem +- **THEN** both rows are rendered; the expired row is disabled, the fresh row is selectable + +### Requirement: Outbound returns and inbound flows are unaffected + +Outbound `RETURN_ORDER` movements live in a separate component tree (`src/js/components/returns/outbound/`) and do not reach the outbound Pick step. The change SHALL NOT modify the returns wizard, the inbound wizard, the stock-transfer wizard, or any picker outside `EditPickModal` rendered for the outbound `STOCK_MOVEMENT` Pick step. + +#### Scenario: Returns wizard is byte-identical + +- **WHEN** the user creates an outbound return through `CreateOutboundReturn.jsx` +- **THEN** the file's diff against develop is empty and the rendering of expired lots in any picker the returns wizard uses is unchanged from upstream + +#### Scenario: Inbound wizard is byte-identical + +- **WHEN** the user receives stock through any inbound wizard step +- **THEN** the inbound wizard's pickers, lot pickers, and item-edit modals render expired lots exactly as upstream + +### Requirement: Server rejects expired-lot picklist submissions for STOCK_MOVEMENT + +The backend SHALL reject any HTTP request to `POST /openboxes/api/stockMovementItems//updatePicklist` when (a) the parent `OutboundStockMovement.stockMovementType` is `STOCK_MOVEMENT` and (b) the request body's `picklistItems[]` contains at least one item referencing an `inventoryItem.id` whose lot is expired. + +The rejection SHALL respond with HTTP `400 Bad Request` and a JSON body containing an i18n message keyed `outboundExpiryRestrictions.expired.cannotShip`, interpolated with the offending lot's `productCode`, `lotNumber`, and `expirationDate`. + +The guard SHALL NOT run for `RETURN_ORDER` movements, inbound movements, or any controller other than `stockMovementItemApi`. + +#### Scenario: API call submitting an expired lot is rejected + +- **WHEN** a client POSTs to `/openboxes/api/stockMovementItems//updatePicklist` for a stockMovementItem whose parent movement is `STOCK_MOVEMENT`, with a `picklistItems[]` entry referencing an `inventoryItem.id` whose lot expired yesterday +- **THEN** the response is HTTP 400 with `{"errorCode": "outboundExpiryRestrictions.expired.cannotShip", "errorMessages": [...]}` and `stockMovementService.updatePicklistItem` is not called + +#### Scenario: API call with all fresh lots is accepted + +- **WHEN** the same shape of request references only fresh lots +- **THEN** the request proceeds to the upstream controller action and persists normally + +#### Scenario: API call with null expirationDate is accepted + +- **WHEN** at least one referenced inventory item has `expirationDate == null` +- **THEN** the request proceeds (the rule treats null as not-expired) + +#### Scenario: API call against a RETURN_ORDER stockMovementItem is accepted + +- **WHEN** the parent movement's `stockMovementType` is `RETURN_ORDER` (defence-in-depth — the UI does not reach this endpoint for returns, but the test confirms no false-positive) +- **THEN** the request proceeds regardless of expired lots in the payload + +### Requirement: No upstream domain or column changes + +The implementation SHALL NOT add, modify, or remove any column on upstream tables (`inventory_item`, `stock_movement`, `requisition_item`, etc.) and SHALL NOT extend upstream domain classes via metaclass, AST, or subclassing. All custom code SHALL live under `org.pih.warehouse.custom.outboundExpiryRestrictions` (backend) and `src/js/custom/outboundExpiryRestrictions/` (frontend). + +The only upstream file edited SHALL be `src/js/components/stock-movement-wizard/outbound/PickPage.jsx`, with a diff equivalent to two lines of change (one import, one component reference). + +#### Scenario: No new columns added to upstream tables + +- **WHEN** the change is applied +- **THEN** `git diff develop..HEAD -- grails-app/migrations/` shows no `addColumn` against any upstream table + +#### Scenario: All new files are under custom paths + +- **WHEN** listing files added by the change +- **THEN** every new `.groovy` / `.java` file path contains `org/pih/warehouse/custom/outboundExpiryRestrictions/`, every new `.jsx` / `.scss` file path contains `src/js/custom/outboundExpiryRestrictions/`, and the only upstream file modified is `src/js/components/stock-movement-wizard/outbound/PickPage.jsx` diff --git a/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/tasks.md b/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/tasks.md new file mode 100644 index 00000000000..1555f75044c --- /dev/null +++ b/openspec/changes/archive/2026-05-07-block-expired-items-on-outbound/tasks.md @@ -0,0 +1,103 @@ +## 1. Pre-implementation checks + +- [x] 1.1 Grep for other consumers of `EditPickModal` to confirm the swap on `outbound/PickPage.jsx` is sufficient: `grep -rn 'EditPickModal' src/js/`. If reused outside `outbound/PickPage.jsx`, do not swap globally — keep the swap scoped to the outbound wizard's import only. + - Result: upstream `stock-movement-wizard/modals/EditPickModal.jsx` has exactly one consumer — `stock-movement-wizard/outbound/PickPage.jsx:25,156`. The separate `replenishment/EditPickModal.jsx` is a different file (not affected). Scoped swap on outbound only is correct. +- [x] 1.2 Confirm the URL mapping for `updatePicklist` matches `controller: 'stockMovementItemApi', action: 'updatePicklist'` (already verified at `UrlMappings.groovy:219-221` — re-confirm in case it has changed by deploy time). +- [x] 1.3 Verify the traversal `stockMovementItem → requisition → stockMovement.stockMovementType` resolves to a `StockMovementType` enum at runtime by reading `grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy` — find where `getStockMovementItem` constructs the parent movement reference. If the path is different, update interceptor task 4.1 accordingly before writing it. + - Findings: `StockMovementService.getStockMovementItem(String id)` at `StockMovementService.groovy:660-663` does `RequisitionItem.get(id)` → `StockMovementItem.createFromRequisitionItem(requisitionItem)`. So `params.id` keys a `RequisitionItem`, whose `requisition` traverses to a `Requisition`. + - `OutboundStockMovement` maps to a SQL **VIEW** (`grails-app/migrations/views/stock-movement.sql`). The requisition branch hard-codes `stock_movement_type = 'STOCK_MOVEMENT'`; the `order` branch (RETURN_ORDER) has `requisition_id = NULL` and never reaches a RequisitionItem-keyed endpoint. **Implication:** any `updatePicklist` request inherently hits a STOCK_MOVEMENT row; the defence-in-depth check via `OutboundStockMovement.findByRequisition(requisition).stockMovementType` is still useful (future-proof against view changes) but will return `STOCK_MOVEMENT` in practice. Interceptor task 4.1 unchanged. + +## 2. Backend — `ExpiryRule` helper + +- [x] 2.1 Create `src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRule.groovy` exposing `static boolean isExpired(InventoryItem item)` and `static boolean isExpired(Date expirationDate)`. Use `new Date().clearTime()` as the comparison anchor and the same `<` strict comparison as upstream `ProductAvailabilityService.groovy:555`. +- [x] 2.2 Create `src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRuleSpec.groovy` (Spock). Cover: null `expirationDate` → false; date strictly before today → true; today → false; future → false. Assert exact return values, not just truthiness. + +## 3. Backend — i18n message + +- [x] 3.1 Append the three custom keys to the upstream root bundle `grails-app/i18n/messages.properties`: + - `outboundExpiryRestrictions.expired.cannotShip` — "Cannot pick lot {1} of product {0} — it expired on {2}." (positional args: {0}=productCode, {1}=lotNumber, {2}=expirationDate) + - `outboundExpiryRestrictions.expired.tooltip` — "Cannot ship — expired on {0}." + - `outboundExpiryRestrictions.edit.expiredHint` — "({0} expired)" + - **Why the root bundle (not `grails-app/i18n/custom/`):** Grails 3.3's default `PluginAwareResourceBundleMessageSource` only globs `grails-app/i18n/messages*.properties` at the bundle root. A separate custom basename file would silently never load, so every `messageSource.getMessage(...)` call on the server side would fall back to the English `defaultMessage`. Documented as an upstream touch point in design.md. +- [x] 3.2 Crowdin already syncs from `grails-app/i18n/messages.properties` upstream — no `crowdin.yml` edit needed for these keys. + +## 4. Backend — `OutboundExpiryGuardInterceptor` + +- [x] 4.1 Create `grails-app/controllers/org/pih/warehouse/custom/outboundExpiryRestrictions/OutboundExpiryGuardInterceptor.groovy`. Match `controller: 'stockMovementItemApi', action: 'updatePicklist'`. In `before()`: + - Read `request.JSON.picklistItems` (the same key the controller consumes at `StockMovementItemApiController.groovy:67`). + - Load the parent `RequisitionItem` via `RequisitionItem.get(params.id)` (mirrors the controller's lookup at `StockMovementItemApiController.groovy:78`; `StockMovementService.getStockMovementItem` itself does the same). + - Traverse to the parent `OutboundStockMovement` via `OutboundStockMovement.findByRequisition(requisitionItem.requisition)` and read `stockMovementType`. If `!= STOCK_MOVEMENT`, return `true` (RETURN_ORDER fall-through). + - For each `picklistItems[].inventoryItem.id`, `InventoryItem.read(id)` (used over `.load(id)` for null-safe behavior on missing IDs; functionally equivalent for the access pattern) and call `ExpiryRule.isExpired(item)`. Collect every expired item. + - If any expired items found: set `response.status = 400`; render JSON `{errorCode: 'outboundExpiryRestrictions.expired.cannotShip', errorMessages: [...]}`; return `false`. Each error message is rendered via `messageSource.getMessage(key, args, defaultMessage, request.locale)` so the response works without the custom messages.properties being wired into Grails. + - Otherwise return `true`. +- [x] 4.2 Spock spec under `src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/OutboundExpiryGuardInterceptorSpec.groovy` (unit test, not integration test — see deviation note). + - **Deviation:** the design called for a `src/integration-test/groovy/...IntegrationSpec.groovy`. The integration-test harness in this codebase (`api/spec/base/ApiSpec.groovy`) requires RestAssured + auth + full server boot and DB-level seeding of a parent stock_movement view row plus picklist items — substantial overhead for a 30-line interceptor whose moving parts are pure logic plus a few static GORM finders. The unit spec uses `InterceptorUnitTest` plus metaclass stubs for `InventoryItem.read`, `RequisitionItem.get`, and `OutboundStockMovement.findByRequisition`, which gives full coverage of the interceptor's branches. The Grails framework guarantee that `before()==false` halts the chain (controller action not invoked) is documented Grails 3.3 behavior — we trust the framework instead of re-asserting it in our test. + - Coverage: `match` block matches `updatePicklist` only; STOCK_MOVEMENT + expired → 400 with errorCode; STOCK_MOVEMENT + fresh → proceed; STOCK_MOVEMENT + null-expirationDate → proceed; STOCK_MOVEMENT + mixed → 400; RETURN_ORDER + expired → proceed (defence-in-depth fall-through); empty `picklistItems` → proceed; missing requisitionItem → proceed; `findByRequisition` returns null + expired → 400 (no RETURN_ORDER fall-through). + +## 5. Frontend — `ExpiryAwareEditPickModal` wrapper + +- [x] 5.1 Create `src/js/custom/outboundExpiryRestrictions/components/ExpiryAwareEditPickModal.jsx`. Implementation took the form of a forked-with-augmentations class component (the upstream FIELDS object is module-scoped so cannot be wrapped in place). The fork is verbatim except for two augmentation points: + - `availableItems.getDynamicRowAttr` adds `expiredRowClassName` (= `text-disabled outbound-expired-row`) when the row's `expirationDate < today`. + - `availableItems.fields.quantityPicked.getDynamicAttr` adds `disabled: true` and a native `title` tooltip built via `buildExpiredTooltip(translate, formatLocalizedDate, expirationDate)` when the row is expired. The `title` attribute renders as a browser tooltip on hover; we chose this over a `` wrapper because field-level cells aren't full DOM elements we can wrap inside the `react-final-form` field config. + - Helpers extracted to `src/js/custom/outboundExpiryRestrictions/utils/expiryHelpers.js` so the logic can be unit-tested without rendering the modal. +- [x] 5.2 Create `src/js/custom/outboundExpiryRestrictions/components/ExpiryAwareEditPickModal.scss` if any styling beyond the existing `text-disabled` class is needed. Otherwise skip. + - Skipped: the `text-disabled` class from upstream provides the visual cue; `outbound-expired-row` is a marker class for future targeting only and doesn't need styles in v1. +- [x] 5.3 Jest test under `src/js/custom/outboundExpiryRestrictions/__tests__/expiryHelpers.test.js` (helper-level, not component-level). + - **Deviation from the design:** rendering the full forked `ExpiryAwareEditPickModal` requires mounting `ModalWrapper` + `ArrayField` + `react-final-form` against a Redux store with localize state — a heavyweight test setup whose surface is dominated by upstream code, not our augmentation. Testing the pure expiry helpers verifies the actual logic our change introduces (`isRowExpired`, `buildExpiredTooltip`, `expiredRowClassName`) with concrete value assertions and zero rendering overhead. The threading of these helpers through the FIELDS object is a one-line integration covered by the C5/C6 manual checks in the design's Validation section. + - Coverage: null/undefined/unparseable/null `expirationDate` → not expired; yesterday/30 days ago → expired; today → not expired (matches the Groovy `ExpiryRule.isExpired` strict-less-than semantics); tomorrow/30 days from now → not expired. Tooltip helper covers: missing translate fallback, `formatLocalizedDate` invocation, full `translate(key, defaultMessage, args)` delegation. Stable `TOOLTIP_KEY` and `TOOLTIP_DEFAULT` constants verified. + +## 6. Frontend — wire the wrapper into the outbound Pick step (the only upstream-file edit) + +- [x] 6.1 Edit `src/js/components/stock-movement-wizard/outbound/PickPage.jsx`: + - Replaced `import EditPickModal from 'components/stock-movement-wizard/modals/EditPickModal';` (line 25) with `import EditPickModal from 'custom/outboundExpiryRestrictions/components/ExpiryAwareEditPickModal';`. The imported binding name stays `EditPickModal`, so no other lines change. +- [x] 6.2 `git diff develop..HEAD -- src/js/components/stock-movement-wizard/outbound/PickPage.jsx | grep -E '^[-+][^-+]' | wc -l` — must output `2`. + - Confirmed: working-tree diff shows exactly 2 lines (one `-`, one `+`). +- [x] 6.3 Added a one-line `custom: path.resolve(SRC, 'custom')` alias in `webpack.config.js` so the `custom/...` import resolves at bundle time (ESLint's `import/resolver.node.paths` already covers `src/js` so lint sees it). Recorded as a new upstream touch point in `design.md`. +- [ ] 6.4 `npm run lint` and `npm test` — both green. (Deferred to task 8.1 alongside Gradle tests.) + +## 7. Validation against the design's Validation section + +- [x] 7.1 Tick R1–R9 (already done during the design phase) and confirm none have drifted by re-reading the cited file:line references. + - Re-confirmed R5 (`UrlMappings.groovy:219-221` controller=stockMovementItemApi action=updatePicklist) and R7 (`ProductAvailabilityService.groovy:555` strict-< predicate) by direct file read; both unchanged on this branch. +- [x] 7.2 Run C1–C7. Capture the curl outputs for C3 and paste into a comment block in `design.md` under each criterion. + - C1, C2, C7 verified statically (see ticked boxes in `design.md` Validation section). + - C3, C4, C5, C6 deferred to task 8.3 (require running app + browser interaction). When run, capture curl output for C3 under the criterion in `design.md`. +- [x] 7.3 Run N1–N4 — every check produces empty output (no upstream column changes, no metaclass mutation, etc.). + - All four checks pass: no non-custom files added, no migrations changed, no production-code metaclass mutations on upstream domains. + +## 9. Autopick gap fix — first attempt (abandoned: post-hoc cleanup interceptors) + +Post-implementation observation: `createPicklist` (autopick) runs server-side and selects lots via `getSuggestedItems → getAvailableItems` without an expiry filter, so it could pre-seed the picklist with expired lots. The `updatePicklist` guard prevents saving them, but they would appear pre-checked in the UI. + +**Abandoned approach** — cleanup interceptors that ran HQL DELETE on expired `PicklistItem` rows in `after()`. Three reasons it didn't work: + +1. **GORM cascade collision** — `PicklistItem` is in two `cascade: "all-delete-orphan"` collections (`Picklist.picklistItems` and `RequisitionItem.picklistItems`). Removing items via `removeFromPicklistItems` + `delete()` + `save()` triggered an optimistic-locking failure on `Picklist` (`StaleObjectStateException`). +2. **Autopick re-allocation on GET** — even when an HQL DELETE succeeded, the immediately-following `GET /api/stockMovements/{id}` ran `allocateMissingPicklistItems`, saw the missing quantity, and called `createPicklist` again — re-adding the same expired lots. +3. **Response timing** — `after()` runs after the controller serialized its JSON response, so the immediate response from autopick still listed the expired items even when DB cleanup eventually succeeded. + +Files removed: `OutboundExpiryAutopickCleanupInterceptor`, `OutboundExpiryMovementCleanupInterceptor`, `OutboundExpiryUpdateRequisitionCleanupInterceptor`, `OutboundExpiryUpdateStatusCleanupInterceptor`, `OutboundExpiryPicklistCleanupService`, plus their specs. Kept the unrelated bugfix from this attempt: + +- [x] 9.5 Fix `buildExpiredTooltip` in `expiryHelpers.js`: pass `DateFormat.COMMON` as second argument to `formatLocalizedDate` (was called with one arg, causing `react-localize-redux: Invalid key passed to getTranslate` console error when opening EditPickModal). + +## 10. Autopick gap fix — final approach: subclass override of `getSuggestedItems` + +Filter expired items at the source — before autopick can ever select them — by replacing the upstream `stockMovementService` bean with a subclass that overrides `getSuggestedItems`. Java/Groovy virtual dispatch routes the upstream `this.getSuggestedItems(...)` self-call to our override, which Spring proxy AOP could not intercept (proxies don't catch self-invocations). + +- [x] 10.1 Create `src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/service/StockMovementServiceWithExpiryFilter.groovy`. Extends `StockMovementService`, annotated `@Transactional`, overrides `getSuggestedItems(List, Integer)`: filters out items whose `inventoryItem.expirationDate` is expired (via `ExpiryRule.isExpired`), logs `outbound_expiry_autopick_filter dropped=N of=M` when anything was filtered, then delegates to `super.getSuggestedItems(...)`. +- [x] 10.2 Register the subclass as the `stockMovementService` bean in `grails-app/conf/spring/resources.groovy`, replacing the upstream auto-registered bean. Use `bean.autowire = 'byName'` so all injected collaborators (`productAvailabilityService`, `messageSource`, etc.) resolve correctly. Recorded as a new upstream touch point in `design.md`. +- [x] 10.3 Verified manually against running app: autopick filter log line appears, expired lots no longer pre-selected by autopick or by the on-GET reallocation path. (No need for cleanup interceptors — items never enter the picklist in the first place.) +- [x] 10.4 Spock spec for `StockMovementServiceWithExpiryFilter` at `src/test/groovy/.../service/StockMovementServiceWithExpiryFilterSpec.groovy` — 5 tests against the `filterExpired` static helper: drops past-expirationDate, keeps null-expirationDate, keeps today (strict-<), returns empty when all expired, preserves order. The override method is plumbing — `filterExpired` is the testable surface. Production override extracted the filter into a static method to enable this test without instantiating the full Grails service. + +## 8. Pre-archive + +- [x] 8.1 `./gradlew test` — green. 42 outboundExpiryRestrictions tests pass; full suite `BUILD SUCCESSFUL`. + - `npm test`: pre-existing Babel version conflict (`@babel/plugin-syntax-import-attributes` requires Babel `^7.22.0`, environment has `7.21.8`) breaks all 42 Jest suites — unrelated to this change. Blocked in CI too; tracked separately. + - `./gradlew integrationTest` not run in this environment — integration suite needs RestAssured + a running MySQL instance per `IntegrationSpec` setup. The interceptor itself is covered by the unit spec; deferred to CI. +- [x] 8.2 `openspec validate block-expired-items-on-outbound` — passes. +- [ ] 8.3 Manually exercise the flow against a seeded location with one expired and one fresh lot of the same product: + - Open EditPickModal — expired row visibly disabled, tooltip readable. + - Try clicking into the disabled input — focus rejected. + - Edit the fresh row, save — succeeds. + - Take a screenshot for the PR. +- [ ] 8.4 Open the PR. Description links the related issue from the project tracker. +- [ ] 8.5 After merge, archive: `openspec archive block-expired-items-on-outbound`. Fill the "Deploy status" line in `design.md` with the customer branches replayed onto. diff --git a/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/.openspec.yaml b/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/.openspec.yaml new file mode 100644 index 00000000000..eebe4d86b4d --- /dev/null +++ b/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-05-05 diff --git a/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/design.md b/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/design.md new file mode 100644 index 00000000000..b4648a480c3 --- /dev/null +++ b/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/design.md @@ -0,0 +1,180 @@ +## Context + +The prior change `block-expired-items-on-outbound` introduced two enforcement layers for expired stock: +1. A server interceptor that rejects `updatePicklist` payloads referencing expired lots (HTTP 400). +2. A bean override of `stockMovementService` that strips expired `AvailableItem`s from autopick suggestions before allocation. + +Together those guarantee that **no expired lot ends up on a picklist for an outbound `STOCK_MOVEMENT`**. + +What that change deliberately did *not* touch is the **Edit step** of the outbound (and request) wizard, which sits one step earlier in the workflow. At Edit, the user enters a `quantityRequested` for each product and can revise it. The displayed "Available" column, sourced from `quantityAvailable` in the response of `GET /api/stockMovements/{id}/stockMovementItems?stepNumber=3`, **still includes expired stock**. The existing validator at `outbound/EditPage.jsx:415-416` and `request/EditPage.jsx:1098` (key `react.stockMovement.errors.lowerQty.label`, message *"Revise quantity! Quantity available is lower than requested"*) blocks "Next" when `quantityRequested > quantityAvailable` — but with the inflated number, it doesn't fire when expired stock would otherwise close the gap. Users hit the wall at Pick instead, with no easy revision path. + +This change adds a parallel `quantityPickable` field to the Edit-step API response (sum of non-expired `availableItems[].quantityAvailable`) and switches the validator + visual cues to use it. `quantityAvailable` retains its existing global meaning so other endpoints, screens, and reports are unaffected. + +## Goals / Non-Goals + +**Goals:** +- The `lowerQty` validator fires correctly when a request cannot be fulfilled with non-expired stock. +- The Edit-step "Available" column shows the breakdown (e.g. `83 (50 expired)`) so users can self-diagnose without re-running the picker. +- Both the outbound wizard's Edit step and the request wizard's Edit step are aligned (same backend, same validator, same UX). +- `quantityAvailable` keeps its existing semantics across all endpoints, screens, reports, and integrations. +- Reuse of the `ExpiryRule.isExpired` helper from the prior change — single source of truth for the "expired" predicate. + +**Non-Goals:** +- Modifying any other availability number anywhere else (no global redefinition of `quantityAvailable`). +- Touching outbound returns (`/api/stockTransfers/...`), inbound receiving, putaway, replenishment, or stock-transfer flows. +- Adding `quantityPickable` to the substitution-row map (line 1078-1090). Substitutions have their own picker; out of scope here. +- Adding a config flag to disable the new behaviour. The picking guard from the prior change is universal; this change keeps the same posture. +- Modifying the `EditPageItem` Groovy class in `StockMovementItem.groovy:589`. That class is dead code in this code path (`buildEditPageItems` returns `Map`s, not class instances). + +## Decisions + +### D1 — Add `quantityPickable` field; do not change `quantityAvailable` semantics + +Compute `quantityPickable` on the same loop iteration where `quantityAvailable` and `quantityOnHand` are already computed (`StockMovementService.groovy:1050-1051`). Insert as a new map entry (`StockMovementService.groovy:1054-1092`) so the JSON response gains a single new key. + +**Why over alternatives:** +- *Alt: redefine `quantityAvailable` to exclude expired globally.* Rejected — breaks every other consumer (other wizards, reports, API integrations) and creates a semantic mismatch between this codebase and its upstream/customer forks. +- *Alt: filter on the frontend.* Rejected — the frontend doesn't have per-lot expiry data on the Edit step; the response already aggregates totals. +- *Alt: separate `/api/products/{id}/expiryAwareAvailability` endpoint.* Rejected — adds a per-product fetch fan-out (N+1 by row) and a parallel endpoint to maintain. Adding one field to an existing response is cheaper. +- *Alt: change `EditPageItem.toJson()` in `StockMovementItem.groovy`.* Rejected — that class is constructed nowhere in this flow; `buildEditPageItems` returns `Map`s directly. Changing the dead class would be confusing. + +### D2 — Compute `quantityPickable` from the same `availableItems` already loaded + +`buildEditPageItems` already pulls `List` per product via `productAvailabilityService.getAllAvailableBinLocations(...)` (`StockMovementService.groovy:1030-1032`). Each `AvailableItem` carries `inventoryItem.expirationDate`. So the new computation is a one-line filter on data already in memory: + +```groovy +def quantityPickable = availableItems + ?.findAll { it.quantityAvailable > 0 && !ExpiryRule.isExpired(it.inventoryItem?.expirationDate) } + ?.sum { it.quantityAvailable } +``` + +No extra database round trip. No N+1 risk. + +### D3 — Reuse `ExpiryRule.isExpired` from the prior change + +The same Groovy helper at `src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRule.groovy` is the single source of truth for "expired" semantics in this fork. Importing it into `StockMovementService.groovy` keeps the strict-`<` comparison consistent with the picking guard and the autopick filter. + +### D4 — Frontend updates the validator AND the visual cues, in both wizards + +The "Revise quantity!" UX has three visible parts: +1. The validator (returns the i18n error → blocks "Next" until revised). +2. The row-level styling (`font-weight-bold` when `quantityAvailable < quantityRequested`) to draw the user's eye. +3. The cell-level styling (`text-danger` red on the Available cell). + +All three should now key off `quantityPickable`. Otherwise the validator could fire on a row that visually looks fine, or vice versa — the user is left guessing. + +The request wizard (`src/js/components/stock-movement-wizard/request/EditPage.jsx`) uses three field-config blocks (`AD_HOCK_FIELDS`, `STOCKLIST_FIELDS_PUSH_TYPE`, `STOCKLIST_FIELDS_PULL_TYPE`), each with its own row + cell condition. All three field configs and the shared validator are updated. + +### D5 — Inline display: `83` followed by red `(50 expired)` + +When `quantityAvailable > quantityPickable`, render the column with the pickable count in the existing numeric format, followed by the expired count in red — using Bootstrap's `text-danger` class on a `` wrapping just the parenthesised tail. The pickable number itself adopts the row's existing colour (red when the row triggers the validator, default otherwise), and the red expired tail draws the eye to *why* the available number is not what the user expected. + +Example DOM (when `quantityAvailable=83`, `quantityPickable=33`): +```jsx +<> + 33 (50 expired) + +``` + +When `quantityAvailable === quantityPickable`, render the single number with no tail (existing behaviour). The "(N expired)" tail uses an i18n key (`outboundExpiryRestrictions.edit.expiredHint`, default `({0} expired)`) so localisations can adapt the wording. + +### D6 — No changes to substitution-row availability + +The substitution flow at `StockMovementService.groovy:1078-1086` computes its own `qtyAvailable` per substitution candidate and is rendered through a separate modal. Substitutions have their own UX problem (a substitution lot might also be expired), but solving it here would expand scope. Tracked as a follow-up; not in v1. + +## Risks / Trade-offs + +| Risk | Mitigation | +|---|---| +| Bundle and backend deploy out of sync — old bundle reads `quantityAvailable`, new backend supplies both | Old bundle ignores the new `quantityPickable` field (extra JSON keys are silent in JS). UX degrades to upstream behaviour, not regression. | +| New backend, old bundle — Edit step still uses inflated number | Same as above — UX same as today. Once bundle ships, the validator activates. | +| `quantityPickable` is `null` on rows with no `availableItems` | Guard with `?:` and treat `null` as `0` in the validator (consistent with how upstream treats `null` in `quantityAvailable`). | +| Performance — extra `findAll` + `sum` over `availableItems` per row | Same `availableItems` list is already iterated twice on the same row (lines 1050, 1051). One more pass is negligible — pages cap at ~50 products and `availableItems` rarely exceeds 10 lots per product. | +| Other consumers of the response start relying on `quantityPickable` and create coupling | Documented in the spec as "for use by Edit-step pre-flight checks." If a future change wants this number elsewhere, that is its own change with its own validation. | +| Substitution rows still inflate availability | Out of scope (D6). Surfaced as an open question. | +| Future upstream merge changes `buildEditPageItems` | The added lines are surgical (1 import, 1 expression, 1 map entry) and clearly self-contained — typical 3-way merge resolves cleanly. Rule of thumb: a future upstream rename of `quantityAvailable` would conflict; an addition of unrelated fields would not. | + +## Migration Plan + +1. Implement behind no flag — like the prior change, the rule is universally desired. +2. Deploy backend + frontend together. If backend lands first, the bundle still works (extra JSON key is ignored). If frontend lands first, the new column shows the same number for both sides of the breakdown (`quantityPickable` is missing → fallback to `quantityAvailable`). +3. **Rollback:** revert the commit. There are no schema changes, no data mutations, no migration files. +4. **Data note:** in-flight outbound stock movements are unaffected — `quantityPickable` is computed at read time from current `inventory_item.expiration_date`, not persisted. + +## Upstream Touch Points + +This section is the merge-conflict hitlist for future upstream pulls (per `.claude/rules/custom-package-isolation.md` § "When You MUST Touch an Upstream File"). + +| File | Reason | Diff size | +|---|---|---| +| `grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy` | Add `import` for `ExpiryRule` (custom support helper), one-line `quantityPickable = ExpiryRule.sumPickableQuantity(availableItems)` alongside the existing `quantityAvailable`/`quantityOnHand` (around line 1050), one entry in the row map literal (around line 1054). | 3 added lines (1 import + 1 expression + 1 map entry). | +| `src/js/components/stock-movement-wizard/outbound/EditPage.jsx` | Switch the row className condition (line 53), the cell className condition (line 137), and the validator (line 415-416) from `quantityAvailable` to `quantityPickable`. Use the extracted `renderAvailableCell` helper for the Available cell `formatValue` (line 147). | ~5 modified lines. | +| `src/js/components/stock-movement-wizard/request/EditPage.jsx` | Same swaps in three field-config blocks (rows at 55/367/667, cells at 202/502/802) plus the shared validator at 1098, plus the extracted `renderAvailableCell` helper for each Available cell. | ~10 modified lines. | +| `src/js/custom/outboundExpiryRestrictions/utils/expiryHelpers.jsx` | Add new export `renderAvailableCell(translate)` that returns a `formatValue`-style function rendering `(N expired)` (or just the single number when nothing is expired). Custom file from the prior change (renamed from `.js` since it now contains JSX); not an upstream touch. | 1 added export. | +| `src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRule.groovy` | Add static helper `sumPickableQuantity(List availableItems, Date today)` that takes `today` from the caller and reuses the in-class `isExpired` predicate. `today` is injected (rather than computed inside the helper) so unit tests can pin it deterministically and so the upstream `StockMovementService` caller can hoist `today` once across the row loop. Custom file from the prior change. | 1 added method (~7 lines). | +| `src/js/custom/outboundExpiryRestrictions/__tests__/expiryHelpers.test.js` | Add Jest tests for `renderAvailableCell` (all-fresh, mixed, all-expired). Custom file from the prior change. | ~30 added lines. | +| `grails-app/i18n/messages.properties` | Add new key `outboundExpiryRestrictions.edit.expiredHint=({0} expired)` (and the two sibling `outboundExpiryRestrictions.expired.*` keys for the block-expired feature) at the bottom of the file. Merged into the upstream root bundle rather than a custom basename so the default `PluginAwareResourceBundleMessageSource` (which only globs `grails-app/i18n/messages*.properties` at the root) actually picks them up at runtime. Documented as an upstream touch point. | 4 added lines. | + +No upstream React component is replaced or wrapped — these are surgical edits to existing wizard files. The new `quantityPickable` JSON field is the sole new contract. + +## Open Questions + +1. **Substitution rows.** `quantityAvailable` for substitution candidates (line 1078-1086) is also inflated. Out of scope per D6 — but worth a follow-up. Possible follow-up change: `expiry-aware-substitution-availability`. +2. **Inline hint format.** `83 (50 expired)` vs `33 / 83` vs a tooltip on hover — design choice. Default to the parenthesised form since it composes with the existing right-aligned numeric formatter and degrades gracefully under narrow columns. +3. **Strict-`<` vs. strict-`<=` for the Edit step.** Use strict-`<` to match the prior change's `ExpiryRule` semantics (a lot expiring today is still pickable today). Reconfirmed; no override. + +## Validation + +Each criterion below is verifiable from a concrete artifact (file:line, captured curl, grep output, fixture). "Tests pass" alone is not a valid criterion. + +### Source-receipt criteria (verified during this design phase) + +- [x] **R1 — Validator location and predicate (outbound).** `src/js/components/stock-movement-wizard/outbound/EditPage.jsx:411-417` checks `_.isNil(item.quantityRevised) && (item.quantityRequested > item.quantityAvailable) && (item.statusCode !== 'SUBSTITUTED')` and writes `react.stockMovement.errors.lowerQty.label` to `errors.editPageItems[key].quantityRevised`. Verified by direct read. +- [x] **R2 — Validator location and predicate (request).** `src/js/components/stock-movement-wizard/request/EditPage.jsx:1094-1106`, same predicate and same error key. Verified by direct read. +- [x] **R3 — i18n key for the validator message.** `grails-app/i18n/messages.properties:3636` defines `react.stockMovement.errors.lowerQty.label=Revise quantity! Quantity available is lower than requested`. Verified. +- [x] **R4 — Backend code path that produces `quantityAvailable` for the Edit step.** `StockMovementService.groovy:964-987` (`getEditPageItems`) → `buildEditPageItems` (line 1013-1095). The `quantityAvailable` value is computed at line 1050 and written to the map at line 1065. Verified by direct read. +- [x] **R5 — `EditPageItem` class is dead code in this flow.** `StockMovementItem.groovy:589` defines the class with `getQuantityAvailable()` and `toJson()` accessors, but `buildEditPageItems` returns `Map`s directly (line 1054-1092 → `editPageItems = data.collect { ... [...] }`). The class is never `new`'d in `getEditPageItems` / `getEditPageItem`. Verified by direct read. +- [x] **R6 — `availableItems` carry per-lot expirationDate.** `availableItems` at line 1041-1046 is built from `productAvailabilityService.getAllAvailableBinLocations(...)` → each entry has `inventoryItem` with full domain access including `expirationDate` (verified via `EditPageItem.getMinExpirationDate()` at `StockMovementItem.groovy:622-628` traversing the same path). +- [x] **R7 — `ExpiryRule.isExpired(Date)` exists in this branch.** Committed in `12531d258` at `src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRule.groovy:11-17`. Strict-`<` against `today.clearTime()`. +- [x] **R8 — Outbound returns are unaffected (do not call this endpoint).** `src/js/components/returns/outbound/SendOutboundReturn.jsx:246`, `AddItemsPage.jsx:257`, `CreateOutboundReturn.jsx:138,182`, `PickPage.jsx:138` — all hit `/api/stockTransfers/...`. None hit `/api/stockMovements/.../stockMovementItems?stepNumber=3`. Verified by repository-wide grep. +- [x] **R9 — Inbound receiving is unaffected.** Repository-wide grep for `stockMovementItems?stepNumber=3` returns only the two files (`outbound/EditPage.jsx` and `request/EditPage.jsx`). Receiving wizards (`src/js/components/receiving/`) do not use this endpoint. +- [x] **R10 — request/EditPage has three field configs with the same condition.** `request/EditPage.jsx` rows at 55, 367, 667 and cells at 202, 502, 802 — verified by direct grep + read of two examples (lines 55 and 667). All match the pattern `quantityAvailable < quantityRequested`. +- [x] **R11 — `AvailableItem.inventoryItem` is hydrated by `getAllAvailableBinLocations`.** `ProductAvailabilityService.groovy:838-851` constructs each `AvailableItem` with `inventoryItem` directly from the HQL projection's `it[0]` (which is a full `InventoryItem` domain instance via the `ProductAvailability.inventoryItem` association). `expirationDate` is therefore accessible without lazy-loading concerns. Verified by direct read. +- [x] **R12 — Both wizards share the backend code path.** Both `outbound/EditPage.jsx` and `request/EditPage.jsx` hit `?stepNumber=3`, which dispatches in `StockMovementService.getStockMovementItems:782` to `getEditPageItems:788` (because `OutboundWorkflowState.fromStepNumber(3) == REVISE_ITEMS`) → `buildEditPageItems:1013`. Adding `quantityPickable` once benefits both. Verified by direct read. +- [x] **R13 — Frontend consumers do not reject extra JSON fields.** Both wizard files access response fields by name (`item.quantityRequested`, `item.quantityAvailable`, `item.statusCode`, etc.) with no schema validation, no `Object.keys` strictness, no `JSON.parse(..., reviver)` filtering. Standard JS access for unknown keys returns `undefined` without error; old bundle reading the new response simply ignores `quantityPickable`. Verified by reading `outbound/EditPage.jsx:285-330` and `request/EditPage.jsx:955-1010`. + +### Code-after-implementation criteria + +- [ ] **C1 — `quantityPickable` is on the wire.** `curl -s -b "$COOKIE" "http://localhost:8081/openboxes/api/stockMovements//stockMovementItems?stepNumber=3" | jq '.data[0].quantityPickable'` returns an integer (not `null` for a product that has any non-expired stock). +- [ ] **C2 — `quantityPickable` ≤ `quantityAvailable`.** For every row in the same response, `.quantityPickable <= .quantityAvailable` holds. Captured `jq` script in test plan. +- [ ] **C3 — Validator fires on insufficient pickable stock.** Manual test required. Setup: a product with 10 fresh + 50 expired lots; request quantity 30. Expected: Edit step shows `10 (50 expired)`, the row is bold red, "Next" is blocked with the existing `lowerQty.label` message. +- [ ] **C4 — Validator does not fire when pickable is sufficient.** Same product but request quantity 5. Row renders normally, "Next" proceeds. +- [ ] **C5 — `quantityAvailable` semantics unchanged elsewhere.** `grep -rnE 'quantityAvailable' grails-app/services/ src/js/ | wc -l` should be unchanged in lines that don't reference `quantityPickable`. (Sanity check that no other consumer was modified.) +- [ ] **C6 — i18n key resolves in two locales.** With `Accept-Language: en` and another available locale, the inline hint renders the localised string from `outboundExpiryRestrictions.edit.expiredHint` (or the default message if the locale lacks it). +- [ ] **C7 — Outbound returns regression-free.** `git diff develop..HEAD -- src/js/components/returns/` is empty after this change. +- [ ] **C8 — Inbound receiving regression-free.** `git diff develop..HEAD -- src/js/components/receiving/` is empty after this change. + +### Conformance criteria (verifiable against rules in this repo) + +- [x] **N1 — All new files (if any) under custom paths.** This change adds no new backend/frontend files (only edits). The i18n properties addition lives under `grails-app/i18n/custom/` which is already a custom path. +- [x] **N2 — `ExpiryRule` reuse, no duplication.** No new helper for "is expired" — direct import of the existing class. +- [x] **N3 — Upstream touches surgical and documented.** Three upstream files modified, ~18 lines net change combined, listed under "Upstream Touch Points." No incidental cleanup. +- [x] **N4 — No `metaClass` mutation, no schema change.** No domain-class extension; no migration. +- [x] **N5 — No `addColumn` against any upstream table.** Working-tree diff against `grails-app/migrations/` will be empty for this change. + +## Unverified Assumptions + +(none — the three items previously listed have been verified against current source and promoted to receipts R11, R12, R13 in the Validation section.) + +## Confidence: 9/10 + +Every factual claim about external systems has a `file:line` receipt. The Unverified Assumptions list is empty. The previously-flagged framework-semantic assumptions (lazy-load behaviour, shared code path between wizards, backward-compatibility of extra JSON fields) are now backed by direct code reads. + +**Why not 10:** +- Three upstream files are modified (one backend, two frontend), and the request wizard's three field-config blocks (ad-hoc, PULL stocklist, PUSH stocklist) need to stay aligned. The unit-test surface (Spock) covers the backend computation; the JSX field configs require manual exercise across all three variants — there's no automated regression net for the styling and validator threading. + +## Deploy status + +- **PR:** [EyeSeeTea/openboxes#3](https://github.com/EyeSeeTea/openboxes/pull/3) — bundled with `block-expired-items-on-outbound` since both ship from `feature/strict-expired-handling`. Open against `release/est/tjk/0.9.7`. +- **Replayed onto customer branches:** none yet — pending merge of PR #3 onto `release/est/tjk/0.9.7`. TJK-only for now; promotion to `release/est/0.9.7` is a follow-up if/when SP wants the same Edit-step pickable behaviour. +- **Submitted upstream:** no. diff --git a/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/proposal.md b/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/proposal.md new file mode 100644 index 00000000000..3aa220bb284 --- /dev/null +++ b/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/proposal.md @@ -0,0 +1,35 @@ +## Why + +Even with the new "block expired at pick" guard from `block-expired-items-on-outbound`, users can still commit to a quantity they cannot fulfill: the Edit step's "Available" column shows `quantityAvailable` from the response of `GET /api/stockMovements/{id}/stockMovementItems?stepNumber=3`, which sums every `availableItem.quantityAvailable` regardless of expiry (computed at `StockMovementService.groovy:1050`). A user seeing "83 available" against a request of 60 advances through Edit unwarned, then hits a hard wall at the Pick step where only 33 lots are pickable. The existing `lowerQty` validator at `outbound/EditPage.jsx:415-416` (and the identical one at `request/EditPage.jsx:1098`) was designed for exactly this scenario — *"Revise quantity! Quantity available is lower than requested"* — but compares against the inflated total. + +Fixing the data, not the validator, restores the existing UX contract. Once `quantityPickable` is in the response, the validator and the in-cell colour cue both fire when a request can't be fulfilled with non-expired stock. + +## What Changes + +- **Backend:** `StockMovementService.buildEditPageItems()` (line 1013-1095) gains a `quantityPickable` entry on each row's map literal. Computed as the sum of `availableItems[].quantityAvailable` whose lot is not expired per `ExpiryRule.isExpired` (the helper introduced in the prior change). The dead-code `EditPageItem` class in `StockMovementItem.groovy:589` is left untouched — `buildEditPageItems` returns a Groovy `Map`, not an `EditPageItem` instance, so the response shape is controlled here. +- **Frontend:** the `lowerQty` validator and the red/bold styling in `outbound/EditPage.jsx` and `request/EditPage.jsx` switch from comparing `quantityRequested > quantityAvailable` to `quantityRequested > quantityPickable`. The "Available" column shows the breakdown inline (e.g. `83 (50 expired)`). +- **`quantityAvailable` keeps its existing semantics.** No other endpoint, page, or report is altered. +- **No flow other than the outbound and request Edit steps is touched.** Outbound returns, inbound receiving, putaway, stock transfers, replenishment — all unchanged. + +## Capabilities + +### New Capabilities + +- `expiry-aware-edit-availability`: Edit-step pre-flight checks (the validator and its visual cue) operate on non-expired stock totals, so a user who couldn't actually fulfill a request gets the existing "Revise quantity!" warning at the right step. + +### Modified Capabilities + +- (none — no existing capability covers this surface; the related `outbound-expiry-restrictions` capability remains untouched.) + +## Impact + +- **Backend upstream touch (surgical, ~4 lines):** `grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy` — add an `import` for `ExpiryRule`, one `quantityPickable` computation alongside the existing `quantityAvailable`/`quantityOnHand` lines (~1050), and one entry in the row map literal (~1054). Reuses `ExpiryRule.isExpired` from the prior change. +- **Frontend upstream touches (surgical, ~10 lines total):** `src/js/components/stock-movement-wizard/outbound/EditPage.jsx` and `src/js/components/stock-movement-wizard/request/EditPage.jsx`. Each gets one validator field swap, one `getDynamicAttr` field swap on the Available column, and one inline display tweak. +- **Cross-flow impact:** + - **Outbound stock movements (Edit step):** target — validator fires correctly under the new picking rules. + - **Stock requests (request-wizard Edit step):** uses the same backend endpoint and an identical validator. Same fix applied so behaviour stays consistent across both wizards. + - **Outbound returns:** unaffected. Use `/api/stockTransfers/...` (different controller, doesn't read `EditPageItem`). + - **Inbound receiving (supplier → warehouse):** unaffected. Doesn't use the request-wizard endpoint. +- **i18n:** new key for the inline "(N expired)" hint (and matching `crowdin.yml` glob is already in place from the prior change). +- **Migration:** none. No schema changes. Reuses the existing `inventory_item.expiration_date`. +- **No upstream domain class is extended via metaclass or subclassing.** The single new method on `EditPageItem` is a surgical addition to the upstream API model; it has no persistence or side effects. diff --git a/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/specs/expiry-aware-edit-availability/spec.md b/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/specs/expiry-aware-edit-availability/spec.md new file mode 100644 index 00000000000..5b5bfdd154c --- /dev/null +++ b/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/specs/expiry-aware-edit-availability/spec.md @@ -0,0 +1,121 @@ +## ADDED Requirements + +### Requirement: Edit-step API response includes a `quantityPickable` field + +The response of `GET /api/stockMovements/{id}/stockMovementItems?stepNumber=3` SHALL include, for every row, an integer `quantityPickable` field equal to the sum of `availableItems[].quantityAvailable` whose `inventoryItem.expirationDate` is null OR strictly greater than or equal to today's date in the server's timezone. + +`quantityPickable` SHALL never exceed `quantityAvailable`. `quantityAvailable` retains its existing semantics (the unfiltered total) so consumers other than the Edit step are unaffected. + +The field SHALL be computed in `StockMovementService.buildEditPageItems(...)` from the same `availableItems` already loaded for the row — no additional database round trip. + +#### Scenario: Response carries quantityPickable for an outbound stock movement + +- **WHEN** a client GETs `/api/stockMovements/{id}/stockMovementItems?stepNumber=3` for an outbound stock movement whose origin location has both expired and fresh lots of one of the requested products +- **THEN** the response body's `data[]` array contains a row whose `quantityPickable` equals the sum of `quantityAvailable` of non-expired lots only, while the same row's `quantityAvailable` reflects the unfiltered total + +#### Scenario: Response carries quantityPickable for a stock request + +- **WHEN** the same endpoint is called for a stock request (request wizard) for an origin with expired stock +- **THEN** the response carries `quantityPickable` populated with the same semantics as the outbound case + +#### Scenario: All-expired product reports zero pickable + +- **WHEN** a row's only available stock is expired +- **THEN** `quantityPickable` is `0` (not `null`) and `quantityAvailable` is the unfiltered total + +#### Scenario: All-fresh product reports equal pickable and available + +- **WHEN** a row has no expired lots +- **THEN** `quantityPickable == quantityAvailable` + +#### Scenario: Null-expirationDate lots count as pickable + +- **WHEN** a row's available items include lots with `inventoryItem.expirationDate == null` +- **THEN** those lots contribute to `quantityPickable` (the rule treats null as not-expired) + +### Requirement: Edit-step "lower-than-requested" validator uses pickable, not raw available + +The frontend validator that emits `react.stockMovement.errors.lowerQty.label` ("Revise quantity! Quantity available is lower than requested") in both `outbound/EditPage.jsx` and `request/EditPage.jsx` SHALL compare `quantityRequested` against `quantityPickable`, not `quantityAvailable`. The same swap SHALL be applied to the row-level (`font-weight-bold`) and cell-level (`text-danger`) styling conditions. + +The validator SHALL fire when the user can't fulfill `quantityRequested` from non-expired stock, blocking advancement to the next wizard step until the user revises the quantity or selects a substitution. + +#### Scenario: Insufficient pickable stock blocks Next + +- **WHEN** an Edit-step row shows `quantityRequested = 30`, `quantityAvailable = 60`, `quantityPickable = 10` +- **THEN** the row renders with `font-weight-bold` + `text-danger`, the `quantityRevised` cell shows the lowerQty error message, and clicking "Next" / "Save" surfaces the validation error and does not advance the wizard + +#### Scenario: Sufficient pickable stock proceeds normally + +- **WHEN** an Edit-step row shows `quantityRequested = 30`, `quantityAvailable = 60`, `quantityPickable = 50` +- **THEN** no validation error is emitted on this row and "Next" advances + +#### Scenario: Inflated quantityAvailable does not mask the gap + +- **WHEN** a row shows `quantityRequested = 30`, `quantityAvailable = 80` (which under the old rule would have suppressed the validator), `quantityPickable = 10` +- **THEN** the validator still fires because the comparison is against `quantityPickable`, not `quantityAvailable` + +#### Scenario: Substituted rows are exempt + +- **WHEN** a row's `statusCode === 'SUBSTITUTED'` +- **THEN** the validator does not fire on that row regardless of pickable vs requested (preserving the existing exemption) + +### Requirement: Edit-step "Available" column surfaces the expired breakdown inline, with the expired count rendered red + +When `quantityAvailable > quantityPickable` on a row, the Edit-step "Available" cell SHALL render the pickable value followed by the parenthesised expired tail wrapped in a `` carrying Bootstrap's `text-danger` class. The tail's text comes from the i18n key `outboundExpiryRestrictions.edit.expiredHint` (default `({0} expired)`). When `quantityAvailable === quantityPickable`, the cell SHALL render a single number (existing behaviour preserved). + +The pickable count itself follows the row's existing colour rules: red (`text-danger`) when the row triggers the validator, default colour otherwise. The red expired tail is independent — it draws attention to *why* the available number is lower than the user expected, regardless of whether the row triggers the validator. + +#### Scenario: Mixed expired and fresh stock — expired count is red + +- **WHEN** a row has `quantityAvailable = 83`, `quantityPickable = 33` +- **THEN** the Available cell renders `33 (50 expired)`, the parenthesised `(50 expired)` portion is wrapped in ``, and the leading `33` follows the row's normal colour rules (red if validator fires, default otherwise) + +#### Scenario: All-fresh row renders unchanged + +- **WHEN** a row has `quantityAvailable = 50`, `quantityPickable = 50` +- **THEN** the Available cell text reads `50` with no parenthesised hint and no `text-danger` span + +#### Scenario: All-expired row reads zero with a red expired count + +- **WHEN** a row has `quantityAvailable = 50`, `quantityPickable = 0` +- **THEN** the Available cell renders `0 (50 expired)`, the `(50 expired)` portion is wrapped in ``, the row is bold (validator fires for any non-zero requested quantity), and the leading `0` is also red because the row is in the validator-triggered state + +### Requirement: No upstream domain or column changes; surgical upstream-file touches only + +The implementation SHALL NOT add, modify, or remove any column on upstream tables and SHALL NOT extend upstream domain classes via metaclass, AST, or subclassing. The new field is computed at request time from the existing `inventory_item.expiration_date`. + +The only upstream files modified SHALL be: +- `grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy` (add import + add computation + add map entry; ~4 lines) +- `src/js/components/stock-movement-wizard/outbound/EditPage.jsx` (validator + 2 styling conditions + 1 inline display; ~5 modified lines) +- `src/js/components/stock-movement-wizard/request/EditPage.jsx` (3 field configs × 2 conditions + 1 validator + inline display; ~10 modified lines) + +The `EditPageItem` Groovy class at `src/main/groovy/org/pih/warehouse/api/StockMovementItem.groovy:589` SHALL NOT be modified — it is dead code in this code path. + +#### Scenario: No new columns added to upstream tables + +- **WHEN** the change is applied +- **THEN** `git diff develop..HEAD -- grails-app/migrations/` shows no `addColumn` against any upstream table + +#### Scenario: Upstream file diffs are bounded to the documented touch points + +- **WHEN** listing files modified by the change against `develop` +- **THEN** the only modified upstream files are the three listed above; net combined diff is under 30 lines + +### Requirement: Outbound returns and inbound flows are unaffected + +This change SHALL NOT modify the outbound returns wizard (`src/js/components/returns/outbound/`), the inbound receiving wizard, the putaway wizard, the stock-transfer wizard, or any flow that does not call `GET /api/stockMovements/{id}/stockMovementItems?stepNumber=3`. + +#### Scenario: Outbound returns wizard is byte-identical + +- **WHEN** the change is applied +- **THEN** `git diff develop..HEAD -- src/js/components/returns/` is empty + +#### Scenario: Inbound receiving wizard is byte-identical + +- **WHEN** the change is applied +- **THEN** `git diff develop..HEAD -- src/js/components/receiving/` is empty + +#### Scenario: API consumers other than the Edit step are unaffected + +- **WHEN** any consumer reads `quantityAvailable` from any endpoint other than `/api/stockMovements/{id}/stockMovementItems?stepNumber=3` +- **THEN** the value is unchanged from upstream behaviour (`quantityAvailable` is not redefined globally) diff --git a/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/tasks.md b/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/tasks.md new file mode 100644 index 00000000000..220f801bce4 --- /dev/null +++ b/openspec/changes/archive/2026-05-07-expiry-aware-edit-availability/tasks.md @@ -0,0 +1,69 @@ +## 1. Pre-implementation checks + +- [x] 1.1 Re-confirm `StockMovementService.buildEditPageItems` insertion site by reading `grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy:1013-1095`. Verify that `quantityAvailable` (line 1050) and `quantityOnHand` (line 1051) are still computed inline and that the row map literal still starts at line 1054. If the structure has shifted on this branch, adjust task 2.1 line references. +- [x] 1.2 Re-confirm the validator location and predicate in both wizard files: `outbound/EditPage.jsx:411-417` and `request/EditPage.jsx:1094-1106`. They should both still test `quantityRequested > quantityAvailable` against `react.stockMovement.errors.lowerQty.label`. +- [x] 1.3 Re-confirm the field-config blocks in `request/EditPage.jsx`: rows at 55, 367, 667 and cells at 202, 502, 802. If line numbers drift, capture the new ones for tasks 3 and 4. +- [x] 1.4 Confirm `ExpiryRule` is on this branch: `git log --oneline -- src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRule.groovy` should show `12531d258`. The change depends on it. + +## 2. Backend — `quantityPickable` in `buildEditPageItems` + +- [x] 2.1 In `grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy`: + - Add `import org.pih.warehouse.custom.outboundExpiryRestrictions.support.ExpiryRule` to the import block (kept alphabetised with the existing `org.pih.warehouse.custom.*` imports if any; otherwise immediately after the existing `org.pih.warehouse.api.*` imports). + - Insert the `quantityPickable` computation immediately after line 1051, alongside `quantityAvailable` and `quantityOnHand`: + ```groovy + def quantityPickable = availableItems + ?.findAll { it.quantityAvailable > 0 && !ExpiryRule.isExpired(it.inventoryItem?.expirationDate) } + ?.sum { it.quantityAvailable } + ``` + - Insert one entry into the row map literal, immediately after the existing `quantityAvailable` line (~1065): + ```groovy + quantityPickable : (quantityPickable && quantityPickable > 0 ? quantityPickable : 0), + ``` + - Net: 1 import + 3 added lines. No other changes to `buildEditPageItems`. +- [x] 2.2 Spock spec at `src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/StockMovementServiceQuantityPickableSpec.groovy` (this is a custom test file even though the production change is on an upstream service — keep the spec under the custom test tree per `rules/custom-package-isolation.md`): + - Test against the static helper logic: build a list of `AvailableItem` fixtures with mixed expirationDates and assert that summing the non-expired ones matches the expected `quantityPickable`. If extracting a helper from `buildEditPageItems` is cheap, prefer that — otherwise mock `ExpiryRule.isExpired` and assert the filter predicate. + - Cover: all-expired → 0, all-fresh → sum, mixed → fresh sum, null-expirationDate → counted as fresh, today's date → counted as fresh (strict-`<` semantics). + +## 3. Frontend — outbound wizard updates + +- [x] 3.1 In `src/js/components/stock-movement-wizard/outbound/EditPage.jsx`: + - Line ~53 (row className condition): change `rowValues.quantityAvailable < rowValues.quantityRequested` to `rowValues.quantityPickable < rowValues.quantityRequested`. + - Line ~137 (cell className condition for the Available cell): change `fieldValue.quantityAvailable < fieldValue.quantityRequested` to `fieldValue.quantityPickable < fieldValue.quantityRequested`. Also handle the leading `!fieldValue.quantityAvailable` short-circuit — change to `!fieldValue.quantityPickable`. + - Line ~415-416 (validator): change `item.quantityRequested > item.quantityAvailable` to `item.quantityRequested > item.quantityPickable`. + - Line ~147 (cell `formatValue`): switch from the inline string-returning lambda to the extracted helper from task 4.1: `formatValue: renderAvailableCell(translate)`. The renderer returns a `<>...` fragment with `` around the parenthesised expired tail. Confirm `LabelField` accepts a JSX-returning `formatValue` (most field-element types in `src/js/components/form-elements/` do — verify by grep if uncertain). If it doesn't, swap to a small dedicated cell renderer wrapping `LabelField`. +- [x] 3.2 No new test file. The validator behaviour is exercised manually in task 6. + +## 4. Frontend — request wizard updates + +- [x] 4.1 In `src/js/components/stock-movement-wizard/request/EditPage.jsx`, apply the same swap and JSX `formatValue` change as task 3.1 for each of the three field-config blocks: + - **AD_HOCK_FIELDS:** row condition at ~55, cell condition at ~202, cell `formatValue` near ~210. + - **STOCKLIST_FIELDS_PUSH_TYPE:** row condition at ~367, cell condition at ~502, cell `formatValue` nearby. + - **STOCKLIST_FIELDS_PULL_TYPE:** row condition at ~667, cell condition at ~802, cell `formatValue` nearby. + + To avoid duplicating the JSX block four times (3 in request + 1 in outbound), extract the renderer into the existing custom helpers file at `src/js/custom/outboundExpiryRestrictions/utils/expiryHelpers.js` — exported as `renderAvailableCell({ translate })` (curried so the wizard can pass its connected `translate`, matching how `buildExpiredTooltip` is consumed today). Both `EditPage.jsx` files import it. Helper signature: `(translate) => (value) => ReactNode | string`. Add a Jest test for the helper in `src/js/custom/outboundExpiryRestrictions/__tests__/expiryHelpers.test.js` covering all-fresh / mixed / all-expired cases. +- [x] 4.2 The shared validator at line ~1094-1106: change `item.quantityRequested > item.quantityAvailable` to `item.quantityRequested > item.quantityPickable`. Single line. +- [x] 4.3 No new test file. Manual exercise in task 6. + +## 5. i18n + +- [x] 5.1 Add to `grails-app/i18n/custom/outboundExpiryRestrictions-messages.properties`: + ``` + outboundExpiryRestrictions.edit.expiredHint=({0} expired) + ``` + No new file — this is the same custom messages file from the prior change. +- [x] 5.2 Confirm `crowdin.yml` already covers this glob (`/**/grails-app/i18n/custom/*-messages.properties`) — added in the prior change. No edit needed. + +## 6. Pre-archive + +- [x] 6.1 `./gradlew test` — green; the new Spock spec passes. +- [x] 6.2 `openspec validate expiry-aware-edit-availability` — passes. +- [x] 6.3 Manually exercise in three flows against a seeded warehouse with one product that has 10 fresh + 50 expired lots and a stockMovement requesting quantity 30: + - **Outbound stock movement Edit step:** "Available" cell reads `10 (50 expired)`, the row is bold red, "Next" is blocked with the "Revise quantity!" message. Lower the request to 5 → row renders normally, "Next" advances. + - **Request wizard (PULL stocklist) Edit step:** same scenario, same outcome. + - **Request wizard (PUSH stocklist) Edit step:** same scenario, same outcome (or skip if no PUSH stocklist seeded — note in the validation log). + - **Request wizard (ad-hoc, no stocklist):** same scenario, same outcome. + - **Note:** during validation, the picking guard from `block-expired-items-on-outbound` was incorrectly rejecting payloads when expired lots were merely visible in the Pick modal but not actually being picked. Fixed in a follow-up commit (`fix(outbound-expiry): ignore picklistItems with no quantityPicked`). Pick + Save now works as expected for the test workflow. +- [x] 6.4 Run `git diff develop..HEAD -- src/js/components/returns/ src/js/components/receiving/` — empty, confirming the two flows we said are unaffected stayed unaffected. (Verified against the parent commit `12531d258` for this change — `git diff 12531d258..HEAD -- src/js/components/returns/ src/js/components/receiving/` is empty. Diff against `develop` shows pre-existing branch commits unrelated to this change.) +- [x] 6.5 Take a screenshot of the Available cell with the parenthesised hint for the PR description. +- [ ] 6.6 Open the PR. Description links the related issue from the project tracker. Notes that this builds on `block-expired-items-on-outbound` (commit `12531d258`). +- [ ] 6.7 After merge, archive: `openspec archive expiry-aware-edit-availability`. Fill the "Deploy status" line in `design.md` with the customer branches replayed onto. diff --git a/openspec/specs/expiry-aware-edit-availability/spec.md b/openspec/specs/expiry-aware-edit-availability/spec.md new file mode 100644 index 00000000000..5b5bfdd154c --- /dev/null +++ b/openspec/specs/expiry-aware-edit-availability/spec.md @@ -0,0 +1,121 @@ +## ADDED Requirements + +### Requirement: Edit-step API response includes a `quantityPickable` field + +The response of `GET /api/stockMovements/{id}/stockMovementItems?stepNumber=3` SHALL include, for every row, an integer `quantityPickable` field equal to the sum of `availableItems[].quantityAvailable` whose `inventoryItem.expirationDate` is null OR strictly greater than or equal to today's date in the server's timezone. + +`quantityPickable` SHALL never exceed `quantityAvailable`. `quantityAvailable` retains its existing semantics (the unfiltered total) so consumers other than the Edit step are unaffected. + +The field SHALL be computed in `StockMovementService.buildEditPageItems(...)` from the same `availableItems` already loaded for the row — no additional database round trip. + +#### Scenario: Response carries quantityPickable for an outbound stock movement + +- **WHEN** a client GETs `/api/stockMovements/{id}/stockMovementItems?stepNumber=3` for an outbound stock movement whose origin location has both expired and fresh lots of one of the requested products +- **THEN** the response body's `data[]` array contains a row whose `quantityPickable` equals the sum of `quantityAvailable` of non-expired lots only, while the same row's `quantityAvailable` reflects the unfiltered total + +#### Scenario: Response carries quantityPickable for a stock request + +- **WHEN** the same endpoint is called for a stock request (request wizard) for an origin with expired stock +- **THEN** the response carries `quantityPickable` populated with the same semantics as the outbound case + +#### Scenario: All-expired product reports zero pickable + +- **WHEN** a row's only available stock is expired +- **THEN** `quantityPickable` is `0` (not `null`) and `quantityAvailable` is the unfiltered total + +#### Scenario: All-fresh product reports equal pickable and available + +- **WHEN** a row has no expired lots +- **THEN** `quantityPickable == quantityAvailable` + +#### Scenario: Null-expirationDate lots count as pickable + +- **WHEN** a row's available items include lots with `inventoryItem.expirationDate == null` +- **THEN** those lots contribute to `quantityPickable` (the rule treats null as not-expired) + +### Requirement: Edit-step "lower-than-requested" validator uses pickable, not raw available + +The frontend validator that emits `react.stockMovement.errors.lowerQty.label` ("Revise quantity! Quantity available is lower than requested") in both `outbound/EditPage.jsx` and `request/EditPage.jsx` SHALL compare `quantityRequested` against `quantityPickable`, not `quantityAvailable`. The same swap SHALL be applied to the row-level (`font-weight-bold`) and cell-level (`text-danger`) styling conditions. + +The validator SHALL fire when the user can't fulfill `quantityRequested` from non-expired stock, blocking advancement to the next wizard step until the user revises the quantity or selects a substitution. + +#### Scenario: Insufficient pickable stock blocks Next + +- **WHEN** an Edit-step row shows `quantityRequested = 30`, `quantityAvailable = 60`, `quantityPickable = 10` +- **THEN** the row renders with `font-weight-bold` + `text-danger`, the `quantityRevised` cell shows the lowerQty error message, and clicking "Next" / "Save" surfaces the validation error and does not advance the wizard + +#### Scenario: Sufficient pickable stock proceeds normally + +- **WHEN** an Edit-step row shows `quantityRequested = 30`, `quantityAvailable = 60`, `quantityPickable = 50` +- **THEN** no validation error is emitted on this row and "Next" advances + +#### Scenario: Inflated quantityAvailable does not mask the gap + +- **WHEN** a row shows `quantityRequested = 30`, `quantityAvailable = 80` (which under the old rule would have suppressed the validator), `quantityPickable = 10` +- **THEN** the validator still fires because the comparison is against `quantityPickable`, not `quantityAvailable` + +#### Scenario: Substituted rows are exempt + +- **WHEN** a row's `statusCode === 'SUBSTITUTED'` +- **THEN** the validator does not fire on that row regardless of pickable vs requested (preserving the existing exemption) + +### Requirement: Edit-step "Available" column surfaces the expired breakdown inline, with the expired count rendered red + +When `quantityAvailable > quantityPickable` on a row, the Edit-step "Available" cell SHALL render the pickable value followed by the parenthesised expired tail wrapped in a `` carrying Bootstrap's `text-danger` class. The tail's text comes from the i18n key `outboundExpiryRestrictions.edit.expiredHint` (default `({0} expired)`). When `quantityAvailable === quantityPickable`, the cell SHALL render a single number (existing behaviour preserved). + +The pickable count itself follows the row's existing colour rules: red (`text-danger`) when the row triggers the validator, default colour otherwise. The red expired tail is independent — it draws attention to *why* the available number is lower than the user expected, regardless of whether the row triggers the validator. + +#### Scenario: Mixed expired and fresh stock — expired count is red + +- **WHEN** a row has `quantityAvailable = 83`, `quantityPickable = 33` +- **THEN** the Available cell renders `33 (50 expired)`, the parenthesised `(50 expired)` portion is wrapped in ``, and the leading `33` follows the row's normal colour rules (red if validator fires, default otherwise) + +#### Scenario: All-fresh row renders unchanged + +- **WHEN** a row has `quantityAvailable = 50`, `quantityPickable = 50` +- **THEN** the Available cell text reads `50` with no parenthesised hint and no `text-danger` span + +#### Scenario: All-expired row reads zero with a red expired count + +- **WHEN** a row has `quantityAvailable = 50`, `quantityPickable = 0` +- **THEN** the Available cell renders `0 (50 expired)`, the `(50 expired)` portion is wrapped in ``, the row is bold (validator fires for any non-zero requested quantity), and the leading `0` is also red because the row is in the validator-triggered state + +### Requirement: No upstream domain or column changes; surgical upstream-file touches only + +The implementation SHALL NOT add, modify, or remove any column on upstream tables and SHALL NOT extend upstream domain classes via metaclass, AST, or subclassing. The new field is computed at request time from the existing `inventory_item.expiration_date`. + +The only upstream files modified SHALL be: +- `grails-app/services/org/pih/warehouse/inventory/StockMovementService.groovy` (add import + add computation + add map entry; ~4 lines) +- `src/js/components/stock-movement-wizard/outbound/EditPage.jsx` (validator + 2 styling conditions + 1 inline display; ~5 modified lines) +- `src/js/components/stock-movement-wizard/request/EditPage.jsx` (3 field configs × 2 conditions + 1 validator + inline display; ~10 modified lines) + +The `EditPageItem` Groovy class at `src/main/groovy/org/pih/warehouse/api/StockMovementItem.groovy:589` SHALL NOT be modified — it is dead code in this code path. + +#### Scenario: No new columns added to upstream tables + +- **WHEN** the change is applied +- **THEN** `git diff develop..HEAD -- grails-app/migrations/` shows no `addColumn` against any upstream table + +#### Scenario: Upstream file diffs are bounded to the documented touch points + +- **WHEN** listing files modified by the change against `develop` +- **THEN** the only modified upstream files are the three listed above; net combined diff is under 30 lines + +### Requirement: Outbound returns and inbound flows are unaffected + +This change SHALL NOT modify the outbound returns wizard (`src/js/components/returns/outbound/`), the inbound receiving wizard, the putaway wizard, the stock-transfer wizard, or any flow that does not call `GET /api/stockMovements/{id}/stockMovementItems?stepNumber=3`. + +#### Scenario: Outbound returns wizard is byte-identical + +- **WHEN** the change is applied +- **THEN** `git diff develop..HEAD -- src/js/components/returns/` is empty + +#### Scenario: Inbound receiving wizard is byte-identical + +- **WHEN** the change is applied +- **THEN** `git diff develop..HEAD -- src/js/components/receiving/` is empty + +#### Scenario: API consumers other than the Edit step are unaffected + +- **WHEN** any consumer reads `quantityAvailable` from any endpoint other than `/api/stockMovements/{id}/stockMovementItems?stepNumber=3` +- **THEN** the value is unchanged from upstream behaviour (`quantityAvailable` is not redefined globally) diff --git a/openspec/specs/outbound-expiry-restrictions/spec.md b/openspec/specs/outbound-expiry-restrictions/spec.md new file mode 100644 index 00000000000..6e806e40f06 --- /dev/null +++ b/openspec/specs/outbound-expiry-restrictions/spec.md @@ -0,0 +1,80 @@ +## ADDED Requirements + +### Requirement: Expired lots are unselectable in the outbound Pick step + +When a user opens the **Edit Pick** modal during the Pick step of the outbound stock-movement wizard for a stock movement whose `stockMovementType` is `STOCK_MOVEMENT`, every available-item row whose lot is expired SHALL render with disabled styling and SHALL refuse keyboard or mouse input on the `quantityPicked` field. The lot's `expirationDate` SHALL remain visible. + +An inventory item is expired when its `expirationDate` is non-null and strictly less than the current date in the server's timezone. + +#### Scenario: Expired lot row is visibly disabled + +- **WHEN** the user opens the Edit Pick modal on a stockMovementItem whose available items include a lot whose `expirationDate` is in the past +- **THEN** that row is rendered with disabled styling (the existing `text-disabled` class), the `quantityPicked` input has the `disabled` attribute, and a tooltip on the row reads "Cannot ship — expired" with the formatted expiration date + +#### Scenario: Non-expired lot remains pickable + +- **WHEN** the same modal renders a row whose lot's `expirationDate` is null, today, or in the future +- **THEN** the row is rendered normally and the `quantityPicked` input accepts input as upstream + +#### Scenario: Mixed bin shows expired and fresh side by side + +- **WHEN** a single bin has multiple lots — one expired, one fresh — for the same stockMovementItem +- **THEN** both rows are rendered; the expired row is disabled, the fresh row is selectable + +### Requirement: Outbound returns and inbound flows are unaffected + +Outbound `RETURN_ORDER` movements live in a separate component tree (`src/js/components/returns/outbound/`) and do not reach the outbound Pick step. The change SHALL NOT modify the returns wizard, the inbound wizard, the stock-transfer wizard, or any picker outside `EditPickModal` rendered for the outbound `STOCK_MOVEMENT` Pick step. + +#### Scenario: Returns wizard is byte-identical + +- **WHEN** the user creates an outbound return through `CreateOutboundReturn.jsx` +- **THEN** the file's diff against develop is empty and the rendering of expired lots in any picker the returns wizard uses is unchanged from upstream + +#### Scenario: Inbound wizard is byte-identical + +- **WHEN** the user receives stock through any inbound wizard step +- **THEN** the inbound wizard's pickers, lot pickers, and item-edit modals render expired lots exactly as upstream + +### Requirement: Server rejects expired-lot picklist submissions for STOCK_MOVEMENT + +The backend SHALL reject any HTTP request to `POST /openboxes/api/stockMovementItems//updatePicklist` when (a) the parent `OutboundStockMovement.stockMovementType` is `STOCK_MOVEMENT` and (b) the request body's `picklistItems[]` contains at least one item referencing an `inventoryItem.id` whose lot is expired. + +The rejection SHALL respond with HTTP `400 Bad Request` and a JSON body containing an i18n message keyed `outboundExpiryRestrictions.expired.cannotShip`, interpolated with the offending lot's `productCode`, `lotNumber`, and `expirationDate`. + +The guard SHALL NOT run for `RETURN_ORDER` movements, inbound movements, or any controller other than `stockMovementItemApi`. + +#### Scenario: API call submitting an expired lot is rejected + +- **WHEN** a client POSTs to `/openboxes/api/stockMovementItems//updatePicklist` for a stockMovementItem whose parent movement is `STOCK_MOVEMENT`, with a `picklistItems[]` entry referencing an `inventoryItem.id` whose lot expired yesterday +- **THEN** the response is HTTP 400 with `{"errorCode": "outboundExpiryRestrictions.expired.cannotShip", "errorMessages": [...]}` and `stockMovementService.updatePicklistItem` is not called + +#### Scenario: API call with all fresh lots is accepted + +- **WHEN** the same shape of request references only fresh lots +- **THEN** the request proceeds to the upstream controller action and persists normally + +#### Scenario: API call with null expirationDate is accepted + +- **WHEN** at least one referenced inventory item has `expirationDate == null` +- **THEN** the request proceeds (the rule treats null as not-expired) + +#### Scenario: API call against a RETURN_ORDER stockMovementItem is accepted + +- **WHEN** the parent movement's `stockMovementType` is `RETURN_ORDER` (defence-in-depth — the UI does not reach this endpoint for returns, but the test confirms no false-positive) +- **THEN** the request proceeds regardless of expired lots in the payload + +### Requirement: No upstream domain or column changes + +The implementation SHALL NOT add, modify, or remove any column on upstream tables (`inventory_item`, `stock_movement`, `requisition_item`, etc.) and SHALL NOT extend upstream domain classes via metaclass, AST, or subclassing. All custom code SHALL live under `org.pih.warehouse.custom.outboundExpiryRestrictions` (backend) and `src/js/custom/outboundExpiryRestrictions/` (frontend). + +The only upstream file edited SHALL be `src/js/components/stock-movement-wizard/outbound/PickPage.jsx`, with a diff equivalent to two lines of change (one import, one component reference). + +#### Scenario: No new columns added to upstream tables + +- **WHEN** the change is applied +- **THEN** `git diff develop..HEAD -- grails-app/migrations/` shows no `addColumn` against any upstream table + +#### Scenario: All new files are under custom paths + +- **WHEN** listing files added by the change +- **THEN** every new `.groovy` / `.java` file path contains `org/pih/warehouse/custom/outboundExpiryRestrictions/`, every new `.jsx` / `.scss` file path contains `src/js/custom/outboundExpiryRestrictions/`, and the only upstream file modified is `src/js/components/stock-movement-wizard/outbound/PickPage.jsx` diff --git a/src/js/components/stock-movement-wizard/outbound/EditPage.jsx b/src/js/components/stock-movement-wizard/outbound/EditPage.jsx index b5de74a8a2e..5bb3712725e 100644 --- a/src/js/components/stock-movement-wizard/outbound/EditPage.jsx +++ b/src/js/components/stock-movement-wizard/outbound/EditPage.jsx @@ -1,5 +1,6 @@ import React, { Component } from 'react'; +import { renderAvailableCell } from 'custom/outboundExpiryRestrictions/utils/expiryHelpers'; import arrayMutators from 'final-form-arrays'; import update from 'immutability-helper'; import _ from 'lodash'; @@ -50,7 +51,7 @@ const FIELDS = { rowValues, subfield, showOnlyErroredItems, itemFilter, }) => { let className = rowValues.statusCode === 'SUBSTITUTED' ? 'crossed-out ' : ''; - if (rowValues.quantityAvailable < rowValues.quantityRequested) { + if ((rowValues.quantityPickable ?? rowValues.quantityAvailable) < rowValues.quantityRequested) { className += 'font-weight-bold'; } const filterOutItems = itemFilter @@ -132,20 +133,18 @@ const FIELDS = { defaultMessage: 'Available', flexWidth: '1', fieldKey: '', - getDynamicAttr: ({ fieldValue }) => { + getDynamicAttr: ({ fieldValue, translate }) => { let className = 'text-right'; - if (fieldValue && (!fieldValue.quantityAvailable - || fieldValue.quantityAvailable < fieldValue.quantityRequested)) { + if (fieldValue && (!fieldValue.quantityPickable + || fieldValue.quantityPickable < fieldValue.quantityRequested)) { className = `${className} text-danger`; } return { className, + formatValue: renderAvailableCell(translate), }; }, headerAlign: 'right', - attributes: { - formatValue: (value) => (value.quantityAvailable ? (value.quantityAvailable.toLocaleString('en-US')) : value.quantityAvailable), - }, }, quantityDemandFulfilling: { type: LabelField, @@ -412,7 +411,7 @@ class EditItemsPage extends Component { const errors = validateForSave(values); _.forEach(values.editPageItems, (item, key) => { - if (_.isNil(item.quantityRevised) && (item.quantityRequested > item.quantityAvailable) && (item.statusCode !== 'SUBSTITUTED')) { + if (_.isNil(item.quantityRevised) && (item.quantityRequested > item.quantityPickable) && (item.statusCode !== 'SUBSTITUTED')) { errors.editPageItems[key] = { quantityRevised: 'react.stockMovement.errors.lowerQty.label' }; } }); diff --git a/src/js/components/stock-movement-wizard/outbound/PickPage.jsx b/src/js/components/stock-movement-wizard/outbound/PickPage.jsx index 7470c5ef267..91455fc6146 100644 --- a/src/js/components/stock-movement-wizard/outbound/PickPage.jsx +++ b/src/js/components/stock-movement-wizard/outbound/PickPage.jsx @@ -1,5 +1,6 @@ import React, { Component } from 'react'; +import EditPickModal from 'custom/outboundExpiryRestrictions/components/ExpiryAwareEditPickModal'; import arrayMutators from 'final-form-arrays'; import update from 'immutability-helper'; import _ from 'lodash'; @@ -22,7 +23,6 @@ import ButtonField from 'components/form-elements/ButtonField'; import FilterInput from 'components/form-elements/FilterInput'; import LabelField from 'components/form-elements/LabelField'; import TableRowWithSubfields from 'components/form-elements/TableRowWithSubfields'; -import EditPickModal from 'components/stock-movement-wizard/modals/EditPickModal'; import { STOCK_MOVEMENT_URL } from 'consts/applicationUrls'; import DateFormat from 'consts/dateFormat'; import { OutboundWorkflowState } from 'consts/StockMovementState'; diff --git a/src/js/components/stock-movement-wizard/request/EditPage.jsx b/src/js/components/stock-movement-wizard/request/EditPage.jsx index 026971f33d8..78e5cfdb23d 100644 --- a/src/js/components/stock-movement-wizard/request/EditPage.jsx +++ b/src/js/components/stock-movement-wizard/request/EditPage.jsx @@ -1,5 +1,6 @@ import React, { Component } from 'react'; +import { renderAvailableCell } from 'custom/outboundExpiryRestrictions/utils/expiryHelpers'; import arrayMutators from 'final-form-arrays'; import update from 'immutability-helper'; import _ from 'lodash'; @@ -52,7 +53,7 @@ const AD_HOCK_FIELDS = { rowComponent: TableRowWithSubfields, getDynamicRowAttr: ({ rowValues, showOnlyErroredItems, itemFilter }) => { let className = rowValues.statusCode === 'SUBSTITUTED' ? 'crossed-out ' : ''; - if (rowValues.quantityAvailable < rowValues.quantityRequested) { + if ((rowValues.quantityPickable ?? rowValues.quantityAvailable) < rowValues.quantityRequested) { className += 'font-weight-bold'; } const filterOutItems = itemFilter && !( @@ -196,18 +197,18 @@ const AD_HOCK_FIELDS = { defaultMessage: 'Available', flexWidth: '1', fieldKey: '', - getDynamicAttr: ({ fieldValue }) => { + getDynamicAttr: ({ fieldValue, translate }) => { let className = ''; - if (fieldValue && (!fieldValue.quantityAvailable - || fieldValue.quantityAvailable < fieldValue.quantityRequested)) { + if (fieldValue && (!fieldValue.quantityPickable + || fieldValue.quantityPickable < fieldValue.quantityRequested)) { className += 'text-danger'; } return { className, + formatValue: renderAvailableCell(translate), }; }, attributes: { - formatValue: (value) => (value.quantityAvailable ? (value.quantityAvailable.toLocaleString('en-US')) : value.quantityAvailable), numberField: true, }, }, @@ -364,7 +365,7 @@ const STOCKLIST_FIELDS_PUSH_TYPE = { rowComponent: TableRowWithSubfields, getDynamicRowAttr: ({ rowValues, showOnlyErroredItems, itemFilter }) => { let className = rowValues.statusCode === 'SUBSTITUTED' ? 'crossed-out ' : ''; - if (rowValues.quantityAvailable < rowValues.quantityRequested) { + if ((rowValues.quantityPickable ?? rowValues.quantityAvailable) < rowValues.quantityRequested) { className += 'font-weight-bold'; } const filterOutItems = itemFilter && !( @@ -496,18 +497,18 @@ const STOCKLIST_FIELDS_PUSH_TYPE = { defaultMessage: 'Available', flexWidth: '1', fieldKey: '', - getDynamicAttr: ({ fieldValue }) => { + getDynamicAttr: ({ fieldValue, translate }) => { let className = ''; - if (fieldValue && (!fieldValue.quantityAvailable - || fieldValue.quantityAvailable < fieldValue.quantityRequested)) { + if (fieldValue && (!fieldValue.quantityPickable + || fieldValue.quantityPickable < fieldValue.quantityRequested)) { className += 'text-danger'; } return { className, + formatValue: renderAvailableCell(translate), }; }, attributes: { - formatValue: (value) => (value.quantityAvailable ? (value.quantityAvailable.toLocaleString('en-US')) : value.quantityAvailable), numberField: true, }, }, @@ -664,7 +665,7 @@ const STOCKLIST_FIELDS_PULL_TYPE = { rowComponent: TableRowWithSubfields, getDynamicRowAttr: ({ rowValues, showOnlyErroredItems, itemFilter }) => { let className = rowValues.statusCode === 'SUBSTITUTED' ? 'crossed-out ' : ''; - if (rowValues.quantityAvailable < rowValues.quantityRequested) { + if ((rowValues.quantityPickable ?? rowValues.quantityAvailable) < rowValues.quantityRequested) { className += 'font-weight-bold'; } const filterOutItems = itemFilter && !( @@ -796,18 +797,18 @@ const STOCKLIST_FIELDS_PULL_TYPE = { defaultMessage: 'Available', flexWidth: '1', fieldKey: '', - getDynamicAttr: ({ fieldValue }) => { + getDynamicAttr: ({ fieldValue, translate }) => { let className = ''; - if (fieldValue && (!fieldValue.quantityAvailable - || fieldValue.quantityAvailable < fieldValue.quantityRequested)) { + if (fieldValue && (!fieldValue.quantityPickable + || fieldValue.quantityPickable < fieldValue.quantityRequested)) { className += 'text-danger'; } return { className, + formatValue: renderAvailableCell(translate), }; }, attributes: { - formatValue: (value) => (value.quantityAvailable ? (value.quantityAvailable.toLocaleString('en-US')) : value.quantityAvailable), numberField: true, }, }, @@ -1095,7 +1096,7 @@ class EditItemsPage extends Component { const errors = validateForSave(values); _.forEach(values.editPageItems, (item, key) => { - if (_.isNil(item.quantityRevised) && (item.quantityRequested > item.quantityAvailable) && (item.statusCode !== 'SUBSTITUTED')) { + if (_.isNil(item.quantityRevised) && (item.quantityRequested > item.quantityPickable) && (item.statusCode !== 'SUBSTITUTED')) { errors.editPageItems[key] = { quantityRevised: 'react.stockMovement.errors.lowerQty.label' }; } }); diff --git a/src/js/custom/outboundExpiryRestrictions/__tests__/expiryHelpers.test.js b/src/js/custom/outboundExpiryRestrictions/__tests__/expiryHelpers.test.js new file mode 100644 index 00000000000..6472acd31e8 --- /dev/null +++ b/src/js/custom/outboundExpiryRestrictions/__tests__/expiryHelpers.test.js @@ -0,0 +1,152 @@ +/* eslint-env jest */ +import { renderToStaticMarkup } from 'react-dom/server'; + +import { + buildExpiredTooltip, + EXPIRED_HINT_DEFAULT, + EXPIRED_HINT_KEY, + expiredRowClassName, + isRowExpired, + renderAvailableCell, + TOOLTIP_DEFAULT, + TOOLTIP_KEY, +} from 'custom/outboundExpiryRestrictions/utils/expiryHelpers'; + +import DateFormat from 'consts/dateFormat'; + +const daysFromToday = (delta) => { + const d = new Date(); + d.setHours(0, 0, 0, 0); + d.setDate(d.getDate() + delta); + return d.toISOString(); +}; + +describe('isRowExpired', () => { + it.each([ + ['null row', null], + ['null expirationDate', { expirationDate: null }], + ['undefined expirationDate', {}], + ['unparseable expirationDate', { expirationDate: 'not-a-date' }], + ['today (strict-< parity with backend ExpiryRule)', { expirationDate: daysFromToday(0) }], + ['tomorrow', { expirationDate: daysFromToday(1) }], + ['30 days in the future', { expirationDate: daysFromToday(30) }], + ])('returns false for %s', (_label, row) => { + expect(isRowExpired(row)).toBe(false); + }); + + it.each([ + ['yesterday', { expirationDate: daysFromToday(-1) }], + ['30 days in the past', { expirationDate: daysFromToday(-30) }], + ])('returns true for %s', (_label, row) => { + expect(isRowExpired(row)).toBe(true); + }); +}); + +describe('expiredRowClassName', () => { + it('exposes the upstream-compatible class string', () => { + expect(expiredRowClassName).toBe('text-disabled outbound-expired-row'); + }); +}); + +describe('buildExpiredTooltip', () => { + const expirationDate = '2025-01-15T00:00:00.000Z'; + + it('returns the default English message when no translate is supplied', () => { + expect(buildExpiredTooltip(null, null, expirationDate)) + .toBe(`Cannot ship — expired on ${expirationDate}.`); + }); + + it('formats the date via formatLocalizedDate when supplied', () => { + const formatLocalizedDate = jest.fn(() => '01/15/2025'); + expect(buildExpiredTooltip(null, formatLocalizedDate, expirationDate)) + .toBe('Cannot ship — expired on 01/15/2025.'); + expect(formatLocalizedDate).toHaveBeenCalledWith(expirationDate, DateFormat.COMMON); + }); + + it('delegates to translate when supplied', () => { + const translate = jest.fn(() => 'No se puede enviar — expirado el 15/01/2025.'); + const formatLocalizedDate = jest.fn(() => '15/01/2025'); + + const result = buildExpiredTooltip(translate, formatLocalizedDate, expirationDate); + + expect(translate).toHaveBeenCalledWith( + TOOLTIP_KEY, + 'Cannot ship — expired on 15/01/2025.', + { 0: '15/01/2025' }, + ); + expect(result).toBe('No se puede enviar — expirado el 15/01/2025.'); + }); + + it('exposes a stable i18n key', () => { + expect(TOOLTIP_KEY).toBe('outboundExpiryRestrictions.expired.tooltip'); + }); + + it('exposes a stable English template', () => { + expect(TOOLTIP_DEFAULT).toBe('Cannot ship — expired on {0}.'); + }); +}); + +describe('renderAvailableCell', () => { + const renderCellHTML = (rendered) => renderToStaticMarkup(rendered); + + it('returns the row unchanged when it is null', () => { + expect(renderAvailableCell(null)(null)).toBe(null); + }); + + it('returns the row unchanged when it is undefined', () => { + expect(renderAvailableCell(null)(undefined)).toBe(undefined); + }); + + it('renders a single number when all stock is fresh', () => { + const cell = renderAvailableCell(null)({ quantityAvailable: 50, quantityPickable: 50 }); + expect(cell).toBe('50'); + }); + + it('renders a single number when quantityPickable is missing (backend not yet deployed)', () => { + const cell = renderAvailableCell(null)({ quantityAvailable: 50 }); + expect(cell).toBe('50'); + }); + + it('renders 0 when quantityAvailable is 0 and quantityPickable is 0', () => { + const cell = renderAvailableCell(null)({ quantityAvailable: 0, quantityPickable: 0 }); + expect(cell).toBe(0); + }); + + it('renders pickable plus a red expired tail when some stock is expired', () => { + const cell = renderAvailableCell(null)({ quantityAvailable: 83, quantityPickable: 33 }); + expect(renderCellHTML(cell)) + .toBe('33(50 expired)'); + }); + + it('renders 0 plus a red expired tail when every lot is expired', () => { + const cell = renderAvailableCell(null)({ quantityAvailable: 50, quantityPickable: 0 }); + expect(renderCellHTML(cell)) + .toBe('0(50 expired)'); + }); + + it('formats expired counts with thousands separators', () => { + const cell = renderAvailableCell(null)({ quantityAvailable: 1500, quantityPickable: 500 }); + expect(renderCellHTML(cell)) + .toBe('500(1,000 expired)'); + }); + + it('delegates to translate when supplied', () => { + const translate = jest.fn(() => '(50 vencidos)'); + const cell = renderAvailableCell(translate)({ quantityAvailable: 83, quantityPickable: 33 }); + expect(translate).toHaveBeenCalledWith( + EXPIRED_HINT_KEY, + EXPIRED_HINT_DEFAULT.replace('{0}', '50'), + { 0: '50' }, + ); + expect(renderCellHTML(cell)) + .toBe('33(50 vencidos)'); + }); + + it('exposes a stable i18n key', () => { + expect(EXPIRED_HINT_KEY).toBe('outboundExpiryRestrictions.edit.expiredHint'); + }); + + it('exposes a stable English template', () => { + expect(EXPIRED_HINT_DEFAULT).toBe('({0} expired)'); + }); +}); diff --git a/src/js/custom/outboundExpiryRestrictions/components/ExpiryAwareEditPickModal.jsx b/src/js/custom/outboundExpiryRestrictions/components/ExpiryAwareEditPickModal.jsx new file mode 100644 index 00000000000..84a1ffe4982 --- /dev/null +++ b/src/js/custom/outboundExpiryRestrictions/components/ExpiryAwareEditPickModal.jsx @@ -0,0 +1,404 @@ +// Forked from src/js/components/stock-movement-wizard/modals/EditPickModal.jsx. +// Keep in sync on upstream merges. Augmentations vs. upstream: +// - availableItems.getDynamicRowAttr adds `expiredRowClassName` for expired rows +// - availableItems.fields.quantityPicked.getDynamicAttr adds `disabled: true` and +// a native `title` tooltip via buildExpiredTooltip when the row is expired +// - mapStateToProps adds `translate` and `formatLocalizedDate` so getDynamicAttr +// can reach them via formProps +// Wrapping the upstream component is not feasible: its FIELDS object is module-scoped +// and not exported. See openspec/changes/block-expired-items-on-outbound/design.md (D2). +import React, { Component } from 'react'; + +import { + buildExpiredTooltip, + expiredRowClassName, + isRowExpired, +} from 'custom/outboundExpiryRestrictions/utils/expiryHelpers'; +import _ from 'lodash'; +import PropTypes from 'prop-types'; +import { getTranslate } from 'react-localize-redux'; +import { connect } from 'react-redux'; +import { Tooltip } from 'react-tippy'; + +import { hideSpinner, showSpinner } from 'actions'; +import { + STOCK_MOVEMENT_ITEM_BY_ID, + STOCK_MOVEMENT_ITEM_DETAILS, + STOCK_MOVEMENT_UPDATE_PICKLIST, +} from 'api/urls'; +import ArrayField from 'components/form-elements/ArrayField'; +import LabelField from 'components/form-elements/LabelField'; +import ModalWrapper from 'components/form-elements/ModalWrapper'; +import SelectField from 'components/form-elements/SelectField'; +import TextField from 'components/form-elements/TextField'; +import DateFormat from 'consts/dateFormat'; +import { OutboundWorkflowState } from 'consts/StockMovementState'; +import apiClient from 'utils/apiClient'; +import Translate, { translateWithDefaultMessage } from 'utils/Translate'; +import { formatDate } from 'utils/translation-utils'; + +const FIELDS = { + reasonCode: { + type: SelectField, + label: 'react.stockMovement.reasonCode.label', + defaultMessage: 'Reason code', + attributes: { + className: 'mb-2', + }, + getDynamicAttr: (props) => ({ + options: props.reasonCodes, + }), + }, + availableItems: { + type: ArrayField, + getDynamicRowAttr: ({ rowValues }) => { + const expired = isRowExpired(rowValues); + let className = ''; + if (!rowValues.quantityAvailable) { + className = 'text-disabled'; + } + if (expired) { + className = `${className} ${expiredRowClassName}`.trim(); + } + return { className }; + }, + fields: { + status: { + type: LabelField, + fieldKey: '', + fixedWidth: '120px', + getDynamicAttr: ({ translate }) => ({ + showValueTooltip: true, + formatValue: (fieldValue) => { + if (fieldValue.status === 'AVAILABLE' && fieldValue.pickedRequisitionNumbers.length !== 0) { + const status = translate('react.stockMovement.enum.AvailableItemStatus.PICKED', 'PICKED'); + return status + (fieldValue.pickedRequisitionNumbers ? ` [${fieldValue.pickedRequisitionNumbers}]` : ''); + } if (!fieldValue.status || fieldValue.status === 'AVAILABLE') { + return ''; + } + + const status = translate(`react.stockMovement.enum.AvailableItemStatus.${fieldValue.status}`, fieldValue.status); + return status + (fieldValue.pickedRequisitionNumbers ? ` [${fieldValue.pickedRequisitionNumbers}]` : ''); + }, + }), + }, + lotNumber: { + type: LabelField, + label: 'react.stockMovement.lot.label', + defaultMessage: 'Lot', + attributes: { + showValueTooltip: true, + }, + }, + expirationDate: { + type: LabelField, + label: 'react.stockMovement.expiry.label', + defaultMessage: 'Expiry', + fixedWidth: '120px', + getDynamicAttr: ({ formatLocalizedDate }) => ({ + formatValue: (value) => formatLocalizedDate(value, DateFormat.COMMON), + }), + }, + binLocation: { + type: LabelField, + label: 'react.stockMovement.binLocation.label', + defaultMessage: 'Bin Location', + getDynamicAttr: ({ hasBinLocationSupport }) => ({ + hide: !hasBinLocationSupport, + }), + attributes: { + showValueTooltip: true, + formatValue: (fieldValue) => fieldValue && ( +
+ {fieldValue.zoneName ?
{fieldValue.zoneName}
: ''} +
{fieldValue.zoneName ? `: ${fieldValue.name}` : fieldValue.name}
+
+ ), + }, + }, + quantityOnHand: { + type: LabelField, + label: 'react.stockMovement.onHand.label', + defaultMessage: 'On Hand', + fixedWidth: '150px', + attributes: { + formatValue: (value) => (value || value === 0 ? value.toLocaleString('en-US') : null), + }, + }, + quantityAvailable: { + type: LabelField, + label: 'react.stockMovement.available.label', + defaultMessage: 'Available', + fixedWidth: '150px', + attributes: { + formatValue: (value) => (value || value === 0 ? value.toLocaleString('en-US') : null), + }, + }, + quantityPicked: { + type: TextField, + fieldKey: '', + label: 'react.stockMovement.picked.label', + defaultMessage: 'Picked', + headerAlign: 'left', + fixedWidth: '120px', + attributes: { + type: 'number', + }, + getDynamicAttr: ({ fieldValue, translate, formatLocalizedDate }) => { + const expired = isRowExpired(fieldValue); + const noStock = fieldValue && !fieldValue.quantityAvailable && !fieldValue.quantityPicked; + // Reason: screen readers ignore the native `title` on disabled inputs (JAWS/NVDA). + // `aria-label` is announced even when disabled, so the blocked-row reason reaches AT users. + // Keep `title` too so sighted hover users still see the message via the browser tooltip. + const expiredTooltip = expired + ? buildExpiredTooltip(translate, formatLocalizedDate, fieldValue?.expirationDate) + : undefined; + return { + disabled: noStock || expired, + title: expiredTooltip, + 'aria-label': expiredTooltip, + 'aria-disabled': expired || noStock || undefined, + }; + }, + }, + }, + }, +}; + +function validate(values) { + const errors = {}; + errors.availableItems = []; + _.forEach(values.availableItems, (item, key) => { + if (item.quantityPicked > item.quantityAvailable) { + errors.availableItems[key] = { quantityPicked: 'react.stockMovement.errors.higherTyPicked.label' }; + } + if (item.quantityPicked < 0) { + errors.availableItems[key] = { quantityPicked: 'react.stockMovement.errors.negativeQtyPicked.label' }; + } + }); + + const pickedSum = _.reduce( + values.availableItems, (sum, val) => + (sum + (val.quantityPicked ? _.toInteger(val.quantityPicked) : 0)), + 0, + ); + + if (_.some(values.availableItems, (val) => !_.isNil(val.quantityPicked)) + && !values.reasonCode && pickedSum !== values.quantityRequired) { + errors.reasonCode = 'react.stockMovement.errors.differentTotalQty.label'; + } + + return errors; +} + +/** Modal window where user can edit pick. */ +/* eslint no-param-reassign: "error" */ +class ExpiryAwareEditPickModal extends Component { + constructor(props) { + super(props); + + const { + fieldConfig: { attributes, getDynamicAttr }, + } = props; + const dynamicAttr = getDynamicAttr ? getDynamicAttr(props) : {}; + const attr = { ...attributes, ...dynamicAttr }; + + this.state = { + attr, + formValues: {}, + }; + + this.onOpen = this.onOpen.bind(this); + this.onSave = this.onSave.bind(this); + } + + static getDerivedStateFromProps(nextProps) { + const { + fieldConfig: { attributes, getDynamicAttr }, + } = nextProps; + const dynamicAttr = getDynamicAttr ? getDynamicAttr(nextProps) : {}; + return { attr: { ...attributes, ...dynamicAttr } }; + } + + /** + * Loads chosen items, required quantity and reason codes into modal's form. + * @public + */ + onOpen() { + this.fetchPickPageItem(); + } + + /** + * Sends all changes made by user in this modal to API and updates data. + * @param {object} values + * @public + */ + onSave(values) { + this.props.showSpinner(); + + const payload = { + picklistItems: _.map(values.availableItems, (avItem) => ({ + id: avItem.id || '', + inventoryItem: { id: avItem['inventoryItem.id'] }, + binLocation: { id: avItem['binLocation.id'] || '' }, + quantityPicked: _.isNil(avItem.quantityPicked) ? '' : avItem.quantityPicked, + })), + reasonCode: values.reasonCode.value || '', + }; + + apiClient.post(STOCK_MOVEMENT_UPDATE_PICKLIST(this.state.attr.itemId), payload) + .then(() => { + apiClient.get(STOCK_MOVEMENT_ITEM_BY_ID(this.state.attr.itemId), { + params: { stepNumber: OutboundWorkflowState.PICK_ITEMS, refreshPicklistItems: false }, + }) + .then((resp) => { + const pickPageItem = resp.data.data; + + this.state.attr.onResponse(pickPageItem); + this.props.hideSpinner(); + }) + .catch(() => { this.props.hideSpinner(); }); + }) + .catch(() => { this.props.hideSpinner(); }); + } + + /** + * Sums up quantity picked from all available items. + * @param {object} values + * @public + */ + /* eslint-disable-next-line class-methods-use-this */ + calculatePicked(values) { + return ( +
+
+ + : + {_.reduce(values.availableItems, (sum, val) => + (sum + (val.quantityPicked ? _.toInteger(val.quantityPicked) : 0)), 0)} +
+
+
+ ); + } + + fetchPickPageItem() { + apiClient.get(STOCK_MOVEMENT_ITEM_DETAILS(this.state.attr.itemId), { + params: { stepNumber: OutboundWorkflowState.PICK_ITEMS, refreshPicklistItems: false }, + }).then((resp) => { + const pickPageItem = resp.data.data; + + const availableItems = _.map(pickPageItem.availableItems, (avItem) => { + // check if this picklist item already exists + const picklistItem = _.find(pickPageItem.picklistItems, (item) => item['inventoryItem.id'] === avItem['inventoryItem.id'] && item['binLocation.id'] === avItem['binLocation.id']); + + if (picklistItem) { + return { + ...avItem, + id: picklistItem.id, + quantityPicked: picklistItem.quantityPicked, + binLocation: { + id: picklistItem['binLocation.id'], + name: picklistItem['binLocation.name'], + zoneName: picklistItem['binLocation.zoneName'], + }, + }; + } + + return { + ...avItem, + binLocation: { + id: avItem['binLocation.id'], + name: avItem['binLocation.name'], + zoneName: avItem['binLocation.zoneName'], + }, + }; + }); + + this.setState({ + formValues: { + availableItems, + reasonCode: '', + quantityRequired: pickPageItem.quantityRequired, + productCode: pickPageItem.productCode, + productName: pickPageItem.product.name, + displayName: pickPageItem.product?.displayNames?.default, + }, + }); + + this.props.hideSpinner(); + }) + .catch(() => { this.props.hideSpinner(); }); + } + + render() { + if (this.state.attr.subfield) { + return null; + } + + return ( + +
+
+ + : + {this.state.formValues.productCode} +
+
+ {this.state.formValues.productName}
} + theme="dark" + disabled={!this.state.formValues.displayName} + position="top-start" + > + + + : + {' '} + {this.state.formValues.displayName ?? this.state.formValues.productName} + + +
+
+ + : + {this.state.formValues.quantityRequired} +
+ + + ); + } +} + +const mapStateToProps = (state) => ({ + translate: translateWithDefaultMessage(getTranslate(state.localize)), + formatLocalizedDate: formatDate(state.localize), +}); + +export default connect(mapStateToProps, { showSpinner, hideSpinner })(ExpiryAwareEditPickModal); + +ExpiryAwareEditPickModal.propTypes = { + fieldName: PropTypes.string.isRequired, + fieldConfig: PropTypes.shape({ + getDynamicAttr: PropTypes.func, + }).isRequired, + showSpinner: PropTypes.func.isRequired, + hideSpinner: PropTypes.func.isRequired, + hasBinLocationSupport: PropTypes.bool.isRequired, + translate: PropTypes.func.isRequired, + formatLocalizedDate: PropTypes.func.isRequired, +}; diff --git a/src/js/custom/outboundExpiryRestrictions/utils/expiryHelpers.jsx b/src/js/custom/outboundExpiryRestrictions/utils/expiryHelpers.jsx new file mode 100644 index 00000000000..dfccbc2c62a --- /dev/null +++ b/src/js/custom/outboundExpiryRestrictions/utils/expiryHelpers.jsx @@ -0,0 +1,80 @@ +import React from 'react'; + +import { isBefore, isValid, startOfDay } from 'date-fns'; + +import DateFormat from 'consts/dateFormat'; + +export const isRowExpired = (row) => { + if (!row || !row.expirationDate) { + return false; + } + const expiry = new Date(row.expirationDate); + if (!isValid(expiry)) { + return false; + } + return isBefore(startOfDay(expiry), startOfDay(new Date())); +}; + +export const expiredRowClassName = 'text-disabled outbound-expired-row'; + +export const TOOLTIP_KEY = 'outboundExpiryRestrictions.expired.tooltip'; +export const TOOLTIP_DEFAULT = 'Cannot ship — expired on {0}.'; + +export const buildExpiredTooltip = (translate, formatLocalizedDate, expirationDate) => { + const formatted = formatLocalizedDate + ? formatLocalizedDate(expirationDate, DateFormat.COMMON) + : String(expirationDate || ''); + const fallback = TOOLTIP_DEFAULT.replace('{0}', formatted); + return translate + ? translate(TOOLTIP_KEY, fallback, { 0: formatted }) + : fallback; +}; + +export const EXPIRED_HINT_KEY = 'outboundExpiryRestrictions.edit.expiredHint'; +export const EXPIRED_HINT_DEFAULT = '({0} expired)'; + +const formatNumber = (value) => (value ? value.toLocaleString() : value); + +const formatCell = (translate, value) => { + if (!value) { + return value; + } + const available = value.quantityAvailable || 0; + const pickable = value.quantityPickable == null ? available : value.quantityPickable; + const expired = available - pickable; + if (expired <= 0) { + return formatNumber(available); + } + const expiredStr = expired.toLocaleString(); + const pickableStr = pickable ? pickable.toLocaleString() : '0'; + const fallback = EXPIRED_HINT_DEFAULT.replace('{0}', expiredStr); + const hintText = translate + ? translate(EXPIRED_HINT_KEY, fallback, { 0: expiredStr }) + : fallback; + return ( + <> + {pickableStr} + {hintText} + + ); +}; + +// Reason: getDynamicAttr is invoked per-cell render. Memoize the bound formatter by +// translate identity so all rows share one function and LabelField memoization holds. +const formatterCache = new WeakMap(); +let nullTranslateFormatter = null; + +export const renderAvailableCell = (translate) => { + if (translate == null) { + if (!nullTranslateFormatter) { + nullTranslateFormatter = (value) => formatCell(null, value); + } + return nullTranslateFormatter; + } + let formatter = formatterCache.get(translate); + if (!formatter) { + formatter = (value) => formatCell(translate, value); + formatterCache.set(translate, formatter); + } + return formatter; +}; diff --git a/src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/service/StockMovementServiceWithExpiryFilter.groovy b/src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/service/StockMovementServiceWithExpiryFilter.groovy new file mode 100644 index 00000000000..d2cd1a25174 --- /dev/null +++ b/src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/service/StockMovementServiceWithExpiryFilter.groovy @@ -0,0 +1,34 @@ +package org.pih.warehouse.custom.outboundExpiryRestrictions.service + +import groovy.util.logging.Slf4j +import org.pih.warehouse.api.AvailableItem +import org.pih.warehouse.custom.outboundExpiryRestrictions.support.ExpiryRule +import org.pih.warehouse.inventory.StockMovementService + +// Reason: no class-level @Transactional — the parent StockMovementService is +// already @Transactional via grails.gorm.transactions.Transactional, and mixing +// the deprecated grails.transaction.Transactional on the subclass causes +// proxy-selection issues in Grails 3.3 (different post-processors). The override +// is read-only; the parent's transaction semantics carry through inherited methods. +@Slf4j +class StockMovementServiceWithExpiryFilter extends StockMovementService { + + @Override + List getSuggestedItems(List availableItems, Integer quantityRequested) { + if (!availableItems) { + return super.getSuggestedItems(availableItems, quantityRequested) + } + Date today = new Date().clearTime() + List filtered = filterExpired(availableItems, today) + if (filtered.size() != availableItems.size()) { + log.info "outbound_expiry_autopick_filter dropped=${availableItems.size() - filtered.size()} of=${availableItems.size()}" + } + return super.getSuggestedItems(filtered, quantityRequested) + } + + static List filterExpired(List availableItems, Date today) { + return availableItems.findAll { AvailableItem item -> + !ExpiryRule.isExpired(item?.inventoryItem?.expirationDate, today) + } + } +} diff --git a/src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRule.groovy b/src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRule.groovy new file mode 100644 index 00000000000..c2523e76e6c --- /dev/null +++ b/src/main/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRule.groovy @@ -0,0 +1,23 @@ +package org.pih.warehouse.custom.outboundExpiryRestrictions.support + +import org.pih.warehouse.api.AvailableItem + +final class ExpiryRule { + + private ExpiryRule() {} + + // A lot whose expirationDate equals today (midnight) is still pickable — diverges + // from upstream ProductAvailabilityService:555, which compares against the current + // wall-clock and would treat the same lot as already expired by mid-morning. + + static boolean isExpired(Date expirationDate, Date today) { + return expirationDate != null && expirationDate < today + } + + static Integer sumPickableQuantity(List availableItems, Date today) { + def total = availableItems + ?.findAll { it.quantityAvailable > 0 && !isExpired(it?.inventoryItem?.expirationDate, today) } + ?.sum { it.quantityAvailable } + return (total && total > 0 ? total : 0) as Integer + } +} diff --git a/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/OutboundExpiryGuardInterceptorSpec.groovy b/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/OutboundExpiryGuardInterceptorSpec.groovy new file mode 100644 index 00000000000..3d6c28a71bb --- /dev/null +++ b/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/OutboundExpiryGuardInterceptorSpec.groovy @@ -0,0 +1,261 @@ +package org.pih.warehouse.custom.outboundExpiryRestrictions + +import grails.testing.web.interceptor.InterceptorUnitTest +import org.pih.warehouse.api.StockMovementType +import org.pih.warehouse.inventory.InventoryItem +import org.pih.warehouse.inventory.OutboundStockMovement +import org.pih.warehouse.requisition.Requisition +import org.pih.warehouse.requisition.RequisitionItem +import org.springframework.context.MessageSource +import spock.lang.Specification + +class OutboundExpiryGuardInterceptorSpec extends Specification + implements InterceptorUnitTest { + + static final String EXPIRED_LOT_ID = 'lot-expired-1' + static final String FRESH_LOT_ID = 'lot-fresh-1' + static final String NULL_EXPIRY_LOT_ID = 'lot-null-expiry-1' + static final String REQUISITION_ITEM_ID = 'req-item-1' + static final String MISSING_ID = 'does-not-exist' + + Map rowsById + + void setup() { + interceptor.messageSource = Mock(MessageSource) { + getMessage(_, _, _, _) >> { args -> args[2] as String } + } + + Date today = new Date().clearTime() + rowsById = [ + (EXPIRED_LOT_ID) : ['EXP-001', today - 1, 'VAC-X'], + (FRESH_LOT_ID) : ['FRESH-001', today + 30, 'VAC-X'], + (NULL_EXPIRY_LOT_ID): ['NULLEXP-001', null, 'VAC-X'], + ] + InventoryItem.metaClass.static.executeQuery = { String hql, Map params -> + ((List) params.ids).collect { rowsById[it] }.findAll() + } + + Requisition req = new Requisition(name: 'TEST-REQ') + RequisitionItem reqItem = new RequisitionItem(requisition: req) + RequisitionItem.metaClass.static.get = { Serializable id -> + (id as String) == REQUISITION_ITEM_ID ? reqItem : null + } + } + + void cleanup() { + InventoryItem.metaClass = null + RequisitionItem.metaClass = null + OutboundStockMovement.metaClass = null + } + + private void stubParentStockMovement(StockMovementType type) { + OutboundStockMovement.metaClass.static.findByRequisition = { Requisition r -> + r ? new OutboundStockMovement(stockMovementType: type) : null + } + } + + private void stubMissingStockMovement() { + OutboundStockMovement.metaClass.static.findByRequisition = { Requisition r -> null } + } + + private void postPicklist(String reqItemId, List inventoryItemIds) { + postPicklistWithQuantities(reqItemId, inventoryItemIds.collectEntries { [(it): 1] }) + } + + private void postPicklistWithQuantities(String reqItemId, Map quantitiesByInventoryItemId) { + def picklistItems = quantitiesByInventoryItemId.collect { id, qty -> + [inventoryItem: [id: id], binLocation: [id: 'bin-1'], quantityPicked: qty] + } + request.method = 'POST' + request.contentType = 'application/json' + request.json = [picklistItems: picklistItems, reasonCode: ''] + params.id = reqItemId + withRequest(controller: 'stockMovementItemApi', action: 'updatePicklist') + } + + def "matches stockMovementItemApi.updatePicklist"() { + when: + withRequest(controller: 'stockMovementItemApi', action: 'updatePicklist') + + then: + interceptor.doesMatch() + } + + def "does not match other actions on the same controller"() { + when: + withRequest(controller: 'stockMovementItemApi', action: 'createPicklist') + + then: + !interceptor.doesMatch() + } + + def "rejects an expired lot for STOCK_MOVEMENT with HTTP 400 and the expected errorCode"() { + given: + stubParentStockMovement(StockMovementType.STOCK_MOVEMENT) + postPicklist(REQUISITION_ITEM_ID, [EXPIRED_LOT_ID]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == false + response.status == 400 + response.json.errorCode == OutboundExpiryGuardInterceptor.ERROR_CODE + response.json.errorMessages.size() == 1 + } + + def "allows a fresh-only payload for STOCK_MOVEMENT"() { + given: + stubParentStockMovement(StockMovementType.STOCK_MOVEMENT) + postPicklist(REQUISITION_ITEM_ID, [FRESH_LOT_ID]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == true + } + + def "allows null-expirationDate lots"() { + given: + stubParentStockMovement(StockMovementType.STOCK_MOVEMENT) + postPicklist(REQUISITION_ITEM_ID, [NULL_EXPIRY_LOT_ID]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == true + } + + def "rejects payloads that mix expired and fresh lots"() { + given: + stubParentStockMovement(StockMovementType.STOCK_MOVEMENT) + postPicklist(REQUISITION_ITEM_ID, [FRESH_LOT_ID, EXPIRED_LOT_ID]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == false + response.status == 400 + response.json.errorMessages.size() == 1 + } + + def "lets a RETURN_ORDER through even with an expired lot (defence-in-depth fall-through)"() { + given: + stubParentStockMovement(StockMovementType.RETURN_ORDER) + postPicklist(REQUISITION_ITEM_ID, [EXPIRED_LOT_ID]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == true + } + + def "lets the request through when picklistItems is empty (controller will reject)"() { + given: + stubParentStockMovement(StockMovementType.STOCK_MOVEMENT) + postPicklist(REQUISITION_ITEM_ID, []) + + when: + boolean proceed = interceptor.before() + + then: + proceed == true + } + + def "fails open when the requisitionItem id does not resolve"() { + given: + stubParentStockMovement(StockMovementType.STOCK_MOVEMENT) + postPicklist(MISSING_ID, [EXPIRED_LOT_ID]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == true + } + + def "ignores expired lots in the payload that are not actually being picked (#scenario)"() { + given: + stubParentStockMovement(StockMovementType.STOCK_MOVEMENT) + postPicklistWithQuantities(REQUISITION_ITEM_ID, [ + (FRESH_LOT_ID) : 13, + (EXPIRED_LOT_ID): notPickedValue, + ]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == true + + where: + scenario | notPickedValue + 'empty string' | '' + 'whitespace' | ' ' + 'zero integer' | 0 + 'zero string' | '0' + 'null' | null + 'negative integer' | -1 + } + + def "still rejects when an expired lot is being picked alongside a fresh one"() { + given: + stubParentStockMovement(StockMovementType.STOCK_MOVEMENT) + postPicklistWithQuantities(REQUISITION_ITEM_ID, [ + (FRESH_LOT_ID) : 5, + (EXPIRED_LOT_ID): 3, + ]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == false + response.status == 400 + response.json.errorCode == OutboundExpiryGuardInterceptor.ERROR_CODE + } + + def "lets the request through when an inventoryItem.id is unknown to the DB (executeQuery returns fewer rows than requested)"() { + given: + stubParentStockMovement(StockMovementType.STOCK_MOVEMENT) + postPicklist(REQUISITION_ITEM_ID, ['lot-unknown-1', FRESH_LOT_ID]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == true + } + + def "still rejects when the payload mixes a known-expired lot and an unknown id"() { + given: + stubParentStockMovement(StockMovementType.STOCK_MOVEMENT) + postPicklist(REQUISITION_ITEM_ID, ['lot-unknown-1', EXPIRED_LOT_ID]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == false + response.status == 400 + response.json.errorCode == OutboundExpiryGuardInterceptor.ERROR_CODE + } + + def "still applies the expiry check when the parent traversal returns null (no RETURN_ORDER fall-through)"() { + given: + stubMissingStockMovement() + postPicklist(REQUISITION_ITEM_ID, [EXPIRED_LOT_ID]) + + when: + boolean proceed = interceptor.before() + + then: + proceed == false + response.status == 400 + response.json.errorCode == OutboundExpiryGuardInterceptor.ERROR_CODE + } +} diff --git a/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/StockMovementServiceQuantityPickableSpec.groovy b/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/StockMovementServiceQuantityPickableSpec.groovy new file mode 100644 index 00000000000..c1d0601b13a --- /dev/null +++ b/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/StockMovementServiceQuantityPickableSpec.groovy @@ -0,0 +1,98 @@ +package org.pih.warehouse.custom.outboundExpiryRestrictions + +import org.pih.warehouse.api.AvailableItem +import org.pih.warehouse.custom.outboundExpiryRestrictions.support.ExpiryRule +import org.pih.warehouse.inventory.InventoryItem +import spock.lang.Specification + +class StockMovementServiceQuantityPickableSpec extends Specification { + + private static AvailableItem item(int qty, Date expirationDate) { + new AvailableItem( + inventoryItem: new InventoryItem(expirationDate: expirationDate), + quantityAvailable: qty, + quantityOnHand: qty, + ) + } + + private static Date daysFromToday(int delta) { + return new Date().clearTime() + delta + } + + def "all expired items return 0"() { + given: + List items = [ + item(10, daysFromToday(-1)), + item(20, daysFromToday(-30)), + ] + + expect: + ExpiryRule.sumPickableQuantity(items, new Date().clearTime()) == 0 + } + + def "all fresh items sum normally"() { + given: + List items = [ + item(10, daysFromToday(30)), + item(20, daysFromToday(60)), + ] + + expect: + ExpiryRule.sumPickableQuantity(items, new Date().clearTime()) == 30 + } + + def "mixed items sum only the fresh ones"() { + given: + List items = [ + item(10, daysFromToday(-1)), + item(20, daysFromToday(30)), + item(50, daysFromToday(-365)), + ] + + expect: + ExpiryRule.sumPickableQuantity(items, new Date().clearTime()) == 20 + } + + def "null expirationDate counts as pickable"() { + given: + List items = [ + item(10, null), + item(20, daysFromToday(30)), + ] + + expect: + ExpiryRule.sumPickableQuantity(items, new Date().clearTime()) == 30 + } + + def "today's date counts as pickable (strict-< semantics)"() { + given: + List items = [ + item(15, daysFromToday(0)), + item(25, daysFromToday(-1)), + ] + + expect: + ExpiryRule.sumPickableQuantity(items, new Date().clearTime()) == 15 + } + + def "zero-quantity items are excluded even when fresh"() { + given: + List items = [ + item(0, daysFromToday(30)), + item(7, daysFromToday(30)), + ] + + expect: + ExpiryRule.sumPickableQuantity(items, new Date().clearTime()) == 7 + } + + def "empty list returns 0"() { + expect: + ExpiryRule.sumPickableQuantity([], new Date().clearTime()) == 0 + } + + def "null list returns 0"() { + expect: + ExpiryRule.sumPickableQuantity(null, new Date().clearTime()) == 0 + } +} diff --git a/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/service/StockMovementServiceWithExpiryFilterSpec.groovy b/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/service/StockMovementServiceWithExpiryFilterSpec.groovy new file mode 100644 index 00000000000..3d9e7bf39b3 --- /dev/null +++ b/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/service/StockMovementServiceWithExpiryFilterSpec.groovy @@ -0,0 +1,83 @@ +package org.pih.warehouse.custom.outboundExpiryRestrictions.service + +import org.pih.warehouse.api.AvailableItem +import org.pih.warehouse.inventory.InventoryItem +import spock.lang.Specification + +class StockMovementServiceWithExpiryFilterSpec extends Specification { + + private static AvailableItem itemWithExpiry(Date expirationDate) { + new AvailableItem( + inventoryItem: new InventoryItem(expirationDate: expirationDate), + quantityAvailable: 10, + quantityOnHand: 10, + ) + } + + private static final Date TODAY = new Date().clearTime() + + private static Date daysFromToday(int days) { + return TODAY + days + } + + def "filterExpired drops items with past expirationDate"() { + given: + AvailableItem expired = itemWithExpiry(daysFromToday(-30)) + AvailableItem fresh = itemWithExpiry(daysFromToday(30)) + + when: + List result = StockMovementServiceWithExpiryFilter.filterExpired([expired, fresh], TODAY) + + then: + result == [fresh] + } + + def "filterExpired keeps items with null expirationDate"() { + given: + AvailableItem nullExpiry = itemWithExpiry(null) + + when: + List result = StockMovementServiceWithExpiryFilter.filterExpired([nullExpiry], TODAY) + + then: + result == [nullExpiry] + } + + def "filterExpired keeps items expiring today (strict-< matches ProductAvailabilityService:555)"() { + given: + AvailableItem expiringToday = itemWithExpiry(daysFromToday(0)) + + when: + List result = StockMovementServiceWithExpiryFilter.filterExpired([expiringToday], TODAY) + + then: + result == [expiringToday] + } + + def "filterExpired returns empty list when every item is expired"() { + given: + List input = [ + itemWithExpiry(daysFromToday(-1)), + itemWithExpiry(daysFromToday(-365)), + ] + + when: + List result = StockMovementServiceWithExpiryFilter.filterExpired(input, TODAY) + + then: + result == [] + } + + def "filterExpired preserves order of fresh items"() { + given: + AvailableItem fresh1 = itemWithExpiry(daysFromToday(10)) + AvailableItem expired = itemWithExpiry(daysFromToday(-10)) + AvailableItem fresh2 = itemWithExpiry(daysFromToday(20)) + + when: + List result = StockMovementServiceWithExpiryFilter.filterExpired([fresh1, expired, fresh2], TODAY) + + then: + result == [fresh1, fresh2] + } +} diff --git a/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRuleSpec.groovy b/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRuleSpec.groovy new file mode 100644 index 00000000000..f12f1d493db --- /dev/null +++ b/src/test/groovy/org/pih/warehouse/custom/outboundExpiryRestrictions/support/ExpiryRuleSpec.groovy @@ -0,0 +1,35 @@ +package org.pih.warehouse.custom.outboundExpiryRestrictions.support + +import spock.lang.Specification +import spock.lang.Unroll + +@Unroll +class ExpiryRuleSpec extends Specification { + + private static Date today() { + return new Date().clearTime() + } + + private static Date daysFromToday(int delta) { + return today() + delta + } + + def "isExpired returns false when expirationDate is null"() { + expect: + ExpiryRule.isExpired(null, today()) == false + } + + def "isExpired returns #expected for delta=#delta days from today"() { + expect: + ExpiryRule.isExpired(daysFromToday(delta), today()) == expected + + where: + delta || expected + -365 || true + -7 || true + -1 || true + 0 || false + 1 || false + 365 || false + } +}