Fix for Amazon purchase dialog not showing up #552

vegaro · 2022-06-24T13:43:26Z

purchases-android has to be initialized on application onCreate otherwise the purchasing dialog for Amazon doesn’t show up in production.

The reason why this happens is because when initiating a purchase, the Amazon app store checks if there’s a UI to show the dialog on top of. We know this by looking at the logs. When an Activity starts, there’s a log indicating that the onCreate of the Activity is called, same for the onDestroy . They probably have an implementation similar to what’s suggested in https://medium.com/the-devops-corner/how-to-detect-android-app-foreground-status-c9443ddef260

We used to have an implementation similar to that one in our SDK to determine if the app was on foreground. We removed it and started using androidx Lifecycle because that implementation has a big flaw: it only works if the Activity lifecycle listener is added when the Application onCreate is called. It relies on listening to the onCreate of an Activity to be called to determine if the app is in foreground. If the Activity lifecycle listener is added after the Activity onCreate is called, it will not detect the app is in foreground (since onCreate has already be called).

If developers call configure (which is where we set the Amazon listener and where Amazon will start listening to Activity lifecycles) outside of Application’s onCreate and after an Activity onCreate has been called, Amazon will think the app is not in foreground and will not show the purchase dialog in production. For some reason it works when the app is executed in debug mode. I think the reason is that the code to check if the app is in foreground lives in the Amazon Appstore, and Amazon App Tester doesn’t perform the check, because I don’t see any logs.

One solution, is to be very clear that the SDK has to be configured in Application’s onCreate. The problem is that in hybrids developers don’t have easy access to the Application onCreate.

Another solution would be to open a transparent Activity when performing the purchase. This is basically what Google does with ProxyBillingActivity https://github.com/DimaDake/billing/blob/ea2fe60d54832dc8890bd4857841808b13afb9e5/library/src/main/java/com/android/billingclient/api/ProxyBillingActivity.java

tonidero

I had another idea about using a BroadcastReceiver to communicate with the new activity once the purchase ends... I was wondering, can we obtain a reference to the application context from here? If so, we could potentially use the application context to send the broadcast and have the new activity listen to that using LocalBroadcastManager. That way we don't need to hold a reference to the original activity. We do need to access the application but that should be easier (hopefully). What do you think?

feature/amazon/src/main/java/com/revenuecat/purchases/amazon/ProxyAmazonBillingActivity.kt

feature/amazon/src/main/java/com/revenuecat/purchases/amazon/AmazonBilling.kt

tonidero · 2022-06-24T14:36:37Z

feature/amazon/src/main/java/com/revenuecat/purchases/amazon/handler/PurchaseHandler.kt

+                synchronized(this@PurchaseHandler) {
+                    val requestId = resultData?.get("request_id") as? RequestId
+                    if (requestId != null) {
+                        purchaseCompletedCallbacks[requestId] = onPurchaseCompleted


So this suggests that we support having multiple purchase requests initiated at the same time, but considering we are using the same PROXY_AMAZON_BILLING_ACTIVITY_REQUEST_CODE, we only really support one (and I think that's fine). So probably we could refactor this to allow only 1 purchase to be initiated at the same time? If we do that, we won't need to be passing a callback and we can call purchasingServiceProvider.onPurchaseCompleted(activity) on the onPurchaseResponse. We would still need to hold a reference to the activity here (same as we are doing now, inside the callback), we can make it a WeakReference... (not a fan of this either...)

That's true... We actually error out before getting to this point indicating there's another purchase in progress

vegaro · 2022-06-24T14:56:39Z

@tonidero I like the BroadcastReceiver idea. We have the application context. We use it for registering the Amazon listener in AmazonBilling, and we actually hold it in AmazonBilling.

...ure/amazon/src/main/java/com/revenuecat/purchases/amazon/DefaultPurchasingServiceProvider.kt

feature/amazon/src/main/java/com/revenuecat/purchases/amazon/AmazonBilling.kt

tonidero · 2022-06-25T12:16:55Z

I was thinking about something we should test with this approach of starting a custom activity while the purchase process is ongoing. That is, make sure the theme of the original activity is respected, or at least that it doesn't do anything visually funky. For example, if the original activity is in full screen mode, it's possible we leave full screen when we start this proxy activity, and depending how the original activity is setup, it's possible we don't return to full screen after the purchase is completed/cancelled/errored (for example if we go to full screen on the original activity onCreate which won't get called when going back to that activity or on a specific part of that activity which doesn't get called in the onResume).

I'm thinking when we start the new activity, we might have to pass some flags and the theme to the new activity so we can follow the same looks, but then again I think we should test this to make sure it's a problem.

vegaro · 2022-06-27T09:23:18Z

Leaving this here https://developer.android.com/topic/libraries/app-startup. I think with this we can hook to app startup using that Google library. Then we could register the Amazon listener when the app initializes, which will make sure the Amazon listener is registered before any Activity is created. The problem is that involves SO MUCH refactor since in order to register the listener we need to have initialized the listener first which right now is AmazonBilling and has so many dependencies, plus we don't know if the developers want to configure for the Amazon App store until they call configure

tonidero · 2022-06-27T10:17:42Z

Good find! I hadn't thought about that one. I didn't know it allows libraries to configure startup things as well.

we don't know if the developers want to configure for the Amazon App store until they call configure

We could kind of assume they do if they include the amazon sdk dependency I guess? It's not great to do those assumptions though, so not sure if we would have to add some config files to allow to disable it... (again, not a great option).

Having said that, I haven't used any libraries that use that google library afaik, so not sure how well that would work. I know starting a proxy activity as we were planning to do can potentially cause some issues in specific situations, so I don't like that option much either. If I had to vote for one approach, I feel using this library would be less hacky, but I'm not familiar with how much refactor we would have to do to make it work. If it's too much as you say, it might make sense to try with the proxy activity approach first, since we were close and see if we indeed have any issues with the theming and/or full screen modes.

vegaro · 2022-06-27T12:32:34Z

Regarding the theming issues... I am not extremely concerned because Google Billing Library has an ProxyBillingActivity and I copied their manifest. But we should definitely test it thoroughly. I might need some help with that.

vegaro · 2022-06-27T12:42:40Z

I am actually seeing the notification bar going from top to bottom of screen when the activity is terminated 👎

tonidero · 2022-06-27T12:54:53Z

Hmm you mean the status bar right? Could you record a short video to see how it looks? We might be able to fix it with some flags. And yeah not sure if Google's proxy activity sets up these flags as well... They are usually set in the manifest or in the activity itself... And I'm happy to help with testing this, we probably can try to play with the theme in the activity in one of our example apps

vegaro · 2022-06-27T13:10:00Z

@tonidero This issue is basically what I am seeing RevenueCat/cordova-plugin-purchases#11 RevenueCat/purchases-flutter#324

Although I am trying the solution and now I see a black background 🤔 Also... those theme flags are only available for Android 21+ and our min sdk is 14 (Amazon's is also 14)

aboedo · 2022-06-27T14:19:24Z

question about the library: any concerns that introducing that dependency might mean another way for us to have version conflicts for observer mode? I get that it's not particularly widely used, but billing client versioning has been pretty painful in the past

vegaro · 2022-06-28T11:25:58Z

It could definitely introduce incompatibilities. By the way, we managed to fix the status bar animation yesterday, huge props to @tonidero for helping me out. I am going to be testing it today in a Fire Tv to make sure it looks ok and in an Android device with Android 7.0

vegaro · 2022-06-28T16:40:12Z

Ok I finally managed to test in a Fire TV (that was tough) and it works as expected!

# Conflicts: # feature/amazon/build.gradle

feature/amazon/src/main/AndroidManifest.xml

feature/amazon/src/main/java/com/revenuecat/purchases/amazon/ProxyAmazonBillingActivity.kt

tonidero · 2022-06-30T12:49:47Z

...src/main/java/com/revenuecat/purchases/amazon/ProxyAmazonBillingActivityBroadcastReceiver.kt

+import android.content.Context
+import android.content.Intent
+
+internal class ProxyAmazonBillingActivityBroadcastReceiver(private val activity: Activity) : BroadcastReceiver() {


Hmm there is a circular dependency between the broadcast receiver and the activity... I think we shouldn't leak it, but we should run some tests to make sure the activity and receiver gets destroyed as expected.

Yes, I don't like this... Do you think it would be a better idea to make ProxyAmazonBillingActivity implement BroadcastReceiver? That way it will finish itself

I don't think that's possible since BroadcastReceiver is a class, not an interface... Basically we keep running into the need of holding some reference to the proxy activity somewhere to finish it when it's done... Let me think about other possible options...

In this case we could use WeakReference to hold the activity weakly in the broadcast receiver so we ensure there are no memory leaks. Thoughts?

ugh yes, it's a class, I didn't notice. Hmmmm. Yeah I don't think there's any alternative. I am calling unregister in the onDestroy and setting it to null so I don't think it will leak, but who knows.

Done in 2cbd49c

tonidero · 2022-06-30T12:54:31Z

feature/amazon/src/main/java/com/revenuecat/purchases/amazon/ProxyAmazonBillingActivity.kt

+            val sku = intent.getStringExtra("sku")
+            val resultReceiver = intent.getParcelableExtra<ResultReceiver>("result_receiver")
+            val purchasingServiceProvider =
+                intent.getParcelableExtra<PurchasingServiceProvider>("service_provider")


We can add a static method in this activity that creates an intent with the required properties. This is so we centralize here what arguments are needed to start this activity, and also to centralize the extra "keys" (which we should extract to constants). Additionally, we should show a proper error if any argument is missing.

Perfect, I was waiting for some comments around how to extract those keys

Another idea could be to create a ProxyAmazonBillingActivityArgs class that contains all these 3 and make it parcelable, so there is only one argument. That way is slightly safer since the arguments definition is also centralized

I went with the newStartIntent approach which is what I did in the past and I think it fixes the problem. Regarding the error... The arguments are non-nullable. Are you referring to fail if they are missing in the created intent?

Are you referring to fail if they are missing in the created intent?

That's right. We should show a meaningful error if the arguments are missing in the intent.

feature/amazon/src/main/java/com/revenuecat/purchases/amazon/handler/PurchaseHandler.kt

...test/java/com/revenuecat/purchases/amazon/ProxyAmazonBillingActivityBroadcastReceiverTest.kt

tonidero · 2022-06-30T13:13:21Z

feature/amazon/src/test/java/com/revenuecat/purchases/amazon/ProxyAmazonBillingActivityTest.kt

+import org.junit.runner.RunWith
+
+@RunWith(AndroidJUnit4::class)
+class ProxyAmazonBillingActivityTest {


I've usually avoided testing activities or other android UI classes directly like this. In general, I feel that these classes shouldn't hold any logic and if there is any, it should be abstracted to a different class. In this case, we are mostly parsing some parameters from the Intent, starting the purchase and calling one of the parameters as a callback. What do you think about moving all that to a helper class, something like ProxyAmazonBillingHelper, with a startAmazonPurchase method or something like that?

I agree, doing and testing this made me remember why architectures like MVP make sense 😆

I created ProxyAmazonBillingHelper. I didn't move the stuff around ProxyAmazonBillingActivityBroadcastReceiver to that helper because I want to know what you think about #552 (comment)

feature/amazon/src/test/java/com/revenuecat/purchases/amazon/handler/PurchaseHandlerTest.kt

vegaro · 2022-06-30T16:27:56Z

@tonidero I think I've finished addressing your comments. I will fix the tests after your review, in case you think other changes to the architecture should be made.

tonidero

It's looking pretty good! Just some small things. Lmk once it's final and I can give it a final review.

tonidero · 2022-06-30T19:33:38Z

...src/main/java/com/revenuecat/purchases/amazon/ProxyAmazonBillingActivityBroadcastReceiver.kt

+    }
+
+    @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
+    var onReceiveCalled = false


So this is only added for a test... I'm not sure but is there a way of testing whether the activity was finished in the test scenario? If so we could remove this property entirely. If not, I guess we can keep it for better tests.

I tried but I couldn't find anything 😢 I thought about checking if state is ON_DESTROY but it wasn't passing because sendBroadcast is asynchronous. Maybe I can mock that?

It will probably be hard to mock, since it's created by the activity. I think it's ok, we can leave as is 👍

...src/main/java/com/revenuecat/purchases/amazon/ProxyAmazonBillingActivityBroadcastReceiver.kt

vegaro · 2022-07-01T08:27:52Z

feature/amazon/src/main/java/com/revenuecat/purchases/amazon/ProxyAmazonBillingActivity.kt

+        super.onCreate(savedInstanceState)
+
+        proxyAmazonBillingDelegate = ProxyAmazonBillingDelegate()
+        proxyAmazonBillingDelegate?.onCreate(this, savedInstanceState)


@tonidero any ideas on how to test that this is called? I could do some little DI framework that injects a mocked ProxyAmazonBillingDelegate but I think that's too much lol

tonidero · 2022-07-01T15:41:39Z

...mazon/src/main/java/com/revenuecat/purchases/amazon/purchasing/ProxyAmazonBillingDelegate.kt

+
+    fun onCreate(activity: Activity, savedInstanceState: Bundle?) {
+        broadcastReceiver = ProxyAmazonBillingActivityBroadcastReceiver(activity)
+        activity.applicationContext.registerReceiver(broadcastReceiver, filter)


Hmm I noticed we are now registering the receiver in the application context, instead of the ProxyAmazonBillingActivity. I think it would be safer on the activity, since if we leave it on the application, and the onDestroy doesn't get called for any reason, the application can remain registered forever.

tonidero

I think this looks good!

tonidero · 2022-07-01T15:43:54Z

...mazon/src/main/java/com/revenuecat/purchases/amazon/purchasing/ProxyAmazonBillingDelegate.kt

+        val purchasingServiceProvider =
+            intent.getParcelableExtra<PurchasingServiceProvider>(
+                ProxyAmazonBillingActivity.EXTRAS_PURCHASING_SERVICE_PROVIDER
+            )


I was wondering whether getting the arguments from the intent could be done in the activity... But I guess it's fine here so we have that logic available for testing here as well 👍.

tonidero · 2022-07-01T15:49:31Z

...test/java/com/revenuecat/purchases/amazon/ProxyAmazonBillingActivityBroadcastReceiverTest.kt

+
+    @After
+    fun tearDown() {
+        clearAllMocks()


NABD but currently we are reseting all mocks on the @Before block, so this isn't really needed. If we created the mocks and didn't create new ones for each test, then this would be needed.

Oh yes, true, I will remove it

vegaro · 2022-07-01T15:58:47Z

@tonidero I removed the clearAllMocks and moved the ProxyBillingActivity stuff to a purchasing package

adds ProxyAmazonBillingActivity

3de0b41

vegaro force-pushed the amazon-proxy-billing-activity branch from 116d446 to 3de0b41 Compare June 24, 2022 14:44

tonidero reviewed Jun 24, 2022

View reviewed changes

remove finish()

a11a295

aboedo reviewed Jun 24, 2022

View reviewed changes

...ure/amazon/src/main/java/com/revenuecat/purchases/amazon/DefaultPurchasingServiceProvider.kt Outdated Show resolved Hide resolved

feature/amazon/src/main/java/com/revenuecat/purchases/amazon/AmazonBilling.kt Outdated Show resolved Hide resolved

implementation using broadcast receiver

619967e

fix for status bar

4b6faea

vegaro added 9 commits June 29, 2022 14:04

fix AmazonBillingTest

0e93706

fixes more tests

e8c5a3e

savedInstanceState

5aa5306

refactor to pass purchasingserviceprovider so we can test

7d9a3fa

more refactor

51c425b

more tests

94d85a6

fix PurchaseHandlerTest

e1d7863

missing dependencies

7792135

Merge branch 'main' into amazon-proxy-billing-activity

bb5cfeb

# Conflicts: # feature/amazon/build.gradle

vegaro marked this pull request as ready for review June 30, 2022 11:20

vegaro requested a review from tonidero June 30, 2022 11:20

tonidero reviewed Jun 30, 2022

View reviewed changes

vegaro added 3 commits June 30, 2022 16:53

creates WeakReference of Activity

2cbd49c

rename to ProxyAmazonBillingDelegate and moves logic there

28ee907

add error logging

0b147e4

vegaro requested a review from tonidero June 30, 2022 16:28

tonidero reviewed Jun 30, 2022

View reviewed changes

vegaro added 3 commits July 1, 2022 09:28

makes packagename an argument

3cf3048

companion object moved to top

3d03404

makes proxyAmazonBillingDelegate visiblefortesting

3ed4185

vegaro commented Jul 1, 2022

View reviewed changes

vegaro added 4 commits July 1, 2022 10:42

changes activity test to only test creation of delegate

22a4aa3

ProxyAmazonBillingActivityTest fixed

9a17cb2

ProxyAmazonBillingDelegateTest

11dad87

fixes test

bfed36f

vegaro requested a review from tonidero July 1, 2022 13:43

tonidero reviewed Jul 1, 2022

View reviewed changes

registers broadcast in activity context instead of activity

a9f6872

vegaro requested a review from tonidero July 1, 2022 15:48

tonidero approved these changes Jul 1, 2022

View reviewed changes

vegaro added 2 commits July 1, 2022 17:56

Remove clearAllMocks

f8bb051

move to package and another clearAllMocks

252d825

tonidero approved these changes Jul 1, 2022

View reviewed changes

vegaro added 2 commits July 1, 2022 18:11

adds theme back because it broke the transparency

0872ebb

Merge branch 'main' into amazon-proxy-billing-activity

f641be4

vegaro merged commit 15a55dc into main Jul 1, 2022

vegaro deleted the amazon-proxy-billing-activity branch July 1, 2022 16:46

tonidero mentioned this pull request Jul 7, 2022

test-Release/5.2.0 #556

Closed

vegaro mentioned this pull request Jul 12, 2022

Release/5.2.0 #549

Merged

1 task

aboedo mentioned this pull request Jul 13, 2022

Release: 5.2.1 #563

Merged

Fix for Amazon purchase dialog not showing up #552

Fix for Amazon purchase dialog not showing up #552

Conversation

vegaro commented Jun 24, 2022 • edited by jira bot Loading

tonidero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegaro commented Jun 24, 2022

tonidero commented Jun 25, 2022

vegaro commented Jun 27, 2022 • edited Loading

tonidero commented Jun 27, 2022

vegaro commented Jun 27, 2022

vegaro commented Jun 27, 2022

tonidero commented Jun 27, 2022

vegaro commented Jun 27, 2022 • edited Loading

aboedo commented Jun 27, 2022

vegaro commented Jun 28, 2022

vegaro commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegaro Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonidero Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegaro commented Jun 30, 2022

tonidero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonidero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vegaro commented Jul 1, 2022

vegaro commented Jun 24, 2022 •

edited by jira bot

Loading

vegaro commented Jun 27, 2022 •

edited

Loading

vegaro commented Jun 27, 2022 •

edited

Loading

vegaro Jun 30, 2022 •

edited

Loading

tonidero Jun 30, 2022 •

edited

Loading