From 3f17d6dfa258c652ab8042af19e46110edea586a Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Tue, 29 Sep 2026 11:59:52 +0200 Subject: [PATCH 01/12] BAH-5159: Save privileges when importing a form 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 --- src/form-builder/components/FormBuilder.jsx | 32 ++++++++-- .../components/FormBuilder.spec.js | 59 +++++++++++++++++++ 2 files changed, 86 insertions(+), 5 deletions(-) diff --git a/src/form-builder/components/FormBuilder.jsx b/src/form-builder/components/FormBuilder.jsx index a8700e4f..efd6a04b 100644 --- a/src/form-builder/components/FormBuilder.jsx +++ b/src/form-builder/components/FormBuilder.jsx @@ -26,6 +26,7 @@ import { remove } from 'lodash'; import Spinner from 'common/Spinner'; import { formEventUpdate, saveEventUpdate } from 'form-builder/actions/control'; import { validateFormHyperlinks } from 'form-builder/helpers/hyperlinkValidationHelper'; +import { saveFormPrivileges } from 'common/apis/formPrivilegesApi'; export default class FormBuilder extends Component { @@ -238,6 +239,7 @@ export default class FormBuilder extends Component { const formName = formJson.name; const value = JSON.parse(formJson.resources[0].value); const nameTranslations = formJson.resources[1] && formJson.resources[1].value; + const privileges = formJson.privileges || []; const form = { name: formName, version: '1', @@ -256,7 +258,9 @@ export default class FormBuilder extends Component { }); self.updateImportErrors(fileName, message); } else { - self.formJSONs.push({ form, value, formName, translations, nameTranslations }); + self.formJSONs.push({ + form, value, formName, translations, nameTranslations, privileges, + }); } }); } @@ -288,16 +292,30 @@ export default class FormBuilder extends Component { const self = this; const importFormJsonPromises = []; formJsons.forEach(formJson => { - const { form, value, formName, translations, nameTranslations } = formJson; + const { form, value, formName, translations, nameTranslations, privileges } = formJson; importFormJsonPromises.push(self.saveFormJson(form, value, formName, translations, - nameTranslations)); + nameTranslations, privileges)); }); Promise.all(importFormJsonPromises) .then(() => self.hideLoader()) .catch(() => self.hideLoader()); } - saveFormJson(form, value, formName, translations, nameTranslations) { + saveImportedFormPrivileges(formId, formVersion, privileges) { + if (!privileges || privileges.length === 0) { + return; + } + const formPrivileges = privileges.map((privilege) => ({ + formId, + formVersion, + privilegeName: privilege.privilegeName, + editable: privilege.editable, + viewable: privilege.viewable, + })); + saveFormPrivileges(formPrivileges); + } + + saveFormJson(form, value, formName, translations, nameTranslations, privileges) { const self = this; const val = value; const hyperlinkErrors = validateFormHyperlinks(val, this.props.allowedDomains || []); @@ -307,7 +325,9 @@ export default class FormBuilder extends Component { ); return Promise.resolve(); } - return httpInterceptor.post(formBuilderConstants.formUrl, form).then((response) => { + const createParams = 'v=custom:(id,uuid,name,version,published)'; + const createUrl = `${formBuilderConstants.formUrl}?${createParams}`; + return httpInterceptor.post(createUrl, form).then((response) => { val.uuid = response.uuid; const formResource = { form: { @@ -329,6 +349,7 @@ export default class FormBuilder extends Component { Object.assign({}, eachTranslation, { formUuid: response.uuid })); self.props.saveFormResource(formResource, translationsWithFormUuid, formNameTranslationsResource); + self.saveImportedFormPrivileges(response.id, response.version, privileges); }) .catch(() => { const formUuid = self.getFormUuid(formName); @@ -355,6 +376,7 @@ export default class FormBuilder extends Component { uuid: '', }; self.props.saveFormResource(formResource, translations, formNameTranslationsResource); + self.saveImportedFormPrivileges(data.id, data.version, privileges); }); }); } diff --git a/test/form-builder/components/FormBuilder.spec.js b/test/form-builder/components/FormBuilder.spec.js index 2d5c749e..c2ccf1c2 100644 --- a/test/form-builder/components/FormBuilder.spec.js +++ b/test/form-builder/components/FormBuilder.spec.js @@ -21,6 +21,7 @@ import jsonpath from 'jsonpath/jsonpath'; import * as FormBuilderBreadcrumbs from 'form-builder/components/FormBuilderBreadcrumbs.jsx'; import { MemoryRouter } from 'react-router-dom'; import { formEventUpdate, saveEventUpdate } from 'form-builder/actions/control'; +import { formBuilderConstants } from 'form-builder/constants'; chai.use(chaiEnzyme()); @@ -428,6 +429,64 @@ describe('Import form', () => { done(); }, 500); }); + + it('should save imported form privileges against the newly created form', (done) => { + const formBuilderInstance = wrapper.find('FormBuilder').instance(); + const privileges = [ + { privilegeName: 'sample', editable: true, viewable: false }, + ]; + const formJson = { + form: { name: 'Vitals', version: '1', published: false }, + value: { name: 'Vitals', controls: [] }, + formName: 'Vitals', + translations: [], + nameTranslations: undefined, + privileges, + }; + sinon.stub(httpInterceptor, 'post').callsFake((url) => { + if (url === formBuilderConstants.saveFormPrivilegesUrl) { + return Promise.resolve(); + } + return Promise.resolve(Object.assign({}, data[0], { uuid: 'new_uuid' })); + }); + formBuilderInstance.importValidForms([formJson]); + setTimeout(() => { + const privilegesCall = httpInterceptor.post.getCalls() + .find((call) => call.args[0] === formBuilderConstants.saveFormPrivilegesUrl); + expect(privilegesCall).not.to.eql(undefined); + expect(privilegesCall.args[1]).to.deep.eql([ + { + formId: data[0].id, + formVersion: data[0].version, + privilegeName: 'sample', + editable: true, + viewable: false, + }, + ]); + done(); + }, 500); + }); + + it('should not call saveFormPrivileges when the imported form has no privileges', (done) => { + const formBuilderInstance = wrapper.find('FormBuilder').instance(); + const formJson = { + form: { name: 'Vitals', version: '1', published: false }, + value: { name: 'Vitals', controls: [] }, + formName: 'Vitals', + translations: [], + nameTranslations: undefined, + privileges: [], + }; + sinon.stub(httpInterceptor, 'post').callsFake(() => + Promise.resolve(Object.assign({}, data[0], { uuid: 'new_uuid' }))); + formBuilderInstance.importValidForms([formJson]); + setTimeout(() => { + const privilegesCall = httpInterceptor.post.getCalls() + .find((call) => call.args[0] === formBuilderConstants.saveFormPrivilegesUrl); + expect(privilegesCall).to.eql(undefined); + done(); + }, 500); + }); }); describe('Export Forms', () => { From bc3236a54996f6e5cf14fcd457e1e85e7827fe0b Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Wed, 30 Sep 2026 11:31:24 +0200 Subject: [PATCH 02/12] BAH-5159: Fix privilege import when re-importing an existing form 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 --- src/form-builder/components/FormBuilder.jsx | 28 +++++++++++++++---- .../components/FormBuilderContainer.jsx | 8 ++++-- .../components/FormBuilder.spec.js | 3 +- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/form-builder/components/FormBuilder.jsx b/src/form-builder/components/FormBuilder.jsx index efd6a04b..5c75569f 100644 --- a/src/form-builder/components/FormBuilder.jsx +++ b/src/form-builder/components/FormBuilder.jsx @@ -63,7 +63,8 @@ export default class FormBuilder extends Component { const version = this.getFormVersion(formName); let uuid = ''; this.props.data.forEach(form => { - if (form.name === formName && form.version === version) { + // eslint-disable-next-line + if (form.name === formName && parseInt(form.version) === version) { uuid = form.uuid; } }); @@ -347,9 +348,16 @@ export default class FormBuilder extends Component { }; const translationsWithFormUuid = translations.map((eachTranslation) => Object.assign({}, eachTranslation, { formUuid: response.uuid })); - self.props.saveFormResource(formResource, translationsWithFormUuid, - formNameTranslationsResource); - self.saveImportedFormPrivileges(response.id, response.version, privileges); + return self.props.saveFormResource(formResource, translationsWithFormUuid, + formNameTranslationsResource) + .then((savedForm) => { + self.saveImportedFormPrivileges(savedForm.id, savedForm.version, privileges); + }) + .catch(() => { + self.props.onValidationError( + `Import failed for form "${formName}": could not save form content` + ); + }); }) .catch(() => { const formUuid = self.getFormUuid(formName); @@ -375,8 +383,16 @@ export default class FormBuilder extends Component { value: nameTranslations, uuid: '', }; - self.props.saveFormResource(formResource, translations, formNameTranslationsResource); - self.saveImportedFormPrivileges(data.id, data.version, privileges); + return self.props + .saveFormResource(formResource, translations, formNameTranslationsResource) + .then((savedForm) => { + self.saveImportedFormPrivileges(savedForm.id, savedForm.version, privileges); + }); + }) + .catch(() => { + self.props.onValidationError( + `Import failed for form "${formName}": could not resolve the existing form` + ); }); }); } diff --git a/src/form-builder/components/FormBuilderContainer.jsx b/src/form-builder/components/FormBuilderContainer.jsx index fd88f16c..2805aa7b 100644 --- a/src/form-builder/components/FormBuilderContainer.jsx +++ b/src/form-builder/components/FormBuilderContainer.jsx @@ -156,7 +156,7 @@ export class FormBuilderContainer extends Component { saveFormResource(formJson, formTranslations, formNameTranslationsResource) { const self = this; self.setMessage('Importing Form...', commonConstants.responseType.success); - httpInterceptor.post(formBuilderConstants.bahmniFormResourceUrl, formJson) + return httpInterceptor.post(formBuilderConstants.bahmniFormResourceUrl, formJson) .then((form) => { const updatedTranslations = map(formTranslations, (translation) => { const formTranslation = translation; @@ -165,8 +165,12 @@ export class FormBuilderContainer extends Component { return formTranslation; }); self.saveTranslations(updatedTranslations, formNameTranslationsResource); + return form.form; }) - .catch((error) => this.showErrors(error)); + .catch((error) => { + this.showErrors(error); + throw error; + }); } render() { diff --git a/test/form-builder/components/FormBuilder.spec.js b/test/form-builder/components/FormBuilder.spec.js index c2ccf1c2..5310f1ea 100644 --- a/test/form-builder/components/FormBuilder.spec.js +++ b/test/form-builder/components/FormBuilder.spec.js @@ -103,7 +103,8 @@ describe('Import form', () => { let wrapper; let breadcrumbsStub; const saveFormSpy = sinon.spy(); - const saveFormResourceSpy = sinon.spy(); + const saveFormResourceSpy = sinon.spy(() => + Promise.resolve({ id: 201, uuid: 'new_uuid', version: 1 })); const dispatchSpy = sinon.spy(); const file = [ { From 3d794aeea98ece2db3e255032afde948a51dbf56 Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Wed, 30 Sep 2026 13:05:06 +0200 Subject: [PATCH 03/12] BAH-5159: Include privileges when exporting multiple forms 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 --- src/form-builder/components/FormBuilder.jsx | 33 +++++++++++-------- .../components/FormBuilder.spec.js | 26 +++++++++++++++ 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/src/form-builder/components/FormBuilder.jsx b/src/form-builder/components/FormBuilder.jsx index 5c75569f..4a4392f7 100644 --- a/src/form-builder/components/FormBuilder.jsx +++ b/src/form-builder/components/FormBuilder.jsx @@ -26,7 +26,7 @@ import { remove } from 'lodash'; import Spinner from 'common/Spinner'; import { formEventUpdate, saveEventUpdate } from 'form-builder/actions/control'; import { validateFormHyperlinks } from 'form-builder/helpers/hyperlinkValidationHelper'; -import { saveFormPrivileges } from 'common/apis/formPrivilegesApi'; +import { saveFormPrivileges, getFormPrivilegesFromUuid } from 'common/apis/formPrivilegesApi'; export default class FormBuilder extends Component { @@ -474,7 +474,6 @@ export default class FormBuilder extends Component { return; } const zip = new JSZip(); - let fileName; let params = ''; const uuids = this.state.selectedForms; uuids.forEach((uuid, index) => { @@ -488,19 +487,27 @@ export default class FormBuilder extends Component { commonConstants.responseType.error); } const formData = exportResponse.bahmniFormDataList; - formData.forEach(form => { - fileName = `${form.formJson.name}_${form.formJson.version}`; - zip.file(`${fileName}.json`, JSON.stringify(form)); - }); - if (formData.length > 0) { - zip.generateAsync({ type: 'blob', compression: 'DEFLATE' }).then((content) => { - saveAs(content, commonConstants.exportFileName); + const privilegesPromises = formData.map((form) => + getFormPrivilegesFromUuid(form.formJson.uuid)); + return Promise.all(privilegesPromises).then((privilegesList) => { + formData.forEach((form, index) => { + const fileName = `${form.formJson.name}_${form.formJson.version}`; + const formWithPrivileges = Object.assign({}, form, { + formJson: Object.assign({}, form.formJson, + { privileges: privilegesList[index] }), + }); + zip.file(`${fileName}.json`, JSON.stringify(formWithPrivileges)); }); - if (exportResponse.errorFormList.length === 0) { - this.setMessage(commonConstants.exportFormsSuccessMessage, - commonConstants.responseType.success); + if (formData.length > 0) { + zip.generateAsync({ type: 'blob', compression: 'DEFLATE' }).then((content) => { + saveAs(content, commonConstants.exportFileName); + }); + if (exportResponse.errorFormList.length === 0) { + this.setMessage(commonConstants.exportFormsSuccessMessage, + commonConstants.responseType.success); + } } - } + }); }) .catch(() => { this.setMessage('Export failed', commonConstants.responseType.error); diff --git a/test/form-builder/components/FormBuilder.spec.js b/test/form-builder/components/FormBuilder.spec.js index 5310f1ea..1a067851 100644 --- a/test/form-builder/components/FormBuilder.spec.js +++ b/test/form-builder/components/FormBuilder.spec.js @@ -583,6 +583,32 @@ describe('Export Forms', () => { done(); }, 50); }); + + it('should include privileges when exporting multiple forms', (done) => { + if (JSZip.prototype.file.restore !== undefined) { + JSZip.prototype.file.restore(); + } + const spyZipFile = sinon.spy(JSZip.prototype, 'file'); + wrapper.instance().state.selectedForms = ['uuid1']; + const privileges = [{ formId: 1, privilegeName: 'sample', editable: true, viewable: false }]; + exportResponse = { + bahmniFormDataList: [{ formJson: { name: 'Form', version: '1', uuid: 'uuid1' } }], + errorFormList: [], + }; + mockHttp.get.withArgs('/openmrs/ws/rest/v1/bahmniie/form/export?uuid=uuid1') + .returns(Promise.resolve(exportResponse)); + mockHttp.get + .withArgs('/openmrs/ws/rest/v1/bahmniie/form/getFormPrivilegesFromUuid?formUuid=uuid1') + .returns(Promise.resolve(privileges)); + wrapper.instance().exportForms(); + setTimeout(() => { + sinon.assert.calledOnce(spyZipFile); + const zippedContent = JSON.parse(spyZipFile.getCall(0).args[1]); + expect(zippedContent.formJson.privileges).to.deep.eql(privileges); + spyZipFile.restore(); + done(); + }, 50); + }); }); describe('Import Multiple Forms', () => { From c0ea8b2d044329f4fddcb812c044d8dbb32dfb6c Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Wed, 30 Sep 2026 13:44:12 +0200 Subject: [PATCH 04/12] BAH-5159: Refresh form list once after a bulk import finishes 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 --- src/form-builder/components/FormBuilder.jsx | 8 ++++++- .../components/FormBuilderContainer.jsx | 1 + .../components/FormBuilder.spec.js | 24 +++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/src/form-builder/components/FormBuilder.jsx b/src/form-builder/components/FormBuilder.jsx index 4a4392f7..930f2a58 100644 --- a/src/form-builder/components/FormBuilder.jsx +++ b/src/form-builder/components/FormBuilder.jsx @@ -298,7 +298,12 @@ export default class FormBuilder extends Component { nameTranslations, privileges)); }); Promise.all(importFormJsonPromises) - .then(() => self.hideLoader()) + .then(() => { + if (self.props.onImportComplete) { + self.props.onImportComplete(); + } + self.hideLoader(); + }) .catch(() => self.hideLoader()); } @@ -574,6 +579,7 @@ FormBuilder.propTypes = { isExact: PropTypes.bool.isRequired, params: PropTypes.object, }), + onImportComplete: PropTypes.func, onValidationError: PropTypes.func, routes: PropTypes.array, saveForm: PropTypes.func.isRequired, diff --git a/src/form-builder/components/FormBuilderContainer.jsx b/src/form-builder/components/FormBuilderContainer.jsx index 2805aa7b..10cd87e8 100644 --- a/src/form-builder/components/FormBuilderContainer.jsx +++ b/src/form-builder/components/FormBuilderContainer.jsx @@ -185,6 +185,7 @@ export class FormBuilderContainer extends Component { data={this.state.data} dispatch={this.props.dispatch} match={this.props.match} + onImportComplete={() => this.getFormData()} onValidationError={(messages) => this.onValidationError(messages)} routes={this.props.routes} saveForm={(formName) => this.saveForm(formName)} diff --git a/test/form-builder/components/FormBuilder.spec.js b/test/form-builder/components/FormBuilder.spec.js index 1a067851..162714f5 100644 --- a/test/form-builder/components/FormBuilder.spec.js +++ b/test/form-builder/components/FormBuilder.spec.js @@ -488,6 +488,30 @@ describe('Import form', () => { done(); }, 500); }); + + it('should call onImportComplete once after all forms in the batch are imported', (done) => { + const onImportCompleteSpy = sinon.spy(); + const localWrapper = mount(); + const formBuilderInstance = localWrapper.find('FormBuilder').instance(); + const formJsons = ['Vitals1', 'Vitals2'].map((name) => ({ + form: { name, version: '1', published: false }, + value: { name, controls: [] }, + formName: name, + translations: [], + nameTranslations: undefined, + privileges: [], + })); + sinon.stub(httpInterceptor, 'post').callsFake(() => + Promise.resolve(Object.assign({}, data[0], { uuid: 'new_uuid' }))); + formBuilderInstance.importValidForms(formJsons); + setTimeout(() => { + sinon.assert.calledOnce(onImportCompleteSpy); + done(); + }, 500); + }); }); describe('Export Forms', () => { From 6cffa777f2c7a4935e85385b5265ac816a4ead71 Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Wed, 30 Sep 2026 15:53:48 +0200 Subject: [PATCH 05/12] BAH-5150 | Fix. Change let to const for immutable variables --- src/form-builder/components/FormBuilderContainer.jsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/form-builder/components/FormBuilderContainer.jsx b/src/form-builder/components/FormBuilderContainer.jsx index 10cd87e8..bc49b206 100644 --- a/src/form-builder/components/FormBuilderContainer.jsx +++ b/src/form-builder/components/FormBuilderContainer.jsx @@ -48,8 +48,8 @@ export class FormBuilderContainer extends Component { } getFormData() { - let initialForms = []; - let forms = []; + const initialForms = []; + const forms = []; const queryParams = '?='; const fetchFormsUrl = `${formBuilderConstants.formUrl}?v=custom:(id,uuid,name,version,published,auditInfo)`; return httpInterceptor.get(fetchFormsUrl) From 4984f1dd5e8596cb37f0c7b963c5b1c4054fc021 Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Thu, 1 Oct 2026 14:42:56 +0200 Subject: [PATCH 06/12] BAH-5159: Fix broken privilege-save promise chain during form import 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. --- src/form-builder/components/FormBuilder.jsx | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/form-builder/components/FormBuilder.jsx b/src/form-builder/components/FormBuilder.jsx index 930f2a58..0b91e8e2 100644 --- a/src/form-builder/components/FormBuilder.jsx +++ b/src/form-builder/components/FormBuilder.jsx @@ -309,7 +309,7 @@ export default class FormBuilder extends Component { saveImportedFormPrivileges(formId, formVersion, privileges) { if (!privileges || privileges.length === 0) { - return; + return Promise.resolve(); } const formPrivileges = privileges.map((privilege) => ({ formId, @@ -318,7 +318,7 @@ export default class FormBuilder extends Component { editable: privilege.editable, viewable: privilege.viewable, })); - saveFormPrivileges(formPrivileges); + return saveFormPrivileges(formPrivileges); } saveFormJson(form, value, formName, translations, nameTranslations, privileges) { @@ -355,9 +355,8 @@ export default class FormBuilder extends Component { Object.assign({}, eachTranslation, { formUuid: response.uuid })); return self.props.saveFormResource(formResource, translationsWithFormUuid, formNameTranslationsResource) - .then((savedForm) => { - self.saveImportedFormPrivileges(savedForm.id, savedForm.version, privileges); - }) + .then((savedForm) => + self.saveImportedFormPrivileges(savedForm.id, savedForm.version, privileges)) .catch(() => { self.props.onValidationError( `Import failed for form "${formName}": could not save form content` @@ -390,9 +389,8 @@ export default class FormBuilder extends Component { }; return self.props .saveFormResource(formResource, translations, formNameTranslationsResource) - .then((savedForm) => { - self.saveImportedFormPrivileges(savedForm.id, savedForm.version, privileges); - }); + .then((savedForm) => + self.saveImportedFormPrivileges(savedForm.id, savedForm.version, privileges)); }) .catch(() => { self.props.onValidationError( From ce5e81f6c60d3430e298182e90ba46c28cf6f3c1 Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Thu, 1 Oct 2026 14:45:32 +0200 Subject: [PATCH 07/12] BAH-5159: Fix dropped return on the "form already exists" fallback GET 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. --- src/form-builder/components/FormBuilder.jsx | 2 +- .../components/FormBuilder.spec.js | 52 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/form-builder/components/FormBuilder.jsx b/src/form-builder/components/FormBuilder.jsx index 0b91e8e2..07091435 100644 --- a/src/form-builder/components/FormBuilder.jsx +++ b/src/form-builder/components/FormBuilder.jsx @@ -369,7 +369,7 @@ export default class FormBuilder extends Component { const params = 'v=custom:(id,uuid,name,version,published,auditInfo,' + 'resources:(value,dataType,uuid))'; - httpInterceptor.get(`${formBuilderConstants.formUrl}/${formUuid}?${params}`) + return httpInterceptor.get(`${formBuilderConstants.formUrl}/${formUuid}?${params}`) .then((data) => { const formResource = { form: { diff --git a/test/form-builder/components/FormBuilder.spec.js b/test/form-builder/components/FormBuilder.spec.js index 162714f5..1f5511e6 100644 --- a/test/form-builder/components/FormBuilder.spec.js +++ b/test/form-builder/components/FormBuilder.spec.js @@ -489,6 +489,58 @@ describe('Import form', () => { }, 500); }); + it('should save privileges and wait for completion on the "form already exists" ' + + 'fallback path', (done) => { + const callOrder = []; + const onImportCompleteSpy = sinon.spy(() => callOrder.push('onImportComplete')); + const fallbackSaveFormResourceSpy = sinon.spy(() => + Promise.resolve({ id: 301, uuid: 'existing_form_uuid', version: 2 })); + const localWrapper = mount(); + const formBuilderInstance = localWrapper.find('FormBuilder').instance(); + const privileges = [ + { privilegeName: 'sample', editable: true, viewable: false }, + ]; + const formJson = { + form: { name: '1', version: '1', published: false }, + value: { name: '1', controls: [] }, + formName: '1', + translations: [], + nameTranslations: undefined, + privileges, + }; + sinon.stub(httpInterceptor, 'post').callsFake((url) => { + if (url === formBuilderConstants.saveFormPrivilegesUrl) { + callOrder.push('privileges'); + return Promise.resolve(); + } + return Promise.reject(new Error('form already exists')); + }); + sinon.stub(httpInterceptor, 'get').callsFake(() => + Promise.resolve({ resources: [{ uuid: 'resource_uuid' }] })); + formBuilderInstance.importValidForms([formJson]); + setTimeout(() => { + sinon.assert.calledOnce(fallbackSaveFormResourceSpy); + const privilegesCall = httpInterceptor.post.getCalls() + .find((call) => call.args[0] === formBuilderConstants.saveFormPrivilegesUrl); + expect(privilegesCall).not.to.eql(undefined); + expect(privilegesCall.args[1]).to.deep.eql([ + { + formId: 301, + formVersion: 2, + privilegeName: 'sample', + editable: true, + viewable: false, + }, + ]); + sinon.assert.calledOnce(onImportCompleteSpy); + expect(callOrder).to.eql(['privileges', 'onImportComplete']); + done(); + }, 500); + }); + it('should call onImportComplete once after all forms in the batch are imported', (done) => { const onImportCompleteSpy = sinon.spy(); const localWrapper = mount( Date: Thu, 1 Oct 2026 14:47:21 +0200 Subject: [PATCH 08/12] BAH-5159: Extract shared buildFormPrivilegesPayload helper 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. --- src/common/apis/formPrivilegesApi.js | 9 ++++++++ src/form-builder/components/FormBuilder.jsx | 13 ++++-------- .../components/FormDetailContainer.jsx | 21 ++----------------- 3 files changed, 15 insertions(+), 28 deletions(-) diff --git a/src/common/apis/formPrivilegesApi.js b/src/common/apis/formPrivilegesApi.js index 1c780d3d..61d82e1b 100644 --- a/src/common/apis/formPrivilegesApi.js +++ b/src/common/apis/formPrivilegesApi.js @@ -14,6 +14,15 @@ import { UrlHelper } from 'form-builder/helpers/UrlHelper'; export function saveFormPrivileges(formPrivileges) { return httpInterceptor.post(formBuilderConstants.saveFormPrivilegesUrl, formPrivileges); } +export function buildFormPrivilegesPayload(formId, formVersion, formPrivileges) { + return formPrivileges.map((privilege) => ({ + formId, + formVersion, + privilegeName: privilege.privilegeName, + editable: privilege.editable, + viewable: privilege.viewable, + })); +} export function getFormPrivileges(formId, formVersion) { return httpInterceptor.get(new UrlHelper() .getFormPrivilegesUrl(formId, formVersion), 'text'); diff --git a/src/form-builder/components/FormBuilder.jsx b/src/form-builder/components/FormBuilder.jsx index 07091435..9ae125b7 100644 --- a/src/form-builder/components/FormBuilder.jsx +++ b/src/form-builder/components/FormBuilder.jsx @@ -26,7 +26,9 @@ import { remove } from 'lodash'; import Spinner from 'common/Spinner'; import { formEventUpdate, saveEventUpdate } from 'form-builder/actions/control'; import { validateFormHyperlinks } from 'form-builder/helpers/hyperlinkValidationHelper'; -import { saveFormPrivileges, getFormPrivilegesFromUuid } from 'common/apis/formPrivilegesApi'; +import { + saveFormPrivileges, getFormPrivilegesFromUuid, buildFormPrivilegesPayload, +} from 'common/apis/formPrivilegesApi'; export default class FormBuilder extends Component { @@ -311,14 +313,7 @@ export default class FormBuilder extends Component { if (!privileges || privileges.length === 0) { return Promise.resolve(); } - const formPrivileges = privileges.map((privilege) => ({ - formId, - formVersion, - privilegeName: privilege.privilegeName, - editable: privilege.editable, - viewable: privilege.viewable, - })); - return saveFormPrivileges(formPrivileges); + return saveFormPrivileges(buildFormPrivilegesPayload(formId, formVersion, privileges)); } saveFormJson(form, value, formName, translations, nameTranslations, privileges) { diff --git a/src/form-builder/components/FormDetailContainer.jsx b/src/form-builder/components/FormDetailContainer.jsx index 596eaed1..ba155f2c 100644 --- a/src/form-builder/components/FormDetailContainer.jsx +++ b/src/form-builder/components/FormDetailContainer.jsx @@ -44,7 +44,7 @@ import { } from 'common/apis/formTranslationApi'; import FormPreviewModal from 'form-builder/components/FormPreviewModal.jsx'; import Popup from 'reactjs-popup'; -import { saveFormPrivileges } from 'common/apis/formPrivilegesApi'; +import { saveFormPrivileges, buildFormPrivilegesPayload } from 'common/apis/formPrivilegesApi'; import { validateFormHyperlinks, fetchAllowedDomains } from 'form-builder/helpers/hyperlinkValidationHelper'; export class FormDetailContainer extends Component { @@ -227,9 +227,8 @@ export class FormDetailContainer extends Component { }); } _saveFormPrivileges(formId, formVersion) { - let formVersionTemp = formVersion; saveFormPrivileges( - this._createReqObject(formId, formVersionTemp, this.state.formPrivileges) + buildFormPrivilegesPayload(formId, formVersion, this.state.formPrivileges) ) .then(() => { const message = 'Form Privileges saved successfully'; @@ -242,22 +241,6 @@ export class FormDetailContainer extends Component { }); } - _createReqObject(formId, formVersion, formPrivileges) { - const formPrivilegeObj = []; - for (let i = 0; i < formPrivileges.length; i++) { - const privilege = formPrivileges[i]; - const privilegeCopy = { - formId, - privilegeName: privilege.privilegeName, - editable: privilege.editable, - viewable: privilege.viewable, - formVersion, - }; - formPrivilegeObj.push(privilegeCopy); - } - return formPrivilegeObj; - } - onPublish() { try { const formJson = this.getFormResource(); From 91b59e857e2233bce7cd1f8a471f3d9ce18082fd Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Thu, 1 Oct 2026 14:47:44 +0200 Subject: [PATCH 09/12] BAH-5159: Drop unused richer representation on form-create POST 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. --- src/form-builder/components/FormBuilder.jsx | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/form-builder/components/FormBuilder.jsx b/src/form-builder/components/FormBuilder.jsx index 9ae125b7..974bf210 100644 --- a/src/form-builder/components/FormBuilder.jsx +++ b/src/form-builder/components/FormBuilder.jsx @@ -326,9 +326,7 @@ export default class FormBuilder extends Component { ); return Promise.resolve(); } - const createParams = 'v=custom:(id,uuid,name,version,published)'; - const createUrl = `${formBuilderConstants.formUrl}?${createParams}`; - return httpInterceptor.post(createUrl, form).then((response) => { + return httpInterceptor.post(formBuilderConstants.formUrl, form).then((response) => { val.uuid = response.uuid; const formResource = { form: { From 252838ce245c3cf8ea3d84fdf1a376250798da23 Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Thu, 1 Oct 2026 14:49:20 +0200 Subject: [PATCH 10/12] BAH-5159: Don't abort multi-form export over one privilege-fetch failure 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. --- src/form-builder/components/FormBuilder.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/form-builder/components/FormBuilder.jsx b/src/form-builder/components/FormBuilder.jsx index 974bf210..d50b2e5a 100644 --- a/src/form-builder/components/FormBuilder.jsx +++ b/src/form-builder/components/FormBuilder.jsx @@ -484,7 +484,7 @@ export default class FormBuilder extends Component { } const formData = exportResponse.bahmniFormDataList; const privilegesPromises = formData.map((form) => - getFormPrivilegesFromUuid(form.formJson.uuid)); + getFormPrivilegesFromUuid(form.formJson.uuid).catch(() => [])); return Promise.all(privilegesPromises).then((privilegesList) => { formData.forEach((form, index) => { const fileName = `${form.formJson.name}_${form.formJson.version}`; From c62b9be8ad9d1326279c0d292f1ab65f48d8d636 Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Thu, 1 Oct 2026 14:50:53 +0200 Subject: [PATCH 11/12] BAH-5159: Stop double-reporting form-resource save errors on import 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. --- src/form-builder/components/FormBuilderContainer.jsx | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/form-builder/components/FormBuilderContainer.jsx b/src/form-builder/components/FormBuilderContainer.jsx index bc49b206..96763810 100644 --- a/src/form-builder/components/FormBuilderContainer.jsx +++ b/src/form-builder/components/FormBuilderContainer.jsx @@ -166,10 +166,6 @@ export class FormBuilderContainer extends Component { }); self.saveTranslations(updatedTranslations, formNameTranslationsResource); return form.form; - }) - .catch((error) => { - this.showErrors(error); - throw error; }); } From 187efcde99a720c66eb59d88efcd48a28f45012b Mon Sep 17 00:00:00 2001 From: Kirsten Gilgenberg <84097116+kirsten-gilgenberg@users.noreply.github.com> Date: Thu, 1 Oct 2026 15:11:11 +0200 Subject: [PATCH 12/12] BAH-5159: Fix CI failure in export success-notification test 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. --- test/form-builder/components/FormBuilder.spec.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/form-builder/components/FormBuilder.spec.js b/test/form-builder/components/FormBuilder.spec.js index 1f5511e6..74315f59 100644 --- a/test/form-builder/components/FormBuilder.spec.js +++ b/test/form-builder/components/FormBuilder.spec.js @@ -652,6 +652,9 @@ describe('Export Forms', () => { }; mockHttp.get.withArgs('/openmrs/ws/rest/v1/bahmniie/form/export?uuid=uuid1') .returns(Promise.resolve(exportResponse)); + mockHttp.get + .withArgs('/openmrs/ws/rest/v1/bahmniie/form/getFormPrivilegesFromUuid?formUuid=undefined') + .returns(Promise.resolve([])); wrapper.instance().exportForms(); setTimeout(() => { sinon.assert.calledTwice(spyZipFile);