diff --git a/sandbox/grist/action_summary.py b/sandbox/grist/action_summary.py index 9bc3e07b..de6fad1f 100644 --- a/sandbox/grist/action_summary.py +++ b/sandbox/grist/action_summary.py @@ -85,27 +85,37 @@ class ActionSummary(object): table_id = root_name(table_id) col_id = root_name(col_id) + def update_action(filtered_row_ids, delta_index): + values = [column_delta[r][delta_index] for r in filtered_row_ids] + return actions.BulkUpdateRecord(table_id, filtered_row_ids, {col_id: values}).simplify() + if not defunct: - row_ids = self.filter_out_gone_rows(table_id, full_row_ids) - if row_ids: - values = [column_delta[r][1] for r in row_ids] - out_stored.append(actions.BulkUpdateRecord(table_id, row_ids, {col_id: values}).simplify()) + row_ids_after = self.filter_out_gone_rows(table_id, full_row_ids) + if row_ids_after: + out_stored.append(update_action(row_ids_after, 1)) if self.is_created(table_id, col_id) and not defunct: - # A newly-create column, and not replacing a defunct one. Don't generate undo actions. - pass + # A newly-created column, and not replacing a defunct one. Don't generate undo actions. + return + + ## Maybe add one or two undo update actions for rows that existed before the change. + row_ids_before = self.filter_out_new_rows(table_id, full_row_ids) + + if defunct: + preserved_row_ids = [] else: - row_ids = self.filter_out_new_rows(table_id, full_row_ids) - if row_ids: - values = [column_delta[r][0] for r in row_ids] - undo_action = actions.BulkUpdateRecord(table_id, row_ids, {col_id: values}).simplify() - if defunct: - # If we deleted the column (or its containing table), then during undo, the updates for it - # should come after it's re-added. So we need to insert the undos *before*. - out_undo.insert(0, undo_action) - else: - out_undo.append(undo_action) + preserved_row_ids = self.filter_out_gone_rows(table_id, row_ids_before) + + preserved_row_ids_set = set(preserved_row_ids) + defunct_row_ids = [r for r in row_ids_before if r not in preserved_row_ids_set] + + if preserved_row_ids: + out_undo.append(update_action(preserved_row_ids, 0)) + if defunct_row_ids: + # Updates for deleted rows/columns/tables should come after they're re-added. + # So we need to insert the undos *before*. + out_undo.insert(0, update_action(defunct_row_ids, 0)) def _forTable(self, table_id): return self._tables.get(table_id) or self._tables.setdefault(table_id, TableDelta()) diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index ac2bc9e2..41d687a6 100644 --- a/sandbox/grist/engine.py +++ b/sandbox/grist/engine.py @@ -826,11 +826,6 @@ class Engine(object): # know, so it can set the cell value appropriately and do some other bookkeeping. cycle = required and (node, row_id) in self._locked_cells value = self._recompute_one_cell(table, col, row_id, cycle=cycle, node=node) - except table_module.EmptySummaryRow: - # This record is going to be deleted after the update loop completes. - # Don't save a value for it because that will lead to broken undo actions - # trying to update a record that doesn't exist. - save_value = False except RequestingError: # The formula will be evaluated again soon when we have a response. save_value = False @@ -956,9 +951,8 @@ class Engine(object): raise self._cell_required_error # pylint: disable=raising-bad-type self.formula_tracer(col, record) return result - except (MemoryError, table_module.EmptySummaryRow): + except MemoryError: # Don't try to wrap memory errors. - # EmptySummaryRow should be handled in _recompute_step raise except: # pylint: disable=bare-except # Since col.method runs untrusted user code, we use a bare except to catch all diff --git a/sandbox/grist/table.py b/sandbox/grist/table.py index ed6d85bf..0a9c9d53 100644 --- a/sandbox/grist/table.py +++ b/sandbox/grist/table.py @@ -563,10 +563,6 @@ class Table(object): # Remove rows with empty groups self._engine.docmodel.setAutoRemove(rec, not result) - if not result: - # The group is empty, tell the engine that this record will be deleted - raise EmptySummaryRow() - return result else: return None @@ -662,10 +658,3 @@ class Table(object): # creates a dependency and brings formula columns up-to-date. self._engine._use_node(col.node, relation, row_ids) return [col.get_cell_value(row_id) for row_id in row_ids] - - -class EmptySummaryRow(Exception): - """ - Special exception indicating that the summary group is empty and the row should be removed. - """ - pass diff --git a/sandbox/grist/test_derived.py b/sandbox/grist/test_derived.py index 8c4c0ed3..bc367e09 100644 --- a/sandbox/grist/test_derived.py +++ b/sandbox/grist/test_derived.py @@ -280,6 +280,24 @@ class TestDerived(test_engine.EngineTestCase): [5, 2007, 1, 15.0, [1]], ]) + self.assertPartialOutActions(out_actions_update, { + 'stored': [ + ['UpdateRecord', 'Orders', 1, {'year': 2007}], + ['AddRecord', 'Orders_summary_year', 5, {'year': 2007}], + ['RemoveRecord', 'Orders_summary_year', 1], + ['UpdateRecord', 'Orders_summary_year', 5, {'amount': 15.0}], + ['UpdateRecord', 'Orders_summary_year', 5, {'count': 1}], + ['UpdateRecord', 'Orders_summary_year', 5, {'group': ['L', 1]}], + ], + 'undo': [ + ['UpdateRecord', 'Orders_summary_year', 1, {'group': ['L', 1]}], + ['UpdateRecord', 'Orders_summary_year', 1, {'count': 1}], + ['UpdateRecord', 'Orders_summary_year', 1, {'amount': 15.0}], + ['UpdateRecord', 'Orders', 1, {'year': 2012}], + ['RemoveRecord', 'Orders_summary_year', 5], + ['AddRecord', 'Orders_summary_year', 1, {'group': ['L'], 'year': 2012}], + ]}) + # Undo and ensure that the new line is gone from the summary table. out_actions_undo = self.apply_undo_actions(out_actions_update.undo) self.assertPartialData("Orders_summary_year", ["id", "year", "count", "amount", "group" ], [ @@ -289,13 +307,7 @@ class TestDerived(test_engine.EngineTestCase): [4, 2015, 4, 106.0, [7,8,9,10]], ]) self.assertPartialOutActions(out_actions_undo, { - "stored": [ - actions.AddRecord("Orders_summary_year", 1, { - "amount": 15.0, "count": 1, "group": [1], "year": 2012 - }), - actions.RemoveRecord("Orders_summary_year", 5), - actions.UpdateRecord("Orders", 1, {"year": 2012}), - ], + "stored": out_actions_update.undo[::-1], "calls": { "Orders_summary_year": { "#lookup#": 1, "#lookup#year": 1, "group": 1, "amount": 1, "count": 1 diff --git a/sandbox/grist/test_engine.py b/sandbox/grist/test_engine.py index cddbc9be..7a523f5c 100644 --- a/sandbox/grist/test_engine.py +++ b/sandbox/grist/test_engine.py @@ -388,6 +388,8 @@ def test_undo(test_method): self.assertEqualDocData(self.getFullEngineData(), expected_engine_data) return wrapped +test_undo.__test__ = False # tells pytest that this isn't a test + class TestEngine(EngineTestCase): samples = {} diff --git a/sandbox/grist/test_summary.py b/sandbox/grist/test_summary.py index 735c6897..0dc1ee67 100644 --- a/sandbox/grist/test_summary.py +++ b/sandbox/grist/test_summary.py @@ -503,6 +503,28 @@ class Address: ] }) + # Add a new source record which creates a new summary row, + # then delete the source row which implicitly deletes the summary row. + # Overall this is a no-op, but it tests a specific undo-related bugfix. + self.add_record(source_tbl_name, city="Nowhere", state="??", amount=666) + out_actions = self.remove_record(source_tbl_name, 34) + self.assertOutActions(out_actions, { + 'calc': [], + 'direct': [True, False], + 'stored': [ + ['RemoveRecord', source_tbl_name, 34], + ['RemoveRecord', summary_tbl_name, 12], + ], + 'undo': [ + ['UpdateRecord', summary_tbl_name, 12, {'group': ['L', 34]}], + ['UpdateRecord', summary_tbl_name, 12, {'count': 1}], + ['UpdateRecord', summary_tbl_name, 12, {'amount': 666.0}], + ['AddRecord', source_tbl_name, 34, {'amount': 666.0, 'city': 'Nowhere', 'state': '??'}], + ['AddRecord', summary_tbl_name, 12, + {'city': 'Nowhere', 'group': ['L'], 'state': '??'}], + ], + }) + # Verify the resulting data after all the updates. self.assertTableData(summary_tbl_name, cols="subset", data=[ [ "id", "city", "state", "count", "amount" ], diff --git a/sandbox/grist/test_undo.py b/sandbox/grist/test_undo.py index 439cb7fa..79fe6e78 100644 --- a/sandbox/grist/test_undo.py +++ b/sandbox/grist/test_undo.py @@ -1,6 +1,8 @@ import re import test_engine import testsamples +import testutil + class TestUndo(test_engine.EngineTestCase): def test_bad_undo(self): @@ -80,3 +82,43 @@ class TestUndo(test_engine.EngineTestCase): [3, "Address", [21]], [4, "Table1", [22, 23]], ]) + + @test_engine.test_undo + def test_auto_remove_undo(self): + """ + Test that a formula using docmodel.setAutoRemove doesn't break when undoing. + We don't actually recommend using docmodel.setAutoRemove in formulas, + but it'd be nice, and this is really testing that a bugfix about summary tables + also helps outside of summary tables. + """ + self.load_sample(testutil.parse_test_sample({ + "SCHEMA": [ + [1, "Table", [ + [11, "amount", "Numeric", False, "", "", ""], + [12, "amount2", "Numeric", True, "$amount", "", ""], + [13, "remove", "Any", True, + "table.table._engine.docmodel.setAutoRemove(rec, not $amount2)", "", ""], + ]] + ], + "DATA": { + "Table": [ + ["id", "amount", "amount2"], + [21, 1, 1], + [22, 2, 2], + ] + } + })) + out_actions = self.update_record('Table', 21, amount=0) + self.assertOutActions(out_actions, { + 'calc': [], + 'direct': [True, False], + 'stored': [['UpdateRecord', 'Table', 21, {'amount': 0.0}], + ['RemoveRecord', 'Table', 21]], + 'undo': [['UpdateRecord', 'Table', 21, {'amount2': 1.0}], + ['UpdateRecord', 'Table', 21, {'amount': 1.0}], + ['AddRecord', 'Table', 21, {}]], + }) + self.assertTableData('Table', cols="subset", data=[ + ["id", "amount", "amount2"], + [22, 2, 2], + ])