`Paywalls`: add identifier to events by joshdholtz · Pull Request #3332 · RevenueCat/purchases-ios

joshdholtz · 2023-10-24T10:30:37Z

Motivation

Send a unique id on each paywall event

Description

Added id: ID? to PaywallEvent.Data
- Optional so its backwards compatible with events recorded before this change (see actual tests from below)
Add withNewID() function that will mutate the id when tracked for cancel and close

Events recored before `id` changes but posted with `id` changes

Events recorded and posted with `id` changes

NachoSoto · 2023-10-24T15:40:45Z

How are you thinking of this not being backwards compatible? Just drop previous unsent events?

joshdholtz · 2023-10-24T15:51:11Z

How are you thinking of this not being backwards compatible? Just drop previous unsent events?

@NachoSoto The backend has this event id being optional so that its backwards compatible 🤷‍♂️ I could have made new "version 2" validation models on the backend but it wouldn't provide that much value and more code to maintain

NachoSoto · 2023-10-24T16:08:59Z

What i mean is that PaywallStoredEvent/PaywallEvent now has a new mandatory field, so PaywallEventStore.fetch will not decode them anymore.

The current API only provided a `CustomerInfo`. Note that this does not include `userCanceled`. We can expose separately with another `.onPurchaseCancelled` modifier in the future.

These were failing in some of the earlier Xcode 15 betas, but they all seem to be passing now. `PresentIfNeededTests` are still not working though. But I just manually verified that these modifiers do work correctly on iOS 17 on an app.

…e failing test right now)

The current API only provided a `CustomerInfo`. Note that this does not include `userCanceled`. We can expose separately with another `.onPurchaseCancelled` modifier in the future.

@joshdholtz

…3335) Requested by @joshdholtz for [paywalls-events-add-id](https://github.com/RevenueCat/purchases-ios/tree/paywalls-events-add-id) Co-authored-by: Distiller <distiller@static.38.39.183.26.cyberlynk.net>

@joshdholtz

…3336) Requested by @joshdholtz for [paywalls-events-add-id](https://github.com/RevenueCat/purchases-ios/tree/paywalls-events-add-id) Co-authored-by: Distiller <distiller@static.38.39.184.242.cyberlynk.net>

@joshdholtz

…3337) Requested by @joshdholtz for [paywalls-events-add-id](https://github.com/RevenueCat/purchases-ios/tree/paywalls-events-add-id) Co-authored-by: distiller <distiller@static.38.23.41.34.cyberlynk.net>

…mpatiblility

joshdholtz · 2023-10-25T14:25:56Z

        }

-        self.track(.close(data.withCurrentDate()))
+        self.track(.close(data.withNewID().withCurrentDate()))


Need this otherwise the id was the same for each event in the session 😅

This is a smell in how we're using the type.

I think to prevent this we should put the Event.ID inside of PaywallEvent not PaywallEvent.Data. What do you think?

joshdholtz · 2023-10-25T14:26:23Z

    public struct Data {

        // swiftlint:disable missing_docs
+        public var id: ID?


Optional to make backwards compatible with previously stored events

Let's leave a comment explaining that.

codecov · 2023-10-25T16:12:02Z

Codecov Report

Attention: Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.00%. Comparing base (50bd457) to head (37a328b).
Report is 499 commits behind head on main.

Files with missing lines	Patch %	Lines
Sources/Paywalls/Events/PaywallEvent.swift	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3332      +/-   ##
==========================================
+ Coverage   85.98%   86.00%   +0.02%     
==========================================
  Files         237      237              
  Lines       17086    17097      +11     
==========================================
+ Hits        14691    14704      +13     
+ Misses       2395     2393       -2

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

NachoSoto

Almost ready, a few comments:

Re-using event data and having to change IDs
Test for posting events with no ID
Also can you add a test in PaywallEventStoreTests that verifies that we can fetch events with no ID?

NachoSoto · 2023-10-25T16:25:21Z

        }

-        self.track(.close(data.withCurrentDate()))
+        self.track(.close(data.withNewID().withCurrentDate()))


This is a smell in how we're using the type.

I think to prevent this we should put the Event.ID inside of PaywallEvent not PaywallEvent.Data. What do you think?

NachoSoto · 2023-10-25T16:25:36Z

    public struct Data {

        // swiftlint:disable missing_docs
+        public var id: ID?


Let's leave a comment explaining that.

NachoSoto · 2023-10-25T16:26:07Z

        let data = storedEvent.event.data

        self.init(
+            id: data.id?.uuidString,


Let's add a test verifying that we can send events with no ID.

NachoSoto · 2023-10-25T20:12:37Z

Nit: I'd mark this as a "refactor" since it's not a user facing change that users need to know about in the release notes IMO.

NachoSoto · 2023-10-25T21:12:41Z

        }

-        self.track(.cancel(data.withCurrentDate()))
+        self.track(.cancel(.init(), data))


Much better 👍🏻

@NachoSoto

**This is an automatic release.** ### New Features * `PaywallColor`: change visibility of `Color.init(light:dark:)` to `private` (#3345) via NachoSoto (@NachoSoto) ### RevenueCatUI * `Paywalls`: new `.onPurchaseCompleted` overload with `StoreTransaction` (#3323) via NachoSoto (@NachoSoto) * `Paywalls`: finished template 5 (#3340) via NachoSoto (@NachoSoto) * `Paywalls`: new `onDismiss` parameter for `presentPaywallIfNeeded` (#3342) via NachoSoto (@NachoSoto) * `Paywalls`: disable shimmering on footer loading view (#3324) via NachoSoto (@NachoSoto) ### Bugfixes * `ErrorUtils.purchasesError(withSKError:)`: handle `URLError`s (#3346) via NachoSoto (@NachoSoto) ### Other Changes * `Paywalls`: add identifier to events (#3332) via Josh Holtz (@joshdholtz) * `Paywalls`: create new event session when paywall appears (#3330) via Josh Holtz (@joshdholtz) * `HTTPClient`: verbose logs for request IDs (#3320) via NachoSoto (@NachoSoto) * `Paywalls Tester`: fix `macOS` build (#3341) via NachoSoto (@NachoSoto) * `ProductFetcherSK1`: enable `TimingUtil` log (#3327) via NachoSoto (@NachoSoto) * `Paywall Tester`: fixed paywall presentation (#3339) via NachoSoto (@NachoSoto) * `CI`: replace Carthage build jobs with `xcodebuild` (#3338) via NachoSoto (@NachoSoto) * `Integration Tests`: use repetition count from test plan (#3329) via NachoSoto (@NachoSoto) * `Integration Tests`: new logs for troubleshooting flaky tests (#3328) via NachoSoto (@NachoSoto) * `CircleCI`: change iOS 17 job to use M1 Large resource (#3322) via NachoSoto (@NachoSoto) * `Paywalls Tester`: fix release build (#3321) via NachoSoto (@NachoSoto) * `Paywalls`: enable all iOS 17 tests (#3331) via NachoSoto (@NachoSoto) * `CI`: added workaround for Snapshots in `Xcode Cloud` (#2857) via NachoSoto (@NachoSoto) * `StoreKit 1`: disabled `finishTransactions` log on observer mode (#3314) via NachoSoto (@NachoSoto) --------- Co-authored-by: NachoSoto <ignaciosoto90@gmail.com>

joshdholtz added enhancement pr:RevenueCatUI labels Oct 24, 2023

joshdholtz force-pushed the paywalls-events-add-id branch 2 times, most recently from fd7f872 to 71acd2b Compare October 24, 2023 11:16

NachoSoto reviewed Oct 24, 2023

View reviewed changes

Comment thread Sources/Paywalls/Events/PaywallEvent.swift Outdated

NachoSoto reviewed Oct 24, 2023

View reviewed changes

Comment thread Sources/Paywalls/Events/PaywallEvent.swift Outdated

Base automatically changed from paywalls-only-track-impression-and-close-once to main October 24, 2023 20:21

joshdholtz force-pushed the paywalls-events-add-id branch from 78eb666 to e4ba124 Compare October 24, 2023 20:31

NachoSoto and others added 18 commits October 25, 2023 09:00

Paywalls: new .onPurchaseCompleted overload with StoreTransaction

b27b37b

The current API only provided a `CustomerInfo`. Note that this does not include `userCanceled`. We can expose separately with another `.onPurchaseCancelled` modifier in the future.

Fixed API tester

adbf8b5

Paywalls: enable all iOS 17 tests

b2688be

These were failing in some of the earlier Xcode 15 betas, but they all seem to be passing now. `PresentIfNeededTests` are still not working though. But I just manually verified that these modifiers do work correctly on iOS 17 on an app.

Only allow impression and close to track once

870487b

Allow multiple sessions to be record for a single paywall (but has on…

fd1af6b

…e failing test right now)

Remove tests that didn't add anything new

63dda9a

Paywalls: new .onPurchaseCompleted overload with StoreTransaction

48c862d

The current API only provided a `CustomerInfo`. Note that this does not include `userCanceled`. We can expose separately with another `.onPurchaseCancelled` modifier in the future.

Start of adding id to events

fb896f4

Use hardcoded UUID for tests

6008568

Empty get CI run again

fd0fbb4

Added missing id

bed459a

Forgot a tvOS file now

f59af8c

Generating new test snapshots for paywalls-events-add-id - ios-15 (#…

3e0ac4e

…3335) Requested by @joshdholtz for [paywalls-events-add-id](https://github.com/RevenueCat/purchases-ios/tree/paywalls-events-add-id) Co-authored-by: Distiller <distiller@static.38.39.183.26.cyberlynk.net>

Generating new test snapshots for paywalls-events-add-id - ios-16 (#…

ee23c46

…3336) Requested by @joshdholtz for [paywalls-events-add-id](https://github.com/RevenueCat/purchases-ios/tree/paywalls-events-add-id) Co-authored-by: Distiller <distiller@static.38.39.184.242.cyberlynk.net>

Generating new test snapshots for paywalls-events-add-id - ios-17 (#…

a7f3b03

…3337) Requested by @joshdholtz for [paywalls-events-add-id](https://github.com/RevenueCat/purchases-ios/tree/paywalls-events-add-id) Co-authored-by: distiller <distiller@static.38.23.41.34.cyberlynk.net>

Rename EventID to ID

e64050f

Maybe fix lint

88dd1a5

Or this

bdfc1ee

joshdholtz force-pushed the paywalls-events-add-id branch from c1e0b1a to e8cf052 Compare October 25, 2023 14:23

Fix for new id not being tracked and making optional for backwards co…

07fa54a

…mpatiblility

joshdholtz force-pushed the paywalls-events-add-id branch from e8cf052 to 07fa54a Compare October 25, 2023 14:25

joshdholtz commented Oct 25, 2023

View reviewed changes

Fix lint again???

0b2302b

joshdholtz requested a review from NachoSoto October 25, 2023 15:02

joshdholtz marked this pull request as ready for review October 25, 2023 15:02

OMG LINT

9eb4daa

NachoSoto reviewed Oct 25, 2023

View reviewed changes

NachoSoto mentioned this pull request Oct 25, 2023

[AUTOMATIC] Release/4.29.0 #3347

Closed

Moved event id and date to PaywallEvent.CreationData

59f3085

joshdholtz added refactor and removed enhancement labels Oct 25, 2023

joshdholtz added 4 commits October 25, 2023 15:30

Tests and API tester changes

b2ed6af

Fix lint

e0ffba4

My local xcode is so busted and not showing me these errors

aa1386d

I'm going to start using vim instead of xcode

df66bba

NachoSoto approved these changes Oct 25, 2023

View reviewed changes

This is the one

37a328b

joshdholtz merged commit 38b4e3b into main Oct 26, 2023

joshdholtz deleted the paywalls-events-add-id branch October 26, 2023 01:26

RCGitBot mentioned this pull request Oct 26, 2023

[AUTOMATIC] Release/4.29.0 #3350

Merged

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

Conversation

joshdholtz commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Description

Events recored before id changes but posted with id changes

Events recorded and posted with id changes

Uh oh!

NachoSoto commented Oct 24, 2023

Uh oh!

joshdholtz commented Oct 24, 2023

Uh oh!

NachoSoto commented Oct 24, 2023

Uh oh!

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!

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

Uh oh!

Codecov Report

Uh oh!

NachoSoto 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!

Choose a reason for hiding this comment

Uh oh!

NachoSoto commented Oct 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joshdholtz commented Oct 24, 2023 •

edited

Loading

Events recored before `id` changes but posted with `id` changes

Events recorded and posted with `id` changes

codecov Bot commented Oct 25, 2023 •

edited

Loading