fix(spp_base_common): make the menu-icon hook best-effort on database errors

[gonzalesedwin1123]

Summary

Closes #383.

update_menu_icons() in spp_base_common/models/ir_module_module.py ran search, env.ref and write against the cursor with no guard. Any psycopg2 error propagated out of ir.module.module.next() and left the caller's transaction aborted. Decorating app icons is cosmetic, so the whole pass now runs in its own savepoint, and a psycopg2.Error is logged at WARNING and skipped. Missing menu xmlids use raise_if_not_found=False instead of catching ValueError.

• psycopg2.Error rather than DatabaseError: InterfaceError ("connection already closed") is a sibling class, not a subclass (same lesson as fix(worker): survive a job-level database error instead of quarantining the DB odoo-job-worker#22).
• Whole-body savepoint rather than a per-lookup guard: search([]) and write are just as exposed on a poisoned cursor as the lookup, and one database error abandoning the rest of a cosmetic pass is acceptable.
• spp_base_common 19.0.2.0.2 → 19.0.2.0.3, HISTORY fragment added. README.rst left to CI's generator.

Caveat on the issue's premise

On Odoo 19, ir.module.module.next() is called from exactly one place, _button_immediate_function (the install/upgrade/uninstall buttons), after cr.reset(). Plain registry load never runs this hook, so the "kills a job worker on registry load" framing inherited from odoo-job-worker#22 does not match this code path. The hardening is still right on the hook's own contract, which is what the tests assert.

The hook that does run on every registry load with the same unguarded env.ref → _xmlid_lookup shape is spp_hide_menus_base's _register_hook → hide_menus() (present since 2026-06-04, before the 2026-07-30 incident). That is almost certainly the frame the incident traceback showed. It is a different module with its own version, so it is filed separately rather than folded in here; see #526.

Tests

./spp t spp_base_common: 10 tests, 0 failed, 0 errors (baseline 7). New tests in test_ir_module_module.py:

• missing menu xmlid in ICON_MAP is skipped and the other menus are still decorated;
• a real SQL failure raised inside the lookup is logged, next() still returns its action, and the cursor is usable afterwards (savepoint contained it);
• the cursor is already aborted when the hook starts (TRANSACTION_STATUS_INERROR asserted as precondition): update_menu_icons() does not raise.

Both guard tests were RED before the fix (UndefinedTable / InFailedSqlTransaction propagating out of the hook). Two test-design notes worth knowing: Odoo's assertRaises wraps its body in a savepoint and rolls it back, so it cannot be used to poison a cursor; and every menu in ICON_MAP is also in spp_hide_menus_base.MENU_APP, so the failing lookup targets a test-only xmlid to keep the sibling next() override out of the picture.

Lint: pre-commit run (ruff, ruff-format, pylint_odoo, OpenSPP checks, bandit, semgrep) clean on the changed files.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.93%. Comparing base (1a3c591) to head (2540c79).

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #525      +/-   ##
==========================================
+ Coverage   76.91%   76.93%   +0.02%     
==========================================
  Files         704      732      +28     
  Lines       45774    47914    +2140     
==========================================
+ Hits        35205    36862    +1657     
- Misses      10569    11052     +483     

Flag	Coverage Δ
spp_analytics	93.25% <ø> (ø)
spp_api_v2_change_request	73.37% <ø> (ø)
spp_api_v2_cycles	71.03% <ø> (ø)
spp_api_v2_data	77.77% <ø> (ø)
spp_api_v2_entitlements	70.23% <ø> (ø)
spp_api_v2_gis	74.60% <ø> (ø)
spp_api_v2_programs	92.22% <ø> (ø)
spp_api_v2_service_points	71.03% <ø> (ø)
spp_api_v2_simulation	71.19% <ø> (ø)
spp_approval	50.85% <ø> (ø)
spp_area	80.16% <ø> (?)
spp_area_hdx	81.60% <ø> (?)
spp_audit	72.13% <ø> (?)
spp_base_common	91.52% <100.00%> (+0.45%)	⬆️
spp_case_cel	89.50% <ø> (ø)
spp_case_demo	94.82% <ø> (ø)
spp_programs	67.58% <ø> (ø)
spp_registry	89.00% <ø> (ø)
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_base_common/models/ir_module_module.py	100.00% <100.00%> (ø)

... and 28 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]

Review round (3 reviewers) and fix-ups → e913d3b

Three independent reviews were run on 9a14fec: the repo's code-reviewer profile, an adversarial staff-engineer pass over seven attack surfaces (savepoint/flush semantics on an aborted transaction, cursor state for super().next(), swallowed retryable errors, ORM cache after rollback, tests-as-proof incl. ormcache/patch leakage and TransactionCase interaction, MRO order vs the spp_hide_menus_base override, housekeeping), and a verify-module pass with a negative control.

Verdicts: no blocking findings. Negative control confirmed the tests bite: with only the guard reverted, test_03 errors with UndefinedTable and test_04 with InFailedSqlTransaction; tests 1 and 2 still pass.

One Important finding, fixed in e913d3b: cr.savepoint() is a _FlushingSavepoint, so it flushed the caller's pending ORM writes inside the guard. A failure there was swallowed and logged as a menu-icon problem, and the caller only discovered the aborted transaction at its next statement. Latent today (the sole caller, _button_immediate_function, calls next() right after cr.reset()), but the guard now covers only the decoration: self.env.cr.flush() runs before the try. New test_05 pins this (pending write + aborted cursor → the flush error surfaces, no hook warning); it was RED before the change.

Optional items adopted: docstring records why retryable OperationalErrors are deliberately swallowed (a retry would rebuild the registry for a cosmetic write); operator-facing warning text; worker renamed _write_menu_icons; test_03 now also asserts the pass is atomic (an icon written before the failure is rolled back) and no longer couples to the act_url branch of next(); test_04/05 use the public cr.connection.get_transaction_status() and release their savepoints; functools.wraps on the lookup stand-in keeps __cache__ for ormcache callers; HISTORY wording no longer overstates the guarantee and points at #526 for the registry-load exposure.

Not adopted: renaming cls.survey_module (pre-existing, out of scope).

Local: ./spp t spp_base_common 11 tests, 0 failed, 0 errors, no leaked warnings. HISTORY changed, so CI's README generator will produce a new rendering to apply.