Conversation
Add ability for department admins to edit existing trainings: - Add Edit GET/POST actions to TrainingsController - Create EditTrainingModel view model - Create Edit.cshtml view with form for editing training details - Add Edit button to training index page (admin only) - Preserve existing attachments, questions, and user assignments - Support adding new attachments, users, groups, and roles - Add comprehensive unit tests for TrainingService Files added: - Tests/Resgrid.Tests/Mocks/MockTrainingRepository.cs - Tests/Resgrid.Tests/Mocks/MockTrainingAttachmentRepository.cs - Tests/Resgrid.Tests/Mocks/MockTrainingQuestionRepository.cs - Tests/Resgrid.Tests/Mocks/MockTrainingUserRepository.cs - Tests/Resgrid.Tests/Services/TrainingServiceTests.cs - Web/Resgrid.Web/Areas/User/Models/Training/EditTrainingModel.cs - Web/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtml - Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js Files modified: - Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs - Web/Resgrid.Web/Areas/User/Views/Trainings/Index.cshtml
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…tionality feat(training): Add edit functionality for training module
Contributor
📝 WalkthroughWalkthroughAdds training edit UI and server-side handling, localizes training views, fixes communication protocol/address bugs, adds null-check in TrainingService, implements Environment Canada and MeteoAlarm weather providers with improved caching, introduces test mocks and comprehensive TrainingService tests, and updates related client scripts and views. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces new domain capabilities around call video feeds, communication tests, and weather alerts, along with supporting repositories, migrations, DI registration, claims, and localization updates.
Changes:
- Added Call Video Feed persistence + service APIs (including repositories, SQL config, and tests).
- Added Communication Test and Weather Alert data models, repositories/queries, migrations, DI modules, and claims.
- Extended notification services to support “dispatch cancelled” notifications (push/SMS/email) and added new settings/config knobs.
Reviewed changes
Copilot reviewed 153 out of 246 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/Resgrid.Tests/Services/CallVideoFeedTests.cs | Adds unit tests covering CallsService call video feed CRUD behaviors. |
| Tests/Resgrid.Tests/Resgrid.Tests.csproj | Adds a test project reference for the new Weather provider project. |
| Tests/Resgrid.Tests/Mocks/MockTrainingUserRepository.cs | Adds in-memory training user repo to support service/unit tests. |
| Tests/Resgrid.Tests/Mocks/MockTrainingRepository.cs | Adds in-memory training repo to support service/unit tests. |
| Tests/Resgrid.Tests/Mocks/MockTrainingQuestionRepository.cs | Adds in-memory training question repo for tests. |
| Tests/Resgrid.Tests/Mocks/MockTrainingAttachmentRepository.cs | Adds in-memory training attachment repo for tests. |
| Repositories/Resgrid.Repositories.DataRepository/WeatherAlertZoneRepository.cs | Adds Dapper repository for weather alert zones. |
| Repositories/Resgrid.Repositories.DataRepository/WeatherAlertSourceRepository.cs | Adds Dapper repository for weather alert sources. |
| Repositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.cs | Adds SQL Server table/query config for call video feeds, communication tests, and weather alerts. |
| Repositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.cs | Adds PostgreSQL table/query config for call video feeds, communication tests, and weather alerts. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertsByDepartmentAndSeverityQuery.cs | Adds query generator for severity-based weather alert retrieval. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertsByDepartmentAndCategoryQuery.cs | Adds query generator for category-based weather alert retrieval. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertZonesByDepartmentIdQuery.cs | Adds query generator for weather alert zones by department. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertSourcesByDepartmentIdQuery.cs | Adds query generator for weather alert sources by department. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertHistoryByDepartmentQuery.cs | Adds query generator for weather alert history by department/date range. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectWeatherAlertByExternalIdAndSourceIdQuery.cs | Adds query generator to locate alerts by external id + source. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectUnnotifiedWeatherAlertsQuery.cs | Adds query generator for alerts pending notification. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectExpiredUnprocessedWeatherAlertsQuery.cs | Adds query generator for expired/unprocessed alerts. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectActiveWeatherAlertsByDepartmentIdQuery.cs | Adds query generator for active alerts by department. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectActiveWeatherAlertZonesByDepartmentIdQuery.cs | Adds query generator for active zones by department. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/WeatherAlerts/SelectActiveWeatherAlertSourcesForPollingQuery.cs | Adds query generator for polling-enabled sources. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/CommunicationTests/SelectOpenCommTestRunsQuery.cs | Adds query generator for open communication test runs. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/CommunicationTests/SelectCommTestRunsByTestIdQuery.cs | Adds query for runs by test id. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/CommunicationTests/SelectCommTestRunByRunCodeQuery.cs | Adds query for run lookup by run code. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/CommunicationTests/SelectCommTestResultsByRunIdQuery.cs | Adds query for results by run id. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/CommunicationTests/SelectCommTestResultByResponseTokenQuery.cs | Adds query for results by response token. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/CommunicationTests/SelectActiveCommTestsByScheduleTypeQuery.cs | Adds query for active tests by schedule type. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/CallVideoFeeds/SelectCallVideoFeedsByDepartmentIdQuery.cs | Adds query generator for call video feeds by department. |
| Repositories/Resgrid.Repositories.DataRepository/Queries/CallVideoFeeds/SelectCallVideoFeedsByCallIdQuery.cs | Adds query generator for call video feeds by call id. |
| Repositories/Resgrid.Repositories.DataRepository/Modules/TestingDataModule.cs | Registers newly added repositories in the testing DI module. |
| Repositories/Resgrid.Repositories.DataRepository/Modules/NonWebDataModule.cs | Registers newly added repositories in the non-web DI module. |
| Repositories/Resgrid.Repositories.DataRepository/Modules/DataModule.cs | Registers newly added repositories in the main data DI module. |
| Repositories/Resgrid.Repositories.DataRepository/Modules/ApiDataModule.cs | Registers newly added repositories in the API DI module. |
| Repositories/Resgrid.Repositories.DataRepository/Configs/SqlConfiguration.cs | Adds configuration properties for new tables/queries. |
| Repositories/Resgrid.Repositories.DataRepository/CommunicationTestRunRepository.cs | Adds Dapper repository for communication test runs. |
| Repositories/Resgrid.Repositories.DataRepository/CommunicationTestResultRepository.cs | Adds Dapper repository for communication test results. |
| Repositories/Resgrid.Repositories.DataRepository/CommunicationTestRepository.cs | Adds Dapper repository for communication tests. |
| Repositories/Resgrid.Repositories.DataRepository/CallVideoFeedRepository.cs | Adds Dapper repository for call video feeds. |
| Providers/Resgrid.Providers.Weather/WeatherProviderModule.cs | Adds Autofac module for weather alert providers/factory. |
| Providers/Resgrid.Providers.Weather/WeatherAlertResponseCache.cs | Adds shared in-memory cache for provider responses. |
| Providers/Resgrid.Providers.Weather/WeatherAlertProviderFactory.cs | Adds provider factory implementation. |
| Providers/Resgrid.Providers.Weather/Resgrid.Providers.Weather.csproj | Introduces new Weather provider project. |
| Providers/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.cs | Adds MeteoAlarm provider stub + caching hook. |
| Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs | Adds Environment Canada provider stub + caching hook. |
| Providers/Resgrid.Providers.MigrationsPg/Migrations/M0063_AddingWeatherAlertsPg.cs | Adds PostgreSQL migrations for weather alerts. |
| Providers/Resgrid.Providers.MigrationsPg/Migrations/M0062_AddingCommunicationTestsPg.cs | Adds PostgreSQL migrations for communication tests. |
| Providers/Resgrid.Providers.MigrationsPg/Migrations/M0061_AddingCallVideoFeedsPg.cs | Adds PostgreSQL migrations for call video feeds. |
| Providers/Resgrid.Providers.Migrations/Migrations/M0063_AddingWeatherAlerts.cs | Adds SQL Server migrations for weather alerts. |
| Providers/Resgrid.Providers.Migrations/Migrations/M0062_AddingCommunicationTests.cs | Adds SQL Server migrations for communication tests. |
| Providers/Resgrid.Providers.Migrations/Migrations/M0061_AddingCallVideoFeeds.cs | Adds SQL Server migrations for call video feeds. |
| Providers/Resgrid.Providers.Claims/ResgridResources.cs | Adds new claim resource constants for the new modules. |
| Providers/Resgrid.Providers.Claims/ResgridIdentity.cs | Adds helpers to attach new module claims. |
| Providers/Resgrid.Providers.Claims/ResgridClaimTypes.cs | Extends claim type resources for new modules. |
| Providers/Resgrid.Providers.Claims/JwtTokenProvider.cs | Adds new module claims into JWT generation. |
| Providers/Resgrid.Providers.Claims/ClaimsPrincipalFactory.cs | Adds new module claims into identity principal creation. |
| Providers/Resgrid.Providers.Claims/ClaimsLogic.cs | Implements claim assignment logic for new modules. |
| Core/Resgrid.Services/TrainingService.cs | Fixes null handling when loading training details. |
| Core/Resgrid.Services/SmsService.cs | Adds SMS cancellation notification support for call dispatches. |
| Core/Resgrid.Services/ServicesModule.cs | Registers new services for communication tests and weather alerts. |
| Core/Resgrid.Services/EmailService.cs | Adds email cancellation notification support for call dispatches. |
| Core/Resgrid.Services/DepartmentSettingsService.cs | Adds a typed setting accessor method. |
| Core/Resgrid.Services/CommunicationService.cs | Extends notification orchestration with cancellation notifications. |
| Core/Resgrid.Services/CallsService.cs | Adds call video feed APIs and optional call population of video feeds. |
| Core/Resgrid.Model/WeatherAlertZone.cs | Adds WeatherAlertZone model. |
| Core/Resgrid.Model/WeatherAlertUrgency.cs | Adds weather alert urgency enum. |
| Core/Resgrid.Model/WeatherAlertStatus.cs | Adds weather alert status enum. |
| Core/Resgrid.Model/WeatherAlertSourceType.cs | Adds weather alert source type enum. |
| Core/Resgrid.Model/WeatherAlertSource.cs | Adds WeatherAlertSource model. |
| Core/Resgrid.Model/WeatherAlertSeverity.cs | Adds weather alert severity enum. |
| Core/Resgrid.Model/WeatherAlertCertainty.cs | Adds weather alert certainty enum. |
| Core/Resgrid.Model/WeatherAlertCategory.cs | Adds weather alert category enum. |
| Core/Resgrid.Model/WeatherAlert.cs | Adds WeatherAlert model. |
| Core/Resgrid.Model/Services/IWeatherAlertService.cs | Introduces Weather Alert service interface. |
| Core/Resgrid.Model/Services/ISmsService.cs | Adds cancellation SMS method to service contract. |
| Core/Resgrid.Model/Services/IEmailService.cs | Adds cancellation email method to service contract. |
| Core/Resgrid.Model/Services/IDepartmentSettingsService.cs | Adds typed department setting API to service contract. |
| Core/Resgrid.Model/Services/ICommunicationTestService.cs | Introduces communication test service interface. |
| Core/Resgrid.Model/Services/ICommunicationService.cs | Adds cancellation notification methods to communication service contract. |
| Core/Resgrid.Model/Services/ICallsService.cs | Adds call video feed APIs to calls service contract. |
| Core/Resgrid.Model/Repositories/IWeatherAlertZoneRepository.cs | Adds repository contract for weather alert zones. |
| Core/Resgrid.Model/Repositories/IWeatherAlertSourceRepository.cs | Adds repository contract for weather alert sources. |
| Core/Resgrid.Model/Repositories/IWeatherAlertRepository.cs | Adds repository contract for weather alerts. |
| Core/Resgrid.Model/Repositories/ICommunicationTestRunRepository.cs | Adds repository contract for test runs. |
| Core/Resgrid.Model/Repositories/ICommunicationTestResultRepository.cs | Adds repository contract for test results. |
| Core/Resgrid.Model/Repositories/ICommunicationTestRepository.cs | Adds repository contract for tests. |
| Core/Resgrid.Model/Repositories/ICallVideoFeedRepository.cs | Adds repository contract for call video feeds. |
| Core/Resgrid.Model/Providers/IWeatherAlertProviderFactory.cs | Adds provider factory contract. |
| Core/Resgrid.Model/Providers/IWeatherAlertProvider.cs | Adds provider contract. |
| Core/Resgrid.Model/Events/EventTypes.cs | Adds weather alert-related event types. |
| Core/Resgrid.Model/DepartmentSettingTypes.cs | Adds weather alert-related department setting types. |
| Core/Resgrid.Model/CommunicationTestScheduleType.cs | Adds schedule type enum for communication tests. |
| Core/Resgrid.Model/CommunicationTestRunStatus.cs | Adds run status enum for communication test runs. |
| Core/Resgrid.Model/CommunicationTestRun.cs | Adds CommunicationTestRun model. |
| Core/Resgrid.Model/CommunicationTestResult.cs | Adds CommunicationTestResult model. |
| Core/Resgrid.Model/CommunicationTestChannel.cs | Adds communication test channel enum. |
| Core/Resgrid.Model/CommunicationTest.cs | Adds CommunicationTest model. |
| Core/Resgrid.Model/CallVideoFeedTypes.cs | Adds enum for call video feed types. |
| Core/Resgrid.Model/CallVideoFeedStatuses.cs | Adds enum for call video feed statuses. |
| Core/Resgrid.Model/CallVideoFeedFormats.cs | Adds enum for call video feed formats. |
| Core/Resgrid.Model/CallVideoFeed.cs | Adds CallVideoFeed model. |
| Core/Resgrid.Model/Call.cs | Adds VideoFeeds collection to Call and ignores it for persistence mapping. |
| Core/Resgrid.Model/Billing/Api/GetActivePaymentProviderResult.cs | Adds billing API DTO for active provider response. |
| Core/Resgrid.Model/AuditLogTypes.cs | Adds audit log types for communication tests and weather alerts. |
| Core/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.cs | Adds localization marker class for weather alerts. |
| Core/Resgrid.Localization/Areas/User/Trainings/Trainings.cs | Adds localization marker class for trainings. |
| Core/Resgrid.Localization/Areas/User/Dispatch/Call.uk.resx | Adds strings for cancellation notify + call video feeds (UK). |
| Core/Resgrid.Localization/Areas/User/Dispatch/Call.sv.resx | Adds strings for cancellation notify + call video feeds (SV). |
| Core/Resgrid.Localization/Areas/User/Dispatch/Call.pl.resx | Adds strings for cancellation notify + call video feeds (PL). |
| Core/Resgrid.Localization/Areas/User/Dispatch/Call.it.resx | Adds strings for cancellation notify + call video feeds (IT). |
| Core/Resgrid.Localization/Areas/User/Dispatch/Call.fr.resx | Adds strings for cancellation notify + call video feeds (FR). |
| Core/Resgrid.Localization/Areas/User/Dispatch/Call.es.resx | Adds strings for cancellation notify + call video feeds (ES). |
| Core/Resgrid.Localization/Areas/User/Dispatch/Call.en.resx | Adds strings for cancellation notify + call video feeds (EN). |
| Core/Resgrid.Localization/Areas/User/Dispatch/Call.de.resx | Adds strings for cancellation notify + call video feeds (DE). |
| Core/Resgrid.Localization/Areas/User/Dispatch/Call.ar.resx | Adds strings for cancellation notify + call video feeds (AR). |
| Core/Resgrid.Localization/Areas/User/CommunicationTest/CommunicationTest.uk.resx | Adds communication test strings (UK). |
| Core/Resgrid.Localization/Areas/User/CommunicationTest/CommunicationTest.pl.resx | Adds communication test strings (PL). |
| Core/Resgrid.Localization/Areas/User/CommunicationTest/CommunicationTest.it.resx | Adds communication test strings (IT). |
| Core/Resgrid.Localization/Areas/User/CommunicationTest/CommunicationTest.fr.resx | Adds communication test strings (FR). |
| Core/Resgrid.Localization/Areas/User/CommunicationTest/CommunicationTest.es.resx | Adds communication test strings (ES). |
| Core/Resgrid.Localization/Areas/User/CommunicationTest/CommunicationTest.cs | Adds localization marker class for communication tests. |
| Core/Resgrid.Localization/Account/Login.en.resx | Adds “Region” label for login. |
| Core/Resgrid.Config/PaymentProviderConfig.cs | Adds active payment provider toggle and helpers. |
| Core/Resgrid.Config/InfoConfig.cs | Adds AppUrl to system locations and helper for login URL. |
Core/Resgrid.Services/SmsService.cs
Outdated
| if (String.IsNullOrWhiteSpace(protocols)) | ||
| protocols = protocol.Data; | ||
| else | ||
| protocols = protocol + "," + protocol.Data; |
There was a problem hiding this comment.
The protocol concatenation uses protocol (object) instead of the accumulating protocols string, which will produce incorrect output (and could call ToString() unexpectedly). Update the else branch to append to protocols (e.g., protocols = protocols + "," + protocol.Data).
Suggested change
| protocols = protocol + "," + protocol.Data; | |
| protocols = protocols + "," + protocol.Data; |
Comment on lines
+306
to
+319
| if (call.Protocols != null && call.Protocols.Any()) | ||
| { | ||
| string protocols = String.Empty; | ||
| foreach (var protocol in call.Protocols) | ||
| { | ||
| if (String.IsNullOrWhiteSpace(protocols)) | ||
| protocols = protocol.Data; | ||
| else | ||
| protocols = protocol + "," + protocol.Data; | ||
| } | ||
|
|
||
| if (!String.IsNullOrWhiteSpace(protocols)) | ||
| natureOfCall = natureOfCall + " (" + protocols + ")"; | ||
| } |
There was a problem hiding this comment.
Same issue as in SmsService: the concatenation uses protocol instead of appending to the protocols accumulator. This will format the protocol list incorrectly; append to protocols instead.
Comment on lines
+261
to
+267
| if (dispatch.User != null && dispatch.User != null) | ||
| emailAddress = dispatch.User.Email; | ||
| else | ||
| { | ||
| var user = _usersService.GetUserById(dispatch.UserId, false); | ||
|
|
||
| if (user != null && user != null) |
There was a problem hiding this comment.
The null checks are duplicated (dispatch.User != null && dispatch.User != null and user != null && user != null), which is redundant and makes the intent unclear. Replace each duplicated check with a single null check.
Suggested change
| if (dispatch.User != null && dispatch.User != null) | |
| emailAddress = dispatch.User.Email; | |
| else | |
| { | |
| var user = _usersService.GetUserById(dispatch.UserId, false); | |
| if (user != null && user != null) | |
| if (dispatch.User != null) | |
| emailAddress = dispatch.User.Email; | |
| else | |
| { | |
| var user = _usersService.GetUserById(dispatch.UserId, false); | |
| if (user != null) |
| try | ||
| { | ||
| var payment = await _subscriptionsService.GetCurrentPaymentForDepartmentAsync(departmentId); | ||
| await _smsService.SendCancelCallAsync(call, dispatch, departmentNumber, departmentId, profile, call.Address, payment); |
There was a problem hiding this comment.
SendCancelCallAsync accepts an address parameter, but this call always passes call.Address, ignoring any resolved address (including the address parameter passed into CommunicationService or a geocoded address used for the push subtitle). Pass the best-available resolved address value here (e.g., address if provided, otherwise the computed value) so SMS content matches what users see elsewhere.
Suggested change
| await _smsService.SendCancelCallAsync(call, dispatch, departmentNumber, departmentId, profile, call.Address, payment); | |
| var resolvedAddress = string.IsNullOrWhiteSpace(address) ? call.Address : address; | |
| await _smsService.SendCancelCallAsync(call, dispatch, departmentNumber, departmentId, profile, resolvedAddress, payment); |
Comment on lines
+121
to
+136
| [Test] | ||
| public async Task GetCallVideoFeedsByCallIdAsync_ShouldNotReturnDeletedFeeds() | ||
| { | ||
| var feeds = new List<CallVideoFeed> | ||
| { | ||
| new CallVideoFeed { CallVideoFeedId = "feed1", CallId = 1, Name = "Feed 1", Url = "http://example.com/1", IsDeleted = false }, | ||
| new CallVideoFeed { CallVideoFeedId = "feed2", CallId = 1, Name = "Feed 2", Url = "http://example.com/2", IsDeleted = true } | ||
| }; | ||
|
|
||
| _callVideoFeedRepo.Setup(x => x.GetByCallIdAsync(1)).ReturnsAsync(feeds); | ||
|
|
||
| var result = await _service.GetCallVideoFeedsByCallIdAsync(1); | ||
|
|
||
| // The service returns all feeds from repo; filtering is done at the API layer | ||
| result.Should().HaveCount(2); | ||
| } |
There was a problem hiding this comment.
The test name asserts deleted feeds should be excluded, but the assertion expects both feeds and the comment says filtering happens elsewhere. This will lock in ambiguous behavior; either (mandatory) rename the test to match the asserted behavior, or (preferably) update the service to filter IsDeleted == false and change the assertion to HaveCount(1) to match the repository SQL that already filters out deleted feeds.
Comment on lines
+14
to
+30
| public async Task<List<WeatherAlert>> FetchAlertsAsync(WeatherAlertSource source, CancellationToken ct = default) | ||
| { | ||
| // Check shared cache first | ||
| if (WeatherAlertResponseCache.TryGet(SourceType, source.AreaFilter, out var cachedAlerts)) | ||
| { | ||
| return cachedAlerts; | ||
| } | ||
|
|
||
| // TODO: Implement CAP XML parsing from Environment Canada | ||
| // Endpoint: https://dd.weather.gc.ca/alerts/cap/ | ||
| var alerts = new List<WeatherAlert>(); | ||
|
|
||
| // Store in shared cache | ||
| WeatherAlertResponseCache.Set(SourceType, source.AreaFilter, alerts); | ||
|
|
||
| return alerts; | ||
| } |
There was a problem hiding this comment.
This method is marked async but has no await, which will generate compiler warnings and adds overhead. Make it non-async and return Task.FromResult(alerts) (or add real awaited I/O once implemented).
Comment on lines
+14
to
+30
| public async Task<List<WeatherAlert>> FetchAlertsAsync(WeatherAlertSource source, CancellationToken ct = default) | ||
| { | ||
| // Check shared cache first | ||
| if (WeatherAlertResponseCache.TryGet(SourceType, source.AreaFilter, out var cachedAlerts)) | ||
| { | ||
| return cachedAlerts; | ||
| } | ||
|
|
||
| // TODO: Implement MeteoAlarm API integration | ||
| // Endpoint: https://feeds.meteoalarm.org/api/v1/ | ||
| var alerts = new List<WeatherAlert>(); | ||
|
|
||
| // Store in shared cache | ||
| WeatherAlertResponseCache.Set(SourceType, source.AreaFilter, alerts); | ||
|
|
||
| return alerts; | ||
| } |
There was a problem hiding this comment.
Same as the EnvironmentCanada provider: async without await. Remove async and return a completed task until real async work is added.
Comment on lines
+13
to
+17
| public string CallVideoFeedId { get; set; } | ||
|
|
||
| public int CallId { get; set; } | ||
|
|
||
| public int DepartmentId { get; set; } |
There was a problem hiding this comment.
CallVideoFeedId is the primary key (per migrations and repository usage), but it lacks key/length annotations that other entities in this codebase typically include. Add [Key], [Required], and an appropriate [MaxLength] (e.g., 128) to prevent inconsistent persistence/mapping behavior across layers.
| public object IdValue | ||
| { | ||
| get { return CallVideoFeedId; } | ||
| set { CallVideoFeedId = (string)value; } |
There was a problem hiding this comment.
The setter casts value directly to string, which will throw if the repository/framework assigns a non-string (e.g., Guid, or another id representation). Use value?.ToString() (and handle null) to make IdValue robust and consistent with other entities’ IdValue patterns.
Suggested change
| set { CallVideoFeedId = (string)value; } | |
| set { CallVideoFeedId = value?.ToString(); } |
Comment on lines
+11
to
+18
| Create.Table("weatheralertsources") | ||
| .WithColumn("weatheralertsourceid").AsCustom("citext").NotNullable().PrimaryKey() | ||
| .WithColumn("departmentid").AsInt32().NotNullable() | ||
| .WithColumn("name").AsCustom("citext").NotNullable() | ||
| .WithColumn("sourcetype").AsInt32().NotNullable() | ||
| .WithColumn("areafilter").AsCustom("citext").Nullable() | ||
| .WithColumn("apikey").AsCustom("citext").Nullable() | ||
| .WithColumn("customendpoint").AsCustom("citext").Nullable() |
There was a problem hiding this comment.
The PostgreSQL migration defines primary keys as citext with no default generation, while the corresponding model types (WeatherAlertSourceId, WeatherAlertId, WeatherAlertZoneId) are Guid with [DatabaseGenerated(DatabaseGeneratedOption.Identity)]. This mismatch will break inserts/identity expectations (and may fail Dapper type conversion depending on stored values). Align storage/types by using uuid columns with a DB-side default (gen_random_uuid() / uuid_generate_v4()), or change the model IDs to string for PG and ensure the service explicitly sets them.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| [Authorize(Policy = ResgridResources.Call_View)] | ||
| public async Task<ActionResult<CallVideoFeedsResult>> GetCallVideoFeeds(string callId) | ||
| { | ||
| if (String.IsNullOrWhiteSpace(callId) || !int.TryParse(callId, out var cId)) |
| [Authorize(Policy = ResgridResources.Call_View)] | ||
| public async Task<ActionResult<CallVideoFeedsResult>> GetCallVideoFeeds(string callId) | ||
| { | ||
| if (String.IsNullOrWhiteSpace(callId) || !int.TryParse(callId, out var cId)) |
| [Authorize(Policy = ResgridResources.Call_View)] | ||
| public async Task<ActionResult<CallVideoFeedsResult>> GetCallVideoFeeds(string callId) | ||
| { | ||
| if (String.IsNullOrWhiteSpace(callId) || !int.TryParse(callId, out var cId)) |
| [ProducesResponseType(StatusCodes.Status201Created)] | ||
| [ProducesResponseType(StatusCodes.Status400BadRequest)] | ||
| [Authorize(Policy = ResgridResources.Call_Create)] | ||
| public async Task<ActionResult<SaveCallVideoFeedResult>> SaveCallVideoFeed(SaveCallVideoFeedInput input, CancellationToken cancellationToken) |
| if (!ModelState.IsValid) | ||
| return BadRequest(); | ||
|
|
||
| if (!int.TryParse(input.CallId, out var parsedCallId)) |
| [HttpPost("SaveSource")] | ||
| [Authorize(Policy = ResgridResources.WeatherAlert_Create)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<GetWeatherAlertSourcesResult>> SaveSource([FromBody] SaveWeatherAlertSourceInput input) |
| [HttpPost("SaveZone")] | ||
| [Authorize(Policy = ResgridResources.WeatherAlert_Create)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<GetWeatherAlertZonesResult>> SaveZone([FromBody] SaveWeatherAlertZoneInput input) |
| [HttpPost("SaveSettings")] | ||
| [Authorize(Policy = ResgridResources.WeatherAlert_Update)] | ||
| [ProducesResponseType(StatusCodes.Status200OK)] | ||
| public async Task<ActionResult<GetWeatherAlertSettingsResult>> SaveSettings([FromBody] SaveWeatherAlertSettingsInput input) |
| if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) | ||
| return Unauthorized(); | ||
|
|
||
| if (model.Test == null || model.Test.CommunicationTestId == Guid.Empty) |
| if (!ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) | ||
| return Unauthorized(); | ||
|
|
||
| if (model.Test == null || model.Test.CommunicationTestId == Guid.Empty) |
![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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Core/Resgrid.Services/EmailService.cs (1)
229-238:⚠️ Potential issue | 🟡 MinorFilter empty protocol entries before appending to avoid malformed output.
The accumulator fix is correct, but these loops still append empty/whitespace
protocol.Data, which can create extra commas inNatureOfCalltext.💡 Suggested patch
if (call.Protocols != null && call.Protocols.Any()) { string protocols = String.Empty; foreach (var protocol in call.Protocols) { - if (String.IsNullOrWhiteSpace(protocols)) - protocols = protocol.Data; - else - protocols = protocols + "," + protocol.Data; + if (String.IsNullOrWhiteSpace(protocol?.Data)) + continue; + + if (String.IsNullOrWhiteSpace(protocols)) + protocols = protocol.Data; + else + protocols = protocols + "," + protocol.Data; }Also applies to: 306-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/EmailService.cs` around lines 229 - 238, The loop that builds the protocols string (variable protocols) from call.Protocols is appending empty/whitespace protocol.Data values which yields extra commas in the NatureOfCall output; update the logic to skip any protocol where String.IsNullOrWhiteSpace(protocol.Data) (e.g., filter call.Protocols first or only append when protocol.Data has content) so only non-empty values are concatenated (or use string.Join on call.Protocols.Where(p => !String.IsNullOrWhiteSpace(p.Data)).Select(p => p.Data)); apply the same change to the second occurrence around the block at 306-315.
🧹 Nitpick comments (7)
Providers/Resgrid.Providers.Weather/WeatherAlertResponseCache.cs (1)
17-26: Minor race condition on expired entry removal.The proactive cleanup of expired entries is a good improvement. However, there's a potential race between
TryGetValueandTryRemove: another thread couldSeta fresh entry for the same key between these two operations, andTryRemovewould delete the fresh data.In practice, the calling thread would then fetch and re-cache, so this only causes duplicate HTTP requests rather than data corruption. Consider using
ConcurrentDictionary.TryRemove(key, entry)overload (available in .NET 5+) that removes only if the value matches, avoiding deletion of a freshly-set entry.♻️ Optional fix using value-matching TryRemove
if (_cache.TryGetValue(key, out var entry)) { if (entry.ExpiresUtc > DateTime.UtcNow) { alerts = entry.Alerts; return true; } - _cache.TryRemove(key, out _); + // Only remove if entry hasn't been replaced by another thread + _cache.TryRemove(new KeyValuePair<string, CacheEntry>(key, entry)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/WeatherAlertResponseCache.cs` around lines 17 - 26, The code in WeatherAlertResponseCache.cs uses _cache.TryGetValue(key, out var entry) then unconditionally calls _cache.TryRemove(key, out _) which can remove a freshly set value; change the removal to the value-matching overload _cache.TryRemove(key, entry) so the entry is only removed if it still matches the previously read entry (use the same local variable entry from TryGetValue and the ConcurrentDictionary.TryRemove(key, value) overload available in .NET 5+).Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs (2)
116-137: Basic HTML parsing may be fragile.The directory link extraction uses simple string operations. This works for Environment Canada's current format but could break if the HTML structure changes (e.g., different quote styles, additional attributes). If reliability is critical, consider using a lightweight HTML parser or regex with more flexibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs` around lines 116 - 137, The current loop in EnvironmentCanadaWeatherAlertProvider that splits content and uses IndexOf/Substring (variables: lines, hrefStart, hrefEnd, href, capUrls, feedUrl) is brittle; replace it with a robust HTML parse of the feed HTML (e.g., HtmlAgilityPack or a safe HTML parser) to select all <a> elements and extract their href attributes, normalizing quotes and resolving relative URLs against feedUrl and only adding those that end with ".cap" (case-insensitive); ensure you also handle single-quoted attributes and trim values before adding to capUrls.
63-88: Consider adding logging for failed feed/document fetches.Similar to the MeteoAlarm provider, exceptions are silently swallowed. Adding diagnostic logging would help troubleshoot issues when specific feeds or CAP documents fail to load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs` around lines 63 - 88, In EnvironmentCanadaWeatherAlertProvider, the two empty catch blocks around FetchCapUrlsFromFeedAsync(feedUrl, ct) and FetchAndParseCapDocumentAsync(capUrl, source, ct) swallow exceptions without context; update those catch blocks to log the exception and relevant identifiers (e.g., feedUrl and capUrl) using the provider's logger (e.g., _logger or existing logging facility) so you capture exception.Message/stack and the failing feed/document URL before continuing; keep the current behavior of skipping the failed feed/document after logging.Providers/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.cs (2)
238-363: Significant code duplication with EnvironmentCanadaWeatherAlertProvider.The methods
CloneAlertsForSource,MapCategory,MapSeverity,MapUrgency,MapCertainty,MapStatus, andParseAreaFilterare nearly identical to those inEnvironmentCanadaWeatherAlertProvider. Consider extracting these to a shared static helper class (e.g.,WeatherAlertMapper) to reduce duplication and ensure consistent mapping behavior.As per coding guidelines, prefer composition and separate state from behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.cs` around lines 238 - 363, The duplicated mapping and utility methods (CloneAlertsForSource, MapCategory, MapSeverity, MapUrgency, MapCertainty, MapStatus, ParseAreaFilter, GetStringProp, GetDateProp) should be extracted into a shared static helper (suggested name WeatherAlertMapper) and consumed by both MeteoAlarmWeatherAlertProvider and EnvironmentCanadaWeatherAlertProvider; create WeatherAlertMapper with matching public/static methods that reference the same enum types (WeatherAlertCategory, WeatherAlertSeverity, WeatherAlertUrgency, WeatherAlertCertainty, WeatherAlertStatus) and keep CloneAlertsForSource signature (List<WeatherAlert>, WeatherAlertSource), then replace the local implementations in both providers with calls to WeatherAlertMapper.<methodName> to remove duplication and centralize mapping logic.
57-61: Silent exception swallowing hinders diagnostics.Swallowing exceptions silently makes production debugging difficult. Consider logging at
DebugorWarninglevel to aid troubleshooting when individual countries fail.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.cs` around lines 57 - 61, In MeteoAlarmWeatherAlertProvider (inside the loop that fetches per-country alerts) replace the empty catch(Exception) block with a logged warning/debug entry that includes the exception and the country identifier so failures aren’t swallowed; call the existing logger instance (e.g., _logger or Logger) to log a message like "Failed to fetch MeteoAlarm alerts for {country}" with the exception object at Debug or Warning level, then continue as before to process remaining countries.Tests/Resgrid.Tests/Mocks/MockTrainingUserRepository.cs (1)
84-85:DeleteMultipleAsyncis currently a no-op and can mask test failures.Returning
truewithout mutating_usersweakens test fidelity for bulk-delete flows.💡 Proposed fix
public Task<bool> DeleteMultipleAsync(TrainingUser entity, string parentKeyName, object parentKeyId, List<object> ids, CancellationToken cancellationToken) - => Task.FromResult(true); +{ + if (ids == null || ids.Count == 0) + return Task.FromResult(true); + + var idSet = new HashSet<int>(ids.OfType<int>()); + _users.RemoveAll(u => idSet.Contains(u.TrainingUserId)); + return Task.FromResult(true); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Mocks/MockTrainingUserRepository.cs` around lines 84 - 85, The mock method DeleteMultipleAsync in MockTrainingUserRepository currently returns true without changing _users, which hides bugs; update DeleteMultipleAsync(TrainingUser entity, string parentKeyName, object parentKeyId, List<object> ids, CancellationToken cancellationToken) to remove any entries from the repository's backing collection (_users) whose Id (or other identifying key on TrainingUser) is contained in ids, and return true/false based on whether the expected items were removed (e.g., removed count equals ids.Count); ensure you use the same identifier property used elsewhere in MockTrainingUserRepository to match items and handle null/empty ids safely.Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs (1)
363-403: Consider usingint.TryParsefor form input parsing.The
int.Parse(group)andint.Parse(role)calls (lines 367, 388) will throwFormatExceptionif the form data contains non-numeric values. UsingTryParsewould be more defensive.Proposed fix for group parsing
foreach (var group in groups) { - if (!string.IsNullOrWhiteSpace(group)) + if (!string.IsNullOrWhiteSpace(group) && int.TryParse(group, out var groupId)) { - var members = await _departmentGroupsService.GetAllMembersForGroupAsync(int.Parse(group)); + var members = await _departmentGroupsService.GetAllMembersForGroupAsync(groupId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs` around lines 363 - 403, The parsing of form inputs uses int.Parse on variables `group` and `role` which can throw if non-numeric; update the loops in the TrainingsController method to use int.TryParse (e.g., parse into local ints like groupId and roleId) and only call `_departmentGroupsService.GetAllMembersForGroupAsync(groupId)` and `_personnelRolesService.GetAllMembersOfRoleAsync(roleId)` when TryParse returns true, otherwise skip that entry; preserve the existing logic that initializes `existingTraining.Users` and checks `existingTraining.Users.All(x => x.UserId != member.UserId)` before adding new TrainingUser entries.
🤖 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/TrainingService.cs`:
- Around line 96-97: The method in TrainingService currently returns null when a
training is missing, which breaks callers that dereference model.Training in
TrainingsController; change the missing-training behavior in the TrainingService
method (e.g., GetTraining / GetTrainingById) to not return null but instead
throw a clear exception (such as KeyNotFoundException or a domain-specific
NotFoundException) so controllers can catch it and produce a proper 404, or
alternatively restore the prior contract by returning a non-null placeholder
only if that matches previous behavior; update TrainingService (the method that
contains "if (training == null) return null;") accordingly.
In
`@Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs`:
- Around line 233-237: The current code in
EnvironmentCanadaWeatherAlertProvider.cs uses geocodes.ToDictionary(g => g.Name,
g => g.Value) inside the block that sets alert.Geocodes, which will throw an
ArgumentException on duplicate geocode names and cause the outer handler to skip
the document; change this to build a dictionary that tolerates duplicates (e.g.,
group by g.Name and pick a single value or aggregate values into a list) before
serializing, or use a ToDictionary overload/pattern that overwrites duplicates
(last-wins), so that the conversion of geocodes for alert.Geocodes never throws
for duplicate Name keys.
In `@Providers/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.cs`:
- Around line 105-106: The method in MeteoAlarmWeatherAlertProvider.cs is
mutating the caller's object by assigning response.Headers.ETag.Tag to
source.LastETag; instead, avoid side-effects by not changing source
directly—capture the ETag value (response.Headers.ETag.Tag) and return it as
part of the method's result (e.g., add an ETag/LastETag field to the returned
payload or wrapper) or populate it on a new copy/explicit output parameter;
update the calling code to read the returned ETag rather than relying on
source.LastETag so source remains immutable and shared callers are not impacted.
In `@Tests/Resgrid.Tests/Mocks/MockTrainingRepository.cs`:
- Around line 24-27: The GetByIdAsync mock currently does an unsafe cast (var
intId = (int)id) which will throw for null or non-int inputs; update
GetByIdAsync to defensively validate the id parameter (use pattern matching like
"if (!(id is int intId)) return Task.FromResult<Training>(null);" or
equivalent), then use intId to find the training from _trainings (FirstOrDefault
on TrainingId) and return Task.FromResult(training); ensure the method never
throws on bad input and consistently returns a completed Task<Training>.
In `@Tests/Resgrid.Tests/Mocks/MockTrainingUserRepository.cs`:
- Around line 21-27: The GetByIdAsync method currently casts id with (int)id
which throws on null or non-int inputs; change it to guard the input by checking
that id is an int (e.g., use "is int intId") and if not return
Task.FromResult<TrainingUser>(null) so the mock safely returns null for
unsupported ids; update the implementation inside
MockTrainingUserRepository.GetByIdAsync to use the guarded intId when querying
_users for TrainingUser where u.TrainingUserId == intId.
In `@Tests/Resgrid.Tests/Services/TrainingServiceTests.cs`:
- Around line 376-383: The test
SetTrainingAsViewedAsync_Should_Return_Null_For_NonExistent_User currently calls
_trainingService.SetTrainingAsViewedAsync(999, TestData.Users.TestUser1Id) which
supplies a non-existent training id instead of a non-existent user; either
change the call to pass a valid training id and a non-existent user id (e.g.
_trainingService.SetTrainingAsViewedAsync(TestData.Trainings.<ExistingTrainingId>,
999)) or rename the test to match the current scenario, ensuring you reference
the test method SetTrainingAsViewedAsync_Should_Return_Null_For_NonExistent_User
and the method under test _trainingService.SetTrainingAsViewedAsync when making
the fix.
In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs`:
- Around line 305-307: The code synchronously reads the uploaded file into
uploadedFile using stream.Read; update the async controller to perform an
asynchronous read by replacing stream.Read(...) with await
stream.ReadAsync(uploadedFile, 0, uploadedFile.Length) (optionally with
.ConfigureAwait(false)) within the async action/method that contains using var
stream = file.OpenReadStream(); to avoid blocking threads and keep consistency
with the async method.
In `@Web/Resgrid.Web/Areas/User/Views/Trainings/New.cshtml`:
- Around line 200-213: Build the resgridTrainingsI18n object on the server and
emit safe JSON instead of interpolating '@localizer[...]' literals; create a C#
anonymous object (e.g. var i18n = new { selectGroups =
localizer["SelectGroupsPlaceholder"].Value, selectRoles =
localizer["SelectRolesPlaceholder"].Value, ... }) and serialize it with
System.Text.Json.JsonSerializer.Serialize, then output it into the script via
Html.Raw(JsonSerializer.Serialize(i18n)) so resgridTrainingsI18n is valid
JSON-safe JS (replace the current literal entries inside the <script> block with
the serialized output).
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js`:
- Around line 39-49: The recipient selector enable/disable logic only runs on
the '#SendToAll' change event so if the page loads with '#SendToAll' already
checked the select2 inputs '#groupsToAdd', '#rolesToAdd', and '#usersToAdd' can
be in the wrong state; fix by invoking the same logic on initial render—either
call the change handler for '#SendToAll' once after it is bound or extract the
enable/disable block into a function (e.g., updateRecipientSelectors) and call
it both from the $('#SendToAll').change(...) handler and immediately after
binding to ensure the three selects are correctly enabled/disabled on page load.
- Line 94: Summary: The generated answer table reuses id="addAnswerButton"
causing duplicate IDs; change it to a unique id per table and update any
selectors/handlers. Fix: when building answersTable (the string that creates
answersTable_ + count) replace id="addAnswerButton" with a unique id like
id="addAnswerButton_' + count + '" (or better, give buttons a shared class like
"addAnswerButton" and a data-count attribute) and then update any code that
selects or binds to "#addAnswerButton" to use the new per-count id or the
class/data-count pattern so resgrid.training.edittraining.addAnswer(count)
continues to be invoked correctly.
- Around line 99-119: The generate(4) function can produce duplicate IDs causing
answer input name/id collisions; replace this non-deterministic generator with a
guaranteed-unique scheme (e.g., a module-level incremental counter or a
timestamp-based string) and update addAnswer to call the new generator;
specifically modify the generate function (and any callers like addAnswer which
sets answerForQuestion_<count>_<id>) to produce a unique token per invocation
(use a global answerIdCounter++ or Date.now()+counter) so
answerForQuestion_<n>_<uniqueId> will never collide.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.newtraining.js`:
- Line 83: The variable addAnswerTooltip is incorrectly sourcing
i18n.addQuestionTooltip; update the assignment so it uses a dedicated key
i18n.addAnswerTooltip (with an appropriate fallback like 'Add Answer' or 'Add
Answers to Question') to ensure the add-answer button gets the correct localized
string—modify the line that sets addAnswerTooltip to reference
i18n.addAnswerTooltip instead of i18n.addQuestionTooltip.
- Around line 76-94: The i18n strings are injected directly into concatenated
HTML in functions addQuestion, generateAnswersTable, and addAnswer (e.g.,
data-original-title, button text, data-bv-notempty-message), which can break the
DOM or allow injection; fix by HTML-encoding/escaping all i18n values before
concatenation (or build these elements with DOM APIs and set text/attributes via
textContent/setAttribute) — add/choose a single helper like escapeHtml(... ) and
apply it to removeTooltip, addAnswerLabel, addAnswerTooltip, correctLabel,
answerTextLabel, answerRequired, removeAnswerTooltip and any other i18n usage in
newtraining.addQuestion, newtraining.generateAnswersTable and
newtraining.addAnswer so translated quotes/markup are safe.
---
Outside diff comments:
In `@Core/Resgrid.Services/EmailService.cs`:
- Around line 229-238: The loop that builds the protocols string (variable
protocols) from call.Protocols is appending empty/whitespace protocol.Data
values which yields extra commas in the NatureOfCall output; update the logic to
skip any protocol where String.IsNullOrWhiteSpace(protocol.Data) (e.g., filter
call.Protocols first or only append when protocol.Data has content) so only
non-empty values are concatenated (or use string.Join on call.Protocols.Where(p
=> !String.IsNullOrWhiteSpace(p.Data)).Select(p => p.Data)); apply the same
change to the second occurrence around the block at 306-315.
---
Nitpick comments:
In
`@Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs`:
- Around line 116-137: The current loop in EnvironmentCanadaWeatherAlertProvider
that splits content and uses IndexOf/Substring (variables: lines, hrefStart,
hrefEnd, href, capUrls, feedUrl) is brittle; replace it with a robust HTML parse
of the feed HTML (e.g., HtmlAgilityPack or a safe HTML parser) to select all <a>
elements and extract their href attributes, normalizing quotes and resolving
relative URLs against feedUrl and only adding those that end with ".cap"
(case-insensitive); ensure you also handle single-quoted attributes and trim
values before adding to capUrls.
- Around line 63-88: In EnvironmentCanadaWeatherAlertProvider, the two empty
catch blocks around FetchCapUrlsFromFeedAsync(feedUrl, ct) and
FetchAndParseCapDocumentAsync(capUrl, source, ct) swallow exceptions without
context; update those catch blocks to log the exception and relevant identifiers
(e.g., feedUrl and capUrl) using the provider's logger (e.g., _logger or
existing logging facility) so you capture exception.Message/stack and the
failing feed/document URL before continuing; keep the current behavior of
skipping the failed feed/document after logging.
In `@Providers/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.cs`:
- Around line 238-363: The duplicated mapping and utility methods
(CloneAlertsForSource, MapCategory, MapSeverity, MapUrgency, MapCertainty,
MapStatus, ParseAreaFilter, GetStringProp, GetDateProp) should be extracted into
a shared static helper (suggested name WeatherAlertMapper) and consumed by both
MeteoAlarmWeatherAlertProvider and EnvironmentCanadaWeatherAlertProvider; create
WeatherAlertMapper with matching public/static methods that reference the same
enum types (WeatherAlertCategory, WeatherAlertSeverity, WeatherAlertUrgency,
WeatherAlertCertainty, WeatherAlertStatus) and keep CloneAlertsForSource
signature (List<WeatherAlert>, WeatherAlertSource), then replace the local
implementations in both providers with calls to WeatherAlertMapper.<methodName>
to remove duplication and centralize mapping logic.
- Around line 57-61: In MeteoAlarmWeatherAlertProvider (inside the loop that
fetches per-country alerts) replace the empty catch(Exception) block with a
logged warning/debug entry that includes the exception and the country
identifier so failures aren’t swallowed; call the existing logger instance
(e.g., _logger or Logger) to log a message like "Failed to fetch MeteoAlarm
alerts for {country}" with the exception object at Debug or Warning level, then
continue as before to process remaining countries.
In `@Providers/Resgrid.Providers.Weather/WeatherAlertResponseCache.cs`:
- Around line 17-26: The code in WeatherAlertResponseCache.cs uses
_cache.TryGetValue(key, out var entry) then unconditionally calls
_cache.TryRemove(key, out _) which can remove a freshly set value; change the
removal to the value-matching overload _cache.TryRemove(key, entry) so the entry
is only removed if it still matches the previously read entry (use the same
local variable entry from TryGetValue and the
ConcurrentDictionary.TryRemove(key, value) overload available in .NET 5+).
In `@Tests/Resgrid.Tests/Mocks/MockTrainingUserRepository.cs`:
- Around line 84-85: The mock method DeleteMultipleAsync in
MockTrainingUserRepository currently returns true without changing _users, which
hides bugs; update DeleteMultipleAsync(TrainingUser entity, string
parentKeyName, object parentKeyId, List<object> ids, CancellationToken
cancellationToken) to remove any entries from the repository's backing
collection (_users) whose Id (or other identifying key on TrainingUser) is
contained in ids, and return true/false based on whether the expected items were
removed (e.g., removed count equals ids.Count); ensure you use the same
identifier property used elsewhere in MockTrainingUserRepository to match items
and handle null/empty ids safely.
In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs`:
- Around line 363-403: The parsing of form inputs uses int.Parse on variables
`group` and `role` which can throw if non-numeric; update the loops in the
TrainingsController method to use int.TryParse (e.g., parse into local ints like
groupId and roleId) and only call
`_departmentGroupsService.GetAllMembersForGroupAsync(groupId)` and
`_personnelRolesService.GetAllMembersOfRoleAsync(roleId)` when TryParse returns
true, otherwise skip that entry; preserve the existing logic that initializes
`existingTraining.Users` and checks `existingTraining.Users.All(x => x.UserId !=
member.UserId)` before adding new TrainingUser entries.
🪄 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: 3d141f0c-8985-4af9-aa66-bf9ced47cf43
⛔ Files ignored due to path filters (9)
Core/Resgrid.Localization/Areas/User/Trainings/Trainings.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Trainings/Trainings.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Trainings/Trainings.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Trainings/Trainings.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Trainings/Trainings.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Trainings/Trainings.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Trainings/Trainings.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Trainings/Trainings.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Trainings/Trainings.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (24)
Core/Resgrid.Localization/Areas/User/Trainings/Trainings.csCore/Resgrid.Services/CommunicationService.csCore/Resgrid.Services/EmailService.csCore/Resgrid.Services/SmsService.csCore/Resgrid.Services/TrainingService.csProviders/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.csProviders/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.csProviders/Resgrid.Providers.Weather/WeatherAlertResponseCache.csTests/Resgrid.Tests/Mocks/MockTrainingAttachmentRepository.csTests/Resgrid.Tests/Mocks/MockTrainingQuestionRepository.csTests/Resgrid.Tests/Mocks/MockTrainingRepository.csTests/Resgrid.Tests/Mocks/MockTrainingUserRepository.csTests/Resgrid.Tests/Services/TrainingServiceTests.csWeb/Resgrid.Web/Areas/User/Controllers/TrainingsController.csWeb/Resgrid.Web/Areas/User/Models/Training/EditTrainingModel.csWeb/Resgrid.Web/Areas/User/Views/Home/Dashboard.cshtmlWeb/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Trainings/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Trainings/New.cshtmlWeb/Resgrid.Web/Areas/User/Views/Trainings/Quiz.cshtmlWeb/Resgrid.Web/Areas/User/Views/Trainings/Report.cshtmlWeb/Resgrid.Web/Areas/User/Views/Trainings/View.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.jsWeb/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.newtraining.js
Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs
Show resolved
Hide resolved
Comment on lines
+105
to
+106
| if (response.Headers.ETag != null) | ||
| source.LastETag = response.Headers.ETag.Tag; |
Contributor
There was a problem hiding this comment.
Mutating input parameter is an unexpected side effect.
Modifying source.LastETag directly mutates the caller's object. This side effect may be unintended if source is cached or shared. If ETag persistence is needed, consider returning it as part of the result or using an explicit output mechanism.
As per coding guidelines, "Prefer pure methods over methods with side effects."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Providers/Resgrid.Providers.Weather/MeteoAlarmWeatherAlertProvider.cs` around
lines 105 - 106, The method in MeteoAlarmWeatherAlertProvider.cs is mutating the
caller's object by assigning response.Headers.ETag.Tag to source.LastETag;
instead, avoid side-effects by not changing source directly—capture the ETag
value (response.Headers.ETag.Tag) and return it as part of the method's result
(e.g., add an ETag/LastETag field to the returned payload or wrapper) or
populate it on a new copy/explicit output parameter; update the calling code to
read the returned ETag rather than relying on source.LastETag so source remains
immutable and shared callers are not impacted.
Comment on lines
+24
to
+27
| public Task<Training> GetByIdAsync(object id) | ||
| { | ||
| var intId = (int)id; | ||
| var training = _trainings.FirstOrDefault(t => t.TrainingId == intId); |
Contributor
There was a problem hiding this comment.
GetByIdAsync should avoid unsafe object-to-int cast.
Using (int)id can throw on null/non-int inputs. A defensive type check makes this mock more robust.
💡 Proposed fix
public Task<Training> GetByIdAsync(object id)
{
- var intId = (int)id;
+ if (id is not int intId)
+ return Task.FromResult<Training>(null);
var training = _trainings.FirstOrDefault(t => t.TrainingId == intId);
return Task.FromResult(training);
}📝 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 Task<Training> GetByIdAsync(object id) | |
| { | |
| var intId = (int)id; | |
| var training = _trainings.FirstOrDefault(t => t.TrainingId == intId); | |
| public Task<Training> GetByIdAsync(object id) | |
| { | |
| if (id is not int intId) | |
| return Task.FromResult<Training>(null); | |
| var training = _trainings.FirstOrDefault(t => t.TrainingId == intId); | |
| return Task.FromResult(training); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Mocks/MockTrainingRepository.cs` around lines 24 - 27,
The GetByIdAsync mock currently does an unsafe cast (var intId = (int)id) which
will throw for null or non-int inputs; update GetByIdAsync to defensively
validate the id parameter (use pattern matching like "if (!(id is int intId))
return Task.FromResult<Training>(null);" or equivalent), then use intId to find
the training from _trainings (FirstOrDefault on TrainingId) and return
Task.FromResult(training); ensure the method never throws on bad input and
consistently returns a completed Task<Training>.
Comment on lines
+21
to
+27
| public Task<TrainingUser> GetByIdAsync(object id) | ||
| { | ||
| var intId = (int)id; | ||
| var user = _users.FirstOrDefault(u => u.TrainingUserId == intId); | ||
| return Task.FromResult(user); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Guard GetByIdAsync against invalid id input types.
(int)id throws for null/non-int values. Returning null for unsupported ids makes the mock safer and closer to repository behavior under bad inputs.
💡 Proposed fix
public Task<TrainingUser> GetByIdAsync(object id)
{
- var intId = (int)id;
+ if (id is not int intId)
+ return Task.FromResult<TrainingUser>(null);
var user = _users.FirstOrDefault(u => u.TrainingUserId == intId);
return Task.FromResult(user);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Mocks/MockTrainingUserRepository.cs` around lines 21 -
27, The GetByIdAsync method currently casts id with (int)id which throws on null
or non-int inputs; change it to guard the input by checking that id is an int
(e.g., use "is int intId") and if not return Task.FromResult<TrainingUser>(null)
so the mock safely returns null for unsupported ids; update the implementation
inside MockTrainingUserRepository.GetByIdAsync to use the guarded intId when
querying _users for TrainingUser where u.TrainingUserId == intId.
Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js
Outdated
Show resolved
Hide resolved
Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js
Outdated
Show resolved
Hide resolved
Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js
Show resolved
Hide resolved
Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.newtraining.js
Outdated
Show resolved
Hide resolved
Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.newtraining.js
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs (1)
47-61: Consider fetching previous day's alerts near midnight UTC.Using only the current UTC date means alerts from the previous day become inaccessible immediately at midnight UTC. If
FetchAlertsAsyncruns shortly after midnight, recently-posted alerts from the previous day may be missed during the brief window before new alerts are posted.♻️ Proposed enhancement to include previous day near midnight
- var today = DateTime.UtcNow.ToString("yyyyMMdd"); - var feedUrls = new List<string>(); + var now = DateTime.UtcNow; + var dates = new List<string> { now.ToString("yyyyMMdd") }; + + // Include previous day's alerts within first hour of the day to avoid gaps + if (now.Hour < 1) + dates.Add(now.AddDays(-1).ToString("yyyyMMdd")); + + var feedUrls = new List<string>(); if (provinces.Length > 0) { - foreach (var province in provinces) + foreach (var date in dates) + foreach (var province in provinces) { - feedUrls.Add($"{baseUrl}/{today}/{province.ToUpperInvariant()}/"); + feedUrls.Add($"{baseUrl}/{date}/{province.ToUpperInvariant()}/"); } } else { - feedUrls.Add($"{baseUrl}/{today}/"); + foreach (var date in dates) + feedUrls.Add($"{baseUrl}/{date}/"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs` around lines 47 - 61, The provider currently constructs feedUrls using only the current UTC date (variable today) which can miss alerts posted on the previous UTC day when FetchAlertsAsync runs just after midnight; update EnvironmentCanadaWeatherAlertProvider (where today and feedUrls are built) to also include the previous day's feed URL(s) (calculate DateTime.UtcNow.AddDays(-1).ToString("yyyyMMdd")) — either always add the previous-day URL or add it conditionally when DateTime.UtcNow.TimeOfDay is within a small window after midnight — and ensure you apply the same province/non-province logic when appending the previous-day URL(s).Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs (1)
288-298: Consider extracting the file extension allowlist to a shared constant.The extension validation logic is duplicated between the
New(lines 71-76) andEdit(lines 293-298) actions. Note that theEditaction correctly usesPath.GetExtension()which is more robust than the substring approach inNew.♻️ Suggested refactor to eliminate duplication
private static readonly HashSet<string> AllowedExtensions = new(StringComparer.OrdinalIgnoreCase) { "jpg", "jpeg", "png", "gif", "pdf", "doc", "docx", "ppt", "pptx", "pps", "ppsx", "odt", "xls", "xlsx", "txt", "mpg", "avi", "mpeg" }; private const long MaxAttachmentSize = 30_000_000; private bool IsAllowedExtension(string extension) => AllowedExtensions.Contains(extension);Then use in both actions:
-if (extension != "jpg" && extension != "jpeg" && ...) +if (!IsAllowedExtension(extension)) ModelState.AddModelError("fileToUpload", ...);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs` around lines 288 - 298, The file extension allowlist is duplicated between TrainingsController.New and TrainingsController.Edit and should be centralized: add a private static readonly HashSet<string> AllowedExtensions (use StringComparer.OrdinalIgnoreCase) and a private bool IsAllowedExtension(string extension) helper (and optionally MaxAttachmentSize constant), replace the inline extension checks in both New and Edit with calls to IsAllowedExtension (Edit should continue to use Path.GetExtension and trim the leading dot before checking); update both actions to use the shared constant to eliminate duplication and ensure consistent behavior.Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js (1)
103-117: Answer ID generation is now deterministic.The
generate()function uses an incremental counter (++_answerIdCounter) instead of random generation, eliminating the risk of form-key collisions. Note thatgenerate(4)at line 104 passes an unused argument—consider removing it for clarity.🧹 Minor cleanup
function addAnswer(count) { - var id = generate(4); + var id = generate();🤖 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/training/resgrid.training.edittraining.js` around lines 103 - 117, The addAnswer function calls generate(4) but the generate function ignores its parameter and uses a deterministic counter (_answerIdCounter), so remove the unused argument to avoid confusion: update the call in addAnswer to just generate() (or alternatively update generate to accept and use a parameter if randomness was intended); ensure the unique symbols referenced are the addAnswer function's call to generate and the generate function (and _answerIdCounter) so the change is applied consistently.Web/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtml (1)
226-226: Input type mismatch between server-rendered and JS-generated questions.Server-rendered questions use
<input type="text">(line 226), butaddQuestion()in JS generates<textarea>. This causes visual inconsistency. Consider aligning them.🧹 Use textarea for consistency
-<input type="text" name="question_@qIndex" value="@question.Question" class="form-control" /> +<textarea name="question_@qIndex" rows="4" cols="40" class="form-control">@question.Question</textarea>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtml` at line 226, Server-rendered question uses <input type="text"> while the client addQuestion() function generates a <textarea>, causing UI inconsistency; update the server markup in Edit.cshtml to render a <textarea name="question_@qIndex" class="form-control"> with the question text as its inner content (HTML-encoded) so it matches the textarea created by addQuestion(), and ensure the name pattern and CSS class match the JS-generated element.
🤖 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.Weather/EnvironmentCanadaWeatherAlertProvider.cs`:
- Around line 106-112: The HttpRequestMessage instance named request in
EnvironmentCanadaWeatherAlertProvider should be disposed after use; wrap its
creation/usage in a using statement or a using declaration (e.g., using var
request = new HttpRequestMessage(...)) so that
request.Headers.UserAgent.ParseAdd(...), _httpClient.SendAsync(request, ct), and
response handling occur while the request is automatically disposed; update the
method containing this request to ensure proper disposal of request.
- Around line 146-152: The HttpRequestMessage named request (and the
HttpResponseMessage response returned from _httpClient.SendAsync) are not being
disposed in the method that creates "var request = new
HttpRequestMessage(HttpMethod.Get, capUrl)"; wrap the request and response in
using blocks or use C# using-declarations so both request and response are
disposed (e.g., using var request = new HttpRequestMessage(...); using var
response = await _httpClient.SendAsync(request, ct);), then call
response.EnsureSuccessStatusCode() and await
response.Content.ReadAsStringAsync() as before; mirror the same disposal pattern
used in FetchCapUrlsFromFeedAsync to fix the leak.
In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs`:
- Line 371: Replace the unsafe int.Parse calls for the variables group and role
in the TrainingsController action with int.TryParse, validate the results, and
handle parse failures (e.g., return a BadRequest or skip processing) instead of
allowing a FormatException; specifically change the call that passes
int.Parse(group) to _departmentGroupsService.GetAllMembersForGroupAsync to use a
parsed groupId via int.TryParse, and apply the same pattern to the role parsing
at the other location so both group and role are defensively validated before
use.
- Line 428: The current LINQ that builds questions (the line starting with
List<int> questions = ... in the TrainingsController New action) uses int.Parse
on form key suffixes which can throw for malformed keys; change the logic to
extract the suffix, use int.TryParse to safely parse it, and only include values
where TryParse succeeds (apply the same defensive change to the similar
occurrence around the other line noted). Locate the LINQ that references keys
starting with "question_" and replace the direct int.Parse with a pattern that
obtains the suffix string, calls int.TryParse into an out variable, and selects
the out int when parsing returns true.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js`:
- Around line 110-112: removeQuestion fails for dynamically added rows because
addQuestion creates rows without the id used by removeQuestion
(`#questionRow_`<index>); update addQuestion to assign a unique id (e.g.,
"questionRow_"+qIndex or a generated index) to the newly appended row element so
removeQuestion(index) can find and remove it, or alternatively change
removeQuestion to accept an element reference and remove the row via DOM
traversal (e.g., $(element).closest('tr').remove()) and update addQuestion's
delete control to call removeQuestion(this) instead; modify either addQuestion
or removeQuestion (functions named addQuestion and removeQuestion and the
selector "#questionRow_") so both approaches use the same removal mechanism.
---
Nitpick comments:
In
`@Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs`:
- Around line 47-61: The provider currently constructs feedUrls using only the
current UTC date (variable today) which can miss alerts posted on the previous
UTC day when FetchAlertsAsync runs just after midnight; update
EnvironmentCanadaWeatherAlertProvider (where today and feedUrls are built) to
also include the previous day's feed URL(s) (calculate
DateTime.UtcNow.AddDays(-1).ToString("yyyyMMdd")) — either always add the
previous-day URL or add it conditionally when DateTime.UtcNow.TimeOfDay is
within a small window after midnight — and ensure you apply the same
province/non-province logic when appending the previous-day URL(s).
In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs`:
- Around line 288-298: The file extension allowlist is duplicated between
TrainingsController.New and TrainingsController.Edit and should be centralized:
add a private static readonly HashSet<string> AllowedExtensions (use
StringComparer.OrdinalIgnoreCase) and a private bool IsAllowedExtension(string
extension) helper (and optionally MaxAttachmentSize constant), replace the
inline extension checks in both New and Edit with calls to IsAllowedExtension
(Edit should continue to use Path.GetExtension and trim the leading dot before
checking); update both actions to use the shared constant to eliminate
duplication and ensure consistent behavior.
In `@Web/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtml`:
- Line 226: Server-rendered question uses <input type="text"> while the client
addQuestion() function generates a <textarea>, causing UI inconsistency; update
the server markup in Edit.cshtml to render a <textarea name="question_@qIndex"
class="form-control"> with the question text as its inner content (HTML-encoded)
so it matches the textarea created by addQuestion(), and ensure the name pattern
and CSS class match the JS-generated element.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js`:
- Around line 103-117: The addAnswer function calls generate(4) but the generate
function ignores its parameter and uses a deterministic counter
(_answerIdCounter), so remove the unused argument to avoid confusion: update the
call in addAnswer to just generate() (or alternatively update generate to accept
and use a parameter if randomness was intended); ensure the unique symbols
referenced are the addAnswer function's call to generate and the generate
function (and _answerIdCounter) so the change is applied consistently.
🪄 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: c604f530-5357-4707-85af-ca41dce8d137
📒 Files selected for processing (6)
Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.csWeb/Resgrid.Web/Areas/User/Controllers/TrainingsController.csWeb/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Trainings/New.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.jsWeb/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.newtraining.js
🚧 Files skipped from review as they are similar to previous changes (2)
- Web/Resgrid.Web/Areas/User/Views/Trainings/New.cshtml
- Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.newtraining.js
Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs
Outdated
Show resolved
Hide resolved
Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs
Outdated
Show resolved
Hide resolved
Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs (3)
340-415: Consider initializingUserscollection once before the conditionals.The null check and initialization of
existingTraining.Usersis repeated four times (lines 345, 362, 382, 403). Moving this to a single initialization before theif (model.SendToAll)block would reduce duplication.♻️ Suggested simplification
+ // Initialize Users collection once if needed + if (existingTraining.Users == null) + existingTraining.Users = new List<TrainingUser>(); + if (model.SendToAll) { var allUsers = await _departmentsService.GetAllUsersForDepartmentAsync(DepartmentId); foreach (var user in allUsers) { - if (existingTraining.Users == null) - existingTraining.Users = new List<TrainingUser>(); - if (existingTraining.Users.All(x => x.UserId != user.UserId)) {Apply the same removal to the other three locations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs` around lines 340 - 415, The Users collection on existingTraining is being initialized repeatedly; before the if (model.SendToAll) block, ensure existingTraining.Users is non-null by assigning new List<TrainingUser>() if null, then remove the repeated null checks inside the SendToAll branch and in the user/group/role loops (references: existingTraining.Users, model.SendToAll, TrainingUser, and the loops that iterate users, groups, and roles), leaving only the duplicate-avoidance checks (existingTraining.Users.All(...)) and adding users via existingTraining.Users.Add(...).
506-530: Good null check and TryParse additions.The defensive parsing at lines 527-530 prevents exceptions from malformed form keys.
However, line 541 (unchanged) still uses
int.Parse(answerForQuestion)on user-submitted form data, which can throwFormatExceptionif tampered. Consider applying the same defensive pattern for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs` around lines 506 - 530, In the Quiz POST action, avoid using int.Parse on user-submitted values (answerForQuestion) to prevent FormatException; replace the int.Parse(answerForQuestion) call in the Quiz method with int.TryParse and handle a failed parse by skipping that answer or treating it as incorrect (e.g., continue the loop or count as wrong), mirroring the defensive parsing used for question IDs so the method (Quiz and variables answerForQuestion, correctAnswers) no longer throws on tampered form input.
460-476: RedundantTrainingQuestionIdassignment on new answer.Line 465 sets
trainingQuestionAnswer.TrainingQuestionId = question.TrainingQuestionId, but sincequestionis newly created, itsTrainingQuestionIdis 0. EF Core will establish the FK relationship through the navigation property when the answer is added toquestion.Answers. TheNewaction (lines 200-217) correctly relies on this behavior without explicit FK assignment.♻️ Suggested fix for consistency with New action
var trainingQuestionAnswer = new TrainingQuestionAnswer(); var answerForQuestion = form["answerForQuestion_" + i + "_" + answer]; var possibleAnswer = form["answer_" + i]; trainingQuestionAnswer.Answer = answerForQuestion; - trainingQuestionAnswer.TrainingQuestionId = question.TrainingQuestionId; if (!string.IsNullOrWhiteSpace(possibleAnswer))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs` around lines 460 - 476, Remove the redundant explicit FK assignment to TrainingQuestionId when building answers for a newly created question: in the TrainingsController where TrainingQuestionAnswer instances are created and added to question.Answers, delete the line that sets trainingQuestionAnswer.TrainingQuestionId = question.TrainingQuestionId and rely on EF Core to fix up the relationship via the navigation property (question.Answers) just like the New action does; keep setting trainingQuestionAnswer.Answer and trainingQuestionAnswer.Correct, then add to question.Answers and existingTraining.Questions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs`:
- Around line 340-415: The Users collection on existingTraining is being
initialized repeatedly; before the if (model.SendToAll) block, ensure
existingTraining.Users is non-null by assigning new List<TrainingUser>() if
null, then remove the repeated null checks inside the SendToAll branch and in
the user/group/role loops (references: existingTraining.Users, model.SendToAll,
TrainingUser, and the loops that iterate users, groups, and roles), leaving only
the duplicate-avoidance checks (existingTraining.Users.All(...)) and adding
users via existingTraining.Users.Add(...).
- Around line 506-530: In the Quiz POST action, avoid using int.Parse on
user-submitted values (answerForQuestion) to prevent FormatException; replace
the int.Parse(answerForQuestion) call in the Quiz method with int.TryParse and
handle a failed parse by skipping that answer or treating it as incorrect (e.g.,
continue the loop or count as wrong), mirroring the defensive parsing used for
question IDs so the method (Quiz and variables answerForQuestion,
correctAnswers) no longer throws on tampered form input.
- Around line 460-476: Remove the redundant explicit FK assignment to
TrainingQuestionId when building answers for a newly created question: in the
TrainingsController where TrainingQuestionAnswer instances are created and added
to question.Answers, delete the line that sets
trainingQuestionAnswer.TrainingQuestionId = question.TrainingQuestionId and rely
on EF Core to fix up the relationship via the navigation property
(question.Answers) just like the New action does; keep setting
trainingQuestionAnswer.Answer and trainingQuestionAnswer.Correct, then add to
question.Answers and existingTraining.Questions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bf85ec8-4d8b-4f53-96b3-28bbf199b1cd
📒 Files selected for processing (3)
Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.csWeb/Resgrid.Web/Areas/User/Controllers/TrainingsController.csWeb/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js
🚧 Files skipped from review as they are similar to previous changes (2)
- Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js
- Providers/Resgrid.Providers.Weather/EnvironmentCanadaWeatherAlertProvider.cs
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests