(core) Fix 'select by' when adding summary table widget to page

Summary:
When adding a summary table widget to a page and using 'select by' in the add widget config (as opposed to in the right panel for an existing widget):

1. If an equivalent summary table already exists, use its referencing columns (if any) to construct link nodes. Previously the source table columns were being used instead, which could include referencing columns that don't have any equivalent in the summary table, and exclude referencing columns in the summary table.
2. If no such summary table exists yet, then keep using the source table columns, but only the selected groupby columns, and
3. After the summary table is created, correct the `linkTargetColRef` (which points to a source table column) to the corresponding column from the new summary table instead.

This fixes bugs which only appeared recently since 'select by' for a summary table previously involved no target columns.

Test Plan: Added two new tests to `nbrowser/SelectBySummaryRef`, and confirmed that they fail without the fixes to all three points above.

Reviewers: dsagal

Reviewed By: dsagal

Subscribers: dsagal

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

app/client/components/GristDoc.ts

@@ -42,16 +42,16 @@ import {startDocTour} from "app/client/ui/DocTour";
 import {showDocSettingsModal} from 'app/client/ui/DocumentSettings';
 import {isTourActive} from "app/client/ui/OnBoardingPopups";
 import {IPageWidget, toPageWidget} from 'app/client/ui/PageWidgetPicker';
-import {IPageWidgetLink, linkFromId, selectBy} from 'app/client/ui/selectBy';
+import {linkFromId, selectBy} from 'app/client/ui/selectBy';
 import {startWelcomeTour} from 'app/client/ui/welcomeTour';
 import {isNarrowScreen, mediaSmall, testId} from 'app/client/ui2018/cssVars';
-import {invokePrompt} from 'app/client/ui2018/modals';
 import {IconName} from 'app/client/ui2018/IconList';
+import {invokePrompt} from 'app/client/ui2018/modals';
 import {FieldEditor} from "app/client/widgets/FieldEditor";
 import {MinimalActionGroup} from 'app/common/ActionGroup';
 import {ClientQuery} from "app/common/ActiveDocAPI";
-import {delay} from 'app/common/delay';
 import {CommDocUsage, CommDocUserAction} from 'app/common/CommTypes';
+import {delay} from 'app/common/delay';
 import {DisposableWithEvents} from 'app/common/DisposableWithEvents';
 import {isSchemaAction, UserAction} from 'app/common/DocActions';
 import {OpenLocalDocResult} from 'app/common/DocListAPI';
@@ -62,8 +62,19 @@ import {LocalPlugin} from "app/common/plugin";
 import {StringUnion} from 'app/common/StringUnion';
 import {TableData} from 'app/common/TableData';
 import {DocStateComparison} from 'app/common/UserAPI';
-import {bundleChanges, Computed, dom, Emitter, Holder, IDisposable, IDomComponent, Observable,
-        styled, subscribe, toKo} from 'grainjs';
+import {
+  bundleChanges,
+  Computed,
+  dom,
+  Emitter,
+  Holder,
+  IDisposable,
+  IDomComponent,
+  Observable,
+  styled,
+  subscribe,
+  toKo
+} from 'grainjs';
 import * as ko from 'knockout';
 import cloneDeepWith = require('lodash/cloneDeepWith');
 import isEqual = require('lodash/isEqual');
@@ -549,7 +560,6 @@ export class GristDoc extends DisposableWithEvents {
    */
   public async addWidgetToPageImpl(val: IPageWidget, tableId: string|null = null) {
     const viewRef = this.activeViewId.get();
-    const link = linkFromId(val.link);
     const tableRef = val.table === 'New Table' ? 0 : val.table;
     const result = await this.docData.sendAction(
       ['CreateViewSection', tableRef, viewRef, val.type, val.summarize ? val.columns : null, tableId]
@@ -557,13 +567,7 @@ export class GristDoc extends DisposableWithEvents {
     if (val.type === 'chart') {
       await this._ensureOneNumericSeries(result.sectionRef);
     }
-    await this.docData.sendAction(
-      ['UpdateRecord', '_grist_Views_section', result.sectionRef, {
-        linkSrcSectionRef: link.srcSectionRef,
-        linkSrcColRef: link.srcColRef,
-        linkTargetColRef: link.targetColRef
-      }]
-    );
+    await this.saveLink(val.link, result.sectionRef);
     return result;
   }
 
@@ -647,7 +651,7 @@ export class GristDoc extends DisposableWithEvents {
 
         // update link
         if (oldVal.link !== newVal.link) {
-          await this.saveLink(linkFromId(newVal.link));
+          await this.saveLink(newVal.link);
         }
         return section;
       },
@@ -698,11 +702,23 @@ export class GristDoc extends DisposableWithEvents {
     }));
   }
 
-  // Save link for the active section.
-  public async saveLink(link: IPageWidgetLink) {
-    const viewModel = this.viewModel;
+  // Save link for a given section, by default the active section.
+  public async saveLink(linkId: string, sectionId?: number) {
+    sectionId = sectionId || this.viewModel.activeSection.peek().getRowId();
+    const link = linkFromId(linkId);
+    if (link.targetColRef) {
+      const targetTable = this.docModel.viewSections.getRowModel(sectionId).table();
+      const targetCol = this.docModel.columns.getRowModel(link.targetColRef);
+      if (targetTable.id() !== targetCol.table().id()) {
+        // targetColRef is actually not a column in the target table.
+        // This should mean that the target table is a summary table (which didn't exist when the
+        // option was selected) and targetColRef is from the source table.
+        // Change it to the corresponding summary table column instead.
+        link.targetColRef = targetTable.columns().all().find(c => c.summarySourceCol() === link.targetColRef)!.id();
+      }
+    }
     return this.docData.sendAction(
-      ['UpdateRecord', '_grist_Views_section', viewModel.activeSection.peek().getRowId(), {
+      ['UpdateRecord', '_grist_Views_section', sectionId, {
         linkSrcSectionRef: link.srcSectionRef,
         linkSrcColRef: link.srcColRef,
         linkTargetColRef: link.targetColRef

app/client/ui/RightPanel.ts

@@ -25,7 +25,7 @@ import {reportError} from 'app/client/models/AppModel';
 import {ColumnRec, ViewSectionRec} from 'app/client/models/DocModel';
 import {GridOptions} from 'app/client/ui/GridOptions';
 import {attachPageWidgetPicker, IPageWidget, toPageWidget} from 'app/client/ui/PageWidgetPicker';
-import {linkFromId, linkId, selectBy} from 'app/client/ui/selectBy';
+import {linkId, selectBy} from 'app/client/ui/selectBy';
 import {CustomSectionConfig} from 'app/client/ui/CustomSectionConfig';
 import {VisibleFieldsConfig} from 'app/client/ui/VisibleFieldsConfig';
 import {IWidgetType, widgetTypes} from 'app/client/ui/widgetTypes';
@@ -409,7 +409,7 @@ export class RightPanel extends Disposable {
       )
     );
 
-    link.onWrite((val) => this._gristDoc.saveLink(linkFromId(val)));
+    link.onWrite((val) => this._gristDoc.saveLink(val));
     return [
       this._disableIfReadonly(),
       cssLabel('DATA TABLE'),

app/client/ui/selectBy.ts

@@ -4,6 +4,7 @@ import { getReferencedTableId } from 'app/common/gristTypes';
 import { IOptionFull } from 'grainjs';
 import assert from 'assert';
 import * as gutil from "app/common/gutil";
+import isEqual = require('lodash/isEqual');
 
 // some unicode characters
 const BLACK_CIRCLE = '\u2022';
@@ -243,24 +244,44 @@ function fromPageWidget(docModel: DocModel, pageWidget: IPageWidget): LinkNode[]
 
   if (typeof pageWidget.table !== 'number') { return []; }
 
-  const table = docModel.tables.getRowModel(pageWidget.table);
+  let table = docModel.tables.getRowModel(pageWidget.table);
   const isSummary = pageWidget.summarize;
+  const groupbyColumns = isSummary ? new Set(pageWidget.columns) : undefined;
+  let tableExists = true;
+  if (isSummary) {
+    const summaryTable = docModel.tables.rowModels.find(
+      t => t?.summarySourceTable.peek() && isEqual(t.summarySourceColRefs.peek(), groupbyColumns));
+    if (summaryTable) {
+      // The selected source table and groupby columns correspond to this existing summary table.
+      table = summaryTable;
+    } else {
+      // This summary table doesn't exist yet. `fromColumns` will be using columns from the source table.
+      // Make sure it only uses columns that are in the selected groupby columns.
+      // The resulting targetColRef will incorrectly be from the source table,
+      // but will be corrected in GristDoc.saveLink after the summary table is created.
+      tableExists = false;
+    }
+  }
+
   const mainNode: LinkNode = {
     tableId: table.primaryTableId.peek(),
     isSummary,
-    groupbyColumns: isSummary ? new Set(pageWidget.columns) : undefined,
+    groupbyColumns,
     widgetType: pageWidget.type,
     ancestors: new Set(),
     section: docModel.viewSections.getRowModel(pageWidget.section),
   };
 
-  return fromColumns(table, mainNode);
+  return fromColumns(table, mainNode, tableExists);
 }
 
-function fromColumns(table: TableRec, mainNode: LinkNode): LinkNode[] {
+function fromColumns(table: TableRec, mainNode: LinkNode, tableExists: boolean = true): LinkNode[] {
   const nodes = [mainNode];
   const columns = table.columns.peek().peek();
   for (const column of columns) {
+    if (!tableExists && !mainNode.groupbyColumns!.has(column.getRowId())) {
+      continue;
+    }
     const tableId = getReferencedTableId(column.type.peek());
     if (tableId) {
       nodes.push({...mainNode, tableId, column});

test/nbrowser/gristUtils.ts

@@ -871,6 +871,7 @@ export interface PageWidgetPickerOptions {
   tableName?: string;
   selectBy?: RegExp|string;      // Optional pattern of SELECT BY option to pick.
   summarize?: (RegExp|string)[];   // Optional list of patterns to match Group By columns.
+  dontAdd?: boolean;  // If true, configure the widget selection without actually adding to the page
 }
 
 // Add a new page using the 'Add New' menu and wait for the new page to be shown.
@@ -939,8 +940,14 @@ export async function selectWidget(
     await driver.findContent('.test-wselect-selectby option', options.selectBy).doClick();
   }
 
-  // let's select right type and save
+  // select right type
   await driver.findContent('.test-wselect-type', typeRe).doClick();
+
+  if (options.dontAdd) {
+    return;
+  }
+
+  // add the widget
   await driver.find('.test-wselect-addBtn').doClick();
 
   // if we selected a new table, there will be a popup for a name