theClubForum #1117
theClubForum #1117
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive support for club reviews and browsing while updating related APIs, templates, and models for theClubForum project.
- Introduces Club and ClubCategory models with supporting migrations and admin interfaces.
- Updates templates (reviews, new review, club, category, browse) to conditionally render club-specific UI elements.
- Extends API endpoints and serializers to include club-related data and refactors several management commands and utilities to accommodate clubs.
Reviewed Changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tcf_website/templates/reviews/review.html | Adds conditional branch for club reviews & adjusts tooltip text accordingly. |
| tcf_website/templates/reviews/new_review.html | Includes club_review.js when in club mode. |
| tcf_website/templates/club/club.html & category.html | New templates for displaying club details and categories. |
| tcf_website/templates/browse/browse.html | Introduces club mode toggle and conditional rendering of club categories. |
| tcf_website/static/reviews/club_review.js | Handles dynamic dropdown population for club reviews. |
| tcf_website/models/models.py & migrations | Adds Club and ClubCategory models and integrates optional club review field. |
| tcf_website/api/* | Updates viewsets, serializers, and URLs to support club data endpoints. |
| management/commands & admin.py | Updates for club data handling and administration interfaces. |
Files not reviewed (2)
- package.json: Language not supported
- tcf_website/static/browse/browse.css: Language not supported
Comments suppressed due to low confidence (1)
tcf_website/management/commands/fetch_data.py:29
- The 'page += 1' statement appears redundant inside a for-loop that already controls page incrementation, which could lead to unexpected behavior. Removing it should ensure reliable pagination.
page += 1
…django command; updated club view with image and application required
There was a problem hiding this comment.
Pull Request Overview
This PR introduces new Club and ClubCategory models along with their associated migrations and API endpoints, and it refactors several management commands and query methods to support more flexible data handling and filtering. Key changes include:
- Creation of Club and ClubCategory models with corresponding admin and migration updates.
- Updates to instructor data annotations and sorting to support a "recent" filter.
- Addition of API endpoints and serializers for clubs as well as improvements in management commands for data import and fetching.
Reviewed Changes
Copilot reviewed 49 out of 53 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tcf_website/models/models.py | Added Club and ClubCategory models; updated instructor annotation logic. |
| tcf_website/migrations/* | New migrations for club-related models and index additions. |
| tcf_website/management/commands/*.py | Updated data import commands with new club handling logic. |
| tcf_website/api/*.py | Added Club endpoints and serializers; updated enrollment logic. |
| tcf_core/context_processors.py | Added a new context value ("min_gpa") for search filters. |
Files not reviewed (4)
- Dockerfile: Language not supported
- scripts/container-startup.sh: Language not supported
- tcf_website/static/browse/browse.css: Language not supported
- tcf_website/static/club/mode_toggle.css: Language not supported
Comments suppressed due to low confidence (1)
tcf_website/management/commands/load_clubs.py:103
- Using only the first 4 characters for a slug may lead to collisions; consider generating a longer slug or implementing a uniqueness strategy.
slug = slugify(category_name).upper()[:4]
WalkthroughThis update introduces comprehensive support for clubs as a new entity alongside courses throughout the application. It adds models for clubs and club categories, updates migrations, and extends admin, API, and serializers. The browsing, search, and review flows are all adapted to handle both courses and clubs, including new templates, view logic, and static assets for toggling modes and filtering. Club reviews are supported with tailored forms, validation, and display logic. The UI is enhanced with new CSS and JavaScript for search filters, mode toggles, and responsive design. Database indexes and migrations are added to optimize performance for new and existing features. Changes
Sequence Diagram(s)Club Browsing and Review FlowsequenceDiagram
participant User
participant Browser
participant DjangoView
participant DB
User->>Browser: Selects "Clubs" mode / navigates to browse/search/review
Browser->>DjangoView: Sends request with mode=clubs
DjangoView->>DB: Fetches ClubCategories and Clubs (optionally filtered)
DB-->>DjangoView: Returns categories/clubs
DjangoView->>Browser: Renders club browse/search/review page
User->>Browser: Fills club review form, submits
Browser->>DjangoView: POST review data (club, ratings, etc.)
DjangoView->>DB: Validates and saves Review linked to Club
DB-->>DjangoView: Confirmation
DjangoView->>Browser: Renders success message or errors
Search with Filters and Mode TogglesequenceDiagram
participant User
participant Browser
participant FiltersJS
participant DjangoView
participant DB
User->>Browser: Opens search page
Browser->>FiltersJS: Loads filter UI, initializes event listeners
User->>FiltersJS: Adjusts filters (e.g., min GPA, subjects)
FiltersJS->>Browser: Updates filter state, enables/disables filter button
User->>Browser: Submits search with filters and mode
Browser->>DjangoView: Sends GET with query, filters, mode
DjangoView->>DB: Applies search/filter logic (courses or clubs)
DB-->>DjangoView: Returns results
DjangoView->>Browser: Renders paginated results
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 34
🔭 Outside diff range comments (2)
tcf_website/api/enrollment.py (1)
60-64: 🛠️ Refactor suggestionReplace
Plain
print()statements in async code end up on stdout, cannot be filtered by level, and may be lost in production environments.-import requests +import logging +import requests … -except requests.exceptions.RequestException as e: - print(f"Network error while fetching section {section.sis_section_number}: {e}") +logger = logging.getLogger(__name__) +… +except requests.exceptions.RequestException as exc: + logger.warning("Network error while fetching section %s: %s", + section.sis_section_number, exc)Apply the same to the JSON decode branch.
tcf_website/models/models.py (1)
1255-1263: 🛠️ Refactor suggestion
get_sorted_reviewsignores club reviewsThe filtering clause hard-codes
instructor+course:Review.objects.filter(instructor=instructor_id, course=course_id, …)For a club review (
course=None,club_idset) it will never match,
so club reviews can’t be displayed or paginated.Either split this logic into two paths (course vs club) or parameterise
the filters so the correct FK is queried.
🧹 Nitpick comments (44)
tcf_website/static/club/mode_toggle.css (1)
131-135: Improve disabled state semantics.
Relying solely on a.disabledclass may not communicate non-interactivity to assistive technologies. Instead:
- Add the native
disabledattribute to the button.- Include
aria-disabled="true".tcf_website/templates/landing/landing.html (1)
91-92: Ensure consistent terminology.
The description reads "clubs, classes, and professors." Elsewhere the site uses "courses" instead of "classes"—consider updating this text to "clubs, courses, and professors" for consistency.tcf_website/admin.py (1)
176-177: Consider using explicit imports instead of star imports.While the admin registrations work correctly, the static analysis tools flagged potential issues with
ClubandClubCategorybeing undefined or defined from star imports (line 8). Consider updating the imports to be explicit rather than usingfrom .models import *.- from .models import * + from .models import ( + User, Review, Vote, Question, Answer, Schedule, + ScheduledCourse, School, Subdepartment, Department, + Semester, Discipline, Course, Instructor, Section, + CourseGrade, CourseInstructorGrade, SectionTime, + SectionEnrollment, CourseEnrollment, Club, ClubCategory + )🧰 Tools
🪛 Ruff (0.8.2)
176-176:
Clubmay be undefined, or defined from star imports(F405)
177-177:
ClubCategorymay be undefined, or defined from star imports(F405)
tcf_website/templates/club/browse_category.html (1)
1-7: Align CSS classes with club context
The template currently reusesschoolanddepartment-listclasses (e.g.,<div class="school card">,<ul class="department-list">) which are semantically tied to departments/courses. Consider introducingclub-listorcategory-listCSS classes to clearly differentiate club styling and avoid confusion.tcf_website/migrations/0022_coursegrade_tcf_website_course__76e103_idx_and_more.py (1)
13-55: Review composite and single-column indexes
Consider whether both composite indexes(course, instructor)and(instructor, course)are necessary or if one can be dropped to reduce index bloat. Also, if the application frequently queries reviews by course alone (e.g., on course detail pages), you may want to add an index onreview.course, since the composite(instructor, course)index won’t optimize queries filtering only by course.tcf_website/templates/reviews/review.html (1)
94-99: Dynamic tooltip labels for rating and commitment
The conditional titles correctly change “Instructor Rating” → “Leadership Rating” and “Difficulty” → “Commitment” for club reviews. Ensure that the model’sreview.instructor_ratingandreview.difficultyfields are appropriately populated for club contexts, and consider wrapping these literals in{% trans %}tags for future internationalization.Also applies to: 106-109
tcf_website/templates/reviews/new_review.html (2)
7-8: Load mode toggle stylesheet conditionally
Currently,club/mode_toggle.cssis always included. Consider wrapping this link in{% if is_club %}so it only loads on club review pages.
38-42: Conditional script loading
The dynamic inclusion ofclub_review.jsversusnew_review.jsbased onis_clubis well-structured. For non-blocking rendering, consider adding thedeferattribute to these<script>tags.tcf_website/templates/common/pagination.html (6)
1-4: Add landmark and accessibility attributes
Wrap the pagination container in a<nav aria-label="Pagination">or addrole="navigation"to improve screen reader support.
7-11: DRY up querystring construction
The repeated{% for key, value in request.GET.items %}blocks for building links can be replaced with a custom template filter (e.g.query_transform) or by usingrequest.GET.urlencodewithpageoverridden. This will reduce duplication and simplify maintenance.
13-17: Extract first-page link logic
Same as above: the link for page 1 uses identical query-building code. Consider a helper or filter to generate links to arbitrary pages, passing onlypage=1.
23-33: Leverage Django’s elided page range
Instead of manually looping and filteringpage_range, you can usepaginator.get_elided_page_range(paginated_items.number, on_each_side=2, on_ends=1)(Django 3.2+) to generate an elided sequence. This yields proper ellipses and avoids iterating over all pages.
35-37: Consolidate trailing ellipses
The trailing dots logic duplicates the intent ofget_elided_page_range. Removing manual dots if using elided ranges will simplify this block.
49-52: DRY next/last controls
The “next” and “last” links use the same querystring logic as the “prev” and “first” controls. Extract this pattern into a reusable snippet or filter to reduce boilerplate.tcf_website/templates/browse/browse.html (2)
30-38: Guard against missingclub_categories
Ifclub_categoriesis everNone, the{% if club_categories %}will error. You may want to default it to an empty list in the view or add a{% default_if_none %}filter.
57-63: Harden collapse toggle logic
The ID extraction viathis.id.replace("header-", "")assumes the header always starts withheader-. To avoid silent failures, you could store target IDs in adata-targetattribute and read fromheader.dataset.target. This decouples the JS from naming conventions.tcf_website/templates/club/mode_toggle.html (2)
9-17: Enhance radio toggle accessibility
Wrap the radio inputs in a<fieldset role="radiogroup" aria-label="Mode toggle">and addaria-checkedstates to the labels for improved screen-reader support.
18-25: Preserve existing query parameters in link mode
The “Courses” and “Clubs” links drop other GET parameters. Consider appending existing query parameters (request.GET.urlencode) when building the URLs to avoid losing filters or search terms on mode switch.scripts/container-startup.sh (3)
1-3: Enable pipefail and unset variable detection
Addset -o pipefail -ualong withset -eto catch failures in piped commands and undefined variables.
6-10: Use non-interactive migrate
For fully automated container startup, pass--no-inputtomigrateand verify that collectstatic errors are handled gracefully.
17-18: Use production-grade server in containers
Runningmanage.py runserveris not recommended in production. Consider replacing with Gunicorn or uWSGI for better performance and stability.tcf_website/static/club/mode_toggle.js (1)
30-33: PreferrequestAnimationFrameover a hard-coded timeout
setTimeout(..., 100)works in most cases, but on slower devices the element may reflow later, causing the first transition to be skipped. Replacing the timeout withrequestAnimationFrame(possibly twice) guarantees the class is removed on the next paint without magic numbers.tcf_website/templates/search/search.html (1)
241-248: Remove stray console debugging
console.log("Icon found:", icon);will surface in production logs/DevTools and may leak DOM details. Consider removing or gating behindif (DEBUG).- console.log("Icon found:", icon);tcf_website/api/views.py (1)
171-183: Inefficient category filtering
filterset_fields = ["category"]already adds a/api/clubs/?category=<id>filter via DRF filters. The manual filtering inget_queryset()duplicates this logic and executes two queries. You can drop the override entirely unless additional conditions are required.tcf_website/management/commands/fetch_clubs.py (2)
14-15: Consider adjusting HTTP timeout and session handling.The timeout value of 300 seconds (5 minutes) is quite long for HTTP requests. Most web services expect responses much faster, and long-running requests may impact script performance.
-session = requests.session() -TIMEOUT = 300 +session = requests.session() +TIMEOUT = 60 # More reasonable timeout valueAdditionally, consider wrapping session handling in a context manager or a class to better manage the lifecycle of the session object.
18-20: The HTML tag stripping regex could be improved.The current regex for stripping HTML tags is simple but may not handle all edge cases like nested tags, attribute values containing '>', or certain malformed HTML.
For more robust HTML parsing, consider using an HTML parser library:
-def strip_html_tags(text): - """Remove HTML tags from text.""" - return re.sub(r"<.*?>", "", text) +def strip_html_tags(text): + """Remove HTML tags from text.""" + if not text: + return "" + try: + from bs4 import BeautifulSoup + return BeautifulSoup(text, "html.parser").get_text() + except ImportError: + # Fallback to regex if BeautifulSoup is not available + return re.sub(r"<.*?>", "", text)tcf_website/management/commands/load_clubs.py (1)
16-61: Consider moving mapping data to a configuration file.The
SQUASH_MAPandBUCKET_PRIORITYconstants are quite large and might be better maintained in a separate configuration file (JSON, YAML, etc.) rather than hardcoded in the Python file. This would make it easier to update category mappings without modifying code.You could move these mappings to a JSON file and load them in the command:
import json import os # Load mappings from configuration file config_path = os.path.join(os.path.dirname(__file__), "club_data/config/category_mappings.json") with open(config_path, "r") as f: config = json.load(f) SQUASH_MAP = config["squash_map"] BUCKET_PRIORITY = config["bucket_priority"]tcf_website/templates/club/club.html (3)
7-7: Consider creating club-specific CSS.The template currently reuses
course_professor.css, but clubs might have unique styling needs. Creating a dedicated CSS file for club pages would improve maintainability.-<link rel="stylesheet" href="{% static 'course/course_professor.css' %}" /> +<link rel="stylesheet" href="{% static 'course/course_professor.css' %}" /> +<link rel="stylesheet" href="{% static 'club/club.css' %}" />
86-95: Duplicate markup for authenticated and non-authenticated users.The "Add your review!" button appears twice with nearly identical markup. Consider refactoring to reduce duplication.
- {% if user.is_authenticated %} - <a href="{% url 'new_review' %}?mode=clubs&club={{ club.id }}" class="btn bg-tcf-orange text-white mb-3"> - <i class="fa fa-plus" aria-hidden="true"></i> Add your review! - </a> - {% else %} - <a href="#loginModal" data-toggle="modal" class="btn bg-tcf-orange text-white mb-3"> - <i class="fa fa-plus" aria-hidden="true"></i> Add your review! - </a> - {% endif %} + <a href="{% if user.is_authenticated %}{% url 'new_review' %}?mode=clubs&club={{ club.id }}{% else %}#loginModal{% endif %}" + {% if not user.is_authenticated %}data-toggle="modal"{% endif %} + class="btn bg-tcf-orange text-white mb-3"> + <i class="fa fa-plus" aria-hidden="true"></i> Add your review! + </a>
105-116: Query parameter generation could be simplified.The URL generation for the sorting dropdown is verbose and repetitive. Consider using a template tag or a simpler approach.
Create a template tag
update_query_string.pyin your app's templatetags directory:from django import template register = template.Library() @register.simple_tag(takes_context=True) def url_replace(context, **kwargs): query = context['request'].GET.copy() for k, v in kwargs.items(): query[k] = v return query.urlencode()Then update your template:
- <a href="{{ request.path }}?{% for key, value in request.GET.items %}{% if key != 'method' %}{{ key }}={{ value }}&{% endif %}{% endfor %}method=Most Helpful#reviews" class="dropdown-item"> + <a href="{{ request.path }}?{% url_replace method='Most Helpful' %}#reviews" class="dropdown-item"> Most Helpful </a> - <a href="{{ request.path }}?{% for key, value in request.GET.items %}{% if key != 'method' %}{{ key }}={{ value }}&{% endif %}{% endfor %}method=Most Recent#reviews" class="dropdown-item"> + <a href="{{ request.path }}?{% url_replace method='Most Recent' %}#reviews" class="dropdown-item"> Most Recent </a> - <a href="{{ request.path }}?{% for key, value in request.GET.items %}{% if key != 'method' %}{{ key }}={{ value }}&{% endif %}{% endfor %}method=Highest Rating#reviews" class="dropdown-item"> + <a href="{{ request.path }}?{% url_replace method='Highest Rating' %}#reviews" class="dropdown-item"> Highest Rating </a> - <a href="{{ request.path }}?{% for key, value in request.GET.items %}{% if key != 'method' %}{{ key }}={{ value }}&{% endif %}{% endfor %}method=Lowest Rating#reviews" class="dropdown-item"> + <a href="{{ request.path }}?{% url_replace method='Lowest Rating' %}#reviews" class="dropdown-item"> Lowest Rating </a>tcf_website/migrations/0021_clubcategory_alter_review_course_and_more.py (1)
92-99: Consider adding additional indexes for performance.You've added a GIN index for text search, but given the new relationships, adding additional indexes could improve performance for common queries.
Consider adding these indexes in a follow-up migration:
# For foreign key relationships migrations.AddIndex( model_name="review", index=models.Index(fields=["club"], name="review_club_idx"), ), # For common filtering operations migrations.AddIndex( model_name="club", index=models.Index(fields=["application_required"], name="club_app_req_idx"), ), # For category browsing migrations.AddIndex( model_name="club", index=models.Index(fields=["category"], name="club_category_idx"), ),tcf_website/static/browse/browse.css (1)
21-24: GPU hint for smoother hover animationAdding
transform: translateY(-2px)on hover triggers layout & paint on some browsers.
Declare intent upfront for better performance:.school { … + will-change: transform; }tcf_website/api/enrollment.py (1)
75-81: Two-hour throttle window – promote to a module constantThe magic number
2is embedded in the timedelta; moving it to a named constant improves readability and makes future tuning trivial.ENROLLMENT_REFRESH_COOLDOWN = timedelta(hours=2) … if not created and timezone.now() - enrollment_tracking.last_update < ENROLLMENT_REFRESH_COOLDOWN: returntcf_website/views/review.py (2)
114-119: Duplicatehours_per_weekcalculation
ReviewForm.save()already computeshours_per_week; recalculating it here is redundant and risks future drift if the formula changes.- instance.hours_per_week = ( - instance.amount_reading - + instance.amount_writing - + instance.amount_group - + instance.amount_homework - )You can safely delete the block.
124-131: Redirect drops the active modeAfter a successful submission you always
redirect("reviews"), losing the?mode=clubsquery. Users land on the course-review list even if they just reviewed a club.-redirect_url = "reviews" +redirect_url = f"{reverse_lazy('reviews')}?mode={mode}"Consider using
reverse_lazyso URL changes propagate automatically.tcf_website/static/reviews/club_review.js (1)
25-31:parseIntwithout radix may mis-parse octal-like IDs
parseInt(urlParams.get("club"))can interpret strings such as"08"as octal in some JS engines. Pass10explicitly:- clubId = parseInt(urlParams.get("club")); + clubId = parseInt(urlParams.get("club"), 10);tcf_website/views/browse.py (1)
137-140: Duplicateparse_modeHelpers Across Modules
parse_modenow lives in at least three files (browse.py,search.py,review.py).
Consider extracting it into a small utility (e.g.tcf_website/utils/request.py) and import everywhere to keep behaviour consistent and DRY.tcf_website/static/search/searchbar.css (1)
211-219: Duplicate Style Block – Potential Maintenance Hazard
.subject-list, .discipline-list { … }appears twice (lines 211-221 and 269-273) with identical rules. Remove one instance to avoid divergence in the future.-.subject-list, -.discipline-list { - max-height: 150px; - overflow-y: auto; - border: 1px solid #cbd5e0; - border-radius: 8px; - padding: 0.75rem; - margin-top: 0.5rem; - background: #fff; - min-height: 200px; -}(Nothing else relies on the duplicate).
tcf_website/views/search.py (2)
16-20: Consolidateparse_modeImplementationSame helper exists in
browse.py&review.py. Centralising it avoids silent divergence (e.g., default value changes).# utils/request.py def parse_mode(request, default="courses"): mode = request.GET.get("mode", default) return mode, mode == "clubs"Then
from tcf_website.utils.request import parse_mode.
152-167: No Explicit Ordering When No Query – Results May DriftWhen
query == "",fetch_clubsreturns an un-ordered queryset converted to a list.
The final list order is DB-dependent and can change between requests.Add a deterministic order, e.g. alphabetical:
return list( Club.objects + .order_by("name") .annotate(This also leverages DB-side ordering before slicing for pagination.
tcf_website/models/models.py (4)
235-238:combined_namedoes not add extra value and may break future search UX
combined_nameis currently set to the plain clubname.
That duplicates data already stored in thenamecolumn, while the trigram index is built on the new column. Two issues arise:
- You pay the storage/index cost for a second text column that stores identical data.
- The moment you want to search by category + name (e.g. “Performing Arts – Glee Club”) you’ll need a data migration and re–index.
If you really need a denormalised search column, consider concatenating
category.nameor the slug so it actually gives a richer search surface, or drop the column altogether and put the trigram index directly onname.- self.combined_name = self.name + # Richer search surface: “Category – Name” + self.combined_name = f"{self.category.name} – {self.name}"Alternatively, drop
combined_nameand its index to avoid redundant data.
788-799: Variable naming is misleading – considerascendinginstead ofreverse
reverse = order != "desc"makesreverse=Truemean ascending. That is
hard to reason about, and inconsistent with Python’ssorted(reverse=True)
semantics. Recommend renaming toascending(or invert the boolean).This change is purely for maintainability/readability.
935-940: Redundant indexes onCourseInstructorGradeYou create four indexes:
- (
course,instructor)- (
instructor,course)- (
course)- (
instructor)The composite indexes (1) and (2) already cover the single–column look-ups
for their leftmost columns, so (3) and (4) are usually unnecessary and just
bloat disk / slow writes. Suggest dropping the single-column indexes unless
you have a proven query plan that cannot leverage the composite ones.
1345-1347:__str__prints ‘None’ for club reviewsWhen
courseorinstructoris null the current__str__yields
“Review by Alice for None taught by None”.
Return a friendlier string, e.g.:- return f"Review by {self.user} for {self.course} taught by {self.instructor}" + target = self.club or self.course or "unknown target" + return f"Review by {self.user} for {target}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tcf_website/management/commands/club_data/csv/clubs.csvis excluded by!**/*.csv
📒 Files selected for processing (54)
Dockerfile(1 hunks)scripts/container-startup.sh(1 hunks)tcf_core/context_processors.py(1 hunks)tcf_website/admin.py(2 hunks)tcf_website/api/enrollment.py(3 hunks)tcf_website/api/serializers.py(1 hunks)tcf_website/api/urls.py(1 hunks)tcf_website/api/views.py(1 hunks)tcf_website/management/commands/fetch_clubs.py(1 hunks)tcf_website/management/commands/fetch_data.py(1 hunks)tcf_website/management/commands/fetch_enrollment.py(1 hunks)tcf_website/management/commands/load_clubs.py(1 hunks)tcf_website/management/commands/load_grades.py(1 hunks)tcf_website/management/commands/load_review_drive.py(1 hunks)tcf_website/migrations/0021_clubcategory_alter_review_course_and_more.py(1 hunks)tcf_website/migrations/0022_coursegrade_tcf_website_course__76e103_idx_and_more.py(1 hunks)tcf_website/models/__init__.py(1 hunks)tcf_website/models/models.py(11 hunks)tcf_website/static/browse/browse.css(1 hunks)tcf_website/static/club/mode_toggle.css(1 hunks)tcf_website/static/club/mode_toggle.js(1 hunks)tcf_website/static/reviews/club_review.js(1 hunks)tcf_website/static/reviews/new_review.js(1 hunks)tcf_website/static/reviews/review.js(1 hunks)tcf_website/static/search/filters.js(1 hunks)tcf_website/static/search/searchbar.css(1 hunks)tcf_website/templates/browse/browse.html(3 hunks)tcf_website/templates/browse/school.html(1 hunks)tcf_website/templates/club/browse_category.html(1 hunks)tcf_website/templates/club/category.html(1 hunks)tcf_website/templates/club/club.html(1 hunks)tcf_website/templates/club/mode_toggle.html(1 hunks)tcf_website/templates/common/pagination.html(1 hunks)tcf_website/templates/course/course.html(0 hunks)tcf_website/templates/department/department.html(1 hunks)tcf_website/templates/landing/landing.html(1 hunks)tcf_website/templates/reviews/edit_review.html(1 hunks)tcf_website/templates/reviews/new_review.html(2 hunks)tcf_website/templates/reviews/review.html(3 hunks)tcf_website/templates/reviews/review_form_content.html(1 hunks)tcf_website/templates/reviews/reviews.html(1 hunks)tcf_website/templates/reviews/zero_hours_modal.html(1 hunks)tcf_website/templates/search/search.html(5 hunks)tcf_website/templates/search/searchbar.html(4 hunks)tcf_website/tests/test_browse.py(0 hunks)tcf_website/urls.py(1 hunks)tcf_website/utils/enrollment.py(0 hunks)tcf_website/views/__init__.py(1 hunks)tcf_website/views/browse.py(8 hunks)tcf_website/views/profile.py(1 hunks)tcf_website/views/qa.py(1 hunks)tcf_website/views/review.py(9 hunks)tcf_website/views/schedule.py(1 hunks)tcf_website/views/search.py(6 hunks)
💤 Files with no reviewable changes (3)
- tcf_website/templates/course/course.html
- tcf_website/tests/test_browse.py
- tcf_website/utils/enrollment.py
🧰 Additional context used
🧬 Code Graph Analysis (7)
tcf_website/api/urls.py (1)
tcf_website/api/views.py (2)
ClubCategoryViewSet(161-165)ClubViewSet(168-183)
tcf_website/urls.py (1)
tcf_website/views/browse.py (1)
club_category(539-573)
tcf_website/static/reviews/new_review.js (1)
tcf_website/static/reviews/club_review.js (2)
params(17-17)urlParams(22-22)
tcf_website/static/reviews/club_review.js (1)
tcf_website/static/reviews/new_review.js (12)
params(18-18)urlParams(23-23)i(246-246)review(170-170)numberOfWords(171-171)encouragedWordCount(172-172)numberOfWordsInMessage(180-185)semEndpoint(148-148)arrayOfWords(240-240)countNonAlphaWords(243-243)allNonAlpha(248-248)j(249-249)
tcf_website/api/enrollment.py (1)
tcf_website/models/models.py (8)
Course(537-873)CourseEnrollment(876-883)Section(943-993)SectionEnrollment(1056-1100)Semester(461-522)save(235-238)save(437-439)save(567-571)
tcf_website/admin.py (1)
tcf_website/models/models.py (2)
Club(220-250)ClubCategory(204-217)
tcf_website/views/search.py (3)
tcf_website/models/models.py (1)
Club(220-250)tcf_website/views/browse.py (1)
parse_mode(137-140)tcf_website/views/review.py (1)
parse_mode(23-26)
🪛 Ruff (0.8.2)
tcf_website/models/__init__.py
9-9: .models.Answer imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
10-10: .models.Course imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
11-11: .models.Club imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
12-12: .models.ClubCategory imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
13-13: .models.CourseEnrollment imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
14-14: .models.CourseGrade imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
15-15: .models.CourseInstructorGrade imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
16-16: .models.Department imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
17-17: .models.Discipline imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
18-18: .models.Instructor imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
19-19: .models.Question imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
20-20: .models.Review imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
21-21: .models.Schedule imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
22-22: .models.ScheduledCourse imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
23-23: .models.School imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
24-24: .models.Section imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
25-25: .models.SectionEnrollment imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
26-26: .models.SectionTime imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
27-27: .models.Semester imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
28-28: .models.Subdepartment imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
29-29: .models.User imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
30-30: .models.Vote imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
tcf_website/views/__init__.py
7-7: .auth.collect_extra_info imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
7-7: .auth.login imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
7-7: .auth.login_error imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
7-7: .auth.logout imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
7-7: .auth.password_error imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
9-9: .browse.browse imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
10-10: .browse.course_instructor imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
11-11: .browse.course_view imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
12-12: .browse.course_view_legacy imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
13-13: .browse.department imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
14-14: .browse.instructor_view imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
15-15: .browse.club_category imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
17-17: .index.AboutView imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
17-17: .index.index imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
17-17: .index.privacy imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
17-17: .index.terms imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
18-18: .profile.DeleteProfile imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
18-18: .profile.profile imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
18-18: .profile.reviews imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
20-20: .qa.DeleteAnswer imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
21-21: .qa.DeleteQuestion imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
22-22: .qa.downvote_answer imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
23-23: .qa.downvote_question imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
24-24: .qa.edit_answer imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
25-25: .qa.edit_question imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
26-26: .qa.new_answer imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
27-27: .qa.new_question imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
28-28: .qa.upvote_answer imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
29-29: .qa.upvote_question imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
31-31: .review.DeleteReview imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
31-31: .review.downvote imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
31-31: .review.edit_review imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
31-31: .review.new_review imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
31-31: .review.upvote imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
33-33: .schedule.delete_schedule imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
34-34: .schedule.duplicate_schedule imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
35-35: .schedule.edit_schedule imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
36-36: .schedule.modal_load_editor imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
37-37: .schedule.modal_load_sections imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
38-38: .schedule.new_schedule imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
39-39: .schedule.schedule_add_course imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
40-40: .schedule.view_schedules imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
41-41: .schedule.view_select_schedules_modal imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
tcf_website/admin.py
176-176: Club may be undefined, or defined from star imports
(F405)
177-177: ClubCategory may be undefined, or defined from star imports
(F405)
🪛 Biome (1.9.4)
tcf_website/static/reviews/club_review.js
[error] 167-167: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: django
- GitHub Check: pylint
🔇 Additional comments (40)
tcf_website/views/qa.py (1)
8-8: Consolidated import for LoginRequiredMixin improves consistency
This single‐line import aligns with the formatting in other view modules (e.g.,profile.py) and has no impact on behavior.tcf_website/views/profile.py (1)
7-7: Unified import of LoginRequiredMixin is spot‐on
Switching to a single‐line import maintains consistency across views without altering functionality.tcf_website/management/commands/load_review_drive.py (1)
12-12: Simplified import grouping is clean and PEP8-compliant.
The consolidated model import maintains alphabetical order and correct separation from standard-library and third-party imports.Dockerfile (1)
32-34: Executable permission set for container-startup script.
Thechmod +x /app/scripts/container-startup.shensures the startup script is runnable. Please verify that:
- The file exists in
/app/scripts/.- It begins with a valid shebang (e.g.,
#!/bin/bash).tcf_website/static/reviews/review.js (1)
70-73: Verify Bootstrap version compatibility for tooltips.
This initialization usesdata-toggle="tooltip", which is the Bootstrap 4 convention. If the codebase has migrated to Bootstrap 5, update todata-bs-toggle="tooltip"or adjust the selector accordingly to ensure tooltips function.tcf_website/management/commands/load_grades.py (1)
13-13: Clean import simplification.The import statement has been simplified to consolidate multiple imports into a single line, which is a good practice for readability when the list of imports is manageable.
tcf_website/models/__init__.py (1)
8-31: Good restructuring and addition of new models.The import format has been updated from a single-line tuple to a multi-line format, which improves readability. The addition of the
ClubandClubCategorymodels aligns with the broader feature addition for club support throughout the application.Note: Static analysis shows these imports as "unused" but this is a false positive. In Django's pattern for organizing models in packages, these imports are re-exported to be available when importing from
tcf_website.modelselsewhere in the application.🧰 Tools
🪛 Ruff (0.8.2)
9-9:
.models.Answerimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
10-10:
.models.Courseimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
11-11:
.models.Clubimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
12-12:
.models.ClubCategoryimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
13-13:
.models.CourseEnrollmentimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
14-14:
.models.CourseGradeimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
15-15:
.models.CourseInstructorGradeimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
16-16:
.models.Departmentimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
17-17:
.models.Disciplineimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
18-18:
.models.Instructorimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
19-19:
.models.Questionimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
20-20:
.models.Reviewimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
21-21:
.models.Scheduleimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
22-22:
.models.ScheduledCourseimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
23-23:
.models.Schoolimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
24-24:
.models.Sectionimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
25-25:
.models.SectionEnrollmentimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
26-26:
.models.SectionTimeimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
27-27:
.models.Semesterimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
28-28:
.models.Subdepartmentimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
29-29:
.models.Userimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
30-30:
.models.Voteimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
tcf_website/templates/reviews/reviews.html (1)
1-95: Formatting improvements look good.The changes to this file are purely formatting-related (whitespace adjustments and consistent indentation) with no functional modifications. These changes improve code readability while maintaining the same functionality for rendering review listings, pagination, and JavaScript event bindings.
tcf_core/context_processors.py (1)
66-66: Good addition of minimum GPA filter.The new context variable
min_gpaproperly retrieves the minimum GPA filter value from the session's saved filters, defaulting to0.0if not set. This change aligns with the enhanced search functionality being introduced across the application.tcf_website/management/commands/fetch_data.py (1)
26-26: LGTM: Line spacing changeThis is a minor spacing change that improves code readability without affecting functionality.
tcf_website/urls.py (1)
9-13: Good addition of the club category URL patternThis new URL pattern properly routes requests to the club category view function, which supports browsing clubs by category. The implementation follows Django's URL pattern conventions with a named capture group for the category slug.
tcf_website/templates/browse/school.html (1)
13-13: UI behavior change: Schools collapsed by defaultRemoving the
showclass means school details will be collapsed by default, requiring user interaction to expand them. This creates a more consistent browsing experience, especially when considering the club-related UI changes in this PR.tcf_website/views/schedule.py (2)
10-10: LGTM: Import formatting improvementsThe parentheses and line breaks have been removed from the import statements, improving code readability while maintaining the same functionality.
Also applies to: 16-16
19-19: Added pylint directive to suppress duplicate code warningsThis addition helps suppress false positives from the linter for code patterns that might be similar to other files but are necessary in this context.
tcf_website/views/__init__.py (1)
7-16: Good addition of club_category to support club functionalityThe addition of the
club_categoryimport aligns well with the PR's objective of adding comprehensive club support. The reformatting of imports to use parentheses and one item per line with trailing commas is a good practice that improves readability and makes future additions easier.Note: Don't worry about the static analysis warnings about unused imports - this is normal behavior for
__init__.pyfiles which re-export views.🧰 Tools
🪛 Ruff (0.8.2)
7-7:
.auth.collect_extra_infoimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
7-7:
.auth.loginimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
7-7:
.auth.login_errorimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
7-7:
.auth.logoutimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
7-7:
.auth.password_errorimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
9-9:
.browse.browseimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
10-10:
.browse.course_instructorimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
11-11:
.browse.course_viewimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
12-12:
.browse.course_view_legacyimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
13-13:
.browse.departmentimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
14-14:
.browse.instructor_viewimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
15-15:
.browse.club_categoryimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
tcf_website/api/urls.py (1)
16-17: Well-structured API endpoints for club functionalityThe addition of API routes for club categories and clubs follows REST API best practices:
- Uses kebab-case for multi-word resources (
club-categories)- Uses plural nouns for collection endpoints
- Matches the corresponding view implementation with appropriate filtering
These endpoints will properly support the new club browsing and searching features.
tcf_website/templates/reviews/edit_review.html (2)
8-8: Good addition of mode toggle CSSAppropriate inclusion of the CSS file for the mode toggle component to support the dual mode functionality.
13-19: Well-structured header with mode toggleThe header has been thoughtfully restructured to include both the title and the mode toggle component. The use of flexbox with
justify-content-betweenensures proper spacing between elements. The Bootstrap classes are used correctly for responsive layout.The toggle implementation allows users to seamlessly switch between course and club review modes.
tcf_website/templates/reviews/zero_hours_modal.html (1)
12-20: Good context-aware messaging for zero hours reviewsThe conditional messaging based on the
is_clubvariable provides appropriate context-specific language:
- For clubs: refers to "participation" in clubs
- For courses: maintains the original text about "work" for classes
This small but important change ensures that the verification message is relevant to the type of entity being reviewed.
tcf_website/admin.py (2)
153-156: Admin configuration for Club model looks good.The admin class follows the established pattern in the file and provides sensible ordering and search fields.
158-161: Admin configuration for ClubCategory model looks good.The admin class properly configures ordering and search by name, consistent with other admin classes in the file.
tcf_website/static/reviews/new_review.js (1)
21-36: Good improvement using URLSearchParams for query parsing.Refactoring to use the
URLSearchParamsAPI is a modern approach that provides better handling of query parameters. The added checks for parameter existence before parsing make the code more robust against missing parameters.tcf_website/management/commands/fetch_enrollment.py (1)
76-81: Inlined URL construction simplifies the code.Replacing the utility function call with direct URL construction removes a dependency and makes the function more self-contained. The multi-line string formatting is also easy to read.
tcf_website/api/serializers.py (3)
5-14: Model imports are properly updated.The imports are correctly updated to include the new
ClubandClubCategorymodels.
163-169: ClubCategory serializer follows established patterns.The serializer is well-structured and follows the same patterns as other model serializers in the file.
171-178: Club serializer with nested category is well-designed.The
ClubSerializercorrectly includes a nestedClubCategorySerializerfor the category field, making it consistent with how other relationships are handled in the codebase (likeCourseSerializerwith nestedSubdepartmentSerializer).tcf_website/templates/department/department.html (1)
43-43: Use of common pagination component
Replacing inline pagination with the sharedcommon/pagination.htmlDRYs up the code and ensures consistent behavior across pages. Make sure that any required CSS (e.g.,common/pagination.css) is being loaded globally or in the base template since the oldreviews/pagination.cssreference was removed.tcf_website/templates/club/browse_category.html (1)
15-18: Verify URL pattern for club detail
The link uses{% url 'course' mnemonic=category.slug course_number=club.id %}?mode=clubs. If there is a dedicatedclub_detailview (e.g.,club/<slug>/), prefer using that for clarity. Otherwise, confirm that thecourseendpoint withmode=clubsis the intended club landing page.tcf_website/migrations/0022_coursegrade_tcf_website_course__76e103_idx_and_more.py (1)
13-55: Migration indexing is well-structured
The migration cleanly adds per-model and composite indexes with unique, descriptive names. This will optimize query performance for grades and reviews.tcf_website/templates/reviews/review.html (1)
15-27: Verify conditional header links for club reviews
The template toggles between course-instructor and club views, linking clubs via thecourseURL with?mode=clubs. Please confirm that this is the intended endpoint for club details—if a dedicated club route exists, switching to it would improve clarity.tcf_website/templates/club/category.html (1)
45-47: Link to club detail, not course
The<a href="{% url 'course' ... %}?mode=clubs">points to the course view. It should use the club detail URL pattern (e.g.{% url 'club_detail' category_slug=category.slug club_id=club.id %}). Please verify the correct route name and parameters.tcf_website/templates/browse/browse.html (2)
6-9: Include club toggle stylesheet
Addingclub/mode_toggle.cssis correct; please ensure the static file path matches and that the CSS is loaded only when needed to avoid unnecessary payload for course-only pages.
21-27: Verifyis_clubcontext and use absolute include paths
Ensure the view providesis_clubandclub_categories. Also, consider using{% include "club/mode_toggle.html" %}(absolute path) instead of a relative reference to avoid include resolution issues.scripts/container-startup.sh (1)
12-16: Validate theload_clubscommand
Ensure theload_clubsmanagement command exists and handles idempotency. If it fails, the container will exit; consider adding retries or a health check.tcf_website/static/search/filters.js (2)
57-63: Potential accidental multiple form submissionsInside the Enter-key handler you blindly click the first
'button[type="submit"]'.
If there are multiple forms on the page (e.g., advanced search + feedback), this may submit the wrong one.Consider instead:
this.form.requestSubmit(); // modern browsersor scope the selector to
this.closest('form').
236-259: Mobile resize auto-reset may surprise usersSilently wiping all filters when the viewport dips below 992 px can frustrate users who rotate their device or resize the window.
Instead of destructive resets, consider merely collapsing the dropdown or showing a toast asking the user to reset manually.tcf_website/static/browse/browse.css (1)
26-33: Ensure CSS custom properties existThe selectors rely on
--secondary-color,--main-color,--dark-color, and--accent-color. If any page is rendered without the root palette (e.g., email templates or admin), these will fallback toinherit, hurting contrast.Verify the variables are defined globally or add fallbacks:
background-color: var(--secondary-color, #005f99);tcf_website/templates/search/searchbar.html (1)
120-142:min_gpacontrols lackname→ server never receives the value without JSThe slider
#min-gpa-sliderand text box hold the user’s choice, but neither element has anameattribute.
Only the hidden input (#min-gpa-input) is submitted, and it depends on client-side JS (filters.js) to keep it in sync. Without JS the field silently breaks, degrading accessibility.Recommendation: give the slider a
name="min_gpa"and drop the hidden field, or keep both but initialise them consistently server-side.tcf_website/models/models.py (2)
245-250: Ensurepg_trgmis enabled in migrations
GinIndex(opclasses=["gin_trgm_ops"])silently fails at runtime if the
pg_trgmextension is missing.
Add a migration step that runsCREATE EXTENSION IF NOT EXISTS pg_trgm;
(or usedjango.contrib.postgres.operations.TrigramExtension) before the
model migration so deploys don’t break.
848-851: Good use ofOuterReffor open section filteringThe rewritten sub-query is concise and avoids an unnecessary
.exists()call.
|
looks good mr president, but please get better commit messages T_T |
…board navigation users
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
tcf_website/management/commands/fetch_data.py (4)
66-66: 🛠️ Refactor suggestionUpdate error handling to use command's output methods.
Replace direct print statements with Django command's output methods for consistent error handling.
- print(f"An error occurred: {e}") + self.stderr.write(self.style.ERROR(f"An error occurred: {e}"))
68-68: 🛠️ Refactor suggestionUse command's output methods for consistency.
Replace print statement with Django command's output methods.
- print(f"Total pages: {total_pages}") + self.stdout.write(f"Total pages: {total_pages}")
78-78: 🛠️ Refactor suggestionUpdate error handling to use command's output methods.
Replace print statement with Django command's output methods for consistent error handling.
- print(f"An error occurred: {e}") + self.stderr.write(self.style.ERROR(f"An error occurred: {e}"))
98-98: 🛠️ Refactor suggestionUse command's output methods for consistency.
Replace print statement with Django command's output methods.
- print("Data fetching complete.") + self.stdout.write(self.style.SUCCESS("Data fetching complete."))
♻️ Duplicate comments (10)
tcf_website/management/commands/fetch_clubs.py (2)
76-77:⚠️ Potential issueImprove error handling with proper logging.
Current error handling just prints to the console. For a management command, it's better to use Django's logging system or the command's stdout/stderr methods.
- print(f"Error {uri}: {e}") + self.stderr.write(self.style.ERROR(f"Error processing {uri}: {e}"))This requires passing
selfto theprocess_organizationfunction from theCommandclass.
101-101: 🛠️ Refactor suggestionUse command's output methods instead of print statement.
Use Django's command output methods for consistent styling and proper logging.
- print("Done.") + self.stdout.write("Done.")tcf_website/static/search/filters.js (4)
19-21: EnsureopenSectionsexists before adding event listener.The checkbox only exists on course search pages but not club pages, which can cause "cannot read property 'addEventListener' of null" errors.
Apply this fix to avoid JavaScript errors:
-[...filterInputs, ...dayFilters, openSections].forEach((input) => { +[...filterInputs, ...dayFilters, openSections] + .filter(Boolean) + .forEach((input) => { input.addEventListener("change", updateButtonState); });
27-33:⚠️ Potential issueAdd null checks before attaching event listeners to GPA elements.
The GPA slider and text input elements only exist on course search pages. When this script runs on club pages, these elements will be null, causing JavaScript errors.
Add null checks before attaching event listeners:
-minGpaSlider.addEventListener("input", function () { +if (minGpaSlider) { + minGpaSlider.addEventListener("input", function () { const value = parseFloat(this.value).toFixed(1); // Ensures clean decimal minGpaText.value = value; minGpaInput.value = value; updateButtonState(); - }); + }); +}
57-63:⚠️ Potential issueAdd null check before adding keyup event listener to minGpaText.
The event listener is attached unconditionally, but the element might not exist on club pages.
Add a null check:
-minGpaText.addEventListener("keyup", function (event) { +if (minGpaText) { + minGpaText.addEventListener("keyup", function (event) { if (event.key === "Enter") { event.preventDefault(); this.blur(); // Remove focus to trigger change event document.querySelector('button[type="submit"]').click(); // Submit the form } - }); + }); +}
35-54:⚠️ Potential issueAdd null checks before attaching event listeners to minGpaText.
Similar to the GPA slider issue, the text input needs null checks before attaching event listeners.
Add null checks:
-minGpaText.addEventListener("change", function () { +if (minGpaText) { + minGpaText.addEventListener("change", function () { // Only update if valid number let value = parseFloat(this.value); // Validate and constrain the value if (isNaN(value)) { value = 0.0; // Default to 0.0 if invalid } else { // Keep value between 0 and 4 value = Math.max(0, Math.min(4, value)); // Round to nearest 0.1 value = Math.round(value * 10) / 10; } // Update all inputs with the cleaned value this.value = value.toFixed(1); minGpaSlider.value = value; minGpaInput.value = value; updateButtonState(); - }); + }); +}tcf_website/views/browse.py (3)
169-171: Fix case-sensitivity risk when resolving category slugs.Using
category__slug=mnemonic.upper()assumes that all stored slugs are uppercase. This might lead to lookup failures if slugs are stored in different casings.Use a case-insensitive lookup instead:
-club = get_object_or_404( - Club, id=course_number, category__slug=mnemonic.upper() -) +club = get_object_or_404( + Club, id=course_number, category__slug__iexact=mnemonic +)
547-551: Same slug-casing issue inclub_categoryview.The
ClubCategory.objects.get(slug=category_slug.upper())has the same case-sensitivity issue as the club lookup.Use a case-insensitive lookup instead:
-category = get_object_or_404(ClubCategory, slug=category_slug.upper()) +category = get_object_or_404(ClubCategory, slug__iexact=category_slug)
558-564:⚠️ Potential issueHandle
PageNotAnIntegerin Pagination.Only
EmptyPageis caught;PageNotAnIntegercan still bubble up and cause a 500 error.Reuse the import that already exists at the top:
try: paginated_clubs = paginator.page(page_number) -except EmptyPage: +except (PageNotAnInteger, EmptyPage): paginated_clubs = paginator.page(paginator.num_pages)tcf_website/models/models.py (1)
1110-1124:⚠️ Potential issueMissing integrity guarantee between
course,club, andinstructor.The model allows any combination of
course,club,instructor(including all three being null), which could lead to orphaned reviews or runtime errors.Add model-level validation with CheckConstraints:
class Review(models.Model): # ... existing code ... class Meta: # ... existing indexes ... + constraints = [ + models.CheckConstraint( + check=( + # XOR: one and only one of course / club + ( + (Q(course__isnull=False) & Q(club__isnull=True)) | + (Q(course__isnull=True) & Q(club__isnull=False)) + ) + ), + name="review_course_xor_club", + ), + models.CheckConstraint( + check=( + # instructor required if course review + Q(course__isnull=True) | Q(instructor__isnull=False) + ), + name="review_instructor_required_for_course", + ), + ]
🧹 Nitpick comments (14)
tcf_website/management/commands/fetch_clubs.py (3)
19-21: Consider setting a more reasonable timeout value.The current timeout of 300 seconds (5 minutes) seems excessive for API calls. Consider reducing this to a more reasonable value like 60 seconds, especially since you're already implementing retry logic with backoff.
28-34: Use typed arguments for backoff decorator.For better code maintainability, consider specifying the argument types in the
backoff.on_exceptiondecorator.@backoff.on_exception( backoff.expo, (requests.exceptions.Timeout, requests.exceptions.ConnectionError), max_tries=5, + jitter=backoff.full_jitter, )
115-118: Command output could show more details about processing.The command output is quite minimal. Consider adding more information such as the number of clubs fetched and any processing statistics.
def handle(self, *args, **options): csv_file = options["output"] self.stdout.write(f"Fetching club data and writing to {csv_file}...") write_csv(csv_file) - self.stdout.write(self.style.SUCCESS("Successfully fetched club data.")) + with open(csv_file, 'r') as f: + club_count = sum(1 for _ in f) - 1 # Subtract 1 for header + self.stdout.write( + self.style.SUCCESS(f"Successfully fetched {club_count} clubs and saved to {csv_file}.") + )tcf_website/static/club/mode_toggle.js (2)
8-8: Fix indentation as flagged by ESLint.Remove extra whitespace to maintain consistent indentation.
const coursesRadio = document.getElementById("search-mode-courses"); const clubsRadio = document.getElementById("search-mode-clubs"); - + // Abort early if the expected inputs are missing if (!coursesRadio || !clubsRadio) { console.error( "mode_toggle.js: Could not find expected #search-mode-(courses|clubs) radios.", ); return; } - + const filterButtonElement = document.getElementById("filter-button");Also applies to: 16-16
🧰 Tools
🪛 GitHub Check: eslint
[failure] 8-8:
Delete····
20-40: Consider adding debounce to event handlers.For radio button changes that may trigger multiple events or expensive DOM updates, consider adding a debounce function to prevent rapid successive calls.
+ // Simple debounce function + function debounce(func, wait) { + let timeout; + return function() { + const context = this; + const args = arguments; + clearTimeout(timeout); + timeout = setTimeout(() => func.apply(context, args), wait); + }; + } // If filter button exists, handle toggle state affecting filter button if (filterButtonElement) { function updateFilterButtonState() { if (clubsRadio.checked) { filterButtonElement.disabled = true; filterButtonElement.setAttribute("aria-disabled", "true"); filterButtonElement.classList.add("disabled"); } else { filterButtonElement.disabled = false; filterButtonElement.setAttribute("aria-disabled", "false"); filterButtonElement.classList.remove("disabled"); } } // Set initial state updateFilterButtonState(); // Listen for changes - coursesRadio.addEventListener("change", updateFilterButtonState); - clubsRadio.addEventListener("change", updateFilterButtonState); + const debouncedUpdate = debounce(updateFilterButtonState, 100); + coursesRadio.addEventListener("change", debouncedUpdate); + clubsRadio.addEventListener("change", debouncedUpdate); }tcf_website/management/commands/fetch_data.py (1)
239-269: Legacy comparison function needs update.The
compare_csv_filesfunction still uses direct print statements. Consider updating it to use the command's output methods or making it a separate command.This function doesn't appear to be used in the current command implementation. Consider either removing it or updating it to use the command's output methods.
🧰 Tools
🪛 Ruff (0.8.2)
254-254: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
266-266: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
doc/useful-commands.md (2)
89-89: Fix markdownlint warnings about command formatting.The documentation has markdownlint warnings about using dollar signs before commands without showing output (MD014). Consider removing the dollar signs or showing example output.
-```console -$ docker exec -it tcf_django python manage.py fetch_data <year>_<season> +```bash +docker exec -it tcf_django python manage.py fetch_data <year>_<season>Apply similar changes to the other command examples.
Also applies to: 101-101, 111-111, 123-123
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
89-89: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
94-94: Consider using relative links for file references.The documentation uses absolute GitHub URLs for file references. Consider using relative links for better maintainability across branches and forks.
-See [fetch_data.py](https://github.com/thecourseforum/theCourseForum2/blob/dev/tcf_website/management/commands/fetch_data.py) for more information. +See [fetch_data.py](../tcf_website/management/commands/fetch_data.py) for more information.Apply similar changes to other file references.
tcf_website/static/search/filters.js (2)
72-72: Use optional chaining for openSections.Using optional chaining is a more concise way to handle potentially null references.
- (openSections && openSections.checked) || + openSections?.checked ||🧰 Tools
🪛 Biome (1.9.4)
[error] 72-72: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
98-106: Filter elements found in setupSearch.The
setupSearchfunction doesn't check if any elements are found by the selector, which could lead to errors if the structure changes.Add a check to avoid potential errors:
function setupSearch(searchInput, itemsSelector) { const items = document.querySelectorAll(itemsSelector); + if (!searchInput || items.length === 0) return; searchInput.addEventListener("input", function (e) { const searchTerm = e.target.value.toLowerCase(); items.forEach((item) => { const text = item.textContent.toLowerCase(); item.style.display = text.includes(searchTerm) ? "" : "none"; }); }); }tcf_website/views/search.py (2)
335-344: Optimize min_gpa filter to prevent duplicate rows.Filtering directly with
coursegrade__average__gtecan cause row duplication before thedistinct()call, potentially impacting performance.Consider using annotation first to aggregate the GPA values:
min_gpa = filters.get("min_gpa") if min_gpa: try: min_gpa_float = float(min_gpa) - # Filter directly using a subquery on CourseGrade - results = results.filter(coursegrade__average__gte=min_gpa_float) + # Use annotation to aggregate first, then filter + results = results.annotate( + avg_gpa=Avg("coursegrade__average") + ).filter(avg_gpa__gte=min_gpa_float) except (ValueError, TypeError): # Silently ignore invalid values pass
149-187: Consider adding more specific error handling in fetch_clubs.The current implementation doesn't have specific error handling for invalid query parameters or database errors.
Consider adding try/except blocks to handle potential database errors and log or return appropriate responses:
def fetch_clubs(query): """Get club data using Django Trigram similarity.""" threshold = 0.15 + try: if not query: return list( Club.objects.annotate( max_similarity=Value(1.0, output_field=FloatField()), category_slug=F("category__slug"), category_name=F("category__name"), ).values( "id", "name", "description", "max_similarity", "category_slug", "category_name", ) ) qs = ( Club.objects.annotate(sim=TrigramSimilarity("combined_name", query)) .annotate(max_similarity=F("sim")) .filter(max_similarity__gte=threshold) .annotate(category_slug=F("category__slug"), category_name=F("category__name")) .order_by("-max_similarity") ) return [ { "id": c.id, "name": c.name, "description": c.description, "max_similarity": c.max_similarity, "category_slug": c.category_slug, "category_name": c.category_name, } for c in qs ] + except Exception as e: + # Log the error + print(f"Error fetching clubs: {e}") + # Return empty list instead of raising error + return []tcf_website/models/models.py (2)
229-234: Consider adding more descriptive fields to the Club model.The current model has some basic fields, but you might want to consider adding more fields for a comprehensive club representation.
Consider adding more fields:
class Club(models.Model): """Club model. Belongs to a ClubCategory. Has many Reviews. """ name = models.CharField(max_length=255) description = models.TextField(blank=True) category = models.ForeignKey(ClubCategory, on_delete=models.CASCADE) combined_name = models.CharField(max_length=255, blank=True, editable=False) application_required = models.BooleanField(default=False) photo_url = models.CharField(max_length=255, blank=True) meeting_time = models.CharField(max_length=255, blank=True) + social_media = models.JSONField(blank=True, null=True, default=dict) # Store social media links + website = models.URLField(blank=True) + email = models.EmailField(blank=True) + membership_size = models.IntegerField(blank=True, null=True) + founded_year = models.IntegerField(blank=True, null=True)
674-675: Add clarifying comment for default_value logic.The logic for
default_valueis somewhat complex and could benefit from a clear explanation.Add an explanatory comment:
-# Set default values based on whether we're using the optimized path or historical path +# Set default values based on the context: +# - Use 0 for recent data as a neutral starting value. +# - Use -1 for historical data in ascending order (reverse=False). +# - Use 1e9 for historical data in descending order (reverse=True). +# These sentinel values ensure proper sorting when actual data is missing. default_value = 0 if recent else (-1 if not reverse else 1e9)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tcf_website/management/commands/club_data/csv/clubs.csvis excluded by!**/*.csv
📒 Files selected for processing (12)
doc/useful-commands.md(1 hunks)tcf_website/api/views.py(1 hunks)tcf_website/management/commands/fetch_clubs.py(1 hunks)tcf_website/management/commands/fetch_data.py(6 hunks)tcf_website/models/models.py(11 hunks)tcf_website/static/club/mode_toggle.css(1 hunks)tcf_website/static/club/mode_toggle.js(1 hunks)tcf_website/static/search/filters.js(1 hunks)tcf_website/templates/club/category.html(1 hunks)tcf_website/templates/club/mode_toggle.html(1 hunks)tcf_website/views/browse.py(8 hunks)tcf_website/views/search.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tcf_website/templates/club/category.html
- tcf_website/templates/club/mode_toggle.html
- tcf_website/static/club/mode_toggle.css
- tcf_website/api/views.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
tcf_website/views/browse.py (3)
tcf_website/models/models.py (12)
Club(220-250)ClubCategory(204-217)Course(537-873)Department(55-161)Instructor(294-458)Review(1103-1366)School(37-52)Section(943-993)Semester(461-522)sort(1281-1322)paginate(140-150)paginate(1325-1335)tcf_website/views/search.py (1)
parse_mode(16-19)tcf_website/views/review.py (1)
parse_mode(23-26)
🪛 markdownlint-cli2 (0.17.2)
doc/useful-commands.md
89-89: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
101-101: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
111-111: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
123-123: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
🪛 GitHub Check: eslint
tcf_website/static/club/mode_toggle.js
[failure] 16-16:
Delete ····
[failure] 8-8:
Delete ····
🪛 Biome (1.9.4)
tcf_website/static/search/filters.js
[error] 72-72: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pylint
- GitHub Check: django
🔇 Additional comments (8)
tcf_website/management/commands/fetch_data.py (2)
88-88: Appropriate reduction in thread pool size.Reducing the ThreadPoolExecutor's max workers from 20 to 10 is a good change, as it helps prevent overwhelming the server with too many concurrent requests.
271-310: Well-structured Django management command.The conversion to a proper Django management command with appropriate argument handling and output formatting is well done. The command validates input format and provides helpful error messages.
doc/useful-commands.md (2)
84-94: Well-updated documentation for fetching semester data.The documentation has been properly updated to reflect the new Django management command approach, with clear examples of how to run the command in Docker.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
89-89: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
106-127: Excellent documentation for new club data features.The additions for fetching and loading club data are clear, concise, and follow the same format as other command documentation, making it easy to understand.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
111-111: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
123-123: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
tcf_website/views/browse.py (1)
208-210:✅ Verification successful
Ensure data is properly saved to session.
When saving club information to the session, ensure the URL format matches what is used elsewhere in the app.
🏁 Script executed:
#!/bin/bash # Check how the course_code is used throughout the application rg "course_code.*=" -A 3Length of output: 1092
🏁 Script executed:
#!/bin/bash # Search for `code` method definitions to see how course.code() is implemented rg "def code" -n tcf_websiteLength of output: 83
🏁 Script executed:
#!/bin/bash # Extract the implementation of the code() method for verification sed -n '600,650p' tcf_website/models/models.pyLength of output: 1998
🏁 Script executed:
#!/bin/bash # Extract club-related code in the browse view for context rg -n "club" -n tcf_website/views/browse.pyLength of output: 1988
No action needed: session “course_code” format is consistent
We verified that
Course.code()returnsf"{self.subdepartment.mnemonic} {self.number}"and the club branch uses the same space-separated pattern:
f"{club.category.slug} {club.id}"— so the session value matches the existing format.
• Location: tcf_website/views/browse.py lines 208–209
tcf_website/views/search.py (1)
56-67: Excellent implementation of reusable pagination.The
paginate_resultsfunction properly handles pagination with all necessary exception cases, and it's reused across different parts of the view. This is good code organization.tcf_website/models/models.py (2)
245-250: Well-implemented GIN index for Club search.The GIN index on combined_name with the appropriate opclass will greatly improve trigram similarity searches. This is a good implementation.
934-940: Good addition of indexes for performance optimization.Adding appropriate indexes on CourseInstructorGrade will significantly improve query performance, especially for the commonly accessed
courseandinstructorfields. The compound indexes will also help with joins.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tcf_website/models/models.py (1)
1345-1346: 🛠️ Refactor suggestionUpdate the Review model's str method.
The current str method refers only to course and instructor, but doesn't handle the case when club is set instead.
def __str__(self): - return f"Review by {self.user} for {self.course} taught by {self.instructor}" + if self.course: + return f"Review by {self.user} for {self.course} taught by {self.instructor}" + else: + return f"Review by {self.user} for club {self.club}"
♻️ Duplicate comments (5)
tcf_website/views/browse.py (3)
167-171:⚠️ Potential issueCase-sensitivity risk when resolving category slugs.
Using
category__slug=mnemonic.upper()assumes that all stored slugs are uppercase. If slugs in the database are stored in mixed case or lowercase (which is common convention), the lookup will fail.Use a case-insensitive lookup instead:
- club = get_object_or_404( - Club, id=course_number, category__slug=mnemonic.upper() - ) + club = get_object_or_404( + Club, id=course_number, category__slug__iexact=mnemonic + )
547-551:⚠️ Potential issueSame slug-casing issue in
club_categoryview.The same case sensitivity issue exists here with
ClubCategory.objects.get(slug=category_slug.upper()).Use a case-insensitive lookup instead:
- category = get_object_or_404(ClubCategory, slug=category_slug.upper()) + category = get_object_or_404(ClubCategory, slug__iexact=category_slug)
558-563:⚠️ Potential issueHandle
PageNotAnIntegerin Pagination.Only
EmptyPageis caught;PageNotAnIntegercan still bubble up and cause a 500 error.Reuse the import that already exists at the top:
-try: - paginated_clubs = paginator.page(page_number) -except EmptyPage: - paginated_clubs = paginator.page(paginator.num_pages) +try: + paginated_clubs = paginator.page(page_number) +except (PageNotAnInteger, EmptyPage): + paginated_clubs = paginator.page(paginator.num_pages)tcf_website/models/models.py (2)
203-216:⚠️ Potential issueClubCategory model needs a save method for slug normalization.
Since you're using
.upper()in the views when querying by slug, you should ensure slugs are consistently stored in uppercase in the database.Add a save method to normalize the slug:
class ClubCategory(models.Model): """ClubCategory model. Has many Clubs. """ # Human name name = models.CharField(max_length=255, unique=True) description = models.TextField(blank=True) # slug for routing in the existing course URL slug = models.SlugField(max_length=255, unique=True) + def save(self, *args, **kwargs): + # Ensure slug is always uppercase for consistent lookup + self.slug = self.slug.upper() + super().save(*args, **kwargs) + def __str__(self): return self.name
1109-1123:⚠️ Potential issueMissing integrity guarantee between
course,club, andinstructor.The Review model allows any combination of
course,club,instructorfields to be null or populated, which could lead to orphaned reviews and runtime errors in code that expects at least one foreign key to be set.Add model-level validation via
CheckConstraints in the Meta class:class Review(models.Model): # ...existing fields... class Meta: # Improve scanning of reviews by course and instructor. indexes = [ models.Index(fields=["course", "instructor"]), models.Index(fields=["user", "-created"]), models.Index(fields=["instructor", "course"]), models.Index(fields=["instructor"]), + models.Index(fields=["club"]), + models.Index(fields=["club", "user"]), ] + constraints = [ + models.CheckConstraint( + check=( + # XOR: one and only one of course / club + ( + (Q(course__isnull=False) & Q(club__isnull=True)) | + (Q(course__isnull=True) & Q(club__isnull=False)) + ) + ), + name="review_course_xor_club", + ), + models.CheckConstraint( + check=( + # instructor required if course review + Q(course__isnull=True) | Q(instructor__isnull=False) + ), + name="review_instructor_required_for_course", + ), + ]
🧹 Nitpick comments (1)
tcf_website/models/models.py (1)
673-675: Default value calculation for sorting is improved but could be clearer.The default value calculation for sorting is complex. The change from a potential large value to more reasonable defaults is good, but this could benefit from a comment explaining the rationale.
- # Set default values based on whether we're using the optimized path or historical path - default_value = 0 if recent else (-1 if not reverse else 1e9) + # Set default values based on whether we're using the optimized path or historical path + # - Use 0 for recent data as a neutral starting value + # - Use -1 for historical data in ascending order (reverse=False) + # - Use 1e9 for historical data in descending order (reverse=True) + default_value = 0 if recent else (-1 if not reverse else 1e9)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tcf_website/api/views.py(1 hunks)tcf_website/management/commands/fetch_clubs.py(1 hunks)tcf_website/management/commands/fetch_data.py(6 hunks)tcf_website/models/models.py(11 hunks)tcf_website/static/club/mode_toggle.js(1 hunks)tcf_website/views/browse.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tcf_website/management/commands/fetch_data.py
- tcf_website/api/views.py
- tcf_website/static/club/mode_toggle.js
- tcf_website/management/commands/fetch_clubs.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: pylint
- GitHub Check: django
🔇 Additional comments (12)
tcf_website/views/browse.py (9)
10-11: Good use of database optimization imports.I see you've added imports for database functions and additional query optimization tools like Prefetch and Concat. This will help with more complex queries needed for the club functionality.
17-30: Models imports organized well with new club-related models.The imports of models are cleanly organized to include the new
ClubandClubCategorymodels alongside existing ones. Good job maintaining alphabetical order in the list.
35-35: Well-designed mode parsing utility.The use of a separate
parse_modefunction is a good abstraction that will be reusable across multiple views.
37-59: Efficient club category fetching with prefetch_related.Good implementation using
prefetch_relatedwithPrefetchto optimize the database queries. This efficiently loads all clubs for each category in a single query, avoiding the N+1 query problem.
143-147: Clean implementation of mode parsing helper.This helper function effectively extracts the mode parameter from the request and returns both the mode string and a boolean indicating whether it's club mode. This simplifies the code in multiple views.
173-195: Clean and consistent pagination implementation for club reviews.The code correctly fetches reviews for clubs with appropriate filtering (toxicity, hidden, etc.) and adds vote annotations for authenticated users, following the same pattern used for courses.
196-210: Well-structured breadcrumbs implementation for club navigation.The breadcrumbs implementation for clubs maintains a consistent user experience with the existing course navigation, correctly passing mode parameters when needed.
211-222: Consistent template rendering for clubs.The template context includes all necessary data, preserves
is_clubandmodeflags consistently with other views, and follows the same pattern used for courses.
308-311: Improved query for last taught section.Good improvement changing to order by descending semester number and using
first()instead of ordering ascending and usinglast(). This is both more readable and potentially more efficient.tcf_website/models/models.py (3)
219-250: Well-structured Club model with appropriate index.The Club model is well-structured with a thoughtful selection of fields. The GIN trigram index on combined_name is excellent for supporting efficient case-insensitive text search.
905-909: Good database optimization with additional indexes.The addition of indexes for CourseGrade will improve query performance when filtering or joining on these fields. This is particularly important for frequently accessed data like course grades.
933-939: Comprehensive indexing strategy for CourseInstructorGrade.Adding multiple indexes on different field combinations shows thoughtful consideration of the various query patterns that will be used. This will improve performance for queries that filter or join on course, instructor, or both.
Summary by CodeRabbit
New Features
User Interface
Bug Fixes
Performance
Chores/Refactor
Documentation