feat(#10903): sub contact attachment routing by benkags · Pull Request #10923 · medic/cht-core

benkags · 2026-04-24T21:01:12Z

Description

Every file uploaded in an Enketo form was attached to the main doc, regardless of which sub-document it was uploaded in. A photo added inside a CHW (sibling) section of a CHW Area creation form landed on the CHW Area doc; a file inside a db-doc="true" group or
repeat landed on the report doc instead of the sub-report.

This PR routes each upload and inline binary to the document that owns it, in both save pipelines:

Contact forms (#10903)

Resolves the owner of each [type=file] / [type=binary] node by its form section: main section → primary doc, <contact>/<parent> section → the matching sibling doc, i-th <repeat> <child> → the i-th repeat doc. Unmatched uploads fall back to the main doc.
Filename sanitization and orphaned-attachment cleanup now run per-doc instead of main-doc-only.
Attachment size validation now runs on the contact save path (previously reports only).

Report forms (#10904)

Routes uploads and inline binaries to their nearest db-doc="true" sub-document, falling back to the main report.
Writes each media field's value into doc.fields (repeat-index aware for the main doc), so the field value always resolves its attachment as user-file- + value.

Shared pipeline
Both services now delegate to a new AttachmentRoutingService (upload routing → inline-binary routing → per-doc finalize), parameterized by a small per-pipeline strategy (owner resolution, reference container, field path). Pure DOM primitives live in
attachment-routing.provider.ts.

Attachment naming & backwards compatibility

Inline-binary attachments on newly saved docs are named user-file-<owner-relative-xpath> (e.g. user-file-photo, user-file-group/photo, user-file-my_repeat[2]/photo) instead of the legacy user-file/<form-id>/<xpath>, and the field value now stores the bare
reference. File-widget uploads keep the user-file-<filename> scheme.
Existing docs are not migrated. Legacy slash-named attachments are left intact on edit (covered by tests), and orphan cleanup only considers user-file--prefixed names it can account for.
On edit, an untouched inline-binary field is preserved via a data-attachment-ref sidecar attribute stashed at load time; a fresh upload always wins over a stale sidecar. A dedicated spec pins the enketo-core merge behavior this relies on.
Testing
- Unit: routing primitives, shared pipeline, contact and report strategies, translation-service binary handling (sidecar stash/restore, no value bind-back).
- E2E: sub-contact routing on create and edit, db-doc sub-report routing (including repeats and multiple sub-docs), untouched inline-binary preservation on report edit.
- Closes sub-contact photo capture: Contacts core routing + validation + unit tests #10903
- Closes sub-contact photo capture: Reports core routing + unit tests #10904
- Closes Unable to hide individual fields inside a repeat from the report view #10791

Code review checklist

Readable: Concise, well named, follows the style guide
Documented: Configuration and user documentation on cht-docs
draft docs PR chore: document per-doc attachment routing and inline-binary naming (cht-core#10903) cht-docs#2226
Tested: Unit and/or e2e where appropriate
Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.
AI disclosure: Please disclose use of AI per the guidelines.
Used claude code for code walkthrough, reasoning about the problem, design and implementing code stubs

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

benkags · 2026-05-07T04:50:00Z

This is ready for review.

jkuester

@benkags once again, I am so glad you are working through this! ✨ I spent hours this afternoon reading through and debugging around this code. I have not finished reading it all yet, but I ran out of time today and wanted to push the thoughts that I have so for. 👍

benkags · 2026-05-14T08:23:27Z

@jkuester please take another look

jkuester

Okay, I have been deep down the rabbit hole here. 😬 🐇

All of this is so complicated and interlocked that I ended up just creating benkags#10 with my suggested changes instead of trying to put everything in comments here. 😓 The main focus of my changes was getting all the attachments for the main report and sub-reports to be rendered as expected when viewing the report in the "Reports" tab.

I used file_uploads.xlsx to test with.

A couple things I learned in all my testing and debugging:

Current attachment naming strategies (before this PR):

New-style file type attachements (>=4.9.0):
- Property value set to attachment name
- Attachment name has user-file- prefix + property value
Old-style file type attachments (<4.9.0):
- Property value set to attachment name
- Attachment name has user-file/ prefex + path/to/property
All binary type attachments:
- Property value set to ""
- Attachment name has user-file/ prefex + path/to/property

#1 and #2 are what you got when using any of the image/audio/file/etc types in your xform and the user selects a file from the device to upload.

#3 should mainly be coming from the external Android app integration where you are getting base64 data loaded as the value for a field in the form with instance:: binary set to true. (Technically can also enter base64 text right in the form or load it as a default value.) This data does not go through the Enekto file manager.

Editing docs with attachments

Currently, (before this PR) you can edit docs with file attachments. The attachments with type=file are rendered properly in the form, but the type=binary attachments are not rendered. Depending on how we plan to load profile pictures into a contact form, this may or may not be something we need to worry about....

The rendering of the type=binary attachments is not fixed (even by my changes in the linked PR). I was up way too late last night trying to sort out a solution (see the first commit in my PR for what I came up with). I think there is something viable here that we could move forwards with in the future, but ultimately I felt it was out of scope for your PR here since the existing functionality remains unchanged.

The changes in my PR come without any updates to the tests 😓 or even any manual validation of the contact-attachment flow. I have been purely testing with reports. I am out of time for the week, though, and I figured I would post what I have and get your thoughts. 👍

benkags · 2026-05-16T15:12:07Z

Thanks @jkuester for going down the rabbit hole. I have been down there myself a couple of times with this PR 😃 . Thanks for PR benkags#10 - I agree, it a much better PR review feedback approach. This is what I picked from PR as net change. Let me know if I missed something

a bug fix that I'd missed: File-widget routing bug in xmlToDocs
updated xpath based attachment name logic; so that the sub docs have the relative file path
undoes the 'attachment-filename-in-the-field-value' for the binary types in favor of constructing the base64 string from the attachment itself for re-population when editing the report (the spirit of the first commit)

I think it really comes down to what approach to adopt between Approach A (the one I have here) and Approach B (in your PR) from a design perpective

Approach A: standardize as much as possible with [type=file] approach by populating the type=binary field value with the attachment name. This saves us the extra logic of constructing base64 string from the attachment for edit. The report view is handled by reading the attachment directly and injecting it as an image like any other file object of type image. The cost to pay for here is to handle exiting reports before this PR by populating the binary field from the saved xml form model at display.
- the reports edit / view works in the PR

the report edit was broken before (but report view worked)

Approach B: Does not save attachment name in the saved doc, reconstructs the attachment name from the xml at display and constructs the binary's base64 string to populate the type=binary field.

jkuester · 2026-05-18T20:08:21Z

I think you have pretty clearly understood my PR. 👍 Just a couple comments:

Approach A: standardize as much as possible with [type=file] approach by populating the type=binary field value with the attachment name.

I 100% was hoping this would work out as you have described. 😓 Unfortunately, it did not really seem to save us any complexity.

This saves us the extra logic of constructing base64 string from the attachment for edit. The report view is handled by reading the attachment directly and injecting it as an image like any other file object of type image.

The format-data-record service will still need to keep the extra logic to support any existing reports with the user-file/ format. And the logic in the reports-add component for loading the image attachments when editing a report will still not work for type=binary fields (because the form model knows that they are not type=file fields and so their handling logic is totally different).

On top of that, changing to store the attachment name as the property value for type=binary fields ends up creating new challenges in the edit workflow:

If we do not do anything and just leave the attachment name set as the value, it breaks the display-base64-image preview functionality so that a "invalid image" icon is shown in the form (instead of the current edit "functionality" where nothing is shown when editing a report for the display-base64-image fields.... 😬 (FTR, we can avoid the icon and actually display the image if we re-load the base64 data from the attachment back into the property value before rendering the form. However, as I found in my first commit, this is not super easy. We will probably need to support this eventually, though... 🤷 )
If/when we end up re-injecting the base64 image data for type=binary fields on edit, we immediately hit the challenge of how to avoid adding a duplicate attachment file each time the report is edited even if the type=binary field did not change. Using a random uuid-based attachment name is obviously a problem because we would always get different names. My first commit used the hash of the file contents which worked but added overhead/complexity. Another option would be to just use the path to the field (e.g. like the original type=binary logic was doing).

It ended up being similar complexity to just properly calculate the field path (even for the sub-docs). That was the point where I started trying to figure out what benefits I was actually getting from Approach A.

On the other hand, with Approach B., we end up with a minimal amount of code changed and all the type=binary logic still just follows what it was doing before (but now also works for sub-docs). We have not simplified anything, but we really have not added much more complexity either... 😅 🤷

@benkags if you have a strong preference for Approach A, I am not set against it (as long as we can find a maintainable way to sort out the above challenges). Otherwise, I do think Approach B (as drafted in my PR) should result in the functionality we need with the minimal amount of changes.

Also happy to jump on a call to discuss further if that would be helpful! 👍

benkags · 2026-05-22T01:41:11Z

Ready for review @jkuester

jkuester · 2026-06-02T23:50:31Z

@dianabarsan @benkags you have continued the discussion/efforts here in my absence last week. 🙏 Thank you!

I know there are some plans in motion regarding a Feature Release, etc. I do not intent to change our direction there at all, but I did want to start a new conversation about how to move forwards in the future with getting these changes (or an future iteration of them) into master.

Coming back this week and taking a fresh look at the code, I have put together a sort of design doc where I try to clearly outline the details of the existing binary logic in master, the changes to that logic that are introduced in this PR, and then my latest thoughts on the additional changes that we need to make to get this merged back to master. You should be able to comment line-by-line in the doc so please give some thoughts/feedback when you get a chance! 👍

…rt forms (medic#10922) Co-authored-by: Bernard K. <kagondubernard81@gmail.com>

…rt forms When a report form contains [db-doc="true"] sub-documents with binary/file fields, attachments are now routed to the owning sub-document instead of always attaching to the main report doc. - Add resolveOwnerDoc() to walk up XML tree to nearest db-doc ancestor - Route FileManager file uploads to correct owner doc - Route inline binary blobs to correct owner doc - Fall back to main report doc when element is not inside a sub-doc

processAllAttachments now walks the parsed XML to determine which prepared doc owns each [type=binary] element (main / sibling / repeat child) and attaches files accordingly, instead of dumping every upload on preparedDocs[0]. Field-value sanitization and orphan cleanup also run per-doc. Adds two private helpers: - resolveContactOwnerDoc: DOM-walk from any element to its section root, then to the owning prepared doc (with mainDoc fallback). - findContactOwnerForFilename: locates the [type=binary] node whose text matches a FileManager filename and resolves its owner. No public API changes; no new service dependencies.

saveContact now uses validateAttachments(preparedDocs.preparedDocs)

New 'attachment routing to sub-contacts' describe block exercising: - file uploaded inside a sibling section -> sibling doc - file uploaded inside a repeat child -> i-th repeat doc - mixed uploads across main / sibling / repeat -> each owner - FileManager file with no matching binary node -> main doc fallback - inline binary (draw widget) inside sibling -> sibling doc - per-doc field-value sanitization (sibling field rewritten, main untouched) - main-doc orphan cleanup on edit path with per-doc loop

- main-doc & sub-doc oversize attachment fails saveContact - normal-sized attachment passes validation

Enketo's setVal rewrites uploaded binary nodes to type="file" the moment a value is set.

Port the sub-doc attachment-routing feature (medic#10903/medic#10904) onto master's restructured Enketo form-save code (medic#11256). - Re-home report + contact attachment routing onto EnketoService.saveReport / saveContact via AttachmentRoutingService strategies that resolve each file / inline-binary owner by nearest owning DOM element (EnketoReportFormData / EnketoContactFormData), replacing processFormAttachments. - Move the data-attachment-ref sidecar + node-name findCurrentElement into EnketoPrepopulationDataService.bindJsonToXml (ex-EnketoTranslationService home). - Add validateAttachments on the contact save path in FormService. - Restore AttachmentService (the routing engine's doc-mutation API). - Add EnketoContactFormData.getMainData(). - Route on minified contact docs to avoid the parent/contact reference cycle; drop empty _attachments maps. - Delete the obsolete ContactSaveService / EnketoTranslationService and their specs; update enketo.service.spec attachment assertions to owner-relative user-file-<ref> naming. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…tachment

github-actions · 2026-08-06T04:35:42Z

Hi @benkags — thank you for the contribution! Before this PR can be reviewed, a few things need to be addressed:

The PR description does not follow the pull request template. Please update it to keep the Description, Code review checklist, License sections, and fill in the ones left empty in the template.

While these checks fail, the PR carries the Waiting for contributor label. The checks run again automatically whenever the PR is edited or updated, and the label is replaced with Ready for review once everything above is addressed.

jkuester

Okay, @dianabarsan, this should be ready for your final review! 🙏

As was foretold, the merge here with the latest changes in master was spicy. The main changes that now exist here are that I moved all the attachment handling for the docs out of the enketo.service and down into the form-data classes. That just provided cleaner re-usability (and got a bit more logic out of the EnektoService...).

Bernard had a bunch of e2e tests and I combined a few and stacked some more in their too, so we really have good e2e coverage of all this functionality now!

A couple other things that changed were:

binary attachment names for reports now include the top-level fields in the path. This is just to unify the naming standard across all docs (reports + contacts).
Updated the format-data-record.service to properly account for this naming change when loading the binary images for the Reports view.
Also updated the format-data-record.service to properly handle repeat groups in the hidden_fields list (fixes #10791).
To support all of this, I ended up refactoring/simplifying the xpath code to allow for injecting the form's repeatPaths context.

@benkags can you support here by testing the latest code in this branch against some of the edge cases you and Imran were working through? 🙏 I am pretty sure everything you had automated tests for is covered, but as you know there are just an unending amount of nasty edge cases and I want to be sure we do not regress on anything that you had working before... 😬

dianabarsan · 2026-08-06T13:17:30Z

+      .reduce((binaryAttachments, { filename, attachment }) => ({ ...binaryAttachments, [filename]: attachment }), {});
+    const newFileAttachments = FileManager
+      .getCurrentFiles()
+      .filter(({ name }) => this.findNodeWithTextContent(name))


Will two files with the same name, but different content, get mapped correctly in different docs here?

two files with the same name, but different content

This is technically impossible. Enketo appends a unique(ish) value to the end of a filename when you upload it into the form (hh-mm-ss). If the user manages to upload different files with the same name into two different form questions within the same second, the collision will happen in the Enekto code (and I think the second file will win). If the form questions are routed to different docs, the winning file will be attached to both docs (since both docs have a property that references the file-name). This seemed like the most acceptable behavior without patching Enketo to have more unique file names.

benkags · 2026-08-07T11:25:07Z

Thank you @jkuester for the hard work merging master. I'll manually test this and update here.

benkags · 2026-08-07T18:55:02Z

D-Tree's household form works as expected.

benkags changed the title ~~feat(#10903) sub contact routing~~ feat(#10903): sub contact attachment routing Apr 27, 2026

benkags mentioned this pull request Apr 28, 2026

feat(#10904): route file attachments to correct sub-docs in report forms #10922

Merged

6 tasks

benkags force-pushed the 10903-sub-contact-routing branch 3 times, most recently from 16849ee to 75a7787 Compare May 6, 2026 18:27

jkuester self-requested a review May 6, 2026 18:39

jkuester requested changes May 7, 2026

View reviewed changes

Comment thread webapp/src/ts/services/enketo.service.ts Outdated

Comment thread webapp/src/ts/services/enketo.service.ts Outdated

Comment thread webapp/src/ts/services/enketo.service.ts Outdated

Comment thread webapp/src/ts/services/contact-save.service.ts Outdated

jkuester requested changes May 15, 2026

View reviewed changes

benkags force-pushed the 10903-sub-contact-routing branch 2 times, most recently from 39ccc3c to 69fe840 Compare May 21, 2026 07:37

benkags changed the base branch from 10700-photo-capture-in-sub-contacts-and-reports to master May 21, 2026 07:38

benkags force-pushed the 10903-sub-contact-routing branch from e26b095 to e850f29 Compare May 21, 2026 23:44

jkuester self-requested a review May 25, 2026 18:10

benkags force-pushed the 10903-sub-contact-routing branch 2 times, most recently from 19e33c9 to d3f9c15 Compare May 27, 2026 09:31

benkags mentioned this pull request May 27, 2026

feat(#10662): display contact profile photo #11087

Open

7 tasks

benkags force-pushed the 10903-sub-contact-routing branch from e3ec0be to 0c4ecea Compare June 10, 2026 22:27

YASHSHARMAOFFICIALLY and others added 7 commits June 11, 2026 01:41

feat(medic#10904): route file attachments to correct sub-docs in repo…

0df57c6

…rt forms (medic#10922) Co-authored-by: Bernard K. <kagondubernard81@gmail.com>

feat(medic#10903): validate attachment size on contact save path

f909b74

saveContact now uses validateAttachments(preparedDocs.preparedDocs)

test(medic#10903): cover validateAttachments wiring on contact save path

d47e791

- main-doc & sub-doc oversize attachment fails saveContact - normal-sized attachment passes validation

fix(medic#10903): match type="file" upload nodes when routing

de4b26e

Enketo's setVal rewrites uploaded binary nodes to type="file" the moment a value is set.

benkags mentioned this pull request Jul 8, 2026

chore: document per-doc attachment routing and inline-binary naming (cht-core#10903) medic/cht-docs#2226

Draft

jkuester mentioned this pull request Jul 20, 2026

refactor(#10700): re-write Enketo form save workflow #11256

Merged

5 tasks

dianabarsan self-requested a review July 28, 2026 03:36

jkuester and others added 16 commits July 31, 2026 15:40

Initial refactoring

2c7757f

Clean up binary attachment naming to only include the path

7a78a00

Fix binary attachment handling for edit contact forms

f26cfd8

Fix contact edit file attachment handling

d66c790

Clean up unit tests

7990ce1

Fix prepopulation-data service issue

7bae381

Merge branch 'master' into 10903-sub-contact-routing-merge

8f3e65e

Clean up basic contact-attachments form to properly include binary at…

98a7584

…tachment

Clean up sub-contact-attachments e2e test

ff1916d

Update binary attachment naming to include report fields in name

3364974

Rename photo-upload test

bb9e82a

Clean up e2e tests

ae5c81a

Update report db-doc attachment tests to one-shot the report

46ebcbc

Fix bugs in format-data-record service around xpaths

8617c7f

Fix pregnancy test broken by fixing hidden_fields in repeats

9d7d68c

github-actions Bot added the Waiting for contributor label Aug 6, 2026

jkuester reviewed Aug 6, 2026

View reviewed changes

jkuester self-requested a review August 6, 2026 04:50

This comment was marked as duplicate.

Sign in to view

jkuester self-requested a review August 6, 2026 04:51

dianabarsan reviewed Aug 6, 2026

View reviewed changes

Fix TODO in db-docs-with-attachments

94e54ba

jkuester requested a review from dianabarsan August 6, 2026 14:27

Uh oh!

Uh oh!

Conversation

benkags commented Apr 24, 2026 • edited by jkuester Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Attachment naming & backwards compatibility

Code review checklist

License

Uh oh!

benkags commented May 7, 2026

Uh oh!

jkuester left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benkags commented May 14, 2026

Uh oh!

jkuester left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Current attachment naming strategies (before this PR):

Editing docs with attachments

Uh oh!

benkags commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkuester commented May 18, 2026

Uh oh!

benkags commented May 22, 2026

Uh oh!

jkuester commented Jun 2, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

jkuester left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as duplicate.

Uh oh!

Uh oh!

Uh oh!

dianabarsan Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

jkuester Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benkags commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benkags commented Aug 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benkags commented Apr 24, 2026 •

edited by jkuester

Loading

jkuester left a comment •

edited

Loading

benkags commented May 16, 2026 •

edited

Loading

jkuester Aug 6, 2026 •

edited

Loading

benkags commented Aug 7, 2026 •

edited

Loading