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

FIO-9255: fixed an issue where nested forms lose data after submission if some parent has conditional components #180

Conversation

TanyaGashtold
Copy link
Contributor

@TanyaGashtold TanyaGashtold commented Oct 28, 2024

Link to Jira Ticket

https://formio.atlassian.net/browse/FIO-9255

Description

What changed?

The getComponent inside the simple conditional check overrides the component path that is initially set inside eachComponentData. That courses the nested forms components to have paths related to main form (not the local root how it is expected). That made the filters incorrectly set data into filtered submission object. This PR made so that getComponent works clean and did not change component objects as the only purpose of the getComponent is to return the requested component.

How has this PR been tested?

Manually + tests

Checklist:

  • I have completed the above PR template
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • My changes generate no new warnings
  • My changes include tests that prove my fix is effective (or that my feature works as intended)
  • New and existing unit/integration tests pass locally with my changes
  • Any dependent changes have corresponding PRs that are listed above

Copy link
Contributor

@brendanbond brendanbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not immediately obvious to me what runClean means - my understanding of this issue is that the pathing used by getComponent is incorrect, is there an opportunity to just improve the overall pathing for nested forms? I need to discuss this ticket with CSRs anyway, so I'm moving it to Needs Review for now.

@travist
Copy link
Member

travist commented Oct 29, 2024

We will not be merging this pull request since it seems you are adding a lot of "flags" to get around the issue where you are expecting a "full path" when you are only getting a "local path" inside of the nested form. For any case where this is true, you should be wrapping the "local path" with the function called "getComponentAbsolutePath". This function will take the local path of a component (provided within an eachComponentData iteration inside of a nested form), and then give you the full absolute path of that component, which makes sense to allow "getComponent" to work by providing the absolute path.

https://github.com/formio/core/blob/master/src/utils/formUtil/index.ts#L190

You may also take a look at the work done here #162. It seems this was resolving a ticket where a "similar" issue was found with the paths within Nested Forms.

@TanyaGashtold
Copy link
Contributor Author

TanyaGashtold commented Oct 30, 2024

@brendanbond , @travist , let me explain what is happening in my use case.
It is important to keep in mind the following statements:

  • the 'eachComponent' adds an absolute path (that is relative to the main root) to each component;
  • the 'eachComponentData' adds a relative path (that is relative to the local root) to each component;
  • the 'getComponent' uses the 'eachComponent' inside, that means that the absolute path will be assigned to each component;
  • the core processors expect that the components and their parents have a relative path. If some processor needs absolute path, it calls getComponentAbsolutePath for the component.

General description of the bug:

When the process is started, it iterates through the components using 'eachComponentData' and a relative path is added to each component.
Each component goes through the filter process and later goes to the simple conditions process. The filter process calls getComponentAbsolutePath and, using the absolute path, adds components values to scope.filter. If some component inside deeply nested form (level 2 ,3) has simple conditionals, it goes to checkSimpleConditional.
While checking the condition, the getComponent is called where the form components are passed as an argument. Initially the form components have relatives paths, but inside the getComponent all paths will be overridden by the absolute ones. That means that components, that go after the component with a simple condition in 'eachComponentData' iteration, will have parents with absolute path. The getComponentAbsolutePath will return wrong absolute path for such components. So, wrong absolute paths inside scope.filter causes wrong final data abject as those paths are used for setting values in final filtered data.

Real example:

Lets imagine we have the following form structure:

                             Form 1 (main form)
                                        |
                          Form 2 (nested in form 1, key - nested2)
                                        |
                          Form 3 (nested in form 2, key - nested3)
                         /                                                       \
        Form 4 (nested in form 3, key - nested4)           Form 5 (nested in form 3, key - nested5)

Form 4 components:

  • selectForm4
  • textFieldForm4 (with condition!!!: show when selectForm4 is a)
  • textAreaForm4 (with condition!!!: show when selectForm4 is b)

Form 5 components:

  • numberForm5

Lets imagine we submit this form with the following values:
selectForm4: 'b'
textFieldForm4 - no value as conditionally hidden
textAreaForm4: 'test4'
numberForm5: 5555

The eachComponentData iterates the Form1 components in the following order:

(1) nested2 - (2) nested3 - (3) nested4 - (4) nested4.selectForm4 - (5) nested4.textFieldForm4 - (6) nested4.textAreaForm4 - (7) nested5 - (8) nested5.numberForm5.

Everything works as expected until iteration (5). The nested4.textFieldForm4 has a simple condition. When checking the condition, getComponent overrides paths for all form components to the absolute one. When nested4.textAreaForm4 (6) is processed, its parent components have absolute path (not the relative one as expected). Because of it, getComponentAbsolutePath calculates the absolute path wrongly inside filter process (as "nested2.data.nested3.data.nested2.data.nested3.data.nested4.data.textAreaForm4"). The right path should look like this: "nested2.data.nested3.data.nested4.data.textAreaForm4".
pathCalculation

Similarly, wrong absolute paths will be calculated for components from iteration 6-8 inside the filter process that will cause incorrect final data object structure.
filter
As a result, components nested4.textAreaForm4 and nested5.numberForm5 will not have values when viewing submission.

Solution:

The root of the problem is that getComponent inside checkSimpleConditional overrides the relative paths by absolute paths that leads to the paths calculation issues inside other processors. In general, even not noting this issue, getComponent should just return a requested component and not create any side effects. In other words, it must be a clean function, that is function that does not change any input parameters.
This PR adds an additional optional parameter to eachComponent called 'runClean' and sets it to true inside getComponent. If 'runClean' is true, eachComponent will not add any properties to a component object (like path or parent), it will be running in a clean/pure way. This prevents paths overriding and fixes the issue.

Let me know if you need any additional explanations.

@lane-formio lane-formio merged commit 87688e6 into master Nov 1, 2024
8 checks passed
lane-formio added a commit that referenced this pull request Nov 1, 2024
…data-if-some-component-are-conditional

FIO-9255: fixed an issue where nested forms lose data after submission if some parent has conditional components
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants