Extracted TransactionPoster from PurchasesOrchestrator

[NachoSoto]

Necessary refactor for #2533. This allows CustomerInfoManager to have an instance of TransactionPosterType instead of the whole PurchasesOrchestrator.

Essentially this becomes the hierarchy now:

Changes:

• Created stateless TransactionPoster
• Extracted handlePurchasedTransaction into TransactionPoster
• The new implementation there is mostly the same, with the one difference that it's stateless. Instead of holding purchaseCompleteCallbacksByProductID, that's done by PurchasesOrchestrator. The methods there all take a callback, so they're easier to use and don't require keeping a callback around.
• Created PurchaseSource to abstract the combo of isRestore and InitiationSource. This is still messy using the deprecated allowSharingAppStoreAccount, but at least that's abstracted out and the new TransactionPoster won't deal with that deprecated property.
• I didn't mock TransactionPoster, which means that all existing tests that check PurchasesOrchestrator continue working the same way. This was on purpose to keep this refactor as simple as possible.
• This supersedes PurchasesOrchestrator: simplified StoreKit2TransactionListenerDelegate transaction handling #2536: the change made there was now possible here without any integration test failures, thanks to the fact that we can process transactions independently without messing with the purchaseCompleteCallbacksByProductID state now.

Future improvements

This is just a start, there's a lot more than can be done to simplify the PurchasesOrchestrator monster. The main thing would be to remove all the custom code for restorePurchases, which is largely the same as what's in TransactionPoster now.

Notes:

Just posting this here for posteriority. I had to trace the call hierarchy for both SK1 and SK2. This "block" is what's now in TransactionPoster:

[codecov]

Codecov Report

Merging #2540 (0d27ed5) into main (b8fba2e) will increase coverage by 0.19%.
The diff coverage is 98.43%.

@@            Coverage Diff             @@
##             main    #2540      +/-   ##
==========================================
+ Coverage   87.72%   87.92%   +0.19%     
==========================================
  Files         200      201       +1     
  Lines       13789    13858      +69     
==========================================
+ Hits        12096    12184      +88     
+ Misses       1693     1674      -19     

Impacted Files	Coverage Δ
Sources/Misc/Concurrency/OperationDispatcher.swift	65.71% <ø> (ø)
Sources/Purchasing/Purchases/Purchases.swift	77.53% <92.30%> (+0.24%)	⬆️
...urces/Purchasing/Purchases/TransactionPoster.swift	98.19% <98.19%> (ø)
...s/Purchasing/Purchases/PurchasesOrchestrator.swift	85.04% <100.00%> (-1.46%)	⬇️

... and 5 files with indirect coverage changes

[NachoSoto]

I hated this new name, but just realized it matches our other AttributionPoster 🤝

[aboedo]

Looks great! When we're done with the refactors it'd be great to get a tests specifically for TransactionPoster, even though we're currently reusing the existing tests

[aboedo]

is the swift version check no longer needed? I thought you could still build using Xcode 13 and upload to app store

[NachoSoto]

I forgot to explain this.
Until this PR, turns out we weren't relying on OperationDispatcher being Sendable. This broke on Swift 5.8, because we didn't have an #else.
I don't think they're ever going to make DispatchQueue Sendable, but our @unchecked Sendable is still valid, hence why I removed the conditional.

[aboedo]

this (protocol + implementation) should be our new standard for any helper classes, it's so much nicer for testing afterwards

[NachoSoto]

Totally. It's so weird that some of our mocks rely only partially on the parent implementation. Plus what we lose on performance by requiring dynamic dispatch versus being able to make the classes final (and being able to make them Sendable).

[aboedo]

It took me a bit to understand this method tbh, there's a lot of jumping back and forth.

To validate my understanding, we're calling handlePurchasedTransaction, then calling completed(Result) but entirely ignoring the result?

If that's the case, maybe it'd be better to explicitly mention it or directly not have a completion?
Also, I love the trailing closure syntax as much as the next person, but in this case it made me even more confused, given that we're calling it from within the body of another closure syntax, with the completion method being the parameter that we get from the parent call. It just got confusing.

[aboedo]

the checked continuation happens regardless of us calling completion, right? So we don't need that completion unless I'm missing something?

[NachoSoto]

Good point. I think it's still useful to make this method wait for this to finish, but we don't actually care about the result so I'll remove that.

[NachoSoto]

Actually this is used because of the try. This becomes a bit simpler in #2542:

_ = try await Async.call { completed in
    self.transactionPoster.handlePurchasedTransaction(
        StoreTransaction.from(transaction: transaction),
        data: .init(
            appUserID: self.appUserID,
            presentedOfferingID: nil,
            unsyncedAttributes: self.unsyncedAttributes,
            storefront: storefront,
            source: .init(
                isRestore: self.allowSharingAppStoreAccount,
                initiationSource: .queue
            )
        )
    ) { result in
        completed(result.mapError(\.asPurchasesError))
    }
}

[aboedo]

I realize this was already there, but I'm not sure it's necessarily easier to read having this one-line function called twice vs just calling the same line twice manually

[aboedo]

lmk if it was done for @Sendable purposes

[NachoSoto]

I don't think so, it was to remove the duplication so we didn't accidentally use dispatchOnMainQueue or something in one of them.

[aboedo]

... or a renewal, right?

I realize a renewal is a type of purchase, but given that in parts of the codebase we make distinctions between one and the other to determine initiation source, I think it might be good to clarify

[aboedo]

and it might also be good to clarify that this won't be called from restores, since restores aren't tied to specific transactions

[NachoSoto]

This was an old comment but yeah I'll update 👍🏻

[NachoSoto]

Actually I'll remove this comment (in #2542 so I don't have to wait on CI for a comment), it's not that relevant in this context anymore. It was more important because PurchasesOrchestrator has a slightly separate flow for sync/restore.

[NachoSoto]

When we're done with the refactors it'd be great to get a tests specifically for TransactionPoster, even though we're currently reusing the existing tests

Good idea 👍🏻 doing that right now in a separate PR.

[NachoSoto]

Tests: #2557