Let the studio register the students who cannot register themselves

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

tests/Unit/Auth/RoleManagerTest.php

@@ -155,4 +155,48 @@ class RoleManagerTest extends TestCase
 
         (new RoleManager())->createRoles();
     }
+
+    /**
+     * The predicate every staff-side "register this person" path shares. It is
+     * deliberately the role and not `book_lesson`, so the two accounts that have
+     * that capability withheld — a guardian's child and an unapproved signup — are
+     * still people the studio can act for.
+     */
+    public function testIsStudentAcceptsAnyHolderOfTheStudentRole(): void
+    {
+        Functions\when('get_userdata')->justReturn($this->userWithRoles([RoleManager::STUDENT]));
+
+        self::assertTrue(RoleManager::isStudent(5));
+    }
+
+    public function testIsStudentRejectsSomeoneWhoIsNotAStudent(): void
+    {
+        Functions\when('get_userdata')->justReturn($this->userWithRoles([RoleManager::INSTRUCTOR]));
+
+        self::assertFalse(RoleManager::isStudent(5));
+    }
+
+    public function testIsStudentRejectsAnAccountThatNoLongerExists(): void
+    {
+        Functions\when('get_userdata')->justReturn(false);
+
+        self::assertFalse(RoleManager::isStudent(5));
+    }
+
+    public function testIsStudentRejectsNoOneChosenWithoutLookingAnyoneUp(): void
+    {
+        Functions\expect('get_userdata')->never();
+
+        self::assertFalse(RoleManager::isStudent(0));
+    }
+
+    /** @param list<string> $roles */
+    private function userWithRoles(array $roles): \WP_User
+    {
+        $user        = \Mockery::mock(\WP_User::class);
+        $user->ID    = 5;
+        $user->roles = $roles;
+
+        return $user;
+    }
 }

tests/Unit/Booking/AdminBookingTest.php

@@ -5,6 +5,7 @@ namespace Unsupervised\Schedular\Tests\Unit\Booking;
 
 use Brain\Monkey\Functions;
 use Mockery;
+use Unsupervised\Schedular\Auth\RoleManager;
 use Unsupervised\Schedular\Availability\AvailabilityRepository;
 use Unsupervised\Schedular\Availability\AvailabilitySlot;
 use Unsupervised\Schedular\Booking\AdminBooking;
@@ -36,10 +37,10 @@ class AdminBookingTest extends TestCase
         Functions\when('mysql2date')->alias(
             static fn (string $format, string $date): string => date($format, (int) strtotime($date))
         );
-        Functions\when('get_userdata')->justReturn(false);
+        // The picker offers holders of the student role, and that is what the guard
+        // accepts; it is exercised on its own below.
+        Functions\when('get_userdata')->alias(fn (int $id): \WP_User => $this->user($id, 'Jane Doe'));
         Functions\when('get_users')->justReturn([]);
-        // Everyone offered in the picker can book; the guard is exercised on its own.
-        Functions\when('user_can')->justReturn(true);
         // The staff member doing the booking; stamped on the lesson as booked_by.
         Functions\when('get_current_user_id')->justReturn(3);
 
@@ -175,9 +176,9 @@ class AdminBookingTest extends TestCase
         self::assertSame('slot_taken', $result->get_error_code());
     }
 
-    public function testSomeoneWhoCannotBookLessonsIsRefused(): void
+    public function testSomeoneWhoIsNotAStudentIsRefused(): void
     {
-        Functions\when('user_can')->justReturn(false);
+        Functions\when('get_userdata')->alias(fn (int $id): \WP_User => $this->user($id, 'Jane Doe', [RoleManager::INSTRUCTOR]));
         $this->availability->shouldReceive('findById')->never();
 
         $result = $this->admin->book(42, 7, 3, Lesson::RECURRENCE_SINGLE, false, '');
@@ -186,6 +187,69 @@ class AdminBookingTest extends TestCase
         self::assertSame('invalid_student', $result->get_error_code());
     }
 
+    public function testAnAccountThatNoLongerExistsIsRefused(): void
+    {
+        Functions\when('get_userdata')->justReturn(false);
+        $this->availability->shouldReceive('findById')->never();
+
+        $result = $this->admin->book(42, 7, 3, Lesson::RECURRENCE_SINGLE, false, '');
+
+        self::assertInstanceOf(\WP_Error::class, $result);
+        self::assertSame('invalid_student', $result->get_error_code());
+    }
+
+    public function testNoStudentChosenIsRefusedWithoutLookingAnyoneUp(): void
+    {
+        Functions\expect('get_userdata')->never();
+        $this->availability->shouldReceive('findById')->never();
+
+        $result = $this->admin->book(0, 7, 3, Lesson::RECURRENCE_SINGLE, false, '');
+
+        self::assertInstanceOf(\WP_Error::class, $result);
+        self::assertSame('invalid_student', $result->get_error_code());
+    }
+
+    /**
+     * A child holds the student role but never `book_lesson` — withheld so the
+     * account cannot book in its own name. The studio booking for them is the only
+     * route a child has to a lesson, so it must not be blocked by that.
+     */
+    public function testBooksForAGuardiansChildWhoCannotBookThemselves(): void
+    {
+        Functions\when('user_can')->justReturn(false);
+        $this->availability->shouldReceive('findById')->with(7)->andReturn($this->slot());
+        $this->offerings->shouldReceive('findById')->with(3)->andReturn($this->offering());
+        $this->availability->shouldReceive('claim')->once()->with(7)->andReturn(true);
+        $this->bookings->shouldReceive('insert')->once()->andReturn(100);
+        $this->payments->shouldReceive('createForRegistration')->once()->andReturn($this->pendingPayment());
+
+        $notice = $this->admin->book(42, 7, 3, Lesson::RECURRENCE_SINGLE, false, '');
+
+        self::assertIsString($notice);
+        self::assertStringContainsString('30 min piano', $notice);
+    }
+
+    /**
+     * Same for a self-signup the studio has not approved yet: the front desk can
+     * still get them onto the calendar while the paperwork catches up.
+     */
+    public function testBooksForAStudentStillAwaitingApproval(): void
+    {
+        Functions\when('user_can')->justReturn(false);
+        $this->availability->shouldReceive('findById')->with(7)->andReturn($this->slot());
+        $this->offerings->shouldReceive('findById')->with(3)->andReturn($this->offering());
+        $this->availability->shouldReceive('claim')->once()->with(7)->andReturn(true);
+        $this->bookings->shouldReceive('insert')->once()->with(Mockery::on(
+            static fn (Lesson $l): bool => 42 === $l->studentId
+        ))->andReturn(100);
+        $this->payments->shouldReceive('createForRegistration')->once()->andReturn($this->pendingPayment());
+
+        $notice = $this->admin->book(42, 7, 3, Lesson::RECURRENCE_SINGLE, false, '');
+
+        self::assertIsString($notice);
+        self::assertStringContainsString('pending payment', $notice);
+    }
+
     public function testATiedTimeCannotBeBookedAsADifferentLessonType(): void
     {
         $this->availability->shouldReceive('findById')->with(7)->andReturn($this->slot(offeringId: 3));
@@ -299,10 +363,12 @@ class AdminBookingTest extends TestCase
         );
     }
 
-    private function user(int $id, string $name): \WP_User
+    /** @param list<string> $roles */
+    private function user(int $id, string $name, array $roles = [RoleManager::STUDENT]): \WP_User
     {
         $user               = Mockery::mock(\WP_User::class);
         $user->ID           = $id;
+        $user->roles        = $roles;
         $user->first_name   = '';
         $user->last_name    = '';
         $user->nickname     = $name;

tests/Unit/Booking/LessonControllerTest.php

@@ -316,6 +316,54 @@ class LessonControllerTest extends TestCase
         self::assertStringContainsString('name="usc_action" value="book_for_student"', $html);
     }
 
+    /**
+     * A refusal used to clear all five fields, so one mistake meant retyping the
+     * whole form — and the panel is only ever reopened *because* something was
+     * refused.
+     */
+    public function testARefusedBookingComesBackWithEveryFieldStillFilledIn(): void
+    {
+        $this->postBooking();
+        $this->offerPanel();
+
+        $this->adminBooking->shouldReceive('book')->once()->andReturn(
+            new \WP_Error('slot_taken', 'That time has already been booked.')
+        );
+        $this->bookings->shouldReceive('findAllUpcoming')->once()->andReturn([]);
+
+        $html = $this->render();
+
+        self::assertStringContainsString('That time has already been booked.', $html);
+        // Attribute spacing in the template is not what is under test here.
+        $tags = (string) preg_replace('/\s+/', ' ', $html);
+
+        // The panel is reopened, showing the student, time and lesson type as posted.
+        self::assertStringContainsString(' open>', $html);
+        self::assertStringContainsString('value="42" selected=\'selected\'', $tags);
+        self::assertStringContainsString('value="7" selected=\'selected\'', $tags);
+        self::assertStringContainsString('value="3" selected=\'selected\'', $tags);
+        // Both ticks and the note survive too.
+        self::assertSame(2, substr_count($html, "checked='checked'"));
+        self::assertStringContainsString('value="Make-up lesson"', $html);
+    }
+
+    public function testASuccessfulBookingLeavesAnEmptyFormForTheNextOne(): void
+    {
+        $this->postBooking();
+        $this->offerPanel();
+
+        $this->adminBooking->shouldReceive('book')->once()->andReturn('Booked.');
+        $this->bookings->shouldReceive('findAllUpcoming')->once()->andReturn([]);
+
+        $html = $this->render();
+
+        self::assertStringContainsString('Booked.', $html);
+        // Nothing carried over, or the next booking silently inherits this one's.
+        self::assertStringNotContainsString("selected='selected'", $html);
+        self::assertStringNotContainsString("checked='checked'", $html);
+        self::assertStringContainsString('value=""', $html);
+    }
+
     public function testTheStudioSchedulerBooksAgainstAnyInstructorsTime(): void
     {
         $this->postBooking();
@@ -390,6 +438,16 @@ class LessonControllerTest extends TestCase
         Functions\when('absint')->alias(static fn ($v): int => abs((int) $v));
     }
 
+    /** The panel with one of each choice, so a re-selected value has somewhere to land. */
+    private function offerPanel(): void
+    {
+        $this->adminBooking->shouldReceive('formData')->once()->andReturn([
+            'students'  => [['id' => 42, 'name' => 'Ada Lovelace']],
+            'offerings' => [['id' => 3, 'label' => '30 min piano (30 min)']],
+            'slots'     => [['id' => 7, 'label' => 'Wed Jul 1, 2026 10:00 AM (30 min)', 'weekly' => false]],
+        ]);
+    }
+
     public function testAStaffBookedLessonOffersTheRecordIntakeForm(): void
     {
         $_GET['lesson_id'] = '1';

tests/Unit/GroupClass/GroupClassControllerTest.php

@@ -7,6 +7,7 @@ use Brain\Monkey\Functions;
 use Mockery;
 use Unsupervised\Schedular\Auth\InviteRepository;
 use Unsupervised\Schedular\Auth\RegistrationMailer;
+use Unsupervised\Schedular\Auth\RoleManager;
 use Unsupervised\Schedular\GroupClass\Enrollment;
 use Unsupervised\Schedular\GroupClass\EnrollmentRepository;
 use Unsupervised\Schedular\GroupClass\GroupAccessRepository;
@@ -87,7 +88,8 @@ class GroupClassControllerTest extends TestCase
     {
         [$first, $last] = array_pad(explode(' ', $full, 2), 2, '');
 
-        $user = Mockery::mock(\WP_User::class);
+        $user               = Mockery::mock(\WP_User::class);
+        $user->roles        = [RoleManager::STUDENT];
         $user->first_name   = $first;
         $user->last_name    = $last;
         $user->nickname     = $full;
@@ -406,6 +408,8 @@ class GroupClassControllerTest extends TestCase
         Functions\when('wp_unslash')->returnArg();
         Functions\when('sanitize_email')->returnArg();
         Functions\when('absint')->alias(static fn ($v) => abs((int) $v));
+        // Every posted id is vetted as a student before it is enrolled or granted.
+        Functions\when('get_userdata')->justReturn($this->userNamed('Ada Lovelace'));
 
         // Render tail: no classes/enrolments to draw so the assertion targets the notice.
         $this->offerings->shouldReceive('findAll')->with(3, Offering::KIND_GROUP_CLASS)->andReturn([]);
@@ -544,6 +548,64 @@ class GroupClassControllerTest extends TestCase
         $this->audit->shouldReceive('acceptances')->with($enrollment)->andReturn([]);
     }
 
+    /**
+     * The multi-select is built from the studio's students, but a posted id is just
+     * a number: it could name an instructor, an administrator, or an account
+     * deleted since the page was drawn. Enrolling one would put a non-student on
+     * the roster and raise a payment against them.
+     */
+    public function testAddDirectIgnoresAnIdThatIsNotAStudent(): void
+    {
+        $_POST = ['usc_action' => 'add_direct', 'offering_id' => 8, 'student_ids' => [5]];
+        $this->stubActionContext();
+
+        $instructor        = Mockery::mock(\WP_User::class);
+        $instructor->roles = [RoleManager::INSTRUCTOR];
+        Functions\when('get_userdata')->justReturn($instructor);
+
+        $this->offerings->shouldReceive('findById')->with(8)->andReturn($this->inviteOnlyOffering(100.0));
+        $this->enrollments->shouldReceive('insert')->never();
+        $this->paymentService->shouldReceive('createForRegistration')->never();
+        $this->access->shouldReceive('markEnrolled')->never();
+
+        $html = $this->renderInstructor();
+
+        self::assertStringContainsString('0 student(s) added to the class.', $html);
+    }
+
+    public function testAddDirectIgnoresAnAccountThatNoLongerExists(): void
+    {
+        $_POST = ['usc_action' => 'add_direct', 'offering_id' => 8, 'student_ids' => [5]];
+        $this->stubActionContext();
+        Functions\when('get_userdata')->justReturn(false);
+
+        $this->offerings->shouldReceive('findById')->with(8)->andReturn($this->inviteOnlyOffering(100.0));
+        $this->enrollments->shouldReceive('insert')->never();
+        $this->paymentService->shouldReceive('createForRegistration')->never();
+
+        $html = $this->renderInstructor();
+
+        self::assertStringContainsString('0 student(s) added to the class.', $html);
+    }
+
+    public function testGrantAccessIgnoresAnIdThatIsNotAStudent(): void
+    {
+        $_POST = ['usc_action' => 'grant_access', 'offering_id' => 8, 'student_ids' => [5]];
+        $this->stubActionContext();
+
+        $instructor        = Mockery::mock(\WP_User::class);
+        $instructor->roles = [RoleManager::INSTRUCTOR];
+        Functions\when('get_userdata')->justReturn($instructor);
+
+        $this->offerings->shouldReceive('findById')->with(8)->andReturn($this->inviteOnlyOffering());
+        $this->access->shouldReceive('insert')->never();
+        $this->mailer->shouldReceive('sendClassAccessGranted')->never();
+
+        $html = $this->renderInstructor();
+
+        self::assertStringContainsString('0 student(s) granted access.', $html);
+    }
+
     public function testGrantAccessCreatesGrantAndEmailsStudent(): void
     {
         $_POST = ['usc_action' => 'grant_access', 'offering_id' => 8, 'student_ids' => [5]];
@@ -554,7 +616,8 @@ class GroupClassControllerTest extends TestCase
         $this->access->shouldReceive('hasGrant')->with(8, 5)->andReturn(false);
         $this->access->shouldReceive('insert')->once()->andReturn(1);
 
-        $user = Mockery::mock(\WP_User::class);
+        $user             = Mockery::mock(\WP_User::class);
+        $user->roles      = [RoleManager::STUDENT];
         $user->user_email = '[email protected]';
         Functions\when('get_userdata')->justReturn($user);
         $this->mailer->shouldReceive('sendClassAccessGranted')->once()->with($user, 'Private Choir')->andReturn(true);