Add isServerDown to error callback for postReceipt and getCustomerInfo requests#963
Conversation
| receivedCustomerInfo = null | ||
| receivedShouldConsumePurchase = null | ||
| receivedCustomerInfoCreated = null | ||
| receivedIsServerError = null |
There was a problem hiding this comment.
We were not clearing all these for each test before...
| onSuccess() | ||
| }, | ||
| onError = { error, shouldConsumePurchase, body -> | ||
| onError = { error, shouldConsumePurchase, _, body -> |
There was a problem hiding this comment.
Currently unused but we will need to use the new parameter to detect whether we need to compute offline entitlements or not.
|
|
||
| /** @suppress */ | ||
| internal typealias CustomerInfoCallback = Pair<(CustomerInfo) -> Unit, (PurchasesError) -> Unit> | ||
| internal typealias CustomerInfoCallback = Pair<(CustomerInfo) -> Unit, (PurchasesError, isServerError: Boolean) -> Unit> |
There was a problem hiding this comment.
Would it be possible to add this data to PurchasesError?
There was a problem hiding this comment.
Hmm it's possible but I don't think it's the way to do it since PurchasesError is actually a public class, so it would mean that data would be public. This seems to be a very specific error which shouldn't be exposed.
There was a problem hiding this comment.
Oh gotcha. We solve this in iOS by having an internal error at the Backend level, that we don't convert to the public type until the top of the hierarchy.
| callbacks.remove(cacheKey) | ||
| }?.forEach { (_, onError) -> | ||
| onError(error) | ||
| onError(error, false) |
There was a problem hiding this comment.
can we add the parameter name here so we know what's being set to false?
There was a problem hiding this comment.
Hmm I think that's not allowed in Kotlin right? I get Named arguments are not allowed for function types. We could maybe pass this by passing an object instead of the parameters like this, but seems like overboard. I will just extract it to a constant before this, so at least we have the name clear here.
There was a problem hiding this comment.
ohhh you're correct, didn't notice that. Nevermind then
| @@ -114,10 +119,13 @@ class Backend( | |||
| if (result.isSuccessful()) { | |||
| onSuccess(CustomerInfoFactory.buildCustomerInfo(result)) | |||
There was a problem hiding this comment.
We are building the CustomerInfo here. Do we need to send to lower layers that the backend is down? Or could we check here if it's a server error and compute the offline customer info and send it to the onSuccess? That way we isolate the logic in the networking layer. We might not want to do that though. Let me know what you think.
There was a problem hiding this comment.
I started on a possible approach for this logic here: #964. I kinda prefer to keep it outside the networking layer since seems like the logic doesn't really belong there...
In that PR, I tried to move as much logic as possible to the OfflineEntitlementsManager, but there is still logic in the Purchases and CustomerInfoHelper classes. And there is actually some duplication in Purchases due to that... I actually would like to refactor some code there to avoid the duplication, but it's tricky...
We can discuss this more over Slack
| const val ERROR = 500 | ||
|
|
||
| fun isSuccessful(statusCode: Int) = statusCode < BAD_REQUEST | ||
| fun isServerError(statusCode: Int) = statusCode >= ERROR |
There was a problem hiding this comment.
Are timeouts 500s in Android? Because I believe if the server is completely down that it timeouts, we want to compute the offline customer info, right?
There was a problem hiding this comment.
Hmm I think that would depend on how the server is configured right? I think you can configure how to respond at different levels in the server stack... Also, if the whole infrastructure was completely down, I guess we would have a reachability exception? In any case, I think this is something we should confirm with COIN 👍
There was a problem hiding this comment.
Also, I don't think we want to enable offline entitlements on a timeout, since it could possibly be a connection issue.
There was a problem hiding this comment.
Just checked the slack conversation
| } | ||
| } catch (e: JSONException) { | ||
| onError(e.toPurchasesError().also { errorLog(it) }, false, null) | ||
| val isServerError = false |
**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>
Description
First PR for SDK-2992
In this PR, we add a
isServerErrorparameter to the error callback for thepostReceiptandgetCustomerInfobackend requests. This will be true if we get a 5xx error code, false otherwise.This is currently unused but will be used to detect whether we need to compute offline entitlements