CU-8687aj3gt Adding Apns to Novu provider for Units #235
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (10)
WalkthroughAdds Novu APNS support and broader Novu user/unit notification APIs, updates platform enum and push handling to favor iOS/Android (FCM/APNS), routes device push location to department code, and updates PushService and providers to call new Novu methods; also adds DepartmentCode to push message model. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DevicesAPI
participant PushService
participant DepartmentsSvc
participant NovuProvider
Client->>DevicesAPI: RegisterDevice(input with DeptId)
DevicesAPI->>DepartmentsSvc: GetDepartmentById(DeptId)
DepartmentsSvc-->>DevicesAPI: department (code)
DevicesAPI->>PushService: Register(pushLocation=department.Code,...)
PushService->>NovuProvider: if iOS -> UpdateUserSubscriberApns(userId, code, deviceId)
PushService->>NovuProvider: if Android -> UpdateUserSubscriberFcm(userId, code, deviceId)
NovuProvider-->>PushService: success/failure
PushService-->>Client: registration result
sequenceDiagram
participant PushService
participant NovuProvider
participant NovuAPI
PushService->>NovuProvider: SendUserMessage(title, body, userId, depCode,...)
NovuProvider->>NovuAPI: POST v1/events/trigger (payload)
NovuAPI-->>NovuProvider: 2xx/4xx
NovuProvider-->>PushService: success/failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Poem
Tip You can get early access to new features in CodeRabbit.Enable the ✨ Finishing Touches
🧪 Generate unit tests
🪧 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: 4
🧹 Nitpick comments (1)
Core/Resgrid.Config/ChatConfig.cs (1)
14-14: Use APNS spelling for consistency and clarity.Field name uses "Aps" while everywhere else (providerId and PR title) use APNS. Recommend renaming to avoid confusion and align with providerId = "apns".
Apply this diff:
- public static string NovuUnitApsProviderId = ""; + public static string NovuUnitApnsProviderId = "";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Core/Resgrid.Config/ChatConfig.cs(1 hunks)Core/Resgrid.Model/Providers/Models/INovuProvider.cs(1 hunks)Core/Resgrid.Services/PushService.cs(1 hunks)Providers/Resgrid.Providers.Messaging/NovuProvider.cs(3 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:
Core/Resgrid.Model/Providers/Models/INovuProvider.csCore/Resgrid.Config/ChatConfig.csCore/Resgrid.Services/PushService.csProviders/Resgrid.Providers.Messaging/NovuProvider.cs
⏰ 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). (1)
- GitHub Check: build
| Task<bool> CreateUnitSubscriber(int unitId, string code, int departmentId, string unitName, string deviceId); | ||
| Task<bool> UpdateUserSubscriberFcm(string userId, string code, string token); | ||
| Task<bool> UpdateUnitSubscriberFcm(int unitId, string code, string token); | ||
| Task<bool> UpdateUnitSubscriberAps(int unitId, string code, string token); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Rename method to Apns to match industry terminology and the providerId.
"Aps" is ambiguous; APNS is the widely used term and matches providerId = "apns". Rename for clarity and consistency across the API and implementation.
Apply this diff:
- Task<bool> UpdateUnitSubscriberAps(int unitId, string code, string token);
+ Task<bool> UpdateUnitSubscriberApns(int unitId, string code, string token);To help update all references after the rename, run:
🏁 Script executed:
#!/bin/bash
# Find all Aps occurrences to update to Apns
rg -n --no-heading $'Update(Unit|User)?SubscriberAps|UpdateSubscriberAps|NovuUnitApsProviderId'Length of output: 921
Rename Aps to Apns across the Novu push API
The method name, its helper, calls, and the provider-ID constant must all be updated for consistency and clarity:
• Core/Resgrid.Model/Providers/Models/INovuProvider.cs
– Rename
diff - Task<bool> UpdateUnitSubscriberAps(int unitId, string code, string token); + Task<bool> UpdateUnitSubscriberApns(int unitId, string code, string token);
• Providers/Resgrid.Providers.Messaging/NovuProvider.cs
– Rename the private helper:
diff - private async Task<bool> UpdateSubscriberAps(string id, string token, string apnsId) + private async Task<bool> UpdateSubscriberApns(string id, string token, string apnsId)
– Rename the public method and its internal call:
diff - public async Task<bool> UpdateUnitSubscriberAps(int unitId, string code, string token) + public async Task<bool> UpdateUnitSubscriberApns(int unitId, string code, string token) … - return await UpdateSubscriberAps($"{code}_Unit_{unitId}", token, ChatConfig.NovuUnitApsProviderId); + return await UpdateSubscriberApns($"{code}_Unit_{unitId}", token, ChatConfig.NovuUnitApnsProviderId);
• Core/Resgrid.Services/PushService.cs
– Update the call site:
diff - await _novuProvider.UpdateUnitSubscriberAps(pushUri.UnitId.Value, pushUri.PushLocation, pushUri.DeviceId); + await _novuProvider.UpdateUnitSubscriberApns(pushUri.UnitId.Value, pushUri.PushLocation, pushUri.DeviceId);
• Core/Resgrid.Config/ChatConfig.cs
– Rename the constant:
diff - public static string NovuUnitApsProviderId = ""; + public static string NovuUnitApnsProviderId = "";
Run a final check to catch any stray “Aps” identifiers:
rg -n 'Aps\b'🤖 Prompt for AI Agents
In Core/Resgrid.Model/Providers/Models/INovuProvider.cs around line 11, the
method UpdateUnitSubscriberAps should be renamed to UpdateUnitSubscriberApns and
all related identifiers updated for consistency; rename the method signature in
this interface and then update
Providers/Resgrid.Providers.Messaging/NovuProvider.cs by renaming the public
method and its internal helper from *Aps to *Apns, updating any calls and
references, change the provider-ID constant name in
Core/Resgrid.Config/ChatConfig.cs from Aps to Apns, and update the call site in
Core/Resgrid.Services/PushService.cs to the new method name; after making these
changes, run a project-wide search (e.g., rg -n 'Aps\b') to find and replace any
remaining occurrences and rebuild to ensure no references are broken.
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (5)
Web/Resgrid.Web/Areas/User/Views/Profile/Devices.cshtml (1)
45-52: Platform label will misreport Android devices as Web due to enum value shiftsWith Platforms.Android moved to 2 and Web to 3 in the new enum, existing persisted devices that still have 3 for Android will render as Web. Avoid relying on enum ToString here and map legacy values explicitly.
Apply this diff to render correct, backward-compatible labels:
- @if ((Platforms)p.PlatformType == Platforms.iOS) - { - <span>iOS</span> - } - else - { - @Html.Raw(((Platforms)p.PlatformType).ToString()) - } + @if (p.PlatformType == 1 || p.PlatformType == 2) + { + <span>iOS</span> + } + else if (p.PlatformType == 3) + { + <span>Android</span> + } + else + { + @(((Platforms)p.PlatformType).ToString()) + }Providers/Resgrid.Providers.Bus/NotificationProvider.cs (2)
60-96: Registration logic never runs because of misplaced else-if; platform branches must not hang off the null-checkThe platform-specific registration is tied to the else of “if (registrations != null)”, so it will skip registration whenever GetRegistrationsByTagAsync returns a non-null result (which is most cases, even when empty). Remove the else and always attempt registration after cleanup.
Apply this diff:
- // Loop through all Azure registrations for this Hashed DeviceId and remove them - if (registrations != null) - { - foreach (var registration in registrations) - { - try - { - await hubClient.DeleteRegistrationAsync(registration); - } - catch (Exception ex) - { - //Framework.Logging.LogException(ex); - } - } - } - - - else if (pushUri.PlatformType == 3) //(int)Platforms.Android) + // Loop through all Azure registrations for this Hashed DeviceId and remove them + if (registrations != null) + { + foreach (var registration in registrations) + { + try + { + await hubClient.DeleteRegistrationAsync(registration); + } + catch (Exception ex) + { + //Framework.Logging.LogException(ex); + } + } + } + + // Register platform-specific + if (pushUri.PlatformType == 3) //(legacy Android) { try { //var result = await hubClient.CreateFcmNativeRegistrationAsync(pushUri.DeviceId, tagsWithHashedDeviceId.ToArray()); var result = await hubClient.CreateFcmV1NativeRegistrationAsync(pushUri.DeviceId, tagsWithHashedDeviceId.ToArray()); } catch (Exception ex) { //Framework.Logging.LogException(ex); } } - else if (pushUri.PlatformType == 1 || pushUri.PlatformType == 2) //(int)Platforms.iPad || pushUri.PlatformType == (int)Platforms.iPhone) + else if (pushUri.PlatformType == 1 || pushUri.PlatformType == 2) //(legacy Apple: iPad/iPhone) { try { var result = await hubClient.CreateAppleNativeRegistrationAsync(pushUri.DeviceId, tagsWithHashedDeviceId.ToArray()); } catch (Exception ex) { //Framework.Logging.LogException(ex); } }Follow-up: Replace magic numbers with a centralized resolver to handle both legacy and new enum values consistently (see next comment).
107-118: Replace magic platform numbers with centralized, backward-compatible checksHard-coded ints (1, 2, 3) are brittle and now contradict the updated enum. Introduce helpers to resolve legacy vs new values and use them here.
Apply this diff:
- if (pushUri.PlatformType == 3)// (int)Platforms.Android) + if (PlatformResolver.IsAndroid(pushUri.PlatformType)) { var androidReg = registration as GcmRegistrationDescription; if (androidReg != null && androidReg.GcmRegistrationId == pushUri.DeviceId) await hubClient.DeleteRegistrationAsync(registration); } - else if (pushUri.PlatformType == 1 || pushUri.PlatformType == 2) //(int)Platforms.iPad || pushUri.PlatformType == (int)Platforms.iPhone) + else if (PlatformResolver.IsApple(pushUri.PlatformType)) { var iosReg = registration as AppleRegistrationDescription; if (iosReg != null && iosReg.DeviceToken == pushUri.DeviceId) await hubClient.DeleteRegistrationAsync(registration); }Add this utility (one-time) to a shared location (e.g., Core/Resgrid.Model/PlatformResolver.cs):
namespace Resgrid.Model { public static class PlatformResolver { // Legacy values: 1=iPad, 2=iPhone, 3=Android, 4=Windows8 public static bool IsApple(int platformType) => platformType == 1 || platformType == 2 || (Platforms)platformType == Platforms.iOS; public static bool IsAndroid(int platformType) => platformType == 3 || (Platforms)platformType == Platforms.Android; } }Providers/Resgrid.Providers.Messaging/NovuProvider.cs (2)
24-24: Fix Authorization header prefix and namespace in CreateSubscriber (will 401).Header must include the "ApiKey " prefix, and the current reference uses a non-existent Config alias. Use ChatConfig directly.
Apply this diff:
- httpClient.DefaultRequestHeaders.Add("Authorization", Config.ChatConfig.NovuSecretKey); + httpClient.DefaultRequestHeaders.Add("Authorization", $"ApiKey {ChatConfig.NovuSecretKey}");
274-277: Avoid serializing null properties in the payload (e.g., channelId when type is null).type can be null in some calls (e.g., SendUserMessage), which leads to "channelId": null and similar fields. Instruct the serializer to ignore nulls to reduce FCM/Novu rejection risk.
Apply this diff:
- var content = new StringContent( - JsonConvert.SerializeObject(payload), + var content = new StringContent( + JsonConvert.SerializeObject(payload, new JsonSerializerSettings { NullValueHandling = NullValueHandling.Ignore }), Encoding.UTF8, "application/json");
♻️ Duplicate comments (2)
Core/Resgrid.Model/Providers/Models/INovuProvider.cs (1)
58-67: Method rename to Apns is correct and consistent with providerId namingThe APNS naming resolves prior ambiguity (“Aps”). Thanks for aligning with industry terminology.
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (1)
20-21: Reuse HttpClient via IHttpClientFactory to avoid socket exhaustion and consolidate headers.Multiple new HttpClient instances per request can exhaust sockets and degrade performance. Prefer a single named client configured with base address and default headers.
Example approach (outside these hunks):
// Field + ctor private readonly IHttpClientFactory _httpClientFactory; public NovuProvider(IHttpClientFactory httpClientFactory) => _httpClientFactory = httpClientFactory; // Usage var client = _httpClientFactory.CreateClient("Novu"); client.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("ApiKey", ChatConfig.NovuSecretKey); client.DefaultRequestHeaders.Add("idempotency-key", Guid.NewGuid().ToString()); // For absolute URLs: client.BaseAddress = new Uri(ChatConfig.NovuBackendUrl);Register the named client in DI with BaseAddress = ChatConfig.NovuBackendUrl and default headers set via HttpClientFactory configuration.
Also applies to: 101-102, 137-138, 173-174, 216-217
🧹 Nitpick comments (9)
Core/Resgrid.Model/Messages/StandardPushMessage.cs (1)
9-9: Consider documenting and validating the new DepartmentCode fieldAdd XML docs for discoverability and clarify whether null/empty is allowed. If this is optional, consider marking it nullable (string?) when NRT is enabled; otherwise ensure upstream always sets it.
Web/Resgrid.Web/Areas/User/Views/Profile/Devices.cshtml (1)
61-61: Avoid Html.Raw on enum strings in confirmation textEnums are safe, but Raw is unnecessary and risks future XSS if inputs change. Let Razor encode by default.
Apply this diff:
- <a asp-controller="Home" asp-action="DeletePushUri" asp-route-area="User" asp-route-pushUriId="@p.PushUriId" class="btn btn-xs btn-danger" data-confirm="WARNING: This will remove the ability to push messages to your @Html.Raw(((Platforms)p.PlatformType).ToString()) device. Are you sure you want to delete your @Html.Raw(((Platforms)p.PlatformType).ToString()) device?" rel="nofollow">Delete</a> + <a asp-controller="Home" asp-action="DeletePushUri" asp-route-area="User" asp-route-pushUriId="@p.PushUriId" class="btn btn-xs btn-danger" data-confirm="WARNING: This will remove the ability to push messages to your @(((Platforms)p.PlatformType).ToString()) device. Are you sure you want to delete your @(((Platforms)p.PlatformType).ToString()) device?" rel="nofollow">Delete</a>Providers/Resgrid.Providers.Bus/NotificationProvider.cs (1)
249-272: Optional: Reuse hubClient across calls and reduce string allocationsYou already allow passing a hubClient; good. Consider caching channel derivation and avoiding repeated ToLower calls by computing eventType once. Not urgent.
Core/Resgrid.Model/Providers/Models/INovuProvider.cs (5)
92-96: Add XML docs for the new SendUser methods*Other members are documented; document these for parity (parameters, behavior, return semantics).
20-31: Consider adding CancellationToken parameters to async methodsImprove resilience and testability by allowing callers to cancel outbound calls to Novu and hub operations.
Example signature change:
Task<bool> CreateUserSubscriber(string userId, string code, int departmentId, string email, string firstName, string lastName, System.Threading.CancellationToken ct = default);Apply similarly to other methods that perform I/O.
Also applies to: 40-50, 58-68, 82-84, 90-90, 92-96
20-20: Parameter naming: clarify “code” semanticAcross the interface, “code” sometimes refers to a Novu integration key and elsewhere to a department code (depCode). Consider renaming to integrationCode and departmentCode respectively to avoid ambiguity.
5-7: Interface summary is helpful; consider referencing workflows/providers explicitlyAdd references to ChatConfig workflow/provider IDs this interface expects (e.g., NovuDispatchUserWorkflowId, NovuUnitApnsProviderId) to make implementers aware of required configuration.
1-4: Namespace/file-scoped style (optional)If the repo targets modern C#, consider file-scoped namespace to reduce indentation noise.
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (1)
259-266: Remove commented-out data block for cleanliness.Dead commented code reduces readability. It’s already represented in the structured payload above.
Apply this diff:
- //data = new - //{ - // title = title, - // message = body, - // eventCode = eventCode, - // type = type - //}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Core/Resgrid.Config/ChatConfig.cs(1 hunks)Core/Resgrid.Model/Messages/StandardPushMessage.cs(1 hunks)Core/Resgrid.Model/Platforms.cs(1 hunks)Core/Resgrid.Model/Providers/Models/INovuProvider.cs(1 hunks)Core/Resgrid.Model/PushUri.cs(1 hunks)Core/Resgrid.Services/PushService.cs(6 hunks)Providers/Resgrid.Providers.Bus/NotificationProvider.cs(5 hunks)Providers/Resgrid.Providers.Bus/UnitNotificationProvider.cs(5 hunks)Providers/Resgrid.Providers.Messaging/NovuProvider.cs(8 hunks)Web/Resgrid.Web.Services/Controllers/v3/DevicesController.cs(1 hunks)Web/Resgrid.Web.Services/Controllers/v4/DevicesController.cs(2 hunks)Web/Resgrid.Web/Areas/User/Views/Profile/Devices.cshtml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/Resgrid.Config/ChatConfig.cs
🧰 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:
Core/Resgrid.Model/Messages/StandardPushMessage.csWeb/Resgrid.Web.Services/Controllers/v3/DevicesController.csCore/Resgrid.Model/PushUri.csWeb/Resgrid.Web.Services/Controllers/v4/DevicesController.csProviders/Resgrid.Providers.Bus/UnitNotificationProvider.csCore/Resgrid.Services/PushService.csProviders/Resgrid.Providers.Bus/NotificationProvider.csCore/Resgrid.Model/Platforms.csCore/Resgrid.Model/Providers/Models/INovuProvider.csProviders/Resgrid.Providers.Messaging/NovuProvider.cs
🧬 Code Graph Analysis (5)
Web/Resgrid.Web.Services/Controllers/v4/DevicesController.cs (1)
Web/Resgrid.Web.Services/Controllers/v3/DevicesController.cs (1)
DevicesController(39-52)
Providers/Resgrid.Providers.Bus/UnitNotificationProvider.cs (2)
Providers/Resgrid.Providers.Bus/NotificationProvider.cs (1)
GetSoundFileNameFromType(371-424)Providers/Resgrid.Providers.Messaging/NovuProvider.cs (1)
GetSoundFileNameFromType(313-365)
Core/Resgrid.Services/PushService.cs (1)
Core/Resgrid.Framework/Logging.cs (2)
Logging(15-178)LogException(65-77)
Providers/Resgrid.Providers.Bus/NotificationProvider.cs (2)
Providers/Resgrid.Providers.Bus/UnitNotificationProvider.cs (1)
GetSoundFileNameFromType(400-460)Providers/Resgrid.Providers.Messaging/NovuProvider.cs (1)
GetSoundFileNameFromType(313-365)
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (5)
Core/Resgrid.Model/Providers/Models/INovuProvider.cs (11)
Task(20-20)Task(31-31)Task(40-40)Task(49-49)Task(58-58)Task(67-67)Task(82-83)Task(90-90)Task(92-92)Task(94-94)Task(96-96)Core/Resgrid.Services/PushService.cs (5)
Task(36-53)Task(55-60)Task(62-80)Task(82-87)Task(94-124)Core/Resgrid.Config/ChatConfig.cs (1)
ChatConfig(3-20)Providers/Resgrid.Providers.Bus/NotificationProvider.cs (1)
GetSoundFileNameFromType(371-424)Providers/Resgrid.Providers.Bus/UnitNotificationProvider.cs (1)
GetSoundFileNameFromType(400-460)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (11)
Providers/Resgrid.Providers.Bus/NotificationProvider.cs (2)
319-319: LGTM: APNS payload sound selection uses iOS consistentlyCorrectly switches to iOS-based sound naming, aligning with the enum rename and APNS expectations.
371-424: LGTM: Unified iOS sound mapping across all branchesConsistent use of Platforms.iOS for CAF sound variants looks correct and matches related provider changes.
Core/Resgrid.Services/PushService.cs (2)
71-72: Method name inconsistency: UpdateUnitSubscriberApns vs UpdateUnitSubscriberFcmThe iOS update method is called
UpdateUnitSubscriberApnswhile the Android method isUpdateUnitSubscriberFcm. For consistency with the user registration methods that useUpdateUserSubscriberApns, this is correct naming.
115-115: No NullReference risk for DepartmentCode in SendUserMessage
TheSendUserMessageimplementation in NovuProvider.cs (around line 301) uses$"{depCode}_User_{userId}"In C#, interpolating a null string yields an empty string rather than throwing, so passing a null
departmentCodewon’t cause a NullReferenceException. You can safely ignore the original null-reference concern.Likely an incorrect or invalid review comment.
Providers/Resgrid.Providers.Messaging/NovuProvider.cs (7)
133-167: APNS subscriber update helper looks correct.Good use of providerId = "apns" with deviceTokens and integrationIdentifier; correct headers and idempotency key.
206-210: Unit APNS wrapper correctly delegates to UpdateSubscriberApns.This aligns with the providerId = "apns" path and uses the correct Unit APNS provider id.
111-111: LGTM: providerId comment for FCM.The inline reference to Novu enums helps future maintainers.
317-317: Platform rename to iOS is consistent and correct.Consistently using Platforms.iOS improves clarity across sound mapping and APNS payload.
Also applies to: 325-325, 333-333, 339-339, 346-346, 353-353, 360-360, 470-470
85-92: Including DeviceId in unit subscriber data is a solid improvement.This metadata will help correlate devices to unit subscribers downstream.
296-309: New user notification wrappers are clear and cohesive.The SendUserDispatch/Message/Notification helpers align with the INovuProvider contract and routing via depCode/userId.
240-251: Incorrect FCM priority recommendations – no changes neededThe existing override already follows Novu’s mapping to Firebase Admin (Node) types:
- Top-level
android.prioritymust be lowercase"high"/"normal"(not uppercase).android.notification.priority(display priority) is a valid field and accepts"min" | "low" | "default" | "high" | "max".- Other fields (
channelId,defaultSound,sticky) are correctly camelCased.You can leave the code as-is:
android = new { priority = androidChannelName == "calls" ? "high" : "normal", notification = new { channelId = type, defaultSound = true, sticky = androidChannelName == "calls", priority = androidChannelName == "calls" ? "max" : "default", }, data = new { // … } }Likely an incorrect or invalid review comment.
| None = 0, | ||
| iOS = 1, | ||
| Android = 2, | ||
| Web = 3, | ||
| Windows8 = 4, | ||
| WindowsPhone8 = 5, | ||
| Blackberry = 6, | ||
| UnitIOS = 7, | ||
| UnitAndroid = 8, | ||
| UnitWin = 9 | ||
| //Windows8 = 4, | ||
| //WindowsPhone8 = 5, | ||
| //Blackberry = 6, | ||
| //UnitIOS = 7, | ||
| //UnitAndroid = 8, | ||
| //UnitWin = 9 |
There was a problem hiding this comment.
Breaking enum value changes will corrupt persisted platform mappings and conflict with hard-coded checks
Previous code (and likely persisted DB values) use 1=iPad, 2=iPhone, 3=Android, 4=Windows8. This change sets iOS=1, Android=2, Web=3, which:
- Mislabels existing devices in the UI (3 now renders as Web).
- Breaks provider logic using numeric checks (e.g., NotificationProvider treats 3 as Android).
Preserve legacy numeric values and introduce new names without reassigning existing numbers. Optionally re-add legacy members marked [Obsolete] for compatibility.
Apply this diff to restore stable values and keep compatibility:
- None = 0,
- iOS = 1,
- Android = 2,
- Web = 3,
- Windows8 = 4,
- //Windows8 = 4,
+ None = 0,
+ // Legacy Apple values retained for compatibility
+ [System.Obsolete("Use iOS (value 2) where possible. This remains for legacy records.")]
+ iPad = 1,
+ // iOS aligns with former iPhone value (2)
+ iOS = 2,
+ // Android retains legacy value (3)
+ Android = 3,
+ Windows8 = 4,
+ // New platform should use an unused value to avoid collisions
+ Web = 5,
//WindowsPhone8 = 5,
//Blackberry = 6,
//UnitIOS = 7,
//UnitAndroid = 8,
//UnitWin = 9If changing enum assignments is not feasible now, introduce centralized helpers to interpret legacy ints consistently and update UI/provider code to use them.
📝 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.
| None = 0, | |
| iOS = 1, | |
| Android = 2, | |
| Web = 3, | |
| Windows8 = 4, | |
| WindowsPhone8 = 5, | |
| Blackberry = 6, | |
| UnitIOS = 7, | |
| UnitAndroid = 8, | |
| UnitWin = 9 | |
| //Windows8 = 4, | |
| //WindowsPhone8 = 5, | |
| //Blackberry = 6, | |
| //UnitIOS = 7, | |
| //UnitAndroid = 8, | |
| //UnitWin = 9 | |
| None = 0, | |
| // Legacy Apple values retained for compatibility | |
| [System.Obsolete("Use iOS (value 2) where possible. This remains for legacy records.")] | |
| iPad = 1, | |
| // iOS aligns with former iPhone value (2) | |
| iOS = 2, | |
| // Android retains legacy value (3) | |
| Android = 3, | |
| Windows8 = 4, | |
| // New platform should use an unused value to avoid collisions | |
| Web = 5, | |
| //WindowsPhone8 = 5, | |
| //Blackberry = 6, | |
| //UnitIOS = 7, | |
| //UnitAndroid = 8, | |
| //UnitWin = 9 |
🤖 Prompt for AI Agents
In Core/Resgrid.Model/Platforms.cs around lines 5 to 15, the enum numeric
assignments were changed which will corrupt persisted platform mappings and
break any numeric checks; restore the original numeric values for legacy members
(e.g., ensure 1=iPad/iOS legacy mapping, 2=iPhone/Android legacy mapping,
3=Android or previous legacy meaning, 4=Windows8) and reintroduce any removed
legacy members as distinct enum entries annotated with [Obsolete] pointing to
the new names so existing DB values and hard-coded numeric checks continue to
work; alternatively, if you cannot change enum values now, add a central
conversion helper that maps persisted ints to the new enum names and update
UI/provider code to call that helper everywhere instead of relying on raw
numeric comparisons.
| //if (((Platforms)PlatformType) == Platforms.Windows8 || ((Platforms)PlatformType) == Platforms.WindowsPhone7 || ((Platforms)PlatformType) == Platforms.WindowsPhone8 || ((Platforms)PlatformType) == Platforms.UnitWin) | ||
| // ChannelUri = new Uri(_pushLocation, UriKind.Absolute); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove commented-out code to maintain code cleanliness
Since Windows platform support is being phased out as part of this PR, the commented code should be removed rather than left as comments. This improves code maintainability and clarity.
-//if (((Platforms)PlatformType) == Platforms.Windows8 || ((Platforms)PlatformType) == Platforms.WindowsPhone7 || ((Platforms)PlatformType) == Platforms.WindowsPhone8 || ((Platforms)PlatformType) == Platforms.UnitWin)
-// ChannelUri = new Uri(_pushLocation, UriKind.Absolute);📝 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.
| //if (((Platforms)PlatformType) == Platforms.Windows8 || ((Platforms)PlatformType) == Platforms.WindowsPhone7 || ((Platforms)PlatformType) == Platforms.WindowsPhone8 || ((Platforms)PlatformType) == Platforms.UnitWin) | |
| // ChannelUri = new Uri(_pushLocation, UriKind.Absolute); |
🤖 Prompt for AI Agents
In Core/Resgrid.Model/PushUri.cs around lines 48-49, there is commented-out
legacy code handling Windows platform ChannelUri creation; remove these two
commented lines entirely to clean up dead code now that Windows platform support
is phased out, and ensure no remaining references depend on this commented
block.
|
|
||
| try | ||
| { | ||
| await _novuProvider.SendUserDispatch(call.Title, call.SubTitle, userId, call.DepartmentCode, string.Format("C{0}", call.CallId), ConvertCallPriorityToSound((int)call.Priority, priority), true, call.ActiveCallCount, color); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify DepartmentCode is populated in StandardPushCall
The code passes call.DepartmentCode to SendUserDispatch, but StandardPushCall may not always have this property populated. Ensure it's properly set before this method is called.
🏁 Script executed:
#!/bin/bash
# Check StandardPushCall definition and usage to verify DepartmentCode is always set
ast-grep --pattern 'class StandardPushCall {
$$$
public string DepartmentCode { $$$; set; }
$$$
}'
# Check where StandardPushCall is instantiated
rg -A 5 "new StandardPushCall"Length of output: 1586
StandardPushCall.DepartmentCode is never set
In PushService.cs you pass call.DepartmentCode to SendUserDispatch, but all instantiations of StandardPushCall in CommunicationService.cs only assign DepartmentId. This means DepartmentCode remains null and downstream notifications won’t include the correct department code.
Please update each instantiation in
Core/Resgrid.Services/CommunicationService.cs
to also set DepartmentCode. For example:
// existing assignment
spc.DepartmentId = departmentId;
+ spc.DepartmentCode = departmentCode; // ensure DepartmentCode is populatedAffected locations (3 occurrences of new StandardPushCall() → setting spc.DepartmentId):
- Core/Resgrid.Services/CommunicationService.cs: around lines where
spc.DepartmentId = …is called.
🤖 Prompt for AI Agents
In Core/Resgrid.Services/CommunicationService.cs around the three locations
where new StandardPushCall() is created and spc.DepartmentId is assigned, the
DepartmentCode property is never set causing null department codes downstream;
update each instantiation to also set spc.DepartmentCode to the correct
department code (e.g., the department object's Code/DepartmentCode or the source
call’s DepartmentCode value depending on available variables) so that
SendUserDispatch receives a valid DepartmentCode.
| } | ||
| } | ||
| else if (pushUri.PlatformType == (int)Platforms.UnitAndroid) | ||
| if (pushUri.PlatformType == 8)//(int)Platforms.UnitAndroid) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use enum values instead of hard-coded platform type numbers
The code uses hard-coded value 8 with a comment indicating it's for UnitAndroid. This should use the enum constant for better maintainability and type safety.
Based on the PR context, it appears platform enums are being simplified. If UnitAndroid is being removed, consider using the new Android enum value:
-if (pushUri.PlatformType == 8)//(int)Platforms.UnitAndroid)
+if (pushUri.PlatformType == (int)Platforms.Android)🤖 Prompt for AI Agents
In Providers/Resgrid.Providers.Bus/UnitNotificationProvider.cs around line 69,
replace the hard-coded numeric literal `8` with the corresponding platform enum
constant (do not leave the commented hint). Update the condition to compare
against the enum member introduced by the refactor (for example
Platforms.UnitAndroid or the new Android enum value provided by the
Platform/Platforms enum), and ensure the correct namespace or using is present
so the code compiles; remove the inline comment and any now-stale numeric
references.
| } | ||
| } | ||
| else if (pushUri.PlatformType == (int)Platforms.UnitIOS) | ||
| else if (pushUri.PlatformType == 7)//(int)Platforms.UnitIOS) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use enum values instead of hard-coded platform type numbers
Similar to the Android check above, use the enum constant instead of hard-coded value 7.
-else if (pushUri.PlatformType == 7)//(int)Platforms.UnitIOS)
+else if (pushUri.PlatformType == (int)Platforms.iOS)🤖 Prompt for AI Agents
In Providers/Resgrid.Providers.Bus/UnitNotificationProvider.cs around line 83,
the platform check uses a hard-coded literal `7`; replace this with the
appropriate enum constant (e.g., Platforms.UnitIOS) instead of the magic number.
Update the comparison to use the enum value (or cast if necessary to match the
pushUri.PlatformType type) so it reads the enum reference rather than `7`,
keeping consistency with the Android check above.
| foreach (var registration in registrations) | ||
| { | ||
| if (pushUri.PlatformType == (int)Platforms.Android) | ||
| if (pushUri.PlatformType == 3 || pushUri.PlatformType == 8)//(int)Platforms.Android) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use enum values for platform type comparisons
Multiple hard-coded platform type values (3, 8) are used. Replace with appropriate enum constants.
-if (pushUri.PlatformType == 3 || pushUri.PlatformType == 8)//(int)Platforms.Android)
+if (pushUri.PlatformType == (int)Platforms.Android)Note: If value 3 represents a different platform that should still be supported, please verify and update accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Providers/Resgrid.Providers.Bus/UnitNotificationProvider.cs around line 107,
the code compares pushUri.PlatformType to hard-coded integers (3 and 8); replace
these numeric literals with the corresponding enum members (e.g.,
Platforms.Android for 8 if that is the intended mapping, and the correct enum
value for 3) so the comparison reads pushUri.PlatformType == (int)Platforms.X
or, better, cast pushUri.PlatformType to the enum and compare directly
(pushUri.PlatformTypeEnum == Platforms.X); verify the actual meaning of value 3
and update the enum member used accordingly to preserve intended behavior.
| await hubClient.DeleteRegistrationAsync(registration); | ||
| } | ||
| else if (pushUri.PlatformType == (int)Platforms.iPad || pushUri.PlatformType == (int)Platforms.iPhone) | ||
| else if (pushUri.PlatformType == 1 || pushUri.PlatformType == 2 || pushUri.PlatformType == 7)//(int)Platforms.iPad || pushUri.PlatformType == (int)Platforms.iPhone) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use enum values for platform type comparisons
Multiple hard-coded platform type values (1, 2, 7) are used. Replace with appropriate enum constants.
-else if (pushUri.PlatformType == 1 || pushUri.PlatformType == 2 || pushUri.PlatformType == 7)//(int)Platforms.iPad || pushUri.PlatformType == (int)Platforms.iPhone)
+else if (pushUri.PlatformType == (int)Platforms.iOS)Note: Based on the PR context, platforms 1, 2, and 7 all represent iOS devices that have been consolidated.
📝 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.
| else if (pushUri.PlatformType == 1 || pushUri.PlatformType == 2 || pushUri.PlatformType == 7)//(int)Platforms.iPad || pushUri.PlatformType == (int)Platforms.iPhone) | |
| @@ Providers/Resgrid.Providers.Bus/UnitNotificationProvider.cs (line 113) | |
| - else if (pushUri.PlatformType == 1 || pushUri.PlatformType == 2 || pushUri.PlatformType == 7)//(int)Platforms.iPad || pushUri.PlatformType == (int)Platforms.iPhone) | |
| + else if (pushUri.PlatformType == (int)Platforms.iOS) |
🤖 Prompt for AI Agents
In Providers/Resgrid.Providers.Bus/UnitNotificationProvider.cs around line 113,
replace the hard-coded numeric platform comparisons (1, 2, 7) with the
appropriate Platforms enum constants (e.g., Platforms.iPad, Platforms.iPhone and
the enum member representing the third iOS device value or the consolidated
Platforms.iOS if present) so the code reads comparisons against enum members
instead of literals; update the comment to reflect enum usage and prefer a
single consolidated enum check for iOS devices if your enum defines one.
| public async Task<bool> UpdateUserSubscriberApns(string userId, string code, string token) | ||
| { | ||
| return await UpdateSubscriberFcm($"{code}_User_{userId}", token, ChatConfig.NovuUnitApnsProviderId); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
UpdateUserSubscriberApns calls the FCM path and uses the Unit APNS provider id.
This should call UpdateSubscriberApns and use the responder/user APNS integration identifier (not the unit one). As-is, APNS tokens will be stored under the wrong provider, and the FCM updater won’t attach APNS tokens.
Proposed fix:
- public async Task<bool> UpdateUserSubscriberApns(string userId, string code, string token)
- {
- return await UpdateSubscriberFcm($"{code}_User_{userId}", token, ChatConfig.NovuUnitApnsProviderId);
- }
+ public async Task<bool> UpdateUserSubscriberApns(string userId, string code, string token)
+ {
+ return await UpdateSubscriberApns($"{code}_User_{userId}", token, ChatConfig.NovuResponderApnsProviderId);
+ }Verify the ChatConfig field exists or add it:
🏁 Script executed:
#!/bin/bash
# Check if the responder APNS provider id is defined anywhere
rg -n "NovuResponderApnsProviderId" -S
# If not found, open Core/Resgrid.Config/ChatConfig.cs and add:
# public static string NovuResponderApnsProviderId = "";Length of output: 38
🏁 Script executed:
#!/bin/bash
set -e
# Locate ChatConfig.cs
echo "Finding ChatConfig.cs..."
CONFIG_FILE=$(fd --ignore-case "ChatConfig\.cs$" || true)
if [[ -z "$CONFIG_FILE" ]]; then
echo "ChatConfig.cs not found"
exit 0
fi
echo "Found: $CONFIG_FILE"
# Show the first 200 lines of ChatConfig.cs
echo "----- ChatConfig.cs (1–200) -----"
sed -n '1,200p' "$CONFIG_FILE"
# List all Novu*ProviderId constants
echo "----- Novu*ProviderId constants -----"
rg -n "Novu.*ProviderId" -S
# Search for UpdateSubscriberApns and UpdateSubscriberFcm definitions
echo "----- UpdateSubscriberApns occurrences -----"
rg -n "UpdateSubscriberApns" -S || echo "None found"
echo "----- UpdateSubscriberFcm occurrences -----"
rg -n "UpdateSubscriberFcm" -S || echo "None found"Length of output: 3499
Correct the APNS path and provider ID for user subscribers
The UpdateUserSubscriberApns method is currently invoking the FCM updater with the unit APNS provider ID. It must call the APNS updater with a responder-specific APNS provider. Additionally, we need to introduce the missing NovuResponderApnsProviderId constant in ChatConfig.cs.
Changes required:
- In
Providers/Resgrid.Providers.Messaging/NovuProvider.cs, updateUpdateUserSubscriberApnsto callUpdateSubscriberApnsand switch toChatConfig.NovuResponderApnsProviderId. - In
Core/Resgrid.Config/ChatConfig.cs, add theNovuResponderApnsProviderIdfield.
Diff for NovuProvider.cs:
--- a/Providers/Resgrid.Providers.Messaging/NovuProvider.cs
+++ b/Providers/Resgrid.Providers.Messaging/NovuProvider.cs
@@ -196,7 +196,7 @@
public async Task<bool> UpdateUserSubscriberApns(string userId, string code, string token)
{
- return await UpdateSubscriberFcm($"{code}_User_{userId}", token, ChatConfig.NovuUnitApnsProviderId);
+ return await UpdateSubscriberApns($"{code}_User_{userId}", token, ChatConfig.NovuResponderApnsProviderId);
}Snippet to add in ChatConfig.cs (inside the ChatConfig class):
public static string NovuResponderApnsProviderId = "";Please implement these updates to ensure APNS tokens are stored under the correct provider.
🤖 Prompt for AI Agents
In Providers/Resgrid.Providers.Messaging/NovuProvider.cs around lines 196-199,
change the method UpdateUserSubscriberApns to call UpdateSubscriberApns (not
UpdateSubscriberFcm) and pass ChatConfig.NovuResponderApnsProviderId as the
provider id so APNS tokens use the responder APNS provider; and in
Core/Resgrid.Config/ChatConfig.cs add a public static string
NovuResponderApnsProviderId = "" inside the ChatConfig class to introduce the
missing constant.
| else if (registrationInput.Plt == (int)Platforms.Android) | ||
| push.PushLocation = "Android"; | ||
| else if (registrationInput.Plt == (int)Platforms.iPhone || registrationInput.Plt == (int)Platforms.iPad) | ||
| else if (registrationInput.Plt == 1 || registrationInput.Plt == 2)//(int)Platforms.iPhone || registrationInput.Plt == (int)Platforms.iPad) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use platform enum values consistently instead of hard-coded integers
The code uses hard-coded numeric values (1 and 2) for platform comparison, while the rest of the codebase uses the Platforms enum. This makes the code less maintainable and harder to understand.
-else if (registrationInput.Plt == 1 || registrationInput.Plt == 2)//(int)Platforms.iPhone || registrationInput.Plt == (int)Platforms.iPad)
+else if (registrationInput.Plt == (int)Platforms.iOS)Note: Based on the PR context, the platform enum has been simplified to iOS=1, Android=2. The old iPhone and iPad values have been consolidated into iOS.
📝 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.
| else if (registrationInput.Plt == 1 || registrationInput.Plt == 2)//(int)Platforms.iPhone || registrationInput.Plt == (int)Platforms.iPad) | |
| else if (registrationInput.Plt == (int)Platforms.iOS) |
🤖 Prompt for AI Agents
In Web/Resgrid.Web.Services/Controllers/v3/DevicesController.cs around line 81,
replace the hard-coded numeric platform checks (registrationInput.Plt == 1 ||
registrationInput.Plt == 2) with comparisons that use the Platforms enum (e.g.,
check registrationInput.Plt against (int)Platforms.iOS and/or
(int)Platforms.Android or cast registrationInput.Plt to Platforms and compare to
Platforms.iOS/Platforms.Android); update the condition to use the enum values so
the intent is clear and future changes to the enum remain consistent.
| var department = await _departmentsService.GetDepartmentByIdAsync(DepartmentId, false); | ||
| push.PushLocation = department.Code; |
There was a problem hiding this comment.
Add null check for department retrieval
The code assumes that GetDepartmentByIdAsync will always return a valid department object. If it returns null, accessing department.Code will throw a NullReferenceException.
var department = await _departmentsService.GetDepartmentByIdAsync(DepartmentId, false);
+if (department == null)
+{
+ result.Status = ResponseHelper.Failure;
+ return result;
+}
push.PushLocation = department.Code;📝 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.
| var department = await _departmentsService.GetDepartmentByIdAsync(DepartmentId, false); | |
| push.PushLocation = department.Code; | |
| var department = await _departmentsService.GetDepartmentByIdAsync(DepartmentId, false); | |
| if (department == null) | |
| { | |
| result.Status = ResponseHelper.Failure; | |
| return result; | |
| } | |
| push.PushLocation = department.Code; |
🤖 Prompt for AI Agents
In Web/Resgrid.Web.Services/Controllers/v4/DevicesController.cs around lines
157-158, the code uses department.Code without checking if department is null;
add a null check after awaiting
_departmentsService.GetDepartmentByIdAsync(DepartmentId, false) and handle the
null case (e.g., return an appropriate IActionResult like NotFound/BadRequest,
or set push.PushLocation to a safe default/null and log a warning) to avoid a
NullReferenceException.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Summary by CodeRabbit
New Features
Bug Fixes
Chores