Conversation
There was a problem hiding this comment.
Pull request overview
Implements CSV export functionality across the stack by adding a backend CSV export endpoint and integrating a new Export modal into the admin UI to trigger downloads with optional filters.
Changes:
- Added
GET /api/inventory/export/endpoint that returns collection items as a CSV with optional query-parameter filters. - Added a reusable
ExportModalcomponent and wired it into Admin Dashboard and Admin Catalogue pages. - Added backend tests for the export endpoint and updated existing frontend tests to mock the new modal.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/pages/admin/AdminDashboard.tsx | Opens the new export modal from the dashboard “Export to CSV” button |
| frontend/src/pages/admin/AdminCataloguePage.tsx | Replaces client-side CSV generation with the export modal flow |
| frontend/src/pages/admin/AdminCataloguePage.test.tsx | Extends component mocks to include ExportModal |
| frontend/src/components/items/index.ts | Re-exports ExportModal from the items component barrel |
| frontend/src/components/items/ExportModal.tsx | Implements the export modal UI + download logic calling the backend export endpoint |
| frontend/src/components/items/ExportModal.css | Styling for the export modal |
| frontend/package-lock.json | Lockfile updates from dependency install |
| backend/inventory/views.py | Adds export_items CSV export endpoint with filters |
| backend/inventory/urls.py | Adds route for /api/inventory/export/ |
| backend/inventory/tests.py | Adds backend tests for CSV export endpoint behavior and filters |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| today = datetime.now().strftime("%Y%m%d") | ||
| response = HttpResponse(content_type="text/csv") | ||
| response["Content-Disposition"] = f'attachment; filename="made_export_{today}.csv"' |
There was a problem hiding this comment.
today = datetime.now() produces a naive timestamp and may not match the app's configured timezone. Use Django's timezone utilities (e.g. django.utils.timezone.localdate()/timezone.now()) so the exported filename date is consistent with the rest of the system.
| # Build CSV response | ||
| today = datetime.now().strftime("%Y%m%d") | ||
| response = HttpResponse(content_type="text/csv") | ||
| response["Content-Disposition"] = f'attachment; filename="made_export_{today}.csv"' | ||
|
|
||
| writer = csv.writer(response) | ||
| writer.writerow( | ||
| [ | ||
| "MADE ID", | ||
| "Title", | ||
| "Platform", | ||
| "Item Type", | ||
| "Box Code", | ||
| "Location", | ||
| "Location Type", | ||
| "Working Condition", | ||
| "Status", | ||
| "Created At", | ||
| ] | ||
| ) | ||
|
|
||
| for item in queryset: | ||
| writer.writerow( |
There was a problem hiding this comment.
This endpoint can return a very large CSV (especially with no filters) and currently builds it into a normal HttpResponse. For better reliability and memory usage, consider switching to StreamingHttpResponse and iterating the queryset (e.g. queryset.iterator()) so exports don't require buffering the entire file in memory.
| [ | ||
| item.item_code, | ||
| item.title, | ||
| item.platform, | ||
| item.get_item_type_display(), | ||
| item.box.box_code if item.box else "", | ||
| item.current_location.name if item.current_location else "", | ||
| item.current_location.get_location_type_display() if item.current_location else "", | ||
| "Yes" if item.working_condition else "No", | ||
| item.get_status_display(), | ||
| item.created_at.strftime("%Y-%m-%d %H:%M:%S") if item.created_at else "", | ||
| ] |
There was a problem hiding this comment.
CSV export is vulnerable to spreadsheet/CSV injection if any user-controlled string fields start with =, +, -, or @ (e.g. title, platform, location name). Consider sanitizing these values before writing (commonly by prefixing an apostrophe) so opening the CSV in Excel/Sheets can’t execute formulas.
| ) | ||
|
|
||
| if box_id: | ||
| queryset = queryset.filter(box__id=box_id) |
There was a problem hiding this comment.
box_id is used directly in the ORM filter; if a non-numeric value is provided (e.g. box_id=abc) Django will raise ValueError: Field 'id' expected a number and return a 500. Parse/validate box_id (and return a 400 on invalid input) before applying the filter, similar to the date parsing above.
| queryset = queryset.filter(box__id=box_id) | |
| try: | |
| box_id_int = int(box_id) | |
| except (TypeError, ValueError): | |
| return Response( | |
| {"error": "Invalid box_id. Must be an integer."}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| queryset = queryset.filter(box__id=box_id_int) |
| if start_date: | ||
| try: | ||
| parsed = datetime.strptime(start_date, "%Y-%m-%d") | ||
| queryset = queryset.filter(created_at__date__gte=parsed.date()) | ||
| except ValueError: | ||
| return Response( | ||
| {"error": "Invalid start_date format. Use YYYY-MM-DD."}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
||
| if end_date: | ||
| try: | ||
| parsed = datetime.strptime(end_date, "%Y-%m-%d") | ||
| queryset = queryset.filter(created_at__date__lte=parsed.date()) | ||
| except ValueError: |
There was a problem hiding this comment.
The created_at__date__gte/lte filters apply a date extraction function to the DB column, which can prevent index usage and become slow on larger tables. Consider filtering with datetime boundaries instead (e.g., created_at__gte=start_of_day and created_at__lt=end_of_day_plus_1) to keep the query sargable.
| return Response( | ||
| {"error": "Invalid start_date format. Use YYYY-MM-DD."}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) |
There was a problem hiding this comment.
Error responses here use an { "error": ... } payload, but other API endpoints in this codebase typically return { "detail": ... } for client-facing errors. For consistency (and to better align with DRF conventions), consider switching these to { "detail": ... } or raising a DRF ValidationError.
vivjd
left a comment
vivjd
left a comment
There was a problem hiding this comment.
Hey this is looking good! Can you resolve the copilot comments and re-request review? I believe some of its suggestions are actually worthwhile and could help improve this PR.
Summary
This PR implements the front-end and back-end logic for CSV exportation.
Related Issues
Changes
How to Test
Screenshots / UI (if applicable)
Checklist