From 662ecf4e20ceab92b62804cb2a06e51f6b897623 Mon Sep 17 00:00:00 2001 From: Nikunj Ladava Date: Mon, 14 Jun 2021 10:44:40 +0000 Subject: [PATCH] [IMP] base_import: improve import wizard preview PURPOSE Improve import wizard as followup of recent improvements. SPECIFICATIONS Preview lines - make the preview lines more visible - set font-weight: 500 on o_import_header_name - remove the text-muted class from the data preview line - allow the user to preview the first 5 non-empty values - when hovering on the data preview span, display a tooltip with the first 5 non-empty values of the file column (with 'Preview' as the tooltip title) - In the 'when a value cannot be matched' dropdown: add a new 'skip record' option for the following field types: boolean, many2one, many2many, selection add a new 'set empty' for the following field types: many2one, many2many, selection hide this option if the field is required When a value cannot be matched: the 'skip record' option will make sure that lines with an unmatched value will not be imported (i.e. skipped at import) the 'set empty' option will set the field value to False when the value cannot be matched - Restore colored background on alert boxes by removing some css rules Task-2504343 closes odoo/odoo#73713 Signed-off-by: Thibault Delavallee (tde) Co-authored-by: Mohammed Shekha --- addons/base_import/models/base_import.py | 18 +++-- .../static/src/js/import_action.js | 38 +++++++++-- .../static/src/scss/base_import.scss | 15 +---- .../static/src/xml/base_import.xml | 15 ++++- addons/base_import/tests/test_base_import.py | 65 ++++++++++++++++++- addons/base_import/tests/test_csv_magic.py | 10 +-- odoo/addons/base/models/ir_fields.py | 22 +++++-- odoo/models.py | 3 + 8 files changed, 145 insertions(+), 41 deletions(-) diff --git a/addons/base_import/models/base_import.py b/addons/base_import/models/base_import.py index 2b7ed221fe0..643af98f570 100644 --- a/addons/base_import/models/base_import.py +++ b/addons/base_import/models/base_import.py @@ -950,16 +950,21 @@ class Import(models.TransientModel): has_relational_match = any(len(match) > 1 for field, match in matches.items() if match) advanced_mode = has_relational_header or has_relational_match - # Take the first non null value for each column to display example to user + # Take first non null values for each column to show preview to users. + # Initially first non null value is displayed to the user. + # On hover preview consists in 5 values. column_example = [] for column_index, _unused in enumerate(preview[0]): + vals = [] for record in preview: if record[column_index]: - column_example.append("%s%s" % (record[column_index][:50], "..." if len(record[column_index]) > 50 else "")) + vals.append("%s%s" % (record[column_index][:50], "..." if len(record[column_index]) > 50 else "")) + if len(vals) == 5: break - # add a blank value if no example have been found at all for the current column - else: - column_example.append("") + column_example.append( + vals or + [""] # blank value if no example have been found at all for the current column + ) # Batch management batch = False @@ -1304,7 +1309,8 @@ class Import(models.TransientModel): model = self.env[self.res_model].with_context( import_file=True, name_create_enabled_fields=name_create_enabled_fields, - import_skip_fields=options.get('import_skip_fields'), + import_set_empty_fields=options.get('import_set_empty_fields', []), + import_skip_records=options.get('import_skip_records', []), _import_limit=import_limit) import_result = model.load(import_fields, merged_data) _logger.info('done') diff --git a/addons/base_import/static/src/js/import_action.js b/addons/base_import/static/src/js/import_action.js index c2fb9e1a362..d839f487ad5 100644 --- a/addons/base_import/static/src/js/import_action.js +++ b/addons/base_import/static/src/js/import_action.js @@ -311,7 +311,8 @@ var DataImport = AbstractAction.extend({ advanced: this.$('input.oe_import_advanced_mode').prop('checked'), keep_matches: this.do_not_change_match, name_create_enabled_fields: {}, - import_skip_fields: [], + import_set_empty_fields: [], + import_skip_records: [], fallback_values: {}, // start at row 1 = skip 0 lines skip: Number(this.$('#oe_import_row_start').val()) - 1 || 0, @@ -339,9 +340,11 @@ var DataImport = AbstractAction.extend({ var field = this.getAttribute('field'); var type = this.getAttribute('type'); if (field) { - if (['many2one', "many2many"].includes(type)) { - if (this.value === 'skip') { - options.import_skip_fields.push(field); + if (['boolean', 'many2one', 'many2many', 'selection'].includes(type) && this.value === 'skip_record') { + options.import_skip_records.push(field); + } else if (['many2one', "many2many", "selection"].includes(type)) { + if (this.value === 'set_empty') { + options.import_set_empty_fields.push(field); } else { options.name_create_enabled_fields[field] = this.value === 'create'; } @@ -456,6 +459,14 @@ var DataImport = AbstractAction.extend({ this.$form.addClass('oe_import_preview'); this.$('input.oe_import_advanced_mode').prop('checked', result.advanced_mode); this.$('.oe_import_grid').html(QWeb.render('ImportView.preview', result)); + this.$('.oe_import_grid .o_import_preview').each((index, element) => { + $(element).popover({ + title: _t("Preview"), + trigger: 'hover', + html: true, + content: QWeb.render('ImportView.preview_popover', { preview: result.preview[index] }), + }); + }); // Activate the batch configuration panel only of the file length > 100. (In order to let the user choose // the batch size even for medium size file. Could be useful to reduce the batch size for complex models). this.fileLength = result.file_length; @@ -515,7 +526,18 @@ var DataImport = AbstractAction.extend({ var $commentCell = $fieldInput.closest('tr.oe_import_grid-row').find('.oe_import_comment_cell'); var $optionsDiv = $commentCell.find('.oe_import_options_div'); var isRelational = fieldInfo.type === 'many2many' || fieldInfo.type === 'many2one'; - var showSkip = !fieldInfo.required && (isRelational || fieldInfo.type === 'selection'); + var setEmpty = !fieldInfo.required && (isRelational || fieldInfo.type === 'selection'); + + // if relational field is child of one2many then do not display skip record option + var isO2MField = false; + if (isRelational && fieldInfo.id.includes('/')) { + var fieldName = fieldInfo.id.split('/')[0]; + var field = result.fields.find((field) => field.name === fieldName); + if (field && field.type === 'one2many') { + isO2MField = true; + } + } + var showSkipRecord = setEmpty && !isO2MField || (!fieldInfo.required && fieldInfo.type === 'boolean'); // get options cell related to that field if (isRelational || fieldInfo.type === 'boolean') { @@ -524,7 +546,8 @@ var DataImport = AbstractAction.extend({ QWeb.render('ImportView.create_record_option', { data: fieldInfo, is_relational: isRelational, - show_skip: showSkip, + set_empty: setEmpty, + show_skip_record: showSkipRecord }) ); } else if (fieldInfo.type === 'selection') { @@ -540,7 +563,8 @@ var DataImport = AbstractAction.extend({ QWeb.render('ImportView.create_record_option', { data: fieldInfo, values: selectionLabels, - show_skip: showSkip, + set_empty: setEmpty, + show_skip_record: showSkipRecord }) ); self.selectionFields[data.id] = values; diff --git a/addons/base_import/static/src/scss/base_import.scss b/addons/base_import/static/src/scss/base_import.scss index d7de1ace8bc..46369769d07 100644 --- a/addons/base_import/static/src/scss/base_import.scss +++ b/addons/base_import/static/src/scss/base_import.scss @@ -161,25 +161,13 @@ .o_single_line_text { @include o-text-overflow($display: table-cell, $max-width: $cell-max-width - $cell-padding); } - .alert { - // alias -error to -danger - &.alert-error { - background-color: transparent; - } - &.alert-warning { - background-color: transparent; - } - &.alert-info { - background-color: transparent; - } - } } } } /* Default Color for placeholder on import fields*/ .select2-default{ - color: #F0AD4E !important; + color: #D67D00 !important; font-weight: 500; } @@ -203,6 +191,7 @@ background-repeat: no-repeat; appearance: none; width: auto; + background-color: white; } .select2-arrow { diff --git a/addons/base_import/static/src/xml/base_import.xml b/addons/base_import/static/src/xml/base_import.xml index 5198eac0d8f..21d8cdf0d1e 100644 --- a/addons/base_import/static/src/xml/base_import.xml +++ b/addons/base_import/static/src/xml/base_import.xml @@ -56,7 +56,8 @@ - + + @@ -80,11 +81,13 @@ - + Untitled - + + + @@ -109,6 +112,12 @@ + +
+
+
+ +

Import preview failed due to: .

diff --git a/addons/base_import/tests/test_base_import.py b/addons/base_import/tests/test_base_import.py index 4b5224f0db0..2a70080fe5d 100644 --- a/addons/base_import/tests/test_base_import.py +++ b/addons/base_import/tests/test_base_import.py @@ -357,7 +357,7 @@ class TestPreview(TransactionCase): {'id': 'somevalue', 'name': 'somevalue', 'string': 'Some Value', 'required': True, 'fields': [], 'type': 'integer', 'model_name': 'base_import.tests.models.preview'}, {'id': 'othervalue', 'name': 'othervalue', 'string': 'Other Variable', 'required': False, 'fields': [], 'type': 'integer', 'model_name': 'base_import.tests.models.preview'}, ]) - self.assertEqual(result['preview'], ['foo', '5', '4']) + self.assertEqual(result['preview'], [['foo', 'bar', 'qux'], ['5'], ['4', '6']]) @unittest.skipUnless(can_import('xlrd'), "XLRD module not available") def test_xls_success(self): @@ -381,7 +381,7 @@ class TestPreview(TransactionCase): {'id': 'somevalue', 'name': 'somevalue', 'string': 'Some Value', 'required': True, 'fields': [], 'type': 'integer', 'model_name': 'base_import.tests.models.preview'}, {'id': 'othervalue', 'name': 'othervalue', 'string': 'Other Variable', 'required': False, 'fields': [], 'type': 'integer', 'model_name': 'base_import.tests.models.preview'}, ]) - self.assertEqual(result['preview'], ['foo', '1', '2']) + self.assertEqual(result['preview'], [['foo', 'bar', 'qux'], ['1', '3', '5'], ['2', '4', '6']]) @unittest.skipUnless(can_import('xlrd.xlsx'), "XLRD/XLSX not available") def test_xlsx_success(self): @@ -405,7 +405,7 @@ class TestPreview(TransactionCase): {'id': 'somevalue', 'name': 'somevalue', 'string': 'Some Value', 'required': True, 'fields': [], 'type': 'integer', 'model_name': 'base_import.tests.models.preview'}, {'id': 'othervalue', 'name': 'othervalue', 'string': 'Other Variable', 'required': False, 'fields': [], 'type': 'integer', 'model_name': 'base_import.tests.models.preview'}, ]) - self.assertEqual(result['preview'], ['foo', '1', '2']) + self.assertEqual(result['preview'], [['foo', 'bar', 'qux'], ['1', '3', '5'], ['2', '4', '6']]) @unittest.skipUnless(can_import('odf'), "ODFPY not available") def test_ods_success(self): @@ -648,6 +648,65 @@ class test_convert_import_data(TransactionCase): self.assertItemsEqual(data, [data_row]) + def test_set_empty_value_import(self): + partners_before = self.env['res.partner'].search([]) + import_wizard = self.env['base_import.import'].create({ + 'res_model': 'res.partner', + 'file': """foo,US,person\n +foo1,Invalid Country,person\n +foo2,US,persons\n""", + 'file_type': 'text/csv' + }) + + results = import_wizard.execute_import( + ['name', 'country_id', 'company_type'], + [], + { + 'quoting': '"', + 'separator': ',', + 'import_set_empty_fields': ['country_id', 'company_type'], + } + ) + partners_now = self.env['res.partner'].search([]) - partners_before + self.assertEqual(len(results['ids']), 3, "should have imported the first 3 records in full, got %s" % results['ids']) + + self.assertEqual(partners_now[0].name, 'foo', "New partner's name should be foo") + self.assertEqual(partners_now[0].country_id.id, self.env.ref('base.us').id, "Foo partner's country should be US") + self.assertEqual(partners_now[0].company_type, 'person', "Foo partner's country should be person") + + self.assertEqual(partners_now[1].country_id.id, False, "foo1 partner's country should be False") + + self.assertEqual(partners_now[2].company_type, False, "foo2 partner's country should be False") + # if results empty, no errors + self.assertItemsEqual(results['messages'], []) + + def test_skip_record_import(self): + partners_before = self.env['res.partner'].search([]) + import_wizard = self.env['base_import.import'].create({ + 'res_model': 'res.partner', + 'file': """foo,US,0,person\n +foo1,Invalid Country,0,person\n +foo2,US,False Value,person\n +foo3,US,0,persons\n""", + 'file_type': 'text/csv' + }) + + results = import_wizard.execute_import( + ['name', 'country_id', 'is_company', 'company_type'], + [], + { + 'quoting': '"', + 'separator': ',', + 'import_skip_records': ['country_id', 'is_company', 'company_type'] + } + ) + partners_now = self.env['res.partner'].search([]) - partners_before + + self.assertEqual(len(results['ids']), 1, "should have imported the first record in full, got %s" % results['ids']) + self.assertEqual(partners_now.name, 'foo', "New partner's name should be foo") + # if results empty, no errors + self.assertItemsEqual(results['messages'], []) + class TestBatching(TransactionCase): def _makefile(self, rows): diff --git a/addons/base_import/tests/test_csv_magic.py b/addons/base_import/tests/test_csv_magic.py index 4e693ed32a1..bc4c1d89841 100644 --- a/addons/base_import/tests/test_csv_magic.py +++ b/addons/base_import/tests/test_csv_magic.py @@ -67,7 +67,7 @@ class TestEncoding(ImportCase): }) self.assertIsNone(r.get('error')) self.assertEqual(r['options']['encoding'], 'iso-8859-1') - self.assertEqual(r['preview'], [s.decode('iso-8859-1')]) + self.assertEqual(r['preview'], [[s.decode('iso-8859-1'), 'text']]) class TestFileSeparator(ImportCase): @@ -90,7 +90,7 @@ d|4 }) self.assertIsNone(r.get('error')) self.assertEqual(r['headers'], ['c', 'f']) - self.assertEqual(r['preview'], ['a', '1']) + self.assertEqual(r['preview'], [['a', 'b', 'c', 'd'], ['1', '2', '3', '4']]) self.assertEqual(r['options']['separator'], '|') def test_explicit_fail(self): @@ -103,7 +103,7 @@ d|4 }) self.assertIsNone(r.get('error')) self.assertEqual(r['headers'], ['c|f']) - self.assertEqual(r['preview'], ['a|1']) + self.assertEqual(r['preview'], [['a|1', 'b|2', 'c|3', 'd|4']]) self.assertEqual(r['options']['separator'], ',') def test_guess_ok(self): @@ -114,7 +114,7 @@ d|4 }) self.assertIsNone(r.get('error')) self.assertEqual(r['headers'], ['c', 'f']) - self.assertEqual(r['preview'], ['a', '1']) + self.assertEqual(r['preview'], [['a', 'b', 'c', 'd'], ['1', '2', '3', '4']]) self.assertEqual(r['options']['separator'], '|') def test_noguess(self): @@ -129,7 +129,7 @@ d|4 }) self.assertIsNone(r.get('error')) self.assertEqual(r['headers'], ['c']) - self.assertEqual(r['preview'], ['a']) + self.assertEqual(r['preview'], [['a', 'b', 'c', 'd']]) self.assertEqual(r['options']['separator'], '') diff --git a/odoo/addons/base/models/ir_fields.py b/odoo/addons/base/models/ir_fields.py index 8dc639b2ee1..f87769b17eb 100644 --- a/odoo/addons/base/models/ir_fields.py +++ b/odoo/addons/base/models/ir_fields.py @@ -204,6 +204,9 @@ class IrFieldsConverter(models.AbstractModel): if value.lower() in falses: return False, [] + if field.name in self._context.get('import_skip_records', []): + return None, [] + return True, [self._format_import_error( ValueError, _(u"Unknown value '%s' for boolean field '%%(field)s'"), @@ -319,6 +322,10 @@ class IrFieldsConverter(models.AbstractModel): if value.lower() == str(item).lower() or any(value.lower() == label.lower() for label in labels): return item, [] + if field.name in self._context.get('import_skip_records', []): + return None, [] + elif field.name in self._context.get('import_set_empty_fields', []): + return False, [] raise self._format_import_error( ValueError, _(u"Value '%s' not found in selection field '%%(field)s'"), @@ -417,12 +424,14 @@ class IrFieldsConverter(models.AbstractModel): subfield ) - import_skip = False + set_empty = False + skip_record = False if self.env.context.get('import_file'): - import_skip_fields = self.env.context.get('import_skip_fields') or [] + import_set_empty_fields = self.env.context.get('import_set_empty_fields') or [] field_path = "/".join((self.env.context.get('parent_fields_hierarchy', []) + [field.name])) - import_skip = field_path in import_skip_fields - if id is None and not import_skip: + set_empty = field_path in import_set_empty_fields + skip_record = field_path in self.env.context.get('import_skip_records', []) + if id is None and not set_empty and not skip_record: if error_msg: message = _("No matching record found for %(field_type)s '%(value)s' in field '%%(field)s' and the following error was encountered when we attempted to create one: %(error_message)s") else: @@ -516,6 +525,11 @@ class IrFieldsConverter(models.AbstractModel): ids.append(id) warnings.extend(ws) + if field.name in self._context.get('import_set_empty_fields', []) and any([id is None for id in ids]): + ids = [id for id in ids if id] + elif field.name in self._context.get('import_skip_records', []) and any([id is None for id in ids]): + return None, warnings + if self._context.get('update_many2many'): return [Command.link(id) for id in ids], warnings else: diff --git a/odoo/models.py b/odoo/models.py index f0d92b697f3..6e16adb906d 100644 --- a/odoo/models.py +++ b/odoo/models.py @@ -1192,6 +1192,9 @@ class BaseModel(metaclass=MetaModel): info = {'rows': {'to': -1}} for id, xid, record, info in converted: + if self.env.context.get('import_file') and self.env.context.get('import_skip_records'): + if any([record.get(field) is None for field in self.env.context['import_skip_records']]): + continue if xid: xid = xid if '.' in xid else "%s.%s" % (current_module, xid) batch_xml_ids.add(xid)