From faa0d9988ebd0551a162812475852a0c0d2c0093 Mon Sep 17 00:00:00 2001 From: Dmitry S Date: Mon, 23 Aug 2021 23:28:07 -0400 Subject: [PATCH] (core) When changing a table for a page widget, unset widget-linking to avoid invalid values. Summary: Previously, using "Change Widget" allowed one to change the underlying table, but would keep the linking settings. This could allow invalid settings which would sometimes lead to JS errors. These manifested in production as "UserError: Query error: n is not a function". - Unset linking settings in this case, to avoid invalid values. - In case invalid values are encountered (e.g. saved previously), treat them as unset, to avoid JS errors. - If an error does occur, report it with a stack trace. Also, for testing, added 'selectBy' option to gristUtils helpers for using page-widget-picker. Test Plan: Added test cases for resetting linking, and for ignoring invalid link settings. Reviewers: alexmojaki Reviewed By: alexmojaki Differential Revision: https://phab.getgrist.com/D2993 --- app/client/components/BaseView.js | 19 +++++++---- app/client/components/LinkingState.js | 4 +-- app/client/ui/PageWidgetPicker.ts | 4 +++ app/client/ui/selectBy.ts | 46 +++++++++++++++++++++++++++ test/nbrowser/gristUtils.ts | 27 ++++++++++------ 5 files changed, 82 insertions(+), 18 deletions(-) diff --git a/app/client/components/BaseView.js b/app/client/components/BaseView.js index 2e888d49..6a328317 100644 --- a/app/client/components/BaseView.js +++ b/app/client/components/BaseView.js @@ -1,5 +1,3 @@ -/* global window */ - var _ = require('underscore'); var ko = require('knockout'); var moment = require('moment-timezone'); @@ -26,6 +24,7 @@ const {copyToClipboard} = require('app/client/lib/copyToClipboard'); const {setTestState} = require('app/client/lib/testState'); const {ExtraRows} = require('app/client/models/DataTableModelWithDiff'); const {createFilterMenu} = require('app/client/ui/ColumnFilterMenu'); +const {LinkConfig} = require('app/client/ui/selectBy'); const {encodeObject} = require("app/plugin/objtypes"); /** @@ -133,10 +132,16 @@ function BaseView(gristDoc, viewSectionModel, options) { let v = this.viewSection; let src = v.linkSrcSection(); const filterByAllShown = v.optionsObj.prop('filterByAllShown'); - return src.getRowId() ? - LinkingState.create.bind(LinkingState, this.gristDoc, - src, v.linkSrcCol().colId(), v, v.linkTargetCol().colId(), filterByAllShown()) : - null; + if (!src.getRowId()) { + return null; + } + try { + const config = new LinkConfig(v); + return LinkingState.create.bind(LinkingState, this.gristDoc, config, filterByAllShown()); + } catch (err) { + console.warn(`Can't create LinkingState: ${err.message}`); + return null; + } })); this._linkingFilter = this.autoDispose(ko.computed(() => { @@ -213,7 +218,7 @@ function BaseView(gristDoc, viewSectionModel, options) { const linkingFilter = this._linkingFilter(); this._queryRowSource.makeQuery(linkingFilter.filters, linkingFilter.operations, (err) => { if (this.isDisposed()) { return; } - if (err) { window.gristNotify(`Query error: ${err.message}`); } + if (err) { reportError(err); } this.onTableLoaded(); }); })); diff --git a/app/client/components/LinkingState.js b/app/client/components/LinkingState.js index ddce580c..01c3bda8 100644 --- a/app/client/components/LinkingState.js +++ b/app/client/components/LinkingState.js @@ -46,7 +46,8 @@ function isSummaryOf(summary, detail) { * value at the cursor. The user can use column filters on srcSection to control what's shown * in the linked tgtSection. */ -function LinkingState(gristDoc, srcSection, srcColId, tgtSection, tgtColId, byAllShown) { +function LinkingState(gristDoc, linkConfig, byAllShown) { + const {srcSection, srcColId, tgtSection, tgtCol, tgtColId} = linkConfig; this._srcSection = srcSection; let srcTableModel = gristDoc.getTableModel(srcSection.table().tableId()); @@ -66,7 +67,6 @@ function LinkingState(gristDoc, srcSection, srcColId, tgtSection, tgtColId, byAl // A computed that evaluates to a filter function to use, or null if not filtering. If // filtering, depends on srcSection.activeRowId(). if (tgtColId) { - const tgtCol = tgtSection.table().columns().all().find(c => c.colId() === tgtColId); const operations = {[tgtColId]: isRefListType(tgtCol.type()) ? 'intersects' : 'in'}; if (byAllShown) { // (This is legacy code that isn't currently reachable) diff --git a/app/client/ui/PageWidgetPicker.ts b/app/client/ui/PageWidgetPicker.ts index decc814c..586f7a7f 100644 --- a/app/client/ui/PageWidgetPicker.ts +++ b/app/client/ui/PageWidgetPicker.ts @@ -371,6 +371,9 @@ export class PageWidgetSelect extends Disposable { } private _selectTable(tid: TableId) { + if (tid !== this._value.table.get()) { + this._value.link.set(NoLink); + } this._value.table.set(tid); this._closeSummarizePanel(); } @@ -386,6 +389,7 @@ export class PageWidgetSelect extends Disposable { if (tid !== this._value.table.get()) { this._value.columns.set([]); this._value.table.set(tid); + this._value.link.set(NoLink); } this._openSummarizePanel(); } diff --git a/app/client/ui/selectBy.ts b/app/client/ui/selectBy.ts index bcbaa082..bef7c9ff 100644 --- a/app/client/ui/selectBy.ts +++ b/app/client/ui/selectBy.ts @@ -2,6 +2,7 @@ import { ColumnRec, DocModel, TableRec, ViewSectionRec } from 'app/client/models import { IPageWidget } from 'app/client/ui/PageWidgetPicker'; import { getReferencedTableId } from 'app/common/gristTypes'; import { IOptionFull } from 'grainjs'; +import * as assert from 'assert'; // some unicode characters const BLACK_CIRCLE = '\u2022'; @@ -213,3 +214,48 @@ export function linkFromId(linkid: string) { const [srcSectionRef, srcColRef, targetColRef] = JSON.parse(linkid); return {srcSectionRef, srcColRef, targetColRef}; } + +export class LinkConfig { + public readonly srcSection: ViewSectionRec; + public readonly tgtSection: ViewSectionRec; + // Note that srcCol and tgtCol may be the empty column records if that column is not used. + public readonly srcCol: ColumnRec; + public readonly tgtCol: ColumnRec; + public readonly srcColId: string|undefined; + public readonly tgtColId: string|undefined; + + // The constructor throws an exception if settings are invalid. When used from inside a knockout + // computed, the constructor subscribes to all parts relevant for linking. + constructor(tgtSection: ViewSectionRec) { + this.tgtCol = tgtSection.linkTargetCol(); + this.srcCol = tgtSection.linkSrcCol(); + this.srcSection = tgtSection.linkSrcSection(); + this.tgtSection = tgtSection; + this.srcColId = this.srcCol.colId(); + this.tgtColId = this.tgtCol.colId(); + this._assertValid(); + } + + // Check if section-linking configuration is valid, and throw exception if not. + private _assertValid(): void { + // 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()) : + this.srcSection.table.peek().primaryTableId.peek()); + const tgtTableId = (tgtCol ? getReferencedTableId(tgtCol.type.peek()) : + this.tgtSection.table.peek().primaryTableId.peek()); + try { + assert(!tgtCol || tgtCol.parentId.peek() === this.tgtSection.tableRef.peek(), "tgtCol belongs to wrong table"); + assert(!srcCol || srcCol.parentId.peek() === this.srcSection.tableRef.peek(), "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"); + assert(srcTableId === tgtTableId, "mismatched tableIds"); + } catch (e) { + throw new Error(`LinkConfig invalid: ` + + `${this.srcSection.getRowId()}:${this.srcCol?.getRowId()}[${srcTableId}] -> ` + + `${this.tgtSection.getRowId()}:${this.tgtCol?.getRowId()}[${tgtTableId}]: ${e}`); + } + } +} diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index 5f69e612..4b9aa8c4 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -764,8 +764,13 @@ export async function addNewTable() { await waitForServer(); } +export interface PageWidgetPickerOptions { + summarize?: RegExp[]; // Optional list of patterns to match Group By columns. + selectBy?: RegExp; // 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. -export async function addNewPage(typeRe: RegExp, tableRe: RegExp, summarize?: RegExp[]) { +export async function addNewPage(typeRe: RegExp, tableRe: RegExp, options?: PageWidgetPickerOptions) { const url = await driver.getCurrentUrl(); // Click the 'Page' entry in the 'Add New' menu @@ -773,27 +778,25 @@ export async function addNewPage(typeRe: RegExp, tableRe: RegExp, summarize?: Re await driver.find('.test-dp-add-new-page').doClick(); // add widget - await selectWidget(typeRe, tableRe, summarize); + await selectWidget(typeRe, tableRe, options); // wait new page to be selected await driver.wait(async () => (await driver.getCurrentUrl()) !== url, 2000); } // Add a new widget to the current page using the 'Add New' menu. -export async function addNewSection(typeRe: RegExp, tableRe: RegExp, summarize?: RegExp[]) { - +export async function addNewSection(typeRe: RegExp, tableRe: RegExp, options?: PageWidgetPickerOptions) { // Click the 'Add widget to page' entry in the 'Add New' menu await driver.findWait('.test-dp-add-new', 2000).doClick(); await driver.findWait('.test-dp-add-widget-to-page', 500).doClick(); // add widget - await selectWidget(typeRe, tableRe, summarize); + await selectWidget(typeRe, tableRe, options); } - // Select type and table that matches respectivelly typeRe and tableRe and save. The widget picker // must be already opened when calling this function. -export async function selectWidget(typeRe: RegExp, tableRe: RegExp, summarize?: RegExp[]) { +export async function selectWidget(typeRe: RegExp, tableRe: RegExp, options: PageWidgetPickerOptions = {}) { const tableEl = driver.findContent('.test-wselect-table', tableRe); @@ -806,16 +809,22 @@ export async function selectWidget(typeRe: RegExp, tableRe: RegExp, summarize?: await tableEl.click(); - if (summarize) { + if (options.summarize) { // if summarize is requested, let's select the corresponding pivot icon await tableEl.find('.test-wselect-pivot').click(); // and all the columns - for (const colRef of summarize) { + for (const colRef of options.summarize) { await driver.findContent('.test-wselect-column', colRef).click(); } } + if (options.selectBy) { + // select link + await driver.find('.test-wselect-selectby').doClick(); + await driver.findContent('.test-wselect-selectby option', options.selectBy).doClick(); + } + // let's select right type and save await driver.findContent('.test-wselect-type', typeRe).doClick(); await driver.find('.test-wselect-addBtn').doClick();