Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

snake and ladder game fix. #253

Open
wants to merge 6 commits into
base: master
from

Conversation

@RohanJnr
Copy link
Contributor

commented Aug 8, 2019

closes #252

fixed the issue of the game not stopping.
Now the server mods also can step in and stop the game.

bot/seasons/evergreen/snakes/utils.py Outdated Show resolved Hide resolved
@sco1
Copy link
Member

left a comment

This does not fully fix the issue, there is still are still missing permission checks on the reaction add:

elif reaction.emoji == CANCEL_EMOJI:
if self.ctx.author == user:
await self.cancel_game(user)
return
else:
await self.player_leave(user)

elif reaction.emoji == CANCEL_EMOJI:
if self.ctx.author == user:
await self.cancel_game(user)
return
else:
await self.player_leave(user)

This needs to account for the possibility of the moderator being part of the game & just wants to leave rather than cancel the entire game.

edit: I don't see any reason why there needs to be a user check in cancel_game at all, it's only ever invoked via reaction or via timeout. The permission check only needs to happen when a user clicks the reaction.

@RohanJnr

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

This does not fully fix the issue, there is still are still missing permission checks on the reaction add:

elif reaction.emoji == CANCEL_EMOJI:
if self.ctx.author == user:
await self.cancel_game(user)
return
else:
await self.player_leave(user)

elif reaction.emoji == CANCEL_EMOJI:
if self.ctx.author == user:
await self.cancel_game(user)
return
else:
await self.player_leave(user)

This needs to account for the possibility of the moderator being part of the game & just wants to leave rather than cancel the entire game.

edit: I don't see any reason why there needs to be a user check in cancel_game at all, it's only ever invoked via reaction or via timeout. The permission check only needs to happen when a user clicks the reaction.

the code is using the same reaction to close game or to exit from the game if not author, should we add another reaction for leaving? because if remove that user check, then if a mod is playing and wants to leave,he/she will end up closing the game.

@lemonsaurus

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

The game should not end just because the author leaves. What if there are 3 other players and they want to keep playing?

Let's instead have the game end only if one of these two conditions are met:

  1. Game has timed out.
  2. There is only one are no players left in the game.
@sco1

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

because if remove that user check, then if a mod is playing and wants to leave,he/she will end up closing the game

Correct, which is why I said there needs to be logic to account for this. You have a list of players you can reference to see if the mod is part of the game or not.

@lemonsaurus

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Okay, keeping in mind the mod idea, a third condition for closing the game occurs:

  1. Game has timed out.
  2. There is only one are no players left in the game.
  3. A moderator clicks on the cancel button and is not currently playing the game. If the moderator is playing, they will simply leave the game.
@SebastiaanZ

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

2. There is only one player left. The game cannot be played with less than 2 players.

You can currently start it with only one player, too. If we want to prohibit people playing it by themselves, we should prevent the game from starting with a single player, too. (Since I assume that we'll only check for a stop condition when someone leaves.)

Anyway, if people play it by themselves, what harm does it do? Why not just stop the game when it drops to zero players?

@lemonsaurus

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

🤷‍♂ I don't have a good reason why we shouldn't allow people to play solo, so let's allow that I guess.

@RohanJnr

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

🤷‍♂ I don't have a good reason why we shouldn't allow people to play solo, so let's allow that I guess.

who plays sal alone?

@SebastiaanZ

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

man_shrugging I don't have a good reason why we shouldn't allow people to play solo, so let's allow that I guess.

who plays sal alone?

People that are trying out the commands to see what they are about. It would kinda suck if you can't see what it looks like because no one else wants to join you at that time.

@sco1 sco1 added this to Needs review in Seasonalbot Tracking Aug 13, 2019

@RohanJnr

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

I am going to close this now and reopen it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.