From 9d74b1f73a92b9030438f590f729bb7866cba191 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 9 Jan 2020 12:14:27 +0000 Subject: [PATCH] [FIX] core: saving of o2m value init'd via onchange In the SSF, o2m updates get initialized with no values (1, id, {}). The record data is only fetched when the "record" gets updated explicitly from which updates will hopefully get properly tracked & saved. However if the "record" was first initialized through an onchange values which are updated by the onchange (diverging from the db) those would not get tracked and thus wouldn't get saved when the record is saved. * use more specific placeholder (None) for "o2m records to update but we don't have values yet" * once we have values, always store them as an update-tracking dict * if we don't have values yet for an o2m and an onchange is trying to write to it, initialize with values from database first (might eventually be a good idea to initialize upfront though there's the question of what happens for default values and recursive views) * mark anything coming back from the onchange and differing from local values as changed (so they get sent out on save) * properly reify parent values for onchange instead of sending them as-is * the evaluation context for contexts (and domains) needs properly formatted values so use `_values_to_save` to get them, however it cares about neither required-ing nor filtering out e.g. unmodified fields, therefore add an awful toggle to handle this Task 2150302 Probably todo in the future: * better UI for change-tracking dict, should have "snapshot" support (to freeze / discard previous changes) * cleanup save, it's unclear that it properly resets the form closes odoo/odoo#43235 X-original-commit: 6c99fe3ffec3aeb8d210e3b943ccef81d62d44ce Signed-off-by: Xavier Morel (xmo) --- .../ir.model.access.csv | 2 + odoo/addons/test_testing_utilities/models.py | 20 +++ .../tests/test_form_impl.py | 38 ++++++ odoo/tests/common.py | 123 ++++++++++++------ 4 files changed, 142 insertions(+), 41 deletions(-) diff --git a/odoo/addons/test_testing_utilities/ir.model.access.csv b/odoo/addons/test_testing_utilities/ir.model.access.csv index 1cb9a9f542a..2e7786fdcc4 100644 --- a/odoo/addons/test_testing_utilities/ir.model.access.csv +++ b/odoo/addons/test_testing_utilities/ir.model.access.csv @@ -20,3 +20,5 @@ access_test_testing_utilities_onchange_count_sub,access_test_testing_utilities_o access_o2m_readonly_subfield_parent,access_o2m_readonly_subfield_parent,model_o2m_readonly_subfield_parent,,1,1,1,1 access_o2m_readonly_subfield_child,access_o2m_readonly_subfield_child,model_o2m_readonly_subfield_child,,1,1,1,1 access_test_testing_utilities_req_bool,access_test_testing_utilities_req_bool,model_test_testing_utilities_req_bool,,1,0,0,0 +access_o2m_changes_children,access_o2m_changes_children,model_o2m_changes_children,base.group_user,1,0,0,0 +access_o2m_changes_children_lines,access_o2m_changes_children_lines,model_o2m_changes_children_lines,base.group_user,1,0,0,0 diff --git a/odoo/addons/test_testing_utilities/models.py b/odoo/addons/test_testing_utilities/models.py index 769a6662317..cf2b4f0e8c6 100644 --- a/odoo/addons/test_testing_utilities/models.py +++ b/odoo/addons/test_testing_utilities/models.py @@ -279,3 +279,23 @@ class ReqBool(models.Model): _name = _description = 'test_testing_utilities.req_bool' f_bool = fields.Boolean(required=True) + +class O2MChangesChildren(models.Model): + _name = _description = 'o2m_changes_children' + + name = fields.Char() + v = fields.Integer() + line_ids = fields.One2many('o2m_changes_children.lines', 'parent_id') + + @api.onchange('v') + def _onchange_v(self): + for record in self: + for line in record.line_ids: + line.v = record.v + +class O2MChangesChildrenLines(models.Model): + _name = _description = 'o2m_changes_children.lines' + + parent_id = fields.Many2one('o2m_changes_children') + v = fields.Integer() + vv = fields.Integer() diff --git a/odoo/addons/test_testing_utilities/tests/test_form_impl.py b/odoo/addons/test_testing_utilities/tests/test_form_impl.py index 3c1d53bd6da..cc2620f9e46 100644 --- a/odoo/addons/test_testing_utilities/tests/test_form_impl.py +++ b/odoo/addons/test_testing_utilities/tests/test_form_impl.py @@ -534,6 +534,44 @@ class TestO2M(TransactionCase): a | b | c ) + def test_o2m_onchange_change_saved(self): + """ If an onchange updates o2m values (in existing sub-records of an + existing record), those updated values should be saved, both if the + sub-records were touched by the user and not (check that one maybe) + """ + # create record: line created before v is updated should reflect it, + # line created after should not + f = Form(self.env['o2m_changes_children']) + with f.line_ids.new() as line: + line.v = 1 + line.vv = 5 + f.v = 5 + with f.line_ids.new() as line: + ... + r = f.save() + self.assertEqual(r.v, 5) + self.assertEqual(r.mapped('line_ids.vv'), [5, 0]) + self.assertEqual(r.line_ids[0].v, 5, "onchange should have updated the existing lines") + self.assertEqual(r.line_ids[1].v, 0, "onchange should not impact new line") + + # update record: onchange then touch the lines + with Form(r) as f: + f.v = 6 + with f.line_ids.edit(0) as line: + line.vv = 1 + with f.line_ids.edit(1) as line: + line.vv = 2 + self.assertEqual(r.v, 6) + self.assertEqual(r.mapped('line_ids.vv'), [1, 2]) + self.assertEqual(r.mapped('line_ids.v'), [6, 6], "onchange should have updated vs") + + # update record: onchange then don't even touch the lines + with Form(r) as f: + f.v = 7 + self.assertEqual(r.v, 7) + self.assertEqual(r.mapped('line_ids.vv'), [1, 2]) + self.assertEqual(r.mapped('line_ids.v'), [7, 7]) + class TestEdition(TransactionCase): """ These use the context manager form as we don't need the record post-save (we already have it) and it's easier to see what bits act on diff --git a/odoo/tests/common.py b/odoo/tests/common.py index 462e6111485..cbde36ac6d6 100644 --- a/odoo/tests/common.py +++ b/odoo/tests/common.py @@ -1489,8 +1489,12 @@ class Form(object): fields = self._view['fields'] def cleanup(k, v): if fields[k]['type'] == 'one2many': - # o2m default gets a (6) at the start, makes no sense - return [c for c in v if c[0] != 6] + return [ + # use None as "empty" value for UPDATE instead of {} + (1, c[1], None) if c[0] == 1 and not c[2] else c + for c in v + if c[0] != 6 # o2m default gets a (6) at the start, nonsensical + ] elif fields[k]['type'] == 'datetime' and isinstance(v, datetime): return odoo.fields.Datetime.to_string(v) elif fields[k]['type'] == 'date' and isinstance(v, date): @@ -1614,7 +1618,8 @@ class Form(object): # * the environment's context (?) # * a few magic values record_id = self._values.get('id') or False - ctx = dict(self._values) + + ctx = dict(self._values_to_save(all_fields=True)) ctx.update(self._env.context) ctx.update( id=record_id, @@ -1670,36 +1675,37 @@ class Form(object): r.write(values) else: r = self._model.create(values) + self._values.update( + record_to_values(self._view['fields'], r) + ) + self._changed.clear() self._model.flush() self._model.invalidate_cache() - [data] = r.read(list(self._view['fields'])) - # FIXME: process relational fields - # alternative: iterate on record & read from it directly? pb: would - # provide recordsets for relational fields which may or may not be - # what we ultimately want, but we've got to re-process them anyway so...? - self._values.update(data) - self._changed.clear() return r - def _values_to_save(self): + def _values_to_save(self, all_fields=False): """ Validates values and returns only fields modified since load/save + + :param bool all_fields: if False (the default), checks for required + fields and only save fields which are changed + and not readonly """ values = {} fields = self._view['fields'] for f in fields: descr = fields[f] v = self._values[f] - if self._get_modifier(f, 'required') and not (descr['type'] == 'boolean' or self._get_modifier(f, 'invisible')): + # note: maybe `invisible` should not skip `required` if model attribute + if not all_fields and self._get_modifier(f, 'required') and not (descr['type'] == 'boolean' or self._get_modifier(f, 'invisible')): assert v is not False, "{} is a required field".format(f) - - # skip unmodified fields - if f not in self._changed: + # skip unmodified fields unless all_fields (also always ignore id) + if f == 'id' or not (all_fields or f in self._changed): continue if self._get_modifier(f, 'readonly'): node = _get_node(self._view, f) - if not node.get('force_save'): + if not (all_fields or node.get('force_save')): continue if descr['type'] == 'one2many': @@ -1716,10 +1722,14 @@ class Form(object): for (c, rid, vs) in oldvals: if c in (0, 1): - items = list(getattr(vs, 'changed_items', vs.items)()) + vs = vs or {} + if all_fields: + items = list(vs.items()) + else: + items = list(getattr(vs, 'changed_items', vs.items)()) fields_ = view['fields'] missing = fields_.keys() - vs.keys() - if missing: + if missing: # FIXME: maaaybe this should be done at the start? Model = self._env[descr['relation']] if c == 0: vs.update(dict.fromkeys(missing, False)) @@ -1732,11 +1742,12 @@ class Form(object): {k: v for k, v in fields_.items() if k not in vs}, Model.browse(rid) )) - vs.setdefault('id', False) - vs['•parent•'] = self._values + context = dict(vs) + context.setdefault('id', False) + context['•parent•'] = self._values vs = { k: v for k, v in items - if nodes[k].get('force_save') or not self._get_modifier(k, 'readonly', modmap=modifiers, vals=vs) + if all_fields or nodes[k].get('force_save') or not self._get_modifier(k, 'readonly', modmap=modifiers, vals=context) } v.append((c, rid, vs)) @@ -1759,7 +1770,7 @@ class Form(object): self._model.flush() self._model.invalidate_cache() if result.get('warning'): - _logger.getChild('onchange').warn("%(title)s %(message)s" % result.get('warning')) + _logger.getChild('onchange').warning("%(title)s %(message)s" % result.get('warning')) values = result.get('value', {}) # mark onchange output as changed self._changed.update(values.keys()) @@ -1801,27 +1812,48 @@ class Form(object): if not descr['views']: return [] + if current is None: + current = [] v = [] - c = {t[1] for t in current if t[0] in (1, 2)} if current else set() + c = {t[1] for t in current if t[0] in (1, 2)} + current_values = {c[1]: c[2] for c in current if c[0] == 1} # which view should this be??? subfields = descr['views']['edition']['fields'] # TODO: simplistic, unlikely to work if e.g. there's a 5 inbetween other commands for command in value: - if command[0] in (0, 1): - c.discard(command[1]) - v.append((command[0], command[1], { - k: self._cleanup_onchange( - subfields[k], v, None - ) + if command[0] == 0: + v.append((0, 0, { + k: self._cleanup_onchange(subfields[k], v, None) for k, v in command[2].items() if k in subfields })) + elif command[0] == 1: + record_id = command[1] + c.discard(record_id) + stored = current_values.get(record_id) + if stored is None: + record = self._env[descr['relation']].browse(record_id) + stored = UpdateDict(record_to_values(subfields, record)) + + updates = ( + (k, self._cleanup_onchange(subfields[k], v, None)) + for k, v in command[2].items() + if k in subfields + ) + for field, value in updates: + # if there are values from the onchange which differ + # from current values, update & mark field as changed + if stored.get(field, value) != value: + stored._changed.add(field) + stored[field] = value + + v.append((1, record_id, stored)) elif command[0] == 2: c.discard(command[1]) v.append((2, command[1], False)) elif command[0] == 4: c.discard(command[1]) - v.append((1, command[1], {})) + v.append((1, command[1], None)) elif command[0] == 5: v = [] # explicitly mark all non-relinked (or modified) records as deleted @@ -1870,7 +1902,10 @@ class O2MForm(Form): if index is None: self._init_from_defaults(m) else: - self._values.update(proxy._records[index]) + vals = proxy._records[index] + self._values.update(vals) + if hasattr(vals, '_changed'): + self._changed.update(vals._changed) def _get_modifier(self, field, modifier, default=False, modmap=None, vals=None): if vals is None: @@ -1882,8 +1917,8 @@ class O2MForm(Form): values = super(O2MForm, self)._onchange_values() # computed o2m may not have a relation_field(?) descr = self._proxy._descr - if 'relation_field' in descr: - values[descr['relation_field']] = self._proxy._parent._values + if 'relation_field' in descr: # note: should be fine because not recursive + values[descr['relation_field']] = self._proxy._parent._onchange_values() return values def save(self): @@ -1898,7 +1933,9 @@ class O2MForm(Form): if c == 0: vs.update(values) elif c == 1: - vs = UpdateDict(vs) + if vs is None: + vs = UpdateDict() + assert isinstance(vs, UpdateDict), type(vs) vs.update(values) commands[index] = (1, id_, vs) else: @@ -1907,12 +1944,14 @@ class O2MForm(Form): # FIXME: should be called when performing on change => value needs to be serialised into parent every time? proxy._parent._perform_onchange([proxy._field]) - def _values_to_save(self): + def _values_to_save(self, all_fields=False): """ Validates values and returns only fields modified since load/save """ values = UpdateDict(self._values) values._changed.update(self._changed) + if all_fields: + return values for f in self._view['fields']: if self._get_modifier(f, 'required') and not (self._get_modifier(f, 'column_invisible') or self._get_modifier(f, 'invisible')): @@ -1924,6 +1963,8 @@ class UpdateDict(dict): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self._changed = set() + if args and isinstance(args[0], UpdateDict): + self._changed.update(args[0]._changed) def changed_items(self): return ( @@ -1957,11 +1998,11 @@ class O2MProxy(X2MProxy): if command == 0: self._records.append(values) elif command == 1: - # read based on view info - r = model.browse(rid) - record = record_to_values(fields, r) - record.update(values) - self._records.append(record) + if values is None: + # read based on view info + r = model.browse(rid) + values = UpdateDict(record_to_values(fields, r)) + self._records.append(values) elif command == 2: pass else: @@ -2129,7 +2170,7 @@ def record_to_values(fields, record): assert v._name == descr['relation'] v = [(6, 0, v.ids)] elif descr['type'] == 'one2many': - v = [(1, r.id, {}) for r in v] + v = [(1, r.id, None) for r in v] elif descr['type'] == 'datetime' and isinstance(v, datetime): v = odoo.fields.Datetime.to_string(v) elif descr['type'] == 'date' and isinstance(v, date):