Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Contributor
📝 WalkthroughWalkthroughWalkthroughInbound SMS and chatbot flows now support restricted operation for unsupported active departments, including stable department options, ChangesSMS and chatbot department switching
Subscription page markup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Sender
participant TwilioController
participant DepartmentsService
participant UsersService
Sender->>TwilioController: Send SWITCH selector
TwilioController->>DepartmentsService: GetSmsSupportedMembershipsForUserAsync
DepartmentsService-->>TwilioController: Return ordered supported memberships
TwilioController->>UsersService: Resolve identity user
TwilioController->>DepartmentsService: SetActiveDepartmentForUserAsync
TwilioController-->>Sender: Return switch response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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: 5
🧹 Nitpick comments (2)
Web/Resgrid.Web.Services/Controllers/SignalWireController.cs (1)
52-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve
IUsersServicethrough the repository’s service locator.Remove the new constructor parameter and resolve
_usersServiceexplicitly in the constructor.Proposed fix
- ICommunicationTestService communicationTestService, IChatbotIngressService chatbotIngressService, IUsersService usersService) + ICommunicationTestService communicationTestService, IChatbotIngressService chatbotIngressService) ... - _usersService = usersService; + _usersService = Bootstrapper.GetKernel().Resolve<IUsersService>();As per coding guidelines, “Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection.”🤖 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 `@Web/Resgrid.Web.Services/Controllers/SignalWireController.cs` around lines 52 - 76, Update SignalWireController’s constructor to remove the IUsersService parameter and resolve _usersService explicitly via Bootstrapper.GetKernel().Resolve<IUsersService>(). Keep the existing field assignment pattern for all other constructor dependencies unchanged.Source: Coding guidelines
Core/Resgrid.Model/Services/IDepartmentsService.cs (1)
81-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd and propagate cache bypass for supported-membership retrieval.
Core/Resgrid.Model/Services/IDepartmentsService.cs#L81-L86: addbool bypassCache = falseto the contract.Core/Resgrid.Services/DepartmentsService.cs#L462-L483: propagate the option through membership and plan-limit retrieval.As per coding guidelines, “Service methods should include a
bypassCacheparameter (default:false) to allow callers to skip cache retrieval when fresh data is required.”🤖 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 `@Core/Resgrid.Model/Services/IDepartmentsService.cs` around lines 81 - 86, Add the optional bool bypassCache = false parameter to IDepartmentsService.GetSmsSupportedMembershipsForUserAsync, then propagate it through DepartmentsService.GetSmsSupportedMembershipsForUserAsync to the membership and plan-limit retrieval calls so callers can bypass both caches when requested. Update Core/Resgrid.Model/Services/IDepartmentsService.cs lines 81-86 and Core/Resgrid.Services/DepartmentsService.cs lines 462-483 accordingly.Source: Coding guidelines
🤖 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 `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs`:
- Around line 156-178: The restricted-department flow must use chatbot-specific
eligibility rather than SMS plan support alone. In
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs lines 156-178, filter
switchable departments by target chatbot enablement and the current platform; in
lines 237-268, pass that filtered set through restricted list and switch intent
dispatch. In Core/Resgrid.Chatbot/Handlers/DepartmentActionHandler.cs lines
40-42, apply the same chatbot-specific eligibility when listing departments and
resolving switch targets.
In `@Core/Resgrid.Services/DepartmentsService.cs`:
- Around line 470-480: Update the candidate filtering in the DepartmentMember
selection flow to exclude disabled memberships in addition to deleted ones
before switch authorization. Preserve the existing ordering and
provisioning-limit checks so only active, non-deleted memberships reach the
supported list used by the SMS/chatbot switch handlers.
In `@Core/Resgrid.Services/TextCommandService.cs`:
- Around line 23-31: Update the SWITCH detection in the text-command parsing
method to use a culture-invariant ordinal, whitespace-aware comparison,
accepting case-insensitive “switch” followed by any whitespace or end of input.
Preserve extracting and trimming the department argument after the command,
including tab-separated input, and avoid culture-sensitive ToLower-based
matching.
In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs`:
- Around line 432-434: Add the SWITCH command to the legacy Core Commands HELP
response in the relevant help-generation logic near the existing command
listings, using the same formatting and visibility rules as the other commands.
Keep the SwitchDepartment handling in HandleSwitchDepartmentCommandAsync
unchanged.
In `@Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml`:
- Around line 246-285: Guard the subscription plan details block containing
Model.Plan.Name and Model.Plan.Frequency so it renders only when Model.Plan is
non-null, and use null-safe access for Model.Payment when checking cancellation
state. Preserve the existing active/canceled labels, expiration text, and
pricing behavior for plans and payments that are present.
---
Nitpick comments:
In `@Core/Resgrid.Model/Services/IDepartmentsService.cs`:
- Around line 81-86: Add the optional bool bypassCache = false parameter to
IDepartmentsService.GetSmsSupportedMembershipsForUserAsync, then propagate it
through DepartmentsService.GetSmsSupportedMembershipsForUserAsync to the
membership and plan-limit retrieval calls so callers can bypass both caches when
requested. Update Core/Resgrid.Model/Services/IDepartmentsService.cs lines 81-86
and Core/Resgrid.Services/DepartmentsService.cs lines 462-483 accordingly.
In `@Web/Resgrid.Web.Services/Controllers/SignalWireController.cs`:
- Around line 52-76: Update SignalWireController’s constructor to remove the
IUsersService parameter and resolve _usersService explicitly via
Bootstrapper.GetKernel().Resolve<IUsersService>(). Keep the existing field
assignment pattern for all other constructor dependencies unchanged.
🪄 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: 40a9a742-e2a0-4f66-a183-cdd53ba5f9f2
📒 Files selected for processing (9)
Core/Resgrid.Chatbot/Handlers/DepartmentActionHandler.csCore/Resgrid.Chatbot/Services/ChatbotIngressService.csCore/Resgrid.Model/Services/IDepartmentsService.csCore/Resgrid.Model/TextCommandTypes.csCore/Resgrid.Services/DepartmentsService.csCore/Resgrid.Services/TextCommandService.csWeb/Resgrid.Web.Services/Controllers/SignalWireController.csWeb/Resgrid.Web.Services/Controllers/TwilioController.csWeb/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml
Comment on lines
+156
to
+178
| // 3b. Check the ACTIVE department's plan supports chatbot features. When it doesn't but the | ||
| // user belongs to other departments that do, don't hard-block: they must still be able to | ||
| // list departments and SWITCH their active department (restricted mode, enforced after | ||
| // intent classification below). Only when there is no supported alternative is this a dead end. | ||
| var isAuthorized = await _limitsService.CanDepartmentProvisionNumberAsync(department.DepartmentId); | ||
| List<Model.DepartmentMember> switchableDepartments = null; | ||
| if (!isAuthorized) | ||
| { | ||
| return await _templateRenderer.RenderResponseAsync("error", | ||
| new Services.ErrorModel { Message = "Your department's plan does not support chatbot features. Please upgrade your plan." }, | ||
| message.Platform, new ChatbotIntent { Type = ChatbotIntentType.Unknown }); | ||
| switchableDepartments = await _departmentsService.GetSmsSupportedMembershipsForUserAsync(identity.UserId); | ||
| if (switchableDepartments == null || switchableDepartments.Count == 0) | ||
| { | ||
| return await _templateRenderer.RenderResponseAsync("error", | ||
| new Services.ErrorModel { Message = "Your department's plan does not support chatbot features. Please upgrade your plan." }, | ||
| message.Platform, new ChatbotIntent { Type = ChatbotIntentType.Unknown }); | ||
| } | ||
| } | ||
|
|
||
| // 3c. Per-department configuration gates (when a config row exists; otherwise the | ||
| // system defaults apply and the chatbot stays enabled for backward compatibility). | ||
| // Skipped in restricted (switch-only) mode: the unsupported department's config must not | ||
| // block the user from switching OUT of it. | ||
| var deptConfig = await _departmentConfigService.GetConfigAsync(department.DepartmentId); | ||
| if (deptConfig != null) | ||
| if (deptConfig != null && isAuthorized) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use chatbot eligibility, not only SMS plan eligibility, for restricted switching.
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs#L156-L178: filter alternatives by target chatbot enablement and current platform.Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs#L237-L268: dispatch restricted list/switch intents using that filtered set.Core/Resgrid.Chatbot/Handlers/DepartmentActionHandler.cs#L40-L42: ensure listing and target resolution apply the same chatbot-specific eligibility.
Otherwise users can switch into a department that immediately rejects every subsequent chatbot request.
📍 Affects 2 files
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs#L156-L178(this comment)Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs#L237-L268Core/Resgrid.Chatbot/Handlers/DepartmentActionHandler.cs#L40-L42
🤖 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 `@Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs` around lines 156 -
178, The restricted-department flow must use chatbot-specific eligibility rather
than SMS plan support alone. In
Core/Resgrid.Chatbot/Services/ChatbotIngressService.cs lines 156-178, filter
switchable departments by target chatbot enablement and the current platform; in
lines 237-268, pass that filtered set through restricted list and switch intent
dispatch. In Core/Resgrid.Chatbot/Handlers/DepartmentActionHandler.cs lines
40-42, apply the same chatbot-specific eligibility when listing departments and
resolving switch targets.
Comment on lines
+470
to
+480
| var candidates = allMembers | ||
| .Where(m => !m.IsDeleted) | ||
| .OrderByDescending(m => m.IsActive) | ||
| .ThenBy(m => m.DepartmentId) | ||
| .ToList(); | ||
|
|
||
| var supported = new List<DepartmentMember>(); | ||
| foreach (var membership in candidates) | ||
| { | ||
| if (canProvisionNumber[i]) | ||
| return await GetDepartmentByIdAsync(ordered[i].DepartmentId, bypassCache); | ||
| if (await _limitsService.CanDepartmentProvisionNumberAsync(membership.DepartmentId)) | ||
| supported.Add(membership); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Exclude disabled memberships from switch authorization.
Every SMS/chatbot switch handler trusts this list. With only IsDeleted filtered, a disabled membership can be activated; the legacy Twilio path then executes commands without another membership-validity check.
Proposed fix
var candidates = allMembers
- .Where(m => !m.IsDeleted)
+ .Where(m => !m.IsDeleted && !m.IsDisabled.GetValueOrDefault())
.OrderByDescending(m => m.IsActive)📝 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
| var candidates = allMembers | |
| .Where(m => !m.IsDeleted) | |
| .OrderByDescending(m => m.IsActive) | |
| .ThenBy(m => m.DepartmentId) | |
| .ToList(); | |
| var supported = new List<DepartmentMember>(); | |
| foreach (var membership in candidates) | |
| { | |
| if (canProvisionNumber[i]) | |
| return await GetDepartmentByIdAsync(ordered[i].DepartmentId, bypassCache); | |
| if (await _limitsService.CanDepartmentProvisionNumberAsync(membership.DepartmentId)) | |
| supported.Add(membership); | |
| var candidates = allMembers | |
| .Where(m => !m.IsDeleted && !m.IsDisabled.GetValueOrDefault()) | |
| .OrderByDescending(m => m.IsActive) | |
| .ThenBy(m => m.DepartmentId) | |
| .ToList(); | |
| var supported = new List<DepartmentMember>(); | |
| foreach (var membership in candidates) | |
| { | |
| if (await _limitsService.CanDepartmentProvisionNumberAsync(membership.DepartmentId)) | |
| supported.Add(membership); |
🤖 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 `@Core/Resgrid.Services/DepartmentsService.cs` around lines 470 - 480, Update
the candidate filtering in the DepartmentMember selection flow to exclude
disabled memberships in addition to deleted ones before switch authorization.
Preserve the existing ordering and provisioning-limit checks so only active,
non-deleted memberships reach the supported list used by the SMS/chatbot switch
handlers.
Comment on lines
+23
to
+31
| // SWITCH [department name/number]: change the user's active department. Checked first — the | ||
| // custom staffing branch below claims any message starting with 's' and would swallow it. | ||
| if (message.Trim().ToLower() == "switch" || message.Trim().ToLower().StartsWith("switch ")) | ||
| { | ||
| payload.Type = TextCommandTypes.SwitchDepartment; | ||
| payload.Data = message.Trim().Length > 6 ? message.Trim().Substring(6).Trim() : ""; | ||
| return payload; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,120p' Core/Resgrid.Services/TextCommandService.csRepository: Resgrid/Core
Length of output: 4460
🏁 Script executed:
python3 - <<'PY'
import locale
samples = ["SWITCH", "switch", "switch\tfoo", "switch\nfoo"]
print("Current locale:", locale.getlocale())
for s in samples:
t = s.strip().lower()
print(repr(s), "->", repr(t), t == "switch", t.startswith("switch "))
PYRepository: Resgrid/Core
Length of output: 329
🏁 Script executed:
rg -n "ToLower\(\)|StartsWith\\(\"switch|switch" Core/Resgrid.Services/TextCommandService.csRepository: Resgrid/Core
Length of output: 1860
Parse SWITCH with an ordinal, whitespace-aware comparison.
ToLower() makes the match culture-sensitive, and StartsWith("switch ") only accepts a literal space, so inputs like SWITCH in Turkish locales or SWITCH\t123 are rejected.
Proposed fix
- if (message.Trim().ToLower() == "switch" || message.Trim().ToLower().StartsWith("switch "))
+ var trimmedMessage = message.Trim();
+ if (trimmedMessage.StartsWith("switch", StringComparison.OrdinalIgnoreCase)
+ && (trimmedMessage.Length == 6 || char.IsWhiteSpace(trimmedMessage[6])))
{
payload.Type = TextCommandTypes.SwitchDepartment;
- payload.Data = message.Trim().Length > 6 ? message.Trim().Substring(6).Trim() : "";
+ payload.Data = trimmedMessage.Length > 6 ? trimmedMessage.Substring(6).Trim() : "";📝 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
| // SWITCH [department name/number]: change the user's active department. Checked first — the | |
| // custom staffing branch below claims any message starting with 's' and would swallow it. | |
| if (message.Trim().ToLower() == "switch" || message.Trim().ToLower().StartsWith("switch ")) | |
| { | |
| payload.Type = TextCommandTypes.SwitchDepartment; | |
| payload.Data = message.Trim().Length > 6 ? message.Trim().Substring(6).Trim() : ""; | |
| return payload; | |
| } | |
| // SWITCH [department name/number]: change the user's active department. Checked first — the | |
| // custom staffing branch below claims any message starting with 's' and would swallow it. | |
| var trimmedMessage = message.Trim(); | |
| if (trimmedMessage.StartsWith("switch", StringComparison.OrdinalIgnoreCase) | |
| && (trimmedMessage.Length == 6 || char.IsWhiteSpace(trimmedMessage[6]))) | |
| { | |
| payload.Type = TextCommandTypes.SwitchDepartment; | |
| payload.Data = trimmedMessage.Length > 6 ? trimmedMessage.Substring(6).Trim() : ""; | |
| return payload; | |
| } |
🤖 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 `@Core/Resgrid.Services/TextCommandService.cs` around lines 23 - 31, Update the
SWITCH detection in the text-command parsing method to use a culture-invariant
ordinal, whitespace-aware comparison, accepting case-insensitive “switch”
followed by any whitespace or end of input. Preserve extracting and trimming the
department argument after the command, including tab-separated input, and avoid
culture-sensitive ToLower-based matching.
Comment on lines
+432
to
+434
| case TextCommandTypes.SwitchDepartment: | ||
| await HandleSwitchDepartmentCommandAsync(messageEvent, response, profile, payload.Data); | ||
| break; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Advertise SWITCH in the legacy HELP response.
The command is now accepted here, but the Core Commands help at Lines 375-426 omits it, leaving supported-department users unable to discover department switching.
🤖 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 `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` around lines 432 -
434, Add the SWITCH command to the legacy Core Commands HELP response in the
relevant help-generation logic near the existing command listings, using the
same formatting and visibility rules as the other commands. Keep the
SwitchDepartment handling in HandleSwitchDepartmentCommandAsync unchanged.
Comment on lines
+246
to
+285
| <h3> | ||
| @Model.Plan.Name | ||
|
|
||
| @if (Model.Payment.Cancelled) | ||
| { | ||
| <span class="label label-danger">@localizer["Canceled"]</span> | ||
| } | ||
| else | ||
| { | ||
| <span class="label label-success">@localizer["Active"]</span> | ||
| } | ||
| </h3> | ||
| @if (Model.Payment.Cancelled) | ||
| { | ||
| <span>@localizer["PlanExpires"]: @Model.Expires</span> | ||
| <span class="label label-danger">@localizer["Canceled"]</span> | ||
| } | ||
| else | ||
| { | ||
| <span>@localizer["PlanRenews"]: @Model.Expires</span> | ||
| <span class="label label-success">@localizer["Active"]</span> | ||
| } | ||
| </div> | ||
| </h3> | ||
| @if (Model.Payment.Cancelled) | ||
| { | ||
| <span>@localizer["PlanExpires"]: @Model.Expires</span> | ||
| } | ||
| else | ||
| { | ||
| <span>@localizer["PlanRenews"]: @Model.Expires</span> | ||
| } | ||
| </div> | ||
| <div class="row" style="padding-bottom: 10px;"> | ||
| <div class="col-md-4"> | ||
| <h3 style="margin-bottom: 0;"> | ||
| @if (Model.Plan.Frequency == (int)PlanFrequency.Yearly) | ||
| { | ||
| <span> | ||
| @currencySymbol@Model.Plan.Cost.ToString("N2")<small>/@localizer["Year"]</small> | ||
| </span> | ||
| } | ||
| else | ||
| { | ||
| <span> | ||
| @currencySymbol@Model.Plan.Cost.ToString("N2")<small>/@localizer["month"]</small> | ||
| </span> | ||
| } | ||
| </h3> | ||
| </div> | ||
| <div class="row" style="padding-bottom: 10px;"> | ||
| <div class="col-md-4"> | ||
| <h3 style="margin-bottom: 0;"> | ||
| @if (Model.Plan.Frequency == (int)PlanFrequency.Yearly) | ||
| { | ||
| <span> | ||
| @currencySymbol@Model.Plan.Cost.ToString("N2")<small>/@localizer["Year"]</small> | ||
| </span> | ||
| } | ||
| else | ||
| { | ||
| <span> | ||
| @currencySymbol@Model.Plan.Cost.ToString("N2")<small>/@localizer["month"]</small> | ||
| </span> | ||
| } | ||
| </h3> | ||
| </div> | ||
| </div> |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Add missing null checks for Model.Plan and Model.Payment.
As per coding guidelines, the SubscriptionsService will return null for the current plan if the user is on the free tier. Accessing @Model.Plan.Name or @Model.Plan.Frequency directly without checking will cause a NullReferenceException and crash the view. Similarly, Model.Payment should be null-checked to prevent crashes.
Wrap the plan details in a null check and use the null-conditional operator to prevent runtime errors.
🐛 Proposed fix for null reference exceptions
- <h3>
- `@Model.Plan.Name`
-
- `@if` (Model.Payment.Cancelled)
- {
- <span class="label label-danger">`@localizer`["Canceled"]</span>
- }
- else
- {
- <span class="label label-success">`@localizer`["Active"]</span>
- }
- </h3>
- `@if` (Model.Payment.Cancelled)
- {
- <span>`@localizer`["PlanExpires"]: `@Model.Expires`</span>
- }
- else
- {
- <span>`@localizer`["PlanRenews"]: `@Model.Expires`</span>
- }
- </div>
- </div>
- <div class="row" style="padding-bottom: 10px;">
- <div class="col-md-4">
- <h3 style="margin-bottom: 0;">
- `@if` (Model.Plan.Frequency == (int)PlanFrequency.Yearly)
- {
- <span>
- `@currencySymbol`@Model.Plan.Cost.ToString("N2")<small>/@localizer["Year"]</small>
- </span>
- }
- else
- {
- <span>
- `@currencySymbol`@Model.Plan.Cost.ToString("N2")<small>/@localizer["month"]</small>
- </span>
- }
- </h3>
- </div>
- </div>
+ <h3>
+ @(Model.Plan?.Name ?? "Free Plan")
+
+ `@if` (Model.Payment?.Cancelled == true)
+ {
+ <span class="label label-danger">`@localizer`["Canceled"]</span>
+ }
+ else if (Model.Plan != null && Model.Plan.PlanId != 1)
+ {
+ <span class="label label-success">`@localizer`["Active"]</span>
+ }
+ </h3>
+ `@if` (Model.Payment?.Cancelled == true)
+ {
+ <span>`@localizer`["PlanExpires"]: `@Model.Expires`</span>
+ }
+ else if (Model.Plan != null && Model.Plan.PlanId != 1)
+ {
+ <span>`@localizer`["PlanRenews"]: `@Model.Expires`</span>
+ }
+ </div>
+ </div>
+ `@if` (Model.Plan != null)
+ {
+ <div class="row" style="padding-bottom: 10px;">
+ <div class="col-md-4">
+ <h3 style="margin-bottom: 0;">
+ `@if` (Model.Plan.Frequency == (int)PlanFrequency.Yearly)
+ {
+ <span>
+ `@currencySymbol`@Model.Plan.Cost.ToString("N2")<small>/@localizer["Year"]</small>
+ </span>
+ }
+ else
+ {
+ <span>
+ `@currencySymbol`@Model.Plan.Cost.ToString("N2")<small>/@localizer["month"]</small>
+ </span>
+ }
+ </h3>
+ </div>
+ </div>
+ }📝 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
| <h3> | |
| @Model.Plan.Name | |
| @if (Model.Payment.Cancelled) | |
| { | |
| <span class="label label-danger">@localizer["Canceled"]</span> | |
| } | |
| else | |
| { | |
| <span class="label label-success">@localizer["Active"]</span> | |
| } | |
| </h3> | |
| @if (Model.Payment.Cancelled) | |
| { | |
| <span>@localizer["PlanExpires"]: @Model.Expires</span> | |
| <span class="label label-danger">@localizer["Canceled"]</span> | |
| } | |
| else | |
| { | |
| <span>@localizer["PlanRenews"]: @Model.Expires</span> | |
| <span class="label label-success">@localizer["Active"]</span> | |
| } | |
| </div> | |
| </h3> | |
| @if (Model.Payment.Cancelled) | |
| { | |
| <span>@localizer["PlanExpires"]: @Model.Expires</span> | |
| } | |
| else | |
| { | |
| <span>@localizer["PlanRenews"]: @Model.Expires</span> | |
| } | |
| </div> | |
| <div class="row" style="padding-bottom: 10px;"> | |
| <div class="col-md-4"> | |
| <h3 style="margin-bottom: 0;"> | |
| @if (Model.Plan.Frequency == (int)PlanFrequency.Yearly) | |
| { | |
| <span> | |
| @currencySymbol@Model.Plan.Cost.ToString("N2")<small>/@localizer["Year"]</small> | |
| </span> | |
| } | |
| else | |
| { | |
| <span> | |
| @currencySymbol@Model.Plan.Cost.ToString("N2")<small>/@localizer["month"]</small> | |
| </span> | |
| } | |
| </h3> | |
| </div> | |
| <div class="row" style="padding-bottom: 10px;"> | |
| <div class="col-md-4"> | |
| <h3 style="margin-bottom: 0;"> | |
| @if (Model.Plan.Frequency == (int)PlanFrequency.Yearly) | |
| { | |
| <span> | |
| @currencySymbol@Model.Plan.Cost.ToString("N2")<small>/@localizer["Year"]</small> | |
| </span> | |
| } | |
| else | |
| { | |
| <span> | |
| @currencySymbol@Model.Plan.Cost.ToString("N2")<small>/@localizer["month"]</small> | |
| </span> | |
| } | |
| </h3> | |
| </div> | |
| </div> | |
| <h3> | |
| @(Model.Plan?.Name ?? "Free Plan") | |
| `@if` (Model.Payment?.Cancelled == true) | |
| { | |
| <span class="label label-danger">`@localizer`["Canceled"]</span> | |
| } | |
| else if (Model.Plan != null && Model.Plan.PlanId != 1) | |
| { | |
| <span class="label label-success">`@localizer`["Active"]</span> | |
| } | |
| </h3> | |
| `@if` (Model.Payment?.Cancelled == true) | |
| { | |
| <span>`@localizer`["PlanExpires"]: `@Model.Expires`</span> | |
| } | |
| else if (Model.Plan != null && Model.Plan.PlanId != 1) | |
| { | |
| <span>`@localizer`["PlanRenews"]: `@Model.Expires`</span> | |
| } | |
| </div> | |
| </div> | |
| `@if` (Model.Plan != null) | |
| { | |
| <div class="row" style="padding-bottom: 10px;"> | |
| <div class="col-md-4"> | |
| <h3 style="margin-bottom: 0;"> | |
| `@if` (Model.Plan.Frequency == (int)PlanFrequency.Yearly) | |
| { | |
| <span> | |
| `@currencySymbol`@Model.Plan.Cost.ToString("N2")<small>/@localizer["Year"]</small> | |
| </span> | |
| } | |
| else | |
| { | |
| <span> | |
| `@currencySymbol`@Model.Plan.Cost.ToString("N2")<small>/@localizer["month"]</small> | |
| </span> | |
| } | |
| </h3> | |
| </div> | |
| </div> | |
| } |
🤖 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 `@Web/Resgrid.Web/Areas/User/Views/Subscription/Index.cshtml` around lines 246
- 285, Guard the subscription plan details block containing Model.Plan.Name and
Model.Plan.Frequency so it renders only when Model.Plan is non-null, and use
null-safe access for Model.Payment when checking cancellation state. Preserve
the existing active/canceled labels, expiration text, and pricing behavior for
plans and payments that are present.
Source: Coding guidelines
Resgrid-Bot
reviewed
Jul 16, 2026
Resgrid-Bot
reviewed
Jul 16, 2026
Comment on lines
+482
to
+487
| for (int i = 0; i < supported.Count; i++) | ||
| { | ||
| var dept = await _departmentsService.GetDepartmentByIdAsync(supported[i].DepartmentId); | ||
| var activeMarker = supported[i].IsActive ? " (active)" : ""; | ||
| sb.Append($"{i + 1}: {dept?.Name ?? ("Department " + supported[i].DepartmentId)}{activeMarker}" + Environment.NewLine); | ||
| } |
There was a problem hiding this comment.
N+1 query performance issue in BuildSwitchOptionsMessageAsync (lines 482-484) and HandleSwitchDepartmentCommandAsync (lines 437-439), which also impacts TwilioController.cs:777-782 and ChatbotIngressService.cs:261-265. The loops call GetDepartmentByIdAsync per membership, executing individual database queries because bypassCache=true by default; batch-fetch all department data before iterating to resolve this.
Kody rule violation: Detect N+1 style queries and suggest batching
// Batch-load all department entities once before the loop:
var deptIds = supported.Select(m => m.DepartmentId).ToList();
var departments = await _departmentsService.GetDepartmentsByIdsAsync(deptIds);
for (int i = 0; i < supported.Count; i++)
{
departments.TryGetValue(supported[i].DepartmentId, out var dept);
var activeMarker = supported[i].IsActive ? " (active)" : "";
sb.Append($"{i + 1}: {dept?.Name ?? ("Department " + supported[i].DepartmentId)}{activeMarker}" + Environment.NewLine);
}Prompt for LLM
File Web/Resgrid.Web.Services/Controllers/SignalWireController.cs:
Line 482 to 487:
Violates rule 'Detect N+1 style queries and suggest batching': BuildSwitchOptionsMessageAsync (lines 482-484) and HandleSwitchDepartmentCommandAsync (lines 437-439) both loop over the supported-department list calling GetDepartmentByIdAsync per membership. Since GetDepartmentByIdAsync defaults to bypassCache=true, each iteration issues a separate database query. Recommend fetching all department data in a single batched call before iterating.
Suggested Code:
// Batch-load all department entities once before the loop:
var deptIds = supported.Select(m => m.DepartmentId).ToList();
var departments = await _departmentsService.GetDepartmentsByIdsAsync(deptIds);
for (int i = 0; i < supported.Count; i++)
{
departments.TryGetValue(supported[i].DepartmentId, out var dept);
var activeMarker = supported[i].IsActive ? " (active)" : "";
sb.Append($"{i + 1}: {dept?.Name ?? ("Department " + supported[i].DepartmentId)}{activeMarker}" + Environment.NewLine);
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Resgrid-Bot
reviewed
Jul 16, 2026
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Resgrid-Bot
reviewed
Jul 16, 2026
| return IsPlatformAllowed(config.AllowedPlatforms, platform); | ||
| } | ||
|
|
||
| // Mirrors ChatbotIngressService.IsPlatformAllowed: null/blank/"*" = all platforms, otherwise a |
There was a problem hiding this comment.
Duplicated logic in the IsPlatformAllowed method (lines 66-79) verbatim copies the identical private static method in ChatbotIngressService.cs (lines 556-569). Extract this parsing logic into a shared utility or extension method so both services reference a single implementation.
Kody rule violation: Extract duplicated logic into functions
// In a shared utility, e.g. ChatbotPlatformHelper:
public static bool IsPlatformAllowed(string allowedPlatforms, ChatbotPlatform platform)
{
if (string.IsNullOrWhiteSpace(allowedPlatforms) || allowedPlatforms.Trim() == "*")
return true;
var platformName = platform.ToString();
foreach (var entry in allowedPlatforms.Split(','))
{
if (string.Equals(entry.Trim(), platformName, StringComparison.OrdinalIgnoreCase))
return true;
}
return false;
}
// Both ChatbotIngressService and ChatbotDepartmentConfigService call ChatbotPlatformHelper.IsPlatformAllowed(...)Prompt for LLM
File Core/Resgrid.Chatbot/Services/ChatbotDepartmentConfigService.cs:
Line 64:
Violates rule 'Extract duplicated logic into functions': the IsPlatformAllowed method (lines 66-79) is a verbatim copy of the identical private static method in ChatbotIngressService.cs (lines 556-569). The comment on line 64 explicitly acknowledges the duplication ('Mirrors ChatbotIngressService.IsPlatformAllowed'). This parsing logic should be extracted to a shared utility or extension method so both services reference a single implementation.
Suggested Code:
// In a shared utility, e.g. ChatbotPlatformHelper:
public static bool IsPlatformAllowed(string allowedPlatforms, ChatbotPlatform platform)
{
if (string.IsNullOrWhiteSpace(allowedPlatforms) || allowedPlatforms.Trim() == "*")
return true;
var platformName = platform.ToString();
foreach (var entry in allowedPlatforms.Split(','))
{
if (string.Equals(entry.Trim(), platformName, StringComparison.OrdinalIgnoreCase))
return true;
}
return false;
}
// Both ChatbotIngressService and ChatbotDepartmentConfigService call ChatbotPlatformHelper.IsPlatformAllowed(...)
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Resgrid-Bot
reviewed
Jul 16, 2026
| if (target == null && int.TryParse(trimmedId, out var deptId)) | ||
| target = supported.FirstOrDefault(m => m.DepartmentId == deptId); | ||
|
|
||
| if (target == null) |
There was a problem hiding this comment.
N+1 database queries occur in BuildSwitchCommandResponseAsync (lines 81-85) and BuildSwitchOptionsMessageAsync (lines 128-132) as both loops repeatedly call GetDepartmentByIdAsync per supported membership. Fetch department data once and reuse it across both loops by adding a batch method to IDepartmentsService or eager-loading the Department navigation property.
Kody rule violation: Detect N+1 style queries and suggest batching
// Pre-load all department names in one batch before the loop to avoid N+1 queries.
var deptIds = supported.Select(m => m.DepartmentId).Distinct().ToList();
var departments = new Dictionary<int, Department>();
foreach (var id in deptIds) // Ideally: a single batch query like GetDepartmentsByIdsAsync(deptIds)
departments[id] = await _departmentsService.GetDepartmentByIdAsync(id, false);
if (target == null)
{
foreach (var membership in supported)
{
if (!departments.TryGetValue(membership.DepartmentId, out var dept) || dept == null)
continue;
if (string.Equals(dept.Name, trimmedId, StringComparison.OrdinalIgnoreCase)
|| string.Equals(dept.Code, trimmedId, StringComparison.OrdinalIgnoreCase))
{
target = membership;
break;
}
if (target == null && !string.IsNullOrWhiteSpace(dept.Name) && dept.Name.IndexOf(trimmedId, StringComparison.OrdinalIgnoreCase) >= 0)
target = membership;
}
}Prompt for LLM
File Core/Resgrid.Services/TextDepartmentSwitchService.cs:
Line 79:
N+1 database queries in BuildSwitchCommandResponseAsync (lines 81-85) and BuildSwitchOptionsMessageAsync (lines 128-132): both loops call GetDepartmentByIdAsync per supported membership, issuing a separate database round-trip for each department. The code comments at lines 84 and 131 explicitly acknowledge this is an N+1 pattern. For a user with N SMS-supported departments, the name/code matching loop fires up to N queries, and the options-list builder fires exactly N queries. Both loops share the same set of department IDs (from GetSmsSupportedMembershipsForUserAsync), so the department data can be fetched once and reused. Consider adding a batch method to IDepartmentsService (e.g., GetDepartmentsByIdsAsync(IEnumerable<int> ids)) and calling it once before both loops, or having GetSmsSupportedMembershipsForUserAsync eager-load the Department navigation property so names are already available.
Suggested Code:
// Pre-load all department names in one batch before the loop to avoid N+1 queries.
var deptIds = supported.Select(m => m.DepartmentId).Distinct().ToList();
var departments = new Dictionary<int, Department>();
foreach (var id in deptIds) // Ideally: a single batch query like GetDepartmentsByIdsAsync(deptIds)
departments[id] = await _departmentsService.GetDepartmentByIdAsync(id, false);
if (target == null)
{
foreach (var membership in supported)
{
if (!departments.TryGetValue(membership.DepartmentId, out var dept) || dept == null)
continue;
if (string.Equals(dept.Name, trimmedId, StringComparison.OrdinalIgnoreCase)
|| string.Equals(dept.Code, trimmedId, StringComparison.OrdinalIgnoreCase))
{
target = membership;
break;
}
if (target == null && !string.IsNullOrWhiteSpace(dept.Name) && dept.Name.IndexOf(trimmedId, StringComparison.OrdinalIgnoreCase) >= 0)
target = membership;
}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Resgrid-Bot
reviewed
Jul 16, 2026
Comment on lines
+110
to
+111
| var identityUser = _usersService.GetUserById(profile.UserId); | ||
| var success = await _departmentsService.SetActiveDepartmentForUserAsync(profile.UserId, target.DepartmentId, identityUser, cancellationToken); |
There was a problem hiding this comment.
Thread blocking occurs in async code because BuildSwitchCommandResponseAsync calls the synchronous _usersService.GetUserById(profile.UserId) at line 110 to perform a database query. Await an async overload of this method or wrap the call in Task.Run before invoking SetActiveDepartmentForUserAsync to avoid blocking the synchronization context.
Kody rule violation: Use Awaitable Methods in Async Code
// If no async overload of GetUserById exists, wrap the synchronous DB call to avoid blocking:
var identityUser = await Task.Run(() => _usersService.GetUserById(profile.UserId));
var success = await _departmentsService.SetActiveDepartmentForUserAsync(profile.UserId, target.DepartmentId, identityUser, cancellationToken);Prompt for LLM
File Core/Resgrid.Services/TextDepartmentSwitchService.cs:
Line 110 to 111:
Violates rule 'Use Awaitable Methods in Async Code': BuildSwitchCommandResponseAsync calls the synchronous _usersService.GetUserById(profile.UserId) at line 110, which performs a database query via _identityRepository.GetUserById(). This blocks the async execution thread. The result is needed before calling SetActiveDepartmentForUserAsync, so the blocking call should be awaited if an async overload exists, or wrapped in Task.Run to avoid blocking the synchronization context.
Suggested Code:
// If no async overload of GetUserById exists, wrap the synchronous DB call to avoid blocking:
var identityUser = await Task.Run(() => _usersService.GetUserById(profile.UserId));
var success = await _departmentsService.SetActiveDepartmentForUserAsync(profile.UserId, target.DepartmentId, identityUser, cancellationToken);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Resgrid-Bot
reviewed
Jul 16, 2026
Comment on lines
+44
to
+52
| var supported = await _departmentsService.GetSmsSupportedMembershipsForUserAsync(profile.UserId); | ||
| if (supported.Count == 0) | ||
| { | ||
| Logging.LogInfo($"{logPrefix} DepartmentId={departmentId} not authorized for inbound text (plan gate); user {profile.UserId} has no SMS-supported departments; replying with unsupported message."); | ||
| return "Resgrid: Inbound text messaging isn't available on your department's current plan. Please upgrade to a paid plan to enable text commands."; | ||
| } | ||
|
|
||
| if (payload.Type == TextCommandTypes.SwitchDepartment) | ||
| return await BuildSwitchCommandResponseAsync(profile, payload.Data, logPrefix, cancellationToken); |
There was a problem hiding this comment.
Redundant database queries execute when BuildUnsupportedActiveDepartmentResponseAsync fetches GetSmsSupportedMembershipsForUserAsync(profile.UserId) at line 44 and BuildSwitchCommandResponseAsync re-fetches the exact same data at line 62. Pass the already-available supported list as a parameter to BuildSwitchCommandResponseAsync to eliminate the second database round-trip.
Kody rule violation: Use computed/derived properties for repeated calculations
var supported = await _departmentsService.GetSmsSupportedMembershipsForUserAsync(profile.UserId);
if (supported.Count == 0)
{
Logging.LogInfo($"{logPrefix} DepartmentId={departmentId} not authorized for inbound text (plan gate); user {profile.UserId} has no SMS-supported departments; replying with unsupported message.");
return "Resgrid: Inbound text messaging isn't available on your department's current plan. Please upgrade to a paid plan to enable text commands.";
}
// Pass the already-fetched list to avoid re-querying inside BuildSwitchCommandResponseAsync.
if (payload.Type == TextCommandTypes.SwitchDepartment)
return await BuildSwitchCommandResponseWithSupportedAsync(profile, payload.Data, logPrefix, supported, cancellationToken);Prompt for LLM
File Core/Resgrid.Services/TextDepartmentSwitchService.cs:
Line 44 to 52:
Redundant database query: BuildUnsupportedActiveDepartmentResponseAsync fetches GetSmsSupportedMembershipsForUserAsync(profile.UserId) at line 44, and when the payload is a SwitchDepartment command (line 51-52), it delegates to BuildSwitchCommandResponseAsync which re-fetches the exact same data at line 62. The supported list is already available and could be passed as a parameter to BuildSwitchCommandResponseAsync (or an overload accepting it) to avoid the second database round-trip within the same request.
Suggested Code:
var supported = await _departmentsService.GetSmsSupportedMembershipsForUserAsync(profile.UserId);
if (supported.Count == 0)
{
Logging.LogInfo($"{logPrefix} DepartmentId={departmentId} not authorized for inbound text (plan gate); user {profile.UserId} has no SMS-supported departments; replying with unsupported message.");
return "Resgrid: Inbound text messaging isn't available on your department's current plan. Please upgrade to a paid plan to enable text commands.";
}
// Pass the already-fetched list to avoid re-querying inside BuildSwitchCommandResponseAsync.
if (payload.Type == TextCommandTypes.SwitchDepartment)
return await BuildSwitchCommandResponseWithSupportedAsync(profile, payload.Data, logPrefix, supported, cancellationToken);
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
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
2 participants
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.
Pull Request Description
This PR introduces several fixes and improvements to the chatbot/SMS text command system, primarily around department switching and unverified mobile number handling.
Key Changes
1. Department Switching via SMS ("SWITCH" command)
SwitchDepartmenttext command type that allows users to change their active department by textingSWITCH <number or name>2. Restricted Mode for Unsupported Departments
3. Unverified Mobile Number Handling
4. No More Silent Department Auto-Picking
GetActiveSmsDepartmentForUserAsyncno longer silently auto-switches users to an SMS-capable department; it respects the user's actual active department and lets downstream logic handle unsupported plans5. Centralized SMS-Supported Department List
GetSmsSupportedMembershipsForUserAsyncmethod to ensure consistent ordering and filtering across all surfaces (chatbot, Twilio, SignalWire) so numeric picks remain stable across stateless SMS requests6. Subscription View Fix
<text>to@:)