-
Notifications
You must be signed in to change notification settings - Fork 328
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
Paywalls manually handle purchases, finishTransactions/ObserverMode -> PurchasesAreCompletedBy #3917
Changes from all commits
2538c2a
4b3d0e7
d914925
1e3b3f8
472a177
fb29c58
6a383dc
839d83a
1ee8110
18a6db3
a5214c8
f642ffa
95cafb7
52faa87
3a282e8
e5e0e07
2244b8e
094722c
1b8390a
3cdc95f
8532e33
b7adeb0
9d7099e
c205d5f
45b4425
534cc83
122d2c2
5c4809e
d816856
7047521
5a0718d
57ec3ab
f526e83
2d2f2ee
df3e1d9
6de881e
262daf6
46c84e0
5bd6065
2f59d3d
5ef84fa
4a02670
be00392
c51d92e
9ed38e3
436ae86
6d0ad58
edccb80
e411c2e
02c266d
fab7581
b4ace48
0899a38
63942c1
dd5d32f
539062a
fa14695
9763b72
ac65d2f
be0b23b
0bfa9db
b43f77e
7da678d
5aa3730
604ea4f
ce8b8e0
40c0685
bd7bbf3
f30bd1f
ef9526f
1df2679
256a450
37b3cb9
f12f20e
6911c38
faf226b
07ef349
b2fc2e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -40,6 +40,11 @@ enum Strings { | |||||||||||
case restore_purchases_with_empty_result | ||||||||||||
case setting_restored_customer_info | ||||||||||||
|
||||||||||||
case executing_purchase_logic | ||||||||||||
case executing_external_purchase_logic | ||||||||||||
case executing_restore_logic | ||||||||||||
case executing_external_restore_logic | ||||||||||||
|
||||||||||||
} | ||||||||||||
|
||||||||||||
@available(iOS 13.0, macOS 10.15, tvOS 13.0, watchOS 6.0, *) | ||||||||||||
|
@@ -98,6 +103,22 @@ extension Strings: CustomStringConvertible { | |||||||||||
|
||||||||||||
case .setting_restored_customer_info: | ||||||||||||
return "Setting restored customer info" | ||||||||||||
|
||||||||||||
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. " + | ||||||||||||
"You must use `.handlePurchaseAndRestore` on your `PaywallView`." | ||||||||||||
|
||||||||||||
case .executing_purchase_logic: | ||||||||||||
return "Will execute purchase logic provided by RevenueCat." | ||||||||||||
|
||||||||||||
case .executing_restore_logic: | ||||||||||||
return "Will execute restore purchases logic provided by RevenueCat." | ||||||||||||
|
||||||||||||
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. " + | ||||||||||||
Comment on lines
+118
to
+120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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..." ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ohh I like that |
||||||||||||
"You must use `.handlePurchaseAndRestore` on your `PaywallView`." | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about something like
There was a problem hiding this comment.
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:
sounds the same to me as:
The only real change is that we won't finish transactions, but when
purchasesAreCompletedBy
is.myApp
, we don't call intoPurchasesOrchestrator.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.
There was a problem hiding this comment.
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