Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Code Review Could Not Complete
|
| Options | Enabled |
|---|---|
| Bug | ✅ |
| Performance | ✅ |
| Security | ✅ |
| Business Logic | ✅ |
Contributor
📝 WalkthroughWalkthroughThis PR adds personnel-role qualification support for unit roles and staffing, plus custom state template browsing/selection and expanded base-type values. It also updates related web UI, API, repository, and migration code to carry the new data. ChangesUnit Role Personnel Qualification and Staffing
Custom State Templates and ActionBaseTypes Expansion
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Resgrid-Bot
reviewed
Jul 2, 2026
Resgrid-Bot
reviewed
Jul 2, 2026
Resgrid-Bot
reviewed
Jul 2, 2026
Resgrid-Bot
reviewed
Jul 2, 2026
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs (1)
598-628: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
model.PersonnelRolesnot repopulated inEditUnitPOST error-recovery paths.The GET action sets
model.PersonnelRoles(Line 489), but neither of the two error-recovery re-render blocks here (UDF validation failure at Lines 598-628, and top-levelModelStateinvalid at Lines 636-663) repopulate it, even thoughmodel.UnitRolesis refreshed in both.EditUnit.cshtmlrelies onModel.PersonnelRolesfor the required-role<select>per row and for_UnitRoleTemplatesModal, so redisplaying after a duplicate-name/duplicate-role-name/UDF error will show empty personnel-role dropdowns (losing previously-selected qualifications) and pass a null/empty list into the template modal.🐛 Proposed fix (apply in both blocks)
model.UnitRoles = await _unitsService.GetRolesForUnitAsync(model.Unit.UnitId); +model.PersonnelRoles = await _personnelRolesService.GetAllRolesForDepartmentAsync(DepartmentId);Also applies to: 636-663
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 598 - 628, The EditUnit POST error-recovery paths are not repopulating model.PersonnelRoles, so the view loses the personnel-role dropdown data when redisplaying validation errors. Update both ModelState-invalid branches in the EditUnit action to reload PersonnelRoles the same way the GET action does, alongside the existing model.UnitRoles refresh. Use the EditUnit method and model.PersonnelRoles as the key points to update so EditUnit.cshtml can render the required-role selects and _UnitRoleTemplatesModal correctly.Web/Resgrid.Web.Services/Controllers/v4/UnitRolesController.cs (2)
208-250: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winReject roles outside the requested unit
GetRoleByIdAsync(...)is global, butSetRoleAssignmentsForUnitnever checksrole.UnitId == unit.UnitIdbefore validating or saving. That lets a caller use a role from another unit, bypass the newPersonnelRoleRequiredgate whenPersonnelRoleIdis null, and persist aUnitActiveRolewith mismatchedUnitId/DepartmentId.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Controllers/v4/UnitRolesController.cs` around lines 208 - 250, The role assignment flow in SetRoleAssignmentsForUnit must reject roles that do not belong to the current unit. Before both the qualification check and the save loop, verify that each role returned by GetRoleByIdAsync has a matching UnitId for the requested unit and skip or fail otherwise; this prevents cross-unit assignments and avoids persisting mismatched UnitActiveRole data. Use the existing SetRoleAssignmentsForUnit, GetRoleByIdAsync, and SaveActiveRoleAsync paths to enforce the unit ownership check consistently.
269-296: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftAvoid per-role qualification lookups in this endpoint.
GetAllUnitRolesAndAssignmentsForDepartmentcalls_unitsService.IsUserQualifiedForUnitRoleAsync(...)once per active role, and that helper loads the user's personnel roles each time. Reuse a department-level user-role map here, likeGetUnitStaffingForDepartmentAsync, to avoid repeated sequential I/O for large departments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web.Services/Controllers/v4/UnitRolesController.cs` around lines 269 - 296, The `GetAllUnitRolesAndAssignmentsForDepartment` flow is doing a per-role qualification check via `_unitsService.IsUserQualifiedForUnitRoleAsync`, which repeats personnel-role loading for every active role. Update this method to build and reuse a department-level user-role lookup (similar to `GetUnitStaffingForDepartmentAsync`) before the loop, then use that cached map when setting `role.IsQualified` instead of calling the service repeatedly. Keep the existing `UnitRolesController` loop and `ActiveUnitRoleResultData` population intact, but replace the sequential qualification lookup with the shared precomputed data.
🧹 Nitpick comments (10)
Web/Resgrid.Web/Areas/User/Views/CustomStatuses/Edit.cshtml (1)
166-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded
baseTypeoptions duplicate the enum and will drift.This "Add Option" modal still hand-lists every
ActionBaseTypesvalue/label pair, whileEditDetail.cshtml(same PR) was refactored to a model-drivenModel.BaseTypesselect list. The 12 new lines added here are exactly the kind of manual sync this duplication requires every time the enum changes — the same source list used inEditDetail.cshtmlshould be surfaced here too (e.g., viaViewBag/a view-model property populated fromEnum.GetValues<ActionBaseTypes>()).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/Edit.cshtml` around lines 166 - 196, The baseType select in CustomStatuses/Edit still hardcodes every ActionBaseTypes option, which duplicates the enum-driven list used elsewhere and will drift over time. Update the Edit view to bind the dropdown from the same model/view-data source as EditDetail.cshtml, using a shared list populated from ActionBaseTypes in the controller or view model, and keep the _BaseTypeDescription partial wired to the selected field.Core/Resgrid.Model/CustomStates/CustomStateTemplateDetail.cs (1)
41-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProjection method mixes behavior into a state model.
ToCustomStateDetail()embeds mapping/projection logic directly on the template DTO. As per coding guidelines, prefer separating state from behavior (e.g., an extension method or static mapper class) rather than instance methods on the model.♻️ Proposed extension-method refactor
- public CustomStateDetail ToCustomStateDetail() - { - return new CustomStateDetail - { - ButtonText = ButtonText, - ButtonColor = ButtonColor, - TextColor = TextColor, - BaseType = (int)BaseType, - NoteType = (int)NoteType, - DetailType = (int)DetailType, - GpsRequired = GpsRequired, - Order = Order, - TTL = Ttl, - IsDeleted = false - }; - } + // moved to a static CustomStateTemplateDetailExtensions.ToCustomStateDetail(this CustomStateTemplateDetail d) class🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Model/CustomStates/CustomStateTemplateDetail.cs` around lines 41 - 60, Move the projection logic out of CustomStateTemplateDetail.ToCustomStateDetail() and into a separate mapper, such as an extension method or static mapping helper, so the template model stays state-only. Preserve the same field-to-field mapping into CustomStateDetail, but relocate the behavior to a dedicated mapper class/method with a clear name that can be reused by callers. Update any call sites to use the new mapper instead of the instance method, and remove the method from CustomStateTemplateDetail if it becomes unused.Source: Coding guidelines
Web/Resgrid.Web/Areas/User/Views/CustomStatuses/_BaseTypeDescription.cshtml (1)
20-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDescription text duplicated between JS and the C# enum — drift risk noted in your own comment.
The comment at the top explicitly calls out the need to "Keep the descriptions below in sync with the
[Description]attributes onResgrid.Model.ActionBaseTypes" — this is a manually-maintained duplicate of server-side metadata that will silently drift over time. Consider generating this map server-side (e.g., reflect overActionBaseTypes[Description]attributes into aDictionary<int,string>and serialize it into the script) so there's a single source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/_BaseTypeDescription.cshtml` around lines 20 - 57, The base type description map in the _BaseTypeDescription view duplicates the C# ActionBaseTypes [Description] metadata and can drift. Update the script so it is generated from the server-side enum descriptions instead of hardcoding the strings in the resgridBaseTypeDescriptions initializer, using ActionBaseTypes as the single source of truth and preserving the numeric aliases.Web/Resgrid.Web/Areas/User/Views/CustomStatuses/New.cshtml (1)
81-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame hardcoded-string concern as
Templates.cshtml.The pre-fill alert text is hardcoded rather than pulled from
localizer, inconsistent with the rest of the form.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/New.cshtml` around lines 81 - 90, The pre-fill alert text in New.cshtml is hardcoded instead of using the view localizer, which is inconsistent with the rest of the form. Update the alert inside the Model.State.GetActiveDetails() block to use the same localizer pattern used elsewhere in this view or in Templates.cshtml, so the message is retrieved from localization resources rather than embedded directly in the markup.Web/Resgrid.Web/Areas/User/Views/CustomStatuses/Templates.cshtml (1)
34-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHardcoded English strings bypass the app's localization convention.
All other views in this cohort (
Index.cshtml,New.cshtml) route every user-facing string through@localizer[...]/@commonLocalizer[...], but this new file hardcodes strings like "Choose a starting point for...", the help paragraph, "options" label, "Use this template", "Start from Blank", and the no-results message directly in markup.♻️ Suggested direction
-<h5>Choose a starting point for your `@typeName.ToLower`() statuses</h5> +<h5>`@localizer`["ChooseTemplateStartingPoint", typeName]</h5>Move remaining literal strings to resource files consistent with the rest of the module.
Also applies to: 42-46, 73-73, 78-78, 87-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/Templates.cshtml` at line 34, The Templates.cshtml view is still emitting several user-facing literal strings instead of using the module’s localization pattern. Update the remaining markup in this view to route the heading, help text, “options” label, action labels like “Use this template” and “Start from Blank,” and the no-results message through the existing `@localizer` / `@commonLocalizer` usage, matching the approach used in Index.cshtml and New.cshtml so all text comes from resource files.Web/Resgrid.Web/Areas/User/Views/Units/EditUnit.cshtml (1)
142-153: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPassing
Model.PersonnelRolesunguarded to the partial, inconsistent with the null-coalescing used just below.Line 152 defensively does
Model.PersonnelRoles ?? new List<...>(), but Line 142'sHtml.PartialAsync("_UnitRoleTemplatesModal", Model.PersonnelRoles)doesn't. Root cause tracked inUnitsController.cs(POST error paths never setPersonnelRoles), but this call site would benefit from the same defensive pattern.♻️ Apply same null-coalescing as line 152
-@await Html.PartialAsync("_UnitRoleTemplatesModal", Model.PersonnelRoles) +@await Html.PartialAsync("_UnitRoleTemplatesModal", Model.PersonnelRoles ?? new List<Resgrid.Model.PersonnelRole>())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Views/Units/EditUnit.cshtml` around lines 142 - 153, The `_UnitRoleTemplatesModal` partial is being passed `Model.PersonnelRoles` without the same null guard used later in the view. Update the `Html.PartialAsync` call in `EditUnit.cshtml` to use a null-coalescing fallback for `Model.PersonnelRoles`, matching the defensive pattern already used in the `Json.Serialize` block so the partial always receives a valid list.Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs (1)
203-225: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
GetRoleByIdAsynclookups: same role fetched twice per submission.The new qualification-validation loop fetches
_unitsService.GetRoleByIdAsync(unitRoleId)for each role, then the save loop below fetches the same role again by the same id. For units with many roles this doubles the round-trips.♻️ Cache the first lookup for reuse in the save loop
+var roleCache = new Dictionary<int, UnitRole>(); foreach (var unitRoleId in unitRoles) { var candidateUserId = form[$"Role_{unitRoleId}"]; if (String.IsNullOrWhiteSpace(candidateUserId)) continue; var roleToCheck = await _unitsService.GetRoleByIdAsync(unitRoleId); + roleCache[unitRoleId] = roleToCheck; ... } ... foreach (var unitRole in unitRoles) { var unitRoleStaffingUserId = form[$"Role_{unitRole}"]; if (!String.IsNullOrWhiteSpace(unitRoleStaffingUserId)) { - var role = await _unitsService.GetRoleByIdAsync(unitRole); + if (!roleCache.TryGetValue(unitRole, out var role)) + role = await _unitsService.GetRoleByIdAsync(unitRole); ... } }Also applies to: 235-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 203 - 225, The qualification-validation loop in UnitsController is reloading each role twice by calling _unitsService.GetRoleByIdAsync(unitRoleId) here and again in the save path below. Cache the fetched role from this validation pass and reuse it in the later assignment/update loop instead of making a second lookup for the same unitRoleId, using the existing role-related variables in UnitsController to keep the flow consistent.Web/Resgrid.Web/Areas/User/Views/Units/UnitStaffing.cshtml (1)
8-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extension methods on
UnitStaffingLevelinstead of view-local static helpers.
StaffingLabelClass/StaffingLabelTextduplicate switch logic that's a natural fit forUnitStaffingLevelextension methods inCore/Resgrid.Model, making them reusable outside this view (e.g., API responses, other views).As per coding guidelines, "Use extension methods appropriately for domain-specific operations."
♻️ Example extension class
public static class UnitStaffingLevelExtensions { public static string ToLabelClass(this UnitStaffingLevel level) => level switch { UnitStaffingLevel.FullyStaffed => "label-success", UnitStaffingLevel.Degraded => "label-warning", UnitStaffingLevel.PartiallyStaffed => "label-warning", UnitStaffingLevel.NotStaffed => "label-danger", _ => "label-default" }; public static string ToLabelText(this UnitStaffingLevel level) => level switch { UnitStaffingLevel.FullyStaffed => "Fully Staffed", UnitStaffingLevel.Degraded => "Degraded", UnitStaffingLevel.PartiallyStaffed => "Partially Staffed", UnitStaffingLevel.NotStaffed => "Not Staffed", _ => "No Roles" }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Views/Units/UnitStaffing.cshtml` around lines 8 - 32, Move the staffing label mapping out of the UnitStaffing.cshtml view-local helpers and into reusable extension methods on UnitStaffingLevel in Core/Resgrid.Model. Create domain-specific methods like ToLabelClass and ToLabelText in an extension class, then update the view to call those methods instead of StaffingLabelClass and StaffingLabelText. Keep the existing switch mappings and default values, but make them available across views and API code.Source: Coding guidelines
Web/Resgrid.Web/wwwroot/js/app/internal/units/resgrid.units.newunit.js (1)
26-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated remove-row logic instead of reusing
removeRole().The new row's inline
onclick='$(this).closest("tr").remove();'(Line 34) duplicates the exact logic already in the unchangedremoveRole()function (Lines 38-40), which is now unreachable for dynamically-added rows. Prefer wiring the button to the existing helper (e.g. via a class + delegated.on('click', ...)binding, oronclick='removeRole.call(this)') to keep the removal logic in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/wwwroot/js/app/internal/units/resgrid.units.newunit.js` around lines 26 - 36, The remove action for dynamically added roles is duplicating row-deletion logic instead of reusing the existing removeRole helper. Update addRole to bind the Remove control to removeRole (or a delegated click handler that calls it) so newly added rows use the same centralized behavior as the existing removeRole function and the removal logic stays in one place.Providers/Resgrid.Providers.Migrations/Migrations/M0086_AddUnitRolePersonnelRole.cs (1)
14-21: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider an index (and/or FK) on
PersonnelRoleId.
PersonnelRoleIdreferencesPersonnelRolesbut no foreign key or index is added. GivenUnitsServicenow joins unit roles against personnel-role lookups for every staffing computation, an index would help; an FK would also prevent orphaned references if aPersonnelRolerow is deleted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Providers/Resgrid.Providers.Migrations/Migrations/M0086_AddUnitRolePersonnelRole.cs` around lines 14 - 21, The M0086_AddUnitRolePersonnelRole migration adds PersonnelRoleId to UnitRoles but does not enforce or optimize the relationship to PersonnelRoles. Update the M0086_AddUnitRolePersonnelRole.Up method to add a foreign key from UnitRoles.PersonnelRoleId to PersonnelRoles, and add an index on PersonnelRoleId so the staffing joins in UnitsService stay efficient. Use the existing UnitRoles/PersonnelRoleId symbols in the migration to locate the change and ensure the FK/index are created only when the column is added.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Model/CustomStates/CustomStateTemplate.cs`:
- Around line 31-34: The `CustomStateTemplate` type is exposing mutable shared
state through `Keywords` and `Details`, which can corrupt the singleton catalog
returned by `CustomStateTemplateCatalog.All`. Update `CustomStateTemplate` to
use read-only/immutable collection shapes for these members, and adjust any
initialization/access patterns in `CustomStateTemplate` and the catalog code so
callers can read the data but cannot mutate it after retrieval.
In `@Core/Resgrid.Model/UnitRoleStaffingResult.cs`:
- Around line 80-108: The staffing loop in UnitRoleStaffingResult is matching
active assignments only by role name, so duplicate UnitRole entries with the
same Name can reuse the same UnitActiveRole and overcount filled seats. Update
the matching logic in the foreach over roles to consume a matched active
assignment after it is used (or otherwise track used active items) so each
UnitActiveRole can satisfy only one seat. Keep the qualification check and
QualificationIssues creation on the matched assignment, but ensure subsequent
iterations cannot match the same user again through active.FirstOrDefault.
In `@Core/Resgrid.Services/UnitsService.cs`:
- Around line 695-734: `GetUnitStaffingForDepartmentAsync` is doing a per-unit
`GetRolesForUnitAsync(unit.UnitId)` lookup inside the `foreach`, causing
repeated role queries. Add a department-wide roles retrieval in `UnitsService`
and reuse it for all units in the loop, then pass the cached/shared role data
into `UnitRoleStaffingResult.Calculate` instead of querying per unit; this read
path can also be wrapped with cache-aside if it’s frequently called.
In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/Edit.cshtml`:
- Around line 183-194: The CustomStatuses dropdown in the Edit view is using new
BaseType labels that are not present in the CustomStatuses localization
resources, so they resolve to raw key text. Add the missing BaseType entries to
the CustomStatuses resource set in the CustomStatuses localization files so the
`@localizer[...]` lookups in `Edit.cshtml` can resolve correctly.
In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/New.cshtml`:
- Around line 190-203: The CustomStatuses dropdown in the New view is already
using the new ActionBaseTypes values, but the matching localization keys are
missing, so add the `Enroute`, `Transporting`, `Delivering`, `AtPatient`,
`AtHospital`, `Searching`, `Loading`, `Standby`, `OnPatrol`, `Maintenance`,
`OnBreak`, and `Completed` entries to the `CustomStatuses` resource files used
by the `CustomStatuses` view. Make sure the labels are added consistently across
the `CustomStatuses*.resx` resources so `@localizer[...]` in `New.cshtml`
resolves to translated text instead of raw keys.
In `@Web/Resgrid.Web/Areas/User/Views/Units/_UnitRoleTemplatesModal.cshtml`:
- Around line 81-121: The row counter used by addRow/nextCount is currently
shared implicitly through window.count, which can cause duplicate unitRole_<n>
ids/names when rows are added from the modal and the page-level addRole paths.
Update the counting logic so all code paths that append to `#unitRoles` use one
shared source of truth for the next row index, and make sure nextCount
reads/writes that shared counter consistently. Verify the modal script and both
unit addRole entry points do not initialize their own independent counters
before calling addRow.
In `@Web/Resgrid.Web/Areas/User/Views/Units/EditUnit.cshtml`:
- Around line 76-78: The table headers in EditUnit.cshtml mix localized content
with hardcoded English text, so update the “Required Personnel Role” and
“Required?” headers to use localization keys like the existing
`@localizer`["RoleNameHeader"] usage. Add or reuse appropriate localized resource
entries for these labels and keep the addRoleButton markup unchanged except for
any related localized text.
In `@Web/Resgrid.Web/Areas/User/Views/Units/NewUnit.cshtml`:
- Around line 71-73: The header row in NewUnit.cshtml mixes localized labels
with hardcoded English text. Update the table header cells that currently render
“Required Personnel Role” and “Required?” to use the same `@localizer`[...]
pattern as the other header/actions in this section, keeping the existing
addRoleButton and browse templates markup intact.
---
Outside diff comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitRolesController.cs`:
- Around line 208-250: The role assignment flow in SetRoleAssignmentsForUnit
must reject roles that do not belong to the current unit. Before both the
qualification check and the save loop, verify that each role returned by
GetRoleByIdAsync has a matching UnitId for the requested unit and skip or fail
otherwise; this prevents cross-unit assignments and avoids persisting mismatched
UnitActiveRole data. Use the existing SetRoleAssignmentsForUnit,
GetRoleByIdAsync, and SaveActiveRoleAsync paths to enforce the unit ownership
check consistently.
- Around line 269-296: The `GetAllUnitRolesAndAssignmentsForDepartment` flow is
doing a per-role qualification check via
`_unitsService.IsUserQualifiedForUnitRoleAsync`, which repeats personnel-role
loading for every active role. Update this method to build and reuse a
department-level user-role lookup (similar to
`GetUnitStaffingForDepartmentAsync`) before the loop, then use that cached map
when setting `role.IsQualified` instead of calling the service repeatedly. Keep
the existing `UnitRolesController` loop and `ActiveUnitRoleResultData`
population intact, but replace the sequential qualification lookup with the
shared precomputed data.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs`:
- Around line 598-628: The EditUnit POST error-recovery paths are not
repopulating model.PersonnelRoles, so the view loses the personnel-role dropdown
data when redisplaying validation errors. Update both ModelState-invalid
branches in the EditUnit action to reload PersonnelRoles the same way the GET
action does, alongside the existing model.UnitRoles refresh. Use the EditUnit
method and model.PersonnelRoles as the key points to update so EditUnit.cshtml
can render the required-role selects and _UnitRoleTemplatesModal correctly.
---
Nitpick comments:
In `@Core/Resgrid.Model/CustomStates/CustomStateTemplateDetail.cs`:
- Around line 41-60: Move the projection logic out of
CustomStateTemplateDetail.ToCustomStateDetail() and into a separate mapper, such
as an extension method or static mapping helper, so the template model stays
state-only. Preserve the same field-to-field mapping into CustomStateDetail, but
relocate the behavior to a dedicated mapper class/method with a clear name that
can be reused by callers. Update any call sites to use the new mapper instead of
the instance method, and remove the method from CustomStateTemplateDetail if it
becomes unused.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0086_AddUnitRolePersonnelRole.cs`:
- Around line 14-21: The M0086_AddUnitRolePersonnelRole migration adds
PersonnelRoleId to UnitRoles but does not enforce or optimize the relationship
to PersonnelRoles. Update the M0086_AddUnitRolePersonnelRole.Up method to add a
foreign key from UnitRoles.PersonnelRoleId to PersonnelRoles, and add an index
on PersonnelRoleId so the staffing joins in UnitsService stay efficient. Use the
existing UnitRoles/PersonnelRoleId symbols in the migration to locate the change
and ensure the FK/index are created only when the column is added.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs`:
- Around line 203-225: The qualification-validation loop in UnitsController is
reloading each role twice by calling _unitsService.GetRoleByIdAsync(unitRoleId)
here and again in the save path below. Cache the fetched role from this
validation pass and reuse it in the later assignment/update loop instead of
making a second lookup for the same unitRoleId, using the existing role-related
variables in UnitsController to keep the flow consistent.
In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/_BaseTypeDescription.cshtml`:
- Around line 20-57: The base type description map in the _BaseTypeDescription
view duplicates the C# ActionBaseTypes [Description] metadata and can drift.
Update the script so it is generated from the server-side enum descriptions
instead of hardcoding the strings in the resgridBaseTypeDescriptions
initializer, using ActionBaseTypes as the single source of truth and preserving
the numeric aliases.
In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/Edit.cshtml`:
- Around line 166-196: The baseType select in CustomStatuses/Edit still
hardcodes every ActionBaseTypes option, which duplicates the enum-driven list
used elsewhere and will drift over time. Update the Edit view to bind the
dropdown from the same model/view-data source as EditDetail.cshtml, using a
shared list populated from ActionBaseTypes in the controller or view model, and
keep the _BaseTypeDescription partial wired to the selected field.
In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/New.cshtml`:
- Around line 81-90: The pre-fill alert text in New.cshtml is hardcoded instead
of using the view localizer, which is inconsistent with the rest of the form.
Update the alert inside the Model.State.GetActiveDetails() block to use the same
localizer pattern used elsewhere in this view or in Templates.cshtml, so the
message is retrieved from localization resources rather than embedded directly
in the markup.
In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/Templates.cshtml`:
- Line 34: The Templates.cshtml view is still emitting several user-facing
literal strings instead of using the module’s localization pattern. Update the
remaining markup in this view to route the heading, help text, “options” label,
action labels like “Use this template” and “Start from Blank,” and the
no-results message through the existing `@localizer` / `@commonLocalizer` usage,
matching the approach used in Index.cshtml and New.cshtml so all text comes from
resource files.
In `@Web/Resgrid.Web/Areas/User/Views/Units/EditUnit.cshtml`:
- Around line 142-153: The `_UnitRoleTemplatesModal` partial is being passed
`Model.PersonnelRoles` without the same null guard used later in the view.
Update the `Html.PartialAsync` call in `EditUnit.cshtml` to use a
null-coalescing fallback for `Model.PersonnelRoles`, matching the defensive
pattern already used in the `Json.Serialize` block so the partial always
receives a valid list.
In `@Web/Resgrid.Web/Areas/User/Views/Units/UnitStaffing.cshtml`:
- Around line 8-32: Move the staffing label mapping out of the
UnitStaffing.cshtml view-local helpers and into reusable extension methods on
UnitStaffingLevel in Core/Resgrid.Model. Create domain-specific methods like
ToLabelClass and ToLabelText in an extension class, then update the view to call
those methods instead of StaffingLabelClass and StaffingLabelText. Keep the
existing switch mappings and default values, but make them available across
views and API code.
In `@Web/Resgrid.Web/wwwroot/js/app/internal/units/resgrid.units.newunit.js`:
- Around line 26-36: The remove action for dynamically added roles is
duplicating row-deletion logic instead of reusing the existing removeRole
helper. Update addRole to bind the Remove control to removeRole (or a delegated
click handler that calls it) so newly added rows use the same centralized
behavior as the existing removeRole function and the removal logic stays in one
place.
🪄 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: f19c2bfe-af3f-4268-a6c4-43e37fb487ec
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Services/DocumentDatabaseProviderSelectionTests.csis excluded by!**/Tests/**
📒 Files selected for processing (37)
Core/Resgrid.Model/ActionBaseTypes.csCore/Resgrid.Model/CustomStates/CustomStateTemplate.csCore/Resgrid.Model/CustomStates/CustomStateTemplateCatalog.csCore/Resgrid.Model/CustomStates/CustomStateTemplateDetail.csCore/Resgrid.Model/Reporting/AvailabilityMatrix.csCore/Resgrid.Model/Services/IUnitsService.csCore/Resgrid.Model/UnitRole.csCore/Resgrid.Model/UnitRoleStaffingResult.csCore/Resgrid.Model/UnitRoles/UnitRoleTemplate.csCore/Resgrid.Model/UnitRoles/UnitRoleTemplateCatalog.csCore/Resgrid.Model/UnitRoles/UnitRoleTemplateRole.csCore/Resgrid.Model/UnitStaffingLevel.csCore/Resgrid.Services/UnitsService.csProviders/Resgrid.Providers.Migrations/Migrations/M0086_AddUnitRolePersonnelRole.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0086_AddUnitRolePersonnelRolePg.csWeb/Resgrid.Web.Services/Controllers/v4/UnitRolesController.csWeb/Resgrid.Web.Services/Models/v4/UnitRoles/ActiveUnitRolesResult.csWeb/Resgrid.Web.Services/Models/v4/UnitRoles/UnitRoleResult.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Controllers/CustomStatusesController.csWeb/Resgrid.Web/Areas/User/Controllers/UnitsController.csWeb/Resgrid.Web/Areas/User/Models/CustomStatuses/CustomStateTemplatesView.csWeb/Resgrid.Web/Areas/User/Models/Units/NewUnitView.csWeb/Resgrid.Web/Areas/User/Models/Units/UnitStaffingView.csWeb/Resgrid.Web/Areas/User/Views/CustomStatuses/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/CustomStatuses/EditDetail.cshtmlWeb/Resgrid.Web/Areas/User/Views/CustomStatuses/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/CustomStatuses/New.cshtmlWeb/Resgrid.Web/Areas/User/Views/CustomStatuses/Templates.cshtmlWeb/Resgrid.Web/Areas/User/Views/CustomStatuses/_BaseTypeDescription.cshtmlWeb/Resgrid.Web/Areas/User/Views/Units/EditUnit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Units/NewUnit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Units/UnitStaffing.cshtmlWeb/Resgrid.Web/Areas/User/Views/Units/_UnitRoleTemplatesModal.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/statuses/resgrid.statuses.newstatus.jsWeb/Resgrid.Web/wwwroot/js/app/internal/units/resgrid.units.editunit.jsWeb/Resgrid.Web/wwwroot/js/app/internal/units/resgrid.units.newunit.js
Comment on lines
+183
to
+194
| <option value="11">@localizer["Enroute"]</option> | ||
| <option value="12">@localizer["Transporting"]</option> | ||
| <option value="13">@localizer["Delivering"]</option> | ||
| <option value="14">@localizer["AtPatient"]</option> | ||
| <option value="15">@localizer["AtHospital"]</option> | ||
| <option value="16">@localizer["Searching"]</option> | ||
| <option value="17">@localizer["Loading"]</option> | ||
| <option value="18">@localizer["Standby"]</option> | ||
| <option value="19">@localizer["OnPatrol"]</option> | ||
| <option value="20">@localizer["Maintenance"]</option> | ||
| <option value="21">@localizer["OnBreak"]</option> | ||
| <option value="22">@localizer["Completed"]</option> |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the CustomStatuses resx and check for the new keys
fd -e resx --iglob '*CustomStatuses*'
rg -n 'name="(Enroute|Transporting|Delivering|AtPatient|AtHospital|Searching|Loading|Standby|OnPatrol|Maintenance|OnBreak|Completed)"' -g '*.resx'Repository: Resgrid/Core
Length of output: 353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find likely resource files for the CustomStatuses view and inspect matching keys.
find . -type f \( -iname '*CustomStatuses*.resx' -o -iname '*Edit*.resx' -o -iname '*User*.resx' \) | sort
echo '--- matching keys ---'
rg -n 'name="(Enroute|Transporting|Delivering|AtPatient|AtHospital|Searching|Loading|Standby|OnPatrol|Maintenance|OnBreak|Completed)"' . -g '*.resx' || true
echo '--- surrounding resource context ---'
for f in $(find . -type f -iname '*.resx' | sort); do
if rg -q 'name="(Enroute|Transporting|Delivering|AtPatient|AtHospital|Searching|Loading|Standby|OnPatrol|Maintenance|OnBreak|Completed)"' "$f"; then
echo "FILE: $f"
rg -n -C 1 'name="(Enroute|Transporting|Delivering|AtPatient|AtHospital|Searching|Loading|Standby|OnPatrol|Maintenance|OnBreak|Completed)"' "$f"
fi
doneRepository: Resgrid/Core
Length of output: 7847
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- CustomStatuses resx key search ---'
rg -n 'name="(Enroute|Transporting|Delivering|AtPatient|AtHospital|Searching|Loading|Standby|OnPatrol|Maintenance|OnBreak|Completed)"' Core/Resgrid.Localization/Areas/User/CustomStatuses/*.resx || true
echo '--- CustomStatuses resource file samples ---'
for f in Core/Resgrid.Localization/Areas/User/CustomStatuses/*.resx; do
echo "FILE: $f"
rg -n -C 1 '<data name="(Enroute|Transporting|Delivering|AtPatient|AtHospital|Searching|Loading|Standby|OnPatrol|Maintenance|OnBreak|Completed)"' "$f" || true
done
echo '--- view localizer binding ---'
sed -n '1,60p' Web/Resgrid.Web/Areas/User/Views/CustomStatuses/Edit.cshtmlRepository: Resgrid/Core
Length of output: 3842
Add the new BaseType labels to Core/Resgrid.Localization/Areas/User/CustomStatuses/CustomStatuses.*.resx. These keys are missing from the CustomStatuses localizer scope, so the dropdown will fall back to the raw key text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web/Areas/User/Views/CustomStatuses/Edit.cshtml` around lines 183
- 194, The CustomStatuses dropdown in the Edit view is using new BaseType labels
that are not present in the CustomStatuses localization resources, so they
resolve to raw key text. Add the missing BaseType entries to the CustomStatuses
resource set in the CustomStatuses localization files so the `@localizer[...]`
lookups in `Edit.cshtml` can resolve correctly.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Repositories/Resgrid.Repositories.DataRepository/UnitRolesRepository.cs (1)
72-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect implementation, mirrors the existing
GetAllRolesByUnitIdAsyncpattern.Connection handling, transaction usage, and exception logging via
Logging.LogExceptionare all consistent with the rest of this repository.Minor/optional: this method is a near-verbatim duplicate of
GetAllRolesByUnitIdAsyncabove it (differing only in the parameter name and query type). Since this connection-handling boilerplate is repeated identically across the whole repository layer, extracting a shared private helper here alone wouldn't address the wider duplication, so it's not essential for this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Repositories/Resgrid.Repositories.DataRepository/UnitRolesRepository.cs` around lines 72 - 112, GetAllRolesByDepartmentIdAsync already mirrors GetAllRolesByUnitIdAsync, so no functional change is needed here; keep the same connection handling, transaction usage, and Logging.LogException pattern in UnitRolesRepository. If you make any edits, ensure the local selectFunction, _unitOfWork.Connection check, and CreateOrGetConnection flow remain consistent, and avoid introducing a partial helper extraction in this PR since it wouldn’t address the broader duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Repositories/Resgrid.Repositories.DataRepository/UnitRolesRepository.cs`:
- Around line 72-112: GetAllRolesByDepartmentIdAsync already mirrors
GetAllRolesByUnitIdAsync, so no functional change is needed here; keep the same
connection handling, transaction usage, and Logging.LogException pattern in
UnitRolesRepository. If you make any edits, ensure the local selectFunction,
_unitOfWork.Connection check, and CreateOrGetConnection flow remain consistent,
and avoid introducing a partial helper extraction in this PR since it wouldn’t
address the broader duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 540e9d17-1162-4581-9e09-08db3dff2c5b
⛔ Files ignored due to path filters (19)
Core/Resgrid.Localization/Areas/User/CustomStatuses/CustomStatuses.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/CustomStatuses/CustomStatuses.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/CustomStatuses/CustomStatuses.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/CustomStatuses/CustomStatuses.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/CustomStatuses/CustomStatuses.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/CustomStatuses/CustomStatuses.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/CustomStatuses/CustomStatuses.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/CustomStatuses/CustomStatuses.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/CustomStatuses/CustomStatuses.uk.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Units/Units.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Units/Units.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Units/Units.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Units/Units.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Units/Units.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Units/Units.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Units/Units.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Units/Units.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Units/Units.uk.resxis excluded by!**/*.resxTests/Resgrid.Tests/Services/UnitRoleStaffingResultTests.csis excluded by!**/Tests/**
📒 Files selected for processing (16)
Core/Resgrid.Model/CustomStates/CustomStateTemplate.csCore/Resgrid.Model/Repositories/IUnitRolesRepository.csCore/Resgrid.Model/UnitRoleStaffingResult.csCore/Resgrid.Services/UnitsService.csRepositories/Resgrid.Repositories.DataRepository/Configs/SqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Queries/Units/SelectUnitRolesByDepartmentIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csRepositories/Resgrid.Repositories.DataRepository/UnitRolesRepository.csWeb/Resgrid.Web.Services/Controllers/v4/UnitRolesController.csWeb/Resgrid.Web/Areas/User/Views/CustomStatuses/_BaseTypeDescription.cshtmlWeb/Resgrid.Web/Areas/User/Views/Units/EditUnit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Units/NewUnit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Units/_UnitRoleTemplatesModal.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/units/resgrid.units.editunit.jsWeb/Resgrid.Web/wwwroot/js/app/internal/units/resgrid.units.newunit.js
🚧 Files skipped from review as they are similar to previous changes (10)
- Web/Resgrid.Web/Areas/User/Views/CustomStatuses/_BaseTypeDescription.cshtml
- Core/Resgrid.Model/CustomStates/CustomStateTemplate.cs
- Web/Resgrid.Web/Areas/User/Views/Units/NewUnit.cshtml
- Web/Resgrid.Web/Areas/User/Views/Units/EditUnit.cshtml
- Core/Resgrid.Services/UnitsService.cs
- Web/Resgrid.Web/wwwroot/js/app/internal/units/resgrid.units.editunit.js
- Web/Resgrid.Web/wwwroot/js/app/internal/units/resgrid.units.newunit.js
- Web/Resgrid.Web/Areas/User/Views/Units/_UnitRoleTemplatesModal.cshtml
- Core/Resgrid.Model/UnitRoleStaffingResult.cs
- Web/Resgrid.Web.Services/Controllers/v4/UnitRolesController.cs
Resgrid-Bot
reviewed
Jul 4, 2026
Resgrid-Bot
reviewed
Jul 4, 2026
Resgrid-Bot
reviewed
Jul 4, 2026
Resgrid-Bot
reviewed
Jul 4, 2026
Resgrid-Bot
reviewed
Jul 4, 2026
| <script> | ||
| // Self-contained: create the namespace defensively (this markup renders in the body, before the | ||
| // page's own unit scripts in the Scripts section). | ||
| var resgrid = resgrid || {}; |
There was a problem hiding this comment.
var usage violates the 'Always use const and let' rule across ~20 declarations in lines 72-161, including immutable bindings (roles, sel, nm, c, options, safeName, checked, tpl, r, q, terms, hay, match) that should be const and the mutable visible counter that should be let. The sibling file _BaseTypeDescription.cshtml in this PR already follows the convention.
const resgrid = resgrid || {};
...
const roles = rt.personnelRoles || [];
for (let i = 0; i < roles.length; i++) {
const sel = ...;
const nm = ...;
}
...
const c = nextCount();
const options = buildOptions(personnelRoleId);
const safeName = ...;
const checked = ...;
...
const tpl = ...;
const r = tpl.roles[i];
...
const q = ...;
const terms = ...;
let visible = 0;
const hay = ...;
const match = ...;Prompt for LLM
File Web/Resgrid.Web/Areas/User/Views/Units/_UnitRoleTemplatesModal.cshtml:
Line 72:
Violates rule 'Always use const and let': the entire script block (lines 72-161) uses `var` for ~20 declarations including loop variables, local consts, and mutable counters. The sibling new file `_BaseTypeDescription.cshtml` in this same PR correctly uses `const`/`let`, confirming the convention. Immutable bindings like `roles`, `sel`, `nm`, `c`, `options`, `safeName`, `checked`, `tpl`, `r`, `q`, `terms`, `hay`, `match` should be `const`; the mutable `visible` counter should be `let`.
Suggested Code:
const resgrid = resgrid || {};
...
const roles = rt.personnelRoles || [];
for (let i = 0; i < roles.length; i++) {
const sel = ...;
const nm = ...;
}
...
const c = nextCount();
const options = buildOptions(personnelRoleId);
const safeName = ...;
const checked = ...;
...
const tpl = ...;
const r = tpl.roles[i];
...
const q = ...;
const terms = ...;
let visible = 0;
const hay = ...;
const match = ...;
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Core/Resgrid.Services/UnitsService.cs (1)
39-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the project service-locator pattern for the new dependency.
Line 39 adds
IPersonnelRolesServiceas another constructor-injected dependency. Resolve it in the constructor instead, so this new dependency follows the project rule.Proposed change
- IDepartmentGroupsService departmentGroupsService, ILimitsService limitsService, IPersonnelRolesService personnelRolesService) + IDepartmentGroupsService departmentGroupsService, ILimitsService limitsService) ... - _personnelRolesService = personnelRolesService; + _personnelRolesService = Bootstrapper.GetKernel().Resolve<IPersonnelRolesService>();As per coding guidelines,
Use Service Locator pattern via Bootstrapper.GetKernel().Resolve<T>() to resolve dependencies explicitly in constructors, rather than constructor injection.Also applies to: 56-56
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/UnitsService.cs` at line 39, The constructor for the service is adding IPersonnelRolesService via injection, but this project expects the service-locator pattern instead. Update the constructor that currently takes departmentGroupsService and limitsService so it no longer accepts IPersonnelRolesService, and resolve that dependency inside the constructor using Bootstrapper.GetKernel().Resolve<IPersonnelRolesService>(). Make the same change anywhere else the new dependency is added, including the matching constructor/initializer location referenced by the review.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Core/Resgrid.Services/UnitsService.cs`:
- Line 39: The constructor for the service is adding IPersonnelRolesService via
injection, but this project expects the service-locator pattern instead. Update
the constructor that currently takes departmentGroupsService and limitsService
so it no longer accepts IPersonnelRolesService, and resolve that dependency
inside the constructor using
Bootstrapper.GetKernel().Resolve<IPersonnelRolesService>(). Make the same change
anywhere else the new dependency is added, including the matching
constructor/initializer location referenced by the review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8c76aa8-e5b0-4305-9e0b-745ce552ee20
📒 Files selected for processing (5)
Core/Resgrid.Model/Services/IUnitsService.csCore/Resgrid.Services/UnitsService.csWeb/Resgrid.Web.Services/Controllers/v4/UnitRolesController.csWeb/Resgrid.Web/Areas/User/Controllers/UnitsController.csWeb/Resgrid.Web/Areas/User/Views/Units/UnitStaffing.cshtml
🚧 Files skipped from review as they are similar to previous changes (3)
- Web/Resgrid.Web/Areas/User/Views/Units/UnitStaffing.cshtml
- Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs
- Web/Resgrid.Web.Services/Controllers/v4/UnitRolesController.cs
Resgrid-Bot
approved these changes
Jul 5, 2026
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.
PR Description: RG-T124 Base Status Type Fixes and Helper Catalogs
This PR significantly expands the custom status and unit role system with new base status types, prebuilt template catalogs for quick setup, and personnel qualification requirements for unit roles.
Key Changes
1. Extended Action Base Types
Added 12 new canonical base status types to
ActionBaseTypes(Enroute, Transporting, Delivering, AtPatient, AtHospital, Searching, Loading, Standby, OnPatrol, Maintenance, OnBreak, Completed) with full XML documentation. Each new type is classified in theAvailabilityMatrixso it drives availability/reporting/automations correctly (e.g., OnPatrol = Available, Transporting = Committed, Maintenance = Unavailable). The enum'sDescriptionAttributewas changed to carry richer help text, and a_BaseTypeDescriptionpartial renders these descriptions live in the UI when selecting a base type.2. Custom State Template Catalogs
Introduced a code-defined catalog of predefined status "starter packs" (
CustomStateTemplate,CustomStateTemplateDetail,CustomStateTemplateCatalog) covering disciplines including EMS, Fire, Law Enforcement, Search & Rescue, Disaster Response, Security, Industrial/Fleet, and Commodity Delivery. A new Templates gallery page lets users browse/search/filter templates by type and category, pick one, and pre-fill the New Custom Status form with editable, unsaved options. Blank-from-scratch remains available.3. Unit Role Template Catalogs
Introduced a parallel code-defined catalog of unit accountability role sets (
UnitRoleTemplate,UnitRoleTemplateRole,UnitRoleTemplateCatalog) such as Fire Engine Company, Ambulance (ALS/BLS), K9 Unit, SWAT Team, SAR Ground Team, Industrial Fire Brigade, Delivery Vehicle, etc. A modal picker on the New/Edit Unit page applies a template's seats (with suggested personnel qualifications matched by name to the department's existing roles).4. Personnel Role Qualifications on Unit Roles
Added optional
PersonnelRoleIdandPersonnelRoleRequiredcolumns toUnitRole(with SQL Server and PostgreSQL migrations M0086). A unit role can now require (hard block) or prefer (warn) a specific personnel qualification. Both the web UI and the v4 API enforce hard requirements on assignment and surface qualification status on responses.5. Computed Unit Staffing Levels
Added
UnitStaffingLevelenum (Unknown, NotStaffed, PartiallyStaffed, Degraded, FullyStaffed) andUnitRoleStaffingResultwhich calculates a unit's staffing from defined roles, active assignments, and qualification checks. The Unit Staffing page now displays staffing badges and per-seat qualification warnings. A newGetAllRolesByDepartmentIdAsyncrepository method supports batch loading to avoid N+1 queries.6. API Enhancements
The v4
UnitRolesControllernow returnsPersonnelRoleId,PersonnelRoleName,PersonnelRoleRequired, andIsQualifiedin role responses, and validates hard qualification requirements before accepting role assignments.7. Localization
Added translations for the 12 new base type names and the new unit role headers (Required Personnel Role, Required?) across all supported languages (ar, de, en, es, fr, it, pl, sv, uk).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation