Skip to content

Commit 808ff42

Browse files
committed
Address review feedback for spectator mode
- Remove unused SPECTATE_ROOM voter code and related unit tests - Add filter_var validation for spectate parameter to ensure boolean type - Fix incomplete test by renaming and adding proper test coverage for existing player rejoin scenario
1 parent a34aebd commit 808ff42

File tree

4 files changed

+44
-166
lines changed

4 files changed

+44
-166
lines changed

src/Api/Controller/UserController.php

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ public function patchUser(Request $request): JsonResponse
147147
// Handle room change
148148
if (array_key_exists('room', $data)) {
149149
$newRoomId = $data['room'];
150-
$joinAsSpectator = $data['spectate'] ?? false;
150+
$joinAsSpectator = filter_var($data['spectate'] ?? false, FILTER_VALIDATE_BOOLEAN);
151151

152152
if ($newRoomId !== null) {
153153
$newRoom = $this->roomRepository->findOneBy(['id' => $newRoomId]);

src/Infrastructure/Security/Voters/RoomVoter.php

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
namespace App\Infrastructure\Security\Voters;
66

77
use App\Domain\Player\Entity\Player;
8-
use App\Domain\Player\Enum\PlayerStatus;
98
use App\Domain\Player\PlayerRepository;
109
use App\Domain\Room\Entity\Room;
1110
use App\Domain\User\Entity\User;
@@ -17,7 +16,6 @@ class RoomVoter extends Voter
1716
public const string EDIT_ROOM = 'edit_room';
1817
public const string VIEW_ROOM = 'view_room';
1918
public const string CREATE_ROOM = 'create_room';
20-
public const string SPECTATE_ROOM = 'spectate_room';
2119

2220
public function __construct(
2321
private readonly PlayerRepository $playerRepository,
@@ -26,7 +24,7 @@ public function __construct(
2624

2725
protected function supports(string $attribute, mixed $subject): bool
2826
{
29-
if (!in_array($attribute, [self::EDIT_ROOM, self::VIEW_ROOM, self::CREATE_ROOM, self::SPECTATE_ROOM], true)) {
27+
if (!in_array($attribute, [self::EDIT_ROOM, self::VIEW_ROOM, self::CREATE_ROOM], true)) {
3028
return false;
3129
}
3230

@@ -51,7 +49,6 @@ protected function voteOnAttribute(string $attribute, mixed $subject, TokenInter
5149
self::VIEW_ROOM => $currentPlayer && $this->canView($subject, $currentPlayer),
5250
self::EDIT_ROOM => $currentPlayer && $this->canEdit($subject, $currentPlayer),
5351
self::CREATE_ROOM => !$currentPlayer && $this->canCreate($currentPlayer),
54-
self::SPECTATE_ROOM => $currentPlayer && $this->canSpectate($subject, $currentPlayer),
5552
default => throw new \LogicException('This code should not be reached'),
5653
};
5754
}
@@ -71,12 +68,4 @@ private function canCreate(?Player $player = null): bool
7168
{
7269
return !$player?->getRoom();
7370
}
74-
75-
private function canSpectate(mixed $room, Player $player): bool
76-
{
77-
return $room instanceof Room
78-
&& $room->isAllowSpectators()
79-
&& $player->getRoom() === $room
80-
&& $player->getStatus() === PlayerStatus::SPECTATING;
81-
}
8271
}

tests/Api/SpectatorModeCest.php

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ public function testGameMasterSeesEverythingDespiteSpectatingStatus(ApiTester $I
215215
Assert::assertArrayHasKey('missions', $response);
216216
}
217217

218-
public function testSpectatorCannotJoinRegularWhenAlreadySpectating(ApiTester $I): void
218+
public function testSpectatorStatusPersistsAfterJoining(ApiTester $I): void
219219
{
220220
// Create admin and room with spectators allowed
221221
$I->createAdminAndUpdateHeaders($I);
@@ -236,5 +236,46 @@ public function testSpectatorCannotJoinRegularWhenAlreadySpectating(ApiTester $I
236236
'name' => 'Spectator',
237237
'status' => PlayerStatus::SPECTATING,
238238
]);
239+
240+
// Verify status persists when fetching user info
241+
$I->sendGetAsJson('/user/me');
242+
$I->seeResponseContainsJson([
243+
'currentPlayer' => [
244+
'status' => PlayerStatus::SPECTATING->value,
245+
],
246+
]);
247+
}
248+
249+
public function testExistingPlayerCannotRejoinAsSpectator(ApiTester $I): void
250+
{
251+
// Create admin and room with spectators allowed
252+
$I->createAdminAndUpdateHeaders($I);
253+
$I->sendPostAsJson('room');
254+
255+
$room = $I->grabEntityFromRepository(Room::class, ['name' => 'Admin\'s room']);
256+
257+
// Enable spectators
258+
$I->sendPatchAsJson(sprintf('/room/%s', $room->getId()), ['allowSpectators' => true]);
259+
260+
// First join as regular player
261+
$I->createPlayerAndUpdateHeaders($I, 'RegularPlayer');
262+
$I->sendPatchAsJson('/user', ['room' => $room->getId()]);
263+
$I->seeResponseCodeIsSuccessful();
264+
265+
// Verify player is ALIVE (not spectating)
266+
$I->seeInRepository(Player::class, [
267+
'name' => 'RegularPlayer',
268+
'status' => PlayerStatus::ALIVE,
269+
]);
270+
271+
// Try to rejoin the same room as spectator (should keep existing player, not change status)
272+
$I->sendPatchAsJson('/user', ['room' => $room->getId(), 'spectate' => true]);
273+
$I->seeResponseCodeIsSuccessful();
274+
275+
// Player should still be ALIVE since they already exist in the room
276+
$I->seeInRepository(Player::class, [
277+
'name' => 'RegularPlayer',
278+
'status' => PlayerStatus::ALIVE,
279+
]);
239280
}
240281
}

tests/Unit/Infrastructure/Security/Voters/RoomVoterSpectateTest.php

Lines changed: 0 additions & 152 deletions
This file was deleted.

0 commit comments

Comments
 (0)