Refactor Paywall events so it can be used for customer center #4376

vegaro · 2024-10-17T09:45:00Z

We want to reuse Paywall events for Customer Center, the issue is that the Paywall events system is very types and requires PaywallEvents to be passed around.

Customer Center events will have different properties than Paywalls events, so we need a way to "untype" the code so it stores and posts events of any type.

After some attempts, I figured that if we just stores AnyEncodable and if we post AnyEncodable we can keep storing and posting paywall events the same way, and also create new Customer Center events without being constrained by paywall events types.

vegaro · 2024-10-21T16:39:44Z

Sources/Paywalls/Events/Networking/PaywallEventsRequest.swift

+                    return nil
+                }
+                return AnyEncodable(event)
+        })


there is going to be a case .customerCenter here, that will create a CustomerCenterEventRequest.Event

(In another PR)

vegaro · 2024-10-21T16:39:59Z

Sources/Paywalls/Events/Networking/PaywallEventsRequest.swift

    }

 }

-extension PaywallEventsRequest {
+protocol FeatureEvent: Encodable {


CustomerCenterEventRequest.Event will also conform to this

vegaro · 2024-10-21T16:40:48Z

Sources/Paywalls/Events/Networking/PaywallEventsRequest.swift

+// This is a `struct` instead of `enum` so that
+// we can use make it conform to Encodable
+// swiftlint:disable:next convenience_type
+struct PaywallEventRequest {

    enum EventType: String {


This used to be part of the PaywallEventsRequest but we now need to have different EventType for PaywallEventRequest and CustomerCenterRequest

vegaro · 2024-10-21T16:41:27Z

Sources/Paywalls/Events/Networking/PaywallEventsRequest.swift

@@ -16,20 +16,39 @@ import Foundation
 /// The content of a request to the events endpoints.
 struct PaywallEventsRequest {


Thought about renaming this, but we are still using the api-paywalls to post all type of events. So I am not sure. I thought about renaming to FeatureEventsRequest

Hmm I think we should rename it or it would be very confusing down the line, and maybe just keep using the old endpoint for now, (or even alias to a new endpoint name in the backend and deprecate the old one... But that can come in a separate PR.

vegaro · 2024-10-21T16:41:58Z

Sources/Paywalls/Events/PaywallEventStore.swift

@@ -40,9 +40,14 @@ internal actor PaywallEventStore: PaywallEventStoreType {

    func store(_ storedEvent: PaywallStoredEvent) async {
        do {
-            Logger.verbose(PaywallEventStoreStrings.storing_event(storedEvent.event))
+            if let eventDescription = try? storedEvent.event.prettyPrintedJSON {
+                Logger.verbose(PaywallEventStoreStrings.storing_event(eventDescription))


Improved the logging a bit so it's easier to see what's being stored

tonidero

I'm mostly concerned about the naming being confusing... not sure if we should perform some renames now or maybe delay it for later.

tonidero · 2024-10-23T13:29:19Z

Sources/Paywalls/Events/Networking/PaywallEventsRequest.swift

@@ -16,20 +16,39 @@ import Foundation
 /// The content of a request to the events endpoints.
 struct PaywallEventsRequest {


Hmm I think we should rename it or it would be very confusing down the line, and maybe just keep using the old endpoint for now, (or even alias to a new endpoint name in the backend and deprecate the old one... But that can come in a separate PR.

tonidero · 2024-10-23T13:32:04Z

Sources/Paywalls/Events/Networking/PaywallEventsRequest.swift

+    init?(storedEvent: PaywallStoredEvent) {
+        guard let eventData = storedEvent.event.value as? [String: Any],
+              let paywallEvent: PaywallEvent = try? JSONDecoder.default.decode(dictionary: eventData) else {
+            return nil


Should we log something in this case? It should ideally never happen.

tonidero · 2024-10-23T13:39:00Z

Sources/Paywalls/Events/Networking/PaywallEventsRequest.swift

+// This is a `struct` instead of `enum` so that
+// we can use make it conform to Encodable
+// swiftlint:disable:next convenience_type
+struct PaywallEventRequest {


Hmm I'm wondering... this is not actually a request (which is PaywallEventsRequest), but a single event... I was wondering whether to rename to PaywallEvent.Event or soemthing along those lines? Not sure... I think it's a bit confusing right now...

vegaro · 2024-10-24T08:13:58Z

@tonidero agree on the naming concerns. Let me give them more thought

tonidero

All the decodable stuff still confuses me a bit but I think it makes sense. Just a few questions to make sure I didn't miss something!

Sources/Events/Networking/EventsRequest.swift

tonidero · 2024-10-24T10:43:01Z

Sources/Paywalls/Events/PaywallStoredEvent.swift


+        self.event = try container.decode(AnyEncodable.self, forKey: .event)
+        self.userID = try container.decode(String.self, forKey: .userID)
+        self.feature = try container.decodeIfPresent(Feature.self, forKey: .feature) ?? .paywalls


Would we need to handle unknown feature types here? As in, just making sure that if there is a value, and it's unknown it won't throw an exception and just default to .paywalls.

wouldn't it go to ?? .paywalls? Or maybe we need to try? yes

Yeah I think we might need to use “try?”

tonidero · 2024-10-24T10:44:30Z

Sources/Paywalls/Events/PaywallStoredEvent.swift

 struct PaywallStoredEvent {

-    var event: PaywallEvent
+    var event: AnyEncodable


One small concern I have over this... If for some reason the event is written partially (say that during the write it crashed or something), this won't catch it and try to send everything? Or would at least fail if it's not a valid structure?

…reate-customer-center-open-event

vegaro · 2024-11-05T12:59:50Z

Tests/UnitTests/Paywalls/Events/PaywallEventsManagerTests.swift

@@ -196,22 +203,42 @@ class PaywallEventsManagerTests: TestCase {
        let event1 = await self.storeRandomEvent()
        _ = await self.storeRandomEvent()

-        let task1 = Task<Int, Error> { [manager = self.manager!] in try await manager.flushEvents(count: 1) }


this test was flaky. It looks like it doesn't flake anymore

tonidero

🙌 Much cleaner now IMO. Thanks!!

tonidero · 2024-11-05T13:32:16Z

cc @MarkVillacampa since you're also working with paywall events

vegaro · 2024-11-05T14:23:23Z

just retested yet again that it works for previous stored events and it works perfectly, so merging

vegaro added the pr:other label Oct 17, 2024

vegaro force-pushed the https/linear.app/revenuecat/issue/SDK-3607/create-customer-center-open-event branch 3 times, most recently from 9ce8a40 to e02864e Compare October 21, 2024 16:39

vegaro commented Oct 21, 2024

View reviewed changes

vegaro force-pushed the https/linear.app/revenuecat/issue/SDK-3607/create-customer-center-open-event branch from e02864e to ec57fbe Compare October 21, 2024 16:43

vegaro mentioned this pull request Oct 21, 2024

Create CustomerCenterEvent #4392

Merged

vegaro added 2 commits October 23, 2024 15:21

makes paywall events flexible

bf081ff

fix missing brace

72e8bfd

vegaro force-pushed the https/linear.app/revenuecat/issue/SDK-3607/create-customer-center-open-event branch from 79779b0 to 72e8bfd Compare October 23, 2024 13:21

vegaro marked this pull request as ready for review October 23, 2024 13:21

vegaro requested a review from a team October 23, 2024 13:21

tonidero reviewed Oct 23, 2024

View reviewed changes

vegaro added 4 commits October 24, 2024 10:45

rename to FeatureEventsRequest

9cd7e66

EventsRequest

281c03f

remove PaywallEventRequest.Event

4a5e9b1

EventsRequest.PaywallEvent

8dbdb17

vegaro requested a review from tonidero October 24, 2024 09:28

vegaro added 2 commits October 24, 2024 11:54

extract to file and add logs

c626848

move eventtype to the extension

dd4fcb0

tonidero approved these changes Oct 24, 2024

View reviewed changes

vegaro added 4 commits October 25, 2024 11:27

Merge branch 'main' into https/linear.app/revenuecat/issue/SDK-3607/c…

b0a0b71

…reate-customer-center-open-event

rename PaywallStoredEvent

a08f35d

fix compilation

eb539d8

fix tests

8430e68

vegaro added 11 commits October 31, 2024 17:40

linter

03e9ae1

fix PaywallEventStoreTests

218db03

refactor to store a dictionary

7cf37dc

fix keys

44b0b43

fix codable

569971c

fix PaywallEventSerializerTests

70b0236

more cleanup of tests

80c516b

revert changes in test

676e995

fix more tests

7c4a984

fix flaky test

4f447e3

lint

2cf152c

vegaro requested a review from tonidero November 5, 2024 12:59

vegaro commented Nov 5, 2024

View reviewed changes

tonidero approved these changes Nov 5, 2024

View reviewed changes

vegaro merged commit ed8fdde into main Nov 5, 2024
7 checks passed

vegaro deleted the https/linear.app/revenuecat/issue/SDK-3607/create-customer-center-open-event branch November 5, 2024 14:23

This was referenced Nov 6, 2024

[AUTOMATIC] Release/5.8.0 #4441

Closed

[AUTOMATIC] Release/5.8.0 #4445

Closed

[AUTOMATIC] Release/5.8.0 #4458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Paywall events so it can be used for customer center #4376

Refactor Paywall events so it can be used for customer center #4376

vegaro commented Oct 17, 2024 •

edited

Loading

vegaro Oct 21, 2024

vegaro Oct 21, 2024

vegaro Oct 21, 2024

vegaro Oct 21, 2024

tonidero Oct 23, 2024

vegaro Oct 21, 2024

tonidero left a comment

tonidero Oct 23, 2024

tonidero Oct 23, 2024

tonidero Oct 23, 2024

vegaro commented Oct 24, 2024

tonidero left a comment

tonidero Oct 24, 2024

vegaro Oct 24, 2024

tonidero Oct 24, 2024

tonidero Oct 24, 2024

vegaro Nov 5, 2024

tonidero left a comment

tonidero commented Nov 5, 2024

vegaro commented Nov 5, 2024

		@@ -16,20 +16,39 @@ import Foundation
		/// The content of a request to the events endpoints.
		struct PaywallEventsRequest {

Refactor Paywall events so it can be used for customer center #4376

Refactor Paywall events so it can be used for customer center #4376

Conversation

vegaro commented Oct 17, 2024 • 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

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 Oct 24, 2024

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

tonidero commented Nov 5, 2024

vegaro commented Nov 5, 2024

vegaro commented Oct 17, 2024 •

edited

Loading