-
Notifications
You must be signed in to change notification settings - Fork 111
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
[PAY-2041] Connect stripe session creation errors to analytics #6465
Changes from 2 commits
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 |
---|---|---|
@@ -1,5 +1,8 @@ | ||
import { IdentityRequestError } from '@audius/sdk' | ||
import { call, takeEvery, put, select } from 'typed-redux-saga' | ||
|
||
import { Name } from 'models/Analytics' | ||
import { ErrorLevel } from 'models/ErrorReporting' | ||
import { createStripeSession } from 'services/audius-backend/stripe' | ||
import { getContext } from 'store/effects' | ||
|
||
|
@@ -12,11 +15,17 @@ import { | |
stripeSessionCreated, | ||
stripeSessionStatusChanged | ||
} from './slice' | ||
import { | ||
StripeSessionCreationError, | ||
StripeSessionCreationErrorResponseData | ||
} from './types' | ||
|
||
function* handleInitializeStripeModal({ | ||
payload: { amount, destinationCurrency, destinationWallet } | ||
}: ReturnType<typeof initializeStripeModal>) { | ||
const audiusBackendInstance = yield* getContext('audiusBackendInstance') | ||
const reportToSentry = yield* getContext('reportToSentry') | ||
const { track, make } = yield* getContext('analytics') | ||
const { onrampFailed } = yield* select(getStripeModalState) | ||
try { | ||
const res = yield* call(createStripeSession, audiusBackendInstance, { | ||
|
@@ -26,13 +35,44 @@ function* handleInitializeStripeModal({ | |
}) | ||
yield* put(stripeSessionCreated({ clientSecret: res.client_secret })) | ||
} catch (e) { | ||
// TODO: When we have better error messages from identity, we should extract them here so | ||
// they make it into analytics. | ||
// https://linear.app/audius/issue/PAY-2041/[usdc]-we-should-pipe-the-stripe-session-creation-error-back-from | ||
const { | ||
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. don't you love it when you delete the TODOs |
||
code, | ||
message: errorMessage, | ||
type | ||
} = ((e as IdentityRequestError).response?.data ?? | ||
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. Not my favorite, but casting errors in catch blocks never feels perfect. |
||
{}) as StripeSessionCreationErrorResponseData | ||
|
||
const error = new StripeSessionCreationError(code, errorMessage, type) | ||
|
||
if (onrampFailed) { | ||
yield* put({ type: onrampFailed.type, payload: { error: e } }) | ||
yield* put({ | ||
type: onrampFailed.type, | ||
payload: { error } | ||
}) | ||
} | ||
yield* put(setVisibility({ modal: 'StripeOnRamp', visible: 'closing' })) | ||
yield* call(reportToSentry, { | ||
level: ErrorLevel.Error, | ||
error, | ||
additionalInfo: { | ||
code, | ||
errorMessage, | ||
type, | ||
amount, | ||
destinationCurrency | ||
} | ||
}) | ||
yield* call( | ||
track, | ||
make({ | ||
eventName: Name.STRIPE_SESSION_CREATION_ERROR, | ||
amount, | ||
code, | ||
destinationCurrency, | ||
errorMessage, | ||
type | ||
}) | ||
) | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,3 +16,21 @@ export type StripeModalState = { | |
stripeSessionStatus?: StripeSessionStatus | ||
stripeClientSecret?: string | ||
} | ||
|
||
export type StripeSessionCreationErrorResponseData = { | ||
error: string | ||
code: string | ||
message: string | ||
type: string | ||
} | ||
|
||
export class StripeSessionCreationError extends Error { | ||
constructor( | ||
public code: string, | ||
// Avoiding `message` to not shadow the `message` property on Error | ||
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. nice, good catch. maybe should be stripeErrorMessage? 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 that works too. I think I was going for a "generic" pattern we could use in the future. But that's not really necessary and your suggestion makes it clear where the error comes from. |
||
public errorMessage: string, | ||
public type: string | ||
) { | ||
super(`Failed to create Stripe session: ${errorMessage}`) | ||
} | ||
} |
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.
??? how did this ever work?
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 do remember a time where circular dependencies would cause serious issues. I think our build tooling at this point is advanced enough to work around it and just generate a warning. FWIW, we have a few other long-standing instances of this.
I have worked on teams in the past where we didn't use index files for exactly this reason, preferring direct module imports instead.
Though, in this case, since we're bundling everything up into a "package", the index files serve as a way to join the things we want to be finally exported at the top level.
I would say when we un-package common, we might do well to get rid of index files :-)