(core) Fix changing type of source column from choice to choicelist

Summary: Updates the summary column type correctly, rebuilds the table model

Test Plan: Added a python unit test, tested manually in browser

Reviewers: dsagal

Reviewed By: dsagal

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

sandbox/grist/summary.py

@@ -91,6 +91,21 @@ def _update_sort_spec(sort_spec, old_table, new_table):
     return ''
 
 
+def summary_groupby_col_type(source_type):
+  """
+  Returns the type of a groupby column in a summary table
+  given the type of the corresponding column in the source table.
+  Most types are returned unchanged.
+  When a source table is grouped by a list-type (RefList/ChoiceList) column
+  the column is 'flattened' into the corresponding non-list type
+  in the summary table.
+  """
+  if source_type == 'ChoiceList':
+    return 'Choice'
+  else:
+    return source_type.replace('RefList:', 'Ref:')
+
+
 class SummaryActions(object):
 
   def __init__(self, useractions, docmodel):
@@ -126,8 +141,7 @@ class SummaryActions(object):
         col=c,
         isFormula=False,
         formula='',
-        type='Choice' if c.type == 'ChoiceList' else
-        c.type.replace('RefList:', 'Ref:')
+        type=summary_groupby_col_type(c.type)
       )
       for c in source_groupby_columns
     ]

sandbox/grist/table.py

@@ -341,6 +341,9 @@ class Table(object):
 
     _updateSummary.is_private = True
     col_id = summary_table._summary_helper_col_id
+    if self.has_column(col_id):
+      # Column type may have changed, replace completely
+      self.delete_column(self.get_column(col_id))
     col_obj = self._create_or_update_col(col_id, _updateSummary)
     self._special_cols[col_id] = col_obj
     self.all_columns[col_id] = col_obj

sandbox/grist/test_summary_choicelist.py

@@ -35,7 +35,7 @@ class TestSummaryChoiceList(EngineTestCase):
 
   # ----------------------------------------------------------------------
 
-  def test_create_view_section(self):
+  def test_summary_by_choice_list(self):
     self.load_sample(self.sample)
 
     # Verify the starting table; there should be no views yet.
@@ -289,3 +289,66 @@ class TestSummaryChoiceList(EngineTestCase):
     ])
 
     self.assertTableData('Table1', data=summary_data, cols="subset")
+
+  def test_change_choice_to_choicelist(self):
+    sample = testutil.parse_test_sample({
+      "SCHEMA": [
+        [1, "Source", [
+          [10, "other", "Text", False, "", "other", ""],
+          [11, "choices1", "Choice", False, "", "choice", ""],
+        ]]
+      ],
+      "DATA": {
+        "Source": [
+          ["id", "choices1", "other"],
+          [21, "a", "foo"],
+          [22, "b", "bar"],
+        ]
+      }
+    })
+
+    starting_table = Table(1, "Source", primaryViewId=0, summarySourceTable=0, columns=[
+      Column(10, "other", "Text", isFormula=False, formula="", summarySourceCol=0),
+      Column(11, "choices1", "Choice", isFormula=False, formula="", summarySourceCol=0),
+    ])
+
+    self.load_sample(sample)
+
+    # Verify the starting table; there should be no views yet.
+    self.assertTables([starting_table])
+    self.assertViews([])
+
+    # Create a summary section, grouped by the "choices1" column.
+    self.apply_user_action(["CreateViewSection", 1, 0, "record", [11]])
+
+    summary_table = Table(
+      2, "GristSummary_6_Source", primaryViewId=0, summarySourceTable=1,
+      columns=[
+        Column(12, "choices1", "Choice", isFormula=False, formula="", summarySourceCol=11),
+        Column(13, "group", "RefList:Source", isFormula=True, summarySourceCol=0,
+               formula="table.getSummarySourceGroup(rec)"),
+        Column(14, "count", "Int", isFormula=True, summarySourceCol=0,
+               formula="len($group)"),
+      ],
+    )
+
+    data = [
+      ["id", "choices1", "group", "count"],
+      [1, "a", [21], 1],
+      [2, "b", [22], 1],
+    ]
+
+    self.assertTables([starting_table, summary_table])
+    self.assertTableData('GristSummary_6_Source', data=data)
+
+    # Change the column from Choice to ChoiceList
+    self.apply_user_action(["UpdateRecord", "_grist_Tables_column", 11, {"type": "ChoiceList"}])
+
+    # Changing type in reality is a bit more complex than these actions
+    # so we put the correct values in place directly
+    self.apply_user_action(["BulkUpdateRecord", "Source", [21, 22],
+                            {"choices1": [["L", "a"], ["L", "b"]]}])
+
+    starting_table.columns[1] = starting_table.columns[1]._replace(type="ChoiceList")
+    self.assertTables([starting_table, summary_table])
+    self.assertTableData('GristSummary_6_Source', data=data)

sandbox/grist/useractions.py

@@ -36,7 +36,7 @@ _modifiable_col_fields = {'type', 'widgetOptions', 'formula', 'isFormula', 'labe
                           'untieColIdFromLabel'}
 
 # Fields of _grist_Tables_column table that are inherited by group-by columns from their source.
-_inherited_groupby_col_fields = {'colId', 'type', 'widgetOptions', 'label', 'untieColIdFromLabel'}
+_inherited_groupby_col_fields = {'colId', 'widgetOptions', 'label', 'untieColIdFromLabel'}
 
 # Fields of _grist_Tables_column table that are inherited by summary formula columns from source.
 _inherited_summary_col_fields = {'colId', 'label'}
@@ -513,8 +513,11 @@ class UserActions(object):
     # A list of individual (col_rec, values) updates, where values is a per-column dict.
     col_updates = OrderedDict()
     avoid_colid_set = set()
+    rebuild_summary_tables = set()
     for i, col_rec, values in self._bulk_action_iter(table_id, row_ids, col_values):
-      col_updates.update(self._adjust_one_column_update(col_rec, values, avoid_colid_set))
+      col_updates.update(
+        self._adjust_one_column_update(col_rec, values, avoid_colid_set, rebuild_summary_tables)
+      )
 
     # Collect all renamings that we are about to apply.
     renames = {(c.parentId.tableId, c.colId): values['colId']
@@ -561,6 +564,11 @@ class UserActions(object):
       self._engine.trigger_columns_changed()
 
     self.doBulkUpdateFromPairs(table_id, update_pairs)
+
+    for table_id in rebuild_summary_tables:
+      table = self._engine.tables[table_id]
+      self._engine._update_table_model(table, table.user_table)
+
     make_acl_updates()
 
 
@@ -622,7 +630,7 @@ class UserActions(object):
     col_obj = table.get_column(col_rec.colId)
     return (col_obj.raw_get(r) for r in table.row_ids)
 
-  def _adjust_one_column_update(self, col, col_values, avoid_colid_set):
+  def _adjust_one_column_update(self, col, col_values, avoid_colid_set, rebuild_summary_tables):
     # Adjust an update for a single column, implementing the meat of _updateColumnRecords().
     # Returns a list of (col, values) pairs (containing the input column but possibly more).
     # Note that it may modify col_values in-place, and may reuse it for multiple results.
@@ -672,7 +680,12 @@ class UserActions(object):
     else:             # A non-summary-table column.
       # If there are group-by columns based on this, change their properties to match (including
       # colId, for renaming), except formula/isFormula.
-      add(col.summaryGroupByColumns, select_keys(col_values, _inherited_groupby_col_fields))
+      changes = select_keys(col_values, _inherited_groupby_col_fields)
+      if 'type' in col_values:
+        changes['type'] = summary.summary_groupby_col_type(col_values['type'])
+        if col_values['type'] != changes['type']:
+          rebuild_summary_tables.update(t.tableId for t in col.summaryGroupByColumns.parentId)
+      add(col.summaryGroupByColumns, changes)
 
       # If there are summary tables with a same-named formula column, rename those to match.
       add(self._get_sister_columns(col.parentId, col),