Conversation
📝 WalkthroughWalkthroughAdds a calendar feature and Presence integration: new Presence service, calendar views/templates/URLs, tag coloring, settings for Presence, caching/retry logic, tests, and many UI/script refactors including removal of legacy API serializers/views and several client-side scripts and templates. Changes
Sequence DiagramsequenceDiagram
actor User
participant View as Django View
participant Service as Presence Service
participant Cache as Django Cache
participant API as Presence API
User->>View: GET /clubs/calendar/
activate View
View->>Service: get_events()
activate Service
Service->>Cache: Check cache (presence::{subdomain}::events)
alt Cache Hit
Cache-->>Service: Return cached events
else Cache Miss
Service->>API: HTTP GET /events (with timeout & retries)
activate API
API-->>Service: JSON events
deactivate API
Service->>Cache: Store events (TTL)
end
Service-->>View: Events list
deactivate Service
View->>View: Normalize, sort, group by date, split upcoming/past
View-->>User: Render calendar_overview.html
deactivate View
User->>View: GET /clubs/calendar/event/{event_uri}/
activate View
View->>Service: get_event_details(event_uri)
activate Service
Service->>Cache: Check cache (presence::{subdomain}::event::{uri})
alt Cache Hit
Cache-->>Service: Return cached details
else Cache Miss
Service->>API: HTTP GET /events/{uri}
activate API
API-->>Service: JSON event detail
deactivate API
Service->>Cache: Store event detail (TTL)
end
Service-->>View: Event detail or None
deactivate Service
alt Event Found
View-->>User: Render event_detail.html
else Event Not Found
View-->>User: HTTP 404
end
deactivate View
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
tcf_website/services/presence.py (1)
22-41: Consider documenting exception behavior.The functions
get_events()andget_event_details()will raise exceptions (via_get) if the API is unreachable after retries or returns HTTP errors. Consider adding docstring notes about what exceptions callers should expect to handle.Example docstring addition:
def get_events(): """ Returns a list of upcoming events from Presence. Cached briefly to avoid rate limits. Raises: requests.RequestException: If the API is unreachable after retries requests.HTTPError: If the API returns an error status code """tcf_website/tests/test_calendar_overview.py (2)
6-8: Use Django'soverride_settingsdecorator instead of direct mutation.Directly mutating
settings.ENABLE_CLUB_CALENDARcan cause issues in test isolation and doesn't follow Django testing best practices.Apply this diff:
+from django.test import TestCase, Client, override_settings -from django.test import TestCase, Client from django.conf import settings +@override_settings(ENABLE_CLUB_CALENDAR=True) class CalendarOverviewTests(TestCase): + """Test cases for the calendar overview view.""" + def setUp(self): - settings.ENABLE_CLUB_CALENDAR = True self.client = Client()
10-12: Consider asserting a specific expected status code.The test currently accepts both 200 and 302, which makes it less precise. If the calendar should always return 200 when enabled (or always redirect), consider asserting the specific expected status code for clearer test semantics.
tcf_website/templates/calendar/calendar_overview.html (1)
39-119: Consider extracting event card into a reusable template.The event card rendering logic is duplicated between past events (lines 39-72) and upcoming events (lines 87-119). This could be refactored into a reusable template include to improve maintainability.
You could create a new file
tcf_website/templates/calendar/_event_card.html:<div class="card rating-card mb-2"> <div class="row no-gutters"> <div class="col-md-8"> <div class="card-body"> <div class="fw-bold text-dark mb-1">{{ event.name }}</div> <div class="mb-2"> <span class="text-primary fw-semibold">{{ event.org }}</span> {% if event.location %} • <span class="text-muted">{{ event.location }}</span>{% endif %} </div> {% if event.tags and event.tags|length > 0 %} <div class="mb-2"> {% for t in event.tags|slice:":3" %} <span class="badge {{ t|tag_color }} text-white me-1">{{ t }}</span> {% endfor %} </div> {% endif %} {% if event.desc_text %} <div class="text-muted text-truncate" style="max-width: 700px;">{{ event.desc_text }}</div> {% endif %} </div> </div> <div class="col-md-4 d-flex align-items-center justify-content-center"> <a class="btn btn-outline-primary btn-sm" href="{% url 'event_detail' event_uri=event.uri %}" title="View event details"> Details </a> </div> </div> </div>Then include it using:
{% include "calendar/_event_card.html" with event=ev %}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
tcf_core/context_processors.py(1 hunks)tcf_core/settings/base.py(2 hunks)tcf_website/services/presence.py(1 hunks)tcf_website/templates/base/sidebar.html(1 hunks)tcf_website/templates/calendar/calendar_overview.html(1 hunks)tcf_website/templates/calendar/disabled.html(1 hunks)tcf_website/templates/calendar/event_detail.html(1 hunks)tcf_website/templatetags/custom_tags.py(2 hunks)tcf_website/tests/test_calendar_overview.py(1 hunks)tcf_website/urls.py(1 hunks)tcf_website/views/__init__.py(1 hunks)tcf_website/views/calendar.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tcf_website/urls.py (1)
tcf_website/views/calendar.py (2)
calendar_overview(43-101)event_detail(104-144)
tcf_website/views/__init__.py (1)
tcf_website/views/calendar.py (2)
calendar_overview(43-101)event_detail(104-144)
tcf_website/views/calendar.py (1)
tcf_website/services/presence.py (2)
get_events(22-31)get_event_details(34-40)
🪛 GitHub Actions: Continuous Integration
tcf_website/tests/test_calendar_overview.py
[error] 14-14: Pylint: trailing newlines (trailing-newlines)
[error] 1-1: Pylint: Missing module docstring (missing-module-docstring)
[error] 5-5: Pylint: Missing class docstring (missing-class-docstring)
[error] 10-10: Pylint: Missing function or method docstring (missing-function-docstring)
tcf_core/context_processors.py
[error] 34-34: Pylint: Missing function or method docstring (missing-function-docstring)
tcf_website/views/calendar.py
[error] 34-34: Pylint: trailing whitespace (trailing-whitespace)
[error] 68-68: Pylint: trailing whitespace (trailing-whitespace)
[error] 90-90: Pylint: Line too long (114/100) (line-too-long)
[error] 91-91: Pylint: Line too long (106/100) (line-too-long)
[error] 146-146: Pylint: Trailing newlines (trailing-newlines)
[error] 1-1: Pylint: Missing module docstring (missing-module-docstring)
[error] 23-23: Pylint: Redefining name 'datetime' from outer scope (line 5) (redefined-outer-name)
[error] 23-23: Pylint: Reimport 'datetime' (reimported)
[error] 23-23: Pylint: Import outside toplevel (datetime.datetime) (import-outside-toplevel)
[error] 43-43: Pylint: Missing function or method docstring (missing-function-docstring)
[warning] 112-112: Pylint: Consider explicitly re-raising using 'except Exception as exc' and 'raise Http404('Event not found') from exc' (raise-missing-from)
[error] 5-5: Pylint: Standard import order: 'from django.conf import settings' should come after standard imports (wrong-import-order)
[error] 2-2: Pylint: Unused import 'get_object_or_404' from django.shortcuts (unused-import)
tcf_website/templatetags/custom_tags.py
[error] 26-26: Pylint: trailing whitespace (trailing-whitespace)
[error] 29-29: Pylint: trailing whitespace (trailing-whitespace)
[error] 53-53: Pylint: trailing whitespace (trailing-whitespace)
[error] 57-57: Pylint: trailing whitespace (trailing-whitespace)
[error] 68-68: Pylint: trailing whitespace (trailing-whitespace)
[error] 72-72: Pylint: trailing whitespace (trailing-whitespace)
[error] 4-4: Pylint: standard import 'import hashlib' should be placed before 'from django import template' (wrong-import-order)
🪛 Ruff (0.14.1)
tcf_website/views/calendar.py
111-111: Do not catch blind exception: Exception
(BLE001)
112-112: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
112-112: Avoid specifying long messages outside the exception class
(TRY003)
115-115: Avoid specifying long messages outside the exception class
(TRY003)
tcf_website/templatetags/custom_tags.py
70-70: Probable use of insecure hash functions in hashlib: md5
(S324)
🔇 Additional comments (10)
tcf_website/views/__init__.py (1)
44-44: LGTM!The calendar view exports follow the existing pattern and are properly integrated with the URL routing.
tcf_website/templatetags/custom_tags.py (1)
70-70: MD5 usage is acceptable here.The static analysis tool flagged MD5 as insecure, but in this context, it's being used for deterministic color generation (not cryptography), so this is perfectly safe.
tcf_core/settings/base.py (2)
137-137: LGTM!The context processor integration follows Django conventions and properly exposes the calendar feature flag to templates.
258-263: LGTM!The Presence/Calendar configuration settings are well-structured with sensible defaults. The timeout and cache durations are appropriate for external API calls.
tcf_website/services/presence.py (1)
1-20: LGTM!The API client implementation is well-structured with appropriate retry logic, timeout handling, and error propagation. The use of
backofffor exponential retry on network errors is a best practice.tcf_website/templates/calendar/disabled.html (1)
1-11: LGTM!The disabled calendar template is clean and straightforward. Minor note: there are extra blank lines at the end (lines 10-11), but this is a very minor style issue.
tcf_website/templates/base/sidebar.html (1)
63-75: LGTM!The calendar navigation item is properly gated with the feature flag and follows the existing sidebar pattern. The active state logic and icon usage are consistent with other sidebar items.
tcf_core/context_processors.py (1)
36-36: Verify default value consistency.The context processor defaults
ENABLE_CLUB_CALENDARtoFalse, but intcf_core/settings/base.py(line 260), the setting defaults toTrue. This mismatch might be intentional (fail-safe), but please verify this is the desired behavior to avoid confusion.tcf_website/urls.py (1)
130-131: LGTM!The new URL patterns for calendar functionality are well-structured and follow Django conventions. The route names are descriptive and the event_uri parameter type is appropriate.
tcf_website/templates/calendar/calendar_overview.html (1)
16-22: Good UX approach for future functionality.Including the disabled search input as a placeholder for upcoming functionality is a nice touch. It sets user expectations and shows the direction of the feature.
…ize styling, fix syntax errors, and update tests Amp-Thread-ID: https://ampcode.com/threads/T-0af986d5-19c4-4fe1-8b44-2038da7cb4ac Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tcf_website/views/calendar.py (1)
5-8: Prune unused imports.
settingsandget_object_or_404are unused and keep tripping lint; drop them so CI passes.
🧹 Nitpick comments (1)
tcf_website/templatetags/custom_tags.py (1)
70-74: Swap to a non-legacy hash for deterministic colors.
hashlib.md5fires our security linters (Ruff S324) even in non-crypto use. Switch tohashlib.sha256(or similar modern hash) so we stay compliant without changing behavior; just adjust the modulus to reuse the existing color list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tcf_core/context_processors.py(1 hunks)tcf_core/settings/base.py(2 hunks)tcf_website/services/presence.py(1 hunks)tcf_website/templates/base/sidebar.html(1 hunks)tcf_website/templates/calendar/calendar_overview.html(1 hunks)tcf_website/templates/calendar/event_detail.html(1 hunks)tcf_website/templatetags/custom_tags.py(2 hunks)tcf_website/tests/test_calendar_overview.py(1 hunks)tcf_website/views/calendar.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tcf_website/templates/calendar/event_detail.html
- tcf_website/templates/base/sidebar.html
- tcf_core/settings/base.py
- tcf_website/tests/test_calendar_overview.py
- tcf_website/services/presence.py
- tcf_core/context_processors.py
🧰 Additional context used
🧬 Code graph analysis (1)
tcf_website/views/calendar.py (1)
tcf_website/services/presence.py (2)
get_events(25-34)get_event_details(37-43)
🪛 GitHub Actions: Continuous Integration
tcf_website/views/calendar.py
[warning] 91-92: C0301: Line too long (114/100) (line-too-long).
[warning] 142-142: C0305: Trailing newlines (trailing-newlines).
[warning] 26-26: W0621: Redefining name 'datetime' from outer scope (line 3) (redefined-outer-name).
[warning] 26-26: W0404: Reimport 'datetime' (imported line 3) (reimported).
[warning] 26-26: C0415: Import outside toplevel (datetime.datetime) (import-outside-toplevel).
[warning] 109-109: W0707: Consider explicitly re-raising using 'except Exception as exc' and 'raise Http404("Event not found") from exc' (raise-missing-from).
[warning] 5-5: W0611: Unused settings imported from django.conf (unused-import).
[warning] 7-7: W0611: Unused get_object_or_404 imported from django.shortcuts (unused-import).
🪛 Ruff (0.14.3)
tcf_website/views/calendar.py
108-108: Do not catch blind exception: Exception
(BLE001)
109-109: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
109-109: Avoid specifying long messages outside the exception class
(TRY003)
112-112: Avoid specifying long messages outside the exception class
(TRY003)
tcf_website/templatetags/custom_tags.py
71-71: Probable use of insecure hash functions in hashlib: md5
(S324)
…fix views, add tests Amp-Thread-ID: https://ampcode.com/threads/T-0af986d5-19c4-4fe1-8b44-2038da7cb4ac Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tcf_core/context_processors.py (1)
17-31: Handle None case when no semesters exist.The pipeline failure confirms that
Semester.latest()can returnNone, causing anAttributeErrorwhen accessing.numberon line 21. This occurs when the database has no semester records, which can happen in testing or fresh environments.Apply this diff to handle the None case gracefully:
def searchbar_context(request): """Provide context for the search bar.""" latest_semester = Semester.latest() + + if latest_semester is None: + return { + "disciplines": Discipline.objects.all().order_by("name"), + "subdepartments": Subdepartment.objects.all().order_by("mnemonic"), + "semesters": Semester.objects.none(), + } + recent_semesters = Semester.objects.filter( number__gte=latest_semester.number - 50 # 50 = 5 years * 10 semesters ).order_by("-number")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tcf_core/context_processors.py(1 hunks)tcf_core/settings/base.py(3 hunks)tcf_core/settings/prod.py(1 hunks)tcf_website/templates/base/sidebar.html(1 hunks)tcf_website/templates/calendar/event_detail.html(1 hunks)tcf_website/tests/test_calendar_overview.py(1 hunks)tcf_website/tests/test_event_detail.py(1 hunks)tcf_website/views/calendar.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tcf_website/templates/calendar/event_detail.html
🧰 Additional context used
🧬 Code graph analysis (2)
tcf_website/views/calendar.py (1)
tcf_website/services/presence.py (2)
get_events(25-34)get_event_details(37-43)
tcf_website/tests/test_event_detail.py (1)
tcf_website/tests/test_calendar_overview.py (1)
setUp(11-12)
🪛 GitHub Actions: Continuous Integration
tcf_website/tests/test_calendar_overview.py
[warning] 24-24: C0301: Line too long (122/100) (line-too-long)
[warning] 25-25: C0301: Line too long (122/100) (line-too-long)
[warning] 26-26: C0301: Line too long (122/100) (line-too-long)
[warning] 38-38: C0305: Trailing newlines (trailing-newlines)
tcf_website/views/calendar.py
[warning] 141-141: C0305: Trailing newlines (trailing-newlines)
[warning] 5-5: W0611: Unused settings imported from django.conf (unused-import)
tcf_website/tests/test_event_detail.py
[warning] 18-18: W0107: Unnecessary pass statement (unnecessary-pass)
[warning] 5-5: W0611: Unused Http404 imported from django.http (unused-import)
tcf_core/settings/base.py
[warning] 261-261: C0305: Trailing newlines (trailing-newlines)
tcf_core/context_processors.py
[error] 21-21: AttributeError: 'NoneType' object has no attribute 'number' in searchbar_context during calendar view.
🪛 Ruff (0.14.3)
tcf_core/settings/prod.py
31-31: env may be undefined, or defined from star imports
(F405)
tcf_website/views/calendar.py
108-108: Avoid specifying long messages outside the exception class
(TRY003)
111-111: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (9)
tcf_core/context_processors.py (1)
32-32: LGTM!The trailing blank line addition is a minor formatting improvement with no functional impact.
tcf_core/settings/prod.py (1)
27-34: LGTM! Production cache configuration addresses previous feedback.The Redis cache configuration is appropriate for production and directly addresses the past review comment about not using the in-memory LocMemCache in production. The setup uses environment-based configuration with a sensible default.
Note: The Ruff F405 warning about
envis a false positive—envis imported frombase.pyvia the star import on line 3.tcf_core/settings/base.py (3)
52-61: LGTM! Cleaner database configuration structure.Moving the default DATABASES configuration to base settings with sensible defaults (localhost:5432) and then updating it for non-dev environments is a cleaner approach than the previous inline conditional blocks.
99-105: LGTM! Better pattern for environment-specific database configuration.Using
update()to modify only the RDS-specific keys is cleaner than redefining the entireDATABASES["default"]dictionary. This preserves the base configuration while overriding only what's necessary for production.
256-260: LGTM! Presence configuration constants are well-chosen.The default values are appropriate:
- 8-second timeout prevents hanging on slow API responses
- 5-minute cache (300s) balances data freshness with API load
- "virginia" subdomain aligns with the PR objectives
tcf_website/views/calendar.py (4)
13-43: LGTM! Helper functions are well-structured.The helper functions are clean and properly handle edge cases:
_safe_textsafely extracts plain text from HTML_sort_keycorrectly usesstart_utc(addressing past review feedback)_format_datetimehandles missing/invalid dates gracefully_is_upcomingtreats events without dates as upcoming (reasonable default)
45-102: LGTM! Calendar overview logic is sound.The function properly:
- Fetches and normalizes event data
- Sorts chronologically using the corrected
_sort_key- Separates upcoming vs. past events
- Groups by date with "TBD" handling
- Passes organized data to the template
The line-length and whitespace issues from past reviews have been addressed.
103-139: LGTM! Event detail view handles errors appropriately.The function correctly:
- Fetches event details with proper exception chaining (
from exc)- Returns 404 for missing events
- Formats all event fields with sensible defaults
- Passes comprehensive data to the template
The exception handling addresses past review feedback.
5-5: Remove unused import.The
settingsimport is not used anywhere in the module. TheENABLE_CLUB_CALENDARcheck appears to have been removed in a previous iteration.Apply this diff:
from datetime import datetime, timezone -from django.conf import settings from django.http import Http404 from django.shortcuts import renderLikely an incorrect or invalid review comment.
…r in sidebar Amp-Thread-ID: https://ampcode.com/threads/T-0af986d5-19c4-4fe1-8b44-2038da7cb4ac Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tcf_website/urls.py(1 hunks)tcf_website/views/__init__.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tcf_website/views/__init__.py (1)
tcf_website/views/calendar.py (2)
calendar_overview(45-102)event_detail(103-139)
tcf_website/urls.py (1)
tcf_website/views/calendar.py (2)
calendar_overview(45-102)event_detail(103-139)
🔇 Additional comments (1)
tcf_website/views/__init__.py (1)
43-43: LGTM!The import follows the established pattern for exposing views from submodules and enables proper routing in
urls.py.
…s, and trailing newlines Amp-Thread-ID: https://ampcode.com/threads/T-0af986d5-19c4-4fe1-8b44-2038da7cb4ac Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tcf_core/settings/base.py (1)
136-136: Critical: The flags context processor is causing an AttributeError.This newly added context processor is triggering an
AttributeErrorwhen the calendar view attempts to render (see pipeline failure oncalendar.pyline 95). The processor implementation must be missing an attribute or have a coding error.This issue is already flagged in the review of
calendar.pylines 95-102 with a verification script to examine the processor implementation.
🧹 Nitpick comments (1)
tcf_website/tests/test_calendar_overview.py (1)
24-41: Fix inconsistent indentation in dictionary literals.The dictionary entries have inconsistent indentation. Lines 27, 33, and 39 (the
"uri"and"startDateTimeUtc"keys) are indented more than the preceding keys, which hurts readability.Apply this diff to align all keys consistently:
mock_events = [ - { - "eventName": "Event C", - "organizationName": "Org C", - "uri": "c", - "startDateTimeUtc": "2025-01-03T10:00:00Z", - }, - { - "eventName": "Event A", - "organizationName": "Org A", - "uri": "a", - "startDateTimeUtc": "2025-01-01T10:00:00Z", - }, - { - "eventName": "Event B", - "organizationName": "Org B", - "uri": "b", - "startDateTimeUtc": "2025-01-02T10:00:00Z", - }, + { + "eventName": "Event C", + "organizationName": "Org C", + "uri": "c", + "startDateTimeUtc": "2025-01-03T10:00:00Z", + }, + { + "eventName": "Event A", + "organizationName": "Org A", + "uri": "a", + "startDateTimeUtc": "2025-01-01T10:00:00Z", + }, + { + "eventName": "Event B", + "organizationName": "Org B", + "uri": "b", + "startDateTimeUtc": "2025-01-02T10:00:00Z", + }, ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tcf_core/settings/base.py(4 hunks)tcf_website/tests/test_calendar_overview.py(1 hunks)tcf_website/tests/test_event_detail.py(1 hunks)tcf_website/views/calendar.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tcf_website/views/calendar.py (1)
tcf_website/services/presence.py (2)
get_events(25-34)get_event_details(37-43)
tcf_website/tests/test_calendar_overview.py (1)
tcf_website/tests/test_event_detail.py (1)
setUp(12-13)
🪛 GitHub Actions: Continuous Integration
tcf_website/views/calendar.py
[error] 95-95: Internal server error on /clubs/calendar/: AttributeError triggered by context processing (see previous failing context_processors.py).
tcf_website/tests/test_event_detail.py
[error] 16-16: IndentationError: expected an indented block after function definition.
🪛 Ruff (0.14.4)
tcf_website/views/calendar.py
108-108: Avoid specifying long messages outside the exception class
(TRY003)
111-111: Avoid specifying long messages outside the exception class
(TRY003)
tcf_website/tests/test_event_detail.py
16-16: Expected an indented block after function definition
(invalid-syntax)
🔇 Additional comments (5)
tcf_website/views/calendar.py (2)
107-111: LGTM: Exception handling is correct.The exception handling properly catches errors from the presence service, preserves the traceback with
from exc, and returns a 404 as expected. The static analysis warning (TRY003) about the exception message length is a false positive—"Event not found" is concise and appropriate.
95-102: The review comment is incorrect and should be dismissed.The
flagscontext processor is properly implemented. It usesgetattr(settings, "ENABLE_CLUB_CALENDAR", False)with a defensive fallback toFalse, which is the correct Django pattern for optional settings. This approach will not raise anAttributeError—getattr()with a default value returns the default when the attribute is missing. No evidence of an actual error exists in the codebase, and the render call incalendar.pyshould function normally with the current context processor setup.Likely an incorrect or invalid review comment.
tcf_core/settings/base.py (3)
52-61: LGTM: Database configuration is properly structured.The PostgreSQL database configuration correctly uses environment variables with sensible defaults for host and port. This follows Django best practices.
99-105: LGTM: AWS RDS configuration update is correct.Using
DATABASES["default"].update()properly overrides the base configuration for non-dev environments without redefining the entire dictionary. This is cleaner than the previous approach.
257-260: LGTM: Presence configuration constants are well-defined.The presence feature flags use appropriate defaults (subdomain "virginia", 8-second timeout, 5-minute cache) and follow the project's naming conventions. These integrate cleanly with the presence service implementation.
Amp-Thread-ID: https://ampcode.com/threads/T-f27f5a04-14ba-4b2a-9cae-d01d74db0f30 Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tcf_website/tests/test_event_detail.py (2)
34-34: Same URL path concern applies here.This test has the same potential URL mismatch as
test_event_detail_not_found_on_exception. If the route is at/clubs/calendar/event/, update this test URL as well.Apply the same fix if verification confirms the mismatch:
- resp = self.client.get("/clubs/events/test-uri/") + resp = self.client.get("/clubs/calendar/event/test-uri/")
15-19: Remove or implement the placeholder test.This empty test provides no validation and causes a pipeline failure. Past reviews have already flagged this issue twice. Either implement the test with proper mocking and assertions for the happy path, or remove it entirely to avoid false confidence in test coverage.
Consider implementing it:
+ @patch('tcf_website.views.calendar.presence.get_event_details') - def test_event_detail_route_ok(self): + def test_event_detail_route_ok(self, mock_get_details): """Test that event detail route handles valid event.""" - # Assuming a valid event URI, but since mocked, we'll add mocks - # Will be covered by mock tests - pass + mock_get_details.return_value = { + "eventName": "Test Event", + "organizationName": "Test Org", + "uri": "test-uri" + } + + resp = self.client.get("/clubs/events/test-uri/") + self.assertEqual(resp.status_code, 200) + self.assertContains(resp, "Test Event")Or simply remove it:
- def test_event_detail_route_ok(self): - """Test that event detail route handles valid event.""" - # Assuming a valid event URI, but since mocked, we'll add mocks - # Will be covered by mock tests - pass -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tcf_website/tests/test_event_detail.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Continuous Integration
tcf_website/tests/test_event_detail.py
[error] 19-19: W0107: Unnecessary pass statement (unnecessary-pass). Pylint: Unnecessary pass.
…context processor, and correct test URLs Amp-Thread-ID: https://ampcode.com/threads/T-f27f5a04-14ba-4b2a-9cae-d01d74db0f30 Co-authored-by: Amp <amp@ampcode.com>
|
@coderabbitai generate docstrings to improve docstring coverage. |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @R0hit-0. * #1178 (comment) The following files were modified: * `tcf_core/context_processors.py` * `tcf_website/services/presence.py` * `tcf_website/templatetags/custom_tags.py` * `tcf_website/tests/test_calendar_overview.py` * `tcf_website/tests/test_event_detail.py` * `tcf_website/views/calendar.py`
Amp-Thread-ID: https://ampcode.com/threads/T-f27f5a04-14ba-4b2a-9cae-d01d74db0f30 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-f27f5a04-14ba-4b2a-9cae-d01d74db0f30 Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-f27f5a04-14ba-4b2a-9cae-d01d74db0f30 Co-authored-by: Amp <amp@ampcode.com>
|
@CodeRabbit generate docstrings to improve docstring coverage to over 80% |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
Docstrings generation was requested by @R0hit-0. * #1178 (comment) The following files were modified: * `tcf_core/context_processors.py` * `tcf_website/services/presence.py` * `tcf_website/templatetags/custom_tags.py` * `tcf_website/tests/test_calendar_overview.py` * `tcf_website/tests/test_event_detail.py` * `tcf_website/views/calendar.py`
…id layout Amp-Thread-ID: https://ampcode.com/threads/T-2d54287f-c8f3-4902-b231-29ea1a9bff3c Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tcf_website/tests/test_event_detail.py (1)
1-30: Consider adding a happy-path test for complete coverage.The current tests thoroughly validate error handling (exception and None return), but there's no test verifying the successful case where
get_event_detailsreturns valid event data and the view responds with HTTP 200.Consider adding a test like this:
@patch('tcf_website.views.calendar.presence.get_event_details') def test_event_detail_success(self, mock_get_details): """Test that event detail renders successfully with valid data.""" mock_get_details.return_value = { "eventName": "Test Event", "organizationName": "Test Org", "startDateTimeUtc": "2025-11-20T18:00:00Z", "uri": "test-uri" } resp = self.client.get("/clubs/calendar/event/test-uri/") self.assertEqual(resp.status_code, 200) self.assertContains(resp, "Test Event")tcf_website/views/calendar.py (1)
111-112: Add blank line between function definitions.PEP 8 requires two blank lines between module-level function definitions. There should be a blank line between the end of
calendar_overview(line 111) and the start ofevent_detail(line 112).Apply this diff:
}, ) + def event_detail(request, event_uri):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tcf_core/context_processors.py(2 hunks)tcf_website/templates/calendar/calendar_overview.html(1 hunks)tcf_website/tests/test_calendar_overview.py(1 hunks)tcf_website/tests/test_event_detail.py(1 hunks)tcf_website/views/calendar.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tcf_core/context_processors.py
- tcf_website/templates/calendar/calendar_overview.html
- tcf_website/tests/test_calendar_overview.py
🧰 Additional context used
🧬 Code graph analysis (2)
tcf_website/views/calendar.py (1)
tcf_website/services/presence.py (2)
get_events(25-34)get_event_details(37-43)
tcf_website/tests/test_event_detail.py (1)
tcf_website/tests/test_calendar_overview.py (1)
setUp(11-12)
🪛 Ruff (0.14.4)
tcf_website/views/calendar.py
117-117: Avoid specifying long messages outside the exception class
(TRY003)
120-120: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
tcf_website/views/calendar.py (2)
112-148: LGTM!The
event_detailview is well-implemented with proper exception handling, defensive data extraction with defaults, and comprehensive event formatting. The exception chain is preserved usingfrom exc, and the None check provides clear 404 behavior.Note: Ruff flags TRY003 on lines 117 and 120, suggesting exception messages should be defined in exception classes rather than inline. However, for simple 404 responses, the current approach is perfectly acceptable and readable.
47-111: LGTM!The
calendar_overviewview is well-structured with clear logic flow: fetch events, normalize data, sort, separate upcoming/past, group by date, and render. The unique tags collection (lines 67-71) provides filtering capability for the UI. Defensive programming withor []andor ""handles missing data gracefully.
Amp-Thread-ID: https://ampcode.com/threads/T-2d54287f-c8f3-4902-b231-29ea1a9bff3c Co-authored-by: Amp <amp@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-2d54287f-c8f3-4902-b231-29ea1a9bff3c Co-authored-by: Amp <amp@ampcode.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tcf_website/services/presence.py`:
- Around line 46-52: Add a concise docstring to the function get_event_details
describing its purpose (fetches event details, using cache), parameters
(event_uri: str), return value (event data dict or JSON), and side-effects
(reads/writes cache via _cache_key/cache with CACHE_TTL and calls _get on BASE).
Place the docstring immediately below the def get_event_details(...) signature
and mirror style/format used by get_events() for consistency in coverage and
documentation.
🧹 Nitpick comments (1)
tcf_website/services/presence.py (1)
9-11: Consider consistent defensive access forPRESENCE_SUBDOMAIN.
CACHE_TTLandTIMEOUTusegetattrwith defaults, butBASEaccessessettings.PRESENCE_SUBDOMAINdirectly. If the setting is missing, this raisesAttributeErrorat module import time. If this is intentional (fail-fast for required config), that's fine—otherwise considergetattrwith an explicit error or default.
| def get_event_details(event_uri: str): | ||
| key = _cache_key(f"event::{event_uri}") | ||
| data = cache.get(key) | ||
| if data is None: | ||
| data = _get(f"{BASE}/events/{event_uri}") | ||
| cache.set(key, data, CACHE_TTL) | ||
| return data |
There was a problem hiding this comment.
Add a docstring for consistency and coverage goals.
get_events() has a docstring but get_event_details() does not. Given the PR comments mention targeting >80% docstring coverage, adding one here would help.
📝 Proposed docstring
def get_event_details(event_uri: str):
+ """
+ Returns details for a specific event by URI. Cached briefly to avoid rate limits.
+
+ Args:
+ event_uri: The unique identifier for the event from Presence.
+
+ Returns:
+ Event details dictionary from the Presence API.
+ """
key = _cache_key(f"event::{event_uri}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_event_details(event_uri: str): | |
| key = _cache_key(f"event::{event_uri}") | |
| data = cache.get(key) | |
| if data is None: | |
| data = _get(f"{BASE}/events/{event_uri}") | |
| cache.set(key, data, CACHE_TTL) | |
| return data | |
| def get_event_details(event_uri: str): | |
| """ | |
| Returns details for a specific event by URI. Cached briefly to avoid rate limits. | |
| Args: | |
| event_uri: The unique identifier for the event from Presence. | |
| Returns: | |
| Event details dictionary from the Presence API. | |
| """ | |
| key = _cache_key(f"event::{event_uri}") | |
| data = cache.get(key) | |
| if data is None: | |
| data = _get(f"{BASE}/events/{event_uri}") | |
| cache.set(key, data, CACHE_TTL) | |
| return data |
🤖 Prompt for AI Agents
In `@tcf_website/services/presence.py` around lines 46 - 52, Add a concise
docstring to the function get_event_details describing its purpose (fetches
event details, using cache), parameters (event_uri: str), return value (event
data dict or JSON), and side-effects (reads/writes cache via _cache_key/cache
with CACHE_TTL and calls _get on BASE). Place the docstring immediately below
the def get_event_details(...) signature and mirror style/format used by
get_events() for consistency in coverage and documentation.
#1223) * fix(api): critical remove public facing api; keep internal server-side rendering * fix(tests): remove API tests since API module was deleted * fix(reviews): avoid empty reviews Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…alendar samples, seed schools Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@tcf_website/services/presence.py`:
- Around line 31-35: The backoff decorator on _get currently retries all
RequestException subclasses (including HTTPError from resp.raise_for_status()),
causing 4xx client errors to be retried; update the retry policy so client
errors are not retried by adding a giveup predicate or changing the exception
handling: in the _get function/decorator (symbols: _get, backoff.on_exception,
requests.RequestException, resp.raise_for_status()), detect HTTPError and its
response status (e.response.status_code) and call giveup for 4xx responses (or
check resp.status_code before calling raise_for_status and only raise/allow
retries for 5xx/connection/timeouts) so only network/5xx errors are retried with
backoff.
In `@tcf_website/templates/base/index.html`:
- Around line 88-91: The current inline onerror handler on the AdSense script
(onerror="$('#adblockModal').modal({backdrop:'static',keyboard:false}).modal('show');")
will show an undismissable modal (id: adblockModal) on any load failure; update
the modal UI instead of making it permanent: modify the adblock modal
(adblockModal) to include a visible dismiss/continue control (e.g., a "Continue
anyway" link/button) and/or a standard close button, and relax the modal options
(remove backdrop:'static' and/or set keyboard:true) so users can proceed on
transient network errors; optionally, move the onerror handler to call a small
JS function (e.g., showAdblockModal()) that can implement a timeout +
bait-element check before showing the modal for more reliable adblock detection.
In `@tcf_website/templates/calendar/calendar_overview.html`:
- Line 51: Remove the hardcoded inline style "min-width: 500px" from the
dropdown element (the div with class "dropdown-menu p-3" and
aria-labelledby="tagFilterDropdown") and instead assign a specific CSS class
(e.g., .tag-filter-dropdown) in the template; then add CSS rules for that class
that preserve the intended desktop sizing but provide a responsive override for
small viewports (for example using a media query to reset min-width and/or set
width: 100% / max-width: 100% or min-width: auto below a breakpoint) so the
dropdown does not cause horizontal overflow on mobile.
- Around line 202-208: The filterEvents function currently hides cards with no
tags because cardTags becomes [] and selectedTags.some(...) returns false;
update the logic in filterEvents (referencing function filterEvents, variable
eventCards, card.dataset.tags and getSelectedTags) so that cards with no tags
are shown by default—i.e., treat an empty cardTags array as a match (e.g.,
consider cardTags.length === 0 as true) or otherwise include a condition that
displays tagless events instead of relying solely on selectedTags.some(...).
In `@tcf_website/templates/common/adblock_modal.html`:
- Around line 3-9: The modal element with id "adblockModal" sets
aria-labelledby="adblockModalLabel" but there is no element with that id; update
the modal title element (the <h5> that reads "Ad Blocker Detected") to include
id="adblockModalLabel" so screen readers correctly associate the modal with its
label (ensure the id matches aria-labelledby exactly).
In `@tcf_website/templates/course/course_professor.html`:
- Around line 234-249: The widthratio usage can produce an empty string when a
limit is zero, creating invalid CSS like width: %; — update the progress-bar
style attributes for both enrollment and waitlist to guard against zero limits
by checking section.enrollment_limit and section.waitlist_limit (or using a
default 0) before calling the {% widthratio %} so the style always renders a
valid numeric percentage; locate the progress-bar divs that use widthratio in
the template (the enrollment progress-bar and the waitlist progress-bar) and
wrap or replace the width expression with a conditional that yields 0% when the
corresponding limit is zero.
- Around line 286-294: The Q&A tab link (anchor with id="qaTab" and
href="#qaTabContent") has no matching content pane inside the surrounding <div
class="tab-content"> (which currently only contains `#reviewsTabContent`), causing
a blank panel; fix by either adding a corresponding tab pane element with
id="qaTabContent" and class="tab-pane" (matching the structure and classes used
by `#reviewsTabContent` and tying into the existing tabs behavior), or remove the
qaTab anchor from the <ul class="nav nav-tabs"> so there is no orphaned link.
In `@tcf_website/templates/reviews/new_review.html`:
- Around line 94-96: The placeholder promises "100+ words" but the HTML
minlength="100" enforces characters, not words; fix by adding client-side
word-count validation for the textarea with id="reviewtext": on input and on
submit (select the form via
document.getElementById('reviewtext').closest('form')), count words by splitting
the textarea.value on whitespace, and if words < 100 call
textarea.setCustomValidity("Please enter at least 100 words.") and
reportValidity()/preventDefault() on submit; otherwise clear the custom validity
so the form can submit. Alternatively, if the intent was characters, change the
placeholder text to "aim for 100+ characters" and keep minlength="100".
- Around line 206-237: The submit handler attached to
document.getElementById('reviewForm') prevents default but has no error handling
for the two fetch flows (the /reviews/check_duplicate/ and
/reviews/check_zero_hours_per_week/ requests), so network/server errors leave
the form unusable; update the fetch chains in that handler to (1) check
response.ok and throw on non-2xx before r.json(), (2) add .catch handlers for
both fetch calls that restore form usability by enabling
document.getElementById('submitBtn') and either submitting the form or showing a
user-facing error, and (3) ensure zeroHoursModalShown logic is preserved so
repeated submits don’t re-open the modal unexpectedly.
In `@tcf_website/views/browse.py`:
- Around line 115-116: The Http404 message raised when latest_semester is None
in the browse view is longer than 100 chars; shorten or extract it to a constant
to satisfy pylint. Update the raise in the function containing latest_semester
to use either a shorter string (e.g., "No semester data loaded. Run
migrations.") or a named constant (e.g., NO_SEMESTER_ERROR = "No semester data
loaded. Run migrations and ensure the database has semester data.") and
reference that constant in the raise Http404(...) call so the line length is
under 100 characters.
In `@tcf_website/views/calendar.py`:
- Around line 95-97: The inline comment above the dev-only sample data fallback
is over the 100-char limit; shorten or wrap it so the line length is <=100. Edit
the comment that precedes the if block using settings.DEBUG and events (the one
explaining _sample_events_for_dev()) to a shorter sentence or split into two
shorter comment lines immediately above the if statement so the if block and
calls to settings.DEBUG and _sample_events_for_dev() remain unchanged.
- Around line 145-147: Insert the required blank lines between the top-level
function definitions: add two blank lines between the end of calendar_overview
and the start of event_detail so event_detail is separated by a blank line pair
per PEP8 (E302); locate the definitions by the function names calendar_overview
and event_detail and update the spacing accordingly.
In `@tcf_website/views/review.py`:
- Around line 142-199: In _handle_course_review_get, guard against
Semester.latest() returning None before accessing latest.year by checking if
latest is None and handling it (e.g. messages.info(request, "No semesters
available; please try later.") and return redirect("/browse?mode=courses") or
another suitable early return), so replace direct uses of latest.year in the
semesters queries with a branch that only runs those queries when latest is not
None and otherwise performs the early redirect/response.
🧹 Nitpick comments (9)
docker-compose.yml (1)
9-14: Hardcodeddb:5432in command doesn't use the declared env vars.Lines 11–12 declare
DB_HOSTandDB_PORTas environment variables, but line 14 hardcodesdb:5432in thewait-for-itcommand. If someone updates the env vars, the wait-for-it target won't follow.Suggested fix
command: - bash -c "/wait-for-it.sh db:5432 -- \ + bash -c "/wait-for-it.sh $${DB_HOST}:$${DB_PORT} -- \(
$$escapes to a literal$in compose, so the shell inside the container resolves the env vars at runtime.)doc/dev.md (1)
41-50: Good documentation addition.Minor note: the database dump instructions (steps 1–4 under "Course browse") duplicate steps 5–6 in the setup section above. Consider referencing the existing steps instead to avoid divergence if the process changes.
tcf_website/views/review.py (1)
114-122: Duplicatehours_per_weekcomputation.
ReviewForm.save(commit=False)(line 65-73) already computes and setshours_per_weekon the instance. Lines 116-121 repeat the exact same calculation. Remove the redundant assignment here or in the form'ssave().Proposed fix
if form.is_valid(): instance = form.save(commit=False) instance.user = request.user - instance.hours_per_week = ( - instance.amount_reading - + instance.amount_writing - + instance.amount_group - + instance.amount_homework - ) instance.save()tcf_website/services/presence.py (1)
32-32: Return type hint is inaccurate —get_eventsexpects a list, not a dict.
_getis typed as returningdict, but the/eventsendpoint returns a JSON array (list). Considerdict | listor using a more generic type.♻️ Suggested fix
-def _get(url: str, params: dict | None = None) -> dict: +def _get(url: str, params: dict | None = None) -> dict | list:tcf_website/templates/calendar/calendar_overview.html (1)
86-117: Near-identical event card markup is duplicated between past and upcoming sections.The card template for past events (Lines 87-116) and upcoming events (Lines 135-164) are virtually identical. Extract into a shared partial (e.g.,
calendar/_event_card.html) and{% include %}it in both places to reduce maintenance burden.Also applies to: 134-165
tcf_website/views/calendar.py (2)
117-135: Consider extracting duplicate date-grouping logic into a helper.The grouping logic at lines 117–122 and 124–129 is identical. A small helper would reduce duplication:
def _group_by_date(events): groups = {} for ev in events: dt = ev["start_utc"] or "" date_key = dt[:10] if len(dt) >= 10 else "TBD" groups.setdefault(date_key, []).append(ev) return dict(sorted( groups.items(), key=lambda x: x[0] if x[0] != "TBD" else "9999-12-31" ))This would also absorb the sorting step from lines 131–135.
77-105:calendar_overviewnormalization and dev fallback look solid.One tiny nit:
sorted(list(all_tags))on line 105 —sorted()already returns a list, so wrapping inlist()is unnecessary.- unique_tags = sorted(list(all_tags)) + unique_tags = sorted(all_tags)tcf_website/templates/course/course_professor.html (2)
268-284: Inline script for section toggle is clean.The
DOMContentLoadedlistener with null-checks (if (showMoreBtn && sectionsList)) is defensive and correct. The Django template tags within the JS string are evaluated server-side, so this works as expected.One minor note: this script block sits inside
{% block content %}rather than{% block js %}. While it works (DOMContentLoaded guarantees DOM readiness), moving it to{% block js %}would be more consistent with the rest of the page's script organization.
222-257: Extract hardcoded SIS URL to a settings constant for maintainability.The SIS URL appears in multiple places across the codebase (template and management commands). Extracting it to a Django settings constant would simplify future updates if the URL structure changes.
Additionally, verify that
section.unitshandles decimal values correctly with thepluralizefilter. Sinceunitsis stored as a CharField (string) rather than a numeric field, values like "1.0" will incorrectly pluralize to "Units" instead of "Unit" because Django's filter checks string equality against "1", not numeric equivalence. If SIS data includes decimal units (e.g., "1.5"), consider converting to numeric type or adjusting the template logic.
| @backoff.on_exception(backoff.expo, (requests.RequestException,), max_tries=3) | ||
| def _get(url: str, params: dict | None = None) -> dict: | ||
| resp = _SESSION.get(url, params=params or {}, timeout=TIMEOUT) | ||
| resp.raise_for_status() | ||
| return resp.json() |
There was a problem hiding this comment.
Backoff retries non-retriable HTTP errors (4xx) unnecessarily.
raise_for_status() throws HTTPError (a subclass of RequestException), so client errors like 403/404 will be retried 3 times with exponential backoff. This wastes time and could aggravate rate limits. Consider limiting retries to connection/timeout errors, or adding a giveup predicate for 4xx responses.
🔧 Proposed fix — skip retries on client errors
+def _is_client_error(exc):
+ """Don't retry 4xx responses — they won't succeed on retry."""
+ if isinstance(exc, requests.HTTPError) and exc.response is not None:
+ return 400 <= exc.response.status_code < 500
+ return False
+
-@backoff.on_exception(backoff.expo, (requests.RequestException,), max_tries=3)
+@backoff.on_exception(
+ backoff.expo,
+ (requests.RequestException,),
+ max_tries=3,
+ giveup=_is_client_error,
+)
def _get(url: str, params: dict | None = None) -> dict:🤖 Prompt for AI Agents
In `@tcf_website/services/presence.py` around lines 31 - 35, The backoff decorator
on _get currently retries all RequestException subclasses (including HTTPError
from resp.raise_for_status()), causing 4xx client errors to be retried; update
the retry policy so client errors are not retried by adding a giveup predicate
or changing the exception handling: in the _get function/decorator (symbols:
_get, backoff.on_exception, requests.RequestException, resp.raise_for_status()),
detect HTTPError and its response status (e.response.status_code) and call
giveup for 4xx responses (or check resp.status_code before calling
raise_for_status and only raise/allow retries for 5xx/connection/timeouts) so
only network/5xx errors are retried with backoff.
|
|
||
| <!--Google AdSense (after jQuery so onerror can show modal)--> | ||
| <script async src="https://pagead2.googlesyndication.com/pagead/js/adsbygoogle.js?client=ca-pub-1816162577232603" | ||
| crossorigin="anonymous" onerror="$('#adblockModal').modal({backdrop:'static',keyboard:false}).modal('show');"></script> |
There was a problem hiding this comment.
onerror will fire on any network failure, not just ad blockers — potentially locking out legitimate users.
The onerror handler on the AdSense script triggers for any load failure (e.g., intermittent network issues, corporate firewalls, DNS failures, regions where Google domains are blocked). Combined with the undismissable modal (backdrop:'static', keyboard:false), a false positive completely locks the user out with no way to proceed — reloading will just fail again.
Consider either:
- Adding a dismiss/close option (e.g., "Continue anyway") so users aren't permanently stuck.
- Using a more reliable detection method (e.g., checking a known bait element after a timeout rather than relying solely on script
onerror).
Proposed fix: add a secondary dismiss option
In tcf_website/templates/common/adblock_modal.html, add a small dismiss link below the button:
<button onclick="location.reload();" class="btn btn-primary bg-tcf-indigo pl-4 pr-4">I've Disabled It</button>
+ <div class="mt-2">
+ <a href="#" data-dismiss="modal" class="text-muted small">Continue anyway</a>
+ </div>🤖 Prompt for AI Agents
In `@tcf_website/templates/base/index.html` around lines 88 - 91, The current
inline onerror handler on the AdSense script
(onerror="$('#adblockModal').modal({backdrop:'static',keyboard:false}).modal('show');")
will show an undismissable modal (id: adblockModal) on any load failure; update
the modal UI instead of making it permanent: modify the adblock modal
(adblockModal) to include a visible dismiss/continue control (e.g., a "Continue
anyway" link/button) and/or a standard close button, and relax the modal options
(remove backdrop:'static' and/or set keyboard:true) so users can proceed on
transient network errors; optionally, move the onerror handler to call a small
JS function (e.g., showAdblockModal()) that can implement a timeout +
bait-element check before showing the modal for more reliable adblock detection.
| <button class="btn btn-outline-primary dropdown-toggle" type="button" id="tagFilterDropdown" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false"> | ||
| Filters | ||
| </button> | ||
| <div class="dropdown-menu p-3" aria-labelledby="tagFilterDropdown" style="min-width: 500px;"> |
There was a problem hiding this comment.
Hardcoded min-width: 500px will overflow on mobile viewports.
The inline style on the dropdown menu forces a 500px minimum width. On screens narrower than ~500px, this causes horizontal overflow. Move this to a CSS class and add a responsive override.
🔧 Proposed fix
- <div class="dropdown-menu p-3" aria-labelledby="tagFilterDropdown" style="min-width: 500px;">
+ <div class="dropdown-menu p-3" aria-labelledby="tagFilterDropdown" style="min-width: min(500px, 90vw);">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="dropdown-menu p-3" aria-labelledby="tagFilterDropdown" style="min-width: 500px;"> | |
| <div class="dropdown-menu p-3" aria-labelledby="tagFilterDropdown" style="min-width: min(500px, 90vw);"> |
🤖 Prompt for AI Agents
In `@tcf_website/templates/calendar/calendar_overview.html` at line 51, Remove the
hardcoded inline style "min-width: 500px" from the dropdown element (the div
with class "dropdown-menu p-3" and aria-labelledby="tagFilterDropdown") and
instead assign a specific CSS class (e.g., .tag-filter-dropdown) in the
template; then add CSS rules for that class that preserve the intended desktop
sizing but provide a responsive override for small viewports (for example using
a media query to reset min-width and/or set width: 100% / max-width: 100% or
min-width: auto below a breakpoint) so the dropdown does not cause horizontal
overflow on mobile.
| function filterEvents() { | ||
| const selectedTags = getSelectedTags(); | ||
| eventCards.forEach(card => { | ||
| const cardTags = card.dataset.tags ? card.dataset.tags.split(',').map(t => t.trim()) : []; | ||
| const hasSelectedTag = selectedTags.some(tag => cardTags.includes(tag)); | ||
| card.style.display = hasSelectedTag ? '' : 'none'; | ||
| }); |
There was a problem hiding this comment.
Events with no tags are always hidden when any filter is selected.
When card.dataset.tags is empty, the ternary on Line 205 yields []. Since no selected tag is found in an empty array, hasSelectedTag is always false, so tagless events are permanently hidden whenever any checkbox is checked (which is the initial state). Consider showing tagless events when they have no tags, or always showing them:
🔧 Proposed fix
eventCards.forEach(card => {
const cardTags = card.dataset.tags ? card.dataset.tags.split(',').map(t => t.trim()) : [];
- const hasSelectedTag = selectedTags.some(tag => cardTags.includes(tag));
- card.style.display = hasSelectedTag ? '' : 'none';
+ // Show tagless events always; for tagged events, require at least one selected tag match
+ const show = cardTags.length === 0 || selectedTags.some(tag => cardTags.includes(tag));
+ card.style.display = show ? '' : 'none';
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function filterEvents() { | |
| const selectedTags = getSelectedTags(); | |
| eventCards.forEach(card => { | |
| const cardTags = card.dataset.tags ? card.dataset.tags.split(',').map(t => t.trim()) : []; | |
| const hasSelectedTag = selectedTags.some(tag => cardTags.includes(tag)); | |
| card.style.display = hasSelectedTag ? '' : 'none'; | |
| }); | |
| function filterEvents() { | |
| const selectedTags = getSelectedTags(); | |
| eventCards.forEach(card => { | |
| const cardTags = card.dataset.tags ? card.dataset.tags.split(',').map(t => t.trim()) : []; | |
| // Show tagless events always; for tagged events, require at least one selected tag match | |
| const show = cardTags.length === 0 || selectedTags.some(tag => cardTags.includes(tag)); | |
| card.style.display = show ? '' : 'none'; | |
| }); |
🤖 Prompt for AI Agents
In `@tcf_website/templates/calendar/calendar_overview.html` around lines 202 -
208, The filterEvents function currently hides cards with no tags because
cardTags becomes [] and selectedTags.some(...) returns false; update the logic
in filterEvents (referencing function filterEvents, variable eventCards,
card.dataset.tags and getSelectedTags) so that cards with no tags are shown by
default—i.e., treat an empty cardTags array as a match (e.g., consider
cardTags.length === 0 as true) or otherwise include a condition that displays
tagless events instead of relying solely on selectedTags.some(...).
| <div class="modal fade" id="adblockModal" tabindex="-1" role="dialog" aria-labelledby="adblockModalLabel" | ||
| aria-hidden="true" data-backdrop="static" data-keyboard="false"> | ||
| <div class="modal-dialog modal-dialog-centered" role="document"> | ||
| <div class="modal-content"> | ||
| <div class="modal-body text-center pt-4 pb-4"> | ||
| <img src="{% static 'base/img/new_logo.svg' %}" alt="theCourseForum" class="img-fluid mb-3" style="max-height: 60px;"> | ||
| <h5 class="mb-3">Ad Blocker Detected</h5> |
There was a problem hiding this comment.
aria-labelledby references a non-existent element ID.
Line 3 sets aria-labelledby="adblockModalLabel", but no element in the modal has id="adblockModalLabel". The <h5> on Line 9 is the intended label target but is missing the corresponding id attribute. Screen readers won't associate the modal with its title.
♿ Proposed fix
- <h5 class="mb-3">Ad Blocker Detected</h5>
+ <h5 id="adblockModalLabel" class="mb-3">Ad Blocker Detected</h5>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div class="modal fade" id="adblockModal" tabindex="-1" role="dialog" aria-labelledby="adblockModalLabel" | |
| aria-hidden="true" data-backdrop="static" data-keyboard="false"> | |
| <div class="modal-dialog modal-dialog-centered" role="document"> | |
| <div class="modal-content"> | |
| <div class="modal-body text-center pt-4 pb-4"> | |
| <img src="{% static 'base/img/new_logo.svg' %}" alt="theCourseForum" class="img-fluid mb-3" style="max-height: 60px;"> | |
| <h5 class="mb-3">Ad Blocker Detected</h5> | |
| <div class="modal fade" id="adblockModal" tabindex="-1" role="dialog" aria-labelledby="adblockModalLabel" | |
| aria-hidden="true" data-backdrop="static" data-keyboard="false"> | |
| <div class="modal-dialog modal-dialog-centered" role="document"> | |
| <div class="modal-content"> | |
| <div class="modal-body text-center pt-4 pb-4"> | |
| <img src="{% static 'base/img/new_logo.svg' %}" alt="theCourseForum" class="img-fluid mb-3" style="max-height: 60px;"> | |
| <h5 id="adblockModalLabel" class="mb-3">Ad Blocker Detected</h5> |
🤖 Prompt for AI Agents
In `@tcf_website/templates/common/adblock_modal.html` around lines 3 - 9, The
modal element with id "adblockModal" sets aria-labelledby="adblockModalLabel"
but there is no element with that id; update the modal title element (the <h5>
that reads "Ad Blocker Detected") to include id="adblockModalLabel" so screen
readers correctly associate the modal with its label (ensure the id matches
aria-labelledby exactly).
| document.getElementById('reviewForm').addEventListener('submit', function(e) { | ||
| e.preventDefault(); | ||
| var form = this; | ||
| var formData = new FormData(form); | ||
|
|
||
| $.ajax({ | ||
| type: "POST", | ||
| url: "/reviews/check_duplicate/", | ||
| data: $("#reviewForm").serialize(), | ||
| async: false, | ||
| success: function (check) { | ||
| if(check.duplicate) { | ||
| $("#duplicate-review").modal(); | ||
| } else { | ||
| // Check if review has 0 total hours/week and warn user if it does | ||
| $.ajax({ | ||
| type: "POST", | ||
| url: "/reviews/check_zero_hours_per_week/", | ||
| data: $("#reviewForm").serialize(), | ||
| async: false, | ||
| success: function (check) { | ||
| // Only show "zero-hours-review" modal once | ||
| if(check.zero && !zeroHoursModalShown) { | ||
| $("#zero-hours-review").modal(); | ||
| zeroHoursModalShown = true; | ||
| } else { | ||
| // Timeout button for 3 seconds so user can't spam button while form is submitting | ||
| $("#submitBtn").prop("disabled", true); | ||
| setTimeout(enableButton, 3000); | ||
| // $("#reviewForm") didn't work for some reason | ||
| // Not a duplicate review and the zero hours modal has been shown or wasn't | ||
| // necessary to show, so proceed with normal form submission for new review | ||
| document.getElementById("reviewForm").action = "{% url 'new_review' %}"; | ||
| document.getElementById("reviewForm").submit(); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| fetch('/reviews/check_duplicate/', { | ||
| method: 'POST', | ||
| body: formData, | ||
| headers: { 'X-CSRFToken': formData.get('csrfmiddlewaretoken') } | ||
| }) | ||
| .then(r => r.json()) | ||
| .then(check => { | ||
| if (check.duplicate) { | ||
| $('#duplicate-review').modal(); | ||
| } else { | ||
| fetch('/reviews/check_zero_hours_per_week/', { | ||
| method: 'POST', | ||
| body: formData, | ||
| headers: { 'X-CSRFToken': formData.get('csrfmiddlewaretoken') } | ||
| }) | ||
| .then(r => r.json()) | ||
| .then(check => { | ||
| if (check.zero && !zeroHoursModalShown) { | ||
| $('#zero-hours-review').modal(); | ||
| zeroHoursModalShown = true; | ||
| } else { | ||
| document.getElementById('submitBtn').disabled = true; | ||
| form.submit(); | ||
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Missing error handling on fetch calls — form becomes unsubmittable on network/server errors.
If /reviews/check_duplicate/ or /reviews/check_zero_hours_per_week/ returns a non-200 response or the network fails, the rejected promise is unhandled. The user's submit is preventDefault()-ed on Line 207 but never recovered, leaving the form stuck.
🔧 Proposed fix — add .catch handlers
fetch('/reviews/check_duplicate/', {
method: 'POST',
body: formData,
headers: { 'X-CSRFToken': formData.get('csrfmiddlewaretoken') }
})
.then(r => r.json())
.then(check => {
if (check.duplicate) {
$('#duplicate-review').modal();
} else {
fetch('/reviews/check_zero_hours_per_week/', {
method: 'POST',
body: formData,
headers: { 'X-CSRFToken': formData.get('csrfmiddlewaretoken') }
})
.then(r => r.json())
.then(check => {
if (check.zero && !zeroHoursModalShown) {
$('#zero-hours-review').modal();
zeroHoursModalShown = true;
} else {
document.getElementById('submitBtn').disabled = true;
form.submit();
}
- });
+ })
+ .catch(() => { form.submit(); });
}
- });
+ })
+ .catch(() => { form.submit(); });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| document.getElementById('reviewForm').addEventListener('submit', function(e) { | |
| e.preventDefault(); | |
| var form = this; | |
| var formData = new FormData(form); | |
| $.ajax({ | |
| type: "POST", | |
| url: "/reviews/check_duplicate/", | |
| data: $("#reviewForm").serialize(), | |
| async: false, | |
| success: function (check) { | |
| if(check.duplicate) { | |
| $("#duplicate-review").modal(); | |
| } else { | |
| // Check if review has 0 total hours/week and warn user if it does | |
| $.ajax({ | |
| type: "POST", | |
| url: "/reviews/check_zero_hours_per_week/", | |
| data: $("#reviewForm").serialize(), | |
| async: false, | |
| success: function (check) { | |
| // Only show "zero-hours-review" modal once | |
| if(check.zero && !zeroHoursModalShown) { | |
| $("#zero-hours-review").modal(); | |
| zeroHoursModalShown = true; | |
| } else { | |
| // Timeout button for 3 seconds so user can't spam button while form is submitting | |
| $("#submitBtn").prop("disabled", true); | |
| setTimeout(enableButton, 3000); | |
| // $("#reviewForm") didn't work for some reason | |
| // Not a duplicate review and the zero hours modal has been shown or wasn't | |
| // necessary to show, so proceed with normal form submission for new review | |
| document.getElementById("reviewForm").action = "{% url 'new_review' %}"; | |
| document.getElementById("reviewForm").submit(); | |
| } | |
| } | |
| }); | |
| } | |
| } | |
| fetch('/reviews/check_duplicate/', { | |
| method: 'POST', | |
| body: formData, | |
| headers: { 'X-CSRFToken': formData.get('csrfmiddlewaretoken') } | |
| }) | |
| .then(r => r.json()) | |
| .then(check => { | |
| if (check.duplicate) { | |
| $('#duplicate-review').modal(); | |
| } else { | |
| fetch('/reviews/check_zero_hours_per_week/', { | |
| method: 'POST', | |
| body: formData, | |
| headers: { 'X-CSRFToken': formData.get('csrfmiddlewaretoken') } | |
| }) | |
| .then(r => r.json()) | |
| .then(check => { | |
| if (check.zero && !zeroHoursModalShown) { | |
| $('#zero-hours-review').modal(); | |
| zeroHoursModalShown = true; | |
| } else { | |
| document.getElementById('submitBtn').disabled = true; | |
| form.submit(); | |
| } | |
| }); | |
| } | |
| }); | |
| document.getElementById('reviewForm').addEventListener('submit', function(e) { | |
| e.preventDefault(); | |
| var form = this; | |
| var formData = new FormData(form); | |
| fetch('/reviews/check_duplicate/', { | |
| method: 'POST', | |
| body: formData, | |
| headers: { 'X-CSRFToken': formData.get('csrfmiddlewaretoken') } | |
| }) | |
| .then(r => r.json()) | |
| .then(check => { | |
| if (check.duplicate) { | |
| $('#duplicate-review').modal(); | |
| } else { | |
| fetch('/reviews/check_zero_hours_per_week/', { | |
| method: 'POST', | |
| body: formData, | |
| headers: { 'X-CSRFToken': formData.get('csrfmiddlewaretoken') } | |
| }) | |
| .then(r => r.json()) | |
| .then(check => { | |
| if (check.zero && !zeroHoursModalShown) { | |
| $('#zero-hours-review').modal(); | |
| zeroHoursModalShown = true; | |
| } else { | |
| document.getElementById('submitBtn').disabled = true; | |
| form.submit(); | |
| } | |
| }) | |
| .catch(() => { form.submit(); }); | |
| } | |
| }) | |
| .catch(() => { form.submit(); }); |
🤖 Prompt for AI Agents
In `@tcf_website/templates/reviews/new_review.html` around lines 206 - 237, The
submit handler attached to document.getElementById('reviewForm') prevents
default but has no error handling for the two fetch flows (the
/reviews/check_duplicate/ and /reviews/check_zero_hours_per_week/ requests), so
network/server errors leave the form unusable; update the fetch chains in that
handler to (1) check response.ok and throw on non-2xx before r.json(), (2) add
.catch handlers for both fetch calls that restore form usability by enabling
document.getElementById('submitBtn') and either submitting the form or showing a
user-facing error, and (3) ensure zeroHoursModalShown logic is preserved so
repeated submits don’t re-open the modal unexpectedly.
| if latest_semester is None: | ||
| raise Http404("No semester data loaded. Run migrations and ensure the database has semester data.") |
There was a problem hiding this comment.
Fix: line too long — CI is failing.
Line 116 is 107 characters, exceeding the 100-character pylint limit. Split the message or move it to a constant.
Proposed fix
if latest_semester is None:
- raise Http404("No semester data loaded. Run migrations and ensure the database has semester data.")
+ raise Http404(
+ "No semester data loaded. "
+ "Run migrations and ensure the database has semester data."
+ )🧰 Tools
🪛 GitHub Actions: Continuous Integration
[error] 116-116: pylint: Line too long (107/100) (line-too-long)
🪛 Ruff (0.14.14)
[warning] 116-116: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In `@tcf_website/views/browse.py` around lines 115 - 116, The Http404 message
raised when latest_semester is None in the browse view is longer than 100 chars;
shorten or extract it to a constant to satisfy pylint. Update the raise in the
function containing latest_semester to use either a shorter string (e.g., "No
semester data loaded. Run migrations.") or a named constant (e.g.,
NO_SEMESTER_ERROR = "No semester data loaded. Run migrations and ensure the
database has semester data.") and reference that constant in the raise
Http404(...) call so the line length is under 100 characters.
| # Only in dev, when API returns no events (e.g. still 403), show sample data so the calendar UI is visible | ||
| if settings.DEBUG and not events: | ||
| events = _sample_events_for_dev() |
There was a problem hiding this comment.
Line too long (110/100) — pipeline failure on Line 95.
The inline comment exceeds the 100-character limit.
Proposed fix
- # Only in dev, when API returns no events (e.g. still 403), show sample data so the calendar UI is visible
+ # In dev, when API returns no events (e.g. 403),
+ # show sample data so the calendar UI is visible.
if settings.DEBUG and not events:
events = _sample_events_for_dev()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Only in dev, when API returns no events (e.g. still 403), show sample data so the calendar UI is visible | |
| if settings.DEBUG and not events: | |
| events = _sample_events_for_dev() | |
| # In dev, when API returns no events (e.g. 403), | |
| # show sample data so the calendar UI is visible. | |
| if settings.DEBUG and not events: | |
| events = _sample_events_for_dev() |
🧰 Tools
🪛 GitHub Actions: Continuous Integration
[error] 95-95: pylint: Line too long (110/100) (line-too-long)
🤖 Prompt for AI Agents
In `@tcf_website/views/calendar.py` around lines 95 - 97, The inline comment above
the dev-only sample data fallback is over the 100-char limit; shorten or wrap it
so the line length is <=100. Edit the comment that precedes the if block using
settings.DEBUG and events (the one explaining _sample_events_for_dev()) to a
shorter sentence or split into two shorter comment lines immediately above the
if statement so the if block and calls to settings.DEBUG and
_sample_events_for_dev() remain unchanged.
| ) | ||
| def event_detail(request, event_uri): | ||
| """Display detailed information for a specific event""" |
There was a problem hiding this comment.
Missing blank lines between top-level function definitions.
PEP 8 (E302) requires two blank lines before a top-level function definition. event_detail starts immediately after calendar_overview with no separating blank lines.
Proposed fix
)
+
+
def event_detail(request, event_uri):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ) | |
| def event_detail(request, event_uri): | |
| """Display detailed information for a specific event""" | |
| ) | |
| def event_detail(request, event_uri): | |
| """Display detailed information for a specific event""" |
🤖 Prompt for AI Agents
In `@tcf_website/views/calendar.py` around lines 145 - 147, Insert the required
blank lines between the top-level function definitions: add two blank lines
between the end of calendar_overview and the start of event_detail so
event_detail is separated by a blank line pair per PEP8 (E302); locate the
definitions by the function names calendar_overview and event_detail and update
the spacing accordingly.
| def _handle_course_review_get(request, mode): | ||
| """Handle GET for course reviews - require course context.""" | ||
| course_id = request.GET.get("course") | ||
| instructor_id = request.GET.get("instructor") | ||
|
|
||
| # No context - need course ID, redirect to home | ||
| if not course_id: | ||
| messages.info(request, "Please select a course to review from a course page.") | ||
| return redirect("/browse?mode=courses") | ||
|
|
||
| course = get_object_or_404(Course, id=course_id) | ||
| latest = Semester.latest() | ||
|
|
||
| if instructor_id: | ||
| # Full context: course + instructor | ||
| instructor = get_object_or_404(Instructor, id=instructor_id) | ||
| instructors = None | ||
| # Semesters when this instructor taught this course (last 5 years) | ||
| semesters = ( | ||
| Semester.objects.filter( | ||
| section__course=course, | ||
| section__instructors=instructor, | ||
| year__gte=latest.year - 5, | ||
| ) | ||
| .distinct() | ||
| .order_by("-number") | ||
| ) | ||
| else: | ||
| # Partial context: only course - let user pick instructor | ||
| instructor = None | ||
| instructors = ( | ||
| Instructor.objects.filter( | ||
| section__course=course, | ||
| section__semester__year__gte=latest.year - 5, | ||
| hidden=False, | ||
| ) | ||
| .distinct() | ||
| .order_by("last_name")[:50] | ||
| ) | ||
| # All recent semesters for this course | ||
| semesters = ( | ||
| Semester.objects.filter(section__course=course, year__gte=latest.year - 5) | ||
| .distinct() | ||
| .order_by("-number") | ||
| ) | ||
|
|
||
| return render( | ||
| request, | ||
| "reviews/new_review.html", | ||
| { | ||
| "is_club": False, | ||
| "mode": mode, | ||
| "course": course, | ||
| "instructor": instructor, | ||
| "instructors": instructors, | ||
| "semesters": semesters, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Semester.latest() may return None on an empty DB.
In _handle_course_review_get, latest = Semester.latest() is used without a None check (lines 153, 164, 175, 183). If no semesters exist, latest.year raises AttributeError. The department() view in browse.py (line 115) now guards against this — consider adding a similar guard here, or at minimum a clear early return/redirect.
🤖 Prompt for AI Agents
In `@tcf_website/views/review.py` around lines 142 - 199, In
_handle_course_review_get, guard against Semester.latest() returning None before
accessing latest.year by checking if latest is None and handling it (e.g.
messages.info(request, "No semesters available; please try later.") and return
redirect("/browse?mode=courses") or another suitable early return), so replace
direct uses of latest.year in the semesters queries with a branch that only runs
those queries when latest is not None and otherwise performs the early
redirect/response.
4 participants
What I did
Screenshots
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Documentation