From f2141851be8db43c383e9f994d346f1431c0144a Mon Sep 17 00:00:00 2001 From: George Gevoian Date: Tue, 23 Jul 2024 11:13:06 -0400 Subject: [PATCH] (core) Automatically remove leaves from layout specs Summary: When fields or sections were being removed from Grist documents, any layout specs that referred to them weren't being updated to no longer do so. This mismatch was causing various buggy scenarios to manifest in cases where the stale ids were being reused. We now automatically update any affected layout specs whenever fields or sections are removed. Test Plan: Python tests. Reviewers: jarek Reviewed By: jarek Differential Revision: https://phab.getgrist.com/D4302 --- sandbox/grist/test_useractions.py | 56 +++++++++++++++++++++++++++++ sandbox/grist/useractions.py | 60 +++++++++++++++++++++++++++---- 2 files changed, 110 insertions(+), 6 deletions(-) diff --git a/sandbox/grist/test_useractions.py b/sandbox/grist/test_useractions.py index 2238028a..c6e43645 100644 --- a/sandbox/grist/test_useractions.py +++ b/sandbox/grist/test_useractions.py @@ -536,6 +536,12 @@ class TestUserActions(test_engine.EngineTestCase): def test_section_removes(self): # Add a couple of tables and views, to trigger creation of some related items. self.init_views_sample() + self.apply_user_action(['BulkUpdateRecord', '_grist_Views', [2], { + 'layoutSpec': [ + "{\"children\":[{\"children\":[{\"leaf\":10},{\"leaf\":4}]}]," + + "\"collapsed\":[{\"leaf\":6}]}", + ] + }]) self.assertViews([ View(1, sections=[ @@ -596,6 +602,56 @@ class TestUserActions(test_engine.EngineTestCase): ]) ]) + # Check that the view layout spec also got updated. + self.assertTableData('_grist_Views', rows='subset', cols='subset', data=[ + ['id', 'layoutSpec'], + [2, "{\"children\": [{\"children\": [{\"leaf\": 4}]}], \"collapsed\": []}"], + ]) + + #---------------------------------------------------------------------- + + def test_field_removes(self): + self.init_views_sample() + self.apply_user_action(['BulkUpdateRecord', '_grist_Views_section', [1, 4, 6, 7], { + 'layoutSpec': [ + "", + "{\"children\":[{\"children\":[{\"leaf\":10},{\"leaf\":11}]},{\"leaf\":12}]}", + "{\"id\":\"d748210f-91e1-4209-8f56-c27f4f62efde\",\"type\":\"Layout\",\"children\":[{\"id" + + "\":\"a939f6b0-c728-4b36-bdde-6bb727ce4027\",\"type\":\"Paragraph\",\"text\":\"## **F" + + "orm Title**\",\"alignment\":\"center\"},{\"id\":\"428c5f4f-b1f8-4bb6-89e5-5cdab4fca5" + + "d0\",\"type\":\"Paragraph\",\"text\":\"Your form description goes here.\",\"alignmen" + + "t\":\"center\"},{\"id\":\"9bfaa427-7776-411e-9724-e351fb4d5bdf\",\"type\":\"Section" + + "\",\"children\":[{\"id\":\"8192f1c8-918f-4b4f-b51c-c775588a6087\",\"type\":\"Paragra" + + "ph\",\"text\":\"### **Header**\"},{\"id\":\"6005123f-069b-4b31-85d1-d8c677c53f7e\"," + + "\"type\":\"Paragraph\",\"text\":\"Description\"},{\"id\":\"444ffba4-5a99-4a3f-abb9-9" + + "09deea5c059\",\"type\":\"Field\",\"leaf\":16},{\"id\":\"c7deaa62-3c06-45a8-aa79-122d" + + "01c77832\",\"type\":\"Field\",\"leaf\":17},{\"id\":\"b7326be0-a73e-4cc1-8f8b-6c5033d" + + "227e4\",\"type\":\"Field\",\"leaf\":18}]},{\"id\":\"3a70d09f-2b08-4875-a813-3432d2d0" + + "ae3d\",\"type\":\"Submit\"}]}", + "invalid", + ] + }]) + + # Remove a couple of fields. Ensure the layout specs of their sections get updated. + self.apply_user_action(['BulkRemoveRecord', '_grist_Views_section_field', [1, 10, 12, 16, 19]]) + self.assertTableData('_grist_Views_section', rows='subset', cols='subset', data=[ + ['id', 'layoutSpec'], + [1, ""], + [4, "{\"children\": [{\"children\": [{\"leaf\": 11}]}]}"], + [6, "{\"children\": [{\"alignment\": \"center\", \"id\": \"a939f6b0-c728-4b36-bdde-6bb727ce4" + + "027\", \"text\": \"## **Form Title**\", \"type\": \"Paragraph\"}, {\"alignment\": \"cen" + + "ter\", \"id\": \"428c5f4f-b1f8-4bb6-89e5-5cdab4fca5d0\", \"text\": \"Your form descript" + + "ion goes here.\", \"type\": \"Paragraph\"}, {\"children\": [{\"id\": \"8192f1c8-918f-4b" + + "4f-b51c-c775588a6087\", \"text\": \"### **Header**\", \"type\": \"Paragraph\"}, {\"id\"" + + ": \"6005123f-069b-4b31-85d1-d8c677c53f7e\", \"text\": \"Description\", \"type\": \"Para" + + "graph\"}, {\"id\": \"c7deaa62-3c06-45a8-aa79-122d01c77832\", \"leaf\": 17, \"type\": \"" + + "Field\"}, {\"id\": \"b7326be0-a73e-4cc1-8f8b-6c5033d227e4\", \"leaf\": 18, \"type\": \"" + + "Field\"}], \"id\": \"9bfaa427-7776-411e-9724-e351fb4d5bdf\", \"type\": \"Section\"}, {" + + "\"id\": \"3a70d09f-2b08-4875-a813-3432d2d0ae3d\", \"type\": \"Submit\"}], \"id\": \"d74" + + "8210f-91e1-4209-8f56-c27f4f62efde\", \"type\": \"Layout\"}"], + [7, "invalid"], + ]) + #---------------------------------------------------------------------- def test_schema_consistency_check(self): diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index 19bfc1c0..eb2e4c72 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -1267,9 +1267,8 @@ class UserActions(object): # Remove all view fields for all removed columns. # Bypass the check for raw data view sections. - field_ids = [f.id for c in col_recs for f in c.viewFields] - - self.doBulkRemoveRecord("_grist_Views_section_field", field_ids) + fields = [f for c in col_recs for f in c.viewFields] + self._doRemoveViewSectionFieldRecords(fields) # If there is a displayCol, it may get auto-removed, but may first produce calc actions # triggered by the removal of this column. To avoid those, remove displayCols immediately. @@ -1353,12 +1352,20 @@ class UserActions(object): Remove view sections, including their fields, without checking for raw view sections. """ self.doBulkRemoveRecord('_grist_Views_section_field', [f.id for vs in recs for f in vs.fields]) - self.doBulkRemoveRecord('_grist_Views_section', [r.id for r in recs]) + views = {r.parentId.id: r.parentId for r in recs}.values() + rec_ids = [r.id for r in recs] + updates = _get_layout_spec_updates(views, set(rec_ids)) + if updates: + self._do_doc_action(actions.BulkUpdateRecord('_grist_Views', + [row_id for (row_id, _) in updates], + {'layoutSpec': [value for (_, value) in updates]} + )) + self.doBulkRemoveRecord('_grist_Views_section', rec_ids) @override_action('BulkRemoveRecord', '_grist_Views_section_field') def _removeViewSectionFieldRecords(self, table_id, row_ids): """ - Remove view sections, including their fields. + Remove view section fields. Raises an error if trying to remove a field of a table's rawViewSectionRef, i.e. hiding a column in a raw data widget. """ @@ -1366,7 +1373,21 @@ class UserActions(object): for rec in recs: if rec.parentId.isRaw: raise ValueError("Cannot remove raw view section field") - self.doBulkRemoveRecord(table_id, row_ids) + self._doRemoveViewSectionFieldRecords(recs) + + def _doRemoveViewSectionFieldRecords(self, recs): + """ + Remove view section fields, without checking for raw view section fields. + """ + view_sections = {r.parentId.id: r.parentId for r in recs}.values() + rec_ids = [r.id for r in recs] + updates = _get_layout_spec_updates(view_sections, rec_ids) + if updates: + self._do_doc_action(actions.BulkUpdateRecord('_grist_Views_section', + [row_id for (row_id, _) in updates], + {'layoutSpec': [value for (_, value) in updates]} + )) + self.doBulkRemoveRecord('_grist_Views_section_field', rec_ids) #---------------------------------------- # User actions on columns. @@ -2361,3 +2382,30 @@ def _is_transform_col(col_id): 'gristHelper_Transform', 'gristHelper_Converted', )) + +def _get_layout_spec_updates(resources, removed_ids): + def get_patched_layout_spec(layout_spec): + if not isinstance(layout_spec, dict): + return layout_spec + if 'leaf' in layout_spec and layout_spec['leaf'] in removed_ids_set: + return None + + patched_layout_spec = layout_spec.copy() + for key in ('children', 'collapsed'): + if key not in layout_spec or not isinstance(layout_spec[key], list): + continue + + patched_values = [get_patched_layout_spec(v) for v in layout_spec[key]] + patched_layout_spec[key] = [v for v in patched_values if v is not None] + return patched_layout_spec + + updates = [] + removed_ids_set = set(removed_ids) + for (row_id, layout_spec) in [(r.id, r.layoutSpec) for r in resources if r.layoutSpec != ""]: + try: + layout_spec_parsed = json.loads(layout_spec) + except ValueError: + continue + new_layout_spec = json.dumps(get_patched_layout_spec(layout_spec_parsed), sort_keys=True) + updates.append((row_id, new_layout_spec)) + return updates