Skip to content
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

Support enabling/disabling offline entitlements #964

Merged

Conversation

tonidero
Copy link
Contributor

@tonidero tonidero commented Apr 12, 2023

Description

This PR enables offline entitlements automatically when we receive 5xx response codes in the postReceipt requests or on getCustomerInfo if there isn't a cached version already. It also disables offline entitlements when we receive a successful response in one of the above requests or logIn/logout.

We went with a stateful approach since we need to return the offline computed customer info from getCustomerInfo after a postReceipt fails.

All this is also behind a flag right now that is hardcoded so we don't enter offline entitlements mode yet.

@tonidero tonidero added the refactor A code change that neither fixes a bug nor adds a feature label Apr 12, 2023
)
} else {
dispatch { callback.onError(error) }
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note how, for restores, I'm only enabling offline entitlements if the "last" purchase to be synced fails with a 5xx. This could potentially mean that we fail with a 5xx in a previous purchase but if the last doesn't fail, we would miss that, but I think it's an edge case we can live with for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh right. Yeah, it's still kinda bonkers that we're not batching these on android tbh.
If the backend is fully down, then this won't make a difference. If the backend is just struggling... there's a chance that we return success but miss a token, right?
What do we do without offline entitlements if a single token is missed?

Copy link
Contributor Author

@tonidero tonidero Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the backend is just struggling... there's a chance that we return success but miss a token, right?

That's correct...

What do we do without offline entitlements if a single token is missed?

Same behavior. We only look at the "last" purchase result, not any others... We would log an error message if any of the post fails, but we would still return a success if the last purchase is sent correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah... I would love for us to eventually clean this up and have a single completion block that waits for all tokens and returns a value for all of them. But that's out of scope for this one, we should try to be as good as the current (not offline) system is for now.

As for the token that didn't get synced, we're still ack'ing but not consuming, and we'll retry on next app open, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the token that didn't get synced, we're still ack'ing but not consuming, and we'll retry on next app open, right?

Actually we are currently not ack'ing nor consuming the purchases on 5xx errors. We can talk about whether we should change that behavior separately.

calculateOfflineCustomerInfo(appUserID, { onSuccess() }, { onError(it) })
} else {
onError(error)
}
Copy link
Contributor Author

@tonidero tonidero Apr 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of duplication in this file... We post the receipt in 3 different places (purchases, sync purchases and restore purchases), and we need to handle all 3. I first tried to extract the post receipt flow to a different class so we could reuse more code, but it got tricky since they aren't exactly the same and there are several nuances... I decided to delay that refactor for now, in favor of getting this done quickly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. It feels like at the very least we can turn these into one-liners and reuse that (as a follow-up)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the end, I worked on that refactor in #967 ... I think it helps to separate that logic and not have it in Purchases, specially when writing the tests. It was a big refactor though...

@@ -76,6 +78,7 @@ class IdentityManager(
deviceCache.cacheAppUserID(newAppUserID)
deviceCache.cacheCustomerInfo(newAppUserID, customerInfo)
copySubscriberAttributesToNewUserIfOldIsAnonymous(oldAppUserID, newAppUserID)
offlineEntitlementsManager.resetOfflineCustomerInfoCache()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have state, we need to remember to clear the cache on successes of any operation

dispatch { callback?.onError(error) }
}
)
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about also disabling offline entitlements if we have a non-server error to try to go back to the normal flow as soon as possible. However, this could potentially be a network error or something else, which would mean that if the user enters offline entitlements mode and then loses connection, they would lose any granted entitlements so in the end I decided not to.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so just to check, we're keeping it on as long as we get 4xx and 5xx, but 2xx and 3xx deactivate, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct! We will keep it enabled until we get a successful response from the backend. Any 4xx, 5xx or network errors won't disable it.

dispatch { callback?.onReceived(offlineComputedCustomerInfo) }
},
onError = {
dispatch { callback?.onError(error) }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note how on errors with calculating the offline entitlements, I propagate the original backend error, and not the error calculating the offline entitlements. I think that's preferable. Lmk if you think otherwise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, if we try but fail to compute offline we should just send whatever the backend sent, no value added in doing anything else

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe renaming error to backendError would make this more clear for the future

@tonidero

This comment was marked as outdated.

Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking great so far!

isServerError: Boolean,
appUserId: String
): Boolean {
return isServerError && deviceCache.getCachedCustomerInfo(appUserId) == null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good catch - without this, if we just rely on the error code, we'll be sending offline customer info to the delegate needlessly, since we already have a cached value and it might be up to date as far as we know

Copy link
Contributor Author

@tonidero tonidero Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is part of the triggers in the doc. This makes it so we don't enable offline entitlements if there is a cached value already. But if the user posted a receipt first, enabling offline entitlements, we would still return that over the cached customer info in getCustomerInfo

dispatch { callback?.onReceived(offlineComputedCustomerInfo) }
},
onError = {
dispatch { callback?.onError(error) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, if we try but fail to compute offline we should just send whatever the backend sent, no value added in doing anything else

dispatch { callback?.onError(error) }
}
)
} else {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so just to check, we're keeping it on as long as we get 4xx and 5xx, but 2xx and 3xx deactivate, right?

)
} else {
dispatch { callback.onError(error) }
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh right. Yeah, it's still kinda bonkers that we're not batching these on android tbh.
If the backend is fully down, then this won't make a difference. If the backend is just struggling... there's a chance that we return success but miss a token, right?
What do we do without offline entitlements if a single token is missed?

calculateOfflineCustomerInfo(appUserID, { onSuccess() }, { onError(it) })
} else {
onError(error)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. It feels like at the very least we can turn these into one-liners and reuse that (as a follow-up)

Base automatically changed from toniricodiez/sdk-2992-write-flow-to-use-locally-computed-1 to main April 14, 2023 08:39
@tonidero tonidero force-pushed the toniricodiez/sdk-2992-write-flow-to-use-locally-computed-2 branch from 033e603 to 4d3d297 Compare April 14, 2023 10:34
@tonidero tonidero changed the base branch from main to toniricodiez/sdk-2992-write-flow-to-use-locally-computed-1.5 April 14, 2023 10:34
Base automatically changed from toniricodiez/sdk-2992-write-flow-to-use-locally-computed-1.5 to main April 17, 2023 09:55
tonidero added a commit that referenced this pull request Apr 17, 2023
### Description
This became a complex refactor, specially on the tests... Basically, we
currently have 3 places where we post receipts to the backend: restores,
sync purchases and purchases. All this logic lived in `Purchases` and
part of it was duplicated. In this refactor, I'm extracting this logic
from all 3 places to a new `PostReceiptHelper` so we can share the
common logic between them.

The main code itself wasn't too bad to refactor, but the tests were very
painful to untangle, since we had them in different places.

The purpose of doing this now is so we can share the logic for offline
entitlements in #964 and also some very needed cleanup of `Purchases`.

#### TODO
- [x] Verify all test cases are also tested with the new code
- [x] Test this change deeply
@tonidero tonidero force-pushed the toniricodiez/sdk-2992-write-flow-to-use-locally-computed-2 branch from 88cddc3 to 21e6e39 Compare April 17, 2023 10:53

fun updateProductEntitlementMappingCacheIfStale() {
if (deviceCache.isProductEntitlementMappingCacheStale()) {
if (appConfig.areOfflineEntitlementsEnabled && deviceCache.isProductEntitlementMappingCacheStale()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the feature flag for updating the mapping as well, so we don't need to keep that code commented out.

onError = {
onError?.let { it(purchase, error) }
}
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty similar between both post methods here. Originally I tried to move it to the common postReceiptAndSubscriberAttributes method in this class, but that was tricky since in that case, we would be calling the onSuccess blocks in these callers which always consume/save the sent token in cache... We don't want to consume purchases on offline entitlements mode, so I had to separate this so it still went through the onError blocks. It's possible this can be refactored better, but I think this is ok for now.

@@ -209,8 +208,7 @@ class Purchases internal constructor(
fetchAndCacheOfferings(identityManager.currentAppUserID, appInBackground = false)
log(LogIntent.RC_SUCCESS, OfferingStrings.OFFERINGS_UPDATED_FROM_NETWORK)
}
// Offline entitlements: Commenting out for now until backend is ready
// offlineEntitlementsManager.updateProductEntitlementMappingCacheIfStale()
offlineEntitlementsManager.updateProductEntitlementMappingCacheIfStale()
Copy link
Contributor Author

@tonidero tonidero Apr 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine now, since we have an internal feature flag in AppConfig that disables it.

@tonidero tonidero changed the title [WIP] Add stateful approach to enabling/disabling offline entitlements Support enabling/disabling offline entitlements Apr 17, 2023
@tonidero tonidero marked this pull request as ready for review April 17, 2023 15:32
@tonidero tonidero requested review from a team and aboedo April 17, 2023 15:32
dispatch { callback?.onReceived(offlineComputedCustomerInfo) }
},
onError = {
dispatch { callback?.onError(error) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe renaming error to backendError would make this more clear for the future

deviceCache.addSuccessfullyPostedToken(purchaseToken)
onSuccess()
},
onError = { error, shouldConsumePurchase, _, _ ->
onError = { error, shouldConsumePurchase, isServerError, _ ->
if (shouldConsumePurchase) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's confusing that you're not checking if isServerError here because shouldConsumePurchase will already be sent as false here if offlineEntitlements shouldn't be used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is pretty confusing and error prone... Will try to refactor this so it's clearer that those 2 flags are exclusive

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried an approach to making this clearer in this commit: d2f64cf. Lmk what you think!

@tonidero tonidero marked this pull request as draft April 18, 2023 11:13
CAN_BE_CONSUMED,
SERVER_ERROR,
CANNOT_BE_CONSUMED
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the best way, but basically, we can do 3 different things on a post receipt error.

  • Consume the purchase, even though it's an error (CAN_BE_CONSUMED)
  • Enable offline entitlements without consuming (SERVER_ERROR)
  • Nothing (CANNOT_BE_CONSUMED)
    This is what I'm trying to represent here... Feedback on naming or alternatives are welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostReceiptErrorHandlingBehavior? Or something to that effect? "Type" feels a little too close to types in terms of coding language

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as for the others... total nitpick but I'd maybe opt for a what should happen rather than what can, i.e.:
SHOULD_BE_CONSUMED | SHOULD_ACK_AND_NOT_CONSUME | SHOULD_NOT_CONSUME,
although right now I'm not entirely sure of the differences in behavior between server_error and cannot_be_consumed, I just started reviewing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... I was trying to avoid having the logic of what to do in the backend class... But that's probably much simpler so will do that 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure of the differences in behavior between server_error and cannot_be_consumed

Basically, one will enter offline entitlements mode and eventually return a success to the developer (if no errors occur calculating offline customer info), the other will just error out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the PostReceiptDataErrorCallback accepted an array? I am not super convinced of SHOULD_USE_OFFLINE_ENTITLEMENTS_AND_NOT_CONSUME and SHOULD_NOT_CONSUME both indicating the behavior of not consuming.

With an array of behaviors you could change this enum to just be CONSUME, USE_OFFLINE_ENTITLEMENTS. Then the absence of CONSUME indicates to not consume, so in the case of USE_OFFLINE_ENTITLEMENTS, it will not consume because CONSUME won't be in the array. The array will never have more than one value though 🤔

What you have is also valid, just wanted to share the idea of the array

Copy link
Contributor Author

@tonidero tonidero Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm the thing about it is that the behavior is currently exclusive... As in, we will never CONSUME if we are using offline entitlements. I think making it an array where we could potentially send both CONSUME and USE_OFFLINE_ENTITLEMENTS but we will not support both at the same time could be confusing...

I could rename SHOULD_USE_OFFLINE_ENTITLEMENTS_AND_NOT_CONSUME to SHOULD_USE_OFFLINE_ENTITLEMENTS. And assume the "not consumption" part to be implicit... What do you think?

@tonidero tonidero marked this pull request as ready for review April 18, 2023 13:33
@tonidero tonidero requested a review from vegaro April 18, 2023 14:54
Comment on lines +26 to +27
// For now hardcoded to false until we are ready to enable it.
val areOfflineEntitlementsEnabled = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

CAN_BE_CONSUMED,
SERVER_ERROR,
CANNOT_BE_CONSUMED
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostReceiptErrorHandlingBehavior? Or something to that effect? "Type" feels a little too close to types in terms of coding language

CAN_BE_CONSUMED,
SERVER_ERROR,
CANNOT_BE_CONSUMED
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as for the others... total nitpick but I'd maybe opt for a what should happen rather than what can, i.e.:
SHOULD_BE_CONSUMED | SHOULD_ACK_AND_NOT_CONSUME | SHOULD_NOT_CONSUME,
although right now I'm not entirely sure of the differences in behavior between server_error and cannot_be_consumed, I just started reviewing

Comment on lines 468 to 471
} else if (purchasesError.code != PurchasesErrorCode.UnsupportedError) {
PostReceiptErrorType.CAN_BE_CONSUMED
} else {
PostReceiptErrorType.CANNOT_BE_CONSUMED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this reads fairly confusingly - if it's unsupported then we can consume, if it's supported we can't?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's actually the opposite right? As in, if it's an UnsupportedError we won't consume. If it's a different error, we will consume. It can be a bit confusing but that's the logic that we have right now though... Will just move things to avoid that negation though so it's a bit clearer

CAN_BE_CONSUMED,
SERVER_ERROR,
CANNOT_BE_CONSUMED
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the PostReceiptDataErrorCallback accepted an array? I am not super convinced of SHOULD_USE_OFFLINE_ENTITLEMENTS_AND_NOT_CONSUME and SHOULD_NOT_CONSUME both indicating the behavior of not consuming.

With an array of behaviors you could change this enum to just be CONSUME, USE_OFFLINE_ENTITLEMENTS. Then the absence of CONSUME indicates to not consume, so in the case of USE_OFFLINE_ENTITLEMENTS, it will not consume because CONSUME won't be in the array. The array will never have more than one value though 🤔

What you have is also valid, just wanted to share the idea of the array

@tonidero tonidero requested a review from vegaro April 26, 2023 09:53
@tonidero
Copy link
Contributor Author

Will merge this as is. If there are any further comment, happy to address them later

@tonidero tonidero merged commit 715d491 into main Apr 27, 2023
4 checks passed
@tonidero tonidero deleted the toniricodiez/sdk-2992-write-flow-to-use-locally-computed-2 branch April 27, 2023 07:10
tonidero added a commit that referenced this pull request May 18, 2023
**This is an automatic release.**

### New Features
* CAT-859 Expose whether or not a SubscriptionOption is Prepaid in the
SDK (#1005) via Deema AlShamaa (@dalshamaa)
### Bugfixes
* [CF-1324] Fix personalizedPrice defaulting to false (#952) via beylmk
(@beylmk)
### Performance Improvements
* Store and return ETag last refresh time header (#978) via Toni Rico
(@tonidero)
### Dependency Updates
* Bump fastlane-plugin-revenuecat_internal from `3b03efa` to `fe45299`
(#991) via dependabot[bot] (@dependabot[bot])
* Bump danger from 9.2.0 to 9.3.0 (#981) via dependabot[bot]
(@dependabot[bot])
* Bump fastlane-plugin-revenuecat_internal from `8482a43` to `3b03efa`
(#974) via dependabot[bot] (@dependabot[bot])
* Bump fastlane from 2.212.1 to 2.212.2 (#973) via dependabot[bot]
(@dependabot[bot])
* Bump fastlane-plugin-revenuecat_internal from `9255366` to `8482a43`
(#961) via dependabot[bot] (@dependabot[bot])
### Other Changes
* Add proration modes to post to backend (#977) via swehner (@swehner)
* Added ENTITLEMENTS_COMPUTED_ON_DEVICE (#939) via Cesar de la Vega
(@vegaro)
* Fix flaky test in OfflineCustomerInfoCalculatorTest (#997) via Cesar
de la Vega (@vegaro)
* Fix `OfflineCustomerInfoCalculatorTest` `Unresolved reference:
ProducType` (#995) via Cesar de la Vega (@vegaro)
* Add support for product_plan_identifier for offline customer info
(#959) via Cesar de la Vega (@vegaro)
* Add non-subscriptions support to offline customer info (#958) via
Cesar de la Vega (@vegaro)
* Query only active purchases when generating offline entitlements
customer info (#1003) via Toni Rico (@tonidero)
* Fix `PurchasesIntegrationTest` building issue (#996 into main) (#998)
via Cesar de la Vega (@vegaro)
* Fail offline entitlements computation if product entitlement mapping
not available (#999) via Toni Rico (@tonidero)
* Fix  build_magic_weather lane (#993) via Cesar de la Vega (@vegaro)
* Add backend integration tests and test product entitlement mapping
endpoint (#988) via Toni Rico (@tonidero)
* Fix purchases integration tests (#980) via Toni Rico (@tonidero)
* Disable offline entitlements if active inapp purchases exist (#983)
via Toni Rico (@tonidero)
* Clear cached customer info upon entering offline entitlements mode
(#989) via Toni Rico (@tonidero)
* Update product entitlement mapping request to new format (#976) via
Toni Rico (@tonidero)
* Support enabling/disabling offline entitlements (#964) via Toni Rico
(@tonidero)
* Add back integration tests automation (#972) via Toni Rico (@tonidero)
* Upgrade to AGP 8.0 (#975) via Toni Rico (@tonidero)
* Extract post receipt logic to PostReceiptHelper (#967) via Toni Rico
(@tonidero)
* Add isServerDown to error callback for postReceipt and getCustomerInfo
requests (#963) via Toni Rico (@tonidero)
* Add back integration test flavors (#962) via Toni Rico (@tonidero)
* Fix storing test results (#966) via Cesar de la Vega (@vegaro)
* Extract detekt job from test job (#965) via Cesar de la Vega (@vegaro)


[CF-1324]:
https://revenuecats.atlassian.net/browse/CF-1324?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

---------

Co-authored-by: revenuecat-ops <ops@revenuecat.com>
Co-authored-by: Toni Rico <antonio.rico.diez@revenuecat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants