feat(spp_drims): Incident Management review — states, closed-incident limits, dashboard and warehouses - #416
feat(spp_drims): Incident Management review — states, closed-incident limits, dashboard and warehouses#416emjay0921 wants to merge 16 commits into
Conversation
…of allocations) and Distributed net of returns (#1160)
…personnel, field edits (#1158)
…e opens form (#1123)
…ids changes (#1094)
…tion/request pickers (#1164)
QA round 1 on OP#1157 returned three findings and a question. Flag As Alert was inserted before the statusbar, which rendered it last. It is now anchored on the first header button so the order reads Flag As Alert, Start Recovery, Close Incident — the order the workflow runs in. Anchored by button name rather than position, so a reordering of the base view fails loudly at upgrade instead of silently drifting back. New incidents landed straight in Active, skipping the triage step the Alert state exists for. The status default becomes "alert", making the lifecycle Alert -> Active -> Recovery -> Closed. Changed on spp_hazard, where the state machine lives, so every consumer behaves the same rather than the same model behaving differently depending on which modules are installed. Close Incident was already correct in effect — with four states, "not closed" and "alert, active or recovery" are the same set — but it is now stated positively so it stays right if a state is ever added. QA also asked when Recovery can be set: only from Active, since Start Recovery is hidden otherwise. With Alert as the entry state a new incident is confirmed Active before Recovery is offered. A test pins that so the answer does not quietly change. Four spp_hazard tests asserted the old default; they now confirm Active explicitly rather than assuming it. Not changed, and flagged for QA instead: DRIMS low-stock alerting and the request-from-template picker both filter on status = "active", so an incident sitting in Alert reaches neither until it is confirmed. Whether an alert-state incident should drive stock alerting is a product decision. Verified across every spp_hazard dependent — spp_hazard, spp_drims, spp_hazard_programs, spp_api_v2_gis, spp_gis_indicators, spp_drims_sl_demo — and by hand on a fresh database. OP#1157
Closing an incident left Hazard Category clickable, so a closed record still offered a way through to the category form. Making the field readonly does not cover this — a readonly many2one still renders as an internal link — and no_open cannot be made conditional, because options is parsed as a static dict and cannot reference status. Declare the field twice instead, with mutually exclusive invisible: an open incident keeps its link, a closed one is inert text. The same pattern is already used across the codebase wherever a field has to change shape with state. Asserted on the arch rather than through behaviour, since whether a many2one is clickable is decided in the client, not the ORM.
…ent-management # Conflicts: # spp_drims/models/request.py # spp_drims/views/request_views.xml
OP#1079 made the request line's quantity_allocated a stored compute over per-warehouse allocation rows, so the helper's direct write no longer registered and the Units KPI counted stock that had in fact been allocated.
Round 2 made Alert the entry state. QA has since changed the requirement: an incident should be entered as a draft, and the person recording it says what it is — Flag As Alert for something being watched, Set Active for a response already under way. Neither is assumed on their behalf. Adds a draft state at the head of the selection and makes it the default. Set Active is now offered from Draft as well, so a response already under way does not have to be flagged as an alert first. Start Recovery and Close Incident stay hidden there, which is what leaves a draft showing exactly the two buttons asked for. The list's default filter needed Draft adding too. It pre-selects Alert, Active and Recovery, so with Draft as the entry state a newly created incident would have disappeared from the list it was created in and read as having failed to save. A test asserts both the filter and its default, because nothing at the model level would notice. A draft is deliberately not a live incident: it is excluded from is_ongoing, from the affected-registrant check, and from the Active-only consumers. It starts counting once classified. A draft also cannot be closed — a mistaken one is deleted rather than closed. Six tests asserting the round-2 lifecycle are rewritten, not dropped.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #416 +/- ##
==========================================
+ Coverage 72.24% 72.85% +0.60%
==========================================
Files 419 464 +45
Lines 29813 31575 +1762
==========================================
+ Hits 21539 23003 +1464
- Misses 8274 8572 +298
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Thanks — this is a well-built PR. The tests are genuinely good (arch-level assertions with rationale, both contributing modules covered, guards tested positive and negative), and I verified the OP#1123 kanban approach against the Odoo 19 source: can_open="0" on <kanban> plus <a type="open"> is correct — type="open" calls openRecord unconditionally (kanban_record.js:346), independent of canOpenRecords. Routing _inverse_drims_warehouses through stock.warehouse.write so the OP#1094 refresh hook fires is a nice touch.
Two things need changes before this can land, plus some questions and nits.
1. Blocker — no version bump and no migration on either module
spp_drims stays at 19.0.3.0.0 and spp_hazard at 19.0.2.0.2, identical to 19.0. Both modules shipped in release 2026.08, so the released-baseline rule applies (same-version code changes give upgraders no signal and skip migrations/ scripts).
On an existing database without an upgrade:
- The new
ir.config_parameterrecorddrims.warehouse.filter_by_incidentnever loads (the code default masks this one). - None of the view changes load — Draft filter/statusbar, Flag As Alert, closed-incident readonly, kanban
can_open— i.e. the bulk of the user-visible work.
And even with an upgrade, stored computes whose semantics changed are not recomputed — Odoo only auto-computes stored fields when the column is newly created:
drims_total_stock_units/drims_stock_item_countchanged meaning from "physical warehouse contents" to "incident-related stock net of allocations". Existing rows keep old-semantics numbers until a dependency happens to change — for quiet or closed incidents, indefinitely.drims_distributed_valueis now net of returns. Stored values and livespp.data.valuecache rows still hold gross numbers. The 15-minute cron self-heals open incidents, but closed incidents are excluded from the cron and never recompute.
Ask: bump both manifests and add a migrations/<version>/post-migration.py that purges the drims_distributed_value (and arguably drims_stock_value) cache rows and recomputes the three stored KPI fields for all incidents.
2. Major — "closed is inert" is UI-only for the incident itself; lifecycle transitions are unguarded server-side
The DRIMS operations got real server-side guards (_drims_ensure_open), but the incident's own lifecycle didn't:
action_close,action_set_active, and the newaction_set_alerthave no state guards — visibility is only enforced in the form header. Via RPC/import/shell, a draft can be closed (contradicting the QA-confirmed "a draft cannot be closed — a mistaken one is deleted") and a closed incident can be flipped back to active/alert, sidestepping every OP#1158 guard.- Field edits on a closed incident are only
readonly=in the views, which Odoo does not enforce on write — so "editing incident fields are all refused" holds only in the web client.
Ask: at minimum, raise in the action_* methods when the transition isn't legal from the current state (e.g. action_close refuses drafts, action_set_alert/action_set_active refuse closed). Optionally a write() guard on closed incidents (allowlisting status/end_date/chatter fields) if server-side enforcement of field edits is wanted. If UI-only is the accepted posture for field edits, let's state that explicitly in the PR description instead.
3. Questions — gaps in the closed-incident guard coverage (possibly intentional)
action_create_dispatchis not guarded: a request allocated before closure can still be dispatched afterwards — stock leaves the warehouse for a closed incident. Allocate is guarded; dispatch isn't. Intentional?action_mark_received/action_stockon donations are not guarded: a donation pledged before closure can be received and stocked in afterwards. Deliberate (goods in flight), or a gap?- The create-time guards check
valsonly, sowrite({'incident_id': <closed>})re-points an existing donation/personnel record to a closed incident unguarded. - A request can still be created against a closed incident (only submit/approve/allocate raise) while a donation cannot even be created — the guard tests themselves rely on creating requests against a closed incident. Is the asymmetry intended?
4. Nits
spp_hazard/tests/test_registrant.py— comment says "incidents now start in alert"; they start in draft._cron_refresh_drims_kpis— the "status field values: alert, active, recovery, closed" comment is missing draft._compute_allowed_warehouse_idsis duplicated verbatim (docstring included) indonation.pyandrequest.py; a small shared helper would keep the filter-on/fallback policy in one place.- Units/Distributed asymmetry: a confirmed return reduces Distributed, but the returned goods are not added back to incident Units (their allocation still counts them out; re-allocating them would count them out twice, clamped at 0 per product). Probably fine — deserves a note.
- In base
spp_hazardwithout DRIMS installed, a draft has no UI path to Alert (the button lives inspp_drims) — only Set Active. Fine if Alert is a DRIMS-driven concept, but the state lives in the base module.
Both warehouse-choice tests create a donation with no items. The donations review on fix/1076-drims-donations-review adds _check_has_lines, which forbids exactly that — so whichever of the two branches merges second turns 19.0 red, and with no textual conflict between them nothing warns either author first. The line is incidental to what these tests assert; it is there to keep the donation valid. Not done here: dropping this branch's create-time closed-incident guard in favour of that review's _check_incident_not_closed constraint, which is strictly broader because it also catches an existing donation re-pointed at a closed incident. The constraint does not exist on this branch, so removing the guard now would leave test_1158_donation_blocked_when_closed with nothing to raise. It should follow once the donations PR has merged.
… and lifecycle guards Version bumps on spp_drims and spp_hazard, and a migration for the part a bump alone does not fix. Three stored computes changed meaning rather than being added: incident stock units and item count now count incident-related stock net of allocations, and distributed value is net of confirmed returns. Odoo only computes a stored field for existing rows when its column is new, so an upgraded database would keep the old numbers until some dependency happened to change — and for closed incidents never, since the refresh cron skips them. The migration drops the stale spp.data.value cache rows the computes would otherwise prefer, then recomputes all three for every incident, closed ones included. The incident's own lifecycle is now enforced on the server, not only hidden in the form header. A draft cannot be closed — QA's rule is that a mistakenly entered incident is deleted — and a closed incident cannot be moved back to alert, active or recovery, which over RPC would have sidestepped every guard that keys off `closed`. Alert goes through the same gate. Field edits on a closed incident stay a UI-level rule, deliberately: a write() guard would have to allowlist its way around the stored KPI computes, which legitimately write to closed incidents. The rule that protects data is on the DRIMS operations, and those are already guarded server-side. Three spp_hazard tests closed an incident straight after creating it, which the draft entry state makes illegal; they now take it through the lifecycle first. Nits from the review: _compute_allowed_warehouse_ids was duplicated verbatim in donation.py and request.py and now calls one helper on the incident, the cron comment listing status values was missing draft, and a test comment still said incidents start in alert.
|
Thanks — blockers 1 and 2 are done, the nits are done, and the questions in 3 are answered below with one of them fixed. Pushed as 1. Version bump + migration — done. The migration does the two things you identified, and the second is the one I would have missed: it drops the 2. Server-side lifecycle guards — done. On the optional Three 3. Your questions:
4. Nits — all done except the two that are notes rather than changes:
Merge order: this branch and #414 both touch |
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Approved — both blockers are done and verified, the questions are answered credibly, and the fix quality matches the rest of this batch.
What I re-verified on f5592290 / c433ad82:
- The migration is correct in the way that matters. It expires the
drims_distributed_value/drims_stock_valuecache rows before recomputing — checked againstspp.data.value.read_values, which excludes expired rows, so the recompute cannot read the pre-change numbers straight back — thenadd_to_computeover all incidents including closed ones, which is exactly where a stale figure would otherwise sit forever.19.0.3.1.0matches the migration directory;spp_hazard's bump needs no migration of its own. Changelogs regenerated on both modules. - The lifecycle gate covers all four actions (
action_close,action_set_active,action_set_recovery, andspp_drims'saction_set_alert), with the four new tests pinning reopen-refused, re-close-refused, draft-deleted-not-closed, and the open lifecycle intact. - The
write()posture is accepted, and for a better reason than cost: the stored KPI computes legitimately write to closed incidents — this PR's own migration does — so a blanket guard would break the mechanism that keeps closed incidents' numbers honest. Now stated in the PR body, which is what I asked for. - The #414 fixture collision is fixed on this side (
c433ad82), with the right call on not yet dropping the duplicate donation guard — removing it before #414's constraint exists on this branch would leavetest_1158_donation_blocked_when_closedasserting nothing.
On the deferred items — dispatch-after-closure guard, personnel incident_id re-pointing, and the request/donation creation asymmetry — the reasoning for deferring each to the #414/#433/#390 reconciliation is sound, and they're now tracked as a ticket on OpenProject so they survive the queue.
One item that crossed with your push by two minutes: the OP#1160 provenance ruling landed (Units/Products keeps the #1160 definition — your migration already matches it), with one suggestion attached — label the dashboard tiles (or their tooltips) so the provenance-vs-location split between Units/Products and Stock Value is legible. Fine as a follow-up.
Your merge-order input (land #414 first; whichever of the two goes second keeps the higher version and concatenates HISTORY) has been passed to Edwin for the queue decision. Merge itself stays with him per the usual flow.
2 participants
Why is this change needed?
OP#1100 (DRIMS - Full review - Incident Management) and all of its children have passed QA — this raises the PR for that work, per the flow of opening PRs once QA signs off.
What is in here
Closed incidents: what is enforced where
Enforced on the server: the DRIMS operations (
_drims_ensure_open), and the incident's own lifecycle — a draft cannot be closed, and a closed incident cannot be moved back to alert, active or recovery.UI only, deliberately: editing the incident's own fields while closed. A
write()guard would have to allowlist its way around the stored KPI computes, which legitimately write to closed incidents — this PR's own migration recomputes them, and so would any future repair. Blocking writes there would break the mechanism that keeps a closed incident's numbers honest, to prevent an edit that carries no operational consequence. The data that matters is protected by the operation guards.Version bumps:
spp_drimsto19.0.3.1.0andspp_hazardto19.0.2.1.0, withmigrations/19.0.3.1.0/post-migration.py. Three stored computes changed meaning rather than being added, so an upgrade alone would leave existing rows on the old semantics — indefinitely for closed incidents, which the refresh cron skips. The migration purges the stalespp.data.valuecache rows first (the computes prefer a live cache entry) and then recomputes all three for every incident.Unit tests executed by the author
On the merged branch, across the base module and everything downstream of it:
./spp lint --all-filesclean.How to test manually
Related links
https://openspp.openproject.com/work_packages/1100
Reviewer notes
OP#1157 changed requirement twice, which is why its history on this branch is three commits. Round 1 exposed the existing Alert state via a button; round 2 made Alert the entry state; round 3 replaced that with a new Draft state, because QA wanted the person recording an incident to say what it is rather than have Alert assumed. The final shape is round 3.
Two decisions were made where the ticket was silent, both confirmed by QA on the ticket: a draft cannot be closed — a mistaken one is deleted — and a draft does not count as a live incident anywhere (not "ongoing", not marking registrants affected, and not offered where only Active incidents are, such as low-stock alerting and the request-from-template picker). It starts counting once classified.
spp_hazardis a base module with dependants, so the runs above cover both modules that inherit the incident form, not just the module under change.The branch was brought up to date by merging
19.0, not rebasing, since it was already published.