-
Notifications
You must be signed in to change notification settings - Fork 86
Break VCs Out of CustomerInfo: API & Caching #2466
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
Break VCs Out of CustomerInfo: API & Caching #2466
Conversation
@Synchronized | ||
fun cacheVirtualCurrencies(appUserID: String, virtualCurrencies: VirtualCurrencies) { | ||
val virtualCurrenciesData = JSONObject().apply { | ||
put("virtual_currencies", virtualCurrencies.rawData) |
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.
Here, we wrap the virtual currencies in a JSON object, keyed by virtual_currencies
to mimic the response from the backend so that we can reuse the decoding function used in the network layer
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…/PurchasesAPI.kt Co-authored-by: Antonio Pallares <ajpallares@users.noreply.github.com>
…es.kt Co-authored-by: Antonio Pallares <ajpallares@users.noreply.github.com>
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! Only real comment is to change the VirtualCurrencyManager
to be a dependency of the PurchasesOrchestrator
instead of Purchases
.
@@ -46,6 +49,7 @@ import java.net.URL | |||
*/ | |||
class Purchases internal constructor( | |||
@get:JvmSynthetic internal val purchasesOrchestrator: PurchasesOrchestrator, | |||
@get:JvmSynthetic internal val virtualCurrencyManager: VirtualCurrencyManager, |
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 could we move this dependency to the PurchasesOrchestrator
? I would consider Purchases
just a facade, without any real logic, that just calls PurchasesOrchestrator
as needed. Then, in CEC we just don't add the API methods calling PurchasesOrchestrator
.
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 works for me - I'll also create a PR to do this in the iOS SDK as well so the architectures are kept in sync with each other.
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 on iOS we've been a little less thorough in keeping everything inside PurchasesOrchestrator, and we do have some logic in the Purchases 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.
Moved VirtualCurrencyManager
from Purchases
to PurchasesOrchestrator
here! 5db3c4d
/** | ||
* The currently cached [VirtualCurrencies] if one is available. | ||
* This is synchronous, and therefore useful for contexts where an app needs a [VirtualCurrencies] | ||
* right away without waiting for a callback. | ||
* | ||
* This allows initializing state to ensure that UI can be loaded from the very first frame. | ||
*/ |
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.
Would be worth adding a note that this will remain empty until virtual currencies have been fetched at least once using getVirtualCurrencies
?
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.
Updated in b23325e!
val cachedJSONObject = JSONObject(json) | ||
return VirtualCurrenciesFactory.buildVirtualCurrencies(body = cachedJSONObject) | ||
} catch (e: JSONException) { | ||
null |
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 would it be worth adding a log here? (not sure in what situations this may happen.
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! I'll add in all of logs together in a follow-up PR to ensure they're done consistently and to keep this PR's size down.
The JSONException
is thrown when the string you're trying to parse isn't valid JSON, like if you try to parse asdf
. This shouldn't happen, but better safe than sorry 😄
* | ||
* @property all - All of the virtual currencies associated to the user. | ||
*/ | ||
@Poko |
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'm not super familiar with the Poko library but I'm reading it synthesises equals and hashCode methods, but we're manually implementing both here, is it necessary? will they interfere?
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.
Ohh I missed that but you're right! We shouldn't need to write our own. We can write it if we want to overwrite it, so ours would have preference, but no need to write these from what I can read 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.
Yeah, I think we can rely on Poko's, I don't see any reason to write custom ones here. I'll remove them!
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.
Removed them here! 3e1dc50
purchases/src/main/kotlin/com/revenuecat/purchases/virtualcurrencies/VirtualCurrency.kt
Outdated
Show resolved
Hide resolved
callback = callback, | ||
) | ||
|
||
// TODO: Cache the VCs from the network |
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 know the fetch from network method is not implemented, but it seems like we could already call cacheVirtualCurrencies
here? is there something else missing?
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.
Added this here: 9ac9904
import org.junit.runner.RunWith | ||
|
||
@RunWith(AndroidJUnit4::class) | ||
class VirtualCurrenciesTest { |
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.
Maybe i missed them but could we add some equality tests for the newly added Parcelable classes?
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.
Added them here! f138374
📸 Snapshot Test642 unchanged
🛸 Powered by Emerge Tools |
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 changes look good! We should make sure we don't merge this to main until the network requests have been implemented and the TODO's addressed. But I think this can be merged to the base branch as is
ae8bc10
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 ```
Description
This PR contains the first major part of breaking virtual currencies out of CustomerInfo in purchases-android. Specifically, it creates the new public-facing APIs and implements the caching layer.
Changes Introduced
VirtualCurrencyInfo
is renamedVirtualCurrency
VirtualCurrencies
objectVirtualCurrencyManager
cachedVirtualCurrencies
computed propertygetVirtualCurrencies()
Testing Done
VirtualCurrencyManager
Next Steps
In a follow up PR, we will implement fetching virtual currencies from the network, add more unit tests for the
VirtualCurrencyManager
, and add backend integration tests.To keep the
virtual-currency-dev
branch clean, this PR will be merged into afetch-virtual-currencies-new-endpoint
branch, which is a clone of thevirtual-currency-dev
branch. We'll make future PRs go into thefetch-virtual-currencies-new-endpoint
branch, and when the functionality of the new APIs is complete, we'll mergefetch-virtual-currencies-new-endpoint
intovirtual-currency-dev
.