[FIX] SSF: handle o2m nesting

The SSF had pretty much punted on nested o2m as that's... not usually
a concern because few people are insane enough to put o2ms in their
o2ms in their views. And also because even if somebody did, sometimes
nothing would break because it turns out to be pretty difficult to
actually get into *using* those things.

MRP managed to do it though, and repro-ing required copying over parts
of mrp.production and then understanding why it didn't fail:

* obviously needs an o2m (f1) which contains an o2m (f2), in the
  view (an invisible tree inside a visible tree)
* needs an onchange which somehow updates the sub-o2m
* needs to actually trigger an onchange on the root form for f1,
  meaning f1 must be a dependency of a compute field or something (here
  I just marked every damn field as on_change as the optimisation of
  "don't call onchange when there's no need to" doesn't matter)

Also needs to be working on an existing record with existing
lines *and sublines* as the issue occurs with records to update.

The issue here is that `_onchange_values` would clean up f1 e.g. send
nothing for unmodified entries, and only send modified fields
otherwise, but it would only do so for the toplevel, meaning the
sub-level would not go through this step, and could send UPDATE
commands with an `id` field (set to the original value but
still). This would then proceed to blow up while loading the record,
as id fields are not writeable.

The fix is to perform `_onchange_values` recursively. Do that using a
separate helper in order to avoid blowing up on override and whatnot,
or faffling about with weird branching to get the "default" values in
case they're not provided, the root function can get all the relevant
bits and call the helper with them, then the helper does that setup
internally and calls itself directly.

An other issue I stumbled upon when investigating is a similar problem
on *save*, due to an implementation detail of the SSF: UPDATE commands
are fetched lazily.

`_values_to_save` took care of "hydrating" all update commands (and
validating and filtering them) of modified o2m fields, but as it would
not do so recursively a modified f2 would not get properly hydrated
and filtered, and could try to write `None` onto existing records.

closes odoo/odoo#51350

X-original-commit: 3dfb4cfad849936748cc6a5b8f712e100461a86f
Signed-off-by: Xavier Morel (xmo) <xmo@odoo.com>

1 files changed, 79 insertions, 36 deletions

diff --git a/odoo/tests/common.py b/odoo/tests/common.py
index 36cf46f5949..a0caa54f547 100644
--- a/odoo/tests/common.py
+++ b/odoo/tests/common.py
@@ -6,6 +6,7 @@ helpers and classes to write tests.
 """
 import base64
 import collections
+import functools
 import importlib
 import inspect
 import itertools
@@ -1843,45 +1844,70 @@ class Form(object):
                                 fields and only save fields which are changed
                                 and not readonly
         """
-        values = {}
+        view = self._view
         fields = self._view['fields']
+        record_values = self._values
+        changed = self._changed
+        return self._values_to_save_(
+            record_values, fields, view,
+            changed, all_fields
+        )
+
+    def _values_to_save_(
+            self, record_values, fields, view,
+            changed, all_fields=False, modifiers_values=None,
+            parent_link=None
+    ):
+        """ Validates & extracts values to save, recursively in order to handle
+         o2ms properly
+
+        :param dict record_values: values of the record to extract
+        :param dict fields: fields_get result
+        :param view: view tree
+        :param set changed: set of fields which have been modified (since last save)
+        :param bool all_fields:
+            whether to ignore normal filtering and just return everything
+        :param dict modifiers_values:
+            defaults to ``record_values``, but o2ms need some additional
+            massaging
+        """
+        values = {}
         for f in fields:
+            get_modifier = functools.partial(
+                self._get_modifier,
+                f, modmap=view['modifiers'],
+                vals=modifiers_values or record_values
+            )
             descr = fields[f]
-            v = self._values[f]
+            v = record_values[f]
             # 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)
+            if v is False and not (all_fields or f == parent_link or descr['type'] == 'boolean' or get_modifier('invisible') or get_modifier('column_invisible')):
+                if get_modifier('required'):
+                    raise AssertionError("{} is a required field ({})".format(f, view['modifiers'][f]))
+
             # skip unmodified fields unless all_fields (also always ignore id)
-            if f == 'id' or not (all_fields or f in self._changed):
+            if f == 'id' or not (all_fields or f in changed):
                 continue
 
-            if self._get_modifier(f, 'readonly'):
-                node = _get_node(self._view, f)
+            if get_modifier('readonly'):
+                node = _get_node(view, f)
                 if not (all_fields or node.get('force_save')):
                     continue
 
             if descr['type'] == 'one2many':
-                view = descr['views']['edition']
-                modifiers = view['modifiers']
+                subview = descr['views']['edition']
+                fields_ = subview['fields']
                 oldvals = v
                 v = []
-
-                nodes = {
-                    n.get('name'): n
-                    for n in view['tree'].iter('field')
-                }
-                nodes['id'] = etree.Element('field', attrib={'name': 'id'})
-
                 for (c, rid, vs) in oldvals:
-                    if c in (0, 1):
+                    if c == 1 and not vs:
+                        c, vs = 4, False
+                    elif c in (0, 1):
                         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: # FIXME: maaaybe this should be done at the start?
+                        # FIXME: maybe do this during initial loading instead?
+                        if missing:
                             Model = self._env[descr['relation']]
                             if c == 0:
                                 vs.update(dict.fromkeys(missing, False))
@@ -1894,13 +1920,14 @@ class Form(object):
                                     {k: v for k, v in fields_.items() if k not in vs},
                                     Model.browse(rid)
                                 ))
-                        context = dict(vs)
-                        context.setdefault('id', False)
-                        context['•parent•'] = self._values
-                        vs = {
-                            k: v for k, v in items
-                            if all_fields or nodes[k].get('force_save') or not self._get_modifier(k, 'readonly', modmap=modifiers, vals=context)
-                        }
+                        vs = self._values_to_save_(
+                            vs, fields_, subview,
+                            vs._changed if isinstance(vs, UpdateDict) else vs.keys(),
+                            all_fields,
+                            modifiers_values={'id': False, **vs, '•parent•': record_values},
+                            # related o2m don't have a relation_field
+                            parent_link=descr.get('relation_field'),
+                        )
                     v.append((c, rid, vs))
 
             values[f] = v
@@ -1936,17 +1963,33 @@ class Form(object):
         )
 
     def _onchange_values(self):
-        f = self._view['fields']
+        return self._onchange_values_(self._view['fields'], self._values)
+
+    def _onchange_values_(self, fields, record):
+        """ Recursively cleanup o2m values for onchanges:
+
+        * if an o2m command is a 1 (UPDATE) and there is nothing to update, send
+          a 4 instead (LINK_TO) instead as that's what the webclient sends for
+          unmodified rows
+        * if an o2m command is a 1 (UPDATE) and only a subset of its fields have
+          been modified, only send the modified ones
+
+        This needs to be recursive as there are people who put invisible o2ms
+        inside their o2ms.
+        """
         values = {}
-        for k, v in self._values.items():
-            if f[k]['type'] == 'one2many':
+        for k, v in record.items():
+            if fields[k]['type'] == 'one2many':
+                subfields = fields[k]['views']['edition']['fields']
                 it = values[k] = []
                 for (c, rid, vs) in v:
+                    if c == 1 and isinstance(vs, UpdateDict):
+                        vs = dict(vs.changed_items())
+
                     if c == 1 and not vs:
-                        # web client sends a 4 for unmodified o2m rows
                         it.append((4, rid, False))
-                    elif c == 1 and isinstance(vs, UpdateDict):
-                        it.append((1, rid, dict(vs.changed_items())))
+                    elif c in (0, 1):
+                        it.append((c, rid, self._onchange_values_(subfields, vs)))
                     else:
                         it.append((c, rid, vs))
             else: