fix(spp_hide_menus_base): a duplicate menu_id row must not abort the registry load

[reichie020212]

Fixes #408.

The bug

hide_menus() reads .state off the result of a search() that can return more than one row:

hidden_menus = self.env["spp.hide.menu"].search([("menu_id", "=", menu.id)])
...
elif hidden_menus.state == "show":     # ensure_one() on a 2-record set

It is called from _register_hook(), which runs at the end of every registry load. So a second row for one menu raises ValueError: Expected singleton there, the registry never loads, and every request returns 500. The instance is unreachable until someone deletes the extra row directly in the database. This took down a DSWD 4Ps instance; the traceback and the observed rows are in #408.

Why a duplicate is easy to create

hide_menus() itself creates a row for every MENU_APP menu that lacks one, so those rows exist on any database that has ever booted, created in Python and owned by no module. A downstream module seeding its own spp.hide.menu record for one of those menus cannot adopt the existing row — its <record> carries a new xml_id and no ir.model.data points at the Python-created one — so it inserts a second.

The failure is asymmetric in the worst possible way:

	result
Fresh install	data file loads before the first _register_hook, so hide_menus() finds the seeded row and skips creating. One row. Everything passes.
Existing database	the row is already there, the seed adds a second, next registry load bricks the instance.

So it is invisible to CI and to any install-time suite, and only appears on deployment to environments that already have data.

The fix, in two halves

Neither half is sufficient alone, which is the main thing I would ask reviewers to check.

1. UNIQUE(menu_id) on spp.hide.menu makes the state unrepresentable going forward.
2. hide_menus() reads .state off _primary(), never off the search result.

The second is not belt-and-braces. Registry.post_constraint (odoo/orm/registry.py) catches any exception from applying a constraint and only logs it — _schema.error on install, _schema.info on upgrade:

except Exception as e:
    if self._is_install:
        _schema.error(*e.args)
    else:
        _schema.info(*e.args)

So a database that still holds duplicates when this lands keeps them and keeps running, unconstrained. Precisely the databases that crash are the ones that would end up without the constraint. The defensive read is what protects them.

Which row survives is not arbitrary

hide_menu() snapshots the menu's group_ids into default_group_ids and collapses group_ids to the hide group. A row created after the menu was already collapsed therefore holds nothing but the hide group, and show_menu() on it restores a menu nobody can see. _primary() and the de-dup migration both apply the same rule: prefer a row that can still restore its menu, lowest id breaking the tie. An empty snapshot is not degraded — a menu declaring no groups is correctly restored to no groups.

Migration ordering

migrations/19.0.2.1.0/pre-migrate.py is pre-migrate deliberately: migrate_module(package, 'pre') (odoo/modules/loading.py:174) precedes registry.init_models(...) (:194), where the constraint is applied. The index therefore lands on data that already satisfies it. Post-migrate would be too late — and would fail quietly, per the post_constraint behaviour above.

Tests

Three added to tests/test_hide_menu.py; nothing removed or weakened.

• test_a_menu_cannot_have_two_hide_configurations — the constraint rejects the state.
• test_primary_prefers_a_row_that_can_still_restore_its_menu — the selection rule, including that an empty snapshot is valid.
• test_hide_menus_tolerates_a_duplicate_the_constraint_could_not_block — the defensive read.

That last one drops the constraint inside the test transaction before inserting the duplicate. That is not a contrivance: it is exactly the database state post_constraint leaves behind when it swallows a failed constraint, and it is the only way to construct one. DDL is transactional in PostgreSQL, so the constraint returns on rollback.

Verification

• -i spp_hide_menus_base --test-tags /spp_hide_menus_base: 16 tests, 0 failed, 0 errors.
• Negative control: reverting only the _primary() call in hide_menus() and re-running gives 1 error(s) of 16 tests — exactly test_hide_menus_tolerates_a_duplicate_the_constraint_could_not_block, failing with ValueError: Expected singleton: spp.hide.menu(9, 10), the same error class as the production outage. So the test measures the fix rather than passing incidentally.

Two disclosures

I ran pre-commit with SKIP=oca-gen-addon-readme,bandit. Neither hook was evaluating this change, and I would rather say so than have it found later:

• oca-gen-addon-readme regenerates every module's README on any run, regardless of what is staged. On a fresh clone of 19.0 it rewrote ~6,600 lines across 90 untouched modules, which aborts the commit and would have buried a 5-file fix in a 171-file diff. That looks like drift between the committed READMEs and what the pinned hook version now generates — worth a separate look, but not something this PR should carry.
• bandit exits 2 with pyproject.toml : toml parser not available, reinstall with toml extra. I confirmed it fails identically on untouched files (spp_registry/models/registrant.py, spp_area/models/area.py), so it is a broken hook environment rather than a finding.

Every other hook passes on the changed files, including ruff, ruff-format, pylint_odoo, oca-checks-odoo-module and the openspp-* checks.

Left alone deliberately

hide_menu() and _reapply_hide() each carry their own copy of the hide-group try/except that I factored into _hide_group(). Folding them into it is an obvious cleanup, but it is unrelated to this bug, so the new helper is used only on the new path. Happy to include it if you would prefer it in one go.

[codecov]

Codecov Report

❌ Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 72.54%. Comparing base (c33d3cb) to head (c4ac87f).

Files with missing lines	Patch %	Lines
spp_hide_menus_base/models/ir_module_module.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #409      +/-   ##
==========================================
+ Coverage   71.49%   72.54%   +1.05%     
==========================================
  Files         243      376     +133     
  Lines       20785    27201    +6416     
==========================================
+ Hits        14860    19733    +4873     
- Misses       5925     7468    +1543     

Flag	Coverage Δ
spp_analytics	93.25% <ø> (?)
spp_api_v2_change_request	66.53% <ø> (ø)
spp_api_v2_cycles	71.03% <ø> (?)
spp_api_v2_data	77.77% <ø> (?)
spp_api_v2_entitlements	70.23% <ø> (?)
spp_api_v2_gis	71.57% <ø> (?)
spp_api_v2_programs	92.22% <ø> (?)
spp_approval	50.34% <ø> (?)
spp_area	80.16% <ø> (?)
spp_area_hdx	81.60% <ø> (?)
spp_base_common	91.07% <ø> (ø)
spp_case_cel	89.50% <ø> (?)
spp_case_demo	94.75% <ø> (?)
spp_case_entitlements	100.00% <ø> (?)
spp_case_programs	100.00% <ø> (?)
spp_hide_menus_base	95.29% <96.00%> (?)
spp_programs	65.27% <ø> (ø)
spp_registry	87.22% <ø> (+0.07%)	⬆️
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_hide_menus_base/models/hide_menu.py	97.95% <100.00%> (ø)
spp_hide_menus_base/models/ir_module_module.py	90.90% <83.33%> (ø)

... and 132 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.

[gonzalesedwin1123]

The root-cause analysis and the two-halves design both check out, and the test quality is genuinely good (the negative control in particular). One gap needs to be addressed before merging:

The migration leaves dangling ir.model.data behind, which turns into a failed upgrade later

migrations/19.0.2.1.0/pre-migrate.py deletes the surplus spp_hide_menu rows with raw SQL, but the downstream seed's ir.model.data entry still points at the deleted id. On the next -u of that downstream module, Odoo sees the xml_id's record is missing and recreates it (the seed is noupdate="0"), which inserts a new row for the same menu_id — now rejected by UNIQUE(menu_id). So the downstream module's upgrade fails with an IntegrityError.

The databases that will hit this are exactly the ones this PR is rescuing: any DB that held duplicates (including the DSWD 4Ps instance from #408) will de-dup cleanly on upgrade, then fail the next upgrade of the downstream module that seeded the duplicate. A loud failure pointing at the offending module is much better than the current bricked instance, but we shouldn't ship a migration that schedules a second incident.

Requested change

After collecting surplus_ids, repoint their ir.model.data entries to the surviving row instead of leaving them dangling, e.g.:

UPDATE ir_model_data d
   SET res_id = <survivor_id_for_that_menu>
 WHERE d.model = 'spp.hide.menu'
   AND d.res_id IN <surplus_ids>;

(one survivor per menu_id, so this can be driven off the same ranked CTE — rank 1 is the survivor, rank > 1 are the surplus rows). With the xml_id adopted by the survivor, the downstream module's next data load updates the existing row rather than inserting a duplicate, and no upgrade breaks.

Two things to be aware of with the repoint (both acceptable, worth a comment in the migration):

• If two seeds pointed at the same menu, both xml_ids end up on one row; that's the correct end state under the constraint.
• The downstream data file will now write its field values onto the survivor on each upgrade (e.g. state). If it writes state = "show" while the menu is already collapsed, hide_menu() will re-snapshot the collapsed group_ids into default_group_ids — the quieter degradation already described in #408. That's a pre-existing issue and out of scope here, but the repoint makes the path slightly more reachable, so it's worth noting.

Separately from the migration: the downstream seed itself should still be dropped (as #408 already says) — the repoint just guarantees the ordering doesn't matter.

[gonzalesedwin1123]

Addendum to my changes-requested review, after re-checking my own requested fix against the Odoo 19 source. Three corrections, the first one substantive:

1. The repoint alone is incomplete — the repointed ir.model.data rows must also get noupdate = true

My review asked for the repoint and reiterated that the downstream seed should eventually be dropped. As written, those two steps combined would delete the surviving row: once the seed leaves the data file, _process_end (odoo/addons/base/models/ir_model.py, ~2641–2713) garbage-collects that module's non-noupdate xml_ids that were not loaded during the upgrade and unlinks the records they point to — and the auto-created survivor has no other xml_id protecting it (#408's table shows the Python-created rows carry none). _process_end runs at loading.py:523, before _register_hook at :588, so in the same upgrade hide_menus() then finds no row for the still-collapsed menu, creates one, and snapshots the hide group — exactly the degradation now tracked in #410. So the repoint I asked for, without noupdate, trades a loud upgrade failure for silent data loss.

Setting noupdate = true on the repointed rows closes this: they are excluded from _process_end's GC query (COALESCE(noupdate, false) != true) and from upgrade-time rewrites (orm/models.py:5165, the update branch is gated on not (update and d_noupdate)). The latter also eliminates the second caveat from my review — the downstream data file can no longer write state = "show" onto the survivor — so the fix is simpler than I first described:

UPDATE ir_model_data d
   SET res_id = <survivor_id_for_that_menu>,
       noupdate = true
 WHERE d.model = 'spp.hide.menu'
   AND d.res_id IN <surplus_ids>;

The xml_id becomes inert but keeps the survivor adoptable, and the seed can then be dropped downstream in any order without consequence.

2. Correction: noupdate="0" is not load-bearing for the recreate scenario

My review said the recreation happens because the seed is noupdate="0". Wrong: the recreate branch for a dangling xml_id ignores noupdate entirely (orm/models.py:5167-5169 — record missing → the stale imd row is unlinked and the record is appended to to_create; the noupdate gate at :5165 only guards the update branch). A noupdate="1" seed dangles and recreates just the same (short of forcecreate="0").

3. Softening: "exactly the ones this PR is rescuing"

Overstated. Duplicates with no xml_id at all (e.g. two workers racing through hide_menus()'s search-then-create) de-dup with nothing left dangling, and where the seeded row wins the ranking its imd already points at the survivor. "Including the DSWD 4Ps instance from #408" is the accurate claim — per its table the seeded rows (ids 23, 24) are the higher-id non-degraded ones, so the lowest-id tie-break keeps the auto rows and dangles both seeds.

The requested change stands, with #1 folded in: de-dup, repoint the surplus rows' xml_ids to the survivor, and mark them noupdate = true.

[reichie020212]

Addressed in 8061c4bf. The requested change is in with the addendum folded in: repoint the surplus rows' xml_ids onto the survivor and mark them noupdate = true.

I checked both mechanisms against the Odoo 19 source rather than taking them from the review, and they hold:

• _process_end selects on COALESCE(noupdate, false) != %s with True bound (addons/base/models/ir_model.py:2632-2635), so noupdate = true rows are excluded from the GC — this is what makes dropping the seed downstream safe.
• The recreate branch really does ignore noupdate: r_id falsy → imd.browse(d_id).unlink() then to_create.append(data), with the not (update and d_noupdate) gate applying only to the update branch (orm/models.py:5152-5167). So your correction deps(actions): Bump actions/setup-node from 4 to 6 #2 is right, and the repoint — not the flag — is what closes the recreate path.

Two implementation notes:

The repoint runs before the DELETE. It is driven off the same ranked CTE, which re-reads spp_hide_menu; run after the delete it matches nothing and every xml_id is left dangling anyway. The CTE is now a module constant shared by the surplus lookup and the repoint, so the two cannot disagree about which row is the survivor — which matters because the survivor is chosen by the degraded-row ranking, not by id order.

noupdate is applied only to surplus xml_ids. A seed that won the ranking already points at the survivor and is left untouched, flag included: switching a healthy seed to noupdate would quietly stop that module maintaining its own record, which is a behaviour change nobody asked for. There is a test for that specifically.

Six tests drive the migration directly (importlib, the pattern spp_gis/tests/test_migration_geofence_tags.py uses) and assert on ir_model_data, because the failure is invisible from the spp.hide.menu side — the row count is right either way, and only the imd row says whether the next upgrade inserts a duplicate. Covered: the repoint, the noupdate flag, two seeds collapsing onto one row, the ranking case where the degraded row has the lower id (a repoint driven off "keep the first" would point at the deleted row), the healthy-seed no-op, and the fresh-install skip.

Negative controls, since the assertions are on a column that is easy to get accidentally right: dropping noupdate = TRUE while keeping the repoint fails exactly the three tests that assert the flag and no others; removing the repoint entirely fails the same three on res_id.

spp_hide_menus_base: 22 tests, 0 failed (16 existing + 6 new).

On your point 3 — agreed, "exactly the ones this PR is rescuing" was overstated. Duplicates created by two workers racing through hide_menus() carry no xml_id and de-dup cleanly, and a seed that wins the ranking needs no repoint. The accurate claim is the one you gave: databases where a seeded row loses the ranking, which per #408's table includes the DSWD 4Ps instance.

One thing I could not run locally: bandit fails in my environment with pyproject.toml : toml parser not available (a tool-loading error, exit 2, not a finding), so I have not seen it pass on these files. The two cr.execute call sites that concatenate the shared CTE carry # noqa: S608 # nosec B608 with a note that both operands are code-owned literals and %(hide)s is bound, matching the convention in spp_programs/models/program_membership.py. Worth a second look if CI disagrees.