-
Notifications
You must be signed in to change notification settings - Fork 9
🎟️ Coupon Validation Service Implementation #82
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
🎟️ Coupon Validation Service Implementation #82
Conversation
WalkthroughAdds a new coupon validation service with comprehensive checks, batch and lock flows, and a validation summary API. Introduces a typed error hierarchy for validation failures. Minor formatting fix in package.json scripts to correct JSON syntax. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client
participant ValidationSvc as CouponValidationService
participant CouponDB as CouponDbService
participant Wallet as WalletService
participant Contract as ContractService (simulated)
User->>Client: Redeem coupon (couponId, userId)
Client->>ValidationSvc: validateCouponForRedemption(couponId, userId)
ValidationSvc->>CouponDB: fetch coupon + relations
CouponDB-->>ValidationSvc: coupon
ValidationSvc->>ValidationSvc: validate ownership/status/expiration
ValidationSvc->>Wallet: validate user wallet
Wallet-->>ValidationSvc: ok/err
ValidationSvc->>ValidationSvc: check window/hours/geo/daily limits
ValidationSvc->>Contract: get status/ownership/redeemed?
Contract-->>ValidationSvc: contractData
ValidationSvc-->>Client: CouponValidationResult (isValid, errors, warnings, couponData, contractData)
sequenceDiagram
autonumber
actor User
participant Client
participant ValidationSvc as CouponValidationService
participant DB as Supabase RPC
participant Fallback as Validation (fallback path)
User->>Client: Redeem (lock)
Client->>ValidationSvc: validateAndLockCoupon(couponId, userId)
ValidationSvc->>DB: rpc('validate_and_lock_coupon', ...)
alt RPC success
DB-->>ValidationSvc: locked + result
ValidationSvc-->>Client: CouponValidationResult
else RPC failure
DB-->>ValidationSvc: error
ValidationSvc->>Fallback: validateCouponForRedemption(...)
Fallback-->>ValidationSvc: result
ValidationSvc-->>Client: CouponValidationResult (fallback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
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.
Actionable comments posted: 11
🧹 Nitpick comments (7)
package.json (2)
40-45
: Choose a single HTTP client (node-fetch vs undici vs built-in fetch)You’re depending on both node-fetch and undici while Node 18+ exposes global fetch. Carrying all three adds weight and increases surface area. Consider standardizing on one (prefer built-in fetch or undici) and dropping the rest.
47-61
: Unify test tooling (Jest vs Vitest) or justify bothScripts use Jest, but devDependencies include Vitest. Keeping both complicates maintenance and CI. Either:
- Remove Vitest, or
- Migrate scripts to Vitest, or
- Add a clear reason for both (e.g., unit vs E2E).
If you want, I can draft an npm script migration plan and a minimal config to move to a single framework.
src/types/coupon-validation-errors.ts (1)
10-74
: Type the error codes to avoid drift and improve DXDefine a string-literal union for codes and use it in the base class and subclasses. This prevents typos and centralizes allowed values.
Here’s an additive snippet to place above the classes:
export type ValidationErrorCode = | 'VALIDATION_ERROR' | 'COUPON_NOT_FOUND' | 'UNAUTHORIZED_REDEMPTION' | 'COUPON_EXPIRED' | 'COUPON_ALREADY_REDEEMED' | 'CONTRACT_VALIDATION_ERROR' | 'WALLET_VALIDATION_ERROR' | 'BUSINESS_HOURS_ERROR' | 'GEOGRAPHIC_RESTRICTION_ERROR' | 'DAILY_LIMIT_EXCEEDED' | 'REDEMPTION_WINDOW_ERROR' | 'COUPON_STATUS_ERROR';Then change
code: string
tocode: ValidationErrorCode
and pass typed codes in subclasses.src/services/coupon-validation.service.ts (4)
359-363
: Geo restriction policy: fail-closed vs fail-openCurrently, if user location lookup fails, the function returns and allows redemption. If geo restrictions are compliance-critical, prefer fail-closed with a clear error.
If you confirm fail-closed is desired, I’ll push a small patch to throw GeographicRestrictionError on lookup failure (and optionally allow a configurable bypass).
519-523
: Cache clearing is only logged, not executedYou log “Clearing validation cache” but do not interact with a cache service. If caching is intended, wire in the cache client here.
If you share which cache (Redis/In-memory/LRU) you’re using, I can add a small cache abstraction and implement proper invalidation.
591-612
: Summary generation can be slow; consider batching/concurrency controlValidating each active coupon sequentially can be N× round-trips. Switch to preValidateCouponBatch with bounded concurrency to keep latency reasonable for large coupon sets.
I can propose a Promise.allSettled with a p-limit of, say, 5–10 to balance throughput and DB load.
297-337
: Operating-hours check uses server local time; consider business timezoneUsing getDay/getHours on server time can reject/allow redemptions incorrectly when the business is in a different timezone. Consider storing a timezone per business and converting.
I can augment the coupon/project model with a timezone field and update this check to use it (via luxon or Intl).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
package.json
(1 hunks)src/services/coupon-validation.service.ts
(1 hunks)src/types/coupon-validation-errors.ts
(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/services/coupon-validation.service.ts (5)
src/types/coupon-validation-errors.ts (12)
ValidationError
(1-8)CouponNotFoundError
(10-14)UnauthorizedRedemptionError
(16-20)CouponAlreadyRedeemedError
(28-32)CouponStatusError
(70-74)CouponExpiredError
(22-26)WalletValidationError
(40-44)ContractValidationError
(34-38)RedemptionWindowError
(64-68)DailyLimitExceededError
(58-62)BusinessHoursError
(46-50)GeographicRestrictionError
(52-56)src/types/coupon.ts (2)
CouponWithRelations
(139-156)isCouponExpired
(119-121)src/services/coupon-db.service.ts (1)
CouponDbService
(14-398)src/services/stellar/index.ts (1)
accountService
(15-15)src/services/stellar/account.service.ts (1)
accountExists
(99-101)
🔇 Additional comments (1)
package.json (1)
24-25
: LGTM: trailing-comma fix in scriptsThe JSON fix on the manual IPFS test script looks correct and removes a parsing hazard. No behavioral change.
contractData = await validateContractCouponStatus(couponData.token_id); | ||
await validateContractOwnership(couponData.token_id, couponData.user_id); | ||
await validateContractNotRedeemed(couponData.token_id); |
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.
Critical: ownership check passes trivially (wrong userId used)
You pass couponData.user_id into validateContractOwnership, so the function compares coupon.user_id === coupon.user_id and always succeeds. Use the requester’s userId instead.
- await validateContractOwnership(couponData.token_id, couponData.user_id);
+ await validateContractOwnership(couponData.token_id, userId);
📝 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.
contractData = await validateContractCouponStatus(couponData.token_id); | |
await validateContractOwnership(couponData.token_id, couponData.user_id); | |
await validateContractNotRedeemed(couponData.token_id); | |
contractData = await validateContractCouponStatus(couponData.token_id); | |
await validateContractOwnership(couponData.token_id, userId); | |
await validateContractNotRedeemed(couponData.token_id); |
🤖 Prompt for AI Agents
In src/services/coupon-validation.service.ts around lines 50-52, the call
validateContractOwnership(couponData.user_id) incorrectly passes the coupon's
owner id (causing a trivial equality check); replace that argument with the
requester’s user id (e.g., userId or requesterId from the current function
scope/handler) so validateContractOwnership compares the coupon owner to the
actual requester. Ensure the requester id variable is in scope (add it as a
parameter if needed) and validate it before calling the function.
export async function validateCouponStatus(coupon: CouponWithRelations): Promise<void> { | ||
if (coupon.status === CouponStatus.REDEEMED) { | ||
throw new CouponAlreadyRedeemedError(); | ||
} | ||
|
||
if (coupon.status !== CouponStatus.ACTIVE) { | ||
throw new CouponStatusError(`Coupon status is ${coupon.status}, expected ACTIVE`); | ||
} | ||
} |
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.
🛠️ Refactor suggestion
Use redeemed_at as the source of truth for “already redeemed”
Per PR objectives, rely on redeemed_at to detect redemption (status can lag). Keep a status sanity-check after.
export async function validateCouponStatus(coupon: CouponWithRelations): Promise<void> {
- if (coupon.status === CouponStatus.REDEEMED) {
- throw new CouponAlreadyRedeemedError();
- }
-
- if (coupon.status !== CouponStatus.ACTIVE) {
- throw new CouponStatusError(`Coupon status is ${coupon.status}, expected ACTIVE`);
- }
+ if (coupon.redeemed_at != null) {
+ throw new CouponAlreadyRedeemedError();
+ }
+
+ if (coupon.status !== CouponStatus.ACTIVE) {
+ throw new CouponStatusError(`Coupon status is ${coupon.status}, expected ${CouponStatus.ACTIVE}`);
+ }
}
📝 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.
export async function validateCouponStatus(coupon: CouponWithRelations): Promise<void> { | |
if (coupon.status === CouponStatus.REDEEMED) { | |
throw new CouponAlreadyRedeemedError(); | |
} | |
if (coupon.status !== CouponStatus.ACTIVE) { | |
throw new CouponStatusError(`Coupon status is ${coupon.status}, expected ACTIVE`); | |
} | |
} | |
export async function validateCouponStatus(coupon: CouponWithRelations): Promise<void> { | |
if (coupon.redeemed_at != null) { | |
throw new CouponAlreadyRedeemedError(); | |
} | |
if (coupon.status !== CouponStatus.ACTIVE) { | |
throw new CouponStatusError(`Coupon status is ${coupon.status}, expected ${CouponStatus.ACTIVE}`); | |
} | |
} |
🤖 Prompt for AI Agents
In src/services/coupon-validation.service.ts around lines 77 to 85, use
coupon.redeemed_at as the primary check for already-redeemed and keep the status
sanity check afterwards: first throw CouponAlreadyRedeemedError when redeemed_at
is non-null/defined, then perform the existing status check (throw
CouponStatusError if status !== CouponStatus.ACTIVE) so that status lag won’t
incorrectly treat a redeemed coupon as merely non-active.
{/*For now, I am not calling the contract, I am just simulating the call | ||
replace with actual Soroban contract interaction */} | ||
const contractData = { |
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.
Invalid JSX-style block comments in .ts file will not compile
{/* ... */}
is JSX-only and will break TypeScript compilation here. Replace with regular block comments.
- {/*For now, I am not calling the contract, I am just simulating the call
- replace with actual Soroban contract interaction */}
+ /* For now, we are not calling the contract; simulate the call.
+ TODO: Replace with actual Soroban contract interaction. */
- {/*For now, I am not calling the contract,
- I am just simulating the call
- replace with actual Soroban contract call
- */}
+ /* For now, we are not calling the contract; simulate the call.
+ TODO: Replace with actual Soroban contract call. */
Also applies to: 194-198
🤖 Prompt for AI Agents
In src/services/coupon-validation.service.ts around lines 133-135 (and also at
194-198), there are JSX-style comments like {/* ... */} which will break
TypeScript compilation; replace those with standard TypeScript block comments /*
... */ (remove the surrounding curly braces) or line comments // as appropriate,
keeping the same text content and spacing so the file compiles.
const contractRedemptionStatus = { | ||
tokenId, | ||
isRedeemed: coupon.redeemed_at !== null, | ||
redeemedAt: coupon.redeemed_at | ||
}; | ||
|
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.
🛠️ Refactor suggestion
Treat null and undefined uniformly for redeemed_at
If historical rows have undefined, !== null
would mark them redeemed. Prefer != null
.
const contractRedemptionStatus = {
tokenId,
- isRedeemed: coupon.redeemed_at !== null,
+ isRedeemed: coupon.redeemed_at != null,
redeemedAt: coupon.redeemed_at
};
📝 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 contractRedemptionStatus = { | |
tokenId, | |
isRedeemed: coupon.redeemed_at !== null, | |
redeemedAt: coupon.redeemed_at | |
}; | |
const contractRedemptionStatus = { | |
tokenId, | |
isRedeemed: coupon.redeemed_at != null, | |
redeemedAt: coupon.redeemed_at | |
}; |
🤖 Prompt for AI Agents
In src/services/coupon-validation.service.ts around lines 198 to 203, the check
coupon.redeemed_at !== null treats undefined as not-null and will incorrectly
mark historical rows as redeemed; change the logic to treat null and undefined
uniformly by using a nullish comparison (coupon.redeemed_at != null) when
setting isRedeemed (or wrap that in Boolean(...)) and keep redeemedAt as the
original value (or normalize with ?? null) so both null and undefined are
handled consistently.
export function checkCouponExpiry(expirationDate: string): boolean { | ||
return new Date(expirationDate) > new Date(); | ||
} |
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.
Function name vs return value mismatch (checkCouponExpiry)
checkCouponExpiry currently returns true when NOT expired. Either rename or invert the boolean. Inverting is safer given it’s exported.
export function checkCouponExpiry(expirationDate: string): boolean {
- return new Date(expirationDate) > new Date();
+ return new Date(expirationDate) <= new Date();
}
📝 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.
export function checkCouponExpiry(expirationDate: string): boolean { | |
return new Date(expirationDate) > new Date(); | |
} | |
export function checkCouponExpiry(expirationDate: string): boolean { | |
return new Date(expirationDate) <= new Date(); | |
} |
🤖 Prompt for AI Agents
In src/services/coupon-validation.service.ts around lines 217 to 219, the
exported function checkCouponExpiry currently returns true when a coupon is NOT
expired; invert the boolean so it returns true when the coupon IS expired.
Change the comparison logic to return true when expirationDate is less than or
equal to now (i.e., treat past or equal dates as expired) and update any inline
comment if present; no renaming required.
const { data: todayRedemptions, error } = await supabase | ||
.from('coupons') | ||
.select('id') | ||
.eq('user_id', userId) | ||
.eq('status', 'redeemed') | ||
.gte('redeemed_at', today.toISOString()) | ||
.lt('redeemed_at', tomorrow.toISOString()); | ||
|
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.
🛠️ Refactor suggestion
Use enum for status filter in daily-limit query
Hard-coding 'redeemed' risks mismatch with DB values/enums. Use CouponStatus.REDEEMED.
- .eq('status', 'redeemed')
+ .eq('status', CouponStatus.REDEEMED)
📝 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 { data: todayRedemptions, error } = await supabase | |
.from('coupons') | |
.select('id') | |
.eq('user_id', userId) | |
.eq('status', 'redeemed') | |
.gte('redeemed_at', today.toISOString()) | |
.lt('redeemed_at', tomorrow.toISOString()); | |
const { data: todayRedemptions, error } = await supabase | |
.from('coupons') | |
.select('id') | |
.eq('user_id', userId) | |
.eq('status', CouponStatus.REDEEMED) | |
.gte('redeemed_at', today.toISOString()) | |
.lt('redeemed_at', tomorrow.toISOString()); |
🤖 Prompt for AI Agents
In src/services/coupon-validation.service.ts around lines 265 to 272, the
daily-limit Supabase query hard-codes the status string 'redeemed'; replace this
literal with the enum value CouponStatus.REDEEMED (importing CouponStatus from
its module if not already imported) so the filter always matches the canonical
status value used by the codebase/DB; ensure the import statement is added at
the top and that the query uses CouponStatus.REDEEMED in the .eq(...) call.
const dailyLimit = parseInt(process.env.DAILY_REDEMPTION_LIMIT || '10'); | ||
const todayCount = todayRedemptions?.length || 0; | ||
|
||
if (todayCount >= dailyLimit) { | ||
throw new DailyLimitExceededError(`Daily redemption limit of ${dailyLimit} exceeded`); | ||
} | ||
|
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.
💡 Verification agent
🧩 Analysis chain
Robust env parsing for daily limit; prevent NaN disabling limit
If DAILY_REDEMPTION_LIMIT is unset or invalid, parseInt can yield NaN and the comparison fails open. Guard it.
- const dailyLimit = parseInt(process.env.DAILY_REDEMPTION_LIMIT || '10');
+ const envLimit = Number(process.env.DAILY_REDEMPTION_LIMIT);
+ const dailyLimit = Number.isFinite(envLimit) && envLimit > 0 ? Math.floor(envLimit) : 10;
Also consider whether the limit should respect the user’s or business’s timezone; see the date range calculation above.
If timezone matters, I can wire in a tz parameter and compute start-of-day using UTC or a supplied IANA zone.
🏁 Script executed:
#!/usr/bin/env bash
set -eo pipefail
# Search for the DAILY_REDEMPTION_LIMIT environment variable usage
rg -n 'DAILY_REDEMPTION_LIMIT' -C3 src/services
# Locate the definition and usage of todayRedemptions in coupon validation
rg -n 'todayRedemptions' -C3 src/services/coupon-validation.service.ts
Length of output: 1283
🏁 Script executed:
#!/usr/bin/env bash
set -eo pipefail
# Locate where 'today' is defined to understand timezone usage
rg -nP --type=ts 'const today\s*=' -C5 src/services/coupon-validation.service.ts
# Dump the redemption query including any .gte/.lt filters to see if it’s using UTC vs local
sed -n '240,290p' src/services/coupon-validation.service.ts
Length of output: 2156
Guard against invalid DAILY_REDEMPTION_LIMIT (NaN fallback)
The current code at src/services/coupon-validation.service.ts:277–283
uses
const dailyLimit = parseInt(process.env.DAILY_REDEMPTION_LIMIT || '10');
If process.env.DAILY_REDEMPTION_LIMIT
is set to a non-numeric string (e.g. "abc"
), parseInt('abc')
yields NaN
. Since todayCount >= NaN
is always false, the limit check never fires → effectively no daily cap.
Please replace with a guard that validates the parsed value and falls back to 10
when invalid:
- const dailyLimit = parseInt(process.env.DAILY_REDEMPTION_LIMIT || '10');
+ const rawLimit = process.env.DAILY_REDEMPTION_LIMIT;
+ const parsedLimit = rawLimit !== undefined ? Number(rawLimit) : NaN;
+ const dailyLimit =
+ Number.isFinite(parsedLimit) && parsedLimit > 0
+ ? Math.floor(parsedLimit)
+ : 10;
Optional: extract this into a shared helper (e.g. getEnvInt('DAILY_REDEMPTION_LIMIT', 10)
) to keep parsing consistent across the codebase.
—
Timezone consideration (optional)
The logic computes “today” via
const today = new Date();
today.setHours(0, 0, 0, 0);
…
.gte('redeemed_at', today.toISOString())
which uses the server’s local timezone. If your business day should align to UTC or a specific IANA zone, consider:
- Injecting a timezone or locale from config.
- Using
setUTCHours(0,0,0,0)
instead ofsetHours
. - Leveraging a library like Luxon or date-fns-tz to compute start/end of day in the desired zone.
🤖 Prompt for AI Agents
In src/services/coupon-validation.service.ts around lines 277–283, the parsed
DAILY_REDEMPTION_LIMIT may be NaN (e.g. parseInt('abc')) which disables the cap;
validate the parsed value and fall back to 10 when invalid (e.g. parseInt then
if Number.isNaN(result) set to default), or replace with a shared helper like
getEnvInt('DAILY_REDEMPTION_LIMIT', 10) to centralize parsing/validation;
optionally, if you need day boundaries in UTC instead of server local time,
compute the day's start using UTC methods or a timezone-aware helper.
const validationPromises = coupons.map(async (coupon: any) => { | ||
try { | ||
const couponWithRelations = coupon as CouponWithRelations; | ||
const result = await validateCouponForRedemption(coupon.id, coupon.user_id); | ||
return result; | ||
} catch (error) { |
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.
Batch pre-validation validates against coupon owner, not the requester
For batch checks, you should validate against the provided userId; otherwise ownership checks don’t reflect the requester.
- const couponWithRelations = coupon as CouponWithRelations;
- const result = await validateCouponForRedemption(coupon.id, coupon.user_id);
+ const result = await validateCouponForRedemption(coupon.id, userId);
return result;
📝 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 validationPromises = coupons.map(async (coupon: any) => { | |
try { | |
const couponWithRelations = coupon as CouponWithRelations; | |
const result = await validateCouponForRedemption(coupon.id, coupon.user_id); | |
return result; | |
} catch (error) { | |
const validationPromises = coupons.map(async (coupon: any) => { | |
try { | |
const result = await validateCouponForRedemption(coupon.id, userId); | |
return result; | |
} catch (error) { | |
// … |
🤖 Prompt for AI Agents
In src/services/coupon-validation.service.ts around lines 422 to 427, the batch
pre-validation is calling validateCouponForRedemption with coupon.user_id (the
coupon owner) instead of the requester userId passed into the batch function;
change the call to pass the provided requester userId (e.g., use the outer-scope
userId variable) so ownership checks run against the requester, keep the
CouponWithRelations casting as needed, and ensure any typing reflects that the
second argument is the requester userId.
const result = await validateCouponForRedemption(couponId, coupon.user_id); | ||
|
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.
Refresh validation uses coupon owner, ignoring the provided userId
This can mask unauthorized access during refresh. Validate for the requester.
- const result = await validateCouponForRedemption(couponId, coupon.user_id);
+ const result = await validateCouponForRedemption(couponId, userId);
📝 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 result = await validateCouponForRedemption(couponId, coupon.user_id); | |
const result = await validateCouponForRedemption(couponId, userId); |
🤖 Prompt for AI Agents
In src/services/coupon-validation.service.ts around lines 537-538, the refresh
validation currently calls validateCouponForRedemption(couponId, coupon.user_id)
which uses the coupon owner and can hide unauthorized refresh attempts; change
the call to validateCouponForRedemption(couponId, userId) (i.e., the requester’s
userId variable passed into the refresh flow) so validation checks the
requesting user, and ensure the surrounding code uses the provided userId
variable (and not coupon.user_id) when enforcing access.
export class ValidationError extends Error { | ||
code: string; | ||
constructor(message: string, code = 'VALIDATION_ERROR') { | ||
super(message); | ||
this.code = code; | ||
Object.setPrototypeOf(this, ValidationError.prototype); | ||
} | ||
} |
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.
🛠️ Refactor suggestion
Harden custom Error base: set name/prototype correctly and add toJSON
Improve runtime behavior (proper instanceof across transpilation targets) and structured logging/serialization.
Apply this diff to ValidationError:
export class ValidationError extends Error {
code: string;
- constructor(message: string, code = 'VALIDATION_ERROR') {
- super(message);
- this.code = code;
- Object.setPrototypeOf(this, ValidationError.prototype);
- }
+ constructor(message: string, code = 'VALIDATION_ERROR') {
+ super(message);
+ // Ensure correct error name and prototype across TS/JS boundaries
+ this.name = new.target.name;
+ this.code = code;
+ Object.setPrototypeOf(this, new.target.prototype);
+ // Preserve clean stack (Node)
+ if ((Error as any).captureStackTrace) {
+ (Error as any).captureStackTrace(this, new.target);
+ }
+ }
+
+ toJSON() {
+ return { name: this.name, code: this.code, message: this.message };
+ }
}
📝 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.
export class ValidationError extends Error { | |
code: string; | |
constructor(message: string, code = 'VALIDATION_ERROR') { | |
super(message); | |
this.code = code; | |
Object.setPrototypeOf(this, ValidationError.prototype); | |
} | |
} | |
export class ValidationError extends Error { | |
code: string; | |
constructor(message: string, code = 'VALIDATION_ERROR') { | |
super(message); | |
// Ensure correct error name and prototype across TS/JS boundaries | |
this.name = new.target.name; | |
this.code = code; | |
Object.setPrototypeOf(this, new.target.prototype); | |
// Preserve clean stack (Node) | |
if ((Error as any).captureStackTrace) { | |
(Error as any).captureStackTrace(this, new.target); | |
} | |
} | |
toJSON() { | |
return { name: this.name, code: this.code, message: this.message }; | |
} | |
} |
🤖 Prompt for AI Agents
In src/types/coupon-validation-errors.ts around lines 1 to 8, the custom
ValidationError should explicitly set the error name, capture the stack, ensure
prototype/constructor are correct for instanceof across transpiled targets, and
provide a toJSON for structured logging; modify the constructor to (1) call
super(message), (2) set this.name = 'ValidationError', (3) call
Error.captureStackTrace(this, ValidationError) when available, (4) call
Object.setPrototypeOf(this, ValidationError.prototype) and set
ValidationError.prototype.constructor = ValidationError, and (5) add a toJSON()
method that returns { name: this.name, message: this.message, code: this.code,
stack: this.stack } so the error serializes cleanly.
@coderabbitai review |
✅ Actions performedReview triggered.
|
Ok, i will go through the coderabbit feedback and then update the code. |
Thanks |
Yea |
I would check for some open issues too. |
🎟️ Validating coupon status before redemption to prevent duplicate or invalid redemptions
🛠️ Issue
📚 Description
This PR introduces a robust coupon validation service to ensure secure and efficient coupon redemption. Key highlights include:
✅ Changes applied
Summary by CodeRabbit