-
Notifications
You must be signed in to change notification settings - Fork 85
Fetch VCs from Network #2496
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
Fetch VCs from Network #2496
Conversation
} | ||
|
||
override fun onError(error: PurchasesError) { | ||
synchronized(this@Backend) { |
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.
Nit: consider logging the PurchasesError
in onError()
here for consistency with the explicit logging in the ViewModel and to ensure errors during VC fetch are visible in logs
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.
Hey @harrytmthy, thanks for the review! I've added logging in 8cdde1d. Network errors are now logged in the VirtualCurrencyManager
class 👍
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.
Looking good! Just some comments
@@ -103,9 +106,41 @@ class OverviewViewModel(private val interactionHandler: OverviewInteractionHandl | |||
interactionHandler.syncAttributes() | |||
} | |||
|
|||
fun onFetchVCsClicked() { | |||
viewModelScope.launch { | |||
val virtualCurrencies: VirtualCurrencies = Purchases.sharedInstance.awaitGetVirtualCurrencies() |
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.
Are we planning on using this for something other than logs? Could be good to add a section to visualize the virtual currency to the purchase tester app I 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.
purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt
Outdated
Show resolved
Hide resolved
purchases/src/main/kotlin/com/revenuecat/purchases/virtualcurrencies/VirtualCurrencyManager.kt
Outdated
Show resolved
Hide resolved
every { | ||
mockBackend.getVirtualCurrencies(any(), any(), any(), any()) | ||
} answers { | ||
val onSuccess = arg<(VirtualCurrencies) -> Unit>(2) |
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.
Should we add some tests to also verify the behavior on an error?
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.
Great idea - I've added a test to ensure that the VCManager forwards the PurchasesError from the backend to the callback here: b1b0117 👍
originalCallback.onReceived(virtualCurrencies) | ||
} | ||
override fun onError(error: PurchasesError) { | ||
log(LogIntent.RC_SUCCESS) { |
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 is this logIntent expected on an error?
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.
Definitely not - I should remove the copy/paste functionality from my computer 🤦♂️
Fixed in 1f1b8b2
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.
Looks good to me in general, but I think there's a small bug in the onError callbacks of the get virtual currencies request. I'm requesting changes to make sure we don't miss that before merging.
Great job btw! 🙌
purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt
Outdated
Show resolved
Hide resolved
log(LogIntent.RC_SUCCESS) { | ||
VirtualCurrencyStrings.VIRTUAL_CURRENCIES_UPDATED_FROM_NETWORK_ERROR.format(error) | ||
} | ||
originalCallback.onError(error) |
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.
Not saying we should, but I wonder: should we clear the cache at this point?
FWIW, following the code, this point would only be reached if the cache is stale
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.
No, I think we still want to allow devs to access stale data by using the cachedVirtualCurrencies
synchronous accessor.
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.
Makes sense. Thank you for the clarification!
…end.kt Co-authored-by: Antonio Pallares <ajpallares@users.noreply.github.com>
@tonidero @ajpallares I've applied the fix that @ajpallares suggested - great find! We already had some tests (here) that ensured that the callbacks were triggered when the JSON deserialization fails, but they were passing despite the bug because the function's callback was still being called 😅 I've tried to find a way to ensure that the |
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 looks great to me!
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.
1b54f74
into
fetch-virtual-currencies-new-endpoint
This PR includes the following PRs with no other changes: - #2466 - #2496 - #2505 It is the collection of all of the work required to update the VC APIs and fetch the virtual currencies from the new v1 endpoint. To keep the PR sizes manageable, the work was broken up into smaller PRs, which have been individually approved and aggregated into this branch. Forgoing a review on this PR since all changes have already been reviewed, and we'll do a final review when we merge `virtual-currency-dev` into `main`
### Description Android counterpart to RevenueCat/purchases-ios#5108. This PR introduces virtual currencies to the Android SDK. It's a large PR, but all parts of it have been reviewed individually before here: - #2466 - #2496 - #2505 It includes the following: ### New APIs for Fetching Virtual Currency Balances + Metadata #### New Top-Level Functions These functions allow you to fetch the virtual currencies for the current subscriber. The `invalidateVirtualCurrenciesCache` function allows developers to clear the cached virtual currencies, which will then force the refreshing of the virtual currencies from the backend the next time `getVirtualCurrencies()` is called. ```kotlin class Purchases { fun getVirtualCurrencies( callback: GetVirtualCurrenciesCallback, ) fun invalidateVirtualCurrenciesCache() val cachedVirtualCurrencies: VirtualCurrencies? } ``` #### New Objects ```kotlin class VirtualCurrencies internal constructor( val all: Map<String, VirtualCurrency>, ) : Parcelable { operator fun get(code: String): VirtualCurrency? = all[code] } class VirtualCurrency internal constructor( val balance: Int, val name: String, val code: String, val serverDescription: String? = null, ) ``` #### Example Usage Kotlin: ```swift Purchases.sharedInstance.invalidateVirtualCurrenciesCache() Purchases.sharedInstance.getVirtualCurrencies( object: GetVirtualCurrenciesCallback { override fun onReceived(virtualCurrencies: VirtualCurrencies) {} override fun onError(error: PurchasesError) {} } ) Purchases.sharedInstance.getVirtualCurrenciesWith( onError = { _: PurchasesError -> }, onSuccess = { _: VirtualCurrencies -> }, ) val getVirtualCurrenciesResult: VirtualCurrencies = Purchases.sharedInstance.awaitGetVirtualCurrencies() val cachedVirtualCurrencies = Purchases.sharedInstance.cachedVirtualCurrencies ```
Android counterpart to RevenueCat/purchases-ios#5108. This PR introduces virtual currencies to the Android SDK. It's a large PR, but all parts of it have been reviewed individually before here: - #2466 - #2496 - #2505 It includes the following: These functions allow you to fetch the virtual currencies for the current subscriber. The `invalidateVirtualCurrenciesCache` function allows developers to clear the cached virtual currencies, which will then force the refreshing of the virtual currencies from the backend the next time `getVirtualCurrencies()` is called. ```kotlin class Purchases { fun getVirtualCurrencies( callback: GetVirtualCurrenciesCallback, ) fun invalidateVirtualCurrenciesCache() val cachedVirtualCurrencies: VirtualCurrencies? } ``` ```kotlin class VirtualCurrencies internal constructor( val all: Map<String, VirtualCurrency>, ) : Parcelable { operator fun get(code: String): VirtualCurrency? = all[code] } class VirtualCurrency internal constructor( val balance: Int, val name: String, val code: String, val serverDescription: String? = null, ) ``` Kotlin: ```swift Purchases.sharedInstance.invalidateVirtualCurrenciesCache() Purchases.sharedInstance.getVirtualCurrencies( object: GetVirtualCurrenciesCallback { override fun onReceived(virtualCurrencies: VirtualCurrencies) {} override fun onError(error: PurchasesError) {} } ) Purchases.sharedInstance.getVirtualCurrenciesWith( onError = { _: PurchasesError -> }, onSuccess = { _: VirtualCurrencies -> }, ) val getVirtualCurrenciesResult: VirtualCurrencies = Purchases.sharedInstance.awaitGetVirtualCurrencies() val cachedVirtualCurrencies = Purchases.sharedInstance.cachedVirtualCurrencies ```
Motivation
This PR is a follow-up PR to #2466 that supports fetching virtual currencies from the backend in the
VirtualCurrencyManager.getVirtualCurrencies()
function.Description
This PR:
VirtualCurrencyManager.getVirtualCurrencies()
when the VC cache is empty or staleVirtualCurrenciesFactory
more explicitVirtualCurrencyFactory
class and its testsPurchases.getVirtualCurrencies
functionHere's a screenshot of the updated Purchase Tester screen:

Testing
This PR also adds unit tests for:
VirtualCurrenciesFactory
I've also manually tested the following scenarios through the purchase tester app:
Purchases.shared.getVirtualCurrencies()
fetches VCs from the network when the cache is empty.Purchases.shared.getVirtualCurrencies()
returns cached VCs when non-stale virtual currencies are cachedPurchases.shared.getVirtualCurrencies()
fetches VCs from the backend when the cached VCs are present on the device but are stalePurchases.shared.cachedVirtualCurrencies
returnsnull
when the cache is empty.Purchases.shared.cachedVirtualCurrencies
returns virtual currencies immediately after fetching virtual currencies from the network-
Purchases.shared.cachedVirtualCurrencies
returns cached VCs even if they are stale (useful if the app is offline)Next Steps
In a future PR, we'll add backend integration tests.
To keep the virtual-currency-dev branch clean, this PR will be merged into a fetch-virtual-currencies-new-endpoint branch. We'll make future PRs go into the fetch-virtual-currencies-new-endpoint branch, and when the functionality of the new APIs is complete, we'll merge fetch-virtual-currencies-new-endpoint into virtual-currency-dev.