-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Feat: Add a step to to Request Physical Card form that collects a magic code #51135
Conversation
Note: It looks like BE currently doesn't throw errors with invalid validate code. |
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.
Onyx migration 's done. I mark this PR is ready so the linked issue won't be overdue. |
@NikkiWines What's the successful response? Can you give me the example? I'm seeing here's the error response, it merges |
Another thing, The error that's showing between assigned card sections and inside ValidateCodeActionModal are different, do we need to fix it as well? Screen.Recording.2024-11-01.at.14.17.49.mov |
@hungvu193 If we already requested validate code in another flow,
I think this is somehow related to #51663 but cannot confirm if it's expected (to prevent excessive API calls within a short duration). Can you confirm? |
Yes, It's to prevent excessive API calls. But I think we should also fix it, we should add optimistic data to our API that use |
Thanks. Please do. |
@hungvu193 what's the latest here, can @dominictb re-review now? |
Yes. I commented here |
Please also merge |
Merged main and addressed all your comments 😄 |
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.
All yours @NikkiWines
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.
Looks like this needs another web PR to fix the error handling. I've got a PR up for that now and I'll CP if we can CP it so it goes live tomorrow. Holding off on approving until then.
Flow looks good once that correct error is being returned 🪅
https://github.com/user-attachments/assets/f99ab563-0978-4d94-9c8e-a87671eb225e
However, it looks like the error for the Assigned cards
needs to be adjusted here so it's not just displaying Oops!...
- maybe we can display the same error (or just red dot?) in this case?
Backend PR for the error handling is on staging, will be deployed tomorrow 😊 |
Yeah, currently we're getting the error from |
Backend logic for the error handling is on prod |
@NikkiWines Did that BE logic handle the double error issue mentioned here? |
No - it just handles updating the error message that we return when getting a 401 from the However, I think this is probably ok for now. I'd like to not display anything other than the 🔴 in that scenario, rather than show the I'm going to double check to see if we have staging friendly QA steps for this command, because I don't think we want go through the real flow of a requesting a new card in this case - once i've got that settled I'll re-review and merge. |
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.
✋ 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/NikkiWines in version: 9.0.66-0 🚀
|
Confirmed this works as expected on staging: Screen.Recording.2024-11-25.at.13.23.31.mov |
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.66-8 🚀
|
Details
This PR added a validate code modal when user want to issue a physical card.
Fixed Issues
$ #50967
PROPOSAL: N/A
Tests
Ask @NikkiWines to do the steps for you or follow this slack guide and ensure you request to cancel the card before it ships by asking in Slack
Ship card
.Offline tests
N/A
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2024-11-01.at.14.16.51.mov
MacOS: Desktop
Desskotp.mov