Update the Assigned Cards Section of the Wallet Page #38998
Conversation
There was a problem hiding this comment.
Looks mostly good to me. I found several small codestyle tweaks that I think you could apply to make this cleaner, so if you have time then please do them or respond to me if something doesnt make sense.
Otherwise Im approving this b/c I see no big breaking bugs 👍
I have not run this, only reviewed the code.
src/libs/Navigation/types.ts
Outdated
| [SCREENS.SETTINGS.REPORT_CARD_LOST_OR_DAMAGED]: { | ||
| /** domain passed via route /settings/wallet/card/:domain/:card */ | ||
| domain: string; | ||
| /** cardId passed via route /settings/wallet/card/:domain/:card */ |
There was a problem hiding this comment.
While I understand this style of comment is already used in this file, I still find it kinda redundant.
Do you really think its needed? and if yes then could we write it in some more human-friendly way?
For example there is a comment in line 141
/** Currently selected country */
^ that reads much better
so something like // the domain of the current card etc
| const domainCards = useMemo(() => cardList && CardUtils.getDomainCards(cardList)[domain], [cardList, domain]); | ||
| const virtualCard = useMemo(() => domainCards?.find((card) => card.isVirtual), [domainCards]); | ||
| const physicalCard = useMemo(() => domainCards?.find((card) => !card.isVirtual), [domainCards]); | ||
| const isCardDomain = !cardList?.[cardId].isAdminIssuedVirtualCard; |
There was a problem hiding this comment.
I find name isCardDomain hard to understand. What actually does this variable mean:
- is something a card domain?
- is the card of domain type?
- does the card domain exist?
this is a nitpick but if you have a better idea for a name, then I'd update it
| const [cardDetailsError, setCardDetailsError] = useState(''); | ||
|
|
||
| const cardsToShow = useMemo( | ||
| () => (isCardDomain ? CardUtils.getDomainCards(cardList)[domain].filter((card) => !card.isAdminIssuedVirtualCard) : [cardList?.[cardId]]), |
There was a problem hiding this comment.
to me this 1 liner reads very hard.
() => {
if (isCardDomain) {
return CardUtils.getDomainCards(cardList)[domain].filter((card) => !card.isAdminIssuedVirtualCard);
}
return [cardList?.[cardId]]maybe this is better?
| setIsCardDetailsLoading((prevState: Record<number, boolean>) => { | ||
| const newLoadingStates = {...prevState}; | ||
| newLoadingStates[revealedCardId] = true; | ||
| return newLoadingStates; |
There was a problem hiding this comment.
would this work?
return {
...prevState,
[revealedCardId]: true
}I find this more readable than creating a temporary const
| setCardsDetailsErrors((prevState) => { | ||
| const newCardsDetailsErrors = {...prevState}; | ||
| newCardsDetailsErrors[revealedCardId] = error; | ||
| return newCardsDetailsErrors; |
There was a problem hiding this comment.
in all 3 cases I believe it should be possible to use the pattern:
return {
...prevState,
[fieldKey]: newValue
}| return {limitName: translate('cardPage.fixedLimit.name'), limitTitle: translate('cardPage.fixedLimit.title', formattedAvailableSpendAmount)}; | ||
| default: | ||
| return {limitName: '', limitTitle: ''}; | ||
| } |
There was a problem hiding this comment.
it feels kinda dirty to define this type of function within the component body. Please move this out of the component, keep in this file but above the component.
There are 2 ways to handle that you don't have the translate function outside of component scope.
Either:
- make your function accept a 2nd argument, which will be the translate fn
- instead of returning objects with results of running translate, return objects with strings (translation keys) and then inside the component call
translate(...)on them
Choose whichever you think is better in this case.
| shouldShowRightIcon | ||
| onPress={() => Navigation.navigate(ROUTES.SETTINGS_WALLET_REPORT_CARD_LOST_OR_DAMAGED.getRoute(domain, cardId))} | ||
| /> | ||
| </> |
There was a problem hiding this comment.
in such cases I personally prefer to do the longer version of arrow: () => { ... } so that I can do an early return, instead of &&.
I find its easier to reason what gets returned if the code looks something like this:
{
[...]
if (!SOME_CONSTS) {
return null
}
return <>JSX....</>
}
SzymczakJ
force-pushed
the
@szymczak/update-cards-section
branch
from
222fa0b to
a10788e
Compare
|
Running a test build :) |
This comment has been minimized.
This comment has been minimized.
|
Hmm @SzymczakJ For this data Here's the page: 
This is totally correct based on the mocks, but in interacting with it I think I'd like to hear design input. For the case that a user already has a physical card and is reprovisioned a second one while the first is still active, I think we could show some kind of copy or indication to the user that they've been provisioned a new card. For example, Green brick road from the wallet to the card page and add a second physical card row with copy explaining that they've been provisioned a new card or something Any thoughts? @Expensify/design |
|
I could totally see that. Is there any action to be taken on the new physical card once it's provisioned though? Just thinking in terms of how these patterns technically work, the green brick road typically means there is something you need to do or an action you need to take. We typically don't use it just to tell you that new data is available. That being said, we don't have any kind of "unread" pattern for Settings either. |
Yeah curious about this too because like Shawn said, we don't usually GBR things unless there's something for you to do. |
I missed this at first as well, but the user needs to open the card page and click on this button to go through the get new card flow to order their new card. It's not really clear, but this indicates that there's a third new card
|
|
That makes sense. But I thought you were asking if we should put a green brick road dot on the new card row once they go through that flow you just referenced? I think we are leaning towards no, because once the card is created, there is no further action to take. |
|
|
||
| useEffect( | ||
| () => () => { | ||
| CardSettings.clearCardListErrors(cardID); | ||
| CardSettings.clearCardListErrors(physicalCard?.cardID ?? 0); |
There was a problem hiding this comment.
Hmm do we want to default to 0 here? It looks like it would merge 0 into the cardList
There was a problem hiding this comment.
Maybe not though? would 0: {} work?
There was a problem hiding this comment.
I could just do if and an early return. Dunno why I didn't think of that. Defaulting to 0 would probably cause bugs 😄
There was a problem hiding this comment.
Yeah let's do an early return instead
| CardSettings.activatePhysicalExpensifyCard(lastFourDigits, physicalCard?.cardID ?? 0); | ||
| }, [lastFourDigits, physicalCard?.cardID]); |
There was a problem hiding this comment.
Maybe we should default to a negative ID
There was a problem hiding this comment.
We could but isn't 0 also a wrong card ID and would cause CardSettings.activatePhysicalExpensifyCard to fail(which is what we want when physical card doesn't have cardID). Do we have some special negative number for such cases?
There was a problem hiding this comment.
Yeah 0 is an incorrect card ID. I think we've historically used -1 but I need to double check. But let me know if you'll try returning early instead
| @@ -104,30 +104,30 @@ function ExpensifyCardPage({ | |||
| const [isCardDetailsLoading, setIsCardDetailsLoading] = useState<Record<number, boolean>>({}); | |||
| const [cardsDetailsErrors, setCardsDetailsErrors] = useState<Record<number, string>>({}); | |||
|
|
|||
| const handleRevealDetails = (revealedCardId: number) => { | |||
| const handleRevealDetails = (revealedcardID: number) => { | |||
|
|
||
| useEffect( | ||
| () => () => { | ||
| CardSettings.clearCardListErrors(cardID); | ||
| CardSettings.clearCardListErrors(physicalCard?.cardID ?? 0); |
There was a problem hiding this comment.
Yeah let's do an early return instead
| CardSettings.activatePhysicalExpensifyCard(lastFourDigits, physicalCard?.cardID ?? 0); | ||
| }, [lastFourDigits, physicalCard?.cardID]); |
There was a problem hiding this comment.
Yeah 0 is an incorrect card ID. I think we've historically used -1 but I need to double check. But let me know if you'll try returning early instead
|
@grgia just fixed your comments |
|
@SzymczakJ could we move this out of drafts today so C+ can take a passover as well? |
|
@rojiphil fixed your comments and uploaded PR author checklist 😸 |
| @@ -109,24 +109,25 @@ function BaseGetPhysicalCard({ | |||
| const styles = useThemeStyles(); | |||
| const isRouteSet = useRef(false); | |||
|
|
|||
| 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 ?? 0; | |||
There was a problem hiding this comment.
I think it would be good to use cardID as string (like cardToBeIssued?.cardID?.toString() ?? ‘’) instead of calling toString many times. We need cardID as a number only in requestPhysicalExpensifyCard where we can use cardToBeIssued?.cardID ?? 0. This will make the code look neater too. What do you think?
|
triggered one more build |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
Testing on my card test account: @SzymczakJ for the combo card, which ID is used in the route? It seems weird to use a cardID if there's two cards here Can we use the domain just for this case? |
|
These pages LGTM :) |
|
@SzymczakJ Can you please update the Precondition: Test 1: Cards from the same domain are grouped under the same card row when card is not issued by an admin
Test 2: When card was issued by the admin then this card will have it's own separate row
Test 3: Show card limit type
Test 4: Show card limit type
Test 5: Show card limit type
Test 6: Show the activate button if ANY of the cards in this group are in the NOT_ACTIVATED state
Test 7: Show
Test 8: Show
Test 9: Make the card non interactive if the card is non
|
|
Great work @SzymczakJ Thank you for the reviews @rojiphil |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 1.4.67-7 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to staging by https://github.com/grgia in version: 1.4.68-0 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 1.4.67-7 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.68-3 🚀
|
| if (!inactiveCard?.cardID) { | ||
| return; | ||
| } | ||
| CardSettings.clearCardListErrors(inactiveCard?.cardID); |
There was a problem hiding this comment.
Error is not cleared when page is reloaded, #51396
Details
This PR changes the way cards are shown on the WalletPage. Cards from the same
domainare grouped under the same card row when card is not issued by an admin. When card was issued by the admin then this card will have it's own separate row. After clicking on each of these rows user will be taken to a ExpensifyCardPage, which shows all the cards grouped under the domain or a single card depending which row was clicked.We determine if the card was issued by an admin by checking if
card.nameValuePairs.issuedByfield exists.Fixed Issues
$ #38468
PROPOSAL:
Tests
Tests Extended
Precondition:
Provision the FE using below data:
Test 1: Cards from the same domain are grouped under the same card row when card is not issued by an admin
Expensify Cardand domaincathycroissants.comand click on the same.1234123412341234and3456345634563456are grouped together. Verify that the virtual card with cardID3456345634563456is displayed.Test 2: When card was issued by the admin then this card will have it's own separate row
Amazon Card(title)and domaincathycroissants.comand click on the same.2345234523452345is displayed.Test 3: Show card limit type
Smartand the limit specific descriptionExpensify Cardand domaincathycroissants.comSmart limitand description with textYou can spend up to $50,000.00 on this card, and the limit will reset as your submitted expenses are approved.is displayed.Test 4: Show card limit type
Monthlyand the limit specific descriptionAdobe Subscriptionand domaincathydumplings.comMonthly limitand description with textYou can spend up to $5,000.00 on this card per month. The limit will reset on the 1st day of each calendar month.is displayed.Test 5: Show card limit type
Fixedand the limit specific descriptionNetflix subscriptionand domaincathycinamons.comFixed limitand description with textYou can spend up to $200.00 on this card, and then it will deactivate.is displayed.Test 6: Show the activate button if ANY of the cards in this group are in the NOT_ACTIVATED state
Expensify Cardand domaincathycroissants.comActivate physical cardbutton is displayed as card with id1234123412341234is inNOT_ACTIVATEDstate.Test 7: Show
Report virtual card fraudpageExpensify Cardand domaincathycroissants.comReport virtual card fraudto launch the report fraud pageTest 8: Show
Report physical card loss / damagepageAmazon Card(title)and domaincathycroissants.comReport physical card loss / damageto launch the loss/damage pageTest 9: Make the card non interactive if the card is non
Expensify CardAmazon Cardand domainkubson.comis non-interactive. Also, right icon should be hiddenOffline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
android.mov
Android: mWeb Chrome
android.web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
iosweb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
deskltop.mov