Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace DataContainer with FormioData in evaluate_form_logic #5139

Open
viktorvanwijk opened this issue Mar 6, 2025 · 2 comments · May be fixed by #5141
Open

Replace DataContainer with FormioData in evaluate_form_logic #5139

viktorvanwijk opened this issue Mar 6, 2025 · 2 comments · May be fixed by #5141

Comments

@viktorvanwijk
Copy link
Contributor

viktorvanwijk commented Mar 6, 2025

This simplifies the data structures. Things to investigate/consider:

  • DataContainer.data returns the python objects of the variables. Can iter_evaluate_rules work without these objects?

  • Can we track all of the variable changes based on the returned mutations of all evaluated actions? (Some context: AcitonOperation.eval returns a data mapping of the applied changes.)

  • Do we want to refactor the following functions to rely on SubmissionValueVariablesState or FormioData? It depends on whether they can work without the python objects of the variables (that is what DataContainer.data currently returns):

    • get_dynamic_configuration (TODO in code "refactor this to rely on SubmissionValueVariablesState")
    • FormioConfigurationWrapper.is_visible_in_frontend
    • inject_variables
@viktorvanwijk viktorvanwijk self-assigned this Mar 6, 2025
@viktorvanwijk viktorvanwijk moved this from Todo to In Progress in Development Mar 6, 2025
viktorvanwijk added a commit that referenced this issue Mar 6, 2025
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.
@viktorvanwijk viktorvanwijk linked a pull request Mar 6, 2025 that will close this issue
10 tasks
@viktorvanwijk
Copy link
Contributor Author

viktorvanwijk commented Mar 6, 2025

Investigation of

DataContainer.data returns the python objects of the variables. Can iter_evaluate_rules work without these objects?

No it cannot (see changes in e5db653.). Test failures, for example in SubmissionReportGenerationTests.test_date_object_is_converted_to_str_when_it_comes_from_logic_rule, the submitted value for date is not converted to a datetime python object, so the action where a month is added to this date fails in jsonLogic as it tries to add a month to a plain string.

Initializing a FormioData instance with all data converted to a python object does pass all current tests. In this case, the line,

data_for_logic = FormioData(data_container.data_without_to_python)

is replaced by

data_for_logic = FormioData(data_container.data)

However, it will not work for date/datetime fields that are changed in multiple logic actions (see added test VariableModificationTests.test_two_actions_on_the_same_variable). For example, the first action sets a datefield to '2025-01-01' and the second action adds a month. Because the set date is not converted to a python object before the second action is evaluated, an error is raised when running the jsonLogic for the second action, and the month is not added.

We could add a routine that converts the updated values to python objects before adding them to the FormioData instance, but that might introduce mismatches between the inferred and actual data type of the variable. It seems like the only way to properly do it is through SubmissionValueVariable.to_python, which requires updating the variables state, and in turn means nothing changes from the current situation.

Moving the conversion to python objects out of SubmissionValueVariable seems like a big task as well, not sure where else to place this at the moment. Or if we even want this...

Investigation of

Can we track all of the variable changes based on the returned mutations of all actions?

This will be tricky in the case when multiple actions change one variable. For example, one action sets a value for a field, and another action changes it back to its original value (not necessarily likely to happen, but also not impossible). This means it was changed twice during logic evaluation and will therefore show up in the mutations, but overall the value hasn't changed. Therefore, we would have to compare it to the original values anyway.

Investigation of

Do we want to refactor the get_dynamic_configuration to rely on SubmissionValueVariablesState or FormioData?

Some good news, time/date-related config modifications do work with strings (see DynamicDateConfigurationTests.test_variable_is_string_serialized_date), which means passing a FormioData instance without the to-python conversion of variables will work. For now, anyway. Tested with the changes in 926a08c. The reason this works is because there is another conversion happening in openforms.formio.dynamic_config.data.convert_to_python_type.

Investigation of

Do we want to refactor FormioConfigurationWrapper.is_visible_in_frontend to rely on SubmissionValueVariablesState or FormioData?

The check whether a component is visible in the frontend seems to work better without the python objects actually. Take for example a simple conditional of show component B when date field A has the value '2025-01-01'. Currently, the value entered in the date field is converted to a date object, and used by is_visible_in_frontend. The values in the conditional, however, are not. Thus the comparison fails (see ComponentModificationTests.test_component_visible_with_date in ca656a6). If we were to pass the raw data to is_visible_in_frontend, this test would pass. The question is though, are these simple conditionals even used? Seems like just an even simpler version of a simple logic rule.

EDIT: they seem to be used for showing/hiding fields within edit grids (and possibly field sets). Currently, there is no access to fields within edit grids or field sets from the simple logic rules. Not sure if it is possible to access them in a advanced logic rule

Investigation of

Do we want to refactor inject_variables to rely on SubmissionValueVariablesState or FormioData?

This injects the variable values into the formio configration to perform template rendering of component properties. It requires python objects for all variables, so it has to use SubmissionValueVariable.to_python

@viktorvanwijk
Copy link
Contributor Author

viktorvanwijk commented Mar 10, 2025

Discussion with Sergei on 10-03-2025:
Since we cannot perform (all of) the investigated actions with JSON values, we want to make sure evaluate_form_logic reasons in native python objects only. So the data flow will be as follows:

  • Convert input data (JSON values) to python objects
  • Evaluate logic rules with these python objects
  • Values that are changed because of the logic actions need to be converted to python objects directly after they are calculated (in iter_evaluate_rules, or first in VariableAction.eval as a proof of concept)
  • Perform other necessary operations: handling of clearOnHide, inject variables in configuration wrapper, and creating a data difference
  • Return data difference as JSON values

Something to think about: what should happen when iterating over a FormioData instance: flat-key structure, nested-key structure, or another FormioData instance for nested structures?

viktorvanwijk added a commit that referenced this issue Mar 10, 2025
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.
viktorvanwijk added a commit that referenced this issue Mar 11, 2025
viktorvanwijk added a commit that referenced this issue Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

2 participants