diff --git a/src/lib/bulkOrderPayload.js b/src/lib/bulkOrderPayload.js index b7ec215..d73dfbf 100644 --- a/src/lib/bulkOrderPayload.js +++ b/src/lib/bulkOrderPayload.js @@ -10,6 +10,7 @@ // so a test that passes is a statement about what the page does, not about a // copy of it that happens to agree today. +import { latitude, longitude } from '@/lib/coords'; import { PICKUP_SOURCE, buildFlowFields, flowForDraft } from '@/lib/orderFlow'; export const BULK_SERVICE_OPTION = 'Normal'; @@ -83,15 +84,40 @@ export const buildBulkBookingPayload = ({ pickupaddress: origin.address || '', pickuppincode: String(origin.pincode || origin.postcode || ''), pickupcity: origin.city || '', - pickuplatitude: Number(origin.latitude) || 0, - pickuplongitude: Number(origin.longitude) || 0, + // A coordinate we do not have is sent as null, never as 0. + // + // `Number(x) || 0` turned every missing pin into (0, 0), and the backend + // cannot tell that apart from a deliberate pin in the Gulf of Guinea. For + // the PICKUP end it currently recovers — adminController falls back to the + // hub or tenant location when pickup is 0,0 — but relying on that means + // relying on a rescue for data we should not have sent. + pickuplatitude: latitude(origin.latitude), + pickuplongitude: longitude(origin.longitude), customer_phone: row?.contactno != null ? String(row.contactno) : '', customer_name: row?.firstname || '', deliveryaddress: row?.address || '', deliverypincode: row?.postcode != null ? String(row.postcode) : '', deliverycity: row?.city || '', - deliverylatitude: Number(row?.latitude) || 0, - deliverylongitude: Number(row?.longitude) || 0, + // The DELIVERY end has no rescue, which is where the money went. With + // delivery at 0,0 the backend skips its distance calculation entirely + // (`if booking.Deliverylatitude != 0 && ...`) and prices the order at base + // rate with no distance component — or, when the console did manage to + // quote a charge from those same 0,0 coordinates, bills an 11,000 km run. + // One defect, opposite errors. + // + // Be clear about what null buys and what it does not. It does NOT make the + // server refuse: adminController declares these as `float64`, not + // `*float64`, and Go unmarshals JSON null into a float64 as 0 with no + // error — verified, not assumed. So by the time a null reaches the handler + // it is indistinguishable from the 0 this used to send. + // + // The submission guard in MultipleOrders is therefore the actual fix; this + // is defence in depth and an honest local shape. It stops the console + // itself from treating a missing pin as a position — the distance + // calculation and the zone filters read these values too — and it means a + // future `*float64` on the server side starts working with no change here. + deliverylatitude: latitude(row?.latitude), + deliverylongitude: longitude(row?.longitude), deliverytime, service_option: BULK_SERVICE_OPTION, finalprice: Number(row?.totalcharge) || 0, diff --git a/src/lib/coords.js b/src/lib/coords.js new file mode 100644 index 0000000..db52350 --- /dev/null +++ b/src/lib/coords.js @@ -0,0 +1,116 @@ +// ==============================|| Coordinates ||============================== // +// +// One place that decides whether a latitude/longitude is real. +// +// ── Why this file exists ── +// +// The console validated coordinates with `Number.isFinite(Number(value))` in +// about twenty places, and that guard does not do what it looks like it does: +// +// Number(null) === 0 → finite → passes +// Number('') === 0 → finite → passes +// Number(false) === 0 → finite → passes +// +// A missing coordinate therefore became latitude 0, longitude 0 — a point in +// the Gulf of Guinea — and every downstream consumer treated it as a real +// place. The cost was not a blank field, it was a wrong price: OSRM refuses to +// route to open ocean, `calculateDrivingRoute` falls back to Haversine, and a +// Coimbatore pickup measured against 0,0 comes out at roughly 11,000 km. That +// distance reaches `calculateTotalCharge`, becomes the row's `finalprice`, and +// the backend honours it verbatim (`adminController.go`: `if req.Finalprice > 0`). +// The same defect bills the opposite way when the console fails to quote at +// all: the backend then measures for itself, skips the calculation because +// delivery is 0,0, and charges base price with no distance component. +// +// So a zero coordinate is rejected here rather than merely reported. On Earth +// (0, 0) is a valid point; in this product it is only ever the residue of a +// failed geocode, and treating it as data has never once been correct. +// +// Range is checked too, because nothing in the console checked it before. It +// is worth being precise about what that does and does not buy, since the +// axes genuinely do swap inside this codebase — OSRM takes lon,lat while +// Leaflet takes lat,lng — so a transposed call site is a live risk: +// +// Caught: a longitude past 90 read as a latitude. Dibrugarh (27.4728, +// 94.9120) transposed gives latitude 94.9, past the pole. +// NOT caught: anything west of 90°E. Coimbatore is (11.0168, 76.9558), and +// 76.9558 is a legal latitude — northern Siberia — so a +// transposed Coimbatore pin satisfies every rule here. +// +// Detecting that second case needs a plausibility check against the operating +// region rather than a bounds check; `geocodingService.isWithinIndia` is the +// right tool and is deliberately not folded in here, because this module is +// about whether a number is a coordinate at all, not about where we operate. +// +// The logic is lifted from `hubForm.js`, which has always got this right. It +// was correct in one form and nowhere else; now it is correct in one place. + +/** Latitude is bounded at the poles, longitude at the antimeridian. */ +const BOUNDS = Object.freeze({ lat: 90, lng: 180 }); + +/** + * One coordinate, or null when there isn't one. + * + * @param {unknown} value raw cell, form field, or API field + * @param {'lat'|'lng'} axis which bound to apply + * @returns {number|null} a usable coordinate, or null + */ +export const coord = (value, axis) => { + if (value === null || value === undefined || value === '') return null; + // A boolean coerces to 0/1 and an empty array to 0. Neither is a coordinate, + // and both reach here from spreadsheet cells. + if (typeof value === 'boolean') return null; + + const n = Number(value); + if (!Number.isFinite(n)) return null; + if (n === 0) return null; + + const max = BOUNDS[axis]; + if (max === undefined) return null; + return n < -max || n > max ? null : n; +}; + +export const latitude = (value) => coord(value, 'lat'); +export const longitude = (value) => coord(value, 'lng'); + +/** + * True when both halves of a pair are usable. + * + * Both or neither: a point with one axis is not half a location, it is no + * location, and passing it on lets the missing half default to 0 somewhere + * further downstream — which is the bug this module exists to end. + */ +export const hasCoords = (lat, lng) => + coord(lat, 'lat') !== null && coord(lng, 'lng') !== null; + +/** + * A `{ latitude, longitude }` pair, or null. + * + * Reads the shapes this codebase actually passes around — `{latitude,longitude}` + * from the API and the forms, `{lat,lng}` from Leaflet and the map pickers — + * so callers stop re-deriving which spelling they were handed. + */ +export const coordsOf = (source) => { + if (!source || typeof source !== 'object') return null; + const lat = coord(source.latitude ?? source.lat, 'lat'); + const lng = coord(source.longitude ?? source.lng, 'lng'); + if (lat === null || lng === null) return null; + return { latitude: lat, longitude: lng }; +}; + +/** + * The message shown when a coordinate is missing or wrong, or null when it is + * fine. Kept next to the rule so the form text and the validation cannot drift. + */ +export const coordError = (value, axis) => { + const axisName = axis === 'lat' ? 'Latitude' : 'Longitude'; + if (value === null || value === undefined || value === '') { + return 'Navigate cannot work without a pin'; + } + const n = Number(value); + if (typeof value === 'boolean' || !Number.isFinite(n)) return 'Enter a number'; + if (n === 0) return 'Navigate cannot work without a pin'; + const max = BOUNDS[axis]; + if (n < -max || n > max) return `${axisName} must be between ${-max} and ${max}`; + return null; +}; diff --git a/src/lib/distance.js b/src/lib/distance.js index f2cc58d..0058db3 100644 --- a/src/lib/distance.js +++ b/src/lib/distance.js @@ -4,6 +4,8 @@ * and falls back seamlessly to Haversine with a 1.3 road winding multiplier. */ +import { coordsOf } from '@/lib/coords'; + let lastRouteDurationMin = null; export const getLastRouteDurationMin = () => lastRouteDurationMin; @@ -15,15 +17,27 @@ export const getLastRouteDurationMin = () => lastRouteDurationMin; * @returns {Promise<{ distance: number, minutes: number, polyline: Array<[number, number]>, resolved: boolean }>} */ export const calculateDrivingRoute = async (origin, destination) => { - const lat1 = Number(origin?.latitude); - const lon1 = Number(origin?.longitude); - const lat2 = Number(destination?.latitude); - const lon2 = Number(destination?.longitude); - - if (!Number.isFinite(lat1) || !Number.isFinite(lon1) || !Number.isFinite(lat2) || !Number.isFinite(lon2)) { + // Validated through lib/coords rather than with Number.isFinite. + // + // The previous guard was `Number.isFinite(Number(value))`, which accepts + // null, '' and false — all three coerce to 0, and 0 is finite. A missing pin + // therefore reached OSRM as (0, 0). OSRM answers HTTP 200 with no `routes` + // for a point in open ocean, so control fell through to the Haversine + // fallback below and a Coimbatore pickup measured against 0,0 returned + // roughly 11,000 km. That number becomes the row's charge and the backend + // honours it verbatim, so the guard being wrong here was a billing defect, + // not a display one. + const from = coordsOf(origin); + const to = coordsOf(destination); + if (!from || !to) { throw new Error("Invalid coordinates"); } + const lat1 = from.latitude; + const lon1 = from.longitude; + const lat2 = to.latitude; + const lon2 = to.longitude; + // 1. Try OSRM API with full GeoJSON geometry for live polyline map preview const osrmBaseUrl = (typeof import.meta !== 'undefined' && import.meta?.env?.VITE_OSRM_URL) || diff --git a/src/lib/geocodingService.js b/src/lib/geocodingService.js index 33f21a6..50024fb 100644 --- a/src/lib/geocodingService.js +++ b/src/lib/geocodingService.js @@ -145,10 +145,25 @@ export const standardizePlace = ({ longitude: Number.isFinite(lngNum) ? lngNum : null, provider, raw, + // Google-Maps-shaped accessors, for the call sites that still read a place + // the way the Places API returned one. + // + // These return null — not 0 — when there is no coordinate, and that is a + // fix rather than a style choice. Returning 0 made this object contradict + // itself: `.latitude` above says null while `.geometry.location.lat()` + // said 0, so which answer a caller got depended on which spelling it + // happened to read. MultipleOrders read the accessor, so a suggestion with + // no coordinates became a drop pinned at (0, 0); the pre-submit guard + // tested `== null` and waved it through; the distance calculation then + // routed Coimbatore to the Gulf of Guinea, fell back to Haversine, and + // billed roughly 11,000 km — a number the backend honours verbatim + // because the console sent it as `finalprice`. + // + // null propagates as "no pin", which every guard already handles. geometry: { location: { - lat: () => (Number.isFinite(latNum) ? latNum : 0), - lng: () => (Number.isFinite(lngNum) ? lngNum : 0) + lat: () => (Number.isFinite(latNum) && latNum !== 0 ? latNum : null), + lng: () => (Number.isFinite(lngNum) && lngNum !== 0 ? lngNum : null) } }, address_components: [ diff --git a/src/pages/doormile/orders/CreateOrder.jsx b/src/pages/doormile/orders/CreateOrder.jsx index 99d745a..07d15cd 100644 --- a/src/pages/doormile/orders/CreateOrder.jsx +++ b/src/pages/doormile/orders/CreateOrder.jsx @@ -32,7 +32,22 @@ import { getAdminTenants, getTenantLocations } from '@/api/doormile/endpoints'; -import { calculateDrivingRoute, calculateTotalCharge, getLastRouteDurationMin } from '@/lib/distance'; +// calculateDrivingDistance was used by the deadhead-leg effect below without +// ever being imported — a ReferenceError on every customer-pickup order that +// had a pinned collection address, thrown synchronously inside the effect so +// the `.catch()` on that promise chain could never see it. Neither the build +// nor the lint config catches an unbound identifier in a .jsx file, which is +// why it survived. Found while auditing the coordinate changes; unrelated to +// them. +// +// getLastRouteDurationMin is gone from this list: it was imported and never +// called, which the project's own lint reports as an error. Dropped while +// rewriting the statement rather than left behind in it. +import { + calculateDrivingDistance, + calculateDrivingRoute, + calculateTotalCharge +} from '@/lib/distance'; import { saveRecentAddress } from '@/lib/geocodingService'; import { useHubs } from '@/lib/doormileHooks'; import { diff --git a/src/pages/doormile/orders/MultipleOrders.jsx b/src/pages/doormile/orders/MultipleOrders.jsx index a906785..5cd79c5 100644 --- a/src/pages/doormile/orders/MultipleOrders.jsx +++ b/src/pages/doormile/orders/MultipleOrders.jsx @@ -37,6 +37,7 @@ import { normalizeHeader, requiredSheetColumns } from '@/lib/bulkOrderColumns'; +import { coord, hasCoords } from '@/lib/coords'; import { calculateDrivingDistance, calculateTotalCharge } from '@/lib/distance'; import { useHubs } from '@/lib/doormileHooks'; import { PICKUP_SOURCE, buildAnchors } from '@/lib/orderFlow'; @@ -465,8 +466,20 @@ export default function MultipleOrders() { }); }); - const latitude = place.geometry.location.lat(); - const longitude = place.geometry.location.lng(); + // Read the place's own fields first and fall back to the Google-shaped + // accessors, matching how line 343 already does it. Reading the accessor + // alone was the entry point for the 0,0 defect: geocodingService used to + // return 0 from `geometry.location.lat()` for a suggestion with no + // coordinates, this assignment stored that 0 verbatim, and the pre-submit + // guard's `== null` test could not see it. + const latitude = coord( + place?.latitude ?? place?.geometry?.location?.lat?.(), + 'lat' + ); + const longitude = coord( + place?.longitude ?? place?.geometry?.location?.lng?.(), + 'lng' + ); const address = place.formatted_address || ''; setDropCust((prev) => @@ -477,6 +490,20 @@ export default function MultipleOrders() { ) ); + // No pin, no quote. Pricing an unpinned row is what produced the wrong + // charges: the distance came back from a fallback measurement to 0,0 and + // the row carried it as `totalcharge`, which the backend then honours as + // `finalprice`. Clearing the charge leaves the row visibly incomplete, and + // the submit guard below refuses it. + if (latitude === null || longitude === null) { + setDropCust((prev) => + prev.map((c) => + c._rowkey === rowkey ? { ...c, distance: null, totalcharge: null } : c + ) + ); + return; + } + try { const { roundedDistance, totalcharge } = await calculateDistance({ latitude, longitude }); setDropCust((prev) => @@ -532,7 +559,17 @@ export default function MultipleOrders() { return; } - const incomplete = dropCust.find((c) => c.latitude == null || c.longitude == null || !c.address); + // Checked with hasCoords, not `== null`. + // + // `== null` matches null and undefined only, so a row carrying latitude 0 + // — which is what a suggestion with no coordinates used to produce — + // passed this guard and was submitted. It then priced either as an + // 11,000 km run or, with no charge quoted, at base rate with no distance + // component. hasCoords also rejects '' and unparseable sheet cells, which + // reach here from an uploaded file. + const incomplete = dropCust.find( + (c) => !hasCoords(c.latitude, c.longitude) || !c.address + ); if (incomplete) { OpenToast( `Please search and complete address for ${incomplete.firstname || 'one of the drops'}`, @@ -542,6 +579,31 @@ export default function MultipleOrders() { return; } + // The collection end, which was never checked here at all. CreateOrder + // gained this check already; the bulk form did not, so a hand-typed + // pickup that failed to geocode submitted every row in the file against a + // pickup the backend has to rescue from the hub record. + // + // Checked per row through rowPickup, not once against the shared pickup: + // a row that resolved its own sender address collects from THAT, and + // buildBulkBookingPayload reads the same `row.__pickup || sharedPickup`. + // Validating only the shared one would leave a per-row collection point + // unchecked, which is the half of the fan-out that varies. + const unpinnedPickup = dropCust.find((c) => { + const origin = rowPickup(c); + return !hasCoords(origin?.latitude, origin?.longitude); + }); + if (unpinnedPickup) { + OpenToast( + rowPickup(unpinnedPickup) === pickCust + ? 'The collection address has no coordinates — pick it from the suggestions or drop a map pin' + : `The sender address for ${unpinnedPickup.firstname || 'one of the drops'} has no coordinates`, + 'warning', + 4000 + ); + return; + } + const deliverytime = dayjs(`${startDate} ${selectedSlotTime}`, 'YYYY-MM-DD HH:mm').format( 'YYYY-MM-DD HH:mm:ss' ); @@ -1048,7 +1110,14 @@ export default function MultipleOrders() { {dropCust.map((row, idx) => { - const needsAddress = row.latitude == null || row.longitude == null || !row.address; + // Same rule as the submit guard, deliberately. When this + // used `== null` and the guard used hasCoords, a row + // carrying latitude 0 rendered as complete and was then + // refused on dispatch — an error with nothing on screen + // pointing at the row that caused it. Both ends ask the + // same question now, so an unusable row always shows its + // address search. + const needsAddress = !hasCoords(row.latitude, row.longitude) || !row.address; return ( {idx + 1} diff --git a/src/utils/distance.js b/src/utils/distance.js index defcd6c..dbe6338 100644 --- a/src/utils/distance.js +++ b/src/utils/distance.js @@ -6,6 +6,8 @@ * @param {Object} destination - { latitude, longitude } * @returns {Promise} - Round distance in KM */ +import { coordsOf } from '@/lib/coords'; + // Duration from the most recent OSRM route, in minutes, or null when the // Haversine fallback was used (a straight-line estimate has no honest // duration — better to show nothing than a made-up ETA). @@ -14,15 +16,25 @@ let lastRouteDurationMin = null; export const getLastRouteDurationMin = () => lastRouteDurationMin; export const calculateDrivingDistance = async (origin, destination) => { - const lat1 = origin?.latitude; - const lon1 = origin?.longitude; - const lat2 = destination?.latitude; - const lon2 = destination?.longitude; - - if (lat1 == null || lon1 == null || lat2 == null || lon2 == null) { + // Validated through lib/coords rather than with `== null`. + // + // `== null` matches null and undefined only, so 0, '' and a garbage sheet + // cell all passed and reached OSRM as (0, 0). OSRM has no route to open + // ocean, so control fell through to the Haversine fallback and a Coimbatore + // pickup came back at roughly 11,000 km — which becomes the row's charge and + // is honoured verbatim by the backend. Same defect as lib/distance.js; this + // is the second live copy of this module (both trees have importers). + const from = coordsOf(origin); + const to = coordsOf(destination); + if (!from || !to) { throw new Error("Invalid coordinates"); } + const lat1 = from.latitude; + const lon1 = from.longitude; + const lat2 = to.latitude; + const lon2 = to.longitude; + // 1. Try OSRM API (either self-hosted or public demo server for fallback) const osrmBaseUrl = (typeof import.meta !== 'undefined' && import.meta.env?.VITE_OSRM_URL) || (typeof process !== 'undefined' && process.env?.REACT_APP_OSRM_URL) || "https://router.project-osrm.org"; try { diff --git a/tests/lib/bulkOrderPayload.test.js b/tests/lib/bulkOrderPayload.test.js new file mode 100644 index 0000000..b869658 --- /dev/null +++ b/tests/lib/bulkOrderPayload.test.js @@ -0,0 +1,136 @@ +import { buildBulkBookingPayload, buildBulkBookingPayloads } from '@/lib/bulkOrderPayload'; + +/** + * Coordinates on the bulk booking payload. + * + * This is the regression guard for the billing defect. The builder used + * `Number(value) || 0` on all four coordinates, so a row with no usable pin + * was submitted as (0, 0) — and the backend cannot tell that apart from a + * deliberate pin in the Gulf of Guinea. + * + * Both failure modes came from that one substitution: + * + * • With a charge quoted, the console had measured the distance from those + * same 0,0 coordinates. OSRM refuses to route to open ocean, so the + * Haversine fallback returned roughly 11,000 km, and adminController + * honours a supplied `finalprice` verbatim. Massive overcharge. + * • With no charge quoted, the backend measured for itself — and skipped the + * calculation entirely, because its guard is + * `if booking.Deliverylatitude != 0 && ...`. Base rate, no distance + * component. Undercharge. + * + * A coordinate we do not have is now null, which the server can refuse. + */ + +const PICKUP = { + address: 'Doormile Base, Peelamedu', + pincode: '641004', + city: 'Coimbatore', + latitude: 11.0168, + longitude: 76.9558, +}; + +const ROW = { + firstname: 'Priya', + contactno: '9876543210', + address: '45 Cross Cut Rd, Gandhipuram', + postcode: '641012', + city: 'Coimbatore', + latitude: 11.0272, + longitude: 76.9722, + totalcharge: 180, +}; + +const build = (row = {}, pickup = PICKUP) => + buildBulkBookingPayload({ + row: { ...ROW, ...row }, + sharedPickup: pickup, + tenantId: 3, + tenantLocationId: 7, + deliverytime: '2026-09-18 10:00:00', + notes: '', + anchors: [], + }); + +describe('buildBulkBookingPayload — coordinates', () => { + it('passes real coordinates through untouched', () => { + const p = build(); + expect(p.pickuplatitude).toBe(11.0168); + expect(p.pickuplongitude).toBe(76.9558); + expect(p.deliverylatitude).toBe(11.0272); + expect(p.deliverylongitude).toBe(76.9722); + }); + + it('sends null, NOT 0, for a delivery pin that is missing', () => { + const p = build({ latitude: null, longitude: null }); + expect(p.deliverylatitude).toBeNull(); + expect(p.deliverylongitude).toBeNull(); + // The specific regression: 0 would have been priced as an 11,000 km run. + expect(p.deliverylatitude).not.toBe(0); + }); + + it('sends null for an empty sheet cell', () => { + // An untouched Excel cell arrives as '' rather than null, and + // `Number('') || 0` is 0. + const p = build({ latitude: '', longitude: '' }); + expect(p.deliverylatitude).toBeNull(); + expect(p.deliverylongitude).toBeNull(); + }); + + it('sends null for an unparseable sheet cell', () => { + const p = build({ latitude: 'N/A', longitude: '--' }); + expect(p.deliverylatitude).toBeNull(); + expect(p.deliverylongitude).toBeNull(); + }); + + it('refuses to forward a literal zero it was handed', () => { + // A row that already carried 0 — which is what the geocoding shim used to + // return for a suggestion with no coordinates — must not be laundered + // into a valid-looking payload. + const p = build({ latitude: 0, longitude: 0 }); + expect(p.deliverylatitude).toBeNull(); + expect(p.deliverylongitude).toBeNull(); + }); + + it('sends null for a coordinate outside its axis range', () => { + const p = build({ latitude: 94.912, longitude: 200 }); + expect(p.deliverylatitude).toBeNull(); + expect(p.deliverylongitude).toBeNull(); + }); + + it('nulls the pickup end too, rather than relying on the backend rescue', () => { + // adminController falls back to the hub or tenant location when pickup is + // 0,0. That rescue exists, but sending data that needs rescuing is not the + // same as sending correct data. + const p = build({}, { ...PICKUP, latitude: null, longitude: undefined }); + expect(p.pickuplatitude).toBeNull(); + expect(p.pickuplongitude).toBeNull(); + }); + + it('treats the two ends independently', () => { + const p = build({ latitude: null, longitude: null }); + expect(p.pickuplatitude).toBe(11.0168); + expect(p.deliverylatitude).toBeNull(); + }); +}); + +describe('buildBulkBookingPayloads — batch', () => { + it('nulls only the rows that are missing a pin', () => { + const payloads = buildBulkBookingPayloads({ + rows: [ROW, { ...ROW, latitude: 0, longitude: 0 }, { ...ROW, latitude: '' }], + sharedPickup: PICKUP, + tenantId: 3, + tenantLocationId: 7, + deliverytime: '2026-09-18 10:00:00', + notes: '', + anchors: [], + }); + + expect(payloads).toHaveLength(3); + expect(payloads[0].deliverylatitude).toBe(11.0272); + expect(payloads[1].deliverylatitude).toBeNull(); + expect(payloads[2].deliverylatitude).toBeNull(); + // The good row keeps its longitude even though a sibling lost its latitude. + expect(payloads[2].deliverylongitude).toBe(76.9722); + }); +}); diff --git a/tests/lib/coords.test.js b/tests/lib/coords.test.js new file mode 100644 index 0000000..1d94901 --- /dev/null +++ b/tests/lib/coords.test.js @@ -0,0 +1,202 @@ +import { + coord, + coordError, + coordsOf, + hasCoords, + latitude, + longitude, +} from '@/lib/coords'; + +/** + * Coordinate validation. + * + * The first block is the whole reason this module exists: the guard it replaces + * was `Number.isFinite(Number(value))`, and every case below passed it, because + * null, '' and false all coerce to 0 and 0 is finite. A missing pin became + * latitude 0 / longitude 0 and was priced as an 11,000 km run. + */ + +const COIMBATORE = { latitude: 11.0168, longitude: 76.9558 }; + +describe('coord', () => { + describe('the empty values that used to pass as zero', () => { + it.each([ + ['null', null], + ['undefined', undefined], + ['empty string', ''], + ['false', false], + ['true', true], + ['an empty array', []], + ])('rejects %s', (_label, value) => { + // Number(value) is 0 for every one of these, and 0 is finite — which is + // exactly how they got through before. + expect(coord(value, 'lat')).toBeNull(); + expect(coord(value, 'lng')).toBeNull(); + }); + + it('confirms the coercion that caused the bug', () => { + expect(Number(null)).toBe(0); + expect(Number.isFinite(Number(null))).toBe(true); + }); + }); + + describe('zero', () => { + it('rejects a literal zero on both axes', () => { + // (0, 0) is a real point in the Gulf of Guinea. In this product it is + // only ever the residue of a failed geocode. + expect(coord(0, 'lat')).toBeNull(); + expect(coord(0, 'lng')).toBeNull(); + expect(coord('0', 'lat')).toBeNull(); + }); + }); + + describe('unparseable values', () => { + it.each(['abc', 'N/A', '--', 'NaN', {}])('rejects %p', (value) => { + expect(coord(value, 'lat')).toBeNull(); + }); + }); + + describe('range', () => { + it('accepts the poles and the antimeridian exactly', () => { + expect(coord(90, 'lat')).toBe(90); + expect(coord(-90, 'lat')).toBe(-90); + expect(coord(180, 'lng')).toBe(180); + expect(coord(-180, 'lng')).toBe(-180); + }); + + it('rejects a latitude past the poles', () => { + expect(coord(90.1, 'lat')).toBeNull(); + expect(coord(-91, 'lat')).toBeNull(); + }); + + it('rejects a longitude past the antimeridian', () => { + expect(coord(180.5, 'lng')).toBeNull(); + expect(coord(-181, 'lng')).toBeNull(); + }); + + it('catches a transposition only when the longitude exceeds 90', () => { + // The honest limit of a range check. Kolkata is (22.5726, 88.3639) and + // Dibrugarh is (27.4728, 94.9120) — transpose the second and the + // "latitude" becomes 94.9, past the pole, so it is caught. + expect(coord(94.912, 'lat')).toBeNull(); + expect(hasCoords(94.912, 27.4728)).toBe(false); + }); + + it('CANNOT catch a transposed Coimbatore pin — documented limitation', () => { + // 76.9558 is a perfectly legal latitude (northern Siberia), so a + // transposed Coimbatore pin passes every check in this module. Range + // validation only catches a swap when one axis leaves the other's + // domain, i.e. |longitude| > 90. Anything west of 90°E transposes + // silently. + // + // Pinned as a test so nobody reads `coord` as swap-proof. Catching this + // needs a plausibility check against the operating region (see + // geocodingService.isWithinIndia), not a bounds check. + expect(coord(COIMBATORE.longitude, 'lat')).toBe(76.9558); + expect(hasCoords(COIMBATORE.longitude, COIMBATORE.latitude)).toBe(true); + }); + + it('rejects an unknown axis rather than guessing a bound', () => { + expect(coord(11.0168, 'elevation')).toBeNull(); + }); + }); + + describe('real coordinates', () => { + it('passes a Coimbatore pin through unchanged', () => { + expect(coord(COIMBATORE.latitude, 'lat')).toBe(11.0168); + expect(coord(COIMBATORE.longitude, 'lng')).toBe(76.9558); + }); + + it('parses a numeric string, which is how a sheet cell arrives', () => { + expect(coord('11.0168', 'lat')).toBe(11.0168); + expect(coord(' 76.9558 ', 'lng')).toBe(76.9558); + }); + + it('accepts southern and western hemispheres', () => { + expect(coord(-33.8688, 'lat')).toBe(-33.8688); + expect(coord(-74.006, 'lng')).toBe(-74.006); + }); + }); + + describe('axis helpers', () => { + it('applies the latitude bound', () => { + expect(latitude(91)).toBeNull(); + expect(latitude(11.0168)).toBe(11.0168); + }); + + it('applies the longitude bound', () => { + expect(longitude(76.9558)).toBe(76.9558); + expect(longitude(181)).toBeNull(); + }); + }); +}); + +describe('hasCoords', () => { + it('needs both halves', () => { + expect(hasCoords(11.0168, 76.9558)).toBe(true); + expect(hasCoords(11.0168, null)).toBe(false); + expect(hasCoords(null, 76.9558)).toBe(false); + expect(hasCoords(11.0168, 0)).toBe(false); + }); + + it('rejects the pair the geocoding shim used to produce', () => { + // geocodingService returned 0 from geometry.location.lat() when a place + // had no coordinates. This is that pair. + expect(hasCoords(0, 0)).toBe(false); + }); +}); + +describe('coordsOf', () => { + it('reads the API/form shape', () => { + expect(coordsOf(COIMBATORE)).toEqual({ latitude: 11.0168, longitude: 76.9558 }); + }); + + it('reads the Leaflet/map-picker shape', () => { + expect(coordsOf({ lat: 11.0168, lng: 76.9558 })).toEqual({ + latitude: 11.0168, + longitude: 76.9558, + }); + }); + + it('returns null when either half is unusable', () => { + expect(coordsOf({ latitude: 11.0168, longitude: null })).toBeNull(); + expect(coordsOf({ latitude: 0, longitude: 0 })).toBeNull(); + expect(coordsOf({ latitude: '', longitude: '' })).toBeNull(); + }); + + it('returns null for a non-object', () => { + expect(coordsOf(null)).toBeNull(); + expect(coordsOf(undefined)).toBeNull(); + expect(coordsOf('11.0168,76.9558')).toBeNull(); + }); + + it('prefers the explicit spelling when both are present', () => { + expect(coordsOf({ latitude: 11.0168, lat: 99, longitude: 76.9558, lng: 99 })).toEqual({ + latitude: 11.0168, + longitude: 76.9558, + }); + }); +}); + +describe('coordError', () => { + it('names a missing pin rather than a number problem', () => { + expect(coordError('', 'lat')).toBe('Navigate cannot work without a pin'); + expect(coordError(null, 'lng')).toBe('Navigate cannot work without a pin'); + expect(coordError(0, 'lat')).toBe('Navigate cannot work without a pin'); + }); + + it('names a number problem when it is one', () => { + expect(coordError('abc', 'lat')).toBe('Enter a number'); + expect(coordError(true, 'lat')).toBe('Enter a number'); + }); + + it('names the axis and its bounds when out of range', () => { + expect(coordError(91, 'lat')).toBe('Latitude must be between -90 and 90'); + expect(coordError(181, 'lng')).toBe('Longitude must be between -180 and 180'); + }); + + it('is null for a good coordinate', () => { + expect(coordError(11.0168, 'lat')).toBeNull(); + expect(coordError(76.9558, 'lng')).toBeNull(); + }); +}); diff --git a/tests/lib/distance.test.js b/tests/lib/distance.test.js index decab12..05e1e5f 100644 --- a/tests/lib/distance.test.js +++ b/tests/lib/distance.test.js @@ -35,11 +35,40 @@ describe('calculateDrivingDistance', () => { expect(global.fetch).not.toHaveBeenCalled(); }); - it('should accept zero coordinates, which are a real position', async () => { + it('should reject zero coordinates, which are only ever a failed geocode', async () => { + // This assertion is INVERTED from what it used to say, and the reason is + // that the old pair of expectations could not both hold. + // + // The case above requires a null latitude to be rejected. `Number(null)` + // is 0, so to any guard built on Number() a null latitude and a zero + // latitude are the same value — there is no implementation that rejects + // one and accepts the other. The old code accepted both, which is why + // "should reject a null latitude value" was failing on main. + // + // So it is a choice, and zero loses. Geographically (0, 0) is a real + // point in the Gulf of Guinea; in this product it is not reachable — + // Doormile operates inside the India bounding box (latitude 6.5 to 37.5, + // longitude 68 to 97.5, see geocodingService.isWithinIndia) and every + // zero coordinate ever seen in production was the residue of a geocode + // that failed. Accepting them cost real money: OSRM has no route to open + // ocean, so the Haversine fallback measured a Coimbatore pickup at + // roughly 11,000 km, and the backend honours that as the order's price. global.fetch.mockResolvedValue(osrmOk(1000, 120)); await expect( calculateDrivingDistance({ latitude: 0, longitude: 0 }, { latitude: 0, longitude: 1 }) - ).resolves.toEqual(expect.any(Number)); + ).rejects.toThrow('Invalid coordinates'); + expect(global.fetch).not.toHaveBeenCalled(); + }); + + it('should reject a coordinate outside its own axis range', async () => { + // Nothing in the console checked this before. Note the limit: this + // catches a longitude read as a latitude only when it exceeds 90, so a + // transposed Coimbatore pin (11.0168, 76.9558) still passes. See + // lib/coords for why, and what would actually catch it. + await expect( + calculateDrivingDistance({ latitude: 94.912, longitude: 27.4728 }, IND) + ).rejects.toThrow('Invalid coordinates'); + expect(global.fetch).not.toHaveBeenCalled(); }); }); @@ -61,7 +90,11 @@ describe('calculateDrivingDistance', () => { await calculateDrivingDistance(BLR, IND); const url = global.fetch.mock.calls[0][0]; expect(url).toContain('/route/v1/driving/77.5946,12.9716;77.6245,12.9352'); - expect(url).toContain('overview=false'); + // The route geometry IS requested: `calculateDrivingRoute` returns a + // polyline for the live map preview, so this asks for overview=full with + // geojson geometry. It read `overview=false` until the polyline landed. + expect(url).toContain('overview=full'); + expect(url).toContain('geometries=geojson'); }); it('should use the public OSRM host by default', async () => { @@ -113,10 +146,13 @@ describe('calculateDrivingDistance', () => { it('should apply the 1.3 road multiplier to the straight-line distance', async () => { // One degree of latitude is 111.19 km great-circle; x1.3 = 144.5 -> 145. + // Measured along a meridian over India rather than from (0, 0): the + // arithmetic is identical at any longitude, and zero coordinates are now + // refused before the fetch. global.fetch.mockResolvedValue({ ok: false, json: async () => ({}) }); const distance = await calculateDrivingDistance( - { latitude: 0, longitude: 0 }, - { latitude: 1, longitude: 0 } + { latitude: 10, longitude: 77 }, + { latitude: 11, longitude: 77 } ); expect(distance).toBe(145); });