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!

tonidero · 2024-10-24T10:38:36Z

Sources/Events/Networking/EventsRequest.swift

+    }
+
+    @available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
+    init(events: [PaywallStoredEvent]) {


I guess ideally we also do the rename for PaywallStoredEvent to something more generic that can include customer center/other features... But that can come in a separate PR I think

oh right, good point

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?

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

Draft

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

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

vegaro Oct 24, 2024

tonidero Oct 24, 2024

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

Are you sure you want to change the base?

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

Choose a reason for hiding this comment

vegaro commented Oct 17, 2024 •

edited

Loading