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.43k stars 2.8k forks source link

[HOLD for payment 2024-07-23] [CRITICAL] [P2P] Refactor the wallet enablement flow to be step by step and conform with our new design system #36648

Closed anmurali closed 1 month ago

anmurali commented 7 months ago

Problem: The existing wallet enablement flow UI does not comply with our new brand design system, particularly our philosophy to take a stey by step approach to getting user input. It also doesn't give the user a sense of progression or context for remaining steps, which could cause drop off.

Solution: Let's refactor the Ui to ask for the info step by step instead of all at once. Let's add a progress bar at the top for the various sections:

Starting point, no change

image

Current Add bank account step, which loads a blank modal and automatically jumps to Plaid flow. Although the only choice here at the moment, it is cleaner to just show this step and allow the user to explicitly click on Connect via Plaid

image

Current Additional Details step, which is shown as one form, with all these details. We will refactor it to ask for the details step by step here

image

Current Verify Identity step, which presents some legalese links and then moves into the Onfido SDK flow. But the Onfido SDK flow begins with disclosures that the user needs to accept, so we don't need to show this legalese twice. We will therefore change the copy at the start of this step as well.

image

The Onfido SDK flow cannot be further customized to match our branding but, can we try to add the progress bar we're using here at the top of that entire flow, if possible?

Current Terms and fees step, which is shown as one long blob, with all these details. We will refactor it to show it step by step here

image

Finished step, no change

image
Issue OwnerCurrent Issue Owner: @muttmuure
shawnborton commented 7 months ago

For the start of the Onfido flow, I guess we should just mirror what we currently have in product (note this whole flow got a face lift recently!) where we basically just have one screen as a primer, then we kick off the Onfido flow? CleanShot 2024-02-16 at 08 41 39@2x

shawnborton commented 7 months ago

Also just cc'ing @mountiny (sorry for all the tags these days!) for visibility, in case we want to have the same group who worked on the VBA flows take this one as well.

mountiny commented 7 months ago

Thanks for the ping! We could ask Callstack due to their experience with this. I am curious though, I am not that familiar with the backend for this flow so curious if there would any changes be required there. @MariaHCD do you think you could chime in? I assume from what I could read the order of the steps is more or less the same.

We are not moving any checkboxes or any inputs to different steps right? (asking because we had that issue in the bank refactor and had to make changes to the backend slightly)

anmurali commented 7 months ago

So far as I can tell, we're not changing any inputs. The only thing worth double clicking on is Current Verify Identity step We currently show

image

I want to change that to image

mountiny commented 7 months ago

Nice, that sounds like basically just UI change!

@koko57 I heard you are keen on helping with this :) if so would you like to comment so I can assign you?

koko57 commented 7 months ago

yes I was typing the message right now! 😃 I'm ready to help

MariaHCD commented 7 months ago

the backend for this flow so curious if there would any changes be required there. @MariaHCD do you think you could chime in? I assume from what I could read the order of the steps is more or less the same.

The order of the steps looks the same to me too:

  1. AdditionalDetails step - which calls the UpdatePersonalDetailsForWallet API command
  2. Onfido step - which calls the OpenOnfidoFlow API command to initialize the flow, followed by VerifyIdentity
  3. Terms step - which calls AcceptWalletTerms

The KYC flow is outlined in this SO (in case it is helpful)

MariaHCD commented 7 months ago

One thing I'd like to add is regarding Knowledge Based Authentication (KBA) questions. @anmurali I'm assuming we still want to have these questions displayed to the user?

mountiny commented 7 months ago

🚀 perfect! @shawnborton will we need any more details? Maybe share a Figma file with Agata?

@MariaHCD Thank you, very helpful!

Noting here as well this is to help with testing https://stackoverflowteams.com/c/expensify/questions/10607/11542#11542

shawnborton commented 7 months ago

This is the Figma file we have been using.

mountiny commented 7 months ago

Thanks, shared with Agata!

koko57 commented 7 months ago

Thanks for sharing the designs and all the info! I think I'm all set to start working on this refactor. I'll keep you updated on the progress

koko57 commented 7 months ago

Yesterday I started working on the first view - the additional step with the "Connect bank account". I'll continue working on this today

koko57 commented 7 months ago

@mountiny @shawnborton I'm almost finished with the UI (easier) part then I'll work on the logic. But before I close the UI part I just wanted to make sure:

Screenshot 2024-02-22 at 17 37 53

Can I also ask for the Spanish translations for the title and the description?

shawnborton commented 7 months ago

Hmm would love @anmurali 's input on those when she is back from OOO, though maybe @joekaufmanexpensify can help answer that. @joekaufmanexpensify would we ever have a "manual" option for connecting your bank account for the Wallet enablement flow?

if so, do we need this text: "choose an option below"? (afaik we don't display it in VBBA flow)

Great point, that label could be removed in that case

And yeah, the space below the button should be removed too I think.

joekaufmanexpensify commented 7 months ago

would we ever have a "manual" option for connecting your bank account for the Wallet enablement flow?

Yep, we definitely will at some point. It just hasn't been built yet. Right now you can only add a bank account via Plaid here.

shawnborton commented 7 months ago

Sounds good. So @koko57 let's plan to just use the one button which means we can remove the preceding label.

shawnborton commented 7 months ago

@koko57 when you have your PR ready for review, can you please tag myself as well as @jamesdeanexpensify who can help out with the correct copy to use? Thanks!

melvin-bot[bot] commented 7 months ago

Triggered auto assignment to @muttmuure (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

koko57 commented 7 months ago

@shawnborton I'm struggling a little bit with the logic for the first steps (but when I get it right it'll be much easier for the next ones). I'll do my best to open the first PR till the EOW.

I also wanted to let you know that I'll be ooo for the next week (03/03 - 03/11). I'll continue working on this from 03/12.

koko57 commented 7 months ago

I unfortunately didn't manage to finish the whole step 1 - still need to improve some logic to make it work properly and cleanup the code, so I decided not to open the PR now. All the changes are on the https://github.com/koko57/App/tree/refactor/36648-wallet-enablement-flow branch.

I would be very grateful for Spanish translations of these texts:

As I've mentioned earlier, I'll be ooo for the next week and will be back on 03/12

jamesdeanexpensify commented 7 months ago

Before we get to Spanish translations, we've been working on rewriting the copy in the Figma file and I left a few more comments for @shawnborton @anmurali when they can get to them! If either of you are able to talk through these today, it might go a bit faster.

anmurali commented 7 months ago

Addressed all your comments. @jamesdeanexpensify make sure we use the personal info flow that includes the last 4 of SSN

jamesdeanexpensify commented 7 months ago

Thanks! I added the SSN screens to the top flow in the Figma and I'll address your comments today!

shawnborton commented 6 months ago

What's the latest here? Anything you all need from me on these?

mountiny commented 6 months ago

Not sure about the designs, but @koko57 should be back tomorrow or Wednesday to resume work on it

jamesdeanexpensify commented 6 months ago

@shawnborton @anmurali some (hopefully) last comments for you both in the Fig

shawnborton commented 6 months ago

Sounds good, responded to a couple, let me know if anything else is needed specifically from me.

jamesdeanexpensify commented 6 months ago

@anmurali just one left for ya here!

koko57 commented 6 months ago

Sorry for the delay here - I had to take care of my other issue. I'll be able to get back to it and focus mainly on that tomorrow/on Friday. I'm catching up with the comments in Figma

koko57 commented 6 months ago

I'm almost finished with my other issues, so this week I'll be working mainly on this task. I've updated the copies, now polishing the logic and doing some code cleanup.

koko57 commented 6 months ago

@shawnborton While working on this task and comparing the flow to the reimbursement VBBA flow I've noticed some differences. I just want to make sure that they are deliberate:

Screenshot 2024-03-20 at 21 25 13
shawnborton commented 6 months ago

‘Ending in 1111’ text is below the bank name in VBBA not the account name

For this one, let's mirror the existing flow in the product

the steps markers are displayed on the step with choosing connection methods (while in VBBA not)

Same, happy to mirror the existing flow in product

copies for the personalInfo steps are slightly different (‘What’s your address?’ vs ‘Enter your address’)

I believe we want to use the updated copy form the Figma file, and we want to update the copy on both Wallet and VBBA flows

cc @jamesdeanexpensify @anmurali for a gut check too.

koko57 commented 6 months ago

@shawnborton thanks for such a quick response! Ok, I will use Figma copies for now, if it would need to be changed later (for either Wallet or VBBA flow), just let me know, I'll take care of it. May I also ask for Spanish translations for the copies? Or are they somehow hidden in Figma?

shawnborton commented 6 months ago

@muttmuure or @mountiny - what is the process for getting Spanish translations? Currently we do not have them in Figma.

muttmuure commented 6 months ago

This is the process!

https://stackoverflowteams.com/c/expensify/questions/10316

jamesdeanexpensify commented 6 months ago

Yes, I think we should plan to use all copy in Figma!

koko57 commented 6 months ago

Sorry, it has taken me so long to open the first PR - https://github.com/Expensify/App/pull/39038 - I wanted to do it as similar as in VBBA flow as possible and reuse as much code as possible. It was a bit hard and still I ended up with some code repetition, but I'm planning to improve it in the subsequent PRs.

For now the PR contains the changes for the Add Bank Account step. I'm planning to split the Personal Info and other steps into smaller PR. When I have them working we can replace the old flow with the new one. I will try to make code as clean as possible during the development but if there will be some need for refactor or code improvements after the flow is done I can take care of it in follow-ups after we replace the new flow with the old one.

Now I'm working on test steps and author's checklist to open the PR asap.

cc @mountiny

koko57 commented 6 months ago

opened for review: https://github.com/Expensify/App/pull/39038

mountiny commented 6 months ago

Thank you, PR is looking great so far, I am asking if the C+ is able to dedicate more time to this or if to look for different one with more capacity

mountiny commented 6 months ago

First PR was merged, great job @koko57 https://github.com/Expensify/App/pull/39038

What is the next step now and your ETA for it?

koko57 commented 6 months ago

@mountiny I've started working on Personal Info step, this one is much easier - I think most of the work here will be done by Wednesday

mountiny commented 6 months ago

Thanks

melvin-bot[bot] commented 5 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

koko57 commented 5 months ago

Issues to solve in the following PRs:

koko57 commented 5 months ago

Asking for this one, just to confirm: currently ,the offline indicator is not shown on the web (wide screen) for the first step of new VBBA flow, so the same should happen for the Enable Wallet flow? Or the offline indicator should be displayed on the wide screen and the prop should be added like it was suggested here?

Screenshot 2024-04-09 at 17 45 32

cc @shawnborton @mountiny

mountiny commented 5 months ago

@koko57 The indicator should be shown there, this is a bug

koko57 commented 5 months ago

Sorry for the delay again - I had to resolve some comments and fix some bugs in my other PR. I've fixed both issues that were reported, I already have the UI (reused from VBBA flow). Still need to do make some adjustments to the logic, but I should be able to open my next PR until EOW.

koko57 commented 5 months ago

@shawnborton @mountiny I've just spotted one inconsistency with the old flow - we don't have an input for the phone number in the new flow. We no longer should send this in the request?

shawnborton commented 5 months ago

cc @anmurali as well - if the old flow asked for a phone number, I assume we still need it here as well.

@koko57 can you share a screenshot of what it looks like in the old flow? Thanks!