From 0d1a28512902abeb6076350aab446734e16f42cb Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Wed, 11 Aug 2021 19:45:24 +0200 Subject: [PATCH] (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 | 18 ++++++- sandbox/grist/table.py | 3 ++ sandbox/grist/test_summary_choicelist.py | 65 +++++++++++++++++++++++- sandbox/grist/useractions.py | 21 ++++++-- 4 files changed, 100 insertions(+), 7 deletions(-) diff --git a/sandbox/grist/summary.py b/sandbox/grist/summary.py index 6f3adeff..180490eb 100644 --- a/sandbox/grist/summary.py +++ b/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 ] diff --git a/sandbox/grist/table.py b/sandbox/grist/table.py index 6112e232..14684b11 100644 --- a/sandbox/grist/table.py +++ b/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 diff --git a/sandbox/grist/test_summary_choicelist.py b/sandbox/grist/test_summary_choicelist.py index f6d90b28..6825e93f 100644 --- a/sandbox/grist/test_summary_choicelist.py +++ b/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) diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 7820bf72..a313bb93 100644 --- a/sandbox/grist/useractions.py +++ b/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),