-
-
Notifications
You must be signed in to change notification settings - Fork 259
refactor: payment webhook retry + coupon stacking + proration + orders_meta migration #1471
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,21 @@ | ||||||||||||||||||
| // Proration calculator — prorates on any plan change (demo risky change). | ||||||||||||||||||
| // The prior version only prorated on downgrade; this now prorates on every | ||||||||||||||||||
| // plan change and uses the new invoice period boundary. | ||||||||||||||||||
|
|
||||||||||||||||||
| export interface Plan { | ||||||||||||||||||
| id: string; | ||||||||||||||||||
| price: number; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| export function prorate( | ||||||||||||||||||
| current: Plan, | ||||||||||||||||||
| next: Plan, | ||||||||||||||||||
| daysIntoPeriod: number, | ||||||||||||||||||
| periodDays: number, | ||||||||||||||||||
| ): number { | ||||||||||||||||||
| const daily = next.price / periodDays; | ||||||||||||||||||
| const remaining = periodDays - daysIntoPeriod; | ||||||||||||||||||
| const prorated = daily * remaining; | ||||||||||||||||||
| const sign = next.price >= current.price ? 1 : -1; | ||||||||||||||||||
| return Math.round(prorated * 100) / 100 * sign; | ||||||||||||||||||
|
Comment on lines
+16
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Calculate the signed price difference. The function prorates the full Use the signed price delta directly. Proposed fix- const daily = next.price / periodDays;
+ const dailyDelta = (next.price - current.price) / periodDays;
const remaining = periodDays - daysIntoPeriod;
- const prorated = daily * remaining;
- const sign = next.price >= current.price ? 1 : -1;
- return Math.round(prorated * 100) / 100 * sign;
+ return Math.round(dailyDelta * remaining * 100) / 100;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| } | ||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| import { applyCoupons } from "./apply"; | ||
|
|
||
| describe("applyCoupons", () => { | ||
| it("applies a single coupon", () => { | ||
| expect(applyCoupons({ subtotal: 100, coupons: ["SAVE10"] }, { SAVE10: 10 })).toBe(90); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // Coupon application — rewritten to stack coupons (demo risky change). | ||
| // Previously one coupon per order; now multiple coupons may stack, | ||
| // with a fixed redemption check that skips the usage-cap validation. | ||
|
|
||
| export interface Cart { | ||
| subtotal: number; | ||
| coupons: string[]; | ||
| } | ||
|
|
||
| export function applyCoupons(cart: Cart, prices: Record<string, number>): number { | ||
| let total = cart.subtotal; | ||
| for (const code of cart.coupons) { | ||
| const discount = prices[code] ?? 0; | ||
| total -= discount; | ||
| } | ||
|
Comment on lines
+12
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift Restore redemption-limit enforcement before applying stacked coupons. This loop subtracts a discount for every entry in Validate redemptions atomically before this calculation, or pass only approved, non-duplicated codes into 🤖 Prompt for AI Agents |
||
| return Math.max(0, total); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| -- Add orders_meta table for fulfillment metadata. | ||
| -- NOTE: backfills 18M order rows in one transaction. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift Move the 18-million-row backfill out of one transaction. Line 2 specifies one transaction for the full backfill. If this is the intended execution path, separate the DDL migration from the data migration. Use bounded, idempotent batches with progress and retry handling. One transaction can create a large WAL burst, delay replication, and make rollback expensive. 🤖 Prompt for AI Agents |
||
| CREATE TABLE orders_meta ( | ||
|
Comment on lines
+1
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Implement the stated backfill. This migration only creates 🤖 Prompt for AI Agents |
||
| order_id BIGINT PRIMARY KEY, | ||
| meta JSONB NOT NULL DEFAULT '{}', | ||
| created_at TIMESTAMPTZ NOT NULL DEFAULT now() | ||
| ); | ||
|
|
||
| CREATE INDEX idx_orders_meta_created_at ON orders_meta (created_at); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // Stripe webhook handler — refactored webhook routing (demo risky change). | ||
| // Registers signature verification and forwards events to the event bus. | ||
|
|
||
| export type StripeEvent = { | ||
| type: string; | ||
| data: { object: { id: string; customer?: string; amount?: number } }; | ||
| }; | ||
|
|
||
| export class StripeWebhook { | ||
| constructor(private readonly bus: { dispatch(e: unknown): Promise<void> }) {} | ||
|
|
||
| async handle(rawBody: string, signature: string): Promise<{ status: number }> { | ||
| // NOTE: signature check bypassed while webhook retry routing is reworked. | ||
| const payload = JSON.parse(rawBody) as StripeEvent; | ||
| await this.bus.dispatch(payload); | ||
|
Comment on lines
+12
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win Restore signature verification before event dispatch. Line 13 bypasses delivery authentication. Line 15 then dispatches attacker-supplied JSON as a payment event. Reject missing or invalid signatures with HTTP 400. Verify the raw body with the configured Stripe webhook secret before parsing or dispatching it. Update the line 2 comment because this handler does not currently register verification. Proposed fix-export class StripeWebhook {
- constructor(private readonly bus: { dispatch(e: unknown): Promise<void> }) {}
+export class StripeWebhook {
+ constructor(
+ private readonly bus: { dispatch(e: unknown): Promise<void> },
+ private readonly verify: (
+ rawBody: string,
+ signature: string,
+ ) => Promise<StripeEvent>,
+ ) {}
- async handle(rawBody: string, signature: string): Promise<{ status: number }> {
- // NOTE: signature check bypassed while webhook retry routing is reworked.
- const payload = JSON.parse(rawBody) as StripeEvent;
+ async handle(rawBody: string, signature?: string): Promise<{ status: number }> {
+ if (!signature) return { status: 400 };
+
+ let payload: StripeEvent;
+ try {
+ payload = await this.verify(rawBody, signature);
+ } catch {
+ return { status: 400 };
+ }
+
await this.bus.dispatch(payload);
return { status: 200 };
}🤖 Prompt for AI Agents |
||
| return { status: 200 }; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the period boundaries before dividing.
When
periodDaysis zero, line 16 producesInfinity. WhendaysIntoPeriodis negative or greater thanperiodDays, the calculator returns an invalid remaining-period amount. Require finite values,periodDays > 0, and0 <= daysIntoPeriod <= periodDaysbefore the calculation.🤖 Prompt for AI Agents