New API to get customer info with a given `CacheFetchPolicy` by tonidero · Pull Request #546 · RevenueCat/purchases-android

tonidero · 2022-06-14T11:05:58Z

Description

https://revenuecats.atlassian.net/browse/CSDK-121

New API:

Purchases getCustomerInfo gets a new overload with a CacheFetchPolicy parameter. More info on the behavior for each case of this policy in the documentation:

/**
 * Specifies behavior for a caching API. Current values:
 * - CACHE_ONLY: Returns values from the cache, or throws an error if not available. It won't initiate a fetch.
 * - FETCH_CURRENT: Ignore whether the cache has a value or not and fetch the most up-to-date data. 
 *  This will return an error if the fetch request fails
 * - NOT_STALE_CACHED_OR_CURRENT: Returns the cached data if available and not stale. 
 *  If not available or stale, it fetches up-to-date data. 
 *  Note that this won't return stale data even if fetch request fails
 * - CACHED_OR_FETCHED: (default) returns the cached data if available (even if stale). 
 *  If not available, fetches up-to-date data. 
 *  If cached data is staled, it initiates a fetch in the background.
 */

I moved a lot of the logic dealing with customer info operations to a new class CustomerInfoHelper in order to move as much logic as possible away from Purchases.

TODO

Check process to update docs if needed

tonidero · 2022-06-14T11:06:40Z

I think this is ok... Since we basically won't be getting any value back, but lmk if you have any concerns.

tonidero · 2022-06-14T11:07:40Z

I moved this log from where it was before to here. Before, it would log success before the request succeeded. I think this is a better place but lmk if you have any concerns

tonidero · 2022-06-14T11:18:32Z

This is the main problem I had after extracting the logic to this class from the Purchases object. After we fetch customer info from the backend, we store the new value in the cache and we send the updated value to the listeners if it has changed. Both operations are synchronized on the purchases object, so I thought moving them from there could cause concurrency problems.

Since this method is always called from Purchases, I added this callback to perform those 2 operations. Originally, I intended to use the already existing ReceiveCustomerInfoCallback instead of this new callback, but whether we execute the operations or not depends on if the customer info comes from cache or from the backend and we don't have that information on the callback.

This solution seems okish considering it's internal but lmk what you think.

I see your point...

Are we caching or doing anything else with the customer info in Purchases though? If we are not maybe it's fine to remove this callback and move the cacheCustomerInfoAndSendToDelegateIfChanged to this class and synchronize on it.

If not, I think we don't have any other solution, unless we create a private monitor in Purchases (instead of synchronizing on Purchases's monitor) and pass it to this class?

Also, I was a bit confused by the name of the callback, maybe we could name it onSuccessfulFetchCallback? That way the name doesn't dictate what the implementation of the callback is supposed to do (caching the customer info).

Purchases also caches the customer info from some other sources, like when we restore purchases, we sync purchases with the backend or post a purchase to the backend. We could rename the CustomerInfoRetriever to CustomerInfoHelper and have all those cache pathways go through this class.

There is also the listener that is synchronized on the purchases object, and that was harder than the caching problem I would say... One solution I thought of is having the Purchases listener act as a proxy but having the actual listener live on the CustomerInfoRetriever/CustomerInfoHelper but I wasn't sure of this solution either...

Going to give it another thought but lmk if you would prefer something like that.

I went with the approach I suggested. The PR got pretty big... But I think we should start moving logic away from purchases, so I think it's a good change overall. Lmk what you think.

tonidero · 2022-06-14T11:21:00Z

With this policy, we will ignore the cache and just fetch from the backend, which is what we were doing before.

tonidero · 2022-06-14T11:22:37Z

Honestly, not sure why that was marked as nullable... It was always non null, lmk if I missed anything.

Interesting! I added that specific nullability in #70 . I am not sure why though 🤔

I checked for issues around that date regarding Handler and couldn't find anything either.

Let's remove the nullability and keep an eye on it just in case

tonidero · 2022-06-14T11:38:03Z

These are covered by other tests, and the fact that we have moved this logic out of here.

tonidero · 2022-06-14T11:39:18Z

We were not calling clearCachesForAppUserID in here at all. I think this was an outdated test?

I think it was to prevent someone from adding it? So if someone adds a call mockCache.clearCachesForAppUserID it will fail. Not a super clear test lol

Ok will add it back and rename it or add a comment so it's a bit clearer 👍

tonidero · 2022-06-14T11:41:33Z

These 2 are the operations I mentioned on this other comment: https://github.com/RevenueCat/purchases-android/pull/546/files#r896692163

vegaro · 2022-06-14T12:40:49Z

For the hybrids to work properly we need these error codes to match between iOS and Android. This is because our error handling in the hybrids is based on the premise that both iOS and Android will send the same code for the same error. In iOS error 25 means empty subscriber attributes at the moment https://github.com/RevenueCat/purchases-ios/blob/main/Sources/Error%20Handling/ErrorCode.swift#L49

At the moment, we are mapping errors to a error map that gets sent to the hybrids. See that code here https://github.com/RevenueCat/purchases-hybrid-common/blob/main/android/src/main/java/com/revenuecat/purchases/hybridcommon/common.kt#L343. So developers would get code: 25, readableErrorCode: CustomerInfoNotFoundInCache in Android and something different in iOS.

I know it is very messy and error prone, we should be doing a mapping from those error codes to the hybrids' own error system, like we should be mapping error code 25 in Android to a particular Javascript (or Dart, or whatever language the hybrid framework uses) error and a different error code in iOS. But we don't have anything like that in place yet.

I see we have error 29 in iOS https://github.com/RevenueCat/purchases-ios/blob/main/Sources/Error%20Handling/ErrorCode.swift#L54 which means CustomerInfoError. Maybe we could bring Android's errors to parity with iOS and use that one. I am not sure if that's the error iOS uses for cases when the customer info is not cached.

Ah interesting thanks for the knowledge dump!

vegaro · 2022-06-14T12:57:08Z

Interesting! I added that specific nullability in #70 . I am not sure why though 🤔

I checked for issues around that date regarding Handler and couldn't find anything either.

Let's remove the nullability and keep an eye on it just in case

vegaro · 2022-06-14T13:16:03Z

I see your point...

Are we caching or doing anything else with the customer info in Purchases though? If we are not maybe it's fine to remove this callback and move the cacheCustomerInfoAndSendToDelegateIfChanged to this class and synchronize on it.

If not, I think we don't have any other solution, unless we create a private monitor in Purchases (instead of synchronizing on Purchases's monitor) and pass it to this class?

vegaro · 2022-06-14T13:17:08Z

Also, I was a bit confused by the name of the callback, maybe we could name it onSuccessfulFetchCallback? That way the name doesn't dictate what the implementation of the callback is supposed to do (caching the customer info).

tonidero

This got bigger than I expected with the latest refactor... Still, I think it's cleaner than the previous approach, with the extra callback. Lmk what you think.

tonidero · 2022-06-15T12:19:54Z

I didn't add errors 26 and 27, since they seemed more related to iOS... I can port them here, but I believe we won't be using them. Lmk if you have any concerns over this.

tonidero · 2022-06-15T12:22:10Z

So I was debating with myself on whether to use the same pattern we use with the PurchasesState here and create something like CustomerInfoHelperState or something like that to hold the listener and the lastSentCustomerInfo. Maybe making the state immutable would be better, but I believe it should also work like this, since access to both properties should be synchronized. I went with the simple approach for now. Lmk if you think we should mimic that pattern cc @vegaro

Separated from that, I was wondering if we've thought about using WeakReference to hold the listener. That way we avoid possible leaks in our users. That said, this might break some of our users implementations, so might not be a good idea.

tonidero · 2022-06-15T12:31:12Z

I made the listener on the Purchases class just a proxy for the one in the CustomerInfoHelper. Most operations related with customer info now will need to go through there.

tonidero · 2022-06-15T12:36:21Z

I added this since I was getting confused with a lot of the deprecated warnings we have (since we need to be testing the deprecated code as well)

vegaro · 2022-06-16T11:09:05Z

Suggested change

* Get purchaser info from cache or network depending on fetch policy.

* Get customer info from cache or network depending on fetch policy.

Line 242 also needs to be updated

vegaro · 2022-06-16T11:41:33Z

Would it make sense to move these three lines to @After?

Well I think either would work... Will move it to @After since it does seem more of a cleanup task 👍

vegaro · 2022-06-16T11:41:54Z

Same question about @After

vegaro · 2022-06-16T11:42:23Z

vegaro · 2022-06-16T11:43:36Z

I think it was to prevent someone from adding it? So if someone adds a call mockCache.clearCachesForAppUserID it will fail. Not a super clear test lol

vegaro · 2022-06-16T11:49:41Z

This can be replaced with a @Synchronized annotation on set(value)

NachoSoto

I think this looks good 👍🏻
I'll let others approve it though

vegaro · 2022-06-17T14:07:45Z

This should be fail("supposed to be a failure")

😅 Good catch!

…tation

…foAndSendToDelegateIfChanged

…nctionality from Purchases to this class

…ding a comment for clarification

tonidero commented Jun 14, 2022

View reviewed changes

tonidero marked this pull request as ready for review June 14, 2022 11:45

tonidero requested a review from a team June 14, 2022 11:45

vegaro reviewed Jun 14, 2022

View reviewed changes

NachoSoto reviewed Jun 14, 2022

View reviewed changes

Comment thread api-tester/src/main/java/com/revenuecat/apitester/java/PurchasesAPI.java Outdated

Comment thread api-tester/src/main/java/com/revenuecat/apitester/kotlin/PurchasesAPI.kt Outdated

Comment thread purchases/src/test/java/com/revenuecat/purchases/PurchasesTest.kt Outdated

tonidero force-pushed the csdk-121 branch from ae56a94 to 0ca76fe Compare June 15, 2022 08:06

tonidero commented Jun 15, 2022

View reviewed changes

tonidero requested a review from vegaro June 15, 2022 12:41

vegaro reviewed Jun 16, 2022

View reviewed changes

tonidero requested review from NachoSoto and vegaro June 16, 2022 14:47

NachoSoto reviewed Jun 16, 2022

View reviewed changes

vegaro approved these changes Jun 17, 2022

View reviewed changes

tonidero added 16 commits June 17, 2022 16:19

Added new method to purchases that supports passing a

eeaa90b

Some test renames and small cleanup

16165af

Change missing customer info cache error to use same codes as iOS

7292f96

Setup comments to each of the cache fetch policies for better documen…

6f7a69b

…tation

Test CacheFetchPolicy enum separated from PurchasesAPI

8b77e29

Rename cacheCustomerInfoAndSendToDelegateIfChanges to cacheCustomerIn…

464a138

…foAndSendToDelegateIfChanged

Add getCustomerInfoWith listener function with fetch policy parameter

e1018c1

Fix detekt after method got too long

3357b8a

Refactor CustomerInfoRetriever to CustomerInfoHelper and move more fu…

b492a17

…nctionality from Purchases to this class

Remove left out commented test

086ef58

Make clear mocks happen during tear down

5f4c8aa

Substitute synchronized block with annotation in setter

fbcf8cf

Improve documentation of getCustomerInfo methods

721aa8c

Add back test to make sure we don't clear cache on request errors. Ad…

6e895f6

…ding a comment for clarification

Move some cleanup tasks to tearDown instead of setup of tests

4e42e61

Fix small issue with new test

716a237

Fix error comment with proper behavior

c4f7df6

tonidero force-pushed the csdk-121 branch from bdb2f1d to c4f7df6 Compare June 17, 2022 14:22

tonidero merged commit 2bed17a into main Jun 20, 2022

tonidero deleted the csdk-121 branch June 20, 2022 11:03

tonidero mentioned this pull request Jun 22, 2022

Release/5.2.0 #549

Merged

1 task

tonidero mentioned this pull request Jul 7, 2022

test-Release/5.2.0 #556

Closed

aboedo mentioned this pull request Jul 13, 2022

Release: 5.2.1 #563

Merged

taquitos mentioned this pull request Aug 1, 2022

PurchaserInfo contains no subscriptions if app was started offline at first start #161

Closed

	* Get purchaser info from cache or network depending on fetch policy.
	* Get customer info from cache or network depending on fetch policy.

Uh oh!

Conversation

tonidero commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero commented Jun 14, 2022 •

edited

Loading