Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Paywalls manually handle purchases, finishTransactions/ObserverMode -> PurchasesAreCompletedBy #3917

Closed
wants to merge 78 commits into from

Conversation

joshdholtz
Copy link
Member

@joshdholtz joshdholtz commented May 24, 2024

Motivation

  • Allow developers who have their own purchase logic with StoreKit to use our paywalls.
  • Adds a new API purchasesAreCompletedBy: PurchasesAreCompletedBy to Purchases, which replaces finishTransactions, and observerMode which have been marked as deprecated (public APIs only).
  • Internal references to finishTransactions and observerMode will be done as a separate PR.

Todo

  • Update any doc on new method to make sure its SUPER CLEAR on what its doing and that it will only work if finishTransactions (trying to not call it observer mode) is disabled
  • Maybe add some debug logs so developer knows which path paywalls purchase buttons are taking if finish transaction is disabled
  • Fix broken unit/integration tests
  • Add new tests
  • Add new API tests
  • Also add a similar method for handleRestore()

Description

Adds new . handlePurchaseAndRestore modifier that can be used to complete purchases initiated via a paywall:

.handlePurchaseAndRestore(
    performPurchase: { storeProduct, purchaseResultReporter in
        // make purchase for `storeProduct`
        
        // report result to RevenueCat
        purchaseResultReporter.reportResult(userCancelled: false, error: nil)
    }, performRestore: { restoreResultReporter in
        // restore purchases

        // report result to RevenueCat
        restoreResultReporter.reportResult(success: true, error: nil)
    })

@joshdholtz joshdholtz added the pr:feat A new feature label May 24, 2024
@RevenueCat-Danger-Bot
Copy link

RevenueCat-Danger-Bot commented May 24, 2024

1 Message
📖 Size increase: 22.06 KB

Generated by 🚫 Danger

content
.onPreferenceChange(HandleRestorePreferenceKey.self) { restoreResultReporter in
if let restoreResultReporter {
self.handler(restoreResultReporter)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The restore method, which is called here, does not provide any information, just a callback which the user can report if it succeeded or not, plus an error. An implementation looks like this:

}, performRestore: { restoreResultReporter in
    // restore purchases

    // report result to RevenueCat
    restoreResultReporter.reportResult(success: true, error: nil)
})

I'm unsure if this is correct, and if we are able to or need to create a correct "CustomerInfo" object after this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine! But @aboedo would know for sure 😊

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit lost on the context, but ideally restore gives you the latest customer info when it's done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's the app developer performing the restore operation (because purchasesAreCompletedBy == .myApp), but all they give us back is if the restore succeeded or not (and any associated error). Is that sufficient?

Copy link
Contributor

@jamesrb1 jamesrb1 Jun 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the code block in my first comment that is the focus here (but this doesn't appear in the PR because that code would be customer code, so I commented on the code that triggers it instead, which is probably confusing!).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh I see, now I understand the initial question, then. Yes, we'd be able to create a CustomerInfo - assuming the SDK is configured (which I'm guessing it is, otherwise it would crash 😅). We would have to internally call syncPurchases however if we want CustomerInfo to reflect stuff that the user might have done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah OK then I will need to add that! And we should do the same after a purchase then too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be only for restores - a purchase should still trigger activity in the SK1 queue, which we'll observe, and in SK2 we check purchases after the app comes back into foreground, which should also work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restores are a bit of a special case because nothing actually happens in the queue - when you restore using RC, we just fetch the receipt and send it to the backend

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thank you!! 🙏

Copy link
Member Author

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks sooooooo good! Just a few nits and then this will be shipshape 🚢

RevenueCatUI/Data/Strings.swift Outdated Show resolved Hide resolved
RevenueCatUI/Data/Strings.swift Outdated Show resolved Hide resolved
RevenueCatUI/Purchasing/PurchaseHandler.swift Outdated Show resolved Hide resolved
RevenueCatUI/Purchasing/PurchaseHandler.swift Outdated Show resolved Hide resolved
RevenueCatUI/Purchasing/PurchaseHandler.swift Show resolved Hide resolved
RevenueCatUI/Purchasing/PurchaseHandler.swift Outdated Show resolved Hide resolved
RevenueCatUI/Purchasing/PurchaseHandler.swift Outdated Show resolved Hide resolved
RevenueCatUI/Purchasing/PurchaseHandler.swift Outdated Show resolved Hide resolved
@jamesrb1 jamesrb1 marked this pull request as ready for review June 7, 2024 00:33
@jamesrb1 jamesrb1 requested review from a team June 7, 2024 00:35
@jamesrb1 jamesrb1 changed the title [WIP] Paywalls manually handle purchases Paywalls manually handle purchases, finishTransactions/ObserverMode -> PurchasesAreCompletedBy Jun 7, 2024
Copy link
Member Author

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking real good! A few really small nits but this is look ship shape 💪 🚢

RevenueCatUI/Purchasing/PurchaseHandler.swift Outdated Show resolved Hide resolved
RevenueCatUI/Purchasing/PurchaseHandler.swift Outdated Show resolved Hide resolved
RevenueCatUI/View+PurchaseRestoreCompleted.swift Outdated Show resolved Hide resolved
content
.onPreferenceChange(HandleRestorePreferenceKey.self) { restoreResultReporter in
if let restoreResultReporter {
self.handler(restoreResultReporter)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine! But @aboedo would know for sure 😊

@@ -143,12 +151,26 @@ import Foundation
* RevenueCat's backend. Default is `false`.
*
* - Warning: This assumes your IAP implementation uses StoreKit 1.
* Observer mode is not compatible with StoreKit 2.
* `.myApp` is not compatible with StoreKit 2.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will end up changing when this gets rebased by 5.0-dev branch, right? 🤔 cc: @fire-at-will @MarkVillacampa

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep we'll change this when it gets rebased.

You can keep it, that way we'll surely notice when we get a conflict 😄


case .executing_external_purchase_logic:
return "Will execute custom StoreKit purchase logic provided by your app. " +
"No StoreKit purchasing logic will be performed by RevenueCat. " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a bit misleading to me - a lot of StoreKit purchasing logic will be performed by RevenueCat regardless. We still fetch products, post receipts, etc, right?
The only real change is that we won't finish transactions. I realize that that might not be a concept that a user grasps all that well, but then again, we're assuming that this developer has a full StoreKit implementation of their own, so it feels reasonable that they could figure out what finishing transactions is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like

Initiating a purchase for product <product_id>. Note that the StoreKit purchasing logic will be provided by your app and not RevenueCat, since the SDK was configured to blah blah blah.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my confusion here is regarding exactly what StoreKit logic is outsourced to our customer's app.

This:

Will execute custom StoreKit purchase logic provided by your app. No StoreKit purchasing logic will be performed by RevenueCat.

sounds the same to me as:

the StoreKit purchasing logic will be provided by your app and not RevenueCat

The only real change is that we won't finish transactions, but when purchasesAreCompletedBy is .myApp, we don't call into PurchasesOrchestrator.purchase(product:package:completion:) - is this part of finishing transactions?

I think what matters most is that someone who has written or is going to write their own StoreKit interfacing code understands exactly what they are now responsible for. Should we list the StoreKit API calls we no longer call? I'm a bit concerned that unless we're crystal clear, there may be confusion on the customer's side too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was my specific concern as well - we need to make it clear that you need to finish transactions.
That's really the only important part.

Finishing transactions happens specifically when we detect a purchase, then post it to the backend, and receive a response from the backend.

So even if you don't go through the purchase method, we can still detect the purchase, and that's when we finish transactions (unless this mode is enabled).

It'd be finishTransaction in SK1, transaction.finish() in SK2

Comment on lines +118 to +120
case .executing_external_restore_logic:
return "Will execute custom StoreKit restore purchases logic provided by your app. " +
"No StoreKit restore purchases logic will be performed by RevenueCat. " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will execute custom StoreKit restore purchases logic provided by your app. No StoreKit restore purchases logic will be performed by RevenueCat.

Doesn't quite roll off the tongue, right? It took me a couple of reads to understand this. Maybe we can simplify it a bit?

how about something like:

Suggested change
case .executing_external_restore_logic:
return "Will execute custom StoreKit restore purchases logic provided by your app. " +
"No StoreKit restore purchases logic will be performed by RevenueCat. " +
case .executing_external_restore_logic:
return "Initiating a restore. Note that the restore logic will be provided by your app and not RevenueCat, since the SDK was configured to blah blah blah...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel the word "Initiating" could mean that it is doing actual restore work. What do you think of "Requesting"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But even "Requesting" - requesting to whom? It needs to be clear it's of the app (not StoreKit). 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The user has requested a restore. Note that the restore logic will be provided by your app and not RevenueCat, since the SDK was configured to blah blah blah..." ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh I like that

Comment on lines +231 to +237
func restorePurchases() async throws -> (info: CustomerInfo, success: Bool) {
switch self.purchases.purchasesAreCompletedBy {
case .revenueCat:
return try await performRestorePurchases()
case .myApp:
return try await performExternalRestoreLogic()
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious about how this works - is the SDK still getting configured in this scenario, and observing queue / delegates?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When purchasesAreCompletedBy equals .revenueCat, there is no change in behavior. When purchasesAreCompletedBy equals .myApp, all behavior is as it was when observerMode was set to true (so I assume this means the answer to your questions about configuration/queues/delegates is "yes"). The change here, which is an addition, is that when someone taps restore on one of our paywalls, the app developer will get a callback via a view modifier that allows them to know this happened and execute their own restore code.

Does that answer everything?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. What happens if I set it up as "purchases completed by my app" but I don't set up the view modifiers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll see a log message in the console that an external purchase/restore has started (we are discussing the exact wording needed in this PR), but if you haven't set up the view modifiers, there will be no error. I tried to think of a good way of detecting this but couldn't, short of setting up a hacky singleton. Discussed somewhere briefly with Josh, did not come to any solution for this. I'll look at this again, it would be a lot better if it actually could identify the bad state....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly it feels like the way that this is usually solved is that instead of "purchases completed by my app", you provide a delegate.
If delegate exists, we call it. If not, we use our own logic. That way you can't possibly set it up wrong, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a huddle about this with Andy, we are considering moving from view modifiers to a new initializer, to be continued next week. I'm going to separate out the API change so that these discussions don't hold up other projects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense to me! 💪

content
.onPreferenceChange(HandleRestorePreferenceKey.self) { restoreResultReporter in
if let restoreResultReporter {
self.handler(restoreResultReporter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh I see, now I understand the initial question, then. Yes, we'd be able to create a CustomerInfo - assuming the SDK is configured (which I'm guessing it is, otherwise it would crash 😅). We would have to internally call syncPurchases however if we want CustomerInfo to reflect stuff that the user might have done

@joshdholtz
Copy link
Member Author

joshdholtz commented Jul 18, 2024

Closing since implement since #3973

@joshdholtz joshdholtz closed this Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:feat A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants