Paywalls: Support sending paywall events to servers (2) by tonidero · Pull Request #1442 · RevenueCat/purchases-android

tonidero · 2023-11-07T11:52:59Z

Description

This follows up on #1436.

Here we are adding the code to send paywall events to our servers. This is not connected with the work done in #1436 yet. That will be done in followup PRs.

Actually read events from disk and use this to send them to servers.
Use paywall events to detect whether a purchase came from a paywall.

tonidero · 2023-11-07T11:55:48Z

    @Volatile var diagnosticsCallbacks = mutableMapOf<CallbackCacheKey, MutableList<DiagnosticsCallback>>()

+    @get:Synchronized @set:Synchronized
+    @Volatile var paywallEventsCallbacks = mutableMapOf<CallbackCacheKey, MutableList<PaywallEventsCallback>>()


We might want to think about refactoring the backend class. It's pretty huge now, and it has a lot of duplicated logic for each request... For now, I created a Linear ticket for this.

👍🏻 yeah long overdue separation as we did in iOS

tonidero · 2023-11-07T11:57:03Z

+            backend.postPaywallEvents(
+                request,
+                onSuccessHandler = {
+                    latch.countDown()


Here we're basically only making sure the request succeeds, since there is not much to check otherwise I think

We use snapshot tests in iOS to verify the request format. Would it be possible to at least verify the body of the request?

I do this in the unit tests, not the backend integration tests: https://github.com/RevenueCat/purchases-android/pull/1442/files#diff-611e526013c5cd77f7141eb4ece99c74f81ecd9477bf84bf0f528e506ad40199R95. Lmk if you think we should still do that here... Might be tricky to intercept though, so I kinda think this is enough

codecov · 2023-11-07T13:11:02Z

Codecov Report

Attention: Patch coverage is 63.10680% with 38 lines in your changes missing coverage. Please review.

Project coverage is 83.67%. Comparing base (4982a45) to head (dfdeb00).
Report is 312 commits behind head on main.

Files with missing lines	Patch %	Lines
...at/purchases/paywalls/events/PaywallStoredEvent.kt	0.00%	12 Missing ⚠️
.../kotlin/com/revenuecat/purchases/common/Backend.kt	76.92%	6 Missing and 3 partials ⚠️
...t/purchases/paywalls/events/PaywallBackendEvent.kt	60.00%	7 Missing and 1 partial ⚠️
...evenuecat/purchases/utils/JsonElementExtensions.kt	68.18%	4 Missing and 3 partials ⚠️
...t/purchases/common/networking/RCHTTPStatusCodes.kt	0.00%	0 Missing and 1 partial ⚠️
...t/purchases/paywalls/events/PaywallEventRequest.kt	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1442      +/-   ##
==========================================
- Coverage   83.99%   83.67%   -0.33%     
==========================================
  Files         203      206       +3     
  Lines        6717     6816      +99     
  Branches      975      990      +15     
==========================================
+ Hits         5642     5703      +61     
- Misses        698      727      +29     
- Partials      377      386       +9

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

…ark events as synced or not

NachoSoto · 2023-11-08T16:42:11Z

    @Volatile var diagnosticsCallbacks = mutableMapOf<CallbackCacheKey, MutableList<DiagnosticsCallback>>()

+    @get:Synchronized @set:Synchronized
+    @Volatile var paywallEventsCallbacks = mutableMapOf<CallbackCacheKey, MutableList<PaywallEventsCallback>>()


👍🏻 yeah long overdue separation as we did in iOS

NachoSoto · 2023-11-08T16:44:36Z

+                )
+            }
+
+            override fun onError(error: PurchasesError) {


Also outside the scope of this PR, but there is a lot of repetition on how we make requests as well that would be useful to abstract.

Absolutely agree.

NachoSoto · 2023-11-08T16:47:12Z

+                    appConfig.paywallEventsURL,
+                    Endpoint.PostPaywallEvents,


It took me a while to find where this was defined. I feel like paywallEventsURL should be part of the Endpoint configuration. What do you think? Otherwise it would be possible (at compile time) to make a request to conflicting endpoints / domains.

Hmm we could certainly move it there, but I also feel with the upcoming work for adding fallback base URLs, it might be better to keep separate... Honestly, I could see it going either way.

I'm gonna hold on this until we actually rewrite the networking code to not increase the size of this PR even more.

Sounds good, and that's a good point.

NachoSoto · 2023-11-08T16:50:13Z

+    // Note: this means that all 4xx (except 404) are considered as successfully synced.
+    // The reason is because it's likely due to a client error, so continuing to retry
+    // won't yield any different results and instead kill pandas.
+    fun isSynced(statusCode: Int) = isSuccessful(statusCode) || !(isServerError(statusCode) || statusCode == NOT_FOUND)


I'm surprised this didn't exist yet. We use it in iOS for marking attributes as synced.

Hmm that's slightly different in Android for subscriber attributes... There, we would mark them as synced if it's not a server error and we receive a backend error code 24 (UnsupportedError), but we don't mark them as synced with 404... I guess we could add that as well?

NachoSoto · 2023-11-08T16:51:29Z

+import kotlinx.serialization.json.jsonPrimitive
+import kotlinx.serialization.json.longOrNull
+
+fun JsonElement.asMap(): Map<String, Any?>? {


This can be internal?

🤦 Yup thanks!

NachoSoto · 2023-11-08T16:52:20Z

+            backend.postPaywallEvents(
+                request,
+                onSuccessHandler = {
+                    latch.countDown()


We use snapshot tests in iOS to verify the request format. Would it be possible to at least verify the body of the request?

NachoSoto · 2023-11-08T16:52:49Z

+    @Test
+    fun `Paywall events has correct path`() {
+        val endpoint = Endpoint.PostPaywallEvents
+        val expectedPath = "/events"
+        assertThat(endpoint.getPath()).isEqualTo(expectedPath)
+    }
+


Ideally this verifies the domain too if/when that's part of Endpoint.

NachoSoto · 2023-11-08T16:53:16Z

            every { baseURL } returns URL("https://api.revenuecat.com")
+            every { paywallEventsURL } returns URL("https://api-paywalls.revenuecat.com/")


This could use AppConfig instead of duplicating it?

Yeah, just have to expose it, but might be better.

@NachoSoto

**This is an automatic release.** ### RevenueCatUI * `Paywalls`: improve error log when images fail to load (#1454) via NachoSoto (@NachoSoto) ### Other Changes * Paywall events: Send paywall data with post receipt requests (#1452) via Toni Rico (@tonidero) * Paywalls: Track paywall events (#1447) via Toni Rico (@tonidero) * Paywall events: Handle errors parsing specific paywall event lines (#1451) via Toni Rico (@tonidero) * Paywalls: Move logic for events file helper to common generic class (4) (#1446) via Toni Rico (@tonidero) * Paywalls: Add paywall events flush logic and tests (3) (#1445) via Toni Rico (@tonidero) * Paywalls: Support sending paywall events to servers (2) (#1442) via Toni Rico (@tonidero) * `CircleCI`: fix `record-revenuecatui-snapshots` (#1455) via NachoSoto (@NachoSoto) * Lower request jitter log level from warning to debug (#1453) via Toni Rico (@tonidero) Co-authored-by: revenuecat-ops <ops@revenuecat.com>

Support sending paywall events to servers

5a813f2

tonidero added refactor pr:RevenueCatUI labels Nov 7, 2023

Revert unintended changes

3575c2d

tonidero force-pushed the toniricodiez/pwl-321-paywall-events-2 branch from db7f1b9 to 3575c2d Compare November 7, 2023 12:02

tonidero commented Nov 7, 2023

View reviewed changes

Fix tests

d0e558c

tonidero force-pushed the toniricodiez/pwl-321-paywall-events-2 branch from 3f175fb to d0e558c Compare November 7, 2023 12:55

tonidero marked this pull request as ready for review November 7, 2023 12:55

tonidero requested a review from a team November 7, 2023 12:55

Add property to backend error callback indicating whether we should m…

de9b279

…ark events as synced or not

tonidero force-pushed the toniricodiez/pwl-321-paywall-events-2 branch from 316e913 to de9b279 Compare November 7, 2023 13:29

NachoSoto reviewed Nov 8, 2023

View reviewed changes

Some changes from review

dfdeb00

tonidero requested a review from NachoSoto November 9, 2023 10:46

NachoSoto approved these changes Nov 9, 2023

View reviewed changes

tonidero merged commit 530b61b into main Nov 10, 2023

tonidero deleted the toniricodiez/pwl-321-paywall-events-2 branch November 10, 2023 08:53

tonidero removed the pr:RevenueCatUI label Nov 10, 2023

RCGitBot mentioned this pull request Nov 10, 2023

[AUTOMATIC] Release/7.2.2 #1457

Merged

vegaro added pr:other and removed pr:refactor labels Sep 17, 2024

		every { baseURL } returns URL("https://api.revenuecat.com")
		every { paywallEventsURL } returns URL("https://api-paywalls.revenuecat.com/")

Conversation

tonidero commented Nov 7, 2023

Description

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!

codecov Bot commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

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!

3 participants

codecov Bot commented Nov 7, 2023 •

edited

Loading