feat: add course bookmarks feature#972
Open
Premkumar-2004 wants to merge 3 commits intoalphaonelabs:mainfrom
Open
feat: add course bookmarks feature#972Premkumar-2004 wants to merge 3 commits intoalphaonelabs:mainfrom
Premkumar-2004 wants to merge 3 commits intoalphaonelabs:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
WalkthroughAdds a course bookmarking feature: new CourseBookmark model and migration, views and URLs to toggle/list bookmarks, template and JS updates for bookmark toggles and a bookmarks page, and tests covering bookmark behavior and minor tracker test changes. Changes
Sequence Diagram(s)mermaid User->>Browser: Click bookmark toggle (course slug) mermaid User->>Browser: Visit My Bookmarks page Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_trackers.py`:
- Line 60: The import of Course, CourseBookmark, Subject in
tests/test_trackers.py is placed late (after class/definitions) causing E402 and
import-order failures; move the line "from web.models import Course,
CourseBookmark, Subject" into the module-level import block at the top of the
file alongside the other imports so all imports are declared before any
code/class definitions and conform to isort/flake8/Black checks.
In `@web/templates/account/my_bookmarks.html`:
- Line 19: The bookmark card div with id "bookmark-card-{{ course.slug }}" is
missing the required background and shadow Tailwind classes; update its class
attribute (currently "rounded-lg p-4 border border-gray-200
dark:border-gray-700") to include "bg-white dark:bg-gray-800 shadow-lg" so it
becomes "bg-white dark:bg-gray-800 rounded-lg shadow-lg p-4 border
border-gray-200 dark:border-gray-700" to meet the card styling guideline.
- Around line 100-102: Update the "View Course" button classes to use the
project's success color and transitions: replace the current bg-green-500
hover:bg-green-600 with bg-green-600 and an appropriate darker hover (e.g.,
hover:bg-green-700) and add transition duration-200; locate the anchor element
that renders the "View Course" button in my_bookmarks.html (the <a> with text
"View Course") and update its class attribute accordingly.
- Around line 133-147: The fetch promise chain that starts with
.then(function(response) { return response.json(); }) lacks error handling and
doesn't check response.ok, so failures are silent; update the chain to first
verify response.ok (or throw with response.status/text) before parsing JSON,
then add a .catch handler to handle network or server errors, and in the error
path avoid removing the DOM node with id 'bookmark-card-' + slug or updating
'bookmark-count' incorrectly—instead show a user-visible error (e.g., alert or
inline message) and log the error; ensure the success branch (data.bookmarked
=== false) still performs the existing DOM removals and location.reload
behavior.
In `@web/templates/base.html`:
- Around line 508-511: Add the same "My Bookmarks" link into the mobile account
menu by copying the anchor using the {% url 'my_bookmarks' %} href and the <i
class="fas fa-heart"> icon (the anchor shown in the diff) into the mobile
account/menu block, and ensure it is visible only on small screens by adding the
Tailwind responsive visibility class (e.g., add sm:hidden to the anchor or its
wrapper so it shows on mobile and is hidden on sm+). Make sure the class list
matches the desktop styling (block px-4 py-2 text-sm text-gray-700
dark:text-gray-200 hover:bg-gray-100 dark:hover:bg-gray-700) and place it
alongside other mobile menu items.
In `@web/templates/courses/detail.html`:
- Around line 574-577: This toggle button needs an initial aria-pressed
attribute and runtime updates: set aria-pressed="{{
is_bookmarked|yesno:'true,false' }}" on the button with id="bookmark-btn" in the
template, and then update the toggleBookmark(slug) JavaScript function to flip
aria-pressed and update the aria-label/text after the server toggles state (use
document.getElementById('bookmark-btn') to set
button.setAttribute('aria-pressed', newState) and
button.setAttribute('aria-label', newLabel)). Apply the same change to the other
bookmark button instance referenced in the template so both initial markup and
the toggleBookmark handler reflect the pressed state for assistive tech.
- Line 575: Update the bookmarked-state class in the is_bookmarked conditional:
replace the use of text-red-500 with the project's danger tone text-red-600 and
add a dark-mode variant (dark:text-red-400) inside the same class string used on
the element (the conditional class that contains bg-red-100 dark:bg-red-900
text-red-500); apply the same change to the duplicate occurrence referenced at
the other location (the other is_bookmarked conditional).
- Around line 1712-1747: toggleBookmark can send overlapping POSTs causing race
conditions; add an in-flight guard and disable the button while the request is
pending. In toggleBookmark, use a per-button flag (e.g., element.dataset.busy or
a closure/Map keyed by slug) to early-return if busy, set the flag and disable
the button (set disabled attribute and/or aria-disabled and a visual class)
before calling fetch, then perform the UI toggle only after a successful
response and clear the flag and re-enable the button in both the then and catch
(or finally) paths; reference the toggleBookmark function and the 'bookmark-btn'
element (and ids 'bookmark-icon-outline', 'bookmark-icon-filled',
'bookmark-text') when making these changes.
In `@web/templates/courses/search.html`:
- Around line 35-39: The bookmark button lacks an ARIA state; add
aria-pressed="{{ course.id in bookmarked_course_ids }}" to the button markup and
update the toggleCardBookmark(element) JavaScript function to set
element.setAttribute('aria-pressed', String(isBookmarked)) whenever the bookmark
state changes (where isBookmarked reflects the new boolean state). Ensure the
initial template uses the same boolean expression (bookmarked_course_ids) and
apply the same aria-pressed addition/update for the other button instance
mentioned (lines ~340-347) so assistive tech can read both initial and dynamic
bookmark states.
In `@web/views.py`:
- Around line 8878-8881: The current code prefetches the entire
course__web_requests relation via bookmark_qs which can pull many rows into
memory; instead remove "course__web_requests" from the prefetch_related on
CourseBookmark (refer to bookmark_qs/CourseBookmark) and, if the template only
needs counts or simple aggregates, annotate the Course queryset with
Count("web_requests") (or use a Prefetch with a limited queryset) so you expose
web_request_count on each course rather than loading all rows; update the code
that builds courses = [b.course for b in bookmark_qs] (or the template) to use
the annotated count (e.g., course.web_request_count) accordingly.
- Line 8863: The new view functions toggle_bookmark and the other new view at
the nearby location should include explicit type hints for the request parameter
and their return type; update the signatures (e.g., def toggle_bookmark(request:
HttpRequest) -> HttpResponse / JsonResponse) and add the necessary imports (from
django.http import HttpRequest, HttpResponse, JsonResponse or typing.Union if
needed) so both functions are properly typed per the project's Python typing
guidelines.
- Around line 8882-8884: The current membership set uses course titles and can
misclassify when titles duplicate: change the set comprehension that builds
user_courses from {e.course.title for e in enrollments} to use course IDs
(e.course.id) so user_courses contains integers; update any template checks that
compare against course title to instead compare against course.id (ensure the
template compares the same integer/string type as provided). Locate the
Enrollment.objects.filter(...).select_related("course") block and the variable
user_courses to make this change and update the template logic accordingly.
- Line 23: Replace the private import from
allauth.account.internal.flows.email_verification and any usages of
send_email_confirmation with the public, model-centric API: import EmailAddress
from allauth.account.models and at each place you currently call
send_email_confirmation(user, ...) retrieve the EmailAddress via
EmailAddress.objects.get_for_user(user, user.email) and call
email_address.send_confirmation(request, signup=False); alternatively you may
import the public wrapper from allauth.account.utils as send_email_confirmation,
but prefer EmailAddress.send_confirmation to avoid relying on internal APIs and
to ensure compatibility with django-allauth 65.3.1+.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
tests/test_trackers.pyweb/migrations/0064_add_course_bookmark.pyweb/models.pyweb/templates/account/my_bookmarks.htmlweb/templates/base.htmlweb/templates/courses/detail.htmlweb/templates/courses/search.htmlweb/urls.pyweb/views.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_trackers.py`:
- Line 5: The test file imports Notification but never uses it; remove
Notification from the import list in tests/test_trackers.py by editing the
import line that currently reads "from web.models import Course, CourseBookmark,
Notification, ProgressTracker, Subject" to omit the Notification symbol so only
used models (Course, CourseBookmark, ProgressTracker, Subject) remain.
In `@web/templates/courses/search.html`:
- Around line 34-48: The bookmarked button’s active color class is inconsistent;
in the conditional class string for the button (and the SVG fill/stroke
behavior) used with toggleCardBookmark and the condition "course.id in
bookmarked_course_ids", replace the active state class "text-red-500" with the
project standard "text-red-600 dark:text-red-400" so the bookmarked state
matches detail.html; ensure you update the conditional branch that currently
renders "{% if course.id in bookmarked_course_ids %}bg-red-100 dark:bg-red-900
text-red-500{% else %}...{% endif %}" to use "text-red-600 dark:text-red-400"
instead of "text-red-500".
- Around line 326-358: The toggleCardBookmark function can suffer race
conditions from rapid clicks; add a busy guard by checking and setting a
per-button flag (e.g., btn.dataset.busy or btn.setAttribute('data-busy','true'))
at the start and returning early if already busy, set the flag to true
immediately before initiating fetch, and in the fetch completion path (both
.then and .catch or a finally-like branch) clear the flag (remove data-busy or
set it to false) and optionally update aria-busy; reference the
toggleCardBookmark function and the btn element when adding this guard so
duplicate requests are prevented and UI state updates stay consistent.
In `@web/views.py`:
- Around line 1341-1342: Replace the direct call to
EmailAddress.objects.get_for_user(user, user.email) with the safe get_or_create
pattern so missing EmailAddress rows won’t raise EmailAddress.DoesNotExist; i.e.
call EmailAddress.objects.get_or_create(user=user, email=user.email) and use the
returned instance (email_addr) before invoking
email_addr.send_confirmation(request). Apply the same change to the other
occurrence in web/forms.py to ensure both places handle legacy accounts safely.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
tests/test_trackers.pyweb/templates/base.htmlweb/templates/courses/detail.htmlweb/templates/courses/search.htmlweb/views.py
github-actions
bot
added
files-changed: 10
Author
|
@A1L13N could you please review it |
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.
Related issues
Fixes #967
Implemented a course
bookmarksystem that lets authenticated users save and manage their favorite courses. Users can togglebookmarksfrom the course detail page via a heart icon, and view all their saved courses on a dedicated "My Bookmarks" page with course details, stats, and inline removal.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests