diff --git a/app/client/components/GridView.js b/app/client/components/GridView.js index b7454b0f..3ddc2f7e 100644 --- a/app/client/components/GridView.js +++ b/app/client/components/GridView.js @@ -890,10 +890,14 @@ GridView.prototype.deleteColumns = function(selection) { }); return Promise.resolve(false); } - let actions = fields.filter(col => !col.disableModify()).map(col => ['RemoveColumn', col.colId()]); - if (actions.length > 0) { - return this.tableModel.sendTableActions(actions, `Removed columns ${actions.map(a => a[1]).join(', ')} ` + - `from ${this.tableModel.tableData.tableId}.`).then(() => this.clearSelection()); + const columns = fields.filter(col => !col.disableModify()); + const colRefs = columns.map(col => col.colRef.peek()); + if (colRefs.length > 0) { + return this.gristDoc.docData.sendAction( + ['BulkRemoveRecord', '_grist_Tables_column', colRefs], + `Removed columns ${columns.map(col => col.colId.peek()).join(', ')} ` + + `from ${this.tableModel.tableData.tableId}.` + ).then(() => this.clearSelection()); } return Promise.resolve(false); }; diff --git a/sandbox/grist/docmodel.py b/sandbox/grist/docmodel.py index 8eec2726..4197c1bb 100644 --- a/sandbox/grist/docmodel.py +++ b/sandbox/grist/docmodel.py @@ -260,7 +260,11 @@ class DocModel(object): Remove the records marked for removal. """ # Sort to make sure removals are done in deterministic order. - gone_records = sorted(self._auto_remove_set) + gone_records = sorted( + self._auto_remove_set, + # Remove tables last to prevent errors trying to remove rows or columns from deleted tables. + key=lambda r: (r._table.table_id == "_grist_Tables", r) + ) self._auto_remove_set.clear() # setAutoRemove is called by formulas, notably summary tables, and shouldn't be blocked by ACL. with self._engine.user_actions.indirect_actions(): diff --git a/sandbox/grist/test_summary.py b/sandbox/grist/test_summary.py index fa19df8a..6af9e2a8 100644 --- a/sandbox/grist/test_summary.py +++ b/sandbox/grist/test_summary.py @@ -4,10 +4,12 @@ files: test_summary.py and test_summary2.py. """ import logging + import actions import summary -import testutil import test_engine +import testutil +import useractions from test_engine import Table, Column, View, Section, Field from useractions import allowed_summary_change @@ -870,6 +872,57 @@ class Address: [ 2, 2.0, [3], 1, 6 ], ]) + def test_remove_source_columns_with_display_column(self): + # Verify a fix for a specific bug: removing multiple groupby source columns + # when the summary table contains a display column. + + self.apply_user_action(["AddEmptyTable", None]) + # Group by A and B + self.apply_user_action(["CreateViewSection", 1, 0, "record", [2,3], None]) + # Add a display column for the group column + self.apply_user_action(['SetDisplayFormula', 'Table1_summary_A_B', None, 7, '$group.C']) + + # Verify metadata and initially. + self.assertTables([ + Table(1, "Table1", summarySourceTable=0, primaryViewId=1, columns=[ + Column(1, "manualSort", "ManualSortPos", False, "", 0), + Column(2, "A", "Any", True, "", 0), + Column(3, "B", "Any", True, "", 0), + Column(4, "C", "Any", True, "", 0), + ]), + Table(2, "Table1_summary_A_B", summarySourceTable=1, primaryViewId=0, columns=[ + Column(5, "A", "Any", False, "", 2), + Column(6, "B", "Any", False, "", 3), + Column(7, "group", "RefList:Table1", True, "table.getSummarySourceGroup(rec)", 0), + Column(8, "count", "Int", True, "len($group)", 0), + Column(9, "gristHelper_Display", "Any", True, "$group.C", 0), + ]) + ]) + + user_actions = [ + useractions.from_repr(ua) for ua in + [ + ['RemoveColumn', 'Table1', 'A'], + ['RemoveColumn', 'Table1', 'B'], + ] + ] + self.engine.apply_user_actions(user_actions) + + # Verify that the final structure is as expected. + self.assertTables([ + Table(1, "Table1", summarySourceTable=0, primaryViewId=1, columns=[ + Column(1, "manualSort", "ManualSortPos", False, "", 0), + Column(4, "C", "Any", True, "", 0), + ]), + # Table1_summary_A_B was removed and recreated as Table1_summary. + # This removed Table1_summary_A_B.group which automatically removed gristHelper_Display + # which led to an error in the past. + Table(4, "Table1_summary", summarySourceTable=1, primaryViewId=0, columns=[ + Column(14, "group", "RefList:Table1", True, "table.getSummarySourceGroup(rec)", 0), + Column(15, "count", "Int", True, "len($group)", 0), + ]) + ]) + #---------------------------------------------------------------------- # pylint: disable=R0915 def test_allow_select_by_change(self): diff --git a/test/fixtures/docs/DeleteColumnsUndo.grist b/test/fixtures/docs/DeleteColumnsUndo.grist new file mode 100644 index 00000000..4365e365 Binary files /dev/null and b/test/fixtures/docs/DeleteColumnsUndo.grist differ diff --git a/test/nbrowser/DeleteColumnsUndo.ts b/test/nbrowser/DeleteColumnsUndo.ts new file mode 100644 index 00000000..9bad15f3 --- /dev/null +++ b/test/nbrowser/DeleteColumnsUndo.ts @@ -0,0 +1,33 @@ +import {assert, driver, Key} from 'mocha-webdriver'; + +import * as gu from 'test/nbrowser/gristUtils'; +import {server, setupTestSuite} from "test/nbrowser/testUtils"; + +describe('DeleteColumnsUndo', function () { + this.timeout(20000); + setupTestSuite(); + + before(async function () { + await server.simulateLogin("Chimpy", "chimpy@getgrist.com", 'nasa'); + const doc = await gu.importFixturesDoc('chimpy', 'nasa', 'Horizon', 'DeleteColumnsUndo.grist', false); + await driver.get(`${server.getHost()}/o/nasa/doc/${doc.id}/p/2`); + await gu.waitForDocToLoad(); + }); + + it('should be able to delete multiple columns and undo without errors', async function () { + const revert = await gu.begin(); + assert.deepEqual(await gu.getColumnNames(), ['A', 'B', 'C', 'D']); + await gu.getColumnHeader({col: 'A'}).click(); + await gu.sendKeys(Key.chord(Key.SHIFT, Key.RIGHT)); + await gu.sendKeys(Key.chord(Key.SHIFT, Key.RIGHT)); + const selectedCols = await driver.findAll(".column_name.selected"); + assert.lengthOf(selectedCols, 3); + await gu.openColumnMenu('A', 'Delete 3 columns'); + await gu.waitForServer(); + assert.deepEqual(await gu.getColumnNames(), ['D']); + await revert(); + await gu.checkForErrors(); + assert.deepEqual(await gu.getColumnNames(), ['A', 'B', 'C', 'D']); + }); + +});