Skip to content

CodeQL 7: refactor: tighten foreach-to-LINQ where it improves the call site#195

Open
rlorenzo wants to merge 1 commit into
mainfrom
codeql/7-effort-linq
Open

CodeQL 7: refactor: tighten foreach-to-LINQ where it improves the call site#195
rlorenzo wants to merge 1 commit into
mainfrom
codeql/7-effort-linq

Conversation

@rlorenzo
Copy link
Copy Markdown
Contributor

@rlorenzo rlorenzo commented May 13, 2026

Summary

Closes 6 of 34 cs/linq/missed-* alerts where the foreach-to-LINQ conversion makes the intent clearer:

  • UserHelper.IsInRole - claim-search loop → claims.Any(c => c.Type == ClaimTypes.Role && c.Value == roleName) (also collapses the surrounding null guard).
  • CrestCourseService.CourseSessionOfferingsToCourses / …Sessions - staging-list + foreach-Add pattern → csos.GroupBy(...).Select(g => new Course/Session(...)).ToList().
  • AssessmentController.GetAssessmentsForStudent - build-and-mutate loop → assessmentsList.Select(a => { … }).ToList().
  • RoleTemplatesController.Apply - foreach (... in preview.Roles) { if (!UserHasRole) … }foreach (... in preview.Roles.Where(r => !r.UserHasRole)).
  • RoleViews delete loop - compound if filter hoisted into the foreach … in roleMembers.Where(...).

Also fixed two stray tab-indented lines in RoleViews.cs that dotnet format flagged.

Why 28 alerts are not addressed

The remaining 28 cs/linq/missed-select alerts all live in PDF/Excel cell-generation loops in the Effort report services - TeachingActivityService, MeritReportService, SchoolSummaryService, DeptSummaryService, MeritMultiYearService, EvaluationReportService, MeritSummaryService. Pattern is:

foreach (var type in orderedTypes)
{
    var val = course.EffortByType.GetValueOrDefault(type, 0);
    table.Cell().PaddingVertical(cellPadV).Text(val > 0 ? val.ToString() : "0");
}

The var val = … is what CodeQL identifies as a "mapping", but the surrounding body is QuestPDF.table.Cell()… / ClosedXML.ws.Cell()… side effects, not collection-building. Forcing a Select here would just add a tuple-deconstruction foreach over a .Select(t => (Type: t, Val: …)) with the same side-effectful body - more code, same behavior, lower readability. These are dismissible as wontfix-by-design on the CodeQL dashboard.

Context

Seventh in the CodeQL N: cleanup series (after #189, #190, #191, #192, #193, #194).

Test plan

  • npm run test:backend - 1946 tests passing
  • npm run verify:build - clean (0 errors)
  • Pre-commit lint+test+verify all passed
  • CodeQL workflow on this PR shows 6 of the 34 LINQ alerts closed

@codecov-commenter
Copy link
Copy Markdown

Bundle Report

Bundle size has no change ✅

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 13, 2026

Codecov Report

❌ Patch coverage is 10.00000% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.03%. Comparing base (ed1f48b) to head (071a111).

Files with missing lines Patch % Lines
web/Areas/CTS/Services/CrestCourseService.cs 0.00% 18 Missing ⚠️
web/Areas/RAPS/Services/RoleViews.cs 0.00% 6 Missing ⚠️
.../Areas/RAPS/Controllers/RoleTemplatesController.cs 0.00% 2 Missing ⚠️
web/Classes/UserHelper.cs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #195   +/-   ##
=======================================
  Coverage   43.02%   43.03%           
=======================================
  Files         881      881           
  Lines       51437    51421   -16     
  Branches     4812     4804    -8     
=======================================
- Hits        22131    22129    -2     
+ Misses      28780    28766   -14     
  Partials      526      526           
Flag Coverage Δ
backend 43.11% <10.00%> (+<0.01%) ⬆️
frontend 41.47% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 13, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 379e3cea-ceda-47ec-8fec-db539241c5dc

📥 Commits

Reviewing files that changed from the base of the PR and between 38de1ad and 8916163.

📒 Files selected for processing (5)
  • web/Areas/CTS/Controllers/AssessmentController.cs
  • web/Areas/CTS/Services/CrestCourseService.cs
  • web/Areas/RAPS/Controllers/RoleTemplatesController.cs
  • web/Areas/RAPS/Services/RoleViews.cs
  • web/Classes/UserHelper.cs

📝 Walkthrough

Walkthrough

Five methods across role management, course/session transformations, and assessment processing were rewritten from imperative foreach/if patterns to LINQ expressions (Select, GroupBy, Where, Any) with no public API or behavioral changes.

Changes

Refactoring to LINQ Expressions

Layer / File(s) Summary
Assessment transformation
web/Areas/CTS/Controllers/AssessmentController.cs
GetAssessments replaces foreach construction with Select(...) projection that builds StudentAssessment objects and sets Editable flag from ctsSecurityService.CanEditStudentAssessment(...).
Course and session grouping
web/Areas/CTS/Services/CrestCourseService.cs
CourseSessionOfferingsToCourses and CourseSessionOfferingsToSessions convert nested offerings into course/session hierarchies via GroupBy(...).Select(...).ToList() instead of manual loops.
Role application filtering
web/Areas/RAPS/Controllers/RoleTemplatesController.cs
RoleTemplateApply now pre-filters preview.Roles.Where(r => !r.UserHasRole) before iterating and calling AddMemberToRole.
Role member removal refactoring
web/Areas/RAPS/Services/RoleViews.cs
UpdateRole iterates roleMembers.Where(...) for removals that match the prior predicate; minor whitespace/indent fixes in GetViewMembers and class closing brace.
Role membership claim check
web/Classes/UserHelper.cs
IsInRole now uses claims?.Any(...) == true to test for a matching ClaimTypes.Role value instead of a null-checked foreach loop; fallback GetRoles unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: refactoring foreach loops to LINQ in targeted locations to improve code clarity and close CodeQL alerts.
Description check ✅ Passed The description is directly related to the changeset, detailing which files were modified and why, along with clear rationale for what was not changed and why.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codeql/7-effort-linq

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rlorenzo
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@rlorenzo
Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

✅ Actions performed

Full review triggered.

@rlorenzo
Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR continues the CodeQL cleanup series by replacing selected C# foreach patterns with LINQ where the resulting code keeps behavior equivalent and clarifies intent.

Changes:

  • Replaced filtering loops with Any, Where, and Select at targeted call sites.
  • Converted course/session grouping construction to LINQ projections.
  • Cleaned tab indentation in RoleViews.cs.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
web/Classes/UserHelper.cs Simplifies role-claim lookup with Any.
web/Areas/RAPS/Services/RoleViews.cs Hoists delete filtering into Where and fixes indentation.
web/Areas/RAPS/Controllers/RoleTemplatesController.cs Filters preview roles before applying missing roles.
web/Areas/CTS/Services/CrestCourseService.cs Converts grouped course/session object creation to LINQ projections.
web/Areas/CTS/Controllers/AssessmentController.cs Converts assessment DTO mapping loop to Select.

@rlorenzo rlorenzo force-pushed the codeql/7-effort-linq branch from 533a616 to 071a111 Compare May 26, 2026 17:38
@rlorenzo rlorenzo requested a review from bsedwards May 26, 2026 18:39
@rlorenzo
Copy link
Copy Markdown
Contributor Author

@bsedwards Merged to TEST.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants