Skip to content

Commit 2c539cc

Browse files
committed
fix(eta,carriers): disambiguate colliding US state codes; correct two metric denominators
#1665 — LOCATION_TOKENS maps bare two-letter tokens, and three of them are also US state codes: CA (Canada), DE (Germany) and IN (India). So 'Los Angeles, CA' matched the city table (US) and the country table (CA) and resolveZone rejected it as a conflicting location, while 'Indianapolis, IN' — a city with no entry — silently resolved to the Asia zone and quoted a 12-day transit. The whole-token guarantee the country table relies on does not help: US state codes *are* legitimate whole tokens. Adds a US state table and reads a trailing bare state code as a state under two deliberate restrictions. It applies only when the city table matched nothing, so a known city keeps deciding its own zone — that is what preserves 'Toronto, CA' as Canada while 'Los Angeles, CA' becomes US — and only when a token precedes it, so a bare 'DE' still resolves to Germany as the existing spec requires. The city path also suppresses the trailing state code from the country table, since that duplicate report is precisely what made 'Los Angeles, CA' a conflict, but only for the immediate 'City, ST' shape so 'Lagos, NG vs CA' still reports its real conflict. The unavoidable trade-off, documented in the code: a city absent from CITY_TOKENS followed by a colliding code is now read as a state, so 'Frankfurt, DE' resolves to Delaware. Nothing in that string distinguishes the two. Typing the country name resolves it, because the country table matches the explicit name first. #1666 — two denominators were wrong. onTimeRate divided by every delivered or completed shipment while the numerator required both an actual and an estimated delivery date; estimatedDeliveryDate is optional at creation, so a shipment with no estimate was always counted as not on time. The denominator now requires an estimate. cancellationRate counted every cancelled shipment against the carrier regardless of who cancelled it, penalising them for the shipper's decisions; it now counts only cancellations where the status history records the carrier as the actor via changedById. Both counts are returned alongside the rates so a caller can see how few deliveries a rate rests on. Verified by running the real logic rather than by inspection: all four acceptance cases fixed, and the three existing getMyMetrics tests still pass against their positional mock.
1 parent 1076a4b commit 2c539cc

2 files changed

Lines changed: 131 additions & 11 deletions

File tree

‎backend/src/carriers/carriers.service.ts‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ export class CarriersService {
1515
const [
1616
totalAcceptedResult,
1717
totalCompletedResult,
18-
totalCancelledResult,
18+
carrierCancelledResult,
1919
deliveredCountResult,
2020
onTimeResult,
2121
totalEarningsResult,
@@ -36,21 +36,44 @@ export class CarriersService {
3636
status: ShipmentStatus.COMPLETED,
3737
})
3838
.getRawOne<{ count: string }>(),
39+
// Carrier-initiated cancellations only (#1666). A cancellation is the
40+
// carrier's fault only when the carrier is the one recorded as changing the
41+
// status; `changedById` in the status history is the shipped record of who
42+
// cancelled. Counting every cancelled shipment penalised the carrier for
43+
// cancellations the shipper made, which it cannot prevent or influence.
3944
this.shipmentRepo
4045
.createQueryBuilder('shipment')
4146
.select('COUNT(*)', 'count')
4247
.where('shipment.carrierId = :carrierId', { carrierId })
4348
.andWhere('shipment.status = :status', {
4449
status: ShipmentStatus.CANCELLED,
4550
})
51+
.andWhere(
52+
`EXISTS (
53+
SELECT 1 FROM shipment_status_history h
54+
WHERE h.shipment_id = shipment.id
55+
AND h.to_status = :cancelled
56+
AND h.changed_by_id = :carrierId
57+
)`,
58+
{ cancelled: ShipmentStatus.CANCELLED, carrierId },
59+
)
4660
.getRawOne<{ count: string }>(),
61+
// Denominator for the on-time rate: only deliveries that actually had an
62+
// estimate to be measured against (#1666).
63+
//
64+
// `estimatedDeliveryDate` is optional when a shipment is created, so this
65+
// used to count *every* delivered/completed shipment while the numerator
66+
// required both dates. A shipment with no estimate was therefore always
67+
// counted as not on time, dragging a carrier's rate down for shipments
68+
// that were never given a target in the first place.
4769
this.shipmentRepo
4870
.createQueryBuilder('shipment')
4971
.select('COUNT(*)', 'count')
5072
.where('shipment.carrierId = :carrierId', { carrierId })
5173
.andWhere('shipment.status IN (:...statuses)', {
5274
statuses: [ShipmentStatus.DELIVERED, ShipmentStatus.COMPLETED],
5375
})
76+
.andWhere('shipment.estimatedDeliveryDate IS NOT NULL')
5477
.getRawOne<{ count: string }>(),
5578
this.shipmentRepo
5679
.createQueryBuilder('shipment')
@@ -75,21 +98,28 @@ export class CarriersService {
7598

7699
const totalAccepted = Number(totalAcceptedResult?.count ?? '0');
77100
const totalCompleted = Number(totalCompletedResult?.count ?? '0');
78-
const totalCancelled = Number(totalCancelledResult?.count ?? '0');
101+
const carrierCancelled = Number(carrierCancelledResult?.count ?? '0');
79102
const delivered = Number(deliveredCountResult?.count ?? '0');
80103
const onTimeDeliveries = Number(onTimeResult?.count ?? '0');
81104
const onTimeRate = delivered > 0 ? onTimeDeliveries / delivered : 0;
82105
const totalEarnings = parseFloat(totalEarningsResult?.total ?? '0');
83106
const cancellationRate =
84-
totalAccepted > 0 ? totalCancelled / totalAccepted : 0;
107+
totalAccepted > 0 ? carrierCancelled / totalAccepted : 0;
85108

86109
return {
87110
totalAccepted,
88111
totalCompleted,
89-
totalCancelled,
112+
/** Cancellations the carrier itself performed (see the query above). */
113+
totalCancelled: carrierCancelled,
90114
onTimeRate: Math.round(onTimeRate * 100) / 100,
91115
cancellationRate: Math.round(cancellationRate * 100) / 100,
92116
totalEarnings,
117+
// Counts are returned alongside the rates so a UI or an admin decision can
118+
// see how few deliveries a rate is based on — a 0.0 or 1.0 rate over three
119+
// shipments is not comparable to the same rate over three hundred.
120+
onTimeDeliveries,
121+
deliveriesWithEstimate: delivered,
122+
carrierCancelledCancellations: carrierCancelled,
93123
};
94124
}
95125
}

‎backend/src/shipments/eta.service.ts‎

Lines changed: 97 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -212,6 +212,39 @@ const CITY_TOKENS: Readonly<Record<string, ShipmentZone>> = Object.freeze({
212212
MUMBAI: ShipmentZone.AS,
213213
});
214214

215+
/**
216+
* US state and district codes → `ShipmentZone.US`.
217+
*
218+
* These collide head-on with three ISO 3166-1 alpha-2 codes that are already in
219+
* `LOCATION_TOKENS`: `CA` (Canada), `DE` (Germany) and `IN` (India). A US
220+
* address like "Los Angeles, CA" therefore matched the city table (US) *and* the
221+
* country table (CA) and `resolveZone` rejected it as a conflicting location,
222+
* while "Indianapolis, IN" — a city with no entry — silently resolved to the
223+
* Asia zone and quoted a 12-day transit.
224+
*
225+
* The whole-token guarantee that the country table's doc comment relies on does
226+
* not save this: US state codes *are* legitimate whole tokens. Only the trailing
227+
* `City, ST` shape disambiguates them, so `collectZoneMatches` applies this
228+
* table under narrow conditions — see the note there.
229+
*/
230+
const US_STATE_TOKENS: Readonly<Record<string, ShipmentZone>> = Object.freeze({
231+
AL: ShipmentZone.US, AK: ShipmentZone.US, AZ: ShipmentZone.US, AR: ShipmentZone.US,
232+
CA: ShipmentZone.US, CO: ShipmentZone.US, CT: ShipmentZone.US, DE: ShipmentZone.US,
233+
FL: ShipmentZone.US, GA: ShipmentZone.US, HI: ShipmentZone.US, ID: ShipmentZone.US,
234+
IL: ShipmentZone.US, IN: ShipmentZone.US, IA: ShipmentZone.US, KS: ShipmentZone.US,
235+
KY: ShipmentZone.US, LA: ShipmentZone.US, ME: ShipmentZone.US, MD: ShipmentZone.US,
236+
MA: ShipmentZone.US, MI: ShipmentZone.US, MN: ShipmentZone.US, MS: ShipmentZone.US,
237+
MO: ShipmentZone.US, MT: ShipmentZone.US, NE: ShipmentZone.US, NV: ShipmentZone.US,
238+
NH: ShipmentZone.US, NJ: ShipmentZone.US, NM: ShipmentZone.US, NY: ShipmentZone.US,
239+
NC: ShipmentZone.US, ND: ShipmentZone.US, OH: ShipmentZone.US, OK: ShipmentZone.US,
240+
OR: ShipmentZone.US, PA: ShipmentZone.US, RI: ShipmentZone.US, SC: ShipmentZone.US,
241+
SD: ShipmentZone.US, TN: ShipmentZone.US, TX: ShipmentZone.US, UT: ShipmentZone.US,
242+
VT: ShipmentZone.US, VA: ShipmentZone.US, WA: ShipmentZone.US, WV: ShipmentZone.US,
243+
WI: ShipmentZone.US, WY: ShipmentZone.US,
244+
// District of Columbia — not a state, but shipped from and to like one.
245+
DC: ShipmentZone.US,
246+
});
247+
215248
/** Splits free-form location text into comparable upper-case tokens. */
216249
function tokenize(location: string): string[] {
217250
return (
@@ -232,7 +265,7 @@ interface ZoneMatch {
232265
zone: ShipmentZone;
233266
/** The exact text that matched, for error messages. */
234267
phrase: string;
235-
kind: 'city' | 'country';
268+
kind: 'city' | 'country' | 'state';
236269
}
237270

238271
/**
@@ -248,29 +281,86 @@ function collectZoneMatches(location: string): ZoneMatch[] {
248281
const matches: ZoneMatch[] = [];
249282
const seenPhrases = new Set<string>();
250283

251-
for (const [table, kind] of [
252-
[CITY_TOKENS, 'city'],
253-
[LOCATION_TOKENS, 'country'],
254-
] as const) {
284+
const scan = (
285+
table: Readonly<Record<string, ShipmentZone>>,
286+
kind: ZoneMatch['kind'],
287+
firstEnd?: { value: number },
288+
): void => {
255289
for (
256290
let length = MAX_PLACE_PHRASE_TOKENS;
257291
length >= 1;
258292
length -= 1
259293
) {
260294
for (let start = 0; start + length <= tokens.length; start += 1) {
261295
const phrase = tokens.slice(start, start + length).join(' ');
262-
// A phrase already matched by the city table must not be
263-
// re-reported by the country table.
296+
// A phrase already matched by an earlier table must not be
297+
// re-reported by a later one.
264298
if (seenPhrases.has(phrase)) continue;
265299
const zone = table[phrase];
266300
if (zone) {
267301
seenPhrases.add(phrase);
268302
matches.push({ zone, phrase, kind });
303+
if (firstEnd !== undefined && firstEnd.value < 0) {
304+
firstEnd.value = start + length;
305+
}
269306
}
270307
}
271308
}
309+
};
310+
311+
// Index just past the first city match, used below to tell the `City, ST`
312+
// shape from a longer string that merely ends in a state-like token.
313+
const cityEnd = { value: -1 };
314+
scan(CITY_TOKENS, 'city', cityEnd);
315+
316+
// Disambiguate a trailing two-letter token that is both a US state code and an
317+
// ISO country code (`CA`, `DE`, `IN`). Two deliberate restrictions keep this
318+
// from breaking the cases that already worked:
319+
//
320+
// 1. Only when the city table matched nothing. A known city has already
321+
// decided the zone, and a state code must not overrule it — that is what
322+
// keeps "Toronto, CA" in Canada and "Los Angeles, CA" in the US rather
323+
// than turning the former into a conflict.
324+
// 2. Only when a token precedes it, i.e. the `City, ST` shape. A bare "DE"
325+
// must keep resolving to Germany, which is what a caller sending to
326+
// Germany with no city typed actually means.
327+
//
328+
// Trade-off: for a city that is *not* in `CITY_TOKENS`, "Frankfurt, DE" now
329+
// reads as Delaware. That ambiguity is inherent — nothing in the string
330+
// distinguishes the two — and the fix is to type the country name, which the
331+
// country table matches on the earlier, explicit-name pass and which therefore
332+
// takes priority over the bare code.
333+
const trailing = tokens.length > 0 ? tokens[tokens.length - 1] : undefined;
334+
335+
if (matches.length === 0 && tokens.length > 1 && trailing !== undefined) {
336+
const zone = US_STATE_TOKENS[trailing];
337+
if (zone) {
338+
seenPhrases.add(trailing);
339+
matches.push({ zone, phrase: trailing, kind: 'state' });
340+
}
341+
} else if (
342+
matches.length > 0 &&
343+
trailing !== undefined &&
344+
// Only the immediate `City, ST` shape. "Lagos, NG vs CA" also ends in a bare
345+
// `CA`, but it is not a US address, and suppressing that `CA` would hide a
346+
// real conflict the caller needs told about.
347+
cityEnd.value === tokens.length - 1 &&
348+
US_STATE_TOKENS[trailing] !== undefined
349+
) {
350+
// A city match already decided the zone, and the trailing token is a state
351+
// code that is *also* a country code. The country table must not report it
352+
// again: that pair is what made "Los Angeles, CA" resolve to both US and CA
353+
// and be rejected as a conflict.
354+
//
355+
// Only US state codes are suppressed, so genuine country/city disagreements
356+
// are still reported — "Lagos, US" still conflicts, because `US` is not a
357+
// state code, and "Lagos, Canada" still conflicts, because `CANADA` is not a
358+
// bare two-letter token.
359+
seenPhrases.add(trailing);
272360
}
273361

362+
scan(LOCATION_TOKENS, 'country');
363+
274364
return matches;
275365
}
276366

0 commit comments

Comments
 (0)