Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
This comment has been minimized.
This comment has been minimized.
|
Warning Review limit reached
Next review available in: 38 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSMS department eligibility is now plan-based and shared across chatbot department actions, chatbot ingress, and inbound Twilio message handling. Department selection prioritizes active/default memberships and falls back to inbound-number resolution when needed. ChangesSMS department resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TwilioController
participant UserProfileService
participant DepartmentsService
participant LimitsService
TwilioController->>UserProfileService: GetProfileByMobileNumberAsync(mobileNumber)
TwilioController->>DepartmentsService: GetActiveSmsDepartmentForUserAsync(userId)
DepartmentsService->>LimitsService: CanDepartmentProvisionNumberAsync(departmentId)
LimitsService-->>DepartmentsService: SMS eligibility
DepartmentsService-->>TwilioController: department context
TwilioController->>DepartmentsService: GetDepartmentIdByTextToCallNumberAsync(number)
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🧹 Nitpick comments (1)
Core/Resgrid.Chatbot/Handlers/DepartmentActionHandler.cs (1)
38-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unnecessary fully qualified namespaces.
The
System.Collections.Generic.namespace qualification is likely unnecessary and makes the method signature verbose. Removing it improves readability.♻️ Proposed fix
- private async Task<System.Collections.Generic.List<DepartmentMember>> GetSmsSupportingMembershipsAsync(string userId) + private async Task<List<DepartmentMember>> GetSmsSupportingMembershipsAsync(string userId) { var allMemberRecords = await _departmentsService.GetAllDepartmentsForUserAsync(userId); if (allMemberRecords == null || allMemberRecords.Count == 0) - return new System.Collections.Generic.List<DepartmentMember>(); + return new List<DepartmentMember>(); var candidates = allMemberRecords .Where(m => !m.IsDeleted) .OrderByDescending(m => m.IsActive) .ThenBy(m => m.DepartmentId) .ToList(); - var supported = new System.Collections.Generic.List<DepartmentMember>(); + var supported = new List<DepartmentMember>(); foreach (var membership in candidates) { if (await _limitsService.CanDepartmentProvisionNumberAsync(membership.DepartmentId)) supported.Add(membership); } return 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 `@Core/Resgrid.Chatbot/Handlers/DepartmentActionHandler.cs` around lines 38 - 57, In GetSmsSupportingMembershipsAsync, remove the unnecessary System.Collections.Generic qualification from the List<DepartmentMember> return type and both list constructions, using the existing List symbol/import instead.
🤖 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/Handlers/DepartmentActionHandler.cs`:
- Around line 21-31: Replace constructor injection in DepartmentActionHandler
with explicit Service Locator resolution using
Bootstrapper.GetKernel().Resolve<T>() for IDepartmentsService,
IUserProfileService, IUsersService, and ILimitsService, and update the
constructor accordingly while preserving the existing private field assignments.
In `@Core/Resgrid.Services/DepartmentsService.cs`:
- Around line 440-472: The GetActiveSmsDepartmentForUserAsync contract lacks
cache bypass support and redundantly checks the preferred department. In
Core/Resgrid.Services/DepartmentsService.cs lines 440-472, add bool bypassCache
= false, remove the pre-loop CanDepartmentProvisionNumberAsync check, and pass
bypassCache to every GetDepartmentByIdAsync call; in
Core/Resgrid.Model/Services/IDepartmentsService.cs line 81, update the interface
signature to match.
---
Nitpick comments:
In `@Core/Resgrid.Chatbot/Handlers/DepartmentActionHandler.cs`:
- Around line 38-57: In GetSmsSupportingMembershipsAsync, remove the unnecessary
System.Collections.Generic qualification from the List<DepartmentMember> return
type and both list constructions, using the existing List symbol/import instead.
🪄 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: b8bbaccf-0793-4be0-9e50-56eb543a3c5a
📒 Files selected for processing (7)
Core/Resgrid.Chatbot/Handlers/DepartmentActionHandler.csCore/Resgrid.Chatbot/Services/ChatbotIngressService.csCore/Resgrid.Model/Services/IDepartmentsService.csCore/Resgrid.Services/DepartmentsService.csCore/Resgrid.Services/LimitsService.csCore/Resgrid.Services/SubscriptionsService.csWeb/Resgrid.Web.Services/Controllers/TwilioController.cs
| public DepartmentActionHandler( | ||
| IDepartmentsService departmentsService, | ||
| IUserProfileService userProfileService, | ||
| IUsersService usersService) | ||
| IUsersService usersService, | ||
| ILimitsService limitsService) | ||
| { | ||
| _departmentsService = departmentsService; | ||
| _userProfileService = userProfileService; | ||
| _usersService = usersService; | ||
| _limitsService = limitsService; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Resolve dependencies using the Service Locator pattern.
As per coding guidelines, dependencies should be resolved explicitly in constructors using Bootstrapper.GetKernel().Resolve<T>() rather than via constructor injection.
🛠️ Proposed fix
- public DepartmentActionHandler(
- IDepartmentsService departmentsService,
- IUserProfileService userProfileService,
- IUsersService usersService,
- ILimitsService limitsService)
- {
- _departmentsService = departmentsService;
- _userProfileService = userProfileService;
- _usersService = usersService;
- _limitsService = limitsService;
- }
+ public DepartmentActionHandler()
+ {
+ _departmentsService = Bootstrapper.GetKernel().Resolve<IDepartmentsService>();
+ _userProfileService = Bootstrapper.GetKernel().Resolve<IUserProfileService>();
+ _usersService = Bootstrapper.GetKernel().Resolve<IUsersService>();
+ _limitsService = Bootstrapper.GetKernel().Resolve<ILimitsService>();
+ }📝 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.
| public DepartmentActionHandler( | |
| IDepartmentsService departmentsService, | |
| IUserProfileService userProfileService, | |
| IUsersService usersService) | |
| IUsersService usersService, | |
| ILimitsService limitsService) | |
| { | |
| _departmentsService = departmentsService; | |
| _userProfileService = userProfileService; | |
| _usersService = usersService; | |
| _limitsService = limitsService; | |
| } | |
| public DepartmentActionHandler() | |
| { | |
| _departmentsService = Bootstrapper.GetKernel().Resolve<IDepartmentsService>(); | |
| _userProfileService = Bootstrapper.GetKernel().Resolve<IUserProfileService>(); | |
| _usersService = Bootstrapper.GetKernel().Resolve<IUsersService>(); | |
| _limitsService = Bootstrapper.GetKernel().Resolve<ILimitsService>(); | |
| } |
🤖 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/Handlers/DepartmentActionHandler.cs` around lines 21 -
31, Replace constructor injection in DepartmentActionHandler with explicit
Service Locator resolution using Bootstrapper.GetKernel().Resolve<T>() for
IDepartmentsService, IUserProfileService, IUsersService, and ILimitsService, and
update the constructor accordingly while preserving the existing private field
assignments.
Source: Coding guidelines
| public async Task<Department> GetActiveSmsDepartmentForUserAsync(string userId) | ||
| { | ||
| var allMembers = await GetAllDepartmentsForUserAsync(userId); | ||
| if (allMembers == null || allMembers.Count == 0) | ||
| return null; | ||
|
|
||
| // Prefer the member marked IsActive, then IsDefault, then first. | ||
| var ordered = allMembers | ||
| .Where(m => !m.IsDeleted) | ||
| .OrderByDescending(m => m.IsActive) | ||
| .ThenByDescending(m => m.IsDefault) | ||
| .ToList(); | ||
|
|
||
| if (ordered.Count == 0) | ||
| return null; | ||
|
|
||
| var preferred = ordered[0]; | ||
|
|
||
| // If the preferred department supports SMS (non-free plan) use it; otherwise auto-pick the first | ||
| // of the user's departments that does, so a user whose active department is on the free plan still | ||
| // lands in an SMS-capable department. If none support SMS, fall back to the preferred one so the | ||
| // caller's plan gate returns the proper "plan doesn't support" message. | ||
| if (await _limitsService.CanDepartmentProvisionNumberAsync(preferred.DepartmentId)) | ||
| return await GetDepartmentByIdAsync(preferred.DepartmentId); | ||
|
|
||
| foreach (var membership in ordered) | ||
| { | ||
| if (await _limitsService.CanDepartmentProvisionNumberAsync(membership.DepartmentId)) | ||
| return await GetDepartmentByIdAsync(membership.DepartmentId); | ||
| } | ||
|
|
||
| return await GetDepartmentByIdAsync(preferred.DepartmentId); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add bypassCache parameter and remove redundant SMS capability check.
As per coding guidelines, service methods should include a bypassCache parameter (default: false). Additionally, the current implementation checks the preferred department's SMS capability before the loop, and then redundantly re-checks it during the first iteration of the loop (since preferred is exactly ordered[0]). Removing the pre-check simplifies the logic without changing behavior.
Core/Resgrid.Services/DepartmentsService.cs#L440-L472: Add the parameter, pass it down toGetDepartmentByIdAsync, and remove the redundantCanDepartmentProvisionNumberAsyncpre-check.Core/Resgrid.Model/Services/IDepartmentsService.cs#L81-L81: Update the interface signature to includebool bypassCache = false.
♻️ Proposed fixes
Core/Resgrid.Services/DepartmentsService.cs:
- public async Task<Department> GetActiveSmsDepartmentForUserAsync(string userId)
+ public async Task<Department> GetActiveSmsDepartmentForUserAsync(string userId, bool bypassCache = false)
{
var allMembers = await GetAllDepartmentsForUserAsync(userId);
if (allMembers == null || allMembers.Count == 0)
return null;
// Prefer the member marked IsActive, then IsDefault, then first.
var ordered = allMembers
.Where(m => !m.IsDeleted)
.OrderByDescending(m => m.IsActive)
.ThenByDescending(m => m.IsDefault)
.ToList();
if (ordered.Count == 0)
return null;
var preferred = ordered[0];
// If the preferred department supports SMS (non-free plan) use it; otherwise auto-pick the first
// of the user's departments that does, so a user whose active department is on the free plan still
// lands in an SMS-capable department. If none support SMS, fall back to the preferred one so the
// caller's plan gate returns the proper "plan doesn't support" message.
- if (await _limitsService.CanDepartmentProvisionNumberAsync(preferred.DepartmentId))
- return await GetDepartmentByIdAsync(preferred.DepartmentId);
-
foreach (var membership in ordered)
{
if (await _limitsService.CanDepartmentProvisionNumberAsync(membership.DepartmentId))
- return await GetDepartmentByIdAsync(membership.DepartmentId);
+ return await GetDepartmentByIdAsync(membership.DepartmentId, bypassCache);
}
- return await GetDepartmentByIdAsync(preferred.DepartmentId);
+ return await GetDepartmentByIdAsync(preferred.DepartmentId, bypassCache);
}Core/Resgrid.Model/Services/IDepartmentsService.cs:
- Task<Department> GetActiveSmsDepartmentForUserAsync(string userId);
+ Task<Department> GetActiveSmsDepartmentForUserAsync(string userId, bool bypassCache = false);📝 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.
| public async Task<Department> GetActiveSmsDepartmentForUserAsync(string userId) | |
| { | |
| var allMembers = await GetAllDepartmentsForUserAsync(userId); | |
| if (allMembers == null || allMembers.Count == 0) | |
| return null; | |
| // Prefer the member marked IsActive, then IsDefault, then first. | |
| var ordered = allMembers | |
| .Where(m => !m.IsDeleted) | |
| .OrderByDescending(m => m.IsActive) | |
| .ThenByDescending(m => m.IsDefault) | |
| .ToList(); | |
| if (ordered.Count == 0) | |
| return null; | |
| var preferred = ordered[0]; | |
| // If the preferred department supports SMS (non-free plan) use it; otherwise auto-pick the first | |
| // of the user's departments that does, so a user whose active department is on the free plan still | |
| // lands in an SMS-capable department. If none support SMS, fall back to the preferred one so the | |
| // caller's plan gate returns the proper "plan doesn't support" message. | |
| if (await _limitsService.CanDepartmentProvisionNumberAsync(preferred.DepartmentId)) | |
| return await GetDepartmentByIdAsync(preferred.DepartmentId); | |
| foreach (var membership in ordered) | |
| { | |
| if (await _limitsService.CanDepartmentProvisionNumberAsync(membership.DepartmentId)) | |
| return await GetDepartmentByIdAsync(membership.DepartmentId); | |
| } | |
| return await GetDepartmentByIdAsync(preferred.DepartmentId); | |
| } | |
| public async Task<Department> GetActiveSmsDepartmentForUserAsync(string userId, bool bypassCache = false) | |
| { | |
| var allMembers = await GetAllDepartmentsForUserAsync(userId); | |
| if (allMembers == null || allMembers.Count == 0) | |
| return null; | |
| // Prefer the member marked IsActive, then IsDefault, then first. | |
| var ordered = allMembers | |
| .Where(m => !m.IsDeleted) | |
| .OrderByDescending(m => m.IsActive) | |
| .ThenByDescending(m => m.IsDefault) | |
| .ToList(); | |
| if (ordered.Count == 0) | |
| return null; | |
| var preferred = ordered[0]; | |
| // If the preferred department supports SMS (non-free plan) use it; otherwise auto-pick the first | |
| // of the user's departments that does, so a user whose active department is on the free plan still | |
| // lands in an SMS-capable department. If none support SMS, fall back to the preferred one so the | |
| // caller's plan gate returns the proper "plan doesn't support" message. | |
| foreach (var membership in ordered) | |
| { | |
| if (await _limitsService.CanDepartmentProvisionNumberAsync(membership.DepartmentId)) | |
| return await GetDepartmentByIdAsync(membership.DepartmentId, bypassCache); | |
| } | |
| return await GetDepartmentByIdAsync(preferred.DepartmentId, bypassCache); | |
| } |
📍 Affects 2 files
Core/Resgrid.Services/DepartmentsService.cs#L440-L472(this comment)Core/Resgrid.Model/Services/IDepartmentsService.cs#L81-L81
🤖 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 440 - 472, The
GetActiveSmsDepartmentForUserAsync contract lacks cache bypass support and
redundantly checks the preferred department. In
Core/Resgrid.Services/DepartmentsService.cs lines 440-472, add bool bypassCache
= false, remove the pre-loop CanDepartmentProvisionNumberAsync check, and pass
bypassCache to every GetDepartmentByIdAsync call; in
Core/Resgrid.Model/Services/IDepartmentsService.cs line 81, update the interface
signature to match.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.Model/Services/IDepartmentsService.cs`:
- Line 81: Update DepartmentsService.GetActiveSmsDepartmentForUserAsync to
forward the bypassCache argument when calling
GetAllDepartmentsForUserAsync(userId). Ensure fresh resolution uses bypassCache:
true while preserving cached behavior when it is false.
🪄 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: c661cebc-d87a-4f91-aa44-20e84c32431f
📒 Files selected for processing (2)
Core/Resgrid.Model/Services/IDepartmentsService.csCore/Resgrid.Services/DepartmentsService.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/Resgrid.Services/DepartmentsService.cs
| /// downstream plan gate can produce the proper "plan doesn't support" message. Null when the user has | ||
| /// no non-deleted memberships. | ||
| /// </summary> | ||
| Task<Department> GetActiveSmsDepartmentForUserAsync(string userId, bool bypassCache = false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate bypassCache through the resolver implementation.
This contract exposes bypassCache, but DepartmentsService.GetActiveSmsDepartmentForUserAsync still calls GetAllDepartmentsForUserAsync(userId) without forwarding it. As a result, callers requesting fresh SMS department resolution can receive stale membership/active-department data from cache.
Update the implementation to honor bypassCache when loading the user’s memberships. As per coding guidelines, fresh data must use bypassCache: true or cache invalidation.
🤖 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` at line 81, Update
DepartmentsService.GetActiveSmsDepartmentForUserAsync to forward the bypassCache
argument when calling GetAllDepartmentsForUserAsync(userId). Ensure fresh
resolution uses bypassCache: true while preserving cached behavior when it is
false.
Source: Coding guidelines
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
2 participants
Pull Request Description
Title: RG-T125 Twilio fixes
Summary
This PR fixes several issues in the Twilio SMS integration by correcting how SMS plan eligibility is determined, centralizing department resolution for SMS operations, and supporting the new master-number inbound model.
Key Changes
Fixed SMS plan eligibility check: The
CanDepartmentProvisionNumberAsyncmethod previously relied on a hardcoded allow-list of specific plan IDs to determine SMS/text feature support. This was replaced with a dynamic check (IsPlanRestrictedOrFree) so that any non-free plan automatically qualifies, eliminating the need to manually add new plan IDs. TheIsPlanRestrictedOrFreemethod, which previously always returnedfalse, was implemented to correctly identify free/restricted tiers (Free, Beta 2yr, Open Preview, Unlimited Free).Centralized SMS department resolution: A new shared method (
GetActiveSmsDepartmentForUserAsync) resolves the department a user should operate in for SMS/chatbot operations. It prefers the user's active (then default) department, but automatically falls back to another SMS-capable department if the preferred one is on a free plan. Both the chatbot ingress service and the Twilio controller now use this single resolver, ensuring they always agree on which department context applies.Filtered chatbot department lists to SMS-capable departments only: The chatbot's "List Departments" and "Switch Department" actions now only display and allow switching to departments whose plan supports SMS. This prevents users from selecting a free-plan department and getting dead-ended, and ensures that numeric selections map correctly to the displayed list.
Supported the master-number inbound model: The Twilio
IncomingMessagehandler now resolves the department from the sender's user profile (their active SMS-capable department) as the primary path—supporting the going-forward model where one master Resgrid number handles all inbound SMS. The legacy fallback (resolving department from a per-department provisioned inbound number) remains for backward compatibility.Summary by CodeRabbit
New Features
Bug Fixes