Trending Courses/Professors Feature | UI & Analytics#1247
Closed
ArislanT wants to merge 27 commits into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughAdds DynamoDB-backed trending analytics: asynchronous recording of course and instructor page views, DynamoDB updates with TTL and view counts, retrieval of top trending IDs with caching, UI and CSS for a Trending section on the landing page, and environment variables to enable/configure analytics. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ViewHandler as Course View Handler
participant Analytics as analytics_utils
participant Executor as ThreadPoolExecutor
participant DynamoDB
User->>ViewHandler: GET /course/{id}
ViewHandler->>Analytics: record_course_view(course_id)
Analytics->>Analytics: Validate config/session
alt analytics enabled
Analytics->>Analytics: Increment pending counter
alt pending <= MAX_BACKLOG
Analytics->>Executor: Submit task
Executor->>Executor: Queue task
Executor->>DynamoDB: update_item(pk, sk, increment view_count, set TTL if_not_exists)
DynamoDB-->>Executor: Success / Error
else pending > MAX_BACKLOG
Analytics->>Analytics: Drop event (log)
end
Executor->>Analytics: Decrement pending counter (finally)
else analytics disabled
Analytics-->>ViewHandler: No-op
end
ViewHandler-->>User: Render course page
sequenceDiagram
participant User
participant IndexView as Landing View
participant Pages as home/pages.py
participant Cache as Django Cache
participant DynamoDB
participant ORM as Django ORM
participant Template
User->>IndexView: GET /
IndexView->>Pages: get_trending_courses()
Pages->>Cache: check 'trending_courses'
alt cache hit
Cache-->>Pages: return cached list
else cache miss
Pages->>DynamoDB: Query EntityIndex (expires_at filter, GSI)
DynamoDB-->>Pages: return items (pk, sk)
Pages->>Pages: parse IDs, dedupe, preserve order
Pages->>ORM: fetch Course objects by IDs (validity filters)
ORM-->>Pages: return queryset
Pages->>Cache: store result (24h)
end
IndexView->>Template: include trending_courses & trending_instructors
Template-->>User: Render landing with trending section
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 52-58: The file's analytics env block (ending with the commented
variable DYNAMODB_TABLE_NAME) is missing a trailing newline which fails
dotenv-linter; fix by adding a single blank line (newline) at the end of the
file so the file ends with a newline character after the DYNAMODB_TABLE_NAME
comment.
In `@tcf_website/analytics_utils.py`:
- Around line 31-40: The current logic disables analytics unless both
AWS_ANALYTICS_ACCESS_KEY_ID and AWS_ANALYTICS_SECRET_ACCESS_KEY are set; instead
instantiate boto3.Session with explicit aws_access_key_id/aws_secret_access_key
only when provided, otherwise call boto3.Session(region_name=env("AWS_REGION",
default="us-east-1")) to allow the default credential chain (EC2/ECS role,
shared config, etc.), then determine _ANALYTICS_ENABLED by checking the
resulting session's credentials (e.g., session.get_credentials() is not None) or
by a lightweight credentials check; update the code around _SESSION, access_key,
secret_key, boto3.Session and _ANALYTICS_ENABLED accordingly so analytics is
enabled whenever boto3 can resolve credentials, not only when static keys are
present.
In `@tcf_website/static/css/site/pages/landing.css`:
- Around line 208-223: Remove the stray blank line inside the .trending-card
rule: open the CSS rule for .trending-card and delete the empty line immediately
before background-color so the declarations start without an intervening blank
line (update the .trending-card block that contains height: 5rem; and
background-color: var(--bg-elevated);). Ensure there are no other unexpected
blank lines inside that rule to satisfy Stylelint.
In `@tcf_website/templates/site/home/landing.html`:
- Around line 103-108: Remove the inline styles from the div and span elements
in landing.html (the div containing the name/title and the two spans currently
using inline styles) and instead add the classes trending-card__text to the
wrapper div, trending-card__name--tight to the course name span, and
trending-card__subtitle to the subtitle span; then add the corresponding CSS
rules into tcf_website/static/css/site/pages/landing.css (rules should implement
display:flex; flex-direction:column; justify-content:center; min-width:0 for
.trending-card__text, line-height:1.2 for .trending-card__name--tight, and
font-size:0.85em; opacity:0.7; margin-top:2px; plus overflow/ellipsis rules for
.trending-card__subtitle).
- Around line 96-100: The section element with class "trending" has
aria-labelledby="trending-heading" but no element with that id exists; fix by
updating the aria-labelledby to point to the existing heading id
"trending-courses-heading" (or alternatively add an element with id
"trending-heading"); locate the <section class="trending"
aria-labelledby="trending-heading"> and change the attribute to
aria-labelledby="trending-courses-heading" so it references the <h2
id="trending-courses-heading" class="trending__title"> element.
In `@tcf_website/views/home/pages.py`:
- Line 115: Remove the whitespace-only blank line in pages.py that is triggering
Ruff W293: open tcf_website/views/home/pages.py and delete the blank line at the
location reported (the standalone line with only whitespace), ensuring there are
no trailing spaces so the file no longer contains a whitespace-only line.
- Around line 134-138: The current query that builds the instructors list uses
excludes on first_name/last_name and thus drops valid instructors and doesn't
respect visibility; update the Instructor query that populates the local
variable instructors (the Instructor.objects.filter(id__in=instructor_ids) call)
to instead filter out non-public records by adding a visibility condition (e.g.,
filter(hidden=False) or filter(is_public=True) depending on the model field) and
remove the .exclude(first_name="")/.exclude(last_name="") clauses so optional
names are preserved; keep filtering by id__in=instructor_ids and any existing
ordering/limit logic intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6aadcb0e-8f7b-4789-a723-ec3576cb51ab
📒 Files selected for processing (8)
.env.exampletcf_website/analytics_utils.pytcf_website/static/css/site/pages/landing.csstcf_website/templates/site/home/landing.htmltcf_website/views/catalog/browse.pytcf_website/views/courses/course.pytcf_website/views/courses/course_instructor.pytcf_website/views/home/pages.py
Comment on lines
+96
to
+100
| <section class="trending" aria-labelledby="trending-heading"> | ||
| <div class="trending__grid"> | ||
|
|
||
| <article class="trending__column"> | ||
| <h2 id="trending-courses-heading" class="trending__title">🔥 Trending Courses</h2> |
Contributor
There was a problem hiding this comment.
Fix the broken aria-labelledby target.
Line 96 points to trending-heading, but that id does not exist in this template. The section ends up with an invalid accessible label reference.
One simple fix
- <section class="trending" aria-labelledby="trending-heading">
+ <section class="trending" aria-label="Trending">📝 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.
Suggested change
| <section class="trending" aria-labelledby="trending-heading"> | |
| <div class="trending__grid"> | |
| <article class="trending__column"> | |
| <h2 id="trending-courses-heading" class="trending__title">🔥 Trending Courses</h2> | |
| <section class="trending" aria-label="Trending"> | |
| <div class="trending__grid"> | |
| <article class="trending__column"> | |
| <h2 id="trending-courses-heading" class="trending__title">🔥 Trending Courses</h2> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tcf_website/templates/site/home/landing.html` around lines 96 - 100, The
section element with class "trending" has aria-labelledby="trending-heading" but
no element with that id exists; fix by updating the aria-labelledby to point to
the existing heading id "trending-courses-heading" (or alternatively add an
element with id "trending-heading"); locate the <section class="trending"
aria-labelledby="trending-heading"> and change the attribute to
aria-labelledby="trending-courses-heading" so it references the <h2
id="trending-courses-heading" class="trending__title"> element.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tcf_website/static/css/site/pages/landing.css (1)
315-317:⚠️ Potential issue | 🟡 MinorRemove the empty line before
white-spaceto satisfy Stylelint.Line 316 has an empty line before a declaration, which triggers
declaration-empty-line-before.Lint-only cleanup
.trending-card__title { display: block; /* Ensures it sits on its own line */ font-size: var(--text-xs); opacity: 0.7; margin-top: var(--space-1); - white-space: nowrap; overflow: hidden; text-overflow: ellipsis; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/static/css/site/pages/landing.css` around lines 315 - 317, Remove the stray empty line immediately before the "white-space" declaration to satisfy Stylelint's declaration-empty-line-before rule; edit the CSS rule block containing the "white-space" and "overflow" properties so "white-space: nowrap;" directly follows the previous line (no blank line) and re-run linting to confirm the warning is gone.
🧹 Nitpick comments (1)
tcf_website/static/css/site/pages/landing.css (1)
274-280: Use the existing design token instead of undefined--color-error-600.Line 278 references
--color-error-600, but the shared tokens define--color-danger-600(intcf_website/static/css/site/tokens.css). The current fallback works, but it bypasses the design system contract.Suggested token-aligned fix
.trending-card__badge { font-size: var(--text-xs); font-weight: var(--font-bold); - /* Use design system variable with manual red as fallback */ - color: var(--color-error-600, `#b91c1c`); + color: var(--color-danger-600); background-color: rgba(239, 68, 68, 0.1); padding: var(--space-1) var(--space-3); border-radius: var(--radius-full); white-space: nowrap; flex-shrink: 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/static/css/site/pages/landing.css` around lines 274 - 280, The CSS rule .trending-card__badge is using a non-existent token --color-error-600; change it to the defined design token --color-danger-600 (i.e., color: var(--color-danger-600, `#b91c1c`)) so the component uses the shared design token while retaining the same fallback; update only the variable name in the color declaration to align with the tokens.css contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tcf_website/static/css/site/pages/landing.css`:
- Around line 315-317: Remove the stray empty line immediately before the
"white-space" declaration to satisfy Stylelint's declaration-empty-line-before
rule; edit the CSS rule block containing the "white-space" and "overflow"
properties so "white-space: nowrap;" directly follows the previous line (no
blank line) and re-run linting to confirm the warning is gone.
---
Nitpick comments:
In `@tcf_website/static/css/site/pages/landing.css`:
- Around line 274-280: The CSS rule .trending-card__badge is using a
non-existent token --color-error-600; change it to the defined design token
--color-danger-600 (i.e., color: var(--color-danger-600, `#b91c1c`)) so the
component uses the shared design token while retaining the same fallback; update
only the variable name in the color declaration to align with the tokens.css
contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88e947b7-4735-47b9-942f-56c1a68b6c56
📒 Files selected for processing (4)
tcf_website/analytics_utils.pytcf_website/static/css/site/pages/landing.csstcf_website/templates/site/home/landing.htmltcf_website/views/home/pages.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tcf_website/templates/site/home/landing.html
- tcf_website/views/home/pages.py
- tcf_website/analytics_utils.py
| # Configuration | ||
| _MAX_BACKLOG = 100 | ||
| _MAX_WORKERS = 5 | ||
| _TTL_DAYS = max(1, env.int("ANALYTICS_TTL_DAYS", default=7)) # Minimum 1 day TTL |
Contributor
There was a problem hiding this comment.
What is your reasoning for this duration for the TTL?
ArislanT
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub Issues addressed
Closed as frontend and backend PR just got consolidated into one PR (#1248)
(diff has backend as well as must be merged for true functionality)
Frontend changes are in landing.html and landing.css
Only difference between backend code here and other PR is
which was added to optimize the frontend rendering and prevent N+1 query issues.
What I did
Some notes:
Placed top 5 trending courses/professors on front page. Location is just mock up as the priority was to ensure functionality of the backend engine (advised this and that there was no need for extensive design).
Cards are clickable for all trending items and has a hovering effect
Course Symbol, Mnemonic, and Full Name displayed
Professor only has their name displayed as dealing with departments gets tricky for professors with relationships with several departments
CSS key ideas:
Performance:
Screenshots
Testing
Summary by CodeRabbit
New Features
Chores