diff --git a/server/scripts/size-baseline.json b/server/scripts/size-baseline.json index 1d8837261..68ded4957 100644 --- a/server/scripts/size-baseline.json +++ b/server/scripts/size-baseline.json @@ -7,17 +7,17 @@ "src/db/repositories/Users.repository.ts": 1710, "src/nest/atlas/atlas-geo.ts": 1005, "src/nest/atlas/atlas.service.ts": 1182, - "src/nest/auth/auth.service.ts": 1166, + "src/nest/auth/auth.service.ts": 1165, "src/nest/budget/budget.service.ts": 1703, "src/nest/collections/collections.service.ts": 1488, "src/nest/doc-sync/doc-sync.service.ts": 1060, "src/nest/doc-sync/providers/synology-drive.client.ts": 1160, "src/nest/journey/journey-domain.service.ts": 1804, "src/nest/maps/maps.service.ts": 3252, - "src/nest/places/places.service.ts": 2022, + "src/nest/places/places.service.ts": 2020, "src/nest/plugins/plugin-runtime.service.ts": 1463, "src/nest/plugins/runtime/plugin-sdk.ts": 1094, "src/nest/reservations/reservations.service.ts": 1248, - "src/nest/trips/trips.service.ts": 1014, - "src/nest/vacay/vacay.service.ts": 1442 + "src/nest/trips/trips.service.ts": 1013, + "src/nest/vacay/vacay.service.ts": 1440 } diff --git a/server/src/nest/accommodations/accommodations.service.ts b/server/src/nest/accommodations/accommodations.service.ts index 1eb02e5bf..641c50cb8 100644 --- a/server/src/nest/accommodations/accommodations.service.ts +++ b/server/src/nest/accommodations/accommodations.service.ts @@ -619,8 +619,8 @@ export class AccommodationsService { const newConfirmation = fields.confirmation !== undefined ? fields.confirmation : existing.confirmation; const newNotes = fields.notes !== undefined ? fields.notes : existing.notes; - // The stay row and the day stop that mirrors it describe the same booking, so a - // move that wrote only one of the two must not survive. + // The stay row, the day stop that mirrors it and the linked bookings' times all + // describe the same booking, so a write that landed only some of them must not survive. const mirror = await this.uow.transactional(async () => { // AC36 — `existing.id`, the SAME id AC34's gate (`getAccommodation`) // already resolved (rule 21): the caller always reads `existing` from @@ -636,25 +636,24 @@ export class AccommodationsService { confirmation: newConfirmation, notes: newNotes, }); - return await this.remirrorStay(existing.id, newPlaceId, newStartDayId, newCheckIn, { + const moved = await this.remirrorStay(existing.id, newPlaceId, newStartDayId, newCheckIn, { checkInChanged: fields.check_in !== undefined && (fields.check_in || null) !== (existing.check_in || null), }); - }); - // Sync check-in/out/confirmation to every linked reservation. The booking form - // lets more than one hotel booking point at the same block and there is no - // unique constraint on reservations.accommodation_id, so a single .get() would - // silently leave the others on the old times. - // AC37 — runs AFTER the transaction above commits (§18.6, R5 class: pre-existing, flagged not fixed). - const linkedRes = await this.reservationsRepo.listIdMetadataByStay(existing.id); - for (const res of linkedRes) { - const meta = res.metadata ? JSON.parse(res.metadata) : {}; - if (newCheckIn) meta.check_in_time = newCheckIn; - if (newCheckInEnd) meta.check_in_end_time = newCheckInEnd; - if (newCheckOut) meta.check_out_time = newCheckOut; - // AC38 - await this.reservationsRepo.setMetadataAndConfirmation(res.id, JSON.stringify(meta), newConfirmation || null); - } + // Sync check-in/out/confirmation to every linked reservation (AC37/AC38). The + // booking form lets more than one hotel booking point at the same block and there + // is no unique constraint on reservations.accommodation_id, so a single .get() + // would silently leave the others on the old times. + const linkedRes = await this.reservationsRepo.listIdMetadataByStay(existing.id); + for (const res of linkedRes) { + const meta = res.metadata ? JSON.parse(res.metadata) : {}; + if (newCheckIn) meta.check_in_time = newCheckIn; + if (newCheckInEnd) meta.check_in_end_time = newCheckInEnd; + if (newCheckOut) meta.check_out_time = newCheckOut; + await this.reservationsRepo.setMetadataAndConfirmation(res.id, JSON.stringify(meta), newConfirmation || null); + } + return moved; + }); return { accommodation: await this.getAccommodationWithPlace(existing.id), mirror }; } diff --git a/server/src/nest/addons/addons.service.ts b/server/src/nest/addons/addons.service.ts index 4e1abf13a..4b558a8ca 100644 --- a/server/src/nest/addons/addons.service.ts +++ b/server/src/nest/addons/addons.service.ts @@ -16,6 +16,7 @@ import { AppSettings } from '../../db/entities/AppSettings.entity'; import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository'; import { Users } from '../../db/entities/Users.entity'; import type { UsersRepository } from '../../db/repositories/Users.repository'; +import { UnitOfWork } from '../database/unit-of-work'; /** * Thin wrapper around the enabled-addons + photo-provider read that the legacy @@ -45,6 +46,7 @@ export class AddonsService { @InjectRepository(PhotoProviderFields) private readonly photoProviderFields: PhotoProviderFieldsRepository, @InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository, @InjectRepository(Users) private readonly users: UsersRepository, + private readonly uow: UnitOfWork, ) {} async isAddonEnabled(addonId: string): Promise { @@ -88,13 +90,7 @@ export class AddonsService { }; } - /** - * Ruling (plan §"Do not fix legacy behaviour"): the legacy handler wrote up - * to 5 `app_settings` rows one at a time, with no transaction around them — - * this stays exactly that way. `AppSettingsRepository.setValue` imposes no - * transaction of its own (Task 0 concern #5), so the loop below is still 0 - * to 5 independent, individually-committed upserts. - */ + /** Up to five `app_settings` rows, written together: a save lands whole or not at all. */ async updateCollabFeatures(features: { chat?: boolean; notes?: boolean; links?: boolean; polls?: boolean; whatsnext?: boolean }) { const mapping: Record = { chat: 'collab_chat_enabled', @@ -104,10 +100,12 @@ export class AddonsService { whatsnext: 'collab_whatsnext_enabled', }; const before = await this.getCollabFeatures(); - for (const [feat, key] of Object.entries(mapping)) { - const value = features[feat as keyof typeof features]; - if (value !== undefined) await this.appSettings.setValue(key, value ? 'true' : 'false'); - } + await this.uow.transactional(async () => { + for (const [feat, key] of Object.entries(mapping)) { + const value = features[feat as keyof typeof features]; + if (value !== undefined) await this.appSettings.setValue(key, value ? 'true' : 'false'); + } + }); const after = await this.getCollabFeatures(); // Collab flags gate MCP tool/resource registration, so callers must know // whether anything actually flipped — a no-op save must not tear down every diff --git a/server/src/nest/airports/airports.service.ts b/server/src/nest/airports/airports.service.ts index a8207a6f3..d9f4c5d7a 100644 --- a/server/src/nest/airports/airports.service.ts +++ b/server/src/nest/airports/airports.service.ts @@ -7,6 +7,7 @@ import { ReservationsRepository } from '../../db/repositories/Reservations.repos import { ReservationEndpoints } from '../../db/entities/ReservationEndpoints.entity'; import { ReservationEndpointsRepository } from '../../db/repositories/ReservationEndpoints.repository'; import { CronRegistrarService } from '../scheduling/cron-registrar.service'; +import { UnitOfWork } from '../database/unit-of-work'; /** * The in-container face of the airport dataset, plus the flight-endpoint @@ -42,6 +43,7 @@ export class AirportsService implements OnApplicationBootstrap { @InjectRepository(Reservations) private readonly reservationsRepo: ReservationsRepository, @InjectRepository(ReservationEndpoints) private readonly endpointsRepo: ReservationEndpointsRepository, private readonly registrar: CronRegistrarService, + private readonly uow: UnitOfWork, ) {} async onApplicationBootstrap(): Promise { @@ -66,6 +68,14 @@ export class AirportsService implements OnApplicationBootstrap { return findByIata(code) as Airport | null; } + /** + * Endpoints for every flight booked before they existed, from the IATA codes in + * its metadata; a flight without usable codes is flagged for review instead. + * + * @txIndependent one transaction per flight: its two endpoints land together or + * not at all (half a pair hid the flight from every later run), and a flight that + * failed is picked up again on the next boot. + */ async backfillFlightEndpoints(): Promise { const pending = await this.reservationsRepo.listFlightsMissingEndpoints(); @@ -76,36 +86,42 @@ export class AirportsService implements OnApplicationBootstrap { let filled = 0; let flagged = 0; for (const r of pending) { - if (!r.metadata) { await this.reservationsRepo.markNeedsReview(r.id); flagged++; continue; } - let meta: any; - try { meta = JSON.parse(r.metadata); } catch { await this.reservationsRepo.markNeedsReview(r.id); flagged++; continue; } - - const dep = meta.departure_airport ? findByIata(String(meta.departure_airport).slice(0, 3)) : null; - const arr = meta.arrival_airport ? findByIata(String(meta.arrival_airport).slice(0, 3)) : null; - - if (!dep || !arr) { await this.reservationsRepo.markNeedsReview(r.id); flagged++; continue; } - - const split = (iso: string | null) => { - if (!iso) return { date: null as string | null, time: null as string | null }; - const [date, time] = iso.split('T'); - return { date: date || null, time: time ? time.slice(0, 5) : null }; - }; - const depParts = split(r.reservation_time); - const arrParts = split(r.reservation_end_time); - - await this.endpointsRepo.insertEndpoint({ - reservation_id: r.id, role: 'from', sequence: 0, - name: dep.city ? `${dep.city} (${dep.iata})` : dep.name, code: dep.iata, - lat: dep.lat, lng: dep.lng, timezone: dep.tz, local_time: depParts.time, local_date: depParts.date, - }); - await this.endpointsRepo.insertEndpoint({ - reservation_id: r.id, role: 'to', sequence: 1, - name: arr.city ? `${arr.city} (${arr.iata})` : arr.name, code: arr.iata, - lat: arr.lat, lng: arr.lng, timezone: arr.tz, local_time: arrParts.time, local_date: arrParts.date, - }); - filled++; + if (await this.uow.transactional(() => this.backfillOne(r))) filled++; + else flagged++; } console.log(`[airports] Backfill: ${filled} filled, ${flagged} flagged for review`); } + + /** One flight's endpoints, or its review flag. True when it was filled. */ + private async backfillOne(r: { id: number; metadata: string | null; reservation_time: string | null; reservation_end_time: string | null }): Promise { + if (!r.metadata) { await this.reservationsRepo.markNeedsReview(r.id); return false; } + let meta: any; + try { meta = JSON.parse(r.metadata); } catch { await this.reservationsRepo.markNeedsReview(r.id); return false; } + + const dep = meta.departure_airport ? findByIata(String(meta.departure_airport).slice(0, 3)) : null; + const arr = meta.arrival_airport ? findByIata(String(meta.arrival_airport).slice(0, 3)) : null; + + if (!dep || !arr) { await this.reservationsRepo.markNeedsReview(r.id); return false; } + + const split = (iso: string | null) => { + if (!iso) return { date: null as string | null, time: null as string | null }; + const [date, time] = iso.split('T'); + return { date: date || null, time: time ? time.slice(0, 5) : null }; + }; + const depParts = split(r.reservation_time); + const arrParts = split(r.reservation_end_time); + + await this.endpointsRepo.insertEndpoint({ + reservation_id: r.id, role: 'from', sequence: 0, + name: dep.city ? `${dep.city} (${dep.iata})` : dep.name, code: dep.iata, + lat: dep.lat, lng: dep.lng, timezone: dep.tz, local_time: depParts.time, local_date: depParts.date, + }); + await this.endpointsRepo.insertEndpoint({ + reservation_id: r.id, role: 'to', sequence: 1, + name: arr.city ? `${arr.city} (${arr.iata})` : arr.name, code: arr.iata, + lat: arr.lat, lng: arr.lng, timezone: arr.tz, local_time: arrParts.time, local_date: arrParts.date, + }); + return true; + } } diff --git a/server/src/nest/auth/auth.service.ts b/server/src/nest/auth/auth.service.ts index 92634f1f1..78e7cdcbf 100644 --- a/server/src/nest/auth/auth.service.ts +++ b/server/src/nest/auth/auth.service.ts @@ -437,11 +437,8 @@ export class AuthService { if (!invite) return { error: 'Invalid invite link', status: 404 }; if (invite.max_uses > 0 && invite.used_count >= invite.max_uses) return { error: 'Invite link has been fully used', status: 410 }; if (invite.expires_at && new Date(invite.expires_at) < new Date()) return { error: 'Invite link has expired', status: 410 }; - // Rule 16 (nullable columns stay null on the wire, docs/superpowers/plans/ - // 2026-09-21-orm-migration-program.md:45; task-5-review-security.md F1 / - // task-5-review-template.md T1): pass the repository's `string | null` - // through unchanged — `?? undefined` made JSON.stringify drop the key - // entirely for a never-expiring invite instead of emitting `null`. + // A nullable column stays null on the wire: `?? undefined` would make + // JSON.stringify drop the key for a never-expiring invite. return { valid: true, max_uses: invite.max_uses, used_count: invite.used_count, expires_at: invite.expires_at }; } @@ -777,26 +774,28 @@ export class AuthService { // the whole form in one request. const { blocked } = splitManagedKeys(body as Record, readEnv().managed.enabled); - for (const key of ADMIN_SETTINGS_KEYS) { - if (blocked.includes(key)) continue; - if (body[key] !== undefined) { - let val = String(body[key]); - if (key === 'require_mfa') { - val = body[key] === true || val === 'true' ? 'true' : 'false'; + await this.uow.transactional(async () => { // the whole form lands, or none of it + for (const key of ADMIN_SETTINGS_KEYS) { + if (blocked.includes(key)) continue; + if (body[key] !== undefined) { + let val = String(body[key]); + if (key === 'require_mfa') { + val = body[key] === true || val === 'true' ? 'true' : 'false'; + } + // An unknown provider name is dropped, not stored: the maps service + // degrades an unrecognised row to 'auto', so writing one would show the + // admin a saved setting that quietly does nothing. + if (key === 'places_provider' && !isPlacesProviderChoice(val)) continue; + if (key === 'smtp_pass' && val === '••••••••') continue; + if (key === 'smtp_pass') val = encrypt_api_key(val); + if (key === 'admin_webhook_url' && val === '••••••••') continue; + if (key === 'admin_webhook_url' && val) val = maybe_encrypt_api_key(val) ?? val; + if (key === 'admin_ntfy_token' && val === '••••••••') continue; + if (key === 'admin_ntfy_token' && val) val = maybe_encrypt_api_key(val) ?? val; + await this.appSettings.setValue(key, val); } - // An unknown provider name is dropped, not stored: the maps service - // degrades an unrecognised row to 'auto', so writing one would show the - // admin a saved setting that quietly does nothing. - if (key === 'places_provider' && !isPlacesProviderChoice(val)) continue; - if (key === 'smtp_pass' && val === '••••••••') continue; - if (key === 'smtp_pass') val = encrypt_api_key(val); - if (key === 'admin_webhook_url' && val === '••••••••') continue; - if (key === 'admin_webhook_url' && val) val = maybe_encrypt_api_key(val) ?? val; - if (key === 'admin_ntfy_token' && val === '••••••••') continue; - if (key === 'admin_ntfy_token' && val) val = maybe_encrypt_api_key(val) ?? val; - await this.appSettings.setValue(key, val); } - } + }); const changedKeys = ADMIN_SETTINGS_KEYS.filter(k => !blocked.includes(k) && body[k] !== undefined && !(k === 'smtp_pass' && String(body[k]) === '••••••••')); @@ -1010,15 +1009,15 @@ export class AuthService { return { tokenForDelivery: null, userId: user.id, userEmail: user.email, reason: 'oidc_only' }; } - // Invalidate any prior unconsumed tokens for this user so there is - // always at most one live reset link in flight. - await this.passwordResetTokens.consumeAllLiveForUser(user.id); - const raw = randomBytes(PASSWORD_RESET_TOKEN_BYTES).toString('base64url'); const token_hash = hashResetToken(raw); const expires_at = new Date(Date.now() + PASSWORD_RESET_TTL_MS).toISOString(); - - await this.passwordResetTokens.insertToken({ user_id: user.id, token_hash, expires_at, created_ip: createdIp }); + // Invalidate any prior unconsumed tokens for this user and issue the new one + // together, so there is always exactly one live reset link in flight. + await this.uow.transactional(async () => { + await this.passwordResetTokens.consumeAllLiveForUser(user.id); + await this.passwordResetTokens.insertToken({ user_id: user.id, token_hash, expires_at, created_ip: createdIp }); + }); return { tokenForDelivery: raw, userId: user.id, userEmail: user.email, reason: 'issued' }; } diff --git a/server/src/nest/memories/unified-memories.service.ts b/server/src/nest/memories/unified-memories.service.ts index 5985baeae..bdb83111e 100644 --- a/server/src/nest/memories/unified-memories.service.ts +++ b/server/src/nest/memories/unified-memories.service.ts @@ -130,13 +130,16 @@ export class UnifiedMemoriesService { return providerResult as ServiceResult; } try { - const photoId = await this.photos.getOrCreate(provider, assetId, userId, passphrase); - const added = await this.tripPhotos.insertIgnore({ - trip_id: tripId, - user_id: userId, - photo_id: photoId, - shared: shared ? 1 : 0, - album_link_id: albumLinkId || null, + // The photo row and its trip link together: no registered photo without its link. + const added = await this.uow.transactional(async () => { + const photoId = await this.photos.getOrCreate(provider, assetId, userId, passphrase); + return await this.tripPhotos.insertIgnore({ + trip_id: tripId, + user_id: userId, + photo_id: photoId, + shared: shared ? 1 : 0, + album_link_id: albumLinkId || null, + }); }); return success(added); } @@ -222,9 +225,10 @@ export class UnifiedMemoriesService { } try { - await this.tripPhotos.deleteForUserPhoto(tripId, userId, photoId); - - await this.photos.deleteIfOrphan(photoId); + await this.uow.transactional(async () => { + await this.tripPhotos.deleteForUserPhoto(tripId, userId, photoId); + await this.photos.deleteIfOrphan(photoId); + }); this.realtime.broadcast(tripId, 'memories:updated', { userId }, sid); return success(true); @@ -292,12 +296,9 @@ export class UnifiedMemoriesService { await this.uow.transactional(async () => { await this.tripPhotos.deleteForAlbumLink(tripId, linkId); await this.tripAlbumLinks.deleteScoped(linkId, tripId, userId); + for (const photo_id of linkedPhotoIds) await this.photos.deleteIfOrphan(photo_id); }); - for (const photo_id of linkedPhotoIds) { - await this.photos.deleteIfOrphan(photo_id); - } - return success(true); } catch (error) { return mapDbError(error, 'Failed to remove album link'); diff --git a/server/src/nest/notifications/notification-preferences.service.ts b/server/src/nest/notifications/notification-preferences.service.ts index 859c37cc2..25377e313 100644 --- a/server/src/nest/notifications/notification-preferences.service.ts +++ b/server/src/nest/notifications/notification-preferences.service.ts @@ -370,21 +370,16 @@ export class NotificationPreferencesService { } } - // Apply global prefs outside the transaction (they write to app_settings). - // Mixed atomicity, preserved as-is (plan3f-sql-inventory.md §4b's flag, R10): - // these are single-key upserts with no cross-row invariant, so a partial - // failure here is low-risk — widening this into the transaction below is a - // ruling this task does not make on its own. - for (const [eventType, channels] of Object.entries(globalPrefs)) { - if (!channels) continue; - for (const [channel, enabled] of Object.entries(channels)) { - if (!isAdminGlobalChannel(channel)) continue; - await this.setAdminGlobalPref(eventType as NotifEventType, channel, enabled); - } - } - - // Apply per-user (inapp) prefs in a transaction + // The global rows (app_settings) and the admin's own in-app rows are one save: + // a failure halfway used to keep the email/webhook half of a form. await this.uow.transactional(async () => { + for (const [eventType, channels] of Object.entries(globalPrefs)) { + if (!channels) continue; + for (const [channel, enabled] of Object.entries(channels)) { + if (!isAdminGlobalChannel(channel)) continue; + await this.setAdminGlobalPref(eventType as NotifEventType, channel, enabled); + } + } await this.applyUserChannelPrefs(userId, userPrefs); }); } diff --git a/server/src/nest/oauth/oauth.service.ts b/server/src/nest/oauth/oauth.service.ts index b9d8b4db2..a5e06af14 100644 --- a/server/src/nest/oauth/oauth.service.ts +++ b/server/src/nest/oauth/oauth.service.ts @@ -21,6 +21,7 @@ import { OauthTokens } from '../../db/entities/OauthTokens.entity'; import type { OauthTokenRefreshRow, OauthTokensRepository } from '../../db/repositories/OauthTokens.repository'; import { OauthConsents } from '../../db/entities/OauthConsents.entity'; import type { OauthConsentsRepository } from '../../db/repositories/OauthConsents.repository'; +import { UnitOfWork } from '../database/unit-of-work'; import { ACCESS_TOKEN_TTL_S, CODE_CHALLENGE_RE, @@ -89,6 +90,7 @@ export class OauthService { @InjectRepository(OauthConsents) private readonly consents: OauthConsentsRepository, private readonly addons: AddonsService, private readonly audit: AuditService, + private readonly uow: UnitOfWork, private readonly pendingCodes: PendingCodeStore = pendingCodesSlot.get(), ) {} @@ -202,10 +204,12 @@ export class OauthService { const rawSecret = 'trekcs_' + randomBytes(24).toString('hex'); const secretHash = hashToken(rawSecret); - await this.clients.updateSecretHash(clientRowId, secretHash); - - // Revoke all existing tokens for this client so old sessions are invalidated - await this.tokens.revokeAllForClient(row.client_id); + // The new secret and the revocation of every token issued under the old one + // land together, so a failure never leaves old sessions alive on a new secret. + await this.uow.transactional(async () => { + await this.clients.updateSecretHash(clientRowId, secretHash); + await this.tokens.revokeAllForClient(row.client_id); + }); // Terminate active MCP sessions for this (user, client) pair revokeUserSessionsForClient(userId, row.client_id); @@ -499,14 +503,12 @@ export class OauthService { // request. Killing the session on every routine hourly refresh broke long-lived // MCP connections (#1475). // - // Stays NON-transactional with the issueTokens() call below — legacy - // parity (Task 4 brief ruling, §9.6 of the inventory): revoke-old then - // issue-new are two separate statements, not one transaction. A crash - // between the two would leave the old token revoked with no successor — - // the same window the legacy code always had. - await this.tokens.revokeById(row.id); - - const tokens = await this.issueTokens(clientId, row.user_id, JSON.parse(row.scopes), row.id, row.audience ?? null); + // Revoke-old and issue-new are one transaction: a failure between the two used + // to leave the old token revoked with no successor, logging the client out. + const tokens = await this.uow.transactional(async () => { + await this.tokens.revokeById(row.id); + return await this.issueTokens(clientId, row.user_id, JSON.parse(row.scopes), row.id, row.audience ?? null); + }); await this.audit.writeAudit({ userId: row.user_id, action: 'oauth.token.refresh', details: { client_id: clientId }, ip }); return { tokens }; diff --git a/server/src/nest/oidc/oidc.service.ts b/server/src/nest/oidc/oidc.service.ts index 4f8089f41..bba41b88c 100644 --- a/server/src/nest/oidc/oidc.service.ts +++ b/server/src/nest/oidc/oidc.service.ts @@ -638,6 +638,78 @@ export class OidcService implements OnModuleDestroy { return userInfo.name || userInfo.preferred_username; } + /** + * A login by an account that already exists: link the OIDC identity when the + * verified email matched it, follow the role claim, keep the OIDC avatar current. + * Runs inside findOrCreateUser's transaction. + */ + private async refreshLinkedUser( + user: UserRow, + sub: string, + config: OidcConfig, + userInfo: OidcUserInfo, + picture: string | null, + ): Promise<{ user: User; roleChange?: OidcRoleChange } | { error: string }> { + // Reaching here without an oidc_sub means we matched an existing local + // account by email. Only auto-link the OIDC identity when the IdP asserts + // the email is verified; an unverified email must not auto-link. + if (!user.oidc_sub) { + const emailVerified = userInfo.email_verified === true || userInfo.email_verified === 'true'; + if (!emailVerified) { + return { error: 'email_not_verified' }; + } + await this.usersRepo.linkOidcIdentity(user.id, sub, config.issuer); + user = { ...user, oidc_sub: sub, oidc_issuer: config.issuer }; + } else if (user.oidc_issuer !== config.issuer || user.oidc_sub !== sub) { + // The admin pointed the instance at a different IdP. We got here through the + // verified-email lookup, so this is the same person arriving from the new + // provider; leaving the old sub and issuer on the row would keep the account + // pinned to a provider that no longer exists (#2110). + await this.usersRepo.linkOidcIdentity(user.id, sub, config.issuer); + user = { ...user, oidc_sub: sub, oidc_issuer: config.issuer }; + } + // Update role based on OIDC claims on every login (if claim mapping is configured) + let roleChange: OidcRoleChange | undefined; + if (readEnv().oidc.adminValue) { + const resolution = this.resolveOidcRoleDetailed(userInfo, false); + const newRole = resolution.role; + if (resolution.claimMissing) { + this.warnMissingAdminClaim(resolution, { id: user.id, username: user.username, role: user.role === 'admin' ? 'admin' : 'user' }); + } else if (user.role !== newRole) { + // Never let the claim-based downgrade strip the last admin. The bootstrap + // admin (first SSO user) usually doesn't carry the admin claim, so a forced + // re-login — e.g. after a JWT-secret rotation — would otherwise demote it and + // lock an OIDC-only instance out for good. #1274 + const demotingLastAdmin = + user.role === 'admin' && + newRole !== 'admin' && + (await this.usersRepo.countAdmins()) <= 1; + if (demotingLastAdmin) { + console.warn(`[OIDC] Kept admin role for user ${user.id}: their OIDC claims map to '${newRole}', but they are the only admin — demoting would lock the instance out.`); + } else { + await this.usersRepo.setRole(user.id, newRole); + roleChange = { from: user.role === 'admin' ? 'admin' : 'user', to: newRole, claim: resolution.claimKey }; + user = { ...user, role: newRole }; + } + } + } + // Keep the avatar in sync with the OIDC picture, but never clobber a custom + // upload: only touch it when empty or when the current value is itself an OIDC + // picture URL, so the picture refreshes on each login without overriding an + // uploaded one. #1399 + // + // "In sync" includes the provider having no picture for this user any more. That + // is what a provider switch looks like from here, and the old value points at a + // host this instance no longer talks to, so it renders as a broken image forever + // (#2110). An uploaded avatar is a bare filename and stays untouched either way. + const avatarIsOidc = !!user.avatar && /^https:\/\//i.test(user.avatar); + if (picture ? picture !== user.avatar && (!user.avatar || avatarIsOidc) : avatarIsOidc) { + await this.usersRepo.setAvatarRaw(user.id, picture); + user = { ...user, avatar: picture }; + } + return { user: toClientUser(user), roleChange }; + } + async findOrCreateUser( userInfo: OidcUserInfo, config: OidcConfig, @@ -663,64 +735,9 @@ export class OidcService implements OnModuleDestroy { } if (user) { - // Reaching here without an oidc_sub means we matched an existing local - // account by email. Only auto-link the OIDC identity when the IdP asserts - // the email is verified; an unverified email must not auto-link. - if (!user.oidc_sub) { - const emailVerified = userInfo.email_verified === true || userInfo.email_verified === 'true'; - if (!emailVerified) { - return { error: 'email_not_verified' }; - } - await this.usersRepo.linkOidcIdentity(user.id, sub, config.issuer); - user = { ...user, oidc_sub: sub, oidc_issuer: config.issuer }; - } else if (user.oidc_issuer !== config.issuer || user.oidc_sub !== sub) { - // The admin pointed the instance at a different IdP. We got here through the - // verified-email lookup, so this is the same person arriving from the new - // provider; leaving the old sub and issuer on the row would keep the account - // pinned to a provider that no longer exists (#2110). - await this.usersRepo.linkOidcIdentity(user.id, sub, config.issuer); - user = { ...user, oidc_sub: sub, oidc_issuer: config.issuer }; - } - // Update role based on OIDC claims on every login (if claim mapping is configured) - let roleChange: OidcRoleChange | undefined; - if (readEnv().oidc.adminValue) { - const resolution = this.resolveOidcRoleDetailed(userInfo, false); - const newRole = resolution.role; - if (resolution.claimMissing) { - this.warnMissingAdminClaim(resolution, { id: user.id, username: user.username, role: user.role === 'admin' ? 'admin' : 'user' }); - } else if (user.role !== newRole) { - // Never let the claim-based downgrade strip the last admin. The bootstrap - // admin (first SSO user) usually doesn't carry the admin claim, so a forced - // re-login — e.g. after a JWT-secret rotation — would otherwise demote it and - // lock an OIDC-only instance out for good. #1274 - const demotingLastAdmin = - user.role === 'admin' && - newRole !== 'admin' && - (await this.usersRepo.countAdmins()) <= 1; - if (demotingLastAdmin) { - console.warn(`[OIDC] Kept admin role for user ${user.id}: their OIDC claims map to '${newRole}', but they are the only admin — demoting would lock the instance out.`); - } else { - await this.usersRepo.setRole(user.id, newRole); - roleChange = { from: user.role === 'admin' ? 'admin' : 'user', to: newRole, claim: resolution.claimKey }; - user = { ...user, role: newRole }; - } - } - } - // Keep the avatar in sync with the OIDC picture, but never clobber a custom - // upload: only touch it when empty or when the current value is itself an OIDC - // picture URL, so the picture refreshes on each login without overriding an - // uploaded one. #1399 - // - // "In sync" includes the provider having no picture for this user any more. That - // is what a provider switch looks like from here, and the old value points at a - // host this instance no longer talks to, so it renders as a broken image forever - // (#2110). An uploaded avatar is a bare filename and stays untouched either way. - const avatarIsOidc = !!user.avatar && /^https:\/\//i.test(user.avatar); - if (picture ? picture !== user.avatar && (!user.avatar || avatarIsOidc) : avatarIsOidc) { - await this.usersRepo.setAvatarRaw(user.id, picture); - user = { ...user, avatar: picture }; - } - return { user: toClientUser(user), roleChange }; + // The identity link, the role from the claims and the avatar are one write. + const existing = user; + return await this.uow.transactional(() => this.refreshLinkedUser(existing, sub, config, userInfo, picture)); } // --- New user registration --- diff --git a/server/src/nest/places/places.service.ts b/server/src/nest/places/places.service.ts index a80b5b6aa..e43c5d933 100644 --- a/server/src/nest/places/places.service.ts +++ b/server/src/nest/places/places.service.ts @@ -393,43 +393,42 @@ export class PlacesService { // the value as a stay, and a stop added from the corridor search brings its own // figure — ten minutes for fuel, twenty for a rest area — so the kinds that would // be misread as an hour never take the default in the first place. - const placeId = await this.placesRepo.insertPlace({ - trip_id: tid, - name, - description: description || null, - lat: lat ?? null, - lng: lng ?? null, - address: address || null, - category_id: category_id || null, - price: price ?? null, - currency: currency || null, - place_time: place_time || null, - end_time: end_time || null, - duration_minutes: duration_minutes ?? 60, - notes: notes || null, - image_url: image_url || null, - google_place_id: google_place_id || null, - google_ftid: google_ftid || null, - osm_id: osm_id || null, - amap_poi_id: amap_poi_id || null, - website: website || null, - phone: phone || null, - transport_mode: transport_mode || 'walking', - route_geometry: route_geometry || null, - route_color: route_color || null, - stop_type: stop_type || null, - fill_percent: fill_percent ?? null, - email: email?.trim() || null, - opening_hours: opening_hours || null, + const placeId = await this.uow.transactional(async () => { + const id = await this.placesRepo.insertPlace({ + trip_id: tid, + name, + description: description || null, + lat: lat ?? null, + lng: lng ?? null, + address: address || null, + category_id: category_id || null, + price: price ?? null, + currency: currency || null, + place_time: place_time || null, + end_time: end_time || null, + duration_minutes: duration_minutes ?? 60, + notes: notes || null, + image_url: image_url || null, + google_place_id: google_place_id || null, + google_ftid: google_ftid || null, + osm_id: osm_id || null, + amap_poi_id: amap_poi_id || null, + website: website || null, + phone: phone || null, + transport_mode: transport_mode || 'walking', + route_geometry: route_geometry || null, + route_color: route_color || null, + stop_type: stop_type || null, + fill_percent: fill_percent ?? null, + email: email?.trim() || null, + opening_hours: opening_hours || null, + }); + + // PL5: the row and its tags are one write. + if (tags && tags.length > 0) await this.tagsRepo.insertIgnore(id, await this.tagsOnTrip(tid, tags)); + return id; }); - - // PL5 — `INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES (?, ?)`. - if (tags && tags.length > 0) { - await this.tagsRepo.insertIgnore(placeId, await this.tagsOnTrip(tid, tags)); - } - - // PL6 — `findWithTagsAndRatings` replaces the `getPlaceWithTags` delegation. - return (await this.placesRepo.findWithTagsAndRatings(placeId))!; + return (await this.placesRepo.findWithTagsAndRatings(placeId))!; // PL6 } // ------------------------------------------------------------------------- @@ -453,8 +452,7 @@ export class PlacesService { const tid = toRowId(tripId); if (tid === null) return null; if (!(await this.placesRepo.existsInTrip(id, tid))) return null; - // PL8 — `findWithTagsAndRatings` replaces the `getPlaceWithTags` delegation. - return await this.placesRepo.findWithTagsAndRatings(id); + return await this.placesRepo.findWithTagsAndRatings(id); // PL8 } // ------------------------------------------------------------------------- @@ -467,7 +465,7 @@ export class PlacesService { body: PlaceUpdateInput, ifMatch?: string, ): Promise { - const { result, reclaim } = await this.applyUpdate(tripId, placeId, body, ifMatch); + const { result, reclaim } = await this.uow.transactional(() => this.applyUpdate(tripId, placeId, body, ifMatch)); if (reclaim !== undefined) await this.reclaimPlaceImage(reclaim); return result; } @@ -571,12 +569,8 @@ export class PlacesService { }); if (tags !== undefined) { - // PL12 — `DELETE FROM place_tags WHERE place_id = ?`. - await this.tagsRepo.deleteForPlace(id); - if (tags.length > 0) { - // PL13 — `INSERT OR IGNORE INTO place_tags (place_id, tag_id) VALUES (?, ?)`. - await this.tagsRepo.insertIgnore(id, await this.tagsOnTrip(tid, tags)); - } + await this.tagsRepo.deleteForPlace(id); // PL12 + if (tags.length > 0) await this.tagsRepo.insertIgnore(id, await this.tagsOnTrip(tid, tags)); // PL13 } // A custom uploaded thumbnail (#1136) that was just replaced or cleared leaves @@ -586,8 +580,7 @@ export class PlacesService { ? existingPlace.image_url : undefined; - // PL14 — `findWithTagsAndRatings` replaces the `getPlaceWithTags` delegation. - return { result: await this.placesRepo.findWithTagsAndRatings(id), reclaim }; + return { result: await this.placesRepo.findWithTagsAndRatings(id), reclaim }; // PL14 } // ------------------------------------------------------------------------- @@ -864,9 +857,12 @@ export class PlacesService { // ------------------------------------------------------------------------- async importGpx(tripId: string, fileBuffer: Buffer, opts: GpxImportOptions = {}): Promise { - const result = await this.importGpxRows(tripId, fileBuffer, opts); - await this.colorizeImportedTracks(tripId, result); - return result; + // The rows and their track colours are one write, as for KML below. + return await this.uow.transactional(async () => { + const result = await this.importGpxRows(tripId, fileBuffer, opts); + await this.colorizeImportedTracks(tripId, result); + return result; + }); } /** @@ -1076,9 +1072,11 @@ export class PlacesService { // ------------------------------------------------------------------------- async importMapFile(tripId: string, fileBuffer: Buffer, filename: string, opts: KmlImportOptions = {}): Promise { - const result = await this.importMapFileRows(tripId, fileBuffer, filename, opts); - await this.colorizeImportedTracks(tripId, result); - return result; + return await this.uow.transactional(async () => { + const result = await this.importMapFileRows(tripId, fileBuffer, filename, opts); + await this.colorizeImportedTracks(tripId, result); + return result; + }); } private async importMapFileRows(tripId: string, fileBuffer: Buffer, filename: string, opts: KmlImportOptions = {}): Promise { @@ -1242,15 +1240,13 @@ export class PlacesService { const tracks = result?.places?.filter((p) => p.route_geometry && !p.route_color) ?? []; if (tracks.length === 0) return; - // PL36+PL37 — read (`distinctRouteColors`) and write (`setRouteColor`) in - // ONE transaction so two concurrent imports cannot both read the same set - // of free colours. + // PL36+PL37: read and write in ONE transaction so two concurrent imports + // cannot both read the same set of free colours. await this.uow.transactional(async () => { const taken = new Set(await this.placesRepo.distinctRouteColors(tripId)); const free = TRACK_COLORS.filter((c) => !taken.has(c)); for (const [i, track] of tracks.entries()) { - // Free ones first, then wrap through the whole palette — never reuse a - // free colour twice within the same import. + // Free ones first, then wrap through the palette: never a free colour twice. const color = i < free.length ? free[i] : TRACK_COLORS[(i - free.length) % TRACK_COLORS.length]; await this.placesRepo.setRouteColor(track.id, color); track.route_color = color; @@ -1854,6 +1850,7 @@ export class PlacesService { * The Google pass for places a file brought in (#2536). Only its points: a track * or a drawn path is a line, and looking a line up by its name finds a stranger. * Detached like the list imports, and just as quietly a no-op without a key. + * @txStandalone detached, after the import has committed. */ enrichImportedFilePlaces(tripId: string, userId: number, places: ImportedPlace[]): void { const points = (places as (ImportedPlace & EnrichablePlace)[]).filter(p => !p.route_geometry); @@ -1973,21 +1970,22 @@ export class PlacesService { // PL51 — the only explicit `ON CONFLICT … DO UPDATE` in the cluster; never touches `places.updated_at`. await this.placeRatingsRepo.upsertRating(id, userId, rating); } - // PL52 — `findWithTagsAndRatings` replaces the `getPlaceWithTags` delegation. - return await this.placesRepo.findWithTagsAndRatings(id); + return await this.placesRepo.findWithTagsAndRatings(id); // PL52 } - // Journey hooks — non-fatal, mirroring the route's try/catch wrappers. + /** @txStandalone a journey hook: a non-fatal sync after the place write, like the route's try/catch. */ async onCreated(tripId: string, placeId: number): Promise { try { await this.journey.onPlaceCreated(Number(tripId), placeId); } catch { /* non-fatal */ } } + /** @txStandalone the same non-fatal journey hook. */ async onUpdated(placeId: number): Promise { try { await this.journey.onPlaceUpdated(placeId); } catch { /* non-fatal */ } } + /** @txStandalone the same non-fatal journey hook. */ async onDeleted(placeId: number): Promise { try { await this.journey.onPlaceDeleted(placeId); diff --git a/server/src/nest/trips/trips.service.ts b/server/src/nest/trips/trips.service.ts index d699b4957..afc67113d 100644 --- a/server/src/nest/trips/trips.service.ts +++ b/server/src/nest/trips/trips.service.ts @@ -524,28 +524,30 @@ export class TripsService { : oldReminder; const tripIdNum = Number(tripId); // safe: `trip` above only resolved through the raw-bind seam on a real row (Task 6 review's own ruling on this exact conversion) - await this.tripsRepo.updateTripRow(tripIdNum, { // TP25 - title: newTitle, - description: newDesc ?? null, - start_date: newStart || null, - end_date: newEnd || null, - currency: newCurrency, - // Task 7 security review L1 (absorbed here): `newArchived` is already - // `trip.is_archived` verbatim when `data.is_archived` was never sent — - // the legacy statement bound that value AS-IS, including a stored - // `NULL`. A `?? 0` fold here would write `0` in that case instead, - // silently un-nulling a column the caller never asked to change. - is_archived: newArchived, - cover_image: newCover ?? null, - reminder_days: newReminder, - }); - - if (trip.start_date && trip.end_date && newStart && newStart !== trip.start_date) - await this.vacay.shiftOwnerEntriesForTripWindow(trip.user_id, trip.start_date, trip.end_date, newStart); - + // The row, the leave entries that follow its dates and the rebuilt day grid are + // one write: a failure halfway used to leave a trip whose days missed its dates. let removedDays: DayGridRemoval[] = []; - if (regenerate) { - await this.uow.transactional(async () => { + await this.uow.transactional(async () => { + await this.tripsRepo.updateTripRow(tripIdNum, { // TP25 + title: newTitle, + description: newDesc ?? null, + start_date: newStart || null, + end_date: newEnd || null, + currency: newCurrency, + // Task 7 security review L1 (absorbed here): `newArchived` is already + // `trip.is_archived` verbatim when `data.is_archived` was never sent — + // the legacy statement bound that value AS-IS, including a stored + // `NULL`. A `?? 0` fold here would write `0` in that case instead, + // silently un-nulling a column the caller never asked to change. + is_archived: newArchived, + cover_image: newCover ?? null, + reminder_days: newReminder, + }); + + if (trip.start_date && trip.end_date && newStart && newStart !== trip.start_date) + await this.vacay.shiftOwnerEntriesForTripWindow(trip.user_id, trip.start_date, trip.end_date, newStart); + + if (regenerate) { // Accommodations have no absolute date columns, so their pre-change dates must be // snapshotted before generateDays re-dates the day rows in place. const prevDays = await this.daysRepo.listOrderedForReorder(tripIdNum); // TP26 @@ -556,9 +558,6 @@ export class TripsService { // so re-stamp reservation_time to follow — same rules as reorderDays/insertDay. const newDays = await this.daysRepo.listOrderedForReorder(tripIdNum); // TP27 const newDateByDayId = new Map(newDays.map(d => [d.id, d.date])); - // `tripIdNum`, not `tripId` (Task 9 fix wave, H2): `restampReservationDates` - // now takes the parsed row id, the same value every other DaysService - // survivor takes — see its own docstring. await this.days.restampReservationDates(tripIdNum, prevDateByDayId, newDateByDayId); } else { // Default: generateDays re-dates day rows positionally; re-anchor dated bookings to @@ -567,8 +566,8 @@ export class TripsService { await this.reservations.resyncReservationDays(tripId); await this.days.resyncAccommodationDays(tripIdNum, prevDateByDayId); } - }); - } + } + }); const changes: Record = {}; if (title && title !== trip.title) changes.title = title; diff --git a/server/src/nest/vacay/vacay.service.ts b/server/src/nest/vacay/vacay.service.ts index b2885074e..fc5b64cf3 100644 --- a/server/src/nest/vacay/vacay.service.ts +++ b/server/src/nest/vacay/vacay.service.ts @@ -209,27 +209,13 @@ function parseAsUtcMillis(value: string): number { * legacy lazy require sat in; notifications stay fire-and-forget dynamic * imports. * - * Post-migration fixes on top of the relocated legacy behavior: the - * multi-statement writes (acceptInvite, dissolvePlan, deleteYear, updatePlan's - * carry-over recompute) run in uow.transactional(); every outbound fetch carries - * an AbortSignal timeout and the nager.at responses are ok-checked; - * applyHolidayCalendars honors the cache TTL; addYear no longer swallows real - * errors; the holiday cache is instance state instead of a module-level map. - * All consumers are in-container since the trip fold (TripsService injects - * this class); vacay.bridge.ts was deleted with its last outside-container - * consumer. + * Multi-statement writes run in uow.transactional(), never around the + * nager.at fetch (timed out and ok-checked, cached per instance for the TTL); + * addYear does not swallow real errors. TripsService injects this class. * - * R6 (this task): `removeShare`/`setShareHidden` no longer read an UNSCOPED - * `vacay_shares` row and check ownership in JS — `VacaySharesRepository - * .findScopedForRemoval`/`.findScopedForHide` scope the SAME two checks IN - * the SQL statement itself (two distinct methods, not one with a mode flag — - * see that repository's docstrings). Behaviorally identical to the legacy - * shape on every input (the JS check ran before any mutation either way), - * flagged as a genuine SQL-shape tightening per the plan's "For the user" - * note. `getStats` (VC126) now wraps its per-request carry-over write in - * `uow.transactional`, matching every OTHER multi-row write loop in this - * file (plan3f-inputs.md correction #7's "wrap it, flag it" default) — - * flagged here rather than left silently un-transacted either way. + * `removeShare`/`setShareHidden` scope their ownership check in the SQL + * itself (`VacaySharesRepository.findScopedForRemoval`/`.findScopedForHide`), + * and `getStats` writes its per-request carry-over in `uow.transactional`. */ @Injectable() export class VacayService { @@ -382,19 +368,28 @@ export class VacayService { // Plan management // ------------------------------------------------------------------------- + /** + * The caller's own plan, created with its first year, allowance and colour on + * first use, in one transaction whose second look keeps two first requests + * from creating two plans. + * @txStandalone an idempotent lazy provision, whatever the caller does next. + */ async getOwnPlan(userId: number): Promise { - let plan = await this.plans.findByOwner(userId); - if (!plan) { + const existing = await this.plans.findByOwner(userId); + if (existing) return existing; + return await this.uow.transactional(async () => { + const raced = await this.plans.findByOwner(userId); + if (raced) return raced; await this.plans.insertForOwner(userId); - plan = (await this.plans.findByOwner(userId))!; + const plan = (await this.plans.findByOwner(userId))!; // Seed the period today falls into — with a shifted leave year (#737) that is // not necessarily the current calendar year. const yr = await this.currentPeriodYear(userId); await this.years.insertIgnore(plan.id, yr); await this.userYears.insertIgnore(userId, plan.id, yr, 30, 0); await this.userColors.insertIgnore(userId, plan.id, '#6366f1'); - } - return plan; + return plan; + }); } async getActivePlan(userId: number): Promise { @@ -498,10 +493,17 @@ export class VacayService { } async applyHolidayCalendars(planId: number): Promise { + const dates = await this.holidayDatesToClear(planId); + if (dates.length > 0) await this.uow.transactional(() => this.clearHolidayDates(planId, dates)); + } + + /** The dates the plan's public-holiday calendars clear. Fetched first, outside any transaction. */ + private async holidayDatesToClear(planId: number): Promise { + const dates = new Set(); const holidaysEnabled = await this.plans.getHolidaysEnabled(planId); - if (!holidaysEnabled) return; + if (!holidaysEnabled) return []; const calendars = await this.holidayCalendars.listPublicForPlan(planId); - if (calendars.length === 0) return; + if (calendars.length === 0) return []; const years = await this.years.listForPlan(planId); // A shifted leave year (#737) runs into the next calendar year, so collect the // calendar years the members' windows actually touch — not just the period ids. @@ -520,16 +522,20 @@ export class VacayService { if (!holidays) continue; const hasRegions = holidays.some((h: Holiday) => h.counties && h.counties.length > 0); if (hasRegions && !region) continue; - // Outside the fetch's catch: a failing delete is a real error, and - // swallowing it as "API error" left vacation days standing on holidays. for (const h of holidays) { - if (h.global || !h.counties || (region && h.counties.includes(region))) { - await this.entries.deleteForPlanAndDate(planId, h.date); - await this.companyHolidays.deleteForPlanAndDate(planId, h.date); - } + if (h.global || !h.counties || (region && h.counties.includes(region))) dates.add(h.date); } } } + return [...dates]; + } + + /** Outside the fetch's catch: a failing delete is a real error, not an unanswered API. */ + private async clearHolidayDates(planId: number, dates: string[]): Promise { + for (const date of dates) { + await this.entries.deleteForPlanAndDate(planId, date); + await this.companyHolidays.deleteForPlanAndDate(planId, date); + } } async migrateHolidayCalendars(planId: number, plan: VacayPlan): Promise { @@ -557,25 +563,22 @@ export class VacayService { if (weekend_days !== undefined) patch.weekend_days = String(weekend_days); if (week_start !== undefined) patch.week_start = week_start === 0 ? 0 : 1; - await this.plans.update(planId, patch); + // Two transactions with the holiday fetch between them: network I/O never + // holds the connection. The settings land first, the clean-up they imply second. + await this.uow.transactional(async () => { + await this.plans.update(planId, patch); + if (company_holidays_enabled === true) { + const companyDates = await this.companyHolidays.listForPlan(planId); + for (const { date, fraction } of companyDates) await this.makeRoomForCompanyHoliday(planId, date, fraction); + } + await this.migrateHolidayCalendars(planId, (await this.plans.findById(planId))!); + }); + const holidayDates = await this.holidayDatesToClear(planId); - if (company_holidays_enabled === true) { - const companyDates = await this.companyHolidays.listForPlan(planId); - for (const { date, fraction } of companyDates) await this.makeRoomForCompanyHoliday(planId, date, fraction); - } - - const updatedPlan = (await this.plans.findById(planId))!; - await this.migrateHolidayCalendars(planId, updatedPlan); - await this.applyHolidayCalendars(planId); - - if (carry_over_enabled === false) { - await this.userYears.resetCarriedOverForPlan(planId); - } - - if (carry_over_enabled === true) { - // The chained per-year/per-user recompute is atomic — a failure mid-chain - // would otherwise leave later years carrying stale balances. - await this.uow.transactional(async () => { + await this.uow.transactional(async () => { + await this.clearHolidayDates(planId, holidayDates); + if (carry_over_enabled === false) await this.userYears.resetCarriedOverForPlan(planId); + if (carry_over_enabled === true) { const years = await this.years.listForPlan(planId); const users = await this.getPlanUsers(planId); for (let i = 0; i < years.length - 1; i++) { @@ -584,20 +587,15 @@ export class VacayService { for (const u of users) { const used = await this.usedDays(u.id, planId, yr); const config = await this.userYears.findForYear(u.id, planId, yr); - // L1 (task-7-review.md): legacy bound `config.vacation_days`/ - // `config.carried_over` into this total AS-IS, letting a NULL - // column pass through and coerce to 0 in the `+` (JS: `null + n - // = n`) — never defaulting a NULL vacation_days to 30. `?? 0` - // (not `?? 30`) reproduces that exact coercion under strict - // nullable typing; only the OUTER `config ? … : 30` (no row at - // all) is a genuine default. + // No row at all defaults to 30 days; a NULL column counts as 0, the + // way the legacy statement's arithmetic coerced it. const total = (config ? config.vacation_days ?? 0 : 30) + (config ? config.carried_over ?? 0 : 0); const carry = Math.max(0, total - used); await this.userYears.upsertCarriedOver(u.id, planId, nextYr, carry); } } - }); - } + } + }); await this.notifyPlanUsers(planId, socketId, 'vacay:settings'); diff --git a/server/tests/helpers/test-addons.ts b/server/tests/helpers/test-addons.ts index 1ff4d9f8a..712175037 100644 --- a/server/tests/helpers/test-addons.ts +++ b/server/tests/helpers/test-addons.ts @@ -1,6 +1,7 @@ import type Database from 'better-sqlite3'; import { AddonsService } from '../../src/nest/addons/addons.service'; import { sharedTestOrm } from './test-uow'; +import { UnitOfWork } from '../../src/nest/database/unit-of-work'; import { Addons } from '../../src/db/entities/Addons.entity'; import { PhotoProviders } from '../../src/db/entities/PhotoProviders.entity'; import { PhotoProviderFields } from '../../src/db/entities/PhotoProviderFields.entity'; @@ -40,5 +41,6 @@ export async function createTestAddonsService(db: Database.Database): Promise { nestApp = await buildApp(); app = nestApp.getHttpAdapter().getInstance(); t = await createTestOrm(testDb); - oauthSvc = new OauthService(t.repo(OauthClients), t.repo(OauthTokens), t.repo(OauthConsents), await createTestAddonsService(testDb), new AuditService(t.repo(AuditLog), t.repo(Users))); + oauthSvc = new OauthService(t.repo(OauthClients), t.repo(OauthTokens), t.repo(OauthConsents), await createTestAddonsService(testDb), new AuditService(t.repo(AuditLog), t.repo(Users)), new UnitOfWork(t.em)); }); beforeEach(async () => { diff --git a/server/tests/integration/oauth-authorize.test.ts b/server/tests/integration/oauth-authorize.test.ts index 2cd950f83..eeae60aec 100644 --- a/server/tests/integration/oauth-authorize.test.ts +++ b/server/tests/integration/oauth-authorize.test.ts @@ -41,6 +41,7 @@ import { buildApp } from '../../src/bootstrap'; import { resetTestDb, resetRateLimits } from '../helpers/test-db'; import { createUser } from '../helpers/factories'; import { OauthService } from '../../src/nest/oauth/oauth.service'; +import { UnitOfWork } from '../../src/nest/database/unit-of-work'; import { createTestAddonsService } from '../helpers/test-addons'; import { AuditService } from '../../src/nest/audit/audit.service'; import { createTestOrm, type TestOrm } from '../helpers/test-orm'; @@ -86,7 +87,7 @@ beforeAll(async () => { nestApp = await buildApp(); app = nestApp.getHttpAdapter().getInstance(); t = await createTestOrm(testDb); - containerSideOauth = new OauthService(t.repo(OauthClients), t.repo(OauthTokens), t.repo(OauthConsents), await createTestAddonsService(testDb), new AuditService(t.repo(AuditLog), t.repo(Users))); + containerSideOauth = new OauthService(t.repo(OauthClients), t.repo(OauthTokens), t.repo(OauthConsents), await createTestAddonsService(testDb), new AuditService(t.repo(AuditLog), t.repo(Users)), new UnitOfWork(t.em)); }); beforeEach(async () => { diff --git a/server/tests/integration/oauth.test.ts b/server/tests/integration/oauth.test.ts index 3d9bb2dc4..aec679477 100644 --- a/server/tests/integration/oauth.test.ts +++ b/server/tests/integration/oauth.test.ts @@ -38,6 +38,7 @@ import { createUser } from '../helpers/factories'; import { authCookie } from '../helpers/auth'; import { ALL_SCOPES } from '../../src/mcp/scopes'; import { OauthService } from '../../src/nest/oauth/oauth.service'; +import { UnitOfWork } from '../../src/nest/database/unit-of-work'; import { createTestAddonsService } from '../helpers/test-addons'; import { AuditService } from '../../src/nest/audit/audit.service'; import { createTestOrm, type TestOrm } from '../helpers/test-orm'; @@ -84,7 +85,7 @@ beforeAll(async () => { nestApp = await buildApp(); app = nestApp.getHttpAdapter().getInstance(); t = await createTestOrm(testDb); - containerSideOauth = new OauthService(t.repo(OauthClients), t.repo(OauthTokens), t.repo(OauthConsents), await createTestAddonsService(testDb), new AuditService(t.repo(AuditLog), t.repo(Users))); + containerSideOauth = new OauthService(t.repo(OauthClients), t.repo(OauthTokens), t.repo(OauthConsents), await createTestAddonsService(testDb), new AuditService(t.repo(AuditLog), t.repo(Users)), new UnitOfWork(t.em)); }); beforeEach(async () => { diff --git a/server/tests/integration/plugins/boot-registry-order.test.ts b/server/tests/integration/plugins/boot-registry-order.test.ts index 7249e6589..58edbdc74 100644 --- a/server/tests/integration/plugins/boot-registry-order.test.ts +++ b/server/tests/integration/plugins/boot-registry-order.test.ts @@ -232,6 +232,7 @@ describe('plugin boot vs registry scan ordering', () => { t2.repo(PhotoProviderFields), t2.repo(AppSettings), t2.repo(Users), + new UnitOfWork(t2.em), ); const auditLogRepo2 = t2.repo(AuditLog); const usersRepo2 = t2.repo(Users); diff --git a/server/tests/unit/nest/accommodations.service.test.ts b/server/tests/unit/nest/accommodations.service.test.ts index a5e6cf297..4b86a6099 100644 --- a/server/tests/unit/nest/accommodations.service.test.ts +++ b/server/tests/unit/nest/accommodations.service.test.ts @@ -42,7 +42,7 @@ import { AccommodationsModule } from '../../../src/nest/accommodations/accommoda import { AccommodationsDomainModule } from '../../../src/nest/accommodations/accommodations-domain.module'; import { AccommodationsController } from '../../../src/nest/accommodations/accommodations.controller'; import { expectRegisteredProvider, expectRegisteredController } from '../../helpers/module-providers'; -import { createTestUnitOfWork, createTestAppSettingsRepo } from '../../helpers/test-uow'; +import { createTestUnitOfWork, createTestAppSettingsRepo, createTestReservationsRepo } from '../../helpers/test-uow'; // Named `svc` so the moved cases read exactly as they did on DaysService. let svc: Awaited>; @@ -177,6 +177,23 @@ describe('getAccommodation', () => { }); describe('updateAccommodation', () => { + it('DAY-SVC-TX-001: a failing booking sync rolls the stay edit back with it', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const day = createDay(testDb, trip.id); + const place = createPlace(testDb, trip.id, { name: 'Hotel' }); + await svc.createAccommodation(trip.id, { place_id: place.id, start_day_id: day.id, end_day_id: day.id, check_in: '14:00' }); + const { id } = testDb.prepare('SELECT id FROM day_accommodations WHERE trip_id = ?').get(trip.id) as { id: number }; + const existing = (await svc.getAccommodation(id, trip.id))!; + const reservations = await createTestReservationsRepo(testDb); + const spy = vi.spyOn(reservations, 'setMetadataAndConfirmation').mockRejectedValueOnce(new Error('boom')); + + await expect(svc.updateAccommodation(id, existing, { check_in: '18:00' })).rejects.toThrow('boom'); + + expect(testDb.prepare('SELECT check_in FROM day_accommodations WHERE id = ?').get(id)).toEqual({ check_in: '14:00' }); + spy.mockRestore(); + }); + it('DAY-SVC-023 — updates check-in and check-out times', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/addons.service.test.ts b/server/tests/unit/nest/addons.service.test.ts index 16cc82220..3a3c0ede1 100644 --- a/server/tests/unit/nest/addons.service.test.ts +++ b/server/tests/unit/nest/addons.service.test.ts @@ -27,6 +27,7 @@ import { resetTestDb } from '../../helpers/test-db'; import { createUser } from '../../helpers/factories'; import { createTestOrm, type TestOrm } from '../../helpers/test-orm'; import { AddonsService } from '../../../src/nest/addons/addons.service'; +import { UnitOfWork } from '../../../src/nest/database/unit-of-work'; import { PlaceShadowService } from '../../../src/nest/place-shadow/place-shadow.service'; import { Addons } from '../../../src/db/entities/Addons.entity'; import type { AddonsRepository } from '../../../src/db/repositories/Addons.repository'; @@ -169,7 +170,7 @@ beforeEach(() => { t.clear(); getPhotoProviderConfig.mockReset(); getPhotoProviderConfig.mockReturnValue({}); - svc = new AddonsService(addonsRepo, photoProvidersRepo, photoProviderFieldsRepo, appSettingsRepo, usersRepo); + svc = new AddonsService(addonsRepo, photoProvidersRepo, photoProviderFieldsRepo, appSettingsRepo, usersRepo, new UnitOfWork(t.em)); }); afterEach(() => { @@ -415,6 +416,19 @@ describe('AddonsService addon/feature flags', () => { setValueSpy.mockRestore(); }); + + it('updateCollabFeatures saves the flags together: a failing second write keeps the first one off disk', async () => { + const setValue = appSettingsRepo.setValue.bind(appSettingsRepo); + const setValueSpy = vi.spyOn(appSettingsRepo, 'setValue') + .mockImplementationOnce(setValue) + .mockRejectedValueOnce(new Error('disk full')); + + await expect(svc.updateCollabFeatures({ chat: false, notes: false })).rejects.toThrow('disk full'); + + expect(rawAppSetting('collab_chat_enabled')?.value).not.toBe('false'); + expect(await svc.getCollabFeatures()).toEqual({ chat: true, notes: true, links: true, polls: true, whatsnext: true }); + setValueSpy.mockRestore(); + }); }); /** diff --git a/server/tests/unit/nest/airports/airports.service.test.ts b/server/tests/unit/nest/airports/airports.service.test.ts index 7bbe7802f..3dc47092f 100644 --- a/server/tests/unit/nest/airports/airports.service.test.ts +++ b/server/tests/unit/nest/airports/airports.service.test.ts @@ -19,7 +19,9 @@ import { CronRegistrarService } from '../../../../src/nest/scheduling/cron-regis import type { RuntimeEnvService } from '../../../../src/nest/app-config/runtime-env.service'; import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; -import { createTestReservationsRepo, createTestReservationEndpointsRepo } from '../../../helpers/test-uow'; +import { createTestReservationsRepo, createTestReservationEndpointsRepo, createTestUnitOfWork } from '../../../helpers/test-uow'; +import type { UnitOfWork } from '../../../../src/nest/database/unit-of-work'; +import { createUser, createTrip } from '../../../helpers/factories'; import type { ReservationsRepository } from '../../../../src/db/repositories/Reservations.repository'; import type { ReservationEndpointsRepository } from '../../../../src/db/repositories/ReservationEndpoints.repository'; @@ -27,6 +29,7 @@ const testDb = createSnapshotTestDb(); let t: TestOrm; let reservationsRepo: ReservationsRepository; let endpointsRepo: ReservationEndpointsRepository; +let uow: UnitOfWork; beforeAll(async () => { // Global context disallowed — the production setting — so this genuinely @@ -35,6 +38,7 @@ beforeAll(async () => { t = await createTestOrm(testDb, { allowGlobalContext: false }); reservationsRepo = await createTestReservationsRepo(testDb); endpointsRepo = await createTestReservationEndpointsRepo(testDb); + uow = await createTestUnitOfWork(testDb); }); afterAll(async () => { await t.close(); testDb.close(); }); beforeEach(() => vi.clearAllMocks()); @@ -48,7 +52,7 @@ describe('AirportsService boot backfill', () => { unregister: vi.fn(), runOnBoot: vi.fn(async (name: string, fn: () => void | Promise) => { registered.push([name, fn]); await fn(); }), }; - const svc = new AirportsService(reservationsRepo, endpointsRepo, registrar as unknown as CronRegistrarService); + const svc = new AirportsService(reservationsRepo, endpointsRepo, registrar as unknown as CronRegistrarService, uow); const backfillSpy = vi.spyOn(svc, 'backfillFlightEndpoints').mockResolvedValue(undefined); await svc.onApplicationBootstrap(); expect(registrar.runOnBoot).toHaveBeenCalledWith('airports-flight-endpoints-boot', expect.any(Function)); @@ -57,7 +61,7 @@ describe('AirportsService boot backfill', () => { it('AIRPORTS-SVC-002: with MikroORM wired into the registrar, the boot-time backfill actually runs — no cannotUseGlobalContext, nothing swallowed', async () => { const registrar = new CronRegistrarService(new SchedulerRegistry(), { isTest: () => false } as RuntimeEnvService, t.orm); - const svc = new AirportsService(reservationsRepo, endpointsRepo, registrar); + const svc = new AirportsService(reservationsRepo, endpointsRepo, registrar, uow); const backfillSpy = vi.spyOn(svc, 'backfillFlightEndpoints'); const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); try { @@ -72,7 +76,7 @@ describe('AirportsService boot backfill', () => { it('AIRPORTS-SVC-003: without MikroORM, the boot-time backfill never runs at all — logged with runOnBoot\'s own distinct message, never swallowed into "[DB] Flight endpoint backfill failed"', async () => { const registrar = new CronRegistrarService(new SchedulerRegistry(), { isTest: () => false } as RuntimeEnvService); // no orm - const svc = new AirportsService(reservationsRepo, endpointsRepo, registrar); + const svc = new AirportsService(reservationsRepo, endpointsRepo, registrar, uow); const backfillSpy = vi.spyOn(svc, 'backfillFlightEndpoints'); const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); try { @@ -92,7 +96,7 @@ describe('AirportsService boot backfill', () => { unregister: vi.fn(), runOnBoot: vi.fn(async () => { throw new Error('RequestContext.create blew up'); }), }; - const svc = new AirportsService(reservationsRepo, endpointsRepo, registrar as unknown as CronRegistrarService); + const svc = new AirportsService(reservationsRepo, endpointsRepo, registrar as unknown as CronRegistrarService, uow); const backfillSpy = vi.spyOn(svc, 'backfillFlightEndpoints'); const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); try { @@ -103,4 +107,29 @@ describe('AirportsService boot backfill', () => { errSpy.mockRestore(); } }); + + it('AIRPORTS-SVC-005: a flight gets both endpoints or neither: a failing second insert leaves it to the next run', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const meta = JSON.stringify({ departure_airport: 'FRA', arrival_airport: 'JFK' }); + const half = Number(testDb.prepare("INSERT INTO reservations (trip_id, title, type, metadata) VALUES (?, 'Out', 'flight', ?)").run(trip.id, meta).lastInsertRowid); + const whole = Number(testDb.prepare("INSERT INTO reservations (trip_id, title, type, metadata) VALUES (?, 'Back', 'flight', ?)").run(trip.id, meta).lastInsertRowid); + const insert = endpointsRepo.insertEndpoint.bind(endpointsRepo); + // The first flight's 'to' endpoint fails, which ends this run. + const spy = vi.spyOn(endpointsRepo, 'insertEndpoint') + .mockImplementationOnce(insert) + .mockRejectedValueOnce(new Error('disk full')); + const svc = new AirportsService(reservationsRepo, endpointsRepo, {} as CronRegistrarService, uow); + + await expect(svc.backfillFlightEndpoints()).rejects.toThrow('disk full'); + const count = (id: number) => (testDb.prepare('SELECT COUNT(*) AS c FROM reservation_endpoints WHERE reservation_id = ?').get(id) as { c: number }).c; + expect(count(half)).toBe(0); + spy.mockRestore(); + + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + await svc.backfillFlightEndpoints(); + logSpy.mockRestore(); + expect(count(half)).toBe(2); + expect(count(whole)).toBe(2); + }); }); diff --git a/server/tests/unit/nest/notification-preferences.service.test.ts b/server/tests/unit/nest/notification-preferences.service.test.ts index d31affef2..5f9c9e543 100644 --- a/server/tests/unit/nest/notification-preferences.service.test.ts +++ b/server/tests/unit/nest/notification-preferences.service.test.ts @@ -352,6 +352,15 @@ describe('setAdminPreferences', () => { const row = testDb.prepare("SELECT value FROM app_settings WHERE key = ?").get('admin_notif_pref_version_available_email') as { value: string } | undefined; expect(row?.value).toBe('1'); }); + + it('NPREF-025b: the global and the per-user rows are one save: a failing in-app row keeps the email pref unchanged', async () => { + const { user } = createAdmin(testDb); + const prefsRepo = await createTestNotificationChannelPreferencesRepo(testDb); + const spy = vi.spyOn(prefsRepo, 'upsertPreference').mockRejectedValueOnce(new Error('disk full')); + await expect(setAdminPreferences(user.id, { version_available: { email: false, inapp: false } })).rejects.toThrow('disk full'); + expect(testDb.prepare("SELECT value FROM app_settings WHERE key = ?").get('admin_notif_pref_version_available_email')).toBeUndefined(); + spy.mockRestore(); + }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/server/tests/unit/nest/oauth.service.test.ts b/server/tests/unit/nest/oauth.service.test.ts index 826fb2d98..624c96f1d 100644 --- a/server/tests/unit/nest/oauth.service.test.ts +++ b/server/tests/unit/nest/oauth.service.test.ts @@ -56,6 +56,7 @@ function makePkce() { } import { OauthService } from '../../../src/nest/oauth/oauth.service'; +import { UnitOfWork } from '../../../src/nest/database/unit-of-work'; import { AuditService } from '../../../src/nest/audit/audit.service'; import type { AddonsService } from '../../../src/nest/addons/addons.service'; import { getMcpSafeUrl } from '../../../src/app-config'; @@ -119,7 +120,7 @@ beforeAll(async () => { clientsRepo = t.repo(OauthClients); tokensRepo = t.repo(OauthTokens); consentsRepo = t.repo(OauthConsents); - svc = new OauthService(clientsRepo, tokensRepo, consentsRepo, addonsStub, new AuditService(auditLogRepo, usersRepo)); + svc = new OauthService(clientsRepo, tokensRepo, consentsRepo, addonsStub, new AuditService(auditLogRepo, usersRepo), new UnitOfWork(t.em)); }); beforeEach(() => { @@ -492,6 +493,21 @@ describe('refreshTokens', () => { expect(result.tokens!.access_token.startsWith('trekoa_')).toBe(true); }); + it('revoking the old pair and issuing the new one are one write: a failing issue keeps the old pair', async () => { + const { user } = createUser(testDb); + const created = await makeClient(user.id); + const clientId = created.client!.client_id as string; + const rawSecret = created.client!.client_secret as string; + + const { access_token, refresh_token } = await issueTokens(clientId, user.id, ['trips:read']); + const spy = vi.spyOn(tokensRepo, 'insertToken').mockRejectedValueOnce(new Error('disk full')); + await expect(refreshTokens(refresh_token, clientId, rawSecret)).rejects.toThrow('disk full'); + spy.mockRestore(); + + // Before, the old pair was already revoked here and the client logged out. + expect(await getUserByAccessToken(access_token)).not.toBeNull(); + }); + it('old tokens are revoked after refresh (rotation)', async () => { const { user } = createUser(testDb); const created = await makeClient(user.id); @@ -1308,7 +1324,7 @@ describe('module-scoped OAuth state', () => { codeChallengeMethod: 'S256', }))!; - const secondInstance = new OauthService(clientsRepo, tokensRepo, consentsRepo, addonsStub, new AuditService(auditLogRepo, usersRepo)); + const secondInstance = new OauthService(clientsRepo, tokensRepo, consentsRepo, addonsStub, new AuditService(auditLogRepo, usersRepo), new UnitOfWork(t.em)); expect((await secondInstance.consumeAuthCode(code))?.userId).toBe(42); }); }); diff --git a/server/tests/unit/nest/oidc.service.test.ts b/server/tests/unit/nest/oidc.service.test.ts index 3faf79646..ebfd51e23 100644 --- a/server/tests/unit/nest/oidc.service.test.ts +++ b/server/tests/unit/nest/oidc.service.test.ts @@ -793,6 +793,23 @@ describe('findOrCreateUser', () => { expect(row.avatar).toBeNull(); }); + it('OIDC-SVC-062: the identity switch and the avatar are one write: a failing avatar keeps the old link', async () => { + const { user } = createUser(testDb, { email: 'switch4@example.com' }); + testDb.prepare('UPDATE users SET oidc_sub = ?, oidc_issuer = ?, avatar = ? WHERE id = ?') + .run('sub-old-4', 'https://old-idp.example.com', 'https://old-idp.example.com/u/me.png', user.id); + const users = await createTestUsersRepo(testDb); + const spy = vi.spyOn(users, 'setAvatarRaw').mockRejectedValueOnce(new Error('disk full')); + + await expect(svc.findOrCreateUser( + { sub: 'sub-new-4', email: 'switch4@example.com', name: 'Switcher', email_verified: true }, + { ...MOCK_CONFIG, issuer: 'https://new-idp.example.com' }, + )).rejects.toThrow('disk full'); + + expect(testDb.prepare('SELECT oidc_sub, oidc_issuer FROM users WHERE id = ?').get(user.id)) + .toEqual({ oidc_sub: 'sub-old-4', oidc_issuer: 'https://old-idp.example.com' }); + spy.mockRestore(); + }); + it('OIDC-SVC-061: an uploaded avatar survives the same switch', async () => { const { user } = createUser(testDb, { email: 'switch2@example.com' }); // A local upload is a bare filename, not a URL, and belongs to the user. diff --git a/server/tests/unit/nest/places.service.test.ts b/server/tests/unit/nest/places.service.test.ts index 530469f93..69406c3da 100644 --- a/server/tests/unit/nest/places.service.test.ts +++ b/server/tests/unit/nest/places.service.test.ts @@ -269,6 +269,19 @@ describe('create', () => { expect(Array.isArray(place.tags)).toBe(true); }); + it('PLACE-SVC-007b: the place and its tags are one write: a failing tag insert leaves no place', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + const tag = createTag(testDb, user.id, { name: 'Highlight' }); + const tags = await createTestTagsRepo(testDb); + const spy = vi.spyOn(tags, 'insertIgnore').mockRejectedValueOnce(new Error('disk full')); + + await expect(svc.create(String(trip.id), { name: 'Half Place', tags: [tag.id] })).rejects.toThrow('disk full'); + + expect(testDb.prepare("SELECT id FROM places WHERE name = 'Half Place'").all()).toEqual([]); + spy.mockRestore(); + }); + it('PLACE-SVC-008 — creates a place with tags', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id); diff --git a/server/tests/unit/nest/process-state-ports.test.ts b/server/tests/unit/nest/process-state-ports.test.ts index 9536cdcc3..1ed2933dc 100644 --- a/server/tests/unit/nest/process-state-ports.test.ts +++ b/server/tests/unit/nest/process-state-ports.test.ts @@ -121,7 +121,7 @@ describe('OAuth pending codes', () => { it('PORTS-006: OauthService issues and redeems codes through the injected store', async () => { const store = new InMemoryPendingCodeStore(); - const service = new OauthService({} as never, {} as never, {} as never, {} as never, {} as never, store); + const service = new OauthService({} as never, {} as never, {} as never, {} as never, {} as never, {} as never, store); const issued = await service.createAuthCode({ clientId: 'c', userId: 1, @@ -138,7 +138,7 @@ describe('OAuth pending codes', () => { it('PORTS-007: OauthModule provides the process-wide store, the one a hand-built service defaults to', () => { expectRegisteredProvider(OauthModule, { provide: PendingCodeStore, useValue: processPendingCodes }); - const service = new OauthService({} as never, {} as never, {} as never, {} as never, {} as never); + const service = new OauthService({} as never, {} as never, {} as never, {} as never, {} as never, {} as never); expect((service as unknown as { pendingCodes: unknown }).pendingCodes).toBe(processPendingCodes); }); @@ -150,7 +150,7 @@ describe('OAuth pending codes', () => { expect(pendingCodesSlot.get()).toBe(fake); await sweepPendingCodes(5); expect(sweep).toHaveBeenCalledWith(5); - const service = new OauthService({} as never, {} as never, {} as never, {} as never, {} as never); + const service = new OauthService({} as never, {} as never, {} as never, {} as never, {} as never, {} as never); expect((service as unknown as { pendingCodes: unknown }).pendingCodes).toBe(fake); } finally { oauthModule.onModuleDestroy(); diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index 30856cd3b..faa33f704 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -828,6 +828,19 @@ describe('resyncReservationDays (#1288)', () => { expect(res.day_id).toBe(dayFor(trip.id, '2025-06-02')); }); + it('TRIP-SVC-018b: the trip row and its rebuilt days are one write: a failing rebuild keeps the old dates', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { start_date: '2025-06-01', end_date: '2025-06-05' }); + const days = await createTestDaysRepo(testDb); + const spy = vi.spyOn(days, 'listOrderedForReorder').mockRejectedValueOnce(new Error('boom')); + + await expect(svc.updateTrip(trip.id, user.id, { start_date: '2025-06-02', end_date: '2025-06-06' }, 'user')).rejects.toThrow('boom'); + + expect(testDb.prepare('SELECT start_date, end_date FROM trips WHERE id = ?').get(trip.id)) + .toEqual({ start_date: '2025-06-01', end_date: '2025-06-05' }); + spy.mockRestore(); + }); + it('TRIP-SVC-019: a reservation whose date falls outside the new range keeps its day_id (not nulled)', async () => { const { user } = createUser(testDb); const trip = createTrip(testDb, user.id, { start_date: '2025-06-01', end_date: '2025-06-05' }); diff --git a/server/tests/unit/nest/unified-memories.service.test.ts b/server/tests/unit/nest/unified-memories.service.test.ts index 149741b52..34c0c8486 100644 --- a/server/tests/unit/nest/unified-memories.service.test.ts +++ b/server/tests/unit/nest/unified-memories.service.test.ts @@ -242,6 +242,23 @@ describe('addTripPhotos', () => { }); }); +describe('addTripPhotos is atomic per photo', () => { + it('MEM-UNIFIED-007b: the photo row and its trip link are one write: a failing link leaves no photo row', async () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id); + testDb.prepare( + 'INSERT OR IGNORE INTO photo_providers (id, name, description, icon, enabled, sort_order) VALUES (?, ?, ?, ?, ?, ?)' + ).run('tx-prov', 'Tx', 'Tx provider', 'Image', 1, 98); + const spy = vi.spyOn(tripPhotosRepo, 'insertIgnore').mockRejectedValueOnce(new Error('disk full')); + + const result = await addTripPhotos(String(trip.id), user.id, false, [{ provider: 'tx-prov', asset_ids: ['asset-tx'] }], 'sid'); + + expect(result.success).toBe(false); + expect(testDb.prepare("SELECT id FROM trek_photos WHERE asset_id = 'asset-tx'").all()).toEqual([]); + spy.mockRestore(); + }); +}); + // ── setTripPhotoSharing ─────────────────────────────────────────────────────── describe('setTripPhotoSharing', () => {