[FIX] web: restore dialog callback calls (e.g. cloning a website page)

Commit [1] broke some confirm dialogs because the confirm and cancel
callbacks are not called with the same `this` anymore. Note that [1] was
actually not forward-ported here in 16.0 but now is via this fix.

E.g.:
- Install website
- Go to the page manager (/website/pages)
- Clone a page (choose a name and confirm)
=> Crash (this.$ does not exist).

Note that this flow was quickly fixed with [2] by changing the local
code instead of fixing the root cause (this commit here actually
technically reverts that change so it keeps working). This flow was
also not broken in 16.0 as the code was rewritten differently.

The work done at [1] actually needed some more rework than that to
handle two other potential usecases:

- If the confirm dialog handlers' promises are rejected, the dialog is
  not closed (rightfully, like for example allowing to fill an empty
  required input)... but [1] prevented to click on the button again
  forever. With the same example as above, it can be reproduced: if the
  RPC to clone the page fails, I cannot retry.

- If not both confirm_callback and cancel_callback were given, [1] only
  prevented to multi-click on the related button (e.g. if you click on
  "Ok", a RPC which takes 10 seconds is made, you click on cancel
  immediately afterwards -> the dialog is closed while it should be
  prevented (and would be if cancel_callback was given, following [1])).
  That may not be entirely stable to make this change though but it
  seems to make sense and be better (just keeping what [1] wanted here).

Note: this adds some more tests to check all of this + some more things
that were found during development. For example: [1] relied on the fact
that callbacks returned a Promise or nothing... while developers
actually were returning random things (for no reason as no way to get
the result anyway). Adding a test for this prevents to break that in
stable (the first iteration here crashed in such cases).

[1]: https://github.com/odoo/odoo/commit/4b8b079a7d9991a8dc481fe71a45185d672135c9
[2]: https://github.com/odoo/odoo/commit/8216341f5ad8d82ad0bcb0d1f01d874477d2de7b

Closes https://github.com/odoo/odoo/pull/103712
opw-3033878
opw-3043224
opw-3046485
opw-3042542

closes odoo/odoo#105081

X-original-commit: 90b280c221f3c0819cb0b291e102f161fc47507a
Signed-off-by: Aaron Bohy (aab) <aab@odoo.com>
Signed-off-by: Quentin Smetz (qsm) <qsm@odoo.com>

addons/web/static/src/legacy/js/core/dialog.js

@@ -427,17 +427,43 @@ Dialog.alert = function (owner, message, options) {
 
 // static method to open simple confirm dialog
 Dialog.confirm = function (owner, message, options) {
+    /**
+     * Creates an improved callback from the given callback value at the given
+     * key from the parent function's options parameter. This is improved to:
+     *
+     * - Prevent calling given callbacks once one has been called.
+     *
+     * - Re-allow calling callbacks once a previous callback call's returned
+     *   Promise is rejected.
+     */
+    let isBlocked = false;
+    function makeCallback(key) {
+        const callback = options && options[key];
+        return function () {
+            if (isBlocked) {
+                // Do not (re)call any callback and return a rejected Promise
+                // to prevent closing the Dialog.
+                return Promise.reject();
+            }
+            isBlocked = true;
+            const callbackRes = callback && callback.apply(this, arguments);
+            Promise.resolve(callbackRes).guardedCatch(() => {
+                isBlocked = false;
+            });
+            return callbackRes;
+        };
+    }
     var buttons = [
         {
             text: _t("Ok"),
             classes: 'btn-primary',
             close: true,
-            click: options && options.confirm_callback,
+            click: makeCallback('confirm_callback'),
         },
         {
             text: _t("Cancel"),
             close: true,
-            click: options && options.cancel_callback
+            click: makeCallback('cancel_callback'),
         }
     ];
     return new Dialog(owner, _.extend({

addons/web/static/tests/legacy/core/dialog_tests.js

@@ -103,6 +103,159 @@ QUnit.module('core', {}, function () {
         parent.destroy();
     });
 
+    QUnit.test("click twice on 'Ok' button of a confirm dialog", async function (assert) {
+        assert.expect(5);
+
+        var testPromise = testUtils.makeTestPromise();
+        var parent = await createEmptyParent();
+        var options = {
+            confirm_callback: () => {
+                assert.step("confirm");
+                return testPromise;
+            },
+        };
+        Dialog.confirm(parent, "", options);
+        await testUtils.nextTick();
+
+        assert.verifySteps([]);
+
+        await testUtils.dom.click($('.modal[role="dialog"] .btn-primary'));
+        await testUtils.dom.click($('.modal[role="dialog"] .btn-primary'));
+        await testUtils.nextTick();
+        assert.verifySteps(['confirm']);
+        assert.ok($('.modal[role="dialog"]').hasClass('show'), "Should still be opened");
+        testPromise.resolve();
+        await testUtils.nextTick();
+        assert.notOk($('.modal[role="dialog"]').hasClass('show'), "Should now be closed");
+
+        parent.destroy();
+    });
+
+    QUnit.test("click on 'Cancel' and then 'Ok' in a confirm dialog", async function (assert) {
+        assert.expect(3);
+
+        var parent = await createEmptyParent();
+        var options = {
+            confirm_callback: () => {
+                throw new Error("should not be called");
+            },
+            cancel_callback: () => {
+                assert.step("cancel");
+            }
+        };
+        Dialog.confirm(parent, "", options);
+        await testUtils.nextTick();
+
+        assert.verifySteps([]);
+
+        testUtils.dom.click($('.modal[role="dialog"] footer button:not(.btn-primary)'));
+        testUtils.dom.click($('.modal[role="dialog"] footer .btn-primary'));
+        assert.verifySteps(['cancel']);
+
+        parent.destroy();
+    });
+
+    QUnit.test("click on 'Cancel' and then 'Ok' in a confirm dialog (no cancel callback)", async function (assert) {
+        assert.expect(2);
+
+        var parent = await createEmptyParent();
+        var options = {
+            confirm_callback: () => {
+                throw new Error("should not be called");
+            },
+            // Cannot add a step in cancel_callback, that's the point of this
+            // test, we'll rely on checking the Dialog is opened then closed
+            // without a crash.
+        };
+        Dialog.confirm(parent, "", options);
+        await testUtils.nextTick();
+
+        assert.ok($('.modal[role="dialog"]').hasClass('show'));
+        testUtils.dom.click($('.modal[role="dialog"] footer button:not(.btn-primary)'));
+        testUtils.dom.click($('.modal[role="dialog"] footer .btn-primary'));
+        await testUtils.nextTick();
+        assert.notOk($('.modal[role="dialog"]').hasClass('show'));
+
+        parent.destroy();
+    });
+
+    QUnit.test("Confirm dialog callbacks properly handle rejections", async function (assert) {
+        assert.expect(5);
+
+        var parent = await createEmptyParent();
+        var options = {
+            confirm_callback: () => {
+                assert.step("confirm");
+                return Promise.reject();
+            },
+            cancel_callback: () => {
+                assert.step("cancel");
+                return $.Deferred().reject(); // Test jquery deferred too
+            }
+        };
+        Dialog.confirm(parent, "", options);
+        await testUtils.nextTick();
+
+        assert.verifySteps([]);
+        testUtils.dom.click($('.modal[role="dialog"] footer button:not(.btn-primary)'));
+        await testUtils.nextTick();
+        testUtils.dom.click($('.modal[role="dialog"] footer .btn-primary'));
+        await testUtils.nextTick();
+        testUtils.dom.click($('.modal[role="dialog"] footer button:not(.btn-primary)'));
+        assert.verifySteps(['cancel', 'confirm', 'cancel']);
+
+        parent.destroy();
+    });
+
+    QUnit.test("Properly can rely on the this in confirm and cancel callbacks of confirm dialog", async function (assert) {
+        assert.expect(2);
+
+        let dialogInstance = null;
+        var parent = await createEmptyParent();
+        var options = {
+            confirm_callback: function () {
+                assert.equal(this, dialogInstance, "'this' is properly a reference to the dialog instance");
+                return Promise.reject();
+            },
+            cancel_callback: function () {
+                assert.equal(this, dialogInstance, "'this' is properly a reference to the dialog instance");
+                return Promise.reject();
+            }
+        };
+        dialogInstance = Dialog.confirm(parent, "", options);
+        await testUtils.nextTick();
+
+        testUtils.dom.click($('.modal[role="dialog"] footer button:not(.btn-primary)'));
+        await testUtils.nextTick();
+        testUtils.dom.click($('.modal[role="dialog"] footer .btn-primary'));
+
+        parent.destroy();
+    });
+
+    QUnit.test("Confirm dialog callbacks can return anything without crash", async function (assert) {
+        assert.expect(3);
+        // Note that this test could be removed in master if the related code
+        // is reworked. This only prevents a stable fix to break this again by
+        // relying on the fact what is returned by those callbacks are undefined
+        // or promises.
+
+        var parent = await createEmptyParent();
+        var options = {
+            confirm_callback: () => {
+                assert.step("confirm");
+                return 5;
+            },
+        };
+        Dialog.confirm(parent, "", options);
+        await testUtils.nextTick();
+
+        assert.verifySteps([]);
+        testUtils.dom.click($('.modal[role="dialog"] footer .btn-primary'));
+        assert.verifySteps(['confirm']);
+
+        parent.destroy();
+    });
+
     QUnit.test("Closing alert dialog without using buttons calls confirm callback", async function (assert) {
         assert.expect(3);
 

addons/web/static/tests/legacy/views/form_tests.js

@@ -7682,6 +7682,44 @@ QUnit.module('LegacyViews', {
         form.destroy();
     });
 
+    QUnit.test('buttons with "confirm" attribute: click twice on "Ok"', async function (assert) {
+        assert.expect(7);
+
+        const form = await createView({
+            View: FormView,
+            model: 'partner',
+            data: this.data,
+            arch: `
+                <form>
+                    <header>
+                        <button name="post" class="p" string="Confirm" type="object" confirm="U sure?"/>
+                    </header>
+                </form>`,
+            mockRPC: function (route, args) {
+                assert.step(args.method);
+                return this._super.apply(this, arguments);
+            },
+            intercepts: {
+                execute_action: function (event) {
+                    assert.step('execute_action'); // should be called only once
+                    event.data.on_success();
+                },
+            },
+        });
+
+        assert.verifySteps(["onchange"]);
+
+        await testUtils.dom.click(form.$('.o_statusbar_buttons button'));
+        assert.verifySteps([]);
+
+        testUtils.dom.click($('.modal-footer button.btn-primary'));
+        await Promise.resolve();
+        await testUtils.dom.click($('.modal-footer button.btn-primary'));
+        assert.verifySteps(['create', 'read', 'execute_action']);
+
+        form.destroy();
+    });
+
     QUnit.test('buttons are disabled until action is resolved (in dialogs)', async function (assert) {
         assert.expect(3);