Develop #240
Develop #240
Conversation
WalkthroughAdds system location data and a v4 GetSystemConfig endpoint. Extends communication APIs to accept a Department and threads DepartmentCode into push payloads. Adjusts Novu integration (workflow IDs, conditional sends, payload id). Adds error handling to an email-to-call template. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Worker
participant BroadcastLogic as BroadcastMessageLogic
participant Comm as CommunicationService
participant Push as PushService
participant Novu as NovuProvider
Worker->>BroadcastLogic: prepare message/calendar
BroadcastLogic->>Comm: SendMessageAsync(..., profile, department) / SendCalendarAsync(..., profile, department)
note right of Comm: sets message.DepartmentCode (null for calendar)
Comm->>Push: PushMessage / PushNotification
alt DepartmentCode present
Push->>Novu: SendUserMessage(...) (user workflow)
else DepartmentCode blank
Push->>Novu: SendUserNotification(...) (notification workflow)
end
Novu-->>Push: ack/result
Push-->>Comm: result
Comm-->>BroadcastLogic: result
sequenceDiagram
autonumber
actor Client
participant API as ConfigController (v4)
participant Info as InfoConfig
Client->>API: GET /v4/Config/GetSystemConfig
API->>Info: read Locations
Info-->>API: List<ResgridSystemLocation>
API-->>Client: 200 OK (GetSystemConfigResult { Data.Locations })
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (2)
218-236: Respect enableCustomSounds and handle null type to avoid "null.wav" on iOSCurrently type can be null (messages/notifications), producing "null.wav". Also enableCustomSounds is ignored. Use "default" when custom sounds are disabled or type is empty.
Apply this diff:
using (var httpClient = new HttpClient()) { // Set base URL and headers httpClient.BaseAddress = new Uri(ChatConfig.NovuBackendUrl); httpClient.DefaultRequestHeaders.Add("Authorization", $"ApiKey {ChatConfig.NovuSecretKey}"); httpClient.DefaultRequestHeaders.Add("idempotency-key", Guid.NewGuid().ToString()); string channelName = GetAndroidChannelName(eventCode); + var apnsSoundName = (!enableCustomSounds || string.IsNullOrWhiteSpace(type)) + ? "default" + : GetSoundFileNameFromType(Platforms.iOS, type); // Build request payload var payload = new { name = workflowIdentifier, payload = new { subject = title, body = body, id = eventCode }, overrides = new { fcm = new { android = new { priority = channelName == "calls" ? "high" : "normal", notification = new { channelId = type, defaultSound = true, sticky = channelName == "calls" ? true : false, //priority = androidChannelName == "calls" ? 5 : 3, notification_priority = channelName == "calls" ? "PRIORITY_MAX" : "PRIORITY_DEFAULT", }, data = new { title = title, message = body, eventCode = eventCode, type = type } }, }, apns = new { badge = count, sound = new { - name = GetSoundFileNameFromType(Platforms.iOS, type), + name = apnsSoundName, critical = channelName == "calls" ? 1 : 0, volume = 1.0f }, type = type, category = channelName, eventCode = eventCode, }, },Also applies to: 262-268
321-351: Guard against null/empty type in sound mappingReturn a safe default when type is missing to avoid invalid filenames.
private string GetSoundFileNameFromType(Platforms platform, string type) { + if (string.IsNullOrWhiteSpace(type)) + return "default"; if (type == ((int)PushSoundTypes.CallEmergency).ToString()) { return "callemergency.wav";
🧹 Nitpick comments (19)
Core/Resgrid.Config/SystemBehaviorConfig.cs (1)
188-192: Make LocationName immutable to prevent accidental runtime changes; align with InfoConfig default.Expose as a get-only static to reflect a configuration constant. This reduces mutation risk and matches the usage pattern of other config constants.
- public static string LocationName = "US-West"; + public static string LocationName { get; } = "US-West";Optionally, add a startup validation ensuring this value matches one of InfoConfig.Locations to avoid drift.
Web/Resgrid.Web.Services/Controllers/v4/ConfigController.cs (1)
27-42: Remove unnecessary async; no awaits present.Avoid CS1998 and minor overhead by returning synchronously.
- public async Task<ActionResult<GetSystemConfigResult>> GetSystemConfig() + public ActionResult<GetSystemConfigResult> GetSystemConfig() { var result = new GetSystemConfigResult(); result.PageSize = 1; result.Status = ResponseHelper.Success; ResponseHelper.PopulateV4ResponseData(result); return result; }Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (2)
5554-5558: Be explicit about Data payload type.Using the concrete payload type helps generated docs and external consumers.
Apply this diff:
- <member name="P:Resgrid.Web.Services.Models.v4.Configs.GetSystemConfigResult.Data"> - <summary> - Response Data - </summary> - </member> + <member name="P:Resgrid.Web.Services.Models.v4.Configs.GetSystemConfigResult.Data"> + <summary> + Payload of type GetSystemConfigResultData. + </summary> + </member>
3352-3357: Clarify and Specify Return Type for GetSystemConfigThe XML docs for
ConfigController.GetSystemConfigshould explicitly state the concrete return type and include the product name for consistency.• File: Web/Resgrid.Web.Services/Resgrid.Web.Services.xml
• Lines: 3352–3357Suggested diff:
- <member name="M:Resgrid.Web.Services.Controllers.v4.ConfigController.GetSystemConfig"> - <summary> - Gets the system config - </summary> - <returns></returns> + <member name="M:Resgrid.Web.Services.Controllers.v4.ConfigController.GetSystemConfig"> + <summary> + Gets the Resgrid system configuration. + </summary> + <returns>ActionResult<GetSystemConfigResult>.</returns> </member>Web/Resgrid.Web.Services/Models/v4/Configs/GetSystemConfigResult.cs (3)
30-34: Expose a read-only collection in the API model.Make the DTO immutable to callers to prevent accidental mutation post-serialization and align with the guidelines favoring immutable data in C#.
using System.Collections.Generic; +using System.Collections.ObjectModel; @@ - public List<ResgridSystemLocation> Locations { get; set; } + public IReadOnlyList<ResgridSystemLocation> Locations { get; private set; } @@ - Locations = InfoConfig.Locations + Locations = InfoConfig.Locations .Select(l => new ResgridSystemLocation { Name = l.Name, DisplayName = l.DisplayName, LocationInfo = l.LocationInfo, IsDefault = l.IsDefault, ApiUrl = l.ApiUrl, AllowsFreeAccounts = l.AllowsFreeAccounts }) - .ToList(); + .ToList() + .AsReadOnly();Also applies to: 35-38, 1-1
29-39: Decouple DTO from config layer; pass data in from the controller.Keeping models pure (no static dependencies) improves testability and separation of concerns.
- public class GetSystemConfigResultData + public class GetSystemConfigResultData { @@ - public GetSystemConfigResultData() - { - Locations = InfoConfig.Locations; - } + public GetSystemConfigResultData(IEnumerable<ResgridSystemLocation> locations) + { + Locations = locations + .Select(l => new ResgridSystemLocation + { + Name = l.Name, + DisplayName = l.DisplayName, + LocationInfo = l.LocationInfo, + IsDefault = l.IsDefault, + ApiUrl = l.ApiUrl, + AllowsFreeAccounts = l.AllowsFreeAccounts + }) + .ToList() + .AsReadOnly(); + }Controller sketch (outside this file):
return new GetSystemConfigResult { Data = new GetSystemConfigResultData(InfoConfig.Locations) };
30-38: Include a config version to help clients cache/invalidate.Expose
InfoConfig.ConfigVersionso clients can detect changes without diffing payloads.public class GetSystemConfigResultData { /// <summary> /// Resgrid Datacenter Locations /// </summary> public IReadOnlyList<ResgridSystemLocation> Locations { get; private set; } + + /// <summary> + /// Server-side config version for client caching/invalidation + /// </summary> + public int ConfigVersion { get; private set; } @@ - public GetSystemConfigResultData(IEnumerable<ResgridSystemLocation> locations) + public GetSystemConfigResultData(IEnumerable<ResgridSystemLocation> locations) { Locations = locations .Select(l => new ResgridSystemLocation { Name = l.Name, DisplayName = l.DisplayName, LocationInfo = l.LocationInfo, IsDefault = l.IsDefault, ApiUrl = l.ApiUrl, AllowsFreeAccounts = l.AllowsFreeAccounts }) .ToList() .AsReadOnly(); + ConfigVersion = InfoConfig.ConfigVersion; }Core/Resgrid.Config/ChatConfig.cs (1)
13-16: Externalize provider integration identifiers; avoid hardcoding env-specific IDsHardcoding Novu provider IDs couples code to a specific environment and complicates multi-env deployments. Prefer reading these from configuration (app settings, env vars) and mark as readonly to prevent runtime mutation.
Would you like a follow-up patch to wire these via IOptions and per-environment settings?
Core/Resgrid.Services/CallEmailTemplates/OttawaKingstonTorontoTemplate.cs (2)
54-69: Avoid exception-driven parsing; add bounds checks on sectionsAccessing sections[1] and sectionOneParts[0] without validation relies on exceptions for control flow. Validate lengths and fall back to email.Subject/body when missing to keep the happy path exception-free.
81-93: DRY: factor “attach-to-active-call” logic into a helperThe active-call merge appears in both try and catch paths. Extract into a small local function to reduce duplication and keep behavior consistent.
Core/Resgrid.Services/PushService.cs (1)
115-116: Consider passing explicit type for message pushesYou already guard DepartmentCode—good. To avoid provider falling back to "default" sounds, pass ((int)PushSoundTypes.Message).ToString() instead of null.
- await _novuProvider.SendUserMessage(message.Title, message.SubTitle, userId, message.DepartmentCode, string.Format("M{0}", message.MessageId), null); + await _novuProvider.SendUserMessage(message.Title, message.SubTitle, userId, message.DepartmentCode, string.Format("M{0}", message.MessageId), ((int)PushSoundTypes.Message).ToString());Core/Resgrid.Model/Services/ICommunicationService.cs (3)
21-23: Department parameter added: good; please document the new argumentThe optional Department context is sensible for push scoping. Add XML docs for clarity.
/// <param name="profile">The profile.</param> +/// <param name="department">Optional department context for push routing (maps to DepartmentCode); may be null.</param> /// <returns>Task<System.Boolean>.</returns> Task<bool> SendMessageAsync(Message message, string sendersName, string departmentNumber, int departmentId, UserProfile profile = null, Department department = null);
112-114: Calendar overload gains Department param: unused in impl—document behaviorImpl intentionally ignores department for calendar pushes. Please note that in XML docs to avoid confusion.
/// <param name="profile">The profile.</param> +/// <param name="department">Ignored for calendar push (DepartmentCode is not used).</param> /// <returns>Task<System.Boolean>.</returns> Task<bool> SendCalendarAsync(string userId, int departmentId, string message, string departmentNumber, string title = "Notification", UserProfile profile = null, Department department = null);
58-60: Signature consistency nitUnlike SendMessageAsync/SendCalendarAsync, SendNotificationAsync places Department before title and is non-optional. Consider aligning parameter order and defaulting to null for consistency (not blocking).
Workers/Resgrid.Workers.Framework/Logic/BroadcastMessageLogic.cs (3)
78-79: Unhandled path: add logging or fallbackThis TODO should at least log a warning with message/messageId and departmentId to aid ops.
- //TODO: What to do here, I don't know why this path is empty. -SJ + // Sender profile missing; log and skip to avoid silent drop. + Logging.LogWarning($"BroadcastMessageLogic: sending profile not found. MessageId={mqi.Message?.MessageId}, DeptId={mqi.DepartmentId}, ReceivingUserId={mqi.Message?.ReceivingUserId}");
84-86: Duplicate department fetchDepartment is fetched again; reuse the earlier instance to reduce I/O.
- var departmentService = Bootstrapper.GetKernel().Resolve<IDepartmentsService>(); - var department = await departmentService.GetDepartmentByIdAsync(mqi.DepartmentId); + // Reuse department fetched earlier in method (move that fetch above both branches).
63-65: Consolidate and null‐guard department lookup
- Duplicate resolution of
IDepartmentsServiceat lines 63–65 and 84–86 inBroadcastMessageLogic.cs.- Move both
Resolve<IDepartmentsService>()and the subsequentGetDepartmentByIdAsynccall out of the individual recipient branches and into a single lookup before they run.- Add a null‐check on
departmentto log or skip sending if the department isn’t found.Proposed diff:
@@ public static async Task<bool> ProcessMessageQueueItem(MessageQueueItem mqi) - if (mqi.Message.ReceivingUserId != null && (mqi.Message.Recipients == null || !mqi.Message.Recipients.Any())) + // Resolve department once for all send paths + var departmentService = Bootstrapper.GetKernel().Resolve<IDepartmentsService>(); + var department = await departmentService.GetDepartmentByIdAsync(mqi.DepartmentId); + if (department == null) + { + // TODO: log warning or handle missing department + } + + if (mqi.Message.ReceivingUserId != null && (mqi.Message.Recipients == null || !mqi.Message.Recipients.Any())) { - var sendingToProfile = mqi.Profiles.FirstOrDefault(x => x.UserId == mqi.Message.ReceivingUserId); - var departmentService = Bootstrapper.GetKernel().Resolve<IDepartmentsService>(); - var department = await departmentService.GetDepartmentByIdAsync(mqi.DepartmentId); + var sendingToProfile = mqi.Profiles.FirstOrDefault(x => x.UserId == mqi.Message.ReceivingUserId); if (sendingToProfile != null) - await _communicationService.SendMessageAsync(mqi.Message, name, mqi.DepartmentTextNumber, mqi.DepartmentId, sendingToProfile, department); + await _communicationService.SendMessageAsync(mqi.Message, name, mqi.DepartmentTextNumber, mqi.DepartmentId, sendingToProfile, department); } else if (mqi.Message.MessageRecipients != null && mqi.Message.MessageRecipients.Any()) { - var departmentService = Bootstrapper.GetKernel().Resolve<IDepartmentsService>(); - var department = await departmentService.GetDepartmentByIdAsync(mqi.DepartmentId); + // reuse department and service resolved above foreach (var recipient in mqi.Message.MessageRecipients) { var sendingToProfile = mqi.Profiles.FirstOrDefault(x => x.UserId == recipient.UserId); mqi.Message.ReceivingUserId = recipient.UserId; if (sendingToProfile != null) - await _communicationService.SendMessageAsync(mqi.Message, name, mqi.DepartmentTextNumber, mqi.DepartmentId, sendingToProfile, department); + await _communicationService.SendMessageAsync(mqi.Message, name, mqi.DepartmentTextNumber, mqi.DepartmentId, sendingToProfile, department); } }Core/Resgrid.Services/CommunicationService.cs (2)
83-83: Push scoping via DepartmentCode: OK; consider fallbackSetting DepartmentCode from department?.Code is correct. If department is null, consider optional fallback (e.g., lazily resolve by departmentId) to avoid lost notifications, if product requirements allow.
340-341: Calendar overload adds Department param but doesn’t use itParameter is unused by design; document this in XML comments to prevent misuse, or remove the parameter if signature uniformity isn’t required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
Core/Resgrid.Config/ChatConfig.cs(1 hunks)Core/Resgrid.Config/InfoConfig.cs(2 hunks)Core/Resgrid.Config/SystemBehaviorConfig.cs(1 hunks)Core/Resgrid.Model/Services/ICommunicationService.cs(2 hunks)Core/Resgrid.Services/CalendarService.cs(2 hunks)Core/Resgrid.Services/CallEmailTemplates/OttawaKingstonTorontoTemplate.cs(1 hunks)Core/Resgrid.Services/CommunicationService.cs(6 hunks)Core/Resgrid.Services/PushService.cs(2 hunks)Providers/Resgrid.Providers.Messaging/NovuProvider.cs(1 hunks)Web/Resgrid.Web.Services/Controllers/v4/ConfigController.cs(1 hunks)Web/Resgrid.Web.Services/Models/v4/Configs/GetSystemConfigResult.cs(1 hunks)Web/Resgrid.Web.Services/Resgrid.Web.Services.xml(2 hunks)Workers/Resgrid.Workers.Framework/Logic/BroadcastMessageLogic.cs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Prefer functional patterns and immutable data where appropriate in C#
Use modern C# features appropriately
Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations
Separate state from behavior (e.g., use records for state and place operations in separate static classes)
Prefer pure methods over methods with side effects
Use extension methods appropriately for domain-specific operations
Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions
Minimize constructor injection; keep the number of injected dependencies small
Prefer composition with interfaces to extend behavior
Files:
Providers/Resgrid.Providers.Messaging/NovuProvider.csCore/Resgrid.Config/SystemBehaviorConfig.csWeb/Resgrid.Web.Services/Controllers/v4/ConfigController.csWeb/Resgrid.Web.Services/Models/v4/Configs/GetSystemConfigResult.csCore/Resgrid.Services/PushService.csCore/Resgrid.Model/Services/ICommunicationService.csCore/Resgrid.Config/InfoConfig.csCore/Resgrid.Services/CalendarService.csCore/Resgrid.Config/ChatConfig.csWorkers/Resgrid.Workers.Framework/Logic/BroadcastMessageLogic.csCore/Resgrid.Services/CallEmailTemplates/OttawaKingstonTorontoTemplate.csCore/Resgrid.Services/CommunicationService.cs
🧬 Code graph analysis (5)
Web/Resgrid.Web.Services/Controllers/v4/ConfigController.cs (1)
Web/Resgrid.Web.Services/Models/v4/Configs/GetSystemConfigResult.cs (2)
GetSystemConfigResult(9-23)GetSystemConfigResult(19-22)
Web/Resgrid.Web.Services/Models/v4/Configs/GetSystemConfigResult.cs (1)
Core/Resgrid.Config/InfoConfig.cs (2)
ResgridSystemLocation(52-60)InfoConfig(5-50)
Workers/Resgrid.Workers.Framework/Logic/BroadcastMessageLogic.cs (1)
Workers/Resgrid.Workers.Framework/Bootstrapper.cs (1)
Bootstrapper(23-67)
Core/Resgrid.Services/CallEmailTemplates/OttawaKingstonTorontoTemplate.cs (1)
Workers/Resgrid.Workers.Framework/Logic/CallEmailImporterLogic.cs (1)
Call(137-192)
Core/Resgrid.Services/CommunicationService.cs (1)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (15)
Task(17-64)Task(66-81)Task(83-97)Task(99-133)Task(135-169)Task(171-191)Task(193-196)Task(198-201)Task(203-206)Task(208-211)Task(213-297)Task(299-302)Task(304-307)Task(309-312)Task(314-317)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (csharp)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (13)
Web/Resgrid.Web.Services/Controllers/v4/ConfigController.cs (1)
27-42: LGTM: GetSystemConfigResultData wiring confirmedConfirmed that GetSystemConfigResultData’s constructor sets
Locations = InfoConfig.Locationsin GetSystemConfigResult.cs (lines 28–39). No further changes needed—approving these code updates.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (2)
5549-5563: LGTM: New GetSystemConfigResult type added.Type and ctor docs look fine and align with the new endpoint.
5564-5573: Clarify Locations element type and nullability in XML docsThe
GetSystemConfigResultDataconstructor assigns
Locations = InfoConfig.Locations, andInfoConfig.Locationsis initialized as a non-null
List<ResgridSystemLocation>. Update the XML docs to reflect that.• File: Web/Resgrid.Web.Services/Resgrid.Web.Services.xml
Member:P:Resgrid.Web.Services.Models.v4.Configs.GetSystemConfigResultData.LocationsApply this diff:
- <member name="P:Resgrid.Web.Services.Models.v4.Configs.GetSystemConfigResultData.Locations"> - <summary> - Resgrid Datacenter Locations - </summary> - </member> + <member name="P:Resgrid.Web.Services.Models.v4.Configs.GetSystemConfigResultData.Locations"> + <summary> + List of datacenter locations (Resgrid.Config.ResgridSystemLocation). Never null. + </summary> + </member>Web/Resgrid.Web.Services/Models/v4/Configs/GetSystemConfigResult.cs (1)
9-22: No generic response base found—non-generic pattern is consistentI’ve confirmed there is no
StandardApiResponseV4Base<T>defined or in use anywhere in the v4 models, and all other*Resulttypes inherit from the non-genericStandardApiResponseV4Base. Retaining the existing implementation in GetSystemConfigResult is correct.Core/Resgrid.Config/ChatConfig.cs (1)
20-20: Verify Novu workflow identifier exists and matches codePlease manually confirm the following before merging:
- Ensure that in your Novu dashboard the workflow with identifier
"user-notification"has been created and is currently active.- Confirm that the Novu events/trigger API uses the workflow’s
namefield to select which workflow to execute—that it expects exactly"user-notification".- Update any environment-specific configuration or documentation to reflect this exact identifier so SendUserNotification points at the correct Novu workflow.
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (1)
234-235: ‘payload.id’ is Safe to Use in Novu v1 Events/TriggerAccording to Novu’s v1
/events/triggerAPI documentation, thepayloadobject is entirely free-form and intended to carry any custom key-value pairs for rendering workflows or routing rules. There are no reserved keys insidepayload—the only special identifiers are top-level fields such asname,to,transactionId,actor, andtenant. Consequently, including anidproperty (e.g.id = eventCode) withinpayloadwill not conflict with Novu’s internal fields or message identity (docs.novu.co).Core/Resgrid.Services/CalendarService.cs (1)
472-472: LGTM: Department context propagation aligns with the new signaturePassing department to SendCalendarAsync matches the updated contract and keeps downstream routing consistent.
Also applies to: 491-491, 493-493
Workers/Resgrid.Workers.Framework/Logic/BroadcastMessageLogic.cs (2)
68-69: Passing Department through: LGTMCorrectly threads Department to align with ICommunicationService changes.
94-95: Per-recipient send: OKMatches updated interface; acceptable to keep sequential sends here.
Core/Resgrid.Services/CommunicationService.cs (4)
42-42: Interface parity maintainedSignature updated to include optional Department—good for backward compatibility.
129-129: Include DepartmentCode on calls: LGTMAligns call push payloads with department-aware routing.
369-369: Explicitly nulling DepartmentCode for calendar: LGTMClear intent that calendar pushes are not department-scoped.
474-474: Trouble alert DepartmentCode: LGTMCorrectly threads DepartmentCode through StandardPushCall.
| public static List<ResgridSystemLocation> Locations = new List<ResgridSystemLocation>() | ||
| { | ||
| new ResgridSystemLocation() | ||
| { | ||
| Name = "US-West", | ||
| DisplayName = "Resgrid North America (Global)", | ||
| LocationInfo = | ||
| "This is the Resgrid system hosted in the Western United States (private datacenter). This system services most Resgrid customers.", | ||
| IsDefault = true, | ||
| ApiUrl = "https://api.resgrid.com", | ||
| AllowsFreeAccounts = true | ||
| }, | ||
| new ResgridSystemLocation() | ||
| { | ||
| Name = "EU-Central", | ||
| DisplayName = "Resgrid Europe", | ||
| LocationInfo = | ||
| "This is the Resgrid system hosted in Central Europe (on OVH). This system services Resgrid customers in the European Union to help with data compliance requirements.", | ||
| IsDefault = false, | ||
| ApiUrl = "https://api.eu.resgrid.com", | ||
| AllowsFreeAccounts = false | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose Locations as read-only to avoid global mutation; initialize immutably.
The config list is public and mutable, which is risky. Prefer IReadOnlyList and immutable initialization.
- public static List<ResgridSystemLocation> Locations = new List<ResgridSystemLocation>()
- {
- new ResgridSystemLocation()
- {
- Name = "US-West",
- DisplayName = "Resgrid North America (Global)",
- LocationInfo =
- "This is the Resgrid system hosted in the Western United States (private datacenter). This system services most Resgrid customers.",
- IsDefault = true,
- ApiUrl = "https://api.resgrid.com",
- AllowsFreeAccounts = true
- },
- new ResgridSystemLocation()
- {
- Name = "EU-Central",
- DisplayName = "Resgrid Europe",
- LocationInfo =
- "This is the Resgrid system hosted in Central Europe (on OVH). This system services Resgrid customers in the European Union to help with data compliance requirements.",
- IsDefault = false,
- ApiUrl = "https://api.eu.resgrid.com",
- AllowsFreeAccounts = false
- }
- };
+ public static IReadOnlyList<ResgridSystemLocation> Locations { get; } = new[]
+ {
+ new ResgridSystemLocation(
+ Name: "US-West",
+ DisplayName: "Resgrid North America (Global)",
+ LocationInfo: "This is the Resgrid system hosted in the Western United States (private datacenter). This system services most Resgrid customers.",
+ IsDefault: true,
+ ApiUrl: "https://api.resgrid.com",
+ AllowsFreeAccounts: true
+ ),
+ new ResgridSystemLocation(
+ Name: "EU-Central",
+ DisplayName: "Resgrid Europe",
+ LocationInfo: "This is the Resgrid system hosted in Central Europe (on OVH). This system services Resgrid customers in the European Union to help with data compliance requirements.",
+ IsDefault: false,
+ ApiUrl: "https://api.eu.resgrid.com",
+ AllowsFreeAccounts: false
+ )
+ };Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Core/Resgrid.Config/InfoConfig.cs around lines 27 to 49, the public mutable
List<ResgridSystemLocation> Locations allows global mutation; change the API to
expose a read-only collection and initialize it immutably. Replace the public
mutable field with a private static readonly List<ResgridSystemLocation> backing
list populated with the two locations and expose it via a public static
IReadOnlyList<ResgridSystemLocation> (or ReadOnlyCollection/ImmutableList)
property that returns the immutable/read-only view so callers cannot add/remove
items.
| { | ||
| public string Name { get; set; } | ||
| public string DisplayName { get; set; } | ||
| public string LocationInfo { get; set; } | ||
| public bool IsDefault { get; set; } | ||
| public string ApiUrl { get; set; } | ||
| public bool AllowsFreeAccounts { get; set; } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a record with init-only semantics for config state.
Records with positional parameters make the configuration immutable and concise, matching the guideline to separate state from behavior.
- public class ResgridSystemLocation
- {
- public string Name { get; set; }
- public string DisplayName { get; set; }
- public string LocationInfo { get; set; }
- public bool IsDefault { get; set; }
- public string ApiUrl { get; set; }
- public bool AllowsFreeAccounts { get; set; }
- }
+ public sealed record ResgridSystemLocation(
+ string Name,
+ string DisplayName,
+ string LocationInfo,
+ bool IsDefault,
+ string ApiUrl,
+ bool AllowsFreeAccounts
+ );If other code uses object initializers, convert those call sites accordingly. Also consider adding a helper to fetch the default location:
public static ResgridSystemLocation DefaultLocation => Locations.First(l => l.IsDefault);🤖 Prompt for AI Agents
In Core/Resgrid.Config/InfoConfig.cs around lines 52 to 59, the mutable class
ResgridSystemLocation should be converted to an immutable record with init-only
semantics (preferably a positional record) so configuration state is represented
immutably; replace the class declaration with a record declaration, update
properties to be init-only (or use positional parameters), and adjust any call
sites that use object initializers to use the new positional/with syntax;
optionally add a static helper property (e.g., DefaultLocation) that returns
Locations.First(l => l.IsDefault) to fetch the configured default location.
| Call c = new Call(); | ||
| c.Name = email.Subject; | ||
| c.NatureOfCall = $"ERROR PROCESSING DISPATCH EMAIL, Unprocessed email body: {email.TextBody}"; | ||
|
|
||
| if (activeCall != null) | ||
| if (users != null && users.Any()) | ||
| { | ||
| activeCall.Notes = c.Notes; | ||
| activeCall.LastDispatchedOn = DateTime.UtcNow; | ||
| foreach (var u in users) | ||
| { | ||
| CallDispatch cd = new CallDispatch(); | ||
| cd.UserId = u.UserId; | ||
|
|
||
| return activeCall; | ||
| c.Dispatches.Add(cd); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix NullReference and complete fallback Call construction in catch block
c.Dispatches isn't initialized before Add, causing a NullReferenceException inside catch. Also key fields (Notes, LoggedOn, Priority, ReportingUserId, CallSource, SourceIdentifier) aren't set, and active-call lookup uses a null IncidentNumber.
Apply this diff:
- catch (Exception ex)
+ catch (Exception ex)
{
- Call c = new Call();
- c.Name = email.Subject;
- c.NatureOfCall = $"ERROR PROCESSING DISPATCH EMAIL, Unprocessed email body: {email.TextBody}";
+ Call c = new Call();
+ c.Name = email.Subject ?? "Email-to-Call Error";
+ c.Notes = email.TextBody;
+ c.NatureOfCall = $"ERROR PROCESSING DISPATCH EMAIL, Unprocessed email body: {email.TextBody}";
+ c.LoggedOn = DateTime.UtcNow;
+ c.Priority = priority;
+ c.ReportingUserId = managingUser;
+ c.Dispatches = new Collection<CallDispatch>();
+ c.CallSource = (int)CallSources.EmailImport;
+ c.SourceIdentifier = email.MessageId;
if (users != null && users.Any())
{
foreach (var u in users)
{
CallDispatch cd = new CallDispatch();
cd.UserId = u.UserId;
c.Dispatches.Add(cd);
}
}
- // Search for an active call
- if (activeCalls != null && activeCalls.Any())
+ // Search for an active call only if we have an incident number
+ if (!string.IsNullOrWhiteSpace(c.IncidentNumber) && activeCalls != null && activeCalls.Any())
{
var activeCall = activeCalls.FirstOrDefault(x => x.IncidentNumber == c.IncidentNumber);
if (activeCall != null)
{
activeCall.Notes = c.Notes;
activeCall.LastDispatchedOn = DateTime.UtcNow;
return activeCall;
}
}
return c;
}Also applies to: 114-129
🤖 Prompt for AI Agents
In Core/Resgrid.Services/CallEmailTemplates/OttawaKingstonTorontoTemplate.cs
around lines 99-112 (and similarly for 114-129), the fallback Call created in
the catch block leaves Call.Dispatches null and omits key properties causing
NREs and incorrect active-call lookup. Initialize c.Dispatches to an empty
collection before adding CallDispatch items; set the missing properties (c.Notes
= email.TextBody or appropriate message, c.LoggedOn = DateTime.UtcNow,
c.Priority = default priority, c.ReportingUserId = a valid reporter id or null
handling, c.CallSource = CallSource.Email (or appropriate enum),
c.SourceIdentifier = email.MessageId or a unique identifier) and ensure
IncidentNumber is set to a non-null value (e.g., generate or use
email.MessageId) before doing the active-call lookup; also keep the users
null/empty check and only add dispatches when users exist.
Core/Resgrid.Services/PushService.cs
Outdated
| try | ||
| { | ||
| await _novuProvider.SendUserMessage(message.Title, message.SubTitle, userId, message.DepartmentCode, string.Format("N{0}", message.MessageId), null); | ||
| await _novuProvider.SendUserNotification(message.Title, message.SubTitle, userId, message.DepartmentCode, string.Format("N{0}", message.MessageId), null); |
There was a problem hiding this comment.
Guard DepartmentCode before sending; prevent malformed subscriber IDs
Without a non-empty DepartmentCode, recipientId becomes "User{userId}", which won't match a subscriber. Mirror the message path guard and, optionally, pass a concrete notification type.
- await _novuProvider.SendUserNotification(message.Title, message.SubTitle, userId, message.DepartmentCode, string.Format("N{0}", message.MessageId), null);
+ if (!string.IsNullOrWhiteSpace(message.DepartmentCode))
+ await _novuProvider.SendUserNotification(
+ message.Title,
+ message.SubTitle,
+ userId,
+ message.DepartmentCode,
+ string.Format("N{0}", message.MessageId),
+ ((int)PushSoundTypes.Notifiation).ToString());🤖 Prompt for AI Agents
In Core/Resgrid.Services/PushService.cs around line 147, guard the
DepartmentCode before calling _novuProvider.SendUserNotification: mirror the
same message-path guard used elsewhere by checking
string.IsNullOrWhiteSpace(message.DepartmentCode) and only pass the department
code when it is non-empty (otherwise pass null/empty so the recipientId isn’t
built as "_User_{userId}"); also consider replacing the null notification-type
argument with a concrete type string (eg. "notification") when calling
SendUserNotification.
| public GetSystemConfigResultData() | ||
| { | ||
| Locations = InfoConfig.Locations; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t return a reference to global mutable state (deep-copy Locations).
Assigning InfoConfig.Locations directly exposes and couples response instances to a shared static list. Any downstream mutation (even accidental) would alter global config for all requests. Deep-copy the entries before returning.
using System.Collections.Generic;
+using System.Linq;
using Resgrid.Config;
@@
public GetSystemConfigResultData()
{
- Locations = InfoConfig.Locations;
+ Locations = InfoConfig.Locations
+ .Select(l => new ResgridSystemLocation
+ {
+ Name = l.Name,
+ DisplayName = l.DisplayName,
+ LocationInfo = l.LocationInfo,
+ IsDefault = l.IsDefault,
+ ApiUrl = l.ApiUrl,
+ AllowsFreeAccounts = l.AllowsFreeAccounts
+ })
+ .ToList();
}Also applies to: 1-1
🤖 Prompt for AI Agents
In Web/Resgrid.Web.Services/Models/v4/Configs/GetSystemConfigResult.cs around
lines 35-38, the constructor assigns Locations = InfoConfig.Locations which
exposes a reference to global mutable state; replace this with a deep-copy of
the list and its entries (e.g. null-check InfoConfig.Locations, create a new
list and map/clone each Location item into a new DTO/instance copying all
relevant properties) so the returned Locations cannot mutate the shared static
list.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Configuration
Documentation