From f56dbb46ac80d0cdafe17094b999f6a9d3f8bd3a Mon Sep 17 00:00:00 2001 From: Sam Rolfe Date: Thu, 15 Oct 2026 18:05:33 +1100 Subject: [PATCH] Phase 6 follow-up: today-and-future events, one write per school-wide event Two corrections found while preparing the first live run: - A scan that runs late in the day still records today's school events; the candidate window now starts at the local start of day instead of "now". - The Compass week calendar is school-wide, so one event is stored against each child. Write it once, with no child prefix in the title, and record the same Home Assistant event id on every child's item. A per-child event still keeps the child's name. Also report pending events as events, not rows, in `school calendar status`. Tests cover the window and the de-duplication. Co-Authored-By: Paperclip --- cmd/school/calendar.go | 9 +- docs/phase-6-calendar-notes.md | 20 ++- internal/calendar/sync.go | 221 +++++++++++++++++++++++---------- internal/calendar/sync_test.go | 72 +++++++++-- internal/web/server.go | 2 +- 5 files changed, 244 insertions(+), 80 deletions(-) diff --git a/cmd/school/calendar.go b/cmd/school/calendar.go index 35aaf83..738e4a8 100644 --- a/cmd/school/calendar.go +++ b/cmd/school/calendar.go @@ -125,11 +125,14 @@ func calendarStatus(args []string) { os.Exit(1) } now := time.Now().In(cfg.Location) - candidates, err := st.CalendarCandidates(ctx, nil, nil, now, 0) + candidates, err := st.CalendarCandidates(ctx, nil, nil, calendar.StartOfDay(now), 0) if err != nil { fmt.Fprintln(os.Stderr, "school calendar:", err) os.Exit(1) } + // Count events, not rows: a school-wide event is stored against each + // child but written once. + pendingEvents := len(calendar.GroupDuplicates(candidates)) if asJSON { out := struct { @@ -145,7 +148,7 @@ func calendarStatus(args []string) { BaseURL: haHost(cfg.Calendar.BaseURL), Entity: cfg.Calendar.EntityID, TokenSet: cfg.Calendar.Token != "", - Pending: len(candidates), + Pending: pendingEvents, ReloadedAt: now.Format(time.RFC3339), PushSummary: summary, } @@ -171,7 +174,7 @@ func calendarStatus(args []string) { } else { fmt.Printf("reachable yes\n") } - fmt.Printf("pending events %d (important, not in the calendar yet)\n", len(candidates)) + fmt.Printf("pending events %d (important, not in the calendar yet)\n", pendingEvents) fmt.Printf("pushes %d (%d failed)\n", summary.Total, summary.Failed) if summary.Last != nil { state := "ok" diff --git a/docs/phase-6-calendar-notes.md b/docs/phase-6-calendar-notes.md index 620ecc0..d4bfd64 100644 --- a/docs/phase-6-calendar-notes.md +++ b/docs/phase-6-calendar-notes.md @@ -17,7 +17,12 @@ only write is to the calendar (rule 7 — no approval needed). `5 - 11HLEGB5 - D8 - LHT` and an event as `End of Year Senior Music Concert - HALL (Senior Music Concert) - KSH, CFR`. The first part of a lesson is a period label (`5`, `C`, `RLE GROUP D`). -- Only **future** events are written: a past lesson is history, not a plan. +- Only **today and future** events are written: a scan that runs late in the + day still records today's school events, but yesterday is history. +- The Compass week calendar is **school-wide**, so the same event is stored + against each child. A school-wide event is written **once**, with no child + prefix in its title, and every child's item records the same event id. A + per-child event keeps the child's name in its title (`Harry — …`). - Which events qualify is visible before anything is written: ```bash @@ -116,6 +121,19 @@ internal/web/server.go the item action + the fail-loud calendar banner scripts/calendar-push.sh one-off container runner (no service restart) ``` +## Found while building (worth a decision later) + +The Compass week-calendar page looks **school-wide, not child-filtered**: Finn's +page carries `Year 11 Assembly` and `Tour to China photo` exactly as Harry's +does, while their lesson grids differ. Phase 6 handles that honestly — a shared +event is written once and both children's items record the same event id — but +the **per-child attribution** of a school-wide event is a Phase 3/4 data +question, not a calendar one. It matters for the Phase 7 per-boy brief. + +The week grid also stops at Thursday in the stored pages (Monday–Thursday have +lessons, Friday–Sunday do not), so a Friday scan may be the first to carry +future school events. + ## Verification ```bash diff --git a/internal/calendar/sync.go b/internal/calendar/sync.go index ca4fbf6..96612c3 100644 --- a/internal/calendar/sync.go +++ b/internal/calendar/sync.go @@ -38,10 +38,20 @@ type Options struct { DryRun bool // Now is the clock, so tests are deterministic. Now time.Time + // From overrides the start of the window. It defaults to the start of the + // local day, so a scan that runs late in the day still records today's + // school events instead of dropping them as "past". + From time.Time // Duration overrides DefaultDuration. Duration time.Duration } +// StartOfDay is midnight at the start of t's local day. +func StartOfDay(t time.Time) time.Time { + y, m, d := t.Date() + return time.Date(y, m, d, 0, 0, 0, 0, t.Location()) +} + // Created is one event that the run wrote (or would write on a dry run). type Created struct { ItemID int64 `json:"item_id"` @@ -91,77 +101,75 @@ func Sync(ctx context.Context, st *store.Store, c *Client, opts Options, log *sl if duration <= 0 { duration = DefaultDuration } + from := opts.From + if from.IsZero() { + from = StartOfDay(now) + } res := Result{Entity: c.Entity(), DryRun: opts.DryRun} - candidates, err := st.CalendarCandidates(ctx, opts.ChildID, opts.ItemID, now, opts.Limit) + candidates, err := st.CalendarCandidates(ctx, opts.ChildID, opts.ItemID, from, opts.Limit) if err != nil { return res, err } - res.Candidates = len(candidates) - if len(candidates) == 0 { + // The Compass week calendar is school-wide, so the same event is stored + // against each child. Write it once, and keep every child's item in step. + groups := GroupDuplicates(candidates) + res.Candidates = len(groups) + if len(groups) == 0 { return res, nil } // One window covers every candidate, so the marker scan is a single call. windowEnd := now.Add(24 * time.Hour) - for _, it := range candidates { - if it.DueAt != nil && it.DueAt.After(windowEnd) { - windowEnd = it.DueAt.Add(24 * time.Hour) + for _, g := range groups { + if due := g[0].DueAt; due != nil && due.After(windowEnd) { + windowEnd = due.Add(24 * time.Hour) } } - existing, err := c.Events(ctx, now.Add(-24*time.Hour), windowEnd) + existing, err := c.Events(ctx, from.Add(-24*time.Hour), windowEnd) if err != nil { // A failed read is not a licence to write blind: it would risk // duplicates, so record one loud failure and stop. msg := fmt.Sprintf("calendar: reading existing events failed: %v", err) - res.Failed = len(candidates) + res.Failed = len(groups) res.Errors = append(res.Errors, msg) - for _, it := range candidates { - if _, rerr := st.RecordCalendarPush(ctx, it.ID, c.Entity(), "", false, msg); rerr != nil { - log.Error("record calendar push", "item", it.ID, "err", rerr) - } + for _, g := range groups { + recordPush(ctx, st, c.Entity(), g, "", false, msg, log) } log.Error("calendar sync aborted", "entity", c.Entity(), "err", err) return res, nil } known := indexByMarker(existing) - for _, it := range candidates { - if it.DueAt == nil { + for _, g := range groups { + first := g[0] + if first.DueAt == nil { continue } - marker := Marker(it.ID) + children := strings.Join(groupChildren(g), ", ") + start := first.DueAt.In(time.Local).Format(time.RFC3339) - if ev, ok := known[marker]; ok { - // Adopt an event this run (or an earlier one) already wrote. + if uid, ok := adopt(known, g); ok { res.Skipped++ - uid := ev.UID - if uid == "" { - // No id from Home Assistant: keep the item out of the next - // candidate set by adopting the marker as its reference. - uid = marker - } if !opts.DryRun { - if err := st.MarkItemCalendarSynced(ctx, it.ID, uid, now); err != nil { - log.Error("mark calendar item", "item", it.ID, "err", err) - } - if _, err := st.RecordCalendarPush(ctx, it.ID, c.Entity(), uid, true, ""); err != nil { - log.Error("record calendar push", "item", it.ID, "err", err) + for _, it := range g { + if err := st.MarkItemCalendarSynced(ctx, it.ID, uid, now); err != nil { + log.Error("mark calendar item", "item", it.ID, "err", err) + } } + recordPush(ctx, st, c.Entity(), g, uid, true, "", log) } res.Events = append(res.Events, Created{ - ItemID: it.ID, Child: it.Child, Title: it.Title, - Start: it.DueAt.In(time.Local).Format(time.RFC3339), UID: uid, - Summary: "already in the calendar", + ItemID: first.ID, Child: children, Title: first.Title, + Start: start, UID: uid, Summary: "already in the calendar", }) continue } created := Created{ - ItemID: it.ID, Child: it.Child, Title: it.Title, - Start: it.DueAt.In(time.Local).Format(time.RFC3339), - DryRun: opts.DryRun, + ItemID: first.ID, Child: children, Title: first.Title, + Start: start, DryRun: opts.DryRun, } if opts.DryRun { res.Created++ @@ -170,33 +178,31 @@ func Sync(ctx context.Context, st *store.Store, c *Client, opts Options, log *sl } uid, err := c.CreateEvent(ctx, Event{ - Summary: eventSummary(it), - Description: eventDescription(it), - Start: it.DueAt, - End: ptrTime(it.DueAt.Add(duration)), + Summary: eventSummary(g), + Description: eventDescription(g), + Start: first.DueAt, + End: ptrTime(first.DueAt.Add(duration)), }) if err != nil { res.Failed++ - msg := fmt.Sprintf("item %d (%s %q): %v", it.ID, it.Child, it.Title, err) + msg := fmt.Sprintf("item %d (%s %q): %v", first.ID, children, first.Title, err) res.Errors = append(res.Errors, msg) - log.Error("calendar write failed", "item", it.ID, "child", it.Child, "title", it.Title, "entity", c.Entity(), "err", err) - if _, rerr := st.RecordCalendarPush(ctx, it.ID, c.Entity(), "", false, err.Error()); rerr != nil { - log.Error("record calendar push", "item", it.ID, "err", rerr) - } + log.Error("calendar write failed", "item", first.ID, "child", children, "title", first.Title, "entity", c.Entity(), "err", err) + recordPush(ctx, st, c.Entity(), g, "", false, err.Error(), log) continue } if uid == "" { // Home Assistant did not report an event id. The marker in the // description still makes the next run idempotent, but say so. log.Warn("home assistant returned no event id; relying on the description marker", - "item", it.ID, "child", it.Child, "title", it.Title, "entity", c.Entity()) + "item", first.ID, "child", children, "title", first.Title, "entity", c.Entity()) } - if err := st.MarkItemCalendarSynced(ctx, it.ID, uid, now); err != nil { - log.Error("mark calendar item", "item", it.ID, "err", err) - } - if _, err := st.RecordCalendarPush(ctx, it.ID, c.Entity(), uid, true, ""); err != nil { - log.Error("record calendar push", "item", it.ID, "err", err) + for _, it := range g { + if err := st.MarkItemCalendarSynced(ctx, it.ID, uid, now); err != nil { + log.Error("mark calendar item", "item", it.ID, "err", err) + } } + recordPush(ctx, st, c.Entity(), g, uid, true, "", log) res.Created++ created.UID = uid res.Events = append(res.Events, created) @@ -206,47 +212,130 @@ func Sync(ctx context.Context, st *store.Store, c *Client, opts Options, log *sl return res, nil } -// eventSummary is the calendar entry title. The child's name is prefixed, so a -// shared family calendar says who the event belongs to. -func eventSummary(it store.CalendarItem) string { - title := strings.TrimSpace(it.Title) - if it.Child != "" { - return it.Child + " — " + title +// recordPush appends one push-log row per item in a group, so the log always +// names every child whose event was written (FR-12). +func recordPush(ctx context.Context, st *store.Store, entity string, g []store.CalendarItem, uid string, ok bool, errMsg string, log *slog.Logger) { + for _, it := range g { + if _, err := st.RecordCalendarPush(ctx, it.ID, entity, uid, ok, errMsg); err != nil { + log.Error("record calendar push", "item", it.ID, "err", err) + } + } +} + +// GroupDuplicates collapses the same school event stored against more than one +// child into one calendar write. The key is the source, the type and the +// stable external reference. +func GroupDuplicates(items []store.CalendarItem) [][]store.CalendarItem { + index := make(map[string]int, len(items)) + var out [][]store.CalendarItem + for _, it := range items { + key := it.Source + "\x00" + it.Type + "\x00" + it.ExternalRef + if i, ok := index[key]; ok { + out[i] = append(out[i], it) + continue + } + index[key] = len(out) + out = append(out, []store.CalendarItem{it}) + } + return out +} + +// adopt reports the Home Assistant event id when any item in the group already +// has its event in the calendar, so a re-run never duplicates it. +func adopt(known map[string]Event, g []store.CalendarItem) (string, bool) { + for _, it := range g { + ev, ok := known[Marker(it.ID)] + if !ok { + continue + } + if ev.UID != "" { + return ev.UID, true + } + // No id from Home Assistant: keep the item out of the next candidate + // set by adopting the marker as its reference. + return Marker(it.ID), true + } + return "", false +} + +// groupChildren lists the children that share one event, sorted. +func groupChildren(g []store.CalendarItem) []string { + seen := make(map[string]bool, len(g)) + var out []string + for _, it := range g { + if it.Child == "" || seen[it.Child] { + continue + } + seen[it.Child] = true + out = append(out, it.Child) + } + sort.Strings(out) + return out +} + +// eventSummary is the calendar entry title. A per-child event carries the +// child's name; a school-wide event (stored against more than one child) +// carries just its title, because it is one event for the whole family. +func eventSummary(g []store.CalendarItem) string { + title := strings.TrimSpace(g[0].Title) + if children := groupChildren(g); len(children) == 1 { + return children[0] + " — " + title } return title } // eventDescription carries the school detail, the Compass link and the -// idempotency marker. The marker is what makes a re-run safe. -func eventDescription(it store.CalendarItem) string { +// idempotency markers. The markers are what make a re-run safe. +func eventDescription(g []store.CalendarItem) string { + first := g[0] var b strings.Builder - body := strings.TrimSpace(it.Body) - if body != "" && body != strings.TrimSpace(it.Title) { + body := strings.TrimSpace(first.Body) + if body != "" && body != strings.TrimSpace(first.Title) { b.WriteString(body) b.WriteString("\n") } - if it.URL != "" { - b.WriteString(it.URL) + if first.URL != "" { + b.WriteString(first.URL) b.WriteString("\n") } - if it.Subject != "" { - b.WriteString("Subject: " + it.Subject + "\n") + if first.Subject != "" { + b.WriteString("Subject: " + first.Subject + "\n") + } + if children := groupChildren(g); len(children) > 1 { + b.WriteString("Children: " + strings.Join(children, ", ") + "\n") + } + b.WriteString("\n") + for _, it := range g { + b.WriteString(ItemMarker(it.ID)) + b.WriteString("\n") } - b.WriteString("\n" + ItemMarker(it.ID)) return b.String() } -// indexByMarker maps a marker to the event that carries it. +// indexByMarker maps every marker in a description to the event that carries it. func indexByMarker(events []Event) map[string]Event { out := make(map[string]Event, len(events)) for _, ev := range events { - if m := findMarker(ev.Description); m != "" { + for _, m := range findMarkers(ev.Description) { out[m] = ev } } return out } +// findMarkers returns every compass-nhs marker in a text, in order. +func findMarkers(text string) []string { + var out []string + for { + m := findMarker(text) + if m == "" { + return out + } + out = append(out, m) + text = text[strings.Index(text, m)+len(m):] + } +} + // ptrTime returns a pointer to t, so an event end can hold a computed time. func ptrTime(t time.Time) *time.Time { return &t } diff --git a/internal/calendar/sync_test.go b/internal/calendar/sync_test.go index e76a164..45a5ff6 100644 --- a/internal/calendar/sync_test.go +++ b/internal/calendar/sync_test.go @@ -188,31 +188,35 @@ func TestSyncDryRunWritesNothing(t *testing.T) { } } -func TestSyncIgnoresRoutineLessonsAndPastEvents(t *testing.T) { +func TestSyncWindowIsTodayAndFuture(t *testing.T) { ctx := context.Background() st, childID := testStore(t) now := time.Now() + day := StartOfDay(now) + seedEvent(t, st, childID, "5 - 11HLEGB5 - D8 - LHT", now.Add(3*time.Hour), "normal", "lesson") - seedEvent(t, st, childID, "Year 11 Assembly - HALL", now.Add(-2*time.Hour), "high", "past") + seedEvent(t, st, childID, "Yesterday excursion - CITY", day.Add(-time.Hour), "high", "yesterday") + seedEvent(t, st, childID, "Year 11 Assembly - HALL", day.Add(time.Minute), "high", "today") + ha := newFakeHA(t, "calendar.school") c := testClient(t, ha.URL, "calendar.school") - candidates, err := st.CalendarCandidates(ctx, nil, nil, now, 0) + candidates, err := st.CalendarCandidates(ctx, nil, nil, day, 0) if err != nil { t.Fatalf("candidates: %v", err) } - // The store selects on importance; the lesson and the past event are - // excluded because the parser never marks a lesson high and the past event - // is behind `now`. - if len(candidates) != 0 { + // A routine lesson is never high, and yesterday is behind the window. The + // event earlier today stays a candidate: a scan that runs late in the day + // must still record today's school events. + if len(candidates) != 1 || candidates[0].Title != "Year 11 Assembly - HALL" { t.Fatalf("candidates = %+v", candidates) } res, err := Sync(ctx, st, c, Options{Now: now}, silentLogger()) if err != nil { t.Fatalf("Sync: %v", err) } - if res.Created != 0 || ha.created() != 0 { - t.Fatalf("result = %+v", res) + if res.Candidates != 1 || res.Created != 1 || ha.created() != 1 { + t.Fatalf("result = %+v (create calls %d)", res, ha.created()) } } @@ -236,3 +240,53 @@ func TestSyncReportsAReadFailureInsteadOfWritingBlind(t *testing.T) { t.Errorf("error = %q", res.Errors[0]) } } + +// TestSyncWritesASchoolWideEventOnce proves the shared Compass week calendar is +// not duplicated: one event stored against both children is written once, and +// both children's items record the same Home Assistant event id. +func TestSyncWritesASchoolWideEventOnce(t *testing.T) { + ctx := context.Background() + st, harryID := testStore(t) + if err := st.SeedChildren(ctx, "Finn"); err != nil { + t.Fatalf("seed finn: %v", err) + } + finn, err := st.ChildByName(ctx, "Finn") + if err != nil || finn == nil { + t.Fatalf("finn: %v %v", finn, err) + } + + due := time.Now().Add(4 * time.Hour).UTC().Truncate(time.Minute) + harryItem := seedEvent(t, st, harryID, "Year 11 Assembly - HALL - CMA", due, "high", "assembly") + finnItem := seedEvent(t, st, finn.ID, "Year 11 Assembly - HALL - CMA", due, "high", "assembly") + + ha := newFakeHA(t, "calendar.school") + c := testClient(t, ha.URL, "calendar.school") + + res, err := Sync(ctx, st, c, Options{Now: time.Now()}, silentLogger()) + if err != nil { + t.Fatalf("Sync: %v", err) + } + if res.Candidates != 1 || res.Created != 1 { + t.Fatalf("result = %+v", res) + } + if ha.created() != 1 { + t.Fatalf("school-wide event was written %d times", ha.created()) + } + // A school-wide event carries just its title, not a child's name. + if got := str(ha.lastCreate["summary"]); got != "Year 11 Assembly - HALL - CMA" { + t.Errorf("summary = %q", got) + } + uid := res.Events[0].UID + if uid == "" { + t.Fatal("event written without a Home Assistant id") + } + for _, id := range []int64{harryItem.ID, finnItem.ID} { + ci, err := st.CalendarItemByID(ctx, id) + if err != nil || ci == nil { + t.Fatalf("item %d: %v %v", id, ci, err) + } + if ci.HAEventUID != uid { + t.Errorf("item %d uid = %q, want %q", id, ci.HAEventUID, uid) + } + } +} diff --git a/internal/web/server.go b/internal/web/server.go index f08dd73..2b787e1 100644 --- a/internal/web/server.go +++ b/internal/web/server.go @@ -605,7 +605,7 @@ func (s *Server) itemCalendarView(it store.CalendarItem, now time.Time) calendar UID: it.HAEventUID, } v.Eligible = it.Type == "event" && it.Source == "compass" && it.Importance == "high" && - it.DueAt != nil && it.DueAt.After(now) + it.DueAt != nil && !it.DueAt.Before(calendar.StartOfDay(now)) if it.HAPushedAt != nil { v.PushedAt = it.HAPushedAt.In(s.cfg.Location).Format("Mon 02 Jan 2006 15:04") }