Create stripe customer IDs for users when syncing identity #348
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds Stripe customer creation and checkout-by-customer APIs, introduces Redis-based distributed locking to sync user profiles and conditional Stripe customer creation, adds a user-identity fetch helper and a new /isRequired route, and propagates redis/stripe/logger dependencies through sync and membership flows. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Membership as Membership Handler
participant Sync as Profile Sync
participant Redis as Redis Lock
participant Dynamo as DynamoDB
participant Identity as Identity Helper
participant Stripe as Stripe API
Client->>Membership: Request checkout
Membership->>Sync: syncFullProfile(netId..., redisClient, stripeApiKey, logger)
Sync->>Redis: Acquire lock for netId
activate Redis
alt Lock acquired
Sync->>Dynamo: UpdateItem / Get (ALL_NEW)
Dynamo-->>Sync: Return attributes
alt stripeCustomerId missing
Sync->>Stripe: createStripeCustomer(email, name, metadata)
Stripe-->>Sync: Return customerId
Sync->>Dynamo: Update stripeCustomerId (ALL_NEW)
Dynamo-->>Sync: Confirm update
end
Sync->>Redis: Release lock
else Lock abandoned / aborted
Sync-->>Membership: Throw InternalServerError("lock abandoned")
end
deactivate Redis
Sync-->>Membership: Return profile with stripeCustomerId
Membership->>Stripe: createCheckoutSessionWithCustomer(customerId, items...)
Stripe-->>Membership: Return session URL
Membership-->>Client: Return session URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (3)
Comment |
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/routes/v2/membership.ts (1)
124-133: Validate Stripe secret before invoking downstream Stripe calls.Prevent late failures with a clear error.
- const savePromise = syncFullProfile({ + if (!fastify.secretConfig.stripe_secret_key) { + request.log.error("Missing Stripe secret key."); + throw new InternalServerError({ message: "Stripe is not configured." }); + } + const savePromise = syncFullProfile({ uinHash, firstName: givenName, lastName: surname, netId, dynamoClient: fastify.dynamoClient, redisClient: fastify.redisClient, stripeApiKey: fastify.secretConfig.stripe_secret_key, logger: request.log, });
🧹 Nitpick comments (5)
src/api/functions/sync.ts (2)
82-82: Avoid logging PII (full email) at info level.Log NetID or a hash instead; keep full email to debug level only if necessary.
- logger.info(`Created new Stripe customer for ${userId}.`); + logger.info({ netId }, "Created new Stripe customer.");
5-11: ESLint import/extensions warnings.Either drop “.js” in TS imports or configure eslint-plugin-import for Node16/TypeScript resolution so ESM TS paths ending in .js are allowed.
src/api/functions/stripe.ts (1)
1-1: ESLint import/extensions warning.Same note as other files: either remove “.js” in TS imports or configure the resolver to accept ESM TS with .js suffix.
src/api/routes/v2/membership.ts (2)
14-17: Drop unused import.
createCheckoutSessionisn’t used.-import { - createCheckoutSession, - createCheckoutSessionWithCustomer, -} from "api/functions/stripe.js"; +import { createCheckoutSessionWithCustomer } from "api/functions/stripe.js";
159-177: Pass the already‑validated Stripe key; rely on requiredcustomerId.Minor cleanup once
customerIdis required in the helper.- await createCheckoutSessionWithCustomer({ + await createCheckoutSessionWithCustomer({ successUrl: "https://acm.illinois.edu/paid", returnUrl: "https://acm.illinois.edu/membership", - customerId: userData.stripeCustomerId, - stripeApiKey: fastify.secretConfig.stripe_secret_key as string, + customerId: userData.stripeCustomerId, + stripeApiKey: fastify.secretConfig.stripe_secret_key as string, items: [(Keep as-is; this note is to align with the stricter helper type.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
src/api/functions/stripe.ts(4 hunks)src/api/functions/sync.ts(2 hunks)src/api/routes/syncIdentity.ts(2 hunks)src/api/routes/v2/membership.ts(3 hunks)src/api/utils.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/api/functions/sync.ts
[error] 5-5: Unexpected use of file extension "js" for "api/types.js"
(import/extensions)
[error] 6-6: Unexpected use of file extension "js" for "common/config.js"
(import/extensions)
[error] 8-8: Unexpected use of file extension "js" for "./stripe.js"
(import/extensions)
[error] 9-9: Unexpected use of file extension "js" for "common/errors/index.js"
(import/extensions)
src/api/functions/stripe.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/qWpMiHHrJd'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 1-1: Unexpected use of file extension "js" for "api/utils.js"
(import/extensions)
src/api/routes/v2/membership.ts
[error] 17-17: Unexpected use of file extension "js" for "api/functions/stripe.js"
(import/extensions)
⏰ 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). (2)
- GitHub Check: Run Unit Tests
- GitHub Check: Build Application
🔇 Additional comments (2)
src/api/utils.ts (1)
46-46: LGTM.isProd is clear and side‑effect free.
src/api/routes/syncIdentity.ts (1)
129-129: Log phrasing change acknowledged.Message update is fine.
| }: StripeCheckoutSessionCreateWithCustomerParams): Promise<string> => { | ||
| const stripe = new Stripe(stripeApiKey); | ||
| const payload: Stripe.Checkout.SessionCreateParams = { | ||
| success_url: successUrl || "", | ||
| cancel_url: returnUrl || "", | ||
| payment_method_types: ["card"], | ||
| line_items: items.map((item) => ({ | ||
| price: item.price, | ||
| quantity: item.quantity, | ||
| })), | ||
| mode: "payment", | ||
| customer: customerId, | ||
| metadata: { | ||
| ...(metadata || {}), | ||
| initiator, | ||
| }, | ||
| allow_promotion_codes: allowPromotionCodes, | ||
| custom_fields: customFields, | ||
| }; | ||
| const session = await stripe.checkout.sessions.create(payload); | ||
| if (!session.url) { | ||
| throw new InternalServerError({ | ||
| message: "Could not create Stripe checkout session.", | ||
| }); | ||
| } | ||
| return session.url; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add a runtime guard for missing customerId.
Defensive check to avoid silent bad requests to Stripe.
export const createCheckoutSessionWithCustomer = async ({
successUrl,
returnUrl,
stripeApiKey,
customerId,
items,
initiator,
allowPromotionCodes,
customFields,
metadata,
}: StripeCheckoutSessionCreateWithCustomerParams): Promise<string> => {
+ if (!customerId) {
+ throw new ValidationError({ message: "Missing Stripe customerId." });
+ }
const stripe = new Stripe(stripeApiKey);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }: StripeCheckoutSessionCreateWithCustomerParams): Promise<string> => { | |
| const stripe = new Stripe(stripeApiKey); | |
| const payload: Stripe.Checkout.SessionCreateParams = { | |
| success_url: successUrl || "", | |
| cancel_url: returnUrl || "", | |
| payment_method_types: ["card"], | |
| line_items: items.map((item) => ({ | |
| price: item.price, | |
| quantity: item.quantity, | |
| })), | |
| mode: "payment", | |
| customer: customerId, | |
| metadata: { | |
| ...(metadata || {}), | |
| initiator, | |
| }, | |
| allow_promotion_codes: allowPromotionCodes, | |
| custom_fields: customFields, | |
| }; | |
| const session = await stripe.checkout.sessions.create(payload); | |
| if (!session.url) { | |
| throw new InternalServerError({ | |
| message: "Could not create Stripe checkout session.", | |
| }); | |
| } | |
| return session.url; | |
| }; | |
| }: StripeCheckoutSessionCreateWithCustomerParams): Promise<string> => { | |
| if (!customerId) { | |
| throw new ValidationError({ message: "Missing Stripe customerId." }); | |
| } | |
| const stripe = new Stripe(stripeApiKey); | |
| const payload: Stripe.Checkout.SessionCreateParams = { | |
| success_url: successUrl || "", | |
| cancel_url: returnUrl || "", | |
| payment_method_types: ["card"], | |
| line_items: items.map((item) => ({ | |
| price: item.price, | |
| quantity: item.quantity, | |
| })), | |
| mode: "payment", | |
| customer: customerId, | |
| metadata: { | |
| ...(metadata || {}), | |
| initiator, | |
| }, | |
| allow_promotion_codes: allowPromotionCodes, | |
| custom_fields: customFields, | |
| }; | |
| const session = await stripe.checkout.sessions.create(payload); | |
| if (!session.url) { | |
| throw new InternalServerError({ | |
| message: "Could not create Stripe checkout session.", | |
| }); | |
| } | |
| return session.url; | |
| }; |
🤖 Prompt for AI Agents
In src/api/functions/stripe.ts around lines 133 to 159, add a runtime guard that
validates customerId before constructing the Stripe payload and calling
stripe.checkout.sessions.create; if customerId is missing or empty, immediately
throw a descriptive error (e.g., BadRequest or a clear InternalServerError)
indicating that customerId is required so the code does not send a malformed
request to Stripe.
| const lock = createLock({ | ||
| adapter: new IoredisAdapter(redisClient), | ||
| key: `userSync:${netId}`, | ||
| retryAttempts: 5, | ||
| retryDelay: 300, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Set an explicit lock TTL and reduce work under the lock.
Add a clear duration to avoid silent expiry during network calls. Also check for lock loss both before and after external calls.
const lock = createLock({
adapter: new IoredisAdapter(redisClient),
key: `userSync:${netId}`,
+ duration: 15000, // 15s; tune based on p95 of the section below
retryAttempts: 5,
retryDelay: 300,
});And add a second abort check just before the final Dynamo write (see diff further below).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const lock = createLock({ | |
| adapter: new IoredisAdapter(redisClient), | |
| key: `userSync:${netId}`, | |
| retryAttempts: 5, | |
| retryDelay: 300, | |
| }); | |
| const lock = createLock({ | |
| adapter: new IoredisAdapter(redisClient), | |
| key: `userSync:${netId}`, | |
| duration: 15000, // 15s; tune based on p95 of the section below | |
| retryAttempts: 5, | |
| retryDelay: 300, | |
| }); |
🤖 Prompt for AI Agents
In src/api/functions/sync.ts around lines 33 to 38, the created lock is missing
an explicit TTL and too much work runs while the lock is held; set a clear TTL
on the lock (e.g. ttl in ms) so it cannot silently expire during network calls,
minimize the work done inside the locked section by moving long external/network
calls out of the critical region, and add checks for lock ownership both
immediately after the external calls return and again just before the final
Dynamo write (abort if the lock was lost) so we never proceed with the final
write when we no longer hold the lock.
| if (!stripeCustomerId) { | ||
| if (signal.aborted) { | ||
| throw new InternalServerError({ | ||
| message: | ||
| "Checked on lock before creating Stripe customer, we've lost the lock!", | ||
| }); | ||
| } | ||
| const newStripeCustomerId = await createStripeCustomer({ | ||
| email: userId, | ||
| name: `${firstName} ${lastName}`, | ||
| stripeApiKey, | ||
| }); | ||
| logger.info(`Created new Stripe customer for ${userId}.`); | ||
| const newInfo = await dynamoClient.send( |
There was a problem hiding this comment.
Prevent duplicate Stripe customers: use an idempotency key when creating the customer.
If the lock expires mid‑flow, concurrent runs can each create a Stripe customer. Protect the Stripe call with a deterministic idempotency key tied to the user.
- const newStripeCustomerId = await createStripeCustomer({
- email: userId,
- name: `${firstName} ${lastName}`,
- stripeApiKey,
- });
+ const idempotencyKey = `acm-create-stripe-customer:${userId}`;
+ const newStripeCustomerId = await createStripeCustomer({
+ email: userId,
+ name: `${firstName} ${lastName}`,
+ stripeApiKey,
+ metadata: { userId },
+ idempotencyKey,
+ });Companion changes are proposed in src/api/functions/stripe.ts to accept and pass the idempotency key.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/api/functions/sync.ts around lines 70 to 83, the Stripe customer creation
is vulnerable to duplicates if the lock expires; generate a deterministic
idempotency key (e.g., HMAC/SHA256 of the user id or user id + tenant) and pass
it into createStripeCustomer as an extra argument, then forward that key to
stripe.ts so the Stripe API request includes it in the Idempotency-Key header;
keep the existing lock/abort check and use the deterministic key for all
concurrent attempts for the same user to prevent duplicate customers.
| const newInfo = await dynamoClient.send( | ||
| new UpdateItemCommand({ | ||
| TableName: genericConfig.UserInfoTable, | ||
| Key: { | ||
| id: { S: userId }, | ||
| }, | ||
| UpdateExpression: "SET #stripeCustomerId = :stripeCustomerId", | ||
| ExpressionAttributeNames: { | ||
| "#stripeCustomerId": "stripeCustomerId", | ||
| }, | ||
| ExpressionAttributeValues: { | ||
| ":stripeCustomerId": { S: newStripeCustomerId }, | ||
| }, | ||
| ReturnValues: "ALL_NEW", | ||
| }), | ||
| ); | ||
| return newInfo && newInfo.Attributes |
There was a problem hiding this comment.
Make the stripeCustomerId write conditional; re‑read on conflict.
Ensure only the first writer sets the field; others should not clobber and should read the winning value.
- const newInfo = await dynamoClient.send(
- new UpdateItemCommand({
+ if (signal.aborted) {
+ throw new InternalServerError({ message: "Lost lock before persisting Stripe customer ID." });
+ }
+ try {
+ const newInfo = await dynamoClient.send(
+ new UpdateItemCommand({
TableName: genericConfig.UserInfoTable,
Key: {
id: { S: userId },
},
UpdateExpression: "SET #stripeCustomerId = :stripeCustomerId",
ExpressionAttributeNames: {
"#stripeCustomerId": "stripeCustomerId",
},
ExpressionAttributeValues: {
":stripeCustomerId": { S: newStripeCustomerId },
},
+ ConditionExpression: "attribute_not_exists(#stripeCustomerId)",
ReturnValues: "ALL_NEW",
- }),
- );
- return newInfo && newInfo.Attributes
- ? unmarshall(newInfo.Attributes)
- : updateResult && updateResult.Attributes
- ? unmarshall(updateResult.Attributes)
- : undefined;
+ }),
+ );
+ return newInfo && newInfo.Attributes
+ ? unmarshall(newInfo.Attributes)
+ : updateResult && updateResult.Attributes
+ ? unmarshall(updateResult.Attributes)
+ : undefined;
+ } catch (err: any) {
+ // Another writer won the race; fetch the latest record.
+ const latest = await dynamoClient.send(
+ new UpdateItemCommand({
+ TableName: genericConfig.UserInfoTable,
+ Key: { id: { S: userId } },
+ // no-op update to get ALL_NEW without needing an extra import
+ UpdateExpression: "SET #updatedAt = :updatedAt",
+ ExpressionAttributeNames: { "#updatedAt": "updatedAt" },
+ ExpressionAttributeValues: { ":updatedAt": { S: new Date().toISOString() } },
+ ReturnValues: "ALL_NEW",
+ }),
+ );
+ return latest && latest.Attributes ? unmarshall(latest.Attributes) : undefined;
+ }Note: If preferred, switch to GetItem for a pure read.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/api/functions/sync.ts around lines 83 to 99, the UpdateItem
unconditionally overwrites stripeCustomerId; change it to a conditional write
that only sets stripeCustomerId when it does not yet exist (e.g. use
ConditionExpression like "attribute_not_exists(#stripeCustomerId)" or check for
NULL) so only the first writer succeeds; on ConditionalCheckFailedException
re-read the item (GetItem) and return the existing stripeCustomerId/Attributes
instead of clobbering them; ensure errors are handled and the successful path
returns the updated Attributes.
| redisClient: fastify.redisClient, | ||
| stripeApiKey: fastify.secretConfig.stripe_secret_key, | ||
| logger: request.log, | ||
| }); |
There was a problem hiding this comment.
Validate Stripe secret before calling downstream.
Fail fast with a clear error if stripe_secret_key is unset.
await syncFullProfile({
+ ...(fastify.secretConfig.stripe_secret_key
+ ? {}
+ : (() => {
+ request.log.error("Missing Stripe secret key.");
+ throw new InternalServerError({ message: "Stripe is not configured." });
+ })()),
uinHash,
firstName: givenName,
lastName: surname,
netId,
dynamoClient: fastify.dynamoClient,
redisClient: fastify.redisClient,
stripeApiKey: fastify.secretConfig.stripe_secret_key,
logger: request.log,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| redisClient: fastify.redisClient, | |
| stripeApiKey: fastify.secretConfig.stripe_secret_key, | |
| logger: request.log, | |
| }); | |
| await syncFullProfile({ | |
| ...(fastify.secretConfig.stripe_secret_key | |
| ? {} | |
| : (() => { | |
| request.log.error("Missing Stripe secret key."); | |
| throw new InternalServerError({ message: "Stripe is not configured." }); | |
| })()), | |
| uinHash, | |
| firstName: givenName, | |
| lastName: surname, | |
| netId, | |
| dynamoClient: fastify.dynamoClient, | |
| redisClient: fastify.redisClient, | |
| stripeApiKey: fastify.secretConfig.stripe_secret_key, | |
| logger: request.log, | |
| }); |
🤖 Prompt for AI Agents
In src/api/routes/syncIdentity.ts around lines 112 to 115, the code passes
fastify.secretConfig.stripe_secret_key into downstream code without validating
it; add an early check for the presence (non-empty) of stripe_secret_key before
calling the downstream constructor, and if it is missing log an error and fail
fast by returning a clear error response (e.g., throw
fastify.httpErrors.internal('Stripe secret key not configured') or
reply.code(500).send(...)) so the route never proceeds with an undefined/empty
key.
| const userData = await savePromise; | ||
| if (!userData) { | ||
| request.log.error( | ||
| "Was expecting to get a user data save, but we didn't!", | ||
| ); | ||
| throw new InternalServerError({}); | ||
| } |
There was a problem hiding this comment.
Also ensure stripeCustomerId exists before checkout.
If the customer create/persist failed, fail fast.
const userData = await savePromise;
if (!userData) {
request.log.error(
"Was expecting to get a user data save, but we didn't!",
);
throw new InternalServerError({});
}
+ if (!userData.stripeCustomerId) {
+ request.log.error("Missing stripeCustomerId on user record.");
+ throw new InternalServerError({ message: "Unable to initialize Stripe customer." });
+ }🤖 Prompt for AI Agents
In src/api/routes/v2/membership.ts around lines 145 to 151, after awaiting
savePromise you must verify that the persisted userData contains a
stripeCustomerId and fail fast if it does not; update the block so that if
userData is falsy or userData.stripeCustomerId is missing you log a descriptive
error (include user id/email/context from the request), and throw an
InternalServerError (or appropriate HTTP error) to stop checkout flow instead of
proceeding.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Improvements