BAH-4865: Include form privileges in exported form JSON - #135
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude Describtion : - PR DescriptionJIRA: BAH-4865 What Changed
Why How Tested
Acceptance Criteria Met
|
hamsavarthiniR-Bahmni
left a comment
There was a problem hiding this comment.
Review for BAH-4865
Clean, well-targeted fix — endpoint scoping, response-shape assumptions, and the 'text'→JSON blast radius all checked out on independent trace, and the updated test asserts on the actual serialized payload rather than just that an API was called.
Found 1 suggestion and 3 nits inline, plus 2 items below that touch files/lines outside this diff.
Acceptance Criteria: Export now correctly includes form privileges, scoped to the right version, verified by test. Round-trip (export → re-import) does not currently restore privileges — see below.
Issues not in diff
| File:Line | Type | Issue |
|---|---|---|
| src/form-builder/components/FormBuilder.jsx:237 | suggestion | validateFormJsonAndConcepts destructures the exact { formJson, translations } shape this PR's export now produces, but never reads formJson.privileges — traced saveFormPrivileges usage and it's only called from the manual "Manage Privileges" UI, never from the import pipeline. A form exported with this PR, then re-imported, silently loses its privileges. May be intentionally export-only scope for this ticket — worth confirming/disclosing either way. |
| src/common/apis/formPrivilegesApi.js:17-19 | question | Sibling getFormPrivileges still forces 'text' on what looks like the same class of JSON endpoint that getFormPrivilegesFromUuid below it was just fixed for. Out of scope for this PR, but might be worth a follow-up ticket since it's the identical defect in the same file. |
| const formData = { formJson, translations }; | ||
| fileDownload(JSON.stringify(formData), `${fileName}.json`); | ||
| this.setMessage('Export Successfully', commonConstants.responseType.success); | ||
| getFormPrivilegesFromUuid(form.uuid) |
There was a problem hiding this comment.
nit: Translations and privileges are independent of each other (both only need form.uuid/formJson), so this could be a Promise.all([translationsPromise, privilegesPromise]) instead of a third nested .then()/.catch() level — Promise.all for combining independent calls is already established elsewhere in this directory (FormTranslationsContainer.jsx:102, several spots in FormBuilder.jsx/FormBuilderContainer.jsx). Would also collapse the now-tripled identical catch(() => this.setMessage('Export Failed', ...)) block down to two copies.
| fileDownload(JSON.stringify(formData), `${fileName}.json`); | ||
| this.setMessage('Export Successfully', commonConstants.responseType.success); | ||
| }) | ||
| .catch(() => { |
There was a problem hiding this comment.
nit: No test currently simulates a rejected getFormPrivilegesFromUuid call (with form/translations succeeding) to verify this catch fires 'Export Failed' and skips fileDownload. Worth an isolated test case for this new failure path.
| `formUuid=${formUuid}`; | ||
| } | ||
|
|
||
| getFormPrivilegesFromUuidUrl(formUuid) { |
There was a problem hiding this comment.
nit: FormPrivilegesPreviewGrid.jsx:50 already builds this identical URL string inline (${formBuilderConstants.getFormPrivilegesFromUuidUrl}?formUuid=${formUuid}). Could point that file at this new method (or at getFormPrivilegesFromUuid from the api layer) instead of leaving two copies of the same URL-building logic. Not blocking — low-cost follow-up.
Also, minor readability note: this method's name is one word away from the pre-existing getFormPrivilegesUuidUrl right above it, which actually returns the form endpoint (getFormFromUuid), not privileges — predates this PR, but worth a naming cleanup follow-up given they now sit side by side.
There was a problem hiding this comment.
Done with First comment.
For second one, actually previous function name getFormPrivilegesUuidUrl is not appropiate. We can take seperate tech debt task to correct it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
getFormPrivilegesFromUuidwhen exporting a form and embed them in the exported form JSON'sformJson.privilegesfield.getFormPrivilegesFromUuidUrltoUrlHelper.getFormPrivilegesFromUuidAPI call to parse JSON response instead of forcing'text'.Test plan
yarn testpasses locally (412/412 tests, 0 lint errors)FormList.spec.jsto assert the privileges endpoint is called and that the exported JSON includes the fetched privileges🤖 Generated with Claude Code