Evaluate logic rules in frontend#954
Conversation
Bundle ReportChanges will increase total bundle size by 100.97kB (0.97%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: @open-formulieren/sdk-esmAssets Changed:
Files in
Files in
Files in
Files in
Files in
view changes for bundle: @open-formulieren/sdk-OpenForms-umdAssets Changed:
Files in
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #954 +/- ##
===========================================
+ Coverage 61.65% 80.86% +19.21%
===========================================
Files 238 244 +6
Lines 4825 4945 +120
Branches 762 788 +26
===========================================
+ Hits 2975 3999 +1024
+ Misses 1623 800 -823
+ Partials 227 146 -81
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
80691fc to
e641268
Compare
d1761ca to
daf7e75
Compare
e641268 to
f97d61b
Compare
eb12d01 to
fedc8e3
Compare
f97d61b to
bff7444
Compare
3a50e18 to
a6620c5
Compare
daa7de3 to
ba6f363
Compare
| // FIXME: these deep structures don't type-narrow, so we must use type guards for | ||
| // now. It requires proper backend re-design as well. |
There was a problem hiding this comment.
Requires (big) backend changes
6877ada to
f715861
Compare
9a19e26 to
f2ca967
Compare
Move json-logic related code into a subpackage, so that we have a sibling directory/package for the Open Forms logic rules that make use of it, without creating confusion what belongs where.
The backend PR with the schema/API response changes is merged, so the compatibility layer can be removed. Additionally, the properties `isApplicable` and `completed` at the submission step level are not used - the SDK looks at `Submission.steps` instead. In the future, these can be removed from the backend, see open-formulieren/open-forms#6107
…tion Update how the FormStep component handles the logic rules and dispatches input data changes. Rather than always scheduling a backend logic check, the step-specific configuration and form feature flags are now introspected to test if we can run frontend evaluation. When opting into frontend evaluation, we no longer 'schedule' a logic check, but rather immediately evaluate it, assuming that the execution speed of the logic is fast enough to not need debouncing/scheduling. Similarly to the old situation, this results in an updated components configuration and possible data diff to be passed to the renderer, which takes care of the updated form configuration and calculation results. The step applicability and whether the step can be submitted or not are also updated and reflected in the React state. TODO: * handle clearOnHide when a component becomes hidden, as this must be reflected in the `updatedData` immediately for the next logic trigger evaluation * check that the step completion state is properly tracked when actually submitting the step * cleanup TODOs in the code
Vaguely related, this also updates @utrecht/calendar-react to match the date-fns transitive dependency. Now only @utrecht/component-library-react pulls in the outdated date-fns, but it doesn't actually use it, so it *should* be excluded from our bundle.
… desktop mode The mobile mode has a collapsed progress indicator, which breaks the assertions on the applicable/not-applicable toggles and fails the tests in Chromatic. For the actual logic evaluation, the mobile/desktop viewports are not relevant, as long as the implementation executes the right state updates. According to the chromatic docs, this patch should ensure the story only gets executed in the one specified mode.
Pointed out via PR feedback, the backend logic rule evaluation (in the backend) actually ensures that every variable exists in the variables state, which is passed along to the next logic action and rule trigger evaluation. In the case of a hidden component with clear on hide, it will actually ensure that the original input data from the start of the logic evaluation is set, and if that is absent, it will use the appropriate empty value for the component type. Intuitively, we'd use clear-on-hide to remove it so that it immediately affects the next tick in the logic evaluation, but that would introduce different behaviour between backend and frontend.
…mponentEmptyValue behaviour Validated against the backend behaviour - this makes all the input component types explicit so that regressions can be prevented.
…f backend get_component_empty_value
…ar on hide during backend logic evaluation This knowingly introduces the backend bug where the component value is not properly cleared, but instead set or reset to the input data or empty value appropriate for that component. The frontend evaluation yielding the exact same result as the backend is more important than fixing the bug in one place and not the other. Also, fixing the bug can possibly break existing forms where people (unknowlingly) rely on the buggy behaviour.
…y in property action The backend does not perform a similar check - instead it calls the process visibility for itself (and child components) and passes the `parent_hidden` based on its own new state (so the outcome of the action). However, because the backend code does not actually clear values, this doesn't matter. I've verified that the output of that logic check has: * a `get_data(...)` state that includes the empty values for cleared components, together with the input data of course * a `submission_step.data` delta that is empty, i.e. -> change nothing in the input The resulting updated state/render in the frontend then ensures this converges into a stable UI state without infinite render loops. By matching that "empty value / input data value" restoring behaviour from the backend, this `hasHiddenParent` part has become irrelevant, because it was targeting the "remove the key from the data" flow. I've confirmed that the frontend implementation now also returns an empty delta for the submission step, matching the backend, and there are tests that assert on the in-between-logic-actions-or-rules data state that also match the backend.
…heck Upon navigating to a step, we must make a check logic call to get the up-to-date metadata of the steps in the submission. See 1d8f783 for details.
…entation Validated by implementing the same tests in the backend.
…d to prevent re-introducing them Yep, this turns out to be necessary...
…atch backend What a nightmare this is... The formio-renderer needs some changes to be able to emulate the behaviour of the backend so that we arrive at the same result. It's either passing this emulate flag, or re-implement the processVisibility entirely because there are subtle differences in behaviour. Roughly set, clearing the value by removing the key from the data is not an option, because the frontend falls back to either the default value or the component empty value. We also cannot apply the visibility processing only if there's a change in visibility, because the backend logic replays everything from the beginning and requires the intermediate value changes/ resets to the empty value.
1bd0583 to
b96e35a
Compare
d9e85b2 to
f628a5c
Compare
|
@viktorvanwijk I'm satisfied with my latest patches and consider them ready for review again! |
viktorvanwijk
left a comment
There was a problem hiding this comment.
Time for testing :)
Commit 5953d98 contains more than just changes for the type of jsonLogicTrigger, though. Could you extract those and combine with the last commit that fixes the bug 😇
Ah crap, rebase gone wrong. Will fix yeah! |
While an expression is typically an object, feeding any JSON into json-logic is valid and yields expected results.
…untriggered rule The process visibility must be called even when the logic rule is not triggered. The patch for that broke another thing, because the value action was not updating the value references for values to 'restore', so it was effectively forgetting the value action type side effect. Patching that then broke data diff logic, because the logicState initialValues is being updated, so the dataUpdates value would match the entry in initialValues, and that would in turn cause it to be unset again from the dataUpdates, causing the renderer to miss data updates. The data updates are now no longer done in-flight, but at the end of the logic evaluation process - we actually don't have a use for them while evaluating logic/actions so this does clean up a bit too.
f628a5c to
f129c00
Compare
(Partly) closes open-formulieren/open-forms#5962