From 22cc9660ed3b87d3a03d437cb87925ba8b587bfa Mon Sep 17 00:00:00 2001 From: luvi Date: Fri, 21 Apr 2023 06:56:45 +0000 Subject: [PATCH] [FIX] web: fix SelectMenu object shallowEqual MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With commit (1), the clear button is only displayed when a selected value is found from the available choices. But since we used shallowEqual to find the corresponding choice, we couldn't use objects containing objects in its values, since a simple comparison was made in the utility function. This was a wrong usage of the SelectMenu and we don't want to support objects and arrays as values. The value attribute should only be a string, and it's the parent responsability to handle any object related to this string. The test using an object and an array as values has been removed since it had no purpose after the removal of this partial and wrong support. With this fix, an assertion for the presence of the clear button has been added instead for the import action. (1): d760b812714f317476c0af0424ae330e7d89a0fd closes odoo/odoo#119298 Related: odoo/enterprise#40177 Signed-off-by: Géry Debongnie --- .../static/src/import_action/import_action.js | 5 +- .../import_data_content.js | 2 +- .../import_data_content.xml | 4 +- .../static/tests/import_action_tests.js | 336 +++++++++++------- .../src/core/select_menu/select_menu.js | 8 +- .../static/tests/core/select_menu_tests.js | 60 +--- 6 files changed, 223 insertions(+), 192 deletions(-) diff --git a/addons/base_import/static/src/import_action/import_action.js b/addons/base_import/static/src/import_action/import_action.js index a71e6a3cd7a..ae46e037e93 100644 --- a/addons/base_import/static/src/import_action/import_action.js +++ b/addons/base_import/static/src/import_action/import_action.js @@ -191,8 +191,9 @@ export class ImportAction extends Component { // Fields //-------------------------------------------------------------------------- - onFieldChanged(column, value) { - this.model.setColumnField(column, value); + onFieldChanged(column, fieldId) { + const selection = this.model.fields.find((f) => f.id === fieldId); + this.model.setColumnField(column, selection); } isFieldSet(column) { diff --git a/addons/base_import/static/src/import_data_content/import_data_content.js b/addons/base_import/static/src/import_data_content/import_data_content.js index 3e4dd5a114b..9ff34dda12c 100644 --- a/addons/base_import/static/src/import_data_content/import_data_content.js +++ b/addons/base_import/static/src/import_data_content/import_data_content.js @@ -46,7 +46,7 @@ export class ImportDataContent extends Component { makeChoices(fields) { return fields.map((field) => ({ label: field.label, - value: field, + value: field.id, })); } diff --git a/addons/base_import/static/src/import_data_content/import_data_content.xml b/addons/base_import/static/src/import_data_content/import_data_content.xml index 6725a853ddc..270538945ee 100644 --- a/addons/base_import/static/src/import_data_content/import_data_content.xml +++ b/addons/base_import/static/src/import_data_content/import_data_content.xml @@ -60,9 +60,9 @@ diff --git a/addons/base_import/static/tests/import_action_tests.js b/addons/base_import/static/tests/import_action_tests.js index 6f0eee642e7..4d73482348f 100644 --- a/addons/base_import/static/tests/import_action_tests.js +++ b/addons/base_import/static/tests/import_action_tests.js @@ -160,70 +160,70 @@ function executeFailingImport(field, isMultiline, field_path = "") { ids: false, messages: isMultiline ? [ - { - field, - field_name: serverData.models.partner.fields[field].string, - field_path, - message: "Invalid value", - moreInfo, - record: 0, - rows: { from: 0, to: 0 }, - value: "Invalid value", - priority: "info", - }, - { - field, - field_name: serverData.models.partner.fields[field].string, - field_path, - message: "Duplicate value", - moreInfo, - record: 0, - rows: { from: 1, to: 1 }, - priority: "error", - }, - { - field, - field_name: serverData.models.partner.fields[field].string, - field_path, - message: "Wrong values", - moreInfo, - record: 0, - rows: { from: 2, to: 3 }, - priority: "warning", - }, - { - field, - field_name: serverData.models.partner.fields[field].string, - field_path, - message: "Bad value here", - moreInfo, - record: 0, - rows: { from: 4, to: 4 }, - value: "Bad value", - priority: "warning", - }, - { - field, - field_name: serverData.models.partner.fields[field].string, - field_path, - message: "Duplicate value", - moreInfo, - record: 0, - rows: { from: 5, to: 5 }, - priority: "error", - }, - ] + { + field, + field_name: serverData.models.partner.fields[field].string, + field_path, + message: "Invalid value", + moreInfo, + record: 0, + rows: { from: 0, to: 0 }, + value: "Invalid value", + priority: "info", + }, + { + field, + field_name: serverData.models.partner.fields[field].string, + field_path, + message: "Duplicate value", + moreInfo, + record: 0, + rows: { from: 1, to: 1 }, + priority: "error", + }, + { + field, + field_name: serverData.models.partner.fields[field].string, + field_path, + message: "Wrong values", + moreInfo, + record: 0, + rows: { from: 2, to: 3 }, + priority: "warning", + }, + { + field, + field_name: serverData.models.partner.fields[field].string, + field_path, + message: "Bad value here", + moreInfo, + record: 0, + rows: { from: 4, to: 4 }, + value: "Bad value", + priority: "warning", + }, + { + field, + field_name: serverData.models.partner.fields[field].string, + field_path, + message: "Duplicate value", + moreInfo, + record: 0, + rows: { from: 5, to: 5 }, + priority: "error", + }, + ] : [ - { - field, - field_name: serverData.models.partner.fields[field].string, - field_path, - message: "Incorrect value", - moreInfo, - record: 0, - rows: { from: 0, to: 0 }, - }, - ], + { + field, + field_name: serverData.models.partner.fields[field].string, + field_path, + message: "Incorrect value", + moreInfo, + record: 0, + rows: { from: 0, to: 0 }, + }, + ], name: ["Some invalid content", "Wrong content", "Bad content"], nextrow: 0, }; @@ -579,7 +579,7 @@ QUnit.module("Base Import Tests", (hooks) => { registerFakeHTTPService(); const notificationMock = (message) => { assert.step(message); - return () => { }; + return () => {}; }; registry .category("services") @@ -755,7 +755,6 @@ QUnit.module("Base Import Tests", (hooks) => { "base_import.import/execute_import": (route, args) => { if (shouldFail) { shouldFail = false; - // return executeFailingImport(args, args[1][0]); return executeFailingImport(args[1][0]); } assert.deepEqual( @@ -1181,57 +1180,90 @@ QUnit.module("Base Import Tests", (hooks) => { ); }); - QUnit.test("Import view: relational fields correctly mapped on preview", async function (assert) { - assert.expect(4); + QUnit.test( + "Import view: relational fields correctly mapped on preview", + async function (assert) { + assert.expect(4); - registerFakeHTTPService(); - await createImportAction({ - "base_import.import/parse_preview": async (route, args) => { - return customParsePreview(args[1], { - fields: [ - { id: "id", name: "id", string: "External ID", fields: [], type: "id" }, - { id: "display_name", name: "display_name", string: "Display Name", fields: [], type: "id" }, - { - id: "many2many_field", name: "many2many_field", string: "Many2Many", fields: [ - { id: "id", name: "id", string: "External ID", fields: [], type: "id" }, - ], type: "id" + registerFakeHTTPService(); + await createImportAction({ + "base_import.import/parse_preview": async (route, args) => { + return customParsePreview(args[1], { + fields: [ + { id: "id", name: "id", string: "External ID", fields: [], type: "id" }, + { + id: "display_name", + name: "display_name", + string: "Display Name", + fields: [], + type: "id", + }, + { + id: "many2many_field", + name: "many2many_field", + string: "Many2Many", + fields: [ + { + id: "id", + name: "id", + string: "External ID", + fields: [], + type: "id", + }, + ], + type: "id", + }, + ], + headers: ["id", "display_name", "many2many_field/id"], + rowCount: 5, + matches: { + 0: ["id"], + 1: ["display_name"], + 2: ["many2many_field", "id"], }, - ], - headers: ["id", "display_name", "many2many_field/id"], - rowCount: 5, - matches: { - 0: ["id"], - 1: ["display_name"], - 2: ["many2many_field", "id"] - }, - preview: [ - ["0", "1", "2"], - ["Name 1", "Name 2", "Name 3",], - ["", "1", "2"], - ], - }); - }, - "base_import.import/execute_import": (route, args) => { - assert.deepEqual(args[1], ["id", "display_name", "many2many_field/id"], - "The proper arguments are given for the import"); - assert.deepEqual(args[2], ["id", "display_name", "many2many_field/id"], - "The proper arguments are given for the import"); - return executeImport(args); - }, - }); + preview: [ + ["0", "1", "2"], + ["Name 1", "Name 2", "Name 3"], + ["", "1", "2"], + ], + }); + }, + "base_import.import/execute_import": (route, args) => { + assert.deepEqual( + args[1], + ["id", "display_name", "many2many_field/id"], + "The proper arguments are given for the import" + ); + assert.deepEqual( + args[2], + ["id", "display_name", "many2many_field/id"], + "The proper arguments are given for the import" + ); + return executeImport(args); + }, + }); - // Set and trigger the change of a file for the input - const file = new File(["fake_file"], "fake_file.xls", { type: "text/plain" }); - await editInput(target, "input[type='file']", file); + // Set and trigger the change of a file for the input + const file = new File(["fake_file"], "fake_file.xls", { type: "text/plain" }); + await editInput(target, "input[type='file']", file); - assert.strictEqual(target.querySelector("tr:nth-child(3) .o_import_file_column_cell span.text-truncate").innerText, "many2many_field/id", - "The third row should be the relational field"); + assert.strictEqual( + target.querySelector( + "tr:nth-child(3) .o_import_file_column_cell span.text-truncate" + ).innerText, + "many2many_field/id", + "The third row should be the relational field" + ); - assert.strictEqual(target.querySelector("tr:nth-child(3) .o_select_menu_toggler_slot span").innerText, "Many2Many / External ID", - "The relational field should be selected by default and the name should be the full path."); + assert.strictEqual( + target.querySelector("tr:nth-child(3) .o_select_menu_toggler_slot span").innerText, + "Many2Many / External ID", + "The relational field should be selected by default and the name should be the full path." + ); - await click(target.querySelector(".o_control_panel button:first-child")); - }); + await click(target.querySelector(".o_control_panel button:first-child")); + } + ); QUnit.test("Import view: import errors with relational fields", async function (assert) { registerFakeHTTPService(); @@ -1244,11 +1276,27 @@ QUnit.module("Base Import Tests", (hooks) => { return customParsePreview(args[1], { fields: [ { id: "id", name: "id", string: "External ID", fields: [], type: "id" }, - { id: "display_name", name: "display_name", string: "Display Name", fields: [], type: "id" }, { - id: "many2many_field", name: "many2many_field", string: "Many2Many", fields: [ - { id: "id", name: "id", string: "External ID", fields: [], type: "id" }, - ], type: "id" + id: "display_name", + name: "display_name", + string: "Display Name", + fields: [], + type: "id", + }, + { + id: "many2many_field", + name: "many2many_field", + string: "Many2Many", + fields: [ + { + id: "id", + name: "id", + string: "External ID", + fields: [], + type: "id", + }, + ], + type: "id", }, ], headers: ["id", "display_name", "many2many_field/id"], @@ -1256,11 +1304,11 @@ QUnit.module("Base Import Tests", (hooks) => { matches: { 0: ["id"], 1: ["display_name"], - 2: ["many2many_field", "id"] + 2: ["many2many_field", "id"], }, preview: [ ["0", "1", "2"], - ["Name 1", "Name 2", "Name 3",], + ["Name 1", "Name 2", "Name 3"], ["", "1", "2"], ], }); @@ -1283,17 +1331,30 @@ QUnit.module("Base Import Tests", (hooks) => { "A message is shown if the import was blocked" ); - assert.strictEqual(target.querySelector("tr:nth-child(3) .o_import_file_column_cell span.text-truncate").innerText, "many2many_field/id", - "The third row should be the relational field"); + assert.strictEqual( + target.querySelector("tr:nth-child(3) .o_import_file_column_cell span.text-truncate") + .innerText, + "many2many_field/id", + "The third row should be the relational field" + ); - assert.strictEqual(target.querySelector("tr:nth-child(3) .o_select_menu_toggler_slot span").innerText, "Many2Many / External ID", - "The relational field is properly mapped"); + assert.strictEqual( + target.querySelector("tr:nth-child(3) .o_select_menu_toggler_slot span").innerText, + "Many2Many / External ID", + "The relational field is properly mapped" + ); - assert.containsOnce(target, "tr:nth-child(3) .o_import_report.alert", - "The relational field should have error messages on his row"); + assert.containsOnce( + target, + "tr:nth-child(3) .o_import_report.alert", + "The relational field should have error messages on his row" + ); - assert.strictEqual(target.querySelector("tr:nth-child(3) .o_import_report.alert p b").innerText, "Many2Many / External ID", - "The error should contain the full path of the relational field"); + assert.strictEqual( + target.querySelector("tr:nth-child(3) .o_import_report.alert p b").innerText, + "Many2Many / External ID", + "The error should contain the full path of the relational field" + ); }); QUnit.test("Import view: date format should be converted to strftime", async function (assert) { @@ -1340,4 +1401,31 @@ QUnit.module("Base Import Tests", (hooks) => { await click(target.querySelector(".o_control_panel button:first-child")); assert.verifySteps(["execute_import"]); }); + + QUnit.test("Import action: field selection has a clear button", async function (assert) { + registerFakeHTTPService(); + patchWithCleanup(browser, { + setTimeout: (fn) => fn(), + }); + + await createImportAction(); + + // Set and trigger the change of a file for the input + const file = new File(["fake_file"], "fake_file.xlsx", { type: "text/plain" }); + await editInput(target, "input[type='file']", file); + await editSelectMenu(target, ".o_import_data_content .o_select_menu", "Bar"); + assert.containsN( + target, + ".o_select_menu_toggler_clear", + 2, + "clear button is present for each field to unselect it" + ); + + await click(target.querySelector(".o_select_menu_toggler_clear")); + assert.strictEqual( + target.querySelector("tr:nth-child(2) .o_select_menu").textContent, + "To import, select a field...", + "field has been unselected" + ); + }); }); diff --git a/addons/web/static/src/core/select_menu/select_menu.js b/addons/web/static/src/core/select_menu/select_menu.js index 6c2b4edc987..82924d1b68e 100644 --- a/addons/web/static/src/core/select_menu/select_menu.js +++ b/addons/web/static/src/core/select_menu/select_menu.js @@ -4,7 +4,6 @@ import { Component, useState, useRef, onWillUpdateProps } from "@odoo/owl"; import { Dropdown } from "@web/core/dropdown/dropdown"; import { DropdownItem } from "@web/core/dropdown/dropdown_item"; import { _lt } from "@web/core/l10n/translation"; -import { shallowEqual } from "@web/core/utils/objects"; import { useDebounced } from "@web/core/utils/timing"; import { scrollTo } from "@web/core/utils/scrolling"; import { fuzzyLookup } from "@web/core/utils/search"; @@ -172,12 +171,7 @@ export class SelectMenu extends Component { getSelectedChoice(props) { if (props.value) { const choices = [...props.choices, ...props.groups.flatMap((g) => g.choices)]; - return choices.find((c) => { - if (typeof c.value === "object" && typeof props.value === "object") { - return shallowEqual(c.value, props.value); - } - return c.value === props.value; - }); + return choices.find((c) => c.value === props.value); } else { return undefined; } diff --git a/addons/web/static/tests/core/select_menu_tests.js b/addons/web/static/tests/core/select_menu_tests.js index 07383ef4348..a0559dbee06 100644 --- a/addons/web/static/tests/core/select_menu_tests.js +++ b/addons/web/static/tests/core/select_menu_tests.js @@ -175,58 +175,6 @@ QUnit.module("Web Components", (hooks) => { assert.equal(document.activeElement, target.querySelector(".o_select_menu_input input")); }); - QUnit.test("value props accept array", async (assert) => { - class Parent extends Component { - setup() { - this.choices = [ - { label: "Z", value: [1, 2] }, - { label: "A", value: [1, 3] }, - ]; - this.value = [1, 2]; - } - } - Parent.components = { SelectMenu }; - Parent.template = xml` - - `; - - await mount(Parent, target, { env }); - assert.equal( - target.querySelector(".o_select_menu_toggler_slot").innerText, - "Z", - `The select value shoud be "Z"` - ); - }); - - QUnit.test("value props accept object", async (assert) => { - class Parent extends Component { - setup() { - this.choices = [ - { label: "Z", value: { hello: "world" } }, - { label: "A", value: { paper: "company" } }, - ]; - this.value = { paper: "company" }; - } - } - Parent.components = { SelectMenu }; - Parent.template = xml` - - `; - - await mount(Parent, target, { env }); - assert.equal( - target.querySelector(".o_select_menu_toggler_slot").innerText, - "A", - `The select value shoud be "A"` - ); - }); - QUnit.test( "Value with no corresponding choices displays as if no choice was selected", async (assert) => { @@ -266,10 +214,10 @@ QUnit.module("Web Components", (hooks) => { `; setup() { this.choices = [ - { label: "Z", value: { hello: "world" } }, - { label: "A", value: { paper: "company" } }, + { label: "Z", value: "world" }, + { label: "A", value: "company" }, ]; - this.state = useState({ value: { paper: "company" } }); + this.state = useState({ value: "company" }); } setValue(newValue) { this.state.value = newValue; @@ -279,7 +227,7 @@ QUnit.module("Web Components", (hooks) => { const comp = await mount(Parent, target, { env }); assert.equal(getValue(), "A", `The select value shoud be "A"`); - comp.setValue({ hello: "world" }); + comp.setValue("world"); await nextTick(); assert.equal( getValue(),