From e8f389a153fbc250cb6de3ed3d98a7f15a8ab60c Mon Sep 17 00:00:00 2001 From: NachoSoto Date: Wed, 16 Aug 2023 16:26:47 -0700 Subject: [PATCH] `PurchasesOrchestrator`: fixed callback not invoked regression during downgrades (#3028) Fixes #3020. This essentially reverts #2890. As shown in #3020, downgrades are reported with the same transaction identifier as the original transaction. Therefore our approach in #2890 is invalid. We considered checking whether the `SKPayment` in the `SKPaymentTransaction` matches the one added to the queue as a way to disambiguate these transactions, but it doesn't. So we have no way of telling the difference between queue transactions and purchases. I've left the original tests as a way to at least document this behavior. --- Sources/Logging/Strings/PurchaseStrings.swift | 8 --- .../Purchases/PurchasesOrchestrator.swift | 54 ++++--------------- .../Purchases/PurchasesPurchasingTests.swift | 18 +++++-- 3 files changed, 22 insertions(+), 58 deletions(-) diff --git a/Sources/Logging/Strings/PurchaseStrings.swift b/Sources/Logging/Strings/PurchaseStrings.swift index 022e137e24..2a629e1ea6 100644 --- a/Sources/Logging/Strings/PurchaseStrings.swift +++ b/Sources/Logging/Strings/PurchaseStrings.swift @@ -42,9 +42,6 @@ enum PurchaseStrings { SKPaymentTransaction) case paymentqueue_updated_transaction(SKPaymentTransactionObserver, SKPaymentTransaction) - case paymentqueue_ignoring_callback_for_older_transaction(PurchasesOrchestrator, - StoreTransactionType, - Date) case presenting_code_redemption_sheet case unable_to_present_redemption_sheet case purchases_synced @@ -180,11 +177,6 @@ extension PurchaseStrings: LogMessage { .compactMap { $0 } .joined(separator: " ") - case let .paymentqueue_ignoring_callback_for_older_transaction(observer, transaction, date): - return "\(Strings.objectDescription(observer)): will not notify callback for transaction " + - "'\(transaction.transactionIdentifier)'. " + - "Transaction date '\(transaction.purchaseDate)' - callback date '\(date)'" - case .presenting_code_redemption_sheet: return "Presenting code redemption sheet." diff --git a/Sources/Purchasing/Purchases/PurchasesOrchestrator.swift b/Sources/Purchasing/Purchases/PurchasesOrchestrator.swift index e2cd8c89be..8c0049fba8 100644 --- a/Sources/Purchasing/Purchases/PurchasesOrchestrator.swift +++ b/Sources/Purchasing/Purchases/PurchasesOrchestrator.swift @@ -43,7 +43,7 @@ final class PurchasesOrchestrator { private let _allowSharingAppStoreAccount: Atomic = nil private let presentedOfferingIDsByProductID: Atomic<[String: String]> = .init([:]) - private let purchaseCompleteCallbacksByProductID: Atomic<[String: CallbackData]> = .init([:]) + private let purchaseCompleteCallbacksByProductID: Atomic<[String: PurchaseCompletedBlock]> = .init([:]) private var appUserID: String { self.currentUserProvider.currentAppUserID } private var unsyncedAttributes: SubscriberAttribute.Dictionary { @@ -825,28 +825,19 @@ private extension PurchasesOrchestrator { /// - Parameter restored: whether the transaction state was `.restored` instead of `.purchased`. private func purchaseSource( for productIdentifier: String, - transaction: StoreTransaction, restored: Bool ) -> PurchaseSource { let initiationSource: ProductRequestData.InitiationSource = { // Having a purchase completed callback implies that the transation comes from an explicit call // to `purchase()` instead of a StoreKit transaction notification. - // As long as the transaction date is _after_ the completion block was added. - let completionBlockDate = self.purchaseCompleteCallbacksByProductID.value[productIdentifier]? - .creationDate - - switch (completionBlockDate, restored) { - case let (.some(completionBlockDate), false): - if transaction.purchaseDate > completionBlockDate { - return .purchase - } else { - return .queue - } + let hasPurchaseCallback = self.purchaseCompleteCallbacksByProductID.value.keys.contains(productIdentifier) + switch (hasPurchaseCallback, restored) { + case (true, false): return .purchase // Note that restores initiated through the SDK with `restorePurchases` // won't use this method since those set the initiation source explicitly. - case (.some, true): return .restore - case (.none, _): return .queue + case (true, true): return .restore + case (false, _): return .queue } }() @@ -916,18 +907,6 @@ extension PurchasesOrchestrator: StoreKit2StorefrontListenerDelegate { private extension PurchasesOrchestrator { - struct CallbackData { - - var completion: PurchaseCompletedBlock - var creationDate: Date - - init(_ completion: @escaping PurchaseCompletedBlock, _ creationDate: Date = Date()) { - self.completion = completion - self.creationDate = creationDate - } - - } - /// - Returns: whether the callback was added @discardableResult func addPurchaseCompletedCallback( @@ -956,7 +935,7 @@ private extension PurchasesOrchestrator { return false } - callbacks[productIdentifier] = .init(completion) + callbacks[productIdentifier] = completion return true } } @@ -964,22 +943,8 @@ private extension PurchasesOrchestrator { func getAndRemovePurchaseCompletedCallback( forTransaction transaction: StoreTransaction ) -> PurchaseCompletedBlock? { - return self.purchaseCompleteCallbacksByProductID.modify { callbacks -> PurchaseCompletedBlock? in - guard let value = callbacks[transaction.productIdentifier] else { return nil } - - if !transaction.hasKnownPurchaseDate || value.creationDate <= transaction.purchaseDate { - callbacks.removeValue(forKey: transaction.productIdentifier) - return value.completion - } else { - // This callback was added to handle a purchase _after_ the transaction was created, - // therefore it should not be notified. - Logger.verbose(Strings.purchase.paymentqueue_ignoring_callback_for_older_transaction( - self, - transaction, - value.creationDate - )) - return nil - } + return self.purchaseCompleteCallbacksByProductID.modify { + return $0.removeValue(forKey: transaction.productIdentifier) } } @@ -1115,7 +1080,6 @@ private extension PurchasesOrchestrator { aadAttributionToken: adServicesToken, storefront: storefront, source: self.purchaseSource(for: purchasedTransaction.productIdentifier, - transaction: purchasedTransaction, restored: restored) ) ) { result in diff --git a/Tests/UnitTests/Purchasing/Purchases/PurchasesPurchasingTests.swift b/Tests/UnitTests/Purchasing/Purchases/PurchasesPurchasingTests.swift index 163c62e45f..2d7a29f3e7 100644 --- a/Tests/UnitTests/Purchasing/Purchases/PurchasesPurchasingTests.swift +++ b/Tests/UnitTests/Purchasing/Purchases/PurchasesPurchasingTests.swift @@ -80,7 +80,11 @@ class PurchasesPurchasingTests: BasePurchasesTests { expect(self.backend.postedIsRestore) == false } - func testPurchaseCallbackIsNotInvokedWhenProcessingQueueTransactionForSameProduct() { + func testPurchaseCallbackIsInvokedWhenProcessingQueueTransactionForSameProduct() { + // This documents a race condition that we can't detect in the implementation + // where `PurchasesOrchestrator` can't tell the difference between `StoreKit 1` sending + // us a transaction from the queue, and sending us a transaction as a result of adding an `SKPayment`. + let product = StoreProduct(sk1Product: MockSK1Product(mockProductIdentifier: "com.product.id1")) let mockPayment = MockPayment() @@ -104,16 +108,20 @@ class PurchasesPurchasingTests: BasePurchasesTests { self.storeKit1Wrapper.delegate?.storeKit1Wrapper(self.storeKit1Wrapper, updatedTransaction: transaction) expect(self.backend.postReceiptDataCalled) == true - expect(self.backend.postedInitiationSource) == .queue + expect(self.backend.postedInitiationSource) == .purchase expect(self.storeKit1Wrapper.finishCalled).toEventually(beTrue()) // Avoid false positives because the callback hasn't been invoked yet expect(self.mockOperationDispatcher.pendingMainActorDispatches.value).toEventually(equal(0)) - expect(callbackInvoked) == false + expect(callbackInvoked) == true } - func testHandlesTransactionFromPurchaseIndependentlyFromQueueUpdateForSameProductIdentifier() throws { + func testHandlesTransactionFromPurchaseAfterReviewingQueueUpdateForSameProductIdentifier() throws { + // This documents a race condition that we can't detect in the implementation + // where `PurchasesOrchestrator` can't tell the difference between `StoreKit 1` sending + // us a transaction from the queue, and sending us a transaction as a result of adding an `SKPayment`. + let product = StoreProduct(sk1Product: MockSK1Product(mockProductIdentifier: "com.product.id1")) let mockPayment = MockPayment() @@ -135,7 +143,7 @@ class PurchasesPurchasingTests: BasePurchasesTests { self.storeKit1Wrapper.delegate?.storeKit1Wrapper(self.storeKit1Wrapper, updatedTransaction: queueTransaction) expect(self.backend.postReceiptDataCalled) == true - expect(self.backend.postedInitiationSource) == .queue + expect(self.backend.postedInitiationSource) == .purchase expect(callbackInvoked) == false let purchaseTransaction = MockTransaction()