Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
blimpich opened this issue Aug 7, 2024 · 23 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review

Comments

@blimpich
Copy link
Contributor

blimpich commented Aug 7, 2024

Problem

AddBillingCardAndRequestPolicyOwnerChange isn't 1:1:1.

Solution

Make it 1:1:1.

Issue OwnerCurrent Issue Owner: @blimpich
@blimpich blimpich added Engineering Weekly KSv2 Hot Pick Ready for an engineer to pick up and run with labels Aug 7, 2024
@blimpich blimpich moved this to HOT PICKS in [#whatsnext] #wave-collect Aug 7, 2024
@blimpich blimpich added Internal Requires API changes or must be handled by Expensify staff and removed Internal Requires API changes or must be handled by Expensify staff labels Aug 7, 2024
@garrettmknight garrettmknight added the Bug Something is broken. Auto assigns a BugZero manager. label Aug 20, 2024
@melvin-bot melvin-bot bot added the Overdue label Aug 20, 2024
Copy link

melvin-bot bot commented Aug 20, 2024

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.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Overdue Weekly KSv2 labels Aug 20, 2024
@garrettmknight
Copy link
Contributor

Adding BZ to monitor and take this one off hold when it's ready. Held on #46994

@melvin-bot melvin-bot bot added the Overdue label Aug 22, 2024
@strepanier03 strepanier03 changed the title [Payment card / Subscription][HOLD #46994] Make AddBillingCardAndRequestPolicyOwnerChange 1:1:1 [HOLD for 46994] [Payment card / Subscription][HOLD #46994] Make AddBillingCardAndRequestPolicyOwnerChange 1:1:1 Aug 23, 2024
@strepanier03
Copy link
Contributor

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

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 23, 2024
Copy link

melvin-bot bot commented Aug 26, 2024

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

Copy link

melvin-bot bot commented Aug 28, 2024

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

@blimpich
Copy link
Contributor Author

Not overdue, held

Copy link

melvin-bot bot commented Aug 30, 2024

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

Copy link

melvin-bot bot commented Sep 3, 2024

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

@blimpich blimpich added Weekly KSv2 and removed Daily KSv2 labels Sep 3, 2024
@melvin-bot melvin-bot bot removed the Overdue label Sep 3, 2024
@strepanier03
Copy link
Contributor

#46994 is closed now.

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

@strepanier03 strepanier03 changed the title [HOLD for 46994] [Payment card / Subscription][HOLD #46994] Make AddBillingCardAndRequestPolicyOwnerChange 1:1:1 [HOLD #47528] [Payment card / Subscription][HOLD #46994] Make AddBillingCardAndRequestPolicyOwnerChange 1:1:1 Sep 3, 2024
@blimpich blimpich changed the title [HOLD #47528] [Payment card / Subscription][HOLD #46994] Make AddBillingCardAndRequestPolicyOwnerChange 1:1:1 [HOLD #47528] [Payment card / Subscription] Make AddBillingCardAndRequestPolicyOwnerChange 1:1:1 Sep 5, 2024
@blimpich
Copy link
Contributor Author

blimpich commented Sep 5, 2024

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

  1. need to move all this logic down into the Auth command TakePolicyOwnership
  2. 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 blimpich changed the title [HOLD #47528] [Payment card / Subscription] Make AddBillingCardAndRequestPolicyOwnerChange 1:1:1 [Payment card / Subscription] Make AddBillingCardAndRequestPolicyOwnerChange 1:1:1 Sep 5, 2024
@blimpich blimpich self-assigned this Sep 9, 2024
@blimpich blimpich removed the Hot Pick Ready for an engineer to pick up and run with label Sep 9, 2024
@blimpich
Copy link
Contributor Author

blimpich commented Sep 9, 2024

Picking up, have bandwidth now

@blimpich blimpich moved this from HOT PICKS to Polish in [#whatsnext] #wave-collect Sep 9, 2024
@blimpich blimpich added Daily KSv2 and removed Weekly KSv2 labels Sep 9, 2024
@blimpich
Copy link
Contributor Author

blimpich commented Sep 9, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

blimpich commented Sep 11, 2024

Lets make a checklist to break up what to move:

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

@melvin-bot melvin-bot bot added the Overdue label Sep 16, 2024
@blimpich
Copy link
Contributor Author

blimpich commented Sep 16, 2024

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.

@melvin-bot melvin-bot bot removed the Overdue label Sep 16, 2024
@blimpich
Copy link
Contributor Author

blimpich commented Sep 16, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

blimpich commented Sep 18, 2024

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

@blimpich
Copy link
Contributor Author

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.

@melvin-bot melvin-bot bot added the Overdue label Sep 23, 2024
@blimpich
Copy link
Contributor Author

Waiting to get last bit of feedback on PR before merging, then this issue will be done: https://github.com/Expensify/Web-Expensify/pull/43595#discussion_r1771690613

@melvin-bot melvin-bot bot removed the Overdue label Sep 23, 2024
@blimpich
Copy link
Contributor Author

PR merged, waiting deployment / QA, but going to check off last checkbox right now

@blimpich blimpich added the Reviewing Has a PR in review label Sep 23, 2024
@github-project-automation github-project-automation bot moved this from Polish to Done in [#whatsnext] #wave-collect Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Engineering Internal Requires API changes or must be handled by Expensify staff Reviewing Has a PR in review
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants