mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-08 12:32:43 +00:00
fix(measurement-reminders): recompute nextDueAt from the field actually set, not a nullish fallback
PATCH /api/measurement-reminders/{id} persisted an explicit null cadence
clear correctly, but the immediate nextDueAt recomputation merged fields
with `updateValue ?? existingValue` — a JSON null and an omitted field
are both nullish, so the merge silently fell back to the OLD cadence for
one cycle. This hit interval-to-RRULE and RRULE-to-interval transitions
and an explicit anchorDate clear alike.
The merge now keys off Object.hasOwn(updateData, field): updateData
already encodes exactly which fields are being persisted, including the
mutual-exclusivity auto-clear (setting one cadence nulls the other), so
recomputation can never diverge from what the row ends up holding.
This commit is contained in:
1 parent
8736f44442
commit
1371bb1cf9
2 files changed
+255
-9
No files matched your search
@@ -0,0 +1,229 @@
|
||||
/**
|
||||
* v1.32.1 — regression for iOS coordination issue #62: the `nextDueAt`
|
||||
* recomputation on `PATCH /api/measurement-reminders/{id}` used to read an
|
||||
* explicit `null` cadence clear as "field omitted" (`?? existing.field`
|
||||
* cannot distinguish the two — both are nullish) and recompute against the
|
||||
* STALE cadence for one cycle, even though the persisted row itself already
|
||||
* carried the correct cleared value.
|
||||
*
|
||||
* Every test below drives the REAL `computeReminderNextDueAt` (not a mock)
|
||||
* so the "correct" and "stale" comparison values are genuine recurrence-
|
||||
* engine output, not hand-picked dates — and asserts the fixtures actually
|
||||
* differ before trusting the route's answer, so a fixture collision can't
|
||||
* hide a regression.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
import { NextRequest } from "next/server";
|
||||
|
||||
vi.mock("@/lib/api-handler", () => ({
|
||||
apiHandler: <T extends (...args: unknown[]) => unknown>(fn: T) => fn,
|
||||
requireAuth: vi.fn(async () => ({ user: { id: "u1", locale: "en" } })),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/auth/audit", () => ({
|
||||
auditLog: vi.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/logging/context", () => ({ annotate: vi.fn() }));
|
||||
|
||||
const findFirstMock = vi.fn();
|
||||
const updateMock = vi.fn();
|
||||
const findUserMock = vi.fn();
|
||||
const auditLogCreateMock = vi.fn();
|
||||
vi.mock("@/lib/db", () => ({
|
||||
prisma: {
|
||||
measurementReminder: {
|
||||
findFirst: (...a: unknown[]) => findFirstMock(...a),
|
||||
update: (...a: unknown[]) => updateMock(...a),
|
||||
},
|
||||
user: { findUnique: (...a: unknown[]) => findUserMock(...a) },
|
||||
auditLog: { create: (...a: unknown[]) => auditLogCreateMock(...a) },
|
||||
},
|
||||
}));
|
||||
|
||||
import { PATCH } from "../route";
|
||||
import { computeReminderNextDueAt } from "@/lib/measurement-reminders/scheduling";
|
||||
|
||||
const BASE_ROW = {
|
||||
id: "r1",
|
||||
userId: "u1",
|
||||
label: "Blutdruck messen",
|
||||
measurementType: "BLOOD_PRESSURE_SYS",
|
||||
intervalDays: 30,
|
||||
rrule: null as string | null,
|
||||
anchorDate: null as Date | null,
|
||||
endsOn: null,
|
||||
origin: "VORSORGE",
|
||||
notifyHour: 9,
|
||||
location: null,
|
||||
nextDueAt: new Date("2026-01-31T09:00:00Z"),
|
||||
lastSatisfiedAt: null as Date | null,
|
||||
enabled: true,
|
||||
createdAt: new Date("2026-01-01T00:00:00Z"),
|
||||
updatedAt: new Date("2026-01-01T00:00:00Z"),
|
||||
deletedAt: null,
|
||||
};
|
||||
|
||||
const NOW = new Date("2026-06-15T08:00:00.000Z");
|
||||
const TZ = "Europe/Berlin";
|
||||
|
||||
function makeRequest(body: unknown): NextRequest {
|
||||
return new NextRequest("http://localhost/api/measurement-reminders/r1", {
|
||||
method: "PATCH",
|
||||
headers: { "content-type": "application/json" },
|
||||
body: JSON.stringify(body),
|
||||
});
|
||||
}
|
||||
|
||||
const params = { params: Promise.resolve({ id: "r1" }) };
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
findUserMock.mockResolvedValue({ timezone: TZ });
|
||||
updateMock.mockImplementation(
|
||||
async ({ data }: { data: Record<string, unknown> }) => ({
|
||||
...BASE_ROW,
|
||||
...data,
|
||||
}),
|
||||
);
|
||||
vi.useFakeTimers();
|
||||
vi.setSystemTime(NOW);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
describe("PATCH /api/measurement-reminders/[id] — nextDueAt honours explicit-null cadence clears (regression iOS #62)", () => {
|
||||
it("interval → RRULE: recomputes off the NEW rrule, not the just-cleared interval", async () => {
|
||||
// Existing rolling reminder: intervalDays=30, rrule=null.
|
||||
findFirstMock.mockResolvedValue(BASE_ROW);
|
||||
|
||||
const res = await PATCH(
|
||||
makeRequest({ intervalDays: null, rrule: "FREQ=YEARLY" }),
|
||||
params,
|
||||
);
|
||||
expect(res.status).toBe(200);
|
||||
|
||||
const persisted = updateMock.mock.calls[0]?.[0]?.data as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
// The explicit-null clear + the new rrule both persist correctly —
|
||||
// this half was never broken.
|
||||
expect(persisted.intervalDays).toBeNull();
|
||||
expect(persisted.rrule).toBe("FREQ=YEARLY");
|
||||
|
||||
const correct = computeReminderNextDueAt(
|
||||
{ ...BASE_ROW, intervalDays: null, rrule: "FREQ=YEARLY" },
|
||||
TZ,
|
||||
NOW,
|
||||
);
|
||||
// The bug: `?? existing` reads the cleared intervalDays as "omitted"
|
||||
// and recomputes against the OLD rolling cadence for one cycle.
|
||||
const staleBug = computeReminderNextDueAt(
|
||||
{ ...BASE_ROW, intervalDays: 30, rrule: null },
|
||||
TZ,
|
||||
NOW,
|
||||
);
|
||||
// Mutation-check guard: the fixtures must actually diverge, or this
|
||||
// test can't tell a fix from a no-op.
|
||||
expect(correct).not.toEqual(staleBug);
|
||||
|
||||
expect(persisted.nextDueAt).toEqual(correct);
|
||||
expect(persisted.nextDueAt).not.toEqual(staleBug);
|
||||
});
|
||||
|
||||
it("RRULE → interval: recomputes off the NEW interval, not the just-cleared rrule (incl. its BYHOUR)", async () => {
|
||||
findFirstMock.mockResolvedValue({
|
||||
...BASE_ROW,
|
||||
intervalDays: null,
|
||||
rrule: "FREQ=DAILY;BYHOUR=7,19",
|
||||
});
|
||||
|
||||
const res = await PATCH(
|
||||
makeRequest({ rrule: null, intervalDays: 14 }),
|
||||
params,
|
||||
);
|
||||
expect(res.status).toBe(200);
|
||||
|
||||
const persisted = updateMock.mock.calls[0]?.[0]?.data as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
expect(persisted.rrule).toBeNull();
|
||||
expect(persisted.intervalDays).toBe(14);
|
||||
|
||||
const correct = computeReminderNextDueAt(
|
||||
{ ...BASE_ROW, intervalDays: 14, rrule: null },
|
||||
TZ,
|
||||
NOW,
|
||||
);
|
||||
const staleBug = computeReminderNextDueAt(
|
||||
{ ...BASE_ROW, intervalDays: null, rrule: "FREQ=DAILY;BYHOUR=7,19" },
|
||||
TZ,
|
||||
NOW,
|
||||
);
|
||||
expect(correct).not.toEqual(staleBug);
|
||||
|
||||
expect(persisted.nextDueAt).toEqual(correct);
|
||||
expect(persisted.nextDueAt).not.toEqual(staleBug);
|
||||
});
|
||||
|
||||
it("explicit anchorDate: null clear recomputes off no-anchor, not the stale anchor", async () => {
|
||||
findFirstMock.mockResolvedValue({
|
||||
...BASE_ROW,
|
||||
intervalDays: 30,
|
||||
anchorDate: new Date("2026-01-15T00:00:00Z"),
|
||||
});
|
||||
|
||||
const res = await PATCH(makeRequest({ anchorDate: null }), params);
|
||||
expect(res.status).toBe(200);
|
||||
|
||||
const persisted = updateMock.mock.calls[0]?.[0]?.data as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
expect(persisted.anchorDate).toBeNull();
|
||||
|
||||
const correct = computeReminderNextDueAt(
|
||||
{ ...BASE_ROW, intervalDays: 30, anchorDate: null },
|
||||
TZ,
|
||||
NOW,
|
||||
);
|
||||
const staleBug = computeReminderNextDueAt(
|
||||
{
|
||||
...BASE_ROW,
|
||||
intervalDays: 30,
|
||||
anchorDate: new Date("2026-01-15T00:00:00Z"),
|
||||
},
|
||||
TZ,
|
||||
NOW,
|
||||
);
|
||||
expect(correct).not.toEqual(staleBug);
|
||||
|
||||
expect(persisted.nextDueAt).toEqual(correct);
|
||||
expect(persisted.nextDueAt).not.toEqual(staleBug);
|
||||
});
|
||||
|
||||
it("omitted cadence fields on a label-only edit leave the cadence — and nextDueAt — unchanged", async () => {
|
||||
findFirstMock.mockResolvedValue(BASE_ROW);
|
||||
|
||||
const res = await PATCH(makeRequest({ label: "New label" }), params);
|
||||
expect(res.status).toBe(200);
|
||||
|
||||
const persisted = updateMock.mock.calls[0]?.[0]?.data as Record<
|
||||
string,
|
||||
unknown
|
||||
>;
|
||||
expect(persisted.label).toBe("New label");
|
||||
// The cadence keys are never written at all on a label-only edit — no
|
||||
// clear, no touch.
|
||||
expect("intervalDays" in persisted).toBe(false);
|
||||
expect("rrule" in persisted).toBe(false);
|
||||
expect("anchorDate" in persisted).toBe(false);
|
||||
|
||||
const expected = computeReminderNextDueAt(BASE_ROW, TZ, NOW);
|
||||
expect(persisted.nextDueAt).toEqual(expected);
|
||||
});
|
||||
});
|
||||
@@ -133,18 +133,35 @@ export const PATCH = apiHandler(
|
||||
// Recompute next-due against the merged cadence. Floor the search at
|
||||
// the last-satisfied instant (or now) so a cadence edit re-anchors
|
||||
// cleanly off the user's last fulfilment.
|
||||
//
|
||||
// v1.32.1 — `Object.hasOwn(updateData, field)` replaces a `?? existing`
|
||||
// fallback here (issue #62). `??` cannot tell an explicit `null` clear
|
||||
// apart from an omitted field — both are nullish — so a PATCH that
|
||||
// cleared `intervalDays` to switch a reminder onto an RRULE still
|
||||
// recomputed `nextDueAt` against the OLD rolling interval for one
|
||||
// cycle (rolling cadence wins the dispatcher's precedence order, so the
|
||||
// stale interval silently overrode the just-set RRULE). The same gap
|
||||
// hit the RRULE → interval direction and an explicit `anchorDate: null`
|
||||
// clear. `updateData` already encodes the field-by-field truth of what
|
||||
// is ACTUALLY being persisted — including the mutual-exclusivity
|
||||
// auto-clear a few lines up — so keying off its own keys (present, even
|
||||
// when the value is `null`) can never diverge from what the database
|
||||
// ends up holding.
|
||||
const timezone = await resolveTimezone(user.id);
|
||||
const now = new Date();
|
||||
const merged: ReminderScheduleInput = {
|
||||
intervalDays:
|
||||
(updateData.intervalDays as number | null | undefined) ??
|
||||
existing.intervalDays,
|
||||
rrule: (updateData.rrule as string | null | undefined) ?? existing.rrule,
|
||||
anchorDate:
|
||||
(updateData.anchorDate as Date | null | undefined) ??
|
||||
existing.anchorDate,
|
||||
notifyHour:
|
||||
(updateData.notifyHour as number | undefined) ?? existing.notifyHour,
|
||||
intervalDays: Object.hasOwn(updateData, "intervalDays")
|
||||
? (updateData.intervalDays as number | null)
|
||||
: existing.intervalDays,
|
||||
rrule: Object.hasOwn(updateData, "rrule")
|
||||
? (updateData.rrule as string | null)
|
||||
: existing.rrule,
|
||||
anchorDate: Object.hasOwn(updateData, "anchorDate")
|
||||
? (updateData.anchorDate as Date | null)
|
||||
: existing.anchorDate,
|
||||
notifyHour: Object.hasOwn(updateData, "notifyHour")
|
||||
? (updateData.notifyHour as number)
|
||||
: existing.notifyHour,
|
||||
lastSatisfiedAt: existing.lastSatisfiedAt,
|
||||
createdAt: existing.createdAt,
|
||||
// v1.18.1 — preserve the course window so an edit of a finite
|
||||
|
||||
Reference in new issue
Block a user