From f02b23006b63c375258cf5a33c903318e59e7bbb Mon Sep 17 00:00:00 2001 From: Xavier ALT Date: Mon, 30 May 2016 12:15:06 +0200 Subject: [PATCH] [FIX] fields: during an onchange(), do not invalidate *2many fields because of their domain Our usage of domain on fields One2many seems to trigger an obscure behaviour on onchange. With the following (simplified) config: Message(models.Model): _name = 'test_new_api.message' important = fields.Boolean('Important') Discussion(models.Model): _name = 'test_new_api.discussion' name = fields.Char('Name') important_emails = fields.One2Many('test_new_api.emailmessage', 'discussion', domain=[('important', '=', True)]) Email(models.Model): _name = 'test_new_api.emailmessage' _inherits = {'test_new_api.message': 'message'} discussion = fields.Many2one('test_new_api.discussion', 'Discussion') message = fields.Many2one('test_new_api.message', 'Message') Steps: - We change 'name' on discussion, triggers an `onchange()` call - we ends up filling cache on virtual record (on secondary fields, we calling record.mapped('important_emails.important')) - we get a cache miss ('important' field not provided, only 'important_emails' ids, i.e with no change on existing records) - we fill the cache, this mark 'important' field as modified - because of commit 5676d81 and because 'important' is that case is a related (i.e computed) field we triggers cache recomputation - as there is no way to recompute 'important_emails' for virtual record (no real ID) we ends up with empty 'important_emails' generating removal of existing records. => Finally changing any value for 'test_new_api.discussion' that trigger an onchange will always reset 'important_emails' to empty Fixed by Raphael Collet , and test by Xavier Alt . --- .../addons/test_new_api/ir.model.access.csv | 1 + openerp/addons/test_new_api/models.py | 11 ++++ .../test_new_api/tests/test_onchange.py | 58 +++++++++++++++++++ openerp/addons/test_new_api/views.xml | 23 ++++++++ openerp/fields.py | 7 +++ 5 files changed, 100 insertions(+) diff --git a/openerp/addons/test_new_api/ir.model.access.csv b/openerp/addons/test_new_api/ir.model.access.csv index c05160dde5e..568a2703565 100644 --- a/openerp/addons/test_new_api/ir.model.access.csv +++ b/openerp/addons/test_new_api/ir.model.access.csv @@ -2,6 +2,7 @@ access_category,test_new_api_category,test_new_api.model_test_new_api_category,,1,1,1,1 access_discussion,test_new_api_discussion,test_new_api.model_test_new_api_discussion,,1,1,1,1 access_message,test_new_api_message,test_new_api.model_test_new_api_message,,1,1,1,1 +access_emailmessage,test_new_api_emailmessage,test_new_api.model_test_new_api_emailmessage,,1,1,1,1 access_multi,test_new_api_multi,test_new_api.model_test_new_api_multi,,1,1,1,1 access_multi_line,test_new_api_multi_line,test_new_api.model_test_new_api_multi_line,,1,1,1,1 access_mixed,test_new_api_mixed,test_new_api.model_test_new_api_mixed,,1,1,1,1 diff --git a/openerp/addons/test_new_api/models.py b/openerp/addons/test_new_api/models.py index 2d6d17db541..d08bbcdeb90 100644 --- a/openerp/addons/test_new_api/models.py +++ b/openerp/addons/test_new_api/models.py @@ -145,6 +145,9 @@ class Discussion(models.Model): message_changes = fields.Integer(string='Message changes') important_messages = fields.One2many('test_new_api.message', 'discussion', domain=[('important', '=', True)]) + emails = fields.One2many('test_new_api.emailmessage', 'discussion') + important_emails = fields.One2many('test_new_api.emailmessage', 'discussion', + domain=[('important', '=', True)]) @api.onchange('moderator') def _onchange_moderator(self): @@ -225,6 +228,14 @@ class Message(models.Model): return [('author.partner_id', operator, value)] +class EmailMessage(models.Model): + _name = 'test_new_api.emailmessage' + _inherits = {'test_new_api.message': 'message'} + + message = fields.Many2one('test_new_api.message', 'Message', + required=True, ondelete='cascade') + email_to = fields.Char('To') + class Multi(models.Model): """ Model for testing multiple onchange methods in cascade that modify a one2many field several times. diff --git a/openerp/addons/test_new_api/tests/test_onchange.py b/openerp/addons/test_new_api/tests/test_onchange.py index 062cf6c4532..d30109acfaf 100644 --- a/openerp/addons/test_new_api/tests/test_onchange.py +++ b/openerp/addons/test_new_api/tests/test_onchange.py @@ -8,6 +8,7 @@ class TestOnChange(common.TransactionCase): super(TestOnChange, self).setUp() self.Discussion = self.env['test_new_api.discussion'] self.Message = self.env['test_new_api.message'] + self.EmailMessage = self.env['test_new_api.emailmessage'] def test_default_get(self): """ checking values returned by default_get() """ @@ -237,3 +238,60 @@ class TestOnChange(common.TransactionCase): result = discussion.onchange(values, 'messages', field_onchange) self.assertIn('message_changes', result['value']) self.assertEqual(result['value']['message_changes'], len(discussion.messages)) + + def test_onchange_one2many_with_domain_on_related_field(self): + """ test the value of the one2many field when defined with a domain on a related field""" + discussion = self.env.ref('test_new_api.discussion_0') + demo = self.env.ref('base.user_demo') + + # mimic UI behaviour, so we get subfields + # (we need at least subfield: 'important_emails.important') + view_info = self.Discussion.fields_view_get( + view_id=self.env.ref('test_new_api.discussion_form').id, + view_type='form') + field_onchange = self.Discussion._onchange_spec(view_info=view_info) + self.assertEqual(field_onchange.get('messages'), '1') + + BODY = "What a beautiful day!" + USER = self.env.user + + # create standalone email + email = self.EmailMessage.create({ + 'discussion': discussion.id, + 'name': "[%s] %s" % ('', USER.name), + 'body': BODY, + 'author': USER.id, + 'important': False, + }) + + # check if server-side cache is working correctly + self.env.invalidate_all() + self.assertIn(email, discussion.emails) + self.assertNotIn(email, discussion.important_emails) + email.important = True + self.assertIn(email, discussion.important_emails) + + # check that when trigger an onchange, we don't reset important emails + # (force `invalidate_all` as but appear in onchange only when we get a + # cache miss) + self.env.invalidate_all() + self.assertEqual(len(discussion.messages), 4) + values = { + 'name': "Foo Bar", + 'moderator': demo.id, + 'categories': [(4, cat.id) for cat in discussion.categories], + 'messages': [(4, msg.id) for msg in discussion.messages], + 'participants': [(4, usr.id) for usr in discussion.participants], + 'message_changes': 0, + 'important_messages': [(4, msg.id) for msg in discussion.important_messages], + 'important_emails': [(4, eml.id) for eml in discussion.important_emails], + } + result = discussion.onchange(values, 'name', field_onchange) + + # When one2many domain contains non-computed field, things are ok + self.assertEqual(result['value']['important_messages'], + [(1, email.message.id, {'name': u'[Foo Bar] %s' % USER.name})]) + + # But here with commit 5676d81, we get value of: [(2, email.id)] + self.assertEqual(result['value']['important_emails'], + [(1, email.id, {'name': u'[Foo Bar] %s' % USER.name})]) diff --git a/openerp/addons/test_new_api/views.xml b/openerp/addons/test_new_api/views.xml index cc2b363f753..3870764f5a5 100644 --- a/openerp/addons/test_new_api/views.xml +++ b/openerp/addons/test_new_api/views.xml @@ -54,12 +54,35 @@ +
+ + +