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

Directly send delegate customer info when delegate is set (always sends cached CustomerInfo value) #1828

Merged
merged 4 commits into from
Aug 15, 2022

Conversation

joshdholtz
Copy link
Contributor

@joshdholtz joshdholtz commented Aug 15, 2022

Motivation

Fixes TRIAGE-55

Issue

Delegate would not always/reliably call didReceiveUpdatedCustomerInfo.

customerInfoManager.sendCachedCustomerInfoIfAvailable would only call the delegate when the latest customer info object was already sent.

☝️ This, according to a customer bug report, appears to be a breaking change between v3 and v4. It also appears to be a difference in behavior with Purchases.shared.customerInfoStream as this gets the initial customer value.

Example
@main
struct PurchaseTesterApp: App {
    @UIApplicationDelegateAdaptor(AppDelegate.self) var appDelegate
    @State private var revenueCatCustomerData = RevenueCatCustomerData()
    
    var body: some Scene {
        WindowGroup {
            ContentView()
                .environmentObject(revenueCatCustomerData)
                .task {
                    for await customerInfo in Purchases.shared.customerInfoStream {
                        // ✅ This worked fine before and after change
                        // This will get the initial customerInfo as soon as its set
                        self.revenueCatCustomerData.customerInfo = customerInfo
                        self.revenueCatCustomerData.appUserID = Purchases.shared.appUserID
                    }
                }
        }
    }
}

class AppDelegate: NSObject, UIApplicationDelegate {
    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey : Any]? = nil) -> Bool {

        Purchases.logLevel = .debug
        Purchases.configure(with: Configuration
            .builder(withAPIKey: Constants.apiKey)
            .with(usesStoreKit2IfAvailable: true)
            .build())

        // ❌ This is one way that delegate would not be called right away with customer info
        // There might be other cases too
        // The customerInfo would already have been sent to an async stream and not be forced sent
        // to the new delegate
        DispatchQueue.main.asyncAfter(deadline: .now() + 5) {
            Purchases.shared.delegate = self
        }

        return true
    }
}

extension AppDelegate: PurchasesDelegate {
    func purchases(_ purchases: Purchases, receivedUpdated customerInfo: CustomerInfo) {
        print("New customer info: \(customerInfo)")
    }
}

Bonus Fix

The logs would show CusomterInfo was set to delegate before delegate was set.

2022-08-15 05:19:28.062200-0500 PurchaseTester[10812:71706] [Purchases] - DEBUG: ℹ️ SDK Version - 4.11.0-SNAPSHOT
2022-08-15 05:19:28.062249-0500 PurchaseTester[10812:71706] [Purchases] - DEBUG: 👤 No initial App User ID
2022-08-15 05:19:28.067297-0500 PurchaseTester[10812:71706] [Purchases] - DEBUG: ℹ️ Sending latest CustomerInfo to delegate.
2022-08-15 05:19:30.484881-0500 PurchaseTester[10812:71706] [Purchases] - DEBUG: ℹ️ Delegate set

Description

  • No longer relying on customerInfoManager.sendCachedCustomerInfoIfAvailable to call send a value to the delegate when delegate is being sent
    • Instead, sending initial cached customer info directly to the delegate
  • Moved log of delegate set before potentially sending CustomerInfo (which will also log)

@joshdholtz joshdholtz requested a review from a team August 15, 2022 10:28
@joshdholtz joshdholtz added the refactor A code change that neither fixes a bug nor adds a feature label Aug 15, 2022
@joshdholtz joshdholtz changed the title Log delegate set before sending custoemr info Directly send delegate customer info when delegate is set Aug 15, 2022
@joshdholtz joshdholtz changed the title Directly send delegate customer info when delegate is set Directly send delegate customer info when delegate is set (always get initial CustomerInfo value) Aug 15, 2022
@joshdholtz joshdholtz changed the title Directly send delegate customer info when delegate is set (always get initial CustomerInfo value) Directly send delegate customer info when delegate is set (always sends cached CustomerInfo value) Aug 15, 2022
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.

talked about it on a call, the fix is solid.
We should:

  • write a ticket to make a failing test (Josh will open that ticket up)
  • clean up a little bit, adding comments to explain why we're doing this force-update when setting the delegate

@joshdholtz
Copy link
Contributor Author

@aboedo Updated with new method comments! Do you mind taking another look to make sure they make sense? 😇

@joshdholtz joshdholtz merged commit 5c15b5c into main Aug 15, 2022
@joshdholtz joshdholtz deleted the delegate-set-log-before-sending-customer-info branch August 15, 2022 13:59
joshdholtz added a commit that referenced this pull request Aug 15, 2022
…ds cached CustomerInfo value) (#1828)

* Log delegate set before sending custoemr info

* Force call delegate with customer info when being set

* Committed wrong file

* Moved to own method and added comments
@joshdholtz joshdholtz mentioned this pull request Aug 15, 2022
@NachoSoto
Copy link
Contributor

This makes sense. I agree with @aboedo that a failing test would be nice.

NachoSoto added a commit that referenced this pull request Aug 15, 2022
…s being sent

Fixes [CSDK-399]. Follow up to #1828.

Also changed the implementation to remove the `DispatchQueue.async` which wasn't necessary. I think it's reasonable to send the call on the same thread that the `delegate` was set.
@NachoSoto
Copy link
Contributor

Tests: #1830

@NachoSoto NachoSoto added the fix A bug fix label Aug 15, 2022
NachoSoto added a commit that referenced this pull request Aug 15, 2022
…s being sent (#1830)

Fixes [CSDK-399]. Follow up to #1828.

Also changed the implementation to remove the `DispatchQueue.async` which wasn't necessary. I think it's reasonable to send the call on the same thread that the `delegate` was set.

[CSDK-399]: https://revenuecats.atlassian.net/browse/CSDK-399?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
@NachoSoto NachoSoto mentioned this pull request Aug 17, 2022
NachoSoto added a commit that referenced this pull request Aug 19, 2022
### Bugfixes
* `ErrorResponse`: don't add attribute errors to message if empty (#1844) via NachoSoto (@NachoSoto)
* Purchase cancellations: unify behavior between SK1 and SK2 (#1841) via NachoSoto (@NachoSoto)
* StoreKit 2: `PurchasesOrchestrator`: don't log "purchased product" if it was cancelled (#1840) via NachoSoto (@NachoSoto)
* `Backend`: fixed potential race conditions introduced by `OperationDispatcher.dispatchOnWorkerThread(withRandomDelay:)` (#1827) via NachoSoto (@NachoSoto)
* `DeviceCache`: `Sendable` conformance and fixed thread-safety (#1823) via NachoSoto (@NachoSoto)
* Directly send delegate customer info when delegate is set (always sends cached CustomerInfo value) (#1828) via Josh Holtz (@joshdholtz)
* `SystemInfo.finishTransactions`: made thread-safe (#1807) via NachoSoto (@NachoSoto)
* `Purchases.shared` and `Purchases.isConfigured` are now thread-safe (#1813) via NachoSoto (@NachoSoto)
* `PriceFormatterProvider: Sendable` conformance and fixed thread-safety (#1818) via NachoSoto (@NachoSoto)
* `StoreKitConfigTestCase.changeStorefront`: re-enabled on iOS 16 (#1811) via NachoSoto (@NachoSoto)

### Other Changes
* `DeviceCache`: no longer set cache timestamp before beginning request (#1839) via NachoSoto (@NachoSoto)
* `MagicWeatherSwiftUI`: updated to use `async` APIs (#1843) via NachoSoto (@NachoSoto)
* Release train (#1842) via Cesar de la Vega (@vegaro)
* Adds hotfixes section to RELEASING doc (#1837) via Cesar de la Vega (@vegaro)
* Update fastlane plugin (#1838) via Toni Rico (@tonidero)
* Update migration doc from didReceiveUpdatedCustomerInfo to receivedUpdatedCustomerInfo (#1836) via Josh Holtz (@joshdholtz)
* `PurchasesDelegate`: added test for latest cached customer info always being sent (#1830) via NachoSoto (@NachoSoto)
* `CallbackCache: Sendable` conformance (#1835) via NachoSoto (@NachoSoto)
* `CallbackCache`: simplified implementation using `Atomic` (#1834) via NachoSoto (@NachoSoto)
* `PurchasesLogInTests`: added test to verify `logIn` updates offerings cache (#1833) via NachoSoto (@NachoSoto)
* Created `PurchasesLoginTests` (#1832) via NachoSoto (@NachoSoto)
* `SwiftLint`: cleaned up output (#1821) via NachoSoto (@NachoSoto)
* Link to sdk reference (#1831) via aboedo (@aboedo)
* `Atomic: ExpressibleByBooleanLiteral` (#1822) via NachoSoto (@NachoSoto)
* `SwiftLint`: fixed build warning (#1820) via NachoSoto (@NachoSoto)
* Adds an approval job that will tag the release (#1815) via Cesar de la Vega (@vegaro)
* `Atomic: ExpressibleByNilLiteral` (#1804) via NachoSoto (@NachoSoto)
* `PurchasesAttributionDataTests`: fixed potential race condition in flaky test (#1805) via NachoSoto (@NachoSoto)
* Fixed warnings for unnecessary `try` (#1816) via NachoSoto (@NachoSoto)
* Moved `AttributionFetcherError` inside `AttributionFetcher` (#1808) via NachoSoto (@NachoSoto)
* Update documentation for presentCodeRedemptionSheet (#1817) via Joshua Liebowitz (@taquitos)
* `Dangerfile`: added "next_release" as supported label (#1810) via NachoSoto (@NachoSoto)
* PurchaseTester- Update Podfile.lock (#1814) via Joshua Liebowitz (@taquitos)
* Update to latest fastlane plugin (#1802) via Toni Rico (@tonidero)
* Clean up: moved `BackendIntegrationTests.xctestplan` to `TestPlans` folder (#1812) via NachoSoto (@NachoSoto)
* `SK2StoreProduct`: conditionally removed `@available` workaround (#1794) via NachoSoto (@NachoSoto)
* `SwiftLint`: fixed deprecation warning (#1809) via NachoSoto (@NachoSoto)
* Update gems (#1791) via Joshua Liebowitz (@taquitos)
* Replace usages of replace_in with replace_text_in_files action (#1803) via Toni Rico (@tonidero)
@revenuecat-ops revenuecat-ops mentioned this pull request Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A bug fix 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