feat(expensify-card): add get physical card button and routes #28453
Conversation
|
cc @grgia |
| @@ -817,6 +817,28 @@ export default { | |||
| availableSpend: 'Remaining spending power', | |||
| virtualCardNumber: 'Virtual card number', | |||
| physicalCardNumber: 'Physical card number', | |||
| getPhysicalCard: 'Get physical card', | |||
| }, | |||
| getPhysicalCard: { | |||
There was a problem hiding this comment.
Can I get this translated to Spanish, please?
There was a problem hiding this comment.
We might have some of these already and could move them to common
Lines 1044 to 1046 in 9e35d91
src/libs/actions/Wallet.js
Outdated
| @@ -321,6 +321,31 @@ function answerQuestionsForWallet(answers, idNumber) { | |||
| ); | |||
| } | |||
|
|
|||
| function requestPhysicalExpensifyCard(cardID, updatedPersonalDetails) { | |||
| // TODO: confirm required parameters | |||
There was a problem hiding this comment.
Can you tell me what parameters are missing here and confirm if the onyx data that is being sent it the correct one? Thanks
| if (isConfirmation) { | ||
| // TODO: Use cardID and pass formatted formData here | ||
| Wallet.requestPhysicalExpensifyCard('', formData); | ||
| // TODO: Redirect user to the domain card page (?) |
There was a problem hiding this comment.
Should the user be redirected to the domain card page on submission?
There was a problem hiding this comment.
Yes, let's return to the card page.
src/libs/Navigation/Navigation.js
Outdated
| @@ -255,6 +256,39 @@ function setIsNavigationReady() { | |||
| resolveNavigationIsReadyPromise(); | |||
| } | |||
|
|
|||
| /** | |||
| * @param {Array<String>} loginList | |||
| * @returns {String} phone number, if there is one | |||
There was a problem hiding this comment.
if it can be undefined should we have a union here to account for that?
src/ROUTES.ts
Outdated
| route: 'settings/profile/contact-methods/:contactMethod/details', | ||
| getRoute: (contactMethod: string) => `settings/profile/contact-methods/${encodeURIComponent(contactMethod)}/details`, | ||
| }, | ||
| SETTINGS_CONTACT_METHOD_DETAILS: { route: 'settings/profile/contact-methods/:contactMethod/details', getRoute: (contactMethod: string) => `settings/profile/contact-methods/${encodeURIComponent(contactMethod)}/details` }, |
| if (isConfirmation) { | ||
| // TODO: Use cardID and pass formatted formData here | ||
| Wallet.requestPhysicalExpensifyCard('', formData); | ||
| // TODO: Redirect user to the domain card page (?) |
There was a problem hiding this comment.
Yes, let's return to the card page.
|
assigning @sobitneupane for C+ review |
Sorry @grgia, how can I make it clear for you? 😄 |
pac-guerreiro
force-pushed
the
feature/add-get-physical-card-button-and-necessary-routes
branch
2 times, most recently
from
c040fa4 to
8501763
Compare
|
@grgia @joekaufmanexpensify I require these translated to spanish:
|
|
@pac-guerreiro I've requested translation assistance for these internally, and will LYK when I hear back. |
|
@pac-guerreiro I was advised that a good number of these translations already exist in the app. Mind looking into that first, and let me know which ones you aren't able to find? |
|
I won't be available next week. If any action is required from my end, please feel free to reassign the issue. |
pac-guerreiro
force-pushed
the
feature/add-get-physical-card-button-and-necessary-routes
branch
3 times, most recently
from
f9493f9 to
3fa9a25
Compare
| @@ -891,6 +892,28 @@ export default { | |||
| thatDidntMatch: 'Los 4 últimos dígitos de tu tarjeta no coinciden. Por favor, inténtalo de nuevo.', | |||
| }, | |||
| }, | |||
| // TODO: add translation | |||
There was a problem hiding this comment.
@joekaufmanexpensify I am still missing some spanish translations that I could not find in the codebase. Could you get them for me, please? 😄
| legalFirstName, | ||
| legalLastName, | ||
| phoneNumber, | ||
| addressCity: city, |
There was a problem hiding this comment.
@grgia I followed the specifications but I'm thinking that the country and state is missing here
There was a problem hiding this comment.
👍 agree, I think we need these. Here are the complete params the API expects
authToken,
legalFirstName,
legalLastName,
phoneNumber,
addressCity,
addressStreet,
addressZip,
addressCountry,
addressState
|
cc @marcaaron if you have the bandwidth to take a look at this one as well |
| street1: PropTypes.string, | ||
| street2: PropTypes.string, | ||
| submitButtonText: PropTypes.string, | ||
| zip: PropTypes.string, |
There was a problem hiding this comment.
These please all need some comments above them like we have in other parts of the app.
There was a problem hiding this comment.
@pac-guerreiro could you address this comment
| @@ -817,6 +817,28 @@ export default { | |||
| availableSpend: 'Remaining spending power', | |||
| virtualCardNumber: 'Virtual card number', | |||
| physicalCardNumber: 'Physical card number', | |||
| getPhysicalCard: 'Get physical card', | |||
| }, | |||
| getPhysicalCard: { | |||
There was a problem hiding this comment.
We might have some of these already and could move them to common
Lines 1044 to 1046 in 9e35d91
src/libs/Navigation/Navigation.js
Outdated
| function goToNextPhysicalCardRoute(privatePersonalDetails, loginList) { | ||
| const {address, legalFirstName, legalLastName, phoneNumber} = privatePersonalDetails; | ||
| const currentRoute = navigationRef.current && navigationRef.current.getCurrentRoute(); | ||
| const {domain} = (currentRoute && currentRoute.params) || {domain: ''}; |
There was a problem hiding this comment.
If there can be a fallback to an empty string for domain should we handle it somehow?
Seems like it would be unexpected if that happened based on the fact that we use it to navigate below.
| legalFirstName, | ||
| legalLastName, | ||
| phoneNumber, | ||
| addressCity: city, |
There was a problem hiding this comment.
👍 agree, I think we need these. Here are the complete params the API expects
authToken,
legalFirstName,
legalLastName,
phoneNumber,
addressCity,
addressStreet,
addressZip,
addressCountry,
addressState
src/libs/actions/Wallet.js
Outdated
| { | ||
| onyxMethod: Onyx.METHOD.MERGE, | ||
| key: ONYXKEYS.PRIVATE_PERSONAL_DETAILS, | ||
| value: params, |
There was a problem hiding this comment.
I am not sure if these params can be dropped here as is...
Especially the address you can see here it has this format:
App/src/types/onyx/PrivatePersonalDetails.ts
Lines 1 to 16 in 9e35d91
So what we are storing might be different than what we send to the API.
There was a problem hiding this comment.
@marcaaron That Address type does not match what we're storing on Onyx right now, as you can see below:
There was a problem hiding this comment.
Besides that, you're right, I need to send the data directly without mapping it
There was a problem hiding this comment.
@shawnborton The designs for the confirmation page don't have a fixed size applied to the items of the list, as you can see below:
Should these items size match the existing ones on the app? And if so, could you update the designs, please?
There was a problem hiding this comment.
@pac-guerreiro those should just be existing components in the design system called "push-to-page" rows. They might be called something like MenuOption in the code? cc @grgia for help there
There was a problem hiding this comment.
But you should be able to reuse existing code for these and not need to worry about building something new.
There was a problem hiding this comment.
@shawnborton thanks for the response! I'm already reusing the right component for this but I was applying the styles from Figma, so I removed these styles and tweaked the whole layout to work correctly 😄
This is the current state:
There was a problem hiding this comment.
Sweet, that feels pretty good to me!
pac-guerreiro
force-pushed
the
feature/add-get-physical-card-button-and-necessary-routes
branch
2 times, most recently
from
b27e41b to
e3df320
Compare
Screen.Recording.2023-10-18.at.14.52.43.mov@grgia @shawnborton This is the current state of the flow! I have two questions:
|
|
Feels pretty good to me! |
|
@grgia @allroundexperts @dylanexpensify All reported issues should be fixed now! I added logic to handle these edge cases:
Also, I noticed that while on the not found state of the card page if the user presses the back button then the RHP is closed. So I changed this behaviour to redirect the user to the wallet page instead. |
|
@allroundexperts bump |
|
Getting the following behaviour now @grgia. Notice the end of the video: Screen.Recording.2023-11-20.at.7.40.25.PM.movAlso, the back button is still behaving weirdly: Screen.Recording.2023-11-20.at.7.41.41.PM.mov |
|
@grgia At this point, I really feel like we should merge this and fix the issues in a follow up PR. Let me know if you'd agree. |
|
@allroundexperts I agree, if no bugs are blocking we can handle in a follow up. As you finish checklist and QA steps, can you please catalogue the non-blocking bugs so I can create issues |
| // eslint-disable-next-line es/no-nullish-coalescing-operators | ||
| inputValues[inputID] = defaultValue ?? ''; |
There was a problem hiding this comment.
Why is this being changed?
There was a problem hiding this comment.
I made this change because it's more simpler and does the same thing as before.
There was a problem hiding this comment.
I don't think that's correct. For example, previously, if defaultValue was false, inputValues[inputID] would be false as well. With your changes, it would be ''.
There was a problem hiding this comment.
@pac-guerreiro @allroundexperts Shall we revert this line to avoid breaking other forms?
There was a problem hiding this comment.
although @allroundexperts is this the case you were referring to?
There was a problem hiding this comment.
Ah, actually, its the same. My bad. False alarm 🔕
|
Checklist is complete @grgia. Just need an answer on one question I raised during the code review. Will approve after that. |
|
@allroundexperts question answered |
Replied. Please take a look. |
|
Outstanding issues: #28453 (comment) cc @grgia |
|
✋ 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/grgia in version: 1.4.2-0 🚀
|
| // There are situations where a route already exists on the current navigation stack | ||
| // But we want to push the same route instead of going back in the stack | ||
| // Which would break the user navigation history | ||
| if (type === CONST.NAVIGATION.ACTION_TYPE.PUSH) { | ||
| minimalAction.type = CONST.NAVIGATION.ACTION_TYPE.PUSH; | ||
| } | ||
| // There are situations when the user is trying to access a route which he has no access to | ||
| // So we want to redirect him to the right one and replace the one he tried to access | ||
| if (type === CONST.NAVIGATION.ACTION_TYPE.REPLACE) { | ||
| minimalAction.type = CONST.NAVIGATION.ACTION_TYPE.REPLACE; | ||
| } |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.2-3 🚀
|
| const domainCards = CardUtils.getDomainCards(cardList)[domain]; | ||
| const virtualCard = _.find(domainCards, (card) => card.isVirtual) || {}; | ||
| const cardID = virtualCard.cardID; | ||
| Wallet.requestPhysicalExpensifyCard(cardID, authToken, updatedPrivatePersonalDetails); |
| @@ -250,7 +250,7 @@ function BaseTextInput(props) { | |||
| return ( | |||
| <> | |||
| <View | |||
| style={styles.pointerEventsNone} | |||
| style={[styles.pointerEventsNone, ...props.containerStyles]} | |||
There was a problem hiding this comment.
Moved containerStyles to main View wrapper caused an issue #40519, bcoz containerStyles has max height restriction which is specifically required only for TextInput. Due to this change the error message overflows and overlaps with the bottom content.
Details
Fixed Issues
$ #22877
PROPOSAL:
Tests
Settings/Wallet.On Web, add this mock data in browser console:
On remaining platforms, add this useEffect on WalletPage.js:
Assigned Cardssection list.Get Physical Cardbutton at the bottom of the page.Ship Cardbutton on the confirmation step to go back to theExpensify Cardpage.Expensify Cardpage won't show the updated state when you already requested the physical card.Offline tests
QA Steps
User with no personal details
User with personal details
QA Personal Details due to Refactor
Create a new account
Set all the personal details under settings/profile/personal-details
Verify that personal details are set without error.
Ensure that addresses in USA with the State field are saved without error as well as addresses outside of the USA.
Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Legal Name Step
Phone Number Step
Address Step
Confirmation Step
Mobile Web - Chrome
Expensify Card Page
Legal Name Step
Phone Number Step
Address Step
Confirmation Step
Mobile Web - Safari
Legal Name Step
Phone Number Step
Address Step
Confirmation Step
Expensify Card Page
Desktop
Legal Name Step
Phone Number Step
Address Step
Confirmation Step
iOS
Legal Name Step
Phone Number Step
Address Step
Confirmation Step
Expensify Card Page
Android
Expensify Card Page
Legal Name Step
Phone Number Step
Address Step
Confirmation Step
