-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Fix validated code bug on replace card flow #61666
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
Conversation
@dominictb 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] |
🚧 @mountiny has triggered a test app build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Tested! Magic code piece worked great. |
@dominictb Kindly bump |
Cool so we're gonna only fix the second bug mentioned here. On it now. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@DylanDylann I didn't receive the magic code after confirming the card shipping details: Screen.Recording.2025-05-10.at.10.49.54-compressed.mov
|
@DylanDylann can you take a look at the issue @dominictb raised. Are you able to reproduce? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me but lets confirm the testing steps again
@dominictb I can't reproduce your bug, please reset cache and try again OK.mov |
@mountiny @joekaufmanexpensify Could you help to confirm the below step? QA Steps: Pre: The account already had a shipped card (now pending activation)
|
Thanks @DylanDylann. That issue is not reproducible but found another one.
Screen.Recording.2025-05-12.at.18.13.48.movThis also happens on |
@dominictb Did it happen on the main? I am not sure that my change relates to that flow |
@DylanDylann This also happens on main but since it's blocking testing this PR, can you take a look? |
@dominictb Do you have any card that is pending activation? |
@dominictb Let's use this new steps |
@dominictb About this bug, It caused by "SetPersonalDetailsAndShipExpensifyCards API is triggered with empty response" cc @mountiny |
This is expected for you as your workspace has the testing plaid bank account so its not actually shipping the card |
@DylanDylann seems like this is looking good and we can actually get it to staging and test there, right? |
@mountiny Yeahh, All good now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. Let's QA on staging.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.45-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.45-0 🚀
|
Tested and works well! |
🚀 Deployed to production by https://github.com/francoisl in version: 9.1.45-21 🚀
|
Explanation of Change
Fixed Issues
$ #61239
PROPOSAL: #61239 (comment)
Tests
same as QA Steps
Offline tests
None
QA Steps
Pre: The account already had a shipped card (now pending activation)
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectioncanBeMissing
param foruseOnyx
toggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
Screen.Recording.2025-05-08.at.15.25.24.mov
Android: mWeb Chrome
Screen.Recording.2025-05-08.at.15.25.24.mov
iOS: Native
Screen.Recording.2025-05-08.at.15.23.11.mov
iOS: mWeb Safari
Screen.Recording.2025-05-08.at.15.21.26.mov
MacOS: Chrome / Safari
Screen.Recording.2025-05-08.at.15.07.25.mov
MacOS: Desktop