-
Notifications
You must be signed in to change notification settings - Fork 263
[feat] Add new user-level custom events API to SDK #1552
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
base: 5.4.0_beta_branch
Are you sure you want to change the base?
Conversation
Motivation: - previously it would log like this: "request failed with error: <OneSignalClientError: 0x600000cd49c0> - now it will log like this: "request failed with error: <OneSignalClientError code: 202, message: Error parsing JSON, response: (null), underlyingError: Error Domain=NSCocoaErrorDomain Code=3840 "JSON text did not start with array or object and option to allow fragments not set. around line 1, column 0." UserInfo={NSDebugDescription=JSON text did not start with array or object and option to allow fragments not set. around line 1, column 0., NSJSONSerializationErrorIndex=0} >"
* No need to be on protocol nor exposed
* Add button at the bottom of Dev App called "Track Custom Events" * Add example custom events in Objective-C and Swift
2baf788
to
d4b6c8b
Compare
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.
Code seems good for an alpha release, there are some TODOs in the code that should be addressed for a beta release. I left a nit small comments.
import OneSignalOSCore | ||
|
||
class OSRequestCustomEvents: OneSignalRequest, OSUserRequest { | ||
var sentToClient = false |
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.
This sentToClient
isn't clear to me what it is for.
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.
Oh yeah, it's a property on the OSUserRequest
protocol and you need to have it in each conforming type. I could have better in-code docs in the OSUserRequest protocol about these properties, but sentToClient
means this request has been sent to the client and the client will handle retrying with backoff before returning to the executors, so don't send it again while it is doing that. This will change as I plan to refactor the Operation Repo on iOS.
} | ||
self.identityModel = identityModel | ||
self.stringDescription = "<OSRequestCustomEvents with parameters: \(parameters)>" | ||
super.init() |
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.
nit, calling super.init() seems out of place in the middle of the method, I would recommend calling it at the top or bottom of method.
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.
Yeah it feel weird but it needs to be there in the middle. The stuff above it are properties on this
class and the ones below are on the superclass.
- If you put it at the top, you'll get an error like
Property 'self.stringDescription' not initialized at super.init call
- If you put it at the bottom, it'll error with
'self' used in property access 'parameters' before 'super.init' call
Description
One Line Summary
Add new feature of user-level custom events API to the SDK, initially releasing as an alpha on the next minor version of 5.4.0.
Details
Motivation
Add new feature requested that will improve workflows of clients using our SDK.
API
Scope
✅ Click to see example payload
Testing
Unit testing
None added currently
Manual testing
See commit d4b6c8 for manual test examples.
iPhone 16 simulator on iOS 18.1
Manual testing was done on the simulator with the following scenarios:
Typical Usage
No network connection
New app install without network connection
New app install with multiple users
nan
and add eventsnan
are sent to the correct user.Using the API with Valid + Invalid Properties
nil
properties is fineDate
or a class fails and no-op, logs errorHow to Test this Feature
OSUserRequest
here to bypass the 403 response:Affected code checklist
Checklist
Overview
Testing
Final pass
This change is