Stop the availability form failing in silence
CI / Tests (PHP 8.1) (pull_request) Successful in 56s
CI / Tests (PHP 8.2) (pull_request) Successful in 46s
CI / No Debug Code (pull_request) Successful in 2s
CI / Coding Standards (pull_request) Successful in 3m3s
CI / PHPStan (pull_request) Successful in 2m51s
CI / Tests (PHP 8.3) (pull_request) Successful in 2m50s
CI / Build Plugin Zip (pull_request) Skipped
CI / Tests (PHP 8.1) (pull_request) Successful in 56s
CI / Tests (PHP 8.2) (pull_request) Successful in 46s
CI / No Debug Code (pull_request) Successful in 2s
CI / Coding Standards (pull_request) Successful in 3m3s
CI / PHPStan (pull_request) Successful in 2m51s
CI / Tests (PHP 8.3) (pull_request) Successful in 2m50s
CI / Build Plugin Zip (pull_request) Skipped
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
This commit is contained in:
@@ -8,6 +8,7 @@ use Mockery;
|
||||
use Unsupervised\Schedular\Availability\AvailabilityController;
|
||||
use Unsupervised\Schedular\Availability\AvailabilityRepository;
|
||||
use Unsupervised\Schedular\Availability\AvailabilitySlot;
|
||||
use Unsupervised\Schedular\Availability\WindowValidator;
|
||||
use Unsupervised\Schedular\Offering\OfferingRepository;
|
||||
use Unsupervised\Schedular\Tests\Unit\TestCase;
|
||||
|
||||
@@ -23,7 +24,7 @@ class AvailabilityControllerTest extends TestCase
|
||||
|
||||
$this->repository = Mockery::mock(AvailabilityRepository::class);
|
||||
$this->offerings = Mockery::mock(OfferingRepository::class);
|
||||
$this->controller = new AvailabilityController($this->repository, $this->offerings);
|
||||
$this->controller = new AvailabilityController($this->repository, $this->offerings, new WindowValidator($this->offerings));
|
||||
|
||||
$_POST = [];
|
||||
$_GET = [];
|
||||
@@ -39,6 +40,7 @@ class AvailabilityControllerTest extends TestCase
|
||||
Functions\when('absint')->alias(static fn ($value) => abs((int) $value));
|
||||
Functions\when('get_option')->justReturn(1);
|
||||
Functions\when('current_time')->justReturn('2026-07-06');
|
||||
Functions\when('selected')->justReturn('');
|
||||
Functions\when('admin_url')->justReturn('admin.php?page=us-availability');
|
||||
Functions\when('add_query_arg')->justReturn('admin.php?page=us-availability&usc_view=week');
|
||||
Functions\when('wp_nonce_field')->justReturn('');
|
||||
@@ -131,6 +133,159 @@ class AvailabilityControllerTest extends TestCase
|
||||
self::assertStringNotContainsString('name="slot_ids[]" form="usc-bulk-delete-form" value="6"', $html);
|
||||
}
|
||||
|
||||
/**
|
||||
* The reported bug: a 30-minute window submitted with the lesson length left
|
||||
* on 60 saved nothing and said nothing. It must now say why.
|
||||
*/
|
||||
public function testAddShowsAnErrorWhenTheWindowIsShorterThanTheLessonLength(): void
|
||||
{
|
||||
$_POST = [
|
||||
'usc_action' => 'add',
|
||||
'start_dt' => '2026-09-10T17:30',
|
||||
'end_dt' => '2026-09-10T18:00',
|
||||
'duration_minutes' => '60',
|
||||
];
|
||||
|
||||
$this->repository->shouldNotReceive('createFromWindow');
|
||||
$this->repository->shouldReceive('findByInstructor')->once()->with(3)->andReturn([]);
|
||||
|
||||
$html = $this->render();
|
||||
|
||||
self::assertStringContainsString('notice-error', $html);
|
||||
self::assertStringContainsString('60-minute lesson length', $html);
|
||||
self::assertStringNotContainsString('notice-success', $html);
|
||||
}
|
||||
|
||||
/** @return array<string, array{array<string, string>, string}> */
|
||||
public static function invalidWindows(): array
|
||||
{
|
||||
return [
|
||||
'unparseable start' => [['start_dt' => 'whenever', 'end_dt' => '2026-09-10T18:00'], 'valid start and end'],
|
||||
'end before start' => [['start_dt' => '2026-09-10T18:00', 'end_dt' => '2026-09-10T17:00'], 'after the start time'],
|
||||
'spans two days' => [['start_dt' => '2026-09-10T23:00', 'end_dt' => '2026-09-11T01:00'], 'same day'],
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* Each of these used to be a bare `return` — the page reloaded unchanged and
|
||||
* the instructor had no way to tell the save had failed.
|
||||
*
|
||||
* @dataProvider invalidWindows
|
||||
*
|
||||
* @param array<string, string> $fields
|
||||
*/
|
||||
public function testAddReportsEveryRejectedWindow(array $fields, string $expected): void
|
||||
{
|
||||
$_POST = array_merge([ 'usc_action' => 'add', 'duration_minutes' => '30' ], $fields);
|
||||
|
||||
$this->repository->shouldNotReceive('createFromWindow');
|
||||
$this->repository->shouldReceive('findByInstructor')->once()->with(3)->andReturn([]);
|
||||
|
||||
$html = $this->render();
|
||||
|
||||
self::assertStringContainsString('notice-error', $html);
|
||||
self::assertStringContainsString($expected, $html);
|
||||
}
|
||||
|
||||
public function testAddRejectsAnOfferingTheInstructorDoesNotOwn(): void
|
||||
{
|
||||
// The REST endpoint always checked this; the admin form never did, so a
|
||||
// crafted POST could tie a slot to another instructor's offering.
|
||||
$_POST = [
|
||||
'usc_action' => 'add',
|
||||
'start_dt' => '2026-09-10T17:00',
|
||||
'end_dt' => '2026-09-10T18:00',
|
||||
'duration_minutes' => '60',
|
||||
'offering_id' => '8',
|
||||
];
|
||||
|
||||
$this->offerings->shouldReceive('findById')->once()->with(8)->andReturn(null);
|
||||
$this->repository->shouldNotReceive('createFromWindow');
|
||||
$this->repository->shouldReceive('findByInstructor')->once()->with(3)->andReturn([]);
|
||||
|
||||
$html = $this->render();
|
||||
|
||||
self::assertStringContainsString('notice-error', $html);
|
||||
self::assertStringContainsString('not available', $html);
|
||||
}
|
||||
|
||||
public function testAddReportsHowManySlotsWereCreated(): void
|
||||
{
|
||||
$_POST = [
|
||||
'usc_action' => 'add',
|
||||
'start_dt' => '2026-09-10T17:00',
|
||||
'end_dt' => '2026-09-10T19:00',
|
||||
'duration_minutes' => '60',
|
||||
'recurrence' => 'weekly',
|
||||
'weeks' => '41',
|
||||
];
|
||||
|
||||
$this->repository->shouldReceive('createFromWindow')->once()
|
||||
->with(Mockery::type(AvailabilitySlot::class), true, 41)
|
||||
->andReturn(range(1, 82));
|
||||
$this->repository->shouldReceive('findByInstructor')->once()->with(3)->andReturn([]);
|
||||
|
||||
$html = $this->render();
|
||||
|
||||
self::assertStringContainsString('notice-success', $html);
|
||||
self::assertStringContainsString('Added 82 bookable slots.', $html);
|
||||
}
|
||||
|
||||
public function testAddReportsAFailedWrite(): void
|
||||
{
|
||||
// A valid window always splits into at least one slot, so an empty result
|
||||
// means the inserts themselves failed.
|
||||
$_POST = [
|
||||
'usc_action' => 'add',
|
||||
'start_dt' => '2026-09-10T17:00',
|
||||
'end_dt' => '2026-09-10T18:00',
|
||||
'duration_minutes' => '60',
|
||||
];
|
||||
|
||||
$this->repository->shouldReceive('createFromWindow')->once()->andReturn([]);
|
||||
$this->repository->shouldReceive('findByInstructor')->once()->with(3)->andReturn([]);
|
||||
|
||||
$html = $this->render();
|
||||
|
||||
self::assertStringContainsString('notice-error', $html);
|
||||
self::assertStringContainsString('could not be saved', $html);
|
||||
}
|
||||
|
||||
public function testSingleDeleteReportsAFailureToDelete(): void
|
||||
{
|
||||
$_POST = [ 'usc_action' => 'delete', 'slot_id' => '7' ];
|
||||
|
||||
$other = new AvailabilitySlot(instructorId: 4, startDt: '2026-07-08 09:00:00', endDt: '2026-07-08 10:00:00', id: 7);
|
||||
|
||||
$this->repository->shouldReceive('findById')->once()->with(7)->andReturn($other);
|
||||
$this->repository->shouldReceive('findByInstructor')->once()->with(3)->andReturn([]);
|
||||
|
||||
$html = $this->render();
|
||||
|
||||
self::assertStringContainsString('notice-error', $html);
|
||||
self::assertStringContainsString('could not be deleted', $html);
|
||||
}
|
||||
|
||||
public function testBulkDeleteReportsBothHalvesOfAPartialResult(): void
|
||||
{
|
||||
$_POST = [ 'usc_action' => 'bulk_delete', 'slot_ids' => ['5', '7'] ];
|
||||
|
||||
$owned = new AvailabilitySlot(instructorId: 3, startDt: '2026-07-08 09:00:00', endDt: '2026-07-08 10:00:00', id: 5);
|
||||
$booked = new AvailabilitySlot(instructorId: 3, startDt: '2026-07-08 10:00:00', endDt: '2026-07-08 11:00:00', isBooked: true, id: 7);
|
||||
|
||||
$this->repository->shouldReceive('findById')->once()->with(5)->andReturn($owned);
|
||||
$this->repository->shouldReceive('findById')->once()->with(7)->andReturn($booked);
|
||||
$this->repository->shouldReceive('delete')->once()->with(5)->andReturn(true);
|
||||
// The repository refuses a booked row, reporting it by returning false.
|
||||
$this->repository->shouldReceive('delete')->once()->with(7)->andReturn(false);
|
||||
$this->repository->shouldReceive('findByInstructor')->once()->with(3)->andReturn([]);
|
||||
|
||||
$html = $this->render();
|
||||
|
||||
self::assertStringContainsString('1 slot deleted.', $html);
|
||||
self::assertStringContainsString('1 slot could not be deleted', $html);
|
||||
}
|
||||
|
||||
private function render(): string
|
||||
{
|
||||
ob_start();
|
||||
|
||||
Reference in New Issue
Block a user