From e79613b0edce867c6ba2c27730d5a5f2b7f0e241 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Tue, 27 Dec 2022 16:30:36 +0100 Subject: [PATCH] (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 | 6 ++--- app/client/components/TypeConversion.ts | 4 +-- app/client/components/TypeTransform.ts | 34 ++++++++++++++++-------- sandbox/grist/functions/info.py | 8 ------ sandbox/grist/test_trigger_formulas.py | 2 +- sandbox/grist/useractions.py | 13 +++------ 6 files changed, 32 insertions(+), 35 deletions(-) diff --git a/app/client/components/ColumnTransform.ts b/app/client/components/ColumnTransform.ts index 315e9202..018868d3 100644 --- a/app/client/components/ColumnTransform.ts +++ b/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, ...] diff --git a/app/client/components/TypeConversion.ts b/app/client/components/TypeConversion.ts index a5503370..0fbfd399 100644 --- a/app/client/components/TypeConversion.ts +++ b/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 { + toTypeMaybeFull: string, convertedRef: string): Promise { 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(), }; diff --git a/app/client/components/TypeTransform.ts b/app/client/components/TypeTransform.ts index 509e8890..3a9e67e1 100644 --- a/app/client/components/TypeTransform.ts +++ b/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; + 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); } diff --git a/sandbox/grist/functions/info.py b/sandbox/grist/functions/info.py index 099828f0..87640ef2 100644 --- a/sandbox/grist/functions/info.py +++ b/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`. diff --git a/sandbox/grist/test_trigger_formulas.py b/sandbox/grist/test_trigger_formulas.py index b7c3dd2f..1ea512f7 100644 --- a/sandbox/grist/test_trigger_formulas.py +++ b/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, {}) diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 5485df1c..cb2cf81c 100644 --- a/sandbox/grist/useractions.py +++ b/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)