diff --git a/specifyweb/frontend/js_src/jest.config.cjs b/specifyweb/frontend/js_src/jest.config.cjs index eb33b235af8..aaef15405ea 100644 --- a/specifyweb/frontend/js_src/jest.config.cjs +++ b/specifyweb/frontend/js_src/jest.config.cjs @@ -41,12 +41,12 @@ const config = { // An array of glob patterns indicating a set of files for which coverage information should be collected collectCoverageFrom: [ - '**/*.{js,jsx,ts,tsx}', - '!**/*tests*/**', - '!**/*test*', - '!**/localization/**', - '!**/*Route*/**', - '!**/*Route*', + '**/*.{js,jsx,ts,tsx}', + '!**/*tests*/**', + '!**/*test*', + '!**/localization/**', + '!**/*Route*/**', + '!**/*Route*', ], // The directory where Jest should output its coverage files diff --git a/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx b/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx index 882adfde6fe..3b9fc3fa210 100644 --- a/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx +++ b/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx @@ -38,6 +38,26 @@ function Coordinate({ false ); const isChanging = React.useRef(false); + /** + * Whether the current value arrived from an actual change — the user typing, or + * one of the two resourceOn handlers below reacting to a field being set + * elsewhere — as opposed to simply being read off the resource when the form + * first rendered. Only a real change may write back; see the guard in the + * parsing effect. + */ + const hasValueChanged = React.useRef(false); + /* + * Declared before every other effect so it runs first on a resource swap. + * A record selector slides a NEW resource into this same component instance — + * useFieldParser notes that "Resource changes when sliding in a record + * selector, but react reuses the DOM component". Without this reset the latch + * stays set after any edit and the write-back gate below would stand open for + * every subsequent record, reintroducing the very corruption this guards. + */ + React.useEffect(() => { + hasValueChanged.current = false; + }, [resource, coordinateTextField]); + React.useEffect( () => resourceOn( @@ -49,6 +69,12 @@ function Coordinate({ (resource.get(coordinateTextField) ?? '') === '' && (resource.get(coordinateField) ?? '') !== '' ) + /* + * Deliberately does NOT set hasValueChanged: this handler fires on + * mount (resourceOn(..., true)), so treating it as a change would + * write to a record the curator has only opened. Display updates; + * nothing persists until there is a real edit. + */ updateValue(resource.get(coordinateField)); }, true @@ -65,6 +91,7 @@ function Coordinate({ if (isChanging.current) return; const coordinate = resource.get(coordinateField)?.toString() ?? ''; const parsed = (fieldType === 'Lat' ? Lat : Long).parse(coordinate); + hasValueChanged.current = true; updateValue(parsed?.asFloat() ?? null); }, // Only run this when coordinate field is changed externally @@ -101,6 +128,24 @@ function Coordinate({ : undefined ); + /** + * Opening a record must never modify it. + * + * Everything above this point is display-only — the validation message and + * the formatted "Parsed" column. Everything below writes to the resource and + * marks it dirty, so it may only run in response to an actual user edit. + * + * Without this guard the effect fires on first render and rewrites + * coordinateTextField with the trimmed string. trimLatLong() drops every + * character outside [\s\d"'\-.:ensw°], so a locality stored as "96° 57' O" + * (Spanish Oeste = West, longitude -96.95) is silently rewritten to + * "96° 57' " and +96.95 — the opposite hemisphere — and the evidence that it + * was ever West is destroyed. The verbatim text is the value of record here; + * the decimal is derived from it. That makes this data loss, not a display + * concern, and it happens without the user touching a field. + */ + if (!hasValueChanged.current) return; + isChanging.current = true; /** @@ -145,12 +190,19 @@ function Coordinate({ ]); const isReadOnly = React.useContext(ReadOnlyContext); + const handleValueChange = React.useCallback( + (newValue: string): void => { + hasValueChanged.current = true; + updateValue(newValue); + }, + [updateValue] + ); return ( ); } diff --git a/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx b/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx new file mode 100644 index 00000000000..1e67556f7e2 --- /dev/null +++ b/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx @@ -0,0 +1,183 @@ +/** + * Opening a Locality record must not modify it. + * + * LatLongUi recomputes the decimal coordinate from the verbatim text inside a + * React effect. That effect is keyed on the rendered value, so it fired on the + * FIRST render — before the user had touched anything — and wrote both the + * decimal and the (blackList-trimmed) text back onto the resource. The text + * write was not silent, so simply opening a record marked it dirty; saving it + * for any unrelated reason then persisted the rewrite. + * + * At the California Academy of Sciences this silently moved 2,534 botany + * localities to the opposite hemisphere: a Spanish "96° 57' O" (Oeste = West, + * stored -96.95) became "96° 57' " and +96.95, destroying the evidence that the + * value had ever been West. + * + * These tests pin the invariant: render must be read-only with respect to the + * resource, and a genuine user edit must still update the derived fields. + */ + +import { act, fireEvent, render, waitFor } from '@testing-library/react'; +import React from 'react'; + +import { requireContext } from '../../../tests/helpers'; +import { tables } from '../../DataModel/tables'; +import { LatLongUi } from '../LatLongUi'; + +requireContext(); + +function makeLocality() { + return new tables.Locality.Resource({ + id: 1, + localityname: 'Cerro El Veinte', + lat1text: "17° 33' N", + latitude1: 17.55, + long1text: "96° 57' O", // Oeste = West + longitude1: -96.95, + srclatlongunit: 2, + }); +} + +function makeDecimalOnlyLocality() { + return new tables.Locality.Resource({ + id: 2, + localityname: 'Imported, decimal only', + latitude1: 17.55, + longitude1: -96.95, + }); +} + +describe('LatLongUi does not mutate the resource on render', () => { + /* + * Record-set navigation reuses the component instance — useFieldParser says so + * outright: "Resource changes when sliding in a record selector, but react + * reuses the DOM component". A one-way "has the value changed" ref therefore + * stays latched after any edit, leaving the write-back gate open for every + * record the curator slides to afterwards. That resurrects the corruption in + * the batch-review workflow, where it does the most damage. + */ + test('sliding to another record does not rewrite it, even after an edit', async () => { + const first = makeLocality(); + const { rerender } = render( + + ); + await waitFor(() => expect(first.get('long1text')).toBeDefined()); + + // Simulate a real user edit on the first record. + const input = document.querySelectorAll('input')[1] as HTMLInputElement; + await act(async () => { + fireEvent.change(input, { target: { value: "96° 57' W" } }); + }); + await waitFor(() => expect(first.get('long1text')).toBe("96° 57' W")); + + // Now slide to a different record, as a record set does. + const second = makeLocality(); + await act(async () => { + rerender( + + ); + }); + await waitFor(() => expect(second.get('long1text')).toBeDefined()); + + expect(second.get('long1text')).toBe("96° 57' O"); + expect(Number(second.get('longitude1'))).toBeCloseTo(-96.95, 6); + expect(second.needsSaved).toBe(false); + }); + + /* + * KNOWN REMAINING GAP, deliberately not fixed here. + * + * A record imported with only a decimal (typical of WorkBench / LocalityUpdate) + * still becomes dirty on open: the mount-time back-fill calls updateValue, and + * useFieldParser writes lat1text NON-silently — a path outside this patch's + * gate. Nothing is corrupted (the text is generated from the decimal, and the + * decimal is not rewritten), but the record is flagged as needing saving. + * + * Asserted as-is so the limitation is visible rather than assumed fixed. The + * scope of this patch is therefore "opening cannot CORRUPT a record", not the + * broader "opening cannot touch a record". + */ + test('KNOWN GAP: a decimal-only record is still marked dirty on open', async () => { + const resource = makeDecimalOnlyLocality(); + expect(resource.needsSaved).toBe(false); + await act(async () => { + render( + + ); + }); + await waitFor(() => expect(resource.get('lat1text')).toBeDefined()); + expect(resource.needsSaved).toBe(true); + // The decimal itself is untouched — no corruption, only a dirty flag. + expect(Number(resource.get('latitude1'))).toBeCloseTo(17.55, 6); + }); + + test('merely rendering leaves the verbatim text untouched', async () => { + const resource = makeLocality(); + render( + + ); + + await waitFor(() => expect(resource.get('long1text')).toBeDefined()); + + // The O must survive. Previously this became "96° 57' ". + expect(resource.get('long1text')).toBe("96° 57' O"); + expect(resource.get('lat1text')).toBe("17° 33' N"); + }); + + test('merely rendering leaves the decimal untouched', async () => { + const resource = makeLocality(); + render( + + ); + + await waitFor(() => expect(resource.get('longitude1')).toBeDefined()); + + // Previously flipped to +96.95. + expect(Number(resource.get('longitude1'))).toBeCloseTo(-96.95, 6); + expect(Number(resource.get('latitude1'))).toBeCloseTo(17.55, 6); + }); + + test('merely rendering does not mark the record as needing saving', async () => { + const resource = makeLocality(); + expect(resource.needsSaved).toBe(false); + + render( + + ); + + await waitFor(() => expect(resource.get('long1text')).toBeDefined()); + + // A dirty record is what let an unrelated Save persist the corruption. + expect(resource.needsSaved).toBe(false); + }); +});