Stop the availability form failing in silence #137

Merged

thatguygriff merged 1 commits from fix/availability-silent-failures into main 2026-07-29 02:25:37 +00:00

Conversation 0 Commits 1 Files Changed 15

+878 -88

thatguygriff commented 2026-07-29 02:20:49 +00:00

Owner

Copy Link

Copy Source

Closes #130.

The reported bug

Adding availability for Thursday Sept 10, 5:30–6:00 PM, repeating for 41 weeks, did nothing at all — no slots, no error, just a page reload. Your hunch was right: the Lesson length select was still on its 60-minute default.

A window is persisted as consecutive lesson-length slots, so a 30-minute window at a 60-minute length splits into none. splitByDuration() returned [], createFromWindow() inserted nothing, and addSlot() threw away the result and re-rendered the page unchanged.

Why it only happened on the admin form

The REST endpoint already rejected that window with a 400. The two paths checked the same rules separately, and the admin copy had drifted — it was both laxer and mute:

Case	REST before	Admin form before
Window shorter than lesson length	400 invalid_window	silent return
Unreadable start/end	400 invalid_datetime	silent return
End at or before start	400 invalid_datetime	silent return
Window spans two days	400 invalid_window	silent return
Offering owned by another instructor	400 invalid_offering	not checked at all

That last row is the one worth a second look: a hand-crafted POST could tie a slot to another instructor's offering, and a slot carries its offering's price and payment routing into the booking.

Both callers now go through Availability\WindowValidator, which returns either the window ready to persist or a WP_Error. The endpoint returns that error as-is; the page renders get_error_message() as a notice. One implementation, so they cannot drift again.

Feedback on every path

handleFormAction() now returns a [$notice, $error] pair — the pattern Auth\StudentController already uses — and the template renders it. A successful add reports its count ("Added 82 bookable slots."), deletes report whether the row went, and a bulk delete reports both halves of a partial result.

Two failures that were invisible underneath

• wpdb::insert's result was ignored. insert_id still holds the previous statement's id after a failed write, so a failure returned a plausible id and looked like a success. Worse, in a weekly series that bogus id could become the recurrence_group, orphaning every later occurrence. insert() now returns 0 on failure, failed rows are skipped, and the group is taken from the first row that actually wrote.
• weeks was unbounded server-side. The input says max="52"; only that was stopping a posted weeks=10000. Now clamped in the repository, the single point of truth for the write.

Client side

New assets/js/availability-admin.js (enqueued by AdminMenu::enqueueAssets() on this screen only — the plugin had no admin script before) hides any lesson length longer than the entered window, falls back to the longest that still fits when the current pick is hidden, and disables submit when nothing fits. It deliberately leaves the choices alone while the window is incomplete rather than fighting someone mid-typing. This is what makes the original mistake hard to repeat, but it is a convenience only — the server validates regardless.

Tests

29 new assertions across three files:

• WindowValidatorTest (new, 13 tests) — every rejection, including the exact reported window at 60 minutes vs. the same window at 30; offering ownership; the datetime-local form the browser actually posts.
• AvailabilityControllerTest — the reported case end-to-end through the rendered page, each rejected window via a data provider, the foreign-offering POST, the created/deleted counts, and a failed write.
• AvailabilityRepositoryTest — insert returning 0 on failure, failed chunks omitted, the weeks clamp, and the recurrence group landing on the first row that survived.

composer test (684 tests, 1960 assertions), composer lint, composer cs all pass.

Note

AvailabilitySlot::DURATION_CHOICES is [30, 60], matching the hardcoded options this form has always had. Offerings can carry other lengths (45, say) — worth a follow-up if studios need them, but out of scope here.

🤖 Generated with Claude Code

Closes #130. ## The reported bug Adding availability for Thursday Sept 10, 5:30–6:00 PM, repeating for 41 weeks, did nothing at all — no slots, no error, just a page reload. Your hunch was right: the **Lesson length** select was still on its 60-minute default. A window is persisted as consecutive lesson-length slots, so a 30-minute window at a 60-minute length splits into none. `splitByDuration()` returned `[]`, `createFromWindow()` inserted nothing, and `addSlot()` threw away the result and re-rendered the page unchanged. ## Why it only happened on the admin form The REST endpoint already rejected that window with a `400`. The two paths checked the same rules **separately**, and the admin copy had drifted — it was both laxer and mute: | Case | REST before | Admin form before | |---|---|---| | Window shorter than lesson length | `400 invalid_window` | silent `return` | | Unreadable start/end | `400 invalid_datetime` | silent `return` | | End at or before start | `400 invalid_datetime` | silent `return` | | Window spans two days | `400 invalid_window` | silent `return` | | Offering owned by another instructor | `400 invalid_offering` | **not checked at all** | That last row is the one worth a second look: a hand-crafted POST could tie a slot to another instructor's offering, and a slot carries its offering's price and payment routing into the booking. Both callers now go through `Availability\WindowValidator`, which returns either the window ready to persist or a `WP_Error`. The endpoint returns that error as-is; the page renders `get_error_message()` as a notice. One implementation, so they cannot drift again. ## Feedback on every path `handleFormAction()` now returns a `[$notice, $error]` pair — the pattern `Auth\StudentController` already uses — and the template renders it. A successful add reports its count ("Added 82 bookable slots."), deletes report whether the row went, and a bulk delete reports both halves of a partial result. ## Two failures that were invisible underneath - **`wpdb::insert`'s result was ignored.** `insert_id` still holds the *previous* statement's id after a failed write, so a failure returned a plausible id and looked like a success. Worse, in a weekly series that bogus id could become the `recurrence_group`, orphaning every later occurrence. `insert()` now returns `0` on failure, failed rows are skipped, and the group is taken from the first row that actually wrote. - **`weeks` was unbounded server-side.** The input says `max="52"`; only that was stopping a posted `weeks=10000`. Now clamped in the repository, the single point of truth for the write. ## Client side New `assets/js/availability-admin.js` (enqueued by `AdminMenu::enqueueAssets()` on this screen only — the plugin had no admin script before) hides any lesson length longer than the entered window, falls back to the longest that still fits when the current pick is hidden, and disables submit when nothing fits. It deliberately leaves the choices alone while the window is incomplete rather than fighting someone mid-typing. This is what makes the original mistake hard to repeat, but it is a convenience only — the server validates regardless. ## Tests 29 new assertions across three files: - `WindowValidatorTest` (new, 13 tests) — every rejection, including the exact reported window at 60 minutes vs. the same window at 30; offering ownership; the `datetime-local` form the browser actually posts. - `AvailabilityControllerTest` — the reported case end-to-end through the rendered page, each rejected window via a data provider, the foreign-offering POST, the created/deleted counts, and a failed write. - `AvailabilityRepositoryTest` — insert returning `0` on failure, failed chunks omitted, the weeks clamp, and the recurrence group landing on the first row that survived. `composer test` (684 tests, 1960 assertions), `composer lint`, `composer cs` all pass. ## Note `AvailabilitySlot::DURATION_CHOICES` is `[30, 60]`, matching the hardcoded options this form has always had. Offerings can carry other lengths (45, say) — worth a follow-up if studios need them, but out of scope here. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

thatguygriff added 1 commit 2026-07-29 02:20:49 +00:00

Stop the availability form failing in silence ... 171b655bb8

Adding availability for 5:30-6:00 PM with the lesson length left on its
60-minute default saved nothing and said nothing. A window is stored as
consecutive lesson-length slots, so one that fits no lesson splits into
none: splitByDuration() returned [], createFromWindow() inserted
nothing, and addSlot() discarded the result and re-rendered the page
unchanged.

The REST endpoint already rejected that window with a 400. The admin
form checked the same rules separately, and its copy was both laxer and
mute — an unreadable date, an end before the start, and a two-day window
were bare `return`s, and it never checked offering ownership at all, so
a crafted POST could tie a slot to another instructor's offering and
inherit their price and payment routing.

Both callers now go through WindowValidator, which returns the window or
a WP_Error explaining the refusal. The endpoint returns that error as
is; the page renders its message as a notice. handleFormAction returns
a [notice, error] pair so deletes report themselves too, and a
successful add says how many slots it created.

Two failures could also go unnoticed underneath: wpdb::insert's result
was ignored, and insert_id still holds the previous statement's id after
a failed write, so a failure looked like a success — and could become
the recurrence group of a weekly series, orphaning every later
occurrence. weeks was unbounded server-side despite the form's max=52.

availability-admin.js narrows the lesson-length choices to those that
fit the window and blocks submission when none do, which is what makes
the original mistake hard to repeat. It is a convenience: the server
validates regardless.

Closes #130

thatguygriff merged commit bbc85d88f1 into main 2026-07-29 02:25:37 +00:00

thatguygriff deleted branch fix/availability-silent-failures 2026-07-29 02:25:37 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Something isn't working

enhancement

Extends an existing feature

feature

New user-facing capability

payments

Billing, Stripe, receipts, reporting

security

Security vulnerability or hardening

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

thatguygriff (James Griffin-Allwood)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Unsupervised/unsupervised-scheduler#137