CLM-8914 Cart Translations Issue

[AlexONeillTI]

Closes CLM-8914

Issue: Cart translations were not working in a deployed environment, but did work locally. Video of issue

The issue is with the i18nObject, as it was not picking up all translations. The issue is resolved by using the sourceDefaultNamespace to map translations instead.

I tested this using the Cart component as detailed in the ticket, but also tested the Catalog component to ensure that the translations were still working for other helium components. All looks good in testing.

Please let me know if there is any reason why we should use i18nObject instead of sourceDefaultNamespace.

[chrisvariety]

any way we can add automated test coverage for this? It's fairly critical that translations work, and it sounds like they weren't properly prior to this fix, so it would be good to ensure we don't regress here in the future. I think if we separated out the steps of reading/writing files we could test only the processing step, e.g.

const translations = await readTranslations();
const result = processTranslations(...); // this is what gets tested
await writeTranslations(result);

[AlexONeillTI]

any way we can add automated test coverage for this? It's fairly critical that translations work, and it sounds like they weren't properly prior to this fix, so it would be good to ensure we don't regress here in the future. I think if we separated out the steps of reading/writing files we could test only the processing step, e.g.

const translations = await readTranslations();
const result = processTranslations(...); // this is what gets tested
await writeTranslations(result);

@chrisvariety Good idea, should I include this in the scope of this ticket, or create a new ticket to handle this?

[chrisvariety]

@AlexONeillTI up to you and @rjschill87, my vote would be to include it in this ticket as we generally prefer to ship code changes with corresponding tests.

[rjschill87]

@AlexONeillTI Would definitely include tests within the scope of this ticket.

[AlexONeillTI]

@rjschill87
After inspecting the compiled files, the translations are not only prefixed with t, but also 'o', 'p', 'f', and 'i18n' for hydratedContent. I have added these to the parser.parseFuncFromString call.

I have also added the testing that @chrisvariety has requested. This involved splitting the code into three segments:

1. Reading the data
2. Processing the translations
3. Writing the data

If the translations exist, FINAL_TRANSLATIONS is defined and contains all the necessary translations. writeTranslations then writes the results to the translations file.

If the translations do not exist, then FINALTRANSLATIONS = {}, which now throws a new error: "Error('No translations exist!');"

I have tested these changes on https://alexoneilsandbox.thoughtindustries.com/cart.

[AlexONeillTI]

@rjschill87
Pushed a fix for the former part of CLM-8914.

The translation keys must be translated directly, rather than passed as a variable. I have moved localizedSortMapping into sort-selector.tsx, as it needs to use the useTranslation hook within a react comp.

Will I leave the constants.tsx file as is, or remove it entirely?

[rjschill87]

@AlexONeillTI If that was all that was in the constants.tsx file and it's not referenced anywhere else, you can remove it

[AlexONeillTI]

After investigating the compiled files, I realized that the string that parser.parseFuncFromString is trying to match is contained in .mjs files. I then checked the filepaths.js file, which contains filePathIsValid. This did not match the .mjs file. I have added this condition to the check and the translations are now being picked up correctly.

Let me know if there is any drawback to including the .mjs files in this check.

[rjschill87]

@AlexONeillTI Thanks for digging into that! Looks good -- just needs some tests and we should be good to go.

[AlexONeillTI]

@rjschill87 I pushed the test cases for parse-translations. Few things to note:

1. To use an accurate dist directory, I created a helium app and ran npm run build, and then took the compiled app's dist folder. I thought this was the most simple, but accurate test of translations.
2. I only included english for my test cases for two reasons - the sample compiled folder used an instance in English, and to include the example translations for all languages in the test file was leading to an unncasserily large file.
3. The largest thing to note - the usual flow of the deployment process goes as follows:

Read translations for instance (all keys), process translations (necessary keys), write necessary keys to translations.json, use them, and then reset locales/translations.json.

This makes isolating the testing of parse-translations a little tricky, as we never grab the translations from the instance or reset the translations file for the next time that the translations are processed.

I have done my best to test the read, process, and write operations with mock data that should raise an issue if something is broken or not working as expected. Lmk if this is sufficient.

[AlexONeillTI]

I had a sync with Aalto university yesterday and they showed me how their translations are not working on their dashboard page. This looked identical to this issue. Is there a possibility of revisiting this to see if it fixes their issue?

Can be seen in there production site here: https://aaltoproduction.eu.thoughtindustries.com/learn/dashboard

@chrisvariety @rjschill87

[chrisvariety]

It would be good to have @rjschill87 take another look at it, it seems like there were a lot of commits added after the previous review. The challenge when commits are added after review is we never really know when it's ready for re-review unless you request re-review explicitly.

[AlexONeillTI]

I have changes implemented and tested locally. When trying to push, I am getting that I don't have permission to push to helium repo.

I have messaged @haddadnj and am waiting for a reply. I will update you here when I have successfully pushed the changes.

[chrisvariety]

@AlexONeillTI fixed your permissions :)

[AlexONeillTI]

@AlexONeillTI fixed your permissions :)

Thanks Chris!

[AlexONeillTI]

Review now up to date. Addressed @rjschill87's comment regarding the call to the parserScript here: #220 (comment)

The test suite runs successfully and I also tested the latest changes by deploying an app with the Cart and Dashboard page to my sandbox. Both successfully reflect the translations in locales/translations.json