Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bf081ff
makes paywall events flexible
vegaro Oct 21, 2024
72e8bfd
fix missing brace
vegaro Oct 21, 2024
9cd7e66
rename to FeatureEventsRequest
vegaro Oct 24, 2024
281c03f
EventsRequest
vegaro Oct 24, 2024
4a5e9b1
remove PaywallEventRequest.Event
vegaro Oct 24, 2024
8dbdb17
EventsRequest.PaywallEvent
vegaro Oct 24, 2024
c626848
extract to file and add logs
vegaro Oct 24, 2024
dd4fcb0
move eventtype to the extension
vegaro Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 39 additions & 14 deletions Sources/Paywalls/Events/Networking/PaywallEventsRequest.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,40 @@ import Foundation
/// The content of a request to the events endpoints.
struct PaywallEventsRequest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


var events: [Event]
var events: [AnyEncodable]

init(events: [Event]) {
init(events: [AnyEncodable]) {
self.events = events
}

@available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
init(events: [PaywallStoredEvent]) {
self.init(events: events.map { .init(storedEvent: $0) })
self.init(events: events.compactMap { storedEvent in
switch storedEvent.feature {
case .paywalls:
guard let event = PaywallEventRequest.Event(storedEvent: storedEvent) else {
return nil
}
return AnyEncodable(event)
}
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

(In another PR)

}

}

extension PaywallEventsRequest {
protocol FeatureEvent: Encodable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CustomerCenterEventRequest.Event will also conform to this


var id: String? { get }
var version: Int { get }
var appUserID: String { get }
var sessionID: String { get }

}

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

Choose a reason for hiding this comment

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

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...


enum EventType: String {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


Expand All @@ -39,7 +59,7 @@ extension PaywallEventsRequest {

}

struct Event {
struct Event: FeatureEvent {

let id: String?
let version: Int
Expand All @@ -57,17 +77,22 @@ extension PaywallEventsRequest {

}

extension PaywallEventsRequest.Event {
extension PaywallEventRequest.Event {

@available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
init(storedEvent: PaywallStoredEvent) {
let creationData = storedEvent.event.creationData
let data = storedEvent.event.data
init?(storedEvent: PaywallStoredEvent) {
guard let eventData = storedEvent.event.value as? [String: Any],
let paywallEvent: PaywallEvent = try? JSONDecoder.default.decode(dictionary: eventData) else {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

let creationData = paywallEvent.creationData
let data = paywallEvent.data

self.init(
id: creationData.id.uuidString,
version: Self.version,
type: storedEvent.event.eventType,
type: paywallEvent.eventType,
appUserID: storedEvent.userID,
sessionID: data.sessionIdentifier.uuidString,
offeringID: data.offeringIdentifier,
Expand All @@ -86,7 +111,7 @@ extension PaywallEventsRequest.Event {
@available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *)
private extension PaywallEvent {

var eventType: PaywallEventsRequest.EventType {
var eventType: PaywallEventRequest.EventType {
switch self {
case .impression: return .impression
case .cancel: return .cancel
Expand All @@ -99,9 +124,9 @@ private extension PaywallEvent {

// MARK: - Codable

extension PaywallEventsRequest.EventType: Encodable {}

extension PaywallEventsRequest.Event: Encodable {
extension PaywallEventRequest: Encodable {}
extension PaywallEventRequest.EventType: Encodable {}
extension PaywallEventRequest.Event: Encodable {

private enum CodingKeys: String, CodingKey {

Expand Down
2 changes: 1 addition & 1 deletion Sources/Paywalls/Events/PaywallEventSerializer.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ enum PaywallEventSerializer {
.orThrow(FailedEncodingEventError())
}

/// Decodes a `PaywallEvent`.
/// Decodes a `PaywallStoredEvent`.
static func decode(_ event: String) throws -> PaywallStoredEvent {
return try JSONDecoder.default.decode(jsonData: event.asData)
}
Expand Down
19 changes: 14 additions & 5 deletions Sources/Paywalls/Events/PaywallEventStore.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

} else {
Logger.verbose(PaywallEventStoreStrings.storing_event_without_json)
}

await self.handler.append(line: try PaywallEventSerializer.encode(storedEvent))
let event = try PaywallEventSerializer.encode(storedEvent)
await self.handler.append(line: event)
} catch {
Logger.error(PaywallEventStoreStrings.error_storing_event(error))
}
Expand Down Expand Up @@ -167,7 +172,8 @@ private enum PaywallEventStoreStrings {
case removing_old_documents_store(URL)
case error_removing_old_documents_store(Error)

case storing_event(PaywallEvent)
case storing_event(String)
case storing_event_without_json

case error_storing_event(Error)
case error_fetching_events(Error)
Expand All @@ -191,8 +197,11 @@ extension PaywallEventStoreStrings: LogMessage {
case let .error_removing_old_documents_store(error):
return "Failed removing old store: \((error as NSError).description)"

case let .storing_event(event):
return "Storing event: \(event.debugDescription)"
case let .storing_event(eventDescription):
return "Storing event: \(eventDescription)"

case .storing_event_without_json:
return "Storing an event. There was an error trying to print it"

case let .error_storing_event(error):
return "Error storing event: \((error as NSError).description)"
Expand Down
4 changes: 3 additions & 1 deletion Sources/Paywalls/Events/PaywallEventsManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ actor PaywallEventsManager: PaywallEventsManagerType {
}

func track(paywallEvent: PaywallEvent) async {
await self.store.store(.init(event: paywallEvent, userID: self.userProvider.currentAppUserID))
await self.store.store(.init(event: AnyEncodable(paywallEvent),
userID: self.userProvider.currentAppUserID,
feature: .paywalls))
}

func flushEvents(count: Int) async throws -> Int {
Expand Down
20 changes: 18 additions & 2 deletions Sources/Paywalls/Events/PaywallStoredEvent.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,18 @@

import Foundation

/// Contains the necessary information for `PaywallEventStore`.
/// Contains the necessary information for storing and sending events.
struct PaywallStoredEvent {

var event: PaywallEvent
var event: AnyEncodable
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

var userID: String
var feature: Feature

}

enum Feature: String, Codable {

case paywalls

}

Expand All @@ -31,7 +38,16 @@ extension PaywallStoredEvent: Codable {

case event
case userID = "userId"
case feature

}

init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)

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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

}