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.

Sign up for GitHub

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

Jump to bottom

[No QA] fix: Fixed error with frequently used emojis not showing up #7255

Merged
merged 2 commits into from
Jan 17, 2022
Merged

[No QA] fix: Fixed error with frequently used emojis not showing up #7255

merged 2 commits into from
Jan 17, 2022

Conversation

mananjadhav
Copy link
Collaborator

@mananjadhav mananjadhav commented Jan 15, 2022 •
edited
Loading

Details

Fixed Issues

$ #7074

Tests

  1. Tested frequently used emojis in the EmojiPicker
  • Verify that no errors appear in the JS console

QA Steps

  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Mobile Web

Screenshot 2022-01-15 at 9 47 10 AM

Desktop

Screenshot 2022-01-15 at 9 50 09 AM

iOS

image

Android

Screenshot 2022-01-15 at 10 03 50 AM

@mananjadhav mananjadhav requested a review from a team as a code owner January 15, 2022 04:15
@MelvinBot MelvinBot requested review from madmax330 and removed request for a team January 15, 2022 04:15
@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Jan 15, 2022 •
edited
Loading

@stitesExpensify @parasharrajat @rushatgabhane When working with the last EmojiPicker PR, I introduced a bug where frequently used emojis stopped showing up. Adding a fix for this and I am hoping if this can be CPed to staging.

rushatgabhane
rushatgabhane suggested changes Jan 15, 2022
View reviewed changes
Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing and pushing the fix!
Small change requested

@mananjadhav
Copy link
Collaborator Author

PR updated

@rushatgabhane rushatgabhane mentioned this pull request Jan 15, 2022
Closed
rushatgabhane
rushatgabhane approved these changes Jan 17, 2022
View reviewed changes
Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
cc: @stitesExpensify

stitesExpensify
stitesExpensify approved these changes Jan 17, 2022
View reviewed changes
Copy link
Contributor

@stitesExpensify stitesExpensify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@stitesExpensify stitesExpensify merged commit 73cc4ab into Expensify:main Jan 17, 2022
@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@roryabraham
Copy link
Contributor

Whoa, it seems like the CP label may not have worked here ... 🤔 I've never seen this so I'm going to investigate.

@roryabraham
Copy link
Contributor

roryabraham commented Jan 17, 2022 •
edited
Loading

Okay, so here's what happened:

  1. This pull request was merged before the CP Staging label was added, triggering this preDeploy.yml workflow run.
  2. The merged pull request was fetched here and did not have the CP Staging label at the time. It therefore was not CP'd.
  3. The CP Staging label was added at some point after the PR was already merged (and after it had been fetched in the preDeploy.yml workflow), which triggered a warnCPLabel.yml workflow run.

So two things here:

  1. I am manually CPing this to staging here, so that's an easy fix.
  2. There is a race condition afoot here, and it will be good to resolve to clarify this situation in the future. I've documented it along with a proposed solution here

OSBotify pushed a commit that referenced this pull request Jan 17, 2022
@stitesExpensify @OSBotify
Merge pull request #7255 from mananjadhav/fix/emojis-freq-used-error
d4735c0 
[No QA] fix: Fixed error with frequently used emojis not showing up

(cherry picked from commit 73cc4ab)
@OSBotify OSBotify mentioned this pull request Jan 17, 2022
Merged
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @roryabraham in version: 1.1.30-2 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@roryabraham roryabraham mentioned this pull request Jan 17, 2022
Closed
@OSBotify OSBotify mentioned this pull request Jan 18, 2022
Closed
66 tasks
@botify botify mentioned this pull request Jan 18, 2022
Closed
@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.30-3 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify OSBotify mentioned this pull request Jan 19, 2022
Closed
56 tasks
@OSBotify OSBotify mentioned this pull request Aug 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@parasharrajat parasharrajat parasharrajat left review comments

@stitesExpensify stitesExpensify stitesExpensify approved these changes

@rushatgabhane rushatgabhane rushatgabhane approved these changes

@madmax330 madmax330 Awaiting requested review from madmax330

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mananjadhav @OSBotify @roryabraham @parasharrajat @rushatgabhane @stitesExpensify