From 4cfa03307853f63546b0922e0d7ccdd4260d9798 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Wed, 2 Aug 2023 12:37:00 +0200 Subject: [PATCH] (core) TypeTransform race condition fix Summary: TypeTransformation was flaky. Probably after upgrading AceEditor we introduced a race condition between updating the revised formula and doing the transformation. Now we explicitly make sure that the formula is updated. I also fixed some other flaky tests. Test Plan: Updated Reviewers: paulfitz Reviewed By: paulfitz Subscribers: paulfitz Differential Revision: https://phab.getgrist.com/D3984 --- app/client/components/ColumnTransform.ts | 28 +++++++++++++++++++++++ app/client/components/FormulaTransform.ts | 7 +----- app/client/components/TypeTransform.ts | 8 +------ test/nbrowser/GridOptions.ntest.js | 2 ++ test/nbrowser/gristUtils.ts | 13 +++++++++-- 5 files changed, 43 insertions(+), 15 deletions(-) diff --git a/app/client/components/ColumnTransform.ts b/app/client/components/ColumnTransform.ts index e695f45d..aadd9791 100644 --- a/app/client/components/ColumnTransform.ts +++ b/app/client/components/ColumnTransform.ts @@ -4,6 +4,7 @@ * and TypeTransform. */ import * as commands from 'app/client/components/commands'; +import * as AceEditor from 'app/client/components/AceEditor'; import {GristDoc} from 'app/client/components/GristDoc'; import {ColumnRec} from 'app/client/models/entities/ColumnRec'; import {ViewFieldRec} from 'app/client/models/entities/ViewFieldRec'; @@ -88,6 +89,13 @@ export class ColumnTransform extends Disposable { * @param {String} optInit - Optional initial value for the editor. */ protected buildEditorDom(optInit?: string) { + if (!this.editor) { + this.editor = this.autoDispose(AceEditor.create({ + gristDoc: this.gristDoc, + observable: this.transformColumn.formula, + saveValueOnBlurEvent: false, + })); + } return this.editor.buildDom((aceObj: any) => { this.editor.adjustContentToWidth(); this.editor.attachSaveCommand(); @@ -238,6 +246,7 @@ export class ColumnTransform extends Disposable { {...this.origWidgetOptions as object, ...this._fieldBuilder.options.peek()} : this._fieldBuilder.options.peek(); return [ + ...this.previewActions(), [ 'CopyFromColumn', this._tableData.tableId, @@ -268,4 +277,23 @@ export class ColumnTransform extends Disposable { protected isFinalizing(): boolean { return this._isFinalizing; } + + protected preview() { + if (!this.editor) { return; } + return this.editor.writeObservable(); + } + + /** + * Generates final actions before executing the transform. Used only when the editor was created. + */ + protected previewActions(): UserAction[] { + if (!this.editor) { return []; } + const formula = this.editor.getValue(); + const oldFormula = this.transformColumn.formula(); + if (formula === oldFormula) { return []; } + if (!formula && !oldFormula) { return []; } + return [ + ['UpdateRecord', '_grist_Tables_column', this.transformColumn.getRowId(), {formula}] + ]; + } } diff --git a/app/client/components/FormulaTransform.ts b/app/client/components/FormulaTransform.ts index e4a5e36a..1527b092 100644 --- a/app/client/components/FormulaTransform.ts +++ b/app/client/components/FormulaTransform.ts @@ -5,7 +5,6 @@ */ // Client libraries -import * as AceEditor from 'app/client/components/AceEditor'; import {ColumnTransform} from 'app/client/components/ColumnTransform'; import {GristDoc} from 'app/client/components/GristDoc'; import {cssButtonRow} from 'app/client/ui/RightPanelStyles'; @@ -26,10 +25,6 @@ export class FormulaTransform extends ColumnTransform { * Build the transform menu for a formula transform */ public buildDom() { - this.editor = this.autoDispose(AceEditor.create({ - gristDoc: this.gristDoc, - observable: this.transformColumn.formula, - })); return [ dom('div.transform_menu', dom('div.transform_editor', @@ -40,7 +35,7 @@ export class FormulaTransform extends ColumnTransform { cssButtonRow( basicButton(dom.on('click', () => this.cancel()), 'Cancel', testId("formula-transform-cancel")), - basicButton(dom.on('click', () => this.editor.writeObservable()), + basicButton(dom.on('click', () => this.preview()), 'Preview', dom.cls('disabled', this.formulaUpToDate), { title: 'Update formula (Shift+Enter)' }, diff --git a/app/client/components/TypeTransform.ts b/app/client/components/TypeTransform.ts index a5074733..3fd5e217 100644 --- a/app/client/components/TypeTransform.ts +++ b/app/client/components/TypeTransform.ts @@ -5,7 +5,6 @@ * to be pre-entered for certain transforms (to Reference / Date) which the user can modify via dropdown menus. */ -import * as AceEditor from 'app/client/components/AceEditor'; import {ColumnTransform} from 'app/client/components/ColumnTransform'; import {GristDoc} from 'app/client/components/GristDoc'; import * as TypeConversion from 'app/client/components/TypeConversion'; @@ -50,12 +49,7 @@ export class TypeTransform extends ColumnTransform { public buildDom() { // An observable to disable all buttons before the dom get removed. const disableButtons = Observable.create(null, false); - this._reviseTypeChange.set(false); - this.editor = this.autoDispose(AceEditor.create({ - gristDoc: this.gristDoc, - observable: this.transformColumn.formula, - })); return dom('div', testId('type-transform-top'), dom.maybe(this._transformWidget, transformWidget => transformWidget.buildTransformConfigDom()), @@ -71,7 +65,7 @@ export class TypeTransform extends ColumnTransform { ), dom.domComputed(this._reviseTypeChange, revising => { if (revising) { - return basicButton(dom.on('click', () => this.editor.writeObservable()), + return basicButton(dom.on('click', () => this.preview()), t('Preview'), testId("type-transform-update"), dom.cls('disabled', (use) => use(disableButtons) || use(this.formulaUpToDate)), { title: t('Update formula (Shift+Enter)') } diff --git a/test/nbrowser/GridOptions.ntest.js b/test/nbrowser/GridOptions.ntest.js index de0780a0..1e3045b7 100644 --- a/test/nbrowser/GridOptions.ntest.js +++ b/test/nbrowser/GridOptions.ntest.js @@ -42,6 +42,7 @@ describe("GridOptions.ntest", function() { await gu.supportOldTimeyTestCode(); await gu.useFixtureDoc(cleanup, "World-v10.grist", true); await $('.test-gristdoc').wait(); + await gu.hideBanners(); }); beforeEach(async function() { @@ -109,6 +110,7 @@ describe("GridOptions.ntest", function() { await driver.navigate().refresh(); //await $.injectIntoPage(); await gu.waitForDocToLoad(); + await gu.hideBanners(); await assertHVZ(0, true, true, true); // all on await assertHVZ(1, false, false, false); // all off await assertHVZ(2, false, true, true); // -h +v +z diff --git a/test/nbrowser/gristUtils.ts b/test/nbrowser/gristUtils.ts index f4b81e3e..cff87167 100644 --- a/test/nbrowser/gristUtils.ts +++ b/test/nbrowser/gristUtils.ts @@ -976,6 +976,14 @@ export async function confirm(save = true, remember = false) { } } +/** Hides all top banners by injecting css style */ +export async function hideBanners() { + const style = `.test-banner-element { display: none !important; }`; + await driver.executeScript(`const style = document.createElement('style'); + style.innerHTML = ${JSON.stringify(style)}; + document.head.appendChild(style);`); +} + /** * Returns the left-panel item for the given page, given by a full string name, or a RegExp. * You may simply click it to switch to that page. @@ -1582,10 +1590,11 @@ export async function sendKeys(...keys: string[]) { } /** - * Clears active input by sending HOME + SHIFT END + DELETE. + * Clears active input/textarea by sending CTRL HOME + CTRL + SHIFT END + DELETE. */ export async function clearInput() { - return sendKeys(Key.HOME, Key.chord(Key.SHIFT, Key.END), Key.DELETE); + const ctrl = await modKey(); + return sendKeys(Key.chord(ctrl, Key.HOME), Key.chord(ctrl, Key.SHIFT, Key.END), Key.DELETE); } /**