diff --git a/app/client/widgets/ChoiceTextBox.ts b/app/client/widgets/ChoiceTextBox.ts index 3210e31a..9404e31b 100644 --- a/app/client/widgets/ChoiceTextBox.ts +++ b/app/client/widgets/ChoiceTextBox.ts @@ -47,7 +47,7 @@ export class ChoiceTextBox extends NTextBox { cssChoiceTextWrapper( dom.style('justify-content', (use) => use(this.alignment) === 'right' ? 'flex-end' : use(this.alignment)), dom.domComputed((use) => { - if (use(row._isAddRow)) { return null; } + if (this.isDisposed() || use(row._isAddRow)) { return null; } const formattedValue = use(this.valueFormatter).format(use(value)); if (formattedValue === '') { return null; } diff --git a/sandbox/grist/engine.py b/sandbox/grist/engine.py index e3e9498b..039454b0 100644 --- a/sandbox/grist/engine.py +++ b/sandbox/grist/engine.py @@ -507,7 +507,7 @@ class Engine(object): def _pre_update(self): """ - Called at beginning of _bring_all_up_to_date or _bring_lookups_up_to_date. + Called at beginning of _bring_all_up_to_date or _bring_mlookups_up_to_date. Makes sure cell change accumulation is reset. """ self._changes_map = OrderedDict() @@ -519,7 +519,7 @@ class Engine(object): def _post_update(self): """ - Called at end of _bring_all_up_to_date or _bring_lookups_up_to_date. + Called at end of _bring_all_up_to_date or _bring_mlookups_up_to_date. Issues actions for any accumulated cell changes. """ for node, changes in six.iteritems(self._changes_map): @@ -591,25 +591,30 @@ class Engine(object): if self._recompute_done_counter < self._expected_done_counter: raise Exception('data engine not making progress updating dependencies') if ignore_other_changes: - # For _bring_lookups_up_to_date, we should only wait for the work items + # For _bring_mlookups_up_to_date, we should only wait for the work items # explicitly requested. break # Sanity check that we computed at least one cell. if self.recompute_map and self._recompute_done_counter == 0: raise Exception('data engine not making progress updating formulas') # Figure out remaining work to do, maintaining classic Grist ordering. - nodes = sorted(self.recompute_map.keys(), reverse=True) - work_items = [WorkItem(node, None, []) for node in nodes] + work_items = self._make_sorted_work_items(self.recompute_map.keys()) self._in_update_loop = False + def _make_sorted_work_items(self, nodes): # pylint:disable=no-self-use + # Build WorkItems from a list of nodes, maintaining classic Grist ordering (in order by name). + # WorkItems are processed from the end (hence reverse=True). Additionally, we sort all + # #lookups to be processed first. See note in _bring_mlookups_up_to_date why this is important. + nodes = sorted(nodes, reverse=True, key=lambda n: (not n.col_id.startswith('#lookup'), n)) + return [WorkItem(node, None, []) for node in nodes] + def _bring_all_up_to_date(self): # Bring all nodes up to date. We iterate in sorted order of the keys so that the order is # deterministic (which is helpful for tests in particular). self._pre_update() try: # Figure out remaining work to do, maintaining classic Grist ordering. - nodes = sorted(self.recompute_map.keys(), reverse=True) - work_items = [WorkItem(node, None, []) for node in nodes] + work_items = self._make_sorted_work_items(self.recompute_map.keys()) self._update_loop(work_items) # Check if any potentially unused LookupMaps are still unused, and if so, delete them. for lookup_map in self._unused_lookups: @@ -619,16 +624,20 @@ class Engine(object): self._unused_lookups.clear() self._post_update() - def _bring_lookups_up_to_date(self, triggering_doc_action): - # Just bring the lookup nodes up to date. This is part of a somewhat hacky solution in - # apply_doc_action: lookup nodes don't know exactly what depends on them until they are + def _bring_mlookups_up_to_date(self, triggering_doc_action): + # Just bring the *metadata* lookup nodes up to date. + # + # In general, lookup nodes don't know exactly what depends on them until they are # recomputed. So invalidating lookup nodes doesn't complete all invalidation; further - # invalidations may be generated in the course of recomputing the lookup nodes. So we force + # invalidations may be generated in the course of recomputing the lookup nodes. + # + # We use some private formulas on metadata tables internally (e.g. for a list columns of a + # table). This method is part of a somewhat hacky solution in apply_doc_action: to force # recomputation of lookup nodes to ensure that we see up-to-date results between applying doc # actions. # - # This matters for private formulas used internally; it isn't needed for external use, since - # all nodes are brought up to date before responding to a user action anyway. + # For regular data, correct values aren't needed until we recompute formulas. So we process + # lookups before other formulas, but do not need to update lookups after each doc_action. # # In addition, we expose the triggering doc_action so that lookupOrAddDerived can avoid adding # a record to a derived table when the trigger itself is a change to the derived table. This @@ -636,9 +645,9 @@ class Engine(object): self._pre_update() try: self._triggering_doc_action = triggering_doc_action - nodes = sorted(self.recompute_map.keys(), reverse=True) - nodes = [node for node in nodes if node.col_id.startswith('#lookup')] - work_items = [WorkItem(node, None, []) for node in nodes] + nodes = [node for node in self.recompute_map + if node.col_id.startswith('#lookup') and node.table_id.startswith('_grist_')] + work_items = self._make_sorted_work_items(nodes) self._update_loop(work_items, ignore_other_changes=True) finally: self._triggering_doc_action = None @@ -646,7 +655,7 @@ class Engine(object): def is_triggered_by_table_action(self, table_id): # Workaround for lookupOrAddDerived that prevents AddRecord from being created when the - # trigger is itself an action for the same table. See comments for _bring_lookups_up_to_date. + # trigger is itself an action for the same table. See comments for _bring_mlookups_up_to_date. a = self._triggering_doc_action return a and getattr(a, 'table_id', None) == table_id @@ -1295,11 +1304,11 @@ class Engine(object): # We normally recompute formulas before returning to the user; but some formulas are also used # internally in-between applying doc actions. We have this workaround to ensure that those are - # up-to-date after each doc action. See more in comments for _bring_lookups_up_to_date. + # up-to-date after each doc action. See more in comments for _bring_mlookups_up_to_date. # We check _compute_stack to avoid a recursive call (happens when a formula produces an # action, as for derived/summary tables). if not self._compute_stack: - self._bring_lookups_up_to_date(doc_action) + self._bring_mlookups_up_to_date(doc_action) def autocomplete(self, txt, table_id, column_id, user): """ diff --git a/sandbox/grist/test_derived.py b/sandbox/grist/test_derived.py index 01a2ce46..b3a49d71 100644 --- a/sandbox/grist/test_derived.py +++ b/sandbox/grist/test_derived.py @@ -113,8 +113,8 @@ class TestDerived(test_engine.EngineTestCase): ], "calls": { "GristSummary_6_Orders": {'#lookup#year': 1, "group": 2, "amount": 2, "count": 2}, - "Orders": {"#lookup##summary#GristSummary_6_Orders": 2, - "#summary#GristSummary_6_Orders": 2}} + "Orders": {"#lookup##summary#GristSummary_6_Orders": 1, + "#summary#GristSummary_6_Orders": 1}} }) self.assertPartialData("GristSummary_6_Orders", ["id", "year", "count", "amount", "group" ], [ @@ -296,6 +296,6 @@ class TestDerived(test_engine.EngineTestCase): actions.UpdateRecord("Orders", 1, {"year": 2012}), ], "calls": {"GristSummary_6_Orders": {"group": 1, "amount": 1, "count": 1}, - "Orders": {"#lookup##summary#GristSummary_6_Orders": 2, - "#summary#GristSummary_6_Orders": 2}} + "Orders": {"#lookup##summary#GristSummary_6_Orders": 1, + "#summary#GristSummary_6_Orders": 1}} }) diff --git a/sandbox/grist/test_lookups.py b/sandbox/grist/test_lookups.py index 4c94b80b..1b48332a 100644 --- a/sandbox/grist/test_lookups.py +++ b/sandbox/grist/test_lookups.py @@ -399,7 +399,7 @@ return ",".join(str(r.id) for r in Students.lookupRecords(firstName=fn, lastName self.assertPartialOutActions(out_actions, { "calls": { # No calculations of anything Schools because nothing depends on the incomplete value. - "Students": {"#lookup#firstName:lastName": 2, "schoolIds": 1, "schoolCities": 1} + "Students": {"#lookup#firstName:lastName": 1, "schoolIds": 1, "schoolCities": 1} }, "retValues": [7], }) @@ -408,7 +408,7 @@ return ",".join(str(r.id) for r in Students.lookupRecords(firstName=fn, lastName out_actions = self.add_record("Students", firstName="Chuck", lastName="Norris") self.assertPartialOutActions(out_actions, { "calls": { - "Students": {"#lookup#firstName:lastName": 2, "schoolIds": 1, "schoolCities": 1}, + "Students": {"#lookup#firstName:lastName": 1, "schoolIds": 1, "schoolCities": 1}, "Schools": {"bestStudentId": 1} }, "retValues": [8], diff --git a/sandbox/grist/test_summary_undo.py b/sandbox/grist/test_summary_undo.py new file mode 100644 index 00000000..9f3247aa --- /dev/null +++ b/sandbox/grist/test_summary_undo.py @@ -0,0 +1,55 @@ +""" +Some more test cases for summary tables, involving UNDO. +""" +import logger +import testutil +import test_engine + +log = logger.Logger(__name__, logger.INFO) + +class TestSummaryUndo(test_engine.EngineTestCase): + sample = testutil.parse_test_sample({ + "SCHEMA": [ + [1, "Person", [ + [1, "state", "Text", False], + ]] + ], + "DATA": { + "Person": [ + ["id", "state", ], + [ 1, "NY", ], + [ 2, "IL", ], + [ 3, "ME", ], + [ 4, "NY", ], + [ 5, "IL", ], + ] + } + }) + + def test_summary_undo1(self): + # This tests a particular case of a bug when a summary table wasn't fully updated after UNDO. + self.load_sample(self.sample) + # Create a summary section, grouped by the "State" column. + self.apply_user_action(["CreateViewSection", 1, 0, "record", [1]]) + self.assertTableData('GristSummary_6_Person', cols="subset", data=[ + [ "id", "state", "count"], + [ 1, "NY", 2], + [ 2, "IL", 2], + [ 3, "ME", 1], + ]) + + out_actions = self.update_record('Person', 4, state='ME') + self.assertTableData('GristSummary_6_Person', cols="subset", data=[ + [ "id", "state", "count"], + [ 1, "NY", 1], + [ 2, "IL", 2], + [ 3, "ME", 2], + ]) + + self.apply_undo_actions(out_actions.undo[0:1]) + self.assertTableData('GristSummary_6_Person', cols="subset", data=[ + [ "id", "state", "count"], + [ 1, "NY", 2], + [ 2, "IL", 2], + [ 3, "ME", 1], + ]) diff --git a/sandbox/grist/test_trigger_formulas.py b/sandbox/grist/test_trigger_formulas.py index 216cfe51..3e47b07d 100644 --- a/sandbox/grist/test_trigger_formulas.py +++ b/sandbox/grist/test_trigger_formulas.py @@ -476,36 +476,46 @@ class TestTriggerFormulas(test_engine.EngineTestCase): 6, "BossUpd", "Text", False, "UPPER(value or $Ocean.Head) + '+'", recalcDeps=[2, 6] ) - # Previously there was a bug that meant that columns involved in lookups - # did not recalculate their trigger formulas after changes to themselves + # Previously there were various bugs with trigger formulas in columns involved in lookups: + # 1. They did not recalculate their trigger formulas after changes to themselves + # 2. They calculated the formula twice for new records + # 3. The lookups returned incorrect results creatures_columns.append(testutil.col_schema_row( - 21, "Lookup", "Any", True, "Creatures.lookupRecords(BossUpd='')" + 21, "Lookup", "Any", True, "Creatures.lookupRecords(BossUpd=$BossUpd).id" )) sample = testutil.parse_test_sample(sample_desc) self.load_sample(sample) self.assertTableData("Creatures", cols="subset", data=[ - ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"], - [1, "Dolphin", 2, "Arthur", "Arthur", "Arthur", "Arthur", "Atlantic" ], + ["id","Name", "Ocean", "BossDef","BossNvr", "BossUpd", "BossAll", "OceanName", "Lookup"], + [1, "Dolphin", 2, "Arthur", "Arthur", "Arthur", "Arthur", "Atlantic" , [1]], ]) self.update_record('Creatures', 1, Ocean=3) self.assertTableData("Creatures", cols="subset", data=[ - ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"], - [1, "Dolphin", 3, "Arthur", "Arthur", "ARTHUR+", "Neptune", "Indian" ], + ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName", "Lookup"], + [1, "Dolphin", 3, "Arthur", "Arthur", "ARTHUR+", "Neptune", "Indian" , [1]], ]) self.update_record('Creatures', 1, BossUpd="None") self.assertTableData("Creatures", cols="subset", data=[ - ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"], - [1, "Dolphin", 3, "Arthur", "Arthur", "NONE+", "Neptune", "Indian" ], + ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName", "Lookup"], + [1, "Dolphin", 3, "Arthur", "Arthur", "NONE+", "Neptune", "Indian" , [1]], ]) self.update_record('Creatures', 1, BossUpd="") self.assertTableData("Creatures", cols="subset", data=[ - ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName"], - [1, "Dolphin", 3, "Arthur", "Arthur", "NEPTUNE+", "Neptune", "Indian" ], + ["id","Name", "Ocean", "BossDef", "BossNvr", "BossUpd", "BossAll", "OceanName", "Lookup"], + [1, "Dolphin", 3, "Arthur", "Arthur", "NEPTUNE+","Neptune", "Indian" , [1]], ]) + # Ensuring trigger formula isn't called twice for new records + self.add_record('Creatures', BossUpd="Zeus") + self.assertTableData("Creatures", cols="subset", rows="subset", data=[ + ["id", "BossUpd", "Lookup"], + [2, "ZEUS+" , [2]], + ]) + + def test_last_update_recipe(self): # Use a formula to store time of last-update. Check that it works as expected. # Check that times don't update on reload. diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 1fb9b76c..95bd308e 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -557,13 +557,25 @@ class UserActions(object): update_pairs = col_updates.items() # Disallow most changes to summary group-by columns, except to match the underlying column. + # TODO: This is poor. E.g. renaming a group-by column could rename the underlying column (or + # offer the option to), or could be disabled; either would be better than an error. for col, values in update_pairs: if col.summarySourceCol: - underlying = col_updates.get(col.summarySourceCol, {}) - if not all(value == getattr(col, key) or has_value(underlying, key, value) - for key, value in six.iteritems(values) - if key not in ('displayCol', 'visibleCol')): - raise ValueError("Cannot modify summary group-by column '%s'" % col.colId) + underlying_updates = col_updates.get(col.summarySourceCol, {}) + for key, value in six.iteritems(values): + if key in ('displayCol', 'visibleCol'): + # These can't always match the underlying column, and can now be changed in the + # group-by column. (Perhaps the same should be permitted for all widget options.) + continue + # Properties like colId and type ought to match those of the underlying column (either + # the current ones, or the ones that the underlying column is being changed to). + expected = underlying_updates.get(key, getattr(col, key)) + if key == 'type': + # Type sometimes must differ (e.g. ChoiceList -> Choice). + expected = summary.summary_groupby_col_type(expected) + + if value != expected: + raise ValueError("Cannot modify summary group-by column '%s'" % col.colId) make_acl_updates = acl.prepare_acl_col_renames(self._docmodel, self, renames) diff --git a/test/nbrowser/testUtils.ts b/test/nbrowser/testUtils.ts index 0d96cd67..d4451765 100644 --- a/test/nbrowser/testUtils.ts +++ b/test/nbrowser/testUtils.ts @@ -83,8 +83,11 @@ export function setupTestSuite(options?: TestSuiteOptions) { // After every suite, clear sessionStorage and localStorage to avoid affecting other tests. after(clearCurrentWindowStorage); - // Also, log out, to avoid logins interacting. - after(() => server.removeLogin()); + // Also, log out, to avoid logins interacting, unless NO_CLEANUP is requested (useful for + // debugging tests). + if (!process.env.NO_CLEANUP) { + after(() => server.removeLogin()); + } // If requested, clear user preferences for all test users after this suite. if (options?.clearUserPrefs) {