adding crosslist check so same time warnings doesnt come when crosslisted#76
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 56 minutes and 15 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughA new repository function computes section IDs within a crosslisted group by querying forward and reverse crosslist relationships. The section service now uses this to exclude crosslisted sections from faculty double-booking checks, preventing false warnings when the same faculty teaches linked sections in the same time block. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service as Section Service
participant Repo as Section Repository
participant DB as Database
Client->>Service: error_check(section_id)
Service->>Repo: crosslist_group_section_ids(section_id)
Repo->>DB: Query section & crosslisted_section_id
DB-->>Repo: Section record
Repo->>DB: Query reverse crosslist match
DB-->>Repo: Matching sections
Repo-->>Service: crosslist_group_ids (set)
Service->>DB: Query faculty assignments
DB-->>Service: All assignments for faculty
Service->>Service: Filter: exclude assignments in crosslist_group
Service->>Service: Check for overlaps in remaining assignments
Service-->>Client: Validation warnings (or none)
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
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/section.py (1)
330-345:⚠️ Potential issue | 🟠 MajorCrosslist exclusion unintentionally skews
FACULTY_OVERLOADaccounting.
other_assignmentsis reused for theFACULTY_OVERLOADcheck at Line 344, not justdouble_booked. Excluding the whole crosslist group here makes the reported load depend on which section you're evaluating:
- From a crosslisted section A (partner B), assignments to A and B are both filtered out, so
len(other_assignments) + 1counts the A/B pair as 1 toward the load.- From an unrelated section C taught by the same faculty, A and B are both kept, so the pair counts as 2 toward the load.
So a faculty teaching a crosslisted pair (A,B) + one standalone C with
max_load=2will produce no overload warning from A or B but will produceFACULTY_OVERLOADwhen evaluating C — results are asymmetric and order-of-evaluation dependent. Earlier (pre-change) behavior was symmetric because only the current section row was excluded.Consider splitting the filter so the crosslist group exclusion only applies to the double-booking check, and the load count uses one assignment per crosslist group. Something along these lines:
♻️ Suggested shape
- other_assignments = [ - a - for a in faculty_repo.get_assignments(db, nuid, section.schedule_id) - if a.section_id not in crosslist_ids - ] + all_assignments = faculty_repo.get_assignments(db, nuid, section.schedule_id) + # For double-booking, ignore everything in the current crosslist group. + non_crosslisted_assignments = [a for a in all_assignments if a.section_id not in crosslist_ids] + # For load, collapse each crosslist group (including the current one) to a single entry. + seen_groups: set[frozenset[int]] = set() + load_count = 0 + for a in all_assignments: + group = frozenset(section_repo.crosslist_group_section_ids(db, a.section_id)) + if group in seen_groups: + continue + seen_groups.add(group) + load_count += 1 if section.time_block_id is not None: if not faculty_repo.find_meeting_time_preference(db, nuid, section.time_block_id): warnings.append(WarningType.UNPREFERENCED_TIME) - if section_repo.double_booked(db, other_assignments, section.time_block_id): + if section_repo.double_booked(db, non_crosslisted_assignments, section.time_block_id): warnings.append(WarningType.FACULTY_DOUBLE_BOOKED) if not faculty_repo.find_course_preference(db, nuid, section.course_id): warnings.append(WarningType.UNPREFERENCED_COURSE) f = faculty_repo.get_by_nuid(db, nuid) - if len(other_assignments) + 1 > f.max_load: + if load_count > f.max_load: warnings.append(WarningType.FACULTY_OVERLOAD)(The per-assignment lookup is O(N) queries — if that's a concern, compute crosslist groups once per schedule upstream.)
Worth adding a test for the asymmetric case (crosslisted pair + standalone third section,
max_load=2) to lock behavior down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/section.py` around lines 330 - 345, The FACULTY_OVERLOAD miscounts because other_assignments is filtered by crosslist_ids for the double-booked check and then reused for the load check; change the code to build two separate collections from faculty_repo.get_assignments(db, nuid, section.schedule_id): 1) assignments_for_doublebook = assignments excluding any assignment whose section_id is in crosslist_ids (used only with section_repo.double_booked), and 2) assignments_for_load = assignments excluding only the current section (e.g., a.section_id == section.id) and then collapse/group assignments that belong to the same crosslist group into a single representative before counting; then use len(grouped_assignments) + 1 vs f.max_load to decide WarningType.FACULTY_OVERLOAD while leaving the double-booking logic using assignments_for_doublebook and section_repo.double_booked unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/section.py`:
- Line 321: The file app/services/section.py fails ruff formatting; run the
formatter and commit the changes: run `ruff format app/services/section.py` (or
your editor's ruff integration) to reformat the file, verify the change around
the line that calls section_repo.crosslist_group_section_ids (inside the
function where crosslist_ids is assigned), and include the reformatted file in
your commit so `ruff format --check` passes in CI.
---
Outside diff comments:
In `@backend/app/services/section.py`:
- Around line 330-345: The FACULTY_OVERLOAD miscounts because other_assignments
is filtered by crosslist_ids for the double-booked check and then reused for the
load check; change the code to build two separate collections from
faculty_repo.get_assignments(db, nuid, section.schedule_id): 1)
assignments_for_doublebook = assignments excluding any assignment whose
section_id is in crosslist_ids (used only with section_repo.double_booked), and
2) assignments_for_load = assignments excluding only the current section (e.g.,
a.section_id == section.id) and then collapse/group assignments that belong to
the same crosslist group into a single representative before counting; then use
len(grouped_assignments) + 1 vs f.max_load to decide
WarningType.FACULTY_OVERLOAD while leaving the double-booking logic using
assignments_for_doublebook and section_repo.double_booked unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b49fc4e-1ee5-4a30-8b49-d1d2ea789270
📒 Files selected for processing (3)
backend/app/repositories/section.pybackend/app/services/section.pybackend/tests/test_section_service.py
2 participants
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