Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a department-level setting to exclude specific weather alert event titles from notifications and introduces a new UseCalendarSync permission enforced across calendar export endpoints, UI, and related controllers. Changes
Sequence Diagram(s)sequenceDiagram
participant WeatherAlertService as WeatherAlertService
participant SettingRepo as SettingRepository
participant AlertRepo as AlertRepository
participant MessageQueue as MessageQueue
WeatherAlertService->>SettingRepo: GetSettingByTypeAsync(WeatherAlertExcludedEvents)
SettingRepo-->>WeatherAlertService: excluded events string
WeatherAlertService->>WeatherAlertService: Parse into HashSet (case-insensitive)
loop For each pending alert
WeatherAlertService->>WeatherAlertService: if alert.Event in excluded set?
alt excluded
WeatherAlertService->>AlertRepo: Update(alert.NotificationSent=true, LastUpdatedUtc)
AlertRepo-->>WeatherAlertService: updated
WeatherAlertService->>WeatherAlertService: continue (skip notifications)
else not excluded
WeatherAlertService->>MessageQueue: Create/schedule recipient messages
end
end
sequenceDiagram
participant Client as Client
participant CalendarCtrl as CalendarExportController
participant PermRepo as PermissionRepository
participant DeptGroups as DepartmentGroupsService
participant PermSvc as PermissionsService
Client->>CalendarCtrl: GET /CalendarFeed/{token}
CalendarCtrl->>PermRepo: Get UseCalendarSync permission for department
PermRepo-->>CalendarCtrl: permission (or null)
alt permission found
CalendarCtrl->>DeptGroups: GetGroupForUserAsync(userId)
DeptGroups-->>CalendarCtrl: group + admin status
CalendarCtrl->>PermSvc: IsUserAllowed(permission, adminContext, roles)
PermSvc-->>CalendarCtrl: allowed? (true/false)
alt allowed
CalendarCtrl-->>Client: 200 iCal feed
else not allowed
CalendarCtrl-->>Client: 401 Unauthorized
end
else no permission
CalendarCtrl-->>Client: 200 iCal feed (legacy path)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs (1)
415-416: Consider returningExcludedEventsas an empty string when unset.This avoids null/undefined branching on clients and aligns with the save-path normalization.
♻️ Suggested change
- if (excludedEventsSetting != null && !string.IsNullOrWhiteSpace(excludedEventsSetting.Setting)) - settings.ExcludedEvents = excludedEventsSetting.Setting; + settings.ExcludedEvents = excludedEventsSetting?.Setting ?? "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs` around lines 415 - 416, The current assignment leaves settings.ExcludedEvents null when excludedEventsSetting is null, causing clients to handle nulls; in the WeatherAlertsController locate the block where excludedEventsSetting is read and ensure settings.ExcludedEvents is set to string.Empty when excludedEventsSetting is null or its Setting is null/whitespace (i.e., normalize using excludedEventsSetting?.Setting ?? string.Empty or equivalent), so clients always receive an empty string instead of null.Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml (1)
339-341: Always set#settingExcludedEventsduring settings load.Conditionally setting only truthy values can leave stale text in the textarea on reload paths.
♻️ Suggested change
- if (data.Data.ExcludedEvents) { - $('#settingExcludedEvents').val(data.Data.ExcludedEvents); - } + $('#settingExcludedEvents').val(data.Data.ExcludedEvents || '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml` around lines 339 - 341, The current conditional only sets $('#settingExcludedEvents') when data.Data.ExcludedEvents is truthy, which leaves stale content; change the load logic in Settings.cshtml so the textarea is always updated by calling $('#settingExcludedEvents').val(...) using data.Data.ExcludedEvents (or an empty string when it’s null/undefined) instead of keeping the if (data.Data.ExcludedEvents) branch.Web/Resgrid.Web/Areas/User/Controllers/CommunicationTestController.cs (1)
25-39: Extract the department/group-admin resolution into one helper.
Index()andReport()now repeat the same async group lookup + role resolution, and the controller had to grow two more dependencies to support it. Pulling that into a dedicated helper/service will keep the rule consistent and make these actions easier to test.As per coding guidelines, "Design for testability; avoid hidden dependencies inside methods and prefer explicit, pure functions" and "Minimize constructor injection; keep the number of injected dependencies small".
Also applies to: 45-55, 306-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/CommunicationTestController.cs` around lines 25 - 39, Extract the repeated async department/group lookup and role resolution logic used in Index() and Report() into a single helper/service (e.g., IGroupAdminResolver or DepartmentGroupResolver) so the controller no longer directly performs the lookup using IDepartmentGroupsService and IDepartmentsService; implement a method like ResolveAdminForUserAsync(userId) that returns the resolved department/group and admin-role info, update Index() and Report() to call that method, inject the new resolver (IGroupAdminResolver) into the CommunicationTestController constructor instead of _departmentGroupsService/_departmentsService, and move all duplicated logic (the async group lookup + role resolution) into that new resolver to keep the controller thin and make unit testing easier while removing the extra hidden dependencies.Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs (1)
40-54: Encapsulate calendar-sync authorization behind a dedicated collaborator.This controller now owns permission lookup, group lookup, role lookup, and the authorization decision in multiple actions. Pulling that into a small
ICalendarSyncAuthorizationServicewould remove duplicated policy code and keep the controller from accumulating more dependencies.As per coding guidelines, "Minimize constructor injection; keep the number of injected dependencies small" and "Prefer composition with interfaces to extend behavior".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs` around lines 40 - 54, Controller contains repeated permission/group/role/authorization logic; extract that into a new ICalendarSyncAuthorizationService that exposes a single method (e.g., AuthorizeCalendarSyncAsync or CanSyncCalendarAsync) which encapsulates calls to IPermissionsService, IDepartmentGroupsService, IPersonnelRolesService and IAuthorizationService. Implement the service using the existing services, register it in DI, then update CalendarController to inject ICalendarSyncAuthorizationService (remove the now-encapsulated dependencies: _permissionsService, _departmentGroupsService, _personnelRolesService, _authorizationService) and replace the in-action authorization/group/role checks with calls to the new service (targeting the calendar sync actions inside CalendarController). Ensure existing behavior and error responses are preserved by delegating decision and context info from the new service back to the controller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Core/Resgrid.Services/WeatherAlertService.cs`:
- Around line 368-369: The exclusion check can miss matches due to surrounding
whitespace on alert.Event; in the method using excludedEvents and alert.Event
(in WeatherAlertService), trim alert.Event before checking membership so you
compare against the already-trimmed excludedEvents (i.e., replace uses of
alert.Event in the contains check with alert.Event?.Trim() or a local var like
var eventName = alert.Event?.Trim(); and use that in the if condition and any
downstream logic).
In `@Web/Resgrid.Web.Services/Controllers/v4/CalendarExportController.cs`:
- Around line 147-158: Extract the calendar-sync authorization into a reusable
helper (e.g. EnsureUserHasCalendarSyncPermission or similar) that encapsulates
the current logic: call
_permissionsService.GetPermissionByDepartmentTypeAsync(validated.Value.DepartmentId,
PermissionTypes.UseCalendarSync), fetch department via
_departmentsService.GetDepartmentByIdAsync, check
dept.IsUserAnAdmin(validated.Value.UserId), fetch group via
_departmentGroupsService.GetGroupForUserAsync and check grp.IsUserGroupAdmin,
fetch roles via _personnelRolesService.GetRolesForUserAsync, then call
_permissionsService.IsUserAllowed(...) and return Unauthorized() when false;
replace the inline block in CalendarExportController (the feed consumption path)
with this helper and call the same helper from the token issuance/rotation
endpoints (the methods that currently create/rotate tokens) so token creation is
blocked for users without UseCalendarSync.
In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs`:
- Around line 366-377: You persist input.ExcludedEvents with a null-coalescing
fallback but return the original possibly-null value; fix by normalizing once
(e.g., var excluded = input.ExcludedEvents ?? "") and use that variable when
calling _departmentSettingsService.SaveOrUpdateSettingAsync(DepartmentId,
excluded, DepartmentSettingTypes.WeatherAlertExcludedEvents) and when assigning
result.Data.ExcludedEvents so the returned value matches what was persisted.
In `@Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs`:
- Around line 952-964: RegenerateAllCalendarSyncTokens currently iterates
members and calls _calendarService.RegenerateCalendarSyncAsync per-profile,
which doesn't reapply the UseCalendarSync permission rule and performs
per-profile saves leading to partial rotation on failure; refactor by creating a
permission-aware bulk method on CalendarService (e.g.,
RegenerateCalendarSyncForDepartment or RegenerateCalendarSyncBulk) that takes
DepartmentId and applies the same UseCalendarSync check used elsewhere before
generating tokens, then performs the updates in a single batched operation (or
transaction) instead of calling RegenerateCalendarSyncAsync per user; update
RegenerateAllCalendarSyncTokens to call this new bulk method and remove the
per-member loop that calls _userProfileService.GetProfileByUserIdAsync and
_calendarService.RegenerateCalendarSyncAsync.
In `@Web/Resgrid.Web/Areas/User/Controllers/CommunicationTestController.cs`:
- Around line 45-58: Index() currently lets group admins in but still loads
department-wide test definitions and recent runs; restrict those data loads when
isGroupAdmin is true by scoping to the group only. In
CommunicationTestController.Index(), after you fetch "group" (via
_departmentGroupsService.GetGroupForUserAsync) and set isGroupAdmin, branch the
subsequent data population logic (the code that fills model.TestDefinitions /
model.RecentRuns or calls the services that return department tests/runs) so
that if isGroupAdmin you either call a group-scoped API (e.g.,
GetTestsForGroupAsync / GetRecentRunsForGroupAsync) or filter the department
results by group.Id / group membership before assigning to the view model; keep
the isDeptAdmin path unchanged and preserve model.IsDepartmentAdmin assignment.
Ensure all places that populate the collections (the block around the current
model population and the subsequent similar block referenced in lines 60-67)
apply this same scoping.
- Around line 337-349: The code in CommunicationTestController iterates results
from _departmentGroupsService.GetAllMembersForGroupAsync(...) (groupMembers and
childMembers) without null checks; update the logic to guard both groupMembers
and childMembers before foreaching them: after calling
GetAllMembersForGroupAsync(userGroup.DepartmentGroupId) check for null (or use
an empty enumerable) before iterating and only add m.UserId when the collection
is non-null, and do the same inside the childGroups loop for
GetAllMembersForGroupAsync(childGroup.DepartmentGroupId); keep using the
existing variables (groupMembers, childGroups, childGroup, childMembers,
allowedUserIds, userGroup) so the change is limited to adding null checks or
defaulting to Enumerable.Empty<T>().
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/Index.cshtml`:
- Around line 197-205: The hardcoded admin help text, button label and confirm
prompt in the Calendar view should be moved to resources and use localization
helpers; replace the literal strings inside the form that posts to
CalendarController.RegenerateAllCalendarSyncTokens (the form block guarded by
ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) with calls to the localized
resource entries (e.g., `@Localizer`["Calendar.RegenerateAll.Help"],
`@Localizer`["Calendar.RegenerateAll.Button"], and
`@Localizer`["Calendar.RegenerateAll.Confirm"]) and add corresponding keys to the
resource file(s) for the page so the help copy, button text and JS confirm
message are localized.
In `@Web/Resgrid.Web/Areas/User/Views/Security/Index.cshtml`:
- Around line 316-324: Replace the hardcoded English strings in the
calendar-sync permissions row with localized resource lookups: change "Use
Calendar Sync" to use localizer (e.g., localizer["Security_UseCalendarSync"])
and replace the description text ("Controls who can activate...") with a
localized key (e.g., localizer["Security_UseCalendarSync_Description"]); keep
existing localized tokens like PermissionNA and PermissionNoRoles as-is and add
the new keys to the resource files so translations are available; update the
view element references (calSyncNoRolesSpan, calSyncRolesDiv) only if their
content also needs localization via localizer.
In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml`:
- Around line 90-95: Replace the hardcoded English strings in the WeatherAlerts
settings block with localization resource lookups: change the label text, the
textarea placeholder, and the help-block content to use resource keys (e.g.,
WeatherAlerts_ExcludedEvents_Label, WeatherAlerts_ExcludedEvents_Placeholder,
WeatherAlerts_ExcludedEvents_Help) instead of literal strings; update the Razor
markup referencing the textarea id "settingExcludedEvents" to use the resource
expressions (e.g., `@Resources.WeatherAlerts_ExcludedEvents_Label`,
`@Resources.WeatherAlerts_ExcludedEvents_Placeholder`,
`@Resources.WeatherAlerts_ExcludedEvents_Help` or the project's localization
helper), and add those three keys to the appropriate resource file(s) for all
supported locales.
---
Nitpick comments:
In `@Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs`:
- Around line 415-416: The current assignment leaves settings.ExcludedEvents
null when excludedEventsSetting is null, causing clients to handle nulls; in the
WeatherAlertsController locate the block where excludedEventsSetting is read and
ensure settings.ExcludedEvents is set to string.Empty when excludedEventsSetting
is null or its Setting is null/whitespace (i.e., normalize using
excludedEventsSetting?.Setting ?? string.Empty or equivalent), so clients always
receive an empty string instead of null.
In `@Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs`:
- Around line 40-54: Controller contains repeated
permission/group/role/authorization logic; extract that into a new
ICalendarSyncAuthorizationService that exposes a single method (e.g.,
AuthorizeCalendarSyncAsync or CanSyncCalendarAsync) which encapsulates calls to
IPermissionsService, IDepartmentGroupsService, IPersonnelRolesService and
IAuthorizationService. Implement the service using the existing services,
register it in DI, then update CalendarController to inject
ICalendarSyncAuthorizationService (remove the now-encapsulated dependencies:
_permissionsService, _departmentGroupsService, _personnelRolesService,
_authorizationService) and replace the in-action authorization/group/role checks
with calls to the new service (targeting the calendar sync actions inside
CalendarController). Ensure existing behavior and error responses are preserved
by delegating decision and context info from the new service back to the
controller.
In `@Web/Resgrid.Web/Areas/User/Controllers/CommunicationTestController.cs`:
- Around line 25-39: Extract the repeated async department/group lookup and role
resolution logic used in Index() and Report() into a single helper/service
(e.g., IGroupAdminResolver or DepartmentGroupResolver) so the controller no
longer directly performs the lookup using IDepartmentGroupsService and
IDepartmentsService; implement a method like ResolveAdminForUserAsync(userId)
that returns the resolved department/group and admin-role info, update Index()
and Report() to call that method, inject the new resolver (IGroupAdminResolver)
into the CommunicationTestController constructor instead of
_departmentGroupsService/_departmentsService, and move all duplicated logic (the
async group lookup + role resolution) into that new resolver to keep the
controller thin and make unit testing easier while removing the extra hidden
dependencies.
In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml`:
- Around line 339-341: The current conditional only sets
$('#settingExcludedEvents') when data.Data.ExcludedEvents is truthy, which
leaves stale content; change the load logic in Settings.cshtml so the textarea
is always updated by calling $('#settingExcludedEvents').val(...) using
data.Data.ExcludedEvents (or an empty string when it’s null/undefined) instead
of keeping the if (data.Data.ExcludedEvents) branch.
🪄 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: d476b520-d820-4b80-a091-5e68471e3cb1
📒 Files selected for processing (18)
Core/Resgrid.Model/DepartmentSettingTypes.csCore/Resgrid.Model/PermissionTypes.csCore/Resgrid.Services/WeatherAlertService.csWeb/Resgrid.Web.Services/Controllers/v4/CalendarExportController.csWeb/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/SaveWeatherAlertSettingsInput.csWeb/Resgrid.Web.Services/Models/v4/WeatherAlerts/WeatherAlertSettingsData.csWeb/Resgrid.Web/Areas/User/Controllers/CalendarController.csWeb/Resgrid.Web/Areas/User/Controllers/CommunicationTestController.csWeb/Resgrid.Web/Areas/User/Controllers/SecurityController.csWeb/Resgrid.Web/Areas/User/Models/Calendar/IndexView.csWeb/Resgrid.Web/Areas/User/Models/CommunicationTests/CommunicationTestIndexView.csWeb/Resgrid.Web/Areas/User/Models/Security/PermissionsView.csWeb/Resgrid.Web/Areas/User/Views/Calendar/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/CommunicationTest/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Security/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtmlWeb/Resgrid.Web/wwwroot/js/app/internal/security/resgrid.security.permissions.js
| if (excludedEvents != null && !string.IsNullOrWhiteSpace(alert.Event) && excludedEvents.Contains(alert.Event)) | ||
| { |
There was a problem hiding this comment.
Trim alert.Event before exclusion lookup.
The excluded list is trimmed, but alert.Event is not. Whitespace variance can cause false non-matches.
✅ Suggested change
- if (excludedEvents != null && !string.IsNullOrWhiteSpace(alert.Event) && excludedEvents.Contains(alert.Event))
+ if (excludedEvents != null && !string.IsNullOrWhiteSpace(alert.Event) && excludedEvents.Contains(alert.Event.Trim()))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Core/Resgrid.Services/WeatherAlertService.cs` around lines 368 - 369, The
exclusion check can miss matches due to surrounding whitespace on alert.Event;
in the method using excludedEvents and alert.Event (in WeatherAlertService),
trim alert.Event before checking membership so you compare against the
already-trimmed excludedEvents (i.e., replace uses of alert.Event in the
contains check with alert.Event?.Trim() or a local var like var eventName =
alert.Event?.Trim(); and use that in the if condition and any downstream logic).
Web/Resgrid.Web.Services/Controllers/v4/CalendarExportController.cs
Outdated
Show resolved
Hide resolved
Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs
Outdated
Show resolved
Hide resolved
| public async Task<IActionResult> RegenerateAllCalendarSyncTokens(CancellationToken cancellationToken) | ||
| { | ||
| var members = await _departmentsService.GetAllMembersForDepartmentAsync(DepartmentId); | ||
| if (members != null) | ||
| { | ||
| foreach (var member in members) | ||
| { | ||
| var profile = await _userProfileService.GetProfileByUserIdAsync(member.UserId); | ||
| if (profile != null && !string.IsNullOrWhiteSpace(profile.CalendarSyncToken)) | ||
| { | ||
| await _calendarService.RegenerateCalendarSyncAsync(DepartmentId, member.UserId, cancellationToken); | ||
| } | ||
| } |
There was a problem hiding this comment.
Move department-wide token rotation into a permission-aware bulk service.
Unlike Lines 915-940, this loop regenerates every stored token without reapplying UseCalendarSync per member. It also writes one profile at a time; Core/Resgrid.Services/CalendarService.cs:680-698 shows each regeneration overwrites and saves immediately, so any mid-loop failure leaves the department in a mixed old/new-token state.
Suggested direction
public async Task<IActionResult> RegenerateAllCalendarSyncTokens(CancellationToken cancellationToken)
{
- var members = await _departmentsService.GetAllMembersForDepartmentAsync(DepartmentId);
- if (members != null)
- {
- foreach (var member in members)
- {
- var profile = await _userProfileService.GetProfileByUserIdAsync(member.UserId);
- if (profile != null && !string.IsNullOrWhiteSpace(profile.CalendarSyncToken))
- {
- await _calendarService.RegenerateCalendarSyncAsync(DepartmentId, member.UserId, cancellationToken);
- }
- }
- }
+ var result = await _calendarService.RegenerateDepartmentCalendarSyncTokensAsync(
+ DepartmentId,
+ cancellationToken);
+
+ if (!result.Success)
+ TempData["ErrorMessage"] =
+ $"Rotated {result.RotatedCount} of {result.TotalEligibleCount} eligible calendar sync tokens.";
return RedirectToAction("Index");
}The bulk service should apply the same UseCalendarSync rule used in Lines 915-940 before rotating each member's token.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs` around lines
952 - 964, RegenerateAllCalendarSyncTokens currently iterates members and calls
_calendarService.RegenerateCalendarSyncAsync per-profile, which doesn't reapply
the UseCalendarSync permission rule and performs per-profile saves leading to
partial rotation on failure; refactor by creating a permission-aware bulk method
on CalendarService (e.g., RegenerateCalendarSyncForDepartment or
RegenerateCalendarSyncBulk) that takes DepartmentId and applies the same
UseCalendarSync check used elsewhere before generating tokens, then performs the
updates in a single batched operation (or transaction) instead of calling
RegenerateCalendarSyncAsync per user; update RegenerateAllCalendarSyncTokens to
call this new bulk method and remove the per-member loop that calls
_userProfileService.GetProfileByUserIdAsync and
_calendarService.RegenerateCalendarSyncAsync.
| bool isDeptAdmin = ClaimsAuthorizationHelper.IsUserDepartmentAdmin(); | ||
| bool isGroupAdmin = false; | ||
|
|
||
| if (!isDeptAdmin) | ||
| { | ||
| var group = await _departmentGroupsService.GetGroupForUserAsync(UserId, DepartmentId); | ||
| isGroupAdmin = group != null && group.IsUserGroupAdmin(UserId); | ||
| } | ||
|
|
||
| if (!isDeptAdmin && !isGroupAdmin) | ||
| return Unauthorized(); | ||
|
|
||
| var model = new CommunicationTestIndexView(); | ||
| model.IsDepartmentAdmin = isDeptAdmin; |
There was a problem hiding this comment.
Scope the index data before allowing group-admin access.
Once Index() admits group admins, it still loads all department tests and recent runs. Web/Resgrid.Web/Areas/User/Views/CommunicationTest/Index.cshtml then renders those collections into the test-definition table and the recent-runs table, including run codes and aggregate counts, so this change exposes department-wide communication-test metadata to group admins even though the action buttons are hidden.
Also applies to: 60-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/CommunicationTestController.cs` around
lines 45 - 58, Index() currently lets group admins in but still loads
department-wide test definitions and recent runs; restrict those data loads when
isGroupAdmin is true by scoping to the group only. In
CommunicationTestController.Index(), after you fetch "group" (via
_departmentGroupsService.GetGroupForUserAsync) and set isGroupAdmin, branch the
subsequent data population logic (the code that fills model.TestDefinitions /
model.RecentRuns or calls the services that return department tests/runs) so
that if isGroupAdmin you either call a group-scoped API (e.g.,
GetTestsForGroupAsync / GetRecentRunsForGroupAsync) or filter the department
results by group.Id / group membership before assigning to the view model; keep
the isDeptAdmin path unchanged and preserve model.IsDepartmentAdmin assignment.
Ensure all places that populate the collections (the block around the current
model population and the subsequent similar block referenced in lines 60-67)
apply this same scoping.
| var groupMembers = await _departmentGroupsService.GetAllMembersForGroupAsync(userGroup.DepartmentGroupId); | ||
| foreach (var m in groupMembers) | ||
| allowedUserIds.Add(m.UserId); | ||
|
|
||
| // Add members of child groups | ||
| var childGroups = await _departmentGroupsService.GetAllChildDepartmentGroupsAsync(userGroup.DepartmentGroupId); | ||
| if (childGroups != null) | ||
| { | ||
| foreach (var childGroup in childGroups) | ||
| { | ||
| var childMembers = await _departmentGroupsService.GetAllMembersForGroupAsync(childGroup.DepartmentGroupId); | ||
| foreach (var m in childMembers) | ||
| allowedUserIds.Add(m.UserId); |
There was a problem hiding this comment.
Guard the member lookups before iterating.
GetAllMembersForGroupAsync() is enumerated twice without a null check. If either call returns null, the report path throws for group admins. This method already treats childGroups as nullable, so the member lists should be handled the same way.
Suggested null-safe fix
- var groupMembers = await _departmentGroupsService.GetAllMembersForGroupAsync(userGroup.DepartmentGroupId);
+ var groupMembers = await _departmentGroupsService.GetAllMembersForGroupAsync(userGroup.DepartmentGroupId) ?? new List<DepartmentGroupMember>();
foreach (var m in groupMembers)
allowedUserIds.Add(m.UserId);
...
- var childMembers = await _departmentGroupsService.GetAllMembersForGroupAsync(childGroup.DepartmentGroupId);
+ var childMembers = await _departmentGroupsService.GetAllMembersForGroupAsync(childGroup.DepartmentGroupId) ?? new List<DepartmentGroupMember>();
foreach (var m in childMembers)
allowedUserIds.Add(m.UserId);📝 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 groupMembers = await _departmentGroupsService.GetAllMembersForGroupAsync(userGroup.DepartmentGroupId); | |
| foreach (var m in groupMembers) | |
| allowedUserIds.Add(m.UserId); | |
| // Add members of child groups | |
| var childGroups = await _departmentGroupsService.GetAllChildDepartmentGroupsAsync(userGroup.DepartmentGroupId); | |
| if (childGroups != null) | |
| { | |
| foreach (var childGroup in childGroups) | |
| { | |
| var childMembers = await _departmentGroupsService.GetAllMembersForGroupAsync(childGroup.DepartmentGroupId); | |
| foreach (var m in childMembers) | |
| allowedUserIds.Add(m.UserId); | |
| var groupMembers = await _departmentGroupsService.GetAllMembersForGroupAsync(userGroup.DepartmentGroupId) ?? new List<DepartmentGroupMember>(); | |
| foreach (var m in groupMembers) | |
| allowedUserIds.Add(m.UserId); | |
| // Add members of child groups | |
| var childGroups = await _departmentGroupsService.GetAllChildDepartmentGroupsAsync(userGroup.DepartmentGroupId); | |
| if (childGroups != null) | |
| { | |
| foreach (var childGroup in childGroups) | |
| { | |
| var childMembers = await _departmentGroupsService.GetAllMembersForGroupAsync(childGroup.DepartmentGroupId) ?? new List<DepartmentGroupMember>(); | |
| foreach (var m in childMembers) | |
| allowedUserIds.Add(m.UserId); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Controllers/CommunicationTestController.cs` around
lines 337 - 349, The code in CommunicationTestController iterates results from
_departmentGroupsService.GetAllMembersForGroupAsync(...) (groupMembers and
childMembers) without null checks; update the logic to guard both groupMembers
and childMembers before foreaching them: after calling
GetAllMembersForGroupAsync(userGroup.DepartmentGroupId) check for null (or use
an empty enumerable) before iterating and only add m.UserId when the collection
is non-null, and do the same inside the childGroups loop for
GetAllMembersForGroupAsync(childGroup.DepartmentGroupId); keep using the
existing variables (groupMembers, childGroups, childGroup, childMembers,
allowedUserIds, userGroup) so the change is limited to adding null checks or
defaulting to Enumerable.Empty<T>().
| @if (ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) | ||
| { | ||
| <form asp-controller="Calendar" asp-action="RegenerateAllCalendarSyncTokens" asp-route-area="User" method="post" style="margin-top:12px; padding-top:12px; border-top:1px solid #eee;"> | ||
| @Html.AntiForgeryToken() | ||
| <p class="small text-muted">Admin: Regenerate all calendar sync tokens for every user in the department. This will invalidate all existing subscription URLs.</p> | ||
| <button type="submit" class="btn btn-danger btn-sm" | ||
| onclick="return confirm('This will invalidate ALL calendar sync URLs for every user in your department. This cannot be undone. Continue?');"> | ||
| <i class="fa fa-refresh"></i> Regenerate All Department Tokens | ||
| </button> |
There was a problem hiding this comment.
Move the new admin-only calendar-sync copy into resources.
The added help text, button label, and confirm prompt are hardcoded English while the rest of this page is localized.
🌐 Suggested fix
- <p class="small text-muted">Admin: Regenerate all calendar sync tokens for every user in the department. This will invalidate all existing subscription URLs.</p>
+ <p class="small text-muted">@localizer["CalendarSyncRegenerateAllHelp"]</p>
<button type="submit" class="btn btn-danger btn-sm"
- onclick="return confirm('This will invalidate ALL calendar sync URLs for every user in your department. This cannot be undone. Continue?');">
- <i class="fa fa-refresh"></i> Regenerate All Department Tokens
+ onclick="return confirm('@localizer["CalendarSyncRegenerateAllConfirm"]');">
+ <i class="fa fa-refresh"></i> `@localizer`["CalendarSyncRegenerateAllButton"]
</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Calendar/Index.cshtml` around lines 197 -
205, The hardcoded admin help text, button label and confirm prompt in the
Calendar view should be moved to resources and use localization helpers; replace
the literal strings inside the form that posts to
CalendarController.RegenerateAllCalendarSyncTokens (the form block guarded by
ClaimsAuthorizationHelper.IsUserDepartmentAdmin()) with calls to the localized
resource entries (e.g., `@Localizer`["Calendar.RegenerateAll.Help"],
`@Localizer`["Calendar.RegenerateAll.Button"], and
`@Localizer`["Calendar.RegenerateAll.Confirm"]) and add corresponding keys to the
resource file(s) for the page so the help copy, button text and JS confirm
message are localized.
| <tr> | ||
| <td>Use Calendar Sync</td> | ||
| <td style="max-width: 350px">Controls who can activate and use calendar subscription URLs to sync Resgrid calendar events to external calendar applications.</td> | ||
| <td>@Html.DropDownListFor(m => m.UseCalendarSync, Model.UseCalendarSyncPermissions)</td> | ||
| <td>@localizer["PermissionNA"]</td> | ||
| <td> | ||
| <span id="calSyncNoRolesSpan">@localizer["PermissionNoRoles"]</span> | ||
| <div id="calSyncRolesDiv" style="display: none;"><select id="calSyncRoles" name="calSyncRoles"></select></div> | ||
| </td> |
There was a problem hiding this comment.
Localize the new calendar-sync row text.
This row is the only part of the permissions table here that ships hardcoded English copy, so translated deployments will get a partially untranslated screen.
🌐 Suggested fix
- <td>Use Calendar Sync</td>
- <td style="max-width: 350px">Controls who can activate and use calendar subscription URLs to sync Resgrid calendar events to external calendar applications.</td>
+ <td>@localizer["PermUseCalendarSyncLabel"]</td>
+ <td style="max-width: 350px">@localizer["PermUseCalendarSyncNote"]</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/Security/Index.cshtml` around lines 316 -
324, Replace the hardcoded English strings in the calendar-sync permissions row
with localized resource lookups: change "Use Calendar Sync" to use localizer
(e.g., localizer["Security_UseCalendarSync"]) and replace the description text
("Controls who can activate...") with a localized key (e.g.,
localizer["Security_UseCalendarSync_Description"]); keep existing localized
tokens like PermissionNA and PermissionNoRoles as-is and add the new keys to the
resource files so translations are available; update the view element references
(calSyncNoRolesSpan, calSyncRolesDiv) only if their content also needs
localization via localizer.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml (1)
367-368: Normalize excluded-events input before sending.At Line 368, trim the textarea value so whitespace-only input is not stored as a non-empty setting.
Suggested fix
+ var excludedEvents = ($('#settingExcludedEvents').val() || '').trim(); + var data = { WeatherAlertsEnabled: $('#settingEnabled').is(':checked'), MinimumSeverity: parseInt($('#settingMinSeverity').val()), AutoMessageSeverity: parseInt($('#settingAutoMsgSeverity').val()), CallIntegrationEnabled: $('#settingCallIntegration').is(':checked'), AutoMessageSchedule: schedule, - ExcludedEvents: $('#settingExcludedEvents').val() || '' + ExcludedEvents: excludedEvents };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml` around lines 367 - 368, The ExcludedEvents payload is using $('#settingExcludedEvents').val() directly which can store whitespace-only input; update the construction for ExcludedEvents to read the value from the element with id settingExcludedEvents, coerce to string, call .trim(), and fall back to an empty string if the trimmed result is empty so ExcludedEvents is never a whitespace-only value (refer to the ExcludedEvents property and the settingExcludedEvents input element).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Web/Resgrid.Web/Areas/User/Views/Department/Api.cshtml`:
- Around line 67-78: Replace hardcoded English strings in the Calendar Sync
Tokens block with localized resources using the view's IStringLocalizer (e.g.,
the localizer instance used in this view such as L or Localizer): localize the
<h5> heading, the paragraph text, the button label for the submit button that
triggers Calendar.RegenerateAllCalendarSyncTokens, and the confirm dialog string
used in the onclick handler; ensure the confirm uses the localized string safely
(injecting via the localizer into the onclick or using a JS-escaped/localized
data-attribute) so translations are used for the heading, body, button and
confirmation prompt.
- Around line 62-84: The view renders the Calendar Sync Tokens block
unconditionally while the backend action RegenerateAllCalendarSyncTokens is
protected by the Department_Update policy but there is an existing
UseCalendarSync permission used elsewhere (PermissionsView), causing a
permissions inconsistency; add a boolean CanUseCalendarSync to
DepartmentSettingsModel, populate it in the controller so the view can check it
(like ActiveCallRssKey is used for the RSS section), and wrap the Calendar Sync
Tokens form/block in a conditional that only renders when
Model.CanUseCalendarSync is true—alternatively, if Department_Update is the
intended gate, update docs/comments to state that explicitly (but the preferred
fix is adding and checking CanUseCalendarSync in the view).
In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml`:
- Around line 339-341: The current guard in the loadSettings() response handling
only sets $('#settingExcludedEvents') when data.Data.ExcludedEvents is truthy,
leaving stale text when the backend returns an empty/falsey value; change the
logic in the Settings.cshtml handler that references data.Data.ExcludedEvents to
always call $('#settingExcludedEvents').val(...) and pass a safe fallback (e.g.,
empty string) when data.Data.ExcludedEvents is undefined/null/empty so the
textarea is cleared on subsequent loadSettings() calls.
---
Nitpick comments:
In `@Web/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml`:
- Around line 367-368: The ExcludedEvents payload is using
$('#settingExcludedEvents').val() directly which can store whitespace-only
input; update the construction for ExcludedEvents to read the value from the
element with id settingExcludedEvents, coerce to string, call .trim(), and fall
back to an empty string if the trimmed result is empty so ExcludedEvents is
never a whitespace-only value (refer to the ExcludedEvents property and the
settingExcludedEvents input element).
🪄 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: 3ab5177d-8ac0-4c47-a4c4-7a6e43ee18d0
⛔ Files ignored due to path filters (9)
Core/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/WeatherAlerts/WeatherAlerts.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (6)
Web/Resgrid.Web.Services/Controllers/v4/CalendarExportController.csWeb/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.csWeb/Resgrid.Web/Areas/User/Controllers/CalendarController.csWeb/Resgrid.Web/Areas/User/Views/Calendar/Index.cshtmlWeb/Resgrid.Web/Areas/User/Views/Department/Api.cshtmlWeb/Resgrid.Web/Areas/User/Views/WeatherAlerts/Settings.cshtml
✅ Files skipped from review due to trivial changes (2)
- Web/Resgrid.Web/Areas/User/Views/Calendar/Index.cshtml
- Web/Resgrid.Web.Services/Controllers/v4/WeatherAlertsController.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Web/Resgrid.Web.Services/Controllers/v4/CalendarExportController.cs
- Web/Resgrid.Web/Areas/User/Controllers/CalendarController.cs
| <div class="row"> | ||
| <div class="col-xs-12"> | ||
| <div class="wrapper wrapper-content"> | ||
| <div class="ibox float-e-margins"> | ||
| <div class="ibox-content"> | ||
| <h5>Calendar Sync Tokens</h5> | ||
| <p> | ||
| Calendar sync allows users to subscribe to the department calendar from external calendar applications (Google Calendar, Outlook, Apple Calendar). | ||
| Use the button below to regenerate all calendar sync tokens for every user in the department. This will invalidate all existing subscription URLs | ||
| and users will need to re-subscribe. | ||
| </p> | ||
| <form asp-controller="Calendar" asp-action="RegenerateAllCalendarSyncTokens" asp-route-area="User" method="post"> | ||
| @Html.AntiForgeryToken() | ||
| <button type="submit" class="btn btn-danger" | ||
| onclick="return confirm('This will invalidate ALL calendar sync URLs for every user in your department. Users will need to re-subscribe. Continue?');"> | ||
| <i class="fa fa-refresh"></i> Regenerate All Calendar Sync Tokens | ||
| </button> | ||
| </form> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether calendar-sync permission/feature flags are enforced consistently
rg -n -C3 'UseCalendarSync|CanUseCalendarSync|RegenerateAllCalendarSyncTokens|Department_Update' Web/Resgrid.Web/Areas/User
# Check if Department settings model has a calendar-sync capability flag for view gating
rg -n -C3 'class DepartmentSettingsModel|CanUseCalendarSync|UseCalendarSync' --iglob '*DepartmentSettingsModel*.cs'
# Compare with calendar UI gating implementation
rg -n -C3 'CanUseCalendarSync|UseCalendarSync' Web/Resgrid.Web/Areas/User/Views/Calendar/Index.cshtmlRepository: Resgrid/Core
Length of output: 50368
🏁 Script executed:
# Find the Api controller action in DepartmentController
rg -n -A20 'public async Task<IActionResult> Api\(' Web/Resgrid.Web/Areas/User/Controllers/DepartmentController.cs
# Check the DepartmentSettingsModel structure for permission properties
rg -n 'class DepartmentSettingsModel' --iglob '*DepartmentSettingsModel*' -A30Repository: Resgrid/Core
Length of output: 4276
🏁 Script executed:
# Get full DepartmentSettingsModel structure
wc -l Web/Resgrid.Web/Areas/User/Models/DepartmentSettingsModel.cs
cat -n Web/Resgrid.Web/Areas/User/Models/DepartmentSettingsModel.cs
# Check Api.cshtml for conditional rendering of the calendar sync block
cat -n Web/Resgrid.Web/Areas/User/Views/Department/Api.cshtmlRepository: Resgrid/Core
Length of output: 7133
🏁 Script executed:
# Find RegenerateAllCalendarSyncTokens action
rg -n -A15 'RegenerateAllCalendarSyncTokens' Web/Resgrid.Web/Areas/User/Controllers/Repository: Resgrid/Core
Length of output: 1713
🏁 Script executed:
# Get context before the method
rg -n -B5 'public async Task<IActionResult> RegenerateAllCalendarSyncTokens' Web/Resgrid.Web/Areas/User/Controllers/CalendarController.csRepository: Resgrid/Core
Length of output: 412
Gate the calendar sync regeneration UI behind appropriate permission checks or clarify the intended permission model.
The calendar sync tokens section (lines 62-84) is rendered unconditionally in the view. While the backend RegenerateAllCalendarSyncTokens action is protected by [Authorize(Policy = ResgridResources.Department_Update)], the view doesn't check the UseCalendarSync permission that exists in PermissionsView.cs. This creates an inconsistency: a UseCalendarSync permission gate exists but is not enforced in the Department API settings view, and users with Department_Update will see this button regardless of their calendar sync capability.
Either:
- Add
CanUseCalendarSyncproperty toDepartmentSettingsModeland conditionally render this block (similar to how the RSS section at line 34 checksActiveCallRssKey), or - Document that
Department_Updateis the intended permission gate for this feature
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Department/Api.cshtml` around lines 62 - 84,
The view renders the Calendar Sync Tokens block unconditionally while the
backend action RegenerateAllCalendarSyncTokens is protected by the
Department_Update policy but there is an existing UseCalendarSync permission
used elsewhere (PermissionsView), causing a permissions inconsistency; add a
boolean CanUseCalendarSync to DepartmentSettingsModel, populate it in the
controller so the view can check it (like ActiveCallRssKey is used for the RSS
section), and wrap the Calendar Sync Tokens form/block in a conditional that
only renders when Model.CanUseCalendarSync is true—alternatively, if
Department_Update is the intended gate, update docs/comments to state that
explicitly (but the preferred fix is adding and checking CanUseCalendarSync in
the view).
| <h5>Calendar Sync Tokens</h5> | ||
| <p> | ||
| Calendar sync allows users to subscribe to the department calendar from external calendar applications (Google Calendar, Outlook, Apple Calendar). | ||
| Use the button below to regenerate all calendar sync tokens for every user in the department. This will invalidate all existing subscription URLs | ||
| and users will need to re-subscribe. | ||
| </p> | ||
| <form asp-controller="Calendar" asp-action="RegenerateAllCalendarSyncTokens" asp-route-area="User" method="post"> | ||
| @Html.AntiForgeryToken() | ||
| <button type="submit" class="btn btn-danger" | ||
| onclick="return confirm('This will invalidate ALL calendar sync URLs for every user in your department. Users will need to re-subscribe. Continue?');"> | ||
| <i class="fa fa-refresh"></i> Regenerate All Calendar Sync Tokens | ||
| </button> |
There was a problem hiding this comment.
Localize the new Calendar Sync section text.
The new heading/body/button/confirm text is hardcoded English, while this view already uses IStringLocalizer. This will bypass translations.
💡 Suggested localized update
- <h5>Calendar Sync Tokens</h5>
+ <h5>@localizer["CalendarSyncTokensHeader"]</h5>
<p>
- Calendar sync allows users to subscribe to the department calendar from external calendar applications (Google Calendar, Outlook, Apple Calendar).
- Use the button below to regenerate all calendar sync tokens for every user in the department. This will invalidate all existing subscription URLs
- and users will need to re-subscribe.
+ `@localizer`["CalendarSyncTokensInfo"]
</p>
<form asp-controller="Calendar" asp-action="RegenerateAllCalendarSyncTokens" asp-route-area="User" method="post">
`@Html.AntiForgeryToken`()
<button type="submit" class="btn btn-danger"
- onclick="return confirm('This will invalidate ALL calendar sync URLs for every user in your department. Users will need to re-subscribe. Continue?');">
- <i class="fa fa-refresh"></i> Regenerate All Calendar Sync Tokens
+ onclick="return confirm('@localizer["CalendarSyncTokensConfirm"]');">
+ <i class="fa fa-refresh"></i> `@localizer`["RegenerateAllCalendarSyncTokens"]
</button>
</form>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Web/Resgrid.Web/Areas/User/Views/Department/Api.cshtml` around lines 67 - 78,
Replace hardcoded English strings in the Calendar Sync Tokens block with
localized resources using the view's IStringLocalizer (e.g., the localizer
instance used in this view such as L or Localizer): localize the <h5> heading,
the paragraph text, the button label for the submit button that triggers
Calendar.RegenerateAllCalendarSyncTokens, and the confirm dialog string used in
the onclick handler; ensure the confirm uses the localized string safely
(injecting via the localizer into the onclick or using a JS-escaped/localized
data-attribute) so translations are used for the heading, body, button and
confirmation prompt.
|
Approve |
Summary by CodeRabbit
New Features
UI / UX