Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
|
Too much diff to scan? Review this PR in Change Stack to start with the highest-impact changes. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR introduces a complete platform reporting and analytics system with pre-aggregated daily rollups, multiple dashboard and operational metrics (response time, utilization, participation), department-scoped HTTP APIs, incident CSV export in NFIRS/NEMSIS formats, and automated nightly rollup scheduling. The implementation spans data models, repository and service layers, SQL query builders, migrations, and worker integration. ChangesPlatform Reporting & Analytics Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: 17
🧹 Nitpick comments (8)
Web/Resgrid.Web.Services/Controllers/v4/ReportingController.cs (1)
161-162: ⚖️ Poor tradeoffTreating DateTimeKind.Unspecified as UTC may be unexpected.
The
ToUtchelper treatsDateTimeKind.Unspecifiedvalues as UTC rather than converting them. If a client sends a local DateTime without specifying the kind, it will be incorrectly interpreted as UTC, causing timezone offset errors.While the API documentation specifies UTC parameters, this silent conversion could lead to subtle bugs. Consider either:
- Returning a 400 Bad Request when
Kind == Unspecifiedto force clients to specify UTC explicitly- Adding a warning log when this occurs for debugging
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Controllers/v4/ReportingController.cs` around lines 161 - 162, The helper ToUtc currently treats DateTimeKind.Unspecified as UTC which can misinterpret local timestamps; change the behavior so unspecified kinds are rejected instead of silently converted: modify ToUtc (and callers in ReportingController) to detect DateTimeKind.Unspecified and surface an explicit failure (e.g., throw ArgumentException or return a validation result) and update the controller action(s) to return a 400 Bad Request with a clear message when ToUtc reports unspecified kind; alternatively, if you prefer non-failing behavior, add a warning log in ToUtc (or in ReportingController) when DateTimeKind.Unspecified is seen so callers can diagnose incorrect client usage.Core/Resgrid.Services/PlatformReportingService.cs (1)
1-465: ⚡ Quick winConsider adding logging for observability and debugging.
The service has no logging despite performing critical reporting and analytics operations. Adding structured logging for key operations, errors, and performance metrics would improve observability and debugging capabilities.
Consider logging:
- Report generation start/completion with timing metrics
- Cache hits/misses for analytics
- Repository query failures
- Export generation events
As per coding guidelines: "Use
Resgrid.Framework.Loggingstatic methods (LogException,LogError,LogInfo,LogDebug) for all logging throughout the codebase"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/PlatformReportingService.cs` around lines 1 - 465, Add structured logging calls (Resgrid.Framework.Logging.LogInfo/LogDebug for start/stop/timings, LogError/LogException on failures) around major report flows: at the start and end (with elapsed time) of GetDashboardReportAsync's buildReport local function, and similarly inside the build lambdas passed to CachedAsync for GetResponseTimeReportAsync, GetUtilizationReportAsync, and GetParticipationReportAsync; log cache keys and hits/misses inside CachedAsync (use the cacheKey variable), and log ExportIncidentsCsvAsync export start/completion and thrown NotSupportedException or call/service failures; also wrap repository and rollup calls (e.g., _reportingRepository.* and _rollupRepository.GetRollupsAsync) with try/catch to LogException and rethrow so errors are observable.Repositories/Resgrid.Repositories.DataRepository/ReportingRollupRepository.cs (1)
43-49: ⚡ Quick winDirect mutation of input collection items violates functional programming guidelines.
The method modifies each
ReportingDailyRollupinstance in the caller's collection by settingDepartmentId,BucketDateUtc, andCreatedOnUtc. This violates the coding guideline to "prefer pure methods over methods with side effects" and may surprise callers who don't expect their input to be modified.As per coding guidelines: Prefer functional patterns and immutable data where appropriate in C# and Prefer pure methods over methods with side effects.
♻️ Proposed refactor to eliminate mutation
var rowList = (rows ?? Enumerable.Empty<ReportingDailyRollup>()).ToList(); var now = DateTime.UtcNow; -foreach (var row in rowList) -{ - row.DepartmentId = departmentId; - row.BucketDateUtc = bucketDateUtc; - if (row.CreatedOnUtc == default) - row.CreatedOnUtc = now; -} +var normalizedRows = rowList.Select(row => new ReportingDailyRollup +{ + DepartmentId = departmentId, + BucketDateUtc = bucketDateUtc, + Metric = row.Metric, + Dimension = row.Dimension, + ItemCount = row.ItemCount, + SumValue = row.SumValue, + MinValue = row.MinValue, + MaxValue = row.MaxValue, + P50 = row.P50, + P90 = row.P90, + CreatedOnUtc = row.CreatedOnUtc == default ? now : row.CreatedOnUtc +}).ToList();Then use
normalizedRowsinstead ofrowListin the insert operation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Repositories/Resgrid.Repositories.DataRepository/ReportingRollupRepository.cs` around lines 43 - 49, The code mutates items from the caller by updating ReportingDailyRollup instances in rowList (setting DepartmentId, BucketDateUtc, CreatedOnUtc); instead, create a new collection (e.g., normalizedRows) by projecting each item into a new ReportingDailyRollup (copying existing properties and setting DepartmentId, BucketDateUtc, and CreatedOnUtc when default) and use normalizedRows for the subsequent insert, leaving the original rowList unmodified.Repositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportNewDepartmentsQuery.cs (1)
26-29: 💤 Low valueConsider adding a descriptive message to NotImplementedException.
The generic
GetQuery<TEntity>()overload is part of theISelectQuerycontract but is not used by aggregate reporting queries. Adding a message like"Entity-based query overload not supported for aggregate reporting queries"would make the intent clearer for future maintainers.This comment applies to all query builders in this cohort that share this pattern.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Repositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportNewDepartmentsQuery.cs` around lines 26 - 29, The GetQuery<TEntity>() generic overload currently throws a bare System.NotImplementedException which is ambiguous; update the throw in GetQuery<TEntity>() to include a descriptive message such as "Entity-based query overload not supported for aggregate reporting queries" (or similar) so maintainers understand this overload is intentionally unsupported for classes like SelectReportNewDepartmentsQuery and other query builders in this cohort.Core/Resgrid.Model/Repositories/IReportingRepository.cs (3)
25-25: ⚡ Quick winAdd XML documentation for clarity.
This method lacks an XML doc comment describing its behavior, unlike neighboring methods. Consider adding a summary for consistency and maintainability.
📝 Suggested documentation
+ /// <summary>Count of messages sent within the specified UTC window for the scope.</summary> Task<long> GetMessagesCountAsync(int? departmentId, DateTime startUtc, DateTime endUtc, CancellationToken cancellationToken = default);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Model/Repositories/IReportingRepository.cs` at line 25, Add an XML doc comment for the GetMessagesCountAsync method to match neighboring methods: include a <summary> describing that it returns the count of messages for an optional department within a UTC date range, add <param> tags for departmentId, startUtc, endUtc and cancellationToken describing their purpose, and a <returns> describing that it returns the message count as a long; place the comment immediately above the Task<long> GetMessagesCountAsync(...) declaration in IReportingRepository.
27-27: ⚡ Quick winAdd XML documentation for clarity.
This method lacks an XML doc comment describing its behavior. Consider adding a summary for consistency.
📝 Suggested documentation
+ /// <summary>Count of new user registrations within the specified UTC window for the scope.</summary> Task<long> GetNewUsersCountAsync(int? departmentId, DateTime startUtc, DateTime endUtc, CancellationToken cancellationToken = default);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Model/Repositories/IReportingRepository.cs` at line 27, Add an XML doc comment above the interface method GetNewUsersCountAsync in IReportingRepository describing what the method does (returns count of new users within a date range, optionally filtered by department), document each parameter (departmentId, startUtc, endUtc, cancellationToken) including that dates are UTC, and describe the return value (Task<long> representing the count of new users); keep wording concise and consistent with other repository method docs.
32-32: ⚡ Quick winAdd XML documentation for clarity.
This method lacks an XML doc comment. Consider adding a summary describing what "units" means in the Resgrid domain.
📝 Suggested documentation
+ /// <summary>Count of active units (apparatus) for the scope (excludes deleted).</summary> Task<long> GetUnitsCountAsync(int? departmentId, CancellationToken cancellationToken = default);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Model/Repositories/IReportingRepository.cs` at line 32, Add an XML doc comment to the IReportingRepository.GetUnitsCountAsync method explaining what "units" refers to in the Resgrid domain, and document the parameters and return value; specifically add a <summary> describing "units" (e.g., personnel/vehicles/assigned resources as applicable), <param name="departmentId"> explaining nullable behavior/scoping, <param name="cancellationToken">, and <returns> describing that it returns the total count as a long wrapped in a Task. Ensure the comment is placed directly above the Task<long> GetUnitsCountAsync(int? departmentId, CancellationToken cancellationToken = default); declaration in IReportingRepository.Core/Resgrid.Services/ReportingRollupProcessor.cs (1)
127-163: ⚖️ Poor tradeoffConsider batching unit state queries for departments with many units.
The method executes one
GetAllStatesForUnitInDateRangeAsyncquery per unit (line 142), which could result in N+1 query performance for departments with many units. Since this runs as a nightly batch job, the impact may be acceptable, but consider adding a batch query method likeGetAllStatesForDepartmentInDateRangeAsyncto reduce round trips.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/ReportingRollupProcessor.cs` around lines 127 - 163, ComputeUtilizationRollupsAsync currently issues one GetAllStatesForUnitInDateRangeAsync per unit causing N+1 queries; instead add or call a batch method GetAllStatesForDepartmentInDateRangeAsync on _unitStatesService to fetch all unit states for the department in the date range once, then group the returned states by UnitId and iterate groups to build the ordered list (use the same .OrderBy(s => s.Timestamp).Select(...) pattern and call ReportingMath.UtilizationSeconds per group), keeping the existing logic that skips empty totals and accumulates sumUhu/unitCount before adding the ReportingDailyRollup; update usages of unit.UnitId to group keys and remove the per-unit await inside the foreach.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Model/Services/IPlatformReportingService.cs`:
- Line 54: The interface method GetUnmappedRequiredExportFields currently
returns IReadOnlyList<string> synchronously; update
IPlatformReportingService.GetUnmappedRequiredExportFields to return
Task<IReadOnlyList<string>> and make the method async-compatible, then update
all implementing classes (methods named GetUnmappedRequiredExportFields) to
return Task<IReadOnlyList<string>> (wrapping the current result with
Task.FromResult(...) if no I/O) and update all callers to await the call; ensure
signatures, usings and any unit tests are adjusted accordingly to preserve
behavior while complying with the async service guideline.
In `@Core/Resgrid.Model/Services/IReportingRollupProcessor.cs`:
- Line 14: Rename the service interface IReportingRollupProcessor to follow the
I{Name}Service convention (suggested: IReportingRollupService), update the
interface declaration name and any implementing classes (e.g., classes
implementing IReportingRollupProcessor) to implement IReportingRollupService,
update all references/usages and dependency injection registrations to the new
interface name (including constructor injections and service registrations), and
keep the existing namespace and method signatures unchanged so only the type
name changes.
In `@Core/Resgrid.Services/PlatformReportingService.cs`:
- Around line 455-456: state.GetActiveDetails() may be null causing the foreach
to throw; guard the enumeration by capturing its result into a local (e.g., var
details = state.GetActiveDetails()) and only iterate if details is not null (or
use a null-safe enumerator), then populate map[detail.CustomStateDetailId] =
detail; referencing state.GetActiveDetails(), detail.CustomStateDetailId and map
to locate the code.
- Around line 39-47: Change the PlatformReportingService constructor to use the
service locator: remove constructor parameters for IReportingRepository,
IReportingRollupRepository, ICacheProvider, ICustomStateService, and
ICallsService and instead resolve those dependencies inside the constructor
using Bootstrapper.GetKernel().Resolve<T>() for each dependency, assigning the
results to the existing fields (_reportingRepository, _rollupRepository,
_cacheProvider, _customStateService, _callsService); ensure the class still
initializes all five fields and that any callers expecting the old constructor
are updated to use the parameterless constructor.
In `@Core/Resgrid.Services/ReportingRollupProcessor.cs`:
- Around line 29-37: The constructor of ReportingRollupProcessor currently takes
five services via constructor injection (_callsService, _rollupRepository,
_unitsService, _unitStatesService, _customStateService); change it to use the
Service Locator by removing those parameters and resolving each inside the
constructor body with Bootstrapper.GetKernel().Resolve<T>() for the
corresponding types, assigning the results to the existing fields (e.g.,
_callsService, _rollupRepository, _unitsService, _unitStatesService,
_customStateService) so dependencies are obtained explicitly in the
ReportingRollupProcessor constructor.
- Around line 58-89: Wrap the per-department work in RunDailyRollupForAllAsync
(the body that calls _callsService.GetAllCallsByDepartmentDateRangeAsync,
BuildCallRollups, ComputeUtilizationRollupsAsync and
_rollupRepository.UpsertDailyRollupAsync) in a try-catch so one department
failure doesn't abort the whole loop; in the catch log the departmentId and
exception (using your existing logger) and continue to the next department,
optionally tracking a failure count, and ensure the system-wide aggregate and
final UpsertDailyRollupAsync(null, ...) still run after the loop.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0073_AddingReportingIndexesPg.cs`:
- Around line 16-17: Replace all culture-sensitive ToLower() calls used to
generate PostgreSQL identifiers in M0073_AddingReportingIndexesPg (e.g., the
Create.Table("ReportingDailyRollup".ToLower()), column calls like
"ReportingDailyRollupId".ToLower(), and any index names such as
"IX_...".ToLower()) with ToLowerInvariant() so identifiers are invariant; and
ensure the citext extension is created before any AsCustom("citext") usage by
adding a statement to run CREATE EXTENSION IF NOT EXISTS citext (or the
FluentMigrator equivalent) at the start of the migration (before Create.Table
and any Create.Index calls).
In
`@Repositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportCallsByDayQuery.cs`:
- Around line 26-29: The generic overload GetQuery<TEntity>() in
SelectReportCallsByDayQuery currently throws NotImplementedException; replace
the throw with an implementation that delegates to the non-generic GetQuery()
(i.e., have GetQuery<TEntity>() return GetQuery()) so generic callers use the
same SQL string and avoid runtime crashes; update the method body in
SelectReportCallsByDayQuery to return the result of GetQuery() instead of
throwing.
In `@Repositories/Resgrid.Repositories.DataRepository/ReportingRepository.cs`:
- Around line 41-60: GetCallsCountAsync (and all other repository methods making
Dapper calls) accept a CancellationToken but never pass it into Dapper; update
each ExecuteScalarAsync/QueryAsync call to use a CommandDefinition that includes
the cancellationToken. For example, for GetCallsCountAsync: build a
CommandDefinition with commandText = the query from
_queryFactory.GetQuery<...>(), parameters = p (from ScopeParams), transaction =
_unitOfWork.Transaction and cancellationToken = cancellationToken, then call
conn.ExecuteScalarAsync<long>(cmd). Apply the same pattern to every other
ExecuteScalarAsync/QueryAsync invocation in this file so callers can cancel
in-flight DB operations.
In
`@Repositories/Resgrid.Repositories.DataRepository/ReportingRollupRepository.cs`:
- Around line 38-69: In UpsertDailyRollupAsync (and the other method with
QueryAsync), propagate the incoming CancellationToken into all Dapper calls by
creating and passing a CommandDefinition that includes commandText (e.g.,
deleteQuery / insertQuery / the query from _queryFactory.GetQuery<...>()),
parameters (deleteParams / rowList), transaction (_unitOfWork.Transaction) and
cancellationToken, and use conn.ExecuteAsync(cmd) / conn.QueryAsync(cmd) instead
of the current overloads; update both ExecuteAsync invocations in RunAsync and
any QueryAsync calls so callers can cancel in-flight DB operations.
In
`@Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.cs`:
- Around line 1187-1188: SelectReportLatestPersonnelStatesQuery is including
members with ActionLog entries even if they're deleted/disabled, causing state
buckets to exceed the personnel count; update the query to apply the same
active-member predicates used in SelectReportPersonnelCountQuery (the
"(IsDeleted = false AND (IsDisabled IS NULL OR IsDisabled = false))" condition
along with the department restriction "(%ALLDEPTS% = 1 OR DepartmentId =
%DID%)") so both queries count the exact same population; make the identical
change in SqlServerConfiguration as well to keep PostgreSqlConfiguration and
SqlServerConfiguration consistent.
In
`@Repositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.cs`:
- Around line 1168-1173: The report SQL in SqlServerConfiguration (symbols
SelectReportMessagesCountQuery, SelectReportMessagesByDayQuery,
SelectReportMessagesByMonthQuery) infers department via DepartmentMembers (dm)
and SendingUserId which double-counts messages for users in multiple
departments; change the queries to filter/group by a department identifier that
is stored on the message record (e.g., m.DepartmentId or a message->department
mapping table created at write time) instead of joining to %MEMBERSTABLE% on
SendingUserId, and apply the same fix to the corresponding queries in
PostgreSqlConfiguration so department rollups use the message's explicit
department association.
In `@Repositories/Resgrid.Repositories.DataRepository/SystemAuditRepository.cs`:
- Around line 42-43: Clamp both page and pageSize before constructing SQL
parameters in SystemAuditRepository.cs: ensure page is at least 1 and pageSize
is at least 1 (or a configured minimum) then compute Offset and add PageSize
using the clamped values; update the code paths that call
dynamicParameters.Add("Offset", Math.Max(0, (page - 1) * pageSize)) and
dynamicParameters.Add("PageSize", pageSize) (and the same pair around lines
87-88) to use the clamped variables so zero or negative inputs cannot be passed
to the SQL layer.
In `@Web/Resgrid.Web.Services/Controllers/v4/ReportingController.cs`:
- Around line 35-38: The constructor currently uses constructor injection for
IPlatformReportingService in ReportingController; change it to resolve the
dependency via the service locator by calling
Bootstrapper.GetKernel().Resolve<IPlatformReportingService>() and assign the
result to the _reportingService field inside the constructor (replace the
injected parameter), ensuring ReportingController, _reportingService and the
call to Bootstrapper.GetKernel().Resolve<IPlatformReportingService>() are used
to locate and set the dependency.
In `@Workers/Resgrid.Workers.Console/Commands/ReportingRollupCommand.cs`:
- Line 11: The Metadata property on ReportingRollupCommand is null by default
and can cause null dereferences; initialize it to an empty Dictionary to ensure
safe writes. Update the ReportingRollupCommand class to set Metadata = new
Dictionary<string,string>() either inline on the property (public
Dictionary<string,string> Metadata { get; set; } = new
Dictionary<string,string>();) or in the ReportingRollupCommand constructor, so
downstream command handlers can add entries without null checks.
In `@Workers/Resgrid.Workers.Console/Program.cs`:
- Line 424: Replace the direct use of _logger.Log(LogLevel.Information,
"Scheduling Reporting Rollup") with the repository standard
Resgrid.Framework.Logging.LogInfo call; locate the call to _logger.Log in
Program.Main (or the scheduling setup) and change it to call
Resgrid.Framework.Logging.LogInfo with the same "Scheduling Reporting Rollup"
message so the project uses the static logging helper instead of ILogger.Log.
In `@Workers/Resgrid.Workers.Console/Tasks/ReportingRollupTask.cs`:
- Line 25: The field `_logger` in class ReportingRollupTask is declared public
but uses underscore naming; change its access modifier to private (preferably
private readonly if it's only set in the constructor) to restore encapsulation
and follow C# conventions; update any external references (if any) to use a
property or constructor-injected ILogger instead and ensure the constructor
still assigns the logger to `_logger`.
---
Nitpick comments:
In `@Core/Resgrid.Model/Repositories/IReportingRepository.cs`:
- Line 25: Add an XML doc comment for the GetMessagesCountAsync method to match
neighboring methods: include a <summary> describing that it returns the count of
messages for an optional department within a UTC date range, add <param> tags
for departmentId, startUtc, endUtc and cancellationToken describing their
purpose, and a <returns> describing that it returns the message count as a long;
place the comment immediately above the Task<long> GetMessagesCountAsync(...)
declaration in IReportingRepository.
- Line 27: Add an XML doc comment above the interface method
GetNewUsersCountAsync in IReportingRepository describing what the method does
(returns count of new users within a date range, optionally filtered by
department), document each parameter (departmentId, startUtc, endUtc,
cancellationToken) including that dates are UTC, and describe the return value
(Task<long> representing the count of new users); keep wording concise and
consistent with other repository method docs.
- Line 32: Add an XML doc comment to the IReportingRepository.GetUnitsCountAsync
method explaining what "units" refers to in the Resgrid domain, and document the
parameters and return value; specifically add a <summary> describing "units"
(e.g., personnel/vehicles/assigned resources as applicable), <param
name="departmentId"> explaining nullable behavior/scoping, <param
name="cancellationToken">, and <returns> describing that it returns the total
count as a long wrapped in a Task. Ensure the comment is placed directly above
the Task<long> GetUnitsCountAsync(int? departmentId, CancellationToken
cancellationToken = default); declaration in IReportingRepository.
In `@Core/Resgrid.Services/PlatformReportingService.cs`:
- Around line 1-465: Add structured logging calls
(Resgrid.Framework.Logging.LogInfo/LogDebug for start/stop/timings,
LogError/LogException on failures) around major report flows: at the start and
end (with elapsed time) of GetDashboardReportAsync's buildReport local function,
and similarly inside the build lambdas passed to CachedAsync for
GetResponseTimeReportAsync, GetUtilizationReportAsync, and
GetParticipationReportAsync; log cache keys and hits/misses inside CachedAsync
(use the cacheKey variable), and log ExportIncidentsCsvAsync export
start/completion and thrown NotSupportedException or call/service failures; also
wrap repository and rollup calls (e.g., _reportingRepository.* and
_rollupRepository.GetRollupsAsync) with try/catch to LogException and rethrow so
errors are observable.
In `@Core/Resgrid.Services/ReportingRollupProcessor.cs`:
- Around line 127-163: ComputeUtilizationRollupsAsync currently issues one
GetAllStatesForUnitInDateRangeAsync per unit causing N+1 queries; instead add or
call a batch method GetAllStatesForDepartmentInDateRangeAsync on
_unitStatesService to fetch all unit states for the department in the date range
once, then group the returned states by UnitId and iterate groups to build the
ordered list (use the same .OrderBy(s => s.Timestamp).Select(...) pattern and
call ReportingMath.UtilizationSeconds per group), keeping the existing logic
that skips empty totals and accumulates sumUhu/unitCount before adding the
ReportingDailyRollup; update usages of unit.UnitId to group keys and remove the
per-unit await inside the foreach.
In
`@Repositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportNewDepartmentsQuery.cs`:
- Around line 26-29: The GetQuery<TEntity>() generic overload currently throws a
bare System.NotImplementedException which is ambiguous; update the throw in
GetQuery<TEntity>() to include a descriptive message such as "Entity-based query
overload not supported for aggregate reporting queries" (or similar) so
maintainers understand this overload is intentionally unsupported for classes
like SelectReportNewDepartmentsQuery and other query builders in this cohort.
In
`@Repositories/Resgrid.Repositories.DataRepository/ReportingRollupRepository.cs`:
- Around line 43-49: The code mutates items from the caller by updating
ReportingDailyRollup instances in rowList (setting DepartmentId, BucketDateUtc,
CreatedOnUtc); instead, create a new collection (e.g., normalizedRows) by
projecting each item into a new ReportingDailyRollup (copying existing
properties and setting DepartmentId, BucketDateUtc, and CreatedOnUtc when
default) and use normalizedRows for the subsequent insert, leaving the original
rowList unmodified.
In `@Web/Resgrid.Web.Services/Controllers/v4/ReportingController.cs`:
- Around line 161-162: The helper ToUtc currently treats
DateTimeKind.Unspecified as UTC which can misinterpret local timestamps; change
the behavior so unspecified kinds are rejected instead of silently converted:
modify ToUtc (and callers in ReportingController) to detect
DateTimeKind.Unspecified and surface an explicit failure (e.g., throw
ArgumentException or return a validation result) and update the controller
action(s) to return a 400 Bad Request with a clear message when ToUtc reports
unspecified kind; alternatively, if you prefer non-failing behavior, add a
warning log in ToUtc (or in ReportingController) when DateTimeKind.Unspecified
is seen so callers can diagnose incorrect client usage.
🪄 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: 7370982b-bad9-45a3-a3cc-459963aa64f1
⛔ Files ignored due to path filters (3)
Tests/Resgrid.Tests/Services/IncidentExportTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/ReportingAnalyticsTests.csis excluded by!**/Tests/**Web/Resgrid.Web/Areas/User/Apps/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (65)
Core/Resgrid.Model/Reporting/AvailabilityClass.csCore/Resgrid.Model/Reporting/AvailabilityMatrix.csCore/Resgrid.Model/Reporting/Breakdown.csCore/Resgrid.Model/Reporting/BreakdownItem.csCore/Resgrid.Model/Reporting/DashboardReport.csCore/Resgrid.Model/Reporting/ExportProfile.csCore/Resgrid.Model/Reporting/MetricPoint.csCore/Resgrid.Model/Reporting/MetricSeries.csCore/Resgrid.Model/Reporting/ParticipationReport.csCore/Resgrid.Model/Reporting/ReportGranularity.csCore/Resgrid.Model/Reporting/ReportTotals.csCore/Resgrid.Model/Reporting/ReportingAggregates.csCore/Resgrid.Model/Reporting/ReportingDailyRollup.csCore/Resgrid.Model/Reporting/ReportingMetrics.csCore/Resgrid.Model/Reporting/ResponseTimeReport.csCore/Resgrid.Model/Reporting/UtilizationReport.csCore/Resgrid.Model/Repositories/IReportingRepository.csCore/Resgrid.Model/Repositories/IReportingRollupRepository.csCore/Resgrid.Model/Services/IPlatformReportingService.csCore/Resgrid.Model/Services/IReportingRollupProcessor.csCore/Resgrid.Services/AuditService.csCore/Resgrid.Services/PlatformReportingService.csCore/Resgrid.Services/Reporting/IncidentExport.csCore/Resgrid.Services/ReportingMath.csCore/Resgrid.Services/ReportingRollupProcessor.csCore/Resgrid.Services/ServicesModule.csProviders/Resgrid.Providers.Migrations/Migrations/M0073_AddingReportingIndexes.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0073_AddingReportingIndexesPg.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/Reporting/DeleteReportRollupForDateQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/InsertReportRollupQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportActiveCallsCountQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportCallsByDayQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportCallsByMonthQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportCallsByPriorityQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportCallsByStateQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportCallsByTypeQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportCallsCountAllTimeQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportCallsCountQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportDepartmentsTotalQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportLatestPersonnelStatesQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportLatestUnitStatesQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportMessagesByDayQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportMessagesByMonthQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportMessagesCountQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportNewDepartmentsQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportPersonnelCountQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportRollupsQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Reporting/SelectReportUnitsCountQuery.csRepositories/Resgrid.Repositories.DataRepository/ReportingRepository.csRepositories/Resgrid.Repositories.DataRepository/ReportingRollupRepository.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csRepositories/Resgrid.Repositories.DataRepository/SystemAuditRepository.csWeb/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.csWeb/Resgrid.Web.Services/Controllers/v4/ReportingController.csWeb/Resgrid.Web.Services/Models/v4/Reporting/ReportingResults.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWorkers/Resgrid.Workers.Console/Commands/ReportingRollupCommand.csWorkers/Resgrid.Workers.Console/Program.csWorkers/Resgrid.Workers.Console/Tasks/ReportingRollupTask.cs
| /// Returns the standardized required fields for a profile that Resgrid does not currently capture | ||
| /// (the export "gap report"). Empty for <see cref="ExportProfile.Generic"/>. | ||
| /// </summary> | ||
| IReadOnlyList<string> GetUnmappedRequiredExportFields(ExportProfile profile); |
Contributor
There was a problem hiding this comment.
Service method should be async per coding guidelines.
GetUnmappedRequiredExportFields returns IReadOnlyList<string> synchronously, but the coding guidelines state: "All service methods are async returning Task<T>". If this is a pure lookup with no I/O, the synchronous signature might be intentional, but it deviates from the established service contract pattern.
Consider converting to async for consistency, or confirm this is an approved exception for pure functions.
♻️ Async signature for guideline compliance
- IReadOnlyList<string> GetUnmappedRequiredExportFields(ExportProfile profile);
+ Task<IReadOnlyList<string>> GetUnmappedRequiredExportFieldsAsync(ExportProfile profile);As per coding guidelines: "All service methods are async returning Task<T>" in Core/Resgrid.Services/**/*.cs.
📝 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
| IReadOnlyList<string> GetUnmappedRequiredExportFields(ExportProfile profile); | |
| Task<IReadOnlyList<string>> GetUnmappedRequiredExportFieldsAsync(ExportProfile profile); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Model/Services/IPlatformReportingService.cs` at line 54, The
interface method GetUnmappedRequiredExportFields currently returns
IReadOnlyList<string> synchronously; update
IPlatformReportingService.GetUnmappedRequiredExportFields to return
Task<IReadOnlyList<string>> and make the method async-compatible, then update
all implementing classes (methods named GetUnmappedRequiredExportFields) to
return Task<IReadOnlyList<string>> (wrapping the current result with
Task.FromResult(...) if no I/O) and update all callers to await the call; ensure
signatures, usings and any unit tests are adjusted accordingly to preserve
behavior while complying with the async service guideline.
| /// admin backfill command. Because it is a batch job (not the latency-bounded request path) it may | ||
| /// iterate departments and materialize a day's rows to compute percentiles. | ||
| /// </summary> | ||
| public interface IReportingRollupProcessor |
Contributor
There was a problem hiding this comment.
Service interface name should follow I{Name}Service pattern.
The interface is named IReportingRollupProcessor but resides in the Resgrid.Model.Services namespace. Per coding guidelines, service interface names must follow the pattern I{Name}Service.
Consider renaming to IReportingRollupService or IReportingRollupProcessorService for consistency with the established naming convention, or confirm if "Processor" is an approved exception for background job contracts.
♻️ Suggested rename for guideline compliance
- public interface IReportingRollupProcessor
+ public interface IReportingRollupServiceAs per coding guidelines: "Service interface names must follow the pattern I{Name}Service" in Core/Resgrid.Services/**/*.cs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Model/Services/IReportingRollupProcessor.cs` at line 14, Rename
the service interface IReportingRollupProcessor to follow the I{Name}Service
convention (suggested: IReportingRollupService), update the interface
declaration name and any implementing classes (e.g., classes implementing
IReportingRollupProcessor) to implement IReportingRollupService, update all
references/usages and dependency injection registrations to the new interface
name (including constructor injections and service registrations), and keep the
existing namespace and method signatures unchanged so only the type name
changes.
Comment on lines
+39
to
+47
| public PlatformReportingService(IReportingRepository reportingRepository, IReportingRollupRepository rollupRepository, | ||
| ICacheProvider cacheProvider, ICustomStateService customStateService, ICallsService callsService) | ||
| { | ||
| _reportingRepository = reportingRepository; | ||
| _rollupRepository = rollupRepository; | ||
| _cacheProvider = cacheProvider; | ||
| _customStateService = customStateService; | ||
| _callsService = callsService; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use Service Locator pattern instead of constructor injection.
The coding guidelines require using Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors rather than constructor injection. This applies to all **/*.cs files.
As per coding guidelines: "Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection"
♻️ Proposed refactor to use Service Locator pattern
-public PlatformReportingService(IReportingRepository reportingRepository, IReportingRollupRepository rollupRepository,
- ICacheProvider cacheProvider, ICustomStateService customStateService, ICallsService callsService)
+public PlatformReportingService()
{
- _reportingRepository = reportingRepository;
- _rollupRepository = rollupRepository;
- _cacheProvider = cacheProvider;
- _customStateService = customStateService;
- _callsService = callsService;
+ var kernel = Bootstrapper.GetKernel();
+ _reportingRepository = kernel.Resolve<IReportingRepository>();
+ _rollupRepository = kernel.Resolve<IReportingRollupRepository>();
+ _cacheProvider = kernel.Resolve<ICacheProvider>();
+ _customStateService = kernel.Resolve<ICustomStateService>();
+ _callsService = kernel.Resolve<ICallsService>();
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/PlatformReportingService.cs` around lines 39 - 47,
Change the PlatformReportingService constructor to use the service locator:
remove constructor parameters for IReportingRepository,
IReportingRollupRepository, ICacheProvider, ICustomStateService, and
ICallsService and instead resolve those dependencies inside the constructor
using Bootstrapper.GetKernel().Resolve<T>() for each dependency, assigning the
results to the existing fields (_reportingRepository, _rollupRepository,
_cacheProvider, _customStateService, _callsService); ensure the class still
initializes all five fields and that any callers expecting the old constructor
are updated to use the parameterless constructor.
Comment on lines
+455
to
+456
| foreach (var detail in state.GetActiveDetails()) | ||
| map[detail.CustomStateDetailId] = detail; |
Contributor
There was a problem hiding this comment.
Add null check for GetActiveDetails() result.
The result from state.GetActiveDetails() is not null-checked before iterating. If the method returns null, the foreach will throw a NullReferenceException.
🛡️ Proposed fix to add null guard
foreach (var state in states.Where(s => s.Type == (int)type))
{
- foreach (var detail in state.GetActiveDetails())
+ var details = state.GetActiveDetails();
+ if (details != null)
+ {
+ foreach (var detail in details)
+ map[detail.CustomStateDetailId] = detail;
+ }
- map[detail.CustomStateDetailId] = detail;
}📝 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
| foreach (var detail in state.GetActiveDetails()) | |
| map[detail.CustomStateDetailId] = detail; | |
| foreach (var state in states.Where(s => s.Type == (int)type)) | |
| { | |
| var details = state.GetActiveDetails(); | |
| if (details != null) | |
| { | |
| foreach (var detail in details) | |
| map[detail.CustomStateDetailId] = detail; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/PlatformReportingService.cs` around lines 455 - 456,
state.GetActiveDetails() may be null causing the foreach to throw; guard the
enumeration by capturing its result into a local (e.g., var details =
state.GetActiveDetails()) and only iterate if details is not null (or use a
null-safe enumerator), then populate map[detail.CustomStateDetailId] = detail;
referencing state.GetActiveDetails(), detail.CustomStateDetailId and map to
locate the code.
Comment on lines
+29
to
+37
| public ReportingRollupProcessor(ICallsService callsService, IReportingRollupRepository rollupRepository, | ||
| IUnitsService unitsService, IUnitStatesService unitStatesService, ICustomStateService customStateService) | ||
| { | ||
| _callsService = callsService; | ||
| _rollupRepository = rollupRepository; | ||
| _unitsService = unitsService; | ||
| _unitStatesService = unitStatesService; | ||
| _customStateService = customStateService; | ||
| } |
Contributor
There was a problem hiding this comment.
Use Service Locator pattern instead of constructor injection.
The constructor uses constructor injection for 5 dependencies, which violates the coding guidelines. The codebase standard is to resolve dependencies explicitly in the constructor body using Bootstrapper.GetKernel().Resolve<T>().
♻️ Proposed refactor to use Service Locator pattern
-public ReportingRollupProcessor(ICallsService callsService, IReportingRollupRepository rollupRepository,
- IUnitsService unitsService, IUnitStatesService unitStatesService, ICustomStateService customStateService)
+public ReportingRollupProcessor()
{
- _callsService = callsService;
- _rollupRepository = rollupRepository;
- _unitsService = unitsService;
- _unitStatesService = unitStatesService;
- _customStateService = customStateService;
+ _callsService = Bootstrapper.GetKernel().Resolve<ICallsService>();
+ _rollupRepository = Bootstrapper.GetKernel().Resolve<IReportingRollupRepository>();
+ _unitsService = Bootstrapper.GetKernel().Resolve<IUnitsService>();
+ _unitStatesService = Bootstrapper.GetKernel().Resolve<IUnitStatesService>();
+ _customStateService = Bootstrapper.GetKernel().Resolve<ICustomStateService>();
}As per coding guidelines: "Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection."
📝 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
| public ReportingRollupProcessor(ICallsService callsService, IReportingRollupRepository rollupRepository, | |
| IUnitsService unitsService, IUnitStatesService unitStatesService, ICustomStateService customStateService) | |
| { | |
| _callsService = callsService; | |
| _rollupRepository = rollupRepository; | |
| _unitsService = unitsService; | |
| _unitStatesService = unitStatesService; | |
| _customStateService = customStateService; | |
| } | |
| public ReportingRollupProcessor() | |
| { | |
| _callsService = Bootstrapper.GetKernel().Resolve<ICallsService>(); | |
| _rollupRepository = Bootstrapper.GetKernel().Resolve<IReportingRollupRepository>(); | |
| _unitsService = Bootstrapper.GetKernel().Resolve<IUnitsService>(); | |
| _unitStatesService = Bootstrapper.GetKernel().Resolve<IUnitStatesService>(); | |
| _customStateService = Bootstrapper.GetKernel().Resolve<ICustomStateService>(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/ReportingRollupProcessor.cs` around lines 29 - 37, The
constructor of ReportingRollupProcessor currently takes five services via
constructor injection (_callsService, _rollupRepository, _unitsService,
_unitStatesService, _customStateService); change it to use the Service Locator
by removing those parameters and resolving each inside the constructor body with
Bootstrapper.GetKernel().Resolve<T>() for the corresponding types, assigning the
results to the existing fields (e.g., _callsService, _rollupRepository,
_unitsService, _unitStatesService, _customStateService) so dependencies are
obtained explicitly in the ReportingRollupProcessor constructor.
Comment on lines
+35
to
+38
| public ReportingController(IPlatformReportingService reportingService) | ||
| { | ||
| _reportingService = reportingService; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use Service Locator pattern instead of constructor injection.
The coding guidelines require using Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors rather than constructor injection.
As per coding guidelines: "Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection"
♻️ Proposed refactor to use Service Locator pattern
-public ReportingController(IPlatformReportingService reportingService)
+public ReportingController()
{
- _reportingService = reportingService;
+ _reportingService = Bootstrapper.GetKernel().Resolve<IPlatformReportingService>();
}📝 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
| public ReportingController(IPlatformReportingService reportingService) | |
| { | |
| _reportingService = reportingService; | |
| } | |
| public ReportingController() | |
| { | |
| _reportingService = Bootstrapper.GetKernel().Resolve<IPlatformReportingService>(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Controllers/v4/ReportingController.cs` around lines
35 - 38, The constructor currently uses constructor injection for
IPlatformReportingService in ReportingController; change it to resolve the
dependency via the service locator by calling
Bootstrapper.GetKernel().Resolve<IPlatformReportingService>() and assign the
result to the _reportingService field inside the constructor (replace the
injected parameter), ensuring ReportingController, _reportingService and the
call to Bootstrapper.GetKernel().Resolve<IPlatformReportingService>() are used
to locate and set the dependency.
| { | ||
| public int Id { get; } | ||
| public Guid? CorrelationId { get; set; } | ||
| public Dictionary<string, string> Metadata { get; set; } |
Contributor
There was a problem hiding this comment.
Initialize Metadata to avoid null dereferences in command handlers.
Metadata is currently null by default, which can fail at runtime when downstream code writes to it without a guard.
Suggested fix
- public Dictionary<string, string> Metadata { get; set; }
+ public Dictionary<string, string> Metadata { get; set; } = new Dictionary<string, string>();📝 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
| public Dictionary<string, string> Metadata { get; set; } | |
| public Dictionary<string, string> Metadata { get; set; } = new Dictionary<string, string>(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Workers/Resgrid.Workers.Console/Commands/ReportingRollupCommand.cs` at line
11, The Metadata property on ReportingRollupCommand is null by default and can
cause null dereferences; initialize it to an empty Dictionary to ensure safe
writes. Update the ReportingRollupCommand class to set Metadata = new
Dictionary<string,string>() either inline on the property (public
Dictionary<string,string> Metadata { get; set; } = new
Dictionary<string,string>();) or in the ReportingRollupCommand constructor, so
downstream command handlers can add entries without null checks.
| Cron.MinuteIntervals(5), | ||
| stoppingToken); | ||
|
|
||
| _logger.Log(LogLevel.Information, "Scheduling Reporting Rollup"); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use Resgrid.Framework.Logging.LogInfo for the new scheduling log.
This new log call should follow the repository logging standard instead of ILogger.Log(...).
As per coding guidelines, “Use Resgrid.Framework.Logging static methods (LogException, LogError, LogInfo, LogDebug) for all logging throughout the codebase”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Workers/Resgrid.Workers.Console/Program.cs` at line 424, Replace the direct
use of _logger.Log(LogLevel.Information, "Scheduling Reporting Rollup") with the
repository standard Resgrid.Framework.Logging.LogInfo call; locate the call to
_logger.Log in Program.Main (or the scheduling setup) and change it to call
Resgrid.Framework.Logging.LogInfo with the same "Scheduling Reporting Rollup"
message so the project uses the static logging helper instead of ILogger.Log.
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
Bug Fixes