Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds department-member validation to messaging paths (skipping missing/disabled/deleted members and injecting IDepartmentsService), tightens SendMessage permission checks for system messages, refactors TTS audio profile/filtering and cache key derivation, improves S3 metadata error handling with presigned-HEAD fallback, updates tests and expands API XML docs. ChangesMessaging / Recipient Validation
TTS, S3 Metadata Handling, Tests & Docs
Sequence Diagram(s)sequenceDiagram
participant Controller
participant CommunicationService
participant DepartmentsService
participant RecipientRecord
participant NotificationProvider
Controller->>CommunicationService: SendMessage(request)
CommunicationService->>DepartmentsService: GetDepartmentMemberAsync(userId, deptId)
DepartmentsService-->>CommunicationService: DepartmentMember (or null/disabled/deleted)
alt member valid
CommunicationService->>RecipientRecord: CanSendToUser -> true
CommunicationService->>NotificationProvider: Check channel flags (notification vs message) and send
NotificationProvider-->>CommunicationService: Send result
else member invalid
CommunicationService-->>Controller: skip recipient
end
sequenceDiagram
participant Client
participant TtsService
participant AudioProcessingService
participant S3StorageService
Client->>TtsService: Generate(text, voice, speed)
TtsService->>AudioProcessingService: GetEffectiveSynthesisProfile(voice,speed)
AudioProcessingService-->>TtsService: effectiveProfile
TtsService->>AudioProcessingService: GenerateNormalizedWav(text, voice, speed)
AudioProcessingService->>S3StorageService: Check/Upload audio (ExistsAsync / Put)
S3StorageService->>AudioProcessingService: metadata or presigned-HEAD fallback
AudioProcessingService-->>TtsService: audio bytes
TtsService-->>Client: response (uses requested voice/speed in response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 3 minutes and 34 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs (1)
669-689:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFilter the posted user IDs before computing update deltas.
This
continueonly prevents adding a newCallDispatch. The rest ofUpdateCallstill uses the rawdispatchingUserIdsset for removals and fornewUserIds, so a disabled/deleted user can remain on the call if they were already dispatched, and a newly selected disabled/deleted user can still flow into the later rebroadcast profile lookup. Build one validated user-id set first, then use that same set for removals, additions, andnewUserIds.Suggested direction
+ var validDispatchingUserIds = new List<string>(); + foreach (var userId in dispatchingUserIds) + { + var member = await _departmentsService.GetDepartmentMemberAsync(userId, DepartmentId, false); + if (member == null || member.IsDisabled.GetValueOrDefault() || member.IsDeleted) + continue; + + validDispatchingUserIds.Add(userId); + + if (!call.Dispatches.Any(x => x.UserId == userId)) + { + call.Dispatches.Add(new CallDispatch { CallId = call.CallId, UserId = userId }); + } + } - var dispatchesToRemove = call.Dispatches.Select(x => x.UserId).Where(y => !dispatchingUserIds.Contains(y)).ToList(); + var dispatchesToRemove = call.Dispatches.Select(x => x.UserId).Where(y => !validDispatchingUserIds.Contains(y)).ToList(); ... - var newUserIds = dispatchingUserIds.Where(id => !existingDispatches.Any(d => d.UserId == id)).ToList(); + var newUserIds = validDispatchingUserIds.Where(id => !existingDispatches.Any(d => d.UserId == id)).ToList();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs` around lines 669 - 689, The code uses the raw dispatchingUserIds when computing removals and newUserIds but only skips adding invalid members during the add loop; validate/filter dispatchingUserIds first by calling _departmentsService.GetDepartmentMemberAsync for each id and excluding null/IsDisabled/IsDeleted entries, produce a validatedDispatchingUserIds set, then use that set (not the original dispatchingUserIds) for computing dispatchesToRemove, additions (CallDispatch creation), and newUserIds inside UpdateCall so disabled/deleted users are neither re-added nor included in rebroadcast logic.Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs (1)
303-331:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the same active-member filter to group and role recipients.
Right now the new check only protects
"Everyone"and explicit personnel. Members added through groups or roles still pass on anydepartmentUsersmatch, so disabled/deleted users can still receive the message indirectly. Precompute the active department user ids once and use that predicate in every recipient branch.Suggested direction
+ var activeUserIds = departmentUsers + .Where(x => !x.IsDisabled.GetValueOrDefault() && !x.IsDeleted) + .Select(x => x.UserId) + .ToHashSet(); if (newMessageInput.Recipients.Any(x => x.Name == "Everyone")) { - foreach (var departmentMember in departmentUsers) + foreach (var userId in activeUserIds) { - if (departmentMember.IsDisabled.GetValueOrDefault() || departmentMember.IsDeleted) - continue; - - message.AddRecipient(departmentMember.UserId); + message.AddRecipient(userId); } } ... - var dm = departmentUsers.FirstOrDefault(x => x.UserId == userIdToSendTo); - if (dm != null && !dm.IsDisabled.GetValueOrDefault() && !dm.IsDeleted) + if (activeUserIds.Contains(userIdToSendTo)) ... - if (departmentUsers.Any(x => x.UserId == member.UserId)) + if (activeUserIds.Contains(member.UserId)) ... - if (departmentUsers.Any(x => x.UserId == member.UserId)) + if (activeUserIds.Contains(member.UserId)) ...Also applies to: 337-397
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs` around lines 303 - 331, The recipient logic currently filters disabled/deleted users only for the "Everyone" branch and explicit person recipients (in MessagesController processing newMessageInput), but not for group or role branches; precompute a set/list of active department user ids from departmentUsers (e.g., activeDepartmentUserIds = departmentUsers.Where(d => !d.IsDisabled.GetValueOrDefault() && !d.IsDeleted).Select(d => d.UserId)) and use that predicate everywhere you add recipients (where message.AddRecipient, usersToSendTo are updated) so group- and role-based recipients are only added if they exist in activeDepartmentUserIds and are not the sender and not already in usersToSendTo.Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs (1)
143-178:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe disabled/deleted-member filter is still bypassed through groups and roles.
This new check only covers ids posted in
users. TheSendToMatchOnlypath and the group/role loops below still add recipients without validating the department member state, so the same disabled/deleted user can still be messaged indirectly. Reuse the same membership predicate before everyAddRecipient.Also applies to: 181-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs` around lines 143 - 178, The SendToMatchOnly branch (involving usersInRoles, GetAllMembersOfRoleAsync and GetAllMembersForGroupAsync) currently adds recipients from group/role members without applying the same department membership/active-state predicate used earlier for posted `users`; update the loops that call model.Message.AddRecipient(member.UserId) to first run the same membership validation (the existing predicate used for direct `users` adds—e.g., check member.UserId != UserId, not in excludedUsers, and that the member is not disabled/deleted) and only call Message.AddRecipient when that predicate passes; apply the same change to the similar code block mentioned for lines 181-212 so all indirect additions reuse the membership predicate.
🧹 Nitpick comments (2)
Tests/Resgrid.Tests/Web/Tts/TtsServiceTests.cs (1)
223-304: ⚡ Quick winExtract these command builders instead of testing private methods via reflection.
This fixture is tightly coupled to method names/signatures (
"CreateEspeakStartInfo","CreateFfmpegStartInfo"), so routine refactors will break tests even when behavior stays correct. Since these builders are already pure, consider moving them to aninternalcollaborator or exposing them withInternalsVisibleToand testing that seam directly. As per coding guidelines, "Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Web/Tts/TtsServiceTests.cs` around lines 223 - 304, The tests in AudioProcessingServiceTests are using reflection to call private methods CreateEspeakStartInfo and CreateFfmpegStartInfo via InvokePrivateMethod which couples tests to implementation; refactor by extracting those command-builder functions into a dedicated internal collaborator (e.g., EspeakStartInfoBuilder and FfmpegStartInfoBuilder) or make the builder methods internal and add InternalsVisibleTo for the test assembly, update AudioProcessingService to use the new collaborator or internal methods, and change tests to instantiate and assert on those builders directly (remove InvokePrivateMethod and reflection-based calls).Tests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.cs (1)
126-158: ⚡ Quick winMissing counterpart test: raw
FormatException+ HEAD 404 →falseThe new test only covers the "raw
FormatException→ HEAD 200 →true" path. The existing tests forAmazonUnmarshallingExceptioncover both directions (lines 61–92 for 200 →true, lines 94–124 for 404 →false). A symmetric test for the rawFormatExceptioncase would ensure thereturn falsebranch on page line 353–354 of the production code is exercised.✅ Suggested additional test
+[Test] +public async Task exists_async_should_return_false_when_presigned_head_reports_missing_after_raw_format_exception() +{ + var s3Client = new Mock<IAmazonS3>(MockBehavior.Strict); + s3Client + .Setup(x => x.GetObjectMetadataAsync(It.IsAny<GetObjectMetadataRequest>(), It.IsAny<CancellationToken>())) + .ThrowsAsync(new FormatException("bad metadata expiration header")); + s3Client + .Setup(x => x.GetPreSignedURL(It.IsAny<GetPreSignedUrlRequest>())) + .Returns<GetPreSignedUrlRequest>(request => + { + request.Verb.Should().Be(HttpVerb.HEAD); + return "http://download.example.com/tts/audio.wav?signature=head-raw-format-missing"; + }); + + var handler = new RecordingHttpMessageHandler((request, _) => + Task.FromResult(new HttpResponseMessage(HttpStatusCode.NotFound))); + var service = CreateService(s3Client.Object, handler, useSsl: false); + + var exists = await service.ExistsAsync("tts/audio.wav", CancellationToken.None); + + exists.Should().BeFalse(); + handler.Requests.Should().HaveCount(1); + s3Client.Verify(x => x.GetPreSignedURL(It.IsAny<GetPreSignedUrlRequest>()), Times.Once); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.cs` around lines 126 - 158, Add a symmetric test to cover the raw FormatException + HEAD 404 → false path: create a new async test (mirror exists_async_should_verify_with_presigned_head_when_metadata_throws_raw_format_exception) that sets up the mocked IAmazonS3.GetObjectMetadataAsync to throw FormatException, mocks GetPreSignedURL to return the same pre-signed HEAD URL, uses RecordingHttpMessageHandler to return HttpStatusCode.NotFound, calls service.ExistsAsync("tts/audio.wav", ...), asserts the result is false, verifies GetObjectMetadataAsync was called once and GetPreSignedURL was called once (use the same request predicates as the existing test); use CreateService and RecordingHttpMessageHandler so the test structure matches the original.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 7841-7849: The XML docs for
Resgrid.Web.Services.Models.v4.PersonnelStaffing.SavePersonnelStaffingInput
wrongly reference "Unit" and "UnitStateType"; update the <summary> text for the
SavePersonnelStaffingInput properties (notably the UserId and Type members) to
correctly describe personnel/staffing semantics (e.g., UserId identifies the
personnel being staffed and Type describes the staffing state/type), and make
the same corrections for the other duplicated entries (the similar block around
the SavePersonnelStaffingInput members at lines ~7881-7884) so all
Staffing-related summaries refer to personnel/staffing rather than units/status.
- Around line 7531-7534: Replace the placeholder "<summary>Result containing all
the data required to populate the New Call form</summary>" with
endpoint-accurate text for the affected XML docs; locate the
Resgrid.Web.Services.Models.v4 types such as
T:Resgrid.Web.Services.Models.v4.Personnel.GetAllPersonnelInfosResult and the
Protocol result wrapper types mentioned, and update each <summary> to briefly
describe what the endpoint actually returns (e.g., "Result containing personnel
information for endpoint X" or "Result containing protocol definitions for
endpoint Y"), ensuring each summary uniquely names the endpoint/payload and
purpose so generated API docs are accurate.
- Around line 7731-7744: The XML docs incorrectly refer to "unit location" and
"unit's location" for the PersonnelLocation models; update the summaries for the
type Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult,
its property PersonnelLocationResult.Data, and the type
PersonnelLocationResultData to reference "personnel" or "personnel location"
(e.g., "A personnel location in the Resgrid system", "Response data for a
personnel location", "The information about a specific personnel location") so
the wording matches the PersonnelLocation* model names.
In `@Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs`:
- Around line 83-89: The cache key is still derived from the original requested
voice/speed while GetEspeakInvocation normalizes English requests to
MbrolaEnglishVoice/MbrolaEnglishSpeed, causing identical outputs to have
different keys; to fix, change the caller to derive the cache key from the
effective synthesis profile (either call GetEspeakInvocation before
CreateCacheKey and use the returned
EspeakInvocation.voice/EspeakInvocation.speed for the key, or modify
GetEspeakInvocation to return the normalized voice/speed tuple and use those
values when building the cache key in CreateCacheKey) so the cache key reflects
the actual synthesis settings.
In `@Web/Resgrid.Web.Tts/Services/S3StorageService.cs`:
- Around line 59-62: WasUploadPersistedAsync's catch(FormatException) is now
dead because ExistsAsync swallows FormatException from GetObjectMetadataAsync;
either remove that obsolete catch in WasUploadPersistedAsync, or restore the
previous defensive behavior by having WasUploadPersistedAsync call a variant of
ExistsAsync that does not absorb FormatException (or change
ExistsAsync/GetObjectMetadataAsync to rethrow or propagate a distinct exception
on HEAD failure) so WasUploadPersistedAsync can still return false and trigger a
presigned PUT retry; update the code paths around WasUploadPersistedAsync,
ExistsAsync and GetObjectMetadataAsync accordingly (or remove the catch in
WasUploadPersistedAsync if the new optimistic behavior is intentional).
---
Outside diff comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/MessagesController.cs`:
- Around line 303-331: The recipient logic currently filters disabled/deleted
users only for the "Everyone" branch and explicit person recipients (in
MessagesController processing newMessageInput), but not for group or role
branches; precompute a set/list of active department user ids from
departmentUsers (e.g., activeDepartmentUserIds = departmentUsers.Where(d =>
!d.IsDisabled.GetValueOrDefault() && !d.IsDeleted).Select(d => d.UserId)) and
use that predicate everywhere you add recipients (where message.AddRecipient,
usersToSendTo are updated) so group- and role-based recipients are only added if
they exist in activeDepartmentUserIds and are not the sender and not already in
usersToSendTo.
In `@Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs`:
- Around line 669-689: The code uses the raw dispatchingUserIds when computing
removals and newUserIds but only skips adding invalid members during the add
loop; validate/filter dispatchingUserIds first by calling
_departmentsService.GetDepartmentMemberAsync for each id and excluding
null/IsDisabled/IsDeleted entries, produce a validatedDispatchingUserIds set,
then use that set (not the original dispatchingUserIds) for computing
dispatchesToRemove, additions (CallDispatch creation), and newUserIds inside
UpdateCall so disabled/deleted users are neither re-added nor included in
rebroadcast logic.
In `@Web/Resgrid.Web/Areas/User/Controllers/MessagesController.cs`:
- Around line 143-178: The SendToMatchOnly branch (involving usersInRoles,
GetAllMembersOfRoleAsync and GetAllMembersForGroupAsync) currently adds
recipients from group/role members without applying the same department
membership/active-state predicate used earlier for posted `users`; update the
loops that call model.Message.AddRecipient(member.UserId) to first run the same
membership validation (the existing predicate used for direct `users` adds—e.g.,
check member.UserId != UserId, not in excludedUsers, and that the member is not
disabled/deleted) and only call Message.AddRecipient when that predicate passes;
apply the same change to the similar code block mentioned for lines 181-212 so
all indirect additions reuse the membership predicate.
---
Nitpick comments:
In `@Tests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.cs`:
- Around line 126-158: Add a symmetric test to cover the raw FormatException +
HEAD 404 → false path: create a new async test (mirror
exists_async_should_verify_with_presigned_head_when_metadata_throws_raw_format_exception)
that sets up the mocked IAmazonS3.GetObjectMetadataAsync to throw
FormatException, mocks GetPreSignedURL to return the same pre-signed HEAD URL,
uses RecordingHttpMessageHandler to return HttpStatusCode.NotFound, calls
service.ExistsAsync("tts/audio.wav", ...), asserts the result is false, verifies
GetObjectMetadataAsync was called once and GetPreSignedURL was called once (use
the same request predicates as the existing test); use CreateService and
RecordingHttpMessageHandler so the test structure matches the original.
In `@Tests/Resgrid.Tests/Web/Tts/TtsServiceTests.cs`:
- Around line 223-304: The tests in AudioProcessingServiceTests are using
reflection to call private methods CreateEspeakStartInfo and
CreateFfmpegStartInfo via InvokePrivateMethod which couples tests to
implementation; refactor by extracting those command-builder functions into a
dedicated internal collaborator (e.g., EspeakStartInfoBuilder and
FfmpegStartInfoBuilder) or make the builder methods internal and add
InternalsVisibleTo for the test assembly, update AudioProcessingService to use
the new collaborator or internal methods, and change tests to instantiate and
assert on those builders directly (remove InvokePrivateMethod and
reflection-based calls).
🪄 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: 22fab5f1-b73c-4a2d-b184-4421922acb32
⛔ Files ignored due to path filters (1)
Web/Resgrid.Web/Areas/User/Apps/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
Core/Resgrid.Services/CommunicationService.csCore/Resgrid.Services/WeatherAlertService.csTests/Resgrid.Tests/Web/Tts/S3StorageServiceTests.csTests/Resgrid.Tests/Web/Tts/TtsServiceTests.csWeb/Resgrid.Web.Services/Controllers/v4/MessagesController.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web.Tts/Services/AudioProcessingService.csWeb/Resgrid.Web.Tts/Services/S3StorageService.csWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Controllers/MessagesController.cs
Comment on lines
+7731
to
+7744
| <member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult"> | ||
| <summary> | ||
| A unit location in the Resgrid system | ||
| </summary> | ||
| </member> | ||
| <member name="P:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult.Data"> | ||
| <summary> | ||
| Response Data | ||
| </summary> | ||
| </member> | ||
| <member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResultData"> | ||
| <summary> | ||
| The information about a specific unit's location | ||
| </summary> |
Contributor
There was a problem hiding this comment.
Fix entity wording in PersonnelLocation result summaries.
This block describes “unit location” and “unit's location” even though the model is PersonnelLocation*.
🛠 Suggested XML doc updates
- A unit location in the Resgrid system
+ A personnel location in the Resgrid system
- The information about a specific unit's location
+ The information about a specific person's location📝 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
| <member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult"> | |
| <summary> | |
| A unit location in the Resgrid system | |
| </summary> | |
| </member> | |
| <member name="P:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult.Data"> | |
| <summary> | |
| Response Data | |
| </summary> | |
| </member> | |
| <member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResultData"> | |
| <summary> | |
| The information about a specific unit's location | |
| </summary> | |
| <member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult"> | |
| <summary> | |
| A personnel location in the Resgrid system | |
| </summary> | |
| </member> | |
| <member name="P:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult.Data"> | |
| <summary> | |
| Response Data | |
| </summary> | |
| </member> | |
| <member name="T:Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResultData"> | |
| <summary> | |
| The information about a specific person's location | |
| </summary> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 7731 - 7744,
The XML docs incorrectly refer to "unit location" and "unit's location" for the
PersonnelLocation models; update the summaries for the type
Resgrid.Web.Services.Models.v4.PersonnelLocation.PersonnelLocationResult, its
property PersonnelLocationResult.Data, and the type PersonnelLocationResultData
to reference "personnel" or "personnel location" (e.g., "A personnel location in
the Resgrid system", "Response data for a personnel location", "The information
about a specific personnel location") so the wording matches the
PersonnelLocation* model names.
Comment on lines
+7841
to
+7849
| <member name="P:Resgrid.Web.Services.Models.v4.PersonnelStaffing.SavePersonnelStaffingInput.UserId"> | ||
| <summary> | ||
| UnitId of the apparatus that the state is being set for | ||
| </summary> | ||
| </member> | ||
| <member name="P:Resgrid.Web.Services.Models.v4.PersonnelStaffing.SavePersonnelStaffingInput.Type"> | ||
| <summary> | ||
| The UnitStateType of the Unit | ||
| </summary> |
Contributor
There was a problem hiding this comment.
Correct copy-pasted unit/status wording in PersonnelStaffing docs.
SavePersonnelStaffing* docs currently refer to units and status, which is incorrect for staffing endpoints.
🛠 Suggested XML doc updates
- UnitId of the apparatus that the state is being set for
+ UserId of the person whose staffing is being set
- The UnitStateType of the Unit
+ The staffing type to set for the person
- Saves (sets) and Personnel Status in the system, for a single user
+ Saves (sets) personnel staffing in the system for multiple usersAlso applies to: 7881-7884
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 7841 - 7849,
The XML docs for
Resgrid.Web.Services.Models.v4.PersonnelStaffing.SavePersonnelStaffingInput
wrongly reference "Unit" and "UnitStateType"; update the <summary> text for the
SavePersonnelStaffingInput properties (notably the UserId and Type members) to
correctly describe personnel/staffing semantics (e.g., UserId identifies the
personnel being staffed and Type describes the staffing state/type), and make
the same corrections for the other duplicated entries (the similar block around
the SavePersonnelStaffingInput members at lines ~7881-7884) so all
Staffing-related summaries refer to personnel/staffing rather than units/status.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs (1)
120-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVersion the cache key when the synthesis pipeline changes.
This method now applies
TelephoneAudioFilterto every normalized WAV, so the output bytes change even when(text, voice, speed)stays the same.TtsService.GenerateInternalAsyncstill keys cached objects only on text/effective voice/effective speed, which means pre-change objects can keep being served after deploy and bypass this new filter until the cache is manually evicted. Please salt the cache key with a synthesis-pipeline version before merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs` around lines 120 - 137, The cache key must be versioned to reflect the new synthesis pipeline change: add a constant version/salt (e.g. SynthesisPipelineVersion) near the TtsService or a shared config and increment it for this deploy, then include that version when building the cache key inside TtsService.GenerateInternalAsync so cached entries are segregated from outputs that now apply TelephoneAudioFilter; search for CreateFfmpegStartInfo and TelephoneAudioFilter to locate the related change and update the cache-key construction in GenerateInternalAsync to concatenate or incorporate the pipeline version alongside text/effectiveVoice/effectiveSpeed.Web/Resgrid.Web.Tts/Services/TtsService.cs (1)
92-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the effective profile in the response as well.
Now that
Hash/ObjectKey/Urlare derived fromeffectiveProfile,CreateResponsestill reportsrequest.Voiceandrequest.Speedon Lines 265-266. For English inputs likeenanden-us+klatt4, callers can now get the same cached object while seeing different metadata, even though the audio was synthesized with the samemb-us1/130profile. PasseffectiveProfilethrough toCreateResponseso the response describes the audio that was actually generated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Tts/Services/TtsService.cs` around lines 92 - 99, The response currently reports request.Voice/request.Speed even when the audio was generated using effectiveProfile from GetEffectiveSynthesisProfile; update the CreateResponse usage and signature to accept the effectiveProfile (symbol: effectiveProfile) and use it when populating Hash/ObjectKey/Url and the voice/speed metadata instead of request.Voice/request.Speed (symbol: CreateResponse); change the cached branch (where cachedUrl is returned) to pass effectiveProfile into CreateResponse and update any other CreateResponse calls (non-cached path) to do the same so the response always reflects the actual synthesis profile used.
🧹 Nitpick comments (1)
Tests/Resgrid.Tests/Services/CommunicationServiceTests.cs (1)
46-49: ⚡ Quick winAdd test cases for the new rejection paths in
CanSendToUser.The mock always returns
new DepartmentMember()(both flags at their defaults), which exercises only the "happy path" throughCanSendToUser. The three rejection branches added inCommunicationService.CanSendToUser(membernull,IsDisabled = true,IsDeleted = true) have no test coverage.Consider parameterized tests (or separate
[TestFixture]subclasses) that assert the send methods are not invoked when the mockedGetDepartmentMemberAsyncreturns:
nullnew DepartmentMember { IsDisabled = true }new DepartmentMember { IsDeleted = true }+// Example skeleton for rejection-path coverage +[TestFixture] +public class when_member_is_disabled : with_the_communication_service +{ + protected override void SetupDepartmentsMock() + { + _departmentsServiceMock + .Setup(x => x.GetDepartmentMemberAsync(It.IsAny<string>(), It.IsAny<int>(), It.IsAny<bool>())) + .ReturnsAsync(new DepartmentMember { IsDisabled = true }); + } + + [Test] + public async Task should_not_send_message_to_disabled_member() + { + // arrange + act ... + _smsServiceMock.VerifyNoOtherCalls(); + _emailServiceMock.VerifyNoOtherCalls(); + _pushServiceMock.VerifyNoOtherCalls(); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Services/CommunicationServiceTests.cs` around lines 46 - 49, Add unit tests for the three rejection branches in CommunicationService.CanSendToUser by modifying CommunicationServiceTests: update or add parameterized test(s) that configure _departmentsServiceMock.Setup(x => x.GetDepartmentMemberAsync(It.IsAny<string>(), It.IsAny<int>(), It.IsAny<bool>())) to return respectively (1) null, (2) new DepartmentMember { IsDisabled = true }, and (3) new DepartmentMember { IsDeleted = true }, then assert the send methods on the service are NOT invoked for each case; reference the CanSendToUser method and the GetDepartmentMemberAsync setup to locate where to change/add tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Web/Resgrid.Web.Tts/Services/AudioProcessingService.cs`:
- Around line 120-137: The cache key must be versioned to reflect the new
synthesis pipeline change: add a constant version/salt (e.g.
SynthesisPipelineVersion) near the TtsService or a shared config and increment
it for this deploy, then include that version when building the cache key inside
TtsService.GenerateInternalAsync so cached entries are segregated from outputs
that now apply TelephoneAudioFilter; search for CreateFfmpegStartInfo and
TelephoneAudioFilter to locate the related change and update the cache-key
construction in GenerateInternalAsync to concatenate or incorporate the pipeline
version alongside text/effectiveVoice/effectiveSpeed.
In `@Web/Resgrid.Web.Tts/Services/TtsService.cs`:
- Around line 92-99: The response currently reports request.Voice/request.Speed
even when the audio was generated using effectiveProfile from
GetEffectiveSynthesisProfile; update the CreateResponse usage and signature to
accept the effectiveProfile (symbol: effectiveProfile) and use it when
populating Hash/ObjectKey/Url and the voice/speed metadata instead of
request.Voice/request.Speed (symbol: CreateResponse); change the cached branch
(where cachedUrl is returned) to pass effectiveProfile into CreateResponse and
update any other CreateResponse calls (non-cached path) to do the same so the
response always reflects the actual synthesis profile used.
---
Nitpick comments:
In `@Tests/Resgrid.Tests/Services/CommunicationServiceTests.cs`:
- Around line 46-49: Add unit tests for the three rejection branches in
CommunicationService.CanSendToUser by modifying CommunicationServiceTests:
update or add parameterized test(s) that configure
_departmentsServiceMock.Setup(x =>
x.GetDepartmentMemberAsync(It.IsAny<string>(), It.IsAny<int>(),
It.IsAny<bool>())) to return respectively (1) null, (2) new DepartmentMember {
IsDisabled = true }, and (3) new DepartmentMember { IsDeleted = true }, then
assert the send methods on the service are NOT invoked for each case; reference
the CanSendToUser method and the GetDepartmentMemberAsync setup to locate where
to change/add tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d923898f-06ef-4ece-9353-f0fac2ced0d2
📒 Files selected for processing (5)
Tests/Resgrid.Tests/Services/CommunicationServiceTests.csWeb/Resgrid.Web.Tts/Services/AudioProcessingService.csWeb/Resgrid.Web.Tts/Services/IAudioProcessingService.csWeb/Resgrid.Web.Tts/Services/S3StorageService.csWeb/Resgrid.Web.Tts/Services/TtsService.cs
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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
Tests
Documentation