spp_hide_menus_base: hide_menu() re-snapshots default_group_ids from an already-collapsed menu, permanently losing the real groups

[gonzalesedwin1123]

Split out of #408, where it was described as the "second, quieter bug". PR #409 fixes the outage (duplicate rows aborting the registry load) but deliberately leaves hide_menu() untouched, so this remains open.

The bug

spp_hide_menus_base/models/hide_menu.py, hide_menu():

if rec.state == "show" and rec.menu_id:
    ...
    rec.default_group_ids = rec.menu_id.group_ids   # <-- snapshots whatever is there
    rec.menu_id.write({"group_ids": show_non_openspp_group})
    rec.state = "hide"

The snapshot trusts that state == "show" implies the menu's group_ids still hold the real groups. If a row is in state show while its menu is already collapsed to the hide group, the next hide_menus() run (every registry load, via _register_hook) takes the show branch and re-snapshots default_group_ids from the collapsed group_ids — overwriting the real groups with the hide group.

From that point:

• show_menu() restores group_ids = default_group_ids = the hide group, i.e. a menu nobody but the hide group can see.
• _reapply_hide() cannot repair it: the menu already carries the hide group, so its staleness check skips the row.

Whether the module can ever recover on its own depends on how the menu's groups are declared upstream:

• <record>-style group_ids (Command.set) — the owning module's next upgrade replaces group_ids, removing the hide group; _reapply_hide() then fires and re-snapshots the restored real groups. Self-heals.
• <menuitem>-style groups — convert.py's _tag_menuitem emits only Command.link/Command.unlink, never Command.set, so a reload never removes the hide group, _reapply_hide() keeps skipping, and the loss is permanent short of restoring the groups by hand.

How the desynced state arises

state and the menu's actual group_ids are only in sync as long as exactly one row governs the menu and nothing else touches either side. Known paths out of sync:

1. Duplicate rows (the spp.hide.menu: a duplicate menu_id row aborts the registry load (ValueError: Expected singleton in _register_hook) #408 scenario): two rows, one hides the menu, the other still says show. The de-dup migration in fix(spp_hide_menus_base): a duplicate menu_id row must not abort the registry load #409 keeps the row that can still restore the menu, but does not reconcile the survivor's state against the menu's actual condition — a survivor in state show with a collapsed menu degrades on the next registry load.
2. Downstream data files writing onto a governed row: a noupdate="0" seed with state = "show" re-writes show onto the row at every upgrade of that module while the menu stays collapsed. (The ir.model.data repoint requested on fix(spp_hide_menus_base): a duplicate menu_id row must not abort the registry load #409 marks the repointed xml_ids noupdate = true, which closes this path for rows adopted by the migration — but any live seed pointing at a governed row still triggers it.)
3. Any manual or programmatic write to state or to the menu's group_ids outside the hide_menu()/show_menu() pair.

Note the degradation via hide_menus()/_register_hook only auto-triggers for menus in MENU_APP; for other menus it needs an explicit hide_menu() call (e.g. from the UI).

Proposed fix

Make the snapshot self-defending rather than trying to close every desync path:

• In hide_menu(), do not overwrite default_group_ids when the menu's current group_ids is exactly the hide group — the menu is already collapsed, so whatever snapshot exists is a better guess than what would be captured. Setting state = "hide" to resynchronize is still correct; it is only the snapshot that must be skipped.
• Have the fix(spp_hide_menus_base): a duplicate menu_id row must not abort the registry load #409 de-dup migration (or a follow-up one) reconcile survivors: a row whose menu already carries only the hide group should be stored as state = "hide" without touching its snapshot.

Caveat on the guard — it fails open. A fresh row created while its menu is already collapsed gets an empty snapshot under the guard, and show_menu() on it restores group_ids = [] — a formerly restricted menu becomes visible to every user. That is the same "empty snapshot is valid" trade-off PR #409's _primary() already makes, but here it converts invisible-to-all into visible-to-all rather than the reverse. The migration reconciliation in the second bullet is the mitigation: it prevents the desynced states that make the guard fire in the first place, so the two parts belong together rather than as alternatives.

The guard is a one-line condition and is testable the same way #409 tests _primary(): construct the desynced state directly, run hide_menus(), and assert default_group_ids still holds the real groups.

Related observation (possibly its own issue)

The <menuitem> link-not-set behavior cuts the other way too: when the owning app is upgraded, its <menuitem> re-links the real groups alongside the hide group, so the menu becomes visible to those groups again with nothing re-collapsing it — _reapply_hide()'s staleness check (hide_group not in group_ids) doesn't fire because the hide group is still present. The module's "upgrade may have reset group_ids" premise only fully holds for <record>-style declarations. Filed here as a note; can be split out if confirmed on a live upgrade.

Severity

No outage — the failure is silent loss of a menu's original group_ids, discovered only when someone later unhides the menu and it stays invisible to regular users. Menus whose groups are declared <record>-style self-heal on the owning module's next upgrade; <menuitem>-style ones require restoring the groups by hand from another environment or from the owning module's XML.