Extracted TransactionPoster from PurchasesOrchestrator#2540
Conversation
8bc298f to
dce6717
Compare
TransactionPoster from PurchasesOrchestratorTransactionPoster from PurchasesOrchestrator
Codecov Report
@@ 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
|
|
I hated this new name, but just realized it matches our other |
dce6717 to
0d27ed5
Compare
aboedo
left a comment
There was a problem hiding this comment.
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
|
|
||
| #if swift(<5.8) | ||
| // `DispatchQueue` is not `Sendable` as of Swift 5.7, but this class performs no mutations. | ||
| // `DispatchQueue` is not `Sendable` as of Swift 5.8, but this class performs no mutations. |
There was a problem hiding this comment.
is the swift version check no longer needed? I thought you could still build using Xcode 13 and upload to app store
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| /// A type that can post receipts as a result of a purchased transaction. | ||
| protocol TransactionPosterType: AnyObject, Sendable { |
There was a problem hiding this comment.
this (protocol + implementation) should be our new standard for any helper classes, it's so much nicer for testing afterwards
There was a problem hiding this comment.
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).
| _ = try await Async.call { completed in | ||
| self.transactionPoster.handlePurchasedTransaction( | ||
| StoreTransaction.from(transaction: transaction), | ||
| presentedOfferingID: nil, | ||
| storefront: storefront, | ||
| source: .init( | ||
| isRestore: self.allowSharingAppStoreAccount, | ||
| initiationSource: .queue | ||
| ) | ||
| ) { _, customerInfo, error, _ in | ||
| completed(Result(customerInfo, error)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
the checked continuation happens regardless of us calling completion, right? So we don't need that completion unless I'm missing something?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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))
}
}| @Sendable | ||
| func complete() { | ||
| self.operationDispatcher.dispatchOnMainActor(completion) | ||
| } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
lmk if it was done for @Sendable purposes
There was a problem hiding this comment.
I don't think so, it was to remove the duplication so we didn't accidentally use dispatchOnMainQueue or something in one of them.
|
|
||
| private extension TransactionPoster { | ||
|
|
||
| /// Called as a result a purchase. |
There was a problem hiding this comment.
... 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
There was a problem hiding this comment.
and it might also be good to clarify that this won't be called from restores, since restores aren't tied to specific transactions
There was a problem hiding this comment.
This was an old comment but yeah I'll update 👍🏻
There was a problem hiding this comment.
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.
Good idea 👍🏻 doing that right now in a separate PR. |
|
Tests: #2557 |
This was necessary for a follow-up refactor to #2540. The main change is in `Backend`, the rest are test changes that are required now that the closure is `@Sendable`.
… parameters Follow up to #2540.
… parameters (#2542) Follow up to #2540. This was necessary to ensure that we can implement #2533 without reference cycles. This was the hierarchy before this change:  The red arrows are what this PR removes. ## Changes: - `TransactionPoster` no longer has references to `IdentityManager` and `Attribution` - `markSyncedIfNeeded` is no longer in `TransactionPoster`, moved back to `PurchasesOrchestrator` - Changed `TransactionPoster`'s callback to return `Result<CustomerInfo, BackendError>`, providing greater flexibility for users to be able to read the specific errors. - Extracted `PurchasedTransactionData` to be reduce the number of parameters passed around. This removed the need for 2 `swiftlint:disable:next function_parameter_count`. This is the majority of this diff. It was helpful to combine in this PR because I needed to add another parameter to the flow: `unsyncedAttributes`, since `TransactionPoster` could no longer read these directly.
…2533) This fixes the last edge-case for offline entitlements. Solves the following scenario: ```swift func testPurchaseWhileServerIsDownPostsReceiptWhenForegroundingApp() async throws { let logger = TestLogHandler() // 1. Purchase while server is down self.serverDown() try await self.purchaseMonthlyProduct() logger.verifyMessageWasNotLogged("Finishing transaction") // 2. Server is back self.serverUp() // 3. Request current CustomerInfo let info1 = try await Purchases.shared.customerInfo() try await self.verifyEntitlementWentThrough(info1) // 4. Ensure transaction is finished logger.verifyMessageWasLogged("Finishing transaction", level: .info) // 5. Restart app Purchases.shared.invalidateCustomerInfoCache() await self.resetSingleton() // 6. To ensure (with a clean cache) that the receipt was posted let info2 = try await Purchases.shared.customerInfo() try await self.verifyEntitlementWentThrough(info2) } ``` This race condition happened when fetching `CustomerInfo` after we had computed an offline `CustomerInfo` with a purchase: if the server is still not aware of the purchase, then we lose that until some arbitrary amount of time when `StoreKit` might decide to send the pending transactions in the queue. For that reason, instead of waiting, we proactively check now whenever `CustomerInfoManager` fetches new info. The refactors #2540 and #2542 where required to break a retain cycle. Also thanks to #2534 we know for sure that there aren't any cycles. Note that this is an iOS 15.0+ only feature. It would be useful to better ensure consistency in older versions, but it's not strictly required, as those versions don't support offline entitlements either.
Follow up to #2540. We could add more tests (which are a lot simpler than `PurchasesOrchestratorTests`) but this is just a start.
…#2566) Cherry-picked #2549 post refactors (#2540 and #2542). This becomes a lot simpler since #2542, thanks to the fact that we can put this new parameter in `PurchasedTransactionData`. ### Changes: - Added `aadAttributionToken` to `PurchasedTransactionData` - Exposed `AttributionPoster.adServicesTokenToPostIfNeeded` - Added snapshot test to verify it's sent - Added `PurchasesOrchestrator` tests (SK1/SK2) for sending the attribution token - Added missing `PurchasesOrchestrator` tests for sending attributes - Added log when marking `AdServices` token as synced - Exposed `Purchases.attribution` for custom entitlement computation framework (and added to API tester) - Exposed `Purchases.enableAdServicesAttributionTokenCollection` for custom entitlement computation framework (and added to API tester)
**This is an automatic release.** ### New Features * `Offline Entitlements`: use offline-computed `CustomerInfo` when server is down (#2368) via NachoSoto (@NachoSoto) ### Bugfixes * `AppleReceipt.debugDescription`: don't pretty-print JSON (#2564) via NachoSoto (@NachoSoto) * `SK2StoreProduct`: fix crash on iOS 12 (#2565) via NachoSoto (@NachoSoto) * `GetCustomerInfo` posts receipts if there are pending transactions (#2533) via NachoSoto (@NachoSoto) ### Performance Improvements * `PurchasedProductsFetcher`: cache current entitlements (#2507) via NachoSoto (@NachoSoto) * Performance: new check to ensure serialization / deserialization is done from background thread (#2496) via NachoSoto (@NachoSoto) ### Dependency Updates * Bump fastlane from 2.212.2 to 2.213.0 (#2544) via dependabot[bot] (@dependabot[bot]) ### Other Changes * `CustomerInfoManager`: post all unfinished transactions (#2563) via NachoSoto (@NachoSoto) * `PostReceiptOperation`: added ability to also post `AdServices` token (#2566) via NachoSoto (@NachoSoto) * `Offline Entitlements`: improved computation log (#2562) via NachoSoto (@NachoSoto) * Added `TransactionPoster` tests (#2557) via NachoSoto (@NachoSoto) * Refactored `TransactionPoster`: removed 2 dependencies and abstracted parameters (#2542) via NachoSoto (@NachoSoto) * `CustomerInfoManagerTests`: wait for `getAndCacheCustomerInfo` to finish (#2555) via NachoSoto (@NachoSoto) * `StoreTransaction`: implemented `description` (#2556) via NachoSoto (@NachoSoto) * `Backend.ResponseHandler` is now `@Sendable` (#2541) via NachoSoto (@NachoSoto) * Extracted `TransactionPoster` from `PurchasesOrchestrator` (#2540) via NachoSoto (@NachoSoto) * `enableAdServicesAttributionTokenCollection`: added integration test (#2551) via NachoSoto (@NachoSoto) * `AttributionPoster`: replaced hardcoded strings with constants (#2548) via NachoSoto (@NachoSoto) * `DefaultDecodable`: moved to `Misc/Codable/DefaultDecodable.swift` (#2528) via NachoSoto (@NachoSoto) * `CircleCI`: specify device to run `backend_integration_tests` (#2547) via NachoSoto (@NachoSoto) * Created `StoreKit2TransactionFetcher` (#2539) via NachoSoto (@NachoSoto) * Fix load shedder integration tests (#2546) via Josh Holtz (@joshdholtz) * Fix doc on `Offering.getMetadataValue` (#2545) via Josh Holtz (@joshdholtz) * Extracted and tested `AsyncSequence.extractValues` (#2538) via NachoSoto (@NachoSoto) * `Offline Entitlements`: don't compute offline `CustomerInfo` when purchasing a consumable products (#2522) via NachoSoto (@NachoSoto) * `OfflineEntitlementsManager`: disable offline `CustomerInfo` in observer mode (#2520) via NachoSoto (@NachoSoto) * `BasePurchasesTests`: fixed leak detection (#2534) via NachoSoto (@NachoSoto) * `PurchaseTesterSwiftUI`: added `ProxyView` to `iOS` (#2531) via NachoSoto (@NachoSoto) * `PurchasedProductsFetcher`: removed `AppStore.sync` call (#2521) via NachoSoto (@NachoSoto) * `PurchaseTesterSwiftUI`: added new window on Mac to manage proxy (#2518) via NachoSoto (@NachoSoto) * `PurchasedProductsFetcher`: added log if fetching purchased products is slow (#2515) via NachoSoto (@NachoSoto) * `Offline Entitlements`: disable for custom entitlements mode (#2509) via NachoSoto (@NachoSoto) * `Offline Entitlements`: fixed iOS 12 tests (#2514) via NachoSoto (@NachoSoto) * `PurchasedProductsFetcher`: don't throw errors if purchased products were found (#2506) via NachoSoto (@NachoSoto) * `Offline Entitlements`: allow creating offline `CustomerInfo` with empty `ProductEntitlementMapping` (#2504) via NachoSoto (@NachoSoto) * `Offline Entitlements`: integration tests (#2501) via NachoSoto (@NachoSoto) * `CustomerInfoManager`: don't cache offline `CustomerInfo` (#2378) via NachoSoto (@NachoSoto) * `DangerousSettings`: debug-only `forceServerErrors` (#2486) via NachoSoto (@NachoSoto) * `CocoapodsInstallation`: fixed `Xcode 14.3.0` issue (#2489) via NachoSoto (@NachoSoto) * `CarthageInstallation`: removed workaround (#2488) via NachoSoto (@NachoSoto) --------- Co-authored-by: NachoSoto <ignaciosoto90@gmail.com>
Necessary refactor for #2533. This allows
CustomerInfoManagerto have an instance ofTransactionPosterTypeinstead of the wholePurchasesOrchestrator.Essentially this becomes the hierarchy now:
Changes:
TransactionPosterhandlePurchasedTransactionintoTransactionPosterpurchaseCompleteCallbacksByProductID, that's done byPurchasesOrchestrator. The methods there all take a callback, so they're easier to use and don't require keeping a callback around.PurchaseSourceto abstract the combo ofisRestoreandInitiationSource. This is still messy using the deprecatedallowSharingAppStoreAccount, but at least that's abstracted out and the newTransactionPosterwon't deal with that deprecated property.TransactionPoster, which means that all existing tests that checkPurchasesOrchestratorcontinue working the same way. This was on purpose to keep this refactor as simple as possible.PurchasesOrchestrator: simplifiedStoreKit2TransactionListenerDelegatetransaction 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 thepurchaseCompleteCallbacksByProductIDstate now.Future improvements
This is just a start, there's a lot more than can be done to simplify the
PurchasesOrchestratormonster. The main thing would be to remove all the custom code forrestorePurchases, which is largely the same as what's inTransactionPosternow.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: