-
Notifications
You must be signed in to change notification settings - Fork 112
(refactor) O3-4964 : Refactor Translation Builder - preserve edits & sync keys incrementally #825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@Bharath-K-Shetty looks like the e2e tests are failing, because the translations gets added to the form schema. Have you looked into this? |
Yeah, Nethmi. I was looking into it. But while adding some fixes and rerunning the tests, it started failing due to another issue related to the name field, which isn’t actually affected by this PR.
|
Looking at the PR's failing action - https://github.com/openmrs/openmrs-esm-form-builder/actions/runs/17126732891/job/48580139556?pr=825, I see a different failing error, where are you getting the error in your screenshot from? Hard to say why the error in your screenshot happens, I'll try running locally and see |
|
@Bharath-K-Shetty just fyi, the remaining failing e2e tests are related to your changes |
I know @NethmiRodrigo ,but i was unable to test the changes i made. |
|
@NethmiRodrigo fixed the test cases.Ig all good now..!! |
| // Removed the onUpdateSchema call from here | ||
| // The schema should only be updated on user-initiated actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: We can remove this comment since it doesn't explain the functionality. Lets instead add a comment at the top of this useEffect to explain its purpose
| // Removed the onUpdateSchema call from here | |
| // The schema should only be updated on user-initiated actions | |
| // Removed the onUpdateSchema call from here | |
| // The schema should only be updated on user-initiated actions |
| return formSchema ? extractTranslatableStrings(formSchema) : {}; | ||
| }, [formSchema]); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thought: Seems like this is something that needs to be run whenever either a translation string gets updated or the language gets changed. In that case can't we extract this into a function and just call it in hamdleUpdateValue and languageChanger. That way we can avoid using a useEffect altogether, they can be cause infinite renders and would be very hard to debug.

Requirements
Summary
This PR refactors the Translation Builder to improve translation retention and schema sync:
Add per-language in-memory caching to keep user edits across language switches.
Introduce mergeTranslations(local, backend, fallback) to deterministically merge values: local > backend > fallback.
Add a schema-driven incremental sync effect that adds new keys and removes deleted ones without overwriting existing translations.
Update language switcher to reuse cached translations and to fetch+merge only when needed.
Ensure translations saved to formSchema.translations reflect merged state.
Screenshots
Related Issue
https://openmrs.atlassian.net/browse/O3-4964
Other