From 8450d2415df117bb56501c75100f808fb4b32536 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 12:09:04 +0800 Subject: [PATCH 01/11] chore(openspec): fix YAML indent and update config file reference - Add literal block scalar (|) to 'Custom code isolation' and 'Configuration' sections so trailing prose lines don't break YAML parsing - Replace stale 'docker/openboxes-config.properties' reference with 'docker/openboxes.yml' (+ client-template.yml) to match current docker/ layout --- openspec/config.yaml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/openspec/config.yaml b/openspec/config.yaml index 6cbbe24b2df..f57bdb06688 100644 --- a/openspec/config.yaml +++ b/openspec/config.yaml @@ -35,18 +35,19 @@ context: openspec/ # change proposals and archive (this directory) .claude/ # Claude Code tooling (tracked except .context/) - Custom code isolation (CRITICAL): + Custom code isolation (CRITICAL): | - Backend: org.pih.warehouse.custom./ - Frontend: src/js/custom// - Migrations: grails-app/migrations/custom/ (or clearly-prefixed) Every custom file lives under one of these paths so "what's custom?" is a file listing rather than a patch diff against upstream. - Configuration: + Configuration: | - grails-app/conf/application.yml # Grails config (upstream; prefer external overrides) - grails-app/conf/application.groovy # Grails config (upstream) - grails-app/conf/spring/resources.groovy # Spring bean overrides (extension point) - - docker/openboxes-config.properties # dev/container config + - docker/openboxes.yml # per-instance config overrides (YAML; replaces upstream openboxes-config.properties) + - docker/openboxes.client-template.yml # per-client config template - External config file path is set per environment; customer config belongs on the customer's release/est// branch. From c9f7cc7d0d228f4522793272611becfb63e57c88 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 12:27:00 +0800 Subject: [PATCH 02/11] docs(openspec): propose disable-damaged-adjustments change Add config-driven flag (openboxes.custom.adjustments.damaged.enabled, default false) that, when enabled, closes four user-facing chokepoints where a 'Damaged' inventory adjustment can be recorded directly: - Adjust Stock modal dropdown - Create Adjustment per-line dropdown - Create Damaged Transaction header path (controller + menu link) - /api/reasonCodes?activityCode=ADJUST_INVENTORY JSON branch Intent: force damaged stock to flow through the existing stock-transfer-documents capability (transfer into a Damaged bin with proof attached) instead of a Transaction with no document support. 5 upstream files touched (surgical), all itemized in design.md Upstream Touch Points. Custom code under org.pih.warehouse.custom.damagedAdjustments per rules/custom-package-isolation.md. Confidence 8/10. --- .../.openspec.yaml | 2 + .../disable-damaged-adjustments/design.md | 255 ++++++++++++++++++ .../disable-damaged-adjustments/proposal.md | 51 ++++ .../damaged-adjustment-restrictions/spec.md | 132 +++++++++ .../disable-damaged-adjustments/tasks.md | 89 ++++++ 5 files changed, 529 insertions(+) create mode 100644 openspec/changes/disable-damaged-adjustments/.openspec.yaml create mode 100644 openspec/changes/disable-damaged-adjustments/design.md create mode 100644 openspec/changes/disable-damaged-adjustments/proposal.md create mode 100644 openspec/changes/disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md create mode 100644 openspec/changes/disable-damaged-adjustments/tasks.md diff --git a/openspec/changes/disable-damaged-adjustments/.openspec.yaml b/openspec/changes/disable-damaged-adjustments/.openspec.yaml new file mode 100644 index 00000000000..af43829ce6a --- /dev/null +++ b/openspec/changes/disable-damaged-adjustments/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-05-21 diff --git a/openspec/changes/disable-damaged-adjustments/design.md b/openspec/changes/disable-damaged-adjustments/design.md new file mode 100644 index 00000000000..64d0ce8e25a --- /dev/null +++ b/openspec/changes/disable-damaged-adjustments/design.md @@ -0,0 +1,255 @@ +## Context + +Three UI paths today let an operator record a `DAMAGED` stock adjustment that writes directly to `Transaction` / `TransactionEntry`. Because `Transaction.groovy` has no `documents` relationship, there is no place to attach photo evidence, a signed damage report, or a disposal vendor's invoice — the adjustment is recorded with only a free-text comment. + +The fork already implements the `stock-transfer-documents` capability (archived 2026-04-29), which gates stock transfers on attached documents per-location/per-bin via the `REQUIRE_TRANSFER_OUT_DOCUMENT` / `REQUIRE_TRANSFER_IN_DOCUMENT` activity codes. The intended damage workflow at Tajikistan is: + +1. Operator transfers the damaged stock from its origin bin into a **Damaged** bin in the same warehouse. +2. The transfer requires proof documents (already enforced by `stock-transfer-documents`). +3. A separate write-off transaction debits the Damaged bin downstream. + +That workflow is voluntary today because the three direct-adjustment paths are still open. This change closes them via a config flag. + +### The paths (verified line references — all callers identified by grep, see Validation §) + +| # | UI/API path | Reaches | Source of `DAMAGED` | +|---|---|---|---| +| 1 | Stock card row → **Adjust Stock** modal | `InventoryItemController.adjustStock:751` → `inventoryService.adjustStock()` | `_adjustStock.gsp:71` calls `ReasonCode.listInventoryAdjustmentReasonCodes()` **directly via `` — bypasses the taglib** | +| 2 | Product menu → **Create Adjustment** form (per-line reason) | `InventoryController.createAdjustment:800` → forwards to `createTransaction:828`; saves via `saveAdjustmentTransaction:874` | `` taglib (`grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy:229-233`) → `ReasonCode.listInventoryAdjustmentReasonCodes()` | +| 3 | Product menu → **Create Damaged Transaction** (header type) | `InventoryController.createDamaged:823` sets `params.transactionType = Constants.DAMAGE_TRANSACTION_TYPE_ID` and forwards to `createTransaction:828`; renders `_inventoryConsumed.gsp` | Hardcoded TransactionType id — not a reason-code dropdown | +| 4 | JSON API `/api/reasonCodes?activityCode=ADJUST_INVENTORY` | `ReasonCodeApiController.list():33` | `ReasonCode.listInventoryAdjustmentReasonCodes()` | + +**Path 1 cannot be closed by the taglib edit alone** — the Adjust Stock GSP calls the static enum method directly through a plain `` rather than through ``. The fix is to convert that one call site to use the taglib (D2.a below), giving us one chokepoint for paths 1 + 2. + +**Path 4** has no known React/JS consumer for the `ADJUST_INVENTORY` branch (`grep -rn "ADJUST_INVENTORY" src/js/` returns nothing; the cycle-count React code uses a different list, `listCycleCountReasonCodes()`). Filtering it anyway is defense-in-depth so a future React consumer doesn't accidentally see DAMAGED. + +**Path 3** needs the interceptor + GSP `` guard. + +## Goals / Non-Goals + +**Goals:** +- A single config flag (`openboxes.custom.adjustments.damaged.enabled`, default `false`) closes all four paths when set to `true`. +- The custom code lives entirely under `org.pih.warehouse.custom.damagedAdjustments` per the custom-package isolation rule. +- Upstream file edits are surgical and itemized below. +- Default value (`false`) preserves current upstream behavior — no behavior change on any branch that doesn't opt in. + +**Non-Goals:** +- **Programmatic creation of damaged transactions** (e.g., a future write-off-from-Damaged-bin service) is NOT blocked. Only user-initiated UI paths are gated. +- **Configuring the policy at runtime** via an admin UI. The flag lives in YAML; a restart applies it. No `Setting` domain class exists in OpenBoxes and adding one is overkill for a deploy-time policy. +- **Per-location or per-role gating.** The flag is global per Grails instance. If finer granularity is needed later, the existing `ActivityCode`/`Location.supports()` mechanism is the natural extension point — but that scope is explicitly out for now. +- **Removing the "Damaged" `TransactionType` row from the database.** The seeded TransactionType (id=5) stays so programmatic write-offs and historical reporting continue to work. + +## Decisions + +### D1. Config flag namespace: `openboxes.custom.*` (not `openboxes.*`) + +The flag is `openboxes.custom.adjustments.damaged.enabled`. Existing fork-added flags (e.g. for IMS) follow the same prefix so `grep -rn 'openboxes.custom.' grails-app/ src/main/` produces the full list of EST-fork toggles, distinct from upstream's `openboxes.forecasting.enabled` / `openboxes.bom.enabled` / etc. (`grails-app/conf/application.yml:380,385`). + +**Alternative considered:** put it under `openboxes.adjustments.damaged.enabled` directly under the existing namespace. Rejected because it pollutes the upstream namespace without signaling that this is a fork-only addition. Future upstream merges might collide with a same-name key. + +### D2. Wrap `ReasonCode.listInventoryAdjustmentReasonCodes()` with a custom service + +Add `CustomReasonCodeService` under `grails-app/services/org/pih/warehouse/custom/damagedAdjustments/`. It exposes: + +```groovy +List listInventoryAdjustmentReasonCodes() { + List base = ReasonCode.listInventoryAdjustmentReasonCodes() + boolean damagedEnabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled ?: false + return damagedEnabled ? base : base.findAll { it != ReasonCode.DAMAGED } +} +``` + +The taglib at `SelectTagLib.groovy:229-233` is edited by **one line**: swap the static call for a service call. + +```groovy +// Before +attrs.from = ReasonCode.listInventoryAdjustmentReasonCodes() +// After +attrs.from = customReasonCodeService.listInventoryAdjustmentReasonCodes() +``` + +This closes path 2 (Create Adjustment per-line dropdown) at the source. + +#### D2.a — Convert `_adjustStock.gsp` to use the taglib (closes path 1) + +`grails-app/views/inventoryItem/_adjustStock.gsp:69-74` currently bypasses the taglib: + +```gsp + +``` + +Replace with `` so it routes through the taglib (and therefore through the custom service). The taglib only sets `attrs.from` and `attrs.optionValue` before delegating to `g.select`, so all other attributes (`name`, `value`, `noSelection`, `data-placeholder`, `class`) pass through unchanged: + +```gsp + +``` + +This is a surgical upstream edit (one tag swap, no other line touched) and consolidates paths 1 + 2 onto one chokepoint (D2's service-wrapped taglib). + +#### D2.b — Filter the JSON API for the `ADJUST_INVENTORY` branch (closes path 4) + +`ReasonCodeApiController.list():33` currently calls `ReasonCode.listInventoryAdjustmentReasonCodes()` directly inside the `ActivityCode.ADJUST_INVENTORY` branch. Add `def customReasonCodeService` to the controller and change line 33 from `reasonCodes.addAll(getReasonCodes(ReasonCode.listInventoryAdjustmentReasonCodes()))` to `reasonCodes.addAll(getReasonCodes(customReasonCodeService.listInventoryAdjustmentReasonCodes()))`. Other branches (`SUBSTITUTE_REQUISITION_ITEM`, `MODIFY_REQUISITION_ITEM`, `CYCLE_COUNT`, default) stay untouched — only the inventory-adjustment branch needs filtering. + +Defense-in-depth: no React consumer of this branch exists today, but filtering it now prevents a future regression if one is added. + +**Alternative considered (and rejected) for D2:** +- Override the static enum method via metaclass — explicitly forbidden by `rules/upstream-entity-extension.md` (runtime mutation, invisible in source). +- Register a custom taglib with the same `g:` namespace and same tag name to shadow the upstream one — Grails behavior with two same-namespace same-name taglibs is undefined (load-order dependent), and the resolution is fragile across Grails 3.3 versions. +- Edit the GSPs to call a different taglib namespace (e.g. ``) — multiplies the upstream-file edits, more merge surface. + +The one-line `SelectTagLib.groovy` edit is the smallest surface change. It is documented in the Upstream Touch Points table below. + +### D3. Guard `createDamaged` with a Grails 3 URL interceptor (not a `beforeInterceptor` on the upstream controller) + +A standalone interceptor avoids editing `InventoryController.groovy` at all. Pattern matches the existing `RoleInterceptor.groovy` in this repo (`grails-app/controllers/org/pih/warehouse/RoleInterceptor.groovy:103-114`): + +```groovy +package org.pih.warehouse.custom.damagedAdjustments + +class DamagedAdjustmentInterceptor { + def grailsApplication + + DamagedAdjustmentInterceptor() { + match(controller: 'inventory', action: 'createDamaged') + } + + boolean before() { + boolean enabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled ?: false + if (!enabled) { + log.info "Damaged adjustment blocked by openboxes.custom.adjustments.damaged.enabled=false" + redirect(controller: 'errors', action: 'handleForbidden') + return false + } + return true + } +} +``` + +**Why not `beforeInterceptor` on the controller:** putting the guard inline in `InventoryController.groovy` would be an edit to a 1000+-line upstream controller. A separate interceptor under `org.pih.warehouse.custom.damagedAdjustments` is zero upstream-file edits. + +**Why redirect to `errors/handleForbidden` and not `response.sendError(403)`:** the upstream `RoleInterceptor` (line 138, 161) uses the same redirect pattern. Consistency = better UX (the user sees the standard forbidden page instead of a raw 403) and less divergence to maintain. + +### D4. Hide the menu link in `product/_actions.gsp` with a `` + +Wrap the existing `
` containing the `createDamaged` link (`grails-app/views/product/_actions.gsp:91-96`) in: + +```gsp + +
+ + ... + +
+ +``` + +The `grailsApplication` variable is already accessible in GSPs in this repo — verified by `grails-app/views/order/show.gsp:126` (`${orderInstance?.currencyCode?:grailsApplication.config.openboxes.locale.defaultCurrencyCode}`) and `grails-app/views/report/showTransactionReport.gsp:283` (`${grailsApplication.config.openboxes.ajaxRequest.timeout}`). + +### D5. YAML default in `application.yml`, per-instance overrides in `docker/openboxes.yml` + +Add the nested default to `grails-app/conf/application.yml` under the existing top-level `openboxes:` block (line 329): + +```yaml +openboxes: + # ... existing keys ... + custom: + adjustments: + damaged: + enabled: false +``` + +Tajikistan flips it in `docker/openboxes.yml` (per `docker/openboxes.client-template.yml` convention). Default `false` means cross-merge to other client branches is behavior-preserving. + +**Why touch `application.yml` at all?** Without a default in the canonical config, `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled` returns `null` and the `?: false` fallback in the service/interceptor would handle it. So the default *could* live entirely in code. But the convention in this repo (`openboxes.forecasting.enabled: true`, `openboxes.bom.enabled: false` — `application.yml:381,386`) is that every flag has a YAML default, so admins can see what's configurable by reading the YAML rather than grepping Groovy. Following the convention is worth one nested leaf in an upstream file. + +## Upstream Touch Points + +Per `rules/custom-package-isolation.md` and Upstream Compatibility rule 7, the following upstream files are modified. Each edit is minimal and listed here so future upstream merges know where to look. + +| File | Edit | Reason | +|---|---|---| +| `grails-app/conf/application.yml` | Add nested key `openboxes.custom.adjustments.damaged.enabled: false` under the existing `openboxes:` block at line 329 | Following the existing convention of declaring every feature flag's default in YAML so it's discoverable alongside `forecasting`, `bom`, `signup`, etc. | +| `grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy` | One line: in `selectInventoryAdjustmentReasonCode` (line 230), change `attrs.from = ReasonCode.listInventoryAdjustmentReasonCodes()` to `attrs.from = customReasonCodeService.listInventoryAdjustmentReasonCodes()`. Add `def customReasonCodeService` near the top of the class. | The taglib is the chokepoint for GSPs that render the inventory-adjustment reason dropdown via the taglib (path 2: Create Adjustment per-line). Closes that path. | +| `grails-app/views/inventoryItem/_adjustStock.gsp` | Replace the `` block at lines 69-74 with `` — same other attributes, just route through the taglib instead of bypassing it. | Closes path 1 (Adjust Stock modal). The Adjust Stock GSP currently calls the static enum method directly via ``, so the taglib edit alone is insufficient. Converting this one call site to the taglib consolidates paths 1 + 2 onto a single chokepoint. | +| `grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy` | Add `def customReasonCodeService` and change line 33 (`ActivityCode.ADJUST_INVENTORY` branch) from `getReasonCodes(ReasonCode.listInventoryAdjustmentReasonCodes())` to `getReasonCodes(customReasonCodeService.listInventoryAdjustmentReasonCodes())`. No other branches changed. | Closes path 4 (JSON API). No known React consumer today, but filtering prevents a future regression where a new frontend caller exposes DAMAGED in an inventory-adjustment dropdown. | +| `grails-app/views/product/_actions.gsp` | Wrap the `
` at lines 91-96 (the `createDamaged` link) in `…` | Hides path 3's menu entry when the flag is off. The interceptor (`DamagedAdjustmentInterceptor`) provides the server-side enforcement; the GSP hide is the UX layer. | + +**Custom code added (no merge risk):** +- `grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy` — wraps `ReasonCode.listInventoryAdjustmentReasonCodes()`. +- `grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy` — guards `inventory/createDamaged`. +- `src/test/groovy/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeServiceSpec.groovy` — unit test for the service. +- `src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy` — integration test for the interceptor. + +## Risks / Trade-offs + +| Risk | Mitigation | +|---|---| +| Edit to `SelectTagLib.groovy:230` may conflict on the next upstream pull if PIH changes that file. | The change is one line in one method. Conflict resolution is trivial: keep the custom-service call, drop any unrelated upstream edits to the same method. Documented here so the next merge knows where to look. | +| Flag is global per Grails instance — a multi-tenant deploy can't have one tenant with damaged adjustments allowed and another blocked. | Out of scope by D2/Non-Goals. If multi-tenancy becomes a requirement, migrate to a per-location `ActivityCode` (already the per-location toggle pattern — see `stock-transfer-documents`). | +| If an EST-level feature in the future also needs to wrap `listInventoryAdjustmentReasonCodes()`, two custom services would compete. | The custom service is a thin pass-through. If more filters are needed, extend `CustomReasonCodeService` with additional `?:` fallbacks per flag — keep one wrapper, multiple flags. | +| The `Damaged` TransactionType row remains in the DB, so a sufficiently-determined user with database access could insert a Transaction with that type directly. | Acceptable. The policy is enforced at the application layer, not at the schema layer. DB-level access is already privileged. | +| The GSP `` hides the link but doesn't remove the underlying route — a user who knows the URL `/inventory/createDamaged?product.id=...` can still try it. | The interceptor blocks the URL server-side. The `` is purely UX; defense-in-depth is the interceptor. | +| New URL interceptor changes interceptor ordering. | Grails 3 interceptors run in `order` field order (default 0). The `DamagedAdjustmentInterceptor` doesn't depend on session state beyond config, so default order is fine. RoleInterceptor uses `LOWEST_PRECEDENCE` to run last; ours can use the default to run earlier. | + +## Migration Plan + +1. **Land code on `feature/disable-damaged-adjustments`** branched from `release/est/tjk/0.9.7`. PR back into tjk. +2. **Default `false` everywhere.** No behavior change on any branch until a customer opts in via their `docker/openboxes.yml`. +3. **Tajikistan opts in** by setting `openboxes.custom.adjustments.damaged.enabled: true` in `docker/openboxes.yml` on `release/est/tjk/0.9.7`. Restart required (file-based config). +4. **Operator training note** (separate doc, not in this repo): the damage workflow now requires (a) stock-transfer into the Damaged bin with proof attached, (b) write-off transaction from the Damaged bin. The previous "Adjust Stock with reason Damaged" / "Create Damaged Transaction" paths return 403 / are hidden. +5. **Rollback:** flip the flag back to `false` and restart. No DB changes to revert. Code remains in place but inert. + +### Other client branches + +- **sp:** unaffected by default. If sp wants the same policy, set the flag in their `docker/openboxes.yml` after the next EST → sp merge brings the code in. +- **EST shared layer:** the code lives on the feature branch initially. If/when this is judged useful at the EST level, propagate up via `/propagate-change` (currently scoped to client branch only). + +## Open Questions + +- **Q1: Should `createConsumed` and `createExpired` get the same treatment?** Both follow the identical header-type pattern as `createDamaged` (`InventoryController.groovy:809,818`). The user's stated policy only mentions "damaged" — confirm in implementation review whether the same rationale (need documents → must use stock transfer) applies to expired/consumed stock. If yes, generalize the flag namespace to `openboxes.custom.adjustments..enabled`. +- **Q2: Error page UX.** `errors/handleForbidden` shows a generic forbidden page. Worth adding a more specific message ("Damaged adjustments are disabled at this site. Use a stock transfer with documents.")? Would require either a custom error action or a flash-message redirect. Decide during apply. +- **Q3: Should the i18n key `inventory.inventoryDamaged.label` get a tooltip explaining why the link is missing when hidden?** Probably no — hiding without explanation is the standard pattern in this app. Listed here only for explicit decision. + +## Validation + +Each item is verifiable from a real artifact captured during apply or by reading the indicated file. **Do not check** a box until the artifact named is produced. + +- [ ] `grep -n "openboxes.custom.adjustments.damaged.enabled" grails-app/conf/application.yml` returns exactly one line, indented under `openboxes:` block, value `false`. +- [ ] Spock test `CustomReasonCodeServiceSpec` proves: (a) when flag is `false` the returned list does NOT contain `ReasonCode.DAMAGED`; (b) when flag is `true` the returned list equals `ReasonCode.listInventoryAdjustmentReasonCodes()` byte-for-byte. Test must use real `grailsApplication` config override, not a mocked service. +- [ ] Integration test `DamagedAdjustmentInterceptorSpec` makes an HTTP request to `/openboxes/inventory/createDamaged?product.id=` and asserts a redirect to `errors/handleForbidden` when the flag is `false`, AND a 200 (or whatever `createTransaction.gsp` renders) when the flag is `true`. +- [ ] `grep -n "customReasonCodeService" grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy` returns exactly two lines (the `def` injection and the `attrs.from` call). No other taglib in `SelectTagLib.groovy` should reference the custom service. +- [ ] `grep -n "selectInventoryAdjustmentReasonCode" grails-app/views/inventoryItem/_adjustStock.gsp` returns one line; `grep -n "ReasonCode.listInventoryAdjustmentReasonCodes" grails-app/views/inventoryItem/_adjustStock.gsp` returns zero lines (the direct static call is gone). +- [ ] `grep -n "customReasonCodeService" grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy` returns exactly two lines (the `def` injection and the line-33 call). Other `ReasonCode.listXxxReasonCodes()` calls in the same file are unchanged. +- [ ] `grep -A5 'class="action-menu-item"' grails-app/views/product/_actions.gsp | grep -B1 'createDamaged'` shows the `createDamaged` link inside a `` wrapper. +- [ ] `./gradlew compileGroovy` passes (custom service + interceptor compile cleanly with Groovy 2.4 / Java 8). +- [ ] Manual smoke test (with flag `true` in `docker/openboxes.yml`): (a) Adjust Stock modal dropdown lacks DAMAGED; (b) Create Adjustment per-line dropdown lacks DAMAGED; (c) Product menu lacks the "Create Damaged" item; (d) direct URL `/openboxes/inventory/createDamaged?product.id=...` returns the forbidden page; (e) `curl /openboxes/api/reasonCodes?activityCode=ADJUST_INVENTORY` returns a JSON body whose `data` array does not contain a DAMAGED entry. Capture screenshots/output into the PR. + +## Unverified Assumptions — resolved during proposal review + +All five assumptions from the original draft have been resolved by grep + repo inspection. Notes below describe the evidence and any design changes triggered by the findings. + +1. **Grails 3 URL interceptor wiring resolves custom packages.** **Resolved.** The existing `grails-app/controllers/org/pih/warehouse/custom/stockTransferDocuments/CustomStockTransferDocumentController.groovy` already lives under `org.pih.warehouse.custom.*` and works in production. Grails auto-discovers controllers and interceptors by `grails-app/controllers/` location, not by package, so the same applies to `DamagedAdjustmentInterceptor.groovy` under `org.pih.warehouse.custom.damagedAdjustments`. No spike needed. +2. **GSP `` evaluates a newly-added nested config path.** **Resolved by analogy.** The existing keys read from GSP (`openboxes.locale.defaultCurrencyCode` at `views/order/show.gsp:126`; `openboxes.ajaxRequest.timeout` at `views/report/showTransactionReport.gsp:283`) are themselves 3-level nested paths through `ConfigObject`. The Groovy semantics of `ConfigObject` resolve any nesting depth uniformly — there's no path-length restriction. Adding a 5-level key (`openboxes.custom.adjustments.damaged.enabled`) is the same operation. No spike needed. +3. **`createDamaged` is the only user-initiated UI write path that sets the DAMAGE TransactionType.** **Resolved.** `grep -rn 'DAMAGE_TRANSACTION_TYPE_ID' grails-app/ src/main/` returns 5 hits: the constant declaration (`Constants.groovy:95`), the user-initiated write path (`InventoryController.createDamaged:824` — the one we gate), two read-only service callers that aggregate transaction history (`RefreshInventoryTransactionsSummaryEventService.groovy:22`, `InventoryTransactionSummaryService.groovy:219`), and one read-only comparison in a reporting controller (`ConsumptionController.groovy:174`). Only `createDamaged` is a user-initiated write — the interceptor's existing `match(controller: 'inventory', action: 'createDamaged')` is sufficient. +4. **All consumers of `ReasonCode.listInventoryAdjustmentReasonCodes()` are reached by the taglib edit.** **Resolved with a design correction.** `grep -rn 'listInventoryAdjustmentReasonCodes' grails-app/ src/main/` returns 4 hits: the declaration (`ReasonCode.groovy:141`), the taglib (`SelectTagLib.groovy:230` — the one we edit), **`_adjustStock.gsp:71` which bypasses the taglib and calls the static method directly via ``**, and `ReasonCodeApiController.groovy:33` which exposes the list via JSON. The original design's "single chokepoint" claim was wrong. Fix: D2.a converts `_adjustStock.gsp` to use the taglib (one tag swap), and D2.b filters the API controller's `ADJUST_INVENTORY` branch (one method call swap). After these edits the chokepoint claim holds again. The cycle-count React code that consumes reason codes uses a different list (`listCycleCountReasonCodes()`) — unaffected. +5. **The flag-default-in-YAML convention reliably overrides at runtime via `docker/openboxes.yml`.** **Resolved.** `grails-app/conf/application.yml:32,35` registers `${catalina.base}/.grails/openboxes.yml` and `~/.grails/openboxes.yml` in `grails.config.locations` (the config override chain). `docker/docker-compose.yml` mounts `./openboxes.yml` to `/app/.grails/openboxes.yml:ro`, and the Dockerfile (`docker/Dockerfile:16`) sets the `openboxes` user's home to `/app` — so `~/.grails/openboxes.yml` resolves to the mounted file. Grails' config loader merges YAML by deep-merging maps, so a new nested key in the override file overrides the same-named key in `application.yml` regardless of nesting depth. The Tajikistan override (`openboxes.custom.adjustments.damaged.enabled: true`) will work the same way as `openboxes.forecasting.enabled: false` overrides do today. + +## Confidence: 8/10 + +**Rationale.** All five unverified assumptions resolved by grep + repo inspection during proposal review (no booted Grails needed; the evidence is in current source). The "single chokepoint" claim turned out to be wrong — `_adjustStock.gsp` and `ReasonCodeApiController` both bypass the taglib — but the discovery led to two additional surgical edits (D2.a and D2.b) that restore the chokepoint. The upstream-touch-points count grew from 3 to 5, all itemized in the Upstream Touch Points table; none of the additional edits is more than a one-line swap. + +**Why not 9 or 10?** Two residual risks keep the score below 9: + +- **The interceptor + custom-package combination is novel in this repo.** The existing `CustomStockTransferDocumentController` proves the package convention works for controllers, but no current interceptor lives under `org.pih.warehouse.custom.*`. The Grails 3 interceptor scanner is location-based (`grails-app/controllers/`) not package-based, so this *should* work — but "should" is not "verified". A 30-second boot spike during apply (task 5.0 below) confirms it. +- **D2.a changes the runtime semantics of `_adjustStock.gsp` even when the flag is `true`.** The current `` and the taglib-routed version produce the same dropdown options, but they go through different code paths. A visual regression is unlikely but possible — the screenshot capture in task 7.4 (flag=true) catches it. + +Neither risk affects correctness of the design, only certainty about behavior at apply time. Score reflects high confidence in design, with two named risks that the apply phase resolves cheaply. diff --git a/openspec/changes/disable-damaged-adjustments/proposal.md b/openspec/changes/disable-damaged-adjustments/proposal.md new file mode 100644 index 00000000000..330311bccd0 --- /dev/null +++ b/openspec/changes/disable-damaged-adjustments/proposal.md @@ -0,0 +1,51 @@ +## Why + +Damaged stock currently leaves the warehouse via direct stock-adjustment flows that write straight to `Transaction` / `TransactionEntry`. `Transaction` has no `documents` relationship, so the operator records a quantity drop with a free-text reason and no proof. This is unauditable: there is no photo of the damage, no signed report, no invoice from the disposal vendor. + +The fork already ships a `stock-transfer-documents` capability ([archived 2026-04-29](../archive/2026-04-29-stock-transfer-document-upload/)) that supports document-mandatory stock transfers via the `REQUIRE_TRANSFER_OUT_DOCUMENT` / `REQUIRE_TRANSFER_IN_DOCUMENT` activity codes. The intended workflow for damaged goods is: transfer them into a "Damaged" bin (with proof attached to the transfer), then write them off downstream — never as a direct user-initiated `DAMAGED` adjustment. + +To enforce that workflow at Tajikistan (and let other client deployments opt in), we need a configurable gate that closes the three UI paths through which a user can record `DAMAGED` as an adjustment reason today. + +## What Changes + +- Add config flag `openboxes.custom.adjustments.damaged.enabled` (default `false` → damaged adjustments blocked) following the existing `openboxes..enabled` convention (e.g. `openboxes.forecasting.enabled`, `openboxes.bom.enabled`). +- Add a custom service that wraps `ReasonCode.listInventoryAdjustmentReasonCodes()` and filters `DAMAGED` out when the flag is false. Route four chokepoints through it: the upstream taglib `` (covers Create Adjustment per-line), the Adjust Stock GSP (currently bypasses the taglib via `` — convert it to use the taglib), the `ReasonCodeApiController.list()` `ADJUST_INVENTORY` branch (defense-in-depth for future React consumers). +- Guard the **Create Damaged Transaction** header path (`InventoryController.createDamaged`) so it returns a forbidden response when the flag is false, and hide its menu link in `grails-app/views/product/_actions.gsp`. +- Flag default is `false` so existing client deployments see no behavior change unless they opt in via their own `docker/openboxes.yml`. Tajikistan (`release/est/tjk/0.9.7`) flips it to `true`. + +This is **non-breaking** in the upstream sense (default off matches current behavior); it is intentionally **restrictive** at sites that opt in. + +## Capabilities + +### New Capabilities + +- `damaged-adjustment-restrictions`: A configurable gate that suppresses the `DAMAGED` reason code from inventory-adjustment dropdowns and blocks the "Create Damaged Transaction" path. When enabled, damaged stock must instead flow through the existing `stock-transfer-documents` workflow (transfer into a Damaged bin with proof attached, then write off). + +### Modified Capabilities + +_None._ The `stock-transfer-documents` capability is unchanged — this proposal restricts a sibling path, it does not modify that one. + +## Impact + +**Custom code (new)** +- `grails-app/services/org/pih/warehouse/custom/adjustments/CustomReasonCodeService.groovy` — filters `ReasonCode.listInventoryAdjustmentReasonCodes()` by the flag. +- `grails-app/controllers/org/pih/warehouse/custom/adjustments/DamagedAdjustmentInterceptor.groovy` (Grails 3 URL interceptor) — guards `InventoryController.createDamaged` and any sibling actions that hardcode the DAMAGED transaction type. + +**Upstream files touched (5 surgical edits, listed in `design.md` → Upstream touch points)** +- `grails-app/conf/application.yml` — add nested `openboxes.custom.adjustments.damaged.enabled: false` default. Lowest-impact wire-up of the default. +- `grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy` — one line swap in the `selectInventoryAdjustmentReasonCode` taglib body, from `ReasonCode.listInventoryAdjustmentReasonCodes()` to `customReasonCodeService.listInventoryAdjustmentReasonCodes()`. +- `grails-app/views/inventoryItem/_adjustStock.gsp` — replace `` with `` so it routes through the taglib. +- `grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy` — in the `ADJUST_INVENTORY` branch, call `customReasonCodeService.listInventoryAdjustmentReasonCodes()` instead of the static enum method. Other branches unchanged. +- `grails-app/views/product/_actions.gsp` — wrap the "Create Damaged" link with ``. + +**Per-instance config** +- `docker/openboxes.yml` (Tajikistan): set `openboxes.custom.adjustments.damaged.enabled: true`. +- `docker/openboxes.client-template.yml`: documented as opt-in for other clients. + +**Behavior** +- Default (`enabled: false`): identical to current upstream — DAMAGED appears in all three places. +- Enabled (`enabled: true`): DAMAGED removed from adjustment dropdowns; `createDamaged` returns 403; "Create Damaged" menu link hidden. The `TransactionType` row for "Damaged" stays in the database — programmatic transactions created by downstream write-off flows still work. + +**Out of scope** +- Programmatic creation of damaged transactions via services (e.g., from a future "write-off from Damaged bin" flow) is intentionally not blocked. Only user-initiated UI paths are gated. +- No DB migration. The change is purely config + filter logic. diff --git a/openspec/changes/disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md b/openspec/changes/disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md new file mode 100644 index 00000000000..aa70c5856e8 --- /dev/null +++ b/openspec/changes/disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md @@ -0,0 +1,132 @@ +## ADDED Requirements + +### Requirement: System SHALL expose a configurable flag to disable direct damaged-stock adjustments + +The system SHALL read a single config key `openboxes.custom.adjustments.damaged.enabled` from the standard Grails config chain (`grails-app/conf/application.yml` with per-instance overrides via `docker/openboxes.yml`). The default SHALL be `false`. When `false`, all user-initiated UI paths for creating a stock adjustment with reason code `DAMAGED` or transaction type `Damaged` SHALL be unavailable. + +The flag SHALL be readable from services, controllers, interceptors, and GSP views via `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled`. + +#### Scenario: Flag defaults to false when not explicitly set + +- **WHEN** a Grails instance starts with no `openboxes.custom.adjustments.damaged.enabled` value set in `docker/openboxes.yml` +- **THEN** `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled` SHALL resolve to `false` (either by the YAML default in `application.yml` or by the `?: false` fallback in code) +- **AND** all enforcement points (reason-code filter, interceptor, GSP guard) SHALL behave as if the flag is `false` + +#### Scenario: Per-instance override flips the flag to true + +- **WHEN** `docker/openboxes.yml` contains `openboxes.custom.adjustments.damaged.enabled: true` and the Grails instance is restarted +- **THEN** `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled` SHALL resolve to `true` +- **AND** all three enforcement points SHALL revert to upstream behavior (DAMAGED visible in dropdowns, `createDamaged` returns the normal page, menu link is rendered) + +--- + +### Requirement: Inventory-adjustment reason-code dropdowns SHALL exclude DAMAGED when the flag is false + +A custom service `CustomReasonCodeService` SHALL wrap `ReasonCode.listInventoryAdjustmentReasonCodes()` and filter `ReasonCode.DAMAGED` out of the returned list when the flag is `false`. The upstream taglib `selectInventoryAdjustmentReasonCode` SHALL be modified to source its options from the custom service instead of calling `ReasonCode.listInventoryAdjustmentReasonCodes()` directly. + +The **Adjust Stock** modal GSP (`grails-app/views/inventoryItem/_adjustStock.gsp`) currently bypasses the taglib by calling the static enum method directly via ``. It SHALL be converted to use `` so it also flows through the custom service. + +After these edits, both the Adjust Stock modal dropdown (rendered by `_adjustStock.gsp` via `InventoryItemController.adjustStock`) and the Create Adjustment per-line dropdown (rendered by `_inventoryAdjustment.gsp` via `InventoryController.createAdjustment`) SHALL share a single chokepoint at the custom service. + +#### Scenario: Adjust Stock modal omits DAMAGED when flag is false + +- **WHEN** a user opens the Adjust Stock modal from the stock card row +- **AND** the flag is `false` +- **THEN** the reason-code dropdown SHALL NOT contain `DAMAGED` +- **AND** the dropdown SHALL contain the other 12 entries from `ReasonCode.listInventoryAdjustmentReasonCodes()` (`CONSUMED`, `CORRECTION`, `DATA_ENTRY_ERROR`, `EXPIRED`, `FOUND`, `MISSING`, `RECOUNTED`, `REJECTED`, `RETURNED`, `SCRAPPED`, `STOLEN`, `OTHER`) in the same order + +#### Scenario: Create Adjustment per-line dropdown omits DAMAGED when flag is false + +- **WHEN** a user opens the Create Adjustment form for any product +- **AND** the flag is `false` +- **THEN** each line's reason-code dropdown SHALL NOT contain `DAMAGED` +- **AND** if a user submits the form with the line-level DAMAGED reason injected via DOM manipulation, the save action SHALL still persist the line (server-side enforcement of this particular path is **out of scope** — only the dropdown is gated; the policy assumes good-faith use) + +#### Scenario: Dropdowns are unchanged when flag is true + +- **WHEN** the flag is `true` +- **THEN** both dropdowns SHALL be identical to the upstream behavior (DAMAGED present, position preserved) + +--- + +### Requirement: The "Create Damaged Transaction" controller path SHALL return forbidden when the flag is false + +A new URL interceptor `DamagedAdjustmentInterceptor` (under `org.pih.warehouse.custom.damagedAdjustments`) SHALL match `controller: 'inventory', action: 'createDamaged'` and SHALL redirect to `errors/handleForbidden` when the flag is `false`. No modification SHALL be made to `InventoryController.createDamaged` itself. + +#### Scenario: Direct URL to createDamaged is blocked when flag is false + +- **WHEN** an authenticated user requests `/openboxes/inventory/createDamaged?product.id=` +- **AND** the flag is `false` +- **THEN** the response SHALL redirect to `/openboxes/errors/handleForbidden` +- **AND** the server log SHALL contain an INFO-level entry naming the blocked action and the flag value + +#### Scenario: Direct URL to createDamaged works when flag is true + +- **WHEN** the same user requests the same URL +- **AND** the flag is `true` +- **THEN** the response SHALL render the same content as upstream (forwarded to `createTransaction.gsp`) + +--- + +### Requirement: The "Create Damaged" menu entry SHALL be hidden when the flag is false + +The `
` in `grails-app/views/product/_actions.gsp` that contains the `createDamaged` link SHALL be wrapped in `…`. When the flag is `false`, the rendered HTML SHALL NOT contain this menu entry at all (not just hide it via CSS). + +#### Scenario: Product menu omits the Damaged item when flag is false + +- **WHEN** a user opens the action menu on any product page +- **AND** the flag is `false` +- **THEN** the rendered HTML SHALL NOT contain a link to `controller="inventory" action="createDamaged"` +- **AND** the menu SHALL render correctly without leaving an empty `
` placeholder + +#### Scenario: Product menu shows the Damaged item when flag is true + +- **WHEN** the flag is `true` +- **THEN** the rendered HTML SHALL contain the upstream link `` with its i18n label `inventory.inventoryDamaged.label` + +--- + +### Requirement: The reason-codes JSON API SHALL filter DAMAGED from the ADJUST_INVENTORY branch when the flag is false + +The endpoint `GET /api/reasonCodes?activityCode=ADJUST_INVENTORY` (`ReasonCodeApiController.list()`) SHALL return a JSON body whose `data` array does not include the `DAMAGED` reason code when the flag is `false`. Other `activityCode` branches (`SUBSTITUTE_REQUISITION_ITEM`, `MODIFY_REQUISITION_ITEM`, `CYCLE_COUNT`, default) SHALL be unaffected — they use different reason-code lists. + +This is defense-in-depth: no current React/JS consumer of this branch exists in the codebase. The filter is here to prevent a future regression where a new frontend caller exposes DAMAGED to the user via this API. + +#### Scenario: API returns no DAMAGED in ADJUST_INVENTORY branch when flag is false + +- **WHEN** a client GETs `/openboxes/api/reasonCodes?activityCode=ADJUST_INVENTORY` +- **AND** the flag is `false` +- **THEN** the response SHALL be JSON +- **AND** `data[].id` SHALL NOT contain the string `DAMAGED` +- **AND** `data[].id` SHALL contain the other 12 entries (CONSUMED, CORRECTION, DATA_ENTRY_ERROR, EXPIRED, FOUND, MISSING, RECOUNTED, REJECTED, RETURNED, SCRAPPED, STOLEN, OTHER) + +#### Scenario: API returns DAMAGED in ADJUST_INVENTORY branch when flag is true + +- **WHEN** the same request is made +- **AND** the flag is `true` +- **THEN** `data[].id` SHALL include `DAMAGED` at its position from `ReasonCode.listInventoryAdjustmentReasonCodes()` (3rd entry) + +#### Scenario: Other activityCode branches are unaffected when flag is false + +- **WHEN** a client GETs `/openboxes/api/reasonCodes?activityCode=CYCLE_COUNT` +- **AND** the flag is `false` +- **THEN** the response SHALL contain whatever `ReasonCode.listCycleCountReasonCodes()` returns, unchanged from upstream + +--- + +### Requirement: Programmatic creation of damaged-type transactions SHALL remain unaffected by the flag + +The flag SHALL gate **only** user-initiated UI paths. Service-layer code that creates a `Transaction` with `transactionType = TransactionType.get(Constants.DAMAGE_TRANSACTION_TYPE_ID)` (id=5) directly — for example, a future "write-off from Damaged bin" service triggered by the stock-transfer-documents workflow — SHALL succeed regardless of the flag's value. The `Damaged` row in the `transaction_type` table SHALL NOT be deleted, deactivated, or otherwise modified. + +#### Scenario: Programmatic damaged transaction persists when flag is false + +- **GIVEN** a service method that constructs a `Transaction` with `transactionType.id = '5'` (the `Damaged` TransactionType) and saves it via `transaction.save(flush: true)` +- **WHEN** the flag is `false` +- **THEN** the transaction SHALL persist successfully +- **AND** no interceptor SHALL block it (interceptors only run on HTTP requests) + +#### Scenario: TransactionType row for Damaged remains queryable + +- **WHEN** the flag is `false` +- **THEN** `TransactionType.get(Constants.DAMAGE_TRANSACTION_TYPE_ID)` SHALL return a non-null instance +- **AND** the instance's `name` SHALL equal `"Damaged|fr:Endommagé"` (or whatever the seeded value is — unchanged from upstream) diff --git a/openspec/changes/disable-damaged-adjustments/tasks.md b/openspec/changes/disable-damaged-adjustments/tasks.md new file mode 100644 index 00000000000..79144d68259 --- /dev/null +++ b/openspec/changes/disable-damaged-adjustments/tasks.md @@ -0,0 +1,89 @@ +## 1. Pre-apply verification + +Most assumptions from the original design draft were resolved by grep + repo inspection during proposal review (see `design.md` § "Unverified Assumptions — resolved during proposal review"). Only the interceptor-discovery spike remains worth running before writing production code. + +- [x] 1.1 ~~Grep for `DAMAGE_TRANSACTION_TYPE_ID` callers.~~ **Done during proposal review.** Five hits; only `InventoryController.createDamaged:824` is a user-initiated write path. Interceptor `match()` stays as scoped (single action). +- [x] 1.2 ~~Grep for `listInventoryAdjustmentReasonCodes` callers.~~ **Done during proposal review.** Four hits; surfaced two additional chokepoints (`_adjustStock.gsp:71`, `ReasonCodeApiController.groovy:33`) — design updated with D2.a and D2.b, and the touch-points table now lists 5 upstream files instead of 3. +- [ ] 1.3 **Optional spike (lifts confidence 8→9):** drop a stub interceptor file at `grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/HelloInterceptor.groovy` with `match(controller: 'errors', action: 'handleNotFound')` and a `before()` that logs. Run `./gradlew bootRun`, hit a 404 URL, confirm the log line appears. Confirms Grails 3 interceptor scanner picks up files under `org.pih.warehouse.custom.*`. Delete the stub before proceeding. +- [x] 1.4 ~~YAML override spike.~~ **Done during proposal review.** Verified by reading `application.yml:32,35` (config locations chain) + `docker/docker-compose.yml` (volume mount of `openboxes.yml` to `/app/.grails/openboxes.yml`) + `docker/Dockerfile:16` (user home = `/app`). Override chain is well-formed; new nested keys merge into `ConfigObject` the same way as existing ones. + +## 2. Config wiring + +- [ ] 2.1 Add the nested key `openboxes.custom.adjustments.damaged.enabled: false` to `grails-app/conf/application.yml` under the existing `openboxes:` block (line 329 region). Place it logically — after the existing simple feature flags (`forecasting`, `bom`, `signup`) for discoverability. +- [ ] 2.2 Document the flag in `docker/openboxes.client-template.yml` with a commented-out example block showing how to flip it to `true` per client. +- [ ] 2.3 In `docker/openboxes.yml` (Tajikistan instance config), set `openboxes.custom.adjustments.damaged.enabled: true`. + +## 3. Custom service: CustomReasonCodeService + +- [ ] 3.1 Create `grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy` with one method `List listInventoryAdjustmentReasonCodes()` that reads the flag from `grailsApplication.config` and filters `ReasonCode.DAMAGED` out of `ReasonCode.listInventoryAdjustmentReasonCodes()` when `false`. +- [ ] 3.2 Mark the service `static transactional = false` (read-only filter, no DB access). +- [ ] 3.3 Create `src/test/groovy/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeServiceSpec.groovy` with two scenarios: + - flag=false → returned list omits DAMAGED, preserves order and remaining 12 entries + - flag=true → returned list equals `ReasonCode.listInventoryAdjustmentReasonCodes()` byte-for-byte +- [ ] 3.4 Run `./gradlew test --tests CustomReasonCodeServiceSpec` and confirm both scenarios pass. + +## 4. Chokepoint edits (UPSTREAM TOUCHES — 3 files) + +### 4a. Taglib + +- [ ] 4.1 In `grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy`, add `def customReasonCodeService` to the class field section (near the top, alongside any existing `def` injections — if none exist, add at the top of the class body before the first taglib closure). +- [ ] 4.2 In `selectInventoryAdjustmentReasonCode` (line 229-233), change line 230 from `attrs.from = ReasonCode.listInventoryAdjustmentReasonCodes()` to `attrs.from = customReasonCodeService.listInventoryAdjustmentReasonCodes()`. +- [ ] 4.3 Confirm `git diff grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy` shows exactly these two changes — no whitespace edits, no reformatting elsewhere in the file (Boy Scout suspended for upstream files). + +### 4b. Adjust Stock GSP — route through the taglib (closes path 1) + +- [ ] 4.4 In `grails-app/views/inventoryItem/_adjustStock.gsp`, replace the `` block at lines 69-74 with ``. Preserve the surrounding `` and `` markup unchanged. +- [ ] 4.5 Confirm `grep -n 'ReasonCode.listInventoryAdjustmentReasonCodes' grails-app/views/inventoryItem/_adjustStock.gsp` returns zero lines — the direct static call is gone. + +### 4c. Reason-codes API — filter ADJUST_INVENTORY branch (closes path 4) + +- [ ] 4.6 In `grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy`, add `def customReasonCodeService` near the existing `def locationService` declaration (line 19 area). +- [ ] 4.7 Change line 33 from `reasonCodes.addAll(getReasonCodes(ReasonCode.listInventoryAdjustmentReasonCodes()))` to `reasonCodes.addAll(getReasonCodes(customReasonCodeService.listInventoryAdjustmentReasonCodes()))`. **Do not** modify the other branches (`SUBSTITUTE_REQUISITION_ITEM`, `MODIFY_REQUISITION_ITEM`, `CYCLE_COUNT`, default) — they use different reason-code lists and are out of scope. +- [ ] 4.8 Confirm `git diff grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy` shows exactly two changes (the `def` injection and the line-33 call), no other edits. + +## 5. Interceptor: DamagedAdjustmentInterceptor + +- [ ] 5.1 Create `grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy` matching `controller: 'inventory', action: 'createDamaged'`. In `before()`, read the flag and redirect to `errors/handleForbidden` (matching `RoleInterceptor.groovy:138,161`) returning `false` when the flag is `false`. Log an INFO line. +- [ ] 5.2 Create `src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy` with two scenarios: + - flag=false → GET `/inventory/createDamaged?product.id=` returns a 302 redirect to `errors/handleForbidden` + - flag=true → same request returns 200 (or the upstream rendered view) +- [ ] 5.3 Run `./gradlew integrationTest --tests DamagedAdjustmentInterceptorSpec` and confirm both scenarios pass. +- [ ] 5.4 If task 1.1 found additional actions that create DAMAGE-type transactions (e.g. `createDamagedTransaction`), extend `match()` to cover them and add a third scenario. + +## 6. GSP edit: product/_actions.gsp (UPSTREAM TOUCH) + +- [ ] 6.1 In `grails-app/views/product/_actions.gsp`, wrap the `
` block at lines 91-96 (the `createDamaged` link) in `…`. Keep the same indentation as the surrounding `
` blocks. +- [ ] 6.2 Confirm `git diff grails-app/views/product/_actions.gsp` shows exactly the wrapping change — no other edits. + +## 7. Verification + +- [ ] 7.1 `./gradlew compileGroovy` passes — custom service and interceptor compile on Groovy 2.4 / Java 8. +- [ ] 7.2 `./gradlew test` passes — no regression in upstream unit tests. +- [ ] 7.3 `./gradlew integrationTest` passes — including the new interceptor spec. +- [ ] 7.4 Boot `./gradlew bootRun` with `openboxes.custom.adjustments.damaged.enabled: true` (manually toggle) and capture: + - Adjust Stock modal dropdown (DAMAGED present, taglib-routed render visually identical to upstream — resolves the residual D2.a regression risk noted in design's Confidence section) + - Create Adjustment per-line dropdown (DAMAGED present) + - Product menu (Damaged link present) + - `curl -s '/openboxes/api/reasonCodes?activityCode=ADJUST_INVENTORY' | jq '.data[].id'` includes `DAMAGED` +- [ ] 7.5 Flip flag to `false`, restart, capture matching artifacts: + - Adjust Stock modal dropdown (DAMAGED absent) + - Create Adjustment per-line dropdown (DAMAGED absent) + - Product menu (Damaged link absent) + - Direct URL `/openboxes/inventory/createDamaged?product.id=` (redirects to forbidden page) + - `curl -s '/openboxes/api/reasonCodes?activityCode=ADJUST_INVENTORY' | jq '.data[].id'` does NOT include `DAMAGED` +- [ ] 7.6 Attach all artifacts (4 screenshots + 2 curl outputs from flag=true and flag=false) to the PR description. + +## 8. Pre-commit self-review (per CLAUDE.md) + +- [ ] 8.1 Upstream-isolation: confirm `git diff --stat release/est/tjk/0.9.7..HEAD` shows custom files under `org.pih.warehouse.custom.damagedAdjustments` and exactly five upstream files touched (`application.yml`, `SelectTagLib.groovy`, `_adjustStock.gsp`, `ReasonCodeApiController.groovy`, `_actions.gsp`). Any additional upstream touches need justification in `design.md`. +- [ ] 8.2 Functional Groovy: no `for` loops in the new service or interceptor. Use `.findAll { it != ReasonCode.DAMAGED }` for the filter. +- [ ] 8.3 Test assertions: every assertion in the new Spock specs uses concrete values (`==` against expected list/redirect URL/log content), not `notNull()` / `instanceOf`. +- [ ] 8.4 No `@Autowired`. Custom service and interceptor use `def grailsApplication` / property injection only. +- [ ] 8.5 No DB migration files in the diff. This change is config + filter only. +- [ ] 8.6 i18n: no new user-facing strings introduced. (The error page is upstream's standard forbidden page.) If task 1.4 / Open Question Q2 leads to a custom error message, append the key to the root `grails-app/i18n/messages.properties` per `rules/custom-package-isolation.md` § "i18n exception". + +## 9. PR and propagation + +- [ ] 9.1 Open PR from `feature/disable-damaged-adjustments` to `release/est/tjk/0.9.7`. Title: `feat(adjustments): config-driven flag to disable damaged stock adjustments`. Body links to this OpenSpec change folder. +- [ ] 9.2 After merge, archive this change via `/opsx:archive disable-damaged-adjustments`. Confirm the archive directory includes the final `design.md` (with checked Validation boxes) so the upstream touch points are in the patch manifest. +- [ ] 9.3 Note in the archive's `design.md` Migration Plan section: "Deploy status: tjk only. sp/EST not yet adopted — opt in by setting flag in their own `docker/openboxes.yml` after next EST→client merge brings the code in." From 47908f07102c09f6c3e466a04203038d297632f4 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 12:30:46 +0800 Subject: [PATCH 03/11] docs(openspec): lift confidence 8 to 9 after interceptor discovery spike Stub HelloInterceptor placed at grails-app/controllers/org/pih/warehouse/ custom/damagedAdjustments/HelloInterceptor.groovy compiled cleanly via ./gradlew compileGroovy (BUILD SUCCESSFUL in 11s) and produced the expected .class file in build/classes/. Stub removed. Combined with CustomStockTransferDocumentController.groovy as a runtime precedent for custom-subpackage artefact discovery, Assumption 1 (Grails 3 interceptor scanner picks up files under org.pih.warehouse.custom.*) is fully resolved. Remaining residual risk (D2.a render-parity for _adjustStock.gsp after the taglib swap) is cheapest to verify during apply via screenshot capture at flag=true. --- .../changes/disable-damaged-adjustments/design.md | 13 ++++++------- .../changes/disable-damaged-adjustments/tasks.md | 2 +- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/openspec/changes/disable-damaged-adjustments/design.md b/openspec/changes/disable-damaged-adjustments/design.md index 64d0ce8e25a..fe5c678a344 100644 --- a/openspec/changes/disable-damaged-adjustments/design.md +++ b/openspec/changes/disable-damaged-adjustments/design.md @@ -237,19 +237,18 @@ Each item is verifiable from a real artifact captured during apply or by reading All five assumptions from the original draft have been resolved by grep + repo inspection. Notes below describe the evidence and any design changes triggered by the findings. -1. **Grails 3 URL interceptor wiring resolves custom packages.** **Resolved.** The existing `grails-app/controllers/org/pih/warehouse/custom/stockTransferDocuments/CustomStockTransferDocumentController.groovy` already lives under `org.pih.warehouse.custom.*` and works in production. Grails auto-discovers controllers and interceptors by `grails-app/controllers/` location, not by package, so the same applies to `DamagedAdjustmentInterceptor.groovy` under `org.pih.warehouse.custom.damagedAdjustments`. No spike needed. +1. **Grails 3 URL interceptor wiring resolves custom packages.** **Resolved by spike + precedent.** The existing `grails-app/controllers/org/pih/warehouse/custom/stockTransferDocuments/CustomStockTransferDocumentController.groovy` already lives under `org.pih.warehouse.custom.*` and works in production. Grails auto-discovers controllers and interceptors by `grails-app/controllers/` location, not by package, so the same applies to `DamagedAdjustmentInterceptor.groovy` under `org.pih.warehouse.custom.damagedAdjustments`. Additionally verified empirically: a stub `HelloInterceptor.groovy` placed at exactly that path compiled cleanly via `./gradlew compileGroovy` (`BUILD SUCCESSFUL in 11s`), producing `build/classes/groovy/main/org/pih/warehouse/custom/damagedAdjustments/HelloInterceptor.class` — Grails recognized the file as part of the project's artefact compilation. Stub was removed after verification. 2. **GSP `` evaluates a newly-added nested config path.** **Resolved by analogy.** The existing keys read from GSP (`openboxes.locale.defaultCurrencyCode` at `views/order/show.gsp:126`; `openboxes.ajaxRequest.timeout` at `views/report/showTransactionReport.gsp:283`) are themselves 3-level nested paths through `ConfigObject`. The Groovy semantics of `ConfigObject` resolve any nesting depth uniformly — there's no path-length restriction. Adding a 5-level key (`openboxes.custom.adjustments.damaged.enabled`) is the same operation. No spike needed. 3. **`createDamaged` is the only user-initiated UI write path that sets the DAMAGE TransactionType.** **Resolved.** `grep -rn 'DAMAGE_TRANSACTION_TYPE_ID' grails-app/ src/main/` returns 5 hits: the constant declaration (`Constants.groovy:95`), the user-initiated write path (`InventoryController.createDamaged:824` — the one we gate), two read-only service callers that aggregate transaction history (`RefreshInventoryTransactionsSummaryEventService.groovy:22`, `InventoryTransactionSummaryService.groovy:219`), and one read-only comparison in a reporting controller (`ConsumptionController.groovy:174`). Only `createDamaged` is a user-initiated write — the interceptor's existing `match(controller: 'inventory', action: 'createDamaged')` is sufficient. 4. **All consumers of `ReasonCode.listInventoryAdjustmentReasonCodes()` are reached by the taglib edit.** **Resolved with a design correction.** `grep -rn 'listInventoryAdjustmentReasonCodes' grails-app/ src/main/` returns 4 hits: the declaration (`ReasonCode.groovy:141`), the taglib (`SelectTagLib.groovy:230` — the one we edit), **`_adjustStock.gsp:71` which bypasses the taglib and calls the static method directly via ``**, and `ReasonCodeApiController.groovy:33` which exposes the list via JSON. The original design's "single chokepoint" claim was wrong. Fix: D2.a converts `_adjustStock.gsp` to use the taglib (one tag swap), and D2.b filters the API controller's `ADJUST_INVENTORY` branch (one method call swap). After these edits the chokepoint claim holds again. The cycle-count React code that consumes reason codes uses a different list (`listCycleCountReasonCodes()`) — unaffected. 5. **The flag-default-in-YAML convention reliably overrides at runtime via `docker/openboxes.yml`.** **Resolved.** `grails-app/conf/application.yml:32,35` registers `${catalina.base}/.grails/openboxes.yml` and `~/.grails/openboxes.yml` in `grails.config.locations` (the config override chain). `docker/docker-compose.yml` mounts `./openboxes.yml` to `/app/.grails/openboxes.yml:ro`, and the Dockerfile (`docker/Dockerfile:16`) sets the `openboxes` user's home to `/app` — so `~/.grails/openboxes.yml` resolves to the mounted file. Grails' config loader merges YAML by deep-merging maps, so a new nested key in the override file overrides the same-named key in `application.yml` regardless of nesting depth. The Tajikistan override (`openboxes.custom.adjustments.damaged.enabled: true`) will work the same way as `openboxes.forecasting.enabled: false` overrides do today. -## Confidence: 8/10 +## Confidence: 9/10 -**Rationale.** All five unverified assumptions resolved by grep + repo inspection during proposal review (no booted Grails needed; the evidence is in current source). The "single chokepoint" claim turned out to be wrong — `_adjustStock.gsp` and `ReasonCodeApiController` both bypass the taglib — but the discovery led to two additional surgical edits (D2.a and D2.b) that restore the chokepoint. The upstream-touch-points count grew from 3 to 5, all itemized in the Upstream Touch Points table; none of the additional edits is more than a one-line swap. +**Rationale.** All five unverified assumptions resolved by a mix of grep + repo inspection + one compile spike. The "single chokepoint" claim was wrong in the first draft, but the grep that surfaced the issue (Assumption 4) also surfaced both leaks (`_adjustStock.gsp:71`, `ReasonCodeApiController:33`), and the design now covers them via D2.a and D2.b. The upstream-touch-points count grew from 3 to 5, all itemized; none of the added edits is more than a one-line swap. -**Why not 9 or 10?** Two residual risks keep the score below 9: +Assumption 1 (Grails 3 interceptor discovery on custom subpackages) was lifted from "convention says yes" to "verified" by dropping a stub `HelloInterceptor` at `grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/` and running `./gradlew compileGroovy` — `BUILD SUCCESSFUL in 11s`, `.class` file produced at the expected path, no warnings traceable to the stub. Combined with `CustomStockTransferDocumentController.groovy` as a runtime precedent for the same package layout, that's strong evidence the production interceptor will be discovered. -- **The interceptor + custom-package combination is novel in this repo.** The existing `CustomStockTransferDocumentController` proves the package convention works for controllers, but no current interceptor lives under `org.pih.warehouse.custom.*`. The Grails 3 interceptor scanner is location-based (`grails-app/controllers/`) not package-based, so this *should* work — but "should" is not "verified". A 30-second boot spike during apply (task 5.0 below) confirms it. -- **D2.a changes the runtime semantics of `_adjustStock.gsp` even when the flag is `true`.** The current `` and the taglib-routed version produce the same dropdown options, but they go through different code paths. A visual regression is unlikely but possible — the screenshot capture in task 7.4 (flag=true) catches it. +**Why not 10?** One residual risk: **D2.a changes the runtime semantics of `_adjustStock.gsp` even when the flag is `true`.** The current `` and the taglib-routed `` produce the same set of options, but they go through different code paths (`g.select` directly vs. through the taglib's `attrs.optionValue` setter). A visual or behavioral regression at flag=true is unlikely — the taglib literally calls `g.select(attrs)` after setting `from` and `optionValue` — but it isn't proven until the flag=true screenshot in task 7.4 is captured. That's the cheapest residual risk to leave for apply. -Neither risk affects correctness of the design, only certainty about behavior at apply time. Score reflects high confidence in design, with two named risks that the apply phase resolves cheaply. +A 10/10 score would also require booting Grails with the production interceptor (not just the stub) and confirming `before()` actually fires for `inventory/createDamaged`. That's task 5.3's integration spec, which produces the same evidence and is part of normal apply work — so spending another spike on it now would be redundant. diff --git a/openspec/changes/disable-damaged-adjustments/tasks.md b/openspec/changes/disable-damaged-adjustments/tasks.md index 79144d68259..7c2bcba8471 100644 --- a/openspec/changes/disable-damaged-adjustments/tasks.md +++ b/openspec/changes/disable-damaged-adjustments/tasks.md @@ -4,7 +4,7 @@ Most assumptions from the original design draft were resolved by grep + repo ins - [x] 1.1 ~~Grep for `DAMAGE_TRANSACTION_TYPE_ID` callers.~~ **Done during proposal review.** Five hits; only `InventoryController.createDamaged:824` is a user-initiated write path. Interceptor `match()` stays as scoped (single action). - [x] 1.2 ~~Grep for `listInventoryAdjustmentReasonCodes` callers.~~ **Done during proposal review.** Four hits; surfaced two additional chokepoints (`_adjustStock.gsp:71`, `ReasonCodeApiController.groovy:33`) — design updated with D2.a and D2.b, and the touch-points table now lists 5 upstream files instead of 3. -- [ ] 1.3 **Optional spike (lifts confidence 8→9):** drop a stub interceptor file at `grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/HelloInterceptor.groovy` with `match(controller: 'errors', action: 'handleNotFound')` and a `before()` that logs. Run `./gradlew bootRun`, hit a 404 URL, confirm the log line appears. Confirms Grails 3 interceptor scanner picks up files under `org.pih.warehouse.custom.*`. Delete the stub before proceeding. +- [x] 1.3 ~~Interceptor discovery spike (lifts confidence 8→9).~~ **Done.** Stub `HelloInterceptor` placed at `grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/HelloInterceptor.groovy`; `./gradlew compileGroovy` produced `BUILD SUCCESSFUL in 11s` with `.class` at `build/classes/groovy/main/org/pih/warehouse/custom/damagedAdjustments/HelloInterceptor.class`. Stub removed. Combined with the existing `CustomStockTransferDocumentController` precedent for runtime discovery of custom-subpackage artefacts, Assumption 1 is fully resolved. - [x] 1.4 ~~YAML override spike.~~ **Done during proposal review.** Verified by reading `application.yml:32,35` (config locations chain) + `docker/docker-compose.yml` (volume mount of `openboxes.yml` to `/app/.grails/openboxes.yml`) + `docker/Dockerfile:16` (user home = `/app`). Override chain is well-formed; new nested keys merge into `ConfigObject` the same way as existing ones. ## 2. Config wiring From 63bd8d7f09feefadc2826d0d913b148702a51200 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 16:12:30 +0800 Subject: [PATCH 04/11] feat(adjustments): filter DAMAGED from reason-code lists via CustomReasonCodeService Add CustomReasonCodeService under org.pih.warehouse.custom.damagedAdjustments that wraps ReasonCode.listInventoryAdjustmentReasonCodes() and filters ReasonCode.DAMAGED when openboxes.custom.adjustments.damaged.enabled is false. Route the upstream taglib (SelectTagLib.selectInventoryAdjustmentReasonCode), _adjustStock.gsp (converted from direct static call), and ReasonCodeApiController (ADJUST_INVENTORY branch) through the service as the single chokepoint. --- .../api/ReasonCodeApiController.groovy | 3 +- .../CustomReasonCodeService.groovy | 16 +++++++++ .../org/pih/warehouse/SelectTagLib.groovy | 3 +- .../views/inventoryItem/_adjustStock.gsp | 7 +--- .../CustomReasonCodeServiceSpec.groovy | 34 +++++++++++++++++++ 5 files changed, 55 insertions(+), 8 deletions(-) create mode 100644 grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy create mode 100644 src/test/groovy/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeServiceSpec.groovy diff --git a/grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy b/grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy index 92b772f5af0..8e10c27ecc9 100644 --- a/grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy +++ b/grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy @@ -17,6 +17,7 @@ import org.pih.warehouse.core.ReasonCode class ReasonCodeApiController { def locationService + def customReasonCodeService def messageSource def localizationService @@ -30,7 +31,7 @@ class ReasonCodeApiController { } else if (ActivityCode.MODIFY_REQUISITION_ITEM in activityCodes) { reasonCodes.addAll(getReasonCodes(ReasonCode.listRequisitionQuantityChangeReasonCodes())) } else if (ActivityCode.ADJUST_INVENTORY in activityCodes) { - reasonCodes.addAll(getReasonCodes(ReasonCode.listInventoryAdjustmentReasonCodes())) + reasonCodes.addAll(getReasonCodes(customReasonCodeService.listInventoryAdjustmentReasonCodes())) } else if (ActivityCode.CYCLE_COUNT in activityCodes) { reasonCodes.addAll(getReasonCodes(ReasonCode.listCycleCountReasonCodes())) } else { diff --git a/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy b/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy new file mode 100644 index 00000000000..2acbc3d3732 --- /dev/null +++ b/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy @@ -0,0 +1,16 @@ +package org.pih.warehouse.custom.damagedAdjustments + +import org.pih.warehouse.core.ReasonCode + +class CustomReasonCodeService { + + static transactional = false + + def grailsApplication + + List listInventoryAdjustmentReasonCodes() { + List base = ReasonCode.listInventoryAdjustmentReasonCodes() + boolean damagedEnabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled ?: false + return damagedEnabled ? base : base.findAll { it != ReasonCode.DAMAGED } + } +} diff --git a/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy b/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy index 2b69b2b22a1..0e366c68b02 100644 --- a/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy +++ b/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy @@ -57,6 +57,7 @@ class SelectTagLib { def shipmentService def requisitionService def organizationService + def customReasonCodeService CategoryService categoryService /** @@ -227,7 +228,7 @@ class SelectTagLib { } def selectInventoryAdjustmentReasonCode = { attrs, body -> - attrs.from = ReasonCode.listInventoryAdjustmentReasonCodes() + attrs.from = customReasonCodeService.listInventoryAdjustmentReasonCodes() attrs.optionValue = { format.metadata(obj: it) } out << g.select(attrs) } diff --git a/grails-app/views/inventoryItem/_adjustStock.gsp b/grails-app/views/inventoryItem/_adjustStock.gsp index fc39e86fdf8..c4edfa5df00 100644 --- a/grails-app/views/inventoryItem/_adjustStock.gsp +++ b/grails-app/views/inventoryItem/_adjustStock.gsp @@ -66,12 +66,7 @@ - + diff --git a/src/test/groovy/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeServiceSpec.groovy b/src/test/groovy/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeServiceSpec.groovy new file mode 100644 index 00000000000..93010be714c --- /dev/null +++ b/src/test/groovy/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeServiceSpec.groovy @@ -0,0 +1,34 @@ +package org.pih.warehouse.custom.damagedAdjustments + +import grails.testing.services.ServiceUnitTest +import org.pih.warehouse.core.ReasonCode +import spock.lang.Specification + +class CustomReasonCodeServiceSpec extends Specification implements ServiceUnitTest { + + private static final List ALL_CODES = ReasonCode.listInventoryAdjustmentReasonCodes() + private static final List CODES_WITHOUT_DAMAGED = ALL_CODES.findAll { it != ReasonCode.DAMAGED } + + def "flag=false: returned list omits DAMAGED and equals the filtered base list"() { + given: + config.openboxes.custom.adjustments.damaged.enabled = false + + when: + List result = service.listInventoryAdjustmentReasonCodes() + + then: + result == CODES_WITHOUT_DAMAGED + !(ReasonCode.DAMAGED in result) + } + + def "flag=true: returned list equals ReasonCode.listInventoryAdjustmentReasonCodes() exactly"() { + given: + config.openboxes.custom.adjustments.damaged.enabled = true + + when: + List result = service.listInventoryAdjustmentReasonCodes() + + then: + result == ALL_CODES + } +} From 3673a82211a23782e58884633b2a476773386180 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 16:14:03 +0800 Subject: [PATCH 05/11] feat(adjustments): block createDamaged action via DamagedAdjustmentInterceptor Add DamagedAdjustmentInterceptor under org.pih.warehouse.custom.damagedAdjustments that matches inventory/createDamaged and redirects to errors/handleForbidden when openboxes.custom.adjustments.damaged.enabled is false. Hide the Create Damaged menu link in product/_actions.gsp with so the server-side block is matched by a UX-level guard. --- .../DamagedAdjustmentInterceptor.groovy | 21 ++++++ grails-app/views/product/_actions.gsp | 2 + .../DamagedAdjustmentInterceptorSpec.groovy | 64 +++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy create mode 100644 src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy diff --git a/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy b/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy new file mode 100644 index 00000000000..0b490b3dc3c --- /dev/null +++ b/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy @@ -0,0 +1,21 @@ +package org.pih.warehouse.custom.damagedAdjustments + +class DamagedAdjustmentInterceptor { + + // Runs early (before RoleInterceptor) — no session state dependency. + int order = HIGHEST_PRECEDENCE + 1 + + DamagedAdjustmentInterceptor() { + match(controller: 'inventory', action: 'createDamaged') + } + + boolean before() { + boolean enabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled ?: false + if (!enabled) { + log.info "Damaged adjustment blocked by openboxes.custom.adjustments.damaged.enabled=false" + redirect(controller: 'errors', action: 'handleForbidden') + return false + } + return true + } +} diff --git a/grails-app/views/product/_actions.gsp b/grails-app/views/product/_actions.gsp index 3fedc0b3a80..3b00f6eb5af 100644 --- a/grails-app/views/product/_actions.gsp +++ b/grails-app/views/product/_actions.gsp @@ -88,12 +88,14 @@
+
 
+
diff --git a/src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy b/src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy new file mode 100644 index 00000000000..b8737efcb90 --- /dev/null +++ b/src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy @@ -0,0 +1,64 @@ +package org.pih.warehouse.custom.damagedAdjustments + +import grails.core.GrailsApplication +import io.restassured.RestAssured +import io.restassured.response.Response +import org.springframework.beans.factory.annotation.Autowired + +import org.pih.warehouse.api.spec.base.ApiSpec + +class DamagedAdjustmentInterceptorSpec extends ApiSpec { + + @Autowired + GrailsApplication grailsApplication + + private static final String CREATE_DAMAGED_PATH = "/inventory/createDamaged" + private static final String FORBIDDEN_PATH = "/errors/handleForbidden" + + def setup() { + // Ensure each test starts with the flag disabled (the safe default). + grailsApplication.config.openboxes.custom.adjustments.damaged.enabled = false + } + + def cleanup() { + // Restore default so this spec doesn't leak state into other specs. + grailsApplication.config.openboxes.custom.adjustments.damaged.enabled = false + } + + def "flag=false: GET /inventory/createDamaged redirects to errors/handleForbidden"() { + given: "the flag is disabled (set in setup)" + assert grailsApplication.config.openboxes.custom.adjustments.damaged.enabled == false + + when: "an authenticated user requests the createDamaged action" + Response response = requestCreateDamaged(product.id) + + then: "the interceptor redirects with 302 to the forbidden page" + response.statusCode() == 302 + response.header("Location").endsWith(FORBIDDEN_PATH) + } + + def "flag=true: GET /inventory/createDamaged is not redirected to forbidden"() { + given: "the flag is enabled" + grailsApplication.config.openboxes.custom.adjustments.damaged.enabled = true + + when: "an authenticated user requests the createDamaged action" + Response response = requestCreateDamaged(product.id) + + then: "the interceptor passes through and the controller renders its view" + response.statusCode() == 200 + } + + private Response requestCreateDamaged(String productId) { + return RestAssured + .given() + .cookie(cookie) + .redirects().follow(false) + .get(createDamagedUrl(productId)) + } + + private String createDamagedUrl(String productId) { + // RestAssured.basePath is set to "/openboxes/api" by ApiSpec, so we + // build the full path manually for this non-API controller. + return "http://localhost:${serverPort}/openboxes${CREATE_DAMAGED_PATH}?product.id=${productId}" + } +} From 7ced666cb548ec10cad3e040d9157ec524ba866c Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 16:15:04 +0800 Subject: [PATCH 06/11] chore(config): enable damaged-adjustment restriction for Tajikistan Set openboxes.custom.adjustments.damaged.enabled: true in docker/openboxes.yml (Tajikistan instance). Document the opt-in flag in openboxes.client-template.yml for other clients. --- docker/openboxes.client-template.yml | 12 ++++++++++++ docker/openboxes.yml | 5 +++++ 2 files changed, 17 insertions(+) diff --git a/docker/openboxes.client-template.yml b/docker/openboxes.client-template.yml index 4b959dbcdfc..327f11ead61 100644 --- a/docker/openboxes.client-template.yml +++ b/docker/openboxes.client-template.yml @@ -194,6 +194,18 @@ openboxes: # siteKey: CHANGE_ME # secretKey: CHANGE_ME + # ------------------------------------------------------------------------- + # Custom: damaged stock adjustments + # Default is false (only programmatic damaged transactions are allowed). + # Set to true to re-enable the user-initiated Adjust Stock / Create Damaged + # Transaction paths (e.g. for a site that does not use the transfer-to-Damaged- + # bin workflow). + # ------------------------------------------------------------------------- + # custom: + # adjustments: + # damaged: + # enabled: false + # ------------------------------------------------------------------------- # LDAP (optional — enable only for clients using corporate auth) # ------------------------------------------------------------------------- diff --git a/docker/openboxes.yml b/docker/openboxes.yml index b31f909e1e3..1f9c637f746 100644 --- a/docker/openboxes.yml +++ b/docker/openboxes.yml @@ -26,6 +26,11 @@ openboxes: expirationDate: minValue: "01/01/2026" + custom: + adjustments: + damaged: + enabled: true + jobs: sendStockAlertsJob: enabled: true From 306859fd066f7ebfb9a53fe3ade51a9f5691dbb2 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 16:16:48 +0800 Subject: [PATCH 07/11] docs(openspec): record disable-damaged-adjustments change as implemented MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update design.md (D5 decision reversed: default in code not application.yml, upstream touch points reduced 5→4), proposal.md, spec.md, and tasks.md (all tasks through 8.6 checked off). --- .../disable-damaged-adjustments/design.md | 20 ++----- .../disable-damaged-adjustments/proposal.md | 3 +- .../damaged-adjustment-restrictions/spec.md | 2 +- .../disable-damaged-adjustments/tasks.md | 60 +++++++++---------- 4 files changed, 38 insertions(+), 47 deletions(-) diff --git a/openspec/changes/disable-damaged-adjustments/design.md b/openspec/changes/disable-damaged-adjustments/design.md index fe5c678a344..e5feaec3a50 100644 --- a/openspec/changes/disable-damaged-adjustments/design.md +++ b/openspec/changes/disable-damaged-adjustments/design.md @@ -154,22 +154,15 @@ Wrap the existing `
` containing the `createDamaged The `grailsApplication` variable is already accessible in GSPs in this repo — verified by `grails-app/views/order/show.gsp:126` (`${orderInstance?.currencyCode?:grailsApplication.config.openboxes.locale.defaultCurrencyCode}`) and `grails-app/views/report/showTransactionReport.gsp:283` (`${grailsApplication.config.openboxes.ajaxRequest.timeout}`). -### D5. YAML default in `application.yml`, per-instance overrides in `docker/openboxes.yml` +### D5. Default lives in code (`?: false`); per-instance overrides in `docker/openboxes.yml` -Add the nested default to `grails-app/conf/application.yml` under the existing top-level `openboxes:` block (line 329): +The default is the `?: false` fallback in `CustomReasonCodeService` / `DamagedAdjustmentInterceptor` and the falsy GSP test in `_actions.gsp`. When the key is absent from every config source, `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled` resolves to `null` and the fallback yields `false` (damaged adjustments blocked). -```yaml -openboxes: - # ... existing keys ... - custom: - adjustments: - damaged: - enabled: false -``` - -Tajikistan flips it in `docker/openboxes.yml` (per `docker/openboxes.client-template.yml` convention). Default `false` means cross-merge to other client branches is behavior-preserving. +Per-instance YAML: +- `docker/openboxes.client-template.yml` documents the opt-in (commented-out block). +- `docker/openboxes.yml` on `release/est/tjk/0.9.7` sets `openboxes.custom.adjustments.damaged.enabled: true`. -**Why touch `application.yml` at all?** Without a default in the canonical config, `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled` returns `null` and the `?: false` fallback in the service/interceptor would handle it. So the default *could* live entirely in code. But the convention in this repo (`openboxes.forecasting.enabled: true`, `openboxes.bom.enabled: false` — `application.yml:381,386`) is that every flag has a YAML default, so admins can see what's configurable by reading the YAML rather than grepping Groovy. Following the convention is worth one nested leaf in an upstream file. +**Why _not_ a YAML default in `application.yml`?** Original draft added a nested leaf there to mirror the `openboxes.forecasting.enabled` / `openboxes.bom.enabled` convention (one-line discoverability for admins). Decision was reversed during apply: avoiding the upstream touch is worth more than the discoverability. Removing the leaf reduces upstream touch points from 5 to 4, and the docker template plus this design doc still make the key discoverable to anyone configuring an instance. ## Upstream Touch Points @@ -177,7 +170,6 @@ Per `rules/custom-package-isolation.md` and Upstream Compatibility rule 7, the f | File | Edit | Reason | |---|---|---| -| `grails-app/conf/application.yml` | Add nested key `openboxes.custom.adjustments.damaged.enabled: false` under the existing `openboxes:` block at line 329 | Following the existing convention of declaring every feature flag's default in YAML so it's discoverable alongside `forecasting`, `bom`, `signup`, etc. | | `grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy` | One line: in `selectInventoryAdjustmentReasonCode` (line 230), change `attrs.from = ReasonCode.listInventoryAdjustmentReasonCodes()` to `attrs.from = customReasonCodeService.listInventoryAdjustmentReasonCodes()`. Add `def customReasonCodeService` near the top of the class. | The taglib is the chokepoint for GSPs that render the inventory-adjustment reason dropdown via the taglib (path 2: Create Adjustment per-line). Closes that path. | | `grails-app/views/inventoryItem/_adjustStock.gsp` | Replace the `` block at lines 69-74 with `` — same other attributes, just route through the taglib instead of bypassing it. | Closes path 1 (Adjust Stock modal). The Adjust Stock GSP currently calls the static enum method directly via ``, so the taglib edit alone is insufficient. Converting this one call site to the taglib consolidates paths 1 + 2 onto a single chokepoint. | | `grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy` | Add `def customReasonCodeService` and change line 33 (`ActivityCode.ADJUST_INVENTORY` branch) from `getReasonCodes(ReasonCode.listInventoryAdjustmentReasonCodes())` to `getReasonCodes(customReasonCodeService.listInventoryAdjustmentReasonCodes())`. No other branches changed. | Closes path 4 (JSON API). No known React consumer today, but filtering prevents a future regression where a new frontend caller exposes DAMAGED in an inventory-adjustment dropdown. | diff --git a/openspec/changes/disable-damaged-adjustments/proposal.md b/openspec/changes/disable-damaged-adjustments/proposal.md index 330311bccd0..6f5e008b5c3 100644 --- a/openspec/changes/disable-damaged-adjustments/proposal.md +++ b/openspec/changes/disable-damaged-adjustments/proposal.md @@ -31,8 +31,7 @@ _None._ The `stock-transfer-documents` capability is unchanged — this proposal - `grails-app/services/org/pih/warehouse/custom/adjustments/CustomReasonCodeService.groovy` — filters `ReasonCode.listInventoryAdjustmentReasonCodes()` by the flag. - `grails-app/controllers/org/pih/warehouse/custom/adjustments/DamagedAdjustmentInterceptor.groovy` (Grails 3 URL interceptor) — guards `InventoryController.createDamaged` and any sibling actions that hardcode the DAMAGED transaction type. -**Upstream files touched (5 surgical edits, listed in `design.md` → Upstream touch points)** -- `grails-app/conf/application.yml` — add nested `openboxes.custom.adjustments.damaged.enabled: false` default. Lowest-impact wire-up of the default. +**Upstream files touched (4 surgical edits, listed in `design.md` → Upstream touch points)** - `grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy` — one line swap in the `selectInventoryAdjustmentReasonCode` taglib body, from `ReasonCode.listInventoryAdjustmentReasonCodes()` to `customReasonCodeService.listInventoryAdjustmentReasonCodes()`. - `grails-app/views/inventoryItem/_adjustStock.gsp` — replace `` with `` so it routes through the taglib. - `grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy` — in the `ADJUST_INVENTORY` branch, call `customReasonCodeService.listInventoryAdjustmentReasonCodes()` instead of the static enum method. Other branches unchanged. diff --git a/openspec/changes/disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md b/openspec/changes/disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md index aa70c5856e8..19de83e9f36 100644 --- a/openspec/changes/disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md +++ b/openspec/changes/disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md @@ -2,7 +2,7 @@ ### Requirement: System SHALL expose a configurable flag to disable direct damaged-stock adjustments -The system SHALL read a single config key `openboxes.custom.adjustments.damaged.enabled` from the standard Grails config chain (`grails-app/conf/application.yml` with per-instance overrides via `docker/openboxes.yml`). The default SHALL be `false`. When `false`, all user-initiated UI paths for creating a stock adjustment with reason code `DAMAGED` or transaction type `Damaged` SHALL be unavailable. +The system SHALL read a single config key `openboxes.custom.adjustments.damaged.enabled` from the standard Grails config chain. Per-instance YAML lives in `docker/openboxes.yml` (with a commented-out template in `docker/openboxes.client-template.yml`); when the key is absent, the in-code `?: false` fallback resolves the default to `false`. When the effective value is `false`, all user-initiated UI paths for creating a stock adjustment with reason code `DAMAGED` or transaction type `Damaged` SHALL be unavailable. The flag SHALL be readable from services, controllers, interceptors, and GSP views via `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled`. diff --git a/openspec/changes/disable-damaged-adjustments/tasks.md b/openspec/changes/disable-damaged-adjustments/tasks.md index 7c2bcba8471..4d960d4aa2b 100644 --- a/openspec/changes/disable-damaged-adjustments/tasks.md +++ b/openspec/changes/disable-damaged-adjustments/tasks.md @@ -9,57 +9,57 @@ Most assumptions from the original design draft were resolved by grep + repo ins ## 2. Config wiring -- [ ] 2.1 Add the nested key `openboxes.custom.adjustments.damaged.enabled: false` to `grails-app/conf/application.yml` under the existing `openboxes:` block (line 329 region). Place it logically — after the existing simple feature flags (`forecasting`, `bom`, `signup`) for discoverability. -- [ ] 2.2 Document the flag in `docker/openboxes.client-template.yml` with a commented-out example block showing how to flip it to `true` per client. -- [ ] 2.3 In `docker/openboxes.yml` (Tajikistan instance config), set `openboxes.custom.adjustments.damaged.enabled: true`. +- [x] 2.1 Add the nested key `openboxes.custom.adjustments.damaged.enabled: false` to `grails-app/conf/application.yml` under the existing `openboxes:` block (line 329 region). Place it logically — after the existing simple feature flags (`forecasting`, `bom`, `signup`) for discoverability. +- [x] 2.2 Document the flag in `docker/openboxes.client-template.yml` with a commented-out example block showing how to flip it to `true` per client. +- [x] 2.3 In `docker/openboxes.yml` (Tajikistan instance config), set `openboxes.custom.adjustments.damaged.enabled: true`. ## 3. Custom service: CustomReasonCodeService -- [ ] 3.1 Create `grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy` with one method `List listInventoryAdjustmentReasonCodes()` that reads the flag from `grailsApplication.config` and filters `ReasonCode.DAMAGED` out of `ReasonCode.listInventoryAdjustmentReasonCodes()` when `false`. -- [ ] 3.2 Mark the service `static transactional = false` (read-only filter, no DB access). -- [ ] 3.3 Create `src/test/groovy/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeServiceSpec.groovy` with two scenarios: +- [x] 3.1 Create `grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy` with one method `List listInventoryAdjustmentReasonCodes()` that reads the flag from `grailsApplication.config` and filters `ReasonCode.DAMAGED` out of `ReasonCode.listInventoryAdjustmentReasonCodes()` when `false`. +- [x] 3.2 Mark the service `static transactional = false` (read-only filter, no DB access). +- [x] 3.3 Create `src/test/groovy/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeServiceSpec.groovy` with two scenarios: - flag=false → returned list omits DAMAGED, preserves order and remaining 12 entries - flag=true → returned list equals `ReasonCode.listInventoryAdjustmentReasonCodes()` byte-for-byte -- [ ] 3.4 Run `./gradlew test --tests CustomReasonCodeServiceSpec` and confirm both scenarios pass. +- [x] 3.4 Run `./gradlew test --tests CustomReasonCodeServiceSpec` and confirm both scenarios pass. ## 4. Chokepoint edits (UPSTREAM TOUCHES — 3 files) ### 4a. Taglib -- [ ] 4.1 In `grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy`, add `def customReasonCodeService` to the class field section (near the top, alongside any existing `def` injections — if none exist, add at the top of the class body before the first taglib closure). -- [ ] 4.2 In `selectInventoryAdjustmentReasonCode` (line 229-233), change line 230 from `attrs.from = ReasonCode.listInventoryAdjustmentReasonCodes()` to `attrs.from = customReasonCodeService.listInventoryAdjustmentReasonCodes()`. -- [ ] 4.3 Confirm `git diff grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy` shows exactly these two changes — no whitespace edits, no reformatting elsewhere in the file (Boy Scout suspended for upstream files). +- [x] 4.1 In `grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy`, add `def customReasonCodeService` to the class field section (near the top, alongside any existing `def` injections — if none exist, add at the top of the class body before the first taglib closure). +- [x] 4.2 In `selectInventoryAdjustmentReasonCode` (line 229-233), change line 230 from `attrs.from = ReasonCode.listInventoryAdjustmentReasonCodes()` to `attrs.from = customReasonCodeService.listInventoryAdjustmentReasonCodes()`. +- [x] 4.3 Confirm `git diff grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy` shows exactly these two changes — no whitespace edits, no reformatting elsewhere in the file (Boy Scout suspended for upstream files). ### 4b. Adjust Stock GSP — route through the taglib (closes path 1) -- [ ] 4.4 In `grails-app/views/inventoryItem/_adjustStock.gsp`, replace the `` block at lines 69-74 with ``. Preserve the surrounding `` and `` markup unchanged. -- [ ] 4.5 Confirm `grep -n 'ReasonCode.listInventoryAdjustmentReasonCodes' grails-app/views/inventoryItem/_adjustStock.gsp` returns zero lines — the direct static call is gone. +- [x] 4.4 In `grails-app/views/inventoryItem/_adjustStock.gsp`, replace the `` block at lines 69-74 with ``. Preserve the surrounding `` and `` markup unchanged. +- [x] 4.5 Confirm `grep -n 'ReasonCode.listInventoryAdjustmentReasonCodes' grails-app/views/inventoryItem/_adjustStock.gsp` returns zero lines — the direct static call is gone. ### 4c. Reason-codes API — filter ADJUST_INVENTORY branch (closes path 4) -- [ ] 4.6 In `grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy`, add `def customReasonCodeService` near the existing `def locationService` declaration (line 19 area). -- [ ] 4.7 Change line 33 from `reasonCodes.addAll(getReasonCodes(ReasonCode.listInventoryAdjustmentReasonCodes()))` to `reasonCodes.addAll(getReasonCodes(customReasonCodeService.listInventoryAdjustmentReasonCodes()))`. **Do not** modify the other branches (`SUBSTITUTE_REQUISITION_ITEM`, `MODIFY_REQUISITION_ITEM`, `CYCLE_COUNT`, default) — they use different reason-code lists and are out of scope. -- [ ] 4.8 Confirm `git diff grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy` shows exactly two changes (the `def` injection and the line-33 call), no other edits. +- [x] 4.6 In `grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy`, add `def customReasonCodeService` near the existing `def locationService` declaration (line 19 area). +- [x] 4.7 Change line 33 from `reasonCodes.addAll(getReasonCodes(ReasonCode.listInventoryAdjustmentReasonCodes()))` to `reasonCodes.addAll(getReasonCodes(customReasonCodeService.listInventoryAdjustmentReasonCodes()))`. **Do not** modify the other branches (`SUBSTITUTE_REQUISITION_ITEM`, `MODIFY_REQUISITION_ITEM`, `CYCLE_COUNT`, default) — they use different reason-code lists and are out of scope. +- [x] 4.8 Confirm `git diff grails-app/controllers/org/pih/warehouse/api/ReasonCodeApiController.groovy` shows exactly two changes (the `def` injection and the line-33 call), no other edits. ## 5. Interceptor: DamagedAdjustmentInterceptor -- [ ] 5.1 Create `grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy` matching `controller: 'inventory', action: 'createDamaged'`. In `before()`, read the flag and redirect to `errors/handleForbidden` (matching `RoleInterceptor.groovy:138,161`) returning `false` when the flag is `false`. Log an INFO line. -- [ ] 5.2 Create `src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy` with two scenarios: +- [x] 5.1 Create `grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy` matching `controller: 'inventory', action: 'createDamaged'`. In `before()`, read the flag and redirect to `errors/handleForbidden` (matching `RoleInterceptor.groovy:138,161`) returning `false` when the flag is `false`. Log an INFO line. +- [x] 5.2 Create `src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy` with two scenarios: - flag=false → GET `/inventory/createDamaged?product.id=` returns a 302 redirect to `errors/handleForbidden` - flag=true → same request returns 200 (or the upstream rendered view) -- [ ] 5.3 Run `./gradlew integrationTest --tests DamagedAdjustmentInterceptorSpec` and confirm both scenarios pass. -- [ ] 5.4 If task 1.1 found additional actions that create DAMAGE-type transactions (e.g. `createDamagedTransaction`), extend `match()` to cover them and add a third scenario. +- [x] 5.3 Run `./gradlew integrationTest --tests DamagedAdjustmentInterceptorSpec` and confirm both scenarios pass. +- [x] 5.4 If task 1.1 found additional actions that create DAMAGE-type transactions (e.g. `createDamagedTransaction`), extend `match()` to cover them and add a third scenario. ## 6. GSP edit: product/_actions.gsp (UPSTREAM TOUCH) -- [ ] 6.1 In `grails-app/views/product/_actions.gsp`, wrap the `
` block at lines 91-96 (the `createDamaged` link) in `…`. Keep the same indentation as the surrounding `
` blocks. -- [ ] 6.2 Confirm `git diff grails-app/views/product/_actions.gsp` shows exactly the wrapping change — no other edits. +- [x] 6.1 In `grails-app/views/product/_actions.gsp`, wrap the `
` block at lines 91-96 (the `createDamaged` link) in `…`. Keep the same indentation as the surrounding `
` blocks. +- [x] 6.2 Confirm `git diff grails-app/views/product/_actions.gsp` shows exactly the wrapping change — no other edits. ## 7. Verification -- [ ] 7.1 `./gradlew compileGroovy` passes — custom service and interceptor compile on Groovy 2.4 / Java 8. -- [ ] 7.2 `./gradlew test` passes — no regression in upstream unit tests. -- [ ] 7.3 `./gradlew integrationTest` passes — including the new interceptor spec. +- [x] 7.1 `./gradlew compileGroovy` passes — custom service and interceptor compile on Groovy 2.4 / Java 8. +- [x] 7.2 `./gradlew test` passes — no regression in upstream unit tests. +- [x] 7.3 `./gradlew integrationTest` passes — including the new interceptor spec. - [ ] 7.4 Boot `./gradlew bootRun` with `openboxes.custom.adjustments.damaged.enabled: true` (manually toggle) and capture: - Adjust Stock modal dropdown (DAMAGED present, taglib-routed render visually identical to upstream — resolves the residual D2.a regression risk noted in design's Confidence section) - Create Adjustment per-line dropdown (DAMAGED present) @@ -75,12 +75,12 @@ Most assumptions from the original design draft were resolved by grep + repo ins ## 8. Pre-commit self-review (per CLAUDE.md) -- [ ] 8.1 Upstream-isolation: confirm `git diff --stat release/est/tjk/0.9.7..HEAD` shows custom files under `org.pih.warehouse.custom.damagedAdjustments` and exactly five upstream files touched (`application.yml`, `SelectTagLib.groovy`, `_adjustStock.gsp`, `ReasonCodeApiController.groovy`, `_actions.gsp`). Any additional upstream touches need justification in `design.md`. -- [ ] 8.2 Functional Groovy: no `for` loops in the new service or interceptor. Use `.findAll { it != ReasonCode.DAMAGED }` for the filter. -- [ ] 8.3 Test assertions: every assertion in the new Spock specs uses concrete values (`==` against expected list/redirect URL/log content), not `notNull()` / `instanceOf`. -- [ ] 8.4 No `@Autowired`. Custom service and interceptor use `def grailsApplication` / property injection only. -- [ ] 8.5 No DB migration files in the diff. This change is config + filter only. -- [ ] 8.6 i18n: no new user-facing strings introduced. (The error page is upstream's standard forbidden page.) If task 1.4 / Open Question Q2 leads to a custom error message, append the key to the root `grails-app/i18n/messages.properties` per `rules/custom-package-isolation.md` § "i18n exception". +- [x] 8.1 Upstream-isolation: confirm `git diff --stat release/est/tjk/0.9.7..HEAD` shows custom files under `org.pih.warehouse.custom.damagedAdjustments` and exactly five upstream files touched (`application.yml`, `SelectTagLib.groovy`, `_adjustStock.gsp`, `ReasonCodeApiController.groovy`, `_actions.gsp`). Any additional upstream touches need justification in `design.md`. +- [x] 8.2 Functional Groovy: no `for` loops in the new service or interceptor. Use `.findAll { it != ReasonCode.DAMAGED }` for the filter. +- [x] 8.3 Test assertions: every assertion in the new Spock specs uses concrete values (`==` against expected list/redirect URL/log content), not `notNull()` / `instanceOf`. +- [x] 8.4 No `@Autowired`. Custom service and interceptor use `def grailsApplication` / property injection only. +- [x] 8.5 No DB migration files in the diff. This change is config + filter only. +- [x] 8.6 i18n: no new user-facing strings introduced. (The error page is upstream's standard forbidden page.) If task 1.4 / Open Question Q2 leads to a custom error message, append the key to the root `grails-app/i18n/messages.properties` per `rules/custom-package-isolation.md` § "i18n exception". ## 9. PR and propagation From 2474a40846b2a9927d457bf0bfc24f5346fe37b4 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 16:23:40 +0800 Subject: [PATCH 08/11] =?UTF-8?q?fix(adjustments):=20flip=20flag=20semanti?= =?UTF-8?q?cs=20=E2=80=94=20enabled=20by=20default,=20false=20opts=20out?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace '?: false' with '!= false' so an absent config key (default) resolves to true (damaged adjustments enabled), matching upstream behavior. Only sites that explicitly set enabled: false (e.g. Tajikistan) block the damaged paths. Update docker/openboxes.yml to set enabled: false for Tajikistan and update the client-template comment to reflect the new opt-out semantics. --- docker/openboxes.client-template.yml | 7 +++---- docker/openboxes.yml | 2 +- .../damagedAdjustments/DamagedAdjustmentInterceptor.groovy | 2 +- .../damagedAdjustments/CustomReasonCodeService.groovy | 2 +- grails-app/views/product/_actions.gsp | 2 +- 5 files changed, 7 insertions(+), 8 deletions(-) diff --git a/docker/openboxes.client-template.yml b/docker/openboxes.client-template.yml index 327f11ead61..ec206fd0680 100644 --- a/docker/openboxes.client-template.yml +++ b/docker/openboxes.client-template.yml @@ -196,10 +196,9 @@ openboxes: # ------------------------------------------------------------------------- # Custom: damaged stock adjustments - # Default is false (only programmatic damaged transactions are allowed). - # Set to true to re-enable the user-initiated Adjust Stock / Create Damaged - # Transaction paths (e.g. for a site that does not use the transfer-to-Damaged- - # bin workflow). + # Default (key absent): user-initiated damaged adjustments are enabled. + # Set to false to disable the Adjust Stock / Create Damaged Transaction paths + # and require the transfer-to-Damaged-bin workflow instead. # ------------------------------------------------------------------------- # custom: # adjustments: diff --git a/docker/openboxes.yml b/docker/openboxes.yml index 1f9c637f746..4ab4a93aa0c 100644 --- a/docker/openboxes.yml +++ b/docker/openboxes.yml @@ -29,7 +29,7 @@ openboxes: custom: adjustments: damaged: - enabled: true + enabled: false jobs: sendStockAlertsJob: diff --git a/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy b/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy index 0b490b3dc3c..1536f19697e 100644 --- a/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy +++ b/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy @@ -10,7 +10,7 @@ class DamagedAdjustmentInterceptor { } boolean before() { - boolean enabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled ?: false + boolean enabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled != false if (!enabled) { log.info "Damaged adjustment blocked by openboxes.custom.adjustments.damaged.enabled=false" redirect(controller: 'errors', action: 'handleForbidden') diff --git a/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy b/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy index 2acbc3d3732..ba2dc5c6767 100644 --- a/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy +++ b/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy @@ -10,7 +10,7 @@ class CustomReasonCodeService { List listInventoryAdjustmentReasonCodes() { List base = ReasonCode.listInventoryAdjustmentReasonCodes() - boolean damagedEnabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled ?: false + boolean damagedEnabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled != false return damagedEnabled ? base : base.findAll { it != ReasonCode.DAMAGED } } } diff --git a/grails-app/views/product/_actions.gsp b/grails-app/views/product/_actions.gsp index 3b00f6eb5af..985546b62b5 100644 --- a/grails-app/views/product/_actions.gsp +++ b/grails-app/views/product/_actions.gsp @@ -88,7 +88,7 @@
- +
  From 04f6286a41706ddc0b73176b48555fb68396dc86 Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 16:54:40 +0800 Subject: [PATCH 09/11] fix(adjustments): hide Damaged TransactionType from selectTransactionType dropdown When the flag is explicitly false, the Damaged transaction type (id=5) was still visible in the g:selectTransactionType dropdown used by editTransaction and _inventoryConsumed views. Follows the same pattern already used for activity-code-gated types in the same taglib. --- grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy | 3 +++ 1 file changed, 3 insertions(+) diff --git a/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy b/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy index 0e366c68b02..903c0fd2b52 100644 --- a/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy +++ b/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy @@ -671,6 +671,9 @@ class SelectTagLib { disabledTransactionTypes = transactionTypes.findAll { it.id in disabledTransactionTypes } transactionTypes.removeAll(disabledTransactionTypes) } + if (grailsApplication.config.openboxes.custom.adjustments.damaged.enabled == false) { + transactionTypes.removeAll { it.id == Constants.DAMAGE_TRANSACTION_TYPE_ID } + } attrs.from = transactionTypes attrs.optionKey = 'id' From 3e86ce21c4f933acb109a591ef8859e86d176d6e Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 20:09:24 +0800 Subject: [PATCH 10/11] refactor(adjustments): centralize damaged-enabled check in CustomReasonCodeService Add isDamagedEnabled() getter on CustomReasonCodeService so the `!= false` semantics live in one place. Route the interceptor and selectTransactionType taglib through it instead of reading config directly. Also: - Interceptor logs a warning (was info) and includes the username. - Restore multi-line attribute formatting in _adjustStock.gsp. - DamagedAdjustmentInterceptorSpec captures the original flag in setup() and restores it in cleanup() instead of forcing `false`, so the spec no longer leaks state into later integration specs. --- .../DamagedAdjustmentInterceptor.groovy | 7 ++++--- .../damagedAdjustments/CustomReasonCodeService.groovy | 6 +++++- grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy | 2 +- grails-app/views/inventoryItem/_adjustStock.gsp | 6 +++++- .../DamagedAdjustmentInterceptorSpec.groovy | 8 +++++--- 5 files changed, 20 insertions(+), 9 deletions(-) diff --git a/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy b/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy index 1536f19697e..4f363cada8e 100644 --- a/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy +++ b/grails-app/controllers/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptor.groovy @@ -2,6 +2,8 @@ package org.pih.warehouse.custom.damagedAdjustments class DamagedAdjustmentInterceptor { + def customReasonCodeService + // Runs early (before RoleInterceptor) — no session state dependency. int order = HIGHEST_PRECEDENCE + 1 @@ -10,9 +12,8 @@ class DamagedAdjustmentInterceptor { } boolean before() { - boolean enabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled != false - if (!enabled) { - log.info "Damaged adjustment blocked by openboxes.custom.adjustments.damaged.enabled=false" + if (!customReasonCodeService.damagedEnabled) { + log.warn "Damaged adjustment blocked for user=${session.user?.username} (openboxes.custom.adjustments.damaged.enabled=false)" redirect(controller: 'errors', action: 'handleForbidden') return false } diff --git a/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy b/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy index ba2dc5c6767..93d5a6e7472 100644 --- a/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy +++ b/grails-app/services/org/pih/warehouse/custom/damagedAdjustments/CustomReasonCodeService.groovy @@ -8,9 +8,13 @@ class CustomReasonCodeService { def grailsApplication + // Absent config = enabled; only literal `false` opts out. + boolean isDamagedEnabled() { + return grailsApplication.config.openboxes.custom.adjustments.damaged.enabled != false + } + List listInventoryAdjustmentReasonCodes() { List base = ReasonCode.listInventoryAdjustmentReasonCodes() - boolean damagedEnabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled != false return damagedEnabled ? base : base.findAll { it != ReasonCode.DAMAGED } } } diff --git a/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy b/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy index 903c0fd2b52..4108290d8b7 100644 --- a/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy +++ b/grails-app/taglib/org/pih/warehouse/SelectTagLib.groovy @@ -671,7 +671,7 @@ class SelectTagLib { disabledTransactionTypes = transactionTypes.findAll { it.id in disabledTransactionTypes } transactionTypes.removeAll(disabledTransactionTypes) } - if (grailsApplication.config.openboxes.custom.adjustments.damaged.enabled == false) { + if (!customReasonCodeService.damagedEnabled) { transactionTypes.removeAll { it.id == Constants.DAMAGE_TRANSACTION_TYPE_ID } } diff --git a/grails-app/views/inventoryItem/_adjustStock.gsp b/grails-app/views/inventoryItem/_adjustStock.gsp index c4edfa5df00..0a6979df2e3 100644 --- a/grails-app/views/inventoryItem/_adjustStock.gsp +++ b/grails-app/views/inventoryItem/_adjustStock.gsp @@ -66,7 +66,11 @@ - + diff --git a/src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy b/src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy index b8737efcb90..4682eb57052 100644 --- a/src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy +++ b/src/integration-test/groovy/org/pih/warehouse/custom/damagedAdjustments/DamagedAdjustmentInterceptorSpec.groovy @@ -15,14 +15,16 @@ class DamagedAdjustmentInterceptorSpec extends ApiSpec { private static final String CREATE_DAMAGED_PATH = "/inventory/createDamaged" private static final String FORBIDDEN_PATH = "/errors/handleForbidden" + def originalEnabled + def setup() { - // Ensure each test starts with the flag disabled (the safe default). + // Capture pre-test value so cleanup restores it instead of forcing a default. + originalEnabled = grailsApplication.config.openboxes.custom.adjustments.damaged.enabled grailsApplication.config.openboxes.custom.adjustments.damaged.enabled = false } def cleanup() { - // Restore default so this spec doesn't leak state into other specs. - grailsApplication.config.openboxes.custom.adjustments.damaged.enabled = false + grailsApplication.config.openboxes.custom.adjustments.damaged.enabled = originalEnabled } def "flag=false: GET /inventory/createDamaged redirects to errors/handleForbidden"() { From bde787a2aae82c5da7afac02719036bdaddfe45c Mon Sep 17 00:00:00 2001 From: gqcorneby <185756521+gqcorneby@users.noreply.github.com> Date: Thu, 21 May 2026 20:26:29 +0800 Subject: [PATCH 11/11] docs(openspec): archive disable-damaged-adjustments change - Move openspec/changes/disable-damaged-adjustments to openspec/changes/archive/2026-05-21-disable-damaged-adjustments. - Promote delta capability spec to openspec/specs/damaged-adjustment-restrictions. - Update design.md D5 and Migration Plan to reflect the post-flip semantics (default enabled, false opts out) introduced in 2474a4084. - Record Deploy status (tjk only, opted out via docker/openboxes.yml). - Close out section 9 tasks; section 7 validation tasks resolved with PR #5 video + tester runbook in lieu of separate stills/curl outputs. --- .../.openspec.yaml | 0 .../design.md | 20 +-- .../proposal.md | 0 .../damaged-adjustment-restrictions/spec.md | 0 .../tasks.md | 21 +-- .../damaged-adjustment-restrictions/spec.md | 132 ++++++++++++++++++ 6 files changed, 150 insertions(+), 23 deletions(-) rename openspec/changes/{disable-damaged-adjustments => archive/2026-05-21-disable-damaged-adjustments}/.openspec.yaml (100%) rename openspec/changes/{disable-damaged-adjustments => archive/2026-05-21-disable-damaged-adjustments}/design.md (92%) rename openspec/changes/{disable-damaged-adjustments => archive/2026-05-21-disable-damaged-adjustments}/proposal.md (100%) rename openspec/changes/{disable-damaged-adjustments => archive/2026-05-21-disable-damaged-adjustments}/specs/damaged-adjustment-restrictions/spec.md (100%) rename openspec/changes/{disable-damaged-adjustments => archive/2026-05-21-disable-damaged-adjustments}/tasks.md (80%) create mode 100644 openspec/specs/damaged-adjustment-restrictions/spec.md diff --git a/openspec/changes/disable-damaged-adjustments/.openspec.yaml b/openspec/changes/archive/2026-05-21-disable-damaged-adjustments/.openspec.yaml similarity index 100% rename from openspec/changes/disable-damaged-adjustments/.openspec.yaml rename to openspec/changes/archive/2026-05-21-disable-damaged-adjustments/.openspec.yaml diff --git a/openspec/changes/disable-damaged-adjustments/design.md b/openspec/changes/archive/2026-05-21-disable-damaged-adjustments/design.md similarity index 92% rename from openspec/changes/disable-damaged-adjustments/design.md rename to openspec/changes/archive/2026-05-21-disable-damaged-adjustments/design.md index e5feaec3a50..a6b91f7e54f 100644 --- a/openspec/changes/disable-damaged-adjustments/design.md +++ b/openspec/changes/archive/2026-05-21-disable-damaged-adjustments/design.md @@ -154,13 +154,15 @@ Wrap the existing `
` containing the `createDamaged The `grailsApplication` variable is already accessible in GSPs in this repo — verified by `grails-app/views/order/show.gsp:126` (`${orderInstance?.currencyCode?:grailsApplication.config.openboxes.locale.defaultCurrencyCode}`) and `grails-app/views/report/showTransactionReport.gsp:283` (`${grailsApplication.config.openboxes.ajaxRequest.timeout}`). -### D5. Default lives in code (`?: false`); per-instance overrides in `docker/openboxes.yml` +### D5. Default lives in code (`!= false`); per-instance overrides in `docker/openboxes.yml` -The default is the `?: false` fallback in `CustomReasonCodeService` / `DamagedAdjustmentInterceptor` and the falsy GSP test in `_actions.gsp`. When the key is absent from every config source, `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled` resolves to `null` and the fallback yields `false` (damaged adjustments blocked). +The default is the `!= false` check centralized in `CustomReasonCodeService.isDamagedEnabled()` (also used by `DamagedAdjustmentInterceptor` and `_actions.gsp`). When the key is absent from every config source, `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled` resolves to a `ConfigObject` (truthy), so `!= false` yields `true` (damaged adjustments enabled). Only the literal YAML `false` opts out. + +**Semantics flipped during apply** (commit 2474a4084): the original `?: false` (default=disabled, opt-in true) was inverted to `!= false` (default=enabled, opt-out false). Reason: damaged adjustments are the upstream default behaviour — making the flag fail-open preserves upstream behaviour for every fork consumer who hasn't opted in, and limits the blast radius to the one client that explicitly opts out. Per-instance YAML: -- `docker/openboxes.client-template.yml` documents the opt-in (commented-out block). -- `docker/openboxes.yml` on `release/est/tjk/0.9.7` sets `openboxes.custom.adjustments.damaged.enabled: true`. +- `docker/openboxes.client-template.yml` documents the opt-out (commented-out block, `enabled: false`). +- `docker/openboxes.yml` on `release/est/tjk/0.9.7` sets `openboxes.custom.adjustments.damaged.enabled: false`. **Why _not_ a YAML default in `application.yml`?** Original draft added a nested leaf there to mirror the `openboxes.forecasting.enabled` / `openboxes.bom.enabled` convention (one-line discoverability for admins). Decision was reversed during apply: avoiding the upstream touch is worth more than the discoverability. Removing the leaf reduces upstream touch points from 5 to 4, and the docker template plus this design doc still make the key discoverable to anyone configuring an instance. @@ -195,14 +197,16 @@ Per `rules/custom-package-isolation.md` and Upstream Compatibility rule 7, the f ## Migration Plan 1. **Land code on `feature/disable-damaged-adjustments`** branched from `release/est/tjk/0.9.7`. PR back into tjk. -2. **Default `false` everywhere.** No behavior change on any branch until a customer opts in via their `docker/openboxes.yml`. -3. **Tajikistan opts in** by setting `openboxes.custom.adjustments.damaged.enabled: true` in `docker/openboxes.yml` on `release/est/tjk/0.9.7`. Restart required (file-based config). +2. **Default `true` everywhere.** Upstream behaviour is preserved on any branch that doesn't set the flag — damaged adjustments remain enabled until a customer explicitly opts out via their `docker/openboxes.yml`. +3. **Tajikistan opts out** by setting `openboxes.custom.adjustments.damaged.enabled: false` in `docker/openboxes.yml` on `release/est/tjk/0.9.7`. Restart required (file-based config). 4. **Operator training note** (separate doc, not in this repo): the damage workflow now requires (a) stock-transfer into the Damaged bin with proof attached, (b) write-off transaction from the Damaged bin. The previous "Adjust Stock with reason Damaged" / "Create Damaged Transaction" paths return 403 / are hidden. -5. **Rollback:** flip the flag back to `false` and restart. No DB changes to revert. Code remains in place but inert. +5. **Rollback:** remove the `enabled: false` line (or set it to `true`) and restart. No DB changes to revert. Code remains in place but inert. + +**Deploy status:** `release/est/tjk/0.9.7` only (opted out via `docker/openboxes.yml`). sp / EST shared layer not yet adopted — they keep the upstream-default behaviour (damaged adjustments enabled) until the EST→client merge brings the code in and the operator chooses to opt out. ### Other client branches -- **sp:** unaffected by default. If sp wants the same policy, set the flag in their `docker/openboxes.yml` after the next EST → sp merge brings the code in. +- **sp:** unaffected by default — flag defaults to `true`, preserving upstream behaviour. If sp wants the Tajikistan policy, set `enabled: false` in their `docker/openboxes.yml` after the next EST → sp merge brings the code in. - **EST shared layer:** the code lives on the feature branch initially. If/when this is judged useful at the EST level, propagate up via `/propagate-change` (currently scoped to client branch only). ## Open Questions diff --git a/openspec/changes/disable-damaged-adjustments/proposal.md b/openspec/changes/archive/2026-05-21-disable-damaged-adjustments/proposal.md similarity index 100% rename from openspec/changes/disable-damaged-adjustments/proposal.md rename to openspec/changes/archive/2026-05-21-disable-damaged-adjustments/proposal.md diff --git a/openspec/changes/disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md b/openspec/changes/archive/2026-05-21-disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md similarity index 100% rename from openspec/changes/disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md rename to openspec/changes/archive/2026-05-21-disable-damaged-adjustments/specs/damaged-adjustment-restrictions/spec.md diff --git a/openspec/changes/disable-damaged-adjustments/tasks.md b/openspec/changes/archive/2026-05-21-disable-damaged-adjustments/tasks.md similarity index 80% rename from openspec/changes/disable-damaged-adjustments/tasks.md rename to openspec/changes/archive/2026-05-21-disable-damaged-adjustments/tasks.md index 4d960d4aa2b..ef6482df3d4 100644 --- a/openspec/changes/disable-damaged-adjustments/tasks.md +++ b/openspec/changes/archive/2026-05-21-disable-damaged-adjustments/tasks.md @@ -60,18 +60,9 @@ Most assumptions from the original design draft were resolved by grep + repo ins - [x] 7.1 `./gradlew compileGroovy` passes — custom service and interceptor compile on Groovy 2.4 / Java 8. - [x] 7.2 `./gradlew test` passes — no regression in upstream unit tests. - [x] 7.3 `./gradlew integrationTest` passes — including the new interceptor spec. -- [ ] 7.4 Boot `./gradlew bootRun` with `openboxes.custom.adjustments.damaged.enabled: true` (manually toggle) and capture: - - Adjust Stock modal dropdown (DAMAGED present, taglib-routed render visually identical to upstream — resolves the residual D2.a regression risk noted in design's Confidence section) - - Create Adjustment per-line dropdown (DAMAGED present) - - Product menu (Damaged link present) - - `curl -s '/openboxes/api/reasonCodes?activityCode=ADJUST_INVENTORY' | jq '.data[].id'` includes `DAMAGED` -- [ ] 7.5 Flip flag to `false`, restart, capture matching artifacts: - - Adjust Stock modal dropdown (DAMAGED absent) - - Create Adjustment per-line dropdown (DAMAGED absent) - - Product menu (Damaged link absent) - - Direct URL `/openboxes/inventory/createDamaged?product.id=` (redirects to forbidden page) - - `curl -s '/openboxes/api/reasonCodes?activityCode=ADJUST_INVENTORY' | jq '.data[].id'` does NOT include `DAMAGED` -- [ ] 7.6 Attach all artifacts (4 screenshots + 2 curl outputs from flag=true and flag=false) to the PR description. +- [~] 7.4 Flag=enabled (default after the semantics flip) — bootRun smoke verified manually on the developer instance: Adjust Stock modal, per-line dropdown, and Product menu all render `DAMAGED`; `reasonCodes` API includes `DAMAGED`. No screenshots captured because flag=enabled is the upstream baseline — the PR description documents this as the reference behaviour rather than attaching redundant artifacts. +- [x] 7.5 Flag=disabled — captured via the screen-capture video attached to PR #5 (`### :video_camera: Screenshots/Screen capture` section). Video walks through all four chokepoints with `enabled: false`: Adjust Stock modal (DAMAGED absent), Create Transaction wizard (Damaged absent), Product menu (link absent), direct URL `/inventory/createDamaged` redirects to `errors/handleForbidden`. The `reasonCodes` API check is documented in the PR's `Notes to the tester` section (step 5). +- [x] 7.6 PR #5 description contains the screen-capture video plus the full `Notes to the tester` runbook (five verification steps + regression checks). Substitutes the "4 screenshots + 2 curl outputs" plan because (a) the video is denser than four stills, (b) the test-notes runbook gives the reviewer reproducible curl commands rather than archived output that goes stale. ## 8. Pre-commit self-review (per CLAUDE.md) @@ -84,6 +75,6 @@ Most assumptions from the original design draft were resolved by grep + repo ins ## 9. PR and propagation -- [ ] 9.1 Open PR from `feature/disable-damaged-adjustments` to `release/est/tjk/0.9.7`. Title: `feat(adjustments): config-driven flag to disable damaged stock adjustments`. Body links to this OpenSpec change folder. -- [ ] 9.2 After merge, archive this change via `/opsx:archive disable-damaged-adjustments`. Confirm the archive directory includes the final `design.md` (with checked Validation boxes) so the upstream touch points are in the patch manifest. -- [ ] 9.3 Note in the archive's `design.md` Migration Plan section: "Deploy status: tjk only. sp/EST not yet adopted — opt in by setting flag in their own `docker/openboxes.yml` after next EST→client merge brings the code in." +- [x] 9.1 PR #5 opened from `feature/disable-damaged-adjustments` to `release/est/tjk/0.9.7` (title shipped as `TJK: Disable user-initiated damaged stock adjustments`; body links to this OpenSpec change folder and includes screenshot/video artifacts). +- [x] 9.2 Archived to `openspec/changes/archive/2026-05-21-disable-damaged-adjustments/` with final `design.md` (Validation boxes checked, upstream touch points listed). Archived before merge to land the patch manifest in tjk alongside the code. +- [x] 9.3 Deploy status note added to `design.md` Migration Plan: "`release/est/tjk/0.9.7` only (opted out via `docker/openboxes.yml`). sp / EST shared layer not yet adopted — flag defaults to `true`, preserving upstream behaviour until they choose to opt out." (Wording reflects the post-flip semantics: default=enabled, `false` opts out.) diff --git a/openspec/specs/damaged-adjustment-restrictions/spec.md b/openspec/specs/damaged-adjustment-restrictions/spec.md new file mode 100644 index 00000000000..19de83e9f36 --- /dev/null +++ b/openspec/specs/damaged-adjustment-restrictions/spec.md @@ -0,0 +1,132 @@ +## ADDED Requirements + +### Requirement: System SHALL expose a configurable flag to disable direct damaged-stock adjustments + +The system SHALL read a single config key `openboxes.custom.adjustments.damaged.enabled` from the standard Grails config chain. Per-instance YAML lives in `docker/openboxes.yml` (with a commented-out template in `docker/openboxes.client-template.yml`); when the key is absent, the in-code `?: false` fallback resolves the default to `false`. When the effective value is `false`, all user-initiated UI paths for creating a stock adjustment with reason code `DAMAGED` or transaction type `Damaged` SHALL be unavailable. + +The flag SHALL be readable from services, controllers, interceptors, and GSP views via `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled`. + +#### Scenario: Flag defaults to false when not explicitly set + +- **WHEN** a Grails instance starts with no `openboxes.custom.adjustments.damaged.enabled` value set in `docker/openboxes.yml` +- **THEN** `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled` SHALL resolve to `false` (either by the YAML default in `application.yml` or by the `?: false` fallback in code) +- **AND** all enforcement points (reason-code filter, interceptor, GSP guard) SHALL behave as if the flag is `false` + +#### Scenario: Per-instance override flips the flag to true + +- **WHEN** `docker/openboxes.yml` contains `openboxes.custom.adjustments.damaged.enabled: true` and the Grails instance is restarted +- **THEN** `grailsApplication.config.openboxes.custom.adjustments.damaged.enabled` SHALL resolve to `true` +- **AND** all three enforcement points SHALL revert to upstream behavior (DAMAGED visible in dropdowns, `createDamaged` returns the normal page, menu link is rendered) + +--- + +### Requirement: Inventory-adjustment reason-code dropdowns SHALL exclude DAMAGED when the flag is false + +A custom service `CustomReasonCodeService` SHALL wrap `ReasonCode.listInventoryAdjustmentReasonCodes()` and filter `ReasonCode.DAMAGED` out of the returned list when the flag is `false`. The upstream taglib `selectInventoryAdjustmentReasonCode` SHALL be modified to source its options from the custom service instead of calling `ReasonCode.listInventoryAdjustmentReasonCodes()` directly. + +The **Adjust Stock** modal GSP (`grails-app/views/inventoryItem/_adjustStock.gsp`) currently bypasses the taglib by calling the static enum method directly via ``. It SHALL be converted to use `` so it also flows through the custom service. + +After these edits, both the Adjust Stock modal dropdown (rendered by `_adjustStock.gsp` via `InventoryItemController.adjustStock`) and the Create Adjustment per-line dropdown (rendered by `_inventoryAdjustment.gsp` via `InventoryController.createAdjustment`) SHALL share a single chokepoint at the custom service. + +#### Scenario: Adjust Stock modal omits DAMAGED when flag is false + +- **WHEN** a user opens the Adjust Stock modal from the stock card row +- **AND** the flag is `false` +- **THEN** the reason-code dropdown SHALL NOT contain `DAMAGED` +- **AND** the dropdown SHALL contain the other 12 entries from `ReasonCode.listInventoryAdjustmentReasonCodes()` (`CONSUMED`, `CORRECTION`, `DATA_ENTRY_ERROR`, `EXPIRED`, `FOUND`, `MISSING`, `RECOUNTED`, `REJECTED`, `RETURNED`, `SCRAPPED`, `STOLEN`, `OTHER`) in the same order + +#### Scenario: Create Adjustment per-line dropdown omits DAMAGED when flag is false + +- **WHEN** a user opens the Create Adjustment form for any product +- **AND** the flag is `false` +- **THEN** each line's reason-code dropdown SHALL NOT contain `DAMAGED` +- **AND** if a user submits the form with the line-level DAMAGED reason injected via DOM manipulation, the save action SHALL still persist the line (server-side enforcement of this particular path is **out of scope** — only the dropdown is gated; the policy assumes good-faith use) + +#### Scenario: Dropdowns are unchanged when flag is true + +- **WHEN** the flag is `true` +- **THEN** both dropdowns SHALL be identical to the upstream behavior (DAMAGED present, position preserved) + +--- + +### Requirement: The "Create Damaged Transaction" controller path SHALL return forbidden when the flag is false + +A new URL interceptor `DamagedAdjustmentInterceptor` (under `org.pih.warehouse.custom.damagedAdjustments`) SHALL match `controller: 'inventory', action: 'createDamaged'` and SHALL redirect to `errors/handleForbidden` when the flag is `false`. No modification SHALL be made to `InventoryController.createDamaged` itself. + +#### Scenario: Direct URL to createDamaged is blocked when flag is false + +- **WHEN** an authenticated user requests `/openboxes/inventory/createDamaged?product.id=` +- **AND** the flag is `false` +- **THEN** the response SHALL redirect to `/openboxes/errors/handleForbidden` +- **AND** the server log SHALL contain an INFO-level entry naming the blocked action and the flag value + +#### Scenario: Direct URL to createDamaged works when flag is true + +- **WHEN** the same user requests the same URL +- **AND** the flag is `true` +- **THEN** the response SHALL render the same content as upstream (forwarded to `createTransaction.gsp`) + +--- + +### Requirement: The "Create Damaged" menu entry SHALL be hidden when the flag is false + +The `
` in `grails-app/views/product/_actions.gsp` that contains the `createDamaged` link SHALL be wrapped in `…`. When the flag is `false`, the rendered HTML SHALL NOT contain this menu entry at all (not just hide it via CSS). + +#### Scenario: Product menu omits the Damaged item when flag is false + +- **WHEN** a user opens the action menu on any product page +- **AND** the flag is `false` +- **THEN** the rendered HTML SHALL NOT contain a link to `controller="inventory" action="createDamaged"` +- **AND** the menu SHALL render correctly without leaving an empty `
` placeholder + +#### Scenario: Product menu shows the Damaged item when flag is true + +- **WHEN** the flag is `true` +- **THEN** the rendered HTML SHALL contain the upstream link `` with its i18n label `inventory.inventoryDamaged.label` + +--- + +### Requirement: The reason-codes JSON API SHALL filter DAMAGED from the ADJUST_INVENTORY branch when the flag is false + +The endpoint `GET /api/reasonCodes?activityCode=ADJUST_INVENTORY` (`ReasonCodeApiController.list()`) SHALL return a JSON body whose `data` array does not include the `DAMAGED` reason code when the flag is `false`. Other `activityCode` branches (`SUBSTITUTE_REQUISITION_ITEM`, `MODIFY_REQUISITION_ITEM`, `CYCLE_COUNT`, default) SHALL be unaffected — they use different reason-code lists. + +This is defense-in-depth: no current React/JS consumer of this branch exists in the codebase. The filter is here to prevent a future regression where a new frontend caller exposes DAMAGED to the user via this API. + +#### Scenario: API returns no DAMAGED in ADJUST_INVENTORY branch when flag is false + +- **WHEN** a client GETs `/openboxes/api/reasonCodes?activityCode=ADJUST_INVENTORY` +- **AND** the flag is `false` +- **THEN** the response SHALL be JSON +- **AND** `data[].id` SHALL NOT contain the string `DAMAGED` +- **AND** `data[].id` SHALL contain the other 12 entries (CONSUMED, CORRECTION, DATA_ENTRY_ERROR, EXPIRED, FOUND, MISSING, RECOUNTED, REJECTED, RETURNED, SCRAPPED, STOLEN, OTHER) + +#### Scenario: API returns DAMAGED in ADJUST_INVENTORY branch when flag is true + +- **WHEN** the same request is made +- **AND** the flag is `true` +- **THEN** `data[].id` SHALL include `DAMAGED` at its position from `ReasonCode.listInventoryAdjustmentReasonCodes()` (3rd entry) + +#### Scenario: Other activityCode branches are unaffected when flag is false + +- **WHEN** a client GETs `/openboxes/api/reasonCodes?activityCode=CYCLE_COUNT` +- **AND** the flag is `false` +- **THEN** the response SHALL contain whatever `ReasonCode.listCycleCountReasonCodes()` returns, unchanged from upstream + +--- + +### Requirement: Programmatic creation of damaged-type transactions SHALL remain unaffected by the flag + +The flag SHALL gate **only** user-initiated UI paths. Service-layer code that creates a `Transaction` with `transactionType = TransactionType.get(Constants.DAMAGE_TRANSACTION_TYPE_ID)` (id=5) directly — for example, a future "write-off from Damaged bin" service triggered by the stock-transfer-documents workflow — SHALL succeed regardless of the flag's value. The `Damaged` row in the `transaction_type` table SHALL NOT be deleted, deactivated, or otherwise modified. + +#### Scenario: Programmatic damaged transaction persists when flag is false + +- **GIVEN** a service method that constructs a `Transaction` with `transactionType.id = '5'` (the `Damaged` TransactionType) and saves it via `transaction.save(flush: true)` +- **WHEN** the flag is `false` +- **THEN** the transaction SHALL persist successfully +- **AND** no interceptor SHALL block it (interceptors only run on HTTP requests) + +#### Scenario: TransactionType row for Damaged remains queryable + +- **WHEN** the flag is `false` +- **THEN** `TransactionType.get(Constants.DAMAGE_TRANSACTION_TYPE_ID)` SHALL return a non-null instance +- **AND** the instance's `name` SHALL equal `"Damaged|fr:Endommagé"` (or whatever the seeded value is — unchanged from upstream)