amadan

oficina / calendar Public

booking: twenty-one findings from the code review, twenty-one fixed

GPT-6 reviewed the branch's code and returned DO NOT SHIP with twelve
blockers. Every one is fixed here, with a regression where one was
missing.

The worst was a leak: a line break in a booker's name reaches the mail
SUBJECT, rastrillo's SMTP sender refuses the folded header with an
error that quotes the whole subject, and scrubbing addresses out of
that is not enough — the title and the name go to the platform's logs.
Names and notes refuse control characters now, and no sender's error
TEXT is ever logged, only a word: busy, timeout, refused.

Two more engine defects: WKST was ignored and weeks counted from
Sunday, so a fortnightly Sunday-and-Monday rule from a Sunday dropped
the Monday under the RFC's Monday default; weeks now start where the
rule says, Monday by default. And the output cap reported an expansion
complete, so the strict reader took a truncated month as the whole of
it.

The reminder claim re-checks due_at, so a reschedule landing between
candidate and claim cannot have tomorrow's reminder sent today and then
retired; a transient read error is no longer "gone"; a retirement is
conditional on the claim. Hold and reschedule re-read the page, its
windows and the host behind the write lock, so an unpublished page or
a moved host zone cannot be booked against. A request's immediate mail
attempt claims only its own booking's rows, inside a budget. A hold
mail still owed when the booking is confirmed is retired unsent; a
rescheduled mail is retired once cancelled. The host's confirmation
carries the booker's note. The reschedule picker shows the booking's
snapshot. The drain's phases are context deadlines. Every public
computation, the hold and the reschedule included, sits under the
semaphore, and the caps are checked before any slot arithmetic. The
zone guess reloads through the form rather than relabelling the
select. Sign-in mail goes through the one bounded sender. Host
cancellation queues the revision the bump produces. Ack and defer
report their rows. The token collision retries once. The booked form
renders its disabled controls. Three tests that passed for the wrong
reason now test what they claim.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 1e2ba8315675cedac682391a714cb603de1f6eab parent 5e69e29
26 files changed, +550 −139
  • internal/calendar/app.go +5 −5
  • internal/calendar/booking_mail.go +42 −11
  • internal/calendar/booking_public.go +1 −0
  • internal/calendar/handlers_booking_manage.go +15 −3
  • internal/calendar/handlers_booking_public.go +51 −9
  • internal/calendar/handlers_jobs.go +11 −4
  • internal/calendar/logsafe.go +21 −1
  • internal/calendar/occurrences.go +4 −3
  • internal/calendar/occurrences_step.go +12 −6
  • internal/calendar/recur.go +16 −3
  • internal/calendar/static/booking.js +8 −0
  • internal/calendar/store_booking_cancel.go +7 −3
  • internal/calendar/store_booking_confirm.go +19 −3
  • internal/calendar/store_booking_flow.go +30 −46
  • internal/calendar/store_booking_hold_util.go +88 −0
  • internal/calendar/store_booking_mail.go +30 −16
  • internal/calendar/store_drain.go +14 −4
  • internal/calendar/store_event_write.go +10 −5
  • internal/calendar/templates/event_form.html +5 −2
  • internal/calendar/templates/mail/confirmed.txt +3 −1
  • internal/calendartest/booking_flow_test.go +9 −4
  • internal/calendartest/booking_host_test.go +5 −2
  • internal/calendartest/booking_mail_test.go +4 −4
  • internal/calendartest/booking_review_test.go +92 −0
  • internal/calendartest/occurrence_strict_test.go +20 −4
  • internal/calendartest/recur_engine_test.go +28 −0
diff --git a/internal/calendar/app.go b/internal/calendar/app.go
index a3998e9..1ac3056 100644
--- a/internal/calendar/app.go
+++ b/internal/calendar/app.go
@@ -109,6 +109,10 @@ func New(cfg Config) (*App, error) {
return nil, fmt.Errorf("calendar: writer handle: %w", err)
}
+ // ONE bounded sender for everything this app sends — sign-in,
+ // reminders, booking mail. The recorder stays inside it, so tests
+ // still read what was sent (booking spec §6).
+ mailer = newBoundedSender(mailer)
authPlugin, err := newAuthPlugin(origin, writer, instanceKey, mailer, list, logger)
if err != nil {
d.Close()
@@ -124,11 +128,7 @@ func New(cfg Config) (*App, error) {
mailRecorder: recorder,
auth: authPlugin,
}
- // ONE bounded sender for everything the handlers send — reminders,
- // booking mail. The recorder stays inside it, so tests still read
- // what was sent (booking spec §6).
- h := &handlers{store: store, logger: logger, origin: origin,
- mailer: newBoundedSender(mailer), gate: newGate()}
+ h := &handlers{store: store, logger: logger, origin: origin, mailer: mailer, gate: newGate()}
a.mux = a.routes(h, authPlugin, list, origin)
return a, nil
}
diff --git a/internal/calendar/booking_mail.go b/internal/calendar/booking_mail.go
index 128f0c1..9827f83 100644
--- a/internal/calendar/booking_mail.go
+++ b/internal/calendar/booking_mail.go
@@ -24,6 +24,9 @@ type mailData struct {
publicBooking
ManageURL string
EventURL string
+ // Note is the booker's note, for HOST mail only: the host is on the
+ // team and this is delivery (spec §6). Never set on a booker's mail.
+ Note string
// Stale is set on a "confirmed" mail whose booking is no longer
// confirmed when it finally renders, so the first line says so
// rather than announcing a meeting that was since cancelled (B19).
@@ -39,6 +42,7 @@ func (h *handlers) renderBookingMail(row BookingMail, v BookingView, now time.Ti
}
if row.Recipient == ToHost {
to = v.Host.Address
+ d.Note = v.Note
} else {
to = v.Address
}
@@ -74,6 +78,14 @@ func (h *handlers) sendBookingMail(ctx context.Context, row BookingMail, now tim
if err != nil {
return err
}
+ // A row the booking's CURRENT state makes meaningless is retired
+ // unsent (B19): a hold mail after the hold was confirmed or
+ // expired, a rescheduled mail after a cancellation. Mail says the
+ // current state or says nothing.
+ if obsolete(row, v, now) {
+ _, err := h.store.MarkBookingMailSent(ctx, row.ID, row.ClaimID, now.Unix())
+ return err
+ }
to, subject, body, err := h.renderBookingMail(row, v, now)
if err != nil {
return err
@@ -83,9 +95,11 @@ func (h *handlers) sendBookingMail(ctx context.Context, row BookingMail, now tim
err = h.mailer.Send(sendCtx, to, subject, body)
switch {
case err == nil:
- return h.store.MarkBookingMailSent(ctx, row.ID, row.ClaimID, now.Unix())
+ _, err := h.store.MarkBookingMailSent(ctx, row.ID, row.ClaimID, now.Unix())
+ return err
case errors.Is(err, ErrMailBusy):
- return h.store.DeferBookingMail(ctx, row.ID, row.ClaimID, now.Unix())
+ _, err := h.store.DeferBookingMail(ctx, row.ID, row.ClaimID, now.Unix())
+ return err
default:
gaveUp, merr := h.store.MarkBookingMailFailed(ctx, row.ID, row.ClaimID)
if merr != nil {
@@ -95,26 +109,43 @@ func (h *handlers) sendBookingMail(ctx context.Context, row BookingMail, now tim
if gaveUp {
code = "gave_up"
}
- // No address, no token, no title: a stage and a code.
- h.logger.Error("calendar: booking mail", "stage", "booking.mail", "code", code, "err", scrubErr(err))
+ // No address, no token, no title, and NOT the sender's text —
+ // it quotes the subject when it refuses a header: a stage and
+ // two codes.
+ h.logger.Error("calendar: booking mail", "stage", "booking.mail", "code", code, "why", mailCode(err))
return err
}
}
+// obsolete says whether a queued row has been overtaken by the state
+// it would describe.
+func obsolete(row BookingMail, v BookingView, now time.Time) bool {
+ switch row.Kind {
+ case MailHold:
+ return v.State != BookingHeld || now.Unix() >= v.HoldExpiresAt
+ case MailRescheduled:
+ return v.Cancelled()
+ }
+ return false
+}
+
// trySendNow is the immediate attempt after a transition: claim and
-// send every row the booking owes, once each. Failures are the drain's
-// to retry; the caller learns only whether everything went.
+// send THIS booking's owed rows, once each, inside a bounded budget.
+// Everybody else's mail is the drain's; a stranger's POST must not wait
+// through it. Failures are the drain's to retry; the caller learns only
+// whether everything went.
func (h *handlers) trySendNow(ctx context.Context, bookingID int64, now time.Time) (allSent bool) {
+ ctx, cancel := context.WithTimeout(ctx, 2*maxSendWait)
+ defer cancel()
allSent = true
- for {
- row, ok, err := h.store.ClaimBookingMail(ctx, now.Unix())
+ for i := 0; i < 4; i++ {
+ row, ok, err := h.store.ClaimBookingMailFor(ctx, bookingID, now.Unix())
if err != nil || !ok {
return allSent && err == nil
}
if err := h.sendBookingMail(ctx, row, now); err != nil {
- if row.BookingID == bookingID {
- allSent = false
- }
+ allSent = false
}
}
+ return allSent
}
diff --git a/internal/calendar/booking_public.go b/internal/calendar/booking_public.go
index 12d3e23..ead6143 100644
--- a/internal/calendar/booking_public.go
+++ b/internal/calendar/booking_public.go
@@ -63,6 +63,7 @@ func publicOf(v BookingView, now time.Time) publicBooking {
pb.ExpiresAt = instantWords(v.HoldExpiresAt, v.BookerZone)
case BookingExpired:
pb.Expired = true
+ pb.ExpiresAt = instantWords(v.HoldExpiresAt, v.BookerZone)
}
if pb.CancelledBy == "" && pb.Cancelled {
pb.CancelledBy = CancelledByHost
diff --git a/internal/calendar/handlers_booking_manage.go b/internal/calendar/handlers_booking_manage.go
index 07ca1d2..5f16718 100644
--- a/internal/calendar/handlers_booking_manage.go
+++ b/internal/calendar/handlers_booking_manage.go
@@ -141,6 +141,8 @@ func (h *handlers) bookReschedulePicker(w http.ResponseWriter, r *http.Request)
if !ok {
return
}
+ // The booking's own snapshot, not the page's live text (B20).
+ d.Title, d.Description = v.Title, v.Description
d.RescheduleToken, d.Rev, d.ZoneChosen = token, v.Event.Sequence, true
d.Path = "/book/m/" + token + "/reschedule"
render(w, "book_page", h.page(r, "Move "+v.Title, d))
@@ -152,16 +154,26 @@ func (h *handlers) bookReschedule(w http.ResponseWriter, r *http.Request) {
if !ok {
return
}
- _ = r.ParseForm()
- start, okStart := parseUnix(r.FormValue("start"))
- rev, _ := strconv.Atoi(r.FormValue("rev"))
+ r.Body = http.MaxBytesReader(w, r.Body, 16<<10)
+ if err := r.ParseForm(); err != nil {
+ http.Error(w, "That form was too large.", http.StatusUnprocessableEntity)
+ return
+ }
+ start, okStart := parseUnix(r.PostFormValue("start"))
+ rev, _ := strconv.Atoi(r.PostFormValue("rev"))
if !okStart {
http.Redirect(w, r, "/book/m/"+token+"/reschedule", http.StatusSeeOther)
return
}
end := start + (v.Event.EndUTC - v.Event.StartUTC)
now := time.Now()
+ release, ok := acquireSlots()
+ if !ok {
+ busyNow(w)
+ return
+ }
changed, err := h.store.RescheduleBooking(r.Context(), v.ID, start, end, rev, now)
+ release()
switch {
case errors.Is(err, ErrSlotTaken):
http.Redirect(w, r, "/book/m/"+token+"/reschedule?gone=1", http.StatusSeeOther)
diff --git a/internal/calendar/handlers_booking_public.go b/internal/calendar/handlers_booking_public.go
index 18cc496..fded7f1 100644
--- a/internal/calendar/handlers_booking_public.go
+++ b/internal/calendar/handlers_booking_public.go
@@ -177,6 +177,21 @@ func (h *handlers) bookPost(w http.ResponseWriter, r *http.Request) {
}
bookerLoc, _ := Zone(zone)
day := time.Unix(start, 0).In(bookerLoc).Format(LayoutDate)
+ if strings.TrimSpace(r.FormValue("name")) != "" || r.FormValue("address") != "" {
+ // The hold. No picker first: the store checks the caps and the
+ // slot itself, behind the write lock, and a page is computed
+ // only if it has to say "that time has just gone".
+ end := start + int64(page.DurationMin)*60
+ picked := bookSlot{StartUTC: start, EndUTC: end, Label: time.Unix(start, 0).In(bookerLoc).Format("15:04")}
+ d := bookPageData{Title: page.Title, Description: page.Description, HostName: hostNameOf(host),
+ DurationMin: page.DurationMin, Zone: zone, Zones: zoneChoices(host), Day: day, Path: r.URL.Path,
+ Handle: chi.URLParam(r, "handle"), Code: chi.URLParam(r, "code"), ZoneChosen: chosen, Picked: &picked}
+ if t, err := time.ParseInLocation(LayoutDate, day, bookerLoc); err == nil {
+ d.DayLabel = t.Format("Monday 2 January")
+ }
+ h.hold(w, r, page, host, d, zone)
+ return
+ }
d, ok := h.picker(w, r, page, host, zone, day, 0, 0)
if !ok {
return
@@ -193,12 +208,19 @@ func (h *handlers) bookPost(w http.ResponseWriter, r *http.Request) {
render(w, "book_page", h.page(r, page.Title, d))
return
}
- if strings.TrimSpace(r.FormValue("name")) == "" && r.FormValue("address") == "" {
- // The slot pick: show the details form beneath it.
- render(w, "book_page", h.page(r, page.Title, d))
- return
+ // The slot pick: show the details form beneath it.
+ render(w, "book_page", h.page(r, page.Title, d))
+}
+
+// hasControl reports a control character (a line break, a NUL) in a
+// single-line field.
+func hasControl(s string) bool {
+ for _, r := range s {
+ if r < 0x20 || r == 0x7f {
+ return true
+ }
}
- h.hold(w, r, page, host, d, zone)
+ return false
}
// hold validates the details and makes the hold (§5.2).
@@ -212,8 +234,11 @@ func (h *handlers) hold(w http.ResponseWriter, r *http.Request, page BookingPage
render(w, "book_page", h.page(r, page.Title, d))
}
switch {
- case d.Name == "" || utf8.RuneCountInString(d.Name) > MaxBookerNameRunes || len(d.Name) > MaxBookerNameBytes:
- reject("name", "Your name, please — up to sixty characters.")
+ case d.Name == "" || utf8.RuneCountInString(d.Name) > MaxBookerNameRunes || len(d.Name) > MaxBookerNameBytes || hasControl(d.Name):
+ // A line break in a name would reach a mail SUBJECT, and the
+ // SMTP sender refuses a folded header with an error that quotes
+ // it. Refused here, where it costs a sentence.
+ reject("name", "Your name, please — up to sixty characters, on one line.")
return
case !reAddress.MatchString(d.Address) || reAddress.FindString(d.Address) != d.Address:
reject("address", "That does not look like an email address.")
@@ -221,12 +246,24 @@ func (h *handlers) hold(w http.ResponseWriter, r *http.Request, page BookingPage
case utf8.RuneCountInString(d.Note) > MaxNoteRunes || len(d.Note) > MaxNoteBytes:
reject("note", "Keep the note shorter — a thousand characters is plenty.")
return
+ case hasControl(strings.ReplaceAll(d.Note, "\n", "")):
+ reject("note", "The note has characters that cannot be sent.")
+ return
}
now := time.Now()
+ // The store's own slot arithmetic runs under the same semaphore as
+ // the pickers (§4.2): four public computations at once, whoever
+ // asks for them.
+ release, ok := acquireSlots()
+ if !ok {
+ busyNow(w)
+ return
+ }
b, err := h.store.HoldBooking(r.Context(), HoldInput{
Page: page, Host: host, StartUTC: d.Picked.StartUTC, EndUTC: d.Picked.EndUTC,
Name: d.Name, Address: d.Address, Note: d.Note, BookerZone: zone, IPFP: h.ipFingerprint(r),
}, now)
+ release()
switch {
case errors.Is(err, ErrOverCap):
w.Header().Set("Retry-After", "1800")
@@ -235,9 +272,14 @@ func (h *handlers) hold(w http.ResponseWriter, r *http.Request, page BookingPage
"Title": page.Title, "Why": "Too many bookings in progress. Try again later."}))
return
case errors.Is(err, ErrSlotTaken):
- d.Gone, d.Picked = true, nil
+ // Only now compute the day's page, to say what is still free.
+ again, ok := h.picker(w, r, page, host, zone, d.Day, 0, 0)
+ if !ok {
+ return
+ }
+ again.Handle, again.Code, again.ZoneChosen, again.Gone = d.Handle, d.Code, d.ZoneChosen, true
w.WriteHeader(http.StatusConflict)
- render(w, "book_page", h.page(r, page.Title, d))
+ render(w, "book_page", h.page(r, page.Title, again))
return
case errors.Is(err, ErrIncompleteExpansion):
w.Header().Set("Retry-After", "60")
diff --git a/internal/calendar/handlers_jobs.go b/internal/calendar/handlers_jobs.go
index 04cede7..8304aac 100644
--- a/internal/calendar/handlers_jobs.go
+++ b/internal/calendar/handlers_jobs.go
@@ -61,11 +61,18 @@ func (h *handlers) drain(ctx context.Context, now time.Time) (int, error) {
} else if n > 0 {
h.logger.Info("calendar: ip fingerprints forgotten", "n", n)
}
- sent, err := h.drainReminderQueue(ctx, now, now.Add(drainDeadline/2))
+ // Each phase's deadline is a CONTEXT deadline, not only a loop
+ // condition: the bounded sender waits no longer than what is left,
+ // so a send started at second 44 cannot run to second 64.
+ rctx, rcancel := context.WithDeadline(ctx, now.Add(drainDeadline/2))
+ sent, err := h.drainReminderQueue(rctx, now, now.Add(drainDeadline/2))
+ rcancel()
if err != nil {
return sent, err
}
- mailed, err := h.drainBookingMail(ctx, now, deadline)
+ mctx, mcancel := context.WithDeadline(ctx, deadline)
+ defer mcancel()
+ mailed, err := h.drainBookingMail(mctx, now, deadline)
return sent + mailed, err
}
@@ -112,7 +119,7 @@ func (h *handlers) drainReminderQueue(ctx context.Context, now, deadline time.Ti
// retries. Recording the attempt is what stops a
// permanently undeliverable address being retried forever.
h.logger.Error("calendar: send reminder", "stage", "reminders.send",
- "err", scrubErr(err), "attempts", due.Queue.Attempts)
+ "why", mailCode(err), "attempts", due.Queue.Attempts)
if err := h.store.FailReminder(ctx, due.Queue.ID, due.ClaimID, due.StartUTC); err != nil {
return sent, err
}
@@ -214,7 +221,7 @@ func (h *handlers) DrainOnRequest(ctx context.Context) {
return
}
h.logger.Error("calendar: opportunistic reminder", "stage", "reminders.send",
- "err", scrubErr(err))
+ "why", mailCode(err))
_ = h.store.FailReminder(ctx, due.Queue.ID, due.ClaimID, due.StartUTC)
return
}
diff --git a/internal/calendar/logsafe.go b/internal/calendar/logsafe.go
index 3431429..d0bc069 100644
--- a/internal/calendar/logsafe.go
+++ b/internal/calendar/logsafe.go
@@ -1,6 +1,9 @@
package calendar
-import "regexp"
+import (
+ "errors"
+ "regexp"
+)
// addressPattern matches an email address well enough to redact one.
//
@@ -38,3 +41,20 @@ func scrubErr(err error) string {
}
return scrub(err.Error())
}
+
+// mailCode reduces a sender's error to a word. The sender's TEXT is
+// never logged: rastrillo's SMTP sender quotes the whole subject when
+// it refuses a header, and a subject carries the title and the booker's
+// name. Scrubbing addresses out of that is not enough.
+func mailCode(err error) string {
+ switch {
+ case err == nil:
+ return "ok"
+ case errors.Is(err, ErrMailBusy):
+ return "busy"
+ case errors.Is(err, ErrMailTimeout):
+ return "timeout"
+ default:
+ return "refused"
+ }
+}
diff --git a/internal/calendar/occurrences.go b/internal/calendar/occurrences.go
index e8f1243..eb0db8e 100644
--- a/internal/calendar/occurrences.go
+++ b/internal/calendar/occurrences.go
@@ -107,9 +107,10 @@ func ExpandComplete(startLocal, endLocal, tz string, allDay bool, r Rule, exdate
if eu > from && su < to {
out = append(out, Occurrence{Local: key, StartUTC: su, EndUTC: eu})
if len(out) >= MaxOccurrencesPerSeries {
- // The window was reached; the cap is about how
- // much of it to return, not whether we got there.
- complete = true
+ // The window was reached, but not all of it was
+ // returned. For a view that is a bounded page; for
+ // anything deciding FREE time it is an incomplete
+ // answer, and says so.
break
}
}
diff --git a/internal/calendar/occurrences_step.go b/internal/calendar/occurrences_step.go
index 44b1306..b010ef2 100644
--- a/internal/calendar/occurrences_step.go
+++ b/internal/calendar/occurrences_step.go
@@ -38,7 +38,7 @@ func advance(cur, proto time.Time, r Rule, loc *time.Location) (time.Time, bool)
if !containsWeekday(r.ByDay, cand.Weekday()) {
continue
}
- if r.Interval > 1 && weeksBetween(proto, cand)%r.Interval != 0 {
+ if r.Interval > 1 && weeksBetween(proto, cand, r.WeekStart)%r.Interval != 0 {
continue
}
return onDate(cand, proto, loc), true
@@ -148,14 +148,20 @@ func civil(t time.Time) time.Time {
return time.Date(t.Year(), t.Month(), t.Day(), 0, 0, 0, 0, time.UTC)
}
-// weeksBetween counts calendar weeks (Sunday-started) between two days,
-// for INTERVAL on a weekly rule.
-func weeksBetween(a, b time.Time) int {
- da := civil(a).AddDate(0, 0, -int(a.Weekday()))
- db := civil(b).AddDate(0, 0, -int(b.Weekday()))
+// weeksBetween counts calendar weeks between two days, for INTERVAL on
+// a weekly rule. Weeks start on wkst — the rule's WKST, Monday by the
+// RFC's default — and where a week starts decides which week a Sunday
+// or a Monday is in.
+func weeksBetween(a, b time.Time, wkst time.Weekday) int {
+ da := civil(a).AddDate(0, 0, -daysSinceWeekStart(a.Weekday(), wkst))
+ db := civil(b).AddDate(0, 0, -daysSinceWeekStart(b.Weekday(), wkst))
return int(db.Sub(da).Hours() / (24 * 7))
}
+func daysSinceWeekStart(d, wkst time.Weekday) int {
+ return (int(d) - int(wkst) + 7) % 7
+}
+
// daysBetween is the whole-day span from a start to an end, used to
// carry an event's length across occurrences in wall clock. Civil
// dates, never elapsed hours: 28 March 09:00 to 30 March 09:00 in
diff --git a/internal/calendar/recur.go b/internal/calendar/recur.go
index e25e509..7bd6387 100644
--- a/internal/calendar/recur.go
+++ b/internal/calendar/recur.go
@@ -46,6 +46,10 @@ type Rule struct {
Ordinal int
OrdinalDay time.Weekday
+ // WeekStart is the day weeks are counted from, for INTERVAL on a
+ // weekly rule. The RFC's default is Monday; ParseRule sets it.
+ WeekStart time.Weekday
+
// Count and Until are the two ways a series ends; a series may have
// neither, and then it is forever.
Count int
@@ -62,6 +66,7 @@ type Rule struct {
// ErrForeignRule, which callers store rather than reject.
func ParseRule(s string) (Rule, error) {
r := Rule{Interval: 1}
+ r.WeekStart = time.Monday // the RFC default; WKST overrides
s = strings.TrimSpace(s)
if s == "" {
return r, nil
@@ -120,9 +125,17 @@ func ParseRule(s string) (Rule, error) {
}
r.MonthDay = n
case "WKST":
- // Affects only rules this subset does not express (weekly
- // with INTERVAL > 1 spanning a week boundary is the real
- // case, and it is rare enough to accept the default).
+ // Which day a week starts on, for a weekly rule with an
+ // interval: "every second week, Sunday and Monday" from a
+ // Sunday puts that Monday in the NEXT week under the RFC's
+ // Monday default and in the same week under a Sunday start.
+ // A booking page that read it wrong offered the Monday as
+ // free.
+ wd, ok := weekdayFromICS(v)
+ if !ok {
+ return r, fmt.Errorf("%w: WKST=%s", ErrForeignRule, v)
+ }
+ r.WeekStart = wd
default:
return r, fmt.Errorf("%w: %s", ErrForeignRule, k)
}
diff --git a/internal/calendar/static/booking.js b/internal/calendar/static/booking.js
index 00e6abb..3285b70 100644
--- a/internal/calendar/static/booking.js
+++ b/internal/calendar/static/booking.js
@@ -73,9 +73,17 @@
return;
}
if (!guess) return;
+ if (select.value === guess) return;
for (var i = 0; i < select.options.length; i++) {
if (select.options[i].value === guess) {
+ // Reload THROUGH the form, so the server re-renders every time
+ // on the page in the guessed zone. Changing the select alone
+ // would relabel the selector while the times stayed in the
+ // host's zone — a page that says one zone and means another.
select.value = guess;
+ var form = select.form;
+ if (form && typeof form.requestSubmit === "function") form.requestSubmit();
+ else if (form) form.submit();
return;
}
}
diff --git a/internal/calendar/store_booking_cancel.go b/internal/calendar/store_booking_cancel.go
index 115eed4..7fd9d36 100644
--- a/internal/calendar/store_booking_cancel.go
+++ b/internal/calendar/store_booking_cancel.go
@@ -87,9 +87,13 @@ func cancelBookingsForEvent(tx *gorm.DB, eventID int64, now time.Time) error {
_, err := cancelBookingOn(tx, b, CancelledByHost, now)
return err
case BookingHeld:
- if err := tx.Model(&Booking{}).Where("id = ? AND state = ?", b.ID, BookingHeld).
- Updates(map[string]any{"state": BookingExpired, "updated_at": now.UTC()}).Error; err != nil {
- return fmt.Errorf("calendar: expire held booking: %w", err)
+ res := tx.Model(&Booking{}).Where("id = ? AND state = ?", b.ID, BookingHeld).
+ Updates(map[string]any{"state": BookingExpired, "updated_at": now.UTC()})
+ if res.Error != nil {
+ return fmt.Errorf("calendar: expire held booking: %w", res.Error)
+ }
+ if res.RowsAffected == 0 {
+ return nil // confirmed under us; that transition owns the rest
}
if err := deleteReminderRowsFor(tx, eventID); err != nil {
return err
diff --git a/internal/calendar/store_booking_confirm.go b/internal/calendar/store_booking_confirm.go
index 9c6296c..f3150af 100644
--- a/internal/calendar/store_booking_confirm.go
+++ b/internal/calendar/store_booking_confirm.go
@@ -2,6 +2,7 @@ package calendar
import (
"context"
+ "errors"
"fmt"
"time"
@@ -117,6 +118,14 @@ func (s *Store) RescheduleBooking(ctx context.Context, bookingID, startUTC, endU
if err := tx.Exec(`UPDATE booking_pages SET updated_at = updated_at WHERE id = ?`, v.PageID).Error; err != nil {
return fmt.Errorf("calendar: take write lock: %w", err)
}
+ // The host as they are NOW, behind the lock: tz below is the
+ // zone the slot was computed in (B21), so compute it there.
+ if err := tx.First(&v.Host, v.Host.ID).Error; err != nil {
+ return fmt.Errorf("calendar: re-read host: %w", err)
+ }
+ if loc, err = Zone(v.Host.Zone); err != nil {
+ return err
+ }
again, err := s.freeSlotsOn(tx, v.Page, v.Host, now, day, dayEnd, v.EventID, dur)
if err != nil {
return err
@@ -142,10 +151,14 @@ func (s *Store) RescheduleBooking(ctx context.Context, bookingID, startUTC, endU
return nil
}
changed = true
- if err := tx.Model(&Booking{}).
+ cnt := tx.Model(&Booking{}).
Where("id = ? AND state = ?", bookingID, BookingConfirmed).
- Updates(map[string]any{"reschedules": gorm.Expr("reschedules + 1"), "updated_at": now.UTC()}).Error; err != nil {
- return fmt.Errorf("calendar: count reschedule: %w", err)
+ Updates(map[string]any{"reschedules": gorm.Expr("reschedules + 1"), "updated_at": now.UTC()})
+ if cnt.Error != nil {
+ return fmt.Errorf("calendar: count reschedule: %w", cnt.Error)
+ }
+ if cnt.RowsAffected == 0 {
+ return errClaimLost // cancelled under us; the event UPDATE above rolls back with this
}
if err := s.rescheduleAllForTx(tx, v.EventID); err != nil {
return err
@@ -155,5 +168,8 @@ func (s *Store) RescheduleBooking(ctx context.Context, bookingID, startUTC, endU
}
return queueBookingMail(tx, bookingID, MailRescheduled, []string{ToBooker, ToHost}, rev+1, now)
})
+ if errors.Is(err, errClaimLost) {
+ return false, nil
+ }
return changed, err
}
diff --git a/internal/calendar/store_booking_flow.go b/internal/calendar/store_booking_flow.go
index 7e3819c..f0fa932 100644
--- a/internal/calendar/store_booking_flow.go
+++ b/internal/calendar/store_booking_flow.go
@@ -139,11 +139,27 @@ func (s *Store) HoldBooking(ctx context.Context, in HoldInput, now time.Time) (B
s.afterSlotCheckForTest()
}
+ var b Booking
+ for attempt := 0; attempt < 2; attempt++ {
+ b, err = s.holdOnce(ctx, in, addrFP, day, dayEnd, now)
+ if err == nil || !errors.Is(err, errTokenCollision) {
+ return b, err
+ }
+ // A 256-bit collision is not a thing that happens; a second
+ // draw is the spec's whole answer to it, and then we stop.
+ }
+ return Booking{}, err
+}
+
+var errTokenCollision = errors.New("calendar: token collision")
+
+func (s *Store) holdOnce(ctx context.Context, in HoldInput, addrFP string, day, dayEnd, now time.Time) (Booking, error) {
token := newToken()
sealed, err := s.sealBooker(in, token)
if err != nil {
return Booking{}, err
}
+ loc, _ := Zone(in.Host.Zone)
var b Booking
err = s.g.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
@@ -151,7 +167,19 @@ func (s *Store) HoldBooking(ctx context.Context, in HoldInput, now time.Time) (B
if err := tx.Exec(`UPDATE booking_pages SET updated_at = updated_at WHERE id = ?`, in.Page.ID).Error; err != nil {
return fmt.Errorf("calendar: take write lock: %w", err)
}
- again, err := s.freeSlotsOn(tx, in.Page, in.Host, now, day, dayEnd, 0, 0)
+ // The page, its windows and the host as they are NOW, behind
+ // the lock: a page unpublished or a host who moved zone since
+ // the request read them must not be booked against.
+ page, host, err := s.currentPageOn(tx, in.Page.ID)
+ if err != nil {
+ return err
+ }
+ in.Page, in.Host = page, host
+ loc, err = Zone(host.Zone)
+ if err != nil {
+ return err
+ }
+ again, err := s.freeSlotsOn(tx, page, host, now, day, dayEnd, 0, 0)
if err != nil {
return err
}
@@ -200,7 +228,7 @@ func (s *Store) HoldBooking(ctx context.Context, in HoldInput, now time.Time) (B
}
if err := tx.Create(&b).Error; err != nil {
if isUniqueViolation(err) {
- return fmt.Errorf("calendar: token collision: %w", err)
+ return errTokenCollision
}
return fmt.Errorf("calendar: create booking: %w", err)
}
@@ -211,47 +239,3 @@ func (s *Store) HoldBooking(ctx context.Context, in HoldInput, now time.Time) (B
}
return b, nil
}
-
-type sealedBooker struct {
- token, title, description, name, address, note []byte
-}
-
-func (s *Store) sealBooker(in HoldInput, token string) (sealedBooker, error) {
- var out sealedBooker
- var err error
- seal1 := func(dst *[]byte, purpose, plain, what string) {
- if err != nil {
- return
- }
- if *dst, err = s.seal.SealString(purpose, plain); err != nil {
- err = fmt.Errorf("calendar: seal booking %s: %w", what, err)
- }
- }
- seal1(&out.token, seal.PurposeBookingToken, token, "token")
- seal1(&out.title, seal.PurposeBookingTitle, in.Page.Title, "title")
- seal1(&out.description, seal.PurposeBookingDescription, in.Page.Description, "description")
- seal1(&out.name, seal.PurposeBookerName, in.Name, "name")
- seal1(&out.address, seal.PurposeBookerAddress, normalizeAddress(in.Address), "address")
- seal1(&out.note, seal.PurposeBookerNote, in.Note, "note")
- return out, err
-}
-
-func hasSlot(slots []Slot, startUTC, endUTC int64) bool {
- for _, s := range slots {
- if s.StartUTC == startUTC && s.EndUTC == endUTC {
- return true
- }
- }
- return false
-}
-
-// defaultCalendarOn is DefaultCalendarID on a transaction.
-func (s *Store) defaultCalendarOn(tx *gorm.DB, personID int64) (int64, error) {
- var cal Calendar
- err := tx.Where("owner_id = ? AND kind = ? AND archived_at IS NULL", personID, "personal").
- Order("id").First(&cal).Error
- if err != nil {
- return 0, fmt.Errorf("calendar: host has no personal calendar: %w", err)
- }
- return cal.ID, nil
-}
diff --git a/internal/calendar/store_booking_hold_util.go b/internal/calendar/store_booking_hold_util.go
new file mode 100644
index 0000000..8cfba40
--- /dev/null
+++ b/internal/calendar/store_booking_hold_util.go
@@ -0,0 +1,88 @@
+package calendar
+
+import (
+ "errors"
+ "fmt"
+ "time"
+
+ "gorm.io/gorm"
+
+ "amadan.net/oficina/calendar/internal/calendar/seal"
+)
+
+// The hold's helpers, split from store_booking_flow.go so neither file
+// passes the 300-line cap.
+
+// currentPageOn re-reads a page, its windows and its host on a
+// transaction. A page that is no longer published is ErrSlotTaken to
+// the caller: the time is not on offer any more.
+func (s *Store) currentPageOn(tx *gorm.DB, pageID int64) (BookingPageView, Person, error) {
+ var page BookingPage
+ if err := tx.Where("id = ? AND published_at IS NOT NULL", pageID).First(&page).Error; err != nil {
+ if errors.Is(err, gorm.ErrRecordNotFound) {
+ return BookingPageView{}, Person{}, ErrSlotTaken
+ }
+ return BookingPageView{}, Person{}, fmt.Errorf("calendar: re-read page: %w", err)
+ }
+ var rows []BookingWindow
+ if err := tx.Where("page_id = ?", pageID).Order("weekday, start_min").Find(&rows).Error; err != nil {
+ return BookingPageView{}, Person{}, fmt.Errorf("calendar: re-read windows: %w", err)
+ }
+ windows := make([]Window, 0, len(rows))
+ for _, r := range rows {
+ windows = append(windows, Window{Weekday: time.Weekday(r.Weekday), StartMin: r.StartMin, EndMin: r.EndMin})
+ }
+ view, err := s.openPage(page, windows)
+ if err != nil {
+ return BookingPageView{}, Person{}, err
+ }
+ var host Person
+ if err := tx.First(&host, page.PersonID).Error; err != nil {
+ return BookingPageView{}, Person{}, fmt.Errorf("calendar: re-read host: %w", err)
+ }
+ return view, host, nil
+}
+
+type sealedBooker struct {
+ token, title, description, name, address, note []byte
+}
+
+func (s *Store) sealBooker(in HoldInput, token string) (sealedBooker, error) {
+ var out sealedBooker
+ var err error
+ seal1 := func(dst *[]byte, purpose, plain, what string) {
+ if err != nil {
+ return
+ }
+ if *dst, err = s.seal.SealString(purpose, plain); err != nil {
+ err = fmt.Errorf("calendar: seal booking %s: %w", what, err)
+ }
+ }
+ seal1(&out.token, seal.PurposeBookingToken, token, "token")
+ seal1(&out.title, seal.PurposeBookingTitle, in.Page.Title, "title")
+ seal1(&out.description, seal.PurposeBookingDescription, in.Page.Description, "description")
+ seal1(&out.name, seal.PurposeBookerName, in.Name, "name")
+ seal1(&out.address, seal.PurposeBookerAddress, normalizeAddress(in.Address), "address")
+ seal1(&out.note, seal.PurposeBookerNote, in.Note, "note")
+ return out, err
+}
+
+func hasSlot(slots []Slot, startUTC, endUTC int64) bool {
+ for _, s := range slots {
+ if s.StartUTC == startUTC && s.EndUTC == endUTC {
+ return true
+ }
+ }
+ return false
+}
+
+// defaultCalendarOn is DefaultCalendarID on a transaction.
+func (s *Store) defaultCalendarOn(tx *gorm.DB, personID int64) (int64, error) {
+ var cal Calendar
+ err := tx.Where("owner_id = ? AND kind = ? AND archived_at IS NULL", personID, "personal").
+ Order("id").First(&cal).Error
+ if err != nil {
+ return 0, fmt.Errorf("calendar: host has no personal calendar: %w", err)
+ }
+ return cal.ID, nil
+}
diff --git a/internal/calendar/store_booking_mail.go b/internal/calendar/store_booking_mail.go
index 0580bba..5758513 100644
--- a/internal/calendar/store_booking_mail.go
+++ b/internal/calendar/store_booking_mail.go
@@ -55,11 +55,23 @@ func deleteUnsentMail(tx *gorm.DB, bookingID int64, kinds ...string) error {
// crash after a claim harmless — the row comes due again when the lease
// ends, and NextDueAt reads nothing but due_at.
func (s *Store) ClaimBookingMail(ctx context.Context, now int64) (BookingMail, bool, error) {
+ return s.claimBookingMail(ctx, now, 0)
+}
+
+// ClaimBookingMailFor is the claim restricted to one booking: what a
+// request tries right after its own transition, so a stranger's POST
+// never waits through everybody else's queue.
+func (s *Store) ClaimBookingMailFor(ctx context.Context, bookingID, now int64) (BookingMail, bool, error) {
+ return s.claimBookingMail(ctx, now, bookingID)
+}
+
+func (s *Store) claimBookingMail(ctx context.Context, now, bookingID int64) (BookingMail, bool, error) {
var cand BookingMail
- err := s.g.WithContext(ctx).
- Where("sent_at IS NULL AND due_at <= ?", now).
- Order("due_at ASC, id ASC").
- First(&cand).Error
+ q := s.g.WithContext(ctx).Where("sent_at IS NULL AND due_at <= ?", now)
+ if bookingID != 0 {
+ q = q.Where("booking_id = ?", bookingID)
+ }
+ err := q.Order("due_at ASC, id ASC").First(&cand).Error
if errors.Is(err, gorm.ErrRecordNotFound) {
return BookingMail{}, false, nil
}
@@ -93,14 +105,16 @@ func (s *Store) ClaimBookingMail(ctx context.Context, now int64) (BookingMail, b
}
// MarkBookingMailSent acknowledges a send, conditional on the claim.
-func (s *Store) MarkBookingMailSent(ctx context.Context, id int64, claimID string, now int64) error {
- err := s.g.WithContext(ctx).Model(&BookingMail{}).
+// ok is false when the claim was no longer ours; nothing depends on
+// it, but the fact is reported rather than swallowed.
+func (s *Store) MarkBookingMailSent(ctx context.Context, id int64, claimID string, now int64) (ok bool, err error) {
+ res := s.g.WithContext(ctx).Model(&BookingMail{}).
Where("id = ? AND claim_id = ? AND sent_at IS NULL", id, claimID).
- Update("sent_at", now).Error
- if err != nil {
- return fmt.Errorf("calendar: mark booking mail sent: %w", err)
+ Update("sent_at", now)
+ if res.Error != nil {
+ return false, fmt.Errorf("calendar: mark booking mail sent: %w", res.Error)
}
- return nil
+ return res.RowsAffected == 1, nil
}
// MarkBookingMailFailed counts a real send error. due_at already points
@@ -128,14 +142,14 @@ func (s *Store) MarkBookingMailFailed(ctx context.Context, id int64, claimID str
// DeferBookingMail puts a row back without an attempt: the sender was
// busy and nothing was tried (B22).
-func (s *Store) DeferBookingMail(ctx context.Context, id int64, claimID string, now int64) error {
- err := s.g.WithContext(ctx).Model(&BookingMail{}).
+func (s *Store) DeferBookingMail(ctx context.Context, id int64, claimID string, now int64) (ok bool, err error) {
+ res := s.g.WithContext(ctx).Model(&BookingMail{}).
Where("id = ? AND claim_id = ?", id, claimID).
- Updates(map[string]any{"due_at": now + int64(deferFor.Seconds()), "claimed_at": nil, "claim_id": ""}).Error
- if err != nil {
- return fmt.Errorf("calendar: defer booking mail: %w", err)
+ Updates(map[string]any{"due_at": now + int64(deferFor.Seconds()), "claimed_at": nil, "claim_id": ""})
+ if res.Error != nil {
+ return false, fmt.Errorf("calendar: defer booking mail: %w", res.Error)
}
- return nil
+ return res.RowsAffected == 1, nil
}
// NextBookingMailDue is the earliest owed row's due_at — always a
diff --git a/internal/calendar/store_drain.go b/internal/calendar/store_drain.go
index daf30c5..c48aa57 100644
--- a/internal/calendar/store_drain.go
+++ b/internal/calendar/store_drain.go
@@ -91,8 +91,12 @@ func (s *Store) ClaimDueReminder(ctx context.Context, now int64) (DueReminder, b
claimID := newClaimID()
var q ReminderQueue
err = s.g.WithContext(ctx).Transaction(func(tx *gorm.DB) error {
+ // The whole eligibility predicate again, due_at included: a
+ // reschedule between the candidate read and here rewrites the
+ // row to a later time, and a claim that skipped due_at would
+ // take it, send tomorrow's reminder now, and retire it.
res := tx.Model(&ReminderQueue{}).
- Where("id = ? AND fired_at IS NULL", cand.ID).
+ Where("id = ? AND fired_at IS NULL AND due_at <= ?", cand.ID, now).
Where("retry_at IS NULL OR retry_at <= ?", now).
Where("claimed_at IS NULL OR claimed_at <= ?", cutoff).
Updates(map[string]any{"claimed_at": now, "claim_id": claimID})
@@ -119,13 +123,19 @@ func (s *Store) ClaimDueReminder(ctx context.Context, now int64) (DueReminder, b
err = s.g.WithContext(ctx).
Where("id = ? AND deleted_at IS NULL AND status <> ?", rem.EventID, StatusCancelled).
First(&e).Error
- if err != nil {
+ if errors.Is(err, gorm.ErrRecordNotFound) {
// The event went away, or was cancelled, between queueing and
// firing. Drop the row rather than send a reminder for
- // something that is not happening.
- _ = s.g.WithContext(ctx).Delete(&ReminderQueue{}, q.ID).Error
+ // something that is not happening — but only OUR row, under
+ // our claim: a reschedule that overtook us rewrote it.
+ _ = s.g.WithContext(ctx).Where("id = ? AND claim_id = ?", q.ID, claimID).Delete(&ReminderQueue{}).Error
return DueReminder{}, false, nil
}
+ if err != nil {
+ // A transient read error is not "gone". Leave the claim; the
+ // lease expires and a later pass tries again.
+ return DueReminder{}, false, fmt.Errorf("calendar: read event for reminder: %w", err)
+ }
var p Person
if err := s.g.WithContext(ctx).First(&p, rem.PersonID).Error; err != nil {
return DueReminder{}, false, fmt.Errorf("calendar: read person: %w", err)
diff --git a/internal/calendar/store_event_write.go b/internal/calendar/store_event_write.go
index f79bccc..f6e7607 100644
--- a/internal/calendar/store_event_write.go
+++ b/internal/calendar/store_event_write.go
@@ -101,15 +101,20 @@ func (s *Store) updateEventOn(tx *gorm.DB, existing Event, in EventInput, now ti
} else {
updates["until_utc"] = nil
}
+ // The host cancelling a booked meeting cancels the booking and
+ // tells the booker (booking spec §5.5). BEFORE the event's own
+ // bump, so the cancellation mail names the revision that bump
+ // produces — every cancellation entrance queues seq+1 and then
+ // bumps once.
+ if booked && in.Status == StatusCancelled && stored.Status != StatusCancelled {
+ if err := cancelBookingsForEvent(tx, stored.ID, now); err != nil {
+ return err
+ }
+ }
if err := tx.Model(&Event{}).
Where("id = ?", stored.ID).Updates(updates).Error; err != nil {
return fmt.Errorf("calendar: update event: %w", err)
}
- // The host cancelling a booked meeting cancels the booking and
- // tells the booker (booking spec §5.5).
- if booked && in.Status == StatusCancelled && stored.Status != StatusCancelled {
- return cancelBookingsForEvent(tx, stored.ID, now)
- }
return nil
}
diff --git a/internal/calendar/templates/event_form.html b/internal/calendar/templates/event_form.html
index 5ebe117..3844def 100644
--- a/internal/calendar/templates/event_form.html
+++ b/internal/calendar/templates/event_form.html
@@ -49,8 +49,11 @@
<input type="hidden" name="repeat" value="none">
{{if .Data.AllDay}}<input type="hidden" name="all_day" value="1">{{end}}
<div rst-field cal-booked-lock>
- <span rst-field-label>When</span>
- <p cal-booked-when>{{.Data.StartLocal}} – {{.Data.EndLocal}} <small>({{.Data.TZ}})</small></p>
+ <label rst-field-label for="start-locked">Starts</label>
+ <input rst-input id="start-locked" type="datetime-local" value="{{.Data.StartLocal}}" disabled>
+ <label rst-field-label for="end-locked">Ends</label>
+ <input rst-input id="end-locked" type="datetime-local" value="{{.Data.EndLocal}}" disabled>
+ <p cal-booked-when><small>{{.Data.TZ}} &middot; {{.Data.Kind}}</small></p>
<small rst-field-hint>This is a booked meeting{{with .Data.BookedBy}} with {{.}}{{end}}. Its time is changed by the person who booked it, or by cancelling and letting them rebook.</small>
{{with (if_eq .Data.ErrorField "start" .Data.Error)}}<small rst-field-error>{{.}}</small>{{end}}
</div>
diff --git a/internal/calendar/templates/mail/confirmed.txt b/internal/calendar/templates/mail/confirmed.txt
index 6e9f8a4..78e0362 100644
--- a/internal/calendar/templates/mail/confirmed.txt
+++ b/internal/calendar/templates/mail/confirmed.txt
@@ -1,7 +1,9 @@
{{if .Stale}}This booking has since been {{.State}}. What follows is the confirmation as it was.
{{end}}{{.Name}} ({{.Address}}) booked {{.Title}}, {{if .HostWhen}}{{.HostWhen}}{{else}}{{.When}}{{end}}.
-
+{{with .Note}}
+They wrote: {{.}}
+{{end}}
It is on your calendar:
{{.EventURL}}
diff --git a/internal/calendartest/booking_flow_test.go b/internal/calendartest/booking_flow_test.go
index 402d708..3f1ed0e 100644
--- a/internal/calendartest/booking_flow_test.go
+++ b/internal/calendartest/booking_flow_test.go
@@ -204,14 +204,19 @@ func TestConfirmIsAConditionalUpdateWithGuards(t *testing.T) {
if changed, _ := st.ConfirmBooking(ctx, b2.ID, now.Add(calendar.HoldTTL+time.Minute)); changed {
t.Error("confirm after the hold lapsed changed something")
}
- // Started: no change (B15).
- if _, err := app.DBForTest().Exec(`UPDATE bookings SET hold_expires_at = ? WHERE id = ?`, now.Add(time.Hour).Unix(), b2.ID); err != nil {
+ // Started: no change (B15). The hold is pushed a WEEK out, so the
+ // expiry guard passes and only the start guard can refuse.
+ v2, _ := st.BookingByID(ctx, b2.ID, false)
+ if _, err := app.DBForTest().Exec(`UPDATE bookings SET hold_expires_at = ? WHERE id = ?`, now.AddDate(0, 0, 7).Unix(), b2.ID); err != nil {
t.Fatal(err)
}
- startedAt := time.Unix(slotsFor(t, app, page, host, now, 3)[0].StartUTC, 0)
- if changed, _ := st.ConfirmBooking(ctx, b2.ID, startedAt.Add(2*time.Hour)); changed {
+ startedAt := time.Unix(v2.Event.StartUTC, 0)
+ if changed, _ := st.ConfirmBooking(ctx, b2.ID, startedAt.Add(time.Minute)); changed {
t.Error("confirm after the meeting started changed something")
}
+ if changed, _ := st.ConfirmBooking(ctx, b2.ID, startedAt.Add(-time.Minute)); !changed {
+ t.Error("confirm a minute before the start was refused — the start guard is wrong")
+ }
}
func TestRescheduleNamesItsRevisionAndCanReturnHome(t *testing.T) {
diff --git a/internal/calendartest/booking_host_test.go b/internal/calendartest/booking_host_test.go
index 04dee40..93b3f4c 100644
--- a/internal/calendartest/booking_host_test.go
+++ b/internal/calendartest/booking_host_test.go
@@ -74,9 +74,12 @@ func TestTheHostMayNotMoveKindOrRepeatABookedMeeting(t *testing.T) {
}
})
}
- // All-day needs date-form strings to pass the validator first.
+ // All-day needs date-form strings with an exclusive NEXT-day end to
+ // pass the ordinary validator, so the refusal below is the booking
+ // rule's and not the validator's.
edit := in
- edit.AllDay, edit.StartLocal, edit.EndLocal = true, in.StartLocal[:10], in.EndLocal[:10]
+ nextDay, _ := time.Parse("2006-01-02", in.EndLocal[:10])
+ edit.AllDay, edit.StartLocal, edit.EndLocal = true, in.StartLocal[:10], nextDay.AddDate(0, 0, 1).Format("2006-01-02")
if _, ok := calendar.Invalid(st.UpdateEvent(ctx, host.ID, b.EventID, edit)); !ok {
t.Error("all-day edit of a booked meeting was accepted")
}
diff --git a/internal/calendartest/booking_mail_test.go b/internal/calendartest/booking_mail_test.go
index fcbf3fb..a821557 100644
--- a/internal/calendartest/booking_mail_test.go
+++ b/internal/calendartest/booking_mail_test.go
@@ -31,15 +31,15 @@ func TestBookingMailClaimsHaveAnIdentityAndALease(t *testing.T) {
t.Errorf("NextBookingMailDue = %v, want the lease end", next)
}
// A stale claim id cannot acknowledge it.
- if err := st.MarkBookingMailSent(ctx, row.ID, "not-the-claim", now.Unix()); err != nil {
- t.Fatal(err)
+ if ok, err := st.MarkBookingMailSent(ctx, row.ID, "not-the-claim", now.Unix()); err != nil || ok {
+ t.Fatalf("a stale claim id acknowledged the row: ok=%v err=%v", ok, err)
}
if len(st.UnsentBookingMailForTest(b.ID)) != 1 {
t.Error("a stale claim id acknowledged the row")
}
// Deferral keeps attempts at zero and is claimable again in 30s.
- if err := st.DeferBookingMail(ctx, row.ID, row.ClaimID, now.Unix()); err != nil {
- t.Fatal(err)
+ if ok, err := st.DeferBookingMail(ctx, row.ID, row.ClaimID, now.Unix()); err != nil || !ok {
+ t.Fatalf("defer: ok=%v err=%v", ok, err)
}
if again, ok, _ := st.ClaimBookingMail(ctx, now.Unix()+31); !ok || again.Attempts != 0 {
t.Errorf("after defer: ok=%v attempts=%d", ok, again.Attempts)
diff --git a/internal/calendartest/booking_review_test.go b/internal/calendartest/booking_review_test.go
new file mode 100644
index 0000000..5b04513
--- /dev/null
+++ b/internal/calendartest/booking_review_test.go
@@ -0,0 +1,92 @@
+package calendartest
+
+import (
+ "context"
+ "net/http"
+ "strings"
+ "testing"
+ "time"
+
+ calendar "amadan.net/oficina/calendar/internal/calendar"
+)
+
+// Regressions from the code review of the booking branch.
+
+// A line break in a name would reach the mail subject, and the SMTP
+// sender refuses a folded header with an error that quotes the whole
+// subject — title, name and all — into the log (code review finding 1).
+// Refused at the form instead.
+func TestANameWithALineBreakIsRefused(t *testing.T) {
+ app, _, path, _ := publishedPage(t)
+ h := app.Handler()
+ _, starts := firstDaySlots(t, h, path)
+ rec := strangerPost(t, h, path, holdForm(starts[0], "Jane\nPrivate", "j@example.com"))
+ if rec.Code != http.StatusUnprocessableEntity || !strings.Contains(rec.Body.String(), "one line") {
+ t.Errorf("a name with a newline: %d", rec.Code)
+ }
+}
+
+// The host's confirmation carries the booker's note (spec §6); the
+// booker's mail never carries anything but their own words back.
+func TestTheHostsConfirmationCarriesTheNote(t *testing.T) {
+ app, mailer, path, _ := publishedPage(t)
+ h := app.Handler()
+ _, starts := firstDaySlots(t, h, path)
+ form := holdForm(starts[0], "Doc", "doc@tito.io")
+ form.Set("note", "Bring the folder")
+ strangerPost(t, h, path, form)
+ link := reManage.FindString(mailer.body(0))
+ strangerPost(t, h, link+"/confirm", nil)
+ if !strings.Contains(mailer.body(1), "Bring the folder") {
+ t.Errorf("the host's mail lacks the note:\n%s", mailer.body(1))
+ }
+}
+
+// The reschedule picker shows the booking's snapshotted title, not the
+// page's live one (B20).
+func TestTheReschedulePickerShowsTheSnapshot(t *testing.T) {
+ app, mailer, path, host := publishedPage(t)
+ h := app.Handler()
+ _, starts := firstDaySlots(t, h, path)
+ strangerPost(t, h, path, holdForm(starts[0], "Doc", "doc@tito.io"))
+ link := reManage.FindString(mailer.body(0))
+ strangerPost(t, h, link+"/confirm", nil)
+ st := app.StoreForTest()
+ pages, _ := st.BookingPages(context.Background(), host.ID)
+ ws, _ := calendar.ParseAvailability("every day 9-5")
+ if err := st.UpdateBookingPage(context.Background(), host.ID, pages[0].ID, calendar.BookingPageInput{
+ Title: "Renamed page", Availability: "every day 9-5", DurationMin: 30, HorizonDays: 30}, ws); err != nil {
+ t.Fatal(err)
+ }
+ body := strangerGet(t, h, link+"/reschedule").Body.String()
+ if strings.Contains(body, "Renamed page") || !strings.Contains(body, "Quick chat") {
+ t.Error("the reschedule picker shows the page's live title, not the booking's snapshot")
+ }
+}
+
+// A hold mail that is still owed when the booker confirms (the first
+// try failed, or was ambiguous) is retired unsent, not delivered as an
+// invitation to confirm what is already confirmed (B19).
+func TestAnOwedHoldMailIsRetiredOnceConfirmed(t *testing.T) {
+ t.Setenv("CARLOS_ADMIN_TOKEN", "test-token")
+ app, mailer, path, _ := publishedPage(t)
+ h := app.Handler()
+ _, starts := firstDaySlots(t, h, path)
+ mailer.refuse = "doc@tito.io"
+ strangerPost(t, h, path, holdForm(starts[0], "Doc", "doc@tito.io"))
+ mailer.refuse = ""
+ // The hold mail is owed; confirm through the store, as the booker
+ // who somehow got the link would.
+ var id int64
+ app.DBForTest().QueryRow(`SELECT id FROM bookings`).Scan(&id)
+ if _, err := app.StoreForTest().ConfirmBooking(context.Background(), id, time.Now()); err != nil {
+ t.Fatal(err)
+ }
+ app.DBForTest().Exec(`UPDATE booking_mails SET due_at = ? WHERE sent_at IS NULL`, time.Now().Add(-time.Second).Unix())
+ tick(t, h)
+ for i := 0; i < mailer.count(); i++ {
+ if strings.Contains(mailer.body(i), "confirm it here") {
+ t.Error("the drain sent a hold mail for a booking already confirmed")
+ }
+ }
+}
diff --git a/internal/calendartest/occurrence_strict_test.go b/internal/calendartest/occurrence_strict_test.go
index 74936eb..ada0eb2 100644
--- a/internal/calendartest/occurrence_strict_test.go
+++ b/internal/calendartest/occurrence_strict_test.go
@@ -4,7 +4,6 @@ import (
"context"
"errors"
"net/url"
- "strings"
"testing"
"time"
@@ -93,9 +92,26 @@ func TestTheUntilBoundHasADayOfSlack(t *testing.T) {
"title": {"Night shift"}, "start": {"2026-10-23T00:00"}, "end": {"2026-10-23T04:00"},
"kind": {"commitment"}, "status": {"confirmed"}, "repeat": {"daily"}, "repeat_end": {"2026-10-25"},
})
- body := getAs(t, h, session, "/day/2026-10-25").Body.String()
- if !strings.Contains(body, "Night shift") {
- t.Error("the final occurrence across the fold is missing from its own day")
+ // The final occurrence starts 00:00 IST on the 25th and, across the
+ // fold, runs FIVE elapsed hours to 04:00 GMT. Query only its fifth
+ // hour: a master-duration bound (four hours) would have excluded the
+ // master here; the day of slack keeps it.
+ host := app.PersonByAddressForTest(t, "someone@example.com")
+ dublin, _ := time.LoadLocation("Europe/Dublin")
+ from := time.Date(2026, 10, 25, 3, 30, 0, 0, dublin).Unix()
+ to := time.Date(2026, 10, 25, 4, 0, 0, 0, dublin).Unix()
+ events, err := app.StoreForTest().EventsInRange(context.Background(), host.ID, from, to)
+ if err != nil {
+ t.Fatal(err)
+ }
+ found := false
+ for _, e := range events {
+ if e.Title == "Night shift" {
+ found = true
+ }
+ }
+ if !found {
+ t.Error("the fifth hour of the final occurrence across the fold shows nothing — the UNTIL bound lacks its day of slack")
}
}
diff --git a/internal/calendartest/recur_engine_test.go b/internal/calendartest/recur_engine_test.go
index d42eb41..77a54c7 100644
--- a/internal/calendartest/recur_engine_test.go
+++ b/internal/calendartest/recur_engine_test.go
@@ -114,3 +114,31 @@ func TestExpandReportsWhenItRanOutOfSteps(t *testing.T) {
t.Error("an ordinary daily rule reported incomplete")
}
}
+
+// WKST decides which week a Sunday and a Monday share. Fortnightly on
+// Sunday and Monday from a Sunday: under the RFC's Monday default, that
+// Sunday's Monday is in the NEXT week, so the next eligible pair is 20
+// and 21 September, and 7 September is not an occurrence (code review
+// finding 2).
+func TestWKSTDecidesTheWeekAnIntervalCounts(t *testing.T) {
+ got := expandLocal(t, "2026-09-06T09:00", "2026-09-06T09:30", "UTC",
+ "FREQ=WEEKLY;INTERVAL=2;BYDAY=SU,MO;WKST=MO", "", "2026-09-01", "2026-09-30")
+ assertKeys(t, got, []string{"2026-09-06T09:00", "2026-09-14T09:00", "2026-09-20T09:00", "2026-09-28T09:00"})
+ // The same rule with Sunday weeks keeps Sunday and Monday together.
+ got = expandLocal(t, "2026-09-06T09:00", "2026-09-06T09:30", "UTC",
+ "FREQ=WEEKLY;INTERVAL=2;BYDAY=SU,MO;WKST=SU", "", "2026-09-01", "2026-09-30")
+ assertKeys(t, got, []string{"2026-09-06T09:00", "2026-09-07T09:00", "2026-09-20T09:00", "2026-09-21T09:00"})
+}
+
+// The output cap is not completion: a window that needs more than the
+// cap is reported incomplete, so a strict reader does not take a
+// truncated month as the whole of it.
+func TestTheOutputCapIsNotCompletion(t *testing.T) {
+ r, _ := calendar.ParseRule("FREQ=DAILY")
+ from := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC).Unix()
+ to := time.Date(2030, 1, 1, 0, 0, 0, 0, time.UTC).Unix()
+ _, complete := calendar.ExpandComplete("2026-01-01T09:00", "2026-01-01T09:30", "UTC", false, r, "", from, to)
+ if complete {
+ t.Error("a window past the output cap reported complete")
+ }
+}