Stripe webhook updates for ACH transfer #357
Conversation
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
WalkthroughAdds handling for Stripe Changes
Sequence Diagram(s)sequenceDiagram
participant Stripe as Stripe Service
participant Handler as Webhook Handler
participant DDB as DynamoDB
participant Client as HTTP Client
Stripe->>Handler: POST webhook (payment_intent.succeeded + signature)
activate Handler
Handler->>Handler: Verify signature & parse event
alt event == payment_intent.succeeded
Handler->>Handler: Extract amount, currency, billingEmail, domain, eventId
Handler->>DDB: PutItemCommand -> StripePaymentsDynamoTableName\n(pk: "<acm_org>#<domain>", sk: event.id)
activate DDB
DDB-->>Handler: PutItem result
deactivate DDB
Handler->>Client: 200 { handled: true, requestId }
else
Handler->>Client: 200 / ignored
end
deactivate Handler
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
📜 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 (2)
src/api/routes/stripe.ts(3 hunks)src/common/config.ts(2 hunks)
⏰ 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 (3)
src/common/config.ts (1)
43-43: LGTM! Configuration addition is consistent.The new
StripePaymentsDynamoTableNamefield follows existing naming conventions and patterns in the codebase.Also applies to: 87-87
src/api/routes/stripe.ts (2)
6-6: LGTM! Import correctly added.The
PutItemCommandimport is properly placed with other DynamoDB commands and is necessary for the new webhook handler.
767-770: LGTM! Return handling is correct.The case properly returns a success response and is structured consistently with other webhook handlers in this file.
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/api/routes/stripe.ts (2)
734-748: Add error handling around DynamoDB operation.This issue was previously flagged: The
PutItemCommandlacks error handling. If the database write fails, the webhook will throw an unhandled error, and Stripe will retry the webhook repeatedly.Wrap the DynamoDB operation in a try-catch block consistent with other DynamoDB operations in this file (lines 85-94, 171-186):
+ try { await fastify.dynamoClient.send( new PutItemCommand({ TableName: genericConfig.StripePaymentsDynamoTableName, Item: marshall({ primaryKey: `${acmOrg}#${domain}`, sortKey: `customer`, amount, currency, status: "succeeded", billingEmail: email, createdAt: Date.now(), eventId: event.id, }), }), ); + } catch (e) { + if (e instanceof BaseError) { + throw e; + } + request.log.error(e); + throw new DatabaseInsertError({ + message: "Could not write payment transaction to database.", + }); + }
744-744: Use ISO string format forcreatedAtto maintain consistency.This issue was previously flagged: Line 744 uses
Date.now()which returns a numeric timestamp, but elsewhere in this file (e.g., line 162),createdAtis stored as an ISO string vianew Date().toISOString(). This inconsistency could cause issues when querying or displaying dates.Apply this diff:
status: "succeeded", billingEmail: email, - createdAt: Date.now(), + createdAt: new Date().toISOString(), eventId: event.id, }),
🧹 Nitpick comments (1)
src/api/routes/stripe.ts (1)
732-732: Add validation for email domain extraction.The domain extraction using
email.split("@")[1]could fail if the email format is unexpected (e.g., missing '@' symbol, multiple '@' symbols). While the email has fallbacks, it's safer to validate the extraction.Consider adding validation:
const email = intent.receipt_email ?? intent.metadata?.billing_email ?? "unknown@example.com"; const acmOrg = intent.metadata?.acm_org ?? "ACM@UIUC"; - const domain = email.split("@")[1] ?? "unknown.com"; + const emailParts = email.split("@"); + const domain = emailParts.length === 2 ? emailParts[1] : "unknown.com";
📜 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 (1)
src/api/routes/stripe.ts(3 hunks)
⏰ 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
AdityaK2905
force-pushed
the
stripeWebhookChanges
branch
from
24767dd to
babe292
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/api/routes/stripe.ts (3)
732-746: Add error handling around DynamoDB operation.The
PutItemCommandlacks error handling, unlike similar DynamoDB operations elsewhere in this file (lines 85-94, 171-186). If the database write fails, the webhook will throw an unhandled error.Apply this diff to add error handling consistent with other DynamoDB operations:
+ try { await fastify.dynamoClient.send( new PutItemCommand({ TableName: genericConfig.StripePaymentsDynamoTableName, Item: marshall({ primaryKey: `${acmOrg}#${domain}`, sortKey: `customer`, amount, currency, status: "succeeded", billingEmail: email, createdAt: Date.now(), eventId: event.id, }), }), ); + } catch (e) { + if (e instanceof BaseError) { + throw e; + } + request.log.error(e); + throw new DatabaseInsertError({ + message: "Could not write payment transaction to database.", + }); + }
736-737: Data model will overwrite previous payments.The current DynamoDB key structure (
primaryKey: "${acmOrg}#${domain}"andsortKey: "customer") means only one payment record can exist per organization-domain combination. Each subsequent payment from the same domain will overwrite the previous record, causing data loss.Consider using the payment intent ID as the sort key to ensure uniqueness:
Item: marshall({ primaryKey: `${acmOrg}#${domain}`, - sortKey: `customer`, + sortKey: `PAYMENT#${intent.id}`, amount, currency, status: "succeeded", billingEmail: email, + customerId, createdAt: Date.now(), eventId: event.id, }),
742-742: Use ISO string format forcreatedAtto maintain consistency.Line 742 uses
Date.now()which returns a numeric timestamp, but elsewhere in this file (line 162),createdAtis stored as an ISO string vianew Date().toISOString(). This inconsistency could cause issues when querying or displaying dates.Apply this diff:
status: "succeeded", billingEmail: email, - createdAt: Date.now(), + createdAt: new Date().toISOString(), eventId: event.id, }),
📜 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 (1)
src/api/routes/stripe.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Required Reviews
src/api/routes/stripe.ts
[error] Requirement "Base Requirement" is not satisfied. No reviews from required reviewers (Members of officers, infra-chairs, or infra-leads).
⏰ 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 (1)
src/api/routes/stripe.ts (1)
748-754: LGTM: Logging and response handling.The logging statement provides useful information for tracking payments, and the response format is consistent with other webhook handlers in this file.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
src/api/routes/stripe.ts(2 hunks)
⏰ 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
AdityaK2905
changed the title
Summary by CodeRabbit