[No QA] Filter personal cards from Cardlist#76967
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
e8c142f to
8d7e25e
Compare
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
8d7e25e to
c36180b
Compare
| * Filter out personal (including cash) cards from the card list. | ||
| */ | ||
| function filterPersonalCards(cards: CardList | undefined): CardList { | ||
| return filterObject(cards ?? {}, (key, card) => !!card?.fundID && card.fundID !== '0'); |
There was a problem hiding this comment.
NAB: I think filterPersonalCards is a bit misleading because it filters personal/cash cards out and returns company cards only (fundID present and not '0'). i would recommend to rename to something like filterCompanyCards / excludePersonalAndCashCards so that we highlight that it excludes both personal + cash?
What do u think @rojiphil
There was a problem hiding this comment.
As I understand, cash card is also a type of personal card but additionally having card name as __CASH__. So, the name filterPersonalCards still LGTM as the action is to filter out personal cards from the cardlist.
|
@iwiznia Thanks for the clarification. |
|
@abzokhattab 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] |
|
@abzokhattab @iwiznia One of you needs to 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] |
|
let me know if i should also review this @iwiznia |
|
Reviewing today... |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Left a small comment but overall it looks good to me Thanks |
|
LGTM |
|
|
||
| const areAllCardsShippedSelector = (cardList: OnyxEntry<CardList>) => Object.values(cardList ?? {})?.every((card) => card?.state !== CONST.EXPENSIFY_CARD.STATE.STATE_NOT_ISSUED); | ||
| const areAllCardsShippedSelector = (cardList: OnyxEntry<CardList>) => | ||
| Object.values(cardList ?? {})?.every((card) => card?.state !== CONST.EXPENSIFY_CARD.STATE.STATE_NOT_ISSUED && !!card?.fundID && card.fundID !== '0'); |
There was a problem hiding this comment.
Can you please create a method like isPersonalCard(card) and use it here and in filterPersonalCards so we don't duplicate the logic please?
There was a problem hiding this comment.
@iwiznia Yeah. That makes sense. Updated the code.
|
Oh, you have conflicts |
|
@iwiznia Resolved conflicts. |
|
✋ 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/iwiznia in version: 9.2.90-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.90-2 🚀
|
| /** | ||
| * Check if the given card is a personal card. | ||
| * | ||
| * @param card the card which needs to be checked | ||
| * @returns true if the card is a personal card, false otherwise | ||
| */ | ||
| function isPersonalCard(card?: Card) { | ||
| return !!card?.fundID && card.fundID !== '0'; | ||
| } | ||
|
|
||
| /** | ||
| * Filter out personal (including cash) cards from the card list. | ||
| */ | ||
| function filterPersonalCards(cards: CardList | undefined): CardList { | ||
| return filterObject(cards ?? {}, (key, card) => isPersonalCard(card)); | ||
| } |
There was a problem hiding this comment.
@rojiphil I have follow up question to this code. Based on #73230 (comment)
Personal cards will have fundID set to 0
I assume there is some misleading naming. Based on that comment, I would expect isPersonalCard to check === '0' It should be more like isNotPersonalCard. Could you confirm if I am right?
There was a problem hiding this comment.
Sure, no problem. I’m working on some optimizations and had a hard time understanding the filtering logic here because of the naming. Thanks for clarifying
There was a problem hiding this comment.
I came to this PR from #79562 to ask the same question 😅
| */ | ||
| function ExpensifyCardContextProvider({children}: PropsWithChildren) { | ||
| const [cardList] = useOnyx(ONYXKEYS.CARD_LIST, {canBeMissing: false}); | ||
| const [cardList] = useOnyx(ONYXKEYS.CARD_LIST, {selector: filterPersonalCards, canBeMissing: false}); |
There was a problem hiding this comment.
Coming from this issue #78851, Adding the selector here caused a console warning (useOnyx returned no data for key with canBeMissing set to false for key cardList). We fixed it by adding getEmptyObject as the default value #78851 (comment).
Explanation of Change
This PR filters the
Cardlistto exclude all personal cards.Personal cards are identified when
fundIDis 0.Fixed Issues
$ #70639
PROPOSAL: #73230 (comment)
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
76967-web-chrome-001.mp4