`PurchasesOrchestrator`: replaced calls to `syncPurchases` with posting receipt for an individual product during SK2 purchases by NachoSoto · Pull Request #1666 · RevenueCat/purchases-ios

NachoSoto · 2022-06-07T18:19:57Z

Fixes SDKONCALL-19.
SK2 purchases were always calling syncPurchases, which called Backend.post(receiptData:...) with no productData, which meant that after every SK2 purchase we never posted the price.

This refactor makes a lot of methods in PurchasesOrchestrator use StoreTransaction so they can be used during SK2 purchases as well. Specifically, handlePurchasedTransaction is now called after both SK1 and SK2 purchases.

Other changes:

StoreKit2TransactionListenerDelegate no longer needs to return CustomerInfo
StoreKit2TransactionListenerDelegate.handle(purchaseResult:) no longer notifies the delegate (only done by StoreKit.Transaction.updates), which was leading to duplicate syncPurchases calls.
If the purchase is cancelled, syncPurchases isn't called. Instead, the CustomerInfo is fetched from CustomerInfoManager

NachoSoto · 2022-06-07T18:22:34Z

This was duplicated below, so it's now in a shared local function above.

NachoSoto · 2022-06-07T19:31:22Z

Unit tests pass, but looking into integration tests now (glad to have those since this is a critical change).

NachoSoto · 2022-06-07T19:32:36Z

This simple test fails 🎉 looking into why

func testCanMakePurchase() async throws {
  try await self.purchaseMonthlyOffering()
}

NachoSoto · 2022-06-07T19:36:35Z

NachoSoto · 2022-06-07T20:47:28Z

Integration tests fixed :) This is ready

NachoSoto · 2022-06-07T20:48:38Z

This is an async wrapper on top of self.handlePurchasedTransaction.

NachoSoto · 2022-06-07T20:49:20Z

This avoids calling the delegate during purchases (leading to a duplicate syncPurchases call). Now this is only called for transactions that come from StoreKit.Transaction.updates

taquitos

Nice fix, it's slightly confusing to follow but I think that's to be expected given we're dealing with SK1/SK2, and whether or not we're calling the delegate/syncing/posting receipts. I guess this is why people use RevenueCat 😄

🎈🐐

taquitos · 2022-06-07T20:56:21Z

NachoSoto · 2022-06-07T21:23:40Z

Heads up this depends on #1664 and #1665.

NachoSoto · 2022-06-08T01:43:55Z

I wanna add a new unit test to make sure we always pass product data

aboedo · 2022-06-08T13:47:37Z

just to clarify, in the PR description it says

SK2 purchases were always calling syncPurchases, which called Backend.post(receiptData:...) with no productData, which meant that after every SK2 purchase we never posted the price.

But my understanding is that we were sending two posts, right? one with and one without the product details?

aboedo

still reviewing but I left a few comments

aboedo · 2022-06-08T13:59:36Z

when does this else case happen? i.e.: when do we not have a transaction?

Yeah this is kind of hard to follow because we convert from Product.PurchaseResult (an enum with 3 cases) to a tuple of Optionals. There would be no transaction in the userCancelled or pending cases. This is also related to CSDK-113, it'll change with that fix.

sounds good. I think this is another case where we should add some comments since it'll be confusing to folks without context

Yup, added.

aboedo · 2022-06-08T14:00:07Z

don't we already know whether this is a cancellation from the lines before this?

The line before catches StoreKitError.userCancelled. This comes from Product.PurchaseResult.userCancelled. It's unclear when StoreKit would throw one versus returning the other, but we need to handle both.

could we write that up as a comment here? Otherwise it won't be apparent to new devs looking at this code

aboedo · 2022-06-08T14:02:02Z

🤔 why do we have onlyIfEmpty in this case? If we're calling this method, we know that there was a purchase, right? I feel like we should always refresh in that case, there's a good chance that the receipt isn't up to date otherwise

I added that default value to keep the existing behavior to the previous callers.
Looks like there is only one place: storeKitWrapper(:updatedTransaction:). I suppose it makes sense for that one to use .always as well?

I think so - if anything is coming in from StoreKit it should be safe to assume that there's an apple ID logged into the device

I'll do this in a separate PR if that's ok to minimize the changes in behavior / tests in this PR, so if something breaks it's easier to revert one versus the other.

Filed [CSDK-120]

awesome thanks!

Done: #1703

aboedo · 2022-06-08T14:19:21Z

If the productIdentifier is empty, it means we got a bad SKTransaction.
Current behavior is to just not send it to the backend.

But...
At this point, the user got charged. They won't have access, and they'll get an error. That's a terrible experience. Sure, it's because of a bug in StoreKit, but it's a bug in the price, and the only value of fetching the product here is to send the prices to our backend. So the user gets a bad experience but we're not preventing any issues on their side, this is just for accounting purposes.

The user should come first. If they made a purchase, and it went through, we should prioritize granting them access. Getting the products comes second.

This is illustrated in the current flow by the fact that we don't check whether we actually could fetch the product - there's no guarantee that we will (the request could fail) but we don't care, we just send the receipt.

We should do the same for the case where the SKTransaction is broken. The receipt should still be fine, so even if the transaction is broken, we can send the transaction to the backend, and get the customerInfo, and preserve user experience, in exchange for slightly worse analytics when SKTransactions are broken.

It's not like the customer would be able to fix this by retrying either, they'll already have purchased the product

So I'd suggest to:

Log when the transaction is bad, that'll help us debug issues with prices

Always refresh and send the receipt, even if the transaction was bad

For context, this PR doesn't change the existing behavior. The reason I changed this to notEmpty is because of this (StoreTransactionType.productIdentifier is not Optional) whereas previously the nil check did that.

Log when the transaction is bad, that'll help us debug issues with prices

That's always done by SKPaymentTransaction.productIdentifier.

Always refresh and send the receipt, even if the transaction was bad

I think that makes sense. So to make sure I understand, basically change the else to:

self.postReceipt(withTransaction: transaction, receiptData: receiptData, products: [], // No product data, but still post the receipt storefront: storefront)

I can add unit tests for this too.

Kinda scary that this doesn't make any test fail:

Huh I see, there's just no test coverage for it.
It is an edge case after all, but I'll add a test.

Turns out this breaks during tests, because we rely on product identifiers for the callbacks keys...

I'm gonna leave this for a separate PR to deal with this problem.

Filed [CSDK-119]

I also added a failing test: #1680

thanks for doing that! 💪

aboedo · 2022-06-08T14:22:46Z

the RTTI here feels a little weird... why are we special casing SK1? SK2 transactions have a finish method too

Yeah I agree this is messy. The reason is SK2 transactions are finished by StoreKit2TransactionListener, so I wanted to keep the existing behavior here. Even though this method uses any StoreTransaction, I didn't want to call that twice on SK2 transactions :/

But looking at this now, maybe we can remove the finish call in StoreKit2TransactionListener and call it here for consistency?

I think that's probably a good call, although it doesn't need to be a part of this PR

Okay let me make a Jira for it then.

https://revenuecats.atlassian.net/browse/CSDK-122

This is ready for a final look then :)

NachoSoto · 2022-06-08T16:19:18Z

Thanks for the amazingly thorough review @aboedo! This is a change in a very critical part of the code, so I really appreciate the close look.

NachoSoto · 2022-06-08T16:24:07Z

But my understanding is that we were sending two posts, right? one with and one without the product details?

@aboedo correct, we were posting receipts twice. But no, none of them contained the product details, because in both cases they came from calls to syncPurchases, which never passes productData.

…ng receipt for an individual product during SK2 purchases Fixes [SDKONCALL-19]. SK2 purchases were always calling `syncPurchases`, which called `Backend.post(receiptData:...)` with no `productData`, which meant that after every SK2 purchase we never posted the price. This refactor makes a lot of methods in `PurchasesOrchestrator` use `StoreTransaction` so they can be used during SK2 purchases as well. Specifically, `handlePurchasedTransaction` is now called after both SK1 and SK2 purchases. - `StoreKit2TransactionListenerDelegate` no longer needs to return `CustomerInfo` - `StoreKit2TransactionListenerDelegate.handle(purchaseResult:)` no longer notifies the delegate (only done by `StoreKit.Transaction.updates`), which was leading to duplicate `syncPurchases` calls. - If the purchase is cancelled, `syncPurchases` isn't called. Instead, the `CustomerInfo` is fetched from `CustomerInfoManager`

…r purchasing

aboedo

looks great! I left a couple of minor comments

aboedo · 2022-06-09T19:50:44Z

could we write that up as a comment here? Otherwise it won't be apparent to new devs looking at this code

aboedo · 2022-06-09T19:52:28Z

sounds good. I think this is another case where we should add some comments since it'll be confusing to folks without context

aboedo · 2022-06-09T19:52:51Z

awesome thanks!

aboedo · 2022-06-09T19:55:06Z

thanks for doing that! 💪

…Data`

NachoSoto · 2022-06-09T21:05:46Z

        _ = try await orchestrator.purchase(sk2Product: product, promotionalOffer: nil)

        expect(self.backend.invokedPostReceiptDataCount) == 1
+        expect(self.backend.invokedPostReceiptDataParameters?.productData).toNot(beNil())


I added these checks in several of the tests too.

@beylmk

_This release is compatible with Xcode 14 beta 1_ ### Bug fixes * `EntitlementInfo.isActive` returns true if `requestDate == expirationDate` (#1684) via beylmk (@beylmk) * Fixed usages of `seealso` (#1689) via NachoSoto (@NachoSoto) * Fixed `ROT13.string` thread-safety (#1686) via NachoSoto (@NachoSoto) * `PurchasesOrchestrator`: replaced calls to `syncPurchases` with posting receipt for an individual product during SK2 purchases (#1666) via NachoSoto (@NachoSoto)

…ceipt data Fixes [CSDK-120]. See conversation: #1666 (comment) > why do we have `onlyIfEmpty` in this case? If we're calling this method, we know that there was a purchase, right? I feel like we should always refresh in that case, there's a good chance that the receipt isn't up to date otherwise

…ceipt data (#1703) Fixes [CSDK-120]. See conversation: #1666 (comment) > why do we have `onlyIfEmpty` in this case? If we're calling this method, we know that there was a purchase, right? I feel like we should always refresh in that case, there's a good chance that the receipt isn't up to date otherwise

NachoSoto requested a review from a team June 7, 2022 18:19

NachoSoto commented Jun 7, 2022

View reviewed changes

NachoSoto force-pushed the purchases-orchestrator-error-cancelled branch 2 times, most recently from 59f6c28 to 7facf7d Compare June 7, 2022 19:21

NachoSoto force-pushed the sk2-purchase-post-receipt branch from 02270d6 to 45366d4 Compare June 7, 2022 19:23

NachoSoto mentioned this pull request Jun 7, 2022

Fixed Xcode 14 compilation: avoid @available properties #1661

Merged

NachoSoto commented Jun 7, 2022

View reviewed changes

Comment thread Tests/StoreKitUnitTests/PurchasesOrchestratorTests.swift Outdated

NachoSoto commented Jun 7, 2022

View reviewed changes

Comment thread Tests/UnitTests/Purchasing/Purchases/PurchasesPurchasingTests.swift Outdated

taquitos approved these changes Jun 7, 2022

View reviewed changes

NachoSoto requested a review from aboedo June 7, 2022 22:00

Base automatically changed from purchases-orchestrator-error-cancelled to main June 7, 2022 22:10

NachoSoto force-pushed the sk2-purchase-post-receipt branch from 741347d to 4882022 Compare June 7, 2022 22:28

aboedo requested changes Jun 8, 2022

View reviewed changes

NachoSoto added 5 commits June 9, 2022 11:25

Fixed tests

f557a1d

Using the correct Storefront for SK2

a589b86

Fixed integration tests by keeping ReceiptRefreshPolicy.always afte…

3982689

…r purchasing

Added comment

0b209d2

NachoSoto force-pushed the sk2-purchase-post-receipt branch from 4882022 to 0b209d2 Compare June 9, 2022 18:26

Using shared method

b90257d

NachoSoto requested a review from aboedo June 9, 2022 19:47

aboedo approved these changes Jun 9, 2022

View reviewed changes

NachoSoto added 2 commits June 9, 2022 14:05

Added a couple of comments and renamed method for clarity

02b3659

PurchasesOrchestratorTests: added a few more expects for `product…

5f5a7ea

…Data`

NachoSoto commented Jun 9, 2022

View reviewed changes

NachoSoto merged commit a726fa6 into main Jun 9, 2022

NachoSoto deleted the sk2-purchase-post-receipt branch June 9, 2022 21:06

NachoSoto mentioned this pull request Jun 10, 2022

Release/4.6.1 #1690

Merged

This was referenced Jun 14, 2022

PurchasesOrchestrator.handlePurchasedTransaction: always refresh receipt data #1703

Merged

PurchasesOrchestrator: unify finish transactions between SK1 and SK2 #1704

Merged

Uh oh!

Conversation

NachoSoto commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Other changes:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto commented Jun 7, 2022

Uh oh!

NachoSoto commented Jun 7, 2022

Uh oh!

NachoSoto commented Jun 7, 2022

Uh oh!

NachoSoto commented Jun 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

taquitos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NachoSoto commented Jun 7, 2022

Uh oh!

NachoSoto commented Jun 8, 2022

Uh oh!

aboedo commented Jun 8, 2022

Uh oh!

aboedo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NachoSoto commented Jun 7, 2022 •

edited

Loading

taquitos left a comment •

edited

Loading

aboedo left a comment •

edited

Loading