chore(warnings): resolve 25 analyzer quick wins and fix RAPS bugs#261
Open
rlorenzo wants to merge 8 commits into
Open
chore(warnings): resolve 25 analyzer quick wins and fix RAPS bugs#261rlorenzo wants to merge 8 commits into
rlorenzo wants to merge 8 commits into
Conversation
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #261 +/- ##
==========================================
+ Coverage 45.39% 45.59% +0.20%
==========================================
Files 916 916
Lines 52784 52810 +26
Branches 5005 5012 +7
==========================================
+ Hits 23960 24081 +121
+ Misses 28196 28096 -100
- Partials 628 633 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Contributor
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 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 `@web/Areas/Directory/Controllers/DirectoryController.cs`:
- Line 35: Update the Directory home action route in DirectoryController to
preserve the trailing slash, using an area-root route that resolves to
/Directory/; alternatively, add a redirect from /Directory to /Directory/.
Ensure relative "nav" requests continue resolving under the Directory path.
In `@web/Areas/RAPS/Controllers/RAPSController.cs`:
- Around line 94-99: Update the read-only queries in Nav, specifically the
TblRoles, TblPermissions, and VwAaudUser lookups, to use AsNoTracking() before
execution while preserving their existing filters and null-handling behavior.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: b12c4003-6bfe-4fe7-9c83-98cd8619e44c
📒 Files selected for processing (9)
.editorconfigtest/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cstest/RAPS/AdGroupsControllerTests.csweb/Areas/CMS/Controllers/CMSController.csweb/Areas/CTS/Controllers/CTSController.csweb/Areas/Directory/Controllers/DirectoryController.csweb/Areas/RAPS/Controllers/AdGroupsController.csweb/Areas/RAPS/Controllers/RAPSController.csweb/Areas/RAPS/Models/GroupAddEdit.cs
rlorenzo
force-pushed
the
chore/warning-quick-wins
branch
from
July 17, 2026 07:26
0375aea to
bd4a19f
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR is the first tranche of the .NET analyzer-warning cleanup plan for VIPER’s ASP.NET backend, combining low-risk warning fixes/suppressions with a few behavior-correctness fixes discovered during the cleanup (notably in RAPS and one API bug).
Changes:
- Fixes several real behavior bugs (e.g., AdGroups PUT ID mismatch fallthrough; RAPS
SingleAsync->SingleOrDefaultAsync; RAPS action pipeline double-execution; request-scoped DbContext capture in background sync). - Adds ModelState guards to multiple RAPS actions and uses targeted analyzer suppressions/
[NonAction]where appropriate. - Refactors a test constructor to reduce complexity and adds a regression test for the AdGroups update bug.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/Areas/RAPS/Models/GroupAddEdit.cs | Makes GroupId nullable to better represent “new group” semantics and satisfy analyzer guidance. |
| web/Areas/RAPS/Controllers/RAPSController.cs | Fixes filter pipeline double-execution, hardens nav member lookup, and moves OU sync to a background scope. |
| web/Areas/RAPS/Controllers/AdGroupsController.cs | Fixes missing return on mismatched PUT ID to prevent unintended updates. |
| web/Areas/Directory/Controllers/DirectoryController.cs | Consolidates route prefix at controller level while keeping endpoints consistent. |
| web/Areas/CTS/Controllers/CTSController.cs | Adds targeted analyzer suppression for filter override. |
| web/Areas/CMS/Controllers/CMSController.cs | Uses ILoggerFactory to create a Data.CMS-category logger without triggering the analyzer. |
| test/RAPS/AdGroupsControllerTests.cs | Adds regression coverage for the mismatched group ID PUT bug. |
| test/ClinicalScheduler/Integration/ServiceLayerIntegrationTest.cs | Extracts mock factories to reduce constructor complexity. |
| .editorconfig | Adds/extends justified suppressions for generated contexts and non-bound model types. |
rlorenzo
force-pushed
the
chore/warning-quick-wins
branch
from
July 17, 2026 07:51
bd4a19f to
9c1af27
Compare
- suppress CA1505 for scaffolded EF contexts and S6964 for ApiPagination, which is never model-bound (editorconfig precedent) - create the Data.CMS logger via ILoggerFactory so the logger category matches its consumer (S6672) - move the Directory route prefix to the controller, URLs unchanged (S6931) - return 400 from RAPS view actions when model binding fails instead of proceeding with defaulted params (S6967) - suppress S6967/S6932 on the RAPS and CTS nav filter overrides with justification; mark RAPSController.Nav [NonAction] to remove an unintended endpoint - make GroupAddEdit.GroupId nullable to reflect create semantics (S6964)
Splits the two large NSubstitute fakes out of ServiceLayerIntegrationTest constructor (CA1502); behavior unchanged.
The BadRequest result was discarded, so a PUT whose body groupId did not match the route fell through and updated the group anyway. Add a regression test asserting the 400 and that the group is unchanged.
SingleAsync threw on every RAPS page when the memberId query param matched no user; the nav already handles the null case.
OnActionExecutionAsync awaited both the AreaController base, which already invokes next(), and next() again, so every RAPS action ran its pipeline twice.
The fire-and-forget sync captured the request-scoped RAPSContext, which is disposed once the response returns, faulting the background task. Resolve a fresh context from IServiceScopeFactory and log failures instead of leaving them unobserved.
Unit-locks the new 400-on-binding-failure behavior for all 8 guarded RAPS view actions and covers the permission lookup found/not-found paths plus the GroupSync no-op path (no background sync for unknown groups). Raises the PR patch coverage flagged by Codecov.
rlorenzo
force-pushed
the
chore/warning-quick-wins
branch
from
July 18, 2026 00:56
03058c4 to
d230b4e
Compare
- Closes the CA1502 warning on GetUCD; the shared query helper and the VMACS enrichment were duplicated verbatim across both actions - SQLite-backed tests pin the query helper (field matches, name-span match, current-user filter, ordering)
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+123
to
+130
| .Where(u => (u.DisplayFirstName + " " + u.DisplayLastName).Contains(search) | ||
| || (u.MailId != null && u.MailId.Contains(search)) | ||
| || (u.LoginId != null && u.LoginId.Contains(search)) | ||
| || (u.SpridenId != null && u.SpridenId.Contains(search)) | ||
| || (u.Pidm != null && u.Pidm.Contains(search)) | ||
| || (u.MothraId != null && u.MothraId.Contains(search)) | ||
| || (u.EmployeeId != null && u.EmployeeId.Contains(search)) | ||
| || (u.IamId != null && u.IamId.Contains(search))) |
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
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
First branch of the .NET warnings cleanup plan: closes 26 of 44 code-analysis warnings via low-risk fixes and justified suppressions, plus four real bugs found along the way (three in RAPS, one latent test hazard). Seven single-purpose commits.
Warning cleanup (26 closed)
[NonAction]onRAPSController.Navremoves an unintended endpoint.editorconfigsuppression forApiPagination(never model-bound, always constructed server-side);GroupAddEdit.GroupIdmade nullable.editorconfigsuppression for scaffolded EF*Context.cs, following the existing S3251 precedentILoggerFactoryforData.CMS; log category unchanged[Route]; all 5 URLs provably unchangedServiceLayerIntegrationTestGetUCD)GetandGetUCD, extracted into shared helpers; SQLite-backed tests pin the query helper (field matches, name-span match, current-user filter, ordering)Bug fixes (behavior changes to note)
AdGroupsController.UpdateGroup: missingreturnmeant a PUT with a mismatched group ID fell through and updated the group anyway; now 400. Regression test added (the group-edit UI always sends a matching ID, so normal use is unaffected).SingleAsync500ed every RAPS page when?memberId=matched no user; nowSingleOrDefaultAsyncwith the existing null handling.OnActionExecutionAsyncawaited both theAreaControllerbase (which already invokesnext()) andnext()again.GroupSyncbackground sync: fire-and-forget task captured the request-scopedRAPSContext(disposed when the response returns); now resolves a fresh context fromIServiceScopeFactoryand logs failures.Verification
verify:build: all 26 targeted warnings gone, no new ones; the 18 remaining warnings match the plan's deferred list exactly?roleId=abc400, nonexistent?memberId=200), mismatched-ID group PUT 400, all 5 Directory endpoints, CTS index