From 1b5317a8172923c0ba5cc324a80ce7e9c8462d5d Mon Sep 17 00:00:00 2001 From: Maurice Date: Mon, 27 Jul 2026 17:35:20 +0200 Subject: [PATCH] Give GPX tracks their own colour (#776) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Imported tracks all render in the same blue: the GPX/KML importers never assign a category, so category_color is null for every one of them and several walks in the same area are impossible to tell apart. Adds a nullable route_color on places, kept out of the COALESCE group in updatePlace so an explicit null can reset a track back to inheriting its category colour. The hex is validated in the Nest controller rather than in the shared contract: the update body is an open record on purpose, and tightening it would mean removing PlacesController.update from the ratchet-only body-contract allow-list. The importers now hand each new track a colour of its own, picked from the palette entries the trip is not already using. That happens in the Nest service over the injected DatabaseService — the colour assignment is new logic, and new logic does not belong in the legacy place service (see server/src/nest/README.md). The field itself is only threaded through the existing update/create statements, which the proportionality rule covers. Also fixes duplicateTrip, whose explicit column list has been silently dropping route_geometry all along, so a duplicated trip lost its tracks. --- server/src/db/migrations.ts | 8 + server/src/nest/places/places.controller.ts | 13 ++ server/src/nest/places/places.service.ts | 51 ++++- server/src/services/placeService.ts | 18 +- server/src/services/tripService.ts | 7 +- server/src/types.ts | 2 + server/tests/e2e/places.e2e.test.ts | 18 ++ .../tests/unit/nest/places.controller.test.ts | 31 +++ server/tests/unit/nest/places.service.test.ts | 189 ++++++++++++++++++ .../tests/unit/services/placeService.test.ts | 43 ++++ shared/src/index.ts | 1 + shared/src/place/place.schema.spec.ts | 25 ++- shared/src/place/place.schema.ts | 5 + shared/src/place/track-colors.ts | 32 +++ 14 files changed, 430 insertions(+), 13 deletions(-) create mode 100644 server/tests/unit/nest/places.service.test.ts create mode 100644 shared/src/place/track-colors.ts diff --git a/server/src/db/migrations.ts b/server/src/db/migrations.ts index ae1bb0f62..7719d4e2a 100644 --- a/server/src/db/migrations.ts +++ b/server/src/db/migrations.ts @@ -3822,6 +3822,14 @@ function runMigrations(db: Database.Database): void { ); `); }, + // Manual GPX track colour (#776): imported tracks all render in the same blue + // because the importer never assigns a category, so several walks in the same + // area are indistinguishable. NULL keeps the old behaviour (category colour, + // then the #3b82f6 fallback) for every existing row. + () => { + const hasRouteColor = db.prepare("SELECT 1 FROM pragma_table_info('places') WHERE name = 'route_color'").get(); + if (!hasRouteColor) db.exec('ALTER TABLE places ADD COLUMN route_color TEXT'); + }, ]; if (currentVersion < migrations.length) { diff --git a/server/src/nest/places/places.controller.ts b/server/src/nest/places/places.controller.ts index 054bc9732..19ec2bb44 100644 --- a/server/src/nest/places/places.controller.ts +++ b/server/src/nest/places/places.controller.ts @@ -16,6 +16,7 @@ import { } from '@nestjs/common'; import { FileInterceptor } from '@nestjs/platform-express'; import { memoryStorage } from 'multer'; +import { hexColorSchema } from '@trek/shared'; import { readEnv } from '../../app-config'; import type { User } from '../../types'; import { PlacesService } from './places.service'; @@ -39,6 +40,16 @@ function validateLengths(body: Record): void { } } +// A bad hex makes MapLibre fail hard when it parses the paint property, and the +// update body is an open record that Zod does not police — so check it here. +function validateRouteColor(body: Record): void { + const value = body.route_color; + if (value === undefined || value === null) return; + if (typeof value !== 'string' || !hexColorSchema.safeParse(value).success) { + throw new HttpException({ error: 'route_color must be a hex colour like #4f46e5' }, 400); + } +} + function parseBool(v: unknown, defaultVal: boolean): boolean { return v === undefined || v === null ? defaultVal : String(v) === 'true'; } @@ -93,6 +104,7 @@ export class PlacesController { ) { const trip = this.requireTrip(tripId, user); validateLengths(body); + validateRouteColor(body); this.requireEdit(trip, user); if (!body.name) { throw new HttpException({ error: 'Place name is required' }, 400); @@ -358,6 +370,7 @@ export class PlacesController { ) { const trip = this.requireTrip(tripId, user); validateLengths(body); + validateRouteColor(body); this.requireEdit(trip, user); const result = this.places.update(tripId, id, body as never, ifMatch); if (!result) { diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index e8f1fdb38..3da38ae5f 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -1,4 +1,5 @@ import { Injectable } from '@nestjs/common'; +import { TRACK_COLORS } from '@trek/shared'; import { broadcast } from '../../websocket'; import { DatabaseService } from '../database/database.service'; import { PermissionsService } from '../permissions/permissions.service'; @@ -8,6 +9,8 @@ import { onPlaceCreated, onPlaceUpdated, onPlaceDeleted } from '../../services/j type Trip = { user_id: number }; +type ImportedPlace = { id: number; route_geometry?: string | null; route_color?: string | null }; + /** * Thin Nest wrapper around the existing place service. Trip access mirrors the * requireTripAccess middleware (canAccessTrip); mutations use 'place_edit'. The @@ -66,11 +69,53 @@ export class PlacesService { buffer: Buffer, opts: { importWaypoints: boolean; importRoutes: boolean; importTracks: boolean; defaultName?: string }, ) { - return svc.importGpx(tripId, buffer, opts); + return this.colorizeImportedTracks(tripId, svc.importGpx(tripId, buffer, opts)); } - importMapFile(tripId: string, buffer: Buffer, filename: string, opts: svc.KmlImportOptions) { - return svc.importMapFile(tripId, buffer, filename, opts); + async importMapFile(tripId: string, buffer: Buffer, filename: string, opts: svc.KmlImportOptions) { + return this.colorizeImportedTracks(tripId, await svc.importMapFile(tripId, buffer, filename, opts)); + } + + /** + * Hand every freshly imported track its own colour (#776). + * + * The importers never assign a category, so without this every track in a + * trip renders in the same #3b82f6 — which is the actual complaint behind + * the request: several walks in one area are impossible to tell apart. + * + * Picks the palette entries the trip is not already using rather than + * counting rows: counting collides as soon as somebody recolours a track by + * hand or deletes one, which is exactly when distinguishability matters. + * Once all ten are taken it wraps around — ten walks in one trip is already + * past what a colour alone can separate. + * + * Only rows that carry geometry are touched, and only ones that have no + * colour yet; plain waypoints and existing places stay untouched. + */ + private colorizeImportedTracks(tripId: string, result: T): T { + const tracks = result?.places?.filter((p) => p.route_geometry && !p.route_color) ?? []; + if (tracks.length === 0) return result; + + // Read and write in one transaction so two concurrent imports cannot both + // read the same set of free colours. + this.dbs.transaction((conn) => { + const taken = new Set( + (conn + .prepare('SELECT DISTINCT route_color AS c FROM places WHERE trip_id = ? AND route_color IS NOT NULL') + .all(tripId) as { c: string }[]).map((r) => r.c), + ); + const free = TRACK_COLORS.filter((c) => !taken.has(c)); + const stmt = conn.prepare('UPDATE places SET route_color = ? WHERE id = ?'); + tracks.forEach((track, i) => { + // Free ones first, then wrap through the whole palette — never reuse a + // free colour twice within the same import. + const color = i < free.length ? free[i] : TRACK_COLORS[(i - free.length) % TRACK_COLORS.length]; + stmt.run(color, track.id); + track.route_color = color; + }); + }); + + return result; } importGoogleList(tripId: string, url: string, opts?: Parameters[2]) { diff --git a/server/src/services/placeService.ts b/server/src/services/placeService.ts index 596e44783..2732d4f13 100644 --- a/server/src/services/placeService.ts +++ b/server/src/services/placeService.ts @@ -125,26 +125,28 @@ export function createPlace( place_time?: string; end_time?: string; duration_minutes?: number; notes?: string; image_url?: string; google_place_id?: string; google_ftid?: string; osm_id?: string; website?: string; phone?: string; - transport_mode?: string; tags?: number[]; + transport_mode?: string; route_geometry?: string; route_color?: string; tags?: number[]; }, ) { const { name, description, lat, lng, address, category_id, price, currency, place_time, end_time, duration_minutes, notes, image_url, google_place_id, google_ftid, osm_id, website, phone, - transport_mode, tags = [], + transport_mode, route_geometry, route_color, tags = [], } = body; const result = db.prepare(` INSERT INTO places (trip_id, name, description, lat, lng, address, category_id, price, currency, place_time, end_time, - duration_minutes, notes, image_url, google_place_id, google_ftid, osm_id, website, phone, transport_mode) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + duration_minutes, notes, image_url, google_place_id, google_ftid, osm_id, website, phone, transport_mode, + route_geometry, route_color) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `).run( tripId, name, description || null, lat || null, lng || null, address || null, category_id || null, price || null, currency || null, place_time || null, end_time || null, duration_minutes || 60, notes || null, image_url || null, google_place_id || null, google_ftid || null, osm_id || null, website || null, phone || null, transport_mode || 'walking', + route_geometry || null, route_color || null, ); const placeId = result.lastInsertRowid; @@ -182,7 +184,7 @@ export function updatePlace( place_time?: string; end_time?: string; duration_minutes?: number; notes?: string; image_url?: string; google_place_id?: string; google_ftid?: string; osm_id?: string; website?: string; phone?: string; - transport_mode?: string; tags?: number[]; + transport_mode?: string; route_color?: string | null; tags?: number[]; }, ifMatch?: string, ): ReturnType | UpdateConflict | null { @@ -200,7 +202,7 @@ export function updatePlace( name, description, lat, lng, address, category_id, price, currency, place_time, end_time, duration_minutes, notes, image_url, google_place_id, google_ftid, osm_id, website, phone, - transport_mode, tags, + transport_mode, route_color, tags, } = body; db.prepare(` @@ -224,6 +226,7 @@ export function updatePlace( website = ?, phone = ?, transport_mode = COALESCE(?, transport_mode), + route_color = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ? `).run( @@ -246,6 +249,9 @@ export function updatePlace( website !== undefined ? website : existingPlace.website, phone !== undefined ? phone : existingPlace.phone, transport_mode || null, + // Deliberately not COALESCE: an explicit null is how the picker resets a + // track back to its category colour (#776). + route_color !== undefined ? route_color : existingPlace.route_color, placeId, ); diff --git a/server/src/services/tripService.ts b/server/src/services/tripService.ts index 23e5dba5e..f5bc99d28 100644 --- a/server/src/services/tripService.ts +++ b/server/src/services/tripService.ts @@ -917,14 +917,15 @@ export function copyTripById(sourceTripId: string | number, newOwnerId: number, const insertPlace = db.prepare(` INSERT INTO places (trip_id, name, description, lat, lng, address, category_id, price, currency, reservation_status, reservation_notes, reservation_datetime, place_time, end_time, - duration_minutes, notes, image_url, google_place_id, google_ftid, website, phone, transport_mode, osm_id) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + duration_minutes, notes, image_url, google_place_id, google_ftid, website, phone, transport_mode, osm_id, + route_geometry, route_color) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `); for (const p of oldPlaces) { const r = insertPlace.run(newTripId, p.name, p.description, p.lat, p.lng, p.address, p.category_id, p.price, p.currency, p.reservation_status, p.reservation_notes, p.reservation_datetime, p.place_time, p.end_time, p.duration_minutes, p.notes, p.image_url, p.google_place_id, - p.google_ftid, p.website, p.phone, p.transport_mode, p.osm_id); + p.google_ftid, p.website, p.phone, p.transport_mode, p.osm_id, p.route_geometry, p.route_color); placeMap.set(p.id, r.lastInsertRowid); } diff --git a/server/src/types.ts b/server/src/types.ts index 8f30ceec4..d5adc6549 100644 --- a/server/src/types.ts +++ b/server/src/types.ts @@ -73,6 +73,8 @@ export interface Place { google_place_id?: string | null; google_ftid?: string | null; osm_id?: string | null; + route_geometry?: string | null; + route_color?: string | null; website?: string | null; phone?: string | null; transport_mode?: string; diff --git a/server/tests/e2e/places.e2e.test.ts b/server/tests/e2e/places.e2e.test.ts index f6a6fe18c..89199cc95 100644 --- a/server/tests/e2e/places.e2e.test.ts +++ b/server/tests/e2e/places.e2e.test.ts @@ -114,6 +114,24 @@ describe('Places e2e (real auth guard + temp SQLite)', () => { expect(bad.body).toEqual({ error: 'ids must be an array of numbers' }); }); + it('PUT route_color: hex through, null through, garbage rejected (#776)', async () => { + pl.updatePlace.mockReturnValue({ id: 9, route_color: '#e11d48' }); + const ok = await request(server).put('/api/trips/5/places/9').set('Cookie', sessionCookie(1)).send({ route_color: '#e11d48' }); + expect(ok.status).toBe(200); + expect(ok.body).toEqual({ place: { id: 9, route_color: '#e11d48' } }); + expect(pl.updatePlace).toHaveBeenCalledWith('5', '9', expect.objectContaining({ route_color: '#e11d48' }), undefined); + + // null is the reset back to the inherited category colour. + pl.updatePlace.mockReturnValue({ id: 9, route_color: null }); + const cleared = await request(server).put('/api/trips/5/places/9').set('Cookie', sessionCookie(1)).send({ route_color: null }); + expect(cleared.status).toBe(200); + expect(pl.updatePlace).toHaveBeenLastCalledWith('5', '9', expect.objectContaining({ route_color: null }), undefined); + + const bad = await request(server).put('/api/trips/5/places/9').set('Cookie', sessionCookie(1)).send({ route_color: 'red' }); + expect(bad.status).toBe(400); + expect(bad.body).toEqual({ error: 'route_color must be a hex colour like #4f46e5' }); + }); + it('404 trip when not accessible', async () => { canAccessTrip.mockReturnValue(undefined); const res = await request(server).get('/api/trips/5/places').set('Cookie', sessionCookie(1)); diff --git a/server/tests/unit/nest/places.controller.test.ts b/server/tests/unit/nest/places.controller.test.ts index 163057e10..32a6e23d4 100644 --- a/server/tests/unit/nest/places.controller.test.ts +++ b/server/tests/unit/nest/places.controller.test.ts @@ -236,6 +236,37 @@ describe('PlacesController (parity with the legacy /api/trips/:tripId/places rou expect(onUpdated).toHaveBeenCalledWith(9); }); + describe('route_color (#776)', () => { + it('400s on anything that is not a hex colour, before the permission check', () => { + const canEdit = vi.fn().mockReturnValue(false); // would 403 if it got that far + const err = { status: 400, body: { error: 'route_color must be a hex colour like #4f46e5' } }; + expect(thrown(() => new PlacesController(svc({ canEdit })).update(user, '5', '9', { route_color: 'red' }))).toEqual(err); + expect(thrown(() => new PlacesController(svc({ canEdit })).update(user, '5', '9', { route_color: '#12345' }))).toEqual(err); + expect(thrown(() => new PlacesController(svc({ canEdit })).update(user, '5', '9', { route_color: 123 }))).toEqual(err); + expect(thrown(() => new PlacesController(svc({ canEdit })).create(user, '5', { name: 'T', route_color: 'red' }))).toEqual(err); + expect(canEdit).not.toHaveBeenCalled(); + }); + + it('passes a valid hex through, and null through as the reset to auto', () => { + const update = vi.fn().mockReturnValue({ id: 9, route_color: '#e11d48' }); + const broadcast = vi.fn(); + const s = svc({ update, broadcast } as Partial); + expect(new PlacesController(s).update(user, '5', '9', { route_color: '#e11d48' }, 'sock')) + .toEqual({ place: { id: 9, route_color: '#e11d48' } }); + expect(update).toHaveBeenCalledWith('5', '9', expect.objectContaining({ route_color: '#e11d48' }), undefined); + // The colour has to reach the other members too, not just the DB. + expect(broadcast).toHaveBeenCalledWith('5', 'place:updated', { place: { id: 9, route_color: '#e11d48' } }, 'sock'); + + new PlacesController(svc({ update } as Partial)).update(user, '5', '9', { route_color: null }); + expect(update).toHaveBeenLastCalledWith('5', '9', expect.objectContaining({ route_color: null }), undefined); + }); + + it('accepts the short #abc form', () => { + const update = vi.fn().mockReturnValue({ id: 9 }); + expect(new PlacesController(svc({ update } as Partial)).update(user, '5', '9', { route_color: '#abc' })).toEqual({ place: { id: 9 } }); + }); + }); + it('PUT /:id forwards the base-version token and 409s on a conflict (#1135)', () => { const update = vi.fn().mockReturnValue({ conflict: true, server: { id: 9, name: 'Theirs' } }); const onUpdated = vi.fn(); const broadcast = vi.fn(); diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts new file mode 100644 index 000000000..f47d4eb26 --- /dev/null +++ b/server/tests/unit/nest/places.service.test.ts @@ -0,0 +1,189 @@ +/** + * Unit tests for PlacesService — PLACES-SVC-001 through PLACES-SVC-005. + * + * Covers the one piece of logic the Nest wrapper owns rather than delegates: + * handing every freshly imported track its own colour (#776). The importers + * never assign a category, so without this each track in a trip renders in the + * same blue and several walks in one area are impossible to tell apart. + * + * Uses a real in-memory SQLite DB so the SQL is exercised faithfully; the + * service is constructed directly, no Nest container needed. + */ +import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; +import { TRACK_COLORS } from '@trek/shared'; + +// ── DB setup ────────────────────────────────────────────────────────────────── + +const { testDb, dbMock } = vi.hoisted(() => { + const Database = require('better-sqlite3'); + const db = new Database(':memory:'); + db.exec('PRAGMA journal_mode = WAL'); + db.exec('PRAGMA foreign_keys = ON'); + db.exec('PRAGMA busy_timeout = 5000'); + const mock = { + db, + closeDb: () => {}, + reinitialize: () => {}, + getPlaceWithTags: (placeId: any) => db.prepare('SELECT * FROM places WHERE id = ?').get(placeId) ?? null, + canAccessTrip: () => null, + isOwner: () => false, + }; + return { testDb: db, dbMock: mock }; +}); + +vi.mock('../../../src/db/database', () => dbMock); +vi.mock('../../../src/config', () => ({ + JWT_SECRET: 'test-secret', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, +})); + +import { createTables } from '../../../src/db/schema'; +import { runMigrations } from '../../../src/db/migrations'; +import { resetTestDb } from '../../helpers/test-db'; +import { createUser, createTrip } from '../../helpers/factories'; +import { DatabaseService } from '../../../src/nest/database/database.service'; +import { PlacesService } from '../../../src/nest/places/places.service'; + +// Three tracks plus a plain waypoint — the shared fixture only has waypoints, +// and the whole point here is what happens to geometry. +const GPX_WITH_TRACKS = ` + + Trailhead + Morning walk + + + Afternoon walk + + + Evening walk + + +`; + +const svc = new PlacesService(new DatabaseService(testDb)); + +function importFixture(tripId: number) { + return svc.importGpx(String(tripId), Buffer.from(GPX_WITH_TRACKS), { + importWaypoints: true, importRoutes: true, importTracks: true, + }); +} + +function tracksOf(tripId: number) { + return testDb.prepare('SELECT id, route_color FROM places WHERE trip_id = ? AND route_geometry IS NOT NULL ORDER BY id') + .all(tripId) as { id: number; route_color: string | null }[]; +} + +beforeAll(() => { + createTables(testDb); + runMigrations(testDb); +}); + +beforeEach(() => { + resetTestDb(testDb); +}); + +afterAll(() => { + testDb.close(); +}); + +describe('PlacesService — automatic track colours (#776)', () => { + it('PLACES-SVC-001 — every imported track gets a colour from the shared palette', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + importFixture(trip.id); + + const tracks = tracksOf(trip.id); + expect(tracks.length).toBeGreaterThan(0); + for (const track of tracks) { + expect(TRACK_COLORS).toContain(track.route_color); + } + }); + + it('PLACES-SVC-002 — the returned places carry the colour, not just the DB rows', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const result = importFixture(trip.id) as { places: any[] } | null; + + const returnedTracks = (result?.places ?? []).filter(p => p.route_geometry); + expect(returnedTracks.length).toBeGreaterThan(0); + for (const track of returnedTracks) { + expect(TRACK_COLORS).toContain(track.route_color); + } + }); + + it('PLACES-SVC-003 — plain waypoints keep no colour at all', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + importFixture(trip.id); + + const waypoints = testDb.prepare( + 'SELECT route_color FROM places WHERE trip_id = ? AND route_geometry IS NULL', + ).all(trip.id) as { route_color: string | null }[]; + for (const wp of waypoints) { + expect(wp.route_color).toBeNull(); + } + }); + + it('PLACES-SVC-004 — a second import continues the palette instead of repeating it', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + importFixture(trip.id); + const first = tracksOf(trip.id).map(t => t.route_color); + + // Same fixture again: dedup skips the identical rows, so seed a distinct + // track directly and let the service colour the next import round. + testDb.prepare( + "INSERT INTO places (trip_id, name, lat, lng, route_geometry) VALUES (?, 'Second walk', 1, 1, '[[1,1],[2,2]]')", + ).run(trip.id); + const seeded = testDb.prepare('SELECT id FROM places WHERE name = ?').get('Second walk') as { id: number }; + (svc as any).colorizeImportedTracks(String(trip.id), { + places: [{ id: seeded.id, route_geometry: '[[1,1],[2,2]]', route_color: null }], + }); + + const seededColor = (testDb.prepare('SELECT route_color FROM places WHERE id = ?').get(seeded.id) as any).route_color; + expect(TRACK_COLORS).toContain(seededColor); + expect(first).not.toContain(seededColor); + }); + + it('PLACES-SVC-006 — a colour already in use by hand is not handed out again', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + // Someone recoloured an existing track to the palette's second entry. A + // plain row count would hand exactly that colour to the next import. + testDb.prepare( + "INSERT INTO places (trip_id, name, lat, lng, route_geometry, route_color) VALUES (?, 'Old walk', 1, 1, '[[1,1],[2,2]]', ?)", + ).run(trip.id, TRACK_COLORS[1]); + + importFixture(trip.id); + + const colors = tracksOf(trip.id).map(t => t.route_color); + expect(new Set(colors).size).toBe(colors.length); + }); + + it('PLACES-SVC-007 — gaps left by deleted tracks are reused, not skipped past', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + importFixture(trip.id); + // Drop the first track; its colour becomes free again. + const first = tracksOf(trip.id)[0]; + testDb.prepare('DELETE FROM places WHERE id = ?').run(first.id); + + const seeded = testDb.prepare( + "INSERT INTO places (trip_id, name, lat, lng, route_geometry) VALUES (?, 'Later walk', 9, 9, '[[9,9],[8,8]]') RETURNING id", + ).get(trip.id) as { id: number }; + (svc as any).colorizeImportedTracks(String(trip.id), { + places: [{ id: seeded.id, route_geometry: '[[9,9],[8,8]]', route_color: null }], + }); + + const colors = tracksOf(trip.id).map(t => t.route_color); + expect(new Set(colors).size).toBe(colors.length); + }); + + it('PLACES-SVC-005 — an import that yields nothing is passed straight through', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + expect((svc as any).colorizeImportedTracks(String(trip.id), null)).toBeNull(); + expect((svc as any).colorizeImportedTracks(String(trip.id), { places: [] })).toEqual({ places: [] }); + }); +}); diff --git a/server/tests/unit/services/placeService.test.ts b/server/tests/unit/services/placeService.test.ts index 845ed4aa3..b40274a3d 100644 --- a/server/tests/unit/services/placeService.test.ts +++ b/server/tests/unit/services/placeService.test.ts @@ -232,6 +232,49 @@ describe('updatePlace', () => { const updated = updatePlace(String(trip.id), String(place.id), { tags: [] }) as any; expect(updated.tags).toHaveLength(0); }); + + // ── Track colour (#776) ───────────────────────────────────────────────────── + + it('PLACE-SVC-052 — stores a picked route_color', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Walk' }) as any; + const updated = updatePlace(String(trip.id), String(place.id), { route_color: '#e11d48' }) as any; + expect(updated.route_color).toBe('#e11d48'); + }); + + it('PLACE-SVC-053 — an explicit null clears it again (the reset to auto)', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Walk' }) as any; + updatePlace(String(trip.id), String(place.id), { route_color: '#e11d48' }); + // Guards the COALESCE trap: name/currency/transport_mode can never be + // emptied, and route_color built that way would be a one-way door. + const cleared = updatePlace(String(trip.id), String(place.id), { route_color: null }) as any; + expect(cleared.route_color).toBeNull(); + }); + + it('PLACE-SVC-054 — an unrelated update leaves the colour alone', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const place = createPlace(testDb, trip.id, { name: 'Walk' }) as any; + updatePlace(String(trip.id), String(place.id), { route_color: '#059669' }); + const renamed = updatePlace(String(trip.id), String(place.id), { name: 'Hike' }) as any; + expect(renamed.name).toBe('Hike'); + expect(renamed.route_color).toBe('#059669'); + }); + + it('PLACE-SVC-055 — createPlace carries geometry and colour instead of dropping them', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const created = svcCreatePlace(String(trip.id), { + name: 'Restored track', + route_geometry: '[[48.0,2.0],[49.0,3.0]]', + route_color: '#7c3aed', + }) as any; + expect(created.route_geometry).toBe('[[48.0,2.0],[49.0,3.0]]'); + expect(created.route_color).toBe('#7c3aed'); + }); }); // ── updatePlacesMany ──────────────────────────────────────────────────────────── diff --git a/shared/src/index.ts b/shared/src/index.ts index 3b51cfb12..53a7f346a 100644 --- a/shared/src/index.ts +++ b/shared/src/index.ts @@ -31,6 +31,7 @@ export * from './airtrail/airtrail.schema'; export * from './day/day.schema'; export * from './assignment/assignment.schema'; export * from './place/place.schema'; +export * from './place/track-colors'; export * from './collection/collection.schema'; export * from './trip/trip.schema'; export * from './trip-invite/trip-invite.schema'; diff --git a/shared/src/place/place.schema.spec.ts b/shared/src/place/place.schema.spec.ts index 82208ceb2..7338992ca 100644 --- a/shared/src/place/place.schema.spec.ts +++ b/shared/src/place/place.schema.spec.ts @@ -1,7 +1,30 @@ -import { placeCreateRequestSchema, placeBulkDeleteRequestSchema, placeImportListRequestSchema } from './place.schema'; +import { + placeCreateRequestSchema, + placeBulkDeleteRequestSchema, + placeImportListRequestSchema, + placeSchema, +} from './place.schema'; import { describe, it, expect } from 'vitest'; +describe('placeSchema route_color (#776)', () => { + const place = { id: 1, trip_id: 1, name: 'Walk' }; + + it('takes a hex colour, null, or nothing at all', () => { + expect(placeSchema.safeParse({ ...place, route_color: '#e11d48' }).success).toBe(true); + expect(placeSchema.safeParse({ ...place, route_color: '#abc' }).success).toBe(true); + // null is how a track goes back to inheriting its category colour. + expect(placeSchema.safeParse({ ...place, route_color: null }).success).toBe(true); + expect(placeSchema.safeParse(place).success).toBe(true); + }); + + it('rejects anything a map renderer could not parse', () => { + expect(placeSchema.safeParse({ ...place, route_color: 'blue' }).success).toBe(false); + expect(placeSchema.safeParse({ ...place, route_color: '#12345' }).success).toBe(false); + expect(placeSchema.safeParse({ ...place, route_color: 'e11d48' }).success).toBe(false); + }); +}); + describe('placeCreateRequestSchema', () => { it('requires a name and keeps the other place fields open', () => { expect( diff --git a/shared/src/place/place.schema.ts b/shared/src/place/place.schema.ts index d9d485911..e0918fd94 100644 --- a/shared/src/place/place.schema.ts +++ b/shared/src/place/place.schema.ts @@ -16,6 +16,9 @@ import { z } from 'zod'; const open = z.record(z.string(), z.unknown()); +/** `#rgb` / `#rrggbb`, the form both map renderers and CSS accept. */ +export const hexColorSchema = z.string().regex(/^#([0-9a-fA-F]{3}|[0-9a-fA-F]{6})$/); + /** * Embedded category as returned on a place — a trimmed projection of the * categories row (id/name/color/icon), built inline by placeService and @@ -70,6 +73,8 @@ export const placeSchema = z.object({ google_ftid: z.string().nullable().optional(), osm_id: z.string().nullable().optional(), route_geometry: z.string().nullable().optional(), + // Manual track colour (#776). null = inherit the category colour like before. + route_color: hexColorSchema.nullable().optional(), website: z.string().nullable().optional(), phone: z.string().nullable().optional(), transport_mode: z.string().nullable().optional(), diff --git a/shared/src/place/track-colors.ts b/shared/src/place/track-colors.ts new file mode 100644 index 000000000..786a9fe11 --- /dev/null +++ b/shared/src/place/track-colors.ts @@ -0,0 +1,32 @@ +/** + * Track colour palette (#776) — shared so the importer's automatic assignment + * and the picker in the UI hand out the exact same set. + * + * Tuned against map tiles rather than UI surfaces: the 600 tier survives thin + * 3.5px lines on light basemaps where 400/500 washes out, and nothing here is + * a neutral grey (invisible on CartoDB Dark) or a pale pastel (invisible at + * any size). Blue is deliberately blue-700 — darker than both the #3b82f6 GPX + * fallback and the #0a84ff day route, so a blue track reads as chosen rather + * than defaulted. + * + * The order matters: the importer assigns these round-robin, so the first five + * are the ones most tracks get. They follow the Okabe-Ito idea of staying + * separable under red-green colour blindness (blue, orange, green, magenta, + * cyan) instead of running through the rainbow and handing out four adjacent + * warm tones in a row. + */ +export const TRACK_COLORS = [ + '#1d4ed8', // blue-700 + '#ea580c', // orange-600 + '#059669', // emerald-600 + '#c026d3', // fuchsia-600 + '#0891b2', // cyan-600 + '#e11d48', // rose-600 + '#d97706', // amber-600 + '#65a30d', // lime-600 + '#7c3aed', // violet-600 + '#4f46e5', // indigo-600 +] as const; + +/** Colour a track falls back to when it has neither a manual nor a category colour. */ +export const TRACK_COLOR_FALLBACK = '#3b82f6';