Skip to content

Paywalls: Track paywall events#1447

Merged
tonidero merged 4 commits into
mainfrom
toniricodiez/pwl-321-paywall-events-5
Nov 10, 2023
Merged

Paywalls: Track paywall events#1447
tonidero merged 4 commits into
mainfrom
toniricodiez/pwl-321-paywall-events-5

Conversation

@tonidero

@tonidero tonidero commented Nov 8, 2023

Copy link
Copy Markdown
Contributor

Description

This PR adds tracking of the paywall_impression, paywall_close and paywall_cancel events to the paywalls.

tonidero
tonidero commented Nov 8, 2023
View reviewed changes

@tonidero tonidero left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Need to add tests and test it manually a bit more, but seems to be working.

Comment thread ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/PaywallDialog.kt Outdated

@tonidero tonidero Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I needed the viewmodel in order to track this event when dismissing the dialog correctly. It shouldn't cause duplications since we don't have keys anymore and it's the same model.

@codecov

codecov Bot commented Nov 8, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (530b61b) 83.62% compared to head (94f354b) 84.03%.
Report is 2 commits behind head on main.

❗ Current head 94f354b differs from pull request most recent head 20707b9. Consider uploading reports for the commit 20707b9 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1447      +/-   ##
==========================================
+ Coverage   83.62%   84.03%   +0.40%     
==========================================
  Files         206      206              
  Lines        6811     6848      +37     
  Branches      989      993       +4     
==========================================
+ Hits         5696     5755      +59     
+ Misses        726      706      -20     
+ Partials      389      387       -2
Files Coverage Δ
...otlin/com/revenuecat/purchases/PurchasesFactory.kt 85.24% <100.00%> (+0.32%) ⬆️
...otlin/com/revenuecat/purchases/common/AppConfig.kt 82.25% <100.00%> (ø)
...t/purchases/common/diagnostics/DiagnosticsEntry.kt 100.00% <100.00%> (ø)
...chases/common/diagnostics/DiagnosticsFileHelper.kt 100.00% <100.00%> (+16.66%) ⬆️
...ases/common/diagnostics/DiagnosticsSynchronizer.kt 94.11% <100.00%> (ø)
...purchases/common/diagnostics/DiagnosticsTracker.kt 93.42% <100.00%> (ø)
...revenuecat/purchases/common/networking/Endpoint.kt 100.00% <100.00%> (ø)
.../purchases/paywalls/events/PaywallEventsManager.kt 100.00% <100.00%> (+7.69%) ⬆️
...at/purchases/paywalls/events/PaywallStoredEvent.kt 94.44% <100.00%> (+81.11%) ⬆️
...t/purchases/common/networking/RCHTTPStatusCodes.kt 66.66% <0.00%> (ø)
... and 5 more

... and 5 files with indirect coverage changes

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

NachoSoto
NachoSoto reviewed Nov 8, 2023
View reviewed changes
Comment thread ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/InternalPaywall.kt Outdated

@NachoSoto NachoSoto Nov 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is viewModel.close called twice now with this?

@tonidero tonidero Nov 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No, the BackHandler is only called when the user presses the back button/performs the back navigation gesture.

We only call close manually when the user presses the X button or dismisses the PaywallDialog (by tapping outside it).

@NachoSoto NachoSoto Nov 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh so we weren't calling that then, good catch

Comment thread ...venuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/data/PaywallViewModel.kt Outdated
tonidero
tonidero commented Nov 9, 2023
View reviewed changes
Comment thread ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/PaywallDialog.kt Outdated

@tonidero tonidero Nov 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I would like to add tests to make sure we're calling these appropriately, but those would be UI tests, not snapshot tests, which might be a bit harder to write... Will try to write these tests after.

@tonidero tonidero marked this pull request as ready for review November 9, 2023 13:14
@tonidero tonidero requested a review from a team November 9, 2023 13:15
NachoSoto
NachoSoto approved these changes Nov 9, 2023
View reviewed changes
Comment thread ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/InternalPaywall.kt Outdated

@NachoSoto NachoSoto Nov 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh so we weren't calling that then, good catch

Comment thread ...venuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/data/PaywallViewModel.kt Outdated

@NachoSoto NachoSoto Nov 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

TIL !is

Comment thread ...venuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/data/PaywallViewModel.kt Outdated
Comment thread ...ecatui/src/test/kotlin/com/revenuecat/purchases/ui/revenuecatui/data/PaywallViewModelTest.kt Outdated
@tonidero tonidero force-pushed the toniricodiez/pwl-321-paywall-events-4 branch from 643fa77 to af4c529 Compare November 10, 2023 09:16
Base automatically changed from toniricodiez/pwl-321-paywall-events-4 to main November 10, 2023 09:34
@tonidero tonidero force-pushed the toniricodiez/pwl-321-paywall-events-5 branch from ecf55d6 to 20707b9 Compare November 10, 2023 09:35
@tonidero tonidero enabled auto-merge (squash) November 10, 2023 09:36
@tonidero tonidero merged commit d64f9d7 into main Nov 10, 2023
@tonidero tonidero deleted the toniricodiez/pwl-321-paywall-events-5 branch November 10, 2023 09:52
@RCGitBot RCGitBot mentioned this pull request Nov 10, 2023
Merged
tonidero pushed a commit that referenced this pull request Nov 10, 2023
@RCGitBot
[AUTOMATIC] Release/7.2.2 (#1457)
c8541b6 
**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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@NachoSoto NachoSoto NachoSoto approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr:other

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@tonidero @NachoSoto @vegaro