TASK #00000 : Update discount value issue fix#703
TASK #00000 : Update discount value issue fix#703mahajanmahesh935 wants to merge 2 commits intotekdi:aspire-leadersfrom
Conversation
WalkthroughReused inactive coupon records instead of rejecting duplicates, added Stripe immutability checks and promotion-code handling, reworked Stripe sync to handle resource conflicts and promotion-code lifecycle, added webhook event parsing for Changes
Sequence DiagramsequenceDiagram
actor User
participant Service as Coupon Service
participant DB as Database
participant Stripe as Stripe API
User->>Service: createCoupon(dto)
Service->>DB: Query existing coupon by code
alt Coupon exists & inactive
Service->>DB: Update coupon fields
Service->>Service: syncCouponToStripe()
Service->>Stripe: Create Stripe coupon (desiredCouponId)
alt Stripe returns resource_already_exists
Service->>Stripe: Retrieve existing Stripe coupon
Service->>Service: stripeCouponMatchesEntity()
alt Immutable fields match
Service->>Stripe: Create promotion code
Service->>DB: Save stripePromoCodeId
else Immutable fields differ
Service->>Stripe: Get existing promotion code (if any)
Service->>Stripe: Deactivate conflicting promotion code
Service->>Stripe: Create new Stripe coupon
Service->>Stripe: Create promotion code
Service->>DB: Save new stripePromoCodeId
end
else Stripe coupon creation succeeds
Service->>Stripe: Create promotion code
Service->>DB: Save stripePromoCodeId
end
else Coupon exists & active
Service->>User: BadRequestException
else Coupon doesn't exist
Service->>DB: Create new coupon
Service->>Service: syncCouponToStripe()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/payments/services/coupon.service.ts`:
- Around line 56-68: Reusing the inactive coupon row overwrites terms but keeps
coupon.id, currentRedemptions and existing CouponRedemption rows, which allows a
reactivated coupon to appear already consumed; when updating an inactive
existing (the variable existing in this block) you must reset usage state or
clear past redemptions: either create a fresh coupon row instead of reusing the
same id, or if you must reuse the row, set existing.currentRedemptions = 0 and
remove or mark associated CouponRedemption records as stale/archived so
validateCoupon() checks reflect the new terms; update related logic that
references coupon.id and CouponRedemption to ensure per-user counts and total
redemptions start from zero on reactivation.
- Around line 217-237: When desiredCouponId exists but its immutable fields
differ, avoid always creating a new Stripe coupon; first try to derive/reuse the
active coupon referenced by coupon.stripePromoCodeId (retrieve the promotion
code via this.stripe.promotionCodes.retrieve or retrieve the promo code's coupon
and confirm it matches paramsForNewCoupon) and persist that Stripe coupon ID to
the DB (e.g., coupon.stripeCouponId) before deactivating or creating
replacements. Update the flow in the branch that uses
this.stripe.coupons.retrieve(desiredCouponId) /
this.stripe.promotionCodes.update(...) so it: 1) if mismatch, attempt to fetch
the coupon linked to coupon.stripePromoCodeId and use it when it matches, 2)
only deactivate the promo and create paramsForNewCoupon when no active matching
coupon can be found, and 3) persist the chosen active Stripe coupon ID back to
the entity so subsequent updateCoupon() runs reuse it instead of cycling
replacements; apply the same change to the similar block later in the file.
- Around line 69-86: When reactivating a coupon the old stripePromoCodeId can
remain on the returned object if Stripe sync fails; ensure the coupon's
stripePromoCodeId is cleared before saving so we never return a stale promo id.
In the reactivation flow in coupon.service.ts (references: stripePromoCodeId,
couponRepository.save, syncCouponToStripe, savedCoupon) set
existing.stripePromoCodeId = null when dto.stripePromoCodeId is undefined (or
explicitly override savedCoupon.stripePromoCodeId = null before returning) and
persist that state so a failed sync won't leave a previous Stripe promo id on
the coupon.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0b8f3290-5e7c-466f-9c61-d32efc88493c
📒 Files selected for processing (1)
src/payments/services/coupon.service.ts
| // Same code is reserved by an inactive row (unique constraint). Reuse the row: | ||
| // stable couponId for reporting/redemptions, new terms from this payload. | ||
| existing.contextType = dto.contextType; | ||
| existing.contextId = dto.contextId; | ||
| existing.discountType = dto.discountType; | ||
| existing.discountValue = dto.discountValue; | ||
| existing.currency = dto.currency || 'USD'; | ||
| existing.countryId = dto.countryId ?? null; | ||
| existing.isActive = dto.isActive ?? true; | ||
| existing.validFrom = dto.validFrom ? new Date(dto.validFrom) : null; | ||
| existing.validTill = dto.validTill ? new Date(dto.validTill) : null; | ||
| existing.maxRedemptions = dto.maxRedemptions ?? null; | ||
| existing.maxRedemptionsPerUser = dto.maxRedemptionsPerUser ?? null; |
There was a problem hiding this comment.
Reusing this row also reuses the old redemption history.
This branch rewrites the coupon definition but keeps the same coupon.id, currentRedemptions, and existing CouponRedemption rows. validateCoupon() later checks both total and per-user usage by that ID, so a reactivated coupon can come back already partially or fully exhausted.
🧰 Tools
🪛 ESLint
[error] 62-62: Replace 'USD' with "USD"
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/payments/services/coupon.service.ts` around lines 56 - 68, Reusing the
inactive coupon row overwrites terms but keeps coupon.id, currentRedemptions and
existing CouponRedemption rows, which allows a reactivated coupon to appear
already consumed; when updating an inactive existing (the variable existing in
this block) you must reset usage state or clear past redemptions: either create
a fresh coupon row instead of reusing the same id, or if you must reuse the row,
set existing.currentRedemptions = 0 and remove or mark associated
CouponRedemption records as stale/archived so validateCoupon() checks reflect
the new terms; update related logic that references coupon.id and
CouponRedemption to ensure per-user counts and total redemptions start from zero
on reactivation.
| if (dto.stripePromoCodeId !== undefined) { | ||
| existing.stripePromoCodeId = dto.stripePromoCodeId ?? null; | ||
| } | ||
|
|
||
| const savedCoupon = await this.couponRepository.save(existing); | ||
| if (this.stripe && !dto.stripePromoCodeId) { | ||
| try { | ||
| await this.syncCouponToStripe(savedCoupon); | ||
| } catch (error) { | ||
| this.logger.warn( | ||
| `Failed to sync reactivated coupon ${savedCoupon.id} to Stripe: ${error.message}`, | ||
| ); | ||
| } | ||
| } | ||
| this.logger.log( | ||
| `Reactivated inactive coupon: ${savedCoupon.couponCode} (${savedCoupon.id})`, | ||
| ); | ||
| return savedCoupon; |
There was a problem hiding this comment.
Don't return a reactivated coupon with the previous Stripe promo ID still attached.
Line 73 saves the new terms before Stripe sync, and the catch block only logs. If this inactive row already had stripePromoCodeId, the method returns that old value unchanged when sync fails. In src/payments/services/payment.service.ts:48-147, checkout only re-syncs when the field is missing, so the previous Stripe promotion code can be reused for the new coupon terms.
💡 Safer fallback
+ const previousStripePromoCodeId = existing.stripePromoCodeId;
+ if (dto.stripePromoCodeId === undefined) {
+ existing.stripePromoCodeId = null;
+ }
const savedCoupon = await this.couponRepository.save(existing);
if (this.stripe && !dto.stripePromoCodeId) {
try {
+ savedCoupon.stripePromoCodeId = previousStripePromoCodeId;
await this.syncCouponToStripe(savedCoupon);
} catch (error) {
+ await this.couponRepository.update(savedCoupon.id, {
+ stripePromoCodeId: null,
+ });
this.logger.warn(
`Failed to sync reactivated coupon ${savedCoupon.id} to Stripe: ${error.message}`,
);
}
}🧰 Tools
🪛 ESLint
[error] 79-79: Delete ,
(prettier/prettier)
[error] 84-84: Delete ,
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/payments/services/coupon.service.ts` around lines 69 - 86, When
reactivating a coupon the old stripePromoCodeId can remain on the returned
object if Stripe sync fails; ensure the coupon's stripePromoCodeId is cleared
before saving so we never return a stale promo id. In the reactivation flow in
coupon.service.ts (references: stripePromoCodeId, couponRepository.save,
syncCouponToStripe, savedCoupon) set existing.stripePromoCodeId = null when
dto.stripePromoCodeId is undefined (or explicitly override
savedCoupon.stripePromoCodeId = null before returning) and persist that state so
a failed sync won't leave a previous Stripe promo id on the coupon.
| if (error.code === 'resource_already_exists') { | ||
| stripeCoupon = await this.stripe.coupons.retrieve( | ||
| stripeCouponParams.id as string, | ||
| ); | ||
| const existing = await this.stripe.coupons.retrieve(desiredCouponId); | ||
| if (this.stripeCouponMatchesEntity(existing, coupon)) { | ||
| stripeCoupon = existing; | ||
| } else { | ||
| this.logger.log( | ||
| `Stripe coupon ${desiredCouponId} exists but does not match DB; creating new Stripe coupon (immutable fields changed)`, | ||
| ); | ||
| if (coupon.stripePromoCodeId) { | ||
| try { | ||
| await this.stripe.promotionCodes.update(coupon.stripePromoCodeId, { | ||
| active: false, | ||
| }); | ||
| } catch (deactErr) { | ||
| if (deactErr.code !== 'resource_missing') { | ||
| throw deactErr; | ||
| } | ||
| } | ||
| } | ||
| stripeCoupon = await this.stripe.coupons.create(paramsForNewCoupon); | ||
| } |
There was a problem hiding this comment.
This replacement flow will keep rotating Stripe coupons after the first immutable change.
When desiredCouponId is already taken, Line 236 creates a replacement coupon with a new Stripe-generated ID, but the next sync starts from Line 218 again instead of reusing the coupon behind the stored stripePromoCodeId. That means every later sync can rediscover the old immutable coupon, deactivate the current promo, and create yet another replacement even if the current promo already points to a matching coupon. Since updateCoupon() now syncs on every save, even a later context/country change can churn promo IDs indefinitely; any failure after deactivation also leaves the database pointing at an inactive promo. Persist the active Stripe coupon ID, or derive/reuse it from coupon.stripePromoCodeId before falling back to desiredCouponId.
Also applies to: 243-287
🧰 Tools
🪛 ESLint
[error] 217-217: Replace 'resource_already_exists' with "resource_already_exists"
(prettier/prettier)
[error] 223-223: Delete ,
(prettier/prettier)
[error] 227-227: Replace coupon.stripePromoCodeId, with ⏎··················coupon.stripePromoCodeId,⏎·················
(prettier/prettier)
[error] 228-228: Insert ··
(prettier/prettier)
[error] 229-229: Replace } with ··}⏎················
(prettier/prettier)
[error] 231-231: Replace 'resource_missing' with "resource_missing"
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/payments/services/coupon.service.ts` around lines 217 - 237, When
desiredCouponId exists but its immutable fields differ, avoid always creating a
new Stripe coupon; first try to derive/reuse the active coupon referenced by
coupon.stripePromoCodeId (retrieve the promotion code via
this.stripe.promotionCodes.retrieve or retrieve the promo code's coupon and
confirm it matches paramsForNewCoupon) and persist that Stripe coupon ID to the
DB (e.g., coupon.stripeCouponId) before deactivating or creating replacements.
Update the flow in the branch that uses
this.stripe.coupons.retrieve(desiredCouponId) /
this.stripe.promotionCodes.update(...) so it: 1) if mismatch, attempt to fetch
the coupon linked to coupon.stripePromoCodeId and use it when it matches, 2)
only deactivate the promo and create paramsForNewCoupon when no active matching
coupon can be found, and 3) persist the chosen active Stripe coupon ID back to
the entity so subsequent updateCoupon() runs reuse it instead of cycling
replacements; apply the same change to the similar block later in the file.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/payments/services/payment.service.ts (1)
316-318: Consider clearingfailureReasonwhen status transitions to success.The current logic only sets
failureReasonwhen the webhook status isfailed. If a transaction was previously marked as failed (with a reason) and a subsequent webhook event changes it tosuccess, the oldfailureReasonwould remain. This could be misleading in reports.🛠️ Optional: Clear failure reason on non-failed status updates
// Update existing transaction transactionToUpdate.status = transactionStatus; if (failedWebhookReason) { transactionToUpdate.failureReason = failedWebhookReason; + } else { + transactionToUpdate.failureReason = null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/payments/services/payment.service.ts` around lines 316 - 318, When updating transaction status in the webhook handler, ensure you clear any stale failure reason: instead of only setting transactionToUpdate.failureReason = failedWebhookReason when failedWebhookReason exists, explicitly set transactionToUpdate.failureReason = null (or undefined) when the incoming status is not a failure so previous failure text is removed; adjust the logic around failedWebhookReason assignment in the webhook processing function (the code that currently uses failedWebhookReason and transactionToUpdate.failureReason) to set the field on failure and clear it on non-failure transitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/payments/services/payment.service.ts`:
- Around line 316-318: When updating transaction status in the webhook handler,
ensure you clear any stale failure reason: instead of only setting
transactionToUpdate.failureReason = failedWebhookReason when failedWebhookReason
exists, explicitly set transactionToUpdate.failureReason = null (or undefined)
when the incoming status is not a failure so previous failure text is removed;
adjust the logic around failedWebhookReason assignment in the webhook processing
function (the code that currently uses failedWebhookReason and
transactionToUpdate.failureReason) to set the field on failure and clear it on
non-failure transitions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4e8e8dc9-906e-4ca9-8d1e-4128e9ae9b30
📒 Files selected for processing (2)
src/payments/providers/stripe/stripe.provider.tssrc/payments/services/payment.service.ts
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements