From 25e40bfa9bd133590cdc1eb83b62ba3356625004 Mon Sep 17 00:00:00 2001
From: Cyprien P <cyprien.pindat@gmail.com>
Date: Fri, 8 Apr 2022 15:47:08 +0200
Subject: [PATCH] (core) Fix setting xaxis when both chart aggregation and
 split series

Summary:
 - Symptoms where that Split Series could end up being turned off for
   no good reason. Also both x axis and split series could be mixed
   up.

 - Problems was caused by call to `setGroupByColumns` which modifies
   the sections viewFields. Diff fixes it by adjustin slightly the
   ordering of function call in `_setXAxis()`.

 - Problem of mixing up x axis and split series was fixed by being
   careful on the order of columns passed to the `setGroupByColumns`
   which then determine the ordering of the view fields.

Test Plan: Includes new test cases

Reviewers: jarek

Reviewed By: jarek

Differential Revision: https://phab.getgrist.com/D3365
---
 app/client/components/ChartView.ts | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/app/client/components/ChartView.ts b/app/client/components/ChartView.ts
index 70d1da70..352df9a2 100644
--- a/app/client/components/ChartView.ts
+++ b/app/client/components/ChartView.ts
@@ -692,15 +692,6 @@ export class ChartConfig extends GrainJSDisposable {
       this._freezeXAxis.set(true);
       try {
 
-        // if values aggregation is 'on' update the grouped by columns first. This will make sure
-        // that colId is not missing from the summary table's columns (as could happen if it were a
-        // non-numeric for instance).
-        if (this._optionsObj.prop('aggregate')()) {
-          const splitColId = this._groupDataColId.get();
-          const cols = splitColId === colId ? [colId] : [colId, splitColId];
-          await this._setGroupByColumns(cols);
-        }
-
         // first remove the current field
         if (this._xAxisFieldIndex.get() !== -1 && this._xAxisFieldIndex.get() < viewFields.peek().length) {
           await this._configFieldsHelper.removeField(viewFields.peek()[this._xAxisFieldIndex.get()]);
@@ -709,13 +700,22 @@ export class ChartConfig extends GrainJSDisposable {
         // if x axis was undefined, set option to false
         await setSaveValue(this._optionsObj.prop('isXAxisUndefined'), false);
 
-        // if  new field was used to group by column series, disable multiseries
+        // if new field was used to split series, disable multiseries
         const fieldIndex = viewFields.peek().findIndex((f) => f.column.peek().colId() === colId);
         if (fieldIndex === 0 && optionsObj.prop('multiseries').peek()) {
           await optionsObj.prop('multiseries').setAndSave(false);
           return;
         }
 
+        // if values aggregation is 'on' update the grouped by columns before findColumn()
+        // call. This will make sure that colId is not missing from the summary table's columns (as
+        // could happen if it were a non-numeric for instance).
+        if (this._optionsObj.prop('aggregate')()) {
+          const splitColId = this._groupDataColId.get();
+          const cols = splitColId === colId ? [colId] : [splitColId, colId];
+          await this._setGroupByColumns(cols);
+        }
+
         // if the new column for the x axis is already visible, make it the first visible column,
         // else add it as the first visible field. The field will be first because it will be
         // inserted before current xAxis column (which is already first (or second if we have
@@ -754,7 +754,7 @@ export class ChartConfig extends GrainJSDisposable {
         // non-numeric for instance).
         if (this._optionsObj.prop('aggregate')()) {
           const xAxisColId = this._xAxis.get();
-          const cols = xAxisColId === colId ? [colId] : [xAxisColId, colId];
+          const cols = xAxisColId === colId ? [colId] : [colId, xAxisColId];
           await this._setGroupByColumns(cols);
         }