TASK #00000 : Check course pricing and validate while creating intent#735
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces authoritative price verification for certificate bundles by integrating the LmsClientService into the PaymentService. It adds a new getCoursePricing method to fetch prices from the LMS and a resolveCanonicalOriginalAmount method to ensure the requested payment amount matches the official list price. Feedback includes suggestions to simplify the pricing return logic in the LMS client, parallelize course pricing requests for better performance, and ensure currency fallbacks are consistent with the rest of the system.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA new ChangesLMS Course Pricing Integration
Sequence DiagramsequenceDiagram
participant Client
participant PaymentService
participant LmsClientService
participant LmsService
participant PaymentProvider
Client->>PaymentService: initiatePayment(dto with CERTIFICATE_BUNDLE purpose)
PaymentService->>PaymentService: Check if CERTIFICATE_BUNDLE
PaymentService->>LmsClientService: getCoursePricing(courseId) for each course
LmsClientService->>LmsService: GET /lms-service/v1/courses/:courseId (with headers)
LmsService-->>LmsClientService: course pricing response
LmsClientService-->>PaymentService: normalized { courseId, amount, currency }
PaymentService->>PaymentService: Validate single currency & total matches request
PaymentService->>PaymentService: Compute canonical original amount from LMS
PaymentService->>PaymentService: Apply coupon/discount against canonical amount
PaymentService->>PaymentProvider: Initiate checkout with canonical amount
PaymentProvider-->>PaymentService: checkout session
PaymentService-->>Client: payment intent with discountAmount
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/payments/services/payment.service.ts (1)
140-158:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
dto.targets[0].contextIdruns before targets are validated.
primaryContextIdat line 143 is read beforeresolveCanonicalOriginalAmountperforms the "must include at least one course context" check at lines 84-88. Ifdto.targetsis empty/undefined the request will crash with aTypeErrorinstead of a clean 400. DTO-level validation (@ArrayNotEmpty()ontargets) or an early guard ininitiatePaymentwould harden this entry point.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/payments/services/payment.service.ts` around lines 140 - 158, initiatePayment reads dto.targets[0].contextId before targets are validated, which can cause a TypeError; fix by enforcing non-empty targets and guarding early: add `@ArrayNotEmpty`() (and any needed `@ValidateNested`()/@Type) to the InitiatePaymentDto.targets property so class-validator rejects empty arrays, and/or add an early check in payment.service.ts inside initiatePayment (before accessing dto.targets[0]) that verifies dto.targets is an array with length > 0 and throws a BadRequestException with a clear message; after adding the guard/DTO decorator, keep the existing primaryContextId assignment (dto.targets[0].contextId) and subsequent calls to resolveCanonicalOriginalAmount unchanged.
🧹 Nitpick comments (2)
src/payments/payments.module.ts (1)
26-54: ⚖️ Poor tradeoffConsider extracting
LmsClientServiceinto a dedicated module instead of pulling in all ofPathwaysModule.Importing
PathwaysModulefromPaymentsModuleonly to accessLmsClientServicecouples the payments domain to pathways' controllers, entities, and transitive imports (InterestsModule,StorageModule,CacheModule, etc.) — none of whichPaymentServiceuses. A smallLmsClientModule(providing/exporting onlyLmsClientServiceplusConfigModule) would keep the module graph minimal and avoid accidental circular dependencies as both domains evolve. Not blocking, but worth considering before this surface grows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/payments/payments.module.ts` around lines 26 - 54, PaymentsModule currently imports PathwaysModule just to consume LmsClientService; extract a thin LmsClientModule that declares and exports LmsClientService (and imports ConfigModule as needed), then replace PathwaysModule with LmsClientModule in the PaymentsModule imports and update providers/constructor injection targets (e.g., PaymentService) to consume LmsClientService from the new module; ensure LmsClientModule exports LmsClientService so DI continues to work and remove transitive PathwaysModule dependencies from PaymentsModule.src/pathways/common/services/lms-client.service.ts (1)
542-608: ⚖️ Poor tradeoffConsider distinguishing transient LMS failures from "no pricing" so callers can retry vs. reject.
Today every failure mode collapses to
null(network error, 5xx viavalidateStatus: status < 500actually throws, non-200, missingresult, etc.), which becomesBadRequestExceptionupstream. A transient LMS outage is then surfaced to the user as "Unable to verify pricing for course …", which masks a 5xx or timeout that would warrantServiceUnavailableException/ retry semantics. Returning a small typed result ({ status: 'ok' | 'not_found' | 'unavailable', ... }) — or at least throwing a distinct error for transient failures — would letPaymentServicemap each case to the correct HTTP response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pathways/common/services/lms-client.service.ts` around lines 542 - 608, The current getCoursePricing collapses all failures to null; change its contract to distinguish success/not-found/transient by either returning a typed union (e.g., Promise<{ status: 'ok'|'not_found'|'unavailable'; courseId?: string; amount?: number; currency?: string }>) or throwing a specific transient error (e.g., TransientLmsError) so callers (PaymentService) can retry/return 503. Update getCoursePricing to: treat HTTP 5xx or network/timeout (caught in the catch block or detected by res.status >= 500) as transient (return status: 'unavailable' or throw TransientLmsError), treat non-200 or missing result/pricing as 'not_found', and only return status: 'ok' with courseId/amount/currency when pricing is present; keep using resolvedCourseId logic and normalize currency as before. Ensure callers are updated to handle the new union/exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pathways/common/services/lms-client.service.ts`:
- Around line 582-600: The current method in lms-client.service that builds the
pricing object returns amount: 0 when pricing or pricing.amount is missing,
which allows paid courses to be treated as free; change its behavior to return
null (i.e., an overall null/unknown pricing result) when pricing is absent or
pricing.amount is undefined/null/NaN or non-numeric, rather than amount: 0.
Specifically, in the function that uses result/pricing and resolvedCourseId,
reject non-numeric values by validating Number(pricing.amount) is finite (not
NaN) before returning an object with amount, otherwise return null so callers
like PaymentService.resolveCanonicalOriginalAmount and amountsMatchClientTotal
fail closed and surface a BadRequestException upstream.
In `@src/payments/services/payment.service.ts`:
- Around line 77-104: The current PaymentService block uses BadRequestException
for server/configuration and LMS/unreachable failures; change the branch that
checks lmsUrl, tenantId, and organisationId to throw
InternalServerErrorException (do not expose raw config key names in the message)
and change the branch after this.lmsClientService.getCoursePricing(...) that
currently throws BadRequestException when pricing is null to throw
ServiceUnavailableException (or rethrow a more specific error from
getCoursePricing and map LMS 5xx/network errors to ServiceUnavailableException
while leaving genuine “no pricing record” as a 4xx only if getCoursePricing can
distinguish them); update messages for lmsUrl/tenantId/organisationId and the
pricing failure to be user-friendly and non-sensitive, and locate these edits
around the unique symbols lmsUrl, tenantId, organisationId, and getCoursePricing
in payment.service.ts.
- Around line 93-113: The current serial loop over uniqueCourseIds calling
this.lmsClientService.getCoursePricing causes per-course latency; change it to
fetch all pricings in parallel by mapping uniqueCourseIds to promises and
awaiting Promise.all, then iterate the resolved pricings to (1) ensure none are
undefined (throw BadRequestException with the specific courseId using the
resolved index), (2) enforce a single pricingCurrency (use the first
non-undefined pricing.currency as the canonical value and compare others,
throwing the existing error message if mismatched), and (3) sum pricing.amount
into total; keep references to uniqueCourseIds,
this.lmsClientService.getCoursePricing, pricingCurrency, and total when applying
the refactor.
---
Outside diff comments:
In `@src/payments/services/payment.service.ts`:
- Around line 140-158: initiatePayment reads dto.targets[0].contextId before
targets are validated, which can cause a TypeError; fix by enforcing non-empty
targets and guarding early: add `@ArrayNotEmpty`() (and any needed
`@ValidateNested`()/@Type) to the InitiatePaymentDto.targets property so
class-validator rejects empty arrays, and/or add an early check in
payment.service.ts inside initiatePayment (before accessing dto.targets[0]) that
verifies dto.targets is an array with length > 0 and throws a
BadRequestException with a clear message; after adding the guard/DTO decorator,
keep the existing primaryContextId assignment (dto.targets[0].contextId) and
subsequent calls to resolveCanonicalOriginalAmount unchanged.
---
Nitpick comments:
In `@src/pathways/common/services/lms-client.service.ts`:
- Around line 542-608: The current getCoursePricing collapses all failures to
null; change its contract to distinguish success/not-found/transient by either
returning a typed union (e.g., Promise<{ status: 'ok'|'not_found'|'unavailable';
courseId?: string; amount?: number; currency?: string }>) or throwing a specific
transient error (e.g., TransientLmsError) so callers (PaymentService) can
retry/return 503. Update getCoursePricing to: treat HTTP 5xx or network/timeout
(caught in the catch block or detected by res.status >= 500) as transient
(return status: 'unavailable' or throw TransientLmsError), treat non-200 or
missing result/pricing as 'not_found', and only return status: 'ok' with
courseId/amount/currency when pricing is present; keep using resolvedCourseId
logic and normalize currency as before. Ensure callers are updated to handle the
new union/exception.
In `@src/payments/payments.module.ts`:
- Around line 26-54: PaymentsModule currently imports PathwaysModule just to
consume LmsClientService; extract a thin LmsClientModule that declares and
exports LmsClientService (and imports ConfigModule as needed), then replace
PathwaysModule with LmsClientModule in the PaymentsModule imports and update
providers/constructor injection targets (e.g., PaymentService) to consume
LmsClientService from the new module; ensure LmsClientModule exports
LmsClientService so DI continues to work and remove transitive PathwaysModule
dependencies from PaymentsModule.
🪄 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: d956b7eb-6e67-4ac2-a837-cc50b238e69a
📒 Files selected for processing (4)
src/pathways/common/services/lms-client.service.tssrc/pathways/pathways.module.tssrc/payments/payments.module.tssrc/payments/services/payment.service.ts
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)
src/payments/services/payment.service.ts (2)
167-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBundle coupon eligibility is still based on the first target only.
canonicalOriginalAmountnow represents the full bundle total, butvalidateCoupon()is still fed onlydto.targets[0]. That makes promo eligibility depend on target ordering and can let a course-scoped coupon discount the entire bundle. Validate the promo against all distinct bundle contexts, or reject bundle promos until multi-target validation is supported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/payments/services/payment.service.ts` around lines 167 - 175, The code calls this.couponService.validateCoupon with only the first target (dto.targets[0]) while canonicalOriginalAmount is the full bundle total, causing incorrect eligibility; modify the logic in the payment flow to either (A) validate the promo across all distinct bundle contexts by iterating dto.targets (deduplicating by contextType+contextId) and calling couponService.validateCoupon for each context using canonicalOriginalAmount or per-context amounts as appropriate, aggregating/short-circuiting on failures, or (B) explicitly reject bundle promos when dto.targets.length > 1 by checking dto.promoCode and throwing/returning an error; update the block that references dto.promoCode, dto.targets, canonicalOriginalAmount and the call to couponService.validateCoupon to implement one of these fixes so eligibility is not determined solely by the first target.
145-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck every bundle course before creating the intent.
This guard only looks at
dto.targets[0].contextId. With the new multi-courseCERTIFICATE_BUNDLEflow, a user can still start a payment for a bundle where a latercontextIdis already paid, which risks duplicate charges/unlocks for part of the bundle.🛠️ Possible fix
- const primaryContextId = dto.targets[0].contextId; - const existingPaid = - await this.paymentIntentService.findPaidByUserIdAndContextId( - dto.userId, - primaryContextId, - ); + const contextIds = [...new Set(dto.targets.map((t) => t.contextId))]; + const existingPaid = ( + await Promise.all( + contextIds.map((contextId) => + this.paymentIntentService.findPaidByUserIdAndContextId( + dto.userId, + contextId, + ), + ), + ) + ).find(Boolean); if (existingPaid) { throw new ConflictException({ message: 'Already paid for this context.', alreadyPaid: true, paymentIntentId: existingPaid.id, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/payments/services/payment.service.ts` around lines 145 - 156, The current guard only checks dto.targets[0].contextId and misses paid contexts in other targets; update the check in the create-intent flow (the block calling paymentIntentService.findPaidByUserIdAndContextId) to iterate over all dto.targets, call paymentIntentService.findPaidByUserIdAndContextId(dto.userId, target.contextId) for each target, and if any call returns a paid intent throw the same ConflictException (include message, alreadyPaid: true and paymentIntentId: existingPaid.id) so the request is rejected when any bundle course/context is already paid.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/payments/services/payment.service.ts`:
- Around line 167-175: The code calls this.couponService.validateCoupon with
only the first target (dto.targets[0]) while canonicalOriginalAmount is the full
bundle total, causing incorrect eligibility; modify the logic in the payment
flow to either (A) validate the promo across all distinct bundle contexts by
iterating dto.targets (deduplicating by contextType+contextId) and calling
couponService.validateCoupon for each context using canonicalOriginalAmount or
per-context amounts as appropriate, aggregating/short-circuiting on failures, or
(B) explicitly reject bundle promos when dto.targets.length > 1 by checking
dto.promoCode and throwing/returning an error; update the block that references
dto.promoCode, dto.targets, canonicalOriginalAmount and the call to
couponService.validateCoupon to implement one of these fixes so eligibility is
not determined solely by the first target.
- Around line 145-156: The current guard only checks dto.targets[0].contextId
and misses paid contexts in other targets; update the check in the create-intent
flow (the block calling paymentIntentService.findPaidByUserIdAndContextId) to
iterate over all dto.targets, call
paymentIntentService.findPaidByUserIdAndContextId(dto.userId, target.contextId)
for each target, and if any call returns a paid intent throw the same
ConflictException (include message, alreadyPaid: true and paymentIntentId:
existingPaid.id) so the request is rejected when any bundle course/context is
already paid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8f021f92-9366-4fd8-8146-735abb6f19b4
📒 Files selected for processing (1)
src/payments/services/payment.service.ts
|
2 participants
Summary by CodeRabbit