Filter by User to Educational Memes #625
Filter by User to Educational Memes #625abdelrahman390 wants to merge 7 commits intoalphaonelabs:mainfrom
Conversation
|
Warning Rate limit exceeded@A1L13N has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 35 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds meme test data generation to the test-data command, adds a user filter dropdown and JS to the memes template, and extends the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Browser
participant DjangoView
participant Database
Note over Browser, DjangoView #f9f0d9: Filter UI interaction (new)
User->>Browser: Select user from dropdown and/or subject button
Browser->>DjangoView: GET /memes?user=<user_id>&subject=<subject_slug>
DjangoView->>Database: Query memes (apply subject filter if present)
DjangoView->>Database: Query memes (apply uploader filter if present)
Database-->>DjangoView: Return filtered memes
DjangoView->>Browser: Render memes list with `memes_creators` and `selected_user_id`
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @abdelrahman390, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini or gemini-code-assist here, providing a summary of this pull request to help everyone get up to speed quickly.
This pull request addresses issue #619 by implementing a user filter on the educational memes page. Previously, users could only filter memes by subject. This change adds a dropdown filter that allows users to view memes uploaded by a specific user or all users.
The changes involve modifying the Django view to handle the new filter parameter, updating the template to include the filter dropdown and associated JavaScript for client-side filtering logic, and enhancing the test data creation command to include meme data associated with various users, making testing the new filter easier.
Highlights
- User Filtering: Adds a new filter option to the educational memes page, allowing users to filter the list of memes by the user who uploaded them.
- Backend Logic: Modifies the
meme_listview inweb/views.pyto accept auserquery parameter and filter theMemequeryset accordingly. It also collects the list of unique meme creators to populate the filter dropdown. - Frontend Implementation: Updates the
memes.htmltemplate to include a new dropdown (<select>) element for selecting a user. A small JavaScript snippet is added to handle the selection change and redirect the page with the appropriate query parameter. - Test Data: Enhances the
create_test_datamanagement command to include the creation of educational memes associated with randomly selected users, facilitating testing of the new user filter feature.
Changelog
Click here to see the changelog
- web/management/commands/create_test_data.py
- Imported the
Mememodel (line 27). - Added
Memeto the list of models cleared byclear_data(line 76). - Created a combined list of
teachersandstudentsinto ausersvariable (line 120) to be used for assigning meme uploaders. - Added a call to a new method
create_meme_test_data(line 582) to generate meme test data. - Implemented the
create_meme_test_datamethod (starting line 586) which defines sample meme data across various subjects and createsMemeobjects, assigning a random user as theuploaderand a randomcreated_atdate.
- Imported the
- web/templates/memes.html
- Added a new
<span>and<select>element for the user filter dropdown (lines 18-27). - Populated the user filter dropdown using the
memes_creatorscontext variable, marking theselected_user_idas selected if present (lines 24-25). - Added a
<script>block (starting line 96) with JavaScript to listen for changes on the user filter dropdown and redirect the page with the selected user ID as a query parameter.
- Added a new
- web/views.py
- Retrieved the
userquery parameter from the request (line 4364). - Iterated through the initial
memesqueryset to build a dictionarymemes_creatorsmapping user IDs (as strings) to usernames for all users who have uploaded memes (lines 4366-4371). - Added a conditional filter to the
memesqueryset based on theuser_filterparameter (lines 4376-4377). - Updated the context dictionary passed to the template to include
selected_user_idandmemes_creators(lines 4387-4388).
- Retrieved the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Memes on the page,
Filter by user, a new stage,
See who made what.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by adding the ability to filter educational memes by user. The implementation in the backend (views and test data generation) is largely well done. However, there are a few issues in the frontend template, particularly with how filter parameters are handled in JavaScript and pagination links, which need to be addressed to ensure correct functionality. Additionally, there's a minor logic issue and a potential N+1 query in the view that could be improved.
Summary of Findings
- JavaScript URL construction for filters: The JavaScript handling user filter changes in
memes.htmldoes not preserve the existing subject filter, leading to a loss of the subject context when a user filter is applied. - Pagination links missing user filter: Pagination links in
memes.htmldo not include theuserquery parameter, causing the user filter to be lost when navigating pages. - N+1 Query in
meme_listview: The initial fetching of memes inweb/views.pylacksselect_related('uploader', 'subject'), which can lead to performance issues due to N+1 database queries. - Incorrect logic for populating
memes_creators: Inweb/views.py, the condition to check for existing creators in thememes_creatorsdictionary uses the username against keys that are user IDs, which is logically flawed. - Redundant/Potentially problematic JavaScript variable: In
web/templates/memes.html,const subjectSlug = "{{ subject.slug }}";is declared but not used by the current URL redirection logic, and might be problematic ifsubjectisn't always in the template context.
Merge Readiness
The pull request introduces a useful filtering capability for educational memes. However, there are several issues, primarily in the frontend template (memes.html) related to how filter parameters are handled in JavaScript and pagination links, which impact the user experience and correctness of combined filtering. Additionally, there are opportunities for optimization and a minor logic correction in the backend view (web/views.py).
I recommend addressing the high severity issues concerning URL parameter handling in JavaScript and pagination before merging. The medium severity issues related to query optimization and memes_creators population logic should also be considered for a more robust and efficient implementation.
As an AI, I am not authorized to approve pull requests. Please ensure these changes are reviewed and approved by a human maintainer before merging.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
web/management/commands/create_test_data.py (1)
706-727: 🧹 Nitpick (assertive)Consider using cryptographically secure random generation
For production code, consider using more secure random generation methods, especially when creating test user associations.
- # Select a random user as uploader - uploader = random.choice(users) - - # Generate a random date within the last month - random_date = timezone.now() - timedelta(days=random.randint(0, 30)) + # Select a random user as uploader using cryptographically secure random + import secrets + uploader = users[secrets.randbelow(len(users))] + + # Generate a random date within the last month using secure random + random_date = timezone.now() - timedelta(days=secrets.randbelow(31))Note: This is a low-priority suggestion since this is test data generation, not security-sensitive code.
🧰 Tools
🪛 Ruff (0.8.2)
709-709: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
712-712: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
web/templates/memes.html (1)
71-74: 🛠️ Refactor suggestionUpdate pagination to include user filter parameter
The pagination links don't preserve the user filter parameter, only the subject filter. This will cause the user filter to be lost when navigating between pages.
- <a href="?{% if selected_subject %}subject={{ selected_subject }}&{% endif %}page={{ memes.previous_page_number }}" + <a href="?{% if selected_user_id %}user={{ selected_user_id }}&{% endif %}{% if selected_subject %}subject={{ selected_subject }}&{% endif %}page={{ memes.previous_page_number }}"Apply similar changes to the page number links and next page link.
Also applies to: 80-84, 87-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
web/management/commands/create_test_data.py(4 hunks)web/templates/memes.html(2 hunks)web/views.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/management/commands/create_test_data.py (1)
web/models.py (4)
Meme(1784-1823)Subject(198-216)image(1282-1287)created_at(2337-2339)
🪛 GitHub Check: CodeQL
web/templates/memes.html
[warning] 102-102: DOM text reinterpreted as HTML
DOM text is reinterpreted as HTML without escaping meta-characters.
🪛 Ruff (0.8.2)
web/management/commands/create_test_data.py
586-586: Missing return type annotation for public function create_meme_test_data
Add return type annotation: None
(ANN201)
586-586: Missing type annotation for function argument subjects
(ANN001)
586-586: Missing type annotation for function argument users
(ANN001)
709-709: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
712-712: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Run Tests
🔇 Additional comments (9)
web/management/commands/create_test_data.py (6)
27-27: LGTM - Added Meme model importThe Meme model has been correctly imported to support the new test data generation functionality.
76-76: LGTM - Added Meme model to clear_dataGood practice to include the Meme model in the data clearing process to ensure a clean slate for test data generation.
120-120: LGTM - Created combined users listCreating a combined list of teachers and students for random assignment to memes is appropriate.
581-583: LGTM - Added call to create meme test dataThe new method call is properly placed at the end of the handle method to create test meme data.
590-680: LGTM - Well-structured test dataThe meme data is well-organized by subject, with appropriate titles, captions, and image paths for each meme.
682-705: LGTM - Proper subject handlingThe code correctly handles existing subjects and creates new ones when needed, with appropriate attributes.
web/templates/memes.html (1)
18-27: LGTM - Added user filter dropdownThe user filter dropdown is well-implemented with proper styling that matches the existing subject filter.
web/views.py (2)
4364-4377: Feature implementation for user filtering looks good.The code correctly:
- Retrieves the user filter parameter from the request
- Builds a mapping of meme creator IDs to usernames
- Applies filtering by uploader ID when a user filter is provided
This successfully implements the "Filter by User" feature for educational memes as required in the PR.
4383-4389: Context variables properly set for the template.The updated context dictionary correctly includes:
selected_user_id: For tracking the currently selected user filtermemes_creators: The mapping of creator IDs to usernames for populating the filter dropdownThis data structure will allow the template to display the user filter dropdown and maintain the selected state.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
web/templates/memes.html (1)
72-73: 🛠️ Refactor suggestionPagination controls need to include user filter.
The pagination links only include the subject filter but not the user filter, which will cause the user filter to be lost when navigating between pages.
Update the pagination links to include the user filter:
- <a href="?{% if selected_subject %}subject={{ selected_subject }}&{% endif %}page={{ memes.previous_page_number }}" + <a href="?{% if selected_user_id %}user={{ selected_user_id }}&{% endif %}{% if selected_subject %}subject={{ selected_subject }}&{% endif %}page={{ memes.previous_page_number }}" class="px-4 py-2 text-sm font-medium text-gray-700 dark:text-gray-200 bg-white dark:bg-gray-800 border border-gray-300 dark:border-gray-600 rounded-l-md hover:bg-gray-50 dark:hover:bg-gray-700">And similarly for other pagination links.
Also applies to: 81-82, 88-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
web/management/commands/create_test_data.py(4 hunks)web/templates/memes.html(2 hunks)web/views.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
web/management/commands/create_test_data.py (1)
web/models.py (4)
Meme(1784-1823)Subject(198-216)image(1282-1287)created_at(2337-2339)
🪛 Ruff (0.8.2)
web/management/commands/create_test_data.py
709-709: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
712-712: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-test
- GitHub Check: Run Tests
🔇 Additional comments (14)
web/management/commands/create_test_data.py (8)
27-27: Good addition of the Meme model import.The Meme model is properly imported to support the new test data generation.
76-76: Ensure data cleaning includes Meme model.The Meme model has been correctly added to the list of models cleared during data reset, which ensures a clean slate for test data generation.
120-120: Good approach to combine users.Combining teachers and students into a single list for random selection is an efficient approach for meme creation.
581-583: Well-placed call to create meme test data.The meme data creation function is appropriately called at the end of the handle method, with proper parameters passed.
586-589: Function has proper type annotations.The function includes clear type annotations for parameters and return value, which improves code readability and type safety.
591-680: Well-structured test data with diverse content.The meme data is well-organized by subject categories with detailed titles, captions, and image paths. The variety of subjects and memes will provide good test coverage for the filtering functionality.
684-705: Robust subject handling with fallback creation.The code intelligently reuses existing subjects or creates new ones when needed, which is a good approach for test data that might be run multiple times.
709-709: Using random is appropriate here.Static analysis flagged random.choice and random.randint as not suitable for cryptographic purposes, but for test data generation this is acceptable and appropriate.
Also applies to: 712-712
🧰 Tools
🪛 Ruff (0.8.2)
709-709: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
web/templates/memes.html (3)
18-27: Good implementation of user filter dropdown.The user filter dropdown is well-structured with appropriate options including an "All users" option and proper selection of the current user.
34-38: Button-based subject filtering improves UX.Converting subject links to buttons with data attributes allows for better integration with the JavaScript-based filtering system and maintains the visual design.
97-126: Well-implemented secure filtering with URL API.The JavaScript code correctly:
- Uses the URL API for proper parameter encoding, preventing XSS vulnerabilities
- Preserves both filters when either is changed
- Handles subject and user filtering consistently
This successfully addresses previous review comments about XSS vulnerabilities and filter preservation.
web/views.py (3)
4366-4375: Good implementation of memes creator dictionary for user filtering.The code properly builds a dictionary of meme creators, mapping user IDs to usernames for use in the filter dropdown. The implementation correctly converts user IDs to strings for dictionary keys and ensures each creator is added only once.
4379-4381: Properly implemented user filtering functionality.The user filtering implementation correctly applies the filter only when a valid user ID is provided and handles the "None" case appropriately.
4386-4392: Context variables properly updated for the template.The context dictionary has been properly updated to include both the selected user ID and the memes creators mapping, which will allow the template to correctly display the filter and maintain the selected state.
There was a problem hiding this comment.
Pull Request Overview
This PR adds user filtering functionality to the educational memes page, allowing users to filter memes by both subject and uploader. The enhancement improves the discoverability of content by providing granular filtering options.
- Enhanced meme filtering with dual dropdown interface for subjects and users
- Added backend support for user-based filtering alongside existing subject filtering
- Expanded test data generation to include educational memes with random uploaders
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| web/views.py | Added user filtering logic and context data for meme creators |
| web/templates/memes.html | Implemented user dropdown filter and JavaScript for combined filtering |
| web/management/commands/create_test_data.py | Added comprehensive meme test data generation with random uploaders |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # Get distinct uploaders from the database | ||
| uploaders = memes.values("uploader__id", "uploader__username").distinct() | ||
| memes_creators = {str(uploader["uploader__id"]): uploader["uploader__username"] for uploader in uploaders} | ||
|
|
||
| if subject_filter and subject_filter != "None": | ||
| memes = memes.filter(subject__slug=subject_filter) | ||
|
|
||
| if user_filter and user_filter != "None": | ||
| memes = memes.filter(uploader_id=user_filter) | ||
|
|
There was a problem hiding this comment.
The query for uploaders is executed before applying filters, which could be inefficient. Consider moving this query after the filtering logic to only fetch uploaders from the filtered meme set, or use a separate query on the User model to get all users who have uploaded memes.
| # Get distinct uploaders from the database | |
| uploaders = memes.values("uploader__id", "uploader__username").distinct() | |
| memes_creators = {str(uploader["uploader__id"]): uploader["uploader__username"] for uploader in uploaders} | |
| if subject_filter and subject_filter != "None": | |
| memes = memes.filter(subject__slug=subject_filter) | |
| if user_filter and user_filter != "None": | |
| memes = memes.filter(uploader_id=user_filter) | |
| if subject_filter and subject_filter != "None": | |
| memes = memes.filter(subject__slug=subject_filter) | |
| if user_filter and user_filter != "None": | |
| memes = memes.filter(uploader_id=user_filter) | |
| # Get distinct uploaders from the filtered memes queryset | |
| uploaders = memes.values("uploader__id", "uploader__username").distinct() | |
| memes_creators = {str(uploader["uploader__id"]): uploader["uploader__username"] for uploader in uploaders} |
| const selectedSubject = this.dataset.value; | ||
| // Create URL object for proper encoding of parameters | ||
| const url = new URL(baseUrl, window.location.origin); | ||
| url.searchParams.append('user', selectedUserId); |
There was a problem hiding this comment.
The user parameter is appended even when selectedUserId/selectedUser is empty or 'None'. This will add an empty user parameter to the URL. Consider only appending the parameter when the value is truthy and not 'None'.
|
|
||
| // Create URL object for proper encoding of parameters | ||
| const url = new URL(baseUrl, window.location.origin); | ||
| url.searchParams.append('user', selectedUser); |
There was a problem hiding this comment.
The user parameter is appended even when selectedUserId/selectedUser is empty or 'None'. This will add an empty user parameter to the URL. Consider only appending the parameter when the value is truthy and not 'None'.
| url.searchParams.append('user', selectedUser); | |
| if (selectedUser && selectedUser !== 'None') { | |
| url.searchParams.append('user', selectedUser); | |
| } |
A1L13N
left a comment
A1L13N
left a comment
There was a problem hiding this comment.
Please update this PR:
✅ Ensure all review comments are addressed
🔄 Resolve any merge conflicts
🧭 Verify that database migrations are correct and up to date
Once everything is done, please push the updated commits so we can proceed with the review.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
web/views.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
| memes = Meme.objects.all().order_by("-created_at") | ||
| subjects = Subject.objects.filter(memes__isnull=False).distinct() | ||
|
|
||
| # Filter by subject if provided | ||
| subject_filter = request.GET.get("subject") | ||
| if subject_filter: | ||
|
|
||
| # Filter by user if provided | ||
| user_filter = request.GET.get("user") | ||
|
|
||
| # Get distinct uploaders from the database | ||
| uploaders = memes.values("uploader__id", "uploader__username").distinct() | ||
| memes_creators = {str(uploader["uploader__id"]): uploader["uploader__username"] for uploader in uploaders} |
There was a problem hiding this comment.
Drop the -created_at ordering before calling values().distinct().
memes still carries order_by("-created_at") when you execute memes.values(...).distinct(). On PostgreSQL this becomes SELECT DISTINCT ... ORDER BY created_at, which fails because the ordered column isn’t in the projection. As soon as someone loads the page you’ll hit a database error. Call .order_by() (with no args) before values() or build the uploader queryset off a fresh Meme.objects.order_by().values(...) so the DISTINCT runs without the invalid ORDER BY.
🤖 Prompt for AI Agents
In web/views.py around lines 4437 to 4448, the queryset `memes` still has
`.order_by("-created_at")` when you call `memes.values(...).distinct()`, which
on Postgres produces an invalid ORDER BY on columns not in the projection;
remove that ordering before calling `values().distinct()` by either calling
`memes = memes.order_by()` (no args) prior to `values(...)` or build the
uploader queryset from a fresh un-ordered queryset like
`Meme.objects.order_by().values(...).distinct()`, so the DISTINCT query runs
without the invalid ORDER BY.
Related issues
Fixes #619
Checklist
Fully customized search bar to search for a specific subject and in a specific user.
Before:
After:
Summary by CodeRabbit
New Features
Chores