refactor: extract helpers and enhance fixtures#43
Conversation
PascalRepond
commented
Jan 6, 2026
PascalRepond
commented
- Also deletes cron from Dockerfile as it doesn't seem to work in production. Should probably use an external tool to run auto backups at regular intervals.
- Change Video Game icon to differentiate it better from Series icon.
📝 WalkthroughWalkthroughRefactors media filtering/query logic into new modules, delegates view context building to queries.py, removes cron setup from Docker and entrypoint, updates fixture texts and some titles, changes GAME icon mapping, and adds an optional VSCode setting. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (browser)
participant View as Django View
participant Queries as queries.build_media_context
participant Filters as filters.apply_filters
participant DB as Database (Media/Agent)
participant Paginator as Django Paginator
Client->>View: GET /media?search=...&filters...
View->>Queries: build_media_context(request)
Queries->>Filters: extract_filters + resolve_sorting
Filters->>DB: query Media with filters (joins to Agent for contributors)
DB-->>Filters: filtered queryset
Filters-->>Queries: filtered queryset
Queries->>DB: apply search (if present) / order_by
DB-->>Queries: sorted queryset
Queries->>Paginator: paginate(queryset, per_page=20)
Paginator-->>Queries: page_obj, media_list
Queries-->>View: context dict (media_list, page_obj, filters, sort, choices)
View-->>Client: render template with context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (2)src/core/fixtures/sample_data.json (2)
src/core/filters.py (2)
🔇 Additional comments (10)
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: 1
🤖 Fix all issues with AI Agents
In @src/core/queries.py:
- Around line 12-17: The lookup Q(pub_year__icontains=query) is invalid for the
IntegerField pub_year and must be replaced; in the Media.objects.filter call
remove that icontains clause and instead add a numeric-year check by trying to
parse query to an int and, if successful, include Q(pub_year__exact=parsed_year)
(or implement the alternative prefix-range logic: compute start/end years from a
numeric prefix and add Q(pub_year__gte=start, pub_year__lte=end)); ensure this
change is applied where Media.objects.filter and the
Q(pub_year__icontains=query) expression appear and keep the other Q(...) clauses
and .distinct() intact.
🧹 Nitpick comments (1)
src/core/views.py (1)
15-15: Clean refactoring to centralized context builder!The extraction of media context-building logic into
src/core/queries.pysuccessfully improves code organization and reusability. Both views now delegate to the centralizedbuild_media_contextfunction.💡 Optional: Consider simplifying the return signature
Both views unpack
build_media_contextas_, context = build_media_context(request), discarding the firstpage_objelement since it's already included in the returned context dict (context["page_obj"]).Consider modifying
build_media_contextinsrc/core/queries.pyto return only the context dict, eliminating the redundant tuple:-def build_media_context(request): - """ - Build and filter media queryset from request parameters. - - Returns a tuple of (page_obj, context_dict) ready for rendering. - ... - """ - ... - return page_obj, context +def build_media_context(request): + """ + Build and filter media queryset from request parameters. + + Returns a context dict ready for rendering. + ... + """ + ... + return contextThen simplify the view calls:
-_, context = build_media_context(request) +context = build_media_context(request)This is optional since the current implementation works correctly—it just has a minor redundancy in the API.
Also applies to: 22-23, 88-88
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.vscode/settings.jsonDockerfilescripts/entrypoint.shsrc/core/filters.pysrc/core/fixtures/sample_data.jsonsrc/core/queries.pysrc/core/templatetags/media_tags.pysrc/core/views.py
💤 Files with no reviewable changes (1)
- scripts/entrypoint.sh
🧰 Additional context used
🧬 Code graph analysis (3)
src/core/queries.py (2)
src/core/filters.py (4)
apply_filters(145-152)extract_filters(26-70)get_field_choices(73-79)resolve_sorting(12-23)src/core/models.py (1)
Media(109-227)
src/core/filters.py (1)
src/core/models.py (2)
Agent(95-106)Media(109-227)
src/core/views.py (2)
src/core/queries.py (1)
build_media_context(20-55)src/core/utils.py (2)
create_backup(46-118)delete_orphan_agents_by_ids(32-43)
🔇 Additional comments (16)
.vscode/settings.json (1)
14-15: LGTM!VSCode configuration updates are appropriate and have no impact on the application runtime.
src/core/templatetags/media_tags.py (1)
10-10: LGTM!The icon change from "computer-desktop" to "puzzle-piece" for the GAME media type improves visual differentiation from the TV/Series icon, as stated in the PR objectives.
src/core/fixtures/sample_data.json (1)
1-462: LGTM!The fixture data enhancements successfully standardize content to English and add richer, Markdown-formatted descriptions. These changes align with the PR objective to "enhance test/dev fixtures" and provide better sample data for development and testing.
Dockerfile (1)
32-35: LGTM!The removal of cron aligns with the PR objectives, which note that cron doesn't work reliably in production containers. The backup script remains available at
/app/daily_backup.shfor execution via external scheduling tools (e.g., Kubernetes CronJobs, host-level cron, or CI/CD pipelines).src/core/queries.py (2)
1-8: LGTM! Clean module structure.The imports and organization are appropriate for the extracted utilities.
20-55: Verify the redundant return ofpage_obj.The function returns
page_objboth directly in the tuple and as a key in the context dict (Line 46). This redundancy might be intentional for different consumption patterns, but it's worth confirming.Is the dual return intentional? If callers only need the context dict, consider simplifying to return just the context:
return contextOr document why both are needed in the docstring.
src/core/filters.py (10)
1-10: LGTM! Well-organized imports.The imports are appropriate and the module is clearly documented.
12-23: LGTM! Robust sorting validation.The function correctly validates sort fields against a whitelist and handles both ascending/descending order with proper defaults.
26-70: LGTM! Comprehensive filter extraction with good error handling.The function robustly extracts filters with proper handling of malformed inputs and provides user-friendly display names for the UI.
73-79: LGTM! Simple and clear utility.
82-90: LGTM! Safe contributor filtering.The function properly handles missing contributors with
.first()and returns a clear tuple result.
93-97: LGTM! Clean type filtering.
100-104: LGTM! Clean status filtering.
107-121: LGTM! Robust score filtering with proper null handling.The function correctly handles both the "none" case for unrated media and numeric scores with appropriate error handling for malformed inputs.
124-142: LGTM! Excellent defensive programming for date and content filters.The use of
contextlib.suppressprovides robust error handling for malformed date inputs, and the logic for empty/filled content checks is correct.
145-152: LGTM! Clean orchestration of all filters.The function properly composes all filtering operations and maintains a clear return signature.
- Also deletes cron from Dockerfile as it doesn't seem to work in production. Should probably use an external tool to run auto backups at regular intervals. - Changes Video Game icon to differentiate it better from Series icon. - Fixes search by year not matching pub_year field. Co-Authored-by: Pascal Repond <pascal.repond@rero.ch>
1 participant
