Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.51k stars 2.86k forks source link

[Workspace Feeds] Create a loading state for creating card and provisioning a policy for cards #50699

Open mountiny opened 2 weeks ago

mountiny commented 2 weeks ago

Problems

Both - creating cards and provisioning a policy for Expensify cards - are tasks in the backend that rely on third-party services, and we cannot optimistically take these actions in the App. Right now, we do not wait for a successful response from the server, which in some cases (especially if the card program provisioning fails), leads to bad UX.

Coming from this thread

Solution

Let's add a loading/ waiting/ spinner state for the user once they create a card or when they choose a VBBA bank account as a settlement bank account. We will wait for 200 successful API response before proceeding and if the call fails, we will show them the error message straight away and we won't lead them to failure on other commands.

Can you please mock how these should look like? In case of provisioning the policy for cards, I think its such a rare moment its also work a celebration in the product so maybe some fireworks would be worth it there

melvin-bot[bot] commented 2 weeks ago

Current assignee @dubielzyk-expensify is eligible for the Design assigner, not assigning anyone new.

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @anmurali (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

mountiny commented 2 weeks ago

Sorry no need for b0 person

dubielzyk-expensify commented 2 weeks ago

We should use the button + spinner combo for all places where we need to wait for the response: image

As for the bank account selection flow we can either do the same and add a success screen: image

Or we could use the bank account illustration as a loading state: image

I think if the loading time is likely to be multiple seconds, then maybe use the bank account illustration. If it's relatively short, then use the loading button.

cc @Expensify/design for thoughts too

shawnborton commented 2 weeks ago

Totally agree with that Jon. If we know it's typically at least a few seconds, let's use the animation instead of the loading spinner.

dannymcclain commented 2 weeks ago

Also totally agree with Shawn and Jon above!

mountiny commented 2 weeks ago

Agreed with Jon, Shawn and Danny.

@koko57 Would you want to take this one on?

melvin-bot[bot] commented 2 weeks ago

Current assignee @jamesdeanexpensify is eligible for the Waiting for copy assigner, not assigning anyone new.

mountiny commented 2 weeks ago

Added the copy label here just so we can confirm the copy in the mocks above is what we are happy with

jamesdeanexpensify commented 2 weeks ago

Which specific screens need feedback, assuming we're not using them all? Just want to make sure, thanks!

jamesdeanexpensify commented 2 weeks ago

Also, is the Verifying bank account... screen meant to sit for a few seconds, OR is there a chance it will take a longer time via manual checks so they'll need a way out of that screen + a way to get notified whether or not their bank account was approved later on?

koko57 commented 2 weeks ago

@mountiny yep, of course! Happy to work on this one 🙂

mountiny commented 2 weeks ago

The screens titled as bank account verified and verifying - that's where to confirm the copy please.

Yes there is a chance of manual verification needed, @koko57 that is the waitlist nvp btw

In that case the screen should lead them to concierge page. Then the card page will already have a modal pointing them to concierge

mountiny commented 2 weeks ago
image

These screens.

@dubielzyk-expensify I wonder then if we should have a special screen as the final one in case the bank account was put on a waitlist and we need to verify that manually. We should be able to check that by looking for the waitlist nvp and tell the user they have to go to concierge where there should already be a message with instructions

koko57 commented 2 weeks ago

@mountiny @jamesdeanexpensify I'm wondering - will we ever have the case that the account that is not verified (on the waitlist) will appear on the bank account list? (comment) Do we need any other validation? And for bank accounts we additionally filter out eligible bank accounts.

mountiny commented 2 weeks ago

The waitlist is before you are provisioned for the expensify cards when you choose that bank account @madmax330 so I would say its possible

dubielzyk-expensify commented 2 weeks ago

Can you explain what the waitlist means in simple terms? 😇

But that being said, I think something like what we did for VBA could work: image

Just need confirm I've understood it correctly and likely @jamesdeanexpensify will have to fix my copy 😅

shawnborton commented 2 weeks ago

Mock looks good to me though, nice one Jon!

jamesdeanexpensify commented 2 weeks ago

Verifying bank account... Please wait while we confirm that this account can be used to issue Expensify Cards.

Bank account verified screen looks good! Just add a period at the end.

One more step screen also looks good - well done Jon!

jamesdeanexpensify commented 2 weeks ago

Oh! Actually for the One more step screen - I remembered we discussed a similar screen here @dubielzyk-expensify but I'm not sure which one we landed on. Could we just use whatever we used there, instead, to make things more consistent? I do remember saying we shouldn't use a Concierge button because there wasn't an action for them to take until they heard from Concierge.

dubielzyk-expensify commented 2 weeks ago

We could use similar language, but it sounds like this is one where they actually have to go to Concierge for next step? If there is a wait, then I agree, but from the wording above I interpreted it as next steps immediately available.

koko57 commented 2 weeks ago

ok, so for success screen I should use this image and this copy?

Screenshot 2024-10-17 at 16 11 14

and for the Veryfing step? Which design should I use?

mountiny commented 2 weeks ago

@dubielzyk-expensify For waitlist, I think you got it right; we basically need more details from them about the business and the bank account before we can offer them the line of credit/ decide what the limit will be, and they need to talk to the concierge to provide those details. When they are put on this waitlist, they should get an automatic message in the concierge. We also already have a special view on the cards page when this happens so that will also guide them to concierge

Seems like we are sorted for this one right? @koko57

mountiny commented 2 weeks ago

I think this one on the left for verifying image and then if the waitlist is added we show this one image

koko57 commented 2 weeks ago

May I ask for the puzzle illustration exported as SVG? I cannot find it in the project

shawnborton commented 2 weeks ago

I think Jon might have signed off for the day, so here you go: emptystate__puzzlepieces.svg.zip

It should be displayed at 148x154

koko57 commented 2 weeks ago

@shawnborton thanks a lot! 😊

jamesdeanexpensify commented 2 weeks ago

Just making sure we're incorporating these edits to the copy. Is that right? Thanks!

dubielzyk-expensify commented 1 week ago

Thank you Shawn! Updated mocks based on James' new copy:

image

dubielzyk-expensify commented 1 week ago

Not overdue

koko57 commented 1 week ago

Sorry for the delay - still working on it, I had some other responsibilities that I needed to take care of in the meantime. Will do my best to open the PR for this issue today/tomorrow.

mountiny commented 1 week ago

Thanks for the update

koko57 commented 1 week ago

I have a question - is the image for the Bank Account Verified the Fireworks lottie animation or it's a static image? I'm trying to find it in the project assets

shawnborton commented 1 week ago

It's a Lottie animation

koko57 commented 1 week ago

ok, thanks for confirmation! I've already used that one 🙂 I still have to polish the logic and do some code clean-up, so unfortunately I won't be able to open the PR today, but it should be ready tomorrow.

koko57 commented 1 week ago

@mountiny I've just noticed one thing - for the other collection keys in Onyx we have an _ before the policyID/workspaceAccountID, here we don't have it: nvp_expensify_onCardWaitlistFC4BF004BA67FB5D. Just checking if it's ok

koko57 commented 1 week ago

and another question - now we get 200 when the domain is properly provisioned and 200 with cardOnWaitlist otherwise - is there a possibility that we get an error? In such case should I also show the screen with the puzzle?

koko57 commented 1 week ago

I'm almost ready to open the PR but I have one problem. I'm getting this:

Screenshot 2024-10-23 at 14 44 11

as a response from OpenPolicyExpensifyCardsPage when I open Expensify Card on a new workspace.

After I choose an account that was added to another workspace I get Screenshot 2024-10-23 at 14 50 29 the same response.

And the problem is - how can I differentiate between the state of verification to show either the bank account list to choose and the verification screen with the puzzle?

cc @mountiny

mountiny commented 1 week ago

here we don't have it: nvp_expensify_onCardWaitlistFC4BF004BA67FB5D. Just checking if it's ok

No that is not intentional, I will fix it to have the _ there

mountiny commented 1 week ago

@koko57 I see where the issue is so I updated the waitlist nvp and Max will review https://github.com/Expensify/Auth/pull/12873

we will add the underscore too.

There is a structure problem with the nvp though, essentially it only hold one waitlist information at a time for the particular user, so as you set up another domain that might get on the waitlist, the previous values are just overwritten by the new one. Its not a map per domain which is what we would need in this case

So for now it might wokr well for that one workspace which is a mainline case and we will look into how to make it scale better

koko57 commented 1 week ago

ok, I understand. So it's better to hold on opening PR here?

mountiny commented 1 week ago

Nono, lets just go ahead, I think that what you ran into is very edge casey for normal user, they would set up the cards on one workspace and be done, most likely they would not try to set up the cards on multiple ones at the same time (being on a waitlist for multiple workspaces

koko57 commented 1 week ago

ok, so I will test it for a new user and a new workspace and will open the PR if everything is ok

mountiny commented 1 week ago

Yes, but even for the existing user, the NVP would be correctly set after you try to setup the policy.

I have made updated to the backend so we will return the nvp to the client only if the nvp is specific for the policy settings page you are seeing

koko57 commented 1 week ago

I was about to open the PR but I'm having a strange problem with the animation - it's not changing although it should. I also need to do some style improvements to make the content on the same height.

https://github.com/user-attachments/assets/2cad5d5b-c93f-4920-9dfe-fa59d91c68a5

koko57 commented 1 week ago

BTW, maybe as a follow-up we could make this loader from VBBA flow similar to the one used here - I mean with centered content and a title text (but we would need a new copy) Screenshot 2024-10-23 at 21 03 20Screenshot 2024-10-23 at 21 07 04

wdyt? @shawnborton @dubielzyk-expensify

dannymcclain commented 1 week ago

Yes we definitely need to make it match the Verifying one in terms of styles/text layout/etc.

It also looks like the illustration and page title are knocked back or have an opacity set or something, which they shouldn't.

shawnborton commented 1 week ago

Totally agree with both of you.

koko57 commented 1 week ago

I fixed the problem with Lottie animation not changing, although it's very strange that it didn't work properly when extracted to a separate component. I need to fix the styles and test it, and I'm opening PR shortly.