Skip to content

Sync pending purchases before getting customer info#1073

Merged
tonidero merged 5 commits into
mainfrom
toniricodiez/sdk-3155-android-if-sdk-detects-unsynced-purchases-when-calling-3
Jun 20, 2023
Merged

Sync pending purchases before getting customer info#1073
tonidero merged 5 commits into
mainfrom
toniricodiez/sdk-3155-android-if-sdk-detects-unsynced-purchases-when-calling-3

Conversation

@tonidero

@tonidero tonidero commented Jun 16, 2023

Copy link
Copy Markdown
Contributor

Description

Followup to #1052 and #1058

This changes the behavior of fetching customer info. Now, when fetching customer info, we will first try to sync any pending purchases if any. If there are pending purchases, we will return the customer info from the last request sent. If there aren't, we will then hit the current customer info endpoint to get the latest customer info.

@tonidero tonidero changed the title Sync pending purchases before getting customer info from the backend Sync pending purchases before getting customer info Jun 16, 2023
@tonidero tonidero marked this pull request as ready for review June 16, 2023 09:42
@tonidero tonidero requested a review from a team June 16, 2023 09:42

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the main change of this PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need the IfNeeded? At the end of the day this is just syncing pending purchases right and not really checking if it's needed right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is another change I would like to note. This made it so, in the beginning of the SDK initialization, the billing client would likely be disconnected, so it usually failed. The queryPurchases method already has a mechanism to call the callbacks once the billing client is connected, so this seemed unnecesarily. The only thing that slightly concerns me is that this method may take longer now, since it needs to wait for the billing client to connect during SDK initialization. I thought it was ok though. Thoughts?

@codecov

codecov Bot commented Jun 16, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1073 (3c59515) into main (6edb303) will increase coverage by 0.01%.
The diff coverage is 90.68%.

❗ Current head 3c59515 differs from pull request most recent head 65becd2. Consider uploading reports for the commit 65becd2 to get more accurate results

@@            Coverage Diff             @@
##             main    #1073      +/-   ##
==========================================
+ Coverage   85.84%   85.86%   +0.01%     
==========================================
  Files         179      179              
  Lines        6336     6359      +23     
  Branches      876      876              
==========================================
+ Hits         5439     5460      +21     
- Misses        556      557       +1     
- Partials      341      342       +1     
Impacted Files Coverage Δ
...evenuecat/purchases/strings/CustomerInfoStrings.kt 0.00% <ø> (ø)
...om/revenuecat/purchases/strings/PurchaseStrings.kt 0.00% <ø> (ø)
...rchases/PostTransactionWithProductDetailsHelper.kt 82.85% <82.85%> (ø)
...enuecat/purchases/PostPendingTransactionsHelper.kt 85.45% <85.45%> (-0.76%) ⬇️
...revenuecat/purchases/CustomerInfoUpdateReceiver.kt 87.87% <87.87%> (ø)
...lin/com/revenuecat/purchases/CustomerInfoHelper.kt 89.69% <96.55%> (+0.08%) ⬆️
...tlin/com/revenuecat/purchases/PostReceiptHelper.kt 94.11% <100.00%> (ø)
.../main/kotlin/com/revenuecat/purchases/Purchases.kt 81.41% <100.00%> (+0.12%) ⬆️
...otlin/com/revenuecat/purchases/PurchasesFactory.kt 86.53% <100.00%> (+0.08%) ⬆️
...in/com/revenuecat/purchases/SyncPurchasesHelper.kt 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@tonidero tonidero force-pushed the toniricodiez/sdk-3155-android-if-sdk-detects-unsynced-purchases-when-calling-2 branch from f46db41 to d3cdbcc Compare June 19, 2023 08:30
@tonidero tonidero force-pushed the toniricodiez/sdk-3155-android-if-sdk-detects-unsynced-purchases-when-calling-3 branch from 1ab2c4a to 3c59515 Compare June 19, 2023 08:52

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need the IfNeeded? At the end of the day this is just syncing pending purchases right and not really checking if it's needed right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe: "CustomerInfo updated after syncing pending purchases."

Copy link
Copy Markdown
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 not technically after, it's more "from" syncing pending purchases. As in, if there are pending purchases, we will use the customer info returned by the last request that syncs those pending purchases and we won't do the "get customer info" request after. But yeah I can add that.

@tonidero tonidero force-pushed the toniricodiez/sdk-3155-android-if-sdk-detects-unsynced-purchases-when-calling-2 branch from d3cdbcc to 4a44b45 Compare June 20, 2023 08:20
Base automatically changed from toniricodiez/sdk-3155-android-if-sdk-detects-unsynced-purchases-when-calling-2 to main June 20, 2023 08:32
@tonidero tonidero force-pushed the toniricodiez/sdk-3155-android-if-sdk-detects-unsynced-purchases-when-calling-3 branch from 3c59515 to 65becd2 Compare June 20, 2023 08:53
@tonidero tonidero enabled auto-merge (squash) June 20, 2023 08:54
@tonidero tonidero merged commit e414de2 into main Jun 20, 2023
@tonidero tonidero deleted the toniricodiez/sdk-3155-android-if-sdk-detects-unsynced-purchases-when-calling-3 branch June 20, 2023 09:03
@tonidero tonidero mentioned this pull request Jun 21, 2023
tonidero added a commit that referenced this pull request Jun 22, 2023
### Description
An integration test started failing due to #1073. The error was that the
schemaVersion field in the customer info didn't match between the
fetched and cached customer info. It was actually luck that made it work
before 😬 .

Before that change, this test tried to get customer info twice: On sdk
configuration and the first part of the test. Since both of these happen
pretty quickly, we only made the request once and called both callbacks
with the same response. However, the first request actually modified the
JSONObject, adding the schema version and some other fields in order to
cache it in the device. This made it so the JSONObject actually had
those fields in the second callback, causing the schemaVersion to have a
value. Then, the value returned from cache already has the schemaVersion
field set so the test passed.

After the change in
#1073, the test
still tries to get customer info twice, but the raw json sharing didn't
happen, since when we call getCustomerInfo we now need to check first
for unsynced purchases, causing us to not share the callback, so the
schema version that was set in the first callback wasn't shared with the
second one, causing the schemaVersion field to be 0, causing the test to
fail.

After talking about it, we think defaulting the schema version to the
latest known version by the SDK is probably expected behavior, instead
of defaulting to 0 before it's been saved.

Followup to #1078
tonidero added a commit that referenced this pull request Jun 23, 2023
### Description
#1073 broke some more integration tests. I fixed one in #1080, but
didn't run the offline entitlements integration tests 🤦‍♂️ .

Most of these make sense after that functionality was added
tonidero pushed a commit that referenced this pull request Jun 23, 2023
**This is an automatic release.**

### Bugfixes
* Default customer info schema version to latest known by SDK (#1080)
via Toni Rico (@tonidero)
* Handle other diagnostics-related exceptions (#1076) via Toni Rico
(@tonidero)
* Return error in queryPurchases if error connecting to billing client
(#1072) via Toni Rico (@tonidero)
### Other Changes
* Fix offline entitlements integration tests (#1085) via Toni Rico
(@tonidero)
* Add defaultsRelease variant tests run configuration (#1074) via Toni
Rico (@tonidero)
* Compose sample app: move to gradle catalog (#1081) via Toni Rico
(@tonidero)
* Compose sample app: automate builds (#1082) via Toni Rico (@tonidero)
* Compose sample app (#1056) via Toni Rico (@tonidero)
* Migrate to Gradle version catalog (#1059) via Cesar de la Vega
(@vegaro)
* Trusted entitlements: Add logs with verification mode (#1067) via Toni
Rico (@tonidero)
* Sync pending purchases before getting customer info (#1073) via Toni
Rico (@tonidero)
* Refactor syncing pending transactions logic out of `Purchases` (#1058)
via Toni Rico (@tonidero)
* Refactor CustomerInfo listener and cache logic into
CustomerInfoUpdater (#1052) via Toni Rico (@tonidero)
* Trusted entitlements: Add integration tests (#1071) via Toni Rico
(@tonidero)
* Trusted entitlements: Add internal mechanism to force signing errors
for tests (#1070) via Toni Rico (@tonidero)

Co-authored-by: revenuecat-ops <ops@revenuecat.com>
tonidero added a commit that referenced this pull request Jul 10, 2023
### Description
In some situations, we weren't refreshing the customer info on app open.
This was caused by #1073. In that PR we changed to fetch current
purchases before getting customer info, so we would sync purchases
before getting customer info. However, when we fetch customer info
during the app open's `onAppForeground` callback, the
`billing.purchasesUpdatedListener` was not set. Currently, if that
listener isn't set, we would just [skip the
query](https://github.com/RevenueCat/purchases-android/blob/ef34062996a17aa6a47d6eb2ca0903d66a51ab5e/purchases/src/main/kotlin/com/revenuecat/purchases/google/BillingWrapper.kt#L142)
altogether so we would never get a response.

Thanks @joshdholtz  for bringing it up!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants