From 5e920db3ee25cb9f47ae2c0a98db35c4cda9d60f Mon Sep 17 00:00:00 2001
From: Xavier-Do <xdo@odoo.com>
Date: Mon, 11 Sep 2023 13:06:38 +0200
Subject: [PATCH] [IMP] base: improve css compilation error reporting

The current solution returns two node, one for the css itself, another
for the script managing the error. This means that we must know if we
have a css error when generating the assets links, when rendering a
t-call-assets.

The proposed solution will solve this issue by managing the error
in the css file itself. A slight change will also save the error in the
attachment instead of choosing the previous one, meaning that the
error will be saved and the bundle won't be recomputed anymore.

This solution also removes the only case when a t-call-assets can return
content instead of a link, this will be cleaned in the next commit.

As a slight change, a warning will alway be displayed on the bottom of
the page, to manage the case where the javascript managing the error is
not in the page.

Part-of: odoo/odoo#131353
---
 .../static/src/core/errors/error_dialogs.xml  |  2 +-
 .../src/core/errors/scss_error_dialog.js      | 13 ++-
 addons/web/static/src/start.js                |  2 +-
 odoo/addons/base/models/assetsbundle.py       | 87 ++++++++++---------
 odoo/addons/test_assetsbundle/__manifest__.py |  3 +
 .../static/src/css/test_error.scss            |  3 +
 .../static/tests/test_css_error.js            | 28 ++++++
 .../tests/test_assetsbundle.py                | 23 +++++
 8 files changed, 115 insertions(+), 46 deletions(-)
 create mode 100644 odoo/addons/test_assetsbundle/static/src/css/test_error.scss
 create mode 100644 odoo/addons/test_assetsbundle/static/tests/test_css_error.js

diff --git a/addons/web/static/src/core/errors/error_dialogs.xml b/addons/web/static/src/core/errors/error_dialogs.xml
index a26cab4cded..848d16a9d30 100644
--- a/addons/web/static/src/core/errors/error_dialogs.xml
+++ b/addons/web/static/src/core/errors/error_dialogs.xml
@@ -85,7 +85,7 @@
           <pre t-esc="props.message"/>
         </div>
         <t t-set-slot="footer">
-          <button class="btn btn-primary o-default-button" t-on-click="onClick">Close</button>
+          <button class="btn btn-primary o-default-button" t-on-click="props.close">Close</button>
         </t>
       </Dialog>
     </t>
diff --git a/addons/web/static/src/core/errors/scss_error_dialog.js b/addons/web/static/src/core/errors/scss_error_dialog.js
index 17578758a93..bdb718d23bb 100644
--- a/addons/web/static/src/core/errors/scss_error_dialog.js
+++ b/addons/web/static/src/core/errors/scss_error_dialog.js
@@ -13,11 +13,16 @@ ScssErrorDialog.title = _t("Style error");
 const scssErrorDisplayService = {
     dependencies: ["dialog"],
     start(env, { dialog }) {
-        if (window.__odooScssCompilationError) {
-            dialog.add(ScssErrorDialog, {
-                message: window.__odooScssCompilationError,
-            });
+        const assets = [...document.styleSheets].filter((sheet) => sheet.href?.includes("/web/assets"));
+        for (const { cssRules } of assets) {
+            const lastRule = cssRules?.[cssRules?.length - 1];
+            if (lastRule?.selectorText === "css_error_message") {
+                dialog.add(ScssErrorDialog, {
+                    message: lastRule.style.content.replaceAll("\\a", "\n").replaceAll("\\*", "*").replaceAll(`\\"`, `"`),
+                });
+            }
         }
+
     },
 };
 
diff --git a/addons/web/static/src/start.js b/addons/web/static/src/start.js
index 8f6b6719883..49b33c662f5 100644
--- a/addons/web/static/src/start.js
+++ b/addons/web/static/src/start.js
@@ -26,13 +26,13 @@ export async function startWebClient(Webclient) {
     odoo.isReady = false;
 
     // setup environment
+    await whenReady();
     const env = makeEnv();
     await startServices(env);
 
     Component.env = env;
 
     // start web client
-    await whenReady();
     const app = new App(Webclient, {
         name: "Odoo Web Client",
         env,
diff --git a/odoo/addons/base/models/assetsbundle.py b/odoo/addons/base/models/assetsbundle.py
index c6e483d372b..815f9e3ef36 100644
--- a/odoo/addons/base/models/assetsbundle.py
+++ b/odoo/addons/base/models/assetsbundle.py
@@ -115,33 +115,18 @@ class AssetsBundle(object):
         """
         response = []
         if self.has_css and self.stylesheets:
-            css_attachments = self.css(is_minified=not self.is_debug_assets) or []
-            for attachment in css_attachments:
-                if self.is_debug_assets:
-                    href = self.get_debug_asset_url(extra=self.extra('css'),
-                                                    name=css_attachments.name,
-                                                    extension='')
-                else:
-                    href = attachment.url
-                response.append((href, None))
-            if self.css_errors:
-                msg = '\n'.join(self.css_errors).replace('"', '\\"').replace('\n', '\\n')
-                js_error = """
-                    window.__odooScssCompilationError = "%s";
-                    console.error("SCSS compilation failure:", window.__odooScssCompilationError);
-                    window.addEventListener("DOMContentLoaded", () => {
-                        if (!odoo || !odoo.define) {
-                            alert(window.__odooScssCompilationError);
-                        }
-                    });
-                """ % msg
-                response.append((None, js_error))
-                response.append(('/web/static/lib/bootstrap/dist/css/bootstrap.css', None))
+            css_attachment = self.css(is_minified=not self.is_debug_assets)
+            if self.is_debug_assets:
+                response.append((self.get_debug_asset_url(extra=self.extra('css'), name=css_attachment.name, extension=''), None))
+            else:
+                response.append((css_attachment.url, None))
 
         if self.has_js and self.javascripts:
             js_attachment = self.js(is_minified=not self.is_debug_assets)
-            src = self.get_debug_asset_url(name=js_attachment.name, extension='') if self.is_debug_assets else js_attachment[0].url
-            response.append((src, None))
+            if self.is_debug_assets:
+                response.append((self.get_debug_asset_url(extra=self.extra('js'), name=js_attachment.name, extension=''), None))
+            else:
+                response.append((js_attachment.url, None))
 
         return self.external_assets + response
 
@@ -578,25 +563,47 @@ class AssetsBundle(object):
     def css(self, is_minified=True):
         extension = 'min.css' if is_minified else 'css'
         attachments = self.get_attachments(extension)
-        if not attachments:
-            # get css content
-            css = self.preprocess_css()
-            if self.css_errors:
-                return self.get_attachments(extension, ignore_version=True)
+        if attachments:
+            return attachments
 
-            matches = []
-            css = re.sub(self.rx_css_import, lambda matchobj: matches.append(matchobj.group(0)) and '', css)
+        css = self.preprocess_css()
+        if self.css_errors:
+            error_message = '\n'.join(self.css_errors).replace('"', '\\"').replace('\n', '\A').replace('*', '\*')
+            previous_attachment = self.get_attachments(extension, ignore_version=True)
+            previous_css = previous_attachment.raw.decode() if previous_attachment else ''
+            css_error_message_header = '\n\n/* ## CSS error message ##*/'
+            previous_css = previous_css.split(css_error_message_header)[0]
+            css = css_error_message_header.join([
+                previous_css, """
+body::before {
+  font-weight: bold;
+  content: "A css error occured, using an old style to render this page";
+  position: fixed;
+  left: 0;
+  bottom: 0;
+  z-index: 100000000000;
+  background-color: #C00;
+  color: #DDD;
+}
 
-            if is_minified:
-                # move up all @import rules to the top
-                matches.append(css)
-                css = u'\n'.join(matches)
+css_error_message {
+  content: "%s";
+}
+""" % error_message
+            ])
+            return self.save_attachment(extension, css)
 
-                self.save_attachment(extension, css)
-                attachments = self.get_attachments(extension)
-            else:
-                return self.css_with_sourcemap(u'\n'.join(matches))
-        return attachments
+        matches = []
+        css = re.sub(self.rx_css_import, lambda matchobj: matches.append(matchobj.group(0)) and '', css)
+
+        if is_minified:
+            # move up all @import rules to the top
+            matches.append(css)
+            css = u'\n'.join(matches)
+
+            return self.save_attachment(extension, css)
+        else:
+            return self.css_with_sourcemap(u'\n'.join(matches))
 
     def css_with_sourcemap(self, content_import_rules):
         """Create the ir.attachment representing the not-minified content of the bundleCSS
diff --git a/odoo/addons/test_assetsbundle/__manifest__.py b/odoo/addons/test_assetsbundle/__manifest__.py
index 470d0ad2f49..1ab5561d25b 100644
--- a/odoo/addons/test_assetsbundle/__manifest__.py
+++ b/odoo/addons/test_assetsbundle/__manifest__.py
@@ -12,6 +12,9 @@
     ],
 
     'assets': {
+        'web.assets_tests': [
+            'test_assetsbundle/static/tests/test_css_error.js',
+        ],
         'test_assetsbundle.bundle2': [
             'test_assetsbundle/static/src/css/test_cssfile1.css',
         ],
diff --git a/odoo/addons/test_assetsbundle/static/src/css/test_error.scss b/odoo/addons/test_assetsbundle/static/src/css/test_error.scss
new file mode 100644
index 00000000000..88878b2c0b8
--- /dev/null
+++ b/odoo/addons/test_assetsbundle/static/src/css/test_error.scss
@@ -0,0 +1,3 @@
+.rule1 ()){ /* error */
+    color: black;
+}
diff --git a/odoo/addons/test_assetsbundle/static/tests/test_css_error.js b/odoo/addons/test_assetsbundle/static/tests/test_css_error.js
new file mode 100644
index 00000000000..b8cc8e98ba4
--- /dev/null
+++ b/odoo/addons/test_assetsbundle/static/tests/test_css_error.js
@@ -0,0 +1,28 @@
+/** @odoo-module **/
+
+import { registry } from "@web/core/registry";
+
+registry.category("web_tour.tours").add('css_error_tour', {
+    test: true,
+    url: '/web',
+    steps: () => [
+    {
+        content: "Error message",
+        trigger: ".modal-body",
+        run: () => {},
+    },
+]});
+
+
+registry.category("web_tour.tours").add('css_error_tour_frontend', {
+    test: true,
+    url: '/',
+    steps: () => [
+    {
+        content: "Error message",
+        trigger: ".modal-body",
+        run: () => {},
+    },
+]});
+
+// Note, the ideal steap would be `.modal-body:contains('Error: Invalid CSS after \".rule1\": expected selector, was \"()){ /* error */')` but it fails sometimes
diff --git a/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py b/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py
index 41874774b5a..d178a78cfe5 100644
--- a/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py
+++ b/odoo/addons/test_assetsbundle/tests/test_assetsbundle.py
@@ -2028,3 +2028,26 @@ class AssetsNodeOrmCacheUsage(TransactionCase):
         asset_keys, qweb_keys = self.cache_keys()
         self.assertEqual(len(asset_keys), 1, "lazy_load shouldn't create another entry")
         self.assertEqual(len(qweb_keys), 1, "lazy_load shouldn't create another entry")
+
+@tagged('-at_install', 'post_install')
+class TestErrorManagement(HttpCase):
+
+    def test_assets_bundle_css_error_backend(self):
+        self.env['ir.qweb']._get_asset_bundle('web.assets_backend', assets_params={'website_id': self.env['website'].search([], limit=1).id}).css() # force pregeneration so that we have the base style
+        self.env['ir.asset'].create({
+            'name': 'Css error',
+            'bundle': 'web.assets_backend',
+            'path': 'test_assetsbundle/static/src/css/test_error.scss',
+        })
+
+        with mute_logger('odoo.addons.base.models.assetsbundle'):
+            self.start_tour('/web', 'css_error_tour', login='admin')
+
+    def test_assets_bundle_css_error_frontend(self):
+        self.env['ir.asset'].create({
+            'name': 'Css error',
+            'bundle': 'web.assets_frontend',
+            'path': 'test_assetsbundle/static/src/css/test_error.scss',
+        })
+        with mute_logger('odoo.addons.base.models.assetsbundle'):
+            self.start_tour('/', 'css_error_tour_frontend', login='admin')