[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) <xmo@odoo.com>

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

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()

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

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):