Updating Game List to Display Players Username

[Hakaabi]

Issue number

Relevant issue number

• Resolves [Feature]: Replace the player counts in the Home Page with the player usernames #1103

Please check the following

• Do the tests still pass? (see Run the Tests)
• Is the code formatted properly? (see Linting (Formatting))
• For New Features:
  • Have tests been added to cover any new features or fixes?
  • Has the documentation been updated accordingly?

Please describe additional details for testing this change

This pull request addresses the issue of replacing the player counts in the home page of the game with the players' usernames.

The following changes were made:
Backend
Changes Made:

1. Updated the findOpenGames function in api/hooks/customGameHook/index.js:

Transformed the players array using the map() method to retain only id and username.
Filtered games to include only those with fewer than 2 players.
Enhanced error handling to check for empty game results.
Maintained all other game properties while transforming the players array.
Screen Shot 2024-12-11 at 12 45 36 AM
Testing:

Unit tests were created and executed to validate
The unit tests ensure consistent behavior and compatibility with the frontend requirements.

2.Updated the findSpectatableGames helper under the path api/helpers/find-spectatable-games.js:

Added functionality to populate players in the query to ensure that each game includes player details.
The players' data is transformed so that only the id and username fields are returned, removing the count.

Testing:
Created a new file game-list.spec.js under the path tests/unit/specs/sails/ to unit tests the helper findSpectatbleGames(), specifically created the file to test backend changes in the file api/helpers/find-spectatable-games.js. But then deleted this file because cuttle focuses on testing end to end in cypress, meaning it’s the code is tested through the front and backends simultaneously

Added some test cases in test/e2e/out-of-game/home.spec.js to ensure that in the home page player counts is now shown as players username

Frontend

1. Updated the GameSummary constructor to use the players array itself instead of creating a numPlayers attribute using the length of the players list.
   Updated the joinGame() and otherLeftGame() attributes to .push() and .pop() players from the relevant game’s .players() array rather than incrementing/decrementing the player count.

Unit tests were created and executed to validate
The unit tests ensure consistent behavior and compatibility with the backend requirements.

2. Updates the GameListItem.vue component file in the frontend to ensure that it expects the game to have an array of players and that on the home page's list of open games, it renders text displaying who specifically is in the game (players' usernames) based on the number of players in the game.
   In GameListItem.vue:

Props: Added a ‘players’ prop to the component, which is a required array
Computed Property: Added a ‘playersText’ computed property to generate the text based on the number of players in the game.
Template: Updated the template to use the ‘playersText’ computed property to display the text describing who specifically is in the game.
Testing:

Modified the tests/e2e/specs/out-of-game/home.spec.js file to account for the modified GameListItem.vue component. Included test cases to display "Empty" when there are no players and the displaying of the players' usernames in the game list item.

Testing
Modifications Made to the Files
Backend Unit Testing:
Added and updated unit tests to ensure the correctness of backend logic:
tests/unit/specs/customGameHook.spec.js: Validated changes to the findOpenGames method, ensuring proper transformation and filtering of game data. - for api/hooks/customGameHook/index.js
tests/unit/specs/findSpectatableGames.spec.js: Tested the findSpectatableGames helper to confirm it accurately returns player details in the transformed game data. - for api/helpers/ find-spectatable-games'

Screenshot 2024-12-12 at 11 27 16
End-to-End Testing:
Wrote Cypress E2E tests to simulate real user interactions, focusing on player details displayed in game list items:
Verified functionality of displaying "Empty," single-player names, and multiplayer names in the game list.
Tested user interactions like joining games, handling full lobbies, and spectating games.
Added .only() to specific Cypress test cases during debugging to focus on the new functionality.

Screenshot 2024-12-12 at 11 45 39
Integration:
Fetched teammate changes from the backend and frontend, resolving conflicts to integrate the following:
Backend Updates: Refactored findOpenGames and findSpectatableGames to include player details.
Frontend Updates: Modified GameListItem.vue and gameList.js to dynamically handle and display player information in the UI.

[itsalaidbacklife]

This works and does what the issue described, but can we actually refactor it to use the single-valued attributes of p0 and p1 and to then combine them and transform them into an array?

Right now the Game model has:
p0: the user who is player 0
p1: the user who is player 1
players: the collection of [p0, p1].

players is redundant and being deprecated for removal. The main reason for this is that at the database level, the players collection works by having the user table use a foreign key to the game table, which therefore imposes the constraint that a user can only be in one game at a time (something we'd like to change). Conversely, p0 and p1 are columns in the game table which are foreign keys to the user table, allowing any given user to be in as many games as desired. We plan to refactor the entire codebase to remove the use of .players from the backend and replace it with .p0 and .p1.

As one additional note, when using Array.map(), we generally avoid mutating the original array as this can have unintended consequences. In this case we are producing the list within the function and transforming it just to return so it's not a horrible thing to do, but generally it's preferred to copy the object instead (usually with spread) rather than doing something like game.players = inside the .map().

In this situation, that would look like this

Suggested change

	}).populate('players');
	// Map players to include only id and username
	const transformedGames = games.map(game => {
	game.players = game.players.map(player => ({
	id: player.id,
	username: player.username,
	}));
	return game;
	}).populate('p0')
	.populate('p1');
	// Map players to include only id and username
	const transformedGames = games.map(game => {
	const players = [game.p0, game.p1].filter((player) => !!player)
	.map(player => ({
	id: player.id,
	username: player.username,
	}));
	const transformedGame = {
	...game,
	players,
	};
	return transformedGame;

[itsalaidbacklife]

Good catch; thank you

[itsalaidbacklife]

Suggested change

	},
	},

[itsalaidbacklife]

We can combine these with optional chaining and utilize the fact that 0 is falsey like so

Suggested change

	} else if (!games || games.length === 0) {
	} else if (!games?.length) {

[itsalaidbacklife]

Looks good. I think the above comments are helpful but the ones on the same line are not adding much clarity beyond what the line already says. Let's remove these.

Generally, our approach to commenting is to either:

1. Explain anything weird
2. Break out a larger thing into sections for quick scanning

Suggested change

	id: player.id, // Use 'id' since Sails.js generates it automatically
	username: player.username,
	}));
	return game; // Return the updated game object
	});
	return resolve(openGames); // Resolve with transformed open games
	id: player.id,
	username: player.username,
	}));
	return game;
	});
	return resolve(openGames);

[itsalaidbacklife]

I think this comment is sufficiently explained by the one above the openGames assignment

Suggested change

	.filter(({ players }) => players.length < 2) // Keep games with fewer than 2 players
	.filter(({ players }) => players.length < 2)

[itsalaidbacklife]

We should also remove numPlayers as a prop now that we can just use this.players.length

[itsalaidbacklife]

Let's also remove numPlayers above since its just computed from players.length

[itsalaidbacklife]

We will additionally need to update the places numPlayers are referenced and remove them accordingly

[itsalaidbacklife]

This is actually a little trickier than you have it here and will require a full stack change. Specifically, we don't know which player left, so we can't just assume it was the one at the end of the array. We need to update

api/controllers/game/leave-lobby.js

So that its sails.sockets.blast('leftGame', { id: values[0].id }, req); call additionally specifies which user id left and then we need to update this otherLeftGame method to look up which specific user left the game and remove that one. It could look like this

    otherLeftGame(gameId, userId) {
      const updatedGame = this.openGames.find((game) => game.id === gameId);
      if (updatedGame) {
        updatedGame.players. = updatedGame.players.filter((player) => player.id !== userId);
      }
  }

[itsalaidbacklife]

Overall this looks right. I've commented with some change requests. Two things to consider:

1. This PR, along with Modified the GameListItem.vue component to render the players' usernames in the home page list of games #1128 and all the other changes necessary to complete [Feature]: Replace the player counts in the Home Page with the player usernames #1103 need to be consolidated into a single PR from your fork into the original repo. This is because merging a PR into the main branch of the original repo immediately builds and deploys the result to production and the separate pieces will not work if merged individually. I fully understand wanting to work in small chunks, and also to get feedback along the way, so I am happy to review these individually as I have been doing, but it's important to understand that all of your team's changes need to be merged into a single branch which is PR'd into the original repo, or they can't be used.
2. The otherLeftGame update is a little more complicated than what you've done so far because we need to look up which specific user left the game and therefore to update the backend to send that information in the socket event so that the frontend can remove the right user. Details on how to do this are in my comments.

Overall this is solid work that gets us close to what we need