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-8723: Clear values from submission for hidden comp with clearOnHide flag #137

Merged

Conversation

mikekotikov
Copy link
Contributor

Link to Jira Ticket

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

Description

What changed?

Added reset of component's 'hidden' property in conditionals and logic processors, for cases when array data components (like Edit Grid/Data Grid) with multiple rows were processed.
The problem: the first time 'hidden' value got set in conditionals/logic processor, it was applied to component json and it was then thought to be present on every next row component, so the conditionals/logic wasn't recalculated for them.

Dependencies

None

How has this PR been tested?

Automated test was already there, but the assertion was incorrect, so fixed it

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.

I'm confused on how this fixes the problem that the ticket describes. It seems to me that the problematic example in FIO-8723 just has two components with default values; how do we get from there to row components such as edit grid or data grid?

@mikekotikov
Copy link
Contributor Author

mikekotikov commented Sep 10, 2024

I'm confused on how this fixes the problem that the ticket describes. It seems to me that the problematic example in FIO-8723 just has two components with default values; how do we get from there to row components such as edit grid or data grid?

Sorry, wrote not good description for the ticket. So, the original problem from the ticket is fixed by adding || component.hidden check in src/process/clearHidden.ts.
And other stuff with deleting component.hidden was then added to fix the tests, since there was a wrong behavior for array data components, where in case of logic/conditions, the result of first row calculation was thought to be the same for all the next rows.

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.

Up to now, we've used the hidden component JSON property as a "source of truth" to determine whether or not a component is hidden. A component is conditionally hidden? Add the component path to the conditionallyHidden scope and set the hidden property on the component's JSON to true. A component is logically hidden? Add the component path to the conditionallyHidden scope and set the hidden property on the component's JSON to true.

My problem with using the delete operator to get around the fact that rows in nested data components need independent hidden contexts is that it essentially does an end run around this system. Rather than determining during processing that a component should be hidden or not, it always "resets" the hidden property to false, meaning that the scope will never be aware of the component despite the fact that it has technically been logically or conditionally hidden.

I think a better solution to this problem might be to rethink the way in which we evaluate for hidden. Rather than setting/unsetting the component's hidden property when we determine a component should be hidden, could we instead utilize the scope more effectively and say "if this component appears in the conditionallyHidden scope or if it is manually hidden, then we will clear the value." In other words, we're going to stop using the component's hidden property as a source of truth (and hopefully even stop setting/unsetting it) and use the scope as the source of truth, and ensure that each and every component (which would include nested rows in data grids, e.g.) is correctly added to the conditionallyHidden scope.

Feel free to ping me to discuss further.

@mikekotikov
Copy link
Contributor Author

@brendanbond refactored, removed all the places where component.hidden was mutated and my changes with deleting it

@@ -3129,7 +3129,7 @@ describe('Process Tests', () => {
processSync(context);

expect(context.data).to.deep.equal({
candidates:[{candidate:{data:{section6:{}}}}],
candidates:[{candidate:{data:{section6:{ "c":{}, "d":[]}}}}],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test appeared to be doing invalid assertion, since clearOnHide is false in this case, the fields should be left. Corrected it.

@TanyaGashtold
Copy link
Contributor

This PR fixes one more issue related to conditional components inside EditGrid: https://formio.atlassian.net/browse/FIO-8992

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.

Really nice work @mikekotikov ! I went ahead and added back in a test case (it still works) and merged master to verify that formio-server tests were passing. All looks good, thanks dude

@brendanbond brendanbond merged commit e9f5f0e into master Sep 20, 2024
8 checks passed
lane-formio pushed a commit that referenced this pull request Oct 4, 2024
…ues_for_hidden_components

FIO-8723: Clear values from submission for hidden comp with clearOnHide flag
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.

3 participants