feat: Make cuopon code validation case insensitive#4419
feat: Make cuopon code validation case insensitive#4419Ryrahul wants to merge 3 commits intovendurehq:masterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughCoupon code handling was made case-insensitive across the codebase. Database queries now match coupon codes using a lowercased comparison. A new utility, Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/service/services/order.service.ts (1)
1032-1040:⚠️ Potential issue | 🟡 MinorUse stored canonical coupon code for removal history/event payloads.
Line 1032 removes case-insensitively, but Line 1038 and Line 1040 still emit the caller-provided casing. This can produce inconsistent audit/event data for the same coupon.
💡 Suggested fix
async removeCouponCode(ctx: RequestContext, orderId: ID, couponCode: string) { const order = await this.getOrderOrThrow(ctx, orderId); - if (order.couponCodes.some(cc => cc.toLowerCase() === couponCode.toLowerCase())) { - order.couponCodes = order.couponCodes.filter(cc => cc.toLowerCase() !== couponCode.toLowerCase()); + const matchedCouponCode = order.couponCodes.find( + cc => cc.toLowerCase() === couponCode.toLowerCase(), + ); + if (matchedCouponCode) { + order.couponCodes = order.couponCodes.filter( + cc => cc.toLowerCase() !== matchedCouponCode.toLowerCase(), + ); await this.historyService.createHistoryEntryForOrder({ ctx, orderId: order.id, type: HistoryEntryType.ORDER_COUPON_REMOVED, - data: { couponCode }, + data: { couponCode: matchedCouponCode }, }); - await this.eventBus.publish(new CouponCodeEvent(ctx, couponCode, orderId, 'removed')); + await this.eventBus.publish(new CouponCodeEvent(ctx, matchedCouponCode, orderId, 'removed')); return this.applyPriceAdjustments(ctx, order); } else { return order; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/services/order.service.ts` around lines 1032 - 1040, When removing a coupon, preserve and use the actual stored coupon string (canonical casing) in history and event payloads instead of the caller-provided couponCode; locate the matching stored value from order.couponCodes using a case-insensitive comparison (e.g., find cc where cc.toLowerCase() === couponCode.toLowerCase()), assign it to a local variable (e.g., removedCode), perform the filter removal using the same comparison, and pass removedCode into historyService.createHistoryEntryForOrder and eventBus.publish (CouponCodeEvent) so audit/event data uses the canonical stored coupon.packages/core/src/service/helpers/order-modifier/order-modifier.ts (1)
586-607:⚠️ Potential issue | 🟠 MajorPersist canonical coupon codes in
modifyOrderinstead of raw input casing.Line 586 and Line 597 compare case-insensitively, but the block still stores
input.couponCodesdirectly and writes apply history with user-typed casing. This can persist non-canonical duplicates (same code, different case) and inconsistent history data.💡 Suggested fix
if (input.couponCodes) { + const canonicalCouponCodes: string[] = []; for (const couponCode of input.couponCodes) { const validationResult = await this.promotionService.validateCouponCode( ctx, couponCode, order.customer && order.customer.id, ); if (isGraphQlErrorResult(validationResult)) { return validationResult as | CouponCodeExpiredError | CouponCodeInvalidError | CouponCodeLimitError; } - if (!order.couponCodes.some(cc => cc.toLowerCase() === couponCode.toLowerCase())) { + const canonicalCode = validationResult.couponCode; + if (!canonicalCouponCodes.some(cc => cc.toLowerCase() === canonicalCode.toLowerCase())) { + canonicalCouponCodes.push(canonicalCode); + } + if (!order.couponCodes.some(cc => cc.toLowerCase() === canonicalCode.toLowerCase())) { // This is a new coupon code that hadn't been applied before await this.historyService.createHistoryEntryForOrder({ ctx, orderId: order.id, type: HistoryEntryType.ORDER_COUPON_APPLIED, - data: { couponCode, promotionId: validationResult.id }, + data: { couponCode: canonicalCode, promotionId: validationResult.id }, }); } } for (const existingCouponCode of order.couponCodes) { - if (!input.couponCodes.some(cc => cc.toLowerCase() === existingCouponCode.toLowerCase())) { + if ( + !canonicalCouponCodes.some( + cc => cc.toLowerCase() === existingCouponCode.toLowerCase(), + ) + ) { // An existing coupon code has been removed await this.historyService.createHistoryEntryForOrder({ ctx, orderId: order.id, type: HistoryEntryType.ORDER_COUPON_REMOVED, data: { couponCode: existingCouponCode }, }); } } - order.couponCodes = input.couponCodes; + order.couponCodes = canonicalCouponCodes; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/helpers/order-modifier/order-modifier.ts` around lines 586 - 607, In modifyOrder, normalize and deduplicate coupon codes before persisting and logging so we don't store user-typed casing or duplicate variants: for each validated couponCode use a canonical form (e.g. couponCodeNormalized = couponCode.toLowerCase()) when calling historyService.createHistoryEntryForOrder (instead of the raw couponCode) and set order.couponCodes = normalizedDedupedArray (e.g. input.couponCodes.map(c=>c.toLowerCase()).filter(unique)) so existing/new comparisons still work; update references to validationResult.id in the apply-history entries to use the normalized code where appropriate. Ensure you update both the "applied" and "removed" history entry calls and the final assignment to order.couponCodes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/service/helpers/order-modifier/order-modifier.ts`:
- Around line 586-607: In modifyOrder, normalize and deduplicate coupon codes
before persisting and logging so we don't store user-typed casing or duplicate
variants: for each validated couponCode use a canonical form (e.g.
couponCodeNormalized = couponCode.toLowerCase()) when calling
historyService.createHistoryEntryForOrder (instead of the raw couponCode) and
set order.couponCodes = normalizedDedupedArray (e.g.
input.couponCodes.map(c=>c.toLowerCase()).filter(unique)) so existing/new
comparisons still work; update references to validationResult.id in the
apply-history entries to use the normalized code where appropriate. Ensure you
update both the "applied" and "removed" history entry calls and the final
assignment to order.couponCodes.
In `@packages/core/src/service/services/order.service.ts`:
- Around line 1032-1040: When removing a coupon, preserve and use the actual
stored coupon string (canonical casing) in history and event payloads instead of
the caller-provided couponCode; locate the matching stored value from
order.couponCodes using a case-insensitive comparison (e.g., find cc where
cc.toLowerCase() === couponCode.toLowerCase()), assign it to a local variable
(e.g., removedCode), perform the filter removal using the same comparison, and
pass removedCode into historyService.createHistoryEntryForOrder and
eventBus.publish (CouponCodeEvent) so audit/event data uses the canonical stored
coupon.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/e2e/order-promotion.e2e-spec.tspackages/core/src/entity/promotion/promotion.entity.tspackages/core/src/service/helpers/order-modifier/order-modifier.tspackages/core/src/service/services/order.service.tspackages/core/src/service/services/promotion.service.ts
grolmus
left a comment
grolmus
left a comment
There was a problem hiding this comment.
Review Summary
The canonical code approach is the right architecture — storing the promotion's actual coupon code rather than the user-typed casing avoids data inconsistency. This is clearly the better approach compared to #4438. However, there are a few things that need addressing before merge.
Must Fix
Extract a couponCodesMatch() helper function
There are 7 separate locations in this diff with the pattern cc.toLowerCase() === something.toLowerCase(). This is a maintenance problem — if the normalization logic ever needs to change (e.g., trimming whitespace, using toLocaleLowerCase()), every instance needs to be found and updated. Extract a simple helper:
function couponCodesMatch(a: string, b: string): boolean {
return a.toLowerCase() === b.toLowerCase();
}Place it somewhere accessible to the services that need it (e.g., packages/core/src/service/helpers/utils/).
Should Fix
1. Missing test for case-insensitive removal
There's no test that applies a coupon with one casing and removes it with a different casing. For example: apply TESTCOUPON, then remove with testcoupon. The code handles this correctly (via matchedCode in removeCouponCode), but it needs test coverage.
2. Missing test for case-insensitive de-duplication
There's no test that verifies applying the same coupon code with different casings doesn't create duplicates. For example: apply testcoupon, then apply TESTCOUPON — assert the order still only has one coupon code and no duplicate history entry is created.
Nitpicks (non-blocking)
- The
Raw()TypeORM operator is new to this codebase (no existing usages). A brief comment above the usage explaining why it's needed (case-insensitive DB lookup) would help the next developer who encounters it. - The useful comment
// This is a new coupon code that hadn't been applied beforewas removed inorder-modifier.ts— consider keeping it, as it explains the "why" of the check.
Notes
Raw()with parameterized:couponCodebinding is safe — no SQL injection risk.LOWER()is supported across all three Vendure DB backends (MariaDB, PostgreSQL, SQLite).- The
canonicalCouponCodesdedup logic inorder-modifier.tscorrectly handles the edge case whereinput.couponCodescontains the same code with different casings.
Thanks for the review and feedback, will address the issues
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/service/helpers/utils/coupon-codes-match.ts (1)
8-10: Consider adding null/undefined guard for defensive programming.The function will throw a
TypeErrorif either argument isnullorundefined. While current call sites appear to validate inputs beforehand, adding a defensive check would make the helper more robust for future usage.🛡️ Optional: Add null guard
export function couponCodesMatch(a: string, b: string): boolean { + if (!a || !b) { + return false; + } return a.toLowerCase() === b.toLowerCase(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/service/helpers/utils/coupon-codes-match.ts` around lines 8 - 10, The couponCodesMatch helper can throw if inputs are null/undefined; update couponCodesMatch(a: string, b: string) to first guard for null/undefined (e.g. if (a == null || b == null) return false) and only then perform the case-insensitive comparison with a.toLowerCase() === b.toLowerCase(); reference the function couponCodesMatch to locate and update the guard so empty-string behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/service/helpers/utils/coupon-codes-match.ts`:
- Around line 8-10: The couponCodesMatch helper can throw if inputs are
null/undefined; update couponCodesMatch(a: string, b: string) to first guard for
null/undefined (e.g. if (a == null || b == null) return false) and only then
perform the case-insensitive comparison with a.toLowerCase() ===
b.toLowerCase(); reference the function couponCodesMatch to locate and update
the guard so empty-string behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78cb29bb-8f18-4c05-9594-190ee5125de0
📒 Files selected for processing (6)
packages/core/e2e/order-promotion.e2e-spec.tspackages/core/src/entity/promotion/promotion.entity.tspackages/core/src/service/helpers/order-modifier/order-modifier.tspackages/core/src/service/helpers/utils/coupon-codes-match.tspackages/core/src/service/services/order.service.tspackages/core/src/service/services/promotion.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/src/service/services/promotion.service.ts
- packages/core/src/service/services/order.service.ts
2 participants
Description
Coupon codes are now validated case-insensitively, matching industry-standard behavior (Shopify, WooCommerce, Stripe, etc.).
Previously, a promotion with the coupon code
Summer20would rejectsummer20and throw aCouponCodeInvalidError.With this update, different casing variations of the same coupon code are treated as equivalent.
Changes
validateCouponCode()inPromotionServicenow usesLOWER()in the database query via TypeORM’sRaw()operator, and the redundant case-sensitive JS check (promotion.couponCode !== couponCode) has been removed.applyCouponCode()now stores the canonical coupon code from thePromotionentity instead of the user-typed casing.removeCouponCode(),Promotion.test(), andOrderModifiercoupon comparisons have been updated to use case-insensitive matching.validateCouponCode()to document case-insensitive behavior.Breaking Changes
None.
Existing coupon codes continue to work as before. Codes that previously failed due to casing differences will now succeed.
Checklist
📌 Always
👍 Most of the time
Fixes: #4364