-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[CP Staging] Revert "fix: treat certain US numbers as invalid" #33496
Conversation
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Any bug you found related to phone number? |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@aimane-chnaif this one #33326 the app is crashing for accounts which have lots of chats with users who have phone numbers as their primary login |
@puneetlath 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] |
oh that's bad. I see 2 PRs are being reverted. Which PR exactly is offending one? |
@thienlnam looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Straight reverts |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
…ne-number [CP Staging] Revert "fix: treat certain US numbers as invalid" (cherry picked from commit 5bf78ea)
🚀 Cherry-picked to staging by https://github.com/mountiny in version: 1.4.16-4 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.16-5 🚀
|
Details
Reverts #31726 and #31010
We have been trying to trace down the rootcause of App crashing for some Expensifiers when opening a search component and these two PRs seem to be the root cause.
When the search page is opened, the option rows are rendered and we run the checks for phone numbers in case the user has a primary login as phone number. Adding this regex and check in to the loop seems to have increased the load so much that for the biggest of account who know many users with phone number as their primary login this
Fixed Issues
$ #33326
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
Will be tested by the largest accounts in Expensify to make sure the Search page is not crashing the App
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Straight reverts, tested in the Slack thread linked above
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop