diff --git a/app/client/models/QuerySet.ts b/app/client/models/QuerySet.ts index 48a7ad09..844b8455 100644 --- a/app/client/models/QuerySet.ts +++ b/app/client/models/QuerySet.ts @@ -117,28 +117,16 @@ export class QuerySetManager extends Disposable { * RowSource. */ export class DynamicQuerySet extends RowSource { - /** - * Replace the query represented by this DynamicQuerySet. If multiple makeQuery() calls are made - * quickly (while waiting for the server), cb() may only be called for the latest one. - * - * If there is an error fetching data, cb(err) will be called with that error. The second - * argument to cb() is true if any data was changed, and false if not. Note that for a series of - * makeQuery() calls, cb() is always called at least once, and always asynchronously. - * - * It's possible for this to be called very quickly in succession, - * e.g. when selecting another row of a linked summary table grouped by multiple columns, - * as an observable gets triggered for each one. - * We only want to keep the last call, and _updateQuerySetDebounced may not be called - * in the correct order, so we first debounce here. - */ - public readonly makeQuery = debounce(tbind(this._makeQuery, this), 0); - // Holds a reference to the currently active QuerySet. private _holder = Holder.create>(this); // Shortcut to _holder.get().get(). private _querySet?: QuerySet; + // A ticket number for the latest makeQuery() call. We use it to avoid calling cb() for + // superseded queries. + private _lastTicket = 0; + // We could switch between several different queries quickly. If several queries are done // fetching at the same time (e.g. were already ready), debounce lets us only update the // query-set once to the last query. @@ -163,19 +151,26 @@ export class DynamicQuerySet extends RowSource { return this._querySet ? this._querySet.isTruncated : false; } - private _makeQuery(filters: { [colId: string]: any[] }, - operations: {[colId: string]: QueryOperation}, - cb: (err: Error|null, changed: boolean) => void): void { - if (this.isDisposed()) { - cb(new Error('Disposed'), false); - return; - } + /** + * Replace the query represented by this DynamicQuerySet. If multiple makeQuery() calls are made + * quickly (while waiting for the server), cb() may only be called for the latest one. + * + * If there is an error fetching data, cb(err) will be called with that error. The second + * argument to cb() is true if any data was changed, and false if not. Note that for a series of + * makeQuery() calls, cb() is always called at least once, and always asynchronously. + */ + public makeQuery(filters: {[colId: string]: any[]}, + operations: {[colId: string]: QueryOperation}, + cb: (err: Error|null, changed: boolean) => void): void { const query: ClientQuery = {tableId: this._tableModel.tableData.tableId, filters, operations}; const newQuerySet = this._querySetManager.useQuerySet(this._holder, query); + const ticket = this._getTicket(); // CB should be called asynchronously, since surprising hard-to-debug interactions can happen // if it's sometimes synchronous and sometimes not. newQuerySet.fetchPromise.then(() => { + // Only if we weren't superseded by another query. + if (!ticket.isValid()) { return; } this._updateQuerySetDebounced(newQuerySet, cb); }) .catch((err) => { cb(err, false); }); @@ -201,6 +196,13 @@ export class DynamicQuerySet extends RowSource { cb(err, true); } } + + private _getTicket() { + const myTicket = ++this._lastTicket; + return { + isValid: () => this._lastTicket === myTicket + }; + } } /** diff --git a/test/nbrowser/LinkingErrors.ts b/test/nbrowser/LinkingErrors.ts index ebbdee29..51676833 100644 --- a/test/nbrowser/LinkingErrors.ts +++ b/test/nbrowser/LinkingErrors.ts @@ -1,7 +1,7 @@ /** * Test various error situations with linking page widgets. */ -import {assert, driver, stackWrapFunc} from 'mocha-webdriver'; +import {assert, driver} from 'mocha-webdriver'; import * as gu from 'test/nbrowser/gristUtils'; import {setupTestSuite} from 'test/nbrowser/testUtils'; @@ -19,9 +19,79 @@ describe("LinkingErrors", function() { afterEach(() => gu.checkForErrors()); - it("should link correctly in the normal case", async function() { + before(async function() { session = await gu.session().teamSite.login(); api = session.createHomeApi(); + }); + + it("should maintain links after reload", async function() { + await session.tempNewDoc(cleanup); + + // Recreate the bug. + await gu.sendActions([ + ['AddEmptyTable', 'Table2'], // NOTICE: The order here matters, the bug was all about ordering. + ['AddEmptyTable', 'Table0'], + ['ModifyColumn', 'Table1', 'B', {type: 'Ref:Table0'}], + ['ModifyColumn', 'Table2', 'A', {type: 'Ref:Table1'}], + + ['AddRecord', 'Table0', null, {A: 'A'}], + ['AddRecord', 'Table0', null, {A: 'B'}], + ['AddRecord', 'Table1', null, {A: 'a', B: 1}], + ['AddRecord', 'Table1', null, {A: 'b', B: 1}], + ['AddRecord', 'Table1', null, {A: 'c', B: 1}], + ['AddRecord', 'Table1', null, {A: 'd', B: 1}], + ['AddRecord', 'Table2', null, {A: 2, B: 1}], + ['AddRecord', 'Table2', null, {A: 2, B: 2}], + ['AddRecord', 'Table2', null, {A: 4, B: 3}], + ['AddRecord', 'Table2', null, {A: 4, B: 4}], + ['AddRecord', 'Table2', null, {A: 1, B: 5}], + ['AddRecord', 'Table2', null, {A: 1, B: 6}], + ['AddRecord', 'Table2', null, {A: 3, B: 7}], + ['AddRecord', 'Table2', null, {A: 3, B: 8}], + ]); + + // Now add those two tables to a page, and link them. + // Pay attention to order. + await gu.addNewPage('Table', 'Table1'); + await gu.getCell('B', 1).click(); + await gu.openColumnPanel(); + await gu.setRefShowColumn('A'); + + await gu.addNewSection('Table', 'Table2', {selectBy: 'TABLE1'}); + await gu.getCell('A', 1).click(); + await gu.openColumnPanel(); + await gu.setRefShowColumn('A'); + + await gu.addNewSection('Table', 'Table0'); + await gu.selectSectionByTitle('TABLE1'); + await gu.openWidgetPanel('data'); + await gu.selectBy('TABLE0'); + + // Place cursor on the first row of Table0 + await gu.getCell({section: 'Table1', rowNum: 1, col: 'A'}).click(); + + const checkLink = async () => { + // This should show the linked rows in Table1 + assert.deepEqual(await gu.getVisibleGridCells({section: 'Table1', col: 'A', rowNums: [1, 2, 3, 4]}), + ['a', 'b', 'c', 'd']); + + // This should show the linked rows in Table2 + assert.deepEqual(await gu.getVisibleGridCells({section: 'Table2', col: 'B', rowNums: [1, 2]}), + ['5', '6']); + }; + + await checkLink(); + + // After reloading, we should see the same thing. + await gu.reloadDoc(); + await gu.waitToPass(async () => { + // Linking is done asynchronously, so make sure it is loaded first. + assert.equal(await gu.selectedBy(), 'TABLE0'); + await checkLink(); + }); + }); + + it("should link correctly in the normal case", async function() { docId = await session.tempNewDoc(cleanup, 'LinkingErrors1', {load: false}); // Make a table with some data. @@ -46,20 +116,9 @@ describe("LinkingErrors", function() { await checkLinking(); }); - // Check that normal linking works. - const checkLinking = stackWrapFunc(async function() { - await gu.getCell({section: 'PLANETS', rowNum: 1, col: 'PlanetName'}).click(); - assert.deepEqual(await gu.getVisibleGridCells({section: 'MOONS', col: 'MoonName', rowNums: [1, 2]}), - ['Moon', '']); - await gu.getCell({section: 'PLANETS', rowNum: 2, col: 'PlanetName'}).click(); - assert.deepEqual(await gu.getVisibleGridCells({section: 'MOONS', col: 'MoonName', rowNums: [1, 2, 3]}), - ['Phobos', 'Deimos', '']); - }); - it("should unset linking setting when changing the data table for a widget", async function() { await gu.getCell({section: 'Moons', rowNum: 1, col: 'MoonName'}).click(); - await gu.toggleSidePanel('right', 'open'); - await driver.find('.test-right-tab-pagewidget').click(); + await gu.openWidgetPanel(); await driver.findContent('.test-right-panel button', /Change Widget/).click(); assert.equal(await driver.find('.test-wselect-table-label[class*=-selected]').getText(), 'Moons'); @@ -173,6 +232,7 @@ describe("LinkingErrors", function() { await revert(); await gu.checkForErrors(); }); + }); async function getTableData(docApi: DocAPI, tableId: string): Promise { @@ -180,3 +240,13 @@ async function getTableData(docApi: DocAPI, tableId: string): Promise const tableAction = toTableDataAction(tableId, colValues); return new TableData(tableId, tableAction, (schema as any)[tableId]); } + +// Check that normal linking works. +async function checkLinking() { + await gu.getCell({section: 'PLANETS', rowNum: 1, col: 'PlanetName'}).click(); + assert.deepEqual(await gu.getVisibleGridCells({section: 'MOONS', col: 'MoonName', rowNums: [1, 2]}), + ['Moon', '']); + await gu.getCell({section: 'PLANETS', rowNum: 2, col: 'PlanetName'}).click(); + assert.deepEqual(await gu.getVisibleGridCells({section: 'MOONS', col: 'MoonName', rowNums: [1, 2, 3]}), + ['Phobos', 'Deimos', '']); +} diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index a8f75360..831749d9 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -2705,6 +2705,16 @@ export async function selectBy(table: string|RegExp) { await waitForServer(); } +/** + * Returns "Select by" of the current section. + */ +export async function selectedBy() { + await toggleSidePanel('right', 'open'); + await driver.find('.test-right-tab-pagewidget').click(); + await driver.find('.test-config-data').click(); + return await driver.find('.test-right-select-by').getText(); +} + // Add column to sort. export async function addColumnToSort(colName: RegExp|string) { await driver.find(".test-sort-config-add").click();