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

Created CachingTrialOrIntroPriceEligibilityChecker #2007

Merged
merged 8 commits into from
Jan 12, 2023

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Oct 27, 2022

Fixes CSDK-493.

This will help a little bit with the SK2 performance issues computing eligibility (#1893).

@NachoSoto NachoSoto added the perf A code change that improves performance label Oct 27, 2022
@NachoSoto NachoSoto requested a review from a team October 27, 2022 22:58
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!

Sources/FoundationExtensions/AsyncExtensions.swift Outdated Show resolved Hide resolved
@available(iOS 13.0, tvOS 13.0, watchOS 6.2, macOS 10.15, *)
extension TrialOrIntroPriceEligibilityCheckerType {

func checkEligibility(productIdentifiers: [String]) async -> [String: IntroEligibility] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this seems unused other than in tests?

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 actually public API.

Copy link
Contributor

Choose a reason for hiding this comment

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

But that's the completion version, not this one with async 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.

Oooh good catch! I moved this to just the test file.

NachoSoto added a commit that referenced this pull request Nov 4, 2022
…ons`

This changes the default back to `StoreKit 1`. We decided to do this for the following reasons:
- Purchasing with `PromotionalOffer`s does not work with StoreKit 2 due to an Apple bug (see #2020 (comment))
- `checkTrialOrIntroDiscountEligibility` is significantly slower with StoreKit 2 (#1893). We're adding optimizations to help with that (#2007), but the underlying logic will still be slow.
- A rare race-condition where `StoreKit 2` does not have transactions after a purchase ([TRIAGE-82]). We have some workarounds (#1945), but it's still being investigated.

_ Note: This effectively reverts 0ee540a. That commit made it easier to only change the default in one place which is why this PR is basically just one line._
NachoSoto added a commit that referenced this pull request Nov 4, 2022
…ons` (#2022)

This changes the default back to `StoreKit 1`. We decided to do this for
the following reasons:
- Purchasing with `PromotionalOffer`s does not work with StoreKit 2 due
to an Apple bug (see
#2020 (comment))
- `checkTrialOrIntroDiscountEligibility` is significantly slower with
`StoreKit 2` (#1893). We're adding optimizations to help with that
(#2007), but the underlying logic will still be slow.
- A rare race-condition where `StoreKit 2` does not have transactions
after a purchase ([TRIAGE-82]). We have some workarounds (#1945), but
it's still being investigated.

_Note: This effectively reverts 0ee540a. That commit made it easier to
only change the default in one place which is why this PR is basically
just one line._

[TRIAGE-82]:
https://revenuecats.atlassian.net/browse/TRIAGE-82?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
Base automatically changed from trial-checker-cache to main November 7, 2022 20:40
@NachoSoto
Copy link
Contributor Author

Oh wait we need to invalidate this cache when we make purchases or they expire of course 🤦🏻‍♂️
Caching and off-by-one errors, the 3 hardest problems in computer science.

@NachoSoto NachoSoto marked this pull request as draft November 7, 2022 21:17
@NachoSoto NachoSoto requested a review from a team November 7, 2022 21:17
@NachoSoto NachoSoto changed the base branch from main to notification-subscriptions-changed November 15, 2022 18:18
@NachoSoto NachoSoto marked this pull request as ready for review November 15, 2022 18:56
@NachoSoto
Copy link
Contributor Author

NachoSoto commented Nov 15, 2022

This is ready for review again @RevenueCat/coresdk, I'm now clearing the cache when subscriptions change (using #2057), which should fix integration tests.

@NachoSoto NachoSoto force-pushed the notification-subscriptions-changed branch from 3eddf69 to 2f68cc7 Compare November 15, 2022 19:14
@NachoSoto NachoSoto force-pushed the trial-checker-cache-2 branch 2 times, most recently from e348a00 to f632a4a Compare November 15, 2022 19:16
@NachoSoto NachoSoto removed the blocked label Jan 5, 2023
@NachoSoto NachoSoto changed the base branch from notification-subscriptions-changed to main January 5, 2023 22:52
@@ -1235,6 +1238,11 @@ internal extension Purchases {

private extension Purchases {

func handleCustomerInfoChanged(_ customerInfo: CustomerInfo) {
self.trialOrIntroPriceEligibilityChecker.clearCache()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aboedo per our discussion, this ordering now ensures there are no race conditions.

@NachoSoto NachoSoto requested a review from aboedo January 5, 2023 22:55
@NachoSoto
Copy link
Contributor Author

NachoSoto commented Jan 5, 2023

Updated this to clear caches using the existing CustomerInfoManager observation mechanism /cc @aboedo

@NachoSoto NachoSoto merged commit f3a6d20 into main Jan 12, 2023
@NachoSoto NachoSoto deleted the trial-checker-cache-2 branch January 12, 2023 18:20
NachoSoto pushed a commit that referenced this pull request Jan 17, 2023
**This is an automatic release.**

### New Features
* Added new `ReceiptParser.fetchAndParseLocalReceipt` (#2204) via
NachoSoto (@NachoSoto)
* `PurchasesReceiptParser`: added API to parse receipts from `base64`
string (#2200) via NachoSoto (@NachoSoto)
### Bugfixes
* `CustomerInfo`: support parsing schema version 2 to restore SDK `v3.x`
compatibility (#2213) via NachoSoto (@NachoSoto)
### Other Changes
* `JSONDecoder`: added decoding type when logging
`DecodingError.keyNotFound` (#2212) via NachoSoto (@NachoSoto)
* Added `ReceiptParserTests` (#2203) via NachoSoto (@NachoSoto)
* Deploy `PurchaseTester` for `macOS` (#2011) via NachoSoto (@NachoSoto)
* `ReceiptFetcher`: refactored implementation to log error when failing
to fetch receipt (#2202) via NachoSoto (@NachoSoto)
* `PostReceiptDataOperation`: replaced receipt `base64` with `hash` for
cache key (#2199) via NachoSoto (@NachoSoto)
* `PurchaseTester`: small refactor to simplify `Date` formatting (#2210)
via NachoSoto (@NachoSoto)
* `PurchasesReceiptParser`: improved documentation to reference
`default` (#2197) via NachoSoto (@NachoSoto)
* Created `CachingTrialOrIntroPriceEligibilityChecker` (#2007) via
NachoSoto (@NachoSoto)
* Update Gemfile.lock (#2205) via Cesar de la Vega (@vegaro)
* remove stalebot in favor of SLAs in Zendesk (#2196) via Andy Boedo
(@aboedo)
* Update fastlane-plugin-revenuecat_internal to latest version (#2194)
via Cesar de la Vega (@vegaro)
NachoSoto added a commit that referenced this pull request Jul 24, 2023
This will help provide a better experience when launching paywalls, as hopefully we will have already computed intro eligibility by then.
This is even better than when using `StoreKit` paywalls.

Note that thanks to `CachingTrialOrIntroPriceEligibilityChecker` (#2007) this is cached automatically, and also it handles multiple requests with the same or different product identifiers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf A code change that improves performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants