fix(enrollments): let a player be moved back into a group they have left

The transfer wizard died on the return leg of every shuffle. transfer()
always INSERTs the replacement enrolment, but nothing ever deletes the row
a player leaves — it stays, cancelled — and `enrollments` is unique on
(participant_id, training_group_id). So moving a child from 2015 A to
2015 B worked, and moving them back a month later raised a raw
UniqueConstraintViolationException that the wizard's catch-all reported to
the desk as "خطأ غير متوقع", with nothing to say what had gone wrong.

On the OC-Sport tenant 94 active players already have a former group that
is still open and still listed as a destination, so this was one click away
on any of them.

Revive the existing row rather than loosening the constraint: one player is
in one group once, which is what the unique key says and what the history
should read like. The lookup drops global scopes deliberately — the
constraint is academy-wide and knows nothing about the branch scope, so a
row this request could not see would still collide — and clears the
withdrawal that ended the previous spell, or the revived enrolment would
read as active and withdrawn at once and the renewal command would skip it.

Two neighbours fixed while here, both latent rather than reported:

- transfer() had no "one group per programme" guard, though enroll() has
  one. A transfer into a programme the player was already enrolled in left
  two active enrolments, which GenerateRenewalInvoices bills twice.
- transfer() left academy_id to BelongsToAcademy, which fills it from a
  container binding that only exists in a web request. Same NOT NULL
  failure enroll() was already fixed for; states it explicitly now.

Transferring into the group the player is already in is now refused
instead of cancelling and reviving the same row and reporting success.

Verified against a restored oc_sport tenant (the suite's own convention):
the new test reproduces the violation without the fix and passes with it,
and the full suite shows the same 6 pre-existing failures before and after
— no regressions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

app/Domain/Training/Services/EnrollmentService.php

@@ -334,10 +334,30 @@ public function transfer(Enrollment $enrollment, TrainingGroup $toGroup, User $a
                 throw new DomainException('يمكن نقل التسجيلات النشطة فقط');
             }
 
+            // Moving a player to where they already are would cancel the
+            // enrolment on the way out and revive the same row on the way in,
+            // reporting success for a move that never happened.
+            if ((int) $enrollment->training_group_id === (int) $toGroup->id) {
+                throw new DomainException('المشترك موجود بالفعل في هذه المجموعة');
+            }
+
             if ($toGroup->isFull()) {
                 throw new DomainException('المجموعة المنقول إليها ممتلئة');
             }
 
+            // One group per programme, the same rule enroll() enforces. Without
+            // it a transfer could leave a player holding two active enrolments
+            // in one programme, which the renewal command bills twice.
+            $alreadyInProgram = Enrollment::where('participant_id', $enrollment->participant_id)
+                ->where('training_program_id', $toGroup->training_program_id)
+                ->whereIn('status', ['pending', 'active'])
+                ->where('id', '!=', $enrollment->id)
+                ->exists();
+
+            if ($alreadyInProgram) {
+                throw new DomainException('المشترك مسجل بالفعل في مجموعة أخرى من نفس البرنامج');
+            }
+
             // Same branch only. This is a move between groups, not between
             // branches: the target comes from a picker, and a cross-branch move
             // would leave the member's participants.branch_id pointing at one
@@ -361,8 +381,13 @@ public function transfer(Enrollment $enrollment, TrainingGroup $toGroup, User $a
 
             $this->groupService->decrementCount($enrollment->group);
 
-            // Create new enrollment in target group (carry over billing date)
-            $newEnrollment = Enrollment::create([
+            // The enrolment in the target group (carry over billing date).
+            $attributes = [
+                // Stated rather than inherited, for the reason spelled out in
+                // enroll(): BelongsToAcademy fills this from a binding that only
+                // exists inside a web request, so a transfer driven from a
+                // command or a queued job died on a NOT NULL violation.
+                'academy_id' => $enrollment->academy_id,
                 'branch_id' => $toGroup->branch_id ?? $enrollment->branch_id,
                 'participant_id' => $enrollment->participant_id,
                 'training_group_id' => $toGroup->id,
@@ -377,7 +402,42 @@ public function transfer(Enrollment $enrollment, TrainingGroup $toGroup, User $a
                 'payment_status' => $enrollment->payment_status->value,
                 'sessions_attended' => $enrollment->sessions_attended,
                 'sessions_total' => $enrollment->sessions_total,
-            ]);
+            ];
+
+            // `enrollments` is unique on (participant_id, training_group_id) and
+            // nothing ever deletes the row a player left — it stays, cancelled.
+            // So a player returning to a group they have been in before cannot
+            // be given a second row: the INSERT dies on
+            // enrollments_participant_id_training_group_id_unique, which is what
+            // the transfer wizard surfaced as a bare "خطأ غير متوقع" on the
+            // return leg of every shuffle between parallel groups. Revive the
+            // row instead — one player is in one group once, which is what the
+            // constraint says and what the history should read like.
+            //
+            // Scopes are dropped on purpose: the constraint is academy-wide and
+            // knows nothing about the branch scope, so a row this request cannot
+            // see would still collide. participant + group is the constraint's
+            // own key, so the lookup cannot reach anyone else's enrolment.
+            $existing = Enrollment::withoutGlobalScopes()
+                ->where('participant_id', $enrollment->participant_id)
+                ->where('training_group_id', $toGroup->id)
+                ->first();
+
+            if ($existing) {
+                // The withdrawal that ended the previous spell has to go with
+                // it, or the revived enrolment reads as active and withdrawn at
+                // once — and the renewal command skips anything carrying a
+                // withdrawal date.
+                $existing->fill($attributes + [
+                    'withdrawal_date' => null,
+                    'withdrawal_reason' => null,
+                    'transferred_to_id' => null,
+                ])->save();
+
+                $newEnrollment = $existing->fresh();
+            } else {
+                $newEnrollment = Enrollment::create($attributes);
+            }
 
             $this->groupService->incrementCount($toGroup);
 

tests/Feature/EnrollmentTransferReturnsToFormerGroupTest.php

+<?php
+
+namespace Tests\Feature;
+
+use App\Domain\Shared\Exceptions\DomainException;
+use App\Domain\Training\Models\Enrollment;
+use App\Domain\Training\Models\TrainingGroup;
+use App\Domain\Training\Services\EnrollmentService;
+use App\Models\User;
+use Tests\TestCase;
+
+/**
+ * Moving a player back into a group they used to be in.
+ *
+ * A club shuffles children between the parallel groups of one age band all
+ * season — 2015 A to 2015 B and back again a month later. transfer() used to
+ * answer the return leg with a raw
+ * `enrollments_participant_id_training_group_id_unique` violation, because it
+ * always INSERTs the replacement enrolment while the cancelled one it made on
+ * the way out is still sitting in the table. The desk saw "خطأ غير متوقع" and
+ * had no way to know what it had done wrong.
+ *
+ * The row is unique per (participant, group) for good reason — one player is in
+ * a group once — so the fix is to revive that row rather than to loosen the
+ * constraint.
+ *
+ * Runs against a restored Postgres tenant; skips on the default SQLite suite:
+ *
+ *   DB_CONNECTION=pgsql DB_DATABASE=oc_sport_test \
+ *     ./vendor/bin/phpunit --filter EnrollmentTransferReturnsToFormerGroupTest
+ */
+class EnrollmentTransferReturnsToFormerGroupTest extends TestCase
+{
+    private EnrollmentService $service;
+
+    private User $actor;
+
+    private TrainingGroup $home;
+
+    private TrainingGroup $away;
+
+    private Enrollment $enrollment;
+
+    /** Everything this test touched, restored in teardown. */
+    private array $restore = [];
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        if (config('database.default') !== 'pgsql') {
+            $this->markTestSkipped('Needs a restored Postgres tenant; see the class comment.');
+        }
+
+        $academy = \App\Domain\Shared\Models\Academy::query()->first();
+
+        if (! $academy) {
+            $this->markTestSkipped('No academy in the restored tenant.');
+        }
+
+        app()->instance('current_academy', $academy);
+
+        $this->actor = User::withoutGlobalScopes()->firstOrFail();
+        $this->service = app(EnrollmentService::class);
+
+        // Two groups in the same branch with room in both — the shape a club
+        // actually shuffles players between.
+        $enrollment = Enrollment::withoutGlobalScopes()
+            ->where('status', 'active')
+            ->whereHas('group', fn ($q) => $q->whereColumn('current_count', '<', 'max_capacity'))
+            // Ordered: an UPDATE moves a row in the Postgres heap, so an
+            // unordered first() hands the two test methods different fixtures.
+            ->orderBy('id')
+            ->first();
+
+        if (! $enrollment) {
+            $this->markTestSkipped('No active enrolment with room in its group.');
+        }
+
+        $home = TrainingGroup::withoutGlobalScopes()->find($enrollment->training_group_id);
+
+        $away = TrainingGroup::withoutGlobalScopes()
+            ->where('id', '!=', $home->id)
+            ->where('branch_id', $home->branch_id)
+            ->where('training_program_id', '!=', $home->training_program_id)
+            ->whereIn('status', ['forming', 'active'])
+            ->whereColumn('current_count', '<', 'max_capacity')
+            ->orderBy('id')
+            ->first();
+
+        if (! $away) {
+            $this->markTestSkipped('No second group in the same branch with room.');
+        }
+
+        // Nothing may already occupy the seat we are about to move into and back
+        // out of, or teardown could not tell its rows from ours.
+        $priorAway = Enrollment::withoutGlobalScopes()
+            ->where('participant_id', $enrollment->participant_id)
+            ->where('training_group_id', $away->id)
+            ->exists();
+
+        if ($priorAway) {
+            $this->markTestSkipped('That player has already been in the destination group.');
+        }
+
+        $this->home = $home;
+        $this->away = $away;
+        $this->enrollment = $enrollment;
+
+        $this->restore['enrollment'] = $enrollment->only([
+            'status', 'withdrawal_date', 'withdrawal_reason', 'transferred_to_id',
+            'training_group_id', 'training_program_id', 'enrollment_date', 'start_date',
+            'end_date', 'payment_status', 'enrolled_by',
+        ]);
+        $this->restore['home_count'] = $home->current_count;
+        $this->restore['home_status'] = $home->status->value;
+        $this->restore['away_count'] = $away->current_count;
+        $this->restore['away_status'] = $away->status->value;
+    }
+
+    public function test_a_player_can_be_moved_back_into_a_group_they_have_left(): void
+    {
+        $participantId = $this->enrollment->participant_id;
+
+        // Out …
+        $moved = $this->service->transfer($this->enrollment, $this->away, $this->actor);
+
+        $this->assertSame($this->away->id, $moved->training_group_id);
+        $this->assertSame(
+            $this->away->training_program_id,
+            $moved->training_program_id,
+            'the subscription must follow the player to the destination group\'s programme'
+        );
+
+        // … and back. This is the leg that used to die on the unique constraint.
+        $returned = $this->service->transfer($moved->fresh(), $this->home->fresh(), $this->actor);
+
+        $this->assertSame($this->home->id, $returned->training_group_id);
+        $this->assertSame($this->home->training_program_id, $returned->training_program_id);
+        $this->assertSame('active', $returned->status->value);
+
+        // Revived, not duplicated: the constraint still holds one row per
+        // (player, group), and it is the row the player originally had.
+        $this->assertSame($this->enrollment->id, $returned->id);
+
+        $this->assertSame(1, Enrollment::withoutGlobalScopes()
+            ->where('participant_id', $participantId)
+            ->where('training_group_id', $this->home->id)
+            ->count());
+
+        // The outbound leg is closed off, not left looking active.
+        $this->assertSame('cancelled', $moved->fresh()->status->value);
+
+        // A revived enrolment must not still carry the withdrawal that ended it.
+        $this->assertNull($returned->withdrawal_date);
+        $this->assertNull($returned->transferred_to_id);
+    }
+
+    public function test_transferring_into_the_group_the_player_is_already_in_is_refused(): void
+    {
+        $this->expectException(DomainException::class);
+
+        $this->service->transfer($this->enrollment, $this->home, $this->actor);
+    }
+
+    protected function tearDown(): void
+    {
+        if (isset($this->enrollment)) {
+            Enrollment::withoutGlobalScopes()
+                ->where('participant_id', $this->enrollment->participant_id)
+                ->where('training_group_id', $this->away->id)
+                ->delete();
+
+            Enrollment::withoutGlobalScopes()
+                ->where('id', $this->enrollment->id)
+                ->update($this->restore['enrollment']);
+
+            TrainingGroup::withoutGlobalScopes()->where('id', $this->home->id)->update([
+                'current_count' => $this->restore['home_count'],
+                'status' => $this->restore['home_status'],
+            ]);
+
+            TrainingGroup::withoutGlobalScopes()->where('id', $this->away->id)->update([
+                'current_count' => $this->restore['away_count'],
+                'status' => $this->restore['away_status'],
+            ]);
+        }
+
+        parent::tearDown();
+    }
+}