-
Notifications
You must be signed in to change notification settings - Fork 123
Add a user-data class for generating intercomSettings #173
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
Conversation
fa803c8 to
432bb1d
Compare
| } | ||
| getVerificationSecret() { | ||
| const { verificationSecret } = this.settings; | ||
| delete this.settings.verificationSecret; |
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.
If we're going to delete the secret, we should ensure json can be called multiple times by skipping setUserHash if user_hash is already set in this.settings
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.
Updated!
432bb1d to
70a7103
Compare
lib/user-data.js
Outdated
| this.settings = settings; | ||
| } | ||
| json() { | ||
| const verificationSecret = this.getVerificationSecret(); |
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.
We might also want to wrap the secret/hash things in a conditional and skip them if this.loggedOut
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.
I am skipping the hash already if logged out here: https://github.com/intercom/intercom-node/pull/173/files#diff-4c919967cdaa500e061ab2489c6d0136R38
Do you think we should also skip the verification secret?
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 seems like we should skip this work if this.loggedOut || this.settings.user_hash. Since verificationSecret and identifier are only used in setUserHash in json, and getVerificationSecret/getIdentifier are sibling methods, you could move those getters to be called inside setUserHash as well.
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.
Great point
This adds a
UserDataclass to the Node client, which is exposed through the client like so:It returns an escaped json object, including the
user_hashrequired for Identity Verification. You can call window.Intercom(boot) on the client side with the data returned.