diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f1ce7cc..0bcb083a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,24 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad the link-only fallback that needs no Zoom app. Answers [#35](https://github.com/Calnode/calnode/issues/35). +- **Booking emails name the host a booking was assigned to, not the owner of its event + type.** On a round-robin or multi-host event type those are different people, and the + owner may not attend at all, but the host was looked up through `event_types.user_id`. + Fixes [#48](https://github.com/Calnode/calnode/issues/48), reported with the fix by + [@MinosChatzidakis](https://github.com/MinosChatzidakis). + + - The **reminder** told the attendee they were meeting the owner, and was sent or + skipped by the owner's `notify_reminder` preference rather than the host's. + - The host's **reschedule** notice went to the owner, so the host who was actually + attending was never told the meeting had moved. The attendee's reschedule email and + its `.ics` organizer named the owner too. This applies to every reschedule path: + admin, manage link and MCP. + - The **cancellation** and **reassign** emails loaded the owner first and replaced + them before sending, so neither was visibly wrong. Both now start from the booking's + host, and reassign no longer needs its own second lookup. + + An event type whose owner is also its only host is unaffected. + ## [0.9.0] - 2026-09-10 ### Added diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 58defaab..070cee1f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -641,6 +641,12 @@ as the desired state: itself is `multipart/alternative` (text+HTML) or single-part text. - Multi-host fan-out: each assigned host gets their host-notification; the attendee gets one. (See §9.) +- **The host in a booking email is the booking's, never the event type's owner.** + Name, address and notification prefs come from `bookings.host_id` (the primary host) + or `booking_hosts`, not `event_types.user_id`: on round-robin and multi-host event + types the owner is often not attending. The reminder worker joins on + `bookings.host_id`; `loadCancellationData` (cancel, reschedule, reassign) takes the + host from the `HostID` of the booking it is given. - **Per-event-type customisation:** custom note bodies (`msg_*`) and custom subject lines (`subj_*`, migration 00026) for the four attendee emails; a blank subject falls back to the built-in default (`BookingData.SubjectOverride` / `subjectOr`). diff --git a/internal/handler/booking_handler.go b/internal/handler/booking_handler.go index 6b7abe35..675a4ea3 100644 --- a/internal/handler/booking_handler.go +++ b/internal/handler/booking_handler.go @@ -1786,14 +1786,11 @@ func primaryHost(hosts []assignedHost) assignedHost { return hosts[0] } -// loadHostIntoData fills HostName and HostEmail in d from the users table. -func (h *Handler) loadHostIntoData(ctx context.Context, hostID string, d *mailer.BookingData) error { - return h.db.QueryRowContext(ctx, - `SELECT name, email FROM users WHERE id = ?`, hostID). - Scan(&d.HostName, &d.HostEmail) -} - -// loadCancellationData assembles all fields needed for cancellation emails. +// loadCancellationData assembles the fields the cancellation, reschedule and +// reassign emails share. HostName and HostEmail are those of b.HostID, the +// booking's primary host, so the caller decides which host they describe by the +// booking copy it passes. Paths that notify several hosts still override them +// per host (hostBookingData). func (h *Handler) loadCancellationData(ctx context.Context, b *booking.Booking) (mailer.BookingData, error) { var d mailer.BookingData d.BookingID = b.ID @@ -1802,11 +1799,13 @@ func (h *Handler) loadCancellationData(ctx context.Context, b *booking.Booking) d.LocationValue = b.LocationValue d.CancellationReason = b.CancellationReason - // Event type name + slug and host name + email in one join. + // Event type name + slug, and the booking's host. Not event_types.user_id: the + // owner of a round-robin or multi-host event type is often not the person the + // booking was assigned to, and may not be attending at all. err := h.db.QueryRowContext(ctx, ` SELECT et.name, et.slug, u.name, u.email - FROM event_types et JOIN users u ON u.id = et.user_id - WHERE et.id = ?`, b.EventTypeID). + FROM event_types et JOIN users u ON u.id = ? + WHERE et.id = ?`, b.HostID, b.EventTypeID). Scan(&d.EventTypeName, &d.EventTypeSlug, &d.HostName, &d.HostEmail) if err != nil { return d, fmt.Errorf("load event/host: %w", err) diff --git a/internal/handler/booking_host_mail_internal_test.go b/internal/handler/booking_host_mail_internal_test.go new file mode 100644 index 00000000..9168e17c --- /dev/null +++ b/internal/handler/booking_host_mail_internal_test.go @@ -0,0 +1,237 @@ +package handler + +import ( + "context" + "encoding/json" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/calnode/calnode/internal/booking" + "github.com/calnode/calnode/internal/db" + "github.com/calnode/calnode/internal/mailer" +) + +// Booking emails must name and notify the host a booking was assigned to +// (bookings.host_id), not the owner of its event type (event_types.user_id). On a +// round-robin or hosts-tab event type those are different people, and the owner +// may not be attending at all. Issue #48 reported the reminder; the same owner +// join fed the cancellation, reschedule and reassign emails through +// loadCancellationData. +// +// Every test here uses one shape: the event type is owned by the admin who ran +// setup (Test Host), and the booking is assigned to a second member (Hana Host). + +const ( + etOwnerEmail = "host@example.com" // the setup admin, who owns the event type + etOwnerName = "Test Host" + bookingHostEmail = "hana@example.com" + bookingHostName = "Hana Host" + bookerEmail = "dana@example.com" +) + +type hostMailFixture struct { + h *Handler + cap *captureMailer + apiKey string + b *booking.Booking +} + +func newHostMailFixture(t *testing.T) hostMailFixture { + t.Helper() + database, err := db.Open("sqlite://:memory:") + if err != nil { + t.Fatalf("db.Open: %v", err) + } + t.Cleanup(func() { database.Close() }) + if err := db.Migrate(database); err != nil { + t.Fatalf("db.Migrate: %v", err) + } + + h := New(database, slog.Default()) + cap := &captureMailer{} + h.SetMailer(cap, "https://calnode.example.com") + + rec := httptest.NewRecorder() + h.Setup(rec, httptest.NewRequest(http.MethodPost, "/v1/setup", + strings.NewReader(`{"name":"`+etOwnerName+`","email":"`+etOwnerEmail+`","timezone":"UTC"}`))) + if rec.Code != http.StatusCreated { + t.Fatalf("setup: %d — %s", rec.Code, rec.Body.String()) + } + var setup struct { + APIKey string `json:"api_key"` + UserID string `json:"user_id"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &setup); err != nil { + t.Fatalf("decode setup: %v", err) + } + + start := time.Now().UTC().Add(72 * time.Hour).Truncate(time.Hour) + end := start.Add(30 * time.Minute) + for _, q := range []struct { + sql string + args []any + }{ + {`INSERT INTO users (id, email, name, iana_timezone, is_admin) VALUES ('u-hana', ?, ?, 'UTC', 0)`, + []any{bookingHostEmail, bookingHostName}}, + {`INSERT INTO users (id, email, name, iana_timezone, is_admin) VALUES ('u-nico', 'nico@example.com', 'Nico New', 'UTC', 0)`, nil}, + {`INSERT INTO event_types (id, user_id, slug, name, duration_minutes) VALUES ('et-team', ?, 'team-intro', 'Team Intro', 30)`, + []any{setup.UserID}}, + {`INSERT INTO bookings (id, event_type_id, host_id, start_at, end_at, status) VALUES ('b-1', 'et-team', 'u-hana', ?, ?, 'confirmed')`, + []any{start.Format(time.RFC3339Nano), end.Format(time.RFC3339Nano)}}, + {`INSERT INTO booking_hosts (id, booking_id, user_id, is_primary) VALUES ('bh-1', 'b-1', 'u-hana', 1)`, nil}, + {`INSERT INTO booking_attendees (id, booking_id, name, email, iana_timezone, is_organizer) VALUES ('a-1', 'b-1', 'Dana', ?, 'UTC', 1)`, + []any{bookerEmail}}, + } { + if _, err := database.Exec(q.sql, q.args...); err != nil { + t.Fatalf("seed: %v\n%s", err, q.sql) + } + } + + b, err := h.bookingSvc.Get(context.Background(), "b-1") + if err != nil { + t.Fatalf("load booking: %v", err) + } + return hostMailFixture{h: h, cap: cap, apiKey: setup.APIKey, b: b} +} + +// assertNamesOnly fails unless every body of msg names want and none names the +// event type's owner. +func assertNamesOnly(t *testing.T, label string, msg *mailer.Message, want string) { + t.Helper() + for part, body := range map[string]string{"text": msg.Text, "html": msg.HTML} { + if !strings.Contains(body, want) { + t.Errorf("%s (%s) does not name %q:\n%s", label, part, want, body) + } + if strings.Contains(body, etOwnerName) { + t.Errorf("%s (%s) names the event type's owner %q, who is not this booking's host:\n%s", + label, part, etOwnerName, body) + } + } +} + +func TestLoadCancellationData_namesTheBookingsHostNotTheEventTypeOwner(t *testing.T) { + f := newHostMailFixture(t) + + d, err := f.h.loadCancellationData(context.Background(), f.b) + if err != nil { + t.Fatalf("loadCancellationData: %v", err) + } + if d.HostName != bookingHostName || d.HostEmail != bookingHostEmail { + t.Errorf("host = %q <%s>; want the booking's host %q <%s>, not the event type's owner", + d.HostName, d.HostEmail, bookingHostName, bookingHostEmail) + } + if d.EventTypeName != "Team Intro" || d.EventTypeSlug != "team-intro" { + t.Errorf("event type = %q/%q; want Team Intro/team-intro", d.EventTypeName, d.EventTypeSlug) + } +} + +// The reschedule notice went to the event type's owner, and the host the booking +// was assigned to heard nothing, while the preference consulted was the assigned +// host's. This is the path where the wrong join changed who received mail. +func TestRescheduleSideEffects_notifiesTheBookingsHostNotTheEventTypeOwner(t *testing.T) { + f := newHostMailFixture(t) + + f.h.rescheduleSideEffects(*f.b, f.b.EventTypeID, f.b.StartAt.Add(-24*time.Hour), f.b.EndAt.Add(-24*time.Hour)) + + if msg := f.cap.find(etOwnerEmail); msg != nil { + t.Errorf("the event type's owner was sent %q; they are not this booking's host", msg.Subject) + } + host := f.cap.find(bookingHostEmail) + if host == nil { + t.Fatalf("the booking's host was not notified of the reschedule; sent to: %v", f.cap.recipients()) + } + assertNamesOnly(t, "host reschedule notice", host, bookingHostName) + + attendee := f.cap.find(bookerEmail) + if attendee == nil { + t.Fatalf("no reschedule email to the attendee; sent to: %v", f.cap.recipients()) + } + assertNamesOnly(t, "attendee reschedule email", attendee, bookingHostName) + // The invite's ORGANIZER is the host too, or the attendee's calendar shows the + // owner as the person running the meeting. + if len(attendee.Attachments) == 0 { + t.Fatal("attendee reschedule email carries no .ics (no calendar is connected in this test, so it should)") + } + if ics := string(attendee.Attachments[0].Content); !strings.Contains(ics, "mailto:"+bookingHostEmail) || + strings.Contains(ics, etOwnerEmail) { + t.Errorf("attendee .ics ORGANIZER is not the booking's host:\n%s", ics) + } +} + +// Cancellation already fanned out over booking_hosts and overwrote the owner's +// details per host, so this held before the fix. It is pinned here so that the +// two paths sharing loadCancellationData stay in step. +func TestCancelSideEffects_notifiesTheBookingsHostNotTheEventTypeOwner(t *testing.T) { + f := newHostMailFixture(t) + ctx := context.Background() + if err := f.h.bookingSvc.CancelByID(ctx, f.b.ID, "conflict"); err != nil { + t.Fatalf("cancel: %v", err) + } + b, err := f.h.bookingSvc.Get(ctx, f.b.ID) + if err != nil { + t.Fatalf("reload: %v", err) + } + + f.h.cancelSideEffects(*b) + + if msg := f.cap.find(etOwnerEmail); msg != nil { + t.Errorf("the event type's owner was sent %q; they are not this booking's host", msg.Subject) + } + host := f.cap.find(bookingHostEmail) + if host == nil { + t.Fatalf("the booking's host was not notified of the cancellation; sent to: %v", f.cap.recipients()) + } + assertNamesOnly(t, "host cancellation notice", host, bookingHostName) + attendee := f.cap.find(bookerEmail) + if attendee == nil { + t.Fatalf("no cancellation email to the attendee; sent to: %v", f.cap.recipients()) + } + assertNamesOnly(t, "attendee cancellation email", attendee, bookingHostName) +} + +// Reassign emails the attendee and the NEW host. They must name the new host: not +// the old one, and not the event type's owner. ReassignBooking sends them from its +// own goroutine, so this polls by recipient. +func TestReassignBooking_emailsNameTheNewHost(t *testing.T) { + f := newHostMailFixture(t) + + req := httptest.NewRequest(http.MethodPost, "/v1/bookings/b-1/reassign", strings.NewReader(`{"host_id":"u-nico"}`)) + req.Header.Set("X-API-Key", f.apiKey) + req.SetPathValue("id", "b-1") + rec := httptest.NewRecorder() + f.h.RequireAuth(f.h.ReassignBooking)(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("reassign: %d — %s", rec.Code, rec.Body.String()) + } + + // The attendee is emailed before the new host, so once the host's email has + // arrived both sends have happened. + var newHost *mailer.Message + for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); time.Sleep(5 * time.Millisecond) { + if newHost = f.cap.find("nico@example.com"); newHost != nil { + break + } + } + if newHost == nil { + t.Fatalf("the new host was not notified; sent to: %v", f.cap.recipients()) + } + assertNamesOnly(t, "new host notice", newHost, "Nico New") + + attendee := f.cap.find(bookerEmail) + if attendee == nil { + t.Fatalf("no email to the attendee; sent to: %v", f.cap.recipients()) + } + assertNamesOnly(t, "attendee reassign email", attendee, "Nico New") + if strings.Contains(attendee.Text, bookingHostName) { + t.Errorf("attendee reassign email still names the previous host %q:\n%s", bookingHostName, attendee.Text) + } + for _, to := range []string{etOwnerEmail, bookingHostEmail} { + if msg := f.cap.find(to); msg != nil { + t.Errorf("%s was sent %q; only the attendee and the new host should be emailed", to, msg.Subject) + } + } +} diff --git a/internal/handler/reassign.go b/internal/handler/reassign.go index e3d58c1e..75da61a8 100644 --- a/internal/handler/reassign.go +++ b/internal/handler/reassign.go @@ -193,7 +193,9 @@ func (h *Handler) ReassignBooking(w http.ResponseWriter, r *http.Request) { } // Notify the attendee (their host changed) and the new host, reusing the - // confirmation templates. Host details are overridden to the new host. + // confirmation templates. The emails name the NEW host: ReassignHost returns + // the booking with HostID already set to newHostID, and loadCancellationData + // reads the host from the booking copy it is given. d, err := h.loadCancellationData(ctx, &bCopy) if err != nil { h.logger.Error("reassign: load email data", "error", err, "booking_id", bCopy.ID) @@ -201,9 +203,6 @@ func (h *Handler) ReassignBooking(w http.ResponseWriter, r *http.Request) { } d.BaseURL = h.publicURL() h.applyBranding(ctx, &d) - if err := h.loadHostIntoData(ctx, newHostID, &d); err != nil { - h.logger.Error("reassign: load new host", "error", err, "booking_id", bCopy.ID) - } prefs := h.hostPrefsOrDefault(ctx, bCopy.ID, newHostID) if prefs.NotifyConfirmation { diff --git a/internal/handler/reschedule_handler.go b/internal/handler/reschedule_handler.go index c0c147f6..73810128 100644 --- a/internal/handler/reschedule_handler.go +++ b/internal/handler/reschedule_handler.go @@ -117,7 +117,8 @@ func (h *Handler) RescheduleBooking(w http.ResponseWriter, r *http.Request) { // Side effects (calendar move, Zoom update, emails, webhook, reminders) are shared // with the manage-token reschedule flow — see rescheduleSideEffects in - // manage_handler.go. etSlug is unused here now; it's re-derived inside the helper - // via loadCancellationData's own join, which is equivalent. + // manage_handler.go. etSlug is unused here now; the helper re-reads the slug from + // the booking's event type, and takes the host it names and notifies from the + // booking itself (updated.HostID), not from the event type's owner. go h.rescheduleSideEffects(*updated, etID, previousStart, previousEnd) // #nosec G118 -- deliberately its own context.Background(); see rescheduleSideEffects' doc comment } diff --git a/internal/worker/reminder_test.go b/internal/worker/reminder_test.go index 4b922e6c..fef84d06 100644 --- a/internal/worker/reminder_test.go +++ b/internal/worker/reminder_test.go @@ -2,6 +2,7 @@ package worker_test import ( "context" + "database/sql" "log/slog" "net/http" "strings" @@ -117,6 +118,100 @@ func TestWorker_reminderRespectsAttendeeLocale(t *testing.T) { } } +// --------------------------------------------------------------------------- +// reminder.send: names the booking's host, not the event type's owner (#48) +// --------------------------------------------------------------------------- + +// seedAssignedReminder creates an event type owned by host-01 whose booking was +// assigned to host-02, the shape a round-robin or hosts-tab event type produces, +// plus a due reminder job for it. +func seedAssignedReminder(t *testing.T, database *sql.DB, suffix string) { + t.Helper() + ctx := context.Background() + pastRunAt := time.Now().UTC().Add(-time.Second).Format(time.RFC3339) + bookingStart := time.Now().UTC().Add(25 * time.Hour).Format(time.RFC3339) + for _, q := range []struct { + sql string + args []any + }{ + {`INSERT INTO event_types (id, user_id, slug, name, duration_minutes) + VALUES (?, 'host-01', ?, 'Team Intro', 30)`, []any{"et-" + suffix, "team-intro-" + suffix}}, + {`INSERT INTO bookings (id, event_type_id, host_id, start_at, end_at, status) + VALUES (?, ?, 'host-02', ?, ?, 'confirmed')`, []any{"bk-" + suffix, "et-" + suffix, bookingStart, bookingStart}}, + {`INSERT INTO booking_attendees (id, booking_id, name, email, iana_timezone, is_organizer) + VALUES (?, ?, 'Dana', 'dana@example.com', 'UTC', 1)`, []any{"att-" + suffix, "bk-" + suffix}}, + {`INSERT INTO jobs (id, type, payload, run_at, status, attempts, max_attempts) + VALUES (?, 'reminder.send', ?, ?, 'pending', 0, 3)`, []any{"job-" + suffix, `{"booking_id":"bk-` + suffix + `"}`, pastRunAt}}, + } { + if _, err := database.ExecContext(ctx, q.sql, q.args...); err != nil { + t.Fatalf("seed: %v", err) + } + } +} + +func TestWorker_reminderNamesTheBookingHostNotTheEventTypeOwner(t *testing.T) { + database, svc := setup(t) + ctx := context.Background() + seedAssignedReminder(t, database, "h1") + + m := &captureMailer{} + w := worker.New(database, svc, slog.Default(), + worker.WithMailer(m), + worker.WithHTTPClient(&http.Client{})) + w.Poll(ctx) + + if len(m.sent) != 1 { + t.Fatalf("sent %d emails; want 1", len(m.sent)) + } + msg := m.sent[0] + for name, body := range map[string]string{"Text": msg.Text, "HTML": msg.HTML} { + if !strings.Contains(body, "Host Two") { + t.Errorf("%s does not name the assigned host (Host Two):\n%s", name, body) + } + if strings.Contains(body, "Host One") { + t.Errorf("%s names the event type's owner (Host One), who is not the host of this booking:\n%s", name, body) + } + } +} + +// The notify_reminder preference that decides whether the reminder goes out is +// the assigned host's, since it is their meeting the email is about. +func TestWorker_reminderFollowsTheBookingHostsPreference(t *testing.T) { + cases := []struct { + name string + ownerPref, hostPref int + wantSent int + }{ + {"owner opted out, host did not", 0, 1, 1}, + {"host opted out, owner did not", 1, 0, 0}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + database, svc := setup(t) + ctx := context.Background() + seedAssignedReminder(t, database, "p1") + if _, err := database.ExecContext(ctx, + `UPDATE users SET notify_reminder = ? WHERE id = 'host-01'`, tc.ownerPref); err != nil { + t.Fatal(err) + } + if _, err := database.ExecContext(ctx, + `UPDATE users SET notify_reminder = ? WHERE id = 'host-02'`, tc.hostPref); err != nil { + t.Fatal(err) + } + + m := &captureMailer{} + w := worker.New(database, svc, slog.Default(), + worker.WithMailer(m), + worker.WithHTTPClient(&http.Client{})) + w.Poll(ctx) + + if len(m.sent) != tc.wantSent { + t.Errorf("sent %d emails; want %d", len(m.sent), tc.wantSent) + } + }) + } +} + // --------------------------------------------------------------------------- // reminder.send: cancelled booking → silent skip, job done // --------------------------------------------------------------------------- diff --git a/internal/worker/worker.go b/internal/worker/worker.go index b44ed1bf..9de7941d 100644 --- a/internal/worker/worker.go +++ b/internal/worker/worker.go @@ -263,8 +263,10 @@ func (w *Worker) sendReminder(ctx context.Context, payload string) error { return fmt.Errorf("worker: reminder: parse payload: %w", err) } - // One query: join bookings → event_types → users (host). - // Also load notify_reminder pref and msg_reminder custom note. + // One query: join bookings → event_types, and users on the booking's own host. + // The host is bookings.host_id (the primary host), not event_types.user_id: on a + // round-robin or multi-host event type the owner may not be attending at all. + // Also load that host's notify_reminder pref and the msg_reminder custom note. // Skip if booking is deleted or no longer confirmed. var d mailer.BookingData d.BookingID = p.BookingID @@ -277,7 +279,7 @@ func (w *Worker) sendReminder(ctx context.Context, payload string) error { u.name, u.email, COALESCE(u.notify_reminder, 1) FROM bookings b JOIN event_types et ON et.id = b.event_type_id - JOIN users u ON u.id = et.user_id + JOIN users u ON u.id = b.host_id WHERE b.id = ?`, p.BookingID). Scan(&status, &startAt, &endAt, &locVal, &d.EventTypeName, &d.EventTypeSlug, &msgReminder, &subjReminder,