Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughThis PR introduces a paged audit query layer to replace event-based enqueueing, adds an inventory-by-unit feature with unit population, hardens audit log persistence, and updates IP resolution and datetime handling in shift calendar generation. ChangesAudit Query Layer Infrastructure
Inventory By-Unit Feature
Infrastructure Updates
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Web/Resgrid.Web/Areas/User/Controllers/InventoryController.cs (1)
300-342: ⚡ Quick winConsider extracting common inventory JSON population logic.
GetInventoryByUnitList(lines 300-342) duplicates the JSON population loop fromGetInventoryList(lines 252-292). Both methods fetch department/items/names, loop over inventories, and populateInventoryJsonwith identical null-check fallbacks. Only the filtering (line 310) and ordering (lines 311-312) differ. Extracting a helper method (e.g.,MapInventoryToJson(Inventory item, Department department, IEnumerable<PersonName> names)) would reduce duplication and improve maintainability.♻️ Example refactor with helper method
+private InventoryJson MapInventoryToJson(Inventory item, Department department, IEnumerable<PersonName> names) +{ + var inventory = new InventoryJson + { + InventoryId = item.InventoryId, + Type = item.Type.Type, + Amount = item.Amount, + Batch = item.Batch, + Timestamp = item.TimeStamp.FormatForDepartment(department), + Unit = item.Unit != null ? item.Unit.Name : _localizer["NoUnit"], + Group = item.Group != null ? item.Group.Name : _localizer["NoGroup"] + }; + + var name = names.FirstOrDefault(x => x.UserId == item.AddedByUserId); + inventory.UserName = name != null ? name.Name : _commonLocalizer["Unknown"]; + + return inventory; +} public async Task<IActionResult> GetInventoryList() { // ... foreach (var item in items) { - var inventory = new InventoryJson(); - inventory.InventoryId = item.InventoryId; - // ... (lines 264-286) + inventoryJson.Add(MapInventoryToJson(item, department, names)); } // ... } public async Task<IActionResult> GetInventoryByUnitList() { // ... foreach (var item in items.Where(...).OrderBy(...).ThenBy(...)) { - var inventory = new InventoryJson(); - inventory.InventoryId = item.InventoryId; - // ... (lines 314-336) + inventoryJson.Add(MapInventoryToJson(item, department, names)); } // ... }🤖 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/InventoryController.cs` around lines 300 - 342, GetInventoryByUnitList duplicates the InventoryJson population loop from GetInventoryList; extract a private helper like MapInventoryToJson(Inventory item, Department department, IEnumerable<PersonName> names) that builds and returns an InventoryJson (handling null checks for item.Unit, item.Group, lookup of AddedByUserId and timestamp formatting), then replace the loops in both GetInventoryByUnitList and GetInventoryList to call this helper after applying their specific filtering/ordering so the mapping logic is centralized and maintainability is improved.Core/Resgrid.Services/InventoryService.cs (1)
69-84: ⚡ Quick winInconsistent prefetch pattern for groups.
The method now prefetches units once (line 73) and uses them in the loop (line 80), which avoids N+1 queries. However, groups are still fetched individually inside the loop (line 77), creating N sequential async calls. Consider prefetching groups using
GetAllGroupsForDepartmentAsync(or equivalent) and performing in-memory lookups, consistent with the new unit-loading pattern.♻️ Proposed refactor to prefetch groups
public async Task<List<Inventory>> GetAllTransactionsForDepartmentAsync(int departmentId) { var inventories = await _inventoryRepository.GetAllInventoriesByDepartmentIdAsync(departmentId); var units = await _unitsService.GetUnitsForDepartmentAsync(departmentId); + var groups = await _departmentGroupsService.GetAllGroupsForDepartmentAsync(departmentId); foreach (var inventory in inventories) { - inventory.Group = await _departmentGroupsService.GetGroupByIdAsync(inventory.GroupId); + inventory.Group = groups?.FirstOrDefault(x => x.DepartmentGroupId == inventory.GroupId); if (inventory.UnitId.HasValue && inventory.UnitId.Value > 0) inventory.Unit = units?.FirstOrDefault(x => x.UnitId == inventory.UnitId.Value); } return inventories.ToList(); }🤖 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/InventoryService.cs` around lines 69 - 84, GetAllTransactionsForDepartmentAsync currently causes N+1 calls by calling _departmentGroupsService.GetGroupByIdAsync inside the loop; instead call the bulk loader (e.g. _departmentGroupsService.GetAllGroupsForDepartmentAsync or equivalent) once alongside _unitsService.GetUnitsForDepartmentAsync, build an in-memory lookup (dictionary keyed by GroupId), then in the foreach assign inventory.Group = groupLookup[inventory.GroupId] (or TryGetValue) and remove the per-iteration GetGroupByIdAsync call so groups are pre-fetched similarly to units.
🤖 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/AuditService.cs`:
- Around line 40-44: GetAuditLogsForDepartmentPagedAsync should validate/clamp
the incoming paging parameters before calling the repository: ensure page is at
least 1 (if <=0 set to 1) and ensure pageSize is a positive value (e.g., set to
a min of 1 and optionally clamp to a sensible max like 1000) to avoid generating
negative OFFSET or invalid LIMIT/FETCH; update
GetAuditLogsForDepartmentPagedAsync to compute safePage and safePageSize and
pass those to
_auditLogsRepository.GetAuditLogsForDepartmentPagedAsync(departmentId,
startDate, endDate, (int?)logType, safePage, safePageSize).
In `@Repositories/Resgrid.Repositories.DataRepository/AuditLogsRepository.cs`:
- Around line 42-43: The offset computation can go negative when page <= 0;
update validation to clamp/validate page and pageSize at the service entry
(AuditService.GetAuditLogsForDepartmentPagedAsync) so page >= 1 and pageSize >
0, or alternatively ensure the repository computes Offset as Math.Max(0, (page -
1) * pageSize) before adding dynamicParameters ("Offset") in AuditLogsRepository
(and mirror the fix in SystemAuditsRepository.GetByUserIdPagedAsync and
GetByDepartmentIdPagedAsync) so SQL OFFSET never receives a negative value.
In `@Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs`:
- Around line 91-93: The call to IpAddressHelper.GetRequestIP(Request, true) in
the ContactVerificationController action can throw and crash the request; wrap
the call in a safe guard (try/catch) around
IpAddressHelper.GetRequestIP(Request, true), handle any exception by logging a
warning and falling back to a safe default (e.g., null or empty string) so
verification proceeds even if IP resolution fails, and ensure any downstream use
of the ipAddress variable (in this controller action) tolerates the fallback
value.
---
Nitpick comments:
In `@Core/Resgrid.Services/InventoryService.cs`:
- Around line 69-84: GetAllTransactionsForDepartmentAsync currently causes N+1
calls by calling _departmentGroupsService.GetGroupByIdAsync inside the loop;
instead call the bulk loader (e.g.
_departmentGroupsService.GetAllGroupsForDepartmentAsync or equivalent) once
alongside _unitsService.GetUnitsForDepartmentAsync, build an in-memory lookup
(dictionary keyed by GroupId), then in the foreach assign inventory.Group =
groupLookup[inventory.GroupId] (or TryGetValue) and remove the per-iteration
GetGroupByIdAsync call so groups are pre-fetched similarly to units.
In `@Web/Resgrid.Web/Areas/User/Controllers/InventoryController.cs`:
- Around line 300-342: GetInventoryByUnitList duplicates the InventoryJson
population loop from GetInventoryList; extract a private helper like
MapInventoryToJson(Inventory item, Department department,
IEnumerable<PersonName> names) that builds and returns an InventoryJson
(handling null checks for item.Unit, item.Group, lookup of AddedByUserId and
timestamp formatting), then replace the loops in both GetInventoryByUnitList and
GetInventoryList to call this helper after applying their specific
filtering/ordering so the mapping logic is centralized and maintainability is
improved.
🪄 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: 68672df5-550b-4122-ab92-56efac642120
⛔ Files ignored due to path filters (9)
Core/Resgrid.Localization/Areas/User/Inventory/Inventory.ar.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Inventory/Inventory.de.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Inventory/Inventory.en.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Inventory/Inventory.es.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Inventory/Inventory.fr.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Inventory/Inventory.it.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Inventory/Inventory.pl.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Inventory/Inventory.sv.resxis excluded by!**/*.resxCore/Resgrid.Localization/Areas/User/Inventory/Inventory.uk.resxis excluded by!**/*.resx
📒 Files selected for processing (29)
Core/Resgrid.Model/Providers/IAuditEventProvider.csCore/Resgrid.Model/Repositories/IAuditLogsRepository.csCore/Resgrid.Model/Repositories/IPaymentProviderEventsRepository.csCore/Resgrid.Model/Repositories/ISystemAuditsRepository.csCore/Resgrid.Model/Services/IAuditEventService.csCore/Resgrid.Model/Services/IAuditService.csCore/Resgrid.Services/AuditEventService.csCore/Resgrid.Services/AuditService.csCore/Resgrid.Services/InventoryService.csProviders/Resgrid.Providers.Bus/AuditEventProvider.csRepositories/Resgrid.Repositories.DataRepository/AuditLogsRepository.csRepositories/Resgrid.Repositories.DataRepository/PaymentProviderEventsRepository.csRepositories/Resgrid.Repositories.DataRepository/Queries/AuditLogs/SelectAuditLogsForDepartmentByTypePagedQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/AuditLogs/SelectAuditLogsForDepartmentPagedQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/Payments/SelectPaymentProviderEventsByCustomerIdQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/SystemAudits/SelectSystemAuditsByDepartmentIdPagedQuery.csRepositories/Resgrid.Repositories.DataRepository/Queries/SystemAudits/SelectSystemAuditsByUserIdPagedQuery.csRepositories/Resgrid.Repositories.DataRepository/SystemAuditRepository.csWeb/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.csWeb/Resgrid.Web/Areas/User/Controllers/HomeController.csWeb/Resgrid.Web/Areas/User/Controllers/InventoryController.csWeb/Resgrid.Web/Areas/User/Controllers/ShiftsController.csWeb/Resgrid.Web/Areas/User/Views/Inventory/ByUnit.cshtmlWeb/Resgrid.Web/Areas/User/Views/Inventory/History.cshtmlWeb/Resgrid.Web/Areas/User/Views/Inventory/Index.cshtmlWeb/Resgrid.Web/wwwroot/_references.jsWeb/Resgrid.Web/wwwroot/js/app/internal/inventory/resgrid.inventory.byunit.jsWeb/Resgrid.Web/wwwroot/js/app/internal/inventory/resgrid.inventory.history.jsWorkers/Resgrid.Workers.Framework/Logic/AuditQueueLogic.cs
💤 Files with no reviewable changes (4)
- Core/Resgrid.Model/Services/IAuditEventService.cs
- Core/Resgrid.Model/Providers/IAuditEventProvider.cs
- Core/Resgrid.Services/AuditEventService.cs
- Providers/Resgrid.Providers.Bus/AuditEventProvider.cs
Comment on lines
+40
to
+44
| public async Task<List<AuditLog>> GetAuditLogsForDepartmentPagedAsync(int departmentId, DateTime startDate, DateTime endDate, AuditLogTypes? logType, int page, int pageSize) | ||
| { | ||
| var logs = await _auditLogsRepository.GetAuditLogsForDepartmentPagedAsync(departmentId, startDate, endDate, (int?)logType, page, pageSize); | ||
| return logs.ToList(); | ||
| } |
Contributor
There was a problem hiding this comment.
Validate/clamp page and pageSize before delegating.
This is the single entry point feeding the paged repository methods. Without a guard, page <= 0 propagates a negative Offset to the SQL builders and fails at execution on both SQL Server and PostgreSQL. Likewise a non-positive pageSize produces an invalid LIMIT/FETCH NEXT.
🛡️ Suggested guard
public async Task<List<AuditLog>> GetAuditLogsForDepartmentPagedAsync(int departmentId, DateTime startDate, DateTime endDate, AuditLogTypes? logType, int page, int pageSize)
{
+ if (page < 1) page = 1;
+ if (pageSize < 1) pageSize = 1;
var logs = await _auditLogsRepository.GetAuditLogsForDepartmentPagedAsync(departmentId, startDate, endDate, (int?)logType, page, pageSize);
return logs.ToList();
}🤖 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/AuditService.cs` around lines 40 - 44,
GetAuditLogsForDepartmentPagedAsync should validate/clamp the incoming paging
parameters before calling the repository: ensure page is at least 1 (if <=0 set
to 1) and ensure pageSize is a positive value (e.g., set to a min of 1 and
optionally clamp to a sensible max like 1000) to avoid generating negative
OFFSET or invalid LIMIT/FETCH; update GetAuditLogsForDepartmentPagedAsync to
compute safePage and safePageSize and pass those to
_auditLogsRepository.GetAuditLogsForDepartmentPagedAsync(departmentId,
startDate, endDate, (int?)logType, safePage, safePageSize).
Comment on lines
+42
to
+43
| dynamicParameters.Add("Offset", (page - 1) * pageSize); | ||
| dynamicParameters.Add("PageSize", pageSize); |
Contributor
There was a problem hiding this comment.
Guard against negative Offset for non-positive page.
(page - 1) * pageSize yields a negative offset when page <= 0, which throws on both SQL Server (OFFSET ... ROWS) and PostgreSQL (OFFSET). The same computation is repeated in SystemAuditsRepository.GetByUserIdPagedAsync/GetByDepartmentIdPagedAsync. Best handled once at the service entry point (AuditService.GetAuditLogsForDepartmentPagedAsync) by clamping/validating page and pageSize; flagging the root computation here.
🤖 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/AuditLogsRepository.cs`
around lines 42 - 43, The offset computation can go negative when page <= 0;
update validation to clamp/validate page and pageSize at the service entry
(AuditService.GetAuditLogsForDepartmentPagedAsync) so page >= 1 and pageSize >
0, or alternatively ensure the repository computes Offset as Math.Max(0, (page -
1) * pageSize) before adding dynamicParameters ("Offset") in AuditLogsRepository
(and mirror the fix in SystemAuditsRepository.GetByUserIdPagedAsync and
GetByDepartmentIdPagedAsync) so SQL OFFSET never receives a negative value.
Comment on lines
+91
to
+93
| // Use the X-Forwarded-For aware helper so the audit log records the real client IP | ||
| // rather than the reverse-proxy / load-balancer address. | ||
| string ipAddress = IpAddressHelper.GetRequestIP(Request, true); |
Contributor
There was a problem hiding this comment.
Potential 500 from unguarded IP resolution
IpAddressHelper.GetRequestIP(Request, true) can throw when request metadata is missing, and this action does not handle that path. A failed IP lookup should not fail code confirmation.
Suggested hardening
- string ipAddress = IpAddressHelper.GetRequestIP(Request, true);
+ string ipAddress;
+ try
+ {
+ ipAddress = IpAddressHelper.GetRequestIP(Request, true);
+ }
+ catch
+ {
+ ipAddress = HttpContext?.Connection?.RemoteIpAddress?.ToString();
+ }📝 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.
Suggested change
| // Use the X-Forwarded-For aware helper so the audit log records the real client IP | |
| // rather than the reverse-proxy / load-balancer address. | |
| string ipAddress = IpAddressHelper.GetRequestIP(Request, true); | |
| // Use the X-Forwarded-For aware helper so the audit log records the real client IP | |
| // rather than the reverse-proxy / load-balancer address. | |
| string ipAddress; | |
| try | |
| { | |
| ipAddress = IpAddressHelper.GetRequestIP(Request, true); | |
| } | |
| catch (Exception ex) | |
| { | |
| Logging.LogException(ex, "Failed to resolve request IP, falling back to RemoteIpAddress"); | |
| ipAddress = HttpContext?.Connection?.RemoteIpAddress?.ToString(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Web/Resgrid.Web.Services/Controllers/v4/ContactVerificationController.cs`
around lines 91 - 93, The call to IpAddressHelper.GetRequestIP(Request, true) in
the ContactVerificationController action can throw and crash the request; wrap
the call in a safe guard (try/catch) around
IpAddressHelper.GetRequestIP(Request, true), handle any exception by logging a
warning and falling back to a safe default (e.g., null or empty string) so
verification proceeds even if IP resolution fails, and ensure any downstream use
of the ipAddress variable (in this controller action) tolerates the fallback
value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Refactor