diff --git a/sandbox/grist/test_table_actions.py b/sandbox/grist/test_table_actions.py index 3ae896ec..42f98724 100644 --- a/sandbox/grist/test_table_actions.py +++ b/sandbox/grist/test_table_actions.py @@ -283,11 +283,12 @@ class TestTableActions(test_engine.EngineTestCase): [3, 3], ]) - # Check that reference columns to this table get removed, with associated fields. + # Check that reference columns to this table get converted self.assertTables([ Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[ Column(5, "manualSort", "ManualSortPos", False, "", 0), Column(6, "name", "Text", False, "", 0), + Column(7, "address", "Int", False, "", 0), Column(8, "city", "Any", True, "$address.city", 0), ]), # Note that the summary table is also gone. @@ -296,13 +297,194 @@ class TestTableActions(test_engine.EngineTestCase): View(2, sections=[ Section(3, parentKey="record", tableRef=2, fields=[ Field(7, colRef=6), + Field(8, colRef=7), Field(9, colRef=8), ]), ]), View(3, sections=[ Section(8, parentKey="record", tableRef=2, fields=[ Field(22, colRef=6), + Field(23, colRef=7), Field(24, colRef=8), ]), ]), ]) + + @test_engine.test_undo + def test_remove_table_referenced_by_formula(self): + self.init_sample_data() + + # Add a simple formula column of reference type. + # Its type will be changed to Any. + self.add_column( + "People", "address2", type="Ref:Address", isFormula=True, formula="$address" + ) + # Add a similar reflist column, but change it to a data column. + # The formula is just an easy way to populate values for the test. + # A data column of type reflist should be changed to text. + self.add_column( + "People", "addresses", type="RefList:Address", isFormula=True, formula="[$address, $address]" + ) + self.modify_column("People", "addresses", isFormula=False) + + # Now remove the referenced table and see what happens to the ref columns. + self.apply_user_action(["RemoveTable", "Address"]) + self.assertTables([ + Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[ + Column(5, "manualSort", "ManualSortPos", False, "", 0), + Column(6, "name", "Text", False, "", 0), + # Original data column of type Ref:Address is changed to Int. + Column(7, "address", "Int", False, "", 0), + Column(8, "city", "Any", True, "$address.city", 0), + # Formula column of type Ref:Address is changed to Any. + Column(13, "address2", "Any", True, "$address", 0), + # Data column of type RefList:Address is changed to Text. + Column(14, "addresses", "Text", False, "[$address, $address]", 0), + ]), + ]) + self.assertTableData('People', cols="subset", data=[ + ["id", "name", "address", "address2", "addresses"], + [ 1, "Alice", 22, 22, "22,22"], + [ 2, "Bob", 25, 25, "25,25"], + [ 3, "Carol", 27, 27, "27,27"], + ]) + + @test_engine.test_undo + def test_remove_table_referenced_by_summary_groupby_col_without_visible_col(self): + self.init_sample_data() + # Create a summary table of People grouped by address (a reference column). + self.apply_user_action(["CreateViewSection", 2, 0, 'record', [7], None]) + + self.assertTables([ + Table(1, "Address", primaryViewId=1, summarySourceTable=0, columns=[ + Column(1, "manualSort", "ManualSortPos", False, "", 0), + Column(2, "city", "Text", False, "", 0), + Column(3, "state", "Text", False, "", 0), + Column(4, "amount", "Numeric", False, "", 0), + ]), + Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[ + Column(5, "manualSort", "ManualSortPos", False, "", 0), + Column(6, "name", "Text", False, "", 0), + Column(7, "address", "Ref:Address", False, "", 0), + Column(8, "city", "Any", True, "$address.city", 0), + ]), + Table(3, "Address_summary_state", 0, 1, columns=[ + Column(9, "state", "Text", False, "", summarySourceCol=3), + Column(10, "group", "RefList:Address", True, summarySourceCol=0, + formula="table.getSummarySourceGroup(rec)"), + Column(11, "count", "Int", True, summarySourceCol=0, formula="len($group)"), + Column(12, "amount", "Numeric", True, summarySourceCol=0, formula="SUM($group.amount)"), + ]), + Table(4, "People_summary_address", 0, 2, columns=[ + Column(13, "address", "Ref:Address", False, "", summarySourceCol=7), + Column(14, "group", "RefList:People", True, summarySourceCol=0, + formula="table.getSummarySourceGroup(rec)"), + Column(15, "count", "Int", True, summarySourceCol=0, formula="len($group)"), + ]), + ]) + self.assertTableData('People_summary_address', data=[ + ["id", "address", "count", "group"], + [1, 22, 1, [1]], + [2, 25, 1, [2]], + [3, 27, 1, [3]], + ]) + + # Now remove the referenced table. + self.apply_user_action(["RemoveTable", "Address"]) + # In both the People and summary tables, the 'address' reference column + # is converted to Int, because it didn't have a visible/display column. + self.assertTables([ + Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[ + Column(5, "manualSort", "ManualSortPos", False, "", 0), + Column(6, "name", "Text", False, "", 0), + Column(7, "address", "Int", False, "", 0), + Column(8, "city", "Any", True, "$address.city", 0), + ]), + Table(4, "People_summary_address", 0, 2, columns=[ + Column(13, "address", "Int", False, "", summarySourceCol=7), + Column(14, "group", "RefList:People", True, summarySourceCol=0, + formula="table.getSummarySourceGroup(rec)"), + Column(15, "count", "Int", True, summarySourceCol=0, formula="len($group)"), + ]), + ]) + self.assertTableData('People', cols="subset", data=self.people_table_data) + self.assertTableData('People_summary_address', data=[ + ["id", "address", "count", "group"], + [1, 22, 1, [1]], + [2, 25, 1, [2]], + [3, 27, 1, [3]], + ]) + + @test_engine.test_undo + def test_remove_table_referenced_by_summary_groupby_col_with_visible_col(self): + # Similar to the test above, but now the reference column has a visible column. + self.init_sample_data() + self.modify_column("People", "address", visibleCol=2) + self.apply_user_action(["SetDisplayFormula", "People", 0, 7, "$address.city"]) + self.apply_user_action(["CreateViewSection", 2, 0, 'record', [7], None]) + + self.assertTables([ + Table(1, "Address", primaryViewId=1, summarySourceTable=0, columns=[ + Column(1, "manualSort", "ManualSortPos", False, "", 0), + Column(2, "city", "Text", False, "", 0), + Column(3, "state", "Text", False, "", 0), + Column(4, "amount", "Numeric", False, "", 0), + ]), + Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[ + Column(5, "manualSort", "ManualSortPos", False, "", 0), + Column(6, "name", "Text", False, "", 0), + Column(7, "address", "Ref:Address", False, "", 0), + Column(8, "city", "Any", True, "$address.city", 0), + Column(13, "gristHelper_Display", "Any", True, "$address.city", 0), + ]), + Table(3, "Address_summary_state", 0, 1, columns=[ + Column(9, "state", "Text", False, "", summarySourceCol=3), + Column(10, "group", "RefList:Address", True, summarySourceCol=0, + formula="table.getSummarySourceGroup(rec)"), + Column(11, "count", "Int", True, summarySourceCol=0, formula="len($group)"), + Column(12, "amount", "Numeric", True, summarySourceCol=0, formula="SUM($group.amount)"), + ]), + Table(4, "People_summary_address", 0, 2, columns=[ + Column(14, "address", "Ref:Address", False, "", summarySourceCol=7), + Column(15, "group", "RefList:People", True, summarySourceCol=0, + formula="table.getSummarySourceGroup(rec)"), + Column(16, "count", "Int", True, summarySourceCol=0, formula="len($group)"), + Column(17, "gristHelper_Display", "Any", True, "$address.city", 0), + ]), + ]) + self.assertTableData('People_summary_address', data=[ + ["id", "address", "count", "group", "gristHelper_Display"], + [1, 22, 1, [1], "Albany"], + [2, 25, 1, [2], "Bedford"], + [3, 27, 1, [3], "Buffalo"], + ]) + + self.apply_user_action(["RemoveTable", "Address"]) + self.assertTables([ + Table(2, "People", primaryViewId=2, summarySourceTable=0, columns=[ + Column(5, "manualSort", "ManualSortPos", False, "", 0), + Column(6, "name", "Text", False, "", 0), + # Reference column is converted to the visible column type, i.e. Text. + Column(7, "address", "Text", False, "", 0), + Column(8, "city", "Any", True, "$address.city", 0), + ]), + Table(4, "People_summary_address", 0, 2, columns=[ + # Reference column is converted to the visible column type, i.e. Text. + Column(14, "address", "Text", False, "", summarySourceCol=7), + Column(15, "group", "RefList:People", True, summarySourceCol=0, + formula="table.getSummarySourceGroup(rec)"), + Column(16, "count", "Int", True, summarySourceCol=0, formula="len($group)"), + ]), + ]) + self.assertTableData('People', cols="subset", data=[ + ["id", "name", "address" ], + [ 1, "Alice", "Albany"], + [ 2, "Bob", "Bedford"], + [ 3, "Carol", "Buffalo"], + ]) + self.assertTableData('People_summary_address', data=[ + ["id", "address", "count", "group"], + [ 4, "Albany", 1, [1]], + [ 5, "Bedford", 1, [2]], + [ 6, "Buffalo", 1, [3]], + ]) diff --git a/sandbox/grist/testscript.json b/sandbox/grist/testscript.json index 0bfef040..5c5b7cf5 100644 --- a/sandbox/grist/testscript.json +++ b/sandbox/grist/testscript.json @@ -2398,13 +2398,13 @@ "USER_ACTION": ["RemoveTable", "Schools"], "ACTIONS": { "stored": [ - // Assert that calc actions include a RemoveColumn for Students.school - ["RemoveRecord", "_grist_Tables_column", 5], - ["RemoveColumn", "Students", "school"], + // Students.school is converted from Ref:Schools to Int + ["ModifyColumn", "Students", "school", {"type": "Int"}], + ["UpdateRecord", "_grist_Tables_column", 5, {"type": "Int"}], + ["BulkRemoveRecord", "_grist_Tables_column", [10, 12]], ["RemoveRecord", "_grist_Tables", 2], ["RemoveTable", "Schools"], - // Assert that calc actions include a RemoveColumn for Students.school ["BulkUpdateRecord", "Students", [1, 2, 3, 4, 5, 6, 8], {"schoolRegion": [["E","AttributeError"], ["E","AttributeError"], ["E","AttributeError"], ["E","AttributeError"], ["E","AttributeError"], @@ -2418,12 +2418,8 @@ ], "direct": [true, true, true, true, true, false, false], "undo": [ - ["AddRecord", "_grist_Tables_column", 5, - {"parentPos": 5.0, "parentId": 1, - "colId": "school", "type": "Ref:Schools", "label": "school"}], - ["BulkUpdateRecord", "Students", [1, 2, 3, 4, 5, 6, 8], - {"school": [2, 8, 6, 8, 1, 5, 7]}], - ["AddColumn", "Students", "school", {"isFormula": false, "formula": "", "type": "Ref:Schools"}], + ["ModifyColumn", "Students", "school", {"type": "Ref:Schools"}], + ["UpdateRecord", "_grist_Tables_column", 5, {"type": "Ref:Schools"}], ["BulkAddRecord", "_grist_Tables_column", [10, 12], { "colId": ["name", "address"], "label": ["Name", "Address"], "parentId": [2, 2], "parentPos": [1.0, 2.0], @@ -2449,14 +2445,14 @@ }], ["CHECK_OUTPUT", { "Students": [ - ["id","firstName","lastName","@fullName","@fullNameLen","@schoolShort","@schoolRegion"], - [1, "Barack", "Obama", "Barack Obama", 12, ["E","AttributeError"], ["E","AttributeError"]], - [2, "George W", "Bush", "George W Bush",13, ["E","AttributeError"], ["E","AttributeError"]], - [3, "Bill", "Clinton", "Bill Clinton", 12, ["E","AttributeError"], ["E","AttributeError"]], - [4, "George H", "Bush", "George H Bush",13, ["E","AttributeError"], ["E","AttributeError"]], - [5, "Ronald", "Reagan", "Ronald Reagan",13, ["E","AttributeError"], ["E","AttributeError"]], - [6, "Jimmy", "Carter", "Jimmy Carter", 12, ["E","AttributeError"], ["E","AttributeError"]], - [8, "Gerald", "Ford", "Gerald Ford", 11, ["E","AttributeError"], ["E","AttributeError"]]], + ["id","firstName","lastName","@fullName","@fullNameLen","school","@schoolShort","@schoolRegion"], + [1, "Barack", "Obama", "Barack Obama", 12, 2, ["E","AttributeError"], ["E","AttributeError"]], + [2, "George W", "Bush", "George W Bush",13, 8, ["E","AttributeError"], ["E","AttributeError"]], + [3, "Bill", "Clinton", "Bill Clinton", 12, 6, ["E","AttributeError"], ["E","AttributeError"]], + [4, "George H", "Bush", "George H Bush",13, 8, ["E","AttributeError"], ["E","AttributeError"]], + [5, "Ronald", "Reagan", "Ronald Reagan",13, 1, ["E","AttributeError"], ["E","AttributeError"]], + [6, "Jimmy", "Carter", "Jimmy Carter", 12, 5, ["E","AttributeError"], ["E","AttributeError"]], + [8, "Gerald", "Ford", "Gerald Ford", 11, 7, ["E","AttributeError"], ["E","AttributeError"]]], "Address": [ ["id", "city", "state", "country", "@region"], [2, "Eureka", "IL", "US", "North America"], @@ -2470,23 +2466,23 @@ "USER_ACTION": ["RemoveTable", "Students"], "ACTIONS": { "stored": [ - ["BulkRemoveRecord", "_grist_Tables_column", [1, 2, 3, 4, 6, 9]], + ["BulkRemoveRecord", "_grist_Tables_column", [1, 2, 3, 4, 5, 6, 9]], ["RemoveRecord", "_grist_Tables", 1], ["RemoveTable", "Students"] ], "direct": [true, true, true], "undo": [ - ["BulkAddRecord", "_grist_Tables_column", [1, 2, 3, 4, 6, 9], { - "colId": ["firstName", "lastName", "fullName", "fullNameLen", "schoolShort", + ["BulkAddRecord", "_grist_Tables_column", [1, 2, 3, 4, 5, 6, 9], { + "colId": ["firstName", "lastName", "fullName", "fullNameLen", "school", "schoolShort", "schoolRegion"], - "formula": ["", "", "rec.firstName + ' ' + rec.lastName", "len(rec.fullName)", + "formula": ["", "", "rec.firstName + ' ' + rec.lastName", "len(rec.fullName)", "", "rec.school.name.split(' ')[0]", "addr = $school.address\naddr.state if addr.country == 'US' else addr.region"], - "isFormula": [false, false, true, true, true, true], - "label": ["First Name", "Last Name", "Full Name", "Full Name Length", "School Short", + "isFormula": [false, false, true, true, false, true, true], + "label": ["First Name", "Last Name", "Full Name", "Full Name Length", "school","School Short", "School Region"], - "parentId": [1, 1, 1, 1, 1, 1], "parentPos": [1, 2, 3, 4, 6, 7], - "type": ["Text", "Text", "Any", "Any", "Any", "Any"] + "parentId": [1, 1, 1, 1, 1, 1, 1], "parentPos": [1, 2, 3, 4, 5, 6, 7], + "type": ["Text", "Text", "Any", "Any", "Int", "Any", "Any"] }], ["AddRecord", "_grist_Tables", 1, {"tableId": "Students"}], ["BulkAddRecord", "Students", [1, 2, 3, 4, 5, 6, 8], { @@ -2494,6 +2490,7 @@ "fullName": ["Barack Obama", "George W Bush", "Bill Clinton", "George H Bush", "Ronald Reagan", "Jimmy Carter", "Gerald Ford"], "fullNameLen": [12, 13, 12, 13, 13, 12, 11], "lastName": ["Obama", "Bush", "Clinton", "Bush", "Reagan", "Carter", "Ford"], + "school": [2, 8, 6, 8, 1, 5, 7], "schoolRegion": [["E", "AttributeError"], ["E", "AttributeError"], ["E", "AttributeError"], ["E", "AttributeError"], ["E", "AttributeError"], ["E", "AttributeError"], ["E", "AttributeError"]], "schoolShort": [["E", "AttributeError"], ["E", "AttributeError"], ["E", "AttributeError"], @@ -2510,7 +2507,8 @@ "formula": "rec.school.name.split(' ')[0]", "type": "Any", "id": "schoolShort"}, { "isFormula": true, "formula": "addr = $school.address\naddr.state if addr.country == 'US' else addr.region", "type": "Any" , - "id": "schoolRegion"} + "id": "schoolRegion"}, + {"formula": "", "id": "school", "isFormula": false, "type": "Int"} ]] ] } diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index d0b43792..30868a8e 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -858,6 +858,11 @@ class UserActions(object): # If there are group-by columns based on this, change their properties to match (including # colId, for renaming), except formula/isFormula. changes = select_keys(col_values, _inherited_groupby_col_fields) + for field in ['displayCol', 'visibleCol']: + if field in col_values and not col_values[field]: + # If displayCol or visibleCol is being cleared in this col, it should be cleared + # in the groupby columns based on this. + changes[field] = col_values[field] if 'type' in col_values: changes['type'] = summary.summary_groupby_col_type(col_values['type']) if col_values['type'] != changes['type']: @@ -1095,8 +1100,13 @@ class UserActions(object): # If there are summary tables based on this table, remove those too. remove_table_recs.extend(st for t in remove_table_recs for st in t.summaryTables) - # If other tables have columns referring to this table, remove them. - self.doRemoveColumns(self._collect_back_references(remove_table_recs)) + # Handle columns in other tables referring to this table + for ref in self._collect_back_references(remove_table_recs): + if ref.summarySourceCol: + # Skip summary groupby columns, as updating their values is forbidden. + # They will be handled automatically when the source column is updated. + continue + self._convert_reference_col_for_deleted_table(ref) # Remove all view sections and fields for all tables being removed. # Bypass the check for raw data view sections. @@ -1125,6 +1135,50 @@ class UserActions(object): for table_id in remove_table_ids: self._do_doc_action(actions.RemoveTable(table_id)) + def _convert_reference_col_for_deleted_table(self, col): + # col is a column of type Ref:{table_id} or RefList:{table_id} + # where table_id is a table that is being deleted. + # That type will become invalid. We need to convert it to a type that is valid. + table_id = col.parentId.tableId + col_id = col.colId + visible_col = col.visibleCol + display_col = col.displayCol + if col.isFormula: + # Formula columns are easy, as they allow the Any type. + # The contents will probably become some errors which the user should handle. + self.ModifyColumn(table_id, col_id, dict(type="Any")) + return + + # For data columns, we may also need to update the values. + if not (visible_col and display_col): + # If there's no visible/display column, we just keep row IDs. + if col.type.startswith("Ref:"): + self.ModifyColumn(table_id, col_id, dict(type="Int")) + else: + # Data columns can't be of type Any, and there's no type that can + # hold a list of numbers. So we convert the lists of row IDs + # to strings containing comma-separated row IDs. + # We need to get the values before changing the column type. + table = self._engine.tables[table_id] + new_values = [",".join(map(str, row)) for row in self._get_column_values(col)] + self.ModifyColumn(table_id, col_id, dict(type="Text")) + self.BulkUpdateRecord(table_id, list(table.row_ids), {col_id: new_values}) + return + + if col.type.startswith("Ref:") and visible_col.type != "Any": + # This case is easy: we copy the values from the display column directly into + # the converted ex-reference column. No need for any complicated conversion. + self.CopyFromColumn(table_id, display_col.colId, col_id, visible_col.widgetOptions) + self.ModifyColumn(table_id, col_id, dict(type=visible_col.type)) + else: + # Otherwise, we need to do a 'full' type conversion. + # Note that this involves `call_external`, i.e. calling JS code. + # This is impossible in the Python tests, so this case is tested in DocApi.ts. + self.ConvertFromColumn( + # widgetOptions and visibleColRef are generally used for parsing values into the new type. + # They're not need here since we're just formatting as Text. + table_id, col_id, col_id, typ="Text", widgetOptions="", visibleColRef=0 + ) @override_action('BulkRemoveRecord', '_grist_Tables_column') def _removeColumnRecords(self, table_id, row_ids): @@ -1602,7 +1656,7 @@ class UserActions(object): if src_column.is_formula(): self._engine.bring_col_up_to_date(src_column) - # NOTE: This action is invoked only in a single place (during type/colum/data) + # NOTE: This action is invoked only in a single place in the client (during type/column/data # transformation - where user has a chance to adjust some widgetOptions (though # the UI is limited). Those widget options were already cleared (in js) and are either # nullish (default ones) or are truly adjusted. As Grist doesn't know if the widgetOptions diff --git a/test/server/lib/DocApi.ts b/test/server/lib/DocApi.ts index f6e0ad37..a0b9376b 100644 --- a/test/server/lib/DocApi.ts +++ b/test/server/lib/DocApi.ts @@ -1506,6 +1506,72 @@ function testDocApi() { } } + // This is mostly tested in Python, but this case requires the data engine to call + // 'external' (i.e. JS) code to do the type conversion. + it("converts reference columns when the target table is deleted", async () => { + // Create a test document. + const ws1 = (await userApi.getOrgWorkspaces('current'))[0].id; + const docId = await userApi.newDoc({name: 'testdoc'}, ws1); + const docUrl = `${serverUrl}/api/docs/${docId}`; + + // Make a new table with a reference column pointing at Table1, displaying column A. + let resp = await axios.post(`${docUrl}/apply`, [ + ['AddTable', 'Table2', [{id: 'R', type: 'RefList:Table1'}]], + ['ModifyColumn', 'Table2', 'R', {visibleCol: 2}], + ['SetDisplayFormula', 'Table2', 0, 6, "$R.A"], + ['BulkAddRecord', 'Table1', [1, 2], {A: ["Alice", "Bob"]}], + ['BulkAddRecord', 'Table2', [1], {R: [["L", 1, 2]]}], + ], chimpy); + assert.equal(resp.status, 200); + + // Now delete the referenced table. + // This action has to be separate for the test to pass. + resp = await axios.post(`${docUrl}/apply`, [ + ['RemoveTable', 'Table1'], + ], chimpy); + assert.equal(resp.status, 200); + + resp = await axios.get(`${docUrl}/tables/Table2/columns`, chimpy); + assert.deepEqual(resp.data, { + "columns": [ + { + "id": "R", + "fields": { + "colRef": 6, + "parentId": 2, + "parentPos": 6, + // Type changed from RefList to Text + "type": "Text", + "widgetOptions": "", + "isFormula": false, + "formula": "", + "label": "R", + "description": "", + "untieColIdFromLabel": false, + "summarySourceCol": 0, + // Display and visible columns cleared + "displayCol": 0, + "visibleCol": 0, + "rules": null, + "recalcWhen": 0, + "recalcDeps": null + } + } + ] + } + ); + + resp = await axios.get(`${docUrl}/tables/Table2/records`, chimpy); + assert.deepEqual(resp.data, { + records: + [ + // Reflist converted to comma separated display values. + {id: 1, fields: {R: "Alice, Bob"}}, + ] + } + ); + }); + it("parses strings in user actions", async () => { // Create a test document. const ws1 = (await userApi.getOrgWorkspaces('current'))[0].id;