Conversation
📝 WalkthroughWalkthroughThis PR adds transaction-based Paddle checkout as an alternative to price-based checkout. It introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller
participant Service
participant API as Paddle API
Client->>Controller: Request Paddle Checkout
Controller->>Controller: GetPaddleCheckoutProductId(plan)
alt Product ID Invalid
Controller->>Controller: Validate empty/whitespace
Controller-->>Client: HTTP 500 Error
else Product ID Valid
Controller->>Service: CreatePaddleCheckoutForSub(paddleProductId)
Service->>API: POST /CreatePaddleCheckoutForSubscription
API-->>Service: Response (with optional TransactionId)
Service-->>Controller: CheckoutData
Controller-->>Client: JSON Response (PriceId or TransactionId)
alt TransactionId Present
Client->>API: Open Checkout with transactionId
else Only PriceId Present
Client->>Client: Validate PriceId format (pri_*)
Client->>API: Open Checkout with priceId
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs (1)
115-118: Thin wrapper overplan.GetExternalKey()— consider inlining.
GetPaddleCheckoutProductIdjust delegates toplan?.GetExternalKey()with a null-coalesce. Given the only call site (GetPaddleCheckout) already hasplanin scope and performs a subsequentIsNullOrWhiteSpacecheck, the helper adds little abstraction and slightly obscures the data source. Either:
- Inline it:
var paddleProductId = plan?.GetExternalKey();followed by the existing whitespace guard (which already handles null), or- Give the helper a clearer responsibility (e.g., also validate the
pro_prefix Paddle expects for product IDs, mirroring the client-sidepri_guard added in the views).Keeping the rename
ExternalKey→PaddleCheckoutProductIdmakes sense only if the semantics are Paddle-specific; ifExternalIdis actually used for multiple providers, the current name is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs` around lines 115 - 118, Remove the thin helper GetPaddleCheckoutProductId and inline its usage in GetPaddleCheckout: replace calls to GetPaddleCheckoutProductId(plan) with plan?.GetExternalKey() and rely on the existing IsNullOrWhiteSpace guard in GetPaddleCheckout; alternatively, if you prefer a helper, expand GetPaddleCheckoutProductId to also validate Paddle-specific formatting (e.g., ensure the product id starts with "pro_") and rename it to PaddleCheckoutProductId for clear semantics — update all references to the helper accordingly.Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml (1)
609-624: Duplication withSelectRegistrationPlan.cshtml– consider extracting Paddle checkout JS.The transactionId branch and the
pri_prefix validation are an exact duplicate of the logic added toSelectRegistrationPlan.cshtml(lines 225-239). Consider extracting the Paddle checkout client routine into a shared static asset (e.g.,wwwroot/js/paddle-checkout.js) that both views include, so future changes (e.g., addingeventCallback, error mapping, telemetry) only need to happen in one place.Not blocking — but the two copies can easily drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml` around lines 609 - 624, The Paddle checkout logic duplicated in both views should be extracted into a single shared client script: move the branches that check data.TransactionId, call Paddle.Checkout.open, and validate data.PriceId (/^pri_/) into a reusable function (e.g., openPaddleCheckout(data, id, successUrl) that handles transactionId, PriceId validation, and error popups and supports extension points like eventCallback/telemetry); place that function in a new static asset (e.g., wwwroot/js/paddle-checkout.js), include it from both Index.cshtml and SelectRegistrationPlan.cshtml, and replace the duplicated blocks with a single call to the new function (referencing transactionId, Paddle.Checkout.open, data.PriceId and /^pri_/ in the replacement).Core/Resgrid.Model/Services/ISubscriptionsService.cs (1)
253-253: Add XML doc comment to clarifypaddleProductIdsemantics.The parameter rename from
paddlePriceId→paddleProductIdis consistent across the interface and implementation, and the single internal call site uses positional arguments. Add an XML doc comment to this method (unlike its siblings in the interface) to clarify thatpaddleProductIdrefers to a Paddle product identifier (formatpro_...), not a price identifier. This improves API clarity for developers maintaining this code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Services/ISubscriptionsService.cs` at line 253, Add an XML doc comment to the ISubscriptionsService.CreatePaddleCheckoutForSub method explaining that the paddleProductId parameter is a Paddle product identifier (e.g., starts with "pro_") and not a Paddle price id; update the method declaration for CreatePaddleCheckoutForSub in the ISubscriptionsService interface to include a <param name="paddleProductId"> entry that states it represents the Paddle product id (format "pro_...") to clarify semantics for callers and maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs`:
- Around line 789-794: Add a null check after calling
_subscriptionsService.GetPlanByIdAsync(id) to handle an invalid client-supplied
id: if plan is null return NotFound("Subscription plan not found.") from the
SubscriptionController action before calling GetPaddleCheckoutProductId(plan).
Keep the subsequent paddleProductId guard for misconfiguration but ensure the
GetPaddleCheckoutProductId(plan) call happens only when plan != null.
---
Nitpick comments:
In `@Core/Resgrid.Model/Services/ISubscriptionsService.cs`:
- Line 253: Add an XML doc comment to the
ISubscriptionsService.CreatePaddleCheckoutForSub method explaining that the
paddleProductId parameter is a Paddle product identifier (e.g., starts with
"pro_") and not a Paddle price id; update the method declaration for
CreatePaddleCheckoutForSub in the ISubscriptionsService interface to include a
<param name="paddleProductId"> entry that states it represents the Paddle
product id (format "pro_...") to clarify semantics for callers and maintainers.
In `@Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs`:
- Around line 115-118: Remove the thin helper GetPaddleCheckoutProductId and
inline its usage in GetPaddleCheckout: replace calls to
GetPaddleCheckoutProductId(plan) with plan?.GetExternalKey() and rely on the
existing IsNullOrWhiteSpace guard in GetPaddleCheckout; alternatively, if you
prefer a helper, expand GetPaddleCheckoutProductId to also validate
Paddle-specific formatting (e.g., ensure the product id starts with "pro_") and
rename it to PaddleCheckoutProductId for clear semantics — update all references
to the helper accordingly.
In `@Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml`:
- Around line 609-624: The Paddle checkout logic duplicated in both views should
be extracted into a single shared client script: move the branches that check
data.TransactionId, call Paddle.Checkout.open, and validate data.PriceId
(/^pri_/) into a reusable function (e.g., openPaddleCheckout(data, id,
successUrl) that handles transactionId, PriceId validation, and error popups and
supports extension points like eventCallback/telemetry); place that function in
a new static asset (e.g., wwwroot/js/paddle-checkout.js), include it from both
Index.cshtml and SelectRegistrationPlan.cshtml, and replace the duplicated
blocks with a single call to the new function (referencing transactionId,
Paddle.Checkout.open, data.PriceId and /^pri_/ in the replacement).
🪄 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: 8d7f0a68-1fb7-4506-8368-7794cee610f1
📒 Files selected for processing (6)
Core/Resgrid.Model/Billing/Api/CreatePaddleCheckoutResult.csCore/Resgrid.Model/Services/ISubscriptionsService.csCore/Resgrid.Services/SubscriptionsService.csWeb/Resgrid.Web/Areas/User/Controllers/SubscriptionController.csWeb/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Subscription/SelectRegistrationPlan.cshtml
| var plan = await _subscriptionsService.GetPlanByIdAsync(id); | ||
| var paddleProductId = GetPaddleCheckoutProductId(plan); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(paddleProductId)) | ||
| return StatusCode(StatusCodes.Status500InternalServerError, "Paddle checkout is not configured for this plan."); | ||
|
|
There was a problem hiding this comment.
Missing null-plan guard; 500 is misleading for an invalid id.
GetPlanByIdAsync(id) can return null for an invalid/unknown plan id (see SubscriptionsService.GetPlanByIdAsync which returns null on 404 or missing data). In that case:
paddleProductIdbecomesstring.Emptyvia the helper,- The code then returns 500 Internal Server Error with "Paddle checkout is not configured for this plan."
But that is actually a client-supplied bad id, not a server misconfiguration. A 404 (or 400) with a clear message would be more accurate and avoids false "5xx" alerting noise in observability tooling.
🛠️ Proposed fix
var plan = await _subscriptionsService.GetPlanByIdAsync(id);
+if (plan == null)
+ return NotFound("Plan not found.");
+
var paddleProductId = GetPaddleCheckoutProductId(plan);
if (string.IsNullOrWhiteSpace(paddleProductId))
return StatusCode(StatusCodes.Status500InternalServerError, "Paddle checkout is not configured for this plan.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs` around
lines 789 - 794, Add a null check after calling
_subscriptionsService.GetPlanByIdAsync(id) to handle an invalid client-supplied
id: if plan is null return NotFound("Subscription plan not found.") from the
SubscriptionController action before calling GetPaddleCheckoutProductId(plan).
Keep the subsequent paddleProductId guard for misconfiguration but ensure the
GetPaddleCheckoutProductId(plan) call happens only when plan != null.
|
Approve |
Summary by CodeRabbit
New Features
Improvements