Updating Translate to Manage Pluralization

[brandonhenry]

Details

Fixed Issues

$ https://github.com/Expensify/App/issues/38614 PROPOSAL: https://github.com/Expensify/App/issues/38614#issuecomment-2007980630

Tests

1. Navigate to a chat room or report that contains messages with pluralized text related to the maximum number of participants (e.g., "You've selected the maximum number (2) of participants.").

2. Verify that the pluralized text is displayed correctly based on the count (e.g., "You've selected the maximum number (2) of participants." for count 2, and "You've selected the maximum number (5) of participants." for count 5).

3. Change the app's language to a different supported language (e.g., Spanish) and verify that the pluralized text is correctly translated and displayed based on the count.

4. Navigate to a chat room or report that contains messages with pluralized text related to expense counts (e.g., "2 expenses, 1 scanning").

5. Verify that the pluralized text is displayed correctly based on the count (e.g., "1 expense" for count 1, "2 expenses, 1 scanning" for counts 2 expenses and 1 scanning receipt).

6. Change the app's language to a different supported language (e.g., Spanish) and verify that the pluralized text is correctly translated and displayed based on the counts.

7. Navigate to a workspace settings page that shows the "Selected" count for members (e.g., "2 selected").

8. Verify that the pluralized text is displayed correctly based on the count (e.g., "1 selected" for count 1, "5 selected" for count 5).

9. Change the app's language to a different supported language (e.g., Spanish) and verify that the pluralized text is correctly translated and displayed based on the count.

10. Navigate to a workspace settings page that displays options related to distance rates (e.g., "Delete 2 rates", "Enable 3 rates", "Disable 1 rate", "Are you sure you want to delete these 4 rates?").

11. Verify that the pluralized text is displayed correctly based on the count for each option.

12. Change the app's language to a different supported language (e.g., Spanish) and verify that the pluralized text is correctly translated and displayed based on the count for each option.

• [x] Verify that no errors or warnings related to pluralization appear in the JavaScript console.
• [x] Verify that the pluralized text is displayed correctly based on the count across different contexts (maximum participants, expense counts, member selection, distance rates).

Offline tests

1. Put the device in offline mode or disconnect from the internet.
2. Navigate to chat rooms, reports, and workspace settings pages that contain pluralized text based on the examples mentioned in the "Tests" section.
3. Verify that the pluralized text is still displayed correctly based on the count, even in offline mode.
4. Change the app's language to a different supported language and verify that the pluralized text is still correctly translated and displayed based on the count in offline mode.

• [x] Verify that the pluralized text is displayed correctly based on the count, even in offline mode, across different contexts (maximum participants, expense counts, member selection, distance rates).

QA Steps

1. Navigate to a chat room or report that contains messages with pluralized text.
2. Verify that the pluralized text is displayed correctly based on the count.
3. Test with different counts (e.g., 0, 1, 2, 3, 10) to ensure the correct plural form is used for each case.
4. Change the app's language to each supported language and verify that the pluralized text is correctly translated and displayed based on the count in each language.
5. Verify that no errors or warnings related to pluralization appear in the JavaScript console.
6. Test on different platforms (Android, iOS, web) to ensure consistency in pluralization behavior.

• [x] Verify that the pluralized text is displayed correctly based on the count.
• [x] Verify that the pluralized text is properly cached and retrieved from the cache when revisiting the same screen.
• [ ]

PR Author Checklist

• [x] I linked the correct issue in the ### Fixed Issues section above
• [x] I wrote clear testing steps that cover the changes made in this PR
  • [x] I added steps for local testing in the Tests section
  • [x] I added steps for the expected offline behavior in the Offline steps section
  • [x] I added steps for Staging and/or Production testing in the QA steps section
  • [x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
  • [x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • [x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• [x] I included screenshots or videos for tests on all platforms
• [x] I ran the tests on all platforms & verified they passed on:
  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [x] iOS: Native
  • [x] iOS: mWeb Safari
  • [x] MacOS: Chrome / Safari
  • [x] MacOS: Desktop
• [x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [x] I followed proper code patterns (see Reviewing the code)
  • [x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • [x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • [x] I verified that comments were added to code that is not self explanatory
  • [x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • [x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • [x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • [x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • [x] I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • [x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • [x] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [x] I followed the guidelines as stated in the Review Guidelines
• [x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• [x] I verified that if a function's arguments changed that all usages have also been updated correctly
• [x] If any new file was added I verified that:
  • [x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• [x] If a new CSS style is added I verified that:
  • [x] A similar style doesn't already exist
  • [x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
• [x] If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
• [x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• [x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
• [x] If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
• [x] If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
  • [x] I verified that all the inputs inside a form are aligned with each other.
  • [x] I added Design label and/or tagged @Expensify/design so the design team can review the changes.
• [x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• [x] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

[jayeshmangwani]

@brandonhenry When PR will be ready for review, please add some unit tests for the Pluralization

[brandonhenry]

Will do @jayeshmangwani !

[brandonhenry]

Just wanted to leave an update here that I was working through some other PRs so haven't had that much time to put here. All done on those so this one will get my full attention this week

[brandonhenry]

thanks for you patience here team. had a lot of back and forth and with local build issues and other things going on, this one kind of fell behind.

I've refactored this a lot into a place I think looks good. mind taking a look? @jayeshmangwani @iwiznia

[melvin-bot[bot]]

@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

[jayeshmangwani]

@brandonhenry Above changes will crash the app when we pass the conciergePlaceholderOptions array to localize. Can you test on your side and let us know if the crash is happening on your side, too?

iOS:

https://github.com/Expensify/App/assets/35371050/e5465f51-fb0f-48f3-8517-b554b681b979

web:

[jayeshmangwani]

these lines of code crashing after the changes: https://github.com/Expensify/App/blob/0b2f55fefa8519b86f58730a85e851409f631524/src/pages/home/report/ReportActionCompose/ReportActionCompose.tsx#L201-L208

[jayeshmangwani]

Please update the localize test that includes the pluralized examples

[brandonhenry]

sure thing @jayeshmangwani - Let me check again now, thanks for the callout!

[brandonhenry]

so I did see the issue. fixing this and will have this back before EOD

[brandonhenry]

Still working through issues here

[brandonhenry]

@iwiznia @jayeshmangwani so the reason this was failing is because we for some reason have an array for this specific translation phrase. Every other translation in this file is a string. I think it should always be a string

[brandonhenry]

@jayeshmangwani @iwiznia

Alrighty, updated the function to handle that one off array translation. I also updated it so that we didn't have to alter phraseKey at all. Now, if you want to have multiple forms for a translation, you just return an object of Record<LDMLPluralRule, string> in en.ts and you are good to go.

example:

sayHello: (_count) => ({
          other: 'Say hello!',
          one: 'Say hello!!',
          two: 'Say hello!!!',
}),

we still need to pass in count for all the logic to work its magic under the hood. i don't think liner will like the unused variable 🤔

This works now though, no crashes or weirdness and the tests works

translate('reportActionsView.sayHello', {count: 1});

[jayeshmangwani]

@brandonhenry typecheck and test are failing , please fix

[brandonhenry]

on it @jayeshmangwani ! I will have this updated soon, hopefully before my lunch

[iwiznia]

@iwiznia @jayeshmangwani so the reason this was failing is because we for some reason have an array for this specific translation phrase. Every other translation in this file is a string. I think it should always be a string

Yeah, that seems wrong, saving an array in the translation is not really supported. cc @puneetlath

Alrighty, updated the function to handle that one off array translation.

What exactly did you do? I think we should undo that array, as that's not valid and instead:

• Transform them into an object
• Give each of the keys a name
• Do the random stuff using the object, which is a valid format for the translation

[brandonhenry]

@iwiznia sounds good. i added a condition to check for it but i totally am aligned with removing the array. Then I can remove this condition

[iwiznia]

Nice, yes, let's do that.

[brandonhenry]

Still working through this, sorry didn't have much time yesterday. Hoping to get that other array updated today

[brandonhenry]

this is looking better now @iwiznia @jayeshmangwani - removed that array from the list and tested things out again. looks good - let me know your thoughts

[brandonhenry]

i was testing with local changes (review prior pointed them out so i didn't commit this time). Probably better to write some unit tests to go with this @iwiznia

[brandonhenry]

@iwiznia i took a look into the typing myself and it definitely gets pretty dang deep and nested. I think the main issue, is that everything in en is not returning a string.

We fixed conciergePlaceholderOptions but we also need to fix other instances like this one so we always return a string and not some weird object..

Thoughts? I'm going to go ahead with this logic and start fixing types

[brandonhenry]

To be clear, the types are deeply nested due to our use of Flatten here.

we should be able to do something like type TranslationPaths = keyof EnglishTranslation; and be good, but since there are more than just strings being returned here, we need to do this for type safety.

Two options going fowrad:

1. move some of the types in en.ts to a different file so everything returns string
2. alter the objects in en.ts so every one returns a string
3. Keep as is and do type conversions / alter typing at a higher level

[brandonhenry]

I fixed things on my recent push

Edit: I ended up changing what i went with an alternative route which looks to be working other than breaking the way we call the function. Looking into that last thing today...

[brandonhenry]

sooo looks like i only broke some of the types.. 🙃

[jayeshmangwani]

sooo looks like i only broke some of the types.. 🙃

Yes indeed! 😃

[brandonhenry]

@iwiznia @jayeshmangwani alrighty, this is ready for review again. I altered phraseParameters so that it is no longer a rest parameter (thus resulting in never[]) and instead force use of an object that contains all params you want to provide.

Also, wrote some simple unit tests.

Let me know your thoughts

[blazejkustra]

@brandonhenry Remember to tag me when I should review the PR TS-wise

[jayeshmangwani]

@brandonhenry I am testing your latest code, and the changes have caused a regression; concierge placeholder is not showing on my side; can you check that it's working for you?

[jayeshmangwani]

@brandonhenry please merge the main and push again; typescript is failing your branch locally