BAH-5159: Save privileges when importing a form by kirsten-gilgenberg · Pull Request #138 · Bahmni/implementer-interface

kirsten-gilgenberg · 2026-09-29T10:01:40Z

Summary

Export (BAH-4865, BAH-4865: Include form privileges in exported form JSON #135) now embeds a form's privileges as formJson.privileges in the exported JSON, but the import path (FormBuilder.jsx) never read that field or called saveFormPrivileges, so privileges were silently dropped on import.
validateFormJsonAndConcepts now extracts privileges from the imported JSON and threads it through importValidForms → saveFormJson.
After the form is created (or the existing-version fallback path is taken), a new saveImportedFormPrivileges helper persists each privilege via the existing saveFormPrivileges API, keyed by the newly created form's id/version.
The form-create POST now requests v=custom:(id,uuid,name,version,published) so id/version are available on the response for this.

Test plan

Added unit tests verifying saveFormPrivileges is called with the correct payload when importing a form with privileges, and not called when there are none.
Full suite: karma start — 415/415 passing.
eslint --ext .js --ext .jsx ./src ./test — clean.

Fixes https://bahmni.atlassian.net/browse/BAH-5159

🤖 Generated with Claude Code

Export (BAH-4865) now embeds a form's privileges in the exported JSON, but import never read that field or persisted it, so privileges were silently dropped on import. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Re-importing a form that already exists by name silently dropped privileges. getFormUuid()/getFormVersion() compared a string form.version against a parseInt'd number, so it always resolved to an empty uuid, crashing the recovery fetch. Separately, saving form content against an already-published form makes the backend fork a new form version, but privileges were being saved against the stale pre-fork id/version, so they landed on the wrong version. saveFormResource now returns the actually-saved form and privilege saving uses those identifiers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Bulk export used the server-side /bahmniie/form/export endpoint, which never populated formJson.privileges (unlike the single-form export path added in BAH-4865). Since import reads privileges from that same field regardless of source, bulk-exported files silently carried no privileges to import. Fetch privileges per form via getFormPrivilegesFromUuid and merge them in before zipping, matching the single-export behavior. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Each imported form triggered its own independent form-list refresh (via saveFormResource -> saveTranslations -> getFormData) as soon as its own resource save completed. With multiple forms importing concurrently, these refreshes could resolve out of order, so a stale refresh from an earlier-completing form could overwrite a more complete one, leaving newly-created forms missing from the list even though the import itself succeeded. Add a single onImportComplete callback fired once all forms in the batch have finished saving. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

hamsavarthiniR-Bahmni

Review for BAH-5159

Good fix for the core bug (privileges weren't read on import at all before this), and the data-threading/payload-shape work is correct. But two broken promise chains undermine the reliability of the fix for the exact scenarios the ticket describes — found 1 additional blocking issue that isn't inline-commentable (see below), plus the one here on saveImportedFormPrivileges.

Found 2 blocking issues (1 inline, 1 below), 2 suggestions, and 3 nits.

Acceptance Criteria: Fixes the brand-new-form import case, but the primary JIRA repro — re-importing a previously-exported form as a new version — goes through a fallback branch that isn't actually awaited by the rest of the import flow (see below), so it's not yet reliably fixed for that path.

Issues not in diff (unchanged line, but newly load-bearing)

File:Line	Type	Issue
src/form-builder/components/FormBuilder.jsx:373	blocking	In the "form already exists" `.catch()` fallback branch, `httpInterceptor.get(...)` is called without `return`. This line is unchanged by this PR (so it's outside any diff hunk and can't carry an inline comment), but this PR is the first time that branch's completion actually matters — it now also saves privileges. Because the GET's promise chain is never returned, `saveFormJson()`'s overall promise resolves before the fallback's save-and-privilege-save actually finishes, so `Promise.all`/`onImportComplete`/`hideLoader` don't wait for it. Suggested fix: add `return` before `httpInterceptor.get(...)` on that line.
test/form-builder/components/FormBuilder.spec.js	suggestion	No test exercises the "form already exists" fallback branch (stubbing the create-POST to reject) — exactly the path containing the issue above, and the JIRA's own primary repro scenario.

Full local report with more detail (including a few INFO-level observations, like the parseInt fix in getFormUuid likely being a no-op given orderFormByVersion already normalizes version to a number): PR_138_code_review.md in the repo root.

hamsavarthiniR-Bahmni · 2026-10-01T07:48:07Z

  }

-  saveFormJson(form, value, formName, translations, nameTranslations) {
+  saveImportedFormPrivileges(formId, formVersion, privileges) {


blocking: saveImportedFormPrivileges calls saveFormPrivileges(formPrivileges) without returning it (line 321), and both call sites invoke it from inside a .then((savedForm) => { ... }) body that itself returns undefined. So the privileges POST is fire-and-forget: a rejection becomes an unhandled promise rejection with no user feedback, and Promise.all(importFormJsonPromises) can resolve (hiding the loader, firing onImportComplete) before the privileges POST even finishes. For a fix whose whole purpose is "privileges silently dropped", this reopens the same failure mode.

return saveFormPrivileges(formPrivileges); here, and return self.saveImportedFormPrivileges(...) at both call sites, so failures propagate to the existing .catch(() => onValidationError(...)).

Fixed in 4984f1d: 4984f1d

saveImportedFormPrivileges now returns the saveFormPrivileges(...) promise, and both call sites return self.saveImportedFormPrivileges(...) so failures propagate to the existing .catch(() => onValidationError(...)).

hamsavarthiniR-Bahmni · 2026-10-01T07:48:07Z

      return Promise.resolve();
    }
-    return httpInterceptor.post(formBuilderConstants.formUrl, form).then((response) => {
+    const createParams = 'v=custom:(id,uuid,name,version,published)';


nit: v=custom:(id,uuid,name,version,published) is added here, but only response.uuid is ever read from this response downstream — formId/formVersion passed into saveImportedFormPrivileges actually come from savedForm (the separate saveFormResource/bahmniFormResourceUrl call), not from this response. Looks like leftover from an earlier version of this change that read response.id/response.version directly.

Consider reverting to the plain httpInterceptor.post(formBuilderConstants.formUrl, form) with no query params, since nothing added by the richer representation is consumed.

Fixed in 91b59e8: 91b59e8

Reverted to the plain httpInterceptor.post(formBuilderConstants.formUrl, form) — nothing downstream reads the richer representation.

hamsavarthiniR-Bahmni · 2026-10-01T07:48:07Z

+      return self.props.saveFormResource(formResource, translationsWithFormUuid,
+        formNameTranslationsResource)
+        .then((savedForm) => {
+          self.saveImportedFormPrivileges(savedForm.id, savedForm.version, privileges);


suggestion: FormDetailContainer._createReqObject/_saveFormPrivileges already builds this exact {formId, formVersion, privilegeName, editable, viewable} payload shape for the same saveFormPrivileges call. Now that there are two implementations, they've already diverged (this one has no error handling, the other does).

Consider extracting a shared buildFormPrivilegesPayload(formId, formVersion, privileges) helper (e.g. in common/apis/formPrivilegesApi.js) so both call sites stay in sync.

Fixed in bd61a99: bd61a99

Extracted buildFormPrivilegesPayload(formId, formVersion, formPrivileges) into common/apis/formPrivilegesApi.js and switched both this call site and FormDetailContainer._createReqObject (now removed) to use it.

hamsavarthiniR-Bahmni · 2026-10-01T07:48:07Z

+          return self.props
+            .saveFormResource(formResource, translations, formNameTranslationsResource)
+            .then((savedForm) => {
+              self.saveImportedFormPrivileges(savedForm.id, savedForm.version, privileges);


question: This is the "form already exists" fallback path — re-importing a previously exported form, which is the JIRA's primary repro scenario. Worth flagging: the outer .catch() this .then() chain lives inside (a few lines up, around the httpInterceptor.get(...) call) doesn't return that GET's promise chain, so this whole branch — including this privilege save — is actually detached from saveFormJson()'s returned promise. Promise.all(importFormJsonPromises) won't wait for any of this to finish. See the review summary for details (not inline-commentable since the missing return itself is on an unchanged line outside this diff's hunks).

Fixed in ce5e81f: ce5e81f

Added the missing return before httpInterceptor.get(...) so this fallback branch's completion is actually awaited, and added a regression test covering this path (asserting the privilege save completes before onImportComplete fires).

hamsavarthiniR-Bahmni · 2026-10-01T07:48:07Z

-            if (formData.length > 0) {
-              zip.generateAsync({ type: 'blob', compression: 'DEFLATE' }).then((content) => {
-                saveAs(content, commonConstants.exportFileName);
+            const privilegesPromises = formData.map((form) =>


nit: Promise.all(privilegesPromises) rejects as soon as any single form's getFormPrivilegesFromUuid call fails, aborting the entire multi-form export even though the other forms' data was already fetched successfully. Previously, export only depended on one endpoint succeeding.

Consider wrapping each getFormPrivilegesFromUuid(...) call in its own .catch(() => []) so one form's failure doesn't fail the whole batch.

Fixed in 252838c: 252838c

Wrapped each getFormPrivilegesFromUuid(...) call in .catch(() => []) so one form's privilege-fetch failure no longer aborts the whole batch export.

hamsavarthiniR-Bahmni · 2026-10-01T07:48:07Z

+        return form.form;
      })
-      .catch((error) => this.showErrors(error));
+      .catch((error) => {


nit: This re-throw is necessary (otherwise the caller's savedForm would be undefined), but it now causes the error to be reported twice: once here via showErrors (which asynchronously parses error.response.json() before calling setMessage), and again via FormBuilder.jsx's own .catch(() => onValidationError(...)) catching the re-thrown error and calling setMessage again with a generic message. The two calls race for the same notification state slot, so which message the user actually sees is non-deterministic.

Worth picking one source of truth for the user-facing message on this path.

Fixed in c62b9be: c62b9be

saveFormResource is only used by the import flow, where FormBuilder.jsx's own onValidationError call already shows a form-specific message. Dropped the redundant showErrors() call here so there's a single source of truth for the user-facing message on this path.

saveImportedFormPrivileges() never returned the saveFormPrivileges() promise, and both call sites invoked it from a .then() body that returned undefined. This made the privileges POST fire-and-forget: a rejection became an unhandled promise rejection with no user feedback, and Promise.all(importFormJsonPromises) could resolve (hiding the loader, firing onImportComplete) before the privileges POST finished.

saveFormJson's fallback branch (triggered when the create POST fails because the form already exists - the primary re-import scenario) called httpInterceptor.get(...) without returning it. The outer .catch() therefore resolved immediately instead of waiting for the GET/save/privilege-save chain, so Promise.all in importValidForms could fire onImportComplete/hideLoader before that work finished. Adds coverage for this previously-untested fallback path, asserting the privilege save completes before onImportComplete fires.

FormBuilder.jsx's saveImportedFormPrivileges and FormDetailContainer.jsx's _createReqObject built the same {formId, formVersion, privilegeName, editable, viewable} payload for saveFormPrivileges independently, and had already diverged slightly. Move the payload construction into formPrivilegesApi.js so both call sites share one implementation.

The v=custom:(id,uuid,name,version,published) query param was added to the create POST, but only response.uuid is ever read from it - the formId/formVersion used by saveImportedFormPrivileges actually come from the separate saveFormResource response, not this one. Revert to the plain POST since nothing consumes the richer representation.

Promise.all(privilegesPromises) rejected as soon as any single form's getFormPrivilegesFromUuid call failed, aborting the entire export even though the other forms' data was already fetched successfully. Previously export only depended on one endpoint succeeding. Catch each call individually so one form's failure doesn't fail the whole batch.

saveFormResource's .catch() called showErrors() (which asynchronously parses the server's error message) and then rethrew; the rethrown error was also caught by FormBuilder.jsx's own .catch(), which called onValidationError with a generic message. Both calls raced for the same notification state, so which message the user saw was non-deterministic. saveFormResource is only used by the import flow, where FormBuilder.jsx's form-specific message is already shown, so drop the redundant showErrors call here and let the rejection propagate to that single source of truth.

kirsten-gilgenberg · 2026-10-01T12:55:20Z

Thanks for the thorough review — addressed everything, one commit per fix:

Broken privilege-save promise chain (blocking, inline): 4984f1d
Dropped return on the "form already exists" fallback GET (blocking, noted in the review summary since it's on an unchanged line) — plus added test coverage for that fallback path: ce5e81f
Extracted shared buildFormPrivilegesPayload helper (suggestion): bd61a99
Dropped unused v=custom:(...) query param on form-create POST (nit): 91b59e8
Guarded export's privilege-fetch batch so one form's failure doesn't abort the rest (nit): 252838c
Removed duplicate error reporting on import's save-form-resource failure (nit): c62b9be

Replied inline on each corresponding comment as well.

The "should have notification container with success type" test's fixture forms have no formJson.uuid, so getFormPrivilegesFromUuid(...) hit an unstubbed URL and sinon's default stub returned undefined instead of a promise. Promise.all tolerated that before, but chaining .catch(() => []) directly on it (252838c) throws on a non-promise, which was caught by exportForms' outer .catch and short-circuited the zip before any file() calls - breaking this test in CI. Stub the getFormPrivilegesFromUuid call for the undefined-uuid case so the export flow actually resolves as it does in production, where httpInterceptor.get always returns a real promise.

kirsten-gilgenberg and others added 5 commits September 29, 2026 11:59

BAH-5159: Save privileges when importing a form

3f17d6d

Export (BAH-4865) now embeds a form's privileges in the exported JSON, but import never read that field or persisted it, so privileges were silently dropped on import. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

BAH-5150 | Fix. Change let to const for immutable variables

6cffa77

hamsavarthiniR-Bahmni reviewed Oct 1, 2026

View reviewed changes

kirsten-gilgenberg added 6 commits October 1, 2026 14:42

Conversation

kirsten-gilgenberg commented Sep 29, 2026

Summary

Test plan

Uh oh!

hamsavarthiniR-Bahmni left a comment

Choose a reason for hiding this comment

Review for BAH-5159

Issues not in diff (unchanged line, but newly load-bearing)

Uh oh!

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!

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!

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!

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!

kirsten-gilgenberg commented Oct 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants