-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
fix: get all the cards from different feeds #52612
fix: get all the cards from different feeds #52612
Conversation
@DylanDylann 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] |
@DylanDylann can you please test? |
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
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.
Changes look good to me!
const cards = {}; | ||
for (const [key, values] of Object.entries(allWorkspaceCards ?? {})) { | ||
if (key.includes(workspaceAccountID.toString()) && values) { | ||
const {cardList, ...rest} = values; |
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.
Can you please add a comment that in case of direct feeds, the list of unassigned cards yet is returned in the cardList property in the collection?
Reviewer Checklist
Screenshots/VideosScreen.Recording.2025-01-08.at.20.57.50.mp4Android: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
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.
Tests well, just the comment would be helpful
@koko57 Could you also fix the same problem on Expensify Card? |
The card isn't displayed on the member detail page if we haven't opened the Expensify Card |
@DylanDylann the code I implemented doesn't filter out Expensify Cards and both company and Expensify cards should be displayed if they are in the cardList object. I will check it today |
Codewise it's implemented properly - the only strange thing I see is that there is a lag with the Card appearing - @DylanDylann you must have closed the details before it appeared on the list. I've logged out and in again and the same problem appears for the company cards - for the very first user selected the lag is noticeable. For the subsequent users it doesn't occur. So it's not a problem with Expensify Card. Screen.Recording.2025-01-09.at.09.46.46.mp4Screen.Recording.2025-01-09.at.09.42.57.mp4this is how the data looks - it includes Expensify Card ![]() |
@koko57 From my investigation, the card isn't displayed because shouldShowCardsSection is false <-- paymentAccountID = 0 |
@DylanDylann ok, I will check it, but one question - the card haven't appeared for you after a while like in my case? I cannot repro the case that the card doesn't appear at all |
Yes, I need to open Expensify Card then the BE will return private_expensifyCardSettings_18600056 in OpenPolicyExpensifyCardsPage API. After that I can see the cards on the member detail |
ok, thanks, checking it in a sec |
@DylanDylann it happens always after you log in back? Did you test it on another account? |
@DylanDylann you were testing it on an account with Expensify Cards enabled only (company cards not enabled)? I think that if so that might be the problem that we still have some condition to look for either company cards being enabled or expensify cards settings and that may cause what you report. I think that we should no longer watch for the cards being enabled or not but rather for the cards being issued or not. Otherwise, we would also need to get these Expensify Card settings to be sent just after logging in. I will check it and let you know. cc @mountiny |
ok, so it does happen on workspaces with Exfy Card enabled only. I will test the ideas I mentioned above and if it's possible to fix it only on the FE I will add these changes to this PR |
@mountiny @DylanDylann now in the code we'll be checking if the cards are assigned to anyone in the workspace to display the card section. But I thought about one edge case that will still occur. Before the changes we were looking for paymentAccountID. As it's not available right after the login the card section for Expensify Cards was also not visible. Now as we don't look for paymentAccountID we only display the card section when some cards are already assigned (to anyone). So if there is no card assigned it won't be visible even if the bank account is added and verified (and the workspace has paymentAccountID). We checked for paymentAccountID because we didn't want to show "New Card" button and the Card Section if the workspace doesn't have its paymentAccountID (aka bank account added and verified). So to solve this, I'm afraid that we still need some more info to be sent on OpenApp request - in this case private_expensifyCardSettings LMK WDYT |
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.
Discussed in slack gonna move this ahead and we can sort the remainder edge cases in follow ups
✋ 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/mountiny in version: 9.0.85-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.85-0 🚀
|
Tested and confirmed it works with both commercial and direct feeds! |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.85-0 🚀
|
1 similar comment
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.85-0 🚀
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.85-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.85-4 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.0.85-4 🚀
|
Hey, we are dealing with the missing cards section here #55571 |
@@ -220,7 +208,7 @@ function WorkspaceMemberDetailsPage({personalDetails, policy, route}: WorkspaceM | |||
return <NotFoundPage />; | |||
} | |||
|
|||
const shouldShowCardsSection = (!!policy?.areExpensifyCardsEnabled && !!paymentAccountID) || (!!policy?.areCompanyCardsEnabled && hasMultipleFeeds); | |||
const shouldShowCardsSection = hasWorkspaceCardsAssigned && (!!policy?.areExpensifyCardsEnabled || !!policy?.areCompanyCardsEnabled); |
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 should use the old condition here. this new change caused #56372
Explanation of Change
Fixed Issues
$ #51881
PROPOSAL: -
Tests
PREREQUISITES: An account with Company Cards enabled and some cards assigned to the Workspace Members
Offline tests
n/a
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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 methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label 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
MacOS: Desktop