diff --git a/README.md b/README.md index 3de20a5..944b680 100644 --- a/README.md +++ b/README.md @@ -190,6 +190,13 @@ A new incident is assigned to whoever holds today's schedule entry at the moment it opens (`GET /api/schedule/current`). If nobody is scheduled it opens unassigned. Reassign with `POST /api/incidents/{id}/assign`. +One person holds a given day, so `POST /api/schedule` refuses a date somebody +already has: taking a shift off the person expecting to be paged for it should +not be something a plain call does by accident. Pass `"replace": true` to take +them anyway. Either way the whole request is one transaction — a week where some +days are free and some are taken moves as a unit, and a failure leaves the rota +exactly as it was rather than with a hole in it. + ### Push notifications With `TERDUT_NTFY_URL` set, an incident that opens is pushed to the on-call @@ -467,7 +474,7 @@ unknown" rather than being rejected. | Method | Path | Description | |---|---|---| -| `POST` | `/api/schedule` | Assign user to dates `{"user_id", "dates":["YYYY-MM-DD",...]}` — all-or-nothing | +| `POST` | `/api/schedule` | Assign user to dates `{"user_id", "dates":["YYYY-MM-DD",...], "replace"}` — all-or-nothing | | `GET` | `/api/schedule` | List entries. Filters: `?from=YYYY-MM-DD`, `?to=YYYY-MM-DD` | | `GET` | `/api/schedule/current` | Today's on-call user (UTC), 404 if none | | `DELETE` | `/api/schedule/{id}` | Remove schedule entry | diff --git a/internal/api/api_test.go b/internal/api/api_test.go index 601c5db..1204898 100644 --- a/internal/api/api_test.go +++ b/internal/api/api_test.go @@ -310,6 +310,125 @@ func TestSchedule_MultiDateRollbackOnConflict(t *testing.T) { } } +// --------------------------------------------------------------------------- +// Schedule reassignment +// --------------------------------------------------------------------------- + +// addUser creates a second person to hand a shift to. The bootstrap user is +// admin, id 1. +func addUser(t *testing.T, s *ts, username string) { + t.Helper() + resp := s.req(t, http.MethodPost, "/api/users", + map[string]any{"username": username, "email": username + "@test.com"}) + defer resp.Body.Close() + if resp.StatusCode != http.StatusCreated { + t.Fatalf("create user returned %d", resp.StatusCode) + } +} + +// scheduleHolder reports who is on call for one date, or "" for nobody. +func scheduleHolder(t *testing.T, s *ts, date string) string { + t.Helper() + var entries []map[string]any + decode(t, s.req(t, http.MethodGet, "/api/schedule?from="+date+"&to="+date, nil), &entries) + if len(entries) == 0 { + return "" + } + return entries[0]["username"].(string) +} + +// Taking a day somebody else holds is possible, but only by asking for it. +func TestSchedule_ReplaceTakesAnAssignedDate(t *testing.T) { + s := newTS(t) + addUser(t, s, "alex") + + s.req(t, http.MethodPost, "/api/schedule", + map[string]any{"user_id": 1, "dates": []string{"2026-06-01"}}).Body.Close() + + resp := s.req(t, http.MethodPost, "/api/schedule", + map[string]any{"user_id": 2, "dates": []string{"2026-06-01"}, "replace": true}) + if resp.StatusCode != http.StatusCreated { + t.Fatalf("expected replace to succeed, got %d", resp.StatusCode) + } + resp.Body.Close() + + if got := scheduleHolder(t, s, "2026-06-01"); got != "alex" { + t.Errorf("expected alex to hold the day, got %q", got) + } + + // One row, not two: two entries for a date would mean two people believing + // they are on call for it. + var entries []map[string]any + decode(t, s.req(t, http.MethodGet, "/api/schedule?from=2026-06-01&to=2026-06-01", nil), &entries) + if len(entries) != 1 { + t.Errorf("expected exactly one entry for the date, got %d", len(entries)) + } +} + +// A week where only some days are taken is the case that was impossible before: +// the free days and the taken ones have to land together. +func TestSchedule_ReplaceMixedWeek(t *testing.T) { + s := newTS(t) + addUser(t, s, "alex") + + s.req(t, http.MethodPost, "/api/schedule", + map[string]any{"user_id": 1, "dates": []string{"2026-06-02", "2026-06-04"}}).Body.Close() + + week := []string{"2026-06-01", "2026-06-02", "2026-06-03", "2026-06-04", "2026-06-05"} + resp := s.req(t, http.MethodPost, "/api/schedule", + map[string]any{"user_id": 2, "dates": week, "replace": true}) + if resp.StatusCode != http.StatusCreated { + t.Fatalf("expected the mixed week to succeed, got %d", resp.StatusCode) + } + resp.Body.Close() + + for _, d := range week { + if got := scheduleHolder(t, s, d); got != "alex" { + t.Errorf("%s: expected alex, got %q", d, got) + } + } +} + +// Without replace the guard stands: nobody loses a shift by accident. +func TestSchedule_ReplaceDefaultsOff(t *testing.T) { + s := newTS(t) + addUser(t, s, "alex") + + s.req(t, http.MethodPost, "/api/schedule", + map[string]any{"user_id": 1, "dates": []string{"2026-06-01"}}).Body.Close() + + resp := s.req(t, http.MethodPost, "/api/schedule", + map[string]any{"user_id": 2, "dates": []string{"2026-06-01"}}) + if resp.StatusCode != http.StatusConflict { + t.Fatalf("expected 409 without replace, got %d", resp.StatusCode) + } + resp.Body.Close() + + if got := scheduleHolder(t, s, "2026-06-01"); got != "admin" { + t.Errorf("expected the original holder untouched, got %q", got) + } +} + +// Replace makes a repeated date idempotent rather than a conflict: the second +// pass clears what the first wrote and rewrites it. Worth pinning down, because +// the same input without replace is a 409. +func TestSchedule_ReplaceCollapsesRepeatedDates(t *testing.T) { + s := newTS(t) + + resp := s.req(t, http.MethodPost, "/api/schedule", + map[string]any{"user_id": 1, "dates": []string{"2026-06-01", "2026-06-01"}, "replace": true}) + if resp.StatusCode != http.StatusCreated { + t.Fatalf("expected a repeated date to be accepted under replace, got %d", resp.StatusCode) + } + resp.Body.Close() + + var entries []map[string]any + decode(t, s.req(t, http.MethodGet, "/api/schedule?from=2026-06-01&to=2026-06-01", nil), &entries) + if len(entries) != 1 { + t.Errorf("expected one entry for the repeated date, got %d", len(entries)) + } +} + // --------------------------------------------------------------------------- // Stats // --------------------------------------------------------------------------- diff --git a/internal/api/schedule.go b/internal/api/schedule.go index 1177aee..a19bca5 100644 --- a/internal/api/schedule.go +++ b/internal/api/schedule.go @@ -17,6 +17,12 @@ func handleCreateSchedule(db *sql.DB) http.HandlerFunc { var req struct { UserID int64 `json:"user_id"` Dates []string `json:"dates"` + + // Replace takes dates that somebody else already holds. It defaults + // to off so that the plain call cannot quietly move a shift off the + // person expecting to be paged for it — reassigning has to be asked + // for. + Replace bool `json:"replace"` } if err := decodeJSON(r, &req); err != nil { respond(w, http.StatusBadRequest, errResp("invalid request body")) @@ -44,7 +50,10 @@ func handleCreateSchedule(db *sql.DB) http.HandlerFunc { return } - // All-or-nothing: if any date already has an assignment, reject the whole request. + // All-or-nothing, in both directions: without replace, one taken date + // rejects the whole request; with it, either every date moves or none + // does. The rota must never be left with a hole where a shift used to + // be, so the delete and the insert share one transaction. tx, err := db.BeginTx(r.Context(), nil) if err != nil { respond(w, http.StatusInternalServerError, errResp("internal error")) @@ -53,10 +62,18 @@ func handleCreateSchedule(db *sql.DB) http.HandlerFunc { defer tx.Rollback() for _, d := range req.Dates { + if req.Replace { + if _, err := tx.ExecContext(r.Context(), + "DELETE FROM schedule_entries WHERE date = ?", d); err != nil { + respond(w, http.StatusInternalServerError, errResp("internal error")) + return + } + } if _, err := tx.ExecContext(r.Context(), "INSERT INTO schedule_entries (user_id, date) VALUES (?, ?)", req.UserID, d); err != nil { if strings.Contains(err.Error(), "UNIQUE constraint failed") { - respond(w, http.StatusConflict, errResp("date already assigned: "+d)) + respond(w, http.StatusConflict, + errResp("date already assigned: "+d+" (pass replace to take it)")) return } respond(w, http.StatusInternalServerError, errResp("internal error"))