Web SDK Refactor: Login Updates #1299
Conversation
fadi-george
requested review from
jkasten2 and
sherwinski
and removed request for
jkasten2
| tags?: Record<string, string>; | ||
| language?: string; | ||
| timezoneId?: string; | ||
| timezone_id?: string; |
There was a problem hiding this comment.
what's the reason for renaming this?
There was a problem hiding this comment.
We store property as timezone_id in properties model so in PropertyOperationHelper, there would be a mistake on this line if (allowedKeys.includes(propertyKey)) { since timezoneId is different from timezone_id. So when the request is made to updateUser with new properties. The change to timezone will be ignored since properties object will have an undefined timezoneid value.
There was a problem hiding this comment.
I'm not following, why wouldn't we want to update the timezone?
There was a problem hiding this comment.
you were asking about renaming it
| @@ -49,10 +57,13 @@ export default class UserDirector { | |||
| ...rest, | |||
| }), | |||
| ); | |||
|
|
|||
| // in case init and login are in the same bucket | |||
| await delay(OP_REPO_EXECUTION_INTERVAL * 2); | |||
There was a problem hiding this comment.
I'm not sure I understand what the intention is here
There was a problem hiding this comment.
Ill update the comment to be this:
// in case OneSignal.init and OneSignal.login are both called in a short time period
// we need this create operation to finish before attempting to execute the login (with external id) operation
// otherwise the login operation executor will error since there will be two login operations
There was a problem hiding this comment.
Hmm I can't think of a better way to guard against this (maybe by adding some kind of isExecuting flag) but generally I don't think we should rely on waits protecting us against race conditions.
There was a problem hiding this comment.
Ill look into reset identity model and properties model onesignal id for login, logout, and init
| // OneSignalDeferred.push(async function (OneSignal) { | ||
| // await OneSignal.login('new-web-sdk'); | ||
| // }); |
There was a problem hiding this comment.
Can just let whoever uncomment it if they need to.
| appId, | ||
| currentOneSignalId, | ||
| externalId, | ||
| !currentExternalId ? currentOneSignalId : undefined, |
There was a problem hiding this comment.
Does this save the externalId as currentOneSignalId locally? Or is this actually how it's saved on the backend when a user is created?
There was a problem hiding this comment.
The Android SDK does some other things like reset the onesignal & external ids. I feel these dont need to be done since the login operation executor will update the values as along the identity model values if theres matching ids.
For this login operation, if theres no external id previously then it will call a set alias operation.
If there is a previous external id, it will create a new user.
Description
1 Line Summary
Continuation of the Web SDK Refactor project.
Details
initializeUser, waits for delay since it will error if OneSignal.init and OneSignal.login enqueue ops in the same bucketloginto mimic Android SDK behavior somewhat via enqueuing a login operation. If an external id is not present, it will set an alias. If there is an external id, it will create a new user.Systems Affected
Validation
Tests
Info
Checklist
Programming Checklist
Interfaces:
Functions:
Typescript:
Other:
elem of arraysyntax. PreferforEachor usemapcontextif possible. Instead, we can pass it to function/constructor so that we don't callOneSignal.contextScreenshots
Info
Checklist
Related Tickets
This change is