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

spp_base_common/README.rst

@@ -120,6 +120,21 @@ Dependencies
 Changelog
 =========
 
+19.0.2.0.3
+~~~~~~~~~~
+
+- fix: make the menu-icon decoration run by ``ir.module.module.next()``
+  best-effort (#383). The pass now runs in its own savepoint and any
+  ``psycopg2.Error`` raised while decorating is logged and skipped
+  instead of propagating out of the module operation and leaving the
+  transaction aborted, so this hook is never the cause of a failed
+  install or upgrade; missing menu xmlids use
+  ``raise_if_not_found=False``. The caller's own pending writes are
+  flushed before the guard and stay the caller's responsibility. Odoo 19
+  only calls ``next()`` from the immediate install/upgrade buttons; the
+  registry-load exposure of the same shape lives in
+  ``spp_hide_menus_base`` and is tracked in #526.
+
 19.0.2.0.2
 ~~~~~~~~~~
 

spp_base_common/__manifest__.py

@@ -5,7 +5,7 @@
 {
     "name": "OpenSPP Base (Common)",
     "category": "OpenSPP/Core",
-    "version": "19.0.2.0.2",
+    "version": "19.0.2.0.3",
     "sequence": 1,
     "author": "OpenSPP.org",
     "website": "https://github.com/OpenSPP/OpenSPP2",

spp_base_common/models/ir_module_module.py

@@ -1,5 +1,7 @@
 import logging
 
+import psycopg2
+
 from odoo import models
 
 _logger = logging.getLogger(__name__)
@@ -73,16 +75,37 @@ class IrModuleModule(models.Model):
     }
 
     def update_menu_icons(self):
+        """Point the root menus of known third-party apps at OpenSPP icons.
+
+        Purely cosmetic and best-effort: a database error while decorating
+        must never abort the module operation that triggered it, so the
+        whole pass runs in its own savepoint and is skipped on failure.
+
+        The caller's pending ORM writes are flushed first so that only the
+        decoration itself is covered by the guard; a failure in the caller's
+        own writes stays the caller's error. Retryable errors (serialization
+        failures, deadlocks) are deliberately swallowed too: retrying the
+        module operation would rebuild the registry for a cosmetic write.
+        """
+        self.env.cr.flush()
+        try:
+            with self.env.cr.savepoint():
+                self._write_menu_icons()
+        except psycopg2.Error:
+            _logger.warning(
+                "Skipping the OpenSPP app menu icon update because the database reported an error; "
+                "the menus keep their current icons and the module operation continues",
+                exc_info=True,
+            )
+
+    def _write_menu_icons(self):
         for module in self.search([]):
             icon_info = self.ICON_MAP.get(module.name)
-            if icon_info:
-                try:
-                    menu = self.env.ref(icon_info["menu_xml_id"])
-                except ValueError:
-                    menu = False
-
-                if menu:
-                    menu.write({"web_icon": icon_info["icon"]})
+            if not icon_info:
+                continue
+            menu = self.env.ref(icon_info["menu_xml_id"], raise_if_not_found=False)
+            if menu:
+                menu.write({"web_icon": icon_info["icon"]})
 
     def next(self):
         # Call your icon update logic first

spp_base_common/readme/HISTORY.md

@@ -1,3 +1,7 @@
+### 19.0.2.0.3
+
+- fix: make the menu-icon decoration run by `ir.module.module.next()` best-effort (#383). The pass now runs in its own savepoint and any `psycopg2.Error` raised while decorating is logged and skipped instead of propagating out of the module operation and leaving the transaction aborted, so this hook is never the cause of a failed install or upgrade; missing menu xmlids use `raise_if_not_found=False`. The caller's own pending writes are flushed before the guard and stay the caller's responsibility. Odoo 19 only calls `next()` from the immediate install/upgrade buttons; the registry-load exposure of the same shape lives in `spp_hide_menus_base` and is tracked in #526.
+
 ### 19.0.2.0.2
 
 - test: add a regression test guarding the PDF backend selected by `odoo.tools.pdf`. The Docker image accidentally shipped legacy PyPDF2 3.x next to pypdf; Odoo prefers PyPDF2 when importable, and its removed 1.x API (`numPages`/`getPage`) crashes multi-record PDF printing with a `DeprecationError` (OP#1168). The fix is in `docker/Dockerfile` (`--no-deps` on the Odoo editable install); this test fails on any image that regresses.

spp_base_common/static/description/index.html

@@ -495,6 +495,22 @@ <h2><a class="toc-backref" href="#toc-entry-1">Changelog</a></h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.0.3</h1>
+<ul class="simple">
+<li>fix: make the menu-icon decoration run by <tt class="docutils literal">ir.module.module.next()</tt>
+best-effort (#383). The pass now runs in its own savepoint and any
+<tt class="docutils literal">psycopg2.Error</tt> raised while decorating is logged and skipped
+instead of propagating out of the module operation and leaving the
+transaction aborted, so this hook is never the cause of a failed
+install or upgrade; missing menu xmlids use
+<tt class="docutils literal">raise_if_not_found=False</tt>. The caller’s own pending writes are
+flushed before the guard and stay the caller’s responsibility. Odoo 19
+only calls <tt class="docutils literal">next()</tt> from the immediate install/upgrade buttons; the
+registry-load exposure of the same shape lives in
+<tt class="docutils literal">spp_hide_menus_base</tt> and is tracked in #526.</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.0.2</h1>
 <ul class="simple">
 <li>test: add a regression test guarding the PDF backend selected by
@@ -506,7 +522,7 @@ <h1>19.0.2.0.2</h1>
 this test fails on any image that regresses.</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.2.0.1</h1>
 <ul class="simple">
 <li>fix(security): add <tt class="docutils literal"><span class="pre">groups=&quot;base.group_system&quot;</span></tt> to the existing
@@ -522,7 +538,7 @@ <h1>19.0.2.0.1</h1>
 <tt class="docutils literal">group_ids</tt> to empty.</li>
 </ul>
 </div>
-<div class="section" id="section-3">
+<div class="section" id="section-4">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

spp_base_common/tests/test_ir_module_module.py

@@ -1,4 +1,29 @@
+import contextlib
+import functools
+from unittest.mock import patch
+
+import psycopg2
+import psycopg2.extensions
+
 from odoo.tests import TransactionCase
+from odoo.tools import mute_logger
+
+HOOK_LOGGER = "odoo.addons.spp_base_common.models.ir_module_module"
+DISCUSS_ICON = "spp_base_common,static/description/icon-Discuss-White-line.png"
+
+
+def _failing_lookup_for(menu_xml_id, original_lookup):
+    """Build an ``ir.model.data._xmlid_lookup`` stand-in that aborts the
+    transaction for one menu and behaves normally for every other xmlid, so the
+    other ``next()`` overrides in the MRO keep working."""
+
+    @functools.wraps(original_lookup)
+    def _xmlid_lookup(model, xmlid):
+        if xmlid == menu_xml_id:
+            model.env.cr.execute("SELECT 1 FROM spp_table_that_does_not_exist")
+        return original_lookup(model, xmlid)
+
+    return _xmlid_lookup
 
 
 class TestIRModuleModule(TransactionCase):
@@ -14,7 +39,102 @@ def test_01_update_menu_icons(self):
         # Verify that the icon was updated
         self.survey_module.next()
         menu = self.env.ref("mail.menu_root_discuss")
-        self.assertEqual(
-            menu.web_icon,
-            "spp_base_common,static/description/icon-Discuss-White-line.png",
+        self.assertEqual(menu.web_icon, DISCUSS_ICON)
+
+    def test_02_missing_menu_xmlid_is_skipped(self):
+        """An ICON_MAP entry whose menu does not exist neither raises nor blocks the others."""
+        discuss_menu = self.env.ref("mail.menu_root_discuss")
+        discuss_menu.write({"web_icon": False})
+        broken_entry = {
+            "base": {
+                "menu_xml_id": "spp_base_common.menu_that_does_not_exist",
+                "icon": "spp_base_common,static/description/icon-fast-api.png",
+            }
+        }
+
+        with patch.dict(self.IrModule.ICON_MAP, broken_entry):
+            self.IrModule.update_menu_icons()
+
+        self.assertEqual(discuss_menu.web_icon, DISCUSS_ICON)
+
+    def test_03_database_error_inside_hook_is_logged_and_skipped(self):
+        """A SQL failure while decorating is logged, and the module operation still completes."""
+        # Fail on a menu only this hook looks up, so the sibling next() override in
+        # spp_hide_menus_base (which shares every real menu xmlid) is not affected.
+        test_menu_xml_id = "spp_base_common.test_383_menu"
+        icon_entry = {"base": {"menu_xml_id": test_menu_xml_id, "icon": DISCUSS_ICON}}
+        IrModelData = type(self.env["ir.model.data"])
+        failing_lookup = _failing_lookup_for(test_menu_xml_id, IrModelData._xmlid_lookup)
+        discuss_menu = self.env.ref("mail.menu_root_discuss")
+        discuss_menu.write({"web_icon": False})
+
+        with (
+            patch.dict(self.IrModule.ICON_MAP, icon_entry),
+            patch.object(IrModelData, "_xmlid_lookup", failing_lookup),
+            mute_logger("odoo.sql_db"),
+            self.assertLogs(HOOK_LOGGER, level="WARNING") as captured,
+        ):
+            action = self.survey_module.next()
+
+        # next() returned its action instead of raising (its type depends on open todos).
+        self.assertIsInstance(action, dict)
+        self.assertIn("type", action)
+        self.assertTrue(
+            any("menu icon" in message for message in captured.output),
+            f"expected a skipped-decoration warning, got {captured.output}",
         )
+        # The transaction is still usable: the failure was contained in a savepoint.
+        self.env.cr.execute("SELECT 1")
+        self.assertEqual(self.env.cr.fetchone(), (1,))
+        # ...and the pass is atomic: the Discuss icon written before the failure was rolled back.
+        self.assertFalse(discuss_menu.web_icon)
+
+    def test_04_hook_survives_already_aborted_transaction(self):
+        """The hook must not raise when the cursor is already poisoned before it starts."""
+        # Odoo's assertRaises runs its body in a savepoint and rolls it back, which
+        # would un-poison the cursor; a bare try/except keeps the transaction aborted.
+        self.env.cr.execute("SAVEPOINT test_383_poisoned_cursor")
+        try:
+            with mute_logger("odoo.sql_db"), contextlib.suppress(psycopg2.Error):
+                self.env.cr.execute("SELECT 1 FROM spp_table_that_does_not_exist")
+            self.assertEqual(
+                self.env.cr.connection.get_transaction_status(),
+                psycopg2.extensions.TRANSACTION_STATUS_INERROR,
+                "precondition: the transaction must be aborted before the hook runs",
+            )
+
+            with mute_logger("odoo.sql_db"), self.assertLogs(HOOK_LOGGER, level="WARNING"):
+                self.IrModule.update_menu_icons()
+        finally:
+            self.env.cr.execute("ROLLBACK TO SAVEPOINT test_383_poisoned_cursor")
+            self.env.cr.execute("RELEASE SAVEPOINT test_383_poisoned_cursor")
+
+    def test_05_callers_pending_writes_are_not_swallowed(self):
+        """A failure flushing the caller's own pending writes is the caller's error.
+
+        The guard covers only the decoration pass. Pending ORM writes queued by the
+        caller are flushed before the guard, so their failure surfaces where it
+        belongs instead of being logged as a menu icon problem.
+        """
+        discuss_menu = self.env.ref("mail.menu_root_discuss")
+        self.env.cr.execute("SAVEPOINT test_383_pending_write")
+        try:
+            discuss_menu.write({"web_icon": False})  # queued, not flushed yet
+            with mute_logger("odoo.sql_db"), contextlib.suppress(psycopg2.Error):
+                self.env.cr.execute("SELECT 1 FROM spp_table_that_does_not_exist")
+            self.assertEqual(
+                self.env.cr.connection.get_transaction_status(),
+                psycopg2.extensions.TRANSACTION_STATUS_INERROR,
+                "precondition: the transaction must be aborted before the hook runs",
+            )
+
+            raised = None
+            with mute_logger("odoo.sql_db"), self.assertNoLogs(HOOK_LOGGER, level="WARNING"):
+                try:
+                    self.IrModule.update_menu_icons()
+                except psycopg2.Error as exc:
+                    raised = exc
+            self.assertIsInstance(raised, psycopg2.errors.InFailedSqlTransaction)
+        finally:
+            self.env.cr.execute("ROLLBACK TO SAVEPOINT test_383_pending_write")
+            self.env.cr.execute("RELEASE SAVEPOINT test_383_pending_write")