Commit 76a9a512 authored by Mahmoud Aglan's avatar Mahmoud Aglan

fix(facilities): let a facility record which sport it hosts

facility_activities.academy_id is NOT NULL and the pivot has no model to stamp
it, so $facility->activities()->sync([...]) threw a not-null violation. Both
write paths use exactly that call — FacilityService::create() and
FacilityForm::save() — so ticking a sport on a facility has never once worked on
any tenant. It surfaces only on Postgres, and the default sqlite suite has no
such table, which is why nothing caught it.

The consequence was quiet rather than loud: 2026_08_30_000003 treats a facility
with no rows here as one that hosts anything, so instead of an error the
schedule builder just kept offering every group in the academy for every pitch —
the thing that migration was written to stop. A football pitch still lists
swimming groups.

Fixed on the relation with withPivotValue(), which both fills the column on
write and scopes it on read, so the edit path is covered too without either
caller having to remember.

Also null-coalesces the two optional keys in validateNoOverlap(). Neither is
passed by createDefaultLayout(), so every facility ever created raised
"Undefined array key" — a warning in the app, a hard error under PHPUnit. The
deeper issue is left alone on purpose and noted in place: a null day compiles to
`effective_day_of_week = NULL`, which SQL never satisfies, so an all-days layout
is currently exempt from overlap detection. Every tenant already has an
auto-created all-days default layout, so making it collide would start refusing
temporal layouts that can be added today — a behaviour change that wants its own
decision, not a drive-by.
Co-Authored-By: 's avatarClaude Opus 5 (1M context) <noreply@anthropic.com>
parent c1c1e9bc
......@@ -105,12 +105,17 @@ public function reservations(): HasMany
*/
public function activities(): \Illuminate\Database\Eloquent\Relations\BelongsToMany
{
// facility_activities.academy_id is NOT NULL and the pivot has no model to
// stamp it, so a plain sync() threw on every tenant — which is why no
// facility anywhere has ever had a sport recorded against it, and why the
// schedule builder still offers swimming groups on a football pitch.
// withPivotValue both fills it on write and scopes it on read.
return $this->belongsToMany(
\App\Domain\Training\Models\Activity::class,
'facility_activities',
'facility_id',
'activity_id'
)->withTimestamps();
)->withPivotValue('academy_id', $this->academy_id)->withTimestamps();
}
// No scopeForBranch() here. A method on the class wins over the one the
......
......@@ -222,12 +222,21 @@ private function validateNoOverlap(array $data, ?int $excludeId = null): void
$query->where('id', '!=', $excludeId);
}
// Both keys are optional — createDefaultLayout() passes neither, so this
// raised "Undefined array key" on every facility ever created, which is a
// warning in the app and a hard error under PHPUnit.
//
// NOTE: a null day here still compiles to `effective_day_of_week = NULL`,
// which SQL never satisfies, so an all-days layout is currently exempt
// from overlap detection. Left as-is deliberately: every tenant already
// has an auto-created all-days default layout, and making it collide
// would start refusing temporal layouts that can be added today.
if ($data['is_recurring'] ?? true) {
$query->where('is_recurring', true)
->where('effective_day_of_week', $data['effective_day_of_week']);
->where('effective_day_of_week', $data['effective_day_of_week'] ?? null);
} else {
$query->where('is_recurring', false)
->where('effective_date', $data['effective_date']);
->where('effective_date', $data['effective_date'] ?? null);
}
if ($query->exists()) {
......
<?php
namespace Tests\Feature;
use App\Domain\Facility\Models\Facility;
use App\Domain\Facility\Services\FacilityService;
use App\Domain\Shared\Models\Academy;
use App\Domain\Training\Models\Activity;
use App\Models\User;
use Illuminate\Support\Facades\DB;
use Tests\TestCase;
/**
* Recording which sports a facility can host.
*
* `facility_activities.academy_id` is NOT NULL and the pivot has no model to
* stamp it, so `$facility->activities()->sync([...])` — which is what BOTH write
* paths use, FacilityService::create() and FacilityForm::save() — threw a
* not-null violation on every tenant. Ticking a sport on a facility had never
* once worked, which is why no facility anywhere has a row here and why the
* schedule builder still offers every group in the academy for every pitch:
* `2026_08_30_000003` treats "no rows" as "hosts anything".
*
* Nothing pinned it because the failure needs Postgres to show up at all — the
* default sqlite suite has no such table.
*
* DB_CONNECTION=pgsql DB_HOST=127.0.0.1 DB_DATABASE=oc_sport_test \
* ./vendor/bin/phpunit --filter FacilityActivityPivotTest
*/
class FacilityActivityPivotTest extends TestCase
{
private ?Academy $academy = null;
protected function setUp(): void
{
parent::setUp();
if (config('database.default') !== 'pgsql') {
$this->markTestSkipped('Needs a restored Postgres tenant.');
}
if (! $this->academy = Academy::first()) {
$this->markTestSkipped('No academy in the restored tenant.');
}
app()->instance('current_academy', $this->academy);
DB::beginTransaction();
}
protected function tearDown(): void
{
DB::rollBack();
parent::tearDown();
}
public function test_a_facility_can_record_the_sport_it_hosts(): void
{
$activity = Activity::first();
$actor = User::first();
if (! $activity || ! $actor) {
$this->markTestSkipped('Tenant has no activity or user to build on.');
}
$facility = app(FacilityService::class)->create([
'academy_id' => $this->academy->id,
'branch_id' => $this->branchId(),
'code' => 'PIVOTTEST', 'name' => 'Pivot probe', 'name_ar' => 'اختبار',
'type' => 'field', 'status' => 'active',
'operating_start' => '09:00', 'operating_end' => '23:00',
], $actor, [$activity->id], ['rows' => 1, 'columns' => 1]);
$pivot = DB::table('facility_activities')->where('facility_id', $facility->id)->first();
$this->assertNotNull($pivot, 'sync() wrote no pivot row');
$this->assertSame(
(int) $this->academy->id,
(int) $pivot->academy_id,
'the pivot must carry the academy — the column is NOT NULL',
);
$this->assertSame(1, $facility->activities()->count(), 'the relation must read its own row back');
}
public function test_resyncing_does_not_orphan_the_academy(): void
{
$activities = Activity::limit(2)->get();
$actor = User::first();
if ($activities->count() < 1 || ! $actor) {
$this->markTestSkipped('Tenant has no activity or user to build on.');
}
$facility = app(FacilityService::class)->create([
'academy_id' => $this->academy->id,
'branch_id' => $this->branchId(),
'code' => 'PIVOTTEST2', 'name' => 'Pivot probe 2', 'name_ar' => 'اختبار ٢',
'type' => 'field', 'status' => 'active',
], $actor, [$activities->first()->id], []);
// The edit path — FacilityForm::save() calls sync() directly on the model.
$facility->activities()->sync($activities->pluck('id')->all());
$rows = DB::table('facility_activities')->where('facility_id', $facility->id)->get();
$this->assertCount($activities->count(), $rows);
foreach ($rows as $row) {
$this->assertSame((int) $this->academy->id, (int) $row->academy_id);
}
}
private function branchId(): int
{
return (int) DB::table('branches')
->where('academy_id', $this->academy->id)
->whereNull('deleted_at')
->orderBy('id')
->value('id');
}
}
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment