[FIX] web: Correctly aggregate values in exported parent groups

When exporting a grouped list view with some nested groups, the aggregate value
of parent groups are not correct. It always sums aggregated values of children
whether the group operator is 'sum' or not (could be 'max', 'avg', ...).

This behavior is wrong and can even lead to a crash if the aggregated field is a
date field (e.g. with group_operator='max'). (Try two sum two dates...)

The quick fix 85cf47f was merged just before OXP to avoid any crash. This fix
limited the support of aggregates to only int and float fields.
This commit remove this limitation.

This commit correclty implements the aggregation for parent group for all
field types and all group_operator.

This commit also improves the export feature tests.

X-original-commit: 5e7e4fa98698967e3c4fd0903f4aa8e91981a6cd

1 files changed, 91 insertions, 13 deletions

diff --git a/addons/web/controllers/main.py b/addons/web/controllers/main.py
index 165baa693b5..133421caa98 100644
--- a/addons/web/controllers/main.py
+++ b/addons/web/controllers/main.py
@@ -35,7 +35,7 @@ import odoo
 import odoo.modules.registry
 from odoo.api import call_kw, Environment
 from odoo.modules import get_module_path, get_resource_path
-from odoo.tools import image_process, topological_sort, html_escape, pycompat, ustr, apply_inheritance_specs
+from odoo.tools import image_process, topological_sort, html_escape, pycompat, ustr, apply_inheritance_specs, lazy_property
 from odoo.tools.mimetypes import guess_mimetype
 from odoo.tools.translate import _
 from odoo.tools.misc import str2bool, xlsxwriter, file_open
@@ -64,6 +64,37 @@ DBNAME_PATTERN = '^[a-zA-Z0-9][a-zA-Z0-9_.-]+$'
 
 COMMENT_PATTERN = r'Modified by [\s\w\-.]+ from [\s\w\-.]+'
 
+
+def none_values_filtered(func):
+    @functools.wraps(func)
+    def wrap(iterable):
+        return func(v for v in iterable if v is not None)
+    return wrap
+
+def allow_empty_iterable(func):
+    """
+    Some functions do not accept empty iterables (e.g. max, min with no default value)
+    This returns the function `func` such that it returns None if the iterable
+    is empty instead of raising a ValueError.
+    """
+    @functools.wraps(func)
+    def wrap(iterable):
+        iterator = iter(iterable)
+        try:
+            value = next(iterator)
+            return func(itertools.chain([value], iterator))
+        except StopIteration:
+            return None
+    return wrap
+
+OPERATOR_MAPPING = {
+    'max': none_values_filtered(allow_empty_iterable(max)),
+    'min': none_values_filtered(allow_empty_iterable(min)),
+    'sum': sum,
+    'bool_and': all,
+    'bool_or': any,
+}
+
 #----------------------------------------------------------
 # Odoo Web helpers
 #----------------------------------------------------------
@@ -591,12 +622,67 @@ class GroupsTreeNode:
         self._groupby_type = groupby_type
 
         self.count = 0  # Total number of records in the subtree
-        self.aggregated_values = Counter()  # Fields aggregated values {field_name: aggregated value}
         self.children = OrderedDict()
         self.data = []  # Only leaf nodes have data
+
         if root:
             self.insert_leaf(root)
 
+    def _get_aggregate(self, field_name, data, group_operator):
+        # When exporting one2many fields, multiple data lines might be exported for one record.
+        # Blank cells of additionnal lines are filled with an empty string. This could lead to '' being
+        # aggregated with an integer or float.
+        data = (value for value in data if value != '')
+
+        if group_operator == 'avg':
+            return self._get_avg_aggregate(field_name, data)
+
+        aggregate_func = OPERATOR_MAPPING.get(group_operator)
+        if not aggregate_func:
+            _logger.warning("Unsupported export of group_operator '%s' for field %s on model %s" % (group_operator, field_name, self._model._name))
+            return
+
+        if self.data:
+            return aggregate_func(data)
+        return aggregate_func((child.aggregated_values.get(field_name) for child in self.children.values()))
+
+    def _get_avg_aggregate(self, field_name, data):
+        aggregate_func = OPERATOR_MAPPING.get('sum')
+        if self.data:
+            return aggregate_func(data) / self.count
+        children_sums = (child.aggregated_values.get(field_name) * child.count for child in self.children.values())
+        return aggregate_func(children_sums) / self.count
+
+    def _get_aggregated_field_names(self):
+        """ Return field names of exported field having a group operator """
+        aggregated_field_names = []
+        for field_name in self._fields:
+            if '/' in field_name:
+                # Currently no support of aggregated value for nested record fields
+                # e.g. line_ids/analytic_line_ids/amount
+                continue
+            field = self._model._fields[field_name]
+            if field.group_operator:
+                aggregated_field_names.append(field_name)
+        return aggregated_field_names
+
+    # Lazy property to memoize aggregated values of children nodes to avoid useless recomputations
+    @lazy_property
+    def aggregated_values(self):
+
+        aggregated_values = {}
+
+        # Transpose the data matrix to group all values of each field in one iterable
+        field_values = zip(*self.data)
+        for field_name in self._fields:
+            field_data = self.data and next(field_values) or []
+
+            if field_name in self._get_aggregated_field_names():
+                field = self._model._fields[field_name]
+                aggregated_values[field_name] = self._get_aggregate(field_name, field_data, field.group_operator)
+
+        return aggregated_values
+
     def child(self, key):
         """
         Return the child identified by `key`.
@@ -614,17 +700,9 @@ class GroupsTreeNode:
         Build a leaf from `group` and insert it in the tree.
         :param group: dict as returned by `read_group(lazy=False)`
         """
-        # Pop every known key in the group dict (__domain, __count and grouped values)
-        # Remaining keys are aggregated fields.
-        leaf_path = [group.pop(groupby_field) for groupby_field in self._groupby]
+        leaf_path = [group.get(groupby_field) for groupby_field in self._groupby]
         domain = group.pop('__domain')
         count = group.pop('__count')
-        aggregated_values = group
-
-        keys = list(aggregated_values)
-        for key in keys:
-            if not isinstance(aggregated_values[key], (int, float)):
-                del aggregated_values[key]
 
         records = self._model.search(domain, offset=0, limit=False, order=False)
 
@@ -637,7 +715,6 @@ class GroupsTreeNode:
             node = node.child(node_key)
             # Update count value and aggregated value.
             node.count += count
-            node.aggregated_values += Counter(aggregated_values)
 
         node.data = records.export_data(self._fields).get('datas',[])
 
@@ -737,7 +814,8 @@ class GroupExportXlsxWriter(ExportXlsxWriter):
         self.write(row, column, label, self.header_bold_style)
         for field in self.fields[1:]: # No aggregates allowed in the first column because of the group title
             column += 1
-            self.write(row, column, aggregates.get(field['name'], ''), self.header_bold_style)
+            aggregated_value = aggregates.get(field['name'])
+            self.write(row, column, str(aggregated_value if aggregated_value is not None else ''), self.header_bold_style)
         return row + 1, 0