Save SDK configuration on paywall activity so it can reconfigure automatically by tonidero · Pull Request #1872 · RevenueCat/purchases-android

tonidero · 2024-10-07T15:19:36Z

Description

In here we store the latest known state for Purchases so we can reconfigure when displaying the PaywallActivity and the SDK is not configured. This may happen if the app is killed while displaying the PaywallActivity, the developer is not configuring in the application but on an activity and the user returns through the app through the recent apps menu.

Additionally, it includes the changes from #1866 so we avoid reconfiguring the SDK if the configuration parameters are the same.

…matically

JayShortway

Pretty cool! Some suggestions, but this should be a nice improvement!

JayShortway · 2024-10-07T15:31:25Z

    }

+    fun getLatestConfiguration(): PurchasesConfiguration {
+        return configuration.copy(appUserID = this.appUserID)


We could also make configuration always be the latest? Otherwise we might get confused between configuration and getLatestConfiguration(), and when to use which.

yeah good point... can probably clean this up a bit.

JayShortway · 2024-10-07T15:45:01Z

+
+    private fun getSdkConfigArgs(savedInstanceState: Bundle): SdkConfigArgs? {
+        return if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) {
+            savedInstanceState.getParcelable(SDK_CONFIG_EXTRA, SdkConfigArgs::class.java)


I always found the warning in the docs for this one a bit scary:

Warning: the class that implements Parcelable has to be the immediately enclosing class of the runtime type of its CREATOR field (that is, Class.getEnclosingClass() has to return the parcelable implementing class), otherwise this method might throw an exception. If the Parcelable class does not enclose the CREATOR, use the deprecated getParcelable(String) instead.

We should be good, but we can also just use the deprecated variant to avoid it altogether. If we ever change things around in the future, we might run into this exception.

JayShortway · 2024-10-07T15:46:12Z

+    private fun configureSdkWithSavedData(savedInstanceState: Bundle) {
+        val sdkConfigArgs = getSdkConfigArgs(savedInstanceState)
+        if (sdkConfigArgs == null) {
+            Logger.e("TEST: Missing SDK configuration arguments")


Good log, but maybe without the "TEST"? 😛

😅 Yeah my bad, I didn't clean it up :P

tonidero · 2024-10-07T15:20:23Z

    }

    override fun hashCode(): Int {
-        var result = context.hashCode()


I think we can't compare the context nor the ExecutorService to do this since they can potentially be different. Wdyt?

I think so too. For the purpose of checking whether the configuration set in PaywallActivity is the same as one set in a different Activity, we can ignore the Context and ExecutorService in the equality check. I don't know if we're doing any other equality checks though, or are intending to do so in the future?

tonidero · 2024-10-07T16:05:22Z

+    private fun configureSdkWithSavedData(savedInstanceState: Bundle) {
+        val sdkConfigArgs = getSdkConfigArgs(savedInstanceState)
+        if (sdkConfigArgs == null) {
+            Logger.e("TEST: Missing SDK configuration arguments")


😅 Yeah my bad, I didn't clean it up :P

tonidero · 2024-10-07T16:07:20Z

    }

+    fun getLatestConfiguration(): PurchasesConfiguration {
+        return configuration.copy(appUserID = this.appUserID)


yeah good point... can probably clean this up a bit.

tonidero · 2024-10-08T08:46:47Z

I changed the base to main so it also includes the changes in #1866, since I think it's going to be easier to review all together.

tonidero · 2024-10-08T08:48:10Z

        }

+    val latestConfiguration: PurchasesConfiguration
+        get() = if (initialConfiguration.appUserID == null) {


I had to add this condition... Basically, the condition to avoid reconfiguration was never wrong if the developer doesn't set an appUserId. In this case, we should just reuse the last configured user id, so it should be safe.

codecov · 2024-10-08T09:26:12Z

Codecov Report

Attention: Patch coverage is 50.00000% with 23 lines in your changes missing coverage. Please review.

Project coverage is 82.23%. Comparing base (256b7ee) to head (e66ce8b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...com/revenuecat/purchases/PurchasesConfiguration.kt	47.50%	15 Missing and 6 partials ⚠️
.../com/revenuecat/purchases/PurchasesOrchestrator.kt	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1872      +/-   ##
==========================================
- Coverage   82.41%   82.23%   -0.19%     
==========================================
  Files         226      226              
  Lines        7915     7958      +43     
  Branches     1105     1119      +14     
==========================================
+ Hits         6523     6544      +21     
- Misses        952      967      +15     
- Partials      440      447       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JayShortway

Looks good! Just had 2 more minor suggestions.

JayShortway · 2024-10-08T11:05:39Z

+        get() = if (initialConfiguration.appUserID == null) {
+            initialConfiguration
+        } else {
+            initialConfiguration.copy(appUserID = this.appUserID)


We're now creating new copies every time this is read. Could we keep the up-to-date configuration in a backing var maybe? We can update it whenever the appUserId changes.

Hmm I've been thinking whether this would be more work right now to calculate on user change or as it is now, when there is a configuration or the PaywallActivity's onSaveInstanceState is called... And I think it might be less work as it is right now? Normally users only configure once, so this won't be called, but there is a bigger chance of logging a different user (though also an uncommon case)... In any case I think we might be trying to early optimizing and I prefer to calculate it on the fly rather than trying to cache the value which might be outdated later... Lmk if you think otherwise!

Yea this is less error prone, and the performance difference is probably negligible. I'm okay with it!

JayShortway · 2024-10-08T11:09:31Z

    }
+
+    @Test
+    fun `Calling configure multiple times with same configuration does not create a new instance`() {


Nice tests! 🙌

JayShortway · 2024-10-08T11:14:43Z

+        Purchases.configure(
+            PurchasesConfiguration.Builder(this, sdkConfigArgs.apiKey)
+                .appUserID(sdkConfigArgs.appUserId)
+                .purchasesAreCompletedBy(sdkConfigArgs.purchasesAreCompletedBy)
+                .showInAppMessagesAutomatically(sdkConfigArgs.showInAppMessagesAutomatically)
+                .store(sdkConfigArgs.store)
+                .diagnosticsEnabled(sdkConfigArgs.diagnosticsEnabled)
+                .entitlementVerificationMode(sdkConfigArgs.verificationMode)
+                .dangerousSettings(sdkConfigArgs.dangerousSettings)
+                .pendingTransactionsForPrepaidPlansEnabled(sdkConfigArgs.pendingTransactionsForPrepaidPlansEnabled)
+                .build(),
+        )


If we ever add a configuration option, we have to remember to add it to SdkConfigArgs. Can't really think of a solution though haha (other than some custom Detekt stuff or something).

Yeah... it's not great but we are missing the mandatory Context parameter so can't think of a good solution either 😞

@tonidero

**This is an automatic release.** ## RevenueCat SDK ### 🐞 Bugfixes * Save SDK configuration on paywall activity so it can reconfigure automatically (#1872) via Toni Rico (@tonidero) ### 📦 Dependency Updates * Bump fastlane from 2.223.1 to 2.224.0 (#1870) via dependabot[bot] (@dependabot[bot]) * Bump fastlane-plugin-revenuecat_internal from `5b2e35c` to `3b1e7cf` (#1865) via dependabot[bot] (@dependabot[bot]) * Bump fastlane from 2.222.0 to 2.223.1 (#1860) via dependabot[bot] (@dependabot[bot]) * Bump fastlane-plugin-revenuecat_internal from `55a0455` to `5b2e35c` (#1858) via dependabot[bot] (@dependabot[bot]) ### 🔄 Other Changes * [CustomerCenter] Fix help path deserializing when unknown type (#1869) via Toni Rico (@tonidero) * [CustomerCenter] Create `CustomerCenter` composable and view model with some initial UI (#1867) via Toni Rico (@tonidero) * [CustomerCenter] Add networking layer and models (#1859) via Toni Rico (@tonidero) * [CustomerCenter] Adds SubscriptionDetailsView (#1863) via JayShortway (@JayShortway) Co-authored-by: revenuecat-ops <ops@revenuecat.com>

### Motivation & Description This is a follow-up of #1872 We're getting some errors like this one in flutter: ``` Purchases$Companion.getSharedInstance pf.K - There is no singleton instance. Make sure you configure Purchases before trying to get the default instance. ``` The theory is that this might be related to a similar bug we fixed for paywalls of the app getting backgrounded and killed while on the CustomerCenter activity, and then crashing when coming back

tonidero added 4 commits October 7, 2024 16:42

Avoid recreating Purchases instance if same config

1857333

Detekt fixes

8ab9ad7

Make sure we only keep a reference to the application context

5b57b16

Save SDK configuration on paywall activity so it can reconfigure auto…

0d83b64

…matically

tonidero added the pr:fix A bug fix label Oct 7, 2024

tonidero mentioned this pull request Oct 7, 2024

Fix crash on PaywallActivity if app restarted #1871

Closed

JayShortway reviewed Oct 7, 2024

View reviewed changes

tonidero commented Oct 7, 2024

View reviewed changes

tonidero added 3 commits October 8, 2024 10:19

Some fixes from review

e444dca

Add API tests

ffe1002

Some fixes and reorganization

0ce454d

tonidero changed the base branch from avoid-recreating-purchases-same-config to main October 8, 2024 08:46

tonidero commented Oct 8, 2024

View reviewed changes

Fix some failing tests

1d59b66

tonidero marked this pull request as ready for review October 8, 2024 08:57

tonidero requested review from a team and JayShortway October 8, 2024 08:57

JayShortway approved these changes Oct 8, 2024

View reviewed changes

Rename to currentConfiguration

e66ce8b

tonidero merged commit f889c76 into main Oct 8, 2024

tonidero deleted the save-sdk-configuration-reconfigure-paywall-activity branch October 8, 2024 15:55

tonidero mentioned this pull request Oct 8, 2024

Avoid recreating purchases same config #1866

Closed

RCGitBot mentioned this pull request Oct 9, 2024

[AUTOMATIC] Release/8.8.1 #1876

Merged

facumenzella mentioned this pull request Nov 24, 2025

Restore Purchases config automatically in CustomerCenter #2867

Merged

Uh oh!

Conversation

tonidero commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

JayShortway left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero commented Oct 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JayShortway left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tonidero commented Oct 7, 2024 •

edited

Loading

codecov Bot commented Oct 8, 2024 •

edited

Loading