-
Notifications
You must be signed in to change notification settings - Fork 46
feat(#537)!: error when form internalId does not match file name #776
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
Conversation
…dia functionality.
# Conflicts: # README.md
Also adds logic for automatically populating the form_id so it does not have to be set in the xlsx.
|
|
||
| return convertForms(environment.pathToProject, 'contact', { | ||
| enketo: true, | ||
| force_data_node: 'data', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now (with the new pyxform) the name of the primary instance node is data by default. So we do not have to do anything special for contact forms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This form an an invalid form_id value of "district". It was causing an error. I updated the form_id value to be district_hospital to cover the case of a contact form with the default "file name" id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This form used to have the invalid form_id of person. I actually just totally removed the form_id column from this form to test the case where pyxform auto-assigns the id based on the file name.
| }, | ||
| "xml2sms": "hello world", | ||
| "internalId": "different", | ||
| "internalId": "example", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated this test case to have a valid (matching) internalId to avoid the new error. Then I added the invalid-internal-id case to test the error.
…37_internalId # Conflicts: # src/lib/convert-forms/index.js # src/lib/forms-utils.js # test/data/convert-contact-forms/forms/contact/expected/district_hospital.xml # test/data/convert-contact-forms/forms/contact/expected/person-edit.xml # test/data/convert-contact-forms/forms/contact/expected/person.xml # test/data/convert-training-forms/forms/training/expected/new_actions_training.xml # test/fn/convert-forms.utils.js # test/lib/convert-forms/index.spec.js
dianabarsan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I added one comment about the intermediary save logic.
dianabarsan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VERY VERY NICE!
This saga with the
internalIdhas been bothering me for awhile. It is yet another unnecessary foot-gun and I figured we could continue the process of fixing it as a part of this major release in cht-conf. See #537 and medic/cht-core#3342 for more context.The main change in this PR is that when there is a miss-match between the form_id in the xlsx, the file name for the xml, and the
internalIdfrom the properties file cht-conf will now raise an error when you run theconvert-*-formsaction. (Previously, theinternalIdvalue on the form doc in the DB was set according to this order of precedence: properties file > form_id in xlsx (instance id in xml) > form xlsx/xml file name.)Now if the ids all match, the form will be successfully converted (though a warning will still be printed if the
internalIdis set in the properties file). If noform_idvalue is provided on the settings tab in the xlsx, then new logic will make sure to set the proper id value into the form xml so everything remains consistent. So, you will no longer be able to specify an id value that is miss-matched from your form name, but now you never actually have to specify an id value at all. cht-conf/pyxform will automatically assign one based on your file name.The
internalIdproperty is still populated as before on the form doc. Now it is just guaranteed to match the form file name and form doc id.I spent a lot of time deliberating and tinkering in various error scenarios until I was confident that there should be a migration path available for any viable existing error case. So, an app-dev should always have a way to resolve the error without breaking their whole instance (or doing big migrations):
internalIdof the form gets set on report docs as theformproperty.internalId. You should not update theinternalIdbecause that would start writing reports with a differentformvalue.internalIdvalue on contact forms is not used anywhere (and not recorded in on contact docs in the DB).form_idcolumn from the xlsx and everything should be fine.internalIdvalue does get set on the training "reports" as theform. So, we don't want to change theinternalIdvalue or we could lose the mapping of which trainings the user has completed.internalId.internalIddoes not even have thetraining:prefix. (In that case where would be no way to update the file names to match theinternalId.)internalIdhaving thetraining:prefix. Without it, the form cannot be rendered. So, I think we do not need to worry about this case since the form would never have worked.FYI, I am targeting the
6_0-stagingbranch with this PR so that I can group these changes with #709 and release both these breaking changes in the same6.0.0major release. When this PR is approved, I will "Squash and merge" it into6_0-staging branch. Then I raise a final PR from6_0-staging branchintomainwhich can be added intomainwith the "Rebase and merge" action so that (hopefully) semantic-release will just cut one major release for two breaking changes... 🤞AI Disclosure: some parts of this PR were suggested/discussed with Claude Code, but nothing was generated solely by AI.
Code review items
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.