fix(spp_case_base): clear is_current when an intervention plan is completed by LunarCapsule127 · Pull Request #478 · OpenSPP/OpenSPP2

LunarCapsule127 · 2026-08-29T20:07:09Z

Why is this change needed?

action_complete marked a plan state = "completed" and stamped actual_end_date, but never released is_current. Since the only other writer of is_current = False was the revision path, a plan that finished normally stayed the case's current plan indefinitely.

That left every consumer of the pair reading an incoherent state: current_plan_id (derived purely from is_current) kept pointing at completed work, while "has an active plan" derivations read False. It also tripped the one-current-plan-per-case constraint, blocking anyone from marking a fresh plan as current.

Fixes #458.

How was the change implemented?

spp_case_base — the fix. Folded "is_current": False into the existing action_complete write, so completing a plan ends its tenure as the case's current plan the same way action_create_revision already does. The change goes through write(), so downstream overrides observe it.

spp_case_base — released databases. The code fix covers future completions only. Databases released at 19.0.2.0.0 still hold state = 'completed' AND is_current = true rows, so after upgrading, those cases keep the whole of #458: a stale current_plan_id and a constraint that still refuses a successor plan. Bumped to 19.0.2.0.1 and added migrations/19.0.2.0.1/post-migration.py, which demotes exactly those rows. Deliberately narrow — only completed; action_create_revision already writes is_current = False alongside revised, so that state has no stale population. The log names the affected spp.case ids, because the remedy is per-case (promote the successor plan) and a bare count cannot be acted on.

spp_case_demo — stop re-creating the bug. Two generator sites manufactured the same state, so every generated demo database reproduced #458 on a fresh install, where no migration can help:

_process_case_journey's close_case step wrote {"state": "completed"} directly, bypassing action_complete — the plan kept is_current and never got an actual_end_date.
_add_random_plan passed state = "completed" to create() alongside is_current: True.

Both now complete through action_complete(). _add_random_plan keeps the same distribution of final states and adds its interventions before completing, so a completed demo plan still has a delivery record. No migration in this module — spp_case_base's post-migration repairs the rows these sites already seeded, whichever module created them.

New unit tests

spp_case_base/tests/test_case_intervention_plan.py

test_complete_clears_is_current — an active plan is the case's current_plan_id; after action_complete(), state is completed, is_current is False, and the case reports neither a current plan nor an active one.
test_complete_frees_the_current_plan_slot — the symptom spp_case_base: action_complete never clears is_current, leaving a finished plan as the case's current plan #458 leads with: a successor plan can be created and becomes current once the previous plan completes. Without the fix this fails at the one-current-plan constraint, which is the level an accidental revert has to break.
test_complete_by_case_worker — the assigned case worker may complete a plan and release the flag, so the write is permitted under the worker record rule (own cases only), not just as superuser.
test_plan_approval_workflow (existing) now also asserts the released flag, covering the fix on the genuine draft → approved → active → completed path.

All three new tests reach active through submit → approve → activate first, via a _active_plan helper. Completing straight from draft covered a transition the UI cannot produce (the Complete button is invisible="state != 'active'"), and would have broken if a state guard were later added to action_complete instead of the production path being covered.

spp_case_base/tests/test_migration_complete_clears_is_current.py (new file) — loads the script through importlib, matching spp_gis/tests/test_migration_geofence_tags.py:

test_migration_demotes_completed_plans — releases is_current, leaves state and actual_end_date untouched.
test_migration_frees_the_current_plan_slot — a successor plan can be created after the script runs.
test_migration_leaves_unfinished_plans_alone — an active current plan stays current.
test_migration_skips_fresh_install — migrate(cr, None) returns early.

spp_case_demo/tests/test_generate_cases.py

test_journey_close_case_releases_the_current_plan — after the close_case step the plan is completed, has an actual_end_date, is not is_current, and the case reports no current plan.
test_add_random_plan_completed_plan_is_not_current — forces the random state draw to completed and asserts the same invariant, plus that interventions exist on the finished plan.

Unit tests executed by the author

./spp t <module>, Docker mode, on 2fcb709b:

Module	Result
`spp_case_base`	217 passed, 0 failed, 0 errors
`spp_case_demo`	110 passed, 0 failed, 0 errors
`spp_case_cel`	27 passed, 0 failed, 0 errors
`spp_case_registry`	32 passed, 0 failed, 0 errors
`spp_case_session`	29 passed, 0 failed, 0 errors

No tests removed; spp_case_base goes 211 → 217 and spp_case_demo 108 → 110.

Red/green checks (each test confirmed non-vacuous by reverting the thing it pins):

Model fix reverted, everything else in place → 3 failed, 1 error: test_complete_clears_is_current, test_complete_by_case_worker and test_plan_approval_workflow fail on the is_current assertion; test_complete_frees_the_current_plan_slot errors with ValidationError: Only one plan can be marked as current for a case. The migration tests correctly stay green — they do not depend on action_complete.
Migration predicate mutated (completed → revised) → 1 failed, 1 error, both in the migration test; the two non-mutation assertions correctly stay green.
Demo generator reverted → 2 failed, both new demo tests.

Migration exercised end to end, not only unit-tested. Installed spp_case_base at 19.0.2.0.0 on a real database, seeded three plans by SQL — two completed + is_current (one with actual_end_date, one without) and one active + is_current control — then bumped the manifest and ran ./spp update:

odoo.modules.migration: module spp_case_base: Running upgrade [19.0.2.0.1>] post-migration
odoo.upgrade.spp_case_base.19.0.2.0.1.post-migration: Released is_current on 2 completed
intervention plan(s) that were still flagged as their case's current plan. Cases affected
(spp.case ids): 1, 2. ...

Both stale rows flipped to is_current = false, the active control row was untouched, and on the upgraded database current_plan_id was empty, has_active_plan False, and a successor plan could be created as current.

Lint: pre-commit clean then pre-commit run --files <changed files> — all hooks pass. semgrep cannot run in this environment (TypeError: Metaclasses with custom tp_new are not supported from its protobuf dependency on Python 3.14), so CI is the check for it; the migration's SQL is a single string literal with no %, .format(), f-string or + concatenation, and the change adds no sudo() call. README.rst / static/description/index.html regenerated for both modules with the pinned generator deps, and the incidental drift the generator produces in six unrelated modules was reverted.

How to test manually

Open a case and create an intervention plan. Take it through submit → approve → activate. Confirm it shows as the case's current plan.
Complete the plan. Confirm the case no longer reports it as the current plan, and that you can now create a new plan for the case without hitting the one-current-plan constraint.
Upgrade path: on a database that predates this change and has a completed plan still flagged current, upgrade spp_case_base and confirm the log line above, that the plan is no longer current, and that a successor plan can be created.
Demo data: generate demo cases (spp_case_demo) with plans and closures enabled, then check that no plan is both completed and Is Current Plan — the Intervention Plans list shows both columns.

Expert review

Thanks for the fix — it's correct, minimal, and exactly what #458 asked for: folding "is_current": False into the existing write() means downstream overrides and tracking observe it, matching how action_create_revision already ends a plan's tenure. We verified it independently:

Green: ./spp t spp_case_base → 0 failed, 0 errors of 211 tests.
Red: with only the model file reverted to origin/19.0, exactly test_complete_clears_is_current fails on the is_current assertion — the test genuinely pins the bug.
All is_current/current_plan_id consumers survive the change (has_active_plan already filtered by state; the case-form gate reads has_active_plan; the ribbon/list column/"Current Plans" filter all become more correct), and no other module reads this model's is_current or overrides action_complete.
Lint is clean on the changed files; nothing removed or weakened in tests.

Requesting changes for two things the repo requires of every code fix to a released module, plus a coverage tweak. If you'd rather not deal with the repo-specific mechanics, say so and we're happy to push these onto your branch — the code change itself is done.

Must fix

Version bump + changelog + regenerated docs. spp_case_base shipped at 19.0.2.0.0 in release 2026.08, so this fix needs: __manifest__.py → 19.0.2.0.1, a readme/HISTORY.md fragment (OCA style, newest-first: ### 19.0.2.0.1 + a - fix(case): … bullet), and regenerated README.rst/static/description/index.html. Note the generated files are environment-sensitive — easiest is to push the fragment + bump and apply the diff CI's pre-commit job prints.
Data migration for released DBs. Existing deployments already hold state='completed' AND is_current=true rows, and this fix only covers future completions — after upgrade those cases stay blocked exactly as #458 describes. Please add spp_case_base/migrations/19.0.2.0.1/post-migration.py running the literal statement:
```
UPDATE spp_case_intervention_plan SET is_current = false WHERE state = 'completed' AND is_current = true;
```
(Keep it literal — no f-strings/format/identifier composition, or Semgrep/pylint-odoo flag it.)

Should fix

Assert the reported symptom, not just the flag. #458's user-facing complaint is "a finished plan blocks marking a new plan current". After plan.action_complete(), also create a second plan on the same case (it defaults to is_current=True) and assert it does not raise — that's the assertion an accidental revert of this fix would have to break at the constraint level.
Please fill in the "Unit tests executed by the author" section of the PR body — it still contains the template placeholder. (Our run above can serve as the evidence line if you re-run and get the same.)

Observations for maintainers (not blocking this PR — we'll file follow-ups)

action_complete has no state guard (unlike action_activate), so a draft plan can be completed over RPC; post-fix that mis-call also silently strips the case's current plan, with no way back since action_reset_to_draft refuses completed.
The #458 dead-end partially relocates: complete plan A → new plan B becomes current → "Create Revision" on A (button visible for completed plans) copies with is_current: True and hits the one-current-plan constraint with a message that names no conflicting plan.
spp_case_demo/models/generate_cases.py:381-390 manufactures exactly the #458 state (is_current: True with state randomly "completed"); the invariant "completed ⇒ not current" is enforced nowhere but this one method.
is_current lacks tracking=True while state has it, so a plan losing current status never reaches the chatter.

Nits (take or leave)

Test placement: it sits above test_create_plan, ahead of the file's create → workflow ordering; a natural home is beside test_plan_approval_workflow, which already completes a plan.
The file's convention elsewhere exercises role users (with_user(...)); a case-worker variant would also prove the write is permitted under the worker record rule.
Docstring convention in this file is "Test …".

codecov · 2026-09-01T03:43:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.88%. Comparing base (0341801) to head (2fcb709).
⚠️ Report is 45 commits behind head on 19.0.

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #478      +/-   ##
==========================================
+ Coverage   76.26%   76.88%   +0.61%     
==========================================
  Files         662      703      +41     
  Lines       44225    45740    +1515     
==========================================
+ Hits        33729    35166    +1437     
- Misses      10496    10574      +78

Flag	Coverage Δ
spp_base_common	`91.07% <ø> (ø)`
spp_case_base	`91.83% <ø> (?)`
spp_case_cel	`89.50% <ø> (?)`
spp_case_demo	`94.82% <100.00%> (?)`
spp_case_entitlements	`100.00% <ø> (ø)`
spp_case_graduation	`100.00% <ø> (?)`
spp_case_programs	`100.00% <ø> (ø)`
spp_case_registry	`100.00% <ø> (?)`
spp_case_session	`96.36% <ø> (?)`
spp_grm_case_link	`100.00% <ø> (?)`
spp_grm_cel	`86.18% <ø> (+8.94%)`	⬆️
spp_programs	`67.58% <ø> (ø)`
spp_registry	`88.94% <ø> (+1.15%)`	⬆️
spp_security	`69.56% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
spp_case_base/models/case_intervention_plan.py	`96.47% <ø> (ø)`
spp_case_demo/models/generate_cases.py	`94.23% <100.00%> (ø)`

... and 43 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

kneckinator · 2026-09-04T02:35:03Z

I'll grab the completion of this one @LunarCapsule127 as we want to include this issue in the next release of OpenSPP2

…rent plan The code fix covers future completions only. Databases released at 19.0.2.0.0 still hold rows with state='completed' AND is_current=true, so current_plan_id keeps pointing at finished work and the one-current-plan constraint keeps refusing a successor plan -- the whole of OpenSPP#458 survives the upgrade for existing deployments. Bump to 19.0.2.0.1 and demote those rows in a post-migration. Only 'completed': action_create_revision already writes is_current=False alongside 'revised', so that state has no stale population. The log names the affected cases, because the remedy is per-case (promote the successor plan) and a bare count cannot be acted on.

…rom draft The is_current tests completed a plan straight out of draft, but the Complete button is invisible unless the plan is active, so they covered a transition no user can reach -- and a future state guard on action_complete would break them rather than the production path. Route them through submit -> approve -> activate first, and assert the released flag in test_plan_approval_workflow too, which already walks that path. Also pin the symptom OpenSPP#458 leads with rather than only the flag: a successor plan can be created and made current once the previous one completes. That assertion fails at the one-current-plan constraint if the fix is reverted, which is the level an accidental revert has to break.

… plan Two generator sites manufactured the state OpenSPP#458 is about. The close_case journey step wrote {"state": "completed"} directly, and _add_random_plan passed "completed" to create() alongside is_current=True -- so every generated demo database held plans that were finished yet still their case's current plan, with no actual_end_date and no way to start a successor plan. Both now complete through action_complete(), which stamps the end date and releases the flag. _add_random_plan keeps the same distribution of final states and adds its interventions before completing, so a completed demo plan still has a delivery record. No migration here: spp_case_base's 19.0.2.0.1 post-migration repairs the rows these sites already seeded, whichever module created them.

kneckinator · 2026-09-04T03:22:55Z

Thanks @gonzalesedwin1123 — taking you up on the offer to push the repo mechanics onto the branch, since we want this in next week's release. All four items are done, plus two of the nits and one of your maintainer observations. Details and evidence are in the updated PR body; summary against your list:

Must fix

Version bump + changelog + regenerated docs — spp_case_base → 19.0.2.0.1, readme/HISTORY.md fragment newest-first, and README.rst / static/description/index.html regenerated with the pinned generator deps. Worth noting for next time: the generator has manual: true, so CI's pre-commit run --all-files never runs it and prints no diff to copy — it has to be invoked with --hook-stage manual, and it then rewrites six unrelated modules' READMEs (column widths), which I reverted.
Data migration — migrations/19.0.2.0.1/post-migration.py, statement kept literal as you asked. Two departures from your text, both deliberate:
- Added RETURNING case_id and put the affected case ids in the log line. The remedy is per-case (promote the successor plan), so a bare count can't be acted on.
- Kept it at WARNING rather than INFO, matching spp_grm_cel/migrations/19.0.2.0.2 — it's a silent data mutation an operator should see.
I also exercised it end to end rather than only unit-testing it: installed at 19.0.2.0.0, seeded two completed + is_current rows plus one active control row, then ./spp update. Both stale rows flipped, the control row was untouched, and a successor plan could then be created. Log output is quoted in the body. And there's a test — this repo already has the pattern in four modules (spp_gis/tests/test_migration_geofence_tags.py et al), so test_migration_complete_clears_is_current.py loads the script through importlib and covers the flip, the revised-untouched narrowness, the active control, and the if not version: return guard. Mutating the predicate to revised reds two of those four.

Should fix

Assert the reported symptom — test_complete_frees_the_current_plan_slot does exactly what you described, and with the model fix reverted it errors with ValidationError: Only one plan can be marked as current for a case. — failing at the constraint level, as you wanted.

One thing your review prompted that goes slightly further: all the completion tests were completing a plan straight out of draft, which the Complete button can't produce (invisible="state != 'active'"). So they'd have covered an unreachable transition, and your own observation about a missing state guard on action_complete would have broken them rather than the production path. They now go through submit → approve → activate first, and test_plan_approval_workflow — which already walked that path — asserts the released flag too.
Author's test run — filled in, with the red/green checks and the migration walkthrough.

Nits — took the test placement (now beside test_plan_approval_workflow) and the Test … docstring convention. Also took the with_user suggestion: test_complete_by_case_worker proves the write passes the worker record rule rather than only as superuser. Good call — action_complete writes is_current through write(), so that wasn't free.

Maintainer observations — folded #3 (the demo generator) into this PR, because it's the one that survives the migration: a fresh demo install recreates the bug, so a release demo would still hit "cannot mark a new plan current". There were two sites, not one — _add_random_plan seeding completed + is_current at create time, and the close_case journey step writing {"state": "completed"} directly, which also meant no actual_end_date. Both now go through action_complete(), spp_case_demo bumped to 19.0.2.0.1, two tests added. Your other three observations plus a few from my own pass are listed under "Deliberately left out" in the body with reasoning — the @api.constrains("is_current", "state") one in particular is not a release-week change, since it would reject create({"state": "completed", ...}) which existing tests here and downstream rely on.

Also cross-checked against a downstream implementation that carries a local override for this exact bug: the override becomes a no-op and can be deleted, the merged write passes its guard chain untouched, and the one real side-effect class for integrators is that is_current-based domains now exclude completed plans. Written up in the body — worth flagging in the release notes.

./spp t: spp_case_base 217 passed, spp_case_demo 110 passed, and spp_case_cel / spp_case_registry / spp_case_session all green. Re-review when you get a chance.

kneckinator · 2026-09-04T03:33:07Z

Follow-ups from the review are now filed, so nothing on the "deliberately left out" list is only living in this PR description:

spp_case_base: action_complete has no state guard — an RPC call can complete a draft plan and strand the case with no current plan #497 — action_complete has no state guard (@gonzalesedwin1123's observation 1)
spp_case_base: action_create_revision ignores who holds is_current — collides with a successor plan, and erases a completed plan's completion record #498 — action_create_revision ignores who holds is_current: collides with a successor plan, and erases a completed plan's completion record (observation 2, plus a second defect reachable today)
spp_case_base: nothing constrains is_current against state — a completed plan can be re-marked current, undoing #458 #499 — nothing constrains is_current against state, so a completed plan can be re-marked current; includes the derived-state / partial-unique-index option
spp_case_base: is_current lacks tracking=True — a plan losing current status never reaches the chatter #500 — is_current lacks tracking=True (observation 4)
chore(spp_case_base): action_complete and action_approve write per record although every value is uniform #501 — action_complete / action_approve write per record although every value is uniform

Observation 3 (the demo generator) is fixed in this PR rather than deferred. #501 is ordered after #497 in its own text, since adding the guard changes the shape of the batching.

-                    current_plan.sudo().write({"state": "completed"})
+                    # Through the action, not a bare state write: completing a
+                    # plan also stamps actual_end_date and releases is_current.
+                    current_plan.sudo().action_complete()


            )

+        if final_state == "completed":
+            plan.sudo().action_complete()


gonzalesedwin1123

Re-review: approved

Thanks @kneckinator for picking this up, and @LunarCapsule127 for the original fix. All four items from the first review are addressed, and we re-verified independently on 2fcb709b rather than taking the PR body's word for it:

Green: ./spp t spp_case_base → 0 failed, 0 errors of 217; ./spp t spp_case_demo → 0 failed, 0 errors of 110. Matches the table in the body.
Red (model fix reverted): exactly 3 failed, 1 error — test_complete_clears_is_current, test_complete_by_case_worker, test_plan_approval_workflow on the is_current assertion, and test_complete_frees_the_current_plan_slot erroring with ValidationError: Only one plan can be marked as current for a case. — the symptom-level assertion we asked for, failing at the constraint.
Red (demo generator reverted): exactly 2 failed, both new demo tests.
Migration is safe as raw SQL: spp.case.current_plan_id and has_active_plan are both store=False, so there is no stored denormalisation left stale by bypassing the ORM. The statement is a single literal (Semgrep green), the RETURNING case_id log is a genuine improvement over the bare count we asked for, and the if not version guard is covered.
Release mechanics: both manifests bumped to 19.0.2.0.1, OCA-style ### <version> fragments, and the regenerated README.rst/index.html match CI byte-for-byte — CI's pre-commit job did run "Generate addons README files from fragments" and passed. Small correction to the note in your comment: pre-commit silently ignores the manual: true key on that hook, so CI runs it in the default stage on every push; the diff you had to fish for with --hook-stage manual is the same one CI would have printed on failure.
No other open PR touches spp_case_base or spp_case_demo, so no version collision at merge time. Follow-ups #497–#501 are filed and open.
Both demo sites now reach action_complete() with the plan in active, so the demo stays compatible with an active-only guard when #497 lands.

Optional (not blocking, take or leave)

test_migration_complete_clears_is_current.py lets the script's WARNING through to the test log (twice per run). Wrapping the two demoting tests in self.assertLogs("spp_case_base_post_migration_19_0_2_0_1", level="WARNING") and asserting the case id appears in the message would both keep test output clean and pin the per-case log line you added deliberately; assertNoLogs on the no-op tests would pin the other half.

For the release notes (maintainer to-do, not this PR)

The downstream cross-check point is the one worth carrying into the 2026.09 notes: any integrator domain using plan_id.is_current as "the plan that matters" now excludes completed plans, and current_plan_id is empty after a normal completion. We'll add it when the notes are drafted.

LunarCapsule127 added 2 commits August 29, 2026 20:56

Update case_intervention_plan.py

b5eee37

Update test_case_intervention_plan.py

00f30e5

gonzalesedwin1123 requested changes Sep 1, 2026

View reviewed changes

kneckinator assigned LunarCapsule127 Sep 1, 2026

LunarCapsule127 mentioned this pull request Sep 3, 2026

Fix/473 quarantine noupdate #479

Open

kneckinator self-assigned this Sep 4, 2026

kneckinator added 3 commits September 4, 2026 10:07

kneckinator requested a review from gonzalesedwin1123 September 4, 2026 03:31

github-advanced-security Bot found potential problems Sep 4, 2026

View reviewed changes

gonzalesedwin1123 approved these changes Sep 4, 2026

View reviewed changes

gonzalesedwin1123 merged commit df808ef into OpenSPP:19.0 Sep 4, 2026
29 checks passed

LunarCapsule127 deleted the case_intervention branch September 4, 2026 11:24

Navigation Menu

Uh oh!

fix(spp_case_base): clear is_current when an intervention plan is completed - #478

fix(spp_case_base): clear is_current when an intervention plan is completed#478
gonzalesedwin1123 merged 5 commits into
OpenSPP:19.0from
LunarCapsule127:case_intervention

LunarCapsule127 commented Aug 29, 2026 •

edited by kneckinator

Loading

Uh oh!

gonzalesedwin1123 left a comment

Uh oh!

codecov Bot commented Sep 1, 2026 •

edited

Loading

Uh oh!

kneckinator commented Sep 4, 2026

Uh oh!

kneckinator commented Sep 4, 2026

Uh oh!

kneckinator commented Sep 4, 2026

Uh oh!

gonzalesedwin1123 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

LunarCapsule127 commented Aug 29, 2026 • edited by kneckinator Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why is this change needed?

How was the change implemented?

New unit tests

Unit tests executed by the author

How to test manually

Related links

Notes for reviewers

Deliberately left out of this PR

Uh oh!

gonzalesedwin1123 left a comment

Choose a reason for hiding this comment

Expert review

Must fix

Should fix

Observations for maintainers (not blocking this PR — we'll file follow-ups)

Nits (take or leave)

Uh oh!

codecov Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kneckinator commented Sep 4, 2026

Uh oh!

kneckinator commented Sep 4, 2026

Uh oh!

kneckinator commented Sep 4, 2026

Uh oh!

gonzalesedwin1123 left a comment

Choose a reason for hiding this comment

Re-review: approved

Optional (not blocking, take or leave)

For the release notes (maintainer to-do, not this PR)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LunarCapsule127 commented Aug 29, 2026 •

edited by kneckinator

Loading

codecov Bot commented Sep 1, 2026 •

edited

Loading