From 094a46a3cf910d80bb55251e4eae4f68fc486f3b Mon Sep 17 00:00:00 2001 From: Maurice Date: Wed, 7 Oct 2026 16:59:52 +0200 Subject: [PATCH] fix(vacay): stop swallowing a failed delete as a holiday API error One try around the holiday fetch and the deletes that follow it caught both, so a database error while removing vacation days that fall on a public holiday was logged as nothing and left the days standing. Only the fetch is tolerated now. Also drops two files long gone from the Sonar coverage exclusions. --- server/src/nest/vacay/vacay.service.ts | 50 ++++++++++++-------- server/tests/unit/nest/vacay.service.test.ts | 15 ++++++ sonar-project.properties | 2 - 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/server/src/nest/vacay/vacay.service.ts b/server/src/nest/vacay/vacay.service.ts index 59000b00b..b2885074e 100644 --- a/server/src/nest/vacay/vacay.service.ts +++ b/server/src/nest/vacay/vacay.service.ts @@ -479,6 +479,24 @@ export class VacayService { // Holiday calendar helpers // ------------------------------------------------------------------------- + /** A country's public holidays for a year, cached; undefined when the API does not answer usably. */ + private async fetchPublicHolidays(year: number, country: string): Promise { + const cacheKey = `${year}-${country}`; + const cached = this.holidayCache.get(cacheKey); + if (cached && Date.now() - cached.time < CACHE_TTL) return cached.data as Holiday[]; + if (!COUNTRY_RE.test(country)) return undefined; + try { + const resp = await fetch(`https://date.nager.at/api/v3/PublicHolidays/${year}/${country}`, { signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }); + if (!resp.ok) { discardBody(resp); return undefined; } + const parsed = await readCappedJson(resp, MAX_HOLIDAY_BYTES); + if (parsed === undefined) return undefined; + this.holidayCache.set(cacheKey, { data: parsed, time: Date.now() }); + return parsed; + } catch { + return undefined; + } + } + async applyHolidayCalendars(planId: number): Promise { const holidaysEnabled = await this.plans.getHolidaysEnabled(planId); if (!holidaysEnabled) return; @@ -498,28 +516,18 @@ export class VacayService { const country = cal.region.split('-')[0]; const region = cal.region.includes('-') ? cal.region : null; for (const year of calendarYears) { - try { - const cacheKey = `${year}-${country}`; - const cached = this.holidayCache.get(cacheKey); - let holidays = cached && Date.now() - cached.time < CACHE_TTL ? cached.data as Holiday[] : undefined; - if (!holidays) { - if (!COUNTRY_RE.test(country)) continue; - const resp = await fetch(`https://date.nager.at/api/v3/PublicHolidays/${year}/${country}`, { signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }); - if (!resp.ok) { discardBody(resp); continue; } - const parsed = await readCappedJson(resp, MAX_HOLIDAY_BYTES); - if (parsed === undefined) continue; - holidays = parsed; - this.holidayCache.set(cacheKey, { data: holidays, time: Date.now() }); + const holidays = await this.fetchPublicHolidays(year, country); + 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); } - const hasRegions = holidays.some((h: Holiday) => h.counties && h.counties.length > 0); - if (hasRegions && !region) continue; - 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); - } - } - } catch { /* API error, skip */ } + } } } } diff --git a/server/tests/unit/nest/vacay.service.test.ts b/server/tests/unit/nest/vacay.service.test.ts index 4ad7caa42..65c839430 100644 --- a/server/tests/unit/nest/vacay.service.test.ts +++ b/server/tests/unit/nest/vacay.service.test.ts @@ -1472,6 +1472,21 @@ describe('applyHolidayCalendars', () => { .all(plan.id, holidayDate); expect(remaining).toHaveLength(0); }); + + it('VACAY-SVC-047b: a holiday API that does not answer is skipped, a failing delete is not', async () => { + const { plan } = await setupUserWithPlan(); + testDb.prepare('UPDATE vacay_plans SET holidays_enabled = 1 WHERE id = ?').run(plan.id); + await svc.addHolidayCalendar(plan.id, 'FR', null, undefined, 0, undefined); + + vi.stubGlobal('fetch', vi.fn().mockRejectedValue(new TypeError('fetch failed'))); + await expect(svc.applyHolidayCalendars(plan.id)).resolves.toBeUndefined(); + + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: true, json: async () => [{ date: '2031-07-14', global: true }] })); + const entries = (svc as unknown as { entries: { deleteForPlanAndDate: () => Promise } }).entries; + const del = vi.spyOn(entries, 'deleteForPlanAndDate').mockRejectedValueOnce(new Error('database is locked')); + await expect(svc.applyHolidayCalendars(plan.id)).rejects.toThrow('database is locked'); + del.mockRestore(); + }); }); // ── Read-only calendar shares (#444/#667) ───────────────────────────────────── diff --git a/sonar-project.properties b/sonar-project.properties index 0ccc6a574..e81f2a23f 100644 --- a/sonar-project.properties +++ b/sonar-project.properties @@ -46,8 +46,6 @@ sonar.coverage.exclusions=\ server/src/index.ts,\ server/src/config.ts,\ server/src/db/database.ts,\ - server/src/db/migrations.ts,\ - server/src/db/seeds.ts,\ server/src/demo/**,\ server/src/nest/plugins/runtime/plugin-host-entry.ts,\ server/assets/**,\