[NO QA]: Workspace Feed - Expensify Card #44549
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] |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
I will review this today |
| type EnablePolicyExpensifyCardsParams = { | ||
| policyID: string; | ||
| enabled: boolean; | ||
| }; |
There was a problem hiding this comment.
From the design doc this requires one more param authToken which we seem to be missing here.
Also i checked the endpoint:
This keeps returning 404, c.c. @MariaHCD @mountiny , is the BE part not yet implemented, the api gets on called infinitely on my local setup
Also @narefyev91 , does the option of cards go from the options list? currently when i disable the option i have to either load the page again / go back and come again for the option of Expensify Card to go away.
Screen.Recording.2024-06-29.at.4.43.06.PM.mov
There was a problem hiding this comment.
Yeah good point - will add authToken.
Experiencing the same problem with 404 - and that is a reason with toggling off.
We have this line inside useEffect - which should add/remove left menus
When we fire api call (which is not exists) - field is in pending state - that's why nothing happened immediately
If i just use here direct key from onyx policyFeatureStates and not state featureStates it will work
Probably when API will be working - everything will be works as well
@allgandalf
There was a problem hiding this comment.
ummm, thanks for pointing that out, yes that seems logical to me, lets wait on vit and maria for their thoughts though :)
There was a problem hiding this comment.
@MariaHCD can you weigh in here to keep it moving? Thanks!
There was a problem hiding this comment.
Ah, sorry, I missed this.
The BE API endpoint is not implemented yet. We can focus on the frontend elements / flows for now and then once the BE endpoints are implemented, we can update the FE here and retest the flow.
There was a problem hiding this comment.
Thanks for the changes @narefyev91 ,Design wise this LGTM, waiting on to hear more about this issue, will approve after that point cleared
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / SafariScreen.Recording.2024-07-01.at.8.54.50.PM.movAndroid: NativeScreen.Recording.2024-07-01.at.8.51.12.PM.movAndroid: mWeb ChromeScreen.Recording.2024-07-01.at.8.52.41.PM.moviOS: NativeScreen.Recording.2024-07-01.at.8.53.51.PM.moviOS: mWeb SafariScreen.Recording.2024-07-01.at.8.54.28.PM.movMacOS: DesktopScreen.Recording.2024-07-01.at.8.47.33.PM.mov |
There was a problem hiding this comment.
This looks good to go, if we ignore the one bug i reported according to this #44549 (comment), lets get this merged.
Also @narefyev91 , can you please mark this as NO QA :) thanks
narefyev91
changed the title
| test1: { | ||
| cardholder: {accountID: 1, lastName: 'Smith', firstName: 'Bob', displayName: 'Bob Smith', avatar: ''}, | ||
| name: 'Test 1', | ||
| limit: 1000, | ||
| lastFourPAN: '1234', | ||
| }, |
There was a problem hiding this comment.
Similar to https://github.com/Expensify/App/pull/44469/files#r1661049859
name will be card.nameValuePairs.cardTitle and limit will be card.nameValuePairs.unapprovedExpenseLimit
But we can update that later
|
Merging since @mountiny is OOO today. If there's any improvements we can address them in a follow up PR! |
|
✋ 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/MariaHCD in version: 9.0.4-0 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/tgolen in version: 9.0.4-5 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.5-13 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀
|
| pendingAction: policy?.pendingFields?.areExpensifyCardsEnabled, | ||
| disabled: !isEmptyObject(cardsList), | ||
| action: (isEnabled: boolean) => { | ||
| Policy.enableExpensifyCard(policy?.id ?? '-1', isEnabled); |
There was a problem hiding this comment.
What does is expected to happen if we call Policy.enableExpensifyCard with policyID = -1? I guess the server will throw an error... shouldn't we instead be writing the code like:
action: (isEnabled: boolean) => {
if (!policy?.id) {
return;
}
Policy.enableExpensifyCard(policy.id, isEnabled);
}
and disable this button if we don't have a policy.id with disabled: !isEmptyObject(cardsList) || policy?.id,
There was a problem hiding this comment.
Good point, @aldo-expensify!
@narefyev91 Can we update to not call Policy.enableExpensifyCard with policyID -1?
There was a problem hiding this comment.
BTW @MariaHCD @aldo-expensify we have the same logic for all menu items
And probably all those API calls will failed?
There was a problem hiding this comment.
A little bit confusing if we just re-write only one menu item - and leave everything else as it is. And why such behaviour was not implemented? Probably we do not block action even if policy id is not exists?
There was a problem hiding this comment.
Got it. I'm not quite following what the situation is exactly that you need advice on, but if I can be helpful, please let me know and we can keep discussing.
There was a problem hiding this comment.
I think this matters more from a code perspective rather than UI, since it may be impossible for this to happen in a normal user case? 🤷
There was a problem hiding this comment.
Ok I will make that changes and push a PR to clean up code
There was a problem hiding this comment.
Wanted to say that this screen has the policyID in the URL so probably not possible to access.
I do like the clarity though from the code perspective so feel free to clean this up but noting this is lower priority task so try to prioritize other release related work and. Focus on this if you do not have other tasks to handle @narefyev91 thanks!
Thanks @aldo-expensify for raising
There was a problem hiding this comment.
@mountiny yeah - i'm happy to help with anything for upcoming releases - but probably for now have some time to pick this one (until something else will come up). Probably waiting when Third-party workspace card feed doc will reviewed and start on that project
Details
Workspace Feed - New Feature initial implementation - Expensify Card.
Feature are currently under Beta access - be sure that you have access to all betas or
expensifyCardFeatureFixed Issues
$ #44301, #44303, #44302
PROPOSAL:
Tests
Test disabled toggle:
Offline tests
Test disabled toggle:
QA Steps
Test disabled toggle:
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.mov
Android: mWeb Chrome
android-web.mov
android-web.mov
iOS: Native
ios.mov
ios.mov
iOS: mWeb Safari
ios-web.mov
ios-web.mov
MacOS: Chrome / Safari
web.mov
web2.mov
MacOS: Desktop
desktop.mov
desktop.mov