From a8fc93dfdef56aadf4f81150fa1e7de18a562b1b Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 7 Oct 2026 16:16:26 +0200 Subject: [PATCH] fix(notifications): take today from the instance's TZ and catch up missed trip reminders The reminder crons run at 09:00 in the instance's TZ but computed today in UTC, so east of Greenwich a trip reminder arrived a day late. A day the job did not run also lost that day's trip reminders for good: the match was 'starts exactly N days from today'. Trips now record the start date a reminder went out for, and the job sends every reminder that is due and not yet sent. The active-trip redirect and the upcoming reservations use the instance's date and clock as well. --- server/src/db/entities/Trips.entity.ts | 2 + ...0_a_trip_reminder_remembers_it_was_sent.ts | 25 +++++++++++ .../src/db/repositories/Trips.repository.ts | 10 ++++- server/src/nest/common/timezoneService.ts | 12 +++++ .../notifications/reminder-jobs.service.ts | 45 ++++++++----------- .../nest/reservations/reservations.service.ts | 8 ++-- server/src/nest/trips/trips.service.ts | 3 +- server/tests/unit/db/legacy-baseline.test.ts | 1 + server/tests/unit/nest/app-clock.test.ts | 22 +++++++++ .../unit/nest/reminder-jobs.service.test.ts | 22 +++++++++ 10 files changed, 119 insertions(+), 31 deletions(-) create mode 100644 server/src/db/migrations/Migration20200101042100_a_trip_reminder_remembers_it_was_sent.ts create mode 100644 server/tests/unit/nest/app-clock.test.ts diff --git a/server/src/db/entities/Trips.entity.ts b/server/src/db/entities/Trips.entity.ts index 903e0931a..6f6b6099e 100644 --- a/server/src/db/entities/Trips.entity.ts +++ b/server/src/db/entities/Trips.entity.ts @@ -52,6 +52,7 @@ export class Trips { feed_token?: string | null; created_at?: string | null; updated_at?: string | null; + reminder_sent_for?: string | null; budget_category_order_collection = new Collection(this); budget_items_collection = new Collection(this); budget_settlements_collection = new Collection(this); @@ -112,6 +113,7 @@ export const TripsSchema = defineEntity({ feed_token: p.text().nullable(), created_at: p.type(DbTimestampType).nullable().defaultRaw(`CURRENT_TIMESTAMP`).index('idx_trips_created_at'), updated_at: p.type(DbTimestampType).nullable().defaultRaw(`CURRENT_TIMESTAMP`), + reminder_sent_for: p.text().nullable(), budget_category_order_collection: () => p.oneToMany(BudgetCategoryOrder).mappedBy('trip').hidden(), budget_items_collection: () => p.oneToMany(BudgetItems).mappedBy('trip').hidden(), budget_settlements_collection: () => p.oneToMany(BudgetSettlements).mappedBy('trip').hidden(), diff --git a/server/src/db/migrations/Migration20200101042100_a_trip_reminder_remembers_it_was_sent.ts b/server/src/db/migrations/Migration20200101042100_a_trip_reminder_remembers_it_was_sent.ts new file mode 100644 index 000000000..c07766d3a --- /dev/null +++ b/server/src/db/migrations/Migration20200101042100_a_trip_reminder_remembers_it_was_sent.ts @@ -0,0 +1,25 @@ +import { addColumnIfMissing } from '../migration-utils'; +import { Migration } from '@mikro-orm/migrations'; + +/** + * A trip reminder remembers the start date it was sent for. The daily job + * used to match only "starts exactly N days from today", so a day the server + * was down at the job's hour lost that day's reminders for good. With the + * date on record the job sends every reminder that is due and not yet sent, + * and a trip moved to a new start date is reminded again. + * + * Trips whose reminder day already passed are marked as sent, so the first + * run after the upgrade does not repeat reminders that went out before. + */ +export class Migration20200101042100_a_trip_reminder_remembers_it_was_sent extends Migration { + override name = 'Migration20200101042100_a_trip_reminder_remembers_it_was_sent'; + + override async up(): Promise { + await addColumnIfMissing(this, 'trips', 'reminder_sent_for', 'reminder_sent_for TEXT'); + this.addSql(` + UPDATE trips SET reminder_sent_for = start_date + WHERE start_date IS NOT NULL AND reminder_days > 0 + AND date(start_date, '-' || reminder_days || ' days') < date('now') + `); + } +} diff --git a/server/src/db/repositories/Trips.repository.ts b/server/src/db/repositories/Trips.repository.ts index a40ea9aa2..a5317d224 100644 --- a/server/src/db/repositories/Trips.repository.ts +++ b/server/src/db/repositories/Trips.repository.ts @@ -34,6 +34,7 @@ export interface TripRawRow { feed_token: string | null; created_at: string | null; updated_at: string | null; + reminder_sent_for: string | null; } const _tripRawRowKeys: AssertRowKeys = true; @@ -76,6 +77,7 @@ interface TripSelectKyselyDB { feed_token: string | null; created_at: string | null; updated_at: string | null; + reminder_sent_for: string | null; }; users: { id: number; username: string }; trip_members: { id: number; trip_id: number; user_id: number }; @@ -882,11 +884,16 @@ export class TripsRepository extends TrekRepository { */ async listReminderCandidates(): Promise { return await this.qb('t') - .select(['t.id', 't.title', 't.user', 't.reminder_days', 't.start_date']) + .select(['t.id', 't.title', 't.user', 't.reminder_days', 't.start_date', 't.reminder_sent_for']) .andWhere({ reminder_days: { $gt: 0 }, start_date: { $ne: null } }) .execute('all', false); } + /** `UPDATE trips SET reminder_sent_for = ? WHERE id = ?`: the start date the reminder went out for. */ + async markReminderSent(id: number, startDate: string): Promise { + await this.nativeUpdate({ id }, { reminder_sent_for: startDate }); + } + // --------------------------------------------------------------------------- // Plan 3f Task 6 (`SystemNoticesService`) — additive. // --------------------------------------------------------------------------- @@ -1076,6 +1083,7 @@ export interface TripReminderCandidateRow { user_id: number; reminder_days: number; start_date: string; + reminder_sent_for: string | null; } /** `PublicApiService.listTrips`/`getTrip`'s 8-column trip projection. */ diff --git a/server/src/nest/common/timezoneService.ts b/server/src/nest/common/timezoneService.ts index 5c5861153..040870523 100644 --- a/server/src/nest/common/timezoneService.ts +++ b/server/src/nest/common/timezoneService.ts @@ -1,4 +1,5 @@ import tzlookup from 'tz-lookup'; +import { readEnv } from '../../app-config'; /** Local calendar date and clock time for an instant in an IANA timezone. */ export function localParts(iso: string | null, timezone: string | null): { date: string | null; time: string | null } { @@ -34,3 +35,14 @@ export function resolveTimeZone(lat: unknown, lng: unknown): string | null { return null; } } + +/** + * Today's date and the clock time where the instance runs, in the TZ the crons + * are scheduled in (CronRegistrarService). A UTC "today" disagreed with the + * schedule: east of Greenwich a 09:00 job saw the previous date. + */ +export function appClock(now: Date = new Date()): { date: string; time: string } { + const { date, time } = localParts(now.toISOString(), readEnv().app.tz || null); + return { date: date ?? now.toISOString().slice(0, 10), time: time ?? now.toISOString().slice(11, 16) }; +} + diff --git a/server/src/nest/notifications/reminder-jobs.service.ts b/server/src/nest/notifications/reminder-jobs.service.ts index 716e1987a..946562cbe 100644 --- a/server/src/nest/notifications/reminder-jobs.service.ts +++ b/server/src/nest/notifications/reminder-jobs.service.ts @@ -9,6 +9,8 @@ import { Trips } from '../../db/entities/Trips.entity'; import { TodoItems } from '../../db/entities/TodoItems.entity'; import { NotificationsService } from './notifications.service'; import { CronRegistrarService } from '../scheduling/cron-registrar.service'; +import { addIsoDays } from '@trek/shared'; +import { appClock } from '../common/timezoneService'; /** * The trip-reminder and todo-due reminder crons, in the domain that owns them @@ -75,28 +77,27 @@ export class ReminderJobsService implements OnApplicationBootstrap { this.registrar.register('todo-reminders', '0 9 * * *', () => this.todoTick()); } - /** Daily check for trips starting exactly reminder_days from now. */ + /** + * Daily check for trips whose reminder is due: the reminder day has come, + * the trip has not started yet, and no reminder went out for this start + * date. A day the job did not run is caught up on the next one, and a trip + * moved to a new start date is reminded again. "Today" is the date in the + * TZ the job is scheduled in, not UTC. + */ async tripTick(): Promise { try { if ((await this.appSettings.getValue('notify_trip_reminder')) === 'false') return; - // RJ3 (Task 0's R9 ruling): the legacy statement concatenated a - // per-row column (`t.reminder_days`) into a `date('now', '+' || - // t.reminder_days || ' days')` modifier — no bound-parameter or - // JS-constant-spelled helper can express that. Read the narrower - // candidate set (reminder_days set, start_date set) and do the - // per-row date-equality check in JS instead. `Date.UTC(...)` matches - // SQLite's own `date('now')`, which is UTC, not server-local time. + const today = appClock().date; const candidates = await this.trips.listReminderCandidates(); - const todayUtc = new Date(); - const trips = candidates.filter((t) => { - const target = new Date(Date.UTC(todayUtc.getUTCFullYear(), todayUtc.getUTCMonth(), todayUtc.getUTCDate())); - target.setUTCDate(target.getUTCDate() + t.reminder_days); - return t.start_date === target.toISOString().slice(0, 10); - }); + const trips = candidates.filter((t) => + t.reminder_sent_for !== t.start_date + && t.start_date >= today + && addIsoDays(t.start_date, -t.reminder_days) <= today); for (const trip of trips) { await this.notifications.send({ event: 'trip_reminder', actorId: null, scope: 'trip', targetId: trip.id, params: { trip: trip.title, tripId: String(trip.id) } }).catch(() => {}); + await this.trips.markReminderSent(trip.id, trip.start_date); } if (trips.length > 0) { @@ -112,18 +113,10 @@ export class ReminderJobsService implements OnApplicationBootstrap { try { if ((await this.appSettings.getValue('notify_todo_due')) === 'false') return; - // RJ4 (Task 0's R9 ruling): `date('now', '+' || ? || ' days')` - // concatenates a bound parameter into the modifier — resolved in JS - // the same way RJ3 is (`Date.UTC`-based, matching SQLite's UTC - // `date('now')`). `due_date` is documented as always canonical - // `YYYY-MM-DD` text, so a direct text-range bind reproduces - // `date(ti.due_date) <= / >= ...` for well-formed rows. - const todayUtc = new Date(); - const today = new Date(Date.UTC(todayUtc.getUTCFullYear(), todayUtc.getUTCMonth(), todayUtc.getUTCDate())); - const todayDate = today.toISOString().slice(0, 10); - const cutoff = new Date(today); - cutoff.setUTCDate(cutoff.getUTCDate() + TODO_REMINDER_LEAD_DAYS); - const cutoffDate = cutoff.toISOString().slice(0, 10); + // `due_date` is canonical `YYYY-MM-DD` text, so a text-range bind + // covers the lead window. "Today" is the date in the job's TZ. + const todayDate = appClock().date; + const cutoffDate = addIsoDays(todayDate, TODO_REMINDER_LEAD_DAYS); const todos = await this.todoItems.listDueForReminder(todayDate, cutoffDate); diff --git a/server/src/nest/reservations/reservations.service.ts b/server/src/nest/reservations/reservations.service.ts index 7ddb11631..9fbd7cbd2 100644 --- a/server/src/nest/reservations/reservations.service.ts +++ b/server/src/nest/reservations/reservations.service.ts @@ -37,6 +37,7 @@ import { Trips } from '../../db/entities/Trips.entity'; import { BudgetItems } from '../../db/entities/BudgetItems.entity'; import type { BudgetItemsRepository } from '../../db/repositories/BudgetItems.repository'; import type { TripsRepository } from '../../db/repositories/Trips.repository'; +import { appClock } from '../common/timezoneService'; type Trip = TripAccess; type BudgetEntry = { total_price?: number; category?: string; currency?: string | null; exchange_rate?: number } | undefined; @@ -484,9 +485,10 @@ export class ReservationsService { * docstring for the CTE/`UNION ALL`/GLOB/CAST shape. */ async listUpcoming(userId: number, limit = 6) { - const today = new Date().toISOString().slice(0, 10); - const now = new Date().toISOString(); - return await this.reservationsRepo.listUpcomingForUser(userId, today, now.slice(11, 16), limit); + // Reservation dates and times are wall-clock values, so "now" is the + // instance's local clock rather than UTC. + const { date, time } = appClock(); + return await this.reservationsRepo.listUpcomingForUser(userId, date, time, limit); } async getReservationWithJoins(id: string | number) { diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index fc0f54e2a..d699b4957 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -52,6 +52,7 @@ import { escapeLikePattern } from '../places/places.helpers'; import { NotFoundError, ValidationError } from '../common/domain-errors'; import { UnitOfWork } from '../database/unit-of-work'; import { legacyBoundIntegerText } from '../common/row-id'; +import { appClock } from '../common/timezoneService'; /** * The date range is refused, not cut short: generateDays used to clip the day @@ -455,7 +456,7 @@ export class TripsService { * trip with its per-trip day/place counts. */ /** TP21 — `TripsRepository.activeTrip`: the triple `CASE WHEN … relevance` projection and the double-`CASE WHEN` `ORDER BY`. */ - async activeTrip(userId: number, today = new Date().toISOString().slice(0, 10)) { + async activeTrip(userId: number, today = appClock().date) { return await this.tripsRepo.activeTrip(userId, today) as ActiveTrip & { relevance: number } | undefined; } diff --git a/server/tests/unit/db/legacy-baseline.test.ts b/server/tests/unit/db/legacy-baseline.test.ts index 94400cf9f..8907e17f2 100644 --- a/server/tests/unit/db/legacy-baseline.test.ts +++ b/server/tests/unit/db/legacy-baseline.test.ts @@ -32,6 +32,7 @@ const UNNUMBERED = [ BASELINE, 'Migration20200101040200_trek_photo_cache_meta_cache_key_not_null', 'Migration20200101042000_tours', + 'Migration20200101042100_a_trip_reminder_remembers_it_was_sent', ]; // The last step the positional runner ever had; planLegacyBaseline refuses a // schema_version past it. diff --git a/server/tests/unit/nest/app-clock.test.ts b/server/tests/unit/nest/app-clock.test.ts new file mode 100644 index 000000000..deac8e49c --- /dev/null +++ b/server/tests/unit/nest/app-clock.test.ts @@ -0,0 +1,22 @@ +import { afterEach, describe, expect, it } from 'vitest'; +import { appClock } from '../../../src/nest/common/timezoneService'; + +describe('appClock', () => { + const realTz = process.env.TZ; + afterEach(() => { + process.env.TZ = realTz; + }); + + it('APPCLOCK-001: reads the date and time where the instance runs, not in UTC', () => { + process.env.TZ = 'Australia/Sydney'; + // 23:30 UTC on the 6th is 10:30 on the 7th in Sydney. + expect(appClock(new Date('2026-10-06T23:30:00Z'))).toEqual({ date: '2026-10-07', time: '10:30' }); + }); + + it('APPCLOCK-002: falls back to UTC without a TZ, and for one Intl does not know', () => { + delete process.env.TZ; + expect(appClock(new Date('2026-10-06T23:30:00Z'))).toEqual({ date: '2026-10-06', time: '23:30' }); + process.env.TZ = 'Not/AZone'; + expect(appClock(new Date('2026-10-06T23:30:00Z'))).toEqual({ date: '2026-10-06', time: '23:30' }); + }); +}); diff --git a/server/tests/unit/nest/reminder-jobs.service.test.ts b/server/tests/unit/nest/reminder-jobs.service.test.ts index e08166425..be0b5f914 100644 --- a/server/tests/unit/nest/reminder-jobs.service.test.ts +++ b/server/tests/unit/nest/reminder-jobs.service.test.ts @@ -165,6 +165,28 @@ describe('trip reminder tick', () => { expect(logMock.logInfo).toHaveBeenCalledWith(expect.stringMatching(/^Trip reminders sent for 2 trip\(s\): /)); }); + it('RJOB-012 — catches up a reminder the job missed, sends each one once, and again for a moved trip', async () => { + const { user } = createUser(testDb); + // Due yesterday (start in 2 days, reminder 3 days ahead): the job did not run then. + const missed = createTrip(testDb, user.id, { title: 'Missed' }); + testDb.prepare("UPDATE trips SET reminder_days = 3, start_date = date('now', '+2 days') WHERE id = ?").run(missed.id); + // Already started: no reminder any more. + const started = createTrip(testDb, user.id, { title: 'Started' }); + testDb.prepare("UPDATE trips SET reminder_days = 3, start_date = date('now', '-1 days') WHERE id = ?").run(started.id); + + const { svc, send } = makeJobs(); + await svc.tripTick(); + expect(send.mock.calls.map(([p]) => p.targetId)).toEqual([missed.id]); + + send.mockClear(); + await svc.tripTick(); + expect(send).not.toHaveBeenCalled(); + + testDb.prepare("UPDATE trips SET start_date = date('now', '+1 days') WHERE id = ?").run(missed.id); + await svc.tripTick(); + expect(send.mock.calls.map(([p]) => p.targetId)).toEqual([missed.id]); + }); + it('RJOB-006 — the per-tick gate skips everything when notify_trip_reminder is false', async () => { const { user } = createUser(testDb); tripWithReminder(user.id, 3);