diff --git a/server/src/db/repositories/SchoolHolidayCountries.repository.ts b/server/src/db/repositories/SchoolHolidayCountries.repository.ts index 4d603c730..3aec11875 100644 --- a/server/src/db/repositories/SchoolHolidayCountries.repository.ts +++ b/server/src/db/repositories/SchoolHolidayCountries.repository.ts @@ -1,4 +1,69 @@ import type { SchoolHolidayCountries } from '../entities/SchoolHolidayCountries.entity'; +import { toRow, type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; -export class SchoolHolidayCountriesRepository extends TrekRepository {} +/** A `school_holiday_countries` row as the API emits it (SH1/SH3 — `SchoolHolidayCountryRequest`'s exact shape, `@trek/shared`). */ +export interface SchoolHolidayCountryRow { + code: string; + name: string; +} + +const _schoolHolidayCountryRowKeys: AssertRowKeys = true; + +/** `school_holiday_countries`'s single-table read/write shape (Plan 3f Task 2, SH1/SH3/SH4/SH6). */ +export class SchoolHolidayCountriesRepository extends TrekRepository { + /** SH1 — `SELECT code, name FROM school_holiday_countries ORDER BY name, code`. */ + async list(): Promise { + const countries = await this.find({}, { orderBy: { name: 'asc', code: 'asc' } }); + return countries.map((country) => toRow(country) as SchoolHolidayCountryRow); + } + + /** + * SH3 — `SELECT code, name FROM school_holiday_countries WHERE code = ?`. + * Named `findByCode`, not `find` (the base `EntityRepository.find` name is + * already taken — every other repository in this program follows the same + * `findById`/`findByCode`-style rename, e.g. `CategoriesRepository + * .findById`). + */ + async findByCode(code: string): Promise { + const country = await this.findOne({ code }); + return country ? (toRow(country) as SchoolHolidayCountryRow) : null; + } + + /** + * SH4 — `INSERT OR IGNORE INTO school_holiday_countries (code, name) + * VALUES (?, ?)`. `code` is the table's own PRIMARY KEY (no separate + * `uniques` constraint to name), so a plain `ON CONFLICT DO NOTHING` with + * no column list (the `BudgetCategoryOrderRepository.insertIgnore` + * precedent) covers it. Returns whether a row was actually inserted + * (`result.changes > 0` in the legacy code) — `numInsertedOrUpdatedRows` + * is Kysely's dialect-general affected-row count, 0 when the conflict + * target (an existing `code`) already existed and the insert was + * ignored — the `TripPhotosRepository.insertIgnore` precedent for + * reporting this back to the caller as a boolean instead of swallowing it. + */ + async insertIgnore(country: SchoolHolidayCountryRow): Promise { + const result = await this.kysely() + .insertInto('school_holiday_countries') + .values({ code: country.code, name: country.name }) + .onConflict((oc) => oc.doNothing()) + .executeTakeFirst(); + return (result?.numInsertedOrUpdatedRows ?? 0n) > 0n; + } + + /** + * SH6 — `DELETE FROM school_holiday_countries WHERE code = ?`. The legacy + * caller (`SchoolHolidaysService.deleteCountry`) always pre-checks + * existence (SH3, via `country()`) and the absence of any referencing + * region (SH5) inside the same transaction before calling this, so the + * affected-row count is never consulted — matching that, this returns + * `void`, not a count. + */ + async remove(code: string): Promise { + await this.nativeDelete({ code }); + } +} + +interface SchoolHolidayCountriesKyselyDB { + school_holiday_countries: { code: string; name: string }; +} diff --git a/server/src/db/repositories/SchoolHolidayPeriods.repository.ts b/server/src/db/repositories/SchoolHolidayPeriods.repository.ts index f0a8efba4..0fe2b9838 100644 --- a/server/src/db/repositories/SchoolHolidayPeriods.repository.ts +++ b/server/src/db/repositories/SchoolHolidayPeriods.repository.ts @@ -1,4 +1,62 @@ +import type { SchoolHolidayPeriod } from '@trek/shared'; import type { SchoolHolidayPeriods } from '../entities/SchoolHolidayPeriods.entity'; import { TrekRepository } from './_shared/trek-repository'; -export class SchoolHolidayPeriodsRepository extends TrekRepository {} +/** + * `school_holiday_periods`'s single-table read/write shape (Plan 3f Task 2, + * SH8/SH10/SH11/SH15). No `AssertRowKeys` pairing here (unlike this + * program's usual `toRow(entity) as Row` shape): the legacy SH8 statement + * itself renames `start_date`/`end_date` to `startDate`/`endDate` via SQL + * aliasing (the API's own `SchoolHolidayPeriod` shape, `@trek/shared`), so + * the equivalent here is a plain field-renaming `.map()` after a narrowed + * `find` (the `McpTokensRepository.listByUserAndKind` precedent for a + * `fields`-narrowed-then-mapped projection), not a row interface whose keys + * are expected to match the entity's column names 1:1. + */ +export class SchoolHolidayPeriodsRepository extends TrekRepository { + /** + * SH8 — `SELECT name, start_date AS startDate, end_date AS endDate FROM + * school_holiday_periods WHERE region_id = ? ORDER BY start_date, + * end_date, name`. `id`/`region_id` are not selected — the legacy + * projection never returns them — so `fields` narrows to exactly the + * three legacy columns; none of the three is a `persist(false)` shadow + * scalar (that trap only applies to `region_id`, not selected here), so + * narrowing is safe. + */ + async listForRegion(region_id: number): Promise { + const periods = await this.find( + { region: region_id }, + { fields: ['name', 'start_date', 'end_date'], orderBy: { start_date: 'asc', end_date: 'asc', name: 'asc' } }, + ); + return periods.map((period) => ({ name: period.name, startDate: period.start_date, endDate: period.end_date })); + } + + /** + * SH10/SH15 — `DELETE FROM school_holiday_periods WHERE region_id = ?`, + * the replace-all half of `writePeriods` (called from both `createRegion` + * and `updateRegion`, plus `deleteRegion`'s own cleanup) and always + * called from inside the surrounding `uow.transactional` block, same as + * the legacy code. + */ + async deleteForRegion(region_id: number): Promise { + await this.nativeDelete({ region: region_id }); + } + + /** + * SH11 — `INSERT INTO school_holiday_periods (region_id, name, + * start_date, end_date) VALUES (?, ?, ?, ?)`, the legacy `.prepare()` + * -then-loop shape (the ONE such shape in this whole plan) — one batched + * `insertMany` call here instead of N round trips, same end state (every + * period row written), called only from inside the surrounding + * transaction (never on its own). Named `insertPeriods`, not `insertMany` + * (the base `EntityRepository.insertMany` name is already taken). Empty + * `holidays` is a legitimate legacy no-op (the loop over an empty array + * writes nothing) — guarded here rather than handed to `insertMany`, + * matching every other batch-write method in this program + * (`AssignmentParticipantsRepository.insertIgnore`'s precedent). + */ + async insertPeriods(region_id: number, holidays: SchoolHolidayPeriod[]): Promise { + if (holidays.length === 0) return; + await this.insertMany(holidays.map((holiday) => ({ region: region_id, name: holiday.name, start_date: holiday.startDate, end_date: holiday.endDate }))); + } +} diff --git a/server/src/db/repositories/SchoolHolidayRegions.repository.ts b/server/src/db/repositories/SchoolHolidayRegions.repository.ts index c37c1ba06..3d4233eb0 100644 --- a/server/src/db/repositories/SchoolHolidayRegions.repository.ts +++ b/server/src/db/repositories/SchoolHolidayRegions.repository.ts @@ -1,4 +1,140 @@ +import { collateNoCase, columnIncrementedBy } from '../dialect/sql-functions'; import type { SchoolHolidayRegions } from '../entities/SchoolHolidayRegions.entity'; +import { toRow, type AssertRowKeys } from './_shared/rows'; import { TrekRepository } from './_shared/trek-repository'; -export class SchoolHolidayRegionsRepository extends TrekRepository {} +/** + * A `school_holiday_regions` row as the API emits it, MINUS the synthesized + * `country || '-MANUAL-' || id AS code` column (SH2/SH7) — computed in + * `SchoolHolidaysService` from `country`/`id` instead of in SQL: it is pure, + * derived data with no WHERE clause anywhere ever filtering on it (unlike + * `VacayHolidayCalendars.region`, which stores this same string as a plain + * value to MATCH against), so a JS template string after the read is exactly + * as correct as the legacy SQL concatenation and needs no `concat()` dialect + * helper. + */ +export interface SchoolHolidayRegionRow { + id: number; + country: string; + name: string; + revision: number; +} + +const _schoolHolidayRegionRowKeys: AssertRowKeys = true; + +/** + * `school_holiday_regions`'s single-table read/write shape (Plan 3f Task 2, + * SH2/SH5/SH7/SH9/SH12/SH13/SH16). `country` is a `persist(false)` shadow + * scalar of the real relation (`countryRef`, `.manyToOne(SchoolHolidayCountries) + * .ref().joinColumn('country')`) — §14 of the plan's inventory flagged this + * column as possibly FK-less; reading the entity directly (not assumed) shows + * it IS FK-backed, just under the inverted naming this table's entity + * generator produced (the relation carries the `Ref` suffix, the bare column + * name is the plain-scalar shadow — the opposite of `SchoolHolidayPeriods + * .region`/`.region_id`). A plain, unrestricted `find`/`findOne` (no `fields` + * narrowing) hydrates `country` from the physical column exactly like any + * other persist(false) shadow twin in this program (`TripPhotosRepository + * .listTripIdsSharedForPhoto`'s docstring names the same trap for a narrowed + * read) — every read below reads the whole row, so this never bites here. + * Writes go through `countryRef` (the relation property), never `country` + * itself (`persist(false)` silently drops a direct write), matching every + * other shadow-FK write in this program (`DayNotesRepository.create`'s + * `day: input.day_id` precedent). + */ +export class SchoolHolidayRegionsRepository extends TrekRepository { + /** SH2 — `SELECT *, country || '-MANUAL-' || id AS code FROM school_holiday_regions ORDER BY name, id` (code column computed by the service, see this file's own docstring). */ + async list(): Promise { + const regions = await this.find({}, { orderBy: { name: 'asc', id: 'asc' } }); + return regions.map((region) => toRow(region) as SchoolHolidayRegionRow); + } + + /** + * SH7 — same base text as SH2, single row, `WHERE id = ?`, no `ORDER BY`. + * Named `findById`, not `find` (the base `EntityRepository.find` name is + * already taken). + */ + async findById(id: number): Promise { + const region = await this.findOne({ id }); + return region ? (toRow(region) as SchoolHolidayRegionRow) : null; + } + + /** + * SH5 — `SELECT id FROM school_holiday_regions WHERE country = ? LIMIT 1` + * (`deleteCountry`'s "no region still references this country" guard). + * Also the additive consumer Plan 3f Task 5 (`vacay.service.ts + * #validateManualRegion`, VC45) calls, per the plan's own cross-domain + * sequencing note — this repository lands first, so Task 5 gets a + * populated method instead of leaving its own read raw a beat longer. + */ + async existsForCountry(country: string): Promise { + const row = await this.findOne({ country }, { fields: ['id'] }); + return row !== null; + } + + /** + * SH9 (`checkName`'s duplicate-name guard) — `SELECT id FROM + * school_holiday_regions WHERE country = ? AND name = ? COLLATE NOCASE + * AND id != ?`. Uses `collateNoCase` (Plan 3f Task 0, R9) as the filter + * KEY, the same `{ [lower(platform, col)]: value }` shape + * `Users.repository.ts#findIdByUsernameCI` uses for its own + * case-insensitive collision check — NOT `lower()`: the two helpers are + * proven to agree on every input tested (Task 0's report, SQLF-061) but + * are textually different SQL constructs matching two textually different + * legacy statements, and the brief for this task is explicit that + * `collateNoCase` is the one to use here, not a substitution. + * `collateNoCase` renders the COLLATE on the column side (`name COLLATE + * NOCASE = ?`) where the legacy statement spells it on the value side + * (`name = ? COLLATE NOCASE`) — row-identical (SQLF-060), not just + * assumed, per Task 0's own concern note for this task. + */ + async findIdByNameCI(country: string, name: string, excludeId: number): Promise { + const platform = this.getEntityManager().getPlatform(); + const row = await this.findOne( + { country, [collateNoCase(platform, 'name')]: name, id: { $ne: excludeId } }, + { fields: ['id'] }, + ); + return row?.id ?? null; + } + + /** + * SH12 — `INSERT INTO school_holiday_regions (country, name) VALUES (?, + * ?)`. `revision` is omitted from the write, same as the legacy column + * list — the entity's own default (`p.integer().default(1)`, verified + * against `Migration20200101033600_create_school_holiday_countries.ts`'s + * `revision INTEGER NOT NULL DEFAULT 1`) applies, matching the legacy + * column default exactly. Named `insertRegion`, not `insert` (the base + * `EntityRepository.insert` name is already taken). + */ + async insertRegion(country: string, name: string): Promise { + return this.insert({ countryRef: country, name }); + } + + /** + * SH13 (`updateRegion`) — `UPDATE school_holiday_regions SET name = ?, + * revision = revision + 1 WHERE id = ? AND revision = ?`. A genuine + * optimistic-concurrency ("lost update") guard, not a cosmetic detail: + * this method returns the raw affected-row count (0 or 1) and does NOT + * throw or interpret it itself — `nativeUpdate`'s own return value IS + * that count, so nothing here can silently swallow a 0-row result the + * way an ORM `nativeUpdate` wrapped in a helper that only returns the + * updated entity would. `SchoolHolidaysService.updateRegion` is the one + * that turns a 0 into `ConflictException`. `columnIncrementedBy` + * (`InviteTokensRepository.incrementUsedCount`'s precedent) for the + * sibling-column `revision + 1` — never a hand-spelled `raw()` call here. + */ + async updateWithRevision(id: number, expectedRevision: number, name: string): Promise { + const platform = this.getEntityManager().getPlatform(); + return this.nativeUpdate({ id, revision: expectedRevision }, { name, revision: columnIncrementedBy(platform, 'revision', 1) }); + } + + /** + * SH16 — `DELETE FROM school_holiday_regions WHERE id = ?`. The caller + * (`deleteRegion`) already re-read the region and checked its revision + * inside the same transaction before calling this, so — same as + * `SchoolHolidayCountriesRepository.remove` — the affected-row count is + * never consulted. + */ + async remove(id: number): Promise { + await this.nativeDelete({ id }); + } +} diff --git a/server/src/db/repositories/VacayHolidayCalendars.repository.ts b/server/src/db/repositories/VacayHolidayCalendars.repository.ts index 2d4bbd0f6..6421a55aa 100644 --- a/server/src/db/repositories/VacayHolidayCalendars.repository.ts +++ b/server/src/db/repositories/VacayHolidayCalendars.repository.ts @@ -1,4 +1,26 @@ import type { VacayHolidayCalendars } from '../entities/VacayHolidayCalendars.entity'; import { TrekRepository } from './_shared/trek-repository'; -export class VacayHolidayCalendarsRepository extends TrekRepository {} +/** + * Plan 3f Task 2 (`SchoolHolidaysService.deleteRegion`) appends EXACTLY this + * one additive method — Task 5 (vacay) builds the rest of this repository + * around it later, per the plan's own file-ownership rule for this shared + * file (`.superpowers/sdd/2026-09-25-orm-phase3f/task-2-brief.md`); no other + * method, no class-level change beyond this. + */ +export class VacayHolidayCalendarsRepository extends TrekRepository { + /** + * SH14 (`deleteRegion`'s cross-domain integrity guard, the reverse + * direction of vacay's own VC45) — `SELECT id FROM vacay_holiday_calendars + * WHERE type = 'school_holiday' AND region = ? LIMIT 1`. `region` is a + * plain `.text()` column with no FK/relation to `SchoolHolidayRegions` at + * all (§14/§15.9 of the plan's inventory, verified against both entities + * directly, not assumed) — it stores the synthesized `-MANUAL- + * ` code as a free-form string, matched by VALUE, so this stays a + * plain `WHERE` filter on two scalar columns, never a joined relation. + */ + async existsForSchoolRegion(region: string): Promise { + const row = await this.findOne({ type: 'school_holiday', region }, { fields: ['id'] }); + return row !== null; + } +} diff --git a/server/src/nest/school-holidays/school-holidays.module.ts b/server/src/nest/school-holidays/school-holidays.module.ts index 1d3729949..7ebede236 100644 --- a/server/src/nest/school-holidays/school-holidays.module.ts +++ b/server/src/nest/school-holidays/school-holidays.module.ts @@ -1,8 +1,24 @@ import { McpSharedModule } from '../mcp-shared/mcp-shared.module'; import { Module } from '@nestjs/common'; +import { MikroOrmModule } from '@mikro-orm/nestjs'; +import { SchoolHolidayCountries } from '../../db/entities/SchoolHolidayCountries.entity'; +import { SchoolHolidayRegions } from '../../db/entities/SchoolHolidayRegions.entity'; +import { SchoolHolidayPeriods } from '../../db/entities/SchoolHolidayPeriods.entity'; +import { VacayHolidayCalendars } from '../../db/entities/VacayHolidayCalendars.entity'; import { SchoolHolidaysController } from './school-holidays.controller'; import { SchoolHolidaysService } from './school-holidays.service'; import { SchoolHolidaysMcp } from './school-holidays.mcp'; -@Module({ imports: [McpSharedModule], controllers: [SchoolHolidaysController], providers: [SchoolHolidaysService, SchoolHolidaysMcp] }) +// MikroOrmModule.forFeature registers each repository for its +// @InjectRepository(...) in SchoolHolidaysService — the forFeature + +// @InjectRepository wiring pattern every domain copies (settings.module.ts's +// precedent comment). VacayHolidayCalendars is vacay's entity (Plan 3f Task +// 5, not yet landed) — registered here only because this service injects +// its repository for SH14's cross-domain guard (`existsForSchoolRegion`); +// no other change to that domain. +@Module({ + imports: [McpSharedModule, MikroOrmModule.forFeature([SchoolHolidayCountries, SchoolHolidayRegions, SchoolHolidayPeriods, VacayHolidayCalendars])], + controllers: [SchoolHolidaysController], + providers: [SchoolHolidaysService, SchoolHolidaysMcp], +}) export class SchoolHolidaysModule {} diff --git a/server/src/nest/school-holidays/school-holidays.service.ts b/server/src/nest/school-holidays/school-holidays.service.ts index 0ec89a26c..9a3d322e4 100644 --- a/server/src/nest/school-holidays/school-holidays.service.ts +++ b/server/src/nest/school-holidays/school-holidays.service.ts @@ -1,61 +1,87 @@ import { ConflictException, Injectable, NotFoundException } from '@nestjs/common'; +import { InjectRepository } from '@mikro-orm/nestjs'; import type { SchoolHolidayCatalog, SchoolHolidayCountryRequest, SchoolHolidayPeriod, SchoolHolidayRegion, SchoolHolidayRegionDetail, SchoolHolidayRegionRequest } from '@trek/shared'; -import { DatabaseService } from '../database/database.service'; +import { SchoolHolidayCountries } from '../../db/entities/SchoolHolidayCountries.entity'; +import type { SchoolHolidayCountriesRepository } from '../../db/repositories/SchoolHolidayCountries.repository'; +import { SchoolHolidayRegions } from '../../db/entities/SchoolHolidayRegions.entity'; +import type { SchoolHolidayRegionsRepository, SchoolHolidayRegionRow } from '../../db/repositories/SchoolHolidayRegions.repository'; +import { SchoolHolidayPeriods } from '../../db/entities/SchoolHolidayPeriods.entity'; +import type { SchoolHolidayPeriodsRepository } from '../../db/repositories/SchoolHolidayPeriods.repository'; +import { VacayHolidayCalendars } from '../../db/entities/VacayHolidayCalendars.entity'; +import type { VacayHolidayCalendarsRepository } from '../../db/repositories/VacayHolidayCalendars.repository'; import { UnitOfWork } from '../database/unit-of-work'; +/** + * School-holidays domain service — moved off `DatabaseService`/raw SQL onto + * `SchoolHolidayCountriesRepository`/`SchoolHolidayRegionsRepository`/ + * `SchoolHolidayPeriodsRepository` (Plan 3f Task 2). The synthesized + * `country || '-MANUAL-' || id AS code` column (SH2/SH7) is computed here in + * JS (`toRegion`) rather than in SQL — see `SchoolHolidayRegions.repository + * .ts`'s own docstring for why that is still exact parity. `checkName`'s + * duplicate-name guard and `updateRegion`'s optimistic-concurrency revision + * check keep their exact legacy shapes; see the repository methods they call + * for the parity notes. + */ @Injectable() export class SchoolHolidaysService { constructor( - private readonly db: DatabaseService, + @InjectRepository(SchoolHolidayCountries) private readonly countries: SchoolHolidayCountriesRepository, + @InjectRepository(SchoolHolidayRegions) private readonly regions: SchoolHolidayRegionsRepository, + @InjectRepository(SchoolHolidayPeriods) private readonly periods: SchoolHolidayPeriodsRepository, + @InjectRepository(VacayHolidayCalendars) private readonly vacayCalendars: VacayHolidayCalendarsRepository, private readonly uow: UnitOfWork, ) {} async catalog(): Promise { - return { - countries: this.db.all('SELECT code, name FROM school_holiday_countries ORDER BY name, code'), - regions: this.db.all("SELECT *, country || '-MANUAL-' || id AS code FROM school_holiday_regions ORDER BY name, id"), - }; + const countries = await this.countries.list(); + const regions = await this.regions.list(); + return { countries, regions: regions.map((region) => this.toRegion(region)) }; } async country(code: string): Promise { - const country = this.db.get('SELECT code, name FROM school_holiday_countries WHERE code = ?', code); + const country = await this.countries.findByCode(code); if (!country) throw new NotFoundException('Country not found'); return country; } async createCountry(country: SchoolHolidayCountryRequest) { - const inserted = this.db.run('INSERT OR IGNORE INTO school_holiday_countries (code, name) VALUES (?, ?)', country.code, country.name); - if (!inserted.changes) throw new ConflictException('Country already exists'); + const inserted = await this.countries.insertIgnore(country); + if (!inserted) throw new ConflictException('Country already exists'); return country; } async deleteCountry(code: string) { return await this.uow.transactional(async () => { await this.country(code); - if (this.db.get('SELECT id FROM school_holiday_regions WHERE country = ? LIMIT 1', code)) { + if (await this.regions.existsForCountry(code)) { throw new ConflictException('Remove the regions before deleting this country'); } - this.db.run('DELETE FROM school_holiday_countries WHERE code = ?', code); + await this.countries.remove(code); return { success: true }; }); } async region(id: number): Promise { - const region = this.db.get("SELECT *, country || '-MANUAL-' || id AS code FROM school_holiday_regions WHERE id = ?", id); + const region = await this.regions.findById(id); if (!region) throw new NotFoundException('Region not found'); - return { ...region, holidays: this.db.all('SELECT name, start_date AS startDate, end_date AS endDate FROM school_holiday_periods WHERE region_id = ? ORDER BY start_date, end_date, name', id) }; + const holidays = await this.periods.listForRegion(id); + return { ...this.toRegion(region), holidays }; + } + + /** The SH2/SH7 synthesized `code` column — see `SchoolHolidayRegions.repository.ts`'s class docstring. */ + private toRegion(region: SchoolHolidayRegionRow): SchoolHolidayRegion { + return { id: region.id, country: region.country, name: region.name, revision: region.revision, code: `${region.country}-MANUAL-${region.id}` }; } private async checkName(country: string, name: string, id: number) { - if (this.db.get('SELECT id FROM school_holiday_regions WHERE country = ? AND name = ? COLLATE NOCASE AND id != ?', country, name, id)) { + if (await this.regions.findIdByNameCI(country, name, id)) { throw new ConflictException('A region with this name already exists'); } } private async writePeriods(id: number, holidays: SchoolHolidayPeriod[]) { - this.db.run('DELETE FROM school_holiday_periods WHERE region_id = ?', id); - const insert = this.db.prepare('INSERT INTO school_holiday_periods (region_id, name, start_date, end_date) VALUES (?, ?, ?, ?)'); - for (const holiday of holidays) insert.run(id, holiday.name, holiday.startDate, holiday.endDate); + await this.periods.deleteForRegion(id); + await this.periods.insertPeriods(id, holidays); } async createRegion(country: string, body: SchoolHolidayRegionRequest) { @@ -63,8 +89,7 @@ export class SchoolHolidaysService { await this.country(country); await this.checkName(country, body.name, 0); if (body.revision !== 0) throw new ConflictException('New regions must have revision zero'); - const inserted = this.db.run('INSERT INTO school_holiday_regions (country, name) VALUES (?, ?)', country, body.name); - const id = Number(inserted.lastInsertRowid); + const id = await this.regions.insertRegion(country, body.name); await this.writePeriods(id, body.holidays); return this.region(id); }); @@ -74,8 +99,8 @@ export class SchoolHolidaysService { return await this.uow.transactional(async () => { const region = await this.region(id); await this.checkName(region.country, body.name, id); - const updated = this.db.run('UPDATE school_holiday_regions SET name = ?, revision = revision + 1 WHERE id = ? AND revision = ?', body.name, id, body.revision); - if (!updated.changes) throw new ConflictException('This region changed. Reopen it before saving again.'); + const affected = await this.regions.updateWithRevision(id, body.revision, body.name); + if (!affected) throw new ConflictException('This region changed. Reopen it before saving again.'); await this.writePeriods(id, body.holidays); return this.region(id); }); @@ -85,11 +110,11 @@ export class SchoolHolidaysService { return await this.uow.transactional(async () => { const region = await this.region(id); if (region.revision !== revision) throw new ConflictException('This region changed. Reload before deleting it.'); - if (this.db.get("SELECT id FROM vacay_holiday_calendars WHERE type = 'school_holiday' AND region = ? LIMIT 1", region.code)) { + if (await this.vacayCalendars.existsForSchoolRegion(region.code)) { throw new ConflictException('This region is used by vacation calendars and cannot be deleted'); } - this.db.run('DELETE FROM school_holiday_periods WHERE region_id = ?', id); - this.db.run('DELETE FROM school_holiday_regions WHERE id = ?', id); + await this.periods.deleteForRegion(id); + await this.regions.remove(id); return { success: true }; }); } diff --git a/server/tests/e2e/school-holidays.e2e.test.ts b/server/tests/e2e/school-holidays.e2e.test.ts index 30c047938..44c9afb20 100644 --- a/server/tests/e2e/school-holidays.e2e.test.ts +++ b/server/tests/e2e/school-holidays.e2e.test.ts @@ -3,7 +3,9 @@ import request from 'supertest'; import cookieParser from 'cookie-parser'; import { Test } from '@nestjs/testing'; import type { INestApplication } from '@nestjs/common'; +import { MikroORM } from '@mikro-orm/core'; import { RealtimeModule } from '../../src/nest/realtime/realtime.module'; +import { withRequestContext } from '../../src/nest/database/request-context'; import { sessionCookie } from './harness'; const { db } = vi.hoisted(() => { @@ -29,6 +31,7 @@ const base = '/api/school-holiday-catalog'; const winter = { name: 'Winter break', startDate: '2026-12-20', endDate: '2027-01-06' }; let app: INestApplication; let service: SchoolHolidaysService; +let orm: MikroORM; beforeAll(async () => { db.pragma('foreign_keys = ON'); createTables(db); @@ -41,6 +44,7 @@ beforeAll(async () => { app.useGlobalFilters(new TrekExceptionFilter()); await app.init(); service = app.get(SchoolHolidaysService); + orm = app.get(MikroORM); }); afterAll(async () => { await app.close(); db.close(); }); beforeEach(() => { @@ -52,6 +56,20 @@ async function seed() { return await service.createRegion('US', { name: 'Seattle schools', revision: 0, holidays: [winter] }); } +// Plan 3f Task 2: `SchoolHolidaysService` now reads/writes through +// repositories, which fail closed outside a MikroORM request context +// (`TrekRepository.validateRequestContext`, program rule — D6). An HTTP call +// through `request(app.getHttpServer())...` gets one for free (`@mikro-orm +// /nestjs`'s per-request middleware); a direct `service.xxx(...)`/ +// `mcp.xxx(...)` call from this test file — bypassing both the HTTP +// transport and the MCP registry's own `onInvoke` wrapper — does not, so +// every `it(...)` below that touches `service`/`mcp` directly (every one +// except the pure-401/403 auth-gate test, which never reaches a repository) +// wraps its body in `withRequestContext(orm, ...)`, the same helper an MCP +// tool call, a cron tick or a plugin RPC dispatch uses in production +// (`request-context.ts`'s own docstring; `doc-sync.e2e.test.ts`'s +// precedent). This is a test-harness adaptation, not a behavior change — +// nothing about `SchoolHolidaysService`'s public contract moved. describe('global manual school holidays', () => { it('requires authentication and admin rights for every write', async () => { await request(app.getHttpServer()).get(base).expect(401); @@ -67,7 +85,7 @@ describe('global manual school holidays', () => { expect(() => validateBodyContracts(app)).not.toThrow(); }); - it('lets any member discover regions and read dates without external requests', async () => { + it('lets any member discover regions and read dates without external requests', () => withRequestContext(orm, async () => { const fetchSpy = vi.spyOn(globalThis, 'fetch'); const country = await request(app.getHttpServer()).post(`${base}/countries`).set('Cookie', sessionCookie(1)).send({ code: 'US', name: 'USA' }).expect(201); expect(country.body.code).toBe('US'); @@ -81,17 +99,17 @@ describe('global manual school holidays', () => { expect(await service.holidays(created.body.id, '2025')).toEqual([]); expect(fetchSpy).not.toHaveBeenCalled(); fetchSpy.mockRestore(); - }); + })); - it('validates dates and body shape before writing', async () => { + it('validates dates and body shape before writing', () => withRequestContext(orm, async () => { await service.createCountry({ code: 'US', name: 'USA' }); for (const holidays of [[{ ...winter, endDate: '2026-01-01' }], [{ ...winter, startDate: '2026-02-30' }], [{ ...winter, name: ' ' }]]) { await request(app.getHttpServer()).post(`${base}/countries/US/regions`).set('Cookie', sessionCookie(1)).send({ name: 'Seattle', revision: 0, holidays }).expect(400); } expect((await service.catalog()).regions).toEqual([]); - }); + })); - it('preserves ids on rename and rejects stale saves atomically', async () => { + it('preserves ids on rename and rejects stale saves atomically', () => withRequestContext(orm, async () => { const region = await seed(); const updated = await request(app.getHttpServer()).put(`${base}/regions/${region.id}`).set('Cookie', sessionCookie(1)).send({ name: 'Renamed', revision: 1, holidays: [] }).expect(200); expect(updated.body).toMatchObject({ code: region.code, revision: 2, holidays: [] }); @@ -99,9 +117,9 @@ describe('global manual school holidays', () => { expect((await service.region(region.id)).name).toBe('Renamed'); expect((await service.region(region.id)).holidays).toEqual([]); await request(app.getHttpServer()).delete(`${base}/regions/${region.id}?revision=1`).set('Cookie', sessionCookie(1)).expect(409); - }); + })); - it('rejects duplicates, missing countries and nonzero initial revisions', async () => { + it('rejects duplicates, missing countries and nonzero initial revisions', () => withRequestContext(orm, async () => { await seed(); await expect(service.createCountry({ code: 'US', name: 'America' })).rejects.toThrow('already exists'); await expect(service.createRegion('US', { name: 'SEATTLE SCHOOLS', revision: 0, holidays: [] })).rejects.toThrow('already exists'); @@ -109,9 +127,9 @@ describe('global manual school holidays', () => { await expect(service.createRegion('US', { name: 'School', revision: 1, holidays: [] })).rejects.toThrow('revision zero'); const other = await service.createRegion('US', { name: 'Other', revision: 0, holidays: [] }); await expect(service.updateRegion(other.id, { name: 'Seattle schools', revision: 1, holidays: [] })).rejects.toThrow('already exists'); - }); + })); - it('protects used regions and deletes unused regions with their periods', async () => { + it('protects used regions and deletes unused regions with their periods', () => withRequestContext(orm, async () => { const region = await seed(); db.prepare('INSERT OR IGNORE INTO vacay_plans (id, owner_id) VALUES (1, 1)').run(); db.prepare("INSERT INTO vacay_holiday_calendars (plan_id, type, region) VALUES (1, 'school_holiday', ?)").run(region.code); @@ -122,26 +140,26 @@ describe('global manual school holidays', () => { expect(db.prepare('SELECT * FROM school_holiday_periods').all()).toEqual([]); await request(app.getHttpServer()).delete(`${base}/countries/US`).set('Cookie', sessionCookie(1)).expect(200); expect(await service.catalog()).toEqual({ countries: [], regions: [] }); - }); + })); - it('reports missing regions and malformed year and id parameters', async () => { + it('reports missing regions and malformed year and id parameters', () => withRequestContext(orm, async () => { const region = await seed(); await request(app.getHttpServer()).get(`${base}/regions/${region.id}`).set('Cookie', sessionCookie(2)).expect(200); await request(app.getHttpServer()).get(`${base}/regions/999999`).set('Cookie', sessionCookie(2)).expect(404); await request(app.getHttpServer()).get(`${base}/regions/invalid`).set('Cookie', sessionCookie(2)).expect(400); await request(app.getHttpServer()).get(`${base}/regions/${region.id}/holidays/xx`).set('Cookie', sessionCookie(2)).expect(400); await request(app.getHttpServer()).delete(`${base}/countries/CA`).set('Cookie', sessionCookie(1)).expect(404); - }); + })); - it('exposes the same manual catalog and periods through MCP', async () => { + it('exposes the same manual catalog and periods through MCP', () => withRequestContext(orm, async () => { const region = await seed(); const mcp = app.get(SchoolHolidaysMcp); expect(() => createTestRegistry([mcp], { accessPolicy: trekMcpAccessPolicy, validateAccess: trekMcpValidateAccess })).not.toThrow(); expect(JSON.stringify(await mcp.catalog())).toContain(region.code); expect(JSON.stringify(await mcp.forYear({ regionId: region.id, year: 2027 }))).toContain('Winter break'); - }); + })); - it('denies every MCP mutation to normal users without changing the catalog', async () => { + it('denies every MCP mutation to normal users without changing the catalog', () => withRequestContext(orm, async () => { const region = await seed(); const mcp = app.get(SchoolHolidaysMcp); const member = { userId: 2, scopes: null, isStaticToken: false }; @@ -157,9 +175,9 @@ describe('global manual school holidays', () => { expect(await service.region(region.id)).toEqual(before); expect((await service.catalog()).countries).toHaveLength(1); expect((await service.catalog()).regions).toHaveLength(1); - }); + })); - it('supports the complete admin MCP workflow with validation and revision protection', async () => { + it('supports the complete admin MCP workflow with validation and revision protection', () => withRequestContext(orm, async () => { const mcp = app.get(SchoolHolidaysMcp); const admin = { userId: 1, scopes: null, isStaticToken: false }; expect(await mcp.createCountry({ code: 'US', name: 'USA' }, admin)).not.toHaveProperty('isError', true); @@ -176,5 +194,5 @@ describe('global manual school holidays', () => { expect(await mcp.deleteRegion({ regionId: region.id, revision: 2 }, admin)).not.toHaveProperty('isError', true); expect(await mcp.deleteCountry({ code: 'US' }, admin)).not.toHaveProperty('isError', true); expect(await service.catalog()).toEqual({ countries: [], regions: [] }); - }); + })); }); diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 31dde2f23..73d47a798 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -145,6 +145,10 @@ import { createTestPackingTemplateItemsRepo, } from './packing-repos'; import { createTestTodoItemsRepo, createTestTodoCategoryAssigneesRepo } from './todo-repos'; +import { + createTestSchoolHolidayCountriesRepo, createTestSchoolHolidayRegionsRepo, + createTestSchoolHolidayPeriodsRepo, createTestVacayHolidayCalendarsRepo, +} from './school-holidays-repos'; import { AppSettings } from '../../src/db/entities/AppSettings.entity'; import { AuditLog } from '../../src/db/entities/AuditLog.entity'; import { Users } from '../../src/db/entities/Users.entity'; @@ -362,7 +366,16 @@ export async function createMcpTestRegistry(): Promise { new AssignmentsMcp(assignmentsService, daysService, authService, guards), new CollabMcp(collabService, authService, addonsService, guards), new VacayMcp(new VacayService(dbService, realtimeService, notificationsStub(), await createTestUnitOfWork(dbService.connection)), authService, addonsService), - new SchoolHolidaysMcp(new SchoolHolidaysService(dbService, await createTestUnitOfWork(dbService.connection)), guards), + new SchoolHolidaysMcp( + new SchoolHolidaysService( + await createTestSchoolHolidayCountriesRepo(dbService.connection), + await createTestSchoolHolidayRegionsRepo(dbService.connection), + await createTestSchoolHolidayPeriodsRepo(dbService.connection), + await createTestVacayHolidayCalendarsRepo(dbService.connection), + await createTestUnitOfWork(dbService.connection), + ), + guards, + ), new TripsMcp(tripsService, todoService, collabService, authService, calendarService, membersService, readModelService, addonsService, guards), new TripPromptsMcp(tripsService, readModelService, packingService, addonsService), new ShareMcp(new ShareService(dbService, new SettingsService(await createTestUnitOfWork(dbService.connection), appSettings, await createTestSettingsRepo(dbService.connection)), permissionsService, queryHelpersService, placePhotoCache, await createTestUnitOfWork(dbService.connection), await createTestReservationsRepo(dbService.connection)), authService, guards), diff --git a/server/tests/helpers/school-holidays-repos.ts b/server/tests/helpers/school-holidays-repos.ts new file mode 100644 index 000000000..154cecd3d --- /dev/null +++ b/server/tests/helpers/school-holidays-repos.ts @@ -0,0 +1,46 @@ +import type Database from 'better-sqlite3'; +import { sharedTestOrm } from './test-uow'; +import { SchoolHolidayCountries } from '../../src/db/entities/SchoolHolidayCountries.entity'; +import type { SchoolHolidayCountriesRepository } from '../../src/db/repositories/SchoolHolidayCountries.repository'; +import { SchoolHolidayRegions } from '../../src/db/entities/SchoolHolidayRegions.entity'; +import type { SchoolHolidayRegionsRepository } from '../../src/db/repositories/SchoolHolidayRegions.repository'; +import { SchoolHolidayPeriods } from '../../src/db/entities/SchoolHolidayPeriods.entity'; +import type { SchoolHolidayPeriodsRepository } from '../../src/db/repositories/SchoolHolidayPeriods.repository'; +import { VacayHolidayCalendars } from '../../src/db/entities/VacayHolidayCalendars.entity'; +import type { VacayHolidayCalendarsRepository } from '../../src/db/repositories/VacayHolidayCalendars.repository'; + +/** + * Plan 3f Task 2 (`SchoolHolidaysService`) test-only repository factories, + * bound to a suite's own better-sqlite3 handle via the SAME memoised + * `sharedTestOrm` `test-uow.ts` exports — required so a repository built + * here and a `UnitOfWork` built from `createTestUnitOfWork` (that file) + * resolve the IDENTICAL context-resolving `EntityManager` (the + * `todo-repos.ts`/`files-repos.ts`/`collab-repos.ts` precedent this file + * otherwise copies verbatim: a repository from a SECOND, independent ORM + * instance would write outside the open transaction). + * + * A NEW file rather than an addition to `test-uow.ts`, same reasoning those + * files give for themselves: avoids colliding with a concurrently in-flight + * task's edits to that shared file. + * + * `createTestVacayHolidayCalendarsRepo` is here (not a vacay-owned helper + * file, which does not exist yet — Plan 3f Task 5 has not landed) because + * this task's own tests need to seed/read `vacay_holiday_calendars` rows to + * exercise SH14's cross-domain guard (`existsForSchoolRegion`); Task 5 may + * add its own, separate factory later without colliding with this one. + */ +export function createTestSchoolHolidayCountriesRepo(db: Database.Database): Promise { + return sharedTestOrm(db).then((t) => t.repo(SchoolHolidayCountries)); +} + +export function createTestSchoolHolidayRegionsRepo(db: Database.Database): Promise { + return sharedTestOrm(db).then((t) => t.repo(SchoolHolidayRegions)); +} + +export function createTestSchoolHolidayPeriodsRepo(db: Database.Database): Promise { + return sharedTestOrm(db).then((t) => t.repo(SchoolHolidayPeriods)); +} + +export function createTestVacayHolidayCalendarsRepo(db: Database.Database): Promise { + return sharedTestOrm(db).then((t) => t.repo(VacayHolidayCalendars)); +} diff --git a/server/tests/unit/nest/school-holidays.service.test.ts b/server/tests/unit/nest/school-holidays.service.test.ts new file mode 100644 index 000000000..a4ddcc8b8 --- /dev/null +++ b/server/tests/unit/nest/school-holidays.service.test.ts @@ -0,0 +1,199 @@ +/** + * Unit tests for the repository-backed SchoolHolidaysService (Plan 3f Task + * 2). The service is constructed directly (`new SchoolHolidaysService(...)`) + * over repositories resolved from the suite's own ORM + * (`tests/helpers/school-holidays-repos.ts`/`sharedTestOrm`), same pattern + * as `categories.service.test.ts` — no Nest container needed. A real + * in-memory SQLite DB so SQL logic (the `COLLATE NOCASE` duplicate-name + * guard, the optimistic-concurrency revision check, the cross-domain + * `vacay_holiday_calendars` guard) is exercised faithfully, and so the + * parity assertions below can run the SAME legacy raw statement this + * service replaced, on the same seeded rows, and compare full-key. + */ +import { describe, it, expect, beforeAll, beforeEach, afterAll } from 'vitest'; +import Database from 'better-sqlite3'; +import { ConflictException } from '@nestjs/common'; +import { createTables } from '../../../src/db/schema'; +import { runMigrations } from '../../../src/db/migrations'; +import { resetTestDb } from '../../helpers/test-db'; +import { createUser } from '../../helpers/factories'; +import { sharedTestOrm, createTestUnitOfWork } from '../../helpers/test-uow'; +import { + createTestSchoolHolidayCountriesRepo, createTestSchoolHolidayRegionsRepo, + createTestSchoolHolidayPeriodsRepo, createTestVacayHolidayCalendarsRepo, +} from '../../helpers/school-holidays-repos'; +import type { TestOrm } from '../../helpers/test-orm'; +import { SchoolHolidaysService } from '../../../src/nest/school-holidays/school-holidays.service'; + +const testDb = new Database(':memory:'); +testDb.exec('PRAGMA journal_mode = WAL'); +testDb.exec('PRAGMA foreign_keys = ON'); +testDb.exec('PRAGMA busy_timeout = 5000'); + +let t: TestOrm; +let svc: SchoolHolidaysService; + +beforeAll(async () => { + createTables(testDb); + runMigrations(testDb); + t = await sharedTestOrm(testDb); + svc = new SchoolHolidaysService( + await createTestSchoolHolidayCountriesRepo(testDb), + await createTestSchoolHolidayRegionsRepo(testDb), + await createTestSchoolHolidayPeriodsRepo(testDb), + await createTestVacayHolidayCalendarsRepo(testDb), + await createTestUnitOfWork(testDb), + ); +}); + +beforeEach(() => { + resetTestDb(testDb); + t.clear(); +}); + +afterAll(async () => { + await t.close(); + testDb.close(); +}); + +const winter = { name: 'Winter break', startDate: '2026-12-20', endDate: '2027-01-06' }; +// Overlaps `winter` (2026-12-24..2026-12-31 sits entirely inside +// 2026-12-20..2027-01-06) — the "overlapping holiday periods" fixture the +// brief's parity test asks for; both rows must survive the read unmerged and +// in the legacy `ORDER BY start_date, end_date, name` order. +const overlapping = { name: 'Regional break', startDate: '2026-12-24', endDate: '2026-12-31' }; + +async function seedRegion() { + await svc.createCountry({ code: 'US', name: 'USA' }); + return svc.createRegion('US', { name: 'Seattle schools', revision: 0, holidays: [winter, overlapping] }); +} + +// ── catalog / region — full-key parity against the legacy statements ────────── + +describe('catalog / region parity', () => { + it('SH-SVC-001: catalog() is full-key identical to the legacy SELECTs on fully seeded, overlapping-period rows', async () => { + await seedRegion(); + await svc.createCountry({ code: 'DE', name: 'Germany' }); + + const legacyCountries = testDb.prepare('SELECT code, name FROM school_holiday_countries ORDER BY name, code').all(); + const legacyRegions = testDb.prepare("SELECT *, country || '-MANUAL-' || id AS code FROM school_holiday_regions ORDER BY name, id").all(); + + expect(await svc.catalog()).toEqual({ countries: legacyCountries, regions: legacyRegions }); + }); + + it('SH-SVC-002: region() is full-key identical to the legacy region + periods SELECTs, including the synthesized code column', async () => { + const region = await seedRegion(); + + const legacyRegion = testDb.prepare("SELECT *, country || '-MANUAL-' || id AS code FROM school_holiday_regions WHERE id = ?").get(region.id); + const legacyHolidays = testDb + .prepare('SELECT name, start_date AS startDate, end_date AS endDate FROM school_holiday_periods WHERE region_id = ? ORDER BY start_date, end_date, name') + .all(region.id); + + expect(await svc.region(region.id)).toEqual({ ...(legacyRegion as object), holidays: legacyHolidays }); + // Both overlapping periods are present, unmerged, in the legacy sort order + // (`ORDER BY start_date, ...` — `winter` starts 2026-12-20, `overlapping` starts 2026-12-24). + expect(legacyHolidays).toEqual([winter, overlapping]); + }); + + it('SH-SVC-003: holidays() filters the same region+year-window periods as the legacy in-memory filter', async () => { + const region = await seedRegion(); + expect(await svc.holidays(region.id, '2026')).toEqual([winter, overlapping]); + expect(await svc.holidays(region.id, '2025')).toEqual([]); + }); +}); + +// ── checkName (SH9) — the collateNoCase duplicate-name guard ────────────────── + +describe('checkName (collateNoCase)', () => { + it('SH-SVC-004: a same-country, case-different name is rejected as a duplicate (COLLATE NOCASE, not lower())', async () => { + await seedRegion(); + await expect(svc.createRegion('US', { name: 'SEATTLE SCHOOLS', revision: 0, holidays: [] })).rejects.toThrow(ConflictException); + await expect(svc.createRegion('US', { name: 'seattle schools', revision: 0, holidays: [] })).rejects.toThrow('already exists'); + }); + + it('SH-SVC-005: a different country with the same name is not a duplicate', async () => { + await seedRegion(); + await svc.createCountry({ code: 'CA', name: 'Canada' }); + await expect(svc.createRegion('CA', { name: 'Seattle schools', revision: 0, holidays: [] })).resolves.toBeDefined(); + }); + + it("SH-SVC-006: renaming a region to its OWN current name is not a self-collision (id != ? excludes it)", async () => { + const region = await seedRegion(); + await expect(svc.updateRegion(region.id, { name: region.name, revision: 1, holidays: [] })).resolves.toBeDefined(); + }); +}); + +// ── updateRegion (SH13) — optimistic-concurrency ("lost update") guard ──────── + +describe('updateRegion optimistic concurrency', () => { + it('SH-SVC-007: a stale revision is rejected with ConflictException, and the row is unchanged', async () => { + const region = await seedRegion(); + await svc.updateRegion(region.id, { name: 'Renamed once', revision: 1, holidays: [] }); + await expect(svc.updateRegion(region.id, { name: 'Stale write', revision: 1, holidays: [winter] })).rejects.toThrow(ConflictException); + const current = await svc.region(region.id); + expect(current.name).toBe('Renamed once'); + expect(current.revision).toBe(2); + }); + + it('SH-SVC-008: two updates racing on the same region and revision — exactly one wins, the other sees ConflictException (lost-update proof)', async () => { + const region = await seedRegion(); + const results = await Promise.allSettled([ + svc.updateRegion(region.id, { name: 'Writer A', revision: 1, holidays: [] }), + svc.updateRegion(region.id, { name: 'Writer B', revision: 1, holidays: [] }), + ]); + const fulfilled = results.filter((r) => r.status === 'fulfilled'); + const rejected = results.filter((r) => r.status === 'rejected'); + expect(fulfilled).toHaveLength(1); + expect(rejected).toHaveLength(1); + expect((rejected[0] as PromiseRejectedResult).reason).toBeInstanceOf(ConflictException); + // The row landed at revision 2 exactly once — no double-increment, no silent overwrite. + expect((await svc.region(region.id)).revision).toBe(2); + }); +}); + +// ── deleteRegion (SH14) — the vacay_holiday_calendars cross-domain guard ────── + +describe('deleteRegion cross-domain integrity guard (existsForSchoolRegion)', () => { + it('SH-SVC-009: refused while a vacay_holiday_calendars row of type school_holiday still references the region', async () => { + const region = await seedRegion(); + const { user } = createUser(testDb); + const plan = testDb.prepare('INSERT INTO vacay_plans (owner_id) VALUES (?)').run(user.id); + testDb.prepare("INSERT INTO vacay_holiday_calendars (plan_id, type, region) VALUES (?, 'school_holiday', ?)").run(plan.lastInsertRowid, region.code); + + await expect(svc.deleteRegion(region.id, region.revision)).rejects.toThrow(ConflictException); + // Untouched — the region and its periods are still there. + expect(await svc.region(region.id)).toMatchObject({ id: region.id, revision: region.revision }); + }); + + it('SH-SVC-010: a calendar of a DIFFERENT type referencing the same code string does not block deletion (type is part of the guard)', async () => { + const region = await seedRegion(); + const { user } = createUser(testDb); + const plan = testDb.prepare('INSERT INTO vacay_plans (owner_id) VALUES (?)').run(user.id); + testDb.prepare("INSERT INTO vacay_holiday_calendars (plan_id, type, region) VALUES (?, 'public_holiday', ?)").run(plan.lastInsertRowid, region.code); + + await expect(svc.deleteRegion(region.id, region.revision)).resolves.toEqual({ success: true }); + }); + + it('SH-SVC-011: succeeds once no referencing calendar remains, and removes the region and its periods', async () => { + const region = await seedRegion(); + const { user } = createUser(testDb); + const plan = testDb.prepare('INSERT INTO vacay_plans (owner_id) VALUES (?)').run(user.id); + testDb.prepare("INSERT INTO vacay_holiday_calendars (plan_id, type, region) VALUES (?, 'school_holiday', ?)").run(plan.lastInsertRowid, region.code); + testDb.exec('DELETE FROM vacay_holiday_calendars'); + + await expect(svc.deleteRegion(region.id, region.revision)).resolves.toEqual({ success: true }); + expect(testDb.prepare('SELECT * FROM school_holiday_periods WHERE region_id = ?').all(region.id)).toEqual([]); + await expect(svc.region(region.id)).rejects.toThrow('not found'); + }); +}); + +// ── writePeriods (SH10/SH11) — replace-all stays atomic in the transaction ──── + +describe('writePeriods replace-all', () => { + it('SH-SVC-012: updateRegion replaces the complete period set, not a merge', async () => { + const region = await seedRegion(); + const replaced = { name: 'Spring break', startDate: '2027-03-20', endDate: '2027-03-28' }; + await svc.updateRegion(region.id, { name: region.name, revision: 1, holidays: [replaced] }); + expect((await svc.region(region.id)).holidays).toEqual([replaced]); + }); +});