Skip to content
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

Android - Emoji - Change default skin tone/list of frequently used doesn't work #10106

Closed
kbecciv opened this issue Jul 26, 2022 · 12 comments
Closed
Assignees

Comments

@kbecciv
Copy link

kbecciv commented Jul 26, 2022

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Issue found when executing PR #9677

Action Performed:

  1. Go to App and login with any account
  2. Access any chat you prefer
  3. Click on Emoji
  4. Click on Change default skin tone
  5. Send multiple emoji to add them to the list of most used emoji
  6. Log out

Expected Result:

Functions Change default skin tone should be able to selected

Actual Result:

Change default skin tone doesn't work

Workaround:

Unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.1.86.0

Reproducible in staging?: Yes

Reproducible in production?: No

Email or phone of affected tester (no customers): any

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug5663029_Record_2022-07-26-05-06-50_4f9154176b47c00da84e32064abf1c48.mp4
Bug5663029_Record_2022-07-26-05-03-33_4f9154176b47c00da84e32064abf1c48.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Jul 26, 2022
@OSBotify
Copy link
Contributor

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

@marcaaron
Copy link
Contributor

This seems like two separate bugs. Was unable to reproduce the frequently used emojis bug. But the default skin tone feature appears to be broken completely.

@mvtglobally
Copy link

@marcaaron Do you want us to log them separately?

@marcaaron
Copy link
Contributor

Yes please.

@luacmartins
Copy link
Contributor

I was unable to reproduce the frequently used emojis bug, but setting the skin tone seems broken. It immediately reverts back to the default.

cc @danieldoglas since you seem to have implemented the updatePreferredSkinTone command.

@luacmartins
Copy link
Contributor

luacmartins commented Jul 27, 2022

hmm changing the return type from string to integer here seems to fix the issue, but I'm not sure why since we seem to either accept string or int in App 🤔

@luacmartins
Copy link
Contributor

The issue seems to be this === here. If we are passing a string instead of int, we should not check for type. So a few possible solutions:

  1. Update Web-E to send int instead of string
  2. Update that equality to just ==
  3. Update the skinTone key here to strings

@luacmartins
Copy link
Contributor

Hmm so 3 is not really an option since that NVP seems to be saved as an int and returned as such in OpenApp, which would break that functionality. I'm not a big fan of using ==, so I think the solution is to remove the blocker label and make these changes in Web-E.

Web-E PR here. Removing blocker label.

@luacmartins luacmartins added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jul 27, 2022
@luacmartins luacmartins self-assigned this Jul 27, 2022
@danieldoglas
Copy link
Contributor

danieldoglas commented Jul 27, 2022

That's my bad. After some discussion, I decided that the changes for NVPs were not necessary to test in all platforms before opening the PR since they were changes on the API, but it looks like I was wrong.

@luacmartins
Copy link
Contributor

No worries! I created a PR to fix this in Web-E.

@kavimuru
Copy link

@marcaaron A separate issue is logged for "frequently used emoji's are not saved" #10134

@luacmartins
Copy link
Contributor

Cool. We have a fix for the default skin tone and a separate one for the frequently emoji one. I'll close this one out then! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants