PR 6: refactor: per-area small refactors (CTS, Scheduler, Students, CMS)#179
Open
rlorenzo wants to merge 6 commits into
Open
PR 6: refactor: per-area small refactors (CTS, Scheduler, Students, CMS)#179rlorenzo wants to merge 6 commits into
rlorenzo wants to merge 6 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds IScheduleEntity and ApplyScheduleFilters; refactors Codecs to parameterized EncodeWithMap/DecodeWithMap and adds UU tests; consolidates StudentGroupService photo/Ross IAM logic; converts CourseStudents.vue table to data-driven rendering. ChangesSchedule Filtering Abstraction
Codec Parameterization and Testing
CourseStudents Table Data-Driven Rendering
StudentGroupService Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
25b942b to
d0b2e7f
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
ebe32ec to
75abeb2
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
d0b2e7f to
0ea0117
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
5 times, most recently
from
d26c5d8 to
8cbfb69
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
0ea0117 to
0fa6909
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
2 times, most recently
from
0d62a4e to
a0c55c4
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
0fa6909 to
5cbd5f4
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
a0c55c4 to
d8a3735
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
5cbd5f4 to
8eb247a
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
d8a3735 to
ddd64e7
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
8eb247a to
a461dcd
Compare
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
ddd64e7 to
66fe537
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
a461dcd to
6d0722d
Compare
3 tasks
rlorenzo
force-pushed
the
refactor/dead-code-and-shared-chrome
branch
from
66fe537 to
d09da71
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
6d0722d to
9c30e16
Compare
Bundle ReportChanges will decrease total bundle size by 881 bytes (-0.04%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: viper-frontend-esmAssets Changed:
Files in
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #179 +/- ##
==========================================
+ Coverage 43.02% 44.19% +1.17%
==========================================
Files 881 893 +12
Lines 51436 51491 +55
Branches 4812 4791 -21
==========================================
+ Hits 22131 22758 +627
+ Misses 28779 28172 -607
- Partials 526 561 +35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
9c30e16 to
743f4e5
Compare
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
743f4e5 to
b8152a2
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/Areas/Students/Services/StudentGroupService.cs (1)
407-433: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd
.AsNoTracking()to read-only query.Same issue. Insert
.AsNoTracking()before.OrderBy().⚡ Suggested fix
sg != null ? sg.StudentgrpV3grp : null); // Apply group filtering if specified if (!string.IsNullOrEmpty(groupType) && !string.IsNullOrEmpty(groupId)) { query = groupType.ToLower() switch { "eighths" => query.Where(s => s.EighthsGroup == groupId), "twentieths" => query.Where(s => s.TwentiethsGroup == groupId), "teams" => query.Where(s => s.TeamNumber == groupId), "v3specialty" => query.Where(s => s.V3SpecialtyGroup == groupId), _ => query }; } -var students = await query.OrderBy(s => s.LastName).ThenBy(s => s.FirstName).ToListAsync(); +var students = await query.AsNoTracking().OrderBy(s => s.LastName).ThenBy(s => s.FirstName).ToListAsync();🤖 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/Areas/Students/Services/StudentGroupService.cs` around lines 407 - 433, The LINQ query building a sequence of StudentBaseRecord is read-only and should use AsNoTracking to avoid EF change tracking overhead; update the code that constructs "query" (the projection creating new StudentBaseRecord in StudentGroupService) to call .AsNoTracking() on the IQueryable before applying .OrderBy(...).ThenBy(...)/ToListAsync(), preserving any groupType filtering logic (the switch) so call .AsNoTracking() on the final query instance just prior to ordering and materialization.
🤖 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 `@test/CMS/CodecsTests.cs`:
- Line 4: The namespace declared as Test.CMS doesn't match the project's
expected root namespace; update the namespace declaration to Viper.test.CMS
(replace the Test.CMS namespace token) so the class/test types in this file use
the Viper.test.CMS namespace and satisfy the ReSharper/CI checks.
In `@web/Areas/Students/Services/StudentGroupService.cs`:
- Around line 765-772: The query against _sisContext.StudentDesignations in
StudentGroupService should be executed as a read-only query; insert
.AsNoTracking() immediately after _sisContext.StudentDesignations (before the
existing .Where(...) calls) so the EF Core change tracker is not used for this
read-only projection (retain the rest of the chain:
.Where(...).Select(...).Where(...).Distinct().ToListAsync()).
- Line 738: Remove the null-forgiving operator from student.MailId in
StudentGroupService.cs and handle nullable MailId properly: either filter the
students iteration to only include those with MailId != null before mapping, or
assign MailId = student.MailId (no !) and ensure the consumer (e.g., the
StudentPhoto model or downstream code) accepts a nullable MailId; locate the
mapping where MailId = student.MailId! and replace it with a safe nullable-aware
approach consistent with how MailId is handled elsewhere (see other assignments
around lines with StudentPhoto.MailId).
- Around line 113-126: The LINQ query building StudentBaseRecord instances is
read-only but currently tracks entities; update the query used in
StudentGroupService (the variable named query that projects to
StudentBaseRecord) to call .AsNoTracking() before the OrderBy/ThenBy and
ToListAsync so EF Core doesn't enable change tracking for this read-only
projection; locate the projection creating new StudentBaseRecord and insert
.AsNoTracking() on that IQueryable prior to .OrderBy(s => s.LastName).ThenBy(s
=> s.FirstName).ToListAsync().
- Around line 341-354: The query projection creating StudentBaseRecord from
queryBase should be treated as read-only—modify the LINQ chain so that the
IQueryable returned by the Select (the variable named query) uses AsNoTracking()
before applying OrderBy/ThenBy; locate the Select(...) producing
StudentBaseRecord in StudentGroupService (the variable query) and insert
.AsNoTracking() on that query before calling OrderBy(s => s.LastName).ThenBy(s
=> s.FirstName).ToListAsync().
---
Outside diff comments:
In `@web/Areas/Students/Services/StudentGroupService.cs`:
- Around line 407-433: The LINQ query building a sequence of StudentBaseRecord
is read-only and should use AsNoTracking to avoid EF change tracking overhead;
update the code that constructs "query" (the projection creating new
StudentBaseRecord in StudentGroupService) to call .AsNoTracking() on the
IQueryable before applying .OrderBy(...).ThenBy(...)/ToListAsync(), preserving
any groupType filtering logic (the switch) so call .AsNoTracking() on the final
query instance just prior to ordering and materialization.
🪄 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: d79bf74a-c6b5-4d4d-84dc-7f186950c9ab
📒 Files selected for processing (10)
VueApp/src/CTS/pages/CourseStudents.vuetest/CMS/CodecsTests.csweb/Areas/CMS/Data/Codecs.csweb/Areas/ClinicalScheduler/Services/InstructorScheduleService.csweb/Areas/ClinicalScheduler/Services/ScheduleQueryExtensions.csweb/Areas/ClinicalScheduler/Services/StudentScheduleService.csweb/Areas/Students/Services/StudentGroupService.csweb/Models/CTS/IScheduleEntity.csweb/Models/CTS/InstructorSchedule.csweb/Models/CTS/StudentSchedule.cs
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
d1b9d4c to
b06de1c
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
b06de1c to
77a1736
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@CodeRabbit review |
Contributor
Action performedReview triggered.
|
Photo-gallery queries order/filter on the source before projecting so EF Core can translate them, large Contains lists use EF.Parameter (OPENJSON), and the catch blocks rethrow with context instead of masking failures. Adds SQLite-backed regression tests.
rlorenzo
force-pushed
the
refactor/per-area-small
branch
from
b25beef to
83489ac
Compare
The course photo gallery threw "Cannot use multiple context instances within a single query execution" whenever Ross students were requested and any were active for the term: the Ross-in-course lookup joined the Courses and AAUD DbContexts in a single query, which EF cannot translate. Before the gallery fix this surfaced as a silently empty gallery (the exception was swallowed); after it, as a 500. Split it into the same two-step pattern the rest of the method uses (course-roster PIDMs from Courses, then the Ross IAM IDs from AAUD). Add a SQLite-backed regression test for the include-Ross course path, which reproduces the failure on the old query and passes on the new one.
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
VueApp/src/CTS/pages/CourseStudents.vue (1)
48-84:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReplace native table row/cell markup with Quasar table components in this changed block.
Line 48 introduces a
v-foron native<tr>/<td>, which violates the Vue guideline requiring Quasar components instead of HTML elements. Refactor this table rendering toq-table(or Quasar table primitives) so this section is compliant.As per coding guidelines, "
**/*.vue: Use Quasar components (q-btn,q-dialog,q-banner, etc.) instead of HTML elements."🤖 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 `@VueApp/src/CTS/pages/CourseStudents.vue` around lines 48 - 84, The native <tr>/<td> loop rendering loggedInStudents must be replaced with Quasar table primitives: use <q-table> with :rows="loggedInStudents" and a body slot (or use QTr/QTd components) instead of v-for on <tr>; map columns for name/time and render the action cells using the existing q-btn/q-icon inside QTd, keeping the :key as row.id and preserving the AssessmentCompetency route construction and aria-labels (refer to loggedInStudents, AssessmentCompetency route string, and the q-btn/q-icon instances) so the table uses Quasar components throughout.Source: Coding guidelines
🤖 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 `@test/Students/StudentGroupServiceQueryTests.cs`:
- Around line 200-236: Add a negative Ross fixture to the existing test
GetStudentsByCourseAsync_IncludeRoss_ReturnsRossStudentEnrolledInCourse: create
another SeedStudent + StudentDesignation pair (use a distinct pkey/pidm like
"P10"/"PIDM10" and matching IamId) and add a Roster entry for that pidm with
RosterCrn "12345" but set RosterEnrollStatus to a non-"RE" value (e.g. "WD" or
"DR"). Save to _sisContext and _coursesContext like the others, then keep the
existing call to _service.GetStudentsByCourseAsync(Term, "12345",
includeRossStudents: true) and add an assertion that this non-RE Ross student is
not present in result (e.g. assert no s with LastName matching the new seed or
that result.Count remains 2). Ensure you reference the existing SeedStudent,
StudentDesignation, Roster, _sisContext/_coursesContext and
GetStudentsByCourseAsync in the change.
In `@web/Areas/ClinicalScheduler/Services/ScheduleQueryExtensions.cs`:
- Around line 22-25: The current ScheduleQueryExtensions implementation uses
Week.WeekGradYears.Any(...) inside the Where, causing a correlated subquery;
instead preload the matching WeekIds from WeekGradYears (filter by GradYear and
Select WeekId/Distinct) and change the predicate to use
weekIds.Contains(s.WeekId). Add an overload on the
ApplyScheduleFilters/extension in ScheduleQueryExtensions that accepts an
IEnumerable<int> weekIds (or accept a nullable preloaded list) and, when
provided, apply query = query.Where(s => weekIds.Contains(s.WeekId)); keep the
original signature behavior when weekIds is null so callers that preload WeekIds
(from WeekGradYears where GradYear == classYear) can pass them in and avoid the
per-row Any().
In `@web/Areas/Students/Services/StudentGroupService.cs`:
- Around line 741-755: The GroupAssignment is being built from untrimmed
student.EighthsGroup and student.TwentiethsGroup which causes padded values to
appear even though the DTO fields are trimmed; fix by trimming those inputs
before calling FormatGroupAssignment (use student.EighthsGroup?.Trim() and
student.TwentiethsGroup?.Trim() when computing groupAssignment) so that
FormatGroupAssignment receives normalized values; ensure the existing
EighthsGroup and TwentiethsGroup assignments on the StudentPhoto DTO remain
trimmed as they are.
- Around line 447-459: The Ross-only lookup currently re-queries Rosters into
courseRosterPidms without the original RosterEnrollStatus == "RE" filter, which
can reintroduce withdrawn/waitlisted students; change the second query to reuse
the already-filtered enrolledPidms (the variable created for enrolled students)
instead of courseRosterPidms so the EF query in StudentGroupService (the
rossStudentsInCourse AAUD lookup) only considers actively enrolled PIDMs and
avoids re-adding non-enrolled Ross students.
---
Outside diff comments:
In `@VueApp/src/CTS/pages/CourseStudents.vue`:
- Around line 48-84: The native <tr>/<td> loop rendering loggedInStudents must
be replaced with Quasar table primitives: use <q-table> with
:rows="loggedInStudents" and a body slot (or use QTr/QTd components) instead of
v-for on <tr>; map columns for name/time and render the action cells using the
existing q-btn/q-icon inside QTd, keeping the :key as row.id and preserving the
AssessmentCompetency route construction and aria-labels (refer to
loggedInStudents, AssessmentCompetency route string, and the q-btn/q-icon
instances) so the table uses Quasar components throughout.
🪄 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: 52013b18-6c8b-49ac-997d-893b713acd14
📒 Files selected for processing (11)
VueApp/src/CTS/pages/CourseStudents.vuetest/CMS/CodecsTests.cstest/Students/StudentGroupServiceQueryTests.csweb/Areas/CMS/Data/Codecs.csweb/Areas/ClinicalScheduler/Services/InstructorScheduleService.csweb/Areas/ClinicalScheduler/Services/ScheduleQueryExtensions.csweb/Areas/ClinicalScheduler/Services/StudentScheduleService.csweb/Areas/Students/Services/StudentGroupService.csweb/Models/CTS/IScheduleEntity.csweb/Models/CTS/InstructorSchedule.csweb/Models/CTS/StudentSchedule.cs
Comment on lines
+200
to
+236
| [Fact] | ||
| public async Task GetStudentsByCourseAsync_IncludeRoss_ReturnsRossStudentEnrolledInCourse() | ||
| { | ||
| // A regular V3 student and a Ross student, both enrolled (RE) in the same course. | ||
| SeedStudent("P1", "PIDM1", "Adams", "Bob", "V3"); | ||
| SeedStudent("P9", "PIDM9", "Reardon", "Chelsea", "V3"); | ||
| _sisContext.StudentDesignations.Add(new StudentDesignation | ||
| { | ||
| DesignationType = "Ross", | ||
| IamId = "IAMP9", // matches SeedStudent's "IAM" + pKey | ||
| StartTerm = int.Parse(Term), | ||
| EndTerm = null | ||
| }); | ||
| await _sisContext.SaveChangesAsync(TestContext.Current.CancellationToken); | ||
|
|
||
| foreach (var (pkey, pidm) in new[] { ("RST1", "PIDM1"), ("RST9", "PIDM9") }) | ||
| { | ||
| _coursesContext.Rosters.Add(new Roster | ||
| { | ||
| RosterPkey = pkey, | ||
| RosterTermCode = Term, | ||
| RosterCrn = "12345", | ||
| RosterEnrollStatus = "RE", | ||
| RosterPidm = pidm | ||
| }); | ||
| } | ||
| await _coursesContext.SaveChangesAsync(TestContext.Current.CancellationToken); | ||
|
|
||
| // The Ross-inclusion branch must not join the Courses and AAUD DbContexts in one | ||
| // query: EF cannot translate a cross-context join and throws at execution, which the | ||
| // service surfaces as a failure (previously swallowed into an empty gallery). | ||
| var result = await _service.GetStudentsByCourseAsync(Term, "12345", includeRossStudents: true); | ||
|
|
||
| Assert.Equal(2, result.Count); | ||
| Assert.Contains(result, s => s.LastName == "Adams" && !s.IsRossStudent); | ||
| Assert.Contains(result, s => s.LastName == "Reardon" && s.IsRossStudent); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a non-RE Ross fixture to this regression.
This only proves the happy path. Add one Ross-designated student with a non-enrolled roster status and assert it stays out of the result so the Ross-only course branch cannot drift from the main enrollment filter again.
🤖 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 `@test/Students/StudentGroupServiceQueryTests.cs` around lines 200 - 236, Add a
negative Ross fixture to the existing test
GetStudentsByCourseAsync_IncludeRoss_ReturnsRossStudentEnrolledInCourse: create
another SeedStudent + StudentDesignation pair (use a distinct pkey/pidm like
"P10"/"PIDM10" and matching IamId) and add a Roster entry for that pidm with
RosterCrn "12345" but set RosterEnrollStatus to a non-"RE" value (e.g. "WD" or
"DR"). Save to _sisContext and _coursesContext like the others, then keep the
existing call to _service.GetStudentsByCourseAsync(Term, "12345",
includeRossStudents: true) and add an assertion that this non-RE Ross student is
not present in result (e.g. assert no s with LastName matching the new seed or
that result.Count remains 2). Ensure you reference the existing SeedStudent,
StudentDesignation, Roster, _sisContext/_coursesContext and
GetStudentsByCourseAsync in the change.
Comment on lines
+22
to
+25
| if (classYear.HasValue) | ||
| { | ||
| query = query.Where(s => s.Week.WeekGradYears.Any(gy => gy.GradYear == classYear)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy lift
Correlated subquery on schedule filtering.
The .Any() inside .Where() creates a correlated subquery that executes for each schedule row. Per coding guidelines, pre-load WeekIds for the class year and use .Contains() instead.
♻️ Optimization approach
The current extension signature makes this optimization challenging without breaking the composable query pattern. Consider a separate async method or caller-side pre-loading:
// Caller pre-loads before calling extension
var weekIdsForClassYear = classYear.HasValue
? await _context.WeekGradYears
.Where(gy => gy.GradYear == classYear)
.Select(gy => gy.WeekId)
.Distinct()
.ToListAsync()
: null;
var query = _context.StudentSchedules
.Include(s => s.Week)
.AsNoTracking();
if (weekIdsForClassYear != null)
{
query = query.Where(s => weekIdsForClassYear.Contains(s.WeekId));
}
query = query.ApplyScheduleFilters(null, mothraId, rotationId, serviceId, weekId, startDate, endDate);Alternatively, add an overload accepting pre-loaded week IDs to avoid the signature change.
🤖 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/Areas/ClinicalScheduler/Services/ScheduleQueryExtensions.cs` around lines
22 - 25, The current ScheduleQueryExtensions implementation uses
Week.WeekGradYears.Any(...) inside the Where, causing a correlated subquery;
instead preload the matching WeekIds from WeekGradYears (filter by GradYear and
Select WeekId/Distinct) and change the predicate to use
weekIds.Contains(s.WeekId). Add an overload on the
ApplyScheduleFilters/extension in ScheduleQueryExtensions that accepts an
IEnumerable<int> weekIds (or accept a nullable preloaded list) and, when
provided, apply query = query.Where(s => weekIds.Contains(s.WeekId)); keep the
original signature behavior when weekIds is null so callers that preload WeekIds
(from WeekGradYears where GradYear == classYear) can pass them in and avoid the
per-row Any().
Source: Coding guidelines
Comment on lines
+447
to
+459
| // Get Ross students enrolled in this course. Use a two-step query (like the | ||
| // enrollment lookup above) to avoid joining the Courses and AAUD DbContexts | ||
| // in a single query, which EF cannot translate. | ||
| var courseRosterPidms = await _coursesContext.Rosters | ||
| .Where(r => r.RosterTermCode == termCode && r.RosterCrn == crn) | ||
| .Select(r => r.RosterPidm) | ||
| .Distinct() | ||
| .ToListAsync(); | ||
|
|
||
| var rossStudentsInCourse = await _aaudContext.Ids | ||
| .Where(i => i.IdsIamId != null | ||
| && EF.Parameter(courseRosterPidms).Contains(i.IdsPidm) | ||
| && EF.Parameter(rossIamIds).Contains(i.IdsIamId)) |
Contributor
There was a problem hiding this comment.
Reuse enrolledPidms for the Ross-only lookup.
This second roster query drops the RosterEnrollStatus == "RE" filter from the main path, so includeRossStudents can re-add withdrawn or waitlisted Ross students to the course gallery.
Suggested fix
- var courseRosterPidms = await _coursesContext.Rosters
- .Where(r => r.RosterTermCode == termCode && r.RosterCrn == crn)
- .Select(r => r.RosterPidm)
- .Distinct()
- .ToListAsync();
+ var courseRosterPidms = enrolledPidms;🤖 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/Areas/Students/Services/StudentGroupService.cs` around lines 447 - 459,
The Ross-only lookup currently re-queries Rosters into courseRosterPidms without
the original RosterEnrollStatus == "RE" filter, which can reintroduce
withdrawn/waitlisted students; change the second query to reuse the
already-filtered enrolledPidms (the variable created for enrolled students)
instead of courseRosterPidms so the EF query in StudentGroupService (the
rossStudentsInCourse AAUD lookup) only considers actively enrolled PIDMs and
avoids re-adding non-enrolled Ross students.
Comment on lines
+741
to
+755
| var displayName = FormatStudentDisplayName(student.LastName, student.FirstName ?? string.Empty, student.MiddleName); | ||
| var (photoUrl, hasPhoto) = ResolvePhotoUrl(student.MailId, photoUrls, defaultPhotoUrl); | ||
| var groupAssignment = FormatGroupAssignment(student.EighthsGroup, student.TwentiethsGroup); | ||
|
|
||
| result.Add(new StudentPhoto | ||
| { | ||
| MailId = student.MailId ?? string.Empty, | ||
| FirstName = student.FirstName ?? string.Empty, | ||
| LastName = student.LastName, | ||
| DisplayName = displayName, | ||
| PhotoUrl = photoUrl, | ||
| GroupAssignment = groupAssignment, | ||
| EighthsGroup = student.EighthsGroup?.Trim(), | ||
| TwentiethsGroup = student.TwentiethsGroup?.Trim(), | ||
| TeamNumber = student.ClassLevel == "V3" ? student.TeamNumber?.Trim() : null, |
Contributor
There was a problem hiding this comment.
Trim the group values before formatting GroupAssignment.
EighthsGroup and TwentiethsGroup are normalized for the DTO fields, but GroupAssignment is built from the untrimmed values first. That can leave padded AAUD values in the combined label while the individual properties are clean.
Suggested fix
- var displayName = FormatStudentDisplayName(student.LastName, student.FirstName ?? string.Empty, student.MiddleName);
- var (photoUrl, hasPhoto) = ResolvePhotoUrl(student.MailId, photoUrls, defaultPhotoUrl);
- var groupAssignment = FormatGroupAssignment(student.EighthsGroup, student.TwentiethsGroup);
+ var displayName = FormatStudentDisplayName(student.LastName, student.FirstName ?? string.Empty, student.MiddleName);
+ var (photoUrl, hasPhoto) = ResolvePhotoUrl(student.MailId, photoUrls, defaultPhotoUrl);
+ var eighthsGroup = student.EighthsGroup?.Trim();
+ var twentiethsGroup = student.TwentiethsGroup?.Trim();
+ var groupAssignment = FormatGroupAssignment(eighthsGroup, twentiethsGroup);
@@
- EighthsGroup = student.EighthsGroup?.Trim(),
- TwentiethsGroup = student.TwentiethsGroup?.Trim(),
+ EighthsGroup = eighthsGroup,
+ TwentiethsGroup = twentiethsGroup,🤖 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/Areas/Students/Services/StudentGroupService.cs` around lines 741 - 755,
The GroupAssignment is being built from untrimmed student.EighthsGroup and
student.TwentiethsGroup which causes padded values to appear even though the DTO
fields are trimmed; fix by trimming those inputs before calling
FormatGroupAssignment (use student.EighthsGroup?.Trim() and
student.TwentiethsGroup?.Trim() when computing groupAssignment) so that
FormatGroupAssignment receives normalized values; ensure the existing
EighthsGroup and TwentiethsGroup assignments on the StudentPhoto DTO remain
trimmed as they are.
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
Per-area dedup driven by jscpd findings on the original
code-anaylsisbranch: 4 self-contained refactors across CTS, Scheduler, Students, and CMS (11 files). The Students refactor also uncovered and fixed a photo-gallery bug (see below), so this PR carries a behavior fix and new regression tests in addition to the dedup.(This was formerly "Part 6 of 6" of a stack; PRs #174 through #178 are all merged to
main, and this branch is rebased off the latestmain, so it now stands alone.)Commits
refactor(cts): collapse hardcoded CourseStudents rows into a v-for: replace 3 near-identical<tr>blocks with a singlev-for.refactor(scheduler): share schedule filter LINQ via IScheduleEntity: introduceIScheduleEntity+ScheduleQueryExtensionsso the by-clinician / by-rotation / by-week scheduling queries share their filter LINQ.refactor(students): consolidate StudentGroupService photo + Ross helpers: extractBuildStudentPhotoListAsync,FormatStudentDisplayName,FormatGroupAssignment,ResolvePhotoUrl, andGetActiveRossIamIdsAsyncso the by-class-level / by-group / by-course paths no longer hand-roll the same logic. This commit also includes:StudentBaseRecordconstructor and then appliedOrderBy/Where, which EF Core cannot translate; the resulting runtimeInvalidOperationExceptionwas swallowed by a catch and surfaced as an empty (but 200 OK) result. Fixed by moving ordering and filtering ahead of the projection and rethrowing with context instead of silently returning empty.EF.Parameter()on largeContainslists. The Ross IAM-id and enrolled-PIDM lookups now wrap their collections inEF.Parameter(...)so SQL Server translates them viaOPENJSON(per the >10-item guideline) rather than inlining hundreds of literals.test/Students/StudentGroupServiceQueryTests.cs): SQLite-backed tests that exercise all three query paths end to end, so the projection-translation failure cannot silently regress again.refactor(cms): reduce Codecs to UU-only and cover with tests: collapse the UU encode/decode methods onto shared helpers (DecodeWithMap,EncodeWithMap), delete the unused XX variants and their maps (ColdFusion only ever uses the default UU encoding, so XX was dead in both the legacy app and here), and addCodecsTests.UUEncodehas no caller yet but is kept for the CMS migration, where new files must store their AES key UU-encoded so legacy CF can decrypt them during the parallel-run period.Test plan
StudentGroupServiceQueryTestscover all three paths in CI)CodecsTests, run in CI); XX path removed