fix/CSV Export#77
Conversation
WalkthroughThe CSV export's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
![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/routers/schedule.py (1)
185-191:⚠️ Potential issue | 🔴 CriticalCritical: the same
NoneTypecrash still occurs viatime_prefwhensection.time_block is None.The fix at lines 162-163 only guards the
time_blockcolumn. A few lines below, line 187 unconditionally dereferencessection.time_block.time_block_idinside thetime_prefgenerator. Whenever a section has instructors but no assigned time block, this will still raiseAttributeError: 'NoneType' object has no attribute 'time_block_id'and return HTTP 500 — i.e., the very bug this PR claims to fix is only partially resolved. Note that even with a guard, comparingmp.time_block_id == Noneis never a useful match, so the correct behavior is to short-circuittime_prefto""when the section has no time block.🐛 Proposed fix
- time_pref = "; ".join( - next( - (mp.preference for mp in i.meeting_preferences if mp.time_block_id == section.time_block.time_block_id), - "", - ) - for i in instructors - ) + if section.time_block is None: + time_pref = "" + else: + tb_id = section.time_block.time_block_id + time_pref = "; ".join( + next( + (mp.preference for mp in i.meeting_preferences if mp.time_block_id == tb_id), + "", + ) + for i in instructors + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/routers/schedule.py` around lines 185 - 191, The generator building time_pref dereferences section.time_block.time_block_id unconditionally and will crash when section.time_block is None; change the logic in the time_pref construction (the generator that iterates instructors and checks mp.time_block_id) to first short‑circuit and return an empty string when section.time_block is None, otherwise perform the existing lookup against mp.time_block_id == section.time_block.time_block_id using meeting_preferences and mp identifiers; reference the time_pref variable, section.time_block, and meeting_preferences/mp.time_block_id to locate and modify the code.
🤖 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/routers/schedule.py`:
- Around line 185-191: The generator building time_pref dereferences
section.time_block.time_block_id unconditionally and will crash when
section.time_block is None; change the logic in the time_pref construction (the
generator that iterates instructors and checks mp.time_block_id) to first
short‑circuit and return an empty string when section.time_block is None,
otherwise perform the existing lookup against mp.time_block_id ==
section.time_block.time_block_id using meeting_preferences and mp identifiers;
reference the time_pref variable, section.time_block, and
meeting_preferences/mp.time_block_id to locate and modify the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b54c588-e162-4295-9056-ebe92f5ca6fa
📒 Files selected for processing (1)
backend/app/routers/schedule.py
2 participants
Description
The CSV export endpoint was crashing with a 500 error for any schedule that had sections without a time block assigned. The backend's
get_rich_sectionsservice setstime_block=Nonefor unassigned sectionsType of Change
What Changed?
section.time_blockin the CSV export loop inbackend/app/routers/schedule.pytime_blockcolumn instead of crashing the requestTesting & Validation
How this was tested
Unfinished Work & Known Issues
Notes & Nuances
draftguard earlier in the route already prevents exporting incomplete schedules in theory, but sections can legally havetime_block=NoneReviewer Notes
Summary by CodeRabbit