fix(document): do not reset form fields when changing document type - #1336
Conversation
|
Suite des discussions de #1334. |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1336.westeurope.azurestaticapps.net |
|
En effet, le champ parent est sauvegardé et continue de s'appliquer (il est conservé pour ne pas être perdu si on repasse sur un type qui en a besoin). Du coup la seule solution c'est de vider le champ parent si on bascule sur un document qui n'a pas ce champ (si on repasse sur un type article, il faudra le remettre à la main du coup). Fixed |
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1336.westeurope.azurestaticapps.net |
ClemRz
left a comment
There was a problem hiding this comment.
Issues (Must Fix)
-
[CreateDocument.js:33-39] When the document type is
Event,filterDocumentPayloadstrips thefileskey from the returned object (sincefilesis not in the Event allowed fields). The subsequent destructuringconst { files, ... } = filteredwill yieldfiles === undefined, andfiles.forEach(...)on line 39 will throw aTypeError: Cannot read properties of undefined (reading 'forEach'). The same issue exists inUpdateDocument.js:36wherefor (const file of files)would also crash onundefined.Since Events don't render the file upload form,
document.fileswill typically be[], but if a user adds files while on another type and then switches to Event before submitting, the crash path is reachable.Consider defaulting
filesto an empty array during destructuring:const { files = [], selectOptionAuthorizationDocument, ...rest } = filtered;Apply this in both
CreateDocument.jsandUpdateDocument.js.
Suggestions (Should Consider)
-
[documentTypeHelpers.js:210-246] The
filterDocumentPayloadfunction has no guard for whentypeisundefinedor-1(theUNKNOWNtype used as the initial/default state). IffilterDocumentPayloadis ever called before the user selects a type, it would fall into theelsebranch (full field set), which is probably fine — but it might be worth adding a comment or an early return to make this intentional behavior explicit. -
[AddFileForm/index.jsx:115-126] The change to
isLicenseForced/isAuthForcedlogic is a behavioral change beyond what the PR title describes. Previously,isLicenseForcedwasparent !== null && license !== nullandisAuthForcedwasparent !== null && selectOptionAuthorizationDocument !== null. Now both depend onauthorizationDocument !== null. This means:- A document with a parent and a license set (but no authorization document) will no longer show the license as forced.
- A document with a parent and a radio option selected (but no authorization document linked) will no longer show the auth as forced.
If this is intentional (fixing a separate bug), consider mentioning it in the PR description so reviewers understand the motivation. If it's unintentional, the old conditions may need to be preserved.
-
[AddFileForm/index.jsx:120-126] The
useEffectthat resetsauthorizationDocumentwhen the parent changes runs on every render wheredocument.parentreference changes (even if the id is the same). The ref-based guard (prevParentIdRef) handles this correctly, but there's a missing blank line between theisAuthForcedblock and theuseEffectblock which hurts readability. Also, hooks are typically grouped together before any derived values — here theuseEffectis sandwiched between derived state and theacceptvariable, which breaks the conventional hooks-first ordering.
Nitpicks (Optional)
-
[DocumentTypeSelect.jsx:35] Destructuring
{ isArticle, isIssue }fromdocumentTypeHelpersat module level is fine, but it's a different pattern from howFormContent.jsxuses these helpers (destructuring inside the component body). Not blocking, but worth noting for consistency across the codebase. -
[UpdateDocument.js:120] The
idis destructured fromdocAttributesbefore filtering (const { id } = docAttributes), thenfilterDocumentPayload(docAttributes)is called which also includesidin the output (sinceidis inBASE_PAYLOAD_FIELDS). This is correct but slightly redundant —idis used for the URL and also ends up in the body. No action needed, just noting for clarity.
eff6807 to
d237080
Compare
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1336.westeurope.azurestaticapps.net |
ClemRz
left a comment
There was a problem hiding this comment.
Suggestions (Should Consider)
-
[AddFileForm/index.jsx:115-121] The
useEffectthat resetsauthorizationDocumentwhen the parent changes usesdocument.parentas a dependency, which is an object reference. Every timesetDocumentproduces a new state object (even for unrelated attribute changes),document.parentwill be a new reference if it's a non-null object — though theprevParentIdRefguard correctly prevents spurious resets. Consider usingdocument.parent?.idas the dependency instead, which is a primitive and avoids the effect body running at all on unrelated updates:const parentId = document.parent?.id ?? null; useEffect(() => { if (prevParentIdRef.current === parentId) return; prevParentIdRef.current = parentId; updateAttribute('authorizationDocument', null); }, [parentId, updateAttribute]);
-
[AddFileForm/index.jsx:113-125] The behavioral change to
isLicenseForced/isAuthForcedis a separate fix from the main PR goal (preserving form fields on type change). The old logic checkeddocument.license !== nullanddocument.selectOptionAuthorizationDocument !== nullrespectively; the new logic checksdocument.authorizationDocument !== nullfor both. This appears to fix the bug reported by @urien (field getting stuck as disabled after switching types), but it would help future readers if the PR description mentioned this as a deliberate fix and explained the reasoning — especially since it changes when the license/auth radio group becomes disabled. -
[documentTypeHelpers.js:210] Minor robustness note:
filterDocumentPayloaddestructurestypefromdocAttributeswithout a guard. If called withundefinedornull(e.g., due to a programming error upstream), it would throw. Since the comment already notes thatUNKNOWN (-1)falls through intentionally, this is low-risk, but a defensiveconst { type = DocumentTypes.UNKNOWN } = docAttributes ?? {};would make it bulletproof.
Nitpicks (Optional)
-
[DocumentTypeSelect.jsx:35] Destructuring
{ isArticle, isIssue }at module level is fine and arguably cleaner than doing it inside the component body (avoids re-destructuring on every render). If the team prefers this pattern going forward, it might be worth adopting it consistently across other files that usedocumentTypeHelpers. -
[documentTypeHelpers.js:190-248] The field group constants (
BASE_PAYLOAD_FIELDS,LANGUAGE_PAYLOAD_FIELDS, etc.) are well-organized. Consider adding a brief JSDoc comment onfilterDocumentPayloadexplaining that the field groups mirror the conditional rendering inFormContent.jsx— this makes it easier for future maintainers to keep them in sync.
Follow-up fixesAuthorization section locking (
|
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1336.westeurope.azurestaticapps.net |
3bdb9e6 to
b896ae8
Compare
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1336.westeurope.azurestaticapps.net |
b896ae8 to
9285c71
Compare
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1336.westeurope.azurestaticapps.net |
ClemRz
left a comment
There was a problem hiding this comment.
Suggestions (Should Consider)
-
[documentTypeHelpers.js:225-233] The
AuthorizationToPublishbranch includesFILE_PAYLOAD_FIELDS(which containslicense,selectOptionAuthorizationDocument,authorizationDocument), butFormContent.jsxrenders<AddFileForm showAuthorization={false} />for that type — meaning the license selector and authorization radio group are never shown. These fields will benulland harmlessly skipped bybuildFormData, but it's a minor semantic mismatch between "fields relevant to the type" and what's actually editable. Consider adding a brief comment in theAuthorizationToPublishbranch noting thatlicense/selectOptionAuthorizationDocumentare included for structural consistency withFILE_PAYLOAD_FIELDSeven though they're not user-editable for this type. -
[UpdateDocument.js:117-123] In
updateDocumentWithNewEntities, the filtered payload is sent asJSON.stringify(body). Unlike theupdateDocumentpath (which usesbuildFormDatathat skips nulls),JSON.stringifywill include null-valued fields in the JSON body. This means fields likeparent: null,pages: null,issue: ""will be sent to the API for types that don't use them. This is likely fine (the API should ignore irrelevant nulls), but it's worth noting the behavioral difference between the two update paths. If the API ever interpretsnullas "clear this field," it could produce unintended side effects for the duplicates merge flow.
Nitpicks (Optional)
-
[AddFileForm/index.jsx:116-121] The
useEffectthat resetsauthorizationDocumenton parent change is well-placed and theprevParentIdRefpattern is solid. One minor readability note: the effect runs on mount withprevParentIdRef.current === parentId(both initialized from the same value), so it's a no-op on mount — which is correct. A one-line comment like// Skip initial mount — only react to actual parent changeswould make this immediately obvious to future readers. -
[documentTypeHelpers.js:190-195] The field group constants are well-organized. Consider adding
Object.freeze()to prevent accidental mutation, or simply marking them with a@constJSDoc tag to signal intent — though this is purely stylistic given they're module-scopedconstarrays.
All previous review concerns (the files = [] default, the useEffect dependency on a primitive parentId, and the behavioral change documentation) have been addressed in the latest push. The approach is sound — preserving form state across type switches while filtering the payload at submit time is a clean separation of concerns. The follow-up fixes for the authorization section locking are well-explained in the PR comments.
LGTM 👍
|
Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-rock-0d4f87503-1336.westeurope.azurestaticapps.net |

🤔 What
🤷♂️ Why
Previously, switching the document type called
resetContext(...)inDocumentTypeSelect, which wiped the entire form state back to defaults — keeping onlytitle,mainLanguage,mainLanguageName, andauthors. This meant:🔍 How
1. Stop resetting on type change (
DocumentTypeSelect.jsx)Replaced the
resetContext({ type, title, mainLanguage, ... })call inhandleSelectwith a singleupdateAttribute('type', newDocType).updateAttributewas already exposed byDocumentFormContext, so no context shape change was needed. Field visibility inFormContent.jsxis already conditional on the type, so hidden fields are never rendered — the data just stays alive in state until the user switches back.2. Filter the API payload at submit time (
documentTypeHelpers.js)Added
filterDocumentPayload(docAttributes)— a pure function that, given the current document state, returns only the fields that are actually visible/relevant for the current type:title,description,datePublication,iso3166,authors(no files, no language, no advanced metadata)parent,pages,library,issueare included but will benullfor types that don't use them, sobuildFormDataalready skips themThe field groups mirror exactly the conditional rendering in
FormContent.jsx, including the globalAuthorsSection(rendered for all non-unknown types) and the Advanced Metadata accordion (rendered for all types except Event and Authorization).3. Apply the filter before building FormData
filterDocumentPayloadis called at the top ofpostDocument,updateDocument, andupdateDocumentWithNewEntitiesbefore theFormData/ JSON body is assembled.🔗 Related API PR
None.
🧪 Testing
parent,pages,library,issue.files,mainLanguage,license,identifier,editor,subjects,creatorComment.📸 Previews
N/A — no visual change; the fix affects state management and API payload construction.