From c830d9bc9d46ca67c815aa33b040049299f474e5 Mon Sep 17 00:00:00 2001 From: FrancoisGe Date: Mon, 2 Oct 2023 14:07:16 +0200 Subject: [PATCH] [FIX] web: Record.update with onChange The goal of this commit is to resolve: ===================================== 1. When you edit a Char or Boolean field with an onChange that returns the value before editing, you must display the value of the onChange. 2. In the model, html fields are always Markup fields. But when we evaluate a modifier containing an html field, we use the server value (a string or false). 3. When we edit a field and its onChange returns an server error, we want to revert its value to the valuea before edition. Solution: ========= 1. Always render when applying a change, even if the value does not change, because onChange returns the initial value. To do this, we'll apply the changes to record.data, then apply the changes from onChange. The value will therefore change, which will cause all the components observing the value in record.data to render. 2. When we update an html field, its value will always be markup and we'll store it as a string in _textValue so that we can evaluate the modifiers without the markup. For changes coming from the onChange, we will store the value in _textValue and apply markup on it for record.data. 3. When an onChange returns an server error, we will apply the changes and then revert them directly. This will have the effect of triggering a rendering on all the components observing to the record.data corresponding to the changes and so displaying the values before editing. How to reproduce: ================= Case 1: ------ - Go to a form view containing a boolean field with an onChange - Check the boolean field - The onChange returns with the value false Before this commit: The boolean field is always checked After this commit: The boolean field is no longer checked Case 2: ------ - Go to a form view containing an html field and an field "x" with invisible ="not html_field" and an onChange - Edit the field "x" - The onChange returns the value false for "html_field". Before this commit: The field "x" is always displayed After this commit The field "x" is no longer displayed Case 3: ------ - Go to a form view containing a char field with an onChange - Edit the field - onChange returns a server error Before this commit: The field is marked as invalid and contains the value after editing. After this commit: The field contains the pre-edit value closes odoo/odoo#136924 Related: odoo/enterprise#48515 Signed-off-by: Aaron Bohy (aab) --- .../src/model/relational_model/record.js | 205 +++++++++++------- .../src/views/fields/boolean/boolean_field.js | 18 +- .../views/fields/boolean/boolean_field.xml | 2 +- .../boolean_toggle/boolean_toggle_field.js | 1 + .../tests/views/fields/boolean_field_tests.js | 30 +++ .../tests/views/fields/char_field_tests.js | 60 +++++ .../tests/views/fields/html_field_tests.js | 37 ++++ .../views/fields/one2many_field_tests.js | 8 +- .../tests/views/form/form_view_tests.js | 6 +- 9 files changed, 279 insertions(+), 88 deletions(-) diff --git a/addons/web/static/src/model/relational_model/record.js b/addons/web/static/src/model/relational_model/record.js index 0244663b5d6..3108bd812ee 100644 --- a/addons/web/static/src/model/relational_model/record.js +++ b/addons/web/static/src/model/relational_model/record.js @@ -7,7 +7,6 @@ import { serializeDate, serializeDateTime } from "@web/core/l10n/dates"; import { _t } from "@web/core/l10n/translation"; import { x2ManyCommands } from "@web/core/orm_service"; import { evaluateBooleanExpr } from "@web/core/py_js/py"; -import { pick } from "@web/core/utils/objects"; import { escape } from "@web/core/utils/strings"; import { DataPoint } from "./datapoint"; import { @@ -173,7 +172,7 @@ export class Record extends DataPoint { } else { this.model._updateConfig(this.config, { resId: false }, { reload: false }); this.dirty = false; - this._changes = this._parseServerValues(this._getDefaultValues()); + this._changes = markRaw(this._parseServerValues(this._getDefaultValues())); this._values = markRaw({}); this._textValues = markRaw({}); this.data = { ...this._changes }; @@ -256,7 +255,12 @@ export class Record extends DataPoint { if (this.model._urgentSave) { return this._update(changes, { save: false }); // save is already scheduled } - return this.model.mutex.exec(() => this._update(changes, { save })); + return this.model.mutex.exec(async () => { + await this._update(changes, { withoutOnchange: save }); + if (save) { + return this._save(); + } + }); } urgentSave() { @@ -283,14 +287,49 @@ export class Record extends DataPoint { } } - _applyChanges(changes) { + _applyChanges(changes, serverChanges = {}) { + // We need to generate the undo function before applying the changes + const fieldNameChanges = [...Object.keys({ ...changes, ...serverChanges })]; + const initialTextValues = { ...this._textValues }; + const initialChanges = { ...this._changes }; + const initialData = { ...toRaw(this.data) }; + const invalidFields = toRaw(this._invalidFields); + const invalidFieldNames = fieldNameChanges.filter((fieldName) => + invalidFields.has(fieldName) + ); + const undoChanges = () => { + for (const fieldName of invalidFieldNames) { + this.setInvalidField(fieldName); + } + Object.assign(this.data, initialData); + this._changes = markRaw(initialChanges); + Object.assign(this._textValues, initialTextValues); + this._setEvalContext(); + }; + + // Apply changes for (const fieldName in changes) { - this._changes[fieldName] = changes[fieldName]; - this.data[fieldName] = changes[fieldName]; + const change = changes[fieldName]; + this._changes[fieldName] = change; + this.data[fieldName] = change; + if (this.fields[fieldName].type === "html") { + this._textValues[fieldName] = change === false ? false : change.toString(); + } else if (["char", "text"].includes(this.fields[fieldName].type)) { + this._textValues[fieldName] = change; + } } - Object.assign(this._textValues, this._getTextValues(changes)); + + // Apply server changes + const parsedChanges = this._parseServerValues(serverChanges, this.data); + for (const fieldName in parsedChanges) { + this._changes[fieldName] = parsedChanges[fieldName]; + this.data[fieldName] = parsedChanges[fieldName]; + } + Object.assign(this._textValues, this._getTextValues(serverChanges)); + this._setEvalContext(); - this._removeInvalidFields(Object.keys(changes)); + this._removeInvalidFields(fieldNameChanges); + return undoChanges; } _applyDefaultValues() { @@ -299,7 +338,7 @@ export class Record extends DataPoint { }); const defaultValues = this._getDefaultValues(fieldNames); if (this.isNew) { - this._applyChanges(this._parseServerValues(defaultValues)); + this._applyChanges({}, defaultValues); } else { this._applyValues(defaultValues); } @@ -674,14 +713,6 @@ export class Record extends DataPoint { return parsedValues; } - async _preprocessChanges(changes) { - await Promise.all([ - this._preprocessMany2oneChanges(changes), - this._preprocessReferenceChanges(changes), - this._preprocessX2manyChanges(changes), - ]); - } - async _preprocessReferenceChanges(changes) { const proms = []; for (const [fieldName, value] of Object.entries(changes)) { @@ -812,6 +843,38 @@ export class Record extends DataPoint { } } + _preprocessPropertiesChanges(changes) { + for (const [fieldName, value] of Object.entries(changes)) { + const field = this.fields[fieldName]; + if (field && field.relatedPropertyField) { + const [propertyFieldName, propertyName] = field.name.split("."); + const propertiesData = this.data[propertyFieldName] || []; + if (!propertiesData.find((property) => property.name === propertyName)) { + // try to change the value of a properties that has a different parent + this.model.notification.add( + _t( + "This record belongs to a different parent so you can not change this property." + ), + { type: "warning" } + ); + return; + } + changes[propertyFieldName] = propertiesData.map((property) => + property.name === propertyName ? { ...property, value } : property + ); + delete changes[field.name]; + } + } + } + + _preprocessHtmlChanges(changes) { + for (const [fieldName, value] of Object.entries(changes)) { + if (this.fields[fieldName].type === "html") { + changes[fieldName] = value === false ? false : markup(value); + } + } + } + _removeInvalidFields(fieldNames) { for (const fieldName of fieldNames) { this._invalidFields.delete(fieldName); @@ -911,11 +974,11 @@ export class Record extends DataPoint { } this._setData(records[0]); } else { - this._values = { ...this._values, ...this._changes }; + this._values = markRaw({ ...this._values, ...this._changes }); if ("id" in this.activeFields) { this._values.id = records[0].id; } - this._changes = {}; + this._changes = markRaw({}); this.data = { ...this._values }; this.dirty = false; } @@ -979,63 +1042,57 @@ export class Record extends DataPoint { } } - async _update(changes, { withoutOnchange, withoutParentUpdate, save } = {}) { + async _getOnchangeValues(changes) { + const onChangeFields = Object.keys(changes).filter( + (fieldName) => this.activeFields[fieldName] && this.activeFields[fieldName].onChange + ); + if (!onChangeFields.length) { + return {}; + } + + const localChanges = this._getChanges( + { ...this._changes, ...changes }, + { withReadonly: true } + ); + if (this.config.relationField) { + localChanges[this.config.relationField] = this._parentRecord._getChanges(); + if (!this._parentRecord.isNew) { + localChanges[this.config.relationField].id = this._parentRecord.resId; + } + } + return this.model._onchange(this.config, { + changes: localChanges, + fieldNames: onChangeFields, + evalContext: toRaw(this.evalContext), + onError: (e) => { + // We apply changes and revert them after to force a render of the Field components + const undoChanges = this._applyChanges(changes); + undoChanges(); + throw e; + }, + }); + } + + async _update(changes, { withoutOnchange, withoutParentUpdate } = {}) { this.dirty = true; - const prom = this._preprocessChanges(changes); - if (prom && !this.model._urgentSave) { + const prom = Promise.all([ + this._preprocessMany2oneChanges(changes), + this._preprocessReferenceChanges(changes), + this._preprocessX2manyChanges(changes), + this._preprocessPropertiesChanges(changes), + this._preprocessHtmlChanges(changes), + ]); + if (!this.model._urgentSave) { await prom; } if (this.selected && this.model.multiEdit) { this._applyChanges(changes); return this.model.root._multiSave(this); } - for (const [fieldName, value] of Object.entries(changes)) { - const field = this.fields[fieldName]; - if (field && field.relatedPropertyField) { - const [propertyFieldName, propertyName] = field.name.split("."); - const propertiesData = this.data[propertyFieldName] || []; - if (!propertiesData.find((property) => property.name === propertyName)) { - // try to change the value of a properties that has a different parent - this.model.notification.add( - _t( - "This record belongs to a different parent so you can not change this property." - ), - { type: "warning" } - ); - return; - } - changes[propertyFieldName] = propertiesData.map((property) => - property.name === propertyName ? { ...property, value } : property - ); - delete changes[field.name]; - } - } - const onChangeFields = Object.keys(changes).filter( - (fieldName) => this.activeFields[fieldName] && this.activeFields[fieldName].onChange - ); - if (onChangeFields.length && !this.model._urgentSave && !withoutOnchange && !save) { - const localChanges = this._getChanges( - { ...this._changes, ...changes }, - { withReadonly: true } - ); - if (this.config.relationField) { - localChanges[this.config.relationField] = this._parentRecord._getChanges(); - if (!this._parentRecord.isNew) { - localChanges[this.config.relationField].id = this._parentRecord.resId; - } - } - const otherChanges = await this.model._onchange(this.config, { - changes: localChanges, - fieldNames: onChangeFields, - evalContext: this.evalContext, - onError: (e) => { - for (const fieldName in localChanges) { - this._setInvalidField(fieldName); - } - throw e; - }, - }); - Object.assign(changes, this._parseServerValues(otherChanges, this.data)); + + let onchangeServerValues = {}; + if (!this.model._urgentSave && !withoutOnchange) { + onchangeServerValues = await this._getOnchangeValues(changes); } // changes inside the record set as value for a many2one field must trigger the onchange, // but can't be considered as changes on the parent record, so here we detect if many2one @@ -1049,20 +1106,16 @@ export class Record extends DataPoint { } } } - if (Object.keys(changes).length > 0) { - const initialChanges = pick(this.data, ...Object.keys(changes)); - this._applyChanges(changes); + const undoChanges = this._applyChanges(changes, onchangeServerValues); + if (Object.keys(changes).length > 0 || Object.keys(onchangeServerValues).length > 0) { try { await this._onUpdate({ withoutParentUpdate }); } catch (e) { - this._applyChanges(initialChanges); + undoChanges(); throw e; } await this.model.hooks.onRecordChanged(this, this._getChanges()); } - if (save) { - return this._save(); - } } _updateChildrenContext() { diff --git a/addons/web/static/src/views/fields/boolean/boolean_field.js b/addons/web/static/src/views/fields/boolean/boolean_field.js index 458bab0086a..7e73704a1c4 100644 --- a/addons/web/static/src/views/fields/boolean/boolean_field.js +++ b/addons/web/static/src/views/fields/boolean/boolean_field.js @@ -1,11 +1,11 @@ /** @odoo-module **/ -import { registry } from "@web/core/registry"; -import { _t } from "@web/core/l10n/translation"; -import { standardFieldProps } from "../standard_field_props"; +import { Component, useState } from "@odoo/owl"; import { CheckBox } from "@web/core/checkbox/checkbox"; - -import { Component } from "@odoo/owl"; +import { _t } from "@web/core/l10n/translation"; +import { registry } from "@web/core/registry"; +import { useRecordObserver } from "@web/model/relational_model/utils"; +import { standardFieldProps } from "../standard_field_props"; export class BooleanField extends Component { static template = "web.BooleanField"; @@ -14,10 +14,18 @@ export class BooleanField extends Component { ...standardFieldProps, }; + setup() { + this.state = useState({}); + useRecordObserver((record) => { + this.state.value = record.data[this.props.name]; + }); + } + /** * @param {boolean} newValue */ onChange(newValue) { + this.state.value = newValue; this.props.record.update({ [this.props.name]: newValue }); } } diff --git a/addons/web/static/src/views/fields/boolean/boolean_field.xml b/addons/web/static/src/views/fields/boolean/boolean_field.xml index 6b3bd101a62..e61da57a4a8 100644 --- a/addons/web/static/src/views/fields/boolean/boolean_field.xml +++ b/addons/web/static/src/views/fields/boolean/boolean_field.xml @@ -2,7 +2,7 @@ - + diff --git a/addons/web/static/src/views/fields/boolean_toggle/boolean_toggle_field.js b/addons/web/static/src/views/fields/boolean_toggle/boolean_toggle_field.js index c5b63801640..0648281ad4b 100644 --- a/addons/web/static/src/views/fields/boolean_toggle/boolean_toggle_field.js +++ b/addons/web/static/src/views/fields/boolean_toggle/boolean_toggle_field.js @@ -12,6 +12,7 @@ export class BooleanToggleField extends BooleanField { }; async onChange(newValue) { + this.state.value = newValue; const changes = { [this.props.name]: newValue }; await this.props.record.update(changes, { save: this.props.autosave }); } diff --git a/addons/web/static/tests/views/fields/boolean_field_tests.js b/addons/web/static/tests/views/fields/boolean_field_tests.js index 08c2237359e..b424d7048ae 100644 --- a/addons/web/static/tests/views/fields/boolean_field_tests.js +++ b/addons/web/static/tests/views/fields/boolean_field_tests.js @@ -273,4 +273,34 @@ QUnit.module("Fields", (hooks) => { "checkbox should still be disabled" ); }); + + QUnit.test("onchange return value before toggle checkbox", async function (assert) { + serverData.models.partner.onchanges = { + bar(obj) { + obj.bar = true; + }, + }; + + await makeView({ + type: "form", + resModel: "partner", + resId: 1, + serverData, + arch: `
`, + }); + + assert.containsOnce( + target, + ".o_field_boolean input:checked", + "checkbox should still be checked" + ); + + await click(target, ".o_field_boolean .o-checkbox"); + await nextTick(); + assert.containsOnce( + target, + ".o_field_boolean input:checked", + "checkbox should still be checked" + ); + }); }); diff --git a/addons/web/static/tests/views/fields/char_field_tests.js b/addons/web/static/tests/views/fields/char_field_tests.js index 46601d33cb4..79eeee289b6 100644 --- a/addons/web/static/tests/views/fields/char_field_tests.js +++ b/addons/web/static/tests/views/fields/char_field_tests.js @@ -795,6 +795,30 @@ QUnit.module("Fields", (hooks) => { } ); + QUnit.test("onchange return value before editing input", async function (assert) { + serverData.models.partner.onchanges = { + foo(obj) { + obj.foo = "yop"; + }, + }; + + await makeView({ + type: "form", + resModel: "partner", + resId: 1, + serverData, + arch: ` +
+ + `, + }); + + assert.strictEqual(target.querySelector("[name='foo'] input").value, "yop"); + + await editInput(target, "[name='foo'] input", "tralala"); + assert.strictEqual(target.querySelector("[name='foo'] input").value, "yop"); + }); + QUnit.test( "input field: change value before pending onchange renaming", async function (assert) { @@ -1050,4 +1074,40 @@ QUnit.module("Fields", (hooks) => { "Placeholder" ); }); + + QUnit.test( + "char field: correct value is used to evaluate the modifiers", + async function (assert) { + serverData.models.partner.onchanges = { + foo: (obj) => { + if (obj.foo === "a") { + obj.display_name = false; + } else if (obj.foo === "b") { + obj.display_name = ""; + } + }, + }; + serverData.models.partner.records[0].foo = false; + serverData.models.partner.records[0].display_name = false; + + await makeView({ + type: "form", + resModel: "partner", + serverData, + resId: 1, + arch: ` +
+ + + `, + }); + assert.containsOnce(target, "[name='display_name'] input"); + + await editInput(target, "[name='foo'] input", "a"); + assert.containsOnce(target, "[name='display_name'] input"); + + await editInput(target, "[name='foo'] input", "b"); + assert.containsNone(target, "[name='display_name'] input"); + } + ); }); diff --git a/addons/web/static/tests/views/fields/html_field_tests.js b/addons/web/static/tests/views/fields/html_field_tests.js index 336d4e41192..489ea8775f8 100644 --- a/addons/web/static/tests/views/fields/html_field_tests.js +++ b/addons/web/static/tests/views/fields/html_field_tests.js @@ -353,4 +353,41 @@ QUnit.module("Fields", ({ beforeEach }) => { await clickSave(target); } ); + + QUnit.test( + "html field: correct value is used to evaluate the modifiers", + async function (assert) { + serverData.models.partner.fields.foo = { string: "foo", type: "char" }; + serverData.models.partner.onchanges = { + foo: (obj) => { + if (obj.foo === "a") { + obj.txt = false; + } else if (obj.foo === "b") { + obj.txt = ""; + } + }, + }; + serverData.models.partner.records[0].foo = false; + serverData.models.partner.records[0].txt = false; + + await makeView({ + type: "form", + resModel: "partner", + serverData, + resId: 1, + arch: ` +
+ + + `, + }); + assert.containsOnce(target, "[name='txt'] textarea"); + + await editInput(target, "[name='foo'] input", "a"); + assert.containsOnce(target, "[name='txt'] textarea"); + + await editInput(target, "[name='foo'] input", "b"); + assert.containsNone(target, "[name='txt'] textarea"); + } + ); }); diff --git a/addons/web/static/tests/views/fields/one2many_field_tests.js b/addons/web/static/tests/views/fields/one2many_field_tests.js index 45e14cb5b13..88d3427e857 100644 --- a/addons/web/static/tests/views/fields/one2many_field_tests.js +++ b/addons/web/static/tests/views/fields/one2many_field_tests.js @@ -8930,7 +8930,7 @@ QUnit.module("Fields", (hooks) => { ); QUnit.test("display correct value after validation error", async function (assert) { - assert.expect(5); + assert.expect(4); serviceRegistry.add("error", errorService); function validationHandler(env, error, originalError) { @@ -8986,8 +8986,10 @@ QUnit.module("Fields", (hooks) => { await editInput(target, ".o_field_widget[name=turtle_foo] input", "pinky"); await click(target, ".o_form_view"); assert.strictEqual(target.querySelector(".o_data_row .o_data_cell").textContent, "foo"); - assert.hasClass(target.querySelector(".o_field_widget[name=turtles]"), "o_field_invalid"); - assert.ok(target.querySelector(".o_form_button_save").disabled); + + // we make sure here that when we save, the values are the current + // values displayed in the field. + await clickSave(target); }); QUnit.test("propagate context to sub views without default_* keys", async function (assert) { diff --git a/addons/web/static/tests/views/form/form_view_tests.js b/addons/web/static/tests/views/form/form_view_tests.js index 0973e0cdd43..a65bb7c1c75 100644 --- a/addons/web/static/tests/views/form/form_view_tests.js +++ b/addons/web/static/tests/views/form/form_view_tests.js @@ -6362,9 +6362,9 @@ QUnit.module("Views", (hooks) => { document.body.querySelector(".modal-body").textContent, "Some business message" ); - assert.hasClass( - target.querySelector('.o_field_widget[name="int_field"]'), - "o_field_invalid" + assert.strictEqual( + target.querySelector('.o_field_widget[name="int_field"] input').value, + "9" ); await click(target.querySelector(".modal .btn-close"));