diff --git a/.semgrep/baseline-counts.json b/.semgrep/baseline-counts.json index 1bffd01b..8430508c 100644 --- a/.semgrep/baseline-counts.json +++ b/.semgrep/baseline-counts.json @@ -99,11 +99,11 @@ ], "measured_at": "2026-08-07", "counts": { - "fieldstay-supabase-discarded-result": 83, - "fieldstay-supabase-read-without-error": 168, + "fieldstay-supabase-discarded-result": 79, + "fieldstay-supabase-read-without-error": 150, "fieldstay-supabase-read-without-error-fan-in": 0, - "fieldstay-supabase-unbounded-select-in-list": 34, - "fieldstay-supabase-unbounded-select-org-scoped": 98, + "fieldstay-supabase-unbounded-select-in-list": 32, + "fieldstay-supabase-unbounded-select-org-scoped": 96, "fieldstay-supabase-unbounded-select-single-parent": 29 } } diff --git a/FUTURE_REMEDIATION.md b/FUTURE_REMEDIATION.md index 4fd98505..4ab0502b 100644 --- a/FUTURE_REMEDIATION.md +++ b/FUTURE_REMEDIATION.md @@ -1106,3 +1106,55 @@ turnover is not in the assigned set at all — at which point the existing safe today (assets are pulled narrow and pruned wide, so they are over-retained, never lost), but once the assigned set is bounded, all three should be derived from one helper rather than three hand-rolled sets. + +--- + +## 24. Guest SMS opt-in stores no evidence of the consent it relies on + +**File:** `app/actions/guidebook.ts` (`optInGuestSms`), table +`guidebook_guest_sms_optins` + +Found during the guidebook audit (2026-08-07). Not fixed here: it needs a +migration, and what to retain is a legal question rather than an engineering +one. + +The consent LOGIC is careful — STOP is honoured globally by phone across every +org and booking, re-consent is restricted to the handset (START/YES/UNSTOP), +the revocation record is deliberately kept forever by +`guest-pii-retention`, and a degraded consent read fails closed. What is +missing is the EVIDENCE. + +`guidebook_guest_sms_optins` holds `phone_e164`, `opted_in_at`, +`opted_out_at`, `is_active` and the send bookkeeping — and nothing about the +consent event itself: + +- no record of the disclosure text the guest was actually shown (it lives only + in `app/g/b/[token]/opt-in/opt-in-client.tsx`, and changing that copy leaves + no trace of what earlier opt-ins agreed to) +- no IP address or user agent +- no record of which guidebook token was used + +Under TCPA the burden of proving prior express written consent sits with the +sender. Today the strongest evidence FieldStay could produce for a disputed +opt-in is a row saying a number opted in at a timestamp — with no way to show +what was on screen when it happened, and no way to distinguish the guest from +anyone else holding the booking's guidebook link. + +**Why it was not just added.** Storing an IP and user agent against a phone +number is itself a PII expansion, and `guest-pii-retention` currently deletes +opt-in rows (except revocations) on a schedule — so consent evidence would +need its own retention rule, probably a longer one than the PII it sits +beside. That is a decision for whoever owns the compliance posture, not a +default to pick while auditing. + +**If it is taken on**, the shape that fits the existing design: a +`consent_disclosure_version` (or the literal text hash) written alongside +`opted_in_at`, plus request metadata, and an explicit carve-out in +`lib/inngest/functions/cron/guest-pii-retention.ts` so consent records outlive +the guest data they attest to — mirroring how revocations are already exempt +from that sweep. + +**Adjacent, already fixed 2026-08-07:** the 15-minute number-correction window +was measured from `opted_in_at`, which the upsert refreshes on every +submission, so the window restarted on each resubmit and could be walked +forward indefinitely. It now anchors on the immutable `created_at`. diff --git a/app/actions/guidebook.ts b/app/actions/guidebook.ts index a0bf0072..e517f278 100644 --- a/app/actions/guidebook.ts +++ b/app/actions/guidebook.ts @@ -1,6 +1,7 @@ 'use server' import { createServiceClient } from '@/lib/supabase/server' +import { unwrap } from '@/lib/supabase/unwrap' import { stripe } from '@/lib/stripe/client' import { requireOrgRole } from '@/lib/auth' import { inngest } from '@/lib/inngest/client' @@ -17,20 +18,78 @@ import { reportError } from '@/lib/observability/report-error' * invoked via /api/guidebook/sponsor-checkout rather than called * directly as a Server Action from a client component. */ +/** + * Sponsor states that still carry a LIVE Stripe subscription, so starting a + * fresh checkout would create a second one billing the same business. + * + * `payment_failed` is the non-obvious member. It is set from + * `invoice.payment_failed`, which does NOT end the subscription — Stripe keeps + * it in dunning, and guidebook-sponsor-payment-recovered flips the row + * straight back to 'active' when a retry succeeds. A sponsor who sees the + * failure notice and re-opens their media-kit link was therefore able to buy a + * SECOND subscription while the first was still being retried. + * + * 'cancelled' is deliberately absent: that subscription is gone, and buying + * again is the whole point of keeping the media kit link alive. + */ +const SPONSOR_STATUSES_WITH_LIVE_SUBSCRIPTION = ['active', 'payment_failed'] as const + +/** + * Returns the URL of an already-created Checkout Session if it is still + * payable, so a repeat click reuses it instead of minting another. + * + * Same helper the work-order invoice route has had all along + * (app/api/invoices/[invoiceId]/checkout/route.ts, "Store the session ID for + * potential reuse on duplicate clicks"). The sponsor path WROTE + * checkout_session_id for exactly this purpose and then never read it back + * from anywhere in the codebase — so every click, reload, or retry minted a + * new session, each payable for 24 hours. Two of them paid means two + * subscriptions, two checkout.session.completed webhooks with distinct event + * ids (so the dedup table does not collapse them), and an activation handler + * that overwrites stripe_subscription_id with whichever lands last — leaving + * the other subscription billing monthly with nothing in FieldStay able to + * cancel it. + */ +async function openSessionUrl(sessionId: string | null): Promise { + if (!sessionId) return null + try { + const existing = await stripe.checkout.sessions.retrieve(sessionId) + return existing.status === 'open' ? existing.url : null + } catch { + // Expired or not found — the caller mints a new one. + return null + } +} + export async function createSponsorCheckoutSession( mediaKitToken: string ): Promise<{ url: string } | { error: string }> { try { const supabase = createServiceClient({ publicSurface: 'guidebook-sponsor-media-kit' }) - const { data: sponsor } = await supabase + // maybeSingle + unwrap, not `{ data }` off .single(): discarding the error + // made an outage or an RLS regression indistinguishable from a bad token, + // so a sponsor holding a perfectly valid link was told it was invalid. + // Identical to the defect already fixed in optInGuestSms below — same + // file, one function over. + const sponsorRes = await supabase .from('guidebook_sponsors') - .select('id, org_id, business_name, slot_type, status') + .select('id, org_id, business_name, slot_type, status, checkout_session_id') .eq('media_kit_token', mediaKitToken) - .single() + .maybeSingle() - if (!sponsor) return { error: 'Invalid media kit link.' } - if (sponsor.status === 'active') return { error: 'This sponsorship slot is already active.' } + const sponsor = unwrap(sponsorRes, { + site: 'serverAction.guidebook.createSponsorCheckoutSession.sponsor', + }) + + if (!sponsor) return { error: 'Invalid media kit link.' } + + if ((SPONSOR_STATUSES_WITH_LIVE_SUBSCRIPTION as readonly string[]).includes(sponsor.status)) { + return { error: 'This sponsorship slot is already active.' } + } + + const reusable = await openSessionUrl(sponsor.checkout_session_id) + if (reusable) return { url: reusable } const appUrl = process.env.NEXT_PUBLIC_APP_URL ?? 'https://app.fieldstay.app' @@ -57,7 +116,16 @@ export async function createSponsorCheckoutSession( if (!session.url) return { error: 'Stripe did not return a checkout URL.' } - await supabase + // Compare-and-swap on the value we read, not a blind overwrite. The reuse + // check above closes the repeat-click case; this closes the concurrent one + // — two requests that both saw no reusable session and both created one. + // Whichever swap matches the value it read owns the slot; the loser expires + // its own session and hands back the winner's, so only ONE payable session + // for this sponsor exists at a time. A plain `if (already active)` before + // the write is exactly the TOCTOU the audit checklist calls out: the + // precondition has to be in the WHERE clause. + const priorSessionId = sponsor.checkout_session_id + const claim = supabase .from('guidebook_sponsors') .update({ checkout_session_id: session.id, @@ -66,6 +134,49 @@ export async function createSponsorCheckoutSession( .eq('id', sponsor.id) .eq('org_id', sponsor.org_id) // explicit tenant guard + const claimRes = await ( + priorSessionId + ? claim.eq('checkout_session_id', priorSessionId) + : claim.is('checkout_session_id', null) + ) + .select('id') + .maybeSingle() + + const claimed = unwrap(claimRes, { + site: 'serverAction.guidebook.createSponsorCheckoutSession.claim', + orgId: sponsor.org_id, + }) + + if (!claimed) { + // Lost the swap: a concurrent request stored its session after we read. + // Expire ours so it can never be paid, and return theirs. + await stripe.checkout.sessions.expire(session.id).catch((err: unknown) => { + // Non-fatal, but it must not be silent — an un-expired orphan session + // is precisely the double-subscription risk this block exists to close. + reportError(err, { + site: 'serverAction.guidebook.createSponsorCheckoutSession.expire-orphan', + orgId: sponsor.org_id, + }) + }) + + const currentRes = await supabase + .from('guidebook_sponsors') + .select('checkout_session_id') + .eq('id', sponsor.id) + .eq('org_id', sponsor.org_id) + .maybeSingle() + + const winnerUrl = await openSessionUrl( + unwrap(currentRes, { + site: 'serverAction.guidebook.createSponsorCheckoutSession.reread', + orgId: sponsor.org_id, + })?.checkout_session_id ?? null + ) + + if (winnerUrl) return { url: winnerUrl } + return { error: 'Unable to start checkout. Please try again.' } + } + // Unauthenticated flow (media kit page has no PM session) — no actorId await logAuditEvent({ orgId: sponsor.org_id, @@ -355,6 +466,12 @@ export async function updateStayExtensionSettings( * How long after the original opt-in a guest may correct the number they * entered. Long enough to notice a typo and resubmit; far short of the span * over which a guidebook link circulates. + * + * Measured from guidebook_guest_sms_optins.created_at, which the upsert never + * writes, rather than opted_in_at, which it refreshes on every submission. + * From opted_in_at the window was not 15 minutes from the opt-in at all — it + * was 15 minutes from the LAST submission, so resubmitting inside it restarted + * the clock and the window could be walked forward without limit. */ const OPTIN_CORRECTION_WINDOW_MS = 15 * 60 * 1000 @@ -373,12 +490,24 @@ export async function optInGuestSms( const supabase = createServiceClient({ publicSurface: 'guidebook-guest-sms-optin' }) - const { data: booking } = await supabase + // Binds its error, like the two consent reads below it. Discarding it made + // a transient failure indistinguishable from a bad token, so a guest + // holding a perfectly valid link was told the link was invalid — with + // nothing logged and nothing reported. The two reads that follow this one + // both already failed closed with an explicit note about why; this was the + // odd one out. + const bookingRes = await supabase .from('bookings') .select('id, org_id, property_id') .eq('guidebook_token', guidebookToken) .maybeSingle() + if (bookingRes.error) { + console.error('[optInGuestSms] booking lookup', bookingRes.error.message) + reportError(bookingRes.error, { site: 'serverAction.guidebook.optInGuestSms.booking' }) + return { error: 'Something went wrong. Please try again.' } + } + const booking = bookingRes.data if (!booking) return { error: 'Invalid guidebook link.' } // ── Consent gate 1: has this NUMBER revoked consent anywhere? ──────────── @@ -424,9 +553,15 @@ export async function optInGuestSms( // corrections stay open for a short window after the original opt-in. That // covers the real case — a typo is noticed immediately — while refusing a // repoint days later, which is what a leaked link enables. + // created_at, not opted_in_at. The upsert below REFRESHES opted_in_at on + // every submission, so a window measured from it walks forward: repoint at + // 14 minutes, and the 15-minute clock restarts from there, indefinitely. + // created_at carries the original row's timestamp and the upsert never + // names it, so it is the only immutable anchor here — and "how long after + // the ORIGINAL opt-in" is what this window is documented to mean. const existingRes = await supabase .from('guidebook_guest_sms_optins') - .select('id, phone_e164, opted_in_at') + .select('id, phone_e164, created_at') .eq('booking_id', booking.id) .maybeSingle() @@ -437,8 +572,8 @@ export async function optInGuestSms( const existing = existingRes.data if (existing && existing.phone_e164 !== phoneE164) { - const optedInAt = existing.opted_in_at ? new Date(existing.opted_in_at).getTime() : 0 - if (Date.now() - optedInAt > OPTIN_CORRECTION_WINDOW_MS) { + const firstOptedInAt = existing.created_at ? new Date(existing.created_at).getTime() : 0 + if (Date.now() - firstOptedInAt > OPTIN_CORRECTION_WINDOW_MS) { // Never echo either number back — guest PII, and confirming which // number is on file would itself be a disclosure. return { error: 'A different number is already signed up for this stay. Contact your host to change it.' } diff --git a/app/api/crew/inventory-count/route.ts b/app/api/crew/inventory-count/route.ts index e033ec7b..4cc5ade4 100644 --- a/app/api/crew/inventory-count/route.ts +++ b/app/api/crew/inventory-count/route.ts @@ -4,6 +4,7 @@ import { inngest } from '@/lib/inngest/client' import { logAuditEvents } from '@/lib/audit' import { reportQueryError, unwrapList } from '@/lib/supabase/unwrap' import { fetchAllRows } from '@/lib/inngest/paginate' +import { UUID_RE } from '@/lib/validation/uuid' /** * POST /api/crew/inventory-count @@ -37,7 +38,7 @@ interface CountSubmission { itemNotes?: Record } -const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + /** Double-tap window for clients that submit without a count id. */ const DEDUP_WINDOW_MS = 5 * 60 * 1000 diff --git a/app/api/crew/messages/route.ts b/app/api/crew/messages/route.ts index aa0f9fd0..9fc990a8 100644 --- a/app/api/crew/messages/route.ts +++ b/app/api/crew/messages/route.ts @@ -4,6 +4,7 @@ import { requireCrewMember } from '@/lib/crew-auth' import { createServiceClient } from '@/lib/supabase/server' import { getPmMembers } from '@/lib/inngest/helpers' import { reportError } from '@/lib/observability/report-error' +import { UUID_RE } from '@/lib/validation/uuid' /** * POST /api/crew/messages @@ -110,7 +111,7 @@ async function notifyPmSlack( * (/api/crew/inventory-count) already validates its own, and this one is the * copy that did not. */ -const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + /** * `content` is unbounded `text` in the database and has no maxLength on the diff --git a/app/api/crew/work-orders/[id]/complete/route.ts b/app/api/crew/work-orders/[id]/complete/route.ts index 8a125973..2a7afa63 100644 --- a/app/api/crew/work-orders/[id]/complete/route.ts +++ b/app/api/crew/work-orders/[id]/complete/route.ts @@ -11,6 +11,7 @@ import { type CompletedWorkOrderRow, } from '@/app/(dashboard)/maintenance/complete-work-order-helpers' import type { WoStatus } from '@/types/database' +import { UUID_RE } from '@/lib/validation/uuid' /** * POST /api/crew/work-orders/[id]/complete @@ -33,7 +34,7 @@ import type { WoStatus } from '@/types/database' * Not reachable from our own client (the PWA builds the URL from cached * crew_work_orders ids), which is why it is asserted rather than assumed. */ -const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + /** Free text typed on a phone into an unbounded `completion_notes` column. */ const MAX_NOTES_LENGTH = 2000 diff --git a/app/api/guidebook/redeem/route.ts b/app/api/guidebook/redeem/route.ts index fb903dce..05ea035e 100644 --- a/app/api/guidebook/redeem/route.ts +++ b/app/api/guidebook/redeem/route.ts @@ -4,6 +4,7 @@ import { guidebookRedeemLimiter, checkLimit } from '@/lib/rate-limit' import { extractClientIp } from '@/lib/integrations/webhook-verification' import { reportError } from '@/lib/observability/report-error' import { unwrap } from '@/lib/supabase/unwrap' +import { isUuid } from '@/lib/validation/uuid' /** * POST /api/guidebook/redeem @@ -27,7 +28,16 @@ export async function POST(req: NextRequest): Promise { } const body = await req.json().catch(() => null) as { sponsorId?: string; bookingToken?: string | null } | null - if (!body?.sponsorId || typeof body.sponsorId !== 'string') { + + // Shape-checked before it reaches a `uuid` column, not just type-checked. + // `guidebook_sponsors.id` is a uuid, so a non-UUID string is Postgres 22P02 + // — which unwrap() below turns into a throw, and the catch turns into + // `{ok:true}` PLUS a Sentry report. On a public, unauthenticated endpoint + // that is a free way for anyone to burn the Sentry quota and bury this + // route's real database failures in noise; the limiter bounds it only while + // Redis is up, and this one deliberately fails OPEN. A malformed id is a bad + // request, so say so. + if (!isUuid(body?.sponsorId)) { return NextResponse.json({ error: 'sponsorId is required' }, { status: 400 }) } @@ -47,7 +57,15 @@ export async function POST(req: NextRequest): Promise { } let bookingId: string | null = null - if (body.bookingToken && typeof body.bookingToken === 'string') { + // isUuid, not just a truthy string check — and note this one SKIPS rather + // than 400s. `bookings.guidebook_token` is also a uuid, so a malformed + // token threw 22P02 out of unwrap(), escaped the try entirely, and landed + // in the outer catch: the guest got `{ok:true}` and the redemption insert + // below never ran at all. The stated intent two lines down is to fall back + // to an ANONYMOUS redemption when the booking can't be attributed, so + // that is what an unusable token should do — degrade attribution, not + // discard the redemption. + if (isUuid(body.bookingToken)) { const bookingRes = await supabase .from('bookings') .select('id, org_id') @@ -59,10 +77,39 @@ export async function POST(req: NextRequest): Promise { if (booking && booking.org_id === sponsor.org_id) bookingId = booking.id } - const { error } = await supabase.from('guidebook_offer_redemptions').insert({ - org_id: sponsor.org_id, - sponsor_id: sponsor.id, - booking_id: bookingId, + // Insert-or-increment, as ONE statement (migration 20260807170000). + // + // The row is written when the guest opens the redemption pass — the coupon + // they show at the counter — and opening it more than once is the NORMAL + // case: look at the offer from the couch, close it, walk over, open it + // again for staff. That gives two genuinely different numbers, and the + // sponsor wants both: COUNT(*) is redemptions (deduped per booking per day + // by uniq_guidebook_offer_redemptions_sponsor_booking_day), SUM(open_count) + // is engagement. + // + // Per day, not per stay: a daily perk is legitimately redeemable on each + // day of a booking, so collapsing the whole stay would under-count. + // + // An RPC rather than the JS client, for two reasons. The arbiter is a + // PARTIAL EXPRESSION index (on the UTC date of opened_at) and PostgREST's + // on_conflict takes only plain column names, so this upsert is not + // expressible through the client at all. And read-then-write here would be + // a TOCTOU — two taps racing would both read 1 and both write 2. + // + // Anonymous redemptions (bookingId null, from the property-level + // /g/[slug] guidebook) fall outside the partial index by design — with no + // guest identity there is nothing to dedupe on, and collapsing them by + // (sponsor, day) would merge different guests into one. Each is its own + // row at open_count = 1, so both aggregates still read correctly. + // p_booking_id is OMITTED rather than passed as null for the anonymous + // case — the function declares `DEFAULT NULL` precisely so this is + // expressible. Supabase's type generator has no notion of a nullable + // argument, so without the default the only ways to call this with a null + // would be a cast or a second write path. + const { error } = await supabase.rpc('record_guidebook_offer_open', { + p_org_id: sponsor.org_id, + p_sponsor_id: sponsor.id, + ...(bookingId ? { p_booking_id: bookingId } : {}), }) if (error) { diff --git a/app/api/guidebook/sponsor-checkout/route.ts b/app/api/guidebook/sponsor-checkout/route.ts index 64d96ac4..de7989ca 100644 --- a/app/api/guidebook/sponsor-checkout/route.ts +++ b/app/api/guidebook/sponsor-checkout/route.ts @@ -3,6 +3,7 @@ import { createSponsorCheckoutSession } from '@/app/actions/guidebook' import { guidebookSponsorCheckoutLimiter, checkLimit } from '@/lib/rate-limit' import { extractClientIp } from '@/lib/integrations/webhook-verification' import { reportError } from '@/lib/observability/report-error' +import { isUuid } from '@/lib/validation/uuid' export async function POST(req: NextRequest): Promise { // Throttle BEFORE the token is read, so a guessing attack is capped by the @@ -31,9 +32,17 @@ export async function POST(req: NextRequest): Promise { try { const body = await req.json() as { mediaKitToken?: string } - if (!body.mediaKitToken || typeof body.mediaKitToken !== 'string') { + // Shape-checked, not just type-checked. guidebook_sponsors.media_kit_token + // is a `uuid`, so a malformed token reaches `.eq()` as Postgres 22P02, + // throws out of the action's unwrap(), and lands in its catch — which + // reports to Sentry and tells the sponsor "Unable to start checkout. + // Please try again." for a link that will never work no matter how many + // times they try. On a public unauthenticated endpoint that is also a free + // way to burn the Sentry quota. An unusable token is an invalid link, and + // gets the same message a nonexistent one does. + if (!isUuid(body.mediaKitToken)) { return NextResponse.json( - { error: 'mediaKitToken is required' }, + { error: 'Invalid media kit link.' }, { status: 400 } ) } diff --git a/app/g/[slug]/page.tsx b/app/g/[slug]/page.tsx index e40476d1..88c7eacc 100644 --- a/app/g/[slug]/page.tsx +++ b/app/g/[slug]/page.tsx @@ -9,6 +9,7 @@ import type { GuidebookSponsorView } from '@/components/guidebook/guest-guideboo import type { GuidebookSponsor, GuidebookPropertyConfig, Property } from '@/types/database' import { unwrap, unwrapList } from '@/lib/supabase/unwrap' +/** Only for a property with no timezone — see the note in app/g/b/[token]/page.tsx. */ const FALLBACK_TIMEZONE = 'America/New_York' // A guidebook's active sponsor slots are a small, curated set per org; the @@ -30,7 +31,7 @@ function heroPhotoUrl(path: string | null | undefined): string | null { const CONFIG_FIELDS = ` id, slug, wifi_network, wifi_password, check_in_instructions, check_out_instructions, house_rules, is_published, org_id, - properties(id, name, address, lat, lng, checkin_time, checkout_time) + properties(id, name, address, lat, lng, timezone, checkin_time, checkout_time) ` const getGuidebookConfig = cache(async (slug: string) => { @@ -114,8 +115,9 @@ export default async function GuestGuidebookPage({ const sponsors = unwrapList(sponsorsRes, { site: 'page.g.slug', orgId: config.org_id }) const hourOfDay = Number( - new Intl.DateTimeFormat('en-US', { hour: 'numeric', hour12: false, timeZone: FALLBACK_TIMEZONE }) - .format(new Date()) + new Intl.DateTimeFormat('en-US', { + hour: 'numeric', hour12: false, timeZone: property.timezone || FALLBACK_TIMEZONE, + }).format(new Date()) ) const weather = property.lat && property.lng diff --git a/app/g/b/[token]/page.tsx b/app/g/b/[token]/page.tsx index 40abf856..09bb2e3c 100644 --- a/app/g/b/[token]/page.tsx +++ b/app/g/b/[token]/page.tsx @@ -8,6 +8,15 @@ import { GuidebookUnavailable } from '@/components/guidebook/guidebook-unavailab import type { GuidebookSponsorView } from '@/components/guidebook/guest-guidebook-view' import type { GuidebookSponsor, GuidebookPropertyConfig, Property } from '@/types/database' +/** + * Used only when a property somehow has no timezone. It is NOT the default: + * computing "what time is it for this guest" in Eastern for every property in + * the country is how a Central property's guidebook reads midnight at 11pm + * local — and hourOfDay selects which SPONSOR SLOTS are shown, so the wrong + * hour shows the wrong paying sponsors. Production is already 4 of 27 + * properties in America/Chicago, and the error grows westward: two hours in + * Mountain, three in Pacific, five in Hawaii. + */ const FALLBACK_TIMEZONE = 'America/New_York' const SUPABASE_URL = process.env.NEXT_PUBLIC_SUPABASE_URL @@ -24,10 +33,13 @@ function heroPhotoUrl(path: string | null | undefined): string | null { type StayPhase = 'arrival' | 'mid' | 'checkout' -function computeStay(checkinDate: string, checkoutDate: string): { +export function computeStay(checkinDate: string, checkoutDate: string, timeZone: string): { phase: StayPhase; nightIndex: number; totalNights: number } { - const today = new Intl.DateTimeFormat('en-CA', { timeZone: FALLBACK_TIMEZONE }).format(new Date()) // YYYY-MM-DD + // The guest's local date, not the server's and not Eastern's. On checkout + // eve a Central property crossed into `checkout` phase an hour early, so the + // guidebook showed checkout instructions to a guest still mid-stay. + const today = new Intl.DateTimeFormat('en-CA', { timeZone }).format(new Date()) // YYYY-MM-DD const totalNights = Math.max(1, Math.round( (Date.parse(checkoutDate) - Date.parse(checkinDate)) / 86_400_000 )) @@ -42,7 +54,7 @@ function computeStay(checkinDate: string, checkoutDate: string): { const CONFIG_FIELDS = ` id, slug, wifi_network, wifi_password, check_in_instructions, check_out_instructions, house_rules, is_published, org_id, - properties(id, name, address, lat, lng, checkin_time, checkout_time) + properties(id, name, address, lat, lng, timezone, checkin_time, checkout_time) ` const getGuidebookData = cache(async (token: string) => { @@ -139,8 +151,10 @@ export default async function GuestBookingGuidebookPage({ .eq('org_id', booking.org_id) .eq('status', 'active') + const timeZone = property.timezone || FALLBACK_TIMEZONE + const hourOfDay = Number( - new Intl.DateTimeFormat('en-US', { hour: 'numeric', hour12: false, timeZone: FALLBACK_TIMEZONE }) + new Intl.DateTimeFormat('en-US', { hour: 'numeric', hour12: false, timeZone }) .format(new Date()) ) @@ -174,7 +188,7 @@ export default async function GuestBookingGuidebookPage({ hourOfDay={hourOfDay} weather={weather} heroPhotoUrl={heroPhotoUrl(config.hero_photo_storage_path)} - stay={computeStay(booking.checkin_date, booking.checkout_date)} + stay={computeStay(booking.checkin_date, booking.checkout_date, timeZone)} bookingToken={token} extensionRequest={extensionRequest ?? null} extensionConfig={ diff --git a/app/g/kit/[media_kit_token]/page.tsx b/app/g/kit/[media_kit_token]/page.tsx index 33c24376..45cb0a17 100644 --- a/app/g/kit/[media_kit_token]/page.tsx +++ b/app/g/kit/[media_kit_token]/page.tsx @@ -2,6 +2,7 @@ import { unwrap } from '@/lib/supabase/unwrap' import { notFound } from 'next/navigation' import { Archivo, Source_Serif_4 } from 'next/font/google' import { createServiceClient } from '@/lib/supabase/server' +import { isUuid } from '@/lib/validation/uuid' import { MediaKitClient } from './media-kit-client' import type { GuidebookSponsor } from '@/types/database' @@ -25,6 +26,15 @@ export default async function MediaKitPage({ params: Promise<{ media_kit_token: string }> }) { const { media_kit_token } = await params + + // media_kit_token is a `uuid` column, so a malformed one is Postgres 22P02 — + // which unwrap() turns into a throw and the segment error boundary renders + // as "something went wrong". That is the exact inversion of the note below: + // an outage must not read as an invalid token, but neither may a genuinely + // invalid token read as an outage. Shape-check first and 404 it, so each of + // the two failures gets its own honest surface. + if (!isUuid(media_kit_token)) notFound() + const supabase = createServiceClient({ publicSurface: 'g-kit--media-kit-token-' }) // Token-gated public page: a failed read used to fall into notFound(), diff --git a/app/g/kit/[media_kit_token]/print/page.tsx b/app/g/kit/[media_kit_token]/print/page.tsx index f3dbb600..b1820d85 100644 --- a/app/g/kit/[media_kit_token]/print/page.tsx +++ b/app/g/kit/[media_kit_token]/print/page.tsx @@ -1,6 +1,7 @@ import { notFound } from 'next/navigation' import { Archivo, Source_Serif_4 } from 'next/font/google' import { createServiceClient } from '@/lib/supabase/server' +import { isUuid } from '@/lib/validation/uuid' import { unwrap } from '@/lib/supabase/unwrap' import { PrintKit } from './print-kit' import type { GuidebookSponsor } from '@/types/database' @@ -25,6 +26,12 @@ export default async function PrintKitPage({ params: Promise<{ media_kit_token: string }> }) { const { media_kit_token } = await params + + // Same shape check as the non-print page — media_kit_token is a `uuid`, so a + // malformed one is 22P02, which unwrap() escalates to the error boundary + // rather than the 404 an invalid link deserves. + if (!isUuid(media_kit_token)) notFound() + const supabase = createServiceClient({ publicSurface: 'g-kit--media-kit-token-print' }) const sponsorRes = await supabase diff --git a/components/guidebook/guest-guidebook-view.tsx b/components/guidebook/guest-guidebook-view.tsx index fe60672f..08e11700 100644 --- a/components/guidebook/guest-guidebook-view.tsx +++ b/components/guidebook/guest-guidebook-view.tsx @@ -8,6 +8,7 @@ import { getActiveSlotTypes, getTimeOfDay } from '@/lib/weather/tomorrow' import { formatOffer } from '@/lib/guidebook/offer' import { CopyButton } from './copy-button' import styles from './guest-guidebook-view.module.css' +import { formatTime12h } from '@/lib/utils/time-of-day' const CHARCOAL = '#0E0E10' const CARD = '#17171A' @@ -45,17 +46,6 @@ const WHY_CHIP: Record = { other: "Today's pick", } -function formatTime12h(time: string | null | undefined): string | null { - if (!time) return null - const [hourStr, minuteStr] = time.split(':') - const hour = Number(hourStr) - const minute = Number(minuteStr) - if (Number.isNaN(hour) || Number.isNaN(minute)) return null - const period = hour >= 12 ? 'PM' : 'AM' - const displayHour = hour % 12 === 0 ? 12 : hour % 12 - return `${displayHour}:${minute.toString().padStart(2, '0')} ${period}` -} - function formatClock(d: Date): string { let hour = d.getHours() const minute = d.getMinutes().toString().padStart(2, '0') diff --git a/lib/guidebook/offer.ts b/lib/guidebook/offer.ts index 425a97c1..d662ce75 100644 --- a/lib/guidebook/offer.ts +++ b/lib/guidebook/offer.ts @@ -1,5 +1,44 @@ import type { GuidebookOfferType } from '@/types/database' +/** + * Every member of GuidebookOfferType, as a Record so TypeScript enforces + * EXHAUSTIVENESS: add a value to the union and this stops compiling until it + * is listed here. + * + * That, not lookup speed, is the reason this is not a plain array. A + * five-element array's `.includes()` is if anything faster than a Set — but an + * array has no compile-time link back to the union, so a newly added offer + * type would silently fall through asOfferType() to 'none' and every sponsor + * using it would render no offer at all, with nothing failing anywhere. The + * fallback that makes unknown input safe is exactly what makes a MISSING entry + * invisible, so the list has to be checked by the compiler rather than by + * whoever remembers to update it. + */ +const OFFER_TYPES: Record = { + percentage: true, + fixed_amount: true, + item: true, + custom: true, + none: true, +} + +const OFFER_TYPE_KEYS = new Set(Object.keys(OFFER_TYPES)) + +/** + * Narrows guidebook_sponsors.offer_type — a TEXT column with a CHECK + * constraint, so PostgREST hands it back as a bare `string` — to the union the + * formatters actually branch on. + * + * Same pattern and same reason as asExtensionContactMethod for + * extension_contact_method. Unrecognised input falls back to 'none', which + * formatOffer already treats as "no offer to show" — the safe direction: a + * sponsor line is omitted rather than rendered from a value nothing + * understands. + */ +export function asOfferType(value: string | null | undefined): GuidebookOfferType { + return OFFER_TYPE_KEYS.has(value ?? '') ? (value as GuidebookOfferType) : 'none' +} + function formatOfferPrice(value: number): string { return value % 1 === 0 ? String(value) : value.toFixed(2) } diff --git a/lib/inngest/functions/guidebook-guest-opted-in.ts b/lib/inngest/functions/guidebook-guest-opted-in.ts index d55f980b..7e805222 100644 --- a/lib/inngest/functions/guidebook-guest-opted-in.ts +++ b/lib/inngest/functions/guidebook-guest-opted-in.ts @@ -86,10 +86,9 @@ export const guidebookGuestOptedIn = inngest.createFunction( door_code: doorCode, portal_url: portalUrl, }) - const result = await sendSMS(phoneE164, body, { orgId: property.org_id }) - - if (!result.sent) { - // SMS failed — roll back the claim so a retry can attempt again + // Releases the one-shot claim so a later run can send. Both exits below + // need it, for opposite reasons. + const releaseClaim = async (): Promise => { const { error: rollbackError } = await supabase .from('guidebook_guest_sms_optins') .update({ door_code_sent_at: null }) @@ -102,8 +101,37 @@ export const guidebookGuestOptedIn = inngest.createFunction( orgId: property.org_id, }) } + } - throw new Error(`SMS send failed: ${result.reason ?? 'unknown'}`) + // sendSMS THROWS on a real send failure — dispatchToTelnyx throws on a + // timeout or any non-2xx. That throw used to escape this step with the + // claim still held, so the Inngest retry hit + // `.is('door_code_sent_at', null)`, matched zero rows, and returned + // `already_sent` — reporting success for a door code the guest never + // received, and never trying again. That is the exact failure the + // comment above the claim describes for a different case; this was the + // same shape one layer down, still open. + let result + try { + result = await sendSMS(phoneE164, body, { orgId: property.org_id }) + } catch (err) { + await releaseClaim() + throw err + } + + if (!result.sent) { + // NOT a failure. sendSMS only returns sent:false for a DELIBERATE + // skip — SMS_ENABLED off, the daily nudge budget, demo-org + // suppression — because every real failure throws (above). Throwing + // here turned a config state into a retried failure: with + // SMS_ENABLED=false, every guest opt-in produced a failing run + // reading "SMS send failed", which is both untrue and noise that + // would mask the real failures once SMS is switched on. + // + // Release the claim so the send can happen once the skip no longer + // applies, and end cleanly — a retry cannot change an env var. + await releaseClaim() + return { skipped: result.reason ?? 'not_sent' } } return { sent: true } diff --git a/lib/inngest/functions/guidebook-sms-evening-cron.ts b/lib/inngest/functions/guidebook-sms-evening-cron.ts index 55ee777b..e8cf4f0e 100644 --- a/lib/inngest/functions/guidebook-sms-evening-cron.ts +++ b/lib/inngest/functions/guidebook-sms-evening-cron.ts @@ -2,14 +2,15 @@ import { asBooleanMap } from '@/lib/json' import { inngest } from '@/lib/inngest/client' import { fetchAllRows } from '@/lib/inngest/paginate' import { createServiceClient } from '@/lib/supabase/server' +import { unwrap } from '@/lib/supabase/unwrap' import { getWeatherForLocation } from '@/lib/weather/tomorrow' import { sendSMS, buildSponsorLine } from '@/lib/sms/telnyx' import { renderSmsBody } from '@/lib/sms/templates' -import { claimDailySmsSlot, releaseDailySmsSlot } from '@/lib/sms/optin-claim' -import { pickNearestSponsor } from '@/lib/sms/pick-nearest-sponsor' +import { sendClaimedDailySms } from '@/lib/sms/optin-claim' +import { pickNearestSponsor, SPONSOR_POOL_COLUMNS, type SponsorPoolRow } from '@/lib/sms/pick-nearest-sponsor' import { unwrapJoin } from '@/lib/utils/supabase-joins' import { getFeaturedAmenityLine } from '@/lib/guidebook/featured-amenities' -import type { GuidebookSponsor } from '@/types/database' +import { asOfferType } from '@/lib/guidebook/offer' const FALLBACK_TIMEZONE = 'America/New_York' @@ -111,18 +112,32 @@ export const guidebookSmsEveningSend = inngest.createFunction( // Re-fetch instead of trusting the dispatch-time snapshot: is_active // may have flipped (guest texted STOP) since the cron ran. - const { data: optin } = await supabase + // Unwrapped, not destructured — the same pairing as the morning send. + // `{ data: optin }` collapsed "this guest opted out" and "the consent + // read failed" into the same null, and both ended at `return false`, so + // a transient failure silently suppressed the message with nothing + // logged and no retry. Opting out is final; a failed read must retry. + const optinRes = await supabase .from('guidebook_guest_sms_optins') .select('id, phone_e164, is_active') .eq('id', optinId) .maybeSingle() + + const optin = unwrap(optinRes, { + site: 'inngest.guidebook-sms-evening-send.optin', orgId, + }) if (!optin?.is_active) return false - const { data: property } = await supabase + const propertyRes = await supabase .from('properties') .select('id, name, lat, lng, amenities') .eq('id', propertyId) + .eq('org_id', orgId) .maybeSingle() + + const property = unwrap(propertyRes, { + site: 'inngest.guidebook-sms-evening-send.property', orgId, + }) if (!property?.lat || !property?.lng) return false const weather = await getWeatherForLocation(property.lat, property.lng).catch(() => null) @@ -139,14 +154,26 @@ export const guidebookSmsEveningSend = inngest.createFunction( rotationOffset: 1, }) - const { data: sponsorsData } = await supabase - .from('guidebook_sponsors') - .select('id, org_id, business_name, offer_type, offer_value, offer_item, custom_offer_text, lat, lng, slot_type') - .eq('org_id', orgId) - .eq('status', 'active') - .in('slot_type', ['dinner_pints', 'rainy_day', 'general']) - - const orgSponsors = (sponsorsData ?? []) as GuidebookSponsor[] + // Bound and error-handled for the same reasons as the morning send's + // pool: discarding the error made a failed lookup indistinguishable + // from an org with no sponsors, and both end at "no offer" -> no SMS. + // fetchAllRows, not a bare select. guidebook_sponsors is capped at SIX + // rows per org by the schema itself (slot_number CHECK 1..6 plus + // UNIQUE(org_id, slot_number)), so this drains in exactly one request — + // the pagination costs nothing at current scale and stops the read from + // resting on that cap. If the slot ceiling is ever raised, a .limit() + // would have started truncating silently; this throws instead. + const orgSponsors = await fetchAllRows( + (from, to) => supabase + .from('guidebook_sponsors') + .select(SPONSOR_POOL_COLUMNS) + .eq('org_id', orgId) + .eq('status', 'active') + .in('slot_type', ['dinner_pints', 'rainy_day', 'general']) + .order('id') + .range(from, to), + { label: 'guidebook-sms-evening-send.sponsors' }, + ) // Rain → dinner → general fallback const primarySlot = isRainy ? 'rainy_day' : 'dinner_pints' @@ -158,7 +185,7 @@ export const guidebookSmsEveningSend = inngest.createFunction( const sponsorLine = picked ? buildSponsorLine( picked.sponsor.business_name, - picked.sponsor.offer_type, + asOfferType(picked.sponsor.offer_type), picked.sponsor.offer_value, picked.sponsor.offer_item, picked.sponsor.custom_offer_text, @@ -174,20 +201,20 @@ export const guidebookSmsEveningSend = inngest.createFunction( // Claim the slot atomically before sending — a retry of this step // after a successful send now finds the slot already claimed and // skips re-sending, instead of double-texting the guest. - const claimed = await claimDailySmsSlot(supabase, optinId, 'last_evening_sms_date', todayDate) - if (!claimed) return false - - const templateKey = isRainy && primaryPool.length > 0 ? 'rain_alert' as const : 'evening_nudge' as const - const eveningBody = await renderSmsBody(orgId, templateKey, { - property_name: property.name, - offer_line: offerLine, - }) - const res = await sendSMS(optin.phone_e164, eveningBody, { category: 'nudge', orgId }) - - if (!res.sent) { - await releaseDailySmsSlot(supabase, optinId, 'last_evening_sms_date') - } - return res.sent + // Rendering is INSIDE the claimed section: it can throw too, and + // releasing only around sendSMS left the day's slot claimed for a + // template failure just the same. + return await sendClaimedDailySms( + supabase, optinId, 'last_evening_sms_date', todayDate, + async () => { + const templateKey = isRainy && primaryPool.length > 0 ? 'rain_alert' as const : 'evening_nudge' as const + const eveningBody = await renderSmsBody(orgId, templateKey, { + property_name: property.name, + offer_line: offerLine, + }) + return await sendSMS(optin.phone_e164, eveningBody, { category: 'nudge', orgId }) + }, + ) }) return { optinId, sent } diff --git a/lib/inngest/functions/guidebook-sms-morning-cron.ts b/lib/inngest/functions/guidebook-sms-morning-cron.ts index 5b4ee98d..8aa00652 100644 --- a/lib/inngest/functions/guidebook-sms-morning-cron.ts +++ b/lib/inngest/functions/guidebook-sms-morning-cron.ts @@ -2,14 +2,16 @@ import { asBooleanMap } from '@/lib/json' import { inngest } from '@/lib/inngest/client' import { fetchAllRows } from '@/lib/inngest/paginate' import { createServiceClient } from '@/lib/supabase/server' +import { unwrap } from '@/lib/supabase/unwrap' import { getWeatherForLocation } from '@/lib/weather/tomorrow' import { sendSMS, buildSponsorLine } from '@/lib/sms/telnyx' import { renderSmsBody } from '@/lib/sms/templates' -import { claimDailySmsSlot, releaseDailySmsSlot } from '@/lib/sms/optin-claim' -import { pickNearestSponsor } from '@/lib/sms/pick-nearest-sponsor' +import { sendClaimedDailySms } from '@/lib/sms/optin-claim' +import { formatTime12h } from '@/lib/utils/time-of-day' +import { pickNearestSponsor, SPONSOR_POOL_COLUMNS, type SponsorPoolRow } from '@/lib/sms/pick-nearest-sponsor' import { unwrapJoin } from '@/lib/utils/supabase-joins' import { getFeaturedAmenityLine } from '@/lib/guidebook/featured-amenities' -import type { GuidebookSponsor } from '@/types/database' +import { asOfferType } from '@/lib/guidebook/offer' const FALLBACK_TIMEZONE = 'America/New_York' @@ -132,19 +134,75 @@ export const guidebookSmsMorningSend = inngest.createFunction( // Re-fetch instead of trusting the dispatch-time snapshot: is_active // may have flipped (guest texted STOP) since the cron ran. - const { data: optin } = await supabase + // + // Unwrapped, not destructured. `{ data: optin }` collapsed "this guest + // opted out" and "the consent read failed" into the same null, and both + // ended at `return false` — so a transient failure silently suppressed + // the message with nothing logged and no retry. The two outcomes need + // opposite handling: opted-out is final, a failed read must be retried. + const optinRes = await supabase .from('guidebook_guest_sms_optins') .select('id, phone_e164, is_active') .eq('id', optinId) .maybeSingle() + + const optin = unwrap(optinRes, { + site: 'inngest.guidebook-sms-morning-send.optin', orgId, + }) if (!optin?.is_active) return false - const { data: property } = await supabase + const propertyRes = await supabase .from('properties') - .select('id, name, lat, lng, amenities') + .select('id, name, lat, lng, amenities, checkin_time') .eq('id', propertyId) + .eq('org_id', orgId) .maybeSingle() - if (!property?.lat || !property?.lng) return false + + const property = unwrap(propertyRes, { + site: 'inngest.guidebook-sms-morning-send.property', orgId, + }) + if (!property) return false + + // ── Check-in day: this guest has not arrived yet ────────────────────── + // + // The eligibility filter is `checkin_date <= today AND checkout_date >= + // today`, so a guest whose stay STARTS today is in the set — but this + // cron fires 7-11 AM and check-in is typically mid-afternoon. They were + // getting the full nudge — "it's 72°F at your rental, here's a coffee + // spot 0.4 mi away" — hours before they had keys, as though they were + // already in the house. + // + // Deliberately BEFORE the lat/lng and weather guards below: an arrival + // reminder needs neither, and a property without coordinates should + // still be able to send one. + // + // The outgoing guest on a same-day flip is a different case and is left + // alone: `checkout_date >= today` includes them on checkout morning, + // when they ARE still in the house and a local recommendation still + // lands. The evening cron already excludes them (`checkout_date > + // today`). + // + // Same claim slot, so a guest still gets exactly one morning message — + // this one instead of the nudge, never both. + if (checkinDate === todayDate) { + return await sendClaimedDailySms( + supabase, optinId, 'last_morning_sms_date', todayDate, + async () => { + const checkinAt = formatTime12h(property.checkin_time) + const arrivalBody = await renderSmsBody(orgId, 'arrival_reminder', { + property_name: property.name, + // Omitted entirely rather than left blank when the property has + // no check-in time — 27 of 27 production properties have one, + // but the column is nullable and OwnerRez-synced properties + // explicitly write null. + checkin_line: checkinAt ? `Just a reminder that check-in is at ${checkinAt}.` : '', + }) + return await sendSMS(optin.phone_e164, arrivalBody, { category: 'nudge', orgId }) + }, + ) + } + + if (!property.lat || !property.lng) return false // Featured-amenity content is independent of sponsors — a property // with no active sponsors can still get a message if it has featured @@ -160,14 +218,29 @@ export const guidebookSmsMorningSend = inngest.createFunction( // Rain alert takes priority if precip >= 60% and rainy_day sponsor exists if (weather.precipitationProbability >= 60) { - const { data: rainySponsors } = await supabase - .from('guidebook_sponsors') - .select('id, org_id, business_name, offer_type, offer_value, offer_item, custom_offer_text, lat, lng, slot_type') - .eq('org_id', orgId) - .eq('status', 'active') - .eq('slot_type', 'rainy_day') + // A failed sponsor lookup used to produce an empty pool, which falls + // through to "no offer" and ends at `return false` — a silently + // skipped nudge indistinguishable from an org with no rainy-day + // sponsor. fetchAllRows throws instead. + // fetchAllRows, not a bare select. guidebook_sponsors is capped at SIX + // rows per org by the schema itself (slot_number CHECK 1..6 plus + // UNIQUE(org_id, slot_number)), so this drains in exactly one request — + // the pagination costs nothing at current scale and stops the read from + // resting on that cap. If the slot ceiling is ever raised, a .limit() + // would have started truncating silently; this throws instead. + const rainySponsors = await fetchAllRows( + (from, to) => supabase + .from('guidebook_sponsors') + .select(SPONSOR_POOL_COLUMNS) + .eq('org_id', orgId) + .eq('status', 'active') + .eq('slot_type', 'rainy_day') + .order('id') + .range(from, to), + { label: 'guidebook-sms-morning-send.rainy-sponsors' }, + ) - const pickedRainy = pickNearestSponsor((rainySponsors ?? []) as GuidebookSponsor[], property.lat, property.lng) + const pickedRainy = pickNearestSponsor(rainySponsors, property.lat, property.lng) if (pickedRainy) { const { sponsor: rainySponsor, distanceMiles: rainyDistanceMi } = pickedRainy @@ -175,39 +248,50 @@ export const guidebookSmsMorningSend = inngest.createFunction( // Claim the slot atomically before sending — a retry of this // step after a successful send now finds the slot already // claimed and skips re-sending, instead of double-texting. - const claimed = await claimDailySmsSlot(supabase, optinId, 'last_morning_sms_date', todayDate) - if (!claimed) return false - - const rainOfferLine = buildSponsorLine( - rainySponsor.business_name, - rainySponsor.offer_type, - rainySponsor.offer_value, - rainySponsor.offer_item, - rainySponsor.custom_offer_text, - rainyDistanceMi - ) + // Rendering is INSIDE the claimed section: it can throw too, and + // releasing only around sendSMS left the day's slot claimed for a + // template failure just the same. + return await sendClaimedDailySms( + supabase, optinId, 'last_morning_sms_date', todayDate, + async () => { + const rainOfferLine = buildSponsorLine( + rainySponsor.business_name, + asOfferType(rainySponsor.offer_type), + rainySponsor.offer_value, + rainySponsor.offer_item, + rainySponsor.custom_offer_text, + rainyDistanceMi + ) - const rainBody = await renderSmsBody(orgId, 'rain_alert', { - property_name: property.name, - offer_line: rainOfferLine, - }) - const res = await sendSMS(optin.phone_e164, rainBody, { category: 'nudge', orgId }) - if (!res.sent) { - await releaseDailySmsSlot(supabase, optinId, 'last_morning_sms_date') - } - return res.sent + const rainBody = await renderSmsBody(orgId, 'rain_alert', { + property_name: property.name, + offer_line: rainOfferLine, + }) + return await sendSMS(optin.phone_e164, rainBody, { category: 'nudge', orgId }) + }, + ) } } // Morning brew → general fallback - const { data: sponsorsData } = await supabase - .from('guidebook_sponsors') - .select('id, org_id, business_name, offer_type, offer_value, offer_item, custom_offer_text, lat, lng, slot_type') - .eq('org_id', orgId) - .eq('status', 'active') - .in('slot_type', ['morning_brew', 'general']) - - const orgSponsors = (sponsorsData ?? []) as GuidebookSponsor[] + // Same reasoning as the rainy-day pool above. + // fetchAllRows, not a bare select. guidebook_sponsors is capped at SIX + // rows per org by the schema itself (slot_number CHECK 1..6 plus + // UNIQUE(org_id, slot_number)), so this drains in exactly one request — + // the pagination costs nothing at current scale and stops the read from + // resting on that cap. If the slot ceiling is ever raised, a .limit() + // would have started truncating silently; this throws instead. + const orgSponsors = await fetchAllRows( + (from, to) => supabase + .from('guidebook_sponsors') + .select(SPONSOR_POOL_COLUMNS) + .eq('org_id', orgId) + .eq('status', 'active') + .in('slot_type', ['morning_brew', 'general']) + .order('id') + .range(from, to), + { label: 'guidebook-sms-morning-send.sponsors' }, + ) const morningBrews = orgSponsors.filter((s) => s.slot_type === 'morning_brew') const pool = morningBrews.length > 0 ? morningBrews : orgSponsors.filter((s) => s.slot_type === 'general') const picked = pickNearestSponsor(pool, property.lat, property.lng) @@ -215,7 +299,7 @@ export const guidebookSmsMorningSend = inngest.createFunction( const sponsorLine = picked ? buildSponsorLine( picked.sponsor.business_name, - picked.sponsor.offer_type, + asOfferType(picked.sponsor.offer_type), picked.sponsor.offer_value, picked.sponsor.offer_item, picked.sponsor.custom_offer_text, @@ -229,20 +313,17 @@ export const guidebookSmsMorningSend = inngest.createFunction( if (!offerLine) return false // Claim the slot atomically before sending — see rain-alert branch above. - const claimed = await claimDailySmsSlot(supabase, optinId, 'last_morning_sms_date', todayDate) - if (!claimed) return false - - const morningBody = await renderSmsBody(orgId, 'morning_nudge', { - property_name: property.name, - temperature: Math.round(weather.temperature), - offer_line: offerLine, - }) - const res = await sendSMS(optin.phone_e164, morningBody, { category: 'nudge', orgId }) - - if (!res.sent) { - await releaseDailySmsSlot(supabase, optinId, 'last_morning_sms_date') - } - return res.sent + return await sendClaimedDailySms( + supabase, optinId, 'last_morning_sms_date', todayDate, + async () => { + const morningBody = await renderSmsBody(orgId, 'morning_nudge', { + property_name: property.name, + temperature: Math.round(weather.temperature), + offer_line: offerLine, + }) + return await sendSMS(optin.phone_e164, morningBody, { category: 'nudge', orgId }) + }, + ) }) return { optinId, sent } diff --git a/lib/inngest/functions/guidebook-sponsor-activated.ts b/lib/inngest/functions/guidebook-sponsor-activated.ts index 5d522e64..d78adee2 100644 --- a/lib/inngest/functions/guidebook-sponsor-activated.ts +++ b/lib/inngest/functions/guidebook-sponsor-activated.ts @@ -3,6 +3,7 @@ import { inngest } from '@/lib/inngest/client' import { createServiceClient } from '@/lib/supabase/server' import { getActiveSponsorCount } from '@/lib/guidebook/helpers' import { logAuditEvent } from '@/lib/audit' +import { reportError } from '@/lib/observability/report-error' export const guidebookSponsorActivated = inngest.createFunction( { id: 'guidebook-sponsor-activated', name: 'Guidebook: Sponsor Activated' }, @@ -12,6 +13,35 @@ export const guidebookSponsorActivated = inngest.createFunction( await step.run('activate-sponsor-row', async () => { const supabase = createServiceClient({ system: 'inngest:guidebook-sponsor-activated' }) + + // Read before the write purely to catch the anomaly below. Replacing one + // non-null subscription id with a DIFFERENT one means this sponsor now + // has two live Stripe subscriptions and FieldStay can only ever see the + // second — the first keeps billing the business monthly with nothing + // here able to cancel it. createSponsorCheckoutSession now reuses an + // open session and compare-and-swaps the new one, so our own flow should + // not produce this; a subscription created outside it still can, and it + // must not pass silently. + const existingRes = await supabase + .from('guidebook_sponsors') + .select('stripe_subscription_id') + .eq('id', sponsorId) + .eq('org_id', orgId) + .maybeSingle() + + const existing = unwrap(existingRes, { + site: 'inngest.guidebook-sponsor-activated.existing-subscription', orgId, + }) + + const priorSubscriptionId = existing?.stripe_subscription_id ?? null + if (priorSubscriptionId && priorSubscriptionId !== subscriptionId) { + reportError(new Error('Sponsor activated with a second Stripe subscription'), { + site: 'inngest.guidebook-sponsor-activated.duplicate-subscription', + orgId, + extra: { sponsor_id: sponsorId, prior_subscription_id: priorSubscriptionId, new_subscription_id: subscriptionId }, + }) + } + const { error } = await supabase .from('guidebook_sponsors') .update({ @@ -53,7 +83,12 @@ export const guidebookSponsorActivated = inngest.createFunction( if (!inTrial && activeSponsorCount < 3) return false - await supabase + // Bound and thrown, matching the identical upsert in + // guidebook-sponsor-payment-recovered. Discarded, a failed unlock left + // the guidebook locked while this step returned `wasUnlocked: true` and + // the audit row recorded an unlock that never happened — the sponsor + // paid and the guidebook stayed dark. + const { error } = await supabase .from('guidebook_configurations') .upsert( { @@ -65,6 +100,8 @@ export const guidebookSponsorActivated = inngest.createFunction( { onConflict: 'org_id' } ) + if (error) throw new Error(`Failed to unlock guidebook: ${error.message}`) + return true }) diff --git a/lib/inngest/functions/guidebook-stay-extension-cron.ts b/lib/inngest/functions/guidebook-stay-extension-cron.ts index dccf9908..dbd13435 100644 --- a/lib/inngest/functions/guidebook-stay-extension-cron.ts +++ b/lib/inngest/functions/guidebook-stay-extension-cron.ts @@ -1,6 +1,7 @@ import { inngest } from '@/lib/inngest/client' import { fetchAllRows } from '@/lib/inngest/paginate' import { createServiceClient } from '@/lib/supabase/server' +import { unwrap } from '@/lib/supabase/unwrap' const FALLBACK_TIMEZONE = 'America/New_York' @@ -62,28 +63,60 @@ export const guidebookStayExtensionCron = inngest.createFunction( Date.now() + config.extension_message_days_before * 24 * 60 * 60 * 1000 ).toISOString().split('T')[0] - const { data: bookings } = await supabase - .from('bookings') - .select('id, org_id, property_id, checkout_date') - .eq('org_id', config.org_id) - .eq('checkout_date', targetCheckout) - .eq('status', 'confirmed') - .eq('is_block', false) + // Paginated AND error-bound, for two separate reasons. + // + // The error: discarding it made a failed read indistinguishable from + // "this org has no checkouts that day" — `bookings` came back null, + // `?? []` turned it into zero iterations, and the cron returned a + // successful `dispatched: 0`. + // + // The bound: one org's checkouts on one exact date is ~properties-per- + // org (10-50 for the target user), so this cannot realistically reach + // PostgREST's 1000-row cap. "Realistically" is doing the work in that + // sentence, and it is exactly the reasoning that left eight + // platform-wide crons silently truncated until the 2026-07-30 audit. + // fetchAllRows costs one extra round trip only once the set actually + // exceeds a page — i.e. never, on current assumptions — and removes + // the assumption instead of restating it. + const bookings = await fetchAllRows<{ + id: string; org_id: string; property_id: string; checkout_date: string + }>( + (from, to) => supabase + .from('bookings') + .select('id, org_id, property_id, checkout_date') + .eq('org_id', config.org_id) + .eq('checkout_date', targetCheckout) + .eq('status', 'confirmed') + .eq('is_block', false) + .order('id') + .range(from, to), + { label: 'guidebook-stay-extension-cron.bookings' }, + ) let sent = 0 - for (const booking of bookings ?? []) { + for (const booking of bookings) { // Check if extension request already sent (idempotency via UNIQUE(booking_id)) - const { data: existing } = await supabase + const existingRes = await supabase .from('stay_extension_requests') .select('id') .eq('booking_id', booking.id) + .eq('org_id', config.org_id) .maybeSingle() + // A failed read here returns null too, which read as "not yet + // handled" — the opposite of safe. It would fall through to the + // insert and rely on UNIQUE(booking_id) to catch the duplicate, + // whose own error was then also discarded. + const existing = unwrap(existingRes, { + site: 'inngest.guidebook-stay-extension-cron.existing-request', + orgId: config.org_id, + }) + if (existing) continue // already handled // Find the NEXT booking at this property after checkout - const { data: nextBooking } = await supabase + const nextBookingRes = await supabase .from('bookings') .select('id, checkin_date') .eq('property_id', booking.property_id) @@ -94,6 +127,14 @@ export const guidebookStayExtensionCron = inngest.createFunction( .limit(1) .maybeSingle() + // Same shape again: a failed read looked identical to "no future + // booking", which `continue`s — silently declining to offer the + // extension rather than retrying. + const nextBooking = unwrap(nextBookingRes, { + site: 'inngest.guidebook-stay-extension-cron.next-booking', + orgId: config.org_id, + }) + // Calculate gap const nextCheckin = nextBooking?.checkin_date if (!nextCheckin) continue // no future booking = open calendar, don't offer @@ -107,14 +148,23 @@ export const guidebookStayExtensionCron = inngest.createFunction( if (gapDays < config.extension_gap_threshold_days) continue // gap too small // Get guest SMS opt-in if available - const { data: optin } = await supabase + const optinRes = await supabase .from('guidebook_guest_sms_optins') .select('phone_e164, is_active') .eq('booking_id', booking.id) .maybeSingle() + // A dropped error here degrades silently rather than failing: the + // request is still created and the PM still notified, but + // guestPhoneE164 goes out null, so the guest half of the gap-night + // offer is never sent and nothing says why. + const optin = unwrap(optinRes, { + site: 'inngest.guidebook-stay-extension-cron.optin', + orgId: config.org_id, + }) + // Create the extension request record - const { data: request } = await supabase + const requestRes = await supabase .from('stay_extension_requests') .insert({ org_id: config.org_id, @@ -128,6 +178,18 @@ export const guidebookStayExtensionCron = inngest.createFunction( .select('id') .single() + // 23505 is the ONE benign outcome: another run won the race against + // UNIQUE(booking_id) between the existence check above and here, so + // that run owns the notification. Every other error — an FK + // violation, a constraint failure, an outage — used to take the same + // silent `continue`, dropping the offer and still reporting success. + if (requestRes.error?.code === '23505') continue + + const request = unwrap(requestRes, { + site: 'inngest.guidebook-stay-extension-cron.insert-request', + orgId: config.org_id, + }) + if (!request) continue // Fire event to handle notification + SMS diff --git a/lib/inngest/functions/guidebook-stay-extension-handler.ts b/lib/inngest/functions/guidebook-stay-extension-handler.ts index ec0e2150..a02cf79a 100644 --- a/lib/inngest/functions/guidebook-stay-extension-handler.ts +++ b/lib/inngest/functions/guidebook-stay-extension-handler.ts @@ -1,8 +1,64 @@ import { inngest } from '@/lib/inngest/client' import { createServiceClient } from '@/lib/supabase/server' +import { unwrap } from '@/lib/supabase/unwrap' import { sendSMS, normalizePhoneToE164 } from '@/lib/sms/telnyx' import { renderSmsBody } from '@/lib/sms/templates' import { getPmEmails, getPmMembers } from '@/lib/inngest/helpers' +import { reportError } from '@/lib/observability/report-error' + +/** Minimal shape of the service client, enough for the claim-release helper. */ +type ClaimClient = ReturnType + +/** + * Releases a one-shot send claim (`sms_sent_at` / `pm_notified_at`) so a later + * run can try again. + * + * Both call sites need this for two OPPOSITE reasons, and the second one was + * missing entirely: + * + * - `sendSMS` returns `{sent:false}` only for a DELIBERATE skip — SMS_ENABLED + * off, the daily nudge budget, demo-org suppression. Release so the send can + * happen once the skip no longer applies, then end cleanly; a retry cannot + * change an env var. + * - `sendSMS` THROWS on a real failure (dispatchToTelnyx throws on a timeout + * or any non-2xx). That throw escaped the step with the claim still held, so + * the Inngest retry hit `.is('', null)`, matched zero rows, and + * returned `already_sent` — reporting success for a message nobody ever + * received, and never trying again. + * + * The release's own result was also discarded. A failed release leaves the + * claim held forever with no signal anywhere, which is the same silent dead end + * by a slower route. + */ +async function releaseSendClaim( + supabase: ClaimClient, + requestId: string, + column: 'sms_sent_at' | 'pm_notified_at', + orgId: string, +): Promise { + // Spelled out rather than computed: a `{ [column]: null }` payload widens to + // an index signature the generated table types reject, and casting past that + // would also cast away the protection against naming a column that isn't + // there. + const patch = column === 'sms_sent_at' + ? { sms_sent_at: null } + : { pm_notified_at: null } + + const { error } = await supabase + .from('stay_extension_requests') + .update(patch) + .eq('id', requestId) + .eq('org_id', orgId) + + if (error) { + console.error(`[guidebook-stay-extension-handler] ${column} claim release failed`, error.message) + reportError(error, { + site: 'inngest.guidebook-stay-extension-handler.claim-release', + orgId, + extra: { column }, + }) + } +} export const guidebookStayExtensionHandler = inngest.createFunction( { id: 'guidebook-stay-extension-handler', name: 'Guidebook: Stay Extension Notify' }, @@ -14,7 +70,15 @@ export const guidebookStayExtensionHandler = inngest.createFunction( guestPhoneE164, contactMethod, } = event.data - // Fetch property and booking context + // Fetch property and booking context. + // + // Both errors used to be discarded. A failed read left `booking` null, so + // `portalUrl` was null, so the ENTIRE guest-SMS block below was skipped + // silently — the gap-night offer this whole function exists to send was + // never sent — while the PM email went out reading "checks out on + // undefined". The step returned successfully either way, so Inngest never + // retried it and nothing was logged. Throwing is what makes the retry + // happen. const { property, booking } = await step.run('fetch-context', async () => { const supabase = createServiceClient({ system: 'inngest:guidebook-stay-extension-handler' }) const [propRes, bookRes] = await Promise.all([ @@ -22,14 +86,19 @@ export const guidebookStayExtensionHandler = inngest.createFunction( .from('properties') .select('name') .eq('id', propertyId) + .eq('org_id', orgId) .single(), supabase .from('bookings') .select('guidebook_token, checkout_date') .eq('id', bookingId) + .eq('org_id', orgId) .single(), ]) - return { property: propRes.data, booking: bookRes.data } + return { + property: unwrap(propRes, { site: 'inngest.guidebook-stay-extension-handler.property', orgId }), + booking: unwrap(bookRes, { site: 'inngest.guidebook-stay-extension-handler.booking', orgId }), + } }) const appUrl = process.env.NEXT_PUBLIC_APP_URL ?? 'https://app.fieldstay.app' @@ -47,27 +116,44 @@ export const guidebookStayExtensionHandler = inngest.createFunction( // can execute an arbitrary amount of time later, wide enough for a // guest to have texted STOP in between. Every other guest SMS path // re-checks immediately before sending; this one didn't. - const { data: optin } = await supabase + // + // Unwrapped, not destructured: a failed consent read must NOT be read + // as "no consent". Both readings skip the send, but only one of them + // should do so silently and permanently — the other needs a retry. + const optinRes = await supabase .from('guidebook_guest_sms_optins') .select('is_active') .eq('booking_id', bookingId) .maybeSingle() - if (!optin?.is_active) return + const optin = unwrap(optinRes, { + site: 'inngest.guidebook-stay-extension-handler.optin-recheck', + orgId, + }) + + if (!optin?.is_active) return { skipped: 'not_opted_in' } // ── Atomic claim — wins the race, prevents double-send on retry ─────── // UPDATE only succeeds if sms_sent_at IS NULL. If this step is retried // after a successful SMS send, the timestamp is already set, the // UPDATE affects 0 rows, and we skip the send. Mirrors the pattern in - // guidebook-guest-opted-in.ts. - const { data: claimed } = await supabase + // guidebook-guest-opted-in.ts — including the part that file learned + // and this one hadn't: a FAILED claim also returns null, which read as + // "already sent" and skipped forever. Throwing lets Inngest retry. + const claimRes = await supabase .from('stay_extension_requests') .update({ sms_sent_at: new Date().toISOString() }) .eq('id', requestId) + .eq('org_id', orgId) .is('sms_sent_at', null) .select('id') .maybeSingle() + const claimed = unwrap(claimRes, { + site: 'inngest.guidebook-stay-extension-handler.guest-sms-claim', + orgId, + }) + if (!claimed) return { skipped: 'already_sent' } const discountLine = discountPct @@ -83,15 +169,21 @@ export const guidebookStayExtensionHandler = inngest.createFunction( // 'nudge': guest marketing message — counts against the platform-wide // daily SMS budget (the PM notification below is operational and doesn't) - const result = await sendSMS(guestPhoneE164, text, { category: 'nudge', orgId }) + // See releaseSendClaim() for why both exits below release the claim. + let result + try { + result = await sendSMS(guestPhoneE164, text, { category: 'nudge', orgId }) + } catch (err) { + await releaseSendClaim(supabase, requestId, 'sms_sent_at', orgId) + throw err + } if (!result.sent) { - // SMS failed — roll back the claim so a retry can attempt again - await supabase - .from('stay_extension_requests') - .update({ sms_sent_at: null }) - .eq('id', requestId) + await releaseSendClaim(supabase, requestId, 'sms_sent_at', orgId) + return { skipped: result.reason ?? 'not_sent' } } + + return { sent: true } }) } @@ -110,11 +202,19 @@ export const guidebookStayExtensionHandler = inngest.createFunction( const [pmMember] = await getPmMembers(supabase, orgId, { limit: 1 }) if (!pmMember) return { skipped: true } - const { data: profile } = await supabase + // maybeSingle, not single: a PM with no profile row is a legitimate + // skip, and .single() turns that into a PGRST116 error that unwrap + // would now escalate into a retried failure. + const profileRes = await supabase .from('profiles') .select('phone') .eq('id', pmMember.userId) - .single() + .maybeSingle() + + const profile = unwrap(profileRes, { + site: 'inngest.guidebook-stay-extension-handler.pm-profile', + orgId, + }) if (!profile?.phone) return { skipped: true, reason: 'no_pm_phone' } @@ -126,14 +226,23 @@ export const guidebookStayExtensionHandler = inngest.createFunction( // caller that wins the UPDATE sends. A retry after a successful // send finds pm_notified_at already set and skips, instead of // texting the PM's own phone twice. - const { data: claimed } = await supabase + // Unwrapped for the same reason as the guest claim above: a failed + // claim also returns null, and reading that as "already notified" + // drops the notification permanently. + const claimRes = await supabase .from('stay_extension_requests') .update({ pm_notified_at: new Date().toISOString() }) .eq('id', requestId) + .eq('org_id', orgId) .is('pm_notified_at', null) .select('id') .maybeSingle() + const claimed = unwrap(claimRes, { + site: 'inngest.guidebook-stay-extension-handler.pm-sms-claim', + orgId, + }) + if (!claimed) return { skipped: true, reason: 'already_notified' } const text = @@ -141,17 +250,21 @@ export const guidebookStayExtensionHandler = inngest.createFunction( `${booking?.checkout_date}, ${gapDays} day${gapDays !== 1 ? 's' : ''} ` + `before next booking.${discountLine} Guest was messaged via the guidebook.` - const result = await sendSMS(e164, text, { orgId }) + // See releaseSendClaim() — same two exits as the guest send above. + let result + try { + result = await sendSMS(e164, text, { orgId }) + } catch (err) { + await releaseSendClaim(supabase, requestId, 'pm_notified_at', orgId) + throw err + } if (!result.sent) { - // SMS failed — roll back the claim so a retry can attempt again - await supabase - .from('stay_extension_requests') - .update({ pm_notified_at: null }) - .eq('id', requestId) + await releaseSendClaim(supabase, requestId, 'pm_notified_at', orgId) + return { notified: false, skipped: result.reason ?? 'not_sent' } } - return { notified: result.sent } + return { notified: true } }) } else { // contactMethod === 'email' (also the fallback if null/unset) @@ -181,10 +294,23 @@ export const guidebookStayExtensionHandler = inngest.createFunction( ) if (error) throw new Error(`Resend error: ${JSON.stringify(error)}`) - await supabase + // No claim needed on this path — Resend's idempotencyKey is what + // prevents the double-send, so this write is only a record of it. + // Its result was discarded entirely, which meant a failed write left + // the request looking un-notified forever with nothing logged. + const { error: stampError } = await supabase .from('stay_extension_requests') .update({ pm_notified_at: new Date().toISOString() }) .eq('id', requestId) + .eq('org_id', orgId) + + if (stampError) { + console.error('[guidebook-stay-extension-handler] pm_notified_at stamp failed', stampError.message) + reportError(stampError, { + site: 'inngest.guidebook-stay-extension-handler.pm-email-stamp', + orgId, + }) + } return { notified: true } }) diff --git a/lib/sms/optin-claim.ts b/lib/sms/optin-claim.ts index 17ec7852..6f45a302 100644 --- a/lib/sms/optin-claim.ts +++ b/lib/sms/optin-claim.ts @@ -48,3 +48,46 @@ export async function releaseDailySmsSlot( reportError(error, { site: 'lib.sms.optin-claim.release', extra: { date_column: dateColumn } }) } } + +/** + * Claim → send → release-on-EITHER-failure, as one unit. + * + * The claim and the release already lived here; the sequence that ties them + * together did not, and all three guest-nudge call sites (two morning + * branches, one evening) had independently written the same HALF of it: + * + * const res = await sendSMS(...) + * if (!res.sent) await releaseDailySmsSlot(...) + * return res.sent + * + * `sendSMS` returns `{sent:false}` ONLY for a deliberate skip — SMS_ENABLED + * off, the daily nudge budget, demo-org suppression. Every REAL failure + * throws, because dispatchToTelnyx throws on a timeout or any non-2xx. So the + * one branch that was written handled the case that isn't a failure, and the + * throw walked out past the release with the day's slot still claimed. The + * Inngest retry then re-read the date column, found today's date, skipped — + * and that guest's nudge was silently gone for the day. + * + * Callers get a plain boolean back, so the shape at each site is unchanged. + */ +export async function sendClaimedDailySms( + supabase: SupabaseClient, + optinId: string, + dateColumn: DailySmsDateColumn, + todayDate: string, + send: () => Promise<{ sent: boolean }>, +): Promise { + const claimed = await claimDailySmsSlot(supabase, optinId, dateColumn, todayDate) + if (!claimed) return false + + let res: { sent: boolean } + try { + res = await send() + } catch (err) { + await releaseDailySmsSlot(supabase, optinId, dateColumn) + throw err + } + + if (!res.sent) await releaseDailySmsSlot(supabase, optinId, dateColumn) + return res.sent +} diff --git a/lib/sms/pick-nearest-sponsor.ts b/lib/sms/pick-nearest-sponsor.ts index 46ffe87a..efef6f52 100644 --- a/lib/sms/pick-nearest-sponsor.ts +++ b/lib/sms/pick-nearest-sponsor.ts @@ -1,5 +1,29 @@ import { distanceMiles } from '@/lib/geocoding' -import type { GuidebookSponsor } from '@/types/database' + +/** + * The columns the SMS nudge crons actually read off a sponsor, and the SELECT + * string that fetches exactly them. + * + * Shared so the morning and evening pools cannot drift apart, and so the reads + * can be typed honestly: they select a SUBSET of guidebook_sponsors, and + * asserting the result is a full GuidebookSponsor was a cast that happened to + * compile rather than a fact. + */ +export const SPONSOR_POOL_COLUMNS = + 'id, org_id, business_name, offer_type, offer_value, offer_item, custom_offer_text, lat, lng, slot_type' + +export interface SponsorPoolRow { + id: string + org_id: string + business_name: string + offer_type: string + offer_value: number | null + offer_item: string | null + custom_offer_text: string | null + lat: number | null + lng: number | null + slot_type: string +} /** * Picks the sponsor nearest to the given property coordinates. Sponsors @@ -7,18 +31,22 @@ import type { GuidebookSponsor } from '@/types/database' * one is used as the fallback). Previously duplicated verbatim in both the * morning and evening SMS nudge crons. */ -export function pickNearestSponsor( - sponsors: GuidebookSponsor[], +// Generic over anything carrying coordinates: the only fields this function +// reads are lat/lng, so constraining callers to a full GuidebookSponsor forced +// every narrow SELECT to cast its way in. The caller keeps its own row type and +// gets it back on `.sponsor`. +export function pickNearestSponsor( + sponsors: T[], lat: number, lng: number -): { sponsor: GuidebookSponsor; distanceMiles: number | null } | null { +): { sponsor: T; distanceMiles: number | null } | null { const withCoords = sponsors.filter((s) => s.lat !== null && s.lng !== null) if (withCoords.length === 0) { const fallback = sponsors[0] return fallback ? { sponsor: fallback, distanceMiles: null } : null } - let nearest: GuidebookSponsor | null = null + let nearest: T | null = null let nearestDist = Infinity for (const s of withCoords) { const dist = distanceMiles(lat, lng, s.lat!, s.lng!) diff --git a/lib/sms/template-registry.ts b/lib/sms/template-registry.ts index b879153a..84d4722f 100644 --- a/lib/sms/template-registry.ts +++ b/lib/sms/template-registry.ts @@ -13,6 +13,7 @@ export type SmsTemplateKey = | 'door_code' | 'morning_nudge' + | 'arrival_reminder' | 'evening_nudge' | 'rain_alert' | 'stay_extension' @@ -88,6 +89,17 @@ export const SMS_TEMPLATE_REGISTRY: SmsTemplateConfig[] = [ ], defaultBody: 'Good morning! It\'s {{temperature}}°F at {{property_name}} today. {{offer_line}} Reply STOP to opt out.', }, + { + key: 'arrival_reminder', + label: 'Arrival Reminder — Check-In Day', + description: 'Replaces the morning nudge on a guest\'s CHECK-IN day. The morning cron runs 7-11 AM but check-in is typically mid-afternoon, so a guest arriving today would otherwise get "it\'s 72°F at your rental, here\'s a coffee spot" hours before they have keys.', + audience: 'guest', + variables: [ + { token: '{{property_name}}', description: 'Property name', example: 'Lakeside Lodge' }, + { token: '{{checkin_line}}', description: 'Check-in time sentence — empty when the property has no check-in time set, so the message still reads correctly', example: 'Just a reminder that check-in is at 4:00 PM.' }, + ], + defaultBody: 'Looking forward to hosting you at {{property_name}} today! {{checkin_line}} Reply STOP to opt out.', + }, { key: 'evening_nudge', label: 'Evening Nudge — Guest Stay', diff --git a/lib/sms/templates.ts b/lib/sms/templates.ts index 344bc491..31ec9f50 100644 --- a/lib/sms/templates.ts +++ b/lib/sms/templates.ts @@ -103,6 +103,16 @@ function renderDefault( vars.offer_line ? String(vars.offer_line) : null ) + case 'arrival_reminder': + // Inline rather than a telnyx.ts builder: there is no legacy builder to + // preserve, and checkin_line is already assembled (or omitted) by the + // caller so a property with no check-in time still reads correctly. + return [ + `Looking forward to hosting you at ${vars.property_name ?? 'your rental'} today!`, + vars.checkin_line ? String(vars.checkin_line) : '', + 'Reply STOP to opt out.', + ].filter(Boolean).join(' ') + case 'evening_nudge': return buildEveningNudgeSMS( String(vars.property_name ?? ''), diff --git a/lib/storage/object-path.ts b/lib/storage/object-path.ts index 171ccf2c..d43795f3 100644 --- a/lib/storage/object-path.ts +++ b/lib/storage/object-path.ts @@ -20,12 +20,12 @@ // components, Server Actions, Route Handlers, and Inngest steps alike. No // `server-only`, no Supabase import. +import { UUID_RE } from '@/lib/validation/uuid' + export const ORG_SCOPED_PHOTO_BUCKETS = ['work-order-photos', 'turnover-photos'] as const export type OrgScopedPhotoBucket = (typeof ORG_SCOPED_PHOTO_BUCKETS)[number] -const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i - /** * Builds an object key for an org-scoped photo bucket. * diff --git a/lib/utils/time-of-day.ts b/lib/utils/time-of-day.ts new file mode 100644 index 00000000..6e965027 --- /dev/null +++ b/lib/utils/time-of-day.ts @@ -0,0 +1,28 @@ +// lib/utils/time-of-day.ts +// +// Formatting for a bare `time without time zone` column (properties.checkin_time +// / checkout_time), which arrives from PostgREST as "16:00:00". +// +// Deliberately NOT `import 'server-only'`: a pure string transform with no +// dependencies, used by the guest guidebook (a Client Component) and by the +// morning SMS send (an Inngest step). Same rationale as lib/validation/uuid.ts. +// +// Note these values carry NO timezone — they are wall-clock times at the +// property, which is exactly what a guest wants read back to them ("check-in +// is at 4:00 PM"). Do not run them through Intl with a timeZone; that would +// reinterpret a wall-clock string as an instant and shift it. + +/** + * "16:00:00" → "4:00 PM". Returns null for null/empty/unparseable input so + * callers can omit the phrase entirely rather than render "check-in is at ." + */ +export function formatTime12h(time: string | null | undefined): string | null { + if (!time) return null + const [hourStr, minuteStr] = time.split(':') + const hour = Number(hourStr) + const minute = Number(minuteStr) + if (Number.isNaN(hour) || Number.isNaN(minute)) return null + const period = hour >= 12 ? 'PM' : 'AM' + const displayHour = hour % 12 === 0 ? 12 : hour % 12 + return `${displayHour}:${minute.toString().padStart(2, '0')} ${period}` +} diff --git a/lib/validation/uuid.ts b/lib/validation/uuid.ts new file mode 100644 index 00000000..50905c82 --- /dev/null +++ b/lib/validation/uuid.ts @@ -0,0 +1,41 @@ +// lib/validation/uuid.ts +// +// The UUID shape check, in one place. +// +// Every id this app puts into a `.eq()` is a Postgres `uuid` column, and +// Postgres does not coerce — a non-UUID string is error 22P02 ("invalid input +// syntax for type uuid"), not an empty result. What that error becomes depends +// entirely on where it lands: +// +// - through unwrap(), it throws — which in a public route means a Sentry +// report per malformed request, so anyone can burn the quota and bury the +// real DB failures on that route under noise; +// - destructured as `{ data }`, it silently reads as "no such row". +// +// Neither is what the caller meant. A malformed id is a BAD REQUEST and +// belongs to input validation at the boundary — the item CLAUDE.md's manual +// audit checklist lists as having no mechanical guardrail. +// +// This regex was independently open-coded in four files before this module +// existed (two crew routes, the crew work-order complete route, and +// lib/storage/object-path.ts) and was simply missing from the fifth place that +// needed it. Five copies is how the sixth one gets forgotten too. +// +// Deliberately NOT `import 'server-only'`: a pure predicate over a string, +// usable from a Client Component that wants to reject input before a round +// trip. Same rationale as lib/storage/object-path.ts and lib/supabase/unwrap.ts. + +/** + * Matches the canonical 8-4-4-4-12 hex form, case-insensitively. + * + * Intentionally not version- or variant-aware: the job is to keep malformed + * input out of a `uuid` column, and Postgres accepts any 8-4-4-4-12 hex string + * regardless of the version nibble. A stricter pattern would reject ids + * Postgres itself considers valid. + */ +export const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + +/** True when `value` is a string Postgres will accept for a `uuid` column. */ +export function isUuid(value: unknown): value is string { + return typeof value === 'string' && UUID_RE.test(value) +} diff --git a/supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql b/supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql new file mode 100644 index 00000000..341f6fd9 --- /dev/null +++ b/supabase/migrations/20260807150000_guidebook_offer_redemptions_daily_dedup.sql @@ -0,0 +1,44 @@ +-- guidebook_offer_redemptions: one redemption per sponsor, per booking, per day. +-- +-- The row is written when a guest opens the redemption pass — the full-screen +-- "Guest Perk · Verified Live" card whose own hint reads "Show this screen to +-- staff — clock proves it's live". It is a coupon presented at the counter, not +-- a page-view, and it is the number a paying sponsor will judge their slot by. +-- +-- Opening that pass more than once is the normal case, not an edge case: a +-- guest looks at the offer from the couch, closes it, walks to the business, +-- and opens it again to show staff. Every reopen was a separate row, so raw +-- COUNT(*) overstated real redemptions by however many times the guest looked +-- at their own coupon. +-- +-- Nothing reads this table yet. That is exactly why the constraint goes in now: +-- whoever builds the sponsor report later will reach for COUNT(*), and the +-- honest number has to be the one the table can produce. +-- +-- Day, not stay: a "free coffee" perk is legitimately redeemable on each day of +-- a booking, so collapsing a whole stay to one row would UNDER-count. Within a +-- day, one redemption per sponsor. +-- +-- The day boundary is UTC because the row carries no timezone of its own. For a +-- US property this splits a single evening's repeat opens across two dates only +-- when they straddle UTC midnight (6-8pm local). That errs toward counting one +-- extra, never toward missing a genuinely distinct day — the safe direction for +-- a constraint whose failure mode would otherwise be silently discarding a real +-- redemption. +-- +-- Anonymous redemptions (booking_id IS NULL, from the property-level /g/[slug] +-- guidebook, which has no booking token) are deliberately NOT covered: with no +-- guest identity there is nothing to dedupe on, and collapsing them by +-- (sponsor, day) would merge DIFFERENT guests into one. They stay uncapped and +-- unattributed, which is what they are. + +CREATE UNIQUE INDEX IF NOT EXISTS uniq_guidebook_offer_redemptions_sponsor_booking_day + ON guidebook_offer_redemptions ( + sponsor_id, + booking_id, + ((opened_at AT TIME ZONE 'UTC')::date) + ) + WHERE booking_id IS NOT NULL; + +COMMENT ON INDEX uniq_guidebook_offer_redemptions_sponsor_booking_day IS + 'One redemption per sponsor per booking per UTC day. Repeat opens of the same pass are the same redemption; app/api/guidebook/redeem relies on this for ignoreDuplicates.'; diff --git a/supabase/migrations/20260807170000_guidebook_offer_open_count.sql b/supabase/migrations/20260807170000_guidebook_offer_open_count.sql new file mode 100644 index 00000000..befc5b4c --- /dev/null +++ b/supabase/migrations/20260807170000_guidebook_offer_open_count.sql @@ -0,0 +1,63 @@ +-- guidebook_offer_redemptions.open_count — engagement, alongside the deduped +-- redemption it lives on. +-- +-- 20260807150000 made the row unique per (sponsor, booking, UTC day) so a paying +-- sponsor's redemption count stops being inflated by a guest reopening their own +-- coupon. Correct for that number, but it threw away a real second signal: how +-- many times the pass was actually opened. "12 redemptions, opened 31 times" +-- says something "12 redemptions" alone does not. +-- +-- One column on the existing row rather than a second table: the dedup key IS +-- the natural grain for the counter, so the count stays bounded by the same +-- constraint instead of growing per tap, and both numbers come out of one +-- query — COUNT(*) for redemptions, SUM(open_count) for opens. +-- +-- Anonymous redemptions (booking_id NULL, property-level /g/[slug]) sit outside +-- the partial unique index, so each open is its own row at open_count = 1. Both +-- aggregates still read correctly there; they simply carry no more information +-- than each other, which is the honest answer when there is no guest identity +-- to attribute opens to. + +ALTER TABLE guidebook_offer_redemptions + ADD COLUMN IF NOT EXISTS open_count integer NOT NULL DEFAULT 1; + +COMMENT ON COLUMN guidebook_offer_redemptions.open_count IS + 'Times the redemption pass was opened for this (sponsor, booking, UTC day). COUNT(*) = redemptions; SUM(open_count) = opens.'; + +-- The insert-or-increment, as one statement. +-- +-- It has to be a function: the arbiter is a PARTIAL EXPRESSION index, and +-- PostgREST's on_conflict parameter only accepts plain column names, so the +-- JS client cannot express this upsert at all. Doing it as read-then-write in +-- the route would be a TOCTOU — two taps racing would both read 1 and both +-- write 2. +-- +-- SECURITY INVOKER (the default), deliberately. The only caller is the redeem +-- route holding the service role, which already bypasses RLS; a DEFINER +-- function here would add an privilege-escalation surface that buys nothing. +CREATE OR REPLACE FUNCTION public.record_guidebook_offer_open( + p_org_id uuid, + p_sponsor_id uuid, + -- DEFAULT NULL so the anonymous case (property-level /g/[slug], no booking + -- token) can omit the argument entirely. Without a default, Supabase's type + -- generator emits `p_booking_id: string` — required and non-nullable — and + -- the only ways to call it with a null are a cast or a second write path. + p_booking_id uuid DEFAULT NULL +) RETURNS void +LANGUAGE sql +SET search_path = public +AS $$ + INSERT INTO guidebook_offer_redemptions (org_id, sponsor_id, booking_id) + VALUES (p_org_id, p_sponsor_id, p_booking_id) + ON CONFLICT (sponsor_id, booking_id, ((opened_at AT TIME ZONE 'UTC')::date)) + WHERE booking_id IS NOT NULL + DO UPDATE SET open_count = guidebook_offer_redemptions.open_count + 1; +$$; + +-- Postgres grants EXECUTE on a new function to PUBLIC by default, which on a +-- Supabase project means anon can call it over /rest/v1/rpc/ with the +-- publishable key — i.e. write rows to a tenant table with no session at all. +-- Every anon TABLE grant was revoked on 2026-07-24; a function is the same +-- exposure through a different door. +REVOKE ALL ON FUNCTION public.record_guidebook_offer_open(uuid, uuid, uuid) FROM PUBLIC, anon, authenticated; +GRANT EXECUTE ON FUNCTION public.record_guidebook_offer_open(uuid, uuid, uuid) TO service_role; diff --git a/types/database.generated.ts b/types/database.generated.ts index 2bb412d9..8ad88d50 100644 --- a/types/database.generated.ts +++ b/types/database.generated.ts @@ -1272,6 +1272,7 @@ export type Database = { Row: { booking_id: string | null id: string + open_count: number opened_at: string org_id: string sponsor_id: string @@ -1279,6 +1280,7 @@ export type Database = { Insert: { booking_id?: string | null id?: string + open_count?: number opened_at?: string org_id: string sponsor_id: string @@ -1286,6 +1288,7 @@ export type Database = { Update: { booking_id?: string | null id?: string + open_count?: number opened_at?: string org_id?: string sponsor_id?: string @@ -5820,6 +5823,10 @@ export type Database = { Returns: string } recompute_vendor_scores: { Args: never; Returns: number } + record_guidebook_offer_open: { + Args: { p_booking_id?: string; p_org_id: string; p_sponsor_id: string } + Returns: undefined + } remove_crew_from_turnover: { Args: { p_crew_member_id: string diff --git a/types/database.ts b/types/database.ts index faad49d9..1a60cfaa 100644 --- a/types/database.ts +++ b/types/database.ts @@ -1480,6 +1480,13 @@ export interface GuidebookOfferRedemption { sponsor_id: string booking_id: string | null opened_at: string + /** + * Times the pass was opened for this (sponsor, booking, UTC day). The row + * itself is deduped by uniq_guidebook_offer_redemptions_sponsor_booking_day, + * so COUNT(*) is redemptions and SUM(open_count) is engagement — two numbers + * a sponsor wants separately. NOT NULL DEFAULT 1 (20260807170000). + */ + open_count: number } export interface GuidebookGuestSmsOptin { diff --git a/unit/guardrails/n-plus-one-loops.test.ts b/unit/guardrails/n-plus-one-loops.test.ts index ab87da40..08edeb56 100644 --- a/unit/guardrails/n-plus-one-loops.test.ts +++ b/unit/guardrails/n-plus-one-loops.test.ts @@ -127,7 +127,7 @@ const EXCEPTIONS: Record = { 'Real N+1 (existence-check select + insert per property) left as a known, bounded cost — deferred rather than fixed blind in the same PR that added this guardrail, since it touches live PMS-sync logic. Bounded by properties-per-org (10-50 per CLAUDE.md\'s target user).', 'lib/asset-discovery/seed-from-amenities.ts:171': 'Second pass (absent-asset-types) of the same function — same reasoning as line 63.', - 'lib/inngest/functions/guidebook-stay-extension-cron.ts:75': + 'lib/inngest/functions/guidebook-stay-extension-cron.ts:98': 'Real N+1 (existence check, next-booking lookup, opt-in lookup, insert — 4 queries per booking) left as a known, bounded cost — deferred rather than fixed blind, touches live guest-messaging sync logic. Bounded by same-day checkouts per org per day.', 'lib/inngest/functions/ownerrez/reconciliation-handler.ts:124': 'Real N+1 (cancel booking + cancel its turnovers, per stale booking) left as a known, bounded cost — deferred rather than fixed blind. Contrast lib/inngest/functions/ical-sync.ts, which batches the equivalent booking-cancel via .update().in(\'id\', ids) — a good template for fixing this one later.', diff --git a/unit/guardrails/redemption-dedup-pairing.test.ts b/unit/guardrails/redemption-dedup-pairing.test.ts new file mode 100644 index 00000000..3e630197 --- /dev/null +++ b/unit/guardrails/redemption-dedup-pairing.test.ts @@ -0,0 +1,104 @@ +// unit/guardrails/redemption-dedup-pairing.test.ts +// +// guidebook_offer_redemptions carries TWO sponsor-facing numbers out of one +// row — COUNT(*) is redemptions, SUM(open_count) is engagement — and both +// depend on a unique index, an insert-or-increment function, and a route that +// calls it, spread across three files with nothing tying them together. +// +// The row is written when a guest opens the redemption pass, the coupon they +// show at the counter. Reopening it is the normal case (look at the offer, +// close it, walk over, open it again for staff), which is precisely why the +// two numbers differ and why a sponsor wants both. +// +// Every way this breaks is silent, and none of them shows up as a test failure +// anywhere else — no unit test touches the real database, and the sponsor +// report that would surface a wrong number does not exist yet: +// +// - the unique index goes away (migration reverted, renamed, table rebuilt): +// redemptions quietly re-inflate to one row per tap. +// - the function's ON CONFLICT arbiter stops matching the index expression: +// same inflation, one layer down, where the index still looks present. +// - the route swaps the RPC for a plain .insert(): rows stay deduped, but +// open_count sticks at 1 and engagement silently flatlines. +// - the function keeps Postgres's default EXECUTE-to-PUBLIC grant: anon can +// write rows to a tenant table over /rest/v1/rpc/ with no session at all. +// +// This asserts all four stay closed. + +import { describe, it, expect } from 'vitest' +import { readFileSync, readdirSync } from 'node:fs' +import { join } from 'node:path' + +const ROOT = join(__dirname, '..', '..') +const ROUTE = 'app/api/guidebook/redeem/route.ts' +const MIGRATIONS_DIR = join(ROOT, 'supabase', 'migrations') +const INDEX_NAME = 'uniq_guidebook_offer_redemptions_sponsor_booking_day' +const RPC_NAME = 'record_guidebook_offer_open' + +function migrationSources(): string { + return readdirSync(MIGRATIONS_DIR) + .filter((f) => f.endsWith('.sql')) + .map((f) => readFileSync(join(MIGRATIONS_DIR, f), 'utf8')) + .join('\n') +} + +describe('guidebook redemption dedup — constraint and handler stay paired', () => { + it('a migration creates the unique index the handler relies on', () => { + const sql = migrationSources() + const createsIt = new RegExp( + `CREATE\\s+UNIQUE\\s+INDEX(\\s+IF\\s+NOT\\s+EXISTS)?\\s+${INDEX_NAME}\\b`, + 'i', + ) + expect(sql).toMatch(createsIt) + }) + + it('the index is partial on booking_id, so anonymous redemptions are not collapsed together', () => { + const sql = migrationSources() + const stmt = sql.slice(sql.indexOf(`CREATE UNIQUE INDEX IF NOT EXISTS ${INDEX_NAME}`)) + // Without the WHERE clause, every anonymous redemption (booking_id NULL, + // from the property-level /g/[slug] guidebook) would still be distinct in + // Postgres — NULLs never collide — but a later change to NULLS NOT + // DISTINCT would merge DIFFERENT guests into one row. The predicate makes + // the intent explicit rather than resting on NULL semantics. + expect(stmt.slice(0, 400)).toMatch(/WHERE\s+booking_id\s+IS\s+NOT\s+NULL/i) + }) + + it('a migration defines the insert-or-increment function against that same index', () => { + const sql = migrationSources() + expect(sql).toContain(`FUNCTION public.${RPC_NAME}`) + // The ON CONFLICT arbiter must match the index expression, or the function + // silently loses its conflict target and every open inserts a new row — + // the exact inflation the index exists to prevent, reintroduced one layer + // down where the index still looks present. + const fn = sql.slice(sql.indexOf(`FUNCTION public.${RPC_NAME}`)) + expect(fn).toMatch(/ON CONFLICT[\s\S]{0,200}opened_at AT TIME ZONE 'UTC'/i) + expect(fn).toMatch(/DO UPDATE SET open_count = [\s\S]{0,80}open_count \+ 1/i) + }) + + it('the function is not executable by anon or authenticated', () => { + const sql = migrationSources() + const fn = sql.slice(sql.indexOf(`FUNCTION public.${RPC_NAME}`)) + // Postgres grants EXECUTE to PUBLIC by default, which on Supabase means + // anon can call it over /rest/v1/rpc/ with the publishable key — writing + // rows to a tenant table with no session at all. Every anon TABLE grant + // was revoked on 2026-07-24; a function is the same exposure via a + // different door. + expect(fn).toMatch(/REVOKE ALL ON FUNCTION[\s\S]{0,200}FROM PUBLIC/i) + expect(fn).toMatch(/GRANT EXECUTE ON FUNCTION[\s\S]{0,200}TO service_role/i) + expect(fn).not.toMatch(/GRANT EXECUTE[\s\S]{0,200}TO (anon|authenticated)/i) + }) + + it('the route writes through the function, not a bare insert it could drift from', () => { + const src = readFileSync(join(ROOT, ROUTE), 'utf8') + expect(src).toContain(RPC_NAME) + // A plain .insert() here would bypass the increment entirely: the row + // would still be deduped by the index, but open_count would stay 1 forever + // and the engagement number would silently flatline. + expect(src).not.toMatch(/\.from\(\s*['"]guidebook_offer_redemptions['"]\s*\)\s*\.insert/) + }) + + it('the route names the index it depends on, so a grep for the index finds its consumer', () => { + const src = readFileSync(join(ROOT, ROUTE), 'utf8') + expect(src).toContain(INDEX_NAME) + }) +}) diff --git a/unit/guardrails/supabase-error-handling.test.ts b/unit/guardrails/supabase-error-handling.test.ts index 57d4052d..e69a629d 100644 --- a/unit/guardrails/supabase-error-handling.test.ts +++ b/unit/guardrails/supabase-error-handling.test.ts @@ -106,7 +106,13 @@ const BASELINE: Record = { 'app/(dashboard)/templates/inventory/actions.ts': 2, 'app/(dashboard)/templates/maintenance/actions.ts': 3, 'app/(dashboard)/vendors/actions.ts': 3, - 'app/actions/guidebook.ts': 3, + // 3 -> 2: optInGuestSms's booking lookup now binds and reports its error. + // Discarding it made a transient failure indistinguishable from a bad token, + // so a guest with a valid link was told the link was invalid. + // 2 -> 1: createSponsorCheckoutSession's media-kit-token lookup had the SAME + // defect one function over — a sponsor holding a valid link was told it was + // invalid whenever the query itself failed. + 'app/actions/guidebook.ts': 1, 'app/api/repuguard/generate/route.ts': 3, 'app/api/vendor-connect/[token]/onboard/route.ts': 2, @@ -123,10 +129,28 @@ const BASELINE: Record = { 'lib/inngest/functions/cron/work-order-ops.ts': 2, 'lib/inngest/functions/email-trial-lifecycle.tsx': 4, 'lib/inngest/functions/flagged-turnover-wo.ts': 3, - 'lib/inngest/functions/guidebook-sms-evening-cron.ts': 3, - 'lib/inngest/functions/guidebook-sms-morning-cron.ts': 4, - 'lib/inngest/functions/guidebook-stay-extension-cron.ts': 5, - 'lib/inngest/functions/guidebook-stay-extension-handler.ts': 4, + // 3 -> 0 (entry deleted), same three reads as its morning twin below: a + // failed sponsor lookup was indistinguishable from an org with no sponsors, + // and a failed opt-in read from a guest who opted out — every one of them + // ending at "no SMS" with nothing logged. + // 4 -> 0 (entry deleted): every read in the per-guest send now unwraps. The + // opt-in one mattered most — `{ data: optin }` collapsed "this guest opted + // out" and "the consent read failed" into the same null, and both ended at + // `return false`, so a transient failure silently suppressed the message + // with nothing logged and no retry. The two sponsor reads had the same + // shape: a failed lookup produced an empty pool, indistinguishable from an + // org that simply has no sponsor in that slot. + // + // guidebook-stay-extension-cron.ts 5 -> 0 and + // guidebook-stay-extension-handler.ts 4 -> 0 (both entries deleted): the + // gap-night offer's whole failure + // surface was silent. In the cron a failed bookings read looked like "this + // org has no checkouts", a failed existence check looked like "not yet + // handled", and a failed next-booking read looked like "open calendar" — + // each ending in a successful `dispatched: 0`. In the handler a failed + // context read left `booking` null, so `portalUrl` was null, so the guest + // SMS block was skipped entirely while the PM email went out reading + // "checks out on undefined". 'lib/inngest/functions/hospitable/hospitable-reviews-backfill.ts': 2, 'lib/inngest/functions/hospitable/incremental-sync.ts': 5, 'lib/inngest/functions/hospitable/initial-sync.ts': 2, diff --git a/unit/guardrails/unbounded-select.test.ts b/unit/guardrails/unbounded-select.test.ts index 0f7d6575..cec7a1a7 100644 --- a/unit/guardrails/unbounded-select.test.ts +++ b/unit/guardrails/unbounded-select.test.ts @@ -128,9 +128,6 @@ const BASELINE = new Set([ 'lib/inngest/functions/crew-assignment.ts', 'lib/inngest/functions/cron/daily-wrapup.ts', 'lib/inngest/functions/flagged-turnover-wo.ts', - 'lib/inngest/functions/guidebook-sms-evening-cron.ts', - 'lib/inngest/functions/guidebook-sms-morning-cron.ts', - 'lib/inngest/functions/guidebook-stay-extension-cron.ts', 'lib/inngest/functions/hospitable/calendar-sync-handler.ts', 'lib/inngest/functions/hospitable/hospitable-reviews-backfill.ts', 'lib/inngest/functions/hospitable/initial-sync.ts', diff --git a/unit/guidebook/guest-stay-timezone.test.ts b/unit/guidebook/guest-stay-timezone.test.ts new file mode 100644 index 00000000..68b51673 --- /dev/null +++ b/unit/guidebook/guest-stay-timezone.test.ts @@ -0,0 +1,82 @@ +import { describe, it, expect, vi, afterEach } from 'vitest' +import { computeStay } from '@/app/g/b/[token]/page' + +// ============================================================================ +// The guest guidebook computed "what day is it for this guest" — and, next to +// it, "what hour is it" — in America/New_York for EVERY property, ignoring +// properties.timezone entirely (the column was not even selected). +// +// Two guest-visible consequences, both worst in the evening: +// +// • computeStay: once Eastern rolls past midnight, `today >= checkoutDate` +// fires, so a guest still mid-stay is shown CHECKOUT instructions. One +// hour early in Central, two in Mountain, three in Pacific, five in Hawaii. +// • hourOfDay: feeds getActiveSlotTypes(), which decides WHICH SPONSOR SLOTS +// render. Sponsors pay for that placement, so the wrong hour shows the +// wrong paying businesses — and the whole 5pm–8pm evening dining window on +// the west coast reads as 8pm–11pm. +// +// Production already has 4 of 27 properties in America/Chicago, so this is +// live, not theoretical — and the error only grows as the product moves west. +// ============================================================================ + +afterEach(() => vi.useRealTimers()) + +/** Freezes wall-clock at a real instant so timezone maths is deterministic. */ +function at(iso: string) { + vi.useFakeTimers() + vi.setSystemTime(new Date(iso)) +} + +describe('computeStay uses the property timezone, not Eastern', () => { + // 2026-08-11T04:30:00Z is 11:30pm Aug 10 in Chicago and 12:30am Aug 11 in + // New York. A guest checking out on Aug 11 is still mid-stay in Chicago. + it('does not flip a Central guest to checkout while it is still checkout eve', () => { + at('2026-08-11T04:30:00Z') + + const central = computeStay('2026-08-08', '2026-08-11', 'America/Chicago') + expect(central.phase, 'still the last night of the stay in Chicago').toBe('mid') + }) + + // The same instant in the timezone the code used to hardcode. Keeping this + // alongside proves the fixture actually straddles a date boundary rather + // than passing for some unrelated reason. + it('would have flipped under the old hardcoded Eastern behaviour', () => { + at('2026-08-11T04:30:00Z') + + const eastern = computeStay('2026-08-08', '2026-08-11', 'America/New_York') + expect(eastern.phase, 'Eastern has already rolled to checkout day').toBe('checkout') + }) + + it('reaches checkout once the guest\'s own day arrives', () => { + at('2026-08-11T14:00:00Z') // 9am Chicago on checkout day + + expect(computeStay('2026-08-08', '2026-08-11', 'America/Chicago').phase).toBe('checkout') + }) + + it('reports arrival before check-in day in the property timezone', () => { + at('2026-08-08T02:00:00Z') // 9pm Aug 7 in Chicago + + const stay = computeStay('2026-08-08', '2026-08-11', 'America/Chicago') + expect(stay.phase).toBe('arrival') + expect(stay.nightIndex).toBe(0) + expect(stay.totalNights).toBe(3) + }) + + // Hawaii is the widest US offset and the case that shows the bug is not a + // one-hour rounding curiosity. + it('is five hours out from Eastern at the extreme', () => { + at('2026-08-11T06:00:00Z') // 8pm Aug 10 in Honolulu, 2am Aug 11 in New York + + expect(computeStay('2026-08-08', '2026-08-11', 'Pacific/Honolulu').phase).toBe('mid') + expect(computeStay('2026-08-08', '2026-08-11', 'America/New_York').phase).toBe('checkout') + }) + + it('counts nights from the dates, independent of timezone', () => { + at('2026-08-09T18:00:00Z') + + for (const tz of ['America/New_York', 'America/Chicago', 'Pacific/Honolulu']) { + expect(computeStay('2026-08-08', '2026-08-11', tz).totalNights, tz).toBe(3) + } + }) +}) diff --git a/unit/guidebook/guidebook-actions.test.ts b/unit/guidebook/guidebook-actions.test.ts index 30c44c01..4dbd9e63 100644 --- a/unit/guidebook/guidebook-actions.test.ts +++ b/unit/guidebook/guidebook-actions.test.ts @@ -7,7 +7,11 @@ vi.mock('@/lib/supabase/server', () => ({ createServiceClient: vi.fn(), })) vi.mock('@/lib/stripe/client', () => ({ - stripe: { checkout: { sessions: { create: vi.fn() } } }, + // retrieve/expire back the duplicate-session defence in + // createSponsorCheckoutSession: reuse an already-open session rather than + // minting a second payable one, and expire our own if we lose the + // compare-and-swap to a concurrent request. + stripe: { checkout: { sessions: { create: vi.fn(), retrieve: vi.fn(), expire: vi.fn() } } }, })) vi.mock('@/lib/inngest/client', () => ({ inngest: { send: vi.fn() } })) vi.mock('@/lib/audit', () => ({ logAuditEvent: vi.fn() })) @@ -41,7 +45,11 @@ function makeSupabase(queue: Record) { // `.not(` is the global-STOP consent check // (.not('opted_out_at', 'is', null)); without it the chain call throws and // every opt-in test collapses into the generic catch. - for (const m of ['select', 'insert', 'update', 'upsert', 'eq', 'not', 'order', 'limit']) { + // `.is(` is the compare-and-swap precondition on a sponsor row that has no + // stored checkout session yet (.is('checkout_session_id', null)); without + // it the chain call throws and the whole action collapses into its generic + // catch, which reads as a Stripe failure. + for (const m of ['select', 'insert', 'update', 'upsert', 'eq', 'is', 'not', 'order', 'limit']) { chain[m] = vi.fn((...args: unknown[]) => { calls.push({ table, method: m, args }) return chain @@ -70,7 +78,8 @@ describe('actions/guidebook', () => { it('creates a Stripe checkout session for a valid, inactive sponsor slot', async () => { const supabase = makeSupabase({ guidebook_sponsors: [ - { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'pending' } }, + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'pending', checkout_session_id: null } }, + { data: { id: 'sponsor_1' } }, // compare-and-swap of the session id wins ], }) vi.mocked(createServiceClient).mockReturnValue(supabase as never) @@ -126,6 +135,130 @@ describe('actions/guidebook', () => { expect(result).toEqual({ error: 'Unable to start checkout. Please try again.' }) }) + + // ── Duplicate-subscription defences ────────────────────────────────── + // Two payable sessions for one sponsor means two subscriptions, two + // checkout.session.completed webhooks with distinct event ids (so the + // Stripe dedup table does not collapse them), and an activation handler + // that overwrites stripe_subscription_id with whichever lands last — + // leaving the other one billing monthly with nothing able to cancel it. + + it('reuses an already-open checkout session instead of minting a second payable one', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'pending', checkout_session_id: 'cs_prior' } }, + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + vi.mocked(stripe.checkout.sessions.retrieve).mockResolvedValue({ + id: 'cs_prior', status: 'open', url: 'https://checkout.stripe.com/cs_prior', + } as never) + + const result = await createSponsorCheckoutSession('kit_token_abc') + + // checkout_session_id was written for exactly this and then never read + // back anywhere, so every click, reload, and retry minted a new session + // — each payable for 24 hours. + expect(result).toEqual({ url: 'https://checkout.stripe.com/cs_prior' }) + expect(stripe.checkout.sessions.create).not.toHaveBeenCalled() + }) + + it('mints a new session when the stored one is no longer open', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'pending', checkout_session_id: 'cs_expired' } }, + { data: { id: 'sponsor_1' } }, // compare-and-swap wins + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + vi.mocked(stripe.checkout.sessions.retrieve).mockResolvedValue({ + id: 'cs_expired', status: 'expired', url: null, + } as never) + vi.mocked(stripe.checkout.sessions.create).mockResolvedValue({ + id: 'cs_2', url: 'https://checkout.stripe.com/cs_2', + } as never) + + const result = await createSponsorCheckoutSession('kit_token_abc') + + expect(result).toEqual({ url: 'https://checkout.stripe.com/cs_2' }) + }) + + it('expires its own session and returns the winner\'s when it loses the compare-and-swap', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'pending', checkout_session_id: null } }, + { data: null }, // CAS matched 0 rows — a concurrent request got there first + { data: { checkout_session_id: 'cs_winner' } }, // re-read finds the winner's session + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + vi.mocked(stripe.checkout.sessions.create).mockResolvedValue({ + id: 'cs_loser', url: 'https://checkout.stripe.com/cs_loser', + } as never) + vi.mocked(stripe.checkout.sessions.retrieve).mockResolvedValue({ + id: 'cs_winner', status: 'open', url: 'https://checkout.stripe.com/cs_winner', + } as never) + vi.mocked(stripe.checkout.sessions.expire).mockResolvedValue({} as never) + + const result = await createSponsorCheckoutSession('kit_token_abc') + + // A plain `if (already active)` before the write is the exact TOCTOU the + // audit checklist calls out — the precondition has to be in the WHERE + // clause, and the loser has to clean up after itself. + expect(stripe.checkout.sessions.expire).toHaveBeenCalledWith('cs_loser') + expect(result).toEqual({ url: 'https://checkout.stripe.com/cs_winner' }) + }) + + it('refuses a sponsor in payment_failed — its subscription is still live in dunning', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'payment_failed', checkout_session_id: null } }, + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + + // payment_failed comes from invoice.payment_failed, which does NOT end + // the subscription — Stripe keeps retrying, and + // guidebook-sponsor-payment-recovered flips the row back to 'active'. + // Only 'active' was blocked, so a sponsor who saw the failure notice + // could buy a SECOND subscription while the first was still in dunning. + const result = await createSponsorCheckoutSession('kit_token_abc') + + expect(result).toEqual({ error: 'This sponsorship slot is already active.' }) + expect(stripe.checkout.sessions.create).not.toHaveBeenCalled() + }) + + it('allows a cancelled sponsor to buy again — that subscription is genuinely gone', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { id: 'sponsor_1', org_id: 'org_1', business_name: 'Lakeside Grill', slot_type: 'restaurant', status: 'cancelled', checkout_session_id: null } }, + { data: { id: 'sponsor_1' } }, + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + vi.mocked(stripe.checkout.sessions.create).mockResolvedValue({ + id: 'cs_3', url: 'https://checkout.stripe.com/cs_3', + } as never) + + const result = await createSponsorCheckoutSession('kit_token_abc') + + expect(result).toEqual({ url: 'https://checkout.stripe.com/cs_3' }) + }) + + it('does not tell a sponsor their valid link is invalid when the lookup itself fails', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [{ data: null, error: { message: 'connection reset', code: '08006' } }], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + + // Discarding the read error collapsed "no such token" and "the query + // failed" into the same null — identical to the defect already fixed in + // optInGuestSms, one function over in the same file. + const result = await createSponsorCheckoutSession('kit_token_abc') + + expect(result).toEqual({ error: 'Unable to start checkout. Please try again.' }) + expect(stripe.checkout.sessions.create).not.toHaveBeenCalled() + }) }) describe('upsertSponsor', () => { @@ -424,7 +557,7 @@ describe('actions/guidebook', () => { data: { id: 'optin_1', phone_e164: '+12065551234', - opted_in_at: new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(), + created_at: new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(), }, error: null, }, @@ -453,7 +586,7 @@ describe('actions/guidebook', () => { data: { id: 'optin_1', phone_e164: '+12065551234', - opted_in_at: new Date(Date.now() - 60 * 1000).toISOString(), // 1 min ago + created_at: new Date(Date.now() - 60 * 1000).toISOString(), // 1 min ago }, error: null, }, @@ -467,6 +600,60 @@ describe('actions/guidebook', () => { expect(result).toEqual({ success: true }) }) + // The window is anchored to created_at, which the upsert never writes — + // not opted_in_at, which it REFRESHES on every submission. Measured from + // opted_in_at it was never "15 minutes from the opt-in" at all: it was 15 + // minutes from the LAST submission, so resubmitting just inside it + // restarted the clock and the window could be walked forward without + // limit — exactly the days-later repoint the comment says a leaked link + // enables. + it('refuses a repoint 24h after the original opt-in even if the row was resubmitted a minute ago', async () => { + vi.mocked(normalizePhoneToE164).mockReturnValue('+12065559999') + const supabase = makeSupabase({ + bookings: [{ data: { id: 'booking_1', org_id: 'org_1', property_id: 'prop_1' } }], + guidebook_guest_sms_optins: [ + { data: null, error: null }, + { + data: { + id: 'optin_1', + phone_e164: '+12065551234', + // Original opt-in a day ago … + created_at: new Date(Date.now() - 24 * 60 * 60 * 1000).toISOString(), + // … but refreshed a minute ago, which used to reopen the window. + opted_in_at: new Date(Date.now() - 60 * 1000).toISOString(), + }, + error: null, + }, + ], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + + const result = await optInGuestSms('valid-guidebook-token', '(206) 555-9999') + + expect(result).toEqual({ + error: 'A different number is already signed up for this stay. Contact your host to change it.', + }) + expect(inngest.send).not.toHaveBeenCalled() + }) + + // Two of the three reads in this action already failed closed with an + // explicit note about why. The booking lookup discarded its error, so a + // transient failure was indistinguishable from a bad token and a guest + // holding a valid link was told the link was invalid. + it('fails closed, not "invalid link", when the booking lookup itself errors', async () => { + vi.mocked(normalizePhoneToE164).mockReturnValue('+12065551234') + const supabase = makeSupabase({ + bookings: [{ data: null, error: { message: 'deadlock detected', code: '40P01' } }], + }) + vi.mocked(createServiceClient).mockReturnValue(supabase as never) + + const result = await optInGuestSms('valid-guidebook-token', '(206) 555-1234') + + expect(result).toEqual({ error: 'Something went wrong. Please try again.' }) + expect(JSON.stringify(result)).not.toMatch(/invalid guidebook link/i) + expect(inngest.send).not.toHaveBeenCalled() + }) + it('rejects an invalid/unrecognized guidebook token before writing anything (IDOR/token check)', async () => { vi.mocked(normalizePhoneToE164).mockReturnValue('+12065551234') const supabase = makeSupabase({ bookings: [{ data: null }] }) diff --git a/unit/inngest/guidebook-guest-opted-in.test.ts b/unit/inngest/guidebook-guest-opted-in.test.ts index ceaf14cb..5d08baaa 100644 --- a/unit/inngest/guidebook-guest-opted-in.test.ts +++ b/unit/inngest/guidebook-guest-opted-in.test.ts @@ -158,26 +158,70 @@ describe('guidebookGuestOptedIn', () => { expect(result).toEqual({ optinId: 'optin_1', sentDoorCode: true }) }) - it('rolls back the claim and throws when the SMS send fails, so a retry can attempt again', async () => { + // The two exits below are NOT the same event, and conflating them is what + // shipped: sendSMS THROWS on a real send failure (dispatchToTelnyx throws on + // a timeout or any non-2xx) and returns {sent:false} only for a DELIBERATE + // skip. One test used to feed a {sent:false} and assert a throw, which + // asserted the wrong half of both behaviours at once. + + it('releases the claim and rethrows when the SMS send THROWS, so the Inngest retry can send again', async () => { const supabase = makeSupabase( { properties: [{ data: propertyRow, error: null }], bookings: [{ data: bookingRow, error: null }], guidebook_guest_sms_optins: [ { data: { id: 'optin_1' }, error: null }, // claim succeeds - { data: null, error: null }, // rollback update + { data: null, error: null }, // claim release ], }, { data: '4321', error: null } ) ;(createServiceClient as ReturnType).mockReturnValue(supabase) - ;(sendSMS as ReturnType).mockResolvedValueOnce({ sent: false, reason: 'SMS_ENABLED is not true' }) + ;(sendSMS as ReturnType).mockRejectedValueOnce(new Error('Telnyx 502')) await expect( invokeHandler(guidebookGuestOptedIn, { event: optedInEvent(), step: makeStep() }) - ).rejects.toThrow('SMS send failed: SMS_ENABLED is not true') + ).rejects.toThrow('Telnyx 502') + + // Without the release, the retry hits `.is('door_code_sent_at', null)`, + // matches zero rows, returns `already_sent` — and the guest never gets + // their door code while the run reports success. + const releaseCall = supabase.calls.filter( + (c) => c.table === 'guidebook_guest_sms_optins' && c.method === 'update' + )[1] + expect(releaseCall?.args[0]).toEqual({ door_code_sent_at: null }) + }) + + it('releases the claim and ends cleanly — no throw — when sendSMS reports a deliberate skip', async () => { + const supabase = makeSupabase( + { + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: bookingRow, error: null }], + guidebook_guest_sms_optins: [ + { data: { id: 'optin_1' }, error: null }, // claim succeeds + { data: null, error: null }, // claim release + ], + }, + { data: '4321', error: null } + ) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(sendSMS as ReturnType).mockResolvedValueOnce({ sent: false, reason: 'SMS_ENABLED is not true' }) + + // SMS_ENABLED=false is production's current state. Throwing here made + // EVERY guest opt-in a failing, retried Inngest run reading "SMS send + // failed" — untrue, and noise that would mask real failures once SMS is + // switched on. A retry cannot change an env var. + const result = await invokeHandler(guidebookGuestOptedIn, { + event: optedInEvent(), + step: makeStep(), + }) + expect(result).toEqual({ optinId: 'optin_1', sentDoorCode: true }) - const rollbackCall = supabase.calls.filter((c) => c.table === 'guidebook_guest_sms_optins' && c.method === 'update')[1] - expect(rollbackCall?.args[0]).toEqual({ door_code_sent_at: null }) + // Still released, for the opposite reason: so the send can happen once + // the skip no longer applies. + const releaseCall = supabase.calls.filter( + (c) => c.table === 'guidebook_guest_sms_optins' && c.method === 'update' + )[1] + expect(releaseCall?.args[0]).toEqual({ door_code_sent_at: null }) }) }) diff --git a/unit/inngest/guidebook-sms-evening-cron.test.ts b/unit/inngest/guidebook-sms-evening-cron.test.ts index 80776845..79a45d4c 100644 --- a/unit/inngest/guidebook-sms-evening-cron.test.ts +++ b/unit/inngest/guidebook-sms-evening-cron.test.ts @@ -13,10 +13,42 @@ vi.mock('@/lib/sms/telnyx', () => ({ vi.mock('@/lib/sms/templates', () => ({ renderSmsBody: vi.fn(async () => 'rendered sms body'), })) -vi.mock('@/lib/sms/optin-claim', () => ({ - claimDailySmsSlot: vi.fn(async () => true), - releaseDailySmsSlot: vi.fn(async () => undefined), -})) +// sendClaimedDailySms is stubbed with a faithful delegating implementation +// rather than a bare vi.fn(): every assertion in this file is about WHICH slot +// gets claimed and whether it is released, and those calls now happen inside +// the helper. Delegating keeps them observable here. The helper's own +// release-on-throw contract is tested directly in unit/sms/optin-claim.test.ts +// — the crons must not be the only place it is covered. +vi.mock('@/lib/sms/optin-claim', () => { + const claimDailySmsSlot = vi.fn(async () => true) + const releaseDailySmsSlot = vi.fn(async () => undefined) + return { + claimDailySmsSlot, + releaseDailySmsSlot, + sendClaimedDailySms: vi.fn(async ( + supabase: unknown, + optinId: string, + dateColumn: string, + todayDate: string, + send: () => Promise<{ sent: boolean }>, + ) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const claimed = await (claimDailySmsSlot as any)(supabase, optinId, dateColumn, todayDate) + if (!claimed) return false + let res: { sent: boolean } + try { + res = await send() + } catch (err) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (releaseDailySmsSlot as any)(supabase, optinId, dateColumn) + throw err + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + if (!res.sent) await (releaseDailySmsSlot as any)(supabase, optinId, dateColumn) + return res.sent + }), + } +}) import { guidebookSmsEveningCron, guidebookSmsEveningSend } from '@/lib/inngest/functions/guidebook-sms-evening-cron' import { createServiceClient } from '@/lib/supabase/server' diff --git a/unit/inngest/guidebook-sms-morning-cron.test.ts b/unit/inngest/guidebook-sms-morning-cron.test.ts index 5fa64264..be5e789c 100644 --- a/unit/inngest/guidebook-sms-morning-cron.test.ts +++ b/unit/inngest/guidebook-sms-morning-cron.test.ts @@ -13,10 +13,42 @@ vi.mock('@/lib/sms/telnyx', () => ({ vi.mock('@/lib/sms/templates', () => ({ renderSmsBody: vi.fn(async () => 'rendered sms body'), })) -vi.mock('@/lib/sms/optin-claim', () => ({ - claimDailySmsSlot: vi.fn(async () => true), - releaseDailySmsSlot: vi.fn(async () => undefined), -})) +// sendClaimedDailySms is stubbed with a faithful delegating implementation +// rather than a bare vi.fn(): every assertion in this file is about WHICH slot +// gets claimed and whether it is released, and those calls now happen inside +// the helper. Delegating keeps them observable here. The helper's own +// release-on-throw contract is tested directly in unit/sms/optin-claim.test.ts +// — the crons must not be the only place it is covered. +vi.mock('@/lib/sms/optin-claim', () => { + const claimDailySmsSlot = vi.fn(async () => true) + const releaseDailySmsSlot = vi.fn(async () => undefined) + return { + claimDailySmsSlot, + releaseDailySmsSlot, + sendClaimedDailySms: vi.fn(async ( + supabase: unknown, + optinId: string, + dateColumn: string, + todayDate: string, + send: () => Promise<{ sent: boolean }>, + ) => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const claimed = await (claimDailySmsSlot as any)(supabase, optinId, dateColumn, todayDate) + if (!claimed) return false + let res: { sent: boolean } + try { + res = await send() + } catch (err) { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (releaseDailySmsSlot as any)(supabase, optinId, dateColumn) + throw err + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + if (!res.sent) await (releaseDailySmsSlot as any)(supabase, optinId, dateColumn) + return res.sent + }), + } +}) import { guidebookSmsMorningCron, guidebookSmsMorningSend } from '@/lib/inngest/functions/guidebook-sms-morning-cron' import { createServiceClient } from '@/lib/supabase/server' @@ -330,4 +362,84 @@ describe('guidebookSmsMorningSend (per-guest handler)', () => { expect(result).toEqual({ optinId: 'optin_1', sent: false }) expect(releaseDailySmsSlot).toHaveBeenCalledWith(supabase, 'optin_1', 'last_morning_sms_date') }) + + // ── Check-in day ────────────────────────────────────────────────────────── + // The cron's eligibility filter is `checkin_date <= today AND checkout_date + // >= today`, so a guest whose stay STARTS today is included — but the cron + // fires 7-11 AM and check-in is typically mid-afternoon. + + it('sends an arrival reminder — not the weather/sponsor nudge — on the guest\'s check-in day', async () => { + const supabase = makeSupabase({ + guidebook_guest_sms_optins: [{ data: optinDetailRow(), error: null }], + properties: [{ data: { ...propertyRow, checkin_time: '16:00:00' }, error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + + const arrivalEvent = { data: { ...sendEvent.data, checkin_date: '2026-07-22' } } // == today_date + + const result = await invokeHandler(guidebookSmsMorningSend, { event: arrivalEvent, step: makeStep() }) + + // Previously this guest got "it's 72°F at your rental, here's a coffee + // spot 0.4 mi away" hours before they had keys. + expect(renderSmsBody).toHaveBeenCalledWith('org_1', 'arrival_reminder', { + property_name: 'Lake House', + checkin_line: 'Just a reminder that check-in is at 4:00 PM.', + }) + expect(renderSmsBody).not.toHaveBeenCalledWith('org_1', 'morning_nudge', expect.anything()) + expect(sendSMS).toHaveBeenCalledWith('+15551234567', 'rendered sms body', { category: 'nudge', orgId: 'org_1' }) + expect(result).toEqual({ optinId: 'optin_1', sent: true }) + }) + + it('does not need weather or coordinates to send the arrival reminder', async () => { + const supabase = makeSupabase({ + guidebook_guest_sms_optins: [{ data: optinDetailRow(), error: null }], + // No lat/lng — a property that could never receive the normal nudge. + properties: [{ data: { id: 'prop_1', name: 'Lake House', lat: null, lng: null, checkin_time: '15:00:00' }, error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + + const arrivalEvent = { data: { ...sendEvent.data, checkin_date: '2026-07-22' } } + const result = await invokeHandler(guidebookSmsMorningSend, { event: arrivalEvent, step: makeStep() }) + + expect(getWeatherForLocation).not.toHaveBeenCalled() + expect(result).toEqual({ optinId: 'optin_1', sent: true }) + }) + + it('omits the check-in sentence entirely when the property has no check-in time', async () => { + const supabase = makeSupabase({ + guidebook_guest_sms_optins: [{ data: optinDetailRow(), error: null }], + // OwnerRez-synced properties explicitly write null here. + properties: [{ data: { ...propertyRow, checkin_time: null }, error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + + const arrivalEvent = { data: { ...sendEvent.data, checkin_date: '2026-07-22' } } + await invokeHandler(guidebookSmsMorningSend, { event: arrivalEvent, step: makeStep() }) + + // Empty, not "check-in is at ." — the template joins on a truthy check. + expect(renderSmsBody).toHaveBeenCalledWith('org_1', 'arrival_reminder', { + property_name: 'Lake House', + checkin_line: '', + }) + }) + + it('still sends the normal nudge mid-stay, including on checkout morning', async () => { + const supabase = makeSupabase({ + guidebook_guest_sms_optins: [{ data: optinDetailRow(), error: null }], + properties: [{ data: propertyRow, error: null }], + guidebook_sponsors: [{ data: [sponsorRow()], error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getWeatherForLocation as ReturnType).mockResolvedValue(clearWeather) + + // checkin_date < today. The send handler is not told checkout_date at all, + // so the outgoing guest on a same-day flip takes exactly this path — they + // ARE still in the house on checkout morning and a local recommendation + // still lands. Deliberately unchanged. + await invokeHandler(guidebookSmsMorningSend, { event: sendEvent, step: makeStep() }) + + expect(renderSmsBody).toHaveBeenCalledWith('org_1', 'morning_nudge', expect.anything()) + expect(renderSmsBody).not.toHaveBeenCalledWith('org_1', 'arrival_reminder', expect.anything()) + }) + }) diff --git a/unit/inngest/guidebook-sponsor-activated.test.ts b/unit/inngest/guidebook-sponsor-activated.test.ts index 120cf84d..1cd26faf 100644 --- a/unit/inngest/guidebook-sponsor-activated.test.ts +++ b/unit/inngest/guidebook-sponsor-activated.test.ts @@ -9,11 +9,15 @@ vi.mock('@/lib/guidebook/helpers', () => ({ vi.mock('@/lib/audit', () => ({ logAuditEvent: vi.fn(), })) +vi.mock('@/lib/observability/report-error', () => ({ + reportError: vi.fn(), +})) import { guidebookSponsorActivated } from '@/lib/inngest/functions/guidebook-sponsor-activated' import { createServiceClient } from '@/lib/supabase/server' import { getActiveSponsorCount } from '@/lib/guidebook/helpers' import { logAuditEvent } from '@/lib/audit' +import { reportError } from '@/lib/observability/report-error' import { invokeHandler } from './test-helpers' // Queue-based `.from(table)` mock — same convention as checklist-broadcast @@ -92,8 +96,24 @@ describe('guidebookSponsorActivated', () => { stripe_subscription_id: 'sub_1', stripe_customer_id: 'cus_1', }) - const sponsorEqCalls = supabase.calls.filter((c) => c.table === 'guidebook_sponsors' && c.method === 'eq') - expect(sponsorEqCalls.map((c) => c.args)).toEqual([['id', 'sponsor_1'], ['org_id', 'org_1']]) + // Stated as "every guidebook_sponsors access is scoped by BOTH id and + // org_id" rather than as a positional list, so adding a read to this step + // cannot quietly weaken it into passing on a subset. + const sponsorEqArgs = supabase.calls + .filter((c) => c.table === 'guidebook_sponsors' && c.method === 'eq') + .map((c) => JSON.stringify(c.args)) + + const idScoped = sponsorEqArgs.filter((a) => a === JSON.stringify(['id', 'sponsor_1'])).length + const orgScoped = sponsorEqArgs.filter((a) => a === JSON.stringify(['org_id', 'org_1'])).length + const distinctArgs = new Set(sponsorEqArgs) + + // Every id filter is matched by an org filter, so no statement can reach a + // sponsor row on id alone. + expect(orgScoped).toBe(idScoped) + expect(idScoped).toBeGreaterThan(0) + expect(distinctArgs).toEqual( + new Set([JSON.stringify(['id', 'sponsor_1']), JSON.stringify(['org_id', 'org_1'])]), + ) const configUpsert = supabase.calls.find((c) => c.table === 'guidebook_configurations' && c.method === 'upsert') expect(configUpsert?.args[0]).toMatchObject({ org_id: 'org_1', is_active: true, grace_period_ends_at: null }) @@ -171,4 +191,72 @@ describe('guidebookSponsorActivated', () => { expect(supabase.calls.filter((c) => c.table === 'guidebook_sponsors' && c.method === 'update')).toHaveLength(2) expect(logAuditEvent).toHaveBeenCalledTimes(2) }) + + it('throws when the unlock upsert fails, instead of reporting an unlock that never happened', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [{ data: null, error: null }], + guidebook_configurations: [ + { data: { trial_ends_at: null }, error: null }, + { data: null, error: { message: 'permission denied for table guidebook_configurations' } }, + ], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getActiveSponsorCount as ReturnType).mockResolvedValue(3) + + // Discarded, a failed unlock left the guidebook locked while the step + // returned wasUnlocked: true and the audit row recorded an unlock that + // never happened — the sponsor paid and the guidebook stayed dark, with + // the record saying otherwise. Matches the identical upsert in + // guidebook-sponsor-payment-recovered, which always threw. + await expect( + invokeHandler(guidebookSponsorActivated, { event: checkoutEvent(), step: makeStep() }) + ).rejects.toThrow(/Failed to unlock guidebook/) + + expect(logAuditEvent).not.toHaveBeenCalled() + }) + + it('reports — rather than silently overwriting — when a second subscription id replaces a live one', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { stripe_subscription_id: 'sub_previous' }, error: null }, // already has a live subscription + { data: null, error: null }, // the activation update + ], + guidebook_configurations: [{ data: { trial_ends_at: null }, error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getActiveSponsorCount as ReturnType).mockResolvedValue(1) + + await invokeHandler(guidebookSponsorActivated, { event: checkoutEvent(), step: makeStep() }) + + // Overwriting stripe_subscription_id orphans the previous subscription: + // it keeps billing the business monthly and nothing in FieldStay can + // reach it any more. The write still proceeds (the new subscription is + // the real one now) but the anomaly must not pass unnoticed. + expect(reportError).toHaveBeenCalledWith( + expect.objectContaining({ message: 'Sponsor activated with a second Stripe subscription' }), + expect.objectContaining({ + site: 'inngest.guidebook-sponsor-activated.duplicate-subscription', + extra: expect.objectContaining({ + prior_subscription_id: 'sub_previous', + new_subscription_id: 'sub_1', + }), + }), + ) + }) + + it('does not report when a replay writes back the SAME subscription id', async () => { + const supabase = makeSupabase({ + guidebook_sponsors: [ + { data: { stripe_subscription_id: 'sub_1' }, error: null }, // same as the event's + { data: null, error: null }, + ], + guidebook_configurations: [{ data: { trial_ends_at: null }, error: null }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getActiveSponsorCount as ReturnType).mockResolvedValue(1) + + await invokeHandler(guidebookSponsorActivated, { event: checkoutEvent(), step: makeStep() }) + + expect(reportError).not.toHaveBeenCalled() + }) }) diff --git a/unit/inngest/guidebook-stay-extension-handler.test.ts b/unit/inngest/guidebook-stay-extension-handler.test.ts index e5110013..6f8ab408 100644 --- a/unit/inngest/guidebook-stay-extension-handler.test.ts +++ b/unit/inngest/guidebook-stay-extension-handler.test.ts @@ -229,6 +229,128 @@ describe('guidebookStayExtensionHandler', () => { expect(sendSMS).not.toHaveBeenCalled() }) + it('throws instead of silently dropping the offer when the context read fails', async () => { + const supabase = makeSupabase({ + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: null, error: { message: 'connection reset', code: '08006' } }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getPmEmails as ReturnType).mockResolvedValue(['pm@example.com']) + + // Both errors used to be discarded, which left `booking` null — so + // `portalUrl` was null, the entire guest-SMS block was skipped, and the PM + // email went out reading "checks out on undefined". The step reported + // success, so Inngest never retried and nothing was logged. + await expect( + invokeHandler(guidebookStayExtensionHandler, { event: requestEvent(), step: makeStep() }) + ).rejects.toThrow(/Supabase query failed/) + + expect(sendSMS).not.toHaveBeenCalled() + expect(resend.emails.send).not.toHaveBeenCalled() + }) + + // The two guest-SMS exits below are NOT the same event. sendSMS THROWS on a + // real send failure (dispatchToTelnyx throws on a timeout or any non-2xx) and + // returns {sent:false} only for a DELIBERATE skip. Only the {sent:false} half + // released the claim; the throw escaped with it still held. + it('releases the guest-SMS claim and rethrows when the send THROWS, so the retry can send again', async () => { + const supabase = makeSupabase({ + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: bookingRow, error: null }], + guidebook_guest_sms_optins: [{ data: { is_active: true }, error: null }], + stay_extension_requests: [ + { data: { id: 'req_1' }, error: null }, // claim succeeds + { data: null, error: null }, // claim release + ], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(sendSMS as ReturnType).mockRejectedValueOnce(new Error('Telnyx 502')) + + await expect( + invokeHandler(guidebookStayExtensionHandler, { event: requestEvent(), step: makeStep() }) + ).rejects.toThrow('Telnyx 502') + + // Without the release, the retry hits `.is('sms_sent_at', null)`, matches + // zero rows, returns `already_sent` — the guest never gets the offer and + // the run reports success. + const releaseCall = supabase.calls.filter( + (c) => c.table === 'stay_extension_requests' && c.method === 'update', + )[1] + expect(releaseCall?.args[0]).toEqual({ sms_sent_at: null }) + }) + + it('releases the guest-SMS claim and ends cleanly — no throw — when the send is a deliberate skip', async () => { + const supabase = makeSupabase({ + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: bookingRow, error: null }], + guidebook_guest_sms_optins: [{ data: { is_active: true }, error: null }], + stay_extension_requests: [ + { data: { id: 'req_1' }, error: null }, // claim succeeds + { data: null, error: null }, // claim release + { data: null, error: null }, // pm_notified_at stamp + ], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getPmEmails as ReturnType).mockResolvedValue(['pm@example.com']) + ;(sendSMS as ReturnType).mockResolvedValueOnce({ sent: false, reason: 'SMS_ENABLED is not true' }) + + // SMS_ENABLED=false is production's current state — it must not turn every + // gap-night offer into a retried failure. Released anyway, for the opposite + // reason: so the send can happen once the skip no longer applies. + await invokeHandler(guidebookStayExtensionHandler, { event: requestEvent(), step: makeStep() }) + + const releaseCall = supabase.calls.filter( + (c) => c.table === 'stay_extension_requests' && c.method === 'update', + )[1] + expect(releaseCall?.args[0]).toEqual({ sms_sent_at: null }) + expect(resend.emails.send).toHaveBeenCalled() + }) + + it('releases the PM-SMS claim and rethrows when the PM send THROWS', async () => { + const supabase = makeSupabase({ + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: bookingRow, error: null }], + profiles: [{ data: { phone: '512-555-9999' }, error: null }], + stay_extension_requests: [ + { data: { id: 'req_1' }, error: null }, // pm-sms claim succeeds + { data: null, error: null }, // claim release + ], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + ;(getPmMembers as ReturnType).mockResolvedValue([{ userId: 'u1', email: 'pm@example.com', role: 'owner' }]) + ;(sendSMS as ReturnType).mockRejectedValueOnce(new Error('Telnyx 502')) + + await expect( + invokeHandler(guidebookStayExtensionHandler, { + event: requestEvent({ guestPhoneE164: null, contactMethod: 'sms' }), + step: makeStep(), + }) + ).rejects.toThrow('Telnyx 502') + + const releaseCall = supabase.calls.filter( + (c) => c.table === 'stay_extension_requests' && c.method === 'update', + )[1] + expect(releaseCall?.args[0]).toEqual({ pm_notified_at: null }) + }) + + it('retries rather than skipping forever when the atomic claim itself errors', async () => { + const supabase = makeSupabase({ + properties: [{ data: propertyRow, error: null }], + bookings: [{ data: bookingRow, error: null }], + guidebook_guest_sms_optins: [{ data: { is_active: true }, error: null }], + // A FAILED claim returns null data too — indistinguishable from "already + // claimed" once the error is dropped, so the send was skipped forever. + stay_extension_requests: [{ data: null, error: { message: 'deadlock detected', code: '40P01' } }], + }) + ;(createServiceClient as ReturnType).mockReturnValue(supabase) + + await expect( + invokeHandler(guidebookStayExtensionHandler, { event: requestEvent(), step: makeStep() }) + ).rejects.toThrow(/Supabase query failed/) + + expect(sendSMS).not.toHaveBeenCalled() + }) + it('idempotency: does not re-notify the PM by SMS when a prior run already claimed pm_notified_at', async () => { const supabase = makeSupabase({ properties: [{ data: propertyRow, error: null }], diff --git a/unit/lib/guidebook-offer.test.ts b/unit/lib/guidebook-offer.test.ts index fe71d0de..cdfbb2b9 100644 --- a/unit/lib/guidebook-offer.test.ts +++ b/unit/lib/guidebook-offer.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { formatOffer } from '@/lib/guidebook/offer' +import { formatOffer, asOfferType } from '@/lib/guidebook/offer' describe('formatOffer', () => { describe('percentage', () => { @@ -76,3 +76,37 @@ describe('formatOffer', () => { }) }) }) + +describe('asOfferType', () => { + // guidebook_sponsors.offer_type is a TEXT column with a CHECK constraint, so + // PostgREST hands it back as a bare string. This is the narrowing boundary. + + it.each(['percentage', 'fixed_amount', 'item', 'custom', 'none'])( + 'passes through the known offer type %s', + (value) => { + expect(asOfferType(value)).toBe(value) + }, + ) + + it.each([ + ['an unknown string', 'bogo'], + ['an empty string', ''], + ['null', null], + ['undefined', undefined], + ])('falls back to none for %s', (_label, value) => { + // 'none' is the safe direction: formatOffer renders nothing for it, so a + // sponsor line is omitted rather than built from a value nothing + // understands. + expect(asOfferType(value)).toBe('none') + }) + + it('the fallback that makes unknown input safe is what makes a MISSING entry invisible', () => { + // Which is why the member list is a Record and + // not an array: adding a value to the union fails the BUILD until it is + // listed. There is no runtime assertion that can catch this — a forgotten + // entry looks exactly like a genuinely unknown value, and every sponsor + // using it would silently show no offer. Verified by reverting the Record + // to an array and adding a union member: tsc stayed green. + expect(asOfferType('bogo')).toBe('none') + }) +}) diff --git a/unit/lib/sms-template-registry.test.ts b/unit/lib/sms-template-registry.test.ts index 6fb728bb..412c8754 100644 --- a/unit/lib/sms-template-registry.test.ts +++ b/unit/lib/sms-template-registry.test.ts @@ -8,6 +8,7 @@ import { const EXPECTED_KEYS: SmsTemplateKey[] = [ 'door_code', 'morning_nudge', + 'arrival_reminder', 'evening_nudge', 'rain_alert', 'stay_extension', @@ -57,7 +58,7 @@ describe('renderTemplate', () => { }) describe('SMS_TEMPLATE_REGISTRY', () => { - it('registers exactly the nine known template keys, each exactly once', () => { + it('registers exactly the ten known template keys, each exactly once', () => { const keys = SMS_TEMPLATE_REGISTRY.map((t) => t.key) expect(keys.sort()).toEqual([...EXPECTED_KEYS].sort()) expect(new Set(keys).size).toBe(keys.length) diff --git a/unit/pages/media-kit-token-shape.test.ts b/unit/pages/media-kit-token-shape.test.ts new file mode 100644 index 00000000..97225ebf --- /dev/null +++ b/unit/pages/media-kit-token-shape.test.ts @@ -0,0 +1,93 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('next/navigation', () => ({ + notFound: vi.fn(() => { throw new Error('NEXT_NOT_FOUND') }), +})) +vi.mock('@/lib/supabase/server', () => ({ + createServiceClient: vi.fn(), +})) +// next/font/google runs a real font fetch at import time in this environment. +vi.mock('next/font/google', () => ({ + Archivo: () => ({ variable: 'archivo' }), + Source_Serif_4: () => ({ variable: 'serif' }), +})) +vi.mock('@/app/g/kit/[media_kit_token]/media-kit-client', () => ({ + MediaKitClient: () => null, +})) +vi.mock('@/app/g/kit/[media_kit_token]/print/print-kit', () => ({ + PrintKit: () => null, +})) + +import MediaKitPage from '@/app/g/kit/[media_kit_token]/page' +import PrintKitPage from '@/app/g/kit/[media_kit_token]/print/page' +import { createServiceClient } from '@/lib/supabase/server' +import { notFound } from 'next/navigation' + +const VALID_TOKEN = '66666666-6666-4666-8666-666666666666' + +/** + * Answers 22P02 for a non-UUID compared against a `uuid` column, because that + * is what Postgres does — it does not return zero rows. A double that quietly + * accepts anything makes a malformed-input test pass whether the handling + * exists or not. + */ +const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i + +function makeSupabase() { + const from = vi.fn(() => { + const eqArgs: [string, unknown][] = [] + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const chain: any = {} + chain.select = vi.fn(() => chain) + chain.eq = vi.fn((col: string, val: unknown) => { eqArgs.push([col, val]); return chain }) + chain.maybeSingle = vi.fn(() => { + const bad = eqArgs.find(([col, val]) => + (col === 'media_kit_token' || col === 'id') && !UUID_RE.test(String(val))) + if (bad) { + return Promise.resolve({ + data: null, + error: { code: '22P02', message: `invalid input syntax for type uuid: "${String(bad[1])}"` }, + }) + } + return Promise.resolve({ data: null, error: null }) + }) + return chain + }) + return { from } +} + +const pages: [string, (a: { params: Promise<{ media_kit_token: string }> }) => Promise][] = [ + ['media kit', MediaKitPage as never], + ['print media kit', PrintKitPage as never], +] + +describe.each(pages)('/g/kit — %s page token shape', (_label, Page) => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(createServiceClient).mockReturnValue(makeSupabase() as never) + }) + + it('404s a malformed token instead of rendering an outage', async () => { + // The intent already documented on these pages is that a failed READ must + // not read as an invalid token. The inverse held just as strongly and was + // not handled: media_kit_token is a `uuid`, so a malformed one is 22P02, + // which unwrap() escalates into the segment error boundary — telling a + // sponsor with a plainly bad URL that something went wrong on our side. + await expect( + Page({ params: Promise.resolve({ media_kit_token: 'kit-token-abc-123' }) }) + ).rejects.toThrow('NEXT_NOT_FOUND') + + expect(notFound).toHaveBeenCalled() + // Cheap and load-bearing: no database round trip for input that could + // never match, on a public unauthenticated surface. + expect(createServiceClient).not.toHaveBeenCalled() + }) + + it('still queries — and 404s — for a well-formed token that matches no sponsor', async () => { + await expect( + Page({ params: Promise.resolve({ media_kit_token: VALID_TOKEN }) }) + ).rejects.toThrow('NEXT_NOT_FOUND') + + expect(createServiceClient).toHaveBeenCalled() + }) +}) diff --git a/unit/route-handlers/guidebook-redeem.test.ts b/unit/route-handlers/guidebook-redeem.test.ts index 0081c0d9..13a3b78d 100644 --- a/unit/route-handlers/guidebook-redeem.test.ts +++ b/unit/route-handlers/guidebook-redeem.test.ts @@ -4,6 +4,9 @@ import { NextRequest } from 'next/server' vi.mock('@/lib/supabase/server', () => ({ createServiceClient: vi.fn(), })) +vi.mock('@/lib/observability/report-error', () => ({ + reportError: vi.fn(), +})) vi.mock('@/lib/rate-limit', async () => { // checkLimit() is now the only sanctioned way to consult a limiter // (lib/rate-limit.ts). The stub delegates to the limiter doubles below @@ -19,24 +22,60 @@ vi.mock('@/lib/rate-limit', async () => { import { POST } from '@/app/api/guidebook/redeem/route' import { createServiceClient } from '@/lib/supabase/server' import { guidebookRedeemLimiter } from '@/lib/rate-limit' - -const SPONSOR_ID = 'sponsor_1' -const ORG_ID = 'org_1' -const OTHER_ORG = 'org_2' +import { reportError } from '@/lib/observability/report-error' + +// Real UUIDs, because every one of these ids is a Postgres `uuid` column and +// the route now shape-checks before querying. The previous fixtures +// ('sponsor_1', 'tok_abc') could not have existed in production — against the +// live schema they are error 22P02, not a miss, so the suite was green on +// inputs the database would have rejected outright. +const SPONSOR_ID = '11111111-1111-4111-8111-111111111111' +const ORG_ID = '22222222-2222-4222-8222-222222222222' +const OTHER_ORG = '33333333-3333-4333-8333-333333333333' +const BOOKING_ID = '44444444-4444-4444-8444-444444444444' +const BOOKING_TOK = '55555555-5555-4555-8555-555555555555' + +/** Columns the live schema declares as `uuid` — see the mock's 22P02 branch. */ +const UUID_COLUMNS = new Set(['id', 'guidebook_token']) +const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i function makeServiceClient(opts: { sponsorResult?: { data: unknown; error?: unknown } bookingResult?: { data: unknown; error?: unknown } insertResult?: { error: unknown } } = {}) { + // The write goes through record_guidebook_offer_open (migration + // 20260807170000), which does the insert-or-increment in one statement — the + // arbiter is a partial EXPRESSION index that PostgREST's on_conflict cannot + // name, and read-then-write in the route would be a TOCTOU. + const rpcMock = vi.fn(() => Promise.resolve(opts.insertResult ?? { error: null })) const insertMock = vi.fn(() => Promise.resolve(opts.insertResult ?? { error: null })) const from = vi.fn((table: string) => { // eslint-disable-next-line @typescript-eslint/no-explicit-any const chain: any = {} + const eqArgs: [string, unknown][] = [] chain.select = vi.fn(() => chain) - chain.eq = vi.fn(() => chain) + chain.eq = vi.fn((col: string, val: unknown) => { eqArgs.push([col, val]); return chain }) chain.maybeSingle = vi.fn(() => { + // The double answers 22P02 for a non-UUID filtered against a `uuid` + // column, because that is what Postgres does — it does NOT return zero + // rows. Without this the mock silently accepts ids the real database + // rejects, and any test about malformed-input handling passes whether + // the handling exists or not. + const badUuid = eqArgs.find( + ([col, val]) => UUID_COLUMNS.has(col) && !UUID_RE.test(String(val)), + ) + if (badUuid) { + return Promise.resolve({ + data: null, + error: { + code: '22P02', + message: `invalid input syntax for type uuid: "${String(badUuid[1])}"`, + }, + }) + } + if (table === 'guidebook_sponsors') { return Promise.resolve( opts.sponsorResult ?? { data: { id: SPONSOR_ID, org_id: ORG_ID, status: 'active' }, error: null }, @@ -51,7 +90,7 @@ function makeServiceClient(opts: { return chain }) - return { from, insertMock } + return { from, insertMock, rpc: rpcMock } } function postRequest(body: unknown) { @@ -82,6 +121,39 @@ describe('POST /api/guidebook/redeem', () => { expect(createServiceClient).not.toHaveBeenCalled() }) + it('returns 400 for a malformed sponsorId rather than letting Postgres 22P02 become a Sentry report', async () => { + const res = await POST(postRequest({ sponsorId: 'sponsor_1' })) + + // A well-formed string that is not a UUID used to sail past the + // `typeof === 'string'` check straight into `.eq('id', …)` on a uuid + // column. Postgres answers 22P02, unwrap() throws, and the catch returns + // {ok:true} PLUS a reportError — so on a public unauthenticated endpoint + // anyone could burn the Sentry quota, and this route's genuine DB failures + // would be buried in that noise. + expect(res.status).toBe(400) + expect(createServiceClient).not.toHaveBeenCalled() + }) + + it('still logs the redemption — anonymously — when the bookingToken is malformed', async () => { + const service = makeServiceClient() + vi.mocked(createServiceClient).mockReturnValue(service as never) + + const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: 'tok_abc' })) + + // bookings.guidebook_token is a uuid too. A malformed one threw 22P02 out + // of unwrap(), escaped the try block entirely, and hit the outer catch — + // so the insert never ran and the redemption was DISCARDED, not just left + // unattributed, while the guest still saw {ok:true}. The route's own + // stated fallback is an anonymous redemption; this is it. + expect(res.status).toBe(200) + // p_booking_id omitted, not null: the function declares DEFAULT NULL so + // the anonymous case is expressible without a cast. + expect(service.rpc).toHaveBeenCalledWith('record_guidebook_offer_open', { + p_org_id: ORG_ID, + p_sponsor_id: SPONSOR_ID, + }) + }) + it('returns 429 when the per-IP rate limit is exceeded, without querying the database', async () => { vi.mocked(guidebookRedeemLimiter.limit).mockResolvedValue({ success: false } as never) @@ -95,12 +167,12 @@ describe('POST /api/guidebook/redeem', () => { const service = makeServiceClient({ sponsorResult: { data: null, error: null } }) vi.mocked(createServiceClient).mockReturnValue(service as never) - const res = await POST(postRequest({ sponsorId: 'nonexistent' })) + const res = await POST(postRequest({ sponsorId: '99999999-9999-4999-8999-999999999999' })) const json = await res.json() expect(res.status).toBe(200) expect(json).toEqual({ ok: true }) - expect(service.insertMock).not.toHaveBeenCalled() + expect(service.rpc).not.toHaveBeenCalled() }) it('returns ok:true without inserting when the sponsor is not active', async () => { @@ -114,7 +186,7 @@ describe('POST /api/guidebook/redeem', () => { expect(res.status).toBe(200) expect(json).toEqual({ ok: true }) - expect(service.insertMock).not.toHaveBeenCalled() + expect(service.rpc).not.toHaveBeenCalled() }) it('logs the redemption without a booking_id when no bookingToken is supplied', async () => { @@ -124,43 +196,64 @@ describe('POST /api/guidebook/redeem', () => { const res = await POST(postRequest({ sponsorId: SPONSOR_ID })) expect(res.status).toBe(200) - expect(service.insertMock).toHaveBeenCalledWith({ - org_id: ORG_ID, - sponsor_id: SPONSOR_ID, - booking_id: null, + // p_booking_id omitted, not null: the function declares DEFAULT NULL so + // the anonymous case is expressible without a cast. + expect(service.rpc).toHaveBeenCalledWith('record_guidebook_offer_open', { + p_org_id: ORG_ID, + p_sponsor_id: SPONSOR_ID, }) }) it('attaches the booking_id when the booking token resolves to a booking in the sponsor\'s own org', async () => { const service = makeServiceClient({ - bookingResult: { data: { id: 'booking_1', org_id: ORG_ID }, error: null }, + bookingResult: { data: { id: BOOKING_ID, org_id: ORG_ID }, error: null }, }) vi.mocked(createServiceClient).mockReturnValue(service as never) - const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: 'tok_abc' })) + const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: BOOKING_TOK })) expect(res.status).toBe(200) - expect(service.insertMock).toHaveBeenCalledWith({ - org_id: ORG_ID, - sponsor_id: SPONSOR_ID, - booking_id: 'booking_1', + expect(service.rpc).toHaveBeenCalledWith('record_guidebook_offer_open', { + p_org_id: ORG_ID, + p_sponsor_id: SPONSOR_ID, + p_booking_id: BOOKING_ID, }) }) it('logs anonymously (booking_id: null) when the booking token belongs to a different org — tenant isolation', async () => { const service = makeServiceClient({ - bookingResult: { data: { id: 'booking_1', org_id: OTHER_ORG }, error: null }, + bookingResult: { data: { id: BOOKING_ID, org_id: OTHER_ORG }, error: null }, }) vi.mocked(createServiceClient).mockReturnValue(service as never) - const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: 'tok_cross_org' })) + const res = await POST(postRequest({ sponsorId: SPONSOR_ID, bookingToken: BOOKING_TOK })) expect(res.status).toBe(200) - expect(service.insertMock).toHaveBeenCalledWith({ - org_id: ORG_ID, - sponsor_id: SPONSOR_ID, - booking_id: null, + // p_booking_id omitted, not null: the function declares DEFAULT NULL so + // the anonymous case is expressible without a cast. + expect(service.rpc).toHaveBeenCalledWith('record_guidebook_offer_open', { + p_org_id: ORG_ID, + p_sponsor_id: SPONSOR_ID, + }) + }) + + it('reports a write failure from the open-recorder RPC', async () => { + const reportSpy = vi.mocked(reportError) + const service = makeServiceClient({ + insertResult: { error: { code: '42501', message: 'new row violates row-level security policy' } }, }) + vi.mocked(createServiceClient).mockReturnValue(service as never) + + const res = await POST(postRequest({ sponsorId: SPONSOR_ID })) + + // Best-effort for the guest — they still get their offer — but not silent: + // a sustained failure here is indistinguishable from "nobody redeemed + // anything" in the sponsor's reporting. + expect(res.status).toBe(200) + expect(reportSpy).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ site: 'route.guidebook.redeem.insert' }), + ) }) it('returns ok:true even when the insert fails (table not yet migrated) — never fails the guest UX', async () => { diff --git a/unit/route-handlers/guidebook-sponsor-checkout.test.ts b/unit/route-handlers/guidebook-sponsor-checkout.test.ts index e7a18def..3ea61952 100644 --- a/unit/route-handlers/guidebook-sponsor-checkout.test.ts +++ b/unit/route-handlers/guidebook-sponsor-checkout.test.ts @@ -28,6 +28,13 @@ function postRequest(body: unknown) { }) } +// Real UUIDs: guidebook_sponsors.media_kit_token is a `uuid` column, so the +// previous fixture ('kit-token-abc-123') could not have existed in production +// — against the live schema it is error 22P02, not a miss. The suite was green +// on input the database would have rejected outright. +const KIT_TOKEN = '66666666-6666-4666-8666-666666666666' +const MISSING_TOKEN = '77777777-7777-4777-8777-777777777777' + describe('POST /api/guidebook/sponsor-checkout', () => { beforeEach(() => { vi.clearAllMocks() @@ -37,7 +44,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { it('returns 429 without touching the checkout action when the limiter denies', async () => { vi.mocked(guidebookSponsorCheckoutLimiter.limit).mockResolvedValue({ success: false } as never) - const res = await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + const res = await POST(postRequest({ mediaKitToken: KIT_TOKEN })) const json = await res.json() expect(res.status).toBe(429) @@ -56,7 +63,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { return { url: 'https://checkout.stripe.com/pay/cs_1' } }) - await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + await POST(postRequest({ mediaKitToken: KIT_TOKEN })) expect(callOrder).toEqual(['limit', 'lookup']) }) @@ -72,7 +79,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { 'x-real-ip': '203.0.113.7', 'x-forwarded-for': '1.2.3.4, 203.0.113.7', }, - body: JSON.stringify({ mediaKitToken: 'kit-token-abc-123' }), + body: JSON.stringify({ mediaKitToken: KIT_TOKEN }), }) vi.mocked(createSponsorCheckoutSession).mockResolvedValue({ url: 'https://checkout.stripe.com/pay/cs_1' }) @@ -96,18 +103,32 @@ describe('POST /api/guidebook/sponsor-checkout', () => { expect(createSponsorCheckoutSession).not.toHaveBeenCalled() }) + it('rejects a malformed mediaKitToken as an invalid link, without reaching the action', async () => { + const res = await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + const json = await res.json() + + // media_kit_token is a uuid. A well-formed-but-not-UUID string used to + // sail past the typeof check into the action's `.eq()`, hit 22P02, throw + // out of unwrap(), and land in the action's catch — which reports to + // Sentry and tells the sponsor "Unable to start checkout. Please try + // again." for a link that will never work however many times they try. + expect(res.status).toBe(400) + expect(json).toEqual({ error: 'Invalid media kit link.' }) + expect(createSponsorCheckoutSession).not.toHaveBeenCalled() + }) + it('passes the exact client-supplied mediaKitToken through to the action unmodified — the action itself is the only place org scoping happens (media_kit_token lookup)', async () => { vi.mocked(createSponsorCheckoutSession).mockResolvedValue({ url: 'https://checkout.stripe.com/pay/cs_1' }) - await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + await POST(postRequest({ mediaKitToken: KIT_TOKEN })) - expect(createSponsorCheckoutSession).toHaveBeenCalledWith('kit-token-abc-123') + expect(createSponsorCheckoutSession).toHaveBeenCalledWith(KIT_TOKEN) }) it('returns the checkout URL on success', async () => { vi.mocked(createSponsorCheckoutSession).mockResolvedValue({ url: 'https://checkout.stripe.com/pay/cs_1' }) - const res = await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + const res = await POST(postRequest({ mediaKitToken: KIT_TOKEN })) const json = await res.json() expect(res.status).toBe(200) @@ -117,7 +138,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { it('surfaces an action-level error (e.g. invalid/unknown token) as a 400, not a 500', async () => { vi.mocked(createSponsorCheckoutSession).mockResolvedValue({ error: 'Invalid media kit link.' }) - const res = await POST(postRequest({ mediaKitToken: 'nonexistent-token' })) + const res = await POST(postRequest({ mediaKitToken: MISSING_TOKEN })) const json = await res.json() expect(res.status).toBe(400) @@ -127,7 +148,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { it('returns a generic 500 (no raw error detail) when the action throws unexpectedly', async () => { vi.mocked(createSponsorCheckoutSession).mockRejectedValue(new Error('stripe network timeout')) - const res = await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + const res = await POST(postRequest({ mediaKitToken: KIT_TOKEN })) const json = await res.json() expect(res.status).toBe(500) @@ -137,7 +158,7 @@ describe('POST /api/guidebook/sponsor-checkout', () => { it('returns 400 when the sponsorship slot is already active (a real auth-model rejection for this token route)', async () => { vi.mocked(createSponsorCheckoutSession).mockResolvedValue({ error: 'This sponsorship slot is already active.' }) - const res = await POST(postRequest({ mediaKitToken: 'kit-token-abc-123' })) + const res = await POST(postRequest({ mediaKitToken: KIT_TOKEN })) expect(res.status).toBe(400) }) diff --git a/unit/sms/optin-claim.test.ts b/unit/sms/optin-claim.test.ts new file mode 100644 index 00000000..5175f721 --- /dev/null +++ b/unit/sms/optin-claim.test.ts @@ -0,0 +1,158 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +vi.mock('@/lib/observability/report-error', () => ({ + reportError: vi.fn(), +})) + +import { + claimDailySmsSlot, + releaseDailySmsSlot, + sendClaimedDailySms, +} from '@/lib/sms/optin-claim' +import type { SupabaseClient } from '@supabase/supabase-js' + +/** + * Minimal `.from(table)` chain. Every terminal (`maybeSingle`, or awaiting the + * builder directly) consumes the next queued result in call order, so the claim + * UPDATE and the release UPDATE can be scripted independently. + */ +function makeSupabase(queued: { data?: unknown; error?: unknown }[]) { + let idx = 0 + const calls: { method: string; args: unknown[] }[] = [] + + const from = vi.fn(() => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const chain: any = {} + const record = (method: string, args: unknown[]) => { + calls.push({ method, args }) + return chain + } + chain.update = (...a: unknown[]) => record('update', a) + chain.eq = (...a: unknown[]) => record('eq', a) + chain.or = (...a: unknown[]) => record('or', a) + chain.select = (...a: unknown[]) => record('select', a) + + const resolveNext = () => Promise.resolve(queued[idx++] ?? { data: null, error: null }) + chain.maybeSingle = () => resolveNext() + chain.then = (resolve: (v: unknown) => unknown, reject?: (e: unknown) => unknown) => + resolveNext().then(resolve, reject) + return chain + }) + + return { client: { from } as unknown as SupabaseClient, calls } +} + +const CLAIM_WON = { data: { id: 'optin_1' }, error: null } +const CLAIM_LOST = { data: null, error: null } +const NO_ROWS = { data: null, error: null } + +describe('claimDailySmsSlot', () => { + beforeEach(() => vi.clearAllMocks()) + + it('returns true when this call won the atomic claim', async () => { + const { client } = makeSupabase([CLAIM_WON]) + await expect( + claimDailySmsSlot(client, 'optin_1', 'last_morning_sms_date', '2026-08-07') + ).resolves.toBe(true) + }) + + it('returns false — not an error — when a prior run already claimed today', async () => { + const { client } = makeSupabase([CLAIM_LOST]) + await expect( + claimDailySmsSlot(client, 'optin_1', 'last_morning_sms_date', '2026-08-07') + ).resolves.toBe(false) + }) + + it('throws when the claim query itself fails, instead of reading it as already-sent', async () => { + // A failed claim returns null data too. Reporting that as false told every + // caller "already sent today" and suppressed the guest's SMS silently. + const { client } = makeSupabase([{ data: null, error: { message: 'deadlock detected', code: '40P01' } }]) + await expect( + claimDailySmsSlot(client, 'optin_1', 'last_morning_sms_date', '2026-08-07') + ).rejects.toThrow(/Supabase query failed/) + }) +}) + +describe('sendClaimedDailySms', () => { + beforeEach(() => vi.clearAllMocks()) + + it('sends and keeps the claim on success', async () => { + const { client, calls } = makeSupabase([CLAIM_WON]) + const send = vi.fn(async () => ({ sent: true })) + + await expect( + sendClaimedDailySms(client, 'optin_1', 'last_morning_sms_date', '2026-08-07', send) + ).resolves.toBe(true) + + expect(send).toHaveBeenCalled() + // Exactly one UPDATE: the claim. No release. + expect(calls.filter((c) => c.method === 'update')).toHaveLength(1) + }) + + it('never calls send when the claim was already taken', async () => { + const { client } = makeSupabase([CLAIM_LOST]) + const send = vi.fn(async () => ({ sent: true })) + + await expect( + sendClaimedDailySms(client, 'optin_1', 'last_evening_sms_date', '2026-08-07', send) + ).resolves.toBe(false) + expect(send).not.toHaveBeenCalled() + }) + + // The two failure exits below are NOT the same event, and only one of them + // was handled at the three call sites this helper replaced. sendSMS returns + // {sent:false} ONLY for a deliberate skip (SMS_ENABLED off, nudge budget, + // demo suppression); every real failure THROWS out of dispatchToTelnyx. + it('releases the slot and rethrows when the send throws, so the day is not burned', async () => { + const { client, calls } = makeSupabase([CLAIM_WON, NO_ROWS]) + const send = vi.fn(async () => { throw new Error('Telnyx 502') }) + + await expect( + sendClaimedDailySms(client, 'optin_1', 'last_morning_sms_date', '2026-08-07', send) + ).rejects.toThrow('Telnyx 502') + + // Without the release the Inngest retry re-reads the date column, finds + // today's date, skips — and that guest's nudge is gone for the day. + const updates = calls.filter((c) => c.method === 'update') + expect(updates).toHaveLength(2) + expect(updates[1].args[0]).toEqual({ last_morning_sms_date: null }) + }) + + it('releases the slot and returns false — without throwing — on a deliberate skip', async () => { + const { client, calls } = makeSupabase([CLAIM_WON, NO_ROWS]) + const send = vi.fn(async () => ({ sent: false })) + + await expect( + sendClaimedDailySms(client, 'optin_1', 'last_evening_sms_date', '2026-08-07', send) + ).resolves.toBe(false) + + const updates = calls.filter((c) => c.method === 'update') + expect(updates).toHaveLength(2) + expect(updates[1].args[0]).toEqual({ last_evening_sms_date: null }) + }) + + it('covers rendering too — a template failure inside send releases the slot', async () => { + // Rendering used to sit BETWEEN the claim and the try-less send, so a + // renderSmsBody throw burned the day's slot with no release at all. + const { client, calls } = makeSupabase([CLAIM_WON, NO_ROWS]) + const send = vi.fn(async () => { throw new Error('unknown template key') }) + + await expect( + sendClaimedDailySms(client, 'optin_1', 'last_morning_sms_date', '2026-08-07', send) + ).rejects.toThrow('unknown template key') + expect(calls.filter((c) => c.method === 'update')).toHaveLength(2) + }) +}) + +describe('releaseDailySmsSlot', () => { + beforeEach(() => vi.clearAllMocks()) + + it('swallows a failed release rather than masking the original error', async () => { + // Called from a catch block on the throw path — rethrowing here would + // replace the real send failure with a rollback failure. + const { client } = makeSupabase([{ data: null, error: { message: 'timeout', code: '57014' } }]) + await expect( + releaseDailySmsSlot(client, 'optin_1', 'last_morning_sms_date') + ).resolves.toBeUndefined() + }) +})