[Diagnostics] Add apple_purchase_attempt event
#4253
Conversation
vegaro
force-pushed
the
track-purchases
branch
from
abb80bc
to
b96a7a3
Compare
vegaro
changed the title
apple_products_attempt eventapple_purchase_attempt event
| if #available(iOS 15.4, macOS 12.3, tvOS 15.4, watchOS 8.5, visionOS 1.0, *) { | ||
| return "not_entitled" | ||
| } else { | ||
| return "unknown" |
There was a problem hiding this comment.
Hmm is it possible to get here? Seems weird to branch by version here...
| .storeKitVersion: AnyEncodable("store_kit_\(storeKitVersion.debugDescription)"), | ||
| .errorMessageKey: AnyEncodable(errorMessage), | ||
| .errorCodeKey: AnyEncodable(errorCode), | ||
| .skErrorDescriptionKey: AnyEncodable(storeKitErrorDescription) |
There was a problem hiding this comment.
❤️ I think this could be useful! We probably should add it to the event I added in #4253 if we can.
There was a problem hiding this comment.
yes! I think it will be super useful. I thought about using an int code, but SK2 errors don't have an int representation. So I decided to go with the name of the event. It will make it easier to analyse as well.
vegaro
added
feat:RevenueCatUI
pr:other
and removed
pr:RevenueCatUI
pr:feat
| case .overlayPresentedInBackgroundScene: | ||
| return "overlay_presented_in_background_scene" | ||
| @unknown default: | ||
| return "unknown_future_error" |
There was a problem hiding this comment.
Maybe unknown_store_kit_error?
| return "unknown" | ||
| } | ||
| @unknown default: | ||
| return "unknown_future_error" |
| storeKitErrorDescription = storeKitError.trackingDescription | ||
| } else { | ||
| storeKitErrorDescription = nil | ||
| } |
There was a problem hiding this comment.
I would like this to be a function we can use from other places, but not really sure where to move it to? It would be an extension on Dictionary, which feels weird? Maybe we can just leave it here. cc: @tonidero
There was a problem hiding this comment.
Hmm maybe some kind of StoreKitErrorHelper class to abstract this logic? I think it would be ok even if this is a static method... Wdyt?
I think this will very likely be used in other diagnostics events, so it would be ideal to avoid duplication.
vegaro
force-pushed
the
track-purchases
branch
from
6725240
to
d929e8e
Compare
|
@RCGitBot please test |
1 similar comment
|
@RCGitBot please test |
vegaro
force-pushed
the
track-purchases
branch
from
e330b9b
to
d1c3ddc
Compare
| @@ -413,8 +421,11 @@ final class PurchasesOrchestrator { | |||
|
|
|||
| let addPayment: Bool = self.addPurchaseCompletedCallback( | |||
| productIdentifier: productIdentifier, | |||
| completion: { transaction, customerInfo, error, cancelled in | |||
| completion: { [weak self] transaction, customerInfo, error, cancelled in | |||
| guard let self = self else { return } | |||
There was a problem hiding this comment.
without this, we were leaking PurchasesOrchestator and DeviceCache
cc: @tonidero
There was a problem hiding this comment.
Hmm interesting... I'm not sure I understand the reason... I think this should be mostly safe, except maybe we're not ensuring the completion blocks to be called if changing the purchases orchestrator instance for any reason... Which should normally be ok I think.
In any case, would it be possible to do this inside the trackPurchaseEventIfNeeded if that was the reason for the leak to avoid changing any other code?
There was a problem hiding this comment.
well the reason is that the completion block holds a reference to self, and we are adding the completion block to the purchaseCompletedCallbacks, which I think are not getting triggered when testing, so even if I add it to trackPurchaseEventIfNeeded, we still need to call self. trackPurchaseEventIfNeeded, and the completion would still be holding the reference to self. Am I misunderstanding something?
There was a problem hiding this comment.
Ohh ok right. Make sense. Thanks for the explanation!
|
@RCGitBot please test |
| @@ -413,8 +421,11 @@ final class PurchasesOrchestrator { | |||
|
|
|||
| let addPayment: Bool = self.addPurchaseCompletedCallback( | |||
| productIdentifier: productIdentifier, | |||
| completion: { transaction, customerInfo, error, cancelled in | |||
| completion: { [weak self] transaction, customerInfo, error, cancelled in | |||
| guard let self = self else { return } | |||
There was a problem hiding this comment.
Ohh ok right. Make sense. Thanks for the explanation!
vegaro
force-pushed
the
track-purchases
branch
from
3baa73d
to
abbbbb4
Compare
Create a new
applePurchaseAttemptStill as draft as it is missing tests and backend