[18.0][IMP] report_py3o: improve error observability in LO conversion chain

report_py3o/controllers/main.py

@@ -1,6 +1,7 @@
 # Copyright 2017 ACSONE SA/NV
 # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl)
 import json
+import logging
 import mimetypes
 from urllib.parse import parse_qs
 
@@ -12,6 +13,8 @@
 
 from odoo.addons.web.controllers.report import ReportController as ReportControllerBase
 
+logger = logging.getLogger(__name__)
+
 
 class ReportController(ReportControllerBase):
     @route()
@@ -41,8 +44,7 @@ def report_routes(self, reportname, docids=None, converter=None, **data):
         ).with_context(**context)
         if not action_py3o_report:
             raise exceptions.HTTPException(
-                description="Py3o action report not found for report_name "
-                f"{reportname}"
+                description=f"Py3o action report not found for report_name {reportname}"
             )
         res, filetype = ir_action._render(reportname, docids, data)
         filename = action_py3o_report.gen_report_download_filename(docids, data)
@@ -92,6 +94,7 @@ def report_download(self, data, context=None, token=None, readonly=True):
             response.set_cookie("fileToken", context)
             return response
         except Exception as e:
+            logger.exception("Error while generating py3o report: %s", reportname)
             se = serialize_exception(e)
             error = {"code": 200, "message": "Odoo Server Error", "data": se}
             return request.make_response(html_escape(json.dumps(error)))

report_py3o/models/py3o_report.py

@@ -15,7 +15,7 @@
 from zipfile import ZIP_DEFLATED, ZipFile
 
 from odoo import _, api, fields, models, tools
-from odoo.exceptions import AccessError
+from odoo.exceptions import AccessError, UserError
 from odoo.tools.safe_eval import safe_eval, time
 
 from ._py3o_parser_context import Py3oParserContext
@@ -98,7 +98,7 @@ def _is_valid_template_path(self, path):
         is_valid = real_path.startswith(root_path + os.path.sep)
         if not is_valid:
             logger.warning(
-                "Py3o template path is not valid. %s is not a child of root " "path %s",
+                "Py3o template path is not valid. %s is not a child of root path %s",
                 real_path,
                 root_path,
             )
@@ -262,16 +262,56 @@ def _convert_single_report(self, result_path, model_instance, data):
                     user_installation=tmp_user_installation,
                 )
                 logger.debug("Running command %s", command)
-                output = subprocess.check_output(
-                    command, cwd=os.path.dirname(result_path)
-                )
+                try:
+                    output = subprocess.check_output(
+                        command,
+                        cwd=os.path.dirname(result_path),
+                        stderr=subprocess.STDOUT,
+                    )
+                except FileNotFoundError:
+                    raise UserError(
+                        _(
+                            "LibreOffice binary not found: %s.\n"
+                            "Please check the py3o conversion command "
+                            "configuration."
+                        )
+                        % command[0]
+                    ) from None
+                except subprocess.CalledProcessError as exc:
+                    logger.error(
+                        "LibreOffice conversion failed.\n"
+                        "Exit code: %d\nCommand: %s\nOutput: %s",
+                        exc.returncode,
+                        " ".join(command),
+                        exc.output.decode("utf-8", errors="replace"),
+                    )
+                    raise UserError(
+                        _(
+                            "The report could not be generated. "
+                            "Please contact your administrator."
+                        )
+                    ) from exc
                 logger.debug("Output was %s", output)
                 self._cleanup_tempfiles([result_path])
                 result_path, result_filename = os.path.split(result_path)
                 result_path = os.path.join(
                     result_path,
                     f"{os.path.splitext(result_filename)[0]}.{self.ir_actions_report_id.py3o_filetype}",
                 )
+                if not os.path.exists(result_path):
+                    logger.error(
+                        "LibreOffice reported success but output file not found: %s.\n"
+                        "Stderr output: %s",
+                        result_path,
+                        output.decode("utf-8", errors="replace")[-2000:],
+                    )
+                    raise UserError(
+                        _(
+                            "The report could not be generated. "
+                            "The template may contain unsupported content. "
+                            "Please contact your administrator."
+                        )
+                    )
         return result_path
 
     def _convert_single_report_cmd(
@@ -280,7 +320,7 @@ def _convert_single_report_cmd(
         """Return a command list suitable for use in subprocess.call"""
         lo_bin = self.ir_actions_report_id.lo_bin_path
         if not lo_bin:
-            raise RuntimeError(
+            raise UserError(
                 _(
                     "Libreoffice runtime not available. "
                     "Please contact your administrator."

report_py3o/tests/test_report_py3o.py

@@ -5,6 +5,7 @@
 import logging
 import os
 import shutil
+import subprocess
 import tempfile
 from base64 import b64decode, b64encode
 from contextlib import contextmanager
@@ -20,11 +21,15 @@
     # For PyPDF2 >= 2.0.0
     from PyPDF2 import PageObject
 
-from odoo import tools
-from odoo.exceptions import ValidationError
+import json
+
+from odoo import http, tools
+from odoo.exceptions import UserError, ValidationError
+from odoo.tests import common
 from odoo.tests.common import TransactionCase
 
 from odoo.addons.base.tests.test_mimetypes import PNG
+from odoo.addons.report_py3o.controllers.main import ReportController
 
 from ..models._py3o_parser_context import format_multiline_value
 from ..models.ir_actions_report import PY3O_CONVERSION_COMMAND_PARAMETER
@@ -75,9 +80,8 @@ def _render_patched(self, result_text="test result", call_count=1):
             result = tempfile.mktemp(".txt")
             with open(result, "w") as fp:
                 fp.write(result_text)
-            patched_pdf.side_effect = (
-                lambda record, data: py3o_report_obj._postprocess_report(record, result)
-                or result
+            patched_pdf.side_effect = lambda record, data: (
+                py3o_report_obj._postprocess_report(record, result) or result
             )
             # test the call the the create method inside our custom parser
             self.report._render(self.report.id, self.env.user.ids)
@@ -252,7 +256,7 @@ def test_py3o_report_availability(self):
         self.assertFalse(self.report.is_py3o_native_format)
         self.assertTrue(self.report.is_py3o_report_not_available)
         self.assertTrue(self.report.msg_py3o_report_not_available)
-        with self.assertRaises(RuntimeError):
+        with self.assertRaises(UserError):
             self.report._render(self.report.id, self.env.user.ids)
 
         # if we reset the wrong path, everything should work
@@ -266,3 +270,53 @@ def test_py3o_report_availability(self):
         self.assertFalse(self.report.msg_py3o_report_not_available)
         res = self.report._render(self.report.id, self.env.user.ids)
         self.assertTrue(res)
+
+    @tools.misc.mute_logger("odoo.addons.report_py3o.models.py3o_report")
+    def test_convert_single_report_called_process_error(self):
+        self.report.py3o_filetype = "pdf"
+        py3o_report = self.env["py3o.report"].create(
+            {"ir_actions_report_id": self.report.id}
+        )
+        result_path = tempfile.mkstemp(suffix=".odt")[1]
+        self.addCleanup(os.unlink, result_path)
+        with mock.patch("subprocess.check_output") as mock_co:
+            mock_co.side_effect = subprocess.CalledProcessError(
+                returncode=1, cmd="libreoffice", output=b"test error output"
+            )
+            with self.assertRaises(UserError):
+                py3o_report._convert_single_report(result_path, self.env.user, {})
+
+
+class TestReportPy3oController(common.HttpCase):
+    def setUp(self):
+        super().setUp()
+        self.session = self.authenticate("admin", "admin")
+
+    @tools.misc.mute_logger("odoo.addons.web.controllers.report")
+    def test_report_download_error_logging(self):
+        with (
+            mock.patch.object(ReportController, "report_routes") as route_patch,
+            self.assertLogs(
+                "odoo.addons.report_py3o.controllers.main", level=logging.ERROR
+            ) as cm,
+        ):
+            route_patch.side_effect = Exception("Test error")
+            self.get_report_headers(
+                suffix="/report/py3o/report_py3o.res_users_report_py3o/1",
+                f_type="py3o",
+            )
+            [msg] = cm.output
+            self.assertIn("Error while generating py3o report", msg)
+
+    def get_report_headers(
+        self,
+        suffix="/report/py3o/report_py3o.res_users_report_py3o/1",
+        f_type="py3o",
+    ):
+        return self.url_open(
+            url="/report/download",
+            data={
+                "data": json.dumps([suffix, f_type]),
+                "csrf_token": http.Request.csrf_token(self),
+            },
+        )