Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughThis PR migrates the TTS subsystem from eSpeak NG to Piper (config, runtime, Docker image, model resolution, invocation, health checks), updates tests and documentation to match Piper, switches WeatherAlertService to use ICommunicationService with per-recipient sends, tightens some null-safety checks in billing/plan handling, and replaces CLAUDE.md with a Resgrid project guide. ChangesText-to-Speech Engine Migration: eSpeak → Piper
WeatherAlertService: per-recipient communication
Billing & Data-Shape Null Safety
Tests — Mocking, In-Memory Fixtures, and Resilience
Project Documentation Replacement
XML Docs Normalization
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebTtsService as Resgrid.Web.Tts
participant PiperBinary as /usr/local/bin/piper
participant ModelStore as /usr/local/share/piper-voices
participant Ffmpeg
Client->>WebTtsService: Request synthesize(text, voice, speed)
WebTtsService->>WebTtsService: Preprocess text
WebTtsService->>WebTtsService: Resolve model (VoiceModelMap) and compute length-scale
WebTtsService->>PiperBinary: Start piper --model ModelFile --output_file tmp.wav --length-scale X
PiperBinary->>ModelStore: Load ModelFile.onnx
PiperBinary-->>WebTtsService: Write tmp.wav
WebTtsService->>Ffmpeg: Normalize tmp.wav -> normalized.wav
WebTtsService-->>Client: Return normalized audio URL / stream
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Core/Resgrid.Services/SubscriptionsService.cs (1)
92-119:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
GetPlanCountsForDepartmentAsynclacks exception handling — unhandled exceptions propagate to callers.Unlike
GetCurrentPlanForDepartmentAsync(lines 59–86), which wraps the HTTP call in atry-catch, this method has no exception handling. Any network failure, DNS error, or deserialization exception will propagate unhandled to callers such asValidateDepartmentIsWithinLimitsAsyncandCanDepartmentAddNewUnitinLimitsService. Combined with the 200-secondMaxTimeout(vs. the 5-second timeout on the plan-fetch method), this can cause long-blocking or unhandled exceptions on request threads.🛡️ Suggested fix — add try-catch consistent with `GetCurrentPlanForDepartmentAsync`
- var response = await client.ExecuteAsync<GetPlanCountsForDepartmentResult>(request); - - if (response.StatusCode == HttpStatusCode.NotFound) - return new DepartmentPlanCount(); - - if (response.Data == null || response.Data.Data == null) - return new DepartmentPlanCount(); - - return response.Data.Data; + try + { + var response = await client.ExecuteAsync<GetPlanCountsForDepartmentResult>(request); + + if (response.StatusCode == HttpStatusCode.NotFound) + return new DepartmentPlanCount(); + + if (response.Data == null || response.Data.Data == null) + return new DepartmentPlanCount(); + + return response.Data.Data; + } + catch (Exception ex) + { + Framework.Logging.LogException(ex); + return new DepartmentPlanCount(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/SubscriptionsService.cs` around lines 92 - 119, GetPlanCountsForDepartmentAsync currently makes the RestClient call without any try/catch so network/DNS/deserialization failures (and the long 200s MaxTimeout) will surface to callers; wrap the HTTP call and response handling in a try/catch like GetCurrentPlanForDepartmentAsync, log the exception (use your existing logger), and return a new DepartmentPlanCount() on any exception; while updating, consider aligning MaxTimeout with the shorter timeout used in GetCurrentPlanForDepartmentAsync to avoid long blocking.
🧹 Nitpick comments (1)
Core/Resgrid.Services/SubscriptionsService.cs (1)
141-145: ⚡ Quick win~20 other
return response.Data.Datasites in this file are not guarded againstresponse.Data.Data == null.The fix applied on lines 77 and 112–113 addresses the same NPE risk that exists in many other methods:
GetCurrentPaymentForDepartmentAsync(line 144),GetPreviousNonFreePaymentForDepartmentAsync(line 195),GetUpcomingPaymentForDepartmentAsync(line 230),GetPlanByIdAsync(line 293),GetPlanByExternalIdAsync(line 332), and more. Callers of plan-returning methods in particular may access.PlanIdon the result without null-checking.Consider applying
|| response.Data.Data == nullconsistently across all similar null-guard checks in this class. Based on learnings:response.Data.Datacan be null even when the API succeeds andresponse.Datais non-null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/SubscriptionsService.cs` around lines 141 - 145, Several methods (e.g., GetCurrentPaymentForDepartmentAsync, GetPreviousNonFreePaymentForDepartmentAsync, GetUpcomingPaymentForDepartmentAsync, GetPlanByIdAsync, GetPlanByExternalIdAsync and other similar return sites) currently return response.Data.Data without guarding against response.Data.Data being null; add the same null-guard pattern used earlier (check response == null || response.Data == null || response.Data.Data == null) before returning so these methods return null when response.Data.Data is null to avoid NPEs when callers access fields like .PlanId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 50: Update the misspelled test project name entry in CLAUDE.md: replace
the string "Resgrid.Intergration.Tests/" with the correct
"Resgrid.Integration.Tests/" so the project name is spelled consistently (look
for the line containing Resgrid.Intergration.Tests/ in CLAUDE.md and correct the
typo).
- Around line 69-71: Update the fenced code block that contains the architecture
line "Config → Model → Services → Repositories/Providers → Web/Workers"
to include a language identifier (e.g., change ``` to ```text) so the block is
flagged as plain text and satisfies markdownlint MD040.
- Around line 9-53: The fenced code block in CLAUDE.md is missing a language
identifier, triggering markdownlint MD040; update the opening triple-backtick
for the directory tree to include a language (e.g., add "text" so it reads
```text) so the block is properly identified and the linter error is resolved.
In `@Web/Resgrid.Web.Tts/Dockerfile`:
- Around line 24-37: The Dockerfile currently only provisions
en_US-norman-medium.onnx but AudioProcessingService.cs (and tests) reference
additional voice models (e.g., fr_FR-siwis-medium.onnx and
en_GB-semaine-medium.onnx); update the Dockerfile to download each required
.onnx and its matching .onnx.json into /usr/local/share/piper-voices (reuse the
same curl + -o pattern used for en_US-norman-medium.onnx and
en_US-norman-medium.onnx.json, and keep the ARG PIPER_VERSION-based steps
unchanged), or alternatively remove/limit the voice entries in
AudioProcessingService.cs so only models that exist in the image are resolved.
Ensure filenames exactly match the names resolved by AudioProcessingService.cs.
---
Outside diff comments:
In `@Core/Resgrid.Services/SubscriptionsService.cs`:
- Around line 92-119: GetPlanCountsForDepartmentAsync currently makes the
RestClient call without any try/catch so network/DNS/deserialization failures
(and the long 200s MaxTimeout) will surface to callers; wrap the HTTP call and
response handling in a try/catch like GetCurrentPlanForDepartmentAsync, log the
exception (use your existing logger), and return a new DepartmentPlanCount() on
any exception; while updating, consider aligning MaxTimeout with the shorter
timeout used in GetCurrentPlanForDepartmentAsync to avoid long blocking.
---
Nitpick comments:
In `@Core/Resgrid.Services/SubscriptionsService.cs`:
- Around line 141-145: Several methods (e.g.,
GetCurrentPaymentForDepartmentAsync,
GetPreviousNonFreePaymentForDepartmentAsync,
GetUpcomingPaymentForDepartmentAsync, GetPlanByIdAsync, GetPlanByExternalIdAsync
and other similar return sites) currently return response.Data.Data without
guarding against response.Data.Data being null; add the same null-guard pattern
used earlier (check response == null || response.Data == null ||
response.Data.Data == null) before returning so these methods return null when
response.Data.Data is null to avoid NPEs when callers access fields like
.PlanId.
🪄 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: 26f860c2-9576-4175-bbc8-a8fc32b72a1f
📒 Files selected for processing (15)
CLAUDE.mdCore/Resgrid.Config/TtsConfig.csCore/Resgrid.Services/LimitsService.csCore/Resgrid.Services/SubscriptionsService.csTests/Resgrid.Tests/Web/Services/TwilioControllerVoiceVerificationTests.csTests/Resgrid.Tests/Web/Tts/TtsServiceTests.csWeb/Resgrid.Web.Tts/Configuration/ServiceCollectionExtensions.csWeb/Resgrid.Web.Tts/Configuration/TtsOptions.csWeb/Resgrid.Web.Tts/DockerfileWeb/Resgrid.Web.Tts/Health/TtsDependencyHealthCheck.csWeb/Resgrid.Web.Tts/Resgrid.Web.Tts.csprojWeb/Resgrid.Web.Tts/Services/AudioProcessingService.csWeb/Resgrid.Web.Tts/Services/ITextPreprocessor.csWeb/Resgrid.Web.Tts/Services/TextPreprocessor.csWeb/Resgrid.Web.Tts/k8s/deployment.yaml
Comment on lines
+9
to
+53
| ``` | ||
| Resgrid.sln | ||
| ├── Web/ # ASP.NET web apps | ||
| │ ├── Resgrid.Web/ # Main MVC web application | ||
| │ ├── Resgrid.Web.Services/ # REST API (v4 controllers) | ||
| │ ├── Resgrid.Web.Eventing/ # Webhook/event endpoint | ||
| │ ├── Resgrid.Web.Mcp/ # MCP endpoint | ||
| │ └── Resgrid.Web.Tts/ # Text-to-speech | ||
| ├── Core/ # Core business logic | ||
| │ ├── Resgrid.Config/ # Static config classes (one per domain) | ||
| │ ├── Resgrid.Framework/ # Utilities: Logging, Serialization, Hashing | ||
| │ ├── Resgrid.Localization/ # Localization strings | ||
| │ ├── Resgrid.Model/ # Entities, enums, interfaces (Services, Repositories, Providers) | ||
| │ └── Resgrid.Services/ # Service implementations | ||
| ├── Repositories/ # Data access | ||
| │ ├── Resgrid.Repositories.DataRepository/ # SQL Server / Dapper | ||
| │ └── Resgrid.Repositories.NoSqlRepository/ # MongoDB | ||
| ├── Providers/ # Infrastructure implementations | ||
| │ ├── Resgrid.Providers.Cache/ # Redis caching (AzureRedisCacheProvider) | ||
| │ ├── Resgrid.Providers.Bus/ # Azure Service Bus | ||
| │ ├── Resgrid.Providers.Bus.Rabbit/ # RabbitMQ alternative | ||
| │ ├── Resgrid.Providers.Email/ # Email delivery | ||
| │ ├── Resgrid.Providers.Geo/ # Geolocation | ||
| │ ├── Resgrid.Providers.Marketing/ # Marketing/CRM | ||
| │ ├── Resgrid.Providers.Messaging/ # Push notifications | ||
| │ ├── Resgrid.Providers.Migrations/ # SQL Server migrations | ||
| │ ├── Resgrid.Providers.MigrationsPg/# PostgreSQL migrations | ||
| │ ├── Resgrid.Providers.Number/ # Phone number provisioning | ||
| │ ├── Resgrid.Providers.Pdf/ # PDF generation | ||
| │ ├── Resgrid.Providers.Voip/ # VoIP/SIP | ||
| │ ├── Resgrid.Providers.Weather/ # Weather data | ||
| │ ├── Resgrid.Providers.Workflow/ # Workflow execution | ||
| │ ├── Resgrid.Providers.Claims/ # Custom auth claims | ||
| │ └── Resgrid.Providers.AddressVerification/ | ||
| ├── Workers/ # Background job processing | ||
| │ ├── Resgrid.Workers.Framework/ # Worker logic + Bootstrapper | ||
| │ ├── Resgrid.Workers.Console/ # Worker host (console app) | ||
| │ └── Support/Quidjibo.Postgres/ # Queue backend for PostgreSQL | ||
| ├── Tests/ # Test projects | ||
| │ ├── Resgrid.Tests/ | ||
| │ ├── Resgrid.SmokeTests/ | ||
| │ └── Resgrid.Intergration.Tests/ | ||
| └── Tools/ | ||
| └── Resgrid.Console/ # Admin CLI tools | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language identifier to this fenced code block.
This block triggers markdownlint MD040. Use text (or bash/plaintext as appropriate) after the opening fence.
Suggested fix
-```
+```text
Resgrid.sln
├── Web/ # ASP.NET web apps
...
└── Tools/
└── Resgrid.Console/ # Admin CLI tools</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 9 - 53, The fenced code block in CLAUDE.md is missing
a language identifier, triggering markdownlint MD040; update the opening
triple-backtick for the directory tree to include a language (e.g., add "text"
so it reads ```text) so the block is properly identified and the linter error is
resolved.
| ├── Tests/ # Test projects | ||
| │ ├── Resgrid.Tests/ | ||
| │ ├── Resgrid.SmokeTests/ | ||
| │ └── Resgrid.Intergration.Tests/ |
Contributor
There was a problem hiding this comment.
Fix typo in test project name.
Resgrid.Intergration.Tests appears misspelled; should be Resgrid.Integration.Tests (assuming standard naming).
Suggested fix
-│ └── Resgrid.Intergration.Tests/
+│ └── Resgrid.Integration.Tests/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| │ └── Resgrid.Intergration.Tests/ | |
| │ └── Resgrid.Integration.Tests/ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 50, Update the misspelled test project name entry in
CLAUDE.md: replace the string "Resgrid.Intergration.Tests/" with the correct
"Resgrid.Integration.Tests/" so the project name is spelled consistently (look
for the line containing Resgrid.Intergration.Tests/ in CLAUDE.md and correct the
typo).
Comment on lines
+69
to
+71
| ``` | ||
| Config → Model → Services → Repositories/Providers → Web/Workers | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language identifier to this fenced code block.
This also triggers markdownlint MD040; set a language for the architecture diagram block.
Suggested fix
-```
+```text
Config → Model → Services → Repositories/Providers → Web/Workers</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 69 - 71, Update the fenced code block that contains
the architecture line "Config → Model → Services → Repositories/Providers
→ Web/Workers" to include a language identifier (e.g., change ``` to ```text)
so the block is flagged as plain text and satisfies markdownlint MD040.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Core/Resgrid.Services/WeatherAlertService.cs (1)
379-419:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't mark the alert notified after a partial send failure.
A single
SendMessageAsyncexception stops the loop, skips the remaining recipients, and the code still setsNotificationSent = trueafterward. That turns transient per-user failures into permanent missed notifications.Suggested fix
- try - { - var members = await _departmentsService.GetAllMembersForDepartmentAsync(departmentId); - if (members != null && members.Any()) - { - // Use department managing user as sender for notifications - var senderId = department?.ManagingUserId ?? members.First().UserId; - - var subject = FormatAlertSubject(alert); - var body = FormatAlertMessageBody(alert, department); - - foreach (var member in members) - { - if (member.UserId == senderId || member.IsDisabled.GetValueOrDefault() || member.IsDeleted) - continue; - - var notifyMsg = new Message - { - Subject = subject, - Body = body, - SendingUserId = senderId, - ReceivingUserId = member.UserId, - SentOn = DateTime.UtcNow, - SystemGenerated = true, - IsBroadcast = true, - Type = 0 - }; - - await _communicationService.SendMessageAsync(notifyMsg, "Weather Alert System", null, departmentId, null, department); - } - } - } - catch (Exception ex) - { - Logging.LogException(ex); - } + var sendSucceeded = true; + try + { + var members = await _departmentsService.GetAllMembersForDepartmentAsync(departmentId); + if (members != null && members.Any()) + { + var senderId = department?.ManagingUserId ?? members.First().UserId; + var subject = FormatAlertSubject(alert); + var body = FormatAlertMessageBody(alert, department); + + foreach (var member in members) + { + if (member.UserId == senderId || member.IsDisabled.GetValueOrDefault() || member.IsDeleted) + continue; + + try + { + var notifyMsg = new Message + { + Subject = subject, + Body = body, + SendingUserId = senderId, + ReceivingUserId = member.UserId, + SentOn = DateTime.UtcNow, + SystemGenerated = true, + IsBroadcast = true, + Type = 0 + }; + + await _communicationService.SendMessageAsync(notifyMsg, "Weather Alert System", null, departmentId, null, department); + } + catch (Exception ex) + { + sendSucceeded = false; + Logging.LogException(ex); + } + } + } + } + catch (Exception ex) + { + sendSucceeded = false; + Logging.LogException(ex); + } + + if (!sendSucceeded) + continue;🤖 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/WeatherAlertService.cs` around lines 379 - 419, The current try/catch around the entire member-send block causes a single SendMessageAsync exception to abort sending to remaining recipients while still setting alert.NotificationSent = true; change this so failures are handled per-recipient: inside the WeatherAlertService method that calls _departmentsService.GetAllMembersForDepartmentAsync and loops members, introduce a bool allSent = true; move the try/catch into the foreach around the call to _communicationService.SendMessageAsync so each SendMessageAsync error is caught, logged (Logging.LogException(ex)), sets allSent = false and continues to the next member; after the loop set alert.NotificationSent = allSent (do not mark true on partial failure).
🧹 Nitpick comments (3)
Tests/Resgrid.Tests/Models/TimeZoneTests.cs (1)
45-57: ⚡ Quick winConsider a platform-conditional lower-bound assertion to keep the test meaningful.
Currently the test passes unconditionally regardless of how many zones failed to resolve — including the case where every zone is unresolvable (e.g., a minimal Linux container without
tzdata). Adding a guard for the Windows case ensures the test still enforces something concrete:♻️ Suggested improvement
if (unresolvedZones.Count > 0) { TestContext.WriteLine( $"The following {unresolvedZones.Count} timezone(s) could not be resolved on this platform: " + string.Join(", ", unresolvedZones)); } +// On Windows, all configured timezones must be resolvable. +if (System.Runtime.InteropServices.RuntimeInformation.IsOSPlatform( + System.Runtime.InteropServices.OSPlatform.Windows)) +{ + unresolvedZones.Should().BeEmpty( + "all configured timezone IDs should resolve on Windows"); +}🤖 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 `@Tests/Resgrid.Tests/Models/TimeZoneTests.cs` around lines 45 - 57, Add a platform-conditional lower-bound assertion so the test fails if every timezone is unresolvable on Windows: after the TestContext.WriteLine block, detect Windows (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) and assert that unresolvedZones.Count is less than the total number of zones (or not equal to the total) so the test enforces at least one resolvable zone; reference unresolvedZones and the existing TestContext.WriteLine block when adding the conditional assertion.Tests/Resgrid.Tests/Services/ActionLogsServiceTests.cs (1)
60-67: ⚡ Quick winMake the fake repository update existing IDs instead of always appending.
SaveOrUpdateAsynccurrently adds the log every time, so any update path will create duplicates and make later retrieval/deletion behavior diverge from the real repository.Suggested fix
_actionLogsRepositoryMock.Setup(m => m.SaveOrUpdateAsync(It.IsAny<ActionLog>(), It.IsAny<CancellationToken>(), It.IsAny<bool>())) .ReturnsAsync((ActionLog al, CancellationToken ct, bool firstLevel) => { if (al.ActionLogId <= 0) - al.ActionLogId = _nextLogId++; - _savedLogs.Add(al); + { + al.ActionLogId = _nextLogId++; + _savedLogs.Add(al); + } + else + { + var index = _savedLogs.FindIndex(x => x.ActionLogId == al.ActionLogId); + if (index >= 0) + _savedLogs[index] = al; + else + _savedLogs.Add(al); + } return al; });🤖 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 `@Tests/Resgrid.Tests/Services/ActionLogsServiceTests.cs` around lines 60 - 67, The fake repository's SaveOrUpdateAsync currently always appends, causing duplicates; modify the Setup lambda for SaveOrUpdateAsync so that if al.ActionLogId <= 0 it assigns a new id (using _nextLogId++) and adds to _savedLogs, else locate an existing entry in _savedLogs by ActionLogId and update/replace that entry (or add if not found), then return the updated ActionLog; reference SaveOrUpdateAsync, _savedLogs, ActionLog.ActionLogId and _nextLogId when making this change.Tests/Resgrid.Tests/Services/WeatherAlertServiceTests.cs (1)
30-62: ⚡ Quick winAdd coverage for the new per-recipient notification flow.
This fixture only tracks the constructor swap. The behavior change in this PR is that
SendPendingNotificationsAsyncnow sends viaICommunicationServiceonce per eligible member, so this file should verify the call count and the skip rules for sender/disabled/deleted users.🤖 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 `@Tests/Resgrid.Tests/Services/WeatherAlertServiceTests.cs` around lines 30 - 62, The test fixture in WeatherAlertServiceTests doesn't assert the new per-recipient notification behavior introduced in WeatherAlertService.SendPendingNotificationsAsync; update the tests to exercise SendPendingNotificationsAsync and verify that ICommunicationService.SendNotificationAsync (or the relevant method on ICommunicationService) is invoked once per eligible member and skipped for sender/disabled/deleted users. Specifically, use the existing _communicationServiceMock, arrange department members with varying states (normal, sender, disabled, deleted), set up pending notifications in the _weatherAlertRepoMock/_weatherAlertSourceRepoMock as needed, call _weatherAlertService.SendPendingNotificationsAsync, and add assertions on _communicationServiceMock.Verify call counts and that no calls were made for sender/disabled/deleted members. Ensure test names cover call count and skip rules.
🤖 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 `@Tests/Resgrid.Tests/Services/ActionLogsServiceTests.cs`:
- Around line 78-80: The mock for GetLastActionLogsForUserAsync ignores the
cutoff parameter `time`; update the lambda in _actionLogsRepositoryMock.Setup to
filter _savedLogs by both matching UserId and by the timestamp (e.g.,
l.CreatedOnUtc >= time or > time depending on test expectation) before ordering
by ActionLogId and returning FirstOrDefault(), so the mock honors the cutoff
timestamp passed into GetLastActionLogsForUserAsync.
In `@Tests/Resgrid.Tests/Services/AuthorizationServiceTests.cs`:
- Around line 316-347: The fixture is still hitting the message-auth path
because _messageServiceMock.GetMessageByIdAsync(1) is returning a Message; to
exercise the work-log auth cases for CanUserViewAndEditWorkLogAsync, change the
message mock so it does not claim the same id (either return null for id 1 or
move the message setup to a different id). Update _messageServiceMock.Setup(m =>
m.GetMessageByIdAsync(1)).ReturnsAsync((Message)null) or use a different
MessageId there, leaving _workLogsServiceMock.GetWorkLogByIdAsync(1) returning
the workLog so the work-log authorization branch
(CanUserViewAndEditWorkLogAsync) is executed.
---
Outside diff comments:
In `@Core/Resgrid.Services/WeatherAlertService.cs`:
- Around line 379-419: The current try/catch around the entire member-send block
causes a single SendMessageAsync exception to abort sending to remaining
recipients while still setting alert.NotificationSent = true; change this so
failures are handled per-recipient: inside the WeatherAlertService method that
calls _departmentsService.GetAllMembersForDepartmentAsync and loops members,
introduce a bool allSent = true; move the try/catch into the foreach around the
call to _communicationService.SendMessageAsync so each SendMessageAsync error is
caught, logged (Logging.LogException(ex)), sets allSent = false and continues to
the next member; after the loop set alert.NotificationSent = allSent (do not
mark true on partial failure).
---
Nitpick comments:
In `@Tests/Resgrid.Tests/Models/TimeZoneTests.cs`:
- Around line 45-57: Add a platform-conditional lower-bound assertion so the
test fails if every timezone is unresolvable on Windows: after the
TestContext.WriteLine block, detect Windows
(RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) and assert that
unresolvedZones.Count is less than the total number of zones (or not equal to
the total) so the test enforces at least one resolvable zone; reference
unresolvedZones and the existing TestContext.WriteLine block when adding the
conditional assertion.
In `@Tests/Resgrid.Tests/Services/ActionLogsServiceTests.cs`:
- Around line 60-67: The fake repository's SaveOrUpdateAsync currently always
appends, causing duplicates; modify the Setup lambda for SaveOrUpdateAsync so
that if al.ActionLogId <= 0 it assigns a new id (using _nextLogId++) and adds to
_savedLogs, else locate an existing entry in _savedLogs by ActionLogId and
update/replace that entry (or add if not found), then return the updated
ActionLog; reference SaveOrUpdateAsync, _savedLogs, ActionLog.ActionLogId and
_nextLogId when making this change.
In `@Tests/Resgrid.Tests/Services/WeatherAlertServiceTests.cs`:
- Around line 30-62: The test fixture in WeatherAlertServiceTests doesn't assert
the new per-recipient notification behavior introduced in
WeatherAlertService.SendPendingNotificationsAsync; update the tests to exercise
SendPendingNotificationsAsync and verify that
ICommunicationService.SendNotificationAsync (or the relevant method on
ICommunicationService) is invoked once per eligible member and skipped for
sender/disabled/deleted users. Specifically, use the existing
_communicationServiceMock, arrange department members with varying states
(normal, sender, disabled, deleted), set up pending notifications in the
_weatherAlertRepoMock/_weatherAlertSourceRepoMock as needed, call
_weatherAlertService.SendPendingNotificationsAsync, and add assertions on
_communicationServiceMock.Verify call counts and that no calls were made for
sender/disabled/deleted members. Ensure test names cover call count and skip
rules.
🪄 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: 0ca78753-cabb-4259-8a96-c00ef9624139
📒 Files selected for processing (9)
Core/Resgrid.Services/WeatherAlertService.csTests/Resgrid.Tests/Models/TimeZoneTests.csTests/Resgrid.Tests/Services/ActionLogsServiceTests.csTests/Resgrid.Tests/Services/AuthorizationServiceTests.csTests/Resgrid.Tests/Services/WeatherAlertServiceTests.csTests/Resgrid.Tests/Web/Tts/TtsServiceTests.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web.Tts/DockerfileWeb/Resgrid.Web.Tts/Services/AudioProcessingService.cs
✅ Files skipped from review due to trivial changes (2)
- Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs
- Web/Resgrid.Web.Services/Resgrid.Web.Services.xml
🚧 Files skipped from review as they are similar to previous changes (1)
- Web/Resgrid.Web.Tts/Dockerfile
Comment on lines
+78
to
+80
| _actionLogsRepositoryMock.Setup(m => m.GetLastActionLogsForUserAsync(It.IsAny<string>(), It.IsAny<bool>(), It.IsAny<DateTime>())) | ||
| .ReturnsAsync((string userId, bool disableAuto, DateTime time) => | ||
| _savedLogs.Where(l => l.UserId == userId).OrderByDescending(l => l.ActionLogId).FirstOrDefault()); |
Contributor
There was a problem hiding this comment.
Honor the cutoff timestamp in the mock.
This setup ignores time, so should_get_no_log_after_hour below can only ever pass and no longer proves the one-hour filter works.
Suggested fix
_actionLogsRepositoryMock.Setup(m => m.GetLastActionLogsForUserAsync(It.IsAny<string>(), It.IsAny<bool>(), It.IsAny<DateTime>()))
.ReturnsAsync((string userId, bool disableAuto, DateTime time) =>
- _savedLogs.Where(l => l.UserId == userId).OrderByDescending(l => l.ActionLogId).FirstOrDefault());
+ _savedLogs
+ .Where(l => l.UserId == userId && l.Timestamp >= time)
+ .OrderByDescending(l => l.ActionLogId)
+ .FirstOrDefault());🤖 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 `@Tests/Resgrid.Tests/Services/ActionLogsServiceTests.cs` around lines 78 - 80,
The mock for GetLastActionLogsForUserAsync ignores the cutoff parameter `time`;
update the lambda in _actionLogsRepositoryMock.Setup to filter _savedLogs by
both matching UserId and by the timestamp (e.g., l.CreatedOnUtc >= time or >
time depending on test expectation) before ordering by ActionLogId and returning
FirstOrDefault(), so the mock honors the cutoff timestamp passed into
GetLastActionLogsForUserAsync.
Comment on lines
+316
to
+347
| [SetUp] | ||
| public void Setup() | ||
| { | ||
| // TestUser1Id = managing user and admin | ||
| // TestUser4Id = regular user not an admin | ||
| var dept1 = CreateDepartmentWithAdmins(1, TestData.Users.TestUser1Id, TestData.Users.TestUser1Id); | ||
| _departmentsServiceMock.Setup(m => m.GetDepartmentByUserIdAsync(TestData.Users.TestUser1Id, It.IsAny<bool>())).ReturnsAsync(dept1); | ||
| _departmentsServiceMock.Setup(m => m.GetDepartmentByUserIdAsync(TestData.Users.TestUser4Id, It.IsAny<bool>())).ReturnsAsync(dept1); | ||
|
|
||
| // WorkLog 1 belongs to department 1, logged by TestUser1Id | ||
| var workLog = new Log | ||
| { | ||
| LogId = 1, | ||
| DepartmentId = 1, | ||
| LoggedByUserId = TestData.Users.TestUser1Id, | ||
| Users = new System.Collections.Generic.List<LogUser>() | ||
| }; | ||
| _workLogsServiceMock.Setup(m => m.GetWorkLogByIdAsync(1)).ReturnsAsync(workLog); | ||
| _departmentsServiceMock.Setup(m => m.GetDepartmentByIdAsync(1, It.IsAny<bool>())).ReturnsAsync(dept1); | ||
|
|
||
| // Message setups for the view message tests | ||
| _messageServiceMock.Setup(m => m.GetMessageByIdAsync(1)) | ||
| .ReturnsAsync(new Message | ||
| { | ||
| MessageId = 1, | ||
| SendingUserId = TestData.Users.TestUser1Id, | ||
| MessageRecipients = new System.Collections.Generic.List<MessageRecipient> | ||
| { | ||
| new MessageRecipient { UserId = TestData.Users.TestUser2Id } | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
This fixture is still wired for message auth, not work-log auth.
The added setup for GetMessageByIdAsync keeps the last two cases on the message path, so this fixture still doesn't cover the creator/non-admin scenarios for CanUserViewAndEditWorkLogAsync.
🤖 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 `@Tests/Resgrid.Tests/Services/AuthorizationServiceTests.cs` around lines 316 -
347, The fixture is still hitting the message-auth path because
_messageServiceMock.GetMessageByIdAsync(1) is returning a Message; to exercise
the work-log auth cases for CanUserViewAndEditWorkLogAsync, change the message
mock so it does not claim the same id (either return null for id 1 or move the
message setup to a different id). Update _messageServiceMock.Setup(m =>
m.GetMessageByIdAsync(1)).ReturnsAsync((Message)null) or use a different
MessageId there, leaving _workLogsServiceMock.GetWorkLogByIdAsync(1) returning
the workLog so the work-log authorization branch
(CanUserViewAndEditWorkLogAsync) is executed.
Member
Author
|
Approve |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Behavioral Changes
Documentation
Tests