Rebuild courses from selected processing window#157
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds domain documentation and a normalization utility, applies normalized course-key lookup across controllers and tests, implements a validated CourseRebuild service + SQL gateway with DTOs and tests, registers DI, provides an admin React UI, and adds EF migrations that create/harden and modify the rebuild stored procedures. ChangesCourse Rebuild Feature
Sequence Diagram(s)sequenceDiagram
participant Admin
participant UI as React UI
participant SystemCtrl as SystemController
participant Service as CourseRebuildService
participant Gateway as CourseRebuildSqlGateway
participant DB as SQL Server
Admin->>UI: Load page
UI->>SystemCtrl: GET CourseRebuildOptions
SystemCtrl->>Service: GetAcademicYearSpanOptionsAsync()
Service->>Gateway: GetAcademicYearSpanTermRowsAsync()
Gateway->>DB: EXEC usp_GetCourseRebuildAcademicYearSpanOptions
DB-->>Gateway: Term rows
Gateway-->>Service: rows
Service-->>SystemCtrl: AcademicYearSpanOptionModel[]
SystemCtrl-->>UI: JSON options
Admin->>UI: Select span, confirm rebuild
UI->>SystemCtrl: POST RebuildCourses({ academicTermCodes })
SystemCtrl->>Service: RebuildCoursesAsync(termCodes)
Service->>Service: CourseProcessingWindow.Validate(termCodes)
alt Validation fails
Service-->>SystemCtrl: CourseRebuildValidationException
SystemCtrl-->>UI: HTTP 400
else
Service->>Gateway: RebuildCoursesAsync(termCodes)
Gateway->>DB: EXEC usp_RebuildCoursesFromProcessingWindow (TVP)
DB-->>Gateway: Success
Gateway-->>Service: Complete
Service-->>SystemCtrl: CourseRebuildResultModel
SystemCtrl-->>UI: JSON result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
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: 7
🧹 Nitpick comments (1)
src/tacos.core/Migrations/20260515231740_CourseRebuildResetsSubmissions.cs (1)
15-80: 🏗️ Heavy liftPrefer full procedure definitions over string-surgery patching in migrations.
These
CHARINDEX/STUFFedits are fragile to minor definition drift and can hard-fail deployment (50009/50010). Emitting canonicalCREATE OR ALTER PROCEDUREtext per migration is more deterministic.Also applies to: 92-133
🤖 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 `@src/tacos.core/Migrations/20260515231740_CourseRebuildResetsSubmissions.cs` around lines 15 - 80, The migration is doing fragile string-surgery on the stored proc using `@Definition`, CHARINDEX, STUFF and sp_executesql for dbo.usp_RebuildCoursesFromProcessingWindow; replace that logic with an explicit canonical CREATE OR ALTER PROCEDURE statement containing the full desired procedure body and execute that (still via sp_executesql) instead of editing OBJECT_DEFINITION text, and apply the same replacement for the other similar block around lines 92-133 so the migration deterministically deploys the complete procedure text rather than patching via `@Definition/CHARINDEX/STUFF`.
🤖 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
`@src/tacos.core/Migrations/20260515222222_CourseRebuildFromProcessingWindow.cs`:
- Around line 563-567: The update is truncating CrossListingsString to 50 chars
which can drop members; change the assignment in the migration so
NewCourses.[CrossListingsString] receives the full/200-char-supported value from
CrossListingGroups.[CrossListingsString] (e.g., replace the LEFT(..., 50) usage
with the full value or LEFT(..., 200)) in the UPDATE that touches NewCourses
(refer to the UPDATE statement in CourseRebuildFromProcessingWindow migration
that sets [IsCrossListed], [CrossListingsString], [AverageEnrollment]).
In `@src/tacos.core/Migrations/20260515231740_CourseRebuildResetsSubmissions.cs`:
- Around line 111-125: The rollback guard currently compares
Requests.[CourseNumber] to `#NewCourses`.[Number] directly, which can misfire when
values differ only by spacing or case; update the WHERE NOT EXISTS clause to
normalize both sides (trim surrounding whitespace and use a consistent case)
before comparing — e.g., apply RTRIM/LTRIM (or TRIM) and a case-normalizer like
UPPER (or LOWER) to NewCourses.[Number] and Requests.[CourseNumber] so the NOT
EXISTS check uses the normalized values when deciding whether to THROW the 50007
error about referenced courses.
In `@src/tacos.mvc/ClientApp/pages/ManageCourseRebuild.tsx`:
- Line 150: The alert div in ManageCourseRebuild.tsx currently always uses
role="status" (polite); change it so when the rendered alert is an error
(alert.type includes "alert-error") it uses role="alert" (assertive) to ensure
immediate announcement. Locate the JSX with the div that references alert.type
and replace the fixed role with a conditional expression that selects "alert" if
alert.type contains "alert-error" and "status" otherwise, preserving the
existing classes and behavior.
In `@src/tacos.mvc/Controllers/CoursesController.cs`:
- Around line 30-35: The Details action currently calls
CourseNumberKey.Normalize(id) on raw route input which can throw for
null/invalid IDs; mirror the guard used in Get by validating id first (e.g.,
check for null/empty/invalid format) or wrap CourseNumberKey.Normalize(id) in a
try/catch and return a BadRequest/NotFound instead of letting an exception
bubble. Locate the Details method and adjust input handling around
CourseNumberKey.Normalize (or reuse the same validation helper used by Get) so
bad IDs produce a 400/404 response rather than a 500.
In `@src/tacos.mvc/Controllers/RequestsController.cs`:
- Around line 189-192: Extract course-number normalization and validation into a
single guarded helper method (e.g., TryGetCourseByNormalizedNumber or
ValidateAndFindCourse) and use it from Save, Recalculate, and Submit instead of
calling CourseNumberKey.Normalize inline; the helper should call
CourseNumberKey.Normalize, treat empty/null/invalid results as a BadRequest
result, and return either a BadRequest IActionResult or the matched course
fetched via _context.Courses.MatchingCourseNumber(...).FirstOrDefaultAsync so
callers can short-circuit on invalid input and rely on a single
normalization/guard path.
In `@src/tacos.mvc/Controllers/SystemController.cs`:
- Around line 205-216: The DbException catch in CourseRebuildOptions is
returning ex.Message to clients; change it to log the full exception (use the
controller's ILogger or existing logging helper) and return a generic BadRequest
result with a non-sensitive message like "An error occurred while retrieving
options" (do the same for the DbException handler in RebuildCourses).
Specifically, inside CourseRebuildOptions and RebuildCourses catch (DbException
ex) blocks, replace direct return of ex.Message with logger.LogError(ex,
"...context...") and return BadRequest("A database error occurred while
processing your request.") or similar generic text while keeping domain
exceptions (e.g., CourseRebuildValidationException) unchanged.
In `@src/tacos.mvc/Extensions/CourseNumberQueryExtensions.cs`:
- Around line 14-15: Change the ad-hoc Replace(" ", "") normalization to remove
all Unicode whitespace so DB rows with tabs/newlines still match the normalized
input: update the predicates and ordering expressions that reference c.Number
(the Where/OrderBy occurrences using Replace(" ", "").ToUpper()) to strip all
whitespace (e.g. use Regex.Replace(c.Number, "\\s+", "") or
string.Concat(c.Number.Where(ch => !char.IsWhiteSpace(ch))) and then ToUpper())
so they match CourseNumberKey.Normalize; apply this same change to every
occurrence (the Where/OrderBy uses around the comparisons, including the ones
flagged at the three locations).
---
Nitpick comments:
In `@src/tacos.core/Migrations/20260515231740_CourseRebuildResetsSubmissions.cs`:
- Around line 15-80: The migration is doing fragile string-surgery on the stored
proc using `@Definition`, CHARINDEX, STUFF and sp_executesql for
dbo.usp_RebuildCoursesFromProcessingWindow; replace that logic with an explicit
canonical CREATE OR ALTER PROCEDURE statement containing the full desired
procedure body and execute that (still via sp_executesql) instead of editing
OBJECT_DEFINITION text, and apply the same replacement for the other similar
block around lines 92-133 so the migration deterministically deploys the
complete procedure text rather than patching via `@Definition/CHARINDEX/STUFF`.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a6ce6ea1-8f89-49e1-bfbe-b8f205b5def4
📒 Files selected for processing (25)
CONTEXT.mdTest/Controllers/RequestsControllerTests.csTest/Services/CourseRebuildServiceTests.cssrc/tacos.core/Migrations/20260515222222_CourseRebuildFromProcessingWindow.Designer.cssrc/tacos.core/Migrations/20260515222222_CourseRebuildFromProcessingWindow.cssrc/tacos.core/Migrations/20260515231110_HardenCrossListingParser.Designer.cssrc/tacos.core/Migrations/20260515231110_HardenCrossListingParser.cssrc/tacos.core/Migrations/20260515231740_CourseRebuildResetsSubmissions.Designer.cssrc/tacos.core/Migrations/20260515231740_CourseRebuildResetsSubmissions.cssrc/tacos.core/Migrations/20260515233434_CourseRebuildPreservesReferencedCourses.Designer.cssrc/tacos.core/Migrations/20260515233434_CourseRebuildPreservesReferencedCourses.cssrc/tacos.core/Resources/CourseNumberKey.cssrc/tacos.mvc/ClientApp/css/site.scsssrc/tacos.mvc/ClientApp/index.tsxsrc/tacos.mvc/ClientApp/pages/ManageCourseRebuild.tsxsrc/tacos.mvc/Controllers/CoursesController.cssrc/tacos.mvc/Controllers/RequestsController.cssrc/tacos.mvc/Controllers/SystemController.cssrc/tacos.mvc/Extensions/CourseNumberQueryExtensions.cssrc/tacos.mvc/Models/CourseRebuildModels.cssrc/tacos.mvc/Services/CourseRebuildService.cssrc/tacos.mvc/Startup.cssrc/tacos.mvc/Views/System/Index.cshtmlsrc/tacos.mvc/Views/System/ManageCourseRebuild.cshtmlsrc/tacos.mvc/Views/System/ManageSubmissions.cshtml
💤 Files with no reviewable changes (1)
- src/tacos.mvc/Views/System/ManageSubmissions.cshtml
Refine the course rebuild process to better manage associated requests, either resetting submission status or preventing rebuilds when they would remove active requests. Improve consistency in course number normalization and validation across the application, adding early rejection for invalid inputs. Expand course matching logic to tolerate more whitespace characters in numbers. Also, increase cross-listing string length. Enhance error reporting and accessibility for user alerts.
Contributor
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 `@src/tacos.core/Migrations/20260515231740_CourseRebuildResetsSubmissions.cs`:
- Line 33: The SQL equality expressions in the migration (comparing
NewCourses.[Number] and Requests.[CourseNumber]) only strip spaces; update them
to also strip tabs, newlines and carriage returns to match the application-level
MatchingCourseNumber logic. Replace occurrences of REPLACE(..., N' ', N'') with
a chain that removes NCHAR(9), NCHAR(10) and NCHAR(13) (for example nested
REPLACE calls around the existing LTRIM(RTRIM(...)) and UPPER calls) so the
comparisons in the migration class CourseRebuildResetsSubmissions and its SQL
fragments treat \t, \n, \r the same as spaces; this will align behavior with
CourseNumberQueryExtensions.cs and fix both up and down migration mismatches.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f608557-00ba-4344-94ab-2188fdc00b64
📒 Files selected for processing (8)
Test/Controllers/RequestsControllerTests.cssrc/tacos.core/Migrations/20260515222222_CourseRebuildFromProcessingWindow.cssrc/tacos.core/Migrations/20260515231740_CourseRebuildResetsSubmissions.cssrc/tacos.mvc/ClientApp/pages/ManageCourseRebuild.tsxsrc/tacos.mvc/Controllers/CoursesController.cssrc/tacos.mvc/Controllers/RequestsController.cssrc/tacos.mvc/Controllers/SystemController.cssrc/tacos.mvc/Extensions/CourseNumberQueryExtensions.cs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/tacos.mvc/Extensions/CourseNumberQueryExtensions.cs
- src/tacos.mvc/Controllers/CoursesController.cs
- src/tacos.mvc/ClientApp/pages/ManageCourseRebuild.tsx
- src/tacos.mvc/Controllers/SystemController.cs
- src/tacos.core/Migrations/20260515222222_CourseRebuildFromProcessingWindow.cs
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
Validation
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
Style