From 8ee23f53440e5360f45ca0f200b29e30e0a620d7 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Mon, 23 May 2022 19:36:21 +0200 Subject: [PATCH] (core) Mark column changing actions as indirect when adding data to empty column Summary: Fixing https://gristlabs.getgrist.com/doc/check-ins/p/12#a1.s19.r1045.c19 : > Problem: user creates fresh new empty column. Users with access to write to that column, but not modify schema, will not in fact be able to write into it (since on first data entry column type needs to change). Experience is confusing. Refactored `enter_indirection` and `leave_indirection` to a single context manager method for use with `with` instead of `try/finally`. Used the new method in `_ensure_column_accepts_data` around column changing actions converting empty column to data column. Test Plan: Updated a Python test, reflecting that the correct actions are now marked as direct=False. Tested manually that I can now add data to a blank column without schema access, while I wasn't able to before, and I still can't make other schema changes. Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D3446 --- sandbox/grist/table.py | 15 +++------ sandbox/grist/test_formula_undo.py | 2 +- sandbox/grist/useractions.py | 51 +++++++++++++++++------------- 3 files changed, 35 insertions(+), 33 deletions(-) diff --git a/sandbox/grist/table.py b/sandbox/grist/table.py index 3cfca9ef..c1ab071e 100644 --- a/sandbox/grist/table.py +++ b/sandbox/grist/table.py @@ -324,12 +324,10 @@ class Table(object): if summary_table._summary_simple: @usertypes.formulaType(usertypes.Reference(summary_table.table_id)) def _updateSummary(rec, table): # pylint: disable=unused-argument - try: - # summary table output should be treated as we treat formula columns, for acl purposes - self._engine.user_actions.enter_indirection() + # summary table output should be treated as we treat formula columns, for acl purposes + with self._engine.user_actions.indirect_actions(): return summary_table.lookupOrAddDerived(**{c: getattr(rec, c) for c in groupby_cols}) - finally: - self._engine.user_actions.leave_indirection() + else: @usertypes.formulaType(usertypes.ReferenceList(summary_table.table_id)) def _updateSummary(rec, table): # pylint: disable=unused-argument @@ -368,14 +366,11 @@ class Table(object): new_row_ids.append(None) if new_row_ids and not self._engine.is_triggered_by_table_action(summary_table.table_id): - try: - # summary table output should be treated as we treat formula columns, for acl purposes - self._engine.user_actions.enter_indirection() + # summary table output should be treated as we treat formula columns, for acl purposes + with self._engine.user_actions.indirect_actions(): result += self._engine.user_actions.BulkAddRecord( summary_table.table_id, new_row_ids, values_to_add ) - finally: - self._engine.user_actions.leave_indirection() return result diff --git a/sandbox/grist/test_formula_undo.py b/sandbox/grist/test_formula_undo.py index 540d08e6..b3648d62 100644 --- a/sandbox/grist/test_formula_undo.py +++ b/sandbox/grist/test_formula_undo.py @@ -119,7 +119,7 @@ return '#%s %s' % (table.my_counter, $schoolName) ["UpdateRecord", "_grist_Tables_column", 22, {"isFormula": False}], ["UpdateRecord", "Students", 6, {"newCol": "Boo!"}], ], - "direct": [True, True, True, False, True, True], + "direct": [False, False, False, False, False, True], "undo": [ ["ModifyColumn", "Students", "newCol", {"type": "Any"}], ["UpdateRecord", "_grist_Tables_column", 22, {"type": "Any"}], diff --git a/sandbox/grist/useractions.py b/sandbox/grist/useractions.py index e05cb2bc..46d8f888 100644 --- a/sandbox/grist/useractions.py +++ b/sandbox/grist/useractions.py @@ -3,6 +3,7 @@ from collections import namedtuple, Counter, OrderedDict import re import json import sys +from contextlib import contextmanager import six from six.moves import xrange @@ -221,19 +222,22 @@ class UserActions(object): self._overrides = {key: method.__get__(self, UserActions) for key, method in six.iteritems(_action_method_overrides)} - def enter_indirection(self): + @contextmanager + def indirect_actions(self): """ - Mark any actions following this call as being indirect, until leave_indirection is - called. Nesting is supported (but not used). - """ - self._indirection_level += 1 + Usage: - def leave_indirection(self): - """ - Undo an enter_indirection. + with self.indirect_actions(): + # apply actions here + + This marks those actions as being indirect, for ACL purposes. """ - self._indirection_level -= 1 - assert self._indirection_level >= 0 + try: + self._indirection_level += 1 + yield + finally: + self._indirection_level -= 1 + assert self._indirection_level >= 0 def _do_doc_action(self, action): if hasattr(action, 'simplify'): @@ -862,18 +866,21 @@ class UserActions(object): raise ValueError("Can't save value to formula column %s" % col_id) # An empty column (isFormula=True, formula=""), now is the time to convert it to data. - if schema_col.type == 'Any': - # Guess the type when it starts out as Any. We unfortunately need to update the column - # separately for type conversion, to recompute type-specific defaults - # before they are used in formula->data conversion. - col_info, values = guess_col_info(values) - # If the values are all blank (None or empty string) leave the column empty - if not col_info: - return values - col_rec = self._docmodel.get_column_rec(table_id, col_id) - self._docmodel.update([col_rec], **col_info) - self.ModifyColumn(table_id, col_id, {'isFormula': False}) - return values + # Since the user is merely adding/updating plain records, they shouldn't be blocked by + # ACL rules preventing schema changes, i.e. these column changing actions are not direct. + with self.indirect_actions(): + if schema_col.type == 'Any': + # Guess the type when it starts out as Any. We unfortunately need to update the column + # separately for type conversion, to recompute type-specific defaults + # before they are used in formula->data conversion. + col_info, values = guess_col_info(values) + # If the values are all blank (None or empty string) leave the column empty + if not col_info: + return values + col_rec = self._docmodel.get_column_rec(table_id, col_id) + self._docmodel.update([col_rec], **col_info) + self.ModifyColumn(table_id, col_id, {'isFormula': False}) + return values @useraction def AddOrUpdateRecord(self, table_id, require, col_values, options):