T002 - Refactored export functionality to improve maintainability and separation of concerns - #809
T002 - Refactored export functionality to improve maintainability and separation of concerns#809LizzzzHFFF wants to merge 16 commits into
Conversation
LizzzzHFFF
left a comment
LizzzzHFFF
left a comment
There was a problem hiding this comment.
All done, and check the orders in the export files.
LizzzzHFFF
changed the title
RickWangPerth
left a comment
RickWangPerth
left a comment
There was a problem hiding this comment.
Please add focused unit tests for CSV/XLSX header order and populated values, ENTRY_ID, empty and multi-related records, organisation scoping, and exports above SQL Server's parameter limit.
Code quality: git diff --check currently reports trailing whitespace and a final blank line. Please also remove duplicate imports and commented-out legacy code, and keep the export row-building path single-sourced and tested.
| "OBSERVATION_DATE_OLD": _attr(observation, "observation_date_old"), | ||
| "ALIVE": _raw_fk(observation, "alive"), | ||
|
|
||
| "ENTRY_ID": "", |
There was a problem hiding this comment.
P1: ENTRY_ID is exported as an empty value for every processed row.
The associated TrtDataEntry is already available as de, and DATA_ENTRY_ID is populated immediately below. Please populate ENTRY_ID from the agreed legacy source (likely de.data_entry_id) or remove the column if it is intentionally unavailable. A silently blank required column makes the export incomplete.
|
|
||
| def get_processed_export_row(entry, context): | ||
|
|
||
| observation = entry |
There was a problem hiding this comment.
P1: The detailed observations fetched into context["observations"] are never used.
get_processed_export_row() assigns observation = entry, so later related-object access falls back to per-row lazy queries despite the prefetch work above. Please resolve the observation from context["observations"] with a safe fallback, or put the complete select_related(...) on the export queryset.
| observation.observation_id: observation | ||
| for observation in _safe_queryset( | ||
| TrtObservations.objects.filter( | ||
| observation_id__in=observation_ids |
There was a problem hiding this comment.
P1: Large exports can silently lose their related data on SQL Server.
The complete observation-id set is passed through several __in queries in this helper. Once it exceeds SQL Server's parameter limit, _safe_queryset() catches the DatabaseError and returns [], so the download can succeed with related columns blank. Please query IDs in safe chunks and surface database failures rather than converting them to empty export data.
| summary_dict = { | ||
| s.observation_id: s | ||
| for s in TrvObservationSummary.objects.filter( | ||
| observation_id__in=obs_ids |
There was a problem hiding this comment.
P1: The summary-view lookup also exceeds SQL Server's parameter limit for large exports.
This query sends the entire obs_ids set in one IN (...) clause and is not protected by the helper's error handling, so a large export fails with a 500 response. Please fetch summary rows in the same safe-sized chunks as the other related data.
| if value is None: | ||
| return "" | ||
|
|
||
| return str(value) |
There was a problem hiding this comment.
P2: Escape formula-leading user text before writing CSV/XLSX cells.
Free-text fields such as comments are passed through unchanged into both formats. A value starting with =, +, -, or @ can be interpreted as a spreadsheet formula when the recipient opens the export. Please sanitize formula-leading text or explicitly write it as text.
2 participants
Summary
Changes:
Added export fields
Added Alive Status filter to Export Data page
Compatibility
Testing