Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CallVideoFeed and CommunicationTest features across models, enums, repositories, services, migrations, DI registrations, v4 APIs, UI views/JS, SQL/query configs, workers, and tests; also introduces cancellation notification flows and permission claims. Changes
Sequence DiagramsequenceDiagram
participant Client as rgba(40,116,166,0.5)
participant APIController as rgba(34,139,34,0.5)
participant Service as rgba(218,165,32,0.5)
participant Repository as rgba(128,0,128,0.5)
participant Database as rgba(178,34,34,0.5)
Client->>APIController: POST /api/v4/CallVideoFeeds (Save)
APIController->>Service: SaveCallVideoFeedAsync(feed)
Service->>Repository: SaveOrUpdateAsync(feed)
Repository->>Database: INSERT/UPDATE CallVideoFeeds
Database-->>Repository: OK
Repository-->>Service: saved feed
Service-->>APIController: saved feed
APIController-->>Client: 201 Created {Id}
Client->>APIController: GET /api/v4/CallVideoFeeds?callId=X
APIController->>Service: GetCallVideoFeedsByCallIdAsync(X)
Service->>Repository: GetByCallIdAsync(X)
Repository->>Database: SELECT * FROM CallVideoFeeds WHERE CallId=@CallId AND IsDeleted=0
Database-->>Repository: feeds
Repository-->>Service: feeds
Service-->>APIController: feeds
APIController-->>Client: 200 OK {Data: [feeds]}
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [ProducesResponseType(StatusCodes.Status201Created)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [Authorize(Policy = ResgridResources.Call_View)] | ||
| public async Task<ActionResult<SaveCallVideoFeedResult>> SaveCallVideoFeed(SaveCallVideoFeedInput input, CancellationToken cancellationToken) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix this, add anti-forgery validation to the POST action so requests must include a valid CSRF token.
Best single change (without altering functional behavior): add [ValidateAntiForgeryToken] directly to SaveCallVideoFeed in Web/Resgrid.Web.Services/Controllers/v4/CallVideoFeedsController.cs, alongside existing attributes. Because this attribute is in ASP.NET Core MVC, also add using Microsoft.AspNetCore.Antiforgery; if needed by your project style (attribute is commonly available via MVC assemblies; explicit using is safest if unresolved). This keeps endpoint logic unchanged and only enforces token validation for POST submissions.
| @@ -1,6 +1,7 @@ | ||
| using Microsoft.AspNetCore.Authorization; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Mvc; | ||
| using Microsoft.AspNetCore.Antiforgery; | ||
| using Resgrid.Model.Services; | ||
| using Resgrid.Providers.Claims; | ||
| using System.Threading.Tasks; | ||
| @@ -95,6 +96,7 @@ | ||
| /// <param name="cancellationToken">The cancellation token</param> | ||
| /// <returns>ActionResult.</returns> | ||
| [HttpPost("SaveCallVideoFeed")] | ||
| [ValidateAntiForgeryToken] | ||
| [Consumes(MediaTypeNames.Application.Json)] | ||
| [ProducesResponseType(StatusCodes.Status201Created)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
Core/Resgrid.Model/Services/ICallsService.cs (1)
422-422: Replace the boolean-flag matrix with an options object.This contract is up to ten switches now, and existing callers already pass them positionally. Another addition makes it very easy to request the wrong related data by accident. A small
PopulateCallDataOptionstype would make call sites self-describing and future additions safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Services/ICallsService.cs` at line 422, The PopulateCallData method on ICallsService currently accepts a long positional boolean-flag matrix which is error-prone; replace it with a single options object (e.g., PopulateCallDataOptions) that exposes named bool properties like GetDispatches, GetAttachments, GetNotes, GetGroupDispatches, GetUnitDispatches, GetRoleDispatches, GetProtocols, GetReferences, GetContacts, GetVideoFeeds (with sensible defaults). Change the interface signature from Task<Call> PopulateCallData(Call call, bool..., bool) to Task<Call> PopulateCallData(Call call, PopulateCallDataOptions options) and update all callers to construct and pass a PopulateCallDataOptions instance with named properties instead of positional booleans; ensure any implementations of PopulateCallData read the new options object and preserve previous behavior when properties are omitted (defaults).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Services/CallsService.cs`:
- Around line 590-597: When populating call.VideoFeeds from the repository (the
await _callVideoFeedRepository.GetByCallIdAsync(call.CallId) path) ensure the
returned CallVideoFeed list is ordered by its SortOrder before assigning to
call.VideoFeeds (and likewise in the other occurrence around the 1014–1019
block); change the assignment so that if items != null you set call.VideoFeeds =
items.OrderBy(f => f.SortOrder).ToList() (and when creating an empty collection
keep it as new List<CallVideoFeed>()), ensuring CallVideoFeed.SortOrder is the
stable sort key used in both places.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0061_AddingCallVideoFeedsPg.cs`:
- Around line 11-31: The migrations use the PostgreSQL citext type (seen in
M0061_AddingCallVideoFeedsPg.cs columns like callvideofeedid, name,
addedbyuserid, deletedbyuserid, flaggedbyuserid) but no migration creates the
citext extension; add a new initial migration (e.g.,
M0000_CreateCitextExtension) that executes CREATE EXTENSION IF NOT EXISTS
"citext" as its first step (or ensure the DB runs that SQL before migrations
start) so migrations such as M0001_InitialMigration.sql and
M0061_AddingCallVideoFeedsPg.cs won’t fail with "type 'citext' does not exist".
In
`@Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.cs`:
- Around line 1117-1119: The PostgreSQL query templates for CallVideoFeeds
return soft-deleted rows; update SelectCallVideoFeedsByCallIdQuery and
SelectCallVideoFeedsByDepartmentIdQuery (related to CallVideoFeedsTable) to
filter out deleted rows by adding a condition that IsDeleted = false to the
WHERE clause (matching the SQL Server templates' active-feed behavior) so only
non-deleted feeds are returned.
In
`@Repositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.cs`:
- Around line 1081-1083: The SELECT templates for CallVideoFeeds currently
return soft-deleted rows; update SelectCallVideoFeedsByCallIdQuery and
SelectCallVideoFeedsByDepartmentIdQuery to filter out soft-deleted records by
adding an IsDeleted check (e.g. append "AND [IsDeleted] = 0" to the existing
WHERE clauses) while keeping the existing placeholders and CallVideoFeedsTable
unchanged so queries like SelectCallVideoFeedsByCallIdQuery and
SelectCallVideoFeedsByDepartmentIdQuery exclude deleted feeds by default.
In `@Tests/Resgrid.Tests/Services/CallVideoFeedTests.cs`:
- Around line 121-136: The test
GetCallVideoFeedsByCallIdAsync_ShouldNotReturnDeletedFeeds has a mismatched
assertion: it sets up _callVideoFeedRepo to return two feeds (one IsDeleted =
true) and then asserts result.Should().HaveCount(2) while the name implies
deleted feeds should be excluded; either rename the test to reflect the current
service contract (e.g.,
GetCallVideoFeedsByCallIdAsync_ReturnsAllFeedsIncludingDeleted) or change the
assertion to expect only non-deleted feeds and update the setup/verification
accordingly (filtering logic would be exercised against
_service.GetCallVideoFeedsByCallIdAsync and the CallVideoFeed.IsDeleted flag).
In `@Web/Resgrid.Web.Services/Controllers/v4/CallVideoFeedsController.cs`:
- Around line 96-100: The controller currently uses the read-only policy for
mutating endpoints; change the Authorize attribute on the mutating actions in
CallVideoFeedsController (e.g., the SaveCallVideoFeed POST, the
UpdateCallVideoFeed PUT, and the DeleteCallVideoFeed DELETE methods) to the call
mutating policy instead of ResgridResources.Call_View — replace the policy value
with the registered mutating policy for calls (for example
ResgridResources.Call_Edit or the project’s Call_Modify policy) on each of those
methods so view-only callers cannot perform create/update/delete operations.
- Line 54: Validate and reject invalid numeric input instead of using Parse: in
GetCallVideoFeeds validate the incoming callId string once using int.TryParse
(prefer the overload with System.Globalization.NumberStyles.Integer and
System.Globalization.CultureInfo.InvariantCulture) and return BadRequest() on
failure before calling _callsService.GetCallByIdAsync; in SaveCallVideoFeed
validate input.CallId with int.TryParse before any use and return BadRequest()
on failure; for all decimal parsing in SaveCallVideoFeed and EditCallVideoFeed
(the latitude/longitude parses) replace decimal.Parse with decimal.TryParse
using System.Globalization.NumberStyles.Number and
System.Globalization.CultureInfo.InvariantCulture and return BadRequest() when
parsing fails so malformed coordinates produce a 400 instead of an exception.
In `@Web/Resgrid.Web.Services/Models/v4/CallVideoFeeds/EditCallVideoFeedInput.cs`:
- Around line 10-31: Change the loose string/nullable properties on
EditCallVideoFeedInput to enforce the persisted schema: make CallId an int (not
string) with [Required] and [Range(1, int.MaxValue)], constrain Name and Url
with [Required] and appropriate [StringLength(max)] limits, add
[StringLength(max)] to Description, Latitude and Longitude (or use a regex for
coordinate format if required), and add validation for FeedType/FeedFormat
(e.g., non-nullable enums or [Range] if they are numeric) and for Status and
SortOrder (use [Required] and [Range] to match allowed values). Update the
EditCallVideoFeedInput class property types/attributes (CallId, Name, Url,
Description, Latitude, Longitude, FeedType, FeedFormat, Status, SortOrder) so
model binding rejects invalid formats/over-length values at the API boundary.
In `@Web/Resgrid.Web.Services/Models/v4/CallVideoFeeds/SaveCallVideoFeedInput.cs`:
- Around line 7-26: In SaveCallVideoFeedInput, tighten model validation to
reflect storage constraints: change CallId from string to Guid (or Guid?) and
keep [Required] so invalid GUIDs fail model binding; add appropriate
[MaxLength]/[StringLength] annotations to Name, Url, and Description to match DB
column lengths; add [Url] (or a regex) to Url and consider adding range/regex
validation for Latitude and Longitude or convert them to decimal?double to match
storage; ensure SortOrder remains int and mark nullable only if DB allows nulls.
Reference properties: SaveCallVideoFeedInput.CallId, .Name, .Url, .Description,
.Latitude, .Longitude, .SortOrder.
In `@Web/Resgrid.Web/Areas/User/Models/Calls/ViewCallView.cs`:
- Line 30: The VideoFeeds property on the ViewCallView model can be null and
cause runtime errors when views enumerate it; update the ViewCallView class so
VideoFeeds is initialized defensively (e.g., assign a new List<CallVideoFeed>()
as the property default or initialize it inside the ViewCallView constructor) so
the property is never null at render time.
In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtml`:
- Line 764: The timestamp rendering uses feed.AddedOn.ToString("g"), which
ignores the department timezone/localization; replace that call with the
department-aware formatting helper used elsewhere on this view (i.e. use the
same method/extension that converts datetimes to the department local time and
formats them consistently) so that feed.AddedOn is converted to the department
timezone/localization before being rendered.
- Line 733: The anchor renders feed.Url with target="_blank" without rel or
scheme checks; update the view so the href uses only validated/normalized URLs
(accept only http or https via server-side Uri.TryCreate/validation for feed.Url
or a sanitized property) and add rel="noopener noreferrer" to the <a> element to
prevent reverse-tabnabbing; if the URL is unsafe or has a non-http(s) scheme,
render it as plain text or replace with a safe placeholder instead of linking.
---
Nitpick comments:
In `@Core/Resgrid.Model/Services/ICallsService.cs`:
- Line 422: The PopulateCallData method on ICallsService currently accepts a
long positional boolean-flag matrix which is error-prone; replace it with a
single options object (e.g., PopulateCallDataOptions) that exposes named bool
properties like GetDispatches, GetAttachments, GetNotes, GetGroupDispatches,
GetUnitDispatches, GetRoleDispatches, GetProtocols, GetReferences, GetContacts,
GetVideoFeeds (with sensible defaults). Change the interface signature from
Task<Call> PopulateCallData(Call call, bool..., bool) to Task<Call>
PopulateCallData(Call call, PopulateCallDataOptions options) and update all
callers to construct and pass a PopulateCallDataOptions instance with named
properties instead of positional booleans; ensure any implementations of
PopulateCallData read the new options object and preserve previous behavior when
properties are omitted (defaults).
🪄 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: dc0f19bd-3ce8-4897-b84d-ee13afa6b80b
⛔ Files ignored due to path filters (9)
Core/Resgrid.Localization/Areas/User/Dispatch/Call.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (31)
Core/Resgrid.Model/Call.csCore/Resgrid.Model/CallVideoFeed.csCore/Resgrid.Model/CallVideoFeedFormats.csCore/Resgrid.Model/CallVideoFeedStatuses.csCore/Resgrid.Model/CallVideoFeedTypes.csCore/Resgrid.Model/Repositories/ICallVideoFeedRepository.csCore/Resgrid.Model/Services/ICallsService.csCore/Resgrid.Services/CallsService.csProviders/Resgrid.Providers.Migrations/Migrations/M0061_AddingCallVideoFeeds.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0061_AddingCallVideoFeedsPg.csRepositories/Resgrid.Repositories.DataRepository/CallVideoFeedRepository.csRepositories/Resgrid.Repositories.DataRepository/Configs/SqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Modules/ApiDataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/DataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/NonWebDataModule.csRepositories/Resgrid.Repositories.DataRepository/Modules/TestingDataModule.csRepositories/Resgrid.Repositories.DataRepository/Queries/CallVideoFeeds/SelectCallVideoFeedsByCallIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CallVideoFeeds/SelectCallVideoFeedsByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csTests/Resgrid.Tests/Services/CallVideoFeedTests.csWeb/Resgrid.Web.Services/Controllers/v4/CallVideoFeedsController.csWeb/Resgrid.Web.Services/Models/v4/CallVideoFeeds/CallVideoFeedsResult.csWeb/Resgrid.Web.Services/Models/v4/CallVideoFeeds/DeleteCallVideoFeedResult.csWeb/Resgrid.Web.Services/Models/v4/CallVideoFeeds/EditCallVideoFeedInput.csWeb/Resgrid.Web.Services/Models/v4/CallVideoFeeds/SaveCallVideoFeedInput.csWeb/Resgrid.Web.Services/Models/v4/CallVideoFeeds/SaveCallVideoFeedResult.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Models/Calls/ViewCallView.csWeb/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtml
| .WithColumn("callvideofeedid").AsCustom("citext").NotNullable().PrimaryKey() | ||
| .WithColumn("callid").AsInt32().NotNullable() | ||
| .WithColumn("departmentid").AsInt32().NotNullable() | ||
| .WithColumn("name").AsCustom("citext").NotNullable() | ||
| .WithColumn("url").AsCustom("text").NotNullable() | ||
| .WithColumn("feedtype").AsInt32().Nullable() | ||
| .WithColumn("feedformat").AsInt32().Nullable() | ||
| .WithColumn("description").AsCustom("text").Nullable() | ||
| .WithColumn("status").AsInt32().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("latitude").AsDecimal(10, 7).Nullable() | ||
| .WithColumn("longitude").AsDecimal(10, 7).Nullable() | ||
| .WithColumn("addedbyuserid").AsCustom("citext").NotNullable() | ||
| .WithColumn("addedon").AsDateTime().NotNullable() | ||
| .WithColumn("updatedon").AsDateTime().Nullable() | ||
| .WithColumn("sortorder").AsInt32().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("isdeleted").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("deletedbyuserid").AsCustom("citext").Nullable() | ||
| .WithColumn("deletedon").AsDateTime().Nullable() | ||
| .WithColumn("isflagged").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("flaggedreason").AsCustom("text").Nullable() | ||
| .WithColumn("flaggedbyuserid").AsCustom("citext").Nullable() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any migration/bootstrap step enabling citext.
rg -n --type=cs "create extension.*citext|citext.*extension|AsCustom\\(\"citext\"\\)" Providers
# Narrow to Postgres migrations for explicit extension setup.
rg -n --type=cs "create extension.*citext|Execute\\.Sql\\(\"CREATE EXTENSION.*citext" Providers/Resgrid.Providers.MigrationsPgRepository: Resgrid/Core
Length of output: 50368
🏁 Script executed:
# Search for CREATE EXTENSION in all file types, not just C#
find Providers/Resgrid.Providers.MigrationsPg -type f -exec grep -l "CREATE EXTENSION" {} \;
# Also search for bootstrap/initialization SQL files
find . -type f \( -name "*.sql" -o -name "*bootstrap*" -o -name "*init*" \) | head -20
# Check if there's a setup or database initialization in the migration runner
rg -i "create extension|citext" --type=sql --type=txt --type=md Providers/Resgrid.Providers.MigrationsPgRepository: Resgrid/Core
Length of output: 40583
🏁 Script executed:
# Check the beginning of M0001_InitialMigration.sql for CREATE EXTENSION
head -50 Providers/Resgrid.Providers.MigrationsPg/Sql/M0001_InitialMigration.sql
# Also search for CREATE EXTENSION specifically in that file
grep -n "CREATE EXTENSION" Providers/Resgrid.Providers.MigrationsPg/Sql/M0001_InitialMigration.sqlRepository: Resgrid/Core
Length of output: 3080
🏁 Script executed:
# Check file encoding
file Providers/Resgrid.Providers.MigrationsPg/Sql/M0001_InitialMigration.sql
# Try with iconv to convert if needed
iconv -f UTF-16 -t UTF-8 Providers/Resgrid.Providers.MigrationsPg/Sql/M0001_InitialMigration.sql 2>/dev/null | head -100 | grep -i "CREATE EXTENSION" || echo "Not found in first 100 lines"
# Also check the C# migration classes for explicit extension creation
rg -A 5 "Execute\.Sql.*CREATE EXTENSION" Providers/Resgrid.Providers.MigrationsPg/Migrations/Repository: Resgrid/Core
Length of output: 144
🏁 Script executed:
# Check the Docker database initialization files
head -50 Docker/db/create-databases.sql
# Check the initialization shell scripts
cat Docker/db/initdbs.sh
# Check for CREATE EXTENSION in any SQL file type
find Providers/Resgrid.Providers.MigrationsPg -type f -name "*.sql" -exec grep -l "CREATE EXTENSION" {} \;
find Docker -type f -name "*.sql" -exec grep -l "CREATE EXTENSION" {} \;Repository: Resgrid/Core
Length of output: 546
🏁 Script executed:
# Check M0001 migration C# code for extension creation
cat Providers/Resgrid.Providers.MigrationsPg/Migrations/M0001_InitialMigration.cs | head -80
# Search for Execute.Sql that might create extension in all M0001 files
rg -i "execute|extension" Providers/Resgrid.Providers.MigrationsPg/Migrations/M0001*.cs -A 2 | head -100Repository: Resgrid/Core
Length of output: 279
🏁 Script executed:
# Find the actual M0001 migration file
find Providers/Resgrid.Providers.MigrationsPg/Migrations -name "*M0001*" -type f
# Read the first migration C# file
find Providers/Resgrid.Providers.MigrationsPg/Migrations -name "*001*" -type f -exec cat {} \;
# Check if citext extension is enabled in PostgreSQL Docker setup
find Docker -type f \( -name "*.sh" -o -name "*.sql" -o -name "Dockerfile*" \) -exec grep -l "citext\|CREATE EXTENSION" {} \;Repository: Resgrid/Core
Length of output: 11076
🏁 Script executed:
# The M0001 migration uses an embedded SQL script. Let's check if citext extension is created in that script
# Try different encoding approaches
python3 << 'EOF'
import os
file_path = "Providers/Resgrid.Providers.MigrationsPg/Sql/M0001_InitialMigration.sql"
try:
with open(file_path, 'rb') as f:
content = f.read()
# Try different decodings
for encoding in ['utf-16', 'utf-16-le', 'utf-8', 'latin1']:
try:
text = content.decode(encoding)
if 'CREATE EXTENSION' in text.upper():
print(f"Found CREATE EXTENSION in {encoding} encoding")
# Print first occurrence
idx = text.upper().find('CREATE EXTENSION')
print(text[max(0, idx-50):min(len(text), idx+200)])
break
except:
continue
else:
print("CREATE EXTENSION not found in any encoding")
except Exception as e:
print(f"Error: {e}")
EOF
# Also check what PostgreSQL initialization files exist
find . -type f -path "*/postgres*" -o -path "*/pg*" | grep -i "init\|docker\|sql" | head -20Repository: Resgrid/Core
Length of output: 98
The citext extension prerequisite must be created before any PostgreSQL migrations execute.
The database extension citext is used extensively throughout the migration chain starting with M0001_InitialMigration.sql, yet no migration explicitly creates it. If the extension is not pre-enabled on the target PostgreSQL database, migration execution will fail with a "type "citext" does not exist" error.
Ensure one of the following:
- The PostgreSQL database has
CREATE EXTENSION IF NOT EXISTS "citext"executed before running migrations, or - Add an explicit migration (M0000 or equivalent) that creates the extension as its first step
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0061_AddingCallVideoFeedsPg.cs`
around lines 11 - 31, The migrations use the PostgreSQL citext type (seen in
M0061_AddingCallVideoFeedsPg.cs columns like callvideofeedid, name,
addedbyuserid, deletedbyuserid, flaggedbyuserid) but no migration creates the
citext extension; add a new initial migration (e.g.,
M0000_CreateCitextExtension) that executes CREATE EXTENSION IF NOT EXISTS
"citext" as its first step (or ensure the DB runs that SQL before migrations
start) so migrations such as M0001_InitialMigration.sql and
M0061_AddingCallVideoFeedsPg.cs won’t fail with "type 'citext' does not exist".
Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.cs
Outdated
Show resolved
Hide resolved
Repositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.cs
Outdated
Show resolved
Hide resolved
| [Test] | ||
| public async Task GetCallVideoFeedsByCallIdAsync_ShouldNotReturnDeletedFeeds() | ||
| { | ||
| var feeds = new List<CallVideoFeed> | ||
| { | ||
| new CallVideoFeed { CallVideoFeedId = "feed1", CallId = 1, Name = "Feed 1", Url = "http://example.com/1", IsDeleted = false }, | ||
| new CallVideoFeed { CallVideoFeedId = "feed2", CallId = 1, Name = "Feed 2", Url = "http://example.com/2", IsDeleted = true } | ||
| }; | ||
|
|
||
| _callVideoFeedRepo.Setup(x => x.GetByCallIdAsync(1)).ReturnsAsync(feeds); | ||
|
|
||
| var result = await _service.GetCallVideoFeedsByCallIdAsync(1); | ||
|
|
||
| // The service returns all feeds from repo; filtering is done at the API layer | ||
| result.Should().HaveCount(2); | ||
| } |
There was a problem hiding this comment.
Align the test name with the asserted contract.
The name says deleted feeds should be excluded, but the body asserts both rows are returned and even documents caller-side filtering. As written, the test communicates the opposite of what it checks. Rename it to reflect the current service contract, or change the assertion if deleted feeds are supposed to be hidden here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Services/CallVideoFeedTests.cs` around lines 121 - 136,
The test GetCallVideoFeedsByCallIdAsync_ShouldNotReturnDeletedFeeds has a
mismatched assertion: it sets up _callVideoFeedRepo to return two feeds (one
IsDeleted = true) and then asserts result.Should().HaveCount(2) while the name
implies deleted feeds should be excluded; either rename the test to reflect the
current service contract (e.g.,
GetCallVideoFeedsByCallIdAsync_ReturnsAllFeedsIncludingDeleted) or change the
assertion to expect only non-deleted feeds and update the setup/verification
accordingly (filtering logic would be exercised against
_service.GetCallVideoFeedsByCallIdAsync and the CallVideoFeed.IsDeleted flag).
| [Required] | ||
| public string CallId { get; set; } | ||
|
|
||
| [Required] | ||
| public string Name { get; set; } | ||
|
|
||
| [Required] | ||
| public string Url { get; set; } | ||
|
|
||
| public int? FeedType { get; set; } | ||
|
|
||
| public int? FeedFormat { get; set; } | ||
|
|
||
| public string Description { get; set; } | ||
|
|
||
| public int Status { get; set; } | ||
|
|
||
| public string Latitude { get; set; } | ||
|
|
||
| public string Longitude { get; set; } | ||
|
|
||
| public int SortOrder { get; set; } |
There was a problem hiding this comment.
Align edit input contract with persisted schema constraints.
As written, model binding accepts values that can fail later (non-numeric CallId, over-length text fields). Tightening this at the API boundary will prevent downstream errors.
Suggested fix
[Required]
- public string CallId { get; set; }
+ public int CallId { get; set; }
[Required]
+ [StringLength(500)]
public string Name { get; set; }
[Required]
+ [StringLength(2000)]
public string Url { get; set; }
+ [StringLength(4000)]
public string Description { get; set; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Models/v4/CallVideoFeeds/EditCallVideoFeedInput.cs`
around lines 10 - 31, Change the loose string/nullable properties on
EditCallVideoFeedInput to enforce the persisted schema: make CallId an int (not
string) with [Required] and [Range(1, int.MaxValue)], constrain Name and Url
with [Required] and appropriate [StringLength(max)] limits, add
[StringLength(max)] to Description, Latitude and Longitude (or use a regex for
coordinate format if required), and add validation for FeedType/FeedFormat
(e.g., non-nullable enums or [Range] if they are numeric) and for Status and
SortOrder (use [Required] and [Range] to match allowed values). Update the
EditCallVideoFeedInput class property types/attributes (CallId, Name, Url,
Description, Latitude, Longitude, FeedType, FeedFormat, Status, SortOrder) so
model binding rejects invalid formats/over-length values at the API boundary.
| [Required] | ||
| public string CallId { get; set; } | ||
|
|
||
| [Required] | ||
| public string Name { get; set; } | ||
|
|
||
| [Required] | ||
| public string Url { get; set; } | ||
|
|
||
| public int? FeedType { get; set; } | ||
|
|
||
| public int? FeedFormat { get; set; } | ||
|
|
||
| public string Description { get; set; } | ||
|
|
||
| public string Latitude { get; set; } | ||
|
|
||
| public string Longitude { get; set; } | ||
|
|
||
| public int SortOrder { get; set; } |
There was a problem hiding this comment.
Harden API input validation to match storage constraints.
CallId as string and missing length caps for Name/Url/Description allow invalid payloads through model binding, pushing failures downstream.
Suggested fix
public class SaveCallVideoFeedInput
{
[Required]
- public string CallId { get; set; }
+ public int CallId { get; set; }
[Required]
+ [StringLength(500)]
public string Name { get; set; }
[Required]
+ [StringLength(2000)]
public string Url { get; set; }
public int? FeedType { get; set; }
public int? FeedFormat { get; set; }
+ [StringLength(4000)]
public string Description { get; set; }📝 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.
| [Required] | |
| public string CallId { get; set; } | |
| [Required] | |
| public string Name { get; set; } | |
| [Required] | |
| public string Url { get; set; } | |
| public int? FeedType { get; set; } | |
| public int? FeedFormat { get; set; } | |
| public string Description { get; set; } | |
| public string Latitude { get; set; } | |
| public string Longitude { get; set; } | |
| public int SortOrder { get; set; } | |
| [Required] | |
| public int CallId { get; set; } | |
| [Required] | |
| [StringLength(500)] | |
| public string Name { get; set; } | |
| [Required] | |
| [StringLength(2000)] | |
| public string Url { get; set; } | |
| public int? FeedType { get; set; } | |
| public int? FeedFormat { get; set; } | |
| [StringLength(4000)] | |
| public string Description { get; set; } | |
| public string Latitude { get; set; } | |
| public string Longitude { get; set; } | |
| public int SortOrder { get; set; } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Models/v4/CallVideoFeeds/SaveCallVideoFeedInput.cs`
around lines 7 - 26, In SaveCallVideoFeedInput, tighten model validation to
reflect storage constraints: change CallId from string to Guid (or Guid?) and
keep [Required] so invalid GUIDs fail model binding; add appropriate
[MaxLength]/[StringLength] annotations to Name, Url, and Description to match DB
column lengths; add [Url] (or a regex) to Url and consider adding range/regex
validation for Latitude and Longitude or convert them to decimal?double to match
storage; ensure SortOrder remains int and mark nullable only if DB allows nulls.
Reference properties: SaveCallVideoFeedInput.CallId, .Name, .Url, .Description,
.Latitude, .Longitude, .SortOrder.
| { | ||
| <tr> | ||
| <td>@feed.Name</td> | ||
| <td><a href="@feed.Url" target="_blank">@feed.Url</a></td> |
There was a problem hiding this comment.
Harden video feed links against unsafe schemes and tabnabbing.
Line 733 renders feed.Url directly in href with target="_blank" and no rel safeguards. If URL values are user-provided, this allows unsafe schemes and reverse-tabnabbing.
🔐 Proposed hardening
- <td><a href="@feed.Url" target="_blank">@feed.Url</a></td>
+ @{
+ var isSafeUrl = Uri.TryCreate(feed.Url, UriKind.Absolute, out var parsedFeedUrl) &&
+ (parsedFeedUrl.Scheme == Uri.UriSchemeHttp || parsedFeedUrl.Scheme == Uri.UriSchemeHttps);
+ }
+ <td>
+ `@if` (isSafeUrl)
+ {
+ <a href="@feed.Url" target="_blank" rel="noopener noreferrer">@feed.Url</a>
+ }
+ else
+ {
+ <span>@feed.Url</span>
+ }
+ </td>📝 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.
| <td><a href="@feed.Url" target="_blank">@feed.Url</a></td> | |
| @{ | |
| var isSafeUrl = Uri.TryCreate(feed.Url, UriKind.Absolute, out var parsedFeedUrl) && | |
| (parsedFeedUrl.Scheme == Uri.UriSchemeHttp || parsedFeedUrl.Scheme == Uri.UriSchemeHttps); | |
| } | |
| <td> | |
| `@if` (isSafeUrl) | |
| { | |
| <a href="@feed.Url" target="_blank" rel="noopener noreferrer">@feed.Url</a> | |
| } | |
| else | |
| { | |
| <span>@feed.Url</span> | |
| } | |
| </td> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtml` at line 733, The
anchor renders feed.Url with target="_blank" without rel or scheme checks;
update the view so the href uses only validated/normalized URLs (accept only
http or https via server-side Uri.TryCreate/validation for feed.Url or a
sanitized property) and add rel="noopener noreferrer" to the <a> element to
prevent reverse-tabnabbing; if the URL is unsafe or has a non-http(s) scheme,
render it as plain text or replace with a safe placeholder instead of linking.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Web/Resgrid.Web.Services/Controllers/v4/CallVideoFeedsController.cs (1)
243-263: Consider converting to an extension method.Per coding guidelines, extension methods are appropriate for domain-specific operations. This mapper could be an extension method on
CallVideoFeed:♻️ Suggested refactor
-public static CallVideoFeedResultData ConvertCallVideoFeed(CallVideoFeed feed, string fullName, Department department) +public static CallVideoFeedResultData ToResultData(this CallVideoFeed feed, string fullName, Department department)Usage would then read more fluently:
result.Data.Add(feed.ToResultData(fullName, department));As per coding guidelines: "Use extension methods appropriately for domain-specific operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/CallVideoFeedsController.cs` around lines 243 - 263, ConvertCallVideoFeed currently lives as a static mapper but should be an extension on CallVideoFeed for domain-specific fluency: create a static class (e.g., CallVideoFeedExtensions) and add a public static method ToResultData(this CallVideoFeed feed, string fullName, Department department) that returns CallVideoFeedResultData and contains the same mapping logic from ConvertCallVideoFeed (populate CallVideoFeedResultData properties including AddedOn formatting via feed.AddedOn.TimeConverter(department).FormatForDepartment(department)). Replace callers that invoke ConvertCallVideoFeed(...) with feed.ToResultData(fullName, department) and remove or mark the old ConvertCallVideoFeed method as obsolete.
🤖 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/Controllers/v4/CallVideoFeedsController.cs`:
- Around line 69-75: The loop causes an N+1 lookup by calling
UserHelper.GetFullNameForUser for each feed; change to collect non-deleted feeds
(feeds.Where(f => !f.IsDeleted)), extract distinct AddedByUserId values, call a
batch helper like UserHelper.GetFullNamesForUsers(userIds) to retrieve a
dictionary/map of userId->fullName, then iterate nonDeletedFeeds and call
ConvertCallVideoFeed(feed, userNames[feed.AddedByUserId], department) to add to
result.Data. Ensure you add/implement the batch method on UserHelper and
preserve existing behavior when a userId is missing (e.g., fallback to empty
string).
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/CallVideoFeedsController.cs`:
- Around line 243-263: ConvertCallVideoFeed currently lives as a static mapper
but should be an extension on CallVideoFeed for domain-specific fluency: create
a static class (e.g., CallVideoFeedExtensions) and add a public static method
ToResultData(this CallVideoFeed feed, string fullName, Department department)
that returns CallVideoFeedResultData and contains the same mapping logic from
ConvertCallVideoFeed (populate CallVideoFeedResultData properties including
AddedOn formatting via
feed.AddedOn.TimeConverter(department).FormatForDepartment(department)). Replace
callers that invoke ConvertCallVideoFeed(...) with feed.ToResultData(fullName,
department) and remove or mark the old ConvertCallVideoFeed method as obsolete.
🪄 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: 5629d559-3f7e-467a-bd8f-fa2dfbf6dc2a
📒 Files selected for processing (8)
Core/Resgrid.Services/CallsService.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csWeb/Resgrid.Web.Services/Controllers/v4/CallVideoFeedsController.csWeb/Resgrid.Web.Services/Models/v4/CallVideoFeeds/EditCallVideoFeedInput.csWeb/Resgrid.Web.Services/Models/v4/CallVideoFeeds/SaveCallVideoFeedInput.csWeb/Resgrid.Web/Areas/User/Models/Calls/ViewCallView.csWeb/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtml
✅ Files skipped from review due to trivial changes (3)
- Web/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtml
- Web/Resgrid.Web.Services/Models/v4/CallVideoFeeds/SaveCallVideoFeedInput.cs
- Web/Resgrid.Web.Services/Models/v4/CallVideoFeeds/EditCallVideoFeedInput.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- Web/Resgrid.Web/Areas/User/Models/Calls/ViewCallView.cs
- Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.cs
- Core/Resgrid.Services/CallsService.cs
- Repositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.cs
| if (feeds != null && feeds.Any()) | ||
| { | ||
| foreach (var feed in feeds.Where(f => !f.IsDeleted)) | ||
| { | ||
| var fullName = await UserHelper.GetFullNameForUser(feed.AddedByUserId); | ||
| result.Data.Add(ConvertCallVideoFeed(feed, fullName, department)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if GetCallVideoFeedsByCallIdAsync already filters IsDeleted
ast-grep --pattern 'GetCallVideoFeedsByCallIdAsync($$$)'
rg -n "IsDeleted" --type cs -C 3 -g '*CallVideoFeed*Repository*'Repository: Resgrid/Core
Length of output: 38
🏁 Script executed:
# Locate the CallVideoFeedsController.cs file
find . -name "*CallVideoFeedsController*" -type fRepository: Resgrid/Core
Length of output: 125
🏁 Script executed:
# Search for GetCallVideoFeedsByCallIdAsync method definition
rg -n "GetCallVideoFeedsByCallIdAsync" --type cs -A 5Repository: Resgrid/Core
Length of output: 4127
🏁 Script executed:
# Locate UserHelper and GetFullNameForUser method
fd -i "UserHelper" --type f
rg -n "GetFullNameForUser" --type cs -A 3 -B 1Repository: Resgrid/Core
Length of output: 21955
Refactor to batch-fetch user names and eliminate N+1 query pattern.
Each video feed in the loop triggers UserHelper.GetFullNameForUser, causing a separate database lookup per feed. Instead, collect all user IDs upfront and fetch names in a single batch operation:
Suggested refactoring
if (feeds != null && feeds.Any())
{
var nonDeletedFeeds = feeds.Where(f => !f.IsDeleted).ToList();
var userIds = nonDeletedFeeds.Select(f => f.AddedByUserId).Distinct();
var userNames = await UserHelper.GetFullNamesForUsers(userIds); // batch method
foreach (var feed in nonDeletedFeeds)
{
var fullName = userNames[feed.AddedByUserId];
result.Data.Add(ConvertCallVideoFeed(feed, fullName, department));
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Controllers/v4/CallVideoFeedsController.cs` around
lines 69 - 75, The loop causes an N+1 lookup by calling
UserHelper.GetFullNameForUser for each feed; change to collect non-deleted feeds
(feeds.Where(f => !f.IsDeleted)), extract distinct AddedByUserId values, call a
batch helper like UserHelper.GetFullNamesForUsers(userIds) to retrieve a
dictionary/map of userId->fullName, then iterate nonDeletedFeeds and call
ConvertCallVideoFeed(feed, userNames[feed.AddedByUserId], department) to add to
result.Data. Ensure you add/implement the batch method on UserHelper and
preserve existing behavior when a userId is missing (e.g., fallback to empty
string).
| [Authorize(Policy = ResgridResources.Call_View)] | ||
| public async Task<ActionResult<CallVideoFeedsResult>> GetCallVideoFeeds(string callId) | ||
| { | ||
| if (String.IsNullOrWhiteSpace(callId) || !int.TryParse(callId, out var cId)) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Copilot Autofix
AI 3 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| [Authorize(Policy = ResgridResources.Call_View)] | ||
| public async Task<ActionResult<CallVideoFeedsResult>> GetCallVideoFeeds(string callId) | ||
| { | ||
| if (String.IsNullOrWhiteSpace(callId) || !int.TryParse(callId, out var cId)) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Copilot Autofix
AI 3 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| [Authorize(Policy = ResgridResources.Call_View)] | ||
| public async Task<ActionResult<CallVideoFeedsResult>> GetCallVideoFeeds(string callId) | ||
| { | ||
| if (String.IsNullOrWhiteSpace(callId) || !int.TryParse(callId, out var cId)) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Copilot Autofix
AI 3 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| if (!ModelState.IsValid) | ||
| return BadRequest(); | ||
|
|
||
| if (!int.TryParse(input.CallId, out var parsedCallId)) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
Use a strongly typed int for CallId in this action flow so authorization checks are not guarded by ad-hoc parsing of raw user input in the controller. The best minimal fix in the shown file is to validate input.CallId as a required positive integer, then use that validated value consistently for lookup and authorization checks. This keeps behavior intact (bad input still rejected) while making the guard explicit and strict, reducing the “user-controlled bypass” pattern.
In Web/Resgrid.Web.Services/Controllers/v4/CallVideoFeedsController.cs, update the SaveCallVideoFeed method around lines 107–116:
- Replace the current
TryParsecheck with a stricter parse + range check (<= 0rejected). - Keep server-side call lookup and
DepartmentIdauthorization check unchanged. - No new imports or dependencies are needed.
| @@ -104,7 +104,7 @@ | ||
| if (!ModelState.IsValid) | ||
| return BadRequest(); | ||
|
|
||
| if (!int.TryParse(input.CallId, out var parsedCallId)) | ||
| if (string.IsNullOrWhiteSpace(input.CallId) || !int.TryParse(input.CallId, out var parsedCallId) || parsedCallId <= 0) | ||
| return BadRequest(); | ||
|
|
||
| var call = await _callsService.GetCallByIdAsync(parsedCallId); |
| if (!ModelState.IsValid) | ||
| return BadRequest(); | ||
|
|
||
| if (!int.TryParse(input.CallId, out var parsedCallId)) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Copilot Autofix
AI 4 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
Web/Resgrid.Web/Areas/User/Views/Groups/Geofence.cshtml (1)
46-47: Add live-region semantics for dynamic save feedback.Line 46 and Line 47 should expose status changes to assistive tech when JS toggles these alerts.
♿ Suggested update
-<div id="successArea" class="alert alert-success" style="display:none;"></div> -<div id="alertArea" class="alert alert-danger" style="display:none;"></div> +<div id="successArea" class="alert alert-success" style="display:none;" role="status" aria-live="polite"></div> +<div id="alertArea" class="alert alert-danger" style="display:none;" role="alert" aria-live="assertive"></div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Groups/Geofence.cshtml` around lines 46 - 47, The success and error alert divs (ids successArea and alertArea in Geofence.cshtml) need ARIA live-region semantics so assistive tech detects dynamic state changes: add appropriate attributes (e.g., successArea: role="status" aria-live="polite" aria-atomic="true"; alertArea: role="alert" aria-live="assertive" aria-atomic="true") and keep the existing show/hide behavior in JS so content is announced when you toggle their visibility.
🤖 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/Areas/User/Controllers/CalendarController.cs`:
- Line 264: The update path is not persisting mapped recurrence fields: when
CalendarController calls _calendarService.UpdateCalendarItemAsync(model.Item,
...) the CalendarService update logic must copy the recurrence properties from
the incoming item to the existing entity before saving; locate the
UpdateCalendarItemAsync update flow in Core/Resgrid.Services/CalendarService.cs
and add assignments to set existingEntity.RepeatOnWeek = item.RepeatOnWeek and
existingEntity.RepeatOnDay = item.RepeatOnDay (and any related recurrence fields
used by monthly-weekday edits) so those values are stored on save.
- Around line 258-262: The bug is that for RecurrenceType == 2 the controller
always overwrites month-day values with defaults from Weekday controls; change
the assignment so it only sets model.Item.RepeatOnWeek and
model.Item.RepeatOnDay when the posted weekday controls contain valid data
(e.g., model.WeekdayOccurrence and model.WeekdayDayOfWeek are present/non-zero)
or when an explicit monthly-weekday mode flag is set; in practice update the
block that checks model.Item.RecurrenceType == 2 to additionally verify
model.WeekdayOccurrence/WeekdayDayOfWeek are valid (or a dedicated
monthly-weekday indicator) before assigning RepeatOnWeek and RepeatOnDay so
month-day mode isn’t overwritten.
In `@Web/Resgrid.Web/Areas/User/Controllers/ContactsController.cs`:
- Around line 462-464: In the ContactsController Edit POST action the SelectList
assigned to ViewBag.Categories (editPostCategories) doesn't include the
currently selected category, so on validation failure the dropdown resets;
update the SelectList creation in the Edit POST handler to pass the model's
selected category id (e.g., model.ContactCategoryId or
contact.ContactCategoryId) as the selectedValue parameter when constructing
ViewBag.Categories so the user's selection is preserved on redisplay.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/Edit.cshtml`:
- Line 259: The TextBoxFor for m => m.Item.RepeatOnDay currently disables all
keyboard input via onkeydown="javascript:return false;", which blocks
keyboard-only users; remove that onkeydown attribute and instead constrain input
in an accessible way (e.g., change the input to a numeric control using
`@Html.TextBoxFor`(m => m.Item.RepeatOnDay, new { type = "number", min = "1", max
= "31", inputmode = "numeric" }) or apply readonly only when appropriate and
expose change via an accessible UI), and ensure any validation/formatting is
enforced server- or JS-side without preventing keyboard entry.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/calendar/resgrid.calendar.newEntry.js`:
- Around line 80-82: The script unconditionally forces the "monthday" radio via
$('input[name="month"][value="monthday"]').prop('checked',
true).trigger('change'); which clobbers a previously selected "weekday" when the
page is re-rendered after an invalid POST; change the initialization to only set
the default when no month-mode radio is already checked (i.e., check
$('input[name="month"]:checked') first) or when creating a brand-new entry, and
only then trigger the change so existing user selections are preserved; update
the logic in resgrid.calendar.newEntry.js around the month radio initialization
and the handler that enables/disables weekday dropdowns accordingly.
In `@Web/Resgrid.Web/wwwroot/js/app/internal/groups/resgrid.groups.geofence.js`:
- Around line 119-124: The code uses jQuery .html() to render server messages
into '#successArea' and '#alertArea' which risks DOM injection; update the
success handler where these are set (the block that currently calls
$('#successArea').html(...) and $('#alertArea').html(...)) to use .text()
instead of .html(), preserving the same message selection logic (data.Message ||
fallback and data && data.Message ? data.Message : fallback) and keep the
subsequent .show()/ .hide() calls unchanged.
---
Nitpick comments:
In `@Web/Resgrid.Web/Areas/User/Views/Groups/Geofence.cshtml`:
- Around line 46-47: The success and error alert divs (ids successArea and
alertArea in Geofence.cshtml) need ARIA live-region semantics so assistive tech
detects dynamic state changes: add appropriate attributes (e.g., successArea:
role="status" aria-live="polite" aria-atomic="true"; alertArea: role="alert"
aria-live="assertive" aria-atomic="true") and keep the existing show/hide
behavior in JS so content is announced when you toggle their visibility.
🪄 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: 6030060b-a67b-4dc4-b90a-7d5e5bde2597
📒 Files selected for processing (11)
Web/Resgrid.Web.Services/Startup.csWeb/Resgrid.Web/Areas/User/Controllers/CalendarController.csWeb/Resgrid.Web/Areas/User/Controllers/ContactsController.csWeb/Resgrid.Web/Areas/User/Models/Calendar/EditCalendarEntry.csWeb/Resgrid.Web/Areas/User/Views/Calendar/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Contacts/Add.cshtmlWeb/Resgrid.Web/Areas/User/Views/Contacts/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Groups/Geofence.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/calendar/resgrid.calendar.editEntry.jsWeb/Resgrid.Web/wwwroot/js/app/internal/calendar/resgrid.calendar.newEntry.jsWeb/Resgrid.Web/wwwroot/js/app/internal/groups/resgrid.groups.geofence.js
💤 Files with no reviewable changes (1)
- Web/Resgrid.Web.Services/Startup.cs
✅ Files skipped from review due to trivial changes (1)
- Web/Resgrid.Web/Areas/User/Models/Calendar/EditCalendarEntry.cs
| if (model.Item.RecurrenceType == 2) | ||
| { | ||
| model.Item.RepeatOnWeek = model.WeekdayOccurrence; | ||
| model.Item.RepeatOnDay = model.WeekdayDayOfWeek; | ||
| } |
There was a problem hiding this comment.
Monthly “day-of-month” mode gets overwritten with weekday defaults.
For RecurrenceType == 2, this block always writes WeekdayOccurrence/WeekdayDayOfWeek. In monthday mode those controls are disabled, so posted values are defaults, and RepeatOnDay can be overwritten incorrectly (often 0).
Suggested fix
- if (model.Item.RecurrenceType == 2)
- {
- model.Item.RepeatOnWeek = model.WeekdayOccurrence;
- model.Item.RepeatOnDay = model.WeekdayDayOfWeek;
- }
+ if (model.Item.RecurrenceType == (int)RecurrenceTypes.Monthly)
+ {
+ if (model.WeekdayOccurrence > 0)
+ {
+ // weekday mode (e.g., 2nd Tuesday)
+ model.Item.RepeatOnWeek = model.WeekdayOccurrence;
+ model.Item.RepeatOnDay = model.WeekdayDayOfWeek;
+ }
+ else
+ {
+ // day-of-month mode (e.g., 15th)
+ model.Item.RepeatOnWeek = 0;
+ // keep model.Item.RepeatOnDay as posted from Item_RepeatOnDay
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs` around lines
258 - 262, The bug is that for RecurrenceType == 2 the controller always
overwrites month-day values with defaults from Weekday controls; change the
assignment so it only sets model.Item.RepeatOnWeek and model.Item.RepeatOnDay
when the posted weekday controls contain valid data (e.g.,
model.WeekdayOccurrence and model.WeekdayDayOfWeek are present/non-zero) or when
an explicit monthly-weekday mode flag is set; in practice update the block that
checks model.Item.RecurrenceType == 2 to additionally verify
model.WeekdayOccurrence/WeekdayDayOfWeek are valid (or a dedicated
monthly-weekday indicator) before assigning RepeatOnWeek and RepeatOnDay so
month-day mode isn’t overwritten.
| model.Item.RepeatOnDay = model.WeekdayDayOfWeek; | ||
| } | ||
|
|
||
| await _calendarService.UpdateCalendarItemAsync(model.Item, department.TimeZone, cancellationToken); |
There was a problem hiding this comment.
Mapped recurrence fields are likely not persisted by the update path.
Line 264 calls _calendarService.UpdateCalendarItemAsync(...), but Core/Resgrid.Services/CalendarService.cs update logic (context snippet Line 205-Line 221) does not copy RepeatOnWeek/RepeatOnDay onto the saved entity. This makes the new monthly weekday edit path effectively non-persistent.
Suggested service-side persistence patch
// Core/Resgrid.Services/CalendarService.cs (inside UpdateCalendarItemAsync)
+ calendarItem.RecurrenceType = item.RecurrenceType;
+ calendarItem.RepeatOnWeek = item.RepeatOnWeek;
+ calendarItem.RepeatOnDay = item.RepeatOnDay;
+ calendarItem.RepeatOnMonth = item.RepeatOnMonth;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs` at line 264,
The update path is not persisting mapped recurrence fields: when
CalendarController calls _calendarService.UpdateCalendarItemAsync(model.Item,
...) the CalendarService update logic must copy the recurrence properties from
the incoming item to the existing entity before saving; locate the
UpdateCalendarItemAsync update flow in Core/Resgrid.Services/CalendarService.cs
and add assignments to set existingEntity.RepeatOnWeek = item.RepeatOnWeek and
existingEntity.RepeatOnDay = item.RepeatOnDay (and any related recurrence fields
used by monthly-weekday edits) so those values are stored on save.
| var editPostCategories = await _contactsService.GetContactCategoriesForDepartmentAsync(DepartmentId); | ||
| ViewBag.Categories = new SelectList(editPostCategories, "ContactCategoryId", "Name"); | ||
|
|
There was a problem hiding this comment.
Missing selected value causes dropdown to reset on validation failure.
When the Edit POST action returns the view due to validation errors, the category dropdown will reset to "None" because the SelectList doesn't preserve the user's selection. Compare with the Edit GET action at line 430 which correctly passes the selected value.
🐛 Proposed fix to preserve selected category
var editPostCategories = await _contactsService.GetContactCategoriesForDepartmentAsync(DepartmentId);
-ViewBag.Categories = new SelectList(editPostCategories, "ContactCategoryId", "Name");
+ViewBag.Categories = new SelectList(editPostCategories, "ContactCategoryId", "Name", model.Contact.ContactCategoryId);📝 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 editPostCategories = await _contactsService.GetContactCategoriesForDepartmentAsync(DepartmentId); | |
| ViewBag.Categories = new SelectList(editPostCategories, "ContactCategoryId", "Name"); | |
| var editPostCategories = await _contactsService.GetContactCategoriesForDepartmentAsync(DepartmentId); | |
| ViewBag.Categories = new SelectList(editPostCategories, "ContactCategoryId", "Name", model.Contact.ContactCategoryId); | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/ContactsController.cs` around lines
462 - 464, In the ContactsController Edit POST action the SelectList assigned to
ViewBag.Categories (editPostCategories) doesn't include the currently selected
category, so on validation failure the dropdown resets; update the SelectList
creation in the Edit POST handler to pass the model's selected category id
(e.g., model.ContactCategoryId or contact.ContactCategoryId) as the
selectedValue parameter when constructing ViewBag.Categories so the user's
selection is preserved on redisplay.
| <span> | ||
| <span> | ||
| <input type="text" title="Day "> | ||
| @Html.TextBoxFor(m => m.Item.RepeatOnDay, new { onkeydown = "javascript:return false;" }) |
There was a problem hiding this comment.
Keyboard input is fully blocked for RepeatOnDay.
Line 259 disables all key presses, which blocks keyboard-only users from entering/changing the value. This is an accessibility blocker for that flow.
Suggested fix
- `@Html.TextBoxFor`(m => m.Item.RepeatOnDay, new { onkeydown = "javascript:return false;" })
+ `@Html.TextBoxFor`(m => m.Item.RepeatOnDay, new { type = "number", min = "1", max = "31", step = "1" })📝 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.
| @Html.TextBoxFor(m => m.Item.RepeatOnDay, new { onkeydown = "javascript:return false;" }) | |
| `@Html.TextBoxFor`(m => m.Item.RepeatOnDay, new { type = "number", min = "1", max = "31", step = "1" }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/Edit.cshtml` at line 259, The
TextBoxFor for m => m.Item.RepeatOnDay currently disables all keyboard input via
onkeydown="javascript:return false;", which blocks keyboard-only users; remove
that onkeydown attribute and instead constrain input in an accessible way (e.g.,
change the input to a numeric control using `@Html.TextBoxFor`(m =>
m.Item.RepeatOnDay, new { type = "number", min = "1", max = "31", inputmode =
"numeric" }) or apply readonly only when appropriate and expose change via an
accessible UI), and ensure any validation/formatting is enforced server- or
JS-side without preventing keyboard entry.
| // Default to "monthday" radio selected, weekday dropdowns disabled | ||
| $('input[name="month"][value="monthday"]').prop('checked', true).trigger('change'); | ||
|
|
There was a problem hiding this comment.
Initialization always forces monthday, which can clobber user intent after invalid POST.
Line 81 unconditionally resets mode to monthday. If the page is re-rendered with weekday values (e.g., validation errors), this will override the prior choice.
Suggested fix
- // Default to "monthday" radio selected, weekday dropdowns disabled
- $('input[name="month"][value="monthday"]').prop('checked', true).trigger('change');
+ // Preserve server-provided state when available; otherwise default to monthday
+ const hasWeekdaySelection = parseInt($('#WeekdayOccurrence').val(), 10) > 0;
+ $('input[name="month"][value="' + (hasWeekdaySelection ? 'weekday' : 'monthday') + '"]')
+ .prop('checked', true)
+ .trigger('change');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/calendar/resgrid.calendar.newEntry.js`
around lines 80 - 82, The script unconditionally forces the "monthday" radio via
$('input[name="month"][value="monthday"]').prop('checked',
true).trigger('change'); which clobbers a previously selected "weekday" when the
page is re-rendered after an invalid POST; change the initialization to only set
the default when no month-mode radio is already checked (i.e., check
$('input[name="month"]:checked') first) or when creating a brand-new entry, and
only then trigger the change so existing user selections are preserved; update
the logic in resgrid.calendar.newEntry.js around the month radio initialization
and the handler that enables/disables weekday dropdowns accordingly.
| $('#successArea').html('<p>' + (data.Message || 'Your geofence has been saved.') + '</p>'); | ||
| $('#successArea').show(); | ||
| $('#alertArea').hide(); | ||
| } else { | ||
| $('#alertArea').html('<p>' + (data && data.Message ? data.Message : 'Your geofence could not be saved.') + '</p>'); | ||
| $('#successArea').hide(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all SaveGeofence message assignment sites and whether any are user-influenced.
rg -nP "SaveGeofence\\(|model\\.Message\\s*=|class\\s+SaveGeofenceModel|\\bMessage\\b" -C3Repository: Resgrid/Core
Length of output: 50370
🏁 Script executed:
find . -type f -name "*.cs" | head -20Repository: Resgrid/Core
Length of output: 819
🏁 Script executed:
rg -l "SaveGeofence" --type csRepository: Resgrid/Core
Length of output: 176
🏁 Script executed:
rg "SaveGeofence|data.Message" --type cs -A 5 -B 5 | head -200Repository: Resgrid/Core
Length of output: 1812
🏁 Script executed:
cat -n Web/Resgrid.Web/Areas/User/Controllers/GroupsController.cs | sed -n '1,2000p' | grep -A 50 "public async Task<IActionResult> SaveGeofence"Repository: Resgrid/Core
Length of output: 2204
🏁 Script executed:
cat -n Web/Resgrid.Web/Areas/User/Controllers/GroupsController.cs | sed -n '616,640p'Repository: Resgrid/Core
Length of output: 1016
🏁 Script executed:
rg "model\.Message\s*=" --type cs -B 3 -A 1 | grep -A 5 -B 5 "SaveGeofence\|Geofence"Repository: Resgrid/Core
Length of output: 38
🏁 Script executed:
rg "Message" Web/Resgrid.Web/Areas/User/Models/Groups/SaveGeofenceModel.csRepository: Resgrid/Core
Length of output: 93
Use .text() instead of .html() to prevent potential DOM injection.
Lines 119 and 123 render the server message with $.html(). While the current message is hardcoded on the server, using .text() is a defensive practice to protect against future code changes that might introduce dynamic content.
Safer rendering
-$('#successArea').html('<p>' + (data.Message || 'Your geofence has been saved.') + '</p>');
+$('#successArea').text(data.Message || 'Your geofence has been saved.');
...
-$('#alertArea').html('<p>' + (data && data.Message ? data.Message : 'Your geofence could not be saved.') + '</p>');
+$('#alertArea').text(data && data.Message ? data.Message : 'Your geofence could not be saved.');📝 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.
| $('#successArea').html('<p>' + (data.Message || 'Your geofence has been saved.') + '</p>'); | |
| $('#successArea').show(); | |
| $('#alertArea').hide(); | |
| } else { | |
| $('#alertArea').html('<p>' + (data && data.Message ? data.Message : 'Your geofence could not be saved.') + '</p>'); | |
| $('#successArea').hide(); | |
| $('#successArea').text(data.Message || 'Your geofence has been saved.'); | |
| $('#successArea').show(); | |
| $('#alertArea').hide(); | |
| } else { | |
| $('#alertArea').text(data && data.Message ? data.Message : 'Your geofence could not be saved.'); | |
| $('#successArea').hide(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/wwwroot/js/app/internal/groups/resgrid.groups.geofence.js`
around lines 119 - 124, The code uses jQuery .html() to render server messages
into '#successArea' and '#alertArea' which risks DOM injection; update the
success handler where these are set (the block that currently calls
$('#successArea').html(...) and $('#alertArea').html(...)) to use .text()
instead of .html(), preserving the same message selection logic (data.Message ||
fallback and data && data.Message ? data.Message : fallback) and keep the
subsequent .show()/ .hide() calls unchanged.
| [HttpPost("VoiceWebhook")] | ||
| [AllowAnonymous] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<ContentResult> VoiceWebhook(string token, string Digits) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix this, add CSRF token validation to the POST handler by decorating VoiceWebhook with [ValidateAntiForgeryToken] from Microsoft.AspNetCore.Mvc. This is the minimal, direct fix aligned with the scanner recommendation and does not change endpoint business logic.
In Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestResponseController.cs, update the attribute block above VoiceWebhook (around lines 59–62) to include [ValidateAntiForgeryToken] immediately with the other action attributes. No new imports are required because Microsoft.AspNetCore.Mvc is already present and contains the attribute.
| @@ -58,6 +58,7 @@ | ||
| /// </summary> | ||
| [HttpPost("VoiceWebhook")] | ||
| [AllowAnonymous] | ||
| [ValidateAntiForgeryToken] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<ContentResult> VoiceWebhook(string token, string Digits) | ||
| { |
| [HttpPost("Save")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.CommunicationTest_Create)] | ||
| public async Task<SaveCommunicationTestResult> Save([FromBody] SaveCommunicationTestInput input, CancellationToken cancellationToken) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix this, add anti-forgery validation to the POST action so requests must include a valid CSRF token.
Best single fix here (without changing endpoint behavior): add [ValidateAntiForgeryToken] to the Save method directly in Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.cs, immediately above the method declaration alongside existing attributes. Since the file currently does not import the MVC ViewFeatures namespace where this attribute lives in ASP.NET Core, add:
using Microsoft.AspNetCore.Mvc.ViewFeatures;
This keeps scope tight to the flagged method and does not alter business logic.
| @@ -1,6 +1,7 @@ | ||
| using Microsoft.AspNetCore.Authorization; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.AspNetCore.Mvc; | ||
| using Microsoft.AspNetCore.Mvc.ViewFeatures; | ||
| using Resgrid.Framework; | ||
| using Resgrid.Model; | ||
| using Resgrid.Model.Events; | ||
| @@ -150,6 +151,7 @@ | ||
| /// Creates require CommunicationTest_Create; updates require CommunicationTest_Update. | ||
| /// </summary> | ||
| [HttpPost("Save")] | ||
| [ValidateAntiForgeryToken] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.CommunicationTest_Create)] | ||
| public async Task<SaveCommunicationTestResult> Save([FromBody] SaveCommunicationTestInput input, CancellationToken cancellationToken) |
| [HttpPost("StartRun")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.CommunicationTest_Create)] | ||
| public async Task<StartTestRunResult> StartRun(string testId, CancellationToken cancellationToken) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix this, add ASP.NET Core anti-forgery validation to the POST action by annotating StartRun with [ValidateAntiForgeryToken].
Best single change without altering behavior:
- File:
Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.cs - Region: around the
StartRunaction attributes (lines ~299–302 in your snippet) - Add
[ValidateAntiForgeryToken]directly aboveStartRunalongside existing attributes. - No new import is required because
ValidateAntiForgeryTokenis inMicrosoft.AspNetCore.Mvc, which is already imported in line 3.
| @@ -297,6 +297,7 @@ | ||
| /// Starts a new on-demand test run (admin only, rate limited to once per 48 hours) | ||
| /// </summary> | ||
| [HttpPost("StartRun")] | ||
| [ValidateAntiForgeryToken] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [Authorize(Policy = ResgridResources.CommunicationTest_Create)] | ||
| public async Task<StartTestRunResult> StartRun(string testId, CancellationToken cancellationToken) |
| /// </summary> | ||
| [HttpPost("RecordPushResponse")] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<RecordPushResponseResult> RecordPushResponse([FromBody] RecordPushResponseInput input) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
To fix this without changing business behavior, add ASP.NET Core antiforgery validation to the flagged POST action by applying [ValidateAntiForgeryToken] directly to RecordPushResponse. This ensures CSRF tokens are validated for this endpoint when called in cookie-authenticated/browser contexts.
Exact change in Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.cs:
- In the attribute block above
RecordPushResponse, add:[ValidateAntiForgeryToken]
- No new imports are required because
Microsoft.AspNetCore.Mvcis already imported.
This is the least invasive, targeted fix for the CodeQL finding.
| @@ -473,6 +473,7 @@ | ||
| /// Records a push notification response for a communication test | ||
| /// </summary> | ||
| [HttpPost("RecordPushResponse")] | ||
| [ValidateAntiForgeryToken] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<RecordPushResponseResult> RecordPushResponse([FromBody] RecordPushResponseInput input) | ||
| { |
| if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) | ||
| return Unauthorized(); | ||
|
|
||
| if (model.Test == null || model.Test.CommunicationTestId == Guid.Empty) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 3 days ago
Use a server-trusted identifier path for loading/authorizing the entity, and treat malformed/missing posted model data as validation failure (BadRequest) rather than a branch that influences later auth-related control flow.
Best fix in this file:
- Change POST
Editsignature to acceptstring testId(as the GET already does). - Parse/validate
testIdviaGuid.TryParse; if invalid, returnBadRequest(). - Load
existingusing parsedid(trusted route/query input shape validation), then perform existing server-side auth checks (existing == null || existing.DepartmentId != DepartmentId => Unauthorized()). - Keep model checks only as non-auth validation (
if (model?.Test == null) return BadRequest();), and remove reliance onmodel.Test.CommunicationTestIdfor selecting the target test.
This addresses both alert variants at the same location by removing the tainted condition that guards a path toward the sensitive authorization action.
| @@ -141,15 +141,18 @@ | ||
|
|
||
| [HttpPost] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<IActionResult> Edit(EditCommunicationTestView model, CancellationToken cancellationToken) | ||
| public async Task<IActionResult> Edit(string testId, EditCommunicationTestView model, CancellationToken cancellationToken) | ||
| { | ||
| if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) | ||
| return Unauthorized(); | ||
|
|
||
| if (model.Test == null || model.Test.CommunicationTestId == Guid.Empty) | ||
| return RedirectToAction("Index"); | ||
| if (!Guid.TryParse(testId, out var id)) | ||
| return BadRequest(); | ||
|
|
||
| var existing = await _communicationTestService.GetTestByIdAsync(model.Test.CommunicationTestId); | ||
| if (model?.Test == null) | ||
| return BadRequest(); | ||
|
|
||
| var existing = await _communicationTestService.GetTestByIdAsync(id); | ||
| if (existing == null || existing.DepartmentId != DepartmentId) | ||
| return Unauthorized(); | ||
|
|
| if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) | ||
| return Unauthorized(); | ||
|
|
||
| if (model.Test == null || model.Test.CommunicationTestId == Guid.Empty) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Copilot Autofix
AI 3 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (16)
Web/Resgrid.Web/Areas/User/Views/Forms/Index.cshtml-31-33 (1)
31-33:⚠️ Potential issue | 🟡 MinorLocalize the new deprecation banner copy.
Line 32 hard-codes English text while the rest of the page uses localized resources. This will show mixed-language UI in localized deployments. Please move this message (including link label) to
localizerresource keys.Suggested change
- <div class="alert alert-warning"> - <strong>Deprecated:</strong> Call Forms are deprecated and will be removed in a future release. Please use <a asp-controller="UserDefinedFields" asp-action="Index" asp-route-area="User"><strong>User Defined Fields (Custom Fields)</strong></a> instead. - </div> + <div class="alert alert-warning"> + <strong>@localizer["DeprecatedLabel"]</strong> + `@localizer`["CallFormsDeprecatedMessage"] + <a asp-controller="UserDefinedFields" asp-action="Index" asp-route-area="User"> + <strong>@localizer["UserDefinedFieldsLinkLabel"]</strong> + </a> + `@localizer`["CallFormsDeprecatedSuffix"] + </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Forms/Index.cshtml` around lines 31 - 33, The deprecation banner in Index.cshtml contains hard-coded English text and a hard-coded link label; replace these with localized resource keys via the page's IViewLocalizer/localizer usage so the banner reads from resources instead of inline text. Update the alert div that currently contains "<strong>Deprecated:</strong> Call Forms are deprecated..." and the anchor with asp-controller="UserDefinedFields" asp-action="Index" asp-route-area="User" to use localizer keys (e.g., "FormsDeprecated_Title", "FormsDeprecated_Message" and "FormsDeprecated_LinkLabel") and ensure the message composes the localized title, localized body and localized link label; add those keys to the resource file for this view.Core/Resgrid.Config/InfoConfig.cs-64-67 (1)
64-67:⚠️ Potential issue | 🟡 MinorHarden logon URL construction against misconfiguration.
GetLogonUrl()can produce invalid routing (/Account/LogOnwhenAppUrlis null/empty, or double slashes with trailing/). Validate and normalize before composing the URL.Proposed fix
public string GetLogonUrl() { - return AppUrl + "/Account/LogOn"; + if (string.IsNullOrWhiteSpace(AppUrl)) + throw new global::System.InvalidOperationException($"{nameof(AppUrl)} is not configured for location '{Name}'."); + + return $"{AppUrl.TrimEnd('/')}/Account/LogOn"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Config/InfoConfig.cs` around lines 64 - 67, GetLogonUrl can return invalid paths when AppUrl is null/empty or has trailing slashes; update GetLogonUrl to (1) guard for null/empty AppUrl and return a safe relative path like "/Account/LogOn", (2) normalize AppUrl by trimming any trailing '/' before concatenation, and (3) ensure the final composed URL includes exactly one '/' between the base and "Account/LogOn". Modify the GetLogonUrl method to implement these checks and normalization (use AppUrl and GetLogonUrl as reference points).Core/Resgrid.Services/SmsService.cs-254-269 (1)
254-269:⚠️ Potential issue | 🟡 MinorFix protocol text assembly in cancellation SMS.
Line 264 appends
protocolinstead of the accumulatedprotocolsstring, so the second and later entries stringify the object type instead of preserving prior protocol codes.Suggested fix
if (call.Protocols != null && call.Protocols.Any()) { string protocols = String.Empty; foreach (var protocol in call.Protocols) { if (!String.IsNullOrWhiteSpace(protocol.Data)) { if (String.IsNullOrWhiteSpace(protocols)) protocols = protocol.Data; else - protocols = protocol + "," + protocol.Data; + protocols = protocols + "," + protocol.Data; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/SmsService.cs` around lines 254 - 269, The protocol-assembly logic in SmsService.cs incorrectly appends the loop variable object instead of the accumulated string: inside the loop over call.Protocols (the block building the protocols string) replace the line that currently sets protocols = protocol + "," + protocol.Data with code that appends the new protocol.Data to the existing protocols string (e.g., protocols = protocols + "," + protocol.Data or using string.Concat/Interpolation) so subsequent entries concatenate the prior protocols value plus the new protocol.Data; keep the existing null/whitespace checks for protocol.Data and the final check that appends protocols to text.Core/Resgrid.Services/EmailService.cs-306-318 (1)
306-318:⚠️ Potential issue | 🟡 MinorFix protocol concatenation before sending cancellation emails.
Line 314 resets the accumulator with
protocol + "," + protocol.Data; with multiple protocols that produces an object type name instead of a comma-separated list of protocol data.Suggested fix
if (call.Protocols != null && call.Protocols.Any()) { string protocols = String.Empty; foreach (var protocol in call.Protocols) { if (String.IsNullOrWhiteSpace(protocols)) protocols = protocol.Data; else - protocols = protocol + "," + protocol.Data; + protocols = protocols + "," + protocol.Data; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/EmailService.cs` around lines 306 - 318, In EmailService.cs where cancellation email body is built (the block reading call.Protocols and populating the local variable protocols), fix the concatenation bug: the loop currently does protocols = protocol + "," + protocol.Data which uses the loop variable object instead of the accumulator and yields an object type name; change it to append to the accumulator (e.g., protocols = protocols + "," + protocol.Data) or replace the whole loop with a String.Join over call.Protocols.Select(p => p.Data), then use the resulting protocols string when building natureOfCall.Core/Resgrid.Services/CommunicationService.cs-407-408 (1)
407-408:⚠️ Potential issue | 🟡 MinorUse the resolved
addressfor SMS too.Push uses the override
address, but SMS still sendscall.Address. If the caller supplied a formatted/derived address, cancellation details diverge across channels and SMS can lose the location entirely.Proposed fix
- await _smsService.SendCancelCallAsync(call, dispatch, departmentNumber, departmentId, profile, call.Address, payment); + await _smsService.SendCancelCallAsync(call, dispatch, departmentNumber, departmentId, profile, address ?? call.Address, payment);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/CommunicationService.cs` around lines 407 - 408, The SMS cancellation is using call.Address instead of the resolved address variable that was used for push notifications; update the call to _smsService.SendCancelCallAsync so it passes the resolved address variable (address) instead of call.Address, keeping payment from _subscriptionsService.GetCurrentPaymentForDepartmentAsync and the other parameters the same; modify the argument list in the method invocation around SendCancelCallAsync to use address to ensure SMS and push use the same derived/formatted location.Web/Resgrid.Web.Services/Models/v4/CommunicationTests/SaveCommunicationTestResult.cs-11-11 (1)
11-11:⚠️ Potential issue | 🟡 MinorHarden the response contract for
Id.
Idis nullable by default; if assignment is missed upstream, clients get a null identifier in a “save succeeded” response.Suggested hardening
-public string Id { get; set; } +public string Id { get; set; } = string.Empty;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Models/v4/CommunicationTests/SaveCommunicationTestResult.cs` at line 11, The response model's Id property in SaveCommunicationTestResult is nullable by default which can return null to clients; update the SaveCommunicationTestResult.Id declaration to be non-nullable and guaranteed set (e.g., change to a non-nullable string and provide a safe default or apply a [Required] / validation check), and ensure any code that constructs SaveCommunicationTestResult (e.g., callers that set Id) assigns a valid identifier so the response never contains a null Id.Web/Resgrid.Web/Areas/User/Views/Reports/Index.cshtml-28-28 (1)
28-28:⚠️ Potential issue | 🟡 MinorLocalize the new button label.
Line 28 hard-codes
"Communication Tests"on a localized page. Please move this to resource localization like the rest of the view.💡 Proposed fix
- <a class="btn btn-success" asp-controller="CommunicationTest" asp-action="Index" asp-route-area="User">Communication Tests</a> + <a class="btn btn-success" asp-controller="CommunicationTest" asp-action="Index" asp-route-area="User">@localizer["CommunicationTests"]</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Reports/Index.cshtml` at line 28, Replace the hard-coded "Communication Tests" label inside the anchor element (the <a class="btn btn-success" asp-controller="CommunicationTest" asp-action="Index" asp-route-area="User"> element) with the same localization helper used elsewhere in this view (e.g., the existing IViewLocalizer/localization helper), and add a new resource key like "CommunicationTests" to the view/resource file with appropriate localized strings for each culture; ensure the view uses that key (e.g., Localizer["CommunicationTests"] or your project's equivalent) so the button text is localized consistently.Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestResponseController.cs-62-75 (1)
62-75:⚠️ Potential issue | 🟡 MinorVoiceWebhook returns misleading success message when no recording occurs.
The endpoint always responds with "Your response has been recorded" even when:
tokenis empty/nullDigitsis not "1"RecordVoiceResponseAsyncreturns false (already responded or invalid token)This could confuse users who think their response was captured when it wasn't.
♻️ Proposed fix
[HttpPost("VoiceWebhook")] [AllowAnonymous] [ProducesResponseType(StatusCodes.Status200OK)] public async Task<ContentResult> VoiceWebhook(string token, string Digits) { + var recorded = false; if (!string.IsNullOrWhiteSpace(token) && Digits == "1") { - await _communicationTestService.RecordVoiceResponseAsync(token); + recorded = await _communicationTestService.RecordVoiceResponseAsync(token); } + var message = recorded + ? "Thank you. Your response has been recorded." + : "We could not record your response. Please try again or contact support."; + return new ContentResult { - Content = "<?xml version=\"1.0\" encoding=\"UTF-8\"?><Response><Say>Thank you. Your response has been recorded.</Say><Hangup/></Response>", + Content = $"<?xml version=\"1.0\" encoding=\"UTF-8\"?><Response><Say>{message}</Say><Hangup/></Response>", ContentType = "application/xml", StatusCode = 200 }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestResponseController.cs` around lines 62 - 75, VoiceWebhook always returns a success message even when no recording happens; change it to check inputs and the service result: in VoiceWebhook capture the boolean result of await _communicationTestService.RecordVoiceResponseAsync(token) and only return the "<Say>Thank you. Your response has been recorded.</Say>" XML when token is not null/empty, Digits == "1", and the service returned true; otherwise return an alternate XML response (e.g., "<Say>No response was recorded.</Say><Hangup/>") and appropriate 200 status to accurately reflect no recording occurred. Ensure you reference the VoiceWebhook method and _communicationTestService.RecordVoiceResponseAsync(token) when implementing the branch.Web/Resgrid.Web/Areas/User/Views/CommunicationTest/New.cshtml-144-149 (1)
144-149:⚠️ Potential issue | 🟡 MinorChange the time picker format to a valid format supported by the library.
The format
'h:i A'is not supported by the xdsoft jquery-datetimepicker library (v2.5.4). Use'H:i'for 24-hour format (consistent with other uses in the codebase like shifts) or verify if 12-hour format with AM/PM should use the correct moment.js format'h:mm A'instead of'h:i A'. The current format may fail to parse times correctly on form submission.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/CommunicationTest/New.cshtml` around lines 144 - 149, The datetimepicker initialization for '#timePicker' uses an unsupported format 'h:i A'; update the configuration in the $('#timePicker').datetimepicker({...}) call to a valid xdsoft datetimepicker format—replace both format and formatTime with 'H:i' (24-hour, consistent with shifts) or if 12-hour AM/PM behavior is required use 'h:mm A' instead—ensure the step: 15 setting remains and test form submission parsing after the change.Web/Resgrid.Web/Areas/User/Views/CommunicationTest/Edit.cshtml-5-13 (1)
5-13:⚠️ Potential issue | 🟡 MinorWeekday state is unintentionally forced to Monday
Line [5] defaults
selectedDayto Monday. Then Line [173] runssyncDayRadios()on load, which writes Monday=true into hidden fields even when the test is not weekly or had no weekday selected.🛠️ Suggested change
- var selectedDay = "Monday"; + string selectedDay = null; if (Model.Test.Sunday) selectedDay = "Sunday"; else if (Model.Test.Monday) selectedDay = "Monday"; else if (Model.Test.Tuesday) selectedDay = "Tuesday"; else if (Model.Test.Wednesday) selectedDay = "Wednesday"; else if (Model.Test.Thursday) selectedDay = "Thursday"; else if (Model.Test.Friday) selectedDay = "Friday"; else if (Model.Test.Saturday) selectedDay = "Saturday";function syncDayRadios() { var days = ['Sunday','Monday','Tuesday','Wednesday','Thursday','Friday','Saturday']; + if ($('#scheduleType').val() !== '1') { + days.forEach(function(d) { $('#hdn' + d).val('false'); }); + return; + } var selected = $('input[name="DayOfWeek"]:checked').val(); days.forEach(function(d) { $('#hdn' + d).val(d === selected ? 'true' : 'false'); }); }Also applies to: 164-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/CommunicationTest/Edit.cshtml` around lines 5 - 13, The code forces selectedDay to "Monday" by default which makes syncDayRadios() write Monday=true incorrectly; change the logic in Edit.cshtml so selectedDay is initialized to an empty string (or null) and only set when one of Model.Test.Sunday/ Monday/Tuesday/Wednesday/Thursday/Friday/Saturday is true (preserve the existing if/else order but remove the unconditional default), and ensure syncDayRadios() and any hidden-field writes check that selectedDay is non-empty (or that the test is weekly) before marking a day true; refer to the selectedDay variable and the syncDayRadios() invocation and the Model.Test.Sunday..Saturday properties to locate and fix the code.Core/Resgrid.Services/CommunicationTestService.cs-356-371 (1)
356-371:⚠️ Potential issue | 🟡 MinorSame TOCTOU race condition exists in token-based response recording.
The
RecordResponseByTokenAsyncmethod has the same check-then-act pattern as SMS recording. Line 359 checksresult.Responded, but a concurrent request could update the record between the check and the save at line 364.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/CommunicationTestService.cs` around lines 356 - 371, RecordResponseByTokenAsync has a TOCTOU race: it reads result via GetResultByResponseTokenAsync, checks result.Responded, then updates—allowing a concurrent update to slip in between; fix by performing the check-and-set atomically in the data layer (e.g., add a repository method like TryMarkResponseByTokenAsync or an atomic DB update/WHERE clause that sets Responded = true and RespondedOn = UTC only when Responded = false and Channel matches and returns whether it succeeded), call that new atomic method from RecordResponseByTokenAsync (using the same responseToken and channel) and only when it returns true then fetch the run and call UpdateRunResponseCountAsync.Core/Resgrid.Services/CommunicationTestService.cs-262-286 (1)
262-286:⚠️ Potential issue | 🟡 MinorRace condition in SMS response recording.
The check at line 273 (
!r.Responded) followed by the update at lines 280-282 creates a time-of-check-to-time-of-use (TOCTOU) race condition. Two concurrent SMS responses from the same phone number could both pass the!r.Respondedcheck before either updates the database. While the business impact is low (duplicate response recording), this could inflateTotalResponsescounts.Consider using optimistic concurrency (row versioning) or a database-level constraint to prevent duplicate responses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/CommunicationTestService.cs` around lines 262 - 286, RecordSmsResponseAsync has a TOCTOU race: checking !r.Responded in-memory then saving can let two threads both mark the same result responded and double-count responses; change the flow to perform the “mark responded” as an atomic DB operation instead of read-then-write in-memory — add or use a repository method (e.g., on _communicationTestResultRepository) like MarkResultRespondedIfNotAsync(communicationTestResultId, respondedOn) that issues a single UPDATE ... WHERE CommunicationTestResultId = `@id` AND Responded = 0 and returns rowsAffected, or implement optimistic concurrency using a rowversion/timestamp on the CommunicationTestResult entity and catch concurrency exceptions on SaveOrUpdateAsync, only calling UpdateRunResponseCountAsync when the update actually succeeded; update RecordSmsResponseAsync to call that new repository method (or handle DbUpdateConcurrencyException) and only call UpdateRunResponseCountAsync when the DB confirmed the change.Web/Resgrid.Web/Areas/User/Controllers/CommunicationTestController.cs-219-234 (1)
219-234:⚠️ Potential issue | 🟡 MinorSend audit event after successful deletion.
The audit event is sent before
DeleteTestAsyncis called (line 231). If the deletion fails, the audit log will incorrectly indicate the test was deleted. Move the audit event after the successful delete operation.🔧 Proposed fix
var test = await _communicationTestService.GetTestByIdAsync(id); if (test == null || test.DepartmentId != DepartmentId) return Unauthorized(); +var beforeJson = test.CloneJsonToString(); +await _communicationTestService.DeleteTestAsync(id, cancellationToken); + _eventAggregator.SendMessage<AuditEvent>(new AuditEvent { DepartmentId = DepartmentId, UserId = UserId, Type = AuditLogTypes.CommunicationTestDeleted, - Before = test.CloneJsonToString(), + Before = beforeJson, Successful = true, IpAddress = IpAddressHelper.GetRequestIP(Request, true), ServerName = Environment.MachineName, UserAgent = $"{Request.Headers["User-Agent"]} {Request.Headers["Accept-Language"]}" }); -await _communicationTestService.DeleteTestAsync(id, cancellationToken); - return RedirectToAction("Index");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/CommunicationTestController.cs` around lines 219 - 234, The audit event is currently sent before calling _communicationTestService.DeleteTestAsync(id, cancellationToken) which can log a deletion that may fail; move the _eventAggregator.SendMessage<AuditEvent>(...) call so it executes only after await _communicationTestService.DeleteTestAsync(...) completes successfully (i.e., after the await and before return RedirectToAction("Index")), keeping the same AuditEvent properties (DepartmentId, UserId, Type = AuditLogTypes.CommunicationTestDeleted, Before = test.CloneJsonToString(), Successful = true, IpAddress, ServerName, UserAgent) and ensuring the event uses the same Request/Environment helpers as before.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml-331-366 (1)
331-366:⚠️ Potential issue | 🟡 MinorDocument the
CallVideoFeedsrequest/response models too.These controller entries are present, but this XML still has no
Resgrid.Web.Services.Models.v4.CallVideoFeeds.*members. That leaves the new save/edit/list schemas with little or no field-level description in generated API docs, even though the controller accepts and returns structured video-feed payloads.🤖 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 331 - 366, Add XML documentation members for the request/response models under Resgrid.Web.Services.Models.v4.CallVideoFeeds (e.g., SaveCallVideoFeedInput, EditCallVideoFeedInput, CallVideoFeedDto/Response and any list/collection types) so the API docs show field-level descriptions; for each model (class) add a <member name="T:Resgrid.Web.Services.Models.v4.CallVideoFeeds.{ClassName}"> summary describing the model and then <member> entries for each property using names like "P:Resgrid.Web.Services.Models.v4.CallVideoFeeds.{ClassName}.{PropertyName}" with concise <summary> descriptions and parameter details where applicable so the SaveCallVideoFeed and EditCallVideoFeed controller docs reference fully documented payload schemas.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml-5697-5821 (1)
5697-5821:⚠️ Potential issue | 🟡 MinorThe communication-test model docs are still too sparse for a public API.
Most of the new entries here stop at type-level summaries.
CommunicationTestData,TestRunData, and much ofSaveCommunicationTestInputstill lack property-level docs, so callers won't see meanings for schedule flags, channel toggles, run metadata, and similar fields in the generated schema.🤖 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 5697 - 5821, Add missing XML documentation for the public properties on the communication-test models so the generated API schema is complete: update Resgrid.Web.Services.Models.v4.CommunicationTests.CommunicationTestData to document each property (e.g., schedule flags like ScheduleType/DayOfMonth/Time/ResponseWindowMinutes and channel toggles such as SmsEnabled/EmailEnabled/VoiceEnabled/PushEnabled), document TestRunData properties (e.g., RunId, StartedAt, CompletedAt, Status, SuccessCount, FailureCount, RunCode) and document all properties on SaveCommunicationTestInput (Id, Name, Description, ScheduleType, DayOfMonth, Time, ResponseWindowMinutes, enabled channels and any recipient/filter fields) with concise meanings, allowable values and defaults so callers understand schedule semantics, channel behavior and run metadata; ensure you add <summary> (and where relevant <remarks>) tags for each property/field referenced above.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml-427-486 (1)
427-486:⚠️ Potential issue | 🟡 MinorAdd parameter docs for the new communication-test endpoints.
The summaries were added, but the generated XML still has no
<param>descriptions for inputs liketoken,Digits, and the various string ids used byGet,StartRun,GetRuns, andGetReport. For public v4 API docs, those unlabeled parameters make the endpoints much harder to consume.🤖 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 427 - 486, The XML docs are missing <param> entries for several v4 API endpoints; update Resgrid.Web.Services.xml to add descriptive <param name="..."> tags for CommunicationTestResponseController.EmailConfirm(token) (explain token usage), CommunicationTestResponseController.VoiceWebhook(token, Digits) (describe token and Digits meaning), CommunicationTestsController.Get(id), Delete(id, cancellationToken), StartRun(id, cancellationToken), GetRuns(id), GetReport(id) (describe each id as the communication test or run identifier), Save(SaveCommunicationTestInput, cancellationToken) (document the SaveCommunicationTestInput payload), and RecordPushResponse(RecordPushResponseInput) (document the input model), ensuring each parameter name matches the method signatures and gives a one-line description of purpose/format for public v4 API consumers.
🧹 Nitpick comments (18)
Core/Resgrid.Config/InfoConfig.cs (1)
60-60: Prefer immutable config values forAppUrl.Since this is shared configuration state, make
AppUrlinit-only to avoid runtime mutation after bootstrap.Proposed refactor
-public string AppUrl { get; set; } +public string AppUrl { get; init; }As per coding guidelines, "Prefer functional patterns and immutable data where appropriate in C#".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Config/InfoConfig.cs` at line 60, Make the AppUrl property on InfoConfig immutable by changing its setter to an init-only setter so it cannot be mutated at runtime; locate the InfoConfig class and replace the mutable property declaration for AppUrl with an init-only property, and if any code mutates InfoConfig.AppUrl at runtime (e.g., assignments in startup/bootstrap), update that code to set the value only during object construction or configuration binding (e.g., when deserializing or using configuration binding) rather than after initialization.Core/Resgrid.Services/CommunicationService.cs (1)
315-424: Extract sharedStandardPushCallconstruction into a helper.These two methods duplicate most of the existing call/unit push-building logic, and the copies are already drifting. Pulling the address resolution and sanitization into a small helper would reduce regression risk and make this path much easier to test.
As per coding guidelines, "Prefer pure methods over methods with side effects" and "Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions".
Also applies to: 426-495
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/CommunicationService.cs` around lines 315 - 424, The SendCancelCallAsync method duplicates complex StandardPushCall construction and sanitization; extract that logic into a pure, testable helper (e.g., BuildStandardPushCall) used by SendCancelCallAsync and the similar method around lines 426-495. The helper should accept explicit inputs (Call call, CallDispatch dispatch, int departmentId, string address, UserProfile profile, CallPriority callPriority) and return a fully populated StandardPushCall (resolving address/GeoLocation via an injected IGeoLocationProvider passed into the helper, applying StringHelpers.StripHtmlTagsCharArray, the Regex URL removal, color fallback, Truncate, and slash replacement). Update SendCancelCallAsync to call the helper and then call _pushService.PushCall(spc, ...); ensure the helper has no hidden side-effects so it can be unit-tested.Core/Resgrid.Localization/Areas/User/CommunicationTest/CommunicationTest.cs (1)
3-5: Consider a more explicit marker type name.Using
CommunicationTesthere can be confused withResgrid.Model.CommunicationTest. A suffix likeCommunicationTestResources/CommunicationTestResourceMarkerwould reduce ambiguity.As per coding guidelines, "Use meaningful, descriptive names for types, methods, and parameters; avoid unclear abbreviations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Localization/Areas/User/CommunicationTest/CommunicationTest.cs` around lines 3 - 5, Rename the empty marker class CommunicationTest to a more explicit, unambiguous name such as CommunicationTestResources or CommunicationTestResourceMarker to avoid confusion with Resgrid.Model.CommunicationTest; update the class declaration (the CommunicationTest type) and any usages/imports that reference it (e.g., localization resource lookups or dependency registrations) so all references point to the new name, and ensure the namespace remains correct for resource discovery.Web/Resgrid.Web/Areas/User/Models/CommunicationTests/CommunicationTestIndexView.cs (1)
10-10: PreferGuidkeys forTestNamesinstead ofstring.Line 10 stores test ids as strings, which forces repeated
ToString()conversions and weakens type safety. UsingDictionary<Guid, string>keeps id handling consistent and safer.💡 Proposed fix
+using System; using System.Collections.Generic; using Resgrid.Model; @@ - public Dictionary<string, string> TestNames { get; set; } = new Dictionary<string, string>(); + public Dictionary<Guid, string> TestNames { get; set; } = new Dictionary<Guid, string>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Models/CommunicationTests/CommunicationTestIndexView.cs` at line 10, Change the TestNames property on CommunicationTestIndexView from Dictionary<string, string> to Dictionary<Guid, string> and update all usages to use Guid keys (remove unnecessary ToString()/parse calls), e.g., where code currently constructs or looks up entries using test id strings, use Guid values or parse once at input boundaries; ensure any model binding/serialization points (views or JSON) that expect string ids are adapted to accept/emit Guid values consistently.Web/Resgrid.Web.Services/Models/v4/CommunicationTests/RecordPushResponseInput.cs (1)
6-12: Enforce non-emptyResponseTokenat the API boundary.Line 11 currently allows null/empty token values. Add validation attributes so invalid payloads fail fast before reaching service/repository layers.
💡 Proposed fix
+using System.ComponentModel.DataAnnotations; + public class RecordPushResponseInput { /// <summary> /// The response token from the push notification /// </summary> - public string ResponseToken { get; set; } + [Required(AllowEmptyStrings = false)] + [MinLength(1)] + public string ResponseToken { get; set; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Models/v4/CommunicationTests/RecordPushResponseInput.cs` around lines 6 - 12, The RecordPushResponseInput model currently allows null/empty ResponseToken; update the class to enforce non-empty tokens at the API boundary by applying DataAnnotations (e.g. add [Required] and either [MinLength(1)] or [StringLength(1, int.MaxValue)] to the ResponseToken property) and import System.ComponentModel.DataAnnotations so model binding/validation rejects empty payloads before hitting service/repository layers; ensure the attribute names are applied to the ResponseToken property in the RecordPushResponseInput class.Web/Resgrid.Web/Areas/User/Models/CommunicationTests/EditCommunicationTestView.cs (1)
7-8: Consider using an explicit edit DTO with only editable fields instead of exposing the full domain entity.While the controller properly whitelists which fields are updated (lines 167–184), exposing the full
CommunicationTestentity in the view model couples presentation to the domain model. An explicit edit DTO (e.g.,EditCommunicationTestRequest) containing onlyName,Description,ScheduleType, day flags,Time, and test channel flags would better separate concerns and clarify the public API surface per the coding guidelines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Models/CommunicationTests/EditCommunicationTestView.cs` around lines 7 - 8, Replace the exposed domain entity on the view model — currently the CommunicationTest Test property in EditCommunicationTestView — with a focused edit DTO (e.g., EditCommunicationTestRequest) that contains only editable fields: Name, Description, ScheduleType, the day flags, Time, and the test channel flags; update the view model to use this DTO instead of CommunicationTest and adjust the controller action that accepts/whitelists updates to bind from EditCommunicationTestRequest and map those fields onto the domain CommunicationTest inside the controller (preserving the existing whitelist logic).Web/Resgrid.Web.Services/Controllers/SignalWireController.cs (1)
115-131: Same error handling concern as in TwilioController.The early return for CT- prefix handling bypasses the
try-catch-finallyblock at Lines 133-501. IfRecordSmsResponseAsyncthrows, the message event won't be saved and the exception will propagate. Consider wrapping theRecordSmsResponseAsynccall in a try-catch for consistency with the rest of the method.♻️ Suggested improvement
// Check for Communication Test response (CT- prefix) if (!string.IsNullOrWhiteSpace(textMessage.Text) && textMessage.Text.Trim().StartsWith("CT-", StringComparison.OrdinalIgnoreCase)) { var runCode = textMessage.Text.Trim().Split(' ')[0].ToUpperInvariant(); - await _communicationTestService.RecordSmsResponseAsync(runCode, textMessage.Msisdn); - messageEvent.Processed = true; + try + { + await _communicationTestService.RecordSmsResponseAsync(runCode, textMessage.Msisdn); + messageEvent.Processed = true; + } + catch (Exception ex) + { + Framework.Logging.LogException(ex); + } response = LaMLResponse.Message.Respond("Resgrid received your communication test response. Thank you."); await _numbersService.SaveInboundMessageEventAsync(messageEvent); return new ContentResult { Content = response, ContentType = "application/xml", StatusCode = 200 }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/SignalWireController.cs` around lines 115 - 131, The CT- prefix branch currently calls _communicationTestService.RecordSmsResponseAsync and returns early, which can bypass the surrounding try/catch/finally and skip saving messageEvent; wrap the RecordSmsResponseAsync call in a try-catch (catching Exception) mirroring the method's other handlers, ensure messageEvent.Processed = true is set, always call _numbersService.SaveInboundMessageEventAsync(messageEvent) before returning, and build the LaMLResponse.Message.Respond/ContentResult only after the save so failures in RecordSmsResponseAsync are logged/handled consistently with the rest of the method.Web/Resgrid.Web.Services/Controllers/TwilioController.cs (1)
103-119: Consider adding error handling for the communication test recording.If
RecordSmsResponseAsyncthrows an exception, the inbound message event won't be saved (Lines 112-118 would be skipped), and the user won't receive acknowledgment. The rest of the method has atry-catch-finallypattern that ensuresSaveInboundMessageEventAsyncis always called.♻️ Suggested improvement
// Check for Communication Test response (CT- prefix) if (!string.IsNullOrWhiteSpace(textMessage.Text) && textMessage.Text.Trim().StartsWith("CT-", StringComparison.OrdinalIgnoreCase)) { var runCode = textMessage.Text.Trim().Split(' ')[0].ToUpperInvariant(); - await _communicationTestService.RecordSmsResponseAsync(runCode, textMessage.Msisdn); - messageEvent.Processed = true; + try + { + await _communicationTestService.RecordSmsResponseAsync(runCode, textMessage.Msisdn); + messageEvent.Processed = true; + } + catch (Exception ex) + { + Framework.Logging.LogException(ex); + } response.Message("Resgrid received your communication test response. Thank you."); await _numbersService.SaveInboundMessageEventAsync(messageEvent); return new ContentResult { Content = response.ToString(), ContentType = "application/xml", StatusCode = 200 }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/TwilioController.cs` around lines 103 - 119, Wrap the call to _communicationTestService.RecordSmsResponseAsync (inside the CT- handling block) in a try-catch so any exception is caught and handled; on success keep the existing behavior (set messageEvent.Processed = true, set the confirmation via response.Message and save via _numbersService.SaveInboundMessageEventAsync), and on exception log the error and still set an appropriate user-facing response (e.g. error/acknowledgement) and ensure _numbersService.SaveInboundMessageEventAsync(messageEvent) is always awaited (either in a finally or after the catch) so the inbound event is persisted and the caller always receives a response.Web/Resgrid.Web/Areas/User/Views/CommunicationTest/Report.cshtml (1)
143-173: Extract helper functions outside the foreach loop.The helper functions (
GetStatusClass,GetStatusText,GetVerificationText,GetEnabledText,GetEnabledClass) are re-declared on each iteration of theforeachloop, which is inefficient and reduces readability. Move them to a@functionsblock at the top of the file.♻️ Proposed refactor
Add this block after the
@{ ... }section at the top:`@functions` { string GetStatusClass(CommunicationTestResult r) { if (r == null) return ""; if (!r.SendAttempted) return "background-color:`#e0e0e0`;"; if (r.Responded) return "background-color:`#dff0d8`;"; return "background-color:`#f2dede`;"; } string GetStatusText(CommunicationTestResult r) { if (r == null) return "-"; if (!r.SendAttempted) return "Not Sent"; if (r.Responded) return "Responded"; return "No Response"; } string GetVerificationText(CommunicationTestResult r) { if (r == null) return "-"; return ((ContactVerificationStatus)r.VerificationStatus).ToString(); } string GetEnabledText(bool enabled) => enabled ? "Yes" : "No"; string GetEnabledClass(bool enabled) => enabled ? "color:green;" : "color:red;"; }Then remove the local function declarations from inside the foreach loop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/CommunicationTest/Report.cshtml` around lines 143 - 173, Move the repeated local helper functions GetStatusClass, GetStatusText, GetVerificationText, GetEnabledText, and GetEnabledClass out of the foreach and into a single `@functions` block near the top of the Razor file (after the initial @{ ... } block); implement each helper there (preserving their logic) and then remove the declarations inside the foreach so the loop calls the top-level helpers instead.Workers/Resgrid.Workers.Console/Tasks/CommunicationTestTask.cs (1)
18-18: Make_loggerfield private and readonly.The logger field is currently public and mutable, which violates encapsulation principles.
🔧 Suggested fix
- public ILogger _logger; + private readonly ILogger _logger;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Workers/Resgrid.Workers.Console/Tasks/CommunicationTestTask.cs` at line 18, The _logger field in CommunicationTestTask is currently public and mutable; make it private and readonly (private readonly ILogger _logger) and assign it only via the class constructor (or dependency injection initializer) so existing usages inside CommunicationTestTask continue to work; update any external references to use a public property or constructor injection instead of directly accessing _logger and ensure no other code assigns to _logger outside the constructor.Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestResponseController.cs (1)
27-54: Consider rate limiting on anonymous endpoints.Both
EmailConfirmandVoiceWebhookare[AllowAnonymous]endpoints that accept tokens. Without rate limiting, an attacker could potentially enumerate valid tokens through rapid requests. Consider adding rate limiting middleware or attributes to these endpoints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestResponseController.cs` around lines 27 - 54, The EmailConfirm and VoiceWebhook endpoints (methods EmailConfirm and VoiceWebhook in CommunicationTestResponseController) are AllowAnonymous and vulnerable to token enumeration; add rate limiting to these actions by applying the project's rate limiting attribute or middleware (e.g., a [RateLimit] or custom policy) or configure endpoint-specific limits in the rate limiter middleware, ensuring limits apply to anonymous requests and are tuned to block high-frequency requests while allowing legitimate traffic; also log and return appropriate status (429) when limits are exceeded.Web/Resgrid.Web.Services/Controllers/TwilioProviderController.cs (1)
93-109: Consider checking the result ofRecordSmsResponseAsync.The return value from
RecordSmsResponseAsyncis discarded. If the response recording fails (e.g., invalid run code, already responded, or expired run), the user still receives a "received your communication test response" message, which could be misleading.♻️ Proposed improvement
// Check for Communication Test response (CT- prefix) if (!string.IsNullOrWhiteSpace(textMessage.Text) && textMessage.Text.Trim().StartsWith("CT-", StringComparison.OrdinalIgnoreCase)) { var runCode = textMessage.Text.Trim().Split(' ')[0].ToUpperInvariant(); - await _communicationTestService.RecordSmsResponseAsync(runCode, textMessage.Msisdn); + var recorded = await _communicationTestService.RecordSmsResponseAsync(runCode, textMessage.Msisdn); messageEvent.Processed = true; - response.Message("Resgrid received your communication test response. Thank you."); + response.Message(recorded + ? "Resgrid received your communication test response. Thank you." + : "Resgrid could not match your response to an active test. Please check the code and try again."); await _numbersService.SaveInboundMessageEventAsync(messageEvent);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/TwilioProviderController.cs` around lines 93 - 109, The code currently ignores the boolean/result from _communicationTestService.RecordSmsResponseAsync (called in the CT- branch) so callers always get a success reply; change the flow to capture the return value (e.g., var success = await _communicationTestService.RecordSmsResponseAsync(runCode, textMessage.Msisdn)), and branch on it: if success, mark messageEvent.Processed = true, send the existing "received" response and call _numbersService.SaveInboundMessageEventAsync; if not, send an appropriate failure response (e.g., "Could not record your communication test response" or more specific), do not mark as processed (or mark accordingly), and still persist the inbound event as needed. Ensure you update uses of response, messageEvent, and SaveInboundMessageEventAsync to reflect the success/failure path.Repositories/Resgrid.Repositories.DataRepository/CommunicationTestRepository.cs (1)
43-45: Prefer a single UnitOfWork-first connection pathLine [49] detects UoW connection state, but Line [51] opens a separate provider connection while Line [45] still binds transaction to
_unitOfWork.Transaction. This split path is harder to reason about and easy to drift across repositories.♻️ Suggested refactor
- return await x.QueryAsync<CommunicationTest>(sql: query, - param: dynamicParameters, - transaction: _unitOfWork.Transaction); + return await x.QueryAsync<CommunicationTest>(sql: query, + param: dynamicParameters, + transaction: _unitOfWork?.Transaction);- DbConnection conn = null; - if (_unitOfWork?.Connection == null) - { - using (conn = _connectionProvider.Create()) - { - await conn.OpenAsync(); - return await selectFunction(conn); - } - } - else - { - conn = _unitOfWork.CreateOrGetConnection(); - return await selectFunction(conn); - } + if (_unitOfWork != null) + { + var conn = _unitOfWork.CreateOrGetConnection(); + return await selectFunction(conn); + } + + using (var conn = _connectionProvider.Create()) + { + await conn.OpenAsync(); + return await selectFunction(conn); + }Also applies to: 48-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/CommunicationTestRepository.cs` around lines 43 - 45, The repository currently mixes two connection paths: use the UnitOfWork connection consistently instead of opening a separate provider connection; update CommunicationTestRepository so QueryAsync uses _unitOfWork.Connection (and still passes _unitOfWork.Transaction) and ensure that connection is opened if not already (check _unitOfWork.IsOpen or open _unitOfWork.Connection once) rather than calling the provider to create/open another connection—this keeps the unit-of-work-first pattern and prevents binding a transaction to a different connection than the query.Core/Resgrid.Model/CommunicationTestRun.cs (1)
47-49: HardenIdValuesetter against malformed inputLine [48] uses
Guid.Parse(...)and will throw on invalid values. Defensive parsing keeps this utility property safer for edge-case assignments.🛡️ Suggested change
- set { CommunicationTestRunId = value == null ? Guid.Empty : Guid.Parse(value.ToString()); } + set + { + if (value == null || !Guid.TryParse(value.ToString(), out var parsed)) + CommunicationTestRunId = Guid.Empty; + else + CommunicationTestRunId = parsed; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/CommunicationTestRun.cs` around lines 47 - 49, The IdValue setter on CommunicationTestRun currently calls Guid.Parse and will throw for malformed input; change it to defensively parse using Guid.TryParse (e.g., trim the value string, call Guid.TryParse) and set CommunicationTestRunId to the parsed Guid on success or Guid.Empty on failure (handle null the same as now). Update the IdValue setter logic to use TryParse on value.ToString() and assign CommunicationTestRunId accordingly.Repositories/Resgrid.Repositories.DataRepository/CommunicationTestResultRepository.cs (1)
44-46: Unify UnitOfWork connection/transaction handlingLine [50] and Line [90] branch on
_unitOfWork?.Connection, but the null branch uses raw_connectionProvider.Create()while still relying on_unitOfWork.Transactionin query calls (Line [46], Line [84]). Consolidating this into one UoW-aware helper will make behavior predictable and reduce repeated lifecycle code.♻️ Suggested refactor
- return await x.QueryAsync<CommunicationTestResult>(sql: query, - param: dynamicParameters, - transaction: _unitOfWork.Transaction); + return await x.QueryAsync<CommunicationTestResult>(sql: query, + param: dynamicParameters, + transaction: _unitOfWork?.Transaction);- DbConnection conn = null; - if (_unitOfWork?.Connection == null) - { - using (conn = _connectionProvider.Create()) - { - await conn.OpenAsync(); - return await selectFunction(conn); - } - } - else - { - conn = _unitOfWork.CreateOrGetConnection(); - return await selectFunction(conn); - } + if (_unitOfWork != null) + { + var conn = _unitOfWork.CreateOrGetConnection(); + return await selectFunction(conn); + } + + using (var conn = _connectionProvider.Create()) + { + await conn.OpenAsync(); + return await selectFunction(conn); + }Also applies to: 49-62, 82-84, 89-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/CommunicationTestResultRepository.cs` around lines 44 - 46, The repository branches on _unitOfWork?.Connection but still calls queries with _unitOfWork.Transaction or raw connections, causing inconsistent lifecycle handling; create a single UoW-aware helper (e.g., a private GetConnectionAndTransaction or GetDbContextTuple used by CommunicationTestResultRepository) that returns an open IDbConnection and an optional IDbTransaction (from _unitOfWork if present, otherwise null) and ensure all QueryAsync/ExecuteAsync calls (the usages that currently call QueryAsync<CommunicationTestResult> and other DB calls) use that connection and the returned transaction; remove direct calls to _connectionProvider.Create() and stop referencing _unitOfWork.Transaction outside the helper so connection creation/disposing and transaction selection are centralized and consistent.Repositories/Resgrid.Repositories.DataRepository/CommunicationTestRunRepository.cs (1)
44-46: Connection/transaction orchestration is repeated and inconsistentThe same mixed pattern appears in all three methods: conditional on
_unitOfWork?.Connection, raw provider connection in one branch, and_unitOfWork.Transactionpassed directly in Dapper calls. Centralizing this once (UoW-first) will reduce drift and make transaction behavior explicit.♻️ Suggested direction
- transaction: _unitOfWork.Transaction + transaction: _unitOfWork?.Transaction- if (_unitOfWork?.Connection == null) { using (conn = _connectionProvider.Create()) { ... } } - else { conn = _unitOfWork.CreateOrGetConnection(); ... } + if (_unitOfWork != null) { var conn = _unitOfWork.CreateOrGetConnection(); ... } + else { using (var conn = _connectionProvider.Create()) { await conn.OpenAsync(); ... } }Also applies to: 49-62, 82-84, 89-102, 121-123, 126-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/CommunicationTestRunRepository.cs` around lines 44 - 46, Centralize connection/transaction selection so every Dapper call in CommunicationTestRunRepository uses the UnitOfWork when present: create a small helper (or inline a single pattern) that sets var connection = _unitOfWork?.Connection ?? _connectionProvider.GetOpenConnection(); var transaction = _unitOfWork?.Transaction; then call connection.QueryAsync<CommunicationTestRun>(sql: query, param: dynamicParameters, transaction: transaction) (and the same for Execute/QuerySingle calls) instead of branching between _unitOfWork?.Connection and provider connections or passing _unitOfWork.Transaction inconsistently; update all methods that use QueryAsync/Execute (the ones currently referencing _unitOfWork?.Connection, Provider.GetOpenConnection, and _unitOfWork.Transaction) to use this single pattern so transactional behavior is explicit and consistent.Core/Resgrid.Services/CommunicationTestService.cs (1)
381-389: Use thread-safe randomness for RunCode generation.
new Random()is not thread-safe and could produce duplicate codes when called concurrently from multiple threads. SinceRunCodeis used for SMS response matching and displayed to users, consider usingRandomNumberGeneratorfor better entropy and thread safety.🔒 Proposed thread-safe implementation
+using System.Security.Cryptography; + private static string GenerateRunCode() { const string chars = "ABCDEFGHJKLMNPQRSTUVWXYZ23456789"; - var random = new Random(); var code = new char[4]; - for (int i = 0; i < 4; i++) - code[i] = chars[random.Next(chars.Length)]; + using var rng = RandomNumberGenerator.Create(); + var bytes = new byte[4]; + rng.GetBytes(bytes); + for (int i = 0; i < 4; i++) + code[i] = chars[bytes[i] % chars.Length]; return "CT-" + new string(code); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/CommunicationTestService.cs` around lines 381 - 389, GenerateRunCode uses new Random() which is not thread-safe and can produce collisions under concurrency; replace its use with a cryptographically secure, thread-safe RNG (e.g., System.Security.Cryptography.RandomNumberGenerator or RandomNumberGenerator.GetInt32) inside GenerateRunCode to pick indices from the chars string for each of the 4 characters, then return "CT-" + new string(code); ensure no static Random instances remain and all randomness in GenerateRunCode is derived from the secure RNG for proper entropy and thread safety.Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.cs (1)
154-236: Add input validation for the Save endpoint.The
SaveCommunicationTestInputmodel lacks validation attributes (as noted in the context snippet), and the controller doesn't validate input beyond checking if a scheduled test can be created. Critical fields likeName(max 500 chars per entity),Description(max 4000 chars), andResponseWindowMinutes(should have reasonable bounds) should be validated to prevent truncation errors or abuse.🛡️ Proposed validation additions
public async Task<SaveCommunicationTestResult> Save([FromBody] SaveCommunicationTestInput input, CancellationToken cancellationToken) { var result = new SaveCommunicationTestResult(); + + if (string.IsNullOrWhiteSpace(input.Name) || input.Name.Length > 500) + { + result.Status = ResponseHelper.Failure; + ResponseHelper.PopulateV4ResponseData(result); + return result; + } + + if (input.Description?.Length > 4000) + { + result.Status = ResponseHelper.Failure; + ResponseHelper.PopulateV4ResponseData(result); + return result; + } + bool isNew = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.cs` around lines 154 - 236, Add proper input validation for Save by updating the SaveCommunicationTestInput DTO with data annotations (e.g., [Required] on Name, [StringLength(500)] on Name, [StringLength(4000)] on Description, bounds for ResponseWindowMinutes like [Range(1,1440)] or your chosen limits) and any other needed attributes for ScheduleType/Time fields; then in the Save method of CommunicationTestsController validate ModelState (return a failure response populated via ResponseHelper when invalid) before proceeding to CanCreateScheduledTestAsync. Also defensively clamp/normalize values when mapping into the CommunicationTest entity (e.g., set ResponseWindowMinutes = Math.Clamp(input.ResponseWindowMinutes, 1, 1440) and trim Name/Description) to prevent truncation or out-of-range values.
| Create.Table("communicationtests") | ||
| .WithColumn("communicationtestid").AsCustom("citext").NotNullable().PrimaryKey() | ||
| .WithColumn("departmentid").AsInt32().NotNullable() | ||
| .WithColumn("name").AsCustom("citext").NotNullable() | ||
| .WithColumn("description").AsCustom("text").Nullable() | ||
| .WithColumn("scheduletype").AsInt32().NotNullable().WithDefaultValue(0) | ||
| .WithColumn("sunday").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("monday").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("tuesday").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("wednesday").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("thursday").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("friday").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("saturday").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("dayofmonth").AsInt32().Nullable() | ||
| .WithColumn("time").AsCustom("citext").Nullable() | ||
| .WithColumn("testsms").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("testemail").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("testvoice").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("testpush").AsBoolean().NotNullable().WithDefaultValue(false) | ||
| .WithColumn("active").AsBoolean().NotNullable().WithDefaultValue(true) | ||
| .WithColumn("createdbyuserid").AsCustom("citext").NotNullable() | ||
| .WithColumn("createdon").AsDateTime().NotNullable() | ||
| .WithColumn("updatedon").AsDateTime().Nullable() | ||
| .WithColumn("responsewindowminutes").AsInt32().NotNullable().WithDefaultValue(60); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
FluentMigrator PostgreSQL uuid type with default gen_random_uuid
💡 Result:
In FluentMigrator, use AsGuid() (maps to PostgreSQL uuid) and set the default with raw SQL:
using FluentMigrator;
[Migration(2026040901)]
public class AddUsers : Migration
{
public override void Up()
{
// Needed on PostgreSQL <= 12.x (gen_random_uuid() is in pgcrypto there)
Execute.Sql("CREATE EXTENSION IF NOT EXISTS pgcrypto;"); // optional on PG 13+
Create.Table("users")
.WithColumn("id").AsGuid().PrimaryKey()
.WithDefaultValue(RawSql.Insert("gen_random_uuid()"))
.WithColumn("name").AsString(200).NotNullable();
}
public override void Down() => Delete.Table("users");
}Notes:
RawSql.Insert(...)is the FluentMigrator mechanism for passing a database function/expression as a default value. [1]gen_random_uuid()is built-in starting in PostgreSQL 13; on PostgreSQL 12.13 and earlier it comes from thepgcryptoextension. [2] [3]
Sources: [1] [2] [3]
🏁 Script executed:
# Find the migration file in question
find . -name "*M0062*AddingCommunicationTests*" -type fRepository: Resgrid/Core
Length of output: 231
🏁 Script executed:
# List files in the migrations directory to understand structure
ls -la Providers/Resgrid.Providers.MigrationsPg/Migrations/ | head -20Repository: Resgrid/Core
Length of output: 1511
🏁 Script executed:
# Check the full contents of the PostgreSQL migration file
cat -n Providers/Resgrid.Providers.MigrationsPg/Migrations/M0062_AddingCommunicationTestsPg.csRepository: Resgrid/Core
Length of output: 6175
🏁 Script executed:
# Look for similar UUID/GUID patterns in PostgreSQL migrations
rg "AsCustom\(\"citext\"\)" Providers/Resgrid.Providers.MigrationsPg/Migrations/ -B 2 -A 2 | head -50Repository: Resgrid/Core
Length of output: 6303
🏁 Script executed:
# Check if there's a corresponding SQL Server migration
find . -name "*M0062*AddingCommunicationTests*" -not -name "*Pg*" -type fRepository: Resgrid/Core
Length of output: 141
🏁 Script executed:
# Look for AsGuid patterns in PostgreSQL migrations to see expected pattern
rg "AsGuid\(\)" Providers/Resgrid.Providers.MigrationsPg/Migrations/ -B 1 -A 1 | head -30Repository: Resgrid/Core
Length of output: 38
🏁 Script executed:
cat -n Providers/Resgrid.Providers.Migrations/Migrations/M0062_AddingCommunicationTests.csRepository: Resgrid/Core
Length of output: 6302
🏁 Script executed:
# Also check other SQL Server migrations to see if there's a pattern with primary keys
rg "PrimaryKey()" Providers/Resgrid.Providers.Migrations/Migrations/ -B 2 | head -40Repository: Resgrid/Core
Length of output: 4080
Incorrect data type for UUID primary keys — use AsGuid() with PostgreSQL-specific default instead of citext.
The primary keys across all three tables use AsCustom("citext") (case-insensitive text), but the SQL Server migration uses AsGuid() with SystemMethods.NewGuid. This creates a data type mismatch and also affects all GUID reference columns (communicationtestid, communicationtestrunid, initiatedbyuserid, userid, createdbyuserid), which are stored as text rather than UUID.
For PostgreSQL, use AsGuid() with RawSql.Insert("gen_random_uuid()") as the default:
Corrected fix for communicationtests table
Create.Table("communicationtests")
- .WithColumn("communicationtestid").AsCustom("citext").NotNullable().PrimaryKey()
+ .WithColumn("communicationtestid").AsGuid().NotNullable().PrimaryKey().WithDefaultValue(RawSql.Insert("gen_random_uuid()"))
.WithColumn("departmentid").AsInt32().NotNullable()
- .WithColumn("name").AsCustom("citext").NotNullable()
+ .WithColumn("name").AsString(500).NotNullable()This affects all three tables and their foreign key references. Ensure the same pattern is applied to communicationtestrunid, communicationtestresultid, and all user ID columns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0062_AddingCommunicationTestsPg.cs`
around lines 10 - 33, The migration uses citext for GUID columns; update primary
keys and all GUID reference columns to use AsGuid() with PostgreSQL default
gen_random_uuid(): replace communicationtests.communicationtestid,
communicationtestruns.communicationtestrunid,
communicationtestresults.communicationtestresultid (and any other id PKs) to
AsGuid().PrimaryKey().WithDefault(RawSql.Insert("gen_random_uuid()")), and
change all user/id reference columns (createdbyuserid, initiatedbyuserid,
userid, and other foreign key columns referencing those IDs) from
AsCustom("citext") to AsGuid() so types match and foreign keys align across
communicationtests, communicationtestruns, and communicationtestresults.
| if (newUserIds.Any() || newGroupIds.Any() || newUnitIds.Any() || newRoleIds.Any()) | ||
| { | ||
| var cqi = new CallQueueItem(); | ||
| cqi.Call = call; | ||
|
|
||
| if (newGroupIds.Any() || newUnitIds.Any() || newRoleIds.Any()) | ||
| cqi.Profiles = (await _userProfileService.GetAllProfilesForDepartmentAsync(DepartmentId)).Select(x => x.Value).ToList(); | ||
| else | ||
| cqi.Profiles = await _userProfileService.GetSelectedUserProfilesAsync(newUserIds); | ||
|
|
||
| await _queueService.EnqueueCallBroadcastAsync(cqi, cancellationToken); |
There was a problem hiding this comment.
Resolve only the newly added recipients here.
CallQueueItem.Profiles is the actual delivery target in Core/Resgrid.Model/Queue/CallQueueItem.cs:7-26 / Core/Resgrid.Services/QueueService.cs:143-206. Loading all department profiles on Lines 984-985 means adding a single group, unit, or role rebroadcasts the call to everyone in the department, not just the newly added entities.
🤖 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
979 - 989, The current code loads all department profiles when any
newGroupIds/newUnitIds/newRoleIds exist, causing rebroadcast to everyone;
instead build cqi.Profiles as only the profiles for the newly added recipients
by: gather profiles for newUserIds via
_userProfileService.GetSelectedUserProfilesAsync(newUserIds) and separately
fetch profiles for the specific newGroupIds, newUnitIds and newRoleIds (use or
add service methods that return profiles for given group/unit/role ids), union
those result sets (deduplicate) and assign that combined list to
CallQueueItem.Profiles before calling _queueService.EnqueueCallBroadcastAsync;
keep usage of newUserIds/newGroupIds/newUnitIds/newRoleIds,
CallQueueItem.Profiles, _userProfileService and _queueService to locate the
change.
| { | ||
| public class NewCommunicationTestView : BaseUserModel | ||
| { | ||
| public CommunicationTest Test { get; set; } = new CommunicationTest(); |
There was a problem hiding this comment.
Avoid binding the domain entity directly in the form model.
Using Resgrid.Model.CommunicationTest as POST-bound state opens over-posting risk (client-controlled fields outside intended form scope). Use a dedicated input DTO with only allowed fields, then map server-side.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Web/Resgrid.Web/Areas/User/Models/CommunicationTests/NewCommunicationTestView.cs`
at line 7, The view model currently exposes the domain entity CommunicationTest
via the property Test which risks over-posting; replace that property with a
dedicated input DTO (e.g., NewCommunicationTestInput or CommunicationTestInput)
containing only the fields you expect from the form, update
NewCommunicationTestView to use this DTO instead of
Resgrid.Model.CommunicationTest, and in the controller action that handles the
POST map/validate the DTO to the domain CommunicationTest server-side (construct
a new CommunicationTest and copy only allowed properties) before saving.
| catch (Exception ex) | ||
| { | ||
| Resgrid.Framework.Logging.LogException(ex); | ||
| _logger.LogError(ex.ToString()); | ||
| } |
There was a problem hiding this comment.
Swallowing exceptions hides task failures from the scheduler.
Catching and logging exceptions without rethrowing prevents the Quidjibo scheduler from knowing the task failed. This could lead to silent failures where scheduled tests never run and the issue goes undetected. Consider rethrowing or using a pattern that marks the task as failed.
🛡️ Proposed fix to propagate failure
catch (Exception ex)
{
Resgrid.Framework.Logging.LogException(ex);
_logger.LogError(ex.ToString());
+ throw;
}📝 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.
| catch (Exception ex) | |
| { | |
| Resgrid.Framework.Logging.LogException(ex); | |
| _logger.LogError(ex.ToString()); | |
| } | |
| catch (Exception ex) | |
| { | |
| Resgrid.Framework.Logging.LogException(ex); | |
| _logger.LogError(ex.ToString()); | |
| throw; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Workers/Resgrid.Workers.Console/Tasks/CommunicationTestTask.cs` around lines
41 - 45, The catch in CommunicationTestTask (inside the task execution method)
currently logs exceptions with Resgrid.Framework.Logging.LogException and
_logger.LogError but swallows them; update the catch to propagate failure to
Quidjibo by rethrowing the exception (or rethrowing a wrapped exception) after
logging so the scheduler sees the task as failed; locate the catch block in
CommunicationTestTask and add a throw (or throw new Exception with preserved
inner exception) immediately after the logging calls.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs (1)
984-994:⚠️ Potential issue | 🟠 MajorLimit the auto-broadcast to the newly added recipients.
This still loads every department profile when a single new group, unit, or role is added, so the “new recipients only” path becomes a department-wide rebroadcast again.
🤖 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 984 - 994, The code currently loads all department profiles whenever newGroupIds/newUnitIds/newRoleIds are non-empty, defeating the "new recipients only" intent; change the branch that sets cqi.Profiles so it only fetches profiles for the newly added recipients instead of calling _userProfileService.GetAllProfilesForDepartmentAsync. Use targeted lookups (e.g., call or add service methods like GetProfilesForGroupsAsync(GetProfilesForUnitsAsync/GetProfilesForRolesAsync) or a combined GetProfilesForGroupUnitRoleIdsAsync) to resolve profiles for newGroupIds, newUnitIds and newRoleIds, combine those with profiles from GetSelectedUserProfilesAsync(newUserIds), assign that combined list to cqi.Profiles, then call _queueService.EnqueueCallBroadcastAsync(cqi, cancellationToken).
🧹 Nitpick comments (2)
Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.cs (1)
373-406: Consider verifying test ownership before fetching runs.Unlike
Get(line 110) andGetReport(line 427), this method fetches runs without first verifying that the test exists and belongs to the current department. While the filtering at line 385-386 prevents data leakage, verifying ownership first would be consistent with other endpoints and avoid unnecessary service calls for invalid test IDs.♻️ Suggested improvement for consistency
public async Task<GetTestRunsResult> GetRuns(string testId) { var result = new GetTestRunsResult(); if (!Guid.TryParse(testId, out var id)) { result.Status = ResponseHelper.NotFound; ResponseHelper.PopulateV4ResponseData(result); return result; } + var test = await _communicationTestService.GetTestByIdAsync(id); + if (test == null || test.DepartmentId != DepartmentId) + { + result.Status = ResponseHelper.NotFound; + ResponseHelper.PopulateV4ResponseData(result); + return result; + } + var runs = await _communicationTestService.GetRunsByTestIdAsync(id); if (runs != null) { foreach (var run in runs) { - if (run.DepartmentId != DepartmentId) - continue; - result.Data.Add(new TestRunData {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.cs` around lines 373 - 406, Before calling _communicationTestService.GetRunsByTestIdAsync(id), first fetch the test and verify ownership (e.g. call _communicationTestService.GetByIdAsync(id) or the service method that returns the CommunicationTest) and ensure its DepartmentId == DepartmentId; if the test is null or belongs to another department, set result.Status = ResponseHelper.NotFound, call ResponseHelper.PopulateV4ResponseData(result) and return. Only then call _communicationTestService.GetRunsByTestIdAsync(id) and build result.Data as currently done.Core/Resgrid.Services/CommunicationService.cs (1)
333-398: Extract the cancellation push payload into one helper.The address resolution, title/subtitle cleanup, truncation, and color assignment now exist in multiple methods, and the new unit-cancel path has already drifted from the user-cancel path. A shared helper returning a
StandardPushCallwould keep the rules aligned and make this much easier to test.As per coding guidelines, "Prefer pure methods over methods with side effects" and "Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions".
Also applies to: 432-491
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/CommunicationService.cs` around lines 333 - 398, Extract the repeated push payload construction into a pure helper that returns a StandardPushCall (e.g., BuildCancellationPush(Call call, string address, int departmentId, string departmentCode, string profile?, IGeoLocationProvider geoProvider) ) and replace the duplicated blocks at both the user-cancel and unit-cancel sites with calls to that helper; the helper should set CallId, Title, Priority, ActiveCallCount, DepartmentId/Code, determine Color (fall back to "#000000"), resolve subtitle from address/Call.Address/geo via _geoLocationProvider.GetAproxAddressFromLatLong (inject provider or pass it in), truncate to 200, strip HTML via StringHelpers.StripHtmlTagsCharArray, remove URLs with the same Regex, replace "/" with space, and return the StandardPushCall so callers simply call await _pushService.PushCall(result, dispatch.UserId, profile, call.CallPriority); add unit tests for BuildCancellationPush to assert color, subtitle resolution, truncation and cleaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Services/CommunicationService.cs`:
- Around line 406-423: The SMS and Email sending calls in CommunicationService
(the blocks that call _subscriptionsService.GetCurrentPaymentForDepartmentAsync,
_smsService.SendCancelCallAsync and _emailService.SendCancelCallAsync) are not
exception-safe and can bubble up to make a saved cancellation return 500; wrap
each external call in its own try/catch that logs the exception (using the
existing logger) and swallows it so failures don’t propagate, ensure you still
respect the profile null/permission checks, and preserve ordering (payment fetch
only for SMS) so later notification attempts continue even if one fails.
In `@Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.cs`:
- Around line 274-286: The audit event is being published before the deletion
actually happens; move the _eventAggregator.SendMessage<AuditEvent>(...) call so
it runs after await _communicationTestService.DeleteTestAsync(testId,
cancellationToken) completes successfully, set AuditEvent.Successful = true
there, and (optionally) add a catch around DeleteTestAsync to send an AuditEvent
with Successful = false and error info if the deletion throws; refer to
AuditEvent, _eventAggregator.SendMessage<AuditEvent>, and
_communicationTestService.DeleteTestAsync to locate and update the logic.
In `@Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs`:
- Around line 645-647: The snapshots assume call.GroupDispatches,
call.UnitDispatches, and call.RoleDispatches are non-null which can throw when
any are null; change the snapshot and subsequent HashSet constructions to use
null-coalescing to an empty collection (e.g., new
List<CallDispatchGroup>(call.GroupDispatches ??
Enumerable.Empty<CallDispatchGroup>()) and similarly for CallDispatchUnit and
CallDispatchRole) and apply the same null-safe normalization where the other
snapshots/hashsets are created around lines 896-899 so the diffing/update logic
never operates on a null reference.
- Around line 940-964: The current cancel loops fetch live group/role members
(via _departmentGroupsService.GetAllMembersForGroupAsync and
_personnelRolesService.GetAllMembersOfRoleAsync) after the call save, which can
notify users who were never originally dispatched; instead, capture the user IDs
from existingGroupDispatches and existingRoleDispatches before mutating the call
and iterate those snapshots when computing cancellations (compare that snapshot
set against stillDispatchedUserIds and notifiedUserIds and call
_communicationService.SendCancelCallAsync for user IDs present in the original
dispatch snapshot but not in the post-save audience), ensuring you reference
existingGroupDispatches/existingRoleDispatches,
cancelledGroupIds/cancelledRoleIds, stillDispatchedUserIds, notifiedUserIds and
SendCancelCallAsync in the fix.
---
Duplicate comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs`:
- Around line 984-994: The code currently loads all department profiles whenever
newGroupIds/newUnitIds/newRoleIds are non-empty, defeating the "new recipients
only" intent; change the branch that sets cqi.Profiles so it only fetches
profiles for the newly added recipients instead of calling
_userProfileService.GetAllProfilesForDepartmentAsync. Use targeted lookups
(e.g., call or add service methods like
GetProfilesForGroupsAsync(GetProfilesForUnitsAsync/GetProfilesForRolesAsync) or
a combined GetProfilesForGroupUnitRoleIdsAsync) to resolve profiles for
newGroupIds, newUnitIds and newRoleIds, combine those with profiles from
GetSelectedUserProfilesAsync(newUserIds), assign that combined list to
cqi.Profiles, then call _queueService.EnqueueCallBroadcastAsync(cqi,
cancellationToken).
---
Nitpick comments:
In `@Core/Resgrid.Services/CommunicationService.cs`:
- Around line 333-398: Extract the repeated push payload construction into a
pure helper that returns a StandardPushCall (e.g., BuildCancellationPush(Call
call, string address, int departmentId, string departmentCode, string profile?,
IGeoLocationProvider geoProvider) ) and replace the duplicated blocks at both
the user-cancel and unit-cancel sites with calls to that helper; the helper
should set CallId, Title, Priority, ActiveCallCount, DepartmentId/Code,
determine Color (fall back to "#000000"), resolve subtitle from
address/Call.Address/geo via _geoLocationProvider.GetAproxAddressFromLatLong
(inject provider or pass it in), truncate to 200, strip HTML via
StringHelpers.StripHtmlTagsCharArray, remove URLs with the same Regex, replace
"/" with space, and return the StandardPushCall so callers simply call await
_pushService.PushCall(result, dispatch.UserId, profile, call.CallPriority); add
unit tests for BuildCancellationPush to assert color, subtitle resolution,
truncation and cleaning.
In `@Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.cs`:
- Around line 373-406: Before calling
_communicationTestService.GetRunsByTestIdAsync(id), first fetch the test and
verify ownership (e.g. call _communicationTestService.GetByIdAsync(id) or the
service method that returns the CommunicationTest) and ensure its DepartmentId
== DepartmentId; if the test is null or belongs to another department, set
result.Status = ResponseHelper.NotFound, call
ResponseHelper.PopulateV4ResponseData(result) and return. Only then call
_communicationTestService.GetRunsByTestIdAsync(id) and build result.Data as
currently done.
🪄 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: 2a67ff65-b72a-44d5-9025-2ecc87f85fab
📒 Files selected for processing (13)
Core/Resgrid.Config/PaymentProviderConfig.csCore/Resgrid.Services/CommunicationService.csCore/Resgrid.Services/SmsService.csProviders/Resgrid.Providers.Claims/ClaimsLogic.csProviders/Resgrid.Providers.Claims/ClaimsPrincipalFactory.csProviders/Resgrid.Providers.Claims/JwtTokenProvider.csProviders/Resgrid.Providers.Claims/ResgridClaimTypes.csProviders/Resgrid.Providers.Claims/ResgridIdentity.csWeb/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.csWeb/Resgrid.Web.Services/Startup.csWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Views/CommunicationTest/Edit.cshtmlWeb/Resgrid.Web/Views/Account/LogOn.cshtml
✅ Files skipped from review due to trivial changes (2)
- Providers/Resgrid.Providers.Claims/ResgridClaimTypes.cs
- Providers/Resgrid.Providers.Claims/JwtTokenProvider.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Core/Resgrid.Config/PaymentProviderConfig.cs
- Core/Resgrid.Services/SmsService.cs
Web/Resgrid.Web.Services/Controllers/v4/CommunicationTestsController.cs
Outdated
Show resolved
Hide resolved
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Summary by CodeRabbit
New Features
Improvements
Tests