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.34k stars 2.77k forks source link

[Payment card / Subscription] Make `AddBillingCardAndRequestPolicyOwnerChange` 1:1:1 #46995

Open blimpich opened 1 month ago

blimpich commented 1 month ago

Problem

AddBillingCardAndRequestPolicyOwnerChange isn't 1:1:1.

Solution

Make it 1:1:1.

Issue Owner

Current Issue Owner: @blimpich

melvin-bot[bot] commented 4 weeks ago

Triggered auto assignment to @strepanier03 (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.

garrettmknight commented 4 weeks ago

Adding BZ to monitor and take this one off hold when it's ready. Held on https://github.com/Expensify/App/issues/46994

strepanier03 commented 3 weeks ago

Thanks Garret, I'll keep an eye on it.

melvin-bot[bot] commented 3 weeks ago

@strepanier03 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 3 weeks ago

@strepanier03 Huh... This is 4 days overdue. Who can take care of this?

blimpich commented 3 weeks ago

Not overdue, held

melvin-bot[bot] commented 2 weeks ago

@strepanier03 Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 2 weeks ago

@strepanier03 10 days overdue. Is anyone even seeing these? Hello?

blimpich commented 2 weeks ago

Making this weekly since its waiting on other issues to be done first.

strepanier03 commented 2 weeks ago

46994 is closed now.

There is one remaining PR that we should wait for I think, then this can come off hold.

blimpich commented 1 week ago

Taking it off hold, this should be good to work on now. There's two parts to it:

need to move all this logic down into the Auth command TakePolicyOwnership
Make a new command in Auth called AddBillingCardAndRequestPolicyOwnerChange that calls the CreateFund command and then the TakePolicyOwnership command.

Could be split up into two different issues if the person who picks this up wants to. I currently don't have the bandwidth to pick this up though.

blimpich commented 1 week ago

Picking up, have bandwidth now

blimpich commented 1 week ago

Created new branches for working on this, wasn't able to spend much time on it today due to chores, but intend to get to this Tuesday/Wednesday this week.

I'll be OOO for most and potentially all of Thursday/Friday, so I estimate that I won't get a full PR out for this until sometime next week.

blimpich commented 1 week ago

Wasn't able to work on this today, got assigned a big chore to monitor API performance for the week. That took up most of my day. Hoping I'll get to this either tomorrow or Monday.

blimpich commented 6 days ago

There's a lot of different parts to this so I'm going to try and make many small PRs to complete this issue rather than one larger one. That way we also hopefully make it easier to review, and more likely that we'll find bugs before releasing to users.

blimpich commented 6 days ago

Lets make a checklist to break up what to move:

[ ] changeLog creation
[x] queue recommended account review job, not sure if this is actually even possible right now
[x] transferAccountManager call
[ ] notification to old owner
[x] retention whisper, also unsure if this one is possible but I will look into it

Today I worked on the first checkbox, made good progress.

blimpich commented 2 days ago

Spent time on this, got the Auth PR out for review for the first checkbox, waiting on that to merge before putting web pr up for review.

blimpich commented 1 day ago

Confirmed that we cannot in fact do checkbox number 2 and 5 because they are bedrock jobs and have no way of being queued from Auth. Crossed them off and created this new issue to add the job queues to the verifySetupIntentAndRequestPolicyOwnerChange web call.

blimpich commented 1 day ago

Calling it a day. PRs for the first checkbox are in review or are held waiting for dependent Auth PR. Will work on the remaining checkboxes tomorrow.

blimpich commented 21 hours ago

Couldn't make much progress on this today, caught up with other chores/bugs.

blimpich commented 19 minutes ago

Looked into the 3rd checkbox, gets really complicated really quick but basically its also impossible to fully move over into Auth due to the fact that it calls ChatBotAPI::updateDomainAccountManager which queues jobs. Frustrating but so much of this command is just not possible to move into Auth right now. Checking off the third checkbox.

The 4th checkbox I know for a fact can be ported to Auth since we can queue notifications. So that will be the last thing I do for this issue.