(core) Bundling save funciton in the field editor

Summary:
Some editors do some async work before saving the value (Ref column can add new
records). Those actions were send without bundling, so it wasn't possible to undo those
actions with togheter.

Test Plan: Added new test

Reviewers: georgegevoian

Reviewed By: georgegevoian

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

app/client/widgets/FieldEditor.ts

@@ -380,59 +380,60 @@ export class FieldEditor extends Disposable {
     if (!editor) { return false; }
     // Make sure the editor is save ready
     const saveIndex = this._cursor.rowIndex();
-    await editor.prepForSave();
+    return await this._gristDoc.docData.bundleActions(null, async () => {
-    if (this.isDisposed()) {
+      await editor.prepForSave();
-      // We shouldn't normally get disposed here, but if we do, avoid confusing JS errors.
+      if (this.isDisposed()) {
-      console.warn(t("Unable to finish saving edited cell"));  // tslint:disable-line:no-console
+        // We shouldn't normally get disposed here, but if we do, avoid confusing JS errors.
-      return false;
+        console.warn(t("Unable to finish saving edited cell"));  // tslint:disable-line:no-console
-    }
+        return false;
-
-    // Then save the value the appropriate way
-    // TODO: this isFormula value doesn't actually reflect if editing the formula, since
-    // editingFormula() is used for toggling column headers, and this is deferred to start of
-    // typing (a double-click or Enter) does not immediately set it. (This can cause a
-    // console.warn below, although harmless.)
-    const isFormula = this._field.editingFormula();
-    const col = this._field.column();
-    let waitPromise: Promise<unknown>|null = null;
-
-    if (isFormula) {
-      const formula = String(editor.getCellValue() ?? '');
-      // Bundle multiple changes so that we can undo them in one step.
-      if (isFormula !== col.isFormula.peek() || formula !== col.formula.peek()) {
-        waitPromise = this._gristDoc.docData.bundleActions(null, () => Promise.all([
-          col.updateColValues({isFormula, formula}),
-          // If we're saving a non-empty formula, then also add an empty record to the table
-          // so that the formula calculation is visible to the user.
-          (!this._detached.get() && this._editRow._isAddRow.peek() && formula !== "" ?
-            this._editRow.updateColValues({}) : undefined),
-        ]));
       }
-    } else {
+      // Then save the value the appropriate way
-      const value = editor.getCellValue();
+      // TODO: this isFormula value doesn't actually reflect if editing the formula, since
-      if (col.isRealFormula()) {
+      // editingFormula() is used for toggling column headers, and this is deferred to start of
-        // tslint:disable-next-line:no-console
+      // typing (a double-click or Enter) does not immediately set it. (This can cause a
-        console.warn(t("It should be impossible to save a plain data value into a formula column"));
+      // console.warn below, although harmless.)
+      const isFormula = this._field.editingFormula();
+      const col = this._field.column();
+      let waitPromise: Promise<unknown>|null = null;
+
+      if (isFormula) {
+        const formula = String(editor.getCellValue() ?? '');
+        // Bundle multiple changes so that we can undo them in one step.
+        if (isFormula !== col.isFormula.peek() || formula !== col.formula.peek()) {
+          waitPromise = Promise.all([
+            col.updateColValues({isFormula, formula}),
+            // If we're saving a non-empty formula, then also add an empty record to the table
+            // so that the formula calculation is visible to the user.
+            (!this._detached.get() && this._editRow._isAddRow.peek() && formula !== "" ?
+              this._editRow.updateColValues({}) : undefined),
+          ]);
+        }
       } else {
-        // This could still be an isFormula column if it's empty (isEmpty is true), but we don't
+        const value = editor.getCellValue();
-        // need to toggle isFormula in that case, since the data engine takes care of that.
+        if (col.isRealFormula()) {
-        waitPromise = setAndSave(this._editRow, this._field, value);
+          // tslint:disable-next-line:no-console
+          console.warn(t("It should be impossible to save a plain data value into a formula column"));
+        } else {
+          // This could still be an isFormula column if it's empty (isEmpty is true), but we don't
+          // need to toggle isFormula in that case, since the data engine takes care of that.
+          waitPromise = setAndSave(this._editRow, this._field, value);
+        }
       }
-    }
 
-    const event: FieldEditorStateEvent = {
+      const event: FieldEditorStateEvent = {
-      position : this.cellPosition(),
+        position : this.cellPosition(),
-      wasModified : this._editorHasChanged,
+        wasModified : this._editorHasChanged,
-      currentState : this._editorHolder.get()?.editorState?.get(),
+        currentState : this._editorHolder.get()?.editorState?.get(),
-      type : this._field.column.peek().pureType.peek()
+        type : this._field.column.peek().pureType.peek()
-    };
+      };
-    this.saveEmitter.emit(event);
+      this.saveEmitter.emit(event);
 
-    const cursor = this._cursor;
+      const cursor = this._cursor;
-    // Deactivate the editor. We are careful to avoid using `this` afterwards.
+      // Deactivate the editor. We are careful to avoid using `this` afterwards.
-    this.dispose();
+      this.dispose();
-    await waitPromise;
+      await waitPromise;
-    return isFormula || (saveIndex !== cursor.rowIndex());
+      return isFormula || (saveIndex !== cursor.rowIndex());
+    });
   }
 }
 

test/nbrowser/ReferenceList.ts

@@ -13,6 +13,41 @@ describe('ReferenceList', function() {
   });
 
   describe('other', function() {
+    it('fix: changing ref list with a new referenced row was not bundled', async function() {
+      // When user added a new referenced row through the RefList editor, the UI sent two separate
+      // actions. One for adding the new row, and another for updating the RefList column.
+      await session.tempNewDoc(cleanup);
+      await gu.sendActions([
+        ['ModifyColumn', 'Table1', 'B', {type: 'RefList:Table1'}],
+        ['AddRecord', 'Table1', null, {A: 'a'}],
+      ]);
+      await gu.openColumnPanel();
+      await gu.getCell('B', 1).doClick();
+      await gu.setRefShowColumn('A');
+
+      await gu.getCell('B', 1).click();
+      await gu.sendKeys(Key.ENTER, 'b');
+      await gu.waitToPass(async () => {
+        await driver.findWait('.test-ref-editor-new-item', 100).click();
+      });
+      await gu.sendKeys(Key.ENTER);
+      await gu.waitForServer();
+
+      // Check the data - use waitToPass helper, as previously it might have failed
+      // as 2 separate actions were sent.
+      await gu.waitToPass(async () => {
+        assert.deepEqual(await gu.getVisibleGridCells('A', [1, 2]), ['a', 'b']);
+        assert.deepEqual(await gu.getVisibleGridCells('B', [1, 2]), ['b', '']);
+        assert.equal(await gu.getGridRowCount(), 3);
+      });
+
+      // Now press undo once, and check that the new row is removed and the RefList is updated.
+      await gu.undo();
+      assert.deepEqual(await gu.getVisibleGridCells('A', [1]), ['a']);
+      assert.deepEqual(await gu.getVisibleGridCells('B', [1]), ['']);
+      assert.equal(await gu.getGridRowCount(), 2);
+    });
+
     it('fix: doesnt break when table is renamed', async function() {
       // There was a bug in this scenario:
       // 1. Create a Ref column that targets itself