-
Notifications
You must be signed in to change notification settings - Fork 369
Webhook responses for Stripe use-cases #1858
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
7 Skipped Deployments
|
error: unknown; | ||
message = "Unhandled error"; | ||
|
||
constructor(args: { error: unknown }) { |
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.
Question: shouldn't we capture exception to Sentry here as a side effect?
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.
maybe instead side effect here, add param "sentry: true" and route can do that instead?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1858 +/- ##
==========================================
+ Coverage 25.46% 25.66% +0.19%
==========================================
Files 751 755 +4
Lines 53114 53166 +52
Branches 1812 1835 +23
==========================================
+ Hits 13526 13645 +119
+ Misses 39231 39164 -67
Partials 357 357 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
import { z } from "zod"; | ||
|
||
export const PaymentGatewayInitializeResponseDataShape = z.object({ | ||
stripePublishableKey: z.string(), |
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.
Maybe its too much, but if we want extra security we can do refine() and run PublsihableKey.create here
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.
actually in use-case we take its value from StripeConfig VO which itself is needs to have proper values 🤔
*/ | ||
try { | ||
/* | ||
* todo create config repo |
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 comment is now outdated
const response = new SaleorApiUrlCreateErrorResponse({ | ||
error: saleorApiUrlResult.error, | ||
}); |
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 think this is too detailed, we are now returning this outside. I think it's fine to use here some generic response like "MalformedRequestError". We will know details in Sentry and this is scope of Stripe app only
|
||
export class GetConfigErrorResponse extends ErrorWebhookResponse { | ||
error: InstanceType<typeof GetConfigError>; | ||
message = "App is not configured - error while getting config"; |
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 message goes to external service so I would only say "app is not working", which is true. We dont know if its configured - maybe it was, but not DB is not responding. We probably shouldnt elaborate on DB too.
Sentry on the other hand will have details
return Response.json( | ||
{ | ||
message: this.message, | ||
channelId: this.error.channelId, |
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.
is Saleor going to actually read channelId?
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.
No idea - I will remove it then from from json response
error: unknown; | ||
message = "Unhandled error"; | ||
|
||
constructor(args: { error: unknown }) { |
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.
maybe instead side effect here, add param "sentry: true" and route can do that instead?
|
||
// TODO: add support for other results e.g AUTHORIZE | ||
|
||
class ChargeRequest extends ChargeSuccessResponse { |
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 dont get it, why ChargeRequest extends from ChargeSuccess? It will inherit actions like "REFUND" which is not correct yet
|
||
class ChargeFailure extends ChargeFailureResponse { | ||
result = "CHARGE_FAILURE" as const; | ||
amount = 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.
why zero?
class Success extends SuccessWebhookResponse { | ||
responseData: ResponseDataType; | ||
|
||
constructor(args: { responseData: ResponseDataType }) { |
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 think ResponseDataType
should be part ot "UseCaseResponse".
IMO you should do new Success({pk: PublishableKey})
and success knows internally (getResponse) how to map PublshableKey to this specific ResponseDataType
In you implementation, you require UseCase to "know" the data structure of the response, even if you proposed abstraction for it already (SuccessWebhookResponse)
In this example this is not that problematic, but once we have more complex examples, your use-case will be unnecessarily bloated
export const responseData = z | ||
.object({ | ||
stripePublishableKey: z.string(), | ||
}) | ||
.brand("PaymentGatewayInitializeSessionResponseData"); | ||
|
||
export type ResponseDataType = z.infer<typeof responseData>; |
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 dont have any protected against importing incorrect stuff, and probably we will have as many "ResponseDataType" as many routes we will have
it makes poor DX because Typescript will import one of 5 ResponseDataType - no necessarily from ./
I suggest either:
- Make export names explicit like
PaymentGatewayInitializeSessionResponseDataType
- Or setup dependency cruiser to ensure file from route folder can't import file from another route (which iMO we should do anyway!)
responseData: responseData.parse({ | ||
stripePublishableKey: pk.keyValue, | ||
}), |
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.
IMO this is too early to unpack pk.
IMO
- You should pass directly
pk
toSuccess()
constructor Success()
will internally know how to construct response
This will give us actual abstraction on response shape - now you have class that returns it, but it doesn't create it. There is no reason to have abstraction of response if you pass entire response in the constrcturo from the ourside
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.
IMO this is so much coupled to Response objects, it can be in the same files
export const stripePaymentIntentId = z | ||
.string({ | ||
required_error: "Payment intent id is required", | ||
}) | ||
.startsWith("pi_") | ||
.brand("StripePaymentIntentId"); |
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 is not something specific to response. Response is actually json (that leaves the app) + class that creates such json
On the other hand StripePaymentIntentId is part of our domain, so I think it should be in modules/stripe just like other VOs
export class SuccessWebhookResponse { | ||
statusCode = 200; | ||
} | ||
|
||
export class ErrorWebhookResponse { | ||
statusCode = 500; | ||
} |
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.
make them abstract, they can't be returned as-is
|
||
describe("PaymentGatewayInitializeSessionUseCaseResponses", () => { | ||
describe("Success", () => { | ||
it("should return fetch API response with status code and message", async () => { |
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.
it("should return fetch API response with status code and message", async () => { | |
it("getResponse() returns valid Response with status 200 and formatted 'data' object containing Stripe PK", async () => { |
I think Response is more obvious that "fetch API response" (I am editing this comment because I didnt understand it before)
import { StripePublishableKey } from "@/modules/stripe/stripe-publishable-key"; | ||
|
||
class Success extends SuccessWebhookResponse { | ||
pk: StripePublishableKey; |
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 you pass it to constructor, better make it readonly or private. Mutation of class member can lead to hard to find bugs and I dont think its our intention
); | ||
}); | ||
|
||
it("Throws error when currency coming from Saleor is not supported", async () => { | ||
it("Returns BrokenAppResponse when currency coming from Saleor is not supported", async () => { |
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.
Is that right? I think it should be "InvalidRequest".
App will document supported currencies and if someone tried to pay for Dogecoin, it's not app that is broken but incoming call
} | ||
} | ||
|
||
export class MalformedRequestResponse extends ErrorWebhookResponse { |
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 think you didn't use this, but it fits unsupported currency IMO
Scope of the PR
Related issues
Checklist