[Payment card / Subscription] make TransferPolicyOwnership closer to 1:1:1 and move as much code as possible to Auth

[blimpich]

Problem

TransferPolicyOwnership is not 1:1:1. We strive for all our commands to be 1:1:1 and in addition to the inherent benefit of making this command 1:1:1 this will also unblock #46933 which is necessary in order to create SCA supported flow to add a billing card while transferring ownership.

Solution

Make TransferPolicyOwnership 1:1:1 and move as much of the web code as possible to auth.

[blimpich]

Working on this today.

[blimpich]

Note: even though the command in web is TransferPolicyOwnership in auth its TakePolicyOwnership. That tripped me up.

Update: made a draft PR, slow work, lots of details. Trying my best to divide it up into smaller and smaller bite sized pieces.

[blimpich]

Difficulty moving some of the calls to auth, confused about forwarding commands and using them in Auth. Though seems like this is what I want. Synchronous and able to check for errors.

Seems like the pattern is to set repeek to true, queue the https call, return, and then look for the https call in the repeek. Good explanation in this SO.

[blimpich]

Made good progress today. I got my first "draft" of the changes done for the auth PR, same for the web PR (which is much simpler), and I also was able to save myself some work by figuring out that there was code in web that was no longer needed and could be deleted entirely instead of being ported into Auth (PR).

Still lots of work to be done.

[melvin-bot]

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

[blimpich]

Not overdue. Was out on Friday and then it was the weekend. Working on this today.

[blimpich]

Got it compiling locally without errors! Now to test it out manually and then also probably write some tests / assessing what automated tests already cover this functionality.

Update: looks like I've broken some tests in auth and in web with these changes, so first gotta figure out that.

[blimpich]

Fixed some of the auth tests but not all. Some seems to be quite odd and hard to figure out why they are failing. Pushed up my updates branches with the work I have done today.

[blimpich]

Spent a little more time and didn't fix any tests, but I did get it to work manually after making one more small change to my web-expensify PR. Logging off for now, will continue work on this tomorrow.

[blimpich]

Fixed all the auth tests, now need to fix the broken web tests.

[blimpich]

Made some progress on broken web tests but still working through them. Pushed up all my new commits.

[blimpich]

Chugging along. Fixed some of the web tests but not all of them. Very difficult to debug and find all the errors, but I did find quite a few of them. Some incorrect onyx updates, bad messages on Auth responses, and just some straight up typos/bugs that I missed on my first pass. Biggest revelation was that I need to know whether the request is coming from new dot or not in order to be able to get the web tests to pass. I thought I could get around that but it seems excruciatingly difficult to do so. So I made this PR to help me with that.

Progress is being made. I'm hoping this PR will be ready for review by the end of the week but its possible it won't be.

[blimpich]

Down to one last broken test in the web tests. So close!

[blimpich]

Alright I was hoping to avoid this but we need to temporarily change some functionality. The TransferOwnershipTest::testTransferOwnershipNewDotOwnerAmountOwed test has proved to me that there is a flow that we simply can't make work fully until we've deleted the php code that will be replaced by the auth port.

The issue is that the php code checks "does the user taking over have an amount owed, if so error out, if they don't and the owner has an amount owed, then transfer it over." So we then want the auth code to just go through and transfer the policy over like normal, but now that the amount owed has been transferred Auth now sees that the new owner does have an amount owed, and therefore is errors out when actually we want it to go through like nothing was wrong!

We're stuck in a position where we either let people who want to take over policies who already have amount owed's temporarily transfer policies or we block people from transferring policies where the owner has an amount owed. This will only be an issue temporarily, and I think I can make it such that we get all the necessary changes in one deploy.

I think the way to do this the most smoothly will be to temporarily allow people to transfer policies even though they have an amount owed.

[blimpich]

Okay here is what is left to do on this:

• get green build on Auth PR (very close, passes locally, for some reason it fails on GH though)
• get green build locally using Web-E feature branch
• cleanup Auth PR
• put Auth PR out for review
• get Auth PR merged
• immediately get Web-PR merged
• create PR and do this issue to finalize changes: revert conditional in auth/command/TakePolicyOwnership.cpp #47528

[blimpich]

Half day today, didn't make progress. I'm stumped as to why this 1 web-integration test is passing locally and failing in GH. Will come back Monday and work on it.

[blimpich]

Working on this today.

Got caught up working on another issue. I think my plate should be cleared to work on this tomorrow. Hopefully can squash that final broken test and get this into review.

[blimpich]

Okay I think I finally figured out why that last test isn't passing on the Auth PR. The integration tests we run for Auth PRs are based off the production web branch, and the production branch is waiting for changes to be deployed that we need in order to get a green build. See comment here.

So waiting for those changes to be deployed, meanwhile I'll work on the second item in the checklist.

[blimpich]

Green build on Auth PR 🎉, working on getting green build locally using the web feature branch, 1 failing test left.

[blimpich]

Got green build locally for the feature branch, 2nd checkbox is done

[blimpich]

Lots of progress today, got the auth pr out for review and cleaned up both the auth PR and web PR. Waiting for feedback now.

[blimpich]

Good progress today, got my initial round of feedback on the PR from two reviewers, working through that feedback now.

[blimpich]

Not overdue, just responded to feedback, working on getting green build again and getting approval.

[blimpich]

Not overdue, almost done with this. See checklist. Will put a PR up for the last checklist item today. Will take awhile to get a green build though I think because we will probably have to wait for our web changes to get deployed to production before we'll be able to get a green build on the web-integration tests.

[melvin-bot]

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

[blimpich]

Almost there, web changes just got deployed to production, attempting to get a green build on the last little auth PR here

[blimpich]

Got green build, PR is out for review

[blimpich]

Closing since there is a whole other issue dedicated to tracking the final checkbox of this issue and all other work is done

[blimpich]

renamed because we didn't make the command fully 1:1:1, we just made it more 1:1:1, enough to unblock other issues