(core) Fix linking after a summary update

Summary:
When linking table to a summary, the linking ended up broken after updating the summary group by columns.
This diff fixes that issue.

There were two issues:
 1) some subscriptions were missing due to some .peek() calls instead of directly calling the observable.
 2) the LinkingState instance was not being disposed.
 3) the filterColValues was not updating after source data table has been loaded

Test Plan: Include new test file.

Reviewers: alexmojaki

Reviewed By: alexmojaki

Subscribers: paulfitz

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

app/client/components/LinkingState.ts

@@ -103,7 +103,16 @@ export class LinkingState extends Disposable {
       // TODO: This approach doesn't help cursor-linking (the other direction). If we have the
       // inverse of summary-table's 'group' column, we could implement both, and more efficiently.
       const isDirectSummary = srcSection.table().summarySourceTable() === tgtSection.table().getRowId();
-      this.filterColValues = this.autoDispose(ko.computed(() => {
+      const _filterColValues = ko.observable<FilterColValues>();
+      this.filterColValues = this.autoDispose(ko.computed(() => _filterColValues()));
+
+      // source data table could still be loading (this could happen after changing the group by
+      // columns of a linked summary table for instance), hence the below listeners.
+      this.autoDispose(srcTableData.dataLoadedEmitter.addListener(_update));
+      this.autoDispose(srcTableData.tableActionEmitter.addListener(_update));
+
+      _update();
+      function _update() {
         const result: FilterColValues = {filters: {}, operations: {}};
         const srcRowId = srcSection.activeRowId();
         for (const c of srcSection.table().groupByColumns()) {
@@ -119,8 +128,8 @@ export class LinkingState extends Disposable {
             }
           }
         }
-        return result;
+        _filterColValues(result);
-      }));
+      }
     } else if (isSummaryOf(tgtSection.table(), srcSection.table())) {
       // TODO: We should move the cursor, but don't currently it for summaries. For that, we need a
       // column or map representing the inverse of summary table's "group" column.

app/client/models/entities/ViewSectionRec.ts

@@ -22,7 +22,7 @@ import {arrayRepeat} from 'app/common/gutil';
 import {Sort} from 'app/common/SortSpec';
 import {ColumnsToMap, WidgetColumnMap} from 'app/plugin/CustomSectionAPI';
 import {ColumnToMapImpl} from 'app/client/models/ColumnToMap';
-import {Computed, Observable} from 'grainjs';
+import {Computed, Holder, Observable} from 'grainjs';
 import * as ko from 'knockout';
 import defaults = require('lodash/defaults');
 
@@ -118,6 +118,7 @@ export interface ViewSectionRec extends IRowModel<"_grist_Views_section"> {
   // Linking state maintains .filterFunc and .cursorPos observables which we use for
   // auto-scrolling and filtering.
   linkingState: ko.Computed<LinkingState | null>;
+  _linkingState: Holder<LinkingState>; // Holder for the current value of linkingState
 
   linkingFilter: ko.Computed<FilterColValues>;
 
@@ -473,15 +474,14 @@ export function createViewSectionRec(this: ViewSectionRec, docModel: DocModel):
 
   this.activeRowId = ko.observable(null);
 
+  this._linkingState = Holder.create(this);
   this.linkingState = this.autoDispose(ko.pureComputed(() => {
-    if (!this.linkSrcSection().getRowId()) {
-      return null;
-    }
     try {
       const config = new LinkConfig(this);
-      return new LinkingState(docModel, config);
+      return LinkingState.create(this._linkingState, docModel, config);
     } catch (err) {
-      console.warn(`Can't create LinkingState: ${err.message}`);
+      // Dispose old LinkingState in case creating the new one failed.
+      this._linkingState.dispose();
       return null;
     }
   }));

app/client/ui/selectBy.ts

@@ -257,13 +257,14 @@ export class LinkConfig {
     // Use null for unset cols (rather than an empty ColumnRec) for easier comparisons below.
     const srcCol = this.srcCol?.getRowId() ? this.srcCol : null;
     const tgtCol = this.tgtCol?.getRowId() ? this.tgtCol : null;
-    const srcTableId = (srcCol ? getReferencedTableId(srcCol.type.peek()) :
+    const srcTableId = (srcCol ? getReferencedTableId(srcCol.type()) :
-      this.srcSection.table.peek().primaryTableId.peek());
+      this.srcSection.table().primaryTableId());
-    const tgtTableId = (tgtCol ? getReferencedTableId(tgtCol.type.peek()) :
+    const tgtTableId = (tgtCol ? getReferencedTableId(tgtCol.type()) :
-      this.tgtSection.table.peek().primaryTableId.peek());
+      this.tgtSection.table().primaryTableId());
     try {
-      assert(!tgtCol || tgtCol.parentId.peek() === this.tgtSection.tableRef.peek(), "tgtCol belongs to wrong table");
+      assert(Boolean(this.srcSection.getRowId()), "srcSection was disposed");
-      assert(!srcCol || srcCol.parentId.peek() === this.srcSection.tableRef.peek(), "srcCol belongs to wrong table");
+      assert(!tgtCol || tgtCol.parentId() === this.tgtSection.tableRef(), "tgtCol belongs to wrong table");
+      assert(!srcCol || srcCol.parentId() === this.srcSection.tableRef(), "srcCol belongs to wrong table");
       assert(this.srcSection.getRowId() !== this.tgtSection.getRowId(), "srcSection links to itself");
       assert(tgtTableId, "tgtCol not a valid reference");
       assert(srcTableId, "srcCol not a valid reference");

test/nbrowser/gristUtils.ts

@@ -331,8 +331,9 @@ export function getActiveCell(): WebElementPromise {
  * Get the numeric value from the row header of the first selected row. This would correspond to
  * the row with the cursor when a single rows is selected.
  */
-export async function getSelectedRowNum(): Promise<number> {
+export async function getSelectedRowNum(section?: string): Promise<number> {
-  const rowNum = await driver.find('.active_section .gridview_data_row_num.selected').getText();
+  const sectionElem = section ? await getSection(section) : await driver.find('.active_section');
+  const rowNum = await sectionElem.find('.gridview_data_row_num.selected').getText();
   return parseInt(rowNum, 10);
 }
 
@@ -805,7 +806,7 @@ export async function addNewTable() {
 
 export interface PageWidgetPickerOptions {
   summarize?: RegExp[];   // Optional list of patterns to match Group By columns.
-  selectBy?: RegExp;      // Optional pattern of SELECT BY option to pick.
+  selectBy?: RegExp|string;      // Optional pattern of SELECT BY option to pick.
 }
 
 // Add a new page using the 'Add New' menu and wait for the new page to be shown.
@@ -847,14 +848,18 @@ export async function selectWidget(typeRe: RegExp, tableRe: RegExp, options: Pag
   // let's select table
   await tableEl.click();
 
+  const pivotEl = tableEl.find('.test-wselect-pivot');
+  if (await pivotEl.isPresent()) {
+    await toggleSelectable(pivotEl, Boolean(options.summarize));
+  }
 
   if (options.summarize) {
-    // if summarize is requested, let's select the corresponding pivot icon
+    for (const columnEl of await driver.findAll('.test-wselect-column')) {
-    await tableEl.find('.test-wselect-pivot').click();
+      const label = await columnEl.getText();
-
+      // TODO: Matching cols with regexp calls for trouble and adds no value. I think function should be
-    // and all the columns
+      // rewritten using string matching only.
-    for (const colRef of options.summarize) {
+      const goal = Boolean(options.summarize.find(r => label.match(r)));
-      await driver.findContent('.test-wselect-column', colRef).click();
+      await toggleSelectable(columnEl, goal);
     }
   }
 
@@ -870,6 +875,17 @@ export async function selectWidget(typeRe: RegExp, tableRe: RegExp, options: Pag
   await waitForServer();
 }
 
+/**
+ * Toggle elem if not selected. Expects elem to be clickable and to have a class ending with
+ * -selected when selected.
+ */
+async function toggleSelectable(elem: WebElement, goal: boolean) {
+  const isSelected = await elem.matches('[class*=-selected]');
+  if (goal !== isSelected) {
+    await elem.click();
+  }
+}
+
 /**
  * Rename the given page to a new name. The oldName can be a full string name or a RegExp.
  */