From 0ce82b811be8725da082a4090be430c962f744b9 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 20 Aug 2026 18:00:55 +0200 Subject: [PATCH] fix(atlas): count only the flights a person is actually on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 4.0 lets a booking name the people it is for (#1517), and the travel figures never read that. They scoped reservations by trip membership, so on a shared trip everyone was credited with everyone's flights: two people meeting in New York, one from Rome and one from Mexico City, each got both departures, and each had "visited" the other's country. The rule now asks whether the reservation belongs to the person asking, in one fragment used by all three queries that derive figures from reservations — flown distance, the passport card's countries, and Atlas's own endpoint scan — so there is one answer to that question rather than three. Its second half is the important one: a booking with nobody named on it counts for everyone on the trip. Every reservation made before 4.0 looks exactly like that, and a plain join would have taken the flown distance of every upgrading install to zero. With no assignments anywhere the numbers are identical to before. Where someone HAS assigned travelers, their figures will go down after this, because they were counting journeys they did not make. That is the point, but it is worth a release note. Places are untouched: they have no per-traveler concept, and this is only about numbers derived from bookings. Closes #1966 --- server/src/nest/atlas/atlas.service.ts | 28 ++++- .../unit/nest/travel-stats.service.test.ts | 108 +++++++++++++++++- 2 files changed, 132 insertions(+), 4 deletions(-) diff --git a/server/src/nest/atlas/atlas.service.ts b/server/src/nest/atlas/atlas.service.ts index 0bbfd2acf..b966b9a0d 100644 --- a/server/src/nest/atlas/atlas.service.ts +++ b/server/src/nest/atlas/atlas.service.ts @@ -337,9 +337,10 @@ export class AtlasService { FROM reservation_endpoints e JOIN reservations r ON e.reservation_id = r.id WHERE r.trip_id IN (${tripIds.map(() => '?').join(',')}) AND e.role IN ('from', 'to') + AND ${AtlasService.TRAVELER_OWNS} `, ) - .all(...tripIds) as EndpointRow[]; + .all(...tripIds, userId) as EndpointRow[]; // A layover the traveler never left is only marked 'stop' while both its legs sit in // one booking. Split over two bookings it is a plain 'to' plus 'from' at the same @@ -1004,7 +1005,8 @@ export class AtlasService { WHERE (t.user_id = ? OR tm.user_id = ?) AND e.role IN ('from', 'to') AND COALESCE(t.start_date, t.end_date) IS NOT NULL AND COALESCE(t.start_date, t.end_date) <= date('now') - `, userId, userId); + AND ${AtlasService.TRAVELER_OWNS} + `, userId, userId, userId); const transfers = transferEndpointIds( endpointRows.filter(e => e.reservation_type === 'flight' && e.reservation_status !== 'cancelled') ); @@ -1043,6 +1045,25 @@ export class AtlasService { * between consecutive points are added up so multi-stop flights count * correctly. */ + /** + * Whether a reservation counts toward one person's own travel figures. + * + * It does when they are named on it (#1517 assigns members and guests to + * bookings), and it does when nobody is named at all — an unassigned booking + * still belongs to the whole trip, which is what every reservation made + * before 4.0 looks like. + * + * That second half is what keeps this from being a breaking change: the + * assignment table is empty on any install upgrading from 3.4.1, and a plain + * join would have taken those users' flown distance to zero overnight. With + * no assignments anywhere the result is identical to before (#1966). + * + * Binds ONE parameter, the user id, and expects the reservation aliased `r`. + */ + private static readonly TRAVELER_OWNS = ` + (NOT EXISTS (SELECT 1 FROM reservation_travelers rt WHERE rt.reservation_id = r.id) + OR EXISTS (SELECT 1 FROM reservation_travelers rt WHERE rt.reservation_id = r.id AND rt.user_id = ?))`; + private flightDistanceKm(userId: number): number { const rows = this.db.all<{ reservation_id: number; lat: number; lng: number }>(` SELECT re.reservation_id, re.lat, re.lng @@ -1053,8 +1074,9 @@ export class AtlasService { WHERE (t.user_id = ? OR tm.user_id IS NOT NULL) AND r.type = 'flight' AND r.status != 'cancelled' + AND ${AtlasService.TRAVELER_OWNS} ORDER BY re.reservation_id, re.sequence - `, userId, userId); + `, userId, userId, userId); let total = 0; let prev: { id: number; lat: number; lng: number } | null = null; diff --git a/server/tests/unit/nest/travel-stats.service.test.ts b/server/tests/unit/nest/travel-stats.service.test.ts index d24d9289d..4f62e5859 100644 --- a/server/tests/unit/nest/travel-stats.service.test.ts +++ b/server/tests/unit/nest/travel-stats.service.test.ts @@ -27,7 +27,7 @@ import { describe, it, expect, beforeAll, beforeEach, afterAll, vi } from 'vites import { createTables } from '../../../src/db/schema'; import { runMigrations } from '../../../src/db/migrations'; import { resetTestDb } from '../../helpers/test-db'; -import { createUser, createTrip, createPlace, createReservation } from '../../helpers/factories'; +import { createUser, createTrip, createPlace, createReservation, addTripMember } from '../../helpers/factories'; import { AtlasService } from '../../../src/nest/atlas/atlas.service'; import { DatabaseService } from '../../../src/nest/database/database.service'; @@ -226,3 +226,109 @@ describe('travel-stats quirk fixes', () => { expect(stats.coords).toContainEqual({ lat: 0, lng: 0 }); }); }); + +/** + * Whose trip it is, and whose flight (#1966). + * + * 4.0 lets a booking name the people it is for (#1517), and the figures never + * read that: they scoped by trip membership, so on a shared trip everyone was + * credited with everyone's flights. Two people flying to the same place from + * different cities each got both departures, and the countries each of them had + * "visited" included the other's. + * + * The rule has a second half that matters more than the first: a booking with + * nobody named on it still counts for everyone. Every reservation made before + * 4.0 looks exactly like that, so without it this would have taken the flown + * distance of every existing install to zero. + */ +describe('personal figures on a shared trip (#1966)', () => { + const PAST_START = '2023-05-01'; + const PAST_END = '2023-05-10'; + + const endpoint = (reservationId: number, role: 'from' | 'to', sequence: number, lat: number, lng: number, code: string) => + testDb.prepare( + 'INSERT INTO reservation_endpoints (reservation_id, role, sequence, name, lat, lng, code) VALUES (?, ?, ?, ?, ?, ?, ?)' + ).run(reservationId, role, sequence, `Endpoint ${sequence}`, lat, lng, code); + + const assignTo = (reservationId: number, userId: number) => + testDb.prepare('INSERT INTO reservation_travelers (reservation_id, user_id) VALUES (?, ?)').run(reservationId, userId); + + /** A shared trip: A flies Rome→New York, B flies Mexico City→New York. */ + function sharedTrip() { + const { user: alice } = createUser(testDb); + const { user: bob } = createUser(testDb); + const trip = createTrip(testDb, alice.id, { title: 'Shared', start_date: PAST_START, end_date: PAST_END }); + addTripMember(testDb, trip.id, bob.id); + + const fromRome = createReservation(testDb, trip.id, { type: 'flight', title: 'FCO-JFK' }); + endpoint(fromRome.id, 'from', 0, 41.8003, 12.2389, 'FCO'); + endpoint(fromRome.id, 'to', 1, 40.6413, -73.7781, 'JFK'); + + const fromMexico = createReservation(testDb, trip.id, { type: 'flight', title: 'MEX-JFK' }); + endpoint(fromMexico.id, 'from', 0, 19.4363, -99.0721, 'MEX'); + endpoint(fromMexico.id, 'to', 1, 40.6413, -73.7781, 'JFK'); + + return { alice, bob, trip, fromRome, fromMexico }; + } + + it('credits each traveler with their own flight only', () => { + const { alice, bob, fromRome, fromMexico } = sharedTrip(); + assignTo(fromRome.id, alice.id); + assignTo(fromMexico.id, bob.id); + + const a = atlas.getTravelStats(alice.id); + const b = atlas.getTravelStats(bob.id); + + // Rome to New York is a good deal further than Mexico City to New York, so + // equal distances would mean both are still counting both flights. + expect(a.totalDistanceKm).toBeGreaterThan(0); + expect(b.totalDistanceKm).toBeGreaterThan(0); + expect(a.totalDistanceKm).not.toBe(b.totalDistanceKm); + }); + + it('does not stamp another traveler s departure country on your passport', () => { + const { alice, bob, fromRome, fromMexico } = sharedTrip(); + assignTo(fromRome.id, alice.id); + assignTo(fromMexico.id, bob.id); + + // Alice never went through Mexico. + expect(atlas.getTravelStats(alice.id).countries).not.toContain('MX'); + expect(atlas.getTravelStats(bob.id).countries).not.toContain('IT'); + }); + + /* + * The case that keeps this from being a breaking change. The assignment table + * is empty on every install upgrading from 3.4.1. + */ + it('still counts a booking nobody is named on, for everyone on the trip', () => { + const { alice, bob } = sharedTrip(); + + const a = atlas.getTravelStats(alice.id); + const b = atlas.getTravelStats(bob.id); + expect(a.totalDistanceKm).toBeGreaterThan(0); + expect(a.totalDistanceKm).toBe(b.totalDistanceKm); + }); + + it('counts a booking they are both named on, once for each of them', () => { + const { alice, bob, fromRome, fromMexico } = sharedTrip(); + assignTo(fromRome.id, alice.id); + assignTo(fromRome.id, bob.id); + assignTo(fromMexico.id, alice.id); + assignTo(fromMexico.id, bob.id); + + const a = atlas.getTravelStats(alice.id); + const b = atlas.getTravelStats(bob.id); + expect(a.totalDistanceKm).toBe(b.totalDistanceKm); + expect(a.countries).toEqual(b.countries); + }); + + it('leaves a traveler assigned nothing with the unassigned bookings only', () => { + const { alice, bob, fromRome } = sharedTrip(); + assignTo(fromRome.id, bob.id); + + // Alice keeps the Mexico flight (nobody named) but loses the Rome one. + const a = atlas.getTravelStats(alice.id); + expect(a.countries).not.toContain('IT'); + expect(a.totalDistanceKm).toBeGreaterThan(0); + }); +});