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.54k stars 2.88k forks source link

[P2P Distance] Move creating the payer account for money requests from Web-E to Auth #37061

Closed neil-marcellini closed 5 months ago

neil-marcellini commented 8 months ago

Move creating the payer account for money requests from Web-E to Auth following this plan.

Heads up, I have extracted this into a shared util function for now, but the main task of this issue remains the same. This migration will support 1:1:1 and will be a fairly large amount of work. We're tackling it in a separate issue to not block progress of the new feature as whole, and to keep it a focused migration.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01951ae9eca521e68a
  • Upwork Job ID: 1760485484024840192
  • Last Price Increase: 2024-02-22
melvin-bot[bot] commented 8 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01951ae9eca521e68a

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to Contributor Plus for review of internal employee PR - @ishpaul777 (Internal)

neil-marcellini commented 8 months ago

Totally backend, no C+ needed

neil-marcellini commented 8 months ago

I'm jumping in on this refactor now because it will be helpful for enabling distance splits where we also plan to move the split normalization to Auth.

neil-marcellini commented 8 months ago

Decision: The first step of this process is normalizing the email to a valid login email, with getNormalizedCanonicalLogin. We need to use the Twilio API to do that, so I'm going to leave that part in Web-E since I think it would be a lot of work to move it to Auth, and it seems like we keep most external API calls in Web-E. It also doesn't involve an extra Auth request so we don't gain much moving it to auth.

neil-marcellini commented 8 months ago

At a high level we do this for requesting money:

  1. Normalize the login
  2. Get an email token to check if the account is open
  3. If not, get account or create closed
  4. Send the personal details of the new user to the requesting account

At a high level we do this for creating splits:

  1. If there's an email normalize the login
  2. Call getAccountOrCreate
  3. Return account data
  4. Later push the personal details of all created participants to the requesting account.
  5. If there's only an accountID instead of an email, get the account and return

So both are pretty similar, but split doesn't try to get the token first and it also uses a different function.

I'll look into what exactly needs to be done for deciding whether to create the account in Auth, and then actually creating it. At first glance it's odd that one appears to be creating an open account.

neil-marcellini commented 8 months ago

I need to learn a bit more about what the partner list is and how it operates before I can fully understand this. I'll look in SO and ask a question if there isn't something there already. I'll also share to engineering chat for help and look into the code myself.

I'll come back to this issue later.

neil-marcellini commented 8 months ago

I also kind of jumped the gun, and I should work on this issue first [P2P Distance] Update CreateDistanceRequest params in App

neil-marcellini commented 8 months ago

I have some drafts up. I'm trying to migrate a fairly small part of it to Auth and keep a lot of the initial parameter validation in Web-E because some of that is fairly involved and hard to move to Auth.

neil-marcellini commented 8 months ago

❗❗ Heads up, I'm going to be OOO working from Spain 🇪🇸 part time until 3/28. Most days I will be working 50%, some days 100%. Please DM me if something needs urgent attention.❗❗

neil-marcellini commented 8 months ago

I'm starting on this again now

neil-marcellini commented 7 months ago

I paused work on this on Friday after discovering that P2P distance was reverted in Web-E. I have a PR to re-enable it, so I plan to merge that branch into my Web-E draft PR, change the base of the PR to it, and then start working on an integration test.

neil-marcellini commented 7 months ago

I'm focusing on supporting distance for track expense first, then I will get back to my draft PRs for this.

neil-marcellini commented 7 months ago

Same, it might be a while so moving this to weekly.

neil-marcellini commented 7 months ago

Working on higher priorities related to p2p distance tracked expense editing still. I've made it clear that this issue is available, otherwise I'll get back to it in a week or so

neil-marcellini commented 6 months ago

Unfortunately still focused on other stuff

neil-marcellini commented 6 months ago

Holding still

neil-marcellini commented 6 months ago

I'm finally ready to jump back in on this. It might take me a little while to figure out where I left off and the best path forward.

neil-marcellini commented 6 months ago

PRs are up for review! Create closed payer accounts in Auth [HOLD Auth 10164] Move creating closed payer accounts to Auth

neil-marcellini commented 5 months ago

Still waiting for the Web-E PR to deploy

neil-marcellini commented 5 months ago

This has been done for a while