-
Notifications
You must be signed in to change notification settings - Fork 11
Store prerequisites #1201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Store prerequisites #1201
Conversation
Removed Julia Chen's information from the team info.
WalkthroughThis PR introduces prerequisite tracking for courses by adding a JSONField to the Course model, implementing extraction logic to parse prerequisites from course descriptions during data loading, and includes a database migration and validation tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)tcf_website/migrations/0024_course_prerequisites.py (1)
🪛 Ruff (0.14.4)tcf_website/migrations/0024_course_prerequisites.py8-10: Mutable class attributes should be annotated with (RUF012) 12-18: Mutable class attributes should be annotated with (RUF012) 🔇 Additional comments (3)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tcf_website/management/commands/load_semester.py (1)
209-226: Consider reusing existing prerequisite parsing logic.The Course model already has a
parse_course_description()method (lines 582-612 in models.py) that extracts prerequisite text from descriptions. Consider refactoring to reuse that method rather than duplicating the string manipulation logic, which would improve maintainability.For example, you could refactor like this:
if not pd.isnull(description): # Temporarily set description to parse prerequisites temp_desc = course.description course.description = description pre_req_text, _ = course.parse_course_description() course.description = temp_desc if pre_req_text: # Match only on course mnemonic and code matches = re.findall(r'([A-Z]{2,4}\s?\d{4})', pre_req_text) prereq_codes = [m.strip().upper() for m in matches] course.prerequisites = prereq_codesOr extend the
parse_course_description()method to also return the extracted course codes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tcf_website/management/commands/load_semester.py(1 hunks)tcf_website/models/models.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tcf_website/models/models.py (1)
tcf_website/migrations/0013_discipline_section_cost_course_disciplines.py (1)
Migration(6-38)
🔇 Additional comments (1)
tcf_website/models/models.py (1)
550-551: LGTM! Prerequisites field properly defined.The JSONField with
default=listandblank=Trueis appropriate for storing prerequisite course codes as a JSON array. This complements the existingparse_course_description()method and provides structured storage.
|
Please do not add your about changes to this PR. Please make a new branch and add the prerequisite changes there. Thanks! |
|
Hey Artie, I forgot to stash my changes when I was working on the about-juliachen branch, so they also appear on this branch. I believe I removed the about changes from this PR, but please let me know if you still see the about changes on this branch. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tcf_website/models/models.py (1)
617-618: Clarify relationship with existing prerequisite parsing methods.The new
prerequisitesJSONField introduces a structured storage for prerequisites, which is a good improvement. However, the existingcompute_pre_req()andparse_course_description()methods (lines 641-679) parse prerequisites from thedescriptiontext field, creating two different sources for prerequisite data.Consider:
- Will the existing methods be deprecated in favor of this field?
- Should
compute_pre_req()be updated to read from this field instead?- How will you ensure consistency between the two sources during the transition?
Additionally, consider adding validation to ensure the JSON structure matches expectations:
# Course prerequisites. Optional. -prerequisites = models.JSONField(default=list, blank=True) +from django.core.validators import ValidationError + +def validate_prerequisites_list(value): + if not isinstance(value, list): + raise ValidationError("Prerequisites must be a list") + if not all(isinstance(item, str) for item in value): + raise ValidationError("All prerequisites must be strings") + +prerequisites = models.JSONField( + default=list, + blank=True, + validators=[validate_prerequisites_list], + help_text="List of prerequisite course codes (e.g., ['CS 2150', 'MATH 3100'])" +)Note: The use of
default=list(passing the class as a callable) is correct for avoiding mutable default issues.
Refactor prerequisite handling to use description variable instead of course.description for consistency.
GitHub Issues addressed
What I did
Screenshots
Testing
Questions/Discussions/Notes
Summary by CodeRabbit