-
Notifications
You must be signed in to change notification settings - Fork 317
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.
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
Refactor Paywall events so it can be used for customer center #4376
base: main
Are you sure you want to change the base?
Changes from 2 commits
bf081ff
72e8bfd
9cd7e66
281c03f
4a5e9b1
8dbdb17
c626848
dd4fcb0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,20 +16,40 @@ import Foundation | |
/// The content of a request to the events endpoints. | ||
struct PaywallEventsRequest { | ||
|
||
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) | ||
} | ||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is going to be a case (In another PR) |
||
} | ||
|
||
} | ||
|
||
extension PaywallEventsRequest { | ||
protocol FeatureEvent: Encodable { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I'm wondering... this is not actually a request (which is |
||
|
||
enum EventType: String { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This used to be part of the |
||
|
||
|
@@ -39,7 +59,7 @@ extension PaywallEventsRequest { | |
|
||
} | ||
|
||
struct Event { | ||
struct Event: FeatureEvent { | ||
|
||
let id: String? | ||
let version: Int | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -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 | ||
|
@@ -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 { | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
} | ||
|
@@ -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) | ||
|
@@ -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)" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
} | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wouldn't it go to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think we might need to use “try?” |
||
} | ||
|
||
} |
There was a problem hiding this comment.
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 toFeatureEventsRequest
There was a problem hiding this comment.
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.