App: [HOLD for payment 2023-05-25] [$1000] Dev: console error when emoji suggestions list open

[@mollfpr] The PR that introduced the bug has been identified. Link to the PR: [@mollfpr] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:

No offending PR.

[@mollfpr] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

The regression step should be enough.

[@mollfpr] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Propose regression step

1. Login with any account
2. Go to any chat
3. Type in semicolon with emoji prefix (i.e. :sm)
4. Verify that the error Invalid prop preferredSkinToneIndex of type string supplied to EmojiSuggestions, expected number does not appear in the JS console
5. 👍 or 👎

@gadhiyamanan @tienifr @mollfpr - offers sent

@mollfpr can you complete to above check list?

Proposal

Please re-state the problem that we are trying to solve in this issue.

It shows console error(Invalid prop preferredSkinToneIndex of type string supplied to EmojiSuggestions, expected number. ) when opening emoji suggestions list.

What is the root cause of that problem?

In here https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/components/EmojiSuggestions.js#L47 we require preferredSkinToneIndex to be a number.

While the preferredSkinTone that we get from Onyx could sometimes be default, causing the prop type mismatch error.

default value for the preferredSkinTone was legacy, it’s not a good practice to let a value being of two different types (string and number). We recently have made the move to standardize the preferredSkinTone to -1, as can be seen here https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/CONST.js#L677, also if we select the default skin tone in the EmojiPicker, it will also update to -1, which is correct.

But it seems that when a new account is created, the preferredSkinTone is still initially set to default, causing this issue.

What changes do you think we should make in order to solve the problem?

Since this is due to legacy logic, we shouldn’t penetrate this legacy type into the codebase by modifying the type of the preferredSkinToneIndex. We should instead get rid of it at the boundary where we retrieve the data from Onyx.

A similar scenario has been done earlier in here https://github.com/Expensify/App/blob/main/src/libs/actions/Report.js#L44-L46 where the preferredSkinTone is converted to -1 if it’s nully or 'default'. The code comment there also further explains the situation with the 'default' value:

// the preferred skin tone is sometimes still "default", although it
// was changed that "default" has become -1.

We should do the same here. We need to introduce this common method to get the correct number value of preferredSkinTone from the legacy value.

const getPreferredSkinToneIndex = (val) => {
   if (!_.isNull(val) && Number.isInteger(Number(val))) {
       return val;
   }
   
   return -1;
}

Then use it as the selector here https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/pages/home/report/ReportActionCompose.js#L1204 where we retrieve value from Onyx

selector: getPreferredSkinToneIndex,

We should do it from the boundary like this to avoid rerendering in the component.

This should be enough to fix the issue here, some further cleanups can be considered:

• The mentioned logic here https://github.com/Expensify/App/blob/main/src/libs/actions/Report.js#L44-L46 to solve the same issue in another place, can be replaced by the above common function.
• The withOnyx in other functions can use the selector above as well to limit the legacy preferredSkinTone value at the boundary
• The BE can be changed a bit so that for new accounts it will have -1 as the preferredSkinTone instead of default, thus limiting the legacy 'default' value to only old accounts.

What alternative solutions did you explore? (Optional)

NA

Hi @muneebkhan4, could you update your proposal with our PROPOSAL_TEMPLATE.md? Thanks!

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

📣 @muneebkhan4! 📣 Hey, it seems we don’t have your contributor details yet! You’ll only have to do this once, and this is how we’ll hire you on Upwork. Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don’t already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It’s necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It’ll look like this. If you don’t already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>