feat(#10509): support uploading attachments in contact forms#10570
Conversation
|
@dianabarsan I've created a draft PR with some initial work so that I can get feedback and see if this is the right direction to go. |
23b480b to
64ee2be
Compare
5e544cf to
7d0c41d
Compare
bd060a7 to
ff04047
Compare
dianabarsan
left a comment
There was a problem hiding this comment.
Nice quick work! Thanks for adding the AI disclosure, that's super helpful when trying to make sense of some complexities that AI tends to add.
I added a few comments below, mainly as a path to simplify things.
Thanks a lot for the quick turn on this!
| }); | ||
| } | ||
|
|
||
| private buildDocumentBoundaries( |
There was a problem hiding this comment.
Wow! this function!
I think the existence of this function is because the code is built to allow attachments for any of the documents the contact form creates. However, for reports, we only attach binary files for the main document. I think that following this model should be sufficient for the first iteration of this work: following the report format and only allowing attachments for the main document.
There was a problem hiding this comment.
This attempt to allow attachments for any of the documents the contact form creates us because in a comment on the issue, @jkuester said
The tricky part is going to be handling the fact that contact forms can create multiple documents (primary doc, sibling docs, repeated child docs), and ensuring that attachments are mapped to the correct document. I think, for each file attachment we will need to look up where in the XML structure it came from, and then map that to the corresponding document. Because of this, I am not sure if there is anything we can easily reuse from xmlToDocs directly, but we can at least borrow the logic for processing FileManager files and binary fields to use in our logic that can be added to ContactSaveService.prepareSubmittedDocsForSave.
There was a problem hiding this comment.
What I think would be helpful is guidance on
- What the XML looks like for contact forms. What elements do they have? Are there a limited set of objects that can show up as the primary, the sibling or the repeated child docs? Which ones can have attachments?
- Which documents we should support attachments for. Are we limiting it to the primary like @dianabarsan is suggesting we do? Or do we need or want to support attachments on the sibling and child docs that @jkuester mentions are possible?
There was a problem hiding this comment.
I asked Josh to clarify this comment about attachments to child docs, we are in a bit of a disagreement and my preference would be to keep things simple.
For sample contact forms, you can always look at the default config contact forms, though these would not have child documents. I will search for a real-world example that does have child documents.
There was a problem hiding this comment.
I asked Josh to clarify this comment about attachments to child docs, we are in a bit of a disagreement and my preference would be to keep things simple.
Sounds good. I'm happy to take whichever path the team decides is best.
For sample contact forms, you can always look at the default config contact forms, though these would not have child documents.
Thanks for the tip!
I will search for a real-world example that does have child documents.
Thank you!
There was a problem hiding this comment.
Based on Josh's reply to your questions, it looks like you two are in agreement about starting with a simple approach. I will be happy to update this PR so that we only support attachments on the main document.
There was a problem hiding this comment.
For sample contact forms, you can always look at the default config contact forms, though these would not have child documents. I will search for a real-world example that does have child documents.
Thanks again for pointing me to that directory, @dianabarsan. I have a couple of follow-up questions
- If I am trying to write realistic tests, are there particular contact forms that it might be good to choose for my tests? For example, person-create or person-edit?
- What is a realistic example of having an attachment to the main document for one of those forms? For example, in person-create xml, would we see an attachment at something like
/data/person/photoor/data/person/notes/attachmentordata/person/ephemeral_dob/birth_certificate? I'd like to be able write tests that are realistic.
There was a problem hiding this comment.
What is a realistic example of having an attachment to the main document
Because this feature has never existed, I do not believe we have one. I have heard of deployments wanting to upload PDFs, others to upload signatures (?).
As for a report that has child documents, I found a few examples in private configs and all of them follow this model: https://github.com/medic/cht-core/blob/master/config/default/forms/contact/clinic-create.xml where the main contact gets created along with a primary contact.
There might be some more elaborate examples out there, but I think for the tests we're good to go with this
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implement XPath-based attachment routing to correctly map binary fields to sibling documents in multi-document contact forms. Binary fields are now attached to the document they belong to based on their XPath location in the XML structure. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Implement XPath-based attachment routing for repeated child documents in contact forms. Binary fields in repeat groups are now correctly mapped to their corresponding child documents based on repeat indices in the XPath. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Handles cases where contact forms generate multiple document types: - prepareRepeatedDocs now supports any repeat group name (not just child_data) by flattening all repeat groups from the repeats object - buildDocumentBoundaries now correctly maps siblings that appear before repeats in XML but after repeats in preparedDocs array using two-pass algorithm (count repeats, then assign indices) This ensures attachments route correctly when forms have both siblings and repeats, regardless of their order in the XML structure. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Tests complex scenario combining all document types (main, siblings, repeats) with attachments to verify XPath-based routing works correctly when all features are used together. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
AttachmentService is never reassigned after construction, so marking it as readonly satisfies code quality checks. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
AttachmentService is never reassigned after construction, so marking it as readonly satisfies code quality checks. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
AttachmentService is never reassigned after construction, so marking it as readonly satisfies code quality checks. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ff04047 to
0ae7f03
Compare
|
I have a question about process. If a PR is still in draft and the branch becomes out-of-date with the base branch, is it okay if I rebase or does your team prefer a merge commit instead? I'm inclined to rebase until the PR is out of draft status because it keeps the commit history looking cleaner without a merge commit, but I'm happy to follow whatever convention your team prefers. |
We general preference is to merge, because rebase loses all intermediary review comments which are very valuable to understand code progression. So please, avoid rebasing. We only rebase when the branch has additional commits (commits accidentally cherry picked) which makes reviewing very difficult. |
we ALWAYS squash and merge, so the commit history gets lost anwyay. |
|
There is more info about process in the docs: https://docs.communityhealthtoolkit.org/community/contributing/code/workflow/#opening-pull-requests |
Changes: - Remove complex XPath-based document routing logic - Simplify processAllAttachments to attach everything to main doc - Revert prepareRepeatedDocs to original simpler implementation - Remove tests for sibling/repeat attachment routing (not needed in v1) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ct-form-attachments
Add integration tests to verify file upload widgets work in contact forms and attachments are correctly included in form submission. - Test single file attachment in contact form - Test multiple attachments (image + document) - Add test form XML with photo and document upload fields Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
I've run through manual testing again to
Here's a recording of that Screen.Recording.2026-02-09.at.09.16.52.mov |
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
|
It looks like the ci-integration-all-all flaked out. When this happens, should I post asking for a re-run? Or just know that eventually someone will re-run it if needed? |
|
I re-ran the test and it passed. I didn't have time today to do a full review, just took a quick look at the latest commits and I'm really liking the new traversals! Thanks! |
Yeah - here or in slack! |
| * @returns The sanitized file name with special characters removed | ||
| */ | ||
| private sanitizeFileName(fileName: string): string { | ||
| return fileName.replace(/[^a-zA-Z0-9_.-]/g, ''); // NOSONAR |
There was a problem hiding this comment.
@dianabarsan
Is this character limitation a problem for places that use other writing systems? Will the files always be named with Latin characters? Do we need to adjust the sanitization?
There was a problem hiding this comment.
Good point. I am nooooot sure. I suppose we could ask @binokaryg or @bhishankc about how files are named in Nepali. The safer option would be to only strip out special characters?
There was a problem hiding this comment.
I propose we keep this as is. In order of relevance, here's why I think this:
- we have many examples of using
a-zA-Zas "safe" - While Nepali likely is an alphabet we should support, this feature is actually initially being deployed in East Africa. As well, what about Arabic? What about Cyrillic?
- There's a larger idea called universal acceptance (UA) that we should consider.
I think the approach should be, at a later date, do an audit of our code for UA and address this concern then.
There was a problem hiding this comment.
After thought: Maybe there's a defensive code happy path where if a file is named تصدير and the result of sanitizeFileName() returns an empty string in which case we give the filename of a UUIDv7 instead?
Right now a file is uploaded called تصدير , we'd return a file name called user-file-, if I'm following the code correctly. Maybe worth an update 🤷 (oh maybe files without suffixes (eg .png) aren't allowed by enketo/browser?)
There was a problem hiding this comment.
User-generated files can sometimes have Devanagari script in their names. Devanagari script is common across Nepali, Hindi, and other regional languages.
For example, a user could capture an image and rename it to "घर.jpg" in Nepali for easier identification later.
It would be good to support UTF-8 characters if not too difficult.
There was a problem hiding this comment.
Following up here to see if there is any action needed right now. Are we going to stick with things the way they are for this PR and adjust later or do we want an adjustment like the one that @mrjones-plip suggested:
Maybe there's a defensive code happy path where if a file is named
تصديرand the result ofsanitizeFileName()returns an empty string in which case we give the filename of a UUIDv7 instead?
|
Please let me know if there are any actions I need to take on this PR. I can't do any merging, so I assume someone else will merge if this is still considered to be approved. |
|
I was expecting a follow-up on the conversation about non-latin characters in file names. We would at least add a unit test showing what happens when you have a file containing only non-latin characters in its name. |
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
…ct-form-attachments
Thanks for clarifying the expectations. I've updated the sanitization function so that it uses a UUID when there are no characters that match the regex. Unit test added as well. Please let me know if we want anything else. |
|
Thanks for keeping this PR moving Cliff and Diana! I think the unit test is great and the UUID handling is icing on the cake. I defer to Diana to finalize the review on the last changes. |
dianabarsan
left a comment
There was a problem hiding this comment.
Awesome! One small request inline.
| const hasExtension = lastDotIndex > 0; | ||
|
|
||
| if (!hasExtension) { | ||
| return fileName.replace(/[^a-zA-Z0-9_.-]/g, '') || uuidV4(); // NOSONAR |
There was a problem hiding this comment.
One request about storing the regex in a variable, so it's absolutely clear we're sanitizing with the same rules.
There was a problem hiding this comment.
Yes. Of course. Done!
|
I'm unfamiliar with the CHT process for merging approved PRs. Does the PR stay unmerged until the target version of CHT is almost ready to be released? Let me know if there is anything I need to be doing or if there is anything that needs updating before merging. |
|
Hey @cliftonmcintosh ! Thanks for reaching out. For external contributors, the process is that the person who reviewed the PR is the one who merges it. If you have no more pending feedback to tend to and the PR has been approved, you're good to go! (which both are true here ; ) I've kicked off CI again in hopes it was a fluke that it didn't pass before 🤷 |
|
I can merge when we believe it's ready, but @cliftonmcintosh comes up with the best questions! I think we would need an answer for these questions, the worst that can happen is that we ungracefully crash if a sub-document has an image attachment. |
@dianabarsan I'll be happy to explore this. I will dig in to find examples of sub-documents. If you have pointers for where I might find some to test with in the default config, please let me know. That might save us a little time. I am traveling today through Monday so likely won't get back to this until Tuesday. Diana code reviewing |
|
Nevermind. I tested this myself. The attachment gets added to the main doc. |
Great! Thanks. I was just investigating to try to figure that out. |
|
while the issue is closed, for next time @cliftonmcintosh, instead of running the whole build config as you mentioned in Step 2: Compiled and Uploaded the Form, if you install CHT Conf which can do any/all of the build calls, enables a much more targeted upload of a specific form: As this JUST calls Right now I'm hoping to test a branch build of your work, and when coupled with Docker Helper, I can be up and running very quickly! (though, admittedly, I can't find images for your branch so I may have to build them 🤷 ). Docker helper is of course not good for core dev work like in this branch - but ideal for ad hoc testing. |
Thanks for the tip! |

Description
This PR adds attachment support to contact forms, enabling file uploads and binary field attachments similar to what already exists for report forms.
What's Implemented
Following discussion in #10509, the implementation was simplified to match report form behavior: all attachments attach to the main contact document only, regardless of form structure.
Backend Changes
FileManagerand attached to the contact documentuser-file-{filename}user-file/{form-id}/{xpath}_defaults()mergeImplementation Details
ContactSaveService.prepareSubmittedDocsForSave()(following @jkuester's guidance)processAllAttachments()methodAttachmentService,FileManager,XpathutilityEnketoService.xmlToDocs()for report formsTest Coverage
webapp/tests/karma/ts/services/contact-save.service.spec.tstests/integration/cht-form/default/contact-with-attachments.wdio-spec.jstests/e2e/default/contacts/contact-attachments.wdio-spec.jsUnit tests are intended to verify:
E2E tests are intended to verify:
All added tests passing ✅
Manual Testing Performed
user-file-{filename})Details about manual testing
Testing Photo Upload on Contact Create
Step 1: Modified the Contact Form
Please note that form modifications have not been committed. The file was modified locally.
File:
config/default/forms/contact/person-create.xml1a. Added field to instance data (~line 922)
1b. Added binding (~line 1062)
1c. Added upload widget to body (~line 1159)
Step 2: Compiled and Uploaded the Form
2a. Compiled the form
2b. Uploaded to CouchDB
Deleted the existing form and posted the new version:
2c. Refreshed the browser
Hard refresh with
Cmd+Shift+R.Step 3: Tested the Upload
Step 4: Verified the Attachment Was Saved
Queried CouchDB directly:
Looked for:
"photo": "filename.png"- the field value"_attachments": { "user-file-filename.png": {...} }- the stored attachmentTest Result
Contact created: "Photo Contact"
Attachment saved:
Testing the Edit Form (Display Existing Attachment)
Step 1: Modified the Edit Form
Please note that form modifications have not been committed. The file was modified locally.
File:
config/default/forms/contact/person-edit.xml1a. Added field to instance data (~line 1013)
1b. Added binding (~line 1166)
Note: The binding used:
type="string"(not binary) because we were displaying the filename, not uploadingreadonly="true()"to prevent editingrelevant="../photo != ''"to only show when a photo exists1c. Added read-only input to body (~line 1284)
Step 2: Compiled and Uploaded the Edit Form
2a. Compiled the form
2b. Uploaded to CouchDB
Deleted the existing form and posted the new version:
2c. Refreshed the browser
Hard refresh with
Cmd+Shift+R.Step 3: Tested Viewing the Existing Attachment
Test Result
Contact edited: "Photo Contact"
Photo field displayed: ✅ Yes - filename shown as read-only
Filename shown:
Screenshot 2026-01-26 at 09.06.22-9_30_5.pngStatus: ✅ Existing attachment filename correctly displayed in edit form
Screenshots
Empty person create form with photo field
Person create form with photo added
Person edit form with photo shown
Not in Scope for V1
Per discussion in #10509, these are deferred:
UI Preview on Edit: Attachments persist when editing, but users cannot see previews of existing attachments (can be added in V2)preview on edit is included as per @dianabarsan's request in feat(#10509): support uploading attachments in contact forms #10570AI Disclosure
AI Assistance: This PR was developed using Claude Code.
How AI was used:
processAllAttachments()methodHuman oversight:
Closes #10509
Code review checklist
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.