(core) Restoring separated transform columns

Summary:
Fix for a bug that prevented two users to change column types at
the same time.

Test Plan: Added and updated

Reviewers: georgegevoian

Reviewed By: georgegevoian

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

app/client/components/ColumnTransform.ts

@@ -141,11 +141,11 @@ export class ColumnTransform extends Disposable {
     // started doing something else, and we should finalize the transform.
     return actions.every(action => (
       // ['AddColumn', USER_TABLE, 'gristHelper_Transform', colInfo]
-      (action[2] === 'gristHelper_Transform') ||
+      (action[2]?.toString().startsWith('gristHelper_Transform')) ||
       // ['AddColumn', USER_TABLE, 'gristHelper_Converted', colInfo]
-      (action[2] === 'gristHelper_Converted') ||
+      (action[2]?.toString().startsWith('gristHelper_Converted')) ||
       // ['ConvertFromColumn', USER_TABLE, SOURCE_COLUMN, 'gristHelper_Converted']
-      (action[3] === 'gristHelper_Converted') ||
+      (action[3]?.toString().startsWith('gristHelper_Converted')) ||
       // ["SetDisplayFormula", USER_TABLE, ...]
       (action[0] === 'SetDisplayFormula') ||
       // ['UpdateRecord', '_grist_Table_column', transformColId, ...]

app/client/components/TypeConversion.ts

@@ -84,7 +84,7 @@ function getRefTableIdFromData(docModel: DocModel, column: ColumnRec): string|nu
 // will be set to the expression to compute the new values from the old ones.
 // @param toTypeMaybeFull: Type to convert the column to, either full ('Ref:Foo') or pure ('Ref').
 export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRec, origDisplayCol: ColumnRec,
-                                           toTypeMaybeFull: string): Promise<ColInfo> {
+                                           toTypeMaybeFull: string, convertedRef: string): Promise<ColInfo> {
   const toType = gristTypes.extractTypeFromColType(toTypeMaybeFull);
   const tableData: TableData = docModel.docData.getTable(origCol.table().tableId())!;
 
@@ -117,7 +117,7 @@ export async function prepTransformColInfo(docModel: DocModel, origCol: ColumnRe
     type: addColTypeSuffix(toTypeMaybeFull, origCol, docModel),
     isFormula: true,
     visibleCol: 0,
-    formula: "CURRENT_CONVERSION(rec)",
+    formula: `rec.${convertedRef}`,
     rules: origCol.rules(),
   };
 

app/client/components/TypeTransform.ts

@@ -14,6 +14,7 @@ import {cssButtonRow} from 'app/client/ui/RightPanelStyles';
 import {basicButton, primaryButton} from 'app/client/ui2018/buttons';
 import {testId} from 'app/client/ui2018/cssVars';
 import {FieldBuilder} from 'app/client/widgets/FieldBuilder';
+import {ColumnRec} from 'app/client/models/DocModel';
 import {NewAbstractWidget} from 'app/client/widgets/NewAbstractWidget';
 import {UserAction} from 'app/common/DocActions';
 import {Computed, dom, fromKo, Observable} from 'grainjs';
@@ -30,6 +31,7 @@ const t = makeT('components.TypeTransformation');
 export class TypeTransform extends ColumnTransform {
   private _reviseTypeChange = Observable.create(this, false);
   private _transformWidget: Computed<NewAbstractWidget|null>;
+  private _convertColumn: ColumnRec;                 // Set in prepare()
 
   constructor(gristDoc: GristDoc, fieldBuilder: FieldBuilder) {
     super(gristDoc, fieldBuilder);
@@ -96,29 +98,37 @@ export class TypeTransform extends ColumnTransform {
    */
   protected async addTransformColumn(toType: string) {
     const docModel = this.gristDoc.docModel;
-    const colInfo = await TypeConversion.prepTransformColInfo(docModel, this.origColumn, this.origDisplayCol, toType);
+    const newColInfos = await this._tableData.sendTableActions([
+      ['AddColumn', 'gristHelper_Converted', {type: 'Any'}],
+      ['AddColumn', 'gristHelper_Transform', {type: 'Any'}],
+    ]);
+    const gristHelper_ConvertedRef = newColInfos[0].colRef;
+    const gristHelper_TransformRef = newColInfos[1].colRef;
+    this.transformColumn = docModel.columns.getRowModel(gristHelper_TransformRef);
+    this._convertColumn = docModel.columns.getRowModel(gristHelper_ConvertedRef);
+    const colInfo = await TypeConversion.prepTransformColInfo(
+      docModel, this.origColumn,
+      this.origDisplayCol, toType, this._convertColumn.colId.peek());
     // NOTE: We could add rules with AddColumn action, but there are some optimizations that converts array values.
     const rules = colInfo.rules;
     delete (colInfo as any).rules;
-    const newColInfos = await this._tableData.sendTableActions([
-      ['AddColumn', 'gristHelper_Converted', {...colInfo, isFormula: false, formula: ''}],
-      ['AddColumn', 'gristHelper_Transform', colInfo],
+    await this._tableData.sendTableActions([
+      ['ModifyColumn', this._convertColumn.colId.peek(), {...colInfo, isFormula: false, formula: ''}],
+      ['ModifyColumn', this.transformColumn.colId.peek(), colInfo],
     ]);
-    const transformColRef = newColInfos[1].colRef;
     if (rules) {
       await this.gristDoc.docData.sendActions([
-        ['UpdateRecord', '_grist_Tables_column', transformColRef, { rules }]
+        ['UpdateRecord', '_grist_Tables_column', gristHelper_TransformRef, { rules }]
       ]);
     }
-    this.transformColumn = docModel.columns.getRowModel(transformColRef);
     await this.convertValues();
-    return transformColRef;
+    return gristHelper_TransformRef;
   }
 
   protected convertValuesActions(): UserAction[] {
     const tableId = this._tableData.tableId;
     const srcColId = this.origColumn.colId.peek();
-    const dstColId = "gristHelper_Converted";
+    const dstColId = this._convertColumn.colId.peek();
     const type = this.transformColumn.type.peek();
     const widgetOptions = this.transformColumn.widgetOptions.peek();
     const visibleColRef = this.transformColumn.visibleCol.peek();
@@ -153,7 +163,7 @@ export class TypeTransform extends ColumnTransform {
    * Overrides parent method to delete extra column
    */
   protected cleanup() {
-    void this._tableData.sendTableAction(['RemoveColumn', 'gristHelper_Converted']);
+    void this._tableData.sendTableAction(['RemoveColumn', this._convertColumn.colId.peek()]);
   }
 
   /**
@@ -161,7 +171,9 @@ export class TypeTransform extends ColumnTransform {
    */
   public async setType(toType: string) {
     const docModel = this.gristDoc.docModel;
-    const colInfo = await TypeConversion.prepTransformColInfo(docModel, this.origColumn, this.origDisplayCol, toType);
+    const colInfo = await TypeConversion.prepTransformColInfo(
+      docModel, this.origColumn, this.origDisplayCol,
+      toType, this._convertColumn.colId.peek());
     const tcol = this.transformColumn;
     await tcol.updateColValues(colInfo as any);
   }

sandbox/grist/functions/info.py

@@ -504,14 +504,6 @@ def N(value):
   return 0
 
 
-def CURRENT_CONVERSION(rec):
-  """
-  Internal function used by Grist during column type conversions. Not available for use in
-  formulas.
-  """
-  return rec.gristHelper_Converted
-
-
 def NA():
   """
   Returns the "value not available" error, `#N/A`.

sandbox/grist/test_trigger_formulas.py

@@ -357,7 +357,7 @@ class TestTriggerFormulas(test_engine.EngineTestCase):
     ])
 
     # ModifyColumn doesn't trigger recalcs.
-    out_actions = self.apply_user_action(["ModifyColumn", "Creatures", "Size", {type: 'Numeric'}])
+    out_actions = self.apply_user_action(["ModifyColumn", "Creatures", "Size", {"type": 'Numeric'}])
     self.assertEqual(out_actions.calls, {})
 
 

sandbox/grist/useractions.py

@@ -40,10 +40,8 @@ _action_types = {}
 # requested.
 DIRECT_ACTION = 0
 
-# Fields of _grist_Tables_column table that may be modified using ModifyColumns useraction.
-_modifiable_col_fields = {'type', 'widgetOptions', 'formula', 'isFormula', 'label',
-                          'untieColIdFromLabel'}
-
+# Fields of _grist_Tables_column table that can't be modified using ModifyColumn useraction.
+_unmodifiable_col_fields = {'colId', 'id', 'parentId'}
 # Fields of _grist_Tables_column table that are inherited by group-by columns from their source.
 _inherited_groupby_col_fields = {'colId', 'widgetOptions', 'label', 'untieColIdFromLabel'}
 
@@ -1302,11 +1300,6 @@ class UserActions(object):
         ))
     )
 
-    if transform:
-      # Delete any currently existing transform columns with the same id
-      if self._engine.tables[table_id].has_column(col_id):
-        self.RemoveColumn(table_id, col_id)
-
     ret = self.doAddColumn(table_id, col_id, col_info)
 
     if not transform and table_rec.rawViewSectionRef:
@@ -1469,7 +1462,7 @@ class UserActions(object):
     # metadata record. We implement the former interface by forwarding to the latter.
     col = self._docmodel.get_column_rec(table_id, col_id)
 
-    update_values = {k: v for k, v in six.iteritems(col_info) if k in _modifiable_col_fields}
+    update_values = {k: v for k, v in six.iteritems(col_info) if k not in _unmodifiable_col_fields}
     if '_position' in col_info:
       update_values['parentPos'] = col_info['_position']
     self._docmodel.update([col], **update_values)