feat(training): Add edit functionality for training module#319
Open
waylonkenning wants to merge 106 commits intoResgrid:developfrom
Open
feat(training): Add edit functionality for training module#319waylonkenning wants to merge 106 commits intoResgrid:developfrom
waylonkenning wants to merge 106 commits intoResgrid:developfrom
Conversation
CU-3wen6jw fixing worker issue
CU-3wen6jw adding permission checks for users
CU-868a1kgf7 update for android push
CU-868a1kgf7 fixing iOS critical notification bug.
CU-868a3neq0 trying to fix eventing issue.
CU-868a5nz1n fixed wrong url for messages email
CU-868a5nz1n setting new default for error logger (console)
CU-868a5nz1n trying to fix pricing page.
CU-868a7khdq making subscription page a bit more clear.
CU-868a7khdq Changing how stripeCustomerId empty are handled
CU-868a7khdq trying to fix sub bug
CU-868ba84kp fixing bug with db container reg
CU-8687aj3gt Adjusting angular build
CU-8687aj3gt Working on Novu api call provider
CU-8687aj3gt Working on more Novu push work
CU-3w8z67w Fixing docker paths
CU-3w8z67w Working on lib issue.
CU-868d85dv8 Fixed missing connection string issues.
CU-868d85dv8 Fixing docker typo
CU-868d85dv8 More upgrade work and posthog work.
CU-868d85dv8 fixed url shortner to use kutt
RE1-T100 Fixing tests
RE1-T60 Updated to SCIM and SSO flow
RE1-T46 Fixing bug with action logs query.
RE1-T46 bug fixes
RE1-T46 Bug fixes
RE1-T46 Flagged report, select unit and report bug fix.
RE1-T46 Trying to fix Sentry issue
RE1-T106 Notification page fix
RE1-T105 More custom maps work
RE1-T105 Fixing map issue
RE1-T105 Map fixes
RE1-T105 Fixing issues with routes.
RE1-T105 Added Languages to missing pages, gdpr support for export, e…
RE1-T107 Added checkin timers to calls
…all/Station The commit RE1-T104 switched from Kendo UI Windows to Bootstrap modals but didn't add the required modal HTML structure. Kendo's kendoWindow() could auto-create the window on empty divs, but Bootstrap modals require actual HTML with .modal, .modal-dialog, .modal-content classes. This fix adds proper Bootstrap modal HTML for both respondToACallWindow and respondToAStationWindow modals in Dashboard.cshtml. Fixes Resgrid#317
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
|
Thanks for opening this pull request! A contributor should be by to give feedback soon. In the meantime, please check out the contributing guidelines. |
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
![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: 11
🧹 Nitpick comments (4)
Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js (1)
91-101: Consider using a more robust unique ID generator.The
generate()function creates a 4-digit numeric ID by picking random numbers 1-20 without repeats, then joining them. This has a limited collision space and the logic is somewhat convoluted.♻️ Simpler approach using timestamp or counter
function generate(length) { - var arr = []; - var n; - for (var i = 0; i < length; i++) { - do - n = Math.floor(Math.random() * 20 + 1); - while (arr.indexOf(n) !== -1); - arr[i] = n; - } - return arr.join(''); + return Date.now().toString(36) + Math.random().toString(36).substr(2, 5); }🤖 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 91 - 101, The generate() function builds a small, collision-prone ID by sampling unique numbers 1–20—replace it with a simpler, more robust unique ID approach: remove the array/loop logic in generate() and instead return a combination of a high-resolution timestamp and random entropy (e.g., Date.now() plus a few random hex digits or use crypto.getRandomValues where available) or integrate a lightweight UUID generator; update any call sites expecting a 4-digit string if necessary and keep the function name generate for compatibility.Web/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtml (1)
222-225: Consider also fixing related Razor interpolations in the questions section.Similar to the answers, the question row elements should use explicit parentheses for consistency and safety.
♻️ Proposed fix
-<tr id="questionRow_@qIndex"> +<tr id="questionRow_@(qIndex)"> <td> - <input type="hidden" name="questionId_@qIndex" value="@question.TrainingQuestionId" /> - <input type="text" name="question_@qIndex" value="@question.Question" class="form-control" /> + <input type="hidden" name="questionId_@(qIndex)" value="@question.TrainingQuestionId" /> + <input type="text" name="question_@(qIndex)" value="@question.Question" class="form-control" /> </td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtml` around lines 222 - 225, The Razor markup in the question row should use explicit interpolation parentheses for consistency and safety: update the id and input name/value attributes in the question row (elements referencing questionRow_@qIndex, questionId_@qIndex, question_@qIndex and values using question.TrainingQuestionId and question.Question) to use the @(...) form (e.g., questionRow_@(qIndex), name="questionId_@(qIndex)", value="@(question.TrainingQuestionId)", name="question_@(qIndex)", value="@(question.Question)") so all dynamic parts are wrapped with parentheses like the answers section.Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs (1)
274-312: Consider extracting duplicate attachment handling logic.The attachment validation and processing logic (extension check, size validation, byte reading) is nearly identical between
New(lines 58-92) andEdit(lines 274-311). Consider extracting to a private helper method.Also applies to: 54-93
🤖 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 274 - 312, Extract the duplicated attachment handling in TrainingsController (used in the New and Edit actions) into a single private helper, e.g., ProcessAttachments(IEnumerable<IFormFile> attachments, Training existingTraining, int trainingId, ModelStateDictionary modelState), that: checks null/empty, ensures existingTraining.Attachments is initialized, validates file extension (preserving the allowed list and lowercasing), validates file size (<30MB) and adds ModelState errors exactly as currently done, reads the file stream into a byte[] safely (use stream.Length/Read pattern once) and creates/truncates a TrainingAttachment (FileType, FileName, TrainingId, Data) then adds it to existingTraining.Attachments; replace the duplicated blocks in both New and Edit with a call to this helper so behavior and error reporting remain identical.Tests/Resgrid.Tests/Services/TrainingServiceTests.cs (1)
54-142: Consider extracting a shared training fixture/builder to reduce Arrange duplication.The repeated
new Training { ... }setup makes test maintenance harder; a small helper would keep intent clearer and reduce drift.Also applies to: 172-204, 284-293, 313-322, 346-355, 393-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Resgrid.Tests/Services/TrainingServiceTests.cs` around lines 54 - 142, The tests duplicate Training object construction; create a small test helper/builder (e.g., TrainingBuilder or a static TestData factory method) to produce Training instances with sensible defaults and optional overrides, then replace inline new Training { ... } calls used with _trainingRepository.SeedTraining in tests like GetAllTrainingsForDepartmentAsync_Should_Return_Trainings_For_Department and the other listed test blocks (references: Training, _trainingRepository.SeedTraining, and the test methods), so each Arrange becomes a single call like SeedTraining(TrainingBuilder.Create(deptId:1, name:"Training 1").WithOverrides(...).Build()) to reduce duplication and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/Resgrid.Tests/Mocks/MockTrainingQuestionRepository.cs`:
- Around line 81-82: The DeleteMultipleAsync implementation currently
short-circuits and returns true without mutating the in-memory store, which can
hide regressions; update the MockTrainingQuestionRepository.DeleteMultipleAsync
method to actually remove entries from the _questions collection that belong to
the specified parent (matching parentKeyName/parentKeyId) and whose IDs are in
the provided ids list, respect the CancellationToken, and return true if all
requested IDs were removed (or false if any were not found) so tests accurately
reflect deletion behavior.
- Around line 90-101: SeedQuestion currently appends a TrainingQuestion
regardless of duplicate TrainingQuestionId which can produce duplicate entries
and stale reads from GetByIdAsync; update SeedQuestion in
MockTrainingQuestionRepository to check _questions for an existing question with
the same TrainingQuestionId and replace/update that entry instead of adding a
new one, and still adjust _nextId using Math.Max(_nextId,
question.TrainingQuestionId + 1) when an explicit id is provided; ensure
GetByIdAsync continues to query the single authoritative entry (or the replaced
entry) so consumers get the latest seeded object.
In `@Tests/Resgrid.Tests/Services/TrainingServiceTests.cs`:
- Around line 376-383: The test is checking the "non-existent user" path but
calls SetTrainingAsViewedAsync with a non-existent training id (999), so it
never exercises the missing-user branch; update the test to call
_trainingService.SetTrainingAsViewedAsync with a valid existing training id
(e.g., TestData.Trainings.Training1Id or any arranged training created in the
test fixture) and pass the non-existent user id (999 or a constant) instead,
ensuring the arranged training exists in the test setup (or create one) so
SetTrainingAsViewedAsync(...) truly executes the missing-user logic and the
result.Should().BeNull() assertion remains valid.
In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs`:
- Around line 263-264: The POST Edit action (Edit in TrainingsController) is
missing antiforgery validation; add the [ValidateAntiForgeryToken] attribute
above the public async Task<IActionResult> Edit(int trainingId,
EditTrainingModel model, IFormCollection form, ICollection<IFormFile>
attachments) method so the server validates the `@Html.AntiForgeryToken`() from
the form (mirror the approach used on the Quiz POST action).
- Around line 283-284: The code that extracts file extensions using
file.FileName.Substring(...) can throw ArgumentOutOfRangeException when there is
no '.' in the filename; update both locations in TrainingsController (the
file-upload handling in the New action and the other action where extension is
computed) to safely obtain the extension using
System.IO.Path.GetExtension(file.FileName) (or check that
file.FileName.IndexOf('.') >= 0 before using Substring), then normalize the
result (trim leading '.') and handle empty/null extension cases appropriately
before further processing.
- Around line 305-306: The code calls file.OpenReadStream() twice and never
disposes either stream; replace that with a single using (or await using) around
a single var stream = file.OpenReadStream(), then read the entirety into
uploadedFile (e.g., allocate by stream.Length or copy into a MemoryStream) and
dispose the stream; update the logic around the uploadedFile variable in
TrainingsController to use the single stream and ensure proper disposal to avoid
the resource leak.
- Around line 417-418: The fix: don't set existingTraining.Questions = null
because SyncChildArrayUpdates skips deletion when the collection is null;
instead either explicitly delete the old question records before rebuilding or
keep the collection and replace its items. Locate the block that assigns
existingTraining.Questions = null in TrainingsController (around the
question-handling code) and either call the repository method that removes
questions for the training (e.g., delete existing training questions via the
repo) prior to adding new ones, or clear and repopulate
existingTraining.Questions (e.g., existingTraining.Questions.Clear() then add
new Question instances) so SyncChildArrayUpdates can detect and remove or update
or ensure orphaned questions are deleted.
In `@Web/Resgrid.Web/Areas/User/Models/Training/EditTrainingModel.cs`:
- Around line 4-12: The EditTrainingModel class is declared inside the
Resgrid.Web.Areas.User.Models.Training namespace which collides with the
Resgrid.Model.Training type (causing CS0118); fix by either fully qualifying the
property type (change the Training property to use the full type name e.g.
Resgrid.Model.Training) or rename the namespace (e.g.,
Resgrid.Web.Areas.User.Models.Trainings) so the Training identifier in
EditTrainingModel.Training resolves to the intended class rather than a
namespace; update any references to EditTrainingModel if you rename the
namespace.
In `@Web/Resgrid.Web/Areas/User/Views/Home/Dashboard.cshtml`:
- Around line 225-248: The modal containers with id="respondToACallWindow" and
id="respondToAStationWindow" no longer include the legacy CSS classes
.respondToACallWindow and .respondToAStationWindow which breaks JS event
handlers in resgrid.home.dashboard.js and resgrid.dispatch.smallcallgrid.js;
restore those class attributes on the respective outer modal divs (the same
elements that have the IDs) so the existing .on(...) and .trigger(...) selectors
find them again, i.e., add class="respondToACallWindow" to the
respondToACallWindow modal container and class="respondToAStationWindow" to the
respondToAStationWindow modal container.
In `@Web/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtml`:
- Around line 234-235: The Razor interpolation for the radio input value and the
text input name uses adjacent underscores with `@qIndex` and `@aIndex` which can
parse incorrectly; update both occurrences of
"answerForQuestion_@qIndex_@aIndex" (the value attribute on the radio and the
name attribute on the text input) to use explicit Razor expressions like
answerForQuestion_@(qIndex)_@(aIndex) so qIndex and aIndex are evaluated
correctly; keep the radio group name "answer_@qIndex" (or similarly change to
answer_@(qIndex) for consistency) and leave the checked logic @(answer.Correct ?
"checked" : "") as-is.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js`:
- Around line 70-76: The questionsCount is blindly set to 0 causing index
collisions with server-rendered questions; instead, initialize
resgrid.training.edittraining.questionsCount based on existing question rows
when the DOM is ready (e.g., count rows under '#questions tbody' or compute the
max existing question_N index) so addQuestion() increments from the correct
starting value; update the initialization in the document-ready block that
currently sets resgrid.training.edittraining.questionsCount = 0 to derive the
count from the DOM before any calls to edittraining.addQuestion and keep
addQuestion, generateAnswersTable and the existing element IDs unchanged.
---
Nitpick comments:
In `@Tests/Resgrid.Tests/Services/TrainingServiceTests.cs`:
- Around line 54-142: The tests duplicate Training object construction; create a
small test helper/builder (e.g., TrainingBuilder or a static TestData factory
method) to produce Training instances with sensible defaults and optional
overrides, then replace inline new Training { ... } calls used with
_trainingRepository.SeedTraining in tests like
GetAllTrainingsForDepartmentAsync_Should_Return_Trainings_For_Department and the
other listed test blocks (references: Training,
_trainingRepository.SeedTraining, and the test methods), so each Arrange becomes
a single call like SeedTraining(TrainingBuilder.Create(deptId:1, name:"Training
1").WithOverrides(...).Build()) to reduce duplication and improve readability.
In `@Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs`:
- Around line 274-312: Extract the duplicated attachment handling in
TrainingsController (used in the New and Edit actions) into a single private
helper, e.g., ProcessAttachments(IEnumerable<IFormFile> attachments, Training
existingTraining, int trainingId, ModelStateDictionary modelState), that: checks
null/empty, ensures existingTraining.Attachments is initialized, validates file
extension (preserving the allowed list and lowercasing), validates file size
(<30MB) and adds ModelState errors exactly as currently done, reads the file
stream into a byte[] safely (use stream.Length/Read pattern once) and
creates/truncates a TrainingAttachment (FileType, FileName, TrainingId, Data)
then adds it to existingTraining.Attachments; replace the duplicated blocks in
both New and Edit with a call to this helper so behavior and error reporting
remain identical.
In `@Web/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtml`:
- Around line 222-225: The Razor markup in the question row should use explicit
interpolation parentheses for consistency and safety: update the id and input
name/value attributes in the question row (elements referencing
questionRow_@qIndex, questionId_@qIndex, question_@qIndex and values using
question.TrainingQuestionId and question.Question) to use the @(...) form (e.g.,
questionRow_@(qIndex), name="questionId_@(qIndex)",
value="@(question.TrainingQuestionId)", name="question_@(qIndex)",
value="@(question.Question)") so all dynamic parts are wrapped with parentheses
like the answers section.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js`:
- Around line 91-101: The generate() function builds a small, collision-prone ID
by sampling unique numbers 1–20—replace it with a simpler, more robust unique ID
approach: remove the array/loop logic in generate() and instead return a
combination of a high-resolution timestamp and random entropy (e.g., Date.now()
plus a few random hex digits or use crypto.getRandomValues where available) or
integrate a lightweight UUID generator; update any call sites expecting a
4-digit string if necessary and keep the function name generate for
compatibility.
🪄 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: 518de8e0-1ce8-46be-8bc9-baf55a44670a
📒 Files selected for processing (11)
Tests/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/wwwroot/js/app/internal/training/resgrid.training.edittraining.js
Comment on lines
+81
to
+82
| public Task<bool> DeleteMultipleAsync(TrainingQuestion entity, string parentKeyName, object parentKeyId, List<object> ids, CancellationToken cancellationToken) | ||
| => Task.FromResult(true); |
Contributor
There was a problem hiding this comment.
DeleteMultipleAsync is a no-op and can mask edit-flow bugs in tests.
This currently returns success without deleting anything from _questions, so tests can pass even when deletion logic regresses.
Proposed fix
public Task<bool> DeleteMultipleAsync(TrainingQuestion 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.Select(System.Convert.ToInt32));
+ _questions.RemoveAll(q => idSet.Contains(q.TrainingQuestionId));
+ 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/MockTrainingQuestionRepository.cs` around lines 81
- 82, The DeleteMultipleAsync implementation currently short-circuits and
returns true without mutating the in-memory store, which can hide regressions;
update the MockTrainingQuestionRepository.DeleteMultipleAsync method to actually
remove entries from the _questions collection that belong to the specified
parent (matching parentKeyName/parentKeyId) and whose IDs are in the provided
ids list, respect the CancellationToken, and return true if all requested IDs
were removed (or false if any were not found) so tests accurately reflect
deletion behavior.
Comment on lines
+90
to
+101
| public void SeedQuestion(TrainingQuestion question) | ||
| { | ||
| if (question.TrainingQuestionId == 0) | ||
| { | ||
| question.TrainingQuestionId = _nextId++; | ||
| } | ||
| else | ||
| { | ||
| _nextId = System.Math.Max(_nextId, question.TrainingQuestionId + 1); | ||
| } | ||
| _questions.Add(question); | ||
| } |
Contributor
There was a problem hiding this comment.
SeedQuestion can introduce duplicate IDs and stale reads.
Seeding the same TrainingQuestionId more than once appends duplicates; GetByIdAsync then returns the first match, which may be stale.
Proposed fix
public void SeedQuestion(TrainingQuestion question)
{
+ var existing = _questions.FirstOrDefault(q => q.TrainingQuestionId == question.TrainingQuestionId);
+ if (existing != null)
+ {
+ _questions.Remove(existing);
+ }
+
if (question.TrainingQuestionId == 0)
{
question.TrainingQuestionId = _nextId++;
}
else
{
_nextId = System.Math.Max(_nextId, question.TrainingQuestionId + 1);
}
_questions.Add(question);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Mocks/MockTrainingQuestionRepository.cs` around lines 90
- 101, SeedQuestion currently appends a TrainingQuestion regardless of duplicate
TrainingQuestionId which can produce duplicate entries and stale reads from
GetByIdAsync; update SeedQuestion in MockTrainingQuestionRepository to check
_questions for an existing question with the same TrainingQuestionId and
replace/update that entry instead of adding a new one, and still adjust _nextId
using Math.Max(_nextId, question.TrainingQuestionId + 1) when an explicit id is
provided; ensure GetByIdAsync continues to query the single authoritative entry
(or the replaced entry) so consumers get the latest seeded object.
Comment on lines
+376
to
+383
| public async Task SetTrainingAsViewedAsync_Should_Return_Null_For_NonExistent_User() | ||
| { | ||
| // Act | ||
| var result = await _trainingService.SetTrainingAsViewedAsync(999, TestData.Users.TestUser1Id); | ||
|
|
||
| // Assert | ||
| result.Should().BeNull(); | ||
| } |
Contributor
There was a problem hiding this comment.
Test intent and setup don’t match for “non-existent user”.
This test currently uses a non-existent training ID (999) with no arranged data, so it can pass without exercising the missing-user path.
Proposed fix
[Test]
public async Task SetTrainingAsViewedAsync_Should_Return_Null_For_NonExistent_User()
{
+ // Arrange
+ _trainingRepository.SeedTraining(new Training
+ {
+ TrainingId = 1,
+ DepartmentId = 1,
+ Name = "Training",
+ Description = "Description",
+ TrainingText = "Text",
+ CreatedByUserId = TestData.Users.TestUser1Id,
+ CreatedOn = DateTime.UtcNow
+ });
+
+ _userRepository.SeedUser(new TrainingUser
+ {
+ TrainingUserId = 1,
+ TrainingId = 1,
+ UserId = TestData.Users.TestUser2Id,
+ Viewed = false,
+ Complete = false
+ });
+
// Act
- var result = await _trainingService.SetTrainingAsViewedAsync(999, TestData.Users.TestUser1Id);
+ var result = await _trainingService.SetTrainingAsViewedAsync(1, TestData.Users.TestUser1Id);
// Assert
result.Should().BeNull();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Services/TrainingServiceTests.cs` around lines 376 - 383,
The test is checking the "non-existent user" path but calls
SetTrainingAsViewedAsync with a non-existent training id (999), so it never
exercises the missing-user branch; update the test to call
_trainingService.SetTrainingAsViewedAsync with a valid existing training id
(e.g., TestData.Trainings.Training1Id or any arranged training created in the
test fixture) and pass the non-existent user id (999 or a constant) instead,
ensuring the arranged training exists in the test setup (or create one) so
SetTrainingAsViewedAsync(...) truly executes the missing-user logic and the
result.Should().BeNull() assertion remains valid.
Comment on lines
+263
to
+264
| [HttpPost] | ||
| public async Task<IActionResult> Edit(int trainingId, EditTrainingModel model, IFormCollection form, ICollection<IFormFile> attachments) |
Contributor
There was a problem hiding this comment.
Missing [ValidateAntiForgeryToken] attribute on POST Edit.
The form includes @Html.AntiForgeryToken() but the controller action doesn't validate it. Compare with the Quiz POST action at line 496 which has [ValidateAntiForgeryToken].
🔒 Proposed fix
[HttpPost]
+[ValidateAntiForgeryToken]
public async Task<IActionResult> Edit(int trainingId, EditTrainingModel model, IFormCollection form, ICollection<IFormFile> attachments)📝 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
| [HttpPost] | |
| public async Task<IActionResult> Edit(int trainingId, EditTrainingModel model, IFormCollection form, ICollection<IFormFile> attachments) | |
| [HttpPost] | |
| [ValidateAntiForgeryToken] | |
| public async Task<IActionResult> Edit(int trainingId, EditTrainingModel model, IFormCollection form, ICollection<IFormFile> attachments) |
🤖 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
263 - 264, The POST Edit action (Edit in TrainingsController) is missing
antiforgery validation; add the [ValidateAntiForgeryToken] attribute above the
public async Task<IActionResult> Edit(int trainingId, EditTrainingModel model,
IFormCollection form, ICollection<IFormFile> attachments) method so the server
validates the `@Html.AntiForgeryToken`() from the form (mirror the approach used
on the Quiz POST action).
Comment on lines
+417
to
+418
| // Handle questions - remove existing and add new ones | ||
| existingTraining.Questions = null; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how TrainingService.SaveAsync handles questions - look for cascade delete or explicit removal
rg -n -C5 'SaveAsync|Questions' --type=cs -g '*TrainingService*.cs'Repository: Resgrid/Core
Length of output: 18650
🏁 Script executed:
sed -n '400,450p' Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.csRepository: Resgrid/Core
Length of output: 2146
🏁 Script executed:
rg -n -A10 'SaveOrUpdateAsync' Core/Resgrid.Services/TrainingService.cs | head -30Repository: Resgrid/Core
Length of output: 1068
🏁 Script executed:
# Search for Training repository implementation to see cascade delete configuration
fd -t f 'ITrainingRepository\|TrainingRepository' --type=cs | head -5Repository: Resgrid/Core
Length of output: 225
🏁 Script executed:
find . -name "*TrainingRepository*" -type fRepository: Resgrid/Core
Length of output: 239
🏁 Script executed:
# Also search for DbContext or entity configurations
rg -l 'DbContext|OnModelCreating' --type=cs | grep -i training | head -5Repository: Resgrid/Core
Length of output: 38
🏁 Script executed:
rg -n -A15 'SaveOrUpdateAsync' ./Repositories/Resgrid.Repositories.DataRepository/TrainingRepository.csRepository: Resgrid/Core
Length of output: 38
🏁 Script executed:
# Also check the Training entity model for cascade delete configuration
rg -n -B5 -A10 'class Training' Core/Resgrid.Model/Entities/Training.cs | head -40Repository: Resgrid/Core
Length of output: 135
🏁 Script executed:
find . -name "Training.cs" -type f | grep -v Test | grep -v binRepository: Resgrid/Core
Length of output: 88
🏁 Script executed:
cat ./Repositories/Resgrid.Repositories.DataRepository/TrainingRepository.csRepository: Resgrid/Core
Length of output: 4884
🏁 Script executed:
find . -name "RepositoryBase.cs" -type fRepository: Resgrid/Core
Length of output: 124
🏁 Script executed:
cat ./Core/Resgrid.Model/Training.cs | head -80Repository: Resgrid/Core
Length of output: 1736
🏁 Script executed:
rg -n -A20 'SaveOrUpdateAsync' ./Repositories/Resgrid.Repositories.DataRepository/RepositoryBase.cs | head -50Repository: Resgrid/Core
Length of output: 2220
🏁 Script executed:
rg -n -A25 'private async Task<T> UpdateAsync' ./Repositories/Resgrid.Repositories.DataRepository/RepositoryBase.cs | head -60Repository: Resgrid/Core
Length of output: 38
🏁 Script executed:
rg -n 'async Task.*Update' ./Repositories/Resgrid.Repositories.DataRepository/RepositoryBase.csRepository: Resgrid/Core
Length of output: 401
🏁 Script executed:
wc -l ./Repositories/Resgrid.Repositories.DataRepository/RepositoryBase.csRepository: Resgrid/Core
Length of output: 128
🏁 Script executed:
sed -n '213,280p' ./Repositories/Resgrid.Repositories.DataRepository/RepositoryBase.csRepository: Resgrid/Core
Length of output: 1788
🏁 Script executed:
sed -n '488,542p' ./Repositories/Resgrid.Repositories.DataRepository/RepositoryBase.csRepository: Resgrid/Core
Length of output: 1861
Explicitly delete existing questions before nullifying the collection, or populate the collection with new questions instead.
Setting existingTraining.Questions = null leaves existing question records orphaned in the database. The SyncChildArrayUpdates method in the repository layer only removes orphaned questions when the collection is non-null; it skips deletion logic entirely when the collection itself is null. Either delete old questions explicitly before rebuilding, or repopulate the existing collection rather than nullifying it first.
🤖 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
417 - 418, The fix: don't set existingTraining.Questions = null because
SyncChildArrayUpdates skips deletion when the collection is null; instead either
explicitly delete the old question records before rebuilding or keep the
collection and replace its items. Locate the block that assigns
existingTraining.Questions = null in TrainingsController (around the
question-handling code) and either call the repository method that removes
questions for the training (e.g., delete existing training questions via the
repo) prior to adding new ones, or clear and repopulate
existingTraining.Questions (e.g., existingTraining.Questions.Clear() then add
new Question instances) so SyncChildArrayUpdates can detect and remove or update
or ensure orphaned questions are deleted.
Comment on lines
+234
to
+235
| <input type="radio" name="answer_@qIndex" value="answerForQuestion_@qIndex_@aIndex" @(answer.Correct ? "checked" : "") /> Correct | ||
| <input type="text" name="answerForQuestion_@qIndex_@aIndex" value="@answer.Answer" class="form-control" style="display:inline-block; width:auto;" /> |
Contributor
There was a problem hiding this comment.
Razor interpolation may produce incorrect form field names.
The expression answerForQuestion_@qIndex_@aIndex could be parsed incorrectly by Razor due to underscores adjacent to @ symbols. This may cause the radio button values to not match the expected answerForQuestion_N_M pattern that the controller parses.
🐛 Proposed fix: Use explicit parentheses for Razor expressions
-<input type="radio" name="answer_@qIndex" value="answerForQuestion_@qIndex_@aIndex" @(answer.Correct ? "checked" : "") /> Correct
-<input type="text" name="answerForQuestion_@qIndex_@aIndex" value="@answer.Answer" class="form-control" style="display:inline-block; width:auto;" />
+<input type="radio" name="answer_@(qIndex)" value="answerForQuestion_@(qIndex)_@(aIndex)" @(answer.Correct ? "checked" : "") /> Correct
+<input type="text" name="answerForQuestion_@(qIndex)_@(aIndex)" value="@answer.Answer" class="form-control" style="display:inline-block; width:auto;" />📝 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
| <input type="radio" name="answer_@qIndex" value="answerForQuestion_@qIndex_@aIndex" @(answer.Correct ? "checked" : "") /> Correct | |
| <input type="text" name="answerForQuestion_@qIndex_@aIndex" value="@answer.Answer" class="form-control" style="display:inline-block; width:auto;" /> | |
| <input type="radio" name="answer_@(qIndex)" value="answerForQuestion_@(qIndex)_@(aIndex)" @(answer.Correct ? "checked" : "") /> Correct | |
| <input type="text" name="answerForQuestion_@(qIndex)_@(aIndex)" value="@answer.Answer" class="form-control" style="display:inline-block; width:auto;" /> |
🤖 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` around lines 234 -
235, The Razor interpolation for the radio input value and the text input name
uses adjacent underscores with `@qIndex` and `@aIndex` which can parse incorrectly;
update both occurrences of "answerForQuestion_@qIndex_@aIndex" (the value
attribute on the radio and the name attribute on the text input) to use explicit
Razor expressions like answerForQuestion_@(qIndex)_@(aIndex) so qIndex and
aIndex are evaluated correctly; keep the radio group name "answer_@qIndex" (or
similarly change to answer_@(qIndex) for consistency) and leave the checked
logic @(answer.Correct ? "checked" : "") as-is.
Comment on lines
+70
to
+76
| resgrid.training.edittraining.questionsCount = 0; | ||
| }); | ||
| function addQuestion() { | ||
| resgrid.training.edittraining.questionsCount++; | ||
| $('#questions tbody').first().append("<tr><td style='max-width: 215px;'><textarea id='question_" + edittraining.questionsCount + "' name='question_" + edittraining.questionsCount + "' rows='4' cols='40'></textarea></td><td>" + resgrid.training.edittraining.generateAnswersTable(edittraining.questionsCount) + "</td><td style='text-align:center;'><a onclick='$(this).parent().parent().remove();' class='tip-top' data-original-title='Remove this question'><i class='fa fa-minus' style='color: red;'></i></a></td></tr>"); | ||
| } | ||
| edittraining.addQuestion = addQuestion; |
Contributor
There was a problem hiding this comment.
Question index collision when adding new questions to existing training.
questionsCount is initialized to 0, but the server-rendered questions in Edit.cshtml use indices starting from 0. When addQuestion() is called, it increments to 1 and creates question_1. If the training already has questions at indices 0, 1, 2, the new question will overwrite question_1's form data.
🐛 Proposed fix: Initialize questionsCount from existing questions
In the $(document).ready block, count existing question rows before setting questionsCount:
initSelect2('#groupsToAdd', 'Select groups...', resgrid.absoluteBaseUrl + '/User/Department/GetRecipientsForGrid?filter=1');
initSelect2('#rolesToAdd', 'Select roles...', resgrid.absoluteBaseUrl + '/User/Department/GetRecipientsForGrid?filter=2');
initSelect2('#usersToAdd', 'Select users...', resgrid.absoluteBaseUrl + '/User/Department/GetRecipientsForGrid?filter=3&filterSelf=true');
-resgrid.training.edittraining.questionsCount = 0;
+// Initialize questionsCount based on existing server-rendered questions to avoid index collisions
+resgrid.training.edittraining.questionsCount = $('#questions tbody tr').length;🤖 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 70 - 76, The questionsCount is blindly set to 0 causing index
collisions with server-rendered questions; instead, initialize
resgrid.training.edittraining.questionsCount based on existing question rows
when the DOM is ready (e.g., count rows under '#questions tbody' or compute the
max existing question_N index) so addQuestion() increments from the correct
starting value; update the initialization in the document-ready block that
currently sets resgrid.training.edittraining.questionsCount = 0 to derive the
count from the DOM before any calls to edittraining.addQuestion and keep
addQuestion, generateAnswersTable and the existing element IDs unchanged.
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>
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
2 participants
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
This PR implements editable training articles as requested in #180, allowing department admins to modify training content after creation.
Fixes #180
Changes
New Features
Files Added
Tests/Resgrid.Tests/Mocks/MockTrainingRepository.cs- In-memory repository for unit testsTests/Resgrid.Tests/Mocks/MockTrainingAttachmentRepository.cs- Mock for attachment queriesTests/Resgrid.Tests/Mocks/MockTrainingQuestionRepository.cs- Mock for question queriesTests/Resgrid.Tests/Mocks/MockTrainingUserRepository.cs- Mock for user assignment queriesTests/Resgrid.Tests/Services/TrainingServiceTests.cs- 15 unit tests for service layerWeb/Resgrid.Web/Areas/User/Models/Training/EditTrainingModel.cs- View modelWeb/Resgrid.Web/Areas/User/Views/Trainings/Edit.cshtml- Edit form viewWeb/Resgrid.Web/wwwroot/js/app/internal/training/resgrid.training.edittraining.js- JavaScript for edit pageFiles Modified
Web/Resgrid.Web/Areas/User/Controllers/TrainingsController.cs- Added Edit GET/POST actionsWeb/Resgrid.Web/Areas/User/Views/Trainings/Index.cshtml- Added Edit button for adminsIssue Requirements Addressed
From #180:
Future Enhancements (Not in this PR)
Testing
Unit Tests (15 tests)
GetTrainingByIdAsync- Returns training with questions/attachmentsGetAllTrainingsForDepartmentAsync- Filters by departmentSaveAsync- Create/update with questions, HTML sanitizationDeleteTrainingAsync- Removes trainingSetTrainingAsViewedAsync- Marks viewed, auto-completes when no questionsRecordTrainingQuizResultAsync- Records score and completionResetUserAsync- Resets user progressGetTrainingUsersForUserAsync- Gets user's assigned trainingsManual Testing Checklist
Screenshots
Edit button added to training index page next to Report and Delete buttons for department admins
Edit form pre-populates all existing training data including attachments, questions, and user assignments
Summary by CodeRabbit
New Features
Style