TJK: Block expired stock on outbound stock movements#3
Open
gqcorneby wants to merge 10 commits intorelease/est/tjk/0.9.7from
Open
TJK: Block expired stock on outbound stock movements#3gqcorneby wants to merge 10 commits intorelease/est/tjk/0.9.7from
gqcorneby wants to merge 10 commits intorelease/est/tjk/0.9.7from
Conversation
Implements the `block-expired-items-on-outbound` OpenSpec change. Three layers, each isolated under `org.pih.warehouse.custom.outboundExpiryRestrictions` or `src/js/custom/outboundExpiryRestrictions`: - Server guard: `OutboundExpiryGuardInterceptor` matches `stockMovementItemApi.updatePicklist` and rejects payloads referencing expired `InventoryItem`s with HTTP 400 + i18n error code. Lets RETURN_ORDER flows through (defence-in-depth, never reachable in current schema). - Autopick filter: `StockMovementServiceWithExpiryFilter` extends the upstream `StockMovementService` and overrides `getSuggestedItems` to drop expired `AvailableItem`s before delegation. Wired via bean override in `resources.groovy` so virtual dispatch reaches the override (proxy AOP cannot intercept the upstream self-call from `createPicklist`). - UI: `ExpiryAwareEditPickModal` clones the upstream `EditPickModal` and augments two `getDynamicAttr` hooks to disable expired rows and surface a tooltip; helpers live in `utils/expiryHelpers.js` for unit testing. Strict-< comparison against `today` matches upstream `ProductAvailabilityService:555`. Helper centralised in `ExpiryRule`. Upstream touches (surgical, documented in design.md): - `src/js/components/stock-movement-wizard/outbound/PickPage.jsx` — 2-line swap of `EditPickModal` import to the custom wrapper. - `grails-app/conf/spring/resources.groovy` — 5-line bean override. - `webpack.config.js` — 1-line `custom` alias. - `crowdin.yml` — 2-line glob for `grails-app/i18n/custom/*-messages.properties`. Tests: 22 Spock features (`ExpiryRuleSpec`, `OutboundExpiryGuardInterceptorSpec`, `StockMovementServiceWithExpiryFilterSpec`) + Jest helper tests. All green. Edit-step `quantityAvailable` still inflates with expired stock — separate follow-up OpenSpec change to make the existing `lowerQty` validator fire correctly under the new picking rules.
Implements OpenSpec change `expiry-aware-edit-availability`. The Edit-step `lowerQty` validator and Available-cell styling now compare against a new `quantityPickable` field — the sum of `availableItems` quantities whose lots are not expired per `ExpiryRule`. The Available column inline-renders the breakdown (`33 (50 expired)`) when the totals diverge, with the parenthesised tail in red. Backend - `StockMovementService.buildEditPageItems` ships a new `quantityPickable` entry by delegating to a new static helper `sumPickableQuantity` on `StockMovementServiceWithExpiryFilter`. Hoisting `today` once inside the helper avoids the per-item `Date` allocation that would otherwise occur via the 1-arg `ExpiryRule.isExpired` overload. - `quantityAvailable` semantics unchanged across all other endpoints. Frontend - `outbound/EditPage.jsx` and `request/EditPage.jsx` swap the validator and row/cell `text-danger` styling from `quantityAvailable` to `quantityPickable`. - New helper `renderAvailableCell(translate)` in `custom/outboundExpiryRestrictions/utils/expiryHelpers.jsx` (renamed from `.js` since the file now contains JSX) returns a `formatValue` function rendering the breakdown. - New i18n key `outboundExpiryRestrictions.edit.expiredHint`. Upstream touches (surgical, documented in design.md): 1 import + 1 expression + 1 map entry on `StockMovementService.groovy`; field-config swaps and a one-line validator change on the two `EditPage.jsx` files. No schema changes, no `metaclass` mutation, no impact on substitution rows. Tests: 8 Spock features in `StockMovementServiceQuantityPickableSpec` exercising `sumPickableQuantity` directly + 11 new Jest cases for `renderAvailableCell`. All green. ESLint clean.
`OutboundExpiryGuardInterceptor` was rejecting any payload whose `picklistItems[]` referenced an expired `InventoryItem`, regardless of whether that item was actually being picked. The Pick modal sends every `availableItem` in the payload (so the API can record un-picks), so a product whose origin had a stale expired lot would fail to save even when the user only picked from fresh lots. Narrow the predicate to consider only items with `parsePickedQuantity(quantityPicked) > 0`. Empty string, whitespace, null, zero, and negative values all evaluate to "not picking" and are skipped. Mixed payloads where the user IS picking from an expired lot alongside a fresh one still get rejected — covered by a new spec case. Tests: 6 data-driven rows in `OutboundExpiryGuardInterceptorSpec` covering the not-picked sentinels (`''`, `' '`, `0`, `'0'`, `null`, `-1`) plus a regression test that defence-in-depth still rejects actively-picked expired lots. Surfaced during manual validation of `expiry-aware-edit-availability`.
The interceptor was passing a formatted English template as the `defaultMessage` argument to `messageSource.getMessage(...)`, duplicating the localised template in `outboundExpiryRestrictions-messages.properties`. That meant an English string lived in source code and could drift from the .properties file, and Crowdin only ever extracted the .properties copy. Replace the formatted fallback with a generic non-interpolated safety net (`Expired stock cannot be picked.`). The .properties bundle is now the single source of truth for the formatted user-facing message; the fallback only renders if the bundle is missing entirely, which is a catastrophic case never seen in normal operation.
- Replace Integer.parseInt with BigDecimal.signum in the picklist guard so fractional quantityPicked values cannot bypass the expired-lot check (upstream parses with new BigDecimal(...) then .intValueExact(), which crashes after clearPicklist() runs and wipes legitimate picks). - Move sumPickableQuantity from StockMovementServiceWithExpiryFilter to ExpiryRule so the upstream StockMovementService import depends on a leaf helper, not on the bean-replacement subclass. - Drop the unused single-arg ExpiryRule.isExpired(Date) overload. - Add a fork-header comment to ExpiryAwareEditPickModal naming the upstream source and the augmentation seams. - Document fail-closed behavior on null parent traversal in design.md and add an inline matrix comment in OutboundExpiryGuardInterceptor.before().
gqcorneby
changed the title
- ExpiryRule.sumPickableQuantity now takes `today` as a parameter instead of allocating a new Date per call. StockMovementService.buildEditPageItems hoists one Date outside the .collect loop, so a 20-row Edit page allocates one Date instead of twenty. - Tighten the BigDecimal-parser and parent-traversal comments in OutboundExpiryGuardInterceptor to drop narration and rot-prone line refs.
gqcorneby
changed the title
gqcorneby
force-pushed
the
feature/strict-expired-handling
branch
from
ec84bef to
441a485
Compare
- i18n: merge custom keys into root messages.properties so Grails 3.3's PluginAwareResourceBundleMessageSource actually loads them; drop the unreachable grails-app/i18n/custom/ bundle and the now-unneeded crowdin.yml glob - service: drop class-level @transactional from StockMovementServiceWithExpiryFilter to avoid mixed-namespace proxy selection; inject today into filterExpired - interceptor: set order = LOWEST_PRECEDENCE so SecurityInterceptor runs first; add specs for unknown inventoryItem.id (fall-through and mixed) - frontend: pickable null-guard in outbound + request EditPage row attrs; add aria-label/aria-disabled to the disabled picked input so screen readers announce the blocked-row reason; replace deprecated componentWillReceiveProps with getDerivedStateFromProps; memoize renderAvailableCell per translate; drop hardcoded en-US locale - tests: rename expiryHelpers.test.jsx -> .test.js, swap RTL render for renderToStaticMarkup - docs: correct D3 step 2, isExpired/sumPickableQuantity signatures, and the i18n upstream touch-point in both design.md files
…are-edit-availability Both changes are artifact-complete and ship from feature/strict-expired-handling (PR #3, open against release/est/tjk/0.9.7). Promote each change's delta spec to openspec/specs/<capability>/ and add a Deploy status block to each design.md noting PR #3 is the deployment vehicle and TJK is the only target branch so far. The four still-unchecked tasks across the two changes are post-merge items (open PR — done, manual QA, archive — being done now, fill Deploy status — done in this commit).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 References
Purpose
📝 Implementation
OutboundExpiryGuardInterceptor) underorg.pih.warehouse.custom.outboundExpiryRestrictions.*ExpiryRule(predicate +sumPickableQuantityaggregation) andStockMovementServiceWithExpiryFilter(autopick filter, registered as astockMovementServicebean override)ExpiryAwareEditPickModalundersrc/js/custom/outboundExpiryRestrictions/(forked from upstreamEditPickModalbecause itsFIELDSobject is module-scoped)POST /api/stockMovementItems/{id}/updatePicklistreturning HTTP 400 with i18nerrorCode: outboundExpiryRestrictions.expired.cannotShipquantityPickablefield on the Edit-step JSON response so the existinglowerQtyvalidator (react.stockMovement.errors.lowerQty.label) compares against non-expired stock totalsgrails-app/i18n/custom/outboundExpiryRestrictions-messages.properties+crowdin.ymlglob for translator pickup✨ Description of Change
Description:
Strict expired-stock handling on outbound
STOCK_MOVEMENTflows. Four-layer enforcement so the user cannot reach an expired lot whether they go through the UI, autopick, the API, or the Edit-step preflight.Key design decisions:
STOCK_MOVEMENTonly: outbound returns (RETURN_ORDER), inbound, replenishment, stock transfers — all unaffected. Expired stock can still be shipped back to a supplier or moved bin-to-bin.custom/paths; upstream files have surgical, documented edits (see design.md upstream touch points inopenspec/changes/block-expired-items-on-outbound/andopenspec/changes/expiry-aware-edit-availability/).<against today (midnight): a lot expiring today is still pickable today.📹 Screenshots/Screen capture
TODO before merge: capture Pick modal with disabled expired row + tooltip; Edit page Available cell showing
33 (50 expired).🔥 Notes to the tester
To run
Setup: pick a location with at least one product that has both an expired lot and a fresh lot of the same item.
<pickable> (<expired> expired)with the parenthesised tail in red. Row is bold red, "Revise quantity!" message fires on Save.text-disabled), thequantityPickedinput rejects focus, hover shows "Cannot ship — expired on …".outbound_expiry_autopick_filter dropped=N of=Mconfirms.curl -X POST .../api/stockMovementItems/<id>/updatePicklist -d '{"picklistItems":[{"inventoryItem":{"id":"<expired-lot>"},"binLocation":{"id":"<bin>"},"quantityPicked":1}],"reasonCode":""}'returns HTTP 400 witherrorCode: outboundExpiryRestrictions.expired.cannotShip.Returns → Outbound Returns) — expired lots render normally and can be added to the return.