Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughRegion-aware subscription pricing and UI were added (EU currency, multiplier, thresholds), a minimal Razor layout was introduced, an EU app URL was corrected, and weather-alert auto-message scheduling was implemented end-to-end (models, controller, service, and settings UI). Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as WeatherAlertService
participant DeptStore as DepartmentSettingsStore
participant TimeConv as TimeConverter
participant Notifier as NotificationSender
Scheduler->>DeptStore: Load WeatherAlertAutoMessageSchedule (JSON)
DeptStore-->>Scheduler: JSON or empty
Scheduler->>Scheduler: Parse JSON -> AutoMessageSeveritySchedule[]
Scheduler->>TimeConv: Convert current UTC -> dept local time
TimeConv-->>Scheduler: DeptLocalHour
Scheduler->>Scheduler: For each alert: ShouldSendAutoMessage(severity, localHour)
alt Schedule present and enabled for severity
Scheduler->>Notifier: Enqueue/send auto-message
else No schedule or disabled -> fallback
Scheduler->>DeptStore: Load WeatherAlertAutoMessageSeverity (legacy threshold)
DeptStore-->>Scheduler: threshold
Scheduler->>Notifier: Send if alert.Severity <= threshold
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Web/Resgrid.Web/Areas/User/Views/Subscription/SelectRegistrationPlan.cshtml (1)
157-158: Please centralize the pricing rules instead of duplicating them in both subscription views.This tier matrix, EU multiplier, and min-entity / pack-count logic now exists here and in
Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml, and the two flows have already drifted apart in this PR. Moving the calculation to a shared server-side helper/endpoint would make the checkout count and displayed totals much harder to desynchronize again.Also applies to: 229-278
🤖 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/SelectRegistrationPlan.cshtml` around lines 157 - 158, The pricing rules (tier matrix, IS_EU/EU_MULTIPLIER, and min-entity / pack-count logic) are duplicated between SelectRegistrationPlan.cshtml and Index.cshtml and have diverged; extract this logic into a single server-side helper (e.g., a new SubscriptionPricingHelper with a method like CalculatePlanTotals or GetPricingForPlan) or an endpoint that accepts plan parameters and returns computed totals, then replace the inline JS/variables (IS_EU, EU_MULTIPLIER, tier matrix, min-entity/pack logic) in both views to call that shared helper/endpoint so both flows use the same authoritative calculation.
🤖 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/Views/Shared/_MinimalLayout.cshtml`:
- Around line 30-35: The asp-fallback-src value on the script tag is
route-relative and breaks on nested routes; update the asp-fallback-src on the
<script> tag in _MinimalLayout.cshtml (the attribute
asp-fallback-src="lib/jquery/jquery-1.12.3.min.js") to be application-rooted
(e.g. prefix with "~/lib/..." or "/lib/...") so the browser requests the correct
file regardless of the current route; leave other attributes (asp-fallback-test,
integrity, crossorigin) unchanged.
In `@Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml`:
- Around line 9-12: The active subscription header still forces US formatting;
update the rendering of Model.Plan.Cost in the Index.cshtml header (the places
currently using Cultures.UnitedStates) to use the computed region info (e.g.,
the existing locationName/isEU or currencySymbol variables) so the header
formats amounts for EU departments the same as the calculator below; locate the
two places that format Model.Plan.Cost with Cultures.UnitedStates and switch
them to use the appropriate culture or symbol based on isEU/location.
- Around line 643-645: The EU checkout branch is still using the non-EU pack
math: update the stripeCheckout and paddleCheckout logic so EU purchases allow
amount == 10 and do not subtract one pack; specifically use IS_EU (or isEU) and
EU_MULTIPLIER to compute packs and price: for EU use packs = amount / 10 (or
Math.floor/Math.ceil as appropriate for your intent) and for non-EU keep packs =
(amount / 10) - 1, and change the condition from amount > 10 to amount >= 10
where IS_EU is true so a 10-entity EU purchase is accepted; adjust any price
calc to multiply by EU_MULTIPLIER when IS_EU is true.
In `@Web/Resgrid.Web/Areas/User/Views/Subscription/SelectRegistrationPlan.cshtml`:
- Around line 77-79: The EU branch in SelectRegistrationPlan.cshtml uses the
isEU conditional and renders "<text>Each</text>" which leads to "Each additional
pack of 10 entities…" elsewhere; remove the word "additional" for EU so the
sentence matches billing rules. Update the EU branch text (the <text>…</text>
fragment controlled by isEU) so the rendered phrase becomes "Each pack of 10
entities" (or adjust the surrounding fragments so the final sentence reads "Each
pack of 10 entities is billed…") ensuring the isEU conditional and the HTML
fragment generating that word are the only changes.
---
Nitpick comments:
In `@Web/Resgrid.Web/Areas/User/Views/Subscription/SelectRegistrationPlan.cshtml`:
- Around line 157-158: The pricing rules (tier matrix, IS_EU/EU_MULTIPLIER, and
min-entity / pack-count logic) are duplicated between
SelectRegistrationPlan.cshtml and Index.cshtml and have diverged; extract this
logic into a single server-side helper (e.g., a new SubscriptionPricingHelper
with a method like CalculatePlanTotals or GetPricingForPlan) or an endpoint that
accepts plan parameters and returns computed totals, then replace the inline
JS/variables (IS_EU, EU_MULTIPLIER, tier matrix, min-entity/pack logic) in both
views to call that shared helper/endpoint so both flows use the same
authoritative calculation.
🪄 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: 35664264-9642-4b0e-abce-184f171cf76a
📒 Files selected for processing (4)
Core/Resgrid.Config/InfoConfig.csWeb/Resgrid.Web/Areas/User/Views/Shared/_MinimalLayout.cshtmlWeb/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Subscription/SelectRegistrationPlan.cshtml
Comment on lines
+30
to
+35
| <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/1.12.3/jquery.min.js" | ||
| asp-fallback-src="lib/jquery/jquery-1.12.3.min.js" | ||
| asp-fallback-test="window.jQuery" | ||
| crossorigin="anonymous" | ||
| integrity="sha384-ugqypGWrzPLdx2zEQTF17cVktjb01piRKaDNnbYGRSxyEoeAm+MKZVtbDUYjxfZ6"> | ||
| </script> |
Contributor
There was a problem hiding this comment.
Fix the jQuery fallback URL.
asp-fallback-src="lib/jquery/jquery-1.12.3.min.js" is route-relative, so when the CDN fallback is needed on /User/... pages the browser will request the wrong path and all page JS will break.
🐛 Proposed fix
- asp-fallback-src="lib/jquery/jquery-1.12.3.min.js"
+ asp-fallback-src="~/lib/jquery/jquery-1.12.3.min.js"📝 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.
Suggested change
| <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/1.12.3/jquery.min.js" | |
| asp-fallback-src="lib/jquery/jquery-1.12.3.min.js" | |
| asp-fallback-test="window.jQuery" | |
| crossorigin="anonymous" | |
| integrity="sha384-ugqypGWrzPLdx2zEQTF17cVktjb01piRKaDNnbYGRSxyEoeAm+MKZVtbDUYjxfZ6"> | |
| </script> | |
| <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/1.12.3/jquery.min.js" | |
| asp-fallback-src="~/lib/jquery/jquery-1.12.3.min.js" | |
| asp-fallback-test="window.jQuery" | |
| crossorigin="anonymous" | |
| integrity="sha384-ugqypGWrzPLdx2zEQTF17cVktjb01piRKaDNnbYGRSxyEoeAm+MKZVtbDUYjxfZ6"> | |
| </script> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Shared/_MinimalLayout.cshtml` around lines
30 - 35, The asp-fallback-src value on the script tag is route-relative and
breaks on nested routes; update the asp-fallback-src on the <script> tag in
_MinimalLayout.cshtml (the attribute
asp-fallback-src="lib/jquery/jquery-1.12.3.min.js") to be application-rooted
(e.g. prefix with "~/lib/..." or "/lib/...") so the browser requests the correct
file regardless of the current route; leave other attributes (asp-fallback-test,
integrity, crossorigin) unchanged.
Web/Resgrid.Web/Areas/User/Views/Subscription/SelectRegistrationPlan.cshtml
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs (2)
360-364: Inconsistent JSON serializer usage.The file imports
System.Text.Json(line 14) but usesNewtonsoft.Jsonfor serializingAutoMessageSchedule. While both work, mixing serializers in the same file creates maintenance overhead and potential for subtle behavioral differences in edge cases.Consider using a consistent serializer throughout the controller.
♻️ Suggested refactor using System.Text.Json for consistency
-var scheduleJson = Newtonsoft.Json.JsonConvert.SerializeObject(input.AutoMessageSchedule); +var scheduleJson = JsonSerializer.Serialize(input.AutoMessageSchedule);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 360 - 364, The code serializes input.AutoMessageSchedule using Newtonsoft.Json.JsonConvert.SerializeObject which is inconsistent with the file's System.Text.Json usage; replace that call with System.Text.Json.JsonSerializer.Serialize to produce scheduleJson, preserving any required options (e.g., camelCase, ignore nulls) to match previous behavior, then pass the resulting string into _departmentSettingsService.SaveOrUpdateSettingAsync with DepartmentId and DepartmentSettingTypes.WeatherAlertAutoMessageSchedule.
402-409: Silent exception swallowing hinders debugging.The empty
catchblock silently ignores deserialization failures. If the stored JSON becomes malformed or the schema changes, this will silently fall back to legacy behavior with no indication of the issue.Consider logging the exception at debug/warning level.
🔧 Suggested fix to log deserialization failures
if (scheduleSetting != null && !string.IsNullOrWhiteSpace(scheduleSetting.Setting)) { try { settings.AutoMessageSchedule = Newtonsoft.Json.JsonConvert.DeserializeObject<System.Collections.Generic.List<WeatherAlertSeverityScheduleData>>(scheduleSetting.Setting); } - catch { } + catch (Exception ex) + { + // Log but continue - will fall back to legacy settings + Framework.Logging.LogError($"Failed to deserialize WeatherAlertAutoMessageSchedule for department {DepartmentId}: {ex.Message}"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 402 - 409, The empty catch swallows JSON deserialization errors for scheduleSetting.Setting; update the try/catch around Newtonsoft.Json.JsonConvert.DeserializeObject<WeatherAlertSeverityScheduleData> to catch (Exception ex) and log the exception (including scheduleSetting.Setting or its length) at debug/warning using the controller's logger (e.g., _logger or ILogger) so failures are visible, then leave settings.AutoMessageSchedule unchanged on failure.Core/Resgrid.Services/WeatherAlertService.cs (2)
323-341: Silent catch block and consistent exception handling.The empty
catchblock at line 330 silently swallows deserialization errors. This mirrors the same pattern in the controller. If the stored JSON is malformed, debugging will be difficult.Consider logging the error for observability:
🔧 Suggested fix
if (scheduleSetting != null && !string.IsNullOrWhiteSpace(scheduleSetting.Setting)) { - try { schedule = JsonConvert.DeserializeObject<List<AutoMessageSeveritySchedule>>(scheduleSetting.Setting); } - catch { } + try + { + schedule = JsonConvert.DeserializeObject<List<AutoMessageSeveritySchedule>>(scheduleSetting.Setting); + } + catch (Exception ex) + { + Logging.LogError($"Failed to deserialize WeatherAlertAutoMessageSchedule for department {departmentId}: {ex.Message}"); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/WeatherAlertService.cs` around lines 323 - 341, The empty catch swallowing JSON deserialization errors for AutoMessageSeveritySchedule makes failures invisible; update the try/catch around JsonConvert.DeserializeObject<List<AutoMessageSeveritySchedule>>(...) in WeatherAlertService (where schedule is set) to catch the exception into a variable and log it via the service logger (or process logger) with contextual info (departmentId and DepartmentSettingTypes.WeatherAlertAutoMessageSchedule), then leave schedule as null so the existing legacyThreshold fallback logic remains intact; ensure the catch does not rethrow so behavior is unchanged except now errors are observable.
647-653: Duplicate DTO class creates potential for schema drift.
AutoMessageSeveritySchedulehas identical properties toWeatherAlertSeverityScheduleDatain the Web.Services models. If one is updated without the other, deserialization could silently fail or produce unexpected behavior.Options:
- Move the shared type to
Core/Resgrid.Modelwhere both layers can reference it- Keep separate but add a comment noting the coupling
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/WeatherAlertService.cs` around lines 647 - 653, The AutoMessageSeveritySchedule DTO in WeatherAlertService duplicates WeatherAlertSeverityScheduleData from Web.Services and risks schema drift; remove the local AutoMessageSeveritySchedule class, replace its usages in WeatherAlertService methods with the shared WeatherAlertSeverityScheduleData type (or alternatively move the shared definition into Core/Resgrid.Model and update both projects to reference that model), and update namespaces/imports so WeatherAlertSeverityScheduleData is used consistently; if you choose to keep separate types, add a clear comment on AutoMessageSeveritySchedule that it must stay in sync with WeatherAlertSeverityScheduleData and add unit tests validating serialization compatibility.Web/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertSettingsData.cs (1)
14-20: Consider adding validation attributes for API consumers.The
StartHourandEndHourproperties should be constrained to 0-23, andSeverityto 0-4 (matching the dropdown values in the UI). While the UI enforces these constraints, API consumers could submit invalid values.Also note: A duplicate
AutoMessageSeverityScheduleclass with identical properties exists inWeatherAlertService.cs(lines 647-653). Consider reusing this public DTO in the service to avoid schema drift.🛡️ Suggested validation attributes
public class WeatherAlertSeverityScheduleData { + [Range(0, 4)] public int Severity { get; set; } public bool Enabled { get; set; } + [Range(0, 23)] public int StartHour { get; set; } + [Range(0, 23)] public int EndHour { get; set; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertSettingsData.cs` around lines 14 - 20, Add data annotation validation to the WeatherAlertSeverityScheduleData DTO: decorate Severity with [Range(0,4)] and StartHour/EndHour with [Range(0,23)], and ensure the file has "using System.ComponentModel.DataAnnotations;". Also remove or replace the duplicate AutoMessageSeveritySchedule in WeatherAlertService by reusing WeatherAlertSeverityScheduleData (or make AutoMessageSeveritySchedule reference/alias WeatherAlertSeverityScheduleData) so the service uses the single validated DTO to prevent schema drift.Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml (2)
625-627: Consider externalizingEU_MULTIPLIERto server-side configuration.The 1.25 multiplier is hardcoded in JavaScript. If this value needs adjustment (e.g., for tax or pricing changes), it would require a code deployment. Moving it to
SystemBehaviorConfigor a similar config source would allow runtime changes.🤖 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 625 - 627, The hardcoded JavaScript constant EU_MULTIPLIER should be moved to server-side configuration so it can be changed at runtime: add a property (e.g., EuMultiplier) to SystemBehaviorConfig (or existing config class), populate it from appsettings/environment, expose it to the view model used by the Subscription Index view, and replace the inline EU_MULTIPLIER in the view with the server-provided value; keep the IS_EU JS variable as-is and ensure the view/model names (EU_MULTIPLIER, IS_EU, SystemBehaviorConfig/EuMultiplier) are updated consistently.
722-730: EU tier construction assumes at least 2 base tiers exist.The logic
baseTiers.slice(2)and accessingbaseTiers[0]andbaseTiers[1]will fail or produce incorrect results if someone modifies the pricing tier arrays to have fewer than 2 entries. Consider adding a comment documenting this dependency.🤖 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 722 - 730, The EU-tier merge logic assumes baseTiers has at least two entries and will break if it doesn't; update the block that computes tiers (references: baseTiers, IS_EU, tiers) to guard against short arrays by checking baseTiers.length >= 2 before accessing baseTiers[0] and baseTiers[1], and fall back to a safe behavior (e.g. use baseTiers as-is or merge only when enough entries exist); also add a short comment above the logic documenting the dependency on having at least two base tiers so future changes won't introduce errors.
🤖 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/Views/Subscription/Index.cshtml`:
- Around line 743-745: The code uses Math.round when applying the EU multiplier
which strips cents before display; instead, remove Math.round and keep the
full-precision multiplication (i.e., set finalCost = finalCost * EU_MULTIPLIER
or just multiply when formatting) and rely on toFixed(2) when rendering; update
the IS_EU branch that references finalCost and EU_MULTIPLIER so finalCost
retains decimal precision for display.
---
Nitpick comments:
In `@Core/Resgrid.Services/WeatherAlertService.cs`:
- Around line 323-341: The empty catch swallowing JSON deserialization errors
for AutoMessageSeveritySchedule makes failures invisible; update the try/catch
around JsonConvert.DeserializeObject<List<AutoMessageSeveritySchedule>>(...) in
WeatherAlertService (where schedule is set) to catch the exception into a
variable and log it via the service logger (or process logger) with contextual
info (departmentId and DepartmentSettingTypes.WeatherAlertAutoMessageSchedule),
then leave schedule as null so the existing legacyThreshold fallback logic
remains intact; ensure the catch does not rethrow so behavior is unchanged
except now errors are observable.
- Around line 647-653: The AutoMessageSeveritySchedule DTO in
WeatherAlertService duplicates WeatherAlertSeverityScheduleData from
Web.Services and risks schema drift; remove the local
AutoMessageSeveritySchedule class, replace its usages in WeatherAlertService
methods with the shared WeatherAlertSeverityScheduleData type (or alternatively
move the shared definition into Core/Resgrid.Model and update both projects to
reference that model), and update namespaces/imports so
WeatherAlertSeverityScheduleData is used consistently; if you choose to keep
separate types, add a clear comment on AutoMessageSeveritySchedule that it must
stay in sync with WeatherAlertSeverityScheduleData and add unit tests validating
serialization compatibility.
In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs`:
- Around line 360-364: The code serializes input.AutoMessageSchedule using
Newtonsoft.Json.JsonConvert.SerializeObject which is inconsistent with the
file's System.Text.Json usage; replace that call with
System.Text.Json.JsonSerializer.Serialize to produce scheduleJson, preserving
any required options (e.g., camelCase, ignore nulls) to match previous behavior,
then pass the resulting string into
_departmentSettingsService.SaveOrUpdateSettingAsync with DepartmentId and
DepartmentSettingTypes.WeatherAlertAutoMessageSchedule.
- Around line 402-409: The empty catch swallows JSON deserialization errors for
scheduleSetting.Setting; update the try/catch around
Newtonsoft.Json.JsonConvert.DeserializeObject<WeatherAlertSeverityScheduleData>
to catch (Exception ex) and log the exception (including scheduleSetting.Setting
or its length) at debug/warning using the controller's logger (e.g., _logger or
ILogger) so failures are visible, then leave settings.AutoMessageSchedule
unchanged on failure.
In
`@Web/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertSettingsData.cs`:
- Around line 14-20: Add data annotation validation to the
WeatherAlertSeverityScheduleData DTO: decorate Severity with [Range(0,4)] and
StartHour/EndHour with [Range(0,23)], and ensure the file has "using
System.ComponentModel.DataAnnotations;". Also remove or replace the duplicate
AutoMessageSeveritySchedule in WeatherAlertService by reusing
WeatherAlertSeverityScheduleData (or make AutoMessageSeveritySchedule
reference/alias WeatherAlertSeverityScheduleData) so the service uses the single
validated DTO to prevent schema drift.
In `@Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml`:
- Around line 625-627: The hardcoded JavaScript constant EU_MULTIPLIER should be
moved to server-side configuration so it can be changed at runtime: add a
property (e.g., EuMultiplier) to SystemBehaviorConfig (or existing config
class), populate it from appsettings/environment, expose it to the view model
used by the Subscription Index view, and replace the inline EU_MULTIPLIER in the
view with the server-provided value; keep the IS_EU JS variable as-is and ensure
the view/model names (EU_MULTIPLIER, IS_EU, SystemBehaviorConfig/EuMultiplier)
are updated consistently.
- Around line 722-730: The EU-tier merge logic assumes baseTiers has at least
two entries and will break if it doesn't; update the block that computes tiers
(references: baseTiers, IS_EU, tiers) to guard against short arrays by checking
baseTiers.length >= 2 before accessing baseTiers[0] and baseTiers[1], and fall
back to a safe behavior (e.g. use baseTiers as-is or merge only when enough
entries exist); also add a short comment above the logic documenting the
dependency on having at least two base tiers so future changes won't introduce
errors.
🪄 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: 4e50bc74-a0fc-4f64-8b8b-d3e82a7ca8ec
📒 Files selected for processing (8)
Core/Resgrid.Model/DepartmentSettingTypes.csCore/Resgrid.Services/WeatherAlertService.csWeb/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertSettingsInput.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertSettingsData.csWeb/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Subscription/SelectRegistrationPlan.cshtmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml
✅ Files skipped from review due to trivial changes (1)
- Core/Resgrid.Model/DepartmentSettingTypes.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Web/Resgrid.Web/Areas/User/Views/Subscription/SelectRegistrationPlan.cshtml
Comment on lines
+743
to
745
| if (IS_EU) { | ||
| finalCost = Math.round(finalCost * EU_MULTIPLIER); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for EU multiplier or pricing calculation logic on the server side
rg -n -C3 'EU.*[Mm]ultiplier|1\.25' --type csRepository: Resgrid/Core
Length of output: 38
🏁 Script executed:
# First, let's examine the file mentioned in the review
wc -l "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml"Repository: Resgrid/Core
Length of output: 118
🏁 Script executed:
# Read the section around lines 743-745 to see context
sed -n '730,750p' "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml" | cat -nRepository: Resgrid/Core
Length of output: 982
🏁 Script executed:
# Look at the entire calculateCostFromUsers function mentioned in the comment
sed -n '699,748p' "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml" | cat -nRepository: Resgrid/Core
Length of output: 2832
🏁 Script executed:
# Search for EU_MULTIPLIER definition and IS_EU definition
rg -n 'EU_MULTIPLIER|IS_EU|1\.25' "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml"Repository: Resgrid/Core
Length of output: 874
🏁 Script executed:
# Search for server-side subscription pricing logic more broadly
rg -n 'multiplier|EU.*price|pricing' --type cs -i | grep -i 'subscription\|eu\|multiplier' | head -20Repository: Resgrid/Core
Length of output: 298
🏁 Script executed:
# Find where calculateCostFromUsers is called
rg -n 'calculateCostFromUsers' "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml"Repository: Resgrid/Core
Length of output: 318
🏁 Script executed:
# Look for toFixed usage and finalCost display
rg -n 'toFixed|finalCost' "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml" | head -30Repository: Resgrid/Core
Length of output: 553
🏁 Script executed:
# Search for where the calculated price is displayed to user
rg -n 'cost.*toFixed|price.*toFixed|amount.*toFixed' "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml"Repository: Resgrid/Core
Length of output: 38
🏁 Script executed:
# Search for server-side subscription validation/pricing logic
rg -n 'class.*Subscription|calculatePrice|validate.*price' --type cs -i | head -20Repository: Resgrid/Core
Length of output: 1401
🏁 Script executed:
# Look at the SubscriptionController to see what it does with amounts
sed -n '1,100p' "Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs" | cat -nRepository: Resgrid/Core
Length of output: 5002
🏁 Script executed:
# Search for checkout or payment processing
rg -n 'checkout|payment|charge|amount' --type cs "Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs" -i | head -20Repository: Resgrid/Core
Length of output: 1584
🏁 Script executed:
# Search for Paddle or Stripe integration that validates/charges amounts
rg -n 'Paddle|Stripe|checkout' --type cs | grep -i 'amount\|price\|cost' | head -20Repository: Resgrid/Core
Length of output: 1007
🏁 Script executed:
# Check what the view sends to backend after calculating cost
rg -n 'ajax\|post\|submit' "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml" -i -A2 | head -40Repository: Resgrid/Core
Length of output: 38
🏁 Script executed:
# Search for where amount/cost is used in checkout flow
rg -n 'checkout|payment|stripe|paddle' "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml" -i -B2 -A2 | head -50Repository: Resgrid/Core
Length of output: 3755
🏁 Script executed:
# Look at the SubscriptionsService to understand pricing
sed -n '1,100p' "Core/Resgrid.Services/SubscriptionsService.cs" | cat -nRepository: Resgrid/Core
Length of output: 5054
🏁 Script executed:
# Search for how CreatePaddleCheckoutForSub handles pricing
rg -n 'CreatePaddleCheckoutForSub' --type cs -B5 -A15Repository: Resgrid/Core
Length of output: 7907
🏁 Script executed:
# Search for where the JavaScript-calculated cost is used
rg -n 'totalCostMonthly|totalCostYearly|calculateCostFromUsers' "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml" -B2 -A2Repository: Resgrid/Core
Length of output: 754
🏁 Script executed:
# Look for stripeCheckout and paddleCheckout functions to see what parameters they pass
rg -n 'function.*stripeCheckout|function.*paddleCheckout|const.*stripeCheckout|const.*paddleCheckout' "Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml" -A15Repository: Resgrid/Core
Length of output: 2115
The Math.round() call is unnecessary for display purposes.
The calculated cost is only displayed to users via toFixed(2) and is not sent to the server for billing. The server uses Paddle/Stripe price IDs and the pack count to determine actual charges. However, Math.round(finalCost * EU_MULTIPLIER) still unnecessarily loses decimal precision for display (e.g., 152.50 rounds to 153, displayed as "153.00").
Use toFixed(2) directly instead:
Suggested fix
if (IS_EU) {
- finalCost = Math.round(finalCost * EU_MULTIPLIER);
+ finalCost = parseFloat((finalCost * EU_MULTIPLIER).toFixed(2));
}This preserves decimal precision for accurate display without unnecessary rounding.
🤖 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 743
- 745, The code uses Math.round when applying the EU multiplier which strips
cents before display; instead, remove Math.round and keep the full-precision
multiplication (i.e., set finalCost = finalCost * EU_MULTIPLIER or just multiply
when formatting) and rely on toFixed(2) when rendering; update the IS_EU branch
that references finalCost and EU_MULTIPLIER so finalCost retains decimal
precision for display.
Member
Author
|
Approve |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes