From 76f291b844eb2a76cfe1b27746b3906538ea1b6b Mon Sep 17 00:00:00 2001 From: Jairo Llopis Date: Tue, 18 Oct 2016 12:53:13 +0200 Subject: [PATCH] [8.0][FIX][base_export_manager] Fix "Expected singleton" bug. (#521) [FIX][base_export_manager] Fix "Expected singleton" bug. If you had a field that got translated in more than 1 addon, you'd possibly getto this error: File "/opt/odoo/0079_ahk_openerp/oca/base_export_manager/models/ir_exports_line.py", line 105, in _compute_label field.name)), File "/opt/odoo/common/openerp/v8/openerp/fields.py", line 825, in __get__ record.ensure_one() File "/opt/odoo/common/openerp/v8/openerp/models.py", line 5355, in ensure_one raise except_orm("ValueError", "Expected singleton: %s" % self) except_orm: ('ValueError', 'Expected singleton: ir.translation(4899, 703976)') With this patch, now we let Odoo return the translated string by using its standard method to do so, so we have to care for less. * Move installation outside a data file. This makes the whole installation to be able to roll back if something goes wrong, instead of entering an error loop. * Include envorionment in its manager. * Add 4th field * Move to api.multi, refactoring some stuff. - Add some comments in complex parts. - Rename `onchange_name` to `_onchange_name` (guidelines). - Make `_compute_name`'s try block shorter and easier to understand. * Allow R/W of name directly in model. * Update tests to cover new behaviors. --- base_export_manager/README.rst | 3 + base_export_manager/__init__.py | 1 + base_export_manager/__openerp__.py | 4 +- base_export_manager/data/ir_exports_data.xml | 8 - base_export_manager/hooks.py | 20 +++ .../migrations/8.0.2.1.0/post-migrate.py | 9 + base_export_manager/models/ir_exports_line.py | 168 ++++++++++-------- base_export_manager/static/src/js/main.js | 8 +- base_export_manager/tests/__init__.py | 3 +- .../tests/test_ir_exports_line.py | 53 +++++- base_export_manager/views/ir_exports_view.xml | 28 ++- 11 files changed, 200 insertions(+), 105 deletions(-) delete mode 100644 base_export_manager/data/ir_exports_data.xml create mode 100644 base_export_manager/hooks.py create mode 100644 base_export_manager/migrations/8.0.2.1.0/post-migrate.py diff --git a/base_export_manager/README.rst b/base_export_manager/README.rst index a202b9954..84cd85348 100644 --- a/base_export_manager/README.rst +++ b/base_export_manager/README.rst @@ -29,6 +29,7 @@ To manage export profiles, you need to: * Choose a name. * Choose a model (table in the database). * Choose the fields to export. + * If you choose a related field, you can choose also up to 3 levels of subfields. * You can drag & drop to reorder the fields. @@ -49,6 +50,8 @@ Known issues / Roadmap ====================== * Translated labels are not used in final exported file. +* If your database has dangling export profiles, you could get an error when + installing. You will have to manually clean it before. Bug Tracker =========== diff --git a/base_export_manager/__init__.py b/base_export_manager/__init__.py index 9861d230b..07963b3b7 100644 --- a/base_export_manager/__init__.py +++ b/base_export_manager/__init__.py @@ -3,3 +3,4 @@ # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). from . import models +from .hooks import post_init_hook diff --git a/base_export_manager/__openerp__.py b/base_export_manager/__openerp__.py index 1125b55a0..7097a982a 100644 --- a/base_export_manager/__openerp__.py +++ b/base_export_manager/__openerp__.py @@ -5,12 +5,11 @@ { 'name': "Manage model export profiles", 'category': 'Personalization', - 'version': '8.0.2.0.1', + 'version': '8.0.2.1.0', 'depends': [ 'web', ], 'data': [ - 'data/ir_exports_data.xml', 'views/assets.xml', 'views/ir_exports_view.xml', ], @@ -23,4 +22,5 @@ 'website': 'http://www.antiun.com', 'license': 'AGPL-3', 'installable': True, + 'post_init_hook': 'post_init_hook', } diff --git a/base_export_manager/data/ir_exports_data.xml b/base_export_manager/data/ir_exports_data.xml deleted file mode 100644 index e4e403f0d..000000000 --- a/base_export_manager/data/ir_exports_data.xml +++ /dev/null @@ -1,8 +0,0 @@ - - - - - - - - diff --git a/base_export_manager/hooks.py b/base_export_manager/hooks.py new file mode 100644 index 000000000..5718758cc --- /dev/null +++ b/base_export_manager/hooks.py @@ -0,0 +1,20 @@ +# -*- coding: utf-8 -*- +# Copyright 2016 Jairo Llopis +# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). +from openerp import api, SUPERUSER_ID + + +def post_init_hook(cr, registry): + """Loaded after installing the module. + + ``ir.exports.line.name`` was before a char field, and now it is a computed + char field with stored values. We have to inverse it to avoid database + inconsistencies. + """ + with api.Environment.manage(): + env = api.Environment(cr, SUPERUSER_ID, {}) + env["ir.exports.line"].search([ + ("field1_id", "=", False), + ("export_id", "!=", False), + ("name", "!=", False), + ])._inverse_name() diff --git a/base_export_manager/migrations/8.0.2.1.0/post-migrate.py b/base_export_manager/migrations/8.0.2.1.0/post-migrate.py new file mode 100644 index 000000000..146b535b8 --- /dev/null +++ b/base_export_manager/migrations/8.0.2.1.0/post-migrate.py @@ -0,0 +1,9 @@ +# -*- coding: utf-8 -*- +# Copyright 2016 Jairo Llopis +# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). +from openerp.addons.base_export_manager import post_init_hook + + +def migrate(cr, version): + """When updating, now you need the post_init_hook.""" + post_init_hook(cr, None) diff --git a/base_export_manager/models/ir_exports_line.py b/base_export_manager/models/ir_exports_line.py index 99c8962aa..9e786aa31 100644 --- a/base_export_manager/models/ir_exports_line.py +++ b/base_export_manager/models/ir_exports_line.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- # © 2015 Antiun Ingeniería S.L. - Antonio Espinosa # Copyright 2015-2016 Jairo Llopis +# Copyright 2016 Pedro M. Baeza # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). from openerp import models, fields, api, exceptions @@ -13,7 +14,6 @@ class IrExportsLine(models.Model): name = fields.Char( required=False, - readonly=True, store=True, compute="_compute_name", inverse="_inverse_name", @@ -30,6 +30,10 @@ class IrExportsLine(models.Model): "ir.model.fields", "Third field", domain="[('model_id', '=', model3_id)]") + field4_id = fields.Many2one( + "ir.model.fields", + "Fourth field", + domain="[('model_id', '=', model4_id)]") model1_id = fields.Many2one( "ir.model", "First model", @@ -44,6 +48,10 @@ class IrExportsLine(models.Model): "ir.model", "Third model", compute="_compute_model3_id") + model4_id = fields.Many2one( + "ir.model", + "Fourth model", + compute="_compute_model4_id") sequence = fields.Integer() label = fields.Char( compute="_compute_label") @@ -54,103 +62,117 @@ class IrExportsLine(models.Model): return self.env.context.get("default_model1_id", False) @api.multi - @api.depends("field1_id", "field2_id", "field3_id") + @api.depends("field1_id", "field2_id", "field3_id", "field4_id") def _compute_name(self): """Get the name from the selected fields.""" - for s in self: - s.name = "/".join((s.field_n(num).name - for num in range(1, 4) - if s.field_n(num))) + for one in self: + name = "/".join((one.field_n(num).name for num in range(1, 5) + if one.field_n(num))) + if name != one.name: + one.name = name @api.multi @api.depends("field1_id") def _compute_model2_id(self): """Get the related model for the second field.""" - ir_model = self.env["ir.model"] - for s in self: - s.model2_id = ( - s.field1_id.ttype and - "2" in s.field1_id.ttype and - ir_model.search([("model", "=", s.field1_id.relation)])) + IrModel = self.env["ir.model"] + for one in self: + one.model2_id = ( + one.field1_id.ttype and + "2" in one.field1_id.ttype and + IrModel.search([("model", "=", one.field1_id.relation)])) @api.multi @api.depends("field2_id") def _compute_model3_id(self): """Get the related model for the third field.""" - ir_model = self.env["ir.model"] - for s in self: - s.model3_id = ( - s.field2_id.ttype and - "2" in s.field2_id.ttype and - ir_model.search([("model", "=", s.field2_id.relation)])) + IrModel = self.env["ir.model"] + for one in self: + one.model3_id = ( + one.field2_id.ttype and + "2" in one.field2_id.ttype and + IrModel.search([("model", "=", one.field2_id.relation)])) + + @api.multi + @api.depends("field3_id") + def _compute_model4_id(self): + """Get the related model for the third field.""" + IrModel = self.env["ir.model"] + for one in self: + one.model4_id = ( + one.field3_id.ttype and + "2" in one.field3_id.ttype and + IrModel.search([("model", "=", one.field3_id.relation)])) @api.multi @api.depends('name') def _compute_label(self): """Column label in a user-friendly format and language.""" - translations = self.env["ir.translation"] - for s in self: + for one in self: parts = list() - for num in range(1, 4): - field = s.field_n(num) + for num in range(1, 5): + field = one.field_n(num) if not field: break - # Translate label if possible - parts.append( - translations.search([ - ("type", "=", "field"), - ("lang", "=", self.env.context.get("lang")), - ("name", "=", "%s,%s" % (s.model_n(num).model, - field.name)), - ]).value or - field.display_name) - s.label = ("%s (%s)" % ("/".join(parts), s.name) - if parts and s.name else False) + try: + parts.append( + one.env[one.model_n(num).model]._fields[field.name] + .get_description(one.env)["string"]) + except KeyError: + # No human-readable string available, so empty this + return + one.label = ("%s (%s)" % ("/".join(parts), one.name) + if parts and one.name else False) @api.multi def _inverse_name(self): """Get the fields from the name.""" - for s in self: - # Field names can have up to only 3 indentation levels - parts = s.name.split("/", 2) + for one in self: + # Field names can have up to only 4 indentation levels + parts = one.name.split("/") + if len(parts) > 4: + raise exceptions.ValidationError( + _("It's not allowed to have more than 4 levels depth: " + "%s") % one.name) + for num in range(1, 5): + if num > len(parts): + # Empty subfield in this case + one[one.field_n(num, True)] = False + continue + field_name = parts[num - 1] + model = one.model_n(num) + # You could get to failing constraint while populating the + # fields, so we skip the uniqueness check and manually check + # the full constraint after the loop + one.with_context(skip_check=True)[one.field_n(num, True)] = ( + one._get_field_id(model, field_name)) + one._check_name() - for num in range(1, 4): - try: - # Fail in excessive subfield level - field_name = parts[num - 1] - except IndexError: - # Remove subfield on failure - s[s.field_n(num, True)] = False - else: - model = s.model_n(num) - s[s.field_n(num, True)] = self._get_field_id( - model, field_name) - - @api.one - @api.constrains("field1_id", "field2_id", "field3_id") + @api.multi + @api.constrains("field1_id", "field2_id", "field3_id", "field4_id") def _check_name(self): - if not self.label: - raise exceptions.ValidationError( - _("Field '%s' does not exist") % self.name) - lines = self.search([('export_id', '=', self.export_id.id), - ('name', '=', self.name)]) - if len(lines) > 1: - raise exceptions.ValidationError( - _("Field '%s' already exists") % self.name) + for one in self: + if not one.label: + raise exceptions.ValidationError( + _("Field '%s' does not exist") % one.name) + if not one.env.context.get('skip_check'): + lines = one.search([('export_id', '=', one.export_id.id), + ('name', '=', one.name)]) + if len(lines) > 1: + raise exceptions.ValidationError( + _("Field '%s' already exists") % one.name) - @api.model - def _install_base_export_manager(self): - """Populate ``field*_id`` fields.""" - self.search([("export_id", "=", False)]).unlink() - lines = self.search( - [("field1_id", "=", False), ("name", "!=", False)]) - for line in lines: - try: - line._inverse_name() - except: - # Prevent possible inexisting fields - pass + @api.multi + @api.onchange('name') + def _onchange_name(self): + if self.name: + self._inverse_name() + else: + self.field1_id = False + self.field2_id = False + self.field3_id = False + self.field4_id = False @api.model def _get_field_id(self, model, name): @@ -162,9 +184,13 @@ class IrExportsLine(models.Model): :param str name: Technical name of the field, like ``child_ids``. """ - return self.env["ir.model.fields"].search( + field = self.env["ir.model.fields"].search( [("name", "=", name), ("model_id", "=", model.id)]) + if not field.exists(): + raise exceptions.ValidationError( + _("Field '%s' not found in model '%s'") % (name, model.model)) + return field @api.multi def field_n(self, n, only_name=False): diff --git a/base_export_manager/static/src/js/main.js b/base_export_manager/static/src/js/main.js index 43d04a5fe..76f0c9039 100644 --- a/base_export_manager/static/src/js/main.js +++ b/base_export_manager/static/src/js/main.js @@ -2,7 +2,7 @@ * License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). */ // Check jQuery available -if (typeof jQuery === 'undefined') { throw new Error('Requires jQuery') } +if (typeof jQuery === 'undefined') { throw new Error('Requires jQuery'); } +function ($) { 'use strict'; @@ -21,12 +21,12 @@ if (typeof jQuery === 'undefined') { throw new Error('Requires jQuery') } }, add_field: function(field_id, string) { var field_list = this.$el.find('#fields_list'); - if (this.$el.find("#fields_list option[value='" + field_id + "']") - && !this.$el.find("#fields_list option[value='" + field_id + "']").length) { + if (this.$el.find("#fields_list option[value='" + field_id + "']") && + !this.$el.find("#fields_list option[value='" + field_id + "']").length) { field_list.append(new Option(string + ' (' + field_id + ')', field_id)); } }, }); - } + }; }(jQuery); diff --git a/base_export_manager/tests/__init__.py b/base_export_manager/tests/__init__.py index 07077ba6d..eea19d973 100644 --- a/base_export_manager/tests/__init__.py +++ b/base_export_manager/tests/__init__.py @@ -3,4 +3,5 @@ # © 2015 Antiun Ingeniería S.L. - Jairo Llopis # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). -from . import test_ir_exports, test_ir_exports_line +from . import test_ir_exports +from . import test_ir_exports_line diff --git a/base_export_manager/tests/test_ir_exports_line.py b/base_export_manager/tests/test_ir_exports_line.py index 4954cc5e0..5a68482d6 100644 --- a/base_export_manager/tests/test_ir_exports_line.py +++ b/base_export_manager/tests/test_ir_exports_line.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- # © 2015 Antiun Ingenieria S.L. - Javier Iniesta +# Copyright 2016 Pedro M. Baeza # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). from openerp.tests.common import TransactionCase @@ -7,12 +8,19 @@ from openerp.exceptions import ValidationError class TestIrExportsLineCase(TransactionCase): - def setUp(self): super(TestIrExportsLineCase, self).setUp() m_ir_exports = self.env['ir.exports'] self.export = m_ir_exports.create({'name': 'Partner Test', 'resource': 'res.partner'}) + self.partner_model = self.env['ir.model'].search( + [('model', '=', 'res.partner')]) + self.field_parent_id = self.env['ir.model.fields'].search( + [('name', '=', 'parent_id'), + ('model_id', '=', self.partner_model.id)]) + self.field_name = self.env['ir.model.fields'].search( + [('name', '=', 'name'), + ('model_id', '=', self.partner_model.id)]) def test_check_name(self): m_ir_exports_line = self.env['ir.exports.line'] @@ -34,3 +42,46 @@ class TestIrExportsLineCase(TransactionCase): with self.assertRaises(ValidationError): m_ir_exports_line.create({'name': '', 'export_id': self.export.id}) + + def test_model_default_by_context(self): + """Fields inherit the model_id by context.""" + line = self.env["ir.exports.line"].with_context( + default_model1_id=self.export.model_id.id).create({ + "name": "name", + "export_id": self.export.id, + }) + self.assertEqual(line.model1_id, self.export.model_id) + + def test_inverse_name(self): + line = self.env['ir.exports.line'].create({ + 'export_id': self.export.id, + 'name': 'parent_id/parent_id/parent_id/name', + }) + self.assertEqual(line.model1_id, self.partner_model) + self.assertEqual(line.model2_id, self.partner_model) + self.assertEqual(line.field1_id, self.field_parent_id) + self.assertEqual(line.field2_id, self.field_parent_id) + self.assertEqual(line.field3_id, self.field_parent_id) + self.assertEqual(line.field4_id, self.field_name) + + def test_compute_name(self): + line = self.env['ir.exports.line'].create({ + 'export_id': self.export.id, + 'field1_id': self.field_parent_id.id, + 'field2_id': self.field_parent_id.id, + 'field3_id': self.field_parent_id.id, + 'field4_id': self.field_name.id, + }) + self.assertEqual(line.name, 'parent_id/parent_id/parent_id/name') + + def test_write_name_same_root(self): + self.env['ir.exports.line'].create({ + 'export_id': self.export.id, + 'name': 'parent_id', + }) + line = self.env['ir.exports.line'].create({ + 'export_id': self.export.id, + 'name': 'name', + }) + # This should end without errors + line.name = 'parent_id/name' diff --git a/base_export_manager/views/ir_exports_view.xml b/base_export_manager/views/ir_exports_view.xml index a9b9b9a6b..c184cec06 100644 --- a/base_export_manager/views/ir_exports_view.xml +++ b/base_export_manager/views/ir_exports_view.xml @@ -47,33 +47,25 @@ + + options="{'no_open': True, 'no_create': True}"/> + attrs="{'readonly': [('model2_id', '=', False)]}" + options="{'no_open': True, 'no_create': True}"/> + attrs="{'readonly': [('model3_id', '=', False)]}" + options="{'no_open': True, 'no_create': True}"/> +