Refactored TransactionPoster: removed 2 dependencies and abstracted parameters
#2542
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NachoSoto
commented
May 25, 2023
| expect(self.mockSubscriberAttributesManager.invokedMarkAttributesParameters!.syncedAttributes) == mockAttributes | ||
| expect(self.mockSubscriberAttributesManager.invokedMarkAttributesParameters!.appUserID) == | ||
| mockIdentityManager.currentAppUserID | ||
| expect(self.mockSubscriberAttributesManager.invokedMarkAttributes).toEventually(beTrue()) |
There was a problem hiding this comment.
This is the only semantic change. Because of this refactor, attributes are marked as synced after the transactions are finished. The test below (like 732) already had toEventually. This is purely an ordering change and shouldn't have any impact.
Codecov Report
@@ Coverage Diff @@
## main #2542 +/- ##
==========================================
+ Coverage 87.88% 87.95% +0.07%
==========================================
Files 201 201
Lines 13871 13868 -3
==========================================
+ Hits 12190 12198 +8
+ Misses 1681 1670 -11
|
NachoSoto
force-pushed
the
transaction-poster-2
branch
from
bd092b3
to
50e5ce3
Compare
NachoSoto
force-pushed
the
transaction-poster-3
branch
from
4fcfb63
to
34de8f5
Compare
This was referenced May 26, 2023
NachoSoto
force-pushed
the
transaction-poster-2
branch
from
50e5ce3
to
2eba02c
Compare
NachoSoto
force-pushed
the
transaction-poster-3
branch
from
34de8f5
to
55a39c0
Compare
… parameters Follow up to #2540.
NachoSoto
force-pushed
the
transaction-poster-3
branch
from
55a39c0
to
397edda
Compare
tonidero
approved these changes
May 29, 2023
| initiationSource: initiationSource, | ||
| subscriberAttributesByKey: subscriberAttributesToPost) | ||
| let postData = PostReceiptDataOperation.PostData( | ||
| transactionData: transactionData.withAttributesToPost(subscriberAttributesToPost), |
There was a problem hiding this comment.
So we are recreating the transaction data here since the consentStatus could be added to the attributes correct? If so, I wonder if we could extract that piece of code adding that attribute to the moment where we create the PurchasedTransactionData to avoid this recreation. Not a blocker though.
There was a problem hiding this comment.
So we are recreating the transaction data here since the consentStatus could be added to the attributes correct
Yeah, which matches the existing behavior
If so, I wonder if we could extract that piece of code adding that attribute to the moment where we create the PurchasedTransactionData to avoid this recreation. Not a blocker though.
That might make more sense, but it would change the behavior and I'm trying to keep this exactly working just like before.
| data: .init( | ||
| appUserID: self.appUserID, | ||
| presentedOfferingID: offeringID, | ||
| unsyncedAttributes: unsyncedAttributes, |
There was a problem hiding this comment.
It seems that we were not syncing unsyncedAttributes in some of these posts. I think it shouldn't cause any issues, but it seems to be a behavior change so I just wanted to bring it up.
There was a problem hiding this comment.
That was being done inside by TransactionPoster because it had access to the unsynced attributes there.
| source: PurchaseSource, | ||
| completion: @escaping PurchaseCompletedBlock | ||
| data: PurchasedTransactionData, | ||
| completion: @escaping CustomerAPI.CustomerInfoResponseHandler |
There was a problem hiding this comment.
Hmm it's a bit weird to have a reference to CustomerAPI in the TransactionPoster... I think it's ok though.
There was a problem hiding this comment.
CustomerAPI also has the post(receiptData:) method.
NachoSoto
added a commit
that referenced
this pull request
May 30, 2023
…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.
NachoSoto
added a commit
that referenced
this pull request
May 31, 2023
…#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 was referenced May 31, 2023
Closed
NachoSoto
added a commit
that referenced
this pull request
Jun 1, 2023
**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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
TransactionPosterno longer has references toIdentityManagerandAttributionmarkSyncedIfNeededis no longer inTransactionPoster, moved back toPurchasesOrchestratorTransactionPoster's callback to returnResult<CustomerInfo, BackendError>, providing greater flexibility for users to be able to read the specific errors.PurchasedTransactionDatato be reduce the number of parameters passed around. This removed the need for 2swiftlint: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, sinceTransactionPostercould no longer read these directly.