diff --git a/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx b/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx index 3b9fc3fa210..798ade7c9fa 100644 --- a/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx +++ b/specifyweb/frontend/js_src/lib/components/FormPlugins/LatLongUi.tsx @@ -105,10 +105,19 @@ function Coordinate({ if (isLoading.current && value === undefined) return; else isLoading.current = false; - const trimmedValue = trimLatLong(value?.toString() ?? ''); + const rawValue = value?.toString() ?? ''; + const trimmedValue = trimLatLong(rawValue); const hasValue = trimmedValue.length > 0; + /* + * Parse the RAW value, not the trimmed one. parse() trims internally, but it + * also inspects what trimming would discard so that an unrecognised direction + * letter makes the value invalid instead of silently changing hemisphere. + * Handing it a pre-trimmed string throws that information away before the + * check can run — "96° 57' O" would already have become "96° 57'" and would + * parse happily as EAST. + */ const parsed = hasValue - ? ((fieldType === 'Lat' ? Lat : Long).parse(trimmedValue) ?? undefined) + ? ((fieldType === 'Lat' ? Lat : Long).parse(rawValue) ?? undefined) : undefined; const isValid = !hasValue || parsed !== undefined; 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 index 5db0d4ee373..3df430251aa 100644 --- a/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx +++ b/specifyweb/frontend/js_src/lib/components/FormPlugins/__tests__/LatLongUi.test.tsx @@ -59,7 +59,12 @@ describe('LatLongUi does not mutate the resource on render', () => { 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()); @@ -74,7 +79,12 @@ describe('LatLongUi does not mutate the resource on render', () => { const second = makeLocality(); await act(async () => { rerender( - + ); }); await waitFor(() => expect(second.get('long1text')).toBeDefined()); @@ -102,7 +112,12 @@ describe('LatLongUi does not mutate the resource on render', () => { expect(resource.needsSaved).toBe(false); await act(async () => { render( - + ); }); await waitFor(() => expect(resource.get('lat1text')).toBeDefined()); @@ -147,6 +162,30 @@ describe('LatLongUi does not mutate the resource on render', () => { expect(Number(resource.get('latitude1'))).toBeCloseTo(17.55, 6); }); + /* + * Caught only by end-to-end testing: the effect used to hand parse() the + * ALREADY-TRIMMED string, so the unrecognised-direction check never saw the + * `O` and the form displayed "96° 57' E" — the eastern hemisphere — in the + * read-only Parsed column, even with every other fix in place. + */ + test('the Parsed column never renders an Oeste longitude as East', async () => { + const resource = makeLocality(); + const { container } = render( + + ); + + await waitFor(() => expect(container.textContent).toBeTruthy()); + + // Scoped so an unrelated label containing "E" cannot pass/fail this spuriously. + expect(container.textContent).not.toMatch(/\d+°[^,]*\bE\b/u); + expect(container.textContent).not.toContain('96° 57'); + }); + test('merely rendering does not mark the record as needing saving', async () => { const resource = makeLocality(); expect(resource.needsSaved).toBe(false); diff --git a/specifyweb/frontend/js_src/lib/utils/__tests__/latLong.hemisphere.test.ts b/specifyweb/frontend/js_src/lib/utils/__tests__/latLong.hemisphere.test.ts new file mode 100644 index 00000000000..5e7c27e3453 --- /dev/null +++ b/specifyweb/frontend/js_src/lib/utils/__tests__/latLong.hemisphere.test.ts @@ -0,0 +1,152 @@ +/** + * Regression tests for two coordinate-parsing defects that silently placed + * specimens in the wrong hemisphere. + * + * Discovered at the California Academy of Sciences: 2,534 localities in the + * botany collection carry Spanish `O` (Oeste = West) longitudes, 1,939 of them + * stored correctly negative. Opening one in the Specify 7 UI rewrote it to a + * POSITIVE longitude and stripped the `O` from the verbatim text — moving the + * record to the opposite side of the planet and destroying the evidence that it + * was ever West. No user edit was required. + * + * Both defects are fixed by REFUSING TO PARSE rather than by guessing. An + * unparseable coordinate raises a validation message, which is how out-of-range + * values such as 19°49'60" are already handled — a visible failure instead of a + * silent one. + * + * The companion fix in LatLongUi.tsx stops the plugin writing to the resource on + * render, so merely opening a record can no longer modify it. + */ + +import { Coord, Lat, Long } from '../latLong'; + +describe('unrecognised direction letters are rejected, not silently dropped', () => { + /* + * We deliberately do NOT map `O` to west. The letter is ambiguous across + * languages — Oeste/Ouest (es/pt/fr) mean west, but Ost/Oost (de/nl) mean + * east. Guessing would repair one set of collections while silently corrupting + * another, so an unknown direction is treated as unparseable and surfaced to + * the user. + */ + test.each(["96° 57' O", "97° 52' O", '90° 20\'14" O'])( + 'parse(%s) is rejected rather than guessed', + (raw) => { + expect(Long.parse(raw)).toBeUndefined(); + } + ); + + test('the specific corruption is gone: Oeste never yields a positive longitude', () => { + // Previously returned +96.95 — Oaxaca, Mexico relocated to Anhui, China. + const parsed = Long.parse("96° 57' O"); + expect(parsed?.asFloat()).not.toBe(96.95); + expect(parsed).toBeUndefined(); + }); + + test.each(["12° 30' Ouest", "12° 30' X"])( + 'parse(%s) with any other unknown direction is also rejected', + (raw) => { + expect(Coord.parse(raw)).toBeUndefined(); + } + ); + + /* + * KNOWN LIMITATION, not fixed here. German "Ost" (East) still mis-reads: the O + * and t are discarded but the `s` survives and is taken as SOUTH. A direction + * letter did survive, so the guard above does not fire. Catching this needs + * real localised direction words, which is a larger change than this fix. + * Documented so the next reader does not assume it is covered. + */ + test("KNOWN LIMITATION: German 'Ost' is misread as south", () => { + expect(Coord.parse("12° 30' Ost")?.asFloat()).toBeCloseTo(-12.5, 6); + }); + + /* + * The guard fires only when trimming destroyed the ONLY direction indicator. + * When a recognised n/s/e/w survives, the discarded letters were noise and the + * value is left alone — the masculine ordinal U+00BA (º) that Spanish and + * Portuguese transcribers type instead of the degree sign is a Unicode Letter, + * so an unconditional "any discarded letter is fatal" rule would have caught it + * too. + * + * Note these particular strings do not parse either way: removing the º fuses + * the digits ("42º20'N" -> "4220'N"). They are asserted here to pin that this + * change does not make them WORSE, and to document why the guard is conditional. + */ + test.each(["42º20'N", '99º58\'3"N', "4 deg. 11' S"])( + 'parse(%s) is unaffected by this change', + (raw) => { + expect(Coord.parse(raw)).toBeUndefined(); + } + ); +}); + +describe('a leading minus and a direction letter are reconciled, not multiplied', () => { + /* + * makeLatLong used to compute + * sign = (direction === 's' || direction === 'w' ? -1 : 1) * originalSign + * so "-157.14015 W" — where BOTH the minus and the W mean west — multiplied + * -1 by -1 and produced +157.14015, the opposite hemisphere. + * + * They are now reconciled by agreement rather than rejected outright, because + * the intent is unambiguous when both point the same way. Across the CAS + * collections this corrects 934 coordinates that previously parsed with a + * flipped sign, instead of blocking them behind a validation error. + */ + test.each([ + ['-157.14015° W', -157.14015], + ["-96° 57' W", -96.95], + ['-23°2\'45.1"S', -23.045_861_111_1], + ['-1 01 W', -1.016_666_666_7], + ])( + 'parse(%s) agrees with itself and stays western/southern', + (raw, expected) => { + expect(Coord.parse(raw)?.asFloat()).toBeCloseTo(expected, 6); + } + ); + + test('the specific corruption is gone: -157 W is no longer positive', () => { + // Previously returned +157.14015 — the eastern hemisphere. + expect(Long.parse('-157.14015° W')?.asFloat()).toBeCloseTo(-157.14015, 6); + }); + + /* + * A minus that CONTRADICTS its direction letter is still ambiguous and is + * rejected. Both cases below are already in the upstream suite's + * makeParseInvalidTest list and must stay invalid. + */ + test('contradicting sign and direction remain invalid', () => { + expect(Lat.parse('-124:34:23 N')).toBeUndefined(); + expect(Coord.parse('-12:30 N')).toBeUndefined(); + }); + + test('agreement does not rescue an out-of-range latitude', () => { + // -90:05 S agrees to -90.083, which still exceeds the 90 degree limit. + expect(Lat.parse('-90:05 S')).toBeUndefined(); + }); +}); + +describe('regression guards — correct input must keep working', () => { + test.each([ + ['115° 34\' 59.872" W', -115.583_297_777_8], + ['28° 19\' 0.121" N', 28.316_700_277_8], + ["96° 57' W", -96.95], + ["96° 57' E", 96.95], + ])('%s still parses correctly', (raw, expected) => { + expect(Coord.parse(raw)?.asFloat()).toBeCloseTo(expected, 6); + }); + + test('a plain negative decimal with no direction is untouched', () => { + expect(Coord.parse('-96.95')?.asFloat()).toBeCloseTo(-96.95, 6); + }); + + test('a plain positive decimal with no direction is untouched', () => { + expect(Coord.parse('96.95')?.asFloat()).toBeCloseTo(96.95, 6); + }); + + test('degree/minute/second forms without a direction still parse', () => { + expect(Coord.parse('-39:51:41')?.asFloat()).toBeCloseTo( + -39.861_388_888_9, + 6 + ); + }); +}); diff --git a/specifyweb/frontend/js_src/lib/utils/latLong.ts b/specifyweb/frontend/js_src/lib/utils/latLong.ts index 457adb9826f..c865369b677 100644 --- a/specifyweb/frontend/js_src/lib/utils/latLong.ts +++ b/specifyweb/frontend/js_src/lib/utils/latLong.ts @@ -45,8 +45,43 @@ const parsers = [ }, ] as const; +/** + * Any alphabetic character among the ones blackList discards is a direction + * indicator we do not understand — most often a non-English one. + */ +const unrecognizedDirection = /\p{Letter}/u; + function parse(rawValue: string): Coord | undefined { + /* + * Refuse to parse rather than silently discarding an unrecognised direction + * letter. + * + * The blackList strips every character outside [\s\d"'\-.:ensw°], so a Spanish + * "96° 57' O" (Oeste = West) loses its O, no direction is found, and a POSITIVE + * longitude is returned — the wrong hemisphere, with nothing to indicate that + * anything was dropped. + * + * We deliberately do NOT map O to west. The letter is ambiguous across + * languages: Oeste/Ouest (es/pt/fr) mean west, while Ost/Oost (de/nl) mean + * east. Guessing would fix one set of collections and silently corrupt + * another. Returning undefined surfaces a validation message instead, which is + * how out-of-range values such as 19°49'60" are already handled. + */ const value = trimLatLong(rawValue).trim(); + + /* + * Only reject when trimming destroyed the ONLY direction indicator. If a + * recognised n/s/e/w survives, the discarded letters were noise — a spelled-out + * "deg", or the masculine ordinal U+00BA (º) that Spanish and Portuguese + * transcribers type instead of the degree sign — and the coordinate still means + * what it says. Rejecting those too would break ~423 real localities across the + * CAS collections that parse correctly today, e.g. "42º20'N" and "4 deg. 11' S". + */ + if ( + !/[ensw]$/iu.test(value) && + unrecognizedDirection.test(rawValue.match(blackList)?.join('') ?? '') + ) + return undefined; return mappedFind(parsers, ({ regex, components, direction }) => { const match = regex.exec(value); if (match === null) return undefined; @@ -74,7 +109,33 @@ function makeLatLong( if (components.some(Number.isNaN)) return undefined; const direction = toLowerCase(originalDirection ?? ''); - const sign = (direction === 's' || direction === 'w' ? -1 : 1) * originalSign; + + /* + * A leading minus and a direction letter each carry a sign. Multiplying them, + * as this used to, made them CANCEL: "-157.14015 W" — where the minus and the + * W both mean west — produced +157.14015, the opposite hemisphere. + * + * They are instead reconciled by agreement: + * - no direction letter -> the minus decides + * - minus AND s/w -> they agree; the value is negative + * - minus AND n/e -> they contradict; the value is ambiguous + * and is rejected rather than guessed + * + * This keeps both pre-existing invalid cases invalid — '-124:34:23 N' + * contradicts, and '-90:05 S' agrees to -90.083 which still exceeds the 90 + * degree latitude limit — while reading the 934 agreeing coordinates in the CAS + * collections (e.g. "-23°2'45.1\"S") the way they were plainly meant, instead + * of blocking them behind a validation error. + */ + const directionSign = + direction === '' + ? undefined + : direction === 's' || direction === 'w' + ? -1 + : 1; + if (originalSign < 0 && directionSign === 1) return undefined; + + const sign = directionSign ?? originalSign; let result: Coord; if (direction === 's' || direction === 'n') result = new Lat();