-
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
[CustomerCenter] Add action handler #4057
[CustomerCenter] Add action handler #4057
Conversation
self.setAlertType(.purchasesNotFound) | ||
} | ||
let alertType = await self.customerCenterViewModel.performRestore() | ||
self.setAlertType(alertType) |
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 tried to move all this logic to the VM since I think it makes much more sense for it to be there. We could potentially create a different view model, but it's easier to pass the CustomerCenterViewModel
around... Lmk what you think!
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.
fantastic
public protocol CustomerCenterActionHandler { | ||
@objc optional func purchaseCompleted(_ customerInfo: CustomerInfo) | ||
@objc optional func restoreStarted() | ||
@objc optional func restoreCompleted(_ customerInfo: CustomerInfo) |
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 also thought about having a single method here and a enum for the different events, but this seems simpler and less prone to breaking changes IMO. Lmk what you think!
a910b59
to
6cd489f
Compare
d989594
to
0c045ee
Compare
import RevenueCat | ||
|
||
/// Allows to be notified of certain events the customer may perform during the Customer Center flow | ||
public protocol CustomerCenterActionHandler { |
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've been wondering whether to rename this to CustomerCenterActionDelegate
instead, since it might be more common in iOS... Wdyt?
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.
Yes, I think that should be a Delegate
. But maybe @aboedo can seal the deal since he has more iOS experience
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.
Done in da03b7c
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.
Hmm actually swiftlint is complaining with the new name: warning: Class Delegate Protocol Violation: Delegate protocols should be class-only so they can be weakly referenced (class_delegate_protocol)
. I guess this means we should mark it as @objc
which would make it so we can't implement it on structs like this... I'm wondering whether to just revert the rename or follow the convention specified by swiftlint...
@@ -31,6 +31,8 @@ struct RestorePurchasesAlert: ViewModifier { | |||
@Environment(\.openURL) | |||
var openURL | |||
|
|||
@EnvironmentObject private var customerCenterViewModel: CustomerCenterViewModel |
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.
Made this an environment object for ease of access.
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.
ohhh nice
RevenueCatUI/CustomerCenter/Data/CustomerCenterActionHandler.swift
Outdated
Show resolved
Hide resolved
func performRestore() async -> RestorePurchasesAlert.AlertType { | ||
self.customerCenterActionHandler?.onRestoreStarted() | ||
do { | ||
let customerInfo = try await Purchases.shared.restorePurchases() |
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.
In order to be able to mock this easily when we start writing more tests I think we can have a purchasesProvider
like the one we have in ManageSubscriptionsViewModel
that has the functions that are being used in this view model. Something like:
protocol CustomerCenterPurchasesType: Sendable {
@Sendable
func customerInfo() async throws -> CustomerInfo
@Sendable
func restorePurchases() async throws -> CustomerInfo
}
If we do that, we can remove the private var customerInfoFetcher: CustomerInfoFetcher
in this class and mock the return of customerInfo()
using a MockCustomerCenterPurchasesType
By the way the PurchasesType
comes from a PaywallPurchasesType
in Paywalls
. That's where I got it from.
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.
Talked about this and we are going to unify the different abstractions into a single one for CustomerCenter, since we will need a few, and it was getting complex. However, will do this on a separate PR.
@@ -49,27 +49,34 @@ class ManageSubscriptionsViewModel: ObservableObject { | |||
@Published | |||
private(set) var refundRequestStatusMessage: String? | |||
|
|||
private var purchasesProvider: ManageSubscriptionsPurchaseType | |||
private let purchasesProvider: ManageSubscriptionsPurchaseType |
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.
Oopsies, I just realized this should be ManageSubscriptionsPurchasesType
. I can fix it in another PR
import RevenueCat | ||
|
||
/// Allows to be notified of certain events the customer may perform during the Customer Center flow | ||
public protocol CustomerCenterActionHandler { |
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.
Yes, I think that should be a Delegate
. But maybe @aboedo can seal the deal since he has more iOS experience
@@ -31,6 +31,8 @@ struct RestorePurchasesAlert: ViewModifier { | |||
@Environment(\.openURL) | |||
var openURL | |||
|
|||
@EnvironmentObject private var customerCenterViewModel: CustomerCenterViewModel |
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.
ohhh nice
self.setAlertType(.purchasesNotFound) | ||
} | ||
let alertType = await self.customerCenterViewModel.performRestore() | ||
self.setAlertType(alertType) |
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.
fantastic
Changed to a new approach where there is a single lambda parameter for the |
e38d435
to
8cb5067
Compare
523fc2d
to
34e4536
Compare
2a9cd2c
into
integration/customer_support_workflow
Description
This PR provides a possible approach to implementing an "action" handler. This allows developer to respond to events that happen during the customer support flow.
The current approach consists of making
CustomerCenterActionHandler
, a lambda that receives an action that can be passed in by the developer. Then calling that with the appropriate action from the customer center. This PR also moves some code to the view model for simplicity and moving logic away from the view layer.