(core) Fix selecting new row in chain of filter links

Summary:
When the 'new' row of a table is selected, another table filter linked to the first shows no data. This diff ensures that a third table filtered by the second also shows no data, i.e. that it behaves the same as if the second table was also on the 'new' row. Video of the bug: https://grist.slack.com/archives/C069RUP71/p1692622810900179

The functional code is copied almost verbatim from https://github.com/gristlabs/grist-core/pull/666 by @jvorob which was working correctly. A comment there mentioned a possible bug where:

> ...you can have the grayed-out "No row selected" text from disableEditing but still have rows showing up in the section. Haven't been able to reproduce...

I noticed this behaviour when I copied only part of the fix, but it disappeared after copying the whole thing, so it seems likely to me that this is why it couldn't be reproduced.

Test Plan: Added a new nbrowser test with a new fixture, which also tests filter link chains and selecting the new row more generally, since I couldn't find other tests of this.

Reviewers: georgegevoian

Reviewed By: georgegevoian

Subscribers: jvorob

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

app/client/components/LinkingState.ts

@@ -350,7 +350,11 @@ export class LinkingState extends Disposable {
    * Returns a boolean indicating whether editing should be disabled in the destination section.
    */
   public disableEditing(): boolean {
-    return Boolean(this.filterState) && this._srcSection.activeRowId() === 'new';
+    if (!this.filterState) {
+      return false;
+    }
+    const srcRowId = this._srcSection.activeRowId();
+    return srcRowId === 'new' || srcRowId === null;
   }
 
 
@@ -438,11 +442,6 @@ export class LinkingState extends Disposable {
       //Get selector-rowId
       const srcRowId = this._srcSection.activeRowId();
 
-      if (srcRowId === null) {
-        console.warn("LinkingState._makeFilterObs activeRowId is null");
-        return EmptyFilterState;
-      }
-
       //Get values from selector row
       const selectorCellVal = selectorValGetter(srcRowId);
       const displayCellVal  = displayValGetter(srcRowId);
@@ -473,30 +472,45 @@ export class LinkingState extends Disposable {
         }
       }
 
-      //Need to use 'intersects' for ChoiceLists or RefLists
+      // ==== Determine operation to use for filter ====
+      // Common case: use 'in' for single vals, or 'intersects' for ChoiceLists & RefLists
       let operation = (tgtColId && isListType(tgtCol!.type())) ? 'intersects' : 'in';
 
-      // If selectorVal is a blank-cell value, need to change operation for correct behavior with lists
+      // # Special case 1:
       // Blank selector shouldn't mean "show no records", it should mean "show records where tgt column is also blank"
-      if(srcRowId !== 'new') { //(EXCEPTION: the add-row, which is when we ACTUALLY want to show no records)
+      // This is the default behavior for single-ref -> single-ref links
+      // However, if tgtCol is a list and the selectorVal is blank/empty, the default behavior ([] intersects tgtlist)
+      //    doesn't work, we need to explicitly specify the operation to be 'empty', to select empty cells
+      if (tgtCol?.type() === "ChoiceList" && !isSrcRefList && selectorCellVal === "")    { operation = 'empty'; }
+      else if (isTgtRefList               && !isSrcRefList && selectorCellVal === 0)     { operation = 'empty'; }
+      else if (isTgtRefList               &&  isSrcRefList && filterValues.length === 0) { operation = 'empty'; }
+      // Note, we check each case separately since they have different "blank" values"
+      // Other types can have different falsey values when non-blank (e.g. a Ref=0 is a blank cell, but for numbers,
+      //      0 would be a valid value, and to check for an empty number-cell you'd check for null)
+      // However, we don't need to check for those here, since they can't be linked to list types
 
-        // If tgtCol is a list (RefList or Choicelist) and selectorVal is null/blank, operation must be 'empty'
-        if (tgtCol?.type() === "ChoiceList" && !isSrcRefList && selectorCellVal === "")    { operation = 'empty'; }
-        else if (isTgtRefList               && !isSrcRefList && selectorCellVal === 0)     { operation = 'empty'; }
-        else if (isTgtRefList               &&  isSrcRefList && filterValues.length === 0) { operation = 'empty'; }
-        // other types can have falsey values when non-blank (e.g. for numbers, 0 is a valid value; blank cell is null)
-        // However, we don't need to check for those here, since we only care about lists (Reflist or Choicelist)
+      // NOTES ON CHOICELISTS: they only show up in a few cases.
+      // - ChoiceList can only ever appear in links as the tgtcol
+      //   (ChoiceLists can only be linked from summ. tables, and summary flattens lists, so srcCol would be 'Choice')
+      // - empty Choice is [""].
 
-        // If tgtCol is a single ref, nullness is represented by [0], not by [], so need to create that null explicitly
-        else if (!isTgtRefList && isSrcRefList && filterValues.length === 0) {
-          filterValues = [0];
-          displayValues = [''];
-        }
+      // # Special case 2:
+      //  If tgtCol is a single ref, blankness is represented by [0]
+      //  However if srcCol is a RefList, blankness is represented by [], which won't match the [0].
+      //  We create the 0 explicitly so the filter will select the blank Refs
+      else if (!isTgtRefList && isSrcRefList && filterValues.length === 0) {
+        filterValues = [0];
+        displayValues = [''];
+      }
 
-        // NOTES ON CHOICELISTS: they only show up in a few cases.
-        // - ChoiceList can only ever appear in links as the tgtcol
-        //   (ChoiceLists can only be linked from summ. tables, and summary flattens lists, so srcCol would be 'Choice')
-        // - empty choicelist is [""].
+      // # Special case 3:
+      // If the srcSection has no row selected (cursor on the add-row, or no data in srcSection), we should
+      //    show no rows in tgtSection. (we also gray it out and show the "No row selected in $SRCSEC" msg)
+      // This should line up with when this.disableEditing() returns true
+      if (srcRowId === 'new' || srcRowId === null) {
+        operation = 'in';
+        filterValues = [];
+        displayValues = [];
       }
 
       // Run values through formatters (for dates, numerics, Refs with visCol = rowId)

test/nbrowser/FilterLinkChain.ts

@@ -0,0 +1,130 @@
+import {assert} from 'mocha-webdriver';
+import * as gu from 'test/nbrowser/gristUtils';
+import {setupTestSuite} from 'test/nbrowser/testUtils';
+
+describe('FilterLinkChain', function () {
+  this.timeout(10000);
+  const cleanup = setupTestSuite();
+
+  before(async function () {
+    const session = await gu.session().teamSite.login();
+    await session.tempDoc(cleanup, 'FilterLinkChain.grist');
+  });
+
+  it('should work with chains of filter links', async function () {
+    async function checkCells(sectionName: string, cols: string[], expected: string[]) {
+      assert.deepEqual(
+        await gu.getVisibleGridCells({section: sectionName, cols, rowNums: [1, 2]}),
+        expected
+      );
+
+      // Sanity-check the selectors in checkSectionEmpty() below.
+      const section = gu.getSection(sectionName);
+      assert.isEmpty(await section.findAll('.disable_viewpane'));
+      assert.isNotEmpty(await section.findAll('.gridview_row .gridview_data_row_num'));
+      assert.isNotEmpty(await section.findAll('.gridview_row .record'));
+      assert.isNotEmpty(await section.findAll('.gridview_row .field'));
+    }
+
+    async function checkTopCells(expected: string[]) {
+      await checkCells('TOP', ['Top'], expected);
+    }
+
+    async function checkMiddleCells(expected: string[]) {
+      await checkCells('MIDDLE', ['Top', 'Middle'], expected);
+    }
+
+    async function checkBottomCells(expected: string[]) {
+      await checkCells('BOTTOM', ['Top', 'Middle', 'Bottom'], expected);
+    }
+
+    async function checkSectionEmpty(sectionName: string, text: string) {
+      const section = gu.getSection(sectionName);
+      assert.equal(await section.find('.disable_viewpane').getText(), text);
+      assert.isEmpty(await section.findAll('.gridview_row .gridview_data_row_num'));
+      assert.isEmpty(await section.findAll('.gridview_row .record'));
+      assert.isEmpty(await section.findAll('.gridview_row .field'));
+    }
+
+    async function checkBottomEmpty() {
+      await checkSectionEmpty('BOTTOM', 'No row selected in MIDDLE');
+    }
+
+    async function checkMiddleEmpty() {
+      await checkSectionEmpty('MIDDLE', 'No row selected in TOP');
+    }
+
+    // The initially visible data.
+    // The bottom section is selected by the middle section,
+    // which is selected by the top section.
+    await checkTopCells([
+      'A',  // selected initially
+      'B',
+    ]);
+    // Filtered to 'A'
+    await checkMiddleCells([
+      'A', 'A1',  // selected initially
+      'A', 'A2',
+    ]);
+    // Filtered to 'A1'
+    await checkBottomCells([
+      'A', 'A1', '1',
+      'A', 'A1', '2',
+    ]);
+
+    // Select 'A2'
+    await gu.getCell({section: 'MIDDLE', col: 'Middle', rowNum: 2}).click();
+    await checkBottomCells([
+      'A', 'A2', '3',
+      'A', 'A2', '4',
+    ]);
+
+    // Select the 'new' row
+    await gu.getCell({section: 'MIDDLE', col: 'Middle', rowNum: 3}).click();
+    await checkSectionEmpty('BOTTOM', 'No row selected in MIDDLE');
+
+    // Select 'B'
+    await gu.getCell({section: 'TOP', col: 'Top', rowNum: 2}).click();
+    await checkMiddleCells([
+      'B', 'B1',  // selected initially
+      'B', 'B2',
+    ]);
+    // Filtered to 'B1'
+    await checkBottomCells([
+      'B', 'B1', '5',
+      'B', 'B1', '6',
+    ]);
+
+    // Select 'B2'
+    await gu.getCell({section: 'MIDDLE', col: 'Middle', rowNum: 2}).click();
+    await checkBottomCells([
+      'B', 'B2', '7',
+      'B', 'B2', '8',
+    ]);
+
+    // Select the 'new' row, making the bottom empty
+    await gu.getCell({section: 'MIDDLE', col: 'Middle', rowNum: 3}).click();
+    await checkBottomEmpty();
+
+    // Select the 'new' in the top section, which makes middle empty, which means bottom stays empty.
+    await gu.getCell({section: 'TOP', col: 'Top', rowNum: 3}).click();
+    await checkMiddleEmpty();
+    await checkBottomEmpty();
+
+    // Double-check: make all sections show some data again,
+    // and then make both the middle and bottom empty in one click instead of one at a time.
+    await gu.getCell({section: 'TOP', col: 'Top', rowNum: 2}).click();
+    await checkMiddleCells([
+      'B', 'B1',  // selected initially
+      'B', 'B2',
+    ]);
+    await checkBottomCells([
+      'B', 'B1', '5',
+      'B', 'B1', '6',
+    ]);
+
+    await gu.getCell({section: 'TOP', col: 'Top', rowNum: 3}).click();
+    await checkMiddleEmpty();
+    await checkBottomEmpty();
+  });
+});