From 0cec54b92c01fd559464c871ca78c87aac191ac8 Mon Sep 17 00:00:00 2001 From: "Lucas Perais (lpe)" Date: Wed, 23 Jun 2021 15:20:21 +0000 Subject: [PATCH] [FIX] web: action_service: better error handling on a legacy form view, do an action in target new The view in target new should, during its onchange, trigger a ValidationError Before this commit, the dialog_container was stuck in an error loop. This was because the received error was never an instance of Error, rather it was a legacy error event. So it was never transmitted upstream were the ErrorHandler could remove the dialog in error from is dialog array. After this commit, the ValidationError mesage is displayed. This commit additionally fixes another issue: An action in target main that failed to render erased the whole DOM. This is not what we want, at least for now. Imagine a list view, open a record, this record fails to load. We still want to be on the list view, not in a blank state. --- .../src/webclient/actions/action_container.js | 15 ---- .../src/webclient/actions/action_service.js | 16 ++-- .../webclient/actions/error_handling_tests.js | 5 +- .../webclient/actions/load_state_tests.js | 45 ++++++++++- .../webclient/actions/window_action_tests.js | 81 ++++++++++++++++++- 5 files changed, 136 insertions(+), 26 deletions(-) diff --git a/addons/web/static/src/webclient/actions/action_container.js b/addons/web/static/src/webclient/actions/action_container.js index f6d856902a6..7bb144f229d 100644 --- a/addons/web/static/src/webclient/actions/action_container.js +++ b/addons/web/static/src/webclient/actions/action_container.js @@ -20,21 +20,6 @@ export class ActionContainer extends Component { this.env.bus.off("ACTION_MANAGER:UPDATE", this); super.destroy(); } - - catchError() { - this.info = {}; - this.render(); - // we trigger here the 'MENUS-APP-CHANGED' event to make sure the navbar - // is properly displayed/updated. - this.env.bus.trigger("MENUS:APP-CHANGED"); - - // we do not rethrow the error here, because the controller component - // already caught the error, and rejected the doAction promise. That - // doAction promise will then trigger an unhandledRejection error, which - // will be displayed by the error service (unless the promise is handled - // by some caller code, but then in that case, it is its own responsibility - // to handle the error properly. - } } ActionContainer.components = { ActionDialog }; ActionContainer.template = tags.xml` diff --git a/addons/web/static/src/webclient/actions/action_service.js b/addons/web/static/src/webclient/actions/action_service.js index f7c3d540ecf..144c9173b08 100644 --- a/addons/web/static/src/webclient/actions/action_service.js +++ b/addons/web/static/src/webclient/actions/action_service.js @@ -458,11 +458,15 @@ function makeActionManager(env) { } } catchError(error) { - // The above component should truely handle the error reject(error); - // Re-throw in case it is a programming error - if (error && error.name) { + cleanDomFromBootstrap(); + if (action.target === "new") { + // get the dialog service to close the dialog. throw error; + } else { + const lastCt = controllerStack[controllerStack.length - 1]; + const info = lastCt ? lastCt.__info__ : {}; + env.bus.trigger("ACTION_MANAGER:UPDATE", info); } } mounted() { @@ -582,11 +586,13 @@ function makeActionManager(env) { const nextStack = controllerStack.slice(0, index).concat(controllerArray); controller.props.breadcrumbs = _getBreadcrumbs(nextStack.slice(0, nextStack.length - 1)); const closingProm = _executeCloseAction(); - env.bus.trigger("ACTION_MANAGER:UPDATE", { + + controller.__info__ = { id: ++id, Component: ControllerComponent, componentProps: controller.props, - }); + }; + env.bus.trigger("ACTION_MANAGER:UPDATE", controller.__info__); return Promise.all([currentActionProm, closingProm]).then((r) => r[0]); } diff --git a/addons/web/static/tests/webclient/actions/error_handling_tests.js b/addons/web/static/tests/webclient/actions/error_handling_tests.js index 29cb0b27920..a156c48ba8a 100644 --- a/addons/web/static/tests/webclient/actions/error_handling_tests.js +++ b/addons/web/static/tests/webclient/actions/error_handling_tests.js @@ -24,12 +24,13 @@ QUnit.module("ActionManager", (hooks) => { const webClient = await createWebClient({ serverData }); assert.strictEqual(webClient.el.querySelector(".o_action_manager").innerHTML, ""); await doAction(webClient, "1"); - assert.ok(webClient.el.querySelector(".o_action_manager").innerHTML !== ""); + const contents = webClient.el.querySelector(".o_action_manager").innerHTML; + assert.ok(contents !== ""); try { await doAction(webClient, "Boom"); } catch (e) { assert.ok(e instanceof TypeError); } - assert.strictEqual(webClient.el.querySelector(".o_action_manager").innerHTML, ""); + assert.strictEqual(webClient.el.querySelector(".o_action_manager").innerHTML, contents); }); }); diff --git a/addons/web/static/tests/webclient/actions/load_state_tests.js b/addons/web/static/tests/webclient/actions/load_state_tests.js index 657e60e8706..a1f007f5c57 100644 --- a/addons/web/static/tests/webclient/actions/load_state_tests.js +++ b/addons/web/static/tests/webclient/actions/load_state_tests.js @@ -8,8 +8,7 @@ import core from "web.core"; import AbstractAction from "web.AbstractAction"; import { registerCleanup } from "../../helpers/cleanup"; import { makeTestEnv } from "../../helpers/mock_env"; -import { makeFakeRouterService } from "../../helpers/mock_services"; -import { getFixture, legacyExtraNextTick, patchWithCleanup } from "../../helpers/utils"; +import { click, getFixture, legacyExtraNextTick, patchWithCleanup } from "../../helpers/utils"; import { createWebClient, doAction, @@ -17,13 +16,13 @@ import { loadState, setupWebClientServiceRegistry, } from "./../helpers"; +import { errorService } from "@web/core/errors/error_service"; const { Component, mount, tags } = owl; let serverData; const actionRegistry = registry.category("actions"); -const serviceRegistry = registry.category("services"); QUnit.module("ActionManager", (hooks) => { hooks.beforeEach(() => { @@ -927,4 +926,44 @@ QUnit.module("ActionManager", (hooks) => { ); } ); + + QUnit.test("initial action crashes", async (assert) => { + assert.expect(7); + + const handler = (ev) => { + // need to preventDefault to remove error from console (so python test pass) + ev.preventDefault(); + }; + window.addEventListener("unhandledrejection", handler); + registerCleanup(() => window.removeEventListener("unhandledrejection", handler)); + + patchWithCleanup(QUnit, { + onUnhandledRejection: () => {}, + }); + + browser.location.hash = "#action=__test__client__action__&menu_id=1"; + const ClientAction = registry.category("actions").get("__test__client__action__"); + class Override extends ClientAction { + setup() { + super.setup(); + assert.step("clientAction setup"); + throw new Error("my error"); + } + } + registry.category("actions").add("__test__client__action__", Override, { force: true }); + + registry.category("services").add("error", errorService); + + const webClient = await createWebClient({ serverData }); + assert.verifySteps(["clientAction setup"]); + await click(webClient.el, "nav .o_navbar_apps_menu .o_dropdown_toggler "); + assert.containsN(webClient, ".o_dropdown_item.o_app", 3); + assert.containsNone(webClient, ".o_menu_brand"); + assert.containsOnce(webClient, ".o_dialog_error"); + assert.strictEqual(webClient.el.querySelector(".o_action_manager").innerHTML, ""); + assert.deepEqual(webClient.env.services.router.current.hash, { + action: "__test__client__action__", + menu_id: 1, + }); + }); }); diff --git a/addons/web/static/tests/webclient/actions/window_action_tests.js b/addons/web/static/tests/webclient/actions/window_action_tests.js index a14870c5c60..38d8536ad68 100644 --- a/addons/web/static/tests/webclient/actions/window_action_tests.js +++ b/addons/web/static/tests/webclient/actions/window_action_tests.js @@ -10,8 +10,18 @@ import FormView from "web.FormView"; import ListController from "web.ListController"; import testUtils from "web.test_utils"; import legacyViewRegistry from "web.view_registry"; -import { click, legacyExtraNextTick, nextTick, patchWithCleanup } from "../../helpers/utils"; +import { + click, + legacyExtraNextTick, + makeDeferred, + nextTick, + patchWithCleanup, +} from "../../helpers/utils"; import { createWebClient, doAction, getActionManagerServerData, loadState } from "./../helpers"; +import { errorService } from "../../../src/core/errors/error_service"; +import { RPCError } from "@web/core/network/rpc_service"; +import { registerCleanup } from "../../helpers/cleanup"; +import { WarningDialog } from "@web/core/errors/error_dialogs"; let serverData; const serviceRegistry = registry.category("services"); @@ -2294,4 +2304,73 @@ QUnit.module("ActionManager", (hooks) => { }); } ); + + QUnit.test("window action in target new fails (onchange)", async (assert) => { + assert.expect(3); + + /* + * By-pass QUnit's and test's error handling because the error service needs to be active + */ + const handler = (ev) => { + // need to preventDefault to remove error from console (so python test pass) + ev.preventDefault(); + }; + window.addEventListener("unhandledrejection", handler); + registerCleanup(() => window.removeEventListener("unhandledrejection", handler)); + + patchWithCleanup(QUnit, { + onUnhandledRejection: () => {}, + }); + + const originOnunhandledrejection = window.onunhandledrejection; + window.onunhandledrejection = () => {}; + registerCleanup(() => { + window.onunhandledrejection = originOnunhandledrejection; + }); + /* + * End By pass error handling + */ + + const warningOpened = makeDeferred(); + class WarningDialogWait extends WarningDialog { + setup() { + super.setup(); + owl.hooks.onMounted(() => warningOpened.resolve()); + } + } + + serviceRegistry.add("error", errorService); + registry + .category("error_dialogs") + .add("odoo.exceptions.ValidationError", WarningDialogWait); + + const mockRPC = (route, args) => { + if (args.method === "onchange" && args.model === "partner") { + const error = new RPCError(); + error.exceptionName = "odoo.exceptions.ValidationError"; + error.code = 200; + return Promise.reject(error); + } + }; + + serverData.views["partner,666,form"] = /*xml*/ ` +
+
+
+ + `; + + const webClient = await createWebClient({ serverData, mockRPC }); + await doAction(webClient, 24); + await click(webClient.el, ".o_form_view button"); + + await warningOpened; + assert.containsOnce(webClient, ".modal"); + assert.containsOnce(webClient, ".modal .o_dialog_warning"); + assert.strictEqual( + webClient.el.querySelector(".modal .modal-title").textContent, + "Validation Error" + ); + }); });