Resolve "Expose leave game endpoint" - [merged]

[paulmis]

Merges 123-expose-leave-game-endpoint -> dev

Closes #123. Somehow there were lots of test for this.

This also changes the local postgres port to 5555 which should fix some issues some of you had when already having the db installed. Remember to rebuild the container with:

cd <repo>
docker-compose down
docker-compose up -d

[paulmis]

requested review from @rstular and @ddinucujianu

[paulmis]

marked this merge request as draft

[paulmis]

marked this merge request as ready

[paulmis]

E2E in progress, but please review already.

[paulmis]

added 1 commit

• a23bd646 - fix GameControllerTest::leaveConflict

Compare with previous version

[paulmis]

In GitLab by @rstular on Mar 14, 2022, 14:33

Commented on server/src/main/java/server/services/GameService.java line 94

This should close the user's SSE listener as well.

[paulmis]

In GitLab by @rstular on Mar 14, 2022, 14:33

Commented on server/src/main/java/server/database/repositories/game/GameRepository.java line 36

You could replace this with Optional<Game> findByPlayers_User_IdEqualsAndStatusAndPlayers_Abandoned(UUID id, GameStatus status, boolean abandoned); instead.

[paulmis]

In GitLab by @rstular on Mar 14, 2022, 14:33

Commented on server/src/main/java/server/database/entities/game/GamePlayer.java line 40

The findByPlayers_User_IdEqualsAndStatus method in GameRepository should be changed to findByPlayers_User_IdEqualsAndStatusAndPlayers_Abandoned.

As of now, all abandoned players won't be detected as such, as the old method fails to account for the abandoned GamePlayer field.

[paulmis]

In GitLab by @rstular on Mar 14, 2022, 14:33

Commented on server/src/main/java/server/database/entities/game/Game.java line 178

This can be replaced with .orElseThrow(LastPlayerRemovedException::new), and the subsequent null check can be removed.

[paulmis]

Can you link the docs on how that's done?

[paulmis]

Yeah I did initially but this is literally longer than the line character limit and requires 2 arguments that are invariable, so I think this is better.

[paulmis]

It can't because then the host isn't removed.

[paulmis]

I think we're using that one for lobbies only, and players can't abandon lobbies (they are completely removed instead). I think it'd be better to rename that function instead.

[paulmis]

In GitLab by @rstular on Mar 14, 2022, 15:00

Commented on server/src/main/java/server/services/GameService.java line 94

In the SSEManager, first get(UUID userId) the emitter for the user, call .complete() on the emitter, and then call SSEManager.unregister(UUID userId).

I think it would make sense to add a convenience method for that to the SSEManager class.

[paulmis]

In GitLab by @rstular on Mar 14, 2022, 15:00

Commented on server/src/main/java/server/database/repositories/game/GameRepository.java line 36

Fair enough

[paulmis]

In GitLab by @rstular on Mar 14, 2022, 15:03

Commented on server/src/main/java/server/database/entities/game/GamePlayer.java line 40

It's used here as well, this usage probably needs to account for abandoned status.

[paulmis]

In GitLab by @rstular on Mar 14, 2022, 15:04

Commented on server/src/main/java/server/database/entities/game/Game.java line 178

I see, my bad.

[paulmis]

Changed the function in SSEManager and added description to findByPlayers_User_IdEqualsAndStatus.

[paulmis]

Added.

[paulmis]

added 1 commit

• 990c8051 - add SSE to GameService, modify db to run on 5555

Compare with previous version

[paulmis]

added 1 commit

• 29a54939 - change CI port

Compare with previous version

[paulmis]

added 1 commit

• b5635447 - add SPRING_PROPS_POSTGRES_PORT env variable

Compare with previous version

[paulmis]

added 1 commit

• 0bb9085f - set CI postgres port to 5432

Compare with previous version

[paulmis]

added 71 commits

• 0bb9085f...23f99430 - 59 commits from branch dev
• 6d1a060a - add lobby leave endpoint
• 015c03c2 - add game leave endpoint
• 48dede6f - change CI db user
• 182b8281 - add lobby leave tests
• 3a573431 - add game, game leave tests
• 14b65aed - fix GameControllerTest::leaveConflict
• c7be0356 - add SSE to GameService, modify db to run on 5555
• 7b94c9eb - change CI port
• 0d9f9e1f - add SPRING_PROPS_POSTGRES_PORT env variable
• 846ac69d - set CI postgres port to 5432
• f8ae979a - fix rebase
• dfe61c0c - Merge remote-tracking branch 'origin/123-expose-leave-game-endpoint' into...

Compare with previous version

[paulmis]

In GitLab by @rstular on Mar 16, 2022, 06:01

As for the "get new container" instructions, I think using docker-compose down is more appropriate, killing seems a bit harsh (and unnecessary, as down both stops and removes containers and networks).

[paulmis]

In GitLab by @rstular on Mar 16, 2022, 06:02

resolved all threads

[paulmis]

In GitLab by @rstular on Mar 16, 2022, 06:03

resolved all threads

[paulmis]

Changed.

[paulmis]

In GitLab by @rstular on Mar 16, 2022, 06:12

Commented on server/src/test/java/server/api/LobbyControllerTest.java line 345

What exactly is being tested here?

[paulmis]

Apparently nothing :shrug:

[paulmis]

In GitLab by @rstular on Mar 16, 2022, 06:25

Commented on server/src/test/java/server/api/LobbyControllerTest.java line 345

Yeet

[paulmis]

added 1 commit

• 47fc4dfe - fix missing request

Compare with previous version

[paulmis]

Fixed.

[paulmis]

In GitLab by @ddinucujianu on Mar 17, 2022, 02:03

Commented on server/src/main/java/server/services/GameService.java line 97

Could this also announce every player in the game that somebody left? Like just an event with playerLeft or something like that? Would work nicely to update the top bar leaderboard quickly or even display a short message.

[paulmis]

In GitLab by @gpezzali on Mar 17, 2022, 11:36

Commented on docker-compose.yml line 11

Why is there a minus sign here and not in application.yml?

[paulmis]

Same format, different parsing rules. docker-compose's default argument is preceded by a -.

[paulmis]

added 1 commit

• 3feb0116 - disconnect SSE when leaving the game

Compare with previous version

[paulmis]

Added to the ongoing leave endpoint.

[paulmis]

In GitLab by @ddinucujianu on Mar 17, 2022, 13:56

Commented on server/src/main/java/server/services/GameService.java line 111

I don't think this can work with the current set-up on the client can you do something like:

SseEmitter.SseEventBuilder event = SseEmitter.event()
                    .id("1")
                    .name("playerLeft").data(user.getId());
            game.getEmitters().sendAll(event);

Putting the player id in the data is optional cause we can just query the whole database but yeah.

[paulmis]

In GitLab by @gpezzali on Mar 17, 2022, 14:37

Commented on server/src/main/java/server/api/GameController.java line 45

Every other lobby/game-related endpoint takes the game id as path variable, it would be nice to keep them coherent

[paulmis]

The game can be derived from the user. This has the benefit of having automatic verification (i.e. no need to check if the player is in the specified game).

[paulmis]

In GitLab by @gpezzali on Mar 17, 2022, 14:56

Commented on server/src/test/java/server/services/GameServiceTest.java line 70

Should use getUUID

[paulmis]

In GitLab by @gpezzali on Mar 17, 2022, 14:59

Commented on server/src/test/java/server/services/LobbyServiceTest.java line 46

This should use TestHelpers::getUUID too

[paulmis]

changed this line in version 10 of the diff

[paulmis]

added 1 commit

• 6bce3cc9 - change playerLeave SSE event format

Compare with previous version

[paulmis]

Changed.

[paulmis]

changed this line in version 11 of the diff

[paulmis]

added 1 commit

• c10ba69b - switch from UUID.fromString to TestHelpers::getUUID

Compare with previous version

[paulmis]

In GitLab by @ddinucujianu on Mar 17, 2022, 15:06

Looks good

[paulmis]

In GitLab by @ddinucujianu on Mar 17, 2022, 15:07

approved this merge request