🚧 [#5139] More WIP

Author: viktorvanwijk

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,

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",
@@ -94,47 +95,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 +135,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 +154,76 @@ 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: We can't compare `initial_data` with `data_for_evaluation` because
+    #  invalid data that is converted to python objects will be present as `None`, so
+    #  there might not be a change. If we compare `variable.initial_value` with
+    #  `variable.value`, data changes for invalid data are detected, as the invalid
+    #  value is saved to `variable.value` anyway. This check also doesn't work, as
+    #  variable.value is updated with python objects, and initial value is not
+    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
             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.

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.
 
@@ -147,8 +148,5 @@ 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
+                yield operation, mutations

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:

src/openforms/submissions/tests/form_logic/test_submission_logic.py

@@ -1121,6 +1121,51 @@ def test_component_value_set_to_today(self):
         new_value = response.json()["step"]["data"]["date"]
         self.assertEqual(new_value, "2024-03-18")
 
+    def test_asdf(self):
+        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()
+
+        self.assertNotIn("date", data["step"]["data"])
+
 
 def is_valid_expression(expr: dict):
     try: