Let the studio register the students who cannot register themselves #186

2026-08-24T21:43:29Z

thatguygriff commented

2026-08-24 21:43:29 +00:00

Closes #185.

Three defects in the staff-registers-someone-else paths, found when booking for a student errored with "Choose a student to book for." and cleared the form.

The root cause

AdminBooking built its student picker from the us_student role and vetted the submission with the book_lesson capability. Guardian\ChildLoginGate and Auth\RegistrationLoginGate strip that capability from accounts that keep the role, so the panel offered every guardian-managed child and every unapproved signup, then refused them — with a message saying no student had been chosen, when one had.

Withholding book_lesson is what stops those accounts registering in their own name: a child's account is never signed in to, an unapproved signup is waiting on the studio. Neither restriction was ever meant to stop staff acting for them — that is what the panel exists to do, and for a child it is the only route to a lesson besides their guardian. Neither gate is weakened here; only the studio-acting-for-them path changed.

What changed

1. One shared predicate. New RoleManager::isStudent() — the student role, not the capability — now used by both the guard and (implicitly) the set the pickers are built from. The docblock records why, since the old studentOptions() docblock claimed the exact opposite of the code ("since both hold book_lesson") and is likely how this got in.

2. Group enrolment vets its ids. GroupClassController::postedStudentIds() fed addDirect() and grantAccess() raw ints with no eligibility check at all — an instructor, an administrator, or an account deleted since the page was drawn would be enrolled, and for a priced class billed. Now filtered through the same predicate, so children and unapproved signups stay enrollable while non-students are skipped. Ineligible ids are skipped silently, matching how these controls already skip duplicate enrolments; the %d student(s) added count reports what actually went through.

3. A refused booking keeps its fields. templates/admin/lessons.php had no repopulation, so any refusal lost all five fields — on the one path that is only ever reached because something was refused. Both the booking and the redisplay now read the form through a single LessonController::submittedBooking(), so they cannot drift on a field name. A successful booking still renders an empty form, so the next one does not inherit the last.

Notes for review

The $_POST re-read for redisplay happens only after handleFormAction() has verified the nonce, and only feeds escaped output — noted on the phpcs suppression rather than left bare.
Fix 2 is the one with a security edge (a stale page or tampered POST could bill a non-student), so it may be worth a closer look than the other two.
No Schema.php change, so no version bump.

Tests

composer test — 966 passing (2795 assertions), 9 new:

RoleManagerTest — the predicate: student role, non-student, deleted account, and id 0 short-circuiting without a lookup
AdminBookingTest — booking for a child and for an unapproved student, both with user_can stubbed false so they fail against the old guard; plus the three refusals that still stand
LessonControllerTest — a refusal keeps all five fields, a success clears them
GroupClassControllerTest — a non-student id and a deleted account are skipped by both add-direct and grant-access

The clearing test and all three group tests were mutation-checked (reverting the source makes them fail), since assert-absence tests can pass vacuously.

composer lint — no errors. composer cs — clean.

Docs updated in docs/features/lesson-booking.md and docs/features/group-classes.md; CHANGELOG.md has three entries under ## [1.5.4].

Closes #185. Three defects in the staff-registers-someone-else paths, found when booking for a student errored with **"Choose a student to book for."** and cleared the form. ## The root cause `AdminBooking` built its student picker from the `us_student` **role** and vetted the submission with the `book_lesson` **capability**. `Guardian\ChildLoginGate` and `Auth\RegistrationLoginGate` strip that capability from accounts that keep the role, so the panel offered every guardian-managed child and every unapproved signup, then refused them — with a message saying no student had been chosen, when one had. Withholding `book_lesson` is what stops those accounts registering *in their own name*: a child's account is never signed in to, an unapproved signup is waiting on the studio. Neither restriction was ever meant to stop staff acting **for** them — that is what the panel exists to do, and for a child it is the only route to a lesson besides their guardian. Neither gate is weakened here; only the studio-acting-for-them path changed. ## What changed **1. One shared predicate.** New `RoleManager::isStudent()` — the student role, not the capability — now used by both the guard and (implicitly) the set the pickers are built from. The docblock records why, since the old `studentOptions()` docblock claimed the exact opposite of the code (*"since both hold `book_lesson`"*) and is likely how this got in. **2. Group enrolment vets its ids.** `GroupClassController::postedStudentIds()` fed `addDirect()` and `grantAccess()` raw ints with no eligibility check at all — an instructor, an administrator, or an account deleted since the page was drawn would be enrolled, and for a priced class billed. Now filtered through the same predicate, so children and unapproved signups stay enrollable while non-students are skipped. Ineligible ids are skipped silently, matching how these controls already skip duplicate enrolments; the `%d student(s) added` count reports what actually went through. **3. A refused booking keeps its fields.** `templates/admin/lessons.php` had no repopulation, so any refusal lost all five fields — on the one path that is only ever reached *because* something was refused. Both the booking and the redisplay now read the form through a single `LessonController::submittedBooking()`, so they cannot drift on a field name. A successful booking still renders an empty form, so the next one does not inherit the last. ## Notes for review - The `$_POST` re-read for redisplay happens only after `handleFormAction()` has verified the nonce, and only feeds escaped output — noted on the phpcs suppression rather than left bare. - Fix 2 is the one with a security edge (a stale page or tampered POST could bill a non-student), so it may be worth a closer look than the other two. - No `Schema.php` change, so no version bump. ## Tests `composer test` — **966 passing** (2795 assertions), 9 new: - `RoleManagerTest` — the predicate: student role, non-student, deleted account, and id `0` short-circuiting without a lookup - `AdminBookingTest` — booking for a child and for an unapproved student, both with `user_can` stubbed `false` so they fail against the old guard; plus the three refusals that still stand - `LessonControllerTest` — a refusal keeps all five fields, a success clears them - `GroupClassControllerTest` — a non-student id and a deleted account are skipped by both add-direct and grant-access The clearing test and all three group tests were mutation-checked (reverting the source makes them fail), since assert-absence tests can pass vacuously. `composer lint` — no errors. `composer cs` — clean. Docs updated in `docs/features/lesson-booking.md` and `docs/features/group-classes.md`; `CHANGELOG.md` has three entries under `## [1.5.4]`.

thatguygriff added 1 commit 2026-08-24 21:43:29 +00:00

Let the studio register the students who cannot register themselves

CI / No Debug Code (pull_request) Successful in 4s

Details

CI / Tests (PHP 8.2) (pull_request) Successful in 50s

Details

CI / Tests (PHP 8.1) (pull_request) Successful in 1m3s

Details

CI / Tests (PHP 8.5) (pull_request) Successful in 2m48s

Details

CI / Tests (PHP 8.3) (pull_request) Successful in 3m24s

Details

CI / Coding Standards & Static Analysis (pull_request) Successful in 8m21s

Details

CI / Build Plugin Zip (pull_request) Skipped

Details

5ce42f0003

The Book a lesson for a student panel built its picker from the us_student
role but vetted the submission with the book_lesson capability. ChildLoginGate
and RegistrationLoginGate withhold that capability from accounts that keep the
role, so the panel offered every guardian-managed child and every unapproved
signup and then refused them — with a message claiming no student had been
chosen, and a form cleared of all five fields.

Withholding book_lesson stops those accounts registering in their own name. It
was never meant to stop the studio acting for them, which is what the panel is
for, and for a child is the only route to a lesson besides their guardian.

Guard the student role instead, via a new RoleManager::isStudent() shared with
every picker and guard on the staff side so the two cannot drift apart again.
Group enrolment gets the same predicate: addDirect() and grantAccess() vetted
their posted ids not at all, and would enrol an instructor, an administrator,
or an account deleted since the page was drawn — raising a real payment against
them for a priced class.

Keep a refused booking's fields as submitted, reading the form through one
LessonController::submittedBooking() so what gets booked and what is shown
again cannot disagree about a field name. A booking that succeeds still leaves
an empty form, so the next one does not inherit it.

Closes #185

Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01XunBYk2sFEc1oL14sUiuBU

thatguygriff merged commit 7278309bf5 into main

2026-08-24 22:03:11 +00:00

thatguygriff deleted branch fix/185-book-for-child-and-vet-group-enrolment

2026-08-24 22:03:12 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Unsupervised/unsupervised-scheduler#186