Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
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 check‑in timers and calendar attendance: new domain models, repositories, services, DI registrations, SQL migrations (SQL Server/Postgres), v4 API endpoints and MVC UI/JS, SignalR auth tightening, audit events, permission changes for log deletion, and unit tests covering timers and calendar check‑ins. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Dispatch User
participant Browser as Browser JS
participant Controller as DispatchController
participant Service as CheckInTimerService
participant Repo as CheckInRecordRepository
participant DB as Database
User->>Browser: Click "Perform Check‑In"
Browser->>Browser: Attempt geolocation (timeout)
Browser->>Controller: POST /PerformCheckIn(payload)
Controller->>Service: PerformCheckInAsync(record)
Service->>Repo: SaveAsync(record)
Repo->>DB: INSERT checkin record
DB-->>Repo: inserted record
Repo-->>Service: saved record
Service-->>Controller: saved record
Controller->>Browser: 200 OK (id)
Controller->>Browser: (SignalR) broadcast CheckInPerformed
Browser->>Controller: GET status/history (refresh)
sequenceDiagram
participant User as Dispatcher
participant API as v4 CheckInTimersController
participant Service as CheckInTimerService
participant ConfigRepo as CheckInTimerConfigRepository
participant OverrideRepo as CheckInTimerOverrideRepository
participant Calls as CallsService
participant DB as Database
User->>API: GET /GetTimersForCall(callId)
API->>Calls: Get call, validate department
API->>Service: ResolveAllTimersForCallAsync(call)
Service->>ConfigRepo: GetByDepartmentIdAsync(deptId)
ConfigRepo->>DB: SELECT configs
DB-->>ConfigRepo: configs
Service->>OverrideRepo: GetMatchingOverridesAsync(deptId, callType, priority)
OverrideRepo->>DB: SELECT overrides
DB-->>OverrideRepo: overrides
Service->>Service: Apply override priority and seed timers
Service-->>API: List<ResolvedCheckInTimer>
API-->>User: Return resolved timers
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.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [Authorize(Policy = ResgridResources.Department_Update)] | ||
| public async Task<ActionResult<SaveCheckInTimerConfigResult>> SaveTimerConfig([FromBody] CheckInTimerConfigInput 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 2 days ago
In general, to fix missing CSRF validation in ASP.NET Core MVC/Web API when using cookie-based auth, you apply anti-forgery validation to all unsafe HTTP methods (POST/PUT/PATCH/DELETE). This is commonly done via [ValidateAntiForgeryToken] on individual actions, or [AutoValidateAntiforgeryToken] at the controller level, and ensuring clients send the anti-forgery token with the request.
For this specific issue, the minimal, targeted fix without altering existing behavior is to add [ValidateAntiForgeryToken] to the SaveTimerConfig action method. This leverages ASP.NET Core’s built-in anti-forgery validation and aligns with CodeQL’s recommendation. Because the file already imports Microsoft.AspNetCore.Mvc, no new imports are needed; [ValidateAntiForgeryToken] is defined in that namespace. We do not change routing, parameters, or the internal logic of SaveTimerConfig; we only add the attribute above the method (ideally between the existing attributes for clarity). The change is confined to Web/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.cs, lines around the SaveTimerConfig declaration.
Suggested changeset
1
Web/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.cs
| @@ -78,6 +78,7 @@ | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [Authorize(Policy = ResgridResources.Department_Update)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<SaveCheckInTimerConfigResult>> SaveTimerConfig([FromBody] CheckInTimerConfigInput input, CancellationToken cancellationToken) | ||
| { | ||
| var result = new SaveCheckInTimerConfigResult(); |
Copilot is powered by AI and may make mistakes. Always verify output.
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [Authorize(Policy = ResgridResources.Department_Update)] | ||
| public async Task<ActionResult<SaveCheckInTimerOverrideResult>> SaveTimerOverride([FromBody] CheckInTimerOverrideInput 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 2 days ago
In general, to fix missing CSRF protection on ASP.NET Core MVC actions that handle POSTs with cookie-based authentication, you decorate the action (or controller) with [ValidateAntiForgeryToken] (or [AutoValidateAntiforgeryToken] on the controller) and ensure clients send a valid anti-forgery token with each state-changing request. This ensures that any POST originating from a browser must include a token that an attacker cannot forge, mitigating CSRF.
For this specific method, the minimal and safest fix that preserves existing behavior is to add the [ValidateAntiForgeryToken] attribute to SaveTimerOverride. This will enforce validation of the anti-forgery token for that POST endpoint only, without altering the semantics of other actions. Since the file already imports Microsoft.AspNetCore.Mvc, no new imports are required; ValidateAntiForgeryTokenAttribute is in that namespace. Concretely, in Web/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.cs, directly above the public async Task<ActionResult<SaveCheckInTimerOverrideResult>> SaveTimerOverride(...) signature (currently preceded by [HttpPost("SaveTimerOverride")], [Consumes(...)], [ProducesResponseType(...)], and [Authorize(...)]), add [ValidateAntiForgeryToken]. No other code changes are needed in this snippet.
Suggested changeset
1
Web/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.cs
| @@ -175,6 +175,7 @@ | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [Authorize(Policy = ResgridResources.Department_Update)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<SaveCheckInTimerOverrideResult>> SaveTimerOverride([FromBody] CheckInTimerOverrideInput input, CancellationToken cancellationToken) | ||
| { | ||
| var result = new SaveCheckInTimerOverrideResult(); |
Copilot is powered by AI and may make mistakes. Always verify output.
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Call_Update)] | ||
| public async Task<ActionResult<PerformCheckInResult>> PerformCheckIn([FromBody] PerformCheckInInput 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 2 days ago
In general, to fix missing CSRF protection in ASP.NET Core MVC for a POST action, you enable anti-forgery token validation on that action (or globally) and ensure that legitimate clients send the token. For an individual action, this is done by adding the [ValidateAntiForgeryToken] attribute so the framework validates the token on each request, rejecting forged requests.
In this file, the minimal and most direct fix is to add the [ValidateAntiForgeryToken] attribute to the PerformCheckIn method while leaving existing behavior otherwise unchanged. ASP.NET Core’s antiforgery attributes are defined in Microsoft.AspNetCore.Mvc, which is already imported on line 3, so no new imports are needed. Concretely, in Web/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.cs, just above the PerformCheckIn method declaration (line 322), add [ValidateAntiForgeryToken] below the existing [Authorize(Policy = ResgridResources.Call_Update)] or alongside the other attributes. No further code changes inside the method body are needed for validation; token creation and client-side changes are outside the scope of this snippet.
Suggested changeset
1
Web/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.cs
| @@ -319,6 +319,7 @@ | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status404NotFound)] | ||
| [Authorize(Policy = ResgridResources.Call_Update)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<PerformCheckInResult>> PerformCheckIn([FromBody] PerformCheckInInput input, CancellationToken cancellationToken) | ||
| { | ||
| var result = new PerformCheckInResult(); |
Copilot is powered by AI and may make mistakes. Always verify output.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (11)
Core/Resgrid.Model/ResolvedCheckInTimer.cs (1)
3-18: Consider making this state model immutable (record/init-only).This is a pure data carrier, so immutability would reduce accidental mutation and simplify reasoning.
♻️ Suggested refactor
-public class ResolvedCheckInTimer -{ - public int TargetType { get; set; } - public int? UnitTypeId { get; set; } - public string TargetEntityId { get; set; } - public string TargetName { get; set; } - public int DurationMinutes { get; set; } - public int WarningThresholdMinutes { get; set; } - public bool IsFromOverride { get; set; } -} +public record ResolvedCheckInTimer +{ + public int TargetType { get; init; } + public int? UnitTypeId { get; init; } + public string TargetEntityId { get; init; } + public string TargetName { get; init; } + public int DurationMinutes { get; init; } + public int WarningThresholdMinutes { get; init; } + public bool IsFromOverride { get; init; } +}As per coding guidelines, "Prefer functional patterns and immutable data where appropriate in C#" and "Separate state from behavior (e.g., use records for state and place operations in separate static classes)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/ResolvedCheckInTimer.cs` around lines 3 - 18, Convert the mutable POCO ResolvedCheckInTimer into an immutable state record by replacing the class with a record type (keep the name ResolvedCheckInTimer) and turn all properties (TargetType, UnitTypeId, TargetEntityId, TargetName, DurationMinutes, WarningThresholdMinutes, IsFromOverride) into init-only properties (or positional record parameters) so instances are immutable after construction; update any call sites that assign to these properties to construct a new record instance instead.Core/Resgrid.Model/CheckInTimerTargetType.cs (1)
7-8: Consider expanding abbreviated enum members for long-term clarity.
ICandPARare terse in a public contract; consider fully descriptive names (or at least XML docs) before wider usage spreads.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.Model/CheckInTimerTargetType.cs` around lines 7 - 8, The enum CheckInTimerTargetType contains terse members IC and PAR which are unclear; rename these to descriptive identifiers (for example IC -> IndividualCheckIn or IncidentCheckIn and PAR -> Paramedic or Participant) throughout the codebase (search/replace occurrences of CheckInTimerTargetType.IC and CheckInTimerTargetType.PAR) and update any XML docs or comments for the enum and its members to describe their purpose; ensure serialization names or mappings (if used) are preserved or updated (e.g., add EnumMember attributes or maintain backward-compatible numeric values 2 and 3) so external consumers are not broken.Core/Resgrid.Model/CheckInTimerStatus.cs (1)
5-24: Consider using arecordfor this state-holding type.Per coding guidelines, records are preferred for state representation. Additionally, the
Statusproperty could benefit from an enum (e.g.,CheckInTimerState) instead of magic strings like"Green","Warning","Critical"to improve type safety and prevent typos.♻️ Suggested refactor using record and enum
public enum CheckInTimerState { Green, Warning, Critical } public record CheckInTimerStatus( int TargetType, string TargetEntityId, string TargetName, int? UnitId, DateTime? LastCheckIn, int DurationMinutes, int WarningThresholdMinutes, double ElapsedMinutes, CheckInTimerState Status );As per coding guidelines: "Separate state from behavior (e.g., use records for state and place operations in separate static classes)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/CheckInTimerStatus.cs` around lines 5 - 24, Replace the mutable CheckInTimerStatus class with an immutable record to represent state and add a CheckInTimerState enum for Status to avoid magic strings; specifically change the type named CheckInTimerStatus into a record (positional or read-only properties) and introduce an enum CheckInTimerState with values like Green, Warning, Critical, then update the Status property/type to use CheckInTimerState across usages to preserve type safety and prevent string typos.Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs (3)
1649-1711: Missing[Authorize]attribute onGetCheckInTimerStatusesendpoint.The
GetCheckInTimerStatusesendpoint checkscall.DepartmentId != DepartmentIdbut lacks an explicit[Authorize(Policy = ...)]attribute like other endpoints in this controller. WhileSecureBaseControllerlikely provides base authentication, consider adding the policy attribute for consistency with other call-related endpoints (e.g.,GetCallNotesuses[Authorize(Policy = ResgridResources.Call_View)]).♻️ Add authorization attribute
[HttpGet] +[Authorize(Policy = ResgridResources.Call_View)] public async Task<IActionResult> GetCheckInTimerStatuses(int callId)🤖 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 1649 - 1711, The GetCheckInTimerStatuses action is missing an explicit authorization attribute; add the same policy used by other call endpoints by decorating the GetCheckInTimerStatuses method with [Authorize(Policy = ResgridResources.Call_View)] so it enforces the Call_View permission (keep the existing DepartmentId check and method logic intact).
1751-1778: Missing[Authorize]attribute onPerformCheckInendpoint.The
PerformCheckInendpoint modifies data (creates a check-in record) but lacks an explicit authorization policy. Consider adding[Authorize(Policy = ResgridResources.Call_View)]or a more specific policy.♻️ Add authorization attribute
[HttpPost] +[Authorize(Policy = ResgridResources.Call_View)] public async Task<IActionResult> PerformCheckIn([FromBody] PerformCheckInInput input, CancellationToken cancellationToken)🤖 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 1751 - 1778, The PerformCheckIn action lacks an authorization attribute; add an authorization policy attribute (e.g. [Authorize(Policy = ResgridResources.Call_View)] or a more restrictive policy you have for modifying calls) to the PerformCheckIn method so only authorized users can create check-in records; locate the PerformCheckIn method in DispatchController and apply the attribute directly above the method signature to enforce the policy.
1713-1749: Missing[Authorize]attribute onGetCheckInHistoryendpoint.Same as
GetCheckInTimerStatuses- consider adding[Authorize(Policy = ResgridResources.Call_View)]for consistency.♻️ Add authorization attribute
[HttpGet] +[Authorize(Policy = ResgridResources.Call_View)] public async Task<IActionResult> GetCheckInHistory(int callId)🤖 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 1713 - 1749, Add the missing authorization attribute to the GetCheckInHistory action: apply [Authorize(Policy = ResgridResources.Call_View)] to the GetCheckInHistory method (same as used on GetCheckInTimerStatuses) so access is restricted consistently; update the method signature above public async Task<IActionResult> GetCheckInHistory(int callId) to include the attribute and run tests to verify authorization behavior.Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js (2)
7-14: Consider clearing the timer interval when the call is no longer active.The
timerIntervalis set on page load but never cleared. If the call state changes (e.g., closed via another action), the interval continues running unnecessarily. Consider adding cleanup logic when the component is destroyed or call state changes.♻️ Suggested cleanup approach
// Add a function to stop the timer function stopTimerInterval() { if (timerInterval) { clearInterval(timerInterval); timerInterval = null; } } // Call stopTimerInterval when navigating away or when call state changes $(window).on('beforeunload', stopTimerInterval);🤖 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/dispatch/resgrid.dispatch.checkin-timers.js` around lines 7 - 14, The interval set by timerInterval in the document ready block (where loadTimerStatuses() and setInterval(updateCountdowns, 1000) are called) is never cleared; add cleanup logic to stop the interval when the call is no longer active or the page unloads by implementing a stopTimerInterval function that checks timerInterval, calls clearInterval(timerInterval), and nulls it, and invoke stopTimerInterval when callState changes or on window beforeunload/unload to ensure updateCountdowns is not left running unnecessarily.
16-27: Consider adding error handling for AJAX requests.The
loadTimerStatusesandloadCheckInHistoryfunctions don't handle AJAX failures. Users won't be informed if the data fails to load.♻️ Add .fail() handlers
function loadTimerStatuses() { $.ajax({ url: resgrid.absoluteBaseUrl + '/User/Dispatch/GetCheckInTimerStatuses?callId=' + callId, contentType: 'application/json; charset=utf-8', type: 'GET' }).done(function (result) { if (result) { timerStatuses = result; renderTimerStatuses(); } + }).fail(function () { + console.warn('Failed to load timer statuses'); }); }🤖 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/dispatch/resgrid.dispatch.checkin-timers.js` around lines 16 - 27, Add error handling for the AJAX calls in the loadTimerStatuses and loadCheckInHistory functions: attach .fail(...) handlers to the $.ajax(...) chains used in loadTimerStatuses and loadCheckInHistory, log the error (e.g., console.error or existing logger) and provide user feedback (e.g., show a toast/alert or display an error message in the UI where renderTimerStatuses or history rendering would normally go), and optionally retry or disable UI elements until a successful load; ensure you reference the existing timerStatuses and renderTimerStatuses usage so failures keep the UI consistent.Repositories/Resgrid.Repositories.DataRepository/CheckInTimerOverrideRepository.cs (1)
48-61: Refactor for clarity: make transaction handling explicit and eliminate code duplication.The pattern of passing
_unitOfWork.Transaction(null when using standalone connections) is intentional and consistent across the codebase. However, this implicit behavior can be made more explicit. Additionally, theselectFunctionpattern is duplicated in both methods—consider extracting a shared helper method to reduce duplication and improve maintainability.Example improvement:
private async Task<IEnumerable<CheckInTimerOverride>> ExecuteSelectAsync( Func<DbConnection, DbTransaction, Task<IEnumerable<CheckInTimerOverride>>> selectFunction) { if (_unitOfWork?.Connection == null) { using (var conn = _connectionProvider.Create()) { await conn.OpenAsync(); return await selectFunction(conn, null); } } else { var conn = _unitOfWork.CreateOrGetConnection(); return await selectFunction(conn, _unitOfWork.Transaction); } }Then update methods to accept transaction parameter explicitly in the selectFunction delegate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Repositories/Resgrid.Repositories.DataRepository/CheckInTimerOverrideRepository.cs` around lines 48 - 61, Refactor the duplicated connection/transaction handling in CheckInTimerOverrideRepository by extracting a shared helper (e.g., ExecuteSelectAsync) that takes a delegate Func<DbConnection, DbTransaction, Task<IEnumerable<CheckInTimerOverride>>>; inside the helper, if _unitOfWork?.Connection is null create and open a using connection from _connectionProvider.Create() and pass null for the transaction, otherwise get the connection via _unitOfWork.CreateOrGetConnection() and pass _unitOfWork.Transaction; update existing methods to call ExecuteSelectAsync and change their selectFunction delegates to accept the explicit DbTransaction parameter to eliminate duplication and make transaction handling explicit.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (1)
311-365: Add contract-level XML docs for the new check-in timer endpoints.These actions are discoverable, but the generated XML only has summaries here, and this file has no
Resgrid.Web.Services.Models.v4.CheckInTimers.*members elsewhere. That leaves the new v4 surface without field or return descriptions for its request/response payloads. Please add source XML comments for the action parameters/returns and theCheckInTimersDTO properties, then regenerate this file.🤖 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 311 - 365, The generated XML lacks parameter/return and DTO member docs for the new CheckInTimers v4 API; open the controller methods (GetTimerConfigs, SaveTimerConfig, DeleteTimerConfig, GetTimerOverrides, SaveTimerOverride, DeleteTimerOverride, GetTimersForCall, GetTimerStatuses, PerformCheckIn, GetCheckInHistory, ToggleCallTimers) and add C# XML comments for each action's parameters and returns, then add XML comments on all DTO properties under Resgrid.Web.Services.Models.v4.CheckInTimers (e.g., CheckInTimerConfigInput, CheckInTimerOverrideInput, PerformCheckInInput and any response models) describing fields and expected values, rebuild to regenerate Resgrid.Web.Services.xml so the contract includes parameter/return descriptions and DTO member documentation.Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs (1)
64-72: Split the new timer settings surface out ofDepartmentController.Adding
ICheckInTimerServicehere pushes an already very large constructor further out of shape and makes this controller harder to test. The new check-in timer CRUD/actions look substantial enough for a dedicated controller or composed settings coordinator instead of continuing to grow this one.As per coding guidelines "Minimize constructor injection; keep the number of injected dependencies small".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs` around lines 64 - 72, The DepartmentController constructor has grown too large by adding ICheckInTimerService; extract the check‑in timer functionality into its own controller or coordinator to reduce constructor injection and improve testability: remove ICheckInTimerService from DepartmentController's ctor and move related CRUD/actions into a new CheckInTimerController (or CheckInTimerSettingsCoordinator) that depends only on ICheckInTimerService and any minimal collaborators; update routing/DI to register the new controller/service and adjust any calls from DepartmentController to delegate to the new component (reference symbols: DepartmentController, ICheckInTimerService, and the check‑in timer CRUD/action methods).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 276: Remove the duplicate `.claude/settings.local.json` entry from
.gitignore by deleting the redundant line that repeats the same pattern; look
for the string ".claude/settings.local.json" (it appears twice) and remove the
second occurrence so the file contains only a single entry for that path.
In `@Core/Resgrid.Services/CheckInTimerService.cs`:
- Around line 36-52: When updating or deleting timers, first verify the existing
entity's DepartmentId matches the caller's department before persisting or
removing it: in SaveTimerConfigAsync, when config.CheckInTimerConfigId is
present, load the existing entity via _configRepository.GetByIdAsync(existingId)
and compare existing.DepartmentId to the caller's department (either
config.DepartmentId or a new callerDepartmentId parameter); if they differ,
reject (throw UnauthorizedAccessException or return an error) instead of saving;
similarly in DeleteTimerConfigAsync, after loading config from
_configRepository.GetByIdAsync(configId) verify config.DepartmentId matches the
caller's department and return false or throw when it does not before calling
_configRepository.DeleteAsync.
- Around line 88-152: ResolveAllTimersForCallAsync collapses different concrete
targets into a single TimerTargetType/UnitTypeId bucket; change the resolution
to carry a concrete target identity through and use that same identity when
computing statuses. Specifically: add a TargetId (or ConcreteTargetId) property
to ResolvedCheckInTimer and populate it when building defaults and when applying
overrides in ResolveAllTimersForCallAsync (use the config/override field that
identifies the specific unit/person/team), change the dictionary key to include
TargetId (e.g., $"{def.TimerTargetType}_{def.UnitTypeId}_{def.TargetId}"),
preserve IsFromOverride, then update GetActiveTimerStatusesForCallAsync to match
check-ins using the identical composite key (Timer/CheckIn type + UnitTypeId +
TargetId) when selecting latest records; also fix the reporting bug by returning
UnitId where Line 222 currently emits UnitTypeId.
- Around line 101-149: The override application currently materializes all
overrides into resolved regardless of o.IsEnabled; update the loop that
processes scoredOverrides in CheckInTimerService (the block using
scoredOverrides, o, key, and resolved) to skip any override where o.IsEnabled is
false (i.e., continue to next scored item) so disabled overrides are not applied
and therefore cannot suppress or replace defaults; ensure you only construct a
ResolvedCheckInTimer and set IsFromOverride = true when o.IsEnabled == true.
In `@Core/Resgrid.Services/ServicesModule.cs`:
- Line 89: The registration for CheckInTimerService currently uses
SingleInstance() and creates a captive dependency with its scoped constructor
dependencies; change the Autofac registration of
CheckInTimerService.As<ICheckInTimerService>() to use InstancePerLifetimeScope()
(instead of SingleInstance()) so the service shares the same lifetime as its
InstancePerLifetimeScope() dependencies and avoids elevating scoped repositories
to singletons; verify no other registrations reference CheckInTimerService as a
singleton.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0056_AddingCheckInTimers.cs`:
- Around line 31-33: The UNIQUE constraints
UQ_CheckInTimerConfigs_Dept_Target_Unit (on CheckInTimerConfigs: DepartmentId,
TimerTargetType, UnitTypeId) and UQ_CheckInTimerOverrides (on
CheckInTimerOverrides: DepartmentId, TimerTargetType, CallTypeId, CallPriority,
UnitTypeId) include nullable columns and will prevent multiple NULLs; replace
them with SQL Server filtered unique nonclustered indexes that enforce
uniqueness only when those nullable columns are NOT NULL (e.g., create unique
nonclustered index for CheckInTimerConfigs on DepartmentId, TimerTargetType,
UnitTypeId WHERE UnitTypeId IS NOT NULL, and similarly for CheckInTimerOverrides
include a WHERE clause excluding rows where CallTypeId IS NULL OR CallPriority
IS NULL OR UnitTypeId IS NULL as appropriate) so multiple NULL-valued rows are
allowed.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0056_AddingCheckInTimersPg.cs`:
- Around line 83-90: The migration currently creates ix_checkinrecords_callid
and ix_checkinrecords_departmentid_timestamp but does not support the hot
"latest check-in" lookups by (callid, userid) and (callid, unitid); update the
migration to add compound indexes that match those access patterns by creating
indexes on table "checkinrecords" for (callid, userid, timestamp DESC) and
(callid, unitid, timestamp DESC) so queries seeking the latest record per
user/unit under a call can use the index rather than scanning/sorting—place
these alongside the existing Create.Index calls (refer to the existing
Create.Index("ix_checkinrecords_callid") and
Create.Index("ix_checkinrecords_departmentid_timestamp") usages to locate where
to add the new index creations).
- Around line 31-33: The migration's current unique constraint
Create.UniqueConstraint("uq_checkintimerconfigs_dept_target_unit") on
checkintimerconfigs still allows duplicate "Any/None" rules because PostgreSQL
treats NULLs as distinct; update the migration to enforce uniqueness for
nullable override columns by either (a) using a NULLS NOT DISTINCT unique index
if targeting PostgreSQL 15+ (replace the Create.UniqueConstraint with a CREATE
UNIQUE INDEX ... ON checkintimeroverrides (...) NULLS NOT DISTINCT), or (b) add
partial unique indexes that cover only non-NULL combinations for the nullable
override columns (e.g., unique partial indexes on checkintimeroverrides for
(departmentid, timertargettype, calltypeid) WHERE calltypeid IS NOT NULL,
similarly for callpriority and unittypeid) so that duplicates for "Any/None"
cannot be inserted; ensure you reference and update the constraint name
("uq_checkintimerconfigs_dept_target_unit") or create appropriate index names to
avoid conflicts.
In
`@Repositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectLastCheckInForUnitOnCallQuery.cs`:
- Around line 28-30: The generic GetQuery<TEntity>() in the check-in query
classes currently throws NotImplementedException; update each of the eight
classes (SelectCheckInRecordsByCallIdQuery,
SelectCheckInRecordsByDepartmentIdAndDateRangeQuery,
SelectCheckInTimerConfigByDepartmentAndTargetQuery,
SelectCheckInTimerConfigsByDepartmentIdQuery,
SelectCheckInTimerOverridesByDepartmentIdQuery,
SelectLastCheckInForUnitOnCallQuery, SelectLastCheckInForUserOnCallQuery,
SelectMatchingCheckInTimerOverridesQuery) so the generic overload simply
delegates to the non-generic GetQuery() (i.e., implement GetQuery<TEntity>() to
return GetQuery()) to avoid runtime failures when _queryFactory.GetQuery<T>() is
called.
In `@Web/Resgrid.Web.Services/Hubs/EventingHub.cs`:
- Around line 101-115: The CheckInPerformed and CheckInTimersUpdated hub methods
(and Connect()) currently accept caller-supplied departmentId on an
[AllowAnonymous] hub, allowing unauthenticated clients to broadcast to arbitrary
groups; fix by restricting access or validating membership: either add
[Authorize] to the hub class (or the specific methods) to require authenticated
callers, or perform server-side validation inside CheckInPerformed,
CheckInTimersUpdated and Connect() by verifying the caller's identity/claims
(e.g., user id and department membership from Context.User) matches the supplied
departmentId before calling Clients.Group/departments operations; if these are
intended for server-only use, remove the public hub methods and invoke
broadcasts via IHubContext instead.
In `@Web/Resgrid.Web.Services/Models/v4/CheckInTimers/CheckInTimerModels.cs`:
- Around line 32-41: The DTOs accept zero/negative defaults because non-nullable
ints bind to 0 and [Required] doesn't reject that; update the three input models
to enforce positive values by adding a Range(1, int.MaxValue) constraint to the
specified properties: in CheckInTimerConfigInput add Range to TimerTargetType,
DurationMinutes, WarningThresholdMinutes; in CheckInTimerOverrideInput add Range
to TimerTargetType, DurationMinutes, WarningThresholdMinutes; and in
PerformCheckInInput add Range to CallId and CheckInType so invalid (0 or
negative) values are rejected at model validation.
In `@Web/Resgrid.Web/Areas/User/Models/Dispatch/PerformCheckInInput.cs`:
- Around line 5-10: The PerformCheckInInput DTO accepts unconstrained values;
add DataAnnotations to validate at the boundary: mark CallId and CheckInType
with [Required] and [Range(1, int.MaxValue)] (or an appropriate enum range) to
ensure positive IDs, add [Required] plus a numeric/coordinate regex via
[RegularExpression(@"^-?\d+(\.\d+)?$")] (or a more specific lat/lon regex) to
Latitude and Longitude to ensure valid decimal coordinates, mark UnitId as
nullable but add [Range(1, int.MaxValue)] to prevent zero/negative IDs, and
constrain Note with [StringLength(1000)] (or desired max) to limit payload size;
apply these attributes to the properties CallId, CheckInType, Latitude,
Longitude, UnitId, and Note in the PerformCheckInInput class so malformed
check-ins are rejected before business logic runs.
In `@Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml`:
- Around line 163-166: Wrap the localized string used inside the onclick confirm
call with Html.Raw so Razor doesn't HTML-encode the already JavaScript-encoded
value: generate a JavaScript-safe string for
localizer["CheckInTimerDeleteConfirm"] (using the same JS-encoding helper
pattern used elsewhere in the project) and then output it with `@Html.Raw`(...)
inside the single-quoted confirm(...) literal; apply the same change to the
other occurrences mentioned (the block around lines 278-281) so all confirm(...)
usages use Html.Raw around the JS-encoded localized text.
In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/CallExport.cshtml`:
- Around line 5-6: Remove the duplicate Razor injection: there are two `@inject`
declarations creating the same property localizer of type
IStringLocalizer<Resgrid.Localization.Areas.User.Dispatch.Call>; keep a single
`@inject` IStringLocalizer<Resgrid.Localization.Areas.User.Dispatch.Call>
localizer and delete the other so the compiler does not generate duplicate
property declarations (search for the duplicate `@inject` lines referencing
"localizer" to locate them).
In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/NewCall.cshtml`:
- Around line 410-412: Replace the raw checkbox input with the MVC tag helper
bound to the model property (use asp-for="Call.CheckInTimersEnabled") so ASP.NET
Core will render the checked state on re-render and include the hidden false
input for reliable posting; keep the surrounding label and the
`@localizer`["EnableCheckInTimers"] text but change the <input> to use asp-for on
the Call.CheckInTimersEnabled property to ensure model binding preserves state.
In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtml`:
- Around line 774-833: The PerformCheckIn POST currently sends JSON from
resgrid.dispatch.checkin-timers.js without an antiforgery token and the
corresponding controller action in DispatchController (PerformCheckIn) lacks
[ValidateAntiForgeryToken]; fix by adding antiforgery protection: update the
client (resgrid.dispatch.checkin-timers.js -> PerformCheckIn request) to include
the antiforgery token (e.g., read the token from the page meta/cookie/header)
and modify the server action (DispatchController.PerformCheckIn) to validate the
token—either add [ValidateAntiForgeryToken] and accept the token in a header (or
change to a form-bound DTO) or call IAntiforgery.ValidateRequestAsync inside the
action if you must keep JSON binding; ensure token name used on client matches
what the server expects.
In `@Web/Resgrid.Web/Areas/User/Views/Templates/Edit.cshtml`:
- Around line 96-103: Replace the manual checkbox input for
Model.Template.CheckInTimersEnabled with the helper that emits the hidden
fallback input so false values are posted; locate the checkbox using the symbol
Model.Template.CheckInTimersEnabled and change the markup to use
`@Html.CheckBoxFor`(m => m.Template.CheckInTimersEnabled) (keeping the surrounding
label and layout) to ensure proper model binding of both true and false.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.js`:
- Around line 322-326: The checkbox is left in a stale state when
data.CheckInTimersEnabled is null/undefined; replace the if/else block with a
single deterministic assignment for $('input[name="Call.CheckInTimersEnabled"]')
using a boolean conversion of data.CheckInTimersEnabled (e.g., use
Boolean(data.CheckInTimersEnabled) or !!data.CheckInTimersEnabled) so the
checkbox is explicitly set to checked/unchecked every time; update the code
around the existing input selector and the data.CheckInTimersEnabled reference
accordingly.
---
Nitpick comments:
In `@Core/Resgrid.Model/CheckInTimerStatus.cs`:
- Around line 5-24: Replace the mutable CheckInTimerStatus class with an
immutable record to represent state and add a CheckInTimerState enum for Status
to avoid magic strings; specifically change the type named CheckInTimerStatus
into a record (positional or read-only properties) and introduce an enum
CheckInTimerState with values like Green, Warning, Critical, then update the
Status property/type to use CheckInTimerState across usages to preserve type
safety and prevent string typos.
In `@Core/Resgrid.Model/CheckInTimerTargetType.cs`:
- Around line 7-8: The enum CheckInTimerTargetType contains terse members IC and
PAR which are unclear; rename these to descriptive identifiers (for example IC
-> IndividualCheckIn or IncidentCheckIn and PAR -> Paramedic or Participant)
throughout the codebase (search/replace occurrences of CheckInTimerTargetType.IC
and CheckInTimerTargetType.PAR) and update any XML docs or comments for the enum
and its members to describe their purpose; ensure serialization names or
mappings (if used) are preserved or updated (e.g., add EnumMember attributes or
maintain backward-compatible numeric values 2 and 3) so external consumers are
not broken.
In `@Core/Resgrid.Model/ResolvedCheckInTimer.cs`:
- Around line 3-18: Convert the mutable POCO ResolvedCheckInTimer into an
immutable state record by replacing the class with a record type (keep the name
ResolvedCheckInTimer) and turn all properties (TargetType, UnitTypeId,
TargetEntityId, TargetName, DurationMinutes, WarningThresholdMinutes,
IsFromOverride) into init-only properties (or positional record parameters) so
instances are immutable after construction; update any call sites that assign to
these properties to construct a new record instance instead.
In
`@Repositories/Resgrid.Repositories.DataRepository/CheckInTimerOverrideRepository.cs`:
- Around line 48-61: Refactor the duplicated connection/transaction handling in
CheckInTimerOverrideRepository by extracting a shared helper (e.g.,
ExecuteSelectAsync) that takes a delegate Func<DbConnection, DbTransaction,
Task<IEnumerable<CheckInTimerOverride>>>; inside the helper, if
_unitOfWork?.Connection is null create and open a using connection from
_connectionProvider.Create() and pass null for the transaction, otherwise get
the connection via _unitOfWork.CreateOrGetConnection() and pass
_unitOfWork.Transaction; update existing methods to call ExecuteSelectAsync and
change their selectFunction delegates to accept the explicit DbTransaction
parameter to eliminate duplication and make transaction handling explicit.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 311-365: The generated XML lacks parameter/return and DTO member
docs for the new CheckInTimers v4 API; open the controller methods
(GetTimerConfigs, SaveTimerConfig, DeleteTimerConfig, GetTimerOverrides,
SaveTimerOverride, DeleteTimerOverride, GetTimersForCall, GetTimerStatuses,
PerformCheckIn, GetCheckInHistory, ToggleCallTimers) and add C# XML comments for
each action's parameters and returns, then add XML comments on all DTO
properties under Resgrid.Web.Services.Models.v4.CheckInTimers (e.g.,
CheckInTimerConfigInput, CheckInTimerOverrideInput, PerformCheckInInput and any
response models) describing fields and expected values, rebuild to regenerate
Resgrid.Web.Services.xml so the contract includes parameter/return descriptions
and DTO member documentation.
In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs`:
- Around line 64-72: The DepartmentController constructor has grown too large by
adding ICheckInTimerService; extract the check‑in timer functionality into its
own controller or coordinator to reduce constructor injection and improve
testability: remove ICheckInTimerService from DepartmentController's ctor and
move related CRUD/actions into a new CheckInTimerController (or
CheckInTimerSettingsCoordinator) that depends only on ICheckInTimerService and
any minimal collaborators; update routing/DI to register the new
controller/service and adjust any calls from DepartmentController to delegate to
the new component (reference symbols: DepartmentController,
ICheckInTimerService, and the check‑in timer CRUD/action methods).
In `@Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs`:
- Around line 1649-1711: The GetCheckInTimerStatuses action is missing an
explicit authorization attribute; add the same policy used by other call
endpoints by decorating the GetCheckInTimerStatuses method with
[Authorize(Policy = ResgridResources.Call_View)] so it enforces the Call_View
permission (keep the existing DepartmentId check and method logic intact).
- Around line 1751-1778: The PerformCheckIn action lacks an authorization
attribute; add an authorization policy attribute (e.g. [Authorize(Policy =
ResgridResources.Call_View)] or a more restrictive policy you have for modifying
calls) to the PerformCheckIn method so only authorized users can create check-in
records; locate the PerformCheckIn method in DispatchController and apply the
attribute directly above the method signature to enforce the policy.
- Around line 1713-1749: Add the missing authorization attribute to the
GetCheckInHistory action: apply [Authorize(Policy = ResgridResources.Call_View)]
to the GetCheckInHistory method (same as used on GetCheckInTimerStatuses) so
access is restricted consistently; update the method signature above public
async Task<IActionResult> GetCheckInHistory(int callId) to include the attribute
and run tests to verify authorization behavior.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js`:
- Around line 7-14: The interval set by timerInterval in the document ready
block (where loadTimerStatuses() and setInterval(updateCountdowns, 1000) are
called) is never cleared; add cleanup logic to stop the interval when the call
is no longer active or the page unloads by implementing a stopTimerInterval
function that checks timerInterval, calls clearInterval(timerInterval), and
nulls it, and invoke stopTimerInterval when callState changes or on window
beforeunload/unload to ensure updateCountdowns is not left running
unnecessarily.
- Around line 16-27: Add error handling for the AJAX calls in the
loadTimerStatuses and loadCheckInHistory functions: attach .fail(...) handlers
to the $.ajax(...) chains used in loadTimerStatuses and loadCheckInHistory, log
the error (e.g., console.error or existing logger) and provide user feedback
(e.g., show a toast/alert or display an error message in the UI where
renderTimerStatuses or history rendering would normally go), and optionally
retry or disable UI elements until a successful load; ensure you reference the
existing timerStatuses and renderTimerStatuses usage so failures keep the UI
consistent.
🪄 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: a43aff96-8354-480c-a2f0-07054022c380
⛔ Files ignored due to path filters (28)
Core/Resgrid.Localization/Areas/User/Department/Department.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.uk.resxis excluded by!**/*.resxCore/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!**/*.resxCore/Resgrid.Localization/Areas/User/Templates/Templates.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Templates/Templates.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Templates/Templates.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Templates/Templates.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Templates/Templates.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Templates/Templates.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Templates/Templates.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Templates/Templates.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Templates/Templates.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (62)
.gitignoreCore/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Model/Call.csCore/Resgrid.Model/CallQuickTemplate.csCore/Resgrid.Model/CheckInRecord.csCore/Resgrid.Model/CheckInTimerConfig.csCore/Resgrid.Model/CheckInTimerOverride.csCore/Resgrid.Model/CheckInTimerStatus.csCore/Resgrid.Model/CheckInTimerTargetType.csCore/Resgrid.Model/DepartmentSettingTypes.csCore/Resgrid.Model/Repositories/ICheckInRecordRepository.csCore/Resgrid.Model/Repositories/ICheckInTimerConfigRepository.csCore/Resgrid.Model/Repositories/ICheckInTimerOverrideRepository.csCore/Resgrid.Model/ResolvedCheckInTimer.csCore/Resgrid.Model/Services/ICheckInTimerService.csCore/Resgrid.Model/Services/IDepartmentSettingsService.csCore/Resgrid.Services/CheckInTimerService.csCore/Resgrid.Services/DepartmentSettingsService.csCore/Resgrid.Services/ServicesModule.csProviders/Resgrid.Providers.Migrations/Migrations/M0056_AddingCheckInTimers.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0056_AddingCheckInTimersPg.csRepositories/Resgrid.Repositories.DataRepository/CheckInRecordRepository.csRepositories/Resgrid.Repositories.DataRepository/CheckInTimerConfigRepository.csRepositories/Resgrid.Repositories.DataRepository/CheckInTimerOverrideRepository.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/CheckIns/SelectCheckInRecordsByCallIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInRecordsByDepartmentIdAndDateRangeQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInTimerConfigByDepartmentAndTargetQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInTimerConfigsByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInTimerOverridesByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectLastCheckInForUnitOnCallQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectLastCheckInForUserOnCallQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectMatchingCheckInTimerOverridesQuery.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csTests/Resgrid.Tests/Services/CheckInTimerServiceTests.csWeb/Resgrid.Web.Services/Controllers/v4/CallsController.csWeb/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.csWeb/Resgrid.Web.Services/Hubs/EventingHub.csWeb/Resgrid.Web.Services/Models/v4/Calls/CallResult.csWeb/Resgrid.Web.Services/Models/v4/Calls/NewCallInput.csWeb/Resgrid.Web.Services/Models/v4/CheckInTimers/CheckInTimerModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Models/Departments/DispatchSettingsView.csWeb/Resgrid.Web/Areas/User/Models/Dispatch/CallExportView.csWeb/Resgrid.Web/Areas/User/Models/Dispatch/PerformCheckInInput.csWeb/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtmlWeb/Resgrid.Web/Areas/User/Views/Department/Settings.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/CallExport.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/NewCall.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/UpdateCall.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtmlWeb/Resgrid.Web/Areas/User/Views/Templates/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Templates/New.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.js
| /Web/Resgrid.Web/wwwroot/lib | ||
| .dual-graph/ | ||
| .claude/settings.local.json | ||
| .claude/settings.local.json |
Contributor
There was a problem hiding this comment.
Remove duplicate .gitignore entry.
The entry .claude/settings.local.json already exists at line 275. This duplicate adds no value and should be removed to keep the configuration clean.
🧹 Proposed fix
-.claude/settings.local.json📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .claude/settings.local.json |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore at line 276, Remove the duplicate `.claude/settings.local.json`
entry from .gitignore by deleting the redundant line that repeats the same
pattern; look for the string ".claude/settings.local.json" (it appears twice)
and remove the second occurrence so the file contains only a single entry for
that path.
Comment on lines
+18
to
+20
| public int DurationMinutes { get; set; } | ||
|
|
||
| public int WarningThresholdMinutes { get; set; } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether timer invariants are enforced in service logic and migrations.
# 1) Find check-in timer service implementations.
fd -i 'CheckInTimerService.cs' | xargs -r rg -nP 'DurationMinutes|WarningThresholdMinutes|<=\s*0|<\s*0|>\s*.*DurationMinutes|throw|Validation|Argument'
# 2) Find migrations creating CheckInTimerConfigs and inspect constraints.
fd -i 'M0056*CheckInTimers*.cs' | xargs -r rg -nP 'CheckInTimerConfigs|DurationMinutes|WarningThresholdMinutes|CHECK|constraint|Unique|Index'
# 3) Broader search for DB check constraints on these columns.
rg -nP 'CheckInTimerConfigs|DurationMinutes|WarningThresholdMinutes|CHECK\s*\('Repository: Resgrid/Core
Length of output: 14477
Add validation to enforce timer invariant constraints at service and database boundaries.
Invalid timer configurations can persist and cause broken timer behavior. Currently, neither the service layer nor the database schema enforce constraints: DurationMinutes must be > 0, WarningThresholdMinutes must be ≥ 0, and WarningThresholdMinutes must be ≤ DurationMinutes. The service logic at CheckInTimerService (line 212) performs unchecked arithmetic (DurationMinutes + WarningThresholdMinutes), and the migration (M0056_AddingCheckInTimers.cs) defines columns as Int32.NotNullable() without CHECK constraints.
Add explicit validation in the controller/service before persistence and add database-level CHECK constraints in the migration to prevent invalid state.
Comment on lines
+96
to
+103
| <div class="form-group"> | ||
| <label class="col-sm-2 control-label"> | ||
| @localizer["EnableCheckInTimersLabel"] | ||
| </label> | ||
| <div class="col-sm-10"> | ||
| <div class="col-md-10"> | ||
| <input type="checkbox" name="Template.CheckInTimersEnabled" value="true" @(Model.Template.CheckInTimersEnabled == true ? "checked" : "") /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In ASP.NET Core MVC, when posting a form, does an unchecked submit any value, and is a hidden input required for reliablefalse binding?
💡 Result:
No, an unchecked does not submit any value to the server. This is standard HTML form behavior: checkboxes only include their name-value pair in the form data if checked. For reliable false binding in ASP.NET Core MVC model binding to a bool property: - Use the asp-for tag helper: . This automatically renders a checkbox (value="true") + hidden input (value="false", same name). On post: - Checked: submits true,false; binder takes first (true). - Unchecked: submits only false; binder sets false. - Manual HTML requires explicit hidden input before checkbox: . This ensures consistent true/false binding without custom logic. Raw HTML checkbox alone binds true if checked, default false if absent (reliable for bool defaults), but tag helper guarantees explicit false value.
Citations:
- 1: https://stackoverflow.com/questions/1809494/post-unchecked-html-checkboxes
- 2: https://stackoverflow.com/questions/77181652/checkbox-values-not-returning-to-controller-asp-net-core-7-mvc
- 3: https://learn.microsoft.com/en-us/answers/questions/1179859/dont-see-the-value-of-the-checkboxes-in-the-http-p
- 4: https://stackoverflow.com/questions/57079165/why-do-my-checkboxes-not-pass-their-value-to-my-controller-in-asp-net-core/57087930
- 5: https://stackoverflow.com/questions/50513690/asp-net-core-tag-helper-checkbox-does-not-save-unchecked-value/50514167
- 6: https://www.learnrazorpages.com/razor-pages/forms/checkboxes
- 7: https://stackoverflow.com/questions/50513690/asp-net-core-tag-helper-checkbox-does-not-save-unchecked-value
- 8: https://stackoverflow.com/questions/57079165/why-do-my-checkboxes-not-pass-their-value-to-my-controller-in-asp-net-core
🏁 Script executed:
# Check if the file exists and read the relevant section
if [ -f "Web/Resgrid.Web/Areas/User/Views/Templates/Edit.cshtml" ]; then
echo "=== File exists ==="
wc -l "Web/Resgrid.Web/Areas/User/Views/Templates/Edit.cshtml"
echo ""
echo "=== Lines 96-110 ==="
sed -n '96,110p' "Web/Resgrid.Web/Areas/User/Views/Templates/Edit.cshtml" | cat -n
else
echo "File not found at that path"
fd -t f "Edit.cshtml" | head -20
fiRepository: Resgrid/Core
Length of output: 1301
🏁 Script executed:
# Search for other manual checkbox instances in the same file
echo "=== All checkboxes in the file ==="
rg -n 'type="checkbox"' "Web/Resgrid.Web/Areas/User/Views/Templates/Edit.cshtml" -A 2 -B 1Repository: Resgrid/Core
Length of output: 412
🏁 Script executed:
# Find the controller handling this Edit action
fd -t f "TemplatesController.cs" Web/Repository: Resgrid/Core
Length of output: 180
🏁 Script executed:
# Search for the Edit action in Templates controller
rg -n "public.*Edit" --type cs "Web/Resgrid.Web/Areas/User/Controllers/" | grep -i templateRepository: Resgrid/Core
Length of output: 636
🏁 Script executed:
# Examine the POST Edit action to see how binding and model update work
sed -n '123,180p' "Web/Resgrid.Web/Areas/User/Controllers/TemplatesController.cs" | cat -nRepository: Resgrid/Core
Length of output: 2236
Use @Html.CheckBoxFor() to reliably persist checkbox state.
Line 102's manual checkbox markup does not emit a hidden input, which is the standard ASP.NET Core approach for ensuring unchecked values are posted. While default model binding typically handles missing bool values as false, relying on this implicit behavior is fragile. An edit flow that preserves prior state could leave stale values if form data is incomplete or custom binding logic exists. Use @Html.CheckBoxFor(m => m.Template.CheckInTimersEnabled) to emit the proper hidden fallback input that guarantees both true and false states are reliably captured.
Suggested change
- <input type="checkbox" name="Template.CheckInTimersEnabled" value="true" @(Model.Template.CheckInTimersEnabled == true ? "checked" : "") />
+ `@Html.CheckBoxFor`(m => m.Template.CheckInTimersEnabled)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Templates/Edit.cshtml` around lines 96 -
103, Replace the manual checkbox input for Model.Template.CheckInTimersEnabled
with the helper that emits the hidden fallback input so false values are posted;
locate the checkbox using the symbol Model.Template.CheckInTimersEnabled and
change the markup to use `@Html.CheckBoxFor`(m => m.Template.CheckInTimersEnabled)
(keeping the surrounding label and layout) to ensure proper model binding of
both true and false.
Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.js
Outdated
Show resolved
Hide resolved
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status401Unauthorized)] | ||
| [Authorize(Policy = ResgridResources.Schedule_View)] | ||
| public async Task<ActionResult<SetCalendarCheckInResult>> SetCalendarCheckIn([FromBody] CalendarCheckInInput 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 2 days ago
In general, to fix missing CSRF validation in ASP.NET Core MVC/API controllers that are using cookie-based authentication, you apply the built-in antiforgery validation filter (e.g., [ValidateAntiForgeryToken] or [AutoValidateAntiforgeryToken]) to state-changing methods (POST, PUT, DELETE, etc.), and ensure the client includes a valid antiforgery token with those requests. This causes ASP.NET Core to automatically validate the token on each such request and reject forged ones.
For this specific method, the minimal change without altering existing behavior is to add an antiforgery validation attribute to SetCalendarCheckIn. In ASP.NET Core, the typical attribute is [ValidateAntiForgeryToken] applied to individual actions that must be protected. This attribute is defined in Microsoft.AspNetCore.Mvc, which is already imported at the top of the file (using Microsoft.AspNetCore.Mvc;), so no new imports are needed. You should place [ValidateAntiForgeryToken] alongside the other attributes decorating the method, for example directly under [HttpPost("SetCalendarCheckIn")]. No other code in the method body needs to change.
Concretely, in Web/Resgrid.Web.Services/Controllers/v4/CalendarController.cs, around line 273–282 where SetCalendarCheckIn is declared, insert the [ValidateAntiForgeryToken] attribute in the attribute list. This will cause ASP.NET Core to enforce CSRF token validation whenever this POST endpoint is invoked, assuming the application is configured with the standard antiforgery services and the client sends the token.
Suggested changeset
1
Web/Resgrid.Web.Services/Controllers/v4/CalendarController.cs
| @@ -274,6 +274,7 @@ | ||
| /// Checks in to a calendar event | ||
| /// </summary> | ||
| [HttpPost("SetCalendarCheckIn")] | ||
| [ValidateAntiForgeryToken] | ||
| [Consumes(MediaTypeNames.Application.Json)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] |
Copilot is powered by AI and may make mistakes. Always verify output.
| { | ||
| var result = new SetCalendarCheckInResult(); | ||
|
|
||
| if (input == null || input.CalendarEventId <= 0) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Copilot Autofix
AI 1 day 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.
| { | ||
| var result = new SetCalendarCheckInResult(); | ||
|
|
||
| if (input == null || input.CalendarEventId <= 0) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
General approach: Separate null/shape validation of the user-controlled input from the sensitive authorization branching, and avoid relying directly on user-controlled flags to choose which authorization method to invoke. Derive the “admin check-in” decision from server-side information (e.g., whether the authenticated user is checking in someone else) and keep the structure simple so that a branch leading to Unauthorized() is not controlled in a way that confuses the analyzer.
Best concrete fix here:
- Make the null/
CalendarEventIdvalidation an early, standalone guard, then proceed with the authorization logic in a clearly separated block. - Slightly restructure the
targetUserId/isAdminCheckIncalculation so it’s clearer and less dependent on an inline ternary with user input that immediately affects the subsequent security check. We keep the functional behavior the same:- If
input.UserIdis non-empty and different fromUserId, we treat it as an admin check-in to that target user, and we call_authorizationService.CanUserAdminCheckInCalendarEventAsync. - Otherwise, we treat it as a self check-in and call
_authorizationService.CanUserCheckInToCalendarEventAsync.
- If
- Preserve all existing result shaping and auditing behavior.
Concretely, in Web/Resgrid.Web.Services/Controllers/v4/CalendarController.cs inside SetCalendarCheckIn:
- Keep the initial
if (input == null || input.CalendarEventId <= 0) return BadRequest();but then put the rest of the logic into anelseblock so that the analyzer sees that the sensitive branches are not directly guarded by a condition involving user input. - Expand the
targetUserId/isAdminCheckIncomputation into a couple of simple, explicit statements.
No new imports or helper methods are required.
Suggested changeset
1
Web/Resgrid.Web.Services/Controllers/v4/CalendarController.cs
| @@ -283,12 +283,20 @@ | ||
| { | ||
| var result = new SetCalendarCheckInResult(); | ||
|
|
||
| // Validate basic shape of the request before performing any authorization decisions. | ||
| if (input == null || input.CalendarEventId <= 0) | ||
| return BadRequest(); | ||
|
|
||
| var targetUserId = !string.IsNullOrWhiteSpace(input.UserId) && input.UserId != UserId ? input.UserId : UserId; | ||
| // Determine which user is being checked in. Default to the current user when the input | ||
| // user id is missing or matches the current user; otherwise, treat as an admin check-in. | ||
| var targetUserId = UserId; | ||
| if (!string.IsNullOrWhiteSpace(input.UserId) && input.UserId != UserId) | ||
| targetUserId = input.UserId; | ||
|
|
||
| var isAdminCheckIn = targetUserId != UserId; | ||
|
|
||
| // Authorization: choose the appropriate server-side authorization check based on whether | ||
| // the authenticated user is checking in someone else. | ||
| if (isAdminCheckIn) | ||
| { | ||
| if (!await _authorizationService.CanUserAdminCheckInCalendarEventAsync(UserId, input.CalendarEventId, targetUserId)) |
Copilot is powered by AI and may make mistakes. Always verify output.
| { | ||
| var result = new SetCalendarCheckInResult(); | ||
|
|
||
| if (input == null || input.CalendarEventId <= 0) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Copilot Autofix
AI 1 day 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.
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status401Unauthorized)] | ||
| [Authorize(Policy = ResgridResources.Schedule_View)] | ||
| public async Task<ActionResult<SetCalendarCheckInResult>> SetCalendarCheckOut([FromBody] CalendarCheckOutInput 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 2 days ago
In general, state-changing POST actions that can be invoked from a browser with ambient credentials should validate an anti-forgery token. In ASP.NET Core MVC, this is done by applying [ValidateAntiForgeryToken] to the action (or controller) and ensuring that clients send the antiforgery token with each POST (e.g., via form field or header). This prevents attackers from causing unwanted actions via cross-site requests.
For this specific method, the minimal, non-breaking fix is to decorate SetCalendarCheckOut with the antiforgery validation attribute. Since we already use ASP.NET Core MVC and have using Microsoft.AspNetCore.Mvc; at the top of the file, we can directly add [ValidateAntiForgeryToken] above the action without changing its signature or logic. This will cause the framework to automatically validate the antiforgery token for requests to POST /SetCalendarCheckOut. Any clients that currently call this endpoint without sending an antiforgery token will begin to receive 400 responses until they are updated to include a valid token. If this endpoint is intended to be called only by non-browser clients using bearer tokens, you might choose instead to exempt it explicitly with [IgnoreAntiforgeryToken], but since the CodeQL issue is about missing validation and we are to fix it, the secure choice is to add [ValidateAntiForgeryToken].
Concretely:
- In
Web/Resgrid.Web.Services/Controllers/v4/CalendarController.cs, locate theSetCalendarCheckOutaction (lines 334–343 in the snippet). - Insert a
[ValidateAntiForgeryToken]attribute between[Consumes(...)]and[ProducesResponseType(...)](or anywhere in the attribute list for that method). - No new imports are needed;
ValidateAntiForgeryTokenAttributelives inMicrosoft.AspNetCore.Mvc, which is already imported. - No additional helper methods or definitions are required; the runtime antiforgery configuration is assumed to be set up where appropriate in the application startup.
Suggested changeset
1
Web/Resgrid.Web.Services/Controllers/v4/CalendarController.cs
| @@ -336,6 +336,7 @@ | ||
| /// </summary> | ||
| [HttpPost("SetCalendarCheckOut")] | ||
| [Consumes(MediaTypeNames.Application.Json)] | ||
| [ValidateAntiForgeryToken] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status401Unauthorized)] |
Copilot is powered by AI and may make mistakes. Always verify output.
| { | ||
| var result = new SetCalendarCheckInResult(); | ||
|
|
||
| if (input == null || input.CalendarEventId <= 0) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Copilot Autofix
AI 1 day 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.
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status401Unauthorized)] | ||
| [Authorize(Policy = ResgridResources.Schedule_Update)] | ||
| public async Task<ActionResult<SetCalendarCheckInResult>> UpdateCalendarCheckIn([FromBody] CalendarCheckInUpdateInput 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 2 days ago
In general, the way to fix missing CSRF token validation in ASP.NET Core MVC is to ensure that any state-changing POST (or other unsafe HTTP verbs) is protected via the built-in anti-forgery system: the client sends a token (usually via form field or header), and the server validates it with [ValidateAntiForgeryToken] on the action (or a global filter). For APIs, a common pattern is to require the anti-forgery token in a header such as X-CSRF-TOKEN, which still uses the same middleware.
For this codebase, the minimal, non-breaking change is to decorate UpdateCalendarCheckIn with the anti-forgery validation attribute while leaving all existing logic intact. Since this is an ASP.NET Core controller using Microsoft.AspNetCore.Mvc, the appropriate attribute is ValidateAntiForgeryToken from that namespace (already imported via using Microsoft.AspNetCore.Mvc;). No method code needs to change; we only add the attribute to enforce CSRF token validation when the endpoint is called. The client-side will need to include a valid anti-forgery token for these requests, but that is outside the scope of the current file.
Concretely, in Web/Resgrid.Web.Services/Controllers/v4/CalendarController.cs, just above the UpdateCalendarCheckIn method definition, add [ValidateAntiForgeryToken] on its own line, alongside the existing attributes [HttpPost("UpdateCalendarCheckIn")], [Consumes], [ProducesResponseType], and [Authorize]. No new imports are necessary because Microsoft.AspNetCore.Mvc is already in use.
Suggested changeset
1
Web/Resgrid.Web.Services/Controllers/v4/CalendarController.cs
| @@ -401,6 +401,7 @@ | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [ProducesResponseType(StatusCodes.Status401Unauthorized)] | ||
| [Authorize(Policy = ResgridResources.Schedule_Update)] | ||
| [ValidateAntiForgeryToken] | ||
| public async Task<ActionResult<SetCalendarCheckInResult>> UpdateCalendarCheckIn([FromBody] CalendarCheckInUpdateInput input, CancellationToken cancellationToken) | ||
| { | ||
| var result = new SetCalendarCheckInResult(); |
Copilot is powered by AI and may make mistakes. Always verify output.
| { | ||
| var result = new SetCalendarCheckInResult(); | ||
|
|
||
| if (input == null || string.IsNullOrWhiteSpace(input.CheckInId)) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Copilot Autofix
AI 1 day 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.
| { | ||
| var result = new SetCalendarCheckInResult(); | ||
|
|
||
| if (input == null || string.IsNullOrWhiteSpace(input.CheckInId)) |
Check failure
Code scanning / CodeQL
User-controlled bypass of sensitive method High
Copilot Autofix
AI 1 day 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.
| [HttpPost] | ||
| [Authorize(Policy = ResgridResources.Reports_View)] | ||
| [ResponseCache(NoStore = true, Location = ResponseCacheLocation.None)] | ||
| public async Task<IActionResult> EventAttendanceReportParams(EventAttendanceReportParams model) |
Check failure
Code scanning / CodeQL
Missing cross-site request forgery token validation High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 2 days ago
In general, to fix missing CSRF token validation in ASP.NET Core MVC, you add the [ValidateAntiForgeryToken] attribute (or rely on a global AutoValidateAntiforgeryTokenAttribute filter) to all state-changing or parameter-accepting POST actions and ensure the corresponding form includes an antiforgery token (e.g., via @Html.AntiForgeryToken() or the built-in tag helpers).
For this specific case, the best minimal fix that does not change existing behavior is to decorate the POST EventAttendanceReportParams(EventAttendanceReportParams model) action with [ValidateAntiForgeryToken]. That ensures that any POST to this action must include a valid antiforgery token, otherwise the request will be rejected. We do not need any new imports: in ASP.NET Core, ValidateAntiForgeryTokenAttribute is defined in Microsoft.AspNetCore.Mvc, which is already imported at the top of the file. No changes are required to the GET action or the report-generating action.
Concretely:
- In
Web/Resgrid.Web/Areas/User/Controllers/ReportsController.cs, locate the POST version ofEventAttendanceReportParams. - Insert a
[ValidateAntiForgeryToken]attribute between[HttpPost]and[Authorize(...)](or anywhere among the attributes) for that method. - No additional supporting methods, definitions, or dependencies are needed in this file.
Suggested changeset
1
Web/Resgrid.Web/Areas/User/Controllers/ReportsController.cs
| @@ -2067,6 +2067,7 @@ | ||
| } | ||
|
|
||
| [HttpPost] | ||
| [ValidateAntiForgeryToken] | ||
| [Authorize(Policy = ResgridResources.Reports_View)] | ||
| [ResponseCache(NoStore = true, Location = ResponseCacheLocation.None)] | ||
| public async Task<IActionResult> EventAttendanceReportParams(EventAttendanceReportParams model) |
Copilot is powered by AI and may make mistakes. Always verify output.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs (1)
236-252:⚠️ Potential issue | 🔴 CriticalRe-check item-level authorization in the POST edit flow.
Edit(int id)protects the read path withCanUserModifyCalendarEntryAsync, but this POST still trustsmodel.Item.CalendarItemIdand proceeds to update the event. A crafted POST can bypass the GET gate and now also trigger attendee enrollment/notifications on an event the caller should not control.🔐 Proposed fix
if (ModelState.IsValid) { + if (!await _authorizationService.CanUserModifyCalendarEntryAsync(UserId, model.Item.CalendarItemId)) + return Unauthorized(); + var department = await _departmentsService.GetDepartmentByIdAsync(DepartmentId); // Snapshot existing attendees before update so we can diff for notifications var existingItem = await _calendarService.GetCalendarItemByIdAsync(model.Item.CalendarItemId);🤖 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 236 - 252, The POST edit flow must re-check item-level authorization before trusting model.Item.CalendarItemId; call the same authorization used in the GET path (CanUserModifyCalendarEntryAsync) passing model.Item.CalendarItemId (and DepartmentId/UserId/context as appropriate) and abort (return Forbidden/throw) if it fails, before loading existingItem or calling _calendarService.UpdateCalendarItemAsync; ensure the check happens early in the Edit POST handler (the method containing the shown block) so a crafted POST cannot bypass the GET gate and trigger UpdateCalendarItemAsync or attendee notifications.
🧹 Nitpick comments (5)
Web/Resgrid.Web/Areas/User/Views/Calendar/SignupSheet.cshtml (1)
95-102: Simplify duplicated date rendering branch.Lines 95-102 render the same output in both branches; this can be collapsed to one expression for readability.
🤖 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/SignupSheet.cshtml` around lines 95 - 102, The if/else around Model.CalendarItem.IsAllDay duplicates the same rendering; remove the conditional and render the date once by calling Model.CalendarItem.Start.TimeConverter(Model.Department).ToString("dddd, MMMM d, yyyy") directly (replace the entire if/else block with that single expression) to simplify the view and preserve existing behavior.Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml (1)
376-380: Consider extracting the hardcoded 15-minute check-in window.The 15-minute check-in window before event start is hardcoded. Consider making this configurable (e.g., a department setting or calendar item property) to accommodate different organizational needs.
🤖 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/View.cshtml` around lines 376 - 380, The 15-minute hardcoded window in the check-in logic needs to be extracted to a configurable value: replace the literal 15 used in computing checkInWindowOpen (DateTime.UtcNow >= Model.CalendarItem.Start.AddMinutes(-15)) with a property or setting (for example Model.CalendarItem.CheckInWindowMinutes or Model.Department.CheckInWindowMinutes) and fall back to a sensible default (15) if the property is null/0; update any UI/code that relies on this value (references: checkInWindowOpen, Model.CalendarItem.Start, showSelfPanel, adminBypassWindow) so the window is configurable per department or calendar item.Core/Resgrid.Model/Repositories/ICalendarItemCheckInRepository.cs (1)
7-13: Consider adding CancellationToken parameters for cancellation support.The async methods don't accept
CancellationTokenparameters. While this may align with existing repository patterns in the codebase, adding cancellation support improves responsiveness for long-running queries and aligns with modern async best practices.♻️ Optional enhancement
public interface ICalendarItemCheckInRepository : IRepository<CalendarItemCheckIn> { - Task<CalendarItemCheckIn> GetCheckInByCalendarItemAndUserAsync(int calendarItemId, string userId); - Task<IEnumerable<CalendarItemCheckIn>> GetCheckInsByCalendarItemIdAsync(int calendarItemId); - Task<IEnumerable<CalendarItemCheckIn>> GetCheckInsByDepartmentAndDateRangeAsync(int departmentId, DateTime start, DateTime end); - Task<IEnumerable<CalendarItemCheckIn>> GetCheckInsByUserAndDateRangeAsync(string userId, int departmentId, DateTime start, DateTime end); + Task<CalendarItemCheckIn> GetCheckInByCalendarItemAndUserAsync(int calendarItemId, string userId, CancellationToken cancellationToken = default); + Task<IEnumerable<CalendarItemCheckIn>> GetCheckInsByCalendarItemIdAsync(int calendarItemId, CancellationToken cancellationToken = default); + Task<IEnumerable<CalendarItemCheckIn>> GetCheckInsByDepartmentAndDateRangeAsync(int departmentId, DateTime start, DateTime end, CancellationToken cancellationToken = default); + Task<IEnumerable<CalendarItemCheckIn>> GetCheckInsByUserAndDateRangeAsync(string userId, int departmentId, DateTime start, DateTime end, CancellationToken cancellationToken = default); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Repositories/ICalendarItemCheckInRepository.cs` around lines 7 - 13, The interface async methods lack CancellationToken support; update ICalendarItemCheckInRepository by adding a CancellationToken parameter (e.g., CancellationToken cancellationToken = default) to GetCheckInByCalendarItemAndUserAsync, GetCheckInsByCalendarItemIdAsync, GetCheckInsByDepartmentAndDateRangeAsync, and GetCheckInsByUserAndDateRangeAsync, then adjust all implementations of those methods and any callers to accept/pass the token (or use default) so long-running DB queries can be cancelled.Core/Resgrid.Model/Services/ICalendarService.cs (1)
78-134: Consider extracting the attendance API into its own service contract.
ICalendarServicenow mixes schedule CRUD, attendee notifications, external feed sync, and check-in persistence/reporting. Splitting the new attendance surface behind a dedicated interface would keep consumers narrower and help stop constructor growth in the implementation.As per coding guidelines, "Minimize constructor injection; keep the number of injected dependencies small" and "Prefer composition with interfaces to extend behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Services/ICalendarService.cs` around lines 78 - 134, The ICalendarService interface mixes core calendar responsibilities with attendance/check-in APIs (methods like CheckInToEventAsync, CheckOutFromEventAsync, UpdateCheckInTimesAsync, GetCheckInByCalendarItemAndUserAsync, GetCheckInByIdAsync, GetCheckInsByCalendarItemAsync, DeleteCheckInAsync, GetCheckInsByDepartmentDateRangeAsync, GetCheckInsByUserDateRangeAsync); extract these attendance-related methods into a new ICalendarAttendanceService (or IAttendanceService) and move their implementations into a corresponding class, have the existing CalendarService implement only scheduling/feed APIs (ActivateCalendarSyncAsync, RegenerateCalendarSyncAsync, etc.), update DI registrations to bind the new service, and refactor any consumers/constructors to depend on the narrower interface to reduce constructor injection size and keep responsibilities separated.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (1)
4065-4068: Keep the calendar identifier name consistent.The rest of v4 uses
CalendarItemId/GetCalendarItem, but these new request DTOs rename the same key toCalendarEventId. That inconsistency leaks into generated clients for the same entity. PreferCalendarItemIdhere unless this is intentionally a different object.Also applies to: 4100-4103, 4210-4213
🤖 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 4065 - 4068, The DTOs introduce a property named CalendarEventId (e.g., Resgrid.Web.Services.Models.v4.Calendar.CalendarCheckInInput.CalendarEventId) that should be renamed to CalendarItemId to match the rest of v4 (CalendarItemId/GetCalendarItem); change the property name and its XML doc summary to CalendarItemId, update any JSON/serialization attributes, constructors, and references/usages (including the other DTOs flagged) so clients and generated code remain consistent, and run/build to ensure no remaining references to CalendarEventId remain.
🤖 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/AuthorizationService.cs`:
- Around line 1567-1588: The admin/creator branches in
CanUserAdminCheckInCalendarEventAsync return true without confirming the
targetUserId belongs to the same department/event, allowing arbitrary IDs to be
checked in; before returning true from the department.IsUserAnAdmin(userId) or
creator (item.CreatorUserId == userId) branches, resolve the target user’s
department (e.g., call
_departmentsService.GetDepartmentByUserIdAsync(targetUserId)) and ensure its
DepartmentId matches item.DepartmentId (or alternatively verify targetUserId is
an attendee of the calendar item via item.Attendees), and only then allow the
admin/creator check-in; update CanUserAdminCheckInCalendarEventAsync accordingly
so CalendarService.CheckInToEventAsync cannot persist foreign user IDs.
In `@Core/Resgrid.Services/CalendarService.cs`:
- Around line 784-799: The CheckOutFromEventAsync should be made idempotent by
not modifying an existing check-in once it already has a CheckOutTime; update
the method (CheckOutFromEventAsync) to inspect existing.CheckOutTime (or null)
and immediately return the existing record without calling
_calendarItemCheckInRepository.SaveOrUpdateAsync if a checkout has already
occurred, otherwise proceed to set
CheckOutTime/CheckOutByUserId/CheckOutNote/CheckOutLatitude/CheckOutLongitude
and save as before.
- Around line 801-816: Add validation in UpdateCheckInTimesAsync to reject
manual ranges where the provided checkOutTime is earlier than checkInTime: after
loading the existing CalendarItemCheckIn (method UpdateCheckInTimesAsync) and
before mutating/saving its fields (CheckInTime, CheckOutTime), check if
checkOutTime.HasValue && checkOutTime.Value < checkInTime and if so throw an
ArgumentException (or a ValidationException) with a clear message about invalid
check-in/check-out range so the API layer can map it to a 400 Bad Request.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0057_AddingCalendarItemCheckIns.cs`:
- Around line 36-47: The migration currently adds indexes for CalendarItemId and
DepartmentId+UserId but the read patterns require composite indexes including
CheckInTime; update the migration to add composite indexes matching the query
shapes: add an index named like
"IX_CalendarItemCheckIns_CalendarItemId_CheckInTime" on table
"CalendarItemCheckIns" for columns CalendarItemId (Ascending) and CheckInTime
(Descending or as used by queries), add
"IX_CalendarItemCheckIns_DepartmentId_CheckInTime" on DepartmentId and
CheckInTime, and add "IX_CalendarItemCheckIns_DepartmentId_UserId_CheckInTime"
on DepartmentId, UserId, CheckInTime; keep the existing
Create.UniqueConstraint("UQ_CalendarItemCheckIns_CalItem_User") intact so
uniqueness on (CalendarItemId, UserId) remains.
In
`@Repositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByItemIdQuery.cs`:
- Around line 28-31: The generic GetQuery<TEntity>() in
SelectCalendarItemCheckInsByItemIdQuery currently throws
NotImplementedException; implement it to simply delegate to the existing
non-generic GetQuery() so callers like
GetQuery<SelectCalendarItemCheckInsByItemIdQuery>() succeed—replace the
NotImplementedException with a return of GetQuery(); ensure the method signature
public string GetQuery<TEntity>() where TEntity : class, IEntity remains
unchanged.
In `@Web/Resgrid.Web.Services/Controllers/v4/CalendarController.cs`:
- Around line 461-505: The code returns attendee coordinates
(CheckInLatitude/CheckInLongitude/CheckOutLatitude/CheckOutLongitude) without
checking per-user location permission; update the loop in CalendarController
(where _authorizationService.CanUserViewCalendarCheckInsAsync is used and
checkIns are mapped to CalendarCheckInResultData) to call
_authorizationService.CanUserViewPersonLocationAsync(UserId, checkIn.UserId) for
each checkIn and only populate the latitude/longitude fields when that call
returns true, otherwise set those fields to null (or omit them) so location data
is not returned to users lacking location permission.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 331-390: The public timer DTOs referenced by the controller (e.g.,
Resgrid.Web.Services.Models.v4.CheckInTimers.CheckInTimerConfigInput,
CheckInTimerOverrideInput, PerformCheckInInput and any other classes under
Resgrid.Web.Services.Models.v4.CheckInTimers) lack XML comments so they are
missing from the generated API XML; add XML summary comments to each DTO class
and to every public property (and any enums) in those classes in source so the
compiler emits corresponding T:/ and P:/ entries in Resgrid.Web.Services.xml,
ensuring field-level schema documentation is included for consumers.
- Around line 84-108: The XML docs for the new calendar attendance endpoints
only contain summaries; update the controller source to add full action-level
XML comments for SetCalendarCheckIn(CalendarCheckInInput, CancellationToken),
SetCalendarCheckOut(CalendarCheckOutInput, CancellationToken),
UpdateCalendarCheckIn(CalendarCheckInUpdateInput, CancellationToken),
GetCalendarItemCheckIns(int), and DeleteCalendarCheckIn(string,
CancellationToken) by adding <param> tags that describe the request body/payload
and the route identifier (e.g., the meaning of the string id in
DeleteCalendarCheckIn) and a <returns> tag that documents the response shape
(status codes and returned model), so the generated v4 docs are self-describing.
Ensure the comments reference the exact parameter names used in each method to
keep the generated Resgrid.Web.Services.xml accurate.
In `@Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs`:
- Around line 254-264: The Edit path is expanding model.entities into attendee
rows and notifying everyone regardless of signup type; restrict this behavior to
only when model.Item.SignupType == CalendarItemSignupTypes.None by checking
model.Item.SignupType before calling AddEntitiesAsAttendeesAsync and
NotifyUsersAboutCalendarItemAsync (i.e., move the entities
expansion/notification block behind a condition that ensures SignupType ==
CalendarItemSignupTypes.None); if RSVP edits still require alerts, route them
via the generic notification path instead of creating required attendee rows
(refer to AddEntitiesAsAttendeesAsync, NotifyUsersAboutCalendarItemAsync,
model.entities, and CalendarItemSignupTypes.None).
- Around line 1087-1100: The branch handling "G:" group entities in
CalendarController accepts any group returned by GetGroupByIdAsync; add a guard
to reject groups from other departments by verifying group != null and
group.DepartmentId == calendarItem.DepartmentId before iterating group.Members
(skip or return an error if the department IDs differ) so only same-department
users are added via _calendarService.SignupForEvent.
In `@Web/Resgrid.Web/Areas/User/Controllers/ReportsController.cs`:
- Around line 2197-2232: The loop is causing N+1 calls by invoking
_calendarService.GetCalendarItemByIdAsync for each checkIn; fix by collecting
all CalendarItemId values from checkIns, call a batch fetch (either an existing
GetCalendarItemsByIdsAsync or add that method to ICalendarService) to retrieve
all calendar items once, build a Dictionary<int, CalendarItem> keyed by
CalendarItemId, then inside the foreach replace the per-item call with a lookup
in that dictionary when populating EventAttendanceDetail (used with
model.Details, checkIns, and the existing properties like
CheckInByUserId/CheckOutByUserId).
In `@Web/Resgrid.Web/Areas/User/Models/Calendar/SignupSheetView.cs`:
- Line 10: The PersonnelNames property on the SignupSheetView model can be null
and cause view iteration errors; initialize it to an empty list to prevent
null-path rendering (either set PersonnelNames = new List<PersonName>() as the
property default or populate it in the SignupSheetView constructor) so views
iterating over PersonnelNames never encounter a null reference.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/Edit.cshtml`:
- Around line 411-421: The localizer values are being injected directly into JS
with Html.Raw which can break strings and open XSS; define JSON-encoded
constants at the top of the script (e.g., const L_ENTITIES_TO_NOTIFY =
`@Json.Serialize`(localizer["EntitiesToNotify"]), L_ENTITIES_TO_NOTIFY_HELP =
`@Json.Serialize`(localizer["EntitiesToNotifyHelp"]), L_ATTENDEES =
`@Json.Serialize`(localizer["Attendees"]), L_ATTENDEES_ADDED_DIRECTLY =
`@Json.Serialize`(localizer["AttendeesAddedDirectly"]) ), then replace all inline
Html.Raw(localizer[…]) uses in the switch/if blocks that reference signupType
and the DOM elements $('#entitiesLabel'), $('#entitiesHelp'),
$('#entitiesGroup') with these constants (e.g., set text to L_ENTITIES_TO_NOTIFY
or L_ATTENDEES, set help to L_ENTITIES_TO_NOTIFY_HELP or
L_ATTENDEES_ADDED_DIRECTLY, or empty string) so strings are properly escaped and
consistent with other files.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/EditCheckIn.cshtml`:
- Around line 37-46: The view currently renders CheckIn/CheckOut inputs using a
hardcoded "MM/dd/yyyy HH:mm" while the client datetimepicker uses its own
default format, causing culture-dependent parsing errors; update the inputs
(elements with id/name CheckInTime, CheckOutTime and the
Model.CheckIn.CheckInTime/CheckOutTime value rendering) to emit a consistent,
culture-safe format (prefer ISO 8601 like "yyyy-MM-ddTHH:mm" or the culture's
DateTimeFormat) and update the datetimepicker initialization to explicitly use
the same format string so the value posted matches what the ASP.NET model binder
expects; ensure both server-side ToString formatting and client-side picker
format are identical to prevent mis-parsing.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/New.cshtml`:
- Around line 363-373: The localized strings are being injected raw into
JavaScript string context (see usages that set $('#entitiesLabel'),
$('#entitiesHelp') based on signupType and show $('#entitiesGroup')), which can
break JS if translations contain quotes or special chars; fix by
serializing/JS-encoding the localized values once on the server and use that
safe object in the script (for example create a server-side object of the needed
keys and emit it with a JSON encoder/HttpUtility.JavaScriptStringEncode via
`@Html.Raw`(Json.Encode(...)) or equivalent), then replace direct
'@Html.Raw(localizer["..."])' usages with references into that encoded object;
apply the identical change to both the Calendar New and Edit views so
$('#entitiesLabel'), $('#entitiesHelp'), and related code read from the encoded
JS object instead of raw strings.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/SignupSheet.cshtml`:
- Around line 141-147: The attendee name lookup uses a case-sensitive equality
(Model.PersonnelNames?.FirstOrDefault(x => x.UserId == attendee.UserId) ),
causing misses because PersonName.UserId is always uppercase; change the
comparison to a case-insensitive match such as String.Equals(x.UserId,
attendee.UserId, StringComparison.OrdinalIgnoreCase) or compare x.UserId to
attendee.UserId?.ToUpperInvariant() so names resolve correctly; apply the same
fix for the identical pattern found in Calendar/View.cshtml and other views
using Model.PersonnelNames/PersonName.UserId.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml`:
- Around line 489-492: The view currently displays the wrong localization key
when CheckOutTime is null: change the label rendered in the else branch that
uses localizer["NotCheckedIn"] to a correct key such as
localizer["NotCheckedOut"] (or an existing key like "Pending"/"InProgress") so
the column correctly reflects a missing checkout; update the View.cshtml else
branch where CheckOutTime is evaluated and ensure the chosen localization key is
added to resource files if it doesn't already exist.
In `@Web/Resgrid.Web/Areas/User/Views/Reports/EventAttendanceReport.cshtml`:
- Line 13: The top-level HTML tag in EventAttendanceReport.cshtml is hard-coded
with lang="en"; change it to bind to the current UI culture instead (use the
Razor/C# culture API such as System.Globalization.CultureInfo.CurrentUICulture
or CultureInfo.CurrentUICulture.TwoLetterISOLanguageName) so the lang attribute
reflects the user's UI culture rather than always being English.
---
Outside diff comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs`:
- Around line 236-252: The POST edit flow must re-check item-level authorization
before trusting model.Item.CalendarItemId; call the same authorization used in
the GET path (CanUserModifyCalendarEntryAsync) passing model.Item.CalendarItemId
(and DepartmentId/UserId/context as appropriate) and abort (return
Forbidden/throw) if it fails, before loading existingItem or calling
_calendarService.UpdateCalendarItemAsync; ensure the check happens early in the
Edit POST handler (the method containing the shown block) so a crafted POST
cannot bypass the GET gate and trigger UpdateCalendarItemAsync or attendee
notifications.
---
Nitpick comments:
In `@Core/Resgrid.Model/Repositories/ICalendarItemCheckInRepository.cs`:
- Around line 7-13: The interface async methods lack CancellationToken support;
update ICalendarItemCheckInRepository by adding a CancellationToken parameter
(e.g., CancellationToken cancellationToken = default) to
GetCheckInByCalendarItemAndUserAsync, GetCheckInsByCalendarItemIdAsync,
GetCheckInsByDepartmentAndDateRangeAsync, and
GetCheckInsByUserAndDateRangeAsync, then adjust all implementations of those
methods and any callers to accept/pass the token (or use default) so
long-running DB queries can be cancelled.
In `@Core/Resgrid.Model/Services/ICalendarService.cs`:
- Around line 78-134: The ICalendarService interface mixes core calendar
responsibilities with attendance/check-in APIs (methods like
CheckInToEventAsync, CheckOutFromEventAsync, UpdateCheckInTimesAsync,
GetCheckInByCalendarItemAndUserAsync, GetCheckInByIdAsync,
GetCheckInsByCalendarItemAsync, DeleteCheckInAsync,
GetCheckInsByDepartmentDateRangeAsync, GetCheckInsByUserDateRangeAsync); extract
these attendance-related methods into a new ICalendarAttendanceService (or
IAttendanceService) and move their implementations into a corresponding class,
have the existing CalendarService implement only scheduling/feed APIs
(ActivateCalendarSyncAsync, RegenerateCalendarSyncAsync, etc.), update DI
registrations to bind the new service, and refactor any consumers/constructors
to depend on the narrower interface to reduce constructor injection size and
keep responsibilities separated.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 4065-4068: The DTOs introduce a property named CalendarEventId
(e.g.,
Resgrid.Web.Services.Models.v4.Calendar.CalendarCheckInInput.CalendarEventId)
that should be renamed to CalendarItemId to match the rest of v4
(CalendarItemId/GetCalendarItem); change the property name and its XML doc
summary to CalendarItemId, update any JSON/serialization attributes,
constructors, and references/usages (including the other DTOs flagged) so
clients and generated code remain consistent, and run/build to ensure no
remaining references to CalendarEventId remain.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/SignupSheet.cshtml`:
- Around line 95-102: The if/else around Model.CalendarItem.IsAllDay duplicates
the same rendering; remove the conditional and render the date once by calling
Model.CalendarItem.Start.TimeConverter(Model.Department).ToString("dddd, MMMM d,
yyyy") directly (replace the entire if/else block with that single expression)
to simplify the view and preserve existing behavior.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml`:
- Around line 376-380: The 15-minute hardcoded window in the check-in logic
needs to be extracted to a configurable value: replace the literal 15 used in
computing checkInWindowOpen (DateTime.UtcNow >=
Model.CalendarItem.Start.AddMinutes(-15)) with a property or setting (for
example Model.CalendarItem.CheckInWindowMinutes or
Model.Department.CheckInWindowMinutes) and fall back to a sensible default (15)
if the property is null/0; update any UI/code that relies on this value
(references: checkInWindowOpen, Model.CalendarItem.Start, showSelfPanel,
adminBypassWindow) so the window is configurable per department or calendar
item.
🪄 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: 71bce143-6fd3-4c4d-96f5-d5a0f32e884c
⛔ Files ignored due to path filters (18)
Core/Resgrid.Localization/Areas/User/Calendar/Calendar.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.uk.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Reports/Reports.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Reports/Reports.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Reports/Reports.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Reports/Reports.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Reports/Reports.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Reports/Reports.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Reports/Reports.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Reports/Reports.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Reports/Reports.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (53)
Core/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Model/CalendarItem.csCore/Resgrid.Model/CalendarItemCheckIn.csCore/Resgrid.Model/CalendarItemCheckInTypes.csCore/Resgrid.Model/Repositories/ICalendarItemCheckInRepository.csCore/Resgrid.Model/Services/IAuthorizationService.csCore/Resgrid.Model/Services/ICalendarService.csCore/Resgrid.Services/AuditService.csCore/Resgrid.Services/AuthorizationService.csCore/Resgrid.Services/CalendarService.csProviders/Resgrid.Providers.Migrations/Migrations/M0057_AddingCalendarItemCheckIns.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0057_AddingCalendarItemCheckInsPg.csRepositories/Resgrid.Repositories.DataRepository/CalendarItemCheckInRepository.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/Calendar/SelectCalendarItemCheckInByItemAndUserQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByDeptDateRangeQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByItemIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByUserDateRangeQuery.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csTests/Resgrid.Tests/Services/CalendarServiceCheckInTests.csTests/Resgrid.Tests/Services/CalendarServiceTests.csWeb/Resgrid.Web.Services/Controllers/v4/CalendarController.csWeb/Resgrid.Web.Services/Models/v4/Calendar/CalendarCheckInInput.csWeb/Resgrid.Web.Services/Models/v4/Calendar/CalendarCheckInResultData.csWeb/Resgrid.Web.Services/Models/v4/Calendar/CalendarCheckInUpdateInput.csWeb/Resgrid.Web.Services/Models/v4/Calendar/CalendarCheckOutInput.csWeb/Resgrid.Web.Services/Models/v4/Calendar/GetAllCalendarItemResult.csWeb/Resgrid.Web.Services/Models/v4/Calendar/GetCalendarCheckInsResult.csWeb/Resgrid.Web.Services/Models/v4/Calendar/SetCalendarCheckInResult.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Controllers/CalendarController.csWeb/Resgrid.Web/Areas/User/Controllers/ReportsController.csWeb/Resgrid.Web/Areas/User/Models/Calendar/CalendarCheckInView.csWeb/Resgrid.Web/Areas/User/Models/Calendar/CalendarItemView.csWeb/Resgrid.Web/Areas/User/Models/Calendar/EditCalendarCheckInView.csWeb/Resgrid.Web/Areas/User/Models/Calendar/SignupSheetView.csWeb/Resgrid.Web/Areas/User/Models/Reports/EventAttendance/EventAttendanceDetailView.csWeb/Resgrid.Web/Areas/User/Models/Reports/EventAttendance/EventAttendanceView.csWeb/Resgrid.Web/Areas/User/Models/Reports/Params/EventAttendanceReportParams.csWeb/Resgrid.Web/Areas/User/Views/Calendar/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Calendar/EditCheckIn.cshtmlWeb/Resgrid.Web/Areas/User/Views/Calendar/New.cshtmlWeb/Resgrid.Web/Areas/User/Views/Calendar/SignupSheet.cshtmlWeb/Resgrid.Web/Areas/User/Views/Calendar/View.cshtmlWeb/Resgrid.Web/Areas/User/Views/Reports/EventAttendanceDetailReport.cshtmlWeb/Resgrid.Web/Areas/User/Views/Reports/EventAttendanceReport.cshtmlWeb/Resgrid.Web/Areas/User/Views/Reports/EventAttendanceReportParams.cshtmlWeb/Resgrid.Web/Areas/User/Views/Reports/Index.cshtml
✅ Files skipped from review due to trivial changes (10)
- Core/Resgrid.Model/CalendarItemCheckInTypes.cs
- Web/Resgrid.Web.Services/Models/v4/Calendar/GetCalendarCheckInsResult.cs
- Web/Resgrid.Web.Services/Models/v4/Calendar/CalendarCheckOutInput.cs
- Web/Resgrid.Web/Areas/User/Models/Reports/Params/EventAttendanceReportParams.cs
- Web/Resgrid.Web.Services/Models/v4/Calendar/CalendarCheckInUpdateInput.cs
- Web/Resgrid.Web.Services/Models/v4/Calendar/CalendarCheckInInput.cs
- Web/Resgrid.Web/Areas/User/Models/Calendar/CalendarCheckInView.cs
- Web/Resgrid.Web.Services/Models/v4/Calendar/CalendarCheckInResultData.cs
- Web/Resgrid.Web/Areas/User/Models/Reports/EventAttendance/EventAttendanceView.cs
- Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- Repositories/Resgrid.Repositories.DataRepository/Modules/DataModule.cs
- Repositories/Resgrid.Repositories.DataRepository/Modules/TestingDataModule.cs
- Repositories/Resgrid.Repositories.DataRepository/Modules/NonWebDataModule.cs
- Core/Resgrid.Model/AuditLogTypes.cs
Comment on lines
+36
to
+47
| Create.Index("IX_CalendarItemCheckIns_CalendarItemId") | ||
| .OnTable("CalendarItemCheckIns") | ||
| .OnColumn("CalendarItemId"); | ||
|
|
||
| Create.Index("IX_CalendarItemCheckIns_DepartmentId_UserId") | ||
| .OnTable("CalendarItemCheckIns") | ||
| .OnColumn("DepartmentId").Ascending() | ||
| .OnColumn("UserId").Ascending(); | ||
|
|
||
| Create.UniqueConstraint("UQ_CalendarItemCheckIns_CalItem_User") | ||
| .OnTable("CalendarItemCheckIns") | ||
| .Columns("CalendarItemId", "UserId"); |
Contributor
There was a problem hiding this comment.
Add indexes that match the new attendance read paths.
The new calendar check-in queries read CalendarItemCheckIns by CalendarItemId + CheckInTime, DepartmentId + CheckInTime, and DepartmentId + UserId + CheckInTime, but this migration only adds CalendarItemId and DepartmentId + UserId. Once departments accumulate real attendance history, the item view and date-range reports will end up scanning/sorting this table.
[suggested adjustment below keeps the uniqueness rule, but keys the secondary indexes to the actual query shapes.]
Possible migration adjustment
- Create.Index("IX_CalendarItemCheckIns_CalendarItemId")
- .OnTable("CalendarItemCheckIns")
- .OnColumn("CalendarItemId");
+ Create.Index("IX_CalendarItemCheckIns_CalendarItemId_CheckInTime")
+ .OnTable("CalendarItemCheckIns")
+ .OnColumn("CalendarItemId").Ascending()
+ .OnColumn("CheckInTime").Descending();
- Create.Index("IX_CalendarItemCheckIns_DepartmentId_UserId")
+ Create.Index("IX_CalendarItemCheckIns_DepartmentId_CheckInTime")
+ .OnTable("CalendarItemCheckIns")
+ .OnColumn("DepartmentId").Ascending()
+ .OnColumn("CheckInTime").Descending();
+
+ Create.Index("IX_CalendarItemCheckIns_DepartmentId_UserId_CheckInTime")
.OnTable("CalendarItemCheckIns")
.OnColumn("DepartmentId").Ascending()
- .OnColumn("UserId").Ascending();
+ .OnColumn("UserId").Ascending()
+ .OnColumn("CheckInTime").Descending();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0057_AddingCalendarItemCheckIns.cs`
around lines 36 - 47, The migration currently adds indexes for CalendarItemId
and DepartmentId+UserId but the read patterns require composite indexes
including CheckInTime; update the migration to add composite indexes matching
the query shapes: add an index named like
"IX_CalendarItemCheckIns_CalendarItemId_CheckInTime" on table
"CalendarItemCheckIns" for columns CalendarItemId (Ascending) and CheckInTime
(Descending or as used by queries), add
"IX_CalendarItemCheckIns_DepartmentId_CheckInTime" on DepartmentId and
CheckInTime, and add "IX_CalendarItemCheckIns_DepartmentId_UserId_CheckInTime"
on DepartmentId, UserId, CheckInTime; keep the existing
Create.UniqueConstraint("UQ_CalendarItemCheckIns_CalItem_User") intact so
uniqueness on (CalendarItemId, UserId) remains.
...grid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByItemIdQuery.cs
Show resolved
Hide resolved
Web/Resgrid.Web/Areas/User/Views/Reports/EventAttendanceReport.cshtml
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (3)
Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml (1)
168-168:⚠️ Potential issue | 🟠 MajorJS-encode the localized confirm text before embedding it in
onclick.This is still rendering localized text straight into a single-quoted JavaScript literal. A quote or line break in either localized value will break the handler.
Suggested fix
-<button type="submit" class="btn btn-xs btn-danger" onclick="return confirm('@localizer["CheckInTimerDeleteConfirm"]');">@commonLocalizer["Delete"]</button> +<button type="submit" class="btn btn-xs btn-danger" onclick="return confirm('@Html.Raw(System.Text.Encodings.Web.JavaScriptEncoder.Default.Encode(localizer["CheckInTimerDeleteConfirm"].Value))');">@commonLocalizer["Delete"]</button> -<button type="submit" class="btn btn-xs btn-danger" onclick="return confirm('@localizer["CheckInTimerOverrideDeleteConfirm"]');">@commonLocalizer["Delete"]</button> +<button type="submit" class="btn btn-xs btn-danger" onclick="return confirm('@Html.Raw(System.Text.Encodings.Web.JavaScriptEncoder.Default.Encode(localizer["CheckInTimerOverrideDeleteConfirm"].Value))');">@commonLocalizer["Delete"]</button>Also applies to: 294-294
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml` at line 168, The inline localized string used in the onclick confirm handler (localizer["CheckInTimerDeleteConfirm"]) is injected raw into a single-quoted JS literal and can break if it contains quotes/newlines; update the Razor to JS-encode that localized value before embedding (for example use HttpUtility.JavaScriptStringEncode(localizer["CheckInTimerDeleteConfirm"]) or Json.Encode/JavaScriptEncoder) so the confirm('...') argument is safe, and apply the same change to the other occurrence of localizer["CheckInTimerDeleteConfirm"] noted in the file (line ~294).Core/Resgrid.Services/CheckInTimerService.cs (2)
154-189:⚠️ Potential issue | 🔴 CriticalDisabled overrides still don't suppress an inherited default.
The
continueon Lines 169-171 leaves the default timer inresolved, so a department-wide timer still survives even when the most specific matching override is disabled. Because lower-scored overrides are processed afterward, the key also needs to be marked handled once the highest-priority match wins.🧩 One way to make the highest-scoring override authoritative
var scoredOverrides = overrideList .Select(o => new { Override = o, Score = (o.CallTypeId.HasValue && o.CallPriority.HasValue) ? 3 : o.CallTypeId.HasValue ? 2 : o.CallPriority.HasValue ? 1 : 0 }) .OrderByDescending(x => x.Score) .ToList(); +var handledKeys = new HashSet<string>(); + foreach (var scored in scoredOverrides) { var o = scored.Override; - if (!o.IsEnabled) - continue; - var targetId = o.UnitTypeId?.ToString(); var key = $"{o.TimerTargetType}_{o.UnitTypeId}_{targetId}"; - // Only apply if this is the best scoring override for this key - if (!resolved.ContainsKey(key) || !resolved[key].IsFromOverride) + if (!handledKeys.Add(key)) + continue; + + if (!o.IsEnabled) { - resolved[key] = new ResolvedCheckInTimer - { - TargetType = o.TimerTargetType, - UnitTypeId = o.UnitTypeId, - TargetEntityId = targetId, - DurationMinutes = o.DurationMinutes, - WarningThresholdMinutes = o.WarningThresholdMinutes, - IsFromOverride = true, - ActiveForStates = o.ActiveForStates - }; + resolved.Remove(key); + continue; } + + resolved[key] = new ResolvedCheckInTimer + { + TargetType = o.TimerTargetType, + UnitTypeId = o.UnitTypeId, + TargetEntityId = targetId, + DurationMinutes = o.DurationMinutes, + WarningThresholdMinutes = o.WarningThresholdMinutes, + IsFromOverride = true, + ActiveForStates = o.ActiveForStates + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/CheckInTimerService.cs` around lines 154 - 189, The loop over scoredOverrides currently skips disabled overrides but leaves the default in resolved, so a higher-priority disabled override doesn't prevent an inherited timer; fix by treating the highest-scoring override as authoritative for its key whether enabled or not: in the foreach over scoredOverrides (working with overrideList/scoredOverrides, resolved, ResolvedCheckInTimer and key), if the key is not yet in resolved and the override is disabled (o.IsEnabled == false) insert a handled entry into resolved for that key (e.g., a ResolvedCheckInTimer marked IsFromOverride = true or another sentinel) to prevent lower-scored or default timers from applying; otherwise keep the existing behavior for enabled overrides.
140-147:⚠️ Potential issue | 🔴 Critical
TargetEntityIdstill isn't a concrete timer target.Here it's just
UnitTypeId?.ToString(), and for personnel timers it staysnull, so resolution still has no concrete user/unit identity. That leaks into Lines 249-250 (CheckInRecord.UnitId == timer.UnitTypeId) and Lines 338-340, where any dispatched unit can activate a unit-type timer. One entity can still reset or activate unrelated timers.Also applies to: 173-183, 249-250, 333-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/CheckInTimerService.cs` around lines 140 - 147, The TargetEntityId field is being set to UnitTypeId?.ToString() which is not a concrete entity and leaves personnel timers null; update the resolution logic where resolved[key] is created (uses def.TimerTargetType and ResolvedCheckInTimer) to set TargetEntityId to the actual entity id string based on TimerTargetType (e.g., if TimerTargetType == TimerTargetType.Unit set TargetEntityId = def.UnitId?.ToString(); if TimerTargetType == TimerTargetType.Personnel set TargetEntityId = def.PersonnelId?.ToString(); otherwise use the existing concrete id), and then change comparisons elsewhere (the checks in the code that compare CheckInRecord.UnitId or PersonnelId to timer.UnitTypeId) to compare CheckInRecord.UnitId/PersonnelId to timer.TargetEntityId (converting types as needed) so timers are matched to the specific unit/person entity (references: ResolvedCheckInTimer.TargetEntityId, def.TimerTargetType, def.UnitId, def.PersonnelId, CheckInRecord.UnitId, CheckInRecord.PersonnelId).
🧹 Nitpick comments (5)
Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js (1)
156-166: Consider specifying radix forparseIntcalls.Lines 157 and 164 use
parseInt()without specifying the radix. While modern browsers default to base 10, explicitly specifyingparseInt(value, 10)is a best practice to avoid edge cases with leading zeros.✨ Suggested improvement
$(document).on('click', '#checkInSubmitBtn', function () { - var checkInType = parseInt($('#checkInTargetType').val()); + var checkInType = parseInt($('#checkInTargetType').val(), 10); var unitIdVal = $('#checkInUnitId').val(); var note = $('#checkInNote').val(); var input = { CallId: callId, CheckInType: checkInType, - UnitId: unitIdVal ? parseInt(unitIdVal) : null, + UnitId: unitIdVal ? parseInt(unitIdVal, 10) : null, Note: note || null };🤖 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/dispatch/resgrid.dispatch.checkin-timers.js` around lines 156 - 166, The click handler for '#checkInSubmitBtn' uses parseInt without a radix; update the parseInt calls for parsing '#checkInTargetType' (checkInType) and '#checkInUnitId' (UnitId parsing in the input object) to use parseInt(value, 10) so they explicitly parse base-10 integers and avoid edge cases with leading zeros.Web/Resgrid.Web/Areas/User/Views/Dispatch/CallExport.cshtml (1)
503-520: Includetimer.TargetNamein the exported config table.
TargetTypealone does not identify which resolved timer a row represents, so repeatedUnitType/Personnelentries become ambiguous. The view model already carriesResolvedCheckInTimer.TargetName, and rendering it here would make the export self-describing.🤖 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/CallExport.cshtml` around lines 503 - 520, The table lacks the resolved target name so rows with the same TargetType are ambiguous; update the CheckInTimerConfigSection table to include a new column header (e.g., "CheckInTimerTargetName") and render the resolved target name for each entry inside the foreach over Model.TimerConfigs (use the existing property on the timer object such as TargetName or ResolvedCheckInTimer.TargetName) alongside the other cells (TargetType, DurationMinutes, WarningThresholdMinutes, IsFromOverride) so each exported row is self-describing.Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs (1)
64-72: Move the check-in timer endpoints out ofDepartmentController.This controller already has a very large dependency surface, and adding another service makes this area even harder to test and reason about. A dedicated controller/handler for the timer CRUD + lookup flow would keep this slice much smaller.
As per coding guidelines, "Minimize constructor injection; keep the number of injected dependencies small" and "Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs` around lines 64 - 72, The DepartmentController currently receives ICheckInTimerService (constructor param ICheckInTimerService checkInTimerService) and hosts the check-in timer endpoints; extract those endpoints into a new CheckInTimerController (or similar) that owns the check-in timer CRUD and lookup actions, inject only ICheckInTimerService (and any minimal collaborators it needs) into that new controller, move the action methods from DepartmentController to the new controller, update routing/authorization attributes as needed, and remove ICheckInTimerService from DepartmentController's constructor and any references in that class to minimize its dependency surface.Tests/Resgrid.Tests/Services/CheckInTimerServiceTests.cs (1)
60-101: The suite still misses the two risky regressions in this service.Every override case here uses enabled overrides, and every status/filter case uses a single dispatch or unit. That means a disabled high-specificity override, or two concrete targets sharing the same timer type, can still slip through green. I'd add one regression for each before merging.
Also applies to: 341-442
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Services/CheckInTimerServiceTests.cs` around lines 60 - 101, Add two regression tests to cover (1) a matching override that is IsEnabled = false should be ignored so the service falls back to the config or another enabled override, and (2) when multiple enabled overrides target the same TimerTargetType (e.g. two concrete overrides with same TimerTargetType but different specificity) the service returns one resolved timer using the highest-specificity override (no duplicate timers). In CheckInTimerServiceTests add a test calling ResolveAllTimersForCallAsync with _configRepo.GetByDepartmentIdAsync and _overrideRepo.GetMatchingOverridesAsync set up to return (a) a disabled CheckInTimerOverride (IsEnabled = false) alongside a config and assert the config value is used, and (b) two enabled CheckInTimerOverride entries that both match the Call (same TimerTargetType) but different CallTypeId/CallPriority and assert only one timer is returned with the expected DurationMinutes from the most specific override; reference ResolveAllTimersForCallAsync, CheckInTimerOverride.IsEnabled, TimerTargetType, CallTypeId, CallPriority, _overrideRepo and _configRepo.Core/Resgrid.Model/ResolvedCheckInTimer.cs (1)
3-20: Convert to a record class with init accessors.
ResolvedCheckInTimeris a pure data carrier with no behavior, making it an ideal candidate forrecord classwithinitaccessors to enforce immutability. This aligns with the coding guidelines: "Prefer functional patterns and immutable data where appropriate in C#" and "Separate state from behavior (e.g., use records for state and place operations in separate static classes)".Example:
public record class ResolvedCheckInTimer { public int TargetType { get; init; } public int? UnitTypeId { get; init; } public string TargetEntityId { get; init; } public string TargetName { get; init; } public int DurationMinutes { get; init; } public int WarningThresholdMinutes { get; init; } public bool IsFromOverride { get; init; } public string ActiveForStates { get; init; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/ResolvedCheckInTimer.cs` around lines 3 - 20, Convert the plain mutable class ResolvedCheckInTimer into an immutable record class by changing its declaration to "record class ResolvedCheckInTimer" and replacing all property setters (e.g., TargetType, UnitTypeId, TargetEntityId, TargetName, DurationMinutes, WarningThresholdMinutes, IsFromOverride, ActiveForStates) from get; set; to get; init; so the type becomes an immutable data carrier while preserving property names and types.
🤖 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/AuthorizationService.cs`:
- Around line 1611-1618: The current admin membership check in
AuthorizationService (the block using
_departmentGroupsService.GetAllChildDepartmentGroupsAsync and the similar block
at 1654-1661) only inspects immediate children and misses deeper descendants;
update the logic to walk the full descendant tree by either calling a service
method that returns all descendants (e.g., implement/use
GetAllDescendantDepartmentGroupsAsync in DepartmentGroupsService) or perform a
recursive traversal of GetAllChildDepartmentGroupsAsync results, checking
IsUserInGroup(targetUserId) on every descendant group; ensure both the 1611
block and the 1654-1661 block use the new recursive/descendant-aware lookup so
users in grandchild (or deeper) groups are correctly recognized.
- Around line 1583-1599: The code currently calls
_departmentsService.GetDepartmentByUserIdAsync(targetUserId) inside the admin
and creator branches which can return null for valid department users; replace
those lookups with a membership check on the current department (e.g., use
department.IsUserInDepartment(targetUserId) or check
department.DepartmentMembers/IsUserMember) so the admin check
(department.IsUserAnAdmin(userId)) and creator check (item.CreatorUserId ==
userId) correctly allow check-ins for target users who are members even if
GetDepartmentByUserIdAsync would return null; update both branches to use the
department membership method and remove the GetDepartmentByUserIdAsync calls,
keeping behavior consistent with CanUserViewUserAsync.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0058_FixCheckInTimerNullableUniqueConstraints.cs`:
- Around line 17-29: The SQL Server migrations
(M0058_FixCheckInTimerNullableUniqueConstraints.cs) create filtered unique
indexes (UQ_CheckInTimerConfigs_Dept_Target_Unit and
UQ_CheckInTimerOverrides_Dept_Call_Target_Unit) that exclude NULLs, allowing
multiple NULL rows, while the PostgreSQL migration
(M0058_FixCheckInTimerNullableUniqueConstraintsPg.cs) enforces uniqueness for
NULLs using NULLS NOT DISTINCT; align behavior by changing the SQL Server side
to treat NULL as a distinct value for uniqueness: add a persisted computed
column for each nullable key (e.g., UnitTypeId_ForUnique =
ISNULL(CONVERT(varchar(50), UnitTypeId), '<NULL>') or COALESCE(UnitTypeId,
<safe-sentinel>)) and create the unique indexes on those computed columns
instead of filtering (update the index creation for both CheckInTimerConfigs and
CheckInTimerOverrides to reference the computed columns so SQL Server will allow
only one "NULL"/Any row matching the intent).
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0058_FixCheckInTimerNullableUniqueConstraintsPg.cs`:
- Around line 10-30: The migration creates new NULLS NOT DISTINCT unique indexes
but doesn't remove existing duplicate rows that would violate them; before
executing CREATE UNIQUE INDEX for uq_checkintimerconfigs_dept_target_unit on
checkintimerconfigs and uq_checkintimeroverrides_dept_call_target_unit on
checkintimeroverrides, add SQL to detect and deduplicate legacy rows (e.g.,
group by the indexed key columns and keep one row per group, deleting others) or
merge data as needed; implement this cleanup in
M0058_FixCheckInTimerNullableUniqueConstraintsPg.cs so the Execute.Sql(...) that
creates each unique index runs only after duplicates have been removed.
In `@Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs`:
- Around line 630-639: Model.IsAdmin is being set globally which causes the view
to render admin actions for all attendees; instead determine admin rights
per-attendee using the same group-scoped checks used by
CanUserAdminCheckInCalendarEventAsync and CanUserEditCalendarCheckInAsync.
Replace the blanket model.IsAdmin assignment with logic that calls
_departmentGroupsService.GetGroupForUserAsync(UserId, DepartmentId) and
group.IsUserGroupAdmin(UserId) to compute a per-attendee flag (e.g.,
Attendee.CanAdmin or a dictionary on the model keyed by attendee user id) and
populate that for each CalendarItem attendee so the view only shows
check-in/check-out/edit buttons for attendees within the admin’s group tree.
In `@Web/Resgrid.Web/Areas/User/Controllers/ReportsController.cs`:
- Around line 2086-2089: EventAttendanceDetailReport currently returns another
user's detailed attendance (exact notes and check-in/out coordinates) without
authorization; update EventAttendanceDetailReport to authorize the target user
before building the model by calling the same permission checks used elsewhere
(e.g., CanUserViewPersonViaMatrixAsync and CanUserViewPersonLocationAsync) and
only pass full coordinates/notes into BuildEventAttendanceDetailReportModel when
those checks succeed (otherwise redact or omit location data); ensure the
Reports_View permission gating remains but add per-target-user checks so callers
cannot view another member's sensitive location data via query string.
- Around line 2138-2170: The loop is adding every user to model.EventHours
without checking the personnel visibility matrix; call and honor
_authorizationService.CanUserViewPersonViaMatrixAsync(requestingUserId, userId)
(await it if the controller action is async, or adapt the action to be async)
inside the foreach over allUserIds and skip creating/adding personHours for any
user where that call returns false; keep the existing calculations (TotalEvents,
MissedEvents, TotalSeconds, Name, Group) but only perform and add personHours to
model.EventHours when the visibility check permits.
- Around line 2059-2061: The default Start/End use DateTime.UtcNow.Year which
ignores the department time zone; update the ReportsController initialization of
EventAttendanceReportParams (model.Start/model.End) to compute the current year
in the department's local time (e.g., get the department's TimeZoneInfo, convert
DateTime.UtcNow to that zone or obtain now in that zone, then use now.Year to
build the Jan 1 / Dec 31 23:59:59 bounds) so the default range aligns with the
department calendar year.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml`:
- Around line 376-390: The check-in form condition currently only checks the
lower bound (checkInWindowOpen) so non-admins see it indefinitely; update the
conditional that renders the check-in panel (the if using Model.UserCheckIn) to
require the event is not past for non-admins by combining the checks: require
Model.UserCheckIn == null and ((checkInWindowOpen && !eventIsPast) ||
adminBypassWindow). This uses the existing symbols
showSelfPanel/checkInWindowOpen/eventIsPast/adminBypassWindow/Model.UserCheckIn
to ensure only admins can check in after the event end.
In `@Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml`:
- Line 163: The table cell is rendering the raw comma-separated ID string
config.ActiveForStates instead of the human-readable state names; split
config.ActiveForStates on commas, trim the IDs, map each ID to its display label
using the view’s state lookup (e.g., Model.States or ViewBag.States — a
dictionary/collection of id→name), then join the labels with commas and render
that (fall back to "All" when ActiveForStates is null/empty). Apply the same
change for the other occurrence of config.ActiveForStates mentioned in the
comment.
- Around line 144-145: Replace the hard-coded English labels in
DispatchSettings.cshtml (e.g. "Active States", 'Leave empty for "All States".'
and the other English-only strings added around the same sections) with the
localized resource keys used elsewhere in the view; create or reuse resource
entries (e.g. ActiveStates, LeaveEmptyForAllStates) and render them via the
existing localization helper used in this project (match other lines in the file
— e.g. `@Resources.YourKey` or `@Html.Resource`("YourKey")) so all instances are
fully localized (also update the other occurrences of these literal strings in
the same view).
In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/NewCall.cshtml`:
- Around line 405-411: The NewCall GET path is creating a fresh Call without
setting Call.CheckInTimersEnabled so the checkbox defaults to false and can
override the department default; update DispatchController.NewCall to seed the
new Call's CheckInTimersEnabled by calling the same department helper used in v4
(e.g., invoke GetCheckInTimersAutoEnableForNewCallsAsync or the Departments
service method that returns the department default) and assign its result to
Call.CheckInTimersEnabled before returning the view so the checkbox reflects the
department auto-enable setting on initial render.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js`:
- Around line 16-27: The AJAX call in loadTimerStatuses lacks error handling;
add a .fail() (or .catch for promise) handler on the $.ajax call that logs the
error and surfaces UI feedback (e.g. set a status flag or show a message) so the
user knows the request failed; update loadTimerStatuses to handle failures for
the URL built with callId, ensure timerStatuses is not overwritten on error, and
optionally call a new or existing renderTimerStatusesError / renderTimerStatuses
fallback path to update the UI.
- Around line 4-14: The interval assigned to timerInterval (set in the
document.ready block after calling loadTimerStatuses and updateCountdowns) is
never cleared; add cleanup to clearInterval(timerInterval) when the page unloads
or when the call becomes inactive (e.g., on window.beforeunload/window.unload or
when callState changes from 0), and ensure timerInterval is checked for null
before clearing and set to null after clearing to avoid duplicate clears and
memory leaks.
- Around line 108-118: The AJAX call in loadCheckInHistory lacks error handling
and will fail silently; update loadCheckInHistory to attach a .fail (or .catch)
handler to the $.ajax promise (similar to loadTimerStatuses) that handles errors
by logging to console or calling an existing error logger and showing a
user-friendly message/UI state, and ensure any cleanup or fallback (e.g.,
clearing/loading spinners) is done before/after calling renderCheckInHistory to
keep behavior consistent on failure.
- Around line 60-71: The default branch in getStatusBadge directly injects the
status string into HTML, creating an XSS risk; update the default case in
function getStatusBadge to HTML-escape or encode the status value (or build the
element text-safe, e.g., create a span and set textContent) before inserting it
into the returned markup so arbitrary status values cannot inject HTML/JS;
ensure the sanitization/encoding routine is applied to any use of the status
variable in this function.
---
Duplicate comments:
In `@Core/Resgrid.Services/CheckInTimerService.cs`:
- Around line 154-189: The loop over scoredOverrides currently skips disabled
overrides but leaves the default in resolved, so a higher-priority disabled
override doesn't prevent an inherited timer; fix by treating the highest-scoring
override as authoritative for its key whether enabled or not: in the foreach
over scoredOverrides (working with overrideList/scoredOverrides, resolved,
ResolvedCheckInTimer and key), if the key is not yet in resolved and the
override is disabled (o.IsEnabled == false) insert a handled entry into resolved
for that key (e.g., a ResolvedCheckInTimer marked IsFromOverride = true or
another sentinel) to prevent lower-scored or default timers from applying;
otherwise keep the existing behavior for enabled overrides.
- Around line 140-147: The TargetEntityId field is being set to
UnitTypeId?.ToString() which is not a concrete entity and leaves personnel
timers null; update the resolution logic where resolved[key] is created (uses
def.TimerTargetType and ResolvedCheckInTimer) to set TargetEntityId to the
actual entity id string based on TimerTargetType (e.g., if TimerTargetType ==
TimerTargetType.Unit set TargetEntityId = def.UnitId?.ToString(); if
TimerTargetType == TimerTargetType.Personnel set TargetEntityId =
def.PersonnelId?.ToString(); otherwise use the existing concrete id), and then
change comparisons elsewhere (the checks in the code that compare
CheckInRecord.UnitId or PersonnelId to timer.UnitTypeId) to compare
CheckInRecord.UnitId/PersonnelId to timer.TargetEntityId (converting types as
needed) so timers are matched to the specific unit/person entity (references:
ResolvedCheckInTimer.TargetEntityId, def.TimerTargetType, def.UnitId,
def.PersonnelId, CheckInRecord.UnitId, CheckInRecord.PersonnelId).
In `@Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml`:
- Line 168: The inline localized string used in the onclick confirm handler
(localizer["CheckInTimerDeleteConfirm"]) is injected raw into a single-quoted JS
literal and can break if it contains quotes/newlines; update the Razor to
JS-encode that localized value before embedding (for example use
HttpUtility.JavaScriptStringEncode(localizer["CheckInTimerDeleteConfirm"]) or
Json.Encode/JavaScriptEncoder) so the confirm('...') argument is safe, and apply
the same change to the other occurrence of
localizer["CheckInTimerDeleteConfirm"] noted in the file (line ~294).
---
Nitpick comments:
In `@Core/Resgrid.Model/ResolvedCheckInTimer.cs`:
- Around line 3-20: Convert the plain mutable class ResolvedCheckInTimer into an
immutable record class by changing its declaration to "record class
ResolvedCheckInTimer" and replacing all property setters (e.g., TargetType,
UnitTypeId, TargetEntityId, TargetName, DurationMinutes,
WarningThresholdMinutes, IsFromOverride, ActiveForStates) from get; set; to get;
init; so the type becomes an immutable data carrier while preserving property
names and types.
In `@Tests/Resgrid.Tests/Services/CheckInTimerServiceTests.cs`:
- Around line 60-101: Add two regression tests to cover (1) a matching override
that is IsEnabled = false should be ignored so the service falls back to the
config or another enabled override, and (2) when multiple enabled overrides
target the same TimerTargetType (e.g. two concrete overrides with same
TimerTargetType but different specificity) the service returns one resolved
timer using the highest-specificity override (no duplicate timers). In
CheckInTimerServiceTests add a test calling ResolveAllTimersForCallAsync with
_configRepo.GetByDepartmentIdAsync and _overrideRepo.GetMatchingOverridesAsync
set up to return (a) a disabled CheckInTimerOverride (IsEnabled = false)
alongside a config and assert the config value is used, and (b) two enabled
CheckInTimerOverride entries that both match the Call (same TimerTargetType) but
different CallTypeId/CallPriority and assert only one timer is returned with the
expected DurationMinutes from the most specific override; reference
ResolveAllTimersForCallAsync, CheckInTimerOverride.IsEnabled, TimerTargetType,
CallTypeId, CallPriority, _overrideRepo and _configRepo.
In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs`:
- Around line 64-72: The DepartmentController currently receives
ICheckInTimerService (constructor param ICheckInTimerService
checkInTimerService) and hosts the check-in timer endpoints; extract those
endpoints into a new CheckInTimerController (or similar) that owns the check-in
timer CRUD and lookup actions, inject only ICheckInTimerService (and any minimal
collaborators it needs) into that new controller, move the action methods from
DepartmentController to the new controller, update routing/authorization
attributes as needed, and remove ICheckInTimerService from
DepartmentController's constructor and any references in that class to minimize
its dependency surface.
In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/CallExport.cshtml`:
- Around line 503-520: The table lacks the resolved target name so rows with the
same TargetType are ambiguous; update the CheckInTimerConfigSection table to
include a new column header (e.g., "CheckInTimerTargetName") and render the
resolved target name for each entry inside the foreach over Model.TimerConfigs
(use the existing property on the timer object such as TargetName or
ResolvedCheckInTimer.TargetName) alongside the other cells (TargetType,
DurationMinutes, WarningThresholdMinutes, IsFromOverride) so each exported row
is self-describing.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js`:
- Around line 156-166: The click handler for '#checkInSubmitBtn' uses parseInt
without a radix; update the parseInt calls for parsing '#checkInTargetType'
(checkInType) and '#checkInUnitId' (UnitId parsing in the input object) to use
parseInt(value, 10) so they explicitly parse base-10 integers and avoid edge
cases with leading zeros.
🪄 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: 17d6d5e5-e07f-4b8e-a9e8-6ce2ae0b1305
⛔ Files ignored due to path filters (9)
Core/Resgrid.Localization/Areas/User/Calendar/Calendar.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Calendar/Calendar.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (48)
Core/Resgrid.Model/CheckInTimerConfig.csCore/Resgrid.Model/CheckInTimerOverride.csCore/Resgrid.Model/ResolvedCheckInTimer.csCore/Resgrid.Model/Services/ICheckInTimerService.csCore/Resgrid.Services/AuthorizationService.csCore/Resgrid.Services/CalendarService.csCore/Resgrid.Services/CheckInTimerService.csCore/Resgrid.Services/CustomStateService.csCore/Resgrid.Services/ServicesModule.csProviders/Resgrid.Providers.Migrations/Migrations/M0058_FixCheckInTimerNullableUniqueConstraints.csProviders/Resgrid.Providers.Migrations/Migrations/M0059_AddActiveForStatesToCheckInTimers.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0058_FixCheckInTimerNullableUniqueConstraintsPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0059_AddActiveForStatesToCheckInTimersPg.csRepositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInByItemAndUserQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByDeptDateRangeQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByItemIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByUserDateRangeQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInRecordsByCallIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInRecordsByDepartmentIdAndDateRangeQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInTimerConfigByDepartmentAndTargetQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInTimerConfigsByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInTimerOverridesByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectLastCheckInForUnitOnCallQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectLastCheckInForUserOnCallQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectMatchingCheckInTimerOverridesQuery.csTests/Resgrid.Tests/Services/CheckInTimerServiceTests.csWeb/Resgrid.Web.Services/Controllers/v4/CalendarController.csWeb/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.csWeb/Resgrid.Web.Services/Hubs/EventingHub.csWeb/Resgrid.Web.Services/Models/v4/CheckInTimers/CheckInTimerModels.csWeb/Resgrid.Web/Areas/User/Controllers/CalendarController.csWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Controllers/ReportsController.csWeb/Resgrid.Web/Areas/User/Models/Calendar/SignupSheetView.csWeb/Resgrid.Web/Areas/User/Views/Calendar/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Calendar/EditCheckIn.cshtmlWeb/Resgrid.Web/Areas/User/Views/Calendar/New.cshtmlWeb/Resgrid.Web/Areas/User/Views/Calendar/SignupSheet.cshtmlWeb/Resgrid.Web/Areas/User/Views/Calendar/View.cshtmlWeb/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/CallExport.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/NewCall.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtmlWeb/Resgrid.Web/Areas/User/Views/Reports/EventAttendanceReport.cshtmlWeb/Resgrid.Web/Areas/User/Views/Templates/Edit.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.js
✅ Files skipped from review due to trivial changes (11)
- Providers/Resgrid.Providers.Migrations/Migrations/M0059_AddActiveForStatesToCheckInTimers.cs
- Web/Resgrid.Web/Areas/User/Models/Calendar/SignupSheetView.cs
- Web/Resgrid.Web/Areas/User/Views/Calendar/SignupSheet.cshtml
- Repositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectMatchingCheckInTimerOverridesQuery.cs
- Repositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByItemIdQuery.cs
- Web/Resgrid.Web/Areas/User/Views/Calendar/EditCheckIn.cshtml
- Web/Resgrid.Web/Areas/User/Views/Reports/EventAttendanceReport.cshtml
- Core/Resgrid.Model/Services/ICheckInTimerService.cs
- Core/Resgrid.Model/CheckInTimerConfig.cs
- Core/Resgrid.Model/CheckInTimerOverride.cs
- Web/Resgrid.Web.Services/Models/v4/CheckInTimers/CheckInTimerModels.cs
🚧 Files skipped from review as they are similar to previous changes (18)
- Web/Resgrid.Web/Areas/User/Views/Templates/Edit.cshtml
- Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.js
- Repositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInTimerConfigByDepartmentAndTargetQuery.cs
- Web/Resgrid.Web/Areas/User/Views/Calendar/New.cshtml
- Repositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInRecordsByCallIdQuery.cs
- Repositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInTimerConfigsByDepartmentIdQuery.cs
- Repositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectLastCheckInForUserOnCallQuery.cs
- Repositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInTimerOverridesByDepartmentIdQuery.cs
- Repositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByUserDateRangeQuery.cs
- Repositories/Resgrid.Repositories.DataRepository/Queries/Calendar/SelectCalendarItemCheckInsByDeptDateRangeQuery.cs
- Repositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectCheckInRecordsByDepartmentIdAndDateRangeQuery.cs
- Repositories/Resgrid.Repositories.DataRepository/Queries/CheckIns/SelectLastCheckInForUnitOnCallQuery.cs
- Web/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtml
- Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs
- Web/Resgrid.Web.Services/Hubs/EventingHub.cs
- Core/Resgrid.Services/ServicesModule.cs
- Web/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.cs
- Core/Resgrid.Services/CalendarService.cs
Comment on lines
+1611
to
+1618
| var childGroups = await _departmentGroupsService.GetAllChildDepartmentGroupsAsync(adminGroup.DepartmentGroupId); | ||
| if (childGroups != null) | ||
| { | ||
| foreach (var childGroup in childGroups) | ||
| { | ||
| if (childGroup.IsUserInGroup(targetUserId)) | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Walk the full descendant tree, not just direct children.
GetAllChildDepartmentGroupsAsync only returns immediate children in Core/Resgrid.Services/DepartmentGroupsService.cs, so these checks will deny valid admin check-in/edit operations for users who belong to grandchild groups under the admin's branch.
Also applies to: 1654-1661
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Services/AuthorizationService.cs` around lines 1611 - 1618, The
current admin membership check in AuthorizationService (the block using
_departmentGroupsService.GetAllChildDepartmentGroupsAsync and the similar block
at 1654-1661) only inspects immediate children and misses deeper descendants;
update the logic to walk the full descendant tree by either calling a service
method that returns all descendants (e.g., implement/use
GetAllDescendantDepartmentGroupsAsync in DepartmentGroupsService) or perform a
recursive traversal of GetAllChildDepartmentGroupsAsync results, checking
IsUserInGroup(targetUserId) on every descendant group; ensure both the 1611
block and the 1654-1661 block use the new recursive/descendant-aware lookup so
users in grandchild (or deeper) groups are correctly recognized.
Comment on lines
+17
to
+29
| // Replace with filtered unique indexes that only enforce uniqueness | ||
| // when nullable columns are NOT NULL, allowing multiple NULL rows. | ||
| Execute.Sql(@" | ||
| CREATE UNIQUE NONCLUSTERED INDEX UQ_CheckInTimerConfigs_Dept_Target_Unit | ||
| ON CheckInTimerConfigs (DepartmentId, TimerTargetType, UnitTypeId) | ||
| WHERE UnitTypeId IS NOT NULL; | ||
| "); | ||
|
|
||
| Execute.Sql(@" | ||
| CREATE UNIQUE NONCLUSTERED INDEX UQ_CheckInTimerOverrides_Dept_Call_Target_Unit | ||
| ON CheckInTimerOverrides (DepartmentId, CallTypeId, CallPriority, TimerTargetType, UnitTypeId) | ||
| WHERE CallTypeId IS NOT NULL AND CallPriority IS NOT NULL AND UnitTypeId IS NOT NULL; | ||
| "); |
Contributor
There was a problem hiding this comment.
Cross-platform behavioral inconsistency with PostgreSQL migration.
The filtered unique indexes here allow multiple NULL rows (WHERE UnitTypeId IS NOT NULL excludes NULLs from uniqueness checks). However, the PostgreSQL equivalent in M0058_FixCheckInTimerNullableUniqueConstraintsPg.cs uses NULLS NOT DISTINCT, which prevents duplicate NULL rows.
This means:
- SQL Server: Multiple rows with
UnitTypeId = NULL(or other nullable columns) are allowed - PostgreSQL: Only one row with
UnitTypeId = NULLis allowed per unique key combination
If the intent is to prevent duplicate "Any/None" rules (as stated in the PostgreSQL migration comment), the SQL Server filtered indexes achieve the opposite effect. Please verify the intended behavior and align both migrations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0058_FixCheckInTimerNullableUniqueConstraints.cs`
around lines 17 - 29, The SQL Server migrations
(M0058_FixCheckInTimerNullableUniqueConstraints.cs) create filtered unique
indexes (UQ_CheckInTimerConfigs_Dept_Target_Unit and
UQ_CheckInTimerOverrides_Dept_Call_Target_Unit) that exclude NULLs, allowing
multiple NULL rows, while the PostgreSQL migration
(M0058_FixCheckInTimerNullableUniqueConstraintsPg.cs) enforces uniqueness for
NULLs using NULLS NOT DISTINCT; align behavior by changing the SQL Server side
to treat NULL as a distinct value for uniqueness: add a persisted computed
column for each nullable key (e.g., UnitTypeId_ForUnique =
ISNULL(CONVERT(varchar(50), UnitTypeId), '<NULL>') or COALESCE(UnitTypeId,
<safe-sentinel>)) and create the unique indexes on those computed columns
instead of filtering (update the index creation for both CheckInTimerConfigs and
CheckInTimerOverrides to reference the computed columns so SQL Server will allow
only one "NULL"/Any row matching the intent).
Comment on lines
+10
to
+30
| // Drop the existing unique constraints that allow duplicate NULL rows | ||
| Delete.UniqueConstraint("uq_checkintimerconfigs_dept_target_unit") | ||
| .FromTable("checkintimerconfigs"); | ||
|
|
||
| Delete.UniqueConstraint("uq_checkintimeroverrides_dept_call_target_unit") | ||
| .FromTable("checkintimeroverrides"); | ||
|
|
||
| // Replace with NULLS NOT DISTINCT unique indexes (PostgreSQL 15+) | ||
| // so that NULL values are treated as equal for uniqueness checks, | ||
| // preventing duplicate "Any/None" rules. | ||
| Execute.Sql(@" | ||
| CREATE UNIQUE INDEX uq_checkintimerconfigs_dept_target_unit | ||
| ON checkintimerconfigs (departmentid, timertargettype, unittypeid) | ||
| NULLS NOT DISTINCT; | ||
| "); | ||
|
|
||
| Execute.Sql(@" | ||
| CREATE UNIQUE INDEX uq_checkintimeroverrides_dept_call_target_unit | ||
| ON checkintimeroverrides (departmentid, calltypeid, callpriority, timertargettype, unittypeid) | ||
| NULLS NOT DISTINCT; | ||
| "); |
Contributor
There was a problem hiding this comment.
Handle legacy duplicate NULL rows before building these indexes.
The old constraints allowed duplicates whenever one of the key columns was NULL, so upgraded databases can already contain rows that violate the new NULLS NOT DISTINCT rule. CREATE UNIQUE INDEX will fail on those tenants unless you merge/delete duplicates or explicitly guard for them first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0058_FixCheckInTimerNullableUniqueConstraintsPg.cs`
around lines 10 - 30, The migration creates new NULLS NOT DISTINCT unique
indexes but doesn't remove existing duplicate rows that would violate them;
before executing CREATE UNIQUE INDEX for uq_checkintimerconfigs_dept_target_unit
on checkintimerconfigs and uq_checkintimeroverrides_dept_call_target_unit on
checkintimeroverrides, add SQL to detect and deduplicate legacy rows (e.g.,
group by the indexed key columns and keep one row per group, deleting others) or
merge data as needed; implement this cleanup in
M0058_FixCheckInTimerNullableUniqueConstraintsPg.cs so the Execute.Sql(...) that
creates each unique index runs only after duplicates have been removed.
Comment on lines
+630
to
+639
| // Check if user is event creator or group admin (for admin check-in buttons) | ||
| if (!model.IsAdmin) | ||
| { | ||
| if (!string.IsNullOrWhiteSpace(model.CalendarItem.CreatorUserId) && model.CalendarItem.CreatorUserId == UserId) | ||
| model.IsAdmin = true; | ||
| else | ||
| { | ||
| var group = await _departmentGroupsService.GetGroupForUserAsync(UserId, DepartmentId); | ||
| if (group != null && group.IsUserGroupAdmin(UserId)) | ||
| model.IsAdmin = true; |
Contributor
There was a problem hiding this comment.
Model.IsAdmin is too coarse for attendee-level actions.
Any group admin flips this flag for the entire page, but CanUserAdminCheckInCalendarEventAsync and CanUserEditCalendarCheckInAsync only allow actions on users in that admin's group tree. The view will render check-in/check-out/edit buttons for unrelated attendees and then bounce those submits with Unauthorized().
🤖 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
630 - 639, Model.IsAdmin is being set globally which causes the view to render
admin actions for all attendees; instead determine admin rights per-attendee
using the same group-scoped checks used by CanUserAdminCheckInCalendarEventAsync
and CanUserEditCalendarCheckInAsync. Replace the blanket model.IsAdmin
assignment with logic that calls
_departmentGroupsService.GetGroupForUserAsync(UserId, DepartmentId) and
group.IsUserGroupAdmin(UserId) to compute a per-attendee flag (e.g.,
Attendee.CanAdmin or a dictionary on the model keyed by attendee user id) and
populate that for each CalendarItem attendee so the view only shows
check-in/check-out/edit buttons for attendees within the admin’s group tree.
Comment on lines
+405
to
+411
| <div class="hr-line-dashed"></div> | ||
| <div class="form-group"> | ||
| <label class="col-sm-2 control-label">@localizer["CheckInTimersLabel"]</label> | ||
| <div class="col-sm-10"> | ||
| <div class="checkbox"> | ||
| <label> | ||
| <input asp-for="Call.CheckInTimersEnabled" /> @localizer["EnableCheckInTimers"] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and verify the view file path and check the line range
find . -type f -name "NewCall.cshtml" 2>/dev/null | head -20Repository: Resgrid/Core
Length of output: 114
🏁 Script executed:
# Find the DispatchController file
find . -type f -name "DispatchController.cs" 2>/dev/null | head -20Repository: Resgrid/Core
Length of output: 182
🏁 Script executed:
# Find the Call model class
find . -type f -name "Call.cs" 2>/dev/null | head -20Repository: Resgrid/Core
Length of output: 140
🏁 Script executed:
# Check the NewCall.cshtml file around lines 405-411
sed -n '400,415p' ./Web/Resgrid.Web/Areas/User/Views/Dispatch/NewCall.cshtml | cat -nRepository: Resgrid/Core
Length of output: 1001
🏁 Script executed:
# Check the DispatchController NewCall method and surrounding code
grep -n "NewCall" ./Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs | head -5Repository: Resgrid/Core
Length of output: 367
🏁 Script executed:
# Verify the Call.CheckInTimersEnabled property in the model
grep -n "CheckInTimersEnabled" ./Core/Resgrid.Model/Call.cs | head -10Repository: Resgrid/Core
Length of output: 108
🏁 Script executed:
# Look for GetCheckInTimersAutoEnableForNewCallsAsync method
grep -rn "GetCheckInTimersAutoEnableForNewCallsAsync" --include="*.cs" 2>/dev/nullRepository: Resgrid/Core
Length of output: 703
🏁 Script executed:
# Get the full NewCall GET method (lines 179-206)
sed -n '179,206p' ./Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs | cat -nRepository: Resgrid/Core
Length of output: 942
🏁 Script executed:
# Find and check the FillNewCallView method
grep -n "FillNewCallView" ./Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs | head -5Repository: Resgrid/Core
Length of output: 310
🏁 Script executed:
# Check how v4 CallsController handles this (line 616 context)
sed -n '610,625p' ./Web/Resgrid.Web.Services/Controllers/v4/CallsController.cs | cat -nRepository: Resgrid/Core
Length of output: 780
🏁 Script executed:
# Check the FillNewCallView implementation (line 2578)
sed -n '2578,2650p' ./Web/Resgrid.Web/Areas/User/Controllers/DispatchController.cs | cat -nRepository: Resgrid/Core
Length of output: 4141
🏁 Script executed:
# Check if FillNewCallView mentions CheckInTimersEnabled anywhere
grep -n "CheckInTimersEnabled" ./Web/Resgrid.Web/Areas/User/Controllers/DispatchController.csRepository: Resgrid/Core
Length of output: 631
Seed Call.CheckInTimersEnabled from the department setting on the GET request.
The NewCall GET method creates new Call() without populating CheckInTimersEnabled. Because this property is a non-nullable bool, the checkbox renders unchecked by default. If the dispatcher doesn't manually check it, the form post sends false, overriding the department's auto-enable setting.
The v4 CallsController already implements the correct pattern: when CheckInTimersEnabled is not explicitly set, it queries GetCheckInTimersAutoEnableForNewCallsAsync to apply the department default. Apply the same logic in the web form:
Fix in `DispatchController.NewCall`
var model = new NewCallView();
-model.Call = new Resgrid.Model.Call();
+model.Call = new Resgrid.Model.Call
+{
+ CheckInTimersEnabled = await _departmentSettingsService.GetCheckInTimersAutoEnableForNewCallsAsync(DepartmentId)
+};
model = await FillNewCallView(model);🤖 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/NewCall.cshtml` around lines 405 -
411, The NewCall GET path is creating a fresh Call without setting
Call.CheckInTimersEnabled so the checkbox defaults to false and can override the
department default; update DispatchController.NewCall to seed the new Call's
CheckInTimersEnabled by calling the same department helper used in v4 (e.g.,
invoke GetCheckInTimersAutoEnableForNewCallsAsync or the Departments service
method that returns the department default) and assign its result to
Call.CheckInTimersEnabled before returning the view so the checkbox reflects the
department auto-enable setting on initial render.
Comment on lines
+4
to
+14
| var timerStatuses = []; | ||
| var timerInterval = null; | ||
|
|
||
| $(document).ready(function () { | ||
| // callState 0 = Active | ||
| if (typeof callState !== 'undefined' && callState === 0) { | ||
| loadTimerStatuses(); | ||
| timerInterval = setInterval(updateCountdowns, 1000); | ||
| } | ||
| loadCheckInHistory(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Timer interval is never cleared, causing potential memory leak.
The timerInterval is initialized on document ready but never cleared. If the user navigates away (especially in SPA-style navigation) or the DOM elements are removed, the interval continues running, potentially causing errors and memory leaks.
🛠️ Proposed fix
Add cleanup when appropriate (e.g., on page unload or when call becomes inactive):
var timerStatuses = [];
var timerInterval = null;
$(document).ready(function () {
// callState 0 = Active
if (typeof callState !== 'undefined' && callState === 0) {
loadTimerStatuses();
timerInterval = setInterval(updateCountdowns, 1000);
}
loadCheckInHistory();
+
+ $(window).on('beforeunload', function () {
+ if (timerInterval) {
+ clearInterval(timerInterval);
+ timerInterval = null;
+ }
+ });
});🤖 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/dispatch/resgrid.dispatch.checkin-timers.js`
around lines 4 - 14, The interval assigned to timerInterval (set in the
document.ready block after calling loadTimerStatuses and updateCountdowns) is
never cleared; add cleanup to clearInterval(timerInterval) when the page unloads
or when the call becomes inactive (e.g., on window.beforeunload/window.unload or
when callState changes from 0), and ensure timerInterval is checked for null
before clearing and set to null after clearing to avoid duplicate clears and
memory leaks.
Comment on lines
+16
to
+27
| function loadTimerStatuses() { | ||
| $.ajax({ | ||
| url: resgrid.absoluteBaseUrl + '/User/Dispatch/GetCheckInTimerStatuses?callId=' + callId, | ||
| contentType: 'application/json; charset=utf-8', | ||
| type: 'GET' | ||
| }).done(function (result) { | ||
| if (result) { | ||
| timerStatuses = result; | ||
| renderTimerStatuses(); | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing error handling for AJAX request.
The loadTimerStatuses function lacks a .fail() handler. If the request fails (network error, server error), the UI will silently remain in its current state without any feedback to the user.
🛠️ Proposed fix
function loadTimerStatuses() {
$.ajax({
url: resgrid.absoluteBaseUrl + '/User/Dispatch/GetCheckInTimerStatuses?callId=' + callId,
contentType: 'application/json; charset=utf-8',
type: 'GET'
}).done(function (result) {
if (result) {
timerStatuses = result;
renderTimerStatuses();
}
- });
+ }).fail(function () {
+ console.error('Failed to load check-in timer statuses');
+ });
}🤖 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/dispatch/resgrid.dispatch.checkin-timers.js`
around lines 16 - 27, The AJAX call in loadTimerStatuses lacks error handling;
add a .fail() (or .catch for promise) handler on the $.ajax call that logs the
error and surfaces UI feedback (e.g. set a status flag or show a message) so the
user knows the request failed; update loadTimerStatuses to handle failures for
the URL built with callId, ensure timerStatuses is not overwritten on error, and
optionally call a new or existing renderTimerStatusesError / renderTimerStatuses
fallback path to update the UI.
Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js
Show resolved
Hide resolved
Comment on lines
+108
to
+118
| function loadCheckInHistory() { | ||
| $.ajax({ | ||
| url: resgrid.absoluteBaseUrl + '/User/Dispatch/GetCheckInHistory?callId=' + callId, | ||
| contentType: 'application/json; charset=utf-8', | ||
| type: 'GET' | ||
| }).done(function (result) { | ||
| if (result) { | ||
| renderCheckInHistory(result); | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing error handling for AJAX request.
Same issue as loadTimerStatuses — loadCheckInHistory silently fails on errors.
🛠️ Proposed fix
function loadCheckInHistory() {
$.ajax({
url: resgrid.absoluteBaseUrl + '/User/Dispatch/GetCheckInHistory?callId=' + callId,
contentType: 'application/json; charset=utf-8',
type: 'GET'
}).done(function (result) {
if (result) {
renderCheckInHistory(result);
}
- });
+ }).fail(function () {
+ console.error('Failed to load check-in history');
+ });
}🤖 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/dispatch/resgrid.dispatch.checkin-timers.js`
around lines 108 - 118, The AJAX call in loadCheckInHistory lacks error handling
and will fail silently; update loadCheckInHistory to attach a .fail (or .catch)
handler to the $.ajax promise (similar to loadTimerStatuses) that handles errors
by logging to console or calling an existing error logger and showing a
user-friendly message/UI state, and ensure any cleanup or fallback (e.g.,
clearing/loading spinners) is done before/after calling renderCheckInHistory to
keep behavior consistent on failure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js (3)
108-118:⚠️ Potential issue | 🟡 MinorHandle check-in history fetch failures explicitly.
loadCheckInHistoryalso has no.fail()branch, so the panel can silently remain stale on network/server errors.Proposed fix
$.ajax({ url: resgrid.absoluteBaseUrl + '/User/Dispatch/GetCheckInHistory?callId=' + callId, contentType: 'application/json; charset=utf-8', type: 'GET' }).done(function (result) { if (result) { renderCheckInHistory(result); } + }).fail(function () { + console.error('Failed to load check-in history'); });🤖 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/dispatch/resgrid.dispatch.checkin-timers.js` around lines 108 - 118, The loadCheckInHistory function lacks error handling for its AJAX call; add a .fail(...) handler to the $.ajax chain for the function loadCheckInHistory so network/server errors are handled explicitly (log the error, show a user-friendly message or update the UI panel to indicate the fetch failed), and consider adding a .always(...) or timeout handling to ensure the check-in history panel is not left stale; keep renderCheckInHistory as the success path and use the new fail handler to surface or recover from errors.
7-14:⚠️ Potential issue | 🟡 MinorClear
timerIntervalon teardown/inactive state.Line 11 creates a repeating interval, but there is no cleanup path. This can keep running after navigation and cause stale DOM polling.
Proposed fix
$(document).ready(function () { // callState 0 = Active if (typeof callState !== 'undefined' && callState === 0) { loadTimerStatuses(); timerInterval = setInterval(updateCountdowns, 1000); } loadCheckInHistory(); + + $(window).on('beforeunload unload', function () { + if (timerInterval !== null) { + clearInterval(timerInterval); + timerInterval = null; + } + }); });🤖 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/dispatch/resgrid.dispatch.checkin-timers.js` around lines 7 - 14, The interval started by timerInterval in the $(document).ready block (setInterval(updateCountdowns, 1000)) has no teardown; modify the code so you clear any existing timerInterval before creating a new one (check timerInterval and call clearInterval(timerInterval)), and register a cleanup handler (e.g. $(window).on('unload' or 'beforeunload') or a teardown function) that calls clearInterval(timerInterval) when the page is unloaded or when callState becomes inactive; updateCountdowns, loadTimerStatuses and loadCheckInHistory references remain the same.
16-27:⚠️ Potential issue | 🟡 MinorHandle timer-status fetch failures explicitly.
loadTimerStatuseshas no failure branch, so request errors fail silently and leave stale UI.Proposed fix
$.ajax({ url: resgrid.absoluteBaseUrl + '/User/Dispatch/GetCheckInTimerStatuses?callId=' + callId, contentType: 'application/json; charset=utf-8', type: 'GET' }).done(function (result) { if (result) { timerStatuses = result; renderTimerStatuses(); } + }).fail(function () { + console.error('Failed to load check-in timer statuses'); });🤖 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/dispatch/resgrid.dispatch.checkin-timers.js` around lines 16 - 27, loadTimerStatuses currently only handles the successful AJAX case and swallows errors; add an explicit failure handler for the $.ajax call in loadTimerStatuses to avoid stale UI. In the fail callback (or .always with status check) capture and log the error, set timerStatuses to a safe empty state (e.g., [] or null) and call renderTimerStatuses() so the UI updates, and surface a user-visible error (e.g., via existing notification/alert method) indicating the check-in timer fetch failed; reference function loadTimerStatuses, variable timerStatuses, function renderTimerStatuses and the callId/ajax request when making these changes.
🧹 Nitpick comments (10)
Web/Resgrid.Web.Services/Controllers/v4/PersonnelController.cs (2)
174-184: Duplicate async call for group retrieval on disabled/hidden users.When a disabled/hidden user passes the group-admin check (line 180),
GetGroupForUserAsyncis called here, and then called again at line 195 for the same user. Consider caching the result to avoid the duplicate database roundtrip.♻️ Proposed fix to cache group lookup
// Skip hidden/disabled users unless current user is dept admin or group admin of their group var member = departmentMembers.FirstOrDefault(x => x.UserId == u.UserId); -if (member != null && ((member.IsDisabled.HasValue && member.IsDisabled.Value) || (member.IsHidden.HasValue && member.IsHidden.Value))) +DepartmentGroup userGroup = null; +if (member != null && (member.IsDisabled.GetValueOrDefault() || member.IsHidden.GetValueOrDefault())) { if (!department.IsUserAnAdmin(UserId)) { - var userGroup = await _departmentGroupsService.GetGroupForUserAsync(u.UserId, DepartmentId); + userGroup = await _departmentGroupsService.GetGroupForUserAsync(u.UserId, DepartmentId); if (userGroup == null || !userGroup.IsUserGroupAdmin(UserId)) continue; } }Then at line 195:
-var group = await _departmentGroupsService.GetGroupForUserAsync(u.UserId, DepartmentId); +var group = userGroup ?? await _departmentGroupsService.GetGroupForUserAsync(u.UserId, DepartmentId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelController.cs` around lines 174 - 184, The code calls _departmentGroupsService.GetGroupForUserAsync(u.UserId, DepartmentId) twice for the same user when checking hidden/disabled membership and later group-admin checks; modify PersonnelController.cs to fetch the group once into a local variable (e.g., userGroup) before the first conditional and reuse that variable for the later IsUserGroupAdmin(UserId) check, ensuring you only await GetGroupForUserAsync a single time per user and adjust the existing member/is-hidden/is-disabled and department.IsUserAnAdmin(UserId) logic to reference the cached userGroup.
167-167: Consider using a dictionary for O(1) lookup instead of repeated linear search.The
departmentMembers.FirstOrDefault()inside the loop results in O(n) lookup per user. With large departments, this becomes O(n²) overall. A dictionary would provide O(1) lookups.♻️ Suggested optimization
-var departmentMembers = await _departmentsService.GetAllMembersForDepartmentAsync(DepartmentId); +var departmentMembers = (await _departmentsService.GetAllMembersForDepartmentAsync(DepartmentId)) + .ToDictionary(x => x.UserId, x => x); foreach (var u in users) { if (!await _authorizationService.CanUserViewPersonViaMatrixAsync(u.UserId, UserId, DepartmentId)) continue; // Skip hidden/disabled users unless current user is dept admin or group admin of their group - var member = departmentMembers.FirstOrDefault(x => x.UserId == u.UserId); + departmentMembers.TryGetValue(u.UserId, out var member);Also applies to: 174-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelController.cs` at line 167, Replace repeated linear searches by building a lookup dictionary from departmentMembers keyed by MemberId (e.g., var membersById = departmentMembers.ToDictionary(m => m.MemberId)) and then use membersById.TryGetValue(user.Id, out var deptMember) or membersById[user.Id] inside the loop instead of departmentMembers.FirstOrDefault(...); update the other occurrences noted (the calls around lines 174–176) to use the same dictionary for O(1) lookups in PersonnelController where departmentMembers is fetched.Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs (1)
923-934: Consider extracting the hidden/disabled user filtering logic to reduce duplication.The filtering logic for skipping hidden/disabled users is duplicated across
Index()(lines 143-153),GetPersonnelList()(lines 923-934), andGetPersonnelListPaged()(lines 1068-1079). Extracting this to a helper method would improve maintainability and ensure consistent behavior when visibility rules change.♻️ Suggested extraction
private async Task<bool> ShouldSkipHiddenOrDisabledMemberAsync( DepartmentMember member, Department department, string targetUserId) { if (member == null) return false; bool isHiddenOrDisabled = (member.IsDisabled.HasValue && member.IsDisabled.Value) || (member.IsHidden.HasValue && member.IsHidden.Value); if (!isHiddenOrDisabled) return false; if (department.IsUserAnAdmin(UserId)) return false; var userGroup = await _departmentGroupsService.GetGroupForUserAsync(targetUserId, DepartmentId); return userGroup == null || !userGroup.IsUserGroupAdmin(UserId); }Then replace the duplicated blocks with:
if (await ShouldSkipHiddenOrDisabledMemberAsync(member, department, user.UserId)) continue;As per coding guidelines, this aligns with "Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs` around lines 923 - 934, Extract the duplicated hidden/disabled user check into a single async helper (e.g., ShouldSkipHiddenOrDisabledMemberAsync) that accepts the DepartmentMember, Department, and the target userId, performs the IsDisabled/IsHidden checks, skips if the current user is a department admin (Department.IsUserAnAdmin(UserId)), otherwise queries _departmentGroupsService.GetGroupForUserAsync(targetUserId, DepartmentId) and returns whether to skip based on IsUserGroupAdmin; then replace the repeated blocks in Index(), GetPersonnelList(), and GetPersonnelListPaged() with a single call: if (await ShouldSkipHiddenOrDisabledMemberAsync(member, department, user.UserId)) continue; ensuring the helper is reused everywhere to maintain consistent behavior.Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js (1)
197-200: Keep the modal open on submit failure to preserve user input.On failure, hiding the modal discards context and forces re-entry. Consider keeping it open and re-enabling the button so users can correct/retry immediately.
Proposed UX-friendly failure handling
}).fail(function () { $('#checkInSubmitBtn').prop('disabled', false); - $('#checkInModal').modal('hide'); alert('Failed to perform check-in. Please try again.'); });🤖 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/dispatch/resgrid.dispatch.checkin-timers.js` around lines 197 - 200, The fail handler currently hides the modal and alerts, which discards user input; update the .fail(function () { ... }) block so it does NOT call $('#checkInModal').modal('hide'), keeps the modal visible, re-enables the submit control $('#checkInSubmitBtn').prop('disabled', false), and surface the error in-place (e.g., show an inline error message element inside the modal or update an existing status container) so users can correct or retry without re-entering data.Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs (2)
1777-1777: Fix inconsistent indentation.Line 1777 uses tabs instead of the surrounding whitespace pattern, breaking visual consistency with adjacent lines.
🛠️ Suggested fix
- model.CallTypes = await _callsService.GetCallTypesForDepartmentAsync(DepartmentId) ?? new List<CallType>(); + model.CallTypes = await _callsService.GetCallTypesForDepartmentAsync(DepartmentId) ?? new List<CallType>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs` at line 1777, The line assigning model.CallTypes in DepartmentController (model.CallTypes = await _callsService.GetCallTypesForDepartmentAsync(DepartmentId) ?? new List<CallType>();) uses a tab for indentation which is inconsistent with surrounding lines; update that line to use the same whitespace pattern as adjacent code (replace the tab with the project’s preferred spaces), ensuring the statement remains unchanged except for indentation so DepartmentController formatting matches the file.
1827-1835: Consider handling the delete result for user feedback.
DeleteTimerConfigAsyncreturnsfalsewhen the config doesn't exist, but the result is ignored. While this works (redirect happens regardless), users won't know if deletion actually succeeded or if the item was already gone.💡 Optional improvement
public async Task<IActionResult> DeleteCheckInTimerConfig(string configId, CancellationToken cancellationToken) { if (!await _authorizationService.CanUserModifyDepartmentAsync(UserId, DepartmentId)) return Unauthorized(); - await _checkInTimerService.DeleteTimerConfigAsync(configId, DepartmentId, cancellationToken); + var deleted = await _checkInTimerService.DeleteTimerConfigAsync(configId, DepartmentId, cancellationToken); + // Optionally: set TempData message if !deleted return RedirectToAction("DispatchSettings"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs` around lines 1827 - 1835, The DeleteCheckInTimerConfig action ignores the boolean result from _checkInTimerService.DeleteTimerConfigAsync so users get no feedback if deletion failed or the config didn't exist; update DeleteCheckInTimerConfig to await the boolean result from DeleteTimerConfigAsync(configId, DepartmentId, cancellationToken), and branch on that result to set appropriate user feedback (e.g., TempData or ModelState) or choose a different redirect/view when false versus true, ensuring the method still enforces authorization via _authorizationService.CanUserModifyDepartmentAsync.Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs (1)
166-171: Extract the standard permission options into a helper.This is the same four-option list already copied for
CreateLog,DeleteCall,CloseCall, and several other permissions. Another inline copy makes future label/scope changes easy to miss.♻️ Suggested refactor
- var deleteLogPermissions = new List<dynamic>(); - deleteLogPermissions.Add(new { Id = 3, Name = "Everyone" }); - deleteLogPermissions.Add(new { Id = 0, Name = "Department Admins" }); - deleteLogPermissions.Add(new { Id = 1, Name = "Department and Group Admins" }); - deleteLogPermissions.Add(new { Id = 2, Name = "Department Admins and Select Roles" }); - model.DeleteLogPermissions = new SelectList(deleteLogPermissions, "Id", "Name"); + model.DeleteLogPermissions = BuildCrudPermissionOptions();private static SelectList BuildCrudPermissionOptions() => new SelectList(new[] { new { Id = 3, Name = "Everyone" }, new { Id = 0, Name = "Department Admins" }, new { Id = 1, Name = "Department and Group Admins" }, new { Id = 2, Name = "Department Admins and Select Roles" } }, "Id", "Name");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs` around lines 166 - 171, Extract the repeated four-option permission list into a single helper and replace the inline construction used for model.DeleteLogPermissions (and the copies used for CreateLog, DeleteCall, CloseCall, etc.) with that helper; add a private static method such as BuildCrudPermissionOptions() that returns a SelectList of the four entries (Ids 3,0,1,2 with the corresponding Names) and then set model.DeleteLogPermissions = BuildCrudPermissionOptions() (and update the other places to call this same method) so label/scope changes are centralized.Providers/Resgrid.Providers.Claims/ResgridIdentity.cs (1)
237-280: Keep one source of truth for delete-log claim evaluation.The active claim builders now route through
ClaimsLogic.AddDeleteLogClaims, and this instance copy is not used in the provided call-site context. Maintaining both implementations will be brittle.♻️ Suggested fix
- public void AddDeleteLogClaims(bool isAdmin, List<Permission> permissions, bool isGroupAdmin, List<PersonnelRole> roles) - { - if (permissions != null && permissions.Any(x => x.PermissionType == (int)PermissionTypes.DeleteLog)) - { - var permission = permissions.First(x => x.PermissionType == (int)PermissionTypes.DeleteLog); - - if (permission.Action == (int)PermissionActions.DepartmentAdminsOnly && isAdmin) - { - AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); - } - else if (permission.Action == (int)PermissionActions.DepartmentAndGroupAdmins && (isAdmin || isGroupAdmin)) - { - AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); - } - else if (permission.Action == (int)PermissionActions.DepartmentAdminsAndSelectRoles && isAdmin) - { - AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); - } - else if (permission.Action == (int)PermissionActions.DepartmentAdminsAndSelectRoles && !isAdmin) - { - if (!String.IsNullOrWhiteSpace(permission.Data)) - { - var roleIds = permission.Data.Split(char.Parse(",")).Select(int.Parse); - var role = from r in roles - where roleIds.Contains(r.PersonnelRoleId) - select r; - - if (role.Any()) - { - AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); - } - } - } - else if (permission.Action == (int)PermissionActions.Everyone) - { - AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); - } - } - else - { - // Default: everyone can delete logs - AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); - } - } + public void AddDeleteLogClaims(bool isAdmin, List<Permission> permissions, bool isGroupAdmin, List<PersonnelRole> roles) + { + ClaimsLogic.AddDeleteLogClaims(this, isAdmin, permissions, isGroupAdmin, roles); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Claims/ResgridIdentity.cs` around lines 237 - 280, This AddDeleteLogClaims method in ResgridIdentity.cs duplicates logic already centralized in ClaimsLogic.AddDeleteLogClaims; replace the duplicated implementation in the ResgridIdentity.AddDeleteLogClaims method with a single delegation call to ClaimsLogic.AddDeleteLogClaims (forward the same parameters: isAdmin, permissions, isGroupAdmin, roles) so all delete-log claim evaluation comes from the single source of truth (ClaimsLogic.AddDeleteLogClaims) and remove the local branching/permission parsing code.Web/Resgrid.Web/Areas/User/Controllers/LogsController.cs (2)
361-363: Consider convertingpersonnelNamesto a dictionary for O(1) lookups.The current implementation performs
FirstOrDefaultsearches onpersonnelNames(line 399) which is O(n) per lookup. With many logs and users, this becomes inefficient.♻️ Suggested optimization
var personnelNames = await _departmentsService.GetAllPersonnelNamesForDepartmentAsync(DepartmentId); var units = await _unitsService.GetUnitsForDepartmentAsync(DepartmentId); var unitLookup = units.ToDictionary(u => u.UnitId, u => u.Name); +var personnelLookup = personnelNames.ToDictionary(p => p.UserId, p => p.Name);Then at line 399:
-var pn = personnelNames.FirstOrDefault(x => x.UserId == logUser.UserId); -if (pn != null) - terms.Add(pn.Name); +if (personnelLookup.TryGetValue(logUser.UserId, out var personnelName)) + terms.Add(personnelName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/LogsController.cs` around lines 361 - 363, The code performs repeated FirstOrDefault searches against personnelNames causing O(n) lookups; change the result of _departmentsService.GetAllPersonnelNamesForDepartmentAsync(DepartmentId) into a dictionary (e.g., personnelById) keyed by PersonnelId for O(1) access, update subsequent lookups that use personnelNames (where FirstOrDefault is called) to query personnelById.TryGetValue or index into the dictionary, and ensure the new dictionary variable name (personnelById) and the service call (GetAllPersonnelNamesForDepartmentAsync) are used in LogsController methods that build log entries so behavior remains identical.
415-434: Consider batch-loading calls to reduce database round-trips.The current implementation makes individual
GetCallByIdAsynccalls within the loop (line 420). While the cache prevents duplicate lookups, logs with many uniqueCallIdvalues will result in sequential database calls.♻️ Suggested optimization
Pre-collect all unique
CallIdvalues and batch-load them before the loop:// Before the foreach loop var callIds = logs.Where(l => l.CallId.HasValue).Select(l => l.CallId.Value).Distinct().ToList(); var calls = await _callsService.GetCallsByIdsAsync(callIds); // If such method exists var callCache = calls.ToDictionary(c => c.CallId);This would require adding a batch-load method to
ICallsServiceif not already available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/LogsController.cs` around lines 415 - 434, The loop in LogsController that fetches call details uses _callsService.GetCallByIdAsync per log (see log.CallId and callCache usage), causing many DB round-trips; instead pre-collect distinct CallId values from the logs, add a new batch method on ICallsService (e.g., GetCallsByIdsAsync), call that once to load all matching Call objects, populate callCache from the returned list, and then use callCache inside the existing foreach so you no longer call GetCallByIdAsync for each log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Providers/Resgrid.Providers.Claims/ClaimsLogic.cs`:
- Around line 236-278: Remove the unsafe fallback in AddDeleteLogClaims that
grants Log/Delete when the DeleteLog permission is missing: instead, if
permissions is null or no PermissionTypes.DeleteLog exists, check for a legacy
PermissionTypes.CreateLog permission and, if present, apply the same
PermissionActions mapping (DepartmentAdminsOnly, DepartmentAndGroupAdmins,
DepartmentAdminsAndSelectRoles, Everyone) to determine whether to add the
ResgridClaimTypes.Resources.Log + Actions.Delete claim; if no legacy CreateLog
mapping exists, do not add the claim (deny by default). Update the analogous
logic in SecurityController (the same DeleteLog fallback) to follow this
behavior so departments without explicit DeleteLog remain restricted unless an
explicit DeleteLog or legacy CreateLog mapping grants it.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0060_FixCheckInTimerNullUniqueness.cs`:
- Around line 16-38: The SQL in the migration is ordering duplicates by Guid
string IDs (CheckInTimerConfigId / CheckInTimerOverrideId) which are
non-chronological; update both CTE ORDER BY clauses to use UpdatedOn DESC as the
primary sort and then the ID (CheckInTimerConfigId or CheckInTimerOverrideId)
DESC as a deterministic tiebreaker so the most recently modified row is
preserved; apply the same change to the PostgreSQL variant (the analogous
MAX(...) selection) and ensure you reference UpdatedOn first, then the ID as
tie-breaker.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0060_FixCheckInTimerNullUniquenessPg.cs`:
- Around line 5-45: The cleanup DELETEs that remove legacy duplicate rows are
executed in M0060 after strict NULLS NOT DISTINCT indexes are recreated, but
M0058 already creates those strict indexes earlier so migration 58 will fail
before M0060 can run; move the duplicate-row removal SQL (the two DELETE FROM
... WHERE ... NOT IN (SELECT MAX(...)) statements currently in M0060.Up) to run
before any CREATE UNIQUE INDEX calls — either by inserting those DELETEs into
M0058_FixCheckInTimerNullableUniqueConstraintsPg.Up before it creates
uq_checkintimerconfigs_dept_target_unit and
uq_checkintimeroverrides_dept_call_target_unit, or by moving the DELETEs to the
top of M0060.Up so they execute before any strict index creation; ensure the
index names (uq_checkintimerconfigs_dept_target_unit,
uq_checkintimeroverrides_dept_call_target_unit) and table identifiers
(checkintimerconfigs, checkintimeroverrides) remain referenced correctly.
In `@Web/Resgrid.Web/Areas/User/Controllers/LogsController.cs`:
- Around line 482-494: The code calls GetWorkLogByIdAsync and immediately
accesses log.CloneJsonToString(), which will throw if log is null; update the
LogsController method to check the retrieved log for null after calling
GetWorkLogByIdAsync and handle it safely (e.g., return a NotFound/appropriate
result or set auditEvent.Before to null/empty) before using
log.CloneJsonToString(); ensure you reference the log variable, the
CloneJsonToString call, and the AuditEvent/_eventAggregator.SendMessage flow so
the audit event is only populated with log data when log != null.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml`:
- Around line 536-538: The attendee lookup uses a case-sensitive equality
(c.UserId == a.UserId) causing inconsistent behavior with other case-insensitive
checks; update the Any predicate in the Model.CalendarItem.Attendees filtering
to use a case-insensitive comparison, e.g. replace c.UserId == a.UserId with
string.Equals(c.UserId, a.UserId, StringComparison.OrdinalIgnoreCase) (ensure
StringComparison is available where this view runs) so UserId comparisons match
the other usage and attendees are correctly filtered.
In `@Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml`:
- Around line 438-442: The localized placeholder string is being injected
directly into the JavaScript literal for $statesSelect.select2 (placeholder:
'@localizer["CheckInTimerActiveStatesLabel"]'), which can break JS if the
translation contains quotes or backslashes; change it to emit a JavaScript-safe
string by encoding the localization value before rendering (e.g., use a
server-side JS string encoder such as Json.Encode or
HttpUtility.JavaScriptStringEncode on
localizer["CheckInTimerActiveStatesLabel"]) and use that encoded value for the
placeholder property in the $statesSelect.select2 call.
In `@Web/Resgrid.Web/Areas/User/Views/Documents/Index.cshtml`:
- Around line 72-79: The EditDocument anchor is nested inside the outer
GetDocument download anchor (causing invalid nested <a> tags); update the
Documents tile markup so there is only one anchor per clickable region: keep the
outer anchor (asp-action="GetDocument"/download) wrapping the document tile or
name for download, and move the conditional edit link
(asp-action="EditDocument", ClaimsAuthorizationHelper.IsUserDepartmentAdmin()
check using doc.DocumentId/doc.Name) outside of that outer anchor (or limit the
outer anchor to only the icon), ensuring the edit link is rendered as a separate
sibling anchor so that EditDocument is not nested inside GetDocument.
In `@Web/Resgrid.Web/Areas/User/Views/Security/Index.cshtml`:
- Around line 167-168: The two table cells currently hardcode English ("Delete
Log Entries" and "Who in your department is allowed to delete log entries") and
must be localized like the other permission rows; replace the hardcoded strings
in Index.cshtml with calls to the existing IViewLocalizer/localizer (e.g.,
localizer["DeleteLogEntries"] and localizer["DeleteLogEntriesNote"]) and add
corresponding resource keys/values to the view resource file alongside the
existing permission strings so the row translates with the page.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js`:
- Around line 43-48: Replace the inline onclick that concatenates t.TargetType
and t.UnitId into HTML with data attributes and a delegated click handler:
render the button as a safe element with data-target-type and data-unit-id (set
to null or the id) instead of using showCheckInDialog(...) inline, then add a
delegated event listener on the container (or document) that listens for clicks
on that button selector, reads data-target-type and data-unit-id, coerces them
to numbers with strict parsing (parseInt/Number and check for NaN) and calls
showCheckInDialog(parsedTargetType, parsedUnitIdOrNull); this removes string
concatenation injection surface and enforces numeric typing for t.TargetType and
t.UnitId.
---
Duplicate comments:
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js`:
- Around line 108-118: The loadCheckInHistory function lacks error handling for
its AJAX call; add a .fail(...) handler to the $.ajax chain for the function
loadCheckInHistory so network/server errors are handled explicitly (log the
error, show a user-friendly message or update the UI panel to indicate the fetch
failed), and consider adding a .always(...) or timeout handling to ensure the
check-in history panel is not left stale; keep renderCheckInHistory as the
success path and use the new fail handler to surface or recover from errors.
- Around line 7-14: The interval started by timerInterval in the
$(document).ready block (setInterval(updateCountdowns, 1000)) has no teardown;
modify the code so you clear any existing timerInterval before creating a new
one (check timerInterval and call clearInterval(timerInterval)), and register a
cleanup handler (e.g. $(window).on('unload' or 'beforeunload') or a teardown
function) that calls clearInterval(timerInterval) when the page is unloaded or
when callState becomes inactive; updateCountdowns, loadTimerStatuses and
loadCheckInHistory references remain the same.
- Around line 16-27: loadTimerStatuses currently only handles the successful
AJAX case and swallows errors; add an explicit failure handler for the $.ajax
call in loadTimerStatuses to avoid stale UI. In the fail callback (or .always
with status check) capture and log the error, set timerStatuses to a safe empty
state (e.g., [] or null) and call renderTimerStatuses() so the UI updates, and
surface a user-visible error (e.g., via existing notification/alert method)
indicating the check-in timer fetch failed; reference function
loadTimerStatuses, variable timerStatuses, function renderTimerStatuses and the
callId/ajax request when making these changes.
---
Nitpick comments:
In `@Providers/Resgrid.Providers.Claims/ResgridIdentity.cs`:
- Around line 237-280: This AddDeleteLogClaims method in ResgridIdentity.cs
duplicates logic already centralized in ClaimsLogic.AddDeleteLogClaims; replace
the duplicated implementation in the ResgridIdentity.AddDeleteLogClaims method
with a single delegation call to ClaimsLogic.AddDeleteLogClaims (forward the
same parameters: isAdmin, permissions, isGroupAdmin, roles) so all delete-log
claim evaluation comes from the single source of truth
(ClaimsLogic.AddDeleteLogClaims) and remove the local branching/permission
parsing code.
In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelController.cs`:
- Around line 174-184: The code calls
_departmentGroupsService.GetGroupForUserAsync(u.UserId, DepartmentId) twice for
the same user when checking hidden/disabled membership and later group-admin
checks; modify PersonnelController.cs to fetch the group once into a local
variable (e.g., userGroup) before the first conditional and reuse that variable
for the later IsUserGroupAdmin(UserId) check, ensuring you only await
GetGroupForUserAsync a single time per user and adjust the existing
member/is-hidden/is-disabled and department.IsUserAnAdmin(UserId) logic to
reference the cached userGroup.
- Line 167: Replace repeated linear searches by building a lookup dictionary
from departmentMembers keyed by MemberId (e.g., var membersById =
departmentMembers.ToDictionary(m => m.MemberId)) and then use
membersById.TryGetValue(user.Id, out var deptMember) or membersById[user.Id]
inside the loop instead of departmentMembers.FirstOrDefault(...); update the
other occurrences noted (the calls around lines 174–176) to use the same
dictionary for O(1) lookups in PersonnelController where departmentMembers is
fetched.
In `@Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs`:
- Line 1777: The line assigning model.CallTypes in DepartmentController
(model.CallTypes = await
_callsService.GetCallTypesForDepartmentAsync(DepartmentId) ?? new
List<CallType>();) uses a tab for indentation which is inconsistent with
surrounding lines; update that line to use the same whitespace pattern as
adjacent code (replace the tab with the project’s preferred spaces), ensuring
the statement remains unchanged except for indentation so DepartmentController
formatting matches the file.
- Around line 1827-1835: The DeleteCheckInTimerConfig action ignores the boolean
result from _checkInTimerService.DeleteTimerConfigAsync so users get no feedback
if deletion failed or the config didn't exist; update DeleteCheckInTimerConfig
to await the boolean result from DeleteTimerConfigAsync(configId, DepartmentId,
cancellationToken), and branch on that result to set appropriate user feedback
(e.g., TempData or ModelState) or choose a different redirect/view when false
versus true, ensuring the method still enforces authorization via
_authorizationService.CanUserModifyDepartmentAsync.
In `@Web/Resgrid.Web/Areas/User/Controllers/LogsController.cs`:
- Around line 361-363: The code performs repeated FirstOrDefault searches
against personnelNames causing O(n) lookups; change the result of
_departmentsService.GetAllPersonnelNamesForDepartmentAsync(DepartmentId) into a
dictionary (e.g., personnelById) keyed by PersonnelId for O(1) access, update
subsequent lookups that use personnelNames (where FirstOrDefault is called) to
query personnelById.TryGetValue or index into the dictionary, and ensure the new
dictionary variable name (personnelById) and the service call
(GetAllPersonnelNamesForDepartmentAsync) are used in LogsController methods that
build log entries so behavior remains identical.
- Around line 415-434: The loop in LogsController that fetches call details uses
_callsService.GetCallByIdAsync per log (see log.CallId and callCache usage),
causing many DB round-trips; instead pre-collect distinct CallId values from the
logs, add a new batch method on ICallsService (e.g., GetCallsByIdsAsync), call
that once to load all matching Call objects, populate callCache from the
returned list, and then use callCache inside the existing foreach so you no
longer call GetCallByIdAsync for each log.
In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs`:
- Around line 923-934: Extract the duplicated hidden/disabled user check into a
single async helper (e.g., ShouldSkipHiddenOrDisabledMemberAsync) that accepts
the DepartmentMember, Department, and the target userId, performs the
IsDisabled/IsHidden checks, skips if the current user is a department admin
(Department.IsUserAnAdmin(UserId)), otherwise queries
_departmentGroupsService.GetGroupForUserAsync(targetUserId, DepartmentId) and
returns whether to skip based on IsUserGroupAdmin; then replace the repeated
blocks in Index(), GetPersonnelList(), and GetPersonnelListPaged() with a single
call: if (await ShouldSkipHiddenOrDisabledMemberAsync(member, department,
user.UserId)) continue; ensuring the helper is reused everywhere to maintain
consistent behavior.
In `@Web/Resgrid.Web/Areas/User/Controllers/SecurityController.cs`:
- Around line 166-171: Extract the repeated four-option permission list into a
single helper and replace the inline construction used for
model.DeleteLogPermissions (and the copies used for CreateLog, DeleteCall,
CloseCall, etc.) with that helper; add a private static method such as
BuildCrudPermissionOptions() that returns a SelectList of the four entries (Ids
3,0,1,2 with the corresponding Names) and then set model.DeleteLogPermissions =
BuildCrudPermissionOptions() (and update the other places to call this same
method) so label/scope changes are centralized.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js`:
- Around line 197-200: The fail handler currently hides the modal and alerts,
which discards user input; update the .fail(function () { ... }) block so it
does NOT call $('#checkInModal').modal('hide'), keeps the modal visible,
re-enables the submit control $('#checkInSubmitBtn').prop('disabled', false),
and surface the error in-place (e.g., show an inline error message element
inside the modal or update an existing status container) so users can correct or
retry without re-entering data.
🪄 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: 3d891b68-b860-43a9-bffb-42fbe969f71e
⛔ Files ignored due to path filters (2)
Core/Resgrid.Localization/Areas/User/Department/Department.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Department/Department.resxis excluded by!**/*.resx
📒 Files selected for processing (30)
Core/Resgrid.Model/AuditLogTypes.csCore/Resgrid.Model/PermissionTypes.csCore/Resgrid.Services/AuthorizationService.csProviders/Resgrid.Providers.Claims/ClaimsLogic.csProviders/Resgrid.Providers.Claims/ClaimsPrincipalFactory.csProviders/Resgrid.Providers.Claims/JwtTokenProvider.csProviders/Resgrid.Providers.Claims/ResgridIdentity.csProviders/Resgrid.Providers.Migrations/Migrations/M0060_FixCheckInTimerNullUniqueness.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0060_FixCheckInTimerNullUniquenessPg.csWeb/Resgrid.Web.Services/Controllers/v4/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/DepartmentController.csWeb/Resgrid.Web/Areas/User/Controllers/DocumentsController.csWeb/Resgrid.Web/Areas/User/Controllers/LogsController.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/ReportsController.csWeb/Resgrid.Web/Areas/User/Controllers/SecurityController.csWeb/Resgrid.Web/Areas/User/Models/Departments/DispatchSettingsView.csWeb/Resgrid.Web/Areas/User/Models/Logs/LogForListJson.csWeb/Resgrid.Web/Areas/User/Models/Security/PermissionsView.csWeb/Resgrid.Web/Areas/User/Views/Calendar/View.cshtmlWeb/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtmlWeb/Resgrid.Web/Areas/User/Views/Documents/EditDocument.cshtmlWeb/Resgrid.Web/Areas/User/Views/Documents/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Documents/NewDocument.cshtmlWeb/Resgrid.Web/Areas/User/Views/Logs/NewLog.cshtmlWeb/Resgrid.Web/Areas/User/Views/Personnel/AddPerson.cshtmlWeb/Resgrid.Web/Areas/User/Views/Security/Index.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.jsWeb/Resgrid.Web/wwwroot/js/app/internal/logs/resgrid.logs.index.jsWeb/Resgrid.Web/wwwroot/js/app/internal/security/resgrid.security.permissions.js
✅ Files skipped from review due to trivial changes (3)
- Web/Resgrid.Web/Areas/User/Models/Logs/LogForListJson.cs
- Core/Resgrid.Model/PermissionTypes.cs
- Core/Resgrid.Services/AuthorizationService.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Core/Resgrid.Model/AuditLogTypes.cs
- Web/Resgrid.Web/Areas/User/Controllers/ReportsController.cs
Comment on lines
+236
to
278
| public static void AddDeleteLogClaims(ClaimsIdentity identity, bool isAdmin, List<Permission> permissions, bool isGroupAdmin, List<PersonnelRole> roles) | ||
| { | ||
| if (permissions != null && permissions.Any(x => x.PermissionType == (int)PermissionTypes.DeleteLog)) | ||
| { | ||
| var permission = permissions.First(x => x.PermissionType == (int)PermissionTypes.DeleteLog); | ||
|
|
||
| if (permission.Action == (int)PermissionActions.DepartmentAdminsOnly && isAdmin) | ||
| { | ||
| identity.AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); | ||
| } | ||
| else if (permission.Action == (int)PermissionActions.DepartmentAndGroupAdmins && (isAdmin || isGroupAdmin)) | ||
| { | ||
| identity.AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); | ||
| } | ||
| else if (permission.Action == (int)PermissionActions.DepartmentAdminsAndSelectRoles && isAdmin) | ||
| { | ||
| identity.AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); | ||
| } | ||
| else if (permission.Action == (int)PermissionActions.DepartmentAdminsAndSelectRoles && !isAdmin) | ||
| { | ||
| if (!String.IsNullOrWhiteSpace(permission.Data)) | ||
| { | ||
| var roleIds = permission.Data.Split(char.Parse(",")).Select(int.Parse); | ||
| var role = from r in roles | ||
| where roleIds.Contains(r.PersonnelRoleId) | ||
| select r; | ||
|
|
||
| if (role.Any()) | ||
| { | ||
| identity.AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); | ||
| } | ||
| } | ||
| } | ||
| else if (permission.Action == (int)PermissionActions.Everyone) | ||
| { | ||
| identity.AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Default: everyone can delete logs | ||
| identity.AddClaim(new Claim(ResgridClaimTypes.Resources.Log, ResgridClaimTypes.Actions.Delete)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for DeleteLog definitions, migrations, and seeds..."
rg -n -C3 --glob '*.cs' --glob '*.sql' 'PermissionTypes\.(CreateLog|DeleteLog)|DeleteLog\s*= ?27'
echo
echo "Searching for permission bootstrap / backfill paths..."
rg -n -C3 --glob '*.cs' --glob '*.sql' 'SetPermissionForDepartmentAsync|PermissionTypes\.(CreateLog|DeleteLog)|CreateDepartment|PermissionType'Repository: Resgrid/Core
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching migration files for DeleteLog ====="
find . -type f \( -name "*.sql" -o -name "*Migration*.cs" \) | head -20
echo
echo "=== Checking SQL migrations for DeleteLog (27) ====="
fd -e sql -e cs | xargs grep -l "DeleteLog\|PermissionType.*27" 2>/dev/null | head -10
echo
echo "=== Looking for department creation/initialization code ====="
rg -n --glob '*.cs' 'CreateDepartmentAsync|InitializePermissions|SetPermission' | grep -v test | head -20
echo
echo "=== Checking Permissions table inserts in migrations ====="
rg -n --glob '*.sql' 'INSERT.*Permissions' -A 2 | head -40Repository: Resgrid/Core
Length of output: 3915
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== DepartmentsService.CreateDepartmentAsync implementation ====="
sed -n '187,250p' Core/Resgrid.Services/DepartmentsService.cs
echo
echo "=== PermissionsService initialization/defaults ====="
head -100 Core/Resgrid.Services/PermissionsService.cs
echo
echo "=== Search for initial permission seeding ====="
rg -n 'PermissionTypes\.CreateLog|PermissionTypes\.DeleteLog' Core/Resgrid.Services/PermissionsService.cs -A 5 -B 2 | head -60Repository: Resgrid/Core
Length of output: 5708
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Checking M0001_InitialMigration.sql for Permissions inserts ====="
grep -n "INSERT.*Permissions\|DELETE.*Permissions" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | head -20
echo
echo "=== Full Permissions table creation ====="
sed -n '/CREATE TABLE.*Permissions/,/^[[:space:]]*\);/p' Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql
echo
echo "=== Checking for any DeleteLog (27) permission inserts ====="
grep -r "PermissionType.*27\|DeleteLog" Providers/Resgrid.Providers.Migrations/Sql/ 2>/dev/null | head -10
echo
echo "=== List all migration files to check for backfill migrations ====="
ls -la Providers/Resgrid.Providers.Migrations/Sql/ | grep -i migrationRepository: Resgrid/Core
Length of output: 125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Reading M0001_InitialMigration.sql (Permissions table area) ====="
grep -n "Permissions" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | head -5
echo
echo "=== Checking if M0001 has any permission inserts at all ====="
wc -l Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql
echo
echo "=== Line count around Permissions table ====="
sed -n '1709,1730p' Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql
echo
echo "=== Checking DeleteService for permission-related logic ====="
grep -n "DeleteLog\|PermissionTypes\|CreateLog" Core/Resgrid.Services/DeleteService.cs | head -20Repository: Resgrid/Core
Length of output: 125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify migration files exist ====="
ls -la Providers/Resgrid.Providers.Migrations/Sql/M*.sql 2>/dev/null || echo "Not found in Migrations"
ls -la Providers/Resgrid.Providers.MigrationsPg/Sql/M*.sql 2>/dev/null || echo "Not found in MigrationsPg"
echo
echo "=== Check file size ====="
wc -l Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql 2>/dev/null || echo "File not found"
echo
echo "=== Check if Permissions mentioned anywhere in migrations ====="
find Providers -name "*.sql" -exec grep -l "Permissions" {} \;
echo
echo "=== Search C# code for default/initial permission creation ====="
rg -n 'new Permission|CreateDefaultPermissions|InitializePermissions' Core/Resgrid.Services/ --glob '*.cs' | head -15Repository: Resgrid/Core
Length of output: 860
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search M0001_InitialMigration.sql for Permission table (with context) ====="
grep -n -i "permission" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | head -20
echo
echo "=== Check first 100 lines of M0001 migration ====="
head -100 Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql
echo
echo "=== Search for INSERT statements in M0001 ====="
grep -n "^INSERT" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | wc -l
echo
echo "=== Check if M0001 creates the Permissions table at all ====="
grep -n "CREATE TABLE" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sqlRepository: Resgrid/Core
Length of output: 135
🏁 Script executed:
#!/bin/bash
set -euo pipefac
echo "=== File encoding check ====="
file Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql
echo
echo "=== Read first 200 lines with cat ====="
head -200 Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | tail -100
echo
echo "=== Search for CREATE TABLE statements ====="
grep -o "CREATE TABLE \[dbo\]\.\[[^]]*\]" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | sort | uniq
echo
echo "=== Check if [Permissions] table exists ====="
grep "\[Permissions\]" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | head -5Repository: Resgrid/Core
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== File encoding check ====="
file Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql
echo
echo "=== Check if [Permissions] table exists ====="
grep "\[Permissions\]" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | head -5
echo
echo "=== List all tables created in M0001 ====="
grep -o "CREATE TABLE \[dbo\]\.\[[^]]*\]" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | sort | uniq | head -30Repository: Resgrid/Core
Length of output: 147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if [Permissions] table exists ====="
grep "\[Permissions\]" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | head -5
echo
echo "=== Search for Permissions (case-insensitive) ====="
grep -i "permission" Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | head -10Repository: Resgrid/Core
Length of output: 101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check file content directly (first 500 lines) ====="
head -500 Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql | tail -200
echo
echo "=== Use Python to search for Permissions table ====="
python3 <<'EOF'
with open('Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql', 'r', encoding='utf-8', errors='ignore') as f:
content = f.read()
# Search for Permissions table definition
if 'Permissions' in content:
lines = content.split('\n')
for i, line in enumerate(lines):
if 'permission' in line.lower():
print(f"Line {i+1}: {line}")
if i < len(lines) - 5:
for j in range(1, 6):
print(f"Line {i+1+j}: {lines[i+j]}")
break
else:
print("No 'Permissions' found in file")
# Check total lines
print(f"\nTotal lines: {len(lines)}")
# Show some table names
import re
tables = re.findall(r'CREATE TABLE \[dbo\]\.\[([^\]]+)\]', content)
print(f"\nTables found: {len(tables)}")
for table in tables[:15]:
print(f" - {table}")
EOFRepository: Resgrid/Core
Length of output: 14096
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check PostgreSQL migration file for Permissions ====="
python3 <<'EOF'
import os
files_to_check = [
'Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql',
'Providers/Resgrid.Providers.MigrationsPg/Sql/M0001_InitialMigration.sql'
]
for fname in files_to_check:
if os.path.exists(fname):
try:
with open(fname, 'rb') as f:
content = f.read()
# Check encoding
try:
text = content.decode('utf-8')
if 'Permissions' in text or 'Permission' in text:
print(f"\n✓ Found 'Permissions' in {fname}")
# Find line numbers
lines = text.split('\n')
for i, line in enumerate(lines):
if 'permission' in line.lower():
print(f" Line {i+1}: {line[:100]}")
else:
print(f"✗ No 'Permissions' in {fname}")
except:
# Try UTF-16
text = content.decode('utf-16', errors='ignore')
if 'Permissions' in text or 'Permission' in text:
print(f"\n✓ Found 'Permissions' in {fname} (UTF-16)")
else:
print(f"✗ No 'Permissions' in {fname} (unusual encoding)")
except Exception as e:
print(f"Error reading {fname}: {e}")
EOF
echo
echo "=== Search C# code for permission initialization on department creation ====="
rg -n 'CreateDepartmentAsync|AddUserToDepartment' Core/Resgrid.Services/DepartmentsService.cs -A 20 | grep -i "permission\|setpermission" | head -10Repository: Resgrid/Core
Length of output: 410
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 <<'EOF'
# Read the UTF-16 encoded SQL file
with open('Providers/Resgrid.Providers.Migrations/Sql/M0001_InitialMigration.sql', 'rb') as f:
content = f.read().decode('utf-16', errors='ignore')
# Find Permissions table definition
lines = content.split('\n')
in_permissions = False
permission_lines = []
for i, line in enumerate(lines):
if 'Permission' in line and 'CREATE TABLE' in line:
in_permissions = True
start_idx = i
print(f"Found Permissions table at line {i+1}")
if in_permissions:
permission_lines.append((i+1, line.strip()))
# End of table definition
if ') ON [PRIMARY]' in line and i > start_idx + 10:
break
print("\n=== Permissions Table Definition ===")
for line_num, line in permission_lines[:30]:
print(f"{line_num}: {line[:120]}")
# Search for INSERT statements with PermissionType
print("\n=== INSERT statements for Permissions ===")
for i, line in enumerate(lines):
if 'INSERT' in line and 'Permission' in line:
print(f"\nLine {i+1}:")
# Print context
for j in range(max(0, i-2), min(len(lines), i+10)):
print(f" {j+1}: {lines[j][:100]}")
break
# Count total INSERTs
insert_count = sum(1 for line in lines if 'INSERT' in line)
print(f"\n=== Total INSERT statements: {insert_count} ===")
# Check if any DELETE Log (PermissionType 27) exists
has_delete_log = any('27' in line and 'PermissionType' in line for line in lines)
print(f"\nDeleteLog (PermissionType 27) found: {has_delete_log}")
EOFRepository: Resgrid/Core
Length of output: 822
Remove the fallback to Everyone when DeleteLog permission is missing.
The initial migration does not seed permission rows, and CreateDepartmentAsync does not create default permissions. This means any department created or migrated without an explicit DeleteLog permission row will fall through to the else clause at line 274, which grants Log/Delete to everyone—a silent privilege escalation from the legacy CreateLog model.
The same unsafe default occurs in SecurityController.cs lines 161–164. Since there is no migration backfilling DeleteLog from CreateLog, existing departments will unknowingly grant delete access to all members instead of preserving the original CreateLog restrictions.
Replace the else block with either:
- An explicit check for the legacy
CreateLogpermission and mirror its scope, or - Deny the claim entirely and require explicit
DeleteLogconfiguration before activation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Providers/Resgrid.Providers.Claims/ClaimsLogic.cs` around lines 236 - 278,
Remove the unsafe fallback in AddDeleteLogClaims that grants Log/Delete when the
DeleteLog permission is missing: instead, if permissions is null or no
PermissionTypes.DeleteLog exists, check for a legacy PermissionTypes.CreateLog
permission and, if present, apply the same PermissionActions mapping
(DepartmentAdminsOnly, DepartmentAndGroupAdmins, DepartmentAdminsAndSelectRoles,
Everyone) to determine whether to add the ResgridClaimTypes.Resources.Log +
Actions.Delete claim; if no legacy CreateLog mapping exists, do not add the
claim (deny by default). Update the analogous logic in SecurityController (the
same DeleteLog fallback) to follow this behavior so departments without explicit
DeleteLog remain restricted unless an explicit DeleteLog or legacy CreateLog
mapping grants it.
Providers/Resgrid.Providers.Migrations/Migrations/M0060_FixCheckInTimerNullUniqueness.cs
Show resolved
Hide resolved
Providers/Resgrid.Providers.MigrationsPg/Migrations/M0060_FixCheckInTimerNullUniquenessPg.cs
Show resolved
Hide resolved
Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js (3)
16-27:⚠️ Potential issue | 🟡 MinorAdd error handling for timer status fetch failures.
loadTimerStatusesonly handles success; failed requests are silent, leaving stale timer UI without feedback.Suggested fix
}).done(function (result) { if (result) { timerStatuses = result; renderTimerStatuses(); } + }).fail(function () { + console.error('Failed to load check-in timer statuses'); }); }🤖 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/dispatch/resgrid.dispatch.checkin-timers.js` around lines 16 - 27, The loadTimerStatuses function currently only handles successful AJAX responses; add failure handling for the GET to '/User/Dispatch/GetCheckInTimerStatuses' by attaching a .fail (or .catch) callback to the $.ajax promise that logs the error, sets a safe state for timerStatuses (or clears it) and calls renderTimerStatuses() or a dedicated UI error handler to display a user-facing message indicating timers could not be refreshed (include reference to callId in the log/message for context); ensure the success path still assigns timerStatuses and calls renderTimerStatuses() while the failure path provides feedback and avoids leaving stale UI.
108-118:⚠️ Potential issue | 🟡 MinorAdd error handling for check-in history fetch failures.
loadCheckInHistoryalso fails silently today; users get no indication that history did not load.Suggested fix
}).done(function (result) { if (result) { renderCheckInHistory(result); } + }).fail(function () { + console.error('Failed to load check-in history'); }); }🤖 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/dispatch/resgrid.dispatch.checkin-timers.js` around lines 108 - 118, loadCheckInHistory currently only handles successful responses and fails silently on errors; update the AJAX call in loadCheckInHistory to add error handling (use .fail or the error callback) and surface failures to the user and/or log them. Specifically, catch failed requests for resgrid.absoluteBaseUrl + '/User/Dispatch/GetCheckInHistory?callId=' + callId, call a user-facing notification (or fallback UI) and optionally log the error, and ensure renderCheckInHistory is only invoked on success; include the AJAX error object/message in the notification for debugging.
7-14:⚠️ Potential issue | 🟡 MinorClear
timerIntervalwhen the page/module is torn down.Line 11 starts a long-running interval, but there is no cleanup path. This can keep firing after navigation and leak work against detached DOM.
Suggested fix
$(document).ready(function () { // callState 0 = Active if (typeof callState !== 'undefined' && callState === 0) { loadTimerStatuses(); timerInterval = setInterval(updateCountdowns, 1000); } loadCheckInHistory(); + + $(window).on('beforeunload unload', function () { + if (timerInterval !== null) { + clearInterval(timerInterval); + timerInterval = null; + } + }); });🤖 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/dispatch/resgrid.dispatch.checkin-timers.js` around lines 7 - 14, The interval set in the document ready handler (timerInterval = setInterval(updateCountdowns, 1000)) is never cleared; add a teardown that calls clearInterval(timerInterval) (and set timerInterval = null) when the page/module is unloaded or torn down (e.g., window.beforeunload/window.unload or a module-specific destroy handler) and guard the clear with a typeof timerInterval !== 'undefined' check; update the code around the $(document).ready(...) block where loadTimerStatuses(), updateCountdowns, and loadCheckInHistory() are referenced to ensure the interval is stopped to avoid leaks.Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml (1)
172-172:⚠️ Potential issue | 🟠 MajorJavaScript-encode localized confirm text in inline handlers.
Lines 172 and 302 inject localized strings directly into single-quoted
confirm(...). Translations with quotes/backslashes can break script and create an injection surface.Suggested fix
-<button type="submit" class="btn btn-xs btn-danger" onclick="return confirm('@localizer["CheckInTimerDeleteConfirm"]');">@commonLocalizer["Delete"]</button> +<button type="submit" class="btn btn-xs btn-danger" onclick="return confirm('@Html.Raw(System.Text.Encodings.Web.JavaScriptEncoder.Default.Encode(localizer["CheckInTimerDeleteConfirm"].Value))');">@commonLocalizer["Delete"]</button>-<button type="submit" class="btn btn-xs btn-danger" onclick="return confirm('@localizer["CheckInTimerOverrideDeleteConfirm"]');">@commonLocalizer["Delete"]</button> +<button type="submit" class="btn btn-xs btn-danger" onclick="return confirm('@Html.Raw(System.Text.Encodings.Web.JavaScriptEncoder.Default.Encode(localizer["CheckInTimerOverrideDeleteConfirm"].Value))');">@commonLocalizer["Delete"]</button>Also applies to: 302-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml` at line 172, The inline confirm(...) handlers inject localized strings raw and can break/allow injection when translations contain quotes or backslashes; replace usages of '@localizer["CheckInTimerDeleteConfirm"]' (and the similar occurrence at the other button) with a JavaScript-encoded string expression, e.g. use Razor to emit a JSON-encoded literal like Html.Raw(Json.Encode(localizer["CheckInTimerDeleteConfirm"])) so the confirm() receives a safely escaped JS string; update both the button at the checked diff (onclick="return confirm(...)") and the duplicate at the other occurrence.
🧹 Nitpick comments (2)
Providers/Resgrid.Providers.Migrations/Migrations/M0060_FixCheckInTimerNullUniqueness.cs (1)
40-52: Sentinel value -1 should be safe, but verify domain constraints.Using
-1as the sentinel forISNULLcoalescing is a common pattern and should be safe assumingCallTypeId,CallPriority, andUnitTypeIdare all positive integers in practice. If any of these columns could legitimately hold-1(or if there's no domain constraint preventing it), two rows with-1would conflict with rows havingNULL.If there's any doubt, consider using a more extreme sentinel like
INT_MIN(-2147483648) or adding CHECK constraints to these columns ensuring values are non-negative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Migrations/Migrations/M0060_FixCheckInTimerNullUniqueness.cs` around lines 40 - 52, The migration adds persisted computed columns UnitTypeId_Unique, CallTypeId_Unique, and CallPriority_Unique using ISNULL(..., -1); ensure the sentinel value won't collide with valid data by either using a truly out-of-domain sentinel (e.g. INT_MIN) in the ALTER TABLE statements or by adding CHECK constraints on the underlying columns (CallTypeId, CallPriority, UnitTypeId) to guarantee they are non-negative; update M0060_FixCheckInTimerNullUniqueness.cs to replace -1 with the chosen sentinel or add matching CHECK constraints before adding the computed columns so uniqueness semantics are safe.Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml (1)
512-519: Add accessible names to the icon-only admin actions.The checkout and edit controls rely on
title, which is not a reliable accessible name. Addaria-labelor visually hidden text so keyboard and screen-reader users can tell the actions apart.Suggested fix
- <button type="submit" class="btn btn-xs btn-warning" title="@localizer["CheckOut"]"> + <button type="submit" class="btn btn-xs btn-warning" title="@localizer["CheckOut"]" aria-label="@localizer["CheckOut"]"> <i class="fa fa-sign-out"></i> </button> @@ - <a href="@Url.Action("EditCheckIn", "Calendar", new { area = "User", checkInId = ci.CalendarItemCheckInId })" - class="btn btn-xs btn-default" title="@localizer["EditCheckInTimes"]"> + <a href="@Url.Action("EditCheckIn", "Calendar", new { area = "User", checkInId = ci.CalendarItemCheckInId })" + class="btn btn-xs btn-default" title="@localizer["EditCheckInTimes"]" aria-label="@localizer["EditCheckInTimes"]"> <i class="fa fa-pencil"></i> </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/Calendar/View.cshtml` around lines 512 - 519, The icon-only checkout button and the EditCheckIn anchor rely solely on title attributes for identification; update the submit button (the one rendering <button type="submit" class="btn btn-xs btn-warning" title="@localizer["CheckOut"]"> with the fa-sign-out icon) and the EditCheckIn link (the anchor created via Url.Action("EditCheckIn", "Calendar", new { area = "User", checkInId = ci.CalendarItemCheckInId }) with the fa-pencil icon) to include accessible names by adding an aria-label attribute using the same localized strings (e.g. aria-label="@localizer["CheckOut"]" and aria-label="@localizer["EditCheckInTimes"]") or by inserting visually hidden text inside the elements so screen readers and keyboard users can distinguish the actions.
🤖 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/LogsController.cs`:
- Around line 487-498: The audit event is being sent with Successful = true
before the actual deletion; move the creation/sending of AuditEvent (AuditEvent,
_eventAggregator.SendMessage<AuditEvent>) to occur after awaiting
_workLogsService.DeleteLogAsync(logId, cancellationToken), or wrap the
DeleteLogAsync call in try/catch and set auditEvent.Successful based on whether
DeleteLogAsync completed without exception (and include exception details when
failed) before calling _eventAggregator.SendMessage<AuditEvent>; update
LogsController accordingly.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml`:
- Around line 125-131: The SignupSheet view is protected by Model.CanEdit but
the SignupSheet action in CalendarController does not enforce the same
permission, so update the CalendarController.SignupSheet action to perform the
identical editor permission check used by the view (i.e., verify the current
user is an editor for the department or has the same rights that Model.CanEdit
represents) and return an Unauthorized/Forbid or redirect when that check fails;
ensure the permission gating happens before loading/returning department
personnel names or the signup sheet data so a non-editor cannot access/print the
sheet by calling the URL directly.
- Around line 472-480: The view currently concatenates a localized label and a
hard-coded " by " fragment (using localizer["CheckIn"] + " by " + name), which
prevents proper localization and word-order changes; update the CheckIn and
CheckOut lines to use a single localized template that accepts the actor name as
an argument (e.g. replace the concatenation with calls like
localizer["CheckInBy", byName?.Name ?? ci.CheckInByUserId] and
localizer["CheckOutBy", byName?.Name ?? ci.CheckOutByUserId]) so translators can
control the full sentence; ensure the resource keys CheckInBy and CheckOutBy
contain a format placeholder (e.g. "{0} checked in") and keep the rest of the
logic (ci.CheckInByUserId, ci.CheckOutByUserId, Model.PersonnelNames) unchanged.
- Around line 405-407: The current branch that checks Model.UserCheckIn == null
&& (!checkInWindowOpen || eventIsPast) && !adminBypassWindow always shows
localizer["CheckInNotYetAvailable"], which is wrong when eventIsPast is true;
change the view logic to render a different localized message when eventIsPast
is true (e.g., localizer["CheckInClosed"] or
localizer["CheckInNoLongerAvailable"]) and keep
localizer["CheckInNotYetAvailable"] for the case where checkInWindowOpen is
false but eventIsPast is false; update or add the corresponding localization
keys if they don't exist and reference the same condition variables
(Model.UserCheckIn, checkInWindowOpen, eventIsPast, adminBypassWindow) when
making the conditional render.
- Around line 340-341: The view currently gates admin UI using Model.IsAdmin
which ignores whether the admin can act on a specific target attendee; fix by
performing the same per-target authorization used by
CanUserAdminCheckInCalendarEventAsync in the controller and pass that result to
the view (e.g., add a per-attendee boolean like Attendee.CanBeAdminCheckedIn or
a dictionary Model.CanAdminCheckInTarget[userId]); then update the view
conditions (where it currently checks Model.IsAdmin at signup lines and
action-column/attendee-button locations) to use the per-attendee flag or
pre-filter the attendee list to only include attendees within the admin's scope
before rendering admin buttons.
In `@Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml`:
- Around line 413-425: The loadStates function lacks an error path for the
statesUrl request; update loadStates to use $.ajax or attach a .fail() handler
to the $.getJSON call so failures are handled: on error, restore a safe state
for $statesSelect (e.g., add a single disabled option like "Error loading
states" or re-populate with a cached/default option), trigger the select2 change
event ($statesSelect.trigger('change.select2')), and surface the error to the
user or log it (use existing UI notification or console). Ensure you reference
the existing symbols loadStates, statesUrl and $statesSelect and do not leave
the select empty on request failure.
---
Duplicate comments:
In `@Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml`:
- Line 172: The inline confirm(...) handlers inject localized strings raw and
can break/allow injection when translations contain quotes or backslashes;
replace usages of '@localizer["CheckInTimerDeleteConfirm"]' (and the similar
occurrence at the other button) with a JavaScript-encoded string expression,
e.g. use Razor to emit a JSON-encoded literal like
Html.Raw(Json.Encode(localizer["CheckInTimerDeleteConfirm"])) so the confirm()
receives a safely escaped JS string; update both the button at the checked diff
(onclick="return confirm(...)") and the duplicate at the other occurrence.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js`:
- Around line 16-27: The loadTimerStatuses function currently only handles
successful AJAX responses; add failure handling for the GET to
'/User/Dispatch/GetCheckInTimerStatuses' by attaching a .fail (or .catch)
callback to the $.ajax promise that logs the error, sets a safe state for
timerStatuses (or clears it) and calls renderTimerStatuses() or a dedicated UI
error handler to display a user-facing message indicating timers could not be
refreshed (include reference to callId in the log/message for context); ensure
the success path still assigns timerStatuses and calls renderTimerStatuses()
while the failure path provides feedback and avoids leaving stale UI.
- Around line 108-118: loadCheckInHistory currently only handles successful
responses and fails silently on errors; update the AJAX call in
loadCheckInHistory to add error handling (use .fail or the error callback) and
surface failures to the user and/or log them. Specifically, catch failed
requests for resgrid.absoluteBaseUrl +
'/User/Dispatch/GetCheckInHistory?callId=' + callId, call a user-facing
notification (or fallback UI) and optionally log the error, and ensure
renderCheckInHistory is only invoked on success; include the AJAX error
object/message in the notification for debugging.
- Around line 7-14: The interval set in the document ready handler
(timerInterval = setInterval(updateCountdowns, 1000)) is never cleared; add a
teardown that calls clearInterval(timerInterval) (and set timerInterval = null)
when the page/module is unloaded or torn down (e.g.,
window.beforeunload/window.unload or a module-specific destroy handler) and
guard the clear with a typeof timerInterval !== 'undefined' check; update the
code around the $(document).ready(...) block where loadTimerStatuses(),
updateCountdowns, and loadCheckInHistory() are referenced to ensure the interval
is stopped to avoid leaks.
---
Nitpick comments:
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0060_FixCheckInTimerNullUniqueness.cs`:
- Around line 40-52: The migration adds persisted computed columns
UnitTypeId_Unique, CallTypeId_Unique, and CallPriority_Unique using ISNULL(...,
-1); ensure the sentinel value won't collide with valid data by either using a
truly out-of-domain sentinel (e.g. INT_MIN) in the ALTER TABLE statements or by
adding CHECK constraints on the underlying columns (CallTypeId, CallPriority,
UnitTypeId) to guarantee they are non-negative; update
M0060_FixCheckInTimerNullUniqueness.cs to replace -1 with the chosen sentinel or
add matching CHECK constraints before adding the computed columns so uniqueness
semantics are safe.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml`:
- Around line 512-519: The icon-only checkout button and the EditCheckIn anchor
rely solely on title attributes for identification; update the submit button
(the one rendering <button type="submit" class="btn btn-xs btn-warning"
title="@localizer["CheckOut"]"> with the fa-sign-out icon) and the EditCheckIn
link (the anchor created via Url.Action("EditCheckIn", "Calendar", new { area =
"User", checkInId = ci.CalendarItemCheckInId }) with the fa-pencil icon) to
include accessible names by adding an aria-label attribute using the same
localized strings (e.g. aria-label="@localizer["CheckOut"]" and
aria-label="@localizer["EditCheckInTimes"]") or by inserting visually hidden
text inside the elements so screen readers and keyboard users can distinguish
the actions.
🪄 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: 69f7945a-ce0c-477e-b177-7a34e795df98
⛔ Files ignored due to path filters (1)
Core/Resgrid.Localization/Areas/User/Security/Security.en.resxis excluded by!**/*.resx
📒 Files selected for processing (8)
Providers/Resgrid.Providers.Migrations/Migrations/M0060_FixCheckInTimerNullUniqueness.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0060_FixCheckInTimerNullUniquenessPg.csWeb/Resgrid.Web/Areas/User/Controllers/LogsController.csWeb/Resgrid.Web/Areas/User/Views/Calendar/View.cshtmlWeb/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtmlWeb/Resgrid.Web/Areas/User/Views/Documents/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Security/Index.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.js
🚧 Files skipped from review as they are similar to previous changes (3)
- Web/Resgrid.Web/Areas/User/Views/Documents/Index.cshtml
- Providers/Resgrid.Providers.MigrationsPg/Migrations/M0060_FixCheckInTimerNullUniquenessPg.cs
- Web/Resgrid.Web/Areas/User/Views/Security/Index.cshtml
Comment on lines
+340
to
+341
| @* Signup form — admins/creators can sign up people even after the event has passed *@ | ||
| @if (Model.CalendarItem.SignupType != 0 && !Model.CalendarItem.IsUserAttending(Model.UserId) && (Model.CalendarItem.Start >= DateTime.UtcNow || Model.IsAdmin)) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '\nAssignments to CalendarItemView.CanEdit / IsAdmin:\n'
rg -n -C3 '\b(CanEdit|IsAdmin)\s*=' Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs
printf '\nCalendarItemView flags:\n'
sed -n '1,40p' Web/Resgrid.Web/Areas/User/Models/Calendar/CalendarItemView.cs
printf '\nAdmin check-in/out actions and their authorization calls:\n'
rg -n -C4 --type=cs 'AdminCheck(In|Out)\(|CanUserAdminCheck(In|Out)CalendarEventAsync'Repository: Resgrid/Core
Length of output: 11032
🏁 Script executed:
#!/bin/bash
set -e
printf '\nFull CanUserAdminCheckInCalendarEventAsync implementation:\n'
sed -n '1567,1610p' Core/Resgrid.Services/AuthorizationService.cs
printf '\n\nView code at lines 375-380:\n'
sed -n '375,380p' Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml
printf '\n\nView code at lines 459-460:\n'
sed -n '459,460p' Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml
printf '\n\nView code at lines 534-535:\n'
sed -n '534,535p' Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtmlRepository: Resgrid/Core
Length of output: 2519
🏁 Script executed:
#!/bin/bash
set -e
printf '\nContext around lines 459-460 (20 lines before and after):\n'
sed -n '440,480p' Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml
printf '\n\nContext around lines 534-535 (20 lines before and after):\n'
sed -n '515,555p' Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtml
printf '\n\nContext around lines 375-380 (15 lines before and after):\n'
sed -n '360,395p' Web/Resgrid.Web/Areas/User/Views/Calendar/View.cshtmlRepository: Resgrid/Core
Length of output: 7591
Verify that Model.IsAdmin accounts for target-user scope in admin check-in UI gates.
The view gates admin check-in UI (admin check-in form, action column) based solely on whether the current user is a department admin, event creator, or group admin. However, the backend authorization (CanUserAdminCheckInCalendarEventAsync) additionally validates that the target user is within the admin's scope:
- Department admins can check in users in their department
- Event creators can check in users in their department
- Group admins can only check in users in their group or child groups
This creates a UX issue for group admins: they will see admin check-in buttons for all event attendees (line 534-535) and admin action columns (line 459-460), but the backend will reject attempts to check in attendees outside their group. The same issue applies to the admin bypass window for post-event check-in (line 375-380) if multiple group admins with different scopes view the same event.
Lines 340-341 (late signup) and self check-in bypass (line 375-380) are less problematic since they apply to the current user, not target users.
Consider filtering the attendee list by scope before rendering admin buttons, or pass additional authorization context to the view for per-attendee validation.
Also applies to: 375-380, 459-460, 534-535
🤖 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/View.cshtml` around lines 340 -
341, The view currently gates admin UI using Model.IsAdmin which ignores whether
the admin can act on a specific target attendee; fix by performing the same
per-target authorization used by CanUserAdminCheckInCalendarEventAsync in the
controller and pass that result to the view (e.g., add a per-attendee boolean
like Attendee.CanBeAdminCheckedIn or a dictionary
Model.CanAdminCheckInTarget[userId]); then update the view conditions (where it
currently checks Model.IsAdmin at signup lines and action-column/attendee-button
locations) to use the per-attendee flag or pre-filter the attendee list to only
include attendees within the admin's scope before rendering admin buttons.
Comment on lines
+413
to
+425
| function loadStates($statesSelect, timerTargetType, unitTypeId) { | ||
| var params = { timerTargetType: timerTargetType }; | ||
| if (unitTypeId) params.unitTypeId = unitTypeId; | ||
|
|
||
| $statesSelect.val(null).trigger('change').empty(); | ||
|
|
||
| $.getJSON(statesUrl, params, function (data) { | ||
| $.each(data, function (i, item) { | ||
| $statesSelect.append(new Option(item.text, item.id, false, false)); | ||
| }); | ||
| $statesSelect.trigger('change.select2'); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle active-state lookup failures explicitly.
loadStates has no failure path, so state selectors can silently stay empty when the endpoint fails, and submits may unintentionally persist “all states”.
Suggested fix
-$.getJSON(statesUrl, params, function (data) {
- $.each(data, function (i, item) {
- $statesSelect.append(new Option(item.text, item.id, false, false));
- });
- $statesSelect.trigger('change.select2');
-});
+$.getJSON(statesUrl, params, function (data) {
+ $.each(data, function (i, item) {
+ $statesSelect.append(new Option(item.text, item.id, false, false));
+ });
+ $statesSelect.trigger('change.select2');
+}).fail(function () {
+ console.error('Failed to load active states');
+ $statesSelect.val(null).trigger('change').empty();
+});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Department/DispatchSettings.cshtml` around
lines 413 - 425, The loadStates function lacks an error path for the statesUrl
request; update loadStates to use $.ajax or attach a .fail() handler to the
$.getJSON call so failures are handled: on error, restore a safe state for
$statesSelect (e.g., add a single disabled option like "Error loading states" or
re-populate with a cached/default option), trigger the select2 change event
($statesSelect.trigger('change.select2')), and surface the error to the user or
log it (use existing UI notification or console). Ensure you reference the
existing symbols loadStates, statesUrl and $statesSelect and do not leave the
select empty on request failure.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit