Trending Courses/Professors Feature | UI & Analytics#1248
Open
ArislanT wants to merge 33 commits into
Open
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 24 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a DynamoDB-backed trending analytics feature: env config, an async recorder module, view-side recording on GET requests, retrieval/caching of top trending IDs, and UI/templates/styles to display trending courses and instructors. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant View as Django View
participant Analytics as analytics_utils
participant Executor as ThreadPoolExecutor
participant DynamoDB
User->>View: GET /course/{id}
View->>Analytics: record_course_view(course_id)
Analytics->>Executor: submit async update task
Executor-->>DynamoDB: update_item (ADD view_count, set expires_at, set entity_type)
View->>User: render course page
sequenceDiagram
actor User
participant HomeView as Home view
participant Cache as Django Cache
participant DynamoDB
participant ORM as Course/Instructor ORM
User->>HomeView: GET /
HomeView->>Cache: get(trending_key)
alt cache hit
Cache-->>HomeView: return cached list
else cache miss
HomeView->>DynamoDB: query EntityIndex (expires_at filter, paginate)
DynamoDB-->>HomeView: return items
HomeView->>HomeView: aggregate and sort IDs
HomeView->>ORM: fetch objects by id__in
ORM-->>HomeView: return ORM instances
HomeView->>Cache: set(trending_key)
end
HomeView->>User: render landing with trending lists
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ArislanT
added
Feature
![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: 6
🧹 Nitpick comments (1)
tcf_website/views/catalog/browse.py (1)
15-15: Move trending retrieval out of the home view module.
browse.pynow imports from..home.pages, coupling catalog views to home-page code. Consider movingget_trending_courses()/get_trending_instructors()into a dedicated analytics/trending service module and importing that from both views.Also applies to: 128-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/catalog/browse.py` at line 15, The catalog view is tightly coupled to home page code because browse.py imports get_trending_courses and get_trending_instructors from ..home.pages; extract those functions into a dedicated module (e.g., a new analytics/trending service) and update import sites to use that service: move the implementation of get_trending_courses and get_trending_instructors out of home.pages into the new module, export them from there, then change browse.py and home.pages to import the functions from the new trending service to remove the cross-module dependency.
🤖 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 .env.example trending analytics block is missing the
ANALYTICS_TTL_DAYS variable (used by analytics_utils.py) and an explicit opt-in
flag; update the example to include a commented ANALYTICS_TTL_DAYS= (number of
days) and add a commented enable flag (e.g., ENABLE_TRENDING_ANALYTICS=
true/false) so operators can discover the TTL knob and opt-in behavior;
reference analytics_utils.py and the ANALYTICS_TTL_DAYS environment variable
when adding these entries.
In `@tcf_website/analytics_utils.py`:
- Around line 17-55: Move all analytics configuration parsing into the safe init
path and make analytics opt-in: stop initializing _TTL_DAYS, AWS keys, and table
name at import time; introduce a feature flag (e.g., ANALYTICS_ENABLED default
False) and only set _ANALYTICS_ENABLED = True after validating access by
performing a cheap, synchronous check (e.g., a describe_table or a simple
DynamoDB call) using the created _SESSION; ensure _SESSION is created inside the
try block and on any failure set _ANALYTICS_ENABLED = False (don’t rely on
boto3.Session() alone), cache the resolved DYNAMODB_TABLE_NAME during init
instead of calling env() inside get_table(), and keep get_table() returning None
when _ANALYTICS_ENABLED is False; reference symbols to change: _TTL_DAYS,
_ANALYTICS_ENABLED, ANALYTICS_ENABLED (new flag), _SESSION, get_table, and
_BOTO_CONFIG.
In `@tcf_website/static/css/site/pages/landing.css`:
- Around line 310-319: In .trending-card__title remove the stray blank line
before the white-space declaration so there is no empty line between margin-top:
var(--space-1); and white-space: nowrap; (this fixes the
declaration-empty-line-before Stylelint rule); update the selector block in
landing.css to have contiguous declarations with no empty line.
In `@tcf_website/templates/site/home/landing.html`:
- Line 112: The decorative fire emoji inside the span with class
trending-card__badge is being announced to assistive tech; make it ignored by
screen readers by adding aria-hidden="true" (and optionally focusable="false"
for SVGs but here the span is sufficient) to the span element(s) where
trending-card__badge is used (both occurrences shown), ensuring the visual badge
remains but is not read aloud.
In `@tcf_website/views/home/pages.py`:
- Around line 101-103: The empty-result path in get_top_trending_ids handling
returns before populating the cache, causing repeated cold reads; change the
early-return branches (the one after course_ids = get_top_trending_ids("course")
and the similar block at the other occurrence) to first call cache.set(...) with
an empty list and a short TTL (e.g., a few minutes) using the same cache key
used for successful results, then return the empty list; ensure you reuse the
exact cache key construction and keep the short TTL so new trends aren’t hidden
for 24 hours.
- Around line 35-89: The loop prematurely stops when items is empty even though
response may include LastEvaluatedKey and it also only deduplicates by entity_id
without summing view_count across date-partitioned rows; change the aggregation
to track totals per entity (replace seen/list with a dict like entity_totals
mapping entity_id -> total_views), include "view_count" in ProjectionExpression
in query_kwargs, and when iterating items parse pk as before, read view_count =
int(item.get("view_count", 0)) and add it to entity_totals[entity_id]; only
append entity IDs to unique_ids (or build a sorted final list) once their totals
are accumulated and you have enough unique entities; also remove the early "if
not items: break" so pagination continues when Items is empty but
response.get("LastEvaluatedKey") exists (still break only when no
LastEvaluatedKey or pages >= max_pages).
---
Nitpick comments:
In `@tcf_website/views/catalog/browse.py`:
- Line 15: The catalog view is tightly coupled to home page code because
browse.py imports get_trending_courses and get_trending_instructors from
..home.pages; extract those functions into a dedicated module (e.g., a new
analytics/trending service) and update import sites to use that service: move
the implementation of get_trending_courses and get_trending_instructors out of
home.pages into the new module, export them from there, then change browse.py
and home.pages to import the functions from the new trending service to remove
the cross-module dependency.
🪄 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: c6edcadb-b446-4da3-9743-21e4a7432d50
📒 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
…rs, and resolve context overwrite bug
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tcf_website/views/home/pages.py (1)
1-256:⚠️ Potential issue | 🟠 MajorFix ruff formatting to unblock CI.
The CI pipeline reports
ruff format --check failed. Runuv run ruff format .locally and commit the result before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/home/pages.py` around lines 1 - 256, The PR fails ruff formatting; run the formatter and commit the fixes: run the exact command suggested (uv run ruff format .) in your repo root, review changes touching the functions/classes in this file such as get_top_trending_ids, get_trending_courses, get_trending_instructors, index and AboutView (including _normalize_member/_normalize_members), save the formatted file, run tests/ruff --check again, and commit the resulting changes so CI will pass.
🧹 Nitpick comments (2)
tcf_website/views/home/pages.py (2)
97-102: Redundantexclude(number__isnull=True).
Course.numberis a non-nullableIntegerField(seetcf_website/models/models.pylines 632-634), so this exclude is a no-op. Consider dropping it for clarity.♻️ Proposed simplification
courses = list( Course.objects.filter(id__in=course_ids) .select_related("subdepartment") .exclude(subdepartment__mnemonic="") - .exclude(number__isnull=True) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/home/pages.py` around lines 97 - 102, The query that builds courses uses Course.objects.filter(...).select_related("subdepartment").exclude(subdepartment__mnemonic="").exclude(number__isnull=True), but Course.number is a non-nullable IntegerField so exclude(number__isnull=True) is a no-op; remove that redundant .exclude(number__isnull=True) from the expression that assigns courses to simplify the query (look for the Course.objects.filter(...) call that produces the courses list in views/home/pages.py).
40-82: LGTM on the trending query.The fixes from previous reviews are correctly applied: per-entity
view_countaggregation viascores_by_id, pagination continues through empty result pages (driven solely byLastEvaluatedKey),view_countis in theProjectionExpression, and errors are caught at the outer boundary withexc_info=True.ScanIndexForward=False+Limit=20correctly assumes theEntityIndexGSI is sorted byview_countdescending — verify this assumption matches the actual GSI definition in AWS, as there is no schema or IaC definition for EntityIndex in the repository.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tcf_website/views/home/pages.py` around lines 40 - 82, The code assumes the EntityIndex GSI is sorted by view_count (using ScanIndexForward=False) which may not match the actual AWS GSI; verify the GSI definition and if its sort key is not view_count, update the query to use the correct sort key (or remove the descending assumption) in the query building logic (see query_kwargs/IndexName "EntityIndex" and ScanIndexForward), or change to a scan/secondary query that orders by the correct attribute; ensure the ProjectionExpression and KeyConditionExpression align with the GSI key schema and adjust Query parameters accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tcf_website/views/home/pages.py`:
- Around line 1-256: The PR fails ruff formatting; run the formatter and commit
the fixes: run the exact command suggested (uv run ruff format .) in your repo
root, review changes touching the functions/classes in this file such as
get_top_trending_ids, get_trending_courses, get_trending_instructors, index and
AboutView (including _normalize_member/_normalize_members), save the formatted
file, run tests/ruff --check again, and commit the resulting changes so CI will
pass.
---
Nitpick comments:
In `@tcf_website/views/home/pages.py`:
- Around line 97-102: The query that builds courses uses
Course.objects.filter(...).select_related("subdepartment").exclude(subdepartment__mnemonic="").exclude(number__isnull=True),
but Course.number is a non-nullable IntegerField so exclude(number__isnull=True)
is a no-op; remove that redundant .exclude(number__isnull=True) from the
expression that assigns courses to simplify the query (look for the
Course.objects.filter(...) call that produces the courses list in
views/home/pages.py).
- Around line 40-82: The code assumes the EntityIndex GSI is sorted by
view_count (using ScanIndexForward=False) which may not match the actual AWS
GSI; verify the GSI definition and if its sort key is not view_count, update the
query to use the correct sort key (or remove the descending assumption) in the
query building logic (see query_kwargs/IndexName "EntityIndex" and
ScanIndexForward), or change to a scan/secondary query that orders by the
correct attribute; ensure the ProjectionExpression and KeyConditionExpression
align with the GSI key schema and adjust Query parameters accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7b9f3e8-4718-43b1-986a-a76aef28c249
📒 Files selected for processing (6)
.env.exampledocker-compose.ymltcf_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 due to trivial changes (3)
- docker-compose.yml
- .env.example
- tcf_website/static/css/site/pages/landing.css
🚧 Files skipped from review as they are similar to previous changes (1)
- tcf_website/analytics_utils.py
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
1 participant
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.
Context
Created a feature that illustrates the popularity of courses and professor
This PR includes the Analytics, how the data is getting tracked, and the Frontend, the retrieval and display of the analytics.
Frontend simply changes landing.html & landing.css, the other changes are towards the analytics or general fixes
Key Aspects
Analytics Aspect:
Previously, no metric/method of tracking views/popularity. So I had to build the methodology from scratch: Views are collected passively at the routing layer. Not from client-side JavaScript, pixel tracking, or separate API call, as user accessibility and site stability is the priority. Rather, when a user hits a course or instructor page via a standard GET request, the view function fires record_course_view or record_instructor_view before returning the response. This increments a view_count counter in DynamoDB for that entity's daily row using an atomic ADD operation, which is safe under concurrent writes without any locking.
Page views are recorded asynchronously so analytics writes never block the request/response cycle
(On shutdown, atexit ensures all pending tasks cleanly flush before the process exits)
DynamoDB Schema && TTL:
Safe Initialization:
24-Hour Cache
Trending results are cached in Django's cache backend for 24 hours. DynamoDB is only hit on a cache miss, meaning the GSI query runs at most once per day per entity type regardless of traffic volume. This makes the feature effectively zero-cost at runtime for the vast majority of requests.
GSI (Entity Index) & Retrieval Logic: Replaces the original full table scan, making trending lookups efficient at scale
UI Aspect: (Location and design is just a mock-up. Created for quicker advancement in the development cycle)
Changes
Backend:
tcf_website/views/catalog/browse.py
Both course.py & course_instructor.py makes it so that the write is dispatched to a background thread immediately and the view continues rendering without waiting for DB to complete, meaning 0 latency impact on page load
tcf_website/views/courses/course.py
tcf_website/views/courses/course_instructor.py
tcf_website/analytics_utils.py
tcf_website/views/home/pages.py
.env.example
Design Decision
UI:
tcf_website/static/css/site/pages/landing.css
tcf_website/templates/site/home/landing.html
Screenshots
UI/Frontend
Testing
Backend:
(The seven views were successfully rendered for users but just not counted in analytics)
UI/Frontend
Summary by CodeRabbit