(core) Fix some bugs with ChoiceList in summary tables, and evaluation of lookups.

Summary:
Addresses several issues:
- Error 'Cannot modify summary group-by column' when changing Text ->
  ChoiceList in the presence of summary tables.
- Error 'ModifyColumn in unexpected position' when changing ChoiceList -> Text
  in the presence of summary tables.
- Double-evaluation of trigger formulas in some cases.

Fixes include:
- Fixed verification that summary group-by columns match the underlying ones,
  and added comments to explain.
- Avoid updating non-metadata lookups after each doc-action (early lookups
  generated extra actions to populate summary tables, causing the 'ModifyColumn
  in unexpected position' bug)
- When updating formulas, do update lookups first.
- Made a client-side tweak to avoid a JS error in case of some undos.

Solution to reduce lookups is based on https://phab.getgrist.com/D3069?vs=on&id=12445,
and tests for double-evaluation of trigger formulas are taken from there.

Add a new test case to protect against bugs caused by incorrect order of
evaluating #lookup columns.

Enhanced ChoiceList browser test to check a conversion scenario in the presence
of summary tables, previously triggering bugs.

Test Plan: Various tests added or enhanced.

Reviewers: alexmojaki

Reviewed By: alexmojaki

Subscribers: jarek

Differential Revision: https://phab.getgrist.com/D3184

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; }

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):
     """

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}}
     })

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],

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],
+    ])

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.

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)
 

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) {