chore(#10700): re-write Enketo form save workflow - #11256
Conversation
jkuester
changed the title
jkuester
changed the title
|
I came here to see what this was all about and saw this:
Amazing work Josh!! |
dianabarsan
left a comment
dianabarsan
left a comment
There was a problem hiding this comment.
Very nice rewrite! I think you've definitely taken this into the right direction.
I left some minor comments and possibly found some issues, but nothing major.
| .map(fieldName => ({ fieldName, doc: formData.getSiblingData(fieldName)?.deserializeDoc(config) })) | ||
| .map(({ fieldName, doc }) => ({ fieldName, doc: this.initializeContactSibling(rootOutputDoc, doc)})); |
There was a problem hiding this comment.
Do we need two chained maps here or can everything be done within one iteration?
Wondering if we did something like:
const siblingSave = EnektoContactFormData.SIBLING_FIELD_NAMES.map(async (fieldName) => {
const siblingData = formData.getSiblingData(fieldName)?.deserializeDoc(config);
const siblingDoc = this.initializeContactSibling(rootOutputDoc, siblingData);
rootOutputDoc[fieldName] = await this.getContactSiblingValue(
siblingDoc, rootOutputDoc[fieldName], defaultData[fieldName]
);
});
await Promise.all(siblingSave);There was a problem hiding this comment.
I def agree on combining the first two map calls. It is poor form to re-assign the doc property in the second map. 👍
However, I will push back on adding in the third map (where we do the assignment on the roodOutputDoc). Technically it works to put it in, but IMHO it really detracts from the readability. I don't really like side-effects in map calls (even though I am def guilty of doing it). But right here the logic is already pretty involved, so, I would prefer maximize the readibility of the code here.
| const newFileAttachments = FileManager | ||
| .getCurrentFiles() | ||
| .map(file => ({ | ||
| name: `${this.USER_FILE_ATTACHMENT_PREFIX}${file.name}`, |
There was a problem hiding this comment.
Old code stripped special characters (UUID fallback for e.g. Devanagari-only names) and rewrote the referencing field values. The specs checking this were dropped without replacement.
There was a problem hiding this comment.
So, that was recent logic added for contact attachments that never existed for the report attachments. I tried a variety of values to see if I could find anything that was a valid file-name but would break the attachment handling and I could not come up with any. For example, I tried file with names like these:
50%offcafé résuméincense"the"gpuincense_the_gpuincense_the_gpu #2is it done?नमस्
These all worked for me. But, I am happy to add that special char handing back in if we have a use for it! 👍
| return { _id: this.formId }; | ||
| get formConfig() { | ||
| return new FormConfig( | ||
| { _id: this.formId }, |
There was a problem hiding this comment.
Do we need additional fields on this doc here?
saveReport destructures internalId/xmlVersion from config.doc — with only _id, the emitted report docs seem to lose their form field (on master completeNewReport set form: formId), and binary attachments get named user-file/undefined/...
There was a problem hiding this comment.
All good points, but IMHO can be out of scope for this work since none of this was included before. Basically we might need to think of a more structured way of accepting form properties here in cht-form. The form xml, model, and html were added as separate string properties on cht-form originally because it meant that the values could be passed via HTML attributes on the web component element. That approach is limited to just strings, though, and even now we have other properties like contactSummary that can only be set via JS.
Happy to do more refactoring here if you want to worry about this now, but otherwise, I think we still at least have the same behavior as before...
3 participants
Description
This PR basically re-writes the doc-saving workflow when an Enekto form (either contact/app/training form) is completed. The existing legacy code suffered from several major problems that made it very difficult to continue developing features on top of it:
The main impetus behind is PR is to provide a better structured base for solving #10700. The hope is that the new functionality in #10923 can be ported on top of the changes in this PR in a way that is way more simple and maintainable.
What is in this PR
The goal was to have this PR be a complete drop-in replacement for the existing functionality. I have done my best to ensure the new code accurately matches the behavior of the old code and there are no new features that I intended to directly support by these changes.
The only behavior difference that I am aware of currently is a new error I added to the save workflow that will fail with a helpful error message when a contact form does not have the contact data in a group that is named for the actual contact type. (So, a person-create form needs to have the contact data in the
persongroup.) This requirement is listed explicitly in our docs and it is implicitly expected by the pre-population logic for setting the initial data onto the contact. Previously it was technically possible to save a contact form with data in a different group. Because our documentation is explicit about how the forms are supposed to be structured, I do not see this as a breaking change. Also, if some does have this issue, they will hit the error and be unable to save those contact forms. It will not just cause weird data issues by failing silently. The fix would just be to update the form config to name the group properly.Added
FormConfigA new
FormConfigclass has been added inwebapp/src/ts/services/form/form-config.tsthat serves as a wrapper for the form doc and its relevant attachments. This FormConfig is retrieved via the XmlFormsService when rendering the form and then can be re-used when saving the form. This wrapper basically centralizes the handling of the form doc and its attachments and avoids the need to retrieve these things multiple times during the render/save process.Added new
EnketoFormDataclassesThe new
EnketoFormDataclasses added inwebapp/src/ts/services/form/form-data.tsare wrappers to encapsulate the XML data model produced when saving an Enekto form. These data classes own the logic for interacting/querying the XML data as well as de-serializing that data to JS objects.Being able to operate against the XML data is useful for queries (either via css selectors or XPaths) which do not have a simple alternative when using normal JS objects. These wrappers help define the available data queries as well as hold on to the
_idreferences for the data to allow for easy interlinking.The xml de-serialization logic from the now-deleted
EnketoTranslationServicewas moved into these EnketoFormData classes. (The JS -> XML serialization logic was just moved to where it was being used in theEnketoPrepoulationDataService.)Move Contact Save functionality into EnketoService
Previously, we had the logic for saving contact forms separated out into the ContactSaveService. I have move this into the EnektoService so now there are only two entry points for saving a form:
EnketoService.saveContactandEnketoService.saveReport. Keeping the code in the same service allows for better alignment between the workflows.As before, there is still probably "too much" code in the EnektoService, but I have balanced things out a bit by deleting duplicate logic and/or moving it into the EnektoFormData classes. The end result is the actual LOC in the enketo.service file has only grown around 30 lines...
Clean up form/enketo service unit tests
A large percentage of the changed lines in this PR are a result of me overhauling the unit tests for the FormService and EnketoService. Previously there was a TON of duplication/leaking between those unit tests (because of how I previously handled separating the FormService from enketo... 😓). The time has come to pay the piper and I have refactored the FormService tests especially to just mock the EnektoService.
AI Disclosure
Basically every single line of the implementation code was hand-written by me. I leveraged Claude a lot for the investigation and minor code structure advice, but ultimately, I really wanted full control/responsibility for porting all the logic line-by-line. Any bugs will be ones that I personally wrote. 👍 😅
I did leverage Claude heavily for the updates to the unit tests. Even there, I have gone through everything line-by-line and made many editorial changes and requested additional tests, etc. There is a lot of lines changed, so it is possible we missed some stuff, but I am highly confident the functionality is well tested and the tests are very maintainable going forwards.
Code review checklist
can_view_old_navigationpermission to see the old design. Test it has appropriate design for RTL languages.Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.