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-8316 invalid data submitted in nested form #94

Merged
merged 8 commits into from
Jun 26, 2024

Conversation

johnformio
Copy link
Contributor

Link to Jira Ticket

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

Description

What changed?

Some changes allowed data to be saved with the submission not defined on a nested form. The current implementation sets the entire data field in the submission if it is an object. The change to fix this was it skip the component if the model type is dataObject. Child components for dataObject models are recursively added to the filter scope, allowing the child fields to be processed correctly.

Code coverage was added with nyc / istanbul.js

Breaking Changes / Backwards Compatibility

No breaking changes

Dependencies

None

How has this PR been tested?

Unit tests in core.
I also linked this version to formio-server and ran the tests there. I had some breaking tests, but I don't think they are related to this change.

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

@@ -13,7 +13,7 @@
"./dist/formio.core.min.js": "./dist/formio.core.min.js"
},
"scripts": {
"test": "TEST=1 mocha -r ts-node/register -r tsconfig-paths/register -r mock-local-storage -r jsdom-global/register -b -t 0 'src/**/__tests__/*.test.ts'",
"test": "TEST=1 nyc --reporter=lcov --reporter=text --reporter=text-summary mocha -r ts-node/register -r tsconfig-paths/register -r mock-local-storage -r jsdom-global/register -t 0 'src/**/__tests__/*.test.ts'",
Copy link
Contributor Author

@johnformio johnformio May 14, 2024

Choose a reason for hiding this comment

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

This will run the test through the code coverage. It creates an HTML report to look at manually and file coverage and summary reports in the console.

I also removed the bail flag so all tests run. Not sure if that will cause and issue downstream or not.

"src/experimental/**/*.ts",
"src/types/**/*.ts"
],
"all": true
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

code coverage configuration. Some of the exclusions should be reviewed. the coverage thresholds are based on current value based on the entire project.

// excluding those from this and setting their components onto the submission directly
if (pathFilter.compModelType !== 'dataObject') {
set(filtered, path, value);
} else {
Copy link
Contributor Author

@johnformio johnformio May 14, 2024

Choose a reason for hiding this comment

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

removing the logic to append the data object when the path is an object.

skipping dataObject model types. the child field are included in the scope.filter set, so they get processed and when they do it will set the data field correctly

@johnformio
Copy link
Contributor Author

@AlexeyNikipelau This change was from #78 PR. Please make sure I didn't break anything. All of the unit tests you added are still passing.

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 ran this branch versus the tests on a stable version of formio-server, and the following test failed:

1) Project Tests
       Nested Submissions
         Should allow you to update a submission with sub-submissions.:

      Uncaught AssertionError [ERR_ASSERTION]: The childA form was not submitted
      + expected - actual

      -false
      +true

Lmk if you'd like to discuss, I think this is a pretty tricky issue.

@johnformio johnformio requested a review from brendanbond June 12, 2024 17:38
@johnformio
Copy link
Contributor Author

I ran this branch versus the tests on a stable version of formio-server, and the following test failed:

1) Project Tests
       Nested Submissions
         Should allow you to update a submission with sub-submissions.:

      Uncaught AssertionError [ERR_ASSERTION]: The childA form was not submitted
      + expected - actual

      -false
      +true

Lmk if you'd like to discuss, I think this is a pretty tricky issue.

@brendanbond I ran the test in server and this test did not fail, but a validation one did. I don't think that's related to this change though.

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.

FWIW the tests pass versus formio-master for me, LGTM

@johnformio johnformio marked this pull request as ready for review June 13, 2024 20:11
@johnformio johnformio merged commit 6c50a6a into master Jun 26, 2024
5 checks passed
lane-formio pushed a commit that referenced this pull request Jul 1, 2024
* skipping dataObject model types when filtering

* only skipping dataObject model types

* integrated istanbul code coverage testing with current thresholds

* removed console statements

* using functional version of lodash set so original submit object doesn't get mutated during filter post process

* verified dataGrid submissions are valid

---------

Co-authored-by: John Teague <[email protected]>
lane-formio pushed a commit that referenced this pull request Jul 8, 2024
* skipping dataObject model types when filtering

* only skipping dataObject model types

* integrated istanbul code coverage testing with current thresholds

* removed console statements

* using functional version of lodash set so original submit object doesn't get mutated during filter post process

* verified dataGrid submissions are valid

---------

Co-authored-by: John Teague <[email protected]>
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.

2 participants