Archives/gristlabs_grist-core
1
0
Fork 0
mirror of https://github.com/gristlabs/grist-core.git synced 2024-10-27 20:44:07 +00:00
Code Issues Projects Releases Wiki Activity

(core) Fixing hidden col missing after summary update

Browse Source 
Summary:
Updating one summary groupby columns is handle by the
`update_summary_section`. That routine is in charge of creating a new
summary table and transfer all possible columns from old table to the
new one, so that the new table appear to change only the minimum to
users.

The problem that this diff address is that, the logic to decide what columns we need to keep, only applies to the visible
fields and ignore the hidden columns, causing all hidden columns to be removed. This diff, simply
changes that routine to address all columns.

the mutliple fixes on the test_summary2.py result from a change of
ordering of columns: the culprit is the `group` columns, which is by
default a hidden columns and that had to be explicitely added back to
the new summary table. It is now transferred automatically, like other
columns, which does cause a little change of ordering on the db. This
should not result in any display order changes for the users.

Recent related diff: https://phab.getgrist.com/D3351

Test Plan: Includes new test case; both python and nbrowser

Reviewers: alexmojaki

Reviewed By: alexmojaki

Differential Revision: https://phab.getgrist.com/D3393
This commit is contained in:
Cyprien P 2022-04-26 11:01:47 +02:00
parent e59dcc142d
commit 0829ae67ef
4 changed files with 108 additions and 90 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
sandbox/grist/summary.py
View File

@ -197,27 +197,26 @@ class SummaryActions(object):
def update_summary_section(self, view_section, source_table, source_groupby_columns):
source_groupby_colset = set(source_groupby_columns)
groupby_colids = {c.colId for c in source_groupby_columns}
prev_fields = list(view_section.fields)
# Go through fields figuring out which ones we'll keep.
prev_group_fields, formula_fields, delete_fields, missing_colinfo = [], [], [], []
for field in prev_fields:
srcCol = field.colRef.summarySourceCol
# Go through columns figuring out which ones we'll keep.
prev_group_cols, formula_colinfo = [], []
for col in view_section.tableRef.columns:
srcCol = col.summarySourceCol
# Records implement __hash__, so we can look them up in sets.
if srcCol in source_groupby_colset:
prev_group_fields.append(field)
elif field.colRef.isFormula and field.colRef.colId not in groupby_colids:
formula_fields.append(field)
prev_group_cols.append(col)
elif col.isFormula and col.colId not in groupby_colids:
formula_colinfo.append(_make_col_info(col))
else:
# if user is removing a numeric column from the group by columns we must add it back as a
# sum formula column
self._append_sister_column_if_any(missing_colinfo, source_table, srcCol)
delete_fields.append(field)
self._append_sister_column_if_any(formula_colinfo, source_table, srcCol)
# Prepare ColInfo for all columns we want to keep.
formula_colinfo = [_make_col_info(f.colRef) for f in formula_fields]
# All fields with a column that we don't keep, must be deleted
colid_keep_set = set(c.colId for c in prev_group_cols + formula_colinfo)
delete_fields = [f for f in view_section.fields if f.colRef.colId not in colid_keep_set]
have_group_col = any(f.colRef.colId == 'group' for f in formula_fields)
have_group_col = any(ci.colId == 'group' for ci in formula_colinfo)
if not have_group_col:
formula_colinfo.append(_group_colinfo(source_table))
@ -242,19 +241,18 @@ class SummaryActions(object):
# Delete fields no longer relevant.
self.docmodel.remove(delete_fields)
# Add missing sum column
for ci in missing_colinfo:
col = self.useractions.AddColumn(summary_table.tableId, ci.colId,
_get_colinfo_dict(ci, with_id=False))
# AddColumn user action did not add the fields as the view section was not yet updated with
# new table, hence adds it manually
self.docmodel.add(view_section.fields, colRef=[col['colRef']])
# Update fields for all formula fields and reused group-by fields to point to new columns.
colid_to_field_map = {field.colRef.colId: field for field in view_section.fields}
prev_group_fields = [
colid_to_field_map[col.colId] for col in prev_group_cols
if col.colId in colid_to_field_map
]
source_col_map = dict(zip(source_groupby_columns, groupby_columns))
prev_group_columns = [source_col_map[f.colRef.summarySourceCol] for f in prev_group_fields]
visible_formula_columns = [c for c in formula_columns if c.colId in colid_to_field_map]
formula_fields = [colid_to_field_map[c.colId] for c in visible_formula_columns]
self.docmodel.update(formula_fields + prev_group_fields,
colRef=[c.id for c in formula_columns + prev_group_columns])
colRef=[c.id for c in visible_formula_columns + prev_group_columns])
# Finally, we need to create fields for newly-added group-by columns. If there were missing
# fields for any group-by columns before, they'll be created now.

10
sandbox/grist/test_column_actions.py
View File

@ -356,9 +356,9 @@ class TestColumnActions(test_engine.EngineTestCase):
]),
# Note that the summary table here switches to a new one, without the deleted group-by.
Table(4, "GristSummary_7_Address2", 0, 1, columns=[
Column(22, "count", "Int", True, summarySourceCol=0, formula="len($group)"),
Column(23, "amount", "Numeric", True, summarySourceCol=0, formula="SUM($group.amount)"),
Column(24, "group", "RefList:Address", True, summarySourceCol=0,
Column(23, "count", "Int", True, summarySourceCol=0, formula="len($group)"),
Column(24, "amount", "Numeric", True, summarySourceCol=0, formula="SUM($group.amount)"),
Column(22, "group", "RefList:Address", True, summarySourceCol=0,
formula="table.getSummarySourceGroup(rec)"),
]),
])
@ -372,8 +372,8 @@ class TestColumnActions(test_engine.EngineTestCase):
Field(12, colRef=17),
]),
Section(5, parentKey="record", tableRef=4, fields=[
Field(14, colRef=22),
Field(15, colRef=23),
Field(14, colRef=23),
Field(15, colRef=24),
]),
]),
View(2, sections=[

6
sandbox/grist/test_summary.py
View File

@ -824,9 +824,9 @@ class Address:
]),
Table(3, "GristSummary_6_Table1_2", summarySourceTable=1, primaryViewId=0, columns=[
Column(10, "B", "Numeric", False, "", 3),
Column(11, "count", "Int", True, "len($group)", 0),
Column(12, "C", "Numeric", True, "SUM($group.C)", 0),
Column(13, "group", "RefList:Table1", True, "table.getSummarySourceGroup(rec)", 0),
Column(12, "count", "Int", True, "len($group)", 0),
Column(13, "C", "Numeric", True, "SUM($group.C)", 0),
Column(11, "group", "RefList:Table1", True, "table.getSummarySourceGroup(rec)", 0),
])
])
self.assertTableData('Table1', data=[

140
sandbox/grist/test_summary2.py
View File

@ -456,16 +456,16 @@ class TestSummary2(test_engine.EngineTestCase):
# Note that Table #2 is gone at this point, since it's unused.
Table(3, "GristSummary_7_Address2", 0, 1, columns=[
Column(19, "state", "Text", False, "", 12),
Column(20, "count", "Int", True, "len($group)", 0),
Column(21, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(22, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(20, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(21, "count", "Int", True, "len($group)", 0),
Column(22, "amount", "Numeric", True, "SUM($group.amount)", 0),
]),
])
self.assertViews([View(1, sections=[
Section(1, parentKey="record", tableRef=3, fields=[
Field(2, colRef=19),
Field(3, colRef=20),
Field(4, colRef=21),
Field(3, colRef=21),
Field(4, colRef=22),
])
])])
self.assertTableData('GristSummary_7_Address2', cols="subset", data=[
@ -481,7 +481,7 @@ class TestSummary2(test_engine.EngineTestCase):
self.assertTableData('_grist_Tables_column', rows="subset", cols="subset", data=[
['id', 'colId', 'type', 'formula', 'widgetOptions', 'label'],
[19, 'state', 'Text', '', 'WidgetOptions1', 'State'],
[21, 'amount', 'Numeric', 'SUM($group.amount)', 'WidgetOptions2', 'Amount'],
[22, 'amount', 'Numeric', 'SUM($group.amount)', 'WidgetOptions2', 'Amount'],
])
# Change group-by to a different single column ('city')
@ -491,16 +491,16 @@ class TestSummary2(test_engine.EngineTestCase):
# Note that Table #3 is gone at this point, since it's unused.
Table(4, "GristSummary_7_Address", 0, 1, columns=[
Column(23, "city", "Text", False, "", 11),
Column(24, "count", "Int", True, "len($group)", 0),
Column(25, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(26, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(24, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(25, "count", "Int", True, "len($group)", 0),
Column(26, "amount", "Numeric", True, "SUM($group.amount)", 0),
]),
])
self.assertViews([View(1, sections=[
Section(1, parentKey="record", tableRef=4, fields=[
Field(5, colRef=23),
Field(3, colRef=24),
Field(4, colRef=25),
Field(3, colRef=25),
Field(4, colRef=26),
])
])])
self.assertTableData('GristSummary_7_Address', cols="subset", data=[
@ -520,7 +520,7 @@ class TestSummary2(test_engine.EngineTestCase):
self.assertTableData('_grist_Tables_column', rows="subset", cols="subset", data=[
['id', 'colId', 'type', 'formula', 'widgetOptions', 'label'],
[23, 'city', 'Text', '', '', 'City'],
[25, 'amount', 'Numeric', 'SUM($group.amount)', 'WidgetOptions2', 'Amount'],
[26, 'amount', 'Numeric', 'SUM($group.amount)', 'WidgetOptions2', 'Amount'],
])
# Change group-by to no columns (totals)
@ -529,15 +529,15 @@ class TestSummary2(test_engine.EngineTestCase):
self.starting_table,
# Note that Table #4 is gone at this point, since it's unused.
Table(5, "GristSummary_7_Address2", 0, 1, columns=[
Column(27, "count", "Int", True, "len($group)", 0),
Column(28, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(29, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(27, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(28, "count", "Int", True, "len($group)", 0),
Column(29, "amount", "Numeric", True, "SUM($group.amount)", 0),
]),
])
self.assertViews([View(1, sections=[
Section(1, parentKey="record", tableRef=5, fields=[
Field(3, colRef=27),
Field(4, colRef=28),
Field(3, colRef=28),
Field(4, colRef=29),
])
])])
self.assertTableData('GristSummary_7_Address2', cols="subset", data=[
@ -554,17 +554,17 @@ class TestSummary2(test_engine.EngineTestCase):
Table(6, "GristSummary_7_Address", 0, 1, columns=[
Column(30, "state", "Text", False, "", 12),
Column(31, "city", "Text", False, "", 11),
Column(32, "count", "Int", True, "len($group)", 0),
Column(33, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(34, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(32, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(33, "count", "Int", True, "len($group)", 0),
Column(34, "amount", "Numeric", True, "SUM($group.amount)", 0),
]),
])
self.assertViews([View(1, sections=[
Section(1, parentKey="record", tableRef=6, fields=[
Field(5, colRef=30),
Field(6, colRef=31),
Field(3, colRef=32),
Field(4, colRef=33),
Field(3, colRef=33),
Field(4, colRef=34),
])
])])
self.assertTableData('GristSummary_7_Address', cols="subset", data=[
@ -588,23 +588,23 @@ class TestSummary2(test_engine.EngineTestCase):
Table(6, "GristSummary_7_Address", 0, 1, columns=[
Column(30, "state", "Text", False, "", 12),
Column(31, "city", "Text", False, "", 11),
Column(32, "count", "Int", True, "len($group)", 0),
Column(33, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(34, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(32, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(33, "count", "Int", True, "len($group)", 0),
Column(34, "amount", "Numeric", True, "SUM($group.amount)", 0),
]),
])
self.assertViews([View(1, sections=[
Section(1, parentKey="record", tableRef=6, fields=[
Field(5, colRef=30),
Field(6, colRef=31),
Field(3, colRef=32),
Field(4, colRef=33),
Field(3, colRef=33),
Field(4, colRef=34),
]),
Section(2, parentKey="record", tableRef=6, fields=[
Field(7, colRef=31),
Field(8, colRef=30),
Field(9, colRef=32),
Field(10, colRef=33),
Field(9, colRef=33),
Field(10, colRef=34),
])
])])
self.assertEqual(get_helper_cols('Address'), ['#summary#GristSummary_7_Address'])
@ -616,26 +616,26 @@ class TestSummary2(test_engine.EngineTestCase):
Table(6, "GristSummary_7_Address", 0, 1, columns=[
Column(30, "state", "Text", False, "", 12),
Column(31, "city", "Text", False, "", 11),
Column(32, "count", "Int", True, "len($group)", 0),
Column(33, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(34, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(32, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(33, "count", "Int", True, "len($group)", 0),
Column(34, "amount", "Numeric", True, "SUM($group.amount)", 0),
]),
Table(7, "GristSummary_7_Address2", 0, 1, columns=[
Column(35, "count", "Int", True, "len($group)", 0),
Column(36, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(37, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(35, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(36, "count", "Int", True, "len($group)", 0),
Column(37, "amount", "Numeric", True, "SUM($group.amount)", 0),
]),
])
self.assertViews([View(1, sections=[
Section(1, parentKey="record", tableRef=6, fields=[
Field(5, colRef=30),
Field(6, colRef=31),
Field(3, colRef=32),
Field(4, colRef=33),
Field(3, colRef=33),
Field(4, colRef=34),
]),
Section(2, parentKey="record", tableRef=7, fields=[
Field(9, colRef=35),
Field(10, colRef=36),
Field(9, colRef=36),
Field(10, colRef=37),
])
])])
self.assertEqual(get_helper_cols('Address'), ['#summary#GristSummary_7_Address',
@ -649,17 +649,17 @@ class TestSummary2(test_engine.EngineTestCase):
Table(6, "GristSummary_7_Address", 0, 1, columns=[
Column(30, "state", "Text", False, "", 12),
Column(31, "city", "Text", False, "", 11),
Column(32, "count", "Int", True, "len($group)", 0),
Column(33, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(34, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(32, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(33, "count", "Int", True, "len($group)", 0),
Column(34, "amount", "Numeric", True, "SUM($group.amount)", 0),
])
])
self.assertViews([View(1, sections=[
Section(1, parentKey="record", tableRef=6, fields=[
Field(5, colRef=30),
Field(6, colRef=31),
Field(3, colRef=32),
Field(4, colRef=33),
Field(3, colRef=33),
Field(4, colRef=34),
])
])])
self.assertEqual(get_helper_cols('Address'), ['#summary#GristSummary_7_Address'])
@ -673,8 +673,8 @@ class TestSummary2(test_engine.EngineTestCase):
Column(35, "city", "Text", False, "", 11),
Column(36, "state", "Text", False, "", 12),
Column(37, "amount", "Numeric", False, "", 13),
Column(38, "count", "Int", True, "len($group)", 0),
Column(39, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(38, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(39, "count", "Int", True, "len($group)", 0),
]),
])
self.assertViews([View(1, sections=[
@ -682,7 +682,7 @@ class TestSummary2(test_engine.EngineTestCase):
Field(6, colRef=35),
Field(5, colRef=36),
Field(7, colRef=37),
Field(3, colRef=38),
Field(3, colRef=39),
])
])])
self.apply_user_action(["UpdateSummaryViewSection", 1, [11,12]])
@ -691,9 +691,9 @@ class TestSummary2(test_engine.EngineTestCase):
Table(8, "GristSummary_7_Address", 0, 1, columns=[
Column(40, "city", "Text", False, "", 11),
Column(41, "state", "Text", False, "", 12),
Column(42, "count", "Int", True, "len($group)", 0),
Column(42, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(43, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(44, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(44, "count", "Int", True, "len($group)", 0),
]),
])
@ -701,8 +701,28 @@ class TestSummary2(test_engine.EngineTestCase):
Section(1, parentKey="record", tableRef=8, fields=[
Field(6, colRef=40),
Field(5, colRef=41),
Field(3, colRef=42),
Field(7, colRef=44),
Field(7, colRef=42),
Field(3, colRef=44),
])
])])
# Hide a formula and update group by columns; check that the formula columns had not been
# deleted
self.apply_user_action(['RemoveRecord', '_grist_Views_section_field', 7])
self.apply_user_action(["UpdateSummaryViewSection", 1, [11]])
self.assertTables([
self.starting_table,
Table(9, "GristSummary_7_Address2", 0, 1, columns=[
Column(45, "city", "Text", False, "", 11),
Column(46, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(48, "count", "Int", True, "len($group)", 0),
Column(47, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
]),
])
self.assertViews([View(1, sections=[
Section(1, parentKey="record", tableRef=9, fields=[
Field(6, colRef=45),
Field(3, colRef=48),
])
])])
@ -755,9 +775,9 @@ class TestSummary2(test_engine.EngineTestCase):
Table(3, "GristSummary_7_Address2", 0, 1, columns=[
Column(19, "city", "Text", False, "", 11),
Column(20, "state", "Text", False, "", 12),
Column(21, "count", "Int", True, "len($group)", 0),
Column(22, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(23, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(21, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(22, "count", "Int", True, "len($group)", 0),
Column(23, "amount", "Numeric", True, "SUM($group.amount)", 0),
]),
])
self.assertViews([View(1, sections=[
@ -765,8 +785,8 @@ class TestSummary2(test_engine.EngineTestCase):
# We requested 'city' to come before 'state', check that this is the case.
Field(4, colRef=19),
Field(1, colRef=20),
Field(2, colRef=21),
Field(3, colRef=22),
Field(2, colRef=22),
Field(3, colRef=23),
])
])])
@ -890,15 +910,15 @@ class TestSummary2(test_engine.EngineTestCase):
# Note that Table #2 is gone at this point, since it's unused.
Table(3, "GristSummary_7_Address2", 0, 1, columns=[
Column(19, "state", "Text", False, "", 12),
Column(20, "count", "Int", True, "len($group)", 0),
Column(21, "amount", "Numeric", True, "SUM($group.amount)", 0),
Column(22, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(20, "group", "RefList:Address", True, "table.getSummarySourceGroup(rec)", 0),
Column(21, "count", "Int", True, "len($group)", 0),
Column(22, "amount", "Numeric", True, "SUM($group.amount)", 0),
]),
])
# Verify that sortColRefs refers to new columns.
self.assertTableData('_grist_Views_section', cols="subset", data=[
["id", "tableRef", "sortColRefs"],
[1, 3, "[19,-20]"],
[1, 3, "[19,-21]"],
])
#----------------------------------------------------------------------