Conversation
This comment has been minimized.
This comment has been minimized.
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
📝 WalkthroughWalkthroughThe PR separates department access revocation from full account deactivation, expands deletion cleanup, updates CORS and Redis handling, adds call-scoped chat filtering, and skips orphaned call-pruning settings. ChangesUser deprovisioning
CORS origin matching
Redis connection handling
Chat channel filtering
Call-pruning orphan guard
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PersonnelController
participant DeleteService
participant DistributionListsService
participant ScheduledTasksService
participant IdentityRepository
PersonnelController->>DeleteService: DeleteUserAsync
DeleteService->>DistributionListsService: Remove department memberships
DeleteService->>ScheduledTasksService: Delete department or global tasks
DeleteService->>IdentityRepository: Clear account authentication data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Providers/Resgrid.Providers.Cache/AzureRedisCacheProvider.cs`:
- Around line 52-56: Replace Logging.LogError(ex) with Logging.LogException(ex)
in every RedisConnectionException and TimeoutException handler in
AzureRedisCacheProvider, including the additional listed handlers. Preserve each
handler’s existing fallback return value.
In `@Workers/Resgrid.Workers.Console/Tasks/CallPruneTask.cs`:
- Line 56: Add a LogWarning method to the Resgrid.Framework.Logging facade, then
update CallPruneTask to call that facade method instead of _logger.LogWarning
while preserving the existing orphaned-department message and interpolation.
🪄 Autofix
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 Plus
Run ID: 8f15c6f6-b78f-4f0a-ad29-aca36bf1212c
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Config/CorsHelperTests.csis excluded by!**/Tests/**
📒 Files selected for processing (7)
Core/Resgrid.Config/ApiConfig.csCore/Resgrid.Config/CorsHelper.csProviders/Resgrid.Providers.Cache/AzureRedisCacheProvider.csRepositories/Resgrid.Repositories.DataRepository/DeleteRepository.csWeb/Resgrid.Web.Services/Controllers/v4/ChatController.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xmlWorkers/Resgrid.Workers.Console/Tasks/CallPruneTask.cs
| catch (RedisConnectionException ex) | ||
| { | ||
| // Transient connection drop (idle reset, failover); fallback below handles it. | ||
| Logging.LogError(ex); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use LogException in the new exception handlers.
The RedisConnectionException handlers call Logging.LogError(ex). The new TimeoutException handlers also discard the exception without logging. Use Logging.LogException(ex) in each handler. Preserve the existing fallback return values.
Proposed fix
-Logging.LogError(ex);
+Logging.LogException(ex);
-catch (TimeoutException)
-{
-}
+catch (TimeoutException ex)
+{
+ Logging.LogException(ex);
+}As per coding guidelines, caught exceptions must use Resgrid.Framework.Logging.LogException(...) because it captures caller information.
Also applies to: 74-77, 98-103, 143-147, 165-168, 195-198, 226-231, 264-269, 306-311
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Providers/Resgrid.Providers.Cache/AzureRedisCacheProvider.cs` around lines 52
- 56, Replace Logging.LogError(ex) with Logging.LogException(ex) in every
RedisConnectionException and TimeoutException handler in
AzureRedisCacheProvider, including the additional listed handlers. Preserve each
handler’s existing fallback return value.
Source: Coding guidelines
| { | ||
| // Orphaned pruning row for a deleted department; skip it so we don't | ||
| // null-ref in CallPruneLogic or spam error logs every run. | ||
| _logger.LogWarning($"CallPrune::Skipping orphaned pruning settings for deleted DepartmentId:{item.PruneSettings.DepartmentId}"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 '\bResgrid\.Framework\.Logging\b|\bLogWarning\s*\(' --glob '*.cs'Repository: Resgrid/Core
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='Workers/Resgrid.Workers.Console/Tasks/CallPruneTask.cs'
printf '%s\n' '--- target file ---'
cat -n "$file" | sed -n '1,110p'
printf '%s\n' '--- logging facade references ---'
rg -n -C 2 'class Logging|static class Logging|LogException|LogError|LogInfo|LogDebug|LogWarning|ILogger' --glob '*.cs' . | sed -n '1,240p'
printf '%s\n' '--- logging-related files ---'
git ls-files '*.cs' | rg -i 'logging|logger|log'Repository: Resgrid/Core
Length of output: 34818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Resgrid.Framework.Logging implementation ---'
cat -n Core/Resgrid.Framework/Logging.cs | sed -n '1,240p'
printf '%s\n' '--- worker logging call patterns ---'
rg -n '(_logger|Logging|Framework\.Logging)\.(Log[A-Za-z]+)' Workers --glob '*.cs' | sed -n '1,240p'
printf '%s\n' '--- read-only API check ---'
python3 - <<'PY'
from pathlib import Path
import re
facade = Path("Core/Resgrid.Framework/Logging.cs").read_text(encoding="utf-8-sig")
target = Path("Workers/Resgrid.Workers.Console/Tasks/CallPruneTask.cs").read_text(encoding="utf-8-sig")
methods = sorted(set(re.findall(r'\b(?:public|private|internal|protected)\s+static\s+\w+(?:<[^>]+>)?\s+(Log\w+)\s*\(', facade)))
calls = sorted(set(re.findall(r'\b(?:_logger|Logging|Framework\.Logging)\.(Log\w+)\s*\(', target)))
print("facade static methods:", methods)
print("target logging methods:", calls)
print("target LogWarning facade method exists:", "LogWarning" in methods)
PYRepository: Resgrid/Core
Length of output: 38951
Add a warning method to Resgrid.Framework.Logging.
The facade has no LogWarning method. Add one and replace _logger.LogWarning(...) at line 56 with it. Preserve the orphaned-department message.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Workers/Resgrid.Workers.Console/Tasks/CallPruneTask.cs` at line 56, Add a
LogWarning method to the Resgrid.Framework.Logging facade, then update
CallPruneTask to call that facade method instead of _logger.LogWarning while
preserving the existing orphaned-department message and interpolation.
Source: Coding guidelines
| catch (RedisConnectionException ex) | ||
| { | ||
| // Transient connection drop (idle reset, failover); fallback below handles it. | ||
| Logging.LogError(ex); |
There was a problem hiding this comment.
Insufficient logging context in the Logging.LogError call, as the method passes only the exception object without the operation name or identifiers like cacheKey, violating Rule 3 for traceable errors. Include context using Logging.LogError("Retrieve cache failed", new { cacheKey, ex }).
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Providers/Resgrid.Providers.Cache/AzureRedisCacheProvider.cs:
Line 55:
Insufficient logging context in the Logging.LogError call, as the method passes only the exception object without the operation name or identifiers like cacheKey, violating Rule 3 for traceable errors. Include context using Logging.LogError("Retrieve cache failed", new { cacheKey, ex }).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| catch (TimeoutException) | ||
| { } |
There was a problem hiding this comment.
Silent exception swallowing in the catch block, where the empty block catches TimeoutException without logging or handling it, violating Rule 28. Log the timeout with operation context using Logging.LogError("GetStringAsync timed out", new { cacheKey }).
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Providers/Resgrid.Providers.Cache/AzureRedisCacheProvider.cs:
Line 226 to 227:
Silent exception swallowing in the catch block, where the empty block catches TimeoutException without logging or handling it, violating Rule 28. Log the timeout with operation context using Logging.LogError("GetStringAsync timed out", new { cacheKey }).
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This comment has been minimized.
This comment has been minimized.
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 (1)
Core/Resgrid.Services/DeleteService.cs (1)
50-75: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winResolve
IScheduledTasksServicein the constructor.Line 50 adds constructor injection. Resolve
IScheduledTasksServicethroughBootstrapper.GetKernel().Resolve<IScheduledTasksService>()in the constructor instead. Remove the added constructor parameter.As per coding guidelines, use "
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/DeleteService.cs` around lines 50 - 75, Update the DeleteService constructor to remove the IScheduledTasksService parameter and resolve it inside the constructor using Bootstrapper.GetKernel().Resolve<IScheduledTasksService>(). Keep assigning the resolved service to _scheduledTasksService and preserve the existing handling of all other dependencies.Source: Coding guidelines
🧹 Nitpick comments (3)
Web/Resgrid.Web/Areas/User/Controllers/ProfileController.cs (1)
53-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve
IDeleteServicewith the service locator instead of adding a constructor parameter.The coding guidelines require dependencies to be resolved with
Bootstrapper.GetKernel().Resolve<T>()inside the constructor, and they require the number of constructor-injected dependencies to stay small. This constructor already accepts 14 dependencies. The change adds a 15th.♻️ Proposed refactor
public ProfileController(IDepartmentsService departmentsService, IUsersService usersService, Model.Services.IAuthorizationService authorizationService, IUserProfileService userProfileService, IScheduledTasksService scheduledTasksService, ICertificationService certificationService, ICustomStateService customStateService, IImageService imageService, IOptions<AppOptions> appOptionsAccessor, IEmailService emailService, UserManager<IdentityUser> userManager, SignInManager<IdentityUser> signInManager, IDepartmentSsoService departmentSsoService, - IStringLocalizer<Resgrid.Localization.Areas.User.Security.Security> secLocalizer, IDeleteService deleteService) + IStringLocalizer<Resgrid.Localization.Areas.User.Security.Security> secLocalizer) {- _deleteService = deleteService; + _deleteService = Bootstrapper.GetKernel().Resolve<IDeleteService>();As per coding guidelines: "Use
Service Locatorpattern viaBootstrapper.GetKernel().Resolve<T>()to resolve dependencies explicitly in constructors, rather than constructor injection" and "Minimize constructor injection; keep the number of injected dependencies small".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/ProfileController.cs` around lines 53 - 60, Remove the IDeleteService deleteService parameter from ProfileController’s constructor and resolve the dependency inside the constructor with Bootstrapper.GetKernel().Resolve<IDeleteService>(). Assign the resolved service to the existing _deleteService field while preserving all other constructor dependencies and behavior.Source: Coding guidelines
Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs (1)
138-180: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftRoute the new deletes through
_connectionProviderand_unitOfWork, and wrap them in a transaction.Three points apply to both new methods:
- Both methods create raw
NpgsqlConnectionandSqlConnectioninstances.GetAllUpcomingOrRecurringReportDeliveryTasksAsyncat Lines 97-127 uses_connectionProviderand_unitOfWorkinstead. The new methods therefore run outside any ambient transaction that a caller started.- Each method runs two
DELETEstatements with no transaction. If the second statement fails, the log rows are already gone but the tasks remain.- Both methods return a constant
true. Theboolresult carries no information, so a caller cannot detect a failure.
DeleteAllTasksForUserAsyncandDeleteAllTasksForUserInDepartmentAsyncdiffer only by the department predicate. Extract one private helper that takes the optional department id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs` around lines 138 - 180, Refactor DeleteAllTasksForUserAsync and DeleteAllTasksForUserInDepartmentAsync into one private helper accepting an optional departmentId, and have both public methods delegate to it. Use _connectionProvider and _unitOfWork like GetAllUpcomingOrRecurringReportDeliveryTasksAsync, execute both deletes within a transaction, and return the actual operation result rather than a constant true.Repositories/Resgrid.Repositories.DataRepository/DeleteRepository.cs (1)
124-151: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftMaterialize the parent id sets once instead of repeating the subqueries.
(SELECT CallId FROM [dbo].[Calls] WHERE DepartmentId =@DepartmentId)appears 8 times. TheShiftschain nests the same lookup up to three levels deep on Lines 140-142. SQL Server re-evaluates each subquery per statement, and the whole batch holds one transaction open. This lengthens lock duration on a busy database.Load the ids into temp tables once, then delete against them.
♻️ Proposed refactor (pattern)
+ SELECT CallId INTO `#DeptCalls` FROM [dbo].[Calls] WHERE DepartmentId = `@DepartmentId` + SELECT ShiftId INTO `#DeptShifts` FROM [dbo].[Shifts] WHERE DepartmentId = `@DepartmentId` + SELECT ShiftSignupId INTO `#DeptShiftSignups` FROM [dbo].[ShiftSignups] WHERE ShiftId IN (SELECT ShiftId FROM `#DeptShifts`) + -- Call child data (parents deleted further down) - DELETE FROM [dbo].[CallAttachments] WHERE CallId IN (SELECT CallId FROM [dbo].[Calls] WHERE DepartmentId = `@DepartmentId`) - DELETE FROM [dbo].[CallNotes] WHERE CallId IN (SELECT CallId FROM [dbo].[Calls] WHERE DepartmentId = `@DepartmentId`) + DELETE FROM [dbo].[CallAttachments] WHERE CallId IN (SELECT CallId FROM `#DeptCalls`) + DELETE FROM [dbo].[CallNotes] WHERE CallId IN (SELECT CallId FROM `#DeptCalls`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Repositories/Resgrid.Repositories.DataRepository/DeleteRepository.cs` around lines 124 - 151, In the department-delete SQL block, materialize the department’s CallId, ShiftId, ShiftSignupId, and related parent ID sets into temporary tables once, then reuse those tables for the CallAttachments through CallProtocols deletes and the ShiftSignup/ShiftGroup deletes. Replace the repeated nested subqueries while preserving deletion order and the existing `@DepartmentId` scope, and clean up the temporary tables within the batch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/DeleteService.cs`:
- Around line 111-117: The DeactivateUserAccountCoreAsync cleanup flow must not
leave memberships deleted when a later cleanup operation fails. In
Core/Resgrid.Services/DeleteService.cs lines 111-117 and 171-180, ensure all
role, group, distribution-list, and scheduled-task cleanup completes
successfully before invoking the membership-deletion operation DeleteUserAsync;
preserve the same ordering and atomic behavior at both affected sites.
In `@Core/Resgrid.Services/ScheduledTasksService.cs`:
- Around line 235-248: Update DeleteAllTasksForUserAsync and
DeleteAllTasksForUserInDepartmentAsync to call
cancellationToken.ThrowIfCancellationRequested() before invoking their
repository deletion methods. Propagate the token through the scheduled-task
repository contract, implementations, and database commands so in-progress
deletions can observe cancellation where supported, while preserving cache
invalidation and return behavior.
In `@Repositories/Resgrid.Repositories.DataRepository/DeleteRepository.cs`:
- Around line 152-236: In the department deletion SQL within the repository’s
department-delete method, move the CommandDefinitions deletion ahead of the
CallTypes deletion so CommandDefinitions.CallTypeId dependencies are removed
first. Leave DepartmentCallPriorities ordering unchanged because it does not
reference Calls.
In `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs`:
- Around line 483-497: The multi-statement de-provisioning in
ClearOutUserLoginAsync must execute atomically. In
Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs lines
483-497, begin a transaction on the NpgsqlConnection, pass it to every
ExecuteAsync call, and commit after the final statement; apply the identical
transaction wrapper to the SqlConnection branch at lines 514-528, rolling back
on failure as supported by the existing flow.
In
`@Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs`:
- Around line 66-67: Update both member predicates in
GetAllActiveTasksForTypesAsync: at
Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs
lines 66-67, allow NULL IsDeleted values alongside false while preserving the
existing IsDisabled guard; apply the equivalent NULL-or-zero condition at lines
85-86 for the second database branch.
In `@Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs`:
- Line 713: Update the user-facing error message in PersonnelController’s
deletion flow to replace the misspelled word “latter” with “later,” leaving the
rest of the message unchanged.
In `@Web/Resgrid.Web/Areas/User/Controllers/ProfileController.cs`:
- Line 1168: Update all three RevokeDepartmentAccessAsync call sites in
DeleteDepartmentLink—Web/Resgrid.Web/Areas/User/Controllers/ProfileController.cs#L1168-L1168,
`#L1196-L1196`, and `#L1220-L1220`—to capture the returned bool. When revocation
fails, log the failure, add an error, and return to YourDepartments; preserve
the existing successful re-sign-in/dashboard flow for the first two paths and
successful YourDepartments redirect for the third.
---
Outside diff comments:
In `@Core/Resgrid.Services/DeleteService.cs`:
- Around line 50-75: Update the DeleteService constructor to remove the
IScheduledTasksService parameter and resolve it inside the constructor using
Bootstrapper.GetKernel().Resolve<IScheduledTasksService>(). Keep assigning the
resolved service to _scheduledTasksService and preserve the existing handling of
all other dependencies.
---
Nitpick comments:
In `@Repositories/Resgrid.Repositories.DataRepository/DeleteRepository.cs`:
- Around line 124-151: In the department-delete SQL block, materialize the
department’s CallId, ShiftId, ShiftSignupId, and related parent ID sets into
temporary tables once, then reuse those tables for the CallAttachments through
CallProtocols deletes and the ShiftSignup/ShiftGroup deletes. Replace the
repeated nested subqueries while preserving deletion order and the existing
`@DepartmentId` scope, and clean up the temporary tables within the batch.
In
`@Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs`:
- Around line 138-180: Refactor DeleteAllTasksForUserAsync and
DeleteAllTasksForUserInDepartmentAsync into one private helper accepting an
optional departmentId, and have both public methods delegate to it. Use
_connectionProvider and _unitOfWork like
GetAllUpcomingOrRecurringReportDeliveryTasksAsync, execute both deletes within a
transaction, and return the actual operation result rather than a constant true.
In `@Web/Resgrid.Web/Areas/User/Controllers/ProfileController.cs`:
- Around line 53-60: Remove the IDeleteService deleteService parameter from
ProfileController’s constructor and resolve the dependency inside the
constructor with Bootstrapper.GetKernel().Resolve<IDeleteService>(). Assign the
resolved service to the existing _deleteService field while preserving all other
constructor dependencies and behavior.
🪄 Autofix
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 Plus
Run ID: 45ed5f70-0607-4e36-afd7-62bb59d308b8
⛔ Files ignored due to path filters (9)
Core/Resgrid.Localization/Areas/User/Account/DeleteAccount.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Account/DeleteAccount.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Account/DeleteAccount.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Account/DeleteAccount.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Account/DeleteAccount.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Account/DeleteAccount.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Account/DeleteAccount.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Account/DeleteAccount.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Account/DeleteAccount.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (13)
Core/Resgrid.Model/Repositories/IScheduledTasksRepository.csCore/Resgrid.Model/Services/IDeleteService.csCore/Resgrid.Model/Services/IDistributionListsService.csCore/Resgrid.Model/Services/IScheduledTasksService.csCore/Resgrid.Services/DeleteService.csCore/Resgrid.Services/DistributionListsService.csCore/Resgrid.Services/ScheduledTasksService.csRepositories/Resgrid.Repositories.DataRepository/DeleteRepository.csRepositories/Resgrid.Repositories.DataRepository/IdentityRepository.csRepositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/ProfileController.csWeb/Resgrid.Web/Areas/User/Views/Account/DeleteAccount.cshtml
| await _personnelRolesService.RemoveUserFromAllRolesAsync(userId, departmentId, cancellationToken); | ||
| await _departmentGroupsService.DeleteUserFromGroupsAsync(userId, departmentId, cancellationToken); | ||
| await _distributionListsService.RemoveUserFromAllListsInDepartmentAsync(userId, departmentId, cancellationToken); | ||
| await _scheduledTasksService.DeleteAllTasksForUserInDepartmentAsync(userId, departmentId, cancellationToken); | ||
|
|
||
| // Soft-delete the membership last (this also writes the audit event and clears caches). | ||
| await _departmentsService.DeleteUserAsync(departmentId, userId, revokingUserId, cancellationToken); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs --items all
rg -n -C 12 'DeleteAllTasksForUser(Async|InDepartmentAsync)' \
Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs \
Core/Resgrid.Services/DeleteService.cs \
Core/Resgrid.Services/ScheduledTasksService.csRepository: Resgrid/Core
Length of output: 14838
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ScheduledTasksRepository deletion methods ---'
sed -n '138,190p' Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs
printf '%s\n' '--- DeleteService account-deletion result path ---'
sed -n '122,235p' Core/Resgrid.Services/DeleteService.cs
printf '%s\n' '--- Related cleanup signatures and implementations ---'
rg -n -C 8 \
'RemoveUserFromAllRolesAsync|DeleteUserFromGroupsAsync|RemoveUserFromAllLists(InDepartment)?Async|DeleteUserAsync' \
Core/Resgrid.Services Repositories Core/Resgrid.ModelRepository: Resgrid/Core
Length of output: 35759
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Membership soft-delete implementation ---'
sed -n '286,330p' Core/Resgrid.Services/DepartmentsService.cs
printf '%s\n' '--- Department-group cleanup implementation ---'
sed -n '337,365p' Core/Resgrid.Services/DepartmentGroupsService.cs
printf '%s\n' '--- Distribution-list cleanup implementation ---'
sed -n '111,145p' Core/Resgrid.Services/DistributionListsService.cs
printf '%s\n' '--- Cleanup return-value tests and implementations ---'
rg -n -C 5 \
'DeleteAllTasksForUser(InDepartment)?Async|RemoveUserFromAllRolesAsync|DeleteUserFromGroupsAsync|RemoveUserFromAllLists(InDepartment)?Async|RevokeDepartmentAccessAsync' \
--glob '*Tests*' --glob '*Test*' . || trueRepository: Resgrid/Core
Length of output: 3942
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
repo = Path("Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs").read_text()
service = Path("Core/Resgrid.Services/ScheduledTasksService.cs").read_text()
methods = [
"DeleteAllTasksForUserAsync",
"DeleteAllTasksForUserInDepartmentAsync",
]
for name in methods:
match = re.search(
rf"public async Task<bool> {name}\b.*?\n\t\t\}}",
repo,
re.S,
)
if not match:
raise SystemExit(f"missing repository method: {name}")
body = match.group(0)
print(f"{name}: return_true={bool(re.search(r'\\breturn true;', body))}, "
f"return_false={bool(re.search(r'\\breturn false;', body))}, "
f"throws_explicitly={bool(re.search(r'\\bthrow\\b', body))}")
for name in methods:
match = re.search(
rf"public async Task<bool> {name}\b.*?\n\t\t\}}",
service,
re.S,
)
if not match:
raise SystemExit(f"missing service method: {name}")
body = match.group(0)
print(f"service {name}: forwards_result={bool(re.search(r'return result;', body))}")
PYRepository: Resgrid/Core
Length of output: 373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
repo = Path("Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs").read_text()
service = Path("Core/Resgrid.Services/ScheduledTasksService.cs").read_text()
methods = [
"DeleteAllTasksForUserAsync",
"DeleteAllTasksForUserInDepartmentAsync",
]
for name in methods:
match = re.search(
rf"public async Task<bool> {name}\b.*?\n\t\t\}}",
repo,
re.S,
)
if not match:
raise SystemExit(f"missing repository method: {name}")
body = match.group(0)
print(
f"{name}: "
f"return_true={bool(re.search(r'\\breturn true;', body))}, "
f"return_false={bool(re.search(r'\\breturn false;', body))}, "
f"throws_explicitly={bool(re.search(r'\\bthrow\\b', body))}"
)
for name in methods:
match = re.search(
rf"public async Task<bool> {name}\b.*?\n\t\t\}}",
service,
re.S,
)
if not match:
raise SystemExit(f"missing service method: {name}")
body = match.group(0)
print(
f"service {name}: "
f"forwards_result={bool(re.search(r'return result;', body))}"
)
PYRepository: Resgrid/Core
Length of output: 253
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
repo = Path("Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs").read_text()
service = Path("Core/Resgrid.Services/ScheduledTasksService.cs").read_text()
methods = [
"DeleteAllTasksForUserAsync",
"DeleteAllTasksForUserInDepartmentAsync",
]
for source_name, source in [("repository", repo), ("service", service)]:
for name in methods:
match = re.search(
rf"public async Task<bool> {name}\b.*?\n\t\t\}}",
source,
re.S,
)
if not match:
raise SystemExit(f"missing {source_name} method: {name}")
body = match.group(0)
has_true = bool(re.search(r"\breturn true;", body))
has_false = bool(re.search(r"\breturn false;", body))
has_throw = bool(re.search(r"\bthrow\b", body))
forwards_result = bool(re.search(r"\breturn result;", body))
print(
source_name,
name,
f"return_true={has_true}",
f"return_false={has_false}",
f"throws_explicitly={has_throw}",
f"forwards_result={forwards_result}",
)
PYRepository: Resgrid/Core
Length of output: 648
Make account deactivation atomic across cleanup failures.
DeactivateUserAccountCoreAsync saves deleted memberships before removing roles, groups, distribution-list memberships, and scheduled tasks. If a later cleanup call throws, the login remains active while earlier memberships are deleted and later cleanup may be incomplete. Move membership writes after successful cleanup, or use transactional recovery.
📍 Affects 1 file
Core/Resgrid.Services/DeleteService.cs#L111-L117(this comment)Core/Resgrid.Services/DeleteService.cs#L171-L180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/DeleteService.cs` around lines 111 - 117, The
DeactivateUserAccountCoreAsync cleanup flow must not leave memberships deleted
when a later cleanup operation fails. In Core/Resgrid.Services/DeleteService.cs
lines 111-117 and 171-180, ensure all role, group, distribution-list, and
scheduled-task cleanup completes successfully before invoking the
membership-deletion operation DeleteUserAsync; preserve the same ordering and
atomic behavior at both affected sites.
| WHERE st.departmentid = 0 AND st.active = true AND st.tasktype = any (@types) | ||
| AND dm.isdeleted = false AND (dm.isdisabled IS NULL OR dm.isdisabled = false)", new { types = types }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The new member filters guard NULL on IsDisabled but not on IsDeleted. Both database branches of GetAllActiveTasksForTypesAsync use the same predicate shape. If DepartmentMembers.IsDeleted is nullable, rows with a NULL value are now excluded and their scheduled tasks stop running.
Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs#L66-L67: changedm.isdeleted = falseto(dm.isdeleted IS NULL OR dm.isdeleted = false), or drop the NULL guard onisdisabledif the column is non-nullable.Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs#L85-L86: apply the matching change todm.IsDeleted = 0.
📍 Affects 1 file
Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs#L66-L67(this comment)Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs#L85-L86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs`
around lines 66 - 67, Update both member predicates in
GetAllActiveTasksForTypesAsync: at
Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs
lines 66-67, allow NULL IsDeleted values alongside false while preserving the
existing IsDisabled guard; apply the equivalent NULL-or-zero condition at lines
85-86 for the second database branch.
| foreach (var dm in departments) | ||
| { | ||
| var dep = await _departmentsService.GetDepartmentByUserIdAsync(userIdToDelete); | ||
| var dep = await _departmentsService.GetDepartmentByIdAsync(dm.DepartmentId); |
There was a problem hiding this comment.
N+1 query generated by calling GetDepartmentByIdAsync inside a foreach loop, causing one database round-trip per membership. Fetch all departments in a single call using GetDepartmentsByIdsAsync or a LINQ Contains query before the loop to eliminate sequential queries.
Kody rule violation: Optimize database queries with JOINs
Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 141:
N+1 query generated by calling `GetDepartmentByIdAsync` inside a foreach loop, causing one database round-trip per membership. Fetch all departments in a single call using `GetDepartmentsByIdsAsync` or a LINQ `Contains` query before the loop to eliminate sequential queries.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| public async Task<bool> RevokeDepartmentAccessAsync(string userId, int departmentId, string revokingUserId, CancellationToken cancellationToken = default(CancellationToken)) | ||
| { | ||
| //if (!await _authorizationService.CanUserDeleteUserAsync(departmentId, authorizingUserId, userIdToDelete)) | ||
| // return DeleteUserResults.UnAuthroized; | ||
| // Strip everything that would keep the department reaching the user: roles, | ||
| // group memberships, distribution lists and their scheduled automations | ||
| // (status changes, staffing changes, report deliveries) for this department. | ||
| await _personnelRolesService.RemoveUserFromAllRolesAsync(userId, departmentId, cancellationToken); | ||
| await _departmentGroupsService.DeleteUserFromGroupsAsync(userId, departmentId, cancellationToken); | ||
| await _distributionListsService.RemoveUserFromAllListsInDepartmentAsync(userId, departmentId, cancellationToken); | ||
| await _scheduledTasksService.DeleteAllTasksForUserInDepartmentAsync(userId, departmentId, cancellationToken); | ||
|
|
||
| // Soft-delete the membership last (this also writes the audit event and clears caches). | ||
| await _departmentsService.DeleteUserAsync(departmentId, userId, revokingUserId, cancellationToken); | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
Partial-revocation risk identified in RevokeDepartmentAccessAsync, as 5 sequential async database operations execute without a transaction wrapper. A mid-sequence failure, such as a DB timeout, leaves the user in a partially-revoked state without automatic rollback. Wrap the operations in a database transaction to ensure atomic execution.
// Consider wrapping in a transaction or implementing compensating logic
// so that a failure mid-sequence does not leave the user partially revoked.Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 106 to 120:
Partial-revocation risk identified in `RevokeDepartmentAccessAsync`, as 5 sequential async database operations execute without a transaction wrapper. A mid-sequence failure, such as a DB timeout, leaves the user in a partially-revoked state without automatic rollback. Wrap the operations in a database transaction to ensure atomic execution.
Suggested Code:
// Consider wrapping in a transaction or implementing compensating logic
// so that a failure mid-sequence does not leave the user partially revoked.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| foreach (var member in members.Where(x => departmentListIds.Contains(x.DistributionListId))) | ||
| { | ||
| await _distributionListMemberRepository.DeleteAsync(member, cancellationToken); | ||
| } |
There was a problem hiding this comment.
N+1 delete pattern triggered by calling DeleteAsync individually for each member inside a loop, degrading performance as member count grows. Replace the loop with a batch delete method like DeleteManyAsync or a single DELETE WHERE … IN (…) query.
Kody rule violation: Detect N+1 style queries and suggest batching
Prompt for LLM
File Core/Resgrid.Services/DistributionListsService.cs:
Line 121 to 124:
N+1 delete pattern triggered by calling `DeleteAsync` individually for each member inside a loop, degrading performance as member count grows. Replace the loop with a batch delete method like `DeleteManyAsync` or a single `DELETE WHERE … IN (…)` query.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| WHERE DepartmentId = @DepartmentId | ||
|
|
||
| -- Child rows of data the cursors below delete piecemeal; remove while parents still exist | ||
| DELETE FROM [dbo].[ScheduledTaskLogs] WHERE ScheduledTaskId IN (SELECT ScheduledTaskId FROM [dbo].[ScheduledTasks] WHERE DepartmentId = @DepartmentId) |
There was a problem hiding this comment.
Data inconsistency risk caused by executing a large cascade of DELETE statements without a visible transaction boundary. Wrap the department-deletion batch in an explicit BEGIN TRANSACTION / COMMIT block or a C# TransactionScope with a TRY…CATCH rollback to prevent broken referential integrity upon failure.
Kody rule violation: Handle transaction rollbacks properly
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/DeleteRepository.cs:
Line 51:
Data inconsistency risk caused by executing a large cascade of DELETE statements without a visible transaction boundary. Wrap the department-deletion batch in an explicit `BEGIN TRANSACTION / COMMIT` block or a C# `TransactionScope` with a `TRY…CATCH` rollback to prevent broken referential integrity upon failure.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| DELETE FROM [dbo].[ChatbotDepartmentConfigs] WHERE DepartmentId = @DepartmentId | ||
|
|
||
| -- Remaining department-scoped tables | ||
| DELETE FROM [dbo].[AuditLogs] WHERE DepartmentId = @DepartmentId |
There was a problem hiding this comment.
Data integrity violation caused by permanently deleting audit log entries during the department deletion cascade. Audit logs must remain immutable to preserve records required for compliance; remove this DELETE statement or anonymize the DepartmentId and user identifiers while recording the department-deletion action as a new audit entry.
Kody rule violation: Write immutable audit logs for all ePHI access
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/DeleteRepository.cs:
Line 211:
Data integrity violation caused by permanently deleting audit log entries during the department deletion cascade. Audit logs must remain immutable to preserve records required for compliance; remove this DELETE statement or anonymize the `DepartmentId` and user identifiers while recording the department-deletion action as a new audit entry.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var member = await _departmentsService.DeleteUserAsync(DepartmentId, model.UserId, UserId, cancellationToken); | ||
| //var result = await _deleteService.DeleteUser(DepartmentId, UserId, model.UserId); | ||
| var result = await _deleteService.DeleteUserAsync(DepartmentId, UserId, model.UserId, cancellationToken); |
There was a problem hiding this comment.
Unstructured 500 error risk caused by invoking _deleteService.DeleteUserAsync(...) without error handling. If the deletion service rejects, the exception propagates unhandled. Wrap the await call in a try/catch block, log the exception with context, and return a user-friendly ModelState error.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs:
Line 694:
Unstructured 500 error risk caused by invoking `_deleteService.DeleteUserAsync(...)` without error handling. If the deletion service rejects, the exception propagates unhandled. Wrap the await call in a try/catch block, log the exception with context, and return a user-friendly `ModelState` error.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| var member = await _departmentsService.DeleteUserAsync(DepartmentId, model.UserId, UserId, cancellationToken); | ||
| //var result = await _deleteService.DeleteUser(DepartmentId, UserId, model.UserId); | ||
| var result = await _deleteService.DeleteUserAsync(DepartmentId, UserId, model.UserId, cancellationToken); |
There was a problem hiding this comment.
Unhandled exception risk identified where the external service call _deleteService.DeleteUserAsync(...) executes without a try/catch boundary. Wrap the call in a try/catch block, enrich the logs with DepartmentId and UserId, and return a meaningful error result.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs:
Line 694:
Unhandled exception risk identified where the external service call `_deleteService.DeleteUserAsync(...)` executes without a try/catch boundary. Wrap the call in a try/catch block, enrich the logs with `DepartmentId` and `UserId`, and return a meaningful error result.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs (1)
575-591: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDelete dependent OIDC tokens in one transaction.
OpenIddictTokens.AuthorizationIdhas aRestrictforeign key toOpenIddictAuthorizations. The token delete filters onlySubject, so it can miss tokens that reference the user's authorizations. Include those tokens byAuthorizationId, then delete the authorizations in one transaction in both database branches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs` around lines 575 - 591, The OIDC cleanup in both database branches must delete dependent tokens before authorizations within a single transaction. Update the SQL in the cleanup block to delete tokens matching the user subject or referencing that user’s authorization records via AuthorizationId, then delete OpenIddictAuthorizations using the same transaction connection; apply this consistently to both the PostgreSQL and SQL Server branches.
🧹 Nitpick comments (1)
Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs (1)
468-562: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse asynchronous transaction methods in both provider branches. The project targets
net9.0; useBeginTransactionAsync(),CommitAsync(), andawait using. Remove the explicit rollback catch because disposal rolls back uncommitted transactions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs` around lines 468 - 562, Update both PostgreSQL and SQL Server transaction blocks to use await using with BeginTransactionAsync(), and await CommitAsync() after successful operations. Remove the explicit try/catch rollback handling so transaction disposal rolls back any uncommitted work while preserving exception propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs`:
- Around line 167-168: Update the scheduled-task deletion flow containing the
ExecuteAsync calls to fail closed when departmentId is non-positive. Before
executing SQL, reject departmentId <= 0, or ensure both the scheduledtasks
predicate and its scheduledtasklogs subquery require DepartmentId > 0, while
preserving deletion for valid positive department IDs.
- Around line 145-146: Update the scheduled-task cleanup methods containing the
paired scheduledtasklogs and scheduledtasks DELETE statements, including the
additional affected call sites, to execute both commands within a single
database transaction. Begin the transaction before log deletion, pass it to both
Dapper CommandDefinitions, commit only after both succeed, and roll back on
failure or cancellation so cleanup is atomic.
---
Outside diff comments:
In `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs`:
- Around line 575-591: The OIDC cleanup in both database branches must delete
dependent tokens before authorizations within a single transaction. Update the
SQL in the cleanup block to delete tokens matching the user subject or
referencing that user’s authorization records via AuthorizationId, then delete
OpenIddictAuthorizations using the same transaction connection; apply this
consistently to both the PostgreSQL and SQL Server branches.
---
Nitpick comments:
In `@Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs`:
- Around line 468-562: Update both PostgreSQL and SQL Server transaction blocks
to use await using with BeginTransactionAsync(), and await CommitAsync() after
successful operations. Remove the explicit try/catch rollback handling so
transaction disposal rolls back any uncommitted work while preserving exception
propagation.
🪄 Autofix
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 Plus
Run ID: 45cd9dfc-516e-463a-9160-92b2098b1bc3
📒 Files selected for processing (10)
Core/Resgrid.Framework/Logging.csCore/Resgrid.Model/Repositories/IScheduledTasksRepository.csCore/Resgrid.Services/DeleteService.csCore/Resgrid.Services/ScheduledTasksService.csRepositories/Resgrid.Repositories.DataRepository/DeleteRepository.csRepositories/Resgrid.Repositories.DataRepository/IdentityRepository.csRepositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.csWeb/Resgrid.Web/Areas/User/Controllers/PersonnelController.csWeb/Resgrid.Web/Areas/User/Controllers/ProfileController.csWorkers/Resgrid.Workers.Console/Tasks/CallPruneTask.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- Web/Resgrid.Web/Areas/User/Controllers/PersonnelController.cs
- Workers/Resgrid.Workers.Console/Tasks/CallPruneTask.cs
- Core/Resgrid.Services/ScheduledTasksService.cs
- Web/Resgrid.Web/Areas/User/Controllers/ProfileController.cs
- Repositories/Resgrid.Repositories.DataRepository/DeleteRepository.cs
- Core/Resgrid.Services/DeleteService.cs
| // operation retryable (mirrors the ordering in RevokeDepartmentAccessAsync). | ||
| foreach (var dm in departments) | ||
| { | ||
| await _personnelRolesService.RemoveUserFromAllRolesAsync(userIdToDelete, dm.DepartmentId, cancellationToken); |
There was a problem hiding this comment.
N+1 query pattern identified in the awaited RemoveUserFromAllRolesAsync call inside the foreach loop over departments, multiplying latency and database load linearly with department count. Add a batch overload such as RemoveUserFromAllRolesInAllDepartmentsAsync(userIdToDelete, departmentIds, cancellationToken) that deletes roles for all departments in a single set-based query or batched transaction.
Kody rule violation: Detect N+1 style queries and suggest batching
Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 152:
N+1 query pattern identified in the awaited `RemoveUserFromAllRolesAsync` call inside the `foreach` loop over departments, multiplying latency and database load linearly with department count. Add a batch overload such as `RemoveUserFromAllRolesInAllDepartmentsAsync(userIdToDelete, departmentIds, cancellationToken)` that deletes roles for all departments in a single set-based query or batched transaction.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| foreach (var dm in departments) | ||
| { | ||
| await _personnelRolesService.RemoveUserFromAllRolesAsync(userIdToDelete, dm.DepartmentId, cancellationToken); | ||
| await _departmentGroupsService.DeleteUserFromGroupsAsync(userIdToDelete, dm.DepartmentId, cancellationToken); | ||
| } |
There was a problem hiding this comment.
Partial cleanup state vulnerability occurs when the loop performs multi-step writes across multiple departments with no wrapping transaction. Wrap the entire loop in a single transaction scope (e.g., await using var tx = _unitOfWork.BeginTransactionAsync(cancellationToken)) to ensure all role and group modifications commit or roll back atomically, or explicitly verify the idempotency guarantees of each service method.
Kody rule violation: Handle transaction rollbacks properly
Prompt for LLM
File Core/Resgrid.Services/DeleteService.cs:
Line 150 to 154:
Partial cleanup state vulnerability occurs when the loop performs multi-step writes across multiple departments with no wrapping transaction. Wrap the entire loop in a single transaction scope (e.g., `await using var tx = _unitOfWork.BeginTransactionAsync(cancellationToken)`) to ensure all role and group modifications commit or roll back atomically, or explicitly verify the idempotency guarantees of each service method.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| var result = await _scheduledTaskRepository.DeleteAllTasksForUserInDepartmentAsync(userId, departmentId, cancellationToken); |
There was a problem hiding this comment.
Unguarded external database call occurs because the _scheduledTaskRepository.DeleteAllTasksForUserInDepartmentAsync invocation is not wrapped in a try/catch. Wrap the call in a try/catch, add meaningful context (userId, departmentId, operation name), and rethrow or return a domain-specific error.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File Core/Resgrid.Services/ScheduledTasksService.cs:
Line 249:
Unguarded external database call occurs because the `_scheduledTaskRepository.DeleteAllTasksForUserInDepartmentAsync` invocation is not wrapped in a `try/catch`. Wrap the call in a `try/catch`, add meaningful context (`userId`, `departmentId`, operation name), and rethrow or return a domain-specific error.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| new { userId = userId, deleteId = deleteId, normalizedDeleteId = deleteId.ToUpperInvariant(), maskedEmail = maskedEmail, normalizedMaskedEmail = maskedEmail.ToUpperInvariant(), securityStamp = Guid.NewGuid().ToString(), lockoutEnd = new DateTimeOffset(9999, 12, 31, 23, 59, 59, TimeSpan.Zero) }); | ||
| using (var db = new NpgsqlConnection(DataConfig.CoreConnectionString)) | ||
| { | ||
| await db.OpenAsync(); |
There was a problem hiding this comment.
Unguarded connection failure exception propagates because the awaited db.OpenAsync() call sits before the try block. Move db.OpenAsync() inside the existing try block or add a separate try/catch around it to log context and handle the error appropriately.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs:
Line 470:
Unguarded connection failure exception propagates because the awaited `db.OpenAsync()` call sits before the `try` block. Move `db.OpenAsync()` inside the existing `try` block or add a separate `try/catch` around it to log context and handle the error appropriately.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| // The whole de-provision is atomic: either the account is fully fuzzed and all | ||
| // re-entry vectors are removed, or nothing changes and the operation can be retried. | ||
| using (var transaction = db.BeginTransaction()) |
There was a problem hiding this comment.
Thread blocking violation occurs because the synchronous db.BeginTransaction() is called inside an async method. Replace it with using (var transaction = await db.BeginTransactionAsync()) to await the operation properly.
Kody rule violation: Use Awaitable Methods in Async Code
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/IdentityRepository.cs:
Line 474:
Thread blocking violation occurs because the synchronous `db.BeginTransaction()` is called inside an async method. Replace it with `using (var transaction = await db.BeginTransactionAsync())` to await the operation properly.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| { | ||
| // Orphaned pruning row for a deleted department; skip it so we don't | ||
| // null-ref in CallPruneLogic or spam error logs every run. | ||
| Resgrid.Framework.Logging.LogWarning($"CallPrune::Skipping orphaned pruning settings for deleted DepartmentId:{item.PruneSettings.DepartmentId}"); |
There was a problem hiding this comment.
Unstructured logging violation occurs because the warning log embeds context via string interpolation instead of passing structured fields, preventing the logging framework from capturing DepartmentId as a queryable property. Use a message template with a named placeholder and pass the value as an argument, such as Resgrid.Framework.Logging.LogError("CallPrune::Skipping orphaned pruning settings for deleted DepartmentId:{DepartmentId}", item.PruneSettings.DepartmentId);.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File Workers/Resgrid.Workers.Console/Tasks/CallPruneTask.cs:
Line 56:
Unstructured logging violation occurs because the warning log embeds context via string interpolation instead of passing structured fields, preventing the logging framework from capturing `DepartmentId` as a queryable property. Use a message template with a named placeholder and pass the value as an argument, such as `Resgrid.Framework.Logging.LogError("CallPrune::Skipping orphaned pruning settings for deleted DepartmentId:{DepartmentId}", item.PruneSettings.DepartmentId);`.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This comment has been minimized.
This comment has been minimized.
| using (var transaction = db.BeginTransaction()) | ||
| { | ||
| try | ||
| { | ||
| await db.ExecuteAsync(new Dapper.CommandDefinition(@"DELETE FROM scheduledtasklogs WHERE scheduledtaskid IN (SELECT scheduledtaskid FROM scheduledtasks WHERE userid = @userId)", new { userId = userId }, transaction: transaction, cancellationToken: cancellationToken)); | ||
| await db.ExecuteAsync(new Dapper.CommandDefinition(@"DELETE FROM scheduledtasks WHERE userid = @userId", new { userId = userId }, transaction: transaction, cancellationToken: cancellationToken)); | ||
|
|
||
| transaction.Commit(); | ||
| } | ||
| catch | ||
| { | ||
| transaction.Rollback(); | ||
| throw; | ||
| } |
There was a problem hiding this comment.
Sync-over-async blocking I/O occurs because synchronous transaction calls (db.BeginTransaction(), transaction.Commit(), transaction.Rollback()) execute inside async methods, risking thread-pool starvation under concurrent load. Apply the async variants (await db.BeginTransactionAsync(cancellationToken), etc.) to align with the established pattern in UserProfilesRepository.cs:343-346 across all four transaction blocks (lines 148, 171, 209, 232).
using (var transaction = await db.BeginTransactionAsync(cancellationToken))
{
try
{
await db.ExecuteAsync(new Dapper.CommandDefinition(@"DELETE FROM scheduledtasklogs ...", new { userId = userId }, transaction: transaction, cancellationToken: cancellationToken));
await db.ExecuteAsync(new Dapper.CommandDefinition(@"DELETE FROM scheduledtasks WHERE userid = @userId", new { userId = userId }, transaction: transaction, cancellationToken: cancellationToken));
await transaction.CommitAsync(cancellationToken);
}
catch
{
await transaction.RollbackAsync(cancellationToken);
throw;
}
}Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs:
Line 148 to 161:
Sync-over-async blocking I/O occurs because synchronous transaction calls (`db.BeginTransaction()`, `transaction.Commit()`, `transaction.Rollback()`) execute inside async methods, risking thread-pool starvation under concurrent load. Apply the async variants (`await db.BeginTransactionAsync(cancellationToken)`, etc.) to align with the established pattern in `UserProfilesRepository.cs:343-346` across all four transaction blocks (lines 148, 171, 209, 232).
Suggested Code:
using (var transaction = await db.BeginTransactionAsync(cancellationToken))
{
try
{
await db.ExecuteAsync(new Dapper.CommandDefinition(@"DELETE FROM scheduledtasklogs ...", new { userId = userId }, transaction: transaction, cancellationToken: cancellationToken));
await db.ExecuteAsync(new Dapper.CommandDefinition(@"DELETE FROM scheduledtasks WHERE userid = @userId", new { userId = userId }, transaction: transaction, cancellationToken: cancellationToken));
await transaction.CommitAsync(cancellationToken);
}
catch
{
await transaction.RollbackAsync(cancellationToken);
throw;
}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| await db.OpenAsync(cancellationToken); | ||
|
|
||
| // Logs and their tasks go together: never leave tasks whose logs are already gone. | ||
| using (var transaction = db.BeginTransaction()) |
There was a problem hiding this comment.
Synchronous database call: BeginTransaction() blocks the thread during the network round-trip to issue the BEGIN command inside an async method, reducing scalability. Replace it with await db.BeginTransactionAsync(cancellationToken) on both NpgsqlConnection and SqlConnection types to maintain fully asynchronous execution across lines 171, 209, and 232.
Kody rule violation: Use Awaitable Methods in Async Code
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs:
Line 148:
Synchronous database call: `BeginTransaction()` blocks the thread during the network round-trip to issue the BEGIN command inside an async method, reducing scalability. Replace it with `await db.BeginTransactionAsync(cancellationToken)` on both `NpgsqlConnection` and `SqlConnection` types to maintain fully asynchronous execution across lines 171, 209, and 232.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
|
||
| transaction.Commit(); | ||
| } | ||
| catch |
There was a problem hiding this comment.
Diagnostic context loss: the catch block in DeleteAllTasksForUserInDepartmentAsync performs a silent rethrow, making transaction failures for department-scoped deletes untraceable. Capture the exception and log it using Logging.LogError(ex, "Failed to delete tasks for user {UserId} in department {DepartmentId}", userId, departmentId) before rollback, applying this to lines 157, 180, and 241.
Kody rule violation: Avoid empty catch blocks
Prompt for LLM
File Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs:
Line 218:
Diagnostic context loss: the catch block in `DeleteAllTasksForUserInDepartmentAsync` performs a silent rethrow, making transaction failures for department-scoped deletes untraceable. Capture the exception and log it using `Logging.LogError(ex, "Failed to delete tasks for user {UserId} in department {DepartmentId}", userId, departmentId)` before rollback, applying this to lines 157, 180, and 241.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
|
|
||
| if (_logger != null) | ||
| _logger.Warning(message); |
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs`:
- Line 198: Replace Logging.LogWarning() in
DeleteAllTasksForUserInDepartmentAsync with the approved logging method that
matches this non-fatal skipped-delete event, using LogInfo() for the existing
message and preserving its context.
🪄 Autofix
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 Plus
Run ID: 9c021f88-c8ff-4bcd-848e-ecf3f2d3b22e
⛔ Files ignored due to path filters (1)
Tests/Resgrid.Tests/Mocks/MockScheduledTasksRepository.csis excluded by!**/Tests/**
📒 Files selected for processing (1)
Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs
| // which belong to the user across departments and must survive a single-department revoke. | ||
| if (departmentId <= 0) | ||
| { | ||
| Logging.LogWarning($"DeleteAllTasksForUserInDepartmentAsync called with non-positive departmentId {departmentId} for user {userId}; skipping delete."); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use an approved logging method.
Line 198 calls Logging.LogWarning(). Replace it with an approved logging method that matches this event.
As per coding guidelines, use LogException(), LogError(), LogInfo(), or LogDebug() for all logging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Repositories/Resgrid.Repositories.DataRepository/ScheduledTasksRepository.cs`
at line 198, Replace Logging.LogWarning() in
DeleteAllTasksForUserInDepartmentAsync with the approved logging method that
matches this non-fatal skipped-delete event, using LogInfo() for the existing
message and preserving its context.
Source: Coding guidelines
|
Approve |
3 participants
PR Description
This PR addresses several fixes and enhancements, primarily focused on API CORS handling (RG-T117), along with Redis cache resilience improvements, department deletion cleanup, and minor API/task hardening.
CORS Enhancements (
CorsHelper.cs,ApiConfig.cs)*.resgrid.com, which match the apex domain and all subdomains on any scheme/port. This replaces the need to list individual subdomains.app://.are now matched verbatim against config entries before URI parsing, since these custom-scheme origins cannot be reliably parsed as absolute URIs. This unblocks Electron-based desktop clients.Redis Cache Resilience (
AzureRedisCacheProvider.cs)RedisConnectionExceptionacross all cache operations (retrieve, set, remove, increment — both sync and async), so transient connection drops (idle resets, failovers) gracefully fall back to the source data instead of throwing.Department Deletion Cleanup (
DeleteRepository.cs)DepartmentCallPruningto the cascade of tables deleted when a department is removed, preventing orphaned pruning configuration rows.Call Prune Task Hardening (
CallPruneTask.cs)Chat API Enhancement (
ChatController.cs)callIdparameter to theGetChannelsv4 endpoint, allowing callers (e.g., an incident view) to retrieve only channels attached to a specific call instead of the full department list.Tests (
CorsHelperTests.cs)Summary by CodeRabbit
New Features
Bug Fixes