Skip to content

Conversation

@juliaxchen
Copy link
Collaborator

@juliaxchen juliaxchen commented Nov 10, 2025

GitHub Issues addressed

What I did

  • This adds a JSON field for prerequisites for the course model.
  • The prerequisites include course mnemonic and number and are stored as strings in a list.

Screenshots

  • Before
  • After

Testing

  • A brief explanation of tests done/written or how reviewers can test your work:
  • Run the load_semester.py script and test by running the script below in a separate file
  from django.core.management.base import BaseCommand
  from tcf_website.models.models import Course
  
  class Command(BaseCommand):
      help = "Print all courses and their prerequisites"
  
      def handle(self, *args, **options):
          courses = Course.objects.all()
  
          for c in courses:
              if c.prerequisites:
                  self.stdout.write(f"{c.title}: {c.prerequisites}")

Questions/Discussions/Notes

Summary by CodeRabbit

  • New Features
    • Courses now automatically extract and store prerequisite information from descriptions, making course requirements easily accessible throughout the system.

@coderabbitai
Copy link

coderabbitai bot commented Nov 10, 2025

Walkthrough

This 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

Cohort / File(s) Summary
Model Field Addition
tcf_website/models/models.py
Adds prerequisites as a new JSONField to the Course model with default=list and blank=True to store course prerequisites
Database Migration
tcf_website/migrations/0024_course_prerequisites.py
Django migration that adds the prerequisites JSONField to the course table with proper dependencies on the prior migration
Prerequisite Extraction Logic
tcf_website/management/commands/load_semester.py
Implements text parsing logic inside load_course() to detect "Pre-requisite" or "Prerequisite" in course descriptions, extract text after the colon, truncate at first period if inline, and populate course prerequisites list via regex matching
Unit Tests
tcf_website/tests/test_course.py
Adds two test methods: test_prerequisites_default validates default empty list, and test_prerequisites_set_and_get verifies persistence through database save/refresh cycle

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • The regex-based text parsing logic in load_semester.py requires careful validation to ensure it handles edge cases correctly (malformed descriptions, multiple prerequisites, inline vs. block formats)
  • Verify that the default=list parameter on the JSONField is implemented correctly to avoid potential mutable default issues—should ideally use a callable like default=list with proper Django handling or default=lambda: []
  • Confirm the regex pattern correctly extracts course mnemonic+code combinations across various description formats

Poem

🐰 Prerequisites now find their place,
In JSON fields with textual grace,
Regex dances through descriptions,
Building academic subscriptions,
From load to test, the chain rings clear! 📚✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: adding a prerequisites field to store course prerequisites, which aligns with the core purpose of the changeset.
Description check ✅ Passed The description covers most template sections with specific implementation details, though GitHub Issues section is empty and Screenshots contain only placeholders without actual before/after content.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch store-prerequisites

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f79aad and a9550aa.

📒 Files selected for processing (3)
  • tcf_website/migrations/0024_course_prerequisites.py (1 hunks)
  • tcf_website/models/models.py (1 hunks)
  • tcf_website/tests/test_course.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tcf_website/models/models.py
🧰 Additional context used
🧬 Code graph analysis (1)
tcf_website/migrations/0024_course_prerequisites.py (1)
tcf_website/migrations/0001_initial.py (1)
  • Migration (12-742)
🪛 Ruff (0.14.4)
tcf_website/migrations/0024_course_prerequisites.py

8-10: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


12-18: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

🔇 Additional comments (3)
tcf_website/tests/test_course.py (2)

124-126: LGTM! Default value test is correct.

The test properly verifies that the prerequisites field defaults to an empty list for existing course instances.


128-135: LGTM! Persistence test is well-structured.

The test correctly verifies that prerequisites can be set, saved, and retrieved from the database using Django's standard save/refresh pattern.

tcf_website/migrations/0024_course_prerequisites.py (1)

1-18: LGTM! Migration structure and dependency chain are correct.

The migration properly adds the prerequisites JSONField to the Course model. The use of default=list is correct for Django's JSONField—Django serializes this as a callable reference and will invoke list() for each new instance, avoiding shared mutable default issues. The dependency on migration 0023 is valid, and 0024 is properly sequenced as the latest migration.

Note on static analysis: The Ruff RUF012 warnings about ClassVar annotations are false positives. Django migrations follow a standard framework pattern where dependencies and operations are class-level attributes that don't require ClassVar annotations.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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_codes

Or extend the parse_course_description() method to also return the extracted course codes.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ea4bffc and f308d40.

📒 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=list and blank=True is appropriate for storing prerequisite course codes as a JSON array. This complements the existing parse_course_description() method and provides structured storage.

@artiehumphreys
Copy link
Member

Please do not add your about changes to this PR. Please make a new branch and add the prerequisite changes there. Thanks!

@juliaxchen
Copy link
Collaborator Author

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!
Julia Chen

Copy link

@coderabbitai coderabbitai bot left a 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 prerequisites JSONField introduces a structured storage for prerequisites, which is a good improvement. However, the existing compute_pre_req() and parse_course_description() methods (lines 641-679) parse prerequisites from the description text 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.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f308d40 and b743ec2.

📒 Files selected for processing (1)
  • tcf_website/models/models.py (1 hunks)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants