fix(export): default group-by to None when unset/stale; polish
Final code review of the configurable-data-export feature: ExperimentDetail was forwarding its on-page groupField state (defaults to '__name__') into ExportDataModal, leaking a clustering default into the default (untouched) export. Now passes the raw saved localStorage value, defaulting to '__none__'. The modal also now validates the incoming groupField against '__none__'/'__name__'/'__id__'/active subjectTemplate fields and coerces to '__none__' otherwise, guarding against stale/deleted subject_info fields. Plus a stale module comment fix and optional-chaining consistency fix in listSubjectMembers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -31,7 +31,11 @@ export default function ExportDataModal({
|
|||||||
const [rowDim, setRowDim] = useState('date');
|
const [rowDim, setRowDim] = useState('date');
|
||||||
const [colDim, setColDim] = useState('subject');
|
const [colDim, setColDim] = useState('subject');
|
||||||
const [pinnedId, setPinnedId] = useState(null);
|
const [pinnedId, setPinnedId] = useState(null);
|
||||||
const [groupBy, setGroupBy] = useState(groupField ?? '__none__');
|
const [groupBy, setGroupBy] = useState(() => {
|
||||||
|
const valid = groupField === '__none__' || groupField === '__name__' || groupField === '__id__'
|
||||||
|
|| subjectTemplate.some((f) => f.active && f.fieldId === groupField);
|
||||||
|
return valid ? groupField : '__none__';
|
||||||
|
});
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
let alive = true;
|
let alive = true;
|
||||||
|
|||||||
@@ -1,4 +1,6 @@
|
|||||||
// Pure helpers for exporting daily-parameter data as a date×subject CSV matrix.
|
// Pure helpers for exporting daily-parameter data as a CSV matrix. buildMatrix is
|
||||||
|
// dimension-agnostic: rows and columns are assignable to Date/Subject/Parameter,
|
||||||
|
// with the leftover dimension pinned to a single value.
|
||||||
// No React, no I/O — mirrors the crossSubjectChart.js pure-helper pattern.
|
// No React, no I/O — mirrors the crossSubjectChart.js pure-helper pattern.
|
||||||
|
|
||||||
// Read a single parameter's raw value from one daily status.
|
// Read a single parameter's raw value from one daily status.
|
||||||
@@ -112,7 +114,7 @@ export function listSubjectMembers(animals, groupField) {
|
|||||||
const members = list.map((a) => {
|
const members = list.map((a) => {
|
||||||
const name = a?.animal_name ?? '';
|
const name = a?.animal_name ?? '';
|
||||||
const label = nameCounts.get(name) > 1 ? `${name} (${a?.animal_id_string ?? ''})` : name;
|
const label = nameCounts.get(name) > 1 ? `${name} (${a?.animal_id_string ?? ''})` : name;
|
||||||
return { id: a.id, label, animalId: a.id, group: subjectGroupValue(a, groupField) };
|
return { id: a?.id, label, animalId: a?.id, group: subjectGroupValue(a, groupField) };
|
||||||
});
|
});
|
||||||
const grouped = !!groupField && groupField !== '__none__';
|
const grouped = !!groupField && groupField !== '__none__';
|
||||||
members.sort((x, y) => {
|
members.sort((x, y) => {
|
||||||
|
|||||||
@@ -417,7 +417,7 @@ export default function ExperimentDetail() {
|
|||||||
dailyTemplate={dailyTemplate}
|
dailyTemplate={dailyTemplate}
|
||||||
animals={animals}
|
animals={animals}
|
||||||
subjectTemplate={subjectTemplate}
|
subjectTemplate={subjectTemplate}
|
||||||
groupField={groupField}
|
groupField={localStorage.getItem(`exp-subject-group-${id}`) ?? '__none__'}
|
||||||
onClose={() => setShowExport(false)}
|
onClose={() => setShowExport(false)}
|
||||||
/>
|
/>
|
||||||
</Modal>
|
</Modal>
|
||||||
|
|||||||
@@ -22,6 +22,44 @@ beforeEach(() => {
|
|||||||
client.experimentsApi.getDailyStatuses.mockResolvedValue(statuses);
|
client.experimentsApi.getDailyStatuses.mockResolvedValue(statuses);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Intercepts the Blob passed to URL.createObjectURL during handleExport so tests
|
||||||
|
// can inspect the actual CSV text produced, rather than inferring it from the
|
||||||
|
// <select>'s DOM value (which can be misleading — see the coercion test below).
|
||||||
|
// Also stubs anchor .click() — jsdom treats a real click on an <a href="blob:...">
|
||||||
|
// as a navigation attempt, which it doesn't implement; that throws asynchronously
|
||||||
|
// (after the test has moved on) and can flakily fail an unrelated later test.
|
||||||
|
function mockDownload() {
|
||||||
|
let blob = null;
|
||||||
|
const origCreate = global.URL.createObjectURL;
|
||||||
|
const origRevoke = global.URL.revokeObjectURL;
|
||||||
|
const origClick = window.HTMLAnchorElement.prototype.click;
|
||||||
|
global.URL.createObjectURL = jest.fn((b) => { blob = b; return 'blob:mock'; });
|
||||||
|
global.URL.revokeObjectURL = jest.fn();
|
||||||
|
window.HTMLAnchorElement.prototype.click = jest.fn();
|
||||||
|
return {
|
||||||
|
getBlob: () => blob,
|
||||||
|
// handleExport schedules `setTimeout(() => URL.revokeObjectURL(url), 0)`; flush
|
||||||
|
// that pending macrotask before unmocking, or it fires after restore() has put
|
||||||
|
// back the (non-existent, in jsdom) original revokeObjectURL and throws async —
|
||||||
|
// an error jest attributes to whatever test happens to run next.
|
||||||
|
restore: async () => {
|
||||||
|
await new Promise((r) => setTimeout(r, 0));
|
||||||
|
global.URL.createObjectURL = origCreate;
|
||||||
|
global.URL.revokeObjectURL = origRevoke;
|
||||||
|
window.HTMLAnchorElement.prototype.click = origClick;
|
||||||
|
},
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
function readBlobText(blob) {
|
||||||
|
return new Promise((resolve, reject) => {
|
||||||
|
const fr = new FileReader();
|
||||||
|
fr.onload = () => resolve(fr.result);
|
||||||
|
fr.onerror = reject;
|
||||||
|
fr.readAsText(blob);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
function renderModal(props = {}) {
|
function renderModal(props = {}) {
|
||||||
return render(
|
return render(
|
||||||
<ExportDataModal
|
<ExportDataModal
|
||||||
@@ -61,4 +99,38 @@ describe('ExportDataModal', () => {
|
|||||||
fireEvent.change(rows, { target: { value: 'subject' } }); // collides with column=subject
|
fireEvent.change(rows, { target: { value: 'subject' } }); // collides with column=subject
|
||||||
await waitFor(() => expect(screen.getByLabelText('Columns')).not.toHaveValue('subject'));
|
await waitFor(() => expect(screen.getByLabelText('Columns')).not.toHaveValue('subject'));
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('defaults Group by to None when groupField is __none__, and the exported CSV has no Group row (Issue-1 regression guard: default export reproduces today\'s export)', async () => {
|
||||||
|
const dl = mockDownload();
|
||||||
|
renderModal({ groupField: '__none__' });
|
||||||
|
await screen.findByLabelText('Rows');
|
||||||
|
expect(screen.getByLabelText('Group by')).toHaveValue('__none__');
|
||||||
|
await waitFor(() => expect(screen.getByText(/1 date/i)).toBeInTheDocument());
|
||||||
|
fireEvent.click(screen.getByText('Export CSV'));
|
||||||
|
const text = await readBlobText(dl.getBlob());
|
||||||
|
expect(text).not.toMatch(/^Group,/m);
|
||||||
|
await dl.restore();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('defaults Group by to the saved field when it is a valid active subject field', async () => {
|
||||||
|
renderModal({ groupField: 'grp' });
|
||||||
|
await screen.findByLabelText('Rows');
|
||||||
|
expect(screen.getByLabelText('Group by')).toHaveValue('grp');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('coerces Group by to None when the saved field no longer exists in subjectTemplate, so the export carries no stray Group row (Issue-2 guard)', async () => {
|
||||||
|
// Note: asserting the <select>'s DOM .value here would be misleading — React's
|
||||||
|
// controlled <select> falls back to selecting the first <option> ("__none__")
|
||||||
|
// whenever the given value matches no option, regardless of whether the
|
||||||
|
// component actually validated/coerced its internal state. So we assert the
|
||||||
|
// real behavioral effect (the exported CSV) instead.
|
||||||
|
const dl = mockDownload();
|
||||||
|
renderModal({ groupField: 'ghost-field' });
|
||||||
|
await screen.findByLabelText('Rows');
|
||||||
|
await waitFor(() => expect(screen.getByText(/1 date/i)).toBeInTheDocument());
|
||||||
|
fireEvent.click(screen.getByText('Export CSV'));
|
||||||
|
const text = await readBlobText(dl.getBlob());
|
||||||
|
expect(text).not.toMatch(/^Group,/m);
|
||||||
|
await dl.restore();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -136,6 +136,25 @@ describe('buildMatrix', () => {
|
|||||||
{ statuses: gStatuses, animals: gAnimals, dailyTemplate });
|
{ statuses: gStatuses, animals: gAnimals, dailyTemplate });
|
||||||
expect(m.columns.map((c) => c.label)).not.toContain('X');
|
expect(m.columns.map((c) => c.label)).not.toContain('X');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('neither axis is Subject: drops a blank date row and a blank parameter column together', () => {
|
||||||
|
// Subject is pinned (rowDim=date, colDim=parameter). Both drop-blank filters engage:
|
||||||
|
// 07-02 has no data at all (row dropped), and the custom field X never has data (column dropped).
|
||||||
|
const dailyTemplate = [{ fieldId: 'c-x', key: 'x', label: 'X', builtin: false, active: true }];
|
||||||
|
const statuses = [
|
||||||
|
{ animal_id: 'a1', date: '2026-07-01T00:00:00.000Z', analysis_summary: { total: 5, success_rate: 0.5 } },
|
||||||
|
{ animal_id: 'a1', date: '2026-07-02T00:00:00.000Z', analysis_summary: null },
|
||||||
|
];
|
||||||
|
const subjectMember = { id: 'a1', label: 'Alpha', animalId: 'a1' };
|
||||||
|
const m = buildMatrix(
|
||||||
|
{ rowDim: 'date', colDim: 'parameter', pinnedMember: subjectMember, groupField: '__none__' },
|
||||||
|
{ statuses, animals: gAnimals, dailyTemplate },
|
||||||
|
);
|
||||||
|
expect(m.groupAxis).toBeNull();
|
||||||
|
expect(m.rows.map((r) => r.member.label)).toEqual(['2026-07-01']);
|
||||||
|
expect(m.columns.map((c) => c.label)).not.toContain('X');
|
||||||
|
expect(m.columns.map((c) => c.label)).toEqual(['Total attempts', 'Success rate']);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
import { toCSV, csvFilename } from '../src/lib/dataExport';
|
import { toCSV, csvFilename } from '../src/lib/dataExport';
|
||||||
|
|||||||
Reference in New Issue
Block a user