From cdad7875e1bdd1c640ba9192c22ef7cfca58a1d1 Mon Sep 17 00:00:00 2001 From: Tomas Dvorak Date: Fri, 18 Sep 2026 12:47:07 +0200 Subject: [PATCH] fix(reminders): lifecycle-aware jobs and dispatch skip guard The reminder pipeline fired regardless of what happened to the booking after creation: cancelled customers got reminded about bookings that weren't happening, reschedules kept the old reminder time, and waitlisted bookings armed jobs too. Pending jobs are now cancelled on both cancel paths, deleted and re-armed on reschedule, and dispatch joins bookings.status to skip anything not confirmed. Reschedule also gained the same availability-window and blocked-day validation as booking creation, closing a 3am-reschedule hole via the manage URL. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../internal/bookings/customer_service.go | 26 ++++ apps/backend/internal/bookings/service.go | 12 +- .../backend/internal/bookings/service_test.go | 122 ++++++++++++++++++ apps/backend/internal/catalog/service.go | 1 + apps/backend/internal/db/bookings.go | 20 ++- apps/backend/internal/db/repository.go | 30 +++++ apps/backend/internal/domain/models.go | 1 + .../backend/internal/notifications/service.go | 8 ++ .../internal/notifications/service_test.go | 50 ++++++- apps/backend/openapi/bookra.openapi.yaml | 4 +- packages/api-client/src/generated/types.ts | 1 + project.md | 6 + 12 files changed, 272 insertions(+), 9 deletions(-) diff --git a/apps/backend/internal/bookings/customer_service.go b/apps/backend/internal/bookings/customer_service.go index b587709..506b219 100644 --- a/apps/backend/internal/bookings/customer_service.go +++ b/apps/backend/internal/bookings/customer_service.go @@ -168,10 +168,35 @@ func (s *CustomerService) RescheduleBooking(ctx context.Context, reference strin } } + // Appointments must land inside bookable hours and off blocked days; + // class bookings keep their session-defined window. + if booking.ClassSessionID == nil { + tenant, err := s.repo.GetTenantByID(ctx, booking.TenantID) + if err != nil { + return err + } + if err := validateAppointmentTarget(ctx, s.repo, tenant, booking.StaffID, booking.LocationID, newStartsAt, newEndsAt); err != nil { + return ErrInvalidReschedule + } + } + if err := s.repo.RescheduleBooking(ctx, booking.ID, newStartsAt, newEndsAt); err != nil { return err } + // Re-arm the reminder for the new time; a booking moved inside the + // no-reminder horizon simply gets none. + if err := s.repo.DeletePendingReminderJobsForBooking(ctx, booking.ID); err == nil { + if scheduledFor, ok := reminderSchedule(newStartsAt); ok { + _ = s.repo.CreateReminderJob(ctx, db.ReminderJobParams{ + TenantID: booking.TenantID, + BookingID: booking.ID, + Channel: "email", + ScheduledFor: scheduledFor, + }) + } + } + // Send reschedule email (async) go s.sendRescheduleEmail(booking, newStartsAt, newEndsAt) @@ -195,6 +220,7 @@ func (s *CustomerService) CancelBooking(ctx context.Context, reference string, t if err := s.repo.UpdateBookingStatus(ctx, booking.ID, "cancelled"); err != nil { return err } + _ = s.repo.CancelPendingReminderJobsForBooking(ctx, booking.ID) // Send cancellation email (async) go s.sendCancellationEmail(booking) diff --git a/apps/backend/internal/bookings/service.go b/apps/backend/internal/bookings/service.go index 11d0501..06b1f2d 100644 --- a/apps/backend/internal/bookings/service.go +++ b/apps/backend/internal/bookings/service.go @@ -290,7 +290,7 @@ func (s *Service) Create(ctx context.Context, request domain.CreateBookingReques if !startsAt.Add(expectedDuration).Equal(endsAt) { return domain.CreateBookingResponse{}, fmt.Errorf("%w: appointment duration must match service duration", ErrInvalidBooking) } - if err := s.validateAppointmentTarget(ctx, tenant, request.StaffID, request.LocationID, startsAt, endsAt); err != nil { + if err := validateAppointmentTarget(ctx, s.repo, tenant, request.StaffID, request.LocationID, startsAt, endsAt); err != nil { return domain.CreateBookingResponse{}, err } for _, booking := range existing { @@ -383,9 +383,9 @@ func (s *Service) Create(ctx context.Context, request domain.CreateBookingReques // resources owned by the tenant and lands inside bookable time: the staff and // location must belong to the tenant, the window must fit an availability rule // the booking could claim, and it must not overlap a blocked day. -func (s *Service) validateAppointmentTarget(ctx context.Context, tenant db.TenantRecord, staffID, locationID *string, startsAt, endsAt time.Time) error { +func validateAppointmentTarget(ctx context.Context, repo db.Repository, tenant db.TenantRecord, staffID, locationID *string, startsAt, endsAt time.Time) error { if staffID != nil { - staff, err := s.repo.GetStaffByID(ctx, *staffID) + staff, err := repo.GetStaffByID(ctx, *staffID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { return fmt.Errorf("%w: staffId is not available for tenant", ErrInvalidBooking) @@ -397,7 +397,7 @@ func (s *Service) validateAppointmentTarget(ctx context.Context, tenant db.Tenan } } if locationID != nil { - location, err := s.repo.GetLocationByID(ctx, *locationID) + location, err := repo.GetLocationByID(ctx, *locationID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { return fmt.Errorf("%w: locationId is not available for tenant", ErrInvalidBooking) @@ -409,11 +409,11 @@ func (s *Service) validateAppointmentTarget(ctx context.Context, tenant db.Tenan } } - rules, err := s.repo.ListAvailabilityRulesByTenant(ctx, tenant.ID) + rules, err := repo.ListAvailabilityRulesByTenant(ctx, tenant.ID) if err != nil { return err } - blockedDays, err := s.repo.ListBlockedDaysByTenant(ctx, tenant.ID, startsAt.UTC(), endsAt.UTC()) + blockedDays, err := repo.ListBlockedDaysByTenant(ctx, tenant.ID, startsAt.UTC(), endsAt.UTC()) if err != nil { return err } diff --git a/apps/backend/internal/bookings/service_test.go b/apps/backend/internal/bookings/service_test.go index d250946..40ff770 100644 --- a/apps/backend/internal/bookings/service_test.go +++ b/apps/backend/internal/bookings/service_test.go @@ -518,3 +518,125 @@ func TestDashboardSummaryComputesKPIsWithTrends(t *testing.T) { t.Fatalf("utilization kpi = %+v", utilization) } } + +func bookAppointmentSlot(t *testing.T, repo db.Repository, service *Service) domain.CreateBookingResponse { + t.Helper() + // Target a day ~72h out so the booking lands in the -24h reminder horizon. + target := time.Now().UTC().AddDate(0, 0, 3).Format("2006-01-02") + availability, err := service.Availability(context.Background(), "studio-atelier", AvailabilityFilter{Date: target}) + if err != nil { + t.Fatalf("availability: %v", err) + } + var slot domain.TimeSlot + for _, s := range availability.Slots { + if s.Mode == "appointment" { + slot = s + break + } + } + if slot.StartsAt == "" { + t.Fatal("expected appointment slot") + } + resp, err := service.Create(context.Background(), domain.CreateBookingRequest{ + TenantSlug: "studio-atelier", + BookingMode: "appointment", + ServiceID: slot.ServiceID, + StaffID: slot.StaffID, + LocationID: slot.LocationID, + CustomerName: "Reminder Test", + CustomerEmail: "reminder-test@example.com", + StartsAt: slot.StartsAt, + EndsAt: slot.EndsAt, + }) + if err != nil { + t.Fatalf("create booking: %v", err) + } + return resp +} + +func pendingJobsFor(t *testing.T, repo db.Repository, bookingID string) []db.ReminderJobRecord { + t.Helper() + jobs, err := repo.ListDueReminderJobs(context.Background(), time.Now().UTC().AddDate(1, 0, 0), 100) + if err != nil { + t.Fatalf("list due jobs: %v", err) + } + var out []db.ReminderJobRecord + for _, job := range jobs { + if job.BookingID == bookingID { + out = append(out, job) + } + } + return out +} + +func TestCancelBookingClearsPendingReminder(t *testing.T) { + repo := db.NewMemoryRepository() + service := NewService(repo, nil, "http://localhost:3000") + customerService := NewCustomerService(repo, nil, "http://localhost:3000") + ctx := context.Background() + + resp := bookAppointmentSlot(t, repo, service) + if len(pendingJobsFor(t, repo, resp.BookingID)) != 1 { + t.Fatal("expected one pending reminder job after booking") + } + + manageURL, _ := url.Parse(resp.ManageURL) + token := manageURL.Query().Get("token") + if err := customerService.CancelBooking(ctx, resp.Reference, token); err != nil { + t.Fatalf("cancel: %v", err) + } + if got := pendingJobsFor(t, repo, resp.BookingID); len(got) != 0 { + t.Fatalf("expected no pending jobs after cancel, got %d", len(got)) + } +} + +func TestRescheduleRearmsReminder(t *testing.T) { + repo := db.NewMemoryRepository() + service := NewService(repo, nil, "http://localhost:3000") + customerService := NewCustomerService(repo, nil, "http://localhost:3000") + ctx := context.Background() + + resp := bookAppointmentSlot(t, repo, service) + booking, _ := repo.GetBookingByID(ctx, resp.BookingID) + + newStartsAt := booking.StartsAt.Add(48 * time.Hour) + newEndsAt := booking.EndsAt.Add(48 * time.Hour) + manageURL, _ := url.Parse(resp.ManageURL) + token := manageURL.Query().Get("token") + if err := customerService.RescheduleBooking(ctx, resp.Reference, domain.RescheduleBookingRequest{ + NewStartsAt: newStartsAt.UTC().Format(time.RFC3339), + NewEndsAt: newEndsAt.UTC().Format(time.RFC3339), + }, token); err != nil { + t.Fatalf("reschedule: %v", err) + } + + jobs := pendingJobsFor(t, repo, resp.BookingID) + if len(jobs) != 1 { + t.Fatalf("expected one pending job after reschedule, got %d", len(jobs)) + } + expected := newStartsAt.Add(-24 * time.Hour) + if !jobs[0].ScheduledFor.Equal(expected) { + t.Fatalf("expected reminder at %v, got %v", expected, jobs[0].ScheduledFor) + } +} + +func TestRescheduleRejectsTimeOutsideAvailabilityWindow(t *testing.T) { + repo := db.NewMemoryRepository() + service := NewService(repo, nil, "http://localhost:3000") + customerService := NewCustomerService(repo, nil, "http://localhost:3000") + ctx := context.Background() + + resp := bookAppointmentSlot(t, repo, service) + manageURL, _ := url.Parse(resp.ManageURL) + token := manageURL.Query().Get("token") + + // 03:00 UTC next week is outside every availability rule. + night := time.Now().UTC().AddDate(0, 0, 7).Truncate(24 * time.Hour).Add(3 * time.Hour) + err := customerService.RescheduleBooking(ctx, resp.Reference, domain.RescheduleBookingRequest{ + NewStartsAt: night.UTC().Format(time.RFC3339), + NewEndsAt: night.Add(time.Hour).UTC().Format(time.RFC3339), + }, token) + if !errors.Is(err, ErrInvalidReschedule) { + t.Fatalf("expected ErrInvalidReschedule, got %v", err) + } +} diff --git a/apps/backend/internal/catalog/service.go b/apps/backend/internal/catalog/service.go index eccdef8..25684f4 100644 --- a/apps/backend/internal/catalog/service.go +++ b/apps/backend/internal/catalog/service.go @@ -1298,6 +1298,7 @@ func (s *Service) CancelBooking(ctx context.Context, principal domain.Principal, if err := s.repo.UpdateBookingStatus(ctx, booking.ID, "cancelled"); err != nil { return err } + _ = s.repo.CancelPendingReminderJobsForBooking(ctx, booking.ID) go s.sendBookingEmail(booking, false) diff --git a/apps/backend/internal/db/bookings.go b/apps/backend/internal/db/bookings.go index d335755..30d137a 100644 --- a/apps/backend/internal/db/bookings.go +++ b/apps/backend/internal/db/bookings.go @@ -108,7 +108,7 @@ func (r *PGRepository) ListDueReminderJobs(ctx context.Context, dueBefore time.T rows, err := r.pool.Query(ctx, ` SELECT rj.id, rj.tenant_id, t.name, t.locale, t.timezone, rj.booking_id, rj.channel, rj.scheduled_for, - b.customer_name, b.customer_email, b.reference, b.starts_at, rj.status + b.customer_name, b.customer_email, b.reference, b.starts_at, rj.status, b.status FROM reminder_jobs rj INNER JOIN bookings b ON b.id = rj.booking_id INNER JOIN tenants t ON t.id = rj.tenant_id @@ -138,6 +138,7 @@ func (r *PGRepository) ListDueReminderJobs(ctx context.Context, dueBefore time.T &record.Reference, &record.StartsAt, &record.Status, + &record.BookingStatus, ); err != nil { return nil, err } @@ -191,3 +192,20 @@ func (r *PGRepository) GetDashboardMetrics(ctx context.Context, tenantID string, ) return metrics, err } + +func (r *PGRepository) CancelPendingReminderJobsForBooking(ctx context.Context, bookingID string) error { + _, err := r.pool.Exec(ctx, ` + UPDATE reminder_jobs + SET status = 'cancelled' + WHERE booking_id = $1 AND status = 'pending' + `, bookingID) + return err +} + +func (r *PGRepository) DeletePendingReminderJobsForBooking(ctx context.Context, bookingID string) error { + _, err := r.pool.Exec(ctx, ` + DELETE FROM reminder_jobs + WHERE booking_id = $1 AND status = 'pending' + `, bookingID) + return err +} diff --git a/apps/backend/internal/db/repository.go b/apps/backend/internal/db/repository.go index af82903..1d5e4c8 100644 --- a/apps/backend/internal/db/repository.go +++ b/apps/backend/internal/db/repository.go @@ -40,6 +40,8 @@ type Repository interface { CreateReminderJob(ctx context.Context, params ReminderJobParams) error ListDueReminderJobs(ctx context.Context, dueBefore time.Time, limit int) ([]ReminderJobRecord, error) MarkReminderJobDispatched(ctx context.Context, reminderJobID string, status string, dispatchedAt time.Time) error + CancelPendingReminderJobsForBooking(ctx context.Context, bookingID string) error + DeletePendingReminderJobsForBooking(ctx context.Context, bookingID string) error CreateNotificationDeliveryLog(ctx context.Context, params NotificationDeliveryLogParams) error GetDashboardMetrics(ctx context.Context, tenantID string, startsAt time.Time, endsAt time.Time) (DashboardMetrics, error) GetSubscriptionSnapshot(ctx context.Context, tenantID string) (BillingSnapshotRecord, error) @@ -418,6 +420,7 @@ type ReminderJobRecord struct { Reference string StartsAt time.Time Status string + BookingStatus string } type NotificationDeliveryLogParams struct { @@ -1413,6 +1416,12 @@ func (r *MemoryRepository) ListDueReminderJobs(_ context.Context, dueBefore time var out []ReminderJobRecord for _, job := range r.reminderJobs { if job.Status == "pending" && !job.ScheduledFor.After(dueBefore) { + for _, booking := range r.bookings { + if booking.ID == job.BookingID { + job.BookingStatus = booking.Status + break + } + } out = append(out, job) } } @@ -1433,6 +1442,27 @@ func (r *MemoryRepository) MarkReminderJobDispatched(_ context.Context, reminder return pgx.ErrNoRows } +func (r *MemoryRepository) CancelPendingReminderJobsForBooking(_ context.Context, bookingID string) error { + for index := range r.reminderJobs { + if r.reminderJobs[index].BookingID == bookingID && r.reminderJobs[index].Status == "pending" { + r.reminderJobs[index].Status = "cancelled" + } + } + return nil +} + +func (r *MemoryRepository) DeletePendingReminderJobsForBooking(_ context.Context, bookingID string) error { + kept := r.reminderJobs[:0] + for _, job := range r.reminderJobs { + if job.BookingID == bookingID && job.Status == "pending" { + continue + } + kept = append(kept, job) + } + r.reminderJobs = kept + return nil +} + func (r *MemoryRepository) CreateNotificationDeliveryLog(_ context.Context, params NotificationDeliveryLogParams) error { r.deliveryLogs = append(r.deliveryLogs, params) return nil diff --git a/apps/backend/internal/domain/models.go b/apps/backend/internal/domain/models.go index a5f08a1..7947332 100644 --- a/apps/backend/internal/domain/models.go +++ b/apps/backend/internal/domain/models.go @@ -251,6 +251,7 @@ type DispatchReminderJobsResponse struct { ProcessedCount int `json:"processedCount"` SentCount int `json:"sentCount"` FailedCount int `json:"failedCount"` + SkippedCount int `json:"skippedCount"` } // ============================================ diff --git a/apps/backend/internal/notifications/service.go b/apps/backend/internal/notifications/service.go index f033ce2..b8298cd 100644 --- a/apps/backend/internal/notifications/service.go +++ b/apps/backend/internal/notifications/service.go @@ -68,6 +68,14 @@ func (s *Service) DispatchDue(ctx context.Context, limit int) (domain.DispatchRe for _, job := range jobs { response.ProcessedCount++ + if job.BookingStatus != "confirmed" { + if err := s.repo.MarkReminderJobDispatched(ctx, job.ID, "skipped", s.now()); err != nil { + return domain.DispatchReminderJobsResponse{}, err + } + response.SkippedCount++ + continue + } + status := "sent" provider := "unknown" externalID := "" diff --git a/apps/backend/internal/notifications/service_test.go b/apps/backend/internal/notifications/service_test.go index 4178c13..4676713 100644 --- a/apps/backend/internal/notifications/service_test.go +++ b/apps/backend/internal/notifications/service_test.go @@ -65,9 +65,21 @@ func TestDispatchDueProcessesPendingEmailReminders(t *testing.T) { func TestDispatchDueFailsUnknownChannel(t *testing.T) { repo := db.NewMemoryRepository() + startsAt := time.Now().UTC().Add(26 * time.Hour) + created, err := repo.CreateBooking(context.Background(), db.CreateBookingParams{ + TenantID: "5d6b3551-0a3e-4b86-bdf0-e9df20a47148", + BookingMode: "appointment", + StartsAt: startsAt, + EndsAt: startsAt.Add(time.Hour), + Status: "confirmed", + Reference: "BK-TEST-PUSH", + }) + if err != nil { + t.Fatalf("create booking: %v", err) + } if err := repo.CreateReminderJob(context.Background(), db.ReminderJobParams{ TenantID: "5d6b3551-0a3e-4b86-bdf0-e9df20a47148", - BookingID: "booking-unknown-channel", + BookingID: created.ID, Channel: "push", ScheduledFor: time.Now().UTC().Add(-time.Minute), }); err != nil { @@ -90,3 +102,39 @@ func TestDispatchRequestContractShape(t *testing.T) { t.Fatalf("expected limit 20, got %d", request.Limit) } } + +func TestDispatchDueSkipsNonConfirmedBooking(t *testing.T) { + repo := db.NewMemoryRepository() + startsAt := time.Now().UTC().Add(26 * time.Hour) + created, err := repo.CreateBooking(context.Background(), db.CreateBookingParams{ + TenantID: "5d6b3551-0a3e-4b86-bdf0-e9df20a47148", + BookingMode: "appointment", + StartsAt: startsAt, + EndsAt: startsAt.Add(time.Hour), + Status: "confirmed", + Reference: "BK-TEST-SKIP", + }) + if err != nil { + t.Fatalf("create booking: %v", err) + } + if err := repo.CreateReminderJob(context.Background(), db.ReminderJobParams{ + TenantID: "5d6b3551-0a3e-4b86-bdf0-e9df20a47148", + BookingID: created.ID, + Channel: "email", + ScheduledFor: time.Now().UTC().Add(-time.Minute), + }); err != nil { + t.Fatalf("create reminder job: %v", err) + } + if err := repo.UpdateBookingStatus(context.Background(), created.ID, "cancelled"); err != nil { + t.Fatalf("cancel booking: %v", err) + } + + service := NewService(config.Config{Environment: "development"}, repo) + response, err := service.DispatchDue(context.Background(), 10) + if err != nil { + t.Fatalf("dispatch due: %v", err) + } + if response.ProcessedCount != 1 || response.SkippedCount != 1 || response.SentCount != 0 { + t.Fatalf("expected 1 skipped, got %+v", response) + } +} diff --git a/apps/backend/openapi/bookra.openapi.yaml b/apps/backend/openapi/bookra.openapi.yaml index a47e7be..2866f3c 100644 --- a/apps/backend/openapi/bookra.openapi.yaml +++ b/apps/backend/openapi/bookra.openapi.yaml @@ -2291,7 +2291,7 @@ components: maximum: 200 DispatchReminderJobsResponse: type: object - required: [processedCount, sentCount, failedCount] + required: [processedCount, sentCount, failedCount, skippedCount] properties: processedCount: type: integer @@ -2299,3 +2299,5 @@ components: type: integer failedCount: type: integer + skippedCount: + type: integer diff --git a/packages/api-client/src/generated/types.ts b/packages/api-client/src/generated/types.ts index 34275eb..b74e3ea 100644 --- a/packages/api-client/src/generated/types.ts +++ b/packages/api-client/src/generated/types.ts @@ -1128,6 +1128,7 @@ export interface components { failedCount: number; processedCount: number; sentCount: number; + skippedCount: number; }; EmailTemplate: { bodyText?: string; diff --git a/project.md b/project.md index 467faff..475fcd0 100644 --- a/project.md +++ b/project.md @@ -90,6 +90,12 @@ - the booking-trend chart was already bucketing real `allBookings` into the current week — confirmed, no change needed - demo mode intentionally keeps curated static KPI values; real tenants get the computed set - remaining: live staging validation; "new clients" KPI still has no backend metric (customer entity lacks first-booking tracking) +- `[done/partial]` Milestone 13: Reminder pipeline reliability + - reminder jobs are lifecycle-aware: customer and dashboard cancellation mark pending jobs `cancelled`; reschedule deletes pending jobs and re-arms one via `reminderSchedule` for the new time (no job when moved inside the no-reminder horizon) + - dispatch now joins `bookings.status` and skips non-confirmed bookings (`skipped` job status, `skippedCount` in the response contract) — cancelled/waitlisted customers no longer receive reminders for bookings that are not happening + - reschedule hardened with the same validation as booking creation: appointment reschedules must fit an availability rule and avoid blocked days (`validateAppointmentTarget` is now a shared package-level helper); class bookings keep session-defined times + - new repository methods `CancelPendingReminderJobsForBooking` / `DeletePendingReminderJobsForBooking` in pgx and in-memory implementations + - remaining: live staging validation; SMS reminder channel exists in schema (`channel` column) but only `email` is dispatched — SMS reminders are a separate opt-in feature ## Overview Bookra is a Czech-first booking SaaS for small local service businesses. It targets salons, massage therapists, small clinics, repair shops, and small studios/clubs that need a simple public booking flow, internal schedule management, reminders, and basic business reporting without becoming a full ERP.