Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| var data = $('#selectLinkedCall').select2('data'); | ||
|
|
||
| $('#linkedCalls tbody').first().append(`<tr><td style='max-width: 215px;'>${data[0].text}<input type='hidden' id='linkedCall_${data[0].id}' name='linkedCall_${data[0].id}' value='${data[0].id}' /></td><td>${$('#selectCallNote').val()}<input type='hidden' id='linkedCallNote_${data[0].id}' name='linkedCallNote_${data[0].id}' value='${$('#selectCallNote').val()}' /></td><td style='text-align:center;'><a onclick='$(this).parent().parent().remove();' class='tip-top' data-original-title='Remove this call link'><i class='fa fa-minus' style='color: red;'></i></a></td></tr>`); | ||
| $('#linkedCalls tbody').first().append(`<tr><td style='max-width: 215px;'>${data[0].text}<input type='hidden' id='linkedCall_${data[0].id}' name='linkedCall_${data[0].id}' value='${data[0].id}' /></td><td>${$('#selectCallNote').val()}<input type='hidden' id='linkedCallNote_${data[0].id}' name='linkedCallNote_${data[0].id}' value='${$('#selectCallNote').val()}' /></td><td style='text-align:center;'><a onclick='$(this).parent().parent().remove();' class='tip-top' data-original-title='${getText('removeThisCallLink', 'Remove this call link')}'><i class='fa fa-minus' style='color: red;'></i></a></td></tr>`); |
| var data = $('#selectLinkedCall').select2('data'); | ||
|
|
||
| $('#linkedCalls tbody').first().append(`<tr><td style='max-width: 215px;'>${data[0].text}<input type='hidden' id='linkedCall_${data[0].id}' name='linkedCall_${data[0].id}' value='${data[0].id}' /></td><td>${$('#selectCallNote').val()}<input type='hidden' id='linkedCallNote_${data[0].id}' name='linkedCallNote_${data[0].id}' value='${$('#selectCallNote').val()}' /></td><td style='text-align:center;'><a onclick='$(this).parent().parent().remove();' class='tip-top' data-original-title='Remove this call link'><i class='fa fa-minus' style='color: red;'></i></a></td></tr>`); | ||
| $('#linkedCalls tbody').first().append(`<tr><td style='max-width: 215px;'>${data[0].text}<input type='hidden' id='linkedCall_${data[0].id}' name='linkedCall_${data[0].id}' value='${data[0].id}' /></td><td>${$('#selectCallNote').val()}<input type='hidden' id='linkedCallNote_${data[0].id}' name='linkedCallNote_${data[0].id}' value='${$('#selectCallNote').val()}' /></td><td style='text-align:center;'><a onclick='$(this).parent().parent().remove();' class='tip-top' data-original-title='${getText('removeThisCallLink', 'Remove this call link')}'><i class='fa fa-minus' style='color: red;'></i></a></td></tr>`); |
| var data = $('#selectLinkedCall').select2('data'); | ||
|
|
||
| $('#linkedCalls tbody').first().append(`<tr><td style='max-width: 215px;'>${data[0].text}<input type='hidden' id='linkedCall_${data[0].id}' name='linkedCall_${data[0].id}' value='${data[0].id}' /></td><td>${$('#selectCallNote').val()}<input type='hidden' id='linkedCallNote_${data[0].id}' name='linkedCallNote_${data[0].id}' value='${$('#selectCallNote').val()}' /></td><td style='text-align:center;'><a onclick='$(this).parent().parent().remove();' class='tip-top' data-original-title='Remove this call link'><i class='fa fa-minus' style='color: red;'></i></a></td></tr>`); | ||
| $('#linkedCalls tbody').first().append(`<tr><td style='max-width: 215px;'>${data[0].text}<input type='hidden' id='linkedCall_${data[0].id}' name='linkedCall_${data[0].id}' value='${data[0].id}' /></td><td>${$('#selectCallNote').val()}<input type='hidden' id='linkedCallNote_${data[0].id}' name='linkedCallNote_${data[0].id}' value='${$('#selectCallNote').val()}' /></td><td style='text-align:center;'><a onclick='$(this).parent().parent().remove();' class='tip-top' data-original-title='${getText('removeThisCallLink', 'Remove this call link')}'><i class='fa fa-minus' style='color: red;'></i></a></td></tr>`); |
📝 WalkthroughWalkthroughAdds POI (point-of-interest) destinations across backend, API, migrations, services, controllers, views, frontend map UI, localization, and tests; introduces destination resolution helpers and new POI-related models, endpoints, and routing/ETA handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant UnitStatusController
participant DestinationResolutionHelper
participant CallsService
participant GroupsService
participant MappingService
participant GeoService
Client->>UnitStatusController: Request status/call/unit data (may include destinationId/type)
UnitStatusController->>DestinationResolutionHelper: Resolve(destinationId, destinationType, detailType, activeCalls, groups, pois, localizer)
DestinationResolutionHelper->>CallsService: Lookup call by id (if applicable)
DestinationResolutionHelper->>GroupsService: Lookup station/group by id (if applicable)
DestinationResolutionHelper->>MappingService: Lookup POI by id or list (if applicable)
CallsService-->>DestinationResolutionHelper: Call data
GroupsService-->>DestinationResolutionHelper: Group/station data
MappingService-->>DestinationResolutionHelper: POI data
DestinationResolutionHelper-->>UnitStatusController: ResolvedDestinationData (id,type,name,address,typeName)
UnitStatusController->>GeoService: GetPersonnelEta (uses resolved destination type/address/coords)
GeoService-->>UnitStatusController: ETA/route
UnitStatusController-->>Client: Response with destination fields populated
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
| [HttpPost] | ||
| public async Task<IActionResult> EditPOI(AddPOIView modal, CancellationToken cancellationToken) | ||
| { | ||
| if (modal?.Poi == null || modal.Poi.PoiId <= 0) |
| [HttpPost] | ||
| public async Task<IActionResult> EditPOI(AddPOIView modal, CancellationToken cancellationToken) | ||
| { | ||
| if (modal?.Poi == null || modal.Poi.PoiId <= 0) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Web/Resgrid.Web/Areas/User/Views/Mapping/POIs.cshtml (1)
49-71:⚠️ Potential issue | 🟡 MinorLocalize the new
Destinationcolumn header and Yes/No values.The new header text "Destination" and the "Yes"/"No" cell values are hardcoded English literals, while every other string in this view goes through
@localizer[...]. Given this PR explicitly targets multi-lingual support, please route these through the string localizer (adding the corresponding resource keys) so they translate consistently with the rest of the page.🌐 Proposed fix
- <th> - Destination - </th> + <th> + `@localizer`["Destination"] + </th> @@ - <td> - @(u.IsDestination ? "Yes" : "No") - </td> + <td> + @(u.IsDestination ? `@localizer`["Yes"] : `@localizer`["No"]) + </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/Mapping/POIs.cshtml` around lines 49 - 71, The "Destination" header and the hardcoded "Yes"/"No" cells should be routed through the existing localization system: replace the literal header text with `@localizer`["Destination"] and replace the ternary output @(u.IsDestination ? "Yes" : "No") with @(u.IsDestination ? localizer["Yes"] : localizer["No"]) (or use a boolean-to-string helper that calls localizer), and add the corresponding resource keys ("Destination", "Yes", "No") to the view's resource file so translations are available; ensure you use the same localizer instance (localizer) already used in this view and run/update resource generation as needed.Web/Resgrid.Web/wwwroot/js/ng/react-elements.css (1)
143-243:⚠️ Potential issue | 🔴 CriticalConfirm and correct edits: POI styles must be in source
map.css, never in the generatedwwwroot/js/ng/react-elements.css.The file
react-elements.cssis a Vite build artifact (configured inAreas/User/Apps/vite.config.tsline 37, withcssCodeSplit: falsebundling all CSS into a single output). TheMapElement.tsxcomponent importsmap.css, which gets bundled into the generated file on each build.Any manual edits to
wwwroot/js/ng/react-elements.csswill be lost on the next build. The POI marker styles (.rg-map__poi-marker-*,.rg-map__marker--poi) should exist only in the sourceApps/src/components/map/map.cssand are already present there. Do not replicate them in the build artifact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/wwwroot/js/ng/react-elements.css` around lines 143 - 243, Build artifact react-elements.css contains POI marker styles that must live only in your source map.css; remove the duplicated POI rules from react-elements.css and restore the single source of truth in the source map.css. Specifically, delete the selectors and rules for .rg-map__poi-marker, .rg-map__poi-marker-shape, .rg-map__poi-marker-icon and .rg-map__marker--poi (and any duplicated .rg-map__marker--poi .rg-map__poi-marker-shape/.rg-map__poi-marker-icon blocks) from react-elements.css, confirm the identical rules remain in the source map.css used by the MapElement import, then rebuild (Vite) to ensure the generated react-elements.css is produced from map.css rather than edited manually.Web/Resgrid.Web/Areas/User/Models/Home/BigBoardModel.cs (1)
267-273:⚠️ Potential issue | 🟡 MinorEdge case: a POI destination with a missing/whitespace
Nameis mislabeled as "Responding to Call {DestinationId}".When
destination.DestinationTypeis a POI butdestination.Nameis null/whitespace, the first branch is skipped,callNumberis empty (the resolution went via the POI path, not a call), and execution falls through to the lastelse. That branch formatsactionLog.DestinationIdas a call number, even though it's actually a POI id. Consider an explicit POI fallback (e.g., "Responding to destination {DestinationId}") so the displayed status doesn't mislead dispatchers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Models/Home/BigBoardModel.cs` around lines 267 - 273, The status logic in BigBoardModel.cs currently falls through to formatting actionLog.DestinationId as a call number when destination.DestinationType == (int)DestinationEntityTypes.Poi but destination.Name is null/whitespace; update the conditional branches to detect the POI case explicitly: if destination?.DestinationType == (int)DestinationEntityTypes.Poi and destination.Name is null/whitespace, set status to a POI-specific fallback like "Responding to destination {DestinationId}" (using actionLog.DestinationId), otherwise preserve the existing "Responding to Call {callNumber}" and other branches; adjust the branches around destination, DestinationType, destination.Name, callNumber, actionLog.DestinationId and status to ensure POI ids aren’t shown as call numbers.Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs (1)
731-740:⚠️ Potential issue | 🟠 MajorInconsistent destination handling in
SetUnitStateWithDestForMultiplewill reject empty-destination requests.Unlike
SetUnitStateWithDest(Line 643) andSetUnitStateForMultiple(Line 690), this endpoint unconditionally writesDestinationId/DestinationTypeand then validates without anif (destination > 0)guard. If a caller invokes this action withdestination=0(e.g., a default form value),IsValidDestinationAsync(0, type)will not match any entity and the entire batch will fail withBadRequest, even when no destination was intended. Additionally, validation should run before mutatingstatefor consistency with the sibling endpoints.🛠️ Proposed fix to align with the other endpoints
var state = new UnitState(); state.UnitId = unit.UnitId; state.LocalTimestamp = DateTimeHelpers.GetLocalDateTime(DateTime.UtcNow, department.TimeZone); state.State = stateType; state.Timestamp = DateTime.UtcNow; - state.DestinationId = destination; - state.DestinationType = type; - - if (!await IsValidDestinationAsync(destination, type)) - return BadRequest(); + + if (destination > 0) + { + if (!await IsValidDestinationAsync(destination, type)) + return BadRequest(); + + state.DestinationId = destination; + state.DestinationType = type; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 731 - 740, SetUnitStateWithDestForMultiple currently assigns DestinationId/DestinationType to the UnitState and calls IsValidDestinationAsync unconditionally, causing requests with destination==0 to fail; update the method so it first checks if destination > 0 and only then calls await IsValidDestinationAsync(destination, type), returning BadRequest if that validation fails, and only after passing validation assign state.DestinationId and state.DestinationType on the UnitState instance (keep setting LocalTimestamp/State/Timestamp as before) to mirror the behavior of SetUnitStateWithDest and SetUnitStateForMultiple and avoid mutating state before validation.
♻️ Duplicate comments (1)
Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.addArchivedCall.js (1)
251-251:⚠️ Potential issue | 🔴 CriticalDOM-text-as-HTML injection on the linked-call row.
data[0].text,data[0].id, and$('#selectCallNote').val()are concatenated into an HTML string and inserted via.append(), which CodeQL has flagged. A linked-call name or note containing characters such as<,', or"will break the markup and can be used for XSS. Use jQuery DOM-builder APIs (or.text()for the text content) instead of string concatenation, or HTML-escape every interpolated value before building the string.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.addArchivedCall.js` at line 251, The current append builds HTML with string interpolation using data[0].text, data[0].id and $('#selectCallNote').val(), causing DOM-text-as-HTML injection; fix by constructing the row with jQuery DOM APIs instead of a concatenated string: create a <tr> and three <td> elements, use .text() to set visible text nodes (for data[0].text and the note from $('#selectCallNote').val()), create hidden <input> elements with $('<input>', {type:'hidden', id: 'linkedCall_'+data[0].id, name: 'linkedCall_'+data[0].id, value: data[0].id}) (and similarly for linkedCallNote), append those inputs into the appropriate <td>, and attach the remove handler with .on('click', ...) instead of inline onclick; reference the DOM targets '#linkedCalls tbody', data[0].text, data[0].id, and '#selectCallNote' when locating and replacing the current string-concatenation append.
🟡 Minor comments (13)
Web/Resgrid.Web/Views/Account/Register.cshtml-82-84 (1)
82-84:⚠️ Potential issue | 🟡 MinorMinor: associate the
<label>with the<select>for accessibility.The
<label>has noforattribute pointing atregionSelect, so screen readers won't announce the label when the select is focused. Addfor="regionSelect"(and a matchingidis already present on the select).Proposed fix
- <div class="form-group m-t"> - <label><small>@localizer["Region"]</small></label> - <select id="regionSelect" class="form-control"> + <div class="form-group m-t"> + <label for="regionSelect"><small>@localizer["Region"]</small></label> + <select id="regionSelect" class="form-control">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Views/Account/Register.cshtml` around lines 82 - 84, The label for the region select is missing an association; add a for="regionSelect" attribute to the <label> that corresponds to the existing select id="regionSelect" so screen readers will correctly announce the label when the select is focused (update the <label> element near regionSelect in Register.cshtml).Web/Resgrid.Web/Areas/User/Views/Dispatch/_DispatchLocalizationScript.cshtml-10-10 (1)
10-10:⚠️ Potential issue | 🟡 Minor
activeandinactiveare sourced from different localizers — likely unintentional.
activereads fromcommonLocalizer["Active"](Common resources) whileinactivereads fromlocalizer["Inactive"](Dispatch.Call resources). IfInactiveis not present inResgrid.Localization.Areas.User.Dispatch.Call,IStringLocalizerreturns the key name and you'll get the un-localized string"Inactive"in non-English UIs.- inactive = localizer["Inactive"].Value, + inactive = commonLocalizer["Inactive"].Value,Verify which resource file actually owns these strings and align both keys to the same localizer.
Also applies to: 28-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/_DispatchLocalizationScript.cshtml` at line 10, The two localization keys for the Dispatch view are coming from different IStringLocalizer instances (active uses commonLocalizer while inactive uses localizer), causing inconsistent/escaped keys; pick the correct resource owner (e.g., use commonLocalizer for both or Dispatch localizer for both) and update the assignments for active and inactive in _DispatchLocalizationScript.cshtml so they both call the same localizer instance (align the keys to the resource file that actually contains "Active" and "Inactive"), and verify other occurrences noted (e.g., the one at line ~28) are updated similarly.Web/Resgrid.Web.Services/Models/v4/Mapping/PoiResultModels.cs-23-24 (1)
23-24:⚠️ Potential issue | 🟡 MinorNon-nullable
Latitude/Longitudewill silently serialize as0,0for unset POIs.If a POI has not been geocoded, these
doublefields default to0, which serializes as a valid coordinate (Null Island, off the coast of Africa) and will land pins there on the map. Considerdouble?so the client can branch on "no coordinates" instead of treating0,0as authoritative.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Models/v4/Mapping/PoiResultModels.cs` around lines 23 - 24, Latitude and Longitude in PoiResultModels are non-nullable doubles so unset POIs serialize as 0,0; change the properties Latitude and Longitude to nullable doubles (double?) in the PoiResultModels class and update any mapping/population code that assigns these values (e.g., the mapper that sets PoiResultModels.Latitude/Longitude) to leave them null when geocoding is absent, so clients can detect "no coordinates" instead of receiving 0,0.Web/Resgrid.Web/Helpers/DispatchDisplayHelper.cs-74-77 (1)
74-77:⚠️ Potential issue | 🟡 Minor
ICandPARare hardcoded — bypasses localization in aGetLocalized…helper.Every other branch in
GetLocalizedCheckInTimerTargetTyperesolves through a localizer, but these two return literal English/abbreviation strings. While "IC"/"PAR" are common firefighting acronyms that may not need translation, embedding raw strings here means non-English deployments cannot expand or localize them (e.g., "PC" / "RAP" in some locales) without a code change. Consider routing them through the localizer with the current literal as a fallback so the i18n surface is uniform:♻️ Suggested change
- case CheckInTimerTargetType.IC: - return "IC"; - case CheckInTimerTargetType.PAR: - return "PAR"; + case CheckInTimerTargetType.IC: + return localizer["CheckInTimerTargetIC"].Value; + case CheckInTimerTargetType.PAR: + return localizer["CheckInTimerTargetPAR"].Value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Helpers/DispatchDisplayHelper.cs` around lines 74 - 77, Get rid of the hardcoded "IC" and "PAR" returns in GetLocalizedCheckInTimerTargetType and resolve them through the existing localizer (same approach as other cases), using "IC" and "PAR" as the fallback/default values; specifically update the branches for CheckInTimerTargetType.IC and CheckInTimerTargetType.PAR to call the localizer with a sensible resource key (or the current literal as fallback) so localization is uniform across all CheckInTimerTargetType cases.Web/Resgrid.Web/Areas/User/Views/Mapping/Index.cshtml-45-45 (1)
45-45:⚠️ Potential issue | 🟡 MinorLocalize the "Manage POIs" button text.
The new button uses hardcoded English strings for both
titleand the label, while sibling buttons (ManageLayers,CustomMaps) go throughlocalizer. Given this PR explicitly addresses multi-lingual issues, this introduces a regression for non-English users.🌐 Proposed fix
- <a title="Manage POIs" class="btn btn-primary" asp-controller="Mapping" asp-action="POIs" asp-route-area="User">Manage POIs</a> + <a title="@localizer["ManagePOIs"]" class="btn btn-primary" asp-controller="Mapping" asp-action="POIs" asp-route-area="User">@localizer["ManagePOIs"]</a>Make sure a corresponding
ManagePOIsresource is added to the Mapping localization files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Mapping/Index.cshtml` at line 45, Replace the hardcoded "Manage POIs" text in the Index.cshtml Mapping view with localized strings: use the view's localizer (the same pattern used for ManageLayers/CustomMaps) for both the anchor's title attribute and its label, referencing a new resource key like "ManagePOIs"; then add the "ManagePOIs" entry to the Mapping localization resource files for each supported culture.Web/Resgrid.Web/wwwroot/js/app/internal/mapping/resgrid.mapping.viewType.js-22-39 (1)
22-39:⚠️ Potential issue | 🟡 MinorHardcoded English labels in the POI grid.
Column titles (
Name,Address,Note,Latitude,Longitude,Actions), action labels (Edit,Delete), and the confirm message (Delete this POI?) are all hardcoded English. Other dispatch grids in this PR (e.g.,resgrid.dispatch.smallcallgrid.js) useresgrid.dispatch.getText(...)with a localization partial. Consider applying the same pattern (or an equivalent mapping-localization partial) here so this view participates in the multi-lingual effort.🤖 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/mapping/resgrid.mapping.viewType.js` around lines 22 - 39, Replace hardcoded English labels in the POI DataTable column definitions and the action links/confirm with localized strings: instead of 'Name', 'Address', 'Note', 'Latitude', 'Longitude', 'Actions', 'Edit', 'Delete' and "Delete this POI?" use the project's localization helper (e.g., resgrid.dispatch.getText(...)) so the grid participates in multi-lingual support; update the column titles in the array and the render() return string to call the localization helper for the action text and confirmation message, keeping the existing logic around canManagePois and the PoiId-based URLs intact.Web/Resgrid.Web/Areas/User/Views/Mapping/AddPOI.cshtml-101-108 (1)
101-108:⚠️ Potential issue | 🟡 MinorAddress label is not localized.
Unlike the surrounding fields (
Latitude,Longitude,Name,Note), the Address label uses a literal"Address"string. Replace with@localizer["Address"](and add the resource key) for consistency with the other fields and the multi-lingual scope of this PR.- <label class="col-sm-2 control-label"> - Address - </label> + <label class="col-sm-2 control-label"> + `@localizer`["Address"] + </label>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Mapping/AddPOI.cshtml` around lines 101 - 108, The Address label in the AddPOI view is hard-coded; change the literal "Address" label in the form group for Poi.Address to use the localization helper (replace the literal with `@localizer`["Address"]) and add the "Address" resource key to the view's resource file so it matches the other fields (Latitude, Longitude, Name, Note) that use localization; ensure the same casing/key name is used as in other localized labels to keep consistency.Web/Resgrid.Web/Areas/User/Views/Mapping/AddPOI.cshtml-5-8 (1)
5-8:⚠️ Potential issue | 🟡 MinorHardcoded strings break the multi-lingual goal.
"Edit POI"and"Save POI"are hardcoded English while neighboring strings go throughlocalizer[...]. Given this PR explicitly targets multi-lingual support, these should also be localized. Also notesubmitTextfalls back tolocalizer["AddPOIHeader"]on the add path, which is a header key being used as a button label — likely should be a dedicated button key (e.g.,"AddPOIButton").🌐 Suggested fix
- var pageTitle = isEditing ? "Edit POI" : localizer["AddPOIPageTitle"]; - var pageHeader = isEditing ? "Edit POI" : localizer["AddPOIHeader"]; - var submitText = isEditing ? "Save POI" : localizer["AddPOIHeader"]; + var pageTitle = isEditing ? localizer["EditPOIPageTitle"] : localizer["AddPOIPageTitle"]; + var pageHeader = isEditing ? localizer["EditPOIHeader"] : localizer["AddPOIHeader"]; + var submitText = isEditing ? localizer["EditPOIButton"] : localizer["AddPOIButton"];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Mapping/AddPOI.cshtml` around lines 5 - 8, The strings pageTitle, pageHeader and submitText contain hardcoded English ("Edit POI", "Save POI") and submitText incorrectly uses the header key for the add path; update these to use the localization resource via localizer[...] instead of literals (e.g., replace "Edit POI" with localizer["EditPOIPageTitle"] or similar and "Save POI" with localizer["SavePOIButton"]), and change the add-path submitText key from localizer["AddPOIHeader"] to a dedicated button key like localizer["AddPOIButton"]; keep formAction as-is. Ensure the new localization keys exist in the resource files and use the same identifiers (pageTitle, pageHeader, submitText) so the view logic remains unchanged.Web/Resgrid.Web/Areas/User/Views/Dispatch/CallExportEx.cshtml-615-628 (1)
615-628:⚠️ Potential issue | 🟡 MinorLocalized distance unit labels are decoupled from the hard-coded metric conversion.
distKm/step distance are computed by dividing meters by1000and then displayed withrouteUnitKilometers/routeUnitMinutes. If a translator localizesRouteUnitKilometersto a non-metric unit (e.g., "mi") to match a user/department preference, the displayed number will still be kilometres while the label says miles, silently producing wrong driving information on the export. Either keep the unit label fixed to "km"/"min" (since the math is metric only) or pass the user/department preferred unit and convert before formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/CallExportEx.cshtml` around lines 615 - 628, The code computes distances in meters (routeData.distance and s.distance) and divides by 1000 into distKm but then uses the localized label routeUnitKilometers, which can mislabel units; update the rendering to either (A) always use a fixed metric label ("km" and "min") when you keep the /1000 math (change dvDistance and the step HTML to use "km"/"min" instead of routeUnitKilometers/routeUnitMinutes), or (B) honor a user/dept preferred unit by adding a conversion before formatting: detect the preferred unit (e.g., userDistanceUnit), convert routeData.distance and s.distance from meters to miles when needed, compute distValue and stepValue accordingly, and then use the matching label (routeUnitKilometers or routeUnitMiles) when building dvDistance.innerHTML and the step <li> entries (update references to distKm, s.distance, routeUnitKilometers, and routeUnitMinutes). Ensure the conversion logic is centralized so both the overall distance (distKm) and per-step distances (s.distance formatting) use the same conversion function.Web/Resgrid.Web/Areas/User/Views/Dispatch/NewCall.cshtml-227-233 (1)
227-233:⚠️ Potential issue | 🟡 MinorDestination dropdown is missing an empty option, so Select2's
allowClearwon't work and a destination cannot be unselected.
Call.DestinationPoiIdis a nullableint?, but theDropDownListForis rendered without anoptionLabel/empty entry. The Select2 init below usesallowClear: true, which requires a real empty option in the underlying<select>to function. As a result, once a POI is chosen, dispatchers cannot clear it back to "no destination" on this form.🛠️ Suggested fix
- `@Html.DropDownListFor`(x => x.Call.DestinationPoiId, Model.DestinationPois, new { `@class` = "form-control destination-poi-search", `@style` = "width:100%;" }) + `@Html.DropDownListFor`(x => x.Call.DestinationPoiId, Model.DestinationPois, `@localizer`["DestinationPlaceholder"].Value, new { `@class` = "form-control destination-poi-search", `@style` = "width:100%;" })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/NewCall.cshtml` around lines 227 - 233, The destination <select> rendered by DropDownListFor(x => x.Call.DestinationPoiId, Model.DestinationPois, ...) lacks an empty option so Select2's allowClear cannot clear a selection; update the DropDownListFor call (or the Model.DestinationPois list) to include a blank option—e.g., use the overload DropDownListFor(..., Model.DestinationPois, string.Empty, new { `@class` = "form-control destination-poi-search", `@style` = "width:100%;" }) or prepend a SelectListItem { Text = "", Value = "" } to Model.DestinationPois so Call.DestinationPoiId (int?) can be set back to null and Select2 allowClear will work.Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs-1639-1642 (1)
1639-1642:⚠️ Potential issue | 🟡 MinorUnify
PoiDisplayHelper.GetSelectionLabelinvocation across controllers.
UnitsController.GetPoiDisplayTextcallsGetSelectionLabel(poi)without passing the POI group name, while identical POI grouping logic inPersonnelController(line 1334) andDispatchController(line 3015) pass the group name as the second argument. This inconsistency causes POI labels in the units dropdown to differ from the personnel and dispatch dropdowns. UpdateGetPoiDisplayTextto accept and forward the group name: passpoiGroup.Keyat lines 1574 and 1633.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 1639 - 1642, Update UnitsController's GetPoiDisplayText to accept a group name and forward it to PoiDisplayHelper.GetSelectionLabel: change the GetPoiDisplayText(Poi poi) signature to GetPoiDisplayText(Poi poi, string groupName) and call PoiDisplayHelper.GetSelectionLabel(poi, groupName). Then update the two call sites in UnitsController that build POI dropdowns to pass poiGroup.Key as the second argument when calling GetPoiDisplayText so labels match PersonnelController and DispatchController.Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs-421-438 (1)
421-438:⚠️ Potential issue | 🟡 MinorAlign cache behavior across destination validation calls.
GetGroupByIdAsync()andGetCallByIdAsync()default tobypassCache = true, but the UI controllers (Web/Areas/User/PersonnelController and UnitsController) explicitly passfalseto enable caching. This v4 controller and v4/PersonnelStatusesController omit the parameter, defaulting to cache bypass. For validation-only operations, this creates inconsistent performance and cache behavior across equivalent code paths. Addfalseparameter to both service calls here, and extract this repeated logic into a shared helper to prevent further drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs` around lines 421 - 438, IsValidDestinationAsync currently calls _departmentGroupsService.GetGroupByIdAsync(destinationId) and _callsService.GetCallByIdAsync(destinationId) which default to bypassCache=true; change those to call the overloads with bypassCache=false (i.e. GetGroupByIdAsync(destinationId, false) and GetCallByIdAsync(destinationId, false)) and then extract the whole validation switch into a shared helper (e.g., ValidateDestinationAsync or DestinationValidator.ValidateAsync) so v4 controllers and v4/PersonnelStatusesController can reuse the same cached-behavior logic and avoid drift; keep the POI call as-is (_mappingService.GetDestinationPOIByIdAsync(DepartmentId, destinationId)).Web/Resgrid.Web.Services/Resgrid.Web.Services.xml-7675-7679 (1)
7675-7679:⚠️ Potential issue | 🟡 MinorUpdate the neighboring summaries that still say “Call/Station.”
These additions make POIs valid destinations, but the adjacent docs for
RespondingTo,DestinationId, andDestinationNamestill describe only calls/stations. That will publish a contradictory contract in Swagger/XML docs and can mislead integrators into treating POI ids as invalid.Also applies to: 7765-7769, 9335-9339, 9490-9509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 7675 - 7679, Update the XML summaries that still read “Call/Station” to acknowledge POIs as valid destinations: change the summary text for the members Resgrid.Web.Services.Models.v4.PersonnelStatuses.SavePersonsStatusesInput.RespondingTo, ...RespondingToType, ...DestinationId, and ...DestinationName to include POI (e.g. “Station = 1, Call = 2, POI = 3” or “Call/Station/POI”) so the Swagger/XML contract matches the new RespondingToType values; apply the same exact wording change to the other duplicated occurrences noted in the file to keep all summaries consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb85fbdc-42fa-44a9-8336-62a678c0011a
⛔ Files ignored due to path filters (18)
Core/Resgrid.Localization/Areas/User/Dispatch/Call.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Call.uk.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Dashboard.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Dashboard.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Dashboard.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Dashboard.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Dashboard.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Dashboard.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Dashboard.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Dashboard.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Dispatch/Dashboard.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (89)
Core/Resgrid.Config/InfoConfig.csCore/Resgrid.Model/Call.csCore/Resgrid.Model/Coordinates.csCore/Resgrid.Model/CustomStateDetailTypes.csCore/Resgrid.Model/DestinationEntityTypes.csCore/Resgrid.Model/Helpers/DestinationResolutionHelper.csCore/Resgrid.Model/Helpers/PoiDisplayHelper.csCore/Resgrid.Model/Poi.csCore/Resgrid.Model/Services/IMappingService.csCore/Resgrid.Model/UnitState.csCore/Resgrid.Services/ActionLogsService.csCore/Resgrid.Services/GeoService.csCore/Resgrid.Services/MappingService.csCore/Resgrid.Services/UnitsService.csProviders/Resgrid.Providers.Geo/KmlProvider.csProviders/Resgrid.Providers.Migrations/Migrations/M0066_AddingPoiDestinations.csProviders/Resgrid.Providers.Migrations/Migrations/M0067_AddingPoiName.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0066_AddingPoiDestinationsPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0067_AddingPoiNamePg.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csWeb/Resgrid.Web.Services/Controllers/v4/CallsController.csWeb/Resgrid.Web.Services/Controllers/v4/DispatchController.csWeb/Resgrid.Web.Services/Controllers/v4/MappingController.csWeb/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitStatusController.csWeb/Resgrid.Web.Services/Models/v4/Calls/CallResult.csWeb/Resgrid.Web.Services/Models/v4/Calls/EditCallInput.csWeb/Resgrid.Web.Services/Models/v4/Calls/NewCallInput.csWeb/Resgrid.Web.Services/Models/v4/Dispatch/GetSetUnitStateResult.csWeb/Resgrid.Web.Services/Models/v4/Dispatch/NewCallFormResult.csWeb/Resgrid.Web.Services/Models/v4/Mapping/GetMapDataResult.csWeb/Resgrid.Web.Services/Models/v4/Mapping/PoiResultModels.csWeb/Resgrid.Web.Services/Models/v4/PersonnelStatuses/GetCurrentStatusResult.csWeb/Resgrid.Web.Services/Models/v4/PersonnelStatuses/SavePersonStatusInput.csWeb/Resgrid.Web.Services/Models/v4/PersonnelStatuses/SavePersonsStatusesInput.csWeb/Resgrid.Web.Services/Models/v4/UnitStatus/UnitStatusInput.csWeb/Resgrid.Web.Services/Models/v4/UnitStatus/UnitStatusResult.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Apps/src/components/map/LeafletMapView.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/map/MapElement.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/map/MapboxMapView.tsxWeb/Resgrid.Web/Areas/User/Apps/src/components/map/map.cssWeb/Resgrid.Web/Areas/User/Apps/src/components/map/mapTypes.tsWeb/Resgrid.Web/Areas/User/Controllers/DispatchController.csWeb/Resgrid.Web/Areas/User/Controllers/LinksController.csWeb/Resgrid.Web/Areas/User/Controllers/MappingController.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/UnitsController.csWeb/Resgrid.Web/Areas/User/Models/Calls/NewCallView.csWeb/Resgrid.Web/Areas/User/Models/Calls/UpdateCallView.csWeb/Resgrid.Web/Areas/User/Models/Calls/ViewCallView.csWeb/Resgrid.Web/Areas/User/Models/Dispatch/CallExportView.csWeb/Resgrid.Web/Areas/User/Models/Home/BigBoardModel.csWeb/Resgrid.Web/Areas/User/Models/Mapping/PoiJson.csWeb/Resgrid.Web/Areas/User/Views/Dispatch/AddArchivedCall.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/ArchivedCalls.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/CallData.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/CallExport.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/CallExportEx.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/Chat.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/Dashboard.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/NewCall.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/UpdateCall.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/ViewCall.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/_DispatchLocalizationScript.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/_SmallActiveCallsGridPartial.cshtmlWeb/Resgrid.Web/Areas/User/Views/Dispatch/_SmallCallsGridPartial.cshtmlWeb/Resgrid.Web/Areas/User/Views/Mapping/AddPOI.cshtmlWeb/Resgrid.Web/Areas/User/Views/Mapping/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Mapping/POIs.cshtmlWeb/Resgrid.Web/Areas/User/Views/Mapping/ViewType.cshtmlWeb/Resgrid.Web/Helpers/DispatchDisplayHelper.csWeb/Resgrid.Web/Views/Account/Register.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.addArchivedCall.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.archive.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.callData.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.checkin-timers.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.dashboard.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.editcall.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.newcall.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.smallallcallgrid.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.smallcallgrid.jsWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.viewcall.jsWeb/Resgrid.Web/wwwroot/js/app/internal/mapping/resgrid.mapping.viewType.jsWeb/Resgrid.Web/wwwroot/js/app/internal/personnel/resgrid.personnel.index.jsWeb/Resgrid.Web/wwwroot/js/app/internal/units/resgrid.units.index.jsWeb/Resgrid.Web/wwwroot/js/ng/react-elements.cssWeb/Resgrid.Web/wwwroot/js/ng/react-elements.js
| public static string GetDisplayName(this DestinationEntityTypes destinationType) | ||
| { | ||
| switch (destinationType) | ||
| { | ||
| case DestinationEntityTypes.Station: | ||
| return "Station"; | ||
| case DestinationEntityTypes.Call: | ||
| return "Call"; | ||
| case DestinationEntityTypes.Poi: | ||
| return "POI"; | ||
| default: | ||
| return string.Empty; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find call sites of GetDisplayName to determine user-facing impact.
rg -nP -C3 '\.GetDisplayName\s*\(' --type=csRepository: Resgrid/Core
Length of output: 7315
Hardcoded English display strings undermine the PR's multi-lingual goal.
The GetDisplayName() extension method returns literal "Station", "Call", and "POI" strings, which are consumed by user-facing APIs and controllers (e.g., DestinationResolutionHelper feeding into web controllers). Given this PR explicitly addresses multi-lingual support, any caller using these hardcoded strings will bypass localization.
Either:
- Remove this method and resolve display strings via
IStringLocalizerat the call site (preferred — keeps the model layer pure), or - Have
GetDisplayName()accept anIStringLocalizerand perform lookups internally.
The current implementation is inconsistent with the PR's multi-lingual scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Model/DestinationEntityTypes.cs` around lines 23 - 36, The
GetDisplayName(this DestinationEntityTypes destinationType) method returns
hardcoded English strings, which breaks the PR's multi-lingual goal; remove this
extension or change it to use localization: either delete GetDisplayName and
update all callers (e.g., DestinationResolutionHelper and any controllers/APIs)
to resolve names via IStringLocalizer at the presentation layer, or change the
method signature to GetDisplayName(this DestinationEntityTypes destinationType,
IStringLocalizer localizer) and replace the literal returns with
localizer["Station"], localizer["Call"], localizer["POI"]; ensure all call sites
are updated to pass an IStringLocalizer when using the method.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs (2)
1352-1418:⚠️ Potential issue | 🔴 CriticalDon't mutate personnel status from GET endpoints.
Both actions persist status changes via
GET, so crawlers, link previews, or CSRF-able navigations can change personnel state without user intent. These need to be POST actions with anti-forgery validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs` around lines 1352 - 1418, Change the two controller actions SetActionForUser and SetUserActionForMultiple to use POST rather than GET and add anti-forgery validation: replace [HttpGet] with [HttpPost] and add [ValidateAntiForgeryToken] above each method (keep the existing [Authorize] attributes and signatures). Ensure any client-side forms or JS that call these endpoints use POST and include the antiforgery token, and keep the existing authorization/validation logic (IsValidDestinationAsync, _actionLogsService.SetUserActionAsync, Logging.LogException) intact so behavior is unchanged except for the HTTP method and CSRF protection.
2143-2172:⚠️ Potential issue | 🟠 MajorHistoric event destination resolution only works for active calls.
These report paths resolve destinations against
GetActiveCallsByDepartmentAsync(...). Any historical event that points at a closed call will lose its destination name even though theActionLogstill has the destination id/type. Load the referenced calls for the selected events, or fall back to per-id lookup when resolution misses.Also applies to: 2204-2221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs` around lines 2143 - 2172, The destination resolution uses only active calls from _callsService.GetActiveCallsByDepartmentAsync when calling DestinationResolutionHelper.Resolve for each ActionLog, so historical events that reference closed calls lose their destination name; update the loop that processes eventIds (and the similar block around the 2204-2221 range) to collect DestinationId/DestinationType values from the ActionLog list, load the referenced calls via the calls service (e.g., a GetCallsByIdsAsync or GetCallsForEventsAsync helper) and merge them with activeCalls before calling DestinationResolutionHelper.Resolve, and if Resolve still returns no match, perform a fallback per-id lookup using the loaded calls and ActionLog.DestinationId/ActionLog.DestinationType to set personnelEvent.DestinationName.Web/Resgrid.Web/Areas/User/Controllers/MappingController.cs (1)
360-381:⚠️ Potential issue | 🔴 CriticalProtect
AddPOITypewith anti-forgery validation.This POST creates department POI types but still accepts cross-site form submissions. A forged request can create mapping data under the current user's session unless the action validates an anti-forgery token.
🔒 Suggested fix
[HttpPost] + [ValidateAntiForgeryToken] public async Task<IActionResult> AddPOIType(AddPOITypeView modal, CancellationToken cancellationToken)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/MappingController.cs` around lines 360 - 381, The AddPOIType POST action lacks anti-forgery validation and is vulnerable to CSRF; add the [ValidateAntiForgeryToken] attribute to the AddPOIType method and ensure the form that posts to this action includes an anti-forgery token (e.g., `@Html.AntiForgeryToken`() or equivalent) so the server-side validation in AddPOIType will reject forged requests.Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs (2)
942-959:⚠️ Potential issue | 🟠 MajorHistoric unit reports only resolve active calls.
DestinationResolutionHelperis fed withGetActiveCallsByDepartmentAsync(...), so older unit events tied to closed calls will render without a destination name. Reports need the referenced call ids, not just the currently active call set.Also applies to: 997-1015
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 942 - 959, The destination resolution is using only the current active calls (GetActiveCallsByDepartmentAsync) so historic events referencing closed calls lose their destination; change the logic in UnitsController where you build UnitEventJson (method using eventIds loop and the similar block around lines 997-1015) to load the actual referenced call(s) for each event: when eventRecord.DestinationType indicates a call, fetch that call by its id via the calls service (e.g. _callsService.GetCallByIdAsync(eventRecord.DestinationId) or a batch helper like _callsService.GetCallsByIdsAsync if multiple) and pass those results into DestinationResolutionHelper.Resolve instead of or in addition to the activeCalls list; ensure you still fall back to activeCalls/stations/pois when a specific referenced call is not found.
628-756:⚠️ Potential issue | 🔴 CriticalMove these unit-state writers off GET.
These endpoints change persisted unit state but are still callable by simple GET links. That leaves them open to CSRF, browser prefetchers, and accidental state changes from navigation. They should be POST actions with anti-forgery protection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 628 - 756, These three actions (SetUnitStateWithDest, SetUnitStateForMultiple, SetUnitStateWithDestForMultiple) perform persistent state changes but are marked [HttpGet]; change them to [HttpPost] and add anti-forgery protection by replacing [HttpGet] with [HttpPost] and adding [ValidateAntiForgeryToken] to each method, update any callers/forms/links to submit POST requests with the antiforgery token, and keep existing authorization checks and method bodies as-is so SetUnitStateWithDest, SetUnitStateForMultiple and SetUnitStateWithDestForMultiple remain the same except for the HTTP verb and anti-forgery attribute.Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs (1)
250-280:⚠️ Potential issue | 🔴 Critical
ConvertPersonStatuscannot compile asstatic.The implementation references
_localizer(an instance field) on line 274, but static methods cannot access instance fields. This is a compile-time error that will prevent the build from succeeding until the method is converted to an instance method orIStringLocalizeris passed as an explicit parameter.Suggested fix
- public static GetCurrentStatusResultData ConvertPersonStatus(ActionLog actionLog, Department department, string userId, List<Call> activeCalls, List<DepartmentGroup> stations, List<Poi> pois) + private GetCurrentStatusResultData ConvertPersonStatus(ActionLog actionLog, Department department, string userId, List<Call> activeCalls, List<DepartmentGroup> stations, List<Poi> pois)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs` around lines 250 - 280, Convert ConvertPersonStatus so it no longer accesses the instance field _localizer from a static context: either make ConvertPersonStatus an instance method (remove static) on PersonnelStatusesController so it can reference _localizer, or keep it static and add an IStringLocalizer parameter (e.g. IStringLocalizer localizer) and pass that into DestinationResolutionHelper.Resolve where _localizer is currently used; update all call sites accordingly to provide the controller instance or the localizer argument and adjust the DestinationResolutionHelper.Resolve invocation to use the supplied localizer variable.
🧹 Nitpick comments (5)
Web/Resgrid.Web/Areas/User/Views/Dispatch/Chat.cshtml (2)
4-4: Minor: redundant@inside Razor expression string.
"Resgrid | " +@localizer["DispatchChat"]works, but the inner@is unnecessary (and slightly misleading) once you're already in a Razor code block started byViewBag.Title =. Consider dropping it for consistency with the other usages in this file.♻️ Proposed tidy-up
- ViewBag.Title = "Resgrid | " + `@localizer`["DispatchChat"]; + ViewBag.Title = "Resgrid | " + localizer["DispatchChat"];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/Chat.cshtml` at line 4, The ViewBag.Title assignment uses a redundant Razor directive: change the expression ViewBag.Title = "Resgrid | " + `@localizer`["DispatchChat"] to remove the inner '@' so it reads ViewBag.Title = "Resgrid | " + localizer["DispatchChat"]; update the expression near the ViewBag.Title assignment in the Chat.cshtml view to match other localizer usages for consistency.
142-142: XSS onuserNameis now mitigated — nice fix.
htmlEncode(userName)resolves the prior raw-HTML injection concern for the disconnect notice. One residual nit:loggedOffMessageis interpolated as raw HTML, so any markup characters in the localized "LoggedOff" resource (e.g.,&,<,>) would render incorrectly or as HTML. Since resource strings are author-controlled this is low-risk, but routing it throughhtmlEncode(or building the node with.text()) would make it fully consistent withAddMessageat Line 182.🛡️ Optional hardening
- var disc = $('<div class="disconnect">"' + htmlEncode(userName) + '" ' + loggedOffMessage + '</div>'); + var disc = $('<div class="disconnect"></div>') + .text('"' + userName + '" ' + loggedOffMessage);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/Chat.cshtml` at line 142, The disconnect notice currently injects loggedOffMessage as raw HTML; update the creation of the disconnect element to encode the resource string or set it as text to avoid rendering HTML entities—e.g., use htmlEncode(loggedOffMessage) or build the node with .text() instead of concatenating raw loggedOffMessage (refer to the disconnect creation line using var disc = $('<div class="disconnect">"'+ htmlEncode(userName) +'" ' + loggedOffMessage + '</div>') and the AddMessage pattern around Line 182 for guidance).Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs (2)
7-14: Consider modelingResolvedDestinationDataas an immutable record.The type is a pure data carrier with no mutation needs after construction. A
record(or at leastinit-only setters) would better match the codebase's intent and the functional/immutable preference for new types.♻️ Proposed refactor
- public class ResolvedDestinationData - { - public int? DestinationId { get; set; } - public int? DestinationType { get; set; } - public string Name { get; set; } - public string Address { get; set; } - public string TypeName { get; set; } - } + public class ResolvedDestinationData + { + public int? DestinationId { get; init; } + public int? DestinationType { get; init; } + public string Name { get; init; } + public string Address { get; init; } + public string TypeName { get; init; } + }As per coding guidelines: "Separate state from behavior (e.g., use records for state and place operations in separate static classes)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs` around lines 7 - 14, Convert the mutable POCO ResolvedDestinationData into an immutable data carrier by changing the declaration to a record (e.g., record ResolvedDestinationData(...)) or at minimum switch each property (DestinationId, DestinationType, Name, Address, TypeName) to init-only setters (get; init;); update any call sites that rely on property mutation to construct new instances via positional or object initializer style, and keep the type name ResolvedDestinationData unchanged so existing references still resolve.
18-18: Parameter list is getting long; consider a context/parameter object.
Resolve(...)takes 7 parameters andResolveByType(...)takes 6, with the same(activeCalls, groups, pois, localizer)tuple threaded through. Wrapping these in a smallDestinationResolutionContextrecord would simplify the signature, reduce call-site noise in controllers, and make future additions (e.g., aUnit/Personsource) non-breaking.Also applies to: 71-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs` at line 18, The Resolve and ResolveByType methods take many repeated parameters (activeCalls, groups, pois, localizer); create a small record like DestinationResolutionContext that holds IEnumerable<Call> ActiveCalls, IEnumerable<DepartmentGroup> Groups, IEnumerable<Poi> Pois, and IStringLocalizer Localizer, then change the signatures of Resolve(int? destinationId, int? destinationType, int? detailType, DestinationResolutionContext ctx) and ResolveByType(int? destinationType, int? detailType, DestinationResolutionContext ctx) to accept this context instead of the four separate args; update all internal references within DestinationResolutionHelper to use the context properties and refactor callers (controllers and other call sites) to construct and pass a DestinationResolutionContext instance so the API is simplified and future additions are non-breaking.Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs (1)
260-270: Destination validation and persistence look correct; minor robustness nit.Defaulting
RespondingToTypetoCallpreserves backward compatibility, andIsValidDestinationAsyncenforces tenant isolation per type. One small concern: the existingint.Parse(stateInput.RespondingTo)is called twice on the same string (line 260 and 263) — consider a singleint.TryParseto avoid redundant work and to fail withBadRequeston malformed input rather than throwing into the outercatch(which logs as an exception).♻️ Proposed refactor
- if (!string.IsNullOrWhiteSpace(stateInput.RespondingTo) && int.Parse(stateInput.RespondingTo) > 0) - { - var destinationType = stateInput.RespondingToType ?? (int)DestinationEntityTypes.Call; - var destinationId = int.Parse(stateInput.RespondingTo); - - if (!await IsValidDestinationAsync(destinationId, destinationType)) - return BadRequest(); - - state.DestinationId = destinationId; - state.DestinationType = destinationType; - } + if (!string.IsNullOrWhiteSpace(stateInput.RespondingTo) + && int.TryParse(stateInput.RespondingTo, out var destinationId) + && destinationId > 0) + { + var destinationType = stateInput.RespondingToType ?? (int)DestinationEntityTypes.Call; + + if (!await IsValidDestinationAsync(destinationId, destinationType)) + return BadRequest(); + + state.DestinationId = destinationId; + state.DestinationType = destinationType; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs` around lines 260 - 270, The code calls int.Parse(stateInput.RespondingTo) twice and can throw on malformed input; change the block around stateInput.RespondingTo/RespondingToType to use a single int.TryParse to parse RespondingTo into a local destinationId variable, return BadRequest if parsing fails or value <= 0, set destinationType = stateInput.RespondingToType ?? (int)DestinationEntityTypes.Call, call await IsValidDestinationAsync(destinationId, destinationType) as before, and then assign state.DestinationId and state.DestinationType only after validation succeeds (preserve names: stateInput.RespondingTo, stateInput.RespondingToType, DestinationEntityTypes.Call, IsValidDestinationAsync, state.DestinationId, state.DestinationType).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs`:
- Around line 60-68: The fallback probing (Station → Call → Poi) in
DestinationResolutionHelper uses ResolveByType with only destinationId and can
return the wrong entity when IDs collide across tables; change the logic so that
when effectiveDestinationType == None and detailType is null (or none of its
Supports* predicates match) you do not perform the cross-type fallback: either
return the empty result immediately or require an explicit opt-in flag (e.g.,
add a boolean parameter like allowCrossTypeFallback to the public
Resolve/ResolveDestination surface and check it here); update the call sites to
pass the flag if opting into the old behavior and ensure ResolveByType is only
called for the effective type (or when allowCrossTypeFallback is true).
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs`:
- Around line 347-348: ConvertUnitStatusData is static but reads the instance
field _localizer causing a compilation error; either make ConvertUnitStatusData
an instance method (remove static) and update all call sites that reference it
in UnitStatusController and DispatchController to call it on the controller
instance, or keep it static and add an IStringLocalizer localizer parameter to
the method signature and pass the controller's _localizer from each call site;
update the method signature and every call (the four calls inside
UnitStatusController and the call in DispatchController) accordingly so the
method no longer accesses _localizer directly.
In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs`:
- Around line 1307-1342: Replace hardcoded dropdown labels and prefixes in
PersonnelController where the StringBuilder 'sb' is built (the block using
state.DetailType and looping activeCalls/stations/destinationPois) to use the
injected _localizer instead of literals; change "None", "Calls", "Stations",
"POIs" and the "Call" and "Station" prefixes to localized strings (e.g.,
_localizer["None"], _localizer["Calls"], _localizer["Stations"],
_localizer["Pois"] and format call/station option text with _localizer for the
prefix) and ensure poiGroup.Key is passed through HttpUtility.HtmlEncode after
localization or that a localized fallback is used when poiGroup.Key is the
default "POIs".
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs`:
- Around line 1584-1637: The issue is that BuildUnitStateMenuHtml and
AppendDestinationMenuEntries directly splice the raw targetQuery into hrefs,
enabling reflected XSS; fix by building the query string from typed values
instead of inserting targetQuery raw—parse the unit identifiers (or the
individual parameters) that compose targetQuery, URL-encode each parameter with
HttpUtility.UrlEncode (or use a QueryString/NameValueCollection builder), then
concatenate only the encoded key=value pairs when forming hrefs in
BuildUnitStateMenuHtml and AppendDestinationMenuEntries (affecting links that
use actionWithoutDestination and actionWithDestination and the
destination/type/stateType parameters); ensure every dynamic value (including
target parts) is URL-encoded and the final href is HTML-encoded where
appropriate.
- Around line 1597-1603: The submenu <li> opened in UnitsController string
building is never closed, producing invalid DOM; after the block that appends
the submenu inner </ul> (the code that calls AppendDestinationMenuEntries(sb,
targetQuery, actionWithDestination, activeCalls, stations, destinationPois,
state) and then sb.Append("</ul>")), append the matching closing "</li>" to sb
so each "<li class='dropdown-submenu'>" has a corresponding "</li>" and the
markup is well-formed; locate the string builder usage in UnitsController (the
block that builds unitStateList_{cssKey} and calls AppendDestinationMenuEntries)
and add the closing tag immediately after the existing sb.Append("</ul>").
In `@Web/Resgrid.Web/wwwroot/js/app/internal/mapping/resgrid.mapping.viewType.js`:
- Around line 77-80: The tooltip currently passes poi.Title directly to
marker.bindTooltip which Leaflet treats as HTML and allows XSS; instead create a
DOM element (e.g., a div or span), set its textContent to poi.Title to ensure it
is escaped, and pass that element to marker.bindTooltip with the existing
options; update the code paths that call marker.bindTooltip (referencing
poi.Title and marker.bindTooltip) to use the textContent-backed element instead
of the raw string.
- Around line 22-24: The Name/Address/Note DataTables column definitions render
raw HTML, and marker.bindPopup(poi.InfoWindowContent) and
marker.bindTooltip(poi.Title, ...) insert raw HTML into Leaflet popups/tooltips;
fix by using a text renderer for the DataTables columns (e.g., replace the
column defs for 'Name', 'Address', 'Note' with a renderer such as
$.fn.dataTable.render.text() or an equivalent text-escaping render function) and
by passing a safe DOM node or escaped text to Leaflet instead of raw HTML
(create an element, set its textContent to poi.InfoWindowContent before calling
marker.bindPopup(...) or use L.popup().setContent(safeElement; likewise for
marker.bindTooltip use a DOM element with textContent or
L.tooltip().setContent(safeElement)). Ensure you update the column definitions
and the calls to marker.bindPopup and marker.bindTooltip to use these
safe/escaped values.
---
Outside diff comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs`:
- Around line 250-280: Convert ConvertPersonStatus so it no longer accesses the
instance field _localizer from a static context: either make ConvertPersonStatus
an instance method (remove static) on PersonnelStatusesController so it can
reference _localizer, or keep it static and add an IStringLocalizer parameter
(e.g. IStringLocalizer localizer) and pass that into
DestinationResolutionHelper.Resolve where _localizer is currently used; update
all call sites accordingly to provide the controller instance or the localizer
argument and adjust the DestinationResolutionHelper.Resolve invocation to use
the supplied localizer variable.
In `@Web/Resgrid.Web/Areas/User/Controllers/MappingController.cs`:
- Around line 360-381: The AddPOIType POST action lacks anti-forgery validation
and is vulnerable to CSRF; add the [ValidateAntiForgeryToken] attribute to the
AddPOIType method and ensure the form that posts to this action includes an
anti-forgery token (e.g., `@Html.AntiForgeryToken`() or equivalent) so the
server-side validation in AddPOIType will reject forged requests.
In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs`:
- Around line 1352-1418: Change the two controller actions SetActionForUser and
SetUserActionForMultiple to use POST rather than GET and add anti-forgery
validation: replace [HttpGet] with [HttpPost] and add [ValidateAntiForgeryToken]
above each method (keep the existing [Authorize] attributes and signatures).
Ensure any client-side forms or JS that call these endpoints use POST and
include the antiforgery token, and keep the existing authorization/validation
logic (IsValidDestinationAsync, _actionLogsService.SetUserActionAsync,
Logging.LogException) intact so behavior is unchanged except for the HTTP method
and CSRF protection.
- Around line 2143-2172: The destination resolution uses only active calls from
_callsService.GetActiveCallsByDepartmentAsync when calling
DestinationResolutionHelper.Resolve for each ActionLog, so historical events
that reference closed calls lose their destination name; update the loop that
processes eventIds (and the similar block around the 2204-2221 range) to collect
DestinationId/DestinationType values from the ActionLog list, load the
referenced calls via the calls service (e.g., a GetCallsByIdsAsync or
GetCallsForEventsAsync helper) and merge them with activeCalls before calling
DestinationResolutionHelper.Resolve, and if Resolve still returns no match,
perform a fallback per-id lookup using the loaded calls and
ActionLog.DestinationId/ActionLog.DestinationType to set
personnelEvent.DestinationName.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs`:
- Around line 942-959: The destination resolution is using only the current
active calls (GetActiveCallsByDepartmentAsync) so historic events referencing
closed calls lose their destination; change the logic in UnitsController where
you build UnitEventJson (method using eventIds loop and the similar block around
lines 997-1015) to load the actual referenced call(s) for each event: when
eventRecord.DestinationType indicates a call, fetch that call by its id via the
calls service (e.g. _callsService.GetCallByIdAsync(eventRecord.DestinationId) or
a batch helper like _callsService.GetCallsByIdsAsync if multiple) and pass those
results into DestinationResolutionHelper.Resolve instead of or in addition to
the activeCalls list; ensure you still fall back to activeCalls/stations/pois
when a specific referenced call is not found.
- Around line 628-756: These three actions (SetUnitStateWithDest,
SetUnitStateForMultiple, SetUnitStateWithDestForMultiple) perform persistent
state changes but are marked [HttpGet]; change them to [HttpPost] and add
anti-forgery protection by replacing [HttpGet] with [HttpPost] and adding
[ValidateAntiForgeryToken] to each method, update any callers/forms/links to
submit POST requests with the antiforgery token, and keep existing authorization
checks and method bodies as-is so SetUnitStateWithDest, SetUnitStateForMultiple
and SetUnitStateWithDestForMultiple remain the same except for the HTTP verb and
anti-forgery attribute.
---
Nitpick comments:
In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs`:
- Around line 7-14: Convert the mutable POCO ResolvedDestinationData into an
immutable data carrier by changing the declaration to a record (e.g., record
ResolvedDestinationData(...)) or at minimum switch each property (DestinationId,
DestinationType, Name, Address, TypeName) to init-only setters (get; init;);
update any call sites that rely on property mutation to construct new instances
via positional or object initializer style, and keep the type name
ResolvedDestinationData unchanged so existing references still resolve.
- Line 18: The Resolve and ResolveByType methods take many repeated parameters
(activeCalls, groups, pois, localizer); create a small record like
DestinationResolutionContext that holds IEnumerable<Call> ActiveCalls,
IEnumerable<DepartmentGroup> Groups, IEnumerable<Poi> Pois, and IStringLocalizer
Localizer, then change the signatures of Resolve(int? destinationId, int?
destinationType, int? detailType, DestinationResolutionContext ctx) and
ResolveByType(int? destinationType, int? detailType,
DestinationResolutionContext ctx) to accept this context instead of the four
separate args; update all internal references within DestinationResolutionHelper
to use the context properties and refactor callers (controllers and other call
sites) to construct and pass a DestinationResolutionContext instance so the API
is simplified and future additions are non-breaking.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs`:
- Around line 260-270: The code calls int.Parse(stateInput.RespondingTo) twice
and can throw on malformed input; change the block around
stateInput.RespondingTo/RespondingToType to use a single int.TryParse to parse
RespondingTo into a local destinationId variable, return BadRequest if parsing
fails or value <= 0, set destinationType = stateInput.RespondingToType ??
(int)DestinationEntityTypes.Call, call await
IsValidDestinationAsync(destinationId, destinationType) as before, and then
assign state.DestinationId and state.DestinationType only after validation
succeeds (preserve names: stateInput.RespondingTo, stateInput.RespondingToType,
DestinationEntityTypes.Call, IsValidDestinationAsync, state.DestinationId,
state.DestinationType).
In `@Web/Resgrid.Web/Areas/User/Views/Dispatch/Chat.cshtml`:
- Line 4: The ViewBag.Title assignment uses a redundant Razor directive: change
the expression ViewBag.Title = "Resgrid | " + `@localizer`["DispatchChat"] to
remove the inner '@' so it reads ViewBag.Title = "Resgrid | " +
localizer["DispatchChat"]; update the expression near the ViewBag.Title
assignment in the Chat.cshtml view to match other localizer usages for
consistency.
- Line 142: The disconnect notice currently injects loggedOffMessage as raw
HTML; update the creation of the disconnect element to encode the resource
string or set it as text to avoid rendering HTML entities—e.g., use
htmlEncode(loggedOffMessage) or build the node with .text() instead of
concatenating raw loggedOffMessage (refer to the disconnect creation line using
var disc = $('<div class="disconnect">"'+ htmlEncode(userName) +'" ' +
loggedOffMessage + '</div>') and the AddMessage pattern around Line 182 for
guidance).
🪄 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: 4bdbf302-c7f2-40b4-9a35-a49b20685710
⛔ Files ignored due to path filters (1)
Core/Resgrid.Localization/Common.en.resxis excluded by!**/*.resx
📒 Files selected for processing (17)
Core/Resgrid.Model/DestinationEntityTypes.csCore/Resgrid.Model/Helpers/DestinationResolutionHelper.csCore/Resgrid.Model/Helpers/PoiDisplayHelper.csCore/Resgrid.Model/Resgrid.Model.csprojWeb/Resgrid.Web.Services/Controllers/v4/MappingController.csWeb/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitStatusController.csWeb/Resgrid.Web.Services/Models/v4/Calls/CallResult.csWeb/Resgrid.Web.Services/Models/v4/PersonnelStatuses/GetCurrentStatusResult.csWeb/Resgrid.Web.Services/Models/v4/UnitStatus/UnitStatusResult.csWeb/Resgrid.Web/Areas/User/Controllers/LinksController.csWeb/Resgrid.Web/Areas/User/Controllers/MappingController.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/UnitsController.csWeb/Resgrid.Web/Areas/User/Views/Dispatch/Chat.cshtmlWeb/Resgrid.Web/Areas/User/Views/Mapping/ViewType.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/mapping/resgrid.mapping.viewType.js
✅ Files skipped from review due to trivial changes (1)
- Core/Resgrid.Model/Resgrid.Model.csproj
🚧 Files skipped from review as they are similar to previous changes (5)
- Web/Resgrid.Web/Areas/User/Views/Mapping/ViewType.cshtml
- Web/Resgrid.Web.Services/Models/v4/Calls/CallResult.cs
- Web/Resgrid.Web.Services/Models/v4/UnitStatus/UnitStatusResult.cs
- Web/Resgrid.Web.Services/Models/v4/PersonnelStatuses/GetCurrentStatusResult.cs
- Web/Resgrid.Web/Areas/User/Controllers/LinksController.cs
| public static UnitStatusResultData ConvertUnitStatusData(Unit unit, UnitState stateFound, UnitsLocation latestUnitLocation, | ||
| CustomStateDetail customState, DepartmentGroup group, string timeZone, List<Call> activeCalls, List<DepartmentGroup> groups) | ||
| CustomStateDetail customState, DepartmentGroup group, string timeZone, List<Call> activeCalls, List<DepartmentGroup> groups, List<Poi> pois) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all callers of ConvertUnitStatusData and confirm _localizer usage in a static context.
rg -nP --type=cs -C2 '\bConvertUnitStatusData\s*\('
rg -nP --type=cs -C1 'public\s+static\s+UnitStatusResultData\s+ConvertUnitStatusData'Repository: Resgrid/Core
Length of output: 4150
🏁 Script executed:
sed -n '347,400p' Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.csRepository: Resgrid/Core
Length of output: 1807
Fix compilation error: static method cannot access instance field _localizer.
ConvertUnitStatusData is declared public static (line 347) but dereferences _localizer (line 376), a private instance field. Either remove static and update the four call sites in this controller (lines 103, 108, 167, 172) and the call site in DispatchController (line 189) to use instance method invocation, or add IStringLocalizer localizer as a parameter and pass _localizer at each call site.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs` around lines
347 - 348, ConvertUnitStatusData is static but reads the instance field
_localizer causing a compilation error; either make ConvertUnitStatusData an
instance method (remove static) and update all call sites that reference it in
UnitStatusController and DispatchController to call it on the controller
instance, or keep it static and add an IStringLocalizer localizer parameter to
the method signature and pass the controller's _localizer from each call site;
update the method signature and every call (the four calls inside
UnitStatusController and the call in DispatchController) accordingly so the
method no longer accesses _localizer directly.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs (1)
1599-1605:⚠️ Potential issue | 🔴 CriticalUndefined
targetQuery— won't compile and a duplicate of the past XSS finding.
{targetQuery}is interpolated into thehrefon lines 1599 and 1605, but notargetQueryvariable, parameter, or field exists inBuildUnitStateMenuHtml(or this controller). The other code paths in this method correctly delegate toBuildUnitStateHref(...). These two leftovers both fail to compile and (if they did) would re-introduce the raw-string href XSS that the previous review flagged.🛠️ Suggested fix
- if (state.DetailType == (int)CustomStateDetailTypes.None) - { - sb.Append($"<li><a style=\"color:{HttpUtility.HtmlEncode(state.ButtonColor)};\" href='/User/Units/{actionWithoutDestination}?{targetQuery}&stateType={state.CustomStateDetailId}'>{HttpUtility.HtmlEncode(state.ButtonText)}</a></li>"); - continue; - } - - sb.Append($"<li class='dropdown-submenu'><a style=\"color:{HttpUtility.HtmlEncode(state.ButtonColor)};\" tabindex='-1' href='#'>{HttpUtility.HtmlEncode(state.ButtonText)}</a>"); - sb.Append($"<ul class='dropdown-menu unitStateList_{cssKey}'>"); - sb.Append($"<li><a href='/User/Units/{actionWithoutDestination}?{targetQuery}&stateType={state.CustomStateDetailId}'>{HttpUtility.HtmlEncode(state.ButtonText)}</a></li>"); + if (state.DetailType == (int)CustomStateDetailTypes.None) + { + sb.Append($"<li><a style=\"color:{buttonColor};\" href='{withoutDestinationHref}'>{buttonText}</a></li>"); + continue; + } + + sb.Append($"<li class='dropdown-submenu'><a style=\"color:{buttonColor};\" tabindex='-1' href='#'>{buttonText}</a>"); + sb.Append($"<ul class='dropdown-menu unitStateList_{cssKey}'>"); + sb.Append($"<li><a href='{withoutDestinationHref}'>{buttonText}</a></li>");Note:
withoutDestinationHref,buttonColor, andbuttonTextare already computed on lines 1593–1595 above; reuse those.#!/bin/bash # Confirm `targetQuery` is not defined anywhere in UnitsController.cs. fd -e cs UnitsController.cs --exec rg -nP '\btargetQuery\b' {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 1599 - 1605, The two occurrences using the undefined {targetQuery} in BuildUnitStateMenuHtml will not compile and bypass the existing safe-href generation; replace them to use the already-computed href value(s) instead of targetQuery (either reuse withoutDestinationHref or call BuildUnitStateHref(...) as the other branches do) and keep existing HttpUtility.HtmlEncode for buttonColor/buttonText; update both string interpolations (the lines building the href in the submenu and its child item) to use the safe href variable so compilation succeeds and the previous XSS mitigation remains intact.
🧹 Nitpick comments (5)
Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs (2)
7-14: Consider arecordfor the immutable DTO.
ResolvedDestinationDatais a pure data carrier — a positional or init-onlyrecordwould express its immutability and value semantics more clearly, and lets consumers rely on with-expressions for partial updates.- public class ResolvedDestinationData - { - public int? DestinationId { get; set; } - public int? DestinationType { get; set; } - public string Name { get; set; } - public string Address { get; set; } - public string TypeName { get; set; } - } + public record ResolvedDestinationData + { + public int? DestinationId { get; init; } + public int? DestinationType { get; init; } + public string Name { get; init; } + public string Address { get; init; } + public string TypeName { get; init; } + }As per coding guidelines: "Separate state from behavior (e.g., use records for state and place operations in separate static classes)" and "Prefer functional patterns and immutable data where appropriate in C#".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs` around lines 7 - 14, ResolvedDestinationData is a mutable POCO used purely as DTO; change it to an immutable record to express value semantics and enable with-expressions for partial updates—replace the class ResolvedDestinationData with a record (either a positional record or a record with init-only properties) preserving the existing properties (DestinationId, DestinationType, Name, Address, TypeName) so consumers can use value equality and immutability without changing behavior.
18-18: Makelocalizeroptional or add a no-localizer overload.
localizeris currently a required positional parameter, but several call sites in this PR (e.g.,LinksController.GetPersonnelListline 367 and the new tests at lines 35/56) pass no localizer and won't compile. Consider either:
- Defaulting it:
IStringLocalizer localizer = null(downstreamGetDisplayName(localizer)andPoiDisplayHelper.GetTypeName(..., localizer)would need to tolerate null), or- Adding a thin overload that supplies a fallback.
This both fixes the broken call sites and makes the helper friendlier to test/internal use where localization isn't available.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs` at line 18, The Resolve method requires IStringLocalizer but callers (e.g., LinksController.GetPersonnelList and unit tests) don't pass one; change Resolve(int? destinationId, int? destinationType, int? detailType, IEnumerable<Call> activeCalls, IEnumerable<DepartmentGroup> groups, IEnumerable<Poi> pois, IStringLocalizer localizer, bool allowCrossTypeFallback = false) to accept a nullable localizer (IStringLocalizer localizer = null) or add an overload that calls the existing Resolve with null; then update downstream usages such as ResolvedDestinationData.GetDisplayName(localizer) and PoiDisplayHelper.GetTypeName(..., localizer) to handle a null localizer (use non-localized fallback strings when localizer is null) so callers that don't supply a localizer compile and tests can run.Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs (1)
1448-1454: Threshold inconsistency:stateId > 1here vs.> 25inGetActiveUnitStatusDetailsAsync.
GetActiveUnitStatusDetailsAsync(line 1539) usescustomStateId > 25to distinguish department-defined custom states from built-in defaults. The same pattern should apply here — passing a built-in state id (e.g., 2–25) intoGetCustomSateByIdAsyncwill likely return null (or worse, a wrong record) and fall through silently. Consider routing throughGetActiveUnitStatusDetailsAsyncinstead of duplicating the lookup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 1448 - 1454, The current check in UnitsController (stateId > 1) is inconsistent with GetActiveUnitStatusDetailsAsync which treats department-defined custom states as customStateId > 25; update this block to use the same logic by either changing the condition to stateId > 25 or — better — delegate to the existing GetActiveUnitStatusDetailsAsync method instead of directly calling GetCustomSateByIdAsync; ensure you remove the duplicate lookup (customStates/GetActiveDetails()) and use the unified path so built-in ids (2–25) are not misrouted.Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs (2)
287-304: DuplicateIsValidDestinationAsyncacross controllers.The same switch-by-
DestinationEntityTypesvalidation block exists verbatim inWeb/Resgrid.Web/Areas/User/Controllers/UnitsController.cs(lines 1708–1725). Consider lifting this into a shared helper (e.g., aDestinationValidatoror extension on the relevant services) so the two stay in sync — particularly noting thatUnitsControllerpassesfalsetoGetGroupByIdAsync/GetCallByIdAsync(no-track) but this controller uses the default. That subtle asymmetry is the kind of drift this duplication invites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs` around lines 287 - 304, Duplicate destination validation logic in IsValidDestinationAsync should be centralized: extract the switch-by-DestinationEntityTypes logic into a shared helper or service (e.g., DestinationValidator with a method IsValidDestinationAsync) and replace IsValidDestinationAsync in PersonnelStatusesController and the duplicate in UnitsController to call that single implementation; ensure the new API accepts a parameter to control tracking behavior so callers can pass the no-tracking flag used by UnitsController, and reuse the existing service calls (_departmentGroupsService.GetGroupByIdAsync, _callsService.GetCallByIdAsync, _mappingService.GetDestinationPOIByIdAsync) inside the centralized validator to keep behavior consistent.
250-250:ConvertPersonStatusispublic static— consider making itprivate(or instance) to pass_localizer.The method is
public staticbut only used within the controller. Making it non-static (or acceptingIStringLocalizeras a parameter) lets you pass_localizertoDestinationResolutionHelper.Resolvefor proper localization (see comment on lines 274–281). Marking itprivatealso reduces the public surface area of the controller class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs` at line 250, The ConvertPersonStatus method is public static but only used internally and must access the controller's _localizer; change its signature from "public static GetCurrentStatusResultData ConvertPersonStatus(...)" to a non-static, private instance method (private GetCurrentStatusResultData ConvertPersonStatus(...)) or alternatively add an IStringLocalizer parameter to the method signature so it can pass the localizer into DestinationResolutionHelper.Resolve; update all internal call sites inside the controller to call the instance/private method (or pass _localizer) and remove/static-related references so DestinationResolutionHelper.Resolve receives the _localizer for proper localization and the method is no longer exposed publicly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs`:
- Around line 31-58: The current logic in DestinationResolutionHelper uses
ResolveByType(destinationId.Value, DestinationEntityTypes.*, ...) and only
treats a match as valid if result.Name is non-empty; change this to detect the
presence of an entity itself (not a non-blank Name). Update ResolveByType to
return null when no entity is found (or add a boolean Found flag on the return
object) and then in the detailType branch (where you cast to
CustomStateDetailTypes and call SupportsStations/SupportsCalls/SupportsPois)
check for a non-null return (or result.Found) rather than
string.IsNullOrWhiteSpace(result.Name) so an entity with an empty Name won’t be
skipped and cause incorrect type resolution.
- Line 63: The call to ResolveByType when creating fallbackStation is missing
the required localizer parameter; update the fallbackStation assignment in
DestinationResolutionHelper to call ResolveByType(destinationId.Value,
DestinationEntityTypes.Station, activeCalls, groups, pois, localizer) so it
matches the other ResolveByType usages and returns a localized TypeName.
In `@Tests/Resgrid.Tests/Models/DestinationResolutionHelperTests.cs`:
- Line 35: Tests fail to compile because calls to
DestinationResolutionHelper.Resolve are missing the required IStringLocalizer
parameter and DestinationEntityTypes.Station.GetDisplayName requires a localizer
too; update the failing calls to pass a localizer (e.g., localizer: null or a
NullStringLocalizer) into DestinationResolutionHelper.Resolve(…) and call
DestinationEntityTypes.Station.GetDisplayName(null) (or pass the same
NullStringLocalizer) so the required IStringLocalizer argument is supplied.
In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs`:
- Around line 274-281: The code computes effectiveDestinationType via
actionLog.GetEffectiveDestinationType() but continues to call
DestinationResolutionHelper.Resolve with actionLog.DestinationType (losing
legacy resolution) and omits localization; change the Resolve call to pass the
computed destinationType variable instead of actionLog.DestinationType and pass
the controller's _localizer into Resolve so TypeName is localized; to enable
this, update ConvertPersonStatus (either add an IStringLocalizer parameter or
make it an instance method) so callers can forward _localizer into
DestinationResolutionHelper.Resolve when building statusResult for ActionLog
entries (especially where ActionTypeId == RespondingToScene and DestinationType
may be null).
In `@Web/Resgrid.Web/Areas/User/Controllers/LinksController.cs`:
- Around line 366-368: Remove the unused local variable destinationType and
update the call to DestinationResolutionHelper.Resolve by passing the injected
_localizer instance instead of null (i.e., replace the third argument null with
_localizer) so the call becomes Resolve(al?.DestinationId, al?.DestinationType,
_localizer, calls, stations, pois); keep the rest of the call to
PersonnelViewModel.Create unchanged.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs`:
- Around line 740-744: The current loop assigns state.DestinationId and
state.DestinationType before validating and does so even when destination == 0;
change the logic to mirror SetUnitStateWithDest and SetUnitStateForMultiple:
only call IsValidDestinationAsync when destination > 0, perform the
IsValidDestinationAsync check BEFORE assigning to
state.DestinationId/state.DestinationType, and skip or treat destination == 0 as
"no destination" (do not persist a zero value). Ensure you reference
DestinationId, DestinationType and IsValidDestinationAsync when making the
change so validation precedes assignment and you avoid mid-loop partial updates.
---
Duplicate comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs`:
- Around line 1599-1605: The two occurrences using the undefined {targetQuery}
in BuildUnitStateMenuHtml will not compile and bypass the existing safe-href
generation; replace them to use the already-computed href value(s) instead of
targetQuery (either reuse withoutDestinationHref or call BuildUnitStateHref(...)
as the other branches do) and keep existing HttpUtility.HtmlEncode for
buttonColor/buttonText; update both string interpolations (the lines building
the href in the submenu and its child item) to use the safe href variable so
compilation succeeds and the previous XSS mitigation remains intact.
---
Nitpick comments:
In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs`:
- Around line 7-14: ResolvedDestinationData is a mutable POCO used purely as
DTO; change it to an immutable record to express value semantics and enable
with-expressions for partial updates—replace the class ResolvedDestinationData
with a record (either a positional record or a record with init-only properties)
preserving the existing properties (DestinationId, DestinationType, Name,
Address, TypeName) so consumers can use value equality and immutability without
changing behavior.
- Line 18: The Resolve method requires IStringLocalizer but callers (e.g.,
LinksController.GetPersonnelList and unit tests) don't pass one; change
Resolve(int? destinationId, int? destinationType, int? detailType,
IEnumerable<Call> activeCalls, IEnumerable<DepartmentGroup> groups,
IEnumerable<Poi> pois, IStringLocalizer localizer, bool allowCrossTypeFallback =
false) to accept a nullable localizer (IStringLocalizer localizer = null) or add
an overload that calls the existing Resolve with null; then update downstream
usages such as ResolvedDestinationData.GetDisplayName(localizer) and
PoiDisplayHelper.GetTypeName(..., localizer) to handle a null localizer (use
non-localized fallback strings when localizer is null) so callers that don't
supply a localizer compile and tests can run.
In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs`:
- Around line 287-304: Duplicate destination validation logic in
IsValidDestinationAsync should be centralized: extract the
switch-by-DestinationEntityTypes logic into a shared helper or service (e.g.,
DestinationValidator with a method IsValidDestinationAsync) and replace
IsValidDestinationAsync in PersonnelStatusesController and the duplicate in
UnitsController to call that single implementation; ensure the new API accepts a
parameter to control tracking behavior so callers can pass the no-tracking flag
used by UnitsController, and reuse the existing service calls
(_departmentGroupsService.GetGroupByIdAsync, _callsService.GetCallByIdAsync,
_mappingService.GetDestinationPOIByIdAsync) inside the centralized validator to
keep behavior consistent.
- Line 250: The ConvertPersonStatus method is public static but only used
internally and must access the controller's _localizer; change its signature
from "public static GetCurrentStatusResultData ConvertPersonStatus(...)" to a
non-static, private instance method (private GetCurrentStatusResultData
ConvertPersonStatus(...)) or alternatively add an IStringLocalizer parameter to
the method signature so it can pass the localizer into
DestinationResolutionHelper.Resolve; update all internal call sites inside the
controller to call the instance/private method (or pass _localizer) and
remove/static-related references so DestinationResolutionHelper.Resolve receives
the _localizer for proper localization and the method is no longer exposed
publicly.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs`:
- Around line 1448-1454: The current check in UnitsController (stateId > 1) is
inconsistent with GetActiveUnitStatusDetailsAsync which treats
department-defined custom states as customStateId > 25; update this block to use
the same logic by either changing the condition to stateId > 25 or — better —
delegate to the existing GetActiveUnitStatusDetailsAsync method instead of
directly calling GetCustomSateByIdAsync; ensure you remove the duplicate lookup
(customStates/GetActiveDetails()) and use the unified path so built-in ids
(2–25) are not misrouted.
🪄 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: 2ea03408-77b1-4d70-89da-88fcaf6a043f
⛔ Files ignored due to path filters (9)
Core/Resgrid.Localization/Common.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Common.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (8)
Core/Resgrid.Model/ActionLog.csCore/Resgrid.Model/Helpers/DestinationResolutionHelper.csTests/Resgrid.Tests/Models/DestinationResolutionHelperTests.csWeb/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.csWeb/Resgrid.Web/Areas/User/Controllers/LinksController.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/UnitsController.csWeb/Resgrid.Web/wwwroot/js/app/internal/mapping/resgrid.mapping.viewType.js
✅ Files skipped from review due to trivial changes (1)
- Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Web/Resgrid.Web/wwwroot/js/app/internal/mapping/resgrid.mapping.viewType.js
| if (effectiveDestinationType != DestinationEntityTypes.None) | ||
| return ResolveByType(destinationId.Value, effectiveDestinationType, activeCalls, groups, pois, localizer); | ||
|
|
||
| if (detailType.HasValue) | ||
| { | ||
| var stateDetailType = (CustomStateDetailTypes)detailType.Value; | ||
|
|
||
| if (stateDetailType.SupportsStations()) | ||
| { | ||
| var stationResult = ResolveByType(destinationId.Value, DestinationEntityTypes.Station, activeCalls, groups, pois, localizer); | ||
| if (!string.IsNullOrWhiteSpace(stationResult.Name)) | ||
| return stationResult; | ||
| } | ||
|
|
||
| if (stateDetailType.SupportsCalls()) | ||
| { | ||
| var callResult = ResolveByType(destinationId.Value, DestinationEntityTypes.Call, activeCalls, groups, pois, localizer); | ||
| if (!string.IsNullOrWhiteSpace(callResult.Name)) | ||
| return callResult; | ||
| } | ||
|
|
||
| if (stateDetailType.SupportsPois()) | ||
| { | ||
| var poiResult = ResolveByType(destinationId.Value, DestinationEntityTypes.Poi, activeCalls, groups, pois, localizer); | ||
| if (!string.IsNullOrWhiteSpace(poiResult.Name)) | ||
| return poiResult; | ||
| } | ||
| } |
There was a problem hiding this comment.
Inferred-by-detailType branch silently returns the wrong type when ID resolves but Name is empty.
Each branch (SupportsStations/SupportsCalls/SupportsPois) returns the result only when Name is non-blank. If a Station/Call/POI matches by id but has a blank Name (e.g., a draft Call with no name yet), the branch is skipped and the helper continues probing other types — potentially returning a different entity that happens to share the id. Recommend matching on whether the entity itself was found (i.e., have ResolveByType return null when nothing matches, or add a Found flag), not on whether Name is populated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs` around lines 31 -
58, The current logic in DestinationResolutionHelper uses
ResolveByType(destinationId.Value, DestinationEntityTypes.*, ...) and only
treats a match as valid if result.Name is non-empty; change this to detect the
presence of an entity itself (not a non-blank Name). Update ResolveByType to
return null when no entity is found (or add a boolean Found flag on the return
object) and then in the detailType branch (where you cast to
CustomStateDetailTypes and call SupportsStations/SupportsCalls/SupportsPois)
check for a non-null return (or result.Found) rather than
string.IsNullOrWhiteSpace(result.Name) so an entity with an empty Name won’t be
skipped and cause incorrect type resolution.
| if (!allowCrossTypeFallback) | ||
| return result; | ||
|
|
||
| var fallbackStation = ResolveByType(destinationId.Value, DestinationEntityTypes.Station, activeCalls, groups, pois); |
There was a problem hiding this comment.
Build failure: missing localizer argument.
Per the .NET pipeline (CS7036), this call is missing the required localizer argument that the other two ResolveByType calls (lines 67 and 71) correctly supply. Pass localizer here too so the fallback Station resolution returns a localized TypeName.
🛠️ Suggested fix
- var fallbackStation = ResolveByType(destinationId.Value, DestinationEntityTypes.Station, activeCalls, groups, pois);
+ var fallbackStation = ResolveByType(destinationId.Value, DestinationEntityTypes.Station, activeCalls, groups, pois, localizer);🧰 Tools
🪛 GitHub Actions: .NET
[error] 63-63: dotnet build failed with CS7036: There is no argument given that corresponds to the required parameter 'localizer' of 'DestinationResolutionHelper.ResolveByType(int, DestinationEntityTypes, IEnumerable, IEnumerable, IEnumerable, IStringLocalizer)'.
🪛 GitHub Check: build-and-test
[failure] 63-63:
There is no argument given that corresponds to the required parameter 'localizer' of 'DestinationResolutionHelper.ResolveByType(int, DestinationEntityTypes, IEnumerable, IEnumerable, IEnumerable, IStringLocalizer)'
[failure] 63-63:
There is no argument given that corresponds to the required parameter 'localizer' of 'DestinationResolutionHelper.ResolveByType(int, DestinationEntityTypes, IEnumerable, IEnumerable, IEnumerable, IStringLocalizer)'
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs` at line 63, The
call to ResolveByType when creating fallbackStation is missing the required
localizer parameter; update the fallbackStation assignment in
DestinationResolutionHelper to call ResolveByType(destinationId.Value,
DestinationEntityTypes.Station, activeCalls, groups, pois, localizer) so it
matches the other ResolveByType usages and returns a localized TypeName.
| }; | ||
| var pois = new List<Poi>(); | ||
|
|
||
| var result = DestinationResolutionHelper.Resolve(42, null, null, calls, stations, pois); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm both extension signatures.
rg -nP --type=cs 'public\s+static\s+\w+\s+GetDisplayName\s*\(' Core/Resgrid.Model/
rg -nP --type=cs 'public\s+static\s+ResolvedDestinationData\s+Resolve\s*\(' Core/Resgrid.Model/Helpers/Repository: Resgrid/Core
Length of output: 764
🏁 Script executed:
cat -n Tests/Resgrid.Tests/Models/DestinationResolutionHelperTests.cs | sed -n '30,70p'Repository: Resgrid/Core
Length of output: 1548
Tests won't compile — missing IStringLocalizer arguments.
- Line 35:
DestinationResolutionHelper.Resolve(42, null, null, calls, stations, pois)is missing the requiredIStringLocalizer localizerparameter before the optionalallowCrossTypeFallback. - Line 56:
DestinationResolutionHelper.Resolve(42, null, null, new List<Call>(), stations, new List<Poi>(), allowCrossTypeFallback: true)also lacks the requiredlocalizerparameter; using the named argumentallowCrossTypeFallbackdoes not skip the required parameter before it. - Line 61:
DestinationEntityTypes.Station.GetDisplayName()requires anIStringLocalizerparameter.
Pass localizer: null (or a NullStringLocalizer) to both Resolve calls and pass null to GetDisplayName().
Suggested fix
- var result = DestinationResolutionHelper.Resolve(42, null, null, calls, stations, pois);
+ var result = DestinationResolutionHelper.Resolve(42, null, null, calls, stations, pois, null);
@@
- var result = DestinationResolutionHelper.Resolve(42, null, null, new List<Call>(), stations, new List<Poi>(), allowCrossTypeFallback: true);
+ var result = DestinationResolutionHelper.Resolve(42, null, null, new List<Call>(), stations, new List<Poi>(), null, allowCrossTypeFallback: true);
@@
- result.TypeName.Should().Be(DestinationEntityTypes.Station.GetDisplayName());
+ result.TypeName.Should().Be(DestinationEntityTypes.Station.GetDisplayName(null));Also applies to: 56-56, 61-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/Resgrid.Tests/Models/DestinationResolutionHelperTests.cs` at line 35,
Tests fail to compile because calls to DestinationResolutionHelper.Resolve are
missing the required IStringLocalizer parameter and
DestinationEntityTypes.Station.GetDisplayName requires a localizer too; update
the failing calls to pass a localizer (e.g., localizer: null or a
NullStringLocalizer) into DestinationResolutionHelper.Resolve(…) and call
DestinationEntityTypes.Station.GetDisplayName(null) (or pass the same
NullStringLocalizer) so the required IStringLocalizer argument is supplied.
| var destinationType = actionLog.GetEffectiveDestinationType(); | ||
| var destination = DestinationResolutionHelper.Resolve(actionLog.DestinationId, actionLog.DestinationType, null, activeCalls, stations, pois); | ||
| statusResult.DestinationId = destination.DestinationId; | ||
| statusResult.DestinationType = destination.DestinationType; | ||
| statusResult.DestinationName = destination.Name; | ||
| statusResult.DestinationAddress = destination.Address; | ||
| statusResult.DestinationTypeName = destination.TypeName; | ||
| } |
There was a problem hiding this comment.
Effective destination type is computed but discarded, breaking legacy ActionLog resolution.
GetEffectiveDestinationType() is called on line 274 but never used. The next line passes actionLog.DestinationType (raw, possibly null for legacy rows) into Resolve instead. For the very case GetEffectiveDestinationType is designed to handle — ActionTypeId == RespondingToScene with a null DestinationType — Resolve will see effectiveDestinationType == None and (with allowCrossTypeFallback == false) return only the id, so DestinationName/DestinationAddress/DestinationTypeName will remain empty in the API response.
Additionally, _localizer is not passed to Resolve, so even successful resolutions will return non-localized TypeNames — defeating the multi-lingual goal of this PR.
🛠️ Suggested fix
- if (actionLog.DestinationId.HasValue)
- {
- var destinationType = actionLog.GetEffectiveDestinationType();
- var destination = DestinationResolutionHelper.Resolve(actionLog.DestinationId, actionLog.DestinationType, null, activeCalls, stations, pois);
+ if (actionLog.DestinationId.HasValue)
+ {
+ var destinationType = actionLog.DestinationType ?? (int)actionLog.GetEffectiveDestinationType();
+ var destination = DestinationResolutionHelper.Resolve(actionLog.DestinationId, destinationType, null, activeCalls, stations, pois, _localizer);Note: ConvertPersonStatus would need to accept IStringLocalizer as a parameter (it's currently static), or be made an instance method.
📝 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.
| var destinationType = actionLog.GetEffectiveDestinationType(); | |
| var destination = DestinationResolutionHelper.Resolve(actionLog.DestinationId, actionLog.DestinationType, null, activeCalls, stations, pois); | |
| statusResult.DestinationId = destination.DestinationId; | |
| statusResult.DestinationType = destination.DestinationType; | |
| statusResult.DestinationName = destination.Name; | |
| statusResult.DestinationAddress = destination.Address; | |
| statusResult.DestinationTypeName = destination.TypeName; | |
| } | |
| if (actionLog.DestinationId.HasValue) | |
| { | |
| var destinationType = actionLog.DestinationType ?? (int)actionLog.GetEffectiveDestinationType(); | |
| var destination = DestinationResolutionHelper.Resolve(actionLog.DestinationId, destinationType, null, activeCalls, stations, pois, _localizer); | |
| statusResult.DestinationId = destination.DestinationId; | |
| statusResult.DestinationType = destination.DestinationType; | |
| statusResult.DestinationName = destination.Name; | |
| statusResult.DestinationAddress = destination.Address; | |
| statusResult.DestinationTypeName = destination.TypeName; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs`
around lines 274 - 281, The code computes effectiveDestinationType via
actionLog.GetEffectiveDestinationType() but continues to call
DestinationResolutionHelper.Resolve with actionLog.DestinationType (losing
legacy resolution) and omits localization; change the Resolve call to pass the
computed destinationType variable instead of actionLog.DestinationType and pass
the controller's _localizer into Resolve so TypeName is localized; to enable
this, update ConvertPersonStatus (either add an IStringLocalizer parameter or
make it an instance method) so callers can forward _localizer into
DestinationResolutionHelper.Resolve when building statusResult for ActionLog
entries (especially where ActionTypeId == RespondingToScene and DestinationType
may be null).
| var destinationType = al.GetEffectiveDestinationType(); | ||
| var destination = DestinationResolutionHelper.Resolve(al?.DestinationId, al?.DestinationType, null, calls, stations, pois); | ||
| var personnelViewModel = await Models.BigBoardX.PersonnelViewModel.Create(u.Name, al, us, department, respondingToDepartment, group, u.Roles, callNumber, destination); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there is no overload of DestinationResolutionHelper.Resolve without localizer.
fd -e cs DestinationResolutionHelper.cs --exec cat {}
echo "---"
rg -nP --type=cs '\bDestinationResolutionHelper\.Resolve\s*\(' -C1Repository: Resgrid/Core
Length of output: 8164
Remove unused destinationType and add missing _localizer parameter to Resolve call.
Line 366 computes destinationType but never uses it. Line 367 calls DestinationResolutionHelper.Resolve(...) without the required IStringLocalizer parameter, which will not compile. The _localizer field is already injected and should be passed.
🛠️ Suggested fix
- var destinationType = al.GetEffectiveDestinationType();
- var destination = DestinationResolutionHelper.Resolve(al?.DestinationId, al?.DestinationType, null, calls, stations, pois);
+ var destination = DestinationResolutionHelper.Resolve(al?.DestinationId, al?.DestinationType, null, calls, stations, pois, _localizer);📝 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.
| var destinationType = al.GetEffectiveDestinationType(); | |
| var destination = DestinationResolutionHelper.Resolve(al?.DestinationId, al?.DestinationType, null, calls, stations, pois); | |
| var personnelViewModel = await Models.BigBoardX.PersonnelViewModel.Create(u.Name, al, us, department, respondingToDepartment, group, u.Roles, callNumber, destination); | |
| var destination = DestinationResolutionHelper.Resolve(al?.DestinationId, al?.DestinationType, null, calls, stations, pois, _localizer); | |
| var personnelViewModel = await Models.BigBoardX.PersonnelViewModel.Create(u.Name, al, us, department, respondingToDepartment, group, u.Roles, callNumber, destination); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/LinksController.cs` around lines 366 -
368, Remove the unused local variable destinationType and update the call to
DestinationResolutionHelper.Resolve by passing the injected _localizer instance
instead of null (i.e., replace the third argument null with _localizer) so the
call becomes Resolve(al?.DestinationId, al?.DestinationType, _localizer, calls,
stations, pois); keep the rest of the call to PersonnelViewModel.Create
unchanged.
| state.DestinationId = destination; | ||
| state.DestinationType = type; | ||
|
|
||
| if (!await IsValidDestinationAsync(destination, type)) | ||
| return BadRequest(); |
There was a problem hiding this comment.
Validation order is inconsistent and skips the destination > 0 guard.
Compared to SetUnitStateWithDest (lines 647–654) and SetUnitStateForMultiple (lines 695–701) — which only set/validate when destination > 0 and validate before assignment — this method:
- Always assigns
DestinationId/DestinationTyperegardless of whetherdestination > 0, so a request withdestination = 0will still persist a zero destination instead of being treated as "no destination". - Validates after assignment, which is awkward and means an invalid destination short-circuits the whole batch with
BadRequest()mid-loop, leaving prior units already updated.
Recommend mirroring the pattern in SetUnitStateWithDest for consistency and correctness.
🛠️ Suggested fix
- state.DestinationId = destination;
- state.DestinationType = type;
-
- if (!await IsValidDestinationAsync(destination, type))
- return BadRequest();
+ if (destination > 0)
+ {
+ if (!await IsValidDestinationAsync(destination, type))
+ return BadRequest();
+
+ state.DestinationId = destination;
+ state.DestinationType = type;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 740 -
744, The current loop assigns state.DestinationId and state.DestinationType
before validating and does so even when destination == 0; change the logic to
mirror SetUnitStateWithDest and SetUnitStateForMultiple: only call
IsValidDestinationAsync when destination > 0, perform the
IsValidDestinationAsync check BEFORE assigning to
state.DestinationId/state.DestinationType, and skip or treat destination == 0 as
"no destination" (do not persist a zero value). Ensure you reference
DestinationId, DestinationType and IsValidDestinationAsync when making the
change so validation precedes assignment and you avoid mid-loop partial updates.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs (1)
268-277:⚠️ Potential issue | 🔴 CriticalComputed
destinationTypeis discarded; legacyActionLogdestinations remain unresolved and TypeName isn't localized.
actionLog.GetEffectiveDestinationType()is invoked on Line 270 but never used — Line 271 passes the rawactionLog.DestinationTypeinstead. For the exact caseGetEffectiveDestinationTypeexists to handle (anActionTypeId == RespondingToScenerow withDestinationType == null),Resolve(...)will seeeffectiveDestinationType == Noneand (withallowCrossTypeFallback == false) return only the id — leavingDestinationName,DestinationAddress, andDestinationTypeNameempty in the v4 API response.Additionally, no
IStringLocalizeris forwarded toResolve(...), soTypeNamefalls back to hardcoded English regardless of caller locale, breaking the PR's multi-lingual goal. The controller has no localizer injected at all.🛠️ Suggested fix
- if (actionLog.DestinationId.HasValue) - { - var destinationType = actionLog.GetEffectiveDestinationType(); - var destination = DestinationResolutionHelper.Resolve(actionLog.DestinationId, actionLog.DestinationType, null, activeCalls, stations, pois); + if (actionLog.DestinationId.HasValue) + { + var effectiveType = actionLog.DestinationType ?? (int)actionLog.GetEffectiveDestinationType(); + var destination = DestinationResolutionHelper.Resolve(actionLog.DestinationId, effectiveType, null, activeCalls, stations, pois, localizer);This requires injecting
IStringLocalizer<Resgrid.Localization.Common>into the controller, threading it throughConvertPersonStatus(or making it an instance method), and updating call sites accordingly. As per coding guidelines: "Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions" — passing the localizer as an explicit parameter is preferable to capturing it via instance state if you want to keep the method static.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs` around lines 268 - 277, The code calls actionLog.GetEffectiveDestinationType() but discards it and passes actionLog.DestinationType into DestinationResolutionHelper.Resolve, causing unresolved legacy destinations; also no IStringLocalizer is provided so TypeName is not localized—inject IStringLocalizer<Resgrid.Localization.Common> into PersonnelStatusesController, add a parameter for it to ConvertPersonStatus (or convert ConvertPersonStatus to an instance method) and thread that localizer through to DestinationResolutionHelper.Resolve, and modify the Resolve call here to use the computed effectiveDestinationType (result of GetEffectiveDestinationType()) instead of actionLog.DestinationType so DestinationName/Address/TypeName are correctly resolved and localized.Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs (2)
64-64:⚠️ Potential issue | 🟡 Minor
localizerargument missing in fallback Station resolution.Lines 68 and 72 pass
localizertoResolveByType, but Line 64 does not. The signature now defaultslocalizertonull, so this compiles, but the Station-fallback path will silently return a non-localizedTypeName. Passlocalizerfor consistency:🛠️ Suggested fix
- var fallbackStation = ResolveByType(destinationId.Value, DestinationEntityTypes.Station, activeCalls, groups, pois); + var fallbackStation = ResolveByType(destinationId.Value, DestinationEntityTypes.Station, activeCalls, groups, pois, localizer);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs` at line 64, The fallback station resolution call creating fallbackStation calls ResolveByType without passing the localizer, causing a non-localized TypeName; update the invocation that assigns fallbackStation so it passes the existing localizer argument (same way the later ResolveByType calls at the other code paths do) to ensure the resolved Station TypeName is localized; look for the variable fallbackStation and the ResolveByType(...) call and add the localizer parameter to match the method signature.
38-58:⚠️ Potential issue | 🟡 Minor
Name-based "found" detection can return the wrong type on cross-table ID matches.Each branch (
SupportsStations/SupportsCalls/SupportsPois) treats resolution as successful only whenresult.Nameis non-blank, then falls through to the next type when blank. Since IDs are independent acrossDepartmentGroup,Call, andPoi, an entity that exists but has a blank display name (e.g., a station with emptyName) will be skipped, and a numerically equal id in a different table — if it resolves to a non-blank Name — will be returned in its place.Recommend signalling "found" explicitly (e.g., have
ResolveByTypereturnnullwhen no entity matches, or expose aFoundflag onResolvedDestinationData), then check that instead ofstring.IsNullOrWhiteSpace(result.Name).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs` around lines 38 - 58, The current branches in DestinationResolutionHelper (SupportsStations/SupportsCalls/SupportsPois) treat a resolution as successful by checking ResolvedDestinationData.Name, which misidentifies cross-table matches when one entity has an empty Name; update ResolveByType to signal success explicitly (either return null when no entity matches or add a bool Found flag/property to ResolvedDestinationData) and change the callers in DestinationResolutionHelper to check that explicit signal (e.g., result != null or result.Found) instead of string.IsNullOrWhiteSpace(result.Name) when deciding to return the resolved value.Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs (1)
740-744:⚠️ Potential issue | 🟠 MajorValidation runs after assignment and missing
destination > 0guard.This branch still mirrors the issue noted previously:
state.DestinationId/state.DestinationTypeare set unconditionally, and validation runs after assignment. A request withdestination = 0will persist a zero destination instead of being treated as "no destination", and an invalid destination short-circuits the batch withBadRequest()mid-loop — leaving prior units already updated. Mirror the pattern inSetUnitStateWithDest(Lines 647–654) andSetUnitStateForMultiple(Lines 695–701) for consistency.🛠️ Suggested fix
- state.DestinationId = destination; - state.DestinationType = type; - - if (!await IsValidDestinationAsync(destination, type)) - return BadRequest(); + if (destination > 0) + { + if (!await IsValidDestinationAsync(destination, type)) + return BadRequest(); + + state.DestinationId = destination; + state.DestinationType = type; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 740 - 744, The code sets state.DestinationId/state.DestinationType before validating and lacks a destination>0 guard, causing zero/invalid destinations to persist and invalid requests to short-circuit mid-loop; update this block to first check if destination > 0 and run await IsValidDestinationAsync(destination, type) before assigning state.DestinationId/state.DestinationType, and if following the batch pattern used in SetUnitStateWithDest and SetUnitStateForMultiple, avoid returning BadRequest() inside the loop—mirror those methods' flow (validate first, then assign; for multi-unit updates follow the same error-handling/continuation strategy).Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (1)
5118-5123:⚠️ Potential issue | 🟠 Major
DestinationTypeNamestill points clients to the wrong branch key.
DestinationPoiTypeIdis a POI subtype, not a destination discriminator across Station/Call/POI. This still leavesCallResultDatawithout a stable machine-readable destination type for branching logic.Proposed doc/contract adjustment
- Localized display label for the destination type (e.g. "POI", "Station"). Not suitable - for programmatic branching; use <see cref="P:Resgrid.Web.Services.Models.v4.Calls.CallResultData.DestinationPoiTypeId"/> as the - machine-readable POI type identifier instead. + Localized display label for the destination type (e.g. "POI", "Station"). + Not suitable for programmatic branching. + Use a machine-readable destination discriminator (for example: + None = 0, Station = 1, Call = 2, POI = 3).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 5118 - 5123, The XML summary for CallResultData.DestinationTypeName incorrectly points clients to DestinationPoiTypeId (a POI subtype) — update the documentation to reference the correct machine-readable destination discriminator (or add one) so consumers can branch across Station/Call/POI; specifically, modify the summary on DestinationTypeName in CallResultData to stop directing readers to DestinationPoiTypeId and instead reference the correct property name (e.g., DestinationTypeId or the actual existing discriminator) and clarify that DestinationPoiTypeId is only a POI subtype.
🧹 Nitpick comments (5)
Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs (1)
1708-1725: Cast to undefined enum members silently returnsfalse.
(DestinationEntityTypes)destinationTypewill accept any int (e.g.,99) and fall through theswitchtodefault → false. That's safe today, but to be explicit and consistent withToDestinationEntityType()elsewhere in the codebase, preferdestinationType.ToDestinationEntityType()(with the int wrapped toint?) or anEnum.IsDefinedguard. Optional refactor for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 1708 - 1725, The switch in IsValidDestinationAsync currently casts destinationType with (DestinationEntityTypes)destinationType which silently accepts invalid ints and falls to default; change the conversion to use the shared helper ToDestinationEntityType (wrapping the int as nullable if needed) or validate with Enum.IsDefined before switching so only valid DestinationEntityTypes are handled; update the switch to use the validated/converted enum value and preserve existing calls to _departmentGroupsService.GetGroupByIdAsync, _callsService.GetCallByIdAsync, and _mappingService.GetDestinationPOIByIdAsync.Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs (2)
421-438: Same destination-validation logic duplicated across three controllers.
IsValidDestinationAsyncis essentially identical to the one inWeb/Resgrid.Web/Areas/User/Controllers/UnitsController.cs(Lines 1708-1725) andWeb/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs(Lines 283-300). Consider extracting to a small reusable component (e.g., aDestinationValidatorservice taking the three injected services + DepartmentId, or a static helper that accepts the services as parameters) to centralize the rules and keep them in sync as destination types evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs` around lines 421 - 438, Duplicate destination-validation logic in IsValidDestinationAsync should be extracted into a reusable component: create a DestinationValidator service/class that encapsulates the switch over DestinationEntityTypes and uses the existing services (_departmentGroupsService, _callsService, _mappingService) and DepartmentId to validate a destination; replace the local IsValidDestinationAsync implementations in UnitStatusController, UnitsController, and PersonnelStatusesController with calls to a single DestinationValidator.ValidateAsync(destinationId, destinationType, departmentId) (or inject DepartmentId into the service) so the validation logic is centralized and testable.
256-266: Useint.TryParseforRespondingToinstead of relying on the outertry/catch.
int.Parse(stateInput.RespondingTo)is invoked twice (Lines 256 and 259). A non-numeric value throws and is only suppressed by the broadcatchat Line 332, returningBadRequest()after exception overhead and an unstructured log. Prefer:♻️ Proposed refactor
- if (!string.IsNullOrWhiteSpace(stateInput.RespondingTo) && int.Parse(stateInput.RespondingTo) > 0) - { - var destinationType = stateInput.RespondingToType ?? (int)DestinationEntityTypes.Call; - var destinationId = int.Parse(stateInput.RespondingTo); + if (!string.IsNullOrWhiteSpace(stateInput.RespondingTo) + && int.TryParse(stateInput.RespondingTo, out var destinationId) + && destinationId > 0) + { + var destinationType = stateInput.RespondingToType ?? (int)DestinationEntityTypes.Call;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs` around lines 256 - 266, Replace the two int.Parse calls on stateInput.RespondingTo with a single int.TryParse to safely parse the value; if TryParse fails or parses to <= 0, return BadRequest immediately, otherwise use the parsed destinationId for the subsequent IsValidDestinationAsync check and for assigning state.DestinationId and state.DestinationType (use stateInput.RespondingToType ?? (int)DestinationEntityTypes.Call for the destinationType). This removes the duplicate parse and avoids relying on the outer try/catch to handle invalid numeric input.Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs (1)
283-300: Same destination-validation logic exists verbatim in two other controllers.This
IsValidDestinationAsyncis duplicated inWeb/Resgrid.Web/Areas/User/Controllers/UnitsController.cs(Lines 1708-1725) andWeb/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs(Lines 421-438). Extracting to a shared validator/helper would prevent divergence as destination types evolve. As per coding guidelines: avoid violations of DRY.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs` around lines 283 - 300, The IsValidDestinationAsync logic is duplicated; extract it into a shared service (e.g., IDestinationValidator with implementation DestinationValidator) that exposes Task<bool> IsValidDestinationAsync(int destinationId, int destinationType) and internally uses _departmentGroupsService.GetGroupByIdAsync, _callsService.GetCallByIdAsync and _mappingService.GetDestinationPOIByIdAsync with the same DepartmentId checks; register the new service in DI and replace the local IsValidDestinationAsync implementations in PersonnelStatusesController, UnitsController, and UnitStatusController to call the shared IDestinationValidator.IsValidDestinationAsync to avoid duplication and keep behavior identical.Web/Resgrid.Web.Services/Resgrid.Web.Services.xml (1)
9495-9499: MakeDestinationTypeenum mapping explicit for client parity.This summary is clear but less precise than related docs. Adding numeric values here will reduce client-side enum drift.
Suggested doc refinement
- Destination type (Station, Call, or POI). + Machine-readable destination type (None = 0, Station = 1, Call = 2, POI = 3).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 9495 - 9499, Update the XML doc for Resgrid.Web.Services.Models.v4.UnitStatus.UnitStatusResultData.DestinationType to explicitly include the enum numeric mappings used by the API so clients can reliably map values; edit the <summary> to list each enum member with its integer value (e.g., Station = 0, Call = 1, POI = 2) or a note that the numeric values are stable, referencing DestinationType by name so consumers know exactly which numeric value corresponds to each member.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs`:
- Around line 372-377: The DestinationResolutionHelper.Resolve call in
UnitStatusController currently omits localization, so inject an
IStringLocalizer<Resgrid.Localization.Common> into UnitStatusController
(constructor), make ConvertUnitStatusData accept the IStringLocalizer (or
convert ConvertUnitStatusData to an instance method so it can access the
injected localizer), and pass that localizer into
DestinationResolutionHelper.Resolve(...) so resolvedDestination.TypeName is
localized before you assign to destinationTypeName / DestinationTypeName; update
all calls to ConvertUnitStatusData accordingly.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 7680-7684: Update the RespondingToType documentation on the
SavePersonsStatusesInput (and the two other input models noted) to include the
missing enum baseline value "None = 0" and explicitly list all three destination
types (None = 0, Station = 1, Call = 2, POI = 3); also revise the adjacent
RespondingTo summaries to mention POI support and the meaning of None
(unset/cleared destination) so the API docs consistently describe unset/cleared
destinations and POI flows for the RespondingTo/RespondingToType members.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs`:
- Around line 1550-1584: AppendDestinationOptions and
AppendDestinationMenuEntries embed English UI text; change them to use the
IStringLocalizer<Resgrid.Localization.Common> instead of hardcoded strings.
Update the method signatures (e.g., AppendDestinationOptions and
AppendDestinationMenuEntries) to accept an
IStringLocalizer<Resgrid.Localization.Common> localizer parameter (or make them
instance methods and use the controller's _localizer), replace labels like
"Calls", "Stations", "POIs", "Call {id}:{name}", and "Station: {name}" and any
dropdown header HTML with localizer lookups (e.g., localizer["Calls"],
localizer["Stations"], localizer["POIs"], and localizer["CallFormat"] with
formatted args), change the GroupBy fallback key from literal "POIs" to
localizer["POIs"], ensure all localized strings are HTML-encoded via
HttpUtility.HtmlEncode before appending, and update all call sites to pass the
localizer or use the instance method so compilation succeeds.
---
Duplicate comments:
In `@Core/Resgrid.Model/Helpers/DestinationResolutionHelper.cs`:
- Line 64: The fallback station resolution call creating fallbackStation calls
ResolveByType without passing the localizer, causing a non-localized TypeName;
update the invocation that assigns fallbackStation so it passes the existing
localizer argument (same way the later ResolveByType calls at the other code
paths do) to ensure the resolved Station TypeName is localized; look for the
variable fallbackStation and the ResolveByType(...) call and add the localizer
parameter to match the method signature.
- Around line 38-58: The current branches in DestinationResolutionHelper
(SupportsStations/SupportsCalls/SupportsPois) treat a resolution as successful
by checking ResolvedDestinationData.Name, which misidentifies cross-table
matches when one entity has an empty Name; update ResolveByType to signal
success explicitly (either return null when no entity matches or add a bool
Found flag/property to ResolvedDestinationData) and change the callers in
DestinationResolutionHelper to check that explicit signal (e.g., result != null
or result.Found) instead of string.IsNullOrWhiteSpace(result.Name) when deciding
to return the resolved value.
In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs`:
- Around line 268-277: The code calls actionLog.GetEffectiveDestinationType()
but discards it and passes actionLog.DestinationType into
DestinationResolutionHelper.Resolve, causing unresolved legacy destinations;
also no IStringLocalizer is provided so TypeName is not localized—inject
IStringLocalizer<Resgrid.Localization.Common> into PersonnelStatusesController,
add a parameter for it to ConvertPersonStatus (or convert ConvertPersonStatus to
an instance method) and thread that localizer through to
DestinationResolutionHelper.Resolve, and modify the Resolve call here to use the
computed effectiveDestinationType (result of GetEffectiveDestinationType())
instead of actionLog.DestinationType so DestinationName/Address/TypeName are
correctly resolved and localized.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 5118-5123: The XML summary for CallResultData.DestinationTypeName
incorrectly points clients to DestinationPoiTypeId (a POI subtype) — update the
documentation to reference the correct machine-readable destination
discriminator (or add one) so consumers can branch across Station/Call/POI;
specifically, modify the summary on DestinationTypeName in CallResultData to
stop directing readers to DestinationPoiTypeId and instead reference the correct
property name (e.g., DestinationTypeId or the actual existing discriminator) and
clarify that DestinationPoiTypeId is only a POI subtype.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs`:
- Around line 740-744: The code sets state.DestinationId/state.DestinationType
before validating and lacks a destination>0 guard, causing zero/invalid
destinations to persist and invalid requests to short-circuit mid-loop; update
this block to first check if destination > 0 and run await
IsValidDestinationAsync(destination, type) before assigning
state.DestinationId/state.DestinationType, and if following the batch pattern
used in SetUnitStateWithDest and SetUnitStateForMultiple, avoid returning
BadRequest() inside the loop—mirror those methods' flow (validate first, then
assign; for multi-unit updates follow the same error-handling/continuation
strategy).
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.cs`:
- Around line 283-300: The IsValidDestinationAsync logic is duplicated; extract
it into a shared service (e.g., IDestinationValidator with implementation
DestinationValidator) that exposes Task<bool> IsValidDestinationAsync(int
destinationId, int destinationType) and internally uses
_departmentGroupsService.GetGroupByIdAsync, _callsService.GetCallByIdAsync and
_mappingService.GetDestinationPOIByIdAsync with the same DepartmentId checks;
register the new service in DI and replace the local IsValidDestinationAsync
implementations in PersonnelStatusesController, UnitsController, and
UnitStatusController to call the shared
IDestinationValidator.IsValidDestinationAsync to avoid duplication and keep
behavior identical.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs`:
- Around line 421-438: Duplicate destination-validation logic in
IsValidDestinationAsync should be extracted into a reusable component: create a
DestinationValidator service/class that encapsulates the switch over
DestinationEntityTypes and uses the existing services (_departmentGroupsService,
_callsService, _mappingService) and DepartmentId to validate a destination;
replace the local IsValidDestinationAsync implementations in
UnitStatusController, UnitsController, and PersonnelStatusesController with
calls to a single DestinationValidator.ValidateAsync(destinationId,
destinationType, departmentId) (or inject DepartmentId into the service) so the
validation logic is centralized and testable.
- Around line 256-266: Replace the two int.Parse calls on
stateInput.RespondingTo with a single int.TryParse to safely parse the value; if
TryParse fails or parses to <= 0, return BadRequest immediately, otherwise use
the parsed destinationId for the subsequent IsValidDestinationAsync check and
for assigning state.DestinationId and state.DestinationType (use
stateInput.RespondingToType ?? (int)DestinationEntityTypes.Call for the
destinationType). This removes the duplicate parse and avoids relying on the
outer try/catch to handle invalid numeric input.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml`:
- Around line 9495-9499: Update the XML doc for
Resgrid.Web.Services.Models.v4.UnitStatus.UnitStatusResultData.DestinationType
to explicitly include the enum numeric mappings used by the API so clients can
reliably map values; edit the <summary> to list each enum member with its
integer value (e.g., Station = 0, Call = 1, POI = 2) or a note that the numeric
values are stable, referencing DestinationType by name so consumers know exactly
which numeric value corresponds to each member.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs`:
- Around line 1708-1725: The switch in IsValidDestinationAsync currently casts
destinationType with (DestinationEntityTypes)destinationType which silently
accepts invalid ints and falls to default; change the conversion to use the
shared helper ToDestinationEntityType (wrapping the int as nullable if needed)
or validate with Enum.IsDefined before switching so only valid
DestinationEntityTypes are handled; update the switch to use the
validated/converted enum value and preserve existing calls to
_departmentGroupsService.GetGroupByIdAsync, _callsService.GetCallByIdAsync, and
_mappingService.GetDestinationPOIByIdAsync.
🪄 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: 4e89fff9-8c23-4139-85c0-43c960892c99
📒 Files selected for processing (6)
Core/Resgrid.Model/DestinationEntityTypes.csCore/Resgrid.Model/Helpers/DestinationResolutionHelper.csWeb/Resgrid.Web.Services/Controllers/v4/PersonnelStatusesController.csWeb/Resgrid.Web.Services/Controllers/v4/UnitStatusController.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWeb/Resgrid.Web/Areas/User/Controllers/UnitsController.cs
| var resolvedDestination = DestinationResolutionHelper.Resolve(stateFound.DestinationId, stateFound.DestinationType, customState?.DetailType, activeCalls, groups, pois); | ||
| destinationId = resolvedDestination.DestinationId; | ||
| destinationType = resolvedDestination.DestinationType; | ||
| destinationName = resolvedDestination.Name; | ||
| destinationAddress = resolvedDestination.Address; | ||
| destinationTypeName = resolvedDestination.TypeName; |
There was a problem hiding this comment.
Destination type names won't be localized in v4 unit-status responses.
DestinationResolutionHelper.Resolve(...) is called without an IStringLocalizer, so result.TypeName (which is mapped to DestinationTypeName on Line 415) will fall through to the hardcoded English defaults in DestinationEntityTypeExtensions.GetDisplayName() and PoiDisplayHelper.GetTypeName(). This contradicts the PR's multi-lingual goal for v4 API consumers.
Inject IStringLocalizer<Resgrid.Localization.Common> into UnitStatusController, thread it through ConvertUnitStatusData (or make the method an instance method), and pass it into Resolve(...) — same pattern that the user-area UnitsController already uses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Controllers/v4/UnitStatusController.cs` around lines
372 - 377, The DestinationResolutionHelper.Resolve call in UnitStatusController
currently omits localization, so inject an
IStringLocalizer<Resgrid.Localization.Common> into UnitStatusController
(constructor), make ConvertUnitStatusData accept the IStringLocalizer (or
convert ConvertUnitStatusData to an instance method so it can access the
injected localizer), and pass that localizer into
DestinationResolutionHelper.Resolve(...) so resolvedDestination.TypeName is
localized before you assign to destinationTypeName / DestinationTypeName; update
all calls to ConvertUnitStatusData accordingly.
| <member name="P:Resgrid.Web.Services.Models.v4.PersonnelStatuses.SavePersonsStatusesInput.RespondingToType"> | ||
| <summary> | ||
| Destination type for RespondingTo (Station = 1, Call = 2, POI = 3). | ||
| </summary> | ||
| </member> |
There was a problem hiding this comment.
Align RespondingToType docs with POI support and enum baseline.
These new docs omit None = 0, while status output docs include it, and the adjacent RespondingTo summaries still describe only Call/Station. That creates API ambiguity for unset/cleared destinations and POI flows.
Proposed wording update (apply to all three input models)
- Destination type for RespondingTo (Station = 1, Call = 2, POI = 3).
+ Destination type for RespondingTo (None = 0, Station = 1, Call = 2, POI = 3).- The Call/Station the unit is responding to
+ Destination id the user/unit is responding to (Station, Call, or POI)Also applies to: 7770-7774, 9340-9344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web.Services/Resgrid.Web.Services.xml` around lines 7680 - 7684,
Update the RespondingToType documentation on the SavePersonsStatusesInput (and
the two other input models noted) to include the missing enum baseline value
"None = 0" and explicitly list all three destination types (None = 0, Station =
1, Call = 2, POI = 3); also revise the adjacent RespondingTo summaries to
mention POI support and the meaning of None (unset/cleared destination) so the
API docs consistently describe unset/cleared destinations and POI flows for the
RespondingTo/RespondingToType members.
| private static void AppendDestinationOptions(StringBuilder sb, CustomStateDetail state, List<Call> activeCalls, List<DepartmentGroup> stations, List<Poi> destinationPois) | ||
| { | ||
| if (state.DetailType.SupportsCalls()) | ||
| { | ||
| sb.Append("<optgroup label='Calls'>"); | ||
| foreach (var call in activeCalls) | ||
| { | ||
| sb.Append($"<option value='{(int)DestinationEntityTypes.Call}:{call.CallId}'>Call {call.GetIdentifier()}:{HttpUtility.HtmlEncode(call.Name)}</option>"); | ||
| } | ||
| sb.Append("</optgroup>"); | ||
| } | ||
|
|
||
| if (state.DetailType.SupportsStations()) | ||
| { | ||
| sb.Append("<optgroup label='Stations'>"); | ||
| foreach (var station in stations) | ||
| { | ||
| sb.Append($"<option value='{(int)DestinationEntityTypes.Station}:{station.DepartmentGroupId}'>Station: {HttpUtility.HtmlEncode(station.Name)}</option>"); | ||
| } | ||
| sb.Append("</optgroup>"); | ||
| } | ||
|
|
||
| if (state.DetailType.SupportsPois()) | ||
| { | ||
| foreach (var poiGroup in destinationPois.GroupBy(x => !String.IsNullOrWhiteSpace(x.Type?.Name) ? x.Type.Name : "POIs")) | ||
| { | ||
| sb.Append($"<optgroup label=\"{HttpUtility.HtmlEncode(poiGroup.Key)}\">"); | ||
| foreach (var poi in poiGroup.OrderBy(x => x.Name).ThenBy(x => x.Address).ThenBy(x => x.Note)) | ||
| { | ||
| sb.Append($"<option value='{(int)DestinationEntityTypes.Poi}:{poi.PoiId}'>{HttpUtility.HtmlEncode(GetPoiDisplayText(poi))}</option>"); | ||
| } | ||
| sb.Append("</optgroup>"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Hardcoded English labels in destination dropdowns conflict with the PR's multi-lingual goal.
AppendDestinationOptions and AppendDestinationMenuEntries emit user-visible strings like optgroup label='Calls', 'Stations', 'POIs', Call {id}:{name}, Station: {name}, and <li class='dropdown-header'>Calls</li> directly in English. Since this PR explicitly addresses multi-lingual support and the controller already injects IStringLocalizer<Resgrid.Localization.Common>, consider routing these labels through _localizer (would require making these methods instance methods or threading the localizer through). The same applies to the "POIs" GroupBy fallback key on Lines 1574 and 1689.
Also applies to: 1663-1701
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/UnitsController.cs` around lines 1550
- 1584, AppendDestinationOptions and AppendDestinationMenuEntries embed English
UI text; change them to use the IStringLocalizer<Resgrid.Localization.Common>
instead of hardcoded strings. Update the method signatures (e.g.,
AppendDestinationOptions and AppendDestinationMenuEntries) to accept an
IStringLocalizer<Resgrid.Localization.Common> localizer parameter (or make them
instance methods and use the controller's _localizer), replace labels like
"Calls", "Stations", "POIs", "Call {id}:{name}", and "Station: {name}" and any
dropdown header HTML with localizer lookups (e.g., localizer["Calls"],
localizer["Stations"], localizer["POIs"], and localizer["CallFormat"] with
formatted args), change the GroupBy fallback key from literal "POIs" to
localizer["POIs"], ensure all localized strings are HTML-encoded via
HttpUtility.HtmlEncode before appending, and update all call sites to pass the
localizer or use the instance method so compilation succeeds.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Summary by CodeRabbit
New Features
Improvements