From a16361f96d8936004a4d6e2a9fb4b27cd27fc663 Mon Sep 17 00:00:00 2001 From: "Bastien Fafchamps (bafa)" Date: Fri, 24 Feb 2023 16:08:18 +0000 Subject: [PATCH] [FIX] base_import: Fix relationnal fields not being correctly mapped This commit fixes a couple of issues linked to relational fields: - When previewing a file with relational fields/ids, they were not correctly mapped to their respective fields in the selects, the fields were also not correctly mapped in the arguments given for the import. - The select label was also only showing the name of the last sub-field while it should show the full path ("External ID" vs "Company / External ID"). - Some errors were not correctly displayed: wrong field name, not in a column when it should be. Task ID: 3210337 opw-3203704 + opw-3192403 closes odoo/odoo#114490 X-original-commit: 871690afdc2ce4da2770a144cc4fc5ed038e6f73 Signed-off-by: Aaron Bohy (aab) --- .../import_data_column_error.xml | 2 +- .../import_data_content.scss | 4 + .../import_data_content.xml | 2 +- .../import_data_options.js | 8 +- addons/base_import/static/src/import_model.js | 135 ++++----- .../static/tests/import_action_tests.js | 269 +++++++++++++----- 6 files changed, 283 insertions(+), 137 deletions(-) diff --git a/addons/base_import/static/src/import_data_column_error/import_data_column_error.xml b/addons/base_import/static/src/import_data_column_error/import_data_column_error.xml index 0df40591d9e..226c9b4f4b3 100644 --- a/addons/base_import/static/src/import_data_column_error/import_data_column_error.xml +++ b/addons/base_import/static/src/import_data_column_error/import_data_column_error.xml @@ -6,7 +6,7 @@

No matching records found for the following name Multiple errors occurred - in field : + in field :

    diff --git a/addons/base_import/static/src/import_data_content/import_data_content.scss b/addons/base_import/static/src/import_data_content/import_data_content.scss index eab6aa6a8ee..f9ee5c1a887 100644 --- a/addons/base_import/static/src/import_data_content/import_data_content.scss +++ b/addons/base_import/static/src/import_data_content/import_data_content.scss @@ -14,6 +14,10 @@ .o_import_preview { width: fit-content; } + + td { + vertical-align: top; + } } @mixin o-import-sprite-icon($x: 0, $y: 0) { 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 570b025611f..6725a853ddc 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 @@ -74,7 +74,7 @@ t-att-data-tooltip-info="getTooltipDetails(column.fieldInfo)" > - + To import, select a field... diff --git a/addons/base_import/static/src/import_data_options/import_data_options.js b/addons/base_import/static/src/import_data_options/import_data_options.js index 99606bdabcb..40781839bdc 100644 --- a/addons/base_import/static/src/import_data_options/import_data_options.js +++ b/addons/base_import/static/src/import_data_options/import_data_options.js @@ -62,14 +62,18 @@ export class ImportDataOptions extends Component { "import_skip_records", ].includes(ev.target.value) ) { - this.props.onOptionChanged(ev.target.value, ev.target.value, this.props.fieldInfo.name); + this.props.onOptionChanged( + ev.target.value, + ev.target.value, + this.props.fieldInfo.fieldPath + ); } else { const value = { fallback_value: ev.target.value, field_model: this.currentModel, field_type: this.props.fieldInfo.type, }; - this.props.onOptionChanged("fallback_values", value, this.props.fieldInfo.name); + this.props.onOptionChanged("fallback_values", value, this.props.fieldInfo.fieldPath); } } } diff --git a/addons/base_import/static/src/import_model.js b/addons/base_import/static/src/import_model.js index 27ab3a165f4..eb36d7ee57c 100644 --- a/addons/base_import/static/src/import_model.js +++ b/addons/base_import/static/src/import_model.js @@ -164,7 +164,7 @@ export class BaseImportModel { const startRow = this.importOptions.skip; const importRes = { ids: [], - fields: this.columns.map((e) => Boolean(e.fieldInfo) && e.fieldInfo.name), + fields: this.columns.map((e) => Boolean(e.fieldInfo) && e.fieldInfo.fieldPath), columns: this.columns.map((e) => e.name.trim().toLowerCase()), hasError: false, }; @@ -179,7 +179,6 @@ export class BaseImportModel { } const error = await this._executeImportStep(isTest, importRes); - if (error) { let message; const errorData = error.data || {}; @@ -308,8 +307,9 @@ export class BaseImportModel { async _executeImportStep(isTest, importRes) { const importArgs = [this.id, importRes.fields, importRes.columns, this.importOptions]; - const { ids, messages, nextrow, name, error } = await this._callImport(isTest, importArgs); + + // Handle server errors if (error) { return error; } @@ -318,52 +318,13 @@ export class BaseImportModel { importRes.ids = importRes.ids.concat(ids); } + // Handle import errors if (messages && messages.length) { importRes.hasError = true; this.stopImport(); - - if (messages[0].not_matching_error) { - this._addMessage(messages[0].type, [messages[0].message]); + if (this._handleImportErrors(messages, name)) { return false; } - - const sortedMessages = this._groupErrorsByField(messages); - if (sortedMessages[0]) { - this._addMessage(sortedMessages[0].type, [sortedMessages[0].message]); - delete sortedMessages[0]; - } else { - this._addMessage("danger", [_t("The file contains blocking errors (see below)")]); - } - - for (const [columnFieldId, errors] of Object.entries(sortedMessages)) { - // Handle errors regarding specific colums. - const column = this.columns.find( - (e) => e.fieldInfo && e.fieldInfo.id === columnFieldId - ); - if (column) { - column.resultNames = name; - column.errors = errors; - } else { - for (const error of errors) { - // Handle errors regarding specific records. - if (error.record !== undefined) { - this._addMessage("danger", [ - error.rows.from === error.rows.to - ? sprintf( - _t('Error at row %s: "%s"'), - error.record, - error.message - ) - : sprintf(_t("%s at multiple rows"), error.message), - ]); - } - // Handle global errors. - else { - this._addMessage("danger", [error.message]); - } - } - } - } } this.setOption("skip", nextrow || 0); @@ -390,6 +351,47 @@ export class BaseImportModel { } } + _handleImportErrors(messages, name) { + if (messages[0].not_matching_error) { + this._addMessage(messages[0].type, [messages[0].message]); + return true; + } + + const sortedMessages = this._groupErrorsByField(messages); + if (sortedMessages[0]) { + this._addMessage(sortedMessages[0].type, [sortedMessages[0].message]); + delete sortedMessages[0]; + } else { + this._addMessage("danger", [_t("The file contains blocking errors (see below)")]); + } + + for (const [columnFieldId, errors] of Object.entries(sortedMessages)) { + // Handle errors regarding specific colums. + const column = this.columns.find( + (e) => e.fieldInfo && e.fieldInfo.fieldPath === columnFieldId + ); + if (column) { + column.resultNames = name; + column.errors = errors; + } else { + for (const error of errors) { + // Handle errors regarding specific records. + if (error.record !== undefined) { + this._addMessage("danger", [ + error.rows.from === error.rows.to + ? sprintf(_t('Error at row %s: "%s"'), error.record, error.message) + : sprintf(_t("%s at multiple rows"), error.message), + ]); + } + // Handle global errors. + else { + this._addMessage("danger", [error.message]); + } + } + } + } + } + _groupErrorsByField(messages) { const groupedErrors = {}; const errorsByMessage = groupBy(this._sortErrors(messages), (f) => f.message || "0"); @@ -455,20 +457,6 @@ export class BaseImportModel { } _getColumns(res) { - function createColumn(model, id, name, index, previews, preview) { - const fields = model._getFields(res, index); - return { - id, - name, - preview, - previews, - fields, - fieldInfo: model._findField(fields, id), - comments: [], - errors: [], - }; - } - function getId(res, index) { return res.matches && index in res.matches && res.matches[index].length > 0 ? res.matches[index].join("/") @@ -477,8 +465,8 @@ export class BaseImportModel { if (this.importOptions.has_headers && res.headers && res.preview.length > 0) { return res.headers.flatMap((header, index) => { - return createColumn( - this, + return this._createColumn( + res, getId(res, index), header, index, @@ -488,8 +476,8 @@ export class BaseImportModel { }); } else if (res.preview && res.preview.length >= 2) { return res.preview.flatMap((preview, index) => - createColumn( - this, + this._createColumn( + res, preview[0], this.importOptions.has_headers ? preview[0] : preview.join(", "), index, @@ -501,10 +489,24 @@ export class BaseImportModel { return []; } + _createColumn(res, id, name, index, previews, preview) { + const fields = this._getFields(res, index); + return { + id, + name, + preview, + previews, + fields, + fieldInfo: this._findField(fields, id), + comments: [], + errors: [], + }; + } + _findField(fields, id) { return Object.entries(fields) .flatMap((e) => e[1]) - .find((field) => field.name === id); + .find((field) => field.fieldPath === id); } /** @@ -584,7 +586,8 @@ export class BaseImportModel { column.comments = []; column.errors = []; column.resultNames = []; - column.importOptions = column.fieldInfo && this.fieldsToHandle[column.fieldInfo.id]; + column.importOptions = + column.fieldInfo && this.fieldsToHandle[column.fieldInfo.fieldPath]; if (!column.fieldInfo) { continue; @@ -603,7 +606,7 @@ export class BaseImportModel { // If multiple columns are mapped on the same field, inform // the user that they will be concatenated. const samefieldColumns = this.columns.filter( - (col) => col.fieldInfo && col.fieldInfo.id === column.fieldInfo.id + (col) => col.fieldInfo && col.fieldInfo.fieldPath === column.fieldInfo.fieldPath ); if (samefieldColumns.length >= 2) { column.comments.push({ @@ -615,7 +618,7 @@ export class BaseImportModel { } else if (updatedColumn && column.id !== updatedColumn.id && updatedColumn.fieldInfo) { // If column is mapped on an already mapped field, remove that field // from the old column to keep it unique. - if (updatedColumn.fieldInfo.id === column.fieldInfo.id) { + if (updatedColumn.fieldInfo.fieldPath === column.fieldInfo.fieldPath) { column.fieldInfo = null; } } diff --git a/addons/base_import/static/tests/import_action_tests.js b/addons/base_import/static/tests/import_action_tests.js index e3d0ecbbadc..ca38cf08970 100644 --- a/addons/base_import/static/tests/import_action_tests.js +++ b/addons/base_import/static/tests/import_action_tests.js @@ -129,9 +129,29 @@ function parsePreview(opts) { }); } +function customParsePreview(opts, { fields, headers, rowCount, matches, preview }) { + totalRows = rowCount; + + return Promise.resolve({ + advanced_mode: opts.advanced, + batch: false, + fields: fields, + file_length: opts.has_headers ? totalRows - 1 : totalRows, + header_types: false, + headers: headers, + matches: matches, + options: { + ...opts, + sheet: opts.sheet.length ? opts.sheet : "Template", + sheets: ["Template", "Template 2"], + }, + preview: preview, + }); +} + // since executing a real import would be difficult, this method simply returns // some error messages to help testing the UI -function executeFailingImport(field, isMultiline) { +function executeFailingImport(field, isMultiline, field_path = "") { let moreInfo = []; if (serverData.models.partner.fields[field].type === "selection") { moreInfo = serverData.models.partner.fields[field].selection; @@ -140,70 +160,70 @@ function executeFailingImport(field, isMultiline) { 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, }; @@ -559,7 +579,7 @@ QUnit.module("Base Import Tests", (hooks) => { registerFakeHTTPService(); const notificationMock = (message) => { assert.step(message); - return () => {}; + return () => { }; }; registry .category("services") @@ -904,8 +924,8 @@ QUnit.module("Base Import Tests", (hooks) => { ); // Check that errors have been sorted and grouped assert.strictEqual( - target.querySelector(".o_import_report p").textContent.trim(), - "Multiple errors occurred in field foo:" + target.querySelector(".o_import_report p").textContent.trim().toLowerCase(), + "multiple errors occurred in field foo:" ); assert.strictEqual( target.querySelector(".o_import_report li:first-child").textContent.trim(), @@ -1160,4 +1180,119 @@ QUnit.module("Base Import Tests", (hooks) => { "the import will resume at line 1" ); }); + + 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" + }, + ], + 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); + }, + }); + + // 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_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")); + }); + + QUnit.test("Import view: import errors with relational fields", async function (assert) { + registerFakeHTTPService(); + patchWithCleanup(browser, { + setTimeout: (fn) => fn(), + }); + + 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"] + }, + preview: [ + ["0", "1", "2"], + ["Name 1", "Name 2", "Name 3",], + ["", "1", "2"], + ], + }); + }, + "base_import.import/execute_import": (route, args) => { + return executeFailingImport(args[1][0], true, ["many2many_field", "id"]); + }, + }); + + // 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); + // For this test, we force the display of an error message if this field is set + await editSelectMenu(target, ".o_import_data_content .o_select_menu", "Many2Many"); + await click(target.querySelector(".o_control_panel button:nth-child(2)")); + + assert.strictEqual( + target.querySelector(".o_import_data_content .alert-danger").textContent, + "The file contains blocking errors (see below)", + "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_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.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"); + }); });