Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughAdds ContactId to route stops, database migrations, repository/query/config support, a service method to fetch stops by contact (filtered by department plans), controller/view updates for contact-aware stop management, extensive client-side UI for creating/managing stops, and centralized client auth token helpers. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Ctrl as ContactsController
participant Svc as RouteService
participant Repo as RouteStopsRepository
participant DB as Database
User->>Ctrl: Request contact view (contactId)
Ctrl->>Svc: GetRouteStopsForContactAsync(contactId, departmentId)
Svc->>Repo: GetStopsByContactIdAsync(contactId)
Repo->>DB: SELECT * FROM RouteStops WHERE ContactId = ? AND IsDeleted = false
DB-->>Repo: RouteStop records
Repo-->>Svc: IEnumerable<RouteStop>
Svc->>Svc: Filter by department's active RoutePlanIds
Svc-->>Ctrl: List<RouteStop>
Ctrl-->>User: Render view with RouteStops and RoutePlans
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.addArchivedCall.js (1)
112-127: Missingr.okcheck for consistency.Unlike
resgrid.dispatch.newcall.js, this fetch call doesn't validateresponse.okbefore parsing JSON. Consider adding the same error handling pattern for consistency.🤖 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` around lines 112 - 127, The fetch in resgrid.dispatch.addArchivedCall (the ForwardGeocode call that currently does .then(function(r) { return r.json(); })) lacks a response.ok check; update the promise chain to first check the Response object (r.ok) and handle non-2xx responses by logging or throwing an error before calling r.json(), then proceed to inspect result.Data and call resgrid.dispatch.addArchivedCall.setMarkerLocation(lat, lng) as before; ensure errors propagate to the existing .catch so failures are logged consistently (use the same pattern as in resgrid.dispatch.newcall.js and keep evt.preventDefault()).Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.editcall.js (2)
320-332: Consider extractinggetAuthToken()to a shared utility.This identical function is duplicated across at least 5 other files (
resgrid.dispatch.newcall.js,resgrid.dispatch.addArchivedCall.js,resgrid.routes.edit.js,resgrid.routes.new.js). Centralizing this in a shared module (e.g.,resgrid.common.js) would reduce maintenance burden and ensure consistent behavior.Additionally, the empty
catch (e) {}swallows all exceptions silently, which could hide issues during debugging. Consider at minimum logging the error to console.🤖 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.editcall.js` around lines 320 - 332, The duplicated getAuthToken() function should be extracted into a shared utility module (e.g., export a function getAuthToken from a new resgrid.common.js) and all copies (including the getAuthToken function in resgrid.dispatch.editcall.js) should be replaced with imports or references to that shared function; update callers to use the centralized getAuthToken() and export it from the new module. Also replace the silent catch in getAuthToken() with at least a console.error(e) (or use the project logger) so exceptions aren’t swallowed. Ensure the new module is added to the build/bundling pipeline and update any script references so the shared function is available where those files run.
121-136: Inconsistent error handling compared tonewcall.js.This fetch call doesn't check
response.okbefore parsing JSON, unlike the same call inresgrid.dispatch.newcall.js(lines 115-118) which throws on non-OK responses. This inconsistency means failed requests will silently try to parse an error response as JSON here.Suggested improvement
- fetch(resgrid.absoluteApiBaseUrl + '/api/v4/Geocoding/ForwardGeocode?address=' + encodeURIComponent(where), { headers: { 'Authorization': 'Bearer ' + getAuthToken() } }) - .then(function(r) { return r.json(); }) + fetch(resgrid.absoluteApiBaseUrl + '/api/v4/Geocoding/ForwardGeocode?address=' + encodeURIComponent(where), { headers: { 'Authorization': 'Bearer ' + getAuthToken() } }) + .then(function(r) { + if (!r.ok) { throw new Error("Geocode request failed: " + r.status + " " + r.statusText); } + return r.json(); + })🤖 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.editcall.js` around lines 121 - 136, The fetch in resgrid.dispatch.editcall (the anonymous handler that calls fetch(.../Geocoding/ForwardGeocode?address=...)) currently parses r.json() without checking response.ok; update it to mirror resgrid.dispatch.newcall.js by first checking if response.ok and throwing an Error (including response.status/text) on non-OK responses before calling r.json(), so the .catch() handles HTTP errors instead of attempting to parse an error body; keep the existing success branch that sets Latitude/Longitude inputs and calls resgrid.dispatch.editcall.setMarkerLocation and preserve the Authorization header using getAuthToken().Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.new.js (1)
150-154: Redundant conditional logic.Both branches execute the same statement, making the condition unnecessary.
Suggested fix
if (contact) { - if (!$('#stopContactName').val()) $('#stopContactName').val(contact.name); - else $('#stopContactName').val(contact.name); + $('#stopContactName').val(contact.name); if (contact.phone) $('#stopContactNumber').val(contact.phone); }🤖 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/routes/resgrid.routes.new.js` around lines 150 - 154, The code handling the contact name contains a redundant if/else where both branches call $('#stopContactName').val(contact.name); remove the conditional and set the value once when contact is truthy: inside the existing if (contact) block call $('#stopContactName').val(contact.name); (and keep the existing phone assignment for contact.phone) so the checks reference the same variables (`#stopContactName`, contact.name, contact.phone) but avoid duplicate statements.Core/Resgrid.Services/RouteService.cs (1)
87-97: Add a fast-fail guard for invalid contact/department input.A quick check at Line [87] for empty
contactIdand invaliddepartmentIdmakes behavior explicit and avoids unnecessary data access.Proposed refactor
public async Task<List<RouteStop>> GetRouteStopsForContactAsync(string contactId, int departmentId) { + if (string.IsNullOrWhiteSpace(contactId) || departmentId <= 0) + return new List<RouteStop>(); + var stops = await _routeStopsRepository.GetStopsByContactIdAsync(contactId); if (stops == null) return new List<RouteStop>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Core/Resgrid.Services/RouteService.cs` around lines 87 - 97, GetRouteStopsForContactAsync currently calls repositories even when input is invalid; add a fast-fail guard at the start of GetRouteStopsForContactAsync that returns an empty List<RouteStop> immediately when contactId is null/empty or departmentId is <= 0 (or otherwise invalid). This prevents unnecessary calls to _routeStopsRepository/_routePlansRepository and makes intent explicit; place the check before any await calls and keep existing behavior for valid inputs.Web/Resgrid.Web/Areas/User/Views/Contacts/View.cshtml (1)
475-477: Avoid repeated plan lookup inside the Razor loop.Line [477] does a linear search for every stop. Precomputing a plan dictionary once will keep the view simpler and avoid O(n×m) lookups.
Proposed refactor
+@{ + var routePlansById = (Model.RoutePlans ?? new List<Resgrid.Model.RoutePlan>()) + .ToDictionary(p => p.RoutePlanId, p => p); +} ... `@foreach` (var stop in Model.RouteStops.OrderBy(s => s.AddedOn)) { - var plan = Model.RoutePlans?.FirstOrDefault(p => p.RoutePlanId == stop.RoutePlanId); + routePlansById.TryGetValue(stop.RoutePlanId, out var plan); <tr>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/Contacts/View.cshtml` around lines 475 - 477, The view currently does a linear search for each stop using Model.RoutePlans?.FirstOrDefault(p => p.RoutePlanId == stop.RoutePlanId), causing O(n×m) work; replace this by building a lookup (e.g., a dictionary keyed by RoutePlanId) once before the foreach and then use the dictionary to fetch the plan for each stop (use Model.RouteStops, Model.RoutePlans, RoutePlanId and stop.RoutePlanId to locate the right symbols); ensure you handle nulls/missing keys when reading from the dictionary to preserve existing behavior.Web/Resgrid.Web/Areas/User/Controllers/ContactsController.cs (1)
42-57: Consider reducing constructor dependency breadth in this controller.Adding
IRouteServicepushes this controller further into high-dependency territory. A small composition service (e.g., contact-details read model builder) would keep the controller leaner and easier to test.As per coding guidelines, "Minimize constructor injection; keep the number of injected dependencies small".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/ContactsController.cs` around lines 42 - 57, The ContactsController constructor currently injects many services (IContactsService, IDepartmentsService, IUserProfileService, IAddressService, IEventAggregator, ICallsService, IAuthorizationService, IUserDefinedFieldsService, IUdfRenderingService, IDepartmentGroupsService, IRouteService), which makes the controller hard to test and maintain; extract the logic that requires multiple services (especially the route/composition work introduced by IRouteService) into a new ContactDetailsReadModelBuilder (or similar composition service) that encapsulates the calls to IContactsService, IRouteService, IUserProfileService, IAddressService, IDepartmentGroupsService, etc., then replace those specific injected services in ContactsController with a single IContactDetailsBuilder (or IContactReadModelBuilder) dependency and update usages inside ContactsController to call the new builder methods; ensure the new builder is unit-tested and registered in DI so only the reduced dependency (IContactDetailsBuilder) appears on ContactsController.
🤖 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/Areas/User/Controllers/ContactsController.cs`:
- Around line 133-134: The code calls model.RouteStops.Count after assigning
model.RouteStops = await _routeService.GetRouteStopsForContactAsync(contactId,
DepartmentId) which can be null; update ContactsController to guard against null
by either assigning an empty collection when the service returns null or
checking for null before using Count (e.g., replace direct Count access with a
null-safe check or use (model.RouteStops ??
Enumerable.Empty<RouteStop>()).Count/Any()); ensure references to
GetRouteStopsForContactAsync and model.RouteStops are updated accordingly so no
null dereference can occur.
In `@Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs`:
- Around line 64-88: When creating RouteStop from client-supplied PendingStopDto
(the PendingStopsJson deserialization block that constructs new RouteStop and
sets ContactId) and in the AddStop flow, validate that the provided contactId
actually belongs to the current department before assigning or saving it: lookup
the contact (e.g., via the existing contact/people repository method used
elsewhere such as GetContact/GetPerson/GetById) and compare its DepartmentId to
the current department (from model.Plan or the controller context); if the
contact is missing or its DepartmentId does not match, clear or null out
ContactId and/or add a model error and prevent persisting the invalid
association. Ensure this same check is applied to the other creation path
referenced (the AddStop method around the 244-267 area).
- Around line 64-103: The code currently calls SaveRoutePlanAsync before
deserializing and validating model.PendingStopsJson, which can leave a saved
plan without stops if deserialization or any SaveRouteStopAsync fails; to fix,
first attempt to
JsonSerializer.Deserialize<List<PendingStopDto>>(model.PendingStopsJson,
options) inside a try-catch, validate the resulting pendingStops (ensure
required fields, parse PlannedArrival/PlannedDeparture to UTC), and only after
validation succeed call SaveRoutePlanAsync and then persist RouteStop entries
via SaveRouteStopAsync; alternatively wrap SaveRoutePlanAsync and the subsequent
SaveRouteStopAsync calls in a transactional scope (or use a Service/Repository
transaction) so that failures roll back; reference PendingStopsJson,
PendingStopDto, RouteStop, SaveRoutePlanAsync, and SaveRouteStopAsync when
making changes.
- Around line 94-97: The code in RoutesController that sets
stop.PlannedArrivalTime and stop.PlannedDepartureTime is treating HTML
datetime-local strings as server-local by calling .ToUniversalTime(); instead,
parse the string, mark the resulting DateTime as Kind=Unspecified (so it’s not
assumed server-local), resolve the correct timezone (department or user timezone
via the department's TimeZoneId or user preference), and convert to UTC with
TimeZoneInfo.ConvertTimeToUtc; apply this change for ps.PlannedArrival and
ps.PlannedDeparture (and the same logic at the other occurrence around the
274-277 block) so the conversion uses the intended timezone rather than
server-local.
In `@Web/Resgrid.Web/Areas/User/Views/Contacts/View.cshtml`:
- Around line 35-36: Replace the hardcoded English strings added for the Routes
tab and table headers with the view's existing localization calls: change the
anchor text "Routes" (the tab label/icon line containing <i class="fa
fa-map-signs"></i> Routes) and the table header strings "Route", "Stop Name" and
the priority labels (the header cells around lines 467-472 and 490-494) to use
the same localization helper/pattern already used elsewhere in this Razor view
(e.g., the existing `@Resources/Html` helper calls used for other tab and header
strings) so they pull from resource keys (e.g., Routes, Route, StopName,
PriorityLow/Medium/High) instead of hardcoded English.
In `@Web/Resgrid.Web/Areas/User/Views/Routes/Edit.cshtml`:
- Around line 258-267: The stop contact dropdown (`#stopContactPicker`) clears
visually but leaves the hidden contact id (`#stopContactId`) populated; update the
change handler for the stop contact picker in the routes edit JS (the function
that handles the stop contact selection in resgrid.routes.edit.js) so that when
the selected value is empty it explicitly clears/reset the hidden input
`#stopContactId` (and any related name/number fields) to avoid saving the previous
contact id with a cleared picker.
- Line 296: availableContacts serialization can NRE when Model.Contacts is null
and is vulnerable to XSS because JsonConvert.SerializeObject doesn't escape
"</script>" by default; fix by serializing a safe fallback collection and
enabling JSON HTML-escaping. Replace the current expression using Model.Contacts
with Model.Contacts ?? Enumerable.Empty<ContactType>() (or .DefaultIfEmpty
pattern) and call JsonConvert.SerializeObject(..., new JsonSerializerSettings {
StringEscapeHandling = StringEscapeHandling.EscapeHtml }) when building the
anonymous objects (preserving id, GetName(), and phone fallback logic like
c.CellPhoneNumber ?? c.OfficePhoneNumber ?? c.HomePhoneNumber ?? ""), then pass
that escaped JSON to Html.Raw; ensure all nullable contact fields are coalesced
to empty strings to avoid null derefs.
In `@Web/Resgrid.Web/Areas/User/Views/Routes/New.cshtml`:
- Around line 179-181: Summary: The view's input IDs don't match the JavaScript
selectors, causing missing/invalid stop data; update the IDs to match the JS.
Change the select/input IDs currently named linkedCallId, stopLat, and stopLng
to stopCallId, stopLatitude, and stopLongitude (these are referenced by
resgrid.routes.new.js via selectors "#stopCallId", "#stopLatitude",
"#stopLongitude"); apply the same renaming to the other occurrence around lines
225-229 so both places use the JS-expected IDs.
- Around line 112-113: The view New.cshtml contains hardcoded UI strings (e.g.,
the button label "Add Stop" and field labels "Name", "Type", "Priority",
"Cancel") which must be replaced with the page's IViewLocalizer usage; update
the button text and all hardcoded labels in the block around the <i class="fa
fa-plus"></i> Add Stop and the related form section (also check lines 158-272)
to use localizer["Key"] entries matching existing resource conventions (create
sensible resource keys like "AddStop", "Name", "Type", "Priority", "Cancel" if
missing) so the UI uses localized strings consistently.
- Line 281: Replace the unsafe Html.Raw(JsonConvert.SerializeObject(...)) usage
for the availableContacts variable by serializing Model.Contacts with
JsonConvert.SerializeObject using JsonSerializerSettings that set
StringEscapeHandling = StringEscapeHandling.EscapeHtml; this ensures any
HTML-sensitive chars from c.GetName() or phone fields are escaped before
embedding in the script, keeping the variable assignment safe (refer to the
availableContacts variable, the Model.Contacts projection and the
GetName()/phone fields).
---
Nitpick comments:
In `@Core/Resgrid.Services/RouteService.cs`:
- Around line 87-97: GetRouteStopsForContactAsync currently calls repositories
even when input is invalid; add a fast-fail guard at the start of
GetRouteStopsForContactAsync that returns an empty List<RouteStop> immediately
when contactId is null/empty or departmentId is <= 0 (or otherwise invalid).
This prevents unnecessary calls to _routeStopsRepository/_routePlansRepository
and makes intent explicit; place the check before any await calls and keep
existing behavior for valid inputs.
In `@Web/Resgrid.Web/Areas/User/Controllers/ContactsController.cs`:
- Around line 42-57: The ContactsController constructor currently injects many
services (IContactsService, IDepartmentsService, IUserProfileService,
IAddressService, IEventAggregator, ICallsService, IAuthorizationService,
IUserDefinedFieldsService, IUdfRenderingService, IDepartmentGroupsService,
IRouteService), which makes the controller hard to test and maintain; extract
the logic that requires multiple services (especially the route/composition work
introduced by IRouteService) into a new ContactDetailsReadModelBuilder (or
similar composition service) that encapsulates the calls to IContactsService,
IRouteService, IUserProfileService, IAddressService, IDepartmentGroupsService,
etc., then replace those specific injected services in ContactsController with a
single IContactDetailsBuilder (or IContactReadModelBuilder) dependency and
update usages inside ContactsController to call the new builder methods; ensure
the new builder is unit-tested and registered in DI so only the reduced
dependency (IContactDetailsBuilder) appears on ContactsController.
In `@Web/Resgrid.Web/Areas/User/Views/Contacts/View.cshtml`:
- Around line 475-477: The view currently does a linear search for each stop
using Model.RoutePlans?.FirstOrDefault(p => p.RoutePlanId == stop.RoutePlanId),
causing O(n×m) work; replace this by building a lookup (e.g., a dictionary keyed
by RoutePlanId) once before the foreach and then use the dictionary to fetch the
plan for each stop (use Model.RouteStops, Model.RoutePlans, RoutePlanId and
stop.RoutePlanId to locate the right symbols); ensure you handle nulls/missing
keys when reading from the dictionary to preserve existing behavior.
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.addArchivedCall.js`:
- Around line 112-127: The fetch in resgrid.dispatch.addArchivedCall (the
ForwardGeocode call that currently does .then(function(r) { return r.json(); }))
lacks a response.ok check; update the promise chain to first check the Response
object (r.ok) and handle non-2xx responses by logging or throwing an error
before calling r.json(), then proceed to inspect result.Data and call
resgrid.dispatch.addArchivedCall.setMarkerLocation(lat, lng) as before; ensure
errors propagate to the existing .catch so failures are logged consistently (use
the same pattern as in resgrid.dispatch.newcall.js and keep
evt.preventDefault()).
In
`@Web/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.editcall.js`:
- Around line 320-332: The duplicated getAuthToken() function should be
extracted into a shared utility module (e.g., export a function getAuthToken
from a new resgrid.common.js) and all copies (including the getAuthToken
function in resgrid.dispatch.editcall.js) should be replaced with imports or
references to that shared function; update callers to use the centralized
getAuthToken() and export it from the new module. Also replace the silent catch
in getAuthToken() with at least a console.error(e) (or use the project logger)
so exceptions aren’t swallowed. Ensure the new module is added to the
build/bundling pipeline and update any script references so the shared function
is available where those files run.
- Around line 121-136: The fetch in resgrid.dispatch.editcall (the anonymous
handler that calls fetch(.../Geocoding/ForwardGeocode?address=...)) currently
parses r.json() without checking response.ok; update it to mirror
resgrid.dispatch.newcall.js by first checking if response.ok and throwing an
Error (including response.status/text) on non-OK responses before calling
r.json(), so the .catch() handles HTTP errors instead of attempting to parse an
error body; keep the existing success branch that sets Latitude/Longitude inputs
and calls resgrid.dispatch.editcall.setMarkerLocation and preserve the
Authorization header using getAuthToken().
In `@Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.new.js`:
- Around line 150-154: The code handling the contact name contains a redundant
if/else where both branches call $('#stopContactName').val(contact.name); remove
the conditional and set the value once when contact is truthy: inside the
existing if (contact) block call $('#stopContactName').val(contact.name); (and
keep the existing phone assignment for contact.phone) so the checks reference
the same variables (`#stopContactName`, contact.name, contact.phone) but avoid
duplicate statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eb583fdd-b82a-44f2-b9e4-a43bc1014229
📒 Files selected for processing (25)
Core/Resgrid.Model/Repositories/IRouteStopsRepository.csCore/Resgrid.Model/RouteStop.csCore/Resgrid.Model/Services/IRouteService.csCore/Resgrid.Services/RouteService.csProviders/Resgrid.Providers.Migrations/Migrations/M0054_AddContactIdToRouteStops.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0054_AddContactIdToRouteStopsPg.csRepositories/Resgrid.Repositories.DataRepository/Configs/SqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Queries/Routes/SelectRouteStopsByContactIdQuery.csRepositories/Resgrid.Repositories.DataRepository/RouteStopsRepository.csRepositories/Resgrid.Repositories.DataRepository/Servers/PostgreSql/PostgreSqlConfiguration.csRepositories/Resgrid.Repositories.DataRepository/Servers/SqlServer/SqlServerConfiguration.csWeb/Resgrid.Web/Areas/User/Controllers/ContactsController.csWeb/Resgrid.Web/Areas/User/Controllers/IndoorMapsController.csWeb/Resgrid.Web/Areas/User/Controllers/RoutesController.csWeb/Resgrid.Web/Areas/User/Models/Contacts/ViewContactView.csWeb/Resgrid.Web/Areas/User/Models/Routes/RouteViewModels.csWeb/Resgrid.Web/Areas/User/Views/Contacts/View.cshtmlWeb/Resgrid.Web/Areas/User/Views/IndoorMaps/Floors.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/New.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/dispatch/resgrid.dispatch.addArchivedCall.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/routes/resgrid.routes.edit.jsWeb/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.new.js
Comment on lines
+64
to
+103
| if (!string.IsNullOrWhiteSpace(model.PendingStopsJson)) | ||
| { | ||
| var options = new System.Text.Json.JsonSerializerOptions { PropertyNameCaseInsensitive = true }; | ||
| var pendingStops = System.Text.Json.JsonSerializer.Deserialize<List<PendingStopDto>>(model.PendingStopsJson, options); | ||
| if (pendingStops != null) | ||
| { | ||
| for (int i = 0; i < pendingStops.Count; i++) | ||
| { | ||
| var ps = pendingStops[i]; | ||
| var stop = new RouteStop | ||
| { | ||
| RoutePlanId = model.Plan.RoutePlanId, | ||
| Name = ps.Name, | ||
| Description = ps.Description, | ||
| StopType = ps.StopType, | ||
| Priority = ps.Priority, | ||
| Latitude = ps.Latitude, | ||
| Longitude = ps.Longitude, | ||
| Address = ps.Address, | ||
| CallId = ps.CallId, | ||
| EstimatedDwellMinutes = ps.DwellMinutes, | ||
| ContactName = ps.ContactName, | ||
| ContactNumber = ps.ContactNumber, | ||
| ContactId = ps.ContactId, | ||
| Notes = ps.Notes, | ||
| StopOrder = i + 1, | ||
| AddedOn = DateTime.UtcNow, | ||
| IsDeleted = false | ||
| }; | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(ps.PlannedArrival) && DateTime.TryParse(ps.PlannedArrival, out var arrivalDt)) | ||
| stop.PlannedArrivalTime = arrivalDt.ToUniversalTime(); | ||
| if (!string.IsNullOrWhiteSpace(ps.PlannedDeparture) && DateTime.TryParse(ps.PlannedDeparture, out var departureDt)) | ||
| stop.PlannedDepartureTime = departureDt.ToUniversalTime(); | ||
|
|
||
| await _routeService.SaveRouteStopAsync(stop, cancellationToken); | ||
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n ./Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs | sed -n '1,120p'Repository: Resgrid/Core
Length of output: 5005
🏁 Script executed:
git ls-files | grep -i routescontrollerRepository: Resgrid/Core
Length of output: 174
🏁 Script executed:
fd "RoutesController" -type fRepository: Resgrid/Core
Length of output: 226
🏁 Script executed:
rg "class RoutesController" -lRepository: Resgrid/Core
Length of output: 174
🏁 Script executed:
find . -path "*User/Controllers/RoutesController.cs" -type f -exec cat -n {} \;Repository: Resgrid/Core
Length of output: 13902
🏁 Script executed:
ls -la ./Web/Resgrid.Web/Areas/User/Controllers/ 2>/dev/null | head -20Repository: Resgrid/Core
Length of output: 1392
🏁 Script executed:
find . -name "RoutesController.cs" 2>/dev/nullRepository: Resgrid/Core
Length of output: 178
🏁 Script executed:
rg "SaveRoutePlanAsync" -A 10 -B 5Repository: Resgrid/Core
Length of output: 10502
Deserialize and validate pending stops before persisting the plan to ensure transactional consistency.
SaveRoutePlanAsync is called at line 62 before PendingStopsJson deserialization and stop persistence (lines 64–102). If JsonSerializer.Deserialize throws an exception or any SaveRouteStopAsync call fails mid-loop, the plan persists without its stops, creating an orphaned record. Wrap deserialization in a try-catch and move the plan save to occur only after stops are validated.
Suggested approach
- await _routeService.SaveRoutePlanAsync(model.Plan, cancellationToken);
-
- if (!string.IsNullOrWhiteSpace(model.PendingStopsJson))
+ List<PendingStopDto> pendingStops = new List<PendingStopDto>();
+ if (!string.IsNullOrWhiteSpace(model.PendingStopsJson))
{
- var options = new System.Text.Json.JsonSerializerOptions { PropertyNameCaseInsensitive = true };
- var pendingStops = System.Text.Json.JsonSerializer.Deserialize<List<PendingStopDto>>(model.PendingStopsJson, options);
- if (pendingStops != null)
- {
- ...
- }
+ try
+ {
+ var options = new System.Text.Json.JsonSerializerOptions { PropertyNameCaseInsensitive = true };
+ pendingStops = System.Text.Json.JsonSerializer.Deserialize<List<PendingStopDto>>(model.PendingStopsJson, options) ?? new List<PendingStopDto>();
+ }
+ catch (System.Text.Json.JsonException)
+ {
+ ModelState.AddModelError(nameof(model.PendingStopsJson), "Pending stops payload is invalid.");
+ model.Units = (await _unitsService.GetUnitsForDepartmentAsync(DepartmentId)).ToList();
+ model.Contacts = (await _contactsService.GetAllContactsForDepartmentAsync(DepartmentId)).ToList();
+ return View(model);
+ }
}
+
+ await _routeService.SaveRoutePlanAsync(model.Plan, cancellationToken);
+ // persist pendingStops afterwards🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs` around lines 64 -
103, The code currently calls SaveRoutePlanAsync before deserializing and
validating model.PendingStopsJson, which can leave a saved plan without stops if
deserialization or any SaveRouteStopAsync fails; to fix, first attempt to
JsonSerializer.Deserialize<List<PendingStopDto>>(model.PendingStopsJson,
options) inside a try-catch, validate the resulting pendingStops (ensure
required fields, parse PlannedArrival/PlannedDeparture to UTC), and only after
validation succeed call SaveRoutePlanAsync and then persist RouteStop entries
via SaveRouteStopAsync; alternatively wrap SaveRoutePlanAsync and the subsequent
SaveRouteStopAsync calls in a transactional scope (or use a Service/Repository
transaction) so that failures roll back; reference PendingStopsJson,
PendingStopDto, RouteStop, SaveRoutePlanAsync, and SaveRouteStopAsync when
making changes.
Comment on lines
+258
to
+267
| <div class="form-group"> | ||
| <label class="col-sm-3 control-label">Select Contact</label> | ||
| <div class="col-sm-9"> | ||
| <select id="stopContactPicker" class="form-control"> | ||
| <option value="">-- Type manually or select existing --</option> | ||
| </select> | ||
| <input type="hidden" id="stopContactId" /> | ||
| <span class="help-block" style="font-size:11px;">Selecting a contact auto-fills name and number below.</span> | ||
| </div> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Clear hidden contact ID when the picker is reset.
With the new hidden field at Line [264], clearing the dropdown can still keep the previous ID in #stopContactId (see Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.edit.js, Lines 111-116). That can save a stop against the wrong contact.
Proposed fix (JS)
$('#stopContactPicker').on('change', function () {
var id = $(this).val();
if (!id) {
+ $('#stopContactId').val('');
$('#stopContactName').val('');
$('#stopContactNumber').val('');
return;
}
$('#stopContactId').val(id);
var contact = (typeof availableContacts !== 'undefined') ? availableContacts.find(function (c) { return c.id === id; }) : null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Routes/Edit.cshtml` around lines 258 - 267,
The stop contact dropdown (`#stopContactPicker`) clears visually but leaves the
hidden contact id (`#stopContactId`) populated; update the change handler for the
stop contact picker in the routes edit JS (the function that handles the stop
contact selection in resgrid.routes.edit.js) so that when the selected value is
empty it explicitly clears/reset the hidden input `#stopContactId` (and any
related name/number fields) to avoid saving the previous contact id with a
cleared picker.
Comment on lines
+179
to
+181
| <select id="linkedCallId" class="form-control"> | ||
| <option value="">-- Loading calls... --</option> | ||
| </select> |
Contributor
There was a problem hiding this comment.
Fix input ID mismatch with the route-new JavaScript selectors.
resgrid.routes.new.js (Web/Resgrid.Web/wwwroot/js/app/internal/routes/resgrid.routes.new.js, Lines 270-320) reads #stopCallId, #stopLatitude, and #stopLongitude, but this view defines #linkedCallId, #stopLat, and #stopLng. That causes missing/invalid values during stop serialization.
🛠️ Suggested fix (align IDs with JS)
-<select id="linkedCallId" class="form-control">
+<select id="stopCallId" class="form-control">
@@
-<input type="text" id="stopLat" class="form-control" placeholder="Latitude" readonly />
+<input type="text" id="stopLatitude" class="form-control" placeholder="Latitude" readonly />
@@
-<input type="text" id="stopLng" class="form-control" placeholder="Longitude" readonly />
+<input type="text" id="stopLongitude" class="form-control" placeholder="Longitude" readonly />Also applies to: 225-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Routes/New.cshtml` around lines 179 - 181,
Summary: The view's input IDs don't match the JavaScript selectors, causing
missing/invalid stop data; update the IDs to match the JS. Change the
select/input IDs currently named linkedCallId, stopLat, and stopLng to
stopCallId, stopLatitude, and stopLongitude (these are referenced by
resgrid.routes.new.js via selectors "#stopCallId", "#stopLatitude",
"#stopLongitude"); apply the same renaming to the other occurrence around lines
225-229 so both places use the JS-expected IDs.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs (1)
310-313: Consider renaming duplicate variables for clarity.The variable names
dept2anddeptTimeZone2are unclear. Since this is the same pattern used in theNewaction, consider extracting a helper method or using more descriptive names likedepartmentanddepartmentTimeZone.♻️ Suggested naming improvement
-var dept2 = await _departmentsService.GetDepartmentByIdAsync(DepartmentId); -var deptTimeZone2 = !string.IsNullOrWhiteSpace(dept2?.TimeZone) - ? DateTimeHelpers.WindowsToIana(dept2.TimeZone) +var department = await _departmentsService.GetDepartmentByIdAsync(DepartmentId); +var departmentTimeZone = !string.IsNullOrWhiteSpace(department?.TimeZone) + ? DateTimeHelpers.WindowsToIana(department.TimeZone) : "UTC";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs` around lines 310 - 313, The variables dept2 and deptTimeZone2 in RoutesController are ambiguous; rename them to descriptive names (e.g., department and departmentTimeZone) or extract a small helper (e.g., GetDepartmentTimeZoneAsync that calls _departmentsService.GetDepartmentByIdAsync and DateTimeHelpers.WindowsToIana) and use that from this action and the New action to remove duplication and improve clarity; update all references in the action accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/RoutesController.cs`:
- Around line 310-313: The variables dept2 and deptTimeZone2 in RoutesController
are ambiguous; rename them to descriptive names (e.g., department and
departmentTimeZone) or extract a small helper (e.g., GetDepartmentTimeZoneAsync
that calls _departmentsService.GetDepartmentByIdAsync and
DateTimeHelpers.WindowsToIana) and use that from this action and the New action
to remove duplication and improve clarity; update all references in the action
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34ccb19e-4f36-4d2b-9e06-d47b51e990e9
⛔ Files ignored due to path filters (2)
Core/Resgrid.Localization/Areas/User/Contacts/Contacts.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Routes/Routes.en.resxis excluded by!**/*.resx
📒 Files selected for processing (5)
Web/Resgrid.Web/Areas/User/Controllers/ContactsController.csWeb/Resgrid.Web/Areas/User/Controllers/RoutesController.csWeb/Resgrid.Web/Areas/User/Views/Contacts/View.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/Edit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Routes/New.cshtml
🚧 Files skipped from review as they are similar to previous changes (1)
- Web/Resgrid.Web/Areas/User/Views/Routes/New.cshtml
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor