fix(#10132): update how api handles form updates on change by nKataraia · Pull Request #10171 · medic/cht-core

nKataraia · 2025-07-22T08:45:00Z

closes #10132

jkuester · 2025-07-22T20:27:59Z

@dianabarsan Do you have bandwidth to review this PR (you logged the original issue)? If not, I can have a look. 👍

dianabarsan

Very nice! One request below and please add unit test coverage for this!

api/src/services/generate-xform.js

dianabarsan · 2025-08-04T11:29:36Z

api/src/services/generate-xform.js

-    });
-  });
+const updateAttachments = async (doc) => {
+  let generated = null;


You don't need to define this variable here, as it's not reused outside of the inner block.

api/tests/mocha/services/generate-xform.spec.js

dianabarsan · 2025-08-04T11:34:37Z

api/tests/mocha/services/generate-xform.spec.js

+        'xform.xml': { data: Buffer.from(formXml) },
+        'form.html': { data: Buffer.from(currentForm) },
+        'model.xml': { data: Buffer.from(currentModel) }


As above, the get will only return attachment stubs, not full contents.

dianabarsan · 2025-08-04T11:40:36Z

api/src/services/generate-xform.js

-const updateAttachment = (doc, updated, name, type) => {
-  const attachmentData = doc._attachments &&
+const updateAttachment = (doc, update, name, type) => {
+  const attachmentData = doc &&


This function will end up comparing new attachment contents with the existent ones. The problem now is that you're not reading all attachments, because you've changed the get command, and you're only reading the main xml attachment. This means that you never have the data of the model.xml or the form.html attachments, so the app will always think they need to updated, and you end up in an infinite loop. you will need to read the contents of those two attachments too to compare.

…hment definition

…e-changes' into 10132-api-form-update-changes

dianabarsan

Nice! Great improvements!
I did find one issue which I've detailed in the comments.

dianabarsan · 2025-08-12T07:01:20Z

api/tests/mocha/services/generate-xform.spec.js

+      const newForm = '<html><title>Hello</title></html>';
+      const newModel = '<instance><multimedia/></instance>';
+
+      sinon.stub(db.medic, 'getAttachment').resolves({ _attachments: {}});


How would getAttachment return a an empty list of attachments?

dianabarsan · 2025-08-12T07:03:15Z

api/tests/mocha/services/generate-xform.spec.js

+      sinon.stub(db.medic, 'put');
+
+      await service.update('form:exists');
+      expect(db.medic.put.callCount).to.equal(1);


The test title says that the form should not be updated if the attachment is not xml, but then this assertion contradicts the test name, with the form actually getting updated.
I'm confused.

api/tests/mocha/services/generate-xform.spec.js

dianabarsan · 2025-08-12T07:34:00Z

api/src/services/generate-xform.js

+  if (enketoForm) {
+    const name = formsService.getXFormAttachmentName(doc);
+    const rawXML = await db.medic.getAttachment(doc._id, name, { rev: doc._rev });
+    const form = await db.medic.getAttachment(doc._id, 'form.html', { rev: doc._rev });


It's possible that any of these two calls fail with a 404 if the form has just been uploaded. This is causing a lot of e2e test failures now.
You will need to wrap this in a try-catch and always create these attachments if they don't yet exist.

…rrected the assertion

… to IO issues

dianabarsan · 2025-08-13T08:30:46Z

api/src/services/generate-xform.js

+const getAttachment = async (doc, name) => {
+  try { 
+    return await db.medic.getAttachment(doc._id, name, { rev: doc._rev }); 
+  } catch (error) { 


I think we shouldn't catch all errors, just 404s, and throw all others.

api/src/services/generate-xform.js

…wn when fetching attachments

dianabarsan

Thanks for all the changes! Awesome work!
I believe this is done and we can proceed to acceptance test this along with the changes to cht-conf to support uploading large attachments.
The focus of this testing is:

forms with large attachments get uploaded
the model.xml and form.html do not get updated when subsequent attachments are uploaded

dianabarsan · 2025-08-27T10:40:01Z

api/tests/mocha/services/generate-xform.spec.js

+        },
+        _rev: '1-rev'
+      });
+      // sinon.stub(db.medic, 'getAttachment').resolves(Buffer.from(formXml));


Please remove this commented code.

dianabarsan · 2025-08-27T10:40:16Z

api/tests/mocha/services/generate-xform.spec.js

+      expect(db.medic.getAttachment.callCount).to.equal(3);
+      expect(service.generate.callCount).to.equal(0);
+      expect(db.medic.put.callCount).to.equal(0);
+      // expectAttachments(db.medic.put.args[0][0], newForm, newModel);


Please remove this commented code.

github-actions · 2025-09-27T00:05:43Z

This PR is now marked "stale" after 30 days without activity. It will be closed automatically in 10 days unless you add a comment, push new changes or remove the "stale" label.

api/src/services/generate-xform.js

Co-authored-by: Diana Barsan <35681649+dianabarsan@users.noreply.github.com>

dianabarsan · 2025-11-04T13:24:09Z

closing in favor of #10248

Removed updating of attachments binaries except for xform attachments

29a05ed

nKataraia marked this pull request as draft July 22, 2025 08:46

dianabarsan self-requested a review July 23, 2025 05:29

Corrected the referrence to fetching attachment function

338d9dc

dianabarsan requested changes Jul 23, 2025

View reviewed changes

api/src/services/generate-xform.js Outdated Show resolved Hide resolved

nKataraia added 3 commits July 23, 2025 15:42

Changed to async await to remove nesting promise chains

3226bfa

Modifying tests so that the new approach is covered

cea9db6

Fixed some stylistic issues

b28b725

nKataraia marked this pull request as ready for review July 30, 2025 13:06

Merge branch 'master' into 10132-api-form-update-changes

85f0edd

nKataraia requested a review from dianabarsan August 1, 2025 11:03

dianabarsan requested changes Aug 4, 2025

View reviewed changes

nKataraia added 3 commits August 7, 2025 11:19

fetched the model and form.html attachments

53b7006

updated the doc to return stubs in get and adjusted the fake getAttac…

40f4a0b

…hment definition

Merge remote-tracking branch 'refs/remotes/dtree/10132-api-form-updat…

01deca1

…e-changes' into 10132-api-form-update-changes

nKataraia requested a review from dianabarsan August 11, 2025 16:29

dianabarsan requested changes Aug 12, 2025

View reviewed changes

nKataraia added 2 commits August 12, 2025 12:26

Changed the test to now test if the xml attachment is availabe and co…

69220fd

…rrected the assertion

Handled the scenario where the attachments could not be retrieved due…

3afcefe

… to IO issues

nKataraia requested a review from dianabarsan August 12, 2025 09:28

nKataraia added 2 commits August 12, 2025 12:43

handled the null pointer exceptions

d70d554

Logged the errors when failing to fetch attachments

11a574d

dianabarsan requested changes Aug 13, 2025

View reviewed changes

Changed from catching all errors to now catching only 404 errors thro…

d134ff6

…wn when fetching attachments

nKataraia changed the title ~~fix(#10132): Update how API handles form updates on change~~ fix(#10132): update how api handles form updates on change Aug 13, 2025

dianabarsan approved these changes Aug 27, 2025

View reviewed changes

nKataraia and others added 2 commits August 27, 2025 16:12

removed commented code that is no longer used

40e2416

Merge branch 'master' into 10132-api-form-update-changes

fce79dd

github-actions bot added the Stale label Sep 27, 2025

dianabarsan removed the Stale label Sep 27, 2025

dianabarsan reviewed Oct 13, 2025

View reviewed changes

api/src/services/generate-xform.js Outdated Show resolved Hide resolved

Update api/src/services/generate-xform.js

aefff13

Co-authored-by: Diana Barsan <35681649+dianabarsan@users.noreply.github.com>

dianabarsan closed this Nov 4, 2025

Uh oh!

Conversation

nKataraia commented Jul 22, 2025

Uh oh!

jkuester commented Jul 22, 2025

Uh oh!

dianabarsan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dianabarsan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dianabarsan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 27, 2025

Uh oh!

Uh oh!

dianabarsan commented Nov 4, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants