feat(billing): test charge for free trial#1898
Merged
Conversation
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughAdds wallet-aware 3D Secure (3DS) flows across API and SDK: confirmPayment and start-trial now return structured 200/202 responses for immediate vs 3DS-required flows; payment methods gain a validated flag and new validation/test-charge flows; new post-3DS validation endpoint and wallet/trial gating via UserWalletRepository and StripeErrorService. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant FE as Client
participant API as API Gateway
participant WC as WalletController
participant SS as StripeService
participant ST as Stripe
FE->>API: POST /v1/start-trial
API->>WC: create()
WC->>SS: getPaymentMethods(userId, customerId)
WC->>SS: validatePaymentMethodForTrial({customer, payment_method, userId})
SS->>ST: create test auth / payment_intent
alt requires3DS
ST-->>SS: requires_action + client_secret + intent_id
SS-->>WC: { success, requires3DS:true, clientSecret, paymentIntentId, paymentMethodId }
WC-->>API: Wallet payload + 3DS fields
API-->>FE: 202 Accepted (3DS info)
else validated
SS-->>WC: { success, requires3DS:false }
WC-->>API: Wallet payload
API-->>FE: 200 OK
end
sequenceDiagram
autonumber
participant FE as Client
participant API as API Gateway
participant SC as StripeController
participant SS as StripeService
participant ST as Stripe
FE->>API: POST /v1/stripe/transactions/confirm
API->>SC: confirmPayment(params)
SC->>SS: createPaymentIntent(...)
SS->>ST: confirm/create PaymentIntent
alt requires_action
ST-->>SS: requires_action + client_secret
SS-->>SC: { success:true, requiresAction:true, clientSecret, paymentIntentId }
SC-->>API: 202 + payload
API-->>FE: 202 Accepted (3DS)
else succeeded
ST-->>SS: succeeded + id
SS-->>SC: { success:true, paymentIntentId }
SC-->>API: 200 + payload
API-->>FE: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
❌ 15 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 24 ❄️ flaky tests
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/http-sdk/src/stripe/stripe.types.ts (1)
34-45: Align SDK PaymentMethod.validated with API schemaapps/api/src/billing/http-schemas/stripe.schema.ts defines PaymentMethodSchema with validated: z.boolean().optional(); packages/http-sdk/src/stripe/stripe.types.ts declares validated: boolean — change the SDK to validated?: boolean to match (or remove .optional() from the API if validated must be required).
🧹 Nitpick comments (15)
packages/http-sdk/src/stripe/stripe.types.ts (3)
38-45: Breaking shape change: makevalidatedoptional or bump SDK version.Requiring
validated: booleanbreaks existing consumers unless the API now always populates it. Either make it optional or coordinate a version bump and migration.Apply if you choose non‑breaking change:
- validated: boolean; + validated?: boolean;
70-72: Duplicate interface declaration for SetupIntentResponse.This interface already exists at Lines 7–9. Remove the duplicate to avoid confusion.
Apply:
-export interface SetupIntentResponse { - clientSecret: string; -}
108-113: Prefer a discriminated union forConfirmPaymentResponse.This prevents consumers from handling impossible states (e.g., success=true with requiresAction=true).
-export interface ConfirmPaymentResponse { - success: boolean; - requiresAction?: boolean; - clientSecret?: string; - paymentIntentId?: string; -} +export type ConfirmPaymentResponse = + | { success: true; requiresAction?: false; paymentIntentId?: string } + | { success: false; requiresAction: true; clientSecret: string; paymentIntentId: string };apps/api/src/billing/services/stripe-error/stripe-error.service.ts (1)
183-208: Map “Amount must be greater than $0” to a stable frontend code.Currently this falls through to
unknown_payment_error. Map it to an explicit code to keep UX consistent.} else if (messageLower.includes("final amount after discount")) { return "final_amount_too_low"; + } else if (messageLower.includes("amount must be greater than")) { + return "minimum_payment_amount"; } else if (messageLower.includes("payment method does not belong")) { return "payment_method_not_owned";apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts (1)
71-95: Add documented error responses for 3DS validation.Expose 400/402/429 per StripeErrorService so clients can handle known cases.
responses: { description: "Payment method validated successfully", content: { "application/json": { schema: ValidatePaymentMethodResponseSchema } } } + , + 400: { description: "Validation error or card validation failed" }, + 402: { description: "Payment method declined" }, + 429: { description: "Duplicate validation request (rate limited)" } }apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
44-46: Ensure method ordering or prefer already‑validated card.
paymentMethods[0]assumes ordering. Prefer a validated method or sort bycreateddesc.- const paymentMethods = await this.stripeService.getPaymentMethods(currentUser.id, currentUser.stripeCustomerId); + const paymentMethods = await this.stripeService.getPaymentMethods(currentUser.id, currentUser.stripeCustomerId); assert(paymentMethods.length > 0, 400, "You must have a payment method to start a trial."); @@ - const latestPaymentMethod = paymentMethods[0]; + const latestPaymentMethod = + paymentMethods.find(pm => pm.validated) ?? + paymentMethods.slice().sort((a, b) => b.created - a.created)[0];apps/api/src/billing/controllers/stripe/stripe.controller.ts (3)
9-9: Import organization could be improved.Consider grouping related imports together. The ConfirmPaymentResponse import could be placed with other response type imports for better organization.
-import { ApplyCouponRequest, ConfirmPaymentRequest, ConfirmPaymentResponse, Discount, Transaction } from "@src/billing/http-schemas/stripe.schema"; +import { + ApplyCouponRequest, + ConfirmPaymentRequest, + ConfirmPaymentResponse, + Discount, + Transaction +} from "@src/billing/http-schemas/stripe.schema";Also applies to: 53-53
186-199: validatePaymentMethodAfter3DS needs input validation.The method should validate that both
paymentMethodIdandpaymentIntentIdare provided before processing.async validatePaymentMethodAfter3DS({ data: { paymentMethodId, paymentIntentId } }: { data: { paymentMethodId: string; paymentIntentId: string }; }): Promise<{ success: boolean }> { const { currentUser } = this.authService; assert(currentUser.stripeCustomerId, 400, "Stripe customer ID not found"); + assert(paymentMethodId, 400, "Payment method ID is required"); + assert(paymentIntentId, 400, "Payment intent ID is required"); await this.stripe.validatePaymentMethodAfter3DS(currentUser.stripeCustomerId, paymentMethodId, paymentIntentId); return { success: true }; }
126-129: Consider extracting error messages to constants.The error messages "Payment account not properly configured. Please contact support." appears multiple times. Consider extracting to a constant for consistency.
// At the top of the file const ERROR_MESSAGES = { PAYMENT_NOT_CONFIGURED: "Payment account not properly configured. Please contact support.", CANNOT_REMOVE_DURING_TRIAL: "Cannot remove payment method during trial. Please contact support." } as const;apps/api/src/billing/services/stripe/stripe.service.spec.ts (1)
1256-1264: TODO comments should be tracked in issues.The TODO comments for idempotency key conflict scenarios should be tracked as GitHub issues.
Would you like me to create GitHub issues to track the implementation of these idempotency key conflict test scenarios?
apps/api/src/billing/http-schemas/stripe.schema.ts (2)
64-69: Schema naming consistency issue:PaymentIntentResultSchemamixes structure with Stripe response namingThe schema name
PaymentIntentResultSchemadoesn't align with the consistent naming pattern used elsewhere in the file (e.g.,ConfirmPaymentResponseSchema,ValidatePaymentMethodResponseSchema). Consider renaming it to better reflect its purpose.Apply this diff to improve naming consistency:
-export const PaymentIntentResultSchema = z.object({ +export const PaymentIntentStatusSchema = z.object({ success: z.boolean(), requiresAction: z.boolean().optional(), clientSecret: z.string().optional(), paymentIntentId: z.string().optional() });Also update line 80:
export const ConfirmPaymentResponseSchema = z.object({ - data: PaymentIntentResultSchema + data: PaymentIntentStatusSchema });
217-218: Empty OpenAPI metadata on required fieldsThe
openapi({})calls on lines 217-218 are empty and don't provide any useful metadata for API documentation.Apply this diff to add meaningful OpenAPI documentation:
data: z.object({ - paymentMethodId: z.string().openapi({}), - paymentIntentId: z.string().openapi({}) + paymentMethodId: z.string().openapi({ + description: "Stripe payment method ID to validate", + example: "pm_1234567890" + }), + paymentIntentId: z.string().openapi({ + description: "Stripe payment intent ID from the 3DS flow", + example: "pi_1234567890" + }) })apps/api/src/billing/services/stripe/stripe.service.ts (3)
163-165: Potential UX issue: minimum payment amount logicThe minimum payment amount of $20 is only enforced when there are no discounts. This could lead to confusing user experiences where:
- User without discount: Must pay at least $20
- User with $19 discount on a $20 payment: Can pay $1 (below the minimum)
Consider whether this inconsistency is intentional or if the minimum should apply to the pre-discount amount consistently.
Could you clarify the business logic here? Should the minimum payment requirement apply to the original amount regardless of discounts, or is the current implementation intentional?
676-700: Race condition in idempotency error handlingThe retry logic after an idempotency error has a fixed 1-second delay that may not be sufficient in all cases. Additionally, the duplicate validation check logic appears twice (lines 639-648 and 684-694).
Apply this diff to reduce code duplication and improve retry logic:
+ private async checkPaymentMethodValidation(customerId: string, paymentMethodId: string): Promise<{ success: boolean; paymentIntentId?: string } | null> { + const user = await this.userRepository.findOneBy({ stripeCustomerId: customerId }); + if (user) { + const validatedPaymentMethods = await this.paymentMethodRepository.findValidatedByUserId(user.id); + if (validatedPaymentMethods.some(pm => pm.paymentMethodId === paymentMethodId)) { + logger.info({ + event: "PAYMENT_METHOD_ALREADY_VALIDATED", + customerId: customerId, + userId: user.id, + paymentMethodId: paymentMethodId + }); + return { success: true, paymentIntentId: "already_validated" }; + } + } + return null; + } async createTestCharge(params: { customer: string; payment_method: string; }): Promise<{ success: boolean; paymentIntentId?: string; requiresAction?: boolean; clientSecret?: string }> { - const user = await this.userRepository.findOneBy({ stripeCustomerId: params.customer }); - - if (user) { - const validatedPaymentMethods = await this.paymentMethodRepository.findValidatedByUserId(user.id); - if (validatedPaymentMethods.some(pm => pm.paymentMethodId === params.payment_method)) { - logger.info({ - event: "PAYMENT_METHOD_ALREADY_VALIDATED", - customerId: params.customer, - userId: user.id, - paymentMethodId: params.payment_method - }); - return { success: true, paymentIntentId: "already_validated" }; - } - } + const existingValidation = await this.checkPaymentMethodValidation(params.customer, params.payment_method); + if (existingValidation) { + return existingValidation; + } // ... rest of the method ... } catch (error) { // If this is an idempotency error, try to retrieve the existing payment intent if (error instanceof Stripe.errors.StripeError && error.code === "idempotency_key_in_use") { - // Wait a moment and try to retrieve the payment intent - await new Promise(resolve => setTimeout(resolve, 1000)); - - // Get the user to find any existing validations - const user = await this.userRepository.findOneBy({ stripeCustomerId: params.customer }); - if (user) { - // Check if payment method is already validated - const existingValidation = await this.paymentMethodRepository.findValidatedByUserId(user.id); - if (existingValidation.some(pm => pm.paymentMethodId === params.payment_method)) { - logger.info({ - event: "PAYMENT_METHOD_ALREADY_VALIDATED", - customerId: params.customer, - userId: user.id, - paymentMethodId: params.payment_method - }); - return { success: true, paymentIntentId: "already_validated" }; - } + // Implement exponential backoff for retries + for (let attempt = 0; attempt < 3; attempt++) { + await new Promise(resolve => setTimeout(resolve, Math.pow(2, attempt) * 1000)); + + const existingValidation = await this.checkPaymentMethodValidation(params.customer, params.payment_method); + if (existingValidation) { + return existingValidation; + } } throw new Error("Duplicate card validation request detected. Please wait before retrying.");
836-864: Consider transaction safety for validation markingThe
markPaymentMethodAsValidatedmethod swallows database errors (line 862), which could lead to inconsistent state where Stripe shows successful validation but the database doesn't reflect it.Consider either:
- Making this operation more critical and propagating the error
- Implementing a retry mechanism for transient database failures
- Adding a background job to reconcile validation states
} catch (validationError) { logger.error({ event: "PAYMENT_METHOD_VALIDATION_UPDATE_FAILED", customerId, paymentMethodId, error: validationError instanceof Error ? validationError.message : String(validationError) }); - // Don't fail the test charge if validation update fails - the card is still valid + // Retry once for transient failures + try { + await new Promise(resolve => setTimeout(resolve, 1000)); + const user = await this.userRepository.findOneBy({ stripeCustomerId: customerId }); + if (user) { + await this.paymentMethodRepository.markAsValidated(paymentMethodId, user.id); + logger.info({ + event: "PAYMENT_METHOD_VALIDATION_RETRY_SUCCESS", + customerId, + userId: user.id, + paymentMethodId + }); + } + } catch (retryError) { + logger.error({ + event: "PAYMENT_METHOD_VALIDATION_RETRY_FAILED", + customerId, + paymentMethodId, + error: retryError instanceof Error ? retryError.message : String(retryError) + }); + // Consider adding to a queue for background reconciliation + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/api/src/billing/controllers/stripe/stripe.controller.ts(6 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts(3 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(3 hunks)apps/api/src/billing/http-schemas/stripe.schema.ts(5 hunks)apps/api/src/billing/http-schemas/wallet.schema.ts(2 hunks)apps/api/src/billing/routes/start-trial/start-trial.router.ts(2 hunks)apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts(3 hunks)apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts(3 hunks)apps/api/src/billing/services/stripe-error/stripe-error.service.ts(3 hunks)apps/api/src/billing/services/stripe/stripe.service.spec.ts(9 hunks)apps/api/src/billing/services/stripe/stripe.service.ts(7 hunks)packages/http-sdk/src/stripe/stripe.service.ts(2 hunks)packages/http-sdk/src/stripe/stripe.types.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.tsapps/api/src/billing/routes/start-trial/start-trial.router.tsapps/api/src/billing/http-schemas/stripe.schema.tsapps/api/src/billing/controllers/stripe/stripe.controller.tsapps/api/src/billing/services/stripe/stripe.service.spec.tsapps/api/src/billing/services/stripe-error/stripe-error.service.tspackages/http-sdk/src/stripe/stripe.service.tsapps/api/src/billing/controllers/wallet/wallet.controller.spec.tsapps/api/src/billing/http-schemas/wallet.schema.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.tsapps/api/src/billing/services/stripe/stripe.service.tspackages/http-sdk/src/stripe/stripe.types.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.tsapps/api/src/billing/routes/start-trial/start-trial.router.tsapps/api/src/billing/http-schemas/stripe.schema.tsapps/api/src/billing/controllers/stripe/stripe.controller.tsapps/api/src/billing/services/stripe/stripe.service.spec.tsapps/api/src/billing/services/stripe-error/stripe-error.service.tspackages/http-sdk/src/stripe/stripe.service.tsapps/api/src/billing/controllers/wallet/wallet.controller.spec.tsapps/api/src/billing/http-schemas/wallet.schema.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.tsapps/api/src/billing/services/stripe/stripe.service.tspackages/http-sdk/src/stripe/stripe.types.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/billing/services/stripe/stripe.service.spec.tsapps/api/src/billing/controllers/wallet/wallet.controller.spec.ts
🧬 Code graph analysis (11)
apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts (1)
apps/api/src/billing/http-schemas/stripe.schema.ts (2)
ValidatePaymentMethodRequestSchema(215-220)ValidatePaymentMethodResponseSchema(222-224)
apps/api/src/billing/routes/start-trial/start-trial.router.ts (2)
apps/api/src/billing/http-schemas/wallet.schema.ts (1)
WalletResponseOutputSchema(25-27)apps/api/src/dashboard/routes/leases-duration/leases-duration.router.ts (1)
route(15-37)
apps/api/src/billing/http-schemas/stripe.schema.ts (1)
packages/http-sdk/src/stripe/stripe.types.ts (1)
ConfirmPaymentResponse(108-113)
apps/api/src/billing/controllers/stripe/stripe.controller.ts (4)
apps/api/src/billing/http-schemas/stripe.schema.ts (3)
PaymentMethod(233-233)ConfirmPaymentRequest(235-235)ConfirmPaymentResponse(238-238)packages/http-sdk/src/stripe/stripe.types.ts (2)
PaymentMethod(34-45)ConfirmPaymentResponse(108-113)apps/api/src/core/lib/semaphore.decorator.ts (1)
Semaphore(3-28)apps/api/src/auth/services/auth.service.ts (1)
Protected(39-55)
apps/api/src/billing/services/stripe/stripe.service.spec.ts (2)
apps/api/test/seeders/user.seeder.ts (1)
UserSeeder(5-45)apps/api/test/services/stub.ts (1)
stub(2-2)
packages/http-sdk/src/stripe/stripe.service.ts (2)
packages/http-sdk/src/stripe/stripe.types.ts (3)
ConfirmPaymentParams(74-79)ConfirmPaymentResponse(108-113)ThreeDSecureAuthParams(115-118)apps/api/src/billing/http-schemas/stripe.schema.ts (1)
ConfirmPaymentResponse(238-238)
apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts (3)
apps/api/test/seeders/user.seeder.ts (1)
UserSeeder(5-45)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlagValue(7-7)apps/api/test/seeders/payment-method.seeder.ts (1)
generatePaymentMethod(9-52)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (2)
apps/api/src/auth/services/auth.service.ts (2)
currentUser(12-14)currentUser(16-19)apps/api/src/billing/services/stripe/stripe.service.ts (1)
hasDuplicateTrialAccount(618-630)
apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts (1)
apps/api/src/billing/http-schemas/stripe.schema.ts (1)
ConfirmPaymentResponseSchema(79-81)
apps/api/src/billing/services/stripe/stripe.service.ts (2)
apps/api/src/billing/http-schemas/stripe.schema.ts (3)
PaymentMethod(233-233)PaymentIntentResult(236-236)PaymentMethodValidationResult(237-237)packages/http-sdk/src/stripe/stripe.types.ts (1)
PaymentMethod(34-45)
packages/http-sdk/src/stripe/stripe.types.ts (1)
apps/api/src/billing/http-schemas/stripe.schema.ts (1)
ConfirmPaymentResponse(238-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: validate (apps/deploy-web) / validate-unsafe
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (26)
packages/http-sdk/src/stripe/stripe.types.ts (1)
115-118: LGTM: Minimal post-3DS payload.The params are clear and typed.
apps/api/src/billing/services/stripe-error/stripe-error.service.ts (1)
67-78: New payment-method validation mappings: good coverage.429 for duplicate validation and 402/400 for decline/validation failures look right.
apps/api/src/billing/http-schemas/wallet.schema.ts (1)
25-31: OpenAPI uses the optional‑3DS wallet: good.Schema matches the router’s 200/202 branching.
apps/api/src/billing/routes/start-trial/start-trial.router.ts (2)
24-39: 200/202 branching in OpenAPI is correct.Clear description and consistent schema.
45-51: Runtime branching is correct.202 on
requires3DS, otherwise 200 is appropriate.apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts (1)
115-117: LGTM: Handler wiring.Validation uses typed body (
c.req.valid("json")) and delegates to controller cleanly.apps/api/src/billing/controllers/wallet/wallet.controller.ts (2)
75-80: Error normalization path looks good.Known payment errors are mapped; unknowns bubble up.
37-43: Confirm requester cannot start a trial for a different user.
userIdcomes from the body while Stripe checks usecurrentUser. Ensure policy/ability enforces equality or add an explicit check.Proposed guard:
async create({ data: { userId } }: StartTrialRequestInput): Promise<WalletOutputResponse> { const { currentUser } = this.authService; + assert(userId === currentUser.id, 403, "Forbidden");apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts (2)
29-45: LGTM! Well-structured 3DS response handling.The OpenAPI spec correctly differentiates between successful payment processing (200) and 3DS authentication requirements (202), both using the same response schema. This follows RESTful conventions.
94-108: Good implementation of 3DS branching logic.The controller correctly handles the 3DS flow by checking
result.data.requiresActionand returning appropriate HTTP status codes (202 for 3DS, 200 for success).packages/http-sdk/src/stripe/stripe.service.ts (1)
50-52: LGTM! New 3DS validation endpoint properly integrated.The new
validatePaymentMethodAfter3DSmethod follows the established patterns and provides proper typing.apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts (4)
92-92: Test error message updated correctly.The error message change from regex pattern to exact string match improves test precision.
136-168: Comprehensive 3DS scenario test coverage.Excellent test coverage for the 3DS flow, including proper assertions for the response structure with all required 3DS fields (clientSecret, paymentIntentId, paymentMethodId).
252-272: Test setup correctly mocks validatePaymentMethodForTrial scenarios.The mock implementation properly handles all test scenarios: validation failures, Stripe errors, 3DS requirements, and successful validation.
249-249: PaymentMethod includesvalidated— no change needed. The interface at packages/http-sdk/src/stripe/stripe.types.ts declaresvalidated: boolean.apps/api/src/billing/controllers/stripe/stripe.controller.ts (1)
64-89: Well-implemented 3DS flow with proper error handling.The implementation correctly:
- Creates the payment intent with confirmation
- Handles 3DS requirements with appropriate response structure
- Properly differentiates between 3DS required and payment failure cases
apps/api/src/billing/services/stripe/stripe.service.spec.ts (6)
561-566: Test data structure updated correctly for nested card object.The test properly updates to use the nested
cardobject structure withbrandandlast4properties.
1033-1087: Comprehensive test coverage for createTestCharge method.Excellent test coverage including:
- Successful $0 authorization
- Idempotency key generation
- User lookup and validation marking
- Proper amount handling (100 cents for validation)
1089-1115: 3DS authentication flow properly tested.The test correctly verifies that when
requires_actionstatus is returned, the response includes the necessary 3DS fields.
1284-1418: validatePaymentMethodForTrial tests are thorough.Excellent coverage of:
- Success scenarios
- 3DS requirements with proper payment intent creation
- Validation failures with appropriate error messages
- Error handling for 3DS payment intent creation
1421-1528: validatePaymentMethodAfter3DS tests cover all edge cases.Comprehensive test coverage including:
- Successful validation for both 'succeeded' and 'requires_capture' statuses
- Warning logs for unsuccessful payment intents
- Error handling for retrieval failures
- User not found scenarios
1580-1581: Good addition of refunds mocking in test setup.The addition of
refunds.createandrefunds.listmocks ensures comprehensive test coverage for refund-related operations.apps/api/src/billing/services/stripe/stripe.service.ts (4)
157-159: Good validation for payment amountThe validation ensuring the amount is greater than $0 prevents invalid payment attempts and provides a clear error message.
632-755: Well-structured test charge implementation with proper 3DS handlingThe
createTestChargemethod is well-implemented with:
- Proper validation check to avoid duplicate charges
- Idempotency key usage for preventing duplicates
- Comprehensive status handling for different payment intent states
- Clear logging at each step
- Proper error handling for idempotency conflicts
757-790: LGTM: Robust 3DS validation flowThe
validatePaymentMethodAfter3DSmethod properly handles the post-3DS validation with appropriate status checks and comprehensive error handling.
618-630: Verify callers accept Stripe.PaymentMethod[] (confirm getPaymentMethods return type)Found callers: apps/api/src/billing/controllers/wallet/wallet.controller.ts:47 (passes paymentMethods from stripeService.getPaymentMethods) and tests in apps/api/src/billing/services/stripe/stripe.service.spec.ts and apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts. Confirm stripeService.getPaymentMethods returns Stripe.PaymentMethod[] (or cast/convert the array before calling hasDuplicateTrialAccount) and update any other callers if present.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
apps/api/src/billing/routes/start-trial/start-trial.router.ts (3)
11-13: Add an explicit operationId (and consider documenting auth/idempotency).Helps client codegen and clarifies usage. If you use bearer auth and idempotency keys here, reflect them in the spec.
Apply this minimal addition:
method: "post", path: "/v1/start-trial", + operationId: "startTrial", summary: "Start a trial period for a user",
33-40: Make the 202 payload require 3DS fields.Use a dedicated schema so clients can rely on the presence of clientSecret/paymentIntentId/paymentMethodId when 202 is returned.
- 202: { - description: "3D Secure authentication required to complete trial setup", - content: { - "application/json": { - schema: WalletResponseOutputSchema - } - } - } + 202: { + description: "3D Secure authentication required to complete trial setup", + content: { + "application/json": { + schema: WalletResponse3DSOutputSchema + } + } + }
47-54: Minor: simplify branching and guard the property access.Less duplication and safer if the controller ever returns a slightly different shape.
- const result = await container.resolve(WalletController).create(c.req.valid("json")); - - if (result.data.requires3DS) { - return c.json(result, 202); - } - - return c.json(result, 200); + const result = await container.resolve(WalletController).create(c.req.valid("json")); + const status = result?.data?.requires3DS ? 202 : 200; + return c.json(result, status);Optional: hoist DI to avoid per‑request resolution (if the controller is stateless/singleton):
// near router initialization const walletController = container.resolve(WalletController); // ... const result = await walletController.create(c.req.valid("json"));apps/api/src/billing/routes/stripe-prices/stripe-prices.router.ts (1)
41-44: Avoid duplicate controller call.
findPrices()is invoked twice; call once and reuse the result.Apply:
-stripePricesRouter.openapi(route, async function routeStripePrices(c) { - await container.resolve(StripeController).findPrices(); - return c.json(await container.resolve(StripeController).findPrices(), 200); -}); +stripePricesRouter.openapi(route, async function routeStripePrices(c) { + const prices = await container.resolve(StripeController).findPrices(); + return c.json(prices, 200); +});apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts (2)
57-68: Delete route: good 204 semantics and clearer docs.Consider documenting possible error responses (e.g., 404 when
paymentMethodIdnot found) for API consumers.
77-103: New 3DS validation endpoint: route shape looks good.Request/response schemas are minimal and clear. Consider adding documented 400/409 cases for invalid/mismatched
paymentIntentId/paymentMethodIdto set expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
apps/api/src/billing/routes/start-trial/start-trial.router.ts(3 hunks)apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts(5 hunks)apps/api/src/billing/routes/stripe-prices/stripe-prices.router.ts(1 hunks)apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts(4 hunks)apps/api/test/services/wallet-testing.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/test/services/wallet-testing.service.tsapps/api/src/billing/routes/stripe-prices/stripe-prices.router.tsapps/api/src/billing/routes/start-trial/start-trial.router.tsapps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/test/services/wallet-testing.service.tsapps/api/src/billing/routes/stripe-prices/stripe-prices.router.tsapps/api/src/billing/routes/start-trial/start-trial.router.tsapps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts
🧬 Code graph analysis (2)
apps/api/src/billing/routes/start-trial/start-trial.router.ts (2)
apps/api/src/billing/http-schemas/wallet.schema.ts (1)
WalletResponseOutputSchema(25-27)apps/api/src/core/services/open-api-hono-handler/open-api-hono-handler.ts (1)
OpenApiHonoHandler(11-26)
apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts (1)
apps/api/src/billing/http-schemas/stripe.schema.ts (2)
ValidatePaymentMethodRequestSchema(215-220)ValidatePaymentMethodResponseSchema(222-224)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (6)
apps/api/src/billing/routes/stripe-prices/stripe-prices.router.ts (1)
23-30: OpenAPI docs: clear and helpful.Nice improvement to summary/description and 200 response text.
apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts (5)
5-10: Schemas import looks correct.Imports align with the new 3DS validation flow.
17-24: SetupIntent docs improved.Clear guidance on client secret usage.
37-39: Payment methods route docs improved.Accurately reflects validation status exposure.
71-73: 204 response code change is appropriate.Matches handler returning an empty body.
123-125: Handler wiring is correct.Using
c.req.valid("json")ensures schema-validated input.
- Add test charge endpoints to Stripe and wallet controllers - Implement test charge logic in Stripe service with comprehensive tests - Add HTTP schemas for test charge requests - Update payment method and transaction routes - Add Stripe error handling service - Update HTTP SDK with test charge support
baktun14
force-pushed
the
features/billing-test-charge
branch
from
76e4691 to
162529c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/src/billing/routes/stripe-prices/stripe-prices.router.ts (2)
42-44: Avoid double-invoking the controller (duplicate external call).
findPrices()is called twice; the first await is unused and the second repeats the call. This can double Stripe/DB traffic and produce inconsistent results if data changes between calls.Apply this diff:
-stripePricesRouter.openapi(route, async function routeStripePrices(c) { - await container.resolve(StripeController).findPrices(); - return c.json(await container.resolve(StripeController).findPrices(), 200); -}); +stripePricesRouter.openapi(route, async function routeStripePrices(c) { + const controller = container.resolve(StripeController); + const prices = await controller.findPrices(); + return c.json(prices, 200); +});
32-33: Confirm response shape matches OpenAPI schema — remove duplicate call & reconcile schema vs service output
- Controller already wraps the result: apps/api/src/billing/controllers/stripe/stripe.controller.ts (findPrices returns
{ data: ... }).- Route calls findPrices twice; remove the redundant await to avoid double invocation. (apps/api/src/billing/routes/stripe-prices/stripe-prices.router.ts:42–43).
- Service.findPrices returns items with unitAmount in major units (divides by 100), currency lowercased and includes isCustom boolean. (apps/api/src/billing/services/stripe/stripe.service.ts:88–91).
- OpenAPI schema only declares currency and unitAmount (apps/api/src/billing/routes/stripe-prices/stripe-prices.router.ts:8–16) and does not match the returned shape. Action: either update the schema to include isCustom and document the currency casing/unit semantics, or change the service to return the schema’s intended format (e.g., minor-unit integer for unitAmount, ISO‑4217 uppercase currency, and no undeclared fields).
packages/http-sdk/src/stripe/stripe.types.ts (1)
70-72: Remove duplicateSetupIntentResponsedeclaration.This interface is declared twice in this file. Duplicate declarations invite confusion even if they merge in TS.
-export interface SetupIntentResponse { - clientSecret: string; -}
♻️ Duplicate comments (3)
apps/api/src/billing/http-schemas/wallet.schema.ts (1)
18-23: Avoid sentinelid: 0; allowidto be null and enforce 3DS fields when required.This was flagged earlier; returning
0is brittle. Makeidnullable in the extended schema and require 3DS fields whenrequires3DSis true.-const WalletWithOptional3DSSchema = WalletOutputSchema.extend({ +const WalletWithOptional3DSSchema = WalletOutputSchema.extend({ + id: z.number().nullable(), requires3DS: z.boolean().optional(), clientSecret: z.string().optional(), paymentIntentId: z.string().optional(), paymentMethodId: z.string().optional() -}); +}).superRefine((val, ctx) => { + if (val.requires3DS) { + for (const k of ["clientSecret", "paymentIntentId", "paymentMethodId"] as const) { + if (!val[k]) { + ctx.addIssue({ code: z.ZodIssueCode.custom, path: [k], message: `${k} is required when requires3DS=true` }); + } + } + } +});apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
58-73: Don’t return fake wallet IDs or empty-string 3DS fields.Return
id: nulland omit empty-string fallbacks; rely on schema validation to ensure presence.- return { - data: { - id: 0, // Temporary ID for 3D Secure response + return { + data: { + id: null, // No wallet yet; awaiting 3DS completion userId: currentUser.id, address: null, creditAmount: 0, isTrialing: false, requires3DS: true, - clientSecret: validationResult.clientSecret || "", - paymentIntentId: validationResult.paymentIntentId || "", - paymentMethodId: validationResult.paymentMethodId || "" + clientSecret: validationResult.clientSecret!, + paymentIntentId: validationResult.paymentIntentId!, + paymentMethodId: validationResult.paymentMethodId! } };Note: Pair with the wallet schema change to allow
id: nulland enforce 3DS fields.apps/api/src/billing/services/stripe/stripe.service.ts (1)
787-829: Redundant payment intent creation in validatePaymentMethodForTrial.As noted in the past review comment, when
createTestChargereturnsrequiresAction: true, it already created a payment intent with 3DS requirement. Creating another payment intent (lines 796-811) results in duplicate charges.Apply this diff to reuse the existing payment intent:
async validatePaymentMethodForTrial(params: { customer: string; payment_method: string; userId: string }): Promise<PaymentMethodValidationResult> { const validationResult = await this.createTestCharge({ customer: params.customer, payment_method: params.payment_method }); // If the card requires 3D Secure authentication, create a new payment intent for 3DS if (validationResult.requiresAction) { - // Create a new payment intent specifically for 3D Secure authentication - const threeDSPaymentIntent = await this.paymentIntents.create({ - amount: 100, // $1.00 USD in cents - currency: "usd", - customer: params.customer, - payment_method: params.payment_method, - confirm: true, - capture_method: "manual", - automatic_payment_methods: { - enabled: true, - allow_redirects: "never" - }, - metadata: { - type: "payment_method_validation_3ds", - description: "Payment method validation with 3D Secure" - } - }); - return { success: false, requires3DS: true, - clientSecret: threeDSPaymentIntent.client_secret || "", - paymentIntentId: threeDSPaymentIntent.id, + clientSecret: validationResult.clientSecret || "", + paymentIntentId: validationResult.paymentIntentId || "", paymentMethodId: params.payment_method }; }
🧹 Nitpick comments (14)
apps/api/src/billing/routes/stripe-prices/stripe-prices.router.ts (2)
8-12: Tighten schema: currency format and amount units.Add basic constraints to prevent bad data from leaking into docs/clients (e.g., lowercased currency, floats).
Apply this diff:
-const StripePricesOutputSchema = z.object({ - currency: z.string().openapi({}), - unitAmount: z.number().openapi({}), - isCustom: z.boolean().openapi({}) -}); +const StripePricesOutputSchema = z.object({ + currency: z.string().regex(/^[A-Z]{3}$/).openapi({ + description: "ISO 4217 currency code (e.g., USD)" + }), + unitAmount: z.number().int().nonnegative().openapi({ + description: "Amount in minor units (e.g., cents)" + }), + isCustom: z.boolean().openapi({ + description: "True when the amount is user-defined" + }) +});
23-24: Micro copy/style nit: end descriptions with a period for consistency.Apply this diff:
- summary: "Get available Stripe pricing options", - description: "Retrieves the list of available pricing options for wallet top-ups, including custom amounts and standard pricing tiers", + summary: "Get available Stripe pricing options", + description: "Retrieves the list of available pricing options for wallet top-ups, including custom amounts and standard pricing tiers.", @@ - description: "Available pricing options retrieved successfully", + description: "Available pricing options retrieved successfully.",Also applies to: 29-29
packages/http-sdk/src/stripe/stripe.types.ts (2)
38-38: Consider makingvalidatedoptional to prevent API/SDK drift.If some endpoints don’t yet include this flag, making it required can be a breaking change for consumers of the SDK types.
If the server guarantees this field now, keep it required. Otherwise:
- validated: boolean; + validated?: boolean;
108-113: Model 3DS invariants with a discriminated union.confirmPayment simply forwards the API response (this.extractApiData(await this.post("/v1/stripe/transactions/confirm", ...))); encode the invariant that when requiresAction is true both clientSecret and paymentIntentId must be present.
-export interface ConfirmPaymentResponse { - success: boolean; - requiresAction?: boolean; - clientSecret?: string; - paymentIntentId?: string; -} +export type ConfirmPaymentResponse = + | { success: true } + | { success: false; requiresAction: true; clientSecret: string; paymentIntentId: string };apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
50-57: Be explicit about “latest” payment method selection.Stripe may not guarantee ordering; either sort by
createddesc or haveStripeService.getPaymentMethodsreturn the latest first and document it.apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts (3)
136-168: Update expectations if we switch toid: nulland drop empty-string fallbacks.Tests should reflect the recommended API shape (nullable id; required 3DS fields present, no empty strings).
- data: { - id: 0, + data: { + id: null, userId: user.id, address: null, creditAmount: 0, isTrialing: false, requires3DS: true, - clientSecret: "test_client_secret", - paymentIntentId: "test_payment_intent_id", - paymentMethodId: "test_payment_method_id" + clientSecret: "test_client_secret", + paymentIntentId: "test_payment_intent_id", + paymentMethodId: "test_payment_method_id" }
217-276: Isolate DI per test to avoid cross-test pollution.Using the global
containerrisks leakage between cases. Prefer a child container persetup.-import { container as rootContainer } from "tsyringe"; +import { container as rootContainer, DependencyContainer } from "tsyringe"; @@ - function setup(input?: { + function setup(input?: { user?: UserOutput; @@ - }) { - rootContainer.register(AuthService, { + }): DependencyContainer { + const container = rootContainer.createChildContainer(); + container.register(AuthService, { useValue: mock<AuthService>({ @@ - rootContainer.register(FeatureFlagsService, { + container.register(FeatureFlagsService, { @@ - rootContainer.register(WalletInitializerService, { + container.register(WalletInitializerService, { @@ - rootContainer.register(StripeService, { + container.register(StripeService, { @@ - return rootContainer; + return container; }Run tests once with this change to ensure no hidden coupling.
247-251: Minor: extra field on Stripe.PaymentMethod in test data.Adding
validated: trueto aStripe.PaymentMethodliteral may conflict with strict typings depending on mock inference. If TS complains, wrap with a cast at the call site or extend the local test type.- return [{ ...generatePaymentMethod(), validated: true }]; + return [{ ...(generatePaymentMethod() as unknown as Record<string, unknown>), validated: true }] as unknown as Stripe.PaymentMethod[];Or define a local
TestPaymentMethodthat extendsStripe.PaymentMethodwithvalidated?: boolean.apps/api/src/billing/controllers/stripe/stripe.controller.ts (2)
53-97: 3DS-aware confirm flow looks correct; align schema with invariants.Controller returns
{ data: { success: true } }or{ data: { success: false, requiresAction: true, clientSecret, paymentIntentId } }. Ensure the ZodConfirmPaymentResponseSchemaencodes this as a union so clients can rely on it.
186-199: Response shape inconsistency: return{ data: { success } }for 3DS validation.Most controller methods return a
{ data: ... }wrapper. Consider aligning this method for consistency (and update route/schema accordingly).- }): Promise<{ success: boolean }> { + }): Promise<{ data: { success: boolean } }> { @@ - return { success: true }; + return { data: { success: true } };apps/api/src/billing/services/stripe/stripe.service.spec.ts (2)
78-114: Test logic issue: Incorrect amount in fee validation.The test expects
refillService.topUpWalletto be called with 10000 cents, but the discount is 100% (line 99), so the amount after discount would be 0, not 10000. The test should verify that the original amount is topped up, not the discounted amount.Apply this diff to fix the test logic:
- expect(refillService.topUpWallet).toHaveBeenCalledWith(10000, user.id); + expect(refillService.topUpWallet).toHaveBeenCalledWith(10000, user.id); // Original amount is credited despite 100% discount
1531-1590: Consider using a more type-safe approach for the setup function.The setup function could benefit from explicit return type annotations and better type safety for the mocked services.
Consider adding explicit return type annotation:
-function setup() { +function setup(): { + service: StripeService; + userRepository: jest.Mocked<UserRepository>; + refillService: jest.Mocked<RefillService>; + billingConfig: jest.Mocked<BillingConfigService>; + paymentMethodRepository: jest.Mocked<PaymentMethodRepository>; +} {apps/api/src/billing/services/stripe/stripe.service.ts (2)
18-18: Remove unused constant MINIMUM_PAYMENT_AMOUNT.The constant is defined but never used in the code.
Remove the unused constant:
-const MINIMUM_PAYMENT_AMOUNT = 20; -
831-859: Consider improving error handling in markPaymentMethodAsValidated.The method silently continues if the validation update fails (line 857-858). While the comment explains the rationale, consider whether this should at least be tracked/monitored.
Consider adding metrics or alerting:
} catch (validationError) { logger.error({ event: "PAYMENT_METHOD_VALIDATION_UPDATE_FAILED", customerId, paymentMethodId, error: validationError instanceof Error ? validationError.message : String(validationError) }); // Don't fail the test charge if validation update fails - the card is still valid + // TODO: Consider adding metrics/alerting for validation update failures }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/api/test/functional/__snapshots__/docs.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
apps/api/src/billing/controllers/stripe/stripe.controller.ts(6 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts(3 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(3 hunks)apps/api/src/billing/http-schemas/stripe.schema.ts(5 hunks)apps/api/src/billing/http-schemas/wallet.schema.ts(2 hunks)apps/api/src/billing/routes/start-trial/start-trial.router.ts(3 hunks)apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts(5 hunks)apps/api/src/billing/routes/stripe-prices/stripe-prices.router.ts(1 hunks)apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts(4 hunks)apps/api/src/billing/services/stripe-error/stripe-error.service.ts(3 hunks)apps/api/src/billing/services/stripe/stripe.service.spec.ts(8 hunks)apps/api/src/billing/services/stripe/stripe.service.ts(7 hunks)apps/api/test/services/wallet-testing.service.ts(1 hunks)packages/http-sdk/src/stripe/stripe.service.ts(2 hunks)packages/http-sdk/src/stripe/stripe.types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/api/test/services/wallet-testing.service.ts
- apps/api/src/billing/services/stripe-error/stripe-error.service.ts
- packages/http-sdk/src/stripe/stripe.service.ts
- apps/api/src/billing/routes/stripe-payment-methods/stripe-payment-methods.router.ts
- apps/api/src/billing/routes/start-trial/start-trial.router.ts
- apps/api/src/billing/http-schemas/stripe.schema.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.tsapps/api/src/billing/routes/stripe-prices/stripe-prices.router.tsapps/api/src/billing/controllers/stripe/stripe.controller.tspackages/http-sdk/src/stripe/stripe.types.tsapps/api/src/billing/controllers/wallet/wallet.controller.spec.tsapps/api/src/billing/services/stripe/stripe.service.spec.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/services/stripe/stripe.service.tsapps/api/src/billing/http-schemas/wallet.schema.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.tsapps/api/src/billing/routes/stripe-prices/stripe-prices.router.tsapps/api/src/billing/controllers/stripe/stripe.controller.tspackages/http-sdk/src/stripe/stripe.types.tsapps/api/src/billing/controllers/wallet/wallet.controller.spec.tsapps/api/src/billing/services/stripe/stripe.service.spec.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/services/stripe/stripe.service.tsapps/api/src/billing/http-schemas/wallet.schema.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/billing/controllers/wallet/wallet.controller.spec.tsapps/api/src/billing/services/stripe/stripe.service.spec.ts
🧬 Code graph analysis (7)
apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts (1)
apps/api/src/billing/http-schemas/stripe.schema.ts (1)
ConfirmPaymentResponseSchema(79-81)
apps/api/src/billing/controllers/stripe/stripe.controller.ts (2)
apps/api/src/billing/http-schemas/stripe.schema.ts (3)
PaymentMethod(233-233)ConfirmPaymentRequest(235-235)ConfirmPaymentResponse(238-238)packages/http-sdk/src/stripe/stripe.types.ts (2)
PaymentMethod(34-45)ConfirmPaymentResponse(108-113)
packages/http-sdk/src/stripe/stripe.types.ts (1)
apps/api/src/billing/http-schemas/stripe.schema.ts (1)
ConfirmPaymentResponse(238-238)
apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts (3)
apps/api/test/seeders/user.seeder.ts (1)
UserSeeder(5-45)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlagValue(7-7)apps/api/test/seeders/payment-method.seeder.ts (1)
generatePaymentMethod(9-52)
apps/api/src/billing/services/stripe/stripe.service.spec.ts (2)
apps/api/test/seeders/user.seeder.ts (1)
UserSeeder(5-45)apps/api/test/services/stub.ts (1)
stub(2-2)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
apps/api/src/billing/services/stripe/stripe.service.ts (1)
hasDuplicateTrialAccount(613-625)
apps/api/src/billing/services/stripe/stripe.service.ts (2)
apps/api/src/billing/http-schemas/stripe.schema.ts (3)
PaymentMethod(233-233)PaymentIntentResult(236-236)PaymentMethodValidationResult(237-237)packages/http-sdk/src/stripe/stripe.types.ts (1)
PaymentMethod(34-45)
🪛 GitHub Check: validate / validate-app
apps/api/src/billing/services/stripe/stripe.service.ts
[failure] 18-18:
'MINIMUM_PAYMENT_AMOUNT' is assigned a value but never used
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (22)
packages/http-sdk/src/stripe/stripe.types.ts (1)
115-118: LGTM on 3DS params.
ThreeDSecureAuthParamsis concise and aligns with controller usage.apps/api/src/billing/http-schemas/wallet.schema.ts (3)
11-16: LGTM on 3DS schema.Base 3DS schema shape looks good.
25-31: Good upgrade of response schemas to the 3DS-aware wallet variant.
39-41: Type exports look consistent.apps/api/src/billing/controllers/wallet/wallet.controller.ts (4)
16-16: StripeErrorService injection is a solid addition.
32-32: Constructor update LGTM.
44-46: Improved precondition and new getPaymentMethods signature.Clearer assertion and userId-aware call look good.
75-80: Graceful Stripe error mapping is 👍.apps/api/src/billing/routes/stripe-transactions/stripe-transactions.router.ts (3)
19-21: Expanded route description reads well and sets expectations.
33-47: OpenAPI: Good 200/202 separation with a concrete response schema.Nice upgrade from empty 200 bodies; ensure client SDKs are updated to consume the JSON body.
98-111: Runtime branching on 3DS is correct and uses 202 appropriately.apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts (1)
92-93: Assertion message alignment is fine.apps/api/src/billing/controllers/stripe/stripe.controller.ts (2)
40-49: Payment methods endpoint now user-aware — good change.
122-131: Blocking removal of payment methods during trial is appropriate.Also applies to: 126-130
apps/api/src/billing/services/stripe/stripe.service.spec.ts (5)
856-856: Good update for the new API signature.The test correctly reflects the updated
getPaymentMethodssignature that now requires bothuserIdandcustomerId.
941-971: Well-structured tests for trialing wallet validation.The test suite correctly validates the fingerprint-based duplicate detection for trialing wallets, including proper handling of edge cases like null fingerprints.
1033-1282: Comprehensive test coverage for createTestCharge.The test suite thoroughly covers various scenarios including successful validation, 3DS requirements, card declines, and edge cases. The idempotency key generation and error handling are well-tested.
1284-1419: Good coverage for validatePaymentMethodForTrial.The tests appropriately cover the success path, 3DS flow, validation failures, and error handling scenarios.
1421-1528: Thorough tests for validatePaymentMethodAfter3DS.The test suite covers all important scenarios including successful validation, requires_capture status, non-successful intents, and error cases.
apps/api/src/billing/services/stripe/stripe.service.ts (3)
627-750: Well-implemented test charge with idempotency.The
createTestChargemethod correctly implements idempotency key handling, proper authorization without capture, and comprehensive status handling. The logic properly checks for existing validations to avoid duplicate charges.
752-785: Good implementation of 3DS validation completion.The
validatePaymentMethodAfter3DSmethod correctly handles the post-3DS validation flow with proper status checks and error handling.
99-113: Good implementation of payment method validation flag.The
getPaymentMethodsmethod correctly merges Stripe data with database validation status and sorts by creation time.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
58-73: Don’t return fake wallet id or empty-string 3DS fields.Return id: null and avoid "" fallbacks; make schema accept null and non-empty 3DS fields. This mirrors prior feedback.
- id: 0, + id: null, @@ - clientSecret: validationResult.clientSecret || "", - paymentIntentId: validationResult.paymentIntentId || "", - paymentMethodId: validationResult.paymentMethodId || "" + clientSecret: validationResult.clientSecret!, + paymentIntentId: validationResult.paymentIntentId!, + paymentMethodId: validationResult.paymentMethodId!#!/bin/bash # Verify schema allows id: null and 3DS fields rg -nP -C2 --type=ts 'wallet\.schema\.ts|Wallet.*schema' apps/api/src | cat rg -nP -C2 --type=ts '\brequires3DS\b|clientSecret|paymentIntentId|paymentMethodId' apps/api/src | catapps/api/src/billing/services/stripe/stripe.service.ts (1)
183-186: Amount validation needs adjustment for Stripe's minimumPer the past review comment, Stripe requires a minimum charge of $0.50 USD. The current validation only checks for amounts greater than $1 after discount, missing the $0.50-$0.99 range.
Apply this diff to properly validate against Stripe's minimum:
const finalAmountDollars = finalAmountCents / 100; -if (finalAmountDollars > 0 && finalAmountDollars < 1) { - throw new Error("Final amount after discount must be at least $1"); +if (finalAmountDollars > 0 && finalAmountDollars < 0.50) { + throw new Error("Final amount after discount must be at least $0.50 USD"); }
🧹 Nitpick comments (12)
apps/api/test/seeders/stripe-test-data.seeder.ts (3)
59-66: Standardize timestamp units (seconds vs milliseconds).Here created is in seconds (1234567890). Other seeders (e.g., createStripeTransaction default) use ms. Pick one unit project‑wide (prefer Stripe’s seconds) and stick to it.
Also applies to: 65-65
128-134: Add explicit return type to avoid implicit any leakage.Make the function’s return type explicit.
-export function createTestPromotionCode(overrides: Partial<Stripe.PromotionCode> = {}) { +export function createTestPromotionCode(overrides: Partial<Stripe.PromotionCode> = {}): Stripe.PromotionCode {
136-142: Add explicit return type to avoid implicit any leakage.Same here for Coupon.
-export function createTestCoupon(overrides: Partial<Stripe.Coupon> = {}) { +export function createTestCoupon(overrides: Partial<Stripe.Coupon> = {}): Stripe.Coupon {apps/api/test/seeders/user-test.seeder.ts (1)
4-10: Broaden overrides type and add explicit return type.Current type only permits id/stripeCustomerId; tests often need more. Derive types from UserSeeder.create to avoid drift.
-export function createTestUser(overrides: Partial<{ id: string; stripeCustomerId: string | null }> = {}) { - return UserSeeder.create({ +export function createTestUser( + overrides: NonNullable<Parameters<typeof UserSeeder.create>[0]> = {} +): ReturnType<typeof UserSeeder.create> { + return UserSeeder.create({ id: TEST_CONSTANTS.USER_ID, stripeCustomerId: TEST_CONSTANTS.CUSTOMER_ID, ...overrides }); }apps/api/test/seeders/stripe-transaction-test.seeder.ts (2)
7-9: Use consistent epoch units with the rest of the seeders.If Transaction.created is ms (as in createStripeTransaction default), set this to ms as well.
- created: 1640995200, + created: 1640995200000,
11-14: Normalize brand casing for consistency.Elsewhere we use “Visa”. Keep casing consistent across fixtures.
- brand: "visa", + brand: "Visa",apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
50-56: Ensure “latest” payment method is actually the latest.Index 0 assumes sorted DESC. Either sort by created or document getPaymentMethods’ ordering.
- const latestPaymentMethod = paymentMethods[0]; + const latestPaymentMethod = + [...paymentMethods].sort((a, b) => (b.created ?? 0) - (a.created ?? 0))[0];apps/api/src/billing/services/stripe/stripe.service.spec.ts (2)
1026-1026: Potential security leak in test assertionThe idempotency key assertion uses a regular expression that appears to directly embed sensitive IDs. While this is in a test file, the pattern could expose the actual format of production idempotency keys.
Consider using a more generic assertion pattern:
-idempotencyKey: expect.stringMatching(/^card_validation_cus_123_pm_123$/) +idempotencyKey: expect.stringMatching(/^card_validation_/)
1407-1407: Clarify test setup's default-user semanticsMake the setup helper's behavior explicit: omitting user should create a default test user and passing user: null should intentionally mean "no user" — document or change the helper API (e.g. add a createDefaultUser flag) and update tests to omit user for the default and use explicit null only when no user is expected. See apps/api/src/billing/services/stripe/stripe.service.spec.ts (setup usages around lines 1045, 1131 — user: null, 1334 — user: undefined, and the spy near 1407).
apps/api/src/billing/services/stripe/stripe.service.ts (3)
641-642: Consider adding rate limiting for idempotency keysThe idempotency key generation uses customer and payment method IDs, which is good for preventing duplicate charges. However, consider that if a user rapidly retries, they might still hit the Stripe API multiple times before the first request completes.
Consider adding a timestamp component or implementing application-level rate limiting to prevent rapid retries:
-const idempotencyKey = `card_validation_${params.customer}_${params.payment_method}`; +// Include hour timestamp to allow retries after reasonable time +const hourTimestamp = Math.floor(Date.now() / (1000 * 60 * 60)); +const idempotencyKey = `card_validation_${params.customer}_${params.payment_method}_${hourTimestamp}`;
668-669: Unnecessary delay in idempotency error handlingThe 1-second delay after an idempotency error doesn't serve a clear purpose and could slow down the user experience unnecessarily.
Remove the artificial delay:
-// Wait a moment and try to retrieve the payment intent -await new Promise(resolve => setTimeout(resolve, 1000));
746-779: Consider adding timeout for payment intent retrievalThe
validatePaymentMethodAfter3DSmethod retrieves a payment intent without any timeout. Since this is called after 3DS authentication, network issues could cause this to hang.Consider wrapping the Stripe API call with a timeout:
const paymentIntent = await Promise.race([ this.paymentIntents.retrieve(paymentIntentId), new Promise((_, reject) => setTimeout(() => reject(new Error('Payment intent retrieval timeout')), 10000) ) ]) as Stripe.PaymentIntent;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/src/billing/controllers/wallet/wallet.controller.ts(3 hunks)apps/api/src/billing/services/stripe-error/stripe-error.service.ts(2 hunks)apps/api/src/billing/services/stripe/stripe.service.spec.ts(38 hunks)apps/api/src/billing/services/stripe/stripe.service.ts(6 hunks)apps/api/test/seeders/index.ts(1 hunks)apps/api/test/seeders/stripe-test-data.seeder.ts(1 hunks)apps/api/test/seeders/stripe-transaction-test.seeder.ts(1 hunks)apps/api/test/seeders/user-test.seeder.ts(1 hunks)apps/api/test/services/wallet-testing.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/billing/services/stripe-error/stripe-error.service.ts
- apps/api/test/services/wallet-testing.service.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/test/seeders/user-test.seeder.tsapps/api/test/seeders/index.tsapps/api/test/seeders/stripe-transaction-test.seeder.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/test/seeders/stripe-test-data.seeder.tsapps/api/src/billing/services/stripe/stripe.service.spec.tsapps/api/src/billing/services/stripe/stripe.service.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/test/seeders/user-test.seeder.tsapps/api/test/seeders/index.tsapps/api/test/seeders/stripe-transaction-test.seeder.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/test/seeders/stripe-test-data.seeder.tsapps/api/src/billing/services/stripe/stripe.service.spec.tsapps/api/src/billing/services/stripe/stripe.service.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/billing/services/stripe/stripe.service.spec.ts
🧬 Code graph analysis (6)
apps/api/test/seeders/user-test.seeder.ts (2)
apps/api/test/seeders/user.seeder.ts (1)
UserSeeder(5-45)apps/api/test/seeders/stripe-test-data.seeder.ts (1)
TEST_CONSTANTS(5-12)
apps/api/test/seeders/stripe-transaction-test.seeder.ts (2)
apps/api/test/seeders/stripe-transaction.seeder.ts (1)
createStripeTransaction(7-29)apps/api/test/seeders/payment-method.seeder.ts (1)
generatePaymentMethod(9-52)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
apps/api/src/billing/services/stripe/stripe.service.ts (1)
hasDuplicateTrialAccount(607-619)
apps/api/test/seeders/stripe-test-data.seeder.ts (2)
apps/api/src/billing/services/stripe/stripe.service.ts (1)
createTestCharge(621-744)packages/http-sdk/src/stripe/stripe.types.ts (1)
Charge(47-57)
apps/api/src/billing/services/stripe/stripe.service.spec.ts (5)
apps/api/test/seeders/user-test.seeder.ts (1)
createTestUser(4-10)apps/api/test/seeders/stripe-test-data.seeder.ts (5)
TEST_CONSTANTS(5-12)createTestCharge(59-126)createTestPromotionCode(128-134)createTestCoupon(136-142)createTestPaymentIntent(14-57)apps/api/src/billing/services/stripe/stripe.service.ts (1)
createTestCharge(621-744)apps/api/test/seeders/stripe-transaction-test.seeder.ts (1)
createTestTransaction(4-18)apps/api/test/seeders/payment-method.seeder.ts (1)
generatePaymentMethod(9-52)
apps/api/src/billing/services/stripe/stripe.service.ts (2)
packages/http-sdk/src/stripe/stripe.types.ts (1)
PaymentMethod(34-45)apps/api/src/billing/http-schemas/stripe.schema.ts (3)
PaymentMethod(233-233)PaymentIntentResult(236-236)PaymentMethodValidationResult(237-237)
🪛 Gitleaks (8.27.2)
apps/api/src/billing/services/stripe/stripe.service.spec.ts
[high] 1042-1042: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (8)
apps/api/test/seeders/stripe-test-data.seeder.ts (1)
14-56: Align capture_method with service behavior (manual auth for validation).Service createTestCharge uses manual capture for $1 auth. Consider defaulting seeder PaymentIntent to capture_method: "manual" to better mirror prod behavior.
Apply:
- capture_method: "automatic", + capture_method: "manual",apps/api/test/seeders/index.ts (1)
17-19: Re-exports look good.Barrel now exposes the new seeders cleanly.
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
44-46: Signature & callers verified — no action required.Server-side: apps/api/src/billing/services/stripe/stripe.service.ts implements getPaymentMethods(userId, customerId) and all server callers use the two-arg form (apps/api/src/billing/controllers/stripe/stripe.controller.ts, apps/api/src/billing/controllers/wallet/wallet.controller.ts, apps/api/src/billing/services/stripe/stripe.service.spec.ts). Frontend/http-sdk still exposes a zero-arg getPaymentMethods (packages/http-sdk/src/stripe/stripe.service.ts) that calls the API — expected.
apps/api/src/billing/services/stripe/stripe.service.spec.ts (1)
1229-1229: LGTM! Good test for preventing duplicate payment intent creationThe test correctly verifies that only one payment intent is created when 3DS is required, preventing duplicate charges as highlighted in the past review comments.
apps/api/src/billing/services/stripe/stripe.service.ts (4)
781-805: Successfully resolved: No duplicate payment intent creationThe implementation correctly reuses the existing payment intent from
createTestChargewhen 3DS is required, addressing the duplicate charge concern from past reviews.
807-835: Good error handling in payment method validationThe
markPaymentMethodAsValidatedmethod properly handles errors without failing the entire validation process, which is appropriate since the card has already been validated through Stripe.
621-744: Well-implemented test charge with comprehensive status handlingThe
createTestChargemethod properly handles all payment intent statuses, implements idempotency to prevent duplicate charges, and correctly validates payment methods. The $1.00 test charge with manual capture is an appropriate approach for card validation.
97-111: Excellent implementation of payment method validation statusThe updated
getPaymentMethodssignature now includes the userId parameter and returns payment methods with their validation status, properly merging Stripe data with database records.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
37-38: Authorization gap: userId from body can target a different user.A caller could start a trial for another user (wallet created for body.userId while validations use currentUser). Enforce identity.
Apply this diff:
async create({ data: { userId } }: StartTrialRequestInput): Promise<WalletOutputResponse> { const { currentUser } = this.authService; + assert(userId === currentUser.id, 403, "Cannot start a trial for another user");Optionally, deprecate userId in the request and derive it from auth.
Also applies to: 83-85
♻️ Duplicate comments (1)
apps/api/src/billing/routes/start-trial/start-trial.router.ts (1)
25-33: 200/202 schema split: LGTM and matches the intended strict contracts.Explicit 200 (non‑3DS) vs 202 (3DS) schemas are correctly wired. This addresses the earlier concern about permissive success shapes.
Also applies to: 33-40
🧹 Nitpick comments (8)
apps/api/src/billing/routes/start-trial/start-trial.router.ts (1)
47-54: Guard the branch with an explicit boolean check.Use a strict check to avoid accidental truthy values and future refactors introducing non-boolean values.
Apply this diff:
- if (result.data.requires3DS) { + if (result.data?.requires3DS === true) { return c.json(result, 202); }apps/api/src/billing/controllers/wallet/wallet.controller.ts (2)
50-51: Pick the latest/default payment method deterministically.Relying on index 0 is order‑dependent. Prefer newest by created (or default flag if you maintain one).
Apply this diff:
- const latestPaymentMethod = paymentMethods[0]; + const latestPaymentMethod = [...paymentMethods].sort((a, b) => b.created - a.created)[0];
68-71: Avoid|| nullfallbacks for 3DS fields.
||collapses empty strings to null and can violate the 202 schema predicates. Return the raw values (and let schema enforce non‑empty).Apply this diff (pairs with the schema tweak below to require non‑empty strings on 202):
- clientSecret: validationResult.clientSecret || null, - paymentIntentId: validationResult.paymentIntentId || null, - paymentMethodId: validationResult.paymentMethodId || null + clientSecret: validationResult.clientSecret, + paymentIntentId: validationResult.paymentIntentId, + paymentMethodId: validationResult.paymentMethodIdapps/api/src/billing/controllers/wallet/wallet.controller.spec.ts (2)
217-277: Make setup return the SUT and isolate DI via a child container.Prevents shared state across tests and aligns with “setup returns the object under test”.
Apply this diff:
- function setup(input?: { + function setup(input?: { user?: UserOutput; enabledFeatures?: FeatureFlagValue[]; hasPaymentMethods?: boolean; hasDuplicateTrialAccount?: boolean; requires3DS?: boolean; validationFails?: boolean; stripeError?: boolean; }) { - rootContainer.register(AuthService, { + const container = rootContainer.createChildContainer(); + container.register(AuthService, { useValue: mock<AuthService>({ ability: createMongoAbility<MongoAbility>([ @@ - rootContainer.register(FeatureFlagsService, { + container.register(FeatureFlagsService, { useValue: mock<FeatureFlagsService>({ isEnabled: jest.fn().mockImplementation(flag => !!input?.enabledFeatures?.includes(flag)) }) }); - rootContainer.register(WalletInitializerService, { + container.register(WalletInitializerService, { useValue: mock<WalletInitializerService>() }); - rootContainer.register(StripeService, { + container.register(StripeService, { useValue: mock<StripeService>({ getPaymentMethods: jest.fn(async () => { if (!input?.hasPaymentMethods) return []; - return [{ ...generatePaymentMethod(), validated: true }]; + return [{ ...generatePaymentMethod(), validated: true } as Stripe.PaymentMethod & { validated: true }]; }), hasDuplicateTrialAccount: jest.fn().mockResolvedValue(input?.hasDuplicateTrialAccount ?? false), validatePaymentMethodForTrial: jest.fn().mockImplementation(() => { @@ }) }) }); - - return rootContainer; + // Also provide a basic StripeErrorService mock to avoid resolving real dependencies + container.register("StripeErrorService" as any, { useValue: mock<{ isKnownError: any; toAppError: any }>({ + isKnownError: jest.fn().mockReturnValue(true), + toAppError: jest.fn((e: Error) => new Error(e.message)) + })}); + + return container; }Then update call sites to use the returned container as you already do (no further changes needed).
246-274: Type-safety nit: return subtype with extra field while preserving declared return type.Casting the enriched object to an intersection avoids excess‑property errors without resorting to any.
Apply this diff (included in the setup refactor above):
- return [{ ...generatePaymentMethod(), validated: true }]; + return [{ ...generatePaymentMethod(), validated: true } as Stripe.PaymentMethod & { validated: true }];apps/api/src/billing/http-schemas/wallet.schema.ts (3)
29-35: Make 200-response refinements explicit (nullish only), not generic falsy.Current checks accept empty strings. Require null/undefined explicitly to match the description.
Apply this diff:
export const WalletResponseNo3DSOutputSchema = z.object({ data: WalletWithOptional3DSSchema.strict() .refine(data => !data.requires3DS, { message: "requires3DS must be false or undefined for 200 responses" }) - .refine(data => !data.clientSecret, { message: "clientSecret must be null or undefined for 200 responses" }) - .refine(data => !data.paymentIntentId, { message: "paymentIntentId must be null or undefined for 200 responses" }) - .refine(data => !data.paymentMethodId, { message: "paymentMethodId must be null or undefined for 200 responses" }) + .refine(data => data.clientSecret == null, { message: "clientSecret must be null or undefined for 200 responses" }) + .refine(data => data.paymentIntentId == null, { message: "paymentIntentId must be null or undefined for 200 responses" }) + .refine(data => data.paymentMethodId == null, { message: "paymentMethodId must be null or undefined for 200 responses" }) });
37-43: Enforce non-empty strings on 202 fields.Ensure 3DS strings are present and non-empty; pairs with controller change to avoid
|| null.Apply this diff:
export const WalletResponse3DSOutputSchema = z.object({ data: WalletWithOptional3DSSchema.strict() .refine(data => data.requires3DS === true, { message: "requires3DS must be true for 202 responses" }) - .refine(data => data.clientSecret !== null && data.clientSecret !== undefined, { message: "clientSecret is required for 202 responses" }) - .refine(data => data.paymentIntentId !== null && data.paymentIntentId !== undefined, { message: "paymentIntentId is required for 202 responses" }) - .refine(data => data.paymentMethodId !== null && data.paymentMethodId !== undefined, { message: "paymentMethodId is required for 202 responses" }) + .refine(data => typeof data.clientSecret === "string" && data.clientSecret.length > 0, { message: "clientSecret is required for 202 responses" }) + .refine(data => typeof data.paymentIntentId === "string" && data.paymentIntentId.length > 0, { message: "paymentIntentId is required for 202 responses" }) + .refine(data => typeof data.paymentMethodId === "string" && data.paymentMethodId.length > 0, { message: "paymentMethodId is required for 202 responses" }) });
11-16: Optional: reuse ThreeDSecureAuthSchema to avoid duplication.You can compose
WalletWithOptional3DSSchemaviaThreeDSecureAuthSchema.partial()to keep fields DRY.Apply this diff:
-const ThreeDSecureAuthSchema = z.object({ +const ThreeDSecureAuthSchema = z.object({ requires3DS: z.boolean(), - clientSecret: z.string(), - paymentIntentId: z.string(), - paymentMethodId: z.string() + clientSecret: z.string(), + paymentIntentId: z.string(), + paymentMethodId: z.string() }); -const WalletWithOptional3DSSchema = WalletOutputSchema.extend({ - requires3DS: z.boolean().optional(), - clientSecret: z.string().nullable().optional(), - paymentIntentId: z.string().nullable().optional(), - paymentMethodId: z.string().nullable().optional() -}); +const WalletWithOptional3DSSchema = WalletOutputSchema.merge( + ThreeDSecureAuthSchema.partial().extend({ + clientSecret: z.string().nullable().optional(), + paymentIntentId: z.string().nullable().optional(), + paymentMethodId: z.string().nullable().optional() + }) +);Also applies to: 18-23
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts(3 hunks)apps/api/src/billing/controllers/wallet/wallet.controller.ts(3 hunks)apps/api/src/billing/http-schemas/wallet.schema.ts(2 hunks)apps/api/src/billing/routes/start-trial/start-trial.router.ts(2 hunks)packages/http-sdk/src/stripe/stripe.types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/http-sdk/src/stripe/stripe.types.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use type any or cast to type any. Always define the proper TypeScript types.
Files:
apps/api/src/billing/routes/start-trial/start-trial.router.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/controllers/wallet/wallet.controller.spec.tsapps/api/src/billing/http-schemas/wallet.schema.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.{js,ts,tsx}: Never use deprecated methods from libraries.
Don't add unnecessary comments to the code
Files:
apps/api/src/billing/routes/start-trial/start-trial.router.tsapps/api/src/billing/controllers/wallet/wallet.controller.tsapps/api/src/billing/controllers/wallet/wallet.controller.spec.tsapps/api/src/billing/http-schemas/wallet.schema.ts
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/no-jest-mock.mdc)
Don't use
jest.mock()to mock dependencies in test files. Instead, usejest-mock-extendedto create mocks and pass mocks as dependencies to the service under test.
**/*.spec.{ts,tsx}: Usesetupfunction instead ofbeforeEachin test files
setupfunction must be at the bottom of the rootdescribeblock in test files
setupfunction creates an object under test and returns it
setupfunction should accept a single parameter with inline type definition
Don't use shared state insetupfunction
Don't specify return type ofsetupfunction
Files:
apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts
🧬 Code graph analysis (3)
apps/api/src/billing/routes/start-trial/start-trial.router.ts (2)
apps/api/src/billing/http-schemas/wallet.schema.ts (2)
WalletResponseNo3DSOutputSchema(29-35)WalletResponse3DSOutputSchema(37-43)apps/api/src/core/services/open-api-hono-handler/open-api-hono-handler.ts (1)
OpenApiHonoHandler(11-26)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
apps/api/src/billing/services/stripe/stripe.service.ts (1)
hasDuplicateTrialAccount(607-619)
apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts (3)
apps/api/test/seeders/user.seeder.ts (1)
UserSeeder(5-45)apps/api/src/core/services/feature-flags/feature-flags.ts (1)
FeatureFlagValue(7-7)apps/api/test/seeders/payment-method.seeder.ts (1)
generatePaymentMethod(9-52)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Validate local packages
🔇 Additional comments (3)
apps/api/src/billing/controllers/wallet/wallet.controller.ts (1)
74-80: Stripe error mapping: ensure DI is stable in tests.
stripeErrorServiceis injected and used here. Tests should register a mock to avoid accidental resolution of a real implementation.I can provide a small patch in the spec’s
setup()to registerStripeErrorServicewithjest-mock-extended(see spec comment below).apps/api/src/billing/controllers/wallet/wallet.controller.spec.ts (2)
117-134: Add a test to forbid cross‑user trial creation.Covers the authorization fix ensuring body.userId must match the authenticated user.
Apply this diff:
it("creates a wallet for a user with all valid requirements", async () => { @@ }); + + it("rejects when body.userId does not match the authenticated user", async () => { + const current = UserSeeder.create({ + emailVerified: true, + stripeCustomerId: faker.string.uuid() + }); + const other = UserSeeder.create(); + const container = setup({ + user: current, + hasPaymentMethods: true, + hasDuplicateTrialAccount: false + }); + const walletController = container.resolve(WalletController); + await expect(() => + walletController.create({ + data: { userId: other.id } + }) + ).rejects.toThrow(/cannot start a trial for another user/i); + });
193-214: Stabilize Stripe error mapping in tests.Given the controller uses
stripeErrorService, ensure your setup registers a mock (see setup refactor) so this test doesn’t depend on the real service behavior.
stalniy
approved these changes
Sep 17, 2025
stalniy
pushed a commit
that referenced
this pull request
Nov 20, 2025
* feat(billing): implement test charge functionality - Add test charge endpoints to Stripe and wallet controllers - Implement test charge logic in Stripe service with comprehensive tests - Add HTTP schemas for test charge requests - Update payment method and transaction routes - Add Stripe error handling service - Update HTTP SDK with test charge support * fix(billing): stripe service tests * fix(billing): fix tests * fix(billing): improve 3d secure and testing * fix(billing): update wallet response schemas and controller logic * fix(billing): update API docs response schemas to include nullable properties
This was referenced Nov 28, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#1886
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests