Skip to content

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

Merged
merged 10 commits into from
Apr 14, 2025
Merged

Conversation

krzysztofzuraw
Copy link
Member

Scope of the PR

Related issues

Checklist

@krzysztofzuraw krzysztofzuraw added the skip changeset Attach this label to PRs which does not need changes description for the release notes. label Apr 11, 2025
Copy link

changeset-bot bot commented Apr 11, 2025

⚠️ No Changeset found

Latest commit: 8918d0f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Apr 11, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
apps-stripe ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 13, 2025 2:57pm
7 Skipped Deployments
Name Status Preview Comments Updated (UTC)
saleor-app-avatax ⬜️ Skipped (Inspect) Apr 13, 2025 2:57pm
saleor-app-cms ⬜️ Skipped (Inspect) Apr 13, 2025 2:57pm
saleor-app-klaviyo ⬜️ Skipped (Inspect) Apr 13, 2025 2:57pm
saleor-app-products-feed ⬜️ Skipped (Inspect) Apr 13, 2025 2:57pm
saleor-app-search ⬜️ Skipped (Inspect) Apr 13, 2025 2:57pm
saleor-app-segment ⬜️ Skipped (Inspect) Apr 13, 2025 2:57pm
saleor-app-smtp ⬜️ Skipped (Inspect) Apr 13, 2025 2:57pm

error: unknown;
message = "Unhandled error";

constructor(args: { error: unknown }) {
Copy link
Member Author

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?

Copy link
Member

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?

@vercel vercel bot temporarily deployed to Preview – saleor-app-segment April 11, 2025 11:13 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-smtp April 11, 2025 11:13 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-avatax April 11, 2025 11:13 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-search April 11, 2025 11:13 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-cms April 11, 2025 11:13 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-products-feed April 11, 2025 11:13 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-klaviyo April 11, 2025 11:13 Inactive
@krzysztofzuraw krzysztofzuraw marked this pull request as ready for review April 11, 2025 11:13
@krzysztofzuraw krzysztofzuraw requested a review from a team April 11, 2025 11:13
Copy link

codecov bot commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 78.83959% with 62 lines in your changes missing coverage. Please review.

Project coverage is 25.66%. Comparing base (002cbeb) to head (8918d0f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...saleor/payment-gateway-initialize-session/route.ts 0.00% 29 Missing ⚠️
...api/saleor/transaction-initialize-session/route.ts 0.00% 24 Missing ⚠️
...eor/payment-gateway-initialize-session/use-case.ts 72.22% 5 Missing ⚠️
.../saleor/transaction-initialize-session/use-case.ts 92.98% 4 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

import { z } from "zod";

export const PaymentGatewayInitializeResponseDataShape = z.object({
stripePublishableKey: z.string(),
Copy link
Member

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

Copy link
Member Author

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
Copy link
Member

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

Comment on lines 30 to 32
const response = new SaleorApiUrlCreateErrorResponse({
error: saleorApiUrlResult.error,
});
Copy link
Member

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";
Copy link
Member

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,
Copy link
Member

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?

Copy link
Member Author

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 }) {
Copy link
Member

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 {
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why zero?

@vercel vercel bot temporarily deployed to Preview – saleor-app-klaviyo April 11, 2025 11:58 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-avatax April 11, 2025 11:58 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-products-feed April 11, 2025 11:58 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-cms April 11, 2025 11:58 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-klaviyo April 12, 2025 15:21 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-cms April 12, 2025 15:21 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-smtp April 12, 2025 15:21 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-avatax April 12, 2025 15:21 Inactive
class Success extends SuccessWebhookResponse {
responseData: ResponseDataType;

constructor(args: { responseData: ResponseDataType }) {
Copy link
Member

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

Comment on lines 3 to 9
export const responseData = z
.object({
stripePublishableKey: z.string(),
})
.brand("PaymentGatewayInitializeSessionResponseData");

export type ResponseDataType = z.infer<typeof responseData>;
Copy link
Member

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:

  1. Make export names explicit like PaymentGatewayInitializeSessionResponseDataType
  2. Or setup dependency cruiser to ensure file from route folder can't import file from another route (which iMO we should do anyway!)

Comment on lines 63 to 65
responseData: responseData.parse({
stripePublishableKey: pk.keyValue,
}),
Copy link
Member

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

  1. You should pass directly pk to Success() constructor
  2. 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

Copy link
Member

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

Comment on lines 15 to 20
export const stripePaymentIntentId = z
.string({
required_error: "Payment intent id is required",
})
.startsWith("pi_")
.brand("StripePaymentIntentId");
Copy link
Member

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

Comment on lines 1 to 7
export class SuccessWebhookResponse {
statusCode = 200;
}

export class ErrorWebhookResponse {
statusCode = 500;
}
Copy link
Member

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

@vercel vercel bot temporarily deployed to Preview – saleor-app-avatax April 13, 2025 13:10 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-search April 13, 2025 13:10 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-klaviyo April 13, 2025 13:10 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-smtp April 13, 2025 13:10 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-products-feed April 13, 2025 13:10 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-cms April 13, 2025 13:10 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-segment April 13, 2025 13:10 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-avatax April 13, 2025 14:55 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-cms April 13, 2025 14:55 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-segment April 13, 2025 14:55 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-klaviyo April 13, 2025 14:55 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-products-feed April 13, 2025 14:55 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-smtp April 13, 2025 14:55 Inactive
@vercel vercel bot temporarily deployed to Preview – saleor-app-search April 13, 2025 14:55 Inactive

describe("PaymentGatewayInitializeSessionUseCaseResponses", () => {
describe("Success", () => {
it("should return fetch API response with status code and message", async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;
Copy link
Member

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 () => {
Copy link
Member

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 {
Copy link
Member

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

@krzysztofzuraw krzysztofzuraw merged commit 952a108 into main Apr 14, 2025
29 checks passed
@krzysztofzuraw krzysztofzuraw deleted the webhook-response-result branch April 14, 2025 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Payment: Stripe skip changeset Attach this label to PRs which does not need changes description for the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants