(core) Disallow using non numeric type in chart's series

Summary:
We do not support to show non numeric column as chart series.
However we didn't prevent the user from doing it and it could cause unexpected behaviour such as a missing chart.
This diff addresses that issue by doing two following thing:
1) it prevents user from adding non numeric column as series and
2) it makes sure that if there is a non numeric series it does not mess up the chart (it still can happen that a non numeric series ends up in charts even with 1) for instance if users convert a series' column to a non numeric column for instance).

Links to UI discussion:
 - https://grist.quip.com/wb4gAgrQM2aP#TZEADAKPs8n
 - https://grist.quip.com/wb4gAgrQM2aP#TZEADAP8S8N

Test Plan:
 - new behaviour covered in nbrowser/ChartView3.ts
 - Some test were using non-numeric column as series, diff fixes that to.

Reviewers: jarek

Reviewed By: jarek

Differential Revision: https://phab.getgrist.com/D3206

app/client/components/ChartView.ts

@@ -7,7 +7,7 @@ import {Disposable} from 'app/client/lib/dispose';
 import {fromKoSave} from 'app/client/lib/fromKoSave';
 import {loadPlotly, PlotlyType} from 'app/client/lib/imports';
 import * as DataTableModel from 'app/client/models/DataTableModel';
-import {ViewFieldRec, ViewSectionRec} from 'app/client/models/DocModel';
+import {ColumnRec, ViewFieldRec, ViewSectionRec} from 'app/client/models/DocModel';
 import {reportError} from 'app/client/models/errors';
 import {KoSaveableObservable, ObjObservable} from 'app/client/models/modelUtil';
 import {SortedRowSet} from 'app/client/models/rowset';
@@ -17,13 +17,13 @@ import {squareCheckbox} from 'app/client/ui2018/checkbox';
 import {colors, vars} from 'app/client/ui2018/cssVars';
 import {cssDragger} from 'app/client/ui2018/draggableList';
 import {icon} from 'app/client/ui2018/icons';
-import {linkSelect, menu, menuItem, select} from 'app/client/ui2018/menus';
+import {linkSelect, menu, menuItem, menuText, select} from 'app/client/ui2018/menus';
 import {nativeCompare} from 'app/common/gutil';
 import {BaseFormatter} from 'app/common/ValueFormatter';
 import {decodeObject} from 'app/plugin/objtypes';
 import {Events as BackboneEvents} from 'backbone';
-import {Computed, dom, DomContents, DomElementArg, fromKo, Disposable as GrainJSDisposable, IOption,
+import {Computed, dom, DomContents, DomElementArg, fromKo, Disposable as GrainJSDisposable,
-  makeTestId, Observable, styled} from 'grainjs';
+        IDisposable, IOption, ISubscribable, makeTestId, Observable, styled, UseCB} from 'grainjs';
 import * as ko from 'knockout';
 import clamp = require('lodash/clamp');
 import debounce = require('lodash/debounce');
@@ -47,6 +47,23 @@ function isPieLike(chartType: string) {
   return ['pie', 'donut'].includes(chartType);
 }
 
+export function isNumericOnly(chartType: string) {
+  return ['bar', 'pie', 'donut', 'kaplan_meier', 'line', 'area', 'scatter'].includes(chartType);
+}
+
+// Returns the type of the visibleCol if col is of type `Ref`, otherwise returns the type of col.
+function visibleColType(col: ColumnRec, use: UseCB = unwrap) {
+  const colType = use(col.pureType);
+  const isRef = colType === 'Ref';
+  return isRef ? use(use(col.visibleColModel).type) : colType;
+}
+
+// Returns true if col is one of 'Numeric', 'Int', 'Any'.
+export function isNumericLike(col: ColumnRec, use: UseCB = unwrap) {
+  const colType = visibleColType(col, use);
+  return ['Numeric', 'Int', 'Any'].includes(colType);
+}
+
 
 interface ChartOptions {
   multiseries?: boolean;
@@ -179,9 +196,17 @@ export class ChartView extends Disposable {
 
     this._update = debounce(() => this._updateView(), 0);
 
+    let subs: IDisposable[] = [];
     this.autoDispose(this._chartType.subscribe(this._update));
     this.autoDispose(this._options.subscribe(this._update));
-    this.autoDispose(this.viewSection.viewFields().subscribe(this._update));
+    this.autoDispose(this.viewSection.viewFields().subscribe((viewFields: ViewFieldRec[]) => {
+      this._update();
+      subs.forEach((sub) => sub.dispose());
+      subs = [
+        ...viewFields.map((field) => field.displayColModel.peek().type.subscribe(this._update)),
+        ...viewFields.map((field) => field.visibleColModel.peek().type.subscribe(this._update)),
+      ];
+    }));
     this.listenTo(this.sortedRows, 'rowNotify', this._update);
     this.autoDispose(this.sortedRows.getKoArray().subscribe(this._update));
     this.autoDispose(this._formatterComp.subscribe(this._update));
@@ -217,19 +242,21 @@ export class ChartView extends Disposable {
 
     const fields: ViewFieldRec[] = this.viewSection.viewFields().all();
     const rowIds: number[] = this.sortedRows.getKoArray().peek() as number[];
-    let series: Series[] = fields.map((field) => {
-      // Use the colId of the displayCol, which may be different in case of Reference columns.
-      const colId: string = field.displayColModel.peek().colId.peek();
-      const getter = this.tableModel.tableData.getRowPropFunc(colId) as RowPropGetter;
-      const pureType = field.displayColModel().pureType();
-      const fullGetter = (pureType === 'Date' || pureType === 'DateTime') ? dateGetter(getter) : getter;
-      return {
-        label: field.label(),
-        values: rowIds.map(fullGetter),
-      };
-    });
-
     const startIndexForYAxis = this._options.prop('multiseries').peek() ? 2 : 1;
+    let series: Series[] = fields
+      .filter((field, i) => i < startIndexForYAxis || this._isCompatibleSeries(field.column.peek()))
+      .map((field) => {
+        // Use the colId of the displayCol, which may be different in case of Reference columns.
+        const colId: string = field.displayColModel.peek().colId.peek();
+        const getter = this.tableModel.tableData.getRowPropFunc(colId) as RowPropGetter;
+        const pureType = field.displayColModel().pureType();
+        const fullGetter = (pureType === 'Date' || pureType === 'DateTime') ? dateGetter(getter) : getter;
+        return {
+          label: field.label(),
+          values: rowIds.map(fullGetter),
+        };
+      });
+
     for (let i = 0; i < series.length; ++i) {
       if (i < fields.length && LIST_TYPES.includes(fields[i].column.peek().pureType.peek())) {
         if (i < startIndexForYAxis) {
@@ -303,6 +330,10 @@ export class ChartView extends Disposable {
     if (this.isDisposed() || !Plotly || !this._chartDom.parentNode) { return; }
     Plotly.Plots.resize(this._chartDom);
   }
+
+  private _isCompatibleSeries(col: ColumnRec) {
+    return isNumericOnly(this._chartType.peek()) ? isNumericLike(col) : true;
+  }
 }
 
 /**
@@ -583,9 +614,27 @@ export class ChartConfig extends GrainJSDisposable {
       cssRow(
         cssAddYAxis(
           cssAddIcon('Plus'), 'Add Series',
-          menu(() => this._section.hiddenColumns.peek().map((col) => (
+          menu(() => {
-            menuItem(() => this._configFieldsHelper.addField(col), col.label.peek())
+            const hiddenColumns = this._section.hiddenColumns.peek();
-          ))),
+            const filterFunc = this._isCompatibleSeries.bind(this);
+            const nonNumericCount = hiddenColumns.filter((col) => !filterFunc(col)).length;
+            return [
+              ...hiddenColumns
+                .filter((col) => filterFunc(col))
+                .map((col) => menuItem(
+                  () => this._configFieldsHelper.addField(col),
+                  col.label.peek(),
+                )),
+              nonNumericCount ? menuText(
+                `${nonNumericCount} ` + (
+                  nonNumericCount > 1 ?
+                    `non-numeric columns are not shown` :
+                    `non-numeric column is not shown`
+                ),
+                testId('yseries-picker-message'),
+              ) : null,
+            ];
+          }),
           testId('add-y-axis'),
         )
       ),
@@ -672,7 +721,7 @@ export class ChartConfig extends GrainJSDisposable {
     );
   }
 
-  private _buildYAxis(): Element {
+  private _buildYAxis(): DomContents {
 
     // The y-axis are all visible fields that comes after the x-axis and maybe the group data
     // column. Hence the draggable list of y-axis needs to skip either one or two visible fields.
@@ -680,14 +729,22 @@ export class ChartConfig extends GrainJSDisposable {
       multiseries ? 2 : 1
     ));
 
-    return this._configFieldsHelper.buildVisibleFieldsConfigHelper({
+    return dom.domComputed((use) => {
-      itemCreateFunc: (field) => this._buildField(field),
+      const filterFunc = (field: ViewFieldRec) => this._isCompatibleSeries(use(field.column), use);
-      draggableOptions: {
+      return this._configFieldsHelper.buildVisibleFieldsConfigHelper({
-        removeButton: false,
+        itemCreateFunc: (field) => this._buildField(field),
-        drag_indicator: cssDragger,
+        draggableOptions: {
-      }, skipFirst, freeze: this._freezeYAxis
+          removeButton: false,
+          drag_indicator: cssDragger,
+        }, skipFirst, freeze: this._freezeYAxis, filterFunc
+      });
     });
   }
+
+  private _isCompatibleSeries(col: ColumnRec, use: UseCB = unwrap) {
+    return isNumericOnly(use(this._chartType)) ? isNumericLike(col, use) : true;
+  }
+
 }
 
 // Row for a numeric option. User can change value using spinners or directly using keyboard. In
@@ -1076,3 +1133,11 @@ const cssSpinners = styled('input', `
     opacity: 1;
   }
 `);
+
+// Returns the value of both granjs and knockout observable without creating a dependency.
+const unwrap: UseCB = (obs: ISubscribable) => {
+  if ('_getDepItem' in obs) {
+    return obs.get();
+  }
+  return (obs as ko.Observable).peek();
+};

app/client/components/GristDoc.ts

@@ -6,6 +6,7 @@
 import {AccessRules} from 'app/client/aclui/AccessRules';
 import {ActionLog} from 'app/client/components/ActionLog';
 import * as BaseView from 'app/client/components/BaseView';
+import {isNumericLike, isNumericOnly} from 'app/client/components/ChartView';
 import * as CodeEditorPanel from 'app/client/components/CodeEditorPanel';
 import * as commands from 'app/client/components/commands';
 import {CursorPos} from 'app/client/components/Cursor';
@@ -48,7 +49,7 @@ import {MinimalActionGroup} from 'app/common/ActionGroup';
 import {ClientQuery} from "app/common/ActiveDocAPI";
 import {delay} from 'app/common/delay';
 import {DisposableWithEvents} from 'app/common/DisposableWithEvents';
-import {isSchemaAction} from 'app/common/DocActions';
+import {isSchemaAction, UserAction} from 'app/common/DocActions';
 import {OpenLocalDocResult} from 'app/common/DocListAPI';
 import {isList, isRefListType, RecalcWhen} from 'app/common/gristTypes';
 import {HashLink, IDocPage} from 'app/common/gristUrls';
@@ -492,6 +493,9 @@ export class GristDoc extends DisposableWithEvents {
     const result = await this.docData.sendAction(
       ['CreateViewSection', tableRef, viewRef, val.type, val.summarize ? val.columns : null]
     );
+    if (val.type === 'chart') {
+      await this._ensureOneNumericSeries(result.sectionRef);
+    }
     await this.docData.sendAction(
       ['UpdateRecord', '_grist_Views_section', result.sectionRef, {
         linkSrcSectionRef: link.srcSectionRef,
@@ -510,9 +514,15 @@ export class GristDoc extends DisposableWithEvents {
       const result = await this.docData.sendAction(['AddEmptyTable']);
       await this.openDocPage(result.views[0].id);
     } else {
-      const result = await this.docData.sendAction(
+      let result: any;
-        ['CreateViewSection', val.table, 0, val.type, val.summarize ? val.columns : null]
+      await this.docData.bundleActions(`Add new page`, async () => {
-      );
+        result = await this.docData.sendAction(
+          ['CreateViewSection', val.table, 0, val.type, val.summarize ? val.columns : null]
+        );
+        if (val.type === 'chart') {
+          await this._ensureOneNumericSeries(result.sectionRef);
+        }
+      });
       await this.openDocPage(result.viewRef);
       // The newly-added section should be given focus.
       this.viewModel.activeSectionId(result.sectionRef);
@@ -913,6 +923,36 @@ export class GristDoc extends DisposableWithEvents {
     // for real returning users.
     return this._showGristTour.get() ?? (!appModel.currentValidUser);
   }
+
+  /**
+   * Makes sure sure that the first y-series (ie: the view fields at index 1) is a numeric
+   * series. Does not handle chart with the group by option on: it is only intended to be used to
+   * make sure that newly created chart do have a visible y series.
+   */
+  private async _ensureOneNumericSeries(id: number) {
+    const viewSection = this.docModel.viewSections.getRowModel(id);
+    const field = viewSection.viewFields.peek().peek()[1];
+    if (isNumericOnly(viewSection.chartTypeDef.peek()) &&
+        !isNumericLike(field.column.peek())) {
+      const actions: UserAction[] = [];
+
+      // remove non-numeric field
+      actions.push(['RemoveRecord', field.id.peek()]);
+
+      // add new field
+      const newField = viewSection.hiddenColumns.peek().find((col) => isNumericLike(col));
+      if (newField) {
+        const colInfo = {
+          parentId: viewSection.id.peek(),
+          colRef: newField.id.peek(),
+        };
+        actions.push(['AddRecord', null, colInfo]);
+      }
+
+      // send actions
+      await this.docModel.viewFields.sendTableActions(actions);
+    }
+  }
 }
 
 async function finalizeAnchor() {

app/client/ui/VisibleFieldsConfig.ts

@@ -28,6 +28,9 @@ interface DraggableFieldsOption {
   // the group-by-column in the chart type widget.
   skipFirst?: Observable<number>;
 
+  // Allows to filter view fields.
+  filterFunc?: (field: ViewFieldRec, index: number) => boolean;
+
   // Allows to prevent updates of the list. This option is to be used when skipFirst option is used
   // and it is useful to prevent the list to update during changes that only affect the skipped
   // fields.
@@ -98,19 +101,26 @@ export class VisibleFieldsConfig extends Disposable {
     const itemClass = this._useNewUI ? cssDragRow.className : 'view_config_draggable_field';
     let fields = this._section.viewFields.peek();
 
-    if (options.skipFirst) {
+    if (options.skipFirst || options.filterFunc) {
+      const skipFirst = options.skipFirst || Observable.create(this, -1);
+      const filterFunc = options.filterFunc || (() => true);
+
       const freeze = options.freeze;
       const allFields = this._section.viewFields.peek();
       const newArray = new KoArray<ViewFieldRec>();
+
       function update() {
         if (freeze && freeze.get()) { return; }
-        const newValues = allFields.peek().filter((_v, i) => i + 1 > options.skipFirst!.get());
+        const newValues = allFields.peek()
+          .filter((_v, i) => i + 1 > skipFirst.get())
+          .filter(filterFunc)
+        ;
         if (isEqual(newArray.all(), newValues)) { return; }
         newArray.assign(newValues);
       }
       update();
       this.autoDispose(allFields.subscribe(update));
-      this.autoDispose(subscribe(options.skipFirst, update));
+      this.autoDispose(subscribe(skipFirst, update));
       if (options.freeze) {
         this.autoDispose(subscribe(options.freeze, update));
       }