-
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
base: main
Are you sure you want to change the base?
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 |
Co-authored-by: Dominic <[email protected]>
Co-authored-by: Dominic <[email protected]>
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeScreen.Recording.2024-11-15.at.11.56.20-compressed.moviOS: mWeb SafariScreen.Recording.2024-11-15.at.11.27.17-compressed.movMacOS: Chrome / SafariScreen.Recording.2024-11-15.at.11.18.41-compressed.movMacOS: Desktop |
@hungvu193 Are #51135 (comment) and #51135 (comment) expected? Or are we waiting for BE changes? |
I think they're expected for now. I believe we don't want to block existing user from using this feature at the moment because we haven't released this PR yet. cc @NikkiWines |
This comment was marked as resolved.
This comment was marked as resolved.
But seems like it's this one #51135 (comment) and a BE bug. |
I think it is a BE bug as well. |
The above issue ^ together with the rate limit of |
@dominictb you might be able to test more easily by following the setup steps from this PR? @hungvu193 it might be helpful to incorporate something like this into the QA steps as well |
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.
Couple of comments
src/components/ValidateCodeActionModal/ValidateCodeForm/BaseValidateCodeForm.tsx
Outdated
Show resolved
Hide resolved
Addressed all the comments |
@hungvu193 you've got conflicts |
@dominictb can you re-review please 🙇 |
(validateCode: string) => { | ||
setCurrentCardID(cardToBeIssued?.cardID.toString()); | ||
const updatedPrivatePersonalDetails = GetPhysicalCardUtils.getUpdatedPrivatePersonalDetails(draftValues, privatePersonalDetails); | ||
Wallet.requestPhysicalExpensifyCard(cardToBeIssued?.cardID ?? -1, session?.authToken ?? '', updatedPrivatePersonalDetails, validateCode); |
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.
I think this is the line causing the -1
problem. I can still reproduce it.
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.
Please retest before requesting another review round.
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.
Sure. I'll do it shortly
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.
We clear the error when modal is hide, unfortunately we used the old cardID value for it, which caused the issue, can you verify it now? Ty
@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 |
const [draftValues] = useOnyx(ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM_DRAFT); | ||
const [account] = useOnyx(ONYXKEYS.ACCOUNT); | ||
const [isActionCodeModalVisible, setActionCodeModalVisible] = useState(false); | ||
const [formData] = useOnyx(ONYXKEYS.FORMS.REPORT_PHYSICAL_CARD_FORM); | ||
const domainCards = CardUtils.getDomainCards(cardList)[domain] || []; | ||
const cardToBeIssued = domainCards.find((card) => !card?.nameValuePairs?.isVirtual && card?.state === CONST.EXPENSIFY_CARD.STATE.STATE_NOT_ISSUED); | ||
const cardID = cardToBeIssued?.cardID.toString() ?? '-1'; |
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.
Please remove this intermediary variable and replace all its usages to avoid confusion (e.g., why cardID
somewhere and currentCardID
elsewhere).
// Current step of the get physical card flow should be the confirmation page; and | ||
// Card has NOT_ACTIVATED state when successfully being issued so cardToBeIssued should be undefined | ||
// -1 is not a valid cardID, we don't need to clean up the form value in that case. | ||
if (!isConfirmation || !!cardToBeIssued || !currentCardID || currentCardID === '-1') { |
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.
If so, do we still need currentCardID === '-1'
?
// so that no stale data is left on Onyx | ||
FormActions.clearDraftValues(ONYXKEYS.FORMS.GET_PHYSICAL_CARD_FORM); | ||
Wallet.clearPhysicalCardError(currentCardID); | ||
Navigation.navigate(ROUTES.SETTINGS_WALLET_DOMAINCARD.getRoute(currentCardID.toString())); |
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.
Navigation.navigate(ROUTES.SETTINGS_WALLET_DOMAINCARD.getRoute(currentCardID.toString())); | |
Navigation.navigate(ROUTES.SETTINGS_WALLET_DOMAINCARD.getRoute(currentCardID)); |
Please also merge |
Details
This PR added a validate code modal when user want to issue a physical card.
Fixed Issues
$ #50967
PROPOSAL: N/A
Tests
Prerequisite: Your account needs at least one physical card added.
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