-
Notifications
You must be signed in to change notification settings - Fork 11
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
Support new expiresAt/expiresIn/token/passed fields with trackOnce({ fraud: true }) and trackVerified() #159
Conversation
@@ -212,8 +213,11 @@ export interface RadarTrackResponse extends RadarResponse { | |||
events?: RadarEvent[]; | |||
} | |||
|
|||
export interface RadarTrackTokenResponse extends RadarResponse { | |||
export interface RadarTrackVerifiedResponse extends RadarTrackResponse { |
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.
Could also be a separate interface instead of extending RadarTrackResponse
constructor(message: string) { | ||
super(message); | ||
this.name = 'RadarLocationPermissionsError'; | ||
this.name = 'RadarPermissionsError'; |
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.
To match mobile SDKs
this.name = 'RadarDesktopAppError'; | ||
this.status = 'ERROR_DESKTOP_APP'; | ||
super('Radar Verify app not running.'); | ||
this.name = 'RadarVerifyAppError'; |
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.
To account for Radar Verify desktop AND mobile apps
constructor() { | ||
super('Request timed out.'); | ||
this.name = 'RadarTimeoutError'; | ||
this.status = 'ERROR_TIMED_OUT'; | ||
this.name = 'RadarNetworkError'; |
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.
To match mobile SDKs
package.json
Outdated
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "radar-sdk-js", | |||
"version": "4.1.18", | |||
"version": "4.2.0-beta.0", |
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.
Are we shipping these changes as 4.2
?
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.
Removing trackVerifiedToken()
and renaming the error codes are breaking changes, so should prob be 4.2.x
instead of 4.1.x
src/api/track.ts
Outdated
if (expiresAt) { | ||
expiresAt = new Date(expiresAt); | ||
passed = user?.fraud?.passed && user?.country?.passed && user?.state?.passed; | ||
expiresIn = expiresAt ? (expiresAt.getTime() - new Date().getTime()) / 1000 : null; |
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: It looks like expiresAt
will also always be present here.
nit: You can also do Date.now()
instead of new Date().getTime()
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. @tderouin if we can expose expiresIn
(in seconds, expiresAt - now
) in the API response, I can also just remove this, along with passed
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.
@@ -148,10 +148,6 @@ class Radar { | |||
return VerifyAPI.trackVerified(params); | |||
} | |||
|
|||
public static trackVerifiedToken(params: RadarTrackParams = {}) { |
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 don't want this to be a breaking change, could we potentially pass the params
here to track width { fraud: true }
?
Could also include a deprecation notice:
Logger.warn('DEPRECATED - trackVerifiedToken will be removed in a future release.');
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.
Nobody uses this in prod yet (except for our demo page), so I think we're fine to skip this
if (expiresAt) { | ||
expiresAt = new Date(expiresAt); | ||
passed = user?.fraud?.passed && user?.country?.passed && user?.state?.passed; | ||
expiresIn = (expiresAt.getTime() - Date.now()) / 1000; |
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.
should the name be expiresInSeconds
to be explicit? No strong opinion here because jwt uses expiresIn
as well and just has the docs saying it is in seconds
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 don't really include units in the field name anywhere else in our APIs, so prefer to keep it expiresIn
(if I could go back in a time machine to 2017 I might have used this pattern, but too late now, rather optimize for consistency and brevity)
trackOnce({ fraud: true })
, return newtoken/expiresAt/expiresIn/passed
fields. @tderouin right now we're calculatingexpiresIn
andpassed
on the client, but cleaner to calculate them on the server. Could be a fast followtrackVerified()
andtrackVerifiedToken()
. Now just a single function which returns all ofuser/events/location/token/expiresAt/expiresIn/passed
. No desktop app changes requiredRadarTimeoutError
-->RadarNetworkError
,RadarLocationPermissionsError
-->RadarPermissionsError
. Also renamesRadarDesktopAppError
toRadarVerifyAppError
given the addition of the Radar Verify mobile appsRadarLocationPermissionsError
for any location error, including location timeouts. Now we only returnRadarPermissionsError
for true permissions errors (code 1) andRadarLocationError
for location timeouts or unable to determine location (codes 2 and 3)