From 6050a456b2dd5224a9e5cab91931bd19e068dd26 Mon Sep 17 00:00:00 2001 From: Viktor van Wijk Date: Thu, 6 Mar 2025 14:48:50 +0100 Subject: [PATCH 1/4] :construction: [#5139] Remove DataContainer usage from iter_evaluate_rules It's a quick test to see what happens if we were to remove the DataContainer from iter_evaluate_rules, and replace it with a FormioData instance. Also added support for FormioData in recursive_apply to fix several failing tests. test_two_actions_on_the_same_variable illustrates what happens when two actions are performed on the same variable. --- src/openforms/formio/datastructures.py | 5 ++ src/openforms/formio/utils.py | 3 +- src/openforms/submissions/form_logic.py | 34 ++++++++---- src/openforms/submissions/logic/actions.py | 6 ++- .../submissions/logic/datastructures.py | 11 ++++ src/openforms/submissions/logic/rules.py | 11 ++-- .../models/submission_value_variable.py | 53 +++++++++++++------ .../tests/form_logic/test_modify_variables.py | 44 +++++++++++++++ 8 files changed, 135 insertions(+), 32 deletions(-) diff --git a/src/openforms/formio/datastructures.py b/src/openforms/formio/datastructures.py index 8cc32973a6..86a27a3101 100644 --- a/src/openforms/formio/datastructures.py +++ b/src/openforms/formio/datastructures.py @@ -162,12 +162,14 @@ class FormioData(UserDict): data: dict[str, JSONValue] _keys: set[str] + updates: dict[str, JSONValue] """ A collection of flattened key names, for quicker __contains__ access """ def __init__(self, *args, **kwargs): self._keys = set() + self.updates = {} super().__init__(*args, **kwargs) def __getitem__(self, key: str): @@ -206,3 +208,6 @@ def __contains__(self, key: object) -> bool: return True except KeyError: return False + + def track_updates(self, m: dict[str, JSONValue]) -> None: + self.updates.update(m) diff --git a/src/openforms/formio/utils.py b/src/openforms/formio/utils.py index 97e22a3840..fc55ce6293 100644 --- a/src/openforms/formio/utils.py +++ b/src/openforms/formio/utils.py @@ -447,6 +447,7 @@ def recursive_apply( JSON serialization unless transform_leaf flag is set to True where func is applied to the nested value as well. """ + from openforms.formio.datastructures import FormioData match input: # string primitive - we can throw it into the template engine case str(): @@ -460,7 +461,7 @@ def recursive_apply( ] # mapping - map every key/value pair recursively - case dict(): + case dict() | FormioData(): return { key: recursive_apply(nested_bit, func, transform_leaf, *args, **kwargs) for key, nested_bit in input.items() diff --git a/src/openforms/submissions/form_logic.py b/src/openforms/submissions/form_logic.py index f48c999c9f..3aa0e95197 100644 --- a/src/openforms/submissions/form_logic.py +++ b/src/openforms/submissions/form_logic.py @@ -21,6 +21,7 @@ from .models import Submission, SubmissionStep +# TODO-5139: replace `data` type with `FormioData`? @elasticapm.capture_span(span_type="app.submissions.logic") def evaluate_form_logic( submission: "Submission", @@ -84,7 +85,8 @@ def evaluate_form_logic( submission_variables_state.set_values(data) rules = get_rules_to_evaluate(submission, step) - data_container = DataContainer(state=submission_variables_state) + initial_data = submission_variables_state.to_python() + data_for_evaluation = submission_variables_state.to_python() # 5. Evaluate the logic rules in order mutation_operations = [] @@ -97,16 +99,30 @@ def evaluate_form_logic( ): for operation in iter_evaluate_rules( rules, - data_container, + data_for_evaluation, submission=submission, ): mutation_operations.append(operation) + submission_variables_state.set_values(data_for_evaluation.updates) + # 6. The variable state is now completely resolved - we can start processing the # dynamic configuration and side effects. # Calculate which data has changed from the initial, for the step. - updated_step_data = data_container.get_updated_step_data(step) + # TODO-5139: this should ideally be built up from mutation returns inside + # iter_evaluate_rules + relevant_variables = submission_variables_state.get_variables_in_submission_step( + step, include_unsaved=True + ) + + updated_step_data = FormioData() + for key, variable in relevant_variables.items(): + if ( + not variable.form_variable + or variable.value != variable.form_variable.initial_value + ): + updated_step_data[key] = variable.value step.data = DirtyData(updated_step_data.data) # 7. finally, apply the dynamic configuration @@ -129,8 +145,6 @@ def evaluate_form_logic( for mutation in mutation_operations: mutation.apply(step, config_wrapper) - initial_data = FormioData(data_container.initial_data) - # XXX: See #2340 and #2409 - we need to clear the values of components that are # (eventually) hidden BEFORE we do any further processing. This is only a bandaid # fix, as the (stale) data has potentially been input for other logic rules. @@ -155,11 +169,11 @@ def evaluate_form_logic( continue # clear the value - data_container.update({key: empty_value}) + data_for_evaluation.update({key: empty_value}) data_diff[key] = empty_value # 7.2 Interpolate the component configuration with the variables. - inject_variables(config_wrapper, data_container.data) + inject_variables(config_wrapper, data_for_evaluation.data) # 7.3 Handle custom formio types - TODO: this needs to be lifted out of # :func:`get_dynamic_configuration` so that it can use variables. @@ -206,12 +220,14 @@ def check_submission_logic( submission_variables_state = submission.load_submission_value_variables_state() if unsaved_data: submission_variables_state.set_values(unsaved_data) - data_container = DataContainer(state=submission_variables_state) + data_for_evaluation = submission_variables_state.to_python() mutation_operations: list[ActionOperation] = [] - for operation in iter_evaluate_rules(rules, data_container, submission): + for operation in iter_evaluate_rules(rules, data_for_evaluation, submission): mutation_operations.append(operation) + submission_variables_state.set_values(data_for_evaluation.updates) + # we loop over all steps because we have validations that ensure unique component # keys across multiple steps for the whole form. # diff --git a/src/openforms/submissions/logic/actions.py b/src/openforms/submissions/logic/actions.py index 97fca88b1b..c097f09256 100644 --- a/src/openforms/submissions/logic/actions.py +++ b/src/openforms/submissions/logic/actions.py @@ -175,8 +175,12 @@ def eval( context: DataMapping, submission: Submission, ) -> DataMapping: + state = submission.load_submission_value_variables_state() + value_variable = state.variables[self.variable] with log_errors(self.value, self.rule): - return {self.variable: jsonLogic(self.value, context)} + result = jsonLogic(self.value, context) + value_variable.to_python(result) + return {self.variable: value_variable.to_python(result)} @dataclass diff --git a/src/openforms/submissions/logic/datastructures.py b/src/openforms/submissions/logic/datastructures.py index 3e8ab18e7a..61ef7ffe74 100644 --- a/src/openforms/submissions/logic/datastructures.py +++ b/src/openforms/submissions/logic/datastructures.py @@ -37,6 +37,8 @@ def data(self) -> DataMapping: :return: A datamapping (key: variable key, value: variable value) ready for (template context) evaluation. """ + # TODO: this .to_python is needed for conversion of date/datetimes. Not sure yet + # if logic evaluation uses this in any way, though. Maybe we can get rid of it. dynamic_values = { key: variable.to_python() for key, variable in self.state.variables.items() } @@ -44,6 +46,15 @@ def data(self) -> DataMapping: nested_data = FormioData({**dynamic_values, **static_values}) return nested_data.data + @property + def data_without_to_python(self): + dynamic_values = { + key: variable.value for key, variable in self.state.variables.items() + } + static_values = self.state.static_data() + nested_data = FormioData({**dynamic_values, **static_values}) + return nested_data.data + def update(self, updates: DataMapping) -> None: """ Update the dynamic data state. diff --git a/src/openforms/submissions/logic/rules.py b/src/openforms/submissions/logic/rules.py index ba38d049f7..3a5e9bd3fd 100644 --- a/src/openforms/submissions/logic/rules.py +++ b/src/openforms/submissions/logic/rules.py @@ -3,11 +3,11 @@ import elasticapm from json_logic import jsonLogic +from openforms.formio.datastructures import FormioData from openforms.forms.models import FormLogic, FormStep from ..models import Submission, SubmissionStep from .actions import ActionOperation -from .datastructures import DataContainer from .log_utils import log_errors @@ -108,7 +108,7 @@ def get_current_step(submission: Submission) -> SubmissionStep | None: def iter_evaluate_rules( rules: Iterable[FormLogic], - data_container: DataContainer, + data_container: FormioData, submission: Submission, ) -> Iterator[ActionOperation]: """ @@ -137,7 +137,7 @@ def iter_evaluate_rules( triggered = False with log_errors(rule.json_logic_trigger, rule): triggered = bool( - jsonLogic(rule.json_logic_trigger, data_container.data) + jsonLogic(rule.json_logic_trigger, data_container) ) if not triggered: @@ -145,7 +145,10 @@ def iter_evaluate_rules( for operation in rule.action_operations: if mutations := operation.eval( - data_container.data, submission=submission + data_container, submission=submission ): + # TODO-5139: not sure if FormioData should track the changes, seems + # like a task for iter_evaluate_rules itself. data_container.update(mutations) + data_container.track_updates(mutations) yield operation diff --git a/src/openforms/submissions/models/submission_value_variable.py b/src/openforms/submissions/models/submission_value_variable.py index 23686bfcf2..4342b2c247 100644 --- a/src/openforms/submissions/models/submission_value_variable.py +++ b/src/openforms/submissions/models/submission_value_variable.py @@ -14,7 +14,7 @@ from openforms.formio.service import FormioData from openforms.forms.models.form_variable import FormVariable -from openforms.typing import DataMapping, JSONEncodable, JSONObject, JSONSerializable +from openforms.typing import DataMapping, JSONEncodable, JSONObject, JSONSerializable, JSONValue from openforms.utils.date import format_date_value, parse_datetime, parse_time from openforms.variables.constants import FormVariableDataTypes from openforms.variables.service import VariablesRegistry, get_static_variables @@ -253,6 +253,22 @@ def set_values(self, data: DataMapping) -> None: continue variable.value = new_value + # TODO-5139: can this be combined with SubmissionValueVariablesState.get_data? + def to_python(self) -> FormioData: + """ + Collect the total picture of data/variable values. + + The dynamic values are augmented with the static variables. + + :return: A datamapping (key: variable key, value: native python object for the + value) ready for (template context) evaluation. + """ + dynamic_values = { + key: variable.to_python() for key, variable in self.variables.items() + } + static_values = self.static_data() + return FormioData({**dynamic_values, **static_values}) + class SubmissionValueVariableManager(models.Manager): def bulk_create_or_update_from_data( @@ -361,7 +377,7 @@ class Meta: def __str__(self): return _("Submission value variable {key}").format(key=self.key) - def to_python(self) -> Any: + def to_python(self, value: JSONValue = None) -> Any: """ Deserialize the value into the appropriate python type. @@ -371,7 +387,10 @@ def to_python(self) -> Any: to correctly interpret the data. For the time being, this is not needed yet as we focus on NL first. """ - if self.value is None: + if value is None: + value = self.value + + if value is None: return None # it's possible a submission value variable exists without the form variable @@ -390,7 +409,7 @@ def to_python(self) -> Any: "submission_id": self.submission_id, }, ) - return self.value + return value # we expect JSON types to have been properly stored (and thus not as string!) data_type = self.form_variable.data_type @@ -402,18 +421,18 @@ def to_python(self) -> Any: FormVariableDataTypes.float, FormVariableDataTypes.array, ): - return self.value + return value - if self.value and data_type == FormVariableDataTypes.date: - if isinstance(self.value, date): - return self.value - formatted_date = format_date_value(self.value) + if value and data_type == FormVariableDataTypes.date: + if isinstance(value, date): + return value + formatted_date = format_date_value(value) naive_date = parse_date(formatted_date) if naive_date is not None: aware_date = timezone.make_aware(datetime.combine(naive_date, time.min)) return aware_date.date() - maybe_naive_datetime = parse_datetime(self.value) + maybe_naive_datetime = parse_datetime(value) if maybe_naive_datetime is None: return @@ -421,10 +440,10 @@ def to_python(self) -> Any: return maybe_naive_datetime.date() return timezone.make_aware(maybe_naive_datetime).date() - if self.value and data_type == FormVariableDataTypes.datetime: - if isinstance(self.value, datetime): - return self.value - maybe_naive_datetime = parse_datetime(self.value) + if value and data_type == FormVariableDataTypes.datetime: + if isinstance(value, datetime): + return value + maybe_naive_datetime = parse_datetime(value) if maybe_naive_datetime is None: return @@ -432,7 +451,7 @@ def to_python(self) -> Any: return maybe_naive_datetime return timezone.make_aware(maybe_naive_datetime) - if self.value and data_type == FormVariableDataTypes.time: - return parse_time(self.value) + if value and data_type == FormVariableDataTypes.time: + return parse_time(value) - return self.value + return value diff --git a/src/openforms/submissions/tests/form_logic/test_modify_variables.py b/src/openforms/submissions/tests/form_logic/test_modify_variables.py index 4fa0293103..d6151e75a7 100644 --- a/src/openforms/submissions/tests/form_logic/test_modify_variables.py +++ b/src/openforms/submissions/tests/form_logic/test_modify_variables.py @@ -19,6 +19,50 @@ class VariableModificationTests(TestCase): + def test_two_actions_on_the_same_variable(self): + form = FormFactory.create() + form_step = FormStepFactory.create( + form=form, + form_definition__configuration={ + "components": [ + {"type": "date", "key": "date"}, + ] + }, + ) + submission = SubmissionFactory.create(form=form) + submission_step = SubmissionStepFactory.create( + submission=submission, + form_step=form_step, + data={}, + ) + + FormLogicFactory.create( + form=form, + json_logic_trigger=True, + actions=[ + { + "variable": "date", + "action": { + "type": "variable", + "value": "2025-06-06", + }, + }, + { + "variable": "date", + "action": { + "type": "variable", + "value": {"+": [{"var": "date"}, {"duration": "P1M"}]}, + }, + } + ], + ) + + evaluate_form_logic(submission, submission_step, submission.data) + + variables_state = submission.load_submission_value_variables_state() + + self.assertEqual(str(variables_state.variables["date"].value), "2025-07-06") + def test_modify_variable_related_to_step_being_edited(self): form = FormFactory.create() step1 = FormStepFactory.create( From 2917cf369a129679ebc5d3e414a44d2a063c38df Mon Sep 17 00:00:00 2001 From: Viktor van Wijk Date: Thu, 6 Mar 2025 16:56:21 +0100 Subject: [PATCH 2/4] :construction: [#5139] Replace DataContainer with FormioData instance in get_dynamic_configuration --- src/openforms/formio/dynamic_config/date.py | 2 +- src/openforms/submissions/form_logic.py | 8 ++------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/openforms/formio/dynamic_config/date.py b/src/openforms/formio/dynamic_config/date.py index 578295a726..179e548aa4 100644 --- a/src/openforms/formio/dynamic_config/date.py +++ b/src/openforms/formio/dynamic_config/date.py @@ -114,7 +114,7 @@ def calculate_delta( ) -> datetime | None: assert config["mode"] == "relativeToVariable" - base_value = glom(data, config["variable"], default=None) + base_value = data.get(config["variable"]) if not base_value: return diff --git a/src/openforms/submissions/form_logic.py b/src/openforms/submissions/form_logic.py index 3aa0e95197..1546b4c942 100644 --- a/src/openforms/submissions/form_logic.py +++ b/src/openforms/submissions/form_logic.py @@ -132,13 +132,9 @@ def evaluate_form_logic( # TODO: refactor this to rely on variables state config_wrapper = get_dynamic_configuration( config_wrapper, - # context is expected to contain request, as it's the default behaviour with - # DRF view(set)s and serializers. - # TODO: check if we can use context["request"] rather than .get - None is not - # expected, but that currently breaks a lot of tests... - request=context.get("request"), + request=None, submission=submission, - data=data_container.data, + data=data_for_evaluation, ) # 7.1 Apply the component mutation operations From e16ae3f0c9d03cc62412083d8f921db0b4ff100c Mon Sep 17 00:00:00 2001 From: Viktor van Wijk Date: Fri, 7 Mar 2025 12:08:40 +0100 Subject: [PATCH 3/4] :construction: [#5139] Replace DataContainer with FormioData instance in is_visible_in_frontend --- src/openforms/submissions/form_logic.py | 3 +- .../form_logic/test_modify_components.py | 39 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/openforms/submissions/form_logic.py b/src/openforms/submissions/form_logic.py index 1546b4c942..ef22612195 100644 --- a/src/openforms/submissions/form_logic.py +++ b/src/openforms/submissions/form_logic.py @@ -151,7 +151,8 @@ def evaluate_form_logic( data_diff = FormioData() for component in config_wrapper: key = component["key"] - is_visible = config_wrapper.is_visible_in_frontend(key, data_container.data) + # TODO-5139: is_visible_in_frontend must accept a FormioData instance instead + is_visible = config_wrapper.is_visible_in_frontend(key, data_for_evaluation.data) if is_visible: continue diff --git a/src/openforms/submissions/tests/form_logic/test_modify_components.py b/src/openforms/submissions/tests/form_logic/test_modify_components.py index 6b5d24146c..cc452e4949 100644 --- a/src/openforms/submissions/tests/form_logic/test_modify_components.py +++ b/src/openforms/submissions/tests/form_logic/test_modify_components.py @@ -1,3 +1,5 @@ +import unittest + from django.test import TestCase, tag from openforms.forms.constants import LogicActionTypes @@ -874,6 +876,43 @@ def test_component_visible_in_frontend(self): "Some data that must not be cleared!", submission_step.data["textField"] ) + # TODO-5139: this is a bug which currently exists on master as well, and is not + # fixed with the data-structure changes, yet. + @unittest.expectedFailure + def test_component_visible_with_date(self): + form = FormFactory.create() + form_step = FormStepFactory.create( + form=form, + form_definition__configuration={ + "components": [ + { + "key": "date", + "type": "date", + }, + { + "type": "textfield", + "key": "textField", + "hidden": True, + "conditional": {"eq": "2025-01-01", "show": True, "when": "date"}, + "clearOnHide": True, + }, + ] + }, + ) + + submission = SubmissionFactory.create(form=form) + submission_step = SubmissionStepFactory.create( + submission=submission, + form_step=form_step, + data={"date": "2025-01-01", "textField": "Some data that must not be cleared!"}, + ) + + evaluate_form_logic(submission, submission_step, submission.data, dirty=True) + + self.assertEqual( + "Some data that must not be cleared!", submission_step.data["textField"] + ) + @tag("gh-1183") def test_component_incomplete_frontend_logic(self): form = FormFactory.create() From 55e813d26a320ce65d7066f216539cb764264c89 Mon Sep 17 00:00:00 2001 From: Viktor van Wijk Date: Tue, 11 Mar 2025 17:14:17 +0100 Subject: [PATCH 4/4] :construction: [#5139] More WIP --- src/openforms/formio/datastructures.py | 5 - src/openforms/formio/service.py | 1 + src/openforms/submissions/form_logic.py | 115 ++++++++++++------ src/openforms/submissions/logic/actions.py | 6 +- src/openforms/submissions/logic/rules.py | 18 ++- .../models/submission_value_variable.py | 8 +- .../tests/form_logic/test_submission_logic.py | 51 ++++++++ 7 files changed, 144 insertions(+), 60 deletions(-) diff --git a/src/openforms/formio/datastructures.py b/src/openforms/formio/datastructures.py index 86a27a3101..8cc32973a6 100644 --- a/src/openforms/formio/datastructures.py +++ b/src/openforms/formio/datastructures.py @@ -162,14 +162,12 @@ class FormioData(UserDict): data: dict[str, JSONValue] _keys: set[str] - updates: dict[str, JSONValue] """ A collection of flattened key names, for quicker __contains__ access """ def __init__(self, *args, **kwargs): self._keys = set() - self.updates = {} super().__init__(*args, **kwargs) def __getitem__(self, key: str): @@ -208,6 +206,3 @@ def __contains__(self, key: object) -> bool: return True except KeyError: return False - - def track_updates(self, m: dict[str, JSONValue]) -> None: - self.updates.update(m) diff --git a/src/openforms/formio/service.py b/src/openforms/formio/service.py index 75b0913359..996f128a90 100644 --- a/src/openforms/formio/service.py +++ b/src/openforms/formio/service.py @@ -61,6 +61,7 @@ def normalize_value_for_component(component: Component, value: Any) -> Any: return register.normalize(component, value) +# TODO-5139: remove request as an argument from this @elasticapm.capture_span(span_type="app.formio") def get_dynamic_configuration( config_wrapper: FormioConfigurationWrapper, diff --git a/src/openforms/submissions/form_logic.py b/src/openforms/submissions/form_logic.py index ef22612195..d23bec48ef 100644 --- a/src/openforms/submissions/form_logic.py +++ b/src/openforms/submissions/form_logic.py @@ -22,6 +22,7 @@ # TODO-5139: replace `data` type with `FormioData`? +# TODO-5139: replace all DataContainer type hints with FormioData @elasticapm.capture_span(span_type="app.submissions.logic") def evaluate_form_logic( submission: "Submission", @@ -80,6 +81,7 @@ def evaluate_form_logic( # 3. Load the (variables) state submission_variables_state = submission.load_submission_value_variables_state() + actual_initial_data = submission_variables_state.to_python() # 4. Apply the (dirty) data to the variable state. submission_variables_state.set_values(data) @@ -94,47 +96,36 @@ def evaluate_form_logic( # 5.1 - if the action type is to set a variable, update the variable state. This # happens inside of iter_evaluate_rules. This is the ONLY operation that is allowed # to execute while we're looping through the rules. + # TODO-5139: I first put this tracking of data difference inside FormioData, but + # that doesn't seem like it should be its responsibility. Then yielded a complete + # difference in iter_evaluate_rules, but that fails when there is no rules to be + # applied: `data_diff` would be undefined. So decided to track the difference + # outside iter_evaluate_rules, as we also track the mutation operations here. + # Still not sure about it, though + data_diff_total = FormioData() with elasticapm.capture_span( name="collect_logic_operations", span_type="app.submissions.logic" ): - for operation in iter_evaluate_rules( + for operation, mutations in iter_evaluate_rules( rules, data_for_evaluation, submission=submission, ): mutation_operations.append(operation) - - submission_variables_state.set_values(data_for_evaluation.updates) - - # 6. The variable state is now completely resolved - we can start processing the - # dynamic configuration and side effects. - - # Calculate which data has changed from the initial, for the step. - # TODO-5139: this should ideally be built up from mutation returns inside - # iter_evaluate_rules - relevant_variables = submission_variables_state.get_variables_in_submission_step( - step, include_unsaved=True - ) - - updated_step_data = FormioData() - for key, variable in relevant_variables.items(): - if ( - not variable.form_variable - or variable.value != variable.form_variable.initial_value - ): - updated_step_data[key] = variable.value - step.data = DirtyData(updated_step_data.data) + if mutations: + data_diff_total.update(mutations) # 7. finally, apply the dynamic configuration # we need to apply the context-specific configurations before we can apply # mutations based on logic, which is then in turn passed to the serializer(s) - # TODO: refactor this to rely on variables state + # TODO-5139: rewrite `get_dynamic_configuration` and its callees to work with + # FormioData instance config_wrapper = get_dynamic_configuration( config_wrapper, request=None, submission=submission, - data=data_for_evaluation, + data=data_for_evaluation.data, ) # 7.1 Apply the component mutation operations @@ -145,18 +136,16 @@ def evaluate_form_logic( # (eventually) hidden BEFORE we do any further processing. This is only a bandaid # fix, as the (stale) data has potentially been input for other logic rules. # Note that only the dirty data logic check acts on these differences. - - # only keep the changes in the data, so that old values do not overwrite otherwise - # debounced client-side data changes - data_diff = FormioData() for component in config_wrapper: key = component["key"] - # TODO-5139: is_visible_in_frontend must accept a FormioData instance instead + # TODO-5139: rewrite `is_visible_in_frontend` and its callees to work with + # FormioData instance is_visible = config_wrapper.is_visible_in_frontend(key, data_for_evaluation.data) if is_visible: continue - # Reset the value of any field that may have become hidden again after evaluating the logic + # Reset the value of any field that may have become hidden again after + # evaluating the logic original_value = initial_data.get(key, empty) empty_value = get_component_empty_value(component) if original_value is empty or original_value == empty_value: @@ -166,32 +155,75 @@ def evaluate_form_logic( continue # clear the value - data_for_evaluation.update({key: empty_value}) - data_diff[key] = empty_value + data_for_evaluation[key] = empty_value + data_diff_total[key] = empty_value # 7.2 Interpolate the component configuration with the variables. + # TODO-5139: rewrite `inject_variables` and its callees to work with FormioData + # instance inject_variables(config_wrapper, data_for_evaluation.data) - # 7.3 Handle custom formio types - TODO: this needs to be lifted out of - # :func:`get_dynamic_configuration` so that it can use variables. + + # --------------------------------------------------------------------------------------- + + + # 8. All the processing is now complete, so we can update the state. + # TODO-5139: when setting a value to SubmissionValueVariable.value, it doesn't + # automatically get encoded to JSON with the encoder. Probably only happens when + # saving the model. This means we have to do it manually here or in + # SubmissionValueVariablesState.set_values. Or do we want to always convert to + # python objects, like already suggested in + # SubmissionValueVariableState.set_values? Issue 2324 is related to this + submission_variables_state.set_values(data_diff_total) + + # 8.1 build a difference in data for the step. It is important that we only keep the + # changes in the data, so that old values do not overwrite otherwise debounced + # client-side data changes + + # Calculate which data has changed from the initial, for the step. + relevant_variables = submission_variables_state.get_variables_in_submission_step( + step, include_unsaved=True + ) + + # TODO-5139: the problem is that currently `variable.value` can be a json or python + # object, as `iter_evaluate_rules` now returns python objects. This means that a + # comparison with `initial_value` will fail, because it's a json value. We could + # perform this comparison in the python-type domain, but that would mean that + # invalid data will not be in step.data. Is this a bad thing? + updated_step_data = FormioData() + for key, variable in relevant_variables.items(): + if ( + not variable.form_variable + or variable.value != variable.form_variable.initial_value + ): + updated_step_data[key] = variable.value + step.data = DirtyData(updated_step_data.data) # process the output for logic checks with dirty data if dirty: - # Iterate over all components instead of `step.data`, to take hidden fields into account (See: #1755) + data_diff_dirty = FormioData() + # Iterate over all components instead of `step.data`, to take hidden fields into + # account (See: #1755) for component in config_wrapper: key = component["key"] # already processed, don't process it again - if data_diff.get(key, default=empty) is not empty: + if data_diff_dirty.get(key, default=empty) is not empty: continue + # TODO-5139: currently, this might return a python object (because we pass + # python objects to the SubmissionValueVariables.set_values) whereas + # previously it would return a json value. Not sure if this breaks + # anything... new_value = updated_step_data.get(key, default=empty) + # TODO-5139: previously these were python objects, and they still are, so + # that's ok. Or is it? original_value = initial_data.get(key, default=empty) if new_value is empty or new_value == original_value: continue - data_diff[key] = new_value + data_diff_dirty[key] = new_value # only return the 'overrides' - step.data = DirtyData(data_diff.data) + step.data = DirtyData(data_diff_dirty.data) step._form_logic_evaluated = True @@ -220,10 +252,13 @@ def check_submission_logic( data_for_evaluation = submission_variables_state.to_python() mutation_operations: list[ActionOperation] = [] - for operation in iter_evaluate_rules(rules, data_for_evaluation, submission): + data_diff = FormioData({}) + for operation, mutations in iter_evaluate_rules(rules, data_for_evaluation, submission): mutation_operations.append(operation) + if mutations: + data_diff.update(mutations) - submission_variables_state.set_values(data_for_evaluation.updates) + submission_variables_state.set_values(data_diff) # we loop over all steps because we have validations that ensure unique component # keys across multiple steps for the whole form. diff --git a/src/openforms/submissions/logic/actions.py b/src/openforms/submissions/logic/actions.py index c097f09256..97fca88b1b 100644 --- a/src/openforms/submissions/logic/actions.py +++ b/src/openforms/submissions/logic/actions.py @@ -175,12 +175,8 @@ def eval( context: DataMapping, submission: Submission, ) -> DataMapping: - state = submission.load_submission_value_variables_state() - value_variable = state.variables[self.variable] with log_errors(self.value, self.rule): - result = jsonLogic(self.value, context) - value_variable.to_python(result) - return {self.variable: value_variable.to_python(result)} + return {self.variable: jsonLogic(self.value, context)} @dataclass diff --git a/src/openforms/submissions/logic/rules.py b/src/openforms/submissions/logic/rules.py index 3a5e9bd3fd..207a1ec45f 100644 --- a/src/openforms/submissions/logic/rules.py +++ b/src/openforms/submissions/logic/rules.py @@ -1,6 +1,7 @@ from typing import Iterable, Iterator import elasticapm +from glom import mutation from json_logic import jsonLogic from openforms.formio.datastructures import FormioData @@ -110,7 +111,7 @@ def iter_evaluate_rules( rules: Iterable[FormLogic], data_container: FormioData, submission: Submission, -) -> Iterator[ActionOperation]: +) -> Iterator[tuple[ActionOperation, dict]]: """ Iterate over the rules and evaluate the trigger, yielding action operations. @@ -128,6 +129,7 @@ def iter_evaluate_rules( sole argument. Useful to gather metadata about rule evaluation. :returns: An iterator yielding :class:`ActionOperation` instances. """ + state = submission.load_submission_value_variables_state() for rule in rules: with elasticapm.capture_span( "evaluate_rule", @@ -147,8 +149,12 @@ def iter_evaluate_rules( if mutations := operation.eval( data_container, submission=submission ): - # TODO-5139: not sure if FormioData should track the changes, seems - # like a task for iter_evaluate_rules itself. - data_container.update(mutations) - data_container.track_updates(mutations) - yield operation + mutations_python = { + key: state.variables[key].to_python(value) + for key, value in mutations.items() + } + data_container.update(mutations_python) + else: + mutations_python = {} + + yield operation, mutations_python diff --git a/src/openforms/submissions/models/submission_value_variable.py b/src/openforms/submissions/models/submission_value_variable.py index 4342b2c247..777fb07a5a 100644 --- a/src/openforms/submissions/models/submission_value_variable.py +++ b/src/openforms/submissions/models/submission_value_variable.py @@ -233,7 +233,7 @@ def save_prefill_data(self, data: dict[str, Any]) -> None: SubmissionValueVariable.objects.bulk_create(variables_to_create) - def set_values(self, data: DataMapping) -> None: + def set_values(self, data: FormioData | DataMapping) -> None: """ Apply the values from ``data`` to the current state of the variables. @@ -246,7 +246,7 @@ def set_values(self, data: DataMapping) -> None: .. todo:: apply variable.datatype/format to obtain python objects? This also needs to properly serialize back to JSON though! """ - formio_data = FormioData(data) + formio_data = data if isinstance(data, FormioData) else FormioData(data) for key, variable in self.variables.items(): new_value = formio_data.get(key, default=empty) if new_value is empty: @@ -377,7 +377,7 @@ class Meta: def __str__(self): return _("Submission value variable {key}").format(key=self.key) - def to_python(self, value: JSONValue = None) -> Any: + def to_python(self, value: JSONValue = empty) -> Any: """ Deserialize the value into the appropriate python type. @@ -387,7 +387,7 @@ def to_python(self, value: JSONValue = None) -> Any: to correctly interpret the data. For the time being, this is not needed yet as we focus on NL first. """ - if value is None: + if value is empty: value = self.value if value is None: diff --git a/src/openforms/submissions/tests/form_logic/test_submission_logic.py b/src/openforms/submissions/tests/form_logic/test_submission_logic.py index dfc57bb740..c71c6795ed 100644 --- a/src/openforms/submissions/tests/form_logic/test_submission_logic.py +++ b/src/openforms/submissions/tests/form_logic/test_submission_logic.py @@ -1121,6 +1121,57 @@ def test_component_value_set_to_today(self): new_value = response.json()["step"]["data"]["date"] self.assertEqual(new_value, "2024-03-18") + def test_date_field_not_present_in_step_data_when_assigned_same_value(self): + """ + Assert that the 'date' field is not present in the step data when a logic action + sets it to the same value. This illustrates the problem that occurs when setting + python objects directly to SubmissionValueVariable.value. + """ + form = FormFactory.create() + form_step = FormStepFactory.create( + form=form, + form_definition__configuration={ + "components": [ + { + "type": "date", + "key": "date", + "defaultValue": "2025-01-01" + }, + ] + }, + ) + + FormLogicFactory.create( + form=form, + json_logic_trigger=True, + actions=[ + { + "variable": "date", + "action": { + "type": "variable", + "value": "2025-01-01", + }, + } + ], + ) + + submission = SubmissionFactory.create(form=form) + + self._add_submission_to_session(submission) + logic_check_endpoint = reverse( + "api:submission-steps-logic-check", + kwargs={ + "submission_uuid": submission.uuid, + "step_uuid": form_step.uuid, + }, + ) + response = self.client.post(logic_check_endpoint, {"data": {}}) + + data = response.json() + + # The date hasn't changed, so it should not be present in the step data + self.assertNotIn("date", data["step"]["data"]) + def is_valid_expression(expr: dict): try: