fix(spp_cel_widget): make the tours load on Odoo 19 so web.assets_tests stops failing every backend tour (#551) - #552
Conversation
…est page Reproduces #551: cel_widget_tour.js imports the Odoo 17 module @web_tour/tour_service/tour_utils, which no Odoo 19 file defines, so the module loader logs a console error and every backend tour fails. Tours never run in CI (no Chrome in the test image), so the check resolves imports statically with Odoo's own transpiler.
…errors One unresolvable import in any installed module's web.assets_tests makes Odoo's module loader log a console error, which fails every backend tour (#551). Test 24 opens /odoo?debug=tests on the SP-MIS stack and asserts the loader reports no errors and injects no error banner.
…/tour_utils Odoo 19 has no @web_tour/tour_service/tour_utils module, so the tours file failed to load in web.assets_tests and the module loader's console error failed every backend tour on databases with spp_cel_widget installed (auto_install). Fixes #551.
Odoo 19 validates web_tour.tours entries against {name, steps, url,
wait_for}, so "test: true" made the tours module throw on load with
"unknown key 'test'", a module loader console error that still failed
every backend tour after the import fix. Every tour is a test tour in
Odoo 19, so the key has no replacement. Refs #551.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #552 +/- ##
==========================================
+ Coverage 76.91% 77.18% +0.27%
==========================================
Files 704 733 +29
Lines 45774 47666 +1892
==========================================
+ Hits 35205 36793 +1588
- Misses 10569 10873 +304
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
The defined-modules scan missed hand-written odoo.define calls whose arguments start on the next line (e.g. spreadsheet's @odoo/o-spreadsheet wrapper), so importing one would have been reported as unresolved. The e2e comment called the sentinel module the bundle's first; it is not, and any module from web.assets_tests works.
kneckinator
left a comment
kneckinator
left a comment
There was a problem hiding this comment.
Thanks Edwin, nice work, especially finding the test: true rejection that the issue missed. I checked both fixes and both tests against Odoo 19 source: @web_tour/tour_utils is the right path, TourSchema in web_tour/static/src/js/tour_service.js only allows {name, steps, url, wait_for}, and the backend page loads web.assets_tests without ignore_missing_deps (webclient_templates.xml:306 vs :55). The e2e wait is sound too: the loader sets checkErrorProm = null and logs its errors in the same microtask, so a poll that sees null comes after the logging. No other module uses the old import path or the test key.
Approving. The inline comments are non-blocking. The three small ones I'd like to see fixed are the odoo.define scan scope, the loose unit-test assertion and pageerror. The rest are optional.
Two broader points, not for this PR:
- The import check only covers files under
/spp_cel_widget/, but the same failure can come from anyspp_*module. Moving the helper somewhere shared (e.g.spp_base_common) and running it over every installedspp_*module would guard the whole class of bug. Worth a follow-up issue. - The 10 tours still load on every backend test page while testing nothing. The next Odoo tour API change could break every backend tour again, and
spp_cel_widgetisauto_install. That makes #553 worth picking up soon rather than later.
| if header and header["alias"]: | ||
| names.add(header["alias"]) | ||
| # Files that call odoo.define by hand, e.g. the "@odoo/owl" wrapper in web/static/lib. | ||
| names.update(match["name"] for match in ODOO_DEFINE_RE.finditer(content)) |
There was a problem hiding this comment.
nit: this runs ODOO_DEFINE_RE over the raw source of every file in both bundles. An odoo.define("x", [ in a comment, JSDoc example or string adds a module that nothing defines, and with DOTALL the .+? name can run across lines into a garbage name. It can only make the check miss a problem, never fail wrongly, but it can hide a real missing import. Transpiled modules never contain hand-written defines, so scanning only when not is_odoo_module(url, content) would remove the false matches.
There was a problem hiding this comment.
Done in 169a3acf. defined_module_names now only scans the raw source for hand-written odoo.define( calls when
not is_odoo_module(url, content). A transpiled module gets its name from its path and header alias only. The new
test_defined_module_names_ignores_define_text_in_a_transpiled_module puts a JSDoc odoo.define("@example/not_a_module", …)
in an @odoo-module file. It failed before the change and passes now.
| with file_open(TOUR_FILE.lstrip("/")) as tour_file: | ||
| dependencies = module_dependencies(TOUR_FILE, tour_file.read()) | ||
| self.assertTrue( | ||
| any(name.startswith("@web_tour/") for name in dependencies), |
There was a problem hiding this comment.
nit: the old broken path @web_tour/tour_service/tour_utils also passes this check, so the test passes both before and after the fix. Asserting "@web_tour/tour_utils" in dependencies would pin the fix at unit level too.
There was a problem hiding this comment.
Done in 3d121269. It's now assertIn("@web_tour/tour_utils", dependencies). With the import put back to the pre-19
@web_tour/tour_service/tour_utils, it fails with '@web_tour/tour_utils' not found in ['@web/core/registry', '@web_tour/tour_service/tour_utils'], so the fix is pinned at unit level too.
| def _backend_test_page_scripts(self): | ||
| scripts = {} | ||
| for bundle in BACKEND_TEST_BUNDLES: | ||
| for path, full_path, _bundle, _last_modified in self.env["ir.asset"]._get_asset_paths(bundle, {}): |
There was a problem hiding this comment.
Optional: this rebuilds Odoo's asset pipeline by hand (_get_asset_paths + file_open + transpile_javascript/is_odoo_module). self.env["ir.qweb"]._get_asset_bundle(bundle).javascripts already gives JavascriptAsset objects with .url, .is_transpiled and the transpiled .content. It handles DB-backed/URL ir.asset entries (the file_open limitation you noted) and follows any future change in how Odoo decides what to transpile.
There was a problem hiding this comment.
Done in 946a3daa. _backend_test_page_scripts now returns JavascriptAssets from
self.env["ir.qweb"]._get_asset_bundle(bundle, css=False).javascripts. The test takes .is_transpiled from Odoo and
reads dependencies from the transpiled .content, so there's no local re-transpile and attachment-backed ir.asset
entries are covered.
One thing I kept from before: the defined-names side still needs the source as written. The @odoo-module alias= header
and hand-written defines live there, while the transpiled output appends the alias define with backticks, which
ODOO_DEFINE_RE doesn't match. So it reads WebAsset.content.fget(asset), the same base-class property that
JavascriptAsset.is_transpiled and .content read themselves. I checked the API against Odoo 19's assetsbundle.py and
ir_qweb.py. The test still goes red on the old import path (2 of 34 failed).
| scripts[path] = script.read() | ||
| return scripts | ||
|
|
||
| def test_imports_resolve_on_the_backend_test_page(self): |
There was a problem hiding this comment.
Optional: nothing that runs on PRs guards the second bug. If someone adds test: true (or any key outside {name, steps, url, wait_for}) to a tour, PR CI stays green and only test 24 catches it, after merge. A cheap static check on the keys passed to registry.category("web_tour.tours").add(...) in own_modules would catch it at PR time. I know you weighed this and chose to leave it to test 24, so take it or leave it.
There was a problem hiding this comment.
I've left this to e2e test 24, as before. The check needs the top-level keys of the object literal passed to
registry.category("web_tour.tours").add(...). steps is an array of object literals whose own keys (trigger, run,
content…) sit at deeper nesting, so a regex can't tell the two levels apart reliably. Doing it properly means
brace-matching that is aware of strings, comments and template literals, or a JS parser in the Python test. That's more
machinery than the bug class warrants, and it would still miss keys built at runtime. Test 24 checks what actually fails,
Odoo's TourSchema validation at load time.
The PR-time gap you point out is real: test 24 runs on merge, not on PR. I'd rather close it by running test 24 in PR CI
(or making #553's tours go away) than with a partial static check. Happy to revisit if you feel strongly.
| await login(page); | ||
| console.log("✅ Logged in as admin"); | ||
|
|
||
| page.on("console", onConsole); |
There was a problem hiding this comment.
nit: this only listens for console messages matching the four loader patterns. An uncaught exception from a test asset (a script that throws outside odoo.define, or a factory whose failure the loader doesn't report later) also fails the tour runner, and this test would still pass. Also collecting page.on("pageerror") and asserting that list is empty closes the gap for about two lines.
There was a problem hiding this comment.
Done in e6beb9a3. Test 24 now collects page.on("pageerror") and asserts expect(pageErrors).toEqual([]), and detaches
the listener in the finally. For the red check I temporarily added
setTimeout(() => { throw new Error("pageerror-probe"); }) to the tour file, so the throw happened outside anything the
loader reports. The test failed with + "pageerror-probe". Without the probe, e2e 01 and 24 pass.
| null, | ||
| {timeout: 30_000} | ||
| ); | ||
| await page.waitForLoadState("load"); |
There was a problem hiding this comment.
nit: this looks redundant. The sentinel is in the deferred web.assets_tests script, and the loader's error report has already run by the time checkErrorProm === null is seen. It suggests a timing dependency that doesn't exist. Harmless either way.
There was a problem hiding this comment.
Done in 73329dfb: removed. You're right that the sentinel is defined by the deferred web.assets_tests script, and the
loader has reported by the time checkErrorProm === null. Test 24 still passes without the wait.
…the tour-file unit test The old assertion accepted any @web_tour/ import, so it passed with the pre-19 @web_tour/tour_service/tour_utils path too (PR #552 review).
… odoo.define calls
A transpiled module never contains a hand-written define, so an
odoo.define("x", [ in its comments, JSDoc or strings added a module
nothing defines and could hide a real missing import (PR #552 review).
…asset bundles Use ir.qweb._get_asset_bundle(...).javascripts instead of rebuilding the pipeline from ir.asset paths and file_open: Odoo's JavascriptAsset decides what is transpiled and serves the transpiled content, and it also reads ir.asset entries stored as attachments (PR #552 review).
The sentinel is defined by the deferred web.assets_tests script, and the
loader has reported its errors by the time checkErrorProm is null, so the
extra waitForLoadState("load") only suggested a timing dependency that
does not exist (PR #552 review).
An uncaught exception from a test asset, such as a script that throws outside odoo.define, also fails the tour runner without a module-loader console message. Collect pageerror events and assert there are none (PR #552 review).
|
Thanks for the careful review. I checked all six inline comments and changed five of them. Each change is its own
I proved every tightened check red before calling it green:
The static tour-key check stays with test 24; the reasons are in the thread. I filed #578 for your shared |
2 participants
Fixes #551.
Problem
spp_cel_widget/static/tests/tours/cel_widget_tour.jsis inweb.assets_tests, which the backend page (web.webclient_bootstrap) loads whenever tests are enabled or?debug=testsis on, without theignore_missing_depsexemption the frontend layout gets. Two Odoo 17 leftovers stopped the file from loading on 19.0, and each made Odoo's module loader log a console error. The tour runner fails on any console error, so every backend tour failed on a database withspp_cel_widgetinstalled, and it isauto_install.import {stepUtils} from "@web_tour/tour_service/tour_utils": that module doesn't exist in Odoo 19. It's@web_tour/tour_utils(web_tour/static/src/tour_utils.js). Loader error: "needed by other modules but have not been defined: @web_tour/tour_service/tour_utils".web_tour.toursentries against{name, steps, url, wait_for}, and all 10 tours set the pre-18 keytest: true: "Validation error for key "cel_widget_basic_rendering" in registry "web_tour.tours": Invalid object: unknown key 'test'". So the one-line fix proposed in the issue is not enough on its own.We never saw this because the test image has no Chrome (Odoo skips every browser test) and nothing in the repo calls
start_tour.Fix
stepUtilsfrom@web_tour/tour_utils.test: truefrom the 10 tours. Every tour is a test tour in Odoo 19, so the key has no replacement.url,steps, the step keys and allrun:actions (click,edit, functions) are valid in 19.spp_cel_widget19.0.2.0.0 → 19.0.2.0.1 with a HISTORY fragment. README/index.html are left for CI's pinned generator.Tests
spp_cel_widget/tests/test_assets.py, runs in normal CI): lists every JS module defined on the backend test page (web.assets_web+web.assets_tests, viair.asset._get_asset_pathsand Odoo's ownjs_transpiler) and asserts that every import in this module's JS resolves. 5 unit tests cover the helpers, so the check can't pass by matching nothing. Pre-fix it fails with exactly{'/spp_cel_widget/static/tests/tours/cel_widget_tour.js': ['@web_tour/tour_service/tour_utils']}. Every other import in the module resolves. Post-fix:0 failed, 0 error(s) of 33 tests(27 existing + 6 new). The log's WARNING lines are identical to this branch's pre-fix run (10 distinct, all pre-existing); I didn't take a separate 19.0 baseline.e2e/tests/01-spp-starter-spmis.spec.ts, new test 24): opens/odoo?debug=testson the SP-MIS stack (which installsspp_cel_widgetthroughspp_programs) and asserts the module loader logs no errors and injects no error banner. It waits onodoo.loaderstate, notnetworkidle, which never settles because of the bus websocket worker. It's generic, so it guards every installed module's test assets. Run locally (tests 01 + 24) at each stage:@web_tour/tour_service/tour_utils;unknown key 'test'. This is how finding 2 above was found; the static check can't see a runtime registry validation;cel_widget_*tours are registered in the browser.All pre-commit hooks pass on the changed files, including eslint and semgrep.
Review notes (adversarial review run before marking ready)
No blockers. The review re-verified the fix and both tests against Odoo 19 source. It also ran the parsing helpers over every
static/**/*.jsin the image: the transpiler output always matched, and the firstodoo.definewas always the module path. Applied:odoo.define(calls whose arguments start on the next line (spreadsheet's@odoo/o-spreadsheetwrapper). Now tolerant, with a unit test mirroring that file that fails on the old regex.@web/../tests/legacy/utilsthe bundle's first module. Any module fromweb.assets_testsworks.Considered, not changed:
describe.serialsuite, so it is skipped if tests 01–23 fail, and it runs post-merge only. The Python test is the PR-time guard.testkey slipped past it). That's what test 24 is for.file_openon a non-stringfull_path(external URL / DBir.asset): nothing adds either to these bundles in the repo or the image. Not hardened speculatively.Not changed → follow-up issue
The tours are loadable now, but still dead. No Python test runs them, and every one navigates via
spp_programs.spp_manager_menu_root/menu_eligibility_managers, which don't exist in OpenSPP2 (legacy openspp-modules UI).spp_cel_widget/tests/README.mddocuments tour commands that run nothing. Out of scope here, so it's filed as #553 (revive them against the module's ownspp.cel.widget.demowizard, or delete them).