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

Offline Entitlements: don't compute offline CustomerInfo when purchasing a consumable products #2522

Merged
merged 8 commits into from
May 24, 2023

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented May 22, 2023

This will be logged when doing so:

ERROR: 😿 ❗❗ Error computing offline CustomerInfo. Will return original error.
Creation error: Unable to compute offline CustomerInfo when purchasing consumable products.

Changes:

  • Added unit tests for PostReceiptDataOperation
  • Added integration test for this new scenario
  • PostReceiptDataOperation won't use OfflineCustomerInfoCreator when purchasing consumable products

@NachoSoto NachoSoto requested a review from a team May 22, 2023 20:04
@NachoSoto
Copy link
Contributor Author

NachoSoto commented May 22, 2023

I ended up going with this approach which I think is much more elegant. Avoids having to parse the local receipt unnecessarily.

@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Merging #2522 (a55721e) into main (c48b080) will decrease coverage by 0.09%.
The diff coverage is 40.62%.

❗ Current head a55721e differs from pull request most recent head 3184abf. Consider uploading reports for the commit 3184abf to get more accurate results

@@            Coverage Diff             @@
##             main    #2522      +/-   ##
==========================================
- Coverage   87.82%   87.74%   -0.09%     
==========================================
  Files         199      199              
  Lines       13743    13726      -17     
==========================================
- Hits        12070    12044      -26     
- Misses       1673     1682       +9     
Impacted Files Coverage Δ
...OfflineEntitlements/PurchasedProductsFetcher.swift 69.23% <37.93%> (-19.66%) ⬇️
...s/Logging/Strings/OfflineEntitlementsStrings.swift 81.48% <66.66%> (-2.84%) ⬇️

... and 13 files with indirect coverage changes

@@ -39,11 +39,16 @@ final class PostReceiptDataOperation: CacheableNetworkOperation {
customerInfoCallbackCache: CallbackCache<CustomerInfoCallback>,
offlineCustomerInfoCreator: OfflineCustomerInfoCreator?
) -> CacheableNetworkOperationFactory<PostReceiptDataOperation> {
// Can't compute offline CustomerInfo when purchasing non-consumables
let offlineCreator: OfflineCustomerInfoCreator? = postData.isNonSubscriptionProduct
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm so this will only work when posting a receipt right? I don't think this will handle this scenario:

  • User purchases a consumable while offline (the post fails)
  • For some reason, they invalidate customer info or uninstall/reinstall (so no customer info cached)
  • They restart the app
  • During restart, get customer info request fails with a 5xx, so we try to enter offline entitlements even though it has a consumable.

Admittedly, this seems like an uncommon case (users invalidating customer info in the middle), but ideally we would also handle it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. The new implementation fixes that 👍🏻
Also I think it's much cleaner moving the check to use StoreKit.Transaction.unfinished.

Base automatically changed from product-fetcher-sync-appstore to main May 23, 2023 16:05
@NachoSoto NachoSoto force-pushed the nacho/sdk-3030-test-consumables branch from 85fe2ae to cf79196 Compare May 23, 2023 17:03
@NachoSoto NachoSoto requested review from tonidero and aboedo May 23, 2023 17:29
@NachoSoto NachoSoto force-pushed the nacho/sdk-3030-test-consumables branch from 8f70b75 to a55721e Compare May 23, 2023 17:30
@NachoSoto NachoSoto changed the title Offline Entitlements: don't compute offline CustomerInfo when purchasing a consumable product Offline Entitlements: don't compute offline CustomerInfo when purchasing a consumable products May 23, 2023
Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -97,4 +101,48 @@ class PurchasedProductsFetcher: PurchasedProductsFetcherType {
return result
}

private static var hasPendingConsumablePurchase: Bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to unit test these? I guess we would have to mock StoreKit. We do have integration tests so it's probably okish, but I think it's good to test things at different granularities to try to have as much coverage as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's indirectly tested, but I agree it would be useful to test independently 👍🏻 which we can do with SKTestSession.

I added a type in #2533 to do something similar, so I'm going to combine both things and test them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted and tested in #2539 like you suggested.

@@ -97,4 +101,48 @@ class PurchasedProductsFetcher: PurchasedProductsFetcherType {
return result
}

private static var hasPendingConsumablePurchase: Bool {
get async {
return await StoreKit.Transaction.unfinished.contains {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to check, but I'm guessing SK2 will cache the transactions internally right? So this shouldn't take too long.

Also, will this be executed in a background thread? I'm guessing since the API is using async/await that might be handled internally by SK2 though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The short answer is: it doesn't matter. Concurrency in Swift is different from what you'd expect. Even if this takes 1 hour, it's not stopping the thread, it creates a continuation.

NachoSoto and others added 8 commits May 24, 2023 11:56
…chasing a consumable product

### Changes:
- Added unit tests for `PostReceiptDataOperation`
- Added integration test for this new scenario
- `PostReceiptDataOperation` won't use `OfflineCustomerInfoCreator` when purchasing consumable products
… ios-12 (#2526)

Requested by @NachoSoto for
[nacho/sdk-3030-test-consumables](https://github.com/RevenueCat/purchases-ios/tree/nacho/sdk-3030-test-consumables)

Co-authored-by: Distiller <distiller@static.38.39.184.83.cyberlynk.net>
… ios-16 (#2525)

Requested by @NachoSoto for
[nacho/sdk-3030-test-consumables](https://github.com/RevenueCat/purchases-ios/tree/nacho/sdk-3030-test-consumables)

Co-authored-by: Distiller <distiller@static.38.23.39.112.cyberlynk.net>
… ios-13 (#2527)

Requested by @NachoSoto for
[nacho/sdk-3030-test-consumables](https://github.com/RevenueCat/purchases-ios/tree/nacho/sdk-3030-test-consumables)

Co-authored-by: Distiller <distiller@static.38.23.38.23.cyberlynk.net>
@NachoSoto NachoSoto force-pushed the nacho/sdk-3030-test-consumables branch from a55721e to 3184abf Compare May 24, 2023 19:15
@NachoSoto NachoSoto enabled auto-merge (squash) May 24, 2023 19:15
@NachoSoto NachoSoto merged commit edc0f6b into main May 24, 2023
@NachoSoto NachoSoto deleted the nacho/sdk-3030-test-consumables branch May 24, 2023 19:33
This was referenced May 31, 2023
@NachoSoto NachoSoto mentioned this pull request May 31, 2023
NachoSoto added a commit that referenced this pull request Jun 1, 2023
**This is an automatic release.**

### New Features
* `Offline Entitlements`: use offline-computed `CustomerInfo` when
server is down (#2368) via NachoSoto (@NachoSoto)

### Bugfixes
* `AppleReceipt.debugDescription`: don't pretty-print JSON (#2564) via
NachoSoto (@NachoSoto)
* `SK2StoreProduct`: fix crash on iOS 12 (#2565) via NachoSoto
(@NachoSoto)
* `GetCustomerInfo` posts receipts if there are pending transactions
(#2533) via NachoSoto (@NachoSoto)
### Performance Improvements
* `PurchasedProductsFetcher`: cache current entitlements (#2507) via
NachoSoto (@NachoSoto)
* Performance: new check to ensure serialization / deserialization is
done from background thread (#2496) via NachoSoto (@NachoSoto)
### Dependency Updates
* Bump fastlane from 2.212.2 to 2.213.0 (#2544) via dependabot[bot]
(@dependabot[bot])
### Other Changes
* `CustomerInfoManager`: post all unfinished transactions (#2563) via
NachoSoto (@NachoSoto)
* `PostReceiptOperation`: added ability to also post `AdServices` token
(#2566) via NachoSoto (@NachoSoto)
* `Offline Entitlements`: improved computation log (#2562) via NachoSoto
(@NachoSoto)
* Added `TransactionPoster` tests (#2557) via NachoSoto (@NachoSoto)
* Refactored `TransactionPoster`: removed 2 dependencies and abstracted
parameters (#2542) via NachoSoto (@NachoSoto)
* `CustomerInfoManagerTests`: wait for `getAndCacheCustomerInfo` to
finish (#2555) via NachoSoto (@NachoSoto)
* `StoreTransaction`: implemented `description` (#2556) via NachoSoto
(@NachoSoto)
* `Backend.ResponseHandler` is now `@Sendable` (#2541) via NachoSoto
(@NachoSoto)
* Extracted `TransactionPoster` from `PurchasesOrchestrator` (#2540) via
NachoSoto (@NachoSoto)
* `enableAdServicesAttributionTokenCollection`: added integration test
(#2551) via NachoSoto (@NachoSoto)
* `AttributionPoster`: replaced hardcoded strings with constants (#2548)
via NachoSoto (@NachoSoto)
* `DefaultDecodable`: moved to `Misc/Codable/DefaultDecodable.swift`
(#2528) via NachoSoto (@NachoSoto)
* `CircleCI`: specify device to run `backend_integration_tests` (#2547)
via NachoSoto (@NachoSoto)
* Created `StoreKit2TransactionFetcher` (#2539) via NachoSoto
(@NachoSoto)
* Fix load shedder integration tests (#2546) via Josh Holtz
(@joshdholtz)
* Fix doc on `Offering.getMetadataValue` (#2545) via Josh Holtz
(@joshdholtz)
* Extracted and tested `AsyncSequence.extractValues` (#2538) via
NachoSoto (@NachoSoto)
* `Offline Entitlements`: don't compute offline `CustomerInfo` when
purchasing a consumable products (#2522) via NachoSoto (@NachoSoto)
* `OfflineEntitlementsManager`: disable offline `CustomerInfo` in
observer mode (#2520) via NachoSoto (@NachoSoto)
* `BasePurchasesTests`: fixed leak detection (#2534) via NachoSoto
(@NachoSoto)
* `PurchaseTesterSwiftUI`: added `ProxyView` to `iOS` (#2531) via
NachoSoto (@NachoSoto)
* `PurchasedProductsFetcher`: removed `AppStore.sync` call (#2521) via
NachoSoto (@NachoSoto)
* `PurchaseTesterSwiftUI`: added new window on Mac to manage proxy
(#2518) via NachoSoto (@NachoSoto)
* `PurchasedProductsFetcher`: added log if fetching purchased products
is slow (#2515) via NachoSoto (@NachoSoto)
* `Offline Entitlements`: disable for custom entitlements mode (#2509)
via NachoSoto (@NachoSoto)
* `Offline Entitlements`: fixed iOS 12 tests (#2514) via NachoSoto
(@NachoSoto)
* `PurchasedProductsFetcher`: don't throw errors if purchased products
were found (#2506) via NachoSoto (@NachoSoto)
* `Offline Entitlements`: allow creating offline `CustomerInfo` with
empty `ProductEntitlementMapping` (#2504) via NachoSoto (@NachoSoto)
* `Offline Entitlements`: integration tests (#2501) via NachoSoto
(@NachoSoto)
* `CustomerInfoManager`: don't cache offline `CustomerInfo` (#2378) via
NachoSoto (@NachoSoto)
* `DangerousSettings`: debug-only `forceServerErrors` (#2486) via
NachoSoto (@NachoSoto)
* `CocoapodsInstallation`: fixed `Xcode 14.3.0` issue (#2489) via
NachoSoto (@NachoSoto)
* `CarthageInstallation`: removed workaround (#2488) via NachoSoto
(@NachoSoto)

---------

Co-authored-by: NachoSoto <ignaciosoto90@gmail.com>
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.

3 participants