feat: fixing duplicate schedule creation and manage in-flight state f…#70
Conversation
…or schedule operations
WalkthroughBackend course service now raises a "multiple schedules" error only when multiple active schedules exist for a semester/campus. Frontend schedule list component guards concurrent requests with in-flight tracking refs and deduplicates newly created schedules by ID to prevent duplicates and race conditions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
BeWelsh
left a comment
BeWelsh
left a comment
There was a problem hiding this comment.
actually - I don't think this will work as most schedules from the year prior will be finalize and not be active. We still want to use these schedules. I dont think a user should run into this if they are deleted competing schedules for the same semester- i will investigate
BeWelsh
left a comment
BeWelsh
left a comment
There was a problem hiding this comment.
Take a look at what i changed - this is so we are making sure there are no other active schedules for that semester, but we are still using course data from the inactive schedules to build the new schedule
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/course.py (1)
116-125:⚠️ Potential issue | 🔴 Critical
schedules[0]may reference a soft-deleted schedule — copy from prior year silently uses wrong data.The code filters the error check for active schedules (line 118) but continues to use
schedules[0]without filtering. Sinceget_schedules(db, semester, campus_id)is called without theactive_onlyparameter, it returns all schedules (both active and soft-deleted) in undefined order. If the soft-deleted row sorts first, the new draft copies courses/sections from a soft-deleted schedule, contradicting the stated intent.Additionally, if no active prior schedule exists but inactive ones do, the code silently bootstraps from soft-deleted data instead of raising an error—inconsistent with the docstring requirement for "a prior schedule with historical data."
Fix: Extract active schedules first, use the filtered list, and raise when no active schedule exists.
Proposed fix
- # Only consider active schedules. schedules = semester_repo.get_schedules(db, semester, campus_id) - if len([s for s in schedules if s.active]) > 1: + active_schedules = [s for s in schedules if s.active] + if len(active_schedules) > 1: raise ValueError(f"Semester {semester_id} has multiple schedules for campus {campus_id}; expected 1") + if not active_schedules: + raise ValueError(f"No active prior schedule found for semester {semester_id}, campus {campus_id}") - courses = schedule_repo.get_courses(schedules[0]) + prior = active_schedules[0] + courses = schedule_repo.get_courses(prior) if not courses: - raise ValueError(f"Prior schedule (id={schedules[0].schedule_id}) has no sections to copy from") + raise ValueError(f"Prior schedule (id={prior.schedule_id}) has no sections to copy from") - return sort_course_list(get_section_count(schedules[0], courses, [])) + return sort_course_list(get_section_count(prior, courses, []))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/course.py` around lines 116 - 125, The code currently calls semester_repo.get_schedules(...) and then uses schedules[0] without filtering out soft-deleted rows; change to first obtain only active schedules (filter schedules by s.active or request active_only from semester_repo.get_schedules if available), then validate there is exactly one active schedule (raise ValueError if none or >1), and use that active schedule when calling schedule_repo.get_courses(...), get_section_count(...) and sort_course_list(...); ensure error messages reference the semester_id/campus_id and the chosen schedule_id when raising to avoid silently copying from a soft-deleted schedule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/app/services/course.py`:
- Around line 116-125: The code currently calls semester_repo.get_schedules(...)
and then uses schedules[0] without filtering out soft-deleted rows; change to
first obtain only active schedules (filter schedules by s.active or request
active_only from semester_repo.get_schedules if available), then validate there
is exactly one active schedule (raise ValueError if none or >1), and use that
active schedule when calling schedule_repo.get_courses(...),
get_section_count(...) and sort_course_list(...); ensure error messages
reference the semester_id/campus_id and the chosen schedule_id when raising to
avoid silently copying from a soft-deleted schedule.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ee508b2-9f8c-4cf8-92c9-b9a35f2b354e
📒 Files selected for processing (2)
backend/app/services/course.pyfrontend/src/pages/ScheduleList.tsx
2 participants
…or schedule operations
Description
Type of Change
What Changed?
Testing & Validation
How this was tested
Screenshots/Recordings
Unfinished Work & Known Issues
Notes & Nuances
Reviewer Notes
Summary by CodeRabbit
Bug Fixes